is this a secure approach in ActiveRecords in Rails?

Question

Hello,

I am using the following for my customers to unsubscribe from my mailing list;

  def index
    @user = User.find_by_salt(params[:subscribe_code]) 
    if @user.nil? 
      flash[:notice] = "the link is not valid...."
      render :action => 'index'
    else    
      Notification.delete_all(:user_id => @user.id)
      flash[:notice] = "you have been unsubscribed....."
      redirect_to :controller => 'home'
    end 
  end 

my link looks like; http://site.com/unsubscribe/32hj5h2j33j3h333

so the above compares the random string to a field in my user table and accordingly deletes data from the notification table.

My question; is this approach secure? is there a better/more efficient way for doing this?

All suggestions are welcome.

Answer 1

I think this is safer:

@user = User.find :all, :conditions => { salt => params[:subscribe_code] }

That way you are sure that Rails knows params[:subscribe_code] is to be escaped. There are probably lots of ways to write it that would work though.

Answer 2

I don't see anything wrong in your solution if the action doesn't require authentication.

If the action requires authentication then I would make sure that the salt belongs to the current user

@user = User.find_by_id_and_salt(current_user.id, params[:subscribe_code])

Answer 3

your link should be

http://site.com/unsubscribe/?subscribe_code=32hj5h2j33j3h333

otherwise "32hj5h2j33j3h333" will get as a params[:id]

else is fine. Assuming subscribe_code will be unique.

Answer 4

Is it all that important that the user be informed if their unsubscribe link was wrong? What are the chances of that anyway? Wasn't it generated programmatically and that program is tested? If the answer is "yes" (hint: it should be) then my suggestion would be to always tell the user that they've unsubscribed, regardless of what happened.

def index
  begin
    @user = User.find_by_salt!(params[:subscribe_code]) 
  rescue ActiveRecord::RecordNotFound
  ensure
    @user.notifications.delete_all if @user
    flash[:notice] = "You have been unsubscribed."
    redirect_to :action => "index"
  end
end