Make WordPress Core

Opened 9 years ago

Closed 7 years ago

Last modified 7 years ago

#32567 closed defect (bug) (fixed)

Cookies not being deleted.

Reported by: shanee's profile shanee Owned by: johnbillion's profile johnbillion
Milestone: 4.8 Priority: normal
Severity: normal Version: 2.7
Component: Users Keywords: has-patch
Focuses: Cc:

Description

Hello. I noticed that a Wordpress site I had was storing 53 cookies on my computer.

Internet Explorer only allows a domain name to set 50 so this would cause issues (potentially being very hard to debug).

20 of these cookies are from Wordpress "wp-settings-" and "wp-settings-time-" cookies.

I think that these cookies should be deleted after you log out rather than persisting (as they are stored in a database anyway).

Below is a patch that would delete these on log out. (It's a change to the wp_clear_auth_cookie function.)

Index: wp-includes/pluggable.php
===================================================================
--- wp-includes/pluggable.php	(revision 31001)
+++ wp-includes/pluggable.php	(working copy)
@@ -937,6 +937,8 @@
 	 */
 	do_action( 'clear_auth_cookie' );
 
+	setcookie( 'wp-settings-time-' . get_current_user_id(), ' ', time() - YEAR_IN_SECONDS, COOKIEPATH, COOKIE_DOMAIN );
+	setcookie( 'wp-settings-' . get_current_user_id(), ' ', time() - YEAR_IN_SECONDS, COOKIEPATH, COOKIE_DOMAIN );
 	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 );

I'm sorry if this is a bit messy. Additionally, sorry if this is the intended behaviour.

Kind regards,
Shanee Vanstone.

Attachments (2)

32567.1.patch (757 bytes) - added by soulseekah 7 years ago.
32567.2.patch (747 bytes) - added by soulseekah 7 years ago.
COOKIEPATH vs SITECOOKIEPATH

Download all attachments as: .zip

Change History (10)

#1 @johnbillion
9 years ago

  • Component changed from General to Options, Meta APIs
  • Keywords needs-patch added
  • Version changed from 4.2.2 to 2.7

Thanks for the report, shanee. I agree that these cookies should be cleared when the user logs out.

Do you know how you've come to have fifty of these cookies in place? Are you running multisite? Or are you switching between users on the site?

#2 follow-up: @shanee
9 years ago

Hello. Thank you for your quick reply.

I have 53 cookies in total for my domain but only 20 of them are from this bug (presumably from 10 different users).

There's 3 separate installs for different languages (/de and /es for example) although they're not set up as a multisite.

We have a large Wordpress website with quite a lot of staff. I will sometimes log in with an alternate account or a staff member will log in on my computer. As the cookies last for a year, 10 different users is very possible.

If this is patched do you think that deleting a single cookie (as above) is the correct way or would it be better to loop though all cookies similar to this and clear each in turn (in case multiple were present)?

Kind regards,
Shanee Vanstone.

#3 @soulseekah
7 years ago

Interested in seeing this as well as this impacts caching buckets which rely on cookies; once a user is logged out they should get cached content back from the general non-logged-in bucket, but since these unique cookies remain dangling a unique bucket is considered, hurting cache-hit rates.

@soulseekah
7 years ago

#4 @johnbillion
7 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Awaiting Review to 4.8
  • Owner set to johnbillion
  • Status changed from new to reviewing

#5 in reply to: ↑ 2 @soulseekah
7 years ago

Replying to shanee:

If this is patched do you think that deleting a single cookie (as above) is the correct way or would it be better to loop though all cookies similar to this and clear each in turn (in case multiple were present)?

Should absolutely not mess with any of the cookies from other installations unless a logout was explicitly requested for those user sessions. Besides, once/if this behavior accepted you won't be accumulating settings cookies this way so it wouldn't be an issue.

@soulseekah
7 years ago

COOKIEPATH vs SITECOOKIEPATH

#6 @johnbillion
7 years ago

  • Component changed from Options, Meta APIs to Users

#7 @johnbillion
7 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 40580:

Users: Clear the user settings cookies when clearing auth cookies.

This prevents lingering cookies when logging out and when switching between user accounts.

Props soulseekah, shanee
Fixes #32567

#8 @lkraav
7 years ago

@johnbillion I'm fairly certain we have a regression with [40580]. iThemes Security Pro is now infinite looping when auth cookie becomes expired.

Specifically, their core/modules/hide-backend/class-itsec-hide-backend.php module does this:

add_action( 'auth_cookie_expired', array( $this, 'auth_cookie_expired' ) );

and then

/**
         * Lets the module know that this is a reauthorization
         *
         * @since 4.1
         *
         * @return void
         */
        public function auth_cookie_expired() {

                $this->auth_cookie_expired = true;
                wp_clear_auth_cookie();

        }

I'm fairly certain that you calling get_current_user_id() in [40580] triggers a cookie loop. Reverting this patch immediately restores correct operation.

Your thoughts? I vote re-open and investigate.

Version 0, edited 7 years ago by lkraav (next)
Note: See TracTickets for help on using tickets.