Skip to content

Commit db2fddd

Browse files
committed
fix(upload): replace non-existent isForbidden with isFilenameValid
IFilenameValidator has no isForbidden() method. The correct API is isFilenameValid(string $filename): bool which returns true when the name is acceptable – so the guard must negate it. Also updated FileUploadHelperTest: - testValidateUploadedFileSuccess: sets isFilenameValid→true explicitly - testValidateUploadedFileWithForbiddenName: mocks isFilenameValid→false (PHPUnit will not let a per-test stub override a setUp stub, so the default was removed from setUp and added only to the success test) Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
1 parent c7f1027 commit db2fddd

2 files changed

Lines changed: 17 additions & 22 deletions

File tree

lib/Helper/FileUploadHelper.php

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

1111
use InvalidArgumentException;
12-
use OC\Files\FilenameValidator;
12+
use OCP\Files\IFilenameValidator;
1313
use OCP\IL10N;
1414

1515
/**
@@ -18,6 +18,7 @@
1818
class FileUploadHelper {
1919
public function __construct(
2020
private IL10N $l10n,
21+
private IFilenameValidator $filenameValidator,
2122
) {
2223
}
2324

@@ -43,8 +44,7 @@ public function validateUploadedFile(array $uploadedFile): void {
4344
throw new InvalidArgumentException($this->l10n->t('File is too big'));
4445
}
4546

46-
$validator = \OCP\Server::get(FilenameValidator::class);
47-
if ($validator->isForbidden($uploadedFile['tmp_name'])) {
47+
if (!$this->filenameValidator->isFilenameValid(basename($uploadedFile['tmp_name']))) {
4848
@unlink($uploadedFile['tmp_name']);
4949
throw new InvalidArgumentException($this->l10n->t('Invalid file provided'));
5050
}

tests/php/Unit/Helper/FileUploadHelperTest.php

Lines changed: 14 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,15 @@ function is_uploaded_file($filename) {
1616

1717
use InvalidArgumentException;
1818
use OCA\Libresign\Helper\FileUploadHelper;
19+
use OCP\Files\IFilenameValidator;
1920
use OCP\IL10N;
2021
use PHPUnit\Framework\MockObject\MockObject;
2122
use PHPUnit\Framework\TestCase;
2223

2324
class FileUploadHelperTest extends TestCase {
2425
private FileUploadHelper $helper;
2526
private IL10N&MockObject $l10n;
27+
private IFilenameValidator&MockObject $filenameValidator;
2628
private string $tempFile;
2729

2830
protected function setUp(): void {
@@ -32,7 +34,9 @@ protected function setUp(): void {
3234
$this->l10n->method('t')
3335
->willReturnCallback(fn ($text) => $text);
3436

35-
$this->helper = new FileUploadHelper($this->l10n);
37+
$this->filenameValidator = $this->createMock(IFilenameValidator::class);
38+
39+
$this->helper = new FileUploadHelper($this->l10n, $this->filenameValidator);
3640

3741
$this->tempFile = tempnam(sys_get_temp_dir(), 'upload_test_');
3842
file_put_contents($this->tempFile, 'test content');
@@ -46,6 +50,8 @@ protected function tearDown(): void {
4650
}
4751

4852
public function testValidateUploadedFileSuccess(): void {
53+
$this->filenameValidator->method('isFilenameValid')->willReturn(true);
54+
4955
$uploadedFile = [
5056
'tmp_name' => $this->tempFile,
5157
'error' => UPLOAD_ERR_OK,
@@ -131,32 +137,21 @@ public function testReadUploadedFileNotReadable(): void {
131137
}
132138

133139
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+
$this->filenameValidator->method('isFilenameValid')->willReturn(false);
140141

141142
$uploadedFile = [
142-
'tmp_name' => $forbiddenFile,
143+
'tmp_name' => $this->tempFile,
143144
'error' => UPLOAD_ERR_OK,
144-
'size' => filesize($forbiddenFile),
145+
'size' => filesize($this->tempFile),
145146
];
146147

147-
$exceptionThrown = false;
148+
$this->expectException(InvalidArgumentException::class);
149+
$this->expectExceptionMessage('Invalid file provided');
150+
148151
try {
149152
$this->helper->validateUploadedFile($uploadedFile);
150-
} catch (InvalidArgumentException $e) {
151-
$exceptionThrown = true;
152-
$this->assertEquals('Invalid file provided', $e->getMessage());
153-
$this->assertFalse(file_exists($forbiddenFile), 'File should be deleted after validation fails');
154153
} finally {
155-
@unlink($forbiddenFile);
156-
}
157-
158-
if (!$exceptionThrown) {
159-
$this->markTestSkipped('FilenameValidator does not consider this filename as forbidden on this OS');
154+
$this->assertFalse(file_exists($this->tempFile), 'File should be deleted after validation fails');
160155
}
161156
}
162157
}

0 commit comments

Comments
 (0)