Fix a bug for header field getting malformed when it extends over multiple lines #111
Conversation
|
I think the CI failed because the version of |
Codecov Report
@@ Coverage Diff @@
## master #111 +/- ##
==========================================
- Coverage 95.43% 95.11% -0.33%
==========================================
Files 7 7
Lines 482 491 +9
Branches 98 101 +3
==========================================
+ Hits 460 467 +7
- Misses 15 17 +2
Partials 7 7
|
w3lib/http.py
Outdated
| headers = headers_raw.splitlines() | ||
|
|
||
| headers = [] | ||
| for line in headers_raw.split(b'\r\n'): |
There was a problem hiding this comment.
Why limit line boundaries to \r\n?
There was a problem hiding this comment.
The reason is that the specification (RFC 2616 cited above or RFC7230 which appears more recent) states that in HTTP messages, lines are folded specifically by CRLF (\r\n) and not by LF (\n). Not following this had lead to an issue where a header field value containing a LF (\n) was accidentally interpreted as a multiline field value. Without leading at least one space or tab char in the following line, however, the header ended up being interpreted as malformed and the lines were simply discarded when parsed out.
This is rather pathological and use of LF within a field value feels like a bad idea, but it doesn't appear to violate the specification so some HTTP messages with that structure exist.
There was a problem hiding this comment.
I would leave .splitlines() nonetheless, though, given the current information we have, to be lenient with servers that may not follow the standard.
If later it turns out by allowing certain forms of line break we are misinterpreting header values, then we can change this accordingly.
There was a problem hiding this comment.
I propose that the function headers_raw_to_dict take in an optional parameter strict with the default value that does not change the existing behavior. This way, the code works as is, but would be ready for those who want a stricter interpretation of the HTTP standard via a simple switch.
I think exposing this switch to the users requires a similar change at the user-facing API, but that should be done in a separate PR, if desired.
|
Will you consider keeping |
|
Before fixing, let me do a couple tests to see if reverting to One thing though is that by not sticking to the standard, the code would be lenient on using It's okay by me if that's the design compromise for the library (and I myself am not sure how often servers do break the rule, using (One possible workaround might be to inspect each header to see which of |
|
Sorry for taking a while but I finally managed to update the PR as requested. Please note my comment about the optional switch. I hope that the PR makes sense now. |
Co-Authored-By: Adrián Chaves <adrian@chaves.io>
|
Bumping to close outdated PR. |
The message headers section of RFC 2616 (Hypertext Transfer Protocol -- HTTP/1.1) states that
The current implementation of
s3lib.http.headers_raw_to_dictfunction unfortunately tends to read only the first line of such multi-line header field (and discard the rest in the following loop making tuples).This PR refactors the function to be able to properly read multi-line header field. To do this reliably, it also more strictly interprets CRLF (i.e.,
\r\n) as the end-of-line marker, as per RFC 2616.