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 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/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/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/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/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 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/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 220deee..f298fb8 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 @@ -73,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 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 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 -> _)