Merge pull request #57797 from nextcloud/fix/proper-handling-404

fix(FileDisplayResponse): return 404 if not found
2026-02-18 18:28:50 -05:00 · 2026-02-06 20:07:23 +01:00 · 2026-02-06 20:07:23 +01:00 · c10f35333e
commit c10f35333e
parent b0c02b91b4 762ae4520a
4 changed files with 147 additions and 37 deletions
--- a/lib/public/AppFramework/Http/FileDisplayResponse.php
+++ b/lib/public/AppFramework/Http/FileDisplayResponse.php
@ -19,8 +19,7 @@ use OCP\Files\SimpleFS\ISimpleFile;
 * @template-extends Response<Http::STATUS_*, array<string, mixed>>
 */
 class FileDisplayResponse extends Response implements ICallbackResponse {
-	/** @var File|ISimpleFile */
-	private $file;
+	private File|ISimpleFile $file;

 	/**
 	 * FileDisplayResponse constructor.
@ -48,8 +47,18 @@ class FileDisplayResponse extends Response implements ICallbackResponse {
 	 */
 	public function callback(IOutput $output) {
 		if ($output->getHttpResponseCode() !== Http::STATUS_NOT_MODIFIED) {
+			$file = $this->file instanceof File
+				? $this->file->fopen('rb')
+				: $this->file->read();
+
+			if ($file === false) {
+				$output->setHttpResponseCode(Http::STATUS_NOT_FOUND);
+				$output->setOutput('');
+				return;
+			}
+
 			$output->setHeader('Content-Length: ' . $this->file->getSize());
-			$output->setOutput($this->file->getContent());
+			$output->setReadfile($file);
 		}
 	}
 }
--- a/lib/public/Files/SimpleFS/InMemoryFile.php
+++ b/lib/public/Files/SimpleFS/InMemoryFile.php
@ -105,7 +105,7 @@ class InMemoryFile implements ISimpleFile {
 	}

 	/**
-	 * {@inheritDoc}
+	 * @inheritDoc
 	 * @since 24.0.0
 	 */
 	public function getExtension(): string {
@ -113,15 +113,15 @@ class InMemoryFile implements ISimpleFile {
 	}

 	/**
-	 * Stream reading is unsupported for in memory files.
-	 *
-	 * @throws NotPermittedException
+	 * @inheritDoc
 	 * @since 16.0.0
+	 * @since 34.0.0 - return in-memory stream of contents
 	 */
 	public function read() {
-		throw new NotPermittedException(
-			'Stream reading is unsupported for in memory files'
-		);
+		$stream = fopen('php://memory', 'r+');
+		fwrite($stream, $this->contents);
+		rewind($stream);
+		return $stream;
 	}

 	/**
--- a/tests/lib/AppFramework/Http/FileDisplayResponseTest.php
+++ b/tests/lib/AppFramework/Http/FileDisplayResponseTest.php
@ -1,5 +1,7 @@
 <?php

+declare(strict_types=1);
+
 /**
 * SPDX-FileCopyrightText: 2016 Nextcloud GmbH and Nextcloud contributors
 * SPDX-License-Identifier: AGPL-3.0-or-later
@ -9,19 +11,19 @@ namespace Test\AppFramework\Http;

 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\FileDisplayResponse;
+use OCP\AppFramework\Http\IOutput;
 use OCP\Files\File;
+use OCP\Files\SimpleFS\ISimpleFile;
+use PHPUnit\Framework\MockObject\MockObject;

 class FileDisplayResponseTest extends \Test\TestCase {
-	/** @var File|\PHPUnit\Framework\MockObject\MockObject */
-	private $file;
-
-	/** @var FileDisplayResponse */
-	private $response;
+	private File&MockObject $file;
+	private FileDisplayResponse $response;

 	protected function setUp(): void {
-		$this->file = $this->getMockBuilder('OCP\Files\File')
-			->getMock();
+		parent::setUp();

+		$this->file = $this->createMock(File::class);
 		$this->file->expects($this->once())
 			->method('getETag')
 			->willReturn('myETag');
@ -52,7 +54,7 @@ class FileDisplayResponseTest extends \Test\TestCase {
 	}

 	public function test304(): void {
-		$output = $this->getMockBuilder('OCP\AppFramework\Http\IOutput')
+		$output = $this->getMockBuilder(IOutput::class)
 			->disableOriginalConstructor()
 			->getMock();

@ -69,26 +71,125 @@ class FileDisplayResponseTest extends \Test\TestCase {


 	public function testNon304(): void {
-		$output = $this->getMockBuilder('OCP\AppFramework\Http\IOutput')
+		$resource = fopen('php://memory', 'w+b');
+		fwrite($resource, 'my data');
+		rewind($resource);
+
+		$this->file->expects($this->once())
+			->method('fopen')
+			->willReturn($resource);
+		$this->file->expects($this->any())
+			->method('getSize')
+			->willReturn(7);
+
+		$output = $this->getMockBuilder(IOutput::class)
 			->disableOriginalConstructor()
 			->getMock();
-
-		$output->expects($this->any())
+		$output->expects($this->once())
 			->method('getHttpResponseCode')
 			->willReturn(Http::STATUS_OK);
 		$output->expects($this->once())
-			->method('setOutput')
-			->with($this->equalTo('my data'));
+			->method('setReadFile')
+			->with($this->equalTo($resource));
 		$output->expects($this->once())
 			->method('setHeader')
-			->with($this->equalTo('Content-Length: 42'));
-		$this->file->expects($this->once())
-			->method('getContent')
-			->willReturn('my data');
-		$this->file->expects($this->any())
-			->method('getSize')
-			->willReturn(42);
+			->with($this->equalTo('Content-Length: 7'));

 		$this->response->callback($output);
 	}
+
+	public function testFileNotFound(): void {
+		$this->file->expects($this->once())
+			->method('fopen')
+			->willReturn(false);
+
+		$output = $this->getMockBuilder(IOutput::class)
+			->disableOriginalConstructor()
+			->getMock();
+		$output->expects($this->once())
+			->method('getHttpResponseCode')
+			->willReturn(Http::STATUS_OK);
+		$output->expects($this->once())
+			->method('setHttpResponseCode')
+			->with($this->equalTo(Http::STATUS_NOT_FOUND));
+		$output->expects($this->once())
+			->method('setOutput')
+			->with($this->equalTo(''));
+
+		$this->response->callback($output);
+	}
+
+	public function testSimpleFileNotFound(): void {
+		$file = $this->createMock(ISimpleFile::class);
+		$file->expects($this->once())
+			->method('getETag')
+			->willReturn('myETag');
+		$file->expects($this->once())
+			->method('getName')
+			->willReturn('myFileName');
+		$file->expects($this->once())
+			->method('getMTime')
+			->willReturn(1464825600);
+		$file->expects($this->once())
+			->method('read')
+			->willReturn(false);
+
+		$response = new FileDisplayResponse($file);
+
+		$output = $this->getMockBuilder(IOutput::class)
+			->disableOriginalConstructor()
+			->getMock();
+		$output->expects($this->once())
+			->method('getHttpResponseCode')
+			->willReturn(Http::STATUS_OK);
+		$output->expects($this->once())
+			->method('setHttpResponseCode')
+			->with($this->equalTo(Http::STATUS_NOT_FOUND));
+		$output->expects($this->once())
+			->method('setOutput')
+			->with($this->equalTo(''));
+
+		$response->callback($output);
+	}
+
+	public function testSimpleFile(): void {
+		$file = $this->createMock(ISimpleFile::class);
+		$file->expects($this->once())
+			->method('getETag')
+			->willReturn('myETag');
+		$file->expects($this->once())
+			->method('getName')
+			->willReturn('myFileName');
+		$file->expects($this->once())
+			->method('getMTime')
+			->willReturn(1464825600);
+
+		$resource = fopen('php://memory', 'w+b');
+		fwrite($resource, 'my data');
+		rewind($resource);
+
+		$file->expects($this->once())
+			->method('read')
+			->willReturn($resource);
+		$file->expects($this->any())
+			->method('getSize')
+			->willReturn(7);
+
+		$response = new FileDisplayResponse($file);
+
+		$output = $this->getMockBuilder(IOutput::class)
+			->disableOriginalConstructor()
+			->getMock();
+		$output->expects($this->once())
+			->method('getHttpResponseCode')
+			->willReturn(Http::STATUS_OK);
+		$output->expects($this->once())
+			->method('setReadFile')
+			->with($this->equalTo($resource));
+		$output->expects($this->once())
+			->method('setHeader')
+			->with($this->equalTo('Content-Length: 7'));
+
+		$response->callback($output);
+	}
 }
--- a/tests/lib/Files/SimpleFS/InMemoryFileTest.php
+++ b/tests/lib/Files/SimpleFS/InMemoryFileTest.php
@ -4,7 +4,7 @@ declare(strict_types=1);

 /**
 * SPDX-FileCopyrightText: 2018 Nextcloud GmbH and Nextcloud contributors
- * SPDX-License-Identifier: AGPL-3.0-only
+ * SPDX-License-Identifier: AGPL-3.0-or-later
 */

 namespace Test\File\SimpleFS;
@ -14,7 +14,7 @@ use OCP\Files\SimpleFS\InMemoryFile;
 use Test\TestCase;

 /**
- * This class provide test casesf or the InMemoryFile.
+ * This class provide test cases for the InMemoryFile.
 *
 * @package Test\File\SimpleFS
 */
@ -106,13 +106,13 @@ class InMemoryFileTest extends TestCase {


 	/**
-	 * Asserts that read() raises an NotPermittedException.
-	 *
-	 * @return void
+	 * Ensure that read() returns a stream with the same contents than the original file.
 	 */
 	public function testRead(): void {
-		self::expectException(NotPermittedException::class);
-		$this->testPdf->read();
+		self::assertEquals(
+			file_get_contents(__DIR__ . '/../../../data/test.pdf'),
+			stream_get_contents($this->testPdf->read()),
+		);
 	}

 	/**