From 00a2ef1276b43ffeb799517b839af6b4bec83ecb Mon Sep 17 00:00:00 2001 From: Adrian Niculescu <15037449+adrian-niculescu@users.noreply.github.com> Date: Fri, 12 Jun 2026 16:23:22 +0300 Subject: [PATCH 1/5] fix: URLSearchParams construction and iteration spec compliance The constructor previously handled only the string init form; any object init was stringified to "[object Object]", collapsing every query parameter into a single bogus key. It now follows the WHATWG/WebIDL init dispatch (https://url.spec.whatwg.org/#interface-urlsearchparams): record, sequence (any iterable of pairs: Array, Map, Set, another URLSearchParams, generators - with IteratorClose on abrupt completion), other primitives (USVString coercion; Symbol throws), and null/undefined (empty). entries()/keys()/values() returned plain v8::Arrays and the type exposed no @@iterator, so for..of, spread, and the copy-constructor form did not work; the array path also returned the first value for every occurrence of a repeated key. They now return genuine live ES iterators (reflecting mutations mid-iteration, per spec), and @@iterator aliases entries(). Also aligned to spec: get() returns null (not undefined) for a missing name; delete(name, value) and has(name, value) honor the optional value argument (coerced to USVString; an explicit undefined is treated as omitted, matching WPT). --- .../app/tests/testURLSearchParamsImpl.js | 305 +++++++++- .../src/main/cpp/URLSearchParamsImpl.cpp | 541 +++++++++++++++--- 2 files changed, 757 insertions(+), 89 deletions(-) diff --git a/test-app/app/src/main/assets/app/tests/testURLSearchParamsImpl.js b/test-app/app/src/main/assets/app/tests/testURLSearchParamsImpl.js index 3d0ca293f..7b7f6760e 100644 --- a/test-app/app/src/main/assets/app/tests/testURLSearchParamsImpl.js +++ b/test-app/app/src/main/assets/app/tests/testURLSearchParamsImpl.js @@ -1,15 +1,16 @@ describe("Test URLSearchParams ", function () { const fooBar = "foo=1&bar=2"; it("Test URLSearchParams keys", function(){ + // keys() returns a spec iterator, not an array — consume it via spread. const params = new URLSearchParams(fooBar); - const keys = params.keys(); + const keys = [...params.keys()]; expect(keys[0]).toBe("foo"); expect(keys[1]).toBe("bar"); }); it("Test URLSearchParams values", function(){ const params = new URLSearchParams(fooBar); - const values = params.values(); + const values = [...params.values()]; expect(values[0]).toBe("1"); expect(values[1]).toBe("2"); }); @@ -17,7 +18,7 @@ describe("Test URLSearchParams ", function () { it("Test URLSearchParams entries", function(){ const params = new URLSearchParams(fooBar); - const entries = params.entries(); + const entries = [...params.entries()]; expect(entries[0][0]).toBe("foo"); expect(entries[0][1]).toBe("1"); @@ -26,6 +27,74 @@ describe("Test URLSearchParams ", function () { }); + it("Test URLSearchParams keys/values/entries return spec iterators", function(){ + const params = new URLSearchParams(fooBar); + // A spec iterator has a next() and is itself iterable. + expect(typeof params.entries().next).toBe("function"); + expect(typeof params.keys().next).toBe("function"); + expect(typeof params.values().next).toBe("function"); + const it = params.entries(); + const first = it.next(); + expect(first.done).toBe(false); + expect(first.value[0]).toBe("foo"); + expect(first.value[1]).toBe("1"); + }); + + it("Test URLSearchParams entries preserves duplicate keys", function(){ + // Regression: the old get_keys()+get() path returned the first value for + // every occurrence of a repeated key. + const params = new URLSearchParams("a=1&a=2&b=3"); + const entries = [...params.entries()]; + expect(entries.length).toBe(3); + expect(entries[0][1]).toBe("1"); + expect(entries[1][1]).toBe("2"); + expect(entries[2][1]).toBe("3"); + expect([...params.values()].join(",")).toBe("1,2,3"); + }); + + it("Test URLSearchParams default iterator aliases entries", function(){ + // The default @@iterator IS the entries method (browser identity). This binding + // installs members per-instance (not on the prototype), so assert on an instance + // AND assert it is actually a function — not the vacuous undefined === undefined. + const params = new URLSearchParams(fooBar); + expect(typeof params[Symbol.iterator]).toBe("function"); + expect(params[Symbol.iterator]).toBe(params.entries); + }); + + it("Test URLSearchParams iterator carries the spec brand", function(){ + const params = new URLSearchParams(fooBar); + expect(Object.prototype.toString.call(params.entries())).toBe("[object URLSearchParams Iterator]"); + }); + + it("Test URLSearchParams iterator is live", function(){ + // Spec iterators reflect mutations made after they are created. + const params = new URLSearchParams("a=1&b=2"); + const it = params.entries(); + expect(it.next().value[0]).toBe("a"); // consume "a" + params.append("c", "3"); // mutate mid-iteration + const rest = []; + let r; + while (!(r = it.next()).done) { + rest.push(r.value[0]); + } + expect(rest.join(",")).toBe("b,c"); // sees the appended "c" + }); + + it("Test URLSearchParams closes the source iterator on a bad pair", function(){ + // On an abrupt completion (a too-long pair) the source iterator must be + // closed, so a generator's finally runs and resource-backed iterables free. + let closed = false; + function* gen() { + try { + yield ["a", "1", "2"]; // 3-element pair → TypeError + } finally { + closed = true; + } + } + expect(function(){ new URLSearchParams(gen()); }).toThrow(); + expect(closed).toBe(true); + }); + it("Test URLSearchParams size", function(){ const params = new URLSearchParams(fooBar); @@ -43,7 +112,27 @@ describe("Test URLSearchParams ", function () { const params = new URLSearchParams(fooBar); params.append("first", "Osei"); params.delete("first"); - expect(params.get("first")).toBe(undefined); + // Spec: get() returns null for a missing name (url.bs:4016). + expect(params.get("first")).toBe(null); + }); + + it("Test URLSearchParams get returns null for a missing name", function(){ + // Spec get(name): "...otherwise null" (url.bs:4016). + const params = new URLSearchParams("a=1"); + expect(params.get("missing")).toBe(null); + }); + + it("Test URLSearchParams delete with value removes only matching pairs", function(){ + // Spec delete(name, value): when value is given, remove only tuples + // matching BOTH name and value (url.bs:4000). The value is a USVString, + // so a non-string (the number 1) coerces to "1". + const params = new URLSearchParams("a=1&a=2&a=1&b=1"); + params.delete("a", 1); + expect(params.getAll("a").join(",")).toBe("2"); + expect(params.getAll("b").join(",")).toBe("1"); + // Single-arg delete still removes every pair with that name. + params.delete("a"); + expect(params.has("a")).toBe(false); }); @@ -52,6 +141,47 @@ describe("Test URLSearchParams ", function () { expect(params.has("foo")).toBe(true); }); + it("Test URLSearchParams has with value matches name and value", function(){ + // Spec has(name, value): true only for a tuple matching BOTH (url.bs:4028). + // The value is a USVString, so non-strings (number, boolean) coerce. + const params = new URLSearchParams("a=1&a=2&flag=true"); + expect(params.has("a", "1")).toBe(true); + expect(params.has("a", 2)).toBe(true); // number coerces to "2" + expect(params.has("a", "3")).toBe(false); + expect(params.has("flag", true)).toBe(true); // boolean coerces to "true" + expect(params.has("missing", "1")).toBe(false); + // Single-arg has still matches by name only. + expect(params.has("a")).toBe(true); + }); + + it("Test URLSearchParams has/delete throw when the value cannot be coerced", function(){ + // The value argument is a USVString; a Symbol (or a throwing toString) + // cannot convert, so the call must throw rather than silently matching "" + // (WebIDL USVString conversion, url.bs:4000 / 4028). + const params = new URLSearchParams("a=1"); + expect(function(){ params.has("a", Symbol("x")); }).toThrow(); + expect(function(){ params.delete("a", Symbol("x")); }).toThrow(); + }); + + it("Test URLSearchParams has/delete treat an explicit undefined value as omitted", function(){ + // Per WPT (urlsearchparams-has / -delete "respects undefined as second + // arg"), an explicit `undefined` second argument is treated as omitted + // (name-only), NOT as the string "undefined". + const params = new URLSearchParams("a=b&a=d&c&e&"); + expect(params.has("a", "b")).toBe(true); + expect(params.has("a", "c")).toBe(false); + expect(params.has("a", undefined)).toBe(true); // undefined -> name-only + + const del = new URLSearchParams(); + del.append("a", "b"); + del.append("a", "c"); + del.append("b", "c"); + del.append("b", "d"); + del.delete("b", "c"); + del.delete("a", undefined); // undefined -> delete by name + expect(del.toString()).toBe("b=d"); + }); + it("Test URLSearchParams changes propagates to URL parent", function(){ const toBe = 'https://github.com/triniwiz?first=Osei'; const url = new URL('https://github.com/triniwiz'); @@ -114,4 +244,171 @@ describe("Test URLSearchParams ", function () { expect(results[2].value).toBe("3"); }); + it("Test URLSearchParams from record object", function(){ + const params = new URLSearchParams({ foo: "1", bar: "2" }); + expect(params.get("foo")).toBe("1"); + expect(params.get("bar")).toBe("2"); + expect(params.size).toBe(2); + // A plain object must expand to its entries, not collapse into a + // single "[object Object]" key. + expect(params.has("[object Object]")).toBe(false); + }); + + it("Test URLSearchParams from record serializes every pair in toString", function(){ + const params = new URLSearchParams({ one: "1", two: "2" }); + expect(params.toString()).toBe("one=1&two=2"); + }); + + it("Test URLSearchParams from record coerces values to strings", function(){ + const params = new URLSearchParams({ a: 1, b: true }); + expect(params.get("a")).toBe("1"); + expect(params.get("b")).toBe("true"); + }); + + it("Test URLSearchParams from record encodes special characters", function(){ + const params = new URLSearchParams({ q: "a b&c" }); + expect(params.get("q")).toBe("a b&c"); + expect(params.toString()).toBe("q=a+b%26c"); + }); + + it("Test URLSearchParams from array of pairs", function(){ + const params = new URLSearchParams([["foo", "1"], ["bar", "2"], ["foo", "3"]]); + expect(params.get("foo")).toBe("1"); + expect(params.getAll("foo").length).toBe(2); + expect(params.get("bar")).toBe("2"); + expect(params.size).toBe(3); + }); + + it("Test URLSearchParams empty record and no-arg produce empty query", function(){ + expect(new URLSearchParams().toString()).toBe(""); + expect(new URLSearchParams({}).toString()).toBe(""); + }); + + it("Test URLSearchParams from record throws when a value cannot be coerced to a string", function(){ + // Per spec the record/sequence init coerces every value to a USVString; + // a value that cannot convert (a Symbol here) must throw rather than + // silently dropping or emptying the entry. + expect(function(){ new URLSearchParams({ bad: Symbol("x") }); }).toThrow(); + }); + + // --- Iterable (sequence) init: any iterable of pairs, not only arrays. --- + + it("Test URLSearchParams from a Map", function(){ + const params = new URLSearchParams(new Map([["a", "1"], ["b", "2"]])); + expect(params.toString()).toBe("a=1&b=2"); + }); + + it("Test URLSearchParams from a Set of pairs", function(){ + const params = new URLSearchParams(new Set([["x", "1"], ["y", "2"]])); + expect(params.toString()).toBe("x=1&y=2"); + }); + + it("Test URLSearchParams copy-constructs from another URLSearchParams", function(){ + // A URLSearchParams is iterable, so per spec it resolves to the sequence + // (copy) form — including duplicate keys, which proves the @@iterator walks + // entries rather than collapsing them. + const source = new URLSearchParams("a=1&a=2&b=3"); + const copy = new URLSearchParams(source); + expect(copy.toString()).toBe("a=1&a=2&b=3"); + expect(copy.getAll("a").length).toBe(2); + }); + + it("Test URLSearchParams from a generator of pairs", function(){ + function* pairs() { + yield ["a", "1"]; + yield ["b", "2"]; + } + const params = new URLSearchParams(pairs()); + expect(params.toString()).toBe("a=1&b=2"); + }); + + it("Test URLSearchParams from sequence with non-array inner pairs", function(){ + // Each pair need only be a 2-element iterable, not specifically an array. + // A Set is iterable but not an Array, so it exercises the inner iterator path. + const params = new URLSearchParams([new Set(["k", "v"])]); + expect(params.get("k")).toBe("v"); + }); + + it("Test URLSearchParams sequence init throws on a too-long pair", function(){ + expect(function(){ new URLSearchParams([["a", "1", "2"]]); }).toThrow(); + }); + + it("Test URLSearchParams sequence init throws on a too-short pair", function(){ + expect(function(){ new URLSearchParams([["a"]]); }).toThrow(); + }); + + it("Test URLSearchParams sequence init throws on a non-iterable element", function(){ + expect(function(){ new URLSearchParams([null]); }).toThrow(); + expect(function(){ new URLSearchParams([1]); }).toThrow(); + }); + + it("Test URLSearchParams sequence init throws on a primitive string element", function(){ + // WebIDL converts each element to sequence, whose first step throws + // when the element is not an Object. A 2-code-point string must NOT be accepted + // as the pair ("a","b"). + expect(function(){ new URLSearchParams(["ab"]); }).toThrow(); + }); + + it("Test URLSearchParams sequence init accepts a String-object element", function(){ + // A String *object* IS an Object and is iterable, so it is a valid 2-char pair. + const params = new URLSearchParams([new String("ab")]); + expect(params.get("a")).toBe("b"); + }); + + it("Test URLSearchParams throws when @@iterator is present but not callable", function(){ + // Per WebIDL GetMethod, a non-callable @@iterator is a TypeError, not a + // silent fall-back to the record form. + expect(function(){ new URLSearchParams({ [Symbol.iterator]: 5 }); }).toThrow(); + }); + + // --- The type itself is iterable. --- + + it("Test URLSearchParams is spread-iterable via Symbol.iterator", function(){ + const params = new URLSearchParams("a=1&b=2"); + const entries = [...params]; + expect(entries.length).toBe(2); + expect(entries[0][0]).toBe("a"); + expect(entries[0][1]).toBe("1"); + expect(entries[1][0]).toBe("b"); + expect(entries[1][1]).toBe("2"); + }); + + it("Test URLSearchParams works in a for..of loop", function(){ + const params = new URLSearchParams("a=1&a=2"); + const seen = []; + for (const [key, value] of params) { + seen.push(key + "=" + value); + } + expect(seen.length).toBe(2); + expect(seen[0]).toBe("a=1"); + expect(seen[1]).toBe("a=2"); + }); + + // --- Primitive init: coerced to USVString, then parsed. --- + + it("Test URLSearchParams from a number", function(){ + expect(new URLSearchParams(123).toString()).toBe("123="); + }); + + it("Test URLSearchParams from a boolean", function(){ + expect(new URLSearchParams(true).toString()).toBe("true="); + }); + + it("Test URLSearchParams from a bigint", function(){ + expect(new URLSearchParams(10n).toString()).toBe("10="); + }); + + it("Test URLSearchParams strips a single leading question mark", function(){ + expect(new URLSearchParams("?a=1").get("a")).toBe("1"); + }); + + it("Test URLSearchParams throws when init is a Symbol", function(){ + expect(function(){ new URLSearchParams(Symbol("x")); }).toThrow(); + }); + + it("Test URLSearchParams from null or undefined is empty", function(){ + expect(new URLSearchParams(null).toString()).toBe(""); + expect(new URLSearchParams(undefined).toString()).toBe(""); + }); + }); diff --git a/test-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp b/test-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp index 710079bab..d7467e0f1 100644 --- a/test-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp +++ b/test-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp @@ -30,9 +30,11 @@ namespace tns { ArgConverter::ConvertToV8String(isolate, "delete"), v8::FunctionTemplate::New(isolate, Delete)); - tmpl->Set( - ArgConverter::ConvertToV8String(isolate, "entries"), - v8::FunctionTemplate::New(isolate, Entries)); + // Capture the entries template so it can double as the default @@iterator + // (registered below), matching the browser identity + // URLSearchParams.prototype[Symbol.iterator] === entries. + auto entriesTmpl = v8::FunctionTemplate::New(isolate, Entries); + tmpl->Set(ArgConverter::ConvertToV8String(isolate, "entries"), entriesTmpl); tmpl->Set( ArgConverter::ConvertToV8String(isolate, "forEach"), @@ -75,10 +77,382 @@ namespace tns { tmpl->Set(ArgConverter::ConvertToV8String(isolate, "values"), v8::FunctionTemplate::New(isolate, &Values)); + // Declare the type as iterable by exposing + // @@iterator. It aliases entries() (which returns a genuine ES iterator), + // so `for..of`, spread, and `new URLSearchParams(otherParams)` (the spec + // sequence/copy form) all work. Non-enumerable, to match the browser + // prototype shape. + tmpl->Set(v8::Symbol::GetIterator(isolate), entriesTmpl, + v8::PropertyAttribute::DontEnum); return ctorTmpl; } + // Spec-compliant URLSearchParams init (record / sequence / primitive). The + // dispatch mirrors the WebIDL union conversion to + // `(sequence> or record or USVString)` + // that browsers/Node apply (https://url.spec.whatwg.org/#interface-urlsearchparams): + // object with @@iterator -> sequence (Array, Map, Set, URLSearchParams, generator) + // object without @@iterator-> record (own enumerable string keys, in order) + // other primitive -> USVString (number/boolean/bigint -> string; Symbol throws) + // null / undefined -> empty + namespace { + void ThrowTypeError(v8::Isolate *isolate, const char *message) { + isolate->ThrowException( + v8::Exception::TypeError(ArgConverter::ConvertToV8String(isolate, message))); + } + + // --- Live iterators for entries()/keys()/values()/@@iterator. --- + // Spec URLSearchParams iterators are *live*: they reflect append()/delete() + // performed after the iterator is created (and mid-iteration). Each is backed + // by a small hidden state object — { src: the URLSearchParams, idx: cursor, + // knd: which projection } — captured as next()'s Data. No snapshot, and no + // bespoke iterator class/finalizer: the source is kept alive by the state + // reference and collected normally with the iterator. next() re-reads the + // params on every call, so duplicate keys keep their own values too. + enum IteratorKind { ITER_KEYS = 0, ITER_VALUES = 1, ITER_ENTRIES = 2 }; + + const char *const kStateSource = "src"; + const char *const kStateIndex = "idx"; + const char *const kStateKind = "knd"; + + void IteratorSelf(const v8::FunctionCallbackInfo &args) { + // An iterator is itself iterable (iterator[@@iterator]() === iterator), + // so `for..of params.entries()` works. + args.GetReturnValue().Set(args.This()); + } + + void IteratorNext(const v8::FunctionCallbackInfo &args) { + auto isolate = args.GetIsolate(); + auto context = isolate->GetCurrentContext(); + auto state = args.Data().As(); + + auto indexKey = ArgConverter::ConvertToV8String(isolate, kStateIndex); + v8::Local sourceValue; + v8::Local indexValue; + v8::Local kindValue; + if (!state->Get(context, ArgConverter::ConvertToV8String(isolate, kStateSource)) + .ToLocal(&sourceValue) || + !state->Get(context, indexKey).ToLocal(&indexValue) || + !state->Get(context, ArgConverter::ConvertToV8String(isolate, kStateKind)) + .ToLocal(&kindValue)) { + return; + } + auto index = indexValue->Uint32Value(context).FromMaybe(0u); + auto kind = kindValue->Int32Value(context).FromMaybe(ITER_ENTRIES); + + auto result = v8::Object::New(isolate); + auto valueKey = ArgConverter::ConvertToV8String(isolate, "value"); + auto doneKey = ArgConverter::ConvertToV8String(isolate, "done"); + + URLSearchParamsImpl *ptr = sourceValue->IsObject() + ? URLSearchParamsImpl::GetPointer(sourceValue.As()) : nullptr; + if (ptr == nullptr || index >= ptr->GetURLSearchParams()->size()) { + result->Set(context, valueKey, v8::Undefined(isolate)).Check(); + result->Set(context, doneKey, v8::True(isolate)).Check(); + args.GetReturnValue().Set(result); + return; + } + + auto pair = (*ptr->GetURLSearchParams())[index]; + v8::Local value; + if (kind == ITER_KEYS) { + value = ArgConverter::ConvertToV8String(isolate, pair.first); + } else if (kind == ITER_VALUES) { + value = ArgConverter::ConvertToV8String(isolate, pair.second); + } else { + v8::Local kv[] = { + ArgConverter::ConvertToV8String(isolate, pair.first), + ArgConverter::ConvertToV8String(isolate, pair.second), + }; + value = v8::Array::New(isolate, kv, 2); + } + state->Set(context, indexKey, v8::Integer::NewFromUnsigned(isolate, index + 1)).Check(); + result->Set(context, valueKey, value).Check(); + result->Set(context, doneKey, v8::False(isolate)).Check(); + args.GetReturnValue().Set(result); + } + + // Sets `args`' return value to a fresh live iterator of `kind` over + // `args.This()` (the URLSearchParams). The state object is never exposed to + // JS — only the iterator (with next() and @@iterator) is returned. + void ReturnLiveIterator(const v8::FunctionCallbackInfo &args, int kind) { + auto isolate = args.GetIsolate(); + auto context = isolate->GetCurrentContext(); + + auto state = v8::Object::New(isolate); + state->Set(context, ArgConverter::ConvertToV8String(isolate, kStateSource), + args.This()).Check(); + state->Set(context, ArgConverter::ConvertToV8String(isolate, kStateIndex), + v8::Integer::NewFromUnsigned(isolate, 0)).Check(); + state->Set(context, ArgConverter::ConvertToV8String(isolate, kStateKind), + v8::Integer::New(isolate, kind)).Check(); + + v8::Local nextFn; + v8::Local selfFn; + if (!v8::Function::New(context, IteratorNext, state).ToLocal(&nextFn) || + !v8::Function::New(context, IteratorSelf).ToLocal(&selfFn)) { + return; + } + + auto iterator = v8::Object::New(isolate); + iterator->Set(context, ArgConverter::ConvertToV8String(isolate, "next"), nextFn).Check(); + iterator->Set(context, v8::Symbol::GetIterator(isolate), selfFn).Check(); + iterator->DefineOwnProperty(context, v8::Symbol::GetToStringTag(isolate), + ArgConverter::ConvertToV8String(isolate, + "URLSearchParams Iterator"), + v8::PropertyAttribute::DontEnum).Check(); + args.GetReturnValue().Set(iterator); + } + + // Coerce a JS value to a UTF-8 string via the USVString conversion the + // URLSearchParams init algorithm applies to every name/value. Returns + // false if the conversion threw — a Symbol value, or a user toString that + // throws — leaving the JS exception pending so the caller can stop and + // let it propagate (the spec throws here rather than dropping the entry). + // No further V8 API may be called once that happens. + bool ValueToString(v8::Local context, v8::Local value, + std::string &out) { + v8::Local str; + if (!value->ToString(context).ToLocal(&str)) { + return false; + } + out = ArgConverter::ConvertToString(str); + return true; + } + + // Opens a JS sync iterator: invokes `iterMethod` (the resolved @@iterator) + // with `receiver` as the `this` value, requires the result to be an object, + // and reads its callable `next` method. On any deviation the matching JS + // TypeError is thrown (or left pending, if a user callback threw) and the + // function returns false. + bool OpenIterator(v8::Local context, v8::Local receiver, + v8::Local iterMethod, v8::Local &outIterator, + v8::Local &outNext) { + auto isolate = context->GetIsolate(); + v8::Local iteratorValue; + if (!iterMethod->Call(context, receiver, 0, nullptr).ToLocal(&iteratorValue)) { + return false; + } + if (!iteratorValue->IsObject()) { + ThrowTypeError(isolate, "Result of the Symbol.iterator method is not an object"); + return false; + } + outIterator = iteratorValue.As(); + v8::Local nextValue; + if (!outIterator->Get(context, ArgConverter::ConvertToV8String(isolate, "next")) + .ToLocal(&nextValue)) { + return false; + } + if (!nextValue->IsFunction()) { + ThrowTypeError(isolate, "Iterator has no 'next' method"); + return false; + } + outNext = nextValue.As(); + return true; + } + + // Advances `iterator` once. On a normal step writes the yielded value to + // `outValue` and leaves `outDone` false; at completion sets `outDone` true. + // Returns false (exception pending) if next() threw or yielded a non-object + // result. `next` is captured once by the caller, per the iterator protocol. + bool StepIterator(v8::Local context, v8::Local iterator, + v8::Local next, v8::Local &outValue, + bool &outDone) { + auto isolate = context->GetIsolate(); + v8::Local resultValue; + if (!next->Call(context, iterator, 0, nullptr).ToLocal(&resultValue)) { + return false; + } + if (!resultValue->IsObject()) { + ThrowTypeError(isolate, "Iterator result is not an object"); + return false; + } + auto result = resultValue.As(); + v8::Local doneValue; + if (!result->Get(context, ArgConverter::ConvertToV8String(isolate, "done")) + .ToLocal(&doneValue)) { + return false; + } + outDone = doneValue->BooleanValue(isolate); + if (outDone) { + return true; + } + return result->Get(context, ArgConverter::ConvertToV8String(isolate, "value")) + .ToLocal(&outValue); + } + + // ES IteratorClose for an abrupt (error) completion: call the iterator's + // `return` method, if it has one, and discard both its result and any error + // it raises — the triggering exception wins (spec discards `return`'s + // completion on a throw). Generators with `finally` and resource-backed + // iterables rely on this to release state when conversion fails partway. + // Must be called with no pending JS exception: the caller captures and + // re-throws the triggering exception around this call. + void CloseIterator(v8::Local context, v8::Local iterator) { + auto isolate = context->GetIsolate(); + v8::TryCatch tryCatch(isolate); + v8::Local returnMethod; + if (iterator->Get(context, ArgConverter::ConvertToV8String(isolate, "return")) + .ToLocal(&returnMethod) && returnMethod->IsFunction()) { + v8::Local ignored; + (void) returnMethod.As()->Call(context, iterator, 0, nullptr) + .ToLocal(&ignored); + } + // tryCatch swallows anything return() throws when it goes out of scope. + } + + // Materializes one inner element of a sequence init into its strings. Per + // spec each element is itself converted to `sequence`, whose first + // step ("convert to sequence") throws a TypeError when Type(V) is not + // Object. So a non-Object (primitive) element — including a primitive string — + // throws; it is NOT boxed (boxing would wrongly accept ["ab"] as the pair + // ("a","b")). Iterable objects (Array, Set, a String *object*) are materialized + // by driving their @@iterator. If an element fails to coerce, the inner + // iterator is closed before the exception propagates. + bool MaterializeInnerSequence(v8::Local context, v8::Local value, + std::vector &out) { + auto isolate = context->GetIsolate(); + // Per WebIDL "convert to sequence": if Type(V) is not Object, throw a + // TypeError. Primitives (including strings) are NOT boxed here — boxing + // would wrongly accept e.g. ["ab"] as the pair ("a","b") instead of + // throwing. A String/Set/Array *object* element is fine (it is an Object). + if (!value->IsObject()) { + ThrowTypeError(isolate, "URLSearchParams init sequence element is not iterable"); + return false; + } + v8::Local object = value.As(); + v8::Local iterMethod; + if (!object->Get(context, v8::Symbol::GetIterator(isolate)).ToLocal(&iterMethod)) { + return false; + } + if (!iterMethod->IsFunction()) { + ThrowTypeError(isolate, "URLSearchParams init sequence element is not iterable"); + return false; + } + v8::Local iterator; + v8::Local next; + if (!OpenIterator(context, object, iterMethod.As(), iterator, next)) { + return false; + } + while (true) { + v8::Local element; + bool done = false; + if (!StepIterator(context, iterator, next, element, done)) { + return false; // next() threw: the iterator is already finished + } + if (done) { + break; + } + // Capture a conversion failure, then close the iterator outside the + // TryCatch's scope so the close runs with a clean isolate and the + // original exception is re-thrown (not swallowed by this handler). + v8::Local caught; + { + v8::TryCatch tryCatch(isolate); + std::string str; + if (ValueToString(context, element, str)) { + out.push_back(std::move(str)); + } else { + caught = tryCatch.Exception(); + tryCatch.Reset(); + } + } + if (!caught.IsEmpty()) { + CloseIterator(context, iterator); + isolate->ThrowException(caught); + return false; + } + } + return true; + } + + // Sequence init form: iterate the value via its @@iterator (already resolved + // by the caller) and append each [name, value] pair. Covers Array, Map, Set, + // another URLSearchParams and generators uniformly. A pair whose length is + // not exactly 2 throws a TypeError; a name/value that cannot be coerced to a + // string aborts with the JS exception left pending. On either abrupt + // completion the source iterator is closed first (ES IteratorClose). + bool BuildFromSequence(v8::Local context, v8::Local object, + v8::Local iterMethod, ada::url_search_params ¶ms) { + auto isolate = context->GetIsolate(); + v8::Local iterator; + v8::Local next; + if (!OpenIterator(context, object, iterMethod, iterator, next)) { + return false; + } + while (true) { + v8::Local entry; + bool done = false; + if (!StepIterator(context, iterator, next, entry, done)) { + return false; // next() threw: the iterator is already finished + } + if (done) { + break; + } + // Convert and append under a TryCatch; on failure capture the + // exception, then close the source iterator outside the handler's + // scope and re-throw (so the close runs clean and the error survives). + v8::Local caught; + { + v8::TryCatch tryCatch(isolate); + std::vector pair; + bool built = MaterializeInnerSequence(context, entry, pair); + if (built && pair.size() != 2) { + ThrowTypeError(isolate, + "Failed to construct 'URLSearchParams': Sequence initializer " + "must only contain pair elements"); + } + if (tryCatch.HasCaught()) { + caught = tryCatch.Exception(); + tryCatch.Reset(); + } else { + params.append(pair[0], pair[1]); + } + } + if (!caught.IsEmpty()) { + CloseIterator(context, iterator); + isolate->ThrowException(caught); + return false; + } + } + return true; + } + + // Record init form: a plain object of name -> value, iterated in own + // enumerable string-key order. A value that cannot be coerced to a string + // aborts with the JS exception left pending. + bool BuildFromRecord(v8::Local context, v8::Local object, + ada::url_search_params ¶ms) { + auto filter = static_cast( + v8::PropertyFilter::ONLY_ENUMERABLE | v8::PropertyFilter::SKIP_SYMBOLS); + v8::Local keys; + if (!object->GetOwnPropertyNames(context, filter, + v8::KeyConversionMode::kConvertToString).ToLocal(&keys)) { + return false; + } + auto length = keys->Length(); + for (uint32_t i = 0; i < length; i++) { + v8::Local key; + if (!keys->Get(context, i).ToLocal(&key)) { + return false; + } + v8::Local value; + if (!object->Get(context, key).ToLocal(&value)) { + return false; + } + std::string keyStr; + std::string valueStr; + if (!ValueToString(context, key, keyStr) || + !ValueToString(context, value, valueStr)) { + return false; + } + params.append(keyStr, valueStr); + } + return true; + } + } + void URLSearchParamsImpl::Ctor(const v8::FunctionCallbackInfo &args) { auto value = args[0]; auto isolate = args.GetIsolate(); @@ -88,11 +462,43 @@ namespace tns { ada::url_search_params params; if (value->IsString()) { + // ada strips a single leading '?' (constructor step), then runs the + // urlencoded string parser. params = ada::url_search_params(ArgConverter::ConvertToString(value.As())); - } else if (value->IsObject()) { - params = ada::url_search_params( - ArgConverter::ConvertToString(value->ToString(context).ToLocalChecked())); } + // Dispatch the record/sequence/primitive init forms via the helpers above. + // A failed coercion leaves a pending exception; return so V8 throws it. + else if (value->IsObject()) { + auto object = value.As(); + v8::Local iterMethod; + if (!object->Get(context, v8::Symbol::GetIterator(isolate)).ToLocal(&iterMethod)) { + return; // the @@iterator getter threw + } + if (iterMethod->IsNullOrUndefined()) { + if (!BuildFromRecord(context, object, params)) { + return; + } + } else if (iterMethod->IsFunction()) { + if (!BuildFromSequence(context, object, iterMethod.As(), params)) { + return; + } + } else { + // Per WebIDL GetMethod, a present-but-non-callable @@iterator is a + // TypeError rather than a silent fall-back to the record form. + ThrowTypeError(isolate, "URLSearchParams init Symbol.iterator is not a function"); + return; + } + } else if (!value->IsNullOrUndefined()) { + // Other primitive (number / boolean / bigint / symbol): coerce to a + // USVString and run the urlencoded string parser. A Symbol cannot be + // converted and throws here, matching the spec. + std::string init; + if (!ValueToString(context, value, init)) { + return; + } + params = ada::url_search_params(init); + } + // null / undefined -> leave params empty auto searchParams = new URLSearchParamsImpl(params); @@ -120,36 +526,29 @@ namespace tns { return; } auto key = ArgConverter::ConvertToString(args[0].As()); - ptr->GetURLSearchParams()->remove(key.c_str()); + // Spec delete(name, value): when the value argument is given, remove only + // tuples matching BOTH name and value (url.bs:4000). The value is a + // USVString, so coerce via ValueToString (number/boolean -> string; a + // Symbol or throwing toString leaves the pending exception and aborts). + // An explicit `undefined` value is treated as omitted (name-only), matching + // WPT "Two-argument delete() respects undefined as second arg"; only a + // present, non-undefined value takes the value-filtered path. + if (args.Length() > 1 && !args[1]->IsUndefined()) { + std::string value; + if (!ValueToString(args.GetIsolate()->GetCurrentContext(), args[1], value)) { + return; // coercion threw (Symbol / throwing toString) + } + ptr->GetURLSearchParams()->remove(key, value); + } else { + ptr->GetURLSearchParams()->remove(key.c_str()); + } } + // entries()/keys()/values() return live ES iterators (per spec they are + // iterators, not arrays). Reading the params by index per next() also keeps + // each occurrence of a repeated key paired with its own value. void URLSearchParamsImpl::Entries(const v8::FunctionCallbackInfo &args) { - URLSearchParamsImpl *ptr = GetPointer(args.This()); - auto isolate = args.GetIsolate(); - auto context = isolate->GetCurrentContext(); - if (ptr == nullptr) { - args.GetReturnValue().Set(v8::Array::New(isolate)); - return; - } - - auto keys = ptr->GetURLSearchParams()->get_keys(); - auto len = ptr->GetURLSearchParams()->size(); - auto ret = v8::Array::New(isolate, len); - int i = 0; - while (keys.has_next()) { - auto key = keys.next(); - if (key) { - auto keyValue = key.value(); - auto value = ptr->GetURLSearchParams()->get(keyValue).value(); - v8::Local values[] = { - ArgConverter::ConvertToV8String(isolate, keyValue.data()), - ArgConverter::ConvertToV8String(isolate, value.data()), - }; - ret->Set(context, i++, v8::Array::New(isolate, values, 2)); - } - - } - args.GetReturnValue().Set(ret); + ReturnLiveIterator(args, ITER_ENTRIES); } void URLSearchParamsImpl::ForEach(const v8::FunctionCallbackInfo &args) { @@ -197,7 +596,9 @@ namespace tns { auto ret = ArgConverter::ConvertToV8String(isolate, std::string(value.value())); args.GetReturnValue().Set(ret); } else { - args.GetReturnValue().SetUndefined(); + // Spec: get() returns null for a missing name (url.bs:4016), not + // undefined. + args.GetReturnValue().SetNull(); } } @@ -226,35 +627,30 @@ namespace tns { return; } auto key = args[0].As(); - auto value = ptr->GetURLSearchParams()->has(ArgConverter::ConvertToString(key)); + // Spec has(name, value): when the value argument is given, return true + // only for a tuple matching BOTH name and value (url.bs:4028). The value + // is a USVString, so coerce via ValueToString (number/boolean -> string; + // a Symbol or throwing toString leaves the pending exception and aborts). + bool value; + // An explicit `undefined` value is treated as omitted (name-only), matching + // WPT "Two-argument has() respects undefined as second arg"; only a present, + // non-undefined value takes the value-filtered path. + if (args.Length() > 1 && !args[1]->IsUndefined()) { + std::string filter; + if (!ValueToString(args.GetIsolate()->GetCurrentContext(), args[1], filter)) { + return; // coercion threw (Symbol / throwing toString) + } + value = ptr->GetURLSearchParams()->has(ArgConverter::ConvertToString(key), filter); + } else { + value = ptr->GetURLSearchParams()->has(ArgConverter::ConvertToString(key)); + } args.GetReturnValue().Set(value); } + // Live ES iterator (see Entries above). void URLSearchParamsImpl::Keys(const v8::FunctionCallbackInfo &args) { - URLSearchParamsImpl *ptr = GetPointer(args.This()); - auto isolate = args.GetIsolate(); - auto context = isolate->GetCurrentContext(); - if (ptr == nullptr) { - args.GetReturnValue().Set(v8::Array::New(isolate)); - return; - } - - auto keys = ptr->GetURLSearchParams()->get_keys(); - - auto len = ptr->GetURLSearchParams()->size(); - auto ret = v8::Array::New(isolate, len); - int i = 0; - while (keys.has_next()) { - auto key = keys.next(); - if (key) { - auto keyValue = key.value(); - ret->Set(context, i++, ArgConverter::ConvertToV8String(isolate, keyValue.data())); - } - - } - args.GetReturnValue().Set(ret); - + ReturnLiveIterator(args, ITER_KEYS); } void URLSearchParamsImpl::Set(const v8::FunctionCallbackInfo &args) { @@ -308,34 +704,9 @@ namespace tns { args.GetReturnValue().Set(ret); } + // Live ES iterator (see Entries above). void URLSearchParamsImpl::Values(const v8::FunctionCallbackInfo &args) { - URLSearchParamsImpl *ptr = GetPointer(args.This()); - auto isolate = args.GetIsolate(); - auto context = isolate->GetCurrentContext(); - if (ptr == nullptr) { - args.GetReturnValue().Set(v8::Array::New(isolate)); - return; - } - - auto keys = ptr->GetURLSearchParams()->get_keys(); - - auto len = ptr->GetURLSearchParams()->size(); - auto ret = v8::Array::New(isolate, len); - int i = 0; - while (keys.has_next()) { - auto key = keys.next(); - if (key) { - auto value = ptr->GetURLSearchParams()->get(key.value()); - if (value.has_value()) { - ret->Set(context, i++, - ArgConverter::ConvertToV8String(isolate, std::string(value.value()))); - } - - } - - } - args.GetReturnValue().Set(ret); - + ReturnLiveIterator(args, ITER_VALUES); } From 1a0ae120bb1dfddbad264f651533ae65c4f5f643 Mon Sep 17 00:00:00 2001 From: Adrian Niculescu <15037449+adrian-niculescu@users.noreply.github.com> Date: Fri, 12 Jun 2026 16:57:50 +0300 Subject: [PATCH 2/5] fix: coerce names to USVString and brand-check iterator receivers Review follow-up: - get()/getAll()/has()/delete() now coerce the name argument through the same USVString conversion as the optional value argument instead of casting it to a string blindly, so a number/boolean/object name works and a Symbol (or a throwing toString) raises a TypeError. - entries()/keys()/values() and the live iterator's next() brand-check their receivers and throw "Illegal invocation" for foreign objects, per the WebIDL iterable<> semantics. GetPointer also refuses to read an internal field from an object that has none (e.g. entries.call({})), which was an out-of-bounds read. --- .../app/tests/testURLSearchParamsImpl.js | 61 +++++++++++++++++ .../src/main/cpp/URLSearchParamsImpl.cpp | 66 ++++++++++++++++--- 2 files changed, 119 insertions(+), 8 deletions(-) diff --git a/test-app/app/src/main/assets/app/tests/testURLSearchParamsImpl.js b/test-app/app/src/main/assets/app/tests/testURLSearchParamsImpl.js index 7b7f6760e..7e22e4aa4 100644 --- a/test-app/app/src/main/assets/app/tests/testURLSearchParamsImpl.js +++ b/test-app/app/src/main/assets/app/tests/testURLSearchParamsImpl.js @@ -411,4 +411,65 @@ describe("Test URLSearchParams ", function () { expect(new URLSearchParams(undefined).toString()).toBe(""); }); + // --- The name argument is a USVString: coerced, not assumed. --- + + it("Test URLSearchParams coerces a non-string name in get/getAll/has/delete", function(){ + const params = new URLSearchParams("1=a&true=b&null=c"); + expect(params.get(1)).toBe("a"); + expect(params.getAll(1).length).toBe(1); + expect(params.getAll(1)[0]).toBe("a"); + expect(params.has(true)).toBe(true); + expect(params.get(null)).toBe("c"); + params.delete(true); + expect(params.has("true")).toBe(false); + expect(params.has("1")).toBe(true); + }); + + it("Test URLSearchParams coerces an object name via toString", function(){ + const params = new URLSearchParams("a=1"); + const name = { toString: function(){ return "a"; } }; + expect(params.get(name)).toBe("1"); + expect(params.has(name)).toBe(true); + params.delete(name); + expect(params.has("a")).toBe(false); + }); + + it("Test URLSearchParams throws when the name is a Symbol", function(){ + const params = new URLSearchParams("a=1"); + expect(function(){ params.get(Symbol("x")); }).toThrow(); + expect(function(){ params.getAll(Symbol("x")); }).toThrow(); + expect(function(){ params.has(Symbol("x")); }).toThrow(); + expect(function(){ params.delete(Symbol("x")); }).toThrow(); + expect(params.get("a")).toBe("1"); + }); + + // --- Brand checks: iterators require a genuine receiver. --- + + it("Test URLSearchParams entries/keys/values throw on a foreign receiver", function(){ + const params = new URLSearchParams("a=1"); + expect(function(){ params.entries.call({}); }).toThrow(); + expect(function(){ params.keys.call({}); }).toThrow(); + expect(function(){ params.values.call({}); }).toThrow(); + }); + + it("Test URLSearchParams iterator next() throws on a foreign receiver", function(){ + const params = new URLSearchParams("a=1"); + const iterator = params.entries(); + const next = iterator.next; + expect(function(){ next.call({}); }).toThrow(); + // The iterator keeps working when invoked correctly afterwards. + const first = iterator.next(); + expect(first.done).toBe(false); + expect(first.value[0]).toBe("a"); + }); + + it("Test URLSearchParams entries retargets to another instance receiver", function(){ + const a = new URLSearchParams("a=1"); + const b = new URLSearchParams("b=2"); + const entries = [...a.entries.call(b)]; + expect(entries.length).toBe(1); + expect(entries[0][0]).toBe("b"); + expect(entries[0][1]).toBe("2"); + }); + }); diff --git a/test-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp b/test-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp index d7467e0f1..86a0d238d 100644 --- a/test-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp +++ b/test-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp @@ -10,6 +10,12 @@ namespace tns { URLSearchParamsImpl::URLSearchParamsImpl(ada::url_search_params params) : params_(params) {} URLSearchParamsImpl *URLSearchParamsImpl::GetPointer(v8::Local object) { + // A foreign receiver (e.g. entries.call({})) has no internal field, and + // reading one that is not there is undefined behavior, so check first + // and report not-a-URLSearchParams instead. + if (object->InternalFieldCount() < 1) { + return nullptr; + } auto ptr = object->GetAlignedPointerFromInternalField(0); if (ptr == nullptr) { return nullptr; @@ -115,6 +121,7 @@ namespace tns { const char *const kStateSource = "src"; const char *const kStateIndex = "idx"; const char *const kStateKind = "knd"; + const char *const kStateIterator = "itr"; void IteratorSelf(const v8::FunctionCallbackInfo &args) { // An iterator is itself iterable (iterator[@@iterator]() === iterator), @@ -127,6 +134,17 @@ namespace tns { auto context = isolate->GetCurrentContext(); auto state = args.Data().As(); + // WebIDL brand check: next() must be invoked on the iterator it was + // created on, so a detached next() throws instead of advancing the + // captured state. + v8::Local iteratorValue; + if (!state->Get(context, ArgConverter::ConvertToV8String(isolate, kStateIterator)) + .ToLocal(&iteratorValue) || + !iteratorValue->StrictEquals(args.This())) { + ThrowTypeError(isolate, "Illegal invocation"); + return; + } + auto indexKey = ArgConverter::ConvertToV8String(isolate, kStateIndex); v8::Local sourceValue; v8::Local indexValue; @@ -180,6 +198,13 @@ namespace tns { auto isolate = args.GetIsolate(); auto context = isolate->GetCurrentContext(); + // WebIDL brand check: entries()/keys()/values() require a genuine + // URLSearchParams receiver, so e.g. entries.call({}) throws. + if (URLSearchParamsImpl::GetPointer(args.This()) == nullptr) { + ThrowTypeError(isolate, "Illegal invocation"); + return; + } + auto state = v8::Object::New(isolate); state->Set(context, ArgConverter::ConvertToV8String(isolate, kStateSource), args.This()).Check(); @@ -196,6 +221,9 @@ namespace tns { } auto iterator = v8::Object::New(isolate); + // Recorded in the hidden state so next() can brand-check its receiver. + state->Set(context, ArgConverter::ConvertToV8String(isolate, kStateIterator), + iterator).Check(); iterator->Set(context, ArgConverter::ConvertToV8String(isolate, "next"), nextFn).Check(); iterator->Set(context, v8::Symbol::GetIterator(isolate), selfFn).Check(); iterator->DefineOwnProperty(context, v8::Symbol::GetToStringTag(isolate), @@ -525,7 +553,13 @@ namespace tns { if (ptr == nullptr) { return; } - auto key = ArgConverter::ConvertToString(args[0].As()); + // The name is a USVString (url.bs:3861): coerce it like the optional + // value below instead of assuming a string (a Symbol or throwing + // toString leaves the pending exception and aborts). + std::string key; + if (!ValueToString(args.GetIsolate()->GetCurrentContext(), args[0], key)) { + return; + } // Spec delete(name, value): when the value argument is given, remove only // tuples matching BOTH name and value (url.bs:4000). The value is a // USVString, so coerce via ValueToString (number/boolean -> string; a @@ -590,8 +624,13 @@ namespace tns { args.GetReturnValue().SetUndefined(); return; } - auto key = args[0].As(); - auto value = ptr->GetURLSearchParams()->get(ArgConverter::ConvertToString(key)); + // The name is a USVString (url.bs:3862); a Symbol or throwing toString + // leaves the pending exception and aborts. + std::string key; + if (!ValueToString(isolate->GetCurrentContext(), args[0], key)) { + return; + } + auto value = ptr->GetURLSearchParams()->get(key); if (value.has_value()) { auto ret = ArgConverter::ConvertToV8String(isolate, std::string(value.value())); args.GetReturnValue().Set(ret); @@ -610,8 +649,13 @@ namespace tns { args.GetReturnValue().Set(v8::Array::New(isolate)); return; } - auto key = args[0].As(); - auto values = ptr->GetURLSearchParams()->get_all(ArgConverter::ConvertToString(key)); + // The name is a USVString (url.bs:3863); a Symbol or throwing toString + // leaves the pending exception and aborts. + std::string key; + if (!ValueToString(context, args[0], key)) { + return; + } + auto values = ptr->GetURLSearchParams()->get_all(key); auto ret = v8::Array::New(isolate, values.size()); size_t i = 0; for (auto item: values) { @@ -626,7 +670,13 @@ namespace tns { args.GetReturnValue().Set(false); return; } - auto key = args[0].As(); + // The name is a USVString (url.bs:3864): coerce it like the optional + // value below instead of assuming a string (a Symbol or throwing + // toString leaves the pending exception and aborts). + std::string key; + if (!ValueToString(args.GetIsolate()->GetCurrentContext(), args[0], key)) { + return; + } // Spec has(name, value): when the value argument is given, return true // only for a tuple matching BOTH name and value (url.bs:4028). The value // is a USVString, so coerce via ValueToString (number/boolean -> string; @@ -640,9 +690,9 @@ namespace tns { if (!ValueToString(args.GetIsolate()->GetCurrentContext(), args[1], filter)) { return; // coercion threw (Symbol / throwing toString) } - value = ptr->GetURLSearchParams()->has(ArgConverter::ConvertToString(key), filter); + value = ptr->GetURLSearchParams()->has(key, filter); } else { - value = ptr->GetURLSearchParams()->has(ArgConverter::ConvertToString(key)); + value = ptr->GetURLSearchParams()->has(key); } args.GetReturnValue().Set(value); From 3945a0df477e0a483218c92f6ddfb24abaefdd7b Mon Sep 17 00:00:00 2001 From: Adrian Niculescu <15037449+adrian-niculescu@users.noreply.github.com> Date: Fri, 12 Jun 2026 17:12:51 +0300 Subject: [PATCH 3/5] fix: coerce append/set arguments to USVStrings Completes the review follow-up: append() and set() now run both arguments through the same USVString coercion as the rest of the binding, instead of casting them to strings blindly. A number/boolean/object argument coerces via toString; a Symbol (or a throwing toString) raises a TypeError before anything is appended or replaced. --- .../app/tests/testURLSearchParamsImpl.js | 20 +++++++++++++ .../src/main/cpp/URLSearchParamsImpl.cpp | 30 ++++++++++++------- 2 files changed, 40 insertions(+), 10 deletions(-) diff --git a/test-app/app/src/main/assets/app/tests/testURLSearchParamsImpl.js b/test-app/app/src/main/assets/app/tests/testURLSearchParamsImpl.js index 7e22e4aa4..68fc16331 100644 --- a/test-app/app/src/main/assets/app/tests/testURLSearchParamsImpl.js +++ b/test-app/app/src/main/assets/app/tests/testURLSearchParamsImpl.js @@ -443,6 +443,26 @@ describe("Test URLSearchParams ", function () { expect(params.get("a")).toBe("1"); }); + it("Test URLSearchParams coerces non-string arguments in append and set", function(){ + const params = new URLSearchParams(); + params.append(1, 2); + expect(params.get("1")).toBe("2"); + params.set(true, { toString: function(){ return "x"; } }); + expect(params.get("true")).toBe("x"); + params.append("a", null); + expect(params.get("a")).toBe("null"); + }); + + it("Test URLSearchParams append and set throw when an argument is a Symbol", function(){ + const params = new URLSearchParams("a=1"); + expect(function(){ params.append(Symbol("x"), "v"); }).toThrow(); + expect(function(){ params.append("k", Symbol("x")); }).toThrow(); + expect(function(){ params.set(Symbol("x"), "v"); }).toThrow(); + expect(function(){ params.set("k", Symbol("x")); }).toThrow(); + // Nothing was appended or replaced by the failed calls. + expect(params.toString()).toBe("a=1"); + }); + // --- Brand checks: iterators require a genuine receiver. --- it("Test URLSearchParams entries/keys/values throw on a foreign receiver", function(){ diff --git a/test-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp b/test-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp index 86a0d238d..2f1ce5691 100644 --- a/test-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp +++ b/test-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp @@ -543,9 +543,16 @@ namespace tns { if (ptr == nullptr) { return; } - auto key = ArgConverter::ConvertToString(args[0].As()); - auto value = ArgConverter::ConvertToString(args[1].As()); - ptr->GetURLSearchParams()->append(key.c_str(), value.c_str()); + // Both arguments are USVStrings (url.bs:3860); a Symbol or throwing + // toString leaves the pending exception and aborts. + auto context = args.GetIsolate()->GetCurrentContext(); + std::string key; + std::string value; + if (!ValueToString(context, args[0], key) || + !ValueToString(context, args[1], value)) { + return; + } + ptr->GetURLSearchParams()->append(key, value); } void URLSearchParamsImpl::Delete(const v8::FunctionCallbackInfo &args) { @@ -708,13 +715,16 @@ namespace tns { if (ptr == nullptr) { return; } - auto key = args[0].As(); - auto value = args[1].As(); - - ptr->GetURLSearchParams()->set( - ArgConverter::ConvertToString(key), - ArgConverter::ConvertToString(value) - ); + // Both arguments are USVStrings (url.bs:3865); a Symbol or throwing + // toString leaves the pending exception and aborts. + auto context = args.GetIsolate()->GetCurrentContext(); + std::string key; + std::string value; + if (!ValueToString(context, args[0], key) || + !ValueToString(context, args[1], value)) { + return; + } + ptr->GetURLSearchParams()->set(key, value); } void URLSearchParamsImpl::GetSize(v8::Local property, From dfcce8cc07c86b09f05b20d693f6b7894696a7df Mon Sep 17 00:00:00 2001 From: Adrian Niculescu <15037449+adrian-niculescu@users.noreply.github.com> Date: Fri, 12 Jun 2026 18:05:05 +0300 Subject: [PATCH 4/5] fix: align null init, record Symbol keys, and receiver checks with WebIDL - new URLSearchParams(null) parses the string "null" (so "null="). The IDL default "" applies only to undefined/missing; null goes through the union conversion, which has no null special case (the union is not nullable and a record is not a dictionary), so it lands in the USVString branch. Matches the whatwg-url reference implementation; Node treats null as empty and is the outlier, and WPT does not cover the case. - An own enumerable Symbol key in a record init throws a TypeError instead of being silently skipped: WebIDL record conversion takes the keys from [[OwnPropertyKeys]] filtered by enumerability only, and converting a Symbol key to a USVString throws. - The remaining members (get/getAll/has/append/set/delete/forEach/ sort/toString/size) brand-check their receiver and throw an "Illegal invocation" TypeError for a foreign object, consistent with entries()/keys()/values(). --- .../app/tests/testURLSearchParamsImpl.js | 31 +++++++++++- .../src/main/cpp/URLSearchParamsImpl.cpp | 48 ++++++++++++------- 2 files changed, 60 insertions(+), 19 deletions(-) diff --git a/test-app/app/src/main/assets/app/tests/testURLSearchParamsImpl.js b/test-app/app/src/main/assets/app/tests/testURLSearchParamsImpl.js index 68fc16331..b326af9a3 100644 --- a/test-app/app/src/main/assets/app/tests/testURLSearchParamsImpl.js +++ b/test-app/app/src/main/assets/app/tests/testURLSearchParamsImpl.js @@ -406,9 +406,23 @@ describe("Test URLSearchParams ", function () { expect(function(){ new URLSearchParams(Symbol("x")); }).toThrow(); }); - it("Test URLSearchParams from null or undefined is empty", function(){ - expect(new URLSearchParams(null).toString()).toBe(""); + it("Test URLSearchParams from undefined or no argument is empty", function(){ expect(new URLSearchParams(undefined).toString()).toBe(""); + expect(new URLSearchParams().toString()).toBe(""); + }); + + it("Test URLSearchParams from null parses as the string null", function(){ + // The IDL union has no null special case (the type is not nullable and + // a record is not a dictionary), so null coerces like any primitive. + const params = new URLSearchParams(null); + expect(params.toString()).toBe("null="); + expect(params.get("null")).toBe(""); + }); + + it("Test URLSearchParams throws when a record key is a Symbol", function(){ + // Per WebIDL record conversion every own enumerable key is converted to + // a USVString, and converting a Symbol throws. + expect(function(){ new URLSearchParams({ a: "1", [Symbol("x")]: "v" }); }).toThrow(); }); // --- The name argument is a USVString: coerced, not assumed. --- @@ -472,6 +486,19 @@ describe("Test URLSearchParams ", function () { expect(function(){ params.values.call({}); }).toThrow(); }); + it("Test URLSearchParams methods throw on a foreign receiver", function(){ + const params = new URLSearchParams("a=1"); + expect(function(){ params.get.call({}, "a"); }).toThrow(); + expect(function(){ params.getAll.call({}, "a"); }).toThrow(); + expect(function(){ params.has.call({}, "a"); }).toThrow(); + expect(function(){ params.append.call({}, "a", "b"); }).toThrow(); + expect(function(){ params.set.call({}, "a", "b"); }).toThrow(); + expect(function(){ params.delete.call({}, "a"); }).toThrow(); + expect(function(){ params.forEach.call({}, function(){}); }).toThrow(); + expect(function(){ params.sort.call({}); }).toThrow(); + expect(function(){ params.toString.call({}); }).toThrow(); + }); + it("Test URLSearchParams iterator next() throws on a foreign receiver", function(){ const params = new URLSearchParams("a=1"); const iterator = params.entries(); diff --git a/test-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp b/test-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp index 2f1ce5691..259f541c7 100644 --- a/test-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp +++ b/test-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp @@ -99,9 +99,9 @@ namespace tns { // `(sequence> or record or USVString)` // that browsers/Node apply (https://url.spec.whatwg.org/#interface-urlsearchparams): // object with @@iterator -> sequence (Array, Map, Set, URLSearchParams, generator) - // object without @@iterator-> record (own enumerable string keys, in order) - // other primitive -> USVString (number/boolean/bigint -> string; Symbol throws) - // null / undefined -> empty + // object without @@iterator-> record (own enumerable keys, in order; a Symbol key throws) + // other primitive / null -> USVString (number/boolean/bigint/null -> string; Symbol throws) + // undefined / missing -> empty (the IDL default "") namespace { void ThrowTypeError(v8::Isolate *isolate, const char *message) { isolate->ThrowException( @@ -448,12 +448,15 @@ namespace tns { } // Record init form: a plain object of name -> value, iterated in own - // enumerable string-key order. A value that cannot be coerced to a string - // aborts with the JS exception left pending. + // enumerable key order ([[OwnPropertyKeys]]: strings, then symbols). Per + // WebIDL record conversion every enumerable key is converted to a + // USVString, so an own enumerable Symbol key throws a TypeError + // (ValueToString below raises it); symbols are NOT silently skipped. A + // key or value that cannot be coerced to a string aborts with the JS + // exception left pending. bool BuildFromRecord(v8::Local context, v8::Local object, ada::url_search_params ¶ms) { - auto filter = static_cast( - v8::PropertyFilter::ONLY_ENUMERABLE | v8::PropertyFilter::SKIP_SYMBOLS); + auto filter = v8::PropertyFilter::ONLY_ENUMERABLE; v8::Local keys; if (!object->GetOwnPropertyNames(context, filter, v8::KeyConversionMode::kConvertToString).ToLocal(&keys)) { @@ -516,17 +519,21 @@ namespace tns { ThrowTypeError(isolate, "URLSearchParams init Symbol.iterator is not a function"); return; } - } else if (!value->IsNullOrUndefined()) { - // Other primitive (number / boolean / bigint / symbol): coerce to a - // USVString and run the urlencoded string parser. A Symbol cannot be - // converted and throws here, matching the spec. + } else if (!value->IsUndefined()) { + // Other primitive (number / boolean / bigint / symbol / null): coerce + // to a USVString and run the urlencoded string parser. The WebIDL + // union conversion has no null special case (the union is not + // nullable and a record is not a dictionary), so + // new URLSearchParams(null) parses the string "null", as the + // reference implementation does. A Symbol cannot be converted and + // throws here, matching the spec. std::string init; if (!ValueToString(context, value, init)) { return; } params = ada::url_search_params(init); } - // null / undefined -> leave params empty + // undefined / missing -> the IDL default "" -> leave params empty auto searchParams = new URLSearchParamsImpl(params); @@ -541,6 +548,9 @@ namespace tns { void URLSearchParamsImpl::Append(const v8::FunctionCallbackInfo &args) { URLSearchParamsImpl *ptr = GetPointer(args.This()); if (ptr == nullptr) { + // WebIDL brand check: every member requires a genuine URLSearchParams + // receiver (same as entries()/keys()/values()). + ThrowTypeError(args.GetIsolate(), "Illegal invocation"); return; } // Both arguments are USVStrings (url.bs:3860); a Symbol or throwing @@ -558,6 +568,7 @@ namespace tns { void URLSearchParamsImpl::Delete(const v8::FunctionCallbackInfo &args) { URLSearchParamsImpl *ptr = GetPointer(args.This()); if (ptr == nullptr) { + ThrowTypeError(args.GetIsolate(), "Illegal invocation"); return; } // The name is a USVString (url.bs:3861): coerce it like the optional @@ -597,6 +608,7 @@ namespace tns { auto isolate = args.GetIsolate(); auto context = isolate->GetCurrentContext(); if (ptr == nullptr) { + ThrowTypeError(isolate, "Illegal invocation"); return; } auto callback = args[0].As(); @@ -628,7 +640,7 @@ namespace tns { URLSearchParamsImpl *ptr = GetPointer(args.This()); auto isolate = args.GetIsolate(); if (ptr == nullptr) { - args.GetReturnValue().SetUndefined(); + ThrowTypeError(isolate, "Illegal invocation"); return; } // The name is a USVString (url.bs:3862); a Symbol or throwing toString @@ -653,7 +665,7 @@ namespace tns { auto isolate = args.GetIsolate(); auto context = isolate->GetCurrentContext(); if (ptr == nullptr) { - args.GetReturnValue().Set(v8::Array::New(isolate)); + ThrowTypeError(isolate, "Illegal invocation"); return; } // The name is a USVString (url.bs:3863); a Symbol or throwing toString @@ -674,7 +686,7 @@ namespace tns { void URLSearchParamsImpl::Has(const v8::FunctionCallbackInfo &args) { URLSearchParamsImpl *ptr = GetPointer(args.This()); if (ptr == nullptr) { - args.GetReturnValue().Set(false); + ThrowTypeError(args.GetIsolate(), "Illegal invocation"); return; } // The name is a USVString (url.bs:3864): coerce it like the optional @@ -713,6 +725,7 @@ namespace tns { void URLSearchParamsImpl::Set(const v8::FunctionCallbackInfo &args) { URLSearchParamsImpl *ptr = GetPointer(args.This()); if (ptr == nullptr) { + ThrowTypeError(args.GetIsolate(), "Illegal invocation"); return; } // Both arguments are USVStrings (url.bs:3865); a Symbol or throwing @@ -731,7 +744,7 @@ namespace tns { const v8::PropertyCallbackInfo &info) { URLSearchParamsImpl *ptr = GetPointer(info.This()); if (ptr == nullptr) { - info.GetReturnValue().Set(0); + ThrowTypeError(info.GetIsolate(), "Illegal invocation"); return; } @@ -743,6 +756,7 @@ namespace tns { void URLSearchParamsImpl::Sort(const v8::FunctionCallbackInfo &args) { URLSearchParamsImpl *ptr = GetPointer(args.This()); if (ptr == nullptr) { + ThrowTypeError(args.GetIsolate(), "Illegal invocation"); return; } ptr->GetURLSearchParams()->sort(); @@ -751,7 +765,7 @@ namespace tns { void URLSearchParamsImpl::ToString(const v8::FunctionCallbackInfo &args) { URLSearchParamsImpl *ptr = GetPointer(args.This()); if (ptr == nullptr) { - args.GetReturnValue().SetEmptyString(); + ThrowTypeError(args.GetIsolate(), "Illegal invocation"); return; } auto isolate = args.GetIsolate(); From 254f7b083fe07cad9f84eaa959dacc8c297643b7 Mon Sep 17 00:00:00 2001 From: Adrian Niculescu <15037449+adrian-niculescu@users.noreply.github.com> Date: Fri, 12 Jun 2026 19:06:07 +0300 Subject: [PATCH 5/5] docs: clarify when the sequence init runs IteratorClose Comment-only. The previous wording ("on either abrupt completion") could be read as closing the iterator on any failure, including a throw from next() itself. Spelled out the actual rule: conversion failures and wrong-length pairs close the source iterator, while a throw from the stepping (next(), or reading the result's done/value) does not, because the ES iterator protocol marks the iterator done on such a throw and IteratorClose is skipped for a done iterator. --- test-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp b/test-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp index 259f541c7..a0aa9bddb 100644 --- a/test-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp +++ b/test-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp @@ -399,8 +399,12 @@ namespace tns { // by the caller) and append each [name, value] pair. Covers Array, Map, Set, // another URLSearchParams and generators uniformly. A pair whose length is // not exactly 2 throws a TypeError; a name/value that cannot be coerced to a - // string aborts with the JS exception left pending. On either abrupt - // completion the source iterator is closed first (ES IteratorClose). + // string aborts with the JS exception left pending. On either of those two + // failures the source iterator is closed first (ES IteratorClose). A throw + // from the stepping itself (next(), or reading the result's done/value) + // intentionally does NOT close the iterator: the ES iterator protocol marks + // the iterator done on such a throw (IteratorStepValue), and IteratorClose + // is skipped for a done iterator, which is also how for..of behaves. bool BuildFromSequence(v8::Local context, v8::Local object, v8::Local iterMethod, ada::url_search_params ¶ms) { auto isolate = context->GetIsolate();