-
Notifications
You must be signed in to change notification settings - Fork 30k
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.
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
http: relax writeEarlyHints validations #46464
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
Removes the requirement that every call to writeEarlyHints include a `link` header. While the `link` header is clearly the most common usage of `103 Early Hints`, I could find no requirement to include a `link` header as part of [RFC8297](https://www.rfc-editor.org/rfc/rfc8297.html). Additionally this removes the existing incorrect validation of the Link header format in favor of only validating that it is a valid header value. While the validation could be updated to better match [RFC8288 Section 3](https://www.rfc-editor.org/rfc/rfc8288.html#section-3), it appears it would be the only place in the node.js code base where we proactively validate header values beyond verifying they are valid at the HTTP protocol layer. Fixes: nodejs#46453
1e641e5
to
78dd37e
Compare
} | ||
|
||
if (linkHeaderValue.length === 0) { | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the need to remove regexp
but why don't we keep the primitive checks as it is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check skips sending 103 Early Hints
all together if no link header is present. Which would be confusing if you're trying to send early hints with some other header. Admittedly I'm not aware of any current need to send 103 Early Hints
without a link header, but by my reading the RFC doesn't mandate the link header in any way.
While the RFC mandates no formal specification for the early hints, I would keep the validation and therefore I'm towards accepting #46466 in favor of this. |
|
Updated regex for "Link" header validation to better match the specification in RFC 8288 section 3. Does not check for valid URI format but handles the rest of the header more permissively than before. Alternative to another outstanding PR that disables validation entirely. Fixes: nodejs#46453 Refs: https://www.rfc-editor.org/rfc/rfc8288.html#section-3 Refs: nodejs#46464
Updated regex for "Link" header validation to better match the specification in RFC 8288 section 3. Does not check for valid URI format but handles the rest of the header more permissively than before. Alternative to another outstanding PR that disables validation entirely. Fixes: #46453 Refs: https://www.rfc-editor.org/rfc/rfc8288.html#section-3 Refs: #46464 PR-URL: #46466 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Updated regex for "Link" header validation to better match the specification in RFC 8288 section 3. Does not check for valid URI format but handles the rest of the header more permissively than before. Alternative to another outstanding PR that disables validation entirely. Fixes: #46453 Refs: https://www.rfc-editor.org/rfc/rfc8288.html#section-3 Refs: #46464 PR-URL: #46466 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Updated regex for "Link" header validation to better match the specification in RFC 8288 section 3. Does not check for valid URI format but handles the rest of the header more permissively than before. Alternative to another outstanding PR that disables validation entirely. Fixes: #46453 Refs: https://www.rfc-editor.org/rfc/rfc8288.html#section-3 Refs: #46464 PR-URL: #46466 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Removes the requirement that every call to writeEarlyHints include a
link
header. While thelink
header is clearly the most common usage of103 Early Hints
, I could find no requirement to include alink
header as part of RFC8297.Additionally this removes the existing incorrect validation of the Link header format in favor of only validating that it is a valid header value. While the validation could be updated to better match RFC8288 Section 3, it appears it would be the only place in the node.js code base where we proactively validate header values beyond verifying they are valid at the HTTP protocol layer.
Fixes: #46453