search

ampproject / amphtml

The AMP web component framework.
https://amp.dev
Apache License 2.0
14.89k stars 3.89k forks source link

Enable CORS for assets on https://cdn.ampproject.org #3254

Closed adamzr closed 8 years ago

adamzr commented 8 years ago

I'd like to get the browser to preload https://cdn.ampproject.org/v0.js, so I sent an HTTP header on the response from my AMP HTML page like so:

Link:<https://cdn.ampproject.org/v0.js>; rel=preload; crossorigin

However, this gets me the error:

Link element resource from origin 'https://cdn.ampproject.org' has been blocked from loading by Cross-Origin Resource Sharing policy: No 'Access-Control-Allow-Origin' header is present on the requested resource. Origin 'https://adamricheimer.com' is therefore not allowed access.

Please set Access-Control-Allow-Origin: * on all responses from your CDN. This will allow me to use the Link header, which I think should increase performance slightly by allowing the browser to start downloading the file before the <script> tag is parsed.

Thanks!

cramforce commented 8 years ago

Hmm. My understanding is that this is not technically true. For some reason, preloads issued via header are not (somewhat ironically) executed until after the first chunk has been parsed. We tried :)

I think this is because, the browser devs found that resources in the top part of the doc are more critical than the header.

It is a bit sad. Ideally the request would be issued immediately from the network thread, without the 2 extra IPCs that can be quite expensive. Especially on Android.

I'm not super against adding the CORS response, but for now it would be mostly extra bytes.

adamzr commented 8 years ago

I was actually hoping that my CDN (CloudFlare) would push them via HTTP2 Server Push. But, it turns out that they only do that for non-crossorigin assets (not surprising).

Is that after 1st chunk rule part of the spec or just browser implementation? Because browsers may change....

cramforce commented 8 years ago

Sure, may change.

Push does only work if the resources are in the same HTTP2 tunnel (same IP and cert, can be different origin), but push is not good here, because there is little way to know whether the client already has them cached, and this being likely it would waste a lot of bandwidth.

cramforce commented 8 years ago

Anyway, I'm generally fine with adding this on the serving side. Do you know whether it actually doesn't work? Does it go into the cache?

Do you know why there is the origin limitation on the header when it is not present on the equivalent link tag?

adamzr commented 8 years ago

This is the screenshot of the error: Imgur

adamzr commented 8 years ago

I don't know why this limitation is only on the header

Nodemio commented 8 years ago

fork this repo and mantain update you fork... lazy

ScottHelme commented 8 years ago

I came across this same problem today and CORS is required to have the asset integrity checked with Subresource Integrity.

Can you set Access-Control-Allow-Origin: * to allow integrity checking of assets loaded from https://cdn.ampproject.org

ScottHelme commented 8 years ago

@adamzr could I suggest a title update for the issue to something like:

Enable CORS for assets on https://cdn.ampproject.org
cramforce commented 8 years ago

This would really be for /v0* and maybe /rtv/* right?

I don't see subresource integrity could be used with AMP as those assets change weekly, but I might be missing something and if I do I would be delighted :)

ScottHelme commented 8 years ago

@cramforce yeah that'd be right from what I've seen.

As for SRI I didn't realise the assets would change, are they not version controlled in the URL?

https://cdn.ampproject.org/v0.js
https://cdn.ampproject.org/v0/amp-analytics-0.1.js

If the assets do change then you're correct, SRI is not possible.

aidantwoods commented 8 years ago

@cramforce @ScottHelme Touching on the version control point, would it be possible for AMP to provide an alternate resource path for each asset?

https://cdn.ampproject.org/v0.js
https://cdn.ampproject.org/v0.x.y.js

https://cdn.ampproject.org/v0/amp-analytics-0.1.js
https://cdn.ampproject.org/v0/amp-analytics-0.1.z.js

So that the least specific version would be kept up to date, and the most specific would be version controlled – which would facilitate use of SRI.

cramforce commented 8 years ago

I realize this is a tradeoff, but AMP does not allow version locking URLs. Every AMP document on the internet runs the same version (modulo rollouts). As visible in the URLs, we do support major-versioning of the main binary and minor versioning of extensions, but independent of this AMP rolls out weekly.

There are various reasons for this:

Downsides:

ScottHelme commented 8 years ago

This is a shame but I can see why. Thanks for the solid explanation 👍