-
Notifications
You must be signed in to change notification settings - Fork 370
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
[security] Unsafe Inline Script #3876
base: master
Are you sure you want to change the base?
Conversation
257f4f3
to
5b5dd88
Compare
5b5dd88
to
4c3815b
Compare
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.
Looking good overall! I've added few review comments..
@@ -137,6 +137,9 @@ http_500_debug_mode=false | |||
# X-Content-Type-Options: nosniff This is a HTTP response header feature that helps prevent attacks based on MIME-type confusion. | |||
## secure_content_security_policy="script-src 'self' 'unsafe-inline' 'unsafe-eval' *.googletagmanager.com *.doubleclick.net data:;img-src 'self' *.doubleclick.net http://*.tile.osm.org *.tile.osm.org *.gstatic.com data:;style-src 'self' 'unsafe-inline' fonts.googleapis.com;connect-src 'self' *.google-analytics.com;frame-src *;child-src 'self' data: *.vimeo.com;object-src 'none'" | |||
|
|||
# Enable nonce attribute to remove unsafe-inline and auto remove unsafe-inline from csp | |||
## csp_nonce=True |
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.
Nit: true
@@ -142,6 +142,10 @@ | |||
# X-Content-Type-Options: nosniff This is a HTTP response header feature that helps prevent attacks based on MIME-type confusion. | |||
## secure_content_security_policy="script-src 'self' 'unsafe-inline' 'unsafe-eval' *.googletagmanager.com *.doubleclick.net data:;img-src 'self' *.doubleclick.net http://*.tile.osm.org *.tile.osm.org *.gstatic.com data:;style-src 'self' 'unsafe-inline' fonts.googleapis.com;connect-src 'self' *.google-analytics.com;frame-src *;child-src 'self' data: *.vimeo.com;object-src 'none'" | |||
|
|||
# Enable nonce attribute to remove unsafe-inline and auto remove unsafe-inline from csp | |||
## csp_nonce=True |
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.
Nit: true
# Assuming 'csp_nonce' is set in the request and CSP_NONCE controls its usage | ||
if desktop.conf.CSP_NONCE.get(): |
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.
Instead of assuming, why not check for it by using hasattr()
?
if csp_nonce: | ||
return f' nonce={csp_nonce}' | ||
return '' |
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.
Nit: Try something like this once?
return f' nonce={csp_nonce}' if csp_nonce else ''
nonce = secrets.token_urlsafe() | ||
request.csp_nonce = nonce |
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.
Nice!
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.
LGTM
'text script': function (text) { | ||
return text; | ||
} | ||
'text script': text => text |
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.
Just out of curiosity, was this change needed or just an improvement?
@@ -295,8 +296,9 @@ ${ hueIcons.symbols() } | |||
</div> | |||
${ commonshare() | n,unicode } | |||
|
|||
% for bundle in get_hue_bundles('hue'): | |||
${ render_bundle(bundle) | n,unicode } | |||
% for bundle in get_hue_bundles('login' if section == 'login' else 'hue', 'LOGIN' if section == 'login' else 'DEFAULT'): |
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.
Why the login related checks for hue.mako?
<script type="application/json" id="editorOptionsJson"> | ||
${ options_json | n,unicode,antixss } | ||
</script> | ||
<script ${nonce_attribute(request)} src="${ static('desktop/js/editor-component.js') }"></script> |
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.
Missing js file in the commit
Have Closed the existing PR, splitting the PR to smaller PR for easier Review:
Upcoming PRs:
What changes were proposed in this pull request?
How was this patch tested?
Please review Hue Contributing Guide before opening a pull request.