- Notifications
You must be signed in to change notification settings - Fork 191
Adding SameSite attribute to SetCookies header #843
Conversation
| }); | ||
| } | ||
| | ||
| private Net.Http.Headers.SameSiteEnforcementMode ConvertSameSiteEnforcementMode(SameSiteEnforcementMode sameSite) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of the duplication since the two enums will need conversion.
851f68b to 007d1b2 Compare | | ||
| namespace Microsoft.Net.Http.Headers | ||
| { | ||
| public enum SameSiteEnforcementMode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need this in two different places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Features and Net.Http.Headers cannot depend on each other.
| private const string SecureToken = "secure"; | ||
| private const string SameSiteToken = "samesite"; | ||
| private static readonly string SameSiteLaxToken = SameSiteEnforcementMode.Lax.ToString(); | ||
| private static readonly string SameSiteStrictToken = SameSiteEnforcementMode.Strict.ToString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ToLower() for consistency with other tokens?
| | ||
| namespace Microsoft.AspNetCore.Http | ||
| { | ||
| public enum SameSiteEnforcementMode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// This mirrors Microsoft.Net.Http.Headers.SameSiteEnforcementMode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SameSiteMode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link to the RFC?
| private const string DomainToken = "domain"; | ||
| private const string PathToken = "path"; | ||
| private const string SecureToken = "secure"; | ||
| private const string SameSiteToken = "samesite"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RFC link
7ce2948 to 2b60dbc Compare | /// Gets or sets the value for the SameSite attribute of the cookie. | ||
| /// </summary> | ||
| /// <returns>The <see cref="SameSiteEnforcementMode"/> representing the enforcement mode of the cookie.</returns> | ||
| public SameSiteEnforcementMode SameSite { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blowdart should the default be strict?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup
| { | ||
| // RFC Draft: https://tools.ietf.org/html/draft-ietf-httpbis-cookie-same-site-00 | ||
| // This mirrors Microsoft.Net.Http.Headers.SameSiteEnforcementMode | ||
| public enum SameSiteEnforcementMode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SameSiteMode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed
|
|
fbf4b79 to e8123db Compare
Addresses #710, will also update the CookiesPolicyMiddleware in Security.