Merge pull request #59852 from nextcloud/backport/59843/stable33

[stable33] fix(dav): unify content disposition header escaping

Author: nickvergessen

apps/dav/lib/CardDAV/ImageExportPlugin.php

@@ -14,6 +14,7 @@
 use Sabre\DAV\ServerPlugin;
 use Sabre\HTTP\RequestInterface;
 use Sabre\HTTP\ResponseInterface;
+use Symfony\Component\HttpFoundation\HeaderUtils;
 
 class ImageExportPlugin extends ServerPlugin {
 
@@ -86,7 +87,11 @@ public function httpGet(RequestInterface $request, ResponseInterface $response)
 			$file = $this->cache->get($addressbook->getResourceId(), $node->getName(), $size, $node);
 			$response->setHeader('Content-Type', $file->getMimeType());
 			$fileName = $node->getName() . '.' . PhotoCache::ALLOWED_CONTENT_TYPES[$file->getMimeType()];
-			$response->setHeader('Content-Disposition', "attachment; filename=$fileName");
+			$sanitized = str_replace(['/', '\\'], '-', $fileName);
+			$fallback = @iconv('UTF-8', 'ASCII//TRANSLIT', $sanitized) ?: $sanitized;
+			$fallback = preg_replace('/[^\x20-\x7e]/', '', $fallback);
+			$fallback = str_replace('%', '', $fallback);
+			$response->setHeader('Content-Disposition', HeaderUtils::makeDisposition(HeaderUtils::DISPOSITION_ATTACHMENT, $sanitized, $fallback));
 			$response->setStatus(Http::STATUS_OK);
 
 			$response->setBody($file->getContent());

apps/dav/lib/Provisioning/Apple/AppleProvisioningPlugin.php

@@ -15,6 +15,7 @@
 use Sabre\DAV\ServerPlugin;
 use Sabre\HTTP\RequestInterface;
 use Sabre\HTTP\ResponseInterface;
+use Symfony\Component\HttpFoundation\HeaderUtils;
 
 class AppleProvisioningPlugin extends ServerPlugin {
 	/**
@@ -127,7 +128,11 @@ public function httpGet(RequestInterface $request, ResponseInterface $response):
 		));
 
 		$response->setStatus(Http::STATUS_OK);
-		$response->setHeader('Content-Disposition', 'attachment; filename="' . $filename . '"');
+		$sanitized = str_replace(['/', '\\'], '-', $filename);
+		$fallback = @iconv('UTF-8', 'ASCII//TRANSLIT', $sanitized) ?: $sanitized;
+		$fallback = preg_replace('/[^\x20-\x7e]/', '', $fallback);
+		$fallback = str_replace('%', '', $fallback);
+		$response->setHeader('Content-Disposition', HeaderUtils::makeDisposition(HeaderUtils::DISPOSITION_ATTACHMENT, $sanitized, $fallback));
 		$response->setHeader('Content-Type', 'application/xml; charset=utf-8');
 		$response->setBody($body);
 

apps/dav/tests/unit/CardDAV/ImageExportPluginTest.php

@@ -171,4 +171,64 @@ public function testCard(?int $size, bool $photo): void {
 		$result = $this->plugin->httpGet($this->request, $this->response);
 		$this->assertFalse($result);
 	}
+
+	public function testCardWithSpecialCharactersInName(): void {
+		$this->request->method('getQueryParameters')
+			->willReturn(['photo' => null]);
+		$this->request->method('getPath')
+			->willReturn('user/book/card');
+
+		$card = $this->createMock(Card::class);
+		$card->method('getETag')
+			->willReturn('"myEtag"');
+		$card->method('getName')
+			->willReturn('contact "with" special;chars');
+		$book = $this->createMock(AddressBook::class);
+		$book->method('getResourceId')
+			->willReturn(1);
+
+		$this->tree->method('getNodeForPath')
+			->willReturnCallback(function ($path) use ($card, $book) {
+				if ($path === 'user/book/card') {
+					return $card;
+				} elseif ($path === 'user/book') {
+					return $book;
+				}
+				$this->fail();
+			});
+
+		$file = $this->createMock(ISimpleFile::class);
+		$file->method('getMimeType')
+			->willReturn('image/png');
+		$file->method('getContent')
+			->willReturn('imgdata');
+
+		$this->cache->method('get')
+			->with(1, 'contact "with" special;chars', -1, $card)
+			->willReturn($file);
+
+		// When special characters are present, they should be properly quoted in the filename parameter
+		$setHeaderCalls = [
+			['Cache-Control', 'private, max-age=3600, must-revalidate'],
+			['Etag', '"myEtag"'],
+			['Content-Type', 'image/png'],
+			['Content-Disposition', 'attachment; filename="contact \"with\" special;chars.png"'],
+		];
+		$this->response->expects($this->exactly(count($setHeaderCalls)))
+			->method('setHeader')
+			->willReturnCallback(function () use (&$setHeaderCalls): void {
+				$expected = array_shift($setHeaderCalls);
+				$this->assertEquals($expected, func_get_args());
+			});
+
+		$this->response->expects($this->once())
+			->method('setStatus')
+			->with(200);
+		$this->response->expects($this->once())
+			->method('setBody')
+			->with('imgdata');
+
+		$result = $this->plugin->httpGet($this->request, $this->response);
+		$this->assertFalse($result);
+	}
 }

apps/dav/tests/unit/Provisioning/Apple/AppleProvisioningPluginTest.php

@@ -148,7 +148,7 @@ public function testHttpGetOnHttps(): void {
 			->with(200);
 
 		$calls = [
-			['Content-Disposition', 'attachment; filename="userName-apple-provisioning.mobileconfig"'],
+			['Content-Disposition', 'attachment; filename=userName-apple-provisioning.mobileconfig'],
 			['Content-Type', 'application/xml; charset=utf-8'],
 		];
 		$this->sabreResponse->expects($this->exactly(2))

lib/public/AppFramework/Http/DownloadResponse.php

@@ -8,6 +8,7 @@
 namespace OCP\AppFramework\Http;
 
 use OCP\AppFramework\Http;
+use Symfony\Component\HttpFoundation\HeaderUtils;
 
 /**
  * Prompts the user to download the a file
@@ -29,9 +30,11 @@ class DownloadResponse extends Response {
 	public function __construct(string $filename, string $contentType, int $status = Http::STATUS_OK, array $headers = []) {
 		parent::__construct($status, $headers);
 
-		$filename = strtr($filename, ['"' => '\\"', '\\' => '\\\\']);
-
-		$this->addHeader('Content-Disposition', 'attachment; filename="' . $filename . '"');
+		$sanitized = str_replace(['/', '\\'], '-', $filename);
+		$fallback = @iconv('UTF-8', 'ASCII//TRANSLIT', $sanitized) ?: $sanitized;
+		$fallback = preg_replace('/[^\x20-\x7e]/', '', $fallback);
+		$fallback = str_replace('%', '', $fallback);
+		$this->addHeader('Content-Disposition', HeaderUtils::makeDisposition(HeaderUtils::DISPOSITION_ATTACHMENT, $sanitized, $fallback));
 		$this->addHeader('Content-Type', $contentType);
 	}
 }

tests/lib/AppFramework/Http/DownloadResponseTest.php

@@ -23,26 +23,37 @@ public function testHeaders(): void {
 		$response = new ChildDownloadResponse('file', 'content');
 		$headers = $response->getHeaders();
 
-		$this->assertEquals('attachment; filename="file"', $headers['Content-Disposition']);
+		$this->assertEquals('attachment; filename=file', $headers['Content-Disposition']);
 		$this->assertEquals('content', $headers['Content-Type']);
 	}
 
 	#[\PHPUnit\Framework\Attributes\DataProvider('filenameEncodingProvider')]
-	public function testFilenameEncoding(string $input, string $expected): void {
+	public function testFilenameEncoding(string $input, string $expectedDisposition): void {
 		$response = new ChildDownloadResponse($input, 'content');
 		$headers = $response->getHeaders();
 
-		$this->assertEquals('attachment; filename="' . $expected . '"', $headers['Content-Disposition']);
+		$this->assertEquals($expectedDisposition, $headers['Content-Disposition']);
 	}
 
-	public static function filenameEncodingProvider() : array {
+	public static 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'],
-			['\\\\\\\\\\\\', '\\\\\\\\\\\\\\\\\\\\\\\\'],
+			['TestName.txt', 'attachment; filename=TestName.txt'],
+			['A "Quoted" Filename.txt', 'attachment; filename="A \"Quoted\" Filename.txt"'],
+			['A "Quoted" Filename.txt', 'attachment; filename="A \"Quoted\" Filename.txt"'],
+			['A "Quoted" Filename With A Backslash \\.txt', 'attachment; filename="A \"Quoted\" Filename With A Backslash -.txt"'],
+			['A "Very" Weird Filename \ / & <> " >\'""""\.text', 'attachment; filename="A \"Very\" Weird Filename - - & <> \" >\'\"\"\"\"-.text"'],
+			['\\\\\\\\\\\\', 'attachment; filename=------'],
 		];
 	}
+
+	public function testSpecialCharactersInFilename(): void {
+		$filename = 'document "draft" with; special&chars.pdf';
+		$response = new ChildDownloadResponse($filename, 'application/pdf');
+		$headers = $response->getHeaders();
+
+		$this->assertEquals(
+			'attachment; filename="document \"draft\" with; special&chars.pdf"',
+			$headers['Content-Disposition']
+		);
+	}
 }