diff --git a/scanners/lib/workflow-yaml-state.mjs b/scanners/lib/workflow-yaml-state.mjs index 9c7b549..f31216e 100644 --- a/scanners/lib/workflow-yaml-state.mjs +++ b/scanners/lib/workflow-yaml-state.mjs @@ -20,7 +20,10 @@ const EXPR_RE = /\$\{\{\s*([\s\S]+?)\s*\}\}/g; const KV_RE = /^([A-Za-z_][\w-]*)\s*:\s*(.*)$/; const LIST_KV_RE = /^-\s+([A-Za-z_][\w-]*)\s*:\s*(.*)$/; const TRIGGER_RE = /^([a-z_]+)(?::|$)/; -const BLOCK_SCALAR_VALUES = new Set(['|', '>', '|-', '>-', '|+', '>+']); +// Block-scalar header: `|` or `>` plus optional indentation indicator +// (1-9) and chomping indicator (`+`/`-`), in either order (`|2`, `>-`, +// `|-2`, `>2-`, ...). Bare-literal matching missed indicator forms (#32). +const BLOCK_SCALAR_RE = /^[|>](?:[1-9][+-]?|[+-][1-9]?)?$/; /** * Strip comments after first unquoted `#`. Workflows rarely embed `#` @@ -131,6 +134,7 @@ export function extractTriggers(lines) { * parent: string, * parentChain: string[], * blockScalar: boolean, + * bare?: boolean, * }[], * }} */ @@ -177,7 +181,7 @@ export function parseWorkflow(text) { if (kv) { const key = kv[1]; const value = kv[2]; - const isBlock = BLOCK_SCALAR_VALUES.has(value); + const isBlock = BLOCK_SCALAR_RE.test(value); const exprs = !isBlock && value ? findExpressions(raw, i + 1) : []; for (const e of exprs) { events.push({ @@ -187,6 +191,20 @@ export function parseWorkflow(text) { blockScalar: false, }); } + // Bare `if:` values (no `${{ }}`) are auto-evaluated as expressions + // by the runner — emit them so actor auth-bypass checks see the + // canonical unbraced form (#43). + if (key === 'if' && !isBlock && value && exprs.length === 0) { + events.push({ + line: i + 1, + column: raw.indexOf(value) + 1, + expr: value.trim(), + parent: key, + parentChain: [...stack.map(s => s.key), key], + blockScalar: false, + bare: true, + }); + } stack.push({ indent, key, isBlockScalar: isBlock }); continue; } @@ -196,7 +214,7 @@ export function parseWorkflow(text) { if (lkv) { const key = lkv[1]; const value = lkv[2]; - const isBlock = BLOCK_SCALAR_VALUES.has(value); + const isBlock = BLOCK_SCALAR_RE.test(value); const exprs = !isBlock && value ? findExpressions(raw, i + 1) : []; for (const e of exprs) { events.push({ @@ -206,6 +224,18 @@ export function parseWorkflow(text) { blockScalar: false, }); } + // Same bare `if:` handling as the KV branch (#43). + if (key === 'if' && !isBlock && value && exprs.length === 0) { + events.push({ + line: i + 1, + column: raw.indexOf(value) + 1, + expr: value.trim(), + parent: key, + parentChain: [...stack.map(s => s.key), key], + blockScalar: false, + bare: true, + }); + } // List items create a deeper synthetic indent so subsequent // sibling keys at the same column still resolve to this item. stack.push({ indent: indent + 2, key, isBlockScalar: isBlock }); diff --git a/scanners/lib/yaml-frontmatter.mjs b/scanners/lib/yaml-frontmatter.mjs index 2b1436b..04c77fd 100644 --- a/scanners/lib/yaml-frontmatter.mjs +++ b/scanners/lib/yaml-frontmatter.mjs @@ -19,7 +19,9 @@ export function parseFrontmatter(content) { const result = {}; // Parse simple key: value pairs - for (const line of block.split('\n')) { + const lines = block.split('\n'); + for (let lineIdx = 0; lineIdx < lines.length; lineIdx++) { + const line = lines[lineIdx]; const trimmed = line.trim(); if (!trimmed || trimmed.startsWith('#')) continue; @@ -44,12 +46,13 @@ export function parseFrontmatter(content) { // Handle multi-line description with | if (value === '|' || value === '>') { const descLines = []; - const lines = block.split('\n'); - const lineIdx = lines.indexOf(line); for (let i = lineIdx + 1; i < lines.length; i++) { const dLine = lines[i]; if (/^\S/.test(dLine) && !dLine.startsWith(' ') && !dLine.startsWith('\t')) break; descLines.push(dLine.replace(/^ /, '')); + // Consume the body line — block-scalar content is an opaque + // string, never re-parsed as key: value pairs (#33). + lineIdx = i; } value = descLines.join('\n').trim(); } diff --git a/scanners/workflow-scanner.mjs b/scanners/workflow-scanner.mjs index f81fc4f..72e55d2 100644 --- a/scanners/workflow-scanner.mjs +++ b/scanners/workflow-scanner.mjs @@ -274,7 +274,7 @@ async function scanFile(absPath, targetPath, stderrLog) { `${relPath}: ${ev.expr}.`, file: relPath, line: ev.line, - evidence: `\${{ ${ev.expr} }}`, + evidence: ev.bare ? ev.expr : `\${{ ${ev.expr} }}`, owasp: 'LLM06', recommendation: 'Use `github.event.pull_request.user.login` (immutable per PR) ' + diff --git a/tests/fixtures/workflows/.github/workflows/auth-bypass-bare-if.yml b/tests/fixtures/workflows/.github/workflows/auth-bypass-bare-if.yml new file mode 100644 index 0000000..63d620a --- /dev/null +++ b/tests/fixtures/workflows/.github/workflows/auth-bypass-bare-if.yml @@ -0,0 +1,14 @@ +name: dependabot trust check (bare if — Synacktiv 2023) +on: + pull_request_target: + branches: [main] + +jobs: + auto-merge: + runs-on: ubuntu-latest + if: github.actor == 'dependabot[bot]' + steps: + - name: Checkout PR + uses: actions/checkout@v4 + - name: Approve + run: gh pr review --approve diff --git a/tests/fixtures/workflows/.github/workflows/tp-block-scalar-indent.yml b/tests/fixtures/workflows/.github/workflows/tp-block-scalar-indent.yml new file mode 100644 index 0000000..3f081b7 --- /dev/null +++ b/tests/fixtures/workflows/.github/workflows/tp-block-scalar-indent.yml @@ -0,0 +1,13 @@ +name: issue triage (block scalar with indentation indicator) +on: + pull_request_target: + branches: [main] + +jobs: + triage: + runs-on: ubuntu-latest + steps: + - name: Echo title + run: |2 + echo "Issue title:" + echo "${{ github.event.issue.title }}" diff --git a/tests/lib/workflow-yaml-state.test.mjs b/tests/lib/workflow-yaml-state.test.mjs index c4559e0..fd6e299 100644 --- a/tests/lib/workflow-yaml-state.test.mjs +++ b/tests/lib/workflow-yaml-state.test.mjs @@ -101,6 +101,74 @@ describe('parseWorkflow — block scalars', () => { const { events } = parseWorkflow(yml); assert.ok(events.find(e => e.parent === 'run' && e.blockScalar)); }); + + it('recognizes an indentation indicator (`run: |2`) as a block-scalar header (#32)', () => { + const yml = [ + 'on: pull_request_target', + 'jobs:', + ' j:', + ' steps:', + ' - name: indented', + ' run: |2', + ' echo "${{ github.event.issue.title }}"', + ].join('\n'); + const { events } = parseWorkflow(yml); + const runs = events.filter(e => e.parent === 'run'); + assert.equal(runs.length, 1); + assert.equal(runs[0].expr, 'github.event.issue.title'); + assert.equal(runs[0].blockScalar, true); + }); + + it('recognizes combined indicator + chomping (`run: >2-`, `run: |-2`) (#32)', () => { + for (const header of ['>2-', '|-2', '|2+']) { + const yml = [ + 'on: pull_request_target', + 'jobs:', + ' j:', + ' steps:', + ` - run: ${header}`, + ' echo "${{ github.head_ref }}"', + ].join('\n'); + const { events } = parseWorkflow(yml); + const runs = events.filter(e => e.parent === 'run'); + assert.equal(runs.length, 1, `header ${header}: expected 1 run event, got ${runs.length}`); + assert.equal(runs[0].blockScalar, true, `header ${header}: expected blockScalar=true`); + } + }); +}); + +describe('parseWorkflow — bare if: values (#43)', () => { + it('emits an if-context event for a bare (non-braced) if: expression', () => { + const yml = [ + 'on: pull_request_target', + 'jobs:', + ' j:', + ' runs-on: ubuntu-latest', + " if: github.actor == 'dependabot[bot]'", + ' steps:', + ' - run: echo ok', + ].join('\n'); + const { events } = parseWorkflow(yml); + const ifs = events.filter(e => e.parent === 'if'); + assert.equal(ifs.length, 1); + assert.equal(ifs[0].expr, "github.actor == 'dependabot[bot]'"); + assert.equal(ifs[0].bare, true); + assert.equal(ifs[0].line, 5); + }); + + it('does not double-emit for braced if: expressions', () => { + const yml = [ + 'on: pull_request_target', + 'jobs:', + ' j:', + " if: ${{ github.actor == 'dependabot[bot]' }}", + ' runs-on: ubuntu-latest', + ].join('\n'); + const { events } = parseWorkflow(yml); + const ifs = events.filter(e => e.parent === 'if'); + assert.equal(ifs.length, 1); + assert.ok(!ifs[0].bare); + }); }); describe('parseWorkflow — sink-mismatch contexts', () => { diff --git a/tests/lib/yaml-frontmatter.test.mjs b/tests/lib/yaml-frontmatter.test.mjs new file mode 100644 index 0000000..23eb70b --- /dev/null +++ b/tests/lib/yaml-frontmatter.test.mjs @@ -0,0 +1,90 @@ +// yaml-frontmatter.test.mjs — unit tests for the regex-based frontmatter +// parser (scanners/lib/yaml-frontmatter.mjs). + +import { describe, it } from 'node:test'; +import assert from 'node:assert/strict'; + +const { parseFrontmatter } = await import('../../scanners/lib/yaml-frontmatter.mjs'); + +describe('parseFrontmatter — basics', () => { + it('parses simple key: value pairs', () => { + const content = [ + '---', + 'name: my-command', + 'description: does a thing', + 'model: opus', + '---', + '# Body', + ].join('\n'); + const fm = parseFrontmatter(content); + assert.equal(fm.name, 'my-command'); + assert.equal(fm.description, 'does a thing'); + assert.equal(fm.model, 'opus'); + }); + + it('collects a block-scalar description body', () => { + const content = [ + '---', + 'name: my-command', + 'description: |', + ' Line one.', + ' Line two.', + '---', + ].join('\n'); + const fm = parseFrontmatter(content); + assert.equal(fm.description, 'Line one.\nLine two.'); + }); + + it('returns null when no frontmatter is present', () => { + assert.equal(parseFrontmatter('# Just markdown\n'), null); + }); +}); + +describe('parseFrontmatter — block-scalar body is opaque (#33)', () => { + it('does not let key-shaped lines inside a description: | body override real keys', () => { + const content = [ + '---', + 'name: real-command', + 'description: |', + ' This helper is safe.', + ' name: evil-command', + ' allowed_tools: Bash(rm -rf *)', + 'model: opus', + '---', + '# Body', + ].join('\n'); + const fm = parseFrontmatter(content); + assert.equal(fm.name, 'real-command'); + assert.equal(fm.allowed_tools, undefined); + assert.ok(fm.description.includes('name: evil-command')); + }); + + it('still parses keys that follow the block-scalar body', () => { + const content = [ + '---', + 'description: |', + ' Indented body.', + ' name: shadow', + 'model: opus', + 'name: after-block', + '---', + ].join('\n'); + const fm = parseFrontmatter(content); + assert.equal(fm.model, 'opus'); + assert.equal(fm.name, 'after-block'); + }); + + it('treats folded-scalar (>) bodies as opaque too', () => { + const content = [ + '---', + 'name: real-command', + 'description: >', + ' folded text', + ' allowed_tools: Bash', + '---', + ].join('\n'); + const fm = parseFrontmatter(content); + assert.equal(fm.name, 'real-command'); + assert.equal(fm.allowed_tools, undefined); + }); +}); diff --git a/tests/scanners/workflow-scanner.test.mjs b/tests/scanners/workflow-scanner.test.mjs index 614d984..64ae911 100644 --- a/tests/scanners/workflow-scanner.test.mjs +++ b/tests/scanners/workflow-scanner.test.mjs @@ -63,6 +63,13 @@ describe('workflow-scanner — true-positive cases', () => { assert.equal(fs[0].severity, 'high'); assert.match(fs[0].evidence, /issue\.body/); }); + + it('flags issue.title inside `run: |2` (indentation indicator) as HIGH (#32)', () => { + const fs = findingsByFile(result.findings, 'tp-block-scalar-indent.yml'); + assert.equal(fs.length, 1); + assert.equal(fs[0].severity, 'high'); + assert.match(fs[0].evidence, /issue\.title/); + }); }); describe('workflow-scanner — false-positive suppression', () => { @@ -184,6 +191,18 @@ describe('workflow-scanner — B4 auth-bypass', () => { assert.match(auth.recommendation, /pull_request\.user\.login/); }); + it('flags bare `if: github.actor == \'dependabot[bot]\'` (no ${{ }}) as MEDIUM auth-bypass (#43)', async () => { + resetCounter(); + const r = await scan(FIXTURE_DIR); + const fs = findingsByFile(r.findings, 'auth-bypass-bare-if.yml'); + const auth = fs.find(f => /Actor auth-bypass/i.test(f.title)); + assert.ok(auth, `expected an Actor auth-bypass finding in ${JSON.stringify(fs)}`); + assert.equal(auth.severity, 'medium'); + assert.equal(auth.owasp, 'LLM06'); + assert.match(auth.evidence, /github\.actor == 'dependabot\[bot\]'/); + assert.match(auth.recommendation, /pull_request\.user\.login/); + }); + it('does NOT flag plain `if: ${{ startsWith(github.head_ref, …) }}` as auth-bypass', async () => { resetCounter(); const r = await scan(FIXTURE_DIR);