#37000 Set SameSite to Lax via a new setcookie() wrapper
#12444
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1560,6 +1560,99 @@ function nocache_headers() { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Defines a cookie to be sent along with the rest of the HTTP headers. | ||
| * | ||
| * Wrapper for PHP's native setcookie() that provides a filter to adjust the | ||
| * options for all cookies in one place, and a short-circuit filter to prevent | ||
| * a cookie from being sent. | ||
| * | ||
| * The options are passed to setcookie() unchanged, so its native defaults apply | ||
| * to any that are omitted. | ||
| * | ||
| * @since 7.1.0 | ||
| * | ||
| * @param string $name The name of the cookie. | ||
| * @param string $value The value of the cookie. | ||
| * @param array $options { | ||
| * Optional. Options to pass to setcookie(). Default empty array. | ||
| * | ||
| * @type int $expires The time the cookie expires, as a Unix timestamp. | ||
| * @type string $path The path on the server in which the cookie will be available on. | ||
| * @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'. | ||
|
Member
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 additionally, are we setting the default values anywhere? what if someone passes the options but omits the something like this… $options = filter…
$options = filter…
…
$options['samesite'] = isset( $options['samesite'] ) && in_array( $options['samesite'], array( 'Lax', 'Strict', 'None' ), true )
? $options['samesite']
: 'Lax;
Member
There was a problem hiding this comment.
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 Something that Claude pointed out is that
Member
There was a problem hiding this comment. seems there would be, at a minimum, a warrant to perform consistency and coherency checks here. I saw something else too about Chromium treating the absence of |
||
| * } | ||
| * @return bool Whether the cookie was sent successfully. | ||
| * @phpstan-param array{ | ||
| * expires?: int, | ||
| * path?: string, | ||
| * domain?: string, | ||
| * secure?: bool, | ||
| * httponly?: bool, | ||
| * samesite?: 'Lax'|'Strict'|'None', | ||
| * } $options | ||
| */ | ||
| function wp_set_cookie( string $name, string $value, array $options = array() ): bool { | ||
|
Member
There was a problem hiding this comment. should
Member
There was a problem hiding this comment. +1 to @nimesh-xecurify's suggestion on defaulting Counting the call sites the diff adds: all 19 $options = wp_parse_args( $options, array( 'samesite' => 'Lax' ) );before the |
||
| /** | ||
| * Filters the options used when a cookie is sent to the browser. | ||
| * | ||
| * @since 7.1.0 | ||
| * | ||
| * @param array $options The options to pass to setcookie(). | ||
| * @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 ); | ||
|
|
||
|
|
||
| /** | ||
| * Filters whether a cookie should be sent to the browser. | ||
| * | ||
| * Returning false prevents the cookie from being sent. | ||
| * | ||
| * @since 7.1.0 | ||
| * | ||
| * @param bool $send Whether to send the cookie. Default true. | ||
| * @param string $name The name of the cookie. | ||
| * @param string $value The value of the cookie. | ||
| * @param array $options The options to pass to setcookie(). | ||
| */ | ||
| if ( ! apply_filters( 'send_cookie', true, $name, $value, $options ) ) { | ||
| return false; | ||
| } | ||
|
Comment on lines
+1621
to
+1623
|
||
|
|
||
| return setcookie( $name, $value, $options ); | ||
| } | ||
|
|
||
| /** | ||
| * Removes a cookie from the browser. | ||
| * | ||
| * Sends a cookie with an empty value and an expiry time in the past, which | ||
|
|
||
| * instructs the browser to delete it. The path and domain must match those | ||
| * used when the cookie was originally set for the removal to take effect. | ||
| * | ||
| * @since 7.1.0 | ||
| * | ||
| * @param string $name The name of the cookie. | ||
| * @param array $options Optional. Options to pass to setcookie(). See {@see wp_set_cookie()} for the full list. | ||
| * Default empty array. | ||
| * @return bool True if the cookie was removed successfully, false otherwise. | ||
| * @phpstan-param array{ | ||
| * expires?: int, | ||
| * path?: string, | ||
| * domain?: string, | ||
| * secure?: bool, | ||
| * httponly?: bool, | ||
| * samesite?: 'Lax'|'Strict'|'None', | ||
| * } $options | ||
| */ | ||
| function wp_unset_cookie( string $name, array $options = array() ): bool { | ||
| $options['expires'] = time() - YEAR_IN_SECONDS; | ||
|
|
||
| return wp_set_cookie( $name, ' ', $options ); | ||
| } | ||
|
|
||
| /** | ||
| * Sets the HTTP headers for caching for 10 days with JavaScript content type. | ||
| * | ||
|
|
||
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.
These
@since 7.1.0tags — this one, the two hook docblocks at 1601 and 1614, andwp_unset_cookie()at 1635 — should be set from the release schedule at commit time rather than from this branch.Flagging it because it currently reads correct in context and is easy to skim past: this branch's
version.phpis still7.1-alpha-62161-src, while trunk has moved on to7.2-alpha. The Trac milestone says 7.2, and I've heard 7.1.1 mentioned — whichever it lands in, all four need to match.One wrinkle if it does go to 7.1.1: this adds two public functions and two public hooks, which is more new API surface than a point release normally carries.