fix: URLSearchParams construction and iteration spec compliance#388
Merged
NathanWalker merged 5 commits intoJun 12, 2026
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR refactors URLSearchParams to implement WHATWG spec compliance: adds live iterators for entries/keys/values with Symbol.iterator aliasing, reworks the constructor to accept strings/records/sequences/primitives via USVString-like coercion, updates delete/has to support value-filtered operations, and changes get to return null. ChangesURLSearchParams Spec Compliance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@NativeScript/runtime/URLSearchParamsImpl.cpp`:
- Around line 568-584: The Delete and Has implementations still call
tns::ToString(args[0].As<v8::String>()) and must instead coerce the first
argument with ValueToString to follow USVString semantics; replace the existing
key acquisition in URLSearchParamsImpl::Delete and URLSearchParamsImpl::Has by
calling ValueToString(isolate->GetCurrentContext(), args[0], key) into a
std::string key, return immediately if coercion fails (preserving exceptions),
and then call ptr->GetURLSearchParams()->remove(key, ...) or remove(key.c_str())
/ ptr->GetURLSearchParams()->has(key) using that coerced key; remove the use of
args[0].As<v8::String>() and tns::ToString for the name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b1464f66-8765-406b-bb66-26af6cb0d7fe
📒 Files selected for processing (2)
NativeScript/runtime/URLSearchParamsImpl.cppTestRunner/app/tests/URLSearchParams.js
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
NativeScript/runtime/URLSearchParamsImpl.cpp (2)
652-670:⚠️ Potential issue | 🟠 MajorGuard
URLSearchParams.forEachcallback beforeargs[0].As<v8::Function>().
v8::Local<T>::As<S>()is a cast that doesn’t do runtime type checking; without anargs[0]->IsFunction()(and length) guard, non-function/undefined callbacks can reach theCallpath instead of throwing the expected JSTypeError.Proposed fix
void URLSearchParamsImpl::ForEach( const v8::FunctionCallbackInfo<v8::Value>& args) { URLSearchParamsImpl* ptr = GetPointer(args.This()); auto isolate = args.GetIsolate(); auto context = isolate->GetCurrentContext(); if (ptr == nullptr) { ThrowTypeError(isolate, "Illegal invocation"); return; } + if (args.Length() < 1 || !args[0]->IsFunction()) { + ThrowTypeError(isolate, "URLSearchParams.forEach callback must be a function"); + return; + } auto callback = args[0].As<v8::Function>(); auto searchParams = args.This();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/URLSearchParamsImpl.cpp` around lines 652 - 670, The forEach implementation currently casts args[0] to v8::Function without runtime checking; before calling args[0].As<v8::Function>() (and before using callback->Call), add a guard that args.Length() > 0 and args[0]->IsFunction(), and if not throw a JS TypeError (via v8::Exception::TypeError / v8::TypeError::New and context->Throw). Move or defer creation of the v8::Local<v8::Function> callback until after that check, keep thisArg and entries logic the same, and ensure the code paths that call callback->Call only run when the argument is a validated function (referencing the variables callback, args, thisArg, and the existing Call invocation).
25-37:⚠️ Potential issue | 🟠 MajorStrengthen receiver brand check before unwrapping slot 0 internal pointers
URLSearchParamsImpl::GetPointeronly checksInternalFieldCount() >= 1, then casts slot 0 toURLSearchParamsImpl*(lines 25-37). Other wrapped natives also use internal field slot 0 (e.g.URLImpl/URLPatternImplwrite slot 0 and theirGetPointeralso only casts), so callingURLSearchParamsmethods with a foreign receiver can bypass the “Illegal invocation” path and reinterpret a different native pointer asURLSearchParamsImpl*.
- Add a real URLSearchParams brand/type gate in
GetPointer(e.g., dedicated type tag in the internal field, or an instance-of/identity check) before performing the cast.- In
URLSearchParamsImpl::ForEach, guardargs[0]beforeauto callback = args[0].As<v8::Function>();(missing/non-callable callback should throw TypeError instead of relying on an unsafe cast).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/URLSearchParamsImpl.cpp` around lines 25 - 37, The GetPointer implementation in URLSearchParamsImpl currently trusts slot 0 and can miscast foreign receivers; modify URLSearchParamsImpl::GetPointer to perform a real brand/type check before casting (e.g., store and verify a dedicated type tag or magic value in another internal field or use an instance identity check) instead of only checking InternalFieldCount and slot 0 with GetAlignedPointerFromInternalField; additionally, harden URLSearchParamsImpl::ForEach to validate args[0] is a callable before doing auto callback = args[0].As<v8::Function>() and throw a TypeError when it is missing or not a function. Ensure references to InternalFieldCount, GetAlignedPointerFromInternalField, URLSearchParamsImpl::GetPointer and URLSearchParamsImpl::ForEach are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@NativeScript/runtime/URLSearchParamsImpl.cpp`:
- Around line 652-670: The forEach implementation currently casts args[0] to
v8::Function without runtime checking; before calling args[0].As<v8::Function>()
(and before using callback->Call), add a guard that args.Length() > 0 and
args[0]->IsFunction(), and if not throw a JS TypeError (via
v8::Exception::TypeError / v8::TypeError::New and context->Throw). Move or defer
creation of the v8::Local<v8::Function> callback until after that check, keep
thisArg and entries logic the same, and ensure the code paths that call
callback->Call only run when the argument is a validated function (referencing
the variables callback, args, thisArg, and the existing Call invocation).
- Around line 25-37: The GetPointer implementation in URLSearchParamsImpl
currently trusts slot 0 and can miscast foreign receivers; modify
URLSearchParamsImpl::GetPointer to perform a real brand/type check before
casting (e.g., store and verify a dedicated type tag or magic value in another
internal field or use an instance identity check) instead of only checking
InternalFieldCount and slot 0 with GetAlignedPointerFromInternalField;
additionally, harden URLSearchParamsImpl::ForEach to validate args[0] is a
callable before doing auto callback = args[0].As<v8::Function>() and throw a
TypeError when it is missing or not a function. Ensure references to
InternalFieldCount, GetAlignedPointerFromInternalField,
URLSearchParamsImpl::GetPointer and URLSearchParamsImpl::ForEach are updated
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 09a44c34-d1e0-4493-a560-7db4692a020b
📒 Files selected for processing (2)
NativeScript/runtime/URLSearchParamsImpl.cppTestRunner/app/tests/URLSearchParams.js
🚧 Files skipped from review as they are similar to previous changes (1)
- TestRunner/app/tests/URLSearchParams.js
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).
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.
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.
…bIDL - 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().
adrian-niculescu
force-pushed
the
fix/urlsearchparams-construction-live-iterators
branch
from
c959e05 to
c45030e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
NativeScript/runtime/URLSearchParamsImpl.cpp (2)
652-670:⚠️ Potential issue | 🟠 MajorAdd a type-check for the
forEachcallback before casting tov8::Function.
URLSearchParamsImpl::ForEachcastsargs[0]directly viaargs[0].As<v8::Function>()with noIsFunction()(or args-length) guard. Passing a non-callable (or missing) callback should throw a JSTypeErrorat the API boundary instead of relying on V8’s cast/call path.Minimal fix
- auto callback = args[0].As<v8::Function>(); + if (args.Length() < 1 || !args[0]->IsFunction()) { + ThrowTypeError(isolate, "The callback provided as parameter 1 is not a function"); + return; + } + auto callback = args[0].As<v8::Function>();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/URLSearchParamsImpl.cpp` around lines 652 - 670, In URLSearchParamsImpl::ForEach, validate the first argument before casting: check args.Length() > 0 and args[0]->IsFunction() (or context->Global()->Get(isolate, ...) style check) and if not, throw a JS TypeError; replace the direct cast args[0].As<v8::Function>() with a guarded branch that creates a v8::Local<v8::Function> only when IsFunction() is true and otherwise calls isolate->ThrowException(v8::Exception::TypeError(...)) (referencing symbols: URLSearchParamsImpl::ForEach, args, callback, thisArg, and the v8::Function cast).
25-37:⚠️ Potential issue | 🟠 MajorFix receiver branding in
URLSearchParamsImpl::GetPointerto prevent type confusion (UB)The guard only checks
InternalFieldCount() >= 1and that internal field 0 is non-null, then unconditionallystatic_casts it toURLSearchParamsImpl*. Since other NativeScript bindings also useSetInternalFieldCount(1)and store their own native pointers in internal field 0 (e.g.,NativeScript/runtime/URLImpl.cpp,NativeScript/runtime/URLPatternImpl.cpp), a foreign native wrapper can bypass the intendedTypeError("Illegal invocation")path and produce undefined behavior from pointer re-interpretation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/URLSearchParamsImpl.cpp` around lines 25 - 37, GetPointer currently trusts any object with InternalFieldCount() >= 1 and a non-null internal field 0, which allows other native wrappers to be misinterpreted; fix by adding a receiver-brand check before static_cast: store a unique branding token (e.g., a static void* kURLSearchParamsBrand) in a second internal field when creating URLSearchParams instances and in URLSearchParamsImpl::GetPointer require InternalFieldCount() >= 2 and verify object->GetAlignedPointerFromInternalField(1) equals that branding token (or check the type of the internal field is the expected v8::External containing the brand) before casting the pointer from field 0 to URLSearchParamsImpl*. Ensure the code references URLSearchParamsImpl::GetPointer and the new URLSearchParamsImpl::kURLSearchParamsBrand (or similar) to locate changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@NativeScript/runtime/URLSearchParamsImpl.cpp`:
- Around line 394-395: StepIterator failures in MaterializeInnerSequence and
BuildFromSequence return false early and skip ES IteratorClose; update both
functions so that whenever StepIterator(context, iterator, next, element, done)
returns false you first call CloseIterator(context, iterator) (preserving any
original error/abrupt completion behavior) and only then return false,
propagating CloseIterator failures as appropriate; ensure you reference the same
iterator/next values used in the failing StepIterator call and follow the
existing error-return semantics used elsewhere in the file for IteratorClose
handling.
---
Outside diff comments:
In `@NativeScript/runtime/URLSearchParamsImpl.cpp`:
- Around line 652-670: In URLSearchParamsImpl::ForEach, validate the first
argument before casting: check args.Length() > 0 and args[0]->IsFunction() (or
context->Global()->Get(isolate, ...) style check) and if not, throw a JS
TypeError; replace the direct cast args[0].As<v8::Function>() with a guarded
branch that creates a v8::Local<v8::Function> only when IsFunction() is true and
otherwise calls isolate->ThrowException(v8::Exception::TypeError(...))
(referencing symbols: URLSearchParamsImpl::ForEach, args, callback, thisArg, and
the v8::Function cast).
- Around line 25-37: GetPointer currently trusts any object with
InternalFieldCount() >= 1 and a non-null internal field 0, which allows other
native wrappers to be misinterpreted; fix by adding a receiver-brand check
before static_cast: store a unique branding token (e.g., a static void*
kURLSearchParamsBrand) in a second internal field when creating URLSearchParams
instances and in URLSearchParamsImpl::GetPointer require InternalFieldCount() >=
2 and verify object->GetAlignedPointerFromInternalField(1) equals that branding
token (or check the type of the internal field is the expected v8::External
containing the brand) before casting the pointer from field 0 to
URLSearchParamsImpl*. Ensure the code references URLSearchParamsImpl::GetPointer
and the new URLSearchParamsImpl::kURLSearchParamsBrand (or similar) to locate
changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b96c87d0-944b-48d1-8c23-f6af1eccf024
📒 Files selected for processing (2)
NativeScript/runtime/URLSearchParamsImpl.cppTestRunner/app/tests/URLSearchParams.js
🚧 Files skipped from review as they are similar to previous changes (1)
- TestRunner/app/tests/URLSearchParams.js
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.
NathanWalker
approved these changes
Jun 12, 2026
adrian-niculescu
deleted the
fix/urlsearchparams-construction-live-iterators
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the iOS counterpart of NativeScript/android#1970. Same story: we noticed our app hitting a URL that looked like
because the binding only handles the string init form, so an innocent
new URLSearchParams({ limit: "50", cursor: "abc123" })got stringified to"[object Object]"and the whole query collapsed into one bogus key. The same code works in browsers and Node.Same fix as on Android, adapted to this runtime's helpers:
entries()/keys()/values()return live ES iterators per spec instead of plain arrays, and@@iteratoraliasesentries, sofor..of, spread, and thenew URLSearchParams(other)copy form work. This also fixes a duplicate-key bug: the oldget_keys()+get()walk returned the first value for every occurrence of a repeated key.get()returnsnullfor a missing name instead ofundefined(one existing test assertedundefinedand was updated to match the spec).delete(name, value)andhas(name, value)honor the optional value argument, which ada already exposes. An explicitundefinedvalue is treated as omitted, matching WPT.Tests are mirrored from the Android suite (49 specs in the URLSearchParams group; the pre-existing keys/values/entries tests now consume iterators via spread). Verified with the TestRunner scheme on an iPhone 17 simulator (iOS 26.5): TEST SUCCEEDED, 0 failures.
Update after review: every name and value argument across get/getAll/has/delete/append/set now goes through the same USVString coercion (a Symbol throws), entries()/keys()/values() and the iterator's next() brand-check their receivers, and GetPointer no longer reads an internal field from objects that have none (entries.call({}) used to be an out-of-bounds read). A second pass against the IDL and the whatwg-url reference implementation aligned three more corners: new URLSearchParams(null) parses as the string "null" instead of being treated as empty (the union conversion has no null special case; WPT does not cover it and Node is the outlier), an enumerable Symbol key in a record init throws, and the remaining members (get/getAll/has/append/set/delete/forEach/sort/toString/size) brand-check their receiver like the iterator methods.
Summary by CodeRabbit
New Features
Bug Fixes
delete(name[, value])filters by value when provided; explicitundefinedis treated as omitted.get()returnsnullfor missing names; Symbol coercion now throws.Tests