From f520ed49abdcae6db532b06ad3cb59bcd73cbcd4 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 17:31:07 +0000 Subject: [PATCH 1/6] Refuse the default session secret in production and staging The default secret is a constant in the source, so a session cookie signed with it can be forged by anyone. require_secure_secret was off unless an app turned it on, so a deployment that forgot to set a secret ran on the public one with only a log warning. It now defaults to on when CRYSTAL_ENV is production or staging, and can still be turned off explicitly. Development and test keep the warning. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01HT5gvYGR83U2M4DyA6NA6f --- spec/configuration_spec.cr | 44 ++++++++++++++++++++++++++++++++++++++ src/configuration.cr | 9 ++++++-- 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/spec/configuration_spec.cr b/spec/configuration_spec.cr index cdae91d..4f23e16 100644 --- a/spec/configuration_spec.cr +++ b/spec/configuration_spec.cr @@ -234,3 +234,47 @@ describe Session::Configuration do end end end + +describe Session::Configuration do + describe "the default secret outside development" do + around_each do |example| + previous = ENV["CRYSTAL_ENV"]? + example.run + previous ? (ENV["CRYSTAL_ENV"] = previous) : ENV.delete("CRYSTAL_ENV") + end + + {"production", "staging"}.each do |environment| + it "is refused in #{environment}" do + ENV["CRYSTAL_ENV"] = environment + config = Session::Configuration.new + + config.require_secure_secret.should be_true + expect_raises(Session::InsecureSecretException) { config.validate_secret! } + end + end + + it "is only warned about in development" do + ENV["CRYSTAL_ENV"] = "development" + config = Session::Configuration.new + + config.require_secure_secret.should be_false + config.validate_secret! + end + + it "can still be switched off explicitly" do + ENV["CRYSTAL_ENV"] = "production" + config = Session::Configuration.new + config.require_secure_secret = false + + config.validate_secret! + end + + it "does not get in the way of a real secret in production" do + ENV["CRYSTAL_ENV"] = "production" + config = Session::Configuration.new + config.secret = "a-secret-nobody-else-knows-0123456789" + + config.validate_secret! + end + end +end diff --git a/src/configuration.cr b/src/configuration.cr index 220deee..f7742e2 100644 --- a/src/configuration.cr +++ b/src/configuration.cr @@ -32,8 +32,13 @@ module Session # Cluster configuration for multi-node deployments with Redis Pub/Sub property cluster : ClusterConfig = ClusterConfig.new - # Security: require a secure secret (non-default) - raises if default secret is used - property require_secure_secret : Bool = false + # Environments that must not run on the published default secret. + PRODUCTION_ENVIRONMENTS = %w(production staging) + + # Security: require a secure secret (non-default) - raises if default secret is used. + # The default secret is public, so anyone could forge a session cookie signed with it: + # it is refused in production and staging (`CRYSTAL_ENV`) unless turned off explicitly. + property require_secure_secret : Bool = PRODUCTION_ENVIRONMENTS.includes?(ENV["CRYSTAL_ENV"]?) # Digest algorithm for HMAC signatures (:sha256 recommended, :sha1 for legacy) property digest_algorithm : Symbol = :sha256 From 6d1999586ed4e96f59354569d85847995cb334bc Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 17:33:03 +0000 Subject: [PATCH 2/6] Bind sessions to the client they were first used from validate_session_binding was a stub: it built a fingerprint and threw it away, so bind_to_ip and bind_to_user_agent never refused anything, and the session stored no fingerprint to compare with. The fingerprint is now saved in the session (Session::Base#client_fingerprint) on its first request and checked on every later one; a mismatch is answered with a fresh session. The client address used to come from X-Forwarded-For or X-Real-IP, which any client can send, so IP binding could be defeated by forging them. The peer address of the connection is used instead, and the headers only behind a proxy declared with trust_proxy_headers. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01HT5gvYGR83U2M4DyA6NA6f --- spec/binding_spec.cr | 5 ++ spec/client_binding_spec.cr | 95 +++++++++++++++++++++++++++++++++ src/base.cr | 3 ++ src/binding.cr | 21 ++++---- src/configuration.cr | 4 ++ src/handlers/session_handler.cr | 16 +++--- 6 files changed, 125 insertions(+), 19 deletions(-) create mode 100644 spec/client_binding_spec.cr diff --git a/spec/binding_spec.cr b/spec/binding_spec.cr index 75f0a2d..9c74a94 100644 --- a/spec/binding_spec.cr +++ b/spec/binding_spec.cr @@ -1,6 +1,11 @@ require "./spec_helper" describe Session::ClientFingerprint do + # These examples describe a deployment behind a trusted proxy that names the client in + # X-Forwarded-For / X-Real-IP; client_binding_spec covers the default, which ignores them. + before_each { Session.config.trust_proxy_headers = true } + after_each { Session.config.trust_proxy_headers = false } + describe ".binding_enabled?" do it "returns false when both bindings disabled" do Session.config.bind_to_ip = false diff --git a/spec/client_binding_spec.cr b/spec/client_binding_spec.cr new file mode 100644 index 0000000..0bf59bb --- /dev/null +++ b/spec/client_binding_spec.cr @@ -0,0 +1,95 @@ +require "./spec_helper" + +private class Seen + include HTTP::Handler + + getter ids = [] of String + + def call(context : HTTP::Server::Context) + ids << Session.config.store.as(Session::MemoryStore(UserSession)).current_session.session_id + end +end + +private def request_from(cookie : String?, ip : String = "10.0.0.1", user_agent : String = "agent-a", headers : HTTP::Headers = HTTP::Headers.new) : HTTP::Server::Context + headers["User-Agent"] = user_agent + request = HTTP::Request.new("GET", "/", headers) + request.remote_address = Socket::IPAddress.new(ip, 40000) + request.cookies << HTTP::Cookie.new(name: Session.config.session_key, value: cookie) if cookie + HTTP::Server::Context.new(request, HTTP::Server::Response.new(IO::Memory.new)) +end + +private def with_handler(&) + reset_config + Session.config.bind_to_ip = true + Session.config.bind_to_user_agent = true + store = Session::MemoryStore(UserSession).new + Session.config.store = store + seen = Seen.new + handler = Session::SessionHandler(UserSession).new(store) + handler.next = seen + yield handler, seen +ensure + Session.config.bind_to_ip = false + Session.config.bind_to_user_agent = false + Session.config.trust_proxy_headers = false +end + +# The session id the first request of a new visitor was given. +private def first_visit(handler, seen, **client) : String + handler.call(request_from(nil, **client)) + seen.ids.last +end + +describe "session binding in the handler" do + it "keeps a session for the client it was first used from" do + with_handler do |handler, seen| + id = first_visit(handler, seen) + + handler.call(request_from(id)) + + seen.ids.last.should eq id + end + end + + it "does not let another browser use the session" do + with_handler do |handler, seen| + id = first_visit(handler, seen) + + handler.call(request_from(id, user_agent: "agent-b")) + + seen.ids.last.should_not eq id + end + end + + it "does not let another address use the session" do + with_handler do |handler, seen| + id = first_visit(handler, seen) + + handler.call(request_from(id, ip: "203.0.113.9")) + + seen.ids.last.should_not eq id + end + end + + it "ignores X-Forwarded-For unless a trusted proxy is configured, since any client can send it" do + with_handler do |handler, seen| + id = first_visit(handler, seen) + + forged = HTTP::Headers{"X-Forwarded-For" => "10.0.0.1"} + handler.call(request_from(id, ip: "203.0.113.9", headers: forged)) + + seen.ids.last.should_not eq id + end + end + + it "believes X-Forwarded-For behind a trusted proxy" do + with_handler do |handler, seen| + Session.config.trust_proxy_headers = true + id = first_visit(handler, seen, headers: HTTP::Headers{"X-Forwarded-For" => "198.51.100.7"}) + + handler.call(request_from(id, ip: "172.16.0.1", headers: HTTP::Headers{"X-Forwarded-For" => "198.51.100.7"})) + + seen.ids.last.should eq id + end + end +end diff --git a/src/base.cr b/src/base.cr index 043a9af..2d2da4f 100644 --- a/src/base.cr +++ b/src/base.cr @@ -27,6 +27,9 @@ module Session # and reach only the visitor they were meant for. getter flash : Flash = Flash.new + # The client the session was first used from, when session binding is enabled. + property client_fingerprint : ClientFingerprint? = nil + def initialize end diff --git a/src/binding.cr b/src/binding.cr index 8c77002..08a3cf7 100644 --- a/src/binding.cr +++ b/src/binding.cr @@ -57,21 +57,20 @@ module Session @ip_hash.nil? && @user_agent_hash.nil? end - # Extract client IP from request, considering proxy headers + # The client's address: the peer of the connection, or, behind a trusted proxy + # (`trust_proxy_headers`), the one the proxy reports. protected def self.extract_client_ip(request : HTTP::Request) : String? - # Check X-Forwarded-For first (for proxied requests) - if forwarded = request.headers["X-Forwarded-For"]? - # Take the first IP (original client) - return forwarded.split(",").first.strip - end + if Session.config.trust_proxy_headers + if forwarded = request.headers["X-Forwarded-For"]? + return forwarded.split(",").first.strip + end - # Check X-Real-IP - if real_ip = request.headers["X-Real-IP"]? - return real_ip.strip + if real_ip = request.headers["X-Real-IP"]? + return real_ip.strip + end end - # Fall back to remote address (may not be available in all contexts) - nil + request.remote_address.try(&.as?(Socket::IPAddress)).try(&.address) end private def hash_value(value : String) : String diff --git a/src/configuration.cr b/src/configuration.cr index f7742e2..f298fb8 100644 --- a/src/configuration.cr +++ b/src/configuration.cr @@ -78,6 +78,10 @@ module Session property bind_to_ip : Bool = false property bind_to_user_agent : Bool = false + # Whether `X-Forwarded-For` and `X-Real-IP` name the client for IP binding. Any client can + # send them, so they are believed only when a trusted proxy in front of the app sets them. + property trust_proxy_headers : Bool = false + # Track if secret warning has been logged to avoid spamming @secret_warning_logged : Bool = false property on_started : Proc(String, Session::Base, Nil) = ->(sid : String, data : Session::Base) do diff --git a/src/handlers/session_handler.cr b/src/handlers/session_handler.cr index cd00555..4670df6 100644 --- a/src/handlers/session_handler.cr +++ b/src/handlers/session_handler.cr @@ -55,16 +55,16 @@ module Session Log.warn { "Failed to clear corrupted session: #{ex.message}" } end + # A session is tied to the client it was first used from: a request from a different + # client is refused (and answered with a fresh session by `load_session`'s rescue). private def validate_session_binding(request : HTTP::Request) - # Get stored fingerprint from session data if available - # Note: This requires the session data type to have a client_fingerprint property - # For now, we create a new fingerprint and validate against request - # In a full implementation, the fingerprint would be stored in the session - _fingerprint = ClientFingerprint.from_request(request) + session = @session.current_session - # If session has a stored fingerprint, validate it - # This is a simplified implementation - in practice you'd store the fingerprint - # in the session data when the session is created + if fingerprint = session.client_fingerprint + fingerprint.validate!(request) + else + session.client_fingerprint = ClientFingerprint.from_request(request) + end end private def generate_session_id From 06060c550b60e5676248365685f33a2379cc3eee Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 17:48:48 +0000 Subject: [PATCH 3/6] Keep CookieStore's request cookies per request, not per store The session lives in the request's cookies, and CookieStore kept them in one property shared by every request, so a concurrent request could read (or overwrite) another visitor's session cookie. They are now kept per fiber, like the current session, forgotten by release when the request ends, and pruned for fibers that have finished. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01HT5gvYGR83U2M4DyA6NA6f --- spec/cookie_store_isolation_spec.cr | 50 +++++++++++++++++++++++++++++ src/stores/cookie_store.cr | 27 ++++++++++++++-- 2 files changed, 75 insertions(+), 2 deletions(-) create mode 100644 spec/cookie_store_isolation_spec.cr diff --git a/spec/cookie_store_isolation_spec.cr b/spec/cookie_store_isolation_spec.cr new file mode 100644 index 0000000..5a3cd88 --- /dev/null +++ b/spec/cookie_store_isolation_spec.cr @@ -0,0 +1,50 @@ +require "./spec_helper" + +describe "CookieStore request isolation" do + it "keeps each request's cookies to the request that sent them" do + store = Session::CookieStore(UserSession).new + seen = Channel({String, String?}).new + + %w(alice bob).each do |name| + spawn do + cookies = HTTP::Cookies.new + cookies << HTTP::Cookie.new("who", name) + store.cookies = cookies + Fiber.yield # the other request runs before this one reads its cookies + seen.send({name, store.cookies["who"]?.try(&.value)}) + end + end + + results = 2.times.map { seen.receive }.to_a.sort_by(&.first) + results.should eq [{"alice", "alice"}, {"bob", "bob"}] + end + + it "lets a session written in one request be read back only from that request's cookies" do + store = Session::CookieStore(UserSession).new + alice = UserSession.new + alice.username = "alice" + store[alice.session_id] = alice + alices_cookies = store.cookies + + other = Channel(String?).new + spawn do + store.cookies = HTTP::Cookies.new # a visitor without a session cookie + other.send(store[alice.session_id]?.try(&.username)) + end + + other.receive.should be_nil + store.cookies = alices_cookies + store[alice.session_id].username.should eq "alice" + end + + it "does not keep the cookies of requests that are over" do + store = Session::CookieStore(UserSession).new + done = Channel(Nil).new + 50.times { spawn { store.cookies = HTTP::Cookies.new; done.send(nil) } } + 50.times { done.receive } + Fiber.yield + + store.cookies = HTTP::Cookies.new + store.@request_cookies.size.should be < 5 + end +end diff --git a/src/stores/cookie_store.cr b/src/stores/cookie_store.cr index 7ca56f9..505ae55 100644 --- a/src/stores/cookie_store.cr +++ b/src/stores/cookie_store.cr @@ -3,9 +3,32 @@ require "../store" module Session class CookieStore(T) < Store(T) include Enumerable(HTTP::Cookie) - property cookies : HTTP::Cookies = HTTP::Cookies.new - def initialize(@cookies : HTTP::Cookies = HTTP::Cookies.new) + # The cookies of the request being served. The session lives in them, and a store is shared + # by every request, so they are kept per fiber, like the current session: another request + # must never see (or overwrite) them. `release` forgets them when the request ends. + @request_cookies = {} of Fiber => HTTP::Cookies + + def initialize(cookies : HTTP::Cookies = HTTP::Cookies.new) + self.cookies = cookies + end + + def cookies : HTTP::Cookies + @mutex.synchronize { @request_cookies[Fiber.current] ||= remember_cookies(HTTP::Cookies.new) } + end + + def cookies=(cookies : HTTP::Cookies) : HTTP::Cookies + @mutex.synchronize { remember_cookies(cookies) } + end + + def release : Nil + super + @mutex.synchronize { @request_cookies.delete(Fiber.current) } + end + + private def remember_cookies(cookies : HTTP::Cookies) : HTTP::Cookies + @request_cookies.reject! { |fiber, _| fiber.dead? } + @request_cookies[Fiber.current] = cookies end def each(&block : HTTP::Cookie -> _) From 7176171131266d1256a210c401ef7bb9f26d1c74 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 17:48:48 +0000 Subject: [PATCH 4/6] Add Store#authenticate to change privileges under a new session id Reusing the id a visitor had before logging in lets anyone who planted or observed it share the authenticated session (session fixation). authenticate yields the current session for the change and then regenerates its id, so a login cannot forget the second step. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01HT5gvYGR83U2M4DyA6NA6f --- spec/store_spec.cr | 17 +++++++++++++++++ src/store.cr | 12 ++++++++++++ 2 files changed, 29 insertions(+) diff --git a/spec/store_spec.cr b/spec/store_spec.cr index 7bad41c..4ae27f1 100644 --- a/spec/store_spec.cr +++ b/spec/store_spec.cr @@ -227,3 +227,20 @@ if REDIS_AVAILABLE end end end + +describe Session::Store do + describe "#authenticate" do + it "applies the change and moves the session to a new id, so the old one stops working" do + store = Session::MemoryStore(UserSession).new + store.create + before = store.session_id + + store.authenticate { |session| session.username = "alice" } + + store.session_id.should_not eq before + store.current_session.username.should eq "alice" + store[store.session_id].username.should eq "alice" + store[before]?.should be_nil + end + end +end diff --git a/src/store.cr b/src/store.cr index ad57ab2..1e83fe4 100644 --- a/src/store.cr +++ b/src/store.cr @@ -106,6 +106,18 @@ module Session current_session end + # Change the session's privileges (log in, switch user, elevate) and give it a new id in + # one step. Re-using the id a visitor had before logging in lets anyone who planted or + # observed it (session fixation) share the authenticated session. + # + # ``` + # store.authenticate { |session| session.user_id = user.id } + # ``` + def authenticate(& : T -> _) : T + yield current_session + regenerate_id + end + def create : T self.current_session = T.new self[session_id] = current_session From 74be8e68568c8209a37ef73c833423624cb468b8 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 17:53:37 +0000 Subject: [PATCH 5/6] Wait for Pub/Sub invalidation instead of sleeping The multi-node cache-clear spec slept a fixed 150 ms for another node's Pub/Sub message to arrive, and failed on a loaded machine. It now polls for the result with a bounded wait (eventually), so the usual case is faster and a slow one is not a failure. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01HT5gvYGR83U2M4DyA6NA6f --- spec/clustered_redis_store_spec.cr | 4 +--- spec/spec_helper.cr | 11 +++++++++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/spec/clustered_redis_store_spec.cr b/spec/clustered_redis_store_spec.cr index 200175f..31d7f14 100644 --- a/spec/clustered_redis_store_spec.cr +++ b/spec/clustered_redis_store_spec.cr @@ -489,10 +489,8 @@ if REDIS_AVAILABLE # Clear from node 1 store1.clear - sleep(150.milliseconds) - # Node 2's cache should be cleared - store2.coordinator.local_cache.size.should eq 0 + eventually { store2.coordinator.local_cache.size == 0 }.should be_true store1.shutdown store2.shutdown diff --git a/spec/spec_helper.cr b/spec/spec_helper.cr index cd81892..c036fd3 100644 --- a/spec/spec_helper.cr +++ b/spec/spec_helper.cr @@ -47,3 +47,14 @@ end Spec.before_each do reset_config end + +# Waits for something that happens in another fiber, such as a Pub/Sub message, to become true. +# Returns as soon as it does (so the usual case is faster than a fixed sleep) and gives up after +# `timeout` (so a loaded machine does not make the spec fail), returning the last answer. +def eventually(timeout : Time::Span = 3.seconds, & : -> Bool) : Bool + deadline = Time.instant + timeout + until (result = yield) || Time.instant >= deadline + sleep 10.milliseconds + end + result +end From 4a22d95aab2ad4cbef3a2518dd15cb8d567b9102 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 18:03:34 +0000 Subject: [PATCH 6/6] Run the release job only on pushes, and check formatting in CI The release job ran on pull requests too. It computes the next version, tags it and commits the bump to master, so on a pull request it either failed (the tag already exists, as on every PR since 1.0.32) or would have written to master from a PR run. It now needs a push event. The format check was commented out; the repository is formatted, so it is back on. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01HT5gvYGR83U2M4DyA6NA6f --- .github/workflows/crystal.yml | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/.github/workflows/crystal.yml b/.github/workflows/crystal.yml index 160c98a..f305685 100644 --- a/.github/workflows/crystal.yml +++ b/.github/workflows/crystal.yml @@ -33,8 +33,8 @@ jobs: - name: Install dependencies run: shards install - # - name: Check code style - # run: crystal tool format ./src ./spec --check + - name: Check code style + run: crystal tool format --check src spec - name: Run tests run: crystal spec --error-trace @@ -45,7 +45,9 @@ jobs: runs-on: ubuntu-latest needs: - test - if: ${{ success()}} + # Releasing tags the version and commits the bump to master, so only a push to master may do it + # (on a pull request the tag already exists and the job fails, or would write to master). + if: ${{ success() && github.event_name == 'push' }} steps: - name: Checkout uses: actions/checkout@v2