diff --git a/apps/dav/composer/composer/autoload_classmap.php b/apps/dav/composer/composer/autoload_classmap.php index 6d41801728b..ba7b76e6edb 100644 --- a/apps/dav/composer/composer/autoload_classmap.php +++ b/apps/dav/composer/composer/autoload_classmap.php @@ -156,6 +156,7 @@ return array( 'OCA\\DAV\\Connector\\Sabre\\ChecksumUpdatePlugin' => $baseDir . '/../lib/Connector/Sabre/ChecksumUpdatePlugin.php', 'OCA\\DAV\\Connector\\Sabre\\CommentPropertiesPlugin' => $baseDir . '/../lib/Connector/Sabre/CommentPropertiesPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\CopyEtagHeaderPlugin' => $baseDir . '/../lib/Connector/Sabre/CopyEtagHeaderPlugin.php', + 'OCA\\DAV\\Connector\\Sabre\\CorsPlugin' => $baseDir . '/../lib/Connector/Sabre/CorsPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\DavAclPlugin' => $baseDir . '/../lib/Connector/Sabre/DavAclPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\Directory' => $baseDir . '/../lib/Connector/Sabre/Directory.php', 'OCA\\DAV\\Connector\\Sabre\\DummyGetResponsePlugin' => $baseDir . '/../lib/Connector/Sabre/DummyGetResponsePlugin.php', diff --git a/apps/dav/composer/composer/autoload_static.php b/apps/dav/composer/composer/autoload_static.php index 600397c371e..0e062c727ed 100644 --- a/apps/dav/composer/composer/autoload_static.php +++ b/apps/dav/composer/composer/autoload_static.php @@ -171,6 +171,7 @@ class ComposerStaticInitDAV 'OCA\\DAV\\Connector\\Sabre\\ChecksumUpdatePlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/ChecksumUpdatePlugin.php', 'OCA\\DAV\\Connector\\Sabre\\CommentPropertiesPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/CommentPropertiesPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\CopyEtagHeaderPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/CopyEtagHeaderPlugin.php', + 'OCA\\DAV\\Connector\\Sabre\\CorsPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/CorsPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\DavAclPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/DavAclPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\Directory' => __DIR__ . '/..' . '/../lib/Connector/Sabre/Directory.php', 'OCA\\DAV\\Connector\\Sabre\\DummyGetResponsePlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/DummyGetResponsePlugin.php', diff --git a/apps/dav/lib/Connector/Sabre/CorsPlugin.php b/apps/dav/lib/Connector/Sabre/CorsPlugin.php index 718403e356a..236e3000829 100644 --- a/apps/dav/lib/Connector/Sabre/CorsPlugin.php +++ b/apps/dav/lib/Connector/Sabre/CorsPlugin.php @@ -1,8 +1,10 @@ - * * @copyright Copyright (c) 2018, ownCloud GmbH + * + * @author Noveen Sachdeva + * @author Ferdinand Thiessen + * * @license AGPL-3.0 * * This code is free software: you can redistribute it and/or modify @@ -15,12 +17,13 @@ * GNU Affero General Public License for more details. * * You should have received a copy of the GNU Affero General Public License, version 3, - * along with this program. If not, see + * along with this program. If not, see * */ namespace OCA\DAV\Connector\Sabre; +use OCP\IConfig; use OCP\IUserSession; use OCP\Util; use Sabre\DAV\ServerPlugin; @@ -33,53 +36,18 @@ use Sabre\HTTP\ResponseInterface; class CorsPlugin extends ServerPlugin { /** * Reference to main server object - * - * @var \Sabre\DAV\Server */ - private $server; + private \Sabre\DAV\Server $server; - /** - * Reference to logged in user's session - * - * @var IUserSession - */ - private $userSession; - - /** @var array */ - private $extraHeaders; - /** - * @var bool - */ - private $alreadyExecuted = false; + private bool $alreadyExecuted = false; /** * @param IUserSession $userSession */ - public function __construct(IUserSession $userSession) { - $this->userSession = $userSession; - } - - private function getExtraHeaders(RequestInterface $request) { - if ($this->extraHeaders === null) { - if ($this->userSession->getUser() === null) { - $this->extraHeaders['Access-Control-Allow-Methods'] = [ - 'OPTIONS', - 'GET', - 'HEAD', - 'DELETE', - 'PROPFIND', - 'PUT', - 'PROPPATCH', - 'COPY', - 'MOVE', - 'REPORT', - 'SEARCH', - ]; - } else { - $this->extraHeaders['Access-Control-Allow-Methods'] = $this->server->getAllowedMethods($request->getPath()); - } - } - return $this->extraHeaders; + public function __construct( + private IUserSession $userSession, + private IConfig $config, + ) { } /** @@ -104,7 +72,12 @@ class CorsPlugin extends ServerPlugin { if ($this->ignoreOriginHeader($originHeader)) { return; } - if (Util::isSameDomain($originHeader, $request->getAbsoluteUrl())) { + try { + if (Util::isSameDomain($originHeader, $request->getAbsoluteUrl())) { + return; + } + } catch (\InvalidArgumentException $e) { + \OC::$server->getLogger()->debug('Invalid origin header was passed', ['Origin' => $originHeader, 'exception' => $e]); return; } @@ -125,24 +98,30 @@ class CorsPlugin extends ServerPlugin { * @return void */ public function setCorsHeaders(RequestInterface $request, ResponseInterface $response) { - if ($request->getHeader('origin') === null) { + $requesterDomain = $request->getHeader('Origin'); + if ($requesterDomain === null) { return; } + if ($this->alreadyExecuted) { return; } + $this->alreadyExecuted = true; - $requesterDomain = $request->getHeader('origin'); + // unauthenticated request shall add cors headers as well $userId = null; if ($this->userSession->getUser() !== null) { $userId = $this->userSession->getUser()->getUID(); } - $headers = \OC_Response::setCorsHeaders($userId, $requesterDomain, null, $this->getExtraHeaders($request)); - foreach ($headers as $key => $value) { - $response->addHeader($key, \implode(',', $value)); - } + \OC_Response::setCorsHeaders( + $response, + $userId, + $requesterDomain, + $this->config, + $this->server->getAllowedMethods($request->getPath()) + ); } /** @@ -159,7 +138,11 @@ class CorsPlugin extends ServerPlugin { if ($authorization === null || $authorization === '') { // Set the proper response $response->setStatus(200); - $response = \OC_Response::setOptionsRequestHeaders($response, $this->getExtraHeaders($request)); + \OC_Response::setOptionsRequestHeaders( + $response, + $this->config, + $this->server->getAllowedMethods($request->getPath()) + ); // Since All OPTIONS requests are unauthorized, we will have to return false from here // If we don't return false, due to no authorization, a 401-Unauthorized will be thrown @@ -173,8 +156,7 @@ class CorsPlugin extends ServerPlugin { /** * in addition to schemas used by extensions we ignore empty origin header * values as well as 'null' which is not valid by the specification but used - * by some clients. - * @link https://github.com/owncloud/core/pull/32120#issuecomment-407008243 + * by some clients (Buttercup and Tusk) * * @param string $originHeader * @return bool diff --git a/apps/dav/lib/Connector/Sabre/ServerFactory.php b/apps/dav/lib/Connector/Sabre/ServerFactory.php index eb6dfe8317e..b425bf2ab9b 100644 --- a/apps/dav/lib/Connector/Sabre/ServerFactory.php +++ b/apps/dav/lib/Connector/Sabre/ServerFactory.php @@ -100,7 +100,7 @@ class ServerFactory { // Load plugins $server->addPlugin(new \OCA\DAV\Connector\Sabre\MaintenancePlugin($this->config, $this->l10n)); - $server->addPlugin(new \OCA\DAV\Connector\Sabre\CorsPlugin($this->userSession)); + $server->addPlugin(new \OCA\DAV\Connector\Sabre\CorsPlugin($this->userSession, $this->config)); $server->addPlugin(new \OCA\DAV\Connector\Sabre\BlockLegacyClientPlugin($this->config)); $server->addPlugin(new \OCA\DAV\Connector\Sabre\AnonymousOptionsPlugin()); $server->addPlugin($authPlugin); diff --git a/apps/dav/lib/Server.php b/apps/dav/lib/Server.php index 84e55d6247c..de1c82c161c 100644 --- a/apps/dav/lib/Server.php +++ b/apps/dav/lib/Server.php @@ -131,7 +131,7 @@ class Server { $this->server->addPlugin(new ProfilerPlugin($this->request)); $this->server->addPlugin(new BlockLegacyClientPlugin(\OC::$server->getConfig())); $this->server->addPlugin(new AnonymousOptionsPlugin()); - $this->server->addPlugin(new CorsPlugin(\OC::$server->getUserSession())); + $this->server->addPlugin(new CorsPlugin(\OC::$server->getUserSession(), \OC::$server->getConfig())); $authPlugin = new Plugin(); $authPlugin->addBackend(new PublicAuth()); diff --git a/apps/dav/tests/unit/Connector/Sabre/CorsPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/CorsPluginTest.php index 48daf4fb819..4378dee131a 100644 --- a/apps/dav/tests/unit/Connector/Sabre/CorsPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/CorsPluginTest.php @@ -1,13 +1,14 @@ + * @author Ferdinand Thiessen * - * @copyright Copyright (c) 2018, ownCloud GmbH - * @license AGPL-3.0 + * @copyright Copyright (c) 2023, Nextcloud GmbH + * @license AGPL-3.0-or-later * * This code is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License, version 3, - * as published by the Free Software Foundation. + * it under the terms of the GNU Affero General Public License + * as published by the Free Software Foundation, + * either version 3 of the License, or (at your option) any later version. * * This program is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of @@ -24,352 +25,348 @@ use OCA\DAV\Connector\Sabre\CorsPlugin; use OCP\IUserSession; use OCP\IUser; use OCP\IConfig; +use PHPUnit\Framework\MockObject\MockObject; use Sabre\DAV\Server; use Sabre\DAV\ServerPlugin; +use stdClass; use Test\TestCase; class CorsPluginTest extends TestCase { /** - * @var Server - */ - private $server; - - /** - * @var CorsPlugin - */ - private $plugin; - - /** - * @var IUserSession | \PHPUnit\Framework\MockObject\MockObject + * @var IUserSession|MockObject */ private $userSession; /** - * @var IConfig | \PHPUnit\Framework\MockObject\MockObject + * @var IConfig|MockObject */ private $config; public function setUp(): void { parent::setUp(); - $this->server = new Server(); - - $this->server->sapi = $this->getMockBuilder(\stdClass::class) - ->setMethods(['sendResponse']) - ->getMock(); - - $this->server->httpRequest->setMethod('OPTIONS'); - $this->userSession = $this->createMock(IUserSession::class); - $this->config = $this->createMock(IConfig::class); $this->overwriteService('AllConfig', $this->config); - - $this->plugin = new CorsPlugin($this->userSession); - - /** @var ServerPlugin | \PHPUnit\Framework\MockObject\MockObject $extraMethodPlugin */ - $extraMethodPlugin = $this->createMock(ServerPlugin::class); - $extraMethodPlugin->method('getHTTPMethods') - ->with('owncloud/remote.php/dav/files/user1/target/path') - ->willReturn(['EXTRA']); - $extraMethodPlugin->method('getFeatures')->willReturn([]); - - $this->server->addPlugin($extraMethodPlugin); } public function tearDown(): void { $this->restoreService('AllConfig'); } - public function optionsCases() { - $allowedDomains = '["https://requesterdomain.tld", "http://anotherdomain.tld"]'; - - $allowedHeaders = [ - 'OC-Checksum', 'OC-Total-Length', 'OCS-APIREQUEST', 'X-OC-Mtime', - 'OC-RequestAppPassword', 'Accept', - 'Authorization', 'Brief', 'Content-Length', 'Content-Range', - 'Content-Type', 'Date', 'Depth', 'Destination', 'Host', 'If', 'If-Match', - 'If-Modified-Since', 'If-None-Match', 'If-Range', 'If-Unmodified-Since', - 'Location', 'Lock-Token', 'Overwrite', 'Prefer', 'Range', 'Schedule-Reply', - 'Timeout', 'User-Agent', 'X-Expected-Entity-Length', 'Accept-Language', - 'Access-Control-Request-Method', 'Access-Control-Allow-Origin', 'Cache-Control', 'ETag', - 'OC-Autorename', 'OC-CalDav-Import', 'OC-Chunked', 'OC-Etag', 'OC-FileId', - 'OC-LazyOps', 'OC-Total-File-Length', 'Origin', 'X-Request-ID', 'X-Requested-With' - ]; - $allowedMethods = [ - 'GET', - 'OPTIONS', - 'POST', - 'PUT', - 'DELETE', - 'MKCOL', - 'PROPFIND', - 'PATCH', - 'PROPPATCH', - 'REPORT', - 'HEAD', - 'COPY', - 'MOVE', - 'EXTRA', - ]; - $allowedMethodsUnAuthenticated = [ - 'GET', - 'OPTIONS', - 'POST', - 'PUT', - 'DELETE', - 'MKCOL', - 'PROPFIND', - 'PATCH', - 'PROPPATCH', - 'REPORT', - 'HEAD', - 'COPY', - 'MOVE', - ]; - + public function dataInit() { return [ - 'OPTIONS headers' => - [ - $allowedDomains, - false, - [ - 'Origin' => 'https://requesterdomain.tld', - ], - 200, - [ - 'Access-Control-Allow-Headers' => \implode(',', $allowedHeaders), - 'Access-Control-Allow-Origin' => '*', - 'Access-Control-Allow-Methods' => \implode(',', $allowedMethodsUnAuthenticated), - ], + 'missing origin will be ignored' => [ + [], + '', + '', false ], - 'OPTIONS headers with user' => - [ - $allowedDomains, - true, - [ - 'Origin' => 'https://requesterdomain.tld', - 'Authorization' => 'abc', - ], - 200, - [ - 'Access-Control-Allow-Headers' => \implode(',', $allowedHeaders), - 'Access-Control-Allow-Origin' => 'https://requesterdomain.tld', - 'Access-Control-Allow-Methods' => \implode(',', $allowedMethods), - ], - true - ], - 'OPTIONS headers no user' => - [ - $allowedDomains, + 'invalid null origin will be ignored' => [ + ['Origin' => null], + '/', + 'http://cloud.example.com/', false, - [ - 'Origin' => 'https://requesterdomain.tld', - 'Authorization' => 'abc', - ], - 200, - [ - 'Access-Control-Allow-Headers' => null, - 'Access-Control-Allow-Origin' => null, - 'Access-Control-Allow-Methods' => null, - ], - true ], - 'OPTIONS headers domain not allowed' => - [ - '[]', - true, - [ - 'Origin' => 'https://requesterdomain.tld', - 'Authorization' => 'abc', - ], - 200, - [ - 'Access-Control-Allow-Headers' => null, - 'Access-Control-Allow-Origin' => null, - 'Access-Control-Allow-Methods' => null, - ], - true + 'invalid empty origin will be ignored' => [ + ['Origin' => ''], + '/', + 'http://cloud.example.com/', + false, ], - 'OPTIONS headers not allowed but no cross-domain' => - [ - '[]', - true, - [ - 'Origin' => 'https://requesterdomain.tld', - 'Authorization' => 'abc', - ], - 200, - [ - 'Access-Control-Allow-Headers' => null, - 'Access-Control-Allow-Origin' => null, - 'Access-Control-Allow-Methods' => null, - ], - true + 'invalid null string origin will be ignored' => [ + ['Origin' => 'null'], + '/', + 'http://cloud.example.com/', + false, ], - 'OPTIONS headers allowed but no cross-domain' => - [ - '["currentdomain.tld:8080"]', - true, - [ - 'Origin' => 'https://currentdomain.tld:8080', - 'Authorization' => 'abc', - ], - 200, - [ - 'Access-Control-Allow-Headers' => null, - 'Access-Control-Allow-Origin' => null, - 'Access-Control-Allow-Methods' => null, - ], - true + 'invalid moz schema origin will be ignored' => [ + ['Origin' => 'moz-extension://domain.tld'], + '/', + 'http://cloud.example.com/', + false, ], - 'OPTIONS headers allowed, cross-domain through different port' => - [ - '["https://currentdomain.tld:8443"]', - true, - [ - 'Origin' => 'https://currentdomain.tld:8443', - 'Authorization' => 'abc', - ], - 200, - [ - 'Access-Control-Allow-Headers' => \implode(',', $allowedHeaders), - 'Access-Control-Allow-Origin' => 'https://currentdomain.tld:8443', - 'Access-Control-Allow-Methods' => \implode(',', $allowedMethods), - ], - true + 'invalid chrome schema origin will be ignored' => [ + ['Origin' => 'chrome-extension://domain.tld'], + '/', + 'http://cloud.example.com/', + false, ], - 'no Origin header' => - [ - $allowedDomains, + 'same origin will be ignored' => [ + ['Origin' => 'http://cloud.example.com'], + 'remote.php/dav/some/service', + 'http://cloud.example.com/remote.php/dav/some/service', + false, + ], + 'different sub domain will register' => [ + ['Origin' => 'http://www.example.com'], + 'remote.php/dav/some/service', + 'http://cloud.example.com/remote.php/dav/some/service', + true, + ], + 'valid origin will register' => [ + ['Origin' => 'http://domain.tld'], + 'remote.php/dav/some/service', + 'http://cloud.example.com/remote.php/dav/some/service', true, - [ - ], - 200, - [ - 'Access-Control-Allow-Headers' => null, - 'Access-Control-Allow-Origin' => null, - 'Access-Control-Allow-Methods' => null, - ], - true ], ]; } /** - * @dataProvider optionsCases - * @param $allowedDomains - * @param $hasUser - * @param $requestHeaders - * @param $expectedStatus - * @param array $expectedHeaders - * @param bool $expectDavHeaders + * @dataProvider dataInit */ - public function testOptionsHeaders($allowedDomains, $hasUser, $requestHeaders, $expectedStatus, array $expectedHeaders, $expectDavHeaders = false) { - $this->server->sapi->expects($this->once())->method('sendResponse')->with($this->server->httpResponse); - $user = $this->createMock(IUser::class); - $user->method('getUID')->willReturn('someuser'); + public function testInit($requestHeaders, $requestUrl, $requestAbsolutUrl, $shouldRegister) { + $request = new \Sabre\HTTP\Request('OPTIONS', $requestUrl, $requestHeaders); + $request->setAbsoluteUrl($requestAbsolutUrl); - if ($hasUser) { - $this->userSession->method('getUser')->willReturn($user); - } else { - $this->userSession->method('getUser')->willReturn(null); - } + /** + * @var MockObject|Server + */ + $server = $this->createMock(Server::class); + $server->httpRequest = $request; + $server->expects($this->exactly($shouldRegister ? 3 : 0))->method('on'); - $this->config->method('getSystemValue')->willReturn([]); - $this->config->method('getUserValue') - ->with('someuser', 'core', 'domains') - ->willReturn($allowedDomains); + $plugin = new CorsPlugin($this->userSession, $this->config); - $this->server->httpRequest->setHeaders($requestHeaders); - $this->server->httpRequest->setAbsoluteUrl('https://currentdomain.tld:8080/owncloud/remote.php/dav/files/user1/target/path'); - $this->server->httpRequest->setUrl('/owncloud/remote.php/dav/files/user1/target/path'); - - $this->server->addPlugin($this->plugin); - $this->server->start(); - - $this->assertEquals($expectedStatus, $this->server->httpResponse->getStatus()); - - foreach ($expectedHeaders as $headerKey => $headerValue) { - if ($headerValue !== null) { - $this->assertTrue($this->server->httpResponse->hasHeader($headerKey), "Response header \"$headerKey\" exists"); - } else { - $this->assertFalse($this->server->httpResponse->hasHeader($headerKey), "Response header \"$headerKey\" does not exist"); - } - $this->assertEquals($headerValue, $this->server->httpResponse->getHeader($headerKey)); - } - - // if it has DAV headers, it means we did not bypass further processing - $this->assertEquals($expectDavHeaders, $this->server->httpResponse->hasHeader('DAV')); + $plugin->initialize($server); } - /** - * @dataProvider providesOriginUrls - * @param $expectedValue - * @param $url - */ - public function testExtensionRequests($expectedValue, $url) { - $plugin = new CorsPlugin($this->createMock(IUserSession::class)); - self::assertEquals($expectedValue, $plugin->ignoreOriginHeader($url)); - } - - public function providesOriginUrls() { + public function dataOnException() { return [ - 'Firefox extension' => [true, 'moz-extension://mgmnhfbjphngabcpbpmapnnaabhnchmi/'], - 'Chrome extension' => [true, 'chrome-extension://mgmnhfbjphngabcpbpmapnnaabhnchmi/'], - 'Empty Origin' => [true, ''], - 'Null string Origin' => [true, 'null'], - 'Null Origin' => [true, null], - 'plain http' => [false, 'http://example.net/'], + 'with allowed domain' => [ + ['http://some.domain'], + true, + ], + 'without allowed domain' => [ + ['http://other.domain'], + false, + ], + 'without any domain' => [ + [], + false, + ], ]; } - public function testAuthenticatedAdditionalAllowedHeaders() { - $user = $this->createMock(IUser::class); - $user->method('getUID')->willReturn('someuser'); + /** + * @dataProvider dataOnException + */ + public function testOnException(array $allowedDomains, bool $shouldHaveCORS) { + $request = new \Sabre\HTTP\Request('OPTIONS', 'dav/action', ['Origin' => 'http://some.domain']); + $request->setAbsoluteUrl('http://cloud.example.com/dav/action'); - $this->userSession->method('getUser')->willReturn($user); - $this->server->httpRequest->setHeader('Origin', 'https://requesterdomain.tld'); - $this->server->httpRequest->setUrl('/owncloud/remote.php/dav/files/user1/target/path'); + $response = new \Sabre\HTTP\Response(200); - $this->config->method('getSystemValue')->withConsecutive( - ['cors.allowed-domains', []], - ['cors.allowed-headers', []] - ) - ->willReturnMap([ - ['cors.allowed-domains', [], []], - ['cors.allowed-headers', [], ['X-Additional-Configured-Header', 'authorization']] - ]); - $this->config->method('getUserValue')->willReturn('["https://requesterdomain.tld"]'); + $server = $this->createMock(Server::class); + $server->httpRequest = $request; + $server->httpResponse = $response; - $this->server->addPlugin($this->plugin); + $exception = $this->createMock(\Throwable::class); - $this->plugin->setCorsHeaders($this->server->httpRequest, $this->server->httpResponse); - self::assertEquals( - 'X-Additional-Configured-Header,authorization,OC-Checksum,OC-Total-Length,OCS-APIREQUEST,X-OC-Mtime,OC-RequestAppPassword,Accept,Authorization,Brief,Content-Length,Content-Range,Content-Type,Date,Depth,Destination,Host,If,If-Match,If-Modified-Since,If-None-Match,If-Range,If-Unmodified-Since,Location,Lock-Token,Overwrite,Prefer,Range,Schedule-Reply,Timeout,User-Agent,X-Expected-Entity-Length,Accept-Language,Access-Control-Request-Method,Access-Control-Allow-Origin,Cache-Control,ETag,OC-Autorename,OC-CalDav-Import,OC-Chunked,OC-Etag,OC-FileId,OC-LazyOps,OC-Total-File-Length,Origin,X-Request-ID,X-Requested-With', - $this->server->httpResponse->getHeader('Access-Control-Allow-Headers') - ); + $this->config->method('getSystemValue')->willReturnCallback(fn($name, $default) => match(true) { + $name === 'cors.allowed-domains' => $allowedDomains, + default => $default, + }); + + $plugin = new CorsPlugin($this->userSession, $this->config); + $plugin->initialize($server); + + $plugin->onException($exception); + $this->assertTrue($response->hasHeader('Access-Control-Allow-Origin') === $shouldHaveCORS); } - public function testUnauthenticatedAdditionalAllowedHeaders() { - $this->userSession->method('getUser')->willReturn(null); - $this->server->httpRequest->setHeader('Origin', 'https://requesterdomain.tld'); + public function dataSetOptionsHeaders() { + return [ + 'skip requests with authorization header for CSRF' => [ + // headers of the request + ['Authorization' => 'Basic YWxhZGRpbjpvcGVuc2VzYW1l', 'Origin' => 'http://some.domain'], + // expected return value of the function + null, + // should the response have the cors header + false, + // should the response be sent directly (meaning skipped) + false, + //response status + 500, + ], + 'without authorization header the OPTIONS request is answered' => [ + // headers of the request + ['Origin' => 'http://some.domain'], + // expected return value of the function + false, + // should the response have the cors header + true, + // should the response be sent directly (meaning skipped) + true, + //response status + 200, + ] + ]; + } - $this->config->method('getSystemValue')->withConsecutive( - ['cors.allowed-domains', []], - ['cors.allowed-headers', []] - ) - ->willReturnMap([ - ['cors.allowed-domains', [], ['https://requesterdomain.tld']], - ['cors.allowed-headers', [], ['X-Additional-Configured-Header', 'authorization']] - ]); + /** + * @dataProvider dataSetOptionsHeaders + */ + public function testSetOptionsHeaders($requestHeaders, $expectedReturn, $shouldHaveCORS, $shouldSendResponse, $responseStatus) { + $request = new \Sabre\HTTP\Request('OPTIONS', 'dav/action', $requestHeaders); + $request->setAbsoluteUrl('http://cloud.example.com/dav/action'); - $this->server->addPlugin($this->plugin); + $response = new \Sabre\HTTP\Response(); - $this->plugin->setCorsHeaders($this->server->httpRequest, $this->server->httpResponse); - self::assertEquals( - 'X-Additional-Configured-Header,authorization,OC-Checksum,OC-Total-Length,OCS-APIREQUEST,X-OC-Mtime,OC-RequestAppPassword,Accept,Authorization,Brief,Content-Length,Content-Range,Content-Type,Date,Depth,Destination,Host,If,If-Match,If-Modified-Since,If-None-Match,If-Range,If-Unmodified-Since,Location,Lock-Token,Overwrite,Prefer,Range,Schedule-Reply,Timeout,User-Agent,X-Expected-Entity-Length,Accept-Language,Access-Control-Request-Method,Access-Control-Allow-Origin,Cache-Control,ETag,OC-Autorename,OC-CalDav-Import,OC-Chunked,OC-Etag,OC-FileId,OC-LazyOps,OC-Total-File-Length,Origin,X-Request-ID,X-Requested-With', - $this->server->httpResponse->getHeader('Access-Control-Allow-Headers') - ); + $server = $this->createMock(Server::class); + /** @var MockObject */ + $server->sapi = $this->getMockBuilder(\stdClass::class)->addMethods(['sendResponse'])->getMock(); + $server->httpRequest = $request; + $server->httpResponse = $response; + $server->sapi + ->expects($shouldSendResponse ? $this->once() : $this->never()) + ->method('sendResponse') + ->with($response); + + $this->config->method('getSystemValue')->willReturnCallback(fn($name, $default) => $default); + + $plugin = new CorsPlugin($this->userSession, $this->config); + $plugin->initialize($server); + + $this->assertEquals($expectedReturn, $plugin->setOptionsRequestHeaders($request, $response)); + $this->assertEquals($responseStatus, $response->getStatus()); + $this->assertTrue($response->hasHeader('Access-Control-Allow-Origin') === $shouldHaveCORS); + } + + public function dataSetCORSHeaders() { + return [ + 'no header on missing origin' => [ + // method + 'PROPFIND', + // request headers + [], + // allow users domain list + false, + // already executed + false, + // user logged in + false, + // extra headers + ['PROPFIND'], + // added cors methods + null, + // the resulting cors domain + null, + ], + 'allowed origin will set header' => [ + // method + 'PROPFIND', + // request headers + ['Origin' => 'http://good.example.com'], + // allow users domain list + false, + // already executed + false, + // user logged in + false, + // allowed methods + ['PROPFIND', 'SEARCH'], + // added cors methods + ['PROPFIND', 'SEARCH'], + // the resulting cors domain + 'http://good.example.com', + ], + 'no header if already executed' => [ + // method + 'PROPFIND', + // request headers + ['Origin' => 'http://good.example.com'], + // allow users domain list + false, + // already executed + true, + // user logged in + false, + // allowed methods + ['PROPFIND'], + // added cors methods + null, + // the resulting cors domain + null, + ], + 'set header on user domain' => [ + // method + 'PROPFIND', + // request headers + ['Origin' => 'http://users.tld'], + // allow users domain list + true, + // already executed + false, + // user logged in + true, + // allowed methods + ['PROPFIND'], + // added cors methods + ['PROPFIND'], + // the resulting cors domain + 'http://users.tld', + ], + 'no header is set when user also do not match' => [ + // method + 'PROPFIND', + // request headers + ['Origin' => 'http://some-other.tld'], + // allow users domain list + true, + // already executed + false, + // user logged in + true, + // allowed methods + ['PROPFIND'], + // added cors methods + null, + // the resulting cors domain + null, + ], + ]; + } + + /** + * @dataProvider dataSetCORSHeaders + */ + public function testSetCORSHeaders(string $requestMethod, array $requestHeaders, bool $allowUserConfig, bool $alreadyExecuted, bool $hasUser, array $allowedHeaders, ?array $corsMethods, string|null $corsDomain) { + $request = new \Sabre\HTTP\Request($requestMethod, 'dav/action', $requestHeaders); + $request->setAbsoluteUrl('http://cloud.example.com/dav/action'); + + $response = new \Sabre\HTTP\Response(); + + if ($hasUser === true) { + /** @var MockObject */ + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('someuser'); + $this->userSession->method('getUser')->willReturn($user); + } + + $this->config->method('getSystemValue')->willReturnCallback(fn($name, $default) => match(true) { + $name === 'cors.allowed-domains' => ['http://good.example.com'], + default => $default, + }); + $this->config->method('getSystemValueBool')->with('cors.allow-user-domains')->willReturn($allowUserConfig); + $this->config->method('getUserValue')->with('someuser', 'core', 'cors.allowed-domains')->willReturn('["http://users.tld"]'); + + /** @var Server|MockObject */ + $server = $this->createMock(Server::class); + $server->method('getAllowedMethods')->with('dav/action')->willReturn($allowedHeaders); + $server->httpRequest = $request; + $server->httpResponse = $response; + + /** @var CorsPlugin|MockObject */ + $plugin = new CorsPlugin($this->userSession, $this->config); + $plugin->initialize($server); + $plugin->setCorsHeaders($request, $response); + if ($alreadyExecuted) { + $response = new \Sabre\HTTP\Response(); + $plugin->setCorsHeaders($request, $response); + } + + $this->assertEquals($corsDomain, $response->getHeader('Access-Control-Allow-Origin')); + $this->assertEquals($corsMethods === null ? null : join(',', $corsMethods), $response->getHeader('Access-Control-Allow-Methods')); } } diff --git a/cypress/e2e/dav.cy.ts b/cypress/e2e/dav.cy.ts new file mode 100644 index 00000000000..b205ba0ac07 --- /dev/null +++ b/cypress/e2e/dav.cy.ts @@ -0,0 +1,117 @@ +/** + * @copyright Copyright (c) 2022 John Molakvoæ + * + * @author John Molakvoæ + * + * @license AGPL-3.0-or-later + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ +describe('check if dav app supports CORS', function() { + describe('OPTIONS requests', () => { + it('set the access control header', function() { + const url = new URL(Cypress.config('baseUrl') as string) + url.pathname = 'remote.php/dav/files/admin/' + + cy.request({ + method: 'OPTIONS', + url: url.toString(), + headers: { + Origin: 'http://example.com', + 'Access-Control-Allow-Methods': 'http://example.com', + }, + }).then(response => { + expect(response.status).to.equal(200) + expect(response).to.have.property('headers') + expect(response.headers).to.have.property('access-control-allow-origin') + expect(response.headers['access-control-allow-origin']).to.equal('*') + }) + }) + + it('does not set header if Origin is invalid', function() { + const url = new URL(Cypress.config('baseUrl') as string) + url.pathname = 'remote.php/dav/files/admin/' + + cy.request({ + method: 'OPTIONS', + url: url.toString(), + failOnStatusCode: false, + headers: { + Origin: 'example.com', + 'Access-Control-Allow-Methods': 'http://example.com', + }, + }).then(response => { + expect(response.headers).to.not.have.property('access-control-allow-origin') + }) + }) + }) + + describe('WebDAV requests', () => { + before(() => { + cy.runOccCommand('config:system:set --value "http://good.example.com" cors.allowed-domains 0') + }) + after(() => { + cy.runOccCommand('config:system:delete cors.allowed-domains') + }) + it('do not set CORS headers when origin is not allowed', function() { + const url = new URL(Cypress.config('baseUrl') as string) + url.pathname = 'remote.php/dav/files/admin/' + + cy.request({ + method: 'PROPFIND', + url: url.toString(), + failOnStatusCode: false, + headers: { + Origin: 'http://example.com', + Authorization: `Basic ${btoa('admin:admin')}`, + }, + body: ` + + + + +`, + }).then(response => { + expect(response.status).to.equal(207) + expect(response.headers).to.not.have.property('access-control-allow-origin') + }) + }) + + it('set CORS headers when origin is allowed', function() { + const url = new URL(Cypress.config('baseUrl') as string) + url.pathname = 'remote.php/dav/files/admin/' + + cy.request({ + method: 'PROPFIND', + url: url.toString(), + failOnStatusCode: false, + headers: { + Origin: 'http://good.example.com', + Authorization: `Basic ${btoa('admin:admin')}`, + }, + body: ` + + + + +`, + }).then(response => { + expect(response.status).to.equal(207) + expect(response.headers).to.have.property('access-control-allow-origin') + expect(response.headers['access-control-allow-origin']).to.equal('http://good.example.com') + }) + }) + }) +}) diff --git a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php index 24341688253..6b8cf43a40e 100644 --- a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php @@ -8,6 +8,7 @@ * @author Lukas Reschke * @author Morris Jobke * @author Stefan Weil + * @author Ferdinand Thiessen * * @license AGPL-3.0 * @@ -68,22 +69,21 @@ class CORSMiddleware extends Middleware { */ public function afterController($controller, $methodName, Response $response) { $userId = !is_null($this->session->getUser()) ? $this->session->getUser()->getUID() : null; + $requesterDomain = $this->request->getHeader("Origin"); - // only react if it's a CORS request and if the request sends origin and + // only react if it's a CORS request and if the request sends origin and user is logged in $reflectionMethod = new ReflectionMethod($controller, $methodName); - if ($this->request->getHeader("Origin") !== null - && $this->hasAnnotationOrAttribute($reflectionMethod, 'CORS', CORS::class) - && !is_null($userId)) { - $requesterDomain = $this->request->getHeader("Origin"); - \OC_Response::setCorsHeaders($userId, $requesterDomain, $this->config); + if ($requesterDomain !== '' + && !is_null($userId) + && $this->hasAnnotationOrAttribute($reflectionMethod, 'CORS', CORS::class)) { + \OC_Response::setCorsHeaders($response, $userId, $requesterDomain, $this->config); // 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'; + $msg = 'Access-Control-Allow-Credentials must not be set to true in order to prevent CSRF'; throw new SecurityException($msg); } } diff --git a/lib/private/legacy/OC_Response.php b/lib/private/legacy/OC_Response.php index a4646799c62..694688842b7 100644 --- a/lib/private/legacy/OC_Response.php +++ b/lib/private/legacy/OC_Response.php @@ -11,6 +11,7 @@ * @author Roeland Jago Douma * @author Thomas Müller * @author Vincent Petry + * @author Ferdinand Thiessen * * @license AGPL-3.0 * @@ -107,10 +108,11 @@ class OC_Response { /** * This function adds the CORS headers if the requester domain is white-listed * + * @param \OCP\AppFramework\Http\Response|Sabre\HTTP\ResponseInterface $response * @param string $userId * @param string $domain - * @param \OCP\IConfig $config - * @param array $headers + * @param \OCP\IConfig|null $config + * @param array $headers Additional CORS headers to merge when setting * * Format of $headers: * Array [ @@ -118,44 +120,47 @@ class OC_Response { * "Access-Control-Allow-Origin": ["a", "b", "c"], * "Access-Control-Allow-Methods": ["a", "b", "c"] * ] - * - * @return array */ - public static function setCorsHeaders($userId, $domain, \OCP\IConfig $config = null, array $headers = []) { - if ($config === null) { + public static function setCorsHeaders($response, ?string $userId, string $domain, $config = null, array|null $methods = null) { + if (is_null($config)) { $config = \OC::$server->getConfig(); } + // first check if any of the global CORS domains matches $globalAllowedDomains = $config->getSystemValue('cors.allowed-domains', []); $isCorsRequest = (\is_array($globalAllowedDomains) && \in_array($domain, $globalAllowedDomains, true)); - if (!$isCorsRequest && $userId !== null) { - // check if any of the user specific CORS domains matches - $allowedDomains = \json_decode($config->getUserValue($userId, 'core', 'domains'), true); + // check if user defined CORS domains are enabled + $isUserCorsEnabled = $config->getSystemValueBool('cors.allow-user-domains', false); + + // if not a global CORS domain, but user defined ones are enabled, check if one matches + if (!$isCorsRequest && $isUserCorsEnabled && $userId !== null) { + $allowedDomains = \json_decode($config->getUserValue($userId, 'core', 'cors.allowed-domains'), true); $isCorsRequest = (\is_array($allowedDomains) && \in_array($domain, $allowedDomains, true)); } + + // Global or user domain matches so set headers if ($isCorsRequest) { - // TODO: infer allowed verbs from existing known routes - $allHeaders['Access-Control-Allow-Headers'] = self::getAllowedCorsHeaders($config); - $allHeaders['Access-Control-Expose-Headers'] = self::getExposeCorsHeaders(); - $allHeaders['Access-Control-Allow-Origin'] = [$domain]; - $allHeaders['Access-Control-Allow-Methods'] = ['GET', 'OPTIONS', 'POST', 'PUT', 'DELETE', 'MKCOL', 'PROPFIND', 'PATCH', 'PROPPATCH', 'REPORT']; + $allHeaders = [ + 'Access-Control-Allow-Origin' => [$domain], + 'Access-Control-Allow-Headers' => self::getAllowedCorsHeaders($config), + 'Access-Control-Expose-Headers' => self::getExposeCorsHeaders(), + 'Access-Control-Allow-Methods' => $methods ?? self::getAllowedCorsMethods(), + // Indicate that the response might change depending on the origin + 'Vary' => ['Origin'], + ]; - foreach ($headers as $key => $value) { - if (\array_key_exists($key, $allHeaders)) { - $allHeaders[$key] = \array_unique(\array_merge($allHeaders[$key], $value)); - } + foreach ($allHeaders as $key => $value) { + $response->addHeader($key, \join(',', $value)); } - - return $allHeaders; } - return []; } /** * This function adds the CORS headers for all domains * - * @param Sabre\HTTP\ResponseInterface $response - * @param array $headers + * @param \OCP\AppFramework\Http\Response|Sabre\HTTP\ResponseInterface $response + * @param \OCP\IConfig|null $config + * @param array $headers Additional cors headers to merge when setting * * Format of $headers: * Array [ @@ -164,26 +169,18 @@ class OC_Response { * "Access-Control-Allow-Methods": ["a", "b", "c"] * ] * - * @param \OCP\IConfig|null $config - * @return Sabre\HTTP\ResponseInterface $response + * @return void */ - public static function setOptionsRequestHeaders($response, $headers = [], \OCP\IConfig $config = null) { - // TODO: infer allowed verbs from existing known routes - $allHeaders['Access-Control-Allow-Headers'] = self::getAllowedCorsHeaders($config); - $allHeaders['Access-Control-Allow-Origin'] = ['*']; - $allHeaders['Access-Control-Allow-Methods'] = self::getAllowedCorsMethods(); - - foreach ($headers as $key => $value) { - if (\array_key_exists($key, $allHeaders)) { - $allHeaders[$key] = \array_unique(\array_merge($allHeaders[$key], $value)); - } - } + public static function setOptionsRequestHeaders($response, \OCP\IConfig $config = null, ?array $methods = null) { + $allHeaders = [ + 'Access-Control-Allow-Headers' => self::getAllowedCorsHeaders($config), + 'Access-Control-Allow-Origin' => ['*'], + 'Access-Control-Allow-Methods' => $methods ?? self::getAllowedCorsMethods(), + ]; foreach ($allHeaders as $key => $value) { - $response->addHeader($key, \implode(',', $value)); + $response->addHeader($key, \join(',', $value)); } - - return $response; } /** @@ -203,7 +200,10 @@ class OC_Response { 'PATCH', 'PROPPATCH', 'REPORT', - 'SEARCH' + 'SEARCH', + 'COPY', + 'MOVE', + 'HEAD', ]; } diff --git a/lib/public/Util.php b/lib/public/Util.php index 8811db185dd..def43b34c48 100644 --- a/lib/public/Util.php +++ b/lib/public/Util.php @@ -635,6 +635,7 @@ class Util { * * @param string $url full url * @return string protocol, domain and port as string + * @throws \InvalidArgumentException On invalid URL * @since 28.0.0 */ public static function getFullDomain(string $url): string { @@ -647,17 +648,16 @@ class Util { } $protocol = \strtolower($parts['scheme']); $host = \strtolower($parts['host']); - $port = null; - if ($protocol === 'http') { - $port = 80; - } elseif ($protocol === 'https') { - $port = 443; - } else { - throw new \InvalidArgumentException('Only http based URLs supported'); - } - if (isset($parts['port']) && $port !== '') { - $port = $parts['port']; + $port = $parts['port'] ?? null; + if ($port === null || $port === '') { + if ($protocol === 'http') { + $port = 80; + } elseif ($protocol === 'https') { + $port = 443; + } else { + throw new \InvalidArgumentException('Only http based URLs supported'); + } } return $protocol . '://' . \strtolower($host) . ':' . $port; @@ -673,6 +673,7 @@ class Util { * @param string $url2 * * @return bool true if both URLs have the same protocol, domain and port + * @throws \InvalidArgumentException On invalid URL * * @since 28.0.0 */ diff --git a/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php index 0fe429258ef..a5709675b89 100644 --- a/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php @@ -1,12 +1,25 @@ * * @author Bernhard Posselt - * @copyright Bernhard Posselt 2014 + * @author Ferdinand Thiessen + * + * @license AGPL-3.0-or-later + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * */ namespace Test\AppFramework\Middleware\Security; @@ -20,6 +33,7 @@ use OCP\AppFramework\Http\Response; use OCP\IConfig; use OCP\IRequest; use OCP\IRequestId; +use OCP\IUser; use OCP\IUserSession; use OCP\Security\Bruteforce\IThrottler; use PHPUnit\Framework\MockObject\MockObject; @@ -59,17 +73,6 @@ class CORSMiddlewareTest extends \Test\TestCase { ['testSetCORSAPIHeader'], ['testSetCORSAPIHeaderAttribute'], ]; - - $this->session = $this->getMockBuilder('\OC\User\Session') - ->disableOriginalConstructor() - ->getMock(); - - $user = $this->createMock(IUser::class); - $user->method('getUID')->willReturn('user'); - $userSession = $this->createMock(IUserSession::class); - $userSession->method('getUser')->willReturn($user); - - $this->session = $userSession; } /** @@ -85,6 +88,19 @@ class CORSMiddlewareTest extends \Test\TestCase { $this->createMock(IRequestId::class), $this->createMock(IConfig::class) ); + + /** @var MockObject */ + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('user'); + $this->session->expects($this->exactly(2))->method('getUser')->willReturn($user); + + $this->config + ->method('getSystemValue') + ->willReturnCallback(fn (string $key, mixed $default) => match (true) { + $key === 'cors.allowed-domains' => ['http://www.test.com'], + default => $default, + }); + $this->reflector->reflect($this->controller, $method); $middleware = new CORSMiddleware( $request, @@ -163,6 +179,19 @@ class CORSMiddlewareTest extends \Test\TestCase { $this->createMock(IConfig::class) ); $this->reflector->reflect($this->controller, $method); + + /** @var MockObject */ + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('user'); + $this->session->expects($this->exactly(2))->method('getUser')->willReturn($user); + + $this->config + ->method('getSystemValue') + ->willReturnCallback(fn (string $key, mixed $default) => match (true) { + $key === 'cors.allowed-domains' => ['http://www.test.com'], + default => $default, + }); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->config); $response = new Response();