diff --git a/README.md b/README.md index 29e8434..1377e67 100644 --- a/README.md +++ b/README.md @@ -1,29 +1,76 @@ # Redis Session Store -[![Code Climate](https://codeclimate.com/github/roidrage/redis-session-store.svg)](https://codeclimate.com/github/roidrage/redis-session-store) -[![Gem Version](https://badge.fury.io/rb/redis-session-store.svg)](http://badge.fury.io/rb/redis-session-store) - -A simple Redis-based session store for Rails. But why, you ask, -when there's [redis-store](http://github.com/jodosha/redis-store/)? -redis-store is a one-size-fits-all solution, and I found it not to work -properly with Rails, mostly due to a problem that seemed to lie in -Rack's `Abstract::ID` class. I wanted something that worked, so I -blatantly stole the code from Rails' `MemCacheStore` and turned it -into a Redis version. No support for fancy stuff like distributed -storage across several Redis instances. Feel free to add what you -see fit. - -This library doesn't offer anything related to caching, and is -only suitable for Rails applications. For other frameworks or -drop-in support for caching, check out -[redis-store](http://github.com/jodosha/redis-store/). +This is a forked version of the [redis-session-store](https://github.com/roidrage/redis-session-store) gem. It incorporates a few changes: -## Installation +* Configuring usage of a Redis connection pool. +* Passing the `nx: true` option when writing a new session to avoid session collisions. +* Supporting the migration towards hashed session identifiers to more fully address [GHSA-hrqr-hxpp-chr3](https://github.com/advisories/GHSA-hrqr-hxpp-chr3). +* Removes calling `exists` in Redis to check whether a session exists and instead relying on the result of `get`. -For Rails 3+, adding this to your `Gemfile` will do the trick. +## Installation ``` ruby -gem 'redis-session-store' +gem 'redis-session-store', git: 'https://github.com/18F/redis-session-store.git', tag: 'v1.0.1-18f' +``` + +## Migrating from Rack::Session::SessionId#public_id to Rack::Session::SessionId#private_id + +[GHSA-hrqr-hxpp-chr3](https://github.com/advisories/GHSA-hrqr-hxpp-chr3) describes a vulnerability to a timing attack when a key used by the backing store is the same key presented to the client. `redis-session-store` (as of the most recent version 0.11.5) writes the same key to Redis as is presented to the client, typically in the cookie. To allow for a backwards and forwards-compatible zero-downtime migration from `redis-session-store` and using `Rack::Session::SessionId#private_id` to remediate the vulnerability, this forked version provides configuration options to read and write the two versions of the session identifier. A migration path would typically look like: + +1. Deploying with the following configuration, which is backwards-compatible: + +```ruby +Rails.application.config.session_store :redis_session_store, + # ... + redis: { + # ... + read_public_id: true, + write_public_id: true, + read_private_id: false, + write_private_id: false, + } +``` + +2. Enabling writing to the private\_id key + +```ruby +Rails.application.config.session_store :redis_session_store, + # ... + redis: { + # ... + read_public_id: true, + write_public_id: true, + read_private_id: false, + write_private_id: true, + } +``` + +3. Enabling reading the private\_id key and disabling reading the public\_id key + +```ruby +Rails.application.config.session_store :redis_session_store, + # ... + redis: { + # ... + read_public_id: false, + write_public_id: true, + read_private_id: true, + write_private_id: true, + } +``` + +4. Disabling writing for the public\_id key + +```ruby +Rails.application.config.session_store :redis_session_store, + # ... + redis: { + # ... + read_public_id: false, + write_public_id: false, + read_private_id: true, + write_private_id: true, + } ``` ## Configuration @@ -34,9 +81,9 @@ In your Rails app, throw in an initializer with the following contents: ``` ruby Rails.application.config.session_store :redis_session_store, key: 'your_session_key', + expire_after: nil, # cookie expiration redis: { - expire_after: 120.minutes, # cookie expiration - ttl: 120.minutes, # Redis expiration, defaults to 'expire_after' + ttl: 120.minutes, # Redis expiration key_prefix: 'myapp:session:', url: 'redis://localhost:6379/0', } @@ -61,23 +108,19 @@ Rails.application.config.session_store :redis_session_store, By default the Marshal serializer is used. With Rails 4, you can use JSON as a custom serializer: -* `:json` - serialize cookie values with `JSON` (Requires Rails 4+) * `:marshal` - serialize cookie values with `Marshal` (Default) -* `:hybrid` - transparently migrate existing `Marshal` cookie values to `JSON` (Requires Rails 4+) -* `CustomClass` - You can just pass the constant name of any class that responds to `.load` and `.dump` +* `:json` - serialize cookie values with `JSON` +* `CustomClass` - You can just pass the constant name of a class that responds to `.load` and `.dump` ``` ruby Rails.application.config.session_store :redis_session_store, # ... other options ... - serializer: :hybrid + serializer: :json redis: { # ... redis options ... } ``` -**Note**: Rails 4 is required for using the `:json` and `:hybrid` serializers -because the `Flash` object doesn't serialize well in 3.2. See [Rails #13945](https://github.com/rails/rails/pull/13945) for more info. - ### Session load error handling If you want to handle cases where the session data cannot be loaded, a @@ -95,19 +138,6 @@ Rails.application.config.session_store :redis_session_store, **Note** The session will *always* be destroyed when it cannot be loaded. -### Other notes - -It returns with_indifferent_access if ActiveSupport is defined. - -## Rails 2 Compatibility - -This gem is currently only compatible with Rails 3+. If you need -Rails 2 compatibility, be sure to pin to a lower version like so: - -``` ruby -gem 'redis-session-store', '< 0.3' -``` - ## Contributing, Authors, & License See [CONTRIBUTING.md](CONTRIBUTING.md), [AUTHORS.md](AUTHORS.md), and diff --git a/lib/redis-session-store.rb b/lib/redis-session-store.rb index a7f7e67..8b4064e 100644 --- a/lib/redis-session-store.rb +++ b/lib/redis-session-store.rb @@ -5,7 +5,7 @@ # Redis session storage for Rails, and for Rails only. Derived from # the MemCacheStore code, simply dropping in Redis instead. class RedisSessionStore < ActionDispatch::Session::AbstractSecureStore - VERSION = '1.0.0-18f'.freeze + VERSION = '1.0.1-18f'.freeze # ==== Options # * +:key+ - Same as with the other cookie stores, key name @@ -13,7 +13,6 @@ class RedisSessionStore < ActionDispatch::Session::AbstractSecureStore # * +:url+ - Redis url, default is redis://localhost:6379/0 # * +:key_prefix+ - Prefix for keys used in Redis, e.g. +myapp:+ # * +:ttl+ - Default Redis TTL for sessions - # * +:expire_after+ - A number in seconds for session timeout # * +:client+ - Connect to Redis with given object rather than create one # * +:client_pool:+ - Connect to Redis with a ConnectionPool # * +:on_redis_down:+ - Called with err, env, and SID on Errno::ECONNREFUSED @@ -25,7 +24,7 @@ class RedisSessionStore < ActionDispatch::Session::AbstractSecureStore # Rails.application.config.session_store :redis_session_store, # key: 'your_session_key', # redis: { - # expire_after: 120.minutes, + # ttl: 120.minutes, # key_prefix: 'myapp:session:', # url: 'redis://localhost:6379/0' # }, @@ -48,16 +47,27 @@ def initialize(app, options = {}) @serializer = determine_serializer(options[:serializer]) @on_session_load_error = options[:on_session_load_error] @default_redis_ttl = redis_options[:ttl] - @write_fallback = redis_options[:write_fallback] - @read_fallback = redis_options[:read_fallback] + @read_private_id = redis_options.fetch(:read_private_id, true) + @write_private_id = redis_options.fetch(:write_private_id, true) + @read_public_id = redis_options[:read_public_id] + @write_public_id = redis_options[:write_public_id] verify_handlers! + + if !write_private_id && !write_public_id + raise ArgumentError, "write_public_id and write_private_id cannot both be false" + end + + if !read_private_id && !read_public_id + raise ArgumentError, "read_public_id and read_private_id cannot both be false" + end end attr_accessor :on_redis_down, :on_session_load_error private - attr_reader :redis_pool, :single_redis, :key, :default_options, :serializer, :default_redis_ttl, :read_fallback, :write_fallback + attr_reader :redis_pool, :single_redis, :key, :default_options, :serializer, :default_redis_ttl, + :read_private_id, :write_private_id, :read_public_id, :write_public_id def verify_handlers! %w(on_redis_down on_session_load_error).each do |h| @@ -75,7 +85,7 @@ def prefixed(sid) "#{default_options[:key_prefix]}#{private_id}" end - def prefixed_fallback(sid) + def prefixed_public_id(sid) return nil unless sid "#{default_options[:key_prefix]}#{sid}" end @@ -97,11 +107,13 @@ def find_session(req, sid) def load_session_from_redis(redis_connection, req, sid) return nil unless sid - if read_fallback - data = redis_connection.get(prefixed(sid)) || redis_connection.get(prefixed_fallback(sid)) - else - data = redis_connection.get(prefixed(sid)) - end + data = if read_private_id && !read_public_id + redis_connection.get(prefixed(sid)) + elsif read_private_id && read_public_id + redis_connection.get(prefixed(sid)) || redis_connection.get(prefixed_public_id(sid)) + elsif !read_private_id && read_public_id + redis_connection.get(prefixed_public_id(sid)) + end begin data ? decode(data) : nil @@ -120,33 +132,43 @@ def decode(data) def write_session(req, sid, session_data, options = nil) return false unless sid - if write_fallback - key = prefixed_fallback(sid) - else - key = prefixed(sid) - end + key = prefixed(sid) return false unless key expiry = options[:expire_after] || default_redis_ttl new_session = req.env['redis_session_store.new_session'] + encoded_data = encode(session_data) + + result = if write_private_id && !write_public_id + write_redis_session(key, encoded_data, expiry: expiry, new_session: new_session) + elsif write_public_id && write_private_id + public_id_key = prefixed_public_id(sid) + write_redis_session(public_id_key, encoded_data, expiry: expiry, new_session: new_session) + write_redis_session(key, encoded_data, expiry: expiry, new_session: new_session) + elsif write_public_id && !write_private_id + public_id_key = prefixed_public_id(sid) + write_redis_session(public_id_key, encoded_data, expiry: expiry, new_session: new_session) + end + + if result + sid + else + false + end + end + def write_redis_session(key, data, expiry: nil, new_session: false) result = with_redis_connection(default_rescue_value: false) do |redis_connection| if expiry && new_session - redis_connection.set(key, encode(session_data), ex: expiry, nx: true) + redis_connection.set(key, data, ex: expiry, nx: true) elsif expiry - redis_connection.set(key, encode(session_data), ex: expiry) + redis_connection.set(key, data, ex: expiry) elsif new_session - redis_connection.set(key, encode(session_data), nx: true) + redis_connection.set(key, data, nx: true) else - redis_connection.set(key, encode(session_data)) + redis_connection.set(key, data) end end - - if result - sid - else - false - end end def encode(session_data) @@ -165,9 +187,9 @@ def delete_session(req, sid, options) end def delete_session_from_redis(redis_connection, sid, req, options) - if write_fallback - fallback_key = prefixed_fallback(sid) - redis_connection.del(fallback_key) if fallback_key + if write_public_id || read_public_id + public_id_key = prefixed_public_id(sid) + redis_connection.del(public_id_key) if public_id_key end key = prefixed(sid) diff --git a/spec/redis_session_store_spec.rb b/spec/redis_session_store_spec.rb index a6f1be1..1916fb0 100644 --- a/spec/redis_session_store_spec.rb +++ b/spec/redis_session_store_spec.rb @@ -32,7 +32,6 @@ port: 16_379, db: 2, key_prefix: 'myapp:session:', - expire_after: 60 * 120 } } end @@ -57,10 +56,6 @@ expect(default_options[:key_prefix]).to eq('myapp:session:') end - it 'assigns the :expire_after option to @default_options' do - expect(default_options[:expire_after]).to eq(60 * 120) - end - context 'with a :client_pool' do let :options do { @@ -79,7 +74,7 @@ end end - describe 'when configured with both :ttl and :expire_after' do + describe 'when configured with :ttl' do let(:ttl_seconds) { 60 * 120 } let :options do { @@ -91,14 +86,12 @@ db: 2, key_prefix: 'myapp:session:', ttl: ttl_seconds, - expire_after: nil } } end it 'assigns the :ttl option to @default_options' do expect(default_options[:ttl]).to eq(ttl_seconds) - expect(default_options[:expire_after]).to be_nil end end @@ -111,7 +104,6 @@ port: 26_379, db: 4, key_prefix: 'appydoo:session:', - expire_after: 60 * 60 } end @@ -134,10 +126,6 @@ it 'assigns the :key_prefix option to @default_options' do expect(default_options[:key_prefix]).to eq('appydoo:session:') end - - it 'assigns the :expire_after option to @default_options' do - expect(default_options[:expire_after]).to eq(60 * 60) - end end describe 'when initializing with existing redis object' do @@ -148,7 +136,7 @@ redis: { client: redis_client, key_prefix: 'myapp:session:', - expire_after: 60 * 30 + ttl: 60, } } end @@ -167,8 +155,8 @@ expect(default_options[:key_prefix]).to eq('myapp:session:') end - it 'assigns the :expire_after option to @default_options' do - expect(default_options[:expire_after]).to eq(60 * 30) + it 'assigns the :ttl option to @default_options' do + expect(default_options[:ttl]).to eq(60) end end @@ -188,7 +176,9 @@ end end - it 'retrieves the prefixed private_id key from redis when read_fallback is not enabled' do + it 'retrieves the prefixed private_id from redis when read_public_id is not enabled' do + options[:redis] = { write_private_id: true, write_public_id: false, read_private_id: true, read_public_id: false } + store = described_class.new(nil, options) redis = double('redis') allow(store).to receive(:single_redis).and_return(redis) allow(store).to receive(:generate_sid).and_return(fake_key) @@ -199,21 +189,21 @@ store.send(:find_session, ActionDispatch::TestRequest.create, fake_key) end - it 'retrieves the prefixed private_id key from redis when read_fallback is not enabled' do - options[:redis] = { write_fallback: true } + it 'does not retrieve the prefixed public_id from redis when read_public_id is not enabled' do + options[:redis] = { write_private_id: true, write_public_id: false, read_private_id: true, read_public_id: false } store = described_class.new(nil, options) redis = double('redis') allow(store).to receive(:single_redis).and_return(redis) allow(store).to receive(:generate_sid).and_return(fake_key) expect(redis).to receive(:get).with("#{options[:key_prefix]}#{fake_key.private_id}").and_return( - Marshal.dump(''), + nil, ) store.send(:find_session, ActionDispatch::TestRequest.create, fake_key) end - it 'retrieves the prefixed public_id key from redis when read_fallback is enabled and the private_id key does not exist' do - options[:redis] = { read_fallback: true } + it 'retrieves the prefixed public_id from redis when read_public_id is enabled and the private_id does not exist' do + options[:redis] = { read_private_id: true, read_public_id: true } store = described_class.new(nil, options) redis = double('redis') allow(store).to receive(:single_redis).and_return(redis) @@ -228,6 +218,32 @@ store.send(:find_session, ActionDispatch::TestRequest.create, fake_key) end + it 'retrieves the prefixed public_id from redis when read_public_id is enabled and read_private_id is not enabled' do + options[:redis] = { read_private_id: false, read_public_id: true } + store = described_class.new(nil, options) + redis = double('redis') + allow(store).to receive(:single_redis).and_return(redis) + allow(store).to receive(:generate_sid).and_return(fake_key) + expect(redis).to receive(:get).with("#{options[:key_prefix]}#{fake_key.public_id}").and_return( + Marshal.dump('') + ) + + store.send(:find_session, ActionDispatch::TestRequest.create, fake_key) + end + + it 'retrieves the prefixed public_id from redis when read_public_id is enabled and read_private_id is not enabled' do + options[:redis] = { read_private_id: false, read_public_id: true } + store = described_class.new(nil, options) + redis = double('redis') + allow(store).to receive(:single_redis).and_return(redis) + allow(store).to receive(:generate_sid).and_return(fake_key) + expect(redis).to receive(:get).with("#{options[:key_prefix]}#{fake_key.public_id}").and_return( + Marshal.dump('') + ) + + store.send(:find_session, ActionDispatch::TestRequest.create, fake_key) + end + context 'when redis is down' do before do allow(store).to receive(:single_redis).and_raise(Redis::CannotConnectError) @@ -248,17 +264,28 @@ describe 'destroying a session' do context 'when destroyed via #destroy_session' do - it 'deletes the prefixed private_id key from redis when write_fallback is not enabled' do + it 'deletes the prefixed private_id from redis when write_public_id is not enabled' do + redis = double('redis') + allow(store).to receive(:single_redis).and_return(redis) + sid = store.send(:generate_sid) + expect(redis).to receive(:del).with("#{options[:key_prefix]}#{sid.private_id}") + + store.send(:delete_session, {}, sid, { drop: true }) + end + + it 'deletes the prefixed private_id and public_id from redis when write_public_id is enabled' do + store = described_class.new(nil, { redis: { write_public_id: true } }) redis = double('redis') allow(store).to receive(:single_redis).and_return(redis) sid = store.send(:generate_sid) expect(redis).to receive(:del).with("#{options[:key_prefix]}#{sid.private_id}") + expect(redis).to receive(:del).with("#{options[:key_prefix]}#{sid.public_id}") store.send(:delete_session, {}, sid, { drop: true }) end - it 'deletes the prefixed private_id and public_id keys from redis when write_fallback is enabled' do - store = described_class.new(nil, { redis: { write_fallback: true } }) + it 'deletes the prefixed private_id and public_id from redis when read_public_id is enabled' do + store = described_class.new(nil, { redis: { read_public_id: true } }) redis = double('redis') allow(store).to receive(:single_redis).and_return(redis) sid = store.send(:generate_sid) @@ -445,32 +472,50 @@ def self.dump(_value) expect(session).to eq(data2) end - it 'allows changing the session when the session has an expiry' do - env = { 'rack.session.options' => { expire_after: 60 } } - req = ActionDispatch::TestRequest.create(env) + it 'sets EX option when Redis TTL is configured' do + store = described_class.new(nil, { redis: { ttl: 60 } }) + req = ActionDispatch::TestRequest.create({}) sid = Rack::Session::SessionId.new('thisisarediskey') - allow(store).to receive(:single_redis).and_return(Redis.new) - data1 = { 'foo' => 'bar' } - store.send(:write_session, req, sid, data1, {}) - data2 = { 'baz' => 'wat' } - store.send(:write_session, req, sid, data2, {}) - _, session = store.send(:find_session, req, sid) - expect(session).to eq(data2) + redis = double('redis') + allow(store).to receive(:single_redis).and_return(redis) + expect(redis).to receive(:set).with("#{options[:key_prefix]}#{sid.private_id}", instance_of(String), { ex: 60 }) + store.send(:write_session, req, sid, { a: 1 }, {}) end - it 'writes to public_id if write_fallback is enabled' do - store = described_class.new(nil, { redis: { write_fallback: true } }) + it 'sets EX and NX options when Redis TTL is configured and new session is being set' do + store = described_class.new(nil, { redis: { ttl: 60 } }) + req = ActionDispatch::TestRequest.create({ 'redis_session_store.new_session' => true }) + sid = Rack::Session::SessionId.new('thisisarediskey') + redis = double('redis') + allow(store).to receive(:single_redis).and_return(redis) + expect(redis).to receive(:set).with("#{options[:key_prefix]}#{sid.private_id}", instance_of(String), { ex: 60, nx: true }) + store.send(:write_session, req, sid, { a: 1 }, {}) + end + + it 'sets NX option when new session is being set' do + req = ActionDispatch::TestRequest.create({ 'redis_session_store.new_session' => true }) + sid = Rack::Session::SessionId.new('thisisarediskey') + redis = double('redis') + allow(store).to receive(:single_redis).and_return(redis) + expect(redis).to receive(:set).with("#{options[:key_prefix]}#{sid.private_id}", instance_of(String), { nx: true }) + store.send(:write_session, req, sid, { a: 1 }, {}) + end + + it 'writes to private_id and public_id if write_private_id and write_public_id are enabled' do + store = described_class.new(nil, { redis: { write_private_id: true, write_public_id: true } }) redis = double('redis') req = ActionDispatch::TestRequest.create({}) sid = Rack::Session::SessionId.new('thisisarediskey') allow(store).to receive(:single_redis).and_return(redis) data1 = { 'foo' => 'bar' } expect(redis).to receive(:set).with("#{options[:key_prefix]}#{sid.public_id}", instance_of(String)) + expect(redis).to receive(:set).with("#{options[:key_prefix]}#{sid.private_id}", instance_of(String)) store.send(:write_session, req, sid, data1, {}) end - it 'writes to private_id if write_fallback is not enabled' do + it 'writes to private_id if write_private_id is enabled' do + store = described_class.new(nil, { redis: { write_private_id: true } }) redis = double('redis') req = ActionDispatch::TestRequest.create({}) sid = Rack::Session::SessionId.new('thisisarediskey') @@ -480,5 +525,17 @@ def self.dump(_value) store.send(:write_session, req, sid, data1, {}) end + + it 'writes only to public_id if write_public_id is enabled and write_private_id is not enabled' do + store = described_class.new(nil, { redis: { write_private_id: false, write_public_id: true } }) + redis = double('redis') + req = ActionDispatch::TestRequest.create({}) + sid = Rack::Session::SessionId.new('thisisarediskey') + allow(store).to receive(:single_redis).and_return(redis) + data1 = { 'foo' => 'bar' } + expect(redis).to receive(:set).with("#{options[:key_prefix]}#{sid.public_id}", instance_of(String)) + + store.send(:write_session, req, sid, data1, {}) + end end end