Skip to content
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

security implications of legacy url.parse() should be more clearly documented #31279

Closed
sam-github opened this issue Jan 9, 2020 · 1 comment
Labels
discuss Issues opened for discussions and feedbacks. doc Issues and PRs related to the documentations. security Issues and PRs related to security. url Issues and PRs related to the legacy built-in url module.

Comments

@sam-github
Copy link
Contributor

sam-github commented Jan 9, 2020

  • Version: all
  • Platform: all
  • Subsystem: url

url.parse() is "sloppy" with its parsing, so use of it can result in behaviour unexpected by some users that has security implications.

It is marked as deprecated at https://nodejs.org/api/url.html#url_url_parse_urlstring_parsequerystring_slashesdenotehost, but the docs don't specifically call out the security issues, so people won't necessarily know that security is a reason to avoid it.

It also doesn't list the specific (known) security issues, so that its not possible for users of the legacy url.parse() API to determine whether their usage is insecure.

These should be addressed through documentation.

Related

Vulnerability reports in process of disclosure, so link will be dead for a while longer.

@devsnek devsnek added security Issues and PRs related to security. url Issues and PRs related to the legacy built-in url module. discuss Issues opened for discussions and feedbacks. doc Issues and PRs related to the documentations. labels Jan 9, 2020
@sam-github
Copy link
Contributor Author

https://hackerone.com/reports/678487 has been disclosed.

It would be helpful if the documentation was updated to warn about issues such as that.

jasnell added a commit to jasnell/node that referenced this issue Jul 6, 2020
@jasnell jasnell closed this as completed in a95fb93 Jul 9, 2020
MylesBorins pushed a commit that referenced this issue Jul 14, 2020
Fixes: #31279

PR-URL: #34226
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
MylesBorins pushed a commit that referenced this issue Jul 16, 2020
Fixes: #31279

PR-URL: #34226
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
addaleax pushed a commit that referenced this issue Sep 22, 2020
Fixes: #31279

PR-URL: #34226
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
addaleax pushed a commit that referenced this issue Sep 22, 2020
Fixes: #31279

PR-URL: #34226
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks. doc Issues and PRs related to the documentations. security Issues and PRs related to security. url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants