Patchwork do not replace the request header names with lower case

login
register
mail settings
Submitter domruf
Date May 23, 2014, 10:57 a.m.
Message ID <1400842659147-4010765.post@n3.nabble.com>
Download mbox | patch
Permalink /patch/4855/
State Changes Requested
Headers show

Comments

domruf - May 23, 2014, 10:57 a.m.
# HG changeset patch
# User domruf <dominikruf@gmail.com>
# Date 1400792192 -7200
#      Thu May 22 22:56:32 2014 +0200
# Branch stable
# Node ID 023ef1daa66809071a1d2261bb548404e7560368
# Parent  54d7657d7d1e6a62315eea53f4498657e766bb60
do not replace the request header names with lower case

If a webserver expects a header in a certain case the request might fail.
i.e. if a (not so well writen) authentication module only accepts
"Authorization" and fails with "authorization"





--
View this message in context: http://mercurial.808500.n3.nabble.com/PATCH-do-not-replace-the-request-header-names-with-lower-case-tp4010751p4010765.html
Sent from the Development mailing list archive at Nabble.com.
Mads Kiilerich - May 23, 2014, 2:34 p.m.
> I see your point.

mercurial-devel is a mailing list. nabble is terrible and very 
unofficial frontend for posting to it. 99% of the people posting through 
it violate the mailing list etiquette, for instance by not quoting 
properly. Nabble will probably also mangle some of the patches you send. 
We recommend using the patchbomb extension.

