Repository navigation
#12 — infra: headless file:// zero-egress + resource-ref lint + AUDIT.md - #42
Conversation
The PRIMARY zero-egress gate: a real Chromium opens examples/jaffle-shop-report.html via real file:// with DNS denied (--host-resolver-rules=MAP * ~NOTFOUND) and subscribes to every Network.requestWillBeSent event. The proof: zero external requests (http/https/ws/wss/ftp) are emitted by the rendered chrome. Mermaid SVG renders + DataTables initializes — proving the inlined UMD bundle works offline, not just that the request log is empty. Secondary structural lint: walks examples/jaffle-shop-report.html with the tl HTML parser and rejects real loading constructs (<script src>, <link href>, <img src>, CSS @import, CSS url(), protocol-relative //). Strips <script> text content — minified bundles carry hundreds of inert URL string literals that raw grep would false-positive on. Runs in milliseconds. Both CI stubs at .github/workflows/ci.yml flip from `if: false` to required gates on every PR. AUDIT.md is the one-page risk-team index — single entry point to every artifact a reviewer can re-run (headless command, lint command, assets/MANIFEST.toml, tests/fixtures/MANIFEST.toml, deny.toml, Cargo.lock). Plain-language companion to ARCHITECTURE.md §5. SECURITY.md + ARCHITECTURE.md §5 updated from future-tense ("lands in PR 9") to present — the gates are now live. Library choice — headless_chrome over chromiumoxide: sync API, no tokio dep, test reads top-to-bottom for auditor readability. The zero-egress test is 30 lines of linear code; chromiumoxide's async-stream richness adds CI minutes and cognitive load with no proof-strength gain. Auto-fetch path is gated behind the non-default `fetch` feature, so headless_chrome only uses Chrome on PATH. Closes #12 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 42 minutes and 39 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a robust auditability framework to ensure the generated reports are entirely self-contained and perform zero outbound network requests. Key additions include a headless browser test using headless_chrome to monitor runtime network activity and a structured HTML lint using the tl parser to identify forbidden external resource references. The documentation is expanded with a new AUDIT.md index and updates to SECURITY.md and ARCHITECTURE.md. Feedback highlights a bug in the headless test's JavaScript evaluation and suggests strengthening the resource lint to reject all relative paths, ensuring strict adherence to the zero-egress contract.
| && document.querySelector('table.dataTable, table.dt-rows')); \ | ||
| } catch (_) { return false; } \ | ||
| })()", | ||
| false, |
There was a problem hiding this comment.
The evaluate call uses return_by_value: false. In headless_chrome, this means the RemoteObject returned will not contain the actual result value in its value field (it usually returns an objectId or description instead). This causes the subsequent .and_then(|v| v.value).and_then(|v| v.as_bool()) chain to likely always return None, resulting in datatable_ok being false even if DataTables initialized correctly. Setting this to true ensures the boolean result is serialized and available in the value field.
| false, | |
| true, |
There was a problem hiding this comment.
Paraphrase: tab.evaluate(expression, await_promise) is the headless_chrome convenience wrapper around Runtime.Evaluate, and it hardcodes returnByValue: false in its CDP call. With that flag false, the RemoteObject may carry only an objectId handle rather than a serialized value, so the .value.and_then(.as_bool()) chain can silently downgrade to None and datatable_ok collapses to false regardless of the page state.
Disposition: Fixed in 3173820. The fix is not the suggested true literal on the second arg — that argument is await_promise, not return_by_value. Switched to an explicit tab.call_method(Runtime::Evaluate { return_by_value: Some(true), await_promise: Some(false), ... }) call so the deserialized JSON bool always lands in RemoteObject.value regardless of future headless_chrome / CDP-protocol changes. The convenience-wrapper path is no longer load-bearing here.
Verification: cargo test --test headless_zero_egress -- --ignored passes locally in 4.4s; datatable_ok correctly returns true against the committed example. Commit: 3173820.
| fn is_external_url(value: &str) -> bool { | ||
| let v = value.trim(); | ||
| if v.is_empty() || v.starts_with('#') { | ||
| return false; | ||
| } | ||
| // Inline / non-network schemes — allowed. | ||
| if v.starts_with("data:") || v.starts_with("mailto:") { | ||
| return false; | ||
| } | ||
| // Protocol-relative — forbidden by the contract. | ||
| if v.starts_with("//") { | ||
| return true; | ||
| } | ||
| // Network schemes — forbidden. | ||
| if let Some((scheme, _)) = v.split_once(':') { | ||
| let s = scheme.to_ascii_lowercase(); | ||
| return matches!( | ||
| s.as_str(), | ||
| "http" | "https" | "ws" | "wss" | "ftp" | "ftps" | "file", | ||
| ); | ||
| } | ||
| false | ||
| } |
There was a problem hiding this comment.
The is_external_url function is too permissive. It currently returns false for relative paths (e.g., "script.js") and unknown schemes, allowing them to pass the lint. Given the goal of a strictly self-contained report, any resource reference that isn't an inline data: URI or a fragment should be considered a violation. This also ensures consistency with the logic used in find_css_external_refs.
fn is_external_url(value: &str) -> bool {
let v = value.trim();
if v.is_empty() || v.starts_with('#') {
return false;
}
// Inline schemes — allowed.
if v.starts_with("data:") || v.starts_with("mailto:") {
return false;
}
// Everything else (relative paths, network schemes, protocol-relative)
// is a violation of the self-contained report contract.
true
}References
- Prioritize explicit defensive guards that document code invariants over more concise idiomatic alternatives if the guard clarifies intent and ensures robustness against future changes in logic.
There was a problem hiding this comment.
Paraphrase: The original is_external_url only rejected protocol-relative and a known list of network schemes (http/https/ws/wss/ftp/ftps/file). Relative paths (script.js, ./x, ../y, /abs/path) and unknown scheme:-prefixed values returned false (= allowed), which is too permissive for a self-contained-report contract — the rendered chrome has no sibling files to legitimately load.
Disposition: Fixed in 3173820. Renamed is_external_url → is_forbidden_resource_ref (the new name says what it actually decides) and tightened the allowlist to { empty, #fragment, data:, mailto: }. Everything else is a violation. Added 4 new regression tests: relative_path_script_is_caught, absolute_path_link_is_caught, parent_relative_img_is_caught, file_scheme_is_caught.
Note: tightening surfaced a real false positive — tl materializes template-literal substrings like <img src="${e}"> inside minified script bundles as spurious DOM nodes. Added strip_script_bodies() preprocessor that drops <script>...</script> content while preserving the opening tag, so <script src="..."> attribute checks still fire. No coverage loss.
Verification: cargo nextest run --test resource_ref_lint — 16/16 tests pass, including the committed-example assertion. Commit: 3173820.
Two pre-merge fixes from advisor review:
(1) Mark `report_makes_zero_external_requests_when_opened_via_file_url`
`#[ignore]` so the cross-platform `cargo nextest run --all-targets`
invocation (3 OS test matrix + coverage job) does not carry a Chrome
dependency. The dedicated `headless-zero-egress` CI job opts in
explicitly via `cargo test --test headless_zero_egress -- --ignored`,
where setup-chrome has just installed Chrome on PATH. Without this,
a future GitHub runner image change that drops Chrome would break
the standard `test` job even though the dedicated gate would still
pass — the dependency would be invisible to anyone reading the test
matrix.
(2) Drop dead `table.dt-rows` from the DataTables init selector. The
rendered chrome's table elements get the `dataTable` class added at
runtime by DataTables.js; `dt-rows` was a typo carry-over (the
body-cell classes are `dt-body-center` / `dt-body-left` / etc., not
`dt-rows`). The OR-comma fallback was silently doing nothing —
the proof was passing on `table.dataTable` alone.
AUDIT.md updated to show the `-- --ignored` form so the auditor's
re-run command matches what actually runs.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ighten lint allowlist Two findings from gemini-code-assist on PR #42: (1) HIGH: DataTables init probe used `tab.evaluate(_, false)` which is the headless_chrome wrapper that hardcodes `returnByValue: false`. Primitives currently surface in `RemoteObject.value` by chance, but the contract is implicit. Switched to explicit `Runtime::Evaluate { return_by_value: Some(true), ... }` so the deserialized JSON bool is always present regardless of future headless_chrome or CDP changes. (2) MEDIUM: `is_external_url` was too permissive — relative paths like `script.js` / `./x` / `../y` / `/abs/path` and unknown schemes returned `false` (allowed). For a single-self-contained HTML file, there are no sibling files to legitimately load; any such reference is a violation. Renamed `is_external_url` → `is_forbidden_resource_ref` and tightened the allowlist to `{ empty, #fragment, data:, mailto: }` — everything else is a violation. Added 4 new test cases covering relative / absolute / parent-relative / file:// schemes. Tightening the lint surfaced a real false positive: tl materializes template-literal substrings like `<img src="${e}">` inside minified script bundles as spurious DOM nodes. Added `strip_script_bodies()` preprocessor that drops `<script>...</script>` content while preserving the opening tag (so `<script src="...">` attribute checks still fire). Loses no real coverage — the lint never cared about script bodies — and eliminates the false-positive class entirely. Local: 283 tests pass + 1 ignored (headless gate); 17 in the two new test crates including the 4 new tightened-lint cases. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 participant
Summary
PR 9 — the adoption-gate auditability package.
tests/headless_zero_egress.rs) — a real Chromium opens the committedexamples/jaffle-shop-report.htmlvia realfile://with DNS denied (--host-resolver-rules=MAP * ~NOTFOUND) and subscribes to everyNetwork.requestWillBeSentevent. Asserts: zero external requests (http/https/ws/wss/ftp), Mermaid SVG renders, DataTables initializes. The Mermaid + DataTables assertions prove the inlined UMD bundle actually works offline — not just that the request log is empty.tests/resource_ref_lint.rs) — walks the committed HTML with thetlparser and rejects real loading constructs (<script src>,<link href>,<img src>, CSS@import, CSSurl(), protocol-relative//). Runs in milliseconds; catches template-layer regressions before the heavyweight headless test. Skips<script>text bodies — minified bundles carry hundreds of inert URL string literals.if: falseto required gates in.github/workflows/ci.yml(resource-ref-lint+headless-zero-egress). Thetracked: cute-dbt#12exclusion markers are removed at the same time per~/.claude/rules/exclusions.md.AUDIT.md— the one-page risk-team index. Single entry point to every reviewable artifact (headless command, lint command,assets/MANIFEST.toml,tests/fixtures/MANIFEST.toml,deny.toml,Cargo.lock,rust-toolchain.toml). Plain-language companion toARCHITECTURE.md§5.SECURITY.md+ARCHITECTURE.md§5 updated from future-tense ("lands in PR 9") to present.Decisions made at session start
D1 —
headless_chromeoverchromiumoxideSync API, no
tokiodep. The zero-egress test is 30 lines of linear code;chromiumoxide's async-stream richness would add CI minutes and reader cognitive load with no proof-strength gain. The auditor needs to read this test, not parse async/await flow. Auto-fetch path is gated behind the non-defaultfetchfeature; headless_chrome falls through todefault_executable()which honorsCHROMEenv var then PATH-searches (wherebrowser-actions/setup-chromeinstalls Chrome on CI).D2 — spike self-confirm superseded by PR 8b
Issue #12 originally listed a
spike/spike.htmloperator self-confirm as a prereq. PR 8b (#38) shipped the byte-deterministicexamples/jaffle-shop-report.htmlas the test input; the headless test runs against the committed artifact directly. No separate spike needed. Issue body annotated.D3 — DataTables verification scope: initialization, not "working sort + search"
Issue #12 acceptance text said "at least one DataTable initializes with working sort + search."
focus.mdsaid "initializes." Reconciled to initialization-only — the network-block proof is the load-bearing claim; functional DataTables verification adds CI minutes (click events, await re-renders) without strengthening the auditability story. If a regression breaks sort/search but leaves init intact, it's a UX bug, not a zero-egress bug, and the unit-of-work split is correct.Quality gates
cargo nextest run --all-targets --lockedcargo clippy --all-targets --locked -- -D warningscargo fmt --checkcargo deny checkRUSTDOCFLAGS="-D warnings" cargo doc --no-deps --document-private-items --lockedcargo llvm-cov nextest --fail-under-lines 85lefthook run pre-push --all-filesTest plan
resource-ref-lintjob greenheadless-zero-egressjob green (Linux runner,browser-actions/setup-chrome@v1provides Chrome on PATH)example-report-up-to-datestill green (renderer is unchanged)examples/jaffle-shop-report.htmllocally viafile://with DevTools Network — confirm zero external requests visuallyCloses #12
🤖 Generated with Claude Code