Before filter question

dubstep · September 2, 2011, 10:35am

I have a correct_user() before filter that passes all my tests:

class UsersController < ApplicationController
before_filter :authenticate, :only => [:edit, :update]
before_filter :correct_user, :only => [:edit, :update]

…
…

private
def correct_user
user = User.find(params[:id])
redirect_to(root_path) unless get_user_from_session == user
end

But if I change the before filter to this:

def correct_user
redirect_to(root_path) unless get_user_from_session.id.to_s ==
params[:id]
end

all kinds of things start failing. Here’s an example:

UsersController GET edit should be successful
Failure/Error: response.should be_success
expected success? to return true, got false
./spec/controllers/users_controller_spec.rb:15:in `block (3

levels) in <top (required)>’

and the test:

describe UsersController do
render_views

describe “GET edit” do
before(:each) do
@user = Factory(:user)
test_sign_in(@user)
end

it "should be successful" do
  get :edit, :id => @user
  response.should be_success
end

What is the difference between:

def correct_user
  user = User.find(params[:id])
  redirect_to(root_path) unless get_user_from_session == user
end

and:

def correct_user
redirect_to(root_path) unless get_user_from_session.id.to_s ==
params[:id]
end

To try and debug what is happening, I changed correct_user() to this:

def correct_user
  @session_id = get_user_from_session.id
  @session_id_class = get_user_from_session.id.class
  @params_id = params[:id]
  @params_id_class = params[:id].class
end

and then I added those variables to my edit.html.erb page, and this is
what I see when I go to the edit page:

Session id: 1
Sesssion id class: Fixnum
Params id: 3
Params id class: String

(I added stars on either end to see if there was a space anywhere.)

7stud · September 2, 2011, 12:07pm

On Fri, Sep 2, 2011 at 4:35 PM, 7stud – [email protected] wrote:

def correct_user
end
and the test:
 redirect_to(root_path) unless get_user_from_session == user
end

and:

def correct_user
redirect_to(root_path) unless get_user_from_session.id.to_s ==
params[:id]
end

params[:id] may not always be an integer, it can be ‘123-user’

http://groups.google.com/group/rubyonrails-talk?hl=en.

–

7stud · September 2, 2011, 1:07pm

Jim ruther Nill wrote in post #1019766:

params[:id] may not always be an integer, it can be ‘123-user’

I don’t understand how that is relevant. The user signs in, which
stores the user id in the session. Then the user can go to the edit
page. However, a malicious user can look at the address in the address
bar on the edit page:

http://localhost:3000/users/1/edit

and change that to:

http://localhost:3000/users/3/edit

then refresh the page in an attempt to edit a different user’s info.
Hence, the reason for
the before filter. The before filter checks some info before
sending the edit page to the browser:
the user id in the session is compared to the user
id in the url. If the malicious user puts ‘123-user’ in the url, my
code should see that that id is not the same as the id stored in the
session, so instead of sending the edit page for the response, my code
redirects.

7stud · September 2, 2011, 1:26pm

My logic in the correct_user() before filter is: why should I have to
search the database for the user:

user = User.find(params[:id])

when I can just compare the id of the user that was retrieved using the
session info:

get_user_from_session.id.to_s

to the id in the url:

params[:id]

7stud · September 2, 2011, 1:50pm

On Fri, Sep 2, 2011 at 7:26 PM, 7stud – [email protected] wrote:

to the id in the url:

params[:id]

i’m assuming that get_user_from_session returns a User object.

get_user_from_session.id.to_s # User with ID = 1
User.find(‘1-user-name’) # User with ID = 1

so comparing params[:id] with the User ID converted to string will not
always be equal.

http://groups.google.com/group/rubyonrails-talk?hl=en.

–

7stud · September 2, 2011, 2:18pm

On Sep 2, 12:58pm, 7stud – [email protected] wrote:

get_user_from_session.id.to_s # User with ID = 1
User.find(‘1-user-name’) # User with ID = 1

so comparing params[:id] with the User ID converted to string will not
always be equal.

I understand what you are saying, I just don’t see how it’s relevant to
my problem.

I suspect that if you inspected the two things you are comparing you’d
see the relevance. I assume this is not using rails 3.1 ?

Fred

7stud · September 2, 2011, 1:58pm

Jim ruther Nill wrote in post #1019785:

On Fri, Sep 2, 2011 at 7:26 PM, 7stud – [email protected] wrote:

to the id in the url:

params[:id]

i’m assuming that get_user_from_session returns a User object.

Yes.

get_user_from_session.id.to_s # User with ID = 1
User.find(‘1-user-name’) # User with ID = 1

so comparing params[:id] with the User ID converted to string will not
always be equal.

I understand what you are saying, I just don’t see how it’s relevant to
my problem. As far as I can tell, your example demonstrates that if
the malicious user looks at the address in the address bar when he is
viewing his own edit page, and sees:

http://localhost:3000/users/1/edit

and he changes the url to:

http://localhost:3000/users/1-user-name/edit

and refreshes the page, he will be redirected because his user id=1
while params[:id] = ‘1-user-name’.

7stud · September 2, 2011, 2:49pm

Frederick C. wrote in post #1019792:

On Sep 2, 12:58pm, 7stud – [email protected] wrote:

get_user_from_session.id.to_s # User with ID = 1
User.find(‘1-user-name’) # User with ID = 1

so comparing params[:id] with the User ID converted to string will not
always be equal.

I understand what you are saying, I just don’t see how it’s relevant to
my problem.

I suspect that if you inspected the two things you are comparing you’d
see the relevance. I assume this is not using rails 3.1 ?

Fred

rails 3.0.9

Your response does point out a difference in the two code snippets I
posted:

case 1: get_user_from_session.id.to_s == params[:id]

user1 signs in, which puts his id in the session

If a malicious user goes to the edit page, and see this url in the
address bar:

http://localhost:3000/users/1/edit

and changes that to:

http://localhost:300/users/1-user-name/edit

and refreshes the page, he will be redirected because:

get_user_from_session.id.to_s will equal ‘1’

and:

params[:id] will equal ‘1-user-name’

case 2: get_user_from_session == User.find(params[:id])

user1 signs in, which puts his id in the session

If a malicious user goes to the edit page, and see this url in the
address bar:

http://localhost:3000/users/1/edit

and changes that to:

http://localhost:300/users/1-user-name/edit

and refreshes the page, he will be shown his own edit page
again because:

get_user_from_session will return user1

and:

User.find(params[:id]) will return user1

I really don’t care what happens in that case, and I don’t understand
why that would cause my tests to fail.

With both versions of the code, if the user changes the url from:

http://localhost:3000/users/1/edit

to:

http://localhost:3000/users/4/edit

then he will be redirected and not allowed to edit user4’s info.

7stud · September 3, 2011, 7:14am

Frederick C. wrote in post #1019805:

On Sep 2, 1:49pm, 7stud – [email protected] wrote:

User.find(params[:id]) will return user1

I really don’t care what happens in that case, and I don’t understand
why that would cause my tests to fail.

You’re missing the point.

Apparently.

The point is that in your tests params[:id]
isn’t a string

The term params[:id] doesn’t appear in my tests, so I have no idea what
you are talking about. And in the before filter, I’ve showed that
params[:id] is a string:

Session id: 1
Sesssion id class: Fixnum
Params id: 3
Params id class: String

7stud · September 2, 2011, 3:14pm

On Sep 2, 1:49pm, 7stud – [email protected] wrote:

User.find(params[:id]) will return user1

I really don’t care what happens in that case, and I don’t understand
why that would cause my tests to fail.

You’re missing the point. The point is that in your tests params[:id]
isn’t a string (because pre rails 3.1 rails didn’t force this and
would allow you to create params that couldn’t exist in the real
world)

Fred

7stud · September 4, 2011, 2:01pm

See my first post.

7stud · September 3, 2011, 10:05am

On Sep 3, 6:14am, 7stud – [email protected] wrote:

Apparently.

The point is that in your tests params[:id]
isn’t a string

The term params[:id] doesn’t appear in my tests, so I have no idea what
you are talking about.

I was talking about the value of params[:id] in your controller action
when the test is run.

And in the before filter, I’ve showed that
params[:id] is a string:

Session id: 1
Sesssion id class: Fixnum
Params id: 3
Params id class: String

Was this output produced when running the tests or when using the page
from a browser?

Fred

7stud · September 7, 2011, 4:23pm

Anyone have an explanation for the behavior I’m seeing?

7stud · September 7, 2011, 4:54pm

On 7 September 2011 15:23, 7stud – [email protected] wrote:

Anyone have any explanation for the behavior I’m seeing?

What behaviour? You have not quoted any previous message. Looking
back at the thread though I see you do not appear to have answered
Fred’s last questions. If he cannot help you then I doubt whether
anyone here can.

–
Colin
https://plus.google.com/111605230843729345543/

Before filter question

./spec/controllers/users_controller_spec.rb:15:in `block (3