Make WordPress Core

Opened 4 years ago

Last modified 3 months ago

#50522 new defect (bug)

stop setting "older" cookies with multiple path prefixes

Reported by: drzraf's profile drzraf 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)

strip-cookies.patch (2.4 KB) - added by drzraf 4 years ago.
strip cookies example patch
strip-cookies.2.patch (2.4 KB) - added by drzraf 4 years ago.

Download all attachments as: .zip

Change History (10)

@drzraf
4 years ago

strip cookies example patch

#1 @SergeyBiryukov
4 years ago

  • Component changed from Administration to Login and Registration

#2 @SergeyBiryukov
8 months ago

  • Milestone changed from Awaiting Review to 6.4

Moving to 6.4 to get more eyes on this.

Last edited 8 months ago by SergeyBiryukov (previous) (diff)

#3 @SergeyBiryukov
8 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 and ADMIN_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 @oglekler
8 months ago

  • Keywords has-patch changes-requested added

@drzraf can you please address the comment:3?

#5 @nicolefurlan
7 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 @oglekler
6 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.

This ticket was mentioned in Slack in #core by rajinsharwar. View the logs.


3 months ago

#8 @rajinsharwar
3 months ago

  • Milestone changed from 6.5 to Future Release

This ticket as discussed during a recent bug scrub, and it was decided to move this into the Future Release milestone.

Note: See TracTickets for help on using tickets.