Closed Bug 1064481 Opened 5 years ago Closed 5 years ago

DOM searchParams setter doesn't encode correctly

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox32 --- wontfix
firefox33 + fixed
firefox34 + fixed
firefox35 --- fixed

People

(Reporter: benjamin, Assigned: baku)

References

Details

Attachments

(1 file, 1 obsolete file)

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).
Flags: needinfo?(amarchesini)
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.
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.
"%%%02X" is probably marginally better, assuming it works (which I'm fairly sure it does).
Attached patch searchparams.patch (obsolete) — Splinter Review
Attachment #8485944 - Flags: review?(ehsan.akhgari)
Flags: needinfo?(amarchesini)
Assignee: nobody → amarchesini
Attachment #8485944 - Flags: review?(ehsan.akhgari) → review+
(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.
Flags: needinfo?(bzbarsky)
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
Flags: needinfo?(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.  :(
Now maybe the answer is to add another behavior flag to nsEscape...
Keywords: checkin-needed
bz, perhaps we should modify nsEscape? As that algorithm is also used by other parts of the URL Standard.
Adding a new flag might make sense, yes.

Changing the meaning of the existing flags is doable, but means auditing all existing consumers.
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.
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.
> 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.
I was referring to "commoning-up should happen at spec level".
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.
https://hg.mozilla.org/mozilla-central/rev/8615f5866edf
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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?
Flags: needinfo?(amarchesini)
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
Attachment #8486162 - Flags: approval-mozilla-aurora?
Flags: needinfo?(amarchesini)
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.
Attachment #8486162 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(amarchesini)
> 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.
Flags: needinfo?(amarchesini)
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
Attachment #8486162 - Flags: approval-mozilla-beta?
Comment on attachment 8486162 [details] [diff] [review]
searchparams.patch

Thanks for clarifying Andrea. Beta+
Attachment #8486162 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.