check for headers containing return characters

Addresses issue raised in #2947.

This likely needs some tweaking. I added the check in `adapters.py` because it was the lowest level I could find header modification being done before sending. I'm not sure it actually needs to be this low though since it doesn't seem easily reachable with a non-`PreparedRequest` object.

Alternatively, we could move the check to the [`prepare_headers`](https://github.com/kennethreitz/requests/blob/master/requests/models.py#L406) method in `PreparedRequest`.

I also ignored `Accept` headers in the check because I the requirements in [RFC7230](https://tools.ietf.org/html/rfc7230#page-24) seems a bit vague.

> This specification deprecates such line folding except within the message/http media type
>    (Section 8.3.1).  A sender MUST NOT generate a message that includes line folding (i.e., that has any    field-value that contains a match to the obs-fold rule) unless the message is intended for packaging
>    within the message/http media type.

This may be a poor interpretation on my part though.


We should forbid the newline in Accept headers too: that is a misreading of RFC 7230. =) Otherwise, this is basically the right approach. Some notes:
- Can we move the logic into a underscore-prefixed method on the HTTPAdapter? That will let people override the method if they need to.
- Can we also search for line feed characters (`'\r'`)?
- Can we also also confirm the header field does not begin with linear whitespace (e.g. space, tab)?

This should handle most of the header smuggling risks. At this point we probably just want to match a regex against the header.


I understand the reasoning behind putting it in adapters, but I do feel like it should be in the prepare_headers section instead, if possible. PreparedRequests should allow users to do stupid things, like this. No need to have saftey tape at that level, imo. 

I won't be surprised if @Lukasa disagrees :)


If we're treating this as a security issue and trying to prevent injection on dynamically generated headers, I think @Lukasa's internal method allows flexibility while maintaining the smallest surface area for issues. If we're just looking to prevent unintentional foot shooting, prepare_headers seems "cleaner" to me.


Agreed. I feel like it's the latter. Am I incorrect?


No, I was just checking I understood the intent of the patch :)

Working on moving things up to `prepare_headers` with Lukasa's proposed changes now.


Much to @kennethreitz's surprise, I agree with him. =)

I'm worried that users who are unsuspectingly using our high-level APIs aren't subject to risk, but if you're going to drop down to monkeying with PreparedRequests then you should be able to do more-or-less what you like. =) 


I've got some small notes about code structure, but this is looking really good!


Thanks, I think all of the changes you asked for should be in place. If you've got any additional notes, let me know. Always open to feedback :)


Sorry, some more smaller notes. =) This should be the last review though!


To be clear, you need to remove `to_native_string(val)` when using the bytes pattern.


Hrgh, actually, this is a bit trickier than that. We need two regexes, one for unicode strings and one for bytes strings. And at this point, we probably want to turn this into a full-blown utility function.


Hmm, yeah. So you're suggesting something along the lines of this?

```

def validate_header(val):
    if isinstance(val, str):
        regex = str_reg
    else:
        regex = byte_reg
    if regex.match(val):
         raise InvalidHeader()
    return val

---

def prepare_headers(self, headers) 
    self.headers = CaseInsensitiveDict()
    if headers:
        for name, val in headers.items():
                self.headers[to_native_string(name)] = validate_header(val)

```


Yup, something like that, yeah.


@Lukasa Alright, there's rev 1 for a utility function.


Ok, one tiny string fix and I'll finally be done complaining. =D


Cool, LGTM. Rebase away. =D


Squashed and passing. Thanks for taking the time to work through this with me @Lukasa :)


Hooray! I am happy. =)


Thanks @nateprewitt! :sparkles: :cake: :sparkles:


@Lukasa, stealing my emoji chain hahaha


Just to let you know that we bumped into this recently here: https://github.com/jjjake/internetarchive/issues/151 - we were just sending integer 1 or 0 expecting it to be coerced to "1" or "0".
This is was simple to fix but I think this should be marked as a breaking change.

Regarding the implementation: using `isinstance()` checks is kind of [un-pythonic](http://canonical.org/~kragen/isinstance/), it forbids the caller to ship instances of objects that should behave like strings or return rendered strings when asked.


> Regarding the implementation: using isinstance() checks is kind of un-pythonic, it forbids the caller to ship instances of objects that should behave like strings or return rendered strings when asked.

There is no way in Python for something to behave "like a string" without being a string. You can ask something to be a string via `str()`, but because`str()` falls back to `repr()` it cannot be used as a sensible screening mechanism.

What would be your proposed alternative?


That probably means we have no choice but to depend on `str()` ? The original bug was about embedded newlines only I think.


We _cannot_ depend on `str()`, its behaviours are consistently and completely unacceptable. The worst of them being the fact that, on Python 3, `str(b'hello') == "b'hello'"`, meaning that anyone passing a bytestring header value on Python 3 would get their header value totally mangled, despite the fact that passing us a bytestring is actually the _simplest_ case from the perspective of our logic: we pass it through, after checking that no ASCII bytes for newlines or leading/trailing whitespace are present.

Unconditionally casting data to strings does not behave well in Python, and the biggest problem is that it leads to subtle, difficult-to-discover bugs. The bug you hit, @saper, fails loudly and clearly. It is easy to understand what the problem is. However, if we adjust to your proposed behaviour, the failure mode is that someone has passed us something like a `datetime`, leading to Requests sending a header that reads `Date: datetime(days=52, hours=23, minutes=12, seconds=0)`, which is a _clearly_ incorrect header value. That failure mode is much harder to debug: all that you see in your code is a 400 Client Error that needs to be chased down, requiring you to examine everything about your request.

Subtle failure modes are almost always bad. I appreciate that this is an inconvenience for you, but we never intended your use-case to work, and historically it did not. It was by accident that it became acceptable several releases ago, and we have now taken action to formally prevent that use-case from being allowed again.


Question: Did implicit stringification work correctly in both Python releases? (Before we needed an explicit one to cut away newlines?). Except for the `repr()` case of course?

I've seen [{k: str(v) for k, v in headers.items()}](https://github.com/jjjake/internetarchive/pull/150/files#diff-562526d0e51ec94fea4ddda8c54853e5R155) proposed as a fix and I understand from what you are saying that explicit stringification is no good.


That depends on your definition of "correctly", but basically: no. All my complaints about the weirder datatypes remain in place.


Thank you, that makes sense. But wouldn't be good to mark it as a breaking change? A relatively innocent implicit `int` conversion just stopped to work. Not sure if there were people implementing `__str__()` to have their HTTP headers formatted nicely.


That's a reasonable question, and we discussed it. The conclusion was that, given that we had documented that header were strings, that we were covered by that restriction. I felt less strongly on that point than the other core maintainers, but the call was made. =)


Well, let it be then, thanks!


Thanks for the understanding @saper! =)