X Tutup
The Wayback Machine - https://web.archive.org/web/20250514221253/https://github.com/nodejs/node/issues/34347
Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
szmarczak opened this issue Jul 14, 2020 · 9 comments
Open

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

szmarczak opened this issue Jul 14, 2020 · 9 comments
Labels
http Issues or PRs related to the http subsystem.

Comments

@szmarczak
Copy link
Member

  • 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
Copy link
Member

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

@szmarczak
Copy link
Member Author

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
Copy link
Member

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.

@bnoordhuis bnoordhuis added the http Issues or PRs related to the http subsystem. label Jul 14, 2020
@szmarczak
Copy link
Member Author

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
Copy link
Member

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

@preyunk
Copy link
Contributor

preyunk commented Jul 16, 2020 •
edited
Loading

@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
Copy link
Member Author

@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
Copy link
Contributor

preyunk commented Jul 16, 2020 •
edited
Loading

@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
Copy link
Member Author

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

preyunk added a commit to preyunk/node that referenced this issue Jul 17, 2020
@preyunk
http: fixed invalid path with connect method
4f71f9f 
Fixes: nodejs#34347
@preyunk preyunk mentioned this issue Jul 17, 2020
Closed
2 tasks
@preyunk preyunk mentioned this issue Jul 17, 2020
Closed
3 tasks
preyunk added a commit to preyunk/node that referenced this issue Aug 5, 2020
@preyunk
http: fixed invalid path with connect method
edd8f6e 
Fixes: nodejs#34347
preyunk added a commit to preyunk/node that referenced this issue Aug 5, 2020
@preyunk
http: fixed invalid path with connect method
0825564 
Added tests

Fixes: nodejs#34347
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
3 participants
@bnoordhuis @preyunk @szmarczak
X Tutup