From 2fc5878668d8f08ec6989d07e79125d640a73d43 Mon Sep 17 00:00:00 2001 From: Marcos Casagrande Date: Tue, 28 Apr 2020 07:23:06 +0200 Subject: [PATCH] Change URL.port implementation to match WHATWG specifications (#4954) Changed `URL.port` implementation to match [WHATWG specifications](https://url.spec.whatwg.org/#port-state). This PR matches the behaviour of other browsers: 1. a `TypeError` must be thrown when passing an URL with an invalid port to the constructor. 2. When setting an invalid port, using property setter, I haven't found what should happen in this case, so I mimic **Firefox** & **Node** behaviour. If an invalid port is set, it will use the previous value. **Chrome** sets the value to `'0'` if an invalid port is set. I prefer to keep the previous valid value. (I can use Chrome's behaviour if you think it's better, it's a simple value change) ``` url.port = '3000'; // valid url.port = 'deno'; // invalid assertEquals(url.port, '3000'); ``` 3. If the port value equals the current protocol default port value, `port` will be an empty string. --- cli/js/tests/url_test.ts | 101 +++++++++++++++++++++++++++++++++++++++ cli/js/web/url.ts | 42 ++++++++++++++-- 2 files changed, 138 insertions(+), 5 deletions(-) diff --git a/cli/js/tests/url_test.ts b/cli/js/tests/url_test.ts index e4d497ffb4..e8348b206a 100644 --- a/cli/js/tests/url_test.ts +++ b/cli/js/tests/url_test.ts @@ -212,3 +212,104 @@ unitTest(function createBadUrl(): void { new URL("0.0.0.0:8080"); }); }); + +unitTest(function throwForInvalidPortConstructor(): void { + const urls = [ + // If port is greater than 2^16 − 1, validation error, return failure. + `https://baz.qat:${2 ** 16}`, + "https://baz.qat:-32", + "https://baz.qat:deno", + ]; + + for (const url of urls) { + assertThrows(() => new URL(url)); + } +}); + +unitTest(function doNotOverridePortIfInvalid(): void { + const initialPort = "3000"; + const ports = [ + // If port is greater than 2^16 − 1, validation error, return failure. + `${2 ** 16}`, + "-32", + "deno", + ]; + + for (const port of ports) { + const url = new URL(`https://deno.land:${initialPort}`); + url.port = port; + assertEquals(url.port, initialPort); + } +}); + +unitTest(function doNotOverridePortIfInvalid(): void { + const initialPort = "3000"; + const ports = [ + // If port is greater than 2^16 − 1, validation error, return failure. + `${2 ** 16}`, + "-32", + "deno", + ]; + + for (const port of ports) { + const url = new URL(`https://deno.land:${initialPort}`); + url.port = port; + assertEquals(url.port, initialPort); + } +}); + +unitTest(function emptyPortForSchemeDefaultPort(): void { + const nonDefaultPort = "3500"; + const urls = [ + { url: "ftp://baz.qat:21", port: "21", protocol: "ftp:" }, + { url: "https://baz.qat:443", port: "443", protocol: "https:" }, + { url: "wss://baz.qat:443", port: "443", protocol: "wss:" }, + { url: "http://baz.qat:80", port: "80", protocol: "http:" }, + { url: "ws://baz.qat:80", port: "80", protocol: "ws:" }, + { url: "file://home/index.html", port: "", protocol: "file:" }, + { url: "/foo", baseUrl: "ftp://baz.qat:21", port: "21", protocol: "ftp:" }, + { + url: "/foo", + baseUrl: "https://baz.qat:443", + port: "443", + protocol: "https:", + }, + { + url: "/foo", + baseUrl: "wss://baz.qat:443", + port: "443", + protocol: "wss:", + }, + { + url: "/foo", + baseUrl: "http://baz.qat:80", + port: "80", + protocol: "http:", + }, + { url: "/foo", baseUrl: "ws://baz.qat:80", port: "80", protocol: "ws:" }, + { + url: "/foo", + baseUrl: "file://home/index.html", + port: "", + protocol: "file:", + }, + ]; + + for (const { url: urlString, baseUrl, port, protocol } of urls) { + const url = new URL(urlString, baseUrl); + assertEquals(url.port, ""); + + url.port = nonDefaultPort; + assertEquals(url.port, nonDefaultPort); + + url.port = port; + assertEquals(url.port, ""); + + // change scheme + url.protocol = "sftp:"; + assertEquals(url.port, port); + + url.protocol = protocol; + assertEquals(url.port, ""); + } +}); diff --git a/cli/js/web/url.ts b/cli/js/web/url.ts index 374ddd252c..77285e456a 100644 --- a/cli/js/web/url.ts +++ b/cli/js/web/url.ts @@ -40,6 +40,17 @@ const searchParamsMethods: Array = [ "set", ]; +// https://url.spec.whatwg.org/#special-scheme +const schemePorts: { [key: string]: string } = { + ftp: "21", + file: "", + http: "80", + https: "443", + ws: "80", + wss: "443", +}; +const MAX_PORT = 2 ** 16 - 1; + function parse(url: string): URLParts | undefined { const urlMatch = urlRegExp.exec(url); if (urlMatch) { @@ -178,6 +189,18 @@ export class URLImpl implements URL { urls.set(searchParams, this); }; + #validatePort = (value: string): string | undefined => { + // https://url.spec.whatwg.org/#port-state + if (value === "") return value; + + const port = parseInt(value, 10); + + if (!Number.isNaN(port) && port > 0 && port <= MAX_PORT) + return port.toString(); + + return undefined; + }; + get hash(): string { return parts.get(this)!.hash; } @@ -268,14 +291,17 @@ export class URLImpl implements URL { } get port(): string { - return parts.get(this)!.port; + const port = parts.get(this)!.port; + if (schemePorts[parts.get(this)!.protocol] === port) { + return ""; + } + + return port; } set port(value: string) { - const port = parseInt(String(value), 10); - parts.get(this)!.port = isNaN(port) - ? "" - : Math.max(0, port % 2 ** 16).toString(); + const port = this.#validatePort(value); + parts.get(this)!.port = port ?? this.port; } get protocol(): string { @@ -344,6 +370,11 @@ export class URLImpl implements URL { throw new TypeError("Invalid URL."); } + const { port } = (urlParts.protocol ? urlParts : baseParts) as URLParts; + if (this.#validatePort(port) === undefined) { + throw new TypeError("Invalid URL."); + } + if (urlParts.protocol) { parts.set(this, urlParts); } else if (baseParts) { @@ -360,6 +391,7 @@ export class URLImpl implements URL { } else { throw new TypeError("URL requires a base URL."); } + this.#updateSearchParams(); }