Fix for CLN-006 in ServerController

Resolves CWE-601 - URL Redirection to Untrusted Site ('Open Redirect')

Author: Potherca

solid/lib/Controller/ServerController.php

@@ -220,6 +220,26 @@ public function authorize() {
 			return $result; // ->addHeader('Access-Control-Allow-Origin', '*');
 		}
 
+		if (isset($getVars['redirect_uri'])) {
+			$redirectUri = $getVars['redirect_uri'];
+			if (! isset($clientRegistration['redirect_uris']) || ! is_array($clientRegistration['redirect_uris'])) {
+				return new JSONResponse('Invalid client registration, no redirect URIs found', Http::STATUS_BAD_REQUEST);
+			}
+
+			$redirectUris = $clientRegistration['redirect_uris'];
+
+			$validRedirectUris = array_filter($redirectUris, function ($uri) use ($redirectUri) {
+				// @CHECKME: Does either URI need to be normalized when it is a URL?
+				//           For instance, anchors `#` or query parameters `?`
+				return $uri === $redirectUri;
+			});
+
+			if (count($validRedirectUris) === 0) {
+				return new JSONResponse('Provided redirect URI does not match any registered URIs', Http::STATUS_BAD_REQUEST);
+			}
+		}
+
+		// @CHECKME: Can more than one redirect_uri could be provided for custom schemes?
 		$parsedOrigin = parse_url($clientRegistration['redirect_uris'][0]);
 		if (
 			$parsedOrigin['scheme'] != "https" &&

solid/tests/Unit/Controller/ServerControllerTest.php

@@ -171,19 +171,14 @@ public function testAuthorizeWithoutApprovedClient()
 	}
 
 	/**
-	 * @testdox ServerController should return a 302 redirect when asked to authorize client that has been approved
+	 * @testdox ServerController should return a 400 when asked to authorize a client that sends an incorrect redirect URI
 	 *
 	 * @covers ::authorize
 	 */
-	public function testAuthorizeWithApprovedClient()
+	public function testAuthorizeWithInvalidRedirectUri()
 	{
 		$_GET['client_id'] = self::MOCK_CLIENT_ID;
-		$_GET['nonce'] = 'mock-nonce';
-		// JWT with empty payload, HS256 encoded, created with `private.key` from fixtures
-		$_GET['request'] = 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.e30.8VKCTiBegJPuPIZlp0wbV0Sbdn5BS6TE5DCx6oYNc5o';
-		$_GET['response_type'] = 'mock-response-type';
-
-		$_SERVER['REQUEST_URI'] = 'https://mock.server';
+        $_GET['redirect_uri'] = 'https://some.other.client/redirect';
 
 		$clientData = json_encode(['client_name' => 'Mock Client', 'redirect_uris' => ['https://mock.client/redirect']]);
 
@@ -200,15 +195,15 @@ public function testAuthorizeWithApprovedClient()
 		$response = $controller->authorize();
 
 		$expected = [
-			'data' => 'ok',
+			'data' => 'Provided redirect URI does not match any registered URIs',
 			'headers' => [
 				'Cache-Control' => 'no-cache, no-store, must-revalidate',
 				'Content-Security-Policy' => "default-src 'none';base-uri 'none';manifest-src 'self';frame-ancestors 'none'",
 				'Content-Type' => 'application/json; charset=utf-8',
 				'Feature-Policy' => "autoplay 'none';camera 'none';fullscreen 'none';geolocation 'none';microphone 'none';payment 'none'",
 				'X-Robots-Tag' => 'noindex, nofollow',
 			],
-			'status' => Http::STATUS_FOUND,
+			'status' => Http::STATUS_BAD_REQUEST,
 		];
 
 		$actual = [
@@ -217,32 +212,87 @@ public function testAuthorizeWithApprovedClient()
 			'status' => $response->getStatus(),
 		];
 
-		$location = $actual['headers']['Location'];
-
 		// Not comparing time-sensitive data
-		unset($actual['headers']['X-Request-Id'], $actual['headers']['Location']);
+		unset($actual['headers']['X-Request-Id']);
 
 		$this->assertEquals($expected, $actual);
-
-		// @TODO: Move $location assert to a separate test
-		$url = parse_url($location);
-
-		parse_str($url['fragment'], $url['fragment']);
-
-		unset($url['fragment']['access_token'], $url['fragment']['id_token']);
-
-		$this->assertEquals([
-			'scheme' => 'https',
-			'host' => 'mock.client',
-			'path' => '/redirect',
-			'fragment' => [
-				'token_type' => 'Bearer',
-				'expires_in' => '3600',
-			],
-		], $url);
 	}
 
