Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

ActiveRecord::SessionStore allows blank session_id

#4091

ActiveRecord::SessionStore::Session does not check for empty session_id value. So when cookie_only = false and passing in empty session_key value, a session with empty session_id can be saved into db.

The problematic code seems to be in AbstractStore

     def load_session(env)
          request = Rack::Request.new(env)
          sid = request.cookies[@key]
          unless @cookie_only
            sid ||= request.params[@key]
          end
          sid, session = get_session(env, sid)
          [sid, session]
      end

and in ActiveRecord::SessionStore

  def find_session(id)
        @@session_class.find_by_session_id(id) ||
         @@session_class.new(:session_id => id, :data => {})
  end

None of these check for empty value sid.

Reported by phan · March 2nd, 2010 @ 12:39 PM

State: incomplete
Milestone: 3.0.6
Assigned to: Yehuda Katz (wycats) Yehuda Katz (wycats)
Importance: Low

Activity

  1. Yehuda Katz (wycats)
    Yehuda Katz (wycats)
    • State changed from new to incomplete
    • Tag set to question
    • Assigned user set to Yehuda Katz (wycats)
    • Milestone cleared.

    What's the case where the user unintentionally passes in an empty session ID?

    March 29th, 2010 @ 01:08 AM

  2. phan
    phan

    If the user unintentionally pass in a empty session ID, I would think, we'd have to generate one for them. Otherwise if two users * unintentionally* passing empty session ids, they are gonna share a session object therefore a security risk.

    March 29th, 2010 @ 01:14 AM

  3. Dan Pickett
    Dan Pickett
    • Tag changed from question to bugmash, question

    Can someone write a failing test case to verify this behavior? Patches welcome as well, of course!

    May 15th, 2010 @ 02:00 AM

  4. Anil Wadghule
    Anil Wadghule

    not reproducible

    I have added following lines in session_store.rb

    Rails.application.config.session_store :active_record_store, :cookie_only => false, :key => nil

    Attached is a rails app showing it is not reproducible. Hit multiple requests to http://localhost:3000/ (with multiple browsers). It never adds a record with session_id as nil / blank.

    May 15th, 2010 @ 10:27 AM

  5. Rust/OGAWA
    Rust/OGAWA

    Attached patch is a test which should fail : a blank session_id is allowed.

    May 25th, 2010 @ 08:31 AM

  6. Rust/OGAWA
    Rust/OGAWA

    Above patch is for 2-3-stable.

    May 25th, 2010 @ 08:38 AM

  7. Rust/OGAWA
    Rust/OGAWA

    In ActionController::Session::AbstractStore, session_id is regenerated if it is nil. However, session_id is null string(""), it is not regenereted.

    Threfore, if session_id is null string, AbstractStore runs the code like ActionController::Request#reset_session and regenerates session_id. And ActionController::Session::AbstractStore#load_session treats correctly if request.cookies[@key] is blank.

    This patch for 2-3-stable is attached.

    May 25th, 2010 @ 08:50 AM

  8. Jeremy Kemper
    Jeremy Kemper
    • Milestone cleared.

    August 30th, 2010 @ 04:10 AM

  9. Jeremy Kemper
    Jeremy Kemper
    • Milestone set to 3.0.2

    October 15th, 2010 @ 11:01 PM

  10. Ryan Bigg
    Ryan Bigg
    • Tag cleared.
    • Importance changed from to Low

    Automatic cleanup of spam.

    October 19th, 2010 @ 08:34 AM

  11. Santiago Pastorino
    Santiago Pastorino
    • Milestone cleared.

    November 15th, 2010 @ 09:04 PM

  12. Santiago Pastorino
    Santiago Pastorino
    • Milestone set to 3.0.5

    February 12th, 2011 @ 09:58 PM

  13. Santiago Pastorino
    Santiago Pastorino
    • Milestone changed from 3.0.5 to 3.0.6

    February 27th, 2011 @ 03:15 AM

  14. bingbing
    bingbing

    March 26th, 2011 @ 02:10 AM