Skip to content

Conversation

@lizkenyon
Copy link
Contributor

@lizkenyon lizkenyon commented Dec 5, 2025

Description

  • The Session.deserialize method used Oj.load without safe mode, which allows instantiation of arbitrary Ruby objects.
  • These methods were vestigial code from when the library handled session storage (deprecated in v12.3.0). After that deprecation, apps became responsible for their own session persistence, rendering serialize/deserialize unnecessary for their original purpose
  • Investigation confirmed no external usage - the shopify_app gem stores individual session attributes in database columns and reconstructs sessions using Session.new()
  • The only internal usage was copy_attributes_from, which called serialize just to enumerate attribute names via JSON.parse(other.serialize).keys before copying instance variables. This has been refactored to directly copy each
    attribute, eliminating the dependency on serialize.

Fixes #

Please, include a summary of what the PR is for:

  • What is the problem it is solving?
  • What is the context of these changes?
  • What is the impact of this PR?

How has this been tested?

Please, describe the tests that you ran to verify your changes.

Checklist:

  • My commit message follow the pattern described in here
  • I have performed a self-review of my own code.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have updated the project documentation.
  • I have added a changelog line.

@lizkenyon lizkenyon requested a review from a team as a code owner December 5, 2025 16:43
@lizkenyon lizkenyon force-pushed the liz/remove-session-serialize-deserialize branch from 0853372 to 8f3ccbf Compare December 5, 2025 16:50
   The Session.deserialize method used Oj.load without safe mode, which allows
   instantiation of arbitrary Ruby objects. If an attacker could control session
   storage (e.g., compromise a Redis instance or database), they could inject
   malicious serialized data to achieve remote code execution.

   These methods were vestigial code from when the library handled session
   storage (deprecated in v12.3.0). After that deprecation, apps became
   responsible for their own session persistence, rendering serialize/deserialize
   unnecessary for their original purpose. Investigation confirmed no external
   usage - the shopify_app gem stores individual session attributes in database
   columns and reconstructs sessions using Session.new().

   The only internal usage was copy_attributes_from, which called serialize just
   to enumerate attribute names via JSON.parse(other.serialize).keys before
   copying instance variables. This has been refactored to directly copy each
   attribute, eliminating the dependency on serialize.

   Breaking change: Session#serialize and Session.deserialize removed.
   Migration: Apps should use Session.new() to reconstruct sessions from stored
   attributes (the pattern already used by shopify_app).

   Complete removal eliminates the RCE vector entirely while maintaining all
   functionality.
@lizkenyon lizkenyon force-pushed the liz/remove-session-serialize-deserialize branch from 8f3ccbf to d23b611 Compare December 5, 2025 17:04
@lizkenyon lizkenyon merged commit 2a25d93 into main Dec 5, 2025
10 of 21 checks passed
@lizkenyon lizkenyon deleted the liz/remove-session-serialize-deserialize branch December 5, 2025 20:41
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.

2 participants