From 1ce684fcef1ee69ca0921c05081cae47f90939e5 Mon Sep 17 00:00:00 2001
From: =?utf8?q?Gerg=C5=91=20Tisza?= <tgr.huwiki@gmail.com>
Date: Fri, 22 Jan 2016 14:29:40 -0800
Subject: [PATCH] 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
---
 includes/session/CookieSessionProvider.php    | 28 ++++++++++---
 .../session/CookieSessionProviderTest.php     | 40 +++++++++----------
 2 files changed, 42 insertions(+), 26 deletions(-)

diff --git a/includes/session/CookieSessionProvider.php b/includes/session/CookieSessionProvider.php
index 7854416fa1..867c1f8c3c 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 ) {
-				$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 a73bf7c098..46fb0ddd12 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->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() );
 	}
-- 
2.20.1