feat(track-a): multi-provider passthrough routes + 4 security fixes

[PaoloC68]

Part 2 of 3 splitting PR #614.

Depends on PR #757 (httpx-sse dep) — merge order: #757 → #758 → #759.

Changes

• New passthrough routes: /openai/{path}, /gemini/{path}, /anthropic/{path}
• Migration 019: adds agent column to request_logs
• Mock OpenAI and Gemini servers for testing
• sqlite_e2e and mock_e2e tests for passthrough routes

Security Fixes (from PR #614 review)

1. Open proxy closed: /openai and /gemini require strict CLIENT_API_KEY match
2. Body size limit: Enforces MAX_REQUEST_PAYLOAD_BYTES on passthrough body
3. Lifespan-managed httpx clients: No resource leaks on shutdown
4. Hop-by-hop header stripping: transfer-encoding, set-cookie, server stripped from responses

Closes part of #614.

[claude]

Code Review — PR #758 (multi-provider passthrough routes)

Reviewed against CLAUDE.md conventions. Overall the security-fix story is clear and well-tested, but there are a few correctness bugs that I'd want addressed before merge.

🔴 Bugs

1. /anthropic/ alias double-prefixes /v1 against the real API — passthrough_routes.py:29

UPSTREAM_BASES = {
    "openai":    "https://api.openai.com",
    "gemini":    "https://generativelanguage.googleapis.com",
    "anthropic": "https://api.anthropic.com/v1",   # <-- has /v1
}

The rest of the codebase (gateway_routes.py:44 → ANTHROPIC_API_BASE = "https://api.anthropic.com") and the operator-facing setup page (static/client_setup.html:288 → "Check that ANTHROPIC_BASE_URL does not include /v1") treat the base URL as scheme+host only. With the current map, a production request to /anthropic/v1/messages is rewritten to https://api.anthropic.com/v1/v1/messages and fails 404.

The mock e2e tests don't catch it because they set ANTHROPIC_BASE_URL=http://localhost:{port} (no /v1) which takes precedence via _upstream_base. Fix: drop the /v1 suffix to match the rest of the codebase.

2. Streaming responses silently drop the upstream HTTP status — passthrough_routes.py:159-182

if streaming:
    async def stream_chunks():
        status = 200
        ...
        async with streaming_client.stream(...) as response:
            status = response.status_code      # captured here
            async for chunk in response.aiter_bytes():
                yield chunk
        ...
    return StreamingResponse(stream_chunks(), media_type="text/event-stream")

StreamingResponse commits HTTP 200 to the client as soon as it's returned — the inner generator hasn't run yet. So if the upstream answers 401/429/5xx to a streaming request, the client sees 200 OK with the JSON error body framed as SSE bytes. The recorder logs the true status (good) but the client experience is broken.

Standard fix: open the request, await headers, branch — return a regular Response(..., status_code=response.status_code) for non-2xx, only switch to StreamingResponse for 2xx. (You'll need to keep the upstream connection open across the boundary, e.g. by lifting streaming_client.stream(...) out of the generator and using BackgroundTask for cleanup, or by using client.send(request, stream=True).)

3. Hardcoded media_type="text/event-stream" is wrong for Gemini — passthrough_routes.py:182
Gemini's :streamGenerateContent returns a JSON array (application/json) unless the client passes ?alt=sse. Forcing text/event-stream mislabels the response and can break clients that branch on Content-Type. Prefer forwarding the upstream Content-Type (after hop-by-hop stripping) — same way the buffered branch does.

4. Empty outbound row inserted for every passthrough request — recorder.py:242-247
_handle_passthrough only calls record_inbound_* — it never touches the outbound side. But _write_logs() iterates (self._inbound, self._outbound) unconditionally, so every passthrough request writes a fully-NULL outbound row alongside the inbound row. Either skip empty pendings in _write_logs, or call record_outbound_request/record_outbound_response in the passthrough handler so the row carries real data.

5. Migration comments say "Migration 018" — all three 019_add_agent_to_request_logs.sql files (migrations/postgres/, migrations/sqlite/, src/luthien_proxy/utils/sqlite_migrations/) open with -- Migration 018: .... Just stale copy-paste; cosmetic but easy to fix.

🟡 Minor / design questions

• Request body never logged. _handle_passthrough passes body={} to record_inbound_request, so every inbound row in request_logs.request_body is {} regardless of what was sent. If that's intentional (privacy), worth a one-line comment; if not, it's a missing observability feature given that header capture is fairly detailed.
• request.state.luthien_* is dead state. The handler sets luthien_session_id / luthien_agent / luthien_model on request.state but nothing in the passthrough path reads from them (they're handed directly to the recorder). Either drop them or leave a comment explaining what other middleware consumes them.
• _upstream_base re-reads os.environ on every request. Not a real perf issue, but consider caching once at module import — also makes config easier to reason about.
• os.environ.get("OPENAI_API_KEY", "") falls back to empty string silently (passthrough_routes.py:100-106). If the key is unset, the request goes upstream with Authorization: Bearer and OpenAI returns 401. Given the strict CLIENT_API_KEY gate already enforced, consider failing fast at request time with a 503 when the server-side key is missing — matches the failure-mode story you've already adopted for verify_strict_client_key.

✅ What looks good

• The four security fixes called out in the PR body are real and targeted: strict CLIENT_API_KEY for OpenAI/Gemini (verify_strict_client_key), body-size limit, lifespan-managed httpx clients, hop-by-hop + dangerous response header stripping.
• verify_strict_client_key uses secrets.compare_digest and rejects when the env key is unset — good. Test coverage is thorough.
• Inbound x-luthien-* stripping before forwarding upstream is well-tested (test_luthien_headers_stripped_from_outbound).
• Header sanitization at storage time (sanitize_headers) handles Authorization, x-api-key, x-anthropic-api-key, x-goog-api-key.
• mock_openai/server.py and mock_gemini/server.py follow the existing mock_anthropic pattern (dedicated thread + event loop, FIFO queue, thread-safe capture) — easy to maintain.
• Sqlite + Postgres + in-source sqlite_migrations/ files are all in sync; matches the migrations guide.

📋 Test coverage

Strong for the auth, header stripping, and recorder wiring paths. Two gaps worth filling once bugs 1–3 are fixed:

• Streaming upstream error path (4xx/5xx from upstream while stream=True requested) — verify client sees the real status, not 200.
• A test against the default UPSTREAM_BASES["anthropic"] value (i.e., with ANTHROPIC_BASE_URL unset) to catch URL-construction regressions like bug chore(logging): replace prints with structured logging #1.

Happy to look again once the streaming-status / anthropic-base issues are addressed.

[claude]

Review — Track A multi-provider passthrough

Overall this is a well-scoped bridge PR with strong test coverage and the four called-out security fixes are real improvements. Below are the issues I'd want to see addressed (or explicitly deferred) before merge. I've grouped them by severity.

🔴 Should fix before merge

1. request.url.query and the API key in URL leak into request_logs — passthrough_routes.py:148 records url=str(request.url). For Gemini, callers commonly pass ?key=GOOGLE_API_KEY in the query string; for any provider, an OAuth-style access_token=… query param is also plausible. sanitize_headers() is applied but the URL is not, so secrets in the query string land in request_logs.url in plaintext. Either strip the query string before recording, or extend sanitize.py with a URL-scrubbing helper analogous to sanitize_url_for_logging (already present in utils/url.py).

2. Gemini caller-supplied ?key= collides with the server-injected x-goog-api-key — if a client passes both, both go upstream and the upstream's resolution is undefined. Easy fix in _handle_passthrough / _build_outbound_headers: strip key (and any auth-like params) from request.url.query for the Gemini provider before reconstructing upstream_url.

3. Resource leak on response.aread() failure in the streaming error path — passthrough_routes.py:182-198:

if response.status_code >= 300:
    error_body = await response.aread()      # ← may raise
    await upstream_cm.__aexit__(None, None, None)

If aread() raises (network glitch mid-error-body), __aexit__ is never called and the upstream connection leaks. Wrap the aread() + non-2xx return in a try/finally, or use the conventional pattern of installing the __aexit__ cleanup with an ExitStack/AsyncExitStack right after the successful __aenter__().

4. int(content_length) raises ValueError on a malformed header — passthrough_routes.py:123. A client sending Content-Length: abc currently turns into an unhandled 500. Either try/except around the int(...) or just rely on the post-read len(body) > MAX_REQUEST_PAYLOAD_BYTES check (the header check is best-effort anyway since the client can lie).

🟡 Worth addressing

5. Passthrough request/response bodies aren't recorded — recorder.record_inbound_request(..., body={}) on line 150 and record_inbound_response(status=…) with no body= mean request_logs for passthrough traffic has neither request nor response payloads. That's a meaningful debuggability regression vs. the /v1/messages path. If this is deliberate for the bridge (size? binary?), say so in a comment; if not, capture at least JSON bodies under a size cap (the recorder already has MAX_BODY_BYTES truncation).

6. repr(exc) in record_inbound_response(error=…) — passthrough_routes.py:178, 213, 230 log repr() of the httpx exception, which can include the full upstream URL (and thus the injected server API key in the Gemini case, since ?key=… ends up in the URL string before injection moves to headers). Prefer type(exc).__name__ plus a sanitized message, or run the message through the URL scrubber.

7. Duplicate tests in tests/luthien_proxy/unit_tests/test_passthrough_routes.py — test_body_size_limit_413, test_hop_by_hop_stripped, and test_essential_headers_preserved are each defined twice (module-level lines 213/229/245, and again inside TestBodySizeLimit / TestHopByHopHeaderStripping lines 328/356/375). They look like the module-level versions were superseded by the class-scoped ones during iteration but not removed. Delete the duplicates.

8. _upstream_base() reads env on every request — passthrough_routes.py:33-39. Not a correctness issue, but it's nicer to resolve provider bases once at startup (or read from Settings) so they appear in the config dashboard alongside other tunables. The same applies to OPENAI_API_KEY / GOOGLE_API_KEY — startup validation would catch a misconfigured deployment immediately instead of on first request.

9. /anthropic/{path} alias bypasses the policy engine entirely — this is acknowledged in the inline comment ("replaced by native pipeline in Track B"), but it's worth flagging more loudly that this path is a policy-free alternative to /v1/messages. If both paths are exposed on the same deployment, an authenticated client can pick whichever bypasses controls. At minimum, gate the /anthropic/* route behind a feature flag (off by default) and call this out in the changelog so operators know.

10. x-luthien-{session-id, agent, model} aren't bounded — these headers flow straight into request_logs with no length cap. A misbehaving client can write large blobs into the DB. A simple len(value) <= N truncation at the read site (passthrough_routes.py:136-138) is enough.

🟢 Nits

• passthrough_routes.py:97 — headers[k_lower] = v lowercases header names before sending upstream. Some upstreams are case-sensitive about specific values (rare but real, e.g. some signed-request middlewares). Forwarding the original casing is safer; the dedup behavior you get from lowercasing isn't relied on elsewhere.
• passthrough_routes.py:74-78 — _is_streaming parses the entire JSON body just to check stream. For a 10 MB JSON payload that's noticeable. A regex pre-check (b'\"stream\"\\s*:\\s*true') before the JSON parse would short-circuit the common case.
• tests/luthien_proxy/unit_tests/test_passthrough_routes.py:538-567 — the nested patch.dict(\"os.environ\", {}, clear=True) followed by re-deriving env from the now-empty os.environ is confusing and the outer patch is dead code. Collapse to a single patch.dict with the desired env.
• passthrough_routes.py:116 — defaulting user-agent to \"luthien-passthrough/0.1\" is a hardcoded version string that'll go stale. Reference PROXY_DISPLAY_VERSION instead.

✅ Things I liked

• Strict verify_strict_client_key for /openai and /gemini with timing-safe comparison and explicit "unset key disables the route" semantics — the threat-model docstring on passthrough_auth.py:73-86 is exactly the kind of context I want to see.
• Lifespan-managed httpx clients with ResourceWarning-as-error test coverage (test_lifespan_closes_httpx_clients_no_resource_warning).
• The streaming pre-read of upstream status before committing a 200 to the client (passthrough_routes.py:161-198) is the right pattern and worth the complexity.
• Hop-by-hop + dangerous-response-header stripping is correct and matches RFC 7230 §6.1.
• Mock servers for OpenAI and Gemini follow the established MockAnthropicServer pattern — easy to extend.
• Stripping x-luthien-* from outbound requests prevents internal-header leakage to third-party providers.

[claude]

Code Review

Solid PR — the security fixes called out in the description are well-implemented (lifespan-managed httpx clients, hop-by-hop stripping, strict CLIENT_API_KEY for OpenAI/Gemini, two-stage size check). Most of the issues below are around test correctness, defense-in-depth, and a couple of merge-artifact duplicates.

Bugs

1. test_openai_server_key_not_leaked_to_upstream is a false positive — tests/luthien_proxy/e2e_tests/sqlite/test_passthrough_routes.py:93

outbound_auth = captured.get("Authorization", "")
assert api_key not in outbound_auth, "Proxy key must not reach upstream"

The proxy normalizes outbound header names to lowercase (passthrough_routes.py:97 does headers[k_lower] = v). When aiohttp records dict(request.headers), keys come back lowercase. So captured.get("Authorization", "") returns "", making api_key not in "" trivially True. This security-critical assertion would pass even if CLIENT_API_KEY did leak upstream.

Fix: lower-case the lookup, or use a case-insensitive comparison:

outbound_auth = next((v for k, v in captured.items() if k.lower() == "authorization"), "")

2. Duplicate fixtures and tests in tests/luthien_proxy/unit_tests/test_passthrough_routes.py — Looks like a merge artifact:

• _policy_config_file / policy_config_file, _mock_db_pool / mock_db_pool, _mock_redis_client (the underscore copies are dead)
• test_body_size_limit_413, test_body_size_limit_normal_passes, test_hop_by_hop_stripped, test_essential_headers_preserved exist twice each — once at module level and once inside TestBodySizeLimit / TestHopByHopHeaderStripping. The class versions look more thoroughly patched; the module-level ones can go.

3. Comment typo — tests/luthien_proxy/e2e_tests/sqlite/test_request_logs_schema.py:12 says "agent missing — migration 018 not applied" but the migration is 019.

Security / defense in depth

4. Body-size limit can be bypassed via chunked transfer. _handle_passthrough (passthrough_routes.py:122-129) checks content-length first, but if the client omits it (chunked transfer), the second check fires only after await request.body() has already buffered the entire payload into memory. A malicious client can OOM the gateway. The existing /v1/messages path (anthropic_processor.py:501) has the same flaw, so this is at least consistent, but worth fixing in both places — read the body as a stream and abort once you cross the threshold.

5. Unfiltered header forwarding to third-party APIs. All inbound headers except _STRIP_INBOUND / _STRIP_AUTH / x-luthien-* are forwarded to OpenAI / Gemini / Anthropic. That means cookie, referer, origin, x-forwarded-for, and arbitrary client headers (including any other-tenant secrets the client may have set) get sent upstream. For server-to-server use this is usually fine; consider explicitly stripping cookie, referer, origin as defense-in-depth, or switching to an allowlist.

6. /anthropic/* alias bypasses the policy pipeline. This is the intended behavior of a passthrough, but it means an operator with a strict policy on /v1/messages would be surprised that /anthropic/v1/messages goes straight through with no judges / filters / rate-limit. Worth a short doc note (or a startup-warning log) so operators don't accidentally leave a policy-bypass route open. In PASSTHROUGH mode there is no auth either — i.e. the existing /v1/ semantics apply, which is fine, but it's the kind of thing that should be called out in the changelog.

Smaller issues

7. HTTP methods limited to GET/POST/PUT/DELETE/PATCH. No OPTIONS (CORS preflight will fail for browser clients) and no HEAD. If these passthroughs are meant for server-to-server only, fine; otherwise add them.

8. body={} passed to recorder for passthrough requests (passthrough_routes.py:150) — the actual request body is intentionally not logged, unlike the /v1/messages path (anthropic_processor.py:407) which logs dict(raw_http_request.body). Worth a one-line comment explaining why (likely: "non-JSON bodies / large payloads not worth serializing for passthrough").

9. Hardcoded user-agent version luthien-passthrough/0.1 (passthrough_routes.py:116) — will silently drift. Either drop the version suffix or pull from the package version.

10. Unused request.state.luthien_* writes (passthrough_routes.py:136-138) — these are set but never read elsewhere (the values are also passed directly to the recorder). Either remove the request.state writes or use them; right now they're dead bytes.

Things that look good

• Two-phase streaming handling (peek status before committing 200) is exactly right and well-commented.
• secrets.compare_digest for token comparison ✓
• Lifespan-managed httpx clients with explicit aclose() in finally — tested with ResourceWarning-as-error ✓
• x-goog-api-key added to sanitizer ✓
• Migration parity across postgres + sqlite ✓
• Test coverage for upstream-error streaming statuses is thorough.
• verify_strict_client_key correctly fails-closed when CLIENT_API_KEY is unset — well-documented threat model.

🤖 Generated with Claude Code

[claude]

Code Review — multi-provider passthrough routes

Solid security-conscious work. The strict-key gate on /openai and /gemini, hop-by-hop scrubbing, lifespan-managed httpx clients, body-size enforcement, and the streaming "peek status before committing" trick are all well thought through. Test coverage is unusually thorough for a feature PR. A few things worth addressing before merge.

Bugs

1. content-encoding is forwarded with already-decoded bodies (buffered path). src/luthien_proxy/passthrough_routes.py:233-242

httpx auto-decompresses response.content (and aiter_bytes). safe_headers only filters HOP_BY_HOP_HEADERS and DANGEROUS_RESPONSE_HEADERS, so an upstream gzip response forwards content-encoding: gzip together with decompressed bytes — the client will try to gunzip plain JSON and fail. Starlette overwrites content-length from the body, so that one is fine, but content-encoding (and content-language) leaks through.

Fix: add content-encoding, content-length to a "strip from buffered response" set (or to DANGEROUS_RESPONSE_HEADERS). Also worth doing the same in the streaming non-2xx branch at L185-189 which has the same shape.

2. Non-numeric content-length raises ValueError → 500. src/luthien_proxy/passthrough_routes.py:122-124

content_length = request.headers.get(\"content-length\")
if content_length and int(content_length) > MAX_REQUEST_PAYLOAD_BYTES:

A client sending content-length: abc crashes the handler. Wrap in try/except ValueError and either ignore or 400.

3. aread() failure on the non-2xx streaming branch leaks the upstream connection. src/luthien_proxy/passthrough_routes.py:178-194

If await response.aread() raises, upstream_cm.__aexit__ is never called. Move the __aexit__ into a try/finally (or use try: ... finally: await upstream_cm.__aexit__(None, None, None)).

4. Comment/code mismatch on shutdown ordering. src/luthien_proxy/main.py:399-408

The block comment says "Webhook sender goes first" but the new code aclose()s the passthrough httpx clients before await _webhook_sender.stop(). Either the comment is now wrong, or the ordering is. Since the passthrough clients are independent of the webhook sender either order works, but the misleading comment is a footgun for future edits.

Code quality

5. SQLite migration is not idempotent. migrations/sqlite/019_add_agent_to_request_logs.sql uses ADD COLUMN while Postgres uses ADD COLUMN IF NOT EXISTS. SQLite's ADD COLUMN doesn't support IF NOT EXISTS on older versions, and your migration tracker prevents re-runs, so this is fine in normal flow — but it diverges from the convention used in migrations/AGENTS.md (matching numbered pairs). Worth a one-line comment noting that SQLite relies on _migrations tracking for idempotency.

6. Duplicate fixture definitions. tests/luthien_proxy/e2e_tests/conftest.py:138-153 defines session-scoped mock_openai_server/mock_gemini_server that read MOCK_OPENAI_PORT/MOCK_GEMINI_PORT. tests/luthien_proxy/e2e_tests/mock_openai/conftest.py and mock_gemini/conftest.py define the same fixture name at a lower level without the env-var read. Lower-level conftest wins for tests in that directory, so the env-var path is silently ignored when running mock_openai/test_smoke.py directly. Either drop the duplicates or have them defer to the parent.

7. Env vars re-read per request. src/luthien_proxy/passthrough_routes.py:99-108, 33-39

OPENAI_API_KEY, GOOGLE_API_KEY, and *_BASE_URL are read from os.environ on every request. Negligible perf cost, but it means runtime os.environ mutation can flip behavior mid-flight — a small correctness/testing risk. Consider snapshotting at startup like the rest of the config system in config_fields.py.

8. The _reset_mock_server fixture lost its docstring (tests/luthien_proxy/e2e_tests/conftest.py:156) — the old comment explaining the mock_e2e gating is gone. Worth restoring since it documents a non-obvious gating behavior.

Security

9. Strict-key auth is good, but consider: cookie request header is forwarded upstream. _STRIP_AUTH covers API key headers but not cookie. A misbehaving client can ferry arbitrary cookies to api.openai.com/api.anthropic.com. Not exploitable in practice (providers don't honor them), but cleaner to strip.

10. _is_streaming parses the full body as JSON. src/luthien_proxy/passthrough_routes.py:71-78

Bounded by MAX_REQUEST_PAYLOAD_BYTES, so DoS-safe, but json.loads(body) on every passthrough request is wasted work for non-JSON paths (Gemini's :streamGenerateContent is already a fast-path). Minor.

11. request.body() is unbounded at the read layer. The post-read len(body) > MAX check rejects oversized payloads only after they've been buffered into memory. For a Track-A bridge this is acceptable, but worth flagging that an attacker without content-length (chunked) can still force the gateway to allocate up to whatever Starlette's transport accepts.

Performance

Lifespan-managed clients with appropriate timeouts (connect=10, read=300, write=10, pool=30 for streaming; 30 for buffered) look right for proxying LLM traffic. No connection-pool limits are set — defaults to httpx's 100 keepalive/concurrent which should be fine.

Test coverage

Excellent. test_passthrough_routes.py and test_passthrough_auth.py cover the security-critical paths (strict key, mode interactions, body limit, hop-by-hop stripping, lifespan teardown, streaming-error-status-forwarding, missing server key → 503). Sqlite e2e covers header persistence + outbound key isolation + luthien-header stripping. Mock e2e covers streaming for all three providers.

Gaps worth a follow-up (not blockers):

• No test that content-encoding round-trips correctly (would have caught bug chore(logging): replace prints with structured logging #1).
• No test for the streaming aread()-failure path (bug datetime.datetime.utcnow() is deprecated #3).
• No test for malformed content-length (bug Remove PolicyEngine and inline Redis setup #2).

Conventions

• Per CLAUDE.md "One PR = One Concern" — this PR bundles feature + 4 security fixes from trajectory/track a opencode luthien bridge #614 review. The fixes were originally on trajectory/track a opencode luthien bridge #614 so this is unwinding a previous bundle, not adding new bundling. Reasonable in context, but each future security fix should still get its own PR + COE.
• Changelog fragment present ✅
• Matching Postgres/SQLite migration ✅
• Unit tests mirror source layout ✅

Suggested merge action

Address #1, #2, #3 before merging (real bugs, all small). The rest are polish.

[claude]

Review — multi-provider passthrough routes

Overall this is a thoughtful, security-conscious PR. The 4 documented fixes (strict client key on /openai+/gemini, body-size limit, lifespan-managed httpx clients, hop-by-hop stripping) all look correctly implemented. The streaming "peek at status before committing to StreamingResponse" trick at passthrough_routes.py:165-227 is a nice catch.

Below are concerns ordered roughly by impact.

Security

1. ?key= query-param auth bypass on /gemini (medium). In passthrough_routes.py:139-140 the upstream URL is constructed with request.url.query appended verbatim. Gemini accepts API auth via either x-goog-api-key header or ?key=... query parameter. _build_outbound_headers strips client auth headers and injects the server key, but the query string is passed through untouched — so a client can call /gemini/v1beta/models/foo:generateContent?key=THEIR_OWN_KEY, and depending on Gemini's precedence the server-injected header may or may not win. At minimum, stripping key from the query for the gemini provider would close this gap. Worth a test too.

2. Body-size enforcement is reactive (low/medium). passthrough_routes.py:134-137 calls await request.body() first, then checks len(body) > MAX_REQUEST_PAYLOAD_BYTES. By the time the length check runs, Starlette has already buffered the full body in memory. The pre-check on content-length (lines 126-132) handles well-behaved clients, but a request without content-length (or with a lying content-length) can still push the gateway to OOM. The fix is to read incrementally from request.stream() and abort once the cumulative size exceeds the limit.

3. Operator foot-gun: _upstream_base reads OPENAI_BASE_URL / GEMINI_BASE_URL / ANTHROPIC_BASE_URL at request time (low). Useful for tests, but means a stray env var in prod silently redirects all upstream traffic. Worth gating behind a debug/test-mode flag, or at least a startup log line.

Correctness / Bugs

4. Streaming 2xx drops all upstream response headers except content-type (medium). Compare the buffered path (passthrough_routes.py:245-256) which preserves all non-blocked upstream headers via safe_headers, vs. the streaming 2xx path (passthrough_routes.py:208-227) which only forwards media_type=upstream_content_type — so things like x-request-id, openai-organization, anthropic-ratelimit-*, etc. are silently dropped on streaming responses. This is a divergence between streaming and non-streaming clients that will bite anyone debugging streaming-only issues. Fix: build safe_headers and pass to StreamingResponse(headers=...) like the non-streaming branches do.

5. Connection leak window if StreamingResponse is never iterated (low). Once upstream_cm.__aenter__() returns on the 2xx path, the __aexit__ only runs inside stream_chunks's finally. If something between return StreamingResponse(...) and the first iteration raises (rare but possible — e.g. middleware error), the upstream connection stays held. Not blocking; just noting.

6. _handle_passthrough body or None (low). content=body or None on lines 177 and 234 treats an empty bytes() as "no body". For POST/PUT, an intentionally empty body is legal — this would silently change to "no body" semantically. Probably never matters for OpenAI/Gemini/Anthropic but worth knowing.

Test coverage gaps

7. No unit test for the new _write_logs skip-empty-outbound branch (recorder.py:247-250). This is shared infrastructure change — a regression here (e.g. someone "fixes" the skip) would silently start inserting fully-NULL outbound rows for every passthrough request. Quick test: populate only the inbound side, assert mock_conn.execute.call_count == 1.

8. No test for the streaming-headers-dropped behavior in #4 above — if you preserve headers on streaming responses, add a regression test (e.g. mock upstream returns x-request-id, assert it round-trips).

9. No test for the ?key= Gemini bypass (item #1).

Nits

• passthrough_routes.py:46 — _STRIP_AUTH includes x-anthropic-api-key, but the Anthropic alias branch at lines 113-117 re-adds it from the original request headers. Works, but reads as accidentally re-adding a stripped header. A comment would help.
• Naming inconsistency: _STRIP_INBOUND / _STRIP_AUTH / _STRIP_RESPONSE have leading underscores; HOP_BY_HOP_HEADERS / DANGEROUS_RESPONSE_HEADERS are exported. Pick one.
• passthrough_routes.py:154 — body={} to the recorder means request bodies are never logged for passthrough. Comment explains why, but for OpenAI/Gemini specifically, JSON bodies under MAX_BODY_BYTES would be useful for debugging. Consider parsing-on-best-effort.
• Migration 019 SQLite uses bare ADD COLUMN (correct — SQLite doesn't accept IF NOT EXISTS on columns). Good.

Nice work. The architecture comment marking this as "replaced by native pipeline in Track B" sets the right expectation that this is bridge code.

[claude]

Code Review

Solid PR — the security framing in the description matches what's in the diff, the unit and e2e coverage is good, and the streaming "peek status then commit" pattern is the right call. Comments below are roughly in priority order.

Bugs / correctness

1. _PendingLog skip-marker is brittle (recorder.py:247)

if pending.http_method is None and pending.direction == "outbound":
    continue

Using http_method is None as a sentinel for "passthrough — outbound never populated" couples this branch to passthrough's specific behavior. If any non-passthrough flow ever forgets to call record_outbound_request(), that row will be silently dropped instead of producing a visible NULL row that surfaces the bug. Consider an explicit boolean on _PendingLog (e.g. populated: bool = False, flipped in record_outbound_request) — same outcome for passthrough, fails loudly elsewhere.

2. Recorder records the inbound URL verbatim (passthrough_routes.py:170)

recorder.record_inbound_request(... url=str(request.url) ...)

str(request.url) includes any query string the client sent. For /gemini/... the client could attach ?key=AIza... (and the outbound-stripping logic at line 152–156 exists precisely because clients do this). The inbound row in request_logs would then store the raw key. sanitize_headers redacts header-based credentials but not URL-based ones — worth stripping ?key= from url before passing it to record_inbound_request, or running the URL through a URL-sanitizer alongside sanitize_headers.

3. dict(request.headers) collapses duplicate headers (passthrough_routes.py:171)

Starlette's request.headers is multi-valued (e.g. multiple Set-Cookie, Forwarded). Calling dict(...) keeps only the last value. Not load-bearing for the current set of recorded fields, but the sanitizer will under-represent multi-valued headers in the log. If you care about full fidelity, use request.headers.multi_items().

4. Possible upstream connection leak on HTTPException inside streaming setup (passthrough_routes.py:191–197)

streaming_client.stream(...) returns the async context manager synchronously; the resource is acquired by await upstream_cm.__aenter__(). If __aenter__() succeeds and any later code in the streaming branch raises before the StreamingResponse is returned (e.g. an unexpected exception in _safe_response_headers, or a HTTPException raised by code added in the future), nothing calls __aexit__. Today the path between __aenter__ and the return is tight and unlikely to raise, but wrapping it in a try/except with __aexit__(...) in the failure path would be more defensive.

Security

5. x-stainless-* / user-agent leakage to upstream

The Anthropic and OpenAI SDKs emit x-stainless-os, x-stainless-arch, x-stainless-runtime, etc., describing the client environment. The generic forwarding loop at passthrough_routes.py:93–102 forwards them as-is. For /openai and /gemini (where the gateway injects the server's API key), this leaks the operator's or client's environment fingerprint to the upstream alongside server credentials, which is probably not what the operator wants. Consider stripping x-stainless-* (and possibly cookie) similar to the x-luthien-* strip.

6. verify_passthrough_token allows literally any token in PASSTHROUGH/BOTH mode (passthrough_auth.py:38)

Documented behavior, and consistent with the existing /v1 chain. Worth confirming this is fine for the Anthropic alias since the alias forwards client auth as-is. The PR description says "Open proxy closed" for /openai and /gemini — would be worth restating in the changelog that /anthropic intentionally is not subject to strict CLIENT_API_KEY because it inherits the existing /v1 auth semantics, to head off confusion in security audits.

Code quality

7. Duplicated header-filtering at passthrough_routes.py:213–219

Same expression as _safe_response_headers. Just call the helper.

8. Two httpx.AsyncClient instances, only differing in timeout

passthrough_streaming_client (300s read) vs passthrough_buffered_client (30s). Both share httpx connection-pool defaults. Pre-existing API design choice — fine — but worth a one-line comment on main.py:390–393 saying why two clients exist (different read timeouts for streaming vs buffered) so a future reader doesn't collapse them.

9. from __future__ import annotations in passthrough_routes.py but not passthrough_auth.py

Minor consistency nit.

Tests

Coverage is strong. Gaps that would be worth adding:

• No test of the streaming error-peek path (passthrough_routes.py:204–224) — i.e. upstream returns 4xx/5xx for a streaming request, verifying the client sees the real status, the body is forwarded, and __aexit__ is called. This is the most subtle code in the PR and is currently uncovered.
• No test that ?key= is stripped on /gemini — the comment at passthrough_routes.py:153 describes the threat clearly; an end-to-end test that sends ?key=hacker-key and asserts the mock upstream did not receive it in either header or query string would lock that in.
• _is_streaming edge cases — empty body, non-JSON body, {"stream": "true"} (string vs bool) aren't covered.
• Test for the recorder skip-condition (issue chore(logging): replace prints with structured logging #1 above) — verify that a passthrough request produces exactly one row (inbound) in request_logs, not two.

Done well

• Strict client-key auth for the two routes that inject server credentials (/openai, /gemini) — correct threat model.
• secrets.compare_digest everywhere, with the WWW-Authenticate: Bearer header on 401s.
• The streaming "peek status before committing 200" pattern is exactly right; the inline comment explaining it is helpful.
• Lifespan-managed clients with documented shutdown ordering relative to _webhook_sender.stop().
• Hop-by-hop + set-cookie/server stripping on responses.
• x-goog-api-key added to sanitize_headers.
• Matched Postgres/SQLite migration pair (019), and the schema test asserts the column exists.

🤖 Generated with Claude Code

[claude]

Code Review (claude-opus-4-7)

Solid PR overall. The security-fix summary in the description matches the implementation, and unit-test coverage of the new passthrough surface is good. A few items worth addressing before merge.

Likely bugs

1. Recorder comment contradicts behavior — src/luthien_proxy/request_log/recorder.py:249-255.

   if not pending.populated and pending.direction == "outbound":
       # ... non-passthrough flows that forget to call
       # record_outbound_request() surface as a visible NULL row.
       continue

   The comment says forgetful callers will "surface as a visible NULL row," but continue does the opposite — it silently skips the insert. Either the comment or the behavior should change. If silent skip is intended, the comment is misleading; if visible-NULL was intended, the continue should be removed.

2. Silent behavior change for any caller of record_outbound_response() only. With the new populated flag, a caller that only invokes record_outbound_response() (without record_outbound_request()) will silently lose its outbound row. Current internal callers (anthropic_processor.py:177) appear safe, but worth a quick grep + maybe an assertion (record_outbound_response could set populated=True too, or warn if called first) to prevent quiet data loss in the future.

3. MAX_REQUEST_PAYLOAD_BYTES check happens after fully buffering the body — passthrough_routes.py:160-163. await request.body() reads the entire body into memory before the size check. A client that omits Content-Length (or lies about it) can force the proxy to allocate up to whatever Starlette/uvicorn's underlying limit is. The Content-Length pre-check at line 152 only helps honest clients. Consider streaming the read with a running byte budget (or rely on a documented uvicorn --limit-request-size). This matches existing /v1/ behavior at gateway_routes.py:245, so it's not a regression — but a known weakness worth noting in the COE/follow-up.

4. Buffered httpx timeout of 30s is likely too low — main.py:393. Anthropic/OpenAI non-streaming responses for long generations regularly exceed 30s (Claude with extended thinking, GPT-4o with large max_tokens, etc.). The streaming client correctly uses 300s, but the buffered client will return 502 on otherwise-healthy requests. Suggest aligning buffered read timeout closer to 120-300s.

Medium

5. No rate limiter on passthrough routes. /v1/messages consults TokenBucketRateLimiter (gateway_routes.py:183-187), but the new /openai, /gemini, /anthropic/* endpoints bypass it. Since /openai and /gemini burn server-side credits, this is the place you most want rate limiting if it's enabled.

6. OPENAI_BASE_URL/GEMINI_BASE_URL/ANTHROPIC_BASE_URL are not registered in config_fields.py. They're read via os.environ.get(...) on every request (passthrough_routes.py:34-40). If these are test-only knobs, they should probably be made explicit (or read from settings); if they're operator-facing, they should join the config registry so .env.example / /config reflect them.

7. PR scope per "One PR = One Concern" (CLAUDE.md). This PR mixes three concerns: passthrough routes + auth, recorder schema/behavior change (agent column + populated flag), and mock OpenAI/Gemini test servers. The recorder changes in particular feel separable — they could be reviewed as their own PR with their own COE if they were to land first.

8. No policy enforcement on passthrough routes — by design per the in-code comment "replaced by native pipeline in Track B (Multi-provider: OpenAI type definitions + client #563-569)", but worth flagging explicitly in the PR description: these endpoints intentionally bypass Luthien's whole reason for existing. Operators should know not to expose /openai or /gemini to untrusted clients until Track B lands.

Low / nits

9. _is_streaming accepts any truthy stream value — bool(data.get("stream", False)) returns True for "false", "0", [] (well, that's falsy), etc. The existing test_stream_string_true_is_streaming enshrines this. Anthropic/OpenAI normally reject non-boolean values upstream, so it's a minor issue, but stricter data.get("stream") is True would be safer.

10. _sanitize_url only handles Gemini ?key=. Defensive: a generic pass that masks _SECRET_PATTERN matches in any query string would be future-proof, since logs are durable.

11. _handle_passthrough reads os.environ.get("OPENAI_API_KEY") per-request — works, allows hot reload, but slightly surprising. Reading from settings at startup (or via request.app.state.dependencies) would be more consistent with the rest of the codebase.

12. Streaming mid-stream upstream failure. If httpx.RequestError is raised partway through aiter_bytes() (passthrough_routes.py:244-254), the client has already received 200 OK and a partial body; only the DB log gets updated to 502. The comment at line 200 covers the pre-stream case but not the mid-stream one — worth a short note that mid-stream upstream errors are surfaced as a truncated stream, not an error to the client.

13. Test file size. tests/luthien_proxy/unit_tests/test_passthrough_routes.py is ~810 lines for a ~313-line module (≈2.6x). Within the CLAUDE.md guideline (3x cap) but on the high side; the streaming-mock setup is repeated a few times and could be a fixture.

Things done well

• Strict CLIENT_API_KEY enforcement on /openai and /gemini (verify_strict_client_key) with timing-safe comparison and a clear docstring explaining the threat model.
• Auth + x-luthien-* header stripping on outbound, plus query-string key= stripping on Gemini (both for outbound and for logged URLs).
• Hop-by-hop + dangerous response header (set-cookie, server, x-powered-by) stripping.
• Lifespan-managed httpx clients with explicit shutdown ordering relative to webhook sender.
• Streaming peek-then-handoff strategy avoids the StreamingResponse "always 200" trap for upstream 4xx/5xx.
• Migration 019 is matched across all three locations (postgres, sqlite, sqlite_migrations copy) per migrations/CLAUDE.md.
• New routes' header sanitization is consistent with sanitize_headers (x-goog-api-key added to the sensitive list).

— claude-opus-4-7

[claude]

Code Review — PR #758: Multi-provider passthrough routes

Generally solid PR with good test coverage (810 lines of unit tests + sqlite/mock e2e). The 4 explicit security fixes from the #614 review are well-implemented. A few concerns worth surfacing before merge:

Security: /anthropic/{path} bypasses the policy chain

This is the most important finding. /anthropic/v1/messages and /v1/messages both proxy to https://api.anthropic.com/v1/messages, but they take different code paths:

• /v1/messages (gateway_routes.py:193) → full policy chain, event emission, request logging via the normal pipeline
• /anthropic/v1/messages (passthrough_routes.py:305) → raw passthrough, no policy enforcement, no event emission, no transformation

Any client that knows about the new alias can sidestep every policy you've configured (block_dangerous_commands, pii_redaction, judge policies, etc.) just by adding /anthropic to the path. The auth check (verify_passthrough_token) does not gate this — in BOTH/PASSTHROUGH mode any token is accepted.

The code comments say "replaced by native pipeline in Track B (#563-569)" so this appears intentional/temporary, but:

1. The changelog should call this out explicitly as a known limitation
2. Consider whether /anthropic/{path} should be disabled by default and gated behind an opt-in env var until Track B lands
3. At minimum, document the bypass in the route docstring and emit a startup logger.warning when the route is registered

Bugs / correctness issues

StreamingResponse always returns 200 even for 2xx-non-200 upstream status (passthrough_routes.py:259)

return StreamingResponse(stream_chunks(), media_type=upstream_content_type, headers=safe_headers)

The status_code parameter defaults to 200. If upstream returns 201/202/206 for a streaming response, the client sees 200. Worth passing status_code=response.status_code for the 2xx path.

Body size check is post-buffering (passthrough_routes.py:160)

body = await request.body()
if len(body) > MAX_REQUEST_PAYLOAD_BYTES:
    raise HTTPException(status_code=413, ...)

request.body() buffers the entire request into memory before the size check. A malicious client can send a 10 GB body without Content-Length (chunked encoding) and OOM the worker before reaching this check. The header check above only catches clients that honestly declare the size. Either (a) rely on uvicorn / front-proxy limits and document the assumption, or (b) stream the body with a running size counter.

except ValueError: raise HTTPException(...) without from exc (passthrough_routes.py:158)
Drops the underlying exception from the chain. Minor style issue but the rest of the file is exception-chain-aware (e.g. connect_db uses from exc).

_handle_passthrough exception handler only catches httpx.RequestError: other httpx.HTTPError subclasses propagate as 500. In practice the streaming/buffered paths only raise RequestError for connection issues, so this is probably fine — just flagging in case you want a wider net.

Design / consistency concerns

Configuration not routed through config_fields.py: OPENAI_API_KEY, GOOGLE_API_KEY, OPENAI_BASE_URL, GEMINI_BASE_URL, ANTHROPIC_BASE_URL are all read via os.environ.get(...) on every request. Per CLAUDE.md: "All gateway configuration is defined in src/luthien_proxy/config_fields.py — single source of truth". These should be added to CONFIG_FIELDS so they appear in /config, .env.example, and are CLI-overridable. Reading env vars on every request also misses the registry's provenance tracking and means an in-process env-var mutation in tests changes production behavior — already exploited by the test suite, which is itself a smell.

_handle_passthrough is ~130 lines with three semi-independent branches (header build, streaming, buffered). Easy to refactor into smaller helpers; would also simplify the test setup.

No test coverage for non-POST methods: Routes accept GET, POST, PUT, DELETE, PATCH but only POST is tested. Gemini's GET endpoints (e.g. listing models) and Anthropic's GET /v1/models are reachable; worth at least one smoke test per non-POST verb to confirm body handling, auth, and recording work without a JSON body.

_is_streaming parses the entire body as JSON on every request to look up one key. Cheap for typical chat bodies; pricier for very large request bodies. Could short-circuit on b'\"stream\"' not in body.

Things done well

• Pre-streaming status peek so non-2xx upstream errors propagate the real status (great catch — StreamingResponse locks 200 immediately once returned)
• Separate streaming vs buffered httpx clients with different timeouts (300s read vs 120s)
• Lifespan-managed clients with proper teardown
• secrets.compare_digest for token comparison
• Strict CLIENT_API_KEY-only enforcement on /openai and /gemini (regardless of AUTH_MODE) — correct threat model for server-injected upstream keys
• x-luthien-* headers stripped from outbound (test confirms)
• ?key= query param stripped from Gemini outbound + sanitized in logged URL (defense-in-depth — prevents bypass + log leakage)
• Hop-by-hop + dangerous response header stripping
• Migration 019 correctly mirrored across migrations/postgres/, migrations/sqlite/, and src/luthien_proxy/utils/sqlite_migrations/
• Tests cover the 503 path when server keys are missing, sensitive-header redaction, body size enforcement, hop-by-hop stripping, mid-stream upstream error reporting
• record_inbound_request receives raw headers and lets the recorder's sanitize_headers do the redaction — single redaction point is good

Minor nits

• _DEFAULT_RESPONSE in mock_openai/server.py has "created": 0 — real clients may choke on epoch 0, consider int(time.time())
• _safe_response_headers does three .lower() calls per header per check — minor allocation; could lower_key = k.lower() once
• Streaming response forwards upstream content-type via both media_type= and headers= — Starlette dedupes, but passing only one would be clearer

Test coverage assessment

Strong. The unit test file covers happy path, error paths, header stripping, body size, query sanitization, streaming success, streaming error status forwarding, content-encoding stripping, content-length validation, recorder skip-empty-outbound, and _is_streaming parsing edge cases. The sqlite e2e tests cover header persistence to DB, server key non-leak, query-param sanitization, and auth enforcement. Coverage gap: non-POST methods, and explicit confirmation that the /anthropic alias's policy-bypass is documented behavior (which arguably shouldn't be tested as "works" until the bypass is intentional).

---

Recommend addressing the /anthropic bypass concern (at minimum: explicit docs + startup warning) before merge. The 200-status-for-2xx-streaming and post-buffer body check are worth fixing in this PR. Config-registry migration can be a follow-up.

🤖 Generated with Claude Code

[claude]

Review: Track A passthrough routes + security fixes

Thoughtful PR with a clear security narrative and strong test coverage. A few items worth addressing before merge.

Findings

1. httpx-sse dependency is added but unused in this PR
pyproject.toml:45 adds httpx-sse>=0.4 and uv.lock follows suit, but no source/test file imports it. The PR description says this depends on PR #757 (which adds httpx-sse), so this may be a merge artifact — once #757 lands first, the dep entry here is redundant. Either drop it from this PR or document why it's re-added.

2. Streaming flush can be skipped if __aexit__ raises (src/luthien_proxy/passthrough_routes.py:241-257)

finally:
    await upstream_cm.__aexit__(None, None, None)  # if this raises…
    recorder.record_inbound_response(status=status, error=error)  # …never runs
    recorder.flush()

If the upstream connection cleanup itself raises (network blip, TLS teardown error), the recorder is never flushed and you lose the inbound row. Wrap __aexit__ in its own try/except, or move the recorder calls into a nested try so flush always runs. Same pattern is fine on the error path at line 222-226 since the error body is captured first.

3. Passthrough body recorded as {} is misleading (passthrough_routes.py:187)

recorder.record_inbound_request(
    ...
    body={},  # passthrough bodies may be non-JSON or very large; not logged

The recorder serializes {} to the literal string "{}" in request_logs.request_body — indistinguishable from a real empty JSON body. Operators inspecting logs can't tell "not captured" from "empty payload." Suggest passing None instead and widening RequestLogRecorder.record_inbound_request's body param to dict[str, Any] | None (its _serialize_body already handles None correctly).

4. _handle_passthrough is long and mixes concerns (~140 lines)
Splitting into _handle_buffered(...) and _handle_streaming(...) helpers — with the size/headers/recorder bootstrap shared above — would make the streaming-status-peek logic (which is the subtle part) much easier to review and test in isolation. Not blocking.

5. Minor: redundant media_type on StreamingResponse (passthrough_routes.py:262)
safe_headers already contains content-type from the upstream response (it's not stripped), so media_type=upstream_content_type is just a fallback for the case where upstream omits it. Harmless, just noting in case the intent was different.

6. Minor: 503 for missing provider key reads as transient (passthrough_routes.py:107, 112)
Missing OPENAI_API_KEY/GOOGLE_API_KEY is a config error, not "service unavailable, try again." 500 (or 501 Not Implemented) communicates the persistent nature more accurately. Operators searching dashboards for 5xx spikes may misread the 503 as upstream flakiness.

What's good

• Strict client-key gating for /openai and /gemini (passthrough_auth.py:69-110) — the threat model docstring is excellent and the secrets.compare_digest usage is correct. Fail-closed when CLIENT_API_KEY is unset is the right default for an open-proxy risk.
• Body size limit checked both pre-read (Content-Length) and post-read — handles missing/lying Content-Length headers.
• Gemini ?key= stripping on both outbound URL and logged URL (_sanitize_url) — prevents both bypassing server auth injection and leaking client-supplied keys into request_logs.
• Header sanitization architecture — passthrough passes raw dict(request.headers) to the recorder, and sanitization happens in RequestLogRecorder via sanitize_headers. Centralized redaction is the right boundary.
• Lifespan-managed httpx clients with separate timeouts for streaming (300s read) vs buffered (120s) — sensible split.
• Streaming status-peek logic (status checked before committing to StreamingResponse, error body returned as plain Response) — correctly handles the "can't change HTTP status mid-stream" problem.
• Migration 019 follows the postgres + sqlite + sqlite-copy convention from migrations/CLAUDE.md. The "indexing TBD in Track B" note is appropriate for a bridge.
• Test coverage is strong across all three tiers: unit (auth, body limits, hop-by-hop stripping, key stripping, content-encoding), sqlite_e2e (header persistence to DB, key injection, no leakage), mock_e2e (streaming and non-streaming for all three providers).

Nits

• passthrough_routes.py:144 — "key=" not in url is a loose pre-check (matches apikey=, secret-key=, etc.) but harmless since the actual filter at line 147 only strips exact-match "key". Could tighten the check or just drop the optimization.
• main.py:398-402 — The startup warning about /anthropic/* bypassing the policy chain reads as info-level once this route is intentional and documented. Consider logger.info to avoid alarming operators on every boot.
• pyproject.toml:45 — if httpx-sse is kept, add an inline comment or move to its own group noting which subsystem will consume it (eases archaeology when Track B lands).

Overall: solid Track A bridge with the right security posture. Address #2 (flush ordering) before merge; the rest are quality-of-life improvements.

🤖 Review generated with Claude Code

[claude]

Code Review — PR #758: Multi-provider Passthrough Routes

Reviewed passthrough_auth.py, passthrough_routes.py, lifespan changes in main.py, migration 019, request_log/recorder.py and sanitize.py changes, plus the unit/e2e test additions. Overall this is a well-scoped Track A bridge with thoughtful security hardening and strong test coverage. A few items worth addressing or considering.

🔴 Recommend addressing

1. Inbound hop-by-hop headers forwarded upstream (passthrough_routes.py:86-123)

_build_outbound_headers strips only _STRIP_INBOUND (host, content-length), _STRIP_AUTH, and x-luthien-*. Hop-by-hop headers from the client (connection, keep-alive, transfer-encoding, te, trailer, upgrade, proxy-authorization, proxy-authenticate) are passed through to OpenAI/Gemini/Anthropic. By RFC 7230 §6.1 these belong to a single connection and must not be forwarded by intermediaries; in practice this can confuse the upstream (e.g. a leaked transfer-encoding: chunked from a client that buffered the body). You already define HOP_BY_HOP_HEADERS for response stripping at line 53 — apply the same set to inbound. Bonus: also strip cookie from inbound, since none of these three providers consume cookies and forwarding them could leak browser session data if the route is ever called by a non-API client.

2. NoOpRequestLogRecorder.record_inbound_request signature drift (request_log/recorder.py:275-288)

The real method (line 141) is body: dict[str, Any] | None; the NoOp override is body: dict[str, Any] (non-optional). The passthrough route calls it with body=None (passthrough_routes.py:187), which is fine at runtime because NoOp is a no-op, but Pyright will eventually flag the LSP violation. Make the NoOp signature match.

3. media_type and content-type both passed to StreamingResponse (passthrough_routes.py:262-267)

safe_headers already contains the upstream content-type and you also pass media_type=upstream_content_type. Starlette uses media_type to build a Content-Type header and then merges with headers, which can yield duplicate Content-Type lines depending on Starlette version. Drop one — simplest is to remove content-type from safe_headers before passing, or stop passing media_type and let safe_headers carry it.

🟡 Consider

4. Per-request os.environ.get(...) for provider keys and base URLs (passthrough_routes.py:34-40, 104-113)

Every passthrough call does multiple env lookups. Functionally fine, but it bypasses the new config system (config_fields.py) that the rest of the gateway uses, and it means tests must monkeypatch.setenv. Consider promoting OPENAI_API_KEY, GOOGLE_API_KEY, OPENAI_BASE_URL, GEMINI_BASE_URL, ANTHROPIC_BASE_URL to first-class config fields so they show up in the /config admin UI and are validated at startup.

5. Body-size enforcement happens after full buffering (passthrough_routes.py:152-163)

The content-length precheck is good, but a client that omits content-length and streams a 5 GB body has it fully buffered into memory by await request.body() before the post-read check rejects it. Either (a) document that this relies on upstream uvicorn --limit-request-body / Starlette protections, or (b) read in chunks and bail out early. Low priority because uvicorn's defaults usually save you, but it's worth a comment if you're keeping it as-is.

6. int(content_length) accepts negative values (passthrough_routes.py:154-158)

A negative integer parses fine and is < MAX_REQUEST_PAYLOAD_BYTES, so the check passes. Then len(body) is the actual safeguard. Minor — reject < 0 explicitly to surface bad clients early.

7. Streaming __aexit__ swallows exception context (passthrough_routes.py:255-258)

The finally block calls await upstream_cm.__aexit__(None, None, None) without the propagating exception's type/value/tb. For httpx this is mostly benign, but if an exception is in flight, signalling (None, None, None) makes some context managers skip their error-path cleanup. Use sys.exc_info() here.

8. _sanitize_url is Gemini-only (passthrough_routes.py:136-148)

Only ?key= for Gemini is redacted before logging. If a client sends ?api_key=... or any other secret query param to OpenAI/Anthropic, it lands raw in request_logs.url. Consider a generic blocklist (key, api_key, access_token, token) applied to all providers.

9. _is_streaming catches AttributeError (passthrough_routes.py:82)

The data.get("stream") chain catches AttributeError to defend against non-dict JSON bodies (e.g. [...]). A cleaner pattern: check isinstance(data, dict) after json.loads. Same behavior, fewer surprises.

10. Reading request.headers twice for inbound logging (passthrough_routes.py:186, 188-190)

You pass headers=dict(request.headers) and re-read three specific x-luthien-* headers separately. The full headers dict goes through sanitize_headers and stores everything — including the x-luthien-* values you already extracted. Not a bug, just slight redundancy.

🟢 Things done well

• Strict client-key enforcement on /openai and /gemini — the security threat model in verify_strict_client_key's docstring (passthrough_auth.py:73-86) is exactly the right framing. Open-proxy closure is the most important fix here.
• Timing-safe comparison via secrets.compare_digest everywhere.
• Streaming status-code handling (passthrough_routes.py:198-232) correctly opens the upstream connection, peeks at the status, and downgrades non-2xx to a buffered Response so 4xx/5xx don't get masked by StreamingResponse's 200. Good catch — this is exactly the kind of thing that silently breaks in proxies.
• Lifespan-managed httpx clients with separate buffered/streaming timeouts and explicit ordering vs webhook_sender.stop() in main.py:409-419. The inline comment about ordering is excellent.
• Hop-by-hop response header stripping is correctly applied to both buffered and streaming paths.
• Test coverage: ~1200 lines spanning auth modes, body limits, header stripping, Gemini key query stripping, streaming error propagation, lifespan cleanup, content-encoding stripping, and e2e persistence to request_logs. The TestStreamingUpstreamError parametrization over [401, 429, 500, 503] is the right shape for this kind of forwarding logic.

Migration 019

Postgres + SQLite + utils copy are in sync. Index intentionally deferred — fine for a Track A bridge but please track the indexing decision before this table grows, since request_logs already has 8 indexes and you may want a composite (agent, started_at) instead of a standalone agent index.

CLAUDE.md compliance

PR is appropriately scoped per "One PR = One Concern" — passthrough routes + their security fixes + their migration is one cohesive concern. Changelog fragment present. 👍

🤖 Generated with Claude Code

[claude]

Code Review — PR #758

Solid PR. The four advertised security fixes are real and the test coverage for each is concrete. I have one medium-severity concern about the body-size limit, and a handful of nits and consistency observations. Nothing I'd block on.

Security

🟡 Body-size limit is too late to prevent memory exhaustion — src/luthien_proxy/passthrough_routes.py:159-170

content_length = request.headers.get("content-length")
if content_length:
    try:
        if int(content_length) > MAX_REQUEST_PAYLOAD_BYTES:
            raise HTTPException(status_code=413, ...)
    except ValueError as exc:
        raise HTTPException(status_code=400, ...)

body = await request.body()   # ← reads entire body into memory
if len(body) > MAX_REQUEST_PAYLOAD_BYTES:
    raise HTTPException(status_code=413, ...)

The post-body() length check only fires after the full body has been buffered. A client that sends Transfer-Encoding: chunked (no Content-Length) bypasses the header check, and await request.body() will happily accumulate gigabytes before the comparison runs. The right shape is to read incrementally and abort early:

body = b""
async for chunk in request.stream():
    body += chunk
    if len(body) > MAX_REQUEST_PAYLOAD_BYTES:
        raise HTTPException(status_code=413, detail="Request payload too large")

…or enforce the limit at the ASGI middleware layer. The current code is fine against honest clients but doesn't really close the DoS vector against a hostile one.

🟢 The other three security fixes hold up under read-through. verify_strict_client_key is the right call for /openai and /gemini, secrets.compare_digest is used everywhere a constant-time comparison matters, _STRIP_AUTH reliably scrubs caller-supplied keys before forwarding upstream, _STRIP_INBOUND_HOP_BY_HOP correctly includes cookie, response-side stripping handles set-cookie/server/x-powered-by, and _sanitize_url redacts Gemini's ?key= from logs.

🟢 Lifespan-managed httpx clients are correctly torn down in main.py:409-410 — closed first in the shutdown sequence, before webhook/purger/etc. Good ordering and good comment explaining the dependency relationship.

Bugs / Correctness

🟡 Inbound is_streaming is always logged as False for passthrough requests — passthrough_routes.py:190-201

record_inbound_request(...) is called without an is_streaming argument (so it defaults to False), and then streaming = _is_streaming(path, body) is computed on the next line but never propagated to the recorder. The inbound request_logs row will misreport streaming requests as non-streaming. Trivial fix:

streaming = _is_streaming(path, body)
recorder.record_inbound_request(
    ...,
    is_streaming=streaming,
)

🟡 record_outbound_response also sets populated = True — recorder.py:219

If a caller ever invokes record_outbound_response without first calling record_outbound_request, the outbound row will be written with http_method, url, etc. all NULL. No current caller does this, but the invariant ("outbound row is meaningful") would be safer to express by only setting populated in record_outbound_request. The skip-on-populated check at recorder.py:250 is a nice defense-in-depth pattern; it just shouldn't depend on response calls.

🟢 Streaming non-2xx handoff is well-designed — Manually __aenter__/__aexit__-ing the httpx context manager so you can peek at the status code before committing to StreamingResponse is exactly right. Without this, a 4xx/5xx from upstream would silently land as 200 with a JSON error body inside. Tests at test_passthrough_routes.py:392-414 cover this for 401/429/500/503.

🟢 Stream cleanup on mid-flight error — The finally block in stream_chunks() correctly closes the upstream connection even on client disconnect / cancellation. The comment acknowledging the "can't change HTTP status mid-stream" tradeoff is good context for future maintainers.

Code Quality / Consistency

🟡 Provider config bypasses the central config registry — passthrough_routes.py:35-40, 112, 117

OPENAI_API_KEY, GOOGLE_API_KEY, OPENAI_BASE_URL, GEMINI_BASE_URL, and ANTHROPIC_BASE_URL are all read directly via os.environ, not via config_fields.py. Per CLAUDE.md: "All gateway configuration is defined in src/luthien_proxy/config_fields.py — single source of truth." Adding entries there would give them /config dashboard visibility, CLI flags, provenance tracking, and DB overridability for free. Recognized this is documented as a "temporary Track A bridge," so happy to accept the deferral — just flag it so it doesn't become permanent dark config.

🟢 Comments document non-obvious decisions clearly — The two-client design (300s vs 120s timeouts), the >=300 peek-before-stream rationale, the key= query-string stripping for Gemini, and the bridge-warning startup log are all things a future reader would otherwise have to reconstruct.

🟢 Anthropic alias in CLIENT_KEY mode is effectively useless but not broken — In CLIENT_KEY mode, verify_passthrough_token only accepts the configured CLIENT_API_KEY; that same key then gets forwarded to api.anthropic.com, which will reject it. Not a bug — it's the necessary consequence of "passthrough means forward client's key" — but worth a one-line caveat in the route docstring.

Test Coverage

🟢 Excellent. Unit tests cover both auth helpers across all three auth modes, body-size enforcement, hop-by-hop stripping (request and response), missing-server-key 503 paths, streaming error-status forwarding, Gemini ?key= query stripping, content-encoding stripping, and _is_streaming edge cases (empty body, non-JSON, stream: "true" string vs bool, :streamGenerateContent path detection). E2E tests verify x-luthien-* headers persist to request_logs, server keys don't leak upstream, x-goog-api-key gets injected, x-luthien-* gets stripped outbound, and missing auth returns 401.

One small gap: there's no test that verifies the body-size check actually triggers via a streaming/chunked-encoded request (which is the case where the header check is bypassed). Worth adding once that path is hardened.

Migrations

🟢 019 looks correct. Postgres uses ADD COLUMN IF NOT EXISTS for idempotency; SQLite uses plain ADD COLUMN (SQLite doesn't support IF NOT EXISTS for ALTER TABLE). Numbers match across migrations/postgres/, migrations/sqlite/, and the embedded src/luthien_proxy/utils/sqlite_migrations/ copy.

Summary

Severity	Count	Items
Medium	2	Body-size check post-buffering; is_streaming not propagated to recorder
Low	2	record_outbound_response sets populated; provider env vars bypass config registry
Praise	many	Strict-key route gating, hop-by-hop scrubbing, lifespan cleanup, streaming peek-before-commit, test coverage

Approve once the chunked-body DoS path is either fixed or explicitly accepted with a follow-up ticket.

[claude]

Code review — multi-provider passthrough routes

Solid scope reduction from #614 with thoughtful security hardening. The two-client split (streaming vs buffered), peek-before-commit pattern for streaming status codes, and strict CLIENT_API_KEY enforcement for /openai//gemini are all well-handled. A few findings, roughly ordered by impact.

Bugs / correctness

1. /anthropic/v1/messages is not a drop-in replacement for /v1/messages — won't accept x-api-key auth (medium)
The existing /v1/messages route accepts either Authorization: Bearer … or x-api-key: … (see gateway_routes.py:get_request_credential). verify_passthrough_token uses HTTPBearer(auto_error=False) only, so a client that sends x-api-key without a Bearer header gets a 401 "Missing bearer token" — even though most Anthropic SDKs default to x-api-key. If this alias is meant to bridge real Anthropic clients (the comment in main.py:399-402 implies it is), parity with the existing auth chain matters. Suggest: also accept x-api-key/x-anthropic-api-key headers in the dependency for the /anthropic/* route only, or document the Bearer-only constraint loudly.

2. No rate limiting on passthrough routes (medium)
gateway_routes.py:186-187 runs rate_limiter.check(credential.value) on every /v1/messages request. Passthrough routes skip this entirely. Combined with finding (4) below, an authenticated operator-key holder can spend OPENAI_API_KEY/GOOGLE_API_KEY credits as fast as the upstream will accept, bypassing the configured RPM/burst. Suggest reusing get_rate_limiter in _handle_passthrough keyed by token.

3. Possible connection leak between __aenter__ and return StreamingResponse(...) (low)
In passthrough_routes.py:220-272, if any exception is raised after upstream_cm.__aenter__() succeeds but before the StreamingResponse is returned (e.g. while building safe_headers, or if _safe_response_headers were to raise), the upstream connection is never closed. Today the body between those points is just header-dict construction so the risk is theoretical, but wrapping the whole 2xx branch in a try / except → __aexit__ → re-raise would make the invariant local.

4. OPENAI_API_KEY/GOOGLE_API_KEY are read at request time without going through the config registry (low)
_build_outbound_headers calls os.environ.get(\"OPENAI_API_KEY\", \"\") directly. Per CLAUDE.md, all gateway config should live in config_fields.py so it's typed, documented, settable via CLI flag and visible on /config. The *_BASE_URL overrides have the same problem and aren't in .env.example. Hot-swappable behavior may be intentional — if so, please at least add the four env vars to .env.example and the changelog.

Smaller observations

• _is_streaming exception list (passthrough_routes.py:87): (json.JSONDecodeError, AttributeError, ValueError) — JSONDecodeError is a subclass of ValueError, so it's redundant. AttributeError only triggers if the JSON parses to a non-dict (e.g. []), which is worth a one-line comment so the next reader doesn't simplify it away.
• _safe_response_headers is called twice in the streaming 2xx path (passthrough_routes.py:246-249): builds a dict, then comprehends over it again to strip content-type. A single pass would be marginally cleaner: {k: v for k, v in response.headers.items() if k.lower() not in HOP_BY_HOP_HEADERS | DANGEROUS_RESPONSE_HEADERS | _STRIP_RESPONSE | {\"content-type\"}}.
• Bare except Exception swallows close errors silently (passthrough_routes.py:266-268): the warning loses the exception. exc_info=True would surface the cause.
• Path is forwarded verbatim, no allow-list: /openai/anything-OpenAI-adds-tomorrow will be proxied with server credentials. Probably intended, but worth a one-line note in the changelog so operators know the surface is unbounded.
• Buffered timeout 120 s vs streaming 300 s: the comment justifies the asymmetry, but Anthropic extended-thinking non-streaming responses can exceed 120 s on Opus. Worth verifying against real traffic before this displaces existing routes.

Tests

Coverage is genuinely strong — 281 lines on auth, 810 lines on routes, sqlite_e2e and mock_e2e tiers, mock servers for both new providers, schema regression test for migration 019. Particularly nice: test_streaming_upstream_error_returns_real_status parametrized over 401/429/500/503 directly validates the peek-before-commit design.

Gaps I noticed:

• No test for the Anthropic alias accepting x-api-key (because it currently doesn't — see finding chore(logging): replace prints with structured logging #1).
• No test that the body size limit is enforced on the streaming path (it's checked before the branch, so it works, but a regression test would lock that in).
• test_content_encoding_stripped_from_buffered_response has no streaming-path equivalent.
• sqlite_e2e tests use asyncio.sleep(0.5) to wait for the fire-and-forget log write (lines 26, 59). Per the polling pattern in conftest._wait_for_session, this would be more robust with a poll loop.

Migrations

019_add_agent_to_request_logs.sql is present in all three required locations (postgres, sqlite, bundled utils/sqlite_migrations). IF NOT EXISTS on the postgres side is good for idempotency; SQLite ALTER doesn't support it but the migration tracker handles that. Index decision deferred to Track B is reasonable.

---

Nice work overall — the security hardening (closed open proxy, strict client-key check, hop-by-hop stripping, lifespan-managed clients, query key= stripping) addresses the right concerns. Findings #1 and #2 are the ones I'd want resolved or explicitly punted before merge.

[claude]

PR Review: Multi-provider passthrough routes + 4 security fixes

Overall this is a well-scoped, well-tested PR with clear "why" comments throughout and a documented threat model. The security hardening (strict client-key auth, body-size limit, hop-by-hop stripping, lifespan-managed clients) is solid. Below are observations grouped by severity.

Bugs / correctness

1. _is_streaming JSON-parses every request body, even non-JSON ones (src/luthien_proxy/passthrough_routes.py:81). Cheap for small JSON, but the try/except (json.JSONDecodeError, AttributeError, ValueError) swallows decoding errors silently. Consider short-circuiting when content-type isn't application/json.

2. agent column has no length cap (migrations/postgres/019_add_agent_to_request_logs.sql). The value comes directly from the client-controllable x-luthien-agent header. An attacker could send megabyte-long header values to balloon the DB. The recorder doesn't truncate this field (only the body via MAX_BODY_BYTES). Consider VARCHAR(256) or truncating in record_inbound_request.

3. PASSTHROUGH mode + /anthropic/* accepts anonymous callers. With AUTH_MODE=passthrough, verify_passthrough_token returns "" for missing tokens. The route then forwards no auth, Anthropic returns 401, which the proxy wraps and returns. Anonymous callers can consume sockets and log entries without ever providing credentials. Not a credential-leak risk, but consider rejecting empty tokens early on /anthropic/* even in PASSTHROUGH mode.

4. Streaming __aexit__ exception is logged but recorder still reports pre-exception status (passthrough_routes.py:265-268). The except Exception: ... exc_info=True is correct, but status was never updated to reflect the close failure. If upstream cleanup fails after a mid-stream truncation, the log says the stream completed successfully.

5. _safe_response_headers returns original-case keys (passthrough_routes.py:133). If upstream sends two headers that differ only in case, the last-wins dict comprehension drops one. Low risk; httpx normalizes anyway.

Security observations

6. /anthropic/* bypasses the entire policy chain. This is documented in the startup log and route docstring, but it's a significant divergence from /v1/messages. Operators deploying judge policies, content filters, or rate limits will silently lose them on this route. Consider an admin-only opt-in flag (e.g. ENABLE_PASSTHROUGH_ANTHROPIC) so it's not on by default, given Track B will replace it anyway.

7. OPENAI_API_KEY / GOOGLE_API_KEY read from os.environ on every request (passthrough_routes.py:112,117). Reading once at startup (or via the settings/config registry) would be more robust and match the pattern used for other server-side credentials.

8. ?key= strip is Gemini-specific and correctly enforced. Good defense.

9. Hop-by-hop header policy is comprehensive. Minor gap: Strict-Transport-Security and similar from upstream are forwarded unchanged. If the gateway runs behind a non-HTTPS terminator, that could cause issues.

Code quality

10. stream_chunks() lacks a return-type annotation (passthrough_routes.py:251). Adding -> AsyncIterator[bytes] would catch refactor mistakes.

11. record_inbound_response(status=..., error=...) always omits response_body and response_headers for passthrough. Intentional (less log volume, less PII), but worth a one-line comment explaining the design choice so future maintainers don't add fields unknowingly.

12. scripts/start_mock_gateway.py directly mutates auth_config via raw SQL. Couples the script to migration 007's seed-row layout. If the seed ever changes, this breaks silently. Consider going through the admin API instead.

Tests

13. Test coverage is strong:

    • All three AuthMode permutations + missing-key cases
    • Body-size limits (both content-length lies and real body)
    • Hop-by-hop stripping for buffered AND streaming paths
    • Streaming upstream non-2xx → real status forwarded (parameterized 401/429/500/503)
    • Content-Type forwarding for SSE vs JSON
    • Gemini key= query strip and OpenAI key= passthrough
    • Header sanitization (sensitive headers redacted, pattern matching)
    • _write_logs skips empty outbound rows
    • Lifespan opens/closes httpx clients

14. Missing test: end-to-end assertion that agent column persists into Postgres (only SQLite tested in sqlite/test_passthrough_routes.py). The migration-sync test should catch schema parity, but a Postgres integration test would catch type/length mismatches.

15. TestMissingServerKey.test_openai_missing_key_returns_503 uses a nested patch.dict pattern that's hard to read. Consider monkeypatch.delenv("OPENAI_API_KEY", raising=False).

Migration

16. Migration files properly mirrored across postgres/, sqlite/, and src/luthien_proxy/utils/sqlite_migrations/. Follows the documented workflow.

Summary

Approve with minor nits. The bugs above are mostly hardening suggestions; the open-proxy and policy-bypass concerns on /anthropic/* are worth a follow-up ticket if not addressed here, but they're consistent with the documented "Track A bridge — replaced in Track B" framing.

Nice work on the test depth and the threat-model commentary in the docstrings.

🤖 Generated with Claude Code

[claude]

Code Review — PR #758 (multi-provider passthrough + security fixes)

Overall this is a well-scoped split-out. The security posture is materially better than the original PR #614 — strict CLIENT_API_KEY on /openai and /gemini is the right call, the body-size limit is enforced before await request.body() reads the stream, hop-by-hop / cookie / x-luthien-* stripping is thorough, and httpx clients are lifespan-managed. Test coverage is generous (1150+ lines across unit + sqlite_e2e + mock_e2e) and exercises the security paths well.

A few things worth addressing before merge.

Substantive

1. Provider configuration bypasses the centralized config system. passthrough_routes.py reads OPENAI_API_KEY, GOOGLE_API_KEY, OPENAI_BASE_URL, GEMINI_BASE_URL, ANTHROPIC_BASE_URL directly via os.environ.get(...) on every request (passthrough_routes.py:36-40, :112, :117). CLAUDE.md is explicit that all gateway config must go through config_fields.py (single source of truth, with CLI > env > DB > defaults resolution, provenance, and the /config admin UI). These five env vars are invisible to operators in the dashboard, never appear in .env.example, and cannot be set via CLI flags or DB. Recommend: add ConfigFieldMeta entries for each, regenerate settings.py/.env.example, and read them from settings (or via request.app.state.dependencies) instead of os.environ.

2. _is_streaming re-parses the request body on every call. passthrough_routes.py:81-88 does json.loads(body) purely to read stream: bool. Bodies up to MAX_REQUEST_PAYLOAD_BYTES (10 MB by default) will be parsed twice — once here, once implicitly by upstream — wasting CPU on the hot path. Two cheaper options: (a) gate on content-type: application/json and substring-search for "stream" before parsing, or (b) only parse when the path is one that supports streaming. Not a blocker, but a noticeable per-request cost for non-streaming traffic.

3. Duplicated Gemini key= stripping logic. The same parse_qsl / filter-out-key / urlencode sequence appears in _handle_passthrough (:177-178) and _sanitize_url (:153-155). Extract a single _strip_gemini_key_param(query) helper used by both — keeps the security invariant in one place.

4. _sanitize_url only redacts key=. Other auth-shaped query params (e.g. api_key=, access_token=) are logged verbatim into request_logs.url. For a strict CLIENT_API_KEY route this is mostly theoretical (only trusted clients are calling), but if the goal is defense-in-depth, consider redacting any param whose name matches (api_)?key|token|secret (case-insensitive).

5. Mid-stream error semantics are correct but worth verifying with a test. stream_chunks() (:251-270) handles httpx.RequestError raised inside aiter_bytes() by recording status=502 + error in the DB, but the client has already received a 200 and a truncated body. The code comment acknowledges this honestly. I don't see a test that exercises this path — only the pre-stream error path (test_flush_called_on_upstream_error) and the upstream-non-2xx path. Worth adding one to lock in the recorded-status=502 behavior so a future refactor doesn't silently lose error attribution.

Smaller items

6. headers[k_lower] = v lowercases all forwarded request headers (:109). Functionally fine — HTTP is case-insensitive and httpx normalizes — but inconsistent with how response headers are forwarded (case-preserved in _safe_response_headers). Either lowercase both or neither, for clarity.

7. x-luthien-model is recorded but the upstream body's model field is what actually gets billed. If a client sends x-luthien-model: gpt-3.5 but body.model = gpt-4, the log says gpt-3.5 and OpenAI bills gpt-4. This may be intentional (the header is a hint, not a control), but consider documenting it or sourcing model from the parsed body when present, so logs match reality.

8. _safe_response_headers collapses multi-value headers via dict comprehension (:133-140). The only multi-value response header most providers return is set-cookie, which is already in DANGEROUS_RESPONSE_HEADERS. Low risk, but vary could in principle be multi-valued. If you want to be strict, use httpx.Headers.multi_items() and build a list of tuples.

9. /v1/v1 regression test is great (test_default_anthropic_base_has_no_v1_suffix) — exactly the kind of guardrail the URL-prefix design needs.

10. Migration 019 looks clean — IF NOT EXISTS on Postgres, plain ALTER TABLE ADD COLUMN on SQLite, and the in-package SQLite copy matches the canonical file. Per CLAUDE.md the indexing decision is correctly deferred to Track B.

Nits

• passthrough_routes.py:127-128 sets a default user-agent of luthien-passthrough/0.1 — consider sourcing the version from PROXY_DISPLAY_VERSION so it doesn't drift.
• passthrough_routes.py:73-78: get_streaming_client / get_buffered_client aren't used as FastAPI Depends anywhere — they're called directly inside _handle_passthrough. Inlining the request.app.state.* reads would remove two unused indirections.
• Several test classes mix TestClient(app, raise_server_exceptions=True) and raise_server_exceptions=False — easy to read but the choice isn't always semantically meaningful. Minor.

Nothing blocking on security grounds. The config-system bypass (#1) is the most important architectural item; the body-reparse cost (#2) and missing mid-stream-error test (#5) would be nice to address before merge.

[claude]

Review: multi-provider passthrough + 4 security fixes

Solid PR — auth model is tight, the 4 security fixes match real risks, and test coverage on the sanitize/auth surface is genuinely good. A few items worth addressing before merge, plus some smaller polish notes.

Security / correctness — worth fixing before merge

1. Body-size limit can still buffer oversized payloads (passthrough_routes.py:159-170). The content-length pre-check is good, but a client can omit content-length (or send chunked) and the await request.body() on line 167 will allocate the full payload into memory before the post-read check on line 169 rejects it. The size limit is enforced semantically but not as a DoS guard. Consider iterating request.stream() and aborting when the byte counter exceeds MAX_REQUEST_PAYLOAD_BYTES.

2. No size limit on upstream buffered responses (passthrough_routes.py:280-297). response.content reads the entire upstream body into memory. With max_tokens unconstrained on Anthropic/OpenAI, a single request can produce a multi-MB JSON blob. The streaming path is fine because it iterates bytes; the buffered path should either cap response size or document the per-process memory ceiling.

3. Streaming context-manager leak on rare exception path (passthrough_routes.py:220-272). After await upstream_cm.__aenter__() succeeds, the code calls _safe_response_headers, response.headers.get, and constructs StreamingResponse before handing the cm off to the generator's finally. If anything between line 220 and the return on line 272 raises (e.g., a malformed content-type header from upstream), the upstream connection leaks. Wrap that setup in a try/except that calls await upstream_cm.__aexit__(...) on failure.

4. Logger may leak Gemini ?key= on connection errors (passthrough_routes.py:222, 287). repr(exc) from httpx typically includes the URL it was trying to reach. The outbound URL is already stripped of ?key= so this is mostly safe today, but the inbound URL is not — if anywhere down the line we log request.url directly, the unredacted key flows through. The _sanitize_url helper exists; consider routing all URL logging through it.

5. No length cap on tracked x-luthien-* headers (passthrough_routes.py:196-198). A client can send a 1 MB x-luthien-agent and it goes straight into the request_logs.agent column. Either truncate at the recorder boundary or cap at a sane length (e.g., 256 chars) before storing.

Code quality

6. Duplicate Gemini ?key= stripping logic (lines 143-155 vs 173-180). The outbound path and the logging path independently parse/rebuild the query string with slightly different shapes. Extract a _strip_gemini_key(url) helper so they can't drift.

7. NoOpRequestLogRecorder re-declares every signature (recorder.py:265-321). Easy footgun — add a kwarg to the real recorder and you have to remember to add it in the no-op too, or callers using kwargs will break only for users with logging disabled. Either inherit from RequestLogRecorder and override only flush/_write_logs to no-op, or define both via a Protocol.

8. _is_streaming re-parses JSON on every request (passthrough_routes.py:81-88). For GETs (no body) and :streamGenerateContent paths, the path check short-circuits — good. For everything else we parse the body. A request.method == "GET" early-return and/or a content-type guard would skip work on the obvious cases.

9. Hard-coded user-agent luthien-passthrough/0.1 (passthrough_routes.py:128) doesn't pull from PROXY_DISPLAY_VERSION. Easy to forget on the next version bump.

10. recorder.flush() is fire-and-forget but is called from sync code with asyncio.get_running_loop() (recorder.py:227-232). The except RuntimeError only logs at debug. If a future caller invokes flush() outside an event loop (e.g., a unit test path) the log will be silently dropped. Consider raising in development or at least logging at warning.

Tests

11. Coverage on auth, sanitize, and the streaming-status-forwarding edge case is genuinely strong. Two gaps worth adding:

    • No explicit unit test that Authorization is replaced (not duplicated) on OpenAI/Gemini outbound. test_openai_server_key_not_leaked_to_upstream covers the leakage angle, but a direct assertion on _build_outbound_headers output would prevent regressions on the header-merging logic.
    • No test for the post-read body-size check (passthrough_routes.py:169). The pre-read check via content-length is tested in TestBodySizeLimit::test_body_size_limit_413, but a test sending an oversized body with no content-length header would exercise the second guard (and surface concern chore(logging): replace prints with structured logging #1).

12. test_recorder.py:_write_logs path constructs the recorder with RequestLogRecorder.__new__(RequestLogRecorder) (test_passthrough_routes.py:731) which bypasses __init__. Works, but couples the test to private attributes (_inbound, _outbound, _db_pool). Consider a small factory helper or a real constructor with None-safe behavior to make the test less fragile.

Migration

13. migrations/sqlite/019_*.sql omits IF NOT EXISTS while the Postgres variant has it. Per the migration runner's tracking that's probably fine, but mirror the safety for consistency.

14. The agent column is unindexed by design (comment notes Track B will revisit). Fine for now — just flagging that the sqlite e2e test currently filters by session_id/model, so we haven't load-tested any query that would benefit from an agent index.

Things done well, for the record

• secrets.compare_digest everywhere auth tokens are compared.
• verify_strict_client_key is the right call for /openai and /gemini — independent of global AUTH_MODE, so a future operator flipping to passthrough mode doesn't accidentally re-open the proxy.
• Hop-by-hop + dangerous response header stripping with clear comments about why content-encoding is in the strip list.
• Lifespan-managed httpx clients with the documented streaming-vs-buffered timeout split.
• Startup log explicitly calls out that /anthropic/* bypasses the policy chain — this is the most surprising behavior in the PR and deserves the loud warning.
• Sanitize header tests cover case-insensitivity, mixed sensitive/non-sensitive, and the sk-/hex pattern fallback.

Nothing here blocks the spirit of the PR — the security model is correct, and the surface this introduces is well-understood. Items 1–5 are the ones I'd recommend addressing before promoting out of draft.

[scottwofford]

Claude-generated merge-queue triage of all open Luthien PRs, requested by Scott (Jul 7, 2026). Advisory only; Scott has not yet acted on these recommendations.

Recommendation: close in favor of #796, with the security findings here carried forward.

#796 delivers the multi-provider passthrough capture this PR pioneered, is mergeable against current main, and both PRs create the same passthrough_routes.py, so only one can land. The lasting value of this PR is its review trail: the query-param credential handling, hop-by-hop header handling, and body-size findings have been ported into the review checklist on #796, and the missing-auth gap flagged there is exactly the class of issue this PR's passthrough_auth.py addressed. Real credit to this PR for mapping the problem space; closing is about which branch is positioned to merge, not about the quality of the direction.