From d3cf8f654f201a2c3a02f79c52622ec882d7aebf Mon Sep 17 00:00:00 2001 From: Ambrose Chua Date: Fri, 3 Sep 2021 19:19:49 +0800 Subject: [PATCH 1/6] fix: IPv6 literal parsing --- src/index.js | 12 ++++++------ src/request.js | 16 ++++++---------- test/main.js | 30 ++++++++++++++++++++++++++++++ test/utils/server.js | 13 +++++++------ 4 files changed, 49 insertions(+), 22 deletions(-) diff --git a/src/index.js b/src/index.js index d906fffa8..2262b00d1 100644 --- a/src/index.js +++ b/src/index.js @@ -35,12 +35,12 @@ export default async function fetch(url, options_) { return new Promise((resolve, reject) => { // Build request object const request = new Request(url, options_); - const options = getNodeRequestOptions(request); - if (!supportedSchemas.has(options.protocol)) { - throw new TypeError(`node-fetch cannot load ${url}. URL scheme "${options.protocol.replace(/:$/, '')}" is not supported.`); + const { parsedURL, options } = getNodeRequestOptions(request); + if (!supportedSchemas.has(parsedURL.protocol)) { + throw new TypeError(`node-fetch cannot load ${url}. URL scheme "${parsedURL.protocol.replace(/:$/, '')}" is not supported.`); } - if (options.protocol === 'data:') { + if (parsedURL.protocol === 'data:') { const data = dataUriToBuffer(request.url); const response = new Response(data, {headers: {'Content-Type': data.typeFull}}); resolve(response); @@ -48,7 +48,7 @@ export default async function fetch(url, options_) { } // Wrap http.request into fetch - const send = (options.protocol === 'https:' ? https : http).request; + const send = (parsedURL.protocol === 'https:' ? https : http).request; const {signal} = request; let response = null; @@ -77,7 +77,7 @@ export default async function fetch(url, options_) { }; // Send request - const request_ = send(options); + const request_ = send(parsedURL, options); if (signal) { signal.addEventListener('abort', abortAndFinalize); diff --git a/src/request.js b/src/request.js index ab922536f..7171db2e3 100644 --- a/src/request.js +++ b/src/request.js @@ -204,24 +204,20 @@ export const getNodeRequestOptions = request => { // HTTP-network fetch step 4.2 // chunked encoding is handled by Node.js + // Overwrite search to force request() to retain trailing ? (issue #776) const search = getSearch(parsedURL); // Manually spread the URL object instead of spread syntax - const requestOptions = { + const options = { path: parsedURL.pathname + search, - pathname: parsedURL.pathname, - hostname: parsedURL.hostname, - protocol: parsedURL.protocol, - port: parsedURL.port, - hash: parsedURL.hash, - search: parsedURL.search, - query: parsedURL.query, - href: parsedURL.href, method: request.method, headers: headers[Symbol.for('nodejs.util.inspect.custom')](), insecureHTTPParser: request.insecureHTTPParser, agent }; - return requestOptions; + return { + parsedURL, + options + }; }; diff --git a/test/main.js b/test/main.js index 1e1f368c3..77d352ba4 100644 --- a/test/main.js +++ b/test/main.js @@ -2334,3 +2334,33 @@ describe('node-fetch', () => { expect(res.url).to.equal(`${base}m%C3%B6bius`); }); }); + +describe('node-fetch using IPv6', () => { + const local = new TestServer('[::1]'); + let base; + + before(async () => { + await local.start(); + base = `http://${local.hostname}:${local.port}/`; + }); + + after(async () => { + return local.stop(); + }); + + it('should resolve into response', () => { + const url = `${base}hello`; + expect(url).to.contain('[::1]'); + return fetch(url).then(res => { + expect(res).to.be.an.instanceof(Response); + expect(res.headers).to.be.an.instanceof(Headers); + expect(res.body).to.be.an.instanceof(stream.Transform); + expect(res.bodyUsed).to.be.false; + + expect(res.url).to.equal(url); + expect(res.ok).to.be.true; + expect(res.status).to.equal(200); + expect(res.statusText).to.equal('OK'); + }); + }); +}); diff --git a/test/utils/server.js b/test/utils/server.js index 9bfe1c5af..25eec43fa 100644 --- a/test/utils/server.js +++ b/test/utils/server.js @@ -4,7 +4,7 @@ import {once} from 'events'; import Busboy from 'busboy'; export default class TestServer { - constructor() { + constructor(hostname) { this.server = http.createServer(this.router); // Node 8 default keepalive timeout is 5000ms // make it shorter here as we want to close server quickly at the end of tests @@ -15,10 +15,15 @@ export default class TestServer { this.server.on('connection', socket => { socket.setTimeout(1500); }); + this.hostname = hostname || 'localhost'; } async start() { - this.server.listen(0, 'localhost'); + let host = this.hostname; + if (host.startsWith('[')) { + host = host.slice(1, -1); + } + this.server.listen(0, host); return once(this.server, 'listening'); } @@ -31,10 +36,6 @@ export default class TestServer { return this.server.address().port; } - get hostname() { - return 'localhost'; - } - mockResponse(responseHandler) { this.server.nextResponseHandler = responseHandler; return `http://${this.hostname}:${this.port}/mocked`; From f0ca640e30755fbcf2bcfe9dfb76e719577b0feb Mon Sep 17 00:00:00 2001 From: Ambrose Chua Date: Mon, 6 Sep 2021 23:33:54 +0800 Subject: [PATCH 2/6] docs: Explain why search is overwritten --- src/request.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/request.js b/src/request.js index 7171db2e3..6628ca980 100644 --- a/src/request.js +++ b/src/request.js @@ -204,12 +204,14 @@ export const getNodeRequestOptions = request => { // HTTP-network fetch step 4.2 // chunked encoding is handled by Node.js - // Overwrite search to force request() to retain trailing ? (issue #776) const search = getSearch(parsedURL); - // Manually spread the URL object instead of spread syntax + // Pass the full URL directly to request(), but overwrite the following + // options: const options = { + // Overwrite search to retain trailing ? (issue #776) path: parsedURL.pathname + search, + // The following options are not expressed in the URL method: request.method, headers: headers[Symbol.for('nodejs.util.inspect.custom')](), insecureHTTPParser: request.insecureHTTPParser, From a29bf842609356e27160756286aad681f688e436 Mon Sep 17 00:00:00 2001 From: Ambrose Chua Date: Mon, 6 Sep 2021 23:41:38 +0800 Subject: [PATCH 3/6] test: Document the reason for square brackets --- test/utils/server.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/utils/server.js b/test/utils/server.js index 25eec43fa..c2a66f417 100644 --- a/test/utils/server.js +++ b/test/utils/server.js @@ -21,6 +21,8 @@ export default class TestServer { async start() { let host = this.hostname; if (host.startsWith('[')) { + // If we're trying to listen on an IPv6 literal hostname, strip the + // square brackets before binding to the IPv6 address host = host.slice(1, -1); } this.server.listen(0, host); From e069fc2e07d37b5ceab44f9fe013d5526f1dd72d Mon Sep 17 00:00:00 2001 From: Ambrose Chua Date: Tue, 7 Sep 2021 08:50:43 +0800 Subject: [PATCH 4/6] style: Run xo --- src/index.js | 2 +- src/request.js | 2 +- test/utils/server.js | 1 + 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/index.js b/src/index.js index 2262b00d1..a87666512 100644 --- a/src/index.js +++ b/src/index.js @@ -35,7 +35,7 @@ export default async function fetch(url, options_) { return new Promise((resolve, reject) => { // Build request object const request = new Request(url, options_); - const { parsedURL, options } = getNodeRequestOptions(request); + const {parsedURL, options} = getNodeRequestOptions(request); if (!supportedSchemas.has(parsedURL.protocol)) { throw new TypeError(`node-fetch cannot load ${url}. URL scheme "${parsedURL.protocol.replace(/:$/, '')}" is not supported.`); } diff --git a/src/request.js b/src/request.js index 6628ca980..5fa69c8ba 100644 --- a/src/request.js +++ b/src/request.js @@ -206,7 +206,7 @@ export const getNodeRequestOptions = request => { const search = getSearch(parsedURL); - // Pass the full URL directly to request(), but overwrite the following + // Pass the full URL directly to request(), but overwrite the following // options: const options = { // Overwrite search to retain trailing ? (issue #776) diff --git a/test/utils/server.js b/test/utils/server.js index c2a66f417..329a480d7 100644 --- a/test/utils/server.js +++ b/test/utils/server.js @@ -25,6 +25,7 @@ export default class TestServer { // square brackets before binding to the IPv6 address host = host.slice(1, -1); } + this.server.listen(0, host); return once(this.server, 'listening'); } From c2da8d8f04a9c781dcd9ad99446dd0300b8ea4e4 Mon Sep 17 00:00:00 2001 From: Ambrose Chua Date: Tue, 7 Sep 2021 09:28:48 +0800 Subject: [PATCH 5/6] docs: Mention basic auth support as a difference --- docs/v3-LIMITS.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/v3-LIMITS.md b/docs/v3-LIMITS.md index a53202e64..01367fff7 100644 --- a/docs/v3-LIMITS.md +++ b/docs/v3-LIMITS.md @@ -7,6 +7,8 @@ Known differences - On the upside, there are no forbidden headers. +- Passing basic authentication credentials (`https://username:password@hostname`) in the URL is supported. + - `res.url` contains the final url when following redirects. - For convenience, `res.body` is a Node.js [Readable stream][readable-stream], so decoding can be handled independently. From 3c64f0adc9a9dc3ad3be7669f39abc3d21d23852 Mon Sep 17 00:00:00 2001 From: Ambrose Chua Date: Thu, 9 Sep 2021 11:16:54 +0800 Subject: [PATCH 6/6] fix: Raise a TypeError when URL includes credentials --- docs/v3-LIMITS.md | 2 -- src/request.js | 4 ++++ test/request.js | 7 +++++++ 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/docs/v3-LIMITS.md b/docs/v3-LIMITS.md index 01367fff7..a53202e64 100644 --- a/docs/v3-LIMITS.md +++ b/docs/v3-LIMITS.md @@ -7,8 +7,6 @@ Known differences - On the upside, there are no forbidden headers. -- Passing basic authentication credentials (`https://username:password@hostname`) in the URL is supported. - - `res.url` contains the final url when following redirects. - For convenience, `res.body` is a Node.js [Readable stream][readable-stream], so decoding can be handled independently. diff --git a/src/request.js b/src/request.js index 5fa69c8ba..e5856b2c7 100644 --- a/src/request.js +++ b/src/request.js @@ -49,6 +49,10 @@ export default class Request extends Body { input = {}; } + if (parsedURL.username !== '' || parsedURL.password !== '') { + throw new TypeError(`${parsedURL} is an url with embedded credentails.`); + } + let method = init.method || input.method || 'GET'; method = method.toUpperCase(); diff --git a/test/request.js b/test/request.js index 5f1fda0b7..9d14fd137 100644 --- a/test/request.js +++ b/test/request.js @@ -125,6 +125,13 @@ describe('Request', () => { .to.throw(TypeError); }); + it('should throw error when including credentials', () => { + expect(() => new Request('https://john:pass@github.com/')) + .to.throw(TypeError); + expect(() => new Request(new URL('https://john:pass@github.com/'))) + .to.throw(TypeError); + }); + it('should default to null as body', () => { const request = new Request(base); expect(request.body).to.equal(null);