fix(build): npm ci --ignore-scripts in the wheel build hook (GHSA-pfvh, code-only)#461
Conversation
padak
left a comment
There was a problem hiding this comment.
Review of #461 — fix(build): npm ci --ignore-scripts in the wheel build hook (GHSA-pfvh, code-only)
Generated by
kbagent-pr-reviewersubagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed viamake check, not duplicated here.
Summary
This PR adds --ignore-scripts to the npm ci invocation in scripts/hatch_build.py to close a real install-time RCE surface (GHSA-pfvh-6q5w-9m52): without the flag, any postinstall/preinstall lifecycle script in the transitive npm dependency tree executes on the user's machine during git+ install — including on machines that have never consciously run npm. The fix is one-line surgical; the test assertion (assert "--ignore-scripts" in ci_cmd) is correctly scoped to the npm ci call only and does not incorrectly constrain the npm run build call. The deliberate deferral of the version bump and changelog entry is noted in the PR description and is consistent with the documented conflict-immunity pattern used in #422 and #460.
Verdict: APPROVE. No blocking findings. Two non-blocking observations and one nit follow.
Verdict
- Verdict: APPROVE
- Blocking findings: 0
- Non-blocking findings: 2
- Nits: 1
Blocking findings
(none)
Non-blocking findings
[NB-1] tests/test_build_hook.py:209 — test does not assert that --ignore-scripts is absent from the npm run build command
The assertion at line 210 (assert build_cmd[1:] == ["run", "build"]) implicitly confirms --ignore-scripts is not in the build_cmd, but it does so as a positive equality check on the full tail rather than an explicit negative assertion. This is correct and sufficient — npm run ignores the flag anyway — but a future maintainer reading the test may not realize the build_cmd shape is intentionally different from ci_cmd. A one-line comment like # --ignore-scripts intentionally absent: npm run executes a named script, not lifecycle hooks above the build_cmd[1:] assertion would make the asymmetry self-documenting and prevent a well-meaning contributor from "fixing" the apparent inconsistency.
[NB-2] scripts/hatch_build.py:148 — --ignore-scripts suppresses the root package's own prepare script if one is ever added
npm ci --ignore-scripts suppresses ALL lifecycle scripts, including the root package's prepare entry. Currently web/frontend/package.json has no prepare script so this is benign. However, if a future contributor adds "prepare": "some-setup-tool" to the root package.json (a common pattern for husky, lefthook, etc.), the flag will silently suppress it during wheel builds without any visible error. The code comment at line 141–147 explains the security rationale well, but does not call out this constraint. Adding "Note: if web/frontend/package.json ever gains a prepare script, verify it is not needed at wheel-build time" would surface the constraint before it bites.
Nits
[NIT-1]tests/test_build_hook.py:189— the docstring oftest_passes_resolved_npm_path_not_bare_namestill describes only the "Bug 1" Windows.cmdfix. Now that this test also covers the GHSA-pfvh regression, expanding the docstring to mention both responsibilities (or splitting into two focused tests) would keep the test naming honest.
Verification log
gh pr view 461 --json title,body,files,additions,deletions,state→ state=OPEN, 2 files (+12/-1), conventionalfix(build):prefix ✓git rev-parse --abbrev-ref HEAD→fix/build-ignore-scriptsmatches PR branch ✓git show 7cec496 --stat→ onlyscripts/hatch_build.py(+9/-1) andtests/test_build_hook.py(+4/0) touched; nosrc/changes ✓ (Plugin synchronization map: no CLI command surface change, no OPERATION_REGISTRY / context.py / CLAUDE.md / keboola-expert.md updates required)- Layer violation checks (typer in services, httpx in commands, formatter in clients):
grepreturned empty ✓ - Convention checks (magic numbers, raw error_code strings, bare except, print() in src/, token in logs): all empty ✓
python3scan ofweb/frontend/package-lock.json(lockfileVersion=3, 393 packages): 0 packages with lifecycle scripts (postinstall/install/preinstall/prepare) ✓ — confirms--ignore-scriptshas nothing to suppress in the current lock; the security value is defense-in-depth against a future compromised dependencyweb/frontend/package.jsonroot scripts:{dev, build, preview, test}— noprepare/postinstallentries ✓- No
esbuildentries in lock file (vite 8 / rolldown does not depend on esbuild'spostinstallbinary fetcher) ✓ — the classic--ignore-scriptsbreakage vector is absent - No git-hook managers (
husky,lefthook,simple-git-hooks) in dependencies ✓ uv run pytest tests/test_build_hook.py::TestBundleUiBug1NpmInvocation::test_passes_resolved_npm_path_not_bare_name -v→ 1 passed ✓make check→ 4177 passed, 8 skipped, lint/format/typecheck/skill/version/error-codes clean ✓- Behavior verification: the PR author ran the real build with the flag (
npm ci --ignore-scripts ... && npm run build→dist/index.htmlpresent); the lock file scan above independently corroborates that no lifecycle script would have fired anyway; cannot re-run the full build in this reviewer context (no npm in CI sandbox), but the evidence is sufficient
Open questions for the author
(none)
…GHSA-pfvh) The hatch build hook runs `npm ci` at wheel-build / `git+` install time. Without --ignore-scripts, a compromised (transitive) npm dependency's postinstall would execute arbitrary code on the user's machine during install -- a real install-time RCE surface. Add --ignore-scripts. Verified non-breaking: `tsc -b && vite build` needs no dependency install-scripts (vite 8 / rolldown ships prebuilt platform binaries, not postinstall downloads) -- ran the real build with the flag and it produced dist/index.html cleanly. test_build_hook asserts the flag is present. Code-only (no version bump / changelog) to stay conflict-free against the rapid main release cadence; version + changelog added at next release. Private advisory GHSA-pfvh-6q5w-9m52.
7cec496 to
35cb8b5
Compare
Author response — review findings addressed (commit 35cb8b5, still code-only)Thanks — and for the empirical confirmation that
No logic change. Still code-only (version + changelog deferred to the next release alongside #422 / #460). |
Summary
Fixes M9 from the 2026-06-12 security audit (private advisory GHSA-pfvh-6q5w-9m52) — an install-time RCE surface in the wheel build hook.
scripts/hatch_build.pyrunsnpm cito build the React SPA at wheel-build time and when a user installs viagit+https://github.com/keboola/cli(the fallback when no prebuilt wheel asset is present). Without--ignore-scripts, everypreinstall/install/postinstalllifecycle script in the (transitive) npm dependency tree executes on the user's machine during install — so a single compromised transitive dependency lands arbitrary code execution on every user who takes thegit+install path with Node on PATH.Fix
Add
--ignore-scriptsto thenpm ciinvocation.tsc -b && vite buildcompiles the SPA without needing any dependency install-script.Verified non-breaking (not assumed)
--ignore-scriptscan break builds whose deps fetch a binary viapostinstall(the classic esbuild trap). I ran the real build with the flag inweb/frontend:vite 8 uses rolldown (Rust bundler) which ships prebuilt platform binaries as regular package files, not via a postinstall download — so
--ignore-scriptsis safe here. The lockfile (package-lock.json) remains committed + integrity-pinned, so this is defense-in-depth on top of the existing tarball-hash verification.Tests
tests/test_build_hook.py::test_passes_resolved_npm_path_not_bare_namenow also asserts--ignore-scriptsis in thenpm cicommand (mock-based regression test). Full suite green: 4177 passed, 135 skipped; lint/format clean.Touches only
scripts/hatch_build.py+tests/test_build_hook.py— no version bump / changelog entry (added at the next release, same conflict-immunity rationale as #422/#460).Audit progress
Open after this: M7 (plaintext-on-encrypt warning — 3 services), M10 (version regex), + M1/M5 residuals.