Refuse an expired session when loading it, for every store (P1-10)

UPGRADE.md

@@ -18,6 +18,20 @@ Nothing here needs a code change. Semver impact: minor (new opt-in API, one bug
 - `Session::Store#current_session?` returns the request's session without creating one.
 - `Session::Flash#to_h` returns the messages a view shows.
 
+## Expired sessions are refused when loaded (behavior change)
+
+No API change, but sessions can now end sooner than before. Semver impact: patch (security fix).
+
+- Every store now refuses a session whose `expires_at` has passed when it is loaded, and the request starts a fresh
+  session. A copied `CookieStore` cookie no longer stays signed in after the timeout.
+- `RedisStore` sessions now end at their `expires_at` even for a visitor who keeps using them, when
+  `sliding_expiration` is `false` (the default): `expires_at` is set when the session is created (or regenerated),
+  so the session lasts `timeout` from then. Before, every save ran `SETEX`, which reset the Redis TTL, and nothing
+  read `expires_at`, so an active session lived indefinitely. To keep sessions alive while they are used, set
+  `config.sliding_expiration = true`: each request then moves `expires_at` to `timeout` from now.
+- A stored or signed payload without `expires_at` no longer loads. Payloads written by released versions always
+  carry it. A `Session::Base` subclass that overrides `after_initialize` must call `super`.
+
 ## Upgrading from the `SessionData` API to `Session::Base`
 
 Releases before v1.0.28 defined sessions with the `SessionData` module and the `SessionId(T)` wrapper. From v1.0.28 on, a single `Session::Base` class replaces both. There was no 2.0 release: this migration happens inside the 1.0.x line, and the "Before" snippets below show code that no longer compiles.

spec/base_spec.cr

@@ -46,6 +46,22 @@ describe Session::Base do
       restored.expires_at.to_unix.should eq session.expires_at.to_unix
     end
 
+    it "is required when deserialising, so a payload without it is refused instead of given a fresh expiry" do
+      payload = JSON.parse(UserSession.new.to_json).as_h
+      payload.delete("expires_at")
+
+      expect_raises(JSON::SerializableError, /expires_at/) do
+        UserSession.from_json(payload.to_json)
+      end
+    end
+
+    it "keeps the serialised format: payloads written before it was required still load" do
+      json = UserSession.new.to_json
+
+      JSON.parse(json).as_h.keys.should_not contain "expires_at_present"
+      UserSession.from_json(json).valid?.should be_true
+    end
+
     it "reflects configured timeout" do
       Session.config.timeout = 2.hours
       session = UserSession.new

spec/store_spec.cr

@@ -67,6 +67,49 @@ describe Session::Store do
     end
   end
 
+  describe "expiry" do
+    it "does not load a cookie session whose expiry has passed" do
+      store = Session::CookieStore(UserSession).new
+      session = UserSession.new
+      session.username = "replayed"
+      session.expires_at = 1.second.ago
+
+      cookies = HTTP::Cookies.new
+      cookies << HTTP::Cookie.new(name: store.session_key, value: session.session_id)
+      cookies << HTTP::Cookie.new(name: store.data_key, value: Session.config.encryptor.encrypt_and_sign(session.to_json))
+      store.load_from(cookies)
+
+      store.current_session?.should be_nil
+      store.current_session.session_id.should_not eq session.session_id
+    end
+
+    it "does not load a signed cookie session whose payload has no expires_at" do
+      store = Session::CookieStore(UserSession).new
+      session = UserSession.new
+      payload = JSON.parse(session.to_json).as_h
+      payload.delete("expires_at")
+
+      cookies = HTTP::Cookies.new
+      cookies << HTTP::Cookie.new(name: store.session_key, value: session.session_id)
+      cookies << HTTP::Cookie.new(name: store.data_key, value: Session.config.encryptor.encrypt_and_sign(payload.to_json))
+      store.load_from(cookies)
+
+      store.current_session?.should be_nil
+    end
+
+    it "still loads a cookie session that has not expired" do
+      store = Session::CookieStore(UserSession).new
+      session = UserSession.new
+
+      cookies = HTTP::Cookies.new
+      cookies << HTTP::Cookie.new(name: store.session_key, value: session.session_id)
+      cookies << HTTP::Cookie.new(name: store.data_key, value: Session.config.encryptor.encrypt_and_sign(session.to_json))
+      store.load_from(cookies)
+
+      store.current_session.session_id.should eq session.session_id
+    end
+  end
+
   describe "flash messages" do
     it "provides access to flash" do
       store = Session::MemoryStore(UserSession).new
@@ -225,6 +268,35 @@ if REDIS_AVAILABLE
       store.should be_a Session::Store(UserSession)
       store.storage.should eq "Session::RedisStore(UserSession)"
     end
+
+    it "does not load an expired session that is still in Redis, and deletes it" do
+      store = Session::RedisStore(UserSession).new(client: redis_client)
+      session = UserSession.new
+      session.username = "replayed"
+      session.expires_at = 1.second.ago
+      key = "session:#{session.session_id}"
+      redis_client.set(key, session.to_json, ex: 3600)
+
+      cookies = HTTP::Cookies.new
+      cookies << HTTP::Cookie.new(name: store.session_key, value: session.session_id)
+      store.load_from(cookies)
+
+      store.current_session?.should be_nil
+      store.current_session.session_id.should_not eq session.session_id
+      redis_client.get(key).should be_nil
+    end
+
+    it "still loads a live session from Redis" do
+      store = Session::RedisStore(UserSession).new(client: redis_client)
+      session = UserSession.new
+      redis_client.set("session:#{session.session_id}", session.to_json, ex: 3600)
+
+      cookies = HTTP::Cookies.new
+      cookies << HTTP::Cookie.new(name: store.session_key, value: session.session_id)
+      store.load_from(cookies)
+
+      store.current_session.session_id.should eq session.session_id
+    end
   end
 end
 

src/base.cr

@@ -21,7 +21,11 @@ module Session
     getter session_id : String = UUID.random.to_s
     getter created_at : Time = Time.local
 
+    # Required in a stored or signed payload (see `after_initialize`): the default is for `new` only.
+    @[JSON::Field(presence: true)]
     property expires_at : Time = Session.config.timeout.from_now
+    @[JSON::Field(ignore: true)]
+    @expires_at_present : Bool = false
 
     # Messages for the next request. They belong to the session, so they are saved with it
     # and reach only the visitor they were meant for.
@@ -70,5 +74,12 @@ module Session
     private def timeout
       Session.config.timeout.from_now
     end
+
+    # A payload without `expires_at` fails to deserialise, so no store adopts it: the default would
+    # give it a fresh expiry. A subclass that overrides this must call `super`.
+    protected def after_initialize
+      return if @expires_at_present
+      raise JSON::SerializableError.new("Missing JSON attribute: expires_at", self.class.to_s, "expires_at", 0, 0, nil)
+    end
   end
 end

src/store.cr

@@ -137,7 +137,9 @@ module Session
       end
 
       if current_session_id = request_cookies[session_key]?
-        if session = self[current_session_id.value]?
+        # The expiry is checked here, for every store: a cookie's `Expires` is only advice to the
+        # browser, and a copied cookie carries its own signed `expires_at` forever.
+        if (session = self[current_session_id.value]?) && session.valid?
           self.current_session = session
 
           # Apply sliding expiration if enabled

src/stores/redis_store.cr

@@ -93,7 +93,7 @@ module Session
         ) do
           with_redis_connection do |client|
             if data = client.get(prefixed(key))
-              begin
+              session = begin
                 json_data = decrypt_if_enabled(data)
                 T.from_json(json_data)
               rescue ex : Session::SessionEncryptionException
@@ -106,6 +106,13 @@ module Session
                 Log.warn { "Failed to deserialize session data for key #{key}: #{ex.message}" }
                 nil
               end
+
+              # Like MemoryStore: an expired session is removed instead of returned.
+              if session && !session.valid?
+                client.del(prefixed(key))
+                session = nil
+              end
+              session
             end
           end
         end