Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[🐛 BUG]: X-Sendfile not working as expected #1386

Closed
1 task done
tux-rampage opened this issue Dec 7, 2022 · 3 comments · Fixed by roadrunner-server/send#36
Closed
1 task done

[🐛 BUG]: X-Sendfile not working as expected #1386

tux-rampage opened this issue Dec 7, 2022 · 3 comments · Fixed by roadrunner-server/send#36
Assignees
Labels
A-plugin Area: module B-bug Bug: bug, exception B-regression Bug: regression bugs
Milestone

Comments

@tux-rampage
Copy link

tux-rampage commented Dec 7, 2022

No duplicates 🥲.

  • I have searched for a similar issue in our bug tracker and didn't find any solutions.

What happened?

The advertised feature does not work as it is used in frameworks like Symfony.

The following worker should cause the webserver (roadrunner) to stream the file /some/local/file/to/be/sent.mp4 from the given location in the X-Sendfile response header.

<?php

use Spiral\RoadRunner;
use Nyholm\Psr7;

include "vendor/autoload.php";

$worker = RoadRunner\Worker::create();
$psrFactory = new Psr7\Factory\Psr17Factory();

$psr7 = new RoadRunner\Http\PSR7Worker($worker, $psrFactory, $psrFactory, $psrFactory);

while (true) {
    try {
        $request = $psr7->waitRequest();

        if (!($request instanceof \Psr\Http\Message\ServerRequestInterface)) { // Termination request received
            break;
        }
    } catch (\Throwable) {
        $psr7->respond(new Psr7\Response(400)); // Bad Request
        continue;
    }

    try {
        $fileToSend = '/some/local/file/to/be/sent.mp4'; // Make sure this actually exists and it is readable by the webserver
        $response = new Psr7\Response(200);

        $psr7->respond(
            $response->withHeader('X-Sendfile', $fileToSend),
        );
    } catch (\Throwable) {
        $psr7->respond(new Psr7\Response(500, [], 'Something Went Wrong!'));
    }
}

With the PSR response created in the above code, apache's mod_xsendfile would remove the X-Sendfile header and the potential response body from the php response and stream the local file that is referenced by this header to the client.

Roadrunner just passes the (empty) response as is, including the X-Sendfile header.

As far as I checked the YouTube video to this feature and the go code - and correct me when I'm wrong here, it seems that Roadrunner expects X-Sendfile to be passed by the user agent as request header which is not what frameworks like Symfony expect and it is not how X-Sendfile (or X-Accel-Redirect for nginx) works. Additionally this introduces a Security issue by making all files in the current working directory accessible.

Version (rr --version)

rr version 2.12.1 (build time: 2022-12-01T12:41:56+0000, go1.19.3), OS: linux, arch: amd64

How to reproduce the issue?

Configure Roadrunner as described in the docs: https://roadrunner.dev/docs/middleware-sendfile/2.x/en
And use the worker code above. Make sure the File used for X-Sendfile in PHP exists and is readable.

Relevant log output

No response

@rustatian
Copy link
Member

rustatian commented Dec 7, 2022

Hey @tux-rampage 👋🏻

Thanks for the contribution 👍🏻
I'm unfamiliar with PHP and Symfony (any PHP framework), but I'm open to proposals. As far as I read in the docs, I guessed that feature works like the same in Symfony, but...

Additionally this introduces a Security issue by making all files in the current working directory accessible.

RR is not a web-server; it's an application server. It should not be accessible from the internet, thus technically, you can't access the local files, only paths filtered by the LB/web-server/etc.

It would be nice if you share some docs about how this feature should work; it might also be your proposals on how you see this feature. Thanks ⚡

@tux-rampage
Copy link
Author

Hi @rustatian

thanks for your reply. I really like the idea behind roadrunner and in fact I've been looking for similar solutions within the PHP ecosystem. This one here seems the most promising.

I'm sure you already aware of the issue, just to make sure we have a common understanding of the topic and please correct me for the parts where I may be misguided or plainly wrong:

PHP has a huge disadvantage when it comes to large data transfers (i.e. applications that cause a lot of or slow i/o): It is Blocking by default. The problem is also outlined quite well in #923 - The worker cannot be re-used while it is not only processing the request (as by occupying CPU cycles) but also while sending the response. For small chunks of data this mostly unnoticed, but it becomes a limiting factor for larger or slow transfers (large responses, db i/o with high latency, etc...).

This can only be fully addressed by using an async framework like Amp or ReactPHP. But this would kick PSR-7 and especially PSR-15 off the table, since both are designed for the traditional blocking nature of PHP.

This whole Blocking-Issue is mitigated by various strategies of the existing SAPIs (Apache mod_php or pfp-fpm) like forking new processes for each request when required.

X-Sendfile is another Strategy here. Instead of opening a large file in php and streaming it to the http frontend which would block the whole Tread in the meantime, the PHP process Responds with this header and delegates sending this (Local!) File to the Application server which makes the Thread available for the next request. X-Sendfile is not a php framework specific implementation, but a http server strategy used by some.

Nginx has a similar option which is called X-Accel-Redirect. Instead of a Local File-Path, nginx requires this header to reference a resource of a internal location config, which would not only allow delegating local files, but remote resources as well (like files from an Amazon S3, Google Cloud Storage or the like).

The use case for "Why would you open/send the file in php and not address it directly?" is mostly the following: The file should be guarded by the php application and only be sent to the user/client when certain conditions of a business logic are met. That could be a check if the user is authenticated, has proper privileges, has provided a required payment, etc ...

So the traditional flow of the X-Sendfile feature is as follows:

  1. HTTP-Request is delegated to PHP
  2. PHP processed the request and sends a response that meets the following criteria
    • There is a `X-Sendfile' Response-Header
    • The response body is empty (optional)
  3. The http server intercepts the response in the following way:
    • The local file path will be read from the X-Sendfile response header (addressed as localfile here)
    • The X-Sendfile header is removed before sending the response headers to the client
    • The http server will open localfile and stream its content to the client after sending the response headers
    • The http server may remove or replace Content-Length or Transfer-Encoding response headers as needed
    • The http server may ignore or respect Content-Length or Transfer-Encoding response headers from the PHP response when streaming the contents of localfile
    • The http server may apply an existing Range request header and stream localfile accordingly. Necessary Response headers should be added.
    • If there is any response body from PHP it is discarded

Is this even possible with the current HTTP implementation of roadrunner? This is also just a compatibility feature to mitigate effects. #923 - as far as I understood - has way better approaches to handle these concerns and maybe X-Sendfile could also be implemented this way.

@rustatian
Copy link
Member

rustatian commented Dec 8, 2022

Thanks for such a detailed reply 👍🏻
I thought that some outside system should send the X-Sendfile, but now I understand your worries 😃

This definitely should be changed to work in an 'expected' way.

Is this even possible with the current HTTP implementation of roadrunner?

Yeah, sure.

#923 is about streaming directly from the worker (in the case of the gRPC in/out streams). The PHP Worker will be blocked during the stream.

I'll put this bug in the v2.12.2 milestone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-plugin Area: module B-bug Bug: bug, exception B-regression Bug: regression bugs
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants