diff --git a/js/documents.js b/js/documents.js index 36ff1244b..3a522e292 100644 --- a/js/documents.js +++ b/js/documents.js @@ -227,6 +227,58 @@ var documentsMain = { return window.location.protocol + '//' + window.location.host + ocurl; }, + // returns the given value if it is an absolute http(s) url, null otherwise. + // a path is fine, installations can live in a subdirectory + _absoluteHttpUrl: function(value) { + if (!value) { + return null; + } + + var url; + try { + // no base url on purpose, only absolute urls are accepted + url = new URL(value); + } catch (exc) { + return null; + } + + if ((url.protocol !== 'http:' && url.protocol !== 'https:') || !url.host) { + return null; + } + + return url.href; + }, + + // origin of the Collabora Online server, taken from the discovery urlsrc. + // incoming post messages are only accepted from, and outgoing ones only + // sent to, that origin + _wopiOrigin: function() { + // without a urlsrc there is no known origin. new URL() would not throw + // here, an empty value resolves against the base url and would make this + // server its own Collabora Online origin + if (!documentsMain.urlsrc) { + return null; + } + + var resolved; + try { + // urlsrc may be configured relative to this server, hence the base url + resolved = new URL(documentsMain.urlsrc, window.location.href); + } catch (exc) { + console.warn('Cannot determine the Collabora Online origin from ' + documentsMain.urlsrc); + return null; + } + + // a urlsrc that is not http(s) has the opaque origin 'null', which is + // what a sandboxed frame reports as well, so it must never be returned + if (!documentsMain._absoluteHttpUrl(resolved.href)) { + console.warn('Cannot determine the Collabora Online origin from ' + documentsMain.urlsrc); + return null; + } + + return resolved.origin; + }, + UI : { /* Editor wrapper HTML */ container : '
' + @@ -452,6 +504,10 @@ var documentsMain = { // Listen for App_LoadingStatus as soon as possible $('#loleafletframe').ready(function() { var editorInitListener = function(e) { + if (e.origin !== documentsMain._wopiOrigin()) { + return; + } + var msg = JSON.parse(e.data); if (msg.MessageId === 'App_LoadingStatus') { documentsMain.wopiClientFeatures = msg.Values.Features; @@ -464,6 +520,10 @@ var documentsMain = { $('#loleafletframe').load(function(){ // And start listening to incoming post messages window.addEventListener('message', function(e){ + if (e.origin !== documentsMain._wopiOrigin()) { + return; + } + if (documentsMain.isViewerMode) { return; } @@ -613,9 +673,12 @@ var documentsMain = { var shareToken = getURLParameter('shareToken'); if (shareToken != 'null') { - // check if local share or federated share - var server = getURLParameter('server'); - if (server != 'null') { + // check if local share or federated share. + // the server is only ever supplied by the server side, and only for + // federated shares - never read it from the URL, it ends up in + // window.location in onClose() + var server = $('#return-to-server').val(); + if (server) { documentsMain.returnToServer = server; } else { documentsMain.returnToShare = shareToken; @@ -633,13 +696,18 @@ var documentsMain = { WOPIPostMessage: function(iframe, msgId, values) { if (iframe) { + var targetOrigin = documentsMain._wopiOrigin(); + if (!targetOrigin) { + return; + } + var msg = { 'MessageId': msgId, 'SendTime': Date.now(), 'Values': values }; - iframe.contentWindow.postMessage(JSON.stringify(msg), '*'); + iframe.contentWindow.postMessage(JSON.stringify(msg), targetOrigin); } }, @@ -781,12 +849,18 @@ var documentsMain = { documentsMain.UI.hideEditor(); $('#ocToolbar').remove(); + // refuse to navigate to anything but an absolute http(s) url + var returnToServer = documentsMain._absoluteHttpUrl(documentsMain.returnToServer); + if (documentsMain.returnToServer && !returnToServer) { + console.warn('Not returning to ' + documentsMain.returnToServer + ', not an absolute http(s) url'); + } + if (documentsMain.returnToDir) { documentsMain.overlay.documentOverlay('show'); window.location = OC.generateUrl('apps/files?dir={dir}', {dir: documentsMain.returnToDir}, {escape: false}); - } else if (documentsMain.returnToServer) { + } else if (returnToServer) { documentsMain.overlay.documentOverlay('show'); - window.location = documentsMain.returnToServer; + window.location = returnToServer; } else if (documentsMain.returnToShare) { documentsMain.overlay.documentOverlay('show'); window.location = OC.generateUrl('s/{shareToken}', {shareToken: documentsMain.returnToShare}, {escape: false}); diff --git a/lib/Controller/DocumentController.php b/lib/Controller/DocumentController.php index c70a47edb..0e698b08a 100644 --- a/lib/Controller/DocumentController.php +++ b/lib/Controller/DocumentController.php @@ -165,6 +165,30 @@ private function domainOnly($url) { return "$scheme$host$port"; } + /** + * Checks that the given value is an absolute http(s) URL with a non-empty host. + * + * The value is handed to the browser as a navigation target once the editor is + * closed, so everything that is not an absolute http(s) URL has to be rejected - + * most importantly the javascript: and data: schemes, and scheme relative URLs. + * + * @param mixed $url + * @return bool + */ + private function isValidServerUrl($url) { + if (!\is_string($url) || $url === '') { + return false; + } + $parsed_url = \parse_url($url); + if (!\is_array($parsed_url) || !isset($parsed_url['scheme'], $parsed_url['host'])) { + return false; + } + if (!\in_array(\strtolower($parsed_url['scheme']), ['http', 'https'], true)) { + return false; + } + return $parsed_url['host'] !== ''; + } + /** * Get collabora document for: * - the base template if fileId is null @@ -261,7 +285,8 @@ public function index($fileId, $dir) { 'doc_format' => $this->appConfig->getAppValue('doc_format'), 'instanceId' => $this->settings->getSystemValue('instanceid'), 'canonical_webroot' => $this->appConfig->getAppValue('canonical_webroot'), - 'show_custom_header' => false + 'show_custom_header' => false, + 'return_to_server' => '' // only federated shares return to a remote server ], $docRetVal ); @@ -358,7 +383,8 @@ public function public($shareToken, $fileId) { 'doc_format' => $this->appConfig->getAppValue('doc_format'), 'instanceId' => $this->settings->getSystemValue('instanceid'), 'canonical_webroot' => $this->appConfig->getAppValue('canonical_webroot'), - 'show_custom_header' => true // public link should show a customer header without buttons + 'show_custom_header' => true, // public link should show a customer header without buttons + 'return_to_server' => '' // only federated shares return to a remote server ]; $response = new TemplateResponse('richdocuments', 'documents', $retVal, $renderAs); @@ -384,6 +410,19 @@ public function federated($shareToken, $shareRelativePath, $server, $accessToken return $this->responseError($this->l10n->t('Invalid request parameters')); } + // the server is where the editor navigates back to once it is closed, + // see FederationService::getRemoteFileUrl(). this only checks the shape - + // that the value is an absolute http(s) url and therefore safe to hand to + // the browser. whether the host is trusted is decided further down by + // FederationService::isServerAllowed() via getWopiForToken(), which fails + // closed on an empty richdocuments.federation_allowlist. do not drop that + // call or move it behind the template response, on its own the check here + // accepts any host + if (!$this->isValidServerUrl($server)) { + $this->logger->warning("Rejecting federated request with invalid server {server}", ["server" => $server]); + return $this->responseError($this->l10n->t('Invalid request parameters')); + } + $docinfo = $this->documentService->getDocumentByFederatedToken($shareToken, $shareRelativePath); if (!$docinfo) { $this->logger->warning("Cannot retrieve document from share {token} that has path {path}", ["token" => $shareToken, "path" => $shareRelativePath]); @@ -445,7 +484,8 @@ public function federated($shareToken, $shareRelativePath, $server, $accessToken 'doc_format' => $this->appConfig->getAppValue('doc_format'), 'instanceId' => $this->settings->getSystemValue('instanceid'), 'canonical_webroot' => $this->appConfig->getAppValue('canonical_webroot'), - 'show_custom_header' => true // federated share should show a customer header without buttons + 'show_custom_header' => true, // federated share should show a customer header without buttons + 'return_to_server' => $server ]; // Federated share is a user coming from remote instance so cannot show base template diff --git a/templates/documents.php b/templates/documents.php index 7f4386801..4630083c8 100644 --- a/templates/documents.php +++ b/templates/documents.php @@ -65,6 +65,7 @@
+ diff --git a/tests/unit/Controller/DocumentControllerTest.php b/tests/unit/Controller/DocumentControllerTest.php index 5b4998cf2..4b672e784 100644 --- a/tests/unit/Controller/DocumentControllerTest.php +++ b/tests/unit/Controller/DocumentControllerTest.php @@ -15,6 +15,7 @@ use OCA\Richdocuments\DiscoveryService; use OCA\Richdocuments\FederationService; use OCP\App\IAppManager; +use OCP\AppFramework\Http\TemplateResponse; use OCP\IGroupManager; use OCP\INavigationManager; use OCP\IPreview; @@ -173,4 +174,164 @@ public function invalidFilenameProvider(): array { ["filename with / slash"] ]; } + + /** + * The server parameter ends up as a navigation target in the browser, so + * federated() has to reject everything that is not an absolute http(s) URL. + * + * @dataProvider invalidServerProvider + * @param $server mixed + */ + public function testFederatedRejectsInvalidServer($server) { + // the request must not be processed any further + $this->documentService + ->expects($this->never()) + ->method('getDocumentByFederatedToken'); + + $response = $this->documentController->federated('sharetoken', '/document.odt', $server, 'accesstoken'); + + $this->assertInstanceOf(TemplateResponse::class, $response); + $this->assertEquals('error', $response->getTemplateName()); + } + + public function invalidServerProvider(): array { + return [ + 'javascript scheme' => ['javascript:alert(document.domain)'], + 'javascript scheme uppercase' => ['JaVaScRiPt:alert(document.domain)'], + 'data scheme' => ['data:text/html;base64,PHNjcmlwdD5hbGVydCgxKTwvc2NyaXB0Pg=='], + 'scheme relative' => ['//evil.tld'], + 'scheme relative with path' => ['//evil.tld/owncloud'], + 'relative path' => ['/index.php/apps/files'], + 'no scheme' => ['remote.example.com'], + 'scheme without host' => ['https://'], + 'empty' => [''], + 'null' => [null], + 'not a string' => [42], + ]; + } + + /** + * A well formed remote server must pass the validation - in particular one + * with a path, ownCloud can be installed in a subdirectory. + * + * @dataProvider validServerProvider + * @param $server string + */ + public function testFederatedAcceptsValidServer(string $server) { + // reaching the document lookup means the server was accepted + $this->documentService + ->expects($this->once()) + ->method('getDocumentByFederatedToken') + ->with('sharetoken', '/document.odt') + ->willReturn(null); + + $response = $this->documentController->federated('sharetoken', '/document.odt', $server, 'accesstoken'); + + // the document cannot be resolved, so this is still an error response + $this->assertInstanceOf(TemplateResponse::class, $response); + $this->assertEquals('error', $response->getTemplateName()); + } + + public function validServerProvider(): array { + return [ + 'https' => ['https://remote.example.com'], + 'http' => ['http://remote.example.com'], + 'trailing slash' => ['https://remote.example.com/'], + 'subdirectory install' => ['https://remote.example.com/owncloud'], + 'with port' => ['https://remote.example.com:8443/owncloud'], + 'uppercase scheme' => ['HTTPS://remote.example.com'], + ]; + } + + /** + * The validated server has to reach the template, that is where the JS picks + * it up now instead of reading it from the URL. + * + * @group DB + */ + public function testFederatedPassesServerToTemplate() { + $server = 'https://remote.example.com/owncloud'; + + $this->documentService + ->method('getDocumentByFederatedToken') + ->willReturn($this->documentInfo()); + $this->federationService + ->method('getWopiForToken') + ->with($server, 'accesstoken') + ->willReturn(['editor' => 'alice@remote.example.com', 'attributes' => 1]); + $this->settings->method('getUserValue')->willReturn('en'); + $this->mockDiscovery(); + + $response = $this->documentController->federated('sharetoken', '/document.odt', $server, 'accesstoken'); + + $this->assertInstanceOf(TemplateResponse::class, $response); + $this->assertEquals('documents', $response->getTemplateName()); + $this->assertEquals($server, $response->getParams()['return_to_server']); + } + + /** + * A public link is never opened from a remote server, so it must not carry a + * return_to_server value that the JS would navigate to. + * + * @group DB + */ + public function testPublicEmitsNoReturnToServer() { + $this->documentService + ->method('getDocumentByShareToken') + ->willReturn($this->documentInfo()); + $this->settings->method('getUserValue')->willReturn('en'); + $this->mockDiscovery(); + + $response = $this->documentController->public('sharetoken', null); + + $this->assertInstanceOf(TemplateResponse::class, $response); + $this->assertEquals('documents', $response->getTemplateName()); + $params = $response->getParams(); + $this->assertArrayHasKey('return_to_server', $params); + $this->assertSame('', $params['return_to_server']); + } + + /** + * public() must not accept a server at all, so that no request parameter can + * ever influence where the editor returns to. + */ + public function testPublicHasNoServerParameter() { + $parameters = (new \ReflectionMethod(DocumentController::class, 'public'))->getParameters(); + + $names = \array_map(static function (\ReflectionParameter $parameter) { + return $parameter->getName(); + }, $parameters); + + $this->assertEquals(['shareToken', 'fileId'], $names); + } + + /** + * Minimal document index as returned by the DocumentService. + */ + private function documentInfo(): array { + return [ + 'name' => 'document.odt', + 'fileid' => 1234, + 'path' => '/document.odt', + 'owner' => 'alice', + 'version' => 0, + 'mimetype' => 'application/vnd.oasis.opendocument.text', + 'allowEdit' => false, + ]; + } + + /** + * Let the discovery return a usable Collabora Online endpoint. + */ + private function mockDiscovery(): void { + $this->discoveryService + ->method('getWopiSrc') + ->willReturn([ + 'action' => 'view', + 'urlsrc' => 'https://collabora.example.com/browser/abc/cool.html?', + ]); + $this->discoveryService + ->method('getWopiUrl') + ->willReturn('https://collabora.example.com:9980'); + } }