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

Switch Content Security Policy from report-only to enforceable #498

Merged
merged 1 commit into from Dec 17, 2017

Conversation

Changaco
Copy link
Member

@Changaco Changaco commented Jan 5, 2017

In #468 we opted to start with a report-only policy. This PR is so we don't forget to switch to enforcement mode. I want to wait a little while longer and analyze the reports before merging this.

@Changaco Changaco added the defense protecting ourselves, our users and innocent third-parties label Jan 5, 2017
@Changaco
Copy link
Member Author

Changaco commented Jan 6, 2017

From the CSP reports we've received so far it looks like we need to change 'self' to liberapay.com (actually website.env.canonical_host) for the i18n subdomains to be able to load the assets from the top domain.

@Changaco
Copy link
Member Author

I'm seeing reports for script-src eval. Mozilla's wiki says that in addition to the eval() function itself, four other things are blocked when unsafe-eval isn't allowed. Searching for them in our code I see that Function() is used in the old version of jQuery we're using, so I'm going to upgrade that.

@Changaco
Copy link
Member Author

I'm also seeing reports for block-all-mixed-content, due to images included in profile statements. Fixing that requires an image proxy (#504).

@strugee
Copy link

strugee commented Nov 28, 2017

@Changaco FWIW you can send Content-Security-Policy and Content-Security-Policy-Report-Only at the same time, so you should be able to set everything but block-all-mixed-content to enforcing and keep that one as reporting: https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP#Testing_your_policy

@Changaco
Copy link
Member Author

I've dropped block-all-mixed-content since we already use upgrade-insecure-requests anyway.

I've also added 'self' back to the *-src directives just in case.

@Changaco
Copy link
Member Author

I'll merge this tomorrow unless there are objections. Ping @EdOverflow.

b"script-src %(main_domain)s 'unsafe-inline';"
b"style-src %(main_domain)s 'unsafe-inline';"
b"default-src 'self' %(main_domain)s;"
b"script-src 'self' %(main_domain)s 'unsafe-inline';"
Copy link
Member

Choose a reason for hiding this comment

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

The comment above this section of code suggests that we are using this CSP to prevent XSS, but by using 'unsafe-inline' you are doing precisely the opposite.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

We're not ready to drop 'unsafe-inline'. It's still better to have some protection than none at all.

Copy link
Member

Choose a reason for hiding this comment

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

@Changaco, I agree, but is there a ticket addressing this concern somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is now: #879.

@Changaco Changaco mentioned this pull request Dec 16, 2017
5 tasks
@Changaco Changaco merged commit 6065c12 into master Dec 17, 2017
@Changaco Changaco deleted the csp-enforce branch December 17, 2017 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defense protecting ourselves, our users and innocent third-parties
Development

Successfully merging this pull request may close these issues.

None yet

3 participants