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

Fixes #1329, #1328. Add a CSP reporting endpoint and move security headers into app. #1367

Merged
merged 5 commits into from
Mar 2, 2017

Conversation

miketaylr
Copy link
Member

This will report CSP violations to /tmp/webcompat-csp-reports.log (and DevTools also will dump them for the page you're on). Let's let this run wild for a week or so then figure out what needs to change.

(I already noticed the following, which points to lodash):

[Report Only] Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'self' https://www.google-analytics.com".

r? @karlcow

return
with open(app.config['CSP_REPORTS_LOG'], 'a') as r:
r.write(request.data + '\n')
return ('', 204)

This comment was marked as abuse.

@miketaylr
Copy link
Member Author

(just pushed 2 simple tests and a return value for the non-correct content-type case)

Copy link
Member

@karlcow karlcow left a comment

Choose a reason for hiding this comment

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

Some light comments, but probably would love

  • the request.headers.get
  • the function for headers
  • the comments for the tests so "nosetests -v" is more readable.



@app.route('/csp-report', methods=['POST'])
def log_report():

This comment was marked as abuse.

This comment was marked as abuse.

@app.route('/csp-report', methods=['POST'])
def log_report():
'''Route to record CSP header violations.'''
if 'application/csp-report' not in request.headers.get('content-type'):

This comment was marked as abuse.

This comment was marked as abuse.

if 'application/csp-report' not in request.headers.get('content-type'):
return ('Wrong Content-Type.', 400)
with open(app.config['CSP_REPORTS_LOG'], 'a') as r:
r.write(request.data + '\n')

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@@ -51,6 +51,21 @@ def before_request():
@app.after_request
def after_request(response):
session_db.remove()
# Security headers

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

return ('Wrong Content-Type.', 400)
with open(app.config['CSP_REPORTS_LOG'], 'a') as r:
r.write(request.data + '\n')
return ('', 204)

This comment was marked as abuse.

@@ -93,6 +93,17 @@ def test_labeler_webhook(self):
# A random post should 401, only requests from GitHub will 200
self.assertEqual(rv.status_code, 401)

def test_csp_report_uri(self):

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

rv = self.app.post('/csp-report', headers=headers)
self.assertEqual(rv.status_code, 204)

def test_csp_report_uri_bad_content_type(self):

This comment was marked as abuse.

This comment was marked as abuse.

@karlcow
Copy link
Member

karlcow commented Feb 28, 2017

nosetests -v

…
test_csp_report_uri (tests.test_urls.TestURLs) ... ok
test_csp_report_uri_bad_content_type (tests.test_urls.TestURLs) ... ok
…

@miketaylr
Copy link
Member Author

miketaylr commented Feb 28, 2017

Have addressed all feedback except the CSP_LOG part -- will get to that soon.

(note to self: link to comment #1367 (comment))

This branch has conflicts that must be resolved

Looks like I need to rebase and fix conflicts as well.

done.

Mike Taylor added 4 commits February 28, 2017 11:45
Note: we don't set STS because localhost is not served over TLS.
Fixes #1328.
This allows xhr, fonts, images, scripts, css from webcompat.com (or localhost).
It also allows script from google-analytics.com. Let's leave it on for a week or
so and see what we need to tweak before enabling the policy (and where to file bugs
to improve security).
@miketaylr
Copy link
Member Author

@karlcow ok, let's go back to the original log file.

r? for c8065f7

@karlcow
Copy link
Member

karlcow commented Mar 2, 2017

ok r+ @miketaylr

@miketaylr
Copy link
Member Author

Thanks @karlcow.

@zoepage zoepage deleted the issues/1329/1 branch March 3, 2017 19:55
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