Skip to content

Commit d002547

Browse files
kenjiskenjis
authored
Merge pull request #5302 from kenjis/fix-UploadedFile-hasMoved
fix: UploadedFile::move() may return incorrect value
2 parents 6ba7f27 + c0a8005 commit d002547

File tree

2 files changed

+70
-27
lines changed

2 files changed

+70
-27
lines changed

‎system/HTTP/Files/UploadedFile.php

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -141,20 +141,25 @@ public function move(string $targetPath, ?string $name = null, bool $overwrite =
141141
$destination = $overwrite ? $targetPath . $name : $this->getDestination($targetPath . $name);
142142

143143
try {
144-
move_uploaded_file($this->path, $destination);
144+
$this->hasMoved = move_uploaded_file($this->path, $destination);
145145
} catch (Exception $e) {
146146
$error = error_get_last();
147-
$message = isset($error['message']) ? strip_tags($error['message']) : '';
147+
$message = strip_tags($error['message'] ?? '');
148+
149+
throw HTTPException::forMoveFailed(basename($this->path), $targetPath, $message);
150+
}
151+
152+
if ($this->hasMoved === false) {
153+
$message = 'move_uploaded_file() returned false';
148154

149155
throw HTTPException::forMoveFailed(basename($this->path), $targetPath, $message);
150156
}
151157

152158
@chmod($targetPath, 0777 & ~umask());
153159

154160
// Success, so store our new information
155-
$this->path = $targetPath;
156-
$this->name = basename($destination);
157-
$this->hasMoved = true;
161+
$this->path = $targetPath;
162+
$this->name = basename($destination);
158163

159164
return true;
160165
}

‎tests/system/HTTP/Files/FileMovingTest.php

Lines changed: 60 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,9 @@ protected function setUp(): void
3535
}
3636

3737
$_FILES = [];
38+
39+
// Set the mock's return value to true
40+
move_uploaded_file('', '', true);
3841
}
3942

