Skip to content

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

Closed
marco-ippolito wants to merge 14 commits into
nodejs:mainfrom
marco-ippolito:feat/no-host-request
Closed

http: server check Host header in request, to meet RFC 7230 5.4 requirement#45597
marco-ippolito wants to merge 14 commits into
nodejs:mainfrom
marco-ippolito:feat/no-host-request

Conversation

@marco-ippolito
Copy link
Copy Markdown
Member

@marco-ippolito marco-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
Copy Markdown
Collaborator

nodejs-github-bot commented Nov 23, 2022

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-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
@ronag ronag requested a review from mcollina November 23, 2022 11:40
@ronag ronag 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
Copy Markdown
Contributor

@ShogunPanda ShogunPanda 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
Copy Markdown
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
Comment thread lib/_http_server.js Outdated
mcollina
mcollina approved these changes Nov 23, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@mcollina mcollina 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
Comment thread lib/_http_server.js Outdated
mscdex
mscdex reviewed Nov 23, 2022
View reviewed changes
Comment thread lib/_http_server.js
bnoordhuis
bnoordhuis reviewed Nov 23, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis 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. :-)

Comment thread test/parallel/test-http-max-headers-count.js Outdated
Comment thread test/parallel/test-https-max-headers-count.js Outdated
Comment thread lib/_http_server.js Outdated
@marco-ippolito marco-ippolito force-pushed the feat/no-host-request branch 2 times, most recently from 2350f73 to 9f283e1 Compare November 24, 2022 17:42
@bnoordhuis
Copy link
Copy Markdown
Member

bnoordhuis commented Nov 24, 2022

macos test failure appears to be relevant?

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Nov 25, 2022

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

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Nov 25, 2022

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