> Since in the comment it says it is a reimplementation of
> HTTPConnection.request() (respectifly HTTPConnection._send_request()( I
> looked into that method and created a n alternative based on _send_request.
> What do you think about it?

The implementation here is better but I still don't buy it as a 
workaround for bugs in other systems. There could be other reasons to 
take it, though.

> BTW The web server where I struggle with this is github.
> I try to make the hg-git plugin work with github via https

Do you have a link the issue you have filed at github? I assume they in 
general are clueful and wants to be standard compliant and will fix the 
issue ASAP. We should assume it will be fixed before this code could be 
released.


On 05/23/2014 12:57 PM, domruf wrote:
> # HG changeset patch
> # User domruf <dominikruf@gmail.com>
> # Date 1400792192 -7200
> #      Thu May 22 22:56:32 2014 +0200
> # Branch stable
> # Node ID 023ef1daa66809071a1d2261bb548404e7560368
> # Parent  54d7657d7d1e6a62315eea53f4498657e766bb60
> do not replace the request header names with lower case

See http://mercurial.selenic.com/wiki/ContributingChanges point 1.1 . 
The topic could be "http" or "keepalive".

> If a webserver expects a header in a certain case the request might fail.
> i.e. if a (not so well writen) authentication module only accepts
> "Authorization" and fails with "authorization"

It seems like you some words.

If github is the reason you want to make the change, then you should 
mention it here. (But I don't think you should make the change for that 
reason and it is thus not relevant.)

I would like the patch if you described it as something like:

The code lowercased all header names that passed through it. HTTP 
headers are case insensitive so it was thus totally valid. But there is 
also no reason to change it unless we have to ... and it turns out that 
there is no reason to do that. Preserving the casing can for instance 
make debugging simpler. It might also make a difference for other buggy 
systems.

> diff -r 54d7657d7d1e -r 023ef1daa668 mercurial/keepalive.py
> --- a/mercurial/keepalive.py	Mon May 05 16:54:15 2014 +0200
> +++ b/mercurial/keepalive.py	Thu May 22 22:56:32 2014 +0200
> @@ -329,19 +329,19 @@
>           if sys.version_info >= (2, 4):
>               headers.update(req.unredirected_hdrs)
>           headers.update(self.parent.addheaders)
> -        headers = dict((n.lower(), v) for n, v in headers.items())
> +        headersnames = dict((n.lower(), v) for n, v in headers.items())

You do not use the values of the dict so a set would be more 
appropriate. (I would have a slight preference for a name like 
headernames or headerslow.)

>           skipheaders = {}
>           for n in ('host', 'accept-encoding'):
> -            if n in headers:
> +            if n in headersnames:
>                   skipheaders['skip_' + n.replace('-', '_')] = 1
>           try:
>               if req.has_data():
>                   data = req.get_data()
>                   h.putrequest('POST', req.get_selector(), **skipheaders)
> -                if 'content-type' not in headers:
> +                if 'content-type' not in headersnames:
>                       h.putheader('Content-type',
>                                   'application/x-www-form-urlencoded')
> -                if 'content-length' not in headers:
> +                if 'content-length' not in headersnames:
>                       h.putheader('Content-length', '%d' % len(data))
>               else:
>                   h.putrequest('GET', req.get_selector(), **skipheaders)

/Mads
Augie Fackler - May 26, 2014, 4:16 p.m.
On Fri, May 23, 2014 at 04:34:41PM +0200, Mads Kiilerich wrote:
>
> >I see your point.
>
> mercurial-devel is a mailing list. nabble is terrible and very unofficial
> frontend for posting to it. 99% of the people posting through it violate the
> mailing list etiquette, for instance by not quoting properly. Nabble will
> probably also mangle some of the patches you send. We recommend using the
> patchbomb extension.
>
> >Since in the comment it says it is a reimplementation of
> >HTTPConnection.request() (respectifly HTTPConnection._send_request()( I
> >looked into that method and created a n alternative based on _send_request.
> >What do you think about it?
>
> The implementation here is better but I still don't buy it as a workaround
> for bugs in other systems. There could be other reasons to take it, though.
>
> >BTW The web server where I struggle with this is github.
> >I try to make the hg-git plugin work with github via https
>
> Do you have a link the issue you have filed at github? I assume they in
> general are clueful and wants to be standard compliant and will fix the
> issue ASAP. We should assume it will be fixed before this code could be
> released.
>
>
> On 05/23/2014 12:57 PM, domruf wrote:
> ># HG changeset patch
> ># User domruf <dominikruf@gmail.com>
> ># Date 1400792192 -7200
> >#      Thu May 22 22:56:32 2014 +0200
> ># Branch stable
> ># Node ID 023ef1daa66809071a1d2261bb548404e7560368
> ># Parent  54d7657d7d1e6a62315eea53f4498657e766bb60
> >do not replace the request header names with lower case
>
> See http://mercurial.selenic.com/wiki/ContributingChanges point 1.1 . The
> topic could be "http" or "keepalive".
>
> >If a webserver expects a header in a certain case the request might fail.
> >i.e. if a (not so well writen) authentication module only accepts
> >"Authorization" and fails with "authorization"
>
> It seems like you some words.
>
> If github is the reason you want to make the change, then you should mention
> it here. (But I don't think you should make the change for that reason and
> it is thus not relevant.)
>
> I would like the patch if you described it as something like:
>
> The code lowercased all header names that passed through it. HTTP headers
> are case insensitive so it was thus totally valid. But there is also no
> reason to change it unless we have to ... and it turns out that there is no
> reason to do that. Preserving the casing can for instance make debugging
> simpler. It might also make a difference for other buggy systems.

I'd be +1 on this change - this justification is obvious to me, but
I've spent more time than is healthy in RFC 2616. I eagerly await a
resend with that kind of documentation.

>
> >diff -r 54d7657d7d1e -r 023ef1daa668 mercurial/keepalive.py
> >--- a/mercurial/keepalive.py	Mon May 05 16:54:15 2014 +0200
> >+++ b/mercurial/keepalive.py	Thu May 22 22:56:32 2014 +0200
> >@@ -329,19 +329,19 @@
> >          if sys.version_info >= (2, 4):
> >              headers.update(req.unredirected_hdrs)
> >          headers.update(self.parent.addheaders)
> >-        headers = dict((n.lower(), v) for n, v in headers.items())
> >+        headersnames = dict((n.lower(), v) for n, v in headers.items())
>
> You do not use the values of the dict so a set would be more appropriate. (I
> would have a slight preference for a name like headernames or headerslow.)
>
> >          skipheaders = {}
> >          for n in ('host', 'accept-encoding'):
> >-            if n in headers:
> >+            if n in headersnames:
> >                  skipheaders['skip_' + n.replace('-', '_')] = 1
> >          try:
> >              if req.has_data():
> >                  data = req.get_data()
> >                  h.putrequest('POST', req.get_selector(), **skipheaders)
> >-                if 'content-type' not in headers:
> >+                if 'content-type' not in headersnames:
> >                      h.putheader('Content-type',
> >                                  'application/x-www-form-urlencoded')
> >-                if 'content-length' not in headers:
> >+                if 'content-length' not in headersnames:
> >                      h.putheader('Content-length', '%d' % len(data))
> >              else:
> >                  h.putrequest('GET', req.get_selector(), **skipheaders)
>
> /Mads
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff -r 54d7657d7d1e -r 023ef1daa668 mercurial/keepalive.py
--- a/mercurial/keepalive.py	Mon May 05 16:54:15 2014 +0200
+++ b/mercurial/keepalive.py	Thu May 22 22:56:32 2014 +0200
@@ -329,19 +329,19 @@ 
         if sys.version_info >= (2, 4):
             headers.update(req.unredirected_hdrs)
         headers.update(self.parent.addheaders)
-        headers = dict((n.lower(), v) for n, v in headers.items())
+        headersnames = dict((n.lower(), v) for n, v in headers.items())
         skipheaders = {}
         for n in ('host', 'accept-encoding'):
-            if n in headers:
+            if n in headersnames:
                 skipheaders['skip_' + n.replace('-', '_')] = 1
         try:
             if req.has_data():
                 data = req.get_data()
                 h.putrequest('POST', req.get_selector(), **skipheaders)
-                if 'content-type' not in headers:
+                if 'content-type' not in headersnames:
                     h.putheader('Content-type',
                                 'application/x-www-form-urlencoded')
-                if 'content-length' not in headers:
+                if 'content-length' not in headersnames:
                     h.putheader('Content-length', '%d' % len(data))
             else:
                 h.putrequest('GET', req.get_selector(), **skipheaders)