Incorrect $c->req->base using Catalyst::Engine::SCGI on Win32

[Will Hawes-2]

I've noticed that $c->req->base is set incorrectly on Win32 using the
newly released Catalyst::Engine::SCGI.

Steps to reproduce:

catalyst.pl MyApp
cd MyApp
perl script\myapp_create.pl SCGI
perl script\myapp_scgi.pl

Start Apache with the following configuration:

LoadModule scgi_module modules/mod_scgi.so
<VirtualHost *>
    SCGIMount / 127.0.0.1:9000
</VirtualHost>

Add the following methods to MyApp::Controller::Root:

sub action :Global {
    my( $self, $c ) = @_;
    die $c->uri_for( $c->action );
}

sub Catalyst::Engine::SCGI::prepare_path {
    my( $self, $c ) = @_;
    my $env = $self->env;
    use Data::Dumper;
    warn Dumper $env;
    return $self->next::method( $c );
}

Visit http://localhost/action in a browser.

>From a combination of the debug screen in the browser and the server
output, I can see the following under Apache 2.0 on Win32 (observed on
2 machines):

PATH_INFO - /action
SCRIPT_NAME - /action
$c->req->base - http://localhost/action/
$c->uri_for( $c->action ) - http://localhost/action/action

I don't think the SCRIPT_NAME header should contain anything. Under
Apache 2.2 on Ubuntu 9.04, I see the behaviour I expected:

PATH_INFO - /action
SCRIPT_NAME - (not present)
$c->req->base - http://localhost/
$c->uri_for( $c->action ) - http://localhost/action

The old Catalyst::Engine::SCGI code addressed this kind of issue in
its prepare_path method. Not sure if that's still the recommended way
(that method has been removed from the CPAN release), but perhaps
something like the following would be sufficient (untested):

sub prepare_path {
    my( $self, $c ) = @_;
    my $env = $self->env;
    if( $env->{PATH_INFO} && $env->{SCRIPT_NAME} && $env->{PATH_INFO}
eq $env->{SCRIPT_NAME} ) {
        delete $env->{SCRIPT_NAME};
    }
    return $self->next::method( $c );
}

_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/

[Orlando Vazquez]

Hi Will, apologizes for the delayed response;

> From a combination of the debug screen in the browser and the server
> output, I can see the following under Apache 2.0 on Win32 (observed on
> 2 machines):
>
> PATH_INFO - /action
> SCRIPT_NAME - /action
> $c->req->base - http://localhost/action/
> $c->uri_for( $c->action ) - http://localhost/action/action
>
> I don't think the SCRIPT_NAME header should contain anything. Under
> Apache 2.2 on Ubuntu 9.04, I see the behaviour I expected:
>
> PATH_INFO - /action
> SCRIPT_NAME - (not present)
> $c->req->base - http://localhost/
> $c->uri_for( $c->action ) - http://localhost/action
>

Ok, just to make sure I understand the root cause of this problem:
Apache2.0@Win32 sets SCRIPT_NAME to "/action" (or whatever PATH_INFO
is), when it should really be something like "", "/" or undef, is this
correct? I assume this only happens when the application is mounted
under "/", and it does do the right this when mounted under say,
something like "/myapp"?

> The old Catalyst::Engine::SCGI code addressed this kind of issue in
> its prepare_path method. Not sure if that's still the recommended way
> (that method has been removed from the CPAN release), but perhaps
> something like the following would be sufficient (untested):
>
> sub prepare_path {
>     my( $self, $c ) = @_;
>     my $env = $self->env;
>     if( $env->{PATH_INFO} && $env->{SCRIPT_NAME} && $env->{PATH_INFO}
>     eq $env->{SCRIPT_NAME} ) {
>         delete $env->{SCRIPT_NAME};
>     }
>     return $self->next::method( $c );
> }

I'm the one who was responsible for removing the prepare_path method
from the SCGI engine. I did this because it was overriding with an
outdated verbatim copy of the prepare_path method in C::E::CGI, which
::SCGI inherits from.  Hmmm, ::FastCGI seems to include its own
additional path fixes :\ Does it make sense to unify these tweaks in
one place?

Do you know if this problem manifests itself under the same webserver
but different Catalyst engine?

I appreciate the suggested fix but I don't think it's quite safe, as
it would break something like /tags/tags where your app is mounted
under "/tags" and you're calling the "/tags" action (you know, for
whatever reason). But at the end of the day we have to get this to
work well for you, so I'm not thrilled about but also not opposed to
escaping this behaviour for this particular webserver since I know
there's a few different servers that get it wrong.

Could you check out how C::E::FastCGI::_fix_env for how to escape this
for your particular webserver platform + version, and patch/commit a
unittest+fix? I don't have a windows machine handy to test this
appropriately, but you could probably fix this in a couple of minutes.

P.S.

I got your doc bug report - it's been fixed and pushed up to the
Catalyst git repo :)

Many thanks, :)

Orlando Vazquez

_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/

[Tomas Doran]

