resolve_redirects no longer throws an InvalidSchema exception when the scheme is uppercase

I had a case where the Location header had a URL with the scheme in uppercase, which caused an InvalidSchema exception.


First, thanks for reporting this and particular thanks for supplying code as well! Very awesome. =D

Interestingly, we're not spec compliant here. I dove into RFC 2616 looking for a reason to blame the web server admin, but I stumbled upon this (emphasis mine):

> When comparing two URIs to decide if they match or not, a client SHOULD use a case-sensitive octet-by-octet comparison of the entire URIs, with these exceptions:
> - A port that is empty or not given is equivalent to the default port for that 
>   URI-reference;
> - **Comparisons of host names MUST be case-insensitive**;
> - **Comparisons of scheme names MUST be case-insensitive**;
> - An empty abs_path is equivalent to an abs_path of "/".

So I'm +1 on this _idea_, and I actually think we need to extend it to the hostname portion as well.

As for the actual PR, I think it can be improved. =) It would be really helpful to have a test (or even some tests) for this. Also, we should extend it to handle hostnames too (if possible). However, I think the real place to fix this is in the test for the InvalidSchema exception. If we're allowed to send HTTP:// as the scheme, then we should allow that everywhere, not just on redirect.


I added tests, which is good because I wasn't taking into account URLs without a scheme. So it should actually work now beyond my original test case. :)

It looks like httplib is handling uppercase hostnames, so I'm not sure there's a need to address that. I tried your suggestion of moving the fix to get_adapter(), but that creates a whole bunch of other problems with the pool manager expecting the scheme to be lowercase. One of the tests creates a GET request with an uppercase scheme and it's handled properly. So I think this is only an issue with resolve_redirects and the Location header. What do you think?

Also, I updated httpbin with a new endpoint for testing this issue (https://github.com/rcarz/httpbin). If you're happy with my changes, I can submit a pull request for that too.


Fair enough, if moving the fix doesn't work then it doesn't work. :smile:

I like the httpbin endpoint, and I think you should submit a PR for it to that project. As for the tests, they're great. One note: the second test should probably assert on the return code too.

Otherwise, this is perfect. =)


Awesome! Can you do a quick rebase so we can merge it in? :)


Great! I added the assertion and rebased.


Heh it's failing because we're using a non-existent endpoint for httpbin.


There's a PR for it: https://github.com/kennethreitz/httpbin/pull/89


I rebased again.


@kennethreitz Can you restart https://travis-ci.org/kennethreitz/requests/builds/7518571 ? It appears I can't do it without making another commit.


I'm not sure I follow. Per http://www.ietf.org/rfc/rfc2965.txt hostname matching should be case-insensitive. Is that not how it's behaving today? A quick test with uppercase hosts seems to work.

I'm curious the conditions under which #1385 are needed. For example, this prints 200 for me with no changes to requests:

``` python
import requests

r = requests.get('HTTP://www.google.com')
print r.status_code
```

My patch addresses uppercase schemes in the Location header, so it only affects redirects. I did try what @ViktorHaag has proposed, but I seem to remember having issues with a pool manager?


@rcarz That's odd, vanilla requests throws exceptions when I do a get on `HTTP://www.google.com/`. TravisCI does too.

I'm not sure you're right, @Anorov. As @rcarz points out, this change here only affects redirects. You can see this by looking at the Travis CI output for this change, where a test with upper-case scheme fails.

#1385 is more comprehensive than this fix. I need to sit down and confirm whether #1385 covers all cases this fix does.


@Lukasa Ah yeah, you're right, sorry. I only looked at the changes and not the full file.

@rcarz Web servers and DNS servers should always be treating hostnames case insensitively, yes. So it doesn't really matter if `requests` lowercases it or not. I think it would just go along well, aesthetically, if the scheme is also lowercased.

Either way, I think this should all be resolved with one pull (whether it be this one or the other, I'm not sure). I think all schemes should be strictly lowercased early on, before processing any further.

If you place the changes made in this commit, essentially `url = '%s://%s' % (scheme.lower(), uri)`, somewhere early on in `Session.send`, I think every bird would be killed with one stone (so to speak). Anyone disagree?