Ugh, sorry haven't had time to look into this issue lately.

How come you're using class variables instead of default options?

Re naming, we already have authenticity_param, why not authenticity_key?

Also just as a reflex, this patch is really hard to grok by the diff because there's a lot of changing of method visibility. So please give me some time to review this more thoroughly!

Thank you for your patch!!

Yeah, sorry about big diff.

The reason for the class variables for the session key and token length stems from the need to offer a way get a masked authenticity token when building forms in a view. So you could do that now by calling:

Rack::Protection::AuthenticityToken.token(session)

I think most would want to put that in a helper.

Previously you would just use the value session[:csrf], and you could still do that but you wouldn't get BREACH protection.

To avoid using class variables, there are a couple options I can think of:

1. The session key and token length could be passed in along with the session as options. (seems kind of painful to have to check the middleware configuration when building forms).
2. Keep the session key a fixed value of :csrf and token length a fixed value of 32. If a Rails app needs to generate a form to a mounted Sinatra app it could use Rack::Protection::AuthenticityToken.token(session) as the value for the authenticity token (instead of using the standard view helper). If a Sinatra app needs to post to a Rails form it could use a method like Rack::Protection::AuthenticityToken.mask_token(session[:_csrf_token]). A little more involved, but maybe this is the right way for a little used scenario?

So I guess, prior to Rails adding BREACH mitigation, the tokens were compatible. And since then we've been promising a lie, because they were no longer compatible. I did however see something about keeping support for existing session tokens with the Rails patch, but I'm not sure if that affects us.

If that's true (P.D.I.), and we're both using different BREACH strategies (e.g. the token length is different, etc) then I would almost say let's just use the major version bump to break that promise and update the documentation for Sinatra. I have a feeling supporting shared sessions between sinatra and Rails will prove to be an ongoing effort when it is unclear how many people rely on this feature, and could implement it themselves.

Yes, once Rails started using masked tokens in forms, rack-protection would not have been able to handle them. You're right though, Rails would still be able to compare the unmasked authenticity tokens so a Sinatra app could directly access session[_:csrf_token] and that would work.

I think using the major version bump to break the promise of compatibility is good. Or at least the idea of syncing the values of the two different authenticity tokens stored in the session. I've updated the PR to use a fixed value for the session key and token length. This makes the helper method for fetching a masked version for the token easy to use:

Rack::Protection::AuthenticityToken.token(session)

Since the BREACH protection strategy and token length does currently match up with Rails the following code could be used to get a masked version of the Rails authenticity token:

Rack::Protection::AuthenticityToken.token(session, :_csrf_token)

If you don't think it's a good idea to go down that road though, I could remove that optional parameter.

I'm almost 90% sure we should just break the promise, but I'd like to get some feedback to see if anyone is actually using this feature. Maybe @rkh knows?

Sinatra isn't designed to be a middleware, it has it's own router and other utilities that make it set apart from the rest of your stack, be it Rails or whatever. However, RP is designed to be a middleware, but at the core of it is session protection. We can't possibly hope to maintain compatibility with every other session protection out there, so in my opinion if you want to protect a session you should decide before hand how that session will be shared or not -- and which protection library to use.

@zzak I'm all for that suggestion 👍

I do think I'd still rather see the token length a fixed value of 32. It keeps things simple and it makes sure a secure value is used. See: http://security.stackexchange.com/questions/6957/length-of-csrf-token

Also, this way Rack::Protection::AuthenticityToken.token(session) can be used by other apps that do share the session. It will make sure a token is set in the session and return the masked value (without worrying about token length).

If that's good, I'll clean up these commits.

I do think I'd still rather see the token length a fixed value of 32.

👍