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

Allow the v2 encryptor to serialize messages with Marshal #44

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jcmfernandes
Copy link
Contributor

@jcmfernandes jcmfernandes commented Aug 9, 2024

This is a follow-up on #39 that must be merged first.

I deliberately avoided allowing Marshal serialization in the v2 encryptor because 1) Marshal has been a source of RCE vulnerabilities and 2) serializing messages as JSON allows for better interoperability, making it trivial to access sessions created with rack-session in other languages.

However, it's impossible to swap Marhal with JSON without breaking users' code that relies on Marhal behavior that JSON doesn't mimic (e.g., preserving ruby symbols).

So... I see 3 ways forward:

  1. We close this PR and release yet another major
  2. We close this PR and make v2 opt-in instead of the default
  3. We merge this PR

Happy to hear some feedback.

jcmfernandes and others added 5 commits February 13, 2024 09:25
This broke with the release of rack 3.1.x.
Marshal and JSON serialization are unavoidably different. A key
difference being that JSON serializes ruby symbols as strings, while
Marshal preserves them as symbols.
@ioquatix
Copy link
Member

ioquatix commented Aug 11, 2024

I've merger your other PR.

I'm a big fan of symbolic keys if the keys don't represent arbitrary user data, so I'd support the JSON decoder using symbolize_names or similar approaches.

I think we can gracefully introduce support using a flag or option, or deprecation warnings, or as you say, a major version bump.

Do you mind rebasing on main?

@jcmfernandes jcmfernandes changed the title Allow the v2 encrytor to serialize messages with Marshal Allow the v2 encryptor to serialize messages with Marshal Aug 12, 2024
@ioquatix
Copy link
Member

Just touching base here - any chance you are able to rebase on main?

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