From c8e3599cad9c5174260fc1dbe340efac65f1d646 Mon Sep 17 00:00:00 2001 From: Bernhard Posselt Date: Fri, 22 May 2015 13:17:27 +0200 Subject: [PATCH] disallow cookie auth for cors requests testing ... fixes fix test add php doc fix small mistake add another phpdoc remove not working cors annotations from files app --- apps/files/controller/apicontroller.php | 2 - .../dependencyinjection/dicontainer.php | 5 +- .../middleware/security/corsmiddleware.php | 49 ++++++++++-- .../security/CORSMiddlewareTest.php | 75 ++++++++++++++++++- 4 files changed, 118 insertions(+), 13 deletions(-) diff --git a/apps/files/controller/apicontroller.php b/apps/files/controller/apicontroller.php index 8fc22a8aa6..0cc222d7ce 100644 --- a/apps/files/controller/apicontroller.php +++ b/apps/files/controller/apicontroller.php @@ -92,7 +92,6 @@ class ApiController extends Controller { * replace the actual tag selection. * * @NoAdminRequired - * @CORS * * @param string $path path * @param array|string $tags array of tags @@ -126,7 +125,6 @@ class ApiController extends Controller { * Returns a list of all files tagged with the given tag. * * @NoAdminRequired - * @CORS * * @param array|string $tagName tag name to filter by * @return DataResponse diff --git a/lib/private/appframework/dependencyinjection/dicontainer.php b/lib/private/appframework/dependencyinjection/dicontainer.php index 8501fc69ad..a11e4ee05b 100644 --- a/lib/private/appframework/dependencyinjection/dicontainer.php +++ b/lib/private/appframework/dependencyinjection/dicontainer.php @@ -291,7 +291,8 @@ class DIContainer extends SimpleContainer implements IAppContainer { $this->registerService('CORSMiddleware', function($c) { return new CORSMiddleware( $c['Request'], - $c['ControllerMethodReflector'] + $c['ControllerMethodReflector'], + $c['OCP\IUserSession'] ); }); @@ -306,8 +307,8 @@ class DIContainer extends SimpleContainer implements IAppContainer { $middleWares = &$this->middleWares; $this->registerService('MiddlewareDispatcher', function($c) use (&$middleWares) { $dispatcher = new MiddlewareDispatcher(); - $dispatcher->registerMiddleware($c['SecurityMiddleware']); $dispatcher->registerMiddleware($c['CORSMiddleware']); + $dispatcher->registerMiddleware($c['SecurityMiddleware']); foreach($middleWares as $middleWare) { $dispatcher->registerMiddleware($c[$middleWare]); diff --git a/lib/private/appframework/middleware/security/corsmiddleware.php b/lib/private/appframework/middleware/security/corsmiddleware.php index 983742858d..600eb2318c 100644 --- a/lib/private/appframework/middleware/security/corsmiddleware.php +++ b/lib/private/appframework/middleware/security/corsmiddleware.php @@ -24,30 +24,69 @@ namespace OC\AppFramework\Middleware\Security; use OC\AppFramework\Utility\ControllerMethodReflector; use OCP\IRequest; +use OCP\IUserSession; use OCP\AppFramework\Http\Response; use OCP\AppFramework\Middleware; /** - * This middleware sets the correct CORS headers on a response if the + * This middleware sets the correct CORS headers on a response if the * controller has the @CORS annotation. This is needed for webapps that want - * to access an API and dont run on the same domain, see + * to access an API and dont run on the same domain, see * https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS */ class CORSMiddleware extends Middleware { + /** + * @var IRequest + */ private $request; + + /** + * @var ControllerMethodReflector + */ private $reflector; + /** + * @var IUserSession + */ + private $session; + /** * @param IRequest $request * @param ControllerMethodReflector $reflector + * @param IUserSession $session */ - public function __construct(IRequest $request, - ControllerMethodReflector $reflector) { + public function __construct(IRequest $request, + ControllerMethodReflector $reflector, + IUserSession $session) { $this->request = $request; $this->reflector = $reflector; + $this->session = $session; } + /** + * 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 + * @since 6.0.0 + */ + public function beforeController($controller, $methodName){ + // ensure that @CORS annotated API routes are not used in conjunction + // with session authentication since this enables CSRF attack vectors + if ($this->reflector->hasAnnotation('CORS') && + !$this->reflector->hasAnnotation('PublicPage')) { + $user = $this->request->server['PHP_AUTH_USER']; + $pass = $this->request->server['PHP_AUTH_PW']; + + $this->session->logout(); + if(!$this->session->login($user, $pass)) { + throw new SecurityException('CORS requires basic auth'); + } + } + } /** * This is being run after a successful controllermethod call and allows @@ -65,7 +104,7 @@ class CORSMiddleware extends Middleware { if(isset($this->request->server['HTTP_ORIGIN']) && $this->reflector->hasAnnotation('CORS')) { - // allow credentials headers must not be true or CSRF is possible + // allow credentials headers must not be true or CSRF is possible // otherwise foreach($response->getHeaders() as $header => $value ) { if(strtolower($header) === 'access-control-allow-credentials' && diff --git a/tests/lib/appframework/middleware/security/CORSMiddlewareTest.php b/tests/lib/appframework/middleware/security/CORSMiddlewareTest.php index a4f3137cb1..92ea5450ab 100644 --- a/tests/lib/appframework/middleware/security/CORSMiddlewareTest.php +++ b/tests/lib/appframework/middleware/security/CORSMiddlewareTest.php @@ -21,10 +21,12 @@ use OCP\AppFramework\Http\Response; class CORSMiddlewareTest extends \Test\TestCase { private $reflector; + private $session; protected function setUp() { parent::setUp(); $this->reflector = new ControllerMethodReflector(); + $this->session = $this->getMock('\OCP\IUserSession'); } /** @@ -41,7 +43,7 @@ class CORSMiddlewareTest extends \Test\TestCase { $this->getMock('\OCP\IConfig') ); $this->reflector->reflect($this, __FUNCTION__); - $middleware = new CORSMiddleware($request, $this->reflector); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session); $response = $middleware->afterController($this, __FUNCTION__, new Response()); $headers = $response->getHeaders(); @@ -59,7 +61,7 @@ class CORSMiddlewareTest extends \Test\TestCase { $this->getMock('\OCP\Security\ISecureRandom'), $this->getMock('\OCP\IConfig') ); - $middleware = new CORSMiddleware($request, $this->reflector); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session); $response = $middleware->afterController($this, __FUNCTION__, new Response()); $headers = $response->getHeaders(); @@ -77,7 +79,7 @@ class CORSMiddlewareTest extends \Test\TestCase { $this->getMock('\OCP\IConfig') ); $this->reflector->reflect($this, __FUNCTION__); - $middleware = new CORSMiddleware($request, $this->reflector); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session); $response = $middleware->afterController($this, __FUNCTION__, new Response()); $headers = $response->getHeaders(); @@ -100,11 +102,76 @@ class CORSMiddlewareTest extends \Test\TestCase { $this->getMock('\OCP\IConfig') ); $this->reflector->reflect($this, __FUNCTION__); - $middleware = new CORSMiddleware($request, $this->reflector); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session); $response = new Response(); $response->addHeader('AcCess-control-Allow-Credentials ', 'TRUE'); $middleware->afterController($this, __FUNCTION__, $response); } + /** + * @CORS + * @PublicPage + */ + public function testNoCORSShouldAllowCookieAuth() { + $request = new Request( + [], + $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\IConfig') + ); + $this->reflector->reflect($this, __FUNCTION__); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session); + + $middleware->beforeController($this, __FUNCTION__, new Response()); + } + + /** + * @CORS + */ + public function testCORSShouldRelogin() { + $request = new Request( + ['server' => [ + 'PHP_AUTH_USER' => 'user', + 'PHP_AUTH_PW' => 'pass' + ]], + $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\IConfig') + ); + $this->session->expects($this->once()) + ->method('logout'); + $this->session->expects($this->once()) + ->method('login') + ->with($this->equalTo('user'), $this->equalTo('pass')) + ->will($this->returnValue(true)); + $this->reflector->reflect($this, __FUNCTION__); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session); + + $middleware->beforeController($this, __FUNCTION__, new Response()); + } + + /** + * @CORS + * @expectedException \OC\AppFramework\Middleware\Security\SecurityException + */ + public function testCORSShouldNotAllowCookieAuth() { + $request = new Request( + ['server' => [ + 'PHP_AUTH_USER' => 'user', + 'PHP_AUTH_PW' => 'pass' + ]], + $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\IConfig') + ); + $this->session->expects($this->once()) + ->method('logout'); + $this->session->expects($this->once()) + ->method('login') + ->with($this->equalTo('user'), $this->equalTo('pass')) + ->will($this->returnValue(false)); + $this->reflector->reflect($this, __FUNCTION__); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session); + + $middleware->beforeController($this, __FUNCTION__, new Response()); + } + }