roadrunner-server / roadrunner

🤯 High-performance PHP application server, process manager written in Go and powered with plugins
https://docs.roadrunner.dev
MIT License
7.92k stars 411 forks source link

function call is_uploaded_file is not valid #133

Closed see0 closed 5 years ago

see0 commented 5 years ago

Symphony and Laravel UploadedFile class consider all uploded as invalid due to the is_uploaded_file check.

vendor/symfony/http-foundation/File/UploadedFile.php

public function isValid()
{
    $isOk = UPLOAD_ERR_OK === $this->error;

    return $this->test ? $isOk : $isOk && is_uploaded_file($this->getPathname());
}

Swoole solution: https://github.com/swoole/swoole-src/pull/407

My current workaround - I subclassed both HttpFoundationFactory, UploadedFile to get around this issue.

Let me know if this issue is going to be solved here (upstream) or should I send a pull request for my workaround to https://github.com/UPDG/roadrunner-laravel.

Thanks

wolfy-j commented 5 years ago

@Alex-Bond i guess this issue is for you.

Alex-Bond commented 5 years ago

@see0 hey! Can you create PR with some use cases? At this moment I don't really understand where the problem is.

see0 commented 5 years ago

@see0 hey! Can you create PR with some use cases? At this moment I don't really understand where the problem is.

Hmm, i'm surprised that I am the only one facing this issue.

The problem is caused by all (JSON) Data are relayed thru goridge, and then restored to PSR-7 Message in the worker.

Hence, PHP is unable to add all the uploaded_files into their rfc1867_uploaded_files hash table - any function call to is_uploaded_file will return a FALSE result.

When you try to perform any File Validation in Laravel/Symphony Request - the result will always be invalid.

Well, I supposed is_uploaded_file is a php core function used in "olden/PHP 4" days to validate the file is uploaded. It's really an edge case, since solving it will only makes it easier to adopt other frameworks.

My take is:

  1. Don't solve it but document the caveat in the wiki and let individual integration handles them.
  2. (Optionally - not exactly useful) Establish a signing token between the RoadRunner Server and client - the relayed data is trustable.

I'm planning to clean up my Laravel integration (with all the jingles - https://github.com/spiral/roadrunner/issues/103) and open source them in the coming weekend.

wolfy-j commented 5 years ago

I will describe in the documentation for now.

wolfy-j commented 5 years ago

https://github.com/spiral/roadrunner/wiki/Caveats

wolfy-j commented 5 years ago

I think the reason on not meeting this issue earlier is that most of the people rely on PSR-7 UploadedFileInterface which manage uploaded state separately. This issue seems to be Laravel/Symfony specific while mapping data to Request. Doesn't Symfony HttpKernel support file uploads set manually?

hotrush commented 5 years ago

@see0 any luck on your code release?)

tarampampam commented 5 years ago

We (@jetexe) made fix for this case, (changes, working release).

diolan12 commented 2 years ago

I fix this in PHP by copy and then unlink the temp file. in my case, the temp file is in the C: drive meanwhile my project want to move it in D: drive. I remembered that os.Rename doesn't move file to a different drive, so I fix it by copy then unlink, os.Rename is not the case but this works great.

egonbraun commented 2 years ago

I am getting this issue when uploading files to my Laravel 9 and PHP 8.1 based backend running Roadrunner (latest version):

2022-10-06T11:20:49.766Z    INFO    server          In Psr17Factory.php line 49:
2022-10-06T11:20:49.767Z    INFO    server            The file "/tmp/upload607333973" cannot be opened:
2022-10-06T11:20:49.767Z    INFO    server          start [--laravel-path [LARAVEL-PATH]] [--relay-dsn RELAY-DSN] [--refresh-app] [--worker-mode WORKER-MODE]

Does anyone have a workaround?

rustatian commented 2 years ago

RR manages uploads itself, and I guess Laraver interrupts this process and deletes the temporary file.

rustatian commented 2 years ago

@egonbraun Do you use octane or this integration: https://github.com/spiral/roadrunner-laravel?

egonbraun commented 2 years ago

I am using the latter. The spiral/roadrunner-laravel integration.

