plack / Plack

PSGI toolkit and server adapters
http://plackperl.org/
Other
486 stars 214 forks source link

Plack::App::File prunes trailing slashes via split() invocation #405

Closed avar closed 9 years ago

avar commented 11 years ago

I had to deal with a security issue/bug that arose due to an interaction with Plack::App::File which was basically:

  1. I'd do a regex /.txt.pl\z/ on a file to see if it was a text file
  2. If so, do magic to generate text file via perl
  3. Else it's not a /.txt.pl\z/ file, so it must be some other static file with a different extension
  4. Serve it up with Plack::Middleware::Static

The problem is that if you request:

/file.txt.pl/

You'll get the file at:

/file.txt.pl

But without it having gone throug step #2, it just gets served up as a plain file!

The reason for this is that Plack::App::File does a split on "/" without having a third -1 argument. So e.g. this:

$ perl -MData::Dumper -wle 'print Dumper [split /[\\\/]/, shift]' a/file.txt
$VAR1 = [ 
          'a',
          'file.txt'
        ];

Is the same as:

$ perl -MData::Dumper -wle 'print Dumper [split /[\\\/]/, shift]' a/file.txt///
$VAR1 = [
          'a',
          'file.txt'
        ];

As opposed to:

$ perl -MData::Dumper -wle 'print Dumper [split /[\\\/]/, shift, -1]' a/file.txt///
$VAR1 = [
          'a',
          'file.txt',
          '',
          '',
          ''
        ];

I don't think the magical split behavior of ignoring empty trailing fields has any place in the implicit Plack::App::File API. But opinions may differ, so filing this bug.

Note the difference in behavior v.s. standard *nix utilities:

$ touch /tmp/foo.txt
$ file /tmp/foo.txt
/tmp/foo.txt: empty
$ file /tmp/foo.txt/
/tmp/foo.txt/: ERROR: cannot open `/tmp/foo.txt/' (Not a directory)

If touch behaved like Plack::App::File it would happily ignore the trailing slash.

miyagawa commented 11 years ago

Good find - can you make a unit test that represents the bug?

sander85 commented 9 years ago

Hi!

Can someone explain me how to test this fix? I have 1.0031 installed but when I run

$ perl -MData::Dumper -wle 'print Dumper [split /[\\\/]/, shift, -1]' a/file.txt///

then the output is still this:

$VAR1 = [
          'a',
          'file.txt'
        ];

What am I missing?

avar commented 9 years ago

That's not what that command outputs, it emits this:

$ perl -MData::Dumper -wle 'print Dumper [split /[\\\/]/, shift, -1]' a/file.txt///
$VAR1 = [
      'a',
      'file.txt',
      '',
      '',
      ''
    ];

What shell/OS are you running this on, you should be getting something like this in a unix shell:

$ perl -MO=Deparse -MData::Dumper -wle 'print Dumper [split /[\\\/]/, shift, -1]' a/file.txt///
BEGIN { $^W = 1; }
BEGIN { $/ = "\n"; $\ = "\n"; }
use Data::Dumper;
print Dumper([split(m[[\\/]], shift @ARGV, -1)]);
-e syntax OK
sander85 commented 9 years ago

Oh, my bad. I used the wrong command :/ Sorry about that!

But is there any POC for this fix? I mean, how can QA test that the issue is fixed? :)

miyagawa commented 9 years ago

The verification for fix is in the test: https://github.com/plack/Plack/pull/446/files#diff-2