Subject: Re: bug#78984: 31.0.50; `url-build-query-string' fails to escape a literal `%' in keys and values

From: Steven Allen <[email-masked]>
Date: Thu, 17 Jul 2025 13:01:45 -0700

Eli Zaretskii <[email-masked]> writes:
>> That sounds like a reasonable solution for `url-host-allowed-chars',
>> `url-path-allowed-chars', `url-query-allowed-chars', and
>> `url-query-key-value-allowed-chars':
>>
>> 1. Allowing `%' is "correct" as `%' is, in fact, allowed in URLs.
>>
>> 2. These variables are used in conjunction with `url-hexify-string' to
>> escape strings that may already be partially %-encoded.
>>
>> 3. From what I can tell, the documentation in these cases is technically
>> correct already, if a bit misleading.
>>
>> However, IMO, `url-build-query-string' should escape everything,
>> including literal `%' characters:
>>
>> 1. `url-parse-query-string', the inverse of `url-build-query-string',
>> decodes %-encoded strings. In order for these two functions to
>> round-trip, `url-build-query-string' needs to escape `%'. Specifically,
>> the following test should pass:
>>
>>     (ert-deftest url-query-string-round-trips ()
>>       (let* ((query-string "foo=%25%3D")
>>              (parsed-query (url-parse-query-string query-string))
>>              (round-trip (url-build-query-string parsed-query)))
>>         (should (equal round-trip query-string))))
>>
>> 2. `url-build-query-string' is a higher-level tool to transform structured
>> data into a query string. IMO, the user shouldn't ahve to think about
>> pre-escaping their strings here.
>>
>> I'm happy to provide patches (both to improve the docs and fix this
>> issue) if you think this is a reasonable approach.
>
> The situation you described sounds like a contradiction to me: some
> use cases need url-build-query-string to have literal % encoded as
> %25, and others need the literal % NOT encoded.  In particular, you
> originally said that the root cause of the problem was that
> url-path-allowed-chars allows '%', but now you say that
> url-build-query-string should encode literal '%', although
> url-path-allowed-chars is okay as it is?  So I don't understand what
> kind of a solution you have in mind; perhaps I'm missing something?
>
> I guess I'm confused.
>
> Thanks.

I apologize, I shouldn't have called that the root cause.
`url-path-allowed-chars' allowing % isn't incorrect, it's just the
ultimate reason `url-build-query-string' passes through literal %
characters without encoding them. As a matter of fact, this
(`url-path-allowed-chars') cannot be changed without breaking
`url-path-and-query' and `url-encode-url' as I will explain below:

First, `url-path-and-query' must not %-decode the path and the query as
that would make it impossible to parse them correctly. E.g.,
"/some%2fpath" must not be decoded as "/some/path" before splitting the
path into components and the same goes for query strings with respect to
& and =. Specifically, I'm referring to the following in `url-encode-url':

    (let* ...
         (path-and-query (url-path-and-query obj))

Therefore, `url-encode-url' must not %-encode literal % characters when
encoding the path and query because they were not (nor could they have
been) %-decoded by `url-path-and-query'. Specifically, I'm referring to
the following in `url-encode-url':

    (if path
	(setq path (url-hexify-string path url-path-allowed-chars)))
    (if query
	(setq query (url-hexify-string query url-query-allowed-chars)))

That's why `url-path-allowed-chars' and `url-query-allowed-chars' must
allow literal % characters without re-encoding them.

However, `url-build-query-string' and `url-parse-query-string' are
different. Unlike `url-path-and-query', `url-parse-query-string' decodes
the query into a structured object (a key/values alist) and can (and
does) completely %-decode the keys & values. That means the inverse,
`url-build-query-string', should assume that its inputs are not
already %-encoded and should %-encode literal % characters.

I've attached a patch to show you what I mean. In this patch, I
introduced a new `url--query-key-value-preserved-chars' constant,
separate from `url-query-key-value-allowed-chars', to keep
`url-query-key-value-allowed-chars' consistent with the other
`-allowed-chars' constants. That is, while % is ALLOWED to appear in
encoded query keys and values, it shouldn't be PRESERVED when %-escaping
them.

(I'm also happy to change `url-query-key-value-allowed-chars' directly
if that's what you'd prefer)

