Respect AllowRefresh in SecurityStampValidator cookie renewal#65803
Open
matantsach wants to merge 3 commits intodotnet:mainfrom
Open
Respect AllowRefresh in SecurityStampValidator cookie renewal#65803matantsach wants to merge 3 commits intodotnet:mainfrom
matantsach wants to merge 3 commits intodotnet:mainfrom
Conversation
SecurityStampValidator.SecurityStampVerified() unconditionally sets context.ShouldRenew = true, which extends the session even when AllowRefresh is explicitly set to false. This is inconsistent with CookieAuthenticationHandler.CheckForRefreshAsync(), which respects AllowRefresh for sliding expiration. Guard ShouldRenew with the same AllowRefresh check. The in-memory principal is still replaced (claims updated) regardless — only the cookie rewrite is gated. Fixes dotnet#64301
Contributor
|
Thanks for your PR, @@matantsach. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
1 task
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request fixes ASP.NET Core Identity’s security-stamp validation behavior so that cookie renewal respects AuthenticationProperties.AllowRefresh, aligning it with CookieAuthenticationHandler refresh semantics and enabling fixed-lifetime sessions when AllowRefresh = false.
Changes:
- Update
SecurityStampValidator<TUser>.SecurityStampVerified()to setcontext.ShouldRenewbased oncontext.Properties.AllowRefresh ?? trueinstead of always renewing. - Gate the non-sliding-expiration
IssuedUtcrecalculation behindcontext.ShouldRenew. - Add tests covering
AllowRefresh = false(no renewal) andAllowRefreshunset (default renewal behavior preserved).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Identity/Core/src/SecurityStampValidator.cs | Stops unconditional cookie renewal after successful security-stamp validation; renewal now honors AllowRefresh, and IssuedUtc adjustment only happens on actual renewal. |
| src/Identity/test/Identity.Test/SecurityStampValidatorTest.cs | Adds coverage ensuring ShouldRenew is suppressed when AllowRefresh = false, and remains enabled by default when AllowRefresh is not set. |
You can also share your feedback on Copilot code review. Take the survey.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SecurityStampValidator.SecurityStampVerified()unconditionally setscontext.ShouldRenew = trueafter verifying the security stamp. This means the authentication cookie gets rewritten on every validation cycle — even whenAllowRefreshis explicitly set tofalseon the authentication properties.This is inconsistent with
CookieAuthenticationHandler.CheckForRefreshAsync(), which guards its renewal logic behindvar allowRefresh = ticket.Properties.AllowRefresh ?? true. A user who setsAllowRefresh = falseexpects fixed-lifetime sessions that don't get extended by background activity, but the security stamp validator silently overrides that intent.The fix mirrors the handler's pattern:
The in-memory principal is still replaced regardless —
ReplacePrincipalis not gated — so the user's claims stay fresh within the request. Only the cookie rewrite (session extension) is suppressed.The
IssuedUtcrecalculation for non-sliding expiration is also gated onShouldRenew, since it serves the renewal path and has no effect when the cookie isn't rewritten.Test plan
OnValidateIdentityDoesNotRenewWhenAllowRefreshIsFalse— explicitAllowRefresh = falseproducesShouldRenew == false, principal still replacedOnValidateIdentityRenewsWhenAllowRefreshIsNotSet— nullAllowRefresh(the common case) defaults toShouldRenew == true, preserving existing behaviorOnValidateIdentityDoesNotExtendExpirationWhenSlidingIsDisabledstill passes —IssuedUtcadjustment unaffected whenAllowRefreshis not setFixes #64301