Move avoidance of setting deleted cookies into WebResponse

There's no reason this should be only in CookieSessionProvider when
we're already handling deduplication in WebResponse.

Further, this fixes the bug in the existing CookieSessionProvider
implementation that a setCookie() followed by a clearCookie() wouldn't
actually clear the cookie.

This reverts commit 1ce684fcef1ee69ca0921c05081cae47f90939e5.

Bug: T124252
Change-Id: I1098d054facacd59f03ebed7c747ec9ff6bf66e7
Depends-On: I61d14bf80fa7c857dec9cffb366dc3f84dbb4faf

diff --git a/includes/WebResponse.php b/includes/WebResponse.php
index f14cf22..7746edd 100644 (file)
--- a/includes/WebResponse.php
+++ b/includes/WebResponse.php
@@ -131,23 +131,38 @@ class WebResponse {
                if ( Hooks::run( 'WebResponseSetCookie', array( &$name, &$value, &$expire, $options ) ) ) {
                        $cookie = $options['prefix'] . $name;
                        $data = array(
-                               (string)$cookie,
-                               (string)$value,
-                               (int)$expire,
-                               (string)$options['path'],
-                               (string)$options['domain'],
-                               (bool)$options['secure'],
-                               (bool)$options['httpOnly'],
+                               'name' => (string)$cookie,
+                               'value' => (string)$value,
+                               'expire' => (int)$expire,
+                               'path' => (string)$options['path'],
+                               'domain' => (string)$options['domain'],
+                               'secure' => (bool)$options['secure'],
+                               'httpOnly' => (bool)$options['httpOnly'],
                        );
-                       if ( !isset( self::$setCookies[$cookie] ) ||
-                               self::$setCookies[$cookie] !== array( $func, $data )
+
+                       // Per RFC 6265, key is name + domain + path
+                       $key = "{$data['name']}\n{$data['domain']}\n{$date['path']}";
+
+                       // If this cookie name was in the request, fake an entry in
+                       // self::$setCookies for it so the deleting check works right.
+                       if ( isset( $_COOKIE[$cookie] ) && !array_key_exists( $key, self::$setCookies ) ) {
+                               self::$setCookies[$key] = array();
+                       }
+
+                       // PHP deletes if value is the empty string; also, a past expiry is deleting
+                       $deleting = ( $data['value'] === '' || $data['expire'] > 0 && $data['expire'] <= time() );
+
+                       if ( $deleting && !isset( self::$setCookies[$key] ) ) { // isset( null ) is false
+                               wfDebugLog( 'cookie', 'already deleted ' . $func . ': "' . implode( '", "', $data ) . '"' );
+                       } elseif ( !$deleting && isset( self::$setCookies[$key] ) &&
+                               self::$setCookies[$key] === array( $func, $data )
                        ) {
+                               wfDebugLog( 'cookie', 'already set ' . $func . ': "' . implode( '", "', $data ) . '"' );
+                       } else {
                                wfDebugLog( 'cookie', $func . ': "' . implode( '", "', $data ) . '"' );
-                               if ( call_user_func_array( $func, $data ) ) {
-                                       self::$setCookies[$cookie] = array( $func, $data );
+                               if ( call_user_func_array( $func, array_values( $data ) ) ) {
+                                       self::$setCookies[$key] = $deleting ? null : array( $func, $data );
                                }
-                       } else {
-                               wfDebugLog( 'cookie', 'already set ' . $func . ': "' . implode( '", "', $data ) . '"' );
                        }
                }
        }

diff --git a/includes/session/CookieSessionProvider.php b/includes/session/CookieSessionProvider.php
index 867c1f8..7854416 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 ) {
-                               $this->clearCookie( $request, $response, $key, $options );
+                               $response->clearCookie( $key, $options );
                        } else {
                                if ( $extendedExpiry !== null && in_array( $key, $extendedCookies ) ) {
                                        $expiry = time() + (int)$extendedExpiry;
@@ -219,14 +219,15 @@ class CookieSessionProvider extends SessionProvider {
                        'Token' => false,
                );
 
-               $this->clearCookie( $request, $response,  $this->params['sessionName'],
-                       array( 'prefix' => '' ) + $this->cookieOptions );
+               $response->clearCookie(
+                       $this->params['sessionName'], array( 'prefix' => '' ) + $this->cookieOptions
+               );
 
                foreach ( $cookies as $key => $value ) {
-                       $this->clearCookie( $request, $response, $key, $this->cookieOptions );
+                       $response->clearCookie( $key, $this->cookieOptions );
                }
 
-               $this->clearCookie( $request, $response, 'forceHTTPS',
+               $response->clearCookie( 'forceHTTPS',
                        array( 'prefix' => '', 'secure' => false ) + $this->cookieOptions );
        }
 
@@ -298,23 +299,6 @@ 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 46fb0dd..a73bf7c 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->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( '', $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->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->assertNull( $request->response()->getCookie( 'xToken' ) );
-               $this->assertNull( $request->response()->getCookie( 'forceHTTPS' ) );
+               $this->assertSame( '', $request->response()->getCookie( 'xToken' ) );
+               $this->assertSame( null, $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' => !$remember ? null : array(
-                               'value' => $user->getToken(),
-                               'expire' => $extendedExpiry,
+                       'xToken' => array(
+                               'value' => $remember ? $user->getToken() : '',
+                               'expire' => $remember ? $extendedExpiry : -31536000,
                        ) + $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->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( '', $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->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->assertNull( $request->response()->getCookie( 'xToken' ) );
-               $this->assertNull( $request->response()->getCookie( 'forceHTTPS' ) );
+               $this->assertSame( '', $request->response()->getCookie( 'xToken' ) );
+               $this->assertSame( null, $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->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' ) );
+               $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' ) );
 
                $provider->unpersistSession( $this->getSentRequest() );
        }