Skip to content

Commit 20ebcf4

Browse files
michalsngr8man
andauthored
Merge commit from fork
Co-authored-by: Bogdan <matolq@gmail.com>
1 parent f5e463b commit 20ebcf4

4 files changed

Lines changed: 48 additions & 3 deletions

File tree

system/HTTP/Files/UploadedFile.php

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,8 @@ public function __construct(string $path, string $originalName, ?string $mimeTyp
123123
* @see http://php.net/move_uploaded_file
124124
*
125125
* @param string $targetPath Path to which to move the uploaded file.
126-
* @param string|null $name the name to rename the file to.
126+
* @param string|null $name The name to rename the file to. When null, the client-provided name is used and sanitized.
127+
* A caller-supplied name is NOT sanitized.
127128
* @param bool $overwrite State for indicating whether to overwrite the previously generated file with the same
128129
* name or not.
129130
*
@@ -142,7 +143,10 @@ public function move(string $targetPath, ?string $name = null, bool $overwrite =
142143
throw HTTPException::forInvalidFile();
143144
}
144145

145-
$name ??= $this->getName();
146+
if ($name === null) {
147+
helper('security');
148+
$name = sanitize_filename($this->getName());
149+
}
146150
$destination = $overwrite ? $targetPath . $name : $this->getDestination($targetPath . $name);
147151

148152
try {

tests/system/HTTP/Files/FileMovingTest.php

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,38 @@ public function testMove(): void
100100
$this->assertTrue($this->root->hasChild('destination/' . $finalFilename . '_1.txt'));
101101
}
102102

103+
public function testMoveSanitizesClientNameByDefault(): void
104+
{
105+
service('superglobals')->setFilesArray([
106+
'userfile' => [
107+
'name' => '../../public/shell.php',
108+
'type' => 'text/plain',
109+
'size' => 124,
110+
'tmp_name' => '/tmp/fileA.txt',
111+
'error' => 0,
112+
],
113+
]);
114+
115+
$collection = new FileCollection();
116+
117+
$this->assertTrue($collection->hasFile('userfile'));
118+
119+
$destination = $this->destination;
120+
if (! is_dir($destination)) {
121+
mkdir($destination, 0777, true);
122+
}
123+
124+
$file = $collection->getFile('userfile');
125+
$this->assertInstanceOf(UploadedFile::class, $file);
126+
127+
// No second argument: the client-provided name must be sanitized.
128+
$file->move($destination);
129+
130+
$this->assertSame('publicshell.php', $file->getName());
131+
$this->assertTrue($this->root->hasChild('destination/publicshell.php'));
132+
$this->assertFalse($this->root->hasChild('public/shell.php'));
133+
}
134+
103135
public function testMoveOverwriting(): void
104136
{
105137
$finalFilename = 'file_with_delimiters_underscore';

user_guide_src/source/changelogs/v4.7.4.rst

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,15 @@ SECURITY
3030
See the `Security advisory GHSA-c9w5-rwh3-7pm9 <https://github.com/codeigniter4/CodeIgniter4/security/advisories/GHSA-c9w5-rwh3-7pm9>`_
3131
for more information.
3232

33+
- **UploadedFile:** ``UploadedFile::move()`` now sanitizes the client-provided
34+
filename when called without a second argument. Previously, the unsanitized
35+
client filename was used as the default, allowing path traversal sequences
36+
(e.g. ``../../public/shell.php``) to write the uploaded file outside the
37+
intended directory. A name explicitly passed as the second argument is not
38+
sanitized and remains the caller's responsibility.
39+
See the `Security advisory GHSA-hhmc-q9hp-r662 <https://github.com/codeigniter4/CodeIgniter4/security/advisories/GHSA-hhmc-q9hp-r662>`_
40+
for more information.
41+
3342
********
3443
BREAKING
3544
********

user_guide_src/source/libraries/uploaded_files.rst

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -339,7 +339,7 @@ the file to as the first parameter:
339339

340340
.. literalinclude:: uploaded_files/016.php
341341

342-
By default, the original filename was used.
342+
By default, the client-provided filename is sanitized and used.
343343

344344
with New Filename
345345
-----------------

0 commit comments

Comments
 (0)