Merge pull request #27354 from nextcloud/escape-download-response

Escape filename in Content-Disposition

2 files changed, 27 insertions, 17 deletions

diff --git a/lib/public/AppFramework/Http/DownloadResponse.php b/lib/public/AppFramework/Http/DownloadResponse.php
index 78381f0f08f..a7516fc6b85 100644
--- a/lib/public/AppFramework/Http/DownloadResponse.php
+++ b/lib/public/AppFramework/Http/DownloadResponse.php
@@ -30,20 +30,16 @@ namespace OCP\AppFramework\Http;
  * @since 7.0.0
  */
 class DownloadResponse extends Response {
-	private $filename;
-	private $contentType;
-
 	/**
 	 * Creates a response that prompts the user to download the file
 	 * @param string $filename the name that the downloaded file should have
 	 * @param string $contentType the mimetype that the downloaded file should have
 	 * @since 7.0.0
 	 */
-	public function __construct($filename, $contentType) {
+	public function __construct(string $filename, string $contentType) {
 		parent::__construct();
 
-		$this->filename = $filename;
-		$this->contentType = $contentType;
+		$filename = strtr($filename, ['"' => '\\"', '\\' => '\\\\']);
 
 		$this->addHeader('Content-Disposition', 'attachment; filename="' . $filename . '"');
 		$this->addHeader('Content-Type', $contentType);
diff --git a/tests/lib/AppFramework/Http/DownloadResponseTest.php b/tests/lib/AppFramework/Http/DownloadResponseTest.php
index 6c509b8bc59..89de248cea0 100644
--- a/tests/lib/AppFramework/Http/DownloadResponseTest.php
+++ b/tests/lib/AppFramework/Http/DownloadResponseTest.php
@@ -30,22 +30,36 @@ class ChildDownloadResponse extends DownloadResponse {
 
 
 class DownloadResponseTest extends \Test\TestCase {
-
-	/**
-	 * @var ChildDownloadResponse
-	 */
-	protected $response;
-
 	protected function setUp(): void {
 		parent::setUp();
-		$this->response = new ChildDownloadResponse('file', 'content');
 	}
 
-
 	public function testHeaders() {
-		$headers = $this->response->getHeaders();
+		$response = new ChildDownloadResponse('file', 'content');
+		$headers = $response->getHeaders();
+
+		$this->assertEquals('attachment; filename="file"', $headers['Content-Disposition']);
+		$this->assertEquals('content', $headers['Content-Type']);
+	}
+
+	/**
+	 * @dataProvider filenameEncodingProvider
+	 */
+	public function testFilenameEncoding(string $input, string $expected) {
+		$response = new ChildDownloadResponse($input, 'content');
+		$headers = $response->getHeaders();
+
+		$this->assertEquals('attachment; filename="'.$expected.'"', $headers['Content-Disposition']);
+	}
 
-		$this->assertStringContainsString('attachment; filename="file"', $headers['Content-Disposition']);
-		$this->assertStringContainsString('content', $headers['Content-Type']);
+	public function filenameEncodingProvider() : array {
+		return [
+			['TestName.txt', 'TestName.txt'],
+			['A "Quoted" Filename.txt', 'A \\"Quoted\\" Filename.txt'],
+			['A "Quoted" Filename.txt', 'A \\"Quoted\\" Filename.txt'],
+			['A "Quoted" Filename With A Backslash \\.txt', 'A \\"Quoted\\" Filename With A Backslash \\\\.txt'],
+			['A "Very" Weird Filename \ / & <> " >\'""""\.text', 'A \\"Very\\" Weird Filename \\\\ / & <> \\" >\'\\"\\"\\"\\"\\\\.text'],
+			['\\\\\\\\\\\\', '\\\\\\\\\\\\\\\\\\\\\\\\'],
+		];
 	}
 }