fix(logging): redact common credential aliases - #2491
Conversation
ErenAta16
left a comment
There was a problem hiding this comment.
Verified the premise on current main rather than taking it from the description, and the leak is real:
_redact_kwargs({"extra_headers": {...}}) on main:
x-goog-api-key -> 'secret' leaked
client_secret -> 'secret' leaked
access_token -> 'secret' leaked
refresh_token -> 'secret' leaked
api_key -> '[redacted]' already covered
Authorization -> '[redacted]' already covered
max_tokens -> 256 correctly untouched
token_budget -> 1000 correctly untouched
So all four added keys are genuinely exposed today, and the two non-secret controls confirm the change doesn't over-redact. Good that the tests assert both directions; test_redaction_preserves_non_secret_token_configuration is the one that stops someone "fixing" this later by substring-matching on token, which would swallow max_tokens.
The normalization (key.lower().replace("-", "_")) means each entry covers the hyphen and case variants for free, so X-Goog-Api-Key and x_goog_api_key both hit x_goog_api_key. The parametrized test exercises that, which is the right call since header casing is entirely up to the caller.
Keeping the strict allowlist rather than switching to substring matching is also the right trade-off for this function. Substring matching is what usually causes the other failure here, where max_tokens and token_budget start getting redacted and debug logs become useless.
One thing worth deciding rather than leaving implicit: the allowlist approach means anything not enumerated leaks, and there's still a sizeable set of names that show up in real extra_headers / provider kwargs:
password, secret, secret_key, private_key, session_token, id_token, bearer,
auth_token, apikey, aws_access_key_id, aws_secret_access_key, credentials
Not asking you to expand the list in this PR, scope creep on a security fix is its own risk. But it's worth a follow-up issue, because "add names as we notice them" has now happened twice and the failure mode is silent, a credential is only found in the logs after it's already been written to them. A middle ground that keeps the false-positive protection would be matching on a small set of suffixes/stems (_secret, _token, _key, password) with an explicit deny-list for the known-benign ones (max_tokens, token_budget, *_key schema fields), though that needs care and probably its own discussion.
Change as written is correct and I'd take it. LGTM.
|
Consolidated and shipped in #2495. Closing this focused patch as superseded; thank you for the contribution. |
What
Extend v2 request-log redaction to cover common OAuth and Google API
credential aliases in nested request kwargs.
Why
Keys such as
x-goog-api-key,client_secret,access_token, andrefresh_tokencurrently remain visible in DEBUG logs. This can exposecredentials to persistent log sinks.
Fixes #2490
Changes
hyphen variants, input immutability, and non-secret token configuration.
The implementation intentionally avoids substring matching so fields such as
max_tokensandtoken_budgetare not unnecessarily hidden.Testing
pytest tests/coverage/test_core_response_coverage.py tests/processing/test_process_response.py -q31 passedpytest tests/v2 -q1569 passed, 171 skippedruff check instructor/v2/core/response.py tests/coverage/test_core_response_coverage.pyruff format --check instructor/v2/core/response.py tests/coverage/test_core_response_coverage.pyty check --python O:\.venv-py311 instructor/v2/core/response.py tests/coverage/test_core_response_coverage.pygit diff --check