Subject: Re: url-http.el: Not conforming to HTTP spec

From: Jarosław Rzeszótko <[email-masked]>
Date: Sun, 22 Dec 2013 22:55:07 +0100

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 <[email-masked]>

> 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
>