[Catalyst-dev] May (can) I fix Catalyst/Engine/CGI.pm REDIRECT_URL + PATH_INFO bug? Fwd: [Catalyst] Follow-up to new uri_for() bug

Ashley apv at sedition.com
Wed Oct 22 04:38:34 BST 2008


This is an old bug I reported the first time about two years ago,  
IIRC. Short version REDIRECT_URL + PATH_INFO can cause paths to get  
squirrelly if there are regex chars, which URIs can legally have. The  
entire proposed change is below. MST originally said, okay, if you  
get me some tests. I did eventually (the forward below) but got  
Warnocked and never followed up. Well, not never, this is the follow- 
up. :)

> -        $base_path =~ s/$ENV{PATH_INFO}$//;
> +        $base_path =~ s/\Q$ENV{PATH_INFO}\E$//;

I'm writing the dev list to see if it's okay to fix in the repository  
myself and who I should go to for a code review (mostly for the  
tests) once I make a change. This is a ridiculously easy fix and I  
think I have a commit bit already (from doing the OpenID auth  
credential) but I want to make sure I'm doing it by the book. I'm not  
sure if my tests are "perfect" and since they're more than a year old  
I'd revisit them before committing anything. Old mail is below.

-Ashley

Begin forwarded message:
> From: apv <apv at sedition.com>
> Date: July 20, 2007 1:09:45 PM PDT
> To: The elegant MVC web framework <catalyst at lists.rawmode.org>
> Subject: Re: [Catalyst] Follow-up to new uri_for() bug
> Reply-To: The elegant MVC web framework <catalyst at lists.rawmode.org>
>
> A long, long time ago. Summation: req->base, and therefore c- 
> >uri_for, gets hosed in Engine::CGI after redirect requests if the  
> request contains regex chars (several are legal for URIs) b/c it's  
> doing a non-literal substitution.
>
> The bug is actually worse that I thought before. Not only will it  
> hose internal URI stuf, it will crash an app if given an unbalanced  
> "regex" like "http://host/path/args(with-problems"

Example 500 from error log for request to /(oh hai

[Tue Oct 21 12:31:58 2008] [error] [client 127.0.0.1] FastCGI: server  
"/home/apv/sedition/dispatch.fcgi" stderr: [error] Caught exception  
in engine "Unmatched ( in regex; marked by <-- HERE in m//( <-- HERE  
oh hai$/ at /usr/local/lib/perl5/site_perl/5.10.0/Catalyst/Engine/ 
CGI.pm line 117."

>
> I got rebitten by this bug after a Cat update and forgot it never  
> got addressed. It took me an hour of reading through the Cat  
> modules to write a test but I learned a lot.
>
> So, here's the diff to fix the bug.
>
> --- Catalyst/Engine/CGI.pm.orig	2007-07-18 16:57:09.000000000 -0700
> +++ Catalyst/Engine/CGI.pm	2007-07-18 16:57:24.000000000 -0700
> @@ -115,7 +115,7 @@
>      my $base_path;
>      if ( exists $ENV{REDIRECT_URL} ) {
>          $base_path = $ENV{REDIRECT_URL};
> -        $base_path =~ s/$ENV{PATH_INFO}$//;
> +        $base_path =~ s/\Q$ENV{PATH_INFO}\E$//;
>      }
>      else {
>          $base_path = $ENV{SCRIPT_NAME} || '/';
>
> And here is a test which fails (3 of 9 fail) until the patch is  
> applied. I hope it's okay. It's the first non-Mech based Cat test  
> I've written so a core dev should certainly check it over.
>
> #!perl
>
> use strict;
> use warnings;
>
> use FindBin;
> use lib "$FindBin::Bin/lib";
>
> use Test::More tests => 9;
> use Catalyst;
> use Catalyst::Test 'TestApp';
> use Catalyst::Request;
>
> my ( $creq, $context );
>
> # test that req->base and c->uri_for work correctly after a  
> redirected request
> {
>     my $path = '/engine/request/uri/Rx(here)';
>     my $uri = 'http://localhost' . $path;
>     local $ENV{REDIRECT_URL} = $path;
>     local $ENV{PATH_INFO} = $path;
>
>     ok( my $response = request($uri), 'Request' );
>     ok( $response->is_success, 'Response Successful 2xx' );
>     ok( eval '$creq = ' . $response->content, 'Unserialize  
> Catalyst::Request' );
>
>     ok( $context = Catalyst->new({ request => $creq, }), "Created a  
> context from request" );
>
>     is( $creq->path, 'engine/request/uri/Rx(here)', 'URI contains  
> correct path' );
>     is( $creq->base, 'http://localhost/', 'Base is correct' );
>     is( $context->uri_for("/bar/baz")->as_string, "http://localhost/ 
> bar/baz", "uri_for creates correct URI from app root" );
>     is( $context->uri_for("foo/qux")->as_string, "http://localhost/ 
> foo/qux", "uri_for creates correct URI" );
>     is( $creq->path, 'engine/request/uri/Rx(here)', 'URI contains  
> correct path' );
> }
>
>
>
>
> On Sep 11, 2006, at 6:22 PM, Matt S Trout wrote:
>
>> apv wrote:
>>> http://lists.rawmode.org/pipermail/catalyst/2006-September/
>>> 009531.html (I did start a new message there, blame Mail.app, not me
>>> for the bad threading)
>>>
>>> Okay, mean guys. Make me solve my own, er, Catalyst's own, bugs.
>>>
>>> Line 118 (5.7001) of Catalyst::Engine::CGI looks like this:
>>>          $base_path =~ s/$ENV{PATH_INFO}$//;
>>>
>>> Unless I'm losing it, it should look like this:
>>>          $base_path =~ s/\Q$ENV{PATH_INFO}\E$//;
>>>
>>> Otherwise uri_for (well, request->base and anything that uses it)
>>> gets borked by any number of regex chars in the URI.
>>
>> That seems like a sane complaint.
>>
>> If you can add a failing test I can get it into 5.70002 ...
>>
>> -- 
>>       Matt S Trout       Offering custom development, consultancy  
>> and support
>>    Technical Director    contracts for Catalyst, DBIx::Class and  
>> BAST. Contact
>> Shadowcat Systems Ltd.  mst (at) shadowcatsystems.co.uk for more  
>> information
>>
>> + Help us build a better perl ORM: http://dbix- 
>> class.shadowcatsystems.co.uk/ +
>>




More information about the Catalyst-dev mailing list