Review comments of v1 patch:

1. Should this only be in the request.post? branch, so we're not cycling sessions for every render of the login page?
2. This should also be in extlogin.
3. This introduces a regression with saved items within the session (see session_expiry in app controller), they're deleted as the session is reset.
4. Are the API controllers vulnerable too, since they also use the session?

3. the session_expiry handler also saves the current taxonomy - maybe we should put this into a shared method so we don't have discrepancies

4. my understanding is that not creating new session IDs is the issue here. The security bug is that we take the session ID from the request, then escalate its privileges by assigning a current user so an attacker with a copy of the session ID (or who planted it into a user's request) gets escalated privileges.

The solution in the non-API controllers is to always generate new session IDs when we authenticate and escalate privileges. Since we do this on every request in the API, I'm not sure what the equivalent would be. Generating a new session on every request sounds expensive. Can we disable session handling in these controllers, or stop the authentication concern from storing escalated user details in the session when inside API controllers?

4. Ah, perhaps it's already the case that the API controllers don't touch the session: https://github.com/theforeman/foreman/blob/0f7d219a4a65cd795eecd05117b08511d9025de2/test/functional/api/base_controller_subclass_test.rb#L44-L47

EDIT: https://github.com/theforeman/foreman/blob/355bce36288ec6cd9f5a66b656f8a84158f2a8e1/app/controllers/concerns/foreman/controller/authentication.rb#L19

The only minor issue is that the base controller test doesn't test in the case where login=true, only login=false.

Are we referring to this line in the code regarding SSO
https://github.com/theforeman/foreman/blob/develop/app/controllers/concerns/foreman/controller/authentication.rb#L64

This calls authenticate! for each provider.

I haven't worked with the SSO code, so I think it will be more efficient if Marek can take a look at this issue (as I think he wrote the SSO code). I don't have apache installed locally to test it.

Dominic, the session[:location_id] and session[:organization_id] are saving correctly when I test it on my local machine.

it seems this line is not working correctly
backup_session_content { expire_session }

As per our IRC conversation, this works for me. When testing expiry, set the idle_timeout setting to '1' and wait a minute for the session to expire. Deleting your session ID isn't the same as a time-based expiry - it's only on a time-based expiry that we know the user is still the same one, and are able to restore their context.

Dom, are you waiting on me for any action items? I still think that location_id and organizatin_id can be saved in a similar way to session[:original_uri]

:+1:

I've reproduced the issue locally and tested Dominic's patch, which correctly fixes the issue. I've also tested that the session expiry correctly stores the user's current taxonomy scope, so it's restored when logging back in.

Good to merge from my perspective.

Yes, I don't think you're testing the right thing, and even before this patch, what you're testing wouldn't work. On session expiry (that is, the time limit by the idle_timeout setting) then the current context is preserved with this patch.

If you delete your session ID then you're testing something else. We can only restore original_uri in that case because the browser is making a new request for it, while the browser doesn't provide anything in the request about its taxonomy context. We could store this in a cookie instead, but this is unrelated to the session fixation issue and I don't think I've caused a regression.

I do however need an ack from another maintainer to proceed, thanks.

Dom, thanks for the explanation.