http: normalize CONNECT request paths - #64876
Conversation
|
Review requested:
|
987fc94 to
8172f67
Compare
nodejs-github-bot
added
http
Codecov Reportβ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #64876 +/- ##
==========================================
- Coverage 90.29% 90.29% -0.01%
==========================================
Files 760 760
Lines 247061 247094 +33
Branches 46584 46601 +17
==========================================
+ Hits 223096 223120 +24
- Misses 15437 15454 +17
+ Partials 8528 8520 -8
π New features to boost your workflow:
|
Signed-off-by: Efe Karasakal <hi@efe.dev>
7cb68d6 to
673924c
Compare
| if (!isValidConnectPath(path)) { | ||
| throw new ERR_INVALID_ARG_VALUE( | ||
| 'options.path', | ||
| path, |
There was a problem hiding this comment.
Nit: because this isn't options.path, we print the normalized value, not what the user actually passed.
|
|
||
| this[kPath] = options.path || '/'; | ||
| let path = options.path || '/'; | ||
| if (method === 'CONNECT' && options.path != null) { |
There was a problem hiding this comment.
Nit: path defaults to /, but then this checks options.path.
That means if you set no path, we will send /, but if you set path: '/' then we throw. We should make those consistent.
| } | ||
|
|
||
| if (!isValidConnectPath(path)) { | ||
| throw new ERR_INVALID_ARG_VALUE( |
There was a problem hiding this comment.
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.
|
Sorry it's taken me so long to get to this. I've put some comments here, but I think generally I'm -0.5 on this. As is, I think it can be a breaking change, and the upside is quite small. More broadly, it changes the scope of HTTP validation we do in these APIs. Right now we don't do anything similar for other methods - we enforce syntactic correctness (no unescaped spaces, must be correctly framed & parseable) but we don't generally police anything else beyond that. If you want to send random strings as a cookie header, or send We could change that, it's an interesting idea, but if we're going to break this we might as well do something much larger. And to be honest I think it's not helpful: there's plenty of use cases for sending weird HTTP, notably including testing that your server correctly rejects it. This fits into a broader discussion about I think there's a central core we can do here safely and sensibly, roughly: if you specifically pass a URL as the request target and the method is CONNECT, then use the path without any leading slash as the target. No new errors or further validation. The risk of breakage there I think is much smaller (URL usage like this is quite unusual anyway, the correct behaviour was always ambiguous, and it'd only break for servers who exclusively accept the wrong format) and it solves the original issue. What do you think @efekrskl? |
4 participants
Fixes #34347
Partially a revival of #34412 which was apparently moving in the right direction but got stalled and closed