From 08b66a736480a5725c5682caf57c540c7b83923b Mon Sep 17 00:00:00 2001 From: Adam Poit Date: Sat, 18 Jul 2026 16:42:18 -0700 Subject: [PATCH] Validate wrapped GitHub App token flows --- docs/configuration.md | 4 +- src/doctor.ts | 115 +++++++++++++++++++++++++++--------------- tests/doctor.test.ts | 107 ++++++++++++++++++++++++++++++++++----- 3 files changed, 170 insertions(+), 56 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index f68da5d..de1a234 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -59,6 +59,8 @@ The generated workflows require: The App installation must grant Contents read/write and Workflows write. Enable Issues read/write when GitHub issue notifications are configured. Patchlane requests these permissions explicitly when creating each short-lived token, passes the token to checkout and `gh` as `GH_TOKEN`, and leaves the built-in `GITHUB_TOKEN` read-only. +Adapted workflows may instead use a composite action that creates the App token and exposes it as `outputs.token`. Checkout and every `patchlane sync`, `promote`, or `notify` step in a job must consume the same `${{ steps..outputs.token }}` expression. No wrapper details belong in `.patchlane.yml`. Doctor validates this token flow but cannot inspect the wrapper's internal permission requests or credentials; use `verify-auth` for runtime validation. Generated workflows continue to use `actions/create-github-app-token` directly with the standard credentials and explicit least-privilege permissions. + A GitHub App or user token is required for pushes that must start another workflow. GitHub deliberately suppresses most workflow events caused by the built-in `GITHUB_TOKEN`; increasing its workflow permissions does not change that behavior. See [Manual setup](manual-setup.md) for App creation and repository configuration. ## Commands @@ -83,7 +85,7 @@ npx patchlane doctor npx patchlane doctor --json ``` -Doctor checks source resolution, remote patch refs, patch bases, composed workflow configuration and policy, CI triggers, App-token wiring, permissions, and bootstrap state without changing repository state. For GitHub origins it also attempts to inspect Actions enablement and the names—not values—of the expected repository variable and secret. Insufficient metadata access is reported as a warning. +Doctor checks source resolution, remote patch refs, patch bases, composed workflow configuration and policy, CI triggers, App-token wiring, permissions, and bootstrap state without changing repository state. For GitHub origins it also attempts to inspect Actions enablement. When a job uses `actions/create-github-app-token` directly, Doctor additionally inspects the names—not values—of the expected repository variable and secret and strictly validates the action's credential and permission inputs. These direct-provider metadata checks are skipped for wrapper-only workflows. Insufficient metadata access is reported as a warning. ### Verify workflow authentication diff --git a/src/doctor.ts b/src/doctor.ts index bf85923..c48899b 100644 --- a/src/doctor.ts +++ b/src/doctor.ts @@ -6,11 +6,7 @@ import { parse } from 'yaml'; import { loadPatchlaneConfig, type PatchlaneConfig } from './config.js'; import { parseUpstreamSource } from './upstream-source.js'; import { validateWorkflowPolicy } from './workflow-policy.js'; -import { - GITHUB_APP_CLIENT_ID_VARIABLE, - GITHUB_APP_PRIVATE_KEY_SECRET, - GITHUB_APP_TOKEN_STEP_ID, -} from './workflow-templates.js'; +import { GITHUB_APP_CLIENT_ID_VARIABLE, GITHUB_APP_PRIVATE_KEY_SECRET } from './workflow-templates.js'; export type DoctorCheck = { severity: 'error' | 'warning' | 'info'; @@ -249,61 +245,87 @@ function jobSteps(job: Record | undefined) { return Array.isArray(steps) ? steps.flatMap((step) => (objectValue(step) ? [objectValue(step)!] : [])) : []; } +type AuthenticationProvider = 'direct' | 'wrapper'; + export function inspectAuthenticatedJob( workflowFile: string, jobName: string, job: Record | undefined, requirements: { contents: 'read' | 'write'; workflows?: boolean; issues?: boolean }, checks: DoctorCheck[], -) { +): AuthenticationProvider | undefined { if (!job) { checks.push({ severity: 'error', message: `${workflowFile} must define the '${jobName}' job.` }); - return; + return undefined; } const steps = jobSteps(job); - const tokenStep = steps.find( - (step) => - step.id === GITHUB_APP_TOKEN_STEP_ID && - typeof step.uses === 'string' && - step.uses.startsWith('actions/create-github-app-token@'), - ); - const tokenWith = objectValue(tokenStep?.with); - const expectedToken = `\${{ steps.${GITHUB_APP_TOKEN_STEP_ID}.outputs.token }}`; - const expectedClientId = `\${{ vars.${GITHUB_APP_CLIENT_ID_VARIABLE} }}`; - const expectedPrivateKey = `\${{ secrets.${GITHUB_APP_PRIVATE_KEY_SECRET} }}`; - if ( - !tokenWith || - tokenWith['client-id'] !== expectedClientId || - tokenWith['private-key'] !== expectedPrivateKey || - tokenWith['permission-contents'] !== requirements.contents || - (requirements.workflows && tokenWith['permission-workflows'] !== 'write') || - (requirements.issues && tokenWith['permission-issues'] !== 'write') - ) { - const permissions = [ - `contents: ${requirements.contents}`, - requirements.workflows ? 'workflows: write' : '', - requirements.issues ? 'issues: write' : '', - ] - .filter(Boolean) - .join(', '); + const checkout = steps.find((step) => typeof step.uses === 'string' && step.uses.startsWith('actions/checkout@')); + const checkoutToken = objectValue(checkout?.with)?.token; + const tokenExpression = + typeof checkoutToken === 'string' + ? checkoutToken.match(/^\$\{\{\s*steps\.([A-Za-z_][A-Za-z0-9_-]*)\.outputs\.token\s*}}$/) + : undefined; + if (!tokenExpression) { checks.push({ severity: 'error', - message: `${workflowFile} job '${jobName}' must create a Patchlane GitHub App token with ${permissions}.`, + message: `${workflowFile} job '${jobName}' must check out with a GitHub App token expression in the form \${{ steps..outputs.token }}.`, }); + return undefined; } - const checkout = steps.find((step) => typeof step.uses === 'string' && step.uses.startsWith('actions/checkout@')); - if (objectValue(checkout?.with)?.token !== expectedToken) { + const tokenStepId = tokenExpression[1]; + const tokenStep = steps.find((step) => step.id === tokenStepId); + if (!tokenStep) { checks.push({ severity: 'error', - message: `${workflowFile} job '${jobName}' must check out with the Patchlane GitHub App token.`, + message: `${workflowFile} job '${jobName}' checkout references missing token producer step '${tokenStepId}'.`, + }); + return undefined; + } + if (typeof tokenStep.uses !== 'string') { + checks.push({ + severity: 'error', + message: `${workflowFile} job '${jobName}' token producer step '${tokenStepId}' must use an action.`, + }); + return undefined; + } + + const directProvider = tokenStep.uses.startsWith('actions/create-github-app-token@'); + if (directProvider) { + const tokenWith = objectValue(tokenStep.with); + const expectedClientId = `\${{ vars.${GITHUB_APP_CLIENT_ID_VARIABLE} }}`; + const expectedPrivateKey = `\${{ secrets.${GITHUB_APP_PRIVATE_KEY_SECRET} }}`; + if ( + !tokenWith || + tokenWith['client-id'] !== expectedClientId || + tokenWith['private-key'] !== expectedPrivateKey || + tokenWith['permission-contents'] !== requirements.contents || + (requirements.workflows && tokenWith['permission-workflows'] !== 'write') || + (requirements.issues && tokenWith['permission-issues'] !== 'write') + ) { + const permissions = [ + `contents: ${requirements.contents}`, + requirements.workflows ? 'workflows: write' : '', + requirements.issues ? 'issues: write' : '', + ] + .filter(Boolean) + .join(', '); + checks.push({ + severity: 'error', + message: `${workflowFile} job '${jobName}' must create a Patchlane GitHub App token with ${permissions}.`, + }); + } + } else { + checks.push({ + severity: 'info', + message: `${workflowFile} job '${jobName}' uses token wrapper '${tokenStep.uses}'; its internal GitHub App permissions cannot be verified statically. Run patchlane verify-auth to validate the token at runtime.`, }); } for (const step of steps.filter( (step) => typeof step.run === 'string' && /\bpatchlane(?:@[^\s]+)?\s+(?:sync|promote|notify)\b/.test(step.run), )) { - if (objectValue(step.env)?.GH_TOKEN !== expectedToken) { + if (objectValue(step.env)?.GH_TOKEN !== checkoutToken) { checks.push({ severity: 'error', message: `${workflowFile} job '${jobName}' must pass the Patchlane GitHub App token as GH_TOKEN.`, @@ -311,9 +333,11 @@ export function inspectAuthenticatedJob( break; } } + return directProvider ? 'direct' : 'wrapper'; } function inspectWorkflows(config: PatchlaneConfig, sourceSha: string | undefined, cwd: string, checks: DoctorCheck[]) { + const authenticationProviders: AuthenticationProvider[] = []; const files = workflowFiles(config, sourceSha, cwd); for (const violation of validateWorkflowPolicy(config.allowedWorkflows, files)) { checks.push({ severity: 'error', message: violation.message }); @@ -328,13 +352,14 @@ function inspectWorkflows(config: PatchlaneConfig, sourceSha: string | undefined const promotionNotifications = config.notifications?.githubIssues.events.includes('promotion-failed') ?? false; if (!sync?.workflow) checks.push({ severity: 'error', message: 'Missing .github/workflows/sync-upstream.yml.' }); else { - inspectAuthenticatedJob( + const provider = inspectAuthenticatedJob( '.github/workflows/sync-upstream.yml', 'fork-sync', workflowJob(sync.workflow, 'fork-sync'), { contents: 'write', workflows: true, issues: syncNotifications }, checks, ); + if (provider) authenticationProviders.push(provider); const workflowDispatch = objectValue(eventConfig(sync.workflow, 'workflow_dispatch')); if (!objectValue(workflowDispatch?.inputs)?.verification_id) { checks.push({ @@ -346,7 +371,7 @@ function inspectWorkflows(config: PatchlaneConfig, sourceSha: string | undefined if (!promotion?.workflow) { checks.push({ severity: 'error', message: 'Missing .github/workflows/promote-tested-sync.yml.' }); } else { - inspectAuthenticatedJob( + const promotionProvider = inspectAuthenticatedJob( '.github/workflows/promote-tested-sync.yml', 'promote', workflowJob(promotion.workflow, 'promote'), @@ -359,14 +384,16 @@ function inspectWorkflows(config: PatchlaneConfig, sourceSha: string | undefined }, checks, ); + if (promotionProvider) authenticationProviders.push(promotionProvider); if (ciNotifications) { - inspectAuthenticatedJob( + const notificationProvider = inspectAuthenticatedJob( '.github/workflows/promote-tested-sync.yml', 'notify-ci-failure', workflowJob(promotion.workflow, 'notify-ci-failure'), { contents: 'read', issues: true }, checks, ); + if (notificationProvider) authenticationProviders.push(notificationProvider); } const workflowRun = eventConfig(promotion.workflow, 'workflow_run'); const workflows = @@ -394,6 +421,7 @@ function inspectWorkflows(config: PatchlaneConfig, sourceSha: string | undefined } } } + return authenticationProviders; } function inspectPatchRefs(config: PatchlaneConfig, sourceSha: string | undefined, cwd: string, checks: DoctorCheck[]) { @@ -437,6 +465,7 @@ export function inspectGitHubAutomation( cwd: string, checks: DoctorCheck[], runCommand: typeof run = run, + inspectDirectProviderCredentials = true, ) { if (!repository) return; @@ -452,6 +481,8 @@ export function inspectGitHubAutomation( }); } + if (!inspectDirectProviderCredentials) return; + const variables = runCommand( 'gh', ['api', '--paginate', `repos/${repository}/actions/variables?per_page=100`, '--jq', '.variables[].name'], @@ -536,9 +567,9 @@ export function runDoctor(options: DoctorOptions = {}): DoctorReport { if (resolved) checks.push({ severity: 'info', message: `Resolved ${config.source} to ${resolved.label} @ ${resolved.sha}.` }); inspectPatchRefs(config, resolved?.sha, cwd, checks); - inspectWorkflows(config, resolved?.sha, cwd, checks); + const authenticationProviders = inspectWorkflows(config, resolved?.sha, cwd, checks); const repository = githubRepository(remoteUrl(cwd, 'origin')); - inspectGitHubAutomation(repository, cwd, checks); + inspectGitHubAutomation(repository, cwd, checks, run, authenticationProviders.includes('direct')); inspectNotificationAssignees(config, cwd, checks); inspectBootstrap(config, cwd, checks); diff --git a/tests/doctor.test.ts b/tests/doctor.test.ts index 2b6a53f..24f0383 100644 --- a/tests/doctor.test.ts +++ b/tests/doctor.test.ts @@ -46,30 +46,33 @@ function validTokenWith() { function authenticatedJob( options: { includeTokenStep?: boolean; + tokenStepId?: string; + tokenUses?: string; tokenWith?: Record; checkoutToken?: string; ghToken?: string; } = {}, ) { - const tokenWith = options.tokenWith ?? validTokenWith(); + const tokenStepId = options.tokenStepId ?? 'patchlane-token'; + const token = `\${{ steps.${tokenStepId}.outputs.token }}`; return { steps: [ ...(options.includeTokenStep === false ? [] : [ { - id: 'patchlane-token', - uses: 'actions/create-github-app-token@v3', - with: tokenWith, + id: tokenStepId, + uses: options.tokenUses ?? 'actions/create-github-app-token@v3', + with: options.tokenWith ?? validTokenWith(), }, ]), { uses: 'actions/checkout@v4', - with: { token: options.checkoutToken ?? appToken }, + with: { token: options.checkoutToken ?? token }, }, { run: 'npx patchlane@1.2.3 sync', - env: { GH_TOKEN: options.ghToken ?? appToken }, + env: { GH_TOKEN: options.ghToken ?? token }, }, ], }; @@ -115,12 +118,61 @@ describe('inspectAuthenticatedJob', () => { ]); }); - test('reports a missing App token step', () => { + test('reports a missing token producer step', () => { expect(inspectJob(authenticatedJob({ includeTokenStep: false }))).toContainEqual( - expect.objectContaining({ message: expect.stringContaining('must create a Patchlane GitHub App token') }), + expect.objectContaining({ + message: expect.stringContaining("missing token producer step 'patchlane-token'"), + }), + ); + }); + + test('retains strict validation for a direct token provider', () => { + expect(inspectJob(authenticatedJob())).toEqual([]); + }); + + test('accepts a wrapper action when checkout and Patchlane use its token output', () => { + const checks = inspectJob( + authenticatedJob({ tokenStepId: 'not-adam', tokenUses: 'adampoit/not-adam@v1', tokenWith: {} }), + ); + expect(checks).toEqual([ + { + severity: 'info', + message: expect.stringContaining( + "uses token wrapper 'adampoit/not-adam@v1'; its internal GitHub App permissions cannot be verified statically", + ), + }, + ]); + }); + + test('reports a token output expression without a producer', () => { + expect( + inspectJob( + authenticatedJob({ + includeTokenStep: false, + checkoutToken: '${{ steps.missing.outputs.token }}', + ghToken: '${{ steps.missing.outputs.token }}', + }), + ), + ).toContainEqual(expect.objectContaining({ message: expect.stringContaining("producer step 'missing'") })); + }); + + test('reports a non-action token producer', () => { + const job = authenticatedJob() as { steps: Record[] }; + job.steps[0] = { id: 'patchlane-token', run: 'echo token' }; + expect(inspectJob(job)).toContainEqual( + expect.objectContaining({ message: expect.stringContaining('must use an action') }), ); }); + test.each(['${{ github.token }}', '${{ secrets.GITHUB_TOKEN }}', '${{ steps.patchlane-token.outputs.other }}'])( + 'reports invalid checkout token expression %s', + (checkoutToken) => { + expect(inspectJob(authenticatedJob({ checkoutToken }))).toContainEqual( + expect.objectContaining({ message: expect.stringContaining('steps..outputs.token') }), + ); + }, + ); + test.each([ ['client ID', { 'client-id': '${{ vars.WRONG_CLIENT_ID }}' }], ['private key', { 'private-key': '${{ secrets.WRONG_PRIVATE_KEY }}' }], @@ -134,11 +186,17 @@ describe('inspectAuthenticatedJob', () => { ]); }); - test('reports checkout that does not use the App token', () => { - expect(inspectJob(authenticatedJob({ checkoutToken: '${{ github.token }}' }))).toContainEqual( - expect.objectContaining({ - message: expect.stringContaining('must check out with the Patchlane GitHub App token'), - }), + test('reports a wrapper token mismatch between checkout and Patchlane', () => { + expect( + inspectJob( + authenticatedJob({ + tokenStepId: 'not-adam', + tokenUses: 'adampoit/not-adam@v1', + ghToken: appToken, + }), + ), + ).toContainEqual( + expect.objectContaining({ message: expect.stringContaining('must pass the Patchlane GitHub App token') }), ); }); @@ -147,6 +205,17 @@ describe('inspectAuthenticatedJob', () => { expect.objectContaining({ message: expect.stringContaining('must pass the Patchlane GitHub App token') }), ); }); + + test('checks every Patchlane command in the job', () => { + const job = authenticatedJob() as { steps: Record[] }; + job.steps.push({ + run: 'npx patchlane notify --event=sync-failed', + env: { GH_TOKEN: '${{ steps.other.outputs.token }}' }, + }); + expect(inspectJob(job)).toContainEqual( + expect.objectContaining({ message: expect.stringContaining('must pass the Patchlane GitHub App token') }), + ); + }); }); describe('inspectGitHubAutomation', () => { @@ -194,6 +263,18 @@ describe('inspectGitHubAutomation', () => { }, ]); }); + + test('skips standard credential metadata checks for wrapper providers', () => { + const checks: DoctorCheck[] = []; + const endpoints: string[] = []; + const runner = (_command: string, args: string[]) => { + endpoints.push(args.find((arg) => arg.startsWith('repos/')) ?? ''); + return { status: 0, stdout: 'true', stderr: '' }; + }; + inspectGitHubAutomation('example/fork', '/tmp/fork', checks, runner, false); + expect(checks).toEqual([]); + expect(endpoints).toEqual(['repos/example/fork/actions/permissions']); + }); }); test('composes non-overlapping changes to the same workflow from independent patches', () => {