Compare commits

...
Sign in to create a new pull request.

2 commits

Author SHA1 Message Date
pre-commit-ci[bot]
a45536e218 [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
2026-05-22 13:01:33 +00:00
Daniel Han
70a090fd26 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 `:⚠️:` 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.
2026-05-22 13:00:02 +00:00
4 changed files with 221 additions and 15 deletions

View file

@ -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

View file

@ -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

View file

@ -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,
)

View file

@ -382,3 +382,177 @@ 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"
)