Skip to content

Commit 93b4978

Browse files
vitormattosvitormattos
committed
feat: add FileUploadHelper for centralized file upload validation
- Create new FileUploadHelper with validateUploadedFile() and readUploadedFile() - Centralizes upload validation (error check, is_uploaded_file, filename validation, size check) - Automatic cleanup (@Unlink) before throwing exceptions - Comprehensive unit tests with 7 test cases covering all edge cases - Mock is_uploaded_file() via namespace for testing - Follows Nextcloud core patterns (AvatarControllerTest) Signed-off-by: Vitor Mattos <[email protected]>
1 parent 95b1c72 commit 93b4978

File tree

7 files changed

+258
-19
lines changed

7 files changed

+258
-19
lines changed

‎lib/Helper/FileUploadHelper.php‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
/**
5+
* SPDX-FileCopyrightText: 2025 LibreCode coop and contributors
6+
* SPDX-License-Identifier: AGPL-3.0-or-later
7+
*/
8+
9+
namespace OCA\Libresign\Helper;
10+
11+
use InvalidArgumentException;
12+
use OC\Files\FilenameValidator;
13+
use OCP\IL10N;
14+
15+
/**
16+
* Helper for validating and processing uploaded files
17+
*/
18+
class FileUploadHelper {
19+
public function __construct(
20+
private IL10N $l10n,
21+
) {
22+
}
23+
24+
/**
25+
* Validate uploaded file from $_FILES
26+
*
27+
* @param array $uploadedFile Single file from $_FILES (e.g., $_FILES['file'])
28+
* @throws InvalidArgumentException
29+
*/
30+
public function validateUploadedFile(array $uploadedFile): void {
31+
if ($uploadedFile['error'] !== 0) {
32+
@unlink($uploadedFile['tmp_name']);
33+
throw new InvalidArgumentException($this->l10n->t('Invalid file provided'));
34+
}
35+
36+
if (!is_uploaded_file($uploadedFile['tmp_name'])) {
37+
@unlink($uploadedFile['tmp_name']);
38+
throw new InvalidArgumentException($this->l10n->t('Invalid file provided'));
39+
}
40+
41+
$validator = \OCP\Server::get(FilenameValidator::class);
42+
if ($validator->isForbidden($uploadedFile['tmp_name'])) {
43+
@unlink($uploadedFile['tmp_name']);
44+
throw new InvalidArgumentException($this->l10n->t('Invalid file provided'));
45+
}
46+
47+
if ($uploadedFile['size'] > \OCP\Util::uploadLimit()) {
48+
@unlink($uploadedFile['tmp_name']);
49+
throw new InvalidArgumentException($this->l10n->t('File is too big'));
50+
}
51+
}
52+
53+
/**
54+
* Read content from uploaded file
55+
*
56+
* @param array $uploadedFile Single file from $_FILES
57+
* @return string File content
58+
* @throws InvalidArgumentException
59+
*/
60+
public function readUploadedFile(array $uploadedFile): string {
61+
$content = file_get_contents($uploadedFile['tmp_name']);
62+
if ($content === false) {
63+
throw new InvalidArgumentException($this->l10n->t('Cannot read file'));
64+
}
65+
return $content;
66+
}
67+
}

