diff --git a/.github/workflows/security-audit.yml b/.github/workflows/security-audit.yml index a1e7b2efa6..8cfa78c743 100644 --- a/.github/workflows/security-audit.yml +++ b/.github/workflows/security-audit.yml @@ -1096,20 +1096,27 @@ jobs: echo '```' } >> "$GITHUB_STEP_SUMMARY" - - name: Extract base-ref lockfile (PR triggers only) + - name: Extract base-ref lockfile and install-script allowlist (PR triggers only) if: github.event_name == 'pull_request' run: | set -e BASE_SHA="${{ github.event.pull_request.base.sha }}" git show "$BASE_SHA:studio/frontend/package-lock.json" \ > /tmp/base-package-lock.json + # Pull the TRUSTED allowlist from the base ref so a PR cannot + # allowlist its own new postinstall dependency in the same diff + # the checker scans. Missing file is OK (empty allowlist). + git show "$BASE_SHA:studio/frontend/.install-script-allowlist" \ + > /tmp/base-install-script-allowlist 2>/dev/null \ + || : > /tmp/base-install-script-allowlist - name: Diff for newly-added install-script deps if: github.event_name == 'pull_request' run: | python3 scripts/check_new_install_scripts.py \ --base /tmp/base-package-lock.json \ - --head studio/frontend/package-lock.json + --head studio/frontend/package-lock.json \ + --base-allowlist /tmp/base-install-script-allowlist - name: Skip install-script diff (non-PR trigger) if: github.event_name != 'pull_request' diff --git a/scripts/check_new_install_scripts.py b/scripts/check_new_install_scripts.py index e5a6782866..3ce83b6827 100644 --- a/scripts/check_new_install_scripts.py +++ b/scripts/check_new_install_scripts.py @@ -236,29 +236,41 @@ def diff_new_install_scripts(base_lock: dict, head_lock: dict) -> list[Finding]: def _load_allowlist(path: Path) -> set[str]: - """Read a file of newline-separated package names to skip. + """Read a file of newline-separated ``name@version`` entries to skip. - Purely opt-in: an entry on its own line whitelists every version - of that package against the new-install-script gate. Lines - starting with ``#`` are comments; blank lines are ignored. The - intent is to triage well-known, eyeballed dev-only deps (vitest's - esbuild, sharp's libvips, etc.) without weakening the gate for - the long tail. Missing or unreadable file means empty allowlist. + Each entry MUST be pinned to an exact version (``esbuild@0.21.5``, + ``@scope/pkg@1.2.3``). Bare names are rejected so allowlisting + ``esbuild`` cannot silently approve a later malicious + ``esbuild@99.0.0`` published by a compromised maintainer -- every + new version requires its own review. Lines starting with ``#`` are + comments; blank lines are ignored. Missing file = empty allowlist. """ if not path.exists(): return set() out: set[str] = set() try: - for raw in path.read_text(encoding = "utf-8").splitlines(): - line = raw.strip() - if not line or line.startswith("#"): - continue - out.add(line) + text = path.read_text(encoding = "utf-8") except OSError: return set() + for raw in text.splitlines(): + line = raw.strip() + if not line or line.startswith("#"): + continue + name, sep, version = line.rpartition("@") + if not sep or not name or not version: + raise ValueError( + f"{path}: allowlist entry {line!r} must be pinned to an " + "exact version (e.g. 'esbuild@0.21.5'). Bare names are " + "rejected so we cannot silently approve a later release.", + ) + out.add(line.lower()) return out +def _finding_allowlist_key(finding: Finding) -> str: + return f"{finding.name}@{finding.version}".lower() + + def main(argv: list[str] | None = None) -> int: parser = argparse.ArgumentParser( description = ( @@ -280,10 +292,20 @@ def main(argv: list[str] | None = None) -> int: "--allowlist", default = None, help = ( - "Path to a newline-separated allowlist of package names " + "Path to the HEAD newline-separated 'name@version' allowlist " "to skip. Defaults to '/.install-script-allowlist'." ), ) + parser.add_argument( + "--base-allowlist", + default = None, + help = ( + "Path to the TRUSTED BASE allowlist. Defaults to " + "'/.install-script-allowlist'. Entries that exist " + "only on HEAD fail the gate so a PR cannot allowlist its " + "own new postinstall dependency." + ), + ) args = parser.parse_args(argv) try: @@ -293,21 +315,54 @@ def main(argv: list[str] | None = None) -> int: print(f"[install-script-diff] ERROR: {exc}", file = sys.stderr) return 2 - allowlist_path = ( + head_allowlist_path = ( Path(args.allowlist) if args.allowlist else Path(args.head).parent / ".install-script-allowlist" ) - allowlist = _load_allowlist(allowlist_path) + base_allowlist_path = ( + Path(args.base_allowlist) + if args.base_allowlist + else Path(args.base).parent / ".install-script-allowlist" + ) + + try: + head_allowlist = _load_allowlist(head_allowlist_path) + base_allowlist = _load_allowlist(base_allowlist_path) + except ValueError as exc: + print(f"[install-script-diff] ERROR: {exc}", file = sys.stderr) + return 2 + + # Refuse a PR that adds new allowlist entries on its own head branch. + # Allowlist deltas must land in a separate, trusted commit on base + # first; otherwise the same PR could approve its own postinstall. + added_head_only = sorted(head_allowlist - base_allowlist) + if added_head_only: + print( + "[install-script-diff] FAIL: install-script allowlist entries " + "must already exist on the base branch; do not let a PR " + "allowlist its own new postinstall dependency.", + file = sys.stderr, + ) + for entry in added_head_only: + print(f" head-only allowlist entry: {entry}", file = sys.stderr) + return 1 + + # Only the trusted base allowlist participates in the skip set. + allowlist = base_allowlist findings = diff_new_install_scripts(base_lock, head_lock) if allowlist: - skipped = [f for f in findings if f.name in allowlist] - findings = [f for f in findings if f.name not in allowlist] + skipped = [ + f for f in findings if _finding_allowlist_key(f) in allowlist + ] + findings = [ + f for f in findings if _finding_allowlist_key(f) not in allowlist + ] for f in skipped: print( - f"[install-script-diff] SKIP {f.name}@{f.version} " - f"(allowlisted via {allowlist_path.name})", + f"[install-script-diff] SKIP {_finding_allowlist_key(f)} " + "(allowlisted via trusted base allowlist)", flush = True, ) if not findings: diff --git a/studio/frontend/.install-script-allowlist b/studio/frontend/.install-script-allowlist index d970fd927d..33e1023ac6 100644 --- a/studio/frontend/.install-script-allowlist +++ b/studio/frontend/.install-script-allowlist @@ -3,13 +3,15 @@ # refuses any newly-added install-script dep by default; entries listed here # are explicitly skipped. # -# Add a package name on its own line, and a one-line comment above it -# describing what the postinstall does and why it's safe. Pin to the -# package name only -- the gate will skip every version under that name. +# Pin EACH entry to an exact "name@version" so a maintainer compromise that +# ships a new malicious version is not silently swept under the same line. +# Lines starting with "#" are comments; blank lines are ignored. # # DO NOT add packages here without reading the actual install script body. +# Allowlist entries MUST land on main first; the gate rejects head-only +# additions so a PR cannot allowlist its own new postinstall dep. # evanw/esbuild downloads the platform-specific native binary # (esbuild-linux-x64, etc.) in its postinstall. Used transitively by # vitest for dev-only test transforms; no runtime exposure. -esbuild +esbuild@0.21.5