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');
+ }
}