Cookie authentication handler calls SessionStore.RenewAsync() on sign in with expired session.

[jdaigle]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

The CookieAuthenticationHandler will call Options.SessionStore.RemoveAsync(_sessionKey, ...) during authentication as part of ReadCookieTicket() if the ticket loaded from the SessionStore is expired.

If during the same request we attempt to sign in again, the HandleSignInAsync() will call Options.SessionStore.RenewAsync(_sessionKey, ...);.

Depending on the SessionStore implementation, RenewAsync might throw an exception if it expects the session to still exist at the time it is called.

This appears to be caused by the changes from #22732, attempting to fix #22135.

The only workaround I can think of is to design the implementation of RenewAsync such that it adds back the session if it no longer exists. Is this assumption correct? Is this even safe?

Expected Behavior

After calling Options.SessionStore.RemoveAsync(_sessionKey, ...), the private _sessionKey field ought to be reset to null. This way a new session is correctly stored on sign in.

Steps To Reproduce

1. Sign in with cookie authentication.
2. Wait for the ticket to expired.
3. Execute a request which calls SignIn for cookie authentication, and attempts to renew the expired session.

Exceptions (if any)

No response

.NET Version

6.0.202

Anything else?

No response

[jdaigle]

Another workaround could be to implement RetrieveAsync on the SessionStore such that it doesn't return expired sessions. That would prevent CookieAuthenticationHandler from setting the _sessionKey field and incorrectly attempting to Renew an expired session.

[ghost]

Thanks for contacting us.
We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[adityamandaleeka]

@HaoK can you please investigate?

[Tratcher]

The only workaround I can think of is to design the implementation of RenewAsync such that it adds back the session if it no longer exists. Is this assumption correct? Is this even safe?

We are planning to make the fix you suggested (#42606), but I think RenewAsync needs to be written to be resilient against this regardless. The reason is that the _sessionKey is taken from the cookie, and the same cookie can be present on simultaneous requests. You'd need to account for one request reading the ticket from the store and setting the key, a second request removing the ticket from the store, and then the original request calling RenewAsync. I'm not sure if RenewAsync should re-add or discard the ticket in this case since it's called from both the cleanup and signin codepaths.