feat: optional CSRF layer + embedded-asset (single-binary) helper#1
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds feature-gated CSRF primitives and an Axum CsrfLayer, an EmbeddedAssets Tower service, tests for both features, README/CHANGELOG/Cargo updates, Axum adapter re-exports, and example wiring changes (routes, defaults, dev wait). ChangesCSRF Protection and Embedded Assets
Sequence Diagram(s): sequenceDiagram
participant Client
participant CsrfLayer
participant CsrfTokens
participant InnerService
Client->>CsrfLayer: HTTP Request (method, path, headers)
CsrfLayer->>CsrfLayer: parse Cookie, check exclusion, is_mutating?
alt Mutating & not excluded
CsrfLayer->>CsrfLayer: read x-xsrf-token header
CsrfLayer->>CsrfTokens: verify(cookie, header)
alt verify == false
CsrfLayer->>Client: 419 CSRF token mismatch (+ maybe Set-Cookie)
else
CsrfLayer->>InnerService: forward request
end
else
CsrfLayer->>InnerService: forward request
end
InnerService->>CsrfLayer: Response
CsrfLayer->>Client: Response (maybe Set-Cookie)
Estimated code review effort:
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@src/adapters/axum/csrf.rs`:
- Around line 118-121: The call method is creating a fresh clone of self.inner
instead of using the instance that was poll_ready'd, breaking Tower
readiness/backpressure; change call to consume or take the already-ready inner
service rather than cloning (e.g., use std::mem::take or std::mem::replace on
self.inner inside call so you run the poll_ready'd instance), keeping the
existing config clone logic and returning the Box::pin future that uses that
taken inner service; ensure you reference the call method and self.inner so no
fresh clone is used.
In `@src/adapters/axum/embed.rs`:
- Around line 116-117: The function status (fn status(code: StatusCode) ->
Response<Body>) is not formatted to match rustfmt; run rustfmt (or cargo fmt)
and adjust the function formatting accordingly so the builder call and body are
split/indented per rustfmt rules (e.g., ensure
Response::builder().status(code).body(Body::empty()).unwrap() is formatted as
rustfmt expects). After formatting, re-run cargo fmt -- --check to confirm the
mismatch is resolved.
- Line 93: The response header currently always sets header::CACHE_CONTROL to
"public, max-age=31536000, immutable", which applies immutable caching to
non-fingerprinted assets; add a helper like is_content_hashed(path: &str) (use
the suggested heuristic) and change the .header(...) call in the embed response
generation so it sets "public, max-age=31536000, immutable" only when
is_content_hashed(path) is true and otherwise sets "public, max-age=31536000"
(reference the .header(header::CACHE_CONTROL, ...) invocation in
src/adapters/axum/embed.rs to locate where to branch).
In `@src/lib.rs`:
- Around line 104-106: Run rustfmt (cargo fmt) to fix the formatting/order
mismatch in the export block in src/lib.rs; ensure the pub use items (CsrfTokens
with #[cfg(feature = "csrf")] and SsrClient, SsrPayload) are formatted/ordered
per rustfmt rules so that cargo fmt --check passes.
In `@tests/embed.rs`:
- Around line 33-37: The test assertions in tests/embed.rs (the assert_eq!
checks that compare resp.headers().get("content-type") and
resp.headers().get("cache-control")) are not formatted to rustfmt style; run
rustfmt (or cargo fmt) to reformat the file (or run cargo fmt --tests to target
tests) so the assertion blocks at the ranges around the two assert_eq!s are
formatted, then re-run cargo fmt -- --check to verify; commit the reformatted
tests.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ff80151-b36d-41cc-80ad-c2820c50d991
📒 Files selected for processing (12)
CHANGELOG.mdCargo.tomlREADME.mdexamples/axum-react-todo/Cargo.tomlexamples/axum-react-todo/src/main.rssrc/adapters/axum/csrf.rssrc/adapters/axum/embed.rssrc/adapters/axum/mod.rssrc/csrf/mod.rssrc/lib.rstests/csrf.rstests/embed.rs
| .status(StatusCode::OK) | ||
| .header(header::CONTENT_TYPE, ct) | ||
| .header(header::CONTENT_LENGTH, len) | ||
| .header(header::CACHE_CONTROL, "public, max-age=31536000, immutable") |
There was a problem hiding this comment.
Gate immutable caching to hashed assets only.
Line 93 applies immutable to every asset, which can lock browsers to stale content for non-fingerprinted filenames. This also diverges from the stated behavior (“immutable for content-hashed files”).
Suggested fix
- .header(header::CACHE_CONTROL, "public, max-age=31536000, immutable")
+ .header(
+ header::CACHE_CONTROL,
+ if is_content_hashed(path) {
+ "public, max-age=31536000, immutable"
+ } else {
+ "public, max-age=0, must-revalidate"
+ },
+ )fn is_content_hashed(path: &str) -> bool {
// simple heuristic, can be replaced with a stricter matcher
let name = path.rsplit('/').next().unwrap_or(path);
name.split('.')
.next()
.is_some_and(|stem| stem.rsplit('-').next().is_some_and(|s| s.len() >= 8 && s.chars().all(|c| c.is_ascii_hexdigit())))
}🤖 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 `@src/adapters/axum/embed.rs` at line 93, The response header currently always
sets header::CACHE_CONTROL to "public, max-age=31536000, immutable", which
applies immutable caching to non-fingerprinted assets; add a helper like
is_content_hashed(path: &str) (use the suggested heuristic) and change the
.header(...) call in the embed response generation so it sets "public,
max-age=31536000, immutable" only when is_content_hashed(path) is true and
otherwise sets "public, max-age=31536000" (reference the
.header(header::CACHE_CONTROL, ...) invocation in src/adapters/axum/embed.rs to
locate where to branch).
|
|
||
| fn path_excluded(path: &str, excludes: &[String]) -> bool { | ||
| excludes.iter().any(|p| { | ||
| let p = p.trim_end_matches('/'); |
There was a problem hiding this comment.
[Review · Medium] exclude() prefix matching has two silent footguns.
- Fail-open:
.exclude("/")(or.exclude("")) → aftertrim_end_matches('/')the prefix is"".path.starts_with("")is always true andpath.as_bytes().get(0) == Some(&b'/')holds for every request path, so every mutating request skips CSRF verification — a well-meaning.exclude("/")disables protection site-wide with no error. - Fail-closed:
.exclude("webhooks")(no leading slash) never matches/webhooks/...(paths always begin with/), so the endpoint keeps returning 419.
Consider rejecting/normalizing an empty-or-/ prefix and documenting that the argument must be a leading-slash path.
| /// Checks only the signature, not double-submit equality — prefer | ||
| /// [`Self::verify`] for CSRF validation. Exposed for callers that need a | ||
| /// standalone signature check (e.g. deciding whether to re-issue a cookie). | ||
| pub fn is_valid(&self, token: &str) -> bool { |
There was a problem hiding this comment.
[Review · Medium] is_valid() is pub but only checks the HMAC signature — not the cookie↔header binding.
A caller who validates a request with is_valid(header_value) instead of verify(cookie, header) accepts any server-issued token regardless of the XSRF-TOKEN cookie, defeating the double-submit guarantee (an attacker can replay a token minted in their own session). The doc steers to verify, but the only in-crate consumer is the middleware. Consider pub(crate) fn is_valid to remove the public misuse vector entirely while keeping verify as the public validation entry point.
|
|
||
| fn call(&mut self, req: Request<Body>) -> Self::Future { | ||
| let cfg = self.config.clone(); | ||
| let mut inner = self.inner.clone(); |
There was a problem hiding this comment.
[Review · Low] Tower readiness: call() drives a freshly-cloned inner that was never poll_ready'd.
poll_ready readies self.inner, but call clones self.inner and invokes the clone. With a readiness-stateful inner service (Buffer, ConcurrencyLimit, rate limiters) this bypasses backpressure accounting. Harmless with axum's always-ready Router (the intended use), and it mirrors the existing InertiaMiddleware in layer.rs, so it's consistent with the crate — flagging for completeness. Canonical fix: let mut inner = std::mem::replace(&mut self.inner, self.inner.clone()); so the polled instance is the one called.
| _ => false, | ||
| }; | ||
| if !ok { | ||
| let mut resp = Response::new(Body::from("CSRF token mismatch")); |
There was a problem hiding this comment.
[Review · Low] 419 short-circuit response sets no Content-Type.
Response::new(Body::from("CSRF token mismatch")) emits a text body with no Content-Type. The Inertia client ignores the 419 body (it reloads), so this is invisible there, but outer middleware (compression, body-decoding loggers) sees an untyped body. Cheap to set text/plain; charset=utf-8.
| .header(header::CONTENT_TYPE, ct) | ||
| .header(header::CONTENT_LENGTH, len) | ||
| .header(header::CACHE_CONTROL, "public, max-age=31536000, immutable") | ||
| .body(body) |
There was a problem hiding this comment.
[Review · Low] A .mime() override with invalid header bytes panics on every matching request.
If content_type() returns a developer-supplied override containing CR/LF/NUL or non-ASCII, Response::builder().header(CONTENT_TYPE, …).body(body).unwrap() panics — the builder error surfaces at .body(). It's config-time input (not attacker-controlled), but a typo'd MIME string becomes a per-request panic for that extension. Consider validating in .mime() or handling the builder Result gracefully.
| .status(StatusCode::OK) | ||
| .header(header::CONTENT_TYPE, ct) | ||
| .header(header::CONTENT_LENGTH, len) | ||
| .header(header::CACHE_CONTROL, "public, max-age=31536000, immutable") |
There was a problem hiding this comment.
[Review · Low] immutable Cache-Control is applied unconditionally.
public, max-age=31536000, immutable is correct for Vite content-hashed filenames, but anyone serving a non-fingerprinted file (e.g. manifest.webmanifest, robots.txt) through EmbeddedAssets gets a year-long immutable cache with no revalidation path (no ETag/Last-Modified). This matches the documented "hashed assets only" intent — worth either stating that more loudly in the doc or offering an opt-out for non-hashed paths.
Review summaryReviewed at high effort (3 finder angles → verify). Overall: ready to merge — the core is sound and well-tested. No Critical/blocking issues. The CSRF design is a signed double-submit (HMAC + 6 inline findings, all Medium or below — none block merge:
Validation that already passed on the branch: My recommendation: the two Medium items are worth a quick follow-up (guard the empty |
- exclude(): normalize missing leading slash; ignore empty/"/" prefix instead of disabling CSRF site-wide (fail closed) + tests - CsrfTokens::is_valid is now pub(crate) so it can't be misused as a validation entry point that skips the cookie<->header binding - 419 response sets Content-Type: text/plain; charset=utf-8 - EmbeddedAssets: invalid .mime() override falls back to octet-stream instead of panicking the response builder + test - CsrfMiddleware: drive the poll_ready'd inner via mem::replace (Tower contract) rather than an unpolled clone - doc: note EmbeddedAssets assumes content-hashed filenames
Review fixes applied —
|
| # | Finding | Resolution |
|---|---|---|
| Medium | exclude() fail-open/fail-closed footguns |
exclude() now prepends a missing leading slash, and path_excluded ignores an empty/"/" prefix (fails closed) instead of disabling CSRF site-wide. + 2 tests (exclude_root_does_not_disable_csrf, exclude_without_leading_slash_still_matches). |
| Medium | is_valid() public → double-submit bypass if misused |
Now pub(crate). verify is the only public validation entry point. |
| Low | Tower poll_ready/clone |
call() now drives the readied instance via std::mem::replace, leaving a clone for next time. |
| Low | 419 has no Content-Type |
Sets text/plain; charset=utf-8. |
| Low | .mime() invalid value panics |
Invalid override now falls back to application/octet-stream instead of panicking the builder. + test. |
| Low | Unconditional immutable cache |
Documented on EmbeddedAssets that it assumes content-hashed filenames (behavior unchanged — by design). |
Verification: cargo test --all-features green (60 lib + 11 csrf + 6 embed + 6 axum + 1 conformance + 26 spec-coverage + 3 doctests), cargo clippy --all-features --all-targets -- -D warnings clean, default no-features build/test unaffected, example builds.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/adapters/axum/embed.rs (1)
124-126:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRun
cargo fmtto fix CI formatting failures.Pipeline reports formatting differences in the
statusfunction. The exact diff isn't shown, but CI consistently flags this range across multiple runs.#!/bin/bash # Check what rustfmt wants to change cd src/adapters/axum && rustfmt --check embed.rs 2>&1 || true🤖 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 `@src/adapters/axum/embed.rs` around lines 124 - 126, The formatting of the status function (fn status(code: StatusCode) -> Response<Body>) is off and failing CI; run rustfmt/cargo fmt or manually reformat that function so it matches rustfmt style (fix spacing, indentation and line breaks around Response::builder().status(code).body(Body::empty()).unwrap()), then commit the formatted changes so the embed.rs file passes cargo fmt checks.
♻️ Duplicate comments (2)
tests/embed.rs (2)
76-79:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRun
cargo fmtto fix CI formatting failures.Pipeline reports formatting differences in the
EmbeddedAssets::newclosure and.mime()chaining.🤖 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 `@tests/embed.rs` around lines 76 - 79, CI detected rustfmt/style issues in the tests where the EmbeddedAssets::new closure and the chained .mime("custom", ...) call are not formatted; run `cargo fmt` (or apply rustfmt) to reformat the file so the closure passed to EmbeddedAssets::new and the subsequent .mime(...) chaining follow the project's rustfmt rules, ensuring the closure body and method chain are line-wrapped/indented per rustfmt for EmbeddedAssets::new and the .mime method.
32-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRun
cargo fmtto fix CI formatting failures.Pipeline reports formatting differences in the assertion formatting. The
assert_eq!calls withresp.headers().get(...)need reformatting to match rustfmt style.🤖 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 `@tests/embed.rs` around lines 32 - 37, The assertions using assert_eq! with resp.headers().get(...).unwrap() are not formatted to rustfmt style; run cargo fmt and update the three assert_eq! calls so each expression follows rustfmt formatting (e.g., keep short assertions on one line or let rustfmt reflow them) — target the assert_eq! macros that reference resp, resp.headers().get(...), and resp.status() to ensure they match CI formatting.
🤖 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.
Outside diff comments:
In `@src/adapters/axum/embed.rs`:
- Around line 124-126: The formatting of the status function (fn status(code:
StatusCode) -> Response<Body>) is off and failing CI; run rustfmt/cargo fmt or
manually reformat that function so it matches rustfmt style (fix spacing,
indentation and line breaks around
Response::builder().status(code).body(Body::empty()).unwrap()), then commit the
formatted changes so the embed.rs file passes cargo fmt checks.
---
Duplicate comments:
In `@tests/embed.rs`:
- Around line 76-79: CI detected rustfmt/style issues in the tests where the
EmbeddedAssets::new closure and the chained .mime("custom", ...) call are not
formatted; run `cargo fmt` (or apply rustfmt) to reformat the file so the
closure passed to EmbeddedAssets::new and the subsequent .mime(...) chaining
follow the project's rustfmt rules, ensuring the closure body and method chain
are line-wrapped/indented per rustfmt for EmbeddedAssets::new and the .mime
method.
- Around line 32-37: The assertions using assert_eq! with
resp.headers().get(...).unwrap() are not formatted to rustfmt style; run cargo
fmt and update the three assert_eq! calls so each expression follows rustfmt
formatting (e.g., keep short assertions on one line or let rustfmt reflow them)
— target the assert_eq! macros that reference resp, resp.headers().get(...), and
resp.status() to ensure they match CI formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 93511bc5-2e4c-4986-8aee-be02fa21439a
📒 Files selected for processing (5)
src/adapters/axum/csrf.rssrc/adapters/axum/embed.rssrc/csrf/mod.rstests/csrf.rstests/embed.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Cargo.toml (1)
51-51: Updategetrandomdependency check (Cargo.toml)
getrandom0.3 is not current: crates.io latest stable is0.4.2(latest0.3.xis0.3.4).- No known security vulnerabilities were reported by OSV for
getrandom 0.3.4(vulns=null), so the main action is currency (consider bumping to a newer compatiblegetrandomversion).🤖 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 `@Cargo.toml` at line 51, The Cargo.toml lists getrandom = { version = "0.3", optional = true } which is out of date; update the dependency to a current compatible version (for example "0.4.2") while preserving the optional = true flag and any features or dependency targets that rely on getrandom; locate the getrandom entry in Cargo.toml and change its version string to the newer release, then run cargo update / cargo check to verify no downstream API or feature breakage in functions/modules that depend on getrandom.
🤖 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.
Nitpick comments:
In `@Cargo.toml`:
- Line 51: The Cargo.toml lists getrandom = { version = "0.3", optional = true }
which is out of date; update the dependency to a current compatible version (for
example "0.4.2") while preserving the optional = true flag and any features or
dependency targets that rely on getrandom; locate the getrandom entry in
Cargo.toml and change its version string to the newer release, then run cargo
update / cargo check to verify no downstream API or feature breakage in
functions/modules that depend on getrandom.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e6320db-28c9-45df-ae0f-ed12b2bd1eac
📒 Files selected for processing (2)
CHANGELOG.mdCargo.toml
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
- default-run = "axum-react-todo" so `cargo run` / `just dev` aren't
ambiguous with the gen-bindings binary
- /todos/:id -> /todos/{id}: axum 0.8 rejects the 0.7 colon syntax at
runtime (panic on router build); the server now boots
Both pre-existing, unrelated to the csrf/embed features.
The ts-bindings codegen extracted route params with the axum 0.7 colon/star
syntax (:id, *rest), so under the axum 0.8 dependency a {id} route yielded a
param-less URL helper. Add a shared segment_param() helper used by both
parse_params() and write_url_template(), so the type signature and the URL
template stay in sync, and cover it with a unit test.
SSR mode runs the backend with ssr_required(true). just dev launched the Bun SSR sidecar and cargo run in parallel, so on a warm build the backend could take the first request before the sidecar finished booting and return 'ssr transport: error sending request'. Gate the backend start on the sidecar's /health endpoint.
1 participant
Summary
Two opt-in, feature-gated additions surfaced as feedback from a downstream app. Both are off by default and change no existing behavior.
1.
csrf— Inertia/axios-compatible CSRF protectionThe Inertia client (axios) reads an
XSRF-TOKENcookie and echoes it back as theX-XSRF-TOKENheader on every mutating request — no frontend code needed. This adds the server side of that convention:CsrfTokens(src/csrf/mod.rs) — framework-agnostic, stateless signed double-submit token minter/verifier. HMAC-SHA256 (reusing thecookie-sessioncrypto stack),getrandomentropy,subtleconstant-time comparison. No server-side session required. The HMAC signature makes it stronger than plain double-submit: an attacker able to write cookies (sibling subdomain, MITM over HTTP) still can't forge a server-issued token.CsrfLayer(src/adapters/axum/csrf.rs) — a standalone tower layer composed next toInertiaLayer. Issues a JS-readableXSRF-TOKENcookie (Path=/, SameSite=Lax, Secure-by-default, not HttpOnly), verifies the header on POST/PUT/PATCH/DELETE, and short-circuits 419 before the handler runs on mismatch. Builder:.secure(),.same_site(),.cookie_name(),.header_name(),.exclude()(for webhooks).2.
embed— single-binary embedded assetsViteRootView::productionemits asset tags but the bytes had to come fromServeDir(disk). For single-binary deploys (rust-embed /include_dir), the manifest already embeds viainclude_str!(...).parse(); this adds the byte-serving half:EmbeddedAssets(src/adapters/axum/embed.rs) — an infallible axumServicefornest_service, built from a resolver closureFn(&str) -> Option<Cow<'static, [u8]>>. Works with rust-embed,include_dir, or a plain map — Veer depends on none of them. Content-type from a built-in extension map (.mime()to extend), immutableCache-Controlfor content-hashed files, zero-copy for'staticborrowed bytes,Allowheader on 405.Also included
Cargo.tomlfeature flags (csrf,embed) + new optional depsgetrandom,subtle(the rest shared withcookie-session).lib.rs+ README feature-table rows; two README cookbook subsections.CsrfLayerwired into theaxum-react-todoexample.CHANGELOG.md(Keep a Changelog format).Testing
cargo test --all-features: all green — 60 lib unit tests, 9 csrf + 5 embed integration tests, plus the existing 6 axum / 1 conformance / 26 spec-coverage suites and 3 doctests.cargo clippy --all-features --all-targets -- -D warnings: clean.csrf.csrfalone,embedalone, both, all).Review notes
Built via TDD, task-by-task, with per-task spec + code-quality review and a final holistic security review. CSRF-specific scrutiny: no path lets a mutating request reach the handler without a valid token; the 419 path reissues a cookie only when the client lacks a valid one (no needless rotation); constant-time verification throughout.
Summary by CodeRabbit
Release
New Features
Documentation
Tests / Examples