Repopulate ~/.netrc auth. #1892
Conversation
| original_parsed = urlparse(resp.request.url) | ||
| redirect_parsed = urlparse(url) | ||
|
|
||
| if original_parsed.hostname != redirect_parsed.hostname: |
There was a problem hiding this comment.
What about:
if (original_parsed.hostname != redirect_parsed.hostname and
'Authorization' in headers):
del headers['Authorization']
|
One comment otherwise LGTM. You don't need to write a test for this, but I have an idea that I'd like to test it with. |
|
Done and done. |
|
|
|
I need to think about this. |
|
@kennethreitz we absolutely cannot continue reusing authorizations on redirects to sites that are not the same host. With that in mind we almost certainly need to issue a CVE. I'll happily work on that though. We've been leaking credentials and we need to at least address that. Whether we repopulate the auth after stopping the leak or not is more of a feature decision. I'm sure one of the security experts, like @dstufft would back up @Lukasa and I on that. |
|
@kennethreitz , call me the outsider but @sigmavirus24 got a point. As the new guy 'round these parts. You have 1.1million downloads. Even if half that still use, we are talking about a major security failure in the code base. 🙅 |
|
I'm happy to remove the repopulation of |
|
@Lukasa So in that sense, are you suggesting that the session itself may be invalidated and then a restart is required? |
|
No. =) All I was saying is that the previous version of this fix didn't respect the flag that controls whether we should look at The barest minimum we have to do is remove Authorization headers on redirects to new hosts. That's pretty much mandatory. Everything else is gravy. |
|
Oh well alrighty then! :) 👍 |
|
@Sirenity it isn't a big deal but requests should have far more than 8 million downloads last I checked (we're probably upwards of 9 million) and that's only from PyPI. We have no way of measuring how many downloads installations came from distribution repositories (i.e., people installing aptitude, yum, or pacman). |
|
@sigmavirus24 this was an explicit design decision, and it has been stated as such many times before. I've considered implementing a patch much like many times before, and this was a minor concern in the back of my mind when I did "the big refactor". As I said, I need to think about it. Further comments about are neither helpful nor welcome. :) |
|
Oof, I just noticed this. This really should change FWIW, carrying authentication data across access boundaries is a bad idea generally. It'd be up to Mitre but a CVE is probably appropriate as well. If you do get a CVE and you do change this, include the CVE number in the changelog please :] |
|
This simple patch is not a technical change, but a fairly large philosophical one. Traditionally, I have recommended that all security-concious users disable redirection support, and follow them on their own. This patch would make security the something that Requests does for you (something that I already did for the Python world when I verified SSL by default). This would take it to the next level. I'm not judging it as either good or bad. I just need need to think about it. Again, no further input needed. Will be back to you shortly. |
Repopulate ~/.netrc auth.
|
Also, if we could refactor this, it would be great. You ruined my beautiful code. :) |
|
\o/
|
|
I have a CVE Identifier for this when we release the next version. I also have one for the Proxy-Authorization exposure bug. |
|
What is the CVE? Useful for discussion even prior to release.
|
|
For this one: CVE-2014-1829 Edit Just got it from MITRE this morning so it may not show up in searches yet. |
|
I cleaned up the ugly code. :P |
|
We've learned a valuable lesson today: the easiest way to get Kenneth to write more code is to put really ugly code in front of him and let his natural instincts take over. ;) |
This should be a fix for #1885.
@sigmavirus24, can you give me some code review? =)