bug#24117: 25.1; url-http-create-request: Multibyte text in HTTP request

[Dmitry Gutov <dgutov@yandex.ru>]

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

On 08/10/2016 05:35 PM, Eli Zaretskii wrote:

> Are you saying that url-generic-parse-url performs this encoding, and
> that using a unibyte buffer causes that to fail?

No, url-generic-parse-url contains logic that allows to distinguish 
between the domain and the path parts of an URL. So apparently it might 
have to work on multibyte URLs.

That's not strictly necessary, however, given how url-encode-url uses it 
currently (it performs encode-coding-string and decode-coding-string on 
the URL string).

That approach seems flawed to me, but either way, someone will have to 
choose how url-encode-url should use url-generic-parse-url. If we intend 
to leave it as-is, then the proposed patch using set-buffer-multibyte 
actually works fine, even on master, with multibyte URLs.

>> So I think the encoding of the URL parts should be performed inside
>> url-http-create-request.
>
> Fine with me, but when I suggested that, you didn't like the
> suggestion.  If you changed your mind, let's do that.

See below. But yes, I'm more inclined toward this approach now, after 
Lar's objection, and after looking at the code in master.

>> On the master branch, host is passed through IDNA encoding, but
>> real-fname is untouched. On emacs-25, I think we should convert both
>> to unibyte.
>
> Not sure I understand why there should be a difference between the two
> branches.  Encoding an ASCII string doesn't do any harm.

Since it's ASCII, using utf-8 there seems misleading to me. It's a 
question of readability. As a bonus, using us-ascii will validate that 
the strings indeed do not contain any unexpected characters.

>> (Why doesn't (encode-coding-string "aaaa" 'ascii) work?)
>
> It's 'us-ascii, not 'ascii.

Thanks. Attaching a patch, it seems to work well enough.

I'd like to wait for Lar's response now, but someone will have to make 
an executive decision. Both patches (this and the set-multibyte-buffer-p 
one), work in the cases I've tested.

This one seems more conservative, but it'll require a manual merge to 
master. The other one is very trivial, will merge automatically, but 
might cause problems for potential less-careful uses of 
url-generic-parse-url.

[-- Attachment #2: url-http--encode-string.diff --]
[-- Type: text/x-patch, Size: 1359 bytes --]

diff --git a/lisp/url/url-http.el b/lisp/url/url-http.el
index 7156e6f..860e652 100644
--- a/lisp/url/url-http.el
+++ b/lisp/url/url-http.el
@@ -235,7 +235,7 @@ url-http-create-request
 			      'url-http-proxy-basic-auth-storage))
 			 (url-get-authentication url-http-proxy nil 'any nil))))
 	 (real-fname (url-filename url-http-target-url))
-	 (host (url-host url-http-target-url))
+	 (host (url-http--encode-string (url-host url-http-target-url)))
 	 (auth (if (cdr-safe (assoc "Authorization" url-http-extra-headers))
 		   nil
 		 (url-get-authentication (or
@@ -278,7 +278,8 @@ url-http-create-request
           (concat
              ;; The request
              (or url-http-method "GET") " "
-             (if using-proxy (url-recreate-url url-http-target-url) real-fname)
+             (url-http--encode-string
+              (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"
@@ -360,6 +361,11 @@ url-http-create-request
     (url-http-debug "Request is: \n%s" request)
     request))
 
+(defun url-http--encode-string (s)
+  (if (multibyte-string-p s)
+      (encode-coding-string s 'us-ascii)
+    s))
+
 ;; Parsing routines
 (defun url-http-clean-headers ()
   "Remove trailing \r from header lines.