Update secure_headers from 3.7.3 to 6.0.0

[monfresh]

Hi! Before submitting your PR for review, and/or before merging it, please
go through the following checklist:

• For DB changes, check for missing indexes, check to see if the changes
  affect other apps (such as the dashboard), make sure the DB columns in the
  various environments are properly populated, coordinate with devops, plan
  migrations in separate steps.

• For route changes, make sure GET requests don't change state or result in
  destructive behavior. GET requests should only result in information being
  read, not written.

• For encryption changes, make sure it is compatible with data that was
  encrypted with the old code.

• For secrets changes, make sure to update the S3 secrets bucket with the
  new configs in all environments.

• Do not disable Rubocop or Reek offenses unless you are absolutely sure
  they are false positives. If you're not sure how to fix the offense, please
  ask a teammate.

• When reading data, write tests for nil values, empty strings,
  and invalid formats.

• When calling redirect_to in a controller, use _url, not _path.

• When adding user data to the session, use the user_session helper
  instead of the session helper so the data does not persist beyond the user's
  session.

• When adding a new controller that requires the user to be fully
  authenticated, make sure to add before_action :confirm_two_factor_authenticated.

[davemcorwin]

LGTM. It looks like some of our configuration is now the default so they don't need to be there, but it doesn't look like there will be any issues.

We also may want to look at where we can use the extra nonced_ tag helpers for including js.

[mryenq]

@davemcorwin The failing spec can be toggled off and on by reverting to the previously-used version (3.7.3), and back again.

Initially it appeared to be related to block-all-mixed-content, as this was the only difference in headers output between the two versions, but completely removing or setting this to false did not correct the issue, though it did seem to be a possible mixed-content scenario related to the use of example.com (as @monfresh has discussed with the library author before:. And in fact page.body in this portion of the failing spec contains the content of this site.

However the culprit appears to be the form-action directive (also discussed in the above issue), and modifying this to form_action: ["'self'", "http://*.example.com"] strictly as a test appears to work, and has the failing spec passing again. This existed in the past and was removed, and it may be more helpful to have @monfresh review again, given the background and history of the issue.

If we like this fix, we could possibly make it environment-specific, as we're currently doing with some other production-specific directives...

[davemcorwin]

thank you!

…

On Wed, May 23, 2018 at 2:32 PM, Michael Ryan ***@***.***> wrote: @davemcorwin <https://github.com/davemcorwin> The failing spec can be toggled off and on by reverting to the previously-used version (3.7.3), and back again. Initially it appeared to be related to block-all-mixed-content, as this was the only difference in headers output between the two versions, but completely removing or setting this to false did not correct the issue, though it did seem to be a possible mixed-content scenario related to the use of example.com (as @monfresh <https://github.com/monfresh> has discussed with the library author before: <github/secure_headers#221>. And in fact page.body in this portion of the failing spec contains the content of this site. However the culprit appears to be the form-action directive (also discussed in the above issue), and modifying this to form_action: ["'self'", "http://*.example.com"] strictly as a test appears to work, and has the failing spec passing again. This existed in the past and was removed, and it may be more helpful to have @monfresh <https://github.com/monfresh> review again, given the background and history of the issue. If we like this fix, we could possibly make it environment-specific, as we're currently doing with some other production-specific directives... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2175 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGWo4_NKBifXTV0py2N3kRPDLJV6z2-5ks5t1dVygaJpZM4UCUqt> .

-- David Corwin 18F <https://18f.gsa.gov/> | Consulting Engineer david.corwin@gsa.gov

[monfresh]

Your analysis is correct, but the plot is thicker. Here is my detective report.

This is not related to block-all-mixed-content and this is not the same issue that I had brought up on the secure_headers repo. There isn't mixed content in the specs since HTTPS is not used in tests.

The change that is causing the test to fail happened between version 6.0.0.alpha02 and 6.0.0.alpha03 and is due to removing the user agent sniffing code here: github/secure_headers#390

With the user agent parsing code, the HeadlessChrome User Agent is being interpreted by the secure_headers gem as a browser that does not support the form-action directive, which allows the test to pass (because the form-action directive is not present in the CSP headers). I verified this using the middleware linked to in this blog post, which allows us to inspect the response headers (Selenium doesn't have support for page.response_headers).

In the latest version of secure_headers, the form-action directive is not removed, and the reason the test fails is because this is a JS test, and we have configured all JS tests to use 127.0.0.1 as the domain name, but in the last part of the test, when the user clicks on the button in the modal to cancel account creation and delete their account, it tries to go to http://www.example.com/sign_up/start.

This is because the UsersController determines where to redirect based on the cancel_link_url of the decorated_session, which in this case is the ServiceProviderDecoratedSession, whose cancel_link_url is constructed using Rails.application.routes.url_helpers, which, for some reason, ignores the default_url_options defined in ApplicationController and defaults the host to www.example.com, and since the form-action directive doesn't allow example.com, the test fails to redirect. Note that the defaulting to www.example.com only happens in the test environment. I verified this works fine in production.

I verified this by replacing redirect_to url_after_cancellation with redirect_to sign_up_start_url in UsersController, and the test passes. So then I tried passing in the host parameter to sign_up_start_url in the decorator, but that didn't work because Capybara uses a port with 127.0.0.1, but the sign_up_start_url in the decorator doesn't include the port, and so the form-action directive will not allow this redirect. The port is random each time the test is run, so there's no obvious way to set the port, and Capybara.always_include_port = false didn't work.

So then I looked at the decorator class some more, and noticed the view_context that gets passed to it and it struck me: we should be using view_context.sign_up_start_url instead of including Rails.application.routes.url_helpers, and 🎉!