merged from master
John Napiorkowski [Tue, 25 Nov 2014 19:44:32 +0000 (13:44 -0600)]
1  2 
Changes
lib/Catalyst.pm
lib/Catalyst/Engine.pm
lib/Catalyst/Request.pm

diff --combined Changes
+++ b/Changes
@@@ -1,24 -1,24 +1,43 @@@
  # This file documents the revision history for Perl extension Catalyst.
  
 +5.90080_001 - TBD
 +  - MyApp->to_app is now an alias for MyApp->psgi_app in order to better support
 +    existing Plack conventions.
 +  - Modify Catayst::Response->from_psgi_response to allow the first argument to
 +    be an object that does ->as_psgi.
 +  - Modified Catayst::Middleware::Stash to be a shallow copy in $env.  Added some
 +    docs.  Added a test case to make sure stash keys added in a child application
 +    don't bubble back up to the main application.
 +  - We no longer use Encode::is_utf8 since it doesn't work the way we think it
 +    does... This required some UTF-8 changes.  If your application is UTF-8 aware
 +    I highly suggest you test this release.
 +  - We alway do utf8 decoding on incoming URLs (before we only did so if the server
 +    encoding was utf8.  I believe this is correct as per the w3c spec, but please
 +    correct if incorrect :)
 +  - Debug output now shows utf8 characters if those are incoming via Args or as
 +    path or pathparts in your actions.  query and body parameter keys are now also
 +    subject to utf8 decoding (or as specificed via the encoding configuration value).
 +  - lots of UTF8 changes.  Again we think this is now more correct but please test.
 +
+ 5.90077 - 2014-11-18
+   - We store the PSGI $env in Catalyst::Engine for backcompat reasons.  Changed
+     this so that the storage is a weak reference, so that it goes out of scope
+     with the request.  This solves an issue where items in the stash (now in the
+     PSGI env) would not get closed at the end of the request.  This caused some
+     regression, primarily in custom testing classes.
+ 5.90076 - 2014-11-13
+   - If throwing an exception object that does the code method, make sure that
+     method returns an expected HTTP status code before passing it on to the
+     HTTP Exception middleware.
+ 5.90075 - 2014-10-06
+   - Documentation patch for $c->req->param to point out the recently discovered
+     potential security issues: http://blog.gerv.net/2014/10/new-class-of-vulnerability-in-perl-web-applications/
+   - You don't need to install this update, but you should read about the exploit
+     and review if your code is vulnerable.  If you use the $c->req->param interface
+     you really need to review this exploit.
  5.90074 - 2014-10-01
    - Specify Carp minimum version to avoid pointless test fails (valy++)
  
diff --combined lib/Catalyst.pm
@@@ -50,7 -50,7 +50,7 @@@ use Plack::Middleware::RemoveRedundantB
  use Catalyst::Middleware::Stash;
  use Plack::Util;
  use Class::Load 'load_class';
 -use Encode 2.21 ();
 +use Encode 2.21 'decode_utf8', 'encode_utf8';
  
  BEGIN { require 5.008003; }
  
@@@ -127,8 -127,7 +127,8 @@@ __PACKAGE__->stats_class('Catalyst::Sta
  __PACKAGE__->_encode_check(Encode::FB_CROAK | Encode::LEAVE_SRC);
  
  # Remember to update this in Catalyst::Runtime as well!
 -our $VERSION = '5.90077';
 +our $VERSION = '5.90080_001';
 +$VERSION = eval $VERSION if $VERSION =~ /_/; # numify for warning-free dev releases
  
  sub import {
      my ( $class, @arguments ) = @_;
@@@ -496,18 -495,6 +496,18 @@@ Catalyst)
      # stash is automatically passed to the view for use in a template
      $c->forward( 'MyApp::View::TT' );
  
 +The stash hash is currently stored in the PSGI C<$env> and is managed by
 +L<Catalyst::Middleware::Stash>.  Since it's part of the C<$env> items in
 +the stash can be accessed in sub applications mounted under your main
 +L<Catalyst> application.  For example if you delegate the response of an
 +action to another L<Catalyst> application, that sub application will have
 +access to all the stash keys of the main one, and if can of course add
 +more keys of its own.  However those new keys will not 'bubble' back up
 +to the main application.
 +
 +For more information the best thing to do is to review the test case:
 +t/middleware-stash.t in the distribution /t directory.
 +
  =cut
  
  sub stash {
@@@ -1389,38 -1376,30 +1389,38 @@@ sub uri_for 
        ( scalar @args && ref $args[$#args] eq 'HASH' ? pop @args : {} );
  
      carp "uri_for called with undef argument" if grep { ! defined $_ } @args;
 +
 +    my @encoded_args = ();
      foreach my $arg (@args) {
 -        utf8::encode($arg) if utf8::is_utf8($arg);
 -        $arg =~ s/([^$URI::uric])/$URI::Escape::escapes{$1}/go;
 +      if(ref($arg)||'' eq 'ARRAY') {
 +        push @encoded_args, [map {
 +          my $encoded = encode_utf8 $_;
 +          $encoded =~ s/([^$URI::uric])/$URI::Escape::escapes{$1}/go;
 +         $encoded;
 +        } @$arg];
 +      } else {
 +        push @encoded_args, do {
 +          my $encoded = encode_utf8 $arg;
 +          $encoded =~ s/([^$URI::uric])/$URI::Escape::escapes{$1}/go;
 +          $encoded;
 +        }
 +      }
      }
  
      if ( $path->$_isa('Catalyst::Action') ) { # action object
 -        s|/|%2F|g for @args;
 +        s|/|%2F|g for @encoded_args;
          my $captures = [ map { s|/|%2F|g; $_; }
 -                        ( scalar @args && ref $args[0] eq 'ARRAY'
 -                         ? @{ shift(@args) }
 +                        ( scalar @encoded_args && ref $encoded_args[0] eq 'ARRAY'
 +                         ? @{ shift(@encoded_args) }
                           : ()) ];
  
 -        foreach my $capture (@$captures) {
 -            utf8::encode($capture) if utf8::is_utf8($capture);
 -            $capture =~ s/([^$URI::uric])/$URI::Escape::escapes{$1}/go;
 -        }
 -
          my $action = $path;
          # ->uri_for( $action, \@captures_and_args, \%query_values? )
 -        if( !@args && $action->number_of_args ) {
 +        if( !@encoded_args && $action->number_of_args ) {
              my $expanded_action = $c->dispatcher->expand_action( $action );
  
              my $num_captures = $expanded_action->number_of_captures;
 -            unshift @args, splice @$captures, $num_captures;
 +            unshift @encoded_args, splice @$captures, $num_captures;
          }
  
         $path = $c->dispatcher->uri_for_action($action, $captures);
          $path = '/' if $path eq '';
      }
  
 -    unshift(@args, $path);
 +    unshift(@encoded_args, $path);
  
      unless (defined $path && $path =~ s!^/!!) { # in-place strip
          my $namespace = $c->namespace;
          if (defined $path) { # cheesy hack to handle path '../foo'
 -           $namespace =~ s{(?:^|/)[^/]+$}{} while $args[0] =~ s{^\.\./}{};
 +           $namespace =~ s{(?:^|/)[^/]+$}{} while $encoded_args[0] =~ s{^\.\./}{};
          }
 -        unshift(@args, $namespace || '');
 +        unshift(@encoded_args, $namespace || '');
      }
  
      # join args with '/', or a blank string
 -    my $args = join('/', grep { defined($_) } @args);
 +    my $args = join('/', grep { defined($_) } @encoded_args);
      $args =~ s/\?/%3F/g; # STUPID STUPID SPECIAL CASE
      $args =~ s!^/+!!;
  
        # somewhat lifted from URI::_query's query_form
        $query = '?'.join('&', map {
            my $val = $params->{$_};
 -          s/([;\/?:@&=+,\$\[\]%])/$URI::Escape::escapes{$1}/go;
 +          #s/([;\/?:@&=+,\$\[\]%])/$URI::Escape::escapes{$1}/go; ## Commented out because seems to lead to double encoding - JNAP
            s/ /+/g;
            my $key = $_;
            $val = '' unless defined $val;
            (map {
                my $param = "$_";
 -              utf8::encode( $param ) if utf8::is_utf8($param);
 +              $param = encode_utf8($param);
                # using the URI::Escape pattern here so utf8 chars survive
                $param =~ s/([^A-Za-z0-9\-_.!~*'() ])/$URI::Escape::escapes{$1}/go;
                $param =~ s/ /+/g;
 +
 +              $key = encode_utf8($key);
 +              # using the URI::Escape pattern here so utf8 chars survive
 +              $key =~ s/([^A-Za-z0-9\-_.!~*'() ])/$URI::Escape::escapes{$1}/go;
 +              $key =~ s/ /+/g;
 +
                "${key}=$param"; } ( ref $val eq 'ARRAY' ? @$val : $val ));
        } @keys);
      }
@@@ -1802,7 -1775,15 +1802,15 @@@ sub execute 
  
      if ( my $error = $@ ) {
          #rethow if this can be handled by middleware
-         if(blessed $error && ($error->can('as_psgi') || $error->can('code'))) {
+         if(
+           blessed $error && (
+             $error->can('as_psgi') ||
+             (
+               $error->can('code') &&
+               $error->code =~m/^[1-5][0-9][0-9]$/
+             )
+           )
+         ) {
              foreach my $err (@{$c->error}) {
                  $c->log->error($err);
              }
@@@ -2063,9 -2044,8 +2071,9 @@@ sub finalize_encoding 
  
      # Oh my, I wonder what filehandle responses and streams do... - jnap.
      # Encode expects plain scalars (IV, NV or PV) and segfaults on ref's
 -    $c->response->body( $c->encoding->encode( $body, $c->_encode_check ) )
 -        if ref(\$body) eq 'SCALAR';
 +    if (ref(\$body) eq 'SCALAR') {
 +      $c->response->body( $c->encoding->encode( $body, $c->_encode_check ) );
 +    };
  }
  
  =head2 $c->finalize_output
@@@ -2129,7 -2109,15 +2137,15 @@@ sub handle_request 
          $status = $c->finalize;
      } catch {
          #rethow if this can be handled by middleware
-         if(blessed $_ && ($_->can('as_psgi') || $_->can('code'))) {
+         if(
+           blessed($_) && (
+             $_->can('as_psgi') ||
+             (
+               $_->can('code') &&
+               $_->code =~m/^[1-5][0-9][0-9]$/
+             )
+           )
+         ) {
              $_->can('rethrow') ? $_->rethrow : croak $_;
          }
          chomp(my $error = $_);
@@@ -2279,7 -2267,7 +2295,7 @@@ Prepares body parameters
  
  sub prepare_body_parameters {
      my $c = shift;
 -    $c->engine->prepare_body_parameters( $c, @_ );
 +    $c->request->prepare_body_parameters( $c, @_ );
  }
  
  =head2 $c->prepare_connection
@@@ -2373,10 -2361,6 +2389,10 @@@ sub log_request 
      $method ||= '';
      $path = '/' unless length $path;
      $address ||= '';
 +
 +    $path =~ s/%([0-9A-Fa-f]{2})/chr(hex($1))/eg;
 +    $path = decode_utf8($path);
 +
      $c->log->debug(qq/"$method" request for "$path" from "$address"/);
  
      $c->log_request_headers($request->headers);
@@@ -2562,6 -2546,37 +2578,6 @@@ Prepares uploads
  sub prepare_uploads {
      my $c = shift;
      $c->engine->prepare_uploads( $c, @_ );
 -
 -    my $enc = $c->encoding;
 -    return unless $enc;
 -
 -    # Uggg we hook prepare uploads to do the encoding crap on post and query
 -    # parameters!  Sorry -jnap
 -    for my $key (qw/ parameters query_parameters body_parameters /) {
 -        for my $value ( values %{ $c->request->{$key} } ) {
 -            # N.B. Check if already a character string and if so do not try to double decode.
 -            #      http://www.mail-archive.com/catalyst@lists.scsys.co.uk/msg02350.html
 -            #      this avoids exception if we have already decoded content, and is _not_ the
 -            #      same as not encoding on output which is bad news (as it does the wrong thing
 -            #      for latin1 chars for example)..
 -            $value = $c->_handle_unicode_decoding($value);
 -        }
 -    }
 -    for my $value ( values %{ $c->request->uploads } ) {
 -        # skip if it fails for uploads, as we don't usually want uploads touched
 -        # in any way
 -        for my $inner_value ( ref($value) eq 'ARRAY' ? @{$value} : $value ) {
 -            $inner_value->{filename} = try {
 -                $enc->decode( $inner_value->{filename}, $c->_encode_check )
 -            } catch {
 -                $c->handle_unicode_encoding_exception({
 -                    param_value => $inner_value->{filename},
 -                    error_msg => $_,
 -                    encoding_step => 'uploads',
 -                });
 -            };
 -        }
 -    }
  }
  
  =head2 $c->prepare_write
@@@ -2994,9 -3009,7 +3010,9 @@@ sub apply_default_middlewares 
      return $psgi_app;
  }
  
 -=head2 $c->psgi_app
 +=head2 App->psgi_app
 +
 +=head2 App->to_app
  
  Returns a PSGI application code reference for the catalyst application
  C<$c>. This is the bare application without any middlewares
@@@ -3007,8 -3020,6 +3023,8 @@@ reference of your Catalyst application 
  
  =cut
  
 +*to_app = \&psgi_app;
 +
  sub psgi_app {
      my ($app) = @_;
      my $psgi = $app->engine->build_psgi_app($app);
@@@ -3045,7 -3056,6 +3061,7 @@@ Sets up the input/output encoding.  Se
  
  sub setup_encoding {
      my $c = shift;
 +    # This is where you'd set a default encoding
      my $enc = delete $c->config->{encoding};
      $c->encoding( $enc ) if defined $enc;
  }
@@@ -3085,13 -3095,8 +3101,13 @@@ sub _handle_unicode_decoding 
          return $value;
      }
      elsif ( ref $value eq 'HASH' ) {
 -        foreach ( values %$value ) {
 -            $_ = $self->_handle_unicode_decoding($_);
 +        foreach (keys %$value) {
 +            my $encoded_key = $self->_handle_param_unicode_decoding($_);
 +            $value->{$encoded_key} = $self->_handle_unicode_decoding($value->{$_});
 +
 +            # If the key was encoded we now have two (the original and current so
 +            # delete the original.
 +            delete $value->{$_} if $_ ne $encoded_key;
          }
          return $value;
      }
@@@ -3106,7 -3111,9 +3122,7 @@@ sub _handle_param_unicode_decoding 
  
      my $enc = $self->encoding;
      return try {
 -        Encode::is_utf8( $value ) ?
 -            $value
 -        : $enc->decode( $value, $self->_encode_check );
 +      $enc->decode( $value, $self->_encode_check );
      }
      catch {
          $self->handle_unicode_encoding_exception({
diff --combined lib/Catalyst/Engine.pm
@@@ -7,19 -7,23 +7,19 @@@ use CGI::Simple::Cookie
  use Data::Dump qw/dump/;
  use Errno 'EWOULDBLOCK';
  use HTML::Entities;
 -use HTTP::Body;
  use HTTP::Headers;
 -use URI::QueryParam;
  use Plack::Loader;
  use Catalyst::EngineLoader;
 -use Encode ();
 +use Encode 2.21 'decode_utf8';
  use Plack::Request::Upload;
  use Hash::MultiValue;
 -use utf8;
 -
  use namespace::clean -except => 'meta';
  
  # Amount of data to read from input on each pass
  our $CHUNKSIZE = 64 * 1024;
  
  # XXX - this is only here for compat, do not use!
- has env => ( is => 'rw', writer => '_set_env' );
+ has env => ( is => 'rw', writer => '_set_env' , weak_ref=>1);
  my $WARN_ABOUT_ENV = 0;
  around env => sub {
    my ($orig, $self, @args) = @_;
@@@ -589,9 -593,7 +589,9 @@@ sub prepare_query_parameters 
      # Check for keywords (no = signs)
      # (yes, index() is faster than a regex :))
      if ( index( $query_string, '=' ) < 0 ) {
 -        $c->request->query_keywords($self->unescape_uri($query_string));
 +        my $keywords = $self->unescape_uri($query_string);
 +        $keywords = decode_utf8 $keywords;
 +        $c->request->query_keywords($keywords);
          return;
      }
  
      for my $item ( @params ) {
  
          my ($param, $value)
 -            = map { $self->unescape_uri($_) }
 +            = map { decode_utf8($self->unescape_uri($_)) }
                split( /=/, $item, 2 );
  
 -        $param = $self->unescape_uri($item) unless defined $param;
 +        unless(defined $param) {
 +            $param = $self->unescape_uri($item);
 +            $param = decode_utf8 $param;
 +        }
  
          if ( exists $query{$param} ) {
              if ( ref $query{$param} ) {
@@@ -669,25 -668,20 +669,25 @@@ sub prepare_uploads 
      my $request = $c->request;
      return unless $request->_body;
  
 +    my $enc = $c->encoding;
      my $uploads = $request->_body->upload;
      my $parameters = $request->parameters;
      foreach my $name (keys %$uploads) {
 +        $name = $c->_handle_unicode_decoding($name) if $enc;
          my $files = $uploads->{$name};
          my @uploads;
          for my $upload (ref $files eq 'ARRAY' ? @$files : ($files)) {
              my $headers = HTTP::Headers->new( %{ $upload->{headers} } );
 +            my $filename = $upload->{filename};
 +            $filename = $c->_handle_unicode_decoding($filename) if $enc;
 +
              my $u = Catalyst::Request::Upload->new
                (
                 size => $upload->{size},
                 type => scalar $headers->content_type,
                 headers => $headers,
                 tempname => $upload->{tempname},
 -               filename => $upload->{filename},
 +               filename => $filename,
                );
              push @uploads, $u;
          }
diff --combined lib/Catalyst/Request.pm
@@@ -10,7 -10,7 +10,7 @@@ use HTTP::Headers
  use Stream::Buffered;
  use Hash::MultiValue;
  use Scalar::Util;
 -
 +use HTTP::Body;
  use Moose;
  
  use namespace::clean -except => 'meta';
@@@ -312,7 -312,7 +312,7 @@@ sub prepare_body_chunk 
  }
  
  sub prepare_body_parameters {
 -    my ( $self ) = @_;
 +    my ( $self, $c ) = @_;
  
      $self->prepare_body if ! $self->_has_body;
  
        return $self->_use_hash_multivalue ? Hash::MultiValue->new : {};
      }
  
 +    my $params = $self->_body->param;
 +
 +    # If we have an encoding configured (like UTF-8) in general we expect a client
 +    # to POST with the encoding we fufilled the request in. Otherwise don't do any
 +    # encoding (good change wide chars could be in HTML entity style llike the old
 +    # days -JNAP
 +
 +    # so, now that HTTP::Body prepared the body params, we gotta 'walk' the structure
 +    # and do any needed decoding.
 +
 +    # This only does something if the encoding is set via the encoding param.  Remember
 +    # this is assuming the client is not bad and responds with what you provided.  In
 +    # general you can just use utf8 and get away with it.
 +    #
 +    # I need to see if $c is here since this also doubles as a builder for the object :(
 +
 +    if($c and $c->encoding) {
 +        $params = $c->_handle_unicode_decoding($params);
 +    }
 +
      return $self->_use_hash_multivalue ?
 -        Hash::MultiValue->from_mixed($self->_body->param) :
 -        $self->_body->param;
 +        Hash::MultiValue->from_mixed($params) :
 +        $params;
  }
  
  sub prepare_connection {
@@@ -656,6 -636,56 +656,56 @@@ If multiple C<baz> parameters are provi
  cause a hash initialization error. For a more straightforward interface see
  C<< $c->req->parameters >>.
  
+ B<NOTE> Interfaces like this, which are based on L<CGI> and the C<param> method
+ are now known to cause demonstrated exploits. It is highly recommended that you
+ avoid using this method, and migrate existing code away from it.  Here's the
+ whitepaper of the exploit:
+ L<http://blog.gerv.net/2014/10/new-class-of-vulnerability-in-perl-web-applications/>
+ Basically this is an exploit that takes advantage of how L<\param> will do one thing
+ in scalar context and another thing in list context.  This is combined with how Perl
+ chooses to deal with duplicate keys in a hash definition by overwriting the value of
+ existing keys with a new value if the same key shows up again.  Generally you will be
+ vulnerale to this exploit if you are using this method in a direct assignment in a
+ hash, such as with a L<DBIx::Class> create statement.  For example, if you have
+ parameters like:
+     user?user=123&foo=a&foo=user&foo=456
+ You could end up with extra parameters injected into your method calls:
+     $c->model('User')->create({
+       user => $c->req->param('user'),
+       foo => $c->req->param('foo'),
+     });
+ Which would look like:
+     $c->model('User')->create({
+       user => 123,
+       foo => qw(a user 456),
+     });
+ (or to be absolutely clear if you are not seeing it):
+     $c->model('User')->create({
+       user => 456,
+       foo => 'a',
+     });
+ Possible remediations include scrubbing your parameters with a form validator like
+ L<HTML::FormHandler> or being careful to force scalar context using the scalar
+ keyword:
+     $c->model('User')->create({
+       user => scalar($c->req->param('user')),
+       foo => scalar($c->req->param('foo')),
+     });
+ Upcoming versions of L<Catalyst> will disable this interface by default and require
+ you to positively enable it should you require it for backwards compatibility reasons.
  =cut
  
  sub param {
@@@ -897,7 -927,7 +947,7 @@@ sub mangle_params 
          next unless defined $value;
          for ( ref $value eq 'ARRAY' ? @$value : $value ) {
              $_ = "$_";
 -            utf8::encode( $_ ) if utf8::is_utf8($_);
 +            #      utf8::encode($_);
          }
      };