Merge pull request #790 from nextcloud/backport/788/stable-5.2

[stable-5.2] refactor(Controller): read parameter only once
nextcloud · Nov 29, 2023 · 3858303 · 3858303
2 parents 61c6a5e + 04b5ed5
commit 3858303
Show file tree

Hide file tree

Showing 2 changed files with 20 additions and 5 deletions.
diff --git a/lib/Controller/SAMLController.php b/lib/Controller/SAMLController.php
@@ -43,6 +43,7 @@
 use OCP\IURLGenerator;
 use OCP\IUserSession;
 use OCP\Security\ICrypto;
+use OCP\Security\ITrustedDomainHelper;
 use OneLogin\Saml2\Auth;
 use OneLogin\Saml2\Error;
 use OneLogin\Saml2\Settings;
@@ -75,6 +76,7 @@ class SAMLController extends Controller {
 	 * @var ICrypto
 	 */
 	private $crypto;
+	private ITrustedDomainHelper $trustedDomainHelper;
 
 	/**
 	 * @param string $appName
@@ -101,7 +103,8 @@ public function __construct(
 		IL10N         $l,
 		UserResolver  $userResolver,
 		UserData      $userData,
-		ICrypto       $crypto
+		ICrypto       $crypto,
+		ITrustedDomainHelper $trustedDomainHelper
 	) {
 		parent::__construct($appName, $request);
 		$this->session = $session;
@@ -115,6 +118,7 @@ public function __construct(
 		$this->userResolver = $userResolver;
 		$this->userData = $userData;
 		$this->crypto = $crypto;
+		$this->trustedDomainHelper = $trustedDomainHelper;
 	}
 
 	/**
@@ -204,11 +208,17 @@ protected function assertGroupMemberships(): void {
 	 * @throws \Exception
 	 */
 	public function login(int $idp = 1) {
+		$originalUrl = (string)$this->request->getParam('originalUrl', '');
+		if (!$this->trustedDomainHelper->isTrustedUrl($originalUrl)) {
+			$originalUrl = '';
+		}
+
 		$type = $this->config->getAppValue($this->appName, 'type');
 		switch ($type) {
 			case 'saml':
 				$auth = new Auth($this->samlSettings->getOneLoginSettingsArray($idp));
-				$returnUrl = $this->request->getParam('originalUrl', $this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.login'));
+
+				$returnUrl = $originalUrl ?: $this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.login');
 				$ssoUrl = $auth->login($returnUrl, [], false, false, true);
 				$response = new Http\RedirectResponse($ssoUrl);
 
@@ -227,7 +237,7 @@ public function login(int $idp = 1) {
 				// Pack data as JSON so we can properly extract it later
 				$data = json_encode([
 					'AuthNRequestID' => $auth->getLastRequestID(),
-					'OriginalUrl' => $this->request->getParam('originalUrl', ''),
+					'OriginalUrl' => $originalUrl,
 					'Idp' => $idp,
 					'flow' => $flowData,
 				]);
@@ -241,7 +251,7 @@ public function login(int $idp = 1) {
 				$response->addCookie('saml_data', $data, null, 'None');
 				break;
 			case 'environment-variable':
-				$ssoUrl = $this->request->getParam('originalUrl', '');
+				$ssoUrl = $originalUrl;
 				if (empty($ssoUrl)) {
 					$ssoUrl = $this->urlGenerator->getAbsoluteURL('/');
 				}

diff --git a/tests/unit/Controller/SAMLControllerTest.php b/tests/unit/Controller/SAMLControllerTest.php
@@ -40,6 +40,7 @@
 use OCP\IUser;
 use OCP\IUserSession;
 use OCP\Security\ICrypto;
+use OCP\Security\ITrustedDomainHelper;
 use PHPUnit\Framework\MockObject\MockObject;
 use Test\TestCase;
 
@@ -70,6 +71,8 @@ class SAMLControllerTest extends TestCase {
 	private $crypto;
 	/** @var SAMLController */
 	private $samlController;
+	/** @var ITrustedDomainHelper|MockObject */
+	private $trustedDomainController;
 
 	protected function setUp(): void {
 		parent::setUp();
@@ -86,6 +89,7 @@ protected function setUp(): void {
 		$this->userResolver = $this->createMock(UserResolver::class);
 		$this->userData = $this->createMock(UserData::class);
 		$this->crypto = $this->createMock(ICrypto::class);
+		$this->trustedDomainController = $this->createMock(ITrustedDomainHelper::class);
 
 		$this->l->expects($this->any())->method('t')->willReturnCallback(
 			function ($param) {
@@ -111,7 +115,8 @@ function ($param) {
 			$this->l,
 			$this->userResolver,
 			$this->userData,
-			$this->crypto
+			$this->crypto,
+			$this->trustedDomainController
 		);
 	}