Close out review findings: path traversal, redirects, secrets, timeouts, DB reconnect, and doc accuracy

[michielbdejong]

Summary

Works through the 20 open review-finding issues (#48–#66, #72). All P1 and P2 issues with a real code or doc fix are addressed below; #59 and #72 are closed as already resolved on main (no code change needed — see notes).

P1 (security)

• [P1] Normalize proxy paths before checking the catalog allowlist #48 — Reject ./.. path segments before catalog validation, so a request like /repositories/../issues can't pass template matching and then normalize to a different upstream path than the one authorized.
• [P1] Prevent upstream redirects from escaping catalog validation #49 — Disable automatic redirects on the shared HTTP client (reqwest::redirect::Policy::none()), matching the pattern already used for the identity client, so a redirect can't send a request to a destination the catalog never validated.
• [P1] Reject empty session and server secrets #50 — Reject empty required config values (SERVER_SECRET, DATABASE_URL, ENCRYPTION_KEY, the APP_AUTH_* values) instead of silently accepting them; an empty SESSION_SECRET is now treated as unset (triggering random key generation) instead of being hashed into a reproducible, guessable key.
• [P1] Enforce upstream timeouts and response limits while reading #51 — Add connect/read timeouts to the shared HTTP client, and read the upstream response body incrementally (Response::chunk()), rejecting as soon as the 10 MiB limit is crossed instead of buffering the whole body first.

P2 (code)

• [P2] Implement or explicitly bound OpenAPI request validation #52 — Added Catalog::validate_request: a bounded, explicit check that a declared required query parameter is present, an enum-constrained parameter's value is one of the declared values, and a request body's presence/content-type match the operation's declared requestBody. Documented as an explicit bounded subset in SECURITY.md (not full JSON Schema validation).
• [P2] Recover PostgreSQL connectivity after a disconnect #53 — Security now holds its PostgreSQL client behind Arc<RwLock<Arc<Client>>> with a background supervisor that reconnects with exponential backoff and swaps the client in automatically. Added Security::is_ready() and a /healthz endpoint. Covered by an ignored test that kills the backend via pg_terminate_backend and confirms the connection self-heals.
• [P2] Clean up expired OAuth states and connection credentials #54 — oauth_states and connection_codes now get the same opportunistic DELETE ... WHERE expires_at <= NOW() cleanup already used for used_challenges/connection_handoffs, plus supporting indexes.
• [P2] Preserve the pagination and ETag headers advertised to browser clients #55 — Forward ETag, X-Total-Count, and X-Next-Page from the upstream response, matching what CORS already exposes to browser clients.
• [P2] Add end-to-end coverage of forwarding and successful token exchange and refresh #56 — Added an end-to-end test of forward() through the real router: an expired credential is refreshed at a mocked token endpoint, the refreshed token is used against a mocked provider API, the response (with its ETag/Link headers) is forwarded, and the one-time connection code is rotated and verified.
• [P2] Test OAuth profiles against the application default catalog revision #57 — Added a test that loads the catalog from the application's actual default CATALOG_PATH pin (now a shared config::DEFAULT_CATALOG_PATH constant) and asserts the OAuth profiles it supplies; the existing test against a separately-pinned revision is now clearly labeled as such.

P2 (docs)

• [P2] Correct README claims that the service needs no database or shared state #58 — Fixed the contradictory "no database" framing (the very next sentences already said PostgreSQL is required).
• [P2] Remove the obsolete claim that token storage and forwarding are unimplemented #59 — Already resolved on main; the "token storage/forwarding unimplemented" claim is gone. Closing with no code change.
• [P2] Distinguish browser handoff codes from legacy proxy credentials in setup docs #60, [P2] Remove the obsolete GitHub pagination rewriting claim #61 — Rewrote the connection-code paragraph to distinguish the legacy direct connection_code flow from the browser bootstrap flow's handoff-then-redeem, and removed the false claim that GitHub pagination links get rewritten (no such code exists; Link is forwarded verbatim).
• [P2] Make the documented .env startup procedure actually load environment variables #62 — Documented that .env isn't auto-loaded; added an explicit source/set -a step.
• [P2] Include required database and encryption settings in .env.example #63 — Added DATABASE_URL/ENCRYPTION_KEY (with a generation hint) to .env.example.
• [P2] Set the public Spotify client authentication method in the configuration example #64 — Added OAUTH_SPOTIFY_CLIENT_AUTH_METHOD=none to .env.example and the README's Spotify section.
• [P2] Separate implemented security behavior from future requirements and correct credential expiry #65 — Corrected SECURITY.md's stale "five-minute" proxy-credential expiry to ten minutes (matching store_connection_code), keeping the handoff's separate five-minute lifetime explicit.
• [P2] Remove hardcoded Atomic Data Hub wording from generic connection consent #66 — Replaced hardcoded "Atomic Data Hub" consent copy with generic "Destination:" wording (the actual destination is already shown), and dropped the obsolete two-provider description now that every platform is catalog-driven.
• [P2] Add Notion as a catalog platform (via localthought/overlays) #72 — Notion is already registered in the application's default catalog pin (added before this issue was filed) with a working auth overlay and existing fixture tests; the new default_catalog_pin_... test ([P2] Test OAuth profiles against the application default catalog revision #57 above) further confirms oauth_provider("notion") resolves against the live default. No integration-proxy code change is required per the issue's own hand-off framing. Closing with no code change.

Also fixes 3 pre-existing clippy::nonminimal_bool findings (identity.rs, providers.rs) surfaced by the current toolchain — needed to keep cargo clippy --all-targets --all-features -- -D warnings green for this PR's CI run.

Test plan

• cargo fmt --all -- --check
• cargo clippy --all-targets --all-features -- -D warnings
• cargo test (91 passed)
• cargo test -- --include-ignored against a local PostgreSQL instance and live network (111 passed), including the new reconnect, e2e forwarding, and default-catalog-pin tests
• CI will additionally run the ignored PostgreSQL tests automatically

Fixes #48, #49, #50, #51, #52, #53, #54, #55, #56, #57, #58, #59, #60, #61, #62, #63, #64, #65, #66, #72

🤖 Generated with Claude Code

https://claude.ai/code/session_013UCidwbyQwtBvFwHyreivf

---

Generated by Claude Code