diff --git a/lib/tina4/session.rb b/lib/tina4/session.rb index da6ca8b..640de89 100644 --- a/lib/tina4/session.rb +++ b/lib/tina4/session.rb @@ -175,23 +175,72 @@ def delete(key) def clear @data = {} @modified = true + @cleared = true end def to_hash @data.dup end - # Persist the session if dirty. On a backend write failure the error is - # logged and false is returned — the @modified (dirty) flag is RETAINED so - # a later save can retry. Returns true on a successful (or no-op) write. + # Persist this request's session. On a backend failure the error is logged + # and false is returned — the @modified (dirty) flag is RETAINED so a later + # save can retry. Returns true on a successful (or no-op) write. # # A cleared id (@id nil, e.g. after #destroy) is a no-op: there is nothing - # to persist, and a write would re-create the just-destroyed record. Mirrors - # the Python master's `if self._session_id and self._dirty`. + # to persist, and a write would re-create the just-destroyed record. + # + # Another request may have changed or ended this session since this one + # loaded it: a logout, a #regenerate, a set that took a privilege away. + # Writing back the whole snapshot loaded at the start undid all of those, so + # a request in flight across a logout logged the user straight back in. The + # save therefore re-reads the stored record and writes only what THIS + # request changed onto it, and never re-creates a record another request + # removed: it ends the session for this request instead, so no cookie goes + # out for it. + # + # SLIDING EXPIRY (ADR-0087): a started session the store already holds is + # re-written on EVERY save, even when this request changed nothing. That + # write re-stamps the backend deadline to now + TINA4_SESSION_TTL, so expiry + # is measured from the last request that TOUCHED the session, not from its + # last change — a user actively reading pages stays logged in. The re-write + # is the SAME re-read, merged record the concurrent-save contract computes, + # so sliding never clobbers a concurrent change. PHP is the reference. The + # guards above take precedence over the slide: no id → nothing to slide; + # a record another request ended → forget + end, never re-created; a failed + # read → deadline untouched, dirty kept for a retry; TINA4_SESSION_TTL=0 → + # the slide re-stamps a zero (immortal) deadline (ADR-0027). + # + # The slide is scoped to @stored — an id the STORE actually holds (adopted, + # not freshly minted). ADR-0087's own guard: "a request with no session (no + # id issued, OR THE ID WAS NEVER ADOPTED) writes nothing — there is no + # deadline to move." A brand-new, unmodified session (minted id, @stored + # false) therefore still no-ops, exactly as before; a modified session of + # any kind still writes. def save - return true unless @id && @modified - if safe_write(@id, @data, @ttl) + return true unless @id && (@modified || @stored) + + record = @data + if @stored + current, failed = current_record + # Whether the record still exists is unknown, so nothing is written; + # the dirty flag is kept for a later retry. + return false if failed + + if current.nil? + forget + @ended = true + return true + end + record = merged(current) unless @cleared + end + if safe_write(@id, record, @ttl) @modified = false + # An empty record is no session (#start never adopts one), so the next + # save writes whole rather than take this request's own empty write for + # a logout. + @stored = !record.empty? + @cleared = false + @loaded = fingerprints(@data) true else false # dirty flag retained for retry @@ -205,9 +254,7 @@ def save # (nulls sessionId). A fresh session needs a new #start, which mints a new id. def destroy safe_destroy(@id) if @id - @id = nil - @data = {} - @modified = false + forget end # Get a session value with optional default. @@ -272,11 +319,32 @@ def get_flash(key, default = nil) # session fixation (a pre-auth session ID must not survive into the # authenticated session). Destroys the old backend record (best-effort) # and persists under the new ID. + # + # What is carried is the session as it is stored now with this request's + # own changes applied, the same merge #save does. If another request ended + # the session after this one loaded it (a logout, or a regenerate of its + # own), it stays ended: nothing is carried, no id is minted and nil is + # returned, so no cookie goes out to replace the one that request sent. def regenerate + return nil if @ended + old_id = @id + if old_id && @stored + current, failed = current_record + unless failed + if current.nil? + forget + @ended = true + return nil + end + @data.replace(merged(current)) unless @cleared + end + end @id = SecureRandom.hex(32) safe_destroy(old_id) @modified = true + # Nothing is stored under the new id yet, so the save writes it whole. + @stored = false save @id end @@ -383,9 +451,76 @@ def adopt_or_mint(session_id) @id = session_id @data = data @modified = false + # @stored: this request is working on a record the store holds, so a save + # must never re-create it once it has gone. An empty record does not + # count: it is no session. A failed read adopts the id with {} + # (#existing_session_data): not a record this request has seen, so its + # first save writes whole. + @stored = !data.empty? + # @loaded: one fingerprint per key of what this request last saw stored; + # #save compares the live data against it to find what THIS request + # changed. @cleared: #clear ran since the last save, so the save replaces + # the stored record instead of merging into it. + @loaded = @stored ? fingerprints(@data) : {} + @cleared = false + # @ended: this request found its session ended by another request (the + # record it loaded is gone). It stays ended for the rest of the request: + # #regenerate mints nothing either. + @ended = false @id end + # Forget the session in memory: no data, no id, nothing left to save. What + # #destroy does after removing the record, and what #save does when it + # finds another request already removed it. + def forget + @id = nil + @data = {} + @modified = false + @stored = false + @cleared = false + @loaded = {} + end + + # The record as stored right now, as [data, failed]. nil and empty both mean + # "no session", exactly as in #existing_session_data. + def current_record + return [nil, true] if degraded? + + data = @handler.read(@id) + [data.nil? || (data.respond_to?(:empty?) && data.empty?) ? nil : data, false] + rescue StandardError => e + log_backend_error("read", e) + raise if @strict + [nil, true] + end + + # The stored record with only this request's own changes applied to it: the + # keys it set or changed since it last loaded or saved, minus the keys it + # removed. Every other key keeps the value the store holds now. + def merged(current) + record = current.dup + @data.each do |key, value| + now = fingerprint(value) + record[key] = value if now.nil? || @loaded[key] != now + end + @loaded.each_key { |key| record.delete(key) unless @data.key?(key) } + record + end + + def fingerprints(data) + data.each_with_object({}) { |(key, value), prints| prints[key] = fingerprint(value) } + end + + # The stored form of a value, for telling whether a request changed it. + # nil when it cannot be serialised: that always counts as changed, because + # writing a value again is harmless and missing a change is not. + def fingerprint(value) + JSON.generate(value) + rescue StandardError + nil + end + # The stored data for session_id when the backend HOLDS a session under it, # else nil — strict mode's signal to mint a fresh id rather than adopt one # the client chose. diff --git a/spec/session_concurrent_requests_spec.rb b/spec/session_concurrent_requests_spec.rb new file mode 100644 index 0000000..787f30f --- /dev/null +++ b/spec/session_concurrent_requests_spec.rb @@ -0,0 +1,475 @@ +# frozen_string_literal: true +# Copyright (c) 2026 Code Infinity +# SPDX-License-Identifier: MPL-2.0 +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at https://mozilla.org/MPL/2.0/. + + +require "spec_helper" +require "tmpdir" +require "fileutils" +require "json" +require "digest" +require "socket" + +# A request's save must not undo what another request did to the same session. +# +# Every request loads its session when it starts and saves it when it ends +# (Request#session builds one Tina4::Session per request; DispatchPipeline +# #session_save saves it after the handler). The save used to write back the +# WHOLE snapshot loaded at the start, so a request that was in flight across a +# logout put the logged-out session back the moment it saved anything: #destroy, +# #clear and #regenerate were all undone, and so was a privilege change made +# with #set. A copied session cookie outlived the logout meant to kill it. +# +# The save now writes only this request's own changes onto the record as it is +# stored NOW, and never re-creates a record removed after this request loaded +# it. +# +# Two Tina4::Session objects over one directory stand in for two concurrent +# requests. NO MOCKS: the real FileHandler on a real tmp dir, and every outcome +# is read straight off disk, never through the session under test. +RSpec.describe "Tina4::Session saves from concurrent requests" do + let(:tmp_dir) { Dir.mktmpdir("tina4_sess_concurrent") } + let(:options) { { handler: :file, handler_options: { dir: tmp_dir } } } + + after(:each) { FileUtils.rm_rf(tmp_dir) } + + # What the server does at the start of a request: a fresh Session, started + # from the cookie. + def request(session_id = nil) + session = Tina4::Session.new({ "HTTP_COOKIE" => "" }, options) + session.start(session_id) + session + end + + def logged_in(more = {}) + session = request + session.set("user", "alice") + more.each { |key, value| session.set(key, value) } + expect(session.save).to be(true) + session.get_session_id + end + + # The file on disk for session_id ("sess_.json", {"_data" => ..., + # "_expires" => }). + def session_file(session_id) + File.join(tmp_dir, "sess_#{Digest::SHA256.hexdigest(session_id)}.json") + end + + # The record on disk for session_id, or nil when there is none. Read from the + # file itself, never through the session under test. + def stored(session_id) + path = session_file(session_id) + return nil unless File.exist?(path) + + JSON.parse(File.read(path))["_data"] + end + + # The absolute expiry deadline on disk for session_id, read from the file + # itself. The FileHandler bakes `now + ttl` into "_expires" at write time. + def stored_expiry(session_id) + JSON.parse(File.read(session_file(session_id)))["_expires"] + end + + describe "a request in flight does not undo a logout" do + it "keeps a destroyed session destroyed" do + sid = logged_in + slow = request(sid) + request(sid).destroy + + slow.set("cart", "one item") + expect(slow.save).to be(true) + + expect(stored(sid)).to be_nil + # The session has ended for the slow request too: no id, so no cookie. + expect(slow.get_session_id).to be_nil + end + + it "keeps a cleared session cleared" do + sid = logged_in + slow = request(sid) + logout = request(sid) + logout.clear + expect(logout.save).to be(true) + + slow.set("cart", "one item") + slow.save + + expect(stored(sid).to_h).not_to have_key("user") + end + + it "does not bring back the id regenerate retired" do + sid = logged_in + slow = request(sid) + new_id = request(sid).regenerate + + slow.set("cart", "one item") + slow.save + + expect(stored(sid)).to be_nil + expect(stored(new_id)).to eq("user" => "alice") + end + + it "keeps a downgrade made with set" do + sid = logged_in + slow = request(sid) + demote = request(sid) + demote.set("user", "nobody") + expect(demote.save).to be(true) + + slow.set("cart", "one item") + expect(slow.save).to be(true) + + expect(stored(sid)).to eq("user" => "nobody", "cart" => "one item") + end + + it "keeps the session ended for the rest of the slow request" do + sid = logged_in + slow = request(sid) + request(sid).destroy + + slow.set("cart", "one item") + slow.save + slow.set("more", "after") + slow.save + + expect(stored(sid)).to be_nil + end + end + + # The slow request calls #regenerate at its end (a privilege change, an SSO + # callback) after another request ended or changed the session. What it + # loaded must not reach the new id. + describe "a regenerate in flight does not carry an ended session" do + # The user of every session record on disk, read from the files themselves. + def users_stored + Dir.glob(File.join(tmp_dir, "sess_*.json")).map { |path| JSON.parse(File.read(path))["_data"]["user"] } + end + + it "keeps a destroyed session ended" do + sid = logged_in + slow = request(sid) + request(sid).destroy + + expect(slow.regenerate).to be_nil + expect(slow.get_session_id).to be_nil + expect(users_stored).not_to include("alice") + end + + it "keeps a cleared session cleared" do + sid = logged_in + slow = request(sid) + logout = request(sid) + logout.clear + expect(logout.save).to be(true) + + slow.regenerate + + expect(users_stored).not_to include("alice") + end + + it "carries a downgrade along with the slow request's own change" do + sid = logged_in + slow = request(sid) + slow.set("cart", "one item") + demote = request(sid) + demote.set("user", "nobody") + expect(demote.save).to be(true) + + new_id = slow.regenerate + + expect(stored(new_id)).to eq("user" => "nobody", "cart" => "one item") + expect(stored(sid)).to be_nil + end + + it "keeps the login that won when a login is submitted twice" do + # Both requests carry the same pre-login cookie. The first to finish + # rotates the id; the other must not mint a second, empty session whose + # cookie would replace it. + anon = request + anon.set("pending", "state") + expect(anon.save).to be(true) + first = request(anon.get_session_id) + second = request(anon.get_session_id) + + second.set("user", "alice") + winner = second.regenerate + first.set("user", "alice") + first.save + + expect(first.regenerate).to be_nil + expect(first.get_session_id).to be_nil + expect(stored(winner)).to eq("pending" => "state", "user" => "alice") + expect(users_stored).to eq(["alice"]) + end + + it "keeps the winner of a login submitted twice in the documented order" do + # docs/ruby/09-sessions-cookies.md: regenerate, then set the user. + anon = request + anon.set("pending", "state") + expect(anon.save).to be(true) + first = request(anon.get_session_id) + second = request(anon.get_session_id) + + winner = second.regenerate + second.set("user", "alice") + expect(second.save).to be(true) + + expect(first.regenerate).to be_nil + first.set("user", "alice") + first.save + + expect(first.get_session_id).to be_nil + expect(stored(winner)).to eq("pending" => "state", "user" => "alice") + expect(users_stored).to eq(["alice"]) + end + + it "stores the user under the new id in the documented login" do + # docs/ruby/09-sessions-cookies.md: regenerate, then set the user. + anon = request + anon.set("csrf", "token") + expect(anon.save).to be(true) + session = request(anon.get_session_id) + + new_id = session.regenerate + session.set("user_id", 42) + expect(session.save).to be(true) + + expect(stored(new_id)).to eq("csrf" => "token", "user_id" => 42) + expect(stored(anon.get_session_id)).to be_nil + end + + it "still starts afresh on a regenerate after this request's own destroy" do + sid = logged_in + session = request(sid) + session.destroy + + new_id = session.regenerate + + expect(new_id).to be_a(String) + expect(new_id).not_to eq(sid) + expect(stored(sid)).to be_nil + end + end + + describe "concurrent requests keep each other's changes" do + it "persists two requests setting different keys" do + sid = logged_in + first = request(sid) + second = request(sid) + first.set("theme", "dark") + second.set("cart", "one item") + expect(first.save).to be(true) + expect(second.save).to be(true) + + expect(stored(sid)).to eq("user" => "alice", "theme" => "dark", "cart" => "one item") + end + + it "keeps a key another request deleted deleted" do + sid = logged_in("mfa" => "verified") + slow = request(sid) + other = request(sid) + other.delete("mfa") + expect(other.save).to be(true) + + slow.set("cart", "one item") + expect(slow.save).to be(true) + + expect(stored(sid)).to eq("user" => "alice", "cart" => "one item") + end + + it "clears keys another request added when clear runs" do + sid = logged_in + logout = request(sid) + other = request(sid) + other.set("mfa", "verified") + expect(other.save).to be(true) + + logout.clear + expect(logout.save).to be(true) + + expect(stored(sid)).to eq({}) + end + + it "lets the last save win when both change one key" do + sid = logged_in + first = request(sid) + second = request(sid) + first.set("user", "bob") + second.set("user", "carol") + first.save + second.save + + expect(stored(sid)["user"]).to eq("carol") + end + end + + describe "a save still writes what the request changed" do + # ADR-0087: a session's expiry SLIDES on activity. A request that started a + # session the store already holds re-writes the (unchanged) record on save, + # re-stamping the backend deadline to now + TINA4_SESSION_TTL, so a session + # expires after a span of INACTIVITY, not a fixed span after its last change. + # PHP is the reference (SessionConcurrentRequestsTest::testAnUnchangedSession + # IsStillWrittenSoExpiryCountsFromTheLastRequest). The re-written record is + # the re-read, merged current, so sliding never clobbers a concurrent change. + it "a read-only request moves the expiry forward" do + sid = logged_in + + # Wind the stored deadline back to "about to expire" by writing the file + # directly (never through the session under test). Still in the future, so + # the read at save time does not treat it as already expired. + record = JSON.parse(File.read(session_file(sid))) + record["_expires"] = Time.now.to_f + 5 + File.write(session_file(sid), JSON.generate(record)) + + session = request(sid) + session.get("user") # a request that only reads + expect(session.save).to be(true) + + # The deadline has slid forward to ~now + ttl (default 3600s), well past + # the 5s it was wound back to, and the stored data is untouched. + expect(stored_expiry(sid)).to be > (Time.now.to_f + 3000) + expect(stored(sid)).to eq("user" => "alice") + end + + it "saves a value changed in place with the next set" do + sid = logged_in("cart" => ["one item"]) + session = request(sid) + session.get("cart") << "two items" + session.set("seen", true) + expect(session.save).to be(true) + + expect(stored(sid)["cart"]).to eq(["one item", "two items"]) + end + + it "writes a new session whole" do + session = request + session.set("user", "alice") + session.set("theme", "dark") + expect(session.save).to be(true) + + expect(stored(session.get_session_id)).to eq("user" => "alice", "theme" => "dark") + end + + it "writes only what changed since the first save on a second save" do + sid = logged_in + session = request(sid) + session.set("theme", "dark") + expect(session.save).to be(true) + other = request(sid) + other.set("user", "nobody") + other.set("theme", "light") + expect(other.save).to be(true) + + session.set("cart", "one item") + expect(session.save).to be(true) + + expect(stored(sid)).to eq("user" => "nobody", "theme" => "light", "cart" => "one item") + end + + it "does not bring a new session back on a later save once it is destroyed" do + session = request + session.set("user", "alice") + expect(session.save).to be(true) + sid = session.get_session_id + request(sid).destroy + + session.set("cart", "one item") + session.save + + expect(stored(sid)).to be_nil + end + + it "merges again on a save after clear and a save" do + sid = logged_in + session = request(sid) + session.clear + session.set("theme", "dark") + expect(session.save).to be(true) + other = request(sid) + other.set("mfa", "verified") + expect(other.save).to be(true) + + session.set("cart", "one item") + expect(session.save).to be(true) + + expect(stored(sid)).to eq("theme" => "dark", "mfa" => "verified", "cart" => "one item") + end + + it "still stores a set after clear and a save" do + sid = logged_in + session = request(sid) + session.clear + expect(session.save).to be(true) + + session.set("cart", "one item") + expect(session.save).to be(true) + + expect(stored(sid)).to eq("cart" => "one item") + end + + it "still stores a set after regenerating a session this request emptied" do + # The SSO callback: consume the pending state, regenerate, store the identity. + anon = request + anon.set("pending", "state") + expect(anon.save).to be(true) + session = request(anon.get_session_id) + session.delete("pending") + new_id = session.regenerate + + session.set("user", "alice") + expect(session.save).to be(true) + + expect(stored(new_id)).to eq("user" => "alice") + expect(session.get_session_id).to eq(new_id) + end + + it "writes only the new data after clear then set in one request" do + sid = logged_in("theme" => "dark") + session = request(sid) + session.clear + session.set("user", "bob") + expect(session.save).to be(true) + + expect(stored(sid)).to eq("user" => "bob") + end + + it "carries everything to the new id on regenerate" do + sid = logged_in("theme" => "dark") + new_id = request(sid).regenerate + + expect(stored(new_id)).to eq("user" => "alice", "theme" => "dark") + expect(stored(sid)).to be_nil + end + end + + describe "a store that cannot be read at save time" do + # Bind a port, learn its number, close it. Nothing is listening afterwards. + def closed_port + server = TCPServer.new("127.0.0.1", 0) + port = server.addr[1] + server.close + port + end + + it "writes nothing and keeps the change for a retry" do + sid = logged_in + session = request(sid) + session.set("cart", "one item") + + # The store becomes unreachable between start and save: the REAL redis + # handler, pointed at a port the kernel really refuses. + file_handler = session.instance_variable_get(:@handler) + session.instance_variable_set(:@handler, + Tina4::SessionHandlers::RedisHandler.new(host: "127.0.0.1", port: closed_port)) + expect(session.save).to be(false) + expect(stored(sid)).to eq("user" => "alice") + + session.instance_variable_set(:@handler, file_handler) + expect(session.save).to be(true) + expect(stored(sid)).to eq("user" => "alice", "cart" => "one item") + end + end +end