From: Brad Jorsch Date: Sat, 23 Jan 2016 16:13:11 +0000 (-0500) Subject: Move avoidance of setting deleted cookies into WebResponse X-Git-Tag: 1.31.0-rc.0~8230 X-Git-Url: http://git.cyclocoop.org/%22.%24h.%22?a=commitdiff_plain;h=4d6d06253b28ee1fac28301ef596d78c1ba7859b;p=lhc%2Fweb%2Fwiklou.git 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 f14cf2289c..7746edd147 100644 --- 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 867c1f8c3c..7854416fa1 100644 --- 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 46fb0ddd12..a73bf7c098 100644 --- 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() ); }