Skip to content

Commit b49d873

Browse files
sklinglernvclaude
andcommitted
fix(viewer): make the frontend build hermetic and actually verify dist/
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>
1 parent 72b243c commit b49d873

6 files changed

Lines changed: 74 additions & 9 deletions

File tree

.github/workflows/ci.yml

Lines changed: 56 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -62,26 +62,76 @@ jobs:
6262
path: dist/
6363
retention-days: 7
6464

65+
# Proves the committed viewer bundle is exactly what building the committed
66+
# source produces. dist/ is 424 KB of minified JS that no reviewer can read,
67+
# so without this the bundle is an unreviewable trust input that any
68+
# contributor could poison independently of the TypeScript they submit. With
69+
# it, dist/ is a derived value CI re-derives from scratch on every PR, and the
70+
# only way into the viewer is through a reviewable source diff.
71+
#
72+
# This job is only meaningful if it cannot be skipped: for `pull_request`,
73+
# GitHub runs the workflow as written in the PR, so a PR may delete this job.
74+
# It must be a REQUIRED status check on main — otherwise a PR that removes it
75+
# merges green. See also CODEOWNERS on package*.json: `npm run build` executes
76+
# vite.config.ts and every plugin in node_modules, which `--ignore-scripts`
77+
# does not cover, so a lockfile change is arbitrary code execution here.
6578
frontend-build:
6679
runs-on: ubuntu-latest
80+
permissions:
81+
contents: read
6782
steps:
6883
- uses: actions/checkout@v4
6984
- name: Setup Node
7085
uses: actions/setup-node@v4
7186
with:
72-
node-version: "20"
73-
- name: Build the trace viewer
87+
# Exact pin: this job's output IS the security property, so the
88+
# toolchain producing it should not float. Output was verified
89+
# byte-identical on 20.19.5 and 22.20.0, so a bump is safe.
90+
node-version: "20.19.5"
91+
- name: Rebuild the viewer bundle from source
7492
working-directory: src/nooa/viewer/frontend-react
7593
run: |
94+
set -euo pipefail
7695
npm ci --ignore-scripts
96+
# Build into an EMPTY dist/. Two reasons, both load-bearing:
97+
# 1. A file that is committed but that the build does not produce
98+
# (i.e. smuggled in) then shows up as deleted rather than silently
99+
# surviving untouched.
100+
# 2. The build must not see the artifact it is being checked against.
101+
# src/index.css carries `@source not '../dist'` so Tailwind skips
102+
# it; wiping dist/ means the check holds even if that regresses.
103+
rm -rf dist
77104
npm run build
78-
- name: Fail if dist/ is stale
105+
# Audit trail: the hashes CI derived, readable in the job log.
106+
sha256sum dist/index.html dist/assets/*
107+
- name: Fail if committed dist/ is not exactly what the build produced
79108
run: |
80-
if ! git diff --quiet src/nooa/viewer/frontend-react/dist/; then
81-
echo "ERROR: frontend-react/dist/ is stale. Run 'npm run build' and commit dist/."
82-
git diff --stat src/nooa/viewer/frontend-react/dist/
109+
set -euo pipefail
110+
DIST=src/nooa/viewer/frontend-react/dist
111+
# Guard against a vacuous pass. `git status --porcelain <path>` warns
112+
# on stderr and prints NOTHING when the path does not resolve, which
113+
# reads as success — so a wrong path or a relocated dist/ would report
114+
# a clean build having verified nothing. Same failure mode the
115+
# gitleaks job guards against below. Assert we are looking at real,
116+
# tracked files before trusting an empty result.
117+
test -d "$DIST" \
118+
|| { echo "::error::$DIST does not exist — the build did not produce it"; exit 1; }
119+
test "$(git ls-files -- "$DIST" | wc -l)" -gt 0 \
120+
|| { echo "::error::no tracked files under $DIST — the check would pass vacuously"; exit 1; }
121+
# `git status --porcelain`, not `git diff`: git diff only reports
122+
# tracked-and-modified files, so it passes a PR that ADDS a file to
123+
# dist/. Porcelain reports ' M' modified, '??' produced-but-uncommitted
124+
# and ' D'/'AD' committed-but-not-produced — all three are failures.
125+
CHANGES=$(git status --porcelain -- "$DIST")
126+
if [ -n "$CHANGES" ]; then
127+
echo "::error::dist/ does not match a clean rebuild of the committed source."
128+
echo "$CHANGES"
129+
echo
130+
echo "If you changed the viewer UI, rebuild and commit the result:"
131+
echo " cd src/nooa/viewer/frontend-react && npm ci --ignore-scripts && rm -rf dist && npm run build"
83132
exit 1
84133
fi
134+
echo "OK — committed dist/ is reproducible from source."
85135
86136
secret-scan:
87137
runs-on: ubuntu-latest

src/nooa/viewer/frontend-react/dist/assets/index-BN0Wf3Y-.css

Lines changed: 0 additions & 1 deletion
This file was deleted.

src/nooa/viewer/frontend-react/dist/assets/index-DKBq4F_b.css

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/nooa/viewer/frontend-react/dist/assets/index-Bf6CiCw-.js renamed to src/nooa/viewer/frontend-react/dist/assets/index-DwrEQzTi.js

File renamed without changes.

src/nooa/viewer/frontend-react/dist/index.html

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,8 @@
44
<meta charset="UTF-8" />
55
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
66
<title>NVIDIA OO Agents Viewer</title>
7-
<script type="module" crossorigin src="/assets/index-Bf6CiCw-.js"></script>
8-
<link rel="stylesheet" crossorigin href="/assets/index-BN0Wf3Y-.css">
7+
<script type="module" crossorigin src="/assets/index-DwrEQzTi.js"></script>
8+
<link rel="stylesheet" crossorigin href="/assets/index-DKBq4F_b.css">
99
</head>
1010
<body>
1111
<div id="root"></div>

src/nooa/viewer/frontend-react/src/index.css

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,21 @@
11
@import 'tailwindcss';
22
@import 'highlight.js/styles/vs2015.css';
33

4+
/* Exclude the build output from Tailwind's automatic source detection.
5+
*
6+
* dist/ is committed, so Tailwind's project scan would otherwise read the
7+
* PREVIOUS bundle and treat words inside it as class-name candidates. That
8+
* makes the build self-referential: `npm run build` over a populated dist/
9+
* emits utilities (.static, .table, .resize) that a clean build does not, so
10+
* the committed state is a fixed point rather than a function of src/.
11+
*
12+
* Two consequences, both bad. CI's rebuild-and-compare check only passes
13+
* because it builds on top of the artifact it is verifying, and a contributor
14+
* can influence the emitted CSS by planting strings in dist/ instead of in
15+
* reviewable source. This directive makes the build a pure function of src/.
16+
*/
17+
@source not '../dist';
18+
419
body {
520
@apply bg-gray-950 text-gray-200 antialiased;
621
font-family: ui-sans-serif, system-ui, sans-serif;

0 commit comments

Comments
 (0)