1064481 - DOM searchParams setter doesn't encode correctly

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Benjamin Smedberg]

Testcase:

Visit a site like https://developer.mozilla.org/
In the browser console:
window.location.searchParams.set("test", "foo\nbar");

Expected:
window.location.search == "foo%0Abar"

Actual:
window.location.search == "foo%Abar"

This is then misinterpreted on future loads so .searchParams doesn't round-trip successfully.

Nominating for tracking because shipping a broken .searchParams impl might be bad (from bug 1037715).

Comment 1

[Jeff Walden [:Waldo]]

https://mxr.mozilla.org/mozilla-central/source/dom/base/URLSearchParams.cpp#352

Something smells rotten about that code having to implement its own percent-encoding algorithm.

Comment 2

[Boris Zbarsky [:bzbarsky]]

It's trying to implement a very opinionated algorithm from the spec, which doesn't match any other %-encoding thing we have.  :(

But yeah, this needs to append 0 explicitly when *p < 0x10.

Comment 3

[Jeff Walden [:Waldo]]

"%%%02X" is probably marginally better, assuming it works (which I'm fairly sure it does).

Comment 4

[Andrea Marchesini [:baku]]

Attachment: searchparams.patch

Comment 5

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #2)
> It's trying to implement a very opinionated algorithm from the spec, which
> doesn't match any other %-encoding thing we have.  :(

Hmm, which algorithm is that?  <http://url.spec.whatwg.org/#percent-encode> seems fine.

Comment 6

[Jeff Walden [:Waldo]]

The searchParam update steps:

http://url.spec.whatwg.org/#concept-uq-update

which do serialization:

http://url.spec.whatwg.org/#concept-urlencoded-serializer

which convert to UTF-8 (3.1), then perform serialization on the bytes of that conversion:

http://url.spec.whatwg.org/#concept-urlencoded-byte-serializer

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Hmm, which algorithm is that?

http://url.spec.whatwg.org/#concept-urlencoded-byte-serializer which says which exact bytes the %-encoding should be applied to and which ones are passed through as-is.  Last I checked it didn't match any of our existing nsEscape behavior flags, so we couldn't use nsEscape here.  :(

Comment 8

[Boris Zbarsky [:bzbarsky]]

Now maybe the answer is to add another behavior flag to nsEscape...

Comment 9

[Andrea Marchesini [:baku]]

Attachment: searchparams.patch

https://tbpl.mozilla.org/?tree=Try&rev=21b5483cad17

Comment 10

[Anne (:annevk)]

bz, perhaps we should modify nsEscape? As that algorithm is also used by other parts of the URL Standard.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8615f5866edf

Comment 12

[Boris Zbarsky [:bzbarsky]]

Adding a new flag might make sense, yes.

Changing the meaning of the existing flags is doable, but means auditing all existing consumers.

Comment 13

[Jeff Walden [:Waldo]]

It would be nice to have a specific algorithm to refer to for this, instead of augmenting nsEscape, whose semantics may be obvious to the seasoned XPCOM-er but are not obvious to the average spec-reader, and which would not be obviously correct (or obviously the right ones for any given place) when used to implement any given web algorithm.  That this algorithm appears *only* in URLSearchParams stuff doesn't seem to suggest implementation-sharing to me.  The commoning-up should happen at spec level first, in implementations where called second.

Comment 14

[Anne (:annevk)]

Well, unless usage of nsEscape is not covered by any specification, why do you think that has not yet happened? I agree that it requires auditing and work though so we might want to postpone that.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> why do you think that has not yet happened?

Why do I think what has not happened?

Have you looked at the nsEscape API?

What it does may or may not be covered by specs, which it may or may not be following, in the usual way of URLs.

Comment 16

[Anne (:annevk)]

I was referring to "commoning-up should happen at spec level".

Comment 17

[Boris Zbarsky [:bzbarsky]]

Ah, ok.

nsEscape is used in various situations, including by extensions.  I would expect at least some of its uses are not covered by specs, while others might be.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8615f5866edf

Comment 19

[Lawrence Mandel [:lmandel] (use needinfo)]

34 (aurora) is marked as affected. As this bug has been on m-c for several days, can you please submit an aurora approval request?

Comment 20

[Andrea Marchesini [:baku]]

Comment on attachment 8486162 [details] [diff] [review]
searchparams.patch

Approval Request Comment
[Feature/regressing bug #]: bug 887836
[User impact if declined]: we can have wrong unicode sequences in URLSearchParams
[Describe test coverage new/current, TBPL]: m-c, tbpl green
[Risks and why]: none
[String/UUID change made/needed]: none

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8486162 [details] [diff] [review]
searchparams.patch

Aurora+

Comment 0 suggested that this bug was introduced in bug 1037715, which landed in 34. If the bug was introduced with bug 887836, this impacts 29+ and we should consider uplift to beta as well.

Comment 22

[Andrea Marchesini [:baku]]

> Comment 0 suggested that this bug was introduced in bug 1037715, which
> landed in 34. If the bug was introduced with bug 887836, this impacts 29+
> and we should consider uplift to beta as well.

In bug 1037715 we implemented URLSearchParams in the location object but this issue is about how the percentage sequences are encoded/decoded in the URLSearchParams object in general. URL API, HTMLAnchorElements and HTMLAreaElements are effected too from bug 887836.

Comment 23

[Andrea Marchesini [:baku]]

Comment on attachment 8486162 [details] [diff] [review]
searchparams.patch

Approval Request Comment
[Feature/regressing bug #]: bug 887836
[User impact if declined]: we can have wrong unicode sequences in URLSearchParams
[Describe test coverage new/current, TBPL]: m-c, tbpl green
[Risks and why]: none
[String/UUID change made/needed]: none

Comment 24

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8486162 [details] [diff] [review]
searchparams.patch

Thanks for clarifying Andrea. Beta+

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/75e38ef65862
https://hg.mozilla.org/releases/mozilla-beta/rev/f44f06112715