Upgrade-safe URL-safe CSRF tokens

[etiennebarrie]

Summary

As mentioned in #18496 (comment), the change from that PR is problematic if you have zero-downtime deployments, because URL-safe Base64-encoded CSRF tokens generated by the new version of the app need to be decoded by the old version of the app still running at the time.

This PR introduces an application config which changes default with Rails 6.1, ensuring new apps do use URL-safe tokens, while apps upgrading from 6.0 have control over the moment they decide to turn on that behavior.

That way, once application owners know they won't revert to 6.0, they can toggle the config (and eventually just use the 6.1 defaults) and get URL-safe tokens.

Other Information

Here's a repro repo: https://github.com/etiennebarrie/rails-app/

$ git checkout urlsafe-csrf/6.0
$ bundle && rails s

Visit http://localhost:3000/ you get a Base64 strict token.

$ git checkout urlsafe-csrf/6.1.alpha
$ bundle && rails s

Without reloading, click the button, this succeeds and shows backward compatibility was working in that PR.
Make sure the new token you get has a - or _ to trigger the failure, otherwise refresh until you get one.

$ git checkout urlsafe-csrf/6.0
$ bundle && rails s

Without reloading, click the button, you should get an exception backtrace in the server and an alert in the browser.

Now to see the fix working:

$ git checkout urlsafe-csrf/fix
$ bundle && RAILS_DEFAULTS=6.1 rails s

Refresh until you get a token with a - or _, then

$ rails s

Without reloading, click the button, this time it succeeds.

---

I don't think this is a long-term option we want to offer, so once 6.1 ships, we should remove the code generating/decoding/encoding in strict Base64 encoding.

[etiennebarrie]

The other config_accessors in that file don't disable the instance_writer, but I didn't think it was worth being able to set this option in a controller, especially given it's not something we really want to keep long term in my opinion.

[Edouard-chin]

I don't know if it's a good idea but it will have the advantage of being completely transparent to the user without exposing a new flag:

When we encode the token with urlsafe, we disable the padding , which mean no = char should end up in the token. Can't we leverage that and do a simple if token.include?('=') ? Base64.strict_decode64 : Base64.urlsafe_decode64

[etiennebarrie]

When we encode the token with urlsafe, we disable the padding , which mean no = char should end up in the token. Can't we leverage that and do a simple if token.include?('=') ? Base64.strict_decode64 : Base64.urlsafe_decode64

Great idea, that would definitely work to avoid the flag, but we don't even need to discriminate, we can also just try to urlsafe-base64 decode and if it fails strict-base64 decode. (Side-note: I just realized that we can simply do urlsafe_decode64 because the way it's implemented, it turns the urlsafe token into a regular token with padding, then uses strict_decode64: https://github.com/ruby/ruby/blob/v2_7_1/lib/base64.rb#L98-L106 but I think that could break with alternative implementations, or if the implementation changes and doesn't go through that conversion step, and it's how the current backwards-compatibility works, strict encoded tokens still work thanks to this).

That's not the problem though, it's that the current code doesn't accept the url-safe tokens, so if we want to do this without a flag, we need to release a version that supports them first, then release the version that generates them.

Given https://edgeguides.rubyonrails.org/upgrading_ruby_on_rails.html#the-upgrade-process, users are expected to update to the latest patch before updating minor versions, that would mean supporting both encodings in 6.0.3, and generate URL-safe tokens in 6.1. That means we could just have a PR target 6_0_stable that handles both encodings, and not change master.

[Edouard-chin]

That's not the problem though, it's that the current code doesn't accept the url-safe tokens, so if we want to do this without a flag, we need to release a version that supports them first, then release the version that generates them.

You are right, I forgot that the problem was a forward compatibility issue not backward.

That means we could just have a PR target 6_0_stable that handles both encodings, and not change master.

For the few applications targeting edge that won't work. Feel like the ideal solution is what you are proposing and the flag

[etiennebarrie]

cc @rafaelfranca this is necessary for 6.1 or people using rolling deploys will see exceptions.

Also I haven't really thought about the deprecation story here. Would it be something like this?

• 6.1: support URL-safe tokens, allow to generate them as an option
• 6.1 patch: deprecate the option (force people to move to URL-safe tokens)
• 6.2: remove the option and code supporting strict tokens

[etiennebarrie]

Rebased against master to handle conflicts with 358ff18.