fix: redact API key from provider error output (#91)#92
Conversation
When an API key carried a character invalid in an HTTP header — most commonly a trailing CR from a CRLF .env on Windows/WSL — the OpenAI and Anthropic providers interpolated the underlying exception into a ProviderError. That exception's message embeds the raw Authorization / x-api-key header value, so the key was printed in cleartext to the terminal and any CI logs. Two layers of defence: - Strip surrounding whitespace from the key on read, in both providers. This fixes the common CRLF-.env trigger outright: the header becomes valid and the request proceeds normally. - Add scrub_secrets() in providers/base.py and route provider request errors through it, redacting any credential header value (and its printable segments, since an exception repr escapes control chars) from the message. Backstops other malformed-key cases that survive strip(). Tests cover the CRLF-strip behaviour, scrub_secrets directly, and an end-to-end assertion that a key with an embedded newline never appears in the raised OpenAI/Anthropic error. The npm runtime is unaffected by this path (it never interpolates the header value into its error). Closes #91
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
aswhitehouse
left a comment
aswhitehouse
left a comment
There was a problem hiding this comment.
Real bug, right fix shape — strip on read + redact on error is the correct two-layer defence, and catching the repr()-escaped control-char gap mid-development is exactly the kind of thing that makes this stick. CI green. A few items before merge:
Should-fix
1. Changelog structure. Inserting ### Security between Spec bullets orphaned the conformance README note under Security (line 18 now sits under the Security heading). Move that bullet back under ### Spec.
2. Exception __cause__ still holds the raw key. After scrubbing you still do raise ProviderError(...) from exc (openai_http.py ~263, and the Anthropic equivalents). The CLI path prints str(OARunError) so the happy path is safe, but any full traceback (uncaught, --tb=long, logging that walks __cause__) will still print the original ValueError with the cleartext header. Prefer raise ProviderError(...) from None once the message is scrubbed.
3. End-to-end Anthropic assertion is weak for the escaped-repr case. bad_key contains a real newline; after repr() escaping that exact string is never in the message anyway, so assert bad_key not in str(exc) (line 296) can pass even if scrubbing failed. Assert the printable segments: "sk-ant-abc" / "def-embedded-newline" (and tighten the OpenAI test the same way instead of the soft "abc" not in msg or REDACTED in msg).
Nits (non-blocking)
- npm
.trim()follow-up — agreed there's no leak today, but trimming on read is cheap hygiene. len(variant) >= 4— fine; just be aware"Bearer"itself gets redacted as a segment (harmless).
Happy to approve once 1–3 are sorted.
|
|
||
| ### Security | ||
| - **API key no longer leaked in provider error output** — when an API key carried an invalid header character (most commonly a trailing `\r` from a CRLF `.env` on Windows/WSL), the OpenAI and Anthropic providers interpolated the underlying exception — whose message embeds the raw `Authorization` / `x-api-key` value — straight into a user-facing `ProviderError`, printing the key in cleartext to the terminal and any CI logs. Keys are now stripped of surrounding whitespace on read (fixing the common CRLF trigger outright), and any credential header value is redacted from provider error messages as a defence-in-depth backstop for other malformed-key cases. The npm runtime was not affected by this path. | ||
| - Conformance README notes that cases pin the **minimum** `open_agent_spec` version they require, not the suite version. |
There was a problem hiding this comment.
This Spec bullet about the conformance README note got pulled under ### Security when the section was inserted. Move it back under ### Spec so Security only contains the credential-leak fix.
| except Exception as exc: | ||
| raise ProviderError(f"OpenAI request failed: {exc}") from exc | ||
| msg = scrub_secrets(str(exc), all_headers) | ||
| raise ProviderError(f"OpenAI request failed: {msg}") from exc |
There was a problem hiding this comment.
After scrubbing, prefer from None over from exc. Chaining preserves the original ValueError (with the cleartext header) on __cause__, so any full traceback still leaks the key even though str(ProviderError) is clean. Same change needed on both Anthropic sites.
| "endpoint": "https://api.anthropic.com/v1/messages", | ||
| }, | ||
| ) | ||
| assert bad_key not in str(exc_info.value) |
There was a problem hiding this comment.
bad_key contains a literal newline, so after the exception repr() escapes it this assertion is almost vacuously true. Assert the printable segments scrubbing must remove, e.g. assert "sk-ant-abc" not in msg and assert "def-embedded-newline" not in msg (and the same tightening on the OpenAI test above).
3 participants
What changed
Fixes the credential leak reported in #91: the OpenAI and Anthropic providers printed the API key in cleartext when the key contained an invalid HTTP-header character.
Two layers of defence:
\rfrom a CRLF.env(the common Windows/WSL case) is trimmed, so the header is valid and the request just proceeds. Fixes the reported trigger outright.scrub_secrets()inproviders/base.py; provider request-failure paths route their message through it, removing anyAuthorization/x-api-keyvalue (and its printable segments — an exceptionreprescapes control chars, so the raw value alone wasn't enough) before it reaches the user. Backstops any other malformed-key case that survives.strip().Why
When a key had a stray header-invalid character, the providers did:
exchere isValueError: Invalid header value b'Bearer sk-...\r'— the message embeds the key. It landed in the terminal error panel (and any CI log / pasted bug report) in cleartext. CRLF.envfiles are common on Windows/WSL, so this was reachable by accident.How tested
pytest tests/— 511 passed, 3 skipped)New tests in
tests/test_providers.py::TestCredentialRedaction(+ a CRLF-strip case inTestOpenAIProviderAuth):scrub_secretsredactsBearerandx-api-keyheader values, and leaves non-secret headers untouchedBearer <key>header.strip(), so it exercises the redaction path) never appears in the raisedProviderError— asserted for both OpenAI and Anthropic, fully offlineWriting that end-to-end test caught a real gap mid-development: the first redaction pass missed the key because the exception
represcapes the newline (\n→ literal backslash-n), so the raw value no longer matched. The finalscrub_secretsalso redacts the value's printable segments, which do appear verbatim.Scope
oas_cli/providers/{openai_http,anthropic_http,base}.py.res.status+ response body intoOAError, never the header value. (A parity.trim()on the key there is a reasonable low-priority follow-up, but there's no leak to fix.)Type of change
Checklist
ruff check . && ruff format --check .)[Unreleased]→ Security