Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Catalyst: Dev

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

 

 

Catalyst dev RSS feed   Index | Next | Previous | View Threaded


apv at sedition

Oct 21, 2008, 8:38 PM

Post #1 of 1 (990 views)
Permalink
May (can) I fix Catalyst/Engine/CGI.pm REDIRECT_URL + PATH_INFO bug? Fwd: [Catalyst] Follow-up to new uri_for() bug

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>
> Date: July 20, 2007 1:09:45 PM PDT
> To: The elegant MVC web framework <catalyst [at] lists>
> Subject: Re: [Catalyst] Follow-up to new uri_for() bug
> Reply-To: The elegant MVC web framework <catalyst [at] lists>
>
> 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/ +
>>


_______________________________________________
Catalyst-dev mailing list
Catalyst-dev [at] lists
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev

Catalyst dev RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.