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

Make Kolibri compliant with a secure Content Security Policy #12851

Merged
merged 8 commits into from
Nov 22, 2024

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Nov 15, 2024

Summary

  • Use JSON in <template> tags rather than inline <script> tags for all our injection of data into the frontend from the backend
  • Add our own frontend parser (with lazy resolution when Proxies are available) for generated URL JSON data from django-js-reverse
  • Now requires that all frontend URL name references only use snake_case and no kebab-case.
  • Updates how we register i18n frontend messages, by updating our webpack configuration to inject the registration logic as the first thing to happen in the bundled JS - unless it is the core bundle, in which case it happens last, to let the core global object to be defined first
  • Resorbs the browser checking into the core bundle, as it increased complexity of our code, and was putting people with incompatible browsers at higher priority than our frequent users
  • Cleans up all our unused (non-content renderer specific) async module loading code that was relying on injected JS
  • Turns our in-context translation into a plugin that is not enabled by default, gets rid of the inline JS that it previously relied on and load it via a script tag
  • Adds the Django CSP middleware to Kolibri, and sets sensible defaults that allow Kolibri to function fully in both production and dev modes.
  • Adds a single CSP related deployment option to globally add host-sources to the CSP - this is done simplistically for now, and assumes that finegrained control is either not required, or can be achieved by directly changing settings if needed.

References

Fixes #12809

Note: it goes slightly beyond what is outlined in the issue, by actually adding the CSP headers as well with Django CSP, but it seemed like the only way to show it was working as intended!

Reviewer guidance

Do any assets not load properly, is anything too strictly defined for proper functioning either in production or development mode?

Is anything too loosely defined? One particular thing to note is that the iframe src is very permissive when ZIP_CONTENT_ORIGIN is not defined, because we can't set a port on self - so if we wanted to make it stricter, we'd have to dynamically set the CSP based on the host that Kolibri was accessed from.

@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... DEV: frontend DEV: tools Internal tooling for development labels Nov 15, 2024
Copy link
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

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

One major question about script-src being loose

Also, outside of the scope of the issue, I wonder if we should think about adding Access Control headers.

In testing, I noticed some 404s when viewing an African Storybook HTML5. It does seem to be affecting it because compared to the catalog server, it looks very different.
image

kolibri/core/content/hooks.py Show resolved Hide resolved
packages/kolibri/urls.js Outdated Show resolved Hide resolved
kolibri/deployment/default/settings/base.py Show resolved Hide resolved
@rtibbles
Copy link
Member Author

In testing, I noticed some 404s when viewing an African Storybook HTML5. It does seem to be affecting it because compared to the catalog server, it looks very different.

Interesting - the Django CSP middleware shouldn't be affecting anything inside the iframe, but maybe something is happening inadvertently!

@github-actions github-actions bot added APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) labels Nov 22, 2024
@rtibbles
Copy link
Member Author

Hrm, I replicated the 404s in African Storybook, but they seemed to be because the files are actually missing from the zip file, and I replicate exactly the same 404s on develop too.

@rtibbles
Copy link
Member Author

Updated with feedback, will add a note to PR description about making URL names in the frontend more strict as a result.

Copy link
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

if (typeof Proxy === 'undefined' || process.env.NODE_ENV !== 'production') {
this._createFallbackInterface();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

:shipit:

@rtibbles rtibbles merged commit d3ff5ec into learningequality:develop Nov 22, 2024
41 checks passed
@rtibbles rtibbles deleted the unsafe_inline branch November 22, 2024 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) DEV: backend Python, databases, networking, filesystem... DEV: frontend DEV: tools Internal tooling for development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow Kolibri to run without a CSP of unsafe-inline
2 participants