4043
protected function tearDown(): void
@@ -52,8 +55,7 @@ protected function tearDown(): void
5255
public function testMove()
5356
{
5457
$finalFilename = 'fileA';
55-
56-
$_FILES = [
58+
$_FILES = [
5759
'userfile1' => [
5860
'name' => $finalFilename . '.txt',
5961
'type' => 'text/plain',
@@ -76,7 +78,6 @@ public function testMove()
7678
$this->assertTrue($collection->hasFile('userfile2'));
7779

7880
$destination = $this->destination;
79-
8081
// Create the destination if not exists
8182
if (! is_dir($destination)) {
8283
mkdir($destination, 0777, true);
@@ -94,8 +95,7 @@ public function testMove()
9495
public function testMoveOverwriting()
9596
{
9697
$finalFilename = 'file_with_delimiters_underscore';
97-
98-
$_FILES = [
98+
$_FILES = [
9999
'userfile1' => [
100100
'name' => $finalFilename . '.txt',
101101
'type' => 'text/plain',
@@ -126,7 +126,6 @@ public function testMoveOverwriting()
126126
$this->assertTrue($collection->hasFile('userfile3'));
127127

128128
$destination = $this->destination;
129-
130129
// Create the destination if not exists
131130
if (! is_dir($destination)) {
132131
mkdir($destination, 0777, true);
@@ -146,8 +145,7 @@ public function testMoveOverwriting()
146145
public function testMoved()
147146
{
148147
$finalFilename = 'fileA';
149-
150-
$_FILES = [
148+
$_FILES = [
151149
'userfile1' => [
152150
'name' => $finalFilename . '.txt',
153151
'type' => 'text/plain',
@@ -162,7 +160,6 @@ public function testMoved()
162160
$this->assertTrue($collection->hasFile('userfile1'));
163161

164162
$destination = $this->destination;
165-
166163
// Create the destination if not exists
167164
if (! is_dir($destination)) {
168165
mkdir($destination, 0777, true);
@@ -172,15 +169,16 @@ public function testMoved()
172169

173170
$this->assertInstanceOf(UploadedFile::class, $file);
174171
$this->assertFalse($file->hasMoved());
172+
175173
$file->move($destination, $file->getName(), false);
174+
176175
$this->assertTrue($file->hasMoved());
177176
}
178177

179178
public function testStore()
180179
{
181180
$finalFilename = 'fileA';
182-
183-
$_FILES = [
181+
$_FILES = [
184182
'userfile1' => [
185183
'name' => $finalFilename . '.txt',
186184
'type' => 'text/plain',
@@ -195,7 +193,6 @@ public function testStore()
195193
$this->assertTrue($collection->hasFile('userfile1'));
196194

197195
$destination = $this->destination;
198-
199196
// Create the destination if not exists
200197
if (! is_dir($destination)) {
201198
mkdir($destination, 0777, true);
@@ -204,15 +201,16 @@ public function testStore()
204201
$file = $collection->getFile('userfile1');
205202

206203
$this->assertInstanceOf(UploadedFile::class, $file);
204+
207205
$path = $file->store($destination, $file->getName());
206+
208207
$this->assertSame($destination . '/fileA.txt', $path);
209208
}
210209

211210
public function testAlreadyMoved()
212211
{
213212
$finalFilename = 'fileA';
214-
215-
$_FILES = [
213+
$_FILES = [
216214
'userfile1' => [
217215
'name' => $finalFilename . '.txt',
218216
'type' => 'text/plain',
@@ -227,7 +225,6 @@ public function testAlreadyMoved()
227225
$this->assertTrue($collection->hasFile('userfile1'));
228226

229227
$destination = $this->destination;
230-
231228
// Create the destination if not exists
232229
if (! is_dir($destination)) {
233230
mkdir($destination, 0777, true);
@@ -254,15 +251,15 @@ public function testInvalidFile()
254251
];
255252

256253
$destination = $this->destination;
257-
258-
$collection = new FileCollection();
259-
$file = $collection->getFile('userfile');
254+
$collection = new FileCollection();
260255

261256
$this->expectException(HTTPException::class);
257+
258+
$file = $collection->getFile('userfile');
262259
$file->move($destination, $file->getName(), false);
263260
}
264261

265-
public function testFailedMove()
262+
public function testFailedMoveBecauseOfWarning()
266263
{
267264
$_FILES = [
268265
'userfile' => [
@@ -275,16 +272,47 @@ public function testFailedMove()
275272
];
276273

277274
$destination = $this->destination;
278-
279275
// Create the destination and make it read only
280276
if (! is_dir($destination)) {
281277
mkdir($destination, 0400, true);
282278
}
283279

284280
$collection = new FileCollection();
285-
$file = $collection->getFile('userfile');
286281

287282
$this->expectException(HTTPException::class);
283+
284+
$file = $collection->getFile('userfile');
285+
$file->move($destination, $file->getName(), false);
286+
}
287+
288+
public function testFailedMoveBecauseOfFalseReturned()
289+
{
290+
$_FILES = [
291+
'userfile1' => [
292+
'name' => 'fileA.txt',
293+
'type' => 'text/plain',
294+
'size' => 124,
295+
'tmp_name' => '/tmp/fileA.txt',
296+
'error' => 0,
297+
],
298+
];
299+
300+
$collection = new FileCollection();
301+
302+
$this->assertTrue($collection->hasFile('userfile1'));
303+
304+
$destination = $this->destination;
305+
// Create the destination if not exists
306+
if (! is_dir($destination)) {
307+
mkdir($destination, 0777, true);
308+
}
309+
// Set the mock's return value to false
310+
move_uploaded_file('', '', false);
311+
312+
$this->expectException(HTTPException::class);
313+
$this->expectExceptionMessage('move_uploaded_file() returned false');
314+
315+
$file = $collection->getFile('userfile1');
288316
$file->move($destination, $file->getName(), false);
289317
}
290318
}
@@ -311,10 +339,20 @@ function is_uploaded_file($filename)
311339
* This overwrite is for testing the move operation.
312340
*/
313341

314-
function move_uploaded_file($filename, $destination)
342+
function move_uploaded_file($filename, $destination, ?bool $setReturnValue = null)
315343
{
344+
static $return = true;
345+
346+
if ($setReturnValue !== null) {
347+
$return = $setReturnValue;
348+
349+
return true;
350+
}
351+
316352
copy($filename, $destination);
317353
unlink($filename);
354+
355+
return $return;
318356
}
319357

320358
function rrmdir($src)

0 commit comments

Comments
 (0)