bug#16220: url-http.el: Not conforming to HTTP spec

bug#16220: url-http.el: Not conforming to HTTP spec

[Jarosław Rzeszótko]

[-- Attachment #1: Type: text/plain, Size: 3411 bytes --]

Hi,

At the end of every HTTP request to be made with url-http.el and containing
a body, an unnecessary "\r\n" is appended, and additionally those two
characters are not used in the calculation of the Content-Length header.
This normally would not matter, because a carefully build server will
anyway only read "Content-Length" bytes from the body and ignore the final
CRLF, but Emacs additionally defaults to using Connection: keep-alive,
which results in the TCP traffic for what was meant to be a single request,
being interpreted as two separate HTTP requests, the first one being
roughly the intended one, and the other one consisting only of CRLF. In
particular, I am using the HTTP server from net.http in Go language. That
keepalive is enabled by default is strange, especially given how the
variable that controls this is described:

(defvar url-http-attempt-keepalives t
  "Whether to use a single TCP connection multiple times in HTTP.
This is only useful when debugging the HTTP subsystem.  Setting to
nil will explicitly close the connection to the server after every
request.")

Those issues have been somewhat discussed here, but it seems the people
discussing unfortunately don't understand HTTP:

https://groups.google.com/forum/#!msg/gnu.emacs.bug/SF4P7gVI6IQ/SExtWzutKI4J

Please just compare this discussion to
http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html

If you don't go into any fancy things like chunked encoding etc., an HTTP
request is a:

- sequence of headers, each header followed with a newline
- a newline terminating the sequence of headers
- an optional request body

The request body will be read by the server only if one of the headers is a
Content-Length header, and the value of the header is exactly the number of
bytes that is being sent, there is no CRLF terminating the body.  So if
there is no body, the request ends with two CRLFs, if there is a body, it
just ends with [Content-Length] bytes of raw data. There is no possibility
a proper HTTP server could be confused by a request being terminated with
two CRLFs, if the request is otherwise correct. I think there must have
been some confusion as to the reason of the original problem, that was then
turned into this "fix".

For reference, my code is roughly this, and as mentioned I am using
net.http from the Go language on the other end:

(defun server-command (command)
  (let* ((url-request-method "POST")
         (url-request-extra-headers '(("Content-Type" .
"application/x-www-form-urlencoded")))
         (url-request-data (concat (url-hexify-string "command") "="
(url-hexify-string (json-encode command))))
         (result-buffer (url-retrieve-synchronously server-url))
         (result
          (with-current-buffer result-buffer
            (goto-char (point-min))
            (delete-region (point-min) (search-forward "\n\n"))
            (buffer-string))))
    (kill-buffer result-buffer)
    result))

Normally I get from this function the contents of the first, "correct"
response body from the server, but if I run it a few times in quick
succession I additonally get the string "HTTP/1.1 400 Bad Request" at the
end, which is actually the second HTTP response showing up at random in the
buffer (altough it's consistently sent by the server every time, as I can
see in a sniffer).

Cheers,
Jarosław Rzeszótko

