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

Make fetch() use "same-origin" credentials by default #585

Merged
merged 1 commit into from Apr 20, 2018

Conversation

@annevk
Member

annevk commented Aug 25, 2017

Most features in the platform have this default and not having this default causes multiple connections in contemporary implementations. It also causes confusion as switching from XMLHttpRequest to fetch() is not as seamless as it could be.

Making this change will also make it easier for <script type=module> to have this default as per discussion in whatwg/html#2557.


Preview | Diff

Make fetch() use "same-origin" credentials by default
Most features in the platform have this default and not having this default causes multiple connections in contemporary implementations. It also causes confusion as switching from XMLHttpRequest to fetch() is not as seamless as it could be.

Making this change will also make it easier for <script type=module> to have this default as per discussion in whatwg/html#2557.
@annevk

This comment has been minimized.

@Munter

This comment has been minimized.

Munter commented Aug 25, 2017

Even though I have followed the fetch spec development and implementation and have read multiple blog posts on how to use it, I still got tripped up by forgetting to manually add credentials on certain same-origin fetch-calls. +1 from me on this change

@tyoshino

This comment has been minimized.

Member

tyoshino commented Aug 25, 2017

I agree that this is a more reasonable default, but also much concerned with possible break of existing apps, though your guess at whatwg/html#2557 (comment) sounds correct.

Let me cc interop leads: @RByers @foolip

@jakearchibald

This comment has been minimized.

Collaborator

jakearchibald commented Aug 25, 2017

I've reached out to devs on Twitter. Response is almost entirely positive.

One exception: https://twitter.com/rsjanabasis/status/901012433615020033, and I believe they're going to give us details.

@tyoshino

This comment has been minimized.

Member

tyoshino commented Aug 25, 2017

Thanks Jake for checking. It's good that it's supported. And, yeah, that looks really one example of the concern.

@foolip

This comment has been minimized.

Member

foolip commented Aug 25, 2017

IIUC, changing the default from "omit" to "same-origin" can only mean that credentials are sometimes sent when they otherwise would not be. For resources where it makes a difference at all, it seems rather unusual that the no-credentials response is the one that you want, but that the with-credentials response is some broken/unexpected resource. Certainly possible to create, and certain to appear in the wild given enough time, but are there cases we should worry about?

Overall, the compat risk here seems very low.

@wanderview

This comment has been minimized.

Member

wanderview commented Aug 25, 2017

If we are going to do this it would be nice to try to coordinate shipping the change around the same time.

@annevk

This comment has been minimized.

Member

annevk commented Aug 28, 2017

Okay, I'll file four implementation bugs. My thinking is that we can update the tests as part of changing the implementation. Easiest to discover all the tests that need changing that way. Is that okay or should I try and prepare test updates?

@foolip

This comment has been minimized.

Member

foolip commented Aug 28, 2017

That seems like a good idea where testing without an implementation is error-prone, just leave an open "missing tests" issue and point to it from the impl bugs.

@annevk

This comment has been minimized.

Member

annevk commented Aug 28, 2017

I was thinking I'd just leave this open until we have a patch or two and no objections.

@foolip

This comment has been minimized.

Member

foolip commented Aug 28, 2017

Yeah, that works too. If we have a buildup of "awaiting implementation" issues I guess labels can alleviate that.

@tyoshino

This comment has been minimized.

Member

tyoshino commented Aug 29, 2017

Thanks, Philip. I'd like to wait and hear the details of what Foggy said in the tweet quoted in #585 (comment). But except for that, I share your analysis basically.

@rsjanabasis

This comment has been minimized.

rsjanabasis commented Aug 30, 2017

I want to make sure that we can block sending credentials by appropriately filtering the request.

For example, if we shadow "fetch" with safe_fetch:
function safe_fetch(url, init) { return fetch(arguments[0], { credentials: 'omit' } ); }
then this will always block sending credentials in every browser, correct?

@tyoshino

This comment has been minimized.

Member

tyoshino commented Aug 30, 2017

function safe_fetch(url, init) { return fetch(arguments[0], { credentials: 'omit' } ); }
then this will always block sending credentials in every browser, correct?

Right. The topic is about changing the default which is used when the credentials parameter is not specified. You can still explicitly specify the credentials parameter with the value omit as you did to prevent credentials from being sent.

(Your example doesn't inherit values from init. Maybe you want to override credentials but inherit the other values from init.)

@annevk

This comment has been minimized.

Member

annevk commented Aug 30, 2017

I'd write safe_fetch as:

function safe_fetch(input, init) {
  const tempRequest = new Request(input, init);
  return fetch(tempRequest, { credentials: "omit" });
}
@annevk

This comment has been minimized.

Member

annevk commented Sep 1, 2017

Is there anything remaining that would block this from being implemented? I'd like this to be done so we can align JavaScript modules with this default and avoid a ton of confusion there.

@rsjanabasis

This comment has been minimized.

rsjanabasis commented Sep 2, 2017

@tyoshino

This comment has been minimized.

Member

tyoshino commented Sep 13, 2017

Thanks Foggy. So, I'm fine with the plan now.

@annevk

This comment has been minimized.

Member

annevk commented Sep 13, 2017

Okay, I'll merge this as soon as one of the bugs above gets marked fixed and web-platform-tests is updated.

CendioOssman added a commit to novnc/noVNC that referenced this pull request Oct 9, 2017

Get proper same-origin behaviour when loading modules
The browsers currently do not default to same-origin behaviour for
modules, so we need to be explicit in order for necessary
credentials to be passed along. This seems to be changing though,
but we need to wait for the browsers to actually roll out more
lenient defaults:

whatwg/fetch#585
@domfarolino

This comment has been minimized.

Member

domfarolino commented Mar 6, 2018

It seems like WPTs are one of the things holding this PR and Mozilla's implementation back (given https://bugs.chromium.org/p/chromium/issues/detail?id=759543#c7). I'm happy to work on this if nobody else has started.

@annevk

This comment has been minimized.

Member

annevk commented Apr 5, 2018

@domfarolino created web-platform-tests changes in web-platform-tests/wpt#9911 and is planning to go ahead in Chrome. (Unfortunately there isn't much test coverage for this today.)

I wonder if we should simultaneously change module workers since that's the only other feature that exposes this default at the moment. @domfarolino what do you think?

@annevk

This comment has been minimized.

Member

annevk commented Apr 5, 2018

(And I guess that aspect of the feature isn't tested then? @domenic?)

@wanderview

This comment has been minimized.

Member

wanderview commented Apr 20, 2018

I wonder if we should simultaneously change module workers since that's the only other feature that exposes this default at the moment. @domfarolino what do you think?

Does anyone implement module workers yet? Its not something shipped in firefox, so this change does not effect us there.

@wanderview

This comment has been minimized.

Member

wanderview commented Apr 20, 2018

@domfarolino I'm looking to possibly ship this in FF61 around June 28. Can chrome ship in a similar time frame?

@annevk

This comment has been minimized.

Member

annevk commented Apr 20, 2018

Yeah, I guess they are not implemented yet (and have some problems due to allowing CORS). We can work on them separately.

annevk added a commit to web-platform-tests/wpt that referenced this pull request Apr 20, 2018

@annevk annevk merged commit 811575d into master Apr 20, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@annevk annevk deleted the annevk/fetch-api-credentials-mode branch Apr 20, 2018

@domenic

This comment has been minimized.

Member

domenic commented Apr 20, 2018

Normal script type=module also exposes this default, no?

@annevk

This comment has been minimized.

Member

annevk commented Apr 21, 2018

Yeah, I realized that later. (Note that it's not tested if that actually works.) I started working on a fix in whatwg/html#2557.

aarongable pushed a commit to chromium/chromium that referenced this pull request Apr 30, 2018

Set fetch()'s default credentials mode to same-origin
As per whatwg/fetch#585, the
fetch() API's default credentials mode should be set to
same-origin in the Request constructor.

I2s: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/WOAtshyL2As

R=yhirano@chromium.org, yoav@yoav.ws

Bug: 759543
Change-Id: Id5cb8c747c41385edcc13775fdd18e1f27e7c3c9
Reviewed-on: https://chromium-review.googlesource.com/981512
Commit-Queue: Yoav Weiss <yoav@yoav.ws>
Reviewed-by: Yoav Weiss <yoav@yoav.ws>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554720}
@getify

This comment has been minimized.

getify commented Jul 9, 2018

Just FYI: just ran into a bug with my cookies-setting (not working) via response from fetch(..). It was working fine in FF62, but not in latest Chrome or recent Safari. I wrongly assumed that the bug was FF not matching spec. Finally figured out that FF was correct, and Chrome/Safari haven't updated on this yet. :(

@domfarolino

This comment has been minimized.

Member

domfarolino commented Jul 9, 2018

Yeah, this change hasn't landed in latest Chrome stable yet. What version are you using?

@getify

This comment has been minimized.

getify commented Jul 9, 2018

Chrome: 67.0.3396.99
Firefox: 63.0a1 (2018-07-08)

@domfarolino

This comment has been minimized.

Member

domfarolino commented Jul 9, 2018

Seems like Chrome stable and FF nightly or so? This change is definitely in Canary, unsure off the top of my head how far away from stable though.

@yoavweiss

This comment has been minimized.

Collaborator

yoavweiss commented Jul 9, 2018

@domfarolino's Chromium change landed on April 30th, so should be included in Chrome 68, estimated to hit stable towards the end of July 2018.

@getify if you wish to verify, you can try it out in beta right now.

@getify

This comment has been minimized.

getify commented Jul 9, 2018

Seems like Chrome stable and FF nightly or so?

Yes, I happened to be checking in FF nightly when I responded earlier. But AFAICT it's already in FF stable -- v61 from a few weeks ago.

So apparently I just happened to get caught by this in the gap (about a month, it seems?) between FF and Chrome going stable with it.

Anyone know when Safari will be updating this (stable)?

@yoavweiss

This comment has been minimized.

Collaborator

yoavweiss commented Jul 9, 2018

Maybe @cdumez or @youennf would.

@cdumez

This comment has been minimized.

cdumez commented Jul 9, 2018

Definitely something for @youennf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment