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

Add CSP header page #243

Merged
merged 4 commits into from
Dec 7, 2023
Merged

Add CSP header page #243

merged 4 commits into from
Dec 7, 2023

Conversation

sbreker
Copy link
Member

@sbreker sbreker commented Oct 26, 2023

Add security section page detailing CSP header functionality.

@sbreker sbreker force-pushed the dev/csp-header-documentation branch from 0467e87 to 4209c7d Compare October 26, 2023 21:43
@anvit anvit added this to the 2.8 milestone Oct 26, 2023
@sbreker sbreker force-pushed the dev/csp-header-documentation branch from 4209c7d to f1a6101 Compare October 26, 2023 23:07
Copy link
Contributor

@fiver-watson fiver-watson left a comment

Choose a reason for hiding this comment

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

Hi @sbreker , this looks great!

You forgot to add a link to it to the general index.rst landing page for all the docs, and I've requested some subheader anchors be added, but overall this content looks awesome. Thanks!

admin-manual/security/csp-headers.rst Show resolved Hide resolved
Once the response is sent to the user's browser, the browser acts as a security
enforcer. It checks each requested resource and script against the CSP
directive. If any resource or script does not match the trusted sources
specified in the CSP header or lack the correct nonce, the browser rejects those
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: should it be "lacks" rather than lack here? hard to keep track of...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - that sounds better - fixed.

elements, preventing them from loading or executing. This strict validation
process ensures that only content from trusted sources, as defined by the CSP,
is allowed to run on the webpage. In doing so, CSP effectively protects against
XSS attacks and other security threats, making the user's browsing experience
Copy link
Contributor

Choose a reason for hiding this comment

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

might be good to spell this out the first time, i.e. "protects against cross-site scripting (XSS) attacks and..."

Copy link
Member Author

Choose a reason for hiding this comment

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

Added.

XSS attacks and other security threats, making the user's browsing experience
safer and more secure.

.. mermaid::
Copy link
Contributor

Choose a reason for hiding this comment

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

I've not see this used before, did you generate the preview files when writing these and ensure that it renders correctly? If yes, great!

I've always done it by first specifying it's a code block, followed by the specific language / format used - if Sphinx knows it, then it will properly color the syntax, etc - like this:

.. code-block:: mermaid

That said, if this short-hand works in the local renders, then nevermind!

Copy link
Member Author

@sbreker sbreker Oct 30, 2023

Choose a reason for hiding this comment

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

I have tested this locally and it seems to work. I've added the appropriate references for this to work in a second commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor

Choose a reason for hiding this comment

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

yep, looks good! thanks for double-checking too :D

Specifying ``self`` ensures that only trusted resources from the *same origin* are loaded
and executed.

Updating these settings will require restarting **php-fpm**.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest linking to the instructions for restarting PHP-FPM, since it changes with each PHP version. i.e.

Updating these settings will require restarting :ref:`PHP-FPM <troubleshooting-restart-php-fpm>`.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea! Added...

admin-manual/security/csp-headers.rst Show resolved Hide resolved
of the CSP policy by changing the header from
``Content-Security-Policy-Report-Only`` to ``Content-Security-Policy``.

.. _allow-inline-sources:
Copy link
Contributor

Choose a reason for hiding this comment

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

i'd suggest prefacing this anchor with csp- so we can tell what the anchor relates to at a glance, and all CSP anchors are similarly named? (and generally all subheadings on this page, minus the page header one, which gets the security- prefix to match other pages in the section).... e.g.

.. _csp-allow-inline-sources:

Copy link
Member Author

Choose a reason for hiding this comment

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

Added

admin-manual/security/csp-headers.rst Show resolved Hide resolved
* https://developers.google.com/web/fundamentals/security/csp/
* https://content-security-policy.com/examples/google-maps/

.. SEEALSO::
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this was from your security doc! 👍

requirements.txt Outdated
@@ -1,2 +1,3 @@
sphinx
sphinx_rtd_theme
sphinxcontrib-mermaid
Copy link
Contributor

Choose a reason for hiding this comment

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

oh cool, i guess that this add support for the mermaid block you added above! great, if it's tested then you can ignore my comments there

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@fiver-watson fiver-watson left a comment

Choose a reason for hiding this comment

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

I caught one minor typo I missed on the first round, but it's a minor issue. Otherwise, everything else looks great! Thanks Steve!

XSS attacks and other security threats, making the user's browsing experience
safer and more secure.

.. mermaid::
Copy link
Contributor

Choose a reason for hiding this comment

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

yep, looks good! thanks for double-checking too :D


.. IMPORTANT::

CSP headers will only be applied toa response if a Bootstrap 5 based theme is in use. See:
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: just saw this tiiiiiiny typo here (toa instead of "to a")

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch! Fixed 👍

@sbreker sbreker force-pushed the dev/csp-header-documentation branch 2 times, most recently from ab6a85a to 9cc0467 Compare October 30, 2023 21:40
Copy link
Contributor

@fiver-watson fiver-watson left a comment

Choose a reason for hiding this comment

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

👍

@fiver-watson
Copy link
Contributor

@sbreker did this never get merged? or is this an older version and the work got merged elsewhere? just wondering if some cleanup is needed here...

sbreker and others added 4 commits December 5, 2023 14:09
Add security section page detailing CSP header functionality.
Added note about updating the default header type. Updated directive to
include the Google integration changes. Additional detail added to CSP
implementation section.
@sbreker sbreker force-pushed the dev/csp-header-documentation branch from 0449676 to 6209ff0 Compare December 6, 2023 17:06
Copy link
Contributor

@anvit anvit left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks @sbreker !

Copy link
Contributor

@fiver-watson fiver-watson 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!

@sbreker sbreker merged commit 6209ff0 into 2.8 Dec 7, 2023
3 checks passed
@sbreker sbreker deleted the dev/csp-header-documentation branch December 7, 2023 18:07
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.

3 participants