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

diff --git a/Changes b/Changes
index 93e194f..231624d 100644 (file)
--- a/Changes
+++ b/Changes
     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++)
 
index 725494c..ddc77f6 100644 (file)
@@ -1802,7 +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);
             }
@@ -2129,7 +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 = $_);
index 166a02d..194d254 100644 (file)
@@ -19,7 +19,7 @@ use namespace::clean -except => 'meta';
 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) = @_;
index cdaa173..0fe34b0 100644 (file)
@@ -656,6 +656,56 @@ If multiple C<baz> parameters are provided this code might corrupt data or
 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 {
index 5cb3117..ad6544a 100644 (file)
@@ -12,10 +12,6 @@ use Plack::Test;
 {
   package MyApp::Exception;
 
-  use overload
-    # Use the overloading thet HTTP::Exception uses
-    bool => sub { 1 }, '""' => 'as_string', fallback => 1;
-
   sub new {
     my ($class, $code, $headers, $body) = @_;
     return bless +{res => [$code, $headers, $body]}, $class;
@@ -35,8 +31,14 @@ use Plack::Test;
     };
   }
 
+  package MyApp::AnotherException;
+
+  sub new { bless +{}, shift }
+
+  sub code { 400 }
+
   sub as_string { 'bad stringy bad' }
-  
+
   package MyApp::Controller::Root;
 
   use base 'Catalyst::Controller';
@@ -60,6 +62,11 @@ use Plack::Test;
       403, ['content-type'=>'text/plain'], ['Forbidden']);
   }
 
+  sub from_code_type :Local {
+    my $e = MyApp::AnotherException->new;
+    die $e;
+  }
+
   sub classic_error :Local {
     my ($self, $c) = @_;
     Catalyst::Exception->throw("Ex Parrot");
@@ -107,6 +114,14 @@ test_psgi $psgi, sub {
 
 test_psgi $psgi, sub {
     my $cb = shift;
+    my $res = $cb->(GET "/root/from_code_type");
+    is $res->code, 400;
+    is $res->content, 'bad stringy bad', 'bad stringy bad';
+    unlike $res->content, qr'HTTPExceptions', 'HTTPExceptions';
+};
+
+test_psgi $psgi, sub {
+    my $cb = shift;
     my $res = $cb->(GET "/root/classic_error");
     is $res->code, 500;
     like $res->content, qr'Ex Parrot', 'Ex Parrot';
@@ -127,5 +142,5 @@ test_psgi $psgi, sub {
 # in the callbacks might never get run (thus all ran tests pass but not all
 # required tests run).
 
-done_testing(14);
+done_testing(17);
 
index fc0d622..72ddb15 100644 (file)
@@ -22,7 +22,9 @@ use Catalyst::Test 'TestApp';
     ok($resp->is_success);
     #is($ctx->count_leaks, 1);
     # FIXME: find out why this changed from 1 to 2 after 52af51596d
-    is($ctx->count_leaks, 2);
+    # ^^ probably has something to do with env being in Engine and Request - JNAP
+    # ^^ I made the env in Engine a weak ref, should help until we can remove it
+    is($ctx->count_leaks, 1);
 }
 
 {