Set Rails 7.0 cookies_serializer default value to :hybrid

[nbcraft]

Proposing this change as a discussion medium after I experienced troubles upgrading to Rails 7.

Summary

:hybrid is the value set for cookies_serializer in new_framework_defaults_7_0.rb.tt.

This and the value in railties/lib/rails/application/configuration.rb should match, so that when removing new_framework_defaults_7_0.rb and setting config.load_defaults 7.0 in one's application config, we still have the same behaviour.

if we don't want to change railties/lib/rails/application/configuration.rb, maybe the value in new_framework_defaults_7_0.rb.tt should be changed to :json instead, but then suggest (in the documentation) to set it to :hybrid manually in the project's config/application.rb file instead.

Context for this change:

When upgrading my app to Rails 7, I followed the guide, and:

• Enabled all Rails 7 framework defaults in new_framework_defaults_7_0.rb and added a cookie_rotator initializer, while keeping config.load_defaults 6.1.
  • Tested app locally and ran specs, all was fine, and assumed I could move forward with the following:
• Removed new_framework_defaults_7_0.rb and set config.load_defaults 7.0 (which I assumed would have the same behaviour as the above).
• This broke all my current sessions because that changed the cookies_serializer value from :hybrid to :json, which was not backward compatible.

Question:

Was I meant to:

• First deploy a version of the app with config.load_defaults 6.1 and all values enabled in new_framework_defaults_7_0.rb
• And only then in a secondary deploy remove new_framework_defaults_7_0.rb and set config.load_defaults 7.0 ?

Or was it okay to see that my application was running fine locally in the first state (all values enabled in new_framework_defaults_7_0.rb) and assume I could replace that by config.load_defaults 7.0 and go straight for that instead.

If I was meant to do the two deploys, maybe the upgrade guide should specifically mention that instead of this:

this can be done gradually over several deployments

Which to me made it sound like it wasn't a requirement.

[ghiculescu]

Hey, thanks for the report. Really sorry to hear this caused problems for you. I guess I'm responsible for that.

I think there's two issues here. One is around the cookies serializer default. The other is around the upgrade guide.

Cookies serializer default

Was I meant to:

• First deploy a version of the app with config.load_defaults 6.1 and all values enabled in new_framework_defaults_7_0.rb
• And only then in a secondary deploy remove new_framework_defaults_7_0.rb and set config.load_defaults 7.0 ?

Yeah, kind of. More specifically you should:

1. Use :hybrid to convert marshal cookies to JSON cookies, iff you had not set a cookies_serializer previously.
2. Once all cookies have converted, optionally switch to :json.

The :hybrid serializer converts :marshal cookies into :json cookies. See here and here. So if you don't do step 2 it should work the same way. And if you do step 2 too soon, I guess it could also cause problems.

Can you confirm if you had set a cookies_serializer previously? You would have if you had an initializer like this. But based on what broke I'm guessing you didn't.

I've had a go at making the explanation for this more clear #45137

---

Upgrade guide

If I was meant to do the two deploys, maybe the upgrade guide should specifically mention that instead of this:

this can be done gradually over several deployments

Which to me made it sound like it wasn't a requirement.

What this is saying is that you don't need to uncomment all the defaults in the same deployment. eg. you could flip smtp_timeout in one deployment and executor_around_test_case in another one.

What you should do is:

• First deployment: use the initial new_framework_defaults_7_0.rb (with everything commented out still... technically you can enable some defaults in this deployment too but I like to be conservative and split that up)
• n deployments: gradually switch on every new default you want
• Last deployment: remove new_framework_defaults_7_0.rb, set config.load_defaults 7.0, override any new defaults you don't want (eg you may want to keep running wrap_parameters_by_default = false)

Does this make sense? I think we could certainly make this more clear on the guide.

btw, reasons splitting over multiple deploys (as opposed to multiple commits) is necessary:

• It makes it easier to roll back if a particular deploy is problematic.
• Some settings require you to be in a state where you won't be rolling back, before switching over. For example cache_format_version isn't backward compatible so once you change it, you can't safely downgrade to 6.1.

[nbcraft]

Hey Alex, thank you for your detailed response 🙏

Yeah, kind of. More specifically you should:

1. Use :hybrid to convert marshal cookies to JSON cookies, if you had not set a cookies_serializer previously.

Okay yeah, I wasn't sure from the new_framework_defaults_7_0 comment if :hybrid would actually convert existing cookies, or if it just meant that we'd start creating JSON cookies from now on, but I gave it a try and it does indeed convert existing ones 👍

So having that intermediate config loaded in production would've helped indeed. But, actually I think it might have hidden a deeper problem because:

• Our most active customers' cookies would have had their cookies converted if they used the app often, but:
• Our less active customers might not have used the app between the deploy using :hybrid and the deploy using config.load_defaults 7.0, meaning it would've been broken for them once they got back to our app later on, and it wouldn't fix itself either, it would require them to clear their cookies.

And that would've been a whole lot harder to track down.

2. Once all cookies have converted, optionally switch to :json.

The problem is that by moving from the new_framework_defaults_7_0 over to config.load_defaults 7.0, we're effectively switching from :hybrid to :json, and it is hard to realize that as you would except that the values in new_framework_defaults_7_0 and rails/application/configuration.rb#load_defaults to all be the same.
Isn't that the goal of those files? Every other configs have the same values in both.

So even though it's optional, it's being done implicitly in the background when doing that step of the upgrade process.

Can you confirm if you had set a cookies_serializer previously?

I can confirm we did not have one previously. So we were most likely using the default :marshal prior to this upgrade. Hence why we had deserialization errors as soon as we started using config.load_defaults 7.0:
JSON::ParserError: 859: unexpected token at '{I"session_id:ETI"%bc7c45e008b3<...>;

Upgrade guide
What this is saying is that you don't need to uncomment all the defaults in the same deployment. eg. you could flip smtp_timeout in one deployment and executor_around_test_case in another one.
...
Does this make sense? I think we could certainly make this more clear on the guide.

Thanks, your explanation makes sense 👍 And I did understand most of that.

But I also thought at the time that it wasn't discouraged to go straight to config.load_defaults 7.0 if everything was working fine with all directives turned on in new_framework_defaults_7_0.rb.
That's what I'm saying could use a mention that you should have at least one deployment with all of new_framework_defaults_7_0 directives un-commented.

If that's indeed what we want to do, because I do get the sense that new_framework_defaults_X_Y files are meant to reflect the values in load_defaults X.Y (and so did my teammates), but maybe I'm misunderstanding the philosophy for these files?

So, to summarize my argument for still supporting this change:

• It feels like the philosophy of new_framework_defaults_X_Y files are meant to reflect the values in load_defaults X.Y.
  • But it's not the case with the cookies_serializer config which is different in both files for 7_0
  • If that's a wrong assumption, I feel this could be made clearer in the configure-framework-defaults section.
• For cookies_serializer: :hybrid to convert all existing cookies to JSON, all those cookies need to be loaded once during the timeframe between using new_framework_defaults_7_0 fully un-commented and moving over to config.load_defaults 7.0
  • We all know that sometimes some users might not use your app for months at a time.
  • Switching over to config.load_defaults 7.0 before all users have visited, means the app will be broken for those casual users next time they come by, and the only fix will be to manually clear their cookies themselves.

Thanks for your time and for considering this 🙏

PS: I'm sorry if I pinged a hundred people on this PR, I thought I should maybe change the base branch to 7.0-stable and it did this automatically, my bad. Reverted since.

[ghiculescu]

So having that intermediate config loaded in production would've helped indeed. But, actually I think it might have hidden a deeper problem because...

Yeah that's correct. That's why in #45137 I make the warning around this a bit more stern. It's safer to just use :hybrid for a long time. Which leads into...

So, to resume my argument for still supporting this change:

I think it's a fair point. I think the general vibe is to not change defaults once they are live, but we could change it again in 7.1. That said I'm not on the core team, and wouldn't mind getting a second opinion from someone with more experience on this as it is a fairly complex issue. (I think we might have more luck with that post-railsconf - I'll follow up next week if necessary.)

[nbcraft]

It's safer to just use :hybrid for a long time.
I think the general vibe is to not change defaults once they are live, but we could change it again in 7.1

Ah right, I forgot to mention that in my last message, my thought on this was:

• Would be good to have :hybrid as default in 7.0
• And then move to :json in 7.1

