fix(url): make URLSearchParams record constructor spec compliant (#295)

Fixes the issue where byte replacement characters could result in duplicate entries when constructing `URLSearchParams` from a record.

While current implementation follows the [spec](https://url.spec.whatwg.org/#urlsearchparams-initialize) to the word: 
> Otherwise, if init is a [record](https://webidl.spec.whatwg.org/#idl-record), then [for each](https://infra.spec.whatwg.org/#map-iterate) name → value of init, [append](https://infra.spec.whatwg.org/#list-append) (name, value) to query’s [list](https://url.spec.whatwg.org/#concept-urlsearchparams-list).

using `params_append()` directly happens before the `USVString` conversion so that keys that are supposed to collide after conversion can be added as separate entries.

By using `params_set()` we get the correct behavior without having to normalize the record beforehand.

Extending `maybe_consume_sequence_or_record` with separate sequence and record handlers allows applying different behavior.

Passes 2 remaining tests for `URLSearchParams` constructor.

Author: mattermoran

builtins/web/fetch/headers.cpp

@@ -650,7 +650,7 @@ bool Headers::init_entries(JSContext *cx, HandleObject self, HandleValue initv)
   // TODO: But note: forbidden headers have to be applied correctly.
   bool consumed = false;
   if (!core::maybe_consume_sequence_or_record<host_api::HostString, validate_header_name,
-                                              append_valid_header>(cx, initv, self, &consumed,
+                                              append_valid_header, append_valid_header>(cx, initv, self, &consumed,
                                                                    "Headers")) {
     return false;
   }

builtins/web/url.cpp

@@ -184,7 +184,7 @@ jsurl::JSUrlSearchParams *URLSearchParams::get_params(JSObject *self) {
 }
 
 namespace {
-jsurl::SpecString append_impl_validate(JSContext *cx, JS::HandleValue key, const char *_) {
+jsurl::SpecString validate(JSContext *cx, JS::HandleValue key, const char *_) {
   return core::encode_spec_string(cx, key);
 }
 bool append_impl(JSContext *cx, JS::HandleObject self, jsurl::SpecString key, JS::HandleValue val,
@@ -199,6 +199,18 @@ bool append_impl(JSContext *cx, JS::HandleObject self, jsurl::SpecString key, JS
   jsurl::params_append(params, key, value);
   return true;
 }
+bool set_impl(JSContext *cx, JS::HandleObject self, jsurl::SpecString key,
+              JS::HandleValue val, const char *_) {
+  auto *const params = URLSearchParams::get_params(self);
+
+  auto value = core::encode_spec_string(cx, val);
+  if (!value.data) {
+    return false;
+  }
+
+  jsurl::params_set(params, key, value);
+  return true;
+}
 } // namespace
 
 jsurl::SpecSlice URLSearchParams::serialize(JSContext *cx, JS::HandleObject self) {
@@ -207,7 +219,7 @@ jsurl::SpecSlice URLSearchParams::serialize(JSContext *cx, JS::HandleObject self
 
 bool URLSearchParams::append(JSContext *cx, unsigned argc, JS::Value *vp) {
   METHOD_HEADER(2)
-  auto value = append_impl_validate(cx, args[0], "append");
+  auto value = validate(cx, args[0], "append");
   if (!append_impl(cx, self, value, args[1], "append")) {
     return false;
   }
@@ -416,7 +428,7 @@ JSObject *URLSearchParams::create(JSContext *cx, JS::HandleObject self,
 
   bool consumed = false;
   const char *alt_text = ", or a value that can be stringified";
-  if (!core::maybe_consume_sequence_or_record<jsurl::SpecString, append_impl_validate, append_impl>(
+  if (!core::maybe_consume_sequence_or_record<jsurl::SpecString, validate, append_impl, set_impl>(
           cx, params_val, self, &consumed, "URLSearchParams", alt_text)) {
     return nullptr;
   }

runtime/sequence.hpp

@@ -13,7 +13,7 @@ namespace core {
  * Extract <key,value> pairs from the given value if it is either a
  * sequence<sequence<Value> or a record<Value, Value>.
  */
-template <typename T, auto validate, auto apply>
+template <typename T, auto validate, auto apply_sequence, auto apply_record>
 bool maybe_consume_sequence_or_record(JSContext *cx, JS::HandleValue initv, JS::HandleObject target,
                                       bool *consumed, const char *ctor_name,
                                       const char *alt_text = "") {
@@ -92,7 +92,7 @@ bool maybe_consume_sequence_or_record(JSContext *cx, JS::HandleValue initv, JS::
           return api::throw_error(cx, api::Errors::InvalidSequence, ctor_name, alt_text);
         }
 
-        if (!apply(cx, target, std::move(validated_key), value, ctor_name)) {
+        if (!apply_sequence(cx, target, std::move(validated_key), value, ctor_name)) {
           return false;
         }
       }
@@ -128,7 +128,7 @@ bool maybe_consume_sequence_or_record(JSContext *cx, JS::HandleValue initv, JS::
       if (!JS_GetPropertyById(cx, init, curId, &value)) {
         return false;
       }
-      if (!apply(cx, target, std::move(validated_key), value, ctor_name)) {
+      if (!apply_record(cx, target, std::move(validated_key), value, ctor_name)) {
         return false;
       }
     }

tests/wpt-harness/expectations/url/urlsearchparams-constructor.any.js.json

@@ -69,15 +69,15 @@
     "status": "PASS"
   },
   "Construct with 2 unpaired surrogates (no trailing)": {
-    "status": "FAIL"
+    "status": "PASS"
   },
   "Construct with 3 unpaired surrogates (no leading)": {
-    "status": "FAIL"
+    "status": "PASS"
   },
   "Construct with object with NULL, non-ASCII, and surrogate keys": {
     "status": "PASS"
   },
   "Custom [Symbol.iterator]": {
     "status": "PASS"
   }
-}
+}