Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reset_session breaks indifferent access #7478

Closed
iblue opened this issue Aug 29, 2012 · 9 comments · Fixed by #7495
Closed

reset_session breaks indifferent access #7478

iblue opened this issue Aug 29, 2012 · 9 comments · Fixed by #7495

Comments

@iblue
Copy link
Contributor

iblue commented Aug 29, 2012

Hi.

I am running rails 3.2.7. When I run reset_session, it kills indifferent access.

def some_action
  session[:foo] = "bar"
  session.class # => Rack::Session::Abstract::SessionHash
  if session["foo"] == "bar" 
    puts "Everything ok until now".
  end

  reset_session # Deletes the session in the database, creates a new session id

  session[:foo] = "bar"
  session.class # => Hash
  if session["foo"] .nil?
    puts "Now it's broken"
  end
end
@steveklabnik
Copy link
Member

I'm not sure if this is intended behavior or not, but I'm pretty sure it's insane either way. ;)

@reinh
Copy link

reinh commented Aug 29, 2012

I'm pretty sure that reset_session should not change the type of session. +1

@steveklabnik
Copy link
Member

Yeah, I mean, don't get me wrong, I'm 👍, but relying on this behavior seems dangerous.

@reinh
Copy link

reinh commented Aug 29, 2012

I mean, in 7 years of Rails development I have never used reset_session in production code. But I still want it to work properly ;)

@steveklabnik
Copy link
Member

Agreed. @pixeltrix also pointed out a case where it can be legitimately useful: https://twitter.com/pixeltrix/statuses/240879855930048512

@jeffshantz
Copy link
Contributor

See section 2.8 in the Ruby on Rails Security Guide: http://guides.rubyonrails.org/security.html. You probably should be using reset_session. :)

@jeffshantz
Copy link
Contributor

Confirmed in 3.2.8.

  def index
    session[:foo] = "bar"
    str = "Before: class = #{session.class}; foo = #{session["foo"]}<br>"

    reset_session

    # Re-adding :foo = bar to session
    session[:foo] = "bar"
    str += "After: class = #{session.class}; foo = #{session["foo"]}"   # Attempting to print it using "foo" instead of :foo

    render text: str
  end

produces:

Before: class = Rack::Session::Abstract::SessionHash; foo = bar
After: class = Hash; foo =

@jeffshantz
Copy link
Contributor

Of course, upon reloading the page, the session hash is once again a Rack::Session::Abstract::SessionHash.

@iblue
Copy link
Contributor Author

iblue commented Aug 30, 2012

Yes, but the problem is persistent after reloading the page. If I save {:foo => "bar"} in a Rack::Session::Abstract::SessionHash will be dumped to the database as {"foo" => "bar"}. When I load this an a Rack::Session::Abstract::SessionHash, indifferent access works. But when I save {:foo => "bar"} when it's a Hash, it will be saved as {:foo => "bar"} (symbols instead of strings). After reloading the page, I can access it with session[:foo], but not with session["foo"].

I will give you an example. This is what we did, and it breaks stuff on our testing machine. It took us some time to figure out what was going wrong. At least we noticed, before these changed went into production.

class UserController < FrontendController
  def login
    if @user = User.authenticate(params[:email], params[:password])
      reset_session # Security!
      session[:user_id] = @user.id
      redirect_to mordor_path, :notice => "Welcome to mordor"
    else
      flash[:notice] = "One does not simply walk into mordor"
      render :login
    end
  end
end

class User < Sequel::Model
  # All sessions of this user - Yes, I know, it does not scale.
  #
  # BUG: When reset_session is used, will always return zero sessions.
  def sessions
    Rails::SessionStore.session_class.all.select do |session|
      session.data["user_id"] == self.id
    end
  end

  def force_logout!
    sessions.each do |session|
      Notification.new(:logout).send_to(session.id) # Notify the mobile devices.
      session.destroy
    end
  end
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants