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

Script modules and credentials #2557

Closed
jakearchibald opened this issue Apr 19, 2017 · 15 comments · Fixed by #3656
Closed

Script modules and credentials #2557

jakearchibald opened this issue Apr 19, 2017 · 15 comments · Fixed by #3656

Comments

@jakearchibald
Copy link
Contributor

jakearchibald commented Apr 19, 2017

By default <script type="module"> will omit credentials for all module fetches, much like the default for fetch().

However…

<!-- No credentials -->
<script type="module" src="foo.js"></script>
<!-- With credentials -->
<script type="module" src="foo.js" crossorigin></script>

It feels really weird that crossorigin implies adding credentials for same-origin requests.

@domenic
Copy link
Member

domenic commented Apr 19, 2017

This may be clearer if you stop relying on the invalid value default and type crossorigin="anonymous".

We've talked about introducing a new attribute, credentials=omit|same-origin|include (with invalid/missing value default of omit), but I'm not sure it pulls its own weight.

@annevk
Copy link
Member

annevk commented Apr 19, 2017

My tentative suggestion is that we try to flip the default of fetch() and this to "same-origin" and keep "omit" only in fetch() for the extra-paranoid. That simplifies our default setups a bit while still allowing for all the things (albeit some only through service workers). My hope is that the compatibility impact of this will be low, since adding credentials is less problematic than removing them, and that the ensuing developer experience will be much better, since they have one less default to learn.

@domenic
Copy link
Member

domenic commented Apr 19, 2017

One concrete downside there is that it means the many requests per day made with fetch() and modules will now have the extra cookie bytes sent along, which can be quite significant in some cases. But I guess maybe that's minor in compared to the confusion here? I dunno, I'd say introduce a new content attribute if it's really that bad.

@jakearchibald
Copy link
Contributor Author

jakearchibald commented Apr 19, 2017

@domenic

This may be clearer if you stop relying on the invalid value default and type crossorigin="anonymous".

It still feels weird. Without crossorigin, it's anonymous, and if you want to it be not-anonymous to the same origin to tell it to be anonymous cross origin. I guess it's proving the rule with an exception, but it feels a bit backwards.

One concrete downside there is that it means the many requests per day made with fetch() and modules will now have the extra cookie bytes sent along

On the other hand, those requests will be able to use the HTTP/2 connection already opened by the page request, whereas non-credentialed requests require another connection.

Changing fetch's default feels like a huge change, but it does catch people out all the time.

@jakearchibald
Copy link
Contributor Author

jakearchibald commented Apr 19, 2017

This impacts HTTP/2 push as well. If you push your JS along with the page, it'll only be used if the developer opts into credentials for their module scripts.

