[urllib.request]: Comparison of HTTP headers should be insensitive to the case

@orsenthil

BPO	5550
Nosy	@orsenthil, @karlcow, @vadmium, @1st1
Dependencies	bpo-18986: Add a case-insensitive case-preserving dict
Superseder	bpo-2275: Make urllib.request.Request.has_header() etc case-insensitive
Files	issue-5550-test-1.patch issue-5550-2.patch issue5550-5.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 = 'https://github.com/orsenthil'
closed_at = <Date 2015-11-05.06:50:43.029>
created_at = <Date 2009-03-24.04:48:59.352>
labels = ['type-bug', 'library']
title = '[urllib.request]: Comparison of HTTP headers should be insensitive to the case'
updated_at = <Date 2015-11-05.06:51:52.392>
user = 'https://bugs.python.org/cocobear'

bugs.python.org fields:

activity = <Date 2015-11-05.06:51:52.392>
actor = 'martin.panter'
assignee = 'orsenthil'
closed = True
closed_date = <Date 2015-11-05.06:50:43.029>
closer = 'martin.panter'
components = ['Library (Lib)']
creation = <Date 2009-03-24.04:48:59.352>
creator = 'cocobear'
dependencies = ['18986']
files = ['29505', '29511', '36717']
hgrepos = []
issue_num = 5550
keywords = ['patch']
message_count = 19.0
messages = ['84058', '184684', '184768', '184790', '184799', '185402', '185441', '220885', '221401', '227348', '227353', '227411', '227418', '227501', '227646', '227648', '239651', '254050', '254085']
nosy_count = 6.0
nosy_names = ['orsenthil', 'cocobear', 'karlcow', 'BreamoreBoy', 'martin.panter', 'yselivanov']
pr_nums = []
priority = 'normal'
resolution = 'duplicate'
stage = 'test needed'
status = 'closed'
superseder = '2275'
type = 'behavior'
url = 'https://bugs.python.org/issue5550'
versions = ['Python 2.7', 'Python 3.2', 'Python 3.3', 'Python 3.4']

take a look at following code:
<code>
import urllib2

headers = [("Content-Type","application/oct-stream"),]
opener = urllib2.build_opener()
opener.addheaders = headers
urllib2.install_opener(opener)
print "after install_opener"

ret = opener.open('http://www.google.com',data="word=ss")
print ret.read()
</code>

I got real send data by wireshark：

POST / HTTP/1.1

Accept-Encoding: identity

Content-Length: 7

Host: www.dict.cn

Content-Type: application/x-www-form-urlencoded

Connection: close

word=ss

I had already set HTTP header of Content-Type, but actally urllib2
change this header.

I got this piece of code from urllib2.py:

       if request.has_data():  # POST
           data = request.get_data()
           if not request.has_header('Content-type'):
               request.add_unredirected_header(
                   'Content-type',
                   'application/x-www-form-urlencoded')
           if not request.has_header('Content-length'):
               request.add_unredirected_header(
                   'Content-length', '%d' % len(data))

   scheme, sel = splittype(request.get_selector())
   sel_host, sel_path = splithost(sel)
   if not request.has_header('Host'):
       request.add_unredirected_header('Host', sel_host or host)
   for name, value in self.parent.addheaders:
       name = name.capitalize()
       if not request.has_header(name):
           request.add_unredirected_header(name, value)

first,urllib2 will add Content-Type if it's POST method, then it try to
add headers which holded by opener, but there's already 'Content-Type',
so the attribute->addheaders of opener will not append.

I can use Request to add 'Content-Type' header：

 request = urllib2.Request(url,headers=headers,data=body)

The example code that I wrote doesn't correct? or it's a problem of urllib2?

The wireshark trace is a different domain than the code example. But let's see. cocobear added:

headers = [("Content-Type","application/oct-stream"),]

with a "Content-Type", not the capitalized "Type".

BUT in the source code or urllib/request.py there is
http://hg.python.org/cpython/file/3.3/Lib/urllib/request.py#l1184

