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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

browse: add Content-Security-Policy w/ nonce #6425

Merged
merged 3 commits into from
Jul 6, 2024

Conversation

steffenbusch
Copy link
Contributor

This pull request is adding a Content-Security-Policy (CSP) response header to the file server browse template. The CSP Version 3 is using strict-dynamic for script-src and style-src with a generated, unique nonce, which is then used in <script> and <style> to whitelist the content of such elements.

Also:

  • Styles from svg such as the caddy-logo, have been changed into svg-attributes.
  • Moved inline JavaScript from body or href attribute to a dedicated <script> section with an event listener.

See also: https://caddy.community/t/best-practice-csp-for-file-server-browse/24714

Right now, the CSP is enabled by default with $enableCsp := true, so it would be also rather easy to disable it with this single change in a custom browse templates.

At the moment, this browse template is active here: https://alma.stbu.net/testing-something/

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Thanks for the clever enhancement! I like this a lot. Just one question for you, then I think we can merge this.

modules/caddyhttp/fileserver/browse.html Outdated Show resolved Hide resolved
@mholt
Copy link
Member

mholt commented Jul 5, 2024

Also... lol, I love all these test cases: https://alma.stbu.net/testing-something/test-cases/

@steffenbusch steffenbusch requested a review from mholt July 5, 2024 20:41
@mholt mholt merged commit 4ef3607 into caddyserver:master Jul 6, 2024
23 checks passed
@mholt
Copy link
Member

mholt commented Jul 6, 2024

I did notice there's still CSP warnings in Firefox:

Content-Security-Policy: Ignoring “'unsafe-inline'” within script-src: ‘strict-dynamic’ specified [testing-something](https://alma.stbu.net/testing-something/)
Content-Security-Policy: Ignoring “https:” within script-src: ‘strict-dynamic’ specified [testing-something](https://alma.stbu.net/testing-something/)
Content-Security-Policy: Ignoring “http:” within script-src: ‘strict-dynamic’ specified [testing-something](https://alma.stbu.net/testing-something/)
Content-Security-Policy: Ignoring “'unsafe-inline'” within script-src: nonce-source or hash-source specified [testing-something](https://alma.stbu.net/testing-something/)
Content-Security-Policy: Ignoring source “strict-dynamic” (Only supported within script-src). [testing-something](https://alma.stbu.net/testing-something/)
Content-Security-Policy: Ignoring ‘block-all-mixed-content’ because mixed content display upgrading makes block-all-mixed-content obsolete.

@steffenbusch
Copy link
Contributor Author

I did notice there's still CSP warnings in Firefox:

Content-Security-Policy: Ignoring “'unsafe-inline'” within script-src: ‘strict-dynamic’ specified [testing-something](https://alma.stbu.net/testing-something/)
Content-Security-Policy: Ignoring “https:” within script-src: ‘strict-dynamic’ specified [testing-something](https://alma.stbu.net/testing-something/)
Content-Security-Policy: Ignoring “http:” within script-src: ‘strict-dynamic’ specified [testing-something](https://alma.stbu.net/testing-something/)
Content-Security-Policy: Ignoring “'unsafe-inline'” within script-src: nonce-source or hash-source specified [testing-something](https://alma.stbu.net/testing-something/)
Content-Security-Policy: Ignoring source “strict-dynamic” (Only supported within script-src). [testing-something](https://alma.stbu.net/testing-something/)
Content-Security-Policy: Ignoring ‘block-all-mixed-content’ because mixed content display upgrading makes block-all-mixed-content obsolete.

I will have a look on this.

@steffenbusch
Copy link
Contributor Author

I will make the following changes to the Content Security Policy to address specific warnings:

  1. Remove strict-dynamic from style-src:

    Content-Security-Policy: Ignoring source “strict-dynamic” (Only supported within script-src).

  2. Remove block-all-mixed-content:

    Content-Security-Policy: Ignoring ‘block-all-mixed-content’ because mixed content display upgrading makes block-all-mixed-content obsolete.

See also: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/block-all-mixed-content

  1. Remove strict-dynamic, https:, and http: from script-src:

    Content-Security-Policy: Ignoring “'unsafe-inline'” within script-src: ‘strict-dynamic’ specified
    Content-Security-Policy: Ignoring “https:” within script-src: ‘strict-dynamic’ specified
    Content-Security-Policy: Ignoring “http:” within script-src: ‘strict-dynamic’ specified

After these changes, only the following warning will remain due to the presence of 'unsafe-inline' within script-src for backward compatibility with browsers not supporting Content-Security-Policy Version 3 (such as Internet Explorer 11):

Content-Security-Policy: Ignoring “'unsafe-inline'” within script-src: nonce-source or hash-source specified

Unfortunately, Firefox will log this directive being ignored. We must make a trade-off between this warning and maintaining backward compatibility.

See also: https://developer.chrome.com/docs/lighthouse/best-practices/csp-xss/#ensure_csp_is_backwards_compatible

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

Successfully merging this pull request may close these issues.

2 participants