diff --git a/docs/ops/artifact-delivery-rollout.md b/docs/ops/artifact-delivery-rollout.md index 7b1c20c3..2a381ef6 100644 --- a/docs/ops/artifact-delivery-rollout.md +++ b/docs/ops/artifact-delivery-rollout.md @@ -119,11 +119,11 @@ Personal tokens are fallbacks only. They repeat a read GitHub denied the App, su 1. Builds try `ARTIFACT_GITHUB_TOKEN`, then `GITHUB_TOKEN`, then the token of the signed-in account that asked for the run. 2. Every other caller tries `GITHUB_TOKEN`. -Each denial emits an `app-denied` event: `github-credential` for registry reads and `artifact-github-credential` for builds. The `github.credential` field names the fallback, or `none`. Without a fallback, the read fails for that Repository only: a build answers `SOURCE_ACCESS_DENIED`, and the sync records a failure without pausing. If the fallback is spent or expired, the sync also records a failure for that Repository without pausing. The sync paces itself on the App's quota, never on the fallback's. +Each denial emits an `app-denied` event: `github-credential` for registry reads and `artifact-github-credential` for builds. The `github.credential` field names the fallback, or `none`. Without a fallback, the read fails for that Repository only: a build answers `SOURCE_ACCESS_DENIED`, and the sync records a failure without pausing. If the fallback is spent or expired, the sync also records a failure for that Repository without pausing. After a denial, the sync paces itself on the App's quota, not on the fallback's. If GitHub answers 401 to the cached installation token, the read mints one new token and repeats. If GitHub rejects the new token too, the read uses the fallback and emits an `app-token-rejected` event. -If the App secrets are set but unusable, reads use the fallback and emit an event with the reason. +If the App secrets are set but unusable, or GitHub refuses to mint a token, every read uses the fallback and emits an event with the reason. The sync then paces itself on the fallback's quota. Keep the read App apart from the `GITHUB_APP_*` secrets below. Those belong to private delivery. Changing `NUXT_TOKEN_KEY` breaks stored GitHub credentials. diff --git a/layers/artifact-delivery/server/utils/queue.ts b/layers/artifact-delivery/server/utils/queue.ts index 1ee18b08..71481590 100644 --- a/layers/artifact-delivery/server/utils/queue.ts +++ b/layers/artifact-delivery/server/utils/queue.ts @@ -163,8 +163,8 @@ export function createKvGithubObjectCache(kv: KVNamespace): GithubObjectCache { * GitHub counts a personal token's quota per account, and the registry sync * and `/gh` page views spend `GITHUB_TOKEN` to zero before each hourly * reset. Every run that needed GitHub in those minutes failed RATE_LIMITED. - * Builds read with the read App's installation token, whose bucket nothing - * else spends, then `ARTIFACT_GITHUB_TOKEN`, then `GITHUB_TOKEN`. + * Builds read with the read App's installation token, whose bucket no + * personal tool spends, then `ARTIFACT_GITHUB_TOKEN`, then `GITHUB_TOKEN`. */ export function createArtifactGithubSource( env: GithubCredentialEnv, diff --git a/shared/server/github-app-credential.ts b/shared/server/github-app-credential.ts index 30be7363..ef1e30d8 100644 --- a/shared/server/github-app-credential.ts +++ b/shared/server/github-app-credential.ts @@ -472,5 +472,12 @@ async function readBoundedJson(response: Response, maximumBytes: number): Promis bytes.set(chunk, offset) offset += chunk.byteLength } - return JSON.parse(new TextDecoder('utf-8', { fatal: true }).decode(bytes)) as unknown + const text = new TextDecoder('utf-8', { fatal: true }).decode(bytes) + // A JSON.parse error quotes the start of the body, which holds the token. + try { + return JSON.parse(text) as unknown + } + catch { + throw new Error('GitHub App returned invalid JSON') + } } diff --git a/test/unit/artifact-github-read-app.test.ts b/test/unit/artifact-github-read-app.test.ts index 7f65b4a9..d712d9d5 100644 --- a/test/unit/artifact-github-read-app.test.ts +++ b/test/unit/artifact-github-read-app.test.ts @@ -125,6 +125,17 @@ describe('the GitHub credential public Artifact builds read with', () => { expect(reports).toHaveLength(1) }) + // A JSON parse error quotes the start of the body, which holds the token. + it('never puts part of a malformed installation token answer in a report', async () => { + const keys = await appKeys() + const github = fakeGithub({ expiresAt: NOW + 3600, mintBody: '{"token":ghs_SECRETVALUE"}' }) + const reports: GithubCredentialReport[] = [] + await createArtifactGithubSource(appEnv(keys.pem), runtime(github, { report: event => reports.push(event) })).resolve(request) + + expect(reports).toHaveLength(1) + expect(JSON.stringify(reports)).not.toContain('ghs_') + }) + it('repeats a read the Repository denied the read App with the fallback token, and reports it', async () => { const keys = await appKeys() const github = fakeGithub({ expiresAt: NOW + 3600, answersApp: appDenied }) @@ -230,7 +241,7 @@ function appDenied(): Response { } /** Answers every read 404, every read with an installation token `answersApp` when set, and 401 to a token in `rejects`. */ -function fakeGithub(options: { expiresAt: number, mintStatus?: number, answersApp?: () => Response, rejects?: string[] }): FakeGithub { +function fakeGithub(options: { expiresAt: number, mintStatus?: number, mintBody?: string, answersApp?: () => Response, rejects?: string[] }): FakeGithub { const mints: FakeGithub['mints'] = [] const readAuthorizations: FakeGithub['readAuthorizations'] = [] const fetcher = async (input: RequestInfo | URL, init?: RequestInit): Promise => { @@ -240,6 +251,8 @@ function fakeGithub(options: { expiresAt: number, mintStatus?: number, answersAp mints.push({ url, authorization: headers.get('authorization') ?? '', body: JSON.parse(String(init?.body)) }) if (options.mintStatus) return new Response('{}', { status: options.mintStatus }) + if (options.mintBody) + return new Response(options.mintBody, { status: 201 }) return Response.json({ token: `ghs_installation_${mints.length}`, expires_at: new Date(options.expiresAt * 1000).toISOString() }, { status: 201 }) } const authorization = headers.get('authorization') diff --git a/test/unit/registry-github-app-credential.test.ts b/test/unit/registry-github-app-credential.test.ts index 3afdf017..0455a82f 100644 --- a/test/unit/registry-github-app-credential.test.ts +++ b/test/unit/registry-github-app-credential.test.ts @@ -195,15 +195,19 @@ describe('a read GitHub answered 401 for the cached installation token', () => { }) it('mints once for reads GitHub rejected together', async () => { - const github = await stubGithub({ rejects: ['ghs_app_1'] }) + const rejects: string[] = [] + const github = await stubGithub({ rejects }) const bindings = resolveGithubBindings(await appEnv({ GITHUB_TOKEN: 'site' }), runtime()) await getRepo('nuxt', 'ui', bindings) + rejects.push('ghs_app_1') github.reads.length = 0 - await Promise.all([getRepo('nuxt', 'ui', bindings), getRepo('nuxt', 'nuxt', bindings), getRepo('nuxt', 'image', bindings)]) + const repos = await Promise.all([getRepo('nuxt', 'ui', bindings), getRepo('nuxt', 'nuxt', bindings), getRepo('nuxt', 'image', bindings)]) + expect(repos.map(repo => repo.status)).toEqual([200, 200, 200]) expect(github.mints).toBe(2) - expect(github.reads.filter(read => read === 'Bearer site')).toEqual([]) + expect(github.reads.filter(read => read === 'Bearer ghs_app_1')).toHaveLength(3) + expect(github.reads).not.toContain('Bearer site') }) it('reads with GITHUB_TOKEN, and reports it, when GitHub rejects the new token too', async () => {