From d6010fb71088423ccb0a71c71acdfbcd3b322113 Mon Sep 17 00:00:00 2001 From: Aras Abbasi Date: Mon, 15 Apr 2024 02:39:04 +0200 Subject: [PATCH] core: improve parseURL (#3102) --- benchmarks/core/is-valid-port.mjs | 16 +++ lib/core/util.js | 54 +++++++-- test/node-test/client-errors.js | 2 +- test/util.js | 192 +++++++++++++++++++++++++++++- 4 files changed, 250 insertions(+), 14 deletions(-) create mode 100644 benchmarks/core/is-valid-port.mjs diff --git a/benchmarks/core/is-valid-port.mjs b/benchmarks/core/is-valid-port.mjs new file mode 100644 index 00000000000..0c94bc092bf --- /dev/null +++ b/benchmarks/core/is-valid-port.mjs @@ -0,0 +1,16 @@ +import { bench, group, run } from 'mitata' +import { isValidPort } from '../../lib/core/util.js' + +const string = '1234' +const number = 1234 + +group('isValidPort', () => { + bench('string', () => { + return isValidPort(string) + }) + bench('number', () => { + return isValidPort(number) + }) +}) + +await run() diff --git a/lib/core/util.js b/lib/core/util.js index e530732de25..2051f5fad2d 100644 --- a/lib/core/util.js +++ b/lib/core/util.js @@ -52,11 +52,37 @@ function buildURL (url, queryParams) { return url } +function isValidPort (port) { + const value = parseInt(port, 10) + return ( + value === Number(port) && + value >= 0 && + value <= 65535 + ) +} + +function isHttpOrHttpsPrefixed (value) { + return ( + value != null && + value[0] === 'h' && + value[1] === 't' && + value[2] === 't' && + value[3] === 'p' && + ( + value[4] === ':' || + ( + value[4] === 's' && + value[5] === ':' + ) + ) + ) +} + function parseURL (url) { if (typeof url === 'string') { url = new URL(url) - if (!/^https?:/.test(url.origin || url.protocol)) { + if (!isHttpOrHttpsPrefixed(url.origin || url.protocol)) { throw new InvalidArgumentError('Invalid URL protocol: the URL must start with `http:` or `https:`.') } @@ -67,12 +93,8 @@ function parseURL (url) { throw new InvalidArgumentError('Invalid URL: The URL argument must be a non-null object.') } - if (!/^https?:/.test(url.origin || url.protocol)) { - throw new InvalidArgumentError('Invalid URL protocol: the URL must start with `http:` or `https:`.') - } - if (!(url instanceof URL)) { - if (url.port != null && url.port !== '' && !Number.isFinite(parseInt(url.port))) { + if (url.port != null && url.port !== '' && isValidPort(url.port) === false) { throw new InvalidArgumentError('Invalid URL: port must be a valid integer or a string representation of an integer.') } @@ -92,28 +114,36 @@ function parseURL (url) { throw new InvalidArgumentError('Invalid URL origin: the origin must be a string or null/undefined.') } + if (!isHttpOrHttpsPrefixed(url.origin || url.protocol)) { + throw new InvalidArgumentError('Invalid URL protocol: the URL must start with `http:` or `https:`.') + } + const port = url.port != null ? url.port : (url.protocol === 'https:' ? 443 : 80) let origin = url.origin != null ? url.origin - : `${url.protocol}//${url.hostname}:${port}` + : `${url.protocol || ''}//${url.hostname || ''}:${port}` let path = url.path != null ? url.path : `${url.pathname || ''}${url.search || ''}` - if (origin.endsWith('/')) { - origin = origin.substring(0, origin.length - 1) + if (origin[origin.length - 1] === '/') { + origin = origin.slice(0, origin.length - 1) } - if (path && !path.startsWith('/')) { + if (path && path[0] !== '/') { path = `/${path}` } // new URL(path, origin) is unsafe when `path` contains an absolute URL // From https://developer.mozilla.org/en-US/docs/Web/API/URL/URL: // If first parameter is a relative URL, second param is required, and will be used as the base URL. // If first parameter is an absolute URL, a given second param will be ignored. - url = new URL(origin + path) + return new URL(`${origin}${path}`) + } + + if (!isHttpOrHttpsPrefixed(url.origin || url.protocol)) { + throw new InvalidArgumentError('Invalid URL protocol: the URL must start with `http:` or `https:`.') } return url @@ -600,6 +630,8 @@ module.exports = { isValidHeaderChar, isTokenCharCode, parseRangeHeader, + isValidPort, + isHttpOrHttpsPrefixed, nodeMajor, nodeMinor, nodeHasAutoSelectFamily: nodeMajor > 18 || (nodeMajor === 18 && nodeMinor >= 13), diff --git a/test/node-test/client-errors.js b/test/node-test/client-errors.js index 8826bb9dc87..3ac832b1635 100644 --- a/test/node-test/client-errors.js +++ b/test/node-test/client-errors.js @@ -386,7 +386,7 @@ test('invalid options throws', (t, done) => { assert.ok(0) } catch (err) { assert.ok(err instanceof errors.InvalidArgumentError) - assert.strictEqual(err.message, 'Invalid URL protocol: the URL must start with `http:` or `https:`.') + assert.strictEqual(err.message, 'Invalid URL hostname: the hostname must be a string or null/undefined.') } try { diff --git a/test/util.js b/test/util.js index b8b661bca40..b3e1193e506 100644 --- a/test/util.js +++ b/test/util.js @@ -1,9 +1,10 @@ 'use strict' -const { strictEqual } = require('node:assert') +const { strictEqual, throws, doesNotThrow } = require('node:assert') const { test, describe } = require('node:test') -const { isBlobLike } = require('../lib/core/util') +const { isBlobLike, parseURL, isHttpOrHttpsPrefixed, isValidPort } = require('../lib/core/util') const { Blob, File } = require('node:buffer') +const { InvalidArgumentError } = require('../lib/core/errors') describe('isBlobLike', () => { test('buffer', () => { @@ -65,3 +66,190 @@ describe('isBlobLike', () => { strictEqual(isBlobLike(null), false) }) }) + +describe('isHttpOrHttpsPrefixed', () => { + test('returns false for invalid values', () => { + strictEqual(isHttpOrHttpsPrefixed('wss:'), false) + }) + test('returns true for "http:" or "https:"', () => { + strictEqual(isHttpOrHttpsPrefixed('http:'), true) + strictEqual(isHttpOrHttpsPrefixed('https:'), true) + }) +}) + +describe('isValidPort', () => { + test('returns false for invalid values', () => { + strictEqual(isValidPort(NaN), false) + strictEqual(isValidPort(Infinity), false) + strictEqual(isValidPort(-Infinity), false) + strictEqual(isValidPort(NaN.toString()), false) + strictEqual(isValidPort(Infinity.toString()), false) + strictEqual(isValidPort(-Infinity.toString()), false) + strictEqual(isValidPort('port'), false) + strictEqual(isValidPort('65535i'), false) + }) + test('returns true for port in range of 0 to 65535 as number', () => { + for (let i = 0; i < 65536; i++) { + strictEqual(isValidPort(i), true) + } + }) + test('returns true for port in range of 0 to 65535 as string', () => { + for (let i = 0; i < 65536; i++) { + strictEqual(isValidPort(i.toString()), true) + } + }) +}) + +describe('parseURL', () => { + test('throws if url is not a string or object', () => { + throws(() => { parseURL(null) }, new InvalidArgumentError('Invalid URL: The URL argument must be a non-null object.')) + throws(() => { parseURL(1) }, new InvalidArgumentError('Invalid URL: The URL argument must be a non-null object.')) + throws(() => { parseURL(true) }, new InvalidArgumentError('Invalid URL: The URL argument must be a non-null object.')) + throws(() => { parseURL(false) }, new InvalidArgumentError('Invalid URL: The URL argument must be a non-null object.')) + throws(() => { parseURL(false) }, new InvalidArgumentError('Invalid URL: The URL argument must be a non-null object.')) + }) + test('throws if protocol is not beginning with http:', () => { + throws(() => { parseURL('ws://www.example.com') }, new InvalidArgumentError('Invalid URL protocol: the URL must start with `http:` or `https:`.')) + }) + test('throws if protocol is not beginning with https:', () => { + throws(() => { parseURL('wss://www.example.com') }, new InvalidArgumentError('Invalid URL protocol: the URL must start with `http:` or `https:`.')) + }) + test('returns an URL object if url is a string of an https URL', () => { + const url = parseURL('https://www.example.com') + strictEqual(url instanceof URL, true) + strictEqual(url.href, 'https://www.example.com/') + }) + test('returns an URL object if url is a string of an http URL', () => { + const url = parseURL('http://www.example.com') + strictEqual(url instanceof URL, true) + strictEqual(url.href, 'http://www.example.com/') + }) + + describe('when url is an instance of URL', () => { + test('returns the same URL object', () => { + const url = new URL('https://www.example.com') + const parsedURL = parseURL(url) + strictEqual(parsedURL, url) + }) + test('throws if the URL protocol is not http: or https:', () => { + const url = new URL('ws://www.example.com') + throws(() => { parseURL(url) }, new InvalidArgumentError('Invalid URL protocol: the URL must start with `http:` or `https:`.')) + }) + test('passes if the URL protocol is http:', () => { + const url = new URL('http://www.example.com') + delete url.origin + doesNotThrow(() => { parseURL(url) }) + }) + test('passes if the URL protocol is https:', () => { + const url = new URL('https://www.example.com') + delete url.origin + doesNotThrow(() => { parseURL(url) }) + }) + test('passes if the URL protocol is http:', () => { + const url = new URL('http://www.example.com') + doesNotThrow(() => { parseURL(url) }) + }) + test('passes if the URL protocol is https:', () => { + const url = new URL('https://www.example.com') + doesNotThrow(() => { parseURL(url) }) + }) + }) + + describe('when url is an common object', () => { + test.skip('does not throw if a urlLike object is passed', () => { + const url = parseURL({ protocol: 'http:' }) + console.log(url) + }) + + describe('port', () => { + test('throws if port is not an finite number as string', () => { + throws(() => parseURL({ protocol: 'http:', port: 'NaN' }), new InvalidArgumentError('Invalid URL: port must be a valid integer or a string representation of an integer.')) + }) + test('doesn\'t throw if port is valid number', () => { + for (let i = 0; i < 65536; i++) { + doesNotThrow(() => parseURL({ protocol: 'http:', hostname: 'www.example.com', port: i.toString() }), i.toString()) + } + }) + test('throws if port is invalid number', () => { + throws(() => parseURL({ protocol: 'http:', port: '-1' }), new InvalidArgumentError('Invalid URL: port must be a valid integer or a string representation of an integer.')) + throws(() => parseURL({ protocol: 'http:', port: '65536' }), new InvalidArgumentError('Invalid URL: port must be a valid integer or a string representation of an integer.')) + }) + test('sets port based on protocol', () => { + strictEqual(parseURL({ protocol: 'http:', hostname: 'www.example.com', path: '/' }).port, '') + strictEqual(parseURL({ protocol: 'https:', hostname: 'www.example.com', path: '/' }).port, '') + }) + test('don\'t override port with protocol if port was explicitly set', () => { + strictEqual(parseURL({ protocol: 'http:', hostname: 'www.example.com', path: '/', port: 1337 }).port, '1337') + strictEqual(parseURL({ protocol: 'https:', hostname: 'www.example.com', path: '/', port: 1337 }).port, '1337') + }) + }) + + describe('path', () => { + test('doesn\'t throw if path null or undefined', () => { + parseURL({ protocol: 'http:', hostname: 'www.example.com', path: null }) + parseURL({ protocol: 'http:', hostname: 'www.example.com', path: undefined }) + }) + test('throws if path is not as string', () => { + throws(() => parseURL({ protocol: 'http:', hostname: 'www.example.com', path: 1 }), new InvalidArgumentError('Invalid URL path: the path must be a string or null/undefined.')) + }) + test('doesn\'t throw if path is a string', () => { + parseURL({ protocol: 'http:', hostname: 'www.example.com', path: '/' }) + }) + test('accepts path with and without leading /', () => { + strictEqual(parseURL({ protocol: 'http:', hostname: 'www.example.com', path: 'abc' }).pathname, '/abc') + strictEqual(parseURL({ protocol: 'https:', hostname: 'www.example.com', path: '/abc' }).pathname, '/abc') + }) + }) + + describe('pathname', () => { + test('doesn\'t throw if pathname null or undefined', () => { + parseURL({ protocol: 'http:', hostname: 'www.example.com', pathname: null }) + parseURL({ protocol: 'http:', hostname: 'www.example.com', pathname: undefined }) + }) + test('throws if pathname is not as string', () => { + throws(() => parseURL({ protocol: 'http:', pathname: 1 }), new InvalidArgumentError('Invalid URL pathname: the pathname must be a string or null/undefined.')) + }) + test('doesn\'t throw if pathname is a string', () => { + parseURL({ protocol: 'http:', hostname: 'www.example.com', pathname: '/' }) + }) + }) + + describe('hostname', () => { + test('doesn\'t throw if hostname null or undefined', () => { + parseURL({ protocol: 'http:', hostname: null, origin: 'http://www.example.com' }) + parseURL({ protocol: 'http:', hostname: undefined, origin: 'http://www.example.com' }) + }) + test('throws if hostname is not as string', () => { + throws(() => parseURL({ protocol: 'http:', hostname: 1 }), new InvalidArgumentError('Invalid URL hostname: the hostname must be a string or null/undefined.')) + }) + test('doesn\'t throw if hostname is a string', () => { + parseURL({ protocol: 'http:', hostname: 'www.example.com' }) + }) + }) + + describe('origin', () => { + test('doesn\'t throw if origin null or undefined', () => { + parseURL({ protocol: 'http:', hostname: 'www.example.com', origin: null }) + parseURL({ protocol: 'http:', hostname: 'www.example.com', origin: undefined }) + }) + test('throws if origin is not as string', () => { + throws(() => parseURL({ protocol: 'http:', origin: 1 }), new InvalidArgumentError('Invalid URL origin: the origin must be a string or null/undefined.')) + }) + test('doesn\'t throw if origin is a string', () => { + parseURL({ protocol: 'http:', origin: 'https://www.example.com' }) + }) + test('removes trailing /', () => { + strictEqual(parseURL({ protocol: 'http:', origin: 'https://www.example.com/' }).origin, 'https://www.example.com') + }) + }) + + describe('protocol', () => { + test('throws if protocol is not http: or https: and no origin is defined', () => { + throws(() => parseURL({ protocol: 'wss:', hostname: 'www.example.com', path: '' })) + }) + test('doesn\'t throw when origin is not provided', () => { + strictEqual(parseURL({ protocol: 'http:', hostname: 'www.example.com', path: '' }).origin, 'http://www.example.com') + }) + }) + }) +})