[-- Attachment #2: Type: text/html, Size: 3893 bytes --]

bug#16220: url-http.el: Not conforming to HTTP spec

[Jarosław Rzeszótko]

[-- Attachment #1: Type: text/plain, Size: 5069 bytes --]

Hi,

To turn this into a concrete proposal: I suggest this part in url-http.el
(starting line 356 in trunk):

;; End request
"\r\n"
;; Any data
url-http-data
;; If `url-http-data' is nil, avoid two CRLFs (Bug#8931).
(if url-http-data "\r\n")))

Should read simply:

;; End request
"\r\n"
;; Any data
url-http-data))

I believe that the person who originally introduced the additional newline
most likely was confused by some issue in mediawiki.el itself, a broken
HTTP server, a broken PHP script, or something of this kind, and obscured
the real bug instead of fixing it:

http://bzr.savannah.gnu.org/lh/emacs/trunk/revision/100681
http://article.gmane.org/gmane.emacs.diffs/105629

I have confirmed that HTTPS connections work just fine after removing the
new line, which is unsurprising, because this is what valid HTTP looks
like. In fact, anyone can, using https://posttestserver.com/, easily check
that the use case used as motivation for the original change, works just
fine after removing that additional newline as I suggest here:

(let ((url-request-method "POST")
      (url-request-data "action=login"))
  (url-retrieve-synchronously "https://posttestserver.com/post.php"))

Futhermore url-http-attempt-keepalives should be nil as default, or better
yet should be completely removed, as true keepalive connections are anyway
not currently supported on the Emacs side, are they?

Cheers,
Jarosław Rzeszótko


2013/12/22 Jarosław Rzeszótko <sztywny@gmail.com>

> Hi,
>
> At the end of every HTTP request to be made with url-http.el and
> containing a body, an unnecessary "\r\n" is appended, and additionally
> those two characters are not used in the calculation of the Content-Length
> header. This normally would not matter, because a carefully build server
> will anyway only read "Content-Length" bytes from the body and ignore the
> final CRLF, but Emacs additionally defaults to using Connection:
> keep-alive, which results in the TCP traffic for what was meant to be a
> single request, being interpreted as two separate HTTP requests, the first
> one being roughly the intended one, and the other one consisting only of
> CRLF. In particular, I am using the HTTP server from net.http in Go
> language. That keepalive is enabled by default is strange, especially given
> how the variable that controls this is described:
>
> (defvar url-http-attempt-keepalives t
>   "Whether to use a single TCP connection multiple times in HTTP.
> This is only useful when debugging the HTTP subsystem.  Setting to
> nil will explicitly close the connection to the server after every
> request.")
>
> Those issues have been somewhat discussed here, but it seems the people
> discussing unfortunately don't understand HTTP:
>
>
> https://groups.google.com/forum/#!msg/gnu.emacs.bug/SF4P7gVI6IQ/SExtWzutKI4J
>
> Please just compare this discussion to
> http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html
>
> If you don't go into any fancy things like chunked encoding etc., an HTTP
> request is a:
>
> - sequence of headers, each header followed with a newline
> - a newline terminating the sequence of headers
> - an optional request body
>
> The request body will be read by the server only if one of the headers is
> a Content-Length header, and the value of the header is exactly the number
> of bytes that is being sent, there is no CRLF terminating the body.  So if
> there is no body, the request ends with two CRLFs, if there is a body, it
> just ends with [Content-Length] bytes of raw data. There is no possibility
> a proper HTTP server could be confused by a request being terminated with
> two CRLFs, if the request is otherwise correct. I think there must have
> been some confusion as to the reason of the original problem, that was then
> turned into this "fix".
>
> For reference, my code is roughly this, and as mentioned I am using
> net.http from the Go language on the other end:
>
> (defun server-command (command)
>   (let* ((url-request-method "POST")
>          (url-request-extra-headers '(("Content-Type" .
> "application/x-www-form-urlencoded")))
>          (url-request-data (concat (url-hexify-string "command") "="
> (url-hexify-string (json-encode command))))
>          (result-buffer (url-retrieve-synchronously server-url))
>          (result
>           (with-current-buffer result-buffer
>             (goto-char (point-min))
>             (delete-region (point-min) (search-forward "\n\n"))
>             (buffer-string))))
>     (kill-buffer result-buffer)
>     result))
>
> Normally I get from this function the contents of the first, "correct"
> response body from the server, but if I run it a few times in quick
> succession I additonally get the string "HTTP/1.1 400 Bad Request" at the
> end, which is actually the second HTTP response showing up at random in the
> buffer (altough it's consistently sent by the server every time, as I can
> see in a sniffer).
>
> Cheers,
> Jarosław Rzeszótko
>

[-- Attachment #2: Type: text/html, Size: 6174 bytes --]

bug#16220: url-http.el: Not conforming to HTTP spec

[Ted Zlatanov]

On Sun, 22 Dec 2013 22:55:07 +0100 Jarosław Rzeszótko <sztywny@gmail.com> wrote: 

JR> To turn this into a concrete proposal: I suggest this part in url-http.el
JR> (starting line 356 in trunk):

JR> ;; End request
JR> "\r\n"
JR> ;; Any data
JR> url-http-data
JR> ;; If `url-http-data' is nil, avoid two CRLFs (Bug#8931).
JR> (if url-http-data "\r\n")))

JR> Should read simply:

JR> ;; End request
JR> "\r\n"
JR> ;; Any data
JR> url-http-data))
...
JR> Futhermore url-http-attempt-keepalives should be nil as default, or better
JR> yet should be completely removed, as true keepalive connections are anyway
JR> not currently supported on the Emacs side, are they?

Jarosław,

can you please check if your fix corrects
https://github.com/milkypostman/melpa/issues/1193 which seems somewhat related?

I don't know much about this area but your fix could help this annoying
issue as well, if it's valid.

Ted

bug#16220: url-http.el: Not conforming to HTTP spec

[Jarosław Rzeszótko]

Hi,

No, this does not seem directly related - I can't reproduce the GH
error neither with or without my fix, and from the discussion at the
end of the GH issue, it seems it has been fixed by the elpa guys by
adjusting server configuration.

As for the validity of the fix, I found the passage in the spec the
addresses this issue directly (once again from
http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html):

"Certain buggy HTTP/1.0 client implementations generate extra CRLF's
after a POST request. To restate what is explicitly forbidden by the
BNF, an HTTP/1.1 client MUST NOT preface or follow a request with an
extra CRLF."

The BNF this refers to is this:

generic-message = start-line
                  *(message-header CRLF)
                  CRLF
                  [ message-body ]
start-line      = Request-Line | Status-Line

And finally:

"When a Content-Length is given in a message where a message-body is
allowed, its field value MUST exactly match the number of OCTETs in
the message-body. HTTP/1.1 user agents MUST notify the user when an
invalid length is received and detected."

I hope this is enough of a proof that the extra newline is a bug.

Cheers,
Jarosław Rzeszótko

