fix(api): redact ingest bearer token for non-canonical path prefixes - #2385
Open
detail-app[bot] wants to merge 1 commit into
Open
Conversation
## Summary `redact_sensitive_path` is the single chokepoint that strips the live ingest bearer token (`aios_evt_…`) out of structured logs — invoked from `RequestLoggingMiddleware` (the `api.request` line on every HTTP request) and the FastAPI exception handlers in `errors.py` (`api.error` / `api.http_error` / `api.validation_error`). Its regex `^(/v1/triggers/ingest/)[^/]+` was anchored to exactly one leading slash and a single separator before the token, so any ingest-shaped path that differed from the canonical prefix passed through unchanged — including the live token. uvicorn does not normalize `scope["path"]`, so a request like `//v1/triggers/ingest/aios_evt_<token>` (a trailing-slash base-URL join — produced verbatim on the wire by `requests`, `httpx`, `urllib`) or `/v1/triggers/ingest//<token>` (an extra separator before the token) was logged verbatim by BOTH `api.request` and the unmatched-route 404 → `http_exception_handler` `api.http_error` line. A case-variant `/V1/...` also bypassed. The fix widens the prefix match to tolerate duplicated leading slashes (`^/*`), extra separators before the token (`ingest/+`), and case (`re.IGNORECASE`), while the replacement is now a stable literal (`/v1/triggers/ingest/<redacted>`) so the canonical single-slash redacted form is produced regardless of how many slashes the inbound path carried. ## Substrate state changes None — code-only change. ## Test plan Unit: new `test_ingest_token_redacted_regardless_of_prefix_slash_variation` pins `//v1/...`, `/v1/triggers/ingest//<token>`, both at once, and the `/V1/...` case variant to the redacted form; `test_other_paths_untouched` adds the bare trailing-slash (no token), the `ingestery` sibling, and double-slash non-ingest paths (no over-redaction). Integration: `test_noncanonical_ingest_path_redacted_through_raw_asgi` drives a raw ASGI scope (httpx's `TestClient` parses a leading `//` as a network-path reference and 404s at the transport without exercising the middleware, so `TestClient` cannot test this) through the real `RequestLoggingMiddleware` + `install_exception_handlers` and asserts the live token never reaches EITHER the `api.request` line OR the unmatched-route 404 → `api.http_error` line — the log surface the prior suite never exercised for ingest paths. Non-vacuity: reverting the regex to the buggy form fails 7 of the new cases (token leaks into both log surfaces, matching the report), while the canonical-path cases still pass. Full unit suite green: 5914 passed, 1 pre-existing unrelated skip. mypy (strict) and ruff check/format clean. Also ran a live uvicorn server on a free port and sent raw HTTP/1.1 request lines for all three variants over a raw socket — confirmed the live token is redacted in every structured log line end-to-end on the wire. ## Risk / rollback Revert the one-line regex + replacement; no migration, no schema, no config. The redactor remains a defense-in-depth boundary (the route's threat model bounds a leaked token to a single self-disabling trigger); this hardens it against un-normalized request paths, it does not change the canonical redacted output format. --------- Co-authored-by: Detail <noreply@detail.dev>
Contributor
Code reviewVerdict: pass No blocking issues found. The widened redaction match covers the reported non-canonical path variants while preserving unrelated paths, and both direct and raw-ASGI integration coverage exercise the affected request/error log surfaces. Validation performed on
Some repository CI checks were still queued/in progress when reviewed; no check was treated as green unless completed successfully. |
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Detail bug report: View on Detail
Summary
redact_sensitive_pathis the single chokepoint that strips the live ingest bearer token (aios_evt_…) out of structured logs — called fromRequestLoggingMiddleware(theapi.requestline on every HTTP request) and the FastAPI exception handlers inerrors.py(api.error/api.http_error/api.validation_error). Its regex^(/v1/triggers/ingest/)[^/]+was anchored to exactly one leading slash and a single separator before the token, so any ingest-shaped path that didn't begin exactly with/v1/triggers/ingest/passed through unchanged — including the live token.uvicorn does not normalize
scope["path"], and the repo ships no path-normalizing edge proxy, so a request line likePOST //v1/triggers/ingest/aios_evt_<token>(a trailing-slash base-URL join, emitted verbatim on the wire byrequests,httpx, andurllib) or/v1/triggers/ingest//<token>(an extra separator before the token) reached the app unmodified, failed to match the canonical route (404), and was logged verbatim by both the always-emittedapi.requestline and the unmatched-route 404 →http_exception_handlerapi.http_errorline. A case-variant/V1/...also bypassed. A live single-trigger credential was thus persisted in operator structured logs / SIEM.The fix widens the prefix match to tolerate duplicated leading slashes (
^/*), extra separators before the token (ingest/+), and case (re.IGNORECASE); the replacement is now a stable literal so the canonical single-slash redacted form/v1/triggers/ingest/<redacted>is produced regardless of how many slashes the inbound path carried.Substrate state changes
None — code-only change.
Test plan
tests/unit/test_log_redaction.py): newtest_ingest_token_redacted_regardless_of_prefix_slash_variationpins//v1/...,/v1/triggers/ingest//<token>, both at once, and the/V1/...case variant to the redacted form;test_other_paths_untouchedadds the bare trailing-slash (no token), theingesterysibling route, and double-slash non-ingest paths to guard against over-redaction.test_noncanonical_ingest_path_redacted_through_raw_asgidrives a raw ASGI scope (necessary because httpx'sTestClientparses a leading//as a network-path reference and 404s at the transport without ever exercising the middleware) through the real middleware + exception handlers, asserting the live token never reaches EITHER theapi.requestline OR the unmatched-route 404 →api.http_errorline — the log surface the prior suite never exercised for ingest paths (the existing matched-routeNotFoundError→api.errortest doesn't cover it).//v1/triggers/ingest/<token>,/v1/triggers/ingest//<token>, and the canonical path. Confirmed the live token is redacted in every structured log line (api.request,api.http_error, andapi.error) for all three variants — no leak on the wire.mypy, andruffcheck/format all clean; full unit suite green (5914 passed, 1 pre-existing unrelated skip). Confirmed the new raw-ASGI tests do not pollute thestructlog.testing.capture_logs-based middleware tests (they deliberately avoidconfigure_logging, which would freeze theaios.api.middlewarelogger proxy).Risk / rollback
Revert the one-line regex + replacement; no migration, schema, or config change. The redactor remains a defense-in-depth boundary (the route's threat model bounds a leaked token to a single self-disabling trigger, rotatable via
update_triggerre-mint) — this hardens it against un-normalized request paths and does not change the canonical redacted output format.Automatic Fixes PRs can be configured here.