Skip to content

[RFC] partitioned option for setcookie/setrawcookie and sessions #12652

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
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[RFC] partitioned option for setcookie/setrawcookie and sessions #12652

wants to merge 2 commits into from
+222 −49

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Nov 10, 2023 •
edited
Loading

@nielsdos nielsdos mentioned this pull request Nov 10, 2023
Open
@nielsdos nielsdos marked this pull request as ready for review November 25, 2023 18:21
@nielsdos nielsdos requested a review from bukka as a code owner November 25, 2023 18:21
@iluuu1994
Copy link
Member

Partitioned cookies are marked as experimental by Mozilla (https://developer.mozilla.org/en-US/docs/Web/Privacy/Partitioned_cookies). Theoretically this means they are subject to change. It seems they're mostly pushed by Google.

I'm also unsure about the premise of the original issue:

As stated here (link), soon our cookies with SameSite=None; Secured, without the Partitioned parameter, will stop working.

I haven't tested this, but I think what this means is that Partitioned will essentially become the default, unless the domains are part of the same related website set. So I think this is essentially just an opt-in mechanism to ensure the cookies keep working? Related website sets are managed in a GitHub repository (https://github.com/GoogleChrome/related-website-sets). Thank you, Google, for that one. 😒

Maybe this warrants a short e-mail to the ML? I'm not a specialist in regards to cookies. Maybe other people have something to add.

@DustinAPI
Copy link

@nielsdos I am attempting to implement CHIPS for my app with an ugly workaround manually setting the cookie header so this PR would be very helpful but I think it should also include changes to the session cookie to truly allow developers to fix the problem.

session_set_cookie_params(['partitioned' => true])
['partitioned' => true]) = session_get_cookie_params()
ini_set('session.cookie_partitioned', 'on');

My use case is the most basic wherin my app (tool) is displayed in an iframe within a learning management system (LMS) and requires a session cookie for further interaction after the inital LTI launch.

@morsssss
Copy link
Contributor

morsssss commented Jan 5, 2024

Partitioned cookies are marked as experimental by Mozilla (https://developer.mozilla.org/en-US/docs/Web/Privacy/Partitioned_cookies). Theoretically this means they are subject to change. It seems they're mostly pushed by Google.

AFAIK, Mozilla and Safari have experimented with similar ideas, but right now only Chrome supports the Partitioned attribute. Chrome's goal is to let developers opt in to partitioning, instead of doing it by default.

As stated here (link), soon our cookies with SameSite=None; Secured, without the Partitioned parameter, will stop working.

I haven't tested this, but I think what this means is that Partitioned will essentially become the default, unless the domains are part of the same related website set. So I think this is essentially just an opt-in mechanism to ensure the cookies keep working?

That's it indeed! Chrome just started an experiment, now active for 1% of users, in which it will block third-party cookies that lack the Partitioned attribute and that are not allowed by other means. When a third-party site sets a Partitioned cookie, Chrome allows that site to access the cookie when it's embedded on a page from the domain where it was set. Access from other domains is blocked.

If this experiment results in a launch down the road, I think plenty of developers will want to use Partitioned, even though right now it's only a Chrome thing.

@nielsdos
Copy link
Member Author

nielsdos commented Jan 6, 2024

@nielsdos I am attempting to implement CHIPS for my app with an ugly workaround manually setting the cookie header so this PR would be very helpful but I think it should also include changes to the session cookie to truly allow developers to fix the problem.

session_set_cookie_params(['partitioned' => true]) ['partitioned' => true]) = session_get_cookie_params() ini_set('session.cookie_partitioned', 'on');

My use case is the most basic wherin my app (tool) is displayed in an iframe within a learning management system (LMS) and requires a session cookie for further interaction after the inital LTI launch.

Yeah, I fear that the scope of this must be increased, and ini settings are probably necessary too.
That would mean this needs an RFC, which I can do, but I'm busy with other PHP work right now.

@bukka
Copy link
Member

bukka commented Jan 12, 2024

I think we should probably have RFC for this as the agreement is not clear (Derick raised some concerns on internals) so I don't think we can just merge this.

