fix(serve): bind action replay to sessions - #16
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
integrate-your-mind
left a comment
integrate-your-mind
left a comment
There was a problem hiding this comment.
Reviewed the exact head 22a2fe4. The atomic consume path, route/session matching, failure behavior, and bounded-store logic look consistent. I left four actionable inline comments covering two cookie parsing/API edge cases, one Rust source-compatibility issue, and one deployment requirement that should be stated more precisely. I did not rerun the test suite; this review is based on the exact-head source and the verification evidence recorded in the PR.
| } | ||
|
|
||
| fn server_action_session_fingerprint(headers: &HttpHeaders, cookie_name: &str) -> Option<[u8; 32]> { | ||
| let header = headers.get("cookie")?; |
There was a problem hiding this comment.
HttpHeaders is a public BTreeMap, and the new handle_get_with_headers path is documented for Rust embedders. Looking up only the lowercase key makes Cookie or COOKIE silently behave as a missing session even though HTTP field names are case-insensitive; the socket parser hides this by lowercasing names first. Please use the existing header_value(headers, "cookie") helper here (and consider the adjacent CSRF-cookie helper) or normalize headers at the public boundary, with a mixed-case regression test.
| if fingerprint.is_some() { | ||
| return None; | ||
| } | ||
| let value = value.trim(); |
There was a problem hiding this comment.
These trims make malformed cookie-pairs such as app_session = session-123 and app_session= session-123 valid, while the continue above also lets app_session; app_session=session-123 succeed. That conflicts with the documented fail-closed behavior and can create a parsing difference from the app/auth layer whose session Ferrite is meant to bind. Please reject malformed occurrences of the configured name rather than normalizing them, and add these cases to the fingerprint tests.
| pub max_in_flight_requests: usize, | ||
| pub server_action_csrf_token: Option<String>, | ||
| pub server_action_csrf_cookie_name: Option<String>, | ||
| pub server_action_session_cookie_name: Option<String>, |
There was a problem hiding this comment.
Adding a field to this public, non-#[non_exhaustive] struct is a source-breaking Rust API change: downstream struct literals and exhaustive patterns no longer compile. The PR currently marks “No public API … break.” At minimum, please record this compatibility impact in the PR/release notes; otherwise the config shape needs a compatibility-preserving extension strategy.
| - external tracing and audit sinks beyond stderr request/action logs and in-memory metrics counters | ||
|
|
||
| Until those exist, deploy server actions only for controlled beta scenarios or behind app-owned authentication and CSRF middleware that has been reviewed separately. If server actions are enabled in production, set `--server-action-csrf-token-env`, prefer `--server-action-csrf-cookie-name`, and set `--server-action-replay-ttl-ms` when a single Ferrite process owns the action form and action POST path. Rotate the referenced CSRF secret as part of the deployment process. The configured token must be cookie-safe when cookie binding is enabled. If the public TLS origin differs from the upstream Ferrite bind origin, set `--trusted-proxy-public-origin` and configure the proxy to own and sanitize the forwarded proto/host headers. If access logs need public client IPs behind the proxy, set `--trusted-proxy-client-ip-hops` to the exact number of trusted proxy hops and make the edge proxy overwrite `X-Forwarded-For`. | ||
| Until those exist, deploy server actions only for controlled beta scenarios or behind app-owned authentication and CSRF middleware that has been reviewed separately. If server actions are enabled in production, set `--server-action-csrf-token-env`, prefer `--server-action-csrf-cookie-name`, and set `--server-action-replay-ttl-ms` when a single Ferrite process owns the action form and action POST path. When the application already issues a stable, opaque, high-entropy authenticated session cookie, add `--server-action-session-cookie-name <name>` to bind each new nonce to that cookie and route. Ferrite does not create, authenticate, authorize, renew, revoke, or configure that cookie. The application or identity layer must enforce TLS plus appropriate `Secure`, `HttpOnly`, `SameSite`, `Path`, `Domain`, expiry, signing/encryption, fixation resistance, and logout rotation policy. |
There was a problem hiding this comment.
Please state explicitly that the session cookie Path must cover both the route GET and /_ferrite/action—normally Path=/. With a narrower path, Ferrite can render a bound nonce on the page but the browser omits the cookie on the action POST, so every action fails with the generic replay error. “Appropriate Path” is easy to miss for this specific two-endpoint requirement.
2 participants
Summary
This is an AI-authored contribution reviewed by an independent AI security reviewer. It is stacked on #13 and must not merge before that base is finalized and the merge ref is revalidated.
Related issue
No issue. This closes the documented session-binding gap for process-local replay protection; distributed replay defense remains explicitly unsupported.
Change type
Verification
pnpm lintpnpm typecheckpnpm buildpnpm testCurrent reviewed head:
22a2fe4129438e7ba3962ef3991487e898058390Runtime/test head:
94eecd09843c1b9cac76dc102b4962d22d1f126822a2fe4changes only one deployment-summary sentence after94eecd0; runtime, CLI, browser, and test sources are byte-identical.Task-owned cleanup: 611,560 KiB from the first exact functional/coverage cycle, 361,608 KiB from completed coverage, and 248,048 KiB from the documentation-head cycle were removed. The pre-existing
packages/protocol/distremained at tree hashfe29f3fbb176a7a6315d25b3984329eb602d4dfda57145e37e5d497e6f6b486f.Risk and failure paths
Normal: one valid session-bound nonce invokes its route action once.
Failure/odd paths covered: missing and expired nonce; wrong route; missing, duplicate, quoted, malformed, or rotated session cookie; concurrent duplicate POST; render failure; render timeout and unknown outcome; retry with a fresh nonce; bounded store pruning; selective discard; exact expiry deadline; CSRF and origin checks; trusted-proxy origin; generic public errors and privacy-safe internal logs.
Deployment assumptions:
Breaking changes
The new CLI flag is optional. Existing deployments behave as before when it is omitted. If configured, it requires replay protection and fails closed on invalid cookie names or missing sessions.
Security and privacy
The replay store retains only a SHA-256 session-cookie fingerprint, not the raw cookie. Public action failures remain generic; internal logs do not include tokens, cookies, or submitted form data.
Rollback
Revert the seven branch commits, or omit
--server-action-session-cookie-nameto disable only session binding. Existing CSRF and process-local replay flags remain independently configurable. No migration or persistent user state is introduced.Reviewer notes
Start with
crates/ferrite-dev-server/src/lib.rsreplay-store consumption and cookie parsing, then the CLI fail-closed configuration andtest/browser-server-actions.test.mjs. Independent source/security review at22a2fe4is ACCEPT with no P0-P3.Current disposition: READY FOR CODE REVIEW; NOT MERGE ELIGIBLE. Required next gates are #13 finalization, base reconciliation, exact merge-ref full-suite rerun, and repository-required review/checks.