Add more input validation in ServerController.

Author: Potherca

solid/lib/Controller/ServerController.php

@@ -156,6 +156,10 @@ public function authorize() {
 //			return $result->addHeader('Access-Control-Allow-Origin', '*');
 		}
 
+		if (! isset($_GET['client_id'])) {
+			return new JSONResponse('Bad request, missing client_id', 400);
+		}
+
 		if (isset($_GET['request'])) {
 			$jwtConfig = Configuration::forSymmetricSigner(new Sha256(), InMemory::plainText($this->config->getPrivateKey()));
 			try {
@@ -323,7 +327,9 @@ public function session() {
 	 */
 	public function token() {
 		$request = \Laminas\Diactoros\ServerRequestFactory::fromGlobals($_SERVER, $_GET, $_POST, $_COOKIE, $_FILES);
-		$grantType = $request->getParsedBody()['grant_type'];
+
+		$grantType = $request->getParsedBody()['grant_type'] ?? null;
+
 		switch ($grantType) {
 			case "authorization_code":
 				$code = $request->getParsedBody()['code'];
@@ -342,9 +348,9 @@ public function token() {
 			break;
 		}
 
-		$clientId = $request->getParsedBody()['client_id'];
+		$clientId = $request->getParsedBody()['client_id'] ?? null;
 
-		$httpDpop = $request->getServerParams()['HTTP_DPOP'];
+		$httpDpop = $request->getServerParams()['HTTP_DPOP'] ?? null;
 
 		$response = new \Laminas\Diactoros\Response();
 		$server	= new \Pdsinterop\Solid\Auth\Server($this->authServerFactory, $this->authServerConfig, $response);
@@ -361,7 +367,7 @@ public function token() {
 			);
 		}
 
-		return $this->respond($response); // ->addHeader('Access-Control-Allow-Origin', '*');
+		return $this->respond($response);
 	}
 
 	/**
@@ -389,8 +395,13 @@ public function logout() {
 	 * @NoCSRFRequired
 	 */
 	public function register() {
-		$clientData = file_get_contents('php://input');
-		$clientData = json_decode($clientData, true);
+		$postData = file_get_contents('php://input');
+		$clientData = json_decode($postData, true);
+
+		if (! isset($clientData)) {
+			return new JSONResponse("Missing client data", Http::STATUS_BAD_REQUEST);
+		}
+
 		if (! isset($clientData['redirect_uris'])) {
 			return new JSONResponse("Missing redirect URIs", Http::STATUS_BAD_REQUEST);
 		}

solid/tests/Unit/Controller/ServerControllerTest.php

@@ -119,13 +119,33 @@ public function testAuthorizeWithoutUser()
 		$this->assertEquals($expected, $actual);
 	}
 
+	/**
+	 * @testdox ServerController should return a 400 when asked to authorize with a user but without client_id
+	 *
+	 * @covers ::authorize
+	 */
+	public function testAuthorizeWithoutClientId()
+	{
+		$parameters = $this->createMockConstructorParameters();
+
+		$this->mockUserManager->method('userExists')->willReturn(true);
+
+		$controller = new ServerController(...array_values($parameters));
+
+		$actual = $controller->authorize();
+		$expected = new JSONResponse('Bad request, missing client_id', Http::STATUS_BAD_REQUEST);
+
+		$this->assertEquals($expected, $actual);
+	}
+
 	/**
 	 * @testdox ServerController should return a 400 when asked to authorize with a user but without valid token
 	 *
 	 * @covers ::authorize
 	 */
 	public function testAuthorizeWithoutValidToken()
 	{
+		$_GET['client_id'] = self::MOCK_CLIENT_ID;
 		$_GET['response_type'] = 'mock-response-type';
 
 		$parameters = $this->createMockConstructorParameters();
@@ -277,12 +297,33 @@ public function testAuthorize()
 	 *
 	 * @covers ::register
 	 */
+	public function testRegisterWithoutClientData()
+	{
+		$parameters = $this->createMockConstructorParameters();
+
+		$controller = new ServerController(...array_values($parameters));
+
+		$actual = $controller->register();
+
+		$this->assertEquals(
+			new JSONResponse('Missing client data', Http::STATUS_BAD_REQUEST),
+			$actual
+		);
+	}
+
+	/**
+	 * @testdox ServerController should return a 400 when asked to register without redirect URIs
+	 *
+	 * @covers ::register
+	 */
 	public function testRegisterWithoutRedirectUris()
 	{
 		$parameters = $this->createMockConstructorParameters();
 
 		$controller = new ServerController(...$parameters);
 
+		self::$clientData = json_encode([]);
+
 		$actual = $controller->register();
 
 		$this->assertEquals(
@@ -335,6 +376,26 @@ public function testRegisterWithRedirectUris()
 		$this->assertEquals($expected, $actual);
 	}
 
+	public function testTokenWithoutPostBody()
+	{
+		$parameters = $this->createMockConstructorParameters();
+
+		$controller = new ServerController(...$parameters);
+
+		$tokenResponse = $controller->token();
+
+		$expected = $this->createExpectedResponse(Http::STATUS_BAD_REQUEST, 'Bad Request');
+
+		$actual = [
+			'data' => $tokenResponse->getData(),
+			'headers' => $tokenResponse->getHeaders(),
+			'status' => $tokenResponse->getStatus(),
+		];
+		unset($actual['headers']['X-Request-Id']);
+
+		$this->assertEquals($expected, $actual);
+	}
+
 	/**
 	 * @testdox ServerController should consume Post, Server, and Session variables when generating a token
 	 *
@@ -344,6 +405,7 @@ public function testToken()
 	{
 		$_POST['client_id'] = self::MOCK_CLIENT_ID;
 		$_POST['code'] = '';
+		$_POST['grant_type'] = 'authorization_code';
 		$_SERVER['HTTP_DPOP'] = 'mock dpop';
 		$_SESSION['nonce'] = 'mock nonce';