fix: Pass url string to http.request (#1268)

* fix: IPv6 literal parsing

* docs: Explain why search is overwritten

* test: Document the reason for square brackets

* docs: Mention basic auth support as a difference

* fix: Raise a TypeError when URL includes credentials

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);

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();
 
@@ -206,22 +210,20 @@ export const getNodeRequestOptions = request => {
 
 	const search = getSearch(parsedURL);
 
-	// Manually spread the URL object instead of spread syntax
-	const requestOptions = {
+	// 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,
-		pathname: parsedURL.pathname,
-		hostname: parsedURL.hostname,
-		protocol: parsedURL.protocol,
-		port: parsedURL.port,
-		hash: parsedURL.hash,
-		search: parsedURL.search,
-		query: parsedURL.query,
-		href: parsedURL.href,
+		// The following options are not expressed in the URL
 		method: request.method,
 		headers: headers[Symbol.for('nodejs.util.inspect.custom')](),
 		insecureHTTPParser: request.insecureHTTPParser,
 		agent
 	};
 
-	return requestOptions;
+	return {
+		parsedURL,
+		options
+	};
 };

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');
+		});
+	});
+});

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);

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,18 @@ 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('[')) {
+			// 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);
 		return once(this.server, 'listening');
 	}
 
@@ -31,10 +39,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`;