From e0f11228c597703e83783187520a82c663674b55 Mon Sep 17 00:00:00 2001 From: Aleix Quintana Alsius Date: Sun, 23 Apr 2023 23:56:44 +0200 Subject: [PATCH 1/6] Draft proposal of allowed origins cors list --- core/Controller/PreviewController.php | 5 +- core/routes.php | 1 + .../DependencyInjection/DIContainer.php | 4 +- .../Middleware/Security/CORSMiddleware.php | 51 +++++++++++++++++-- 4 files changed, 53 insertions(+), 8 deletions(-) diff --git a/core/Controller/PreviewController.php b/core/Controller/PreviewController.php index 118f1b47752d6..ab9725195670b 100644 --- a/core/Controller/PreviewController.php +++ b/core/Controller/PreviewController.php @@ -28,7 +28,7 @@ namespace OC\Core\Controller; use OCA\Files_Sharing\SharedStorage; -use OCP\AppFramework\Controller; +use OCP\AppFramework\ApiController; use OCP\AppFramework\Http; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\Http\FileDisplayResponse; @@ -39,7 +39,7 @@ use OCP\IPreview; use OCP\IRequest; -class PreviewController extends Controller { +class PreviewController extends ApiController { private ?string $userId; private IRootFolder $root; private IPreview $preview; @@ -87,6 +87,7 @@ public function getPreview( /** * @NoAdminRequired * @NoCSRFRequired + * @CORS * * @return DataResponse|FileDisplayResponse */ diff --git a/core/routes.php b/core/routes.php index 0f9729e54eb09..4c2d334d5d704 100644 --- a/core/routes.php +++ b/core/routes.php @@ -80,6 +80,7 @@ ['name' => 'TwoFactorChallenge#setupProvider', 'url' => 'login/setupchallenge/{providerId}', 'verb' => 'GET'], ['name' => 'TwoFactorChallenge#confirmProviderSetup', 'url' => 'login/setupchallenge/{providerId}', 'verb' => 'POST'], ['name' => 'OCJS#getConfig', 'url' => '/core/js/oc.js', 'verb' => 'GET'], + ['name' => 'Preview#preflighted_cors', 'url' => '/core/preview', 'verb' => 'OPTIONS'], ['name' => 'Preview#getPreviewByFileId', 'url' => '/core/preview', 'verb' => 'GET'], ['name' => 'Preview#getPreview', 'url' => '/core/preview.png', 'verb' => 'GET'], ['name' => 'RecommendedApps#index', 'url' => '/core/apps/recommended', 'verb' => 'GET'], diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index 9a9740b7bccc4..e29d4e2b98c91 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -233,7 +233,9 @@ public function __construct(string $appName, array $urlParams = [], ServerContai $c->get(IRequest::class), $c->get(IControllerMethodReflector::class), $c->get(IUserSession::class), - $c->get(OC\Security\Bruteforce\Throttler::class) + $c->get(OC\Security\Bruteforce\Throttler::class), + $c->get(IConfig::class), + $c->get('AppName') ) ); $dispatcher->registerMiddleware( diff --git a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php index 30ba8d8d6e4fb..6477d6854a621 100644 --- a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php @@ -37,6 +37,7 @@ use OCP\AppFramework\Http\Response; use OCP\AppFramework\Middleware; use OCP\IRequest; +use OCP\IConfig; /** * This middleware sets the correct CORS headers on a response if the @@ -45,7 +46,7 @@ * https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS */ class CORSMiddleware extends Middleware { - /** @var IRequest */ + /** @var IRequest */ private $request; /** @var ControllerMethodReflector */ private $reflector; @@ -53,21 +54,30 @@ class CORSMiddleware extends Middleware { private $session; /** @var Throttler */ private $throttler; + /** @var IConfig */ + private $config; + /** @var string */ + private $appName; /** * @param IRequest $request * @param ControllerMethodReflector $reflector * @param Session $session * @param Throttler $throttler + * @param string $app_name */ public function __construct(IRequest $request, ControllerMethodReflector $reflector, Session $session, - Throttler $throttler) { + Throttler $throttler, + IConfig $config, + $app_name) { $this->request = $request; $this->reflector = $reflector; $this->session = $session; $this->throttler = $throttler; + $this->config = $config; + $this->appName = $app_name; } /** @@ -83,7 +93,8 @@ public function __construct(IRequest $request, 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') || $this->session->isLoggedIn())) { + // Do nothing if HTTP_ORIGIN is not set + if ($this->reflector->hasAnnotation('CORS') && (!$this->reflector->hasAnnotation('PublicPage') || $this->session->isLoggedIn()) && isset($this->request->server['HTTP_ORIGIN'])) { $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; @@ -130,13 +141,15 @@ public function afterController($controller, $methodName, Response $response) { } $origin = $this->request->server['HTTP_ORIGIN']; - $response->addHeader('Access-Control-Allow-Origin', $origin); + if ($this->isOriginAllowed($origin, $this->appName)) { + $response->addHeader('Access-Control-Allow-Origin', $origin); + } } return $response; } /** - * If an SecurityException is being caught return a JSON error response + * If a 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 @@ -158,4 +171,32 @@ public function afterException($controller, $methodName, \Exception $exception) throw $exception; } + + /** + * Check if origin is allowed. + * + * @param string $origin The origin that will be checked + * @param string $app Optionally, the app that will provide the valid origin list + * + * @return bool + * True if origin is in allowed origins list. + */ + private function isOriginAllowed($origin, $app = null ): bool { + // Starting with no allowed origins. + $allowed_origins = []; + // Add first the general allowed origins if defined + // @TODO need to put the general allowed origin setting in suitable core app. + // $allowed_origins = [...$allowed_origins, ...$this->config->getAppValue('suitable_app', 'allowed_origins', [])]; + $allowed_origins = [...$allowed_origins, ...$this->config->getSystemValue('allowed_origins', [])]; + + //Then add the app namespace specific allowed origins if defined + if ($app !== null) { + // @TODO need to put this setting in suitable core app. + //$allowed_origins = [...$allowed_origins, ...$this->config->getAppValue('suitable_app', $app . '.allowed_origins', [])]; + $allowed_origins = [...$allowed_origins, ...$this->config->getSystemValue($app . '.allowed_origins', [])]; + } + $allowed_origins = array_map('trim', $allowed_origins); + + return in_array($origin, $allowed_origins, true); + } } From 57a1cd4026dd48431e2193ac1b655e9be8bd4ca2 Mon Sep 17 00:00:00 2001 From: Aleix Quintana Alsius Date: Mon, 24 Apr 2023 10:11:30 +0200 Subject: [PATCH 2/6] CS fixes --- .../Middleware/Security/CORSMiddleware.php | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php index 6477d6854a621..6b19c9ca1ff5c 100644 --- a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php @@ -36,8 +36,8 @@ use OCP\AppFramework\Http\JSONResponse; use OCP\AppFramework\Http\Response; use OCP\AppFramework\Middleware; -use OCP\IRequest; use OCP\IConfig; +use OCP\IRequest; /** * This middleware sets the correct CORS headers on a response if the @@ -67,11 +67,11 @@ class CORSMiddleware extends Middleware { * @param string $app_name */ public function __construct(IRequest $request, - ControllerMethodReflector $reflector, - Session $session, - Throttler $throttler, - IConfig $config, - $app_name) { + ControllerMethodReflector $reflector, + Session $session, + Throttler $throttler, + IConfig $config, + $app_name) { $this->request = $request; $this->reflector = $reflector; $this->session = $session; @@ -181,7 +181,7 @@ public function afterException($controller, $methodName, \Exception $exception) * @return bool * True if origin is in allowed origins list. */ - private function isOriginAllowed($origin, $app = null ): bool { + private function isOriginAllowed($origin, $app = null): bool { // Starting with no allowed origins. $allowed_origins = []; // Add first the general allowed origins if defined From e0877a8335b8dc2b92e4ad0da6fc9443c280f7aa Mon Sep 17 00:00:00 2001 From: Aleix Quintana Alsius Date: Fri, 28 Apr 2023 12:39:54 +0200 Subject: [PATCH 3/6] Add application to be used as settings frontend --- .../Middleware/Security/CORSMiddleware.php | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php index 6b19c9ca1ff5c..d605c8c28f67e 100644 --- a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php @@ -185,14 +185,16 @@ private function isOriginAllowed($origin, $app = null): bool { // Starting with no allowed origins. $allowed_origins = []; // Add first the general allowed origins if defined - // @TODO need to put the general allowed origin setting in suitable core app. - // $allowed_origins = [...$allowed_origins, ...$this->config->getAppValue('suitable_app', 'allowed_origins', [])]; + $cors_filter_settings_allowed_origins = $this->config->getAppValue('corsOriginFilterSettings', 'allowed_origins', []); + $cors_filter_settings_allowed_origins = explode(",", array_map('trim', $cors_filter_settings_allowed_origins)); + $allowed_origins = [...$allowed_origins, ...$cors_filter_settings_allowed_origins]; $allowed_origins = [...$allowed_origins, ...$this->config->getSystemValue('allowed_origins', [])]; //Then add the app namespace specific allowed origins if defined if ($app !== null) { - // @TODO need to put this setting in suitable core app. - //$allowed_origins = [...$allowed_origins, ...$this->config->getAppValue('suitable_app', $app . '.allowed_origins', [])]; + $cors_filter_settings_app_allowed_origins = $this->config->getAppValue('corsOriginFilterSettings', $app . 'allowed_origins', []); + $cors_filter_settings_app_allowed_origins = explode(",", array_map('trim', $cors_filter_settings_app_allowed_origins)); + $allowed_origins = [...$allowed_origins, ...$cors_filter_settings_app_allowed_origins]; $allowed_origins = [...$allowed_origins, ...$this->config->getSystemValue($app . '.allowed_origins', [])]; } $allowed_origins = array_map('trim', $allowed_origins); From e1406341d368764e3cecacf12e90fc53dc400366 Mon Sep 17 00:00:00 2001 From: Aleix Quintana Alsius Date: Sat, 29 Apr 2023 12:14:06 +0200 Subject: [PATCH 4/6] Fix php syntax errors --- .../Middleware/Security/CORSMiddleware.php | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php index f307959f601f9..e418048ecd928 100644 --- a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php @@ -99,9 +99,9 @@ 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 // also Do nothing if HTTP_ORIGIN is not set - if ($this->hasAnnotationOrAttribute($reflectionMethod, 'CORS', CORS::class) && - (!$this->hasAnnotationOrAttribute($reflectionMethod, 'PublicPage', PublicPage::class) || $this->session->isLoggedIn()) && - && isset($this->request->server['HTTP_ORIGIN'])) { + if ($this->hasAnnotationOrAttribute($reflectionMethod, 'CORS', CORS::class) && + (!$this->hasAnnotationOrAttribute($reflectionMethod, 'PublicPage', PublicPage::class) || $this->session->isLoggedIn()) && + isset($this->request->server['HTTP_ORIGIN'])) { $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; @@ -169,9 +169,10 @@ public function afterController($controller, $methodName, Response $response) { } } - $origin = $this->request->server['HTTP_ORIGIN']; - if ($this->isOriginAllowed($origin, $this->appName)) { - $response->addHeader('Access-Control-Allow-Origin', $origin); + $origin = $this->request->server['HTTP_ORIGIN']; + if ($this->isOriginAllowed($origin, $this->appName)) { + $response->addHeader('Access-Control-Allow-Origin', $origin); + } } } return $response; From 74309e326b5cf15a5c6f10fef7fb46eb09c84487 Mon Sep 17 00:00:00 2001 From: Aleix Quintana Alsius Date: Sat, 29 Apr 2023 12:37:17 +0200 Subject: [PATCH 5/6] Fix corsOriginFilterSettings exploding and protected isOriginAllowed --- .../Middleware/Security/CORSMiddleware.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php index e418048ecd928..f9ea91f1b200f 100644 --- a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php @@ -211,19 +211,19 @@ public function afterException($controller, $methodName, \Exception $exception) * @return bool * True if origin is in allowed origins list. */ - private function isOriginAllowed($origin, $app = null): bool { + protected function isOriginAllowed($origin, $app = null): bool { // Starting with no allowed origins. $allowed_origins = []; // Add first the general allowed origins if defined - $cors_filter_settings_allowed_origins = $this->config->getAppValue('corsOriginFilterSettings', 'allowed_origins', []); - $cors_filter_settings_allowed_origins = explode(",", array_map('trim', $cors_filter_settings_allowed_origins)); + $cors_filter_settings_allowed_origins = $this->config->getAppValue('corsOriginFilterSettings', 'allowed_origins', ''); + $cors_filter_settings_allowed_origins = array_map('trim', explode(",", $cors_filter_settings_allowed_origins)); $allowed_origins = [...$allowed_origins, ...$cors_filter_settings_allowed_origins]; $allowed_origins = [...$allowed_origins, ...$this->config->getSystemValue('allowed_origins', [])]; //Then add the app namespace specific allowed origins if defined if ($app !== null) { - $cors_filter_settings_app_allowed_origins = $this->config->getAppValue('corsOriginFilterSettings', $app . 'allowed_origins', []); - $cors_filter_settings_app_allowed_origins = explode(",", array_map('trim', $cors_filter_settings_app_allowed_origins)); + $cors_filter_settings_app_allowed_origins = $this->config->getAppValue('corsOriginFilterSettings', $app . 'allowed_origins', ''); + $cors_filter_settings_app_allowed_origins = array_map('trim', explode(",", $cors_filter_settings_app_allowed_origins)); $allowed_origins = [...$allowed_origins, ...$cors_filter_settings_app_allowed_origins]; $allowed_origins = [...$allowed_origins, ...$this->config->getSystemValue($app . '.allowed_origins', [])]; } From 7eeb2c81e265d9f96547491c03444a1e0b8b2185 Mon Sep 17 00:00:00 2001 From: Aleix Quintana Alsius Date: Sun, 30 Apr 2023 14:43:50 +0200 Subject: [PATCH 6/6] fix cs --- lib/private/AppFramework/Middleware/Security/CORSMiddleware.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php index f9ea91f1b200f..07e26c9750972 100644 --- a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php @@ -99,7 +99,7 @@ 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 // also Do nothing if HTTP_ORIGIN is not set - if ($this->hasAnnotationOrAttribute($reflectionMethod, 'CORS', CORS::class) && + if ($this->hasAnnotationOrAttribute($reflectionMethod, 'CORS', CORS::class) && (!$this->hasAnnotationOrAttribute($reflectionMethod, 'PublicPage', PublicPage::class) || $this->session->isLoggedIn()) && isset($this->request->server['HTTP_ORIGIN'])) { $user = array_key_exists('PHP_AUTH_USER', $this->request->server) ? $this->request->server['PHP_AUTH_USER'] : null;