On 28 Jun 2009, at 20:41, Orlando Vazquez wrote:
> I'm the one who was responsible for removing the prepare_path method
> from the SCGI engine. I did this because it was overriding with an
> outdated verbatim copy of the prepare_path method in C::E::CGI, which
> ::SCGI inherits from.  Hmmm, ::FastCGI seems to include its own
> additional path fixes :\ Does it make sense to unify these tweaks in
> one place?

Yes, assuming that the behavior SCGI sees is the same as FastCGI - I  
guess the current _fix_env method in the core FCGI engine could just  
be pulled out into a role you could reuse.

If you think reusing it is worthwhile / possible, please feel free to  
patch -Runtime to make that easy for you :)

Cheers
t0m


_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/

[Will Hawes-2]

2009/6/28 Orlando Vazquez <ovazquez@...>:

> Ok, just to make sure I understand the root cause of this problem:
> Apache2.0@Win32 sets SCRIPT_NAME to "/action" (or whatever PATH_INFO
> is), when it should really be something like "", "/" or undef, is this
> correct? I assume this only happens when the application is mounted
> under "/", and it does do the right this when mounted under say,
> something like "/myapp"?

That's correct, the problem only happens when mounted at "/" - things
behave as expected when mounted elsewhere.

> I'm the one who was responsible for removing the prepare_path method
> from the SCGI engine. I did this because it was overriding with an
> outdated verbatim copy of the prepare_path method in C::E::CGI, which
> ::SCGI inherits from.  Hmmm, ::FastCGI seems to include its own
> additional path fixes :\ Does it make sense to unify these tweaks in
> one place?

Almost certainly, although if you are in agreement I'd settle for
getting it working within C::E::SCGI in the first instance and
worrying about that later when I have some more time ;)

> Do you know if this problem manifests itself under the same webserver
> but different Catalyst engine?

Without having tested extensively, I think it's SCGI specific - I
don't see the problem with the CGI engine, at least.

> I appreciate the suggested fix but I don't think it's quite safe, as
> it would break something like /tags/tags where your app is mounted
> under "/tags" and you're calling the "/tags" action (you know, for
> whatever reason).
>
> But at the end of the day we have to get this to
> work well for you, so I'm not thrilled about but also not opposed to
> escaping this behaviour for this particular webserver since I know
> there's a few different servers that get it wrong.
>
> Could you check out how C::E::FastCGI::_fix_env for how to escape this
> for your particular webserver platform + version, and patch/commit a
> unittest+fix? I don't have a windows machine handy to test this
> appropriately, but you could probably fix this in a couple of minutes.

Good point, I'd not thought of the /tags/tags scenario.

Having played around with this a little more it looks as though when
things are working properly, REQUEST_URI is the same as SCRIPT_NAME
concatenated with PATH_INFO. With that in mind, perhaps revising the
suggested fix to something like this:

sub prepare_path { # or _fix_env?
    my( $self, $c ) = @_;
    my $env = $self->env;
    my $path_info = $env->{PATH_INFO} || '';
    my $script_name = $env->{SCRIPT_NAME} || '';
    my $request_uri = $env->{REQUEST_URI} || '';
    unless( $request_uri eq $script_name . $path_info ) {
        delete $env->{SCRIPT_NAME};
    }
    return $self->next::method( $c );
}

To be absolutely safe we could limit it to just this OS/web server as
you suggest:

    if( $^O eq 'MSWin32' && $env->{SERVER_SOFTWARE} =~ m{^Apache/2\.0\.63} ) {
        ...
    }

Let me know your thoughts on the above and I'll get to work on it.

> P.S.
>
> I got your doc bug report - it's been fixed and pushed up to the
> Catalyst git repo :)

Thanks :)

_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/

[Tomas Doran]

On 30 Jun 2009, at 14:51, Will Hawes wrote:

> Having played around with this a little more it looks as though when
> things are working properly, REQUEST_URI is the same as SCRIPT_NAME
> concatenated with PATH_INFO. With that in mind, perhaps revising the
> suggested fix to something like this:
>
> sub prepare_path { # or _fix_env?
>     my( $self, $c ) = @_;
>     my $env = $self->env;
>     my $path_info = $env->{PATH_INFO} || '';
>     my $script_name = $env->{SCRIPT_NAME} || '';
>     my $request_uri = $env->{REQUEST_URI} || '';
>     unless( $request_uri eq $script_name . $path_info ) {
>         delete $env->{SCRIPT_NAME};
>     }
>     return $self->next::method( $c );
> }
>
> To be absolutely safe we could limit it to just this OS/web server as
> you suggest:
>
>     if( $^O eq 'MSWin32' && $env->{SERVER_SOFTWARE} =~ m{^Apache/2\.
> 0\.63} ) {
>         ...
>     }
>
> Let me know your thoughts on the above and I'll get to work on it.

I'd maybe limit it to Win32 and apache (but don't be version  
specific) if this isn't happening to unix folks, but yeah - that  
looks fine, maybe pulling the logic out in the same way that has been  
done in the fastcgi engine?

http://github.com/orlandov/Catalyst-Engine-SCGI/tree/master

Cheers
t0m


_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/