From 423c6c6df694b1e4c4769a3ef5687916455de027 Mon Sep 17 00:00:00 2001 From: Alessio Attilio Date: Sun, 26 Jul 2026 20:16:38 +0200 Subject: [PATCH] url: fix URLPattern.exec() capture group property insertion order --- src/node_url_pattern.cc | 47 +++++++---- src/node_url_pattern.h | 12 ++- .../test-urlpattern-exec-groups-order.js | 81 +++++++++++++++++++ 3 files changed, 119 insertions(+), 21 deletions(-) create mode 100644 test/parallel/test-urlpattern-exec-groups-order.js diff --git a/src/node_url_pattern.cc b/src/node_url_pattern.cc index 571413b47da12d..fc313e07f78f07 100644 --- a/src/node_url_pattern.cc +++ b/src/node_url_pattern.cc @@ -409,22 +409,25 @@ std::optional URLPattern::URLPatternInit::FromJsObject( } MaybeLocal URLPattern::URLPatternComponentResult::ToJSObject( - Environment* env, const ada::url_pattern_component_result& result) { + Environment* env, + const ada::url_pattern_component_result& result, + const std::vector& group_name_list) { auto isolate = env->isolate(); auto context = env->context(); LocalVector group_names(isolate); LocalVector group_values(isolate); - group_names.reserve(result.groups.size()); - group_values.reserve(result.groups.size()); - for (const auto& [group_key, group_value] : result.groups) { + group_names.reserve(group_name_list.size()); + group_values.reserve(group_name_list.size()); + for (const auto& name : group_name_list) { Local key; - if (!ToV8Value(context, group_key).ToLocal(&key)) { + if (!ToV8Value(context, name).ToLocal(&key)) { return {}; } group_names.push_back(key.As()); Local value; - if (group_value) { - if (!ToV8Value(env->context(), *group_value).ToLocal(&value)) { + auto it = result.groups.find(name); + if (it != result.groups.end() && it->second) { + if (!ToV8Value(env->context(), *it->second).ToLocal(&value)) { return {}; } } else { @@ -457,7 +460,9 @@ MaybeLocal URLPattern::URLPatternComponentResult::ToJSObject( } MaybeLocal URLPattern::URLPatternResult::ToJSValue( - Environment* env, const ada::url_pattern_result& result) { + Environment* env, + const ada::url_pattern_result& result, + const ada::url_pattern& url_pattern) { auto isolate = env->isolate(); auto tmpl = env->urlpatternresult_template(); @@ -493,14 +498,22 @@ MaybeLocal URLPattern::URLPatternResult::ToJSValue( return URLPatternInit::ToJsObject(env, init); } }), - URLPatternComponentResult::ToJSObject(env, result.protocol), - URLPatternComponentResult::ToJSObject(env, result.username), - URLPatternComponentResult::ToJSObject(env, result.password), - URLPatternComponentResult::ToJSObject(env, result.hostname), - URLPatternComponentResult::ToJSObject(env, result.port), - URLPatternComponentResult::ToJSObject(env, result.pathname), - URLPatternComponentResult::ToJSObject(env, result.search), - URLPatternComponentResult::ToJSObject(env, result.hash)}; + URLPatternComponentResult::ToJSObject( + env, result.protocol, url_pattern.protocol_component.group_name_list), + URLPatternComponentResult::ToJSObject( + env, result.username, url_pattern.username_component.group_name_list), + URLPatternComponentResult::ToJSObject( + env, result.password, url_pattern.password_component.group_name_list), + URLPatternComponentResult::ToJSObject( + env, result.hostname, url_pattern.hostname_component.group_name_list), + URLPatternComponentResult::ToJSObject( + env, result.port, url_pattern.port_component.group_name_list), + URLPatternComponentResult::ToJSObject( + env, result.pathname, url_pattern.pathname_component.group_name_list), + URLPatternComponentResult::ToJSObject( + env, result.search, url_pattern.search_component.group_name_list), + URLPatternComponentResult::ToJSObject( + env, result.hash, url_pattern.hash_component.group_name_list)}; return NewDictionaryInstanceNullProto(env->context(), tmpl, vals); } @@ -552,7 +565,7 @@ MaybeLocal URLPattern::Exec(Environment* env, std::optional& baseURL) { if (auto result = url_pattern_.exec(input, baseURL ? &*baseURL : nullptr)) { if (result->has_value()) { - return URLPatternResult::ToJSValue(env, result->value()); + return URLPatternResult::ToJSValue(env, result->value(), url_pattern_); } return Null(env->isolate()); } diff --git a/src/node_url_pattern.h b/src/node_url_pattern.h index 48f3956cb74458..99784316245037 100644 --- a/src/node_url_pattern.h +++ b/src/node_url_pattern.h @@ -80,14 +80,18 @@ class URLPattern : public BaseObject { class URLPatternResult { public: - static v8::MaybeLocal ToJSValue( - Environment* env, const ada::url_pattern_result& result); + static v8::MaybeLocal ToJSValue( + Environment* env, + const ada::url_pattern_result& result, + const ada::url_pattern& url_pattern); }; class URLPatternComponentResult { public: - static v8::MaybeLocal ToJSObject( - Environment* env, const ada::url_pattern_component_result& result); + static v8::MaybeLocal ToJSObject( + Environment* env, + const ada::url_pattern_component_result& result, + const std::vector& group_name_list); }; private: diff --git a/test/parallel/test-urlpattern-exec-groups-order.js b/test/parallel/test-urlpattern-exec-groups-order.js new file mode 100644 index 00000000000000..516206a4fec8e0 --- /dev/null +++ b/test/parallel/test-urlpattern-exec-groups-order.js @@ -0,0 +1,81 @@ +'use strict'; + +require('../common'); + +const assert = require('assert'); +const { URLPattern } = require('url'); + +// Regression test for: URLPattern.exec() does not preserve capture group +// declaration order. The groups object returned for each component must +// enumerate its named capture groups in the order they appear in the +// pattern string. + +{ + // Three-group pathname pattern: one, two, three + const pattern = new URLPattern({ pathname: '/:one/:two/:three' }); + const result = pattern.exec('https://example.com/a/b/c'); + + assert.ok(result, 'exec should return a result'); + + const keys = Object.keys(result.pathname.groups); + assert.deepStrictEqual( + keys, + ['one', 'two', 'three'], + 'pathname capture groups must be enumerated in declaration order', + ); + assert.strictEqual(result.pathname.groups.one, 'a'); + assert.strictEqual(result.pathname.groups.two, 'b'); + assert.strictEqual(result.pathname.groups.three, 'c'); +} + +{ + // Reversed alphabetical order to distinguish declaration order from sort + // order: z comes before a in declaration, so must appear first. + const pattern = new URLPattern({ pathname: '/:z/:a/:m' }); + const result = pattern.exec('https://example.com/1/2/3'); + + assert.ok(result, 'exec should return a result'); + + assert.deepStrictEqual( + Object.keys(result.pathname.groups), + ['z', 'a', 'm'], + 'groups must follow declaration order even when names are not alphabetical', + ); +} + +{ + // Multiple components can each carry named groups independently; verify + // that declaration order is preserved per-component. + const pattern = new URLPattern({ + hostname: ':sub.:root', + pathname: '/:last/:first', + }); + const result = pattern.exec('https://api.example.com/smith/john'); + + assert.ok(result, 'exec should return a result'); + + assert.deepStrictEqual( + Object.keys(result.hostname.groups), + ['sub', 'root'], + 'hostname groups must follow declaration order', + ); + + assert.deepStrictEqual( + Object.keys(result.pathname.groups), + ['last', 'first'], + 'pathname groups must follow declaration order', + ); +} + +{ + // A pattern with no named groups should produce an empty groups object. + const pattern = new URLPattern({ pathname: '/fixed/path' }); + const result = pattern.exec('https://example.com/fixed/path'); + + assert.ok(result, 'exec should return a result'); + assert.deepStrictEqual( + Object.keys(result.pathname.groups), + [], + 'a pattern without named groups must yield an empty groups object', + ); +}