-
Notifications
You must be signed in to change notification settings - Fork 30k
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
url.parse would be better to not decode authority? #12168
Comments
If this should be addressed, beware 8.x rc deadline will be closing in 2 days. |
I'd +1 on this to put the patch into 8.0.0, and I think it would be nice if you could open a pull request either. cc @nodejs/url |
My fear is this change is too drastic to be included in 8.0.0 without a prolonged period of testing. In fact, I am not certain the URL parser behavior should be changed at all, for compatibility with implementations in Java, Python, and older versions of Node.js, and for the fact that the bug only manifests itself with FTP, and is not a deficiency per se of the URL parser. On the other hand, I support additional error conditions in userland FTP client modules, such as @mscdex's |
/cc @nodejs/security |
While I've never been fond of the behavior of Given that there is a functional alternative in core (the WHATWG parser), I would rather see efforts focused there than on changing the existing parser. But that's just me. I'm interested in hearing what @nodejs/ctc folks have to say :-) |
I feel the same way. The problem is with (some) users of the module, not the module itself. This probably can't be changed anyway because of backwards compatibility. You wouldn't be able to tell if |
I don't target v8.0 to fix this change.
Ya, I think so. but even though we would be better not to decode any url parts by default. In my humble opinion,
|
Item 2 (recommending WHATWG URL Parser and slowly deprecating |
Speaking of which, a guide on how to migrate from the legacy API to WHATWG URL API would help, but I am not sure if this should be in doc/api/url.md, or in the website repo? |
That requires people to opt in, though. If someone is security-savvy enough to do that, they probably are already careful to encodeURIComponent() their inputs. |
Should this remain open? |
I don't think so. |
Version:
N/A
Platform:
N/A
Subsystem:
I found this article.
According to the article, Java and Python has ftp protocol injection to decode CRLF in the url.
The url has the newline(CRLF) in authority part. Java and Python ftp server recognize following injected code.
And our url module has the same issue for CRLF.
I tried
WHATWG URL
, the new url parser does not decode the authority part.Question
Why our url.parse decode authority by default?
And should we fix this CRLF problem?
I tried to fix this problem to sanitize CRLF url. but this change breaks compatibility. so I would like to hear some opinions.
I tried some npm modules related to ftp, but I cannot find vulnerabilities using this problem.
related urls
The text was updated successfully, but these errors were encountered: