framework: Request allFiles() Method Returns Wrong Data After Modification

Laravel Version: 5.7.19
PHP Version: 7.2
Database Driver & Version: MySql 5.7

Description:

The InteractsWithInput.php Concern will return the wrong files array if a user has at any point modified the files of the request.

I’m fine submitting a PR for this, I wanted to check first if this is considered a bug.

Steps To Reproduce:

Access a controller’s filled Request param that has one or more files:

public function saveModel(Request $request)

Modify the FileBag by calling the replace method

// Modify files in whichever way you wish; remove, add, or edit
$newFiles = $request->allFiles();
$newFiles = $files->forget('some-key')

// Now call the replace method
$problemRequest = (clone $request)->files->replace($newFiles);

Observe that the files array is wrong when calling allFiles()

dd($problemRequest->allFiles());

Steps To Reproduce: Why It Happens

Here’s the method code: https://github.com/illuminate/http/blob/master/Concerns/InteractsWithInput.php#L308

 public function allFiles()
    {
        $files = $this->files->all();
        return $this->convertedFiles
                    ? $this->convertedFiles
                    : $this->convertedFiles = $this->convertUploadedFiles($files);
    }

This method will return the cached convertedFiles property instead of the actual updated files (as would be correct with the $files variable there).

Happy to submit a PR on a preferred solution. Cheers.

About this issue

Original URL
State: closed
Created 5 years ago
Comments: 15 (10 by maintainers)

Most upvoted comments

@zschuessler I finally managed to reproduce this with you code snippet and I think I know what’s going wrong. I’ll send in a PR.

driesvints on Oct 11, 2019

Actually, the actual problem is something different. You don’t even need to clone the object to reproduce this:

    // If we call this here, files get cached and causes problems
    $request->allFiles();

    // Original request has a single file by key `original_uploaded_file`
    $requestFile = $request->files->get('original_uploaded_file');

    $request->files->add([
        'modified_request_file' => $requestFile
    ]);

    // Here, $modifiedRequest correctly shows two keys: the original, and the one we just added
    dump($request->files->all());

    // Here, the `allFiles` method shows the original cached array only, with key `original_uploaded_file`
    dd($request->allFiles());

The thing is that the caching happens and then the underlying $files property is modified but because of the caching it can’t be changed anymore. The caching is needed because converting the files property to UploadedFile instances can be quite intensive if that’s done several times during a request.

I’ve also just realized that you’re not meant to manipulate the request in this way as the files property is more like a read-only property. What I suggest you do is that you pull the files from it in a separate self-owned variable or object and then add any files you want there.

So I’m sorry to say but this will be a no-fix.

driesvints on Oct 11, 2019

Duplicated this, so glad after googling someone else had ran into this. In our instance, we developed a trait to convert base64 image strings into an actual image. Was a hacky fix to prevent breaking validation for old/new clients.

So after we did the conversion, we would shove that new image into the files array using.

        $this->files->add($fileArray);
        $this->merge($fileArray);

Not helpful without full context, but $fileArray is just a bunch of UploadedFiles, indexed by the same key that the posted base64 image string was.

Why this became an issue is beyond me. The only change to the area was refactoring to a null-coalescing operator - https://github.com/laravel/framework/commit/7a7cacc3fd02d7d4c596c1a0f8af3c5b7a2990b4

After more digging though - I think we need to revert it. It is not the same check. The first one is a “truthy” check and 2nd is a “emptyness” check.

iBotPeaches on Jul 25, 2019