fix(logs): redact sensitive headers #1850
Conversation
src/openai/_utils/_logs.py
Outdated
| if isinstance(record.args, dict) and "headers" in record.args: | ||
| if isinstance(record.args["headers"], dict): | ||
| logged_headers: Dict[str, Any] = record.args["headers"] | ||
| for header in logged_headers: | ||
| if header.lower() in ["api-key", "authorization"]: | ||
| logged_headers[header] = "<redacted>" |
There was a problem hiding this comment.
nit: we have an is_dict() header that would make this slightly cleaner imo
| if isinstance(record.args, dict) and "headers" in record.args: | |
| if isinstance(record.args["headers"], dict): | |
| logged_headers: Dict[str, Any] = record.args["headers"] | |
| for header in logged_headers: | |
| if header.lower() in ["api-key", "authorization"]: | |
| logged_headers[header] = "<redacted>" | |
| if is_dict(record.args) and "headers" in record.args and is_dict(record.args["headers"]): | |
| headers = record.args["headers"] | |
| for header in headers: | |
| if str(header).lower() in ["api-key", "authorization"]: | |
| headers[header] = "<redacted>" | |
and from ._utils import is_dict.
also a minor micro optimisation but I think we should extract the list of headers, e.g.
SENSITIVE_HEADERS = {"api-key", "authorization"}There was a problem hiding this comment.
also I haven't actually written a log filter before, is it bad that this mutates the original headers arg? i.e. should it be this?
headers = record.args["headers"] = {**record.args["headers"]}There was a problem hiding this comment.
Updated with your revisions, thanks! The logging docs for filter permits in-place modification:
If deemed appropriate, the record may be modified in-place.
RobertCraigie
changed the title
tests/lib/test_azure.py
Outdated
|
|
||
|
|
||
| @pytest.mark.respx() | ||
| def test_azure_api_key_redacted(respx_mock: MockRouter, _logger_with_filter: logging.Logger, caplog: pytest.LogCaptureFixture) -> None: |
There was a problem hiding this comment.
note: I think you could've avoided the _logger_with_filter stuff by putting all these tests in a class and then doing
@pytest.fixture(autouse=True)
def _logger_with_filter() -> logging.Logger: ...but totally not worth blocking on that change
There was a problem hiding this comment.
Gotcha. Moved the tests in a class, however ruff still complains about the unused logger function, so kept it with underscore prefix.
There was a problem hiding this comment.
ah sorry tbc I meant that the class would just be for the logging tests as I assumed we wouldn't want the fixture to be setup for the other tests but if that would be fine then you don't need a class at all and it can just be at the module level
There was a problem hiding this comment.
Oh! I completely misunderstood, my bad 😆. Updating...
|
ugh looks like mypy doesn't understand the type narrowing, feel free to just tell mypy to ignore that whole file. (that config is in |
Changes being requested
Adds a logging filter to the base client logger which redacts API keys and tokens from debug logs.
Additional context & links
closes #1082