Raibek/requests-github-issues huggingface dataset:
Removed unnecessary if-statements

Passes all unit tests like this.


Those conditionals are 100% necessary. `verify` can be (and frequently is) `False` or a path to a certificate bundle. We must conditionally set the values. Consider the paths through this code: 
1. `verify` is `True`: then `cert_loc` will be the default bundle
2. `verify` is `False`: then `cert_loc` will be `False`
3. `verify` is `/path/to/other/bundle`: then `cert_loc` will be `/path/to/other/bundle`

This code breaks a great deal of features that are apparently untested. This is more of an alarm that those features are not tested rather than a reason for removing the conditionals here. Thanks for your help @benediktkr !
Requests can't handle HTTPS proxy requests.

Consider this snippet of code:

``` python
import requests
from requests.auth import HTTPProxyAuth

auth = HTTPProxyAuth('username', 'password')
proxy = {'https': 'https://192.168.0.1/'}
req = requests.get('http://www.google.com', proxies=proxy, auth=auth)
```

What I expect the code to do is to send an encrypted request (as implied by http_S_ to the proxy URL (192.168.0.1), the payload of which is the URL I want to retrieve (www.google.com).

What Requests does instead is send a query directly to the target (Google), with the proxy credentials, Base64 encoded, in the header! The request does not go near the proxy, which you can check by entering invalid credentials in the snippet above (which should result in some 40\* error, but passes silently.)

If the proxy URL is changed to "HTTP", the transfer works as expected (credentials sent to proxy, proxy fetches target URL), only the conversation is not encrypted.

This was tested under Requests v1.2.0, and the latest version, 2.2.1

Am I just using the module incorrectly, or is this a bug in Requests?


The answer is complicated, let me address it in stages.

First, you're misusing the library. Consider the proxies dictionary. It has this form: `{scheme: proxyURL}`. When you made a request, we look at the scheme for the URL you've asked for, and see if you've asked for proxies on that scheme. In your example above, you've said you want all HTTPS requests proxied, but not HTTP requests. This leads to us not routing via the proxy (since you didn't ask for one). If you want to avoid accidentally leaking your auth, you should place it in the proxy URL: that is, for your current case, your proxy dict should be:

``` python
proxy = {'http': 'https://username:password@192.168.0.1/'}
```

Your other issue (about Requests not making HTTPS connections to your proxy) is discussed at phenomenal length in #1622. The current state of play is: Requests never makes TLS connections to proxies. This is in line, roughly, with what browsers are doing (from the Squid documentation [here](http://wiki.squid-cache.org/Features/HTTPS)):

> Unfortunately, popular modern browsers do not permit configuration of TLS/SSL encrypted proxy connections. There are open bug reports against most of those browsers now, waiting for support to appear. If you have any interest, please assist browser teams with getting that to happen.

I am open to a feature request for making connections via HTTPS to proxies, but it's not a priority at this time, unless you can provide a compelling reason it ought to be. =)


Wow! That was a very quick, and helpful, response. As I suspected, I was using requests incorrectly. Thanks for clearing that up for me, and apologies for wasting your time...


That's not a problem at all. =) In the future, if you're worried about whether a question is a bug or simply a misunderstanding, you can email either myself or @sigmavirus24. =) We both publish our email addresses on GitHub, and we're always happy to take questions.
One last Charade reference to remove here.


This is an obvious :+1:. =D


This amounts to essentially a documentation change. I don't see why it shouldn't be merged immediately (or as soon as the build passes).


Looks like the CI server is having a tough time, I haven't seen any of these builds pass.


It notifies people in IRC right? Freenode has been having some trouble since last night. It could be that the server is blocked trying to connect to IRC to notify of older jobs that have passed/failed.


Anyway, the build is an irrelevance here.
Reinstate falling back to self.text for JSON responses

A JSON response that has no encoding specified will be decoded with a detected UTF codec (compliant with the JSON RFC), but if that fails, we guessed wrong and need to fall back to charade character detection (via `self.text`). Kenneth removed this functionality (by accident?) in 1451ba0c6d395c41f86da35036fa361c3a41bc90, this reinstates it again and adds a log warning.

Fixes #1674


Hooray! :+1: :cake:


> Nowhere else in the file is logging used. Can you remove this line and the import at the top of the file please?

Rest of the `logging` references excised in #1902. 


Thanks @mjpieters! :cake: 


Lets kick the CI server and get this rebuilt shall we?


@Lukasa for the record, i only got notified when someone directly mentions me ;)


Looks like they're all stuck on the IRC notifier. Sigh.


(close/open for build retrigger, sorry)
PEP8 Compliance

I checked `PEP8` compliance using following command, 

```
pep8 --statistics --ignore=E501 --exclude='requests/packages/*' requests test_requests.py
```

The stats are 
- 2        **E125** continuation line does not distinguish itself from next logical line
- 6        **E126** continuation line over-indented for hanging indent
- 30      **E128** continuation line under-indented for visual indent
- 1        **E203** whitespace before ':'
- 2        **E226** missing whitespace around arithmetic operator
- 2        **E23** missing whitespace after ','
- 2        **E241** multiple spaces after ','
- 22      **E251** unexpected spaces around keyword / parameter equals
- 1        **E261** at least two spaces before inline comment
- 3        **E303** too many blank lines (2)
- 1        **W293** blank line contains whitespace

I plan to make `requests` more `PEP8` compliant, and I am wondering which errors should I ignore beside **E501**.


This has been proposed in the past and @kennethreitz vastly dislikes most of pep8 from my experience. I've never been able to tell exactly what style he likes to follow but it's very much his call. Until he can get around to replying to this, I would hold off on your efforts so you do not expend too much energy.

FWIW, As the maintainer of Flake8 as well as this project, I appreciate your desire to make this project more compliant.


Correct. At Requests we view PEP8 more as [a set of guidelines](http://youtu.be/b6kgS_AwuH0). I wouldn't get too invested in this idea. =)


Ok, let's wait @kennethreitz's rely. It is interesting that he does't like PEP8 style.


There are things he likes, there are things he doesn't. =)


You should talk to some of the folks who maintain Twisted about their feelings on PEP8. :wink: 


Haha, that reminds me Shakespeare's famous remark, **There are a thousand Hamlets in a thousand people's eyes**

I will wait @kennethreitz's reply and see what I should do next. 

BTW: I think @kennethreitz should come up with a style guide for `requests` if he doesn't like pep8. 


He's begun a similar project here: http://docs.python-guide.org/en/latest/


@rattrayalex sorry?


While it's not a style guide for requests specifically, @kennethreitz 's guide linked above documents many of his opinions on python style in general. Though he does seem to say nicer things about PEP8 there: http://docs.python-guide.org/en/latest/writing/style/#pep-8


You may find observing the [blames on the PEP8 section](https://github.com/kennethreitz/python-guide/blame/master/docs/writing/style.rst) of interest.


Ah! Not a lot of @kennethreitz in that ;-P
(tl;dr, none of the recent blames are his)
Thanks!


Since this is highly unlikely to be done, I'm closing this. Thanks everyone for your interest.
Made .history type consistent (tuples only)

An empty history was represented by an empty list whereas a non-empty history was a tuple.
Now the type of .history is always a tuple, so it does not break code such as `req.history + (req,)`.


Six test cases failed. See details on this [page](http://ci.kennethreitz.org/job/requests-pr/PYTHON=3.3/lastBuild/console)

```
>           _r.history.append(r)
E           AttributeError: 'tuple' object has no attribute 'append'

requests/auth.py:179: AttributeError
```


This is not a totally unreasonable change, but you've fixed it in the wrong place. If you look at `sessions.py` line 533, you can see where we convert this to a tuple.


The exact line that @Lukasa is referring to is [here](https://github.com/kennethreitz/requests/blob/master/requests/sessions.py#L533). Just out-dent that line and you'll be fine. Please revert the change you've made here before making this change. Also please continue working on this PR and do not open a new one @Zopieux 

Please leave a comment when you've updated the PR.


This breaks the codebase :)


I am sorry I was not reactive enough to submit a working PR. I am pleased to see that other open PRs  include this bugfix!
PreparedRequest.prepare_body should not add a Transfer-Encoding header when manually supplied with a Content-Length

I need to use a PUT request to upload sys.stdin to Amazon S3, but since S3 does not support chunked transfer encoding I have to supply a `Content-Length` header myself.  Doing so seemed to successfully prevent HTTPAdapter.send from trying to do chunking, but S3 rejected the request anyway because PreparedRequest.prepare_body noticed that the body was iterable and set a `Transfer-Encoding` header on its own.  A user-supplied `Content-Length` should prevent the latter from happening as well.


Thanks for raising this!

There's an open issue that contains a substantial discussion on this point, #1648. The short of it, however, is that we almost never expect people to provide their own Content-Length header.

I have a question though: how are you able to set Content-Length when reading from sys.stdin?


Closing to centralize discussion on #1648.
Avoid breaking crappy distribution methods.

Apparently RPM doesn't like us having the full license text in the 'license' section, as in #1878. Seems innocuous to change this, because realistically who the hell cares?


LGTM :+1: 


Before, this was important because it was a lovely feature of crate.io. 

Long live crate.io.


One day, crate.io will return as pypi.python.org. Unless @dstufft explodes in a ball of caremad.


Oh, but he _is_ a ball of caremad! That's his secret power.
Add request to RequestException

Pass request objects in `HTTPAdapter`.

Fixes #1890


LGTM, I'm happy with this. :cake:


Uh, maybe.


@kennethreitz I have no strong opinion on this either way frankly. #1890 seemed like a reasonable feature request though. The decision is all yours.


In the context of grequests, I think this will be nice. Let's just not make a habit of this :)


I'm assuming this is the PreparedRequest. 


It is. =)


@kennethreitz you assume correctly. And this was a one time idea anyway. It seemed reasonable enough to allow it.
include Request object as attribute on RequestExceptions

It'd be handy if you could check RequestException.request to find out about the request that caused the error.


Isn't the response included? That should have the request on it already.


Whoops. That's only for `HTTPError`s. The issue is that you do not necessarily always have a request object that can be passed in. I'm working on a PR to add the attribute when possible. (Same for response objects.)
Fix for 301 redirect and latest PyOpenSSL.

Fixes #1887


This seems like a reasonable fix to me. Unfortunately, the unit test is not a particularly good one because most people don't use the SNI build of Requests in their development systems. Additionally, we don't do it in our CI server (though we probably should, I'll bring it up with @kennethreitz). Leave it there for now: if we can get it into the CI server it'll be fine, otherwise we'll need another test.


Yeah I figured you would say that. Makes sense actually. Funny thing is the main reason I get this is trying to use a SNI enabled private pypi server from my CI server. One CI server needs another :-)
301 redirect broken with latest pyopenssl/SNI

With the latest pyopenssl on Windows 64bit:

```
cryptography==0.2.dev1
ndg-httpsclient==0.3.2
pyOpenSSL==0.13
pyasn1==0.1.7
```

I get an exception raised when `GET`ing a `301` response to a HTTPS request. I see that after the redirect is received the returned URL is [decoded to a Unicode string](https://github.com/kennethreitz/requests/blob/master/requests/adapters.py#L181). Then requests passes the response to `resolve_redirects` which uses the url to make a new request. This leads to a Unicode string being passed to urllib3 and eventually pyopenssl. And because in pyopenssl they now check that the data is of type bytes, an exception is thrown. 

I Wrote this test:

```
    def test_pyopenssl_redirect(self):
        requests.get('https://httpbin.org/status/301')
```

and this is the result of py.test:

```
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <OpenSSL.SSL.Connection object at 0x000000000345CC50>
buf = u'GET /redirect/1 HTTP/1.1\r\nHost: httpbin.org\r\nAccept-Encoding: gzip, defl...cept: */*\r\nUser-Agent: python-r
equests/2.2.1 CPython/2.7.6 Windows/8\r\n\r\n'
flags = 0

    def sendall(self, buf, flags=0):
        """
            Send "all" data on the connection. This calls send() repeatedly until
            all data is sent. If an error occurs, it's impossible to tell how much
            data has been sent.

            :param buf: The string to send
            :param flags: (optional) Included for compatibility with the socket
                          API, the value is ignored
            :return: The number of bytes written
            """
        if isinstance(buf, _memoryview):
            buf = buf.tobytes()
        if not isinstance(buf, bytes):
>           raise TypeError("buf must be a byte string")
E           TypeError: buf must be a byte string

..\testreq\lib\site-packages\OpenSSL\SSL.py:968: TypeError
=================================== 117 tests deselected by '-kpyopenssl_redirect' ====================================
====================================== 1 failed, 117 deselected in 4.47 seconds =======================================
```
Update PyOpenSSL contrib DEFAULT_SSL_CIPHER_LIST

The default cipher suites set in the pyopenssl contrib are now slightly out of date. SSLLabs has updated their recommendations and now suggests not using RC4 (see: [Is BEAST still a threat?](http://blog.ivanristic.com/2013/09/is-beast-still-a-threat.html).

This particular suite has been lifted from the [current master of twisted](https://github.com/twisted/twisted/blob/trunk/twisted/internet/_sslverify.py#L995), but provides a whitelist that prefers forward secrecy for key exchange, AES-GCM for encryption (with fallbacks for CBC and 3DES if AES is unavailable), and disables null auth, etc. This was derived by @hynek, so if there questions about it we can drag him in to answer.


Thanks for this! This is an excellent addition, but you're providing it in the wrong place. We vendor that file straight in from urllib3. Can I get you to open this Pull Request over there?


Whoops :) Will do.


Thanks so much! :cake:
Used method of including license breaks distutils bdist

Hi,

Because the LICENSE file is included in its total form in setup.py, "python setup.py bdist --format=rpm" for example bails out with the following errors:

copying dist/requests-2.2.1.tar.gz -> build/bdist.linux-x86_64/rpm/SOURCES
building RPMs
error: line 14: Unknown tag:    Licensed under the Apache License, Version 2.0 (the "License");
error: query of specfile build/bdist.linux-x86_64/rpm/SPECS/requests.spec failed, can't parse
error: Failed to execute: "rpm -q --qf '%{name}-%{version}-%{release}.src.rpm %{arch}/%{name}-%{version}-%{release}.%{arch}.rpm\n' --specfile 'build/bdist.linux-x86_64/rpm/SPECS/requests.spec'"

The fix is to 'hardcode' the license to a small string, and not the whole text.

Fix for same problem in other project:

https://github.com/nedap/pypuppetdb/commit/4ae1338f39a220bc96f8c5ee33bc3100428bebda


Thanks for this! We'll fix it up. =)


Resolved by #1893.
Add a .multipart() method to Response for auto-decoding multipart/* responses

It would be useful if `Response` objects provided a `.multipart()` method analogous to the `.json()` method for dealing with responses whose `Content-Type` is `multipart/*`, such as is common for geospatial web services.


Considering you're the first person in over 2 years to request this, it seems as though this is not something that is popular enough to belong in the core of requests. A feature like this would need to be requested a whole lot more for us to even consider adding this method.

On the other hand though, you could totally get this into the [requests-toolbelt](https://github.com/sigmavirus24/requests-toolbelt) which is designed for exactly this kind of thing. This is something that would be awesome to have and if you can put together a PR to handle the parsing, that would be awesome! I'd love to merge it over there provided you also add tests around it.

I'll leave this open to make sure that @Lukasa has a chance to weigh in, but I have a hunch he'll have the same opinion as me.


Fair enough.


@sigmavirus24 is exactly right.

The `Response.json()` method has caused us a lot of trouble. In almost every situation we reject features that involve manipulating content on the grounds that requests is an HTTP library, not an "anything-else" library, and every time we do that `Response.json()` gets pointed out. The fact is, we only implemented that because it's an overwhelmingly common use-case, which parsing multipart data is not.

That said, I encourage you to to whack it in the toolbelt. That would be an excellent place for it.

Thanks for raising this issue!
Provide 2.2.1 release.

Needed for `pip`, because they need [this](https://github.com/shazow/urllib3/pull/318) to fix [this](https://github.com/pypa/pip/issues/1488).

Mandatory:
- [x] Bring in a version of urllib3 that contains the above fix ~~(revision not yet known)~~. This is as simple as merging #1876 
- [x] Update changelog using [this](https://github.com/kennethreitz/requests/compare/v2.2.0...master). (done in 2a6b835a5e08c7365b8a4d3bfea4dcd98ad2d205).


:heart: our release manager @Lukasa 


Awesome, thanks. Y'all are awesome.


https://github.com/kennethreitz/requests/pull/1876 verified that it fixes the pip issue as well.


Edited the issue to make it clear that the first TODO could be accomplished by merging #1876.


Unofficial official 


:cake:


Great job!  Thanks everyone :)
Disable TCP_NODELAY for requests to non-proxies

Adds a short discussion of the change to HISTORY.rst


I have no objection to bringing this in. =) LGTM. :+1:


This is actually going to be superseded by #1876, and we don't really need the substantial header comment, so I'm closing this.

Thanks for your work on urllib3 though! :cake:


I suppose, though anyone doing a tcpdump or ngrep on a requests request might be surprised at the difference in packets going over the wire. Maybe this is not a common case though.


I think realistically the difference is irrelevant to most people, and advantageous to a small number. =) I'm not concerned.
Why don't we send `strict` to urllib3?

I was trawling the Python core bugtracker when I spotted [this](http://bugs.python.org/issue17849) beauty. It looks like it should be impossible to hit this if `strict` is set to `True`, but we don't pass that parameter to `urllib3`. Is there a good design reason we don't do this? If there isn't, are we open to a PR that does set that argument?


If it doesn't cause issues, I don't see why not.


`strict` was deprecated in Python3. With that the `LineAndFileWrapper` was removed because HTTP/1.x is assumed. The fact that the `LineAndFileWrapper` is created means that the proxy must be returning some malformed status line or is using HTTP/0.9. Or at least that sounds correct to me.

I might be a bit thick but what are people using to easily setup a proxy? I'd like to take a look at the raw response.


@jschneier I'm not so worried about that particular issue: we've never hit it in Requests. I'm just wondering whether there's a good technical reason we shouldn't just limit ourselves to well-formed HTTP/1.1.


I think printing an annoying deprecation warning on python3 is the only drawback if you only want to deal with HTTP/1.x


Deprecation warnings don't print by default on 3.x I don't think. So it's not even that bad.


Let's do it unless @kennethreitz has an object to restricting the library to HTTP/1.1


I don't see how this is useful.


I don't see how this is unuseful.


You baffle me


I suppose I'm now +0 (while before I was -0) :)


Doing this would solve #2322.


Alrighty, patch in #2323.
Cookie Not Set on 303

I'm making a request to a server that sets an httponly cookie and returns a 303. When I look in `response.headers`, I see the cookie. When I look in `session.cookies`, I don't see the cookie. Why isn't the cookie getting set?


Thanks for raising this issue!

Can you be a bit more specific? There are a number of things that could affect this behaviour. We need to know what version of Requests you're using and whether you're setting `allow_redirects` to False. If you aren't, can you also provide the output of `for h in r.history: print(h.headers)`?


@creese is it possible that you could share the URL with us as well so we can see if we can reproduce the behaviour? It would also be very helpful if you could answer @Lukasa's questions. Thanks for helping us help you!


@sigmavirus24 There was an issue with the form data in the initial request. It's resolved.


Thanks for updating us @creese
Digest auth for proxy

It would be nice to have digest auth support for proxies.

I've hit same problem as described in this SO post:
http://stackoverflow.com/questions/13506455/how-to-pass-proxy-authentication-requires-digest-auth-by-using-python-requests


Hi @oinopion, thanks for raising this issue!

It's an understandable thing to want. However, putting this in Requests by default is a trade-off: each additional authentication option added to Requests is an additional bit of code and maintenance cost. It also adds confusion to the API.

For this reason, Requests undertook to remove some of its authentication helpers in V1.0, as you can see in the [changelog](https://github.com/kennethreitz/requests/blob/1720e4bb87f5db29e9d9f42da1b0cbb1f52a7171/HISTORY.rst#100-2012-12-17). These got moved to helper modules under the requests organisation ([OAuth](https://github.com/requests/requests-oauthlib), [Kerberos](https://github.com/requests/requests-kerberos) and [NTLM](https://github.com/requests/requests-ntlm)), indicating their status as 'blessed' first-party solutions, while reducing the weight and complexity of the main library.

Candidates for the main library need two things: they need to be simple, and they need to be popular. Digest auth for proxies is simple, but it's not popular. I'd argue that it's dramatically less popular than OAuth, and probably less popular than Kerberos. For this reason, I'm disinclined to add support to the main Requests library. Interestingly, because it's so simple, I'm also disinclined to add a whole additional package for it under the requests organisation. I think the best place for it is actually the nascent [requests toolbelt](https://github.com/sigmavirus24/requests-toolbelt), which is the intended home for useful utilities like this kind of thing.

Does that sound like an acceptable approach?


I have a feeling that it should be either included in the core or both proxy and non-proxy digest auth's should be a separate package. Having one here and there is, in my opinion, not... "symmetrical", if you will. This however is just me. I will applaud any solution that saves me from digging into digest spec.

In any case, the most important thing is discoverability: wherever it ends up, there should be mention of it in in core library documentation. 


Yeah I'm thinking of sending a PR about the toolbelt.


We've agreed that core requests isn't taking any action here, so I'll close this down.
Error catch/reraise loses some info about the stack

On my mac,

``` python
import requests
r = requests.get("https://www.howsmyssl.com/a/check")
```

generates this stack trace:

``` python
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "requests/api.py", line 55, in get
    return request('get', url, **kwargs)
  File "requests/api.py", line 44, in request
    return session.request(method=method, url=url, **kwargs)
  File "requests/sessions.py", line 383, in request
    resp = self.send(prep, **send_kwargs)
  File "requests/sessions.py", line 486, in send
    r = adapter.send(request, **kwargs)
  File "requests/adapters.py", line 389, in send
    raise SSLError(e)
requests.exceptions.SSLError: [Errno 1] _ssl.c:507: error:1407742E:SSL routines:SSL23_GET_SERVER_HELLO:tlsv1 alert protocol version
```

which seems to skip a few lines that raised the exception in urllib3. Perhaps we could use `six.reraise()` here for python2/python3 compatibility?


Thanks for raising this @kevinburke!

That area is very carefully designed. We aren't reraising exceptions, we're wrapping them. This means we don't want the urllib3 exception to be raised, we want to raise the `requests.exceptions.SSLError`. In principle we could attach the traceback from the previous exception to this one, but that's unfortunately somewhat misleading. I don't really see any particular problem with this traceback, if I'm honest.

Does anyone disagree?


I'm in complete agreement with @Lukasa. FYI @kevinburke there has been a recent push to catch the last few exceptions that we didn't realize were spuriously being raised by urllib3 and bleeding through.

+1 to close this.


ok
Fixed parsing of username and password encoded in the URI

As per #1856, fixes the parsing of URI encoded usernames and password. Also fixes a bogus test where the username in the URI was not correctly encoded.


Did you not notice #1858 


This looks fine to me. =)


thanks @Lukasa sorry I didn't notice your weekend work..!


@sybeck2k It's really not a problem at all. =) Ego is a dangerous thing in open source so I try to check mine; and besides, you did the very generous thing of merging my changes with yours, so I think you handled this very well. Thankyou. =)


You two rock! :metal: I'll give this another once over when I get into work


Could we just get some comments around the test you added with the percent encoded chars?
Use calendar.timegm when calculating cookie expiration

Fixes #1859

Credit: @lukasa


We need to confirm that this will match the actual parsing logic of cookies before we merge this: we don't want to start messing up cookies created in this way.


That's a good point @Lukasa. I'm guessing @gazpachoking might have a good idea. I'm just too tired to think this hard right now =P


Me too, I'll take a look at this tomorrow.


Any update?


Sorry I never got around to that. @Lukasa how about you?


Uh, no, totally dropped the ball here. I'll put it on my list. =)


@Lukasa how should we make sure this doesn't mess anything up?


I'm not sure, to be honest. I'm wondering if there's a good way we can confirm our cookie-parsing code works in all locales.


Closing due to inactivity.


Ok, I'm reopening this because I've finally validated that the stdlib does this. That suggests that it's the right thing to do. I'll have to do the merge manually, but I'd like to have this.

@sigmavirus24, one question: could we meaningfully add this to a 2.9.1, or should it wait for 2.10.0?


I think this could be included in 2.9.1


\o/ Finally fixed, nearly two years after the original patch was proposed!
Unquote the auth after splitting the url.

This _should_ resolve #1856. /cc @sigmavirus24 @t-8ch.


Just left one note that's a matter of stylistic preference more than anything else. That is not a reason to prevent anyone from merging this though. 

:+1: :shipit: :cake: :beer: :smiley_cat: 


#rewrite


So as part of my research I found this: https://github.com/kennethreitz/requests/blob/ac4e05874a1a983ca126185a0e4d4e74915f792e/requests/adapters.py#L289..L290 which means that we can fix up `adapters.py` to no longer need to unquote the username/password pair. Granted, unquoting something that is already unquoted should return the exact same thing, but I bet by removing the calls to `unquote` will grant ever the slightest overall performance benefit. Also, that comment is just plain wrong now. =P


And the other place it's used is here: https://github.com/kennethreitz/requests/blob/ac4e05874a1a983ca126185a0e4d4e74915f792e/requests/models.py#L456 and notice that there's an optional param to that method that is never used if it is ever passed. We should either use it or remove it (not necessarily in this PR though).

Finally, given that the call to `HTTPAdapter#proxy_headers` is wrapped inside an `if proxy:` block, the param it sends to `get_auth_from_url` should never be `None` or `''`. And `prepare_auth` on the `PreparedRequest` is called after `prepare_url` which should blow up if `url` is not a valid `url`. I think we're safe making `get_auth_from_url` a bit less paranoid. As with all of my reviews though, this is totally up to the discretion of @kennethreitz and @Lukasa 


Thanks for that @sigmavirus24, that's a really helpful set of information! I'll update this PR to reflect it.


Hooray more changes!


@kennethreitz :shipit: :+1:


@Lukasa @sybeck2k found a problem with this test: https://github.com/kennethreitz/requests/pull/1862/files#diff-56c2d754173a4a158ce8f445834c8fe8R705 can you fix it here too?


@sigmavirus24 @Lukasa sorry I didn't notice the issue was tracked also here. I've tried to merge all the changes of Lukasa into my pull request - the main difference is about the test `test_get_auth_from_url_percent_chars` that on my opinion was flawed as the input url was not url-encoded.


There's no reason to have this PR open twice, so I'll close this one. =)
Broken parsing of authenticated URI for Proxy

Hello,
when passing a proxy with authentication, and where the password contains the character '#', the parsing of the URI is broken. For instance:

```
proxy = "http://user:%23Cpassword%23C:server:port"
ParseResult(scheme='http', netloc='user:', path='', params='', query='', fragment=password#@server:port0')
```

This is caused by the `url = unquote(url)` done before the parsing in `utils.py`.
I believe the URI should not be unquoted before the parsing, rather the username and password should be quoted after the parsing. What do you think?

Thank you


Thanks for raising this issue!

It's hard to see how unquoting before parsing the URL is really helping us. We can almost certainly defer the unquote step until after parsing the URL, applying unquote only to the username and password. @sigmavirus24, can you think of any reason that's not going to be ok?


I can't think of a reason it wouldn't be okay and in fact it's the only way I can see of doing this correctly.


Fine then, let's do that. =) I'll fix this up.


Ok, so `urlparse` chokes like a champ on a sample URL, and so does urllib3's superior `parse_url` function:

``` python
>>> urlparse('http://user:pass#pass@complex.url.com/path?query=yes')
ParseResult(scheme='http', netloc='user:pass', path='', params='', query='', fragment='pass@complex.url.com/path?query=yes')
```

``` python
>>> parse_url('http://user:pass#pass@complex.url.com/path?query=yes')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "requests/packages/urllib3/util.py", line 397, in parse_url
    raise LocationParseError("Failed to parse: %s" % url)
requests.packages.urllib3.exceptions.LocationParseError: Failed to parse: Failed to parse: user:pass
```

Thoughts? /cc @shazow


@Lukasa I think the unquoted `#` is not valid at this position:

[RFC 3986](https://tools.ietf.org/html/rfc3986)

```
userinfo    = *( unreserved / pct-encoded / sub-delims / ":" )
```

While `#` is in the `gen-delims` group. Encoding it as `%23` makes both parse functions work for me.


Oh, it's definitely not valid in a URL. I just need to be sure that, if a user provides it to us, it's in an acceptable state at that stage in Requests' execution flow.


@Lukasa Could make urllib3's parser more forgiving if that's what you really want, or just encode things properly. :)


@shazow something something if you change your parsers to accept bad URLs you're going to have a bad time something something. =P


Nah @shazow, you're fine. We'll always have encoded the URL at the point we call `get_auth_from_url`, so we should never have a hash that doesn't represent the fragment. All is well. =)


Fixed in #1856
TA example for SSL version.


Thanks for this!

I've made a couple of suggestions inline in the diff. Can you take a look at them for me? =)


You can also rebase all of those commits to one. If you're unfamiliar with rebasing I can send instructions 


I didn't think I could rebase if I already pushed. Not the case?

Dustin

On Thu, Jan 9, 2014 at 7:38 AM, Ian Cordasco notifications@github.comwrote:

> You can also rebase all of those commits to one. If you're unfamiliar with
> rebasing I can send instructions
> 
> —
> Reply to this email directly or view it on GitHubhttps://github.com/kennethreitz/requests/pull/1852#issuecomment-31926780
> .


Not the case. You can safely rebase so long as no-one has pulled from your branch. We have not. =)


I thought that me pushing to GitHub could also be considered to have the
same effect as GitHub pulling from me, in which cases rebases should not be
done. Why does this apply to GitHub client repos and not GitHub itself?

Dustin
On Jan 9, 2014 10:59 AM, "Cory Benfield" notifications@github.com wrote:

> Not the case. You can safely rebase so long as no-one has pulled from your
> branch. We have not. =)
> 
> —
> Reply to this email directly or view it on GitHubhttps://github.com/kennethreitz/requests/pull/1852#issuecomment-31945935
> .


To be clear, you _will_ have to force push to GitHub, but that's totally fine. =)


Done. I didn't know what else you were referring to by "style", other than
the column width (which I missed since I was working in text and not code).
So, if there's something else, then let me know.

On Thu, Jan 9, 2014 at 11:19 AM, Cory Benfield notifications@github.comwrote:

> To be clear, you _will_ have to force push to GitHub, but that's totally
> fine. =)
> 
> —
> Reply to this email directly or view it on GitHubhttps://github.com/kennethreitz/requests/pull/1852#issuecomment-31948301
> .


That was exactly what I meant, so this looks good to me. =) I'll merge when I get home and can deal with the conflicts.


Much appreciated! Will merge as soon as the conflicts are resolved :)


Thanks, Ken. I'm not trying to be a burden. I just always find SSL-related
hiccups to be a unwelcomed headache.

I appreciate it.
On Jan 9, 2014 2:14 PM, "Kenneth Reitz" notifications@github.com wrote:

> Much appreciated! Will merge as soon as the conflicts are resolved :)
> 
> —
> Reply to this email directly or view it on GitHubhttps://github.com/kennethreitz/requests/pull/1852#issuecomment-31965690
> .


Oh not at all! This is a fantastic pull request, and one that I've wanted us to have for a long time. 

:sparkles: :cake: :sparkles:


Brilliant, I've merged it in. =) All is well with the world. Thanks so much!
implement "send" hook for right before sending a Request

This implements a `send` hook, which is called right before a `Request` object is sent.

My motivation here is for a profiler I've written that automatically records request timing information (by recording a timestamp before a request is sent, and after), but I think there are probably a lot of other use cases, such as automagically munging request headers or whatnot.


Thanks for this work!

We used to have a `send` hook (and a number of others), but we got rid of almost all of them. This is because the recommended way of implementing things like this is now to use [custom Transport Adapters](http://docs.python-requests.org/en/latest/user/advanced/#transport-adapters). If you need an example of how to implement one, I've written a number of examples: [here](https://lukasa.co.uk/2012/12/Writing_A_Transport_Adapter/), [here](https://lukasa.co.uk/2013/01/Choosing_SSL_Version_In_Requests/) and [here](https://lukasa.co.uk/2013/05/Caching_In_Python_Requests/).

This new position means we no longer want to have `send` hooks. Unfortunately, this means that we won't merge this PR. Thanks so much for doing the work though!


We also already provide the `elapsed_time` attribute on a response object that does exactly what you want


So is the general idea that I'd subclass `HTTPAdapter`, and do something approximately like this?

``` python
class MyHTTPAdapter(requests.adapters.HTTPAdapter):

    def send(self, request, **kwargs):
        my_custom_thing_here()
        response = super(MyHTTPAdapter, self).send(request, **kwargs)
        my_other_custom_thing()
        return response

s = requests.Session()
a = MyHTTPAdapter()
s.mount('http://', a)
s.mount('https://', a)
```

(And in response to @sigmavirus24 , it's a bit more complicated than just elapsed time, since the profiler is also profiling memory usage and things from `resource.getrusage()`.)


Absolutely right, yes. =)
Force SSLv3.

This is a _possible_ fix for #1847. 


According to the table on http://docs.python.org/2/library/ssl.html#ssl.wrap_socket this is going to blow up on more mordern servers. (At least it is supposed to)


Is that true? http://docs.python.org/2/library/ssl.html#ssl.PROTOCOL_SSLv3


Based on my reading of that table, selecting `SSLv3` will work on servers that don't offer `SSLv2`, but `SSLv23` will not work. Conversely, `SSLv3` will not work on servers that don't offer anything anything better than `SSLv2`, and the prevailing attitude seems to be "who cares about those people".


Please try: `https://t-8ch.de` I disabled everything except TLS.
Using requests with `SSLv3` doesn't work for me.


Yup, you're right. Goddamn I hate SSL so much.


And `SSLv23` works with all version of python (still openssl 1.0.1f)


Doesn't with old versions of OpenSSL though.


Those probably don't support TLS at all


Thanks @t-8ch
API for reporting failing URLs


Hm?


Like a place to POST to http://report.python-requests.org/ when you have a URL that doesn't behave properly.


Ah okay


Failing in what sense? Non-conformant, serving 500's...


On Fri, Jan 10, 2014 at 04:48:02PM -0800, Kevin Burke wrote:

> Failing in what sense? Non-conformant, serving 500's...

We're not looking for twilio's list of horrible servers and websites that 
cause y'all headaches :P

I think this issue was motivated by the fact that we have been getting a lot 
of bug reports which focus more around one specific URL misbehaving and ends 
with us debugging the service for the user.


How would this be implemented? Is it for Requests developers, users, or someone else?


How about adding a report functionality, where you specify the reporting, which defaults to report.python-requests.org ? I would like to implement this, once we have a clear idea on exactly what we want to report.


I mean where we specify the reporting URL.


The primary use-case here is to avoid Kenneth getting spammed with bug reports in his inbox that basically go: "This website doesn't work for me in Requests". I'm open to this being a more general service like @shshank suggested, but right now our interest is in getting that crud off Kenneth's plate and onto mine and @sigmavirus24's. =)
get_netrc_auth should handle os.path.expanduser failing

https://github.com/toastdriven/django-haystack/issues/924 has a problem report from a user who appears to be running inside a process which does not have `$HOME` defined and is running under a UID which is either not in /etc/passwd or does not have permission to access it.

This causes [utils.get_netrc_auth](https://github.com/kennethreitz/requests/blob/v2.1.0/requests/utils.py#L73) to raise an unexpected `KeyError`. The easiest fix would be to simply add that to the except block at the bottom but that's probably too dangerous – I'd probably handle the `KeyError` right at the source until this can be fixed upstream.


Upstream Python bug report: http://bugs.python.org/issue20164


Well this makes me very sad. Thanks for raising this!

Given that upstream is not going to fix this on 2.6 _for sure_, even if they do implement a fix elsewhere, I think we need to handle this. The correct behaviour is that `get_netrc_auth` should fail and return nothing.

I'm also inclined to be cautious, so we should restrict the scope of the `try...except` block.


@Lukasa I took a first pass at a patch in 0b41cec


Alright, I'm happy with this. You can ignore TravisCI (I have no idea why it's still running), the relevant CI results are these (all of which passed):

http://ci.kennethreitz.org/job/requests-pr/PYTHON=2.6/167/
http://ci.kennethreitz.org/job/requests-pr/PYTHON=2.7/167/
http://ci.kennethreitz.org/job/requests-pr/PYTHON=3.3/167/
http://ci.kennethreitz.org/job/requests-pr/PYTHON=pypy-1.8/167/

Waiting on @sigmavirus24 to review.


Great - thanks for reviewing it!


WHY IS TRAVIS RUNNING AGAIN OH MY GOD


I'm very sad that this requires so many lines of code, but so be it. 


This will at some point become unnecessary once the upstream fix has been widely deployed – hopefully in less than another 5 years:  

https://bugs.python.org/issue10496
Resolve proxy_bypass problems.

This resolves the sad, sad 2.6.x problems in `proxy_bypass` as reported in #1844 and #1841.

The nature of this fix is that, fundamentally, we don't care enough to let this call stop us. Assume that if it fails we aren't bypassing the proxy (usually we won't be) and move on with our lives.

I'm only catching the specific exceptions we've seen. I'm open to wrapping this in a generic `except` block if we want to.


Will get the issue reporter to test this asap


So the issue reporter said he got a different error, but the other old 2.6 issue (the _tunnel_host one). So I'd say that this is probably fixed with this change, and this should be fixed in entirity once both changes happen. I'm happy to have him test once this and the urllib3 updated mentioned in #1842 land to make sure it's all kosher.


Sounds good to me.

For the record, this fix can still lead to failures. If the user is actually using any of the `proxy_bypass` function on their Mac with 2.6.[0-5], we can potentially not bypass the proxy. That sucks, but it sucks far less than this being broken.


This looks pretty safe to me. I'm going to merge it, just so it's out of my queue. 

Comment if it doesn't solve the problem :)
Allow bypassing decoding with iter_content

When servers send a `Content-Encoding` header requests (actually urllib3, but because of decode_content=True kwarg requests passes) will automatically decode the response. In pip this is causing a breakage for servers that serve a .tar.gz with a `Content-Encoding: gzip` header. Would it be at all possible to have iter_content support a `content_decode` kwarg that just get's passed through to urllib3?


For reference this is in pip's issues as https://github.com/pypa/pip/issues/1419 and https://github.com/pypa/pip/issues/1421


I'm obviously tempted to argue that `pip` should be able to spot this and handle it (that is, any compliant server would signal `Content-Encoding: gzip` and `Content-Type: application/x-tar`), but that's not a very helpful way to approach this problem.

In principle we could do that, it would be very easy from our side. @kennethreitz, the API is your call: are you happy to do this?


I'm perfectly happy reaching in and using the raw response myself FWIW, I just figured if requests was OK with exposing this option it'd be easier and might be useful for other people. But either way it's ok with me.


Yeah, that was basically the decision I want @kennethreitz to make: do we just say "tough, use the raw response", or do we provide the kwarg.


The current intended API design is to use .raw for this.

## 

Kenneth Reitz

> On Jan 7, 2014, at 3:52 AM, Cory Benfield notifications@github.com wrote:
> 
> I'm obviously tempted to argue that pip should be able to spot this and handle it (that is, any compliant server would signal Content-Encoding: gzip and Content-Type: application/x-tar), but that's not a very helpful way to approach this problem.
> 
> In principle we could do that, it would be very easy from our side. @kennethreitz, the API is your call: are you happy to do this?
> 
> —
> Reply to this email directly or view it on GitHub.


Ok that's fine! I'll use raw then.
HTTPS connections with requests incompatible with python 2.6.(0-2) 

Creating this for tracking purposes... urllib3 attempts to access an attribute that was added in 2.6.3, breaking older Python 2.6's. 

https://github.com/shazow/urllib3/pull/307 fixes this issue, once that's merged the dependency in requests should probably be updated.


@sigmavirus24 and I are in favour of not supporting very old patch releases of 2.6. For perspective, 2.6.0 was released in 2008 and 2.6.2 was released in April 2009.

Obviously, we'll take the fix in urllib3, but I see no reason for Requests to rush into supporting a five year old release of Python. Anyone who felt the need to be on Python 2.6 should have been taking security patches and be at 2.6.9. 


Agreed, was mostly creating this so people can hopefully find it on Google
if they run into this problem.

## 

Kevin Burke | Twilio
phone: 925.271.7005 | kev.inburke.com

On Mon, Jan 6, 2014 at 12:58 PM, Cory Benfield notifications@github.comwrote:

> @sigmavirus24 https://github.com/sigmavirus24 and I are in favour of
> not supporting very old patch releases of 2.6. For perspective, 2.6.0 was
> released in 2008 and 2.6.2 was released in April 2009.
> 
> Obviously, we'll take the fix in urllib3, but I see no reason for Requests
> to rush into supporting a five year old release of Python. Anyone who felt
> the need to be on Python 2.6 should have been taking security patches and
> be at 2.6.9.
> 
> —
> Reply to this email directly or view it on GitHubhttps://github.com/kennethreitz/requests/issues/1842#issuecomment-31686263
> .


Thanks for that. =)


Would it be at all possible to get you to reconsider releasing this relatively quickly? Especially if it's possible to get #1843 into the same release as well. Older versions of OSX ship with Python 2.6.1 or so with no mechanism for updating besides updating the entire OS. This has affected pip on those older OSX releases.


I'm fine with releasing relatively soon but that responsibility lies entirely on @kenmethreitz's shoulders.


Yeah, when we get this and #1845 resolved I'll push for a release.


You all are awesome :sparkles: 

Once things are merged I can make a test branch for pip and verify it works before the release if you'd like.


@dstufft I'd greatly appreciate that. :)