From 4d6d06253b28ee1fac28301ef596d78c1ba7859b Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Sat, 23 Jan 2016 11:13:11 -0500 Subject: [PATCH] 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 --- includes/WebResponse.php | 41 +++++++++++++------ includes/session/CookieSessionProvider.php | 28 +++---------- .../session/CookieSessionProviderTest.php | 40 +++++++++--------- 3 files changed, 54 insertions(+), 55 deletions(-) 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() ); } -- 2.20.1