ruby on rails - How to trust an association id parameter?

Question

Here's an example of where we need to trust that the conversation_id wasn't altered by the user:

# messages_controller.rb
def create
  @message = Message.new(
    body: message_params[:body], # trustworthy
    user_id: current_user.id, # trustworthy
    conversation_id: message_params[:conversation_id] # not trustworthy!
    )
  @message.save
end

So I considered wrapping the above code in an if statement like so

# messages_controller.rb
def create
  if current_user.conversations.pluck(:id).include? message_params[:conversation_id]
    @message = Message.new(
      body: message_params[:body], 
      user_id: current_user.id, 
      conversation_id: message_params[:conversation_id] 
      )
    @message.save
  end
end

This is the only way I can think of to ensure that the conversation is actually one that the user belongs to (failing to check this carefully could result in a malicious user successfully writing messages to other people's conversations!)

Since this type of check must be fairly common, I just want to know have I done it effectively and efficiently, or is there a better way or more 'rails way'?

I should also add, I have cancan protecting the create method when the message doesn't belong to a conversation involving the user (which should completely prevent mischief on its own) AND I'm using uuid's on the conversation_id (which I know isn't really protection, but it all helps). But I still want to know how I can do this without those protections so as to add depth.

question from:https://stackoverflow.com/questions/65921087/how-to-trust-an-association-id-parameter

Answer 1

The rails way to do this would be to use a nested route instead of passing conversation_id through the request body:

resources :conversations do
  resources :messages, shallow: true
end

class MessagesController < ApplicationController
  # POST /conversations/:conversation_id/messages
  def create
    @conversation = current_user.conversations
                                .find(params[:conversation_id])
    @message = @conversation.messages.new(message_params) do |m|
      m.user = current_user
    end

    if @message.save
      redirect_to @conversation
    else
      render :new
    end
  end

  private
  def message_parameters
    params.require(:message)
          .permit(:body)
  end
end

This works - but its pretty far from perfect. If @conversation = current_user.conversations.find(params[:conversation_id]) does not find a record we get an ActiveRecord::RecordNotFound exception and 404 response instead of a check if the user is authorized to post to that conversation.

A better solution would be to use something like:

@conversation = Conversation.find(params[:conversation_id])
unless conversations.users.exist?(id: current_user.id)
  raise SomeKindOfAuthenticationError
end

Of course you should really be using something like Pundit instead of reinventing the wheel here.