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.

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

doc: document security issues with url.parse() #34226

Closed
wants to merge 3 commits into from
Closed

doc: document security issues with url.parse() #34226

wants to merge 3 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jul 6, 2020

Fixes: #31279

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. url Issues and PRs related to the legacy built-in url module. labels Jul 6, 2020
bnoordhuis
bnoordhuis approved these changes Jul 7, 2020
View reviewed changes
doc/api/url.md Outdated Show resolved Hide resolved
@jasnell @bnoordhuis
[Squash] suggestion
b7eaab8 
Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl>
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 7, 2020
Trott
Trott reviewed Jul 9, 2020
View reviewed changes
doc/api/url.md Outdated Show resolved Hide resolved
Trott
Trott reviewed Jul 9, 2020
View reviewed changes
doc/api/url.md Outdated Show resolved Hide resolved
Trott
Trott reviewed Jul 9, 2020
View reviewed changes
doc/api/url.md Outdated Show resolved Hide resolved
Trott
Trott reviewed Jul 9, 2020
View reviewed changes
doc/api/url.md Outdated Show resolved Hide resolved
Trott
Trott reviewed Jul 9, 2020
View reviewed changes
doc/api/url.md Outdated Show resolved Hide resolved
Trott
Trott previously requested changes Jul 9, 2020
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left a few suggestions and I'd consider two of them mandatory:

  • Use API rather than api
  • Use host name rather than hostname for consistency with our other docs, UNIX man pages, and DNS specifications

Feel free to dismiss this once those things are updated. No need to wait for me to come back to the GitHub interface and do it.

@jasnell @Trott
[squash] nits
81a9ae6 
Co-authored-by: Rich Trott <rtrott@gmail.com>
jasnell added a commit that referenced this pull request Jul 9, 2020
@jasnell
doc: document security issues with url.parse()
a95fb93 
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>
@jasnell
Copy link
Member Author

jasnell commented Jul 9, 2020

Landed in a95fb93

@jasnell jasnell closed this Jul 9, 2020
MylesBorins pushed a commit that referenced this pull request Jul 14, 2020
@jasnell @MylesBorins
doc: document security issues with url.parse()
b403d60 
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 MylesBorins mentioned this pull request Jul 14, 2020
Merged
MylesBorins pushed a commit that referenced this pull request Jul 16, 2020
@jasnell @MylesBorins
doc: document security issues with url.parse()
a6a656a 
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 pull request Sep 22, 2020
@jasnell @addaleax
doc: document security issues with url.parse()
fc94e6f 
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 pull request Sep 22, 2020
@jasnell @addaleax
doc: document security issues with url.parse()
0ebb30b 
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>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@addaleax addaleax addaleax approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@Trott Trott Trott left review comments

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

security implications of legacy url.parse() should be more clearly documented
6 participants
@jasnell @bnoordhuis @Trott @addaleax @trivikr @nodejs-github-bot