Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix error preventing activity log diff #411

Merged
merged 1 commit into from
Feb 8, 2024
Merged

Conversation

crowesn
Copy link
Contributor

@crowesn crowesn commented Nov 30, 2023

Resolves #410

@version.changeset was returning an empty hash due to YAML serialization permitted class exceptions, restrictions introduced after an RCE escalation risk was discovered. Examining the deserializer in the paper_trail gem showed that we needed to allow ActiveSupport::TimeWithZone, Time, ActiveSupport::TimeZone classes for the changes to load.

Debug here: https://github.com/paper-trail-gem/paper_trail/blob/master/lib/paper_trail/version_concern.rb#L357

Related: https://discuss.rubyonrails.org/t/cve-2022-32224-possible-rce-escalation-bug-with-serialized-columns-in-active-record/81017
And: https://stackoverflow.com/a/72970171

Changes

  • Allow ActiveSupport::TimeWithZone, Time, ActiveSupport::TimeZone classes for yaml load in config/application.rb
  • Add rspec helper class provided by PaperTrail
  • Add controller test to activity controller to ensure that diff data is being populated to the changeset
  • Add feature test to check for diff strings in activity details show page.
  • Upgrade PaperTrail to v.15

Note that PaperTrail/versioning is now turned off for specs by default. To include versioning functionality in specs you need to activate in the test block.

@hortongn
Copy link
Member

@crowesn When this PR is ready, please assign it to me and I'll review.

@crowesn crowesn force-pushed the 410/activity-log-diff branch 3 times, most recently from 66dfd65 to 16e8f2e Compare November 30, 2023 20:39
@crowesn crowesn changed the title WIP Fix error preventing activity log diff Fix error preventing activity log diff Nov 30, 2023
@crowesn crowesn assigned crowesn and hortongn and unassigned crowesn Nov 30, 2023
@crowesn
Copy link
Contributor Author

crowesn commented Dec 1, 2023

eh hold of on this for a sec I want to add a controller test for the activity log.

@crowesn crowesn force-pushed the 410/activity-log-diff branch 3 times, most recently from f87b6af to 89d891b Compare December 5, 2023 16:29
@crowesn crowesn force-pushed the 410/activity-log-diff branch from 89d891b to e753389 Compare December 5, 2023 16:47
@crowesn
Copy link
Contributor Author

crowesn commented Dec 5, 2023

@hortongn

This should be ready to rock. I added an rpsec helper class packaged with paper trail as well as a controller test to make sure the version changes is populated with diff data.

@crowesn crowesn requested a review from hortongn February 5, 2024 15:10
Copy link
Member

@hortongn hortongn left a comment

Choose a reason for hiding this comment

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

@crowesn Nice work on this!

@hortongn hortongn merged commit 7209a5d into qa Feb 8, 2024
@hortongn hortongn deleted the 410/activity-log-diff branch February 8, 2024 18:54
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Activity log diffs not showing
2 participants