Only delete cookies which are actually set

Some API clients seem to be confused by cookie deletion.
Prevent cookie deletion on the first leg of the API login sequence
(for a client with an empty cookie jar) by only emitting deletion
headers for cookies which are set in the current request.

Bug: T124252
Change-Id: I180e094ea32f951e22adab2ec87d16e5de7cef97

diff --git a/includes/session/CookieSessionProvider.php b/includes/session/CookieSessionProvider.php
index 7854416..867c1f8 100644 (file)
--- a/includes/session/CookieSessionProvider.php
+++ b/includes/session/CookieSessionProvider.php
@@ -188,7 +188,7 @@ class CookieSessionProvider extends SessionProvider {
 
                foreach ( $cookies as $key => $value ) {
                        if ( $value === false ) {
-                               $response->clearCookie( $key, $options );
+                               $this->clearCookie( $request, $response, $key, $options );
                        } else {
                                if ( $extendedExpiry !== null && in_array( $key, $extendedCookies ) ) {
                                        $expiry = time() + (int)$extendedExpiry;
@@ -219,15 +219,14 @@ class CookieSessionProvider extends SessionProvider {
                        'Token' => false,
                );
 
-               $response->clearCookie(
-                       $this->params['sessionName'], array( 'prefix' => '' ) + $this->cookieOptions
-               );
+               $this->clearCookie( $request, $response,  $this->params['sessionName'],
+                       array( 'prefix' => '' ) + $this->cookieOptions );
 
                foreach ( $cookies as $key => $value ) {
-                       $response->clearCookie( $key, $this->cookieOptions );
+                       $this->clearCookie( $request, $response, $key, $this->cookieOptions );
                }
 
-               $response->clearCookie( 'forceHTTPS',
+               $this->clearCookie( $request, $response, 'forceHTTPS',
                        array( 'prefix' => '', 'secure' => false ) + $this->cookieOptions );
        }
 
@@ -299,6 +298,23 @@ class CookieSessionProvider extends SessionProvider {
                return $value;
        }
 
+       /**
+        * Delete a cookie. Contains an auth-specific hack.
+        * @param \WebRequest $request
+        * @param \WebResponse $response
+        * @param string $key
+        * @param array $options
+        */
+       protected function clearCookie( $request, $response, $key, $options = array() ) {
+               global $wgCookiePrefix;
+
+               $prefix = isset( $options['prefix'] ) ? $options['prefix'] : $wgCookiePrefix;
+
+               if ( $request->getCookie( $key, $prefix ) ) {
+                       $response->clearCookie( $key, $options );
+               }
+       }
+
        /**
         * Return the data to store in cookies
         * @param User $user

diff --git a/tests/phpunit/includes/session/CookieSessionProviderTest.php b/tests/phpunit/includes/session/CookieSessionProviderTest.php
index a73bf7c..46fb0dd 100644 (file)
--- a/tests/phpunit/includes/session/CookieSessionProviderTest.php
+++ b/tests/phpunit/includes/session/CookieSessionProviderTest.php
@@ -379,10 +379,10 @@ class CookieSessionProviderTest extends MediaWikiTestCase {
                $request = new \FauxRequest();
                $provider->persistSession( $backend, $request );
                $this->assertSame( $sessionId, $request->response()->getCookie( 'MySessionName' ) );
-               $this->assertSame( '', $request->response()->getCookie( 'xUserID' ) );
-               $this->assertSame( null, $request->response()->getCookie( 'xUserName' ) );
-               $this->assertSame( '', $request->response()->getCookie( 'xToken' ) );
-               $this->assertSame( null, $request->response()->getCookie( 'forceHTTPS' ) );
+               $this->assertNull( $request->response()->getCookie( 'xUserID' ) );
+               $this->assertNull( $request->response()->getCookie( 'xUserName' ) );
+               $this->assertNull( $request->response()->getCookie( 'xToken' ) );
+               $this->assertNull( $request->response()->getCookie( 'forceHTTPS' ) );
                $this->assertSame( array(), $backend->getData() );
 
                // Logged-in user, no remember
@@ -394,8 +394,8 @@ class CookieSessionProviderTest extends MediaWikiTestCase {
                $this->assertSame( $sessionId, $request->response()->getCookie( 'MySessionName' ) );
                $this->assertSame( (string)$user->getId(), $request->response()->getCookie( 'xUserID' ) );
                $this->assertSame( $user->getName(), $request->response()->getCookie( 'xUserName' ) );
-               $this->assertSame( '', $request->response()->getCookie( 'xToken' ) );
-               $this->assertSame( null, $request->response()->getCookie( 'forceHTTPS' ) );
+               $this->assertNull( $request->response()->getCookie( 'xToken' ) );
+               $this->assertNull( $request->response()->getCookie( 'forceHTTPS' ) );
                $this->assertSame( array(), $backend->getData() );
 
                // Logged-in user, remember
@@ -484,9 +484,9 @@ class CookieSessionProviderTest extends MediaWikiTestCase {
                        'xUserName' => array(
                                'value' => $user->getName(),
                        ) + $defaults,
-                       'xToken' => array(
-                               'value' => $remember ? $user->getToken() : '',
-                               'expire' => $remember ? $extendedExpiry : -31536000,
+                       'xToken' => !$remember ? null : array(
+                               'value' => $user->getToken(),
+                               'expire' => $extendedExpiry,
                        ) + $defaults,
                        'forceHTTPS' => !$secure ? null : array(
                                'value' => 'true',
@@ -568,10 +568,10 @@ class CookieSessionProviderTest extends MediaWikiTestCase {
                $request = new \FauxRequest();
                $provider->persistSession( $backend, $request );
                $this->assertSame( $sessionId, $request->response()->getCookie( 'MySessionName' ) );
-               $this->assertSame( '', $request->response()->getCookie( 'xUserID' ) );
-               $this->assertSame( null, $request->response()->getCookie( 'xUserName' ) );
-               $this->assertSame( '', $request->response()->getCookie( 'xToken' ) );
-               $this->assertSame( null, $request->response()->getCookie( 'forceHTTPS' ) );
+               $this->assertNull( $request->response()->getCookie( 'xUserID' ) );
+               $this->assertNull( $request->response()->getCookie( 'xUserName' ) );
+               $this->assertNull( $request->response()->getCookie( 'xToken' ) );
+               $this->assertNull( $request->response()->getCookie( 'forceHTTPS' ) );
                $this->assertSame( array(), $backend->getData() );
 
                $provider->persistSession( $backend, $this->getSentRequest() );
@@ -606,8 +606,8 @@ class CookieSessionProviderTest extends MediaWikiTestCase {
                $this->assertSame( $sessionId, $request->response()->getCookie( 'MySessionName' ) );
                $this->assertSame( (string)$user->getId(), $request->response()->getCookie( 'xUserID' ) );
                $this->assertSame( $user->getName(), $request->response()->getCookie( 'xUserName' ) );
-               $this->assertSame( '', $request->response()->getCookie( 'xToken' ) );
-               $this->assertSame( null, $request->response()->getCookie( 'forceHTTPS' ) );
+               $this->assertNull( $request->response()->getCookie( 'xToken' ) );
+               $this->assertNull( $request->response()->getCookie( 'forceHTTPS' ) );
                $this->assertSame( 'bar!', $request->response()->getCookie( 'xbar' ) );
                $this->assertSame( (string)$loggedOut, $request->response()->getCookie( 'xLoggedOut' ) );
                $this->assertEquals( array(
@@ -675,11 +675,11 @@ class CookieSessionProviderTest extends MediaWikiTestCase {
 
                $request = new \FauxRequest();
                $provider->unpersistSession( $request );
-               $this->assertSame( '', $request->response()->getCookie( 'MySessionName' ) );
-               $this->assertSame( '', $request->response()->getCookie( 'xUserID' ) );
-               $this->assertSame( null, $request->response()->getCookie( 'xUserName' ) );
-               $this->assertSame( '', $request->response()->getCookie( 'xToken' ) );
-               $this->assertSame( '', $request->response()->getCookie( 'forceHTTPS' ) );
+               $this->assertNull( $request->response()->getCookie( 'MySessionName' ) );
+               $this->assertNull( $request->response()->getCookie( 'xUserID' ) );
+               $this->assertNull( $request->response()->getCookie( 'xUserName' ) );
+               $this->assertNull( $request->response()->getCookie( 'xToken' ) );
+               $this->assertNull( $request->response()->getCookie( 'forceHTTPS' ) );
 
                $provider->unpersistSession( $this->getSentRequest() );
        }