fix(SetupCheck): Properly check public access to data directory

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>

2 files changed, 23 insertions, 13 deletions

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