From 0563757ea43b853770305f80c763a547525abf66 Mon Sep 17 00:00:00 2001
From: Ferdinand Thiessen <opensource@fthiessen.de>
Date: Thu, 11 Jul 2024 20:53:37 +0200
Subject: fix(SetupCheck): Properly check public access to data directory
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When checking for public (web) access to the data directory the status is not enough
as you might have a webserver that forwards to e.g. a login page.
So instead check that the content of the file matches.

For this the `.ncdata` file (renamed from `.ocdata`¹) has minimal text content
to allow checking.

¹The file was renamed from the legacy `.ocdata`, there is a repair step to remove the old one.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
---
 .../lib/SetupChecks/DataDirectoryProtected.php       | 16 ++++++++++++----
 .../tests/SetupChecks/DataDirectoryProtectedTest.php | 20 +++++++++++---------
 2 files changed, 23 insertions(+), 13 deletions(-)

(limited to 'apps/settings')

diff --git a/apps/settings/lib/SetupChecks/DataDirectoryProtected.php b/apps/settings/lib/SetupChecks/DataDirectoryProtected.php
index 5afdfaaddd5..447c66d5c5d 100644
--- a/apps/settings/lib/SetupChecks/DataDirectoryProtected.php
+++ b/apps/settings/lib/SetupChecks/DataDirectoryProtected.php
@@ -42,13 +42,21 @@ class DataDirectoryProtected implements ISetupCheck {
 	public function run(): SetupResult {
 		$datadir = str_replace(\OC::$SERVERROOT . '/', '', $this->config->getSystemValue('datadirectory', ''));
 
-		$dataUrl = $this->urlGenerator->getWebroot() . '/' . $datadir . '/.ocdata';
+		$dataUrl = $this->urlGenerator->getWebroot() . '/' . $datadir . '/.ncdata';
 
 		$noResponse = true;
-		foreach ($this->runHEAD($dataUrl, httpErrors:false) as $response) {
+		foreach ($this->runRequest('GET', $dataUrl, [ 'httpErrors' => false ]) as $response) {
 			$noResponse = false;
-			if ($response->getStatusCode() === 200) {
-				return SetupResult::error($this->l10n->t('Your data directory and files are probably accessible from the internet. The .htaccess file is not working. It is strongly recommended that you configure your web server so that the data directory is no longer accessible, or move the data directory outside the web server document root.'));
+			if ($response->getStatusCode() < 400) {
+				// Read the response body
+				$body = $response->getBody();
+				if (is_resource($body)) {
+					$body = stream_get_contents($body, 64);
+				}
+
+				if (str_contains($body, '# Nextcloud data directory')) {
+					return SetupResult::error($this->l10n->t('Your data directory and files are probably accessible from the internet. The .htaccess file is not working. It is strongly recommended that you configure your web server so that the data directory is no longer accessible, or move the data directory outside the web server document root.'));
+				}
 			} else {
 				$this->logger->debug('[expected] Could not access data directory from outside.', ['url' => $dataUrl]);
 			}
diff --git a/apps/settings/tests/SetupChecks/DataDirectoryProtectedTest.php b/apps/settings/tests/SetupChecks/DataDirectoryProtectedTest.php
index 9925ce12c44..fbee52fcf0f 100644
--- a/apps/settings/tests/SetupChecks/DataDirectoryProtectedTest.php
+++ b/apps/settings/tests/SetupChecks/DataDirectoryProtectedTest.php
@@ -45,7 +45,7 @@ class DataDirectoryProtectedTest extends TestCase {
 		$this->logger = $this->createMock(LoggerInterface::class);
 
 		$this->setupcheck = $this->getMockBuilder(DataDirectoryProtected::class)
-			->onlyMethods(['runHEAD'])
+			->onlyMethods(['runRequest'])
 			->setConstructorArgs([
 				$this->l10n,
 				$this->config,
@@ -59,16 +59,17 @@ class DataDirectoryProtectedTest extends TestCase {
 	/**
 	 * @dataProvider dataTestStatusCode
 	 */
-	public function testStatusCode(array $status, string $expected): void {
-		$responses = array_map(function ($state) {
+	public function testStatusCode(array $status, string $expected, bool $hasBody): void {
+		$responses = array_map(function ($state) use ($hasBody) {
 			$response = $this->createMock(IResponse::class);
 			$response->expects($this->any())->method('getStatusCode')->willReturn($state);
+			$response->expects(($this->atMost(1)))->method('getBody')->willReturn($hasBody ? '# Nextcloud data directory' : 'something else');
 			return $response;
 		}, $status);
 
 		$this->setupcheck
 			->expects($this->once())
-			->method('runHEAD')
+			->method('runRequest')
 			->will($this->generate($responses));
 
 		$this->config
@@ -82,10 +83,11 @@ class DataDirectoryProtectedTest extends TestCase {
 
 	public function dataTestStatusCode(): array {
 		return [
-			'success: forbidden access' => [[403], SetupResult::SUCCESS],
-			'error: can access' => [[200], SetupResult::ERROR],
-			'error: one forbidden one can access' => [[403, 200], SetupResult::ERROR],
-			'warning: connection issue' => [[], SetupResult::WARNING],
+			'success: forbidden access' => [[403], SetupResult::SUCCESS, true],
+			'success: forbidden access with redirect' => [[200], SetupResult::SUCCESS, false],
+			'error: can access' => [[200], SetupResult::ERROR, true],
+			'error: one forbidden one can access' => [[403, 200], SetupResult::ERROR, true],
+			'warning: connection issue' => [[], SetupResult::WARNING, true],
 		];
 	}
 
@@ -95,7 +97,7 @@ class DataDirectoryProtectedTest extends TestCase {
 
 		$this->setupcheck
 			->expects($this->once())
-			->method('runHEAD')
+			->method('runRequest')
 			->will($this->generate([]));
 
 		$this->config
-- 
cgit v1.2.3