diff --git a/src/node_url_pattern.cc b/src/node_url_pattern.cc index 3d6340565d1e06..23331460853c29 100644 --- a/src/node_url_pattern.cc +++ b/src/node_url_pattern.cc @@ -165,6 +165,10 @@ void URLPattern::New(const FunctionCallbackInfo& args) { input = input_buffer.ToString(); } else if (args[0]->IsObject()) { init = URLPatternInit::FromJsObject(env, args[0].As()); + // If init does not have a value here, the implication is that an + // error was thrown. Let's allow that to be handled now by returning + // early. If we don't, the error thrown will be swallowed. + if (!init) return; } else { THROW_ERR_INVALID_ARG_TYPE(env, "Input must be an object or a string"); return; @@ -180,7 +184,9 @@ void URLPattern::New(const FunctionCallbackInfo& args) { CHECK(!options.has_value()); options = URLPatternOptions::FromJsObject(env, args[1].As()); if (!options) { - THROW_ERR_INVALID_ARG_TYPE(env, "options.ignoreCase must be a boolean"); + // If options does not have a value, we assume an error was + // thrown and scheduled on the isolate. Return early to + // propagate it. return; } } else { @@ -197,7 +203,9 @@ void URLPattern::New(const FunctionCallbackInfo& args) { CHECK(!options.has_value()); options = URLPatternOptions::FromJsObject(env, args[2].As()); if (!options) { - THROW_ERR_INVALID_ARG_TYPE(env, "options.ignoreCase must be a boolean"); + // If options does not have a value, we assume an error was + // thrown and scheduled on the isolate. Return early to + // propagate it. return; } } @@ -234,73 +242,29 @@ MaybeLocal URLPattern::URLPatternInit::ToJsObject( auto isolate = env->isolate(); auto context = env->context(); auto result = Object::New(isolate); - if (init.protocol) { - Local protocol; - if (!ToV8Value(context, *init.protocol).ToLocal(&protocol) || - result->Set(context, env->protocol_string(), protocol).IsNothing()) { - return {}; - } - } - if (init.username) { - Local username; - if (!ToV8Value(context, *init.username).ToLocal(&username) || - result->Set(context, env->username_string(), username).IsNothing()) { - return {}; - } - } - if (init.password) { - Local password; - if (!ToV8Value(context, *init.password).ToLocal(&password) || - result->Set(context, env->password_string(), password).IsNothing()) { - return {}; - } - } - if (init.hostname) { - Local hostname; - if (!ToV8Value(context, *init.hostname).ToLocal(&hostname) || - result->Set(context, env->hostname_string(), hostname).IsNothing()) { - return {}; - } - } - if (init.port) { - Local port; - if (!ToV8Value(context, *init.port).ToLocal(&port) || - result->Set(context, env->port_string(), port).IsNothing()) { - return {}; - } - } - if (init.pathname) { - Local pathname; - if (!ToV8Value(context, *init.pathname).ToLocal(&pathname) || - result->Set(context, env->pathname_string(), pathname).IsNothing()) { - return {}; - } - } - if (init.search) { - Local search; - if (!ToV8Value(context, *init.search).ToLocal(&search) || - result->Set(context, env->search_string(), search).IsNothing()) { - return {}; - } - } - if (init.hash) { - Local hash; - if (!ToV8Value(context, *init.hash).ToLocal(&hash) || - result->Set(context, env->hash_string(), hash).IsNothing()) { - return {}; - } - } - if (init.base_url) { - Local base_url; - if (!ToV8Value(context, *init.base_url).ToLocal(&base_url) || - result->Set(context, env->base_url_string(), base_url).IsNothing()) { - return {}; - } + + const auto trySet = [&](auto name, const std::optional& val) { + if (!val) return true; + Local temp; + return ToV8Value(context, *val).ToLocal(&temp) && + result->Set(context, name, temp).IsJust(); + }; + + if (!trySet(env->protocol_string(), init.protocol) || + !trySet(env->username_string(), init.username) || + !trySet(env->password_string(), init.password) || + !trySet(env->hostname_string(), init.hostname) || + !trySet(env->port_string(), init.port) || + !trySet(env->pathname_string(), init.pathname) || + !trySet(env->search_string(), init.search) || + !trySet(env->hash_string(), init.hash) || + !trySet(env->base_url_string(), init.base_url)) { + return {}; } return result; } -ada::url_pattern_init URLPattern::URLPatternInit::FromJsObject( +std::optional URLPattern::URLPatternInit::FromJsObject( Environment* env, Local obj) { ada::url_pattern_init init{}; Local components[] = { @@ -344,6 +308,10 @@ ada::url_pattern_init URLPattern::URLPatternInit::FromJsObject( Utf8Value utf8_value(isolate, value); set_parameter(key.ToStringView(), utf8_value.ToStringView()); } + } else { + // If ToLocal failed then we assume an error occurred, + // bail out early to propagate the error. + return std::nullopt; } } return init; @@ -355,12 +323,8 @@ MaybeLocal URLPattern::URLPatternComponentResult::ToJSObject( auto context = env->context(); auto parsed_group = Object::New(isolate); for (const auto& [group_key, group_value] : result.groups) { - Local key; - if (!String::NewFromUtf8(isolate, - group_key.c_str(), - NewStringType::kNormal, - group_key.size()) - .ToLocal(&key)) { + Local key; + if (!ToV8Value(context, group_key).ToLocal(&key)) { return {}; } Local value; @@ -371,7 +335,9 @@ MaybeLocal URLPattern::URLPatternComponentResult::ToJSObject( } else { value = Undefined(isolate); } - USE(parsed_group->Set(context, key, value)); + if (parsed_group->Set(context, key, value).IsNothing()) { + return {}; + } } Local input; if (!ToV8Value(env->context(), result.input).ToLocal(&input)) { @@ -418,34 +384,20 @@ MaybeLocal URLPattern::URLPatternResult::ToJSValue( LocalVector values(isolate, arraysize(names)); values[0] = Array::New(isolate, inputs.data(), inputs.size()); if (!URLPatternComponentResult::ToJSObject(env, result.protocol) - .ToLocal(&values[1])) { - return {}; - } - if (!URLPatternComponentResult::ToJSObject(env, result.username) - .ToLocal(&values[2])) { - return {}; - } - if (!URLPatternComponentResult::ToJSObject(env, result.password) - .ToLocal(&values[3])) { - return {}; - } - if (!URLPatternComponentResult::ToJSObject(env, result.hostname) - .ToLocal(&values[4])) { - return {}; - } - if (!URLPatternComponentResult::ToJSObject(env, result.port) - .ToLocal(&values[5])) { - return {}; - } - if (!URLPatternComponentResult::ToJSObject(env, result.pathname) - .ToLocal(&values[6])) { - return {}; - } - if (!URLPatternComponentResult::ToJSObject(env, result.search) - .ToLocal(&values[7])) { - return {}; - } - if (!URLPatternComponentResult::ToJSObject(env, result.hash) + .ToLocal(&values[1]) || + !URLPatternComponentResult::ToJSObject(env, result.username) + .ToLocal(&values[2]) || + !URLPatternComponentResult::ToJSObject(env, result.password) + .ToLocal(&values[3]) || + !URLPatternComponentResult::ToJSObject(env, result.hostname) + .ToLocal(&values[4]) || + !URLPatternComponentResult::ToJSObject(env, result.port) + .ToLocal(&values[5]) || + !URLPatternComponentResult::ToJSObject(env, result.pathname) + .ToLocal(&values[6]) || + !URLPatternComponentResult::ToJSObject(env, result.search) + .ToLocal(&values[7]) || + !URLPatternComponentResult::ToJSObject(env, result.hash) .ToLocal(&values[8])) { return {}; } @@ -461,9 +413,15 @@ URLPattern::URLPatternOptions::FromJsObject(Environment* env, if (obj->Get(env->context(), env->ignore_case_string()) .ToLocal(&ignore_case)) { if (!ignore_case->IsBoolean()) { + THROW_ERR_INVALID_ARG_TYPE(env, "options.ignoreCase must be a boolean"); return std::nullopt; } options.ignore_case = ignore_case->IsTrue(); + } else { + // If ToLocal returns false, the assumption is that getting the + // ignore_case_string threw an error, let's propagate that now + // by returning std::nullopt. + return std::nullopt; } return options; } @@ -553,7 +511,9 @@ void URLPattern::Exec(const FunctionCallbackInfo& args) { input_base = input_value.ToString(); input = std::string_view(input_base); } else if (args[0]->IsObject()) { - input = URLPatternInit::FromJsObject(env, args[0].As()); + auto maybeInput = URLPatternInit::FromJsObject(env, args[0].As()); + if (!maybeInput.has_value()) return; + input = std::move(*maybeInput); } else { THROW_ERR_INVALID_ARG_TYPE( env, "URLPattern input needs to be a string or an object"); @@ -594,7 +554,9 @@ void URLPattern::Test(const FunctionCallbackInfo& args) { input_base = input_value.ToString(); input = std::string_view(input_base); } else if (args[0]->IsObject()) { - input = URLPatternInit::FromJsObject(env, args[0].As()); + auto maybeInput = URLPatternInit::FromJsObject(env, args[0].As()); + if (!maybeInput.has_value()) return; + input = std::move(*maybeInput); } else { THROW_ERR_INVALID_ARG_TYPE( env, "URLPattern input needs to be a string or an object"); diff --git a/src/node_url_pattern.h b/src/node_url_pattern.h index 99a2521513b55a..bfb0cd00fa8151 100644 --- a/src/node_url_pattern.h +++ b/src/node_url_pattern.h @@ -59,8 +59,8 @@ class URLPattern : public BaseObject { class URLPatternInit { public: - static ada::url_pattern_init FromJsObject(Environment* env, - v8::Local obj); + static std::optional FromJsObject( + Environment* env, v8::Local obj); static v8::MaybeLocal ToJsObject( Environment* env, const ada::url_pattern_init& init); }; diff --git a/test/parallel/test-urlpattern.js b/test/parallel/test-urlpattern.js new file mode 100644 index 00000000000000..1a8d722c5d3e87 --- /dev/null +++ b/test/parallel/test-urlpattern.js @@ -0,0 +1,28 @@ +'use strict'; + +require('../common'); + +const { throws } = require('assert'); +const { URLPattern } = require('url'); + +// Verify that if an error is thrown while accessing any of the +// init options, the error is appropriately propagated. +throws(() => { + new URLPattern({ + get protocol() { + throw new Error('boom'); + } + }); +}, { + message: 'boom', +}); + +// Verify that if an error is thrown while accessing the ignoreCase +// option, the error is appropriately propagated. +throws(() => { + new URLPattern({}, { get ignoreCase() { + throw new Error('boom'); + } }); +}, { + message: 'boom' +});