Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

Add support for the Partitioned cookie attribute #55371

wants to merge 6 commits into from

Conversation

amcasey
Copy link
Member

@amcasey amcasey commented Apr 25, 2024

...to support CHIPS (Cookies Having Independent Partitioned State).

Fixes #53224
Fixes #55370

@amcasey amcasey requested a review from blowdart April 25, 2024 21:05
@ghost ghost added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Apr 25, 2024
@amcasey amcasey mentioned this pull request Apr 25, 2024
Open
halter73
halter73 reviewed Apr 25, 2024
View reviewed changes
halter73
halter73 reviewed Apr 25, 2024
View reviewed changes
@halter73
Copy link
Member

@blowdart I just left a comment on the issue you filed for this, but I'll repeat it here. Are we really doing this for an expired draft spec that's only supported by chromium-based browsers when the workaround is nearly as easy to use as the proposed API?

New API:

options.Cookie.Partitioned = true;

Workaround:

options.Cookie.Extensions.Add("partitioned");

We did approve the API, but we were a little rushed. And I don't think the workaround was ever demonstrated. The simplicity of the workaround really makes me want to hold off on this until the spec is final.

@blowdart
Copy link
Contributor

I take your point about the spec, but it's one of those that is unlikely to change (in my opinion) now that cookie has full throttle committed to it.

@amcasey
Copy link
Member Author

amcasey commented Apr 26, 2024

I take your point about the spec, but it's one of those that is unlikely to change (in my opinion) now that cookie has full throttle committed to it.

I'm guessing that was meant to be "chrome has full throttle committed to it".

@blowdart
Copy link
Contributor

Mozilla views it positively
Webkit views it positively

(Basically we'll add it soon)

Google started the staged rollout in chrome for 1% of users. although they stopped because regulators objected.

@amcasey
Copy link
Member Author

amcasey commented Apr 26, 2024

@blowdart @halter73 Would it be preferable to merge early and get feedback but be ready to pull it or save this until the last minute to build confidence that browsers will actually adopt it?

gfoidl
gfoidl reviewed Apr 30, 2024
View reviewed changes
src/Http/Http/src/Internal/ResponseCookies.cs
}

[Flags]
private enum MessagesToLog
Copy link
Member

Choose a reason for hiding this comment

The 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
private enum MessagesToLog
private enum MessagesToLog : byte

to make the enum smaller (1 byte vs. 4 bytes for int (default)).

Copy link
Member Author

Choose a reason for hiding this comment

The 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).

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label May 7, 2024
@amcasey
Copy link
Member Author

amcasey commented Jul 11, 2024

@halter73 @blowdart Where are we on this? Is the spec far enough along that we're comfortable merging (once CI is green)? We're running out of time to add new APIs.

@amcasey
Copy link
Member Author

amcasey commented Jul 11, 2024

Force push is a trivial rebase on main.

@amcasey amcasey removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jul 11, 2024
@blowdart
Copy link
Contributor

Go added it already. My vote is to do it now.

@halter73
Copy link
Member

Am I correct in understanding that it's almost been three months since this PR was opened and six months since the issue was filed and there hasn't been any movement on making this expired draft spec more real in that time? AFAICT, Chrome still hasn't blocked third-party cookies by default due to regulator objections.

I didn't realize the regulators were involved in this situation when I first reviewed this PR, but knowing about this makes me even more hesitant to merge this. I can see why regulators might have something to say about Chrome blocking third-party cookies given Google could track you anyway unlike their advertising competitors. And I could see regulators' decisions about what cookies Google can block affecting the final design of CHIPS or a competing standard.

It also appears that Firefox and Safari both still don't support CHIPS even though Firefox already partitions third-party cookies and Safari already blocks them.

I would be for merging this if the spec was ratified. Or if Firefox or Safari implemented. Or maybe if Chrome actually did start blocking cookies by default. But it appears none of this has happened yet. Remember we're comparing

options.Cookie.Partitioned = true;

to

options.Cookie.Extensions.Add("partitioned");

The only benefit to the new API is discoverability, but we don't even know if this is going to exist in 10 years. And anyone who needs this will probably search for "asp.net core partitioned cookie" will find https://stackoverflow.com/a/77846869/719967 I don't understand the rush.

@amcasey amcasey mentioned this pull request Jul 15, 2024
Open
1 task
@amcasey amcasey marked this pull request as draft July 15, 2024 22:01
BrennanConroy
BrennanConroy reviewed Sep 20, 2024
View reviewed changes
src/Http/Http/src/Internal/ResponseCookies.cs
toLog |= MessagesToLog.SameSiteNotSecure;
}

if (options.Partitioned)
Copy link
Member

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

Warning: Cookies with the SameSite=None; Secure that do not also have the Partitioned attribute may be blocked in cross-site contexts on future browser versions. This behavior protects user data from cross-site tracking. See Cookies Having Independent Partitioned State (CHIPS) and Third-party cookies.

@halter73
Copy link
Member

halter73 commented Apr 4, 2025

@blowdart Are you okay with me closing this?

@blowdart
Copy link
Contributor

blowdart commented Apr 4, 2025

🤷‍♂️

@halter73
Copy link
Member

halter73 commented Apr 4, 2025 •
edited
Loading

We can always reopen it.

@halter73 halter73 closed this Apr 4, 2025
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview4 milestone Apr 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@halter73 halter73 halter73 left review comments

@gfoidl gfoidl gfoidl left review comments

@BrennanConroy BrennanConroy BrennanConroy left review comments

@blowdart blowdart Awaiting requested review from blowdart

@captainsafia captainsafia Awaiting requested review from captainsafia captainsafia is a code owner

Assignees
No one assigned
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Milestone
10.0-preview4
Development

Successfully merging this pull request may close these issues.

Add support for Partitioned Cookies Add support for Partitioned Cookies
5 participants
@amcasey @halter73 @blowdart @gfoidl @BrennanConroy