Skip to content
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.

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

verifies and fixes issue 50235 in node. #540

Merged
merged 3 commits into from
Oct 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
29 changes: 20 additions & 9 deletions include/ada/url_aggregator-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -817,20 +817,31 @@ inline bool url_aggregator::has_port() const noexcept {
// is greater than 1, and url's path[0] is the empty string, then append
// U+002F (/) followed by U+002E (.) to output.
ada_log("url_aggregator::has_dash_dot");
// Performance: instead of doing this potentially expensive check, we could
// just have a boolean value in the structure.
#if ADA_DEVELOPMENT_CHECKS
if (components.pathname_start + 1 < buffer.size() &&
components.pathname_start == components.host_end + 2) {
ADA_ASSERT_TRUE(buffer[components.host_end] == '/');
ADA_ASSERT_TRUE(buffer[components.host_end + 1] == '.');
// If pathname_start and host_end are exactly two characters apart, then we
// either have a one-digit port such as http://test.com:5?param=1 or else we
// have a /.: sequence such as "non-spec:/.//". We test that this is the case.
if (components.pathname_start == components.host_end + 2) {
ADA_ASSERT_TRUE((buffer[components.host_end] == '/' &&
buffer[components.host_end + 1] == '.') ||
(buffer[components.host_end] == ':' &&
checkers::is_digit(buffer[components.host_end + 1])));
}
if (components.pathname_start == components.host_end + 2 &&
buffer[components.host_end] == '/' &&
buffer[components.host_end + 1] == '.') {
ADA_ASSERT_TRUE(components.pathname_start + 1 < buffer.size());
ADA_ASSERT_TRUE(buffer[components.pathname_start] == '/');
ADA_ASSERT_TRUE(buffer[components.pathname_start + 1] == '/');
}
#endif
return !has_opaque_path &&
components.pathname_start == components.host_end + 2 &&
components.pathname_start + 1 < buffer.size();
// Performance: it should be uncommon for components.pathname_start ==
// components.host_end + 2 to be true. So we put this check first in the
// sequence. Most times, we do not have an opaque path. Checking for '/.' is
// more expensive, but should be uncommon.
return components.pathname_start == components.host_end + 2 &&
!has_opaque_path && buffer[components.host_end] == '/' &&
buffer[components.host_end + 1] == '.';
}

[[nodiscard]] inline std::string_view url_aggregator::get_href()
Expand Down
9 changes: 9 additions & 0 deletions tests/basic_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -387,3 +387,12 @@ TYPED_TEST(basic_tests, nodejs_49650) {
ASSERT_EQ(out->get_href(), "http://foo/");
SUCCEED();
}

// https://github.com/nodejs/node/issues/50235
TYPED_TEST(basic_tests, nodejs_50235) {
auto out = ada::parse<TypeParam>("http://test.com:5/?param=1");
ASSERT_TRUE(out);
ASSERT_TRUE(out->set_pathname("path"));
ASSERT_EQ(out->get_href(), "http://test.com:5/path?param=1");
SUCCEED();
}
Loading