Skip to content

Commit 65db5e6

Browse files
ChristophWurstChristophWurst
committed
fix(dav): unify content disposition header escaping
Signed-off-by: Christoph Wurst <1374172+ChristophWurst@users.noreply.github.com>
1 parent e1049a8 commit 65db5e6

6 files changed

Lines changed: 100 additions & 16 deletions

File tree

‎apps/dav/lib/CardDAV/ImageExportPlugin.php‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
use Sabre\DAV\ServerPlugin;
1515
use Sabre\HTTP\RequestInterface;
1616
use Sabre\HTTP\ResponseInterface;
17+
use Symfony\Component\HttpFoundation\HeaderUtils;
1718

1819
class ImageExportPlugin extends ServerPlugin {
1920

@@ -86,7 +87,11 @@ public function httpGet(RequestInterface $request, ResponseInterface $response)
8687
$file = $this->cache->get($addressbook->getResourceId(), $node->getName(), $size, $node);
8788
$response->setHeader('Content-Type', $file->getMimeType());
8889
$fileName = $node->getName() . '.' . PhotoCache::ALLOWED_CONTENT_TYPES[$file->getMimeType()];
89-
$response->setHeader('Content-Disposition', "attachment; filename=$fileName");
90+
$sanitized = str_replace(['/', '\\'], '-', $fileName);
91+
$fallback = @iconv('UTF-8', 'ASCII//TRANSLIT', $sanitized) ?: $sanitized;
92+
$fallback = preg_replace('/[^\x20-\x7e]/', '', $fallback);
93+
$fallback = str_replace('%', '', $fallback);
94+
$response->setHeader('Content-Disposition', HeaderUtils::makeDisposition(HeaderUtils::DISPOSITION_ATTACHMENT, $sanitized, $fallback));
9095
$response->setStatus(Http::STATUS_OK);
9196

9297
$response->setBody($file->getContent());

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
use Sabre\DAV\ServerPlugin;
1818
use Sabre\HTTP\RequestInterface;
1919
use Sabre\HTTP\ResponseInterface;
20+
use Symfony\Component\HttpFoundation\HeaderUtils;
2021

2122
class AppleProvisioningPlugin extends ServerPlugin {
2223
/**
@@ -123,7 +124,11 @@ public function httpGet(RequestInterface $request, ResponseInterface $response):
123124
));
124125

125126
$response->setStatus(Http::STATUS_OK);
126-
$response->setHeader('Content-Disposition', 'attachment; filename="' . $filename . '"');
127+
$sanitized = str_replace(['/', '\\'], '-', $filename);
128+
$fallback = @iconv('UTF-8', 'ASCII//TRANSLIT', $sanitized) ?: $sanitized;
129+
$fallback = preg_replace('/[^\x20-\x7e]/', '', $fallback);
130+
$fallback = str_replace('%', '', $fallback);
131+
$response->setHeader('Content-Disposition', HeaderUtils::makeDisposition(HeaderUtils::DISPOSITION_ATTACHMENT, $sanitized, $fallback));
127132
$response->setHeader('Content-Type', 'application/xml; charset=utf-8');
128133
$response->setBody($body);
129134

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

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,4 +171,64 @@ public function testCard(?int $size, bool $photo): void {
171171
$result = $this->plugin->httpGet($this->request, $this->response);
172172
$this->assertFalse($result);
173173
}
174+
175+
public function testCardWithSpecialCharactersInName(): void {
176+
$this->request->method('getQueryParameters')
177+
->willReturn(['photo' => null]);
178+
$this->request->method('getPath')
179+
->willReturn('user/book/card');
180+
181+
$card = $this->createMock(Card::class);
182+
$card->method('getETag')
183+
->willReturn('"myEtag"');
184+
$card->method('getName')
185+
->willReturn('contact "with" special;chars');
186+
$book = $this->createMock(AddressBook::class);
187+
$book->method('getResourceId')
188+
->willReturn(1);
189+
190+
$this->tree->method('getNodeForPath')
191+
->willReturnCallback(function ($path) use ($card, $book) {
192+
if ($path === 'user/book/card') {
193+
return $card;
194+
} elseif ($path === 'user/book') {
195+
return $book;
196+
}
197+
$this->fail();
198+
});
199+
200+
$file = $this->createMock(ISimpleFile::class);
201+
$file->method('getMimeType')
202+
->willReturn('image/png');
203+
$file->method('getContent')
204+
->willReturn('imgdata');
205+
206+
$this->cache->method('get')
207+
->with(1, 'contact "with" special;chars', -1, $card)
208+
->willReturn($file);
209+
210+
// When special characters are present, they should be properly quoted in the filename parameter
211+
$setHeaderCalls = [
212+
['Cache-Control', 'private, max-age=3600, must-revalidate'],
213+
['Etag', '"myEtag"'],
214+
['Content-Type', 'image/png'],
215+
['Content-Disposition', 'attachment; filename="contact \"with\" special;chars.png"'],
216+
];
217+
$this->response->expects($this->exactly(count($setHeaderCalls)))
218+
->method('setHeader')
219+
->willReturnCallback(function () use (&$setHeaderCalls): void {
220+
$expected = array_shift($setHeaderCalls);
221+
$this->assertEquals($expected, func_get_args());
222+
});
223+
224+
$this->response->expects($this->once())
225+
->method('setStatus')
226+
->with(200);
227+
$this->response->expects($this->once())
228+
->method('setBody')
229+
->with('imgdata');
230+
231+
$result = $this->plugin->httpGet($this->request, $this->response);
232+
$this->assertFalse($result);
233+
}
174234
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ public function testHttpGetOnHttps(): void {
148148
->with(200);
149149

150150
$calls = [
151-
['Content-Disposition', 'attachment; filename="userName-apple-provisioning.mobileconfig"'],
151+
['Content-Disposition', 'attachment; filename=userName-apple-provisioning.mobileconfig'],
152152
['Content-Type', 'application/xml; charset=utf-8'],
153153
];
154154
$this->sabreResponse->expects($this->exactly(2))

‎lib/public/AppFramework/Http/DownloadResponse.php‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
namespace OCP\AppFramework\Http;
99

1010
use OCP\AppFramework\Http;
11+
use Symfony\Component\HttpFoundation\HeaderUtils;
1112

1213
/**
1314
* Prompts the user to download the a file
@@ -29,9 +30,11 @@ class DownloadResponse extends Response {
2930
public function __construct(string $filename, string $contentType, int $status = Http::STATUS_OK, array $headers = []) {
3031
parent::__construct($status, $headers);
3132

32-
$filename = strtr($filename, ['"' => '\\"', '\\' => '\\\\']);
33-
34-
$this->addHeader('Content-Disposition', 'attachment; filename="' . $filename . '"');
33+
$sanitized = str_replace(['/', '\\'], '-', $filename);
34+
$fallback = @iconv('UTF-8', 'ASCII//TRANSLIT', $sanitized) ?: $sanitized;
35+
$fallback = preg_replace('/[^\x20-\x7e]/', '', $fallback);
36+
$fallback = str_replace('%', '', $fallback);
37+
$this->addHeader('Content-Disposition', HeaderUtils::makeDisposition(HeaderUtils::DISPOSITION_ATTACHMENT, $sanitized, $fallback));
3538
$this->addHeader('Content-Type', $contentType);
3639
}
3740
}

‎tests/lib/AppFramework/Http/DownloadResponseTest.php‎

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -23,26 +23,37 @@ public function testHeaders(): void {
2323
$response = new ChildDownloadResponse('file', 'content');
2424
$headers = $response->getHeaders();
2525

26-
$this->assertEquals('attachment; filename="file"', $headers['Content-Disposition']);
26+
$this->assertEquals('attachment; filename=file', $headers['Content-Disposition']);
2727
$this->assertEquals('content', $headers['Content-Type']);
2828
}
2929

3030
#[\PHPUnit\Framework\Attributes\DataProvider('filenameEncodingProvider')]
31-
public function testFilenameEncoding(string $input, string $expected): void {
31+
public function testFilenameEncoding(string $input, string $expectedDisposition): void {
3232
$response = new ChildDownloadResponse($input, 'content');
3333
$headers = $response->getHeaders();
3434

35-
$this->assertEquals('attachment; filename="' . $expected . '"', $headers['Content-Disposition']);
35+
$this->assertEquals($expectedDisposition, $headers['Content-Disposition']);
3636
}
3737

38-
public static function filenameEncodingProvider() : array {
38+
public static function filenameEncodingProvider(): array {
3939
return [
40-
['TestName.txt', 'TestName.txt'],
41-
['A "Quoted" Filename.txt', 'A \\"Quoted\\" Filename.txt'],
42-
['A "Quoted" Filename.txt', 'A \\"Quoted\\" Filename.txt'],
43-
['A "Quoted" Filename With A Backslash \\.txt', 'A \\"Quoted\\" Filename With A Backslash \\\\.txt'],
44-
['A "Very" Weird Filename \ / & <> " >\'""""\.text', 'A \\"Very\\" Weird Filename \\\\ / & <> \\" >\'\\"\\"\\"\\"\\\\.text'],
45-
['\\\\\\\\\\\\', '\\\\\\\\\\\\\\\\\\\\\\\\'],
40+
['TestName.txt', 'attachment; filename=TestName.txt'],
41+
['A "Quoted" Filename.txt', 'attachment; filename="A \"Quoted\" Filename.txt"'],
42+
['A "Quoted" Filename.txt', 'attachment; filename="A \"Quoted\" Filename.txt"'],
43+
['A "Quoted" Filename With A Backslash \\.txt', 'attachment; filename="A \"Quoted\" Filename With A Backslash -.txt"'],
44+
['A "Very" Weird Filename \ / & <> " >\'""""\.text', 'attachment; filename="A \"Very\" Weird Filename - - & <> \" >\'\"\"\"\"-.text"'],
45+
['\\\\\\\\\\\\', 'attachment; filename=------'],
4646
];
4747
}
48+
49+
public function testSpecialCharactersInFilename(): void {
50+
$filename = 'document "draft" with; special&chars.pdf';
51+
$response = new ChildDownloadResponse($filename, 'application/pdf');
52+
$headers = $response->getHeaders();
53+
54+
$this->assertEquals(
55+
'attachment; filename="document \"draft\" with; special&chars.pdf"',
56+
$headers['Content-Disposition']
57+
);
58+
}
4859
}

0 commit comments

Comments
 (0)