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

CORS preflight for static import in a module service worker #1574

Open
d0iasm opened this issue Mar 17, 2021 · 5 comments
Open

CORS preflight for static import in a module service worker #1574

d0iasm opened this issue Mar 17, 2021 · 5 comments

Comments

@d0iasm
Copy link

d0iasm commented Mar 17, 2021

A static import for a cross-origin script in a module service worker causes a CORS preflight due to the added "Service-Worker" header (specified in https://w3c.github.io/ServiceWorker/#update-algorithm). Hence the fetch operation will fail unless the remote server handles CORS preflight correctly.

For example, assume there is a service worker script like this

// https://a.com/sw.js
import "https://b.com/cross-origin-script.js";

and if https://b.com/cross-origin-script.js doesn’t have “Access-Control-Allow-Headers: service-worker” and “Access-Control-Allow-Origin: a.com“, the registration of sw.js will fail with the following error:

"Access to script at 'https://b.com/cross-origin-script.js’ from origin 'https://a.com' has been blocked by CORS policy: Request header field service-worker is not allowed by Access-Control-Allow-Headers in preflight response."

(Tested it on Chrome)

I think it’s confusing behavior so I’d like to suggest the following options:

  1. Keep the current behavior and a cross-origin script should explicitly return “Access-Control-Allow-Headers: service-worker” in its response.

  2. Add “Service-Worker” to “CORS-safelisted request-header”.

  3. Do not add the "Service Worker: script" header to import scripts.

@wanderview
Copy link
Member

@youennf, did webkit run into this problem in its implementation?

Adding "service-worker" to the safe list seems the best option to me. What do you think, @annevk?

@annevk
Copy link
Member

annevk commented Mar 17, 2021

  1. Do we still need this header now that we have Sec-Fetch-Dest: serviceworker? Or do these get Sec-Fetch-Dest: script as they are dependencies?
  2. I think technically this is allowed by Fetch as Service Workers does not set request's unsafe-request flag, but that flag is kinda bad and doesn't really make sense from the same-origin policy perspective.
  3. I've tried to get a similar issue resolved for EventSource without much luck (though see 4): Safelist Last-Event-ID whatwg/fetch#568.
  4. Adding it to the safelist would allow developers to use other values for this header, I'm not sure that's what we want. And even if we did want it, we'd probably want to restrict the value space somewhat, similar to what we do for other safelisted headers. It would have been good if this header had used the Sec- prefix as we generally do for new cross-origin request headers.

@wanderview
Copy link
Member

Possibly related: whatwg/fetch#1005

Given that most browsers exempt browser-added headers, should this header be treated the same? Chrome doesn't do this by default, but I think other browsers do.

@jakearchibald
Copy link
Contributor

Do not add the "Service Worker: script" header to import scripts.

We don't add this header to importScripts() scripts, so I think this is the right thing to do here too. This means server logs can identify the root of scripts being used as service workers.

Also, right now we're breaking cases where updateViaCache is set to "imports", which should signal that the browser can go to the HTTP cache for imports when checking for updates. With modules, we're not treating imports as imports by that definition.

It doesn't look like we're doing the byte-identical check on imported scripts either.

@wanderview
Copy link
Member

So it seems we need to move the "Append Service-Worker/script to request’s header list." step from:

https://w3c.github.io/ServiceWorker/#update-algorithm

Step 9 substep 1 to be after substep 4 where "is top-level flag" check is made.

Maybe we should file a separate issue for byte-for-byte update checking issues with es modules.

blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this issue Apr 1, 2021
Link to the spec discussion issue can be found at
w3c/ServiceWorker#1574

Bug: 1191514
Change-Id: I4c8680e11f01178567fe8e58df267b51a4baeff6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2785310
Reviewed-by: Ben Kelly <[email protected]>
Commit-Queue: Ghazale Hosseinabadi <[email protected]>
Cr-Commit-Position: refs/heads/master@{#868484}
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

No branches or pull requests

4 participants