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

url: forbid pipe in URL host #37877

Merged
merged 1 commit into from
Mar 30, 2021
Merged

url: forbid pipe in URL host #37877

merged 1 commit into from
Mar 30, 2021

Conversation

RaisinTen
Copy link
Contributor

Fixes: #37862

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Mar 23, 2021
@RaisinTen RaisinTen mentioned this pull request Mar 23, 2021
Closed
BridgeAR
BridgeAR previously approved these changes Mar 23, 2021
View reviewed changes
@BridgeAR BridgeAR dismissed their stale review March 23, 2021 23:57

The test failed

@RaisinTen
Copy link
Contributor Author

RaisinTen commented Mar 24, 2021 •
edited
Loading

I should've expected a failure because | is not supposed to be an allowed code point in the host. Updated the test. PTAL.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36948/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36971/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36974/

@RaisinTen
Copy link
Contributor Author

All tests are passing now. Please could this have another review? :)
cc @nodejs/url

@RaisinTen @Trott
url: forbid pipe in URL host
4197555 
Fixes: nodejs#37862

PR-URL: nodejs#37877
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott Trott force-pushed the url/forbid-pipe-in-URL-host branch from d4b51c0 to 4197555 Compare March 30, 2021 11:05
@Trott
Copy link
Member

Trott commented Mar 30, 2021

Landed in 4197555

@Trott Trott merged commit 4197555 into nodejs:master Mar 30, 2021
@RaisinTen RaisinTen deleted the url/forbid-pipe-in-URL-host branch March 30, 2021 15:36
MylesBorins pushed a commit that referenced this pull request Apr 4, 2021
@RaisinTen @MylesBorins
url: forbid pipe in URL host
570fbce 
Fixes: #37862

PR-URL: #37877
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 4, 2021
Merged
@RaisinTen
Copy link
Contributor Author

@targos seeing that this PR is labelled backport-blocked-v14.x, I was wondering which PR blocked this. I actually wanted to backport #38742 to v14.x-staging to fix #39798 but it would require this PR to be backported first as the URL WPTs would fail otherwise.

@targos
Copy link
Member

targos commented Aug 22, 2021

I don't know exactly. Probably other PR(s) that updated the WPT.
Note that the main issue with URL backports in v14.x is that some previous changes were marked semver-major so I think it's not possible to just update the WPT anymore (because the tests that depend on these changes are going to fail).

@RaisinTen
Copy link
Contributor Author

@targos Makes sense. So should we close #39798 with a wontfix and mark #38742 as semver-major or just backport #38742 with only the changes inside src?

@targos
Copy link
Member

targos commented Aug 22, 2021

It's a bit late to mark it semver-major, but we can mark it dont-land-on-v14.x. It's probably safer and more consistent to do so, since other behavior changes didn't land on that branch.

@RaisinTen RaisinTen mentioned this pull request Aug 22, 2021
Closed
@RaisinTen RaisinTen mentioned this pull request Aug 22, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott approved these changes

@jasnell jasnell Awaiting requested review from jasnell

@BridgeAR BridgeAR Awaiting requested review from BridgeAR

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

URL: Forbid | (pipe) in URL host
6 participants
@RaisinTen @nodejs-github-bot @Trott @targos @jasnell @BridgeAR