Generate a new CSRF token for the next form submission

[heliocola]

TODO

• [ ] Review devise logic inside sign_in(resource) and `sign_out(resource) The goal is to find way to keep the session as is, but guarantee the details in https://github.com/ruby-passkeys/devise-passkeys/pull/35#issuecomment-1611295505
• [ ] Write tests accordingly

In this PR

• Module ReauthenticationControllerConcern generate and return a new csrf token for the next form submission.

Template app:

In the template app, after the re-authentication, it is necessary to update the form authenticity_token with the value of new_csrf_token.

In my own app, I've changed the file passkey_reauthentication_handler.js to use it. The final lines of the method getReauthenticationToken, should add one more line:

  const reauthenticationTokenResponse = await(await reauthenticationTokenFetch).json()

  form.elements[reauthenticationTokenFieldName].value = reauthenticationTokenResponseJson.reauthentication_token
  form.elements['authenticity_token'].value = reauthenticationTokenResponseJson.csrf
}

[heliocola]

@tcannonfodder : I tested these changes in one of my apps that is using devise-passkeys gem and it mimics the structure of the devise-passkeys-template app, but with some more changes.

So, in my app this works after the passkeys re-auth flow. I still don't have this pushed in my app, but last week I had to defer to skip_before_action :verify_authenticity_token because of this issue.

But I also had this issue with the template app and tomorrow morning I will test this in the template app.

[tcannonfodder]

One thing I want to dig into is if this happens when Rails.ajax is used; since the template repo uses the native fetch method

[heliocola]

@tcannonfodder : the fetches works fine, but it is still strange that to me that we can make 2 using the same crsf in the X-CSRF-Token header. What fails is the final form submit to do some action (PATCH, PUT, POST) after the passkeys re-auth.

For any other async call and library, the user will have to put the CSRF token, and at the end, when they submit the form they will need the new csrf token. If everything is done through async call, it is possible that a solution like X-CSRF-Token will suffice for the 3 async calls.

It will be interesting to see how this whole process can be async and API based, so apps with Rails backend and React/Vue/jQuery front-end can use it.

We need to lookout for apps with such scenario or extend the template app to include a fully async flow and see how it goes.

BTW: I am not sure yet this solution is good for the gem itself, but it solved the issue in my other app. Today I will try to change the template app to use this branch and fix the issue I've found. I think that will help to find the best way forward, without fearing having to re-work this code.

It might be an interesting idea to document items that we want to keep a close eye on it, so we don't forget.

[tcannonfodder]

Yeah, I'm hesitant to make this part of the core gem itself; in favor of keeping it focused & not tying folks to a particular transport mechanism.

I haven't had a chance to dig in to this yet, will try to later this week or over the weekend

[heliocola]

@tcannonfodder : I've just noticed the sign_out and sign_in again...

Here are the references: sign_out: https://github.com/ruby-passkeys/devise-passkeys/blob/main/lib/devise/passkeys/controllers/reauthentication_controller_concern.rb#L96 sign_in: https://github.com/ruby-passkeys/devise-passkeys/blob/main/lib/devise/passkeys/controllers/reauthentication_controller_concern.rb#L98

Does the reauth controller really needs to sign the resource out and in again? If you comment out these lines, the invalid authenticity token goes away.

I've tested the User update in the template app, with these lines commented out and it doesn't throw the error anymore. The line 97 with the warden.authenticate(...) stays as is.

The reason the initial solution, in this PR, worked is because the new CSRF token is generated after the sign_in. And the one in the original HTML page, in the form, is invalid due to the sign_out.

I agree with you that this may not be the proper solution to be merged to the devise-passkeys gem itself. Maybe there are other solutions to this, and above I've just found one, that replace the need for the changes in this PR, but still touches on devise-passkeys gem.

[tcannonfodder]

Ack, that makes sense!

The reasoning for the full sign out & sign in was to try to harden the reauthentication point as much as possible. As in:

• It makes zero assumptions about whether a user is currently authenticated, by forcing them to go through the entire process again
• It went through the entire authentication stack, including any other strategies or devise extensions that are used
• It hits any post-authentication callbacks, such as checking that a user is not disabled

But blowing up the session like this is definitely going to cause problems 😬

So we need to:

• Figure out a way to get as many of those security features outlined above without clobbering the session
• Write regression tests to ensure that we don't break other parts of the session, by writing tests that ensure unrelated session & flash variables remain intact after reauthentication

[heliocola]

Ok. I will do some digging on it next week. Maybe add a BLOCKED label to this PR, as this is probably not going to be the solution merged, but it can help if any other user comes to this issue and try to contribute.

Regarding the steps needed, I will look in details what devise sign_out and sing_in are doing and see how we can execute everything but messing with the session. I will try to find any public method on devise or warden to ensure it, without stepping on the other gems toes...

[tcannonfodder]

Sounds good! Will add some form of postmortem & closing notes once we find a solution.

While you're tackling this, I'll keep our momentum going on the documentation side of things

[heliocola]

@tcannonfodder : the sign_out is the one resetting the session and invalidating the current form authenticity token. If I comment out only the sign_out(resource) (here) it works correctly (meaning: the form is submitted and the existing form authenticity token is considered valid and any data is updated correctly).

I will write down my learning's about the sign_out in order to organize my thoughts, after a few hours studying the stack trace for devise-passkeys reauthenticate method (all the way to warden gem), and to serve as place for you to read and help pick missing things.

TL'DR

1. I will still do some more testing and debugging on some things detailed below
2. It seems clear to me that calling sign_out(resource) inside devise-passkeys reauthenticate must be deleted (and perhaps replaced with a custom method we will have to maintain).

If you have the time to read The long story below, please send me comments. It helped me get a good sense whether the changes to the reauthenticate method will guarantee your points in a previous comment and understand if it is doing all it needs to do.

The long story

1. the sign_out(resource)

Now a deep dive into what the sign_out(resource) is doing: Ref: Devise repo > def sign_out(...)

1. Set scope and user local vars
2. call warden.logout(scope): this is the method that reset the session
3. call warden.clear_strategies_cache!(...)
4. remove (meaning: set with nil) instance var :"@current_#{scope}"

IMO: items 2 and 4 above we should not or don't need to do it. But 5 may be reconstructed during sign_in(...) that will follow.

NOTE: set_flash_messages!(:notice, :signed_in) is called in Devise::SessionControllers before/after sign_in and sign_out methods and I have not looked into this detail yet, but I will still get back to this.

After the above, I went further down looking into warden logout and clear_strategies_cache! methods. See below some comments.

1.1. warden.logout(scope)

This where the user's session is invalidated. It does a lot of cleaning:

1. delete scope from @users
2. run :before_logout callbacks
3. delete "warden.user.#{scope}.session" from raw_session
4. delete scope/user from session_serializer
5. Steps above are run for all scopes
6. And finally call reset_session!

Given this is called from inside Devise sign_out(...) method, I do believe we can't call it (meaning Devise sign_out(...)) inside devise-passkeys reauthenticate method, otherwise the form authenticity_token that is about to be used to update the data after the passkeys reauth flow will become invalid, which will require the approach on this PR (generating a new CSRF and making the JS code update the form that is being submitted or enabled for submission).

So, it seem clear to me that calling sign_out must be deleted from devise-passkeys reauthenticate method (here) This line is (seems to - to be more cautious) a must go!

1.2. warden.clear_strategies_cache!(scope: scope)

This is an interesting one and I believe this should still be called (but not without some disclaimers), maybe in a custom method inside ruby-passkeys/warden-webauthn gem.

This method basically delete scope from @winning_strategies and loop (do |k, v|) to @strategies for this scope and call v.clear! (which internally means _strategies.clear).

I have not dug into this yet, but this looks like the Ruby method clear that clears all content of self, so it just deletes all _strategies (inside Warden::Strategies).

Depending on how this is set, during sign_in(...), we will have to clear this. (DISCLAIMER 1) But the side effect of that is that we are choosing to do part of what Devise sign_out(...) does, so it does look like very intrusive and potentially dangerous. ~If Devise sign_in(...) don't de-reference these objects (making them candidates for GC), memory will keep going up, on every reauthenticate.~

UPDATE: I've stroked through (while revising the entire comment) the line above because I've just found that Warden::Strategies.add(...) will replace the strategy for a given label/scope in a way that looks like it will dereference the old object and make it GCed. But it may be prudent to still clear it.

(DISCLAIMER 2) It is possible the calling Devise sign_in(...) on a already signed in user, this (meaning the warden strategies cleared) won't be recreated as there is a scenario in it where it (aka sign_in) doesn't do anything.

I will try to investigate closely on this aspect. But it would be simpler to have a method in warden-webauthn gem to encapsulate this. And we keep a close eye on warden (via specific version gemspec [which is a DISCLAIMER in and off itself... :-)]) and review changes in warden for new things we would have to consider in warden-webauthn...

2. the sign_in(resource, event: :passkey_reauth)

I have not looked in details in this yet (run out of time today) but there is one area I am a bit concerned here is leaking memory due to doing the same actions twice, without a sign_out(...) being called.

This might not too time consuming to ensure but Devise has a method called bypass_sign_in in which Devise docs states this is "useful in cases the user is already signed in, but we want to refresh the credentials in session". But the same doc content also state that bypasses "the warden callbacks".

I've tested with the sign_in and it works, so the goal to dive into this is to ensure there is not memory leakage (if we opt to sign_in(...) again) or that we do all that is necessary and all we decide to clear during logout (if we opt to bypass_sign_in(...)).

To be continued...

[tcannonfodder]

Yeah, based on what you've found out, calling sign_out is definitely not the right way to go.

IMO: items 2 and 4 above we should not or don't need to do it. But 5 may be reconstructed during sign_in(...) that will follow.

Can you clarify what step 5 is; is that a typo?

I think I found a potential fix. If you check the warden.logout method, the reset_session! method is only called when reset_session = true, which is the default if there is no scope: https://github.com/wardencommunity/warden/blob/88d2f59adf5d650238c1e93072635196aea432dc/lib/warden/proxy.rb#L269C9-L269C9

Is reset_session! actually being called; or put another way: is the scopes variable empty when Warden::Proxy.logout is called? If so, is that something we can fix?

We might have to bypass Devise's sign_out method in this case, and call warden.logout(our_scopes) instead; because it feels like the intended purpose of Devise::Controllers#sign_out is actually different than what we're intending here (to force a full reauthentication for the given user, only logging them out if we cannot reauthenticate)

[tcannonfodder]

Also need to revisit how devise itself handles when you need to reauthenticate (eg: "please provide your current password"), but I suspect it's not relevant because they use a full-on regular form submission that will re-render the page entirely and fix the CSRF token

[tcannonfodder]

Dropping some debugging notes here

lib/devise/hooks/csrf_cleaner.rb:3

[tcannonfodder]

Have a fix! We need to set def clean_up_csrf? to return false in the Reauthentication strategy. Writing tests & making a PR now :)

[tcannonfodder]

Fixed in #45 by fixing the Reauthentication Strategy