Skip to content

Remove secure_headers gem (LG-6184)#6234

Merged
zachmargolis merged 3 commits intomainfrom
margolis-remove-secure-headers-gem
Apr 22, 2022
Merged

Remove secure_headers gem (LG-6184)#6234
zachmargolis merged 3 commits intomainfrom
margolis-remove-secure-headers-gem

Conversation

@zachmargolis
Copy link
Contributor

Previous PRs have added replacement functionality for the things we care about from this gem and added regression specs:

I am going to deploy this to my personal environment to make sure it doesn't break using the acuant SDK on mobile

@zachmargolis
Copy link
Contributor Author

@mitchellhenke @aduth testing in my personal environment, I got some CORS errors... can one of you confirm my understanding that it's probably not related to these changes? Since the headers need to be set on the resource, so on CDN/S3 side instead of the IDP... something else in my environment must be off?

Screen Shot 2022-04-21 at 3 40 21 PM

@zachmargolis
Copy link
Contributor Author

Confirmed that, other than above font issue, this works in a deployed environment with the mobile SDK

**Why**: Tests run without ActionDispatch::SSL middleware so we test it directly
changelog: Internal, Dependencies, Remove secure_headers gem to leverage newer Rails features
_status, headers, _body = ssl_middleware.call(request)

expect(headers['Strict-Transport-Security']).
to eq('max-age=31556952; includeSubDomains; preload')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. this is slightly different than the 31536000 is the old test, but I figured it was close enough
  2. I create this new test here, since the SSL middleware is not added to tests by default, the old request specs didn't exercise it. I couldn't figure out a nice way to add the SSL middleware for just that test, so I went with this approach of just testing the middleware directly with our app's config

@aduth
Copy link
Contributor

aduth commented Apr 22, 2022

testing in my personal environment, I got some CORS errors... can one of you confirm my understanding that it's probably not related to these changes? Since the headers need to be set on the resource, so on CDN/S3 side instead of the IDP... something else in my environment must be off?

Seems similar to an issue I'd once reported on Slack that couldn't be repro'd by others. FWIW, I couldn't see any CORS errors while navigating your sandbox. But yeah, I would sorta assume it's something configured on the CDN side of things.

@zachmargolis zachmargolis merged commit db4c94a into main Apr 22, 2022
@zachmargolis zachmargolis deleted the margolis-remove-secure-headers-gem branch April 22, 2022 15:37
@jmdembe jmdembe mentioned this pull request Apr 26, 2022
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.

5 participants