@nielsdos nielsdos mentioned this pull request Jul 6, 2025
Closed
@nielsdos nielsdos changed the title Implement proof-of-concept partitioned option for setcookie [RFC] partitioned option for setcookie/setrawcookie Jul 13, 2025
@nielsdos nielsdos force-pushed the cookie-partitioned branch from e609e44 to 744fd4c Compare July 14, 2025 17:56
@nielsdos nielsdos requested a review from Girgias as a code owner July 14, 2025 19:58
@nielsdos nielsdos changed the title [RFC] partitioned option for setcookie/setrawcookie [RFC] partitioned option for setcookie/setrawcookie and sessions Jul 14, 2025
@xepozz
Copy link
Contributor

xepozz commented Jul 15, 2025

LGTM

Girgias
Girgias reviewed Jul 15, 2025
View reviewed changes
TimWolla
TimWolla reviewed Aug 12, 2025
View reviewed changes
ext/session/session.c Outdated
@@ -898,6 +898,7 @@ PHP_INI_BEGIN()
STD_PHP_INI_ENTRY("session.cookie_path", "/", PHP_INI_ALL, OnUpdateSessionStr, cookie_path, php_ps_globals, ps_globals)
STD_PHP_INI_ENTRY("session.cookie_domain", "", PHP_INI_ALL, OnUpdateSessionStr, cookie_domain, php_ps_globals, ps_globals)
STD_PHP_INI_BOOLEAN("session.cookie_secure", "0", PHP_INI_ALL, OnUpdateSessionBool, cookie_secure, php_ps_globals, ps_globals)
STD_PHP_INI_BOOLEAN("session.cookie_partitioned","0", PHP_INI_ALL, OnUpdateSessionBool, cookie_partitioned, php_ps_globals, ps_globals)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It already looks misaligned, so let's have the space after comma:

Suggested change
STD_PHP_INI_BOOLEAN("session.cookie_partitioned","0", PHP_INI_ALL, OnUpdateSessionBool, cookie_partitioned, php_ps_globals, ps_globals)
STD_PHP_INI_BOOLEAN("session.cookie_partitioned", "0", PHP_INI_ALL, OnUpdateSessionBool, cookie_partitioned, php_ps_globals, ps_globals)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll just add a commit to reformat this table. But a separate commit for history reasons...

ext/session/session.c
@@ -1725,6 +1736,7 @@ PHP_FUNCTION(session_set_cookie_params)
zend_string *lifetime = NULL, *path = NULL, *domain = NULL, *samesite = NULL;
bool secure = 0, secure_null = 1;
bool httponly = 0, httponly_null = 1;
bool partitioned = false, partitioned_null = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using false/true here is inconsistent with the rest and also with the reassignment in line 1809. This should be unified one way or another.

Independent of this, this should probably be fixed once and for all with a tree-wide Coccinelle run. Something like:

    @@
    bool b;
    @@

    - b = 1
    + b = true

    @@
    bool b;
    @@

    - b = 0
    + b = false

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix this for ext/session post-merge as a follow up

Copy link
Member

@TimWolla TimWolla Aug 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix this for ext/session post-merge as a follow up

Would do this tree-wide (possibly with per-extension commits) to not fix this piecemeal. See also: #19418 (comment)

A good opportunity might be right when branching PHP 8.5.

@TimWolla
Copy link
Member

Vote seems exceedingly likely to pass and the implementation is good except for minor nits, thus already requesting RM review as per https://externals.io/message/128453.

@TimWolla TimWolla requested a review from a team August 12, 2025 16:39
@nielsdos
Copy link
Member Author

Vote was accepted

TimWolla
TimWolla approved these changes Aug 12, 2025
View reviewed changes
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previous remarks are just nits without an effect on functionality, so this should've been an approval.

@nielsdos nielsdos force-pushed the cookie-partitioned branch from 832ea24 to 9d42a82 Compare August 12, 2025 20:55
edorian
edorian approved these changes Aug 12, 2025
View reviewed changes
Copy link
Member

@edorian edorian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No RM objections 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Girgias Girgias Girgias left review comments

@TimWolla TimWolla TimWolla approved these changes

@edorian edorian edorian approved these changes

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

Assignees
No one assigned
Labels
ABI break Extension: session Extension: standard RFC
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@nielsdos @iluuu1994 @DustinAPI @morsssss @bukka @xepozz @TimWolla @edorian @Girgias