diff --git a/.github/workflows/release-desktop.yml b/.github/workflows/release-desktop.yml index e747605322..83778688b2 100644 --- a/.github/workflows/release-desktop.yml +++ b/.github/workflows/release-desktop.yml @@ -361,13 +361,23 @@ jobs: with: node-version: 24 + - name: Lockfile supply-chain audit (pre-install scan) + shell: bash + # Must run BEFORE any `npm install`. This job has + # ``contents: write`` (release upload), so a compromised + # lockfile that runs a malicious postinstall script during the + # Tauri CLI install or frontend install below could publish + # attacker-controlled binaries through the release. The audit + # refuses the run before any npm lifecycle script executes. + run: python3 scripts/lockfile_supply_chain_audit.py + - name: Install pinned Tauri CLI # Lifecycle scripts (esbuild native-binary postinstall, etc.) are # required for `vite build`. The pre-install lockfile structural - # audit (lockfile_supply_chain_audit.py) is the practical defence - # against the npm postinstall-dropper class -- it fires BEFORE any - # tarball runs, on the injection pattern itself rather than an - # advisory-DB lookup. + # audit (lockfile_supply_chain_audit.py) above is the practical + # defence against the npm postinstall-dropper class -- it fires + # BEFORE any tarball runs, on the injection pattern itself + # rather than an advisory-DB lookup. run: npm install --save-dev --prefix studio @tauri-apps/cli@2.10.1 --no-fund --no-audit - name: Verify pinned Tauri CLI diff --git a/.github/workflows/studio-tauri-smoke.yml b/.github/workflows/studio-tauri-smoke.yml index 1156c264ae..3f34462b09 100644 --- a/.github/workflows/studio-tauri-smoke.yml +++ b/.github/workflows/studio-tauri-smoke.yml @@ -60,13 +60,20 @@ jobs: with: workspaces: studio/src-tauri -> target + - name: Lockfile supply-chain audit (pre-install scan) + # Must run BEFORE any `npm install` (including the Tauri CLI + # install below). Lifecycle scripts in a compromised lockfile + # would otherwise execute inside this runner before the audit + # gets a chance to refuse the lockfile. + run: python3 scripts/lockfile_supply_chain_audit.py + - name: Install pinned Tauri CLI (matches release-desktop.yml) # Lifecycle scripts (esbuild native-binary postinstall, etc.) are # required for `vite build`. The pre-install lockfile structural - # audit (lockfile_supply_chain_audit.py) is the practical defence - # against the npm postinstall-dropper class -- it fires BEFORE any - # tarball runs, on the injection pattern itself rather than an - # advisory-DB lookup. + # audit (lockfile_supply_chain_audit.py) above is the practical + # defence against the npm postinstall-dropper class -- it fires + # BEFORE any tarball runs, on the injection pattern itself + # rather than an advisory-DB lookup. run: npm install --save-dev --prefix studio @tauri-apps/cli@2.10.1 --no-fund --no-audit - name: Verify pinned Tauri CLI version @@ -75,9 +82,6 @@ jobs: echo "$out" [ "$out" = "tauri-cli 2.10.1" ] || { echo "::error::expected tauri-cli 2.10.1, got $out"; exit 1; } - - name: Lockfile supply-chain audit (pre-install scan) - run: python3 scripts/lockfile_supply_chain_audit.py - - name: Frontend build (npm ci, vite) working-directory: studio/frontend # Lifecycle scripts (esbuild native-binary postinstall, etc.) are diff --git a/scripts/lockfile_supply_chain_audit.py b/scripts/lockfile_supply_chain_audit.py index ffeea51c23..293d3cb1ec 100644 --- a/scripts/lockfile_supply_chain_audit.py +++ b/scripts/lockfile_supply_chain_audit.py @@ -730,6 +730,14 @@ BLOCKING_KINDS: frozenset[str] = frozenset( "missing-lockfile", "unreadable-lockfile", "missing-toml-parser", + # An unsupported lockfileVersion means the audit could not walk + # the dependency tree at all (the structural rules below only + # apply to npm v2/v3). Treating it as advisory would let a v1 + # downgrade -- a known supply-chain attack shape -- silently + # pass CI: the scanner reports the kind, exits 0, and no + # blocking finding is raised. Keep this blocking so a checked-in + # lockfile cannot be downgraded out of audit coverage. + "unsupported-lockfile-version", } ) @@ -797,17 +805,27 @@ def main(argv: list[str] | None = None) -> int: if _skip_raw is not None: _skip = _skip_raw.strip() _invalid_tokens = {"", "1", "0", "true", "false", "yes", "no", "on", "off"} + # Both branches echo the user-supplied env var inside a + # ``::warning::`` GH Actions workflow command. The raw value can + # contain ``%``, ``\r``, ``\n`` or even another ``::error::`` + # line (workflow-command injection); _gha_escape collapses each + # message onto a single annotation line per the GH workflow- + # commands spec. if _skip.lower() in _invalid_tokens or len(_skip) < 5: print( - "::warning::Lockfile audit skip REQUIRES a justification " - f"value (>=5 chars, not '{_skip_raw}'). Proceeding with " - "audit. Use e.g. UNSLOTH_LOCKFILE_AUDIT_SKIP=ticket-1234.", + "::warning::" + + _gha_escape( + "Lockfile audit skip REQUIRES a justification " + f"value (>=5 chars, not '{_skip_raw}'). Proceeding with " + "audit. Use e.g. UNSLOTH_LOCKFILE_AUDIT_SKIP=ticket-1234." + ), file = sys.stderr, flush = True, ) else: print( - f"::warning::Lockfile audit skipped: reason='{_skip}'", + "::warning::" + + _gha_escape(f"Lockfile audit skipped: reason='{_skip}'"), file = sys.stderr, flush = True, ) diff --git a/tests/security/test_lockfile_supply_chain_audit.py b/tests/security/test_lockfile_supply_chain_audit.py index 483bb9e763..12c8aa767d 100644 --- a/tests/security/test_lockfile_supply_chain_audit.py +++ b/tests/security/test_lockfile_supply_chain_audit.py @@ -382,3 +382,173 @@ def test_skip_env_var_with_short_value_rejected(tmp_path): assert "[lockfile-audit] npm:" in c, ( f"value {bad_val!r} should have fallen through to run audit; " f"got:\n{c}" ) + + +# --------------------------------------------------------------------------- +# Followup regression tests for #5604: +# - unsupported lockfile versions must block in default mode (v1 downgrade +# would otherwise pass with rc=0 because the structural walk only runs +# on v2/v3) +# - the ``UNSLOTH_LOCKFILE_AUDIT_SKIP`` warning must be routed through +# ``_gha_escape()`` so an attacker-controlled value cannot inject a +# second workflow-command line via embedded ``\n::error::...`` +# - the audit script must be invoked BEFORE ``npm install`` in any +# workflow that consumes the audited lockfiles +# --------------------------------------------------------------------------- + + +def test_unsupported_lockfile_version_blocks_default(tmp_path): + """A v1 lockfile (or any non-v2/v3 version) means the structural + dependency walk never runs, so ``blocked-known-malicious`` / + ``known-ioc-string`` findings cannot be produced. Treating that as + advisory lets an attacker downgrade a checked-in lockfile to v1 + and silently exit CI with rc=0. Default mode must refuse. + """ + p = tmp_path / "package-lock.json" + p.write_text( + '{\n' + ' "name": "test",\n' + ' "version": "1.0.0",\n' + ' "lockfileVersion": 1,\n' + ' "dependencies": {"react": {"version": "18.2.0"}}\n' + '}\n' + ) + proc = _run_auditor(root = tmp_path, npm_lockfiles = [p]) + combined = proc.stdout + proc.stderr + assert proc.returncode == 1, ( + f"v1 lockfile must block default mode (was advisory pre-followup); " + f"rc={proc.returncode}\n--- stdout ---\n{proc.stdout}\n" + f"--- stderr ---\n{proc.stderr}" + ) + assert "unsupported-lockfile-version" in combined, combined + + +def test_blocking_kinds_contains_unsupported_lockfile_version(): + """Direct module-level assertion: if anyone moves + ``unsupported-lockfile-version`` back out of BLOCKING_KINDS this + test trips immediately, before they re-introduce the downgrade + bypass.""" + assert "unsupported-lockfile-version" in lsa.BLOCKING_KINDS + + +def test_skip_env_warning_escapes_workflow_command_injection(tmp_path): + """An attacker controlling ``UNSLOTH_LOCKFILE_AUDIT_SKIP`` could + embed a literal ``\\n::error::...`` and split the warning into a + second workflow-command annotation. Both the invalid-skip branch + (raw value echoed) and the accepted-skip branch (stripped value + echoed) must escape the value via ``_gha_escape()`` so the message + is collapsed onto one annotation line -- meaning no stderr line + OTHER than the audit's own ``::warning::`` may begin with ``::``. + """ + fixture = FIXTURES / "clean_lockfile.json" + + def _physical_lines_starting_with_double_colon(stderr: str) -> list[str]: + # GH Actions parses workflow commands per physical line. Only + # lines that START with `::` after any leading whitespace count + # as a new annotation. Any such line BEYOND the first warning + # is an injected command. + return [ln for ln in stderr.splitlines() if ln.lstrip().startswith("::")] + + # Branch A -- invalid skip value (rejected, audit falls through). + # Use a value that survives the validation check (not a boolean + # token, >=5 chars) BUT contains injection chars. The accepted + # branch is the easier-to-trip target; the rejected branch is + # exercised in test_skip_env_var_with_short_value_rejected. + injected_bad = "%inject\n::error::bad" # contains %, \n, and :: + env_a = {**os.environ, "UNSLOTH_LOCKFILE_AUDIT_SKIP": injected_bad} + proc_a = subprocess.run( + [ + sys.executable, + str(SCRIPT), + "--root", + str(tmp_path), + "--npm-lockfile", + str(fixture), + ], + capture_output = True, + text = True, + timeout = 30, + env = env_a, + ) + # The stripped value is "%inject\n::error::bad" (len 21) and is not + # a booleanish token -> accepted-skip path; rc 0, audit skipped. + assert proc_a.returncode == 0 + assert "%0A" in proc_a.stderr and "%25" in proc_a.stderr, ( + "skip value containing \\n and %% must be %0A / %25 escaped; " + f"stderr was:\n{proc_a.stderr}" + ) + cmd_lines_a = _physical_lines_starting_with_double_colon(proc_a.stderr) + assert len(cmd_lines_a) == 1 and cmd_lines_a[0].startswith("::warning::"), ( + "exactly one ::-prefixed physical line expected (the audit's own " + f"::warning::); injection split the message into: {cmd_lines_a}" + ) + + # Branch B -- short skip value with embedded injection. + # The PRE-strip raw value is also interpolated in the rejected + # branch's warning, so it MUST be escaped too. + injected_short = "1\n::error::short-bad" + # Critically, this value strips to "1\n::error::short-bad" which is + # NOT a booleanish token (the literal newline + tail prevents the + # ``_skip.lower() in _invalid_tokens`` match), so the audit ends up + # routing it through the ACCEPTED branch, not the rejected one. + # That is itself a hardening property worth pinning: an attacker + # cannot bypass the length check via embedded control chars without + # the warning being escaped on the way out. + env_b = {**os.environ, "UNSLOTH_LOCKFILE_AUDIT_SKIP": injected_short} + proc_b = subprocess.run( + [ + sys.executable, + str(SCRIPT), + "--root", + str(tmp_path), + "--npm-lockfile", + str(fixture), + ], + capture_output = True, + text = True, + timeout = 30, + env = env_b, + ) + assert "%0A" in proc_b.stderr, ( + f"value with embedded \\n must be %0A-escaped; stderr was:\n{proc_b.stderr}" + ) + cmd_lines_b = _physical_lines_starting_with_double_colon(proc_b.stderr) + assert all(ln.startswith("::warning::") for ln in cmd_lines_b), ( + f"injection split the message into a non-::warning:: physical " + f"line: {cmd_lines_b}" + ) + + +def test_audit_runs_before_npm_install_in_consumer_workflows(): + """Any GH Actions workflow that consumes one of the audited + lockfiles via ``npm install`` / ``npm ci`` must run the + lockfile_supply_chain_audit step BEFORE that install, otherwise a + compromised lockfile's lifecycle scripts execute before the audit + can refuse the run. Static text check so a future edit cannot + silently reintroduce the asymmetric ordering.""" + import re + + workflows_dir = REPO_ROOT / ".github" / "workflows" + # Match `run:` lines invoking the audit / npm install. We look at + # ``run:`` lines specifically so the prose comment block above + # each step (which mentions both audit and `npm install`) does + # not bias the ordering check. + audit_re = re.compile(r"^\s*run:\s*python3\s+scripts/lockfile_supply_chain_audit\.py", re.MULTILINE) + install_re = re.compile(r"^\s*run:\s*(?:.*&&\s*)?npm\s+(?:install|ci)\b", re.MULTILINE) + for wf_name in ("studio-tauri-smoke.yml", "release-desktop.yml"): + wf = workflows_dir / wf_name + assert wf.is_file(), f"missing workflow: {wf}" + text = wf.read_text(encoding = "utf-8") + audit_match = audit_re.search(text) + assert audit_match, ( + f"{wf_name}: must invoke lockfile_supply_chain_audit.py " + f"via a ``run: python3 scripts/...`` line (none found)" + ) + for install_match in install_re.finditer(text): + assert audit_match.start() < install_match.start(), ( + f"{wf_name}: lockfile audit (offset {audit_match.start()}) " + f"must come BEFORE every ``npm install`` / ``npm ci`` run " + f"line (offending offset {install_match.start()}); a " + f"compromised lockfile's lifecycle scripts would otherwise " + f"execute before the audit can refuse the lockfile" + )