feat(sourcemaps): refuse to inject into an SRI build (re-targeted to main) - #49
Conversation
…ipt hashes Injecting appends bytes to every `.js`, so a Subresource Integrity hash the HTML already carries stops matching and the browser refuses to run the script. Measured on a real webpack + webpack-subresource-integrity build in Chromium 151: before injecting the app ran clean; after it, `window.__ran` was false and the console carried "Failed to find a valid digest in the 'integrity' attribute … The resource has been blocked." The page loads and executes NOTHING — strictly worse than shipping no debug-ids at all. `@bugsee/bundler-plugin-core` already refuses this, but it only wires vite and rollup. Angular 17+, esbuild and Deno users drive this binary directly, and Angular's `subresourceIntegrity: true` is exactly the config that breaks — an adversarial review of the JS-side guard is what surfaced the gap. So the check belongs here too. `inject` now scans the HTML under the paths it was given, before writing anything, and exits 20 with the page, the pinned script and what to do about it. Verified on that same webpack build: exit 20, `md5` of both bundles unchanged; `--allow-sri` proceeds and both change. Detected: `<script integrity src=…>` and `<link rel=modulepreload|preload integrity href=…>`, quoted or bare, in any attribute order, with a whitespace-padded URL (browsers strip it). Deliberately NOT flagged, because each would be a silent loss of symbolication for a build we were never going to break: a CDN script, a non-JS target, a path outside the output directory (containment is compared by path COMPONENT, so `dist-2` is not inside `dist`), an empty `integrity`, `data-integrity` / `data-src`, an inline script, a commented-out tag, and `rel=prefetch` — a failed prefetch is discarded, not fatal. An `--exclude`d file does not count either: one we will not touch cannot have its hash invalidated, and `--exclude` and the guard now share one matcher so they cannot disagree. Six mutants caught (guard bypassed, exclusions ignored, containment dropped, empty integrity counted, comments scanned, rel filter dropped). The tests also caught a real bug in the first draft: the dot-directory filter was applied to the walk ROOT, so pointing `inject` at `.next`, `.nuxt` or `.output` scanned nothing at all. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…ets stamped An adversarial review of #45 and #46 found two SEV1s and six SEV2/3s, and they all came from one mistake: the SRI guard computed its own idea of which files were at risk while the walk computed its own idea of which files to stamp, so the two disagreed in both directions. **SEV1 — a CDN `publicPath` build shipped blank.** `output.publicPath: 'https://cdn…/'` with webpack-subresource-integrity is the CANONICAL SRI deployment: hash the local bytes, serve them from a CDN. The HTML then carries an absolute URL, which the guard dropped as "somebody else's file" — while the very bytes it pinned sat in `dist/` and were stamped. A URL is now matched literally first, then by file name, so the origin prefix (or `/static/`, `/_next/`) no longer hides it. Verified on a real webpack build with a CDN publicPath: exit 20, both bundles byte-identical. **SEV1 — a page under a dot-directory or `node_modules` was skipped by the guard but its target was stamped by the walk.** A VitePress build (`docs/.vitepress/dist/index.html`) had its entry stamped and went blank. Worse, a test asserted that skip as desired behaviour. The guard now walks exactly what the stamping walks — no directory filter, no depth cap. Verified on the VitePress shape. The restructure: one `collect_targets` list is built once, used for stamping, and handed to the guard, which refuses only over a file in it. That closes, by construction: - a `./dist` or `../dist` root turning a correctly `--exclude`d pinned file into a refusal (the README's own `--exclude 'polyfills*.js'` example), - `inject dist/polyfills.js` being refused because some OTHER file is pinned, - a refusal naming a path that does not exist (`dist/static/main.js` under a `/static/` publicPath), - a stale page pinning a deleted bundle, or a symlinked directory, refusing the run, - overlapping roots (`inject a a/b`) disagreeing about an excluded file — the walk stamped what the guard had excluded. Also from the review: roots are absolutized, so an absolute `--exclude` pattern works with a relative root; a pattern is tried relative to the current directory too (`dist/vendor/**`); an EMPTY pattern is now a configuration error rather than a silent match-nothing; `.htm`/`.xhtml` count as pages; and six surviving mutants got tests (`preload` vs `modulepreload`, `.htm`/`.xhtml`, multi-line comments, absolute exclude patterns, deep pages, root-relative resolution). Ten mutants caught now; the three tests that chdir take a lock, because `set_current_dir` is process-global and the harness threads. Docs say plainly what the guard cannot see: SRI that never reaches the emitted HTML — a manifest consumed by a server template, a page rendered at request time (Next.js `experimental.sri`), or HTML written outside the directory it was pointed at. Regression checked against origin/main: a plain build with no SRI and no --exclude stamps exactly as before. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Relative to the CURRENT DIRECTORY as well: `--exclude 'dist/vendor/**'` is how a user thinks | ||
| // about their own build, and the roots are absolutized before the walk, so without this the | ||
| // pattern would match nothing at all. | ||
| if let Ok(cwd) = std::env::current_dir() { | ||
| if path | ||
| .strip_prefix(sri::lexical_normalize(&cwd)) | ||
| .is_ok_and(|rel| matcher.is_match(rel)) | ||
| { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
--help drift: --exclude now also matches a pattern relative to the current directory (this new fallback), in addition to the walked root and the full/absolute path. But the clap doc comment on exclude in src/cli/sourcemaps.rs (line 23) still says only "Matched against the path relative to each walked root and against the full path" — it wasn't updated in this change, even though is_excluded's matching semantics changed here. Per CLAUDE.md, --help must stay in sync with behavior in the same change. Suggest updating that doc comment to mention the current-directory-relative match too (the README's "Other defaults" section already got this treatment — the clap doc comment did not).
Code reviewThis adds an SRI (Subresource Integrity) pre-flight guard to Findings: 1 inline (0 blocking).
Non-blocking — recommend merging after a one-line doc-comment fix for the finding above. |
…st stay a no-op
A review of the whole 0.7.11 delta found the guard refusing over files it would never rewrite.
`--help` promises "re-running on already-injected files is a no-op", and the documented workflow for
a build that pins its hashes is `--allow-sri` once, then let the build recompute them. Any later run
without the flag — a second CI job, an upload step that re-injects, a retry after a flaky step —
then failed with exit 20 while changing nothing. A 0.7.10 user with that pipeline would break on
upgrade.
The guard now filters the pinned scripts through `would_rewrite`, which mirrors `inject_one`'s
decision: a fresh bundle, a re-key (regenerated map), or a foreign id still missing our registration
all count; an already-stamped bundle with an unchanged map does not. A test drives both functions
over the same fixtures and asserts they agree, so they cannot drift apart — which is the exact class
of bug the shared-target-list design exists to prevent.
Also from the same review:
- **The guard was blind to a page one level ABOVE the given path.** The standard Vite/webpack layout
is `dist/index.html` beside `dist/assets/*.js`, so `inject dist/assets` (or one bundle by path)
stamped the pinned file and exited 0. Pages directly in the parent are now read too — only when
the given path holds no pages of its own, and bounded to 2000 entries, because that parent can be
a directory nobody meant us to read. (`/tmp` with thousands of entries: the unbounded first
attempt took the inject tests from 0.07 s to 23 s.)
- **The "exit 20 for a bad `--exclude` glob" contract was asserted nowhere** — the test checked only
that the message named the flag, so the classification a CI script actually reads was left to
chance. Same for the dry-run refusal, which asserted a bare `is_err()`.
- Docs: `--exclude`'s `--help` did not mention the cwd-relative match, the CHANGELOG line about it
was garbled ("relative to the absolute path"), and neither said that a CDN script sharing a file
name with one of your bundles IS refused, or that `--dry-run` refuses.
Five mutants caught (filter removed, filter always-on, registration case dropped, parent scan
removed, parent scan emptied).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
…ims had drifted The review of the 0.7.11 delta caught all four: "Matched against the path relative to the absolute path" (nonsense — README had it right), "refuses only over a file this run would really stamp" (now: would REWRITE, which is what makes a re-run a no-op), "not flagged: a genuine third-party CDN script" (unqualified — one sharing a file name with your bundle IS refused), and nothing saying where pages are read from or that --dry-run refuses. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Review of the full 0.7.11 delta — findings fixed here (ca116b2, 9727cd1)A read-only adversarial pass over everything since
Five mutants caught for the new logic (filter removed, filter always-on, registration case dropped, parent scan removed, parent scan emptied). Gates: 🤖 Generated with Claude Code |
Code reviewAdds I checked this against every category in scope: The implementation itself is unusually well defended: Findings: None. Looks sound — happy to merge as is. |
| .map(|e| e.path().to_path_buf()) | ||
| .collect(); | ||
| if pages.is_empty() { | ||
| pages = pages_directly_in(base.parent()); |
There was a problem hiding this comment.
The docs (README/CHANGELOG) promise "pages are read from anywhere under the given paths, plus any sitting directly in a given path's parent" — but the implementation only falls back to the parent when the recursive scan under root finds zero pages (if pages.is_empty()), i.e. it's an "or", not a "plus"/union.
Concretely: inject dist/widget where dist/widget/ contains widget.js plus some unrelated HTML that pins nothing (a demo page, a bundle-analyzer report, a stray doc page copied into the output), while the real entry page dist/index.html (one level up) is the one that pins widget.js's hash — the guard will silently miss it and let the build ship broken, which is exactly the class of SEV1 this PR exists to close.
Consider unioning the parent-level pages in rather than only using them when the under-root scan is empty.
Code reviewThis adds an SRI (Subresource Integrity) guard to Findings: 1 inline (0 blocking).
Not blocking — it requires a specific layout (unrelated HTML inside the given root while the actual pinning page sits in the parent) and the rest of the guard's design and tests are solid. Worth fixing before/soon after merge, but I'd merge as is if timing matters and follow up. |
1 participant
Re-open of #46, which never reached
main. #46 targetedfeat/inject-exclude; #45 (that branch → main) merged ~20 seconds earlier, so #46 merged into a branch that had already been consumed.maintoday has--excludebut NOT the SRI guard — the SEV1 fix. Same two commits, cherry-picked onto currentmain.The guard
Injecting appends bytes to every
.js, so a Subresource Integrity hash the HTML already carries stops matching and the browser refuses to run the script. Measured on a real webpack 5.111 +webpack-subresource-integritybuild in Chromium 151: before injecting the app ran clean; after it, the entry script was blocked and the page executed nothing.sourcemaps injectnow scans the HTML under the given paths before writing anything and exits 20, naming the page, the pinned script and the ways out (stamp earlier,--exclude, or--allow-sri).What the adversarial review of the first draft changed
One list (
collect_targets) now decides both what gets stamped and what the guard can refuse over, which closed two SEV1s and five more findings by construction:publicPathbuild shipped blank — the canonical SRI deployment (hash locally, serve from a CDN) puts an absolute URL in the HTML, which the guard dropped as "somebody else's file" while stamping the very bytes it pinned. URLs are now matched literally, then by file name.node_moduleswas skipped by the guard but its target stamped by the walk (VitePressdocs/.vitepress/dist→ blank page) — and a test asserted that skip as correct../distroots breaking exclusions, a file argument refused over someone else's pin, refusals naming nonexistent paths, stale pages and symlinked dirs refusing, overlapping roots disagreeing.Also: absolute/cwd-relative
--excludepatterns, empty pattern now exit 20,.htm/.xhtmlread, no depth cap, ten mutants caught, and the threechdirtests serialized (set_current_diris process-global).Gates on this branch:
cargo fmt --check,clippy --all-targets -D warnings(0), all unit tests + suites,e2e_flows.pyALL PASS.🤖 Generated with Claude Code