Fix session recovery after deletion

[stengineering0]

It fixes the session recovery after deletion, the same security advisories were published recently within rack repos:

• rack
• rack-session

Rack session middleware prepares the session at the beginning of request, then saves is back to the store with possible changes applied by host rack application. This way the session becomes to be a subject of race conditions in general sense over concurrent rack requests.

In particular the following scenario is possible:

1. Given that user is logged in involving warden gem (devise-backed app);
2. Frontend runs some async request A, it reads the session from the store by _session_id cookie. For instance it might be random ajax or heartbeat API call;
3. User hits logout button which runs logout request B;
4. Request B deletes the session from the store by _session_id cookie for security reasons via :drop or :renew Rack session option, then generates new session_id and responds it via the cookie to the client;
5. Request A at the end will blindly save the fetched session data under old session_id back to the store, effectively restoring it to the state before logout.
6. This way using the old session id we can act as user which logged out just now.

Steps to reproduce:

# Gemfile
source 'https://rubygems.org'

gem 'sinatra'
gem 'rackup'
gem 'dalli'
gem 'puma'

# demo.rb

require 'sinatra'

use Rack::Session::Dalli

get '/' do
  if session.key?(:current_user)
    "
      <p>Logged in as #{session[:current_user]}</p>
      <p><a href='/long' target='blank'>Long running request</a></p>
      <p><a href='/logout'>Logout</a></p>
    "
  else
    "
      <p>Not logged in</p>
      <p><a href='/login'>Login</a></p>
    "
  end
end

get '/long' do
  session.keys # just trigger
  sleep 10
  '<script>window.close();</script>'
end

get '/login' do
  session[:current_user] = 'Super Admin'
  session.options[:renew] = true
  redirect to('/')
end

get '/logout' do
  session.delete(:current_user)
  session.options[:renew] = true
  redirect to('/')
end

1. $> bundle
2. $> bundle exec ruby demo.rb
3. Open app in browser
4. Click on "Login"
5. Copy 'rack.session' cookie into clipboard
6. Click on "Long running request", it will open in new tab
7. Within 10 seconds go back to initial tab and click on "Logout"
8. Wait until "Long running request" tab will be closed
9. Paste 'rack.session' cookie from clipboard back to the browser, refresh the page
10. See that you're logged in

[stengineering0]

For now we forward through the env only persisted session id, but if/when that approach will be accepted I tend to come back and extend session_persisted_data just a bit:

1. calculate the checksum of persisted session once fetched from the store, and check it on saving. This way when session is not changed (which is the case in 99%) we can just refresh the session TTL (dc.touch) but not update the store, effectively dodging the update race condition.
2. cas can be fetched for persisted session and checked on update (dc.set_cas), introducing the optimistic lock strategy over the session store and effectively dodging any kind of race conditions.

[stengineering0]

@danmayer any thoughts on it? )

[danmayer]

yeah the approach seems OK. I do not actively use dalli sessions so I have limited exposure to all the use cases / edge cases. Do you actively use this in a project?

Mostly curious as we are looking to deprecate some of the less used features of dalli and possibly remove some things like the binary protocol in favor of the meta protocol, so gathering active feedback from the community is helpful.

also, take a look at the failing tests

[danmayer]

ahh I think all the failures are due to rubocop issues

[stengineering0]

@danmayer

Do you actively use this in a project?

Yeah, memcaced is our session store for a long time. And due to specifics of our project the given race condition is not theoretical but very practical for us ) For now we fixed it by a sort of monkey patch, then reported the advisory into rack upstream.

My hope was to think up a way to fix that on rack level considering that any other Abstract::PersistedSecure-backed store will suffer from the same race condition. But no luck, now it's needed to fix the things on Dalli level )

ahh I think all the failures are due to rubocop issues

Will check shorty, np )

[stengineering0]

@danmayer pushed )

[Copilot]

Pull Request Overview

This PR fixes the session recovery issue after deletion, ensuring that a previously valid session id cannot be reused after logout.

• Introduces a new test in test/test_rack_session.rb to verify the session id change.
• Updates session handling in lib/rack/session/dalli.rb to raise an error when a stale session is updated.
• Updates the CHANGELOG.md accordingly.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
test/test_rack_session.rb	Adds new tests to validate that session ids are not reused post‑logout.
lib/rack/session/dalli.rb	Adjusts session lookup and write logic to properly handle missing sessions and raises errors as needed.
CHANGELOG.md	Documents the session recovery fix.

Comments suppressed due to low confidence (1)

test/test_rack_session.rb:389

• The variable 'session_match' is referenced without a definition, which may lead to a runtime error during test execution. Consider defining 'session_match' or explicitly extracting the session id from the cookie string.

refute_equal login_cookie[session_match], logout_cookie[session_match]

[stengineering0]

@danmayer should I do something with head ruby failing builds? ) Seems like the fails are not relevant )

[danmayer]

These head failures aren't related. I will try to get ruby head to reproduce locally... I believe there was some changes in Ruby on how it loads and sets up CGI as I have seen similar fixes going into yjit / Ruby so it might get sorted or we might need to bump rack for Ruby head.

Anyways, this looks good to me. You good with me merging @petergoldstein

[petergoldstein]

This looks good. Feel free to merge