From 756688f9be990e1d282b928e8a83d084d8567937 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 02:27:03 -0700 Subject: [PATCH 1/3] fix(github): use GitHub App tokens for backend GitHub calls App installation tokens are now the default credential for every backend GitHub call. The shared GITHUB_PAT is used only when no App is configured, and that fallback is logged. skills-refresh mints one App token per run. Chart: the backend, ops-runner and cloud-codex no longer receive the GITHUB_PAT env. The clawdbot gateway is unchanged (it retires with ADR-021). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VD3azQJZrdMmw8g51iscWD --- .../githubAppService.apiHeaders.test.js | 70 +++++++++++++++++++ backend/routes/github.ts | 6 +- backend/services/githubAppService.ts | 44 ++++++------ backend/services/skillsRefreshService.ts | 35 +++++++--- .../agents/cloud-codex-deployment.yaml | 49 ++----------- .../templates/core/backend-deployment.yaml | 6 -- .../templates/ops/ops-runner-deployment.yaml | 6 -- k8s/helm/commonly/values-staging.yaml | 4 +- k8s/helm/commonly/values.yaml | 10 +-- 9 files changed, 134 insertions(+), 96 deletions(-) create mode 100644 backend/__tests__/unit/services/githubAppService.apiHeaders.test.js diff --git a/backend/__tests__/unit/services/githubAppService.apiHeaders.test.js b/backend/__tests__/unit/services/githubAppService.apiHeaders.test.js new file mode 100644 index 0000000000..2430dfb75e --- /dev/null +++ b/backend/__tests__/unit/services/githubAppService.apiHeaders.test.js @@ -0,0 +1,70 @@ +// Credential precedence for backend GitHub calls: the App installation token is +// the default, and the shared GITHUB_PAT is used only when no App is configured. +const GitHubAppService = require('../../../services/githubAppService'); + +const ENV_KEYS = [ + 'GITHUB_APP_ID', + 'GITHUB_APP_PRIVATE_KEY', + 'GITHUB_APP_INSTALLATION_ID_COMMONLY', + 'GITHUB_PAT', +]; + +describe('GitHubAppService._apiHeaders credential precedence', () => { + const saved = {}; + let mint; + let warn; + + beforeEach(() => { + ENV_KEYS.forEach((k) => { saved[k] = process.env[k]; delete process.env[k]; }); + mint = jest.spyOn(GitHubAppService, 'getInstallationToken') + .mockResolvedValue({ token: 'app-installation-token', expiresAt: null }); + warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + }); + + afterEach(() => { + ENV_KEYS.forEach((k) => { + if (saved[k] === undefined) delete process.env[k]; else process.env[k] = saved[k]; + }); + jest.restoreAllMocks(); + }); + + const configureApp = () => { + process.env.GITHUB_APP_ID = '1'; + process.env.GITHUB_APP_PRIVATE_KEY = 'pem'; + process.env.GITHUB_APP_INSTALLATION_ID_COMMONLY = '42'; + }; + + it('uses the App installation token when both the App and GITHUB_PAT are configured', async () => { + configureApp(); + process.env.GITHUB_PAT = 'shared-pat'; + const headers = await GitHubAppService._apiHeaders(); + expect(headers.Authorization).toBe('Bearer app-installation-token'); + expect(mint).toHaveBeenCalledWith('42'); + expect(warn).not.toHaveBeenCalled(); + }); + + it('mints for an explicit installationId over the default one', async () => { + configureApp(); + await GitHubAppService._apiHeaders('99'); + expect(mint).toHaveBeenCalledWith('99'); + }); + + it('falls back to GITHUB_PAT only when no App is configured, and logs it', async () => { + process.env.GITHUB_PAT = 'shared-pat'; + const headers = await GitHubAppService._apiHeaders(); + expect(headers.Authorization).toBe('Bearer shared-pat'); + expect(mint).not.toHaveBeenCalled(); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('falling back to GITHUB_PAT')); + }); + + it('never falls back to the PAT for an app-only request', async () => { + process.env.GITHUB_PAT = 'shared-pat'; + await expect(GitHubAppService._apiHeaders(undefined, true)).rejects.toThrow('github_not_configured'); + await expect(GitHubAppService._apiHeaders('99')).rejects.toThrow('github_not_configured'); + expect(mint).not.toHaveBeenCalled(); + }); + + it('throws when nothing is configured', async () => { + await expect(GitHubAppService._apiHeaders()).rejects.toThrow('github_not_configured'); + }); +}); diff --git a/backend/routes/github.ts b/backend/routes/github.ts index 200e5d3c74..e20aef7d5a 100644 --- a/backend/routes/github.ts +++ b/backend/routes/github.ts @@ -153,8 +153,10 @@ router.get('/status', auth, async (req: AuthReq, res: Res) => { // mapper count; do not add the marker to a networked route. try { if (req.user?.role !== 'admin') return res.status(403).json({ message: 'Admin only' }); - if (GitHubAppService.isPatConfigured()) return res.json({ mode: 'pat', configured: true }); - if (!GitHubAppService.isConfigured()) return res.json({ mode: 'none', configured: false, message: 'Set GITHUB_PAT or GitHub App env vars' }); + if (!GitHubAppService.isConfigured()) { + if (GitHubAppService.isPatConfigured()) return res.json({ mode: 'pat', configured: true }); + return res.json({ mode: 'none', configured: false, message: 'Set GitHub App env vars' }); + } const appJWT = GitHubAppService.generateAppJWT(); return res.json({ mode: 'app', configured: true, appId: process.env.GITHUB_APP_ID, installationId: process.env.GITHUB_APP_INSTALLATION_ID_COMMONLY, jwtGenerated: !!appJWT }); } catch (err) { diff --git a/backend/services/githubAppService.ts b/backend/services/githubAppService.ts index bbdb3adbba..3ada84d75c 100644 --- a/backend/services/githubAppService.ts +++ b/backend/services/githubAppService.ts @@ -176,23 +176,27 @@ class GitHubAppService { // ─── Issues API ────────────────────────────────────────────────────────── /** - * Shared headers for GitHub REST API calls (uses PAT or App token). - * Supplying an installationId is an app-only request. This keeps a future - * broker tool fail-closed even if its caller forgets to pass forceApp. + * Shared headers for GitHub REST API calls. + * + * The App installation token is the default credential: it lives an hour and + * is scoped to the repos the App is installed on. The shared GITHUB_PAT is a + * fallback for instances with no App configured, and using it is logged. + * Supplying an installationId (or forceApp) is an app-only request, so a + * broker tool stays fail-closed even if its caller forgets to pass forceApp. */ - static async _apiHeaders(token?: string, installationId?: string, forceApp = false): Promise> { - const pat = forceApp || installationId ? undefined : (token || process.env.GITHUB_PAT); - let credential = pat; + static async _apiHeaders(installationId?: string, forceApp = false): Promise> { + const resolvedInstallationId = installationId || process.env.GITHUB_APP_INSTALLATION_ID_COMMONLY; const appConfigured = !!( process.env.GITHUB_APP_ID && process.env.GITHUB_APP_PRIVATE_KEY - && (installationId || process.env.GITHUB_APP_INSTALLATION_ID_COMMONLY) + && resolvedInstallationId ); - if (!credential && appConfigured) { - const installation = await this.getInstallationToken( - installationId || process.env.GITHUB_APP_INSTALLATION_ID_COMMONLY as string, - ); - credential = installation.token; + let credential: string | undefined; + if (appConfigured) { + credential = (await this.getInstallationToken(resolvedInstallationId as string)).token; + } else if (!forceApp && !installationId && process.env.GITHUB_PAT) { + console.warn('[github] GitHub App not configured; falling back to GITHUB_PAT'); + credential = process.env.GITHUB_PAT; } if (!credential) throw new Error('github_not_configured'); return { @@ -206,7 +210,7 @@ class GitHubAppService { * List open issues for a repo (excludes pull requests). */ static async listOpenIssues({ owner = 'Team-Commonly', repo = 'commonly', perPage = 20, installationId, forceApp }: ListIssuesOptions = {}): Promise { - const headers = await this._apiHeaders(undefined, installationId, forceApp); + const headers = await this._apiHeaders(installationId, forceApp); const res = await axios.get( `https://api.github.com/repos/${owner}/${repo}/issues?state=open&per_page=${perPage}`, { headers }, @@ -218,7 +222,7 @@ class GitHubAppService { * Create a new GitHub issue. */ static async createIssue({ owner = 'Team-Commonly', repo = 'commonly', title, body, labels, installationId, forceApp }: CreateIssueOptions): Promise { - const headers = await this._apiHeaders(undefined, installationId, forceApp); + const headers = await this._apiHeaders(installationId, forceApp); const payload: Record = { title }; if (body) payload.body = body; if (labels?.length) payload.labels = labels; @@ -234,7 +238,7 @@ class GitHubAppService { * Add a comment to an existing issue. */ static async addIssueComment({ owner = 'Team-Commonly', repo = 'commonly', issueNumber, body, installationId, forceApp }: IssueCommentOptions): Promise { - const headers = await this._apiHeaders(undefined, installationId, forceApp); + const headers = await this._apiHeaders(installationId, forceApp); const res = await axios.post( `https://api.github.com/repos/${owner}/${repo}/issues/${issueNumber}/comments`, { body }, @@ -250,7 +254,7 @@ class GitHubAppService { if (comment) { await this.addIssueComment({ owner, repo, issueNumber, body: comment, installationId, forceApp }); } - const headers = await this._apiHeaders(undefined, installationId, forceApp); + const headers = await this._apiHeaders(installationId, forceApp); const res = await axios.patch( `https://api.github.com/repos/${owner}/${repo}/issues/${issueNumber}`, { state: 'closed' }, @@ -261,7 +265,7 @@ class GitHubAppService { /** Fetch one issue without exposing the server credential to the caller. */ static async getIssue({ owner = 'Team-Commonly', repo = 'commonly', issueNumber, installationId, forceApp }: IssueNumberOptions): Promise { - const headers = await this._apiHeaders(undefined, installationId, forceApp); + const headers = await this._apiHeaders(installationId, forceApp); const res = await axios.get( `https://api.github.com/repos/${owner}/${repo}/issues/${issueNumber}`, { headers }, @@ -271,7 +275,7 @@ class GitHubAppService { /** Fetch one pull request. */ static async getPullRequest({ owner = 'Team-Commonly', repo = 'commonly', pullNumber, installationId, forceApp }: PullRequestOptions): Promise { - const headers = await this._apiHeaders(undefined, installationId, forceApp); + const headers = await this._apiHeaders(installationId, forceApp); const res = await axios.get( `https://api.github.com/repos/${owner}/${repo}/pulls/${pullNumber}`, { headers }, @@ -281,7 +285,7 @@ class GitHubAppService { /** List files changed by one pull request. */ static async listPullRequestFiles({ owner = 'Team-Commonly', repo = 'commonly', pullNumber, installationId, forceApp }: PullRequestOptions): Promise { - const headers = await this._apiHeaders(undefined, installationId, forceApp); + const headers = await this._apiHeaders(installationId, forceApp); const res = await axios.get( `https://api.github.com/repos/${owner}/${repo}/pulls/${pullNumber}/files`, { headers }, @@ -294,7 +298,7 @@ class GitHubAppService { owner = 'Team-Commonly', repo = 'commonly', pullNumber, mergeMethod, commitTitle, commitMessage, installationId, forceApp, sha, }: MergePullRequestOptions): Promise { - const headers = await this._apiHeaders(undefined, installationId, forceApp); + const headers = await this._apiHeaders(installationId, forceApp); const payload: Record = {}; if (sha) payload.sha = sha; if (mergeMethod) payload.merge_method = mergeMethod; diff --git a/backend/services/skillsRefreshService.ts b/backend/services/skillsRefreshService.ts index a4489eccad..03f99e3357 100644 --- a/backend/services/skillsRefreshService.ts +++ b/backend/services/skillsRefreshService.ts @@ -1,5 +1,6 @@ import fs from 'fs'; import path from 'path'; +import GitHubAppService from './githubAppService'; /** * skillsRefreshService @@ -57,18 +58,19 @@ const resolveLocalCatalogPath = (): string => { return path.resolve(__dirname, '../../docs/skills/awesome-agent-skills-index.json'); }; -const buildGitHubHeaders = (): Record => { - const headers: Record = { +// One credential per refresh run. The App installation token is the default +// (an hour is longer than a run); GitHubAppService falls back to GITHUB_PAT only +// when no App is configured. With neither, requests go out unauthenticated at +// the 60/h anonymous limit, which still covers a small catalog. +const buildGitHubHeaders = async (): Promise> => { + const base: Record = { 'User-Agent': 'commonly-skills-refresh', Accept: 'application/vnd.github+json', 'X-GitHub-Api-Version': '2022-11-28', }; - const token = process.env.GITHUB_PAT; - if (token) { - // Fine-grained PATs use `Bearer`; classic PATs accept it too. - headers.Authorization = `Bearer ${token}`; - } - return headers; + if (!GitHubAppService.isConfigured() && !GitHubAppService.isPatConfigured()) return base; + const authed = await GitHubAppService._apiHeaders(); + return { ...base, Authorization: authed.Authorization }; }; const sleep = (ms: number): Promise => new Promise((resolve) => { @@ -119,9 +121,10 @@ const parseRepoString = (repo: unknown): { owner: string; name: string } | null const fetchRepoStats = async ( owner: string, name: string, + headers: Record, ): Promise<{ stars: number | null; forks: number | null; rateLimited: boolean; authFailed: boolean }> => { const url = `${GITHUB_API_BASE}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(name)}`; - const response = await fetch(url, { headers: buildGitHubHeaders() }); + const response = await fetch(url, { headers }); if (response.status === 401 || response.status === 403) { // 403 with x-ratelimit-remaining=0 is the rate-limit signal; distinguish // from an auth failure by checking the header. @@ -213,15 +216,25 @@ export const refreshSkillsIndex = async (): Promise => { const statsByRepo = new Map(); let stopDueToAuth = false; let stopDueToRateLimit = false; + let githubHeaders: Record | null = null; + if (uniqueRepos.size > 0) { + try { + githubHeaders = await buildGitHubHeaders(); + } catch (error) { + stopDueToAuth = true; + errors.push(new Error(`GitHub App token mint failed: ${(error as Error).message}`)); + } + } // Walk repos sequentially with a delay — avoids secondary rate-limit bursts. for (const [key, parsed] of uniqueRepos) { + if (!githubHeaders) break; try { // eslint-disable-next-line no-await-in-loop - const { stars, forks, rateLimited, authFailed } = await fetchRepoStats(parsed.owner, parsed.name); + const { stars, forks, rateLimited, authFailed } = await fetchRepoStats(parsed.owner, parsed.name, githubHeaders); if (authFailed) { stopDueToAuth = true; - errors.push(new Error(`GitHub auth failed for ${key} — check GITHUB_PAT`)); + errors.push(new Error(`GitHub auth failed for ${key} — check the GitHub App credentials`)); break; } if (rateLimited) { diff --git a/k8s/helm/commonly/templates/agents/cloud-codex-deployment.yaml b/k8s/helm/commonly/templates/agents/cloud-codex-deployment.yaml index 434f578506..2f0e1859ea 100644 --- a/k8s/helm/commonly/templates/agents/cloud-codex-deployment.yaml +++ b/k8s/helm/commonly/templates/agents/cloud-codex-deployment.yaml @@ -273,32 +273,11 @@ spec: echo "[cloud-codex] WARNING: COMMONLY_LITELLM_KEY is empty — model calls will 401 at LiteLLM" fi - # If a GitHub PAT is present, wire it into both git's - # credential helper (so `git clone https://github.com/...` and - # `git push` work without prompting) and gh CLI (so `gh pr - # create` authenticates). Idempotent — re-running rewrites the - # same credentials file. Silent no-op when GITHUB_PAT is empty, - # which keeps non-dev tiers safe if the env ever lands without - # the secret. - if [ -n "${GITHUB_PAT:-}" ]; then - # Trim any trailing newline/carriage-return baked into the - # secret value (often present when the PAT was minted via - # `cat token | base64`). git's libcurl refuses to parse a URL - # whose password component contains a newline ("credential - # url cannot be parsed") — verified empirically against the - # live `commonly-github-pat` 2026-05-15. - TRIMMED_GITHUB_PAT=$(printf '%s' "${GITHUB_PAT}" | tr -d '\n\r') - git config --global credential.helper store - mkdir -p /state - printf 'https://x-access-token:%s@github.com\n' "${TRIMMED_GITHUB_PAT}" > /state/.git-credentials - chmod 600 /state/.git-credentials - git config --global credential.helper "store --file=/state/.git-credentials" - git config --global user.name "${GIT_AUTHOR_NAME:-Cody}" - git config --global user.email "${GIT_AUTHOR_EMAIL:-cody@commonly.me}" - echo "[cloud-codex] git credentials seeded from GITHUB_PAT" - else - echo "[cloud-codex] no GITHUB_PAT in env — git/gh operations will require interactive auth" - fi + # No shared GitHub credential is injected here, only the commit + # identity. A run that needs clone/push/PR access must be handed a + # short-lived GitHub App installation token, never a pod-wide PAT. + git config --global user.name "${GIT_AUTHOR_NAME:-Cody}" + git config --global user.email "${GIT_AUTHOR_EMAIL:-cody@commonly.me}" echo "[cloud-codex] config.toml seeded for LiteLLM provider; starting commonly agent run ${COMMONLY_AGENT_NAME}" exec /tools/bin/commonly agent run "${COMMONLY_AGENT_NAME}" @@ -338,24 +317,6 @@ spec: # during initial helm-upgrade before the operator mints one); # the boot script logs a warning and codex 401s at call time. optional: true - # GitHub PAT for clone/push/PR-open. cloud-codex is a dev-only - # runtime tier (per-instance deploys, never community), so it gets - # the same shared `commonly-github-pat` that clawdbot moltbots - # inherit. Codex CLI picks up GITHUB_PAT / GH_TOKEN via shell env - # when it shells out to git or gh during an agent run — no codex - # config knob needed. - - name: GITHUB_PAT - valueFrom: - secretKeyRef: - name: api-keys - key: GITHUB_PAT - optional: true - - name: GH_TOKEN - valueFrom: - secretKeyRef: - name: api-keys - key: GITHUB_PAT - optional: true volumeMounts: - name: tools mountPath: /tools diff --git a/k8s/helm/commonly/templates/core/backend-deployment.yaml b/k8s/helm/commonly/templates/core/backend-deployment.yaml index 8f4fbdd1db..3ebdb481dc 100644 --- a/k8s/helm/commonly/templates/core/backend-deployment.yaml +++ b/k8s/helm/commonly/templates/core/backend-deployment.yaml @@ -352,12 +352,6 @@ spec: - name: OAUTH_CALLBACK_BASE_URL value: {{ .Values.backend.env.oauthCallbackBaseUrl | quote }} {{- end }} - - name: GITHUB_PAT - valueFrom: - secretKeyRef: - name: api-keys - key: GITHUB_PAT - optional: true - name: GITHUB_APP_ID valueFrom: secretKeyRef: diff --git a/k8s/helm/commonly/templates/ops/ops-runner-deployment.yaml b/k8s/helm/commonly/templates/ops/ops-runner-deployment.yaml index e5310b0a72..98b89cb749 100644 --- a/k8s/helm/commonly/templates/ops/ops-runner-deployment.yaml +++ b/k8s/helm/commonly/templates/ops/ops-runner-deployment.yaml @@ -332,12 +332,6 @@ spec: - name: OAUTH_CALLBACK_BASE_URL value: {{ .Values.backend.env.oauthCallbackBaseUrl | quote }} {{- end }} - - name: GITHUB_PAT - valueFrom: - secretKeyRef: - name: api-keys - key: GITHUB_PAT - optional: true - name: GITHUB_APP_ID valueFrom: secretKeyRef: diff --git a/k8s/helm/commonly/values-staging.yaml b/k8s/helm/commonly/values-staging.yaml index 05dcd3198c..c95b9ead99 100644 --- a/k8s/helm/commonly/values-staging.yaml +++ b/k8s/helm/commonly/values-staging.yaml @@ -50,8 +50,8 @@ email: # No prod GitHub PAT on staging. The shared commonly-github-pat has write power over # prod repos, and staging auto-deploys every merge (the reckless lane), so the # api-keys render must not reference it (Cofounder's ruling, same principle as -# keeping atlasAdmin off staging). The backend reads GITHUB_PAT lazily and degrades -# (GitHub routes report not-configured). If staging agents ever need git, that is a +# keeping atlasAdmin off staging). The backend uses GitHub App installation tokens +# and does not read this key. If staging agents ever need git, that is a # separate fine-grained PAT on a throwaway repo, post-11/1. githubPat: enabled: false diff --git a/k8s/helm/commonly/values.yaml b/k8s/helm/commonly/values.yaml index 2123e898ba..f1497542c2 100644 --- a/k8s/helm/commonly/values.yaml +++ b/k8s/helm/commonly/values.yaml @@ -447,11 +447,11 @@ discord: email: enabled: true -# Shared GitHub PAT for agent git operations (api-keys -> GITHUB_PAT). On (default) -# for prod/dev. Off (staging) omits the ref entirely, so the reckless, -# auto-deploy-every-merge lane holds no credential with write power over prod -# repos. The backend reads GITHUB_PAT lazily and degrades (GitHub routes report -# not-configured) rather than crashing when it is absent. +# Shared GitHub PAT (api-keys -> GITHUB_PAT), now consumed only by the clawdbot +# gateway (retires with ADR-021). The backend, ops-runner and cloud-codex no longer +# read it: backend GitHub calls use GitHub App installation tokens. On (default) +# for prod/dev. Off (staging) omits the ref entirely, so the auto-deploy-every-merge +# lane holds no credential with write power over prod repos. githubPat: enabled: true From f8a8fa99d12cb1b4bb3714247a9284eba6b12e33 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 02:49:20 -0700 Subject: [PATCH 2/3] fix(chart): clear the persisted cloud-codex git credential on boot Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VD3azQJZrdMmw8g51iscWD --- .../commonly/templates/agents/cloud-codex-deployment.yaml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/k8s/helm/commonly/templates/agents/cloud-codex-deployment.yaml b/k8s/helm/commonly/templates/agents/cloud-codex-deployment.yaml index 2f0e1859ea..af378a60b2 100644 --- a/k8s/helm/commonly/templates/agents/cloud-codex-deployment.yaml +++ b/k8s/helm/commonly/templates/agents/cloud-codex-deployment.yaml @@ -276,6 +276,10 @@ spec: # No shared GitHub credential is injected here, only the commit # identity. A run that needs clone/push/PR access must be handed a # short-lived GitHub App installation token, never a pod-wide PAT. + # Earlier boots persisted a credential on the /state PVC; clear it on + # every start so an upgraded pod cannot keep authenticating with it. + rm -f /state/.git-credentials + git config --global --unset-all credential.helper || true git config --global user.name "${GIT_AUTHOR_NAME:-Cody}" git config --global user.email "${GIT_AUTHOR_EMAIL:-cody@commonly.me}" From 3d6ac398f639ce1105983cc032f1dac98795a270 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 03:34:55 -0700 Subject: [PATCH 3/3] test(skills-refresh): cover App-token auth; reject an empty installationId Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VD3azQJZrdMmw8g51iscWD --- .../githubAppService.apiHeaders.test.js | 7 ++ .../skillsRefreshService.githubAuth.test.js | 91 +++++++++++++++++++ backend/services/githubAppService.ts | 6 +- 3 files changed, 103 insertions(+), 1 deletion(-) create mode 100644 backend/__tests__/unit/services/skillsRefreshService.githubAuth.test.js diff --git a/backend/__tests__/unit/services/githubAppService.apiHeaders.test.js b/backend/__tests__/unit/services/githubAppService.apiHeaders.test.js index 2430dfb75e..a6620268c4 100644 --- a/backend/__tests__/unit/services/githubAppService.apiHeaders.test.js +++ b/backend/__tests__/unit/services/githubAppService.apiHeaders.test.js @@ -64,6 +64,13 @@ describe('GitHubAppService._apiHeaders credential precedence', () => { expect(mint).not.toHaveBeenCalled(); }); + it('rejects an explicit empty installationId instead of using the default or the PAT', async () => { + configureApp(); + process.env.GITHUB_PAT = 'shared-pat'; + await expect(GitHubAppService._apiHeaders('')).rejects.toThrow('github_not_configured'); + expect(mint).not.toHaveBeenCalled(); + }); + it('throws when nothing is configured', async () => { await expect(GitHubAppService._apiHeaders()).rejects.toThrow('github_not_configured'); }); diff --git a/backend/__tests__/unit/services/skillsRefreshService.githubAuth.test.js b/backend/__tests__/unit/services/skillsRefreshService.githubAuth.test.js new file mode 100644 index 0000000000..072ecabdbc --- /dev/null +++ b/backend/__tests__/unit/services/skillsRefreshService.githubAuth.test.js @@ -0,0 +1,91 @@ +// skills-refresh GitHub auth: one credential per run, minted through +// GitHubAppService (App token by default), a failed mint makes no repo request, +// and with no credential configured the requests go out unauthenticated. +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const GitHubAppService = require('../../../services/githubAppService'); +const { refreshSkillsIndex } = require('../../../services/skillsRefreshService'); + +const jsonResponse = (body) => ({ + ok: true, + status: 200, + statusText: 'OK', + headers: { get: () => null }, + text: async () => JSON.stringify(body), + json: async () => body, +}); + +describe('skillsRefreshService GitHub auth', () => { + let tmpDir; + let savedPath; + let fetchMock; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'skills-refresh-')); + savedPath = process.env.SKILLS_CATALOG_PATH; + process.env.SKILLS_CATALOG_PATH = path.join(tmpDir, 'index.json'); + fetchMock = jest.fn(async (url) => { + if (String(url).startsWith('https://api.github.com/repos/')) { + return jsonResponse({ stargazers_count: 7, forks_count: 1 }); + } + return jsonResponse({ items: [{ repo: 'a/one' }, { repo: 'b/two' }, { repo: 'a/one' }] }); + }); + global.fetch = fetchMock; + jest.spyOn(console, 'log').mockImplementation(() => {}); + jest.spyOn(console, 'warn').mockImplementation(() => {}); + }); + + afterEach(() => { + if (savedPath === undefined) delete process.env.SKILLS_CATALOG_PATH; + else process.env.SKILLS_CATALOG_PATH = savedPath; + fs.rmSync(tmpDir, { recursive: true, force: true }); + delete global.fetch; + jest.restoreAllMocks(); + }); + + const repoCalls = () => fetchMock.mock.calls + .filter(([url]) => String(url).startsWith('https://api.github.com/repos/')); + + it('mints one App credential per run and sends it on every repo request', async () => { + jest.spyOn(GitHubAppService, 'isConfigured').mockReturnValue(true); + jest.spyOn(GitHubAppService, 'isPatConfigured').mockReturnValue(true); + const headers = jest.spyOn(GitHubAppService, '_apiHeaders') + .mockResolvedValue({ Authorization: 'Bearer app-installation-token' }); + + const result = await refreshSkillsIndex(); + + expect(headers).toHaveBeenCalledTimes(1); + expect(repoCalls()).toHaveLength(2); + repoCalls().forEach(([, init]) => { + expect(init.headers.Authorization).toBe('Bearer app-installation-token'); + }); + expect(result.reposUpdated).toBe(2); + }); + + it('stops before any repo request when the credential mint fails', async () => { + jest.spyOn(GitHubAppService, 'isConfigured').mockReturnValue(true); + jest.spyOn(GitHubAppService, '_apiHeaders').mockRejectedValue(new Error('mint refused')); + + const result = await refreshSkillsIndex(); + + expect(repoCalls()).toHaveLength(0); + expect(result.errors.map((e) => e.message)).toContainEqual(expect.stringContaining('mint refused')); + expect(result.reposUpdated).toBe(0); + expect(result.wroteFile).toBe(true); + }); + + it('goes unauthenticated when no App or PAT is configured', async () => { + jest.spyOn(GitHubAppService, 'isConfigured').mockReturnValue(false); + jest.spyOn(GitHubAppService, 'isPatConfigured').mockReturnValue(false); + const headers = jest.spyOn(GitHubAppService, '_apiHeaders'); + + await refreshSkillsIndex(); + + expect(headers).not.toHaveBeenCalled(); + expect(repoCalls()).toHaveLength(2); + repoCalls().forEach(([, init]) => { + expect(init.headers.Authorization).toBeUndefined(); + }); + }); +}); diff --git a/backend/services/githubAppService.ts b/backend/services/githubAppService.ts index 3ada84d75c..19d81a30cc 100644 --- a/backend/services/githubAppService.ts +++ b/backend/services/githubAppService.ts @@ -185,6 +185,10 @@ class GitHubAppService { * broker tool stays fail-closed even if its caller forgets to pass forceApp. */ static async _apiHeaders(installationId?: string, forceApp = false): Promise> { + // An explicit installationId is app-only even when it is empty, so a blank + // value can neither pick the default installation nor reach the PAT. + const appOnly = forceApp || installationId !== undefined; + if (installationId !== undefined && !installationId) throw new Error('github_not_configured'); const resolvedInstallationId = installationId || process.env.GITHUB_APP_INSTALLATION_ID_COMMONLY; const appConfigured = !!( process.env.GITHUB_APP_ID @@ -194,7 +198,7 @@ class GitHubAppService { let credential: string | undefined; if (appConfigured) { credential = (await this.getInstallationToken(resolvedInstallationId as string)).token; - } else if (!forceApp && !installationId && process.env.GITHUB_PAT) { + } else if (!appOnly && process.env.GITHUB_PAT) { console.warn('[github] GitHub App not configured; falling back to GITHUB_PAT'); credential = process.env.GITHUB_PAT; }