-
Notifications
You must be signed in to change notification settings - Fork 563
[WIP] Adds secure_headers & Content-Security-Policy to Classroom #1166
base: master
Are you sure you want to change the base?
Conversation
I would like it to be in it's own file
Yes, using the forked version is the easiest path ATM so let's go with that one.
Absolutely 😄 |
/cc @oreoshake |
The csp changes look great 👍 but I can't speak to the rubocop changes or whether anything will break 😄. |
@oreoshake Thanks for taking a look! Good catch on the rubocop changes--I think those should match. It was complaining about the quotes for self in |
@@ -0,0 +1,20 @@ | |||
# Setup Secure Headers with default values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing magic comment # frozen_string_literal: true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @anglinb!!
@tarebyte np! Just a heads up: There is a CSP error on pretty much every page b/c the I punted a bit on refactoring js importing b/c its kinda out of scope of just adding CSP so if you're cool with having this error around in the console, then feel free to merge. If not I can try and add a workaround for now. Lmk 😄 |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
👋 Friendly bump |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@srinjoym this sounds right up your alley |
👋 I was taking a look at Classroom as part of the appsec-review process and, while this PR does not address a specific known vulnerability, adding the secureheaders gem and configuring Content-Security-Policy (CSP) Headers will help mitigate any available client-side attacks and those which may occur in the future.
This is a WIP PR b/c I have a few concerns on the best way to integrate secureheaders and handle some issues that came up:
Questions:
config/application.rb
file or should it get broken out into it's own file?jquery-datetimepicker-rails
gem?jquery-datepicker-rails
isv2.4.1.0
and it contains an outdated version of jQuery DateTimePicker plugin. This outdated version relies oneval
which it's newest releasev2.5.4
does not.unsafe-eval
jquery-datepicker-rails
= require_tree .
, which pulls in thehttps://www.youtube.com/iframe_api
.pages.js
file on the homepage.Next Steps:
If ya'll have a sec to answer these questions, I would be happy to fix up this PR. 👍
/cc @gregose