Skip to content

http.client splits headers on non-\r\n characters #66429

Closed
Closed
http.client splits headers on non-\r\n characters#66429
Labels
stdlibStandard Library Python modules in the Lib/ directorytopic-emailtype-bugAn unexpected behavior, bug, or error

Description

@scharron
scharron
mannequin
opened on Aug 20, 2014
BPO 22233
Nosy @warsaw, @gpshead, @ezio-melotti, @dhellmann, @bitdancer, @vadmium, @serhiy-storchaka, @demianbrecht
Files
  • crlf-headers.patch
  • crlf-headers2.patch
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = <Date 2016-09-07.21:49:00.246>
created_at = <Date 2014-08-20.10:02:33.816>
labels = ['type-bug', 'library', 'expert-email']
title = 'http.client splits headers on non-\\r\\n characters'
updated_at = <Date 2016-09-07.22:44:26.851>
user = 'https://bugs.python.org/scharron'

    bugs.python.org fields:

    activity = <Date 2016-09-07.22:44:26.851>
actor = 'martin.panter'
assignee = 'none'
closed = True
closed_date = <Date 2016-09-07.21:49:00.246>
closer = 'r.david.murray'
components = ['Library (Lib)', 'email']
creation = <Date 2014-08-20.10:02:33.816>
creator = 'scharron'
dependencies = []
files = ['41177', '44442']
hgrepos = []
issue_num = 22233
keywords = ['patch']
message_count = 13.0
messages = ['225562', '225565', '246569', '255440', '255460', '255518', '273983', '273988', '274013', '274840', '274899', '274900', '274903']
nosy_count = 11.0
nosy_names = ['barry', 'gregory.p.smith', 'ezio.melotti', 'doughellmann', 'r.david.murray', 'python-dev', 'martin.panter', 'serhiy.storchaka', 'demian.brecht', 'scharron', 'Clay Gerrard']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue22233'
versions = ['Python 3.5', 'Python 3.6']

    Activity

    1. scharron commented on Aug 20, 2014

      @scharron
      scharronmannequin
      on Aug 20, 2014
      MannequinAuthor

      In some cases, the headers from http.client (that uses email.feedparser) splits headers at wrong separators. The bug is from the use of str.splitlines (in email.feedparser) that splits on other characters than \r\n as it should. (See bug http://bugs.python.org/issue22232)

      To reproduce the bug :

      import http.client
c = http.client.HTTPSConnection("graph.facebook.com")
c.request("GET", "/%C4%85", None, {"test": "\x85"})
r = c.getresponse()
print(r.headers)
print(r.headers.keys())
print(r.headers.get("WWW-Authenticate"))

      As you can see, the WWW-Authenticate is wrongly parsed (it misses its final "), and therefore the rest of the headers are ignored.

    2. scharron
      added
      stdlibStandard Library Python modules in the Lib/ directory
      type-bugAn unexpected behavior, bug, or error
      on Aug 20, 2014

    3. scharron commented on Aug 20, 2014

      @scharron
      scharronmannequin
      on Aug 20, 2014
      MannequinAuthor

      A consequence of this bug is that r.read() blocks until a timeout occurs since the content-length header is not interpreted (I think this is related to the HTTPResponse.__init__ comment)

    4. bitdancer
      added
      topic-email
      on Aug 23, 2014

    5. gpshead commented on Jul 10, 2015

      @gpshead
      gpshead
      on Jul 10, 2015
      Member

      The obvious fix seems to be to not use splitlines but explicitly split on the allowed characters for ASCII based protocols and formats that only want \r and \n to be considered.

      I don't think we can rightfully change the unicode splitlines behavior.

    6. gpshead
      changed the title [-]http.client splits headers on none-\r\n characters[/-] [+]http.client splits headers on non-\r\n characters[/+] on Jul 10, 2015

    7. vadmium commented on Nov 26, 2015

      @vadmium
      vadmium
      on Nov 26, 2015
      Member

      For the record, this is a simplified version of the original scenario, showing the low-level HTTP protocol:

      >>> request = (
...     b"GET /%C4%85 HTTP/1.1\r\n"
...     b"Host: graph.facebook.com\r\n"
...     b"\r\n"
... )
>>> s = create_connection(("graph.facebook.com", HTTPS_PORT))
>>> with ssl.wrap_socket(s) as s:
...     s.sendall(request)
...     response = s.recv(3000)
... 
50
>>> pprint(response.splitlines(keepends=True))
[b'HTTP/1.1 404 Not Found\r\n',
 b'WWW-Authenticate: OAuth "Facebook Platform" "not_found" "(#803) Some of the '
 b'aliases you requested do not exist: \xc4\x85"\r\n',
 b'Access-Control-Allow-Origin: *\r\n',
 b'Content-Type: text/javascript; charset=UTF-8\r\n',
 b'X-FB-Trace-ID: H9yxnVcQFuA\r\n',
 b'X-FB-Rev: 2063232\r\n',
 b'Pragma: no-cache\r\n',
 b'Cache-Control: no-store\r\n',
 b'Facebook-API-Version: v2.0\r\n',
 b'Expires: Sat, 01 Jan 2000 00:00:00 GMT\r\n',
 b'X-FB-Debug: 07ouxMl1Z439Ke/YzHSjXx3o9PcpGeZBFS7yrGwTzaaudrZWe5Ef8Z96oSo2dINp'
 b'3GR4q78+1oHDX2pUF2ky1A==\r\n',
 b'Date: Thu, 26 Nov 2015 23:03:47 GMT\r\n',
 b'Connection: keep-alive\r\n',
 b'Content-Length: 147\r\n',
 b'\r\n',
 b'{"error":{"message":"(#803) Some of the aliases you requested do not exist: '
 b'\\u0105","type":"OAuthException","code":803,"fbtrace_id":"H9yxnVcQFuA"}}']

      In my mind, the simplest way forward would be to change the “email” module to only parse lines using the “universal newlines” algorithm. The /Lib/email/feedparser.py module should use StringIO(s, newline="").readlines() rather than s.splitlines(keepends=True). That would mean all email parsing behaviour would change; for instance, given the following message:

      >>> m = email.message_from_string(
...     "WWW-Authenticate: abc\x85<body or header?>\r\n"
...     "\r\n"
... )

      instead of the current behaviour:

      >>> m.items()
[('WWW-Authenticate', 'abc\x85')]
>>> m.get_payload()
'<body or header?>\r\n\r\n'

      it would change to:

      >>> m.items()
[('WWW-Authenticate', 'abc\x85<body or header?>')]
>>> m.get_payload()
''

    8. bitdancer commented on Nov 27, 2015

      @bitdancer
      bitdancer
      on Nov 27, 2015
      Member

      I agree. Can you update the email issue with this suggestion and/or a patch?

      The problem with this, of course is backward compatibility, but since it is more correct per the RFCs, our past policy has been to fix it anyway.

    9. vadmium commented on Nov 28, 2015

      @vadmium
      vadmium
      on Nov 28, 2015
      Member

      David: what is the email issue you mentioned? In the mean time, I am uploading a patch to this issue.

      It seems using StringIO is a bit slower than str.splitlines(). I found a way to optimize building long lines, which compensated a lot of the loss, but this optimization would apply even without using StringIO. My patch makes test.test_email 0.3% slower (the optimization alone would make it 4.4% faster), and test_email.TestFeedParsers.test_long_lines() is 3% slower (optimization 12% faster).

      I also tried two other alternatives to str.splitlines(), but they were both slower than the StringIO technique:

      • _partial is a list of UTF-8 bytes; join and use bytes.splitlines()
      • _partial is a UTF-8 bytearray; use bytearray.splitlines()

    10. ClayGerrard commented on Aug 31, 2016

      @ClayGerrard
      ClayGerrardmannequin
      on Aug 31, 2016
      Mannequin

      BUMP. ;)

      This issue was recently raised as one blocker to OpenStack Object Storage (Swift) finishing our port to python3 (we're hoping to finish adding support >=3.5 by Spring '17 - /me crosses fingers).

      I wonder if someone might confirm or deny the attached patch is likely to be included in the 3.6 timeframe (circa 12/16?) and/or back-ported to the 3.5 series?

      FWIW, I would echo other's sentiment that I would much prefer the implementation to be correct even if there was some worry we might have to choose between further optimization and getting a fix ASAP :D

      Warm Regards,
      -Clay

    11. vadmium commented on Aug 31, 2016

      @vadmium
      vadmium
      on Aug 31, 2016
      Member

      If someone reviews my patch and thinks it is fine, I might commit it. Maybe I can just re-review it myself, now that I have forgotten all the details :)

      If messing with the email package is a problem (performance, or compatibility), another option is to keep the changes to the HTTP module (which I would be more confident in changing on my own). I have another patch for review at bpo-24363 which apparently also fixes this splitlines() bug.

    12. bitdancer commented on Aug 31, 2016

      @bitdancer
      bitdancer
      on Aug 31, 2016
      Member

      I'm hoping to take a look at all of these at the core sprint next week.

    13. bitdancer commented on Sep 7, 2016

      @bitdancer
      bitdancer
      on Sep 7, 2016
      Member

      This looks good to me. However, although it is by no means obvious, the tests in test_parser are supposed to be for the new policies. When I changed the test to test them another place that needed to fixed was revealed. I've updated the patch accordingly.

    14. python-dev commented on Sep 7, 2016

      @python-dev
      python-devmannequin
      on Sep 7, 2016
      Mannequin

      New changeset 69900c5992c5 by R David Murray in branch '3.5':
      bpo-22233: Only split headers on \r and/or \n, per email RFCs.
      https://hg.python.org/cpython/rev/69900c5992c5

      New changeset 4d2369b901be by R David Murray in branch 'default':
      Merge: bpo-22233: Only split headers on \r and/or \n, per email RFCs.
      https://hg.python.org/cpython/rev/4d2369b901be

    15. bitdancer commented on Sep 7, 2016

      @bitdancer
      bitdancer
      on Sep 7, 2016
      Member

      I want to stack another patch on top of this, so I committed it. If you see anything I screwed up, Martin, please let me know.

    16. bitdancer
      closed this as completedon Sep 7, 2016

    17. vadmium commented on Sep 7, 2016

      @vadmium
      vadmium
      on Sep 7, 2016
      Member

      Your modifications look sensible David; thanks for handling this.

    18. ezio-melotti
      transferred this issue fromon Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Metadata

    Metadata

    Assignees

    No one assigned

      Labels

      stdlibStandard Library Python modules in the Lib/ directorytopic-emailtype-bugAn unexpected behavior, bug, or error

      Projects

      No projects

        Milestone

        No milestone

        Relationships

        None yet

        Development

        No branches or pull requests

        Issue actions