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

Ensure font stylesheets are requested in CORS mode in both AMP and non-AMP documents #1486

Merged
merged 2 commits into from
Oct 3, 2018

Conversation

westonruter
Copy link
Member

The AMP plugin is currently using the post-processor to add crossorigin=anonymous to font stylesheet links. This is not being done, however, in the non-AMP responses. This is a problem because when a service worker is caching the font stylesheet responses with CORS but then tries to serve it in response to a stylesheet being requested without CORS, then the fetch fails entirely with:

The FetchEvent resulted in a network error response: an "opaque" response was used for a request whose type is not no-cors

So we need to make sure that such font URLs always get served with CORS (crossorigin="anonymous") when in AMP and not-AMP alike to prevent this problem from happening.

Another benefit of CORS is that it ensures that a service worker caching the external stylesheet will not inflate the storage quota. See:

This PR is cherry-picking 02fd0a7 from #1261. We'll not ship the PWA plugin integration for 1.0, but this fix needs to be deployed first.

@westonruter westonruter added Bug Something isn't working Integration: PWA labels Oct 3, 2018
@westonruter westonruter added this to the v1.0 milestone Oct 3, 2018
Copy link
Member

@amedina amedina left a comment

Choose a reason for hiding this comment

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

LGTM.

@westonruter westonruter force-pushed the fix/font-stylesheet-cors branch from 7ad5fca to 4f2259c Compare October 3, 2018 22:52
@westonruter westonruter merged commit 9bc5c04 into 1.0 Oct 3, 2018
@westonruter westonruter deleted the fix/font-stylesheet-cors branch October 3, 2018 22:59
@kienstra
Copy link
Contributor

kienstra commented Oct 3, 2018

This Looks Good

Hi @westonruter,
To echo @amedina, this PR looks good. As expected, a font stylesheet in Twenty Seventeen has crossorigin="anonymous" in both the AMP and non-AMP URLs:

crossorigin-anonymous-present

kienstra added a commit that referenced this pull request Oct 3, 2018
This was recently merged,
so ensure that it's added.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Integration: PWA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants