From 9a4d204b55da063631f01a780d32b3fd88c729cd Mon Sep 17 00:00:00 2001 From: Bernhard Posselt Date: Thu, 8 May 2014 11:47:18 +0200 Subject: [PATCH] add cors middleware remove methodannotationreader namespace fix namespace for server container fix tests fail if with cors credentials header is set to true, implement a reusable preflighted cors method in the controller baseclass, make corsmiddleware private and register it for every request remove uneeded local in cors middleware registratio dont uppercase cors to easily use it from routes fix indention comment fixes explicitely set allow credentials header to false dont depend on better controllers PR, fix that stuff later split cors methods to be in a seperate controller for exposing apis remove protected definitions from apicontroller since controller has it --- .../dependencyinjection/dicontainer.php | 6 ++ .../middleware/security/corsmiddleware.php | 73 +++++++++++++++ lib/public/appframework/apicontroller.php | 93 +++++++++++++++++++ lib/public/appframework/controller.php | 13 ++- lib/public/appframework/http/response.php | 4 + .../controller/ApiControllerTest.php | 55 +++++++++++ .../controller/ControllerTest.php | 4 +- tests/lib/appframework/http/ResponseTest.php | 2 +- .../security/CORSMiddlewareTest.php | 77 +++++++++++++++ 9 files changed, 322 insertions(+), 5 deletions(-) create mode 100644 lib/private/appframework/middleware/security/corsmiddleware.php create mode 100644 lib/public/appframework/apicontroller.php create mode 100644 tests/lib/appframework/controller/ApiControllerTest.php create mode 100644 tests/lib/appframework/middleware/security/CORSMiddlewareTest.php diff --git a/lib/private/appframework/dependencyinjection/dicontainer.php b/lib/private/appframework/dependencyinjection/dicontainer.php index e478225a53..becd755bda 100644 --- a/lib/private/appframework/dependencyinjection/dicontainer.php +++ b/lib/private/appframework/dependencyinjection/dicontainer.php @@ -30,6 +30,7 @@ use OC\AppFramework\Http\Dispatcher; use OC\AppFramework\Core\API; use OC\AppFramework\Middleware\MiddlewareDispatcher; use OC\AppFramework\Middleware\Security\SecurityMiddleware; +use OC\AppFramework\Middleware\Security\CORSMiddleware; use OC\AppFramework\Utility\SimpleContainer; use OC\AppFramework\Utility\TimeFactory; use OCP\AppFramework\IApi; @@ -92,10 +93,15 @@ class DIContainer extends SimpleContainer implements IAppContainer{ return new SecurityMiddleware($app, $c['Request']); }); + $this['CORSMiddleware'] = $this->share(function($c) { + return new CORSMiddleware($c['Request']); + }); + $middleWares = &$this->middleWares; $this['MiddlewareDispatcher'] = $this->share(function($c) use (&$middleWares) { $dispatcher = new MiddlewareDispatcher(); $dispatcher->registerMiddleware($c['SecurityMiddleware']); + $dispatcher->registerMiddleware($c['CORSMiddleware']); 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 new file mode 100644 index 0000000000..1049b3ed47 --- /dev/null +++ b/lib/private/appframework/middleware/security/corsmiddleware.php @@ -0,0 +1,73 @@ + + * @copyright Bernhard Posselt 2014 + */ + +namespace OC\AppFramework\Middleware\Security; + +use OC\AppFramework\Utility\MethodAnnotationReader; +use OCP\IRequest; +use OCP\AppFramework\Http\Response; +use OCP\AppFramework\Middleware; + +/** + * 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 + * https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS + */ +class CORSMiddleware extends Middleware { + + private $request; + + /** + * @param string $request the name of the method that will be called on + * the controller + */ + public function __construct(IRequest $request) { + $this->request = $request; + } + + + /** + * 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 + * @param string $methodName the name of the method that will be called on + * the controller + * @param Response $response the generated response from the controller + * @return Response a Response object + */ + public function afterController($controller, $methodName, Response $response){ + // only react if its a CORS request and if the request sends origin and + $reflector = new MethodAnnotationReader($controller, $methodName); + + if(isset($this->request->server['HTTP_ORIGIN']) && + $reflector->hasAnnotation('CORS')) { + + // 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' && + strtolower(trim($value)) === 'true') { + $msg = 'Access-Control-Allow-Credentials must not be '. + 'set to true in order to prevent CSRF'; + throw new SecurityException($msg); + } + } + + $origin = $this->request->server['HTTP_ORIGIN']; + $response->addHeader('Access-Control-Allow-Origin', $origin); + } + return $response; + } + + +} diff --git a/lib/public/appframework/apicontroller.php b/lib/public/appframework/apicontroller.php new file mode 100644 index 0000000000..5272f3ed52 --- /dev/null +++ b/lib/public/appframework/apicontroller.php @@ -0,0 +1,93 @@ +. + * + */ + +/** + * Public interface of ownCloud for apps to use. + * AppFramework\Controller class + */ + +namespace OCP\AppFramework; + +use OCP\AppFramework\Http\Response; +use OCP\IRequest; + + +/** + * Base class to inherit your controllers from that are used for RESTful APIs + */ +abstract class ApiController extends Controller { + + private $corsMethods; + private $corsAllowedHeaders; + private $corsMaxAge; + + /** + * constructor of the controller + * @param string $appName the name of the app + * @param IRequest $request an instance of the request + * @param string $corsMethods: comma seperated string of HTTP verbs which + * should be allowed for websites or webapps when calling your API, defaults to + * 'PUT, POST, GET, DELETE, PATCH' + * @param string $corsAllowedHeaders: comma seperated string of HTTP headers + * which should be allowed for websites or webapps when calling your API, + * defaults to 'Authorization, Content-Type, Accept' + * @param int $corsMaxAge number in seconds how long a preflighted OPTIONS + * request should be cached, defaults to 1728000 seconds + */ + public function __construct($appName, + IRequest $request, + $corsMethods='PUT, POST, GET, DELETE, PATCH', + $corsAllowedHeaders='Authorization, Content-Type, Accept', + $corsMaxAge=1728000){ + parent::__construct($appName, $request); + $this->corsMethods = $corsMethods; + $this->corsAllowedHeaders = $corsAllowedHeaders; + $this->corsMaxAge = $corsMaxAge; + } + + + /** + * This method implements a preflighted cors response for you that you can + * link to for the options request + * + * @NoAdminRequired + * @NoCSRFRequired + * @PublicPage + */ + public function preflightedCors() { + if(isset($this->request->server['HTTP_ORIGIN'])) { + $origin = $this->request->server['HTTP_ORIGIN']; + } else { + $origin = '*'; + } + + $response = new Response(); + $response->addHeader('Access-Control-Allow-Origin', $origin); + $response->addHeader('Access-Control-Allow-Methods', $this->corsMethods); + $response->addHeader('Access-Control-Max-Age', $this->corsMaxAge); + $response->addHeader('Access-Control-Allow-Headers', $this->corsAllowedHeaders); + $response->addHeader('Access-Control-Allow-Credentials', 'false'); + return $response; + } + + +} diff --git a/lib/public/appframework/controller.php b/lib/public/appframework/controller.php index 758f0a8008..f42eba172c 100644 --- a/lib/public/appframework/controller.php +++ b/lib/public/appframework/controller.php @@ -28,7 +28,6 @@ namespace OCP\AppFramework; use OCP\AppFramework\Http\TemplateResponse; -use OCP\AppFramework\IAppContainer; use OCP\IRequest; @@ -49,12 +48,22 @@ abstract class Controller { */ protected $request; + /** * constructor of the controller * @param string $appName the name of the app * @param IRequest $request an instance of the request + * @param string $corsMethods: comma seperated string of HTTP verbs which + * should be allowed for websites or webapps when calling your API, defaults to + * 'PUT, POST, GET, DELETE, PATCH' + * @param string $corsAllowedHeaders: comma seperated string of HTTP headers + * which should be allowed for websites or webapps when calling your API, + * defaults to 'Authorization, Content-Type, Accept' + * @param int $corsMaxAge number in seconds how long a preflighted OPTIONS + * request should be cached, defaults to 1728000 seconds */ - public function __construct($appName, IRequest $request){ + public function __construct($appName, + IRequest $request){ $this->appName = $appName; $this->request = $request; } diff --git a/lib/public/appframework/http/response.php b/lib/public/appframework/http/response.php index 45402d9b3b..559d14dd7e 100644 --- a/lib/public/appframework/http/response.php +++ b/lib/public/appframework/http/response.php @@ -92,6 +92,10 @@ class Response { * @return Response Reference to this object */ public function addHeader($name, $value) { + $name = trim($name); // always remove leading and trailing whitespace + // to be able to reliably check for security + // headers + if(is_null($value)) { unset($this->headers[$name]); } else { diff --git a/tests/lib/appframework/controller/ApiControllerTest.php b/tests/lib/appframework/controller/ApiControllerTest.php new file mode 100644 index 0000000000..b772f540ce --- /dev/null +++ b/tests/lib/appframework/controller/ApiControllerTest.php @@ -0,0 +1,55 @@ +. + * + */ + + +namespace OCP\AppFramework; + +use OC\AppFramework\Http\Request; +use OCP\AppFramework\Http\TemplateResponse; + + +class ChildApiController extends ApiController {}; + + +class ApiControllerTest extends \PHPUnit_Framework_TestCase { + + + public function testCors() { + $request = new Request( + array('server' => array('HTTP_ORIGIN' => 'test')) + ); + $this->controller = new ChildApiController('app', $request, 'verbs', + 'headers', 100); + + $response = $this->controller->preflightedCors(); + + $headers = $response->getHeaders(); + + $this->assertEquals('test', $headers['Access-Control-Allow-Origin']); + $this->assertEquals('verbs', $headers['Access-Control-Allow-Methods']); + $this->assertEquals('headers', $headers['Access-Control-Allow-Headers']); + $this->assertEquals('false', $headers['Access-Control-Allow-Credentials']); + $this->assertEquals(100, $headers['Access-Control-Max-Age']); + } + +} diff --git a/tests/lib/appframework/controller/ControllerTest.php b/tests/lib/appframework/controller/ControllerTest.php index f17d5f24aa..b6c83125da 100644 --- a/tests/lib/appframework/controller/ControllerTest.php +++ b/tests/lib/appframework/controller/ControllerTest.php @@ -22,10 +22,9 @@ */ -namespace Test\AppFramework\Controller; +namespace OCP\AppFramework; use OC\AppFramework\Http\Request; -use OCP\AppFramework\Controller; use OCP\AppFramework\Http\TemplateResponse; @@ -129,4 +128,5 @@ class ControllerTest extends \PHPUnit_Framework_TestCase { $this->assertEquals('daheim', $this->controller->env('PATH')); } + } diff --git a/tests/lib/appframework/http/ResponseTest.php b/tests/lib/appframework/http/ResponseTest.php index 27350725d7..4b8d3ae50e 100644 --- a/tests/lib/appframework/http/ResponseTest.php +++ b/tests/lib/appframework/http/ResponseTest.php @@ -42,7 +42,7 @@ class ResponseTest extends \PHPUnit_Framework_TestCase { public function testAddHeader(){ - $this->childResponse->addHeader('hello', 'world'); + $this->childResponse->addHeader(' hello ', 'world'); $headers = $this->childResponse->getHeaders(); $this->assertEquals('world', $headers['hello']); } diff --git a/tests/lib/appframework/middleware/security/CORSMiddlewareTest.php b/tests/lib/appframework/middleware/security/CORSMiddlewareTest.php new file mode 100644 index 0000000000..8224e9b4aa --- /dev/null +++ b/tests/lib/appframework/middleware/security/CORSMiddlewareTest.php @@ -0,0 +1,77 @@ + + * @copyright Bernhard Posselt 2014 + */ + + +namespace OC\AppFramework\Middleware\Security; + +use OC\AppFramework\Http\Request; +use OCP\AppFramework\Http\Response; + + +class CORSMiddlewareTest extends \PHPUnit_Framework_TestCase { + + /** + * @CORS + */ + public function testSetCORSAPIHeader() { + $request = new Request( + array('server' => array('HTTP_ORIGIN' => 'test')) + ); + + $middleware = new CORSMiddleware($request); + $response = $middleware->afterController($this, __FUNCTION__, new Response()); + $headers = $response->getHeaders(); + + $this->assertEquals('test', $headers['Access-Control-Allow-Origin']); + } + + + public function testNoAnnotationNoCORSHEADER() { + $request = new Request( + array('server' => array('HTTP_ORIGIN' => 'test')) + ); + $middleware = new CORSMiddleware($request); + + $response = $middleware->afterController($this, __FUNCTION__, new Response()); + $headers = $response->getHeaders(); + $this->assertFalse(array_key_exists('Access-Control-Allow-Origin', $headers)); + } + + + /** + * @CORS + */ + public function testNoOriginHeaderNoCORSHEADER() { + $request = new Request(); + + $middleware = new CORSMiddleware($request); + $response = $middleware->afterController($this, __FUNCTION__, new Response()); + $headers = $response->getHeaders(); + $this->assertFalse(array_key_exists('Access-Control-Allow-Origin', $headers)); + } + + + /** + * @CORS + * @expectedException \OC\AppFramework\Middleware\Security\SecurityException + */ + public function testCorsIgnoredIfWithCredentialsHeaderPresent() { + $request = new Request( + array('server' => array('HTTP_ORIGIN' => 'test')) + ); + $middleware = new CORSMiddleware($request); + + $response = new Response(); + $response->addHeader('AcCess-control-Allow-Credentials ', 'TRUE'); + $response = $middleware->afterController($this, __FUNCTION__, $response); + } + +}