fix: redact intake audit-log query strings + scrub the token from the phone URL - #45
Conversation
… phone URL (#43) The audit line logged the full request path, and the phone app's first load is GET /?token=<secret> — so the shared secret landed in plaintext in the console and the persistent rotating log, contradicting the line's own 'never the token' contract. audit_path() now drops the query string from the trail. The phone page additionally stashes the URL token in sessionStorage and history.replaceState()s the query away, so the phone browser's address bar/history/autocomplete never keep the secret; reload authenticates from the stored copy and the share-link UX is unchanged.
|
Automated review (Claude code-review sub-agent) Verdict: Approve (no CRITICAL/HIGH issues). Gate: pytest tests/adapters/test_http_intake.py tests/app/test_mobile_api.py tests/adapters/test_capture_intake.py -q -> 59 passed. ruff clean on touched files. Coverage of the leak (a/b): audit_path() is applied at the single logging call site in http_intake.py's _reply, the sole place self.path (with query) ever reaches a logger -- confirmed by grepping every logger.* call in src/grandplan; no other site logs a path/URL/query. capture_intake.py's logger.exception("capture handling failed") and diagnostics.py's crash hooks log tracebacks/messages only, not request paths, so no token can leak through them. Redaction is unconditional on the whole query string (not just token=), so it also covers any future query param, and fires on every route (404s, /api/pending/*, /capture), not just /. Residual token exposure (LOW, not a regression): gui.py:_print_phone_banner still print()s the full ?token=... share URL to stdout. This is separate from the logging module (per diagnostics.py, only logger.* calls hit the rotating file handler), so it does not land in the persistent log -- but it is deliberate, pre-existing, unchanged UX (the user has to learn the token somehow), correctly out of scope per the PR description. JS correctness (c): the sessionStorage/replaceState IIFE is correct -- setItem + replaceState share one try block, so if storage throws, replaceState never runs and the URL (with token) stays intact for a working reload, matching the documented fallback. No external request is introduced (still covered by the pre-existing "http://" not in html assertion). One UX corner case worth knowing (not a bug): after replaceState strips the query, "Add to Home Screen" would bookmark the token-less URL, and a fresh tab from that icon won't have the token in sessionStorage either -- first-open-only authenticates, later opens need a fresh share link. Probably acceptable given sessionStorage's intentional per-tab scope. Test rigor (d) -- be honest: test_audit_path_redacts_query_string is a real, non-vacuous pure-function test (concrete in/out pairs). test_web_app_scrubs_token_from_url_and_history, however, only asserts the substrings sessionStorage, history.replaceState, location.pathname appear in the HTML blob -- it does not execute the JS, so it would pass even if the calls were wired together incorrectly (e.g. wrong argument order, wrong branch). This matches the existing substring-check convention already used in this file for the embedded JS, so it's a pre-existing test-suite limitation, not something newly introduced -- but it does NOT pin runtime behavior. |
Fixes #43.
What leaked
Two copies of the
--serveshared secret, both visible in an ordinary session log:GET /?token=<secret>, so the token landed in plaintext in the console and the persistentrotating log, contradicting the audit line's own "never the token" comment.
?token=sitting in the address bar, so the phone browser'shistory/autocomplete kept the secret.
Fix
http_intake.audit_path()(pure, unit-tested): drops the query string from the audit trail —/?token=abclogs as/?[redacted]; query-free paths log unchanged.sessionStorage, thenhistory.replaceStates the queryaway. Reload authenticates from the stored copy; if storage is unavailable the URL is left
intact so reload still works. Share-link UX and the Bearer-header API auth model are unchanged.
Test plan
test_audit_path_redacts_query_string+test_web_app_scrubs_token_from_url_and_historywritten first (RED — import error / missing markers), pass after (GREEN).
approve a capture, confirm the log shows
/?[redacted].