Skip to content

http: server check Host header in request, to meet RFC 7230 5.4 requirement#45597

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
Closed
marco-ippolito wants to merge 14 commits into from
Closed

http: server check Host header in request, to meet RFC 7230 5.4 requirement #45597

marco-ippolito wants to merge 14 commits into from

Conversation

@marco-ippolito
Copy link
Member

@marco-ippolitomarco-ippolito commented Nov 23, 2022

Since the author of the previous PR #39322 was absent for more than a year I've decided to take it, resolve conflicts and create a new PR.
Resolves: #39033

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 23, 2022

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-botnodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Nov 23, 2022
@ronagronag requested a review from mcollinaNovember 23, 2022 11:40
@ronagronag added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 23, 2022
ShogunPanda
ShogunPanda approved these changes Nov 23, 2022
View reviewed changes
Copy link
Contributor

@ShogunPandaShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 23, 2022

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

lpinca
lpinca reviewed Nov 23, 2022
View reviewed changes
mcollina
mcollina approved these changes Nov 23, 2022
View reviewed changes
Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

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

lgtm

mscdex
mscdex reviewed Nov 23, 2022
View reviewed changes
mscdex
mscdex reviewed Nov 23, 2022
View reviewed changes
bnoordhuis
bnoordhuis reviewed Nov 23, 2022
View reviewed changes
Copy link
Member

@bnoordhuisbnoordhuis left a comment

Choose a reason for hiding this comment

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

Is there a test that checks a 400 is sent back? Plenty of updated tests but none that seem to test the new behavior. :-)

@marco-ippolitomarco-ippolitoforce-pushed the feat/no-host-request branch 2 times, most recently from 2350f73 to 9f283e1CompareNovember 24, 2022 17:42
@bnoordhuis
Copy link
Member

bnoordhuis commented Nov 24, 2022

macos test failure appears to be relevant?

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 25, 2022

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 25, 2022

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

@marco-ippolitomarco-ippolito deleted the feat/no-host-request branch November 28, 2022 18:23
This was referenced Nov 29, 2022
Open
Open
Open
Open
Open
Open
Open
@github-actionsgithub-actionsbot mentioned this pull request Dec 7, 2022
Open
10 tasks
@climba03003climba03003 mentioned this pull request Jan 26, 2023
Merged
4 tasks
@RafaelGSSRafaelGSS mentioned this pull request Apr 3, 2023
Merged
@marco-ippolitomarco-ippolito mentioned this pull request Apr 27, 2023
Merged
@marco-ippolitomarco-ippolito mentioned this pull request May 15, 2024
Open
@climba03003climba03003 mentioned this pull request Oct 21, 2024
Closed
@marco-ippolitomarco-ippolito mentioned this pull request Nov 2, 2024
Closed
@hkleungaihkleungai mentioned this pull request Apr 20, 2025
Merged
7 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuisbnoordhuisbnoordhuis left review comments

@jasnelljasnelljasnell left review comments

@lpincalpincalpinca left review comments

@mcollinamcollinamcollina approved these changes

@ShogunPandaShogunPandaShogunPanda approved these changes

@ronagronagronag approved these changes

@anonriganonrigAwaiting requested review from anonrig

+1 more reviewer

@mscdexmscdexmscdex left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

httpIssues or PRs related to the http subsystem.needs-ciPRs that need a full CI run.semver-majorPRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

HTTP requests with no Host header accepted

12 participants

@marco-ippolito@nodejs-github-bot@bnoordhuis@ShogunPanda@aduh95@mcollina@mscdex@jasnell@lpinca@anonrig@ronag@wwwzbwcom