2013/12/22 Ted Zlatanov <tzz@lifelogs.com>:
> On Sun, 22 Dec 2013 22:55:07 +0100 Jarosław Rzeszótko <sztywny@gmail.com> wrote:
>
> JR> To turn this into a concrete proposal: I suggest this part in url-http.el
> JR> (starting line 356 in trunk):
>
> JR> ;; End request
> JR> "\r\n"
> JR> ;; Any data
> JR> url-http-data
> JR> ;; If `url-http-data' is nil, avoid two CRLFs (Bug#8931).
> JR> (if url-http-data "\r\n")))
>
> JR> Should read simply:
>
> JR> ;; End request
> JR> "\r\n"
> JR> ;; Any data
> JR> url-http-data))
> ...
> JR> Futhermore url-http-attempt-keepalives should be nil as default, or better
> JR> yet should be completely removed, as true keepalive connections are anyway
> JR> not currently supported on the Emacs side, are they?
>
> Jarosław,
>
> can you please check if your fix corrects
> https://github.com/milkypostman/melpa/issues/1193 which seems somewhat related?
>
> I don't know much about this area but your fix could help this annoying
> issue as well, if it's valid.
>
> Ted

bug#16220: url-http.el: Not conforming to HTTP spec

[Ted Zlatanov]

On Mon, 23 Dec 2013 07:51:57 +0100 Jarosław Rzeszótko <sztywny@gmail.com> wrote: 

JR> I hope this is enough of a proof that the extra newline is a bug.

I was already convinced :)

JR> 2013/12/22 Ted Zlatanov <tzz@lifelogs.com>:
>> On Sun, 22 Dec 2013 22:55:07 +0100 Jarosław Rzeszótko <sztywny@gmail.com> wrote:
>> 
JR> To turn this into a concrete proposal: I suggest this part in url-http.el
JR> (starting line 356 in trunk):
>> 
JR> ;; End request
JR> "\r\n"
JR> ;; Any data
JR> url-http-data
JR> ;; If `url-http-data' is nil, avoid two CRLFs (Bug#8931).
JR> (if url-http-data "\r\n")))
>> 
JR> Should read simply:
>> 
JR> ;; End request
JR> "\r\n"
JR> ;; Any data
JR> url-http-data))
>> ...

I am OK with this fix, if anyone else wants to look it over and commit.

JR> Futhermore url-http-attempt-keepalives should be nil as default, or better
JR> yet should be completely removed, as true keepalive connections are anyway
JR> not currently supported on the Emacs side, are they?

Not AFAIK.

Ted

bug#16220: url-http.el: Not conforming to HTTP spec

[Lars Ingebrigtsen]

Jarosław Rzeszótko <sztywny@gmail.com> writes:

> I believe that the person who originally introduced the additional
> newline most likely was confused by some issue in mediawiki.el itself,
> a broken HTTP server, a broken PHP script, or something of this kind,
> and obscured the real bug instead of fixing it:

Well, that's possible.  If there's a significant number of these broken
HTTP servers, then including the workaround is the correct thing to do.

I don't know whether that's the case, though.

-- 
(domestic pets only, the antidote for overdose, milk.)
  bloggy blog http://lars.ingebrigtsen.no/

bug#16220: url-http.el: Not conforming to HTTP spec

[Jarosław Rzeszótko]

Hi,

There are no "real" HTTP servers that require this, as evidenced by
the fact that none of the popular HTTP clients does this, just have a
look at curl source code or the http library of your favourite
programming language. Frankly, this is broken so obviously I feel
stupid further discussing it.

Cheers,
Jarosław Rzeszótko

2013/12/24 Lars Ingebrigtsen <larsi@gnus.org>:
> Jarosław Rzeszótko <sztywny@gmail.com> writes:
>
>> I believe that the person who originally introduced the additional
>> newline most likely was confused by some issue in mediawiki.el itself,
>> a broken HTTP server, a broken PHP script, or something of this kind,
>> and obscured the real bug instead of fixing it:
>
> Well, that's possible.  If there's a significant number of these broken
> HTTP servers, then including the workaround is the correct thing to do.
>
> I don't know whether that's the case, though.
>
> --
> (domestic pets only, the antidote for overdose, milk.)
>   bloggy blog http://lars.ingebrigtsen.no/

bug#16220: url-http.el: Not conforming to HTTP spec

[Stefan Monnier]

> At the end of every HTTP request to be made with url-http.el and containing
> a body, an unnecessary "\r\n" is appended, and additionally those two
> characters are not used in the calculation of the Content-Length header.

Looks like an old workaround.  Could someone get rid of this?


        Stefan

bug#16220: url-http.el: Not conforming to HTTP spec

[Jarosław Rzeszótko]

Hi,

There are more issues besides just the extra "\r\n", the url-http.el
library is super confusing. I read the documentation for url-retrieve,
which makes doing a HTTP request look really straightforward, and then
I spent several hours finding out what the hell happens. For example,
the documentation for url-retrieve says:

