-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Dev Server: Allow crossorigin fetch of JS binaries. #33492
Conversation
build-system/tasks/serve.js
Outdated
@@ -66,6 +66,7 @@ let lazyBuild = false; | |||
*/ | |||
function getMiddleware() { | |||
const middleware = [require('../server/app')]; // Lazy-required to enable live-reload | |||
middleware.push(header({'Access-Control-Allow-Origin': '*'})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think serve.js
should have to know about the details of implementation of how the server responds to a given request. Can we move this to build-system/server/app.js
and insert a generic URL handler that adds the header to all requests? Between lines 74 and 76 might work:
amphtml/build-system/server/app.js
Lines 59 to 86 in 6616e57
// Middleware is executed in order, so this must be at the top. | |
// TODO(#24333): Migrate all server URL handlers to new-server/router and | |
// deprecate app.js. | |
// TODO(erwinmombay, #32865): Make visual diff tests use the new server | |
if (!argv._.includes('visual-diff')) { | |
app.use(require('./new-server/router')); | |
} | |
app.use(require('./routes/a4a-envelopes')); | |
app.use('/amp4test', require('./amp4test').app); | |
app.use('/analytics', require('./routes/analytics')); | |
app.use('/list/', require('./routes/list')); | |
app.use('/test', require('./routes/test')); | |
if (argv.coverage) { | |
app.use('/coverage', require('istanbul-middleware').createHandler()); | |
} | |
// Append ?csp=1 to the URL to turn on the CSP header. | |
// TODO: shall we turn on CSP all the time? | |
app.use((req, res, next) => { | |
if (req.query.csp) { | |
res.set({ | |
'content-security-policy': | |
"default-src * blob: data:; script-src https://cdn.ampproject.org/rtv/ https://cdn.ampproject.org/v0.js https://cdn.ampproject.org/v0/ https://cdn.ampproject.org/viewer/ http://localhost:8000 https://localhost:8000; object-src 'none'; style-src 'unsafe-inline' https://cdn.ampproject.org/rtv/ https://cdn.materialdesignicons.com https://cloud.typography.com https://fast.fonts.net https://fonts.googleapis.com https://maxcdn.bootstrapcdn.com https://p.typekit.net https://use.fontawesome.com https://use.typekit.net; report-uri https://csp-collector.appspot.com/csp/amp", | |
}); | |
} | |
next(); | |
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aah, I now see why you initially added the header here. These lines (written by me a long time ago) set a bad example :)
amphtml/build-system/tasks/serve.js
Lines 72 to 74 in 9cb59ce
if (argv.cache) { | |
middleware.push(header({'cache-control': 'max-age=600'})); | |
} |
Probably makes sense to move this to app.js
as well, but feel free to punt / ignore if you don't think it makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! LGTM.
@rsimha: do we have a way of asking the license/cla check to refresh? |
Either have a CLA admin re-trigger the check, or push a new commit. I thought I was a CLA admin, but it appears I'm not. |
summary
Fixes #33426 (comment).
Requests for
ww.js
andamp-auto-lightbox
fail in Storybook because of CORs.See:
This PR adds the access control headers s.t. crossorigin requests (i.e. from storybook) can
fetch
js binaries.Open to any and all suggestions of alternative solutions.