Skip to content

Add config to use Rails for secure headers options (LG-5771)#5952

Merged
zachmargolis merged 4 commits intomainfrom
margolis-headers-LG-5771
Feb 16, 2022
Merged

Add config to use Rails for secure headers options (LG-5771)#5952
zachmargolis merged 4 commits intomainfrom
margolis-headers-LG-5771

Conversation

@zachmargolis
Copy link
Contributor

The bulk of this PR is setting up regression specs so we can remove SecureHeaders gem confidently

The way SecureHeaders is implemented, it stomps on the default_headers config very late in the game: https://github.com/github/secure_headers/blob/ce2ad139646f9775061159fe5c4707638fbe45c1/lib/secure_headers/railtie.rb#L21-L31

so the only way to test this code was to comment out/remove secure_headers gem entirely and make sure the specs worked as expected

This does mean that this test should be good regression for when we do remove the gem entirely

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Re: Defaults:

  • The default for X-XSS-Protection ("0") doesn't match what we had. Do we want it to?
  • The default for X-Download-Options will be removed in Rails 7.1. Do we care to keep it set?

it 'includes Strict-Transport-Security (HSTS)' do
get root_path

pending 'seems to not get set in plain http tests'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we force it to request as https: ? Edit: Or is this also related to your original comment about secure_headers gem clobbering?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The clobbering was mostly related to the action_dispatch.default_headers this was me being lazy... but with some Googling I found that this works: 9e1077b

@@ -1,5 +1,15 @@
require 'feature_management'

if IdentityConfig.store.rails_csp_tooling_enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

To keep inline with the other calls in this file it should be if FeatureManagement.rails_csp_tooling_enabled?
which calls this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, will update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated: f06ead0

@zachmargolis
Copy link
Contributor Author

Re: Defaults:

  • The default for X-XSS-Protection ("0") doesn't match what we had. Do we want it to?
  • The default for X-Download-Options will be removed in Rails 7.1. Do we care to keep it set?

My original process was trying to create the minimal changes that preserve the behavior we have now. That approach meant that if we upgraded to Rails 7, or 7.1, the default headers would change and the specs would break.

Based on this comment, and to hopefully head off some future confusion, I added b621825 to keep what we now have as our default behavior in the future

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.

3 participants