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

Enable Content Security Policies #5186

Merged
merged 23 commits into from
Apr 14, 2021

Conversation

mishaschwartz
Copy link
Contributor

@mishaschwartz mishaschwartz commented Mar 19, 2021

Motivation and Context

For security reasons, we should implement some CSP policies. This PR makes a first attempt at that.

Ideally we want to enforce strong policies using nonce keys in all cases. However, we currently depend on at least two javascript libraries (heic2any and mathjax) that require more lax CSP policies. MathJax at least has indicated that it is unlikely to improve (mathjax/MathJax#1988). Further discussion on the best way forward is needed.

TODO: further spot-checks of pages to see if the new CSP rules block things that shouldn't be blocked

Your Changes

Description:

Type of change (select all that apply):

  • Other (please specify): security update

Testing

Questions and Comments (if applicable)

Checklist

  • I have performed a self-review of my own code.
  • I have fixed any Hound bot comments.
  • I have verified that the TravisCI tests have passed.
  • I have reviewed the test coverage changes reported on Coveralls.
  • I have updated the Changelog.md file.
  • I have described any required documentation changes below.

Required documentation changes (if applicable)

https://github.com/MarkUsProject/Markus/wiki/Pending-Changes#pr-5186

@mishaschwartz mishaschwartz marked this pull request as ready for review April 6, 2021 12:57
@david-yz-liu
Copy link
Collaborator

@mishaschwartz I'm testing these changes now, and have encountered 3 issues:

  1. The flash message containers don't close when you click on the "X" (it seems there's an onclick attribute set there).
  2. Under the exam template settings page, when you click on the "Add Template Division" link, 2 blank rows get added instead of just 1. (There's another bug which also occurs on master: when there's more than one exam template, clicking "Add Template Division" adds an empty row to every exam template's division table, not just the one that was clicked. You don't need to fix that here, just something I noticed.)
  3. Under the automated test settings page, there's an error when adding a new tester. In the JS console: "Error compiling schema, function code: ..."

I'll try to do more testing on this today, but wanted to get these to you first!

@mishaschwartz
Copy link
Contributor Author

mishaschwartz commented Apr 12, 2021

@david-yz-liu Amazing! Thanks for finding those

Copy link
Collaborator

@david-yz-liu david-yz-liu left a comment

Choose a reason for hiding this comment

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

Looking good! Left a few inline comments, and found two other issues;

  1. Scanned exam issues: I think the CSP is blocking the PDF loading for the "Fix Scans" and "Assign Scans" pages.
  2. In the codeviewer, there's a dynamically-generated toolbar with buttons for "view plan" and increasing/decreasing font size. The toolbar shows up but the buttons don't work.

app/assets/javascripts/Components/submission_table.jsx Outdated Show resolved Hide resolved
app/assets/stylesheets/common/_markus.scss Outdated Show resolved Hide resolved
app/views/annotation_categories/_boot.js.erb Outdated Show resolved Hide resolved
app/views/annotation_categories/index.html.erb Outdated Show resolved Hide resolved
app/views/annotation_categories/index.html.erb Outdated Show resolved Hide resolved
app/views/main/about.js.erb Outdated Show resolved Hide resolved
app/views/graders/_upload_modal.html.erb Outdated Show resolved Hide resolved
app/views/exam_templates/_student_info.html.erb Outdated Show resolved Hide resolved
app/views/exam_templates/index.html.erb Outdated Show resolved Hide resolved
@david-yz-liu david-yz-liu merged commit 62b1399 into MarkUsProject:master Apr 14, 2021
@mishaschwartz mishaschwartz deleted the enable-basic-csp branch April 14, 2021 21:25
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