-
Notifications
You must be signed in to change notification settings - Fork 108
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
fix: [VIDECO-10247] Add CSP header #881
Conversation
server/serverMethods.js
Outdated
contentSecurityPolicy: { | ||
useDefaults: true, | ||
directives: { | ||
defaultSrc: ["'self'", "'unsafe-inline'"], |
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 would advise not to use naked asterisks (*) or "unsafe-inline" where possible. Those are some of the line items that we addressed on ID-Server security review https://jira.vonage.com/browse/VIDECO-9976
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 see. I have removed "'unsafe-inline'" from the defaultSrc
directive. But I found that I had to leave "'unsafe-inline'" in place for the scriptSrc
(which is normal, I think) and styleSrc
directives. However VIDECO-9976 asks us not to set it for styleSrc
. But I will leave it in, because without it, OpenTok.js videos are badly misaligned, and you get an error:
Refused to apply inline style because it violates the following Content Security Policy directive: "style-src 'self' cdnjs.cloudflare.com assets.tokbox.com static.opentok.com https://unpkg.com/@vonage/video-client@2/dist/js/opentok.js https://unpkg.com/@vonage/client-sdk-video@2/dist/js/opentok.js". Either the 'unsafe-inline' keyword, a hash ('sha256-47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU='), or a nonce ('nonce-...') is required to enable inline execution.
This is because OpenTok.js loads an inline style (from the WebPack's style-loader module):
var update = require("!../../node_modules/style-loader/lib/addStyles.js")(content, options);
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.
Got it. My understanding is that it's not a best practice to have 'unsafe-inline' in the script-src. hmmmmm...a common workaround would be to move the javascript into its own file-but these files aren't static.
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.
There's no JavaScript file that we can move here (in this project). The script that loads the styles inline is from WebPack's style-loader, used in OpenTok.js. Not in the source code for this project.
'cdnjs.cloudflare.com', | ||
'assets.tokbox.com', | ||
'static.opentok.com', | ||
], | ||
connectSrc: ['*'], |
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 would narrow the scope here too if possible from "*"
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.
Note that the OpenTok Playground tool has a CSP connect-src
directives that includes '*'
.
To get connect-src and image-src directives to work without '*'
, I need to specify this:
connectSrc: [
"'self'",
'*.tokbox.com',
'*.opentok.com',
'www.google-analytics.com',
'analytics.google.com',
'stats.g.doubleclick.net',
'wss:',
],
imgSrc: [
"'self'",
'cdnjs.cloudflare.com',
'www.googletagmanager.com',
'data:',
],
Note that the 'wss:'
directive for connect-src is given without a domain because the WebSocket URLs used by OpenTok.js are subject to change. (For example, currently when testing, I get a WSS URL of wss://media-sfu2-6444674bb5-vtc7q-us-west-2.media.prod.tokbox.com/rumorwebsocketsv2?socketId=c873c560-a30c-4c8e-83e0-3c3b93967812&attempt=18444deb-99b7-496e-a055-62af697fe0a0.)
I am tempted to leave these set to '*'
. Wdyt?
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.
The more narrow the scope of the wildcards the better. If the app can continue to function properly I would keep 'em scoped down.
No description provided.