"The variables `url-request-data', `url-request-method' and
`url-request-extra-headers' can be dynamically bound around the
request; dynamic binding of other variables doesn't necessarily take
effect."

The problem is that url-http.el sets a lot of headers by default that
can not be overwritten in any other way then dynamically overshadowing
some variables. For example, all connections are keep-alive by
default, which is confusing in itself already, futhermore you can't
just pass "Connection" . "close' in url-request-extra-headers, because
url-http.el joins the default and "extra" headers instead of merging
the "extra" ones and having them overwrite the default ones. So try do
something like this:

(let ((url-request-method "GET")
      (url-request-extra-headers '(("Connection" . "close"))))
  (url-retrieve-synchronously "http://www.google.com/"))

And what is sent is this:

GET / HTTP/1.1
Connection: keep-alive
...
Connection: close

Which again isn't valid HTTP and the behaviour of the HTTP server in
this case is undefined and implementation specific. The only way to
workaround this is doing this:

(let ((url-http-attempt-keepalives nil)
      (url-request-method "GET")
      (url-request-extra-headers '(("Connection" . "close"))))
  (url-retrieve-synchronously "http://www.google.com/"))

Which is only clear by reading url-http.el where again it is described
in a very confusing way:

(defvar url-http-attempt-keepalives t
  "Whether to use a single TCP connection multiple times in HTTP.
This is only useful when debugging the HTTP subsystem.  Setting to
nil will explicitly close the connection to the server after every
request.")

This is all the more irritating so many of the headers are set by
default using the variables url-vars.el. Why those things are at all
variables is a mystery to me. Then there are messages that appear in
the minibuffer and there is no way to disable them. In the end it is
much easier to do HTTP requests manually using make-network-process
then it is with the url library, it really isn't a good general
purpose HTTP component, there are too many completely opaque things
happening behind the scenes, like the hashmap of persistent tcp
connections that is maintained behind the scenes. Didn't anyone else
run into problems with it?

Cheers,
Jarosław Rzeszótko


2013/12/24 Stefan Monnier <monnier@iro.umontreal.ca>:
>> At the end of every HTTP request to be made with url-http.el and containing
>> a body, an unnecessary "\r\n" is appended, and additionally those two
>> characters are not used in the calculation of the Content-Length header.
>
> Looks like an old workaround.  Could someone get rid of this?
>
>
>         Stefan

bug#16220: url-http.el: Not conforming to HTTP spec

[Stefan Monnier]

> The problem is that url-http.el sets a lot of headers by default that
> can not be overwritten in any other way then dynamically overshadowing
> some variables.

Indeed, this is ugly.  Improvements welcome.

> For example, all connections are keep-alive by
> default, which is confusing in itself already,

Not sure why it should be a problem.

> (let ((url-request-method "GET")
>       (url-request-extra-headers '(("Connection" . "close"))))
>   (url-retrieve-synchronously "http://www.google.com/"))
> And what is sent is this:
> GET / HTTP/1.1
> Connection: keep-alive
> ...
> Connection: close

> Which again isn't valid HTTP and the behaviour of the HTTP server in
> this case is undefined and implementation specific. The only way to
> workaround this is doing this:

> (let ((url-http-attempt-keepalives nil)
>       (url-request-method "GET")
>       (url-request-extra-headers '(("Connection" . "close"))))
>   (url-retrieve-synchronously "http://www.google.com/"))

Yuck!  We can probably fix this fairly easily by letting
url-request-extra-headers override (rather than just add to)
other headers.

> This is all the more irritating so many of the headers are set by
> default using the variables url-vars.el. Why those things are at all
> variables is a mystery to me.

Probably partly historical evolution (there was no place to add new
"parameters", so adding dynamic vars was an easy way to add more control
without breaking existing code).

> In the end it is much easier to do HTTP requests manually using
> make-network-process then it is with the url library,

I think that's misleading: the URL library is supposed to deal with
things like proxies and redirections, which "manual requests via
make-network-process" probably won't handle.

> Didn't anyone else run into problems with it?

Apparently not yet.  But I agree that the API might deserve a redesign
(IIRC another problem is in the way headers in the answer are returned
to the caller, which does not work consistently across different kinds
of URLs (ftp, http, file, imap, ...)).


        Stefan

bug#16220: url-http.el: Not conforming to HTTP spec

[Jarosław Rzeszótko]

[-- Attachment #1: Type: text/plain, Size: 2487 bytes --]

I attach a patch that removes the extra "\r\n", adds a function to
merge two alists, and uses it to merge the extra-headers on top of the
default-headers. I added a few basic tests, too.

Cheers,
Jarosław Rzeszótko

2014/1/2 Stefan Monnier <monnier@iro.umontreal.ca>:
>> The problem is that url-http.el sets a lot of headers by default that
>> can not be overwritten in any other way then dynamically overshadowing
>> some variables.
>
> Indeed, this is ugly.  Improvements welcome.
>
>> For example, all connections are keep-alive by
>> default, which is confusing in itself already,
>
> Not sure why it should be a problem.
>
>> (let ((url-request-method "GET")
>>       (url-request-extra-headers '(("Connection" . "close"))))
>>   (url-retrieve-synchronously "http://www.google.com/"))
>> And what is sent is this:
>> GET / HTTP/1.1
>> Connection: keep-alive
>> ...
>> Connection: close
>
>> Which again isn't valid HTTP and the behaviour of the HTTP server in
>> this case is undefined and implementation specific. The only way to
>> workaround this is doing this:
>
>> (let ((url-http-attempt-keepalives nil)
>>       (url-request-method "GET")
>>       (url-request-extra-headers '(("Connection" . "close"))))
>>   (url-retrieve-synchronously "http://www.google.com/"))
>
> Yuck!  We can probably fix this fairly easily by letting
> url-request-extra-headers override (rather than just add to)
> other headers.
>
>> This is all the more irritating so many of the headers are set by
>> default using the variables url-vars.el. Why those things are at all
>> variables is a mystery to me.
>
> Probably partly historical evolution (there was no place to add new
> "parameters", so adding dynamic vars was an easy way to add more control
> without breaking existing code).
>
>> In the end it is much easier to do HTTP requests manually using
>> make-network-process then it is with the url library,
>
> I think that's misleading: the URL library is supposed to deal with
> things like proxies and redirections, which "manual requests via
> make-network-process" probably won't handle.
>
>> Didn't anyone else run into problems with it?
>
> Apparently not yet.  But I agree that the API might deserve a redesign
> (IIRC another problem is in the way headers in the answer are returned
> to the caller, which does not work consistently across different kinds
> of URLs (ftp, http, file, imap, ...)).
>
>
>         Stefan

[-- Attachment #2: url.patch --]
[-- Type: text/x-patch, Size: 15212 bytes --]

=== modified file 'lisp/subr.el'
--- old/lisp/subr.el	2014-01-01 07:43:34 +0000
+++ new/lisp/subr.el	2014-01-03 17:57:14 +0000
@@ -464,6 +464,30 @@
 	    (aset tree i (copy-tree (aref tree i) vecp)))
 	  tree)
       tree)))
+
+(defun merge-alists (alis1 alis2 keycmp)
+  "Merges alists ALIS1 and ALIS2, non-destructively, returning
+  another alist, containing the keys from both ALIS1 and ALIS2,
+  with values from ALIS2 taking precedence. For efficiency, a
+  comparision function KEYCMP have to be supplied, the lists will
+  be sorted and then merged by having two pointers traverse the
+  two lists simulateneously."
+  (let ((alist1 (sort (copy-list alis1) keycmp))
+        (alist2 (sort (copy-list alis2) keycmp))
+        (res (list)))
+    (while (and alist1 alist2)
+      (cond ((equal (car (car alist1)) (car (car alist2)))
+             (push (car alist1) res)
+             (setq alist1 (cdr alist1))
+             (setq alist2 (cdr alist2)))
+            ((funcall keycmp (car alist1) (car alist2))
+             (push (car alist1) res)
+             (setq alist1 (cdr alist1)))
+            (t
+             (push (car alist2) res)
+             (setq alist2 (cdr alist2)))))
+    (nconc res alist1 alist2)))
+
 \f
 ;;;; Various list-search functions.
 

=== modified file 'lisp/url/url-cookie.el'
--- old/lisp/url/url-cookie.el	2014-01-01 07:43:34 +0000
+++ new/lisp/url/url-cookie.el	2014-01-03 17:50:08 +0000
@@ -208,6 +208,7 @@
 		     (if retval
 			 (concat retval "; " chunk)
 		       (concat "Cookie: " chunk)))))
+    (message (prin1-to-string retval))
     (if retval
 	(concat retval "\r\n")
       "")))

=== modified file 'lisp/url/url-http.el'
--- old/lisp/url/url-http.el	2014-01-01 07:43:34 +0000
+++ new/lisp/url/url-http.el	2014-01-03 18:04:03 +0000
@@ -209,20 +209,23 @@
 	(url-http-mark-connection-as-busy host port connection))))
 
 ;; Building an HTTP request
-(defun url-http-user-agent-string ()
+(defun url-http-user-agent ()
   (if (or (eq url-privacy-level 'paranoid)
 	  (and (listp url-privacy-level)
 	       (memq 'agent url-privacy-level)))
-      ""
-    (format "User-Agent: %sURL/%s\r\n"
-	    (if url-package-name
-		(concat url-package-name "/" url-package-version " ")
-	      "")
-	    url-version)))
+      '()
+    `(("User-agent" .
+       ,(format "%sURL/%s"
+                (if url-package-name
+                    (concat url-package-name "/" url-package-version " ")
+                  "")
+                url-version)))))
 
 (defun url-http-create-request (&optional ref-url)
   "Create an HTTP request for `url-http-target-url', referred to by REF-URL."
-  (let* ((extra-headers)
+  (let* ((default-headers)
+         (extra-headers)
+         (headers-string)
 	 (request nil)
 	 (no-cache (cdr-safe (assoc "Pragma" url-http-extra-headers)))
 	 (using-proxy url-http-proxy)
@@ -235,19 +238,13 @@
 			 (url-get-authentication url-http-proxy nil 'any nil))))
 	 (real-fname (url-filename url-http-target-url))
 	 (host (url-host url-http-target-url))
-	 (auth (if (cdr-safe (assoc "Authorization" url-http-extra-headers))
-		   nil
-		 (url-get-authentication (or
-					  (and (boundp 'proxy-info)
-					       proxy-info)
-					  url-http-target-url) nil 'any nil))))
+	 (auth (url-get-authentication (or
+                                        (and (boundp 'proxy-info)
+                                             proxy-info)
+                                        url-http-target-url) nil 'any nil)))
     (if (equal "" real-fname)
 	(setq real-fname "/"))
     (setq no-cache (and no-cache (string-match "no-cache" no-cache)))
-    (if auth
-	(setq auth (concat "Authorization: " auth "\r\n")))
-    (if proxy-auth
-	(setq proxy-auth (concat "Proxy-Authorization: " proxy-auth "\r\n")))
 
     ;; Protection against stupid values in the referrer
     (if (and ref-url (stringp ref-url) (or (string= ref-url "file:nil")
@@ -260,14 +257,56 @@
 		 (memq 'lastloc url-privacy-level)))
 	(setq ref-url nil))
 
-    ;; url-http-extra-headers contains an assoc-list of
-    ;; header/value pairs that we need to put into the request.
-    (setq extra-headers (mapconcat
-			 (lambda (x)
-			   (concat (car x) ": " (cdr x)))
-			 url-http-extra-headers "\r\n"))
-    (if (not (equal extra-headers ""))
-	(setq extra-headers (concat extra-headers "\r\n")))
+    ;; Default-headers and url-http-extra-headers are both alists of
+    ;; header/value pairs
+    (setq default-headers
+          `(("Connection" . ,(if (or using-proxy
+                                     (not url-http-attempt-keepalives))
+                                 "close" "keep-alive"))
+            ("Host" . ,(if (/= (url-port url-http-target-url)
+                               (url-scheme-get-property
+                                (url-type url-http-target-url) 'default-port))
+                           (format "%s:%d" host (url-port url-http-target-url))
+                         host))
+            ("MIME-Version" . "1.0")
+            ("Accept" . ,(or url-mime-accept-string "*/*"))            
+            ,@(when (and (not no-cache)
+                         (member url-http-method '("GET" nil)))
+                (let ((tm (url-is-cached url-http-target-url)))
+                  (if tm
+                      `(("If-modified-since" . ,(url-get-normalized-date tm))))))
+            ,@(when ref-url
+                `(("Referer" . ,ref-url)))
+            ,@(when url-personal-mail-address
+                `(("From" . ,url-personal-mail-address)))
+            ,@(when url-mime-encoding-string
+                `(("Accept-encoding" . ,url-mime-encoding-string)))
+            ,@(when url-mime-charset-string
+                `(("Accept-charset" . ,url-mime-charset-string)))
+            ,@(when url-mime-language-string
+                `(("Accept-language" . ,url-mime-language-string)))
+            ,@(when auth
+                `(("Authorization" . ,auth)))
+            ,@(when proxy-auth
+                `(("Proxy-Authorization" . ,proxy-auth)))
+            ,@(when url-http-data
+                `(("Content-length" . ,(number-to-string (length url-http-data)))))
+            ,@(when url-extensions-header
+                `(("Extension" . ,url-extensions-header)))
+            ,@(url-http-user-agent)))
+
+    ;;; url-http-extra-headers are merged on top default-headers, any
+    ;;; headers specified in both will be sent as per value in
+    ;;; url-http-extra-headers
+    (setq headers-string
+          (concat
+           (mapconcat
+            (lambda (x)
+              (concat (car x) ": " (cdr x)))
+            (merge-alists default-headers url-http-extra-headers
+                          (lambda (a b) (string-lessp (car a) (car b))))
+            "\r\n")
+           "\r\n"))
 
     ;; This was done with a call to `format'.  Concatenating parts has
     ;; the advantage of keeping the parts of each header together and
@@ -287,78 +326,21 @@
            'string-as-unibyte
            (delq nil
             (list
-             ;; The request
+             ;; The request line
              (or url-http-method "GET") " "
              (if using-proxy (url-recreate-url url-http-target-url) real-fname)
              " HTTP/" url-http-version "\r\n"
-             ;; Version of MIME we speak
-             "MIME-Version: 1.0\r\n"
-             ;; (maybe) Try to keep the connection open
-             "Connection: " (if (or using-proxy
-                                    (not url-http-attempt-keepalives))
-                                "close" "keep-alive") "\r\n"
-                                ;; HTTP extensions we support
-             (if url-extensions-header
-                 (format
-                  "Extension: %s\r\n" url-extensions-header))
-             ;; Who we want to talk to
-             (if (/= (url-port url-http-target-url)
-                     (url-scheme-get-property
-                      (url-type url-http-target-url) 'default-port))
-                 (format
-                  "Host: %s:%d\r\n" host (url-port url-http-target-url))
-               (format "Host: %s\r\n" host))
-             ;; Who its from
-             (if url-personal-mail-address
-                 (concat
-                  "From: " url-personal-mail-address "\r\n"))
-             ;; Encodings we understand
-             (if url-mime-encoding-string
-                 (concat
-                  "Accept-encoding: " url-mime-encoding-string "\r\n"))
-             (if url-mime-charset-string
-                 (concat
-                  "Accept-charset: " url-mime-charset-string "\r\n"))
-             ;; Languages we understand
-             (if url-mime-language-string
-                 (concat
-                  "Accept-language: " url-mime-language-string "\r\n"))
-             ;; Types we understand
-             "Accept: " (or url-mime-accept-string "*/*") "\r\n"
-             ;; User agent
-             (url-http-user-agent-string)
-             ;; Proxy Authorization
-             proxy-auth
-             ;; Authorization
-             auth
+             ;; Headers
+             headers-string
              ;; Cookies
-	     (when (url-use-cookies url-http-target-url)
-	       (url-cookie-generate-header-lines
-		host real-fname
-		(equal "https" (url-type url-http-target-url))))
-             ;; If-modified-since
-             (if (and (not no-cache)
-                      (member url-http-method '("GET" nil)))
-                 (let ((tm (url-is-cached url-http-target-url)))
-                   (if tm
-                       (concat "If-modified-since: "
-                               (url-get-normalized-date tm) "\r\n"))))
-             ;; Whence we came
-             (if ref-url (concat
-                          "Referer: " ref-url "\r\n"))
-             extra-headers
-             ;; Length of data
-             (if url-http-data
-                 (concat
-                  "Content-length: " (number-to-string
-                                      (length url-http-data))
-                  "\r\n"))
-             ;; End request
+             (when (url-use-cookies url-http-target-url)
+               (url-cookie-generate-header-lines
+                host real-fname
+                (equal "https" (url-type url-http-target-url))))
+             ;; End of headers
              "\r\n"
-             ;; Any data
-             url-http-data
-	     ;; If `url-http-data' is nil, avoid two CRLFs (Bug#8931).
-	     (if url-http-data "\r\n")))
+             ;; Data
+             url-http-data))
            ""))
     (url-http-debug "Request is: \n%s" request)
     request))

=== added file 'test/automated/url-http-tests.el'
--- old/test/automated/url-http-tests.el	1970-01-01 00:00:00 +0000
+++ new/test/automated/url-http-tests.el	2014-01-03 18:06:21 +0000
@@ -0,0 +1,100 @@
+;;; url-http.el --- Test suite for url-http.
+
+;; Copyright (C) 2011-2014 Free Software Foundation, Inc.
+
+;; Author: Jarosław Rzeszótko <jrzeszotko@gmail.com>
+;; Keywords: data
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Code:
+
+(require 'ert)
+(require 'url-future)
+
+(ert-deftest url-http-create-request/creates-valid-http-get-request ()
+  (let* ((url-http-extra-headers)
+         (url-http-proxy nil)      
+         (url-http-method "GET")
+         (url-http-target-url (url-generic-parse-url "http://www.gnu.org/"))
+         (url-http-data)
+         (url-package-name "XYZ")
+         (url-package-version "2.0"))
+    (with-temp-buffer
+      (insert (url-http-create-request))
+      (goto-char (point-min))
+      (should (looking-at "GET / HTTP/1.1\r\n"))
+      (should (search-forward "Accept: */*\r\n"))
+      (should (search-forward "Host: www.gnu.org\r\n"))
+      (should (search-forward "User-agent: XYZ/2.0 URL/Emacs\r\n")))))
+
+(ert-deftest url-http-create-request/sends-singleline-http-cookies ()
+  (let* ((url-http-extra-headers)
+         (url-http-proxy nil)      
+         (url-http-method "GET")
+         (url-http-target-url (url-generic-parse-url "http://www.url-http-test-host.com/"))
+         (url-http-data)
+         (url-cookie-multiple-line nil))
+    (setf (url-use-cookies url-http-target-url) t)
+    (setq url-cookie-storage nil)
+    (unwind-protect
+        (progn
+          (url-cookie-store "test1" "testvalue1testvalue1testvalue1testvalue1testvalue1" nil "www.url-http-test-host.com" "/")
+          (url-cookie-store "test2" "testvalue2" nil "www.url-http-test-host.com" "/")
+          (with-temp-buffer
+            (insert (url-http-create-request))
+            (goto-char (point-min))
+            (should (search-forward "Cookie: test1=testvalue1testvalue1testvalue1testvalue1testvalue1; test2=testvalue2\r\n"))))
+      (setq url-cookie-storage nil))))
+
+(ert-deftest url-http-create-request/sends-multiline-http-cookies ()
+  (let* ((url-http-extra-headers)
+         (url-http-proxy nil)      
+         (url-http-method "GET")
+         (url-http-target-url (url-generic-parse-url "http://www.url-http-test-host.com/"))
+         (url-http-data)
+         (url-cookie-multiple-line t)
+         (cookie-value))
+    (setf (url-use-cookies url-http-target-url) t)
+    (setq url-cookie-storage nil)
+    (unwind-protect
+        (progn
+          (url-cookie-store "test1" "testvalue1testvalue1testvalue1testvalue1testvalue1" nil "www.url-http-test-host.com" "/")
+          (url-cookie-store "test2" "testvalue2" nil "www.url-http-test-host.com" "/")
+          (with-temp-buffer
+            (insert (url-http-create-request))
+            (goto-char (point-min))
+            (should (search-forward "Cookie: test1=testvalue1testvalue1testvalue1testvalue1testvalue1\r\n"))
+            (should (search-forward "Cookie: test2=testvalue2\r\n"))))
+      (setq url-cookie-storage nil))))
+
+(ert-deftest url-http-create-request/creates-valid-http-post-request ()
+  (let* ((url-http-extra-headers)
+         (url-http-proxy nil)      
+         (url-http-method "POST")
+         (url-http-target-url (url-generic-parse-url "http://www.gnu.org/"))
+         (url-http-data "test"))
+    (with-temp-buffer
+      (insert (url-http-create-request))
+      (goto-char (point-min))
+      (should (looking-at "POST / HTTP/1.1\r\n"))
+      (should (search-forward "Accept: */*\r\n"))
+      (should (search-forward "Content-length: 4\r\n"))
+      (should (search-forward "Host: www.gnu.org\r\n"))
+      (goto-char (point-min))
+      (should (search-forward "\r\n\r\n"))
+      (should (search-forward "test"))
+      (should (equal (point) (point-max))))))

bug#16220: url-http.el: Not conforming to HTTP spec

[Lars Magne Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Probably partly historical evolution (there was no place to add new
> "parameters", so adding dynamic vars was an easy way to add more control
> without breaking existing code).

Yes.

This summer, I started rewriting url-retrieve and friends in the way
that was discussed on emacs-devel a ... couple years back:

(with-url "http://fsf.org" :timeout 10
                           :concurrency 5
                           :request-method "POST"
                           :headers '(("Foo" . "Bar"))
  (message "The result was: %s" (buffer-string)))

but I kinda stopped before I really got started, because I couldn't
figure out how to make this work in the non-lexical binding case.  And
having this work only with lexical binding seemed kinda meh.

You wouldn't happen to have any ideas in that area?  >"?
  
-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#16220: url-http.el: Not conforming to HTTP spec

[Jarosław Rzeszótko]

Hi,

2014/1/5 Lars Magne Ingebrigtsen <larsi@gnus.org>:
> This summer, I started rewriting url-retrieve and friends in the way
> that was discussed on emacs-devel a ... couple years back:
>
> (with-url "http://fsf.org" :timeout 10
>                            :concurrency 5
>                            :request-method "POST"
>                            :headers '(("Foo" . "Bar"))
>   (message "The result was: %s" (buffer-string)))
>
> but I kinda stopped before I really got started, because I couldn't
> figure out how to make this work in the non-lexical binding case.  And
> having this work only with lexical binding seemed kinda meh.
>
> You wouldn't happen to have any ideas in that area?  >"?

I think a natural way of representing the request if this was to be
designed today would be to use alists all the way, perhaps with
optional arguments for adjusting how precisely the request itself
should be sent and handled:

(http "http://www.fsf.org/xyz/zyx"
        '((method . "GET")
          (path . "/")
          (headers . (("Accept-Encoding" . "UTF-8")))
          (form . (("param1" . "value1"))))
         :timeout 10)

This breaks compatibility of course though, as do any sensible changes
I can think of. So meanwhile I would be happy if the patch got
accepted, it would at least make the library at all usable to me, even
if not the easiest to use.

Cheers,
Jarosław Rzeszótko

bug#16220: url-http.el: Not conforming to HTTP spec

[Stefan Monnier]

> but I kinda stopped before I really got started, because I couldn't
> figure out how to make this work in the non-lexical binding case.  And
> having this work only with lexical binding seemed kinda meh.

I think it's perfectly OK to have it require lexical-binding.
The macro can signal an error if lexical-binding is nil.


        Stefan

bug#16220: url-http.el: Not conforming to HTTP spec

[Jarosław Rzeszótko]

Hi,

Is there any chance my patch gets committed or at least that the extra
"\r\n" finally gets removed?

Cheers,
Jarosław Rzeszótko

2014/1/6 Stefan Monnier <monnier@iro.umontreal.ca>:
>> but I kinda stopped before I really got started, because I couldn't
>> figure out how to make this work in the non-lexical binding case.  And
>> having this work only with lexical binding seemed kinda meh.
>
> I think it's perfectly OK to have it require lexical-binding.
> The macro can signal an error if lexical-binding is nil.
>
>
>         Stefan

bug#16220: url-http.el: Not conforming to HTTP spec

[Stefan Monnier]

> Is there any chance my patch gets committed or at least that the extra
> "\r\n" finally gets removed?

I installed the patch below.
Thank you very much,


        Stefan


=== modified file 'lisp/url/ChangeLog'
--- lisp/url/ChangeLog	2014-01-01 07:43:34 +0000
+++ lisp/url/ChangeLog	2014-01-08 18:05:12 +0000
@@ -1,3 +1,8 @@
+2014-01-08  Jarosław Rzeszótko  <sztywny@gmail.com>
+
+	* url-http.el (url-http-create-request): Don't add extra \r\n after
+	http data (bug#16220).
+
 2013-12-28  Glenn Morris  <rgm@gnu.org>
 
 	* url-history.el (url-history-track):

=== modified file 'lisp/url/url-http.el'
--- lisp/url/url-http.el	2014-01-01 07:43:34 +0000
+++ lisp/url/url-http.el	2014-01-08 18:03:55 +0000
@@ -356,9 +356,7 @@
              ;; End request
              "\r\n"
              ;; Any data
-             url-http-data
-	     ;; If `url-http-data' is nil, avoid two CRLFs (Bug#8931).
-	     (if url-http-data "\r\n")))
+             url-http-data))
            ""))
     (url-http-debug "Request is: \n%s" request)
     request))

bug#16220: url-http.el: Not conforming to HTTP spec

[Paul Eggert]

Marking the bug as done since Stefan installed a patch.