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

Workbox precaching network fallback cannot handle integrity #3096

Closed
zlk89 opened this issue Jul 13, 2022 · 5 comments · Fixed by #3099
Closed

Workbox precaching network fallback cannot handle integrity #3096

zlk89 opened this issue Jul 13, 2022 · 5 comments · Fixed by #3099
Assignees

Comments

@zlk89
Copy link

zlk89 commented Jul 13, 2022

Library Affected:
workbox-precaching

Browser & Platform:

Google Chrome v103.0.5060.114 for Mac

Issue or Feature Request Description:

In service worker:

const manifest = self.__WB_MANIFEST;
precacheAndRoute(manifest, {});
cleanupOutdatedCaches();

Way to reproduce:

  1. Set SRI integrity in manifest
  2. load the website and wait for SW to precache all manifest entries and install
  3. Manually delete the cache storage in Application tab or clear storage in Chrome setting
  4. reload the website, it will fallback the network request here, because the cache item has been cleared:
    if (this._fallbackToNetwork) {
  5. You should see the SRI error, because the fetch didn't use cors mode. (see TypeError on Request.integrity with no-cors mode is a foot gun whatwg/fetch#583 for context)

Potential fix: when integrity is used, it should use cors mode to fetch the resource

@jeffposnick
Copy link
Contributor

Thanks for reporting this issue with the precaching fallback logic, when SRI is used.

I can see two possible solutions:

  1. Automatically force a no-cors request to use cors mode when making a fallback request for a precached resource that uses SRI and that's been manually deleted from the cache. Then, use the response to "repair" the deleted cache entry.
  2. When making a fallback request in the same scenario, only attempt to use SRI and repair the deleted cache entry if the original request is in cors mode. If the request is no-cors (i.e. the scenario you're describing), then make the request as-is, without switching its mode, and don't use the response to repair the cache. Just pass the opaque network response back to the client page.

The first solution offers the advantage of repairing the missing cache entry, but there are a couple of (perhaps trivial?) issues:

  • It assumes that the remote server supports CORS, but for some reason, clients are making non-CORS requests. The initial precaching step would have failed if the remote server didn't support CORS, so that may be a safe assumption. But server configs do change over time.
  • It assumes that the client page can deal with a CORS response when it asked for an opaque response. Generally, problems occur when the opposite situation occurs (a client page needs a CORS response and gets back something opaque), but still, this might arguably be seen as a breaking change.

The second solution, to just not attempt to use SRI and repair the cache in that scenario, would be more backwards compatible and "safer".

My inclination is to go with a minor change to implement the second approach to work around the immediate issue, and potentially implement the first approach in Workbox v7.

@tropicadri, what are your thoughts?

@zlk89
Copy link
Author

zlk89 commented Jul 14, 2022

Thanks for the quick response and solutions.
One thing I didn't make it clear is that the original request in my case actually is in CORS mode (using crossorigin="anonymous" attribute in the <script /> tag), so current network fallback didn't use the cors mode of the original request made by the page.

@jeffposnick
Copy link
Contributor

That's unexpected. The fallback fetch() takes place at

response = await handler.fetch(
new Request(request, {
integrity: integrityInRequest || integrityInManifest,
}),
);

The intention behind passing in the network request to the Request constructor was for all of the original properties to carry over, with the addition of the integrity (if available). If you're not seeing that, then it might be a bug in our current logic that could be addressed independently of the other discussions.

@zlk89
Copy link
Author

zlk89 commented Jul 14, 2022

@jeffposnick actually I was wrong, the failed request was made by importScripts() in a worker script, which always uses no-cors request. So the integrity actually comes from manifest, instead of the original request in this case.

@jeffposnick
Copy link
Contributor

Thanks for confirming. So it sounds like the analysis in #3096 (comment) still applies.

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 a pull request may close this issue.

3 participants