From 70a090fd2670750c151be29adfe8141a499c148d Mon Sep 17 00:00:00 2001 From: Daniel Han Date: Fri, 22 May 2026 13:00:02 +0000 Subject: [PATCH 1/2] studio: harden lockfile supply-chain audit followups for #5604 Three regressions surfaced after #5604 landed. All addressed here with matching pytest coverage. 1. Default mode no longer blocks on `unsupported-lockfile-version`. A lockfile downgraded to v1 (or bumped to an unsupported future version) silently passes default CI because the auditor's structural walk only runs on v2/v3, so `blocked-known-malicious` / `known-ioc-string` findings cannot be produced. v1 downgrade is a documented supply-chain attack shape; restoring it to BLOCKING_KINDS. 2. `UNSLOTH_LOCKFILE_AUDIT_SKIP` warnings interpolated the raw env var value into `::warning::` workflow commands without `_gha_escape()`, so a value containing `\n::error::...` was emitted as a second physical workflow-command line (GH Actions parses workflow commands per physical line). Routing both branches through `_gha_escape()` collapses any control char onto a single annotation line. 3. The two workflows that consume `studio/package-lock.json` -- `studio-tauri-smoke.yml` and `release-desktop.yml` -- did not invoke `scripts/lockfile_supply_chain_audit.py` before their `npm install` steps. Lifecycle scripts in a compromised lockfile would run before the audit could refuse the lockfile, defeating the script's "pre-install" guarantee. `release-desktop.yml` additionally has `contents: write`, so the gap had real publish-attacker blast radius. Both workflows now run the audit before any `npm install` or `npm ci`. Tests ----- - test_unsupported_lockfile_version_blocks_default: v1 lockfile must exit 1 in default mode. - test_blocking_kinds_contains_unsupported_lockfile_version: direct module-level pin against future regressions. - test_skip_env_warning_escapes_workflow_command_injection: both skip-env branches must escape `\n` / `%` so no injected physical workflow-command line is produced. - test_audit_runs_before_npm_install_in_consumer_workflows: regex ordering check on the `run:` lines in the two consumer workflows so a future reorder cannot silently bring back the pre-install gap. 16/16 pytest pass; default-mode audit still rc=0 on real lockfiles. --- .github/workflows/release-desktop.yml | 18 +- .github/workflows/studio-tauri-smoke.yml | 18 +- scripts/lockfile_supply_chain_audit.py | 26 ++- .../test_lockfile_supply_chain_audit.py | 170 ++++++++++++++++++ 4 files changed, 217 insertions(+), 15 deletions(-) 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" + ) From a45536e218140a634b4c7fbe33c14d13393bde4b Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 22 May 2026 13:01:30 +0000 Subject: [PATCH 2/2] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- .../test_lockfile_supply_chain_audit.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/tests/security/test_lockfile_supply_chain_audit.py b/tests/security/test_lockfile_supply_chain_audit.py index 12c8aa767d..8e85320715 100644 --- a/tests/security/test_lockfile_supply_chain_audit.py +++ b/tests/security/test_lockfile_supply_chain_audit.py @@ -406,12 +406,12 @@ def test_unsupported_lockfile_version_blocks_default(tmp_path): """ p = tmp_path / "package-lock.json" p.write_text( - '{\n' + "{\n" ' "name": "test",\n' ' "version": "1.0.0",\n' ' "lockfileVersion": 1,\n' ' "dependencies": {"react": {"version": "18.2.0"}}\n' - '}\n' + "}\n" ) proc = _run_auditor(root = tmp_path, npm_lockfiles = [p]) combined = proc.stdout + proc.stderr @@ -509,9 +509,9 @@ def test_skip_env_warning_escapes_workflow_command_injection(tmp_path): 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}" - ) + 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 " @@ -533,8 +533,12 @@ def test_audit_runs_before_npm_install_in_consumer_workflows(): # ``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) + 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}"