From 4f4173fc26be8d9594f638053c2038477d678c92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Wed, 9 Sep 2026 10:06:23 +0200 Subject: [PATCH] fix: only navigate to a validated absolute http(s) return URL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The return-to-server value is now supplied by the server side through a hidden input instead of being read from the URL. federated() validates it as an absolute http(s) URL with a non-empty host and rejects anything else the same way the other request guards do; index() and public() emit an empty value, since only a federated share ever returns to a remote server. documents.js re-checks the value before using it as a navigation target and falls back to the document list if it does not parse. A path is accepted, installations can live in a subdirectory. Also narrows the WOPI postMessage target origin from '*' to the Collabora Online origin derived from the discovery urlsrc, and ignores incoming messages that do not come from that origin. That origin has to fail closed, and new URL() throws for neither of the two ways it can be missing: an empty urlsrc - which is what discovery returns when it cannot be read - resolves against the base URL and would make this server its own Collabora Online origin, and a urlsrc that is not http(s) has the opaque origin 'null', which is also what a sandboxed frame reports as its own origin. Both now yield no origin at all, so messages are neither accepted nor sent. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- js/documents.js | 86 +++++++++- lib/Controller/DocumentController.php | 46 ++++- templates/documents.php | 1 + .../Controller/DocumentControllerTest.php | 161 ++++++++++++++++++ 4 files changed, 285 insertions(+), 9 deletions(-) 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'); + } }