Nyholm / psr7

A super lightweight PSR-7 implementation
MIT License
1.16k stars 75 forks source link

Empty file upload gives fatal error in PHP 8.0 #175

Closed fisharebest closed 3 years ago

fisharebest commented 3 years ago

I have an HTML form with an optional <input type="file"> field.

There is no problem with PHP 7.1-7.4, but with PHP 8.0 I get the following error.

Uncaught ValueError: Path cannot be empty in /Users/gr4376/Projects/webtrees/vendor/nyholm/psr7/src/Factory/Psr17Factory.php:40
Stack trace:
#0 .../vendor/nyholm/psr7/src/Factory/Psr17Factory.php(40): fopen('', 'r')
#1 .../vendor/nyholm/psr7-server/src/ServerRequestCreator.php(216): Nyholm\Psr7\Factory\Psr17Factory-&gt;createStreamFromFile('')
#2 .../vendor/nyholm/psr7-server/src/ServerRequestCreator.php(188): Nyholm\Psr7Server\ServerRequestCreator-&gt;createUploadedFileFromSpec(Array)
#3 .../vendor/nyholm/psr7-server/src/ServerRequestCreator.php(98): Nyholm\Psr7Server\ServerRequestCreator-&gt;normalizeFiles(Array)
#4 .../vendor/nyholm/psr7-server/src/ServerRequestCreator.php(71): Nyholm\Psr7Server\ServerRequestCreator-&gt;fromArrays(Array, Array, Array, Array, Array, Array, Resource id #7)
#5 .../index.php(55): Nyholm\Psr7Server\ServerRequestCreator->fromGlobals()

This is because fopen('') gives a warning in PHP <=7.4, but a fatal error in PHP 8.0.

See https://3v4l.org/cgYav

fisharebest commented 3 years ago

I guess we should throw a ValueError when the filename is empty...

https://github.com/http-interop/http-factory-tests/blob/master/test/StreamFactoryTestCase.php#L95

Nyholm commented 3 years ago

Thank you for this issue. It will be fixed by #174

frkinta commented 3 years ago

Execution is stopped in slim4 with default httpErrorHandler and php7.4

{ "statusCode": 500, "error": { "type": "SERVER_ERROR", "description": "ERROR: fopen(): Filename cannot be empty on line 40 in file \/var\/www\/html\/tratra-api\/vendor\/nyholm\/psr7\/src\/Factory\/Psr17Factory.php." } }

Nyholm commented 3 years ago

Thank you.

Could you confirm that #174 will fix the issue?

frkinta commented 3 years ago

No

{ "statusCode": 500, "error": { "type": "SERVER_ERROR", "description": "ERROR: fopen(): Filename cannot be empty on line 100 in file \/var\/www\/html\/tratra-api\/vendor\/nyholm\/psr7\/src\/Stream.php." } }

frkinta commented 3 years ago

Seems like $_FILES is not really empty. A earlier var_dump on it shows array(1) { ["file"]=> array(5) { ["name"]=> string(0) "" ["type"]=> string(0) "" ["tmp_name"]=> string(0) "" ["error"]=> int(4) ["size"]=> int(0) } } With error code 4 Value: 4; No file was uploaded. PHPDOC.

The fault is not to fopen but to conditions here https://github.com/Nyholm/psr7-server/blob/7542bd761f79b4e51121f8c79566a4c358d77a9b/src/ServerRequestCreator.php#L180-L197

Zegnat commented 3 years ago

I haven’t found a way to reproduce this error exactly yet. I wondered if it was a problem with psr7-server instead, and wrote a test there user your $_FILES array that I have attached below. This test did seem to break for me locally (using PHP 8.0) but it fixed itself as soon as I switched psr7 to use the branch from #174.

I can’t test PHP 7.4 right now, but I’ll see if I have more luck there later. So far I have only been able to confirm that the problem goes away when we merge the PR. @frkinta did you test against the branch from #174? Might you be able to create a breaking PHPUnit test for us to look at?

    public function testFromEmptyFiles()
    {
        $files = [
            'file' => [
                'name' => '',
                'type' => '',
                'tmp_name' => '',
                'error' => UPLOAD_ERR_NO_FILE,
                'size' => 0,
            ],
        ];

        $server = [
            'SERVER_PROTOCOL' => 'HTTP/1.0',
            'REQUEST_METHOD' => 'POST',
        ];

        $server = $this->creator->fromArrays($server, [], [], [], [], $files, 'foobar');

        $server->getUploadedFiles();
    }
frkinta commented 3 years ago

The code actually, in the better case, only ensure $value["tmp_name"] is set and ALWAYS pass its content to fopen(), empty or not. I think it is a good practice to ensure file was successfully uploaded before ... creating stream from it.

We may change this

https://github.com/Nyholm/psr7-server/blob/7542bd761f79b4e51121f8c79566a4c358d77a9b/src/ServerRequestCreator.php#L215-L219

to this

if ($value['error'] === UPLOAD_ERR_OK) {
    $stream = $this->streamFactory->createStreamFromFile($value['tmp_name']);
} else {
    $stream = $this->streamFactory->createStream();
}

I'll send a pull request later

Nyholm commented 3 years ago

PR https://github.com/Nyholm/psr7-server/pull/43 was merged and will be released in 1.0.2.

@fisharebest will this fix the issue?

fisharebest commented 3 years ago

Yes - this fixed the issue for me. Thanks!

Nyholm commented 3 years ago

Awesome!

I'll release it today or tomorrow.

yirutabyose commented 2 years ago

Verify your code