diff --git a/.github/workflows/spec-enforcer.lock.yml b/.github/workflows/spec-enforcer.lock.yml index 503c03bf53..ed1631affa 100644 --- a/.github/workflows/spec-enforcer.lock.yml +++ b/.github/workflows/spec-enforcer.lock.yml @@ -176,7 +176,7 @@ jobs: env: GH_AW_PROMPT: /tmp/gh-aw/aw-prompts/prompt.txt GH_AW_SAFE_OUTPUTS: ${{ runner.temp }}/gh-aw/safeoutputs/outputs.jsonl - GH_AW_EXPR_FE0C1E5E: ${{ github.event.inputs.enforce_all == 'true' && 'full-sweep (enforce_all)' || 'round-robin' }} + GH_AW_EXPR_5A79780F: ${{ github.event.inputs.enforce_all || 'round-robin' }} GH_AW_GITHUB_ACTOR: ${{ github.actor }} GH_AW_GITHUB_EVENT_COMMENT_ID: ${{ github.event.comment.id }} GH_AW_GITHUB_EVENT_DISCUSSION_NUMBER: ${{ github.event.discussion.number }} @@ -250,7 +250,7 @@ jobs: env: GH_AW_PROMPT: /tmp/gh-aw/aw-prompts/prompt.txt GH_AW_ENGINE_ID: "claude" - GH_AW_EXPR_FE0C1E5E: ${{ github.event.inputs.enforce_all == 'true' && 'full-sweep (enforce_all)' || 'round-robin' }} + GH_AW_EXPR_5A79780F: ${{ github.event.inputs.enforce_all || 'round-robin' }} GH_AW_GITHUB_EVENT_INPUTS_ENFORCE_ALL: ${{ github.event.inputs.enforce_all }} GH_AW_GITHUB_REPOSITORY: ${{ github.repository }} GH_AW_GITHUB_RUN_ID: ${{ github.run_id }} @@ -267,7 +267,7 @@ jobs: GH_AW_ALLOWED_EXTENSIONS: '' GH_AW_CACHE_DESCRIPTION: '' GH_AW_CACHE_DIR: '/tmp/gh-aw/cache-memory/' - GH_AW_EXPR_FE0C1E5E: ${{ github.event.inputs.enforce_all == 'true' && 'full-sweep (enforce_all)' || 'round-robin' }} + GH_AW_EXPR_5A79780F: ${{ github.event.inputs.enforce_all || 'round-robin' }} GH_AW_GITHUB_ACTOR: ${{ github.actor }} GH_AW_GITHUB_EVENT_COMMENT_ID: ${{ github.event.comment.id }} GH_AW_GITHUB_EVENT_DISCUSSION_NUMBER: ${{ github.event.discussion.number }} @@ -292,7 +292,7 @@ jobs: GH_AW_ALLOWED_EXTENSIONS: process.env.GH_AW_ALLOWED_EXTENSIONS, GH_AW_CACHE_DESCRIPTION: process.env.GH_AW_CACHE_DESCRIPTION, GH_AW_CACHE_DIR: process.env.GH_AW_CACHE_DIR, - GH_AW_EXPR_FE0C1E5E: process.env.GH_AW_EXPR_FE0C1E5E, + GH_AW_EXPR_5A79780F: process.env.GH_AW_EXPR_5A79780F, GH_AW_GITHUB_ACTOR: process.env.GH_AW_GITHUB_ACTOR, GH_AW_GITHUB_EVENT_COMMENT_ID: process.env.GH_AW_GITHUB_EVENT_COMMENT_ID, GH_AW_GITHUB_EVENT_DISCUSSION_NUMBER: process.env.GH_AW_GITHUB_EVENT_DISCUSSION_NUMBER, diff --git a/.github/workflows/spec-enforcer.md b/.github/workflows/spec-enforcer.md index de73a10aa2..396ee462f7 100644 --- a/.github/workflows/spec-enforcer.md +++ b/.github/workflows/spec-enforcer.md @@ -386,7 +386,7 @@ All tests are derived from README.md specifications, not from implementation sou ### Round-Robin State -- **Run mode**: ${{ github.event.inputs.enforce_all == 'true' && 'full-sweep (enforce_all)' || 'round-robin' }} +- **Run mode**: ${{ github.event.inputs.enforce_all || 'round-robin' }} - **Packages processed this run**: - **Next packages in rotation**: - **Total eligible packages**: N (with README.md) diff --git a/actions/setup/js/fuzz_is_safe_expression_harness.cjs b/actions/setup/js/fuzz_is_safe_expression_harness.cjs new file mode 100644 index 0000000000..3a44383c67 --- /dev/null +++ b/actions/setup/js/fuzz_is_safe_expression_harness.cjs @@ -0,0 +1,70 @@ +// @ts-check +/** + * Fuzz test harness for isSafeExpression in runtime_import.cjs. + * + * Tests the security invariants of expression validation, covering: + * - Simple allowed expressions (github.*, needs.*, steps.*, etc.) + * - Compound expressions: AND (&&), OR (||), comparisons (==, !=, <=, >=) + * - Ternary-style expressions: condition && 'value' || 'default' + * - Standalone literals: strings, numbers, booleans + * - Security boundaries: secrets.*, vars.*, runner.*, github.token + * - Dangerous prototype-pollution property names + * + * Used by tests and by Go's fuzzing framework when run as main module via stdin. + */ + +const { isSafeExpression } = require("./runtime_import.cjs"); + +/** + * Evaluates isSafeExpression and returns a structured result. + * Never throws — all errors are captured in the error field. + * @param {string} expression - The expression to test (without ${{ }}) + * @returns {{ safe: boolean, error: string | null }} + */ +function testIsSafeExpression(expression) { + try { + const safe = isSafeExpression(expression); + return { safe, error: null }; + } catch (err) { + return { safe: false, error: err instanceof Error ? err.message : String(err) }; + } +} + +/** + * Returns true when the expression is known to be unsafe regardless of + * what other sub-expressions it may contain. + * Used by the fuzzer to assert security invariants. + * @param {string} expression + * @returns {boolean} + */ +function containsUnsafeRoot(expression) { + const trimmed = expression.trim(); + // Expressions that start with a disallowed namespace are always unsafe. + // We only check "starts with" to avoid false positives on safe sub-expressions. + return /^secrets\./.test(trimmed) || /^runner\./.test(trimmed) || /^vars\./.test(trimmed) || trimmed === "github.token"; +} + +// Read input from stdin for Go-driven fuzzing +if (require.main === module) { + let input = ""; + + process.stdin.on("data", chunk => { + input += chunk; + }); + + process.stdin.on("end", () => { + try { + // Expected JSON: { expression: string } + const { expression } = JSON.parse(input); + const result = testIsSafeExpression(expression ?? ""); + process.stdout.write(JSON.stringify(result)); + process.exit(0); + } catch (err) { + const errorMsg = err instanceof Error ? err.message : String(err); + process.stdout.write(JSON.stringify({ safe: false, error: errorMsg })); + process.exit(1); + } + }); +} + +module.exports = { testIsSafeExpression, containsUnsafeRoot }; diff --git a/actions/setup/js/fuzz_is_safe_expression_harness.test.cjs b/actions/setup/js/fuzz_is_safe_expression_harness.test.cjs new file mode 100644 index 0000000000..f1c915a21d --- /dev/null +++ b/actions/setup/js/fuzz_is_safe_expression_harness.test.cjs @@ -0,0 +1,307 @@ +// @ts-check +/** + * Fuzz / property-based tests for isSafeExpression — compound expression forms. + * + * Covers the expression types added in the spec-enforcer fix: + * - Standalone literals (string, number, boolean) + * - AND (&&) compound expressions + * - Comparison expressions (==, !=, <=, >=) + * - Ternary-style AND/OR chains + * - Security boundaries: each new form must still block secrets.*, vars.*, runner.* + * + * Also contains property-based combinatorial tests that exhaustively check + * safe×safe, safe×unsafe, and unsafe×safe pairs for every binary operator. + */ + +const { testIsSafeExpression, containsUnsafeRoot } = require("./fuzz_is_safe_expression_harness.cjs"); + +// --------------------------------------------------------------------------- +// Fixtures +// --------------------------------------------------------------------------- + +/** Safe leaf expressions that must always pass `isSafeExpression`. */ +const SAFE_LEAVES = [ + "github.actor", + "github.repository", + "github.run_id", + "github.event.issue.number", + "github.event.pull_request.title", + "github.event.inputs.enforce_all", + "github.event.inputs.branch", + "needs.build.outputs.version", + "steps.test.outputs.result", + "env.NODE_VERSION", + "inputs.branch", +]; + +/** Unsafe leaf expressions that must always be blocked. */ +const UNSAFE_LEAVES = ["secrets.TOKEN", "secrets.GITHUB_TOKEN", "vars.MY_VAR", "runner.os", "runner.temp", "github.token"]; + +/** Safe literal values (standalone). */ +const SAFE_LITERALS = ["'full-sweep (enforce_all)'", "'round-robin'", '"default"', "`value`", "42", "3.14", "true", "false"]; + +/** Comparison operators. */ +const COMPARISON_OPS = ["==", "!=", "<=", ">=", "<", ">"]; + +// --------------------------------------------------------------------------- +// Standalone literals +// --------------------------------------------------------------------------- + +describe("fuzz_is_safe_expression – standalone literals", () => { + for (const lit of SAFE_LITERALS) { + it(`should allow standalone literal ${lit}`, () => { + const { safe, error } = testIsSafeExpression(lit); + expect(error).toBeNull(); + expect(safe).toBe(true); + }); + } + + it("should reject standalone literal containing nested expression", () => { + expect(testIsSafeExpression("'${{ secrets.TOKEN }}'").safe).toBe(false); + expect(testIsSafeExpression("'text }} end'").safe).toBe(false); + }); + + it("should reject standalone literal containing hex escape sequences", () => { + // Note: in JS source, \\x41 is a two-character string: backslash + x41 + expect(testIsSafeExpression("'\\x41-injection'").safe).toBe(false); + expect(testIsSafeExpression("'\\u0041-injection'").safe).toBe(false); + }); +}); + +// --------------------------------------------------------------------------- +// Comparison expressions — allowed +// --------------------------------------------------------------------------- + +describe("fuzz_is_safe_expression – comparisons (allowed)", () => { + for (const op of COMPARISON_OPS) { + for (const left of SAFE_LEAVES) { + it(`should allow ${left} ${op} 'value'`, () => { + const { safe, error } = testIsSafeExpression(`${left} ${op} 'value'`); + expect(error).toBeNull(); + expect(safe).toBe(true); + }); + } + } +}); + +// --------------------------------------------------------------------------- +// Comparison expressions — blocked when unsafe on the left +// --------------------------------------------------------------------------- + +describe("fuzz_is_safe_expression – comparisons (unsafe left side blocked)", () => { + for (const unsafe of UNSAFE_LEAVES) { + it(`should block ${unsafe} == 'value'`, () => { + const { safe } = testIsSafeExpression(`${unsafe} == 'value'`); + expect(safe).toBe(false); + }); + } +}); + +// --------------------------------------------------------------------------- +// AND compound expressions — safe × safe → allowed (no literal operands) +// --------------------------------------------------------------------------- + +describe("fuzz_is_safe_expression – AND (safe × safe, no literals)", () => { + // Spot-check a representative subset of pairs to keep test count manageable + const pairs = SAFE_LEAVES.slice(0, 4).flatMap(a => SAFE_LEAVES.slice(0, 4).map(b => [a, b])); + for (const [a, b] of pairs) { + if (a === b) continue; + it(`should allow ${a} && ${b}`, () => { + const { safe, error } = testIsSafeExpression(`${a} && ${b}`); + expect(error).toBeNull(); + expect(safe).toBe(true); + }); + } + + it("should allow comparison && safe property (no literal in operand)", () => { + expect(testIsSafeExpression("github.event.inputs.enforce_all == 'true' && github.event.inputs.enforce_all").safe).toBe(true); + }); +}); + +// --------------------------------------------------------------------------- +// AND compound expressions — literal operands refused +// --------------------------------------------------------------------------- + +describe("fuzz_is_safe_expression – AND (literal operand blocked)", () => { + for (const lit of SAFE_LITERALS) { + it(`should block github.actor && ${lit} (literal RHS)`, () => { + expect(testIsSafeExpression(`github.actor && ${lit}`).safe).toBe(false); + }); + + it(`should block ${lit} && github.actor (literal LHS)`, () => { + expect(testIsSafeExpression(`${lit} && github.actor`).safe).toBe(false); + }); + } + + it("should block all SAFE_LEAVES && literal combinations (first 3)", () => { + for (const leaf of SAFE_LEAVES.slice(0, 3)) { + for (const lit of SAFE_LITERALS.slice(0, 3)) { + expect(testIsSafeExpression(`${leaf} && ${lit}`).safe).toBe(false); + expect(testIsSafeExpression(`${lit} && ${leaf}`).safe).toBe(false); + } + } + }); +}); + +// --------------------------------------------------------------------------- +// AND compound expressions — blocked when either side is unsafe +// --------------------------------------------------------------------------- + +describe("fuzz_is_safe_expression – AND (unsafe side blocked)", () => { + for (const unsafe of UNSAFE_LEAVES) { + it(`should block ${unsafe} && github.actor`, () => { + expect(testIsSafeExpression(`${unsafe} && github.actor`).safe).toBe(false); + }); + + it(`should block github.actor && ${unsafe}`, () => { + expect(testIsSafeExpression(`github.actor && ${unsafe}`).safe).toBe(false); + }); + } +}); + +// --------------------------------------------------------------------------- +// OR compound expressions — literal on LEFT side refused +// --------------------------------------------------------------------------- + +describe("fuzz_is_safe_expression – OR (literal left side blocked)", () => { + for (const lit of SAFE_LITERALS) { + it(`should block ${lit} || github.actor (literal on left)`, () => { + expect(testIsSafeExpression(`${lit} || github.actor`).safe).toBe(false); + }); + + it(`should block ${lit} || inputs.branch (literal on left)`, () => { + expect(testIsSafeExpression(`${lit} || inputs.branch`).safe).toBe(false); + }); + } +}); + +// --------------------------------------------------------------------------- +// OR compound expressions — blocked when right side is unsafe +// --------------------------------------------------------------------------- + +describe("fuzz_is_safe_expression – OR (unsafe right side blocked)", () => { + for (const unsafe of UNSAFE_LEAVES) { + it(`should block github.actor || ${unsafe}`, () => { + expect(testIsSafeExpression(`github.actor || ${unsafe}`).safe).toBe(false); + }); + + it(`should block github.actor == 'x' || ${unsafe}`, () => { + expect(testIsSafeExpression(`github.actor == 'x' || ${unsafe}`).safe).toBe(false); + }); + } +}); + +// --------------------------------------------------------------------------- +// Ternary-style AND/OR chains — all refused (literal in AND operand) +// --------------------------------------------------------------------------- + +describe("fuzz_is_safe_expression – ternary-style (all refused — literal in AND operand)", () => { + it("should refuse condition && literal || literal (the former spec-enforcer pattern)", () => { + expect(testIsSafeExpression("github.event.inputs.enforce_all == 'true' && 'full-sweep (enforce_all)' || 'round-robin'").safe).toBe(false); + expect(testIsSafeExpression("inputs.mode == 'fast' && 'fast-mode' || 'normal-mode'").safe).toBe(false); + expect(testIsSafeExpression("'yes' && github.actor || 'no'").safe).toBe(false); + }); + + it("should refuse safe_expr && literal || literal for all safe leaves", () => { + for (const leaf of SAFE_LEAVES.slice(0, 5)) { + expect(testIsSafeExpression(`${leaf} && 'yes' || 'no'`).safe).toBe(false); + } + }); + + it("should refuse comparison && literal || literal for all operators", () => { + for (const op of COMPARISON_OPS) { + const expr = `github.event.inputs.enforce_all ${op} 'x' && 'yes' || 'no'`; + expect(testIsSafeExpression(expr).safe).toBe(false); + } + }); +}); + +describe("fuzz_is_safe_expression – ternary-style (unsafe in any position blocked)", () => { + for (const unsafe of UNSAFE_LEAVES) { + it(`should block ${unsafe} == 'x' && github.actor || github.repository`, () => { + expect(testIsSafeExpression(`${unsafe} == 'x' && github.actor || github.repository`).safe).toBe(false); + }); + + it(`should block github.actor && ${unsafe} || github.repository`, () => { + expect(testIsSafeExpression(`github.actor && ${unsafe} || github.repository`).safe).toBe(false); + }); + + it(`should block github.actor == 'value' || ${unsafe}`, () => { + expect(testIsSafeExpression(`github.actor == 'value' || ${unsafe}`).safe).toBe(false); + }); + } +}); + +// --------------------------------------------------------------------------- +// Security invariants: unsafe namespaces are ALWAYS blocked regardless of +// the surrounding compound expression structure +// --------------------------------------------------------------------------- + +describe("fuzz_is_safe_expression – security invariants", () => { + /** + * Wrapper forms that should never make an unsafe sub-expression safe. + * Note: wrappers that would introduce a literal operand in && are now also blocked + * (for the literal reason), but the security invariant still holds. + */ + const wrappers = [ + /** bare */ + u => u, + /** comparison */ + u => `${u} == 'x'`, + /** AND — unsafe on left */ + u => `${u} && github.actor`, + /** AND — unsafe on right */ + u => `github.actor && ${u}`, + /** OR — unsafe on right */ + u => `${u} || 'fallback'`, + /** OR — unsafe on right of a comparison */ + u => `github.actor == 'value' || ${u}`, + /** compound: comparison && unsafe || safe */ + u => `${u} == 'x' && github.actor || github.repository`, + /** compound: safe && unsafe || safe */ + u => `github.actor && ${u} || github.repository`, + /** triple-OR — unsafe in middle */ + u => `github.actor || ${u} || github.repository`, + ]; + + for (const unsafe of UNSAFE_LEAVES) { + for (const wrap of wrappers) { + const expr = wrap(unsafe); + it(`should block: ${expr}`, () => { + expect(testIsSafeExpression(expr).safe).toBe(false); + }); + } + } + + it("should never mark any expression containing secrets. as safe", () => { + const secretExprs = ["secrets.TOKEN", "secrets.GITHUB_TOKEN", "secrets.TOKEN == 'x'", "github.actor && secrets.TOKEN", "secrets.TOKEN || 'fallback'", "secrets.TOKEN == 'x' && github.actor || github.repository"]; + for (const expr of secretExprs) { + expect(testIsSafeExpression(expr).safe).toBe(false); + } + }); + + it("should never mark any expression with a dangerous property name as safe", () => { + const dangerous = ["constructor", "__proto__", "prototype", "hasOwnProperty", "valueOf"]; + for (const prop of dangerous) { + expect(testIsSafeExpression(`github.${prop}`).safe).toBe(false); + expect(testIsSafeExpression(`inputs.${prop}`).safe).toBe(false); + expect(testIsSafeExpression(`github.${prop} == 'x'`).safe).toBe(false); + expect(testIsSafeExpression(`github.actor && github.${prop}`).safe).toBe(false); + } + }); +}); +// --------------------------------------------------------------------------- +// Exhaustive safe-leaf × comparison-operator matrix +// --------------------------------------------------------------------------- + +describe("fuzz_is_safe_expression – exhaustive safe-leaf × operator matrix", () => { + for (const leaf of SAFE_LEAVES) { + for (const op of COMPARISON_OPS) { + it(`should allow ${leaf} ${op} 'constant'`, () => { + const { safe, error } = testIsSafeExpression(`${leaf} ${op} 'constant'`); + expect(error).toBeNull(); + expect(safe).toBe(true); + }); + } + } +}); diff --git a/actions/setup/js/runtime_import.cjs b/actions/setup/js/runtime_import.cjs index 529e655089..6d8484a643 100644 --- a/actions/setup/js/runtime_import.cjs +++ b/actions/setup/js/runtime_import.cjs @@ -199,22 +199,83 @@ function isSafeExpression(expr) { } } - // Check for OR expressions with literals (e.g., "inputs.repository || 'default'") - // Pattern: safe_expression || 'literal' or safe_expression || "literal" or safe_expression || `literal` - // Also supports numbers and booleans as literals + // Strict string-literal regex: the body must not contain an unescaped copy of the + // opening quote character. This prevents compound expressions like + // `'a' || secrets.TOKEN || 'b'` from being misclassified as a string literal because + // they happen to start and end with a quote. + // Pattern: ^(quote)(non-quote-non-backslash | escaped-char)*(same-quote)$ + // [^'\\] = any char except the single-quote and backslash + // \\. = backslash followed by any character (escape sequence) + const STRING_LITERAL_RE = /^'(?:[^'\\]|\\.)*'$|^"(?:[^"\\]|\\.)*"$|^`(?:[^`\\]|\\.)*`$/; + + /** + * Returns true when `expr` is a standalone literal value (string, number, or boolean). + * Used to refuse literal operands inside && / || compound expressions — a literal in a + * conjunction or disjunction is semantically incomplete and may hide injection vectors. + * @param {string} expr - The trimmed expression to test + * @returns {boolean} + */ + const isLiteralValue = expr => { + const t = expr.trim(); + if (STRING_LITERAL_RE.test(t)) return true; + if (/^-?\d+(\.\d+)?$/.test(t)) return true; + if (t === "true" || t === "false") return true; + return false; + }; + + // Allow literal values (string, number, boolean) as *standalone* safe expressions only. + // A literal is only valid when it is the entire expression, not as a sub-expression inside + // && or ||. The checks below enforce this constraint by refusing literal operands there. + const isStringLiteralStandalone = STRING_LITERAL_RE.test(trimmed); + if (isStringLiteralStandalone) { + const contentMatch = trimmed.match(/^(['"`])(.+)\1$/); + if (contentMatch) { + const content = contentMatch[2]; + // Reject nested expressions + if (content.includes("${{") || content.includes("}}")) { + return false; + } + // Reject escape sequences that could hide keywords + if (/\\[xu][\da-fA-F]/.test(content) || /\\[0-7]{1,3}/.test(content)) { + return false; + } + // Reject zero-width characters + if (/[\u200B-\u200D\uFEFF]/.test(content)) { + return false; + } + } + return true; + } + if (/^-?\d+(\.\d+)?$/.test(trimmed) || trimmed === "true" || trimmed === "false") { + return true; + } + + // Check for OR expressions (e.g., "inputs.repository || 'default'"). + // The RIGHT side may be a literal (fallback default), but the LEFT side must not be a + // literal — a literal on the left is always truthy and makes the right side dead code. + // Important: once an OR match is found the decision is final — do NOT fall through to + // the AND/comparison checks below, because doing so would allow a partially-validated + // OR expression like "github.actor == 'x' || secrets.TOKEN" to pass via the comparison + // path even though the right side is unsafe. const orMatch = trimmed.match(/^(.+?)\s*\|\|\s*(.+)$/); if (orMatch) { const leftExpr = orMatch[1].trim(); const rightExpr = orMatch[2].trim(); + // Refuse a literal on the left side of a disjunction — semantically always-true + // and a potential source of confusion or injection vectors. + if (isLiteralValue(leftExpr)) { + return false; + } + // Check if left side is safe - const leftIsSafe = isSafeExpression(leftExpr); - if (!leftIsSafe) { + if (!isSafeExpression(leftExpr)) { return false; } - // Check if right side is a literal string (single, double, or backtick quotes) - const isStringLiteral = /^(['"`]).*\1$/.test(rightExpr); + // Check if right side is a literal string (single, double, or backtick quotes). + // Use the same strict regex that requires no unescaped matching quote in the body. + const isStringLiteral = STRING_LITERAL_RE.test(rightExpr); if (isStringLiteral) { // Validate string literal content for security const contentMatch = rightExpr.match(/^(['"`])(.+)\1$/); @@ -248,10 +309,48 @@ function isSafeExpression(expr) { return true; } - // If right side is also a safe expression (e.g., secrets.FOO || secrets.BAR) + // If right side is also a safe expression (e.g., inputs.repo || github.repository) if (isSafeExpression(rightExpr)) { return true; } + + // Right side is neither a safe literal nor a safe expression — reject. + return false; + } + + // Check for AND expressions (e.g., "github.actor && github.repository"). + // Both sides must be independently safe property expressions — literal operands are refused + // because a literal in a conjunction is semantically incomplete (always truthy/falsy constant) + // and could hide injection vectors. Operator precedence means && binds tighter than ||, so + // this check runs after the OR check above. + // Important: once an AND match is found the decision is final — do NOT fall through to + // the comparison check, which could otherwise allow "github.actor == 'x' && secrets.TOKEN" + // to pass because the comparison extracts only "github.actor" as safe. + const andMatch = trimmed.match(/^(.+?)\s*&&\s*(.+)$/); + if (andMatch) { + const leftExpr = andMatch[1].trim(); + const rightExpr = andMatch[2].trim(); + // Refuse literal sub-expressions in a conjunction + if (isLiteralValue(leftExpr) || isLiteralValue(rightExpr)) { + return false; + } + return isSafeExpression(leftExpr) && isSafeExpression(rightExpr); + } + + // Check for simple comparison expressions (e.g., "github.event.inputs.enforce_all == 'true'"). + // This check only runs for expressions that have no top-level || or && operators (since those + // cases are fully handled above), preventing a partially-validated compound expression from + // sneaking through via the comparison path. + // Extract each property access on the left side of a comparison operator and verify it is in + // the allowed list. This mirrors the Go comparisonExtractionRegex logic. + const compExtractRegex = /([a-zA-Z_][a-zA-Z0-9_.]*)\s*(?:==|!=|<=?|>=?)\s*/g; + const comparisonProps = []; + let compMatch; + while ((compMatch = compExtractRegex.exec(trimmed)) !== null) { + comparisonProps.push(compMatch[1].trim()); + } + if (comparisonProps.length > 0 && comparisonProps.every(prop => isSafeExpression(prop))) { + return true; } return false; diff --git a/actions/setup/js/runtime_import.test.cjs b/actions/setup/js/runtime_import.test.cjs index 2fa2b3775b..d3cce5c80e 100644 --- a/actions/setup/js/runtime_import.test.cjs +++ b/actions/setup/js/runtime_import.test.cjs @@ -302,6 +302,79 @@ describe("runtime_import", () => { expect(isSafeExpression("inputs.value || 'line\\nbreak'")).toBe(!0); expect(isSafeExpression("inputs.value || 'tab\\there'")).toBe(!0); }); + it("should allow standalone string literals", () => { + expect(isSafeExpression("'full-sweep (enforce_all)'")).toBe(!0); + expect(isSafeExpression("'round-robin'")).toBe(!0); + expect(isSafeExpression('"double-quoted"')).toBe(!0); + expect(isSafeExpression("`backtick`")).toBe(!0); + }); + it("should allow standalone number and boolean literals", () => { + expect(isSafeExpression("42")).toBe(!0); + expect(isSafeExpression("3.14")).toBe(!0); + expect(isSafeExpression("true")).toBe(!0); + expect(isSafeExpression("false")).toBe(!0); + }); + it("should reject standalone string literals with nested expressions", () => { + expect(isSafeExpression("'${{ secrets.TOKEN }}'")).toBe(!1); + expect(isSafeExpression("'text }} end'")).toBe(!1); + }); + it("should reject standalone string literals with escape sequences", () => { + expect(isSafeExpression("'\\\\x41 injection'")).toBe(!1); + expect(isSafeExpression("'\\\\u0041 injection'")).toBe(!1); + expect(isSafeExpression("'\\\\101 injection'")).toBe(!1); + }); + it("should allow comparison expressions with safe properties", () => { + expect(isSafeExpression("github.event.inputs.enforce_all == 'true'")).toBe(!0); + expect(isSafeExpression("github.actor == 'octocat'")).toBe(!0); + expect(isSafeExpression("inputs.mode != 'dry-run'")).toBe(!0); + }); + it("should reject comparison expressions with unsafe properties", () => { + expect(isSafeExpression("secrets.TOKEN == 'value'")).toBe(!1); + expect(isSafeExpression("vars.SECRET == 'value'")).toBe(!1); + }); + it("should allow AND compound expressions without literals", () => { + expect(isSafeExpression("github.actor && github.repository")).toBe(!0); + expect(isSafeExpression("inputs.flag && github.event.inputs.mode")).toBe(!0); + expect(isSafeExpression("github.event.inputs.enforce_all == 'true' && github.event.inputs.enforce_all")).toBe(!0); + }); + it("should reject AND compound expressions where either side is a literal", () => { + // literal RHS — the ternary pattern is now refused + expect(isSafeExpression("github.event.inputs.enforce_all == 'true' && 'full-sweep (enforce_all)'")).toBe(!1); + expect(isSafeExpression("inputs.flag && 'enabled'")).toBe(!1); + expect(isSafeExpression("github.actor && 42")).toBe(!1); + expect(isSafeExpression("github.actor && true")).toBe(!1); + // literal LHS + expect(isSafeExpression("'prefix' && github.actor")).toBe(!1); + expect(isSafeExpression("true && github.repository")).toBe(!1); + }); + it("should reject AND compound expressions with unsafe side", () => { + expect(isSafeExpression("secrets.TOKEN && 'safe'")).toBe(!1); + expect(isSafeExpression("github.actor && secrets.TOKEN")).toBe(!1); + // comparison AND unsafe — must not leak via the comparison check + expect(isSafeExpression("github.actor == 'x' && secrets.TOKEN")).toBe(!1); + }); + it("should reject literal on the left side of a disjunction", () => { + // A literal on the left of || is always truthy — the right side would be dead code. + expect(isSafeExpression("'prefix' || github.actor")).toBe(!1); + expect(isSafeExpression("'default' || inputs.branch")).toBe(!1); + expect(isSafeExpression("true || github.repository")).toBe(!1); + expect(isSafeExpression("42 || inputs.count")).toBe(!1); + }); + it("should reject ternary-style AND/OR expressions (literal in AND operand)", () => { + // The ternary emulation pattern uses a literal as the AND right-hand side, which is + // now refused as a literal sub-expression in a conjunction. + expect(isSafeExpression("github.event.inputs.enforce_all == 'true' && 'full-sweep (enforce_all)' || 'round-robin'")).toBe(!1); + expect(isSafeExpression("inputs.mode == 'fast' && 'fast-mode' || 'normal-mode'")).toBe(!1); + // literal on LHS of AND is equally refused + expect(isSafeExpression("'yes' && github.actor || 'no'")).toBe(!1); + }); + it("should reject ternary-style expressions with unsafe properties", () => { + expect(isSafeExpression("secrets.TOKEN == 'x' && 'yes' || 'no'")).toBe(!1); + // unsafe in right side of OR — must not leak via comparison check + expect(isSafeExpression("github.actor == 'value' || secrets.TOKEN")).toBe(!1); + // unsafe in AND consequent after OR + expect(isSafeExpression("true && secrets.TOKEN || 'no'")).toBe(!1); + }); }), describe("evaluateExpression", () => { beforeEach(() => { diff --git a/pkg/parser/runtime_import_fuzz_test.go b/pkg/parser/runtime_import_fuzz_test.go index ee49dfec7d..d5a880f98c 100644 --- a/pkg/parser/runtime_import_fuzz_test.go +++ b/pkg/parser/runtime_import_fuzz_test.go @@ -59,6 +59,39 @@ func FuzzRuntimeImportExpressionValidation(f *testing.F) { f.Add("github.event.release.assets[0].id") // array access f.Add("github" + strings.Repeat(".prop", 50)) // very long chain + // Seed corpus with compound expression forms + // Standalone literals + f.Add("'full-sweep (enforce_all)'") + f.Add("'round-robin'") + f.Add("true") + f.Add("false") + f.Add("42") + // Comparison expressions + f.Add("github.actor == 'octocat'") + f.Add("github.event.inputs.enforce_all == 'true'") + f.Add("inputs.mode != 'dry-run'") + f.Add("github.run_id >= 1000") + // AND compound expressions — both sides must be safe non-literals + f.Add("github.actor && github.repository") + f.Add("inputs.flag && github.event.inputs.mode") + f.Add("github.event.inputs.enforce_all == 'true' && github.event.inputs.enforce_all") + // OR fallback pattern (literal on right is allowed) + f.Add("github.event.inputs.enforce_all || 'round-robin'") + f.Add("inputs.branch || 'main'") + // Refused: AND with literal operand + f.Add("github.event.inputs.enforce_all == 'true' && 'full-sweep (enforce_all)'") + f.Add("inputs.flag && 'enabled'") + // Refused: ternary-style (literal in AND) + f.Add("github.event.inputs.enforce_all == 'true' && 'full-sweep (enforce_all)' || 'round-robin'") + f.Add("inputs.mode == 'fast' && 'fast-mode' || 'normal-mode'") + // Refused: literal on left of OR + f.Add("'default' || github.actor") + // Unsafe compound expressions — must be rejected + f.Add("secrets.TOKEN && github.actor") + f.Add("github.actor && secrets.TOKEN") + f.Add("secrets.TOKEN == 'x' && github.actor || github.repository") + f.Add("github.actor == 'value' || secrets.TOKEN") + // Find node executable nodePath, err := exec.LookPath("node") if err != nil { @@ -143,6 +176,16 @@ try { if strings.Contains(expression, "\n") && result.Safe { t.Errorf("Expression with newline was marked as safe: %q", expression) } + + // Compound expressions whose first token is an unsafe namespace must not be safe. + // We check for a leading unsafe namespace to avoid false positives on safe + // sub-expressions that happen to contain "secrets" as a literal string value. + unsafeLeadingPatterns := []string{"secrets.", "runner.", "vars."} + for _, prefix := range unsafeLeadingPatterns { + if strings.HasPrefix(strings.TrimSpace(expression), prefix) && result.Safe { + t.Errorf("Compound expression starting with %q was marked as safe: %q", prefix, expression) + } + } } }) } @@ -171,6 +214,21 @@ func FuzzRuntimeImportProcessExpressions(f *testing.F) { f.Add("Text }} github.actor }}") // unbalanced f.Add(strings.Repeat("${{ github.actor }} ", 100)) // many expressions + // Seed corpus with compound expression forms + f.Add("Mode: ${{ github.event.inputs.enforce_all || 'round-robin' }}") + f.Add("Flag: ${{ github.actor && github.repository }}") + f.Add("Cond: ${{ github.actor == 'octocat' }}") + f.Add("Literal: ${{ 'static-value' }}") + // Refused: AND with literal operand + f.Add("Bad: ${{ github.event.inputs.enforce_all == 'true' && 'full-sweep (enforce_all)' }}") + f.Add("Bad: ${{ github.event.inputs.enforce_all == 'true' && 'full-sweep' || 'round-robin' }}") + // Refused: literal on left of OR + f.Add("Bad: ${{ 'default' || github.actor }}") + // Unsafe compound patterns — must be rejected + f.Add("Bad: ${{ secrets.TOKEN && github.actor }}") + f.Add("Bad: ${{ github.actor && secrets.TOKEN }}") + f.Add("Bad: ${{ github.actor == 'value' || secrets.TOKEN }}") + nodePath, err := exec.LookPath("node") if err != nil { f.Skip("Node.js not found, skipping fuzz test") diff --git a/pkg/parser/runtime_import_test.go b/pkg/parser/runtime_import_test.go index 9ae608b7f9..c4b275e2dc 100644 --- a/pkg/parser/runtime_import_test.go +++ b/pkg/parser/runtime_import_test.go @@ -91,6 +91,79 @@ func TestRuntimeImportExpressionValidation(t *testing.T) { expectSafe: false, description: "Variables not allowed", }, + // Compound expression forms added by spec-enforcer fix + { + name: "standalone string literal", + expression: "'full-sweep (enforce_all)'", + expectSafe: true, + description: "Standalone string literal is safe", + }, + { + name: "standalone boolean literal", + expression: "true", + expectSafe: true, + description: "Standalone boolean literal is safe", + }, + { + name: "comparison expression with safe property", + expression: "github.event.inputs.enforce_all == 'true'", + expectSafe: true, + description: "Comparison with safe LHS property is allowed", + }, + { + name: "AND compound safe × safe (no literals)", + expression: "github.actor && github.repository", + expectSafe: true, + description: "AND compound: both sides are safe properties", + }, + { + name: "AND compound with literal RHS is now refused", + expression: "github.event.inputs.enforce_all == 'true' && 'full-sweep (enforce_all)'", + expectSafe: false, + description: "AND with literal operand is refused (literal sub-expression in conjunction)", + }, + { + name: "AND compound with literal LHS is refused", + expression: "'prefix' && github.actor", + expectSafe: false, + description: "AND with literal on left is refused", + }, + { + name: "ternary-style AND/OR chain is refused (literal in AND)", + expression: "github.event.inputs.enforce_all == 'true' && 'full-sweep (enforce_all)' || 'round-robin'", + expectSafe: false, + description: "Ternary-style AND/OR chain with literal operand is refused", + }, + { + name: "OR fallback pattern is still allowed", + expression: "github.event.inputs.enforce_all || 'round-robin'", + expectSafe: true, + description: "OR fallback: safe property || literal is allowed", + }, + { + name: "literal on left of OR is refused", + expression: "'default' || github.actor", + expectSafe: false, + description: "Literal on left of OR is refused (always truthy, dead right side)", + }, + { + name: "unsafe AND compound with secrets on right", + expression: "github.actor && secrets.TOKEN", + expectSafe: false, + description: "secrets.TOKEN in AND right side must be blocked", + }, + { + name: "unsafe OR compound with secrets on right", + expression: "github.actor == 'value' || secrets.TOKEN", + expectSafe: false, + description: "secrets.TOKEN in OR right side must be blocked", + }, + { + name: "unsafe compound with secrets", + expression: "secrets.TOKEN == 'x' && github.actor || github.repository", + expectSafe: false, + description: "secrets.TOKEN in compound expression must be blocked", + }, } // Find node executable