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

Fix the issue that unencrypted plaintext values are versioned with ActiveRecord encryption (since Rails 7) #1422

Merged

Conversation

FunnyHector
Copy link
Contributor

Since Rails 7, ActiveRecord introduces a built-in encryption mechanism. When versioning encrypted attributes for JSON columns on PostgreSQL, currently the unencrypted values are saved. This makes the encryption meaningless, and stops people from upgrading to Rails 7.

This PR is mainly from the patch that @vccoffey posted at #1392 (comment). Thanks for the original idea.

Fixes #1392.

Check the following boxes:

  • Wrote [good commit messages][1].
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new
    code introduces user-observable changes.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.

…module method

There will be multiple places calling this method to determine different
behaviours based on the ActiveRecord version that PaperTrail is running
on.

PR: paper-trail-gem#1422
FunnyHector pushed a commit to FunnyHector/paper_trail that referenced this pull request Mar 24, 2023
Since Rails 7, ActiveRecord introduces a built-in encryption mechanism.
We need to serialise these values otherwise plaintext values instead of
ciphertext are versioned, which makes the encryption meaningless.

PR: paper-trail-gem#1422
@FunnyHector FunnyHector force-pushed the active-record-encryption-fix branch from 578eaac to 8f13bfc Compare March 24, 2023 00:56
Since Rails 7, ActiveRecord introduces a built-in encryption mechanism.
We need to serialise these values otherwise plaintext values instead of
ciphertext are versioned, which makes the encryption meaningless.

PR: paper-trail-gem#1422
@FunnyHector FunnyHector force-pushed the active-record-encryption-fix branch from 8f13bfc to c765492 Compare March 24, 2023 00:57
No uncovered lines were added, but more lines were added therefore the
denominator got bigger and the coverage dropped a little ¯\_(ツ)_/¯

PR: paper-trail-gem#1422
@FunnyHector FunnyHector marked this pull request as ready for review March 24, 2023 01:18
@FunnyHector
Copy link
Contributor Author

@jaredbeck Sorry for pinging, could this be looked at please? I imagine this would be stopping lots of users from upgrading to Rails 7.

Copy link
Member

@jaredbeck jaredbeck left a comment

Choose a reason for hiding this comment

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

Nice work, Hector.

end
end
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Nice tests

# ActiveRecord since 7.0 has a built-in encryption mechanism
@encrypted_attributes =
if PaperTrail.active_record_gte_7_0?
@item_class.encrypted_attributes&.map(&:to_s)
Copy link
Member

Choose a reason for hiding this comment

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

Surprisingly, it seems that rails does not initialize encrypted_attributes to an empty array, and so the safe-navigation operator is warranted.

@jaredbeck jaredbeck merged commit 3d37fd4 into paper-trail-gem:master May 21, 2023
@FunnyHector FunnyHector deleted the active-record-encryption-fix branch May 22, 2023 23:49
@FunnyHector
Copy link
Contributor Author

@jaredbeck Thanks for reviewing and merging this! Could I ask about the plan for the new release please? We have been using a monkey patch in production. It'd be good if we could get off the monkey patch and use a released version.

@TastyPi
Copy link

TastyPi commented Jul 20, 2023

@FunnyHector @jaredbeck could we get a new release including this fix? We need it, I could reference the git commit we want temporarily but I'd prefer official releases 🙂

@jaredbeck
Copy link
Member

Released in 15.0.0

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.

Store encrypted values on object_changes column
3 participants