1

There is so much written about the security threat of attr_accessible that I am beginning to wonder if I should even have any attributes in it. Here is the issue. I have a Message model which has the following:

attr_accessible :body,:sender_id,:recipient_id

I do not have the update or edit action in my messages_controller. With the new and create action I am able to create a new message and send it to a recipient. Only users who have logged in and meet certain conditions can message each other. I do that with the help of a before_filter and the conditions work fine. The message is stored and can be viewed by the sender and the recipient. Perfect!

The question I have is that since :body,:sender_id,:recipient_id are included in attr_accessible, can a malicious user somehow change the :body,:sender_id,:recipient_id of the original message? Should I just add these attributes to attr_readonly as well so they cannot be modified once saved?

This question has been haunting me for practically all my models.

pratski
  • 1,448
  • 2
  • 22
  • 37

2 Answers2

3

can a malicious user somehow change the :body,:sender_id,:recipient_id of the original message?

This would depend on other things rather than attr_accesible. attr_accesible will only filter which fields are allowed to be updated using mass assignment. Since you say you don't have any update action, then no, there is now way a user can edit a message since you always create a new Message through you create action.

But there is something you need to care about. What is sender_id? If you do have users in your app and they send messages to each others, then sender_id should not be an accessible field, since this will allow users to send messages on behalf of other users. You probably want to keep that field off the attr_accessible list and do something like this:

m = Message.new params[:message] # body and recipient_id
m.sender_id = current_user.id # this is not mass assignment
m.save
.....
aromero
  • 25,681
  • 6
  • 57
  • 79
  • sender.id == current_user.id. That is how I have it currently. Is there a threat despite this? – pratski May 13 '13 at 13:56
  • 1
    Why would you put sender_id in the list then? This should not be a field allowed to be mass assignable. – aromero May 13 '13 at 14:00
1

Well, it depends on how your are creating your model's instance. If you use:

FooModel.create(params[:foo])

then yes, your are not secure because a logged in user may pass additional parameters to the request even if you don't provide explicitly form fields for those attributes.

So, for your case, anyone posting to your "create" action with sender_id, recipient_id (values in the request) will be able to change them unless you take care about this assignments in your action.

Lazarus Lazaridis
  • 5,803
  • 2
  • 21
  • 35
  • Got it. Thanks. I do care of assignments in the action. For example, the sender is the current_user, whereas the recipient is obtained through params. But the recipient is obtained through the hidden field and has a set user. – pratski May 13 '13 at 13:58
  • 1
    Then you should remove the sender_id from the attr_accessible and set it explicitly in you action. – Lazarus Lazaridis May 13 '13 at 14:00