if not request.has_header('Content-type')

And if you check has_header at
http://hg.python.org/cpython/file/3.3/Lib/urllib/request.py#l367

it compares exactly the string it has received, when HTTP headers are case insensitive. It reminds me of

http://bugs.python.org/issue12455
http://bugs.python.org/issue17322

with capitalization issues.

I'm changing the title to be more exact.

orsenthil,

Added test for python 3.3 for testing the case insensitivity for has_header and get_header. The tests fail as it should.
See issue-5550-test-1.patch

I will make a patch to urllib/request.py to make it work.
OK with you?

First, Sanity check for myself to be sure to understand.
==============================================

→ python3.3
Python 3.3.0 (v3.3.0:bd8afb90ebf2, Sep 29 2012, 01:25:11) 
[GCC 4.2.1 (Apple Inc. build 5666) (dot 3)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import urllib.request
>>> req = urllib.request.Request('http://www.python.org/')
>>> req.headers
{}
>>> req.unredirected_hdrs
{}
>>> r = urllib.request.urlopen(req)
>>> req.headers
{}
>>> req.unredirected_hdrs
{'User-agent': 'Python-urllib/3.3', 'Host': 'www.python.org'}
>>> req.header_items()
[('User-agent', 'Python-urllib/3.3'), ('Host', 'www.python.org')]
>>> req.has_header('host')
False
>>> req.has_header('Host')
True
>>> req.get_header('host')
>>> req.get_header('Host')
'www.python.org'
>>> 'host' in req.unredirected_hdrs
False
>>> 'Host' in req.unredirected_hdrs
True
>>> 'host' in req.header_items()
False
>>> 'Host' in req.header_items()
False
>>> req.unredirected_hdrs.items()
dict_items([('User-agent', 'Python-urllib/3.3'), ('Host', 'www.python.org')])
>>> req.headers.get('Host', req.unredirected_hdrs.get('Host',None))
'www.python.org'
>>> req.headers.get('Host')
>>> req.headers.get('host'.capitalize(), req.unredirected_hdrs.get('host'.capitalize(),None))
'www.python.org'
>>> req.headers.get('HOST'.capitalize(), req.unredirected_hdrs.get('HOST'.capitalize(),None))
'www.python.org'
>>> req.headers.get('host'.title(), req.unredirected_hdrs.get('host'.title(),None))
'www.python.org'
>>> 'host'.capitalize() in req.unredirected_hdrs
True

==============================================

OK. The two add methods force the capitalization thought capitalize() (And not title() which is an issue by itself)

http://hg.python.org/cpython/file/3.3/Lib/urllib/request.py#l359

    def add_header(self, key, val):
        # useful for something like authentication
        self.headers[key.capitalize()] = val

    def add_unredirected_header(self, key, val):
        # will not be added to a redirected request
        self.unredirected_hdrs[key.capitalize()] = val

HTTP headers are case insensitive. The way the methods get_header() and has_header() are currently designed. We could also capitalize the variable before requesting it.

So something like

    def get_header(self, header_name, default=None):
        return self.headers.get(
            header_name.capitalize(),
            self.unredirected_hdrs.get(header_name.capitalize(), default))


    def has_header(self, header_name):
        return (header_name.capitalize() in self.headers or
                header_name.capitalize() in self.unredirected_hdrs)

The method to add headers on request is

>>> req.add_header("foo-bar","booh")
>>> req.headers
{'Foo-bar': 'booh'}
>>> req.unredirected_hdrs
{'User-agent': 'Python-urllib/3.3', 'Host': 'www.python.org'}
>>> req.header_items()
[('Foo-bar', 'booh'), ('User-agent', 'Python-urllib/3.3'), ('Host', 'www.python.org')]

So if someone add an header it will be capitalized. And the query will be correct.

The issue is more with addheader which doesn't have the same constraint.
http://hg.python.org/cpython/file/3.3/Lib/urllib/request.py#l1624

Personally I would have made everything case insensitive ;)

Also note that in this module the casing is not consistent when the values are hardcoded. Sometimes Content-Type, sometimes Content-type.

Anyway I submitted a patch with the code modification AND the test. And this is the result when running the test suite.

→ ./python.exe Lib/test/test_urllib2net.py
test_sni (main.HTTPSTests) ... skipped 'test disabled - test server needed'
test_custom_headers (main.OtherNetworkTests) ... ok
test_file (main.OtherNetworkTests) ... ok
test_ftp (main.OtherNetworkTests) ... ok
test_headers_case_sensitivity (main.OtherNetworkTests) ... ok
test_sites_no_connection_close (main.OtherNetworkTests) ... /Users/karl/Documents/2011/cpython/Lib/socket.py:370: ResourceWarning: unclosed <socket.socket object, fd=5, family=2, type=1, proto=6>
self._sock = None
ok
test_urlwithfrag (main.OtherNetworkTests) ... ok
test_close (main.CloseSocketTest) ... ok
test_ftp_basic (main.TimeoutTest) ... ok
test_ftp_default_timeout (main.TimeoutTest) ... ok
test_ftp_no_timeout (main.TimeoutTest) ... ok
test_ftp_timeout (main.TimeoutTest) ... ok
test_http_basic (main.TimeoutTest) ... ok
test_http_default_timeout (main.TimeoutTest) ... ok
test_http_no_timeout (main.TimeoutTest) ... ok
test_http_timeout (main.TimeoutTest) ... ok

----------------------------------------------------------------------
Ran 16 tests in 15.259s

OK (skipped=1)
[137983 refs]

Karl - a patch is welcome. BTW, I wanted to search for an already existing and I think open issue with a patch ( probably written by me) somewhere. We have to see what's the diff of that issue with new one. I will share that issue in this id. But yeah, this is a good discussion and +1 for this fix.

Case insensitive lookup will be more fool-proof than capitalized look up.
I would in favor of going with case insensitive look up than the capitalized look up one. Because the headers could be added in multiple ways and we cannot be assured that they are all stored as Capitalized header values.

Yes case insensitive make sense. It seems it will require a lot of modifications in the code. So I guess maybe the first step is to identify where it could break.

@karl

@karl do you intend following up on this issue?

@mark

@mark,

yup, I can do that.
I just realized that since my contribution there was a PSF Contributor Agreement. This is signed.

I need to dive a bit again in the code to remember where things fail.

Ok this is an attempt at solving the issue with lowercase. I find my get_header a bit complicated, but if you have a better idea. :) I'll modify the patches.

I have try to run the tests on the mac here but I have an issue currently.

→ ./python.exe -V
Python 3.5.0a0

Traceback (most recent call last):
  File "./Tools/scripts/patchcheck.py", line 6, in <module>
    import subprocess
  File "/Users/karl/code/cpython/Lib/subprocess.py", line 353, in <module>
    import signal
ImportError: No module named 'signal'
make: *** [patchcheck] Error 1

And I had to do a typo in patch3. Submitting patch4. Sorry about that.

Ok my tests are ok.

→ ./python.exe -m unittest -v Lib/test/test_urllib2net.py

test_close (Lib.test.test_urllib2net.CloseSocketTest) ... ok
test_custom_headers (Lib.test.test_urllib2net.OtherNetworkTests) ... FAIL
test_file (Lib.test.test_urllib2net.OtherNetworkTests) ... test_ftp (Lib.test.test_urllib2net.OtherNetworkTests) ... skipped "Resource 'ftp://gatekeeper.research.compaq.com/pub/DEC/SRC/research-reports/00README-Legal-Rules-Regs' is not available"
test_headers_case_sensitivity (Lib.test.test_urllib2net.OtherNetworkTests) ... ok
test_redirect_url_withfrag (Lib.test.test_urllib2net.OtherNetworkTests) ... ok
test_sites_no_connection_close (Lib.test.test_urllib2net.OtherNetworkTests) ... ok
test_urlwithfrag (Lib.test.test_urllib2net.OtherNetworkTests) ... ok
test_ftp_basic (Lib.test.test_urllib2net.TimeoutTest) ... ok
test_ftp_default_timeout (Lib.test.test_urllib2net.TimeoutTest) ... ok
test_ftp_no_timeout (Lib.test.test_urllib2net.TimeoutTest) ... ok
test_ftp_timeout (Lib.test.test_urllib2net.TimeoutTest) ... ok
test_http_basic (Lib.test.test_urllib2net.TimeoutTest) ... ok
test_http_default_timeout (Lib.test.test_urllib2net.TimeoutTest) ... ok
test_http_no_timeout (Lib.test.test_urllib2net.TimeoutTest) ... ok
test_http_timeout (Lib.test.test_urllib2net.TimeoutTest) ... ok

