Skip to content

fix(client): don't KeyError when a proxy is configured#555

Open
RapidPoseidon wants to merge 1 commit intomainfrom
poseidon/fix-proxy-keyerror
Open

fix(client): don't KeyError when a proxy is configured#555
RapidPoseidon wants to merge 1 commit intomainfrom
poseidon/fix-proxy-keyerror

Conversation

@RapidPoseidon
Copy link
Copy Markdown
Contributor

Summary

Any user behind a corporate proxy is hard-bricked on the first SDK call.

```python
client_kwargs = {"verify": ..., "limits": limits} # no "headers"

if self.configuration.proxy:
client_kwargs["proxy"] = self.configuration.proxy
existing_headers = client_kwargs.pop("headers") # KeyError: 'headers'
```

client_kwargs is built literally above without a \"headers\" key, so the pop always raises.

Fix

  • Use client_kwargs.get(\"headers\") or {} as the base.
  • Only write the merged dict back when there are proxy_headers to merge.
  • httpx doesn't have a dedicated proxy_headers kwarg (that's a urllib3 API), so folding into default request headers is the closest equivalent — kept as a comment.
  • Mirrored into openapi/templates/rest.mustache so regen preserves the fix.

Test plan

  • uv run pyright src/rapidata/rapidata_client → 0 errors
  • Manual trace through both branches: proxy without headers, proxy with headers, no proxy.
  • Live verification via HTTPS_PROXY=http://… run (don't have a proxy handy in this env).

🔗 Session: https://session-bc38cc85.poseidon.rapidata.internal/

`_get_session_defaults` built `client_kwargs` with only `verify` and
`limits`, then inside the `if self.configuration.proxy` branch called
`client_kwargs.pop("headers")` — which always raised `KeyError: 'headers'`
because the key had never been inserted. Any user behind a corporate
proxy was hard-bricked on the first call.

Switch to `client_kwargs.get("headers") or {}` and only set the merged
dict when there are proxy_headers to merge. `proxy` itself was fine; the
only broken path was the headers merge.

Also mirrored into `openapi/templates/rest.mustache`.

Session: https://session-bc38cc85.poseidon.rapidata.internal/

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: lino <lino@rapidata.ai>
@claude
Copy link
Copy Markdown

claude Bot commented Apr 24, 2026

Code Review

Overview

This PR fixes a regression that hard-bricks any user behind a corporate proxy. The root cause is clear: client_kwargs is built with only verify and limits keys, so the unconditional client_kwargs.pop("headers") always raises KeyError. The fix is minimal and well-targeted.


What's Good

  • Minimal blast radius — 12 additions, 8 deletions, no unrelated changes.
  • Mustache template updated — follows the CLAUDE.md requirement to mirror generated-client changes back to the template.
  • Behavioural improvement — the original code (had it worked) would have unconditionally written an empty headers dict back into client_kwargs even when no proxy headers existed. The new code skips that entirely, which is cleaner.
  • Safe copydict(client_kwargs.get("headers") or {}) avoids mutating any shared state.

Issues & Suggestions

⚠️ Security: proxy headers will be sent to the origin server

existing_headers = dict(client_kwargs.get("headers") or {})
existing_headers.update(self.configuration.proxy_headers)
client_kwargs["headers"] = existing_headers

proxy_headers typically carries Proxy-Authorization. Merging it into the default request headers means it will be forwarded to the destination API server on every request, not just during the proxy CONNECT handshake. For HTTPS connections this leaks proxy credentials to the origin.

The PR description acknowledges this ("closest equivalent — kept as a comment"), but it's worth a deliberate decision: is this acceptable given the SDK's usage context? If proxy auth headers are a real use-case, a more correct approach in httpx is a custom Auth class or per-mount transport. If proxy_headers is rarely populated in practice (most proxies are unauthenticated), the security risk is low, but the comment could be strengthened to warn callers.

Minor: dict() wrapper is redundant

client_kwargs.get("headers") or {} already produces a fresh {} when there's no "headers" key, so wrapping in dict() creates an unnecessary second copy. Not a bug, but:

# Simpler
existing_headers = dict(client_kwargs.get("headers") or {})
# vs.
existing_headers = client_kwargs.get("headers", {}).copy()

Either is fine; .copy() or dict() make intent explicit. Small nit.

Minor: no automated test coverage

The test plan marks live proxy verification as incomplete. A unit test mocking self.configuration with proxy="http://proxy" and proxy_headers={"X-Custom": "val"} (and without proxy_headers) would prevent this class of regression from silently re-entering. Worth a follow-up ticket if not feasible here.


Summary

The fix is correct and the right change to make. The only real concern worth a decision before merge is the security note about proxy credentials being forwarded to the origin server — whether that's acceptable depends on how proxy_headers is used in practice. Everything else is nitpick-level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant