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

Add support for HTTP2 pseudo headers #55

Merged
merged 3 commits into from
Dec 5, 2023
Merged

Add support for HTTP2 pseudo headers #55

merged 3 commits into from
Dec 5, 2023

Conversation

pcattori
Copy link

@pcattori pcattori commented Dec 5, 2023

No description provided.

@pcattori
fix(headers): do not depend on http for header name validation
68fb00e 
Node internally uses undici instead anyway
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Dec 5, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: a423019

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@remix-run/web-fetch Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@pcattori pcattori requested a review from mjackson December 5, 2023 21:00
@pcattori
fix(headers): support HTTP2 pseudo-headers
6e0c9d5 
which begin with `:` e.g. `:authority`, `:method`, etc.
@pcattori pcattori force-pushed the pedro/pseudo-headers branch from ee6c863 to 6e0c9d5 Compare December 5, 2023 21:01
@brophdawg11
Copy link

Want to to add a unit test alongside the should reject illegal header test (packages/fetch/test/headers.js`) file to assert these are ok?

brophdawg11
brophdawg11 reviewed Dec 5, 2023
View reviewed changes
packages/fetch/src/headers.js

const validateHeaderName = typeof validators.validateHeaderName === 'function' ?
validators.validateHeaderName :
Copy link

@brophdawg11 brophdawg11 Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Do we know exactly what this used to do? Any chance we start inadvertently rejecting headers in the wild that use to be allowed by this?

Copy link
Author

@pcattori pcattori Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The http-based validateHeaderName (which is what validators.validateHeaderName is) looks the same as ours. The only difference now is the :

https://github.com/nodejs/node/blob/main/lib/_http_outgoing.js#L664-L668
https://github.com/nodejs/node/blob/main/lib/_http_common.js#L206-L214

@pcattori
Copy link
Author

pcattori commented Dec 5, 2023

@brophdawg11 good callout, just added a test. Fails before this change, passes now.

@pcattori pcattori merged commit 3986fb3 into main Dec 5, 2023
65 checks passed
@pcattori pcattori deleted the pedro/pseudo-headers branch December 5, 2023 22:58
@brophdawg11 brophdawg11 mentioned this pull request Dec 6, 2023
Merged
This was referenced Dec 14, 2023
Closed
Closed
@hi-ogawa hi-ogawa mentioned this pull request Jan 11, 2024
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brophdawg11 brophdawg11 brophdawg11 left review comments

@mjackson mjackson mjackson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pcattori @brophdawg11 @mjackson