fix(server): static 400s and opt-in identifier scrub (alt to #77)#83
fix(server): static 400s and opt-in identifier scrub (alt to #77)#83ravidsrk wants to merge 8 commits into
Conversation
Extractor rejections and the unknown-channel 400 echoed caller input (serde prose quotes scalars on type mismatch). Messages are now static so no request-derived text reaches a response body. Closes H4 of dodopayments#58.
A sqlx unique violation logged via ?err embeds Postgres DETAIL key values. Record the rule on the blanket From impl: conflicts on secret- or PII-keyed tables are intercepted before conversion. Closes H2 of dodopayments#58.
CHIMELY_LOG_SCRUB_IDENTIFIERS=true replaces subscriber_id and
environment query params and the /v1/subscribers/{id} path
segment with truncated SHA-256 hashes, in the access log and
the http.request span both. Default off, behavior unchanged.
The subscriber_hash credential scrub stays unconditional.
Closes H3 of dodopayments#58.
Requests flow through a router configured with the flag on and off. Assertions read captured access-log lines and http.request span fields, so reverting either middleware call site goes red.
|
| Filename | Overview |
|---|---|
| server/src/http.rs | Adds opt-in path/query identifier scrubbing via SHA-256 truncation; correctly threads scrub flag through AppState; access_log middleware migrated to from_fn_with_state. Admin subscriber path is outside scrub_path coverage (documented but may surprise operators). |
| server/src/extract.rs | Extractor rejections now drop the rejection payload and return a static message; eliminates axum/serde prose that could echo caller scalars into the response body. |
| server/src/config.rs | Adds log_scrub_identifiers bool field with default false, parse_var wiring, Debug impl inclusion, and an env-var contract test that mutates the process environment under unsafe (safe because nextest gives each test its own process). |
| server/tests/log_scrub.rs | New integration test file capturing tracing output via a global BufWriter subscriber to assert scrubbed/raw paths and query params across the config→middleware→output boundary; correctly relies on nextest per-test process isolation for try_init(). |
| server/tests/management.rs | Extends malformed-body test to assert static message content and absence of echoed scalars; adds new malformed_optional_bodies_get_the_static_message test covering the OptionalFromRequest path. |
| server/src/error.rs | Adds doc comment on From for ApiError documenting the conflict-interception requirement (H2) for PII-keyed INSERT paths; no behaviour change. |
| server/src/api/preferences.rs | Channel validation now returns static 'unknown channel' instead of echoing the caller-supplied channel value; comment documents the H4 invariant. |
| server/tests/inbox.rs | Extends unknown-channel 400 test to assert static message text and absence of the submitted channel value in the response body. |
| server/tests/support/mod.rs | Adds log_scrub_identifiers: false to the default test Config struct; no behaviour change. |
| server/tests/chaos.rs | Adds log_scrub_identifiers: false to the chaos test's Config struct; no behaviour change. |
Reviews (1): Last reviewed commit: "test(server): prove scrub across the log..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| pub fn scrub_path(path: &str, scrub_identifiers: bool) -> String { | ||
| if !scrub_identifiers { | ||
| return path.to_owned(); | ||
| } | ||
| match path.strip_prefix("/v1/subscribers/") { | ||
| None | Some("") => path.to_owned(), | ||
| Some(rest) => match rest.split_once('/') { | ||
| Some((id, suffix)) => { | ||
| format!("/v1/subscribers/{}/{suffix}", hash_identifier(id)) | ||
| } | ||
| None => format!("/v1/subscribers/{}", hash_identifier(rest)), | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
scrub_path only covers the /v1/subscribers/ prefix. The admin route /admin/api/environments/{env_id}/subscribers/{subscriber_id} (served by admin::get_subscriber) will log the raw subscriber ID in the path field even when log_scrub_identifiers = true. The doc comment in Config explicitly limits coverage to "the /v1/subscribers/{id} path segment", so this is documented behaviour, but an operator enabling the flag to keep subscriber PII out of their log pipeline would still see subscriber IDs leaked through admin-plane access logs without any indication of the gap.
There was a problem hiding this comment.
Fixed in db703e0. scrub_path now hashes the path segment that follows any subscribers segment instead of matching the /v1/subscribers/ prefix, so /admin/api/environments/{env_id}/subscribers/{subscriber_id} scrubs the subscriber id the same way the public route does (unit-tested both flag states, failing-first). The Config doc comment now says the coverage spans the public and admin planes. The admin {env_id} segment stays raw deliberately: it is an instance-minted UUID, unlike the environment query param, which is the customer-chosen slug and is why the query scrub hashes it.
scrub_path matched only the /v1/subscribers/ prefix, so the admin
route /admin/api/environments/{env}/subscribers/{id} logged raw
subscriber ids with log_scrub_identifiers on. Hash the segment
after any subscribers segment instead and update the Config doc.
1 participant
This PR is an alternative implementation of #58 (H2-H4), opened alongside #77 (thanks @soufian3hm).
From<E> for ApiError.CHIMELY_LOG_SCRUB_IDENTIFIERSflag hashes subscriber/environment identifiers in the access log and thehttp.requestspan, defaulting off so existing operators see no log-format change.The test coverage is the main differentiator, and it's what my review of #77 flagged as missing there: a config-to-output boundary test (identifiers hashed on, raw off), a
Config::from_envtest pinning the default-off contract for absent/true/false, and a malformed optional-body test asserting the static message with no input echo. Each test has a recorded mutation red-proof.