-	/**
+    /**
+     * @testdox ServerController should return a 302 redirect when asked to authorize client that has been approved
+     *
+     * @covers ::authorize
+     */
+    public function testAuthorize()
+    {
+        $_GET['client_id'] = self::MOCK_CLIENT_ID;
+        $_GET['nonce'] = 'mock-nonce';
+        // JWT with empty payload, HS256 encoded, created with `private.key` from fixtures
+        $_GET['request'] = 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.e30.8VKCTiBegJPuPIZlp0wbV0Sbdn5BS6TE5DCx6oYNc5o';
+        $_GET['response_type'] = 'mock-response-type';
+        $_GET['redirect_uri'] = 'https://mock.client/redirect';
+
+        $_SERVER['REQUEST_URI'] = 'https://mock.server';
+        $_SERVER['REQUEST_URI'];
+
+        $clientData = json_encode(['client_name' => 'Mock Client', 'redirect_uris' => ['https://mock.client/redirect']]);
+
+        $parameters = $this->createMockConstructorParameters($clientData);
+
+        $this->mockConfig->method('getUserValue')
+            ->with(self::MOCK_USER_ID, Application::APP_ID, 'allowedClients', '[]')
+            ->willReturn(json_encode([self::MOCK_CLIENT_ID]));
+
+        $this->mockUserManager->method('userExists')->willReturn(true);
+
+        $controller = new ServerController(...$parameters);
+
+        $response = $controller->authorize();
+
+        $expected = [
+            'data' => 'ok',
+            'headers' => [
+                'Cache-Control' => 'no-cache, no-store, must-revalidate',
+                'Content-Security-Policy' => "default-src 'none';base-uri 'none';manifest-src 'self';frame-ancestors 'none'",
+                'Content-Type' => 'application/json; charset=utf-8',
+                'Feature-Policy' => "autoplay 'none';camera 'none';fullscreen 'none';geolocation 'none';microphone 'none';payment 'none'",
+                'X-Robots-Tag' => 'noindex, nofollow',
+            ],
+            'status' => Http::STATUS_FOUND,
+        ];
+
+        $actual = [
+            'data' => $response->getData(),
+            'headers' => $response->getHeaders(),
+            'status' => $response->getStatus(),
+        ];
+
+        $location = $actual['headers']['Location'] ?? '';
+
+        // Not comparing time-sensitive data
+        unset($actual['headers']['X-Request-Id'], $actual['headers']['Location']);
+
+        $this->assertEquals($expected, $actual);
+
+        // @TODO: Move $location assert to a separate test
+        $url = parse_url($location);
+
+        parse_str($url['fragment'], $url['fragment']);
+
+        unset($url['fragment']['access_token'], $url['fragment']['id_token']);
+
+        $this->assertEquals([
+            'scheme' => 'https',
+            'host' => 'mock.client',
+            'path' => '/redirect',
+            'fragment' => [
+                'token_type' => 'Bearer',
+                'expires_in' => '3600',
+            ],
+        ], $url);
+    }
+
+    /**
 	 * @testdox ServerController should return a 400 when asked to register without valid client data
 	 *
 	 * @covers ::register