I wonder if test_custom_headers fails because of my modifications.

Opened issue bpo-22478 for the tests failing. Not related to my modification.

OK after fixing my repo (Thanks orsenthil) I got the tests running properly. The inspection order of the two dictionary was not right, so I had to modify a bit the patch.

→ ./python.exe -m unittest -v
Lib.test.test_urllib2net.OtherNetworkTests.test_headers_case_sensitivity
test_headers_case_sensitivity (Lib.test.test_urllib2net.OtherNetworkTests) ... ok

----------------------------------------------------------------------
Ran 1 test in 0.286s

OK

→ ./python.exe -m unittest -v
Lib.test.test_urllib2net.OtherNetworkTests.test_custom_headers
test_custom_headers (Lib.test.test_urllib2net.OtherNetworkTests) ... ok

----------------------------------------------------------------------
Ran 1 test in 0.575s

OK

New patch bpo-5550-5.patch
unlinking bpo-5550-4.patch

I left some comments in the codereview.

I think that having some half-baked solution is great, but I really would like to see a proper fix, i.e. with remove_header and other methods fixed. Ideally, you should add a UserDict implementation for headers that implements proper __XXXitem__ methods.

Ideally, we should just wait when PEP-455 lands, so we can use TransformDict for headers.

Also, I don't think we can land a fix for this in any pythons out there, I would focus on making this right in 3.5

According to <https://bugs.python.org/issue18986#msg236161\> that PEP isn’t going to be accepted, although no reason seems to have been given. So I wouldn’t hold back because of that.

Regarding the original bug reported by Cocobear, I think this is by design. The “addheaders” attribute seems to serve as a set of default header fields, although this is hardly obvious from the documentation. In fact the current documentation suggests you have to use the application/x-www-form-urlencoded content type for request data. I think the documentation should be relaxed; I opened bpo-23360 about that.

The rest of the thread seems to have side-tracked into patches to make Request.has_header() and friends case insensitive. I’ll have to study the threads closer, but I think this side-track is a duplicate of bpo-2275.

The patch here does the same thing as John’s bpo-2775.patch in bpo-2275. I am closing this thread, because I think John’s patch in the other one is superior (except for being way out of date).

Navigation Menu

Repository navigation

[urllib.request]: Comparison of HTTP headers should be insensitive to the case #49800

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

[urllib.request]: Comparison of HTTP headers should be insensitive to the case #49800

Description

Activity

cocobear commented on Mar 24, 2009

karlcow commented on Mar 19, 2013

karlcow commented on Mar 20, 2013

karlcow commented on Mar 20, 2013

orsenthil commented on Mar 20, 2013

orsenthil commented on Mar 28, 2013

karlcow commented on Mar 28, 2013

BreamoreBoy commented on Jun 17, 2014

karlcow commented on Jun 24, 2014

karlcow commented on Sep 23, 2014

karlcow commented on Sep 23, 2014

karlcow commented on Sep 24, 2014

karlcow commented on Sep 24, 2014

karlcow commented on Sep 25, 2014

1st1 commented on Sep 26, 2014

1st1 commented on Sep 26, 2014

vadmium commented on Mar 31, 2015

vadmium commented on Nov 4, 2015

vadmium commented on Nov 5, 2015

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions