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.

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

feat: remove headers filtering #1469

Merged
merged 5 commits into from May 30, 2022
Merged

feat: remove headers filtering #1469

merged 5 commits into from May 30, 2022

Conversation

KhafraDev
Copy link
Member

@KhafraDev KhafraDev commented May 28, 2022

Fixes #1463
Fixes #1262

@ronag's checklist here should be completed:

  1. "A WinterCG issue" - Cookies and fetch() on servers wintercg/fetch#7
  2. "It's clear in the code that all related logic is outside of spec and a reference motivating it." - Areas of the code where the spec mentioned "filtered" or "safelisted" headers has been commented.
  3. "Clearly added to the README."
  4. "Investigate what exactly deno, node-fetch and cloudflare does so that we don't introduce another variant with slight differences." -

Clearly, following Deno is the best idea and closest to the spec itself. We aren't adding anything in, but simply removing constraints that don't apply to a server environment.

@codecov-commenter
Copy link

codecov-commenter commented May 28, 2022

Codecov Report

Merging #1469 (f460eae) into main (a7669df) will increase coverage by 0.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1469      +/-   ##
==========================================
+ Coverage   94.55%   94.70%   +0.14%     
==========================================
  Files          49       49              
  Lines        4276     4246      -30     
==========================================
- Hits         4043     4021      -22     
+ Misses        233      225       -8     
Impacted Files Coverage Δ
lib/fetch/constants.js 100.00% <ø> (ø)
lib/fetch/response.js 93.33% <ø> (+4.37%) ⬆️
lib/fetch/headers.js 94.69% <100.00%> (-0.48%) ⬇️
lib/fetch/request.js 85.89% <100.00%> (-0.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7669df...f460eae. Read the comment docs.

README.md Show resolved Hide resolved
lib/fetch/request.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/fetch/request.js Outdated Show resolved Hide resolved
Copy link
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

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

I'm still concerned about the 2 places where this PR diverges from the spec. However, I'm going on vacation tomorrow and won't have time to dig into it myself. Merge it if you wish but I'd be happy if you could resolve this before merging.

lib/fetch/request.js Outdated Show resolved Hide resolved
lib/fetch/request.js Outdated Show resolved Hide resolved
lib/fetch/request.js Show resolved Hide resolved
@KhafraDev
Copy link
Member Author

KhafraDev commented May 30, 2022

@ronag Enjoy your vacation 😄, you were completely right - something was wrong with how the guard checks were being done now that the forbidden headers were no longer being checked as well (append and set were returning early so the headers weren't getting set). Only reason my changes worked was because new Headers(...) sets the guard to none by default, which doesn't have special behavior attached to it.

Everything follows the spec entirely now, and issues in the future should be easier now that I understand entirely what's happening!


Going to assume there was a pretty big performance improvement as well from removing the Proxy along with the forbidden header checks!

@KhafraDev KhafraDev merged commit bf6d5a1 into nodejs:main May 30, 2022
@KhafraDev KhafraDev deleted the cookies branch May 30, 2022 01:44
@Rich-Harris Rich-Harris mentioned this pull request May 30, 2022
9 tasks
KhafraDev added a commit to KhafraDev/undici that referenced this pull request Jun 23, 2022
* feat: remove headers filtering

* add wintercg issue to README

* update README

* fix(Headers): properly use/set "this's headers"

* fix: remove broken guard checks
@dsine-de
Copy link

dsine-de commented Sep 7, 2022

Should this be available already in a current Node.js version? I don't see a cookie header being set on a Node.js fetch request when following a redirect with a set-cookie response header in Node.js v.18.8.0, like in the browser or e.g. in the Postman client.

@KhafraDev
Copy link
Member Author

Yes, this is bundled in the current node version. If you make an issue or have a minimal reproduction I'd be glad to look into it.

@dsine-de
Copy link

dsine-de commented Sep 7, 2022

Ok thank you, I don't know what's the decision if this should work with Node.js native fetch, but it seems Postman and the browser fetch have something like a cookie jar and if they get a set-cookie header in a request which redirects, they save that cookie and add it as a cookie header when following the redirect.

If this should work the same with Node.js fetch, I'll make a minimal repro example & issue.

metcoder95 pushed a commit to metcoder95/undici that referenced this pull request Dec 26, 2022
* feat: remove headers filtering

* add wintercg issue to README

* update README

* fix(Headers): properly use/set "this's headers"

* fix: remove broken guard checks
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
* feat: remove headers filtering

* add wintercg issue to README

* update README

* fix(Headers): properly use/set "this's headers"

* fix: remove broken guard checks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants