From f96f3b13e68f3851fd9fadb762c58f441a4c3f48 Mon Sep 17 00:00:00 2001 From: nlf Date: Thu, 24 Feb 2022 14:44:40 -0800 Subject: [PATCH] fix: use URL constructor instead of url.parse() (#33) BREAKING CHANGE: this removes the (hopefully) unused feature that arbitrary strings are allowed as URLs in the Request constructor. we now require that URLs are valid and absolute. --- lib/index.js | 12 +++------- lib/request.js | 34 ++++++++++++++------------ test/index.js | 29 +++++++++++++++++------ test/request.js | 63 +++++++++++++++++++++++++++++-------------------- 4 files changed, 81 insertions(+), 57 deletions(-) diff --git a/lib/index.js b/lib/index.js index 890f117..ca43d27 100644 --- a/lib/index.js +++ b/lib/index.js @@ -1,5 +1,5 @@ 'use strict' -const Url = require('url') +const { URL } = require('url') const http = require('http') const https = require('https') const zlib = require('minizlib') @@ -15,10 +15,6 @@ const { getNodeRequestOptions } = Request const FetchError = require('./fetch-error.js') const AbortError = require('./abort-error.js') -// disabling no-deprecated-api because we need to refactor to switch to the URL constructor -// eslint-disable-next-line node/no-deprecated-api -const resolveUrl = Url.resolve - const fetch = (url, opts) => { if (/^data:/.test(url)) { const request = new Request(url, opts) @@ -130,7 +126,7 @@ const fetch = (url, opts) => { // HTTP fetch step 5.3 const locationURL = location === null ? null - : resolveUrl(request.url, location) + : (new URL(location, request.url)).toString() // HTTP fetch step 5.5 if (request.redirect === 'error') { @@ -174,9 +170,7 @@ const fetch = (url, opts) => { } // Update host due to redirection - // disabling no-deprecated-api pending refactor to URL constructor - // eslint-disable-next-line node/no-deprecated-api - request.headers.set('host', Url.parse(locationURL).host) + request.headers.set('host', (new URL(locationURL)).host) // HTTP-redirect fetch step 6 (counter increment) // Create a new Request object. diff --git a/lib/request.js b/lib/request.js index 73944bd..4702f65 100644 --- a/lib/request.js +++ b/lib/request.js @@ -1,5 +1,5 @@ 'use strict' -const Url = require('url') +const { URL } = require('url') const Minipass = require('minipass') const Headers = require('./headers.js') const { exportNodeCompatibleHeaders } = Headers @@ -12,8 +12,6 @@ const defaultUserAgent = const INTERNALS = Symbol('Request internals') -const { format: formatUrl } = Url - const isRequest = input => typeof input === 'object' && typeof input[INTERNALS] === 'object' @@ -28,12 +26,9 @@ const isAbortSignal = signal => { class Request extends Body { constructor (input, init = {}) { - // we use url.parse() here because swapping for new URL() is not straight forward. - // until the necessary refactoring is done, we'll just silence the linter - // eslint-disable-next-line node/no-deprecated-api - const parsedURL = isRequest(input) ? Url.parse(input.url) - : input && input.href ? Url.parse(input.href) // eslint-disable-line node/no-deprecated-api - : Url.parse(`${input}`) // eslint-disable-line node/no-deprecated-api + const parsedURL = isRequest(input) ? new URL(input.url) + : input && input.href ? new URL(input.href) + : new URL(`${input}`) if (isRequest(input)) { init = { ...input[INTERNALS], ...init } @@ -137,7 +132,7 @@ class Request extends Body { } get url () { - return formatUrl(this[INTERNALS].parsedURL) + return this[INTERNALS].parsedURL.toString() } get headers () { @@ -170,10 +165,6 @@ class Request extends Body { } // Basic fetch - if (!parsedURL.protocol || !parsedURL.hostname) { - throw new TypeError('Only absolute URLs are supported') - } - if (!/^https?:$/.test(parsedURL.protocol)) { throw new TypeError('Only HTTP(S) protocols are supported') } @@ -239,8 +230,21 @@ class Request extends Body { // HTTP-network fetch step 4.2 // chunked encoding is handled by Node.js + // we cannot spread parsedURL directly, so we have to read each property one-by-one + // and map them to the equivalent https?.request() method options + const urlProps = { + auth: parsedURL.username || parsedURL.password + ? `${parsedURL.username}:${parsedURL.password}` + : '', + host: parsedURL.host, + hostname: parsedURL.hostname, + path: parsedURL.pathname, + port: parsedURL.port, + protocol: parsedURL.protocol, + } + return { - ...parsedURL, + ...urlProps, method: request.method, headers: exportNodeCompatibleHeaders(headers), agent, diff --git a/test/index.js b/test/index.js index 04fdaa5..ca4ba8d 100644 --- a/test/index.js +++ b/test/index.js @@ -24,7 +24,7 @@ const fs = require('fs') const http = require('http') // use of url.parse here is intentional and for coverage purposes // eslint-disable-next-line node/no-deprecated-api -const { parse: parseURL, URLSearchParams } = require('url') +const { parse: parseURL, URLSearchParams, URL: CoreURL } = require('url') const vm = require('vm') const { @@ -91,17 +91,21 @@ t.test('expose Headers, Response and Request constructors', t => { t.test('support proper toString output', { skip: !supportToString }, t => { t.equal(new Headers().toString(), '[object Headers]') t.equal(new Response().toString(), '[object Response]') - t.equal(new Request().toString(), '[object Request]') + t.equal(new Request('http://localhost:30000').toString(), '[object Request]') t.end() }) t.test('reject with error if url is protocol relative', t => - t.rejects(fetch('//example.com/'), new TypeError( - 'Only absolute URLs are supported'))) + t.rejects(fetch('//example.com/'), { + code: 'ERR_INVALID_URL', + name: 'TypeError', + })) t.test('reject if url is relative path', t => - t.rejects(fetch('/some/path'), new TypeError( - 'Only absolute URLs are supported'))) + t.rejects(fetch('/some/path'), { + code: 'ERR_INVALID_URL', + name: 'TypeError', + })) t.test('reject if protocol unsupported', t => t.rejects(fetch('ftp://example.com/'), new TypeError( @@ -1503,7 +1507,7 @@ t.test('fetch with Request instance', t => { }) }) -t.test('fetch with Node.js URL object', t => { +t.test('fetch with Node.js legacy URL object', t => { const url = `${base}hello` const urlObj = parseURL(url) const req = new Request(urlObj) @@ -1514,6 +1518,17 @@ t.test('fetch with Node.js URL object', t => { }) }) +t.test('fetch with Node.js URL object', t => { + const url = `${base}hello` + const urlObj = new CoreURL(url) + const req = new Request(urlObj) + return fetch(req).then(res => { + t.equal(res.url, url) + t.equal(res.ok, true) + t.equal(res.status, 200) + }) +}) + t.test('fetch with WHATWG URL object', t => { const url = `${base}hello` const urlObj = new URL(url) diff --git a/test/request.js b/test/request.js index bc79e72..371dcdb 100644 --- a/test/request.js +++ b/test/request.js @@ -84,7 +84,7 @@ t.test('should support wrapping Request instance', t => { t.test('should override signal on derived Request instances', t => { const parentAbortController = new AbortController() const derivedAbortController = new AbortController() - const parentRequest = new Request('test', { + const parentRequest = new Request('http://localhost/test', { signal: parentAbortController.signal, }) const derivedRequest = new Request(parentRequest, { @@ -97,7 +97,7 @@ t.test('should override signal on derived Request instances', t => { t.test('should allow removing signal on derived Request instances', t => { const parentAbortController = new AbortController() - const parentRequest = new Request(`test`, { + const parentRequest = new Request('http://localhost/test', { signal: parentAbortController.signal, }) const derivedRequest = new Request(parentRequest, { @@ -109,17 +109,17 @@ t.test('should allow removing signal on derived Request instances', t => { }) t.test('should throw error with GET/HEAD requests with body', t => { - t.throws(() => new Request('.', { body: '' }), TypeError) - t.throws(() => new Request('.', { body: 'a' }), TypeError) - t.throws(() => new Request('.', { body: '', method: 'HEAD' }), TypeError) - t.throws(() => new Request('.', { body: 'a', method: 'HEAD' }), TypeError) - t.throws(() => new Request('.', { body: 'a', method: 'get' }), TypeError) - t.throws(() => new Request('.', { body: 'a', method: 'head' }), TypeError) + t.throws(() => new Request('http://localhost', { body: '' }), TypeError) + t.throws(() => new Request('http://localhost', { body: 'a' }), TypeError) + t.throws(() => new Request('http://localhost', { body: '', method: 'HEAD' }), TypeError) + t.throws(() => new Request('http://localhost', { body: 'a', method: 'HEAD' }), TypeError) + t.throws(() => new Request('http://localhost', { body: 'a', method: 'get' }), TypeError) + t.throws(() => new Request('http://localhost', { body: 'a', method: 'head' }), TypeError) t.end() }) t.test('should default to null as body', t => { - const req = new Request('.') + const req = new Request(base) t.equal(req.body, null) return req.text().then(result => t.equal(result, '')) }) @@ -194,13 +194,6 @@ t.test('should support blob() method', t => { }) }) -t.test('should support arbitrary url', t => { - const url = 'anything' - const req = new Request(url) - t.equal(req.url, 'anything') - t.end() -}) - t.test('should support clone() method', t => { const url = base const r = new Minipass().end('a=1') @@ -241,7 +234,7 @@ t.test('should support clone() method', t => { }) t.test('should support ArrayBuffer as body', t => { - const req = new Request('', { + const req = new Request('http://localhost', { method: 'POST', body: stringToArrayBuffer('a=1'), }) @@ -249,7 +242,7 @@ t.test('should support ArrayBuffer as body', t => { }) t.test('should support Uint8Array as body', t => { - const req = new Request('', { + const req = new Request('http://localhost', { method: 'POST', body: new Uint8Array(stringToArrayBuffer('a=1')), }) @@ -257,7 +250,7 @@ t.test('should support Uint8Array as body', t => { }) t.test('should support DataView as body', t => { - const req = new Request('', { + const req = new Request('http://localhost', { method: 'POST', body: new DataView(stringToArrayBuffer('a=1')), }) @@ -302,6 +295,18 @@ t.test('get node request options', t => { agent: undefined, }, 'happy path') + t.match(Request.getNodeRequestOptions(new Request('http://user:password@a.b')), { + auth: 'user:password', + }, 'sets both user and password') + + t.match(Request.getNodeRequestOptions(new Request('http://user:@a.b')), { + auth: 'user:', + }, 'sets just user') + + t.match(Request.getNodeRequestOptions(new Request('http://:password@a.b')), { + auth: ':password', + }, 'sets just password') + t.match(Request.getNodeRequestOptions(new Request('http://a.b', { method: 'PATCH', headers: { @@ -349,7 +354,9 @@ t.test('get node request options', t => { body: 'xyz', compress: false, })), { - href: 'http://x.y', + path: '/', + protocol: 'http:', + hostname: 'x.y', method: 'PATCH', headers: { Accept: ['*/*'], @@ -368,7 +375,9 @@ t.test('get node request options', t => { body: new Minipass().end('xyz'), compress: false, })), { - href: 'http://x.y', + path: '/', + protocol: 'http:', + hostname: 'x.y', method: 'PATCH', headers: { Accept: ['*/*'], @@ -382,7 +391,9 @@ t.test('get node request options', t => { method: 'GET', family: 6, })), { - href: 'http://x.y', + path: '/', + protocol: 'http:', + hostname: 'x.y', method: 'GET', family: 6, }) @@ -396,7 +407,9 @@ t.test('get node request options', t => { Request.getNodeRequestOptions(new Request('http://a.b', { agent }), { method: 'GET', - href: 'http://a.b', + path: '/', + protocol: 'http:', + hostname: 'a.b', agent: 420, }) @@ -405,13 +418,11 @@ t.test('get node request options', t => { }) t.throws(() => Request.getNodeRequestOptions(new Request('ok.html')), { - message: 'Only absolute URLs are supported', - constructor: TypeError, + code: 'ERR_INVALID_URL', }) t.throws(() => Request.getNodeRequestOptions(new Request('xyz://ok.html')), { message: 'Only HTTP(S) protocols are supported', - constructor: TypeError, }) t.throws(() => Request.getNodeRequestOptions(new Request('http://a.b', {