diff --git a/.github/workflows/ci-pr-quality.yml b/.github/workflows/ci-pr-quality.yml index 33bad80fcd9..f13c783c43a 100644 --- a/.github/workflows/ci-pr-quality.yml +++ b/.github/workflows/ci-pr-quality.yml @@ -134,6 +134,7 @@ jobs: pnpm-workspace.yaml .code-health-baseline.json packages/testing/code-health/** + .github/workflows/** check-static-analysis: name: Static Analysis diff --git a/packages/testing/code-health/src/index.ts b/packages/testing/code-health/src/index.ts index fc89d367b1e..89fea1bd148 100644 --- a/packages/testing/code-health/src/index.ts +++ b/packages/testing/code-health/src/index.ts @@ -3,9 +3,11 @@ import type { RuleSettingsMap } from '@n8n/rules-engine'; import type { CodeHealthContext } from './context.js'; import { CatalogViolationsRule } from './rules/catalog-violations.rule.js'; +import { WorkflowPrTargetSafetyRule } from './rules/workflow-pr-target-safety.rule.js'; export type { CodeHealthContext } from './context.js'; export { CatalogViolationsRule } from './rules/catalog-violations.rule.js'; +export { WorkflowPrTargetSafetyRule } from './rules/workflow-pr-target-safety.rule.js'; const defaultRuleSettings: RuleSettingsMap = { 'catalog-violations': { @@ -13,6 +15,11 @@ const defaultRuleSettings: RuleSettingsMap = { severity: 'error', options: { workspaceFile: 'pnpm-workspace.yaml' }, }, + 'workflow-pr-target-safety': { + enabled: true, + severity: 'error', + options: { allowedWorkflows: ['ci-cla-check.yml'] }, + }, }; function mergeSettings(defaults: RuleSettingsMap, overrides?: RuleSettingsMap): RuleSettingsMap { @@ -31,6 +38,7 @@ function mergeSettings(defaults: RuleSettingsMap, overrides?: RuleSettingsMap): export function createDefaultRunner(settings?: RuleSettingsMap): RuleRunner { const runner = new RuleRunner(); runner.registerRule(new CatalogViolationsRule()); + runner.registerRule(new WorkflowPrTargetSafetyRule()); runner.applySettings(mergeSettings(defaultRuleSettings, settings)); return runner; } diff --git a/packages/testing/code-health/src/rules/workflow-pr-target-safety.rule.test.ts b/packages/testing/code-health/src/rules/workflow-pr-target-safety.rule.test.ts new file mode 100644 index 00000000000..18b080a7676 --- /dev/null +++ b/packages/testing/code-health/src/rules/workflow-pr-target-safety.rule.test.ts @@ -0,0 +1,267 @@ +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; + +import type { CodeHealthContext } from '../context.js'; +import { WorkflowPrTargetSafetyRule } from './workflow-pr-target-safety.rule.js'; + +function createTempDir(): string { + return fs.mkdtempSync(path.join(os.tmpdir(), 'code-health-workflow-test-')); +} + +function writeWorkflow(dir: string, name: string, content: string): void { + const fullPath = path.join(dir, '.github', 'workflows', name); + fs.mkdirSync(path.dirname(fullPath), { recursive: true }); + fs.writeFileSync(fullPath, content); +} + +describe('WorkflowPrTargetSafetyRule', () => { + let tmpDir: string; + let rule: WorkflowPrTargetSafetyRule; + + beforeEach(() => { + tmpDir = createTempDir(); + rule = new WorkflowPrTargetSafetyRule(); + rule.configure({ options: { allowedWorkflows: ['ci-cla-check.yml'] } }); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + function context(): CodeHealthContext { + return { rootDir: tmpDir }; + } + + it('ignores workflows that only use pull_request', async () => { + writeWorkflow( + tmpDir, + 'safe.yml', + ` +name: Safe +on: + pull_request: + types: [opened] +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + ref: \${{ github.event.pull_request.head.sha }} +`, + ); + + const violations = await rule.analyze(context()); + + expect(violations).toHaveLength(0); + }); + + it('flags any non-allowlisted workflow that uses pull_request_target', async () => { + writeWorkflow( + tmpDir, + 'risky.yml', + ` +name: Risky +on: + pull_request_target: + types: [opened] +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 +`, + ); + + const violations = await rule.analyze(context()); + + expect(violations).toHaveLength(1); + expect(violations[0].rule).toBe('workflow-pr-target-safety'); + expect(violations[0].message).toContain('pull_request_target'); + expect(violations[0].message).toContain('Prefer pull_request'); + }); + + it('flags pull_request_target even when on: is a list', async () => { + writeWorkflow( + tmpDir, + 'list-trigger.yml', + ` +name: List +on: [pull_request_target] +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 +`, + ); + + const violations = await rule.analyze(context()); + + expect(violations).toHaveLength(1); + }); + + it('allows pull_request_target in an allowlisted workflow with no checkout override', async () => { + writeWorkflow( + tmpDir, + 'ci-cla-check.yml', + ` +name: CLA Check +on: + pull_request_target: + types: [opened] +jobs: + check: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + sparse-checkout: .github/scripts/cla +`, + ); + + const violations = await rule.analyze(context()); + + expect(violations).toHaveLength(0); + }); + + it('flags allowlisted workflow that checks out PR head sha', async () => { + writeWorkflow( + tmpDir, + 'ci-cla-check.yml', + ` +name: CLA Check +on: + pull_request_target: + types: [opened] +jobs: + check: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + ref: \${{ github.event.pull_request.head.sha }} +`, + ); + + const violations = await rule.analyze(context()); + + expect(violations).toHaveLength(1); + expect(violations[0].message).toContain('PR-author-controlled ref'); + expect(violations[0].message).toContain('their code, our keys'); + }); + + it('flags allowlisted workflow that checks out github.head_ref', async () => { + writeWorkflow( + tmpDir, + 'ci-cla-check.yml', + ` +name: CLA Check +on: + pull_request_target: +jobs: + check: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + ref: \${{ github.head_ref }} +`, + ); + + const violations = await rule.analyze(context()); + + expect(violations).toHaveLength(1); + expect(violations[0].message).toContain('PR-author-controlled ref'); + }); + + it('flags allowlisted workflow that checks out PR fork repository', async () => { + writeWorkflow( + tmpDir, + 'ci-cla-check.yml', + ` +name: CLA Check +on: + pull_request_target: +jobs: + check: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + repository: \${{ github.event.pull_request.head.repo.full_name }} +`, + ); + + const violations = await rule.analyze(context()); + + expect(violations).toHaveLength(1); + expect(violations[0].message).toContain('PR-author-controlled repository'); + }); + + it('flags shell git checkout of PR head ref', async () => { + writeWorkflow( + tmpDir, + 'ci-cla-check.yml', + ` +name: CLA Check +on: + pull_request_target: +jobs: + check: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: Fetch PR + run: | + git fetch origin \${{ github.event.pull_request.head.sha }} + git checkout FETCH_HEAD +`, + ); + + const violations = await rule.analyze(context()); + + expect(violations).toHaveLength(1); + expect(violations[0].message).toContain('shell git command'); + }); + + it('does not flag a checkout with no ref override', async () => { + writeWorkflow( + tmpDir, + 'ci-cla-check.yml', + ` +name: CLA Check +on: + pull_request_target: +jobs: + check: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/checkout@v4 + with: + path: vendor +`, + ); + + const violations = await rule.analyze(context()); + + expect(violations).toHaveLength(0); + }); + + it('handles workflows with no triggers without throwing', async () => { + writeWorkflow(tmpDir, 'broken.yml', 'name: Broken\njobs: {}\n'); + + const violations = await rule.analyze(context()); + + expect(violations).toHaveLength(0); + }); + + it('skips files that are not valid YAML', async () => { + writeWorkflow(tmpDir, 'invalid.yml', '::: not yaml :::\n\t- broken'); + + await expect(rule.analyze(context())).resolves.toBeDefined(); + }); +}); diff --git a/packages/testing/code-health/src/rules/workflow-pr-target-safety.rule.ts b/packages/testing/code-health/src/rules/workflow-pr-target-safety.rule.ts new file mode 100644 index 00000000000..1ebaaf7be94 --- /dev/null +++ b/packages/testing/code-health/src/rules/workflow-pr-target-safety.rule.ts @@ -0,0 +1,150 @@ +import { BaseRule } from '@n8n/rules-engine'; +import type { Violation } from '@n8n/rules-engine'; +import * as path from 'node:path'; + +import type { CodeHealthContext } from '../context.js'; +import { + findLineContaining, + findWorkflowFiles, + parseWorkflow, + type WorkflowFile, + type WorkflowJobStep, +} from '../utils/workflow-scanner.js'; + +const RISKY_TRIGGER = 'pull_request_target'; + +const UNTRUSTED_REF_EXPRESSIONS = [ + 'github.event.pull_request.head.sha', + 'github.event.pull_request.head.ref', + 'github.event.pull_request.merge_commit_sha', + 'github.head_ref', +]; + +const UNTRUSTED_REPOSITORY_EXPRESSIONS = [ + 'github.event.pull_request.head.repo.full_name', + 'github.event.pull_request.head.repo.clone_url', +]; + +export class WorkflowPrTargetSafetyRule extends BaseRule { + readonly id = 'workflow-pr-target-safety'; + readonly name = 'Workflow pull_request_target Safety'; + readonly description = + 'Disallow pull_request_target triggers. Allowlisted workflows may use it only if they do not check out PR-author-controlled code.'; + readonly severity = 'error' as const; + + async analyze(context: CodeHealthContext): Promise { + const { rootDir } = context; + const options = this.getOptions(); + const allowedWorkflows = Array.isArray(options.allowedWorkflows) + ? (options.allowedWorkflows as string[]) + : []; + + const files = await findWorkflowFiles(rootDir); + const violations: Violation[] = []; + + for (const filePath of files) { + const workflow = parseWorkflow(filePath, rootDir); + if (!workflow) continue; + if (!workflow.triggers.includes(RISKY_TRIGGER)) continue; + + const fileName = path.basename(filePath); + if (!allowedWorkflows.includes(fileName)) { + violations.push(this.flagTrigger(workflow)); + continue; + } + + violations.push(...this.flagUnsafeCheckouts(workflow)); + } + + return violations; + } + + private flagTrigger(workflow: WorkflowFile): Violation { + return this.createViolation( + workflow.filePath, + findLineContaining(workflow.lines, RISKY_TRIGGER), + 1, + `${workflow.relativePath} uses pull_request_target. Prefer pull_request — pull_request_target runs in the base-repo context with secrets exposed, which is unsafe when combined with PR-author-controlled code.`, + 'Switch to pull_request, or if pull_request_target is required, request a security review and add this workflow to the rule allowlist.', + ); + } + + private flagUnsafeCheckouts(workflow: WorkflowFile): Violation[] { + const violations: Violation[] = []; + + for (const job of workflow.jobs) { + for (const step of job.steps) { + violations.push(...this.flagCheckoutAction(workflow, job.id, step)); + violations.push(...this.flagShellCheckout(workflow, job.id, step)); + } + } + + return violations; + } + + private flagCheckoutAction( + workflow: WorkflowFile, + jobId: string, + step: WorkflowJobStep, + ): Violation[] { + if (!step.uses?.startsWith('actions/checkout@')) return []; + const stepWith = step.with; + if (!stepWith) return []; + + const violations: Violation[] = []; + const stepLine = findLineContaining(workflow.lines, step.uses); + + const ref = typeof stepWith.ref === 'string' ? stepWith.ref : undefined; + if (ref && containsExpression(ref, UNTRUSTED_REF_EXPRESSIONS)) { + violations.push( + this.createViolation( + workflow.filePath, + findLineContaining(workflow.lines, 'ref:', stepLine), + 1, + `${workflow.relativePath} job "${jobId}" checks out PR-author-controlled ref under pull_request_target. This pattern ("their code, our keys") is the canonical pull_request_target exploit.`, + 'Remove the ref override (defaults to the base branch), or move this step to a workflow triggered by pull_request.', + ), + ); + } + + const repository = typeof stepWith.repository === 'string' ? stepWith.repository : undefined; + if (repository && containsExpression(repository, UNTRUSTED_REPOSITORY_EXPRESSIONS)) { + violations.push( + this.createViolation( + workflow.filePath, + findLineContaining(workflow.lines, 'repository:', stepLine), + 1, + `${workflow.relativePath} job "${jobId}" checks out a PR-author-controlled repository under pull_request_target.`, + 'Remove the repository override so the checkout stays on github.repository.', + ), + ); + } + + return violations; + } + + private flagShellCheckout( + workflow: WorkflowFile, + jobId: string, + step: WorkflowJobStep, + ): Violation[] { + if (!step.run) return []; + const usesGitCheckout = /\bgit\s+(checkout|fetch|pull)\b/.test(step.run); + if (!usesGitCheckout) return []; + if (!containsExpression(step.run, UNTRUSTED_REF_EXPRESSIONS)) return []; + + return [ + this.createViolation( + workflow.filePath, + findLineContaining(workflow.lines, 'run:'), + 1, + `${workflow.relativePath} job "${jobId}" runs a shell git command that fetches PR-author-controlled refs under pull_request_target.`, + 'Avoid checking out PR head from a pull_request_target workflow; do the work in a pull_request-triggered workflow instead.', + ), + ]; + } +} + +function containsExpression(value: string, needles: string[]): boolean { + return needles.some((needle) => value.includes(needle)); +} diff --git a/packages/testing/code-health/src/utils/workflow-scanner.ts b/packages/testing/code-health/src/utils/workflow-scanner.ts new file mode 100644 index 00000000000..3cb9e9ece37 --- /dev/null +++ b/packages/testing/code-health/src/utils/workflow-scanner.ts @@ -0,0 +1,105 @@ +import fg from 'fast-glob'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { parse as parseYaml } from 'yaml'; + +export interface WorkflowJobStep { + name?: string; + uses?: string; + run?: string; + with?: Record; +} + +export interface WorkflowJob { + id: string; + steps: WorkflowJobStep[]; +} + +export interface WorkflowFile { + filePath: string; + relativePath: string; + lines: string[]; + triggers: string[]; + jobs: WorkflowJob[]; +} + +export async function findWorkflowFiles(rootDir: string): Promise { + return await fg('.github/workflows/*.{yml,yaml}', { + cwd: rootDir, + absolute: true, + dot: true, + }); +} + +export function parseWorkflow(filePath: string, rootDir: string): WorkflowFile | null { + const content = fs.readFileSync(filePath, 'utf-8'); + const lines = content.split('\n'); + + let doc: unknown; + try { + doc = parseYaml(content); + } catch { + return null; + } + + if (!doc || typeof doc !== 'object') return null; + + const workflow = doc as Record; + // YAML's `on:` keyword gets parsed as boolean `true` by the yaml lib + // unless quoted, so check both keys. + const onValue = workflow.on ?? workflow.true; + + return { + filePath, + relativePath: path.relative(rootDir, filePath), + lines, + triggers: extractTriggers(onValue), + jobs: extractJobs(workflow.jobs), + }; +} + +function extractTriggers(onValue: unknown): string[] { + if (typeof onValue === 'string') return [onValue]; + if (Array.isArray(onValue)) return onValue.filter((t): t is string => typeof t === 'string'); + if (onValue && typeof onValue === 'object') return Object.keys(onValue); + return []; +} + +function extractJobs(jobsValue: unknown): WorkflowJob[] { + if (!jobsValue || typeof jobsValue !== 'object') return []; + + const jobs: WorkflowJob[] = []; + for (const [id, raw] of Object.entries(jobsValue as Record)) { + if (!raw || typeof raw !== 'object') continue; + const job = raw as Record; + const stepsValue = job.steps; + if (!Array.isArray(stepsValue)) { + jobs.push({ id, steps: [] }); + continue; + } + + const steps: WorkflowJobStep[] = []; + for (const stepRaw of stepsValue) { + if (!stepRaw || typeof stepRaw !== 'object') continue; + const step = stepRaw as Record; + steps.push({ + name: typeof step.name === 'string' ? step.name : undefined, + uses: typeof step.uses === 'string' ? step.uses : undefined, + run: typeof step.run === 'string' ? step.run : undefined, + with: + step.with && typeof step.with === 'object' + ? (step.with as Record) + : undefined, + }); + } + jobs.push({ id, steps }); + } + return jobs; +} + +export function findLineContaining(lines: string[], needle: string, startLine = 0): number { + for (let i = startLine; i < lines.length; i++) { + if (lines[i].includes(needle)) return i + 1; + } + return 1; +}