Hopefully that would give existing applications enough time under :hybrid before switching over to :json.
But maybe that's wishful thinking, people late to upgrade to 7.X might just execute 7.0 and 7.1 upgrades successively and then hit that issue 🤔
Maybe move to :json in Rails 8 ? 🙈 Not sure what the ideal solution is here.

[p8]

Hey @nbcraft, sorry this broke you sessions.

Brakeman has been recommending using the :json serializer for 3 years.
I don't know if you run Brakeman against your application, but it's great at warning for possible vulnerabilities.

Maybe move to :json in Rails 8

:hybrid still deserializes Ruby objects, so it's still a vulnerability (although unlikely).

[ghiculescu]

Brakeman has been recommending using the :json serializer for 3 years.

This only works if you set the cookies_serializer setting somewhere. I think the issue here is if you didn't have the setting/initializer at all - either because you created your Rails app before 4.1 or because you deleted it. (I'm not saying it's bad advice, just that it wouldn't have caught this.)

[p8]

This only works if you set the cookies_serializer setting somewhere.

Ah yes, it wouldn't work before you added it to the framework defaults 👍 .

[nbcraft]

I don't know if you run Brakeman against your application, but it's great at warning for possible vulnerabilities.

I didn't know about it, thank you, I'll give it a go 👍

This only works if you set the cookies_serializer setting somewhere. I think the issue here is if you didn't have the setting/initializer at all - either because you created your Rails app before 4.1 or because you deleted it.

Yeah we started our app with Rails 6.0.3 and config.load_defaults 6.0, no cookie-specific config or cookie-specific initializer in our initial commit.

I don't know if that was the default state of a Rails 6.0 app, or if something about cookies was done on purpose on our end, but I would think it's the default state because there's really nothing else in that commit, pretty bare-bone project initialization.

Also I ran git log --all --full-history -- "**/cookies_serializer.*", no dice.

So yeah it appears that we simply went ahead with the default config for Rails 6, which is set to :marshal, and Rails 7 changing the default config straight to :json seems to be a breaking change.

(As mentioned before we do go through the :hybrid value while running new_framework_defaults_7_0, but that is short lived)

[nbcraft]

