fix(server): static 400s and identifier scrub#77
Conversation
Log-hygiene hardening from the launch audit (dodopayments#58): H4: extractor rejections and the preference channel check answered 400s with prose that quotes caller-supplied scalars. Rejection messages are now static per failure class, with a canary regression test proving no echo. H3: CHIMELY_LOG_SCRUB_IDENTIFIERS (default off) additionally redacts subscriber_id and environment query params and the subscriber path segment from the access log and the http.request span. subscriber_hash stays unconditionally scrubbed. H2: the plain-INSERT conflict interception rule is recorded at the generic error-logging site. Closes dodopayments#58
Greptile SummaryThis PR hardens request errors and access-log scrubbing. The main changes are:
Confidence Score: 2/5The test suite can fail to compile after the new config field is added.
server/src/config.rs and server/tests/chaos.rs Important Files Changed
Reviews (1): Last reviewed commit: "fix(server): static 400s and identifier ..." | Re-trigger Greptile |
| /// always scrubbed regardless. This flag additionally covers the | ||
| /// subscriber_id and environment query params and the subscriber path | ||
| /// segment, for operators whose logs leave their control. | ||
| pub log_scrub_identifiers: bool, |
There was a problem hiding this comment.
Adding log_scrub_identifiers as a required Config field leaves the direct Config literal in server/tests/chaos.rs incomplete. That test target still closes the struct after shutdown_drain_deadline, so compiling the test suite fails with a missing-field error before the new tests can run.
Context Used: CLAUDE.md (source)
|
I independently implemented this against #58 and hit a few edge cases in testing that look like they apply to this diff too. Sharing them in case they save you a review round-trip.
move |req, next| access_log(scrub_identifiers, req, next)to let path = if scrub_identifiers {
scrub_path(req.uri().path())
} else {
req.uri().path().to_owned()
};to just
log_scrub_identifiers: parse_var("CHIMELY_LOG_SCRUB_IDENTIFIERS", false)?,No test exercises unsafe { std::env::remove_var("CHIMELY_LOG_SCRUB_IDENTIFIERS") }
assert!(!Config::from_env().unwrap().log_scrub_identifiers);
unsafe { std::env::set_var("CHIMELY_LOG_SCRUB_IDENTIFIERS", "true") }
assert!(Config::from_env().unwrap().log_scrub_identifiers);
unsafe { std::env::set_var("CHIMELY_LOG_SCRUB_IDENTIFIERS", "false") }
assert!(!Config::from_env().unwrap().log_scrub_identifiers);(
Err(rejection) => Err(ApiError::bad_request(json_rejection_message(&rejection))),in the Happy to share the full boundary-test file from item 1 if useful. The log-capture harness is about 50 lines and drops in as |
Closes #58.
The three hardenable items from the launch log-hygiene audit.
H4 -- 400 bodies no longer echo caller input
extract.rs: the three rejection arms passed axum'sbody_text()into the envelope, and that prose quotes caller-supplied scalars on a type mismatch (serdeinvalid type: string "...", unknown-variant/field names via serde_path_to_error). Rejections now map to static per-class messages: JSON syntax vs schema mismatch vs content-type vs query string, so callers keep enough signal to debug without any caller bytes reflecting back. The failing field name is deliberately not included -- axum only exposes composed prose, and the serde path segments can themselves be caller input (keys of the free-formpayloadobject).preferences.rs:unknown channel: {}echoed the caller's channel value; now static (channel must be one of: in_app).rejection_messages_never_echo_caller_input: aCANARY-9f2token in a mismatched body field, a query param, and a channel value must never surface in the 400 body, and each arm's static message is pinned exactly. No existing test asserts the old prose (audited every message assertion in the suite -- the only message-text assertion anywhere is the admin login one, untouched).H3 -- opt-in identifier scrubbing for the access log
CHIMELY_LOG_SCRUB_IDENTIFIERS(defaultfalse, so default behavior is byte-identical). When set,scrub_queryadditionally redactssubscriber_idandenvironmentvalues (same machinery as the hash scrub: case-insensitive, percent-decoded names, re-encoded output), and a newscrub_pathredacts the segment after anysubscriberssegment, covering/v1/subscribers/{id},/v1/subscribers/{id}/preferences, and the admin subscriber lookup. The scrubbed path is computed before thehttp.requestspan is created, so the redaction reaches exported traces too -- the issue names the span as a leak surface.subscriber_hashstays unconditionally scrubbed (the sse.rs invariant test keeps pinning it). The flag reaches the stateless middleware as a captured bool inmiddleware::from_fn; documented in the self-hosting env table.Scope note: the
{env_id}path segment on admin routes is not scrubbed -- it is an internal UUID, not the operator-facing slug theenvironment=query param carries.H2 -- rule recorded
The plain-INSERT conflict-interception rule is recorded as a doc comment at the generic
From<E> for ApiErrorlogging site, where a new violation would land.Verification
cargo fmt --checkpasses. Unit tests added for the query scrub (opt-in on/off, case-insensitivity, percent-decoded names) and the path scrub (all three route shapes plus edges). My environment cannot run clippy/nextest locally (no MSVC toolchain); flagging so a maintainer CI approval gets the full suite. No OpenAPI annotation changed, so no generated-artifact churn.