fix(viewer): make the frontend build hermetic and actually verify dist/ - #66
Merged
Merged
Conversation
The committed viewer bundle is 424 KB of minified JS that no reviewer can read. CI's job is to prove it is exactly what building the committed source produces, so the bundle stays a derived value rather than a trust input. Two independent defects meant it was not proving that. 1. The build was self-referential. dist/ is tracked, so Tailwind v4's automatic source detection scanned the PREVIOUS bundle and treated words inside it as class-name candidates. `npm run build` over a populated dist/ emitted three utilities a clean build does not (.static, .table, .resize), making the committed state a fixed point rather than a function of src/. CI therefore passed only because it built on top of the very artifact it was verifying, and a contributor could steer the emitted CSS by planting strings in dist/ instead of in reviewable source. `@source not '../dist'` in src/index.css makes the build a pure function of src/. Verified identical output across a clean dist/, a populated dist/, and a dist/ seeded with decoy class names. dist/ is regenerated here; the only CSS delta is the removal of those three utilities, which no source file uses (resize-y and resize-none are separate utilities and are unaffected). 2. The staleness check used `git diff --quiet`, which only reports tracked, modified files. A PR that ADDS a file to dist/ passed it cleanly — confirmed by committing a dist/assets/evil.js, which the old check waved through. It now builds into an emptied dist/ and uses `git status --porcelain`, which reports modified, added and missing files alike. Output was verified byte-identical on Node 20.19.5 and 22.20.0; the job now pins the version exactly, since its output is the security property. This check is only meaningful if it cannot be skipped. For `pull_request` GitHub runs the workflow as written in the PR, so a PR can delete this job: it must be configured as a required status check on main, which is tracked separately along with CODEOWNERS on package*.json. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
src/nooa/viewer/frontend-react/dist/is 424 KB of minified JS that no reviewer can read. CI'sfrontend-buildjob exists to prove it is exactly what building the committed source produces — so the bundle stays a derived value rather than a trust input, and the only way into the viewer is a reviewable TypeScript diff.Two independent defects meant it was not proving that. Both are exploitable by an external contributor.
1. The build was self-referential
dist/is tracked, so Tailwind v4's automatic source detection scanned the previous bundle and treated words inside it as class-name candidates. Building over a populateddist/emits three utilities a clean build does not:dist/populated (what CI did)dist/empty (clean build).static,.table,.resize)The committed state was a fixed point rather than a function of
src/. CI passed only because it built on top of the artifact it was verifying, and a contributor could steer the emitted CSS by planting strings indist/instead of in reviewable source.@source not '../dist'insrc/index.cssmakes the build a pure function ofsrc/. Verified identical output across a cleandist/, a populateddist/, and adist/seeded with decoy class names.dist/is regenerated here. The only CSS delta is removal of those three utilities — confirmed by a rule-by-rule diff. No source file uses them;resize-yandresize-noneare separate utilities and are unaffected.2. The staleness check missed added files
git diff --quietonly reports tracked, modified files. A PR that adds a file todist/passed cleanly — confirmed by committing adist/assets/evil.js, which the old check waved through.Now builds into an emptied
dist/and usesgit status --porcelain, which reports modified, added and missing files alike. Also guards against a vacuous pass:git status --porcelain <path>prints nothing when the path does not resolve, which reads as success — the same failure mode thesecret-scanjob already guards against.Verification
Ran the exact check locally against this commit:
dist/assets/evil.jsdist/missing (vacuous-pass guard)Output verified byte-identical on Node 20.19.5 and 22.20.0; the job now pins the version exactly, since its output is the security property.
tests/viewer+tests/trace_explorer: 263 passed.Follow-ups (not in this PR)
This check is only meaningful if it cannot be skipped. For
pull_requestGitHub runs the workflow as written in the PR, so a PR can delete this job — andmaincurrently has no branch protection at all.frontend-builda required status check onmain— without this, a PR that removes the job merges greenpackage*.json—npm run buildexecutesvite.config.tsand every plugin innode_modules, which--ignore-scriptsdoes not cover, so a lockfile change is arbitrary code execution in this jobdefault-src 'self') — the frontend is 100% same-origin, so this costs nothing and contains a compromised bundle regardless of build integritywrite; drop toread🤖 Generated with Claude Code