-
Notifications
You must be signed in to change notification settings - Fork 2
fix: redact API key from provider error output (#91) #92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ | |
| from oas_cli.tool_providers.base import InvokeResult, ToolCall | ||
| from oas_cli.usage import from_openai | ||
|
|
||
| from .base import IntelligenceProvider, InvokeOutcome, ProviderError | ||
| from .base import IntelligenceProvider, InvokeOutcome, ProviderError, scrub_secrets | ||
|
|
||
| # Default to the Chat Completions API; set intelligence.endpoint in your spec | ||
| # to switch to the Responses API (https://api.openai.com/v1/responses) or a | ||
|
|
@@ -51,7 +51,8 @@ def invoke_verbose( | |
| history: list[dict[str, Any]] | None = None, | ||
| ) -> InvokeOutcome: | ||
| api_key_env: str | None = config.get("api_key_env", "OPENAI_API_KEY") | ||
| api_key: str | None = os.environ.get(api_key_env) if api_key_env else None | ||
| raw = os.environ.get(api_key_env) if api_key_env else None | ||
| api_key: str | None = raw.strip() if raw else raw | ||
|
|
||
| # Require a key only when the config explicitly names an env var and it's absent. | ||
| # Local / anonymous endpoints (api_key_env=None or "") skip this check. | ||
|
|
@@ -106,7 +107,8 @@ def invoke_with_tools( | |
| config: dict, | ||
| ) -> InvokeResult: | ||
| api_key_env: str | None = config.get("api_key_env", "OPENAI_API_KEY") | ||
| api_key: str | None = os.environ.get(api_key_env) if api_key_env else None | ||
| raw = os.environ.get(api_key_env) if api_key_env else None | ||
| api_key: str | None = raw.strip() if raw else raw | ||
| if api_key_env and not api_key: | ||
| raise ProviderError( | ||
| f"API key not set — export {api_key_env} or set " | ||
|
|
@@ -257,7 +259,8 @@ def _http_post_raw( | |
| detail = exc.read().decode(errors="replace") | ||
| raise ProviderError(f"OpenAI HTTP {exc.code}: {detail}") from exc | ||
| 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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After scrubbing, prefer |
||
|
|
||
|
|
||
| def _extract_text(data: dict, url: str) -> str: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -210,6 +210,91 @@ def test_error_when_key_env_set_but_missing(self): | |
| config={"model": "gpt-4o", "api_key_env": "MISSING_KEY_ENV_XYZ"}, | ||
| ) | ||
|
|
||
| def test_crlf_key_is_stripped_not_leaked(self): | ||
| """A CRLF-terminated key (Windows/WSL .env) must be trimmed, not sent raw. | ||
|
|
||
| Regression for the key-leak bug: the trailing ``\\r`` made an invalid | ||
| header, and the resulting error echoed the key in cleartext. The key is | ||
| now stripped on read, so the header is valid and the value carries no | ||
| stray whitespace. | ||
| """ | ||
| headers = self._make_provider_call( | ||
| api_key_env="OPENAI_API_KEY", | ||
| env_vars={"OPENAI_API_KEY": "sk-test-crlf\r\n"}, | ||
| ) | ||
| assert headers["Authorization"] == "Bearer sk-test-crlf" | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Credential redaction in provider errors (issue #91) | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| class TestCredentialRedaction: | ||
| SECRET = "sk-proj-AbCdEf0123456789SecretKeyValue" | ||
|
|
||
| def test_scrub_secrets_redacts_bearer_header(self): | ||
| from oas_cli.providers.base import scrub_secrets | ||
|
|
||
| headers = {"Authorization": f"Bearer {self.SECRET}", "Content-Type": "x"} | ||
| # Message as an exception repr renders it — trailing char escaped. | ||
| msg = f"Invalid header value b'Bearer {self.SECRET}\\r'" | ||
| out = scrub_secrets(msg, headers) | ||
| assert self.SECRET not in out | ||
| assert "***REDACTED***" in out | ||
|
|
||
| def test_scrub_secrets_redacts_x_api_key_header(self): | ||
| from oas_cli.providers.base import scrub_secrets | ||
|
|
||
| headers = {"x-api-key": self.SECRET} | ||
| out = scrub_secrets(f"bad header b'{self.SECRET}'", headers) | ||
| assert self.SECRET not in out | ||
|
|
||
| def test_scrub_secrets_leaves_non_secret_headers_untouched(self): | ||
| from oas_cli.providers.base import scrub_secrets | ||
|
|
||
| headers = {"Content-Type": "application/json"} | ||
| msg = "some error mentioning application/json" | ||
| assert scrub_secrets(msg, headers) == msg | ||
|
|
||
| def test_openai_provider_error_does_not_leak_key(self): | ||
| """End-to-end: an un-strippable illegal char in the key must not leak. | ||
|
|
||
| An embedded newline survives ``.strip()``, so header construction still | ||
| fails — this exercises the ``except`` redaction path, offline (the error | ||
| is raised while building the request, before any socket I/O). | ||
| """ | ||
|
|
||
| bad_key = "sk-proj-abc\ndef-embedded-newline" # mid-string \n survives strip() | ||
| with patch.dict("os.environ", {"OPENAI_API_KEY": bad_key}, clear=False): | ||
| with pytest.raises(ProviderError) as exc_info: | ||
| OpenAIProvider().invoke( | ||
| system="s", | ||
| user="u", | ||
| config={ | ||
| "model": "gpt-4o", | ||
| "endpoint": "https://api.openai.com/v1/chat/completions", | ||
| }, | ||
| ) | ||
| msg = str(exc_info.value) | ||
| assert bad_key not in msg | ||
| assert "abc" not in msg or "***REDACTED***" in msg | ||
|
|
||
| def test_anthropic_provider_error_does_not_leak_key(self): | ||
|
|
||
| bad_key = "sk-ant-abc\ndef-embedded-newline" | ||
| with patch.dict("os.environ", {"ANTHROPIC_API_KEY": bad_key}, clear=False): | ||
| with pytest.raises(ProviderError) as exc_info: | ||
| AnthropicProvider().invoke( | ||
| system="s", | ||
| user="u", | ||
| config={ | ||
| "model": "claude-3-5-sonnet-20241022", | ||
| "endpoint": "https://api.anthropic.com/v1/messages", | ||
| }, | ||
| ) | ||
| assert bad_key not in str(exc_info.value) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # CustomProvider | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This Spec bullet about the conformance README note got pulled under
### Securitywhen the section was inserted. Move it back under### Specso Security only contains the credential-leak fix.