Passing a URL instance with a CONNECT method results in an invalid path

[szmarczak]

• Version: >=v10.21.0
• Platform: Linux solus 5.6.18-156.current deps: update openssl to 1.0.1j #1 SMP PREEMPT Sun Jun 21 07:16:38 UTC 2020 x86_64 GNU/Linux
• Subsystem: http, https, url

What steps will reproduce the bug?

const http = require('http');

const server = http.createServer();

server.on('connect', (request, stream) => {
	console.log(request.url);
	stream.end('HTTP/1.1 501 Not Implemented\r\n\r\n');
});

server.listen(error => {
	if (error) {
		throw error;
	}

	const url = new URL(`http://localhost:${server.address().port}/example.com`);

	const request = http.request(url, {method: 'CONNECT'}).end();
	request.once('connect', response => {
		response.destroy();

		server.close();
	});
});

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior?

example.com

What do you see instead?

/example.com

Additional information

There is a workaround for this:

-const request = http.request(url, {method: 'CONNECT'}).end();
+const request = http.request({
+	hostname: url.hostname,
+	port: url.port,
+	path: 'example.com',
+	method: 'CONNECT'
+}).end();

/cc @yovanoc

[bnoordhuis]

/example.com is the expected path. Why would you expect example.com?

[szmarczak]

https://tools.ietf.org/html/rfc7231#page-30

A client sending a CONNECT request MUST send the authority form of
request-target (Section 5.3 of [RFC7230]); i.e., the request-target
consists of only the host name and port number of the tunnel
destination, separated by a colon. For example,

 CONNECT server.example.com:80 HTTP/1.1
 Host: server.example.com:80

[bnoordhuis]

Right, so what you're asking is that http.request() special-cases (and validates?) CONNECT requests? The reason I said /example.com is expected is because the URL#pathname property is used as-is.

[szmarczak]

what you're asking is that http.request() special-cases (and validates?) CONNECT requests?

The point is that it doesn't, but it should IMO :)

The reason I said /example.com is expected is because the URL#pathname property is used as-is.

Yup, I noticed that.

[bnoordhuis]

Shouldn't be a controversial change. Pull request welcome.

[preyunk]

@bnoordhuis
I think we could exclude '/' from pathname at the time of conversion of url to options or should we change url.pathname to give out the value without '/'.

[szmarczak]

@preyunk That doesn't seem right. urlToOptions just converts the URL instance to an object, so it's expected to have leading slash in the pathname. It's just that it shouldn't be emitted when sending a CONNECT request.

[preyunk]

@szmarczak
Ok I get it now.
How about we check if options.path property has a leading slash (to prevent cases where it is set explicitly by providing path property in options) for 'CONNECT' method, then slice it to do the needful.

[szmarczak]

That's what's this issue is about...