Workaround Worker same origin restriction#7587
Workaround Worker same origin restriction#7587dreamofabear merged 4 commits intoampproject:masterfrom
Conversation
b999c1e to
d27d79d
Compare
build-system/server.js
Outdated
| app.get(['/dist/ww.js', '/dist/ww.max.js'], function(req, res) { | ||
| fs.readFileAsync(process.cwd() + req.path).then(file => { | ||
| res.setHeader('Content-Type', 'application/javascript'); | ||
| res.setHeader('AMP-Access-Control-Allow-Source-Origin', getUrlPrefix(req)); |
There was a problem hiding this comment.
You want the normal CORS header Access-Control-Allow-Source-Origin and static value of * is OK here.
There was a problem hiding this comment.
Done. Also I think we want ampCors: false in the XHR since this resource isn't source-origin-specific.
build-system/server.js
Outdated
| */ | ||
| app.get(['/dist/ww.js', '/dist/ww.max.js'], function(req, res) { | ||
| fs.readFileAsync(process.cwd() + req.path).then(file => { | ||
| res.setHeader('Content-Type', 'application/javascript'); |
There was a problem hiding this comment.
Its what the AMP cache sends :)
src/web-worker/amp-worker.js
Outdated
| this.fetchPromise_ = this.xhr_.fetchText(url).then(text => { | ||
| // Fetch web worker binary and create from Blob as workaround since | ||
| // Worker constructor won't accept CORS URLs. | ||
| const blob = new win.Blob([text], {type: 'text/javascript'}); |
There was a problem hiding this comment.
You now probably want to factor out a isWorkerSupported function to determine whether to go the worker route.
There was a problem hiding this comment.
Looks like Blob and URL.createObjectURL (Blob URLs) both have good support.
src/web-worker/amp-worker.js
Outdated
| // Worker constructor won't accept CORS URLs. | ||
| const blob = new win.Blob([text], {type: 'text/javascript'}); | ||
| const blobUrl = win.URL.createObjectURL(blob); | ||
| this.worker_ = new win.Worker(blobUrl); |
There was a problem hiding this comment.
Just checked and I'm pretty sure this does not violate our CSP, but we probably should add an explicit worker-src CSP directive to lock that down.
There was a problem hiding this comment.
Good call -- actually it does violate our CSP after this PR due to Blob-ification.
worker-src is CSP3 which has poor support. Going to add blob: to fallback default-src instead: cl/147751445
There was a problem hiding this comment.
Please file an issue for Cloudflare to update their CSP. CC @dknecht.
And update and try out https://github.com/ampproject/amphtml/blob/master/examples/csp.amp.html before submitting cl/147751445
There was a problem hiding this comment.
- @dknecht confirmed Cloudflare change is on staging
- Verified functionality on
csp.amp.html
f68947d to
8124c90
Compare
8124c90 to
cb2a56b
Compare
|
@cramforce Friendly ping. |
* initial commit for worker blob * fix type error * malte's comments * fix unit tests
Fixes #7566.
/to @cramforce