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

themes/mkdocs: Avoid inline ressources (like CSS and JS) #1907

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

nbraud
Copy link

@nbraud nbraud commented Nov 17, 2019

This moves inline resources, like CSS and JS snippets, to base.{css,js}.

Inline resources make it impossible to implement a safe Content Security Policy, as they require using the 'unsafe-inline' directive, essentially voiding all guarantees CSP can provide.

With the suggested changes, the theme functions with the following policy:

Content-Security-Policy:
  default-src 'none'; connect-src 'self'; font-src 'self'; img-src 'self' data:;
  script-src 'self' https://cdnjs.cloudflare.com/ajax/libs/highlight.js/;
  style-src  'self' https://cdnjs.cloudflare.com/ajax/libs/highlight.js/;

@nbraud
Copy link
Author

nbraud commented Nov 17, 2019

There is a preview version of those changes, with the CSP mentionned.

@waylan
Copy link
Member

waylan commented Nov 17, 2019

I don't have any objections to these changes. However, I feel the need to provide a warning regarding the Content Security Policy this allegedly enables. As a reminder, it is common for raw HTML within Markdown to make use of inline styles. In fact, this is even encouraged in Markdown's documentation.

For any markup that is not covered by Markdown’s syntax, you simply use HTML itself.

However, even with raw HTML disabled, many possible XSS vulnerabilities exist in Markdown's syntax. See Markdown and XSS for a great summary of those issues. In other words, updating the theme does not guarantee that the Content Security Policy is being met.

Of course, there will be some users of MkDocs who will want/need a strict Content Security Policy and without these changes they cannot have that (thus the changes will be accepted). This is just a warning that changing the theme alone is not going to guarantee such a policy is met. If users want to enforce such a policy for their Markdown, then they will need to take additional steps. That could be as simple as documenting and enforcing a strict policy of what is and is not allowed in the Markdown for their project. Or it could involve using a Plugin to run Markdown's output through an HTML sanitizer (presumably from the on_page_content event). Either way, those are things which can be implemented/enforced by the user and do not require any additional changes to MkDocs itself.

However, there are a couple additions we might want to consider:

  1. The readthedocs theme should also be updated to meet the CSP (unless it already does -- I haven't checked).
  2. We should add a comment to the release notes which notes that the existing themes now meet the CSP.
  3. We might also want to add a note to the Contributing Guide which outlines a policy in which all contributions to MkDocs itself must meet the CSP so that users of MkDocs can generate sites which meet the CSP if they desire/need.
  4. We might also want to consider what policy to adopt for our own documentation at mkdocs.org and document that as well.

@waylan
Copy link
Member

waylan commented Nov 18, 2019

To be clear, at a minimum, the first two items of the list in my previous comment (update the readthedocs theme and the release notes) are required before this PR can be accepted. Ideally, item 3 would be included as well, although this is not necessary. However, without it, we can't guarantee that future submissions won't undo the changes made here. Item 4 probably should be addressed separately.

footer {
margin-top: 30px;
margin-bottom: 10px;
text-align: center;
font-weight: 200;
}

keyboard-keys {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a class so is missing the . at the beginning.

@@ -117,13 +117,21 @@ a:hover code, a:focus code {
color: #157AB5;
}

centered {
Copy link
Contributor

Choose a reason for hiding this comment

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

not a really big deal - but a class name of text-center would be more semantically correct.

@pawamoy
Copy link
Sponsor Contributor

pawamoy commented Apr 30, 2023

Hi @nbraud, still interested in getting this merged? If so, could you rebase your code on master, and keep the PR related to how resources are loaded, without changing the actual CSS?

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.

None yet

4 participants