Skip to content

simplify process methods in url pattern #999

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

simplify process methods in url pattern #999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
911abb3
simplify process methods in url pattern
anonrig Sep 24, 2025
2a4a265
increase fuzzing coverage on urlpattern
anonrig Sep 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion fuzz/url_pattern.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,22 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
}
auto result_with_init = ada::parse_url_pattern<regex_provider>(
init, &base_source_view, nullptr);
if (result_with_init) exercise_result(*result_with_init);
if (result_with_init) {
exercise_result(*result_with_init);
}

ada::url_pattern_init hostname_init{};
hostname_init.hostname = "*";
auto valid_urlpattern =
ada::parse_url_pattern<regex_provider>(hostname_init, nullptr, nullptr);
std::string_view valid_input =
"https://www.yagiz.co/???this-is-my-search#####this-is-hash";
(void)valid_urlpattern->exec(valid_input, nullptr);
(void)valid_urlpattern->exec(valid_input, &base_source_view);
(void)valid_urlpattern->exec(base_source_view, nullptr);
(void)valid_urlpattern->test(valid_input, nullptr);
(void)valid_urlpattern->test(valid_input, &base_source_view);
(void)valid_urlpattern->test(base_source_view, nullptr);
}

return 0;
Expand Down
40 changes: 19 additions & 21 deletions src/url_pattern.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,13 +261,13 @@ tl::expected<url_pattern_init, errors> url_pattern_init::process(
tl::expected<std::string, errors> url_pattern_init::process_protocol(
std::string_view value, process_type type) {
ada_log("process_protocol=", value, " [", type, "]");
// Let strippedValue be the given value with a single trailing U+003A (:)
// removed, if any.
if (value.ends_with(":")) {
value.remove_suffix(1);
}
// If type is "pattern" then return strippedValue.
if (type == process_type::pattern) {
// Let strippedValue be the given value with a single trailing U+003A (:)
// removed, if any.
if (value.ends_with(":")) {
value.remove_suffix(1);
}
return std::string(value);
}
// Return the result of running canonicalize a protocol given strippedValue.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could add as a comment that canonicalize_protocol will already remove the trailing : if any ? (optional)

Expand Down Expand Up @@ -336,18 +336,17 @@ tl::expected<std::string, errors> url_pattern_init::process_pathname(

tl::expected<std::string, errors> url_pattern_init::process_search(
std::string_view value, process_type type) {
// Let strippedValue be the given value with a single leading U+003F (?)
// removed, if any.
if (value.starts_with("?")) {
value.remove_prefix(1);
}
// We cannot assert that the value is no longer starting with a single
// question mark because technically it can start. The question is whether or
// not we should remove the first question mark. Ref:
// https://github.com/ada-url/ada/pull/992 The spec is not clear on this.

// If type is "pattern" then return strippedValue.
if (type == process_type::pattern) {
// Let strippedValue be the given value with a single leading U+003F (?)
// removed, if any.
if (value.starts_with("?")) {
value.remove_prefix(1);
}
// We cannot assert that the value is no longer starting with a single
// question mark because technically it can start. The question is whether
// or not we should remove the first question mark. Ref:
// https://github.com/ada-url/ada/pull/992 The spec is not clear on this.
return std::string(value);
}
// Return the result of running canonicalize a search given strippedValue.
Expand All @@ -356,14 +355,13 @@ tl::expected<std::string, errors> url_pattern_init::process_search(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If type != process_type::pattern, then you are no longer removing an eventual ?. But I don't see canonicalize_search doing the check. So it looks like you might have made a change in the logic?

tl::expected<std::string, errors> url_pattern_init::process_hash(
std::string_view value, process_type type) {
// Let strippedValue be the given value with a single leading U+0023 (#)
// removed, if any.
if (value.starts_with("#")) {
value.remove_prefix(1);
}
ADA_ASSERT_TRUE(!value.starts_with("#"));
// If type is "pattern" then return strippedValue.
if (type == process_type::pattern) {
// Let strippedValue be the given value with a single leading U+0023 (#)
// removed, if any.
if (value.starts_with("#")) {
value.remove_prefix(1);
}
return std::string(value);
}
// Return the result of running canonicalize a hash given strippedValue.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the same question here... you might now pass a leading # to canonicalize_hash whereas you use not to. Why is this ok?

Expand Down
Loading