Skip to content

fix(server): static 400s and identifier scrub#77

Open
soufian3hm wants to merge 2 commits into
dodopayments:mainfrom
soufian3hm:fix/log-hygiene
Open

fix(server): static 400s and identifier scrub#77
soufian3hm wants to merge 2 commits into
dodopayments:mainfrom
soufian3hm:fix/log-hygiene

Conversation

@soufian3hm

Copy link
Copy Markdown
Contributor

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's body_text() into the envelope, and that prose quotes caller-supplied scalars on a type mismatch (serde invalid 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-form payload object).
  • preferences.rs: unknown channel: {} echoed the caller's channel value; now static (channel must be one of: in_app).
  • New regression test rejection_messages_never_echo_caller_input: a CANARY-9f2 token 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 (default false, so default behavior is byte-identical). When set, scrub_query additionally redacts subscriber_id and environment values (same machinery as the hash scrub: case-insensitive, percent-decoded names, re-encoded output), and a new scrub_path redacts the segment after any subscribers segment, covering /v1/subscribers/{id}, /v1/subscribers/{id}/preferences, and the admin subscriber lookup. The scrubbed path is computed before the http.request span is created, so the redaction reaches exported traces too -- the issue names the span as a leak surface. subscriber_hash stays unconditionally scrubbed (the sse.rs invariant test keeps pinning it). The flag reaches the stateless middleware as a captured bool in middleware::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 the environment= query param carries.

H2 -- rule recorded

The plain-INSERT conflict-interception rule is recorded as a doc comment at the generic From<E> for ApiError logging site, where a new violation would land.

Verification

cargo fmt --check passes. 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.

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-apps

greptile-apps Bot commented Jul 15, 2026

Copy link
Copy Markdown

Greptile Summary

This PR hardens request errors and access-log scrubbing. The main changes are:

  • Static 400 messages for JSON, query, and preference-channel validation failures.
  • Opt-in scrubbing for subscriber and environment identifiers in access logs and spans.
  • Documentation for CHIMELY_LOG_SCRUB_IDENTIFIERS.
  • A logging invariant comment for plain INSERT conflict handling.

Confidence Score: 2/5

The test suite can fail to compile after the new config field is added.

  • Config now requires log_scrub_identifiers.
  • A direct Config literal in server/tests/chaos.rs has not been updated.
  • The main request-scrubbing behavior otherwise appears consistent with the changed tests and wrapper contracts.

server/src/config.rs and server/tests/chaos.rs

Important Files Changed

Filename Overview
server/src/config.rs Adds log_scrub_identifiers to runtime config, env parsing, and debug output, but leaves another direct test initializer incomplete.
server/src/http.rs Threads the scrub flag into access logging and adds query/path redaction helpers.
server/src/extract.rs Replaces extractor rejection body text with static messages.
server/src/api/preferences.rs Replaces the unknown-channel echo with a static allowed-channel message.
server/tests/management.rs Adds regression coverage for non-echoing 400 responses.
server/tests/support/mod.rs Updates the shared test config factory with the new scrub flag default.
server/src/error.rs Documents the conflict-interception requirement at the generic error logging path.
docs/content/docs/self-hosting.mdx Documents the new identifier-scrubbing environment variable.

Reviews (1): Last reviewed commit: "fix(server): static 400s and identifier ..." | Re-trigger Greptile

Comment thread server/src/config.rs
/// 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,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0 Required Config Field Missing

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)

@ravidsrk

Copy link
Copy Markdown

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.

  1. The scrub is tested at the helper seam but not across the log boundary. identifier_scrubbing_is_opt_in and scrub_path_redacts_the_subscriber_segment call the helpers directly, so the wiring is unpinned: hard-coding the captured bool in
move |req, next| access_log(scrub_identifiers, req, next)

to false, or reverting

let path = if scrub_identifiers {
    scrub_path(req.uri().path())
} else {
    req.uri().path().to_owned()
};

to just req.uri().path().to_owned(), leaves every test in this diff green while the flag silently does nothing. The span half is the subtle one. "The scrubbed path is computed before the http.request span is created" holds today only by statement order, so a refactor that opens the span before the scrub also passes everything. What closed this for me was an integration test that installs a buffered tracing_subscriber::fmt writer with .with_span_events(FmtSpan::NEW) (safe as a global install because nextest runs each test in its own process), sends a real PUT /v1/subscribers/usr_123 plus a request with subscriber_id/environment in the query, and asserts the redaction separately on the span-open line and the chimely::access line, with a mirror test asserting raw identifiers still log unchanged when the flag is off. I verified each of the call-site reverts above fails independently against that pair.

  1. The default is claimed but not pinned. The description says default behavior is byte-identical, and that rests on the one literal in
log_scrub_identifiers: parse_var("CHIMELY_LOG_SCRUB_IDENTIFIERS", false)?,

No test exercises Config::from_env with the variable absent, true, or false, so flipping that false to true (or a parse_var regression) passes the full suite while changing every operator's log format. A small test covers it:

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);

(DATABASE_URL needs to be set first for from_env to succeed; the env mutation is safe under nextest process-per-test.)

  1. The OptionalFromRequest arm is fixed but never exercised. rejection_messages_never_echo_caller_input drives the required-body route (POST /v1/notifications), a query param, and the channel value, but the optional-body path lives on PUT /v1/subscribers/{id} (body: Option<ApiJson<UpsertSubscriberRequest>>). Reverting only
Err(rejection) => Err(ApiError::bad_request(json_rejection_message(&rejection))),

in the OptionalFromRequest impl back to rejection.body_text() keeps the new test green, and a malformed optional body echoes serde prose again. My red run captured invalid type: integer `9001` from exactly this branch. One more case in your canary test closes it: PUT /v1/subscribers/usr_x with {"created_at": 9001}, asserting the static schema message and !text.contains("9001").

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 server/tests/log_scrub.rs on top of the test support you already touched.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Log-hygiene hardening from the launch audit (H2-H4)

3 participants