AF Cookie should be reused within the context of same request. #1128
Conversation
| @@ -56,7 +65,7 @@ public void SaveCookieToken(HttpContext httpContext, AntiForgeryToken token) | |||
| { | |||
| options.Secure = true; | |||
| } | |||
|
|
|||
| @@ -25,7 +25,12 @@ public AntiForgeryToken GetCookieToken(HttpContext httpContext) | |||
| var cookie = httpContext.Request.Cookies[_config.CookieName]; | |||
There was a problem hiding this comment.
Will GetCookieToken be called multiple times? If so then consider checking HttpContext.Items first. If it's not there then check the cookie and store the deserialized value in Items.
There was a problem hiding this comment.
Is lookup on request more expensive than lookup on items ( i suppose it is but want to verify).
The save happens later when SaveCookieToken is called.
harshgMSFT
force-pushed
the
AntiForgeryPreserveCookie
branch
from
c732eb9 to
766a3c1
Compare
|
@yishaigalatzer branch updated. |
|
|
and try to get a second set of eyes on it
| // Add the cookie to the request based context. | ||
| // This is useful if the cookie needs to be reloaded in the context of the same request. | ||
| var contextAccessor = httpContext.RequestServices.GetService<IContextAccessor<AntiForgeryContext>>(); | ||
| if (contextAccessor.Value == null) |
There was a problem hiding this comment.
would think that the assert is also not needed, the value should be overwritten if SaveCookie is called multiple times with a different cookie value.
There was a problem hiding this comment.
Calling SaveCookieToken multiple times on the same request is what caused this bug in the first place.
There was a problem hiding this comment.
well, sure... I don't see the downside of adding it.
|
|
harshgMSFT
force-pushed
the
AntiForgeryPreserveCookie
branch
2 times, most recently
from
47cd611 to
9de52b3
Compare
harshgMSFT
force-pushed
the
AntiForgeryPreserveCookie
branch
from
9de52b3 to
0c13563
Compare
Also adding some more functional tests for af.