FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

url: forbid pipe in URL host by RaisinTen · Pull Request #37877 · nodejs/node · GitHub

/ node Public

url: forbid pipe in URL host - #37877

Merged
Trott merged 1 commit into
nodejs:masterfrom
RaisinTen:url/forbid-pipe-in-URL-host
Mar 30, 2021
ConversationCommits (1)ChecksFiles changed
Merged

url: forbid pipe in URL host#37877
Trott merged 1 commit into
nodejs:masterfrom
RaisinTen:url/forbid-pipe-in-URL-host

Conversation

RaisinTen commented Mar 23, 2021

Copy link
Copy Markdown
Member

Fixes: #37862

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 mentioned this pull request Mar 23, 2021
Closed
BridgeAR previously approved these changes Mar 23, 2021
View reviewed changes
BridgeAR dismissed their stale review March 23, 2021 23:57

The test failed

RaisinTen commented Mar 24, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

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 commented Mar 24, 2021

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Mar 25, 2021

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Mar 25, 2021

Copy link
Copy Markdown
Collaborator

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

RaisinTen commented Mar 26, 2021

Copy link
Copy Markdown
Member Author

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

RaisinTen requested review from BridgeAR and jasnell March 26, 2021 13:10
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 force-pushed the url/forbid-pipe-in-URL-host branch from d4b51c0 to 4197555 Compare March 30, 2021 11:05

Trott commented Mar 30, 2021

Copy link
Copy Markdown
Member

Landed in 4197555

Trott merged commit 4197555 into nodejs:master Mar 30, 2021
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
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 mentioned this pull request Apr 4, 2021
Merged

RaisinTen commented Aug 22, 2021

Copy link
Copy Markdown
Member 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 commented Aug 22, 2021

Copy link
Copy Markdown
Member

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 commented Aug 22, 2021

Copy link
Copy Markdown
Member 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 commented Aug 22, 2021

Copy link
Copy Markdown
Member

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 mentioned this pull request Aug 22, 2021
Closed
RaisinTen mentioned this pull request Aug 22, 2021
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott approved these changes
jasnell Awaiting requested review from jasnell
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

Back | FazBrowse Home | New Git URL