From 0469b3130c953b00006c6d8040c02cfdfdb8f0b5 Mon Sep 17 00:00:00 2001 From: JounQin Date: Sat, 14 Nov 2026 07:36:00 +0000 Subject: [PATCH 1/7] fix: check allowFork with the host gh account --- .changeset/allowfork-auth.md | 6 +++ packages/host/src/commands/host.ts | 62 ++++++++++++++++++++---------- test/unit/host-fork-pr.test.ts | 20 +--------- test/unit/host-fork.test.ts | 17 +++----- 4 files changed, 54 insertions(+), 51 deletions(-) create mode 100644 .changeset/allowfork-auth.md diff --git a/.changeset/allowfork-auth.md b/.changeset/allowfork-auth.md new file mode 100644 index 00000000..33d657fe --- /dev/null +++ b/.changeset/allowfork-auth.md @@ -0,0 +1,6 @@ +--- +"gcb-host": patch +--- + +Check `allowFork` through the host's authenticated `gh` account instead of the unauthenticated +public API, whose shared-egress rate limit silently disabled the fork check. diff --git a/packages/host/src/commands/host.ts b/packages/host/src/commands/host.ts index e05e3a92..f39eb96b 100644 --- a/packages/host/src/commands/host.ts +++ b/packages/host/src/commands/host.ts @@ -69,7 +69,7 @@ import { loadConfigSnapshot, resolveConfigPath } from '../config.ts' import { clearInheritedTokens, setHostToken, tokenKeys } from '../gh-env.ts' import { applyProxyChoice, fetcherFor, parseProxy, type ProxyChoice } from '../http.ts' import { hostConfig, parseAllowEntry, repoAllowed } from '../policy.ts' -import { isForkOfRepository, isPublicRepository } from '../public-repo.ts' +import { isPublicRepository } from '../public-repo.ts' import { ConfigWatcher } from '../reload.ts' const run = promisify(execFile) @@ -801,25 +801,26 @@ async function repoIsPublic( /** * Whether an allowlisted fork's parent is the requested repository. * - * @param host - The forge host. - * @param allow - The configured allowlist entries. - * @param repository - The requested parent repository. - * @param proxy - The proxy choice for the check. - * @param apiBaseUrl - An optional API base. - * @param apiVersion - The API version header. - * @returns True when one allowlisted fork names the repository as its parent. + * The check runs through the host's gh account, so it uses its authenticated quota and the same + * proxy/account configuration as the operation it authorizes. The unauthenticated public API is + * rate-limited by shared egress addresses, which would silently turn allowFork off. */ async function forkAllows( host: string, allow: readonly string[], repository: string, proxy: ProxyChoice, - apiBaseUrl?: string, - apiVersion = '2022-11-28', + command: string, + extra: readonly string[], + account: string | undefined, + cwd: string, ): Promise { - const base = - apiBaseUrl ?? - (host.toLowerCase() === 'github.com' ? 'https://api.github.com' : `https://${host}/api/v3`) + let env: NodeJS.ProcessEnv + try { + env = forkEnv(proxy, account, host, command, extra) + } catch { + return false + } for (const raw of allow) { const entry = parseAllowEntry(raw) if (!entry || entry.owner === '*' || entry.repo === '*') { @@ -830,7 +831,26 @@ async function forkAllows( continue } try { - if (await isForkOfRepository(fetcherFor(proxy), base, fork, repository, apiVersion)) { + const result = await runBounded(command, [...extra, 'api', `repos/${fork}`], { + cwd, + env, + timeoutMs: 10_000, + }) + if (result.code !== 0) { + continue + } + const body = JSON.parse(result.stdout) as { + fork?: unknown + parent?: { full_name?: unknown } | null + source?: { full_name?: unknown } | null + } | null + if (body?.fork !== true) { + continue + } + const names = [body.parent?.full_name, body.source?.full_name] + .filter((name): name is string => typeof name === 'string') + .map(name => name.toLowerCase()) + if (names.includes(repository.toLowerCase())) { return true } } catch { @@ -1303,15 +1323,15 @@ async function openOne( } let allowedByFork = false if (!repoAllowed(hostBlock.allow, request.repo) && hostBlock.allowFork === true) { - const apiBaseUrl = 'apiBaseUrl' in hostBlock ? hostBlock.apiBaseUrl : undefined - const apiVersion = 'apiVersion' in hostBlock ? hostBlock.apiVersion : undefined allowedByFork = await forkAllows( host, hostBlock.allow, request.repo, request.proxy, - apiBaseUrl, - apiVersion, + command, + extra, + request.account, + repo, ) } if (!repoAllowed(hostBlock.allow, request.repo) && !allowedByFork) { @@ -1474,8 +1494,10 @@ async function openOne( block.allow, target, request.proxy, - 'apiBaseUrl' in block ? block.apiBaseUrl : undefined, - 'apiBaseUrl' in block ? block.apiVersion : undefined, + command, + extra, + request.account, + repo, )) if (!allowedByFork) { return { error: `repository ${target} is not in the allowlist for github.com` } diff --git a/test/unit/host-fork-pr.test.ts b/test/unit/host-fork-pr.test.ts index 34526862..496eed4a 100644 --- a/test/unit/host-fork-pr.test.ts +++ b/test/unit/host-fork-pr.test.ts @@ -25,28 +25,12 @@ describe('opening a pull request on a fork parent', () => { const command = join(root, 'not-gh') writeFileSync( command, - '#!/bin/sh\nprintf \'%s\\n\' "$@" > ' + + '#!/bin/sh\nif [ "$1" = api ]; then echo \'{"fork":true,"parent":{"full_name":"vite-hub/vitehub"}}\'; exit 0; fi\nprintf \'%s\\n\' "$@" > ' + argvFile + '\necho "https://github.com/vite-hub/vitehub/pull/9"\n', 'utf8', ) chmodSync(command, 0o755) - const originalFetch = globalThis.fetch - const requested: string[] = [] - globalThis.fetch = (async (input: string | URL | Request) => { - const url = String(input) - requested.push(url) - if (!url.toLowerCase().endsWith('/repos/jounqin/vitehub')) { - throw new Error(`unexpected fork lookup: ${url}`) - } - return new Response( - JSON.stringify({ fork: true, parent: { full_name: 'vite-hub/vitehub' } }), - { - status: 200, - headers: { 'content-type': 'application/json' }, - }, - ) - }) as typeof fetch try { const id = writeRequest(dir, { repo: 'vite-hub/vitehub', @@ -63,9 +47,7 @@ describe('opening a pull request on a fork parent', () => { assert.equal(answer?.number, 9, JSON.stringify(answer)) const argv = readFileSync(argvFile, 'utf8').trim().split('\n') assert.equal(argv[argv.indexOf('--repo') + 1], 'vite-hub/vitehub') - assert.deepEqual(requested, ['https://api.github.com/repos/jounqin/vitehub']) } finally { - globalThis.fetch = originalFetch rmSync(root, { recursive: true, force: true }) } }) diff --git a/test/unit/host-fork.test.ts b/test/unit/host-fork.test.ts index 35617e35..d1da4284 100644 --- a/test/unit/host-fork.test.ts +++ b/test/unit/host-fork.test.ts @@ -44,17 +44,12 @@ it('runs an action on the parent of an allowlisted fork', async () => { ) const dir = channelDir(join(repo, '.git')) const command = join(root, 'not-gh') - writeFileSync(command, '#!/bin/sh\necho "fork action"\n', 'utf8') + writeFileSync( + command, + '#!/bin/sh\nif [ "$1" = api ]; then echo \'{"fork":true,"parent":{"full_name":"Acme/Widget"}}\'; exit 0; fi\necho "fork action"\n', + 'utf8', + ) chmodSync(command, 0o755) - const originalFetch = globalThis.fetch - let requested = '' - globalThis.fetch = (async (input: string | URL | Request) => { - requested = String(input) - return new Response(JSON.stringify({ fork: true, parent: { full_name: 'Acme/Widget' } }), { - status: 200, - headers: { 'content-type': 'application/json' }, - }) - }) as typeof fetch try { const id = writeRequest(dir, { kind: 'action', @@ -68,10 +63,8 @@ it('runs an action on the parent of an allowlisted fork', async () => { 0, ) assert.equal(readResult(dir, id)?.stdout, 'fork action\n') - assert.match(requested, /\/repos\/alice\/widget$/u) assert.equal(existsSync(join(repo, '.git', 'gcb', 'host-broker', `${id}.json`)), false) } finally { - globalThis.fetch = originalFetch rmSync(root, { recursive: true, force: true }) } }) From cf4d8abdb0fe4106dd798c4ea0974b118e4363fd Mon Sep 17 00:00:00 2001 From: JounQin Date: Sat, 14 Nov 2026 07:50:23 +0000 Subject: [PATCH 2/7] fix: require a public fork in allowFork --- .changeset/allowfork-auth.md | 3 ++- packages/host/src/commands/host.ts | 8 +++++++- test/unit/host-fork-pr.test.ts | 2 +- test/unit/host-fork.test.ts | 2 +- 4 files changed, 11 insertions(+), 4 deletions(-) diff --git a/.changeset/allowfork-auth.md b/.changeset/allowfork-auth.md index 33d657fe..aab9388e 100644 --- a/.changeset/allowfork-auth.md +++ b/.changeset/allowfork-auth.md @@ -3,4 +3,5 @@ --- Check `allowFork` through the host's authenticated `gh` account instead of the unauthenticated -public API, whose shared-egress rate limit silently disabled the fork check. +public API, whose shared-egress rate limit silently disabled the fork check. The authenticated response must also +report a public fork, so a private fork cannot widen the allowlist to its upstream. diff --git a/packages/host/src/commands/host.ts b/packages/host/src/commands/host.ts index f39eb96b..ab35a178 100644 --- a/packages/host/src/commands/host.ts +++ b/packages/host/src/commands/host.ts @@ -841,10 +841,16 @@ async function forkAllows( } const body = JSON.parse(result.stdout) as { fork?: unknown + private?: unknown + visibility?: unknown parent?: { full_name?: unknown } | null source?: { full_name?: unknown } | null } | null - if (body?.fork !== true) { + if ( + body?.fork !== true || + body.private !== false || + (typeof body.visibility === 'string' && body.visibility !== 'public') + ) { continue } const names = [body.parent?.full_name, body.source?.full_name] diff --git a/test/unit/host-fork-pr.test.ts b/test/unit/host-fork-pr.test.ts index 496eed4a..d2321982 100644 --- a/test/unit/host-fork-pr.test.ts +++ b/test/unit/host-fork-pr.test.ts @@ -25,7 +25,7 @@ describe('opening a pull request on a fork parent', () => { const command = join(root, 'not-gh') writeFileSync( command, - '#!/bin/sh\nif [ "$1" = api ]; then echo \'{"fork":true,"parent":{"full_name":"vite-hub/vitehub"}}\'; exit 0; fi\nprintf \'%s\\n\' "$@" > ' + + '#!/bin/sh\nif [ "$1" = api ]; then echo \'{"fork":true,"private":false,"visibility":"public","parent":{"full_name":"vite-hub/vitehub"}}\'; exit 0; fi\nprintf \'%s\\n\' "$@" > ' + argvFile + '\necho "https://github.com/vite-hub/vitehub/pull/9"\n', 'utf8', diff --git a/test/unit/host-fork.test.ts b/test/unit/host-fork.test.ts index d1da4284..5ddc7c9c 100644 --- a/test/unit/host-fork.test.ts +++ b/test/unit/host-fork.test.ts @@ -46,7 +46,7 @@ it('runs an action on the parent of an allowlisted fork', async () => { const command = join(root, 'not-gh') writeFileSync( command, - '#!/bin/sh\nif [ "$1" = api ]; then echo \'{"fork":true,"parent":{"full_name":"Acme/Widget"}}\'; exit 0; fi\necho "fork action"\n', + '#!/bin/sh\nif [ "$1" = api ]; then echo \'{"fork":true,"private":false,"visibility":"public","parent":{"full_name":"Acme/Widget"}}\'; exit 0; fi\necho "fork action"\n', 'utf8', ) chmodSync(command, 0o755) From f1d61fb8afd476013fe0824f1ace3c7beb723345 Mon Sep 17 00:00:00 2001 From: JounQin Date: Sat, 14 Nov 2026 07:59:41 +0000 Subject: [PATCH 3/7] fix: require public fork and parent metadata --- packages/host/src/commands/host.ts | 30 ++++--- test/unit/host-fork-pr.test.ts | 129 +++++++++++++++++++++-------- test/unit/host-fork.test.ts | 2 +- 3 files changed, 113 insertions(+), 48 deletions(-) diff --git a/packages/host/src/commands/host.ts b/packages/host/src/commands/host.ts index ab35a178..92d7401a 100644 --- a/packages/host/src/commands/host.ts +++ b/packages/host/src/commands/host.ts @@ -839,24 +839,32 @@ async function forkAllows( if (result.code !== 0) { continue } + interface RepoMeta { + full_name?: unknown + private?: unknown + visibility?: unknown + } const body = JSON.parse(result.stdout) as { fork?: unknown private?: unknown visibility?: unknown - parent?: { full_name?: unknown } | null - source?: { full_name?: unknown } | null + parent?: RepoMeta | null + source?: RepoMeta | null } | null - if ( - body?.fork !== true || - body.private !== false || - (typeof body.visibility === 'string' && body.visibility !== 'public') - ) { + const isPublic = (repo: RepoMeta | null | undefined): boolean => + typeof repo === 'object' && + repo !== null && + repo.private === false && + (typeof repo.visibility !== 'string' || repo.visibility === 'public') + if (body?.fork !== true || !isPublic(body)) { continue } - const names = [body.parent?.full_name, body.source?.full_name] - .filter((name): name is string => typeof name === 'string') - .map(name => name.toLowerCase()) - if (names.includes(repository.toLowerCase())) { + const parent = [body.parent, body.source].find( + (repo): repo is RepoMeta => + typeof repo?.full_name === 'string' && + repo.full_name.toLowerCase() === repository.toLowerCase(), + ) + if (parent !== undefined && isPublic(parent)) { return true } } catch { diff --git a/test/unit/host-fork-pr.test.ts b/test/unit/host-fork-pr.test.ts index d2321982..49820359 100644 --- a/test/unit/host-fork-pr.test.ts +++ b/test/unit/host-fork-pr.test.ts @@ -1,6 +1,14 @@ /** A lifecycle `pr create` on the parent of an allowlisted fork, driven end to end. */ import assert from 'node:assert/strict' -import { chmodSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { + chmodSync, + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + writeFileSync, +} from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { describe, it } from 'node:test' @@ -8,47 +16,96 @@ import { describe, it } from 'node:test' import { channelDir, readResult, writeRequest } from '../../packages/core/src/host-request.ts' import { runHostBroker } from '../../packages/host/src/commands/host.ts' +interface Fixture { + readonly answer: ReturnType + readonly argv: string[] + readonly cleanup: () => void +} + +async function runFixture(metadata: string): Promise { + const root = mkdtempSync(join(tmpdir(), 'host-broker-pr-fork-')) + const repo = join(root, 'checkout') + mkdirSync(join(repo, '.git'), { recursive: true }) + writeFileSync(join(repo, '.git', 'HEAD'), 'ref: refs/heads/main\n', 'utf8') + const config = join(root, 'broker.config.yml') + writeFileSync( + config, + `socketPath: ${join(root, 'broker.sock')}\nhosts:\n github.com:\n provider: static\n username: x\n password: y\n allowFork: true\n allow:\n - JounQin/vitehub\n`, + 'utf8', + ) + const dir = channelDir(join(repo, '.git')) + const argvFile = join(root, 'argv') + const command = join(root, 'not-gh') + writeFileSync( + command, + `#!/bin/sh +if [ "$1" = api ]; then + case "$2" in repos/JounQin/vitehub|repos/jounqin/vitehub) ;; *) echo "unexpected $2" >&2; exit 1;; esac + echo '${metadata}' + exit 0 +fi +printf '%s\n' "$@" > ${argvFile} +echo "https://github.com/vite-hub/vitehub/pull/9" +`, + 'utf8', + ) + chmodSync(command, 0o755) + try { + const id = writeRequest(dir, { + repo: 'vite-hub/vitehub', + head: 'feat/x', + base: 'next', + title: 'x', + body: '', + }) + assert.equal( + await runHostBroker(['--repo', repo, '--command', command, '--config', config, '--once']), + 0, + ) + const argv = existsSync(argvFile) ? readFileSync(argvFile, 'utf8').trim().split('\n') : [] + return { + answer: readResult(dir, id), + argv, + cleanup: () => rmSync(root, { recursive: true, force: true }), + } + } catch (error) { + rmSync(root, { recursive: true, force: true }) + throw error + } +} + describe('opening a pull request on a fork parent', () => { it('allows it when allowFork is set', async () => { - const root = mkdtempSync(join(tmpdir(), 'host-broker-pr-fork-')) - const repo = join(root, 'checkout') - mkdirSync(join(repo, '.git'), { recursive: true }) - writeFileSync(join(repo, '.git', 'HEAD'), 'ref: refs/heads/main\n', 'utf8') - const config = join(root, 'broker.config.yml') - writeFileSync( - config, - `socketPath: ${join(root, 'broker.sock')}\nhosts:\n github.com:\n provider: static\n username: x\n password: y\n allowFork: true\n allow:\n - JounQin/vitehub\n`, - 'utf8', + const fixture = await runFixture( + '{"fork":true,"private":false,"visibility":"public","parent":{"full_name":"vite-hub/vitehub","private":false,"visibility":"public"}}', ) - const dir = channelDir(join(repo, '.git')) - const argvFile = join(root, 'argv') - const command = join(root, 'not-gh') - writeFileSync( - command, - '#!/bin/sh\nif [ "$1" = api ]; then echo \'{"fork":true,"private":false,"visibility":"public","parent":{"full_name":"vite-hub/vitehub"}}\'; exit 0; fi\nprintf \'%s\\n\' "$@" > ' + - argvFile + - '\necho "https://github.com/vite-hub/vitehub/pull/9"\n', - 'utf8', + try { + assert.equal(fixture.answer?.number, 9, JSON.stringify(fixture.answer)) + assert.equal(fixture.argv[fixture.argv.indexOf('--repo') + 1], 'vite-hub/vitehub') + } finally { + fixture.cleanup() + } + }) + + it('refuses a private fork', async () => { + const fixture = await runFixture( + '{"fork":true,"private":true,"visibility":"private","parent":{"full_name":"vite-hub/vitehub","private":false,"visibility":"public"}}', + ) + try { + assert.match(String(fixture.answer?.error ?? ''), /not in the allowlist/u) + } finally { + fixture.cleanup() + } + }) + + it('refuses a private parent', async () => { + const fixture = await runFixture( + '{"fork":true,"private":false,"visibility":"public","parent":{"full_name":"vite-hub/vitehub","private":true,"visibility":"private"}}', ) - chmodSync(command, 0o755) try { - const id = writeRequest(dir, { - repo: 'vite-hub/vitehub', - head: 'feat/x', - base: 'next', - title: 'x', - body: '', - }) - assert.equal( - await runHostBroker(['--repo', repo, '--command', command, '--config', config, '--once']), - 0, - ) - const answer = readResult(dir, id) - assert.equal(answer?.number, 9, JSON.stringify(answer)) - const argv = readFileSync(argvFile, 'utf8').trim().split('\n') - assert.equal(argv[argv.indexOf('--repo') + 1], 'vite-hub/vitehub') + assert.match(String(fixture.answer?.error ?? ''), /not in the allowlist/u) } finally { - rmSync(root, { recursive: true, force: true }) + fixture.cleanup() } }) }) diff --git a/test/unit/host-fork.test.ts b/test/unit/host-fork.test.ts index 5ddc7c9c..8ef3ed90 100644 --- a/test/unit/host-fork.test.ts +++ b/test/unit/host-fork.test.ts @@ -46,7 +46,7 @@ it('runs an action on the parent of an allowlisted fork', async () => { const command = join(root, 'not-gh') writeFileSync( command, - '#!/bin/sh\nif [ "$1" = api ]; then echo \'{"fork":true,"private":false,"visibility":"public","parent":{"full_name":"Acme/Widget"}}\'; exit 0; fi\necho "fork action"\n', + '#!/bin/sh\nif [ "$1" = api ]; then\n case "$2" in repos/alice/widget) ;; *) echo "unexpected $2" >&2; exit 1;; esac\n echo \'{"fork":true,"private":false,"visibility":"public","parent":{"full_name":"Acme/Widget","private":false,"visibility":"public"}}\'\n exit 0\nfi\necho "fork action"\n', 'utf8', ) chmodSync(command, 0o755) From 3ecb56a4103a6d8964240b223cb56bd39dfefb22 Mon Sep 17 00:00:00 2001 From: JounQin Date: Sat, 14 Nov 2026 08:03:38 +0000 Subject: [PATCH 4/7] docs: describe the public fork and parent check --- .changeset/allowfork-auth.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.changeset/allowfork-auth.md b/.changeset/allowfork-auth.md index aab9388e..997bf75c 100644 --- a/.changeset/allowfork-auth.md +++ b/.changeset/allowfork-auth.md @@ -4,4 +4,5 @@ Check `allowFork` through the host's authenticated `gh` account instead of the unauthenticated public API, whose shared-egress rate limit silently disabled the fork check. The authenticated response must also -report a public fork, so a private fork cannot widen the allowlist to its upstream. +report a public fork and a public matched parent/source, so a private relationship cannot widen the +allowlist to its upstream. From 897f2a2a36f880d3f52d08b556d36f97d6735f40 Mon Sep 17 00:00:00 2001 From: JounQin Date: Sat, 14 Nov 2026 08:09:54 +0000 Subject: [PATCH 5/7] fix: authenticate the app fork lookup too --- .changeset/allowfork-auth.md | 7 +- packages/host/src/providers/github-app.ts | 109 +++++++++++++++------- packages/host/src/public-repo.ts | 8 +- test/unit/github-app.test.ts | 95 +++++++++++++++---- 4 files changed, 158 insertions(+), 61 deletions(-) diff --git a/.changeset/allowfork-auth.md b/.changeset/allowfork-auth.md index 997bf75c..4c03d8ab 100644 --- a/.changeset/allowfork-auth.md +++ b/.changeset/allowfork-auth.md @@ -2,7 +2,6 @@ "gcb-host": patch --- -Check `allowFork` through the host's authenticated `gh` account instead of the unauthenticated -public API, whose shared-egress rate limit silently disabled the fork check. The authenticated response must also -report a public fork and a public matched parent/source, so a private relationship cannot widen the -allowlist to its upstream. +Check `allowFork` through authenticated credentials instead of the unauthenticated public API: the +host uses its `gh` account, and the app provider uses the installation on the fork (rejecting a fork +without one). Both require a public fork and a public matched parent/source. diff --git a/packages/host/src/providers/github-app.ts b/packages/host/src/providers/github-app.ts index 13bea951..c3335d90 100644 --- a/packages/host/src/providers/github-app.ts +++ b/packages/host/src/providers/github-app.ts @@ -914,39 +914,82 @@ export function createGithubAppProvider(options: GithubAppProviderOptions): Prov tokens.set(request.full, credential) return { ...credential, cached: false } }, - /** Check a public fork with no credential; only confirmed fork relationships pass. */ + /** Check a public fork through the app installation on that fork, not an anonymous request. */ async isForkOf(owner: string, repo: string, bases: readonly string[]): Promise { - return withRepositoryResponse( - fetchImpl, - api, - `${owner}/${repo}`, - { apiVersion, label: 'fork' }, - async response => { - if (!response.ok) { - return false - } - const text = await response.text() - let body: { - fork?: unknown - parent?: { full_name?: unknown } | null - source?: { full_name?: unknown } | null - } | null - try { - body = JSON.parse(text) as typeof body - } catch { - return false - } - if (body?.fork !== true) { - return false - } - for (const candidate of [body.parent?.full_name, body.source?.full_name]) { - if (typeof candidate === 'string' && repoAllowed(bases, candidate)) { - return true - } - } + const installationId = await repositoryInstallation(owner, repo) + if (installationId === undefined) { + return false + } + let token: string + try { + const { json } = await call( + 'POST', + `${api}/app/installations/${installationId}/access_tokens`, + { + repositories: [repo], + permissions: { metadata: 'read' }, + }, + ) + const minted = (json as { token?: unknown } | null)?.token + if (typeof minted !== 'string' || minted === '') { return false - }, - ) + } + token = minted + } catch { + return false + } + const authenticated: FetchLike = (url, init) => + fetchImpl(url, { + ...init, + headers: { ...init.headers, authorization: `Bearer ${token}` }, + }) + try { + return await withRepositoryResponse( + authenticated, + api, + `${owner}/${repo}`, + { apiVersion, label: 'fork', authenticated: true }, + async response => { + if (!response.ok) { + return false + } + const text = await response.text() + let body: { + fork?: unknown + private?: unknown + visibility?: unknown + parent?: { full_name?: unknown; private?: unknown; visibility?: unknown } | null + source?: { full_name?: unknown; private?: unknown; visibility?: unknown } | null + } | null + try { + body = JSON.parse(text) as typeof body + } catch { + return false + } + const isPublic = ( + value: { private?: unknown; visibility?: unknown } | null | undefined, + ): boolean => + typeof value === 'object' && + value !== null && + value.private === false && + (typeof value.visibility !== 'string' || value.visibility === 'public') + if (body?.fork !== true || !isPublic(body)) { + return false + } + return [body.parent, body.source].some( + candidate => + isPublic(candidate) && + typeof candidate?.full_name === 'string' && + repoAllowed(bases, candidate.full_name), + ) + }, + ) + } catch (error) { + if (error instanceof ProviderConfigError) { + throw error + } + return false + } }, /** Verify a public repository with no credential; a 403/429 is a provider error. */ @@ -1019,9 +1062,7 @@ export function createGithubAppProvider(options: GithubAppProviderOptions): Prov if (request.action === 'fork') { const token = await personalToken() if (token === undefined) { - throw new ProviderConfigError( - 'no user token for fork; run gcb authorize', - ) + throw new ProviderConfigError('no user token for fork; run gcb authorize') } const { text } = await callWithToken( 'POST', diff --git a/packages/host/src/public-repo.ts b/packages/host/src/public-repo.ts index 4e0111cd..4ecb68f0 100644 --- a/packages/host/src/public-repo.ts +++ b/packages/host/src/public-repo.ts @@ -12,10 +12,12 @@ export interface RepositoryCheckOptions { readonly timeoutMs?: number /** Name used in the timeout and rate-limit messages. */ readonly label?: string + /** Whether the request carried an app token; changes only the refusal wording. */ + readonly authenticated?: boolean } /** - * Run one unauthenticated repository request with a deadline and no redirects. + * Run one repository request with a deadline and no redirects. * * @param fetchImpl - The transport for this host's proxy choice. * @param apiBaseUrl - The API base, without a trailing path. @@ -52,8 +54,8 @@ export async function withRepositoryResponse( const label = options.label ?? 'repository' throw new ProviderConfigError( exhausted - ? `GitHub rate-limited the unauthenticated ${label} check; retry later or allowlist the repository` - : `GitHub refused the unauthenticated ${label} check; check the access policy or allowlist the repository`, + ? `GitHub rate-limited the ${options.authenticated === true ? 'authenticated' : 'unauthenticated'} ${label} check; retry later or allowlist the repository` + : `GitHub refused the ${options.authenticated === true ? 'authenticated' : 'unauthenticated'} ${label} check; check the access policy or allowlist the repository`, ) } return await handle(response) diff --git a/test/unit/github-app.test.ts b/test/unit/github-app.test.ts index f7beb58e..d634aae7 100644 --- a/test/unit/github-app.test.ts +++ b/test/unit/github-app.test.ts @@ -607,7 +607,7 @@ describe('createGithubAppProvider', () => { } }) - it('passes the deadline and redirect refusal to the fork lookup', async () => { + it('authenticates the fork lookup and passes the deadline and redirect policy', async () => { const calls: RecordedCall[] = [] const provider = providerFor({ calls, @@ -621,15 +621,28 @@ describe('createGithubAppProvider', () => { signal: init.signal, redirect: init.redirect, }) - return jsonResponse({ fork: true, parent: { full_name: 'acme/widget' } }) + if (url.endsWith('/repos/alice/widget/installation')) { + return jsonResponse({ id: 42 }) + } + if (url.includes('/access_tokens')) { + return jsonResponse({ token: 'ghs_fork_token' }, 201) + } + return jsonResponse({ + fork: true, + private: false, + visibility: 'public', + parent: { full_name: 'acme/widget', private: false, visibility: 'public' }, + }) }, }) assert.equal(await provider.isForkOf!('alice', 'widget', ['acme/widget']), true) - assert.ok(calls[0]?.signal instanceof AbortSignal, 'the fork check must carry the abort signal') - assert.equal(calls[0]?.redirect, 'error', 'the fork check must refuse redirects') + const forkCall = calls.find(call => call.url.endsWith('/repos/alice/widget')) + assert.ok(forkCall?.signal instanceof AbortSignal, 'the fork check must carry the abort signal') + assert.equal(forkCall?.redirect, 'error', 'the fork check must refuse redirects') + assert.equal(forkCall?.authorization, 'Bearer ghs_fork_token') }) - it('classifies public and fork lookups without credentials', async () => { + it('classifies public lookups without credentials', async () => { const response = (status: number, body: unknown) => ({ ok: status >= 200 && status < 300, status, @@ -649,31 +662,73 @@ describe('createGithubAppProvider', () => { assert.equal(await provider(404, {}).isPublicRepository!('acme', 'widget'), false) await assert.rejects(provider(429, {}).isPublicRepository!('acme', 'widget'), /rate-limited/u) await assert.rejects(provider(403, {}).isPublicRepository!('acme', 'widget'), /refused/u) + }) + + it('checks forks through the installation and requires public metadata', async () => { + const response = (status: number, body: unknown) => ({ + ok: status >= 200 && status < 300, + status, + headers: { get: () => null }, + text: async () => JSON.stringify(body), + }) + const check = async ( + status: number, + body: unknown, + installed = true, + ): Promise<{ result?: boolean; error?: Error }> => { + const calls: RecordedCall[] = [] + const provider = providerFor({ + calls, + fetchImpl: async (url, init) => { + calls.push({ + url, + method: init.method, + body: recordedBody(init.body), + authorization: init.headers['authorization'], + raw: init.body ?? '', + }) + if (url.endsWith('/repos/alice/widget/installation')) { + return installed ? jsonResponse({ id: 42 }) : jsonResponse({}, 404) + } + if (url.includes('/access_tokens')) { + return jsonResponse({ token: 'ghs_fork_token' }, 201) + } + return response(status, body) + }, + }) + try { + return { result: await provider.isForkOf!('alice', 'widget', ['acme/widget']) } + } catch (error) { + return { error: error as Error } + } + } + const parent = { full_name: 'acme/widget', private: false, visibility: 'public' } assert.equal( - await provider(200, { fork: true, parent: { full_name: 'acme/widget' } }).isForkOf!( - 'alice', - 'widget', - ['acme/widget'], - ), + (await check(200, { fork: true, private: false, visibility: 'public', parent })).result, true, ) assert.equal( - await provider(200, { fork: true, source: { full_name: 'acme/widget' } }).isForkOf!( - 'alice', - 'widget', - ['acme/widget'], - ), + (await check(200, { fork: true, private: false, visibility: 'public', source: parent })) + .result, true, ) assert.equal( - await provider(200, { fork: false }).isForkOf!('alice', 'widget', ['acme/widget']), + (await check(200, { fork: true, private: true, visibility: 'private', parent })).result, false, ) - assert.equal(await provider(404, {}).isForkOf!('alice', 'widget', ['acme/widget']), false) - await assert.rejects( - provider(429, {}).isForkOf!('alice', 'widget', ['acme/widget']), - /rate-limited/u, + assert.equal( + ( + await check(200, { + fork: true, + private: false, + visibility: 'public', + parent: { ...parent, private: true, visibility: 'private' }, + }) + ).result, + false, ) + assert.equal((await check(200, {}, false)).result, false) + assert.match((await check(429, {})).error?.message ?? '', /rate-limited/u) }) it('mints a metadata-only token for a verified public read', async () => { From df720bc3a08ffe043ed7597469e1fc7c9d5bd910 Mon Sep 17 00:00:00 2001 From: JounQin Date: Sat, 14 Nov 2026 08:26:20 +0000 Subject: [PATCH 6/7] fix: share the fork-check deadline and surface auth failures --- packages/host/src/providers/github-app.ts | 32 ++++++++++++++++++++--- packages/host/src/public-repo.ts | 10 +++++-- test/unit/github-app.test.ts | 17 +++++++++++- 3 files changed, 52 insertions(+), 7 deletions(-) diff --git a/packages/host/src/providers/github-app.ts b/packages/host/src/providers/github-app.ts index c3335d90..64ce3661 100644 --- a/packages/host/src/providers/github-app.ts +++ b/packages/host/src/providers/github-app.ts @@ -286,6 +286,8 @@ export function createGithubAppProvider(options: GithubAppProviderOptions): Prov method: string, url: string, body?: Record, + signal?: AbortSignal, + authStage = false, ): Promise<{ json: unknown; link: string | null }> { const jwt = signAppJwt({ privateKeyPem: keyPem, iss, nowMs: now() }) const headers: Record = { @@ -302,6 +304,7 @@ export function createGithubAppProvider(options: GithubAppProviderOptions): Prov method, headers, ...(body ? { body: JSON.stringify(body) } : {}), + ...(signal === undefined ? {} : { signal }), }) const text = await response.text() @@ -312,6 +315,11 @@ export function createGithubAppProvider(options: GithubAppProviderOptions): Prov json = null } if (!response.ok) { + if (authStage && (response.status === 403 || response.status === 429)) { + throw new ProviderConfigError( + `GitHub rate-limited or refused the app installation token request; retry later`, + ) + } const message = json !== null && typeof json === 'object' && 'message' in json ? String((json as { message: unknown }).message) @@ -718,7 +726,11 @@ export function createGithubAppProvider(options: GithubAppProviderOptions): Prov * @param repo - The repository name. * @returns The installation id, or undefined when the app is not installed there. */ - async function repositoryInstallation(owner: string, repo: string): Promise { + async function repositoryInstallation( + owner: string, + repo: string, + signal?: AbortSignal, + ): Promise { const jwt = signAppJwt({ privateKeyPem: keyPem, iss, nowMs: now() }) const response = await fetchImpl(`${api}/repos/${owner}/${repo}/installation`, { method: 'GET', @@ -728,11 +740,17 @@ export function createGithubAppProvider(options: GithubAppProviderOptions): Prov 'x-github-api-version': apiVersion, 'user-agent': 'gcb', }, + ...(signal === undefined ? {} : { signal }), }) if (response.status === 404) { return undefined } if (!response.ok) { + if (response.status === 403 || response.status === 429) { + throw new ProviderConfigError( + 'GitHub rate-limited or refused the app installation lookup; retry later', + ) + } const text = await response.text() throw new Error( `GitHub API GET /repos/${owner}/${repo}/installation failed with ${response.status}: ${text.slice(0, 200)}`, @@ -916,7 +934,8 @@ export function createGithubAppProvider(options: GithubAppProviderOptions): Prov }, /** Check a public fork through the app installation on that fork, not an anonymous request. */ async isForkOf(owner: string, repo: string, bases: readonly string[]): Promise { - const installationId = await repositoryInstallation(owner, repo) + const signal = AbortSignal.timeout(10_000) + const installationId = await repositoryInstallation(owner, repo, signal) if (installationId === undefined) { return false } @@ -929,13 +948,18 @@ export function createGithubAppProvider(options: GithubAppProviderOptions): Prov repositories: [repo], permissions: { metadata: 'read' }, }, + signal, + true, ) const minted = (json as { token?: unknown } | null)?.token if (typeof minted !== 'string' || minted === '') { return false } token = minted - } catch { + } catch (error) { + if (error instanceof ProviderConfigError) { + throw error + } return false } const authenticated: FetchLike = (url, init) => @@ -948,7 +972,7 @@ export function createGithubAppProvider(options: GithubAppProviderOptions): Prov authenticated, api, `${owner}/${repo}`, - { apiVersion, label: 'fork', authenticated: true }, + { apiVersion, label: 'fork', authenticated: true, signal }, async response => { if (!response.ok) { return false diff --git a/packages/host/src/public-repo.ts b/packages/host/src/public-repo.ts index 4ecb68f0..e8ec022e 100644 --- a/packages/host/src/public-repo.ts +++ b/packages/host/src/public-repo.ts @@ -14,6 +14,8 @@ export interface RepositoryCheckOptions { readonly label?: string /** Whether the request carried an app token; changes only the refusal wording. */ readonly authenticated?: boolean + /** Caller-owned deadline shared with the stages around this request. */ + readonly signal?: AbortSignal } /** @@ -35,6 +37,10 @@ export async function withRepositoryResponse( ): Promise { const controller = new AbortController() const timer = setTimeout(() => controller.abort(), options.timeoutMs ?? CHECK_TIMEOUT_MS) + const signal = + options.signal === undefined + ? controller.signal + : AbortSignal.any([controller.signal, options.signal]) try { const response = await fetchImpl(`${apiBaseUrl}/repos/${repository}`, { method: 'GET', @@ -43,7 +49,7 @@ export async function withRepositoryResponse( 'x-github-api-version': options.apiVersion ?? '2022-11-28', 'user-agent': 'gcb', }, - signal: controller.signal, + signal, redirect: 'error', }) if (response.status === 403 || response.status === 429) { @@ -60,7 +66,7 @@ export async function withRepositoryResponse( } return await handle(response) } catch (error) { - if (controller.signal.aborted) { + if (signal.aborted) { throw new ProviderConfigError( `GitHub timed out during the ${options.label ?? 'repository'} check; retry later`, ) diff --git a/test/unit/github-app.test.ts b/test/unit/github-app.test.ts index d634aae7..8e412bab 100644 --- a/test/unit/github-app.test.ts +++ b/test/unit/github-app.test.ts @@ -638,6 +638,14 @@ describe('createGithubAppProvider', () => { assert.equal(await provider.isForkOf!('alice', 'widget', ['acme/widget']), true) const forkCall = calls.find(call => call.url.endsWith('/repos/alice/widget')) assert.ok(forkCall?.signal instanceof AbortSignal, 'the fork check must carry the abort signal') + assert.ok( + calls.find(call => call.url.endsWith('/installation'))?.signal instanceof AbortSignal, + 'the installation lookup shares the deadline', + ) + assert.ok( + calls.find(call => call.url.includes('/access_tokens'))?.signal instanceof AbortSignal, + 'the token mint shares the deadline', + ) assert.equal(forkCall?.redirect, 'error', 'the fork check must refuse redirects') assert.equal(forkCall?.authorization, 'Bearer ghs_fork_token') }) @@ -675,6 +683,7 @@ describe('createGithubAppProvider', () => { status: number, body: unknown, installed = true, + tokenStatus = 201, ): Promise<{ result?: boolean; error?: Error }> => { const calls: RecordedCall[] = [] const provider = providerFor({ @@ -691,7 +700,9 @@ describe('createGithubAppProvider', () => { return installed ? jsonResponse({ id: 42 }) : jsonResponse({}, 404) } if (url.includes('/access_tokens')) { - return jsonResponse({ token: 'ghs_fork_token' }, 201) + return tokenStatus === 201 + ? jsonResponse({ token: 'ghs_fork_token' }, 201) + : jsonResponse({ message: 'rate limited' }, tokenStatus) } return response(status, body) }, @@ -729,6 +740,10 @@ describe('createGithubAppProvider', () => { ) assert.equal((await check(200, {}, false)).result, false) assert.match((await check(429, {})).error?.message ?? '', /rate-limited/u) + assert.match( + (await check(200, { fork: true }, true, 403)).error?.message ?? '', + /rate-limited|refused/u, + ) }) it('mints a metadata-only token for a verified public read', async () => { From bc89c620220369615c99922a426097505f31de08 Mon Sep 17 00:00:00 2001 From: JounQin Date: Sat, 14 Nov 2026 08:36:28 +0000 Subject: [PATCH 7/7] fix: report a fork-check timeout as a provider error --- packages/host/src/providers/github-app.ts | 34 ++++++++++------------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/packages/host/src/providers/github-app.ts b/packages/host/src/providers/github-app.ts index 64ce3661..f02d3e50 100644 --- a/packages/host/src/providers/github-app.ts +++ b/packages/host/src/providers/github-app.ts @@ -935,12 +935,11 @@ export function createGithubAppProvider(options: GithubAppProviderOptions): Prov /** Check a public fork through the app installation on that fork, not an anonymous request. */ async isForkOf(owner: string, repo: string, bases: readonly string[]): Promise { const signal = AbortSignal.timeout(10_000) - const installationId = await repositoryInstallation(owner, repo, signal) - if (installationId === undefined) { - return false - } - let token: string try { + const installationId = await repositoryInstallation(owner, repo, signal) + if (installationId === undefined) { + return false + } const { json } = await call( 'POST', `${api}/app/installations/${installationId}/access_tokens`, @@ -951,23 +950,15 @@ export function createGithubAppProvider(options: GithubAppProviderOptions): Prov signal, true, ) - const minted = (json as { token?: unknown } | null)?.token - if (typeof minted !== 'string' || minted === '') { + const token = (json as { token?: unknown } | null)?.token + if (typeof token !== 'string' || token === '') { return false } - token = minted - } catch (error) { - if (error instanceof ProviderConfigError) { - throw error - } - return false - } - const authenticated: FetchLike = (url, init) => - fetchImpl(url, { - ...init, - headers: { ...init.headers, authorization: `Bearer ${token}` }, - }) - try { + const authenticated: FetchLike = (url, init) => + fetchImpl(url, { + ...init, + headers: { ...init.headers, authorization: `Bearer ${token}` }, + }) return await withRepositoryResponse( authenticated, api, @@ -1009,6 +1000,9 @@ export function createGithubAppProvider(options: GithubAppProviderOptions): Prov }, ) } catch (error) { + if (signal.aborted) { + throw new ProviderConfigError('GitHub timed out during the fork check; retry later') + } if (error instanceof ProviderConfigError) { throw error }