diff --git a/ROADMAP.md b/ROADMAP.md index 977abe4c..e175ae96 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -242,12 +242,11 @@ host_broker: - per-action total timeout, `SIGTERM` then `SIGKILL`, kill the process group; - no automatic retry. A timed-out write reports that it may have completed. (Current executor sends SIGKILL only; its timeout starts after preflight/token mint.) -- [ ] Token lifecycle: +- [x] Token lifecycle: - mint per action with only that action's permissions; - never cache; - `finally`-revoke on success, failure, and timeout; - - if revoke fails, log host-side and report that the token expires on its own. (The host log is - now wired; the caller is not yet told.) + - if revoke fails, log host-side and report that the token expires on its own. - [ ] Single-source action catalog (table below): one declarative table drives the permission preflight, token mint, command table and docs; configured actions extend it under the Phase 1 rules. A test rejects drift between catalog, preflight, mint and docs, and rejects any built-in diff --git a/packages/cli/src/types.ts b/packages/cli/src/types.ts index 11ef8828..58156e9f 100644 --- a/packages/cli/src/types.ts +++ b/packages/cli/src/types.ts @@ -38,6 +38,8 @@ export interface ActionRunResult { readonly error?: string; /** SHA-256 prefix of the action token, for the audit record only. */ readonly tokenFingerprint?: string; + /** Set when the token could not be revoked and so expires on its own. */ + readonly warning?: string; } /** One credential, as handed to git by the helper. */ @@ -279,6 +281,8 @@ export interface WireResponse { readonly stderr?: string; readonly exitCode?: number | null; readonly timedOut?: boolean; + /** A non-fatal action warning, for example a failed token revoke. */ + readonly warning?: string; } /** Where audit records go. */ diff --git a/packages/client/src/commands/gh.ts b/packages/client/src/commands/gh.ts index ffa3f5f2..f8c20563 100644 --- a/packages/client/src/commands/gh.ts +++ b/packages/client/src/commands/gh.ts @@ -123,10 +123,12 @@ export async function runGh(argv: readonly string[]): Promise { fail((error as Error).message); } if (response.ok !== true) { + if (response.warning) process.stderr.write(`warning: ${response.warning}\n`); fail(`refused [${response.code ?? 'unknown'}]: ${response.reason ?? 'no reason'}`); } if (response.stdout) process.stdout.write(response.stdout); if (response.stderr) process.stderr.write(response.stderr); + if (response.warning) process.stderr.write(`warning: ${response.warning}\n`); if (response.truncated === true) { process.stderr.write('warning: the action output was truncated at the broker\x27s limit\n'); } diff --git a/packages/host/src/broker.ts b/packages/host/src/broker.ts index b5ed3b74..2c2bc240 100644 --- a/packages/host/src/broker.ts +++ b/packages/host/src/broker.ts @@ -296,7 +296,8 @@ export function createRequestHandler( values: actionValues, }); if (result.error !== undefined) { - return denyAction(CODES.PROVIDER_ERROR, result.error, 'the host could not start gh'); + const refusal = denyAction(CODES.PROVIDER_ERROR, result.error, 'the host could not start gh'); + return result.warning === undefined ? refusal : { ...refusal, warning: result.warning }; } audit.record({ event: 'action', @@ -319,6 +320,7 @@ export function createRequestHandler( exitCode: result.code, truncated: result.truncated, timedOut: result.timedOut, + ...(result.warning === undefined ? {} : { warning: result.warning }), }; } catch (error) { const reason = String((error as Error).message ?? error).slice(0, 300); diff --git a/packages/host/src/providers/github-app.ts b/packages/host/src/providers/github-app.ts index b39011e9..b485cd4b 100644 --- a/packages/host/src/providers/github-app.ts +++ b/packages/host/src/providers/github-app.ts @@ -19,7 +19,7 @@ */ import { userTokenPath } from '../config.ts'; import { createSign } from 'node:crypto'; -import { getGhAction, ProviderConfigError, tokenFingerprint } from 'gcb-cli'; +import { getGhAction, ProviderConfigError, tokenFingerprint, type ActionRunResult } from 'gcb-cli'; import { runGhAction } from '../gh.ts'; import { existsSync, readFileSync, writeFileSync } from 'node:fs'; @@ -190,6 +190,9 @@ export function createGithubAppProvider(options: GithubAppProviderOptions): Prov * a token minted for git should not also be able to open a pull request. */ const PULL_REQUEST_PERMISSIONS: Record = { pull_requests: 'write', contents: 'read' }; + /** The result of an action that never reached the executor. */ + const NO_RUN = { stdout: '', stderr: '', code: null, signal: null, truncated: false, timedOut: false }; + const REVOKE_WARNING = 'the action token could not be revoked; it expires on its own'; /** owner login (lowercased) -> installation */ const installations = new Map(); @@ -578,8 +581,10 @@ function pullRequestNumber(value: unknown): number | undefined { if (typeof token !== 'string' || !token) { throw new ProviderConfigError('GitHub returned no installation token for the action'); } + let result: ActionRunResult; + let warning: string | undefined; try { - const result = await runGhAction({ + result = await runGhAction({ action: request.action, values: request.values, repo: request.full, @@ -588,7 +593,8 @@ function pullRequestNumber(value: unknown): number | undefined { ghPath, env: process.env, }); - return { ...result, tokenFingerprint: tokenFingerprint(token) }; + } catch (error) { + result = { ...NO_RUN, error: error instanceof Error ? error.message : String(error) }; } finally { try { const response = await fetchImpl(`${api}/installation/token`, { @@ -608,11 +614,14 @@ function pullRequestNumber(value: unknown): number | undefined { // The status is enough; a body that cannot be read is still not a successful revoke. } log(`could not revoke the action token: GitHub answered ${response.status}${detail ? ` ${detail}` : ''}`); + warning = REVOKE_WARNING; } } catch (error) { log(`could not revoke the action token: ${String((error as Error).message ?? error)}`); + warning = REVOKE_WARNING; } } + return { ...result, tokenFingerprint: tokenFingerprint(token), ...(warning === undefined ? {} : { warning }) }; }, async getCredential(request): Promise { diff --git a/test/unit/broker.test.ts b/test/unit/broker.test.ts index 377510ef..ab023738 100644 --- a/test/unit/broker.test.ts +++ b/test/unit/broker.test.ts @@ -577,6 +577,59 @@ describe('broker actions', () => { assert.equal(record?.['token_fingerprint'], 'action-fingerprint'); }); + it('passes a warning with a failed start', async () => { + const provider: Provider = { + name: 'static', + getCredential: async () => ({ username: 'u', password: 'p', expiresAt: Date.now() + 1000 }), + runAction: async () => ({ + stdout: '', + stderr: '', + code: null, + signal: null, + truncated: false, + timedOut: false, + error: 'spawn failed', + warning: 'the action token could not be revoked; it expires on its own', + }), + }; + const harness = makeHarness(undefined, provider); + const response = await harness.handle({ + op: 'action', + host: HOST, + path: 'acme/widget', + ghAction: 'pr:view', + values: { number: 7 }, + }); + assert.equal(response.ok, false); + assert.match(String(response.warning), /expires on its own/); + }); + + it('passes a provider warning to the caller', async () => { + const provider: Provider = { + name: 'static', + getCredential: async () => ({ username: 'u', password: 'p', expiresAt: Date.now() + 1000 }), + runAction: async () => ({ + stdout: 'ok', + stderr: '', + code: 0, + signal: null, + truncated: false, + timedOut: false, + warning: 'the action token could not be revoked; it expires on its own', + }), + }; + const harness = makeHarness(undefined, provider); + const response = await harness.handle({ + op: 'action', + host: HOST, + path: 'acme/widget', + ghAction: 'pr:view', + values: { number: 7 }, + }); + assert.equal(response.ok, true); + assert.match(String(response.warning), /expires on its own/); + }); + it('refuses an undeclared flag before the provider', async () => { let called = false; const provider: Provider = { diff --git a/test/unit/gh-command.test.ts b/test/unit/gh-command.test.ts index db3f76c8..e9f020b9 100644 --- a/test/unit/gh-command.test.ts +++ b/test/unit/gh-command.test.ts @@ -2,32 +2,23 @@ import assert from 'node:assert/strict'; import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; +import { fileURLToPath } from 'node:url'; import { describe, it } from 'node:test'; -import { runGh } from '../../packages/client/src/commands/gh.ts'; import { startBroker } from '../../packages/host/src/broker.ts'; +import { tryRunAsync, type RunResult } from '../lib/run.ts'; import type { AuditSink, BrokerConfig, Provider, StaticHostConfig } from '../../packages/cli/src/types.ts'; -/** Capture stdout for one command. */ -async function capture(run: () => Promise): Promise<{ code: number; stdout: string; stderr: string }> { - let stdout = ''; - let stderr = ''; - const originalOut = process.stdout.write; - const originalErr = process.stderr.write; - process.stdout.write = ((chunk: unknown) => { - stdout += String(chunk); - return true; - }) as typeof process.stdout.write; - process.stderr.write = ((chunk: unknown) => { - stderr += String(chunk); - return true; - }) as typeof process.stderr.write; - try { - return { code: await run(), stdout, stderr }; - } finally { - process.stdout.write = originalOut; - process.stderr.write = originalErr; - } +/** Run the client from source, next to this file rather than from the cwd. */ +const CLIENT_BIN = fileURLToPath(new URL('../../packages/client/bin/gcb.js', import.meta.url)); + +/** Run the installed client as a child, so its output never replaces the test reporter. */ +function runGhCli(args: readonly string[]): Promise { + return tryRunAsync( + process.execPath, + ['--conditions=development', CLIENT_BIN, 'gh', ...args], + { env: process.env, timeoutMs: 5000 }, + ); } describe('gcb gh', () => { @@ -57,10 +48,8 @@ describe('gcb gh', () => { }; const broker = await startBroker({ config, audit, providers: new Map([['github.com', provider]]) }); try { - const result = await capture(() => - runGh(['pr', 'create', '--head', 'feat/x', '--title', 'x', '--body', 'b', '--repo', 'acme/widget', '--socket', socketPath]), - ); - assert.equal(result.code, 1); + const result = await runGhCli(['pr', 'create', '--head', 'feat/x', '--title', 'x', '--body', 'b', '--repo', 'acme/widget', '--socket', socketPath]); + assert.equal(result.status, 1); assert.ok(result.stdout.endsWith('partial output\n')); assert.match(result.stderr, /output was truncated/); assert.match(result.stderr, /may have completed/); @@ -70,6 +59,80 @@ describe('gcb gh', () => { } }); + it('prints a warning on a refused action', async () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-gh-refused-warning-')); + const socketPath = path.join(root, 'broker.sock'); + const block: StaticHostConfig = { provider: 'static', allow: ['acme/widget'] }; + const config: BrokerConfig = { + socketPath, + socketMode: 0o600, + auditPath: null, + tokenCacheSkewSeconds: 300, + hosts: { 'github.com': block }, + }; + const audit: AuditSink = { path: null, record: () => {}, close: () => Promise.resolve() }; + const provider: Provider = { + name: 'static', + getCredential: async () => ({ username: 'u', password: 'p', expiresAt: Date.now() + 1000 }), + runAction: async () => ({ + stdout: '', + stderr: '', + code: null, + signal: null, + truncated: false, + timedOut: false, + error: 'spawn failed', + warning: 'the action token could not be revoked; it expires on its own', + }), + }; + const broker = await startBroker({ config, audit, providers: new Map([['github.com', provider]]) }); + try { + const result = await runGhCli(['pr', 'view', '7', '--repo', 'acme/widget', '--socket', socketPath]); + assert.equal(result.status, 1); + assert.match(result.stderr, /expires on its own/); + assert.match(result.stderr, /refused/); + } finally { + await broker.close(); + fs.rmSync(root, { recursive: true, force: true }); + } + }); + + it('prints a provider warning to stderr', async () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-gh-warning-')); + const socketPath = path.join(root, 'broker.sock'); + const block: StaticHostConfig = { provider: 'static', allow: ['acme/widget'] }; + const config: BrokerConfig = { + socketPath, + socketMode: 0o600, + auditPath: null, + tokenCacheSkewSeconds: 300, + hosts: { 'github.com': block }, + }; + const audit: AuditSink = { path: null, record: () => {}, close: () => Promise.resolve() }; + const provider: Provider = { + name: 'static', + getCredential: async () => ({ username: 'u', password: 'p', expiresAt: Date.now() + 1000 }), + runAction: async () => ({ + stdout: 'viewed\n', + stderr: '', + code: 0, + signal: null, + truncated: false, + timedOut: false, + warning: 'the action token could not be revoked; it expires on its own', + }), + }; + const broker = await startBroker({ config, audit, providers: new Map([['github.com', provider]]) }); + try { + const result = await runGhCli(['pr', 'view', '7', '--repo', 'acme/widget', '--socket', socketPath]); + assert.equal(result.status, 0); + assert.match(result.stderr, /expires on its own/); + } finally { + await broker.close(); + fs.rmSync(root, { recursive: true, force: true }); + } + }); + it('sends a typed action to the broker and prints its stdout', async () => { const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-gh-command-')); const socketPath = path.join(root, 'broker.sock'); @@ -93,10 +156,8 @@ describe('gcb gh', () => { }; const broker = await startBroker({ config, audit, providers: new Map([['github.com', provider]]) }); try { - const result = await capture(() => - runGh(['pr', 'view', '7', '--repo', 'acme/widget', '--socket', socketPath]), - ); - assert.equal(result.code, 0); + const result = await runGhCli(['pr', 'view', '7', '--repo', 'acme/widget', '--socket', socketPath]); + assert.equal(result.status, 0); assert.ok(result.stdout.endsWith('the viewed PR\n')); } finally { await broker.close(); diff --git a/test/unit/github-app.test.ts b/test/unit/github-app.test.ts index 299ea83e..511aa8c1 100644 --- a/test/unit/github-app.test.ts +++ b/test/unit/github-app.test.ts @@ -146,6 +146,7 @@ function createStubFetch(options: { token?: string; tokenStatus?: number; revokeStatus?: number; + revokeReject?: boolean; appPermissions?: Record; installationPermissions?: Record; logText?: string; @@ -157,6 +158,7 @@ function createStubFetch(options: { token = 'ghs_stub_token_value', tokenStatus = 201, revokeStatus = 204, + revokeReject = false, appPermissions = { contents: 'write', pull_requests: 'write' }, installationPermissions = appPermissions, logText = 'stub log\n', @@ -240,7 +242,7 @@ function createStubFetch(options: { return Promise.resolve(jsonResponse({ permissions: installationPermissions })); } if (url.endsWith('/installation/token')) { - return Promise.resolve(jsonResponse({}, revokeStatus)); + return revokeReject ? Promise.reject(new Error('network down')) : Promise.resolve(jsonResponse({}, revokeStatus)); } if (url.includes('/access_tokens')) { if (tokenStatus !== 201) return Promise.resolve(jsonResponse({ message: 'Bad credentials' }, tokenStatus)); @@ -319,11 +321,68 @@ describe('createGithubAppProvider', () => { assert.equal(result?.code, 0); assert.ok(logs.some((line) => line.includes('could not revoke the action token'))); assert.ok(logs.some((line) => line.includes('500'))); + assert.match(result?.warning ?? '', /expires on its own/); } finally { rmSync(dir, { recursive: true, force: true }); } }); + it('reports a rejected action-token revoke', async () => { + const dir = mkdtempSync(join(tmpdir(), 'gcb-action-revoke-reject-')); + const gh = join(dir, 'gh'); + writeFileSync(gh, '#!/bin/sh\nexit 0\n'); + chmodSync(gh, 0o755); + const permissions = { pull_requests: 'read', contents: 'read', checks: 'read', statuses: 'read' }; + const stub = createStubFetch({ + installations: [{ id: 42, account: { login: 'Acme' } }], + nowMs, + appPermissions: permissions, + installationPermissions: permissions, + revokeReject: true, + }); + const logs: string[] = []; + const provider = providerFor(stub, {}, gh, (message) => logs.push(message)); + try { + const result = await provider.runAction?.({ + host: 'github.com', + owner: 'acme', + repo: 'widget', + full: 'acme/widget', + action: 'pr:view', + values: { number: 7 }, + }); + assert.match(result?.warning ?? '', /expires on its own/); + assert.ok(logs.some((line) => line.includes('network down'))); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('returns setup failures and a failed-revoke warning together', async () => { + const permissions = { pull_requests: 'read', contents: 'read', checks: 'read', statuses: 'read' }; + const stub = createStubFetch({ + installations: [{ id: 42, account: { login: 'Acme' } }], + nowMs, + appPermissions: permissions, + installationPermissions: permissions, + revokeStatus: 500, + }); + const logs: string[] = []; + // A NUL makes spawn throw synchronously; an ordinary bad path would be returned as result.error. + const provider = providerFor(stub, {}, '\0', (message) => logs.push(message)); + const result = await provider.runAction?.({ + host: 'github.com', + owner: 'acme', + repo: 'widget', + full: 'acme/widget', + action: 'pr:view', + values: { number: 7 }, + }); + assert.ok(result?.error); + assert.match(result?.warning ?? '', /expires on its own/); + assert.ok(logs.some((line) => line.includes('could not revoke the action token'))); + }); + it('runs an action with an action token and revokes it', async () => { const dir = mkdtempSync(join(tmpdir(), 'gcb-action-')); const gh = join(dir, 'gh');