egonbraun commented 2 years ago

RR manages uploads itself, and I guess Laraver interrupts this process and deletes the temporary file.

Looks like it. Are you aware of any way to change this behaviour? I know you are not a PHP or Laravel specialist but I could definitely use some ideas.

rustatian commented 2 years ago

We may introduce a special flag, but, you may have a look at this ticket: https://github.com/spiral/roadrunner-laravel/issues/84, I guess it was precisely your problem.

egonbraun commented 2 years ago

At least he was able to get the files, I am not even reaching the controller at this point. Seems like the issue is happening while the request object is being created.

The class that fails is part of this library:

vendor/nyholm/psr7/src/Factory/Psr17Factory.php

So it looks more like what @see0 was describing here:

The problem is caused by all (JSON) Data are relayed thru goridge, and then restored to PSR-7 Message in the worker.

Hence, PHP is unable to add all the uploaded_files into their rfc1867_uploaded_files hash table - any function call to > is_uploaded_file will return a FALSE result.

When you try to perform any File Validation in Laravel/Symphony Request - the result will always be invalid.

rustatian commented 2 years ago

To say in truth, we don't actively support this integration. You may try to use a SpiralFramework, which has an official RR integration. Laravel/Octane has very few features of the RR (like ~10%).

elijahchancey commented 2 years ago

To be clear, we're using Spiral + Roadrunner, and not Octane + Roadrunner.

Is there a suggested way of handling file uploads with Laravel9+Php8.1?

Thanks!

wolfy-j commented 2 years ago

Maybe @butschster can help.

elijahchancey commented 2 years ago

It looks like https://github.com/avto-dev/roadrunner-laravel/pull/12/files might fix it? Perhaps the ability to set --not-fix-symfony-file-validation would fix the issue?

Or maybe we're processing file uploads in some deprecated way the relies on is_uploaded_file? Originally, our app was written on Laravel 5 and it has slowly evolved into a behemoth on Laravel 9.

elijahchancey commented 2 years ago

Not sure if this is helpful or not, but this is the error we see:

dev_irl                      | 2022-10-06T15:35:18.412Z DEBUG   server          worker is allocated {"pid": 13716, "internal_event_name": "EventWorkerConstruct"}
dev_irl                      | 2022-10-06T15:35:18.414Z INFO    server
dev_irl                      | 2022-10-06T15:35:18.416Z INFO    server          In Psr17Factory.php line 49:
dev_irl                      |   The file "/tmp/upload3578791433" cannot be opened:
dev_irl                      | start [--laravel-path [LARAVEL-PATH]] [--relay-dsn RELAY-DSN] [--refresh-app] [--worker-mode WORKER-MODE]
dev_irl                      | 2022-10-06T15:35:18.419Z ERROR   http            execute {"start": "2022-10-06T15:35:18.158Z", "elapsed": "260.795709ms", "error": "sync_worker_exec_worker_with_timeout: Network:\n\tsync_worker_exec_payload: EOF"}
dev_irl                      | 2022-10-06T15:35:18.419Z INFO    http            http log    {"status": 500, "method": "POST", "URI": "/2.0/user/update", "remote_address": "127.0.0.1:43024", "read_bytes": 360038, "write_bytes": 0, "start": "2022-10-06T15:35:18.158Z", "elapsed": "261.082333ms"}
dev_irl                      | nginx access_log 172.18.0.1 "POST /2.0/user/update HTTP/1.1" 500 "curl/7.79.1" 0 0.263

We're using Intervention/image; but we think this error will occur with any uploaded file.

elijahchancey commented 2 years ago

Also, files don't appear in /tmp; they never seem to be written. This is the permission on /tmp: drwxrwxrwt

elijahchancey commented 2 years ago

Sorry about flooding this issue with comments; we were just VERY excited to move from FPM to Roadrunner. It showed a 22% reduction in overall latency and a 45% reduction of CPU compared to FPM.

rustatian commented 2 years ago

NP at all, this is not a flood. But it would be better to move the discussion to a separate ticket. Could you please create a new ticket and copy-paste the discussion above?

elijahchancey commented 2 years ago

Will do!