#37000 Set SameSite to Lax via a new setcookie() wrapper - #12444
#37000 Set SameSite to Lax via a new setcookie() wrapper#12444johnbillion wants to merge 12 commits into
setcookie() wrapper#12444Conversation
… using the array signature for options.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
|
If this PR is ready for release, I would appreciate it if you could commit it before tomorrow's 7.1 beta1 commit freeze. |
|
The changed non-test code passes PHPStan rule level 10 (essentially). |
There was a problem hiding this comment.
Pull request overview
This PR standardizes cookie setting across core by introducing a setcookie() wrapper that supports centralized filtering and easier test interception, while explicitly setting SameSite=Lax for consistency across browsers.
Changes:
- Introduces
wp_set_cookie()/wp_unset_cookie()wrappers with filters to adjust options and short-circuit sending. - Migrates multiple core cookie call sites to use the wrapper and explicitly sets
samesite => 'Lax'. - Updates PHPUnit infrastructure/tests to prevent header warnings and to capture/assert cookie sends via the new
send_cookiefilter.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/phpunit/tests/option/wpUserSettings.php | Updates the user settings cookie test to capture cookies via send_cookie. |
| tests/phpunit/tests/functions/wpSetCookie.php | Adds new unit test coverage for wp_set_cookie() / wp_unset_cookie() filtering. |
| tests/phpunit/includes/functions.php | Switches PHPUnit harness to disable cookie sending via send_cookie to avoid header warnings. |
| src/wp-login.php | Replaces direct setcookie() calls with wp_set_cookie() / wp_unset_cookie() and sets SameSite=Lax. |
| src/wp-includes/pluggable.php | Migrates auth cookie set/clear operations to the new cookie wrappers and documents send_cookie. |
| src/wp-includes/option.php | Updates user settings cookie set/unset logic to use the new wrappers with SameSite=Lax. |
| src/wp-includes/functions.php | Adds the new wp_set_cookie() / wp_unset_cookie() wrapper functions and filters. |
| src/wp-includes/comment.php | Migrates comment author cookies to the wrapper and sets SameSite=Lax. |
| src/wp-includes/class-wp-recovery-mode-cookie-service.php | Migrates recovery-mode cookie set/clear to the wrapper and sets SameSite=Lax. |
| src/wp-admin/post.php | Migrates the wp-saving-post cookie to the wrapper and sets SameSite=Lax. |
| src/wp-activate.php | Migrates activation cookie set/clear to the wrapper and sets SameSite=Lax. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @param string $name The name of the cookie. | ||
| * @param string $value The value of the cookie. | ||
| */ | ||
| $options = apply_filters( 'wp_set_cookie_options', $options, $name, $value ); |
| if ( ! apply_filters( 'send_cookie', true, $name, $value, $options ) ) { | ||
| return false; | ||
| } |
| /** | ||
| * Removes a cookie from the browser. | ||
| * | ||
| * Sends a cookie with an empty value and an expiry time in the past, which |
| $options = $this->sent_cookies[0]['options']; | ||
|
|
| $this->assertArrayHasKey( 'wp-settings-' . $user_id, $this->sent_cookies ); | ||
| $this->assertSame( 'foo=bar', $this->sent_cookies[ 'wp-settings-' . $user_id ]['value'] ); | ||
|
|
||
| // The companion timestamp cookie is also sent. | ||
| $this->assertArrayHasKey( 'wp-settings-time-' . $user_id, $this->sent_cookies ); |
|
This looks great overall. One suggestion on the As written, the goal of the patch is only enforced by convention. Any future core call site (or any plugin adopting the wrapper) that omits There's decent precedent for core wrappers being opinionated where the underlying primitive is unsafe or inconsistent: |
|
@johnbillion the WP Consent API plugin already defines it’s unfortunate naming, but I guess that’s also an official plugin? there is probably reason to avoid the collision. looks like more than 200,000 active installs reported for that one. |
| * @type string $domain The (sub)domain that the cookie is available to. | ||
| * @type bool $secure Indicates that the cookie should only be transmitted over a secure HTTPS connection from the client. | ||
| * @type bool $httponly When true the cookie will be made accessible only through the HTTP protocol. | ||
| * @type string $samesite Whether the cookie should be available for cross-site requests. Accepts 'Lax', 'Strict', or 'None'. |
There was a problem hiding this comment.
the claim is that this accepts these values, but we never verify that. perhaps it would be worth adding a check and a _doing_it_wrong(), given the sensitive nature of this flag.
additionally, are we setting the default values anywhere? what if someone passes the options but omits the samesite property? perhaps we would want to use latching logic instead on some of these properties.
something like this…
$options = filter…
$options = filter…
…
$options['samesite'] = isset( $options['samesite'] ) && in_array( $options['samesite'], array( 'Lax', 'Strict', 'None' ), true )
? $options['samesite']
: 'Lax;There was a problem hiding this comment.
the claim is that this accepts these values, but we never verify that. perhaps it would be worth adding a check and a
_doing_it_wrong(), given the sensitive nature of this flag.
Good point. Apparently PHP doesn't do any validation here either, passing through strings verbatim. At least there is PHPStan typing added to help guard against this, but it is case-sensitive. We could do case-insensitive checking and raise _doing_it_wrong() if there is no match.
Something that Claude pointed out is that SameSite=None doesn't do anything unless Secure is also set. So perhaps _doing_it_wrong() should be raised if SameSite is being set but Secure isn't, cf. MDN:
Send the cookie with both cross-site and same-site requests. The
Secureattribute must also be set when using this value.
| * samesite?: 'Lax'|'Strict'|'None', | ||
| * } $options | ||
| */ | ||
| function wp_set_cookie( string $name, string $value, array $options = array() ): bool { |
There was a problem hiding this comment.
should wpCookies.set() in JavaScript also be updated to avoid inconsistencies between JS fetches and HTML-originated browser fetches? or does the policy carry over?
|
the copilot reviews on the filter outputs seem relevant given the context of what this function is doing. either by mistake or by intent, it would be good to guard the most-essential cookie properties |
The
SameSiteattribute on a cookie is a form of CSRF protection that controls whether the browser attaches it to a request based on the request type (top-level navigation, iframe, link click, etc) and whether the request's site matches the top-level browsing context.Strictmeans the cookie will only be attached to the request if its site matches the top-level URL in the browser when the request was triggered. The cookie will not be sent if the request is triggered by or via a third party site, for example clicking a link on another site, submitting a form on another site, or iframing the page into another site.Laxrelaxes this behaviour so the cookie will be attached when clicking a link on another site, but still not sent in other contexts.Nonemeans the cookie will always be sent regardless of the context.See SameSite cookies explained for more info.
The default value for
SameSitein Chromium-based browsers has been set toLaxsince 2020. This breaks some less-common use cases such as iframing a site, but just as importantly, results in inconsistency across browsers because neither Firefox nor Safari do the same.SameSitetoLaxwhen no value is present (since 2020)For consistency and explicitness, the
SameSiteattribute should be set toLaxeverywhere, and a filter should be introduced to control the values of cookie attributes.Changes
Since the minimum supported version of PHP is now > 7.3 we can switch to using the array signature for
setcookie()options and specify a value for theSameSiteflag for consistency.In order to standardise setting
SameSitetoLaxin all browsers, facilitate being able to change theSameSitevalue for any given cookie (either to tighten it toStrictor loosen it toNone), and to make everything more testable, this PR makes the following changes:wp_set_cookie()wrapper function with filters for controlling the cookie options and whether cookies are sent. This increases the testability of functions that set cookies.wp_unset_cookie()function as a convenience wrapper for unsetting cookies.samesiteoption toLax. I have not identified any cookie that's set by WordPress that would benefit from aSameSitevalue ofStrict, and none that need downgrading toNone.Consderations
As far as I can tell, everything that might break as a result of this change has already been broken in all browsers since 2022, primarily iframing a site inside another site and allowing the user of the iframed site to remain logged in.
This change standardises the attribute for consistency and introduces the ability for site owners to adjust its value on a per-cookie basis.
Supporting info
Drupal, Symfony, Laravel, Django, RoR, ASP.NET, Magento, and many other frameworks default to explicitly setting
SameSitetoLaxrather than relying on an implied default in browsers.Trac ticket: https://core.trac.wordpress.org/ticket/37000