From bbc5d32c8e14de78ae12c3acea384c69d2264eca Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Tue, 8 Oct 2024 23:51:38 +0200 Subject: [PATCH] fix(dav): Public WebDAV endpoint should allow `GET` requests `GET` should be allowed even without Ajax header to allow downloading files, or show files in the viewer. All other requests could be guarded, but this should not. Signed-off-by: Ferdinand Thiessen --- apps/dav/appinfo/v2/publicremote.php | 16 +++++---- build/integration/config/behat.yml | 2 +- .../dav_features/dav-v2-public.feature | 36 +++++++++++++++++++ .../features/bootstrap/CommandLineContext.php | 9 +++-- .../features/bootstrap/CommentsContext.php | 2 -- .../features/bootstrap/DavFeatureContext.php | 23 ++++++++++++ .../features/bootstrap/Download.php | 14 ++++++++ .../features/bootstrap/FeatureContext.php | 1 - .../integration/features/bootstrap/WebDav.php | 36 +++++++++++++++++++ 9 files changed, 127 insertions(+), 12 deletions(-) create mode 100644 build/integration/features/bootstrap/DavFeatureContext.php diff --git a/apps/dav/appinfo/v2/publicremote.php b/apps/dav/appinfo/v2/publicremote.php index 53e85d556eb..0b7480872cb 100644 --- a/apps/dav/appinfo/v2/publicremote.php +++ b/apps/dav/appinfo/v2/publicremote.php @@ -73,11 +73,15 @@ preg_match('/(^files\/\w+)/i', substr($requestUri, strlen($baseuri)), $match); $baseuri = $baseuri . $match[0]; $server = $serverFactory->createServer($baseuri, $requestUri, $authPlugin, function (\Sabre\DAV\Server $server) use ($authBackend, $linkCheckPlugin, $filesDropPlugin) { - $isAjax = in_array('XMLHttpRequest', explode(',', $_SERVER['HTTP_X_REQUESTED_WITH'] ?? '')); - $federatedShareProvider = \OCP\Server::get(FederatedShareProvider::class); - if ($federatedShareProvider->isOutgoingServer2serverShareEnabled() === false && !$isAjax) { - // this is what is thrown when trying to access a non-existing share - throw new NotAuthenticated(); + // GET must be allowed for e.g. showing images and allowing Zip downloads + if ($server->httpRequest->getMethod() !== 'GET') { + // If this is *not* a GET request we only allow access to public DAV from AJAX or when Server2Server is allowed + $isAjax = in_array('XMLHttpRequest', explode(',', $_SERVER['HTTP_X_REQUESTED_WITH'] ?? '')); + $federatedShareProvider = \OCP\Server::get(FederatedShareProvider::class); + if ($federatedShareProvider->isOutgoingServer2serverShareEnabled() === false && $isAjax === false) { + // this is what is thrown when trying to access a non-existing share + throw new NotAuthenticated(); + } } $share = $authBackend->getShare(); @@ -132,4 +136,4 @@ $server->addPlugin($linkCheckPlugin); $server->addPlugin($filesDropPlugin); // And off we go! -$server->exec(); +$server->start(); diff --git a/build/integration/config/behat.yml b/build/integration/config/behat.yml index 48c2c91aaf3..21e0cdb3309 100644 --- a/build/integration/config/behat.yml +++ b/build/integration/config/behat.yml @@ -61,7 +61,7 @@ default: paths: - "%paths.base%/../dav_features" contexts: - - FeatureContext: + - DavFeatureContext: baseUrl: http://localhost:8080/ocs/ admin: - admin diff --git a/build/integration/dav_features/dav-v2-public.feature b/build/integration/dav_features/dav-v2-public.feature index 1a167ed4ceb..a1ff85dc77b 100644 --- a/build/integration/dav_features/dav-v2-public.feature +++ b/build/integration/dav_features/dav-v2-public.feature @@ -21,6 +21,42 @@ Feature: dav-v2-public When Requesting share note on dav endpoint Then the single response should contain a property "{http://nextcloud.org/ns}note" with value "Hello" + Scenario: Downloading a file from public share with Ajax header + Given using new dav path + And As an "admin" + And user "user0" exists + And user "user1" exists + And As an "user1" + And user "user1" created a folder "/testshare" + When User "user1" uploads file "data/green-square-256.png" to "/testshare/image.png" + And as "user1" creating a share with + | path | testshare | + | shareType | 3 | + | permissions | 1 | + And As an "user0" + Given using new public dav path + When Downloading public file "/image.png" + Then the downloaded file has the content of "/testshare/image.png" from "user1" data + + # Test that downloading files work to ensure e.g. the viewer works or files can be downloaded + Scenario: Downloading a file from public share without Ajax header and disabled s2s share + Given using new dav path + And As an "admin" + And user "user0" exists + And user "user1" exists + And As an "user1" + And user "user1" created a folder "/testshare" + When User "user1" uploads file "data/green-square-256.png" to "/testshare/image.png" + And as "user1" creating a share with + | path | testshare | + | shareType | 3 | + | permissions | 1 | + And As an "user0" + Given parameter "outgoing_server2server_share_enabled" of app "files_sharing" is set to "no" + Given using new public dav path + When Downloading public file "/image.png" without ajax header + Then the downloaded file has the content of "/testshare/image.png" from "user1" data + Scenario: Download a folder Given using new dav path And As an "admin" diff --git a/build/integration/features/bootstrap/CommandLineContext.php b/build/integration/features/bootstrap/CommandLineContext.php index 47a85885ce4..5ea8d12a970 100644 --- a/build/integration/features/bootstrap/CommandLineContext.php +++ b/build/integration/features/bootstrap/CommandLineContext.php @@ -6,6 +6,7 @@ */ require __DIR__ . '/../../vendor/autoload.php'; +use Behat\Behat\Context\Exception\ContextNotFoundException; use Behat\Behat\Hook\Scope\BeforeScenarioScope; use PHPUnit\Framework\Assert; @@ -41,8 +42,12 @@ class CommandLineContext implements \Behat\Behat\Context\Context { /** @BeforeScenario */ public function gatherContexts(BeforeScenarioScope $scope) { $environment = $scope->getEnvironment(); - // this should really be "WebDavContext" ... - $this->featureContext = $environment->getContext('FeatureContext'); + // this should really be "WebDavContext" + try { + $this->featureContext = $environment->getContext('FeatureContext'); + } catch (ContextNotFoundException) { + $this->featureContext = $environment->getContext('DavFeatureContext'); + } } private function findLastTransferFolderForUser($sourceUser, $targetUser) { diff --git a/build/integration/features/bootstrap/CommentsContext.php b/build/integration/features/bootstrap/CommentsContext.php index 4e3c0fb6bda..17795a48fb4 100644 --- a/build/integration/features/bootstrap/CommentsContext.php +++ b/build/integration/features/bootstrap/CommentsContext.php @@ -29,8 +29,6 @@ class CommentsContext implements \Behat\Behat\Context\Context { } } - - /** * get a named entry from response instead of picking a random entry from values * diff --git a/build/integration/features/bootstrap/DavFeatureContext.php b/build/integration/features/bootstrap/DavFeatureContext.php new file mode 100644 index 00000000000..acca52ccafc --- /dev/null +++ b/build/integration/features/bootstrap/DavFeatureContext.php @@ -0,0 +1,23 @@ +deleteServerConfig('files_sharing', 'outgoing_server2server_share_enabled'); + } +} diff --git a/build/integration/features/bootstrap/Download.php b/build/integration/features/bootstrap/Download.php index bef89d2ddb6..2a66f7c3d89 100644 --- a/build/integration/features/bootstrap/Download.php +++ b/build/integration/features/bootstrap/Download.php @@ -137,4 +137,18 @@ trait Download { 'Local header for folder did not appear once in zip file' ); } + + /** + * @Then the downloaded file has the content of :sourceFilename from :user data + */ + public function theDownloadedFileHasContentOfUserFile($sourceFilename, $user) { + $this->getDownloadedFile(); + $expectedFileContents = file_get_contents($this->getDataDirectory() . "/$user/files" . $sourceFilename); + + // prevent the whole file from being printed in case of error. + Assert::assertEquals( + 0, strcmp($expectedFileContents, $this->downloadedFile), + 'Downloaded file content does not match local file content' + ); + } } diff --git a/build/integration/features/bootstrap/FeatureContext.php b/build/integration/features/bootstrap/FeatureContext.php index 893dc3094ba..59f1d0068dd 100644 --- a/build/integration/features/bootstrap/FeatureContext.php +++ b/build/integration/features/bootstrap/FeatureContext.php @@ -9,7 +9,6 @@ use Behat\Behat\Context\SnippetAcceptingContext; require __DIR__ . '/../../vendor/autoload.php'; - /** * Features context. */ diff --git a/build/integration/features/bootstrap/WebDav.php b/build/integration/features/bootstrap/WebDav.php index 4388c7c8eeb..e71502b6b0c 100644 --- a/build/integration/features/bootstrap/WebDav.php +++ b/build/integration/features/bootstrap/WebDav.php @@ -277,6 +277,42 @@ trait WebDav { } } + /** + * @When Downloading public file :filename + */ + public function downloadingPublicFile(string $filename) { + $token = $this->lastShareData->data->token; + $fullUrl = substr($this->baseUrl, 0, -4) . "public.php/dav/files/$token/$filename"; + + $client = new GClient(); + $options = [ + 'headers' => [ + 'X-Requested-With' => 'XMLHttpRequest', + ] + ]; + + try { + $this->response = $client->request('GET', $fullUrl, $options); + } catch (\GuzzleHttp\Exception\ClientException $e) { + $this->response = $e->getResponse(); + } + } + + /** + * @When Downloading public file :filename without ajax header + */ + public function downloadingPublicFileWithoutHeader(string $filename) { + $token = $this->lastShareData->data->token; + $fullUrl = substr($this->baseUrl, 0, -4) . "public.php/dav/files/$token/$filename"; + + $client = new GClient(); + try { + $this->response = $client->request('GET', $fullUrl); + } catch (\GuzzleHttp\Exception\ClientException $e) { + $this->response = $e->getResponse(); + } + } + /** * @Then Downloaded content should start with :start * @param int $start -- 2.39.5