I was debugging a case recently where a developer was pushing resources along with their page (or service worker, I can't remember) and wondering why they weren't being picked up when they did cache.addAll().

@annevk
Copy link
Member

annevk commented Apr 19, 2017

I think the new HTTP connection coupled with expectations about the default is more severe than a couple of bytes that could end up compressed in H/2 if they are the same for each request (as a single-byte pointer per https://tools.ietf.org/html/rfc7541 if I understand things correctly).

@domenic
Copy link
Member

domenic commented Aug 24, 2017

Just so people are aware, my position is that I would like module scripts to follow fetch(). If fetch changes its defaults, then I think module scripts should too. But if fetch() does not, I'd like to stick with what we have, even with the weird naming.

Is anyone interested in driving a decision on the fetch side?

annevk added a commit to whatwg/fetch that referenced this issue 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.
@annevk
Copy link
Member

annevk commented Aug 25, 2017

Created whatwg/fetch#585.

annevk added a commit to whatwg/fetch that referenced this issue Apr 20, 2018
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.

Tests: web-platform-tests/wpt#9911.
@annevk
Copy link
Member

annevk commented Apr 20, 2018

Fetch is updated now and implementations are expected to align. We should update module scripts too.

@annevk
Copy link
Member

annevk commented Apr 20, 2018

I looked at this and I think we should tackle this together with module workers and no longer allowing CORS for them. Then we can also remove the credentials member there entirely.

@annevk
Copy link
Member

annevk commented Apr 20, 2018

I didn't find time to untangle the entire script loading setup to make the changes needed here. https://github.com/whatwg/html/tree/annevk/module-scripts-and-credentials has some WIP.

@annevk
Copy link
Member

annevk commented Apr 21, 2018

Hmm, the top-level fetch needs to be same-origin, but what about the imports? importScripts() can go cross-origin, so I guess they should be able to do that too? Basically the same as a service worker?

It's probably best if @domenic fixes this since he has a much better handle on the various algorithms, but I'm willing to do the work given some feedback on all of the above.

@domenic
Copy link
Member

domenic commented Apr 21, 2018

Help fixing this would be much appreciated.

I thought the idea was to be consistent with fetch()'s default. In which case, it should be "same-origin" always?

Or are you asking about allowing them to have the option of "include"? For that case, yeah, I'd suggest keeping the credentials member for WorkerOptions, instead of removing it, and just changing the default.

@domenic
Copy link
Member

domenic commented Apr 21, 2018

Note also that importScripts() does not work inside a module worker. (But import does.)

@annevk
Copy link
Member

annevk commented Apr 21, 2018

Basically, I'm wondering if we should fix #3109 as part of this, since it makes credentials mode less interesting for module workers, at least for the top-level fetch. It's a little bit unclear if the imports for such a module worker can still go cross-origin though; I guess in theory there might not be a problem with that.

If we don't couple them, then I guess we need to keep the credentials member, but we should not allow it to be set to "omit" as you cannot get that for module scripts either.

annevk added a commit that referenced this issue Apr 30, 2018
* Module scripts are always fetched with credentials mode "same-origin" by default and can no longer
  use "omit". This makes them match other high-level platform features.
* The top-level script for module workers is always fetched with request mode "same-origin" and
  credentials mode "same-origin". Cross-origin workers did not quite work due to service workers.
* Any module worker import scripts are fetched with request credentials mode "same-origin" and this
  cannot be configured for now.

Fixes #2557 and fixes #3109.
annevk added a commit that referenced this issue Apr 30, 2018
* Module scripts are always fetched with credentials mode "same-origin" by default and can no longer
  use "omit". This makes them match other high-level platform features.
* The top-level script for module workers is always fetched with request mode "same-origin" and
  credentials mode "same-origin". Cross-origin workers did not quite work due to service workers.
* Any module worker import scripts are fetched with request credentials mode "same-origin" and this
  cannot be configured for now.

Fixes #2557 and fixes #3109.
annevk added a commit that referenced this issue May 23, 2018
* Module scripts are always fetched with request credentials mode "same-origin" by default. Only
  worker module scripts can still set that to "omit". Non-worker module scripts cannot, keeping
  them aligned with how the crossorigin attribute works for other platform features.
* The top-level script for module workers is always fetched with request mode "same-origin".
  Cross-origin workers did not quite work due to service workers.

Fixes #2557 and fixes #3109.
annevk added a commit that referenced this issue Oct 8, 2018
* Module scripts are always fetched with request credentials mode "same-origin" by default. Only
  worker module scripts can still set that to "omit". Non-worker module scripts cannot, keeping
  them aligned with how the crossorigin attribute works for other platform features.
* The top-level script for module workers is always fetched with request mode "same-origin".
  Cross-origin workers did not quite work due to service workers.

Fixes #2557 and fixes #3109.
domenic pushed a commit that referenced this issue Oct 9, 2018
* Module scripts are always fetched with request credentials mode
  "same-origin" by default, instead of the previous default of "omit".
  Only worker module scripts can still set that to "omit", using the
  credentials option to the Worker constructor. Non-worker module
  scripts, which only have the crossorigin="" attribute available, can
  only toggle between "same-origin" and "include", similar to how
  crossorigin="" works for other platform features.
* Similarly, import() statements inside of classic scripts now use the
  "same-origin" credentials mode, instead of "omit". This affects both
  <script> elements, where the default can be changed using
  crossorigin="", and other contexts like javascript: URLs and classic
  worker scripts, where the default cannot be changed.
* The top-level script for module workers is always fetched with request
  mode "same-origin". Cross-origin workers did not quite work due to
  service workers.

Fixes #2557. Fixes #3109.

Tests:

* web-platform-tests/wpt#11274
* web-platform-tests/wpt#13176
* web-platform-tests/wpt#13426
mustaqahmed pushed a commit to mustaqahmed/html that referenced this issue Feb 15, 2019
* Module scripts are always fetched with request credentials mode
  "same-origin" by default, instead of the previous default of "omit".
  Only worker module scripts can still set that to "omit", using the
  credentials option to the Worker constructor. Non-worker module
  scripts, which only have the crossorigin="" attribute available, can
  only toggle between "same-origin" and "include", similar to how
  crossorigin="" works for other platform features.
* Similarly, import() statements inside of classic scripts now use the
  "same-origin" credentials mode, instead of "omit". This affects both
  <script> elements, where the default can be changed using
  crossorigin="", and other contexts like javascript: URLs and classic
  worker scripts, where the default cannot be changed.
* The top-level script for module workers is always fetched with request
  mode "same-origin". Cross-origin workers did not quite work due to
  service workers.

Fixes whatwg#2557. Fixes whatwg#3109.

Tests:

* web-platform-tests/wpt#11274
* web-platform-tests/wpt#13176
* web-platform-tests/wpt#13426
mustaqahmed pushed a commit to mustaqahmed/html that referenced this issue Feb 15, 2019
* Module scripts are always fetched with request credentials mode
  "same-origin" by default, instead of the previous default of "omit".
  Only worker module scripts can still set that to "omit", using the
  credentials option to the Worker constructor. Non-worker module
  scripts, which only have the crossorigin="" attribute available, can
  only toggle between "same-origin" and "include", similar to how
  crossorigin="" works for other platform features.
* Similarly, import() statements inside of classic scripts now use the
  "same-origin" credentials mode, instead of "omit". This affects both
  <script> elements, where the default can be changed using
  crossorigin="", and other contexts like javascript: URLs and classic
  worker scripts, where the default cannot be changed.
* The top-level script for module workers is always fetched with request
  mode "same-origin". Cross-origin workers did not quite work due to
  service workers.

Fixes whatwg#2557. Fixes whatwg#3109.

Tests:

* web-platform-tests/wpt#11274
* web-platform-tests/wpt#13176
* web-platform-tests/wpt#13426
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

4 participants
@jakearchibald @domenic @annevk and others