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.

Sign up for GitHub

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

Jump to bottom

csp (attempt 2) #738

Merged
merged 4 commits into from
Oct 1, 2019
Merged

csp (attempt 2) #738

merged 4 commits into from
Oct 1, 2019

Conversation

thomasdziedzic
Copy link
Contributor

Hopefully fixing the issues described at #737

Added connect_src :self to fix the commenting issues.

Also found an additional case that we have to handle when submitting stories.
Unfortunately it performs an eval when previewing a story submission, so we have to enable that as well.

Finally, I learned that we have a comment spec which should have caught the posting issue, but it succeeded so my guess is that capybara tests do not enforce content security policies during a test which is a bummer.

@thomasdziedzic
Copy link
Contributor Author

thomasdziedzic commented Sep 12, 2019 •
edited
Loading

@pushcx I gave myself some time to think of the solution and I would be more comfortable if we kept it report only and modified the csp to match what we would have deployed to production.

This will allow more time to gather metrics on blocked content and allow us to catch any further major issues, especially after breaking commenting/voting today.

@pushcx
Copy link
Member

pushcx commented Oct 1, 2019

Sorry, I'm not sure I understand your suggestion here. I see you set this PR to report-only - so you want me to merge + deploy this to generate some metrics for us to analyze to inform later enforcing the CSP?

@thomasdziedzic
Copy link
Contributor Author

Yup, I want to generate some more metrics with this updated policy. I know there are some browser plugins which might cause false positives, but I want to make sure we can explain all the metrics before merging this in.

Maybe at some point in a couple of weeks, we can revisit all the generated reports and see if there is any need for further policy adjustment.

@pushcx pushcx merged commit 88cf3cc into lobsters:master Oct 1, 2019
@pushcx
Copy link
Member

pushcx commented Oct 1, 2019

Deployed.

@thomasdziedzic thomasdziedzic deleted the csp branch October 2, 2019 00:40
pushcx pushed a commit that referenced this pull request Jan 7, 2022
@thomasdziedzic @pushcx
csp (attempt 2) (#738)
adfc22f 
* enforce content security policy (#711)
* allow connect-src from self, fixes #736
* enable unsafe_eval so that previewing story submission works
* turn back report only mode for now
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@thomasdziedzic @pushcx