@guilleiguaran Any thoughts on this one (since you merged #45137) ? Thank you 🙏

[skipkayhil]

I can confirm we did not have one previously. So we were most likely using the default :marshal prior to this upgrade. Hence why we had deserialization errors as soon as we started using config.load_defaults 7.0:
JSON::ParserError: 859: unexpected token at '{I"session_id:ETI"%bc7c45e008b3<...>;

Would it be possible to rescue JSON::ParserError in the :json deserializer and clear the cookie? That may be a more forgiving approach than errors requiring manually cookie clearing

[nbcraft]

Would it be possible to rescue JSON::ParserError in the :json deserializer and clear the cookie? That may be a more forgiving approach than errors requiring manually cookie clearing

That's a good additional fix as well 👍

It would still break existing sessions when moving over to :json too quickly though, which is why I think we should still keep :hybrid around for a bit.

[rafaelfranca]

We should not change the default to hybrid. New applications should be using json. If I got the problem, it happened because people thought the config inside the 7_0_defaults file could be deleted with all the configs and just swapping to load_defaults 7.0 would give the same behavior.

I think the right fix for this is to make clear in that file that hybrid isn't the 7.0 default and it should be kept as hybrid unless the application is ok with swapping to json.

[rafaelfranca]

Reading the comment, it seems we can clarify that if you want to keep hybrid, you need this config to a different file.

# If you're upgrading and haven't set `cookies_serializer` previously, your cookie serializer
# was `:marshal`. The default for new apps is `:json`.
#
# You can convert all cookies to JSON using the `:hybrid` formatter. It is fine to use
#`:hybrid` long term; you should do that unless you're confident that *all* your cookies
# have been converted to JSON.
#
# You can also use `:marshal` for backward-compatibility with old cookies.
#
# If you have configured the serializer elsewhere, you can remove this.
#
# See https://guides.rubyonrails.org/action_controller_overview.html#cookies for more information.
# Rails.application.config.action_dispatch.cookies_serializer = :hybrid

[ghiculescu]

If I got the problem, it happened because people thought the config inside the 7_0_defaults file could be deleted with all the configs and just swapping to load_defaults 7.0 would give the same behavior.

Should we also update https://github.com/rails/rails/blob/main/guides/source/upgrading_ruby_on_rails.md#configure-framework-defaults to reflect this? It's advised you to remove the file since the section was added in #33685.

[rafaelfranca]

I mean, you should delete the file, but not before reading its content and making sure you are using the configs you want your application to be using.

[rafaelfranca]

The doc says: Once your application is ready to run with new defaults, you can remove this file and flip the config.load_defaults value.

In this case, if you app is not using cookies_serializer = :json yet, you should consider it not ready to run with the new defaults.

[ghiculescu]

In this case, if you app is not using cookies_serializer = :json yet, you should consider it not ready to run with the new defaults.

Ahh, I see what you mean. I'm guessing when @nbcraft read this they interpreted "new defaults" as meaning whatever is in the file (:hybrid, not :json).

Made another docs PR to hopefully clarify further: #45172

[nbcraft]

(Thanks everyone for having a look into this 🙏 )

The doc says: Once your application is ready to run with new defaults, you can remove this file and flip the config.load_defaults value.

In this case, if you app is not using cookies_serializer = :json yet, you should consider it not ready to run with the new defaults.

The problem is the default value put in new_framework_defaults_7_0 is :hybrid, so you'd expect load_defaults 7.0 to use the same value. But it doesn't, and it's the only case where we have this discrepancy.

[nbcraft]

In this case, if you app is not using cookies_serializer = :json yet, you should consider it not ready to run with the new defaults.

Then new_framework_defaults_7_0 should use :json and mention using :hybrid in your own config if you need it.

But most people will instantly break with :json since they'll be upgrading from Rails 6.x which uses :marshal by default.

[nbcraft]

I mean, you should delete the file, but not before reading its content and making sure you are using the configs you want your application to be using.

I did read the file, and expected :hybrid to be the default value once going to load_defaults 7.0, which was fine by me.

This was the description then btw:

# If you're upgrading and haven't set `cookies_serializer` previously, your cookie serializer
# was `:marshal`. Convert all cookies to JSON, using the `:hybrid` formatter.
#
# If you're confident all your cookies are JSON formatted, you can switch to the `:json` formatter.
#
# Continue to use `:marshal` for backward-compatibility with old cookies.
#
# If you have configured the serializer elsewhere, you can remove this.
#
# See https://guides.rubyonrails.org/action_controller_overview.html#cookies for more information.
# Rails.application.config.action_dispatch.cookies_serializer = :hybrid

you can switch to the :json formatter

Which is not the same as: "Going to load_default 7.0 will switch you to :json"

@ghiculescu made the new version better though 👍 :

# If you're upgrading and haven't set `cookies_serializer` previously, your cookie serializer
# was `:marshal`. The default for new apps is `:json`.
#
# You can convert all cookies to JSON using the `:hybrid` formatter. It is fine to use
#`:hybrid` long term; you should do that unless you're confident that *all* your cookies
# have been converted to JSON.
#
# You can also use `:marshal` for backward-compatibility with old cookies.
#
# If you have configured the serializer elsewhere, you can remove this.
#
# See https://guides.rubyonrails.org/action_controller_overview.html#cookies for more information.
# Rails.application.config.action_dispatch.cookies_serializer = :hybrid

I still feel the proposed value (last line above) should reflect the value from load_default 7.0 (which is :json there)

The whole point of this file is to progressively check the new defaults will work properly before committing to load_default 7.0, and help you set your needed custom configs.

[rafaelfranca]

@nbcraft what do you think about the version we merged in #45172? If that isn't clear enough, do you see any way to improve further?

[nbcraft]

Looks good to me 👍
Thanks all for making this a better upgrade flow 🙏

[nbcraft]

Though this is still a worthwhile consideration I'd say:

Would it be possible to rescue JSON::ParserError in the :json deserializer and clear the cookie? That may be a more forgiving approach than errors requiring manually cookie clearing

[rafaelfranca]

@nbcraft I think that would be a good change, yes. Do you want to work on it?

[nbcraft]

I might if I can find the time. But if somebody else gets to it first, please mention it here 🙏

EDIT:

• This has been done here ActionDispatch::Cookies json deserializer discards marshal dumps #45956
• Should be included in rails >= 7.0.5