From 0cb950e4db100f60de52b17055d79263967f7a8f Mon Sep 17 00:00:00 2001 From: noveens Date: Wed, 30 Aug 2017 23:41:07 +0530 Subject: [PATCH] Removed beforeController Logic I removed the beforeController logic here due to the change of handling CORS since PR 28457[1] According to previous implementation, CORS was only allowed with methods that had @PublicPage notation for preventing CSRF attacks. But in the latest PR by me, the current implementations is as follows: * maintain a white-list of domains for whom CORS is enabled * This list can be viewed and edited under settings -> personal -> security This implementation removes the need for `@PublicPage`[2]. [1] https://github.com/owncloud/core/pull/28457 [2] https://github.com/owncloud/core/pull/28864/ --- .../Middleware/Security/CORSMiddleware.php | 80 +++------ lib/private/Route/Router.php | 6 +- .../Security/CORSMiddlewareTest.php | 156 ------------------ 3 files changed, 26 insertions(+), 216 deletions(-) diff --git a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php index c3f1a105400..24341688253 100644 --- a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php @@ -28,11 +28,9 @@ namespace OC\AppFramework\Middleware\Security; use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; use OC\AppFramework\Utility\ControllerMethodReflector; -use OC\Authentication\Exceptions\PasswordLoginForbiddenException; use OCP\AppFramework\Controller; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\CORS; -use OCP\AppFramework\Http\Attribute\PublicPage; use OCP\AppFramework\Http\JSONResponse; use OCP\AppFramework\Http\Response; use OCP\AppFramework\Middleware; @@ -58,63 +56,7 @@ class CORSMiddleware extends Middleware { } /** - * This is being run in normal order before the controller is being - * called which allows several modifications and checks - * - * @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) { - $reflectionMethod = new ReflectionMethod($controller, $methodName); - - // ensure that @CORS annotated API routes are not used in conjunction - // with session authentication since this enables CSRF attack vectors - if ($this->hasAnnotationOrAttribute($reflectionMethod, 'CORS', CORS::class) && - (!$this->hasAnnotationOrAttribute($reflectionMethod, 'PublicPage', PublicPage::class) || $this->session->isLoggedIn())) { - $user = array_key_exists('PHP_AUTH_USER', $this->request->server) ? $this->request->server['PHP_AUTH_USER'] : null; - $pass = array_key_exists('PHP_AUTH_PW', $this->request->server) ? $this->request->server['PHP_AUTH_PW'] : null; - - // Allow to use the current session if a CSRF token is provided - if ($this->request->passesCSRFCheck()) { - return; - } - $this->session->logout(); - try { - if ($user === null || $pass === null || !$this->session->logClientIn($user, $pass, $this->request, $this->throttler)) { - throw new SecurityException('CORS requires basic auth', Http::STATUS_UNAUTHORIZED); - } - } catch (PasswordLoginForbiddenException $ex) { - throw new SecurityException('Password login forbidden, use token instead', Http::STATUS_UNAUTHORIZED); - } - } - } - - /** - * @template T - * - * @param ReflectionMethod $reflectionMethod - * @param string $annotationName - * @param class-string $attributeClass - * @return boolean - */ - protected function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, string $annotationName, string $attributeClass): bool { - if ($this->reflector->hasAnnotation($annotationName)) { - return true; - } - - - if (!empty($reflectionMethod->getAttributes($attributeClass))) { - return true; - } - - return false; - } - - /** - * This is being run after a successful controller method call and allows + * This is being run after a successful controllermethod call and allows * the manipulation of a Response object. The middleware is run in reverse order * * @param Controller $controller the controller that is being called @@ -172,4 +114,24 @@ class CORSMiddleware extends Middleware { throw $exception; } + + /** + * @template T + * + * @param ReflectionMethod $reflectionMethod + * @param string $annotationName + * @param class-string $attributeClass + * @return boolean + */ + protected function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, string $annotationName, string $attributeClass): bool { + if ($this->reflector->hasAnnotation($annotationName)) { + return true; + } + + if (!empty($reflectionMethod->getAttributes($attributeClass))) { + return true; + } + + return false; + } } diff --git a/lib/private/Route/Router.php b/lib/private/Route/Router.php index ac08a56dfff..972fce81a8e 100644 --- a/lib/private/Route/Router.php +++ b/lib/private/Route/Router.php @@ -291,7 +291,7 @@ class Router implements IRouter { \OC_API::respond($response, \OC_API::requestedFormat()); // Return since no more processing for an OPTIONS request is required - return; + return []; } catch (ResourceNotFoundException $e) { if (substr($url, -1) !== '/') { // We allow links to apps/files? for backwards compatibility reasons @@ -340,6 +340,10 @@ class Router implements IRouter { */ public function match($url) { $parameters = $this->findMatchingRoute($url); + if (\OC::$server->getRequest()->getMethod() === "OPTIONS" && count($parameters) === 0) { + // nothing to do here as this is a CORS preflight + return; + } $this->eventLogger->start('route:run', 'Run route'); if (isset($parameters['caller'])) { diff --git a/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php index 30d4eeda79e..0fe429258ef 100644 --- a/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php @@ -179,162 +179,6 @@ class CORSMiddlewareTest extends \Test\TestCase { ]; } - /** - * @dataProvider dataNoCORSOnAnonymousPublicPage - */ - public function testNoCORSOnAnonymousPublicPage(string $method): void { - $request = new Request( - [], - $this->createMock(IRequestId::class), - $this->createMock(IConfig::class) - ); - $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->config); - $this->session->expects($this->once()) - ->method('isLoggedIn') - ->willReturn(false); - $this->session->expects($this->never()) - ->method('logout'); - $this->session->expects($this->never()) - ->method('logClientIn') - ->with($this->equalTo('user'), $this->equalTo('pass')) - ->willReturn(true); - $this->reflector->reflect($this->controller, $method); - - $middleware->beforeController($this->controller, $method); - } - - public function dataCORSShouldNeverAllowCookieAuth(): array { - return [ - ['testCORSShouldNeverAllowCookieAuth'], - ['testCORSShouldNeverAllowCookieAuthAttribute'], - ['testCORSAttributeShouldNeverAllowCookieAuth'], - ['testCORSAttributeShouldNeverAllowCookieAuthAttribute'], - ]; - } - - /** - * @dataProvider dataCORSShouldNeverAllowCookieAuth - */ - public function testCORSShouldNeverAllowCookieAuth(string $method): void { - $request = new Request( - [], - $this->createMock(IRequestId::class), - $this->createMock(IConfig::class) - ); - $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->config); - $this->session->expects($this->once()) - ->method('isLoggedIn') - ->willReturn(true); - $this->session->expects($this->once()) - ->method('logout'); - $this->session->expects($this->never()) - ->method('logClientIn') - ->with($this->equalTo('user'), $this->equalTo('pass')) - ->willReturn(true); - - $this->expectException(SecurityException::class); - $middleware->beforeController($this->controller, $method); - } - - public function dataCORSShouldRelogin(): array { - return [ - ['testCORSShouldRelogin'], - ['testCORSAttributeShouldRelogin'], - ]; - } - - /** - * @dataProvider dataCORSShouldRelogin - */ - public function testCORSShouldRelogin(string $method): void { - $request = new Request( - ['server' => [ - 'PHP_AUTH_USER' => 'user', - 'PHP_AUTH_PW' => 'pass' - ]], - $this->createMock(IRequestId::class), - $this->createMock(IConfig::class) - ); - $this->session->expects($this->once()) - ->method('logout'); - $this->session->expects($this->once()) - ->method('logClientIn') - ->with($this->equalTo('user'), $this->equalTo('pass')) - ->willReturn(true); - $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->config); - - $middleware->beforeController($this->controller, $method); - } - - public function dataCORSShouldFailIfPasswordLoginIsForbidden(): array { - return [ - ['testCORSShouldFailIfPasswordLoginIsForbidden'], - ['testCORSAttributeShouldFailIfPasswordLoginIsForbidden'], - ]; - } - - /** - * @dataProvider dataCORSShouldFailIfPasswordLoginIsForbidden - */ - public function testCORSShouldFailIfPasswordLoginIsForbidden(string $method): void { - $this->expectException(\OC\AppFramework\Middleware\Security\Exceptions\SecurityException::class); - - $request = new Request( - ['server' => [ - 'PHP_AUTH_USER' => 'user', - 'PHP_AUTH_PW' => 'pass' - ]], - $this->createMock(IRequestId::class), - $this->createMock(IConfig::class) - ); - $this->session->expects($this->once()) - ->method('logout'); - $this->session->expects($this->once()) - ->method('logClientIn') - ->with($this->equalTo('user'), $this->equalTo('pass')) - ->will($this->throwException(new \OC\Authentication\Exceptions\PasswordLoginForbiddenException)); - $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->config); - - $middleware->beforeController($this->controller, $method); - } - - public function dataCORSShouldNotAllowCookieAuth(): array { - return [ - ['testCORSShouldNotAllowCookieAuth'], - ['testCORSAttributeShouldNotAllowCookieAuth'], - ]; - } - - /** - * @dataProvider dataCORSShouldNotAllowCookieAuth - */ - public function testCORSShouldNotAllowCookieAuth(string $method): void { - $this->expectException(\OC\AppFramework\Middleware\Security\Exceptions\SecurityException::class); - - $request = new Request( - ['server' => [ - 'PHP_AUTH_USER' => 'user', - 'PHP_AUTH_PW' => 'pass' - ]], - $this->createMock(IRequestId::class), - $this->createMock(IConfig::class) - ); - $this->session->expects($this->once()) - ->method('logout'); - $this->session->expects($this->once()) - ->method('logClientIn') - ->with($this->equalTo('user'), $this->equalTo('pass')) - ->willReturn(false); - $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->config); - - $middleware->beforeController($this->controller, $method); - } - public function testAfterExceptionWithSecurityExceptionNoStatus() { $request = new Request( ['server' => [