diff --git a/.specs/README.md b/.specs/README.md index 708923b..a637d32 100644 --- a/.specs/README.md +++ b/.specs/README.md @@ -78,6 +78,7 @@ board (`backlog/` · `in-progress/` · `blocked/` · `done/`). | [plans/2026-07-02-webhook_user_sync_conformance/plan.md](plans/2026-07-02-webhook_user_sync_conformance/plan.md) | Done | [changes/merged/2026-07-01-webhook_user_sync_conformance.md](changes/merged/2026-07-01-webhook_user_sync_conformance.md) | | [plans/2026-07-02-server_error_handling_and_shutdown/plan.md](plans/2026-07-02-server_error_handling_and_shutdown/plan.md) | Done | [changes/merged/2026-07-01-server_error_handling_and_shutdown.md](changes/merged/2026-07-01-server_error_handling_and_shutdown.md) | | [plans/2026-07-02-implement_lambda_runtime/plan.md](plans/2026-07-02-implement_lambda_runtime/plan.md) | Done | [changes/merged/2026-07-01-implement_lambda_runtime.md](changes/merged/2026-07-01-implement_lambda_runtime.md) | +| [plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/plan.md](plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/plan.md) | Review | [changes/2026-08-05-eliminate_secret_leakage_in_logs_and_spans.md](changes/2026-08-05-eliminate_secret_leakage_in_logs_and_spans.md) | ## Conventions diff --git a/.specs/changes/2026-08-05-eliminate_secret_leakage_in_logs_and_spans.md b/.specs/changes/2026-08-05-eliminate_secret_leakage_in_logs_and_spans.md new file mode 100644 index 0000000..1330b85 --- /dev/null +++ b/.specs/changes/2026-08-05-eliminate_secret_leakage_in_logs_and_spans.md @@ -0,0 +1,483 @@ +# Change: Eliminate secret leakage in logs, spans, and error responses + +**Status:** Proposed · **Date:** 2026-08-05 · **Owner:** Ant Stanley · **Target:** crates/adapters, crates/providers, crates/server (service) + +Make it a compile error, rather than a review miss, for credential-bearing material to reach a +log line, a tracing span field, or a client-facing error body. Introduce a `Secret` newtype in +`crates/core` that implements neither `Debug` nor `Display`, wrap the enumerated set of +credential-derived values in it, replace the three sites that interpolate an upstream response +body into an error detail with a bounded, redacting shared helper, split the domain error's +internal diagnostic from its client-facing description so `/token` stops acting as a validation +oracle, and bound the client-chosen `X-Request-Id` in length and charset. + +--- + +## Motivation + +The repository already treats formatting as a disclosure channel: six types hand-implement +`Debug` purely so that a secret renders as `""` — `WebhookConfig` +(`crates/core/src/config.rs:351`), `InternalApiConfig` (`:394`), `OidcProviderConfig` +(`crates/core/src/domain/provider.rs:22`), `TokenResponse` (`crates/core/src/domain/token.rs:19`), +`ProviderTokens` (`:56`), and `AppleProvider` (`crates/providers/src/apple.rs:39`). Six correct +applications of a discipline is exactly the state in which the seventh is missed. `Session` +(`crates/core/src/domain/session.rs:4`) carries `refresh_token_hash` — the session lookup key — +and plainly derives `Debug`, so `#[instrument]`'s default argument capture publishes it: the +Valkey adapter records the whole struct at `crates/adapters/src/valkey/mod.rs:52`, and the LMDB +adapter names the digest as an explicit span field at `crates/adapters/src/lmdb/mod.rs:55` and +lets it auto-record at `:102` and `:140`. The three adapters that get it right rely on an +unenforced name match between the declared `fields(token_hash)` entry and the parameter name; a +rename defeats them silently. And `OidcProvider` (`crates/adapters/src/oidc/mod.rs:15`) holds a +configured `client_secret` with no `Debug` impl at all, derived or manual — protected only by +that absence until someone derives one. + +The same conflation appears at the provider boundary and at the HTTP boundary. Three sites treat +an upstream non-2xx response body as trusted diagnostic text and move it into +`Error::ProviderError.detail`, which `crates/server/src/error.rs:74` writes to the error log in +full: the shared token-endpoint helper's `_ => raw_body` arm +(`crates/adapters/src/shared/token_endpoint.rs:53`), the OIDC revocation path +(`crates/adapters/src/oidc/mod.rs:251-258`), and the Apple revocation path +(`crates/providers/src/apple.rs:346-353`) — the last two with no classification arm at all, on +requests that carry the token being revoked and, for Apple, a freshly signed ES256 client +assertion. And `map_domain_error_inner` (`crates/server/src/error.rs:80-124`) clones the internal +`reason` or `detail` of every 4xx variant except `UserSuspended` straight into +`error_description`, so an unauthenticated caller +learns which validation step failed and gets its own unverified `kid` echoed back +(`crates/adapters/src/oidc/mod.rs:154`, `crates/providers/src/apple.rs:255`) — the one function +that genericises the `server_error` class and guards it with an `assert_ne!` does not apply the +same rule one arm over. Each of these is a place where someone had to remember something. The +structural answer is a type that cannot be formatted, so the compiler remembers instead. + +--- + +## Affected spec pages + +| Canonical page | Nature of change | +|---|---| +| [`.specs/service/specs/01-domain-model.md`](../service/specs/01-domain-model.md) | `Session.refresh_token_hash` becomes `Secret`; `Session` gains a hand-written `Debug` | +| [`.specs/service/specs/02-ports-and-adapters.md`](../service/specs/02-ports-and-adapters.md) | `SessionRepository` token-hash parameters become `&Secret`; `Shared OIDC utilities` gains the bounded, redacting `upstream` helper | +| [`.specs/service/specs/04-http-api.md`](../service/specs/04-http-api.md) | Middleware stack item 1: `X-Request-Id` bound and charset; Error mapping: `client_description` split, every class logged | +| [`.specs/service/specs/05-provider-system.md`](../service/specs/05-provider-system.md) | Both `revoke_token` paths and `exchange_code` route upstream bodies through the shared helper | +| [`.specs/service/specs/06-configuration.md`](../service/specs/06-configuration.md) | Decisions: the per-type redacting `Debug` decision is superseded by `Secret`; `[user_sync]` and `[internal_api]` wording | +| [`.specs/service/specs/07-telemetry-and-audit.md`](../service/specs/07-telemetry-and-audit.md) | New `Telemetry hygiene` section stating what may not enter a span, log, or error body | +| [`.specs/service/specs/08-persistence.md`](../service/specs/08-persistence.md) | Session-only stores: LMDB and Valkey span-field redaction | + +No new canonical page. No change to +[`canonical-types.schema.json`](../service/specs/canonical-types.schema.json) — see `Type changes`. + +Companion change spec on the same root cause, seen from the audit side: +[2026-08-05-audit_and_throttle_authentication_failures.md](2026-08-05-audit_and_throttle_authentication_failures.md). +That spec owns the audit failure `reason` — it must be a fixed classification string precisely +because `ProviderError`'s `Display` embeds the upstream response body verbatim. This spec owns +the logging, tracing, and error-response surface; neither restates the other's delta. + +--- + +## Proposed changes + +### `.specs/service/specs/01-domain-model.md` → Entities → Session (Modify) + +> ```rust +> struct Session { +> user_id: String, +> refresh_token_hash: Secret, // SHA-256 hex; never the raw token +> provider: String, +> expires_at: DateTime, +> device_id: Option, +> user_agent: Option, +> ip_address: Option, +> created_at: DateTime, +> } +> ``` +> +> The raw refresh token exists only in memory during issuance and in the response to the client. +> Only the hash is stored, and it is stored as `Secret` — a newtype that implements +> neither `Debug` nor `Display`, so no formatter, tracing macro, or `#[instrument]` argument +> capture can render it. `Session` therefore cannot derive `Debug`; it hand-implements one that +> prints `refresh_token_hash: ""` and passes the remaining fields through. The +> serialized form is unchanged: `Secret` is transparent to `serde`, so every store writes and +> reads the same 64-character hex string it did before. `device_id`, `user_agent`, and +> `ip_address` are still populated from the request context: the audit-context middleware +> captures them at the HTTP edge and the exchange flow threads them into the stored session. + +### `.specs/service/specs/02-ports-and-adapters.md` → SessionRepository (Modify) + +> ```rust +> async fn store_refresh_token(&self, session: &Session) -> Result<()>; +> async fn get_session_by_refresh_token(&self, token_hash: &Secret) -> Result>; +> async fn revoke_session(&self, token_hash: &Secret) -> Result<()>; +> async fn revoke_all_user_sessions(&self, user_id: &str) -> Result<()>; +> async fn count_active_sessions(&self) -> Result; +> async fn cleanup_expired_sessions(&self) -> Result; // returns rows deleted +> ``` +> +> The token-hash parameters are `&Secret` rather than `&str`, so an adapter that leaves +> them out of `skip(...)` fails to compile instead of publishing the session lookup key as a span +> field. An adapter reaches the raw digest through `expose()` at the point it builds a store key. + +### `.specs/service/specs/02-ports-and-adapters.md` → Shared OIDC utilities (Modify) + +> Reused by the OIDC and Apple providers. All outbound HTTP goes through a single shared +> `reqwest::Client` per process with a 5s connect timeout, a 10s total request timeout +> (compile-time constants, not configuration), and redirects disabled; a hung or slow provider +> fails the request rather than stalling `/token`. On the token-endpoint and revocation paths a +> non-2xx response is an error whose body reaches nothing but `upstream::error_detail`; success +> payloads are parsed only from 2xx bodies. +> +> - `jwks::JwksCache` — fetches and caches a remote JWKS behind a read/write lock with a TTL +> (default 1h); `with_ttl` overrides. A non-2xx JWKS response is a `ProviderError` and is never +> cached. When a token's `kid` is not in the cached set, the cache refetches once (rate-limited +> by a 30s minimum refresh interval) before the provider rejects the token, so upstream key +> rotation takes effect immediately instead of at TTL expiry. +> - `discovery::discover(issuer)` — fetches and parses `.well-known/openid-configuration` into +> `DiscoveryDocument { issuer, token_endpoint, jwks_uri, revocation_endpoint }` and errors if +> the document's `issuer` does not equal the configured issuer (RFC 8414 §3.3). +> - `token_endpoint::exchange_code(endpoint, client_id, client_secret, code, redirect_uri)` — the +> standard form-encoded `grant_type=authorization_code` POST. A non-2xx response is turned into +> a `ProviderError` detail by `upstream::error_detail`; a 2xx response without an `id_token` is +> an error, not an empty string. +> - `http::read_bounded(response)` — reads a response body to at most `MAX_UPSTREAM_BODY_BYTES` +> (64 KiB) and returns it as `Secret`, so an upstream cannot choose how many bytes the +> service retains and the body cannot be formatted by accident. +> - `upstream::error_detail(status, body)` — the only way to build a `ProviderError` detail from +> upstream bytes. It prefers the structured RFC 6749 error object (`error`, optionally +> `error_description`); failing that it returns the status, the body's byte length, and a +> bounded excerpt (256 characters) produced by a redacting function that percent-decodes first +> and then masks the values of `token`, `refresh_token`, `client_secret`, `code`, and any +> bare compact JWS. It consumes the `Secret` and returns a plain `String`, so it is the +> single audited point at which upstream text becomes loggable. + +### `.specs/service/specs/04-http-api.md` → Middleware stack, item 1 (Modify) + +> 1. **Request ID** (`middleware/request_id.rs`) — reuse an inbound `X-Request-Id` only when it +> is a plausible correlation identifier: non-empty, at most `MAX_REQUEST_ID_LEN` (128) bytes, +> and drawn from `[A-Za-z0-9_-]`. Anything else — absent, empty, over-long, wrongly shaped, or +> not visible ASCII — is discarded and a fresh UUIDv4 is generated instead; the request is +> never failed over a malformed correlation header, and the rejected value is never logged. +> Open a per-request `info_span` carrying `request_id` so all downstream logs — including the +> `server_error` detail log — inherit it; echo in the response header. + +### `.specs/service/specs/04-http-api.md` → Error mapping (Modify) + +> `ApiError` wraps the domain `Error` (plus `UnsupportedGrantType`) and renders +> `{"error": , "error_description": }` (RFC 6749 §5.2). The status and `error` code +> are as tabulated above; the `error_description` is **always** `Error::client_description()` — a +> stable `&'static str` per variant, drawn from a small fixed set, that never embeds caller input, +> library error text, provider key state, or cache internals. The internal `reason`/`detail` an +> adapter composed is never published. (`UnsupportedGrantType`, a route-level error with no +> domain counterpart, keeps its fixed static description — generic by construction, so the same +> rule holds.) +> +> Every mapped domain error — not only the `server_error` class — logs its full `Display` via +> `tracing::error!` (5xx) or `tracing::warn!` (4xx) inside the request span, so the log carries +> the request id and the operator loses no diagnostic power. A debug assertion checks that the +> rendered description equals `err.client_description()` for every arm, generalising the guard +> that previously protected only `server_error`. +> +> The consequence for a caller is that an unknown `kid`, a bad signature, an expired token, and a +> wrong audience are indistinguishable at `/token`: each is +> `400 {"error":"invalid_grant","error_description":"the provided grant could not be validated"}`. +> RFC 6749 §5.2 makes `error_description` optional and developer-facing, so genericising it +> breaks no conformance. + +### `.specs/service/specs/05-provider-system.md` → OidcProvider behaviour (Modify) + +> - `exchange_code` delegates to `shared::token_endpoint::exchange_code` (form-encoded +> `authorization_code` POST with client credentials). A non-2xx upstream response yields a +> detail built by `shared::upstream::error_detail`, never the raw body. +> - `revoke_token` POSTs to the discovered revocation endpoint with the client id. A non-2xx +> response is read with `shared::http::read_bounded` and rendered through +> `shared::upstream::error_detail`, so an intermediary that echoes the submitted form cannot put +> the token being revoked into the error log. + +### `.specs/service/specs/05-provider-system.md` → Tiers, Tier 2 (Modify) + +> Apple is mostly OIDC but requires a freshly signed **ES256 client secret JWT** for each token +> endpoint call (`ClientSecretClaims { iss: team_id, sub: client_id, aud, iat, exp }`, ~5-minute +> lifetime, signed with the `.p8` key). `generate_client_secret` returns that assertion as +> `Secret`, so it can be posted but not formatted. `revoke_token` sends the assertion +> alongside the token being revoked and renders any non-2xx response through +> `shared::upstream::error_detail`. It reuses the shared `JwksCache` for the standard ID-token +> validation parts. + +### `.specs/service/specs/06-configuration.md` → Decisions (Modify) + +Replace the per-type redaction decision: + +> - *Secrets are unprintable by type.* **Credential-bearing config values are `Secret`, a +> newtype implementing neither `Debug` nor `Display`.** `WebhookConfig.secret`, +> `InternalApiConfig.shared_secret`, and `OidcProviderConfig.client_secret` previously relied on +> hand-written `Debug` impls rendering `""`; the newtype makes a leak a compile error +> rather than a per-type discipline — the enclosing structs' `Debug` impls still elide the +> secret field, but forgetting the elision now fails to compile instead of leaking. + +### `.specs/service/specs/06-configuration.md` → Sections → `[user_sync]` and `[internal_api]` (Modify) + +> ### `[user_sync]` +> `enabled` (bool), `adapter` (`webhook`), `[user_sync.webhook] { url, secret, timeout?, +> retries? }`. The `secret` is a `Secret` and cannot be formatted. +> +> ### `[internal_api]` +> `enabled` (false — internal routes are not mounted unless true, regardless of `server.role`; +> a `role = "admin"` instance with the flag off serves only `/health`), `auth_method` +> (`shared_secret`), `shared_secret` (a `Secret`; it cannot be formatted, must be +> non-empty when the internal API is served, and internal auth compares it in constant time via +> `subtle`). + +### `.specs/service/specs/07-telemetry-and-audit.md` → Telemetry hygiene (Add) + +Add after the `Telemetry (telemetry::init_telemetry)` section: + +> ## Telemetry hygiene +> +> Two rules bound what the observability plane may carry, and both are enforced by types rather +> than by convention. +> +> **Credential-derived values cannot be formatted.** `Secret` (`crates/core/src/secret.rs`) +> implements no `Debug`, no `Display`, and no `ToString`. `tracing` records a span field through +> one of those traits, so a `Secret` reaching `tracing::info!(?x)`, `%x`, `format!`, or +> `#[instrument]`'s default argument capture is a compile error. The values it wraps are the +> session refresh-token hash, the raw refresh token at issuance, the three configured secrets +> (`user_sync.webhook.secret`, `internal_api.shared_secret`, `providers..client_secret`), +> Apple's generated client assertion, and every upstream response body read at a provider +> boundary. `expose()` unwraps deliberately and is legible in review; the type prevents accident, +> not intent. +> +> **A client-facing description is a different value from an internal diagnostic.** +> `Error::client_description()` is the only string that crosses the public HTTP boundary; the full +> `Display` is logged under the request span. See +> [04-http-api.md](04-http-api.md) → Error mapping. +> +> Adapter instrumentation states its argument capture explicitly: every `#[instrument]` on a +> session-repository method names each argument in `skip(...)`, and re-projects only +> non-sensitive values into `fields(...)`. Declaring a bare field name (`fields(token_hash)`) keeps the log schema stable — +> the name appears, the value never does — but is treated as a schema aid, not as the control; +> the control is the type. + +### `.specs/service/specs/08-persistence.md` → Session-only stores (Modify) + +> - **LMDB (`adapters/lmdb`)** — embedded `heed` store with two named databases, `sessions` +> (hash → session) and `user_sessions` (user → set of hashes for revoke-all). Constructed with a +> path and a max map size in MB. +> - **Valkey/Redis (`adapters/valkey`)** — `fred` client; keys `{prefix}session:{hash}`, a +> `{prefix}user_sessions:{user_id}` set, and a `{prefix}active_sessions` counter. A session write +> applies the hash, its TTL, the user-set membership, an `INCR` of the counter, and a bump of the +> user set's own TTL to the greatest member expiry — atomically (single pipeline). The set-TTL +> bump uses `EXPIRE … GT` (only-extend), so a concurrent shorter-lived write can never shorten +> the set's life, and idle users' index sets expire on their own. A session whose `expires_at` is +> not in the future is rejected, so no TTL-less key is ever created. `count_active_sessions` +> reads the counter, which is maintained by `INCR` on store and `DECR` on explicit revoke; +> natural TTL expiry cannot decrement it, so it drifts upward between cleanups. +> `cleanup_expired_sessions` prunes `user_sessions` set members whose session key no longer +> exists, reconciles the counter by recomputing it from a SCAN of live `{prefix}session:*` keys, +> and returns the number of members pruned; session bodies themselves need no sweep. +> +> Both implement `SessionRepository` only and are selected via `[session_repository]`. +> +> Every session adapter instruments its three session methods identically: +> `#[instrument(skip(self, session), fields(user_id = %session.user_id))]` on the write path and +> `#[instrument(skip(self, token_hash), fields(token_hash))]` on the lookup and revoke paths. The +> token hash and the session's client provenance (`ip_address`, `user_agent`, `device_id`) never +> become span field values on any backend. + +--- + +## Type changes + +No canonical-schema change. `Secret` is transparent to `serde` — it serializes and +deserializes as the wrapped `T` — so `Session.refresh_token_hash` keeps its 64-character +lowercase-hex string form on every store, and its plain string shape (`NonEmptyString` in +[`canonical-types.schema.json`](../service/specs/canonical-types.schema.json), `string` in +`schemas/datamodel.schema.json`) is untouched. No stored record, wire body, or migration +changes. + +The new type, for reference: + +```rust +/// A value that must never reach a log line, a span field, or an error string. +/// +/// Implements neither `Debug` nor `Display`, so `tracing`'s value capture — including +/// `#[instrument]`'s default argument recording — cannot render it. `serde` support is +/// transparent, because persistence and the log stream are different trust domains. +#[derive(Clone, Serialize, Deserialize)] +#[serde(transparent)] +pub struct Secret(T); + +impl Secret { + pub fn new(value: T) -> Self { Self(value) } + pub fn expose(&self) -> &T { &self.0 } + pub fn into_inner(self) -> T { self.0 } +} +``` + +`PartialEq` is implemented only for `Secret`, in constant time via `subtle`, so a +comparison cannot become a timing oracle. + +### The values it wraps + +| Value | Where it lives today | Becomes | serde needed | +|---|---|---|---| +| `Session.refresh_token_hash` | `crates/core/src/domain/session.rs:8` (`String`) | `Secret` | yes — every store persists it | +| `SessionRepository` token-hash parameters | `crates/core/src/ports/repository.rs:26,27` (`&str`) | `&Secret` | no | +| the refresh token minted at issuance | `crates/core/src/service/exchange.rs:293` (`String`) | `Secret` | yes — one field of `TokenResponse` | +| `WebhookConfig.secret` | `crates/core/src/config.rs:325` (`String`) | `Secret` | deserialize only | +| `InternalApiConfig.shared_secret` | `crates/core/src/config.rs:391` (`Option`) | `Option>` | deserialize only | +| `OidcProviderConfig.client_secret` | `crates/core/src/domain/provider.rs:11` (`Option`) | `Option>` | deserialize only | +| `OidcProvider.client_secret` | `crates/adapters/src/oidc/mod.rs:18` (`Option`) | `Option>` | no | +| Apple's generated client assertion | `crates/providers/src/apple.rs:187` (returns `Result`) | `Result>` | no | +| an upstream response body at a provider boundary | `raw_body` / `body` locals at `token_endpoint.rs:39`, `oidc/mod.rs:253`, `apple.rs:348` | `Secret` from `shared::http::read_bounded` | no | + +Wrapping `Session.refresh_token_hash` removes `Session`'s `#[derive(Debug)]`; it gains a +hand-written `Debug` eliding that field. `TokenResponse` and `ProviderTokens` keep their existing +hand-written `Debug` impls until their remaining `String` fields are wrapped — out of scope here. + +--- + +## Implementation notes + +Order matters: steps 1 and 2 are independent of the type and should land first. + +1. **Ship the two span redactions immediately**, without waiting for `Secret`. At + `crates/adapters/src/lmdb/mod.rs:55` drop `token_hash` from the `fields(...)` list, keeping + `user_id`; at `:102` and `:140` use `#[instrument(skip(self, token_hash), fields(token_hash))]`. + At `crates/adapters/src/valkey/mod.rs:52` use + `#[instrument(skip(self, session), fields(user_id = %session.user_id))]`; at `:141` and `:211` + use `#[instrument(skip(self, token_hash), fields(token_hash))]`. Naming the argument in + `skip(...)` as well as `fields(...)` is deliberate: the sibling adapters' redaction + (`dynamo/mod.rs:653,671`, `postgres/mod.rs:522,536`, `sqlite/mod.rs:516,532`) relies on a name + collision that a rename defeats silently. +2. **Bound the request id.** In `crates/server/src/middleware/request_id.rs`, add + `MAX_REQUEST_ID_LEN: usize = 128` and an `is_acceptable_request_id(&str) -> bool` predicate + (non-empty, within the bound, `[A-Za-z0-9_-]` only), and replace the `.filter(|s| !s.is_empty())` + at `:31` with it. The predicate subsumes the emptiness filter, so the `assert!` at `:38-41` and + its regression test keep holding. Rejection stays silent — logging the rejected value would + reintroduce the unbounded field the bound exists to remove. +3. **Add the bounded, redacting upstream helper.** New `crates/adapters/src/shared/upstream.rs` + with `MAX_UPSTREAM_EXCERPT: usize = 256` and `error_detail(status, Secret) -> String`; + add `read_bounded` with `MAX_UPSTREAM_BODY_BYTES: usize = 65_536` to + `crates/adapters/src/shared/http.rs` (accumulate `response.bytes_stream()` to the ceiling rather + than an unbounded `text()`). The redactor percent-decodes before masking — an echoed form + returns `token=1%2F%2F…`, so a rule that matches the literal value passes while the leak + remains. Export both from `crates/adapters/src/shared/mod.rs`. +4. **Route the three sites through it.** `crates/adapters/src/shared/token_endpoint.rs:44-58` + (replace the `_ => raw_body` arm), `crates/adapters/src/oidc/mod.rs:251-258`, and + `crates/providers/src/apple.rs:346-353` (both of which have no classification arm at all today). + Keep the existing `exchange_code_surfaces_oauth_error_on_non_2xx` test passing — conformant + RFC 6749 error objects must still surface `error` and `error_description`. +5. **Split the error type.** Add `Error::client_description(&self) -> &'static str` in + `crates/core/src/error.rs`. In `crates/server/src/error.rs`, have `map_domain_error_inner` + return `err.client_description().to_string()` for every arm — including the 4xx arms at + `:82-96`, `:97-100` (`UnknownProvider`), `:120-124` (`NotFound`), and the `Conflict` arm — and + lift the log and the guard at `:56-75` out of the `if error_code == "server_error"` block so + they cover every class, logging 5xx at `error` and 4xx at `warn` — the `assert_ne!` against + the full `Display` generalising to the debug assertion that each arm's rendered description + equals `err.client_description()`. Two existing tests + codify the current leak and must be updated: + `invalid_grant_emits_no_server_error_detail_log` asserts + `description == "code already used"` (`crates/server/src/error.rs:334`), and the integration + test at `crates/server/tests/routes.rs:148` asserts the body's `error_description` + `contains("code")`. +6. **Introduce `Secret`** in `crates/core/src/secret.rs`, re-exported from `crates/core/src/lib.rs`. + Land the wraps one value at a time in the order of the `Type changes` table; the compiler + enumerates the call sites for each. `Session`'s `#[derive(Debug)]` at + `crates/core/src/domain/session.rs:4` is the first thing to break — that break is the point. +7. **Tests.** A `trybuild` compile-fail suite asserting that `tracing::info!(?secret)` and + `format!("{secret}")` do not compile is the proof that the control is structural. Add a leak + corpus that drives a store, a refresh, a revoke, and an upstream error through every + `SessionRepository` implementation under a capturing subscriber with `FmtSpan::CLOSE` enabled — + the stock subscriber's `FmtSpan::NONE` would otherwise let the assertion pass vacuously — and + asserts the captured output contains no sentinel digest, token, or secret, matching after + percent-decoding. Add request-id tests for a 64 KiB value, a wrongly-shaped-but-legal ASCII + value, exactly `MAX_REQUEST_ID_LEN` bytes and one byte more, and keep + `preserves_existing_request_id` passing so a fix cannot silently disable reuse. Add error-body + tests asserting an unknown `kid` is not echoed and that signature, `exp`, and `aud` failures are + indistinguishable to the caller. + +Evidence: sealed scan bundle `.security/oidc-exchange/53cbdec9_20260804T102454Z/`, findings +`g1-upstream-token-body-written-to-logs`, `g1-upstream-body-logged-oidc-revoke`, +`g1-upstream-body-logged-apple-revoke`, `g1-token-error-response-oracle`, +`g3-lmdb-token-hash-span-exposure`, `g3-valkey-session-span-exposure`, +`g1-request-id-unbounded-and-client-chosen`; structural context +`hardening/proposals/observability-contract.md` (Option 2); threat model +`artifacts/01_context/threat_model.md` (invariants I10, I15, I16). + +--- + +## Merge plan + +1. Apply each `Proposed changes` block to its canonical page; bump each page's `**Date:**`. +2. Add the new `Telemetry hygiene` section to 07-telemetry-and-audit.md. +3. Replace the superseded redacting-`Debug` decision in 06-configuration.md rather than appending + beside it. +4. No schema change to fold in. +5. Flip this file's `**Status:**` to `Merged`, stamp `**Merged:** YYYY-MM-DD`, and move it to + `.specs/changes/merged/`. +6. Update `.specs/README.md`'s Change specs table. + +--- + +## Assumptions and open questions + +### Assumptions + +- The shipped JSON subscriber (`crates/server/src/telemetry.rs:17-33`) escapes field values, so + none of these leaks is also a log-injection primitive; a plain-text subscriber would change that + conclusion, and nothing in the repository installs one. +- `ed25519_dalek::SigningKey`'s `Debug` deliberately omits the secret key + (`finish_non_exhaustive`), so `LocalKeyManager`'s derived `Debug` + (`crates/adapters/src/local_keys/mod.rs:9`) leaks nothing today. That is an upstream discipline + this repository depends on but does not control. +- No consumer depends on `/token`'s current `error_description` text. The strings are internal + diagnostics; RFC 6749 §5.2 marks the field optional and developer-facing. +- `serde` support on `Secret` is not itself a disclosure channel: `tracing` captures values + through `Debug`/`Display`, never through `Serialize`. + +### Decisions + +- *One generic type, not a family of newtypes.* **`Secret` wraps every credential-derived + value; there is no separate `TokenHash` or `SessionRef`.** The property being enforced — + "cannot be formatted" — is identical for all of them, and distinct newtypes would multiply the + migration without adding a property the surrounding function signatures do not already give. +- *`Secret` keeps `serde`, drops `Debug` and `Display`.* **Persistence and the log stream are + different trust domains.** Dropping `Serialize` would break every session store and the token + response for no gain, since `tracing` cannot reach a value through `serde`. +- *Two redactions ship ahead of the type.* **The LMDB and Valkey `#[instrument]` fixes land + immediately, before `Secret` exists.** They are one-line-scale changes with no design + dependency, and the type's migration is broad enough that waiting would leave a known leak open + across several releases. +- *A rejected request id yields a fresh one, never a failed request.* **An over-long or wrongly + shaped `X-Request-Id` is discarded and a UUIDv4 is generated.** The header is a correlation aid + with no protocol semantics; rejecting the request would convert an observability nicety into an + availability dependency on well-behaved clients, and 4xx-ing an otherwise valid `/token` call + over a diagnostic header is a worse outcome than losing one trace hop. +- *Rejection is silent.* **The rejected id is not logged, not even truncated.** Logging it + reintroduces the unbounded attacker-chosen log field the bound exists to remove. +- *Bodies are bounded at read, not at format.* **`read_bounded` caps the upstream body at 64 KiB + before anything can hold it.** A redactor alone still lets a hostile upstream choose how many + bytes the process buffers, and the cap also serves the bounded-body requirement the threat model + states. +- *`error_detail` is the only constructor.* **A `ProviderError` detail built from upstream bytes + can only be produced by `shared::upstream::error_detail`, which consumes a `Secret`.** + Fixing the three sites independently is what allowed the pattern to be copied twice already; a + single audited constructor gives a fourth copy nowhere to come from. +- *Every class is logged, not only `server_error`.* **4xx errors log their full `Display` at + `warn` under the request span.** Genericising the client body must not cost the operator the + diagnostic; the request id already correlates the generic body with the full detail. +- *`fields(token_hash)` is schema, not control.* **The bare field name stays for log-schema + stability, but the control is the type.** The name-collision behaviour it relies on is defeated + by a parameter rename, which the scan reproduced by accident. + +### Open questions + +- Should the correlation key be server-authored unconditionally, with any inbound id recorded in a + separately named `client_request_id` field? That closes the correlation-key-choice route + completely rather than bounding it, but it changes the documented echo semantics — an inbound id + would no longer come back on the response — and cross-fleet tracing depends on that echo. The + bound in this spec is needed either way. +- `Session.device_id`, `user_agent`, and `ip_address` are client-asserted strings with no length + bound, persisted on the session and recorded in audit events. Bounding them, and distinguishing + observed from asserted provenance, is a `ClientAddr`-shaped change that needs + `into_make_service_with_connect_info` at the serve call; it is not in this spec's scope. +- `TokenResponse.access_token` and `ProviderTokens`' three fields still rely on hand-written + `Debug` impls (`TokenResponse.refresh_token` is already wrapped by this spec). Wrapping them + in `Secret` would retire the last of the hand-rolled redactions, but touches the FFI + and binding surfaces; deferred. diff --git a/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/backlog/01-immediate_session_span_redactions.md b/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/backlog/01-immediate_session_span_redactions.md new file mode 100644 index 0000000..9fc5a9c --- /dev/null +++ b/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/backlog/01-immediate_session_span_redactions.md @@ -0,0 +1,22 @@ +# Task 01 — Immediate session span redactions + +**Status:** Backlog · **Plan:** [plan.md](../plan.md) · **Certificate:** forbidden and intentionally omitted + +**Implements:** [source spec §Implementation notes step 1](../../../changes/2026-08-05-eliminate_secret_leakage_in_logs_and_spans.md#implementation-notes); [`08-persistence.md` Session-only stores](../../../service/specs/08-persistence.md) +**Depends on:** — +**Produces:** LMDB and Valkey session methods explicitly skip sensitive arguments and record only the permitted schema fields before the broad `Secret` migration. +**Pointers:** `crates/adapters/src/lmdb/mod.rs:55,102,140`; `crates/adapters/src/valkey/mod.rs:52,141,211`; sibling patterns in Dynamo/Postgres/SQLite. + +## Steps + +- [ ] Change LMDB store instrumentation to skip `session` on writes and record only `%session.user_id`; skip `token_hash` on lookup/revoke while retaining an empty `token_hash` schema field. +- [ ] Make the equivalent explicit `skip(...)` changes in Valkey; do not record `Session`, refresh-token hash, IP address, user agent, or device ID. +- [ ] Audit all three methods on both backends for accidental default argument capture. +- [ ] Add focused tracing-capture regression tests with span-close events enabled that place sentinel hash/provenance values in sessions and prove none occurs in output while `user_id`/field schema remains observable. + +## Task-specific definition of done + +- [ ] LMDB and Valkey write, lookup, and revoke spans cannot render sentinels from the hash or provenance fields. +- [ ] Tests use `FmtSpan::CLOSE` (or equivalent explicit close-event capture), avoiding a vacuous no-span assertion. +- [ ] Existing session behavior and permitted `user_id` observability remain covered. +- [ ] No certificate file is created; test output is the completion evidence. diff --git a/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/backlog/02-bounded_request_ids.md b/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/backlog/02-bounded_request_ids.md new file mode 100644 index 0000000..2062fbf --- /dev/null +++ b/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/backlog/02-bounded_request_ids.md @@ -0,0 +1,22 @@ +# Task 02 — Bound and validate request IDs + +**Status:** Backlog · **Plan:** [plan.md](../plan.md) · **Certificate:** forbidden and intentionally omitted + +**Implements:** [source spec §04-http-api — Middleware stack](../../../changes/2026-08-05-eliminate_secret_leakage_in_logs_and_spans.md#specsservicespecs04-http-apimd--middleware-stack-item-1-modify); [§Implementation notes step 2](../../../changes/2026-08-05-eliminate_secret_leakage_in_logs_and_spans.md#implementation-notes); [`04-http-api.md` Middleware stack](../../../service/specs/04-http-api.md) +**Depends on:** — +**Produces:** a named 128-byte request-ID limit and predicate that preserves acceptable correlation IDs and silently replaces all other inbound values with UUIDv4 IDs. +**Pointers:** `crates/server/src/middleware/request_id.rs`; its module tests and existing `preserves_existing_request_id` test. + +## Steps + +- [ ] Add `MAX_REQUEST_ID_LEN: usize = 128` and `is_acceptable_request_id(&str) -> bool` with explicit non-empty, length, and ASCII `[A-Za-z0-9_-]` checks. +- [ ] Use the predicate in the inbound-header path; do not log rejected values and do not turn malformed correlation metadata into a request failure. +- [ ] Keep request span recording and response-header echo behavior unchanged for accepted and generated IDs. +- [ ] Add boundary tests for exactly 128 bytes, 129 bytes, a 64 KiB input, a legal-ASCII but wrongly shaped value, and invalid characters; keep accepted-id reuse and absent/invalid generation tests. + +## Task-specific definition of done + +- [ ] Only plausible IDs are reused; every rejected input yields a valid generated UUIDv4 in both span/response behavior. +- [ ] The limit is named and tests cover below/at/above boundary behavior plus charset negative space. +- [ ] Rejection remains silent: capture tests show the inbound malformed value is not emitted. +- [ ] No certificate file is created; test output is the completion evidence. diff --git a/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/backlog/03-secret_type_and_core_migration.md b/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/backlog/03-secret_type_and_core_migration.md new file mode 100644 index 0000000..b585582 --- /dev/null +++ b/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/backlog/03-secret_type_and_core_migration.md @@ -0,0 +1,25 @@ +# Task 03 — Add `Secret` and migrate core contracts + +**Status:** Backlog · **Plan:** [plan.md](../plan.md) · **Certificate:** forbidden and intentionally omitted + +**Implements:** [source spec §Type changes](../../../changes/2026-08-05-eliminate_secret_leakage_in_logs_and_spans.md#type-changes); [§Implementation notes step 6](../../../changes/2026-08-05-eliminate_secret_leakage_in_logs_and_spans.md#implementation-notes); [`01-domain-model.md` Session](../../../service/specs/01-domain-model.md), [`02-ports-and-adapters.md` SessionRepository](../../../service/specs/02-ports-and-adapters.md), [`06-configuration.md`](../../../service/specs/06-configuration.md) +**Depends on:** — +**Produces:** core-owned serde-transparent `Secret` with deliberate exposure APIs, constant-time `Secret` equality, and compiler-enforced migration of the enumerated credential-derived core/config/session/repository values. +**Pointers:** `crates/core/src/lib.rs`, new `secret.rs`, `error.rs`, `config.rs`, `domain/session.rs`, `domain/token.rs`, `domain/provider.rs`, `ports/repository.rs`, `service/exchange.rs`; every SessionRepository adapter and test fixture. + +## Steps + +- [ ] Add and re-export `Secret` with `new`, `expose`, and `into_inner`; derive only `Clone`, `Serialize`, and `Deserialize` with `#[serde(transparent)]`, never `Debug`/`Display`/generic `PartialEq`. +- [ ] Add `subtle` to core and implement constant-time `PartialEq` only for `Secret`; cover equal and unequal values without exposing values in assertions/logging. +- [ ] Convert `Session.refresh_token_hash`, refresh-token issuance/`TokenResponse.refresh_token`, `WebhookConfig.secret`, `InternalApiConfig.shared_secret`, and `OidcProviderConfig.client_secret`; preserve serde storage/wire shapes and existing redacting enclosing `Debug` behavior. +- [ ] Replace `Session` derived `Debug` with a manual implementation that redacts only `refresh_token_hash` and preserves the non-sensitive fields. +- [ ] Change `SessionRepository` lookup/revoke signatures to `&Secret` and migrate Dynamo, LMDB, Postgres, SQLite, Valkey, mocks, callers, store-key construction, and tests to use `expose()` only at deliberate storage/constant-time comparison boundaries. +- [ ] Convert adapter OIDC `client_secret` storage to `Option>`; leave Apple assertion conversion to task 05, which owns its producer and provider call sites. + +## Task-specific definition of done + +- [ ] Every value in the source table except upstream response bodies and Apple’s generated assertion is `Secret`/`Option>` at the stated boundary. +- [ ] Session JSON round trips and token response serialization remain string-identical; no schema or migration changes occur. +- [ ] All repository implementations compile under `&Secret` and their instrumentation uses explicit skips. +- [ ] `Session` debug output contains `` and excludes a hash sentinel. +- [ ] No certificate file is created; test output and compile checks are the completion evidence. diff --git a/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/backlog/04-bounded_redacting_upstream_helper.md b/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/backlog/04-bounded_redacting_upstream_helper.md new file mode 100644 index 0000000..be3e7cd --- /dev/null +++ b/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/backlog/04-bounded_redacting_upstream_helper.md @@ -0,0 +1,23 @@ +# Task 04 — Add bounded, redacting upstream-error helper + +**Status:** Backlog · **Plan:** [plan.md](../plan.md) · **Certificate:** forbidden and intentionally omitted + +**Implements:** [source spec §02-ports-and-adapters — Shared OIDC utilities](../../../changes/2026-08-05-eliminate_secret_leakage_in_logs_and_spans.md#specsservicespecs02-ports-and-adaptersmd--shared-oidc-utilities-modify); [§Implementation notes step 3](../../../changes/2026-08-05-eliminate_secret_leakage_in_logs_and_spans.md#implementation-notes); [`02-ports-and-adapters.md` Shared OIDC utilities](../../../service/specs/02-ports-and-adapters.md) +**Depends on:** 03 +**Produces:** shared bounded HTTP body read returning `Secret` and a single `upstream::error_detail(status, body)` constructor that produces bounded, percent-decoded redacted diagnostics. +**Pointers:** `crates/adapters/src/shared/http.rs`, new `shared/upstream.rs`, `shared/mod.rs`, `shared/token_endpoint.rs`; adapter test utilities/wiremock fixtures. + +## Steps + +- [ ] Add `MAX_UPSTREAM_BODY_BYTES: usize = 65_536` and stream a response body to that ceiling rather than calling unbounded `text()`; return `Secret` and define explicit behavior for read/truncation failure. +- [ ] Add `MAX_UPSTREAM_EXCERPT: usize = 256` and `upstream::error_detail(StatusCode, Secret) -> String` as the only shared upstream-body-to-detail path. +- [ ] Prefer RFC 6749 JSON `error` and optional `error_description`; otherwise produce status, original byte length, and only a bounded redacted excerpt. +- [ ] Percent-decode before masking form keys `token`, `refresh_token`, `client_secret`, and `code`, and redact bare compact JWS values; ensure malformed encoding cannot bypass masking or panic. +- [ ] Export the helper and reader from `shared/mod.rs`; add focused unit/wiremock tests for structured OAuth preservation, encoded echoed form values, JWS, malformed input, oversized bodies, and excerpt boundaries. + +## Task-specific definition of done + +- [ ] No upstream body is fully buffered beyond 64 KiB, and no function returns a format-capable raw body before the reviewed conversion point. +- [ ] Every fallback diagnostic is bounded to 256 characters after redaction and contains no supplied secret sentinels, including percent-encoded forms. +- [ ] Conformant OAuth `error`/`error_description` behavior remains available for task 05’s existing token-endpoint regression. +- [ ] No certificate file is created; test output is the completion evidence. diff --git a/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/backlog/05-provider_boundary_adoption.md b/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/backlog/05-provider_boundary_adoption.md new file mode 100644 index 0000000..12dc6b4 --- /dev/null +++ b/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/backlog/05-provider_boundary_adoption.md @@ -0,0 +1,24 @@ +# Task 05 — Route provider boundaries through the safe helper + +**Status:** Backlog · **Plan:** [plan.md](../plan.md) · **Certificate:** forbidden and intentionally omitted + +**Implements:** [source spec §05-provider-system](../../../changes/2026-08-05-eliminate_secret_leakage_in_logs_and_spans.md#specsservicespecs05-provider-systemmd--oidcprovider-behaviour-modify); [§Implementation notes step 4](../../../changes/2026-08-05-eliminate_secret_leakage_in_logs_and_spans.md#implementation-notes); [`05-provider-system.md`](../../../service/specs/05-provider-system.md) +**Depends on:** 03, 04 +**Produces:** all three upstream non-2xx paths use `read_bounded` and `upstream::error_detail`; OIDC credentials and Apple-generated assertions remain unprintable through request construction. +**Pointers:** `crates/adapters/src/shared/token_endpoint.rs`; `crates/adapters/src/oidc/mod.rs`; `crates/providers/src/apple.rs`; their existing token-exchange/revocation tests. + +## Steps + +- [ ] Replace token-endpoint direct `response.text()`/raw-body fallback with the bounded reader and shared error-detail constructor, preserving its conformant OAuth-error test. +- [ ] Convert OIDC revocation’s non-2xx body handling to the same pair; verify an upstream echo of the revoked token cannot appear in the returned internal detail/log path. +- [ ] Change Apple `generate_client_secret` to return `Secret`, expose only to build the outbound form, and route Apple revocation non-2xx handling through the shared pair. +- [ ] Ensure configured OIDC `Option>` is exposed only when building outbound forms and cannot be captured by traces/errors. +- [ ] Add provider-boundary tests for raw and percent-encoded echoed token/client assertion/client secret/code values, structured OAuth errors, oversize response behavior, and existing success/revocation contracts. + +## Task-specific definition of done + +- [ ] No `response.text()` remains on these three non-2xx paths, and no raw upstream body reaches `ProviderError.detail`. +- [ ] The existing `exchange_code_surfaces_oauth_error_on_non_2xx` regression remains true for safe structured content. +- [ ] OIDC and Apple revoke failures redact submitted token and credential sentinels, including encoded echo cases. +- [ ] Apple assertion generation and all outbound forms compile with non-formatting types. +- [ ] No certificate file is created; test output is the completion evidence. diff --git a/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/backlog/06-generic_client_descriptions_and_logging.md b/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/backlog/06-generic_client_descriptions_and_logging.md new file mode 100644 index 0000000..eca0370 --- /dev/null +++ b/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/backlog/06-generic_client_descriptions_and_logging.md @@ -0,0 +1,23 @@ +# Task 06 — Separate public descriptions from internal diagnostics + +**Status:** Backlog · **Plan:** [plan.md](../plan.md) · **Certificate:** forbidden and intentionally omitted + +**Implements:** [source spec §04-http-api — Error mapping](../../../changes/2026-08-05-eliminate_secret_leakage_in_logs_and_spans.md#specsservicespecs04-http-apimd--error-mapping-modify); [§Implementation notes step 5](../../../changes/2026-08-05-eliminate_secret_leakage_in_logs_and_spans.md#implementation-notes); [`04-http-api.md` Error mapping](../../../service/specs/04-http-api.md) +**Depends on:** 03 +**Produces:** `Error::client_description()` supplies stable static public text for every domain variant; server mapping logs full internal `Display` for all mapped classes, under the request span, without publishing diagnostics. +**Pointers:** `crates/core/src/error.rs`; `crates/server/src/error.rs`; `crates/server/tests/routes.rs:148`; existing `invalid_grant_emits_no_server_error_detail_log` test. + +## Steps + +- [ ] Add exhaustive `Error::client_description(&self) -> &'static str` mapping with a small fixed description set that embeds no caller input, library text, key state, or cache internals. +- [ ] Refactor `map_domain_error_inner` so every domain-error arm emits `client_description()` while retaining the specified status/error code and the static `UnsupportedGrantType` description. +- [ ] Move internal diagnostic logging out of the current `server_error`-only branch: log 5xx at `error!`, 4xx at `warn!`, within the existing request span. +- [ ] Add/debug-assert the mapping invariant that returned text equals `client_description()` for every arm; remove assumptions that public text differs only for server errors. +- [ ] Update leaking expectations and add tests proving unknown `kid` is not echoed and bad signature/expired/wrong-audience grants are indistinguishable in response body while their internal details remain logged with request ID. + +## Task-specific definition of done + +- [ ] Every mapped domain error returns only a static client description and no internal `reason`/`detail` crosses `/token`. +- [ ] All mapped 4xx and 5xx errors produce the correctly leveled operator event under the request span. +- [ ] Existing tests that codified leaked `code already used`/`contains("code")` behavior are replaced with generic-description expectations. +- [ ] No certificate file is created; test output is the completion evidence. diff --git a/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/backlog/07-structural_and_runtime_leak_regression_suite.md b/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/backlog/07-structural_and_runtime_leak_regression_suite.md new file mode 100644 index 0000000..cc787a9 --- /dev/null +++ b/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/backlog/07-structural_and_runtime_leak_regression_suite.md @@ -0,0 +1,24 @@ +# Task 07 — Prove structural and runtime non-leakage + +**Status:** Backlog · **Plan:** [plan.md](../plan.md) · **Certificate:** forbidden and intentionally omitted + +**Implements:** [source spec §Implementation notes step 7](../../../changes/2026-08-05-eliminate_secret_leakage_in_logs_and_spans.md#implementation-notes); [`07-telemetry-and-audit.md` Telemetry hygiene](../../../service/specs/07-telemetry-and-audit.md) +**Depends on:** 01, 02, 05, 06 +**Produces:** compile-fail proof that `Secret` cannot be formatted and a cross-boundary capture corpus proving sensitive sentinels never reach logs, span fields, or public error bodies. +**Pointers:** core/adapters/providers/server test manifests; session repository implementations; request-ID module tests; provider wiremock tests; server error/routes tests. + +## Steps + +- [ ] Add `trybuild` as a core dev-dependency and UI cases that fail for `tracing::info!(?secret)`, `%secret`, `format!("{secret}")`, and default `#[instrument]` argument capture; commit expected compiler diagnostics as test fixtures. +- [ ] Add an explicit capturing subscriber with span-close emission and percent-decoded matching helpers; drive store, refresh, revoke, and upstream-error paths using distinct hash/token/config/assertion sentinels. +- [ ] Exercise every `SessionRepository` backend (Dynamo, LMDB, Postgres, SQLite, Valkey, and mocks where applicable) and assert neither event nor close-span output contains secret/hash/provenance sentinels; assert permitted field names remain schema-compatible. +- [ ] Consolidate request-ID boundary/leak assertions and public error-oracle checks from tasks 02 and 06 into end-to-end request paths. +- [ ] Run and record actual Rust quality gates: `cargo fmt --all --check`, `cargo clippy --workspace -- -D warnings`, and `cargo nextest run --workspace`; fix failures before task completion. + +## Task-specific definition of done + +- [ ] Compile-fail tests prove the type-system control rather than merely redaction convention. +- [ ] Runtime corpus covers all required stores plus refresh/revoke/upstream error paths and detects plain or percent-encoded sentinels with close spans enabled. +- [ ] Request-ID limits and generic error bodies are exercised on an HTTP path, including positive and negative boundary cases. +- [ ] Full Rust gates pass with actual output captured in the PR/task discussion, not a done certificate. +- [ ] No certificate file is created; test output is the completion evidence. diff --git a/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/backlog/08-canonical_specification_synchronization.md b/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/backlog/08-canonical_specification_synchronization.md new file mode 100644 index 0000000..578e44f --- /dev/null +++ b/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/backlog/08-canonical_specification_synchronization.md @@ -0,0 +1,23 @@ +# Task 08 — Synchronize canonical specifications + +**Status:** Backlog · **Plan:** [plan.md](../plan.md) · **Certificate:** forbidden and intentionally omitted + +**Implements:** [source spec §Merge plan steps 1–4](../../../changes/2026-08-05-eliminate_secret_leakage_in_logs_and_spans.md#merge-plan); canonical targets [`01-domain-model.md`](../../../service/specs/01-domain-model.md), [`02-ports-and-adapters.md`](../../../service/specs/02-ports-and-adapters.md), [`04-http-api.md`](../../../service/specs/04-http-api.md), [`05-provider-system.md`](../../../service/specs/05-provider-system.md), [`06-configuration.md`](../../../service/specs/06-configuration.md), [`07-telemetry-and-audit.md`](../../../service/specs/07-telemetry-and-audit.md), and [`08-persistence.md`](../../../service/specs/08-persistence.md) +**Depends on:** 07 +**Produces:** all affected canonical documentation precisely describes demonstrated behavior and records no schema change; the proposed source change remains for merge-process ownership. +**Pointers:** source spec proposed-change blocks and type-change rationale; `.specs/service/specs/`; `.specs/README.md` merge-owned change table. + +## Steps + +- [ ] Apply each source-spec canonical block after implementation is demonstrably complete; bump each affected canonical page date in the same change. +- [ ] Add `Telemetry hygiene` to 07 with type-enforced formatting, public-description separation, and explicit session-instrumentation rules. +- [ ] Replace—not append beside—the superseded per-type redacting-`Debug` decision in 06; update `[user_sync]` and `[internal_api]` wording for `Secret`. +- [ ] Document `Session`/repository signatures, request-ID bounds, provider error flow, Apple assertion type, and session-adapter span rules exactly as shipped. +- [ ] Confirm `canonical-types.schema.json` and `schemas/datamodel.schema.json` remain untouched because serde/wire/store shape is unchanged; validate all internal Markdown links. + +## Task-specific definition of done + +- [ ] Exactly the seven source-listed canonical pages are updated and their dates bumped; no unrelated canonical page changes are introduced. +- [ ] Canonical prose matches tests/code, including named bounds and the no-schema-change rationale. +- [ ] The source change remains `Proposed`; moving/stamping it and updating `.specs/README.md` are explicitly left to the merge process. +- [ ] No certificate file is created; review plus link validation is the completion evidence. diff --git a/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/plan.md b/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/plan.md new file mode 100644 index 0000000..cbdd830 --- /dev/null +++ b/.specs/plans/2026-08-05-eliminate_secret_leakage_in_logs_and_spans/plan.md @@ -0,0 +1,85 @@ +# Plan: Eliminate secret leakage in logs, spans, and error responses + +**Status:** In Progress · **Layout:** kanban · **Date:** 2026-08-05 · **Owner:** Ant Stanley · **Source spec:** [`.specs/changes/2026-08-05-eliminate_secret_leakage_in_logs_and_spans.md`](../../changes/2026-08-05-eliminate_secret_leakage_in_logs_and_spans.md) + +This independent PR makes credential-derived values unprintable, closes the immediately known span leaks, bounds and redacts provider error bodies, genericizes public OAuth descriptions while retaining internal diagnostics, and bounds client-selected request IDs. It changes Rust only, across `core`, `adapters`, `providers`, and `server`; persisted and wire shapes remain unchanged. + +## Scope, siblings, and exclusions + +- **In scope:** all seven canonical pages named by the source spec; the enumerated `Secret` migration; the three upstream non-2xx call sites; all `SessionRepository` implementations; request-id and domain-error behavior; structural and runtime leak tests. +- **Sibling recorded, not absorbed:** the source spec names `2026-08-05-audit_and_throttle_authentication_failures.md`, but it is absent from this working copy. That sibling owns audit failure `reason` classification and authentication throttling. This plan neither creates it nor changes audit/throttling behavior. +- **No-certificate constraint:** no `*-certificate.md` artifact is required or allowed for this work. +- **Explicitly deferred:** server-authored-only correlation keys; client provenance bounds/observed-address plumbing; wrapping `TokenResponse.access_token` and `ProviderTokens` fields; FFI/binding changes; plaintext-subscriber policy. +- **No schema/migration work:** `Secret` is serde-transparent; `canonical-types.schema.json`, `schemas/datamodel.schema.json`, stored session records, and wire bodies keep their current string shapes. +- **Merge bookkeeping is excluded from this independent PR:** moving the source spec, marking it Merged, and updating `.specs/README.md` are owned by the merge process after implementation, not a backlog task. + +## Baseline and global definition of done + +Tasks inherit [`.specs/development-guidelines.md` §Definition of done](../../development-guidelines.md#definition-of-done): tests exercise behavior and negative space; new/touched functions carry meaningful assertions; bounds are named constants; and Rust gates are clean (`cargo fmt --all --check`, `cargo clippy --workspace -- -D warnings`, `cargo nextest run --workspace`). Each task adds its own focused acceptance criteria. **Done certificates are forbidden for this plan and intentionally omitted; no `*-certificate.md` files may be created.** + +Current-code anchors: `Session` and `SessionRepository` are still `String`/`&str`; LMDB and Valkey instrument unsafe values; `token_endpoint`, OIDC revoke, and Apple revoke call unbounded `response.text()`; `request_id_layer` only rejects empty/malformed UTF-8 IDs; and server mapping publishes variant reason/detail for most 4xx errors. + +## Task graph + +```mermaid +graph TD + 01["01 · immediate session span redactions"] + 02["02 · bounded request IDs"] + 03["03 · Secret type and core migration"] + 04["04 · bounded redacting upstream helper"] + 05["05 · provider boundary adoption"] + 06["06 · generic client descriptions and complete logging"] + 07["07 · structural and runtime leak regression suite"] + 08["08 · canonical specification synchronization"] + 03 --> 04 + 03 --> 05 + 04 --> 05 + 03 --> 06 + 01 --> 07 + 02 --> 07 + 05 --> 07 + 06 --> 07 + 07 --> 08 +``` + +The table is authoritative. Every dependency references a lower-numbered task, so the graph is acyclic, and the order matches the implementation notes below. + +| Task | Depends on | Edge kind | Produces | +|---|---|---|---| +| 01 · immediate session span redactions | — | — | LMDB and Valkey exclude session hashes and provenance from recorded values now, without waiting for `Secret` | +| 02 · bounded request IDs | — | — | inbound IDs are reused only when non-empty, ASCII `[A-Za-z0-9_-]`, and at most 128 bytes; invalid values silently receive generated UUIDv4 IDs | +| 03 · Secret type and core migration | — | — | serde-transparent, non-formatting `Secret`; constant-time `Secret` equality; all enumerated core/config/session/repository values typed and migrated | +| 04 · bounded redacting upstream helper | 03 | type, build | bounded provider-body reader returns `Secret` and one audited `upstream::error_detail` redacts before producing a loggable detail | +| 05 · provider boundary adoption | 03, 04 | type, contract | token exchange and OIDC/Apple revocation consume bounded secret bodies and use the one redacting error-detail constructor | +| 06 · generic client descriptions and complete logging | 03 | type, contract | every domain error has a stable static public description; every mapped error logs full internal diagnostics under its request span | +| 07 · structural and runtime leak regression suite | 01, 02, 05, 06 | verification | compile-fail formatting proof plus cross-store/span/provider/error/request-id leak corpus proves the controls and boundary cases | +| 08 · canonical specification synchronization | 07 | review, documentation | all seven listed canonical pages match shipped behavior; no schema change; source change remains Proposed for merge process | + +## Implementation order and milestones + +**Recommended order:** `01, 02, 03, 04, 05, 06, 07, 08`. Tasks 01 and 02 are independent, immediately risk-reducing slices. Task 03 establishes the type boundary required by later provider and error work. Tasks 04 and 06 can proceed in parallel after 03; task 05 joins the shared helper to the three provider surfaces. Task 07 proves all slices together; task 08 documents only demonstrated behavior. + +| Milestone | Tasks | Demonstrable outcome | +|---|---|---| +| M1 — immediate observability containment | 01, 02 | session spans no longer contain hash/provenance values and malformed/oversized IDs are silently replaced | +| M2 — structural secret boundary | 03, 04 | listed credentials cannot be formatted; upstream bodies are bounded before retention and redacted at one constructor | +| M3 — protected external boundaries | 05, 06 | all three provider error paths are safe; `/token` receives stable generic descriptions while operators retain request-correlated diagnostics | +| M4 — regression proof and canonical alignment | 07, 08 | compile/runtime leak tests and all seven canonical-page updates validate the independent PR end to end | + +## Validation checklist + +- [x] Source spec, canonical targets, guidelines, current code, existing completed plans, and relevant tests were read; the review verified links, coverage, DoDs, DAG/order, and the no-certificate constraint. +- [x] Each source-spec implementation note and each enumerated wrapped value is covered by one or more backlog tasks. +- [x] Every backlog task is indexed, in `backlog/`, has a source/canonical back-reference, dependencies, steps, task-specific DoD, and test plan. +- [x] Dependencies are lower-numbered and acyclic; Mermaid matches the authoritative table. +- [x] All links are relative and resolve within this checkout; no sibling scope is absorbed. +- [x] Status is Planned; all task statuses are Backlog; `in-progress/`, `blocked/`, and `done/` are intentionally empty. +- [x] Done certificates are forbidden and documented as omitted. + +## Assumptions and decisions + +- The source spec’s design decisions are settled: `Secret` keeps serde but implements neither `Debug` nor `Display`; `Secret` comparison uses `subtle`; provider bodies cap at 64 KiB; excerpts cap at 256 characters; request IDs cap at 128 bytes; rejected IDs are never logged. +- `trybuild` is not currently a declared workspace dependency, so task 07 explicitly adds it as a core dev-dependency rather than assuming it exists. +- Existing store adapters with bare `fields(token_hash)` are migrated under the repository signature change; explicit `skip(self, token_hash)` is required so a future rename cannot reveal the value. +- The review found no dangling backlinks or canonical-page coverage gaps in the plan/index set. +- No done certificate is a deliverable or acceptable substitute for test evidence in this plan.