From 75f4f2a3a5fe70ad140a14219e4913ce8c05a998 Mon Sep 17 00:00:00 2001
From: =?utf8?q?Tim=20D=C3=BCsterhus?= <duesterhus@woltlab.com>
Date: Thu, 7 Jan 2021 16:29:14 +0100
Subject: [PATCH] Add proper error handling to OAuth2 client

---
 .../lib/action/AbstractOauth2Action.class.php | 69 ++++++++++++++-----
 .../StateValidationException.class.php        | 17 +++++
 wcfsetup/install/lang/de.xml                  |  3 +
 wcfsetup/install/lang/en.xml                  |  3 +
 4 files changed, 74 insertions(+), 18 deletions(-)
 create mode 100644 wcfsetup/install/files/lib/system/user/authentication/oauth/exception/StateValidationException.class.php

diff --git a/wcfsetup/install/files/lib/action/AbstractOauth2Action.class.php b/wcfsetup/install/files/lib/action/AbstractOauth2Action.class.php
index 7e336a1533..2fe725ac0c 100644
--- a/wcfsetup/install/files/lib/action/AbstractOauth2Action.class.php
+++ b/wcfsetup/install/files/lib/action/AbstractOauth2Action.class.php
@@ -1,11 +1,13 @@
 <?php
 namespace wcf\action;
 use GuzzleHttp\ClientInterface;
+use GuzzleHttp\Exception\GuzzleException;
 use GuzzleHttp\Psr7\Request;
 use ParagonIE\ConstantTime\Hex;
 use wcf\system\exception\NamedUserException;
 use wcf\system\exception\PermissionDeniedException;
 use wcf\system\io\HttpFactory;
+use wcf\system\user\authentication\oauth\exception\StateValidationException;
 use wcf\system\user\authentication\oauth\User as OauthUser;
 use wcf\system\WCF;
 use wcf\util\HeaderUtil;
@@ -105,13 +107,13 @@ abstract class AbstractOauth2Action extends AbstractAction {
 	 */
 	protected function validateState() {
 		if (!isset($_GET['state'])) {
-			throw new \Exception('Missing state parameter');
+			throw new StateValidationException('Missing state parameter');
 		}
 		if (!($sessionState = WCF::getSession()->getVar(self::STATE))) {
-			throw new \Exception('Missing state in session');
+			throw new StateValidationException('Missing state in session');
 		}
 		if (!\hash_equals($sessionState, (string) $_GET['state'])) {
-			throw new \Exception('Mismatching state');
+			throw new StateValidationException('Mismatching state');
 		}
 		
 		WCF::getSession()->unregister(self::STATE);
@@ -132,12 +134,18 @@ abstract class AbstractOauth2Action extends AbstractAction {
 			'code' => $_GET['code'],
 		], '', '&', PHP_QUERY_RFC1738));
 		
-		$response = $this->getHttpClient()->send($request);
-		
-		// Validate state. Validation of state is executed after fetching the
-		// access_token to invalidate 'code'.
-		if ($this->supportsState()) {
-			$this->validateState();
+		try {
+			$response = $this->getHttpClient()->send($request);
+		}
+		finally {
+			// Validate state. Validation of state is executed after fetching the
+			// access_token to invalidate 'code'.
+			//
+			// Validation is happening within the `finally` so that the StateValidationException
+			// overwrites any GuzzleException (improving the error message).
+			if ($this->supportsState()) {
+				$this->validateState();
+			}
 		}
 		
 		$parsed = JSON::decode((string) $response->getBody());
@@ -190,17 +198,42 @@ abstract class AbstractOauth2Action extends AbstractAction {
 	public function execute() {
 		parent::execute();
 		
-		if (isset($_GET['code'])) {
-			$accessToken = $this->codeToAccessToken($_GET['code']);
-			$oauthUser = $this->getUser($accessToken);
-			
-			$this->processUser($oauthUser);
+		try {
+			if (isset($_GET['code'])) {
+				$accessToken = $this->codeToAccessToken($_GET['code']);
+				$oauthUser = $this->getUser($accessToken);
+				
+				$this->processUser($oauthUser);
+			}
+			else if (isset($_GET['error'])) {
+				$this->handleError($_GET['error']);
+			}
+			else {
+				$this->initiate();
+			}
 		}
-		else if (isset($_GET['error'])) {
-			$this->handleError($_GET['error']);
+		catch (NamedUserException | PermissionDeniedException $e) {
+			throw $e;
 		}
-		else {
-			$this->initiate();
+		catch (StateValidationException $e) {
+			throw new NamedUserException(WCF::getLanguage()->getDynamicVariable(
+				'wcf.user.3rdparty.login.error.stateValidation'
+			));
+		}
+		catch (\Exception $e) {
+			$exceptionID = \wcf\functions\exception\logThrowable($e);
+			
+			$type = 'genericException';
+			if ($e instanceof GuzzleException) {
+				$type = 'httpError';
+			}
+			
+			throw new NamedUserException(WCF::getLanguage()->getDynamicVariable(
+				'wcf.user.3rdparty.login.error.'.$type,
+				[
+					'exceptionID' => $exceptionID,
+				]
+			));
 		}
 	}
 }
diff --git a/wcfsetup/install/files/lib/system/user/authentication/oauth/exception/StateValidationException.class.php b/wcfsetup/install/files/lib/system/user/authentication/oauth/exception/StateValidationException.class.php
new file mode 100644
index 0000000000..64ed5db9f7
--- /dev/null
+++ b/wcfsetup/install/files/lib/system/user/authentication/oauth/exception/StateValidationException.class.php
@@ -0,0 +1,17 @@
+<?php
+namespace wcf\system\user\authentication\oauth\exception;
+
+/**
+ * Thrown when the CSRF 'state' parameter cannot be validated.
+ * 
+ * @author	Tim Duesterhus
+ * @copyright	2001-2021 WoltLab GmbH
+ * @license	GNU Lesser General Public License <http://opensource.org/licenses/lgpl-license.php>
+ * @package	WoltLabSuite\Core\System\User\Authentication\Oauth\Exception
+ * @since	5.4
+ */
+final class StateValidationException extends \UnexpectedValueException {
+	public function __construct($message, ?\Throwable $previous = null) {
+		parent::__construct($message, 0, $previous);
+	}
+}
diff --git a/wcfsetup/install/lang/de.xml b/wcfsetup/install/lang/de.xml
index b80ceac351..f8f1c1529b 100644
--- a/wcfsetup/install/lang/de.xml
+++ b/wcfsetup/install/lang/de.xml
@@ -5068,6 +5068,9 @@ Die E-Mail-Adresse des neuen Benutzers lautet: {@$user->email}
 		<item name="wcf.user.3rdparty"><![CDATA[Drittanbieter]]></item>
 		<item name="wcf.user.3rdparty.login.error.access_denied"><![CDATA[Da {if LANGUAGE_USE_INFORMAL_VARIANT}du die Berechtigungen verweigert hast{else}Sie die Berechtigungen verweigert haben{/if}, ist die Anmeldung nicht möglich.]]></item>
 		<item name="wcf.user.3rdparty.login.error.denied"><![CDATA[Da {if LANGUAGE_USE_INFORMAL_VARIANT}du die Berechtigungen verweigert hast{else}Sie die Berechtigungen verweigert haben{/if}, ist die Anmeldung nicht möglich.]]></item>
+		<item name="wcf.user.3rdparty.login.error.httpError"><![CDATA[Die Kommunikation mit dem externen Dienst ist fehlgeschlagen. Interner Fehlercode: <code>{$exceptionID}</code>.]]></item>
+		<item name="wcf.user.3rdparty.login.error.genericException"><![CDATA[Bei der Verarbeitung der Anfrage ist ein Fehler aufgetreten. Bitte {if LANGUAGE_USE_INFORMAL_VARIANT}wende dich{else}wenden Sie sich{/if} an den Administrator. Interner Fehlercode: <code>{$exceptionID}</code>.]]></item>
+		<item name="wcf.user.3rdparty.login.error.stateValidation"><![CDATA[Die Anfrage konnte nicht zugeordnet werden. Möglicherweise ist {if LANGUAGE_USE_INFORMAL_VARIANT}deine{else}Ihre{/if} Sitzung abgelaufen.]]></item>
 		<item name="wcf.user.3rdparty.github"><![CDATA[GitHub]]></item>
 		<item name="wcf.user.3rdparty.github.login"><![CDATA[Mit GitHub anmelden]]></item>
 		<item name="wcf.user.3rdparty.github.register"><![CDATA[{if LANGUAGE_USE_INFORMAL_VARIANT}Du erstellst{else}Sie erstellen{/if} ein Benutzerkonto über <span class="icon icon16 fa-github"></span>&nbsp;GitHub. Der Benutzername und {if LANGUAGE_USE_INFORMAL_VARIANT}deine{else}Ihre{/if} E-Mail-Adresse wurden daher bereits ausgefüllt.]]></item>
diff --git a/wcfsetup/install/lang/en.xml b/wcfsetup/install/lang/en.xml
index 4d6b7768d8..7e659db9a1 100644
--- a/wcfsetup/install/lang/en.xml
+++ b/wcfsetup/install/lang/en.xml
@@ -5065,6 +5065,9 @@ You also received a list of emergency codes to use when your second factor becom
 		<item name="wcf.user.3rdparty"><![CDATA[Third-Party Login]]></item>
 		<item name="wcf.user.3rdparty.login.error.access_denied"><![CDATA[Access to your external account has been rejected.]]></item>
 		<item name="wcf.user.3rdparty.login.error.denied"><![CDATA[Access to your external account has been rejected.]]></item>
+		<item name="wcf.user.3rdparty.login.error.httpError"><![CDATA[Unable to communicate with the external service. Internal error code: <code>{$exceptionID}</code>.]]></item>
+		<item name="wcf.user.3rdparty.login.error.genericException"><![CDATA[An error occurred while processing your authentication request. Please contact the administrator for help. Internal error code: <code>{$exceptionID}</code>.]]></item>
+		<item name="wcf.user.3rdparty.login.error.stateValidation"><![CDATA[Unable to find the authentication request. Maybe your session expired.]]></item>
 		<item name="wcf.user.3rdparty.github"><![CDATA[GitHub]]></item>
 		<item name="wcf.user.3rdparty.github.login"><![CDATA[Login with GitHub]]></item>
 		<item name="wcf.user.3rdparty.github.register"><![CDATA[You are creating an account through <span class="icon icon16 fa-github"></span>&nbsp;GitHub. The username and your email address have therefore already been entered.]]></item>
-- 
2.20.1