PR #660 breaks POST requests in app with Plack::Middleware::Static

[dboehmer]

I am working on a Plack app using Plack::Middleware::Static and a coderef for path. Since #660 all POST requests fail because Plack::App::File returns 405 for non GET/HEAD requests regardless of the return value of the path coderef.

I think, the check for the HTTP method should be done only after path returned a true value. At the same time maybe we should another part of Plack to do the URL handling?

Our app has static files and dynamic endpoints mixed in the same namespace. Unfortunately the URL namespace was designed with a different scheme. I chose to use Plack::Middleware::Static to do the URL handling and handle static files.

The code looks like this:

builder {
    enable 'Plack::Middleware::Static', (
        path => sub {
            # manipulate path to match local files
            s/foo/bar/;

            if ( m/some paths must not be used as local files/ ) {
                return;
            }

            return 1; # serve local file or return 404
        },
        root => File::Spec->catdir( $PROJECT_ROOT, 'html' ),
    );

    # handlers for dynamic reqests ...
}

This is not the nicest thing but this is part of a longer migration from CGI to a modern web framework.

[robrwo]

See #682. Fixed with #681

[miyagawa]

Thanks for the report. Reverted the change in #684 and shipped as 1.0050.

[miyagawa]

Since #660 all POST requests fail because Plack::App::File returns 405 for non GET/HEAD requests regardless of the return value of the path coderef.

Is this accurate? the middleware still checks for the return value of path and if it was false, 405 wouldn't have been returned since the App::File doesn't run.

Did you have pass_through option enabled for the middleware, and only got 405 when path matched the request but the file didn't exist?

[dboehmer]

Since #660 all POST requests fail because Plack::App::File returns 405 for non GET/HEAD requests regardless of the return value of the path coderef.

Is this accurate? the middleware still checks for the return value of path and if it was false, 405 wouldn't have been returned since the App::File doesn't run.

Did you have pass_through option enabled for the middleware, and only got 405 when path matched the request but the file didn't exist?

Yes, I was wrong. I didn’t know about pass_through and our path coderef was a bit too complex. I enabled pass_through which simplified the code a lot. It’s now just:

    enable 'Plack::Middleware::Static', (
        path => sub {
            s!/$!/index.html!;
            return 1;
        },
        pass_through => 1,
        root         => File::Spec->catdir( $PROJECT_ROOT, 'html' ),
    );

• v1.0049 returns 405 for a POST request to a URL path which does not exist as a file but is handled by a mounted PSGI app.
• v1.0050 works just fine.

I feel like there could be a setting to enable serving of index files automatically.

[miyagawa]

ok, that makes sense, since pass_through was broken in v1.0049.

What I was curious was that your original code didn't include pass_through (right?) and still was broken in v1.0049? Still unclear how that happened.

[dboehmer]

What I was curious was that your original code didn't include pass_through (right?)

Correct. I carefully wrote path to return false for any paths that are handled by the other PSGI app parts.

and still was broken in v1.0049?

Correct, as stated above.

[miyagawa]

Can you reproduce the error? In v1.0049, if the path returned false, App::File shouldn't run so you're not expected to see 405 from these paths.

[miyagawa]

i can't reproduce the error with 1.0049:

➜  cat cpanfile
requires 'Plack',  '== 1.0049';

➜  carmel exec plackup -e 'use Plack; warn $Plack::VERSION; enable "Static", path => sub { 0 }; sub { [200, [], ["Hi"]] }' -p 5001
1.0049 at (eval 9) line 1.
HTTP::Server::PSGI: Accepting connections at http://0:5001/

➜  curl 0:5001
Hi

➜  curl -XPOST 0:5001
Hi

if I return 1 from the path callback, or add pass_through i get 405 for POST, as expected:

➜  carmel exec plackup -e 'use Plack; warn $Plack::VERSION; enable "Static", path => sub { 1 }; sub { [200, [], ["Hi"]] }' -p 5001       
1.0049 at (eval 9) line 1.
HTTP::Server::PSGI: Accepting connections at http://0:5001/

➜  curl 0:5001    
not found                                                                                                              

➜  curl -XPOST 0:5001
Method Not Allowed

➜  carmel exec plackup -e 'use Plack; warn $Plack::VERSION; enable "Static", path => sub { 1 }, pass_through => 1; sub { [200, [], ["Hi"]] }' -p 5001
1.0049 at (eval 9) line 1.
HTTP::Server::PSGI: Accepting connections at http://0:5001/

➜  curl 0:5001
Hi

➜  curl -XPOST 0:5001
Method Not Allowed

[robrwo]

In #681 I had a PR that should have fixed 1.0049 by moving the method check until after the location checl. The PR also has a test that a POST passthrough worked.

[miyagawa]

@robrwo yes, but @dboehmer is reporting that it was broken without the use of pass_through, that's what I'm trying to figure out.

[robrwo]

Ah, sorry. I just assumed the example was a cut-and-paste error.

[miyagawa]

@dboehmer can you go back to 1.0049 and your original code and reproduce the error? Your original statement:

Since #660 all POST requests fail because Plack::App::File returns 405 for non GET/HEAD requests regardless of the return value of the path coderef.

You're saying that you were not using pass_through, and all POST requests fail even when path was returning false.

I can't see how that's happening, and I can't reproduce that (see above). It would be nice if you can reproduce it so we can investigate further.