Add SameSitePolicy to CookiePolicyMiddleware #1222
Conversation
|
@JunTaoLuo, |
| options.SameSite = SameSiteEnforcementMode.Strict; | ||
| break; | ||
| default: | ||
| throw new InvalidOperationException(); |
There was a problem hiding this comment.
Would it be easier for a developer to diagnose this case if an error message were provided here?
There was a problem hiding this comment.
Poicy.SameSite.ToString(). This is more of an assert for if we add more enum values in the future.
| @@ -23,6 +22,7 @@ public CookieAuthenticationOptions() | |||
| ReturnUrlParameter = CookieAuthenticationDefaults.ReturnUrlParameter; | |||
| ExpireTimeSpan = TimeSpan.FromDays(14); | |||
| SlidingExpiration = true; | |||
| CookieSameSite = SameSiteEnforcementMode.None; | |||
There was a problem hiding this comment.
Probably. Strict would break, for example, links to a protected resource from another site. But given we try to be secure by default ... yea, strict.
| options.SameSite = SameSiteEnforcementMode.Strict; | ||
| break; | ||
| default: | ||
| throw new InvalidOperationException(); |
There was a problem hiding this comment.
Poicy.SameSite.ToString(). This is more of an assert for if we add more enum values in the future.
| public enum SameSitePolicy | ||
| { | ||
| None = 0, | ||
| LaxOrStrict, |
There was a problem hiding this comment.
Names need work. The policy specifies the minimum required level. Maybe you don't need a new enum, you can use the existing one and just name the option SameSiteMinimum.
JunTaoLuo
force-pushed
the
johluo/samesite
branch
from
ea747da to
d989a81
Compare
|
🆙 📅 |
|
ping |
|
This is currently blocking the build since MVC functional tests depends on these changes to pass. Otherwise we need to revert the HttpAbstractions commit. |
| @@ -899,6 +899,7 @@ private void WriteNonceCookie(string nonce) | |||
| new CookieOptions | |||
| { | |||
| HttpOnly = true, | |||
| SameSite = Http.SameSiteMode.Lax, | |||
There was a problem hiding this comment.
This is to differentiate between the one in M.A.Http and M.Net.Http.Headers. Both usings are already present and required.
There was a problem hiding this comment.
Brings up a good point, Should I be using the one from M.A.Http or M.Net.Http.Headers when I have the choice? Is there a preference for any particular one?
There was a problem hiding this comment.
Yup that's the one I try to use as much as possible.
|
|
||
| namespace Microsoft.AspNetCore.CookiePolicy | ||
| { | ||
| public enum MinimumSameSiteStrictnessPolicy |
There was a problem hiding this comment.
MinimumSameSitePolicy?
Since the values match the underlying enum, do we actually need this one?
There was a problem hiding this comment.
Ah yes, it used to be different since there were None, LaxOrStrict, and AlwaysStrict. But now it makes sense to just name the variable differently.
JunTaoLuo
force-pushed
the
johluo/samesite
branch
from
1ce9a52 to
769da5f
Compare
Addresses #908 Reacting to aspnet/HttpAbstractions#843. Still need to update defaults.