‎lib/Helper/ValidateHelper.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ public function __construct(
7878
private IRootFolder $root,
7979
) {
8080
}
81+
8182
public function validateNewFile(array $data, int $type = self::TYPE_TO_SIGN, ?IUser $user = null): void {
8283
$this->validateFile($data, $type, $user);
8384
if (!empty($data['file']['fileId'])) {

‎lib/Service/AccountService.php‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99
namespace OCA\Libresign\Service;
1010

1111
use InvalidArgumentException;
12-
use OC\Files\Filesystem;
1312
use OCA\Libresign\AppInfo\Application;
1413
use OCA\Libresign\Db\File as FileEntity;
1514
use OCA\Libresign\Db\FileMapper;
@@ -23,6 +22,7 @@
2322
use OCA\Libresign\Exception\LibresignException;
2423
use OCA\Libresign\Handler\CertificateEngine\CertificateEngineFactory;
2524
use OCA\Libresign\Handler\SignEngine\Pkcs12Handler;
25+
use OCA\Libresign\Helper\FileUploadHelper;
2626
use OCA\Libresign\Helper\ValidateHelper;
2727
use OCA\Settings\Mailer\NewUserMailHelper;
2828
use OCP\Accounts\IAccountManager;
@@ -77,6 +77,7 @@ public function __construct(
7777
private FolderService $folderService,
7878
private IClientService $clientService,
7979
private ITimeFactory $timeFactory,
80+
private FileUploadHelper $uploadHelper,
8081
) {
8182
}
8283

@@ -509,14 +510,13 @@ public function deleteSignatureElement(?IUser $user, string $sessionId, int $nod
509510
* @throws InvalidArgumentException
510511
*/
511512
public function uploadPfx(array $file, IUser $user): void {
512-
if (
513-
$file['error'] !== 0
514-
|| !is_uploaded_file($file['tmp_name'])
515-
|| Filesystem::isFileBlacklisted($file['tmp_name'])
516-
) {
513+
try {
514+
$this->uploadHelper->validateUploadedFile($file);
515+
} catch (InvalidArgumentException) {
517516
// TRANSLATORS Error when the uploaded certificate file is not valid
518517
throw new InvalidArgumentException($this->l10n->t('Invalid file provided. Need to be a .pfx file.'));
519518
}
519+
520520
if ($file['size'] > 10 * 1024) {
521521
// TRANSLATORS Error when the certificate file is bigger than normal
522522
throw new InvalidArgumentException($this->l10n->t('File is too big'));

‎lib/Service/FileService.php‎

Lines changed: 3 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111
use DateTime;
1212
use DateTimeInterface;
1313
use InvalidArgumentException;
14-
use OC\Files\Filesystem;
1514
use OCA\Libresign\AppInfo\Application;
1615
use OCA\Libresign\Db\File;
1716
use OCA\Libresign\Db\FileElement;
@@ -24,6 +23,7 @@
2423
use OCA\Libresign\Exception\LibresignException;
2524
use OCA\Libresign\Handler\DocMdpHandler;
2625
use OCA\Libresign\Handler\SignEngine\Pkcs12Handler;
26+
use OCA\Libresign\Helper\FileUploadHelper;
2727
use OCA\Libresign\Helper\ValidateHelper;
2828
use OCA\Libresign\ResponseDefinitions;
2929
use OCA\Libresign\Service\IdentifyMethod\IIdentifyMethod;
@@ -93,6 +93,7 @@ public function __construct(
9393
protected LoggerInterface $logger,
9494
protected IL10N $l10n,
9595
private EnvelopeService $envelopeService,
96+
private FileUploadHelper $uploadHelper,
9697
) {
9798
$this->docMdpHandler = $docMdpHandler;
9899
$this->fileData = new stdClass();
@@ -206,18 +207,7 @@ public function setFileFromRequest(?array $file): self {
206207
if ($file === null) {
207208
throw new InvalidArgumentException($this->l10n->t('No file provided'));
208209
}
209-
if (
210-
$file['error'] !== 0
211-
|| !is_uploaded_file($file['tmp_name'])
212-
|| Filesystem::isFileBlacklisted($file['tmp_name'])
213-
) {
214-
unlink($file['tmp_name']);
215-
throw new InvalidArgumentException($this->l10n->t('Invalid file provided'));
216-
}
217-
if ($file['size'] > \OCP\Util::uploadLimit()) {
218-
unlink($file['tmp_name']);
219-
throw new InvalidArgumentException($this->l10n->t('File is too big'));
220-
}
210+
$this->uploadHelper->validateUploadedFile($file);
221211

222212
$this->fileContent = file_get_contents($file['tmp_name']);
223213
$mimeType = $this->mimeTypeDetector->detectString($this->fileContent);

‎lib/Service/RequestSignatureService.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
use OCA\Libresign\Enum\SignatureFlow;
1919
use OCA\Libresign\Events\SignRequestCanceledEvent;
2020
use OCA\Libresign\Handler\DocMdpHandler;
21+
use OCA\Libresign\Helper\FileUploadHelper;
2122
use OCA\Libresign\Helper\ValidateHelper;
2223
use OCA\Libresign\Service\IdentifyMethod\IIdentifyMethod;
2324
use OCP\AppFramework\Db\DoesNotExistException;
@@ -58,6 +59,7 @@ public function __construct(
5859
protected SignRequestStatusService $signRequestStatusService,
5960
protected DocMdpConfigService $docMdpConfigService,
6061
protected EnvelopeService $envelopeService,
62+
protected FileUploadHelper $uploadHelper,
6163
) {
6264
}
6365

‎lib/Service/TFile.php‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
use OCA\Libresign\Exception\LibresignException;
1212
use OCA\Libresign\Handler\DocMdpHandler;
13+
use OCA\Libresign\Helper\FileUploadHelper;
1314
use OCA\Libresign\Vendor\setasign\Fpdi\PdfParserService\Type\PdfTypeException;
1415
use OCP\Files\Node;
1516
use OCP\Http\Client\IClientService;
@@ -19,6 +20,7 @@ trait TFile {
1920
private $mimetype = null;
2021
protected IClientService $client;
2122
protected DocMdpHandler $docMdpHandler;
23+
protected FileUploadHelper $uploadHelper;
2224

2325
public function getNodeFromData(array $data): Node {
2426
if (!$this->folderService->getUserId()) {
@@ -45,6 +47,28 @@ public function getNodeFromData(array $data): Node {
4547
return $folderToFile->newFile($data['name'] . '.' . $extension, $content);
4648
}
4749

50+
public function getNodeFromUploadedFile(array $data): Node {
51+
if (!$this->folderService->getUserId()) {
52+
$this->folderService->setUserId($data['userManager']->getUID());
53+
}
54+
55+
$uploadedFile = $data['uploadedFile'];
56+
57+
$this->uploadHelper->validateUploadedFile($uploadedFile);
58+
$content = $this->uploadHelper->readUploadedFile($uploadedFile);
59+
60+
$extension = $this->getExtension($content);
61+
$this->validateFileContent($content, $extension);
62+
63+
$userFolder = $this->folderService->getFolder();
64+
$folderName = $this->folderService->getFolderName($data, $data['userManager']);
65+
$folderToFile = $userFolder->newFolder($folderName);
66+
67+
@unlink($uploadedFile['tmp_name']);
68+
69+
return $folderToFile->newFile($data['name'] . '.' . $extension, $content);
70+
}
71+
4872
/**
4973
* @throws \Exception
5074
* @throws LibresignException

‎tests/php/Unit/Helper/FileUploadHelperTest.php‎

Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,155 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
/**
5+
* SPDX-FileCopyrightText: 2025 LibreCode coop and contributors
6+
* SPDX-License-Identifier: AGPL-3.0-or-later
7+
*/
8+
9+
namespace OCA\Libresign\Helper;
10+
11+
function is_uploaded_file($filename) {
12+
return file_exists($filename);
13+
}
14+
15+
namespace OCA\Libresign\Tests\Unit\Helper;
16+
17+
use InvalidArgumentException;
18+
use OCA\Libresign\Helper\FileUploadHelper;
19+
use OCP\IL10N;
20+
use PHPUnit\Framework\MockObject\MockObject;
21+
use PHPUnit\Framework\TestCase;
22+
23+
class FileUploadHelperTest extends TestCase {
24+
private FileUploadHelper $helper;
25+
private IL10N&MockObject $l10n;
26+
private string $tempFile;
27+
28+
protected function setUp(): void {
29+
parent::setUp();
30+
31+
$this->l10n = $this->createMock(IL10N::class);
32+
$this->l10n->method('t')
33+
->willReturnCallback(fn ($text) => $text);
34+
35+
$this->helper = new FileUploadHelper($this->l10n);
36+
37+
$this->tempFile = tempnam(sys_get_temp_dir(), 'upload_test_');
38+
file_put_contents($this->tempFile, 'test content');
39+
}
40+
41+
protected function tearDown(): void {
42+
if (file_exists($this->tempFile)) {
43+
@unlink($this->tempFile);
44+
}
45+
parent::tearDown();
46+
}
47+
48+
public function testValidateUploadedFileSuccess(): void {
49+
$uploadedFile = [
50+
'tmp_name' => $this->tempFile,
51+
'error' => UPLOAD_ERR_OK,
52+
'size' => filesize($this->tempFile),
53+
];
54+
55+
$this->helper->validateUploadedFile($uploadedFile);
56+
57+
$this->assertTrue(file_exists($this->tempFile));
58+
}
59+
60+
public function testValidateUploadedFileWithUploadError(): void {
61+
$uploadedFile = [
62+
'tmp_name' => $this->tempFile,
63+
'error' => UPLOAD_ERR_INI_SIZE,
64+
'size' => 0,
65+
];
66+
67+
$this->expectException(InvalidArgumentException::class);
68+
$this->expectExceptionMessage('Invalid file provided');
69+
70+
try {
71+
$this->helper->validateUploadedFile($uploadedFile);
72+
} finally {
73+
$this->assertFalse(file_exists($this->tempFile), 'File should be deleted after error');
74+
}
75+
}
76+
77+
public function testValidateUploadedFileNotActuallyUploaded(): void {
78+
$nonExistentFile = sys_get_temp_dir() . '/non_existent_file_' . time();
79+
80+
$uploadedFile = [
81+
'tmp_name' => $nonExistentFile,
82+
'error' => UPLOAD_ERR_OK,
83+
'size' => 100,
84+
];
85+
86+
$this->expectException(InvalidArgumentException::class);
87+
$this->expectExceptionMessage('Invalid file provided');
88+
89+
$this->helper->validateUploadedFile($uploadedFile);
90+
}
91+
92+
public function testValidateUploadedFileTooBig(): void {
93+
$uploadedFile = [
94+
'tmp_name' => $this->tempFile,
95+
'error' => UPLOAD_ERR_OK,
96+
'size' => \OCP\Util::uploadLimit() + 1,
97+
];
98+
99+
$this->expectException(InvalidArgumentException::class);
100+
$this->expectExceptionMessage('File is too big');
101+
102+
try {
103+
$this->helper->validateUploadedFile($uploadedFile);
104+
} finally {
105+
$this->assertFalse(file_exists($this->tempFile), 'File should be deleted when too big');
106+
}
107+
}
108+
109+
public function testReadUploadedFileSuccess(): void {
110+
$expectedContent = 'test file content';
111+
file_put_contents($this->tempFile, $expectedContent);
112+
113+
$uploadedFile = [
114+
'tmp_name' => $this->tempFile,
115+
];
116+
117+
$content = $this->helper->readUploadedFile($uploadedFile);
118+
119+
$this->assertEquals($expectedContent, $content);
120+
}
121+
122+
public function testReadUploadedFileNotReadable(): void {
123+
$uploadedFile = [
124+
'tmp_name' => '/path/that/does/not/exist.txt',
125+
];
126+
127+
$this->expectException(InvalidArgumentException::class);
128+
$this->expectExceptionMessage('Cannot read file');
129+
130+
$this->helper->readUploadedFile($uploadedFile);
131+
}
132+
133+
public function testValidateUploadedFileWithForbiddenName(): void {
134+
$forbiddenFile = sys_get_temp_dir() . '/test<file>.txt';
135+
136+
if (@file_put_contents($forbiddenFile, 'test') === false) {
137+
$this->markTestSkipped('Cannot create file with forbidden characters on this OS');
138+
return;
139+
}
140+
141+
$uploadedFile = [
142+
'tmp_name' => $forbiddenFile,
143+
'error' => UPLOAD_ERR_OK,
144+
'size' => filesize($forbiddenFile),
145+
];
146+
147+
try {
148+
$this->helper->validateUploadedFile($uploadedFile);
149+
@unlink($forbiddenFile);
150+
} catch (InvalidArgumentException $e) {
151+
$this->assertEquals('Invalid file provided', $e->getMessage());
152+
$this->assertFalse(file_exists($forbiddenFile));
153+
}
154+
}
155+
}

0 commit comments

Comments
 (0)