Skip to content

Commit b174bdb

Browse files
n0nag0nn0nag0n
committed
more changes with upload. Security enhancements. Also got rid of php-watcher
1 parent 049d445 commit b174bdb

File tree

3 files changed

+118
-30
lines changed

3 files changed

+118
-30
lines changed

‎composer.json‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,6 @@
5151
"phpstan/phpstan": "^2.1",
5252
"phpunit/phpunit": "^9.6",
5353
"rregeer/phpunit-coverage-check": "^0.3.1",
54-
"spatie/phpunit-watcher": "^1.23 || ^1.24",
5554
"squizlabs/php_codesniffer": "^3.11"
5655
},
5756
"config": {
@@ -62,7 +61,7 @@
6261
"sort-packages": true
6362
},
6463
"scripts": {
65-
"test": "vendor/bin/phpunit-watcher watch",
64+
"test": "phpunit",
6665
"test-ci": "phpunit",
6766
"test-coverage": "rm -f clover.xml && XDEBUG_MODE=coverage vendor/bin/phpunit --coverage-html=coverage --coverage-clover=clover.xml && vendor/bin/coverage-check clover.xml 100",
6867
"test-server": "echo \"Running Test Server\" && php -S localhost:8000 -t tests/server/",

‎flight/net/UploadedFile.php‎

Lines changed: 46 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,11 @@ class UploadedFile
3333
*/
3434
private int $error;
3535

36+
/**
37+
* @var bool $isPostUploadedFile Indicates if the file was uploaded via POST method.
38+
*/
39+
private bool $isPostUploadedFile = false;
40+
3641
/**
3742
* Constructs a new UploadedFile object.
3843
*
@@ -41,14 +46,20 @@ class UploadedFile
4146
* @param int $size The size of the uploaded file in bytes.
4247
* @param string $tmpName The temporary name of the uploaded file.
4348
* @param int $error The error code associated with the uploaded file.
49+
* @param bool|null $isPostUploadedFile Indicates if the file was uploaded via POST method.
4450
*/
45-
public function __construct(string $name, string $mimeType, int $size, string $tmpName, int $error)
51+
public function __construct(string $name, string $mimeType, int $size, string $tmpName, int $error, ?bool $isPostUploadedFile = null)
4652
{
4753
$this->name = $name;
4854
$this->mimeType = $mimeType;
4955
$this->size = $size;
5056
$this->tmpName = $tmpName;
5157
$this->error = $error;
58+
if (is_uploaded_file($tmpName) === true) {
59+
$this->isPostUploadedFile = true; // @codeCoverageIgnore
60+
} else {
61+
$this->isPostUploadedFile = $isPostUploadedFile ?? false;
62+
}
5263
}
5364

5465
/**
@@ -114,32 +125,42 @@ public function moveTo(string $targetPath): void
114125
throw new Exception($this->getUploadErrorMessage($this->error));
115126
}
116127

128+
if (is_writeable(dirname($targetPath)) === false) {
129+
throw new Exception('Target directory is not writable');
130+
}
131+
132+
// Prevent path traversal attacks
133+
if (strpos($targetPath, '..') !== false) {
134+
throw new Exception('Invalid target path: contains directory traversal');
135+
}
136+
// Prevent absolute paths (basic check for Unix/Windows)
137+
if ($targetPath[0] === '/' || (strlen($targetPath) > 1 && $targetPath[1] === ':')) {
138+
throw new Exception('Invalid target path: absolute paths not allowed');
139+
}
140+
141+
// Prevent overwriting existing files
142+
if (file_exists($targetPath)) {
143+
throw new Exception('Target file already exists');
144+
}
145+
117146
// Check if this is a legitimate uploaded file (POST method uploads)
118-
$isUploadedFile = is_uploaded_file($this->tmpName) === true;
119-
120-
if ($isUploadedFile === true) {
147+
$isUploadedFile = $this->isPostUploadedFile;
148+
149+
// Prevent symlink attacks for non-POST uploads
150+
if (!$isUploadedFile && is_link($this->tmpName)) {
151+
throw new Exception('Invalid temp file: symlink detected');
152+
}
153+
154+
$uploadFunctionToCall = $isUploadedFile === true ?
121155
// Standard POST upload - use move_uploaded_file for security
122-
if (move_uploaded_file($this->tmpName, $targetPath) === false) {
123-
throw new Exception('Cannot move uploaded file'); // @codeCoverageIgnore
124-
}
125-
} elseif (getenv('PHPUNIT_TEST')) {
126-
rename($this->tmpName, $targetPath);
127-
} elseif (file_exists($this->tmpName) === true && is_readable($this->tmpName) === true) {
128-
// Handle non-POST uploads (PATCH, PUT, DELETE) or other valid temp files
129-
// Verify the file is in a valid temp directory for security
130-
$tempDir = sys_get_temp_dir();
131-
$uploadTmpDir = ini_get('upload_tmp_dir') ?: $tempDir;
132-
133-
if (strpos(realpath($this->tmpName), realpath($uploadTmpDir)) === 0 ||
134-
strpos(realpath($this->tmpName), realpath($tempDir)) === 0) {
135-
if (rename($this->tmpName, $targetPath) === false) {
136-
throw new Exception('Cannot move uploaded file');
137-
}
138-
} else {
139-
throw new Exception('Invalid temporary file location');
140-
}
141-
} else {
142-
throw new Exception('Temporary file does not exist or is not readable');
156+
'move_uploaded_file' :
157+
// Handle non-POST uploads (PATCH, PUT, DELETE) or other valid temp files
158+
'rename';
159+
160+
$result = $uploadFunctionToCall($this->tmpName, $targetPath);
161+
162+
if ($result === false) {
163+
throw new Exception('Cannot move uploaded file');
143164
}
144165
}
145166

‎tests/UploadedFileTest.php‎

Lines changed: 71 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,24 @@ public function tearDown(): void
1818
if (file_exists('tmp_name')) {
1919
unlink('tmp_name');
2020
}
21+
if (file_exists('existing.txt')) {
22+
unlink('existing.txt');
23+
}
24+
if (file_exists('real_file')) {
25+
unlink('real_file');
26+
}
27+
28+
// not found with file_exists...just delete it brute force
29+
@unlink('tmp_symlink');
2130
}
2231

23-
public function testMoveToSuccess(): void
32+
public function testMoveToFalseSuccess(): void
2433
{
34+
// This test would have passed in the real world but we can't actually force a post request in unit tests
2535
file_put_contents('tmp_name', 'test');
26-
$uploadedFile = new UploadedFile('file.txt', 'text/plain', 4, 'tmp_name', UPLOAD_ERR_OK);
36+
$uploadedFile = new UploadedFile('file.txt', 'text/plain', 4, 'tmp_name', UPLOAD_ERR_OK, true);
37+
$this->expectExceptionMessage('Cannot move uploaded file');
2738
$uploadedFile->moveTo('file.txt');
28-
$this->assertFileExists('file.txt');
2939
}
3040

3141
public function getFileErrorMessageTests(): array
@@ -53,4 +63,62 @@ public function testMoveToFailureMessages($error, $message)
5363
$this->expectExceptionMessage($message);
5464
$uploadedFile->moveTo('file.txt');
5565
}
66+
67+
public function testMoveToBadLocation(): void
68+
{
69+
file_put_contents('tmp_name', 'test');
70+
$uploadedFile = new UploadedFile('file.txt', 'text/plain', 4, 'tmp_name', UPLOAD_ERR_OK, true);
71+
$this->expectExceptionMessage('Target directory is not writable');
72+
$uploadedFile->moveTo('/root/file.txt');
73+
}
74+
75+
public function testMoveToSuccessNonPost(): void
76+
{
77+
file_put_contents('tmp_name', 'test');
78+
$uploadedFile = new UploadedFile('file.txt', 'text/plain', 4, 'tmp_name', UPLOAD_ERR_OK, false);
79+
$uploadedFile->moveTo('file.txt');
80+
$this->assertFileExists('file.txt');
81+
$this->assertEquals('test', file_get_contents('file.txt'));
82+
}
83+
84+
public function testMoveToPathTraversal(): void
85+
{
86+
file_put_contents('tmp_name', 'test');
87+
$uploadedFile = new UploadedFile('file.txt', 'text/plain', 4, 'tmp_name', UPLOAD_ERR_OK, false);
88+
$this->expectException(Exception::class);
89+
$this->expectExceptionMessage('Invalid target path: contains directory traversal');
90+
$uploadedFile->moveTo('../file.txt');
91+
}
92+
93+
public function testMoveToAbsolutePath(): void
94+
{
95+
file_put_contents('tmp_name', 'test');
96+
$uploadedFile = new UploadedFile('file.txt', 'text/plain', 4, 'tmp_name', UPLOAD_ERR_OK, false);
97+
$this->expectException(Exception::class);
98+
$this->expectExceptionMessage('Invalid target path: absolute paths not allowed');
99+
$uploadedFile->moveTo('/tmp/file.txt');
100+
}
101+
102+
public function testMoveToOverwrite(): void
103+
{
104+
file_put_contents('tmp_name', 'test');
105+
file_put_contents('existing.txt', 'existing');
106+
$uploadedFile = new UploadedFile('file.txt', 'text/plain', 4, 'tmp_name', UPLOAD_ERR_OK, false);
107+
$this->expectException(Exception::class);
108+
$this->expectExceptionMessage('Target file already exists');
109+
$uploadedFile->moveTo('existing.txt');
110+
}
111+
112+
public function testMoveToSymlinkNonPost(): void
113+
{
114+
file_put_contents('real_file', 'test');
115+
if (file_exists('tmp_symlink')) {
116+
unlink('tmp_symlink');
117+
}
118+
symlink('real_file', 'tmp_symlink');
119+
$uploadedFile = new UploadedFile('file.txt', 'text/plain', 4, 'tmp_symlink', UPLOAD_ERR_OK, false);
120+
$this->expectException(Exception::class);
121+
$this->expectExceptionMessage('Invalid temp file: symlink detected');
122+
$uploadedFile->moveTo('file.txt');
123+
}
56124
}

0 commit comments

Comments
 (0)