Refactor guts of C::P::Session to use _session and not {session}, add validate_sessio...

diff --git a/lib/Catalyst/Plugin/Session.pm b/lib/Catalyst/Plugin/Session.pm
index cad6577..e5b3934 100644 (file)
--- a/lib/Catalyst/Plugin/Session.pm
+++ b/lib/Catalyst/Plugin/Session.pm
@@ -14,7 +14,7 @@ use overload            ();
 our $VERSION = "0.02";
 
 BEGIN {
-    __PACKAGE__->mk_accessors(qw/sessionid session_delete_reason/);
+    __PACKAGE__->mk_accessors(qw/_sessionid _session session_delete_reason/);
 }
 
 sub setup {
@@ -60,13 +60,13 @@ sub setup_session {
 sub finalize {
     my $c = shift;
 
-    if ( $c->{session} ) {
+    if ( my $session_data = $c->_session ) {
 
         # all sessions are extended at the end of the request
         my $now = time;
-        @{ $c->{session} }{qw/__updated __expires/} =
+        @{ $session_data }{qw/__updated __expires/} =
           ( $now, $c->config->{session}{expires} + $now );
-        $c->store_session_data( $c->sessionid, $c->{session} );
+        $c->store_session_data( $c->sessionid, $session_data );
     }
 
     $c->NEXT::finalize(@_);
@@ -76,20 +76,21 @@ sub prepare_action {
     my $c = shift;
 
     if ( my $sid = $c->sessionid ) {
-        my $s = $c->{session} ||= $c->get_session_data($sid);
-        if ( !$s or $s->{__expires} < time ) {
+               no warnings 'uninitialized'; # ne __address
+
+        my $session_data = $c->_session || $c->_session( $c->get_session_data($sid) );
+        if ( !$session_data or $session_data->{__expires} < time ) {
 
             # session expired
             $c->log->debug("Deleting session $sid (expired)") if $c->debug;
             $c->delete_session("session expired");
         }
         elsif ($c->config->{session}{verify_address}
-            && $c->{session}{__address}
-            && $c->{session}{__address} ne $c->request->address )
+            && $session_data->{__address} ne $c->request->address )
         {
             $c->log->warn(
                     "Deleting session $sid due to address mismatch ("
-                  . $c->{session}{__address} . " != "
+                  . $session_data->{__address} . " != "
                   . $c->request->address . ")",
             );
             $c->delete_session("address mismatch");
@@ -110,22 +111,44 @@ sub delete_session {
     $c->delete_session_data($sid);
 
     # reset the values in the context object
-    $c->{session} = undef;
-    $c->sessionid(undef);
+    $c->_session(undef);
+    $c->_sessionid(undef);
     $c->session_delete_reason($msg);
 }
 
+sub sessionid {
+       my $c = shift;
+
+       if ( @_ ) {
+               if ( $c->validate_session_id( my $sid = shift ) ) {
+                       return $c->_sessionid( $sid );
+               } else {
+                       my $err = "Tried to set invalid session ID '$sid'";
+                       $c->log->error( $err );
+                       Catalyst::Exception->throw( $err );
+               }
+       }
+
+       return $c->_sessionid;
+}
+
+sub validate_session_id {
+       my ( $c, $sid ) = @_;
+
+       $sid =~ /^[a-f\d]+$/i;
+}
+
 sub session {
     my $c = shift;
 
-    return $c->{session} if $c->{session};
-
-    my $sid = $c->generate_session_id;
-    $c->sessionid($sid);
+    $c->_session || do {
+               my $sid = $c->generate_session_id;
+               $c->sessionid($sid);
 
-    $c->log->debug(qq/Created session "$sid"/) if $c->debug;
+               $c->log->debug(qq/Created session "$sid"/) if $c->debug;
 
-    return $c->initialize_session_data;
+               $c->initialize_session_data;
+       };
 }
 
 sub initialize_session_data {
@@ -133,7 +156,7 @@ sub initialize_session_data {
 
     my $now = time;
 
-    return $c->{session} = {
+    return $c->_session({
         __created => $now,
         __updated => $now,
         __expires => $now + $c->config->{session}{expires},
@@ -143,7 +166,7 @@ sub initialize_session_data {
             ? ( __address => $c->request->address )
             : ()
         ),
-    };
+    });
 }
 
 sub generate_session_id {
@@ -372,6 +395,13 @@ overridable in case you want to provide more random data.
 
 Currently it returns a concatenated string which contains:
 
+=item validate_session_id SID
+
+Make sure a session ID is of the right format.
+
+This currently ensures that the session ID string is any amount of case
+insensitive hexadecimal characters.
+
 =over 4
 
 =item *
@@ -484,7 +514,7 @@ is deleted.
 =item __updated
 
 The last time a session was saved. This is the value of
-C<< $c->{session}{__expires} - $c->config->{session}{expires} >>.
+C<< $c->session->{__expires} - $c->config->session->{expires} >>.
 
 =item __created
 

diff --git a/t/02_session_data.t b/t/02_session_data.t
index 11d3ea3..6331b17 100644 (file)
--- a/t/02_session_data.t
+++ b/t/02_session_data.t
@@ -3,9 +3,10 @@
 use strict;
 use warnings;
 
-use Test::More tests => 19;
+use Test::More tests => 20;
 use Test::MockObject;
 use Test::Deep;
+use Test::Exception;
 
 my $m;
 BEGIN { use_ok( $m = "Catalyst::Plugin::Session" ) }
@@ -16,7 +17,7 @@ my $req      = Test::MockObject->new;
 my @mock_isa = ();
 my %session;
 
-$log->set_true(qw/fatal warn/);
+$log->set_true(qw/fatal warn error/);
 
 $req->set_always( address => "127.0.0.1" );
 
@@ -40,7 +41,7 @@ $req->set_always( address => "127.0.0.1" );
     $c->setup;
 
     $c->prepare_action;
-    ok( !$c->{session}, "without a session ID prepare doesn't load a session" );
+    ok( !$c->_session, "without a session ID prepare doesn't load a session" );
 }
 
 {
@@ -56,10 +57,10 @@ $req->set_always( address => "127.0.0.1" );
     my $c = MockCxt->new;
     $c->setup;
 
-    $c->sessionid("the_session");
+    $c->sessionid("decafbad");
     $c->prepare_action;
 
-    ok( $c->{session}, 'session "restored" with session id' );
+    ok( $c->_session, 'session "restored" with session id' );
 }
 
 {
@@ -73,10 +74,10 @@ $req->set_always( address => "127.0.0.1" );
     my $c = MockCxt->new;
     $c->setup;
 
-    $c->sessionid("the_session");
+    $c->sessionid("decafbad");
     $c->prepare_action;
 
-    ok( !$c->{session}, "expired sessions are deleted" );
+    ok( !$c->_session, "expired sessions are deleted" );
     like( $c->session_delete_reason, qr/expire/i, "with appropriate reason" );
     ok( !$c->sessionid, "sessionid is also cleared" );
 }
@@ -92,10 +93,10 @@ $req->set_always( address => "127.0.0.1" );
     my $c = MockCxt->new;
     $c->setup;
 
-    $c->sessionid("the_session");
+    $c->sessionid("decafbad");
     $c->prepare_action;
 
-    ok( !$c->{session}, "hijacked sessions are deleted" );
+    ok( !$c->_session, "hijacked sessions are deleted" );
     like( $c->session_delete_reason, qr/mismatch/, "with appropriate reason" );
     ok( !$c->sessionid, "sessionid is also cleared" );
 }
@@ -113,10 +114,10 @@ $req->set_always( address => "127.0.0.1" );
     my $c = MockCxt->new;
     $c->setup;
 
-    $c->sessionid("the_session");
+    $c->sessionid("decafbad");
     $c->prepare_action;
 
-    ok( $c->{session}, "address mismatch is OK if verify_address is disabled" );
+    ok( $c->_session, "address mismatch is OK if verify_address is disabled" );
 }
 
 {
@@ -144,7 +145,7 @@ $req->set_always( address => "127.0.0.1" );
         $c->request->address, "address is also correct" );
 
     cmp_deeply(
-        [ keys %{ $c->{session} } ],
+        [ keys %{ $c->_session } ],
         bag(qw/__expires __created __updated __address/),
         "initial keys in session are all there",
     );
@@ -165,11 +166,11 @@ $req->set_always( address => "127.0.0.1" );
 
     my $now = time();
 
-    $c->sessionid("the_session");
+    $c->sessionid("decafbad");
     $c->prepare_action;
     $c->finalize;
 
-    ok( $c->{session},
+    ok( $c->_session,
         "session is still alive after 1/2 expired and finalized" );
 
     cmp_ok(
@@ -178,5 +179,9 @@ $req->set_always( address => "127.0.0.1" );
         $now + 2000,
         "session expires time extended"
     );
+
+       dies_ok {
+               $c->sessionid("user:foo");
+       } "can't set invalid sessionid string";
 }