Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for calculating CSP hashes of inline scripts #1371

Merged
merged 5 commits into from
Sep 3, 2020

Conversation

anders-kiaer
Copy link
Contributor

@anders-kiaer anders-kiaer commented Aug 18, 2020

Closes #630.

With the tests in this PR, we also increase the probability of the core dash framework remaining Content Security Policy (CSP) friendly. 馃敀馃懏


Contributor Checklist

  • I have broken down my PR scope into the following TODO tasks
  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

optionals

  • I have added entry in the CHANGELOG.md

@anders-kiaer anders-kiaer force-pushed the dash_inline_js_csp branch 2 times, most recently from e7e2fce to 35794dc Compare August 18, 2020 12:34
@anders-kiaer anders-kiaer marked this pull request as ready for review August 18, 2020 12:45
@anders-kiaer anders-kiaer force-pushed the dash_inline_js_csp branch 3 times, most recently from ff2f454 to 917a96a Compare August 18, 2020 19:16
def csp_hashes_inline_scripts(self, hash_algorithm="sha256"):
"""Calculates CSP hashes (sha + base64) of all inline scripts, such that
one of the biggest benefits of CSP (disallowing general inline scripts)
can be utilized together with Dash clientside callbacks (inline scripts).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anders-kiaer this looks great, and fantastic tests - I love does_not_raise, hadn't seen that trick before but I can see it coming in handy elsewhere.

The name is a little long, would it be ambiguous if we just call it csp_hashes or script_hashes?

Can you add a bit of usage example into the docstring here? Something like:

Calculate these hashes after all callbacks are defined, and add them to your CSP headers
immediately before starting the server, for example:

flask_talisman.Talisman(app.server, content_security_policy={
    "default-src": "'self'",
    "script-src": ["'self'"] + app.csp_hashes_inline_scripts()
})

Am I understanding correctly that this is what you'd normally do?

I haven't used flask_talisman before, but I presume that it works just as well (and with no changes to your code) with gunicorn as it does with run_server?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name is a little long, would it be ambiguous if we just call it csp_hashes or script_hashes?

Agree, it is currently a bit long. I think both of the two alternatives should be ok. If there is any ambiguity, that probably is:

  • csp_hashes: CSP hashes are, generally, used both for inline styles and inline scripts. So from the name alone it is not clear if it is script hashes, style hashes or both.

    However, in Dash framework context (since dynamic style attributes from Python side are set in a CSP compatible way through React, see Separate CSS file on build聽dash-core-components#753 (comment)), only script hashes should be necessary, which reduces the risk of the ambiguity. There are of course Dash components out there that needs to do changes similar to Separate CSS file on build聽dash-core-components#753, but that is on the different custom/standard Dash components to solve/fix in their webpack build setup, not something concerning the Dash framework.

  • script_hashes: Solves the style vs script ambiguity, but it does not indicate that the hashes are in "CSP format" and ready to go ('{hash algorithm}-{base 64 hash}', including the '').

I like csp_hashes best of the two 馃憤 If there appears e.g. a style hash use case later, for some reason, this can be solved with a natural non-breaking extension with proper default value, e.g. app.csp_hashes(hash_algorithm, source="scripts").

Am I understanding correctly that this is what you'd normally do?

I haven't used flask_talisman before, but I presume that it works just as well (and with no changes to your code) with gunicorn as it does with run_server?

Yes and yes 馃檪 gunicorn passes by default the (CSP) headers through. E.g. with that example CSP configuration in the suggested docstring, Dash users will get 馃敀 A+ rating out of the box on Mozilla observatory.

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! We'll hold off merging for a couple days as we're probably going to make a 1.15.1 patch release first, but I think this is ready to go. Thanks for the tweaks! 馃拑

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DashRenderer initialization and CSP
3 participants