From 027841c964998c87ff684cae5d3da647df8f37d9 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Fri, 3 Mar 2023 16:34:56 -0500 Subject: [PATCH] url: use private properties for brand check PR-URL: https://github.com/nodejs/node/pull/46904 Reviewed-By: Antoine du Hamel Reviewed-By: Ruben Bridgewater Reviewed-By: Benjamin Gruenbaum --- lib/internal/modules/cjs/loader.js | 4 +- lib/internal/modules/esm/hooks.js | 4 +- lib/internal/url.js | 163 +++++++----------- lib/internal/worker.js | 6 +- .../test-whatwg-url-custom-inspect.js | 2 +- test/parallel/test-whatwg-url-invalidthis.js | 12 +- 6 files changed, 74 insertions(+), 117 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index c0b8cbc8a1e099..5544bfee9aac31 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -79,7 +79,7 @@ const { BuiltinModule } = require('internal/bootstrap/loaders'); const { maybeCacheSourceMap, } = require('internal/source_map/source_map_cache'); -const { pathToFileURL, fileURLToPath, isURLInstance } = require('internal/url'); +const { pathToFileURL, fileURLToPath, isURL } = require('internal/url'); const { deprecate, emitExperimentalWarning, @@ -1396,7 +1396,7 @@ const createRequireError = 'must be a file URL object, file URL string, or ' + function createRequire(filename) { let filepath; - if (isURLInstance(filename) || + if (isURL(filename) || (typeof filename === 'string' && !path.isAbsolute(filename))) { try { filepath = fileURLToPath(filename); diff --git a/lib/internal/modules/esm/hooks.js b/lib/internal/modules/esm/hooks.js index 83e5038903df83..9c5c4c99b887d0 100644 --- a/lib/internal/modules/esm/hooks.js +++ b/lib/internal/modules/esm/hooks.js @@ -21,7 +21,7 @@ const { ERR_INVALID_RETURN_PROPERTY_VALUE, ERR_INVALID_RETURN_VALUE, } = require('internal/errors').codes; -const { isURLInstance, URL } = require('internal/url'); +const { isURL, URL } = require('internal/url'); const { isAnyArrayBuffer, isArrayBufferView, @@ -263,7 +263,7 @@ class Hooks { if ( !isMain && typeof parentURL !== 'string' && - !isURLInstance(parentURL) + !isURL(parentURL) ) { throw new ERR_INVALID_ARG_TYPE( 'parentURL', diff --git a/lib/internal/url.js b/lib/internal/url.js index 193965b76d346d..3a44c529876a08 100644 --- a/lib/internal/url.js +++ b/lib/internal/url.js @@ -7,6 +7,7 @@ const { ArrayPrototypePush, ArrayPrototypeReduce, ArrayPrototypeSlice, + Boolean, Int8Array, IteratorPrototype, Number, @@ -15,7 +16,6 @@ const { ObjectGetOwnPropertySymbols, ObjectGetPrototypeOf, ObjectKeys, - ObjectPrototypeHasOwnProperty, ReflectGetOwnPropertyDescriptor, ReflectOwnKeys, RegExpPrototypeSymbolReplace, @@ -534,15 +534,27 @@ ObjectDefineProperties(URLSearchParams.prototype, { }, }); +/** + * Checks if a value has the shape of a WHATWG URL object. + * + * Using a symbol or instanceof would not be able to recognize URL objects + * coming from other implementations (e.g. in Electron), so instead we are + * checking some well known properties for a lack of a better test. + * + * @param {*} self + * @returns {self is URL} + */ function isURL(self) { - return self != null && ObjectPrototypeHasOwnProperty(self, context); + return Boolean(self?.href && self.origin); } class URL { + #context = new URLContext(); + #searchParams; + constructor(input, base = undefined) { // toUSVString is not needed. input = `${input}`; - this[context] = new URLContext(); if (base !== undefined) { base = `${base}`; @@ -558,11 +570,6 @@ class URL { } [inspect.custom](depth, opts) { - if (this == null || - ObjectGetPrototypeOf(this[context]) !== URLContext.prototype) { - throw new ERR_INVALID_THIS('URL'); - } - if (typeof depth === 'number' && depth < 0) return this; @@ -583,7 +590,7 @@ class URL { obj.hash = this.hash; if (opts.showHidden) { - obj[context] = this[context]; + obj[context] = this.#context; } return `${constructor.name} ${inspect(obj, opts)}`; @@ -591,174 +598,125 @@ class URL { #onParseComplete = (href, origin, protocol, hostname, pathname, search, username, password, port, hash) => { - const ctx = this[context]; - ctx.href = href; - ctx.origin = origin; - ctx.protocol = protocol; - ctx.hostname = hostname; - ctx.pathname = pathname; - ctx.search = search; - ctx.username = username; - ctx.password = password; - ctx.port = port; - ctx.hash = hash; - if (this[searchParams]) { - this[searchParams][searchParams] = parseParams(search); + this.#context.href = href; + this.#context.origin = origin; + this.#context.protocol = protocol; + this.#context.hostname = hostname; + this.#context.pathname = pathname; + this.#context.search = search; + this.#context.username = username; + this.#context.password = password; + this.#context.port = port; + this.#context.hash = hash; + if (this.#searchParams) { + this.#searchParams[searchParams] = parseParams(search); } }; toString() { - if (!isURL(this)) - throw new ERR_INVALID_THIS('URL'); - return this[context].href; + return this.#context.href; } get href() { - if (!isURL(this)) - throw new ERR_INVALID_THIS('URL'); - return this[context].href; + return this.#context.href; } set href(value) { - if (!isURL(this)) - throw new ERR_INVALID_THIS('URL'); - const valid = updateUrl(this[context].href, updateActions.kHref, `${value}`, this.#onParseComplete); + const valid = updateUrl(this.#context.href, updateActions.kHref, `${value}`, this.#onParseComplete); if (!valid) { throw ERR_INVALID_URL(`${value}`); } } // readonly get origin() { - if (!isURL(this)) - throw new ERR_INVALID_THIS('URL'); - return this[context].origin; + return this.#context.origin; } get protocol() { - if (!isURL(this)) - throw new ERR_INVALID_THIS('URL'); - return this[context].protocol; + return this.#context.protocol; } set protocol(value) { - if (!isURL(this)) - throw new ERR_INVALID_THIS('URL'); - updateUrl(this[context].href, updateActions.kProtocol, `${value}`, this.#onParseComplete); + updateUrl(this.#context.href, updateActions.kProtocol, `${value}`, this.#onParseComplete); } get username() { - if (!isURL(this)) - throw new ERR_INVALID_THIS('URL'); - return this[context].username; + return this.#context.username; } set username(value) { - if (!isURL(this)) - throw new ERR_INVALID_THIS('URL'); - updateUrl(this[context].href, updateActions.kUsername, `${value}`, this.#onParseComplete); + updateUrl(this.#context.href, updateActions.kUsername, `${value}`, this.#onParseComplete); } get password() { - if (!isURL(this)) - throw new ERR_INVALID_THIS('URL'); - return this[context].password; + return this.#context.password; } set password(value) { - if (!isURL(this)) - throw new ERR_INVALID_THIS('URL'); - updateUrl(this[context].href, updateActions.kPassword, `${value}`, this.#onParseComplete); + updateUrl(this.#context.href, updateActions.kPassword, `${value}`, this.#onParseComplete); } get host() { - if (!isURL(this)) - throw new ERR_INVALID_THIS('URL'); - const port = this[context].port; + const port = this.#context.port; const suffix = port.length > 0 ? `:${port}` : ''; - return this[context].hostname + suffix; + return this.#context.hostname + suffix; } set host(value) { - if (!isURL(this)) - throw new ERR_INVALID_THIS('URL'); - updateUrl(this[context].href, updateActions.kHost, `${value}`, this.#onParseComplete); + updateUrl(this.#context.href, updateActions.kHost, `${value}`, this.#onParseComplete); } get hostname() { - if (!isURL(this)) - throw new ERR_INVALID_THIS('URL'); - return this[context].hostname; + return this.#context.hostname; } set hostname(value) { - if (!isURL(this)) - throw new ERR_INVALID_THIS('URL'); - updateUrl(this[context].href, updateActions.kHostname, `${value}`, this.#onParseComplete); + updateUrl(this.#context.href, updateActions.kHostname, `${value}`, this.#onParseComplete); } get port() { - if (!isURL(this)) - throw new ERR_INVALID_THIS('URL'); - return this[context].port; + return this.#context.port; } set port(value) { - if (!isURL(this)) - throw new ERR_INVALID_THIS('URL'); - updateUrl(this[context].href, updateActions.kPort, `${value}`, this.#onParseComplete); + updateUrl(this.#context.href, updateActions.kPort, `${value}`, this.#onParseComplete); } get pathname() { - if (!isURL(this)) - throw new ERR_INVALID_THIS('URL'); - return this[context].pathname; + return this.#context.pathname; } set pathname(value) { - if (!isURL(this)) - throw new ERR_INVALID_THIS('URL'); - updateUrl(this[context].href, updateActions.kPathname, `${value}`, this.#onParseComplete); + updateUrl(this.#context.href, updateActions.kPathname, `${value}`, this.#onParseComplete); } get search() { - if (!isURL(this)) - throw new ERR_INVALID_THIS('URL'); - return this[context].search; + return this.#context.search; } set search(value) { - if (!isURL(this)) - throw new ERR_INVALID_THIS('URL'); - updateUrl(this[context].href, updateActions.kSearch, toUSVString(value), this.#onParseComplete); + updateUrl(this.#context.href, updateActions.kSearch, toUSVString(value), this.#onParseComplete); } // readonly get searchParams() { - if (!isURL(this)) - throw new ERR_INVALID_THIS('URL'); // Create URLSearchParams on demand to greatly improve the URL performance. - if (this[searchParams] == null) { - this[searchParams] = new URLSearchParams(this[context].search); - this[searchParams][context] = this; + if (this.#searchParams == null) { + this.#searchParams = new URLSearchParams(this.#context.search); + this.#searchParams[context] = this; } - return this[searchParams]; + return this.#searchParams; } get hash() { - if (!isURL(this)) - throw new ERR_INVALID_THIS('URL'); - return this[context].hash; + return this.#context.hash; } set hash(value) { - if (!isURL(this)) - throw new ERR_INVALID_THIS('URL'); - updateUrl(this[context].href, updateActions.kHash, `${value}`, this.#onParseComplete); + updateUrl(this.#context.href, updateActions.kHash, `${value}`, this.#onParseComplete); } toJSON() { - if (!isURL(this)) - throw new ERR_INVALID_THIS('URL'); - return this[context].href; + return this.#context.href; } static createObjectURL(obj) { @@ -1206,7 +1164,7 @@ function getPathFromURLPosix(url) { function fileURLToPath(path) { if (typeof path === 'string') path = new URL(path); - else if (!isURLInstance(path)) + else if (!isURL(path)) throw new ERR_INVALID_ARG_TYPE('path', ['string', 'URL'], path); if (path.protocol !== 'file:') throw new ERR_INVALID_URL_SCHEME('file'); @@ -1282,12 +1240,8 @@ function pathToFileURL(filepath) { return outURL; } -function isURLInstance(fileURLOrPath) { - return fileURLOrPath != null && fileURLOrPath.href && fileURLOrPath.origin; -} - function toPathIfFileURL(fileURLOrPath) { - if (!isURLInstance(fileURLOrPath)) + if (!isURL(fileURLOrPath)) return fileURLOrPath; return fileURLToPath(fileURLOrPath); } @@ -1297,7 +1251,6 @@ module.exports = { fileURLToPath, pathToFileURL, toPathIfFileURL, - isURLInstance, URL, URLSearchParams, domainToASCII, diff --git a/lib/internal/worker.js b/lib/internal/worker.js index 0304d7e4c7c6b8..d6a9ce3abcac88 100644 --- a/lib/internal/worker.js +++ b/lib/internal/worker.js @@ -55,7 +55,7 @@ const { WritableWorkerStdio, } = workerIo; const { deserializeError } = require('internal/error_serdes'); -const { fileURLToPath, isURLInstance, pathToFileURL } = require('internal/url'); +const { fileURLToPath, isURL, pathToFileURL } = require('internal/url'); const { kEmptyObject } = require('internal/util'); const { validateArray } = require('internal/validators'); @@ -148,13 +148,13 @@ class Worker extends EventEmitter { } url = null; doEval = 'classic'; - } else if (isURLInstance(filename) && filename.protocol === 'data:') { + } else if (isURL(filename) && filename.protocol === 'data:') { url = null; doEval = 'module'; filename = `import ${JSONStringify(`${filename}`)}`; } else { doEval = false; - if (isURLInstance(filename)) { + if (isURL(filename)) { url = filename; filename = fileURLToPath(filename); } else if (typeof filename !== 'string') { diff --git a/test/parallel/test-whatwg-url-custom-inspect.js b/test/parallel/test-whatwg-url-custom-inspect.js index a7d30a6ab936c3..d8c732c29eb127 100644 --- a/test/parallel/test-whatwg-url-custom-inspect.js +++ b/test/parallel/test-whatwg-url-custom-inspect.js @@ -61,7 +61,7 @@ assert.strictEqual( assert.strictEqual( util.inspect({ a: url }, { depth: 0 }), - '{ a: [URL] }'); + '{ a: URL {} }'); class MyURL extends URL {} assert(util.inspect(new MyURL(url.href)).startsWith('MyURL {')); diff --git a/test/parallel/test-whatwg-url-invalidthis.js b/test/parallel/test-whatwg-url-invalidthis.js index 790c28e37c13ed..b46b5d8cceb8fa 100644 --- a/test/parallel/test-whatwg-url-invalidthis.js +++ b/test/parallel/test-whatwg-url-invalidthis.js @@ -10,7 +10,8 @@ const assert = require('assert'); 'toJSON', ].forEach((i) => { assert.throws(() => Reflect.apply(URL.prototype[i], [], {}), { - code: 'ERR_INVALID_THIS', + name: 'TypeError', + message: /Cannot read private member/, }); }); @@ -27,11 +28,13 @@ const assert = require('assert'); 'hash', ].forEach((i) => { assert.throws(() => Reflect.get(URL.prototype, i, {}), { - code: 'ERR_INVALID_THIS', + name: 'TypeError', + message: /Cannot read private member/, }); assert.throws(() => Reflect.set(URL.prototype, i, null, {}), { - code: 'ERR_INVALID_THIS', + name: 'TypeError', + message: /Cannot read private member/, }); }); @@ -40,6 +43,7 @@ const assert = require('assert'); 'searchParams', ].forEach((i) => { assert.throws(() => Reflect.get(URL.prototype, i, {}), { - code: 'ERR_INVALID_THIS', + name: 'TypeError', + message: /Cannot read private member/, }); });