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

Remove Activerecord session store #798

Merged
merged 4 commits into from
Mar 9, 2021
Merged

Conversation

zurbergram
Copy link
Contributor

@zurbergram zurbergram commented Mar 9, 2021

Description

GHSA-cvw2-xj8r-mjf7

Removing https://github.com/rails/activerecord-session_store as potential fix in that repo is taking a while to go out
And this is blocking #796

Original issue(s)

department-of-veterans-affairs/va.gov-team#0000

Testing done

Screenshots

Acceptance criteria

  • [ ]

Definition of done

  • Events are logged appropriately
  • Documentation has been updated, if applicable
  • A link has been provided to the originating GitHub issue (or connected to it via ZenHub)
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs

@zurbergram zurbergram requested a review from a team as a code owner March 9, 2021 19:02
@zurbergram zurbergram requested review from ericboehs and a team March 9, 2021 19:02
@zurbergram zurbergram added BAH Booz Allen Hamilton BAH-IR-needed BAH internal code review needed BAH-QA-needed BAH internal QA review needed labels Mar 9, 2021
@LindseySaari
Copy link
Contributor

Can the temporary recommended fix be implemented instead?

@zurbergram
Copy link
Contributor Author

zurbergram commented Mar 9, 2021

Can the temporary recommended fix be implemented instead?

I tried that as well and received below after doing bundle clean --force and bundle install

Name: activerecord-session_store
Version: 1.1.3
Advisory: CVE-2019-25025
Criticality: Unknown
URL: https://github.com/advisories/GHSA-cvw2-xj8r-mjf7
Title: activerecord-session_store Timing Attack
Solution: remove or disable this gem until a patch is available!

Ok, just want to make sure theres no implications from crossing over to cookie store vs session store. Will this cause any hiccups with currently logged in users? After a fix is implemented, do you plan to switch back to session store? If this is a permanent switchover, you may consider removing the session table?

@zurbergram
Copy link
Contributor Author

There are only a handful of users who use the gids dashboard and if an upload fails they will most likely attempt to re-upload the CSV file. After this change has been out there and no issues have come up we will clean up the database

@zurbergram
Copy link
Contributor Author

Looks like #106 added the gem originally

@LindseySaari
Copy link
Contributor

Ok sounds good. It looks like this post suggests some crossover solutions, but if only a handful of users will be logged out, it's not as high of a concern. You may want to send out a message to warn users though

@zurbergram zurbergram merged commit 2e5f3ce into master Mar 9, 2021
@zurbergram zurbergram deleted the activerecord-session-store branch March 9, 2021 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BAH Booz Allen Hamilton BAH-IR-needed BAH internal code review needed BAH-QA-needed BAH internal QA review needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants