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

WDC-457: Document fetch credential header handling on cross-origin redirects #3378

Merged
merged 7 commits into from Feb 28, 2022

Conversation

dhaynespls
Copy link
Contributor

While we are currently following the fetch spec, I believe it would be helpful to document this behavior.

@dhaynespls dhaynespls requested review from a team as code owners February 7, 2022 23:51
@dhaynespls
Copy link
Contributor Author

Context in node-fetch/node-fetch#1449

@@ -81,15 +81,15 @@ let request = new Request(input [, init])

- `headers` <Type>Headers</Type> <PropMeta>optional</PropMeta>

- A [`Headers` object](https://developer.mozilla.org/en-US/docs/Web/API/Headers).
- A [`Headers` object](https://developer.mozilla.org/en-US/docs/Web/API/Headers). <Aside type="warning"> Headers explicitly added to `fetch(url)` will always be sent, even in the case that a redirect is followed. Since Workers does not store nor implicitly add credential headers, explicitly defined `Cookie` and `Authorization` headers will be forwarded when a redirect occurs, even in a cross-origin context.</Aside>
Copy link
Member

Choose a reason for hiding this comment

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

Bikeshedding a bit, what do you think of:

Suggested change
- A [`Headers` object](https://developer.mozilla.org/en-US/docs/Web/API/Headers). <Aside type="warning"> Headers explicitly added to `fetch(url)` will always be sent, even in the case that a redirect is followed. Since Workers does not store nor implicitly add credential headers, explicitly defined `Cookie` and `Authorization` headers will be forwarded when a redirect occurs, even in a cross-origin context.</Aside>
- A [`Headers` object](https://developer.mozilla.org/en-US/docs/Web/API/Headers). Note that, compared to browsers, Cloudflare Workers imposes very few restrictions on what headers you are allowed to send. For example, a browser will not allow you to set the `Cookie` header, since the browser is responsible for handling cookies itself. Workers, however, has no special understanding of cookies, and treats the `Cookie` header like any other header. <Aside type="warning">In the event that the response is a redirect, and the redirect mode is set to `follow` (see below), then all headers will be forwarded to the redirect destination, even if the destination is a different hostname or domain. This includes sensitive headers like `Cookie`, `Authorization`, or any application-specific headers. If this is not the behavior you want, you should set redirect mode to `manual` and implement your own redirect policy. Note that redirect mode defaults to `manual` for requests that originated from the Worker's client, so this warning only applies to `fetch()`es made by a Worker that are not proxying the original request.</Aside>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that refactor is a good compromise from my original words and more directly addresses the intent of this change (clarification about how explicitly adding headers to our fetch() implementation works compared to browsers).

You do say "very few restrictions" -> I'm assuming this is referring to the IP headers?

image

it did look a bit busy like before so I moved it into a note component:

image

Copy link
Member

Choose a reason for hiding this comment

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

I wasn't referring to the IP headers, no. I was referring to the list of headers that browsers don't normally allow to be set using fetch(), like Cookie, User-Agent, Date, ...

I'm OK with either of those two screenshots.

@ejcx ejcx self-requested a review February 10, 2022 02:32
@ejcx
Copy link

ejcx commented Feb 10, 2022

LGTM, don't think I'm a codeowner though

Co-authored-by: Kate Tungusova <70746074+deadlypants1973@users.noreply.github.com>
@cloudflare-pages
Copy link

cloudflare-pages bot commented Feb 23, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 0d312d4
Status: ✅  Deploy successful!
Preview URL: https://2931bfcc.cloudflare-docs-7ou.pages.dev

View logs

@dhaynespls dhaynespls merged commit 523261a into production Feb 28, 2022
@dhaynespls dhaynespls deleted the dhaynes/WDC-457 branch February 28, 2022 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants