Skip to content

Commit f1b48d9

Browse files
petarjakopecpetarjakopec
committed
SPLAT-4180 add stricter checking of encription
1 parent 24eb4e7 commit f1b48d9

2 files changed

Lines changed: 200 additions & 5 deletions

File tree

‎bundle/Controller/Resource/Upload.php‎

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,12 @@
2727
use function is_array;
2828
use function is_file;
2929
use function is_readable;
30+
use function filesize;
31+
use function preg_match;
32+
use function strrpos;
3033
use function strpos;
3134
use function strtolower;
35+
use function substr;
3236

3337
use const SEEK_END;
3438

@@ -142,17 +146,36 @@ private function isEncryptedPdf(UploadedFile $file): bool
142146
return false;
143147
}
144148

145-
$head = (string) fread($fp, 4096);
149+
$fileSize = filesize($path);
146150

147-
@fseek($fp, -16384, SEEK_END);
148-
$tail = (string) fread($fp, 16384);
151+
// For small PDFs, `head` and `tail` reads can overlap and even fully duplicate the file contents.
152+
// This can re-introduce false positives (e.g. `/Encrypt` in a trailing comment after `%%EOF`).
153+
// In that case, scan the full content once.
154+
if ($fileSize !== false && $fileSize <= 20480) {
155+
$content = (string) fread($fp, $fileSize);
156+
} else {
157+
$head = (string) fread($fp, 4096);
158+
159+
@fseek($fp, -16384, SEEK_END);
160+
$tail = (string) fread($fp, 16384);
161+
162+
$content = $head . $tail;
163+
}
149164

150165
fclose($fp);
151166

152-
if (strpos($head, '%PDF-') !== 0) {
167+
if (strpos($content, '%PDF-') !== 0) {
153168
return false;
154169
}
155170

156-
return strpos($head . $tail, '/Encrypt') !== false;
171+
// Ignore anything after the last EOF marker (some tools append non-PDF comments/metadata).
172+
$eofPos = strrpos($content, '%%EOF');
173+
if ($eofPos !== false) {
174+
$content = substr($content, 0, $eofPos + 5);
175+
}
176+
177+
// Detect presence of encryption dictionary reference in PDF object context.
178+
// This avoids false positives where `/Encrypt` appears in metadata or trailing comments.
179+
return (bool) preg_match('/\/(?:Encrypt)\s+(\d+|<<)/m', $content);
157180
}
158181
}

‎tests/bundle/Controller/Resource/UploadTest.php‎

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,178 @@ public function testUploadEncryptedPdfIsRaw(): void
270270
unlink($tmpPdfPath);
271271
}
272272

273+
public function testUploadPdfWithEncryptInMetadataIsAuto(): void
274+
{
275+
$tmpPdfPath = (string) tempnam(sys_get_temp_dir(), 'ngrm_unencrypted_pdf_metadata_');
276+
file_put_contents(
277+
$tmpPdfPath,
278+
"%PDF-1.7\n1 0 obj\n<< /Type /Catalog >>\nendobj\n" .
279+
"/Title (/Encrypt)\n" .
280+
"%%EOF\n",
281+
);
282+
283+
$request = new Request();
284+
$request->request->add([
285+
'folder' => 'media/document',
286+
]);
287+
288+
$uploadedFileMock = $this->createMock(UploadedFile::class);
289+
290+
$uploadedFileMock
291+
->expects(self::once())
292+
->method('isFile')
293+
->willReturn(true);
294+
295+
// getRealPath is used for md5 hash + FileStruct + encryption detector
296+
$uploadedFileMock
297+
->expects(self::exactly(4))
298+
->method('getRealPath')
299+
->willReturn($tmpPdfPath);
300+
301+
$uploadedFileMock
302+
->expects(self::exactly(2))
303+
->method('getClientOriginalName')
304+
->willReturn('unencrypted.pdf');
305+
306+
// getClientOriginalExtension is used for FileStruct + encryption detector
307+
$uploadedFileMock
308+
->expects(self::exactly(3))
309+
->method('getClientOriginalExtension')
310+
->willReturn('pdf');
311+
312+
$request->files->add([
313+
'file' => $uploadedFileMock,
314+
]);
315+
316+
$this->fileHashFactoryMock
317+
->expects(self::once())
318+
->method('createHash')
319+
->with($tmpPdfPath)
320+
->willReturn('md5hash');
321+
322+
$fileStruct = FileStruct::fromUploadedFile($uploadedFileMock);
323+
324+
$resourceStruct = new ResourceStruct(
325+
$fileStruct,
326+
'auto',
327+
Folder::fromPath('media/document'),
328+
'public',
329+
$request->request->get('filename'),
330+
);
331+
332+
$resource = new RemoteResource(
333+
remoteId: 'upload|auto|media/document/unencrypted.pdf',
334+
type: 'auto',
335+
url: 'https://cloudinary.com/test/upload/auto/media/document/unencrypted.pdf',
336+
md5: 'md5hash',
337+
name: 'unencrypted.pdf',
338+
folder: Folder::fromPath('media/document'),
339+
size: 123,
340+
);
341+
342+
$this->providerMock
343+
->expects(self::once())
344+
->method('upload')
345+
->with($resourceStruct)
346+
->willReturn($resource);
347+
348+
$this->providerMock
349+
->expects(self::exactly(0))
350+
->method('buildVariation');
351+
352+
$response = $this->controller->__invoke($request);
353+
354+
self::assertInstanceOf(JsonResponse::class, $response);
355+
356+
unlink($tmpPdfPath);
357+
}
358+
359+
public function testUploadPdfWithEncryptInTrailingCommentAfterEofIsAuto(): void
360+
{
361+
$tmpPdfPath = (string) tempnam(sys_get_temp_dir(), 'ngrm_unencrypted_pdf_comment_');
362+
file_put_contents(
363+
$tmpPdfPath,
364+
"%PDF-1.7\n1 0 obj\n<< /Type /Catalog >>\nendobj\n" .
365+
"%%EOF\n" .
366+
"% /Encrypt 2 0 R\n",
367+
);
368+
369+
$request = new Request();
370+
$request->request->add([
371+
'folder' => 'media/document',
372+
]);
373+
374+
$uploadedFileMock = $this->createMock(UploadedFile::class);
375+
376+
$uploadedFileMock
377+
->expects(self::once())
378+
->method('isFile')
379+
->willReturn(true);
380+
381+
// getRealPath is used for md5 hash + FileStruct + encryption detector
382+
$uploadedFileMock
383+
->expects(self::exactly(4))
384+
->method('getRealPath')
385+
->willReturn($tmpPdfPath);
386+
387+
$uploadedFileMock
388+
->expects(self::exactly(2))
389+
->method('getClientOriginalName')
390+
->willReturn('unencrypted.pdf');
391+
392+
// getClientOriginalExtension is used for FileStruct + encryption detector
393+
$uploadedFileMock
394+
->expects(self::exactly(3))
395+
->method('getClientOriginalExtension')
396+
->willReturn('pdf');
397+
398+
$request->files->add([
399+
'file' => $uploadedFileMock,
400+
]);
401+
402+
$this->fileHashFactoryMock
403+
->expects(self::once())
404+
->method('createHash')
405+
->with($tmpPdfPath)
406+
->willReturn('md5hash');
407+
408+
$fileStruct = FileStruct::fromUploadedFile($uploadedFileMock);
409+
410+
$resourceStruct = new ResourceStruct(
411+
$fileStruct,
412+
'auto',
413+
Folder::fromPath('media/document'),
414+
'public',
415+
$request->request->get('filename'),
416+
);
417+
418+
$resource = new RemoteResource(
419+
remoteId: 'upload|auto|media/document/unencrypted.pdf',
420+
type: 'auto',
421+
url: 'https://cloudinary.com/test/upload/auto/media/document/unencrypted.pdf',
422+
md5: 'md5hash',
423+
name: 'unencrypted.pdf',
424+
folder: Folder::fromPath('media/document'),
425+
size: 123,
426+
);
427+
428+
$this->providerMock
429+
->expects(self::once())
430+
->method('upload')
431+
->with($resourceStruct)
432+
->willReturn($resource);
433+
434+
$this->providerMock
435+
->expects(self::exactly(0))
436+
->method('buildVariation');
437+
438+
$response = $this->controller->__invoke($request);
439+
440+
self::assertInstanceOf(JsonResponse::class, $response);
441+
442+
unlink($tmpPdfPath);
443+
}
444+
273445
public function testUploadProtectedWithContext(): void
274446
{
275447
$uploadContext = [

0 commit comments

Comments
 (0)