-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Add support for the Partitioned cookie attribute #55371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
b717636
c70ac06
40ed63e
fde9c17
58caa90
b5b9af8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,3 @@ | ||
#nullable enable | ||
Microsoft.Net.Http.Headers.SetCookieHeaderValue.Partitioned.get -> bool | ||
Microsoft.Net.Http.Headers.SetCookieHeaderValue.Partitioned.set -> void |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,3 @@ | ||
#nullable enable | ||
Microsoft.AspNetCore.Http.CookieOptions.Partitioned.get -> bool | ||
Microsoft.AspNetCore.Http.CookieOptions.Partitioned.set -> void |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,6 +1,7 @@ | ||||||
// Licensed to the .NET Foundation under one or more agreements. | ||||||
// The .NET Foundation licenses this file to you under the MIT license. | ||||||
|
||||||
using System.Diagnostics.CodeAnalysis; | ||||||
using Microsoft.AspNetCore.Http.Features; | ||||||
using Microsoft.Extensions.DependencyInjection; | ||||||
using Microsoft.Extensions.Logging; | ||||||
|
@@ -15,7 +16,9 @@ namespace Microsoft.AspNetCore.Http; | |||||
internal sealed partial class ResponseCookies : IResponseCookies | ||||||
{ | ||||||
private readonly IFeatureCollection _features; | ||||||
|
||||||
private ILogger? _logger; | ||||||
private bool _retrievedLogger; | ||||||
|
||||||
/// <summary> | ||||||
/// Create a new wrapper. | ||||||
|
@@ -45,19 +48,10 @@ public void Append(string key, string value, CookieOptions options) | |||||
{ | ||||||
ArgumentNullException.ThrowIfNull(options); | ||||||
|
||||||
// SameSite=None cookies must be marked as Secure. | ||||||
if (!options.Secure && options.SameSite == SameSiteMode.None) | ||||||
var messagesToLog = GetMessagesToLog(options); | ||||||
if (messagesToLog != MessagesToLog.None && TryGetLogger(out var logger)) | ||||||
{ | ||||||
if (_logger == null) | ||||||
{ | ||||||
var services = _features.Get<IServiceProvidersFeature>()?.RequestServices; | ||||||
_logger = services?.GetService<ILogger<ResponseCookies>>(); | ||||||
} | ||||||
|
||||||
if (_logger != null) | ||||||
{ | ||||||
Log.SameSiteCookieNotSecure(_logger, key); | ||||||
} | ||||||
LogMessages(logger, messagesToLog, key); | ||||||
} | ||||||
|
||||||
var cookie = options.CreateCookieHeader(key, Uri.EscapeDataString(value)).ToString(); | ||||||
|
@@ -69,21 +63,12 @@ public void Append(ReadOnlySpan<KeyValuePair<string, string>> keyValuePairs, Coo | |||||
{ | ||||||
ArgumentNullException.ThrowIfNull(options); | ||||||
|
||||||
// SameSite=None cookies must be marked as Secure. | ||||||
if (!options.Secure && options.SameSite == SameSiteMode.None) | ||||||
var messagesToLog = GetMessagesToLog(options); | ||||||
if (messagesToLog != MessagesToLog.None && TryGetLogger(out var logger)) | ||||||
{ | ||||||
if (_logger == null) | ||||||
foreach (var keyValuePair in keyValuePairs) | ||||||
{ | ||||||
var services = _features.Get<IServiceProvidersFeature>()?.RequestServices; | ||||||
_logger = services?.GetService<ILogger<ResponseCookies>>(); | ||||||
} | ||||||
|
||||||
if (_logger != null) | ||||||
{ | ||||||
foreach (var keyValuePair in keyValuePairs) | ||||||
{ | ||||||
Log.SameSiteCookieNotSecure(_logger, keyValuePair.Key); | ||||||
} | ||||||
LogMessages(logger, messagesToLog, keyValuePair.Key); | ||||||
} | ||||||
} | ||||||
|
||||||
|
@@ -167,9 +152,95 @@ public void Delete(string key, CookieOptions options) | |||||
}); | ||||||
} | ||||||
|
||||||
private bool TryGetLogger([NotNullWhen(true)] out ILogger? logger) | ||||||
{ | ||||||
if (!_retrievedLogger) | ||||||
{ | ||||||
_retrievedLogger = true; | ||||||
var services = _features.Get<IServiceProvidersFeature>()?.RequestServices; | ||||||
_logger = services?.GetService<ILogger<ResponseCookies>>(); | ||||||
} | ||||||
|
||||||
logger = _logger; | ||||||
return logger is not null; | ||||||
} | ||||||
|
||||||
private static MessagesToLog GetMessagesToLog(CookieOptions options) | ||||||
{ | ||||||
var toLog = MessagesToLog.None; | ||||||
|
||||||
if (!options.Secure && options.SameSite == SameSiteMode.None) | ||||||
{ | ||||||
toLog |= MessagesToLog.SameSiteNotSecure; | ||||||
} | ||||||
|
||||||
if (options.Partitioned) | ||||||
{ | ||||||
if (!options.Secure) | ||||||
{ | ||||||
toLog |= MessagesToLog.PartitionedNotSecure; | ||||||
} | ||||||
|
||||||
if (options.SameSite != SameSiteMode.None) | ||||||
{ | ||||||
toLog |= MessagesToLog.PartitionedNotSameSiteNone; | ||||||
} | ||||||
|
||||||
// Chromium checks this | ||||||
if (options.Path != "/") | ||||||
{ | ||||||
toLog |= MessagesToLog.PartitionedNotPathRoot; | ||||||
} | ||||||
} | ||||||
amcasey marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
return toLog; | ||||||
} | ||||||
|
||||||
private static void LogMessages(ILogger logger, MessagesToLog messages, string cookieName) | ||||||
{ | ||||||
if ((messages & MessagesToLog.SameSiteNotSecure) != 0) | ||||||
{ | ||||||
Log.SameSiteCookieNotSecure(logger, cookieName); | ||||||
} | ||||||
|
||||||
if ((messages & MessagesToLog.PartitionedNotSecure) != 0) | ||||||
{ | ||||||
Log.PartitionedCookieNotSecure(logger, cookieName); | ||||||
} | ||||||
|
||||||
if ((messages & MessagesToLog.PartitionedNotSameSiteNone) != 0) | ||||||
{ | ||||||
Log.PartitionedCookieNotSameSiteNone(logger, cookieName); | ||||||
halter73 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
} | ||||||
|
||||||
if ((messages & MessagesToLog.PartitionedNotPathRoot) != 0) | ||||||
{ | ||||||
Log.PartitionedCookieNotPathRoot(logger, cookieName); | ||||||
} | ||||||
} | ||||||
|
||||||
[Flags] | ||||||
private enum MessagesToLog | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know if it matters here, as the enum isn't embedded in a type.
Suggested change
to make the enum smaller (1 byte vs. 4 bytes for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! I considered that, but decided against it for two reasons. First, I'm expecting there to be more flags in the future. I've already added one since the PR began. 😆 Second, I figured space saving was probably less valuable than simplified alignment on the stack (though I didn't measure). |
||||||
{ | ||||||
None, | ||||||
SameSiteNotSecure = 1 << 0, | ||||||
PartitionedNotSecure = 1 << 1, | ||||||
PartitionedNotSameSiteNone = 1 << 2, | ||||||
PartitionedNotPathRoot = 1 << 3, | ||||||
} | ||||||
|
||||||
private static partial class Log | ||||||
{ | ||||||
[LoggerMessage(1, LogLevel.Warning, "The cookie '{name}' has set 'SameSite=None' and must also set 'Secure'.", EventName = "SameSiteNotSecure")] | ||||||
[LoggerMessage(1, LogLevel.Warning, "The cookie '{name}' has set 'SameSite=None' and must also set 'Secure'. This cookie will likely be rejected by the client.", EventName = "SameSiteNotSecure")] | ||||||
public static partial void SameSiteCookieNotSecure(ILogger logger, string name); | ||||||
|
||||||
[LoggerMessage(2, LogLevel.Warning, "The cookie '{name}' has set 'Partitioned' and must also set 'Secure'. This cookie will likely be rejected by the client.", EventName = "PartitionedNotSecure")] | ||||||
public static partial void PartitionedCookieNotSecure(ILogger logger, string name); | ||||||
|
||||||
[LoggerMessage(3, LogLevel.Debug, "The cookie '{name}' has set 'Partitioned' and should also set 'SameSite=None'. This cookie will likely be rejected by the client.", EventName = "PartitionedNotSameSiteNone")] | ||||||
public static partial void PartitionedCookieNotSameSiteNone(ILogger logger, string name); | ||||||
|
||||||
[LoggerMessage(4, LogLevel.Debug, "The cookie '{name}' has set 'Partitioned' and should also set 'Path=/'. This cookie may be rejected by the client.", EventName = "PartitionedNotPathRoot")] | ||||||
public static partial void PartitionedCookieNotPathRoot(ILogger logger, string name); | ||||||
} | ||||||
} |
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.
We might also want to check for
options.SameSite == SameSiteMode.None && options.Secure && !options.Partitioned
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie