Repository navigation
adapters: asset-inlining infra + assets/MANIFEST.toml provenance - #26
Merged
Merged
Conversation
PR 8a (#10). The vendored frontend bundle embedded into the binary's .rodata, its supply-chain provenance contract, and a placeholder smoke renderer proving the bundle assembles into one self-contained, zero-egress HTML document. - Vendor the five-asset bundle into assets/: Sakura 1.5.0, jQuery 3.7.1, DataTables 2.1.8 (JS + CSS), Mermaid 11.15.0 (UMD). The committed bytes are byte-identical (SHA-256) to a fresh download from each asset's canonical URL — the same bundle the R1 spike empirically proved renders offline with zero network requests. - assets/MANIFEST.toml: one [[asset]] entry per file with name, version, path, canonical source URL, SHA-256, and SPDX license (all MIT). This is the supply-chain artifact an auditor reads for the frontend bundle, paired with Cargo.toml / Cargo.lock / deny.toml for the crate graph. - src/adapters/asset_embed.rs: each asset embedded via include_str! into .rodata as a pub const &str. Every v0.1 asset is text, so there is no include_bytes! user; the favicon is an empty data: URI. MERMAID_INIT is a static constant (securityLevel:'strict' + system fontFamily — the empirical zero-egress contract, ADR-4 §5), pinned exact-match by test. - smoke_report_html(&CteGraph): a placeholder renderer assembling a self-contained HTML that inlines all five assets and renders the graph as a Mermaid graph LR diagram. PR 8b replaces it with the askama renderer and wires that into the run loop; PR 8a does not touch cli. - tests/assets_manifest.rs: a cargo test asserting every assets/ file is listed, every SHA-256 matches disk, every license is permissive, and provenance is complete. tests/asset_embed.rs: integration coverage against the real jaffle-shop CteGraph. - A new assets-manifest-gate CI job, mirroring fixture-manifest-gate: structural enforcement that no file under assets/ is unlisted and no asset license is copyleft. - Doc sync (folds in the CAO clean-arch review's MEDIUM): ARCHITECTURE.md §5 corrected to drop the stale include_bytes!/binary-favicon claim and to rename the MANIFEST field url -> source plus add path; mod.rs and templates/README.md PR-number references updated. Advances report_generation.feature (the self-contained-bundle aspect) and zero_egress.feature (the asset-manifest invariant plus the inlined-bundle / data: favicon foundation); cucumber wiring lands PR 10. Quality: fmt, clippy pedantic -D warnings, nextest 187, cargo-deny, cargo doc -D warnings. /atdd: crap4rs strict worst 10.0 (threshold 15); cargo-mutants 98 mutants, 85 caught, 13 unviable, 0 survivors; CAO clean-arch PASS (0 HIGH; 1 MEDIUM folded in; 3 INFO carried to PR 8b). Closes #10 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR inlines vendored frontend assets at compile-time, populates ChangesAsset Embedding & Smoke Report
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 the frontend asset-inlining infrastructure, embedding CSS and JS dependencies like Sakura, jQuery, DataTables, and Mermaid directly into the binary's read-only data section at compile time. It establishes a supply-chain provenance contract via assets/MANIFEST.toml and includes a placeholder smoke renderer to verify the generation of self-contained, offline-capable HTML reports. Feedback identified a security vulnerability in the smoke renderer where unescaped CTE names are interpolated into the Mermaid diagram, which could break rendering syntax or lead to Cross-Site Scripting (XSS).
| fn node_lines(nodes: &[CteNode]) -> String { | ||
| let mut out = String::new(); | ||
| for (index, node) in nodes.iter().enumerate() { | ||
| let _ = writeln!(out, " n{index}[\"{}\"]", node.name()); |
There was a problem hiding this comment.
The node.name() is interpolated directly into the Mermaid block without escaping. This presents two risks:
- Mermaid Syntax: If a CTE name contains double quotes (e.g., from a quoted identifier in SQL), it will break the
["..."]node label syntax. - Security (XSS): Since this block is inlined into an HTML
<pre>tag, a name containing</pre><script>...</script>could lead to XSS or broken rendering when the report is viewed in a browser.
Even for a placeholder renderer, user-provided identifiers should be sanitized by escaping double quotes and HTML special characters.
Suggested change
| let _ = writeln!(out, " n{index}[\"{}\"]", node.name()); | |
| let _ = writeln!(out, " n{index}[\"{}\"]", node.name().replace('&', "&").replace('"', """).replace('<', "<").replace('>', ">")); |
CTE names are input-derived — they come from the manifest's compiled SQL. `node_lines` interpolated them unescaped into the Mermaid `["..."]` label inside the `<pre>` block, so a quoted SQL identifier carrying a `"` would break the label syntax, and one carrying HTML metacharacters could break out of the `<pre>` (broken render or script injection) when the report is opened in a browser. Add `escape_label` (escapes `& < > "`, with `&` first so the substitutions cannot compound) and apply it to every CTE name in `node_lines`. Covered by a direct unit test and a hostile-name smoke test. Flagged by the gemini-code-assist bot review on PR #26. Quality: fmt, clippy pedantic -D warnings, nextest 189. /atdd: crap4rs strict worst 10.0 (threshold 15); cargo-mutants 100 mutants, 87 caught, 13 unviable, 0 survivors. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 299-312: The assets-manifest-gate job is using actions/checkout@v4
without a pinned commit, not setting persist-credentials: false, and the
workflow lacks an explicit least-privilege permissions block; update the job so
the checkout step references a full commit SHA for actions/checkout, add
persist-credentials: false to that checkout step, and add a minimal permissions
block (e.g., contents: read and any other exact scopes needed) at the job (or
top-level) to restrict the token; target the job named assets-manifest-gate and
the checkout step in the steps list when making these changes.
In `@src/adapters/asset_embed.rs`:
- Around line 169-174: The escape_label function currently replaces double
quotes with the HTML entity """ which breaks Mermaid node labels; change
the replacement for '"' to use Mermaid-safe "`#quot`;" in the escape_label
function and update the corresponding unit test expectations (the test covering
label escaping that asserts for """) to expect "`#quot`;" instead so the test
reflects the new Mermaid-safe escaping.
In `@tests/assets_manifest.rs`:
- Around line 88-94: The code inserts filesystem paths from
abs.strip_prefix(&root)...to_string() into the set `out` using platform-native
separators, causing mismatches with manifest paths that use POSIX `/`; update
the logic around the `rel` variable (the value passed into `out.insert(rel)`) to
normalize separators to POSIX form before insertion (e.g., convert backslashes
to forward slashes or use a canonical POSIX-style join) so all entries inserted
by the `rel` computation are compared using `/` separators.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e48f7e46-406d-4a61-b4b5-7abcd9b9869a
⛔ Files ignored due to path filters (3)
assets/datatables-2.1.8.min.jsis excluded by!**/*.min.jsassets/jquery-3.7.1.min.jsis excluded by!**/*.min.jsassets/mermaid-11.15.0.umd.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (10)
.github/workflows/ci.ymlARCHITECTURE.mdassets/MANIFEST.tomlassets/datatables-2.1.8.min.cssassets/sakura-1.5.0.csssrc/adapters/asset_embed.rssrc/adapters/mod.rstemplates/README.mdtests/asset_embed.rstests/assets_manifest.rs
Comment on lines
+299
to
+312
| assets-manifest-gate: | ||
| # Vendored-asset provenance invariant. Every file under `assets/` | ||
| # (except MANIFEST.toml itself) MUST be listed in `assets/MANIFEST.toml` | ||
| # with a permissive (non-copyleft) SPDX license. This is the STRUCTURAL | ||
| # gate; `tests/assets_manifest.rs` mirrors the constraint and adds | ||
| # SHA-256 verification for fast local signal. Shape mirrors | ||
| # `fixture-manifest-gate`. No untrusted GitHub event interpolation in | ||
| # any `run:` block. | ||
| name: Asset-provenance gate (every asset listed in MANIFEST.toml) | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 5 | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: Every assets/* file must be listed in MANIFEST.toml |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the new job block has explicit permissions and hardened checkout settings.
awk '
/^ assets-manifest-gate:/ {in_job=1}
in_job {print}
/^ resource-ref-lint:/ {in_job=0}
' .github/workflows/ci.yml > /tmp/assets-manifest-gate.yml
echo "== uses entries =="
rg -n 'uses:' /tmp/assets-manifest-gate.yml
echo "== permissions block =="
rg -n '^ permissions:|^ contents:' /tmp/assets-manifest-gate.yml || true
echo "== checkout credential persistence =="
rg -n 'persist-credentials' /tmp/assets-manifest-gate.yml || trueRepository: breezy-bays-labs/cute-dbt
Length of output: 187
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=".github/workflows/ci.yml"
echo "== workflow top-level permissions (if any) =="
rg -n "^(permissions:|default:|on:)" "$file" || true
head -n 120 "$file" | nl -ba | sed -n '1,120p'
echo
echo "== assets-manifest-gate job block (context) =="
nl -ba "$file" | sed -n '250,360p'
echo
echo "== uses entries within assets-manifest-gate =="
awk '
/^ assets-manifest-gate:/ {in_job=1}
in_job {print}
/^ [a-zA-Z0-9_-]+:/ { if (seen && !/^ assets-manifest-gate:/) exit }
BEGIN{seen=0}
/^ assets-manifest-gate:/ {seen=1}
' "$file" | rg -n "uses:|with:|permissions:|persist-credentials" || true
echo
echo "== checkout occurrences elsewhere (to see existing hardening pattern) =="
rg -n "uses:\s*actions/checkout@" "$file" -nRepository: breezy-bays-labs/cute-dbt
Length of output: 182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=".github/workflows/ci.yml"
echo "== first 140 lines (for top-level permissions/default) =="
sed -n '1,140p' "$file" | cat -n
echo
echo "== assets-manifest-gate job block (around 250-380) =="
sed -n '250,380p' "$file" | cat -n
echo
echo "== assets-manifest-gate: uses/with/permissions/persist-credentials =="
awk '
/^ assets-manifest-gate:/ {in_job=1}
in_job {print}
/^ [a-zA-Z0-9_-]+:/ { if (in_job && !/^ assets-manifest-gate:/) {exit} }
' "$file" | rg -n 'uses:|with:|permissions:|persist-credentials|checkout@'Repository: breezy-bays-labs/cute-dbt
Length of output: 12115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=".github/workflows/ci.yml"
echo "== all permissions: occurrences =="
rg -n "^[[:space:]]*permissions:" "$file" || true
echo
echo "== all persist-credentials occurrences =="
rg -n "persist-credentials" "$file" || trueRepository: breezy-bays-labs/cute-dbt
Length of output: 147
Harden assets-manifest-gate permissions and checkout action pinning.
assets-manifest-gateusesactions/checkout@v4without pinning to a full commit SHA.- No
persist-credentials: falseis set for checkout. - The workflow contains no
permissions:block anywhere, so this job inherits broader token defaults.
Suggested patch
assets-manifest-gate:
@@
runs-on: ubuntu-latest
timeout-minutes: 5
+ permissions:
+ contents: read
steps:
- - uses: actions/checkout@v4
+ - uses: actions/checkout@<FULL_LENGTH_COMMIT_SHA>
+ with:
+ persist-credentials: false🧰 Tools
🪛 zizmor (1.25.2)
[warning] 311-311: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 299-379: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[error] 311-311: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 299 - 312, The assets-manifest-gate
job is using actions/checkout@v4 without a pinned commit, not setting
persist-credentials: false, and the workflow lacks an explicit least-privilege
permissions block; update the job so the checkout step references a full commit
SHA for actions/checkout, add persist-credentials: false to that checkout step,
and add a minimal permissions block (e.g., contents: read and any other exact
scopes needed) at the job (or top-level) to restrict the token; target the job
named assets-manifest-gate and the checkout step in the steps list when making
these changes.
4 tasks
Two findings from the CodeRabbit review on PR #26: - `escape_label` mapped `"` to the HTML entity `"`, but the Mermaid block is inlined into a `<pre>` element: the browser's HTML parser decodes `"` back to `"` before Mermaid parses the `["..."]` label, breaking it. Switch to Mermaid's own `#quot;` escape — it carries no `&`, survives the HTML decode, and Mermaid resolves it to a literal quote. `& < >` keep their HTML entities; those guard the HTML layer (a raw `<` could close the `<pre>`). - `assets_manifest.rs::walk_committed_assets` built relative paths with the platform separator; normalize to POSIX `/` so the disk walk matches MANIFEST.toml's forward-slash paths on every platform. The third CodeRabbit finding — CI workflow hardening (scope GITHUB_TOKEN, SHA-pin actions, persist-credentials: false) — is deferred to #27: the new assets-manifest-gate job follows the existing workflow-wide convention, so the fix belongs workflow-wide, not bolted onto one job in isolation. Quality: fmt, clippy pedantic -D warnings, nextest 189. /atdd: crap4rs strict worst 10.0 (threshold 15); cargo-mutants 100 mutants, 87 caught, 13 unviable, 0 survivors. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Author
Bot-review dispositionsThanks to @coderabbitai and gemini-code-assist — between them they caught a real defect. Dispositions for every finding, all in commits
All |
|
Tip For best results, initiate chat on the files or code changes.
On the two CodeRabbit fixes:
On the deferred CI hardening ( The rationale is sound. Hardening one job in isolation while the other ~15 remain unpinned and unpermissioned would give a false sense of security without actually narrowing the attack surface — and risks breaking artifact/ Congratulations on the clean ʕ •ᴥ•ʔ 🎉 ✏️ Learnings added
|
cmbays
added a commit
that referenced
this pull request
May 23, 2026
asset_embed.rs (added in PR #26, merged after the branch was cut) still imported JoinType and called edge.join_type(). After rebasing onto main, update all references: JoinType → EdgeType, join_type() → edge_type(), join_label → edge_label covering all 8 EdgeType variants (From, Inner, Left, Right, Full, Cross, UnionAll, UnionDistinct). Test: join_label_covers_every_join_kind → edge_label_covers_every_edge_kind (8 variants). New test: mermaid_block_labels_from_and_union_edges. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
16 tasks done
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
1 participant
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.
PR 8a (#10) — asset-inlining infrastructure. The vendored frontend
bundle embedded into the binary's
.rodata, its supply-chain provenancecontract, and a placeholder smoke renderer proving the bundle assembles
into one self-contained, zero-egress HTML document. Not blocked on the
Claude Design hand-back — that gates PR 8b only.
What shipped
assets/): Sakura 1.5.0, jQuery 3.7.1,DataTables 2.1.8 (JS + CSS), Mermaid 11.15.0 (UMD). Each committed file
is byte-identical (SHA-256) to a fresh download from its canonical
URL — verified at vendor time — and is the same bundle the R1 spike
empirically proved renders offline with zero network requests.
assets/MANIFEST.toml— one[[asset]]entry per file (name,version, path, canonical
sourceURL,sha256, SPDXlicense; allMIT). The supply-chain artifact for the frontend bundle.
src/adapters/asset_embed.rs— each asset embedded viainclude_str!into.rodataas apub const &str. Every v0.1 assetis text, so there is no
include_bytes!user; the favicon is anempty
data:URI.MERMAID_INITis a static constant(
securityLevel:'strict'+ systemfontFamily) pinned exact-match bya test, so any edit that could reintroduce egress fails the build.
smoke_report_html(&CteGraph)— a documented placeholderrenderer: assembles a self-contained HTML inlining all five assets and
renders the graph as a Mermaid
graph LRdiagram. PR 8b replaces itwith the askama renderer and wires that into the run loop; PR 8a does
not touch
cli.tests/assets_manifest.rs(provenance integrity: everyasset listed / SHA-256 matches disk / permissive license / complete
provenance) and
tests/asset_embed.rs(integration coverage againstthe real jaffle-shop
CteGraph). Achrome_only-based egressself-test scans cute-dbt's own HTML — not the inlined bundles — for
resource-loading constructs.
assets-manifest-gatejob mirroringfixture-manifest-gate: structural enforcement that noassets/fileis unlisted and no asset license is copyleft.
ARCHITECTURE.md§5 corrected (drop the staleinclude_bytes!/binary-favicon claim;url→source+ addpath);mod.rs+templates/README.mdPR-number references updated..feature scenarios advanced
report_generation.feature(the self-contained-bundle aspect) andzero_egress.feature(the asset-manifest invariant + the inlined-bundle/
data:favicon foundation). Full cucumber wiring lands PR 10.Quality
cargo fmt,clippy --all-targetspedantic-D warnings,nextest187 passed,
cargo-deny,cargo doc -D warnings./atddgates — crap4rs strict worst 10.0 (threshold 15);cargo-mutants 98 mutants, 85 caught, 13 unviable, 0 survivors;
CAO clean-arch PASS (0 HIGH; 1 MEDIUM — the
ARCHITECTURE.md§5include_bytes!drift — folded into this PR; 3 INFO carried forward).Carried to PR 8b (#11)
smoke_report_html+ its rendering helpers must be deleted (notleft as dead public surface) when
render.rslands.whether askama templates or a
render.rshelper own it.Closes #10
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
Tests
Documentation