Opened 5 years ago
Last modified 15 months ago
#50522 new defect (bug)
stop setting "older" cookies with multiple path prefixes
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | 5.4.2 |
Component: | Login and Registration | Keywords: | has-patch changes-requested dev-feedback |
Focuses: | administration | Cc: |
Description
According to wp_clear_auth_cookie()
,
<?php // Auth cookies. setcookie( AUTH_COOKIE, ' ', time() - YEAR_IN_SECONDS, ADMIN_COOKIE_PATH, COOKIE_DOMAIN ); setcookie( SECURE_AUTH_COOKIE, ' ', time() - YEAR_IN_SECONDS, ADMIN_COOKIE_PATH, COOKIE_DOMAIN ); setcookie( AUTH_COOKIE, ' ', time() - YEAR_IN_SECONDS, PLUGINS_COOKIE_PATH, COOKIE_DOMAIN ); setcookie( SECURE_AUTH_COOKIE, ' ', time() - YEAR_IN_SECONDS, PLUGINS_COOKIE_PATH, COOKIE_DOMAIN ); setcookie( LOGGED_IN_COOKIE, ' ', time() - YEAR_IN_SECONDS, COOKIEPATH, COOKIE_DOMAIN ); setcookie( LOGGED_IN_COOKIE, ' ', time() - YEAR_IN_SECONDS, SITECOOKIEPATH, COOKIE_DOMAIN ); // Settings cookies. setcookie( 'wp-settings-' . get_current_user_id(), ' ', time() - YEAR_IN_SECONDS, SITECOOKIEPATH ); setcookie( 'wp-settings-time-' . get_current_user_id(), ' ', time() - YEAR_IN_SECONDS, SITECOOKIEPATH ); // Old cookies. setcookie( AUTH_COOKIE, ' ', time() - YEAR_IN_SECONDS, COOKIEPATH, COOKIE_DOMAIN ); setcookie( AUTH_COOKIE, ' ', time() - YEAR_IN_SECONDS, SITECOOKIEPATH, COOKIE_DOMAIN ); setcookie( SECURE_AUTH_COOKIE, ' ', time() - YEAR_IN_SECONDS, COOKIEPATH, COOKIE_DOMAIN ); setcookie( SECURE_AUTH_COOKIE, ' ', time() - YEAR_IN_SECONDS, SITECOOKIEPATH, COOKIE_DOMAIN ); // Even older cookies. setcookie( USER_COOKIE, ' ', time() - YEAR_IN_SECONDS, COOKIEPATH, COOKIE_DOMAIN ); setcookie( PASS_COOKIE, ' ', time() - YEAR_IN_SECONDS, COOKIEPATH, COOKIE_DOMAIN ); setcookie( USER_COOKIE, ' ', time() - YEAR_IN_SECONDS, SITECOOKIEPATH, COOKIE_DOMAIN ); setcookie( PASS_COOKIE, ' ', time() - YEAR_IN_SECONDS, SITECOOKIEPATH, COOKIE_DOMAIN ); // Post password cookie. setcookie( 'wp-postpass_' . COOKIEHASH, ' ', time() - YEAR_IN_SECONDS, COOKIEPATH, COOKIE_DOMAIN );
Which usually means 19 cookies for a login. This itself may represent up to 2.4 kB of header size for that sole purpose. (Let's remind that many reverse-proxy has arbitrary limitation. Eg: HTTP2 push on Cloudflare at 3kB)
An obvious first question is why decade-old cookies are still set instead of the minimal 11 cookies.
We can also observe that in most configurations, COOKIEPATH = /, ADMIN_COOKIE_PATH and SITECOOKIEPATH are either equal or a subpath of COOKIEPATH. As a consequence, these additional granular-path cookies are useless because the cookie is already set for the whole domain. This could further remove 2 or 3 cookies.
Couldn't this be number of cookies halved?
Attachments (2)
Change History (10)
#2
@
21 months ago
- Milestone changed from Awaiting Review to 6.4
Moving to 6.4 to get more eyes on this.
#3
@
21 months ago
Thanks for the ticket and the patch!
Some history here:
- [6387] / #5367 introduced
wp_clear_auth_cookie()
. - [7998] / #7001 introduced
SECURE_AUTH_COOKIE
. - [8069] / #7001 introduced
LOGGED_IN_COOKIE
. - [8197] / #7001 added the cookies that are now in the "Old cookies" section.
- [8209] / #7001 introduced
PLUGINS_COOKIE_PATH
andADMIN_COOKIE_PATH
.
Looking at strip-cookies.2.patch, I'm a bit confused by this conditional:
if ( strpos( PLUGINS_COOKIE_PATH, ADMIN_COOKIE_PATH ) !== 0 ) { ... }
By default, on a clean install the values are:
COOKIEPATH: / ADMIN_COOKIE_PATH: /wp-admin PLUGINS_COOKIE_PATH: /wp-content/plugins
Or, when installed in a subfolder:
COOKIEPATH: /subfolder/ ADMIN_COOKIE_PATH: /subfolder/wp-admin PLUGINS_COOKIE_PATH: /subfolder/wp-content/plugins
So I'm not sure why we're comparing PLUGINS_COOKIE_PATH
and ADMIN_COOKIE_PATH
here, as they would always be different on a typical install. Is there a scenario where ADMIN_COOKIE_PATH
is a subpath of PLUGINS_COOKIE_PATH
?
On a related note, we can use str_starts_with()
instead of strpos()
here, see #58012.
We can also observe that in most configurations, COOKIEPATH = /, ADMIN_COOKIE_PATH and SITECOOKIEPATH are either equal or a subpath of COOKIEPATH. As a consequence, these additional granular-path cookies are useless because the cookie is already set for the whole domain.
Indeed, but it looks like AUTH_COOKIE
and SECURE_AUTH_COOKIE
are only set for COOKIEPATH
in the "Old cookies" section, which this patch removes. I'm curious what the consequences of that could be, as well as what could happen if wp_clear_auth_cookie()
no longer clears the "Even older cookies".
#4
@
20 months ago
- Keywords has-patch changes-requested added
@drzraf can you please address the comment:3?
#5
@
19 months ago
Hi @drzraf! Could you review the comment:3 and update the patch for this ticket? We're looking to include it in the 6.4 release and only have a couple of weeks left.
#6
@
19 months ago
- Keywords dev-feedback added
- Milestone changed from 6.4 to 6.5
We are in 10 days before RC1, and this ticket need more investigation, so, I am moving it to the next milestone.
strip cookies example patch