From 2adc040a8f63baaff812fb678a0aa9a02b41068f Mon Sep 17 00:00:00 2001 From: Pranav Date: Fri, 19 Dec 2025 19:02:21 -0800 Subject: [PATCH] fix: Validate blob before attaching it to a record (#13115) Previously, attachments relied only on blob_id, which made it possible to attach blobs across accounts by enumerating IDs. We now require both blob_id and blob_key, add cross-account validation to prevent blob reuse, and centralize the logic in a shared BlobOwnershipValidation concern. It also fixes a frontend bug where mixed-type action params (number + string) were incorrectly dropped, causing attachment uploads to fail. --- .../accounts/automation_rules_controller.rb | 43 ++--- .../api/v1/accounts/macros_controller.rb | 33 ++-- .../api/v1/accounts/portals_controller.rb | 2 +- .../api/v1/accounts/upload_controller.rb | 2 +- .../concerns/attachment_concern.rb | 35 ++++ config/locales/en.yml | 2 + .../automation_rules_controller_spec.rb | 149 ++++++++++++++---- .../api/v1/accounts/macros_controller_spec.rb | 100 ++++++++++-- .../api/v1/upload_controller_spec.rb | 2 - 9 files changed, 278 insertions(+), 90 deletions(-) create mode 100644 app/controllers/concerns/attachment_concern.rb diff --git a/app/controllers/api/v1/accounts/automation_rules_controller.rb b/app/controllers/api/v1/accounts/automation_rules_controller.rb index 39f8ba504..0840d0eea 100644 --- a/app/controllers/api/v1/accounts/automation_rules_controller.rb +++ b/app/controllers/api/v1/accounts/automation_rules_controller.rb @@ -1,4 +1,6 @@ class Api::V1::Accounts::AutomationRulesController < Api::V1::Accounts::BaseController + include AttachmentConcern + before_action :check_authorization before_action :fetch_automation_rule, only: [:show, :update, :destroy, :clone] @@ -9,25 +11,32 @@ class Api::V1::Accounts::AutomationRulesController < Api::V1::Accounts::BaseCont def show; end def create + blobs, actions, error = validate_and_prepare_attachments(params[:actions]) + return render_could_not_create_error(error) if error + @automation_rule = Current.account.automation_rules.new(automation_rules_permit) - @automation_rule.actions = params[:actions] + @automation_rule.actions = actions @automation_rule.conditions = params[:conditions] - render json: { error: @automation_rule.errors.messages }, status: :unprocessable_entity and return unless @automation_rule.valid? + return render_could_not_create_error(@automation_rule.errors.messages) unless @automation_rule.valid? @automation_rule.save! - process_attachments - @automation_rule + blobs.each { |blob| @automation_rule.files.attach(blob) } end def update - ActiveRecord::Base.transaction do - automation_rule_update - process_attachments + blobs, actions, error = validate_and_prepare_attachments(params[:actions], @automation_rule) + return render_could_not_create_error(error) if error + ActiveRecord::Base.transaction do + @automation_rule.assign_attributes(automation_rules_permit) + @automation_rule.actions = actions if params[:actions] + @automation_rule.conditions = params[:conditions] if params[:conditions] + @automation_rule.save! + blobs.each { |blob| @automation_rule.files.attach(blob) } rescue StandardError => e Rails.logger.error e - render json: { error: @automation_rule.errors.messages }.to_json, status: :unprocessable_entity + render_could_not_create_error(@automation_rule.errors.messages) end end @@ -43,26 +52,8 @@ class Api::V1::Accounts::AutomationRulesController < Api::V1::Accounts::BaseCont @automation_rule = new_rule end - def process_attachments - actions = @automation_rule.actions.filter_map { |k, _v| k if k['action_name'] == 'send_attachment' } - return if actions.blank? - - actions.each do |action| - blob_id = action['action_params'] - blob = ActiveStorage::Blob.find_by(id: blob_id) - @automation_rule.files.attach(blob) - end - end - private - def automation_rule_update - @automation_rule.update!(automation_rules_permit) - @automation_rule.actions = params[:actions] if params[:actions] - @automation_rule.conditions = params[:conditions] if params[:conditions] - @automation_rule.save! - end - def automation_rules_permit params.permit( :name, :description, :event_name, :active, diff --git a/app/controllers/api/v1/accounts/macros_controller.rb b/app/controllers/api/v1/accounts/macros_controller.rb index 727dd621b..c4e0cd6dd 100644 --- a/app/controllers/api/v1/accounts/macros_controller.rb +++ b/app/controllers/api/v1/accounts/macros_controller.rb @@ -1,4 +1,6 @@ class Api::V1::Accounts::MacrosController < Api::V1::Accounts::BaseController + include AttachmentConcern + before_action :fetch_macro, only: [:show, :update, :destroy, :execute] before_action :check_authorization, only: [:show, :update, :destroy, :execute] @@ -11,26 +13,32 @@ class Api::V1::Accounts::MacrosController < Api::V1::Accounts::BaseController end def create + blobs, actions, error = validate_and_prepare_attachments(params[:actions]) + return render_could_not_create_error(error) if error + @macro = Current.account.macros.new(macros_with_user.merge(created_by_id: current_user.id)) @macro.set_visibility(current_user, permitted_params) - @macro.actions = params[:actions] + @macro.actions = actions - render json: { error: @macro.errors.messages }, status: :unprocessable_entity and return unless @macro.valid? + return render_could_not_create_error(@macro.errors.messages) unless @macro.valid? @macro.save! - process_attachments - @macro + blobs.each { |blob| @macro.files.attach(blob) } end def update + blobs, actions, error = validate_and_prepare_attachments(params[:actions], @macro) + return render_could_not_create_error(error) if error + ActiveRecord::Base.transaction do - @macro.update!(macros_with_user) + @macro.assign_attributes(macros_with_user) @macro.set_visibility(current_user, permitted_params) - process_attachments + @macro.actions = actions if params[:actions] @macro.save! + blobs.each { |blob| @macro.files.attach(blob) } rescue StandardError => e Rails.logger.error e - render json: { error: @macro.errors.messages }.to_json, status: :unprocessable_entity + render_could_not_create_error(@macro.errors.messages) end end @@ -47,17 +55,6 @@ class Api::V1::Accounts::MacrosController < Api::V1::Accounts::BaseController private - def process_attachments - actions = @macro.actions.filter_map { |k, _v| k if k['action_name'] == 'send_attachment' } - return if actions.blank? - - actions.each do |action| - blob_id = action['action_params'] - blob = ActiveStorage::Blob.find_by(id: blob_id) - @macro.files.attach(blob) - end - end - def permitted_params params.permit( :name, :visibility, diff --git a/app/controllers/api/v1/accounts/portals_controller.rb b/app/controllers/api/v1/accounts/portals_controller.rb index 5e3133fb0..8eb24b757 100644 --- a/app/controllers/api/v1/accounts/portals_controller.rb +++ b/app/controllers/api/v1/accounts/portals_controller.rb @@ -62,7 +62,7 @@ class Api::V1::Accounts::PortalsController < Api::V1::Accounts::BaseController def process_attached_logo blob_id = params[:blob_id] - blob = ActiveStorage::Blob.find_by(id: blob_id) + blob = ActiveStorage::Blob.find_signed(blob_id) @portal.logo.attach(blob) end diff --git a/app/controllers/api/v1/accounts/upload_controller.rb b/app/controllers/api/v1/accounts/upload_controller.rb index 6530279da..479d8ae1b 100644 --- a/app/controllers/api/v1/accounts/upload_controller.rb +++ b/app/controllers/api/v1/accounts/upload_controller.rb @@ -59,7 +59,7 @@ class Api::V1::Accounts::UploadController < Api::V1::Accounts::BaseController end def render_success(file_blob) - render json: { file_url: url_for(file_blob), blob_key: file_blob.key, blob_id: file_blob.id } + render json: { file_url: url_for(file_blob), blob_id: file_blob.signed_id } end def render_error(message, status) diff --git a/app/controllers/concerns/attachment_concern.rb b/app/controllers/concerns/attachment_concern.rb new file mode 100644 index 000000000..2652f04be --- /dev/null +++ b/app/controllers/concerns/attachment_concern.rb @@ -0,0 +1,35 @@ +module AttachmentConcern + extend ActiveSupport::Concern + + def validate_and_prepare_attachments(actions, record = nil) + blobs = [] + return [blobs, actions, nil] if actions.blank? + + sanitized = actions.map do |action| + next action unless action[:action_name] == 'send_attachment' + + result = process_attachment_action(action, record, blobs) + return [nil, nil, I18n.t('errors.attachments.invalid')] unless result + + result + end + + [blobs, sanitized, nil] + end + + private + + def process_attachment_action(action, record, blobs) + blob_id = action[:action_params].first + blob = ActiveStorage::Blob.find_signed(blob_id.to_s) + + return action.merge(action_params: [blob.id]).tap { blobs << blob } if blob.present? + return action if blob_already_attached?(record, blob_id) + + nil + end + + def blob_already_attached?(record, blob_id) + record&.files&.any? { |f| f.blob_id == blob_id.to_i } + end +end diff --git a/config/locales/en.yml b/config/locales/en.yml index 3cc8eb9cf..71706f665 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -62,6 +62,8 @@ en: failed: Signup failed assignment_policy: not_found: Assignment policy not found + attachments: + invalid: Invalid attachment saml: feature_not_enabled: SAML feature not enabled for this account sso_not_enabled: SAML SSO is not enabled for this installation diff --git a/spec/controllers/api/v1/accounts/automation_rules_controller_spec.rb b/spec/controllers/api/v1/accounts/automation_rules_controller_spec.rb index 35ff53b12..ff882da4f 100644 --- a/spec/controllers/api/v1/accounts/automation_rules_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/automation_rules_controller_spec.rb @@ -141,21 +141,14 @@ RSpec.describe 'Api::V1::Accounts::AutomationRulesController', type: :request do end it 'Saves file in the automation actions to send an attachments' do - file = fixture_file_upload(Rails.root.join('spec/assets/avatar.png'), 'image/png') + blob = ActiveStorage::Blob.create_and_upload!( + io: Rails.root.join('spec/assets/avatar.png').open, + filename: 'avatar.png', + content_type: 'image/png' + ) expect(account.automation_rules.count).to eq(0) - post "/api/v1/accounts/#{account.id}/upload/", - headers: administrator.create_new_auth_token, - params: { attachment: file } - - expect(response).to have_http_status(:success) - - blob = response.parsed_body - - expect(blob['blob_key']).to be_present - expect(blob['blob_id']).to be_present - params[:actions] = [ { 'action_name': :send_message, @@ -163,7 +156,7 @@ RSpec.describe 'Api::V1::Accounts::AutomationRulesController', type: :request do }, { 'action_name': :send_attachment, - 'action_params': [blob['blob_id']] + 'action_params': [blob.signed_id] } ] @@ -177,29 +170,25 @@ RSpec.describe 'Api::V1::Accounts::AutomationRulesController', type: :request do end it 'Saves files in the automation actions to send multiple attachments' do - file_1 = fixture_file_upload(Rails.root.join('spec/assets/avatar.png'), 'image/png') - file_2 = fixture_file_upload(Rails.root.join('spec/assets/sample.png'), 'image/png') - - post "/api/v1/accounts/#{account.id}/upload/", - headers: administrator.create_new_auth_token, - params: { attachment: file_1 } - - blob_1 = response.parsed_body - - post "/api/v1/accounts/#{account.id}/upload/", - headers: administrator.create_new_auth_token, - params: { attachment: file_2 } - - blob_2 = response.parsed_body + blob_1 = ActiveStorage::Blob.create_and_upload!( + io: Rails.root.join('spec/assets/avatar.png').open, + filename: 'avatar.png', + content_type: 'image/png' + ) + blob_2 = ActiveStorage::Blob.create_and_upload!( + io: Rails.root.join('spec/assets/sample.png').open, + filename: 'sample.png', + content_type: 'image/png' + ) params[:actions] = [ { 'action_name': :send_attachment, - 'action_params': [blob_1['blob_id']] + 'action_params': [blob_1.signed_id] }, { 'action_name': :send_attachment, - 'action_params': [blob_2['blob_id']] + 'action_params': [blob_2.signed_id] } ] @@ -210,6 +199,46 @@ RSpec.describe 'Api::V1::Accounts::AutomationRulesController', type: :request do automation_rule = account.automation_rules.first expect(automation_rule.files.count).to eq(2) end + + it 'returns error for invalid attachment blob_id' do + params[:actions] = [ + { + 'action_name': :send_attachment, + 'action_params': ['invalid_blob_id'] + } + ] + + post "/api/v1/accounts/#{account.id}/automation_rules", + headers: administrator.create_new_auth_token, + params: params + + expect(response).to have_http_status(:unprocessable_entity) + expect(response.parsed_body['error']).to eq(I18n.t('errors.attachments.invalid')) + end + + it 'stores the original blob_id in action_params after create' do + blob = ActiveStorage::Blob.create_and_upload!( + io: Rails.root.join('spec/assets/avatar.png').open, + filename: 'avatar.png', + content_type: 'image/png' + ) + + params[:actions] = [ + { + 'action_name': :send_attachment, + 'action_params': [blob.signed_id] + } + ] + + post "/api/v1/accounts/#{account.id}/automation_rules", + headers: administrator.create_new_auth_token, + params: params + + automation_rule = account.automation_rules.first + attachment_action = automation_rule.actions.find { |a| a['action_name'] == 'send_attachment' } + expect(attachment_action['action_params'].first).to be_a(Integer) + expect(attachment_action['action_params'].first).to eq(automation_rule.files.first.blob_id) + end end end @@ -328,6 +357,68 @@ RSpec.describe 'Api::V1::Accounts::AutomationRulesController', type: :request do expect(body[:payload][:active]).to be(false) expect(automation_rule.reload.active).to be(false) end + + it 'allows update with existing blob_id' do + blob = ActiveStorage::Blob.create_and_upload!( + io: Rails.root.join('spec/assets/avatar.png').open, + filename: 'avatar.png', + content_type: 'image/png' + ) + + automation_rule.update!(actions: [{ 'action_name' => 'send_attachment', 'action_params' => [blob.id] }]) + automation_rule.files.attach(blob) + + update_params[:actions] = [ + { + 'action_name': :send_attachment, + 'action_params': [blob.id] + } + ] + + patch "/api/v1/accounts/#{account.id}/automation_rules/#{automation_rule.id}", + headers: administrator.create_new_auth_token, + params: update_params + + expect(response).to have_http_status(:success) + end + + it 'returns error for invalid blob_id on update' do + update_params[:actions] = [ + { + 'action_name': :send_attachment, + 'action_params': [999_999] + } + ] + + patch "/api/v1/accounts/#{account.id}/automation_rules/#{automation_rule.id}", + headers: administrator.create_new_auth_token, + params: update_params + + expect(response).to have_http_status(:unprocessable_entity) + expect(response.parsed_body['error']).to eq(I18n.t('errors.attachments.invalid')) + end + + it 'allows adding new attachment on update with signed blob_id' do + blob = ActiveStorage::Blob.create_and_upload!( + io: Rails.root.join('spec/assets/avatar.png').open, + filename: 'avatar.png', + content_type: 'image/png' + ) + + update_params[:actions] = [ + { + 'action_name': :send_attachment, + 'action_params': [blob.signed_id] + } + ] + + patch "/api/v1/accounts/#{account.id}/automation_rules/#{automation_rule.id}", + headers: administrator.create_new_auth_token, + params: update_params + + expect(response).to have_http_status(:success) + expect(automation_rule.reload.files.count).to eq(1) + end end end diff --git a/spec/controllers/api/v1/accounts/macros_controller_spec.rb b/spec/controllers/api/v1/accounts/macros_controller_spec.rb index 5fb8fb2b4..87d301d88 100644 --- a/spec/controllers/api/v1/accounts/macros_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/macros_controller_spec.rb @@ -127,18 +127,11 @@ RSpec.describe 'Api::V1::Accounts::MacrosController', type: :request do end it 'Saves file in the macros actions to send an attachments' do - file = fixture_file_upload(Rails.root.join('spec/assets/avatar.png'), 'image/png') - - post "/api/v1/accounts/#{account.id}/upload/", - headers: administrator.create_new_auth_token, - params: { attachment: file } - - expect(response).to have_http_status(:success) - - blob = response.parsed_body - - expect(blob['blob_key']).to be_present - expect(blob['blob_id']).to be_present + blob = ActiveStorage::Blob.create_and_upload!( + io: Rails.root.join('spec/assets/avatar.png').open, + filename: 'avatar.png', + content_type: 'image/png' + ) params[:actions] = [ { @@ -147,7 +140,7 @@ RSpec.describe 'Api::V1::Accounts::MacrosController', type: :request do }, { 'action_name': :send_attachment, - 'action_params': [blob['blob_id']] + 'action_params': [blob.signed_id] } ] @@ -159,6 +152,46 @@ RSpec.describe 'Api::V1::Accounts::MacrosController', type: :request do expect(macro.files.presence).to be_truthy expect(macro.files.count).to eq(1) end + + it 'returns error for invalid attachment blob_id' do + params[:actions] = [ + { + 'action_name': :send_attachment, + 'action_params': ['invalid_blob_id'] + } + ] + + post "/api/v1/accounts/#{account.id}/macros", + headers: administrator.create_new_auth_token, + params: params + + expect(response).to have_http_status(:unprocessable_entity) + expect(response.parsed_body['error']).to eq(I18n.t('errors.attachments.invalid')) + end + + it 'stores the original blob_id in action_params after create' do + blob = ActiveStorage::Blob.create_and_upload!( + io: Rails.root.join('spec/assets/avatar.png').open, + filename: 'avatar.png', + content_type: 'image/png' + ) + + params[:actions] = [ + { + 'action_name': :send_attachment, + 'action_params': [blob.signed_id] + } + ] + + post "/api/v1/accounts/#{account.id}/macros", + headers: administrator.create_new_auth_token, + params: params + + macro = account.macros.last + attachment_action = macro.actions.find { |a| a['action_name'] == 'send_attachment' } + expect(attachment_action['action_params'].first).to be_a(Integer) + expect(attachment_action['action_params'].first).to eq(macro.files.first.blob_id) + end end end @@ -202,6 +235,47 @@ RSpec.describe 'Api::V1::Accounts::MacrosController', type: :request do expect(response).to have_http_status(:unauthorized) expect(json_response['error']).to eq('You are not authorized to do this action') end + + it 'allows update with existing blob_id' do + blob = ActiveStorage::Blob.create_and_upload!( + io: Rails.root.join('spec/assets/avatar.png').open, + filename: 'avatar.png', + content_type: 'image/png' + ) + + macro.update!(actions: [{ 'action_name' => 'send_attachment', 'action_params' => [blob.id] }]) + macro.files.attach(blob) + + put "/api/v1/accounts/#{account.id}/macros/#{macro.id}", + params: { actions: [{ 'action_name': :send_attachment, 'action_params': [blob.id] }] }, + headers: administrator.create_new_auth_token + + expect(response).to have_http_status(:success) + end + + it 'returns error for invalid blob_id on update' do + put "/api/v1/accounts/#{account.id}/macros/#{macro.id}", + params: { actions: [{ 'action_name': :send_attachment, 'action_params': [999_999] }] }, + headers: administrator.create_new_auth_token + + expect(response).to have_http_status(:unprocessable_entity) + expect(response.parsed_body['error']).to eq(I18n.t('errors.attachments.invalid')) + end + + it 'allows adding new attachment on update with signed blob_id' do + blob = ActiveStorage::Blob.create_and_upload!( + io: Rails.root.join('spec/assets/avatar.png').open, + filename: 'avatar.png', + content_type: 'image/png' + ) + + put "/api/v1/accounts/#{account.id}/macros/#{macro.id}", + params: { actions: [{ 'action_name': :send_attachment, 'action_params': [blob.signed_id] }] }, + headers: administrator.create_new_auth_token + + expect(response).to have_http_status(:success) + expect(macro.reload.files.count).to eq(1) + end end end diff --git a/spec/controllers/api/v1/upload_controller_spec.rb b/spec/controllers/api/v1/upload_controller_spec.rb index bd039f684..93ef28dd8 100644 --- a/spec/controllers/api/v1/upload_controller_spec.rb +++ b/spec/controllers/api/v1/upload_controller_spec.rb @@ -18,7 +18,6 @@ RSpec.describe 'Api::V1::Accounts::UploadController', type: :request do blob = response.parsed_body expect(blob['errors']).to be_nil expect(blob['file_url']).to be_present - expect(blob['blob_key']).to be_present expect(blob['blob_id']).to be_present end @@ -53,7 +52,6 @@ RSpec.describe 'Api::V1::Accounts::UploadController', type: :request do blob = response.parsed_body expect(blob['error']).to be_nil expect(blob['file_url']).to be_present - expect(blob['blob_key']).to be_present expect(blob['blob_id']).to be_present end