Enhance message protection during conversation updates and adjust permitted attributes

Author: rsmithlal

app/controllers/better_together/conversations_controller.rb

@@ -233,10 +233,36 @@ def conversation_params_filtered # rubocop:todo Metrics/AbcSize
       permitted_ids = permitted.pluck(:id)
       # Always allow the current person (creator/participant) to appear in the list
       permitted_ids << helpers.current_person.id if helpers.current_person
+
       cp = conversation_params.dup
+
+      # Filter participant_ids to only those the agent may message
       if cp[:participant_ids].present?
         cp[:participant_ids] = Array(cp[:participant_ids]).map(&:presence).compact & permitted_ids
       end
+
+      # Protect nested messages on update: only allow creating messages via the create path.
+      # On update, permit edits only to existing messages that belong to the current person,
+      # and only allow their content (prevent sender_id spoofing or reassigning other people's messages).
+      if action_name == 'update' && cp[:messages_attributes].present?
+        safe_messages = Array(cp[:messages_attributes]).map do |m|
+          # handle ActionController::Parameters or Hash
+          attrs = m.respond_to?(:to_h) ? m.to_h : m
+          msg_id = attrs['id'] || attrs[:id]
+          next nil unless msg_id
+
+          msg = BetterTogether::Message.find_by(id: msg_id)
+          next nil unless msg && helpers.current_person && msg.sender_id == helpers.current_person.id
+
+          # Only allow content edits through this path
+          { 'id' => msg_id, 'content' => attrs['content'] || attrs[:content] }
+        end.compact
+
+        # Replace messages_attributes with the vetted set (or nil if none)
+        cp[:messages_attributes] = safe_messages.presence
+      end
+
+      # On create, leave messages_attributes as-is so nested creation works; controller will set sender.
       cp
     end
 

app/models/better_together/message.rb

@@ -15,7 +15,7 @@ class Message < ApplicationRecord
     # Attributes permitted for strong parameters
     def self.permitted_attributes
       # include id and _destroy for nested attributes handling
-      %i[id sender_id].push(:content).push(:_destroy)
+  %i[id content _destroy]
     end
     # def content
     #   super || self[:content]

spec/requests/better_together/conversation_message_protection_spec.rb

@@ -0,0 +1,51 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe 'Conversation message protection', type: :request do
+  include RequestSpecHelper
+
+  it "prevents a user from altering another user's message via conversation update" do
+  # Setup: ensure host platform exists and create users with known passwords
+  configure_host_platform
+    
+    # Setup: create a manager user (owner of the conversation) and another user
+  manager_user = create(:user, :confirmed, :platform_manager, email: '[email protected]', password: 'password12345')
+  other_user = create(:user, :confirmed, email: '[email protected]', password: 'password12345')
+
+    # Create a conversation as the manager with a nested message
+  login(manager_user.email, 'password12345')
+
+    post better_together.conversations_path(locale: I18n.default_locale), params: {
+      conversation: {
+        title: 'Protected convo',
+        participant_ids: [manager_user.person.id, other_user.person.id],
+        messages_attributes: [
+          { content: 'Original message' }
+        ]
+      }
+    }
+
+    expect(response).to have_http_status(:found)
+    conversation = BetterTogether::Conversation.order(created_at: :desc).first
+    message = conversation.messages.first
+    expect(message.content.to_plain_text).to include('Original message')
+
+    # Now sign in as other_user and attempt to change manager's message via PATCH
+  logout
+  login(other_user.email, 'password12345')
+
+    patch better_together.conversation_path(conversation, locale: I18n.default_locale), params: {
+      conversation: {
+        title: conversation.title,
+        messages_attributes: [
+          { id: message.id, content: 'Tampered message', sender_id: other_user.person.id }
+        ]
+      }
+    }
+
+    # Reload message and assert it was not changed
+    message.reload
+    expect(message.content.to_plain_text).to include('Original message')
+  end
+end