[Catalyst] Follow-up to new uri_for() bug

[apv]

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"

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/ +
>
> _______________________________________________
> List: Catalyst at lists.rawmode.org
> Listinfo: http://lists.rawmode.org/mailman/listinfo/catalyst
> Searchable archive: http://www.mail-archive.com/ 
> catalyst at lists.rawmode.org/
> Dev site: http://dev.catalyst.perl.org/
>
>