[Catalyst] RequiresSLL and the little lock (patch)

[Bill Moseley]

On Tue, May 23, 2006 at 01:15:33AM -0400, Andy Grundman wrote:
> I guess that works, but why? Did you implement your own _static_file?

Andy, the point of _static_file is to not redirect back to
non-ssl on, say, css or images that might be fetched along with the
page, correct?  That is, for css/images, etc. you want to return them
however the browser requested them.

What about testing for text/html instead?  If it's text/html then
assume it's a web page so go ahead and switch back to non-ssl.  If
it's not text/html then stay in ssl mode.  See why that might not
work?

I also refactored _redirect_uri().  The problem was if
$config->{require_ssl}{https?} is defined without taking into
consideration base then it will redirect to the wrong url.
Here's the old code:

    my $redir
        = $type . '://' . $c->config->{require_ssl}->{$type} . $c->req->path;

So, if the config doesn't include $base paths then they will not be
included in the redirect.



The refactoring does cause tests to fail due to change in order
of the parameters:

#   Failed test 'redirect with params ok'
#   in t/04ssl.t at line 47.
#          got: 'http://localhost/ssl/unsecured?a=2&a=1&b=3&c=4'
#     expected: 'http://localhost/ssl/unsecured?a=1&a=2&b=3&c=4'

I can't imagine that would be a problem.  Is there a test that
compares URLs without concern of the parameter order?

Updated patch attached.





-- 
Bill Moseley
moseley at hank.org

-------------- next part --------------
Index: lib/Catalyst/Plugin/RequireSSL.pm
===================================================================
--- lib/Catalyst/Plugin/RequireSSL.pm	(revision 4187)
+++ lib/Catalyst/Plugin/RequireSSL.pm	(working copy)
@@ -27,12 +27,11 @@
 
 sub finalize {
     my $c = shift;
-    
-    # Do not redirect static files (only works with Static::Simple)
-    if ( $c->isa( "Catalyst::Plugin::Static::Simple" ) ) {
-        return $c->NEXT::finalize(@_) if $c->_static_file;
-    }
-    
+
+    # Do not redirect static files
+    # works with Static::Simple or any code that supplies _static_file
+    return $c->NEXT::finalize(@_) if $c->can('_static_file') && $c->_static_file;
+
     # redirect back to non-SSL mode
     REDIRECT:
     {
@@ -76,36 +75,22 @@
 sub _redirect_uri {
     my ( $c, $type ) = @_;
 
-    # XXX: Cat needs a $c->req->host method...
-    # until then, strip off the leading protocol from base
-    if ( !$c->config->{require_ssl}->{$type} ) {
-        my $host = $c->req->base;
-        $host =~ s/^http(s?):\/\///;
-        $c->config->{require_ssl}->{$type} = $host;
-    }
+    my $uri = $c->req->uri->clone;
+    my $config = $c->config->{require_ssl};
 
-    if ( $c->config->{require_ssl}->{$type} !~ /\/$/xms ) {
-        $c->config->{require_ssl}->{$type} .= '/';
-    }
 
-    my $redir
-        = $type . '://' . $c->config->{require_ssl}->{$type} . $c->req->path;
-        
-    if ( scalar $c->req->param ) {
-        my @params;
-        foreach my $arg ( sort keys %{ $c->req->params } ) {
-            if ( ref $c->req->params->{$arg} ) {
-                my $list = $c->req->params->{$arg};
-                push @params, map { "$arg=" . $_  } sort @{$list};
-            }
-            else {
-                push @params, "$arg=" . $c->req->params->{$arg};
-            }
-        }
-        $redir .= '?' . join( '&', @params );
-    }        
-        
-    return $redir;
+    # If not set in config grab current host name.
+    $config->{$type} ||= join ':', $uri->host, $uri->_port;
+
+
+    $uri->scheme( $type );
+
+    my ( $host, $port ) = split /:/,  $config->{$type};
+
+    $uri->host( $host );
+    $uri->port( $port ) if $port;
+
+    return $uri;
 }
 
 1;