Return proper status code in case of a CORS exception

When returning a 500 statuscode external applications may interpret this as an error instead of handling this more gracefully. This will now make return a 401 thus.

Fixes https://github.com/owncloud/core/issues/17742

lib/private/appframework/middleware/security/corsmiddleware.php

@@ -2,6 +2,7 @@
 /**
  * @author Bernhard Posselt <dev@bernhard-posselt.com>
  * @author Morris Jobke <hey@morrisjobke.de>
+ * @author Lukas Reschke <lukas@owncloud.com>
  *
  * @copyright Copyright (c) 2015, ownCloud, Inc.
  * @license AGPL-3.0
@@ -23,6 +24,9 @@
 namespace OC\AppFramework\Middleware\Security;
 
 use OC\AppFramework\Utility\ControllerMethodReflector;
+use OCP\AppFramework\Controller;
+use OCP\AppFramework\Http;
+use OCP\AppFramework\Http\JSONResponse;
 use OCP\IRequest;
 use OCP\IUserSession;
 use OCP\AppFramework\Http\Response;
@@ -57,8 +61,8 @@ class CORSMiddleware extends Middleware {
 	 * @param IUserSession $session
 	 */
 	public function __construct(IRequest $request,
-	                            ControllerMethodReflector $reflector,
-	                            IUserSession $session) {
+								ControllerMethodReflector $reflector,
+								IUserSession $session) {
 		$this->request = $request;
 		$this->reflector = $reflector;
 		$this->session = $session;
@@ -71,6 +75,7 @@ class CORSMiddleware extends Middleware {
 	 * @param Controller $controller the controller that is being called
 	 * @param string $methodName the name of the method that will be called on
 	 *                           the controller
+	 * @throws SecurityException
 	 * @since 6.0.0
 	 */
 	public function beforeController($controller, $methodName){
@@ -83,7 +88,7 @@ class CORSMiddleware extends Middleware {
 
 			$this->session->logout();
 			if(!$this->session->login($user, $pass)) {
-				throw new SecurityException('CORS requires basic auth');
+				throw new SecurityException('CORS requires basic auth', Http::STATUS_UNAUTHORIZED);
 			}
 		}
 	}
@@ -97,6 +102,7 @@ class CORSMiddleware extends Middleware {
 	 *                           the controller
 	 * @param Response $response the generated response from the controller
 	 * @return Response a Response object
+	 * @throws SecurityException
 	 */
 	public function afterController($controller, $methodName, Response $response){
 		// only react if its a CORS request and if the request sends origin and
@@ -106,7 +112,7 @@ class CORSMiddleware extends Middleware {
 
 			// allow credentials headers must not be true or CSRF is possible
 			// otherwise
-			foreach($response->getHeaders() as $header => $value ) {
+			foreach($response->getHeaders() as $header => $value) {
 				if(strtolower($header) === 'access-control-allow-credentials' &&
 				   strtolower(trim($value)) === 'true') {
 					$msg = 'Access-Control-Allow-Credentials must not be '.
@@ -121,5 +127,28 @@ class CORSMiddleware extends Middleware {
 		return $response;
 	}
 
+	/**
+	 * If an SecurityException is being caught return a JSON error response
+	 *
+	 * @param Controller $controller the controller that is being called
+	 * @param string $methodName the name of the method that will be called on
+	 *                           the controller
+	 * @param \Exception $exception the thrown exception
+	 * @throws \Exception the passed in exception if it cant handle it
+	 * @return Response a Response object or null in case that the exception could not be handled
+	 */
+	public function afterException($controller, $methodName, \Exception $exception){
+		if($exception instanceof SecurityException){
+			$response =  new JSONResponse(['message' => $exception->getMessage()]);
+			if($exception->getCode() !== 0) {
+				$response->setStatus($exception->getCode());
+			} else {
+				$response->setStatus(Http::STATUS_INTERNAL_SERVER_ERROR);
+			}
+			return $response;
+		}
+
+		throw $exception;
+	}
 
 }

lib/private/appframework/middleware/security/securitymiddleware.php

@@ -69,13 +69,13 @@ class SecurityMiddleware extends Middleware {
 	 * @param bool $isAdminUser
 	 */
 	public function __construct(IRequest $request,
-	                            ControllerMethodReflector $reflector,
-	                            INavigationManager $navigationManager,
-	                            IURLGenerator $urlGenerator,
-	                            ILogger $logger,
-	                            $appName,
-	                            $isLoggedIn,
-	                            $isAdminUser){
+								ControllerMethodReflector $reflector,
+								INavigationManager $navigationManager,
+								IURLGenerator $urlGenerator,
+								ILogger $logger,
+								$appName,
+								$isLoggedIn,
+								$isAdminUser){
 		$this->navigationManager = $navigationManager;
 		$this->request = $request;
 		$this->reflector = $reflector;

tests/lib/appframework/middleware/security/CORSMiddlewareTest.php

@@ -15,6 +15,8 @@ namespace OC\AppFramework\Middleware\Security;
 use OC\AppFramework\Http\Request;
 use OC\AppFramework\Utility\ControllerMethodReflector;
 
+use OCP\AppFramework\Http;
+use OCP\AppFramework\Http\JSONResponse;
 use OCP\AppFramework\Http\Response;
 
 
@@ -181,4 +183,53 @@ class CORSMiddlewareTest extends \Test\TestCase {
 		$middleware->beforeController($this, __FUNCTION__, new Response());
 	}
 
+	public function testAfterExceptionWithSecurityExceptionNoStatus() {
+		$request = new Request(
+			['server' => [
+				'PHP_AUTH_USER' => 'user',
+				'PHP_AUTH_PW' => 'pass'
+			]],
+			$this->getMock('\OCP\Security\ISecureRandom'),
+			$this->getMock('\OCP\IConfig')
+		);
+		$middleware = new CORSMiddleware($request, $this->reflector, $this->session);
+		$response = $middleware->afterException($this, __FUNCTION__, new SecurityException('A security exception'));
+
+		$expected = new JSONResponse(['message' => 'A security exception'], 500);
+		$this->assertEquals($expected, $response);
+	}
+
+	public function testAfterExceptionWithSecurityExceptionWithStatus() {
+		$request = new Request(
+			['server' => [
+				'PHP_AUTH_USER' => 'user',
+				'PHP_AUTH_PW' => 'pass'
+			]],
+			$this->getMock('\OCP\Security\ISecureRandom'),
+			$this->getMock('\OCP\IConfig')
+		);
+		$middleware = new CORSMiddleware($request, $this->reflector, $this->session);
+		$response = $middleware->afterException($this, __FUNCTION__, new SecurityException('A security exception', 501));
+
+		$expected = new JSONResponse(['message' => 'A security exception'], 501);
+		$this->assertEquals($expected, $response);
+	}
+
+	/**
+	 * @expectedException \Exception
+	 * @expectedExceptionMessage A regular exception
+	 */
+	public function testAfterExceptionWithRegularException() {
+		$request = new Request(
+			['server' => [
+				'PHP_AUTH_USER' => 'user',
+				'PHP_AUTH_PW' => 'pass'
+			]],
+			$this->getMock('\OCP\Security\ISecureRandom'),
+			$this->getMock('\OCP\IConfig')
+		);
+		$middleware = new CORSMiddleware($request, $this->reflector, $this->session);
+		$middleware->afterException($this, __FUNCTION__, new \Exception('A regular exception'));
+	}
+
 }