diff --git a/layers/artifact-delivery/server/utils/github-read-credential.ts b/layers/artifact-delivery/server/utils/github-read-credential.ts index 46a49caf..4c836c5b 100644 --- a/layers/artifact-delivery/server/utils/github-read-credential.ts +++ b/layers/artifact-delivery/server/utils/github-read-credential.ts @@ -1,4 +1,4 @@ -import type { PublicGithubSourceClient } from './github-source' +import type { LoadSourceResult, PublicGithubSourceClient, ResolveSourceResult } from './github-source' import { requestInstallationToken, signGithubAppJwt } from './github-app' /** @@ -44,7 +44,7 @@ export interface ArtifactGithubCredentialEnv { /** Why a build read with a fallback token. It names secrets, never a value. */ export interface ArtifactGithubCredentialReport { - outcome: 'app-misconfigured' | 'app-token-unavailable' + outcome: 'app-misconfigured' | 'app-token-unavailable' | 'app-denied' reason: string fallback: FallbackToken['name'] | 'anonymous' } @@ -76,8 +76,18 @@ export interface ArtifactGithubCredentialRuntime { report: (event: ArtifactGithubCredentialReport) => void } -/** The credential for one GitHub read. Undefined reads anonymously. */ -export type ArtifactGithubCredential = () => Promise +/** The credential for one GitHub read. An undefined token reads anonymously. */ +export interface GithubReadCredential { + token: string | undefined + /** The read App's installation token, which a Repository can deny. */ + isApp: boolean +} + +export interface ArtifactGithubCredential { + current: () => Promise + /** The token after the read App, or null. */ + fallback: FallbackToken | null +} export function parseArtifactGithubCredentialConfig(env: ArtifactGithubCredentialEnv): ArtifactGithubCredentialConfig { const fallback = fallbackToken(env) @@ -110,48 +120,77 @@ export function createArtifactGithubCredential( runtime: ArtifactGithubCredentialRuntime, ): ArtifactGithubCredential { if (config._tag === 'anonymous') - return async () => undefined + return { current: async () => ({ token: undefined, isApp: false }), fallback: null } if (config._tag === 'token') - return async () => config.token.token + return { current: async () => ({ token: config.token.token, isApp: false }), fallback: null } const fallbackName = config.fallback?.name ?? 'anonymous' + const fallback = { token: config.fallback?.token, isApp: false } if (config._tag === 'app-misconfigured') { - return async () => { - runtime.report({ outcome: 'app-misconfigured', reason: config.reason, fallback: fallbackName }) - return config.fallback?.token + return { + current: async () => { + runtime.report({ outcome: 'app-misconfigured', reason: config.reason, fallback: fallbackName }) + return fallback + }, + fallback: null, } } const app = config.app const key = `${app.appId}:${app.installationId}` - return async () => { - const cached = runtime.tokenCache.tokens.get(key) - if (cached && cached.expiresAt - INSTALLATION_TOKEN_RENEW_SECONDS > runtime.now()) - return cached.token - let pending = runtime.tokenCache.pending.get(key) - if (!pending) { - pending = mintInstallationToken(app, runtime).finally(() => runtime.tokenCache.pending.delete(key)) - runtime.tokenCache.pending.set(key, pending) - } - const minted = await pending - if ('token' in minted) { - runtime.tokenCache.tokens.set(key, minted) - return minted.token - } - runtime.report({ outcome: 'app-token-unavailable', reason: minted.reason, fallback: fallbackName }) - return config.fallback?.token + return { + current: async () => { + const cached = runtime.tokenCache.tokens.get(key) + if (cached && cached.expiresAt - INSTALLATION_TOKEN_RENEW_SECONDS > runtime.now()) + return { token: cached.token, isApp: true } + let pending = runtime.tokenCache.pending.get(key) + if (!pending) { + pending = mintInstallationToken(app, runtime).finally(() => runtime.tokenCache.pending.delete(key)) + runtime.tokenCache.pending.set(key, pending) + } + const minted = await pending + if ('token' in minted) { + runtime.tokenCache.tokens.set(key, minted) + return { token: minted.token, isApp: true } + } + runtime.report({ outcome: 'app-token-unavailable', reason: minted.reason, fallback: fallbackName }) + return fallback + }, + fallback: config.fallback, } } /** * A GitHub source client whose every read asks for the current credential, so * a long-lived client never reads with an expired installation token. + * + * Some organizations deny the read App on a public Repository that a personal + * token reads: `neondatabase/agent-skills` answered SOURCE_ACCESS_DENIED to + * every run on 2026-10-06. A read the App was denied repeats once with the + * fallback token, and the denial is reported. */ export function withGithubCredential( credential: ArtifactGithubCredential, create: (token: string | undefined) => PublicGithubSourceClient, + report: (event: ArtifactGithubCredentialReport) => void, ): PublicGithubSourceClient { + const read = async ( + repository: { owner: string, repository: string }, + run: (client: PublicGithubSourceClient) => Promise, + ): Promise => { + const current = await credential.current() + const first = await run(create(current.token)) + const fallback = credential.fallback + if (!current.isApp || !fallback || first._tag !== 'rejected' || first.code !== 'SOURCE_ACCESS_DENIED') + return first + report({ + outcome: 'app-denied', + reason: `GitHub denied the read App for ${repository.owner}/${repository.repository}`, + fallback: fallback.name, + }) + return await run(create(fallback.token)) + } return { - resolve: async request => await create(await credential()).resolve(request), - load: async (source, options) => await create(await credential()).load(source, options), + resolve: async request => await read(request, client => client.resolve(request)), + load: async (source, options) => await read(source, client => client.load(source, options)), } } diff --git a/layers/artifact-delivery/server/utils/github-source.ts b/layers/artifact-delivery/server/utils/github-source.ts index 06da8ddf..06d5e309 100644 --- a/layers/artifact-delivery/server/utils/github-source.ts +++ b/layers/artifact-delivery/server/utils/github-source.ts @@ -332,7 +332,8 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi // It used to throw, which wrote nothing: the Resolution sat in its // current state through the delivery retry ladder and then failed as // SERVICE_UNAVAILABLE with no reason on it. - const limited = githubRateLimit(response, now()) + const message = response.status === 403 ? await githubErrorMessage(response) : null + const limited = githubRateLimit(response, now(), message) if (limited) return { _tag: 'settled', value: limited } if (response.status === 401 || response.status === 403) @@ -1392,15 +1393,26 @@ interface GithubRateLimit { resetAt: number | null } +/** The words GitHub uses for a primary limit, a secondary limit, and the older abuse limit. */ +const RATE_LIMIT_MESSAGE = /\brate limit\b|\babuse detection\b/i +const MAX_GITHUB_ERROR_BYTES = 64 * 1024 +const githubErrorSchema = z.object({ message: z.string() }) + /** * GitHub's rate limit answer, or null for any other response. * * GitHub answers a spent primary quota with 403 or 429 and no remaining - * requests, and a secondary limit with 403 or 429 and Retry-After. A 429 is - * always a limit. A 403 with neither header denies access. + * requests, and a secondary limit with 403 or 429 and a message that names + * it. Retry-After can be missing from a secondary limit's first answers, so + * the message alone decides those. A 429 is always a limit. A 403 that names + * no limit denies access. * https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api */ -export function githubRateLimit(response: Pick, now: number): GithubRateLimit | null { +export function githubRateLimit( + response: Pick, + now: number, + message: string | null, +): GithubRateLimit | null { if (response.status !== 403 && response.status !== 429) return null const retryAfter = epochHeader(response.headers, 'retry-after') @@ -1408,7 +1420,21 @@ export function githubRateLimit(response: Pick, return { _tag: 'rate-limited', resetAt: now + retryAfter } if (response.headers.get('x-ratelimit-remaining') === '0') return { _tag: 'rate-limited', resetAt: epochHeader(response.headers, 'x-ratelimit-reset') } - return response.status === 429 ? { _tag: 'rate-limited', resetAt: null } : null + return response.status === 429 || (message !== null && RATE_LIMIT_MESSAGE.test(message)) + ? { _tag: 'rate-limited', resetAt: null } + : null +} + +/** The `message` of a GitHub error body, or null when the body holds none. */ +async function githubErrorMessage(response: Response): Promise { + const body = await readBoundedJson(response, MAX_GITHUB_ERROR_BYTES).catch(() => { + // A body that does not read or parse names no limit, so the status alone decides. + return null + }) + if (body?._tag !== 'json') + return null + const parsed = githubErrorSchema.safeParse(body.value) + return parsed.success ? parsed.data.message : null } function rateLimitRejection(resetAt: number | null): SourceRejection { diff --git a/layers/artifact-delivery/server/utils/queue.ts b/layers/artifact-delivery/server/utils/queue.ts index 8a0db5fe..074f3e9c 100644 --- a/layers/artifact-delivery/server/utils/queue.ts +++ b/layers/artifact-delivery/server/utils/queue.ts @@ -189,6 +189,7 @@ export function createArtifactGithubSource( return withGithubCredential( credential, token => createPublicGithubSourceClient({ fetch: runtime.fetch, token, onReadFailure: reportGithubReadFailure, cache }), + runtime.report, ) } diff --git a/test/unit/artifact-github-read-app.test.ts b/test/unit/artifact-github-read-app.test.ts index 95f3a5d4..d563e577 100644 --- a/test/unit/artifact-github-read-app.test.ts +++ b/test/unit/artifact-github-read-app.test.ts @@ -124,6 +124,59 @@ describe('the GitHub credential public Artifact builds read with', () => { expect(text).not.toContain('PRIVATE KEY') expect(reports).toHaveLength(1) }) + + 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 }) + const reports: ArtifactGithubCredentialReport[] = [] + const source = createArtifactGithubSource(appEnv(keys.pem), runtime(github, { report: event => reports.push(event) })) + + const result = await source.resolve(request) + + expect(github.readAuthorizations).toEqual(['Bearer ghs_installation_1', 'Bearer runs']) + // The fallback read reached GitHub, which has no such Repository. + expect(result).toMatchObject({ _tag: 'rejected', code: 'SOURCE_NOT_FOUND' }) + expect(reports).toEqual([{ outcome: 'app-denied', reason: 'GitHub denied the read App for skilld-dev/skills', fallback: 'ARTIFACT_GITHUB_TOKEN' }]) + }) + + // GITHUB_TOKEN is a personal token the registry sync also spends. A limit on + // the App is no denial, so it never reaches that token. + it.each([ + ['a secondary limit named only in its message', () => Response.json( + { message: 'You have exceeded a secondary rate limit. Please wait a few minutes before you try again.' }, + { status: 403, headers: { 'x-ratelimit-remaining': '4321' } }, + )], + ['a secondary limit with Retry-After', () => Response.json( + { message: 'You have exceeded a secondary rate limit.' }, + { status: 403, headers: { 'retry-after': '60', 'x-ratelimit-remaining': '4321' } }, + )], + ['a spent quota', () => Response.json( + { message: `API rate limit exceeded for installation ID ${INSTALLATION_ID}.` }, + { status: 403, headers: { 'x-ratelimit-remaining': '0', 'x-ratelimit-reset': String(NOW + 600) } }, + )], + ])('never reads with the fallback token after %s on the read App', async (_, answer) => { + const keys = await appKeys() + const github = fakeGithub({ expiresAt: NOW + 3600, answersApp: answer }) + const reports: ArtifactGithubCredentialReport[] = [] + const source = createArtifactGithubSource(appEnv(keys.pem), runtime(github, { report: event => reports.push(event) })) + + const result = await source.resolve(request) + + expect(github.readAuthorizations).toEqual(['Bearer ghs_installation_1']) + expect(result).toMatchObject({ _tag: 'rejected', code: 'RATE_LIMITED' }) + expect(reports).toEqual([]) + }) + + it('keeps the denial when no token follows the read App', async () => { + const keys = await appKeys() + const github = fakeGithub({ expiresAt: NOW + 3600, answersApp: appDenied }) + const env = { ...appEnv(keys.pem), ARTIFACT_GITHUB_TOKEN: '', GITHUB_TOKEN: '' } + + const result = await createArtifactGithubSource(env, runtime(github)).resolve(request) + + expect(github.readAuthorizations).toEqual(['Bearer ghs_installation_1']) + expect(result).toMatchObject({ _tag: 'rejected', code: 'SOURCE_ACCESS_DENIED' }) + }) }) interface FakeGithub { @@ -132,7 +185,14 @@ interface FakeGithub { readAuthorizations: Array } -function fakeGithub(options: { expiresAt: number, mintStatus?: number }): FakeGithub { +// Some organizations answer the read App 403 on a public Repository that a +// personal token reads: neondatabase/agent-skills on 2026-10-06. +function appDenied(): Response { + return Response.json({ message: 'Resource not accessible by integration' }, { status: 403, headers: { 'x-ratelimit-remaining': '4321' } }) +} + +/** Answers every read 404, and every read with an installation token `answersApp` when set. */ +function fakeGithub(options: { expiresAt: number, mintStatus?: number, answersApp?: () => Response }): FakeGithub { const mints: FakeGithub['mints'] = [] const readAuthorizations: FakeGithub['readAuthorizations'] = [] const fetcher = async (input: RequestInfo | URL, init?: RequestInit): Promise => { @@ -144,7 +204,10 @@ function fakeGithub(options: { expiresAt: number, mintStatus?: number }): FakeGi return new Response('{}', { status: options.mintStatus }) return Response.json({ token: `ghs_installation_${mints.length}`, expires_at: new Date(options.expiresAt * 1000).toISOString() }, { status: 201 }) } - readAuthorizations.push(headers.get('authorization')) + const authorization = headers.get('authorization') + readAuthorizations.push(authorization) + if (options.answersApp && authorization?.startsWith('Bearer ghs_')) + return options.answersApp() return Response.json({ message: 'Not Found' }, { status: 404 }) } return { fetch: fetcher as typeof fetch, mints, readAuthorizations } diff --git a/test/unit/artifact-rate-limit.test.ts b/test/unit/artifact-rate-limit.test.ts index 4ad62d84..46ffe350 100644 --- a/test/unit/artifact-rate-limit.test.ts +++ b/test/unit/artifact-rate-limit.test.ts @@ -161,6 +161,22 @@ describe('gitHub secondary rate limits as values', () => { }) }) + // GitHub can send a secondary limit before its Retry-After, so only the + // message names it. As a denial it failed the run and spent the fallback token. + it('rejects a 403 whose message names a secondary rate limit as RATE_LIMITED', async () => { + const client = createPublicGithubSourceClient({ + fetch: vi.fn(async () => Response.json( + { message: 'You have exceeded a secondary rate limit. Please wait a few minutes before you try again.' }, + { status: 403, headers: { 'x-ratelimit-remaining': '4321' } }, + )) as unknown as typeof fetch, + }) + + const result = await client.resolve(request) + + expect(result).toMatchObject({ _tag: 'rejected', code: 'RATE_LIMITED' }) + expect(result).not.toHaveProperty('retryAfterSeconds') + }) + it('rejects a 429 spent quota with its reset time', async () => { const client = createPublicGithubSourceClient({ fetch: vi.fn(async () => new Response('{}', {