Security improvement: raise_for_status + ClientError prevent tokens from leaking

[JPFrancoia]

Describe the bug

When building a client like this:

client = aiohttp.ClientSession(raise_for_status=True)

or more generally, when using raise_for_status, any call to client.get, client.post, etc, can throw a ClientResponseError. By default this exception will include the server headers:

aiohttp/aiohttp/client_reqrep.py

Line 1007 in 2eccb8b

headers=self.headers,

.

If the request was authenticated (i.e: if the headers contained an Authorization field) and if the server sends back the headers, the token will be printed out in the exception.

This scenario just happened to me:

• I have an authenticated client that made a request to an API
• This API returned a 500
• The API returned some server headers, including the Authorization field
• An exception was thrown, with the headers printed out fully in clear
• Since my application logs exceptions, I now have an auth token in clear in my logs

This is problematic. I could do this to solve this problem for myself:

client = aiohttp.ClientSession(raise_for_status=my_own_function_that_ignores_headers)

But I think good security should be provided by default. I would argue that most requests these days are authenticated and that tokens should never be printed out by default. This could be a very sneaky problem, if some day an API decides to send back some headers with a token it could surprise people.

I see several approaches:

• Add a use_headers flag to the raise_for_status function, set to True at first (to not break compatibility), and set to False in a few versions
• Add an optional callable to filter which fields of the headers will be used/obfuscated

I am happy to submit a PR if people think it's a good idea.

To Reproduce

The server needs to send back server errors and the get/post needs to fail.

client = aiohttp.ClientSession(raise_for_status=True)
client.get(url)

Expected behavior

When a ClientError is raised, the headers shouldn't be present in the exception OR we should be able to disable them, to not leak tokens.

Logs/tracebacks

N/A

Python Version

$ python --version

3.12.1

aiohttp Version

$ python -m pip show aiohttp

3.9.5

multidict Version

$ python -m pip show multidict
6.0.5

yarl Version

$ python -m pip show yarl

1.9.4

OS

macOS

Related component

Client

Additional context

No response

Code of Conduct

• I agree to follow the aio-libs Code of Conduct

[Dreamsorcerer]

I'm not sure exactly where this should be changed. Probably in ClientResponseError or RequestInfo.

I also wonder if there shouldn't be a more generic way to mark things as sensitive (e.g. URLs could contain sensitive information). For example, Pydantic has SecretStr.

@webknjaz Any thoughts?

[webknjaz]

@Dreamsorcerer I also have a SecretStr in octomachinery (I think I borrowed it from environs).. It may make sense to have this in yarl even. We also don't know if the username portion is secret (some services use it for tokens). I know that some recommendations exist to have tokens in headers so that they don't hit the access logs, but that's an end-user thing.
The biggest problem is that we don't know upfront what the end-users treat as secret. Another thing to consider is that a lot of software still prints out secrets in verbose/debug modes.

@JPFrancoia I'd like to point out that the responsible way of raising any security-sensitive topics is outlined in the security policy, and it's not public: https://github.com/aio-libs/aiohttp/security/policy.

[JPFrancoia]

@webknjaz fair point, I wasn't sure if I was doing something wrong. I'll err on the side of caution and use the security-sensitive template next time, really sorry about that.

[Dreamsorcerer]

The biggest problem is that we don't know upfront what the end-users treat as secret

Exactly why I thought about a SecretStr, so the user can choose which things to hide from logs. For some users, some headers should not be logged, for others maybe the URL path or even domain should not be logged, etc.

I'd like to point out that the responsible way of raising any security-sensitive topics is outlined in the security policy, and it's not public: https://github.com/aio-libs/aiohttp/security/policy.

I think the wording of the title is bad. I read the issue, not as a security vulnerability in aiohttp, but as a feature request to allow users to improve security in their applications.

[JPFrancoia]

I amended the title. It's a bit tricky because the behaviour of aiohttp is surprising compared to other popular libs, which exclude the headers from their exceptions by default. There is nothing fundamentally wrong with how aiohttp works, but we need to make a conscious effort to prevent headers (and hence tokens) to leak. When debugging code using other libs, I had to voluntarily print the headers to see if the tokens (and other stuff) were correct. IMO the second situation is more fool proof.