>From 613098b7718d7e8b48f6c40bbcdffb0cb850a347 Mon Sep 17 00:00:00 2001
From: Steven Allen <[email-masked]>
Date: Thu, 17 Jul 2025 11:14:45 -0700
Subject: [PATCH] %-encode literal % characters when building query strings

When building a query string via `url-build-query-string', %-encode
literal % characters appearing in both the keys and the values.

* lisp/url/url-util.el (url--query-key-value-preserved-chars): Define a
new constant based on `url-query-key-value-allowed-chars' specifying
the characters that should be preserved when %-encoding query-string
keys and values.
(url-build-query-string): Use the new constant (fixes Bug#78984).
* test/lisp/url-url-test-tests.el (url-util-tests): Add a test.
---
 lisp/url/url-util.el            | 9 ++++++++-
 test/lisp/url/url-util-tests.el | 8 ++++----
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/lisp/url/url-util.el b/lisp/url/url-util.el
index 8435114bbc2..47ef8811c5e 100644
--- a/lisp/url/url-util.el
+++ b/lisp/url/url-util.el
@@ -409,6 +409,13 @@ url-encode-url
 	      (url-hexify-string frag url-query-allowed-chars)))
     (url-recreate-url obj)))
 
+(defconst url--query-key-value-preserved-chars
+  (let ((vec (copy-sequence url-query-key-value-allowed-chars)))
+    (aset vec ?% nil)
+    vec)
+  "Byte mask used for %-encoding keys and values in the query segment of a URI.
+`url-query-key-value-allowed-chars' minus '%'.")
+
 ;;;###autoload
 (defun url-build-query-string (query &optional semicolons keep-empty)
   "Build a query-string.
@@ -436,7 +443,7 @@ url-build-query-string
      (let ((escaped
             (mapcar (lambda (sym)
                       (url-hexify-string (format "%s" sym)
-                                         url-query-key-value-allowed-chars))
+                                         url--query-key-value-preserved-chars))
                     key-vals)))
        (mapconcat (lambda (val)
                     (let ((vprint (format "%s" val))
diff --git a/test/lisp/url/url-util-tests.el b/test/lisp/url/url-util-tests.el
index d34bfc8509c..6e79a798333 100644
--- a/test/lisp/url/url-util-tests.el
+++ b/test/lisp/url/url-util-tests.el
@@ -33,10 +33,10 @@ url-util-tests
             ((key1 "val1") (key2 val2) (key3 val1 val2) ("key4") (key5 "")) t)
            ("key1=val1;key2=val2;key3=val1;key3=val2;key4=;key5="
             ((key1 val1) (key2 val2) ("key3" val1 val2) (key4) (key5 "")) t t)
-           ("key1=val/slash;key2=val%3Bsemi;key3=val%26amp;key4=val%3Deq"
-            ((key1 "val/slash") (key2 "val;semi") (key3 "val&amp") (key4 "val=eq")) t)
-           ("key%3Deq=val1;key%3Bsemi=val2;key%26amp=val3"
-            (("key=eq" val1) ("key;semi" val2) ("key&amp" val3)) t)))
+           ("key1=val/slash;key2=val%3Bsemi;key3=val%26amp;key4=val%3Deq;key5=val%25perc"
+            ((key1 "val/slash") (key2 "val;semi") (key3 "val&amp") (key4 "val=eq") (key5 "val%perc")) t)
+           ("key%3Deq=val1;key%3Bsemi=val2;key%26amp=val3;key%25perc=val4"
+            (("key=eq" val1) ("key;semi" val2) ("key&amp" val3) ("key%perc" val4)) t)))
         test)
     (while tests
       (setq test (car tests)
-- 
2.50.1