-
-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
Don't override Authorization
header when contents are bearer token (or any other token)
#3929
Comments
This behaviour can be overridden by |
Well, if you look at the issue of the social auth library, you may see that it's really nice to override the user/pass. If the |
So, I am open to that, but nervous about it. There are failure modes at either side, and trying to be "clever" may cause us to just be difficult to understand. The virtue of the model we have today is that it is very simple and consistent. So my question stands: do the functions currently available suffice for your use case? |
Well I agree on the fact that it's not really nice to change such behavior right now as it's always breaking something. And indeed the usage of the trust_env is a good option, but in this case the author of the library should give us an option to enable/disable it as a backend developer. Also the trust_env will disable more environmental settings or behavior and not only the netrc function, like proxy settings if I understood right. Another option would be turning it on/off per request that overrides the session trust_env, or have another way to not override one specific header. Anyway, the module is already trying to be "clever" by replacing the whole header 😃 . Which is great when you have full control over the |
So it is quite possible that the library wrapping requests should be setting |
Yep, and that should be the case. But it remains that if you have mixed requests, like I have, it's kinda hard to manage. You need to have two sessions. |
Well, you don't really. You can just flip the setting around as-and-when you need it. However, here's a framework I'd consider for handling auth in the 3.0 branch. I'd welcome a PR to make this the case.
Does that sound like a reasonable approach to your case? |
That sounds like a pretty clear way to solve this case. Only downside is that it can cause breaking code. But it's better to not force override when user manually given the details in my opinion. I'm not sure if I'm capable of doing the PR myself due to time limits. |
I encountered this problem when trying to figure out why the python-digitalocean module, which uses Requests, was failing due to an unexpected authentication error. The root cause turned out to be this
which I used, many years ago, to automate my anonymous FTP logins. I'm surprised that this directive in my So I like @Lukasa's idea above: when the caller specifies an Authorization header, I think the |
Got bitten by this one as well :(
I would say no, because |
It would be nice to allow to disable reading from |
How about provide a auth class something like |
Coming here after spending several hours debugging an issue which ended up being the presence of a ~/.netrc file. This behavior violates POLA and should be explicitly enabled rather than enabled by default. |
I lost half a day because I could not log to production any more, and I couldn't find the issue in our infrastructure. Found out it was because I stored my password in It should only happen with an explicit |
This issue still exists. |
I think this behavior should be changed. I installed an FTP package that silently generated a sample ~/.netrc. This prevented me from using gcloud (the GCP CLI) because gcloud uses Requests and Requests prefers ~/.netrc over gcloud's OAuth headers. I agree fullly with Lord-of-the-Galaxy's suggestion. |
I also just came across this behavior (based on a user report and it took us about a month to figure out that the presence of a |
I also spent a while debugging this exact problem, and I'll add my voice: requests should not override an explicitly set "Authentication" header. |
I tried in #6555, guys 😅 I essentially just implemented @Lukasa's suggestion, but I got slapped first with "It's documented!", followed by "Why aren't you writing an auth adapter instead of adding/modifying the header?", and finally "It's not backwards compatible so fuck off!". The standoffish attitude seems to be typical behavior for this maintainer so at least it's not personal. I would've been happy to discuss alternatives such as displaying a warning to the user when the Authorization header has been set and has been overridden by It's disappointing to say the least. |
**Fixed** - Static type checker not accepting **iterable\[str\]** for **data**. A fix in urllib3.future allows it since v2.1.902. - Unattended override of manually provided **Authorization** if `.netrc` existed with an eligible entry. Taken from closed PR psf#6555 and initially raised in psf#3929 **Added** - **oheaders** property in `Request`, and `PreparedRequest` in addition to `Response`.
**Fixed** - Static type checker not accepting **iterable\[str\]** for **data**. A fix in urllib3.future allows it since v2.1.902. - Unattended override of manually provided **Authorization** if `.netrc` existed with an eligible entry. Taken from closed PR psf#6555 and initially raised in psf#3929 **Added** - **oheaders** property in `Request`, and `PreparedRequest` in addition to `Response`.
I also struggled with this problem for about a day and had to dig deep into the http library to figure out that my .netrc file (which was valid but for another host was to blame). No warnings, nothing. The behavior doesn't makes sens, and is also inconsistent.
I think its fair to call this a bug, that has cos a lot of working hours for many people. In my case i need to keep the .netrc file for another API A workaround for the bug is something like this
So 3 lines of code, instead of one for the same operation. True, the trus_env variable is documented, but why not set it to false as default, or at very least write a warning f headers are ignored. |
I found out that the requests lib is overriding the authorization header when a netrc file is in place, which is awesome. But in some cases you won't want this at all, and is a design flaw imo.
For example you use a bearer token, it gets replaced by the user/password from netrc.
Also see the issue here: python-social-auth/social-core#43
The text was updated successfully, but these errors were encountered: