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

url: fix url spec compliance issues #46547

Closed
wants to merge 3 commits into from
Closed
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
21 changes: 17 additions & 4 deletions lib/internal/url.js
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,6 @@ class URLContext {
href = '';
origin = '';
protocol = '';
host = '';
hostname = '';
pathname = '';
search = '';
Expand Down Expand Up @@ -626,14 +625,13 @@ class URL {
return constructHref(this[context], options);
}

#onParseComplete = (href, origin, protocol, host, hostname, pathname,
#onParseComplete = (href, origin, protocol, hostname, pathname,
search, username, password, port, hash, hasHost,
hasOpaquePath) => {
const ctx = this[context];
ctx.href = href;
ctx.origin = origin;
ctx.protocol = protocol;
ctx.host = host;
ctx.hostname = hostname;
ctx.pathname = pathname;
ctx.search = search;
Expand Down Expand Up @@ -716,7 +714,9 @@ class URL {
get host() {
if (!isURLThis(this))
throw new ERR_INVALID_THIS('URL');
return this[context].host;
const port = this[context].port;
const suffix = port.length > 0 ? `:${port}` : '';
return this[context].hostname + suffix;
}

set host(value) {
Expand Down Expand Up @@ -864,6 +864,19 @@ function update(url, params) {
ctx.search = '?' + serializedParams;
} else {
ctx.search = '';

// Potentially strip trailing spaces from an opaque path
if (ctx.hasOpaquePath && ctx.hash.length === 0) {
let length = ctx.pathname.length;
while (length > 0 && ctx.pathname.charCodeAt(length - 1) === 32) {
length--;
}

// No need to copy the whole string if there is no space at the end
if (length !== ctx.pathname.length) {
ctx.pathname = ctx.pathname.slice(0, length);
}
}
}
ctx.href = constructHref(ctx);
}
Expand Down
21 changes: 9 additions & 12 deletions src/node_url.cc
Original file line number Diff line number Diff line change
Expand Up @@ -52,16 +52,15 @@ void SetArgs(Environment* env, Local<Value> argv[12], const ada::result& url) {
argv[0] = Utf8String(isolate, url->get_href());
argv[1] = Utf8String(isolate, url->get_origin());
argv[2] = Utf8String(isolate, url->get_protocol());
argv[3] = Utf8String(isolate, url->get_host());
argv[4] = Utf8String(isolate, url->get_hostname());
argv[5] = Utf8String(isolate, url->get_pathname());
argv[6] = Utf8String(isolate, url->get_search());
argv[7] = Utf8String(isolate, url->get_username());
argv[8] = Utf8String(isolate, url->get_password());
argv[9] = Utf8String(isolate, url->get_port());
argv[10] = Utf8String(isolate, url->get_hash());
argv[11] = Boolean::New(isolate, url->host.has_value());
argv[12] = Boolean::New(isolate, url->has_opaque_path);
argv[3] = Utf8String(isolate, url->get_hostname());
argv[4] = Utf8String(isolate, url->get_pathname());
argv[5] = Utf8String(isolate, url->get_search());
argv[6] = Utf8String(isolate, url->get_username());
argv[7] = Utf8String(isolate, url->get_password());
argv[8] = Utf8String(isolate, url->get_port());
argv[9] = Utf8String(isolate, url->get_hash());
argv[10] = Boolean::New(isolate, url->host.has_value());
argv[11] = Boolean::New(isolate, url->has_opaque_path);
}

void Parse(const FunctionCallbackInfo<Value>& args) {
Expand Down Expand Up @@ -108,7 +107,6 @@ void Parse(const FunctionCallbackInfo<Value>& args) {
undef,
undef,
undef,
undef,
};
SetArgs(env, argv, out);
USE(success_callback_->Call(
Expand Down Expand Up @@ -259,7 +257,6 @@ void UpdateUrl(const FunctionCallbackInfo<Value>& args) {
undef,
undef,
undef,
undef,
};
SetArgs(env, argv, out);
USE(success_callback_->Call(
Expand Down
2 changes: 1 addition & 1 deletion test/fixtures/wpt/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ Last update:
- resource-timing: https://github.com/web-platform-tests/wpt/tree/22d38586d0/resource-timing
- resources: https://github.com/web-platform-tests/wpt/tree/fbf1e7d247/resources
- streams: https://github.com/web-platform-tests/wpt/tree/9e5ef42bd3/streams
- url: https://github.com/web-platform-tests/wpt/tree/0a187bc169/url
- url: https://github.com/web-platform-tests/wpt/tree/f1ade799d0/url
- user-timing: https://github.com/web-platform-tests/wpt/tree/df24fb604e/user-timing
- wasm/jsapi: https://github.com/web-platform-tests/wpt/tree/d8dbe6990b/wasm/jsapi
- wasm/webapi: https://github.com/web-platform-tests/wpt/tree/fd1b23eeaa/wasm/webapi
Expand Down
41 changes: 41 additions & 0 deletions test/fixtures/wpt/url/IdnaTestV2.window.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
promise_test(() => fetch("resources/IdnaTestV2.json").then(res => res.json()).then(runTests), "Loading data…");

// Performance impact of this seems negligible (performance.now() diff in WebKit went from 48 to 52)
// and there was a preference to let more non-ASCII hit the parser.
function encodeHostEndingCodePoints(input) {
let output = "";
for (const codePoint of input) {
if ([":", "/", "?", "#", "\\"].includes(codePoint)) {
output += encodeURIComponent(codePoint);
} else {
output += codePoint;
}
}
return output;
}

function runTests(idnaTests) {
for (const idnaTest of idnaTests) {
if (typeof idnaTest === "string") {
continue // skip comments
}
if (idnaTest.input === "") {
continue // cannot test empty string input through new URL()
}
// Percent-encode the input such that ? and equivalent code points do not end up counting as
// part of the URL, but are parsed through the host parser instead.
const encodedInput = encodeHostEndingCodePoints(idnaTest.input);

test(() => {
if (idnaTest.output === null) {
assert_throws_js(TypeError, () => new URL(`https://${encodedInput}/x`));
} else {
const url = new URL(`https://${encodedInput}/x`);
assert_equals(url.host, idnaTest.output);
assert_equals(url.hostname, idnaTest.output);
assert_equals(url.pathname, "/x");
assert_equals(url.href, `https://${idnaTest.output}/x`);
}
}, `ToASCII("${idnaTest.input}")${idnaTest.comment ? " " + idnaTest.comment : ""}`);
}
}
5 changes: 5 additions & 0 deletions test/fixtures/wpt/url/a-element-xhtml.xhtml
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,13 @@
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<title>URL Test</title>
<meta name="variant" content="?include=file"/>
<meta name="variant" content="?include=javascript"/>
<meta name="variant" content="?include=mailto"/>
<meta name="variant" content="?exclude=(file|javascript|mailto)"/>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script src="/common/subset-tests-by-key.js"></script>
<base id="base"/>
</head>
<body>
Expand Down
5 changes: 5 additions & 0 deletions test/fixtures/wpt/url/a-element.html
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
<!doctype html>
<meta charset=utf-8>
<meta name="variant" content="?include=file">
<meta name="variant" content="?include=javascript">
<meta name="variant" content="?include=mailto">
<meta name="variant" content="?exclude=(file|javascript|mailto)">
<script src=/resources/testharness.js></script>
<script src=/resources/testharnessreport.js></script>
<script src="/common/subset-tests-by-key.js"></script>
<base id=base>
<div id=log></div>
<script src=resources/a-element.js></script>
Expand Down
8 changes: 8 additions & 0 deletions test/fixtures/wpt/url/historical.any.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,12 @@ test(function() {
assert_equals(URL.domainToUnicode, undefined);
}, "URL.domainToUnicode should be undefined");

test(() => {
assert_throws_dom("DataCloneError", () => self.structuredClone(new URL("about:blank")));
}, "URL: no structured serialize/deserialize support");

test(() => {
assert_throws_dom("DataCloneError", () => self.structuredClone(new URLSearchParams()));
}, "URLSearchParams: no structured serialize/deserialize support");

done();
Loading