1801870 - [DNR] Implement redirect.transform part of redirect actions

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, enhancement, P2)

Description

[Rob Wu [:robwu]]

This is about implementing the redirect.transform part of the DNR API.

For reference, Chromium introduced this capability in https://crbug.com/983685, which links a design doc at https://docs.google.com/document/d/1foL79VwByaVeUwYPBfZeqyC2SYcAawhjjH05eqHqW_E/edit

Comment 1

[Rob Wu [:robwu]]

Noting for posterity: The queryTransform part of the API has unspecified behavior in edge cases, and Chrome's implementation has questionable results:

The queryTransform API has three descriptions:

• redirect.transform.queryTransform is described as:
  "Add, remove or replace query key-value pairs."
• redirect.transform.queryTransform.removeParams is described as
  "The list of query keys to be removed."
• redirect.transform.queryTransform.addOrReplaceParams is described as
  "The list of query key-value pairs to be added or replaced."
  ( = an object with keys key, value and optional replaceOnly (default false). )

"add" here can be interpreted as "set" or "append". With the existence of removeParams, I would have expected "append" to be the default (since one can always simulate "set" by adding removeParams + append). Chrome's implementation however uses set by default. And if a key is repeated multiple times, the repeated keys are appended. Since the common behavior is probably to replace a query parameter, I'll follow Chrome's approach here. If there is ever a desire to support appending without modifying the query string, then a new property can be introduced.

The encoding of removeParams and addOrReplaceParams (key & value) are not specified. The relevant cases have no documentation nor unit tests. According to Chrome's implementation, the input from the extension is escaped (percent-escaped with a plus, i.e. application/x-www-form-urlencoded, all except alphanum and !'()*-._~). This looks seemingly reasonable because the implementation that applies the transformation also searches on a similarly normalized version of a URL. However, because the search param may be constructed in a way that's not consistent with that form-urlencoding, the resulting queryParams API won't be able to erase or modify certain query params.

Test cases checked with Chrome 107:

• http://host/?a+b=c
  • This is form-urlencoded. Can remove with: removeParams: ["a b]
• http://host/?a%20b=c
  • This is still a valid URL/query string, but not in the form-urlencoded format. The form-urlencoded version of this query is a+b=c.
  • Because Chrome url-encodes any input, a%20b becomes a%2520b and it is not possible to delete a%20b. It is not possible to remove the value by with a+b or a b either, as Chrome does not convert ?a%20b=c to ?a+b=c.

The encoding described above is available through the URLSearchParams API. While tempting to re-use that API to parse the query in the DNR implementation, there is an of URLSearchParams that makes it unsuitable: upon serializing the query string, the whole thing is form-urlencoded again, which may change the parts within the query string. Therefore, in order to offer the functionality with minimal side effects, we'd have to iterate over the whole query string and only remove/replace query parameters when a match is found.

If I had to design the API from scratch, I would not have re-encoded any of the input (key/value), and only have validated that there is no unexpected & or # or (in case of keys:) = in it. After all, developers can always url-encode the values themselves if they'd like.

Comment 2

[Rob Wu [:robwu]]

Attachment: Bug 1801870 - Support IPv6 and FQDN hosts in httpd test server + tests

Comment 3

[Rob Wu [:robwu]]

Attachment: Bug 1801870 - Implement DNR action.redirect.transform

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7b90401cb566
Support IPv6 and FQDN hosts in httpd test server + tests r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/b40af7a90b33
Implement DNR action.redirect.transform r=rpl

Comment 5

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/7b90401cb566
https://hg.mozilla.org/mozilla-central/rev/b40af7a90b33