-
-
Notifications
You must be signed in to change notification settings - Fork 36.7k
http: normalize CONNECT request paths #64876
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,7 +60,7 @@ const { | |
| const Agent = require('_http_agent'); | ||
| const { Buffer } = require('buffer'); | ||
| const { defaultTriggerAsyncIdScope } = require('internal/async_hooks'); | ||
| const { URL, urlToHttpOptions, isURL } = require('internal/url'); | ||
| const { URL, URLParse, urlToHttpOptions, isURL } = require('internal/url'); | ||
| const { | ||
| kOutHeaders, | ||
| kNeedDrain, | ||
|
|
@@ -121,6 +121,7 @@ let debug = require('internal/util/debuglog').debuglog('http', (fn) => { | |
| }); | ||
|
|
||
| const INVALID_PATH_REGEX = /[^\u0021-\u00ff]/; | ||
| const CONNECT_PATH_REGEX = /^(\[[^\]]+\]|[^:]+):(\d+)$/; | ||
| const kError = Symbol('kError'); | ||
| const kPath = Symbol('kPath'); | ||
| const kAuthority = Symbol('kAuthority'); | ||
|
|
@@ -193,6 +194,22 @@ function authoritiesMatch(canonicalHost, hostFromHeader) { | |
| return parsed.host === canonicalHost; | ||
| } | ||
|
|
||
| function isValidConnectPath(path) { | ||
| const match = CONNECT_PATH_REGEX.exec(path); | ||
| if (match === null || +match[2] === 0) { | ||
| return false; | ||
| } | ||
|
|
||
| const url = URLParse(`http://${path}`); | ||
| return url !== null && | ||
| url.hostname !== '' && | ||
| url.username === '' && | ||
| url.password === '' && | ||
| url.pathname === '/' && | ||
| url.search === '' && | ||
| url.hash === ''; | ||
| } | ||
|
|
||
| // https://datatracker.ietf.org/doc/html/rfc9112#section-3.2 | ||
| // When the request target is in absolute-form, ensure it is consistent with | ||
| // the request authority: same scheme, no userinfo, and an authority | ||
|
|
@@ -466,7 +483,23 @@ function ClientRequest(input, options, cb) { | |
|
|
||
| this.joinDuplicateHeaders = options.joinDuplicateHeaders; | ||
|
|
||
| this[kPath] = options.path || '/'; | ||
| let path = options.path || '/'; | ||
| if (method === 'CONNECT' && options.path != null) { | ||
| path = String(options.path); | ||
| if (path[0] === '/') { | ||
| path = path.slice(1) || '/'; | ||
| } | ||
|
|
||
| if (!isValidConnectPath(path)) { | ||
| throw new ERR_INVALID_ARG_VALUE( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This didn't used to throw, in fact it would have sent the path successfully. Servers could potentially accept it and use that (they shouldn't, but it's quite possible that weird ones do anyway) or people might have test suites that send invalid values to confirm they're rejected by their server implementation. With this change that working code would validate and throw instead. |
||
| 'options.path', | ||
| path, | ||
| 'must be a valid host:port combo', | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| this[kPath] = path; | ||
| if (cb) { | ||
| this.once('response', cb); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| 'use strict'; | ||
|
|
||
| const common = require('../common'); | ||
| const assert = require('assert'); | ||
| const http = require('http'); | ||
|
|
||
| for (const path of [ | ||
| '', | ||
| 'example.com', | ||
| 'example.com:0', | ||
| 'example.com:65536', | ||
| 'example.com:8080/example', | ||
| 'evil.com:666/good.org:777', | ||
| '/example.com', | ||
| ]) { | ||
| assert.throws(() => http.request({ | ||
| method: 'CONNECT', | ||
| path, | ||
| }), { | ||
| code: 'ERR_INVALID_ARG_VALUE', | ||
| name: 'TypeError', | ||
| message: /^The property 'options\.path' must be a valid host:port combo\./, | ||
| }); | ||
| } | ||
|
|
||
| { | ||
| const server = http.createServer(common.mustNotCall()); | ||
|
|
||
| server.on('connect', common.mustCall((req, socket) => { | ||
| assert.strictEqual(req.url, 'example.com:80'); | ||
| socket.end('HTTP/1.1 501 Not Implemented\r\n\r\n'); | ||
| })); | ||
|
|
||
| server.listen(0, common.mustCall(() => { | ||
| const port = server.address().port; | ||
| const req = http.request( | ||
| new URL(`http://localhost:${port}/example.com:80`), | ||
| { method: 'CONNECT' }, | ||
| ); | ||
|
|
||
| req.on('connect', common.mustCall((res, socket) => { | ||
| assert.strictEqual(res.statusCode, 501); | ||
| socket.destroy(); | ||
| server.close(); | ||
| })); | ||
|
|
||
| req.end(); | ||
| })); | ||
| } | ||
|
|
||
| { | ||
| const server = http.createServer(common.mustNotCall()); | ||
|
|
||
| server.on('connect', common.mustCall((req, socket) => { | ||
| assert.strictEqual(req.url, '[2001:db8::1]:111'); | ||
| socket.end('HTTP/1.1 501 Not Implemented\r\n\r\n'); | ||
| })); | ||
|
|
||
| server.listen(0, common.mustCall(() => { | ||
| const req = http.request({ | ||
| host: 'localhost', | ||
| port: server.address().port, | ||
| method: 'CONNECT', | ||
| path: '[2001:db8::1]:111', | ||
| }); | ||
|
|
||
| req.on('connect', common.mustCall((res, socket) => { | ||
| assert.strictEqual(res.statusCode, 501); | ||
| socket.destroy(); | ||
| server.close(); | ||
| })); | ||
|
|
||
| req.end(); | ||
| })); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
pathdefaults to/, but then this checksoptions.path.That means if you set no path, we will send
/, but if you setpath: '/'then we throw. We should make those consistent.