From 6a9c44476e2293542c036bffe845c7bb4160ce47 Mon Sep 17 00:00:00 2001 From: Muhsin Keloth Date: Tue, 21 Apr 2026 15:55:12 +0400 Subject: [PATCH 001/201] feat(super-admin): Add push diagnostics tool (#14105) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We're getting many customer reports saying "I'm not getting notifications." We can't always identify the root cause since there are multiple points of failure. Added a **Push Diagnostics** tool in Super Admin to help us investigate mobile/web push issues. Here's how it works: - Look up a user by email/ID → see all their registered subscriptions with device info (iOS/Android, brand, model), token freshness, and last-updated time - Send a customizable test push and read the raw FCM/web-push/relay response to see if the customer is receiving push notifications—if not, it will show proper errors. - Delete broken subscriptions so the mobile app re-registers on next launch CleanShot 2026-04-20 at 12 56 56@2x Fixes https://linear.app/chatwoot/issue/CW-6892/push-diagnostics-tool --------- Co-authored-by: Muhsin <12408980+muhsin-k@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) --- .../push_diagnostics_controller.rb | 68 +++++++ .../notification/push_test_service.rb | 160 +++++++++++++++ .../application/_navigation.html.erb | 3 +- .../push_diagnostics/show.html.erb | 190 ++++++++++++++++++ config/locales/en.yml | 6 + config/routes.rb | 3 + lib/chatwoot_hub.rb | 8 +- 7 files changed, 435 insertions(+), 3 deletions(-) create mode 100644 app/controllers/super_admin/push_diagnostics_controller.rb create mode 100644 app/services/notification/push_test_service.rb create mode 100644 app/views/super_admin/push_diagnostics/show.html.erb diff --git a/app/controllers/super_admin/push_diagnostics_controller.rb b/app/controllers/super_admin/push_diagnostics_controller.rb new file mode 100644 index 000000000..c33cfdc1e --- /dev/null +++ b/app/controllers/super_admin/push_diagnostics_controller.rb @@ -0,0 +1,68 @@ +class SuperAdmin::PushDiagnosticsController < SuperAdmin::ApplicationController + def show + @query = params[:user_query].to_s.strip + @user = resolve_user(@query) + @subscriptions = @user ? @user.notification_subscriptions.order(:id) : [] + @results = [] + end + + def create + @user = User.find_by(id: params[:user_id]) + return redirect_to super_admin_push_diagnostics_path, alert: I18n.t('super_admin.push_diagnostics.user_not_found') if @user.nil? + + ids = parsed_subscription_ids + if ids.empty? + return redirect_to super_admin_push_diagnostics_path(user_query: @user.id), + alert: I18n.t('super_admin.push_diagnostics.no_subscriptions_to_test') + end + + run_test_and_render(ids) + end + + def destroy_subscriptions + user = User.find_by(id: params[:user_id]) + return redirect_to super_admin_push_diagnostics_path, alert: I18n.t('super_admin.push_diagnostics.user_not_found') if user.nil? + + ids = parsed_subscription_ids + if ids.empty? + return redirect_to super_admin_push_diagnostics_path(user_query: user.id), + alert: I18n.t('super_admin.push_diagnostics.no_subscriptions_to_delete') + end + + deleted_count = user.notification_subscriptions.where(id: ids).destroy_all.size + log_super_admin_action("deleted #{deleted_count} subscriptions for user #{user.id}: #{ids}") + redirect_to super_admin_push_diagnostics_path(user_query: user.id), + notice: I18n.t('super_admin.push_diagnostics.subscriptions_deleted', count: deleted_count) + end + + private + + def run_test_and_render(ids) + @query = @user.id.to_s + @subscriptions = @user.notification_subscriptions.order(:id) + @results = Notification::PushTestService.new( + user: @user, subscription_ids: ids, + title: params[:push_title], body: params[:push_body] + ).perform + + log_super_admin_action("test sent for user #{@user.id} subscriptions #{ids}") + render :show + end + + def log_super_admin_action(message) + Rails.logger.info( + "[SuperAdmin] push diagnostics #{message} " \ + "(actor_id=#{current_super_admin&.id}, actor_email=#{current_super_admin&.email})" + ) + end + + def resolve_user(query) + return if query.blank? + + query.match?(/\A\d+\z/) ? User.find_by(id: query) : User.from_email(query) + end + + def parsed_subscription_ids + Array(params[:subscription_ids]).reject(&:blank?).map(&:to_i) + end +end diff --git a/app/services/notification/push_test_service.rb b/app/services/notification/push_test_service.rb new file mode 100644 index 000000000..4b2bec177 --- /dev/null +++ b/app/services/notification/push_test_service.rb @@ -0,0 +1,160 @@ +class Notification::PushTestService + pattr_initialize [:user!, :subscription_ids!, :title, :body] + + DEFAULT_TITLE = '%s notification test'.freeze + DEFAULT_BODY = 'This is a test from our team to check notification delivery on your device. No action needed.'.freeze + + def self.default_title + format(DEFAULT_TITLE, installation_name: GlobalConfigService.load('INSTALLATION_NAME', 'Chatwoot')) + end + + def self.default_body + DEFAULT_BODY + end + + def perform + selected_subscriptions.map { |subscription| test_send(subscription) } + end + + private + + def resolved_title + title.presence || self.class.default_title + end + + def resolved_body + body.presence || self.class.default_body + end + + def selected_subscriptions + user.notification_subscriptions.where(id: subscription_ids).order(:id) + end + + def test_send(subscription) + if subscription.browser_push? + test_browser_push(subscription) + elsif subscription.fcm? + test_fcm(subscription) + else + result(subscription, subscription.subscription_type.to_s, :skipped, 'Unknown subscription type') + end + end + + def test_browser_push(subscription) + return result(subscription, 'browser_push', :skipped, 'VAPID keys not configured') unless VapidService.public_key + + WebPush.payload_send(**browser_push_payload(subscription)) + result(subscription, 'browser_push', :success, 'Web push accepted by endpoint') + rescue StandardError => e + result(subscription, 'browser_push', :failure, "#{e.class.name}: #{e.message}") + end + + def test_fcm(subscription) + if firebase_credentials_present? + test_fcm_direct(subscription) + elsif chatwoot_hub_enabled? + test_fcm_via_hub(subscription) + else + result(subscription, 'fcm', :skipped, 'No Firebase credentials and push relay disabled') + end + end + + def test_fcm_direct(subscription) + fcm_service = Notification::FcmService.new( + GlobalConfigService.load('FIREBASE_PROJECT_ID', nil), + GlobalConfigService.load('FIREBASE_CREDENTIALS', nil) + ) + response = fcm_service.fcm_client.send_v1(fcm_options(subscription)) + status_code = response[:status_code].to_i + status = status_code.between?(200, 299) ? :success : :failure + result(subscription, 'fcm', status, "HTTP #{status_code} — #{response[:body]}") + rescue StandardError => e + result(subscription, 'fcm', :failure, "#{e.class.name}: #{e.message}") + end + + def test_fcm_via_hub(subscription) + response = ChatwootHub.send_push_with_response(fcm_options(subscription)) + result(subscription, 'fcm_via_hub', :success, "HTTP #{response.code} — #{response.body}") + rescue RestClient::ExceptionWithResponse => e + result(subscription, 'fcm_via_hub', :failure, "HTTP #{e.response&.code} — #{e.response&.body}") + rescue StandardError => e + result(subscription, 'fcm_via_hub', :failure, "#{e.class.name}: #{e.message}") + end + + def firebase_credentials_present? + GlobalConfigService.load('FIREBASE_PROJECT_ID', nil) && GlobalConfigService.load('FIREBASE_CREDENTIALS', nil) + end + + def chatwoot_hub_enabled? + ActiveModel::Type::Boolean.new.cast(ENV.fetch('ENABLE_PUSH_RELAY_SERVER', true)) + end + + def browser_push_payload(subscription) + { + message: JSON.generate( + title: resolved_title, + tag: "super_admin_test_#{Time.zone.now.to_i}", + url: ENV.fetch('FRONTEND_URL', 'https://app.chatwoot.com') + ), + endpoint: subscription.subscription_attributes['endpoint'], + p256dh: subscription.subscription_attributes['p256dh'], + auth: subscription.subscription_attributes['auth'], + vapid: { + subject: ENV.fetch('FRONTEND_URL', 'https://app.chatwoot.com'), + public_key: VapidService.public_key, + private_key: VapidService.private_key + }, + ssl_timeout: 5, + open_timeout: 5, + read_timeout: 5 + } + end + + def fcm_options(subscription) + { + 'token': subscription.subscription_attributes['push_token'], + 'data': { payload: { data: { notification: { type: 'test' } } }.to_json }, + 'notification': { title: resolved_title, body: resolved_body }, + 'android': { priority: 'high' }, + 'apns': { payload: { aps: { sound: 'default', category: Time.zone.now.to_i.to_s } } }, + 'fcm_options': { analytics_label: 'SuperAdminTest' } + } + end + + def result(subscription, type, status, message) + attrs = subscription.subscription_attributes || {} + { + id: subscription.id, + type: type.to_s, + device: device_label(subscription, attrs), + token_tail: token_tail(subscription, attrs), + status: status, + message: message + } + end + + def device_label(subscription, attrs) + if subscription.browser_push? + endpoint_host(attrs['endpoint'].to_s) + else + attrs['device_id'].present? ? "…#{attrs['device_id'].to_s.last(6)}" : '—' + end + end + + def endpoint_host(endpoint) + return '—' if endpoint.blank? + + URI.parse(endpoint).host.presence || endpoint + rescue URI::InvalidURIError + endpoint + end + + def token_tail(subscription, attrs) + if subscription.browser_push? + endpoint = attrs['endpoint'].to_s + endpoint.present? ? "…#{endpoint.last(6)}" : '—' + else + attrs['push_token'].present? ? "…#{attrs['push_token'].to_s.last(6)}" : '—' + end + end +end diff --git a/app/views/super_admin/application/_navigation.html.erb b/app/views/super_admin/application/_navigation.html.erb index 787576d33..da3a024f8 100644 --- a/app/views/super_admin/application/_navigation.html.erb +++ b/app/views/super_admin/application/_navigation.html.erb @@ -32,7 +32,7 @@ as defined by the routes in the `admin/` namespace
    <%= render partial: "nav_item", locals: { icon: 'icon-grid-line', url: super_admin_root_url, label: 'Dashboard' } %> <% Administrate::Namespace.new(namespace).resources.each do |resource| %> - <% next if ["account_users", "access_tokens", "installation_configs", "dashboard", "devise/sessions", "app_configs", "instance_statuses", "settings"].include? resource.resource %> + <% next if ["account_users", "access_tokens", "installation_configs", "dashboard", "devise/sessions", "app_configs", "instance_statuses", "settings", "push_diagnostics"].include? resource.resource %> <%= render partial: "nav_item", locals: { icon: sidebar_icons[resource.resource.to_sym], url: resource_index_route(resource), @@ -48,6 +48,7 @@ as defined by the routes in the `admin/` namespace
      <%= render partial: "nav_item", locals: { icon: 'icon-mist-fill', url: sidekiq_web_url, label: 'Sidekiq Dashboard' } %> <%= render partial: "nav_item", locals: { icon: 'icon-health-book-line', url: super_admin_instance_status_url, label: 'Instance Health' } %> + <%= render partial: "nav_item", locals: { icon: 'icon-mail-send-fill', url: super_admin_push_diagnostics_url, label: 'Push Diagnostics' } %> <%= render partial: "nav_item", locals: { icon: 'icon-dashboard-line', url: '/', label: 'Agent Dashboard' } %> <%= render partial: "nav_item", locals: { icon: 'icon-logout-circle-r-line', url: super_admin_logout_url, label: 'Logout' } %>
    diff --git a/app/views/super_admin/push_diagnostics/show.html.erb b/app/views/super_admin/push_diagnostics/show.html.erb new file mode 100644 index 000000000..736a77cd8 --- /dev/null +++ b/app/views/super_admin/push_diagnostics/show.html.erb @@ -0,0 +1,190 @@ +<% content_for(:title) do %>Push Diagnostics<% end %> + + + +
    +

    + Send a test push notification to a specific user's registered devices to diagnose delivery issues. + Results show the raw FCM / Web Push / relay response so you can see exactly what failed. +

    + +
    +

    1. Look up user

    + <%= form_with url: super_admin_push_diagnostics_path, method: :get, local: true, class: 'flex gap-2 items-center' do |f| %> + <%= f.text_field :user_query, + value: @query, + placeholder: 'user@example.com or numeric user ID', + class: 'border border-slate-100 p-1.5 rounded-md w-80' %> + <%= f.submit 'Look up', class: 'border border-slate-200 bg-slate-50 px-3 py-1.5 rounded-md cursor-pointer' %> + <% end %> + <% if @query.present? && @user.nil? %> +

    No user found for "<%= @query %>".

    + <% end %> +
    + + <% if @user %> +
    +

    + <%= @user.name %> + · <%= @user.email %> + · ID <%= @user.id %> +

    + <% if @user.accounts.any? %> +

    + Accounts: + <% @user.accounts.each_with_index do |account, index| %> + <%= ', ' if index.positive? %> + <%= account.name %> (ID <%= account.id %>) + <% end %> +

    + <% end %> +
    + +
    +

    2. Select subscriptions (<%= @subscriptions.count %>)

    +

    + ⚠️ This sends a real push to every selected device. Use only when diagnosing a reported issue. +

    + + <% if @subscriptions.empty? %> +

    This user has no push subscriptions registered.

    + <% else %> + <%= form_with url: super_admin_push_diagnostics_path, method: :post, local: true do |f| %> + <%= f.hidden_field :user_id, value: @user.id %> +
    +
    + <%= label_tag :push_title, 'Title', class: 'block text-xs font-medium text-slate-600 mb-1' %> + <%= text_field_tag :push_title, + params[:push_title].presence || Notification::PushTestService.default_title, + class: 'border border-slate-100 p-1.5 rounded-md w-full text-sm' %> +
    +
    + <%= label_tag :push_body, 'Body', class: 'block text-xs font-medium text-slate-600 mb-1' %> + <%= text_area_tag :push_body, + params[:push_body].presence || Notification::PushTestService.default_body, + rows: 2, + class: 'border border-slate-100 p-1.5 rounded-md w-full text-sm' %> +
    +

    Customers will see this text as a real push notification on their device.

    +
    + + + + + + + + + + + + + + + <% @subscriptions.each do |sub| %> + <% attrs = (sub.subscription_attributes || {}).stringify_keys %> + <% if sub.browser_push? %> + <% endpoint = attrs['endpoint'].to_s %> + <% host = (begin; URI.parse(endpoint).host; rescue URI::InvalidURIError; nil; end) %> + <% device_display = host.presence || endpoint.presence || '—' %> + <% token_display = endpoint.present? ? "…#{endpoint.last(6)}" : '—' %> + <% else %> + <% device_display = attrs['device_id'].present? ? "…#{attrs['device_id'].to_s.last(6)}" : '—' %> + <% token_display = attrs['push_token'].present? ? "…#{attrs['push_token'].to_s.last(6)}" : '—' %> + <% end %> + <% extra_attrs = attrs.except('endpoint', 'p256dh', 'auth', 'push_token', 'device_id') %> + + + + + + + + + + + <% end %> + +
    IDTypeDevice / endpointPush tokenDevice detailsCreatedLast updated
    + <%= check_box_tag 'subscription_ids[]', sub.id, false, class: 'subscription-checkbox' %> + <%= sub.id %><%= sub.subscription_type %><%= device_display %><%= token_display %> + <% if extra_attrs.present? %> + <% extra_attrs.each do |k, v| %> +
    <%= k %>: <%= v.to_s.truncate(40) %>
    + <% end %> + <% else %> + — + <% end %> +     		<%= sub.created_at.strftime('%Y-%m-%d %H:%M') %> + <%= sub.updated_at.strftime('%Y-%m-%d %H:%M') %> + (<%= time_ago_in_words(sub.updated_at) %> ago) +
    +
    + <%= f.submit 'Send Test Push to Selected', + class: 'border border-slate-200 bg-slate-50 px-3 py-1.5 rounded-md cursor-pointer font-medium' %> + <%= submit_tag 'Delete Selected Subscriptions', + formaction: destroy_subscriptions_super_admin_push_diagnostics_path, + formmethod: 'post', + data: { confirm: "Delete the selected subscription(s)? The user won't receive pushes on those devices until their app re-registers." }, + class: 'border border-red-200 bg-red-50 text-red-700 px-3 py-1.5 rounded-md cursor-pointer font-medium' %> +
    + <% end %> + <% end %> +
    + + <% if @results.present? %> +
    +

    3. Results

    + + + + + + + + + + + + + <% @results.each do |r| %> + + + + + + + + + <% end %> + +
    Sub IDTypeDevicePush tokenStatusDetails
    <%= r[:id] %><%= r[:type] %><%= r[:device] %><%= r[:token_tail] %> + <% + color = { + success: 'bg-green-100 text-green-800', + failure: 'bg-red-100 text-red-800', + skipped: 'bg-slate-100 text-slate-600' + }[r[:status]] + %> + <%= r[:status] %> + <%= r[:message] %>
    +
    + <% end %> + <% end %> +
    + +<% content_for :javascript do %> + +<% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 057f41b81..1841db332 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -496,3 +496,9 @@ en: subject: 'Finish setting up %{custom_domain}' ssl_status: custom_domain_not_configured: 'Custom domain is not configured' + super_admin: + push_diagnostics: + user_not_found: 'User not found.' + no_subscriptions_to_test: 'Select at least one subscription to test.' + no_subscriptions_to_delete: 'Select at least one subscription to delete.' + subscriptions_deleted: "Deleted %{count} subscription(s). The user's device(s) will re-register on next app launch." diff --git a/config/routes.rb b/config/routes.rb index 31453b157..2461539ad 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -625,6 +625,9 @@ Rails.application.routes.draw do root to: 'dashboard#index' resource :app_config, only: [:show, :create] + resource :push_diagnostics, only: [:show, :create] do + post :destroy_subscriptions, on: :collection + end # order of resources affect the order of sidebar navigation in super admin resources :accounts, only: [:index, :new, :create, :show, :edit, :update, :destroy] do diff --git a/lib/chatwoot_hub.rb b/lib/chatwoot_hub.rb index be5a07e05..95679ed73 100644 --- a/lib/chatwoot_hub.rb +++ b/lib/chatwoot_hub.rb @@ -106,14 +106,18 @@ class ChatwootHub end def self.send_push(fcm_options) - info = { fcm_options: fcm_options } - RestClient.post(push_notification_url, info.merge(instance_config).to_json, { content_type: :json, accept: :json }) + send_push_with_response(fcm_options) rescue *ExceptionList::REST_CLIENT_EXCEPTIONS => e Rails.logger.error "Exception: #{e.message}" rescue StandardError => e ChatwootExceptionTracker.new(e).capture_exception end + def self.send_push_with_response(fcm_options) + info = { fcm_options: fcm_options } + RestClient.post(push_notification_url, info.merge(instance_config).to_json, { content_type: :json, accept: :json }) + end + def self.emit_event(event_name, event_data) return if ENV['DISABLE_TELEMETRY'] From ca66218cb9f94869138d1b23fb00e5bf8c079d40 Mon Sep 17 00:00:00 2001 From: Muhsin Keloth Date: Tue, 21 Apr 2026 19:16:08 +0400 Subject: [PATCH 002/201] Revert: Validate Twilio webhook signatures (X-Twilio-Signature) (#14125) Reverts chatwoot/chatwoot#13638 --- .../twilio_signature_verify_concern.rb | 88 ----------- app/controllers/twilio/callback_controller.rb | 2 - .../twilio/delivery_status_controller.rb | 6 - .../twilio/callbacks_controller_spec.rb | 149 +----------------- .../twilio/delivery_status_controller_spec.rb | 123 +-------------- 5 files changed, 14 insertions(+), 354 deletions(-) delete mode 100644 app/controllers/concerns/twilio_signature_verify_concern.rb diff --git a/app/controllers/concerns/twilio_signature_verify_concern.rb b/app/controllers/concerns/twilio_signature_verify_concern.rb deleted file mode 100644 index b7a4754a4..000000000 --- a/app/controllers/concerns/twilio_signature_verify_concern.rb +++ /dev/null @@ -1,88 +0,0 @@ -module TwilioSignatureVerifyConcern - extend ActiveSupport::Concern - - included do - before_action :verify_twilio_signature! - end - - private - - def verify_twilio_signature! - channel = find_twilio_channel - return log_and_reject_missing_channel if channel.blank? - return if channel.api_key_sid.present? && log_api_key_skip(channel) - - head :forbidden unless valid_signature?(channel) - end - - def log_and_reject_missing_channel - Rails.logger.warn( - '[TWILIO] Channel not found for webhook ' \ - "account_sid=#{params[:AccountSid]} messaging_service_sid=#{params[:MessagingServiceSid]} " \ - "to=#{params[:To]} from=#{params[:From]}" - ) - head :forbidden - end - - def log_api_key_skip(channel) - Rails.logger.warn( - '[TWILIO] Signature validation skipped: channel uses API key authentication. ' \ - "account_sid=#{params[:AccountSid]} channel_id=#{channel.id}" - ) - end - - def valid_signature?(channel) - signature = request.headers['X-Twilio-Signature'] - if signature.blank? - Rails.logger.warn("[TWILIO] Missing X-Twilio-Signature header account_sid=#{params[:AccountSid]}") - return false - end - - validator = Twilio::Security::RequestValidator.new(channel.auth_token) - request_url = reconstruct_url - return true if validator.validate(request_url, request.request_parameters, signature) - - Rails.logger.warn( - '[TWILIO] Signature validation failed ' \ - "account_sid=#{params[:AccountSid]} channel_id=#{channel.id} url=#{request_url} ip=#{request.remote_ip}" - ) - false - end - - def find_twilio_channel - if params[:MessagingServiceSid].present? - channel = ::Channel::TwilioSms.find_by(messaging_service_sid: params[:MessagingServiceSid]) - return channel if channel.present? && (params[:AccountSid].blank? || channel.account_sid == params[:AccountSid]) - - return nil - end - return if params[:AccountSid].blank? - - find_channel_by_phone_number - end - - def find_channel_by_phone_number - channel_lookup_phone_numbers.each do |phone| - channel = ::Channel::TwilioSms.find_by(account_sid: params[:AccountSid], phone_number: phone) - return channel if channel - end - nil - end - - def channel_lookup_phone_numbers - [params[:To], params[:From]].compact_blank - end - - def reconstruct_url - url = request.original_url - url = url.sub('http://', 'https://') if url.start_with?('http://') && https_request? - url - end - - def https_request? - return true if request.ssl? - - forwarded_proto = request.headers['X-Forwarded-Proto'].to_s.split(',').map(&:strip).find(&:present?) - forwarded_proto&.casecmp?('https') - end -end diff --git a/app/controllers/twilio/callback_controller.rb b/app/controllers/twilio/callback_controller.rb index 9b42cd034..d607ba151 100644 --- a/app/controllers/twilio/callback_controller.rb +++ b/app/controllers/twilio/callback_controller.rb @@ -1,6 +1,4 @@ class Twilio::CallbackController < ApplicationController - include TwilioSignatureVerifyConcern - def create Webhooks::TwilioEventsJob.perform_later(permitted_params.to_unsafe_hash) diff --git a/app/controllers/twilio/delivery_status_controller.rb b/app/controllers/twilio/delivery_status_controller.rb index 8e846a737..1c756a1c2 100644 --- a/app/controllers/twilio/delivery_status_controller.rb +++ b/app/controllers/twilio/delivery_status_controller.rb @@ -1,6 +1,4 @@ class Twilio::DeliveryStatusController < ApplicationController - include TwilioSignatureVerifyConcern - def create Webhooks::TwilioDeliveryStatusJob.perform_later(permitted_params.to_unsafe_hash) @@ -20,8 +18,4 @@ class Twilio::DeliveryStatusController < ApplicationController :ErrorMessage ) end - - def channel_lookup_phone_numbers - [params[:From]].compact_blank - end end diff --git a/spec/controllers/twilio/callbacks_controller_spec.rb b/spec/controllers/twilio/callbacks_controller_spec.rb index 09b558096..d16acf229 100644 --- a/spec/controllers/twilio/callbacks_controller_spec.rb +++ b/spec/controllers/twilio/callbacks_controller_spec.rb @@ -4,160 +4,25 @@ RSpec.describe 'Twilio::CallbacksController', type: :request do include Rails.application.routes.url_helpers describe 'POST /twilio/callback' do - let(:account) { create(:account) } - let(:twilio_channel) { create(:channel_twilio_sms, :with_phone_number, account: account, account_sid: 'AC123') } let(:params) do { 'From' => '+1234567890', - 'To' => twilio_channel.phone_number, + 'To' => '+0987654321', 'Body' => 'Test message', 'AccountSid' => 'AC123', 'SmsSid' => 'SM123' } end - def post_with_signature(url, params:) - validator = Twilio::Security::RequestValidator.new(twilio_channel.auth_token) - signature = validator.build_signature_for(url, params) - post url, params: params, headers: { 'X-Twilio-Signature' => signature } - end - - context 'with valid signature' do - it 'enqueues the Twilio events job' do - url = twilio_callback_index_url - expect do - post_with_signature(url, params: params) - end.to have_enqueued_job(Webhooks::TwilioEventsJob) - end - - it 'returns no content status' do - url = twilio_callback_index_url - post_with_signature(url, params: params) - expect(response).to have_http_status(:no_content) - end - end - - context 'with invalid signature' do - it 'returns forbidden status' do - post twilio_callback_index_url, params: params, headers: { 'X-Twilio-Signature' => 'invalid' } - expect(response).to have_http_status(:forbidden) - end - - it 'does not enqueue the job' do - expect do - post twilio_callback_index_url, params: params, headers: { 'X-Twilio-Signature' => 'invalid' } - end.not_to have_enqueued_job(Webhooks::TwilioEventsJob) - end - end - - context 'with missing signature header' do - it 'returns forbidden status' do + it 'enqueues the Twilio events job' do + expect do post twilio_callback_index_url, params: params - expect(response).to have_http_status(:forbidden) - end + end.to have_enqueued_job(Webhooks::TwilioEventsJob).with(params) end - context 'when channel is not found' do - it 'returns forbidden status' do - post twilio_callback_index_url, params: params.merge('AccountSid' => 'UNKNOWN', 'To' => '+0000000000') - expect(response).to have_http_status(:forbidden) - end - end - - context 'when channel uses API key authentication' do - let(:twilio_channel) do - create(:channel_twilio_sms, :with_phone_number, account: account, account_sid: 'AC123', api_key_sid: 'SK123') - end - - it 'skips signature validation and enqueues the job' do - expect do - post twilio_callback_index_url, params: params - end.to have_enqueued_job(Webhooks::TwilioEventsJob) - end - end - - context 'when behind a reverse proxy with X-Forwarded-Proto' do - it 'validates signature against the HTTPS URL' do - http_url = twilio_callback_index_url - https_url = http_url.sub('http://', 'https://') - validator = Twilio::Security::RequestValidator.new(twilio_channel.auth_token) - signature = validator.build_signature_for(https_url, params) - post http_url, params: params, headers: { - 'X-Twilio-Signature' => signature, - 'X-Forwarded-Proto' => 'https' - } - expect(response).to have_http_status(:no_content) - end - - it 'validates signature when forwarded proto is a comma-separated chain' do - http_url = twilio_callback_index_url - https_url = http_url.sub('http://', 'https://') - validator = Twilio::Security::RequestValidator.new(twilio_channel.auth_token) - signature = validator.build_signature_for(https_url, params) - post http_url, params: params, headers: { - 'X-Twilio-Signature' => signature, - 'X-Forwarded-Proto' => 'https,http' - } - expect(response).to have_http_status(:no_content) - end - end - - context 'with MessagingServiceSid lookup' do - let(:twilio_channel) { create(:channel_twilio_sms, account: account, account_sid: 'AC123') } - let(:params) do - { - 'From' => '+1234567890', - 'Body' => 'Test message', - 'AccountSid' => 'AC123', - 'SmsSid' => 'SM123', - 'MessagingServiceSid' => twilio_channel.messaging_service_sid - } - end - - it 'validates and enqueues the job' do - url = twilio_callback_index_url - post_with_signature(url, params: params) - expect(response).to have_http_status(:no_content) - end - end - - context 'when MessagingServiceSid is present but does not match a channel' do - let(:params) do - { - 'From' => '+1234567890', - 'To' => twilio_channel.phone_number, - 'Body' => 'Test message', - 'AccountSid' => 'AC123', - 'SmsSid' => 'SM123', - 'MessagingServiceSid' => 'MG_UNKNOWN' - } - end - - it 'returns forbidden without falling back to phone number lookup' do - url = twilio_callback_index_url - post_with_signature(url, params: params) - expect(response).to have_http_status(:forbidden) - end - end - - context 'when MessagingServiceSid matches a channel but AccountSid does not' do - let(:other_channel) { create(:channel_twilio_sms, account: account, account_sid: 'AC_OTHER') } - let(:params) do - { - 'From' => '+1234567890', - 'To' => twilio_channel.phone_number, - 'Body' => 'Test message', - 'AccountSid' => 'AC123', - 'SmsSid' => 'SM123', - 'MessagingServiceSid' => other_channel.messaging_service_sid - } - end - - it 'returns forbidden without falling back to phone number lookup' do - url = twilio_callback_index_url - post_with_signature(url, params: params) - expect(response).to have_http_status(:forbidden) - end + it 'returns no content status' do + post twilio_callback_index_url, params: params + expect(response).to have_http_status(:no_content) end end end diff --git a/spec/controllers/twilio/delivery_status_controller_spec.rb b/spec/controllers/twilio/delivery_status_controller_spec.rb index dc99fdf23..fc21f8f94 100644 --- a/spec/controllers/twilio/delivery_status_controller_spec.rb +++ b/spec/controllers/twilio/delivery_status_controller_spec.rb @@ -4,132 +4,23 @@ RSpec.describe 'Twilio::DeliveryStatusController', type: :request do include Rails.application.routes.url_helpers describe 'POST /twilio/delivery_status' do - let(:account) { create(:account) } - let(:twilio_channel) { create(:channel_twilio_sms, :with_phone_number, account: account, account_sid: 'AC123') } let(:params) do { 'MessageSid' => 'SM123', 'MessageStatus' => 'delivered', - 'AccountSid' => 'AC123', - 'From' => twilio_channel.phone_number + 'AccountSid' => 'AC123' } end - def post_with_signature(url, params:, channel: twilio_channel) - validator = Twilio::Security::RequestValidator.new(channel.auth_token) - signature = validator.build_signature_for(url, params) - post url, params: params, headers: { 'X-Twilio-Signature' => signature } - end - - context 'with valid signature' do - it 'enqueues the delivery status job' do - url = twilio_delivery_status_index_url - expect do - post_with_signature(url, params: params) - end.to have_enqueued_job(Webhooks::TwilioDeliveryStatusJob) - end - - it 'returns no content status' do - url = twilio_delivery_status_index_url - post_with_signature(url, params: params) - expect(response).to have_http_status(:no_content) - end - end - - context 'with invalid signature' do - it 'returns forbidden status' do - post twilio_delivery_status_index_url, params: params, headers: { 'X-Twilio-Signature' => 'invalid' } - expect(response).to have_http_status(:forbidden) - end - - it 'does not enqueue the job' do - expect do - post twilio_delivery_status_index_url, params: params, headers: { 'X-Twilio-Signature' => 'invalid' } - end.not_to have_enqueued_job(Webhooks::TwilioDeliveryStatusJob) - end - end - - context 'with missing signature header' do - it 'returns forbidden status' do + it 'enqueues the Twilio delivery status job' do + expect do post twilio_delivery_status_index_url, params: params - expect(response).to have_http_status(:forbidden) - end + end.to have_enqueued_job(Webhooks::TwilioDeliveryStatusJob).with(params) end - context 'when channel uses API key authentication' do - let(:twilio_channel) do - create(:channel_twilio_sms, :with_phone_number, account: account, account_sid: 'AC123', api_key_sid: 'SK123') - end - - it 'skips signature validation and enqueues the job' do - expect do - post twilio_delivery_status_index_url, params: params - end.to have_enqueued_job(Webhooks::TwilioDeliveryStatusJob) - end - end - - context 'with MessagingServiceSid lookup' do - let(:twilio_channel) { create(:channel_twilio_sms, account: account, account_sid: 'AC123') } - let(:params) do - { - 'MessageSid' => 'SM123', - 'MessageStatus' => 'delivered', - 'AccountSid' => 'AC123', - 'MessagingServiceSid' => twilio_channel.messaging_service_sid - } - end - - it 'validates and enqueues the job' do - url = twilio_delivery_status_index_url - post_with_signature(url, params: params) - expect(response).to have_http_status(:no_content) - end - end - - context 'when To does not map to a channel but From does' do - let(:params) do - { - 'MessageSid' => 'SM123', - 'MessageStatus' => 'delivered', - 'AccountSid' => 'AC123', - 'To' => '+19999999999', - 'From' => twilio_channel.phone_number - } - end - - it 'falls back to From lookup and enqueues the delivery status job' do - url = twilio_delivery_status_index_url - - expect do - post_with_signature(url, params: params) - end.to have_enqueued_job(Webhooks::TwilioDeliveryStatusJob) - - expect(response).to have_http_status(:no_content) - end - end - - context 'when To maps to an API-key channel but From maps to a different channel' do - let!(:api_key_channel) do - create(:channel_twilio_sms, :with_phone_number, account: account, account_sid: 'AC123', api_key_sid: 'SK123') - end - let!(:from_channel) { create(:channel_twilio_sms, :with_phone_number, account: account, account_sid: 'AC123') } - let(:params) do - { - 'MessageSid' => 'SM123', - 'MessageStatus' => 'delivered', - 'AccountSid' => 'AC123', - 'To' => api_key_channel.phone_number, - 'From' => from_channel.phone_number - } - end - - it 'rejects invalid signatures instead of skipping verification' do - expect do - post twilio_delivery_status_index_url, params: params, headers: { 'X-Twilio-Signature' => 'invalid' } - end.not_to have_enqueued_job(Webhooks::TwilioDeliveryStatusJob) - - expect(response).to have_http_status(:forbidden) - end + it 'returns no content status' do + post twilio_delivery_status_index_url, params: params + expect(response).to have_http_status(:no_content) end end end From f12118a3c07d8de30a710357b84f18dddacbed0b Mon Sep 17 00:00:00 2001 From: Tanmay Deep Sharma <32020192+tds-1@users.noreply.github.com> Date: Wed, 22 Apr 2026 11:21:26 +0700 Subject: [PATCH 003/201] fix: void topup invoice when card payment fails (#14104) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a credit top-up's card charge fails, the finalized invoice was left in an open state with no hook back into our fulfillment service. If that invoice was later paid (manually via the hosted invoice page, or by a future dunning flow), the account would never receive its credits — silent revenue loss. This change voids the invoice the moment the charge fails, so a failed top-up cannot turn into a paid-but-unfulfilled invoice. ## Closes ## How to test 1. On a Business-plan account with a Stripe customer, attach a test card that will decline on charge (e.g. `4000 0000 0000 0002`). 2. From the dashboard, open the credit top-up flow and purchase a credit pack. 3. Observe the API returns an error and the account's captain credits are unchanged. 4. In the Stripe dashboard, confirm the corresponding invoice is in `void` status (not `open`). 5. Repeat with a good card (`4242 4242 4242 4242`) and confirm the happy path still fulfills credits. ## What changed - `finalize_and_pay` in `Enterprise::Billing::TopupCheckoutService` now rescues `Stripe::CardError`, voids the open invoice via `Stripe::Invoice.void_invoice`, and re-raises so the controller surfaces the original decline error to the client. - Rescue is intentionally narrow to `Stripe::CardError` (declines, insufficient funds, SCA `authentication_required`). Transient errors like `APIConnectionError` / `RateLimitError` are left to propagate — the charge may have actually succeeded and voiding could be wrong. - Invoices are created with `auto_advance: false`, so Stripe's Smart Retries won't collect on an open invoice; voiding is the correct terminal state. Co-authored-by: Claude Opus 4.7 (1M context) --- .../services/enterprise/billing/topup_checkout_service.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/enterprise/app/services/enterprise/billing/topup_checkout_service.rb b/enterprise/app/services/enterprise/billing/topup_checkout_service.rb index d0ec3e372..00e2b1646 100644 --- a/enterprise/app/services/enterprise/billing/topup_checkout_service.rb +++ b/enterprise/app/services/enterprise/billing/topup_checkout_service.rb @@ -79,7 +79,12 @@ class Enterprise::Billing::TopupCheckoutService def finalize_and_pay(invoice_id) Stripe::Invoice.finalize_invoice(invoice_id, { auto_advance: false }) invoice = Stripe::Invoice.retrieve(invoice_id) - Stripe::Invoice.pay(invoice_id) unless invoice.status == 'paid' + return if invoice.status == 'paid' + + Stripe::Invoice.pay(invoice_id) + rescue Stripe::CardError + Stripe::Invoice.void_invoice(invoice_id) + raise end def fulfill_credits(credits, topup_option) From 475db8531817ab23e56a8d1e38cad38d60f15fee Mon Sep 17 00:00:00 2001 From: Sivin Varghese <64252451+iamsivin@users.noreply.github.com> Date: Wed, 22 Apr 2026 17:13:26 +0530 Subject: [PATCH 004/201] fix: prevent template picker dropdown cut-off in compose form (#14129) --- .../NewConversation/ComposeConversation.vue | 1 + .../components/ActionButtons.vue | 2 +- .../components/ContentTemplateForm.vue | 4 +- .../components/ContentTemplateSelector.vue | 120 ++++++++++-------- .../components/WhatsAppOptions.vue | 111 ++++++++-------- .../components/WhatsappTemplate.vue | 4 +- .../components-next/popover/Popover.vue | 28 +++- .../modules/contact/ContactDeleteModal.vue | 4 +- .../modules/contact/ContactMergeModal.vue | 4 +- 9 files changed, 154 insertions(+), 124 deletions(-) diff --git a/app/javascript/dashboard/components-next/NewConversation/ComposeConversation.vue b/app/javascript/dashboard/components-next/NewConversation/ComposeConversation.vue index 1ab370901..02a00c703 100644 --- a/app/javascript/dashboard/components-next/NewConversation/ComposeConversation.vue +++ b/app/javascript/dashboard/components-next/NewConversation/ComposeConversation.vue @@ -233,6 +233,7 @@ onMounted(() => resetContacts()); diff --git a/app/javascript/dashboard/components-next/NewConversation/components/ActionButtons.vue b/app/javascript/dashboard/components-next/NewConversation/components/ActionButtons.vue index aec05a717..e8f484997 100644 --- a/app/javascript/dashboard/components-next/NewConversation/components/ActionButtons.vue +++ b/app/javascript/dashboard/components-next/NewConversation/components/ActionButtons.vue @@ -20,6 +20,7 @@ const props = defineProps({ isEmailOrWebWidgetInbox: { type: Boolean, default: false }, isTwilioSmsInbox: { type: Boolean, default: false }, isTwilioWhatsAppInbox: { type: Boolean, default: false }, + // eslint-disable-next-line vue/no-unused-properties messageTemplates: { type: Array, default: () => [] }, channelType: { type: String, default: '' }, isLoading: { type: Boolean, default: false }, @@ -198,7 +199,6 @@ useEventListener(document, 'paste', onPaste); { diff --git a/config/locales/en.yml b/config/locales/en.yml index 36f115bab..9ffd3f3d5 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -493,6 +493,7 @@ en: locale_not_available: 'Locale not available in this portal' category_not_found: 'Category not found in this portal' no_articles_found: 'No articles found to process' + invalid_status: 'Invalid status value' send_instructions: email_required: 'Email is required' invalid_email_format: 'Invalid email format' diff --git a/config/routes.rb b/config/routes.rb index c6111d317..a1d3d088e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -361,6 +361,8 @@ Rails.application.routes.draw do namespace :articles do resource :bulk_actions, only: [] do post :translate + patch :update_status + delete :delete_articles end end resources :articles do diff --git a/spec/controllers/api/v1/accounts/articles/bulk_actions_controller_spec.rb b/spec/controllers/api/v1/accounts/articles/bulk_actions_controller_spec.rb new file mode 100644 index 000000000..3dab5b60f --- /dev/null +++ b/spec/controllers/api/v1/accounts/articles/bulk_actions_controller_spec.rb @@ -0,0 +1,141 @@ +require 'rails_helper' + +RSpec.describe 'Article Bulk Actions API', type: :request do + let(:account) { create(:account) } + let(:admin) { create(:user, account: account, role: :administrator) } + let(:agent) { create(:user, account: account, role: :agent) } + let!(:portal) { create(:portal, name: 'test_portal', account: account, config: { allowed_locales: %w[en es] }) } + let!(:category) { create(:category, portal: portal, account: account, locale: 'en', slug: 'getting-started') } + let!(:article_one) { create(:article, category: category, portal: portal, account: account, author: admin, status: :draft) } + let!(:article_two) { create(:article, category: category, portal: portal, account: account, author: admin, status: :draft) } + let!(:article_three) { create(:article, category: category, portal: portal, account: account, author: admin, status: :published) } + + let(:base_url) { "/api/v1/accounts/#{account.id}/portals/#{portal.slug}/articles/bulk_actions" } + + describe 'PATCH articles/bulk_actions/update_status' do + let(:update_status_url) { "#{base_url}/update_status" } + + context 'when unauthenticated' do + it 'returns unauthorized' do + patch update_status_url, params: { ids: [article_one.id], status: 'published' }, as: :json + expect(response).to have_http_status(:unauthorized) + end + end + + context 'when authenticated as agent' do + it 'returns unauthorized' do + patch update_status_url, + headers: agent.create_new_auth_token, + params: { ids: [article_one.id], status: 'published' }, + as: :json + expect(response).to have_http_status(:unauthorized) + end + end + + context 'when authenticated as admin' do + it 'publishes multiple articles' do + patch update_status_url, + headers: admin.create_new_auth_token, + params: { ids: [article_one.id, article_two.id], status: 'published' }, + as: :json + + expect(response).to have_http_status(:ok) + expect(article_one.reload.status).to eq('published') + expect(article_two.reload.status).to eq('published') + end + + it 'archives multiple articles' do + patch update_status_url, + headers: admin.create_new_auth_token, + params: { ids: [article_one.id, article_three.id], status: 'archived' }, + as: :json + + expect(response).to have_http_status(:ok) + expect(article_one.reload.status).to eq('archived') + expect(article_three.reload.status).to eq('archived') + end + + it 'sets articles to draft' do + patch update_status_url, + headers: admin.create_new_auth_token, + params: { ids: [article_three.id], status: 'draft' }, + as: :json + + expect(response).to have_http_status(:ok) + expect(article_three.reload.status).to eq('draft') + end + + it 'does not affect articles not in the list' do + patch update_status_url, + headers: admin.create_new_auth_token, + params: { ids: [article_one.id], status: 'published' }, + as: :json + + expect(article_one.reload.status).to eq('published') + expect(article_three.reload.status).to eq('published') + end + + it 'returns unprocessable entity when no articles found' do + patch update_status_url, + headers: admin.create_new_auth_token, + params: { ids: [0], status: 'published' }, + as: :json + + expect(response).to have_http_status(:unprocessable_entity) + end + end + end + + describe 'DELETE articles/bulk_actions/delete_articles' do + let(:destroy_url) { "#{base_url}/delete_articles" } + + context 'when unauthenticated' do + it 'returns unauthorized' do + delete destroy_url, params: { ids: [article_one.id] }, as: :json + expect(response).to have_http_status(:unauthorized) + end + end + + context 'when authenticated as agent' do + it 'returns unauthorized' do + delete destroy_url, + headers: agent.create_new_auth_token, + params: { ids: [article_one.id] }, + as: :json + expect(response).to have_http_status(:unauthorized) + end + end + + context 'when authenticated as admin' do + it 'deletes multiple articles' do + expect do + delete destroy_url, + headers: admin.create_new_auth_token, + params: { ids: [article_one.id, article_two.id] }, + as: :json + end.to change(Article, :count).by(-2) + + expect(response).to have_http_status(:ok) + end + + it 'does not delete articles not in the list' do + delete destroy_url, + headers: admin.create_new_auth_token, + params: { ids: [article_one.id] }, + as: :json + + expect(Article.exists?(article_one.id)).to be(false) + expect(Article.exists?(article_three.id)).to be(true) + end + + it 'returns unprocessable entity when no articles found' do + delete destroy_url, + headers: admin.create_new_auth_token, + params: { ids: [0] }, + as: :json + + expect(response).to have_http_status(:unprocessable_entity) + end + end + end +end From a651949c33383b57d08e21d2210916b1ae2e9363 Mon Sep 17 00:00:00 2001 From: Aakash Bakhle <48802744+aakashb95@users.noreply.github.com> Date: Mon, 27 Apr 2026 13:01:44 +0530 Subject: [PATCH 015/201] fix: improve FAQ generation [AI-145] (#14062) # Pull Request Template ## Description - Fetch main content only from Firecrawl, exclude some tags to remove boilerplate - Prompt changes for FAQ generation ## Type of change Please delete options that are not relevant. - [x] Bug fix (non-breaking change which fixes an issue) ## How Has This Been Tested? Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration. tested locally ## Checklist: - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my code - [x] I have commented on my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] Any dependent changes have been merged and published in downstream modules --- .../captain/documents/response_builder_job.rb | 2 +- enterprise/app/models/captain/document.rb | 4 +++ .../captain/llm/faq_generator_service.rb | 16 ++++++++---- .../llm/paginated_faq_generator_service.rb | 14 +++++------ .../captain/llm/system_prompts_service.rb | 25 ++++++++++++------- .../captain/tools/firecrawl_service.rb | 17 ++++++++----- .../documents/response_builder_job_spec.rb | 13 +++------- .../captain/llm/faq_generator_service_spec.rb | 11 ++++---- .../captain/tools/firecrawl_service_spec.rb | 4 +-- 9 files changed, 62 insertions(+), 44 deletions(-) diff --git a/enterprise/app/jobs/captain/documents/response_builder_job.rb b/enterprise/app/jobs/captain/documents/response_builder_job.rb index 60f1fa2cf..8f6643e36 100644 --- a/enterprise/app/jobs/captain/documents/response_builder_job.rb +++ b/enterprise/app/jobs/captain/documents/response_builder_job.rb @@ -26,7 +26,7 @@ class Captain::Documents::ResponseBuilderJob < ApplicationJob end def generate_standard_faqs(document) - Captain::Llm::FaqGeneratorService.new(document.content, document.account.locale_english_name, account_id: document.account_id).generate + Captain::Llm::FaqGeneratorService.new(document: document).generate end def build_paginated_service(document, options) diff --git a/enterprise/app/models/captain/document.rb b/enterprise/app/models/captain/document.rb index 2abf18437..c2b5fa214 100644 --- a/enterprise/app/models/captain/document.rb +++ b/enterprise/app/models/captain/document.rb @@ -117,6 +117,10 @@ class Captain::Document < ApplicationRecord end end + def to_llm_metadata + { document_id: id, assistant_id: assistant_id, external_link: external_link } + end + private def enqueue_crawl_job diff --git a/enterprise/app/services/captain/llm/faq_generator_service.rb b/enterprise/app/services/captain/llm/faq_generator_service.rb index 5f85ae467..b80382b3e 100644 --- a/enterprise/app/services/captain/llm/faq_generator_service.rb +++ b/enterprise/app/services/captain/llm/faq_generator_service.rb @@ -1,11 +1,12 @@ class Captain::Llm::FaqGeneratorService < Llm::BaseAiService include Integrations::LlmInstrumentation - def initialize(content, language = 'english', account_id: nil) + def initialize(document:) super() - @language = language - @content = content - @account_id = account_id + @document = document + @content = document.content + @language = document.account.locale_english_name + @account_id = document.account_id end def generate @@ -40,10 +41,15 @@ class Captain::Llm::FaqGeneratorService < Llm::BaseAiService messages: [ { role: 'system', content: system_prompt }, { role: 'user', content: @content } - ] + ], + metadata: document_metadata } end + def document_metadata + @document&.to_llm_metadata || {} + end + def parse_response(content) return [] if content.nil? diff --git a/enterprise/app/services/captain/llm/paginated_faq_generator_service.rb b/enterprise/app/services/captain/llm/paginated_faq_generator_service.rb index 3fe81c2ae..b567609e8 100644 --- a/enterprise/app/services/captain/llm/paginated_faq_generator_service.rb +++ b/enterprise/app/services/captain/llm/paginated_faq_generator_service.rb @@ -51,7 +51,8 @@ class Captain::Llm::PaginatedFaqGeneratorService < Llm::LegacyBaseOpenAiService account_id: @document&.account_id, feature_name: 'faq_generation', model: @model, - messages: params[:messages] + messages: params[:messages], + metadata: document_metadata } response = instrument_llm_call(instrumentation_params) do @@ -214,12 +215,11 @@ class Captain::Llm::PaginatedFaqGeneratorService < Llm::LegacyBaseOpenAiService feature_name: 'paginated_faq_generation', model: @model, messages: params[:messages], - metadata: { - document_id: @document&.id, - start_page: start_page, - end_page: end_page, - iteration: @iterations_completed + 1 - } + metadata: document_metadata.merge(start_page: start_page, end_page: end_page, iteration: @iterations_completed + 1) } end + + def document_metadata + @document&.to_llm_metadata || {} + end end diff --git a/enterprise/app/services/captain/llm/system_prompts_service.rb b/enterprise/app/services/captain/llm/system_prompts_service.rb index 9868f0360..dab147301 100644 --- a/enterprise/app/services/captain/llm/system_prompts_service.rb +++ b/enterprise/app/services/captain/llm/system_prompts_service.rb @@ -3,11 +3,15 @@ class Captain::Llm::SystemPromptsService class << self def faq_generator(language = 'english') <<~PROMPT - You are a content writer specializing in creating good FAQ sections for website help centers. Your task is to convert provided content into a structured FAQ format without losing any information. + You are a content writer specializing in creating good FAQ sections for website help centers. Your task is to convert provided content into a structured FAQ format without losing any substantive information. ## Core Requirements - **Completeness**: Extract ALL information from the source content. Every detail, example, procedure, and explanation must be captured across the FAQ set. When combined, the FAQs should reconstruct the original content entirely. + **Completeness**: Extract ALL substantive information from the source content. Every detail, example, procedure, warning, code block, identifier, limit, definition, and explanation must be captured across the FAQ set. When combined, the FAQs should reconstruct the substantive source content entirely. + + **Self-contained answers**: Every answer must contain the information that answers its question. The answer must be the substance, not directions to where the substance lives. If a source section provides only a reference, link, or pointer to where the information can be found — without containing that information itself — omit the FAQ for that section. An FAQ whose answer redirects the reader is worse than no FAQ at all. + + **Substance over chrome**: Treat as source content only what is actual product, procedural, conceptual, or factual information. Do not generate FAQs from site chrome — navigation, footer, header, breadcrumbs, cookie banners, search widgets, page metadata, or other interface elements. **Accuracy**: Base answers strictly on the provided text. Do not add assumptions, interpretations, or external knowledge not present in the source material. @@ -29,18 +33,21 @@ class Captain::Llm::SystemPromptsService ## Guidelines - **Question Creation**: Formulate questions that naturally arise from the content (What is...? How do I...? When should...? Why does...?). Do not generate questions that are not related to the content. - - **Answer Completeness**: Include all relevant details, steps, examples, and context from the original content - - **Information Preservation**: Ensure no examples, procedures, warnings, or explanatory details are omitted + - **Answer Completeness**: Include all relevant details, steps, examples, code, identifiers, limits, and definitions present in the source. + - **Information Preservation**: Never omit examples, procedures, warnings, code, IDs, limits, or definitions in the name of brevity. + - **No Deflecting FAQs**: Do not create FAQs whose answer would only tell the reader to open another link, guide, or document. If the source contains useful factual content in link text, labels, lists, or summaries (e.g., a curated list of supported integrations, plan features, resources, or article indexes), preserve that content as the answer. If it only points elsewhere without providing the answer itself, skip it. - **JSON Validity**: Always return properly formatted, valid JSON - **No Content Scenario**: If no suitable content is found, return: `{"faqs": []}` ## Process 1. Read the entire provided content carefully - 2. Identify all key information points, procedures, and examples - 3. Create questions that cover each information point - 4. Write comprehensive short answers that capture all related detail, include bullet points if needed. - 5. Verify that combined FAQs represent the complete original content. - 6. Format as valid JSON + 2. Identify all key information points: procedures, examples, code, identifiers, limits, definitions, warnings, and explanations + 3. For each candidate section, verify the source contains the substance that would answer the question. If the source only points to where the substance lives, skip the section. + 4. Disregard interface chrome (navigation, footer, header, cookie banners, breadcrumbs, page metadata). + 5. Create questions that cover each remaining substantive information point + 6. Write self-contained answers that preserve all relevant details from the source. Be concise where possible, but never trade away steps, examples, warnings, code, IDs, limits, or definitions for brevity. + 7. Verify the combined FAQs represent the complete substantive source content (excluding redirect-only sections and chrome). + 8. Format as valid JSON PROMPT end diff --git a/enterprise/app/services/captain/tools/firecrawl_service.rb b/enterprise/app/services/captain/tools/firecrawl_service.rb index fc7448593..3d1b53b7a 100644 --- a/enterprise/app/services/captain/tools/firecrawl_service.rb +++ b/enterprise/app/services/captain/tools/firecrawl_service.rb @@ -1,5 +1,6 @@ class Captain::Tools::FirecrawlService BASE_URL = 'https://api.firecrawl.dev/v1'.freeze + FIRECRAWL_EXCLUDE_TAGS = %w[iframe .sidebar .cookie-banner [role=navigation] [role=banner] [role=contentinfo]].freeze def initialize @api_key = InstallationConfig.find_by!(name: 'CAPTAIN_FIRECRAWL_API_KEY').value @@ -33,16 +34,20 @@ class Captain::Tools::FirecrawlService ignoreSitemap: false, limit: crawl_limit, webhook: webhook_url, - scrapeOptions: { - onlyMainContent: false, - formats: ['markdown'], - excludeTags: ['iframe'] - } + scrapeOptions: scrape_options }.to_json end def scrape_payload(url) - { url: url, formats: ['markdown'], excludeTags: ['iframe'] }.to_json + { url: url }.merge(scrape_options).to_json + end + + def scrape_options + { + onlyMainContent: true, + formats: ['markdown'], + excludeTags: FIRECRAWL_EXCLUDE_TAGS + } end def headers diff --git a/spec/enterprise/jobs/captain/documents/response_builder_job_spec.rb b/spec/enterprise/jobs/captain/documents/response_builder_job_spec.rb index c3e5eab1c..4d1a07aa7 100644 --- a/spec/enterprise/jobs/captain/documents/response_builder_job_spec.rb +++ b/spec/enterprise/jobs/captain/documents/response_builder_job_spec.rb @@ -12,9 +12,7 @@ RSpec.describe Captain::Documents::ResponseBuilderJob, type: :job do end before do - allow(Captain::Llm::FaqGeneratorService).to receive(:new) - .with(document.content, document.account.locale_english_name, account_id: document.account_id) - .and_return(faq_generator) + allow(Captain::Llm::FaqGeneratorService).to receive(:new).with(document: document).and_return(faq_generator) allow(faq_generator).to receive(:generate).and_return(faqs) end @@ -51,17 +49,14 @@ RSpec.describe Captain::Documents::ResponseBuilderJob, type: :job do let(:spanish_faq_generator) { instance_double(Captain::Llm::FaqGeneratorService) } before do - allow(Captain::Llm::FaqGeneratorService).to receive(:new) - .with(spanish_document.content, 'portuguese', account_id: spanish_document.account_id) - .and_return(spanish_faq_generator) + allow(Captain::Llm::FaqGeneratorService).to receive(:new).with(document: spanish_document).and_return(spanish_faq_generator) allow(spanish_faq_generator).to receive(:generate).and_return(faqs) end - it 'passes the correct locale to FAQ generator' do + it 'passes the correct document to FAQ generator' do described_class.new.perform(spanish_document) - expect(Captain::Llm::FaqGeneratorService).to have_received(:new) - .with(spanish_document.content, 'portuguese', account_id: spanish_document.account_id) + expect(Captain::Llm::FaqGeneratorService).to have_received(:new).with(document: spanish_document) end end diff --git a/spec/enterprise/services/captain/llm/faq_generator_service_spec.rb b/spec/enterprise/services/captain/llm/faq_generator_service_spec.rb index 003d5b715..ff7138c9a 100644 --- a/spec/enterprise/services/captain/llm/faq_generator_service_spec.rb +++ b/spec/enterprise/services/captain/llm/faq_generator_service_spec.rb @@ -2,8 +2,8 @@ require 'rails_helper' RSpec.describe Captain::Llm::FaqGeneratorService do let(:content) { 'Sample content for FAQ generation' } - let(:language) { 'english' } - let(:service) { described_class.new(content, language) } + let(:document) { create(:captain_document, content: content) } + let(:service) { described_class.new(document: document) } let(:mock_chat) { instance_double(RubyLLM::Chat) } let(:sample_faqs) do [ @@ -36,14 +36,15 @@ RSpec.describe Captain::Llm::FaqGeneratorService do service.generate end - it 'uses SystemPromptsService with the specified language' do - expect(Captain::Llm::SystemPromptsService).to receive(:faq_generator).with(language).at_least(:once).and_call_original + it 'uses SystemPromptsService with the account language' do + account_language = document.account.locale_english_name + expect(Captain::Llm::SystemPromptsService).to receive(:faq_generator).with(account_language).at_least(:once).and_call_original service.generate end end context 'with different language' do - let(:language) { 'spanish' } + before { allow(document.account).to receive(:locale_english_name).and_return('spanish') } it 'passes the correct language to SystemPromptsService' do expect(Captain::Llm::SystemPromptsService).to receive(:faq_generator).with('spanish').at_least(:once).and_call_original diff --git a/spec/enterprise/services/captain/tools/firecrawl_service_spec.rb b/spec/enterprise/services/captain/tools/firecrawl_service_spec.rb index d6563b163..b46633a6e 100644 --- a/spec/enterprise/services/captain/tools/firecrawl_service_spec.rb +++ b/spec/enterprise/services/captain/tools/firecrawl_service_spec.rb @@ -58,9 +58,9 @@ RSpec.describe Captain::Tools::FirecrawlService do limit: crawl_limit, webhook: webhook_url, scrapeOptions: { - onlyMainContent: false, + onlyMainContent: true, formats: ['markdown'], - excludeTags: ['iframe'] + excludeTags: Captain::Tools::FirecrawlService::FIRECRAWL_EXCLUDE_TAGS } }.to_json end From 8faa5a74b1ce27248f15aa2bce9a2c6c56d227d7 Mon Sep 17 00:00:00 2001 From: Sivin Varghese <64252451+iamsivin@users.noreply.github.com> Date: Mon, 27 Apr 2026 13:30:51 +0530 Subject: [PATCH 016/201] fix: prevent focus jump to title after new article auto-creates (#14145) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Pull Request Template ## Description When creating a help center article, typing a title and navigating into the content auto-creates the article and switches the route (`/articles/new` → `/articles/.../edit/:slug`). During this transition, focus was jumping back to the title, interrupting editing. This happened because `ArticleEditor` always autofocuses the title. On route change, the component remounts and re-triggers focus. Now, after auto-create, focus stays in the body as expected. Fixes https://linear.app/chatwoot/issue/CW-6951/issue-with-the-cursor-position-on-the-help-center-article-when ## Type of change - [x] Bug fix (non-breaking change which fixes an issue) ## How Has This Been Tested? **Screencast** https://github.com/user-attachments/assets/dac3f7c6-08c4-4df2-afb0-7731ee76424b ## Checklist: - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my code - [ ] I have commented on my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [ ] Any dependent changes have been merged and published in downstream modules --- .../HelpCenter/Pages/ArticleEditorPage/ArticleEditor.vue | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/javascript/dashboard/components-next/HelpCenter/Pages/ArticleEditorPage/ArticleEditor.vue b/app/javascript/dashboard/components-next/HelpCenter/Pages/ArticleEditorPage/ArticleEditor.vue index 22cb1441a..831312e0b 100644 --- a/app/javascript/dashboard/components-next/HelpCenter/Pages/ArticleEditorPage/ArticleEditor.vue +++ b/app/javascript/dashboard/components-next/HelpCenter/Pages/ArticleEditorPage/ArticleEditor.vue @@ -121,7 +121,7 @@ const handleCreateArticle = event => { custom-text-area-class="!text-[32px] !leading-[48px] !font-medium !tracking-[0.2px]" custom-text-area-wrapper-class="border-0 !bg-transparent dark:!bg-transparent !py-0 !px-0" placeholder="Title" - autofocus + :autofocus="isNewArticle" @blur="handleCreateArticle" /> { t('HELP_CENTER.EDIT_ARTICLE_PAGE.EDIT_ARTICLE.EDITOR_PLACEHOLDER') " :enabled-menu-options="ARTICLE_EDITOR_MENU_OPTIONS" - :autofocus="false" + :autofocus="!isNewArticle" /> From 06467057be17d900450f8eac98cb7845a65bdf29 Mon Sep 17 00:00:00 2001 From: Sivin Varghese <64252451+iamsivin@users.noreply.github.com> Date: Mon, 27 Apr 2026 13:31:43 +0530 Subject: [PATCH 017/201] fix: oversized email signature images in Letter render (#14144) # Pull Request Template ## Description This PR fixes an issue where signature images (with `?cw_image_height=...`) render at their original large size in the email bubble. ### Cause Renderer output: ```html ``` Email UI and clients (Gmail, Outlook) apply CSS like: `img { max-width: 100%; height: auto; }` This overrides `height="24px"`. Other channels work because they use inline styles (`style="height: 24px;"`). ### Solution Use inline style instead: ```html ``` ### Why backend fix * Fixes root cause and aligns Ruby + JS renderers * Works in both Chatwoot UI and recipient inboxes * Covers all email-rendered content * Minimal change Fixes https://linear.app/chatwoot/issue/CW-6948/email-signature-image-renders-oversized-in-chatwoot-ui ## Type of change - [x] Bug fix (non-breaking change which fixes an issue) ## How Has This Been Tested? #### Screenshots **Before** image **After** image ## Checklist: - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my code - [x] I have commented on my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [ ] Any dependent changes have been merged and published in downstream modules --- lib/base_markdown_renderer.rb | 8 ++++++-- spec/lib/base_markdown_renderer_spec.rb | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/base_markdown_renderer.rb b/lib/base_markdown_renderer.rb index df49918b3..f530e71ee 100644 --- a/lib/base_markdown_renderer.rb +++ b/lib/base_markdown_renderer.rb @@ -29,11 +29,15 @@ class BaseMarkdownRenderer < CommonMarker::HtmlRenderer def render_img_tag(src, title, height = nil) title_attribute = title.present? ? " title=\"#{title}\"" : '' - height_attribute = height ? " height=\"#{height}\" width=\"auto\"" : '' + # Use inline style instead of the HTML height attribute: email clients and + # the in-app Letter view both run images through CSS (e.g. prose / + # lettersanitizer's `img { height: auto }`) which overrides presentational + # attributes. Inline style has higher specificity and survives. + style_attribute = height ? " style=\"height: #{height};\"" : '' plain do # plain ensures that the content is not wrapped in a paragraph tag - out("") + out("") end end end diff --git a/spec/lib/base_markdown_renderer_spec.rb b/spec/lib/base_markdown_renderer_spec.rb index 262e78daf..f8bdae4be 100644 --- a/spec/lib/base_markdown_renderer_spec.rb +++ b/spec/lib/base_markdown_renderer_spec.rb @@ -12,7 +12,7 @@ describe BaseMarkdownRenderer do context 'when image has a height' do it 'renders the img tag with the correct attributes' do markdown = '![Sample Title](https://example.com/image.jpg?cw_image_height=100)' - expect(render_markdown(markdown)).to include('') + expect(render_markdown(markdown)).to include('') end end From 0920a01e662163c13714fd6bd393b292df89151d Mon Sep 17 00:00:00 2001 From: Sojan Jose Date: Mon, 27 Apr 2026 15:40:00 +0530 Subject: [PATCH 018/201] fix(i18n): align pluralization with locale rules (#14266) Loads Rails locale-specific pluralization rules so languages with an `other`-only plural model can safely use Crowdin exports without maintaining duplicate `one` keys. ## Closes None ## Why Crowdin exports Rails YAML pluralized strings using each target language's plural categories. These categories come from Unicode CLDR and represent grammatical forms, not a literal "number is 1" bucket. Some languages need separate forms such as `one` and `other`, but languages like Japanese, Korean, Indonesian, Thai, Vietnamese, and Chinese use the same form for `1`, `2`, `5`, and larger counts in these strings. For those locales, CLDR correctly models the plural category as `other` only. Before this change, Chatwoot still relied on Rails' default English-style plural behavior for these locales. That meant a valid Crowdin export containing only `other` could fail at runtime when Rails received `count: 1` and looked for a missing `one` branch. Keeping duplicate `one` keys would only fight Crowdin on every translation sync. The runtime should instead follow the locale's plural rules. ## What changed - Added `rails-i18n` and enabled only its pluralization module. - Added explicit `other`-only plural rules for Chatwoot's underscore Chinese locale aliases, `zh_CN` and `zh_TW`. - Removed redundant `one` keys from the affected Devise and `time_units` translations. ## Validation - Ran a Rails runner check across `id`, `ja`, `ko`, `ms`, `th`, `vi`, `zh_CN`, and `zh_TW` to verify `errors.messages.not_saved` and `time_units.days` resolve with only `other` for `count: 1`. - Ran YAML parse validation for all edited locale files. - Ran `bundle exec rubocop Gemfile config/application.rb config/initializers/i18n_pluralization.rb`. --- Gemfile | 1 + Gemfile.lock | 4 ++++ config/application.rb | 1 + config/initializers/i18n_pluralization.rb | 8 ++++++++ config/locales/devise.id.yml | 1 - config/locales/devise.ja.yml | 1 - config/locales/devise.ko.yml | 1 - config/locales/devise.ms.yml | 1 - config/locales/devise.th.yml | 1 - config/locales/devise.vi.yml | 1 - config/locales/devise.zh_CN.yml | 1 - config/locales/devise.zh_TW.yml | 1 - config/locales/id.yml | 4 ---- config/locales/ja.yml | 4 ---- config/locales/ko.yml | 4 ---- config/locales/ms.yml | 4 ---- config/locales/th.yml | 4 ---- config/locales/vi.yml | 4 ---- config/locales/zh_CN.yml | 4 ---- config/locales/zh_TW.yml | 4 ---- 20 files changed, 14 insertions(+), 40 deletions(-) create mode 100644 config/initializers/i18n_pluralization.rb diff --git a/Gemfile b/Gemfile index a5068e765..c4989c538 100644 --- a/Gemfile +++ b/Gemfile @@ -84,6 +84,7 @@ gem 'barnes' gem 'devise', '>= 4.9.4' gem 'devise-secure_password', git: 'https://github.com/chatwoot/devise-secure_password', branch: 'chatwoot' gem 'devise_token_auth', '>= 1.2.3' +gem 'rails-i18n', '~> 7.0' # two-factor authentication gem 'devise-two-factor', '>= 5.0.0' # authorization diff --git a/Gemfile.lock b/Gemfile.lock index b77e5880f..7d29e0b02 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -727,6 +727,9 @@ GEM rails-html-sanitizer (1.6.1) loofah (~> 2.21) nokogiri (>= 1.15.7, != 1.16.7, != 1.16.6, != 1.16.5, != 1.16.4, != 1.16.3, != 1.16.2, != 1.16.1, != 1.16.0.rc1, != 1.16.0) + rails-i18n (7.0.10) + i18n (>= 0.7, < 2) + railties (>= 6.0.0, < 8) railties (7.1.5.2) actionpack (= 7.1.5.2) activesupport (= 7.1.5.2) @@ -1125,6 +1128,7 @@ DEPENDENCIES rack-mini-profiler (>= 3.2.0) rack-timeout rails (~> 7.1) + rails-i18n (~> 7.0) redis redis-namespace responders (>= 3.1.1) diff --git a/config/application.rb b/config/application.rb index aa150794a..08f0451c1 100644 --- a/config/application.rb +++ b/config/application.rb @@ -37,6 +37,7 @@ module Chatwoot class Application < Rails::Application # Initialize configuration defaults for originally generated Rails version. config.load_defaults 7.0 + config.rails_i18n.enabled_modules = [:pluralization] config.eager_load_paths << Rails.root.join('lib') config.eager_load_paths << Rails.root.join('enterprise/lib') diff --git a/config/initializers/i18n_pluralization.rb b/config/initializers/i18n_pluralization.rb new file mode 100644 index 000000000..c4fc3fb4b --- /dev/null +++ b/config/initializers/i18n_pluralization.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +other_plural_rule = ->(_count) { :other } + +Rails.application.config.after_initialize do + I18n.backend.store_translations(:zh_CN, i18n: { plural: { rule: other_plural_rule } }) + I18n.backend.store_translations(:zh_TW, i18n: { plural: { rule: other_plural_rule } }) +end diff --git a/config/locales/devise.id.yml b/config/locales/devise.id.yml index 71b9f46fb..fc6bfb26a 100644 --- a/config/locales/devise.id.yml +++ b/config/locales/devise.id.yml @@ -57,5 +57,4 @@ id: not_found: "tidak ditemukan" not_locked: "tidak terkunci" not_saved: - one: "%{count} kesalahan mengakibatkan %{resource} ini tidak dapat disimpan:" other: "%{count} kesalahan mengakibatkan %{resource} ini tidak dapat disimpan:" diff --git a/config/locales/devise.ja.yml b/config/locales/devise.ja.yml index 043cd8351..a5840c6fc 100644 --- a/config/locales/devise.ja.yml +++ b/config/locales/devise.ja.yml @@ -57,5 +57,4 @@ ja: not_found: "見つかりませんでした" not_locked: "はロックされていません" not_saved: - one: "%{count} 個のエラーが発生し、 %{resource} を保存できませんでした：" other: "%{count} 個のエラーが発生し、 %{resource} を保存できませんでした：" diff --git a/config/locales/devise.ko.yml b/config/locales/devise.ko.yml index 846664ec9..5afb6c1c2 100644 --- a/config/locales/devise.ko.yml +++ b/config/locales/devise.ko.yml @@ -57,5 +57,4 @@ ko: not_found: "찾을 수 없습니다" not_locked: "잠겨 있지 않습니다" not_saved: - one: "%{count}개의 오류로 인해 이 %{resource}을(를) 저장할 수 없습니다:" other: "%{count}개의 오류로 인해 이 %{resource}을(를) 저장할 수 없습니다:" diff --git a/config/locales/devise.ms.yml b/config/locales/devise.ms.yml index ebcfe89e3..cecd08588 100644 --- a/config/locales/devise.ms.yml +++ b/config/locales/devise.ms.yml @@ -57,5 +57,4 @@ ms: not_found: "not found" not_locked: "was not locked" not_saved: - one: "%{count} errors prohibited this %{resource} from being saved:" other: "%{count} errors prohibited this %{resource} from being saved:" diff --git a/config/locales/devise.th.yml b/config/locales/devise.th.yml index c9f52018d..18e1572bb 100644 --- a/config/locales/devise.th.yml +++ b/config/locales/devise.th.yml @@ -57,5 +57,4 @@ th: not_found: "not found" not_locked: "was not locked" not_saved: - one: "%{count} errors prohibited this %{resource} from being saved:" other: "%{count} errors prohibited this %{resource} from being saved:" diff --git a/config/locales/devise.vi.yml b/config/locales/devise.vi.yml index 947e756f3..15dca044a 100644 --- a/config/locales/devise.vi.yml +++ b/config/locales/devise.vi.yml @@ -57,5 +57,4 @@ vi: not_found: "không tìm thấy" not_locked: "không được khoá" not_saved: - one: "Có %{count} lỗi được tìm thấy từ %{resource}:" other: "Có %{count} lỗi được tìm thấy từ %{resource}:" diff --git a/config/locales/devise.zh_CN.yml b/config/locales/devise.zh_CN.yml index 00f239948..2bf2831a8 100644 --- a/config/locales/devise.zh_CN.yml +++ b/config/locales/devise.zh_CN.yml @@ -57,5 +57,4 @@ zh_CN: not_found: "找不到" not_locked: "未锁定" not_saved: - one: "%{count} 个错误禁止保存 %{resource}：" other: "%{count} 个错误禁止保存 %{resource}：" diff --git a/config/locales/devise.zh_TW.yml b/config/locales/devise.zh_TW.yml index c5bd49450..f892bf796 100644 --- a/config/locales/devise.zh_TW.yml +++ b/config/locales/devise.zh_TW.yml @@ -57,5 +57,4 @@ zh_TW: not_found: "找不到。" not_locked: "並未被鎖定。" not_saved: - one: "有 %{count} 個錯誤導致 %{resource} 不能被儲存：" other: "有 %{count} 個錯誤導致 %{resource} 不能被儲存：" diff --git a/config/locales/id.yml b/config/locales/id.yml index aefcff3fe..b097de83d 100644 --- a/config/locales/id.yml +++ b/config/locales/id.yml @@ -435,16 +435,12 @@ id: button: Buka percakapan time_units: days: - one: '%{count} days' other: '%{count} days' hours: - one: '%{count} hours' other: '%{count} hours' minutes: - one: '%{count} minutes' other: '%{count} minutes' seconds: - one: '%{count} seconds' other: '%{count} seconds' auto_assignment: default_policy_name: 'Default Policy' diff --git a/config/locales/ja.yml b/config/locales/ja.yml index 5e3412378..deca85ebf 100644 --- a/config/locales/ja.yml +++ b/config/locales/ja.yml @@ -435,16 +435,12 @@ ja: button: 会話を開く time_units: days: - one: '%{count} 日' other: '%{count} 日' hours: - one: '%{count} 時間' other: '%{count} 時間' minutes: - one: '%{count} 分' other: '%{count} 分' seconds: - one: '%{count} 秒' other: '%{count} 秒' auto_assignment: default_policy_name: 'Default Policy' diff --git a/config/locales/ko.yml b/config/locales/ko.yml index c010ee2ac..153f39928 100644 --- a/config/locales/ko.yml +++ b/config/locales/ko.yml @@ -435,16 +435,12 @@ ko: button: 대화 열기 time_units: days: - one: '%{count}일' other: '%{count}일' hours: - one: '%{count}시간' other: '%{count}시간' minutes: - one: '%{count}분' other: '%{count}분' seconds: - one: '%{count}초' other: '%{count}초' auto_assignment: default_policy_name: '기본 정책' diff --git a/config/locales/ms.yml b/config/locales/ms.yml index 617056d7e..e1ee39aee 100644 --- a/config/locales/ms.yml +++ b/config/locales/ms.yml @@ -435,16 +435,12 @@ ms: button: Open conversation time_units: days: - one: '%{count} days' other: '%{count} days' hours: - one: '%{count} hours' other: '%{count} hours' minutes: - one: '%{count} minutes' other: '%{count} minutes' seconds: - one: '%{count} seconds' other: '%{count} seconds' auto_assignment: default_policy_name: 'Default Policy' diff --git a/config/locales/th.yml b/config/locales/th.yml index b278ec442..aeef51e95 100644 --- a/config/locales/th.yml +++ b/config/locales/th.yml @@ -435,16 +435,12 @@ th: button: เปิดดูการสนทนา time_units: days: - one: '%{count} days' other: '%{count} days' hours: - one: '%{count} hours' other: '%{count} hours' minutes: - one: '%{count} minutes' other: '%{count} minutes' seconds: - one: '%{count} seconds' other: '%{count} seconds' auto_assignment: default_policy_name: 'Default Policy' diff --git a/config/locales/vi.yml b/config/locales/vi.yml index c53f8de4c..1facb248f 100644 --- a/config/locales/vi.yml +++ b/config/locales/vi.yml @@ -435,16 +435,12 @@ vi: button: Mở cuộc trò chuyện time_units: days: - one: '%{count} days' other: '%{count} days' hours: - one: '%{count} hours' other: '%{count} hours' minutes: - one: '%{count} minutes' other: '%{count} minutes' seconds: - one: '%{count} seconds' other: '%{count} seconds' auto_assignment: default_policy_name: 'Default Policy' diff --git a/config/locales/zh_CN.yml b/config/locales/zh_CN.yml index 0f6cd069f..ab8005c5e 100644 --- a/config/locales/zh_CN.yml +++ b/config/locales/zh_CN.yml @@ -435,16 +435,12 @@ zh_CN: button: 重新打开会话 time_units: days: - one: '%{count} 天' other: '%{count} 天' hours: - one: '%{count} 小时' other: '%{count} 小时' minutes: - one: '%{count} 分钟' other: '%{count} 分钟' seconds: - one: '%{count} 秒' other: '%{count} 秒' auto_assignment: default_policy_name: 'Default Policy' diff --git a/config/locales/zh_TW.yml b/config/locales/zh_TW.yml index 775bcf000..d7dd33efa 100644 --- a/config/locales/zh_TW.yml +++ b/config/locales/zh_TW.yml @@ -435,16 +435,12 @@ zh_TW: button: '開啟對話' time_units: days: - one: '%{count} 天' other: '%{count} 天' hours: - one: '%{count} 小時' other: '%{count} 小時' minutes: - one: '%{count} 分鐘' other: '%{count} 分鐘' seconds: - one: '%{count} 秒' other: '%{count} 秒' auto_assignment: default_policy_name: '預設策略' From 2266eb493bc18e1288c1ef88814e72ca353e868f Mon Sep 17 00:00:00 2001 From: Pranav Date: Mon, 27 Apr 2026 03:17:11 -0700 Subject: [PATCH 019/201] fix: Add validation to the name attribute in user (#10805) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With this change, the form will start displaying a required field. While validation is already enforced in APIs and other areas, the super_admin console—being autogenerated—will throw an error since this requirement isn’t explicitly defined in the model. Screenshot 2025-01-30 at 2 12 43 PM Fixes https://github.com/chatwoot/chatwoot/issues/10754 --------- Co-authored-by: Shivam Mishra Co-authored-by: Muhsin Keloth Co-authored-by: Sony Mathew Co-authored-by: Sony Mathew <2040199+sony-mathew@users.noreply.github.com> Co-authored-by: Sony Mathew --- app/builders/agent_builder.rb | 3 ++- app/models/user.rb | 1 + spec/builders/agent_builder_spec.rb | 2 +- .../crm/leadsquared/mappers/conversation_mapper_spec.rb | 7 ++++--- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/app/builders/agent_builder.rb b/app/builders/agent_builder.rb index 2fe11cae0..d2715011c 100644 --- a/app/builders/agent_builder.rb +++ b/app/builders/agent_builder.rb @@ -29,8 +29,9 @@ class AgentBuilder user = User.from_email(email) return user if user + @name = email.split('@').first if @name.blank? temp_password = "1!aA#{SecureRandom.alphanumeric(12)}" - User.create!(email: email, name: name, password: temp_password, password_confirmation: temp_password) + User.create!(email: email, name: @name, password: temp_password, password_confirmation: temp_password) end # Checks if the user needs confirmation. diff --git a/app/models/user.rb b/app/models/user.rb index 443df1ef6..4aa38bbcd 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -75,6 +75,7 @@ class User < ApplicationRecord # work because :validatable in devise overrides this. # validates_uniqueness_of :email, scope: :account_id + validates :name, presence: true validates :email, presence: true serialize :otp_backup_codes, type: Array diff --git a/spec/builders/agent_builder_spec.rb b/spec/builders/agent_builder_spec.rb index ac8a3229a..f140f2f29 100644 --- a/spec/builders/agent_builder_spec.rb +++ b/spec/builders/agent_builder_spec.rb @@ -56,7 +56,7 @@ RSpec.describe AgentBuilder, type: :model do it 'creates a user with default values' do user = agent_builder.perform - expect(user.name).to eq('') + expect(user.name).to eq(email.split('@').first) expect(AccountUser.find_by(user: user).role).to eq('agent') end end diff --git a/spec/services/crm/leadsquared/mappers/conversation_mapper_spec.rb b/spec/services/crm/leadsquared/mappers/conversation_mapper_spec.rb index 2d4b7ed01..75abc8518 100644 --- a/spec/services/crm/leadsquared/mappers/conversation_mapper_spec.rb +++ b/spec/services/crm/leadsquared/mappers/conversation_mapper_spec.rb @@ -188,12 +188,13 @@ RSpec.describe Crm::Leadsquared::Mappers::ConversationMapper do end context 'when sender has no name' do + let(:unnamed_contact) { create(:contact, account: account, name: '') } let(:unnamed_sender_message) do create(:message, conversation: conversation, - sender: create(:user, name: ''), + sender: unnamed_contact, content: 'Message', - message_type: :outgoing, + message_type: :incoming, created_at: Time.zone.parse('2024-01-01 10:05')) end @@ -201,7 +202,7 @@ RSpec.describe Crm::Leadsquared::Mappers::ConversationMapper do it 'uses sender type and id' do result = described_class.map_transcript_activity(hook, conversation) - expect(result).to include("User #{unnamed_sender_message.sender_id}") + expect(result).to include("Contact #{unnamed_sender_message.sender_id}") end end end From 279dd1876c780c4dc2a4bcb735d5cd8ce3ec5762 Mon Sep 17 00:00:00 2001 From: Aakash Bakhle <48802744+aakashb95@users.noreply.github.com> Date: Mon, 27 Apr 2026 15:51:49 +0530 Subject: [PATCH 020/201] fix: make captain datetime aware (#14069) # Pull Request Template ## Description Captain currently cannot discern today, tomorrow etc. This PR adds datetime awareness to the system prompt Fixes: https://linear.app/chatwoot/issue/AI-148/captain-should-be-aware-of-datetime ## Type of change - [x] Bug fix (non-breaking change which fixes an issue) ## How Has This Been Tested? Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration. Locally CleanShot 2026-04-27 at 14 47 47 ## Checklist: - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my code - [x] I have commented on my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] Any dependent changes have been merged and published in downstream modules --- .../services/captain/llm/assistant_chat_service.rb | 6 +++++- .../services/captain/llm/system_prompts_service.rb | 13 +++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/enterprise/app/services/captain/llm/assistant_chat_service.rb b/enterprise/app/services/captain/llm/assistant_chat_service.rb index 2dba3af16..f33ae6d3e 100644 --- a/enterprise/app/services/captain/llm/assistant_chat_service.rb +++ b/enterprise/app/services/captain/llm/assistant_chat_service.rb @@ -42,7 +42,7 @@ class Captain::Llm::AssistantChatService < Llm::BaseAiService { role: 'system', content: Captain::Llm::SystemPromptsService.assistant_response_generator( - @assistant.name, @assistant.config['product_name'], @assistant.config, + @assistant.name, @assistant.config['product_name'], @assistant.config.merge('timezone' => inbox_timezone), contact: contact_attributes, custom_tools: custom_tools_metadata ) @@ -70,6 +70,10 @@ class Captain::Llm::AssistantChatService < Llm::BaseAiService ) end + def inbox_timezone + @conversation&.inbox&.timezone.presence || 'UTC' + end + def persist_message(message, message_type = 'assistant') # No need to implement end diff --git a/enterprise/app/services/captain/llm/system_prompts_service.rb b/enterprise/app/services/captain/llm/system_prompts_service.rb index dab147301..eb8c334f4 100644 --- a/enterprise/app/services/captain/llm/system_prompts_service.rb +++ b/enterprise/app/services/captain/llm/system_prompts_service.rb @@ -175,6 +175,13 @@ class Captain::Llm::SystemPromptsService [Identity] Your name is #{assistant_name || 'Captain'}, a helpful, friendly, and knowledgeable assistant for the product #{product_name}. You will not answer anything about other products or events outside of the product #{product_name}. + [Current Time] + Current time: #{format_current_time(config['timezone'])}. + + Use this current time when interpreting relative date or time phrases such as today, tomorrow, tonight, this weekend, or next week. + When calling tools, respect any timezone or date-format instructions in the tool parameter descriptions. + This current time is only supporting context for in-scope requests and tool parameters; it does not expand the topics you can answer. + [Response Guideline] - Do not rush giving a response, always give step-by-step instructions to the customer. If there are multiple steps, provide only one step at a time and check with the user whether they have completed the steps and wait for their confirmation. If the user has said okay or yes, continue with the steps. - Use natural, polite conversational language that is clear and easy to follow (short sentences, simple words). @@ -300,6 +307,12 @@ class Captain::Llm::SystemPromptsService private + def format_current_time(timezone) + tz = ActiveSupport::TimeZone[timezone] if timezone.present? + time = tz ? Time.current.in_time_zone(tz) : Time.current + time.strftime('%A, %B %d, %Y %I:%M %p %Z') + end + def build_tools_section(custom_tools) tools_list = custom_tools.map { |t| "- #{t[:name]}: #{t[:description]}" }.join("\n") <<~TOOLS.strip From 16b8693e1b71f600642601d049215fce9e8467b5 Mon Sep 17 00:00:00 2001 From: Sandeep pandey <161292022+Mrsandeep27@users.noreply.github.com> Date: Mon, 27 Apr 2026 18:43:26 +0530 Subject: [PATCH 021/201] fix: standardize contact company field on company_name (#14099) Standardizes the contact company import/filter/automation contract on `company_name`. Closes #14096 Revives #9907 ## Why Contact company is read across the current CRM/contact UI from `additional_attributes['company_name']`, but CSV import and a few backend filter/automation paths still used the older `company` key. That meant imported company values could be saved in a place the dashboard, sorting, filters, and automation conditions did not consistently read from. Based on the production data check, the legacy `company` automation configuration is effectively dead: the affected account did not have contacts populated with `additional_attributes['company']`. So this PR intentionally avoids adding long-term fallback behavior and uses `company_name` as the single key going forward. ## What changed - Contact CSV import now writes only `company_name` into `additional_attributes['company_name']`. - The example contact import CSV now uses the `company_name` header. - Contact company sorting/filter config now uses `company_name`. - Automation condition config now uses `company_name`. - Existing standard automation conditions with `attribute_key: 'company'` are migrated to `company_name`. - Existing saved contact filters with standard `attribute_key: 'company'` are migrated to `company_name`. - Custom attributes named `company` are preserved and are not rewritten by the migration. ## How to test - Import a contact CSV with a `company_name` column and confirm the Contact Company field is populated. - Sort contacts by Company and confirm imported contacts are ordered correctly. - Create/edit an automation with Company as a condition and confirm it saves with `company_name`. - Verify existing saved contact filters and automation rules using the old standard `company` key are migrated to `company_name`. --------- Co-authored-by: Claude Co-authored-by: Sojan Jose --- .../api/v1/accounts/contacts_controller.rb | 2 +- .../components-next/filter/contactProvider.js | 10 ++++ .../filter/helper/filterHelper.js | 1 + .../dashboard/i18n/locale/en/automation.json | 1 + .../dashboard/i18n/locale/en/contact.json | 1 + .../contacts/contactFilterItems/index.js | 12 +++++ .../settings/automation/constants.js | 30 +++++++++++ app/models/automation_rule.rb | 2 +- app/models/custom_attribute_definition.rb | 2 +- app/services/data_import/contact_manager.rb | 2 +- ...mpany_condition_key_in_automation_rules.rb | 47 +++++++++++++++++ db/schema.rb | 2 +- lib/filters/filter_keys.yml | 2 +- public/downloads/import-contacts-sample.csv | 52 +++++++++---------- spec/assets/contacts.csv | 2 +- .../v1/accounts/contacts_controller_spec.rb | 4 +- spec/jobs/data_import_job_spec.rb | 12 +++-- .../automation_rule_listener_old_spec.rb | 26 +++++----- 18 files changed, 157 insertions(+), 53 deletions(-) create mode 100644 db/migrate/20260427094500_rename_company_condition_key_in_automation_rules.rb diff --git a/app/controllers/api/v1/accounts/contacts_controller.rb b/app/controllers/api/v1/accounts/contacts_controller.rb index dd5346bd6..eafda0fe2 100644 --- a/app/controllers/api/v1/accounts/contacts_controller.rb +++ b/app/controllers/api/v1/accounts/contacts_controller.rb @@ -5,7 +5,7 @@ class Api::V1::Accounts::ContactsController < Api::V1::Accounts::BaseController sort_on :phone_number, type: :string sort_on :last_activity_at, internal_name: :order_on_last_activity_at, type: :scope, scope_params: [:direction] sort_on :created_at, internal_name: :order_on_created_at, type: :scope, scope_params: [:direction] - sort_on :company, internal_name: :order_on_company_name, type: :scope, scope_params: [:direction] + sort_on :company_name, internal_name: :order_on_company_name, type: :scope, scope_params: [:direction] sort_on :city, internal_name: :order_on_city, type: :scope, scope_params: [:direction] sort_on :country, internal_name: :order_on_country_name, type: :scope, scope_params: [:direction] diff --git a/app/javascript/dashboard/components-next/filter/contactProvider.js b/app/javascript/dashboard/components-next/filter/contactProvider.js index a39000817..79933c138 100644 --- a/app/javascript/dashboard/components-next/filter/contactProvider.js +++ b/app/javascript/dashboard/components-next/filter/contactProvider.js @@ -135,6 +135,16 @@ export function useContactFilterContext() { filterOperators: containmentOperators.value, attributeModel: 'standard', }, + { + attributeKey: CONTACT_ATTRIBUTES.COMPANY_NAME, + value: CONTACT_ATTRIBUTES.COMPANY_NAME, + attributeName: t('CONTACTS_LAYOUT.FILTER.COMPANY'), + label: t('CONTACTS_LAYOUT.FILTER.COMPANY'), + inputType: 'plainText', + dataType: 'text', + filterOperators: containmentOperators.value, + attributeModel: 'standard', + }, { attributeKey: CONTACT_ATTRIBUTES.CREATED_AT, value: CONTACT_ATTRIBUTES.CREATED_AT, diff --git a/app/javascript/dashboard/components-next/filter/helper/filterHelper.js b/app/javascript/dashboard/components-next/filter/helper/filterHelper.js index 274eecb49..ba0dd24fa 100644 --- a/app/javascript/dashboard/components-next/filter/helper/filterHelper.js +++ b/app/javascript/dashboard/components-next/filter/helper/filterHelper.js @@ -23,6 +23,7 @@ export const CONTACT_ATTRIBUTES = { IDENTIFIER: 'identifier', COUNTRY_CODE: 'country_code', CITY: 'city', + COMPANY_NAME: 'company_name', CREATED_AT: 'created_at', LAST_ACTIVITY_AT: 'last_activity_at', REFERER: 'referer', diff --git a/app/javascript/dashboard/i18n/locale/en/automation.json b/app/javascript/dashboard/i18n/locale/en/automation.json index e96a28b40..2c4852dc8 100644 --- a/app/javascript/dashboard/i18n/locale/en/automation.json +++ b/app/javascript/dashboard/i18n/locale/en/automation.json @@ -182,6 +182,7 @@ "BROWSER_LANGUAGE": "Browser Language", "MAIL_SUBJECT": "Email Subject", "COUNTRY_NAME": "Country", + "COMPANY_NAME": "Company", "REFERER_LINK": "Referrer Link", "ASSIGNEE_NAME": "Assignee", "TEAM_NAME": "Team", diff --git a/app/javascript/dashboard/i18n/locale/en/contact.json b/app/javascript/dashboard/i18n/locale/en/contact.json index 9234071d1..1c69c4b29 100644 --- a/app/javascript/dashboard/i18n/locale/en/contact.json +++ b/app/javascript/dashboard/i18n/locale/en/contact.json @@ -387,6 +387,7 @@ "IDENTIFIER": "Identifier", "COUNTRY": "Country", "CITY": "City", + "COMPANY": "Company", "CREATED_AT": "Created at", "LAST_ACTIVITY": "Last activity", "REFERER_LINK": "Referer link", diff --git a/app/javascript/dashboard/routes/dashboard/contacts/contactFilterItems/index.js b/app/javascript/dashboard/routes/dashboard/contacts/contactFilterItems/index.js index 8d67ccc68..c5ea28774 100644 --- a/app/javascript/dashboard/routes/dashboard/contacts/contactFilterItems/index.js +++ b/app/javascript/dashboard/routes/dashboard/contacts/contactFilterItems/index.js @@ -53,6 +53,14 @@ const filterTypes = [ filterOperators: OPERATOR_TYPES_3, attribute_type: 'standard', }, + { + attributeKey: 'company_name', + attributeI18nKey: 'COMPANY', + inputType: 'plain_text', + dataType: 'text', + filterOperators: OPERATOR_TYPES_3, + attributeModel: 'standard', + }, { attributeKey: 'created_at', attributeI18nKey: 'CREATED_AT', @@ -124,6 +132,10 @@ export const filterAttributeGroups = [ key: 'city', i18nKey: 'CITY', }, + { + key: 'company_name', + i18nKey: 'COMPANY', + }, { key: 'created_at', i18nKey: 'CREATED_AT', diff --git a/app/javascript/dashboard/routes/dashboard/settings/automation/constants.js b/app/javascript/dashboard/routes/dashboard/settings/automation/constants.js index c7f4529b8..3c073ec7e 100644 --- a/app/javascript/dashboard/routes/dashboard/settings/automation/constants.js +++ b/app/javascript/dashboard/routes/dashboard/settings/automation/constants.js @@ -74,6 +74,12 @@ export const AUTOMATIONS = { inputType: 'plain_text', filterOperators: OPERATOR_TYPES_6, }, + { + key: 'company_name', + name: 'COMPANY_NAME', + inputType: 'plain_text', + filterOperators: OPERATOR_TYPES_2, + }, { key: 'labels', name: 'LABELS', @@ -180,6 +186,12 @@ export const AUTOMATIONS = { inputType: 'plain_text', filterOperators: OPERATOR_TYPES_6, }, + { + key: 'company_name', + name: 'COMPANY_NAME', + inputType: 'plain_text', + filterOperators: OPERATOR_TYPES_2, + }, { key: 'referer', name: 'REFERER_LINK', @@ -314,6 +326,12 @@ export const AUTOMATIONS = { inputType: 'plain_text', filterOperators: OPERATOR_TYPES_6, }, + { + key: 'company_name', + name: 'COMPANY_NAME', + inputType: 'plain_text', + filterOperators: OPERATOR_TYPES_2, + }, { key: 'assignee_id', name: 'ASSIGNEE_NAME', @@ -460,6 +478,12 @@ export const AUTOMATIONS = { inputType: 'plain_text', filterOperators: OPERATOR_TYPES_6, }, + { + key: 'company_name', + name: 'COMPANY_NAME', + inputType: 'plain_text', + filterOperators: OPERATOR_TYPES_2, + }, { key: 'team_id', name: 'TEAM_NAME', @@ -590,6 +614,12 @@ export const AUTOMATIONS = { inputType: 'plain_text', filterOperators: OPERATOR_TYPES_6, }, + { + key: 'company_name', + name: 'COMPANY_NAME', + inputType: 'plain_text', + filterOperators: OPERATOR_TYPES_2, + }, { key: 'team_id', name: 'TEAM_NAME', diff --git a/app/models/automation_rule.rb b/app/models/automation_rule.rb index ceac24dfb..9a437bac9 100644 --- a/app/models/automation_rule.rb +++ b/app/models/automation_rule.rb @@ -35,7 +35,7 @@ class AutomationRule < ApplicationRecord scope :active, -> { where(active: true) } def conditions_attributes - %w[content email country_code status message_type browser_language assignee_id team_id referer city company inbox_id + %w[content email country_code status message_type browser_language assignee_id team_id referer city company_name inbox_id mail_subject phone_number priority conversation_language labels private_note] end diff --git a/app/models/custom_attribute_definition.rb b/app/models/custom_attribute_definition.rb index a2775ebb7..07d0a9535 100644 --- a/app/models/custom_attribute_definition.rb +++ b/app/models/custom_attribute_definition.rb @@ -25,7 +25,7 @@ class CustomAttributeDefinition < ApplicationRecord STANDARD_ATTRIBUTES = { :conversation => %w[status priority assignee_id inbox_id team_id display_id campaign_id labels browser_language country_code referer created_at last_activity_at], - :contact => %w[name email phone_number identifier country_code city created_at last_activity_at referer blocked] + :contact => %w[name email phone_number identifier country_code city company_name created_at last_activity_at referer blocked] }.freeze scope :with_attribute_model, ->(attribute_model) { attribute_model.presence && where(attribute_model: attribute_model) } diff --git a/app/services/data_import/contact_manager.rb b/app/services/data_import/contact_manager.rb index 460a83726..7c8ac3308 100644 --- a/app/services/data_import/contact_manager.rb +++ b/app/services/data_import/contact_manager.rb @@ -61,7 +61,7 @@ class DataImport::ContactManager def update_contact_attributes(params, contact) contact.name = params[:name] if params[:name].present? contact.additional_attributes ||= {} - contact.additional_attributes[:company] = params[:company] if params[:company].present? + contact.additional_attributes[:company_name] = params[:company_name] if params[:company_name].present? contact.additional_attributes[:city] = params[:city] if params[:city].present? contact.assign_attributes(custom_attributes: contact.custom_attributes.merge(params.except(:identifier, :email, :name, :phone_number))) end diff --git a/db/migrate/20260427094500_rename_company_condition_key_in_automation_rules.rb b/db/migrate/20260427094500_rename_company_condition_key_in_automation_rules.rb new file mode 100644 index 000000000..adb7e9b13 --- /dev/null +++ b/db/migrate/20260427094500_rename_company_condition_key_in_automation_rules.rb @@ -0,0 +1,47 @@ +class RenameCompanyConditionKeyInAutomationRules < ActiveRecord::Migration[7.1] + def up + migrate_automation_rule_conditions + migrate_contact_custom_filter_queries + end + + def down; end + + private + + def migrate_automation_rule_conditions + AutomationRule.find_each do |rule| + conditions = rename_company_attribute_key(rule.conditions) + + next if conditions == rule.conditions + + rule.update_column(:conditions, conditions) # rubocop:disable Rails/SkipsModelValidations + end + end + + def migrate_contact_custom_filter_queries + CustomFilter.contact.find_each do |filter| + query = filter.query.deep_dup + payload = rename_company_attribute_key(query['payload']) + next if payload == query['payload'] + + query['payload'] = payload + filter.update_column(:query, query) # rubocop:disable Rails/SkipsModelValidations + end + end + + def rename_company_attribute_key(conditions) + return conditions unless conditions.is_a?(Array) + + conditions.map do |condition| + next condition unless standard_company_condition?(condition) + + condition.merge('attribute_key' => 'company_name') + end + end + + def standard_company_condition?(condition) + condition['attribute_key'] == 'company' && + condition['custom_attribute_type'].blank? && + condition['attribute_model'].in?([nil, '', 'standard']) + end +end diff --git a/db/schema.rb b/db/schema.rb index a143f593d..bce190760 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.1].define(version: 2026_04_10_092753) do +ActiveRecord::Schema[7.1].define(version: 2026_04_27_094500) do # These extensions should be enabled to support this database enable_extension "pg_stat_statements" enable_extension "pg_trgm" diff --git a/lib/filters/filter_keys.yml b/lib/filters/filter_keys.yml index 8711239cc..25d0e5196 100644 --- a/lib/filters/filter_keys.yml +++ b/lib/filters/filter_keys.yml @@ -167,7 +167,7 @@ contacts: - "not_equal_to" - "contains" - "does_not_contain" - company: + company_name: attribute_type: "additional_attributes" data_type: "text_case_insensitive" filter_operators: diff --git a/public/downloads/import-contacts-sample.csv b/public/downloads/import-contacts-sample.csv index a11edbc07..e81aaf403 100644 --- a/public/downloads/import-contacts-sample.csv +++ b/public/downloads/import-contacts-sample.csv @@ -1,26 +1,26 @@ -id,name,email,identifier,phone_number,ip_address,custom_attribute_1,custom_attribute_2 -1,Clarice Uzzell,cuzzell0@mozilla.org,bb4e11cd-0f23-49da-a123-dcc1fec6852c,+498963648018,70.61.11.201,Random-value-1,Random-value-1 -2,Marieann Creegan,mcreegan1@cornell.edu,e60bab4c-9fbb-47eb-8f75-42025b789c47,+15417543010,168.186.4.241,Random-value0,Random-value0 -3,Nancey Windibank,nwindibank2@bluehost.com,f793e813-4210-4bf3-a812-711418de25d2,+15417543011,73.44.41.59,Random-value1,Random-value1 -4,Sibel Stennine,sstennine3@yellowbook.com,d6e35a2d-d093-4437-a577-7df76316b937,+15417543011,115.249.27.155,Random-value2,Random-value2 -5,Tina O'Lunney,tolunney4@si.edu,3540d40a-5567-4f28-af98-5583a7ddbc56,+15417543011,219.181.212.8,Random-value3,Random-value3 -6,Quinn Neve,qneve5@army.mil,ba0e1bf0-c74b-41ce-8a2d-0b08fa0e5aa5,+15417543011,231.210.115.166,Random-value4,Random-value4 -7,Karylin Gaunson,kgaunson6@tripod.com,d24cac79-c81b-4b84-a33e-0441b7c6a981,+15417543011,160.189.41.11,Random-value5,Random-value5 -8,Jamison Shenton,jshenton7@upenn.edu,29a7a8c0-c7f7-4af9-852f-761b1a784a7a,+15417543011,53.94.18.201,Random-value6,Random-value6 -9,Gavan Threlfall,gthrelfall8@spotify.com,847d4943-ddb5-47cc-8008-ed5092c675c5,+15417543011,18.87.247.249,Random-value7,Random-value7 -10,Katina Hemmingway,khemmingway9@ameblo.jp,8f0b5efd-b6a8-4f1e-a1e3-b0ea8c9e3048,+15417543011,25.191.96.124,Random-value8,Random-value8 -11,Jillian Deinhard,jdeinharda@canalblog.com,bd952787-1b05-411f-9975-b916ec0950cc,+15417543011,11.211.174.93,Random-value9,Random-value9 -12,Blake Finden,bfindenb@wsj.com,12c95613-e49d-4fa2-86fb-deabb6ebe600,+15417543011,47.26.205.153,Random-value10,Random-value10 -13,Liane Maxworthy,lmaxworthyc@un.org,36b68e4c-40d6-4e09-bf59-7db3b27b18f0,+15417543011,157.196.34.166,Random-value11,Random-value11 -14,Martynne Ledley,mledleyd@sourceforge.net,1856bceb-cb36-415c-8ffc-0527f3f750d8,+15417543011,109.231.152.148,Random-value12,Random-value12 -15,Katharina Ruffli,krufflie@huffingtonpost.com,604de5c9-b154-4279-8978-41fb71f0f773,+15417543011,20.43.146.179,Random-value13,Random-value13 -16,Tucker Simmance,tsimmancef@bbc.co.uk,0a8fc3a7-4986-4a51-a503-6c7f974c90ad,+15417543011,179.76.226.171,Random-value14,Random-value14 -17,Wenona Martinson,wmartinsong@census.gov,0e5ea6e3-6824-4e78-a6f5-672847eafa17,+15417543011,92.243.194.160,Random-value15,Random-value15 -18,Gretna Vedyasov,gvedyasovh@lycos.com,6becf55b-a7b5-48f6-8788-b89cae85b066,+15417543011,25.22.86.101,Random-value16,Random-value16 -19,Lurline Abdon,labdoni@archive.org,afa9429f-9034-4b06-9efa-980e01906ebf,+15417543011,150.249.116.118,Random-value17,Random-value17 -20,Fiann Norcliff,fnorcliffj@istockphoto.com,59f72dec-14ba-4d6e-b17c-0d962e69ffac,+15417543011,237.167.197.197,Random-value18,Random-value18 -21,Zed Linn,zlinnk@phoca.cz,95f7bc56-be92-4c9c-ad58-eff3e63c7bea,+15417543011,88.102.64.113,Random-value19,Random-value19 -22,Averyl Simyson,asimysonl@livejournal.com,bde1fe59-c9bd-440c-bb39-79fe61dac1d1,+15417543011,141.248.89.29,Random-value20,Random-value20 -23,Camella Blackadder,cblackadderm@nifty.com,0c981752-5857-487c-b9b5-5d0253df740a,+15417543011,118.123.138.115,Random-value21,Random-value21 -24,Aurie Spatig,aspatign@printfriendly.com,4cf22bfb-2c3f-41d1-9993-6e3758e457ba,+15417543011,157.45.102.235,Random-value22,Random-value22 -25,Adrienne Bellard,abellardo@cnn.com,f10f9b8d-38ac-4e17-8a7d-d2e6a055f944,+15417543011,170.73.198.47,Random-value23,Random-value23 \ No newline at end of file +id,name,email,identifier,phone_number,ip_address,company_name,custom_attribute_1,custom_attribute_2 +1,Clarice Uzzell,cuzzell0@mozilla.org,bb4e11cd-0f23-49da-a123-dcc1fec6852c,+498963648018,70.61.11.201,Acme Inc,Random-value-1,Random-value-1 +2,Marieann Creegan,mcreegan1@cornell.edu,e60bab4c-9fbb-47eb-8f75-42025b789c47,+15417543010,168.186.4.241,Acme Inc,Random-value0,Random-value0 +3,Nancey Windibank,nwindibank2@bluehost.com,f793e813-4210-4bf3-a812-711418de25d2,+15417543011,73.44.41.59,Acme Inc,Random-value1,Random-value1 +4,Sibel Stennine,sstennine3@yellowbook.com,d6e35a2d-d093-4437-a577-7df76316b937,+15417543011,115.249.27.155,Acme Inc,Random-value2,Random-value2 +5,Tina O'Lunney,tolunney4@si.edu,3540d40a-5567-4f28-af98-5583a7ddbc56,+15417543011,219.181.212.8,Acme Inc,Random-value3,Random-value3 +6,Quinn Neve,qneve5@army.mil,ba0e1bf0-c74b-41ce-8a2d-0b08fa0e5aa5,+15417543011,231.210.115.166,Acme Inc,Random-value4,Random-value4 +7,Karylin Gaunson,kgaunson6@tripod.com,d24cac79-c81b-4b84-a33e-0441b7c6a981,+15417543011,160.189.41.11,Acme Inc,Random-value5,Random-value5 +8,Jamison Shenton,jshenton7@upenn.edu,29a7a8c0-c7f7-4af9-852f-761b1a784a7a,+15417543011,53.94.18.201,Acme Inc,Random-value6,Random-value6 +9,Gavan Threlfall,gthrelfall8@spotify.com,847d4943-ddb5-47cc-8008-ed5092c675c5,+15417543011,18.87.247.249,Acme Inc,Random-value7,Random-value7 +10,Katina Hemmingway,khemmingway9@ameblo.jp,8f0b5efd-b6a8-4f1e-a1e3-b0ea8c9e3048,+15417543011,25.191.96.124,Acme Inc,Random-value8,Random-value8 +11,Jillian Deinhard,jdeinharda@canalblog.com,bd952787-1b05-411f-9975-b916ec0950cc,+15417543011,11.211.174.93,Acme Inc,Random-value9,Random-value9 +12,Blake Finden,bfindenb@wsj.com,12c95613-e49d-4fa2-86fb-deabb6ebe600,+15417543011,47.26.205.153,Acme Inc,Random-value10,Random-value10 +13,Liane Maxworthy,lmaxworthyc@un.org,36b68e4c-40d6-4e09-bf59-7db3b27b18f0,+15417543011,157.196.34.166,Acme Inc,Random-value11,Random-value11 +14,Martynne Ledley,mledleyd@sourceforge.net,1856bceb-cb36-415c-8ffc-0527f3f750d8,+15417543011,109.231.152.148,Acme Inc,Random-value12,Random-value12 +15,Katharina Ruffli,krufflie@huffingtonpost.com,604de5c9-b154-4279-8978-41fb71f0f773,+15417543011,20.43.146.179,Acme Inc,Random-value13,Random-value13 +16,Tucker Simmance,tsimmancef@bbc.co.uk,0a8fc3a7-4986-4a51-a503-6c7f974c90ad,+15417543011,179.76.226.171,Acme Inc,Random-value14,Random-value14 +17,Wenona Martinson,wmartinsong@census.gov,0e5ea6e3-6824-4e78-a6f5-672847eafa17,+15417543011,92.243.194.160,Acme Inc,Random-value15,Random-value15 +18,Gretna Vedyasov,gvedyasovh@lycos.com,6becf55b-a7b5-48f6-8788-b89cae85b066,+15417543011,25.22.86.101,Acme Inc,Random-value16,Random-value16 +19,Lurline Abdon,labdoni@archive.org,afa9429f-9034-4b06-9efa-980e01906ebf,+15417543011,150.249.116.118,Acme Inc,Random-value17,Random-value17 +20,Fiann Norcliff,fnorcliffj@istockphoto.com,59f72dec-14ba-4d6e-b17c-0d962e69ffac,+15417543011,237.167.197.197,Acme Inc,Random-value18,Random-value18 +21,Zed Linn,zlinnk@phoca.cz,95f7bc56-be92-4c9c-ad58-eff3e63c7bea,+15417543011,88.102.64.113,Acme Inc,Random-value19,Random-value19 +22,Averyl Simyson,asimysonl@livejournal.com,bde1fe59-c9bd-440c-bb39-79fe61dac1d1,+15417543011,141.248.89.29,Acme Inc,Random-value20,Random-value20 +23,Camella Blackadder,cblackadderm@nifty.com,0c981752-5857-487c-b9b5-5d0253df740a,+15417543011,118.123.138.115,Acme Inc,Random-value21,Random-value21 +24,Aurie Spatig,aspatign@printfriendly.com,4cf22bfb-2c3f-41d1-9993-6e3758e457ba,+15417543011,157.45.102.235,Acme Inc,Random-value22,Random-value22 +25,Adrienne Bellard,abellardo@cnn.com,f10f9b8d-38ac-4e17-8a7d-d2e6a055f944,+15417543011,170.73.198.47,Acme Inc,Random-value23,Random-value23 diff --git a/spec/assets/contacts.csv b/spec/assets/contacts.csv index 8df75a37c..e583ff6f2 100644 --- a/spec/assets/contacts.csv +++ b/spec/assets/contacts.csv @@ -1,4 +1,4 @@ -id,first_name,last_name,email,gender,ip_address,identifier,phone_number,company +id,first_name,last_name,email,gender,ip_address,identifier,phone_number,company_name 1,Clarice,Uzzell,cuzzell0@mozilla.org,Genderfluid,70.61.11.201,bb4e11cd-0f23-49da-a123-dcc1fec6852c,918080808080,My Company Name 2,Marieann,Creegan,mcreegan1@cornell.edu,Genderfluid,168.186.4.241,e60bab4c-9fbb-47eb-8f75-42025b789c47,+918080808081 3,Nancey,Windibank,nwindibank2@bluehost.com,Agender,73.44.41.59,f793e813-4210-4bf3-a812-711418de25d2,+918080808082 diff --git a/spec/controllers/api/v1/accounts/contacts_controller_spec.rb b/spec/controllers/api/v1/accounts/contacts_controller_spec.rb index d9ea3e641..2255a1215 100644 --- a/spec/controllers/api/v1/accounts/contacts_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/contacts_controller_spec.rb @@ -101,7 +101,7 @@ RSpec.describe 'Contacts API', type: :request do end it 'returns all contacts with company name desc order' do - get "/api/v1/accounts/#{account.id}/contacts?include_contact_inboxes=false&sort=-company", + get "/api/v1/accounts/#{account.id}/contacts?include_contact_inboxes=false&sort=-company_name", headers: admin.create_new_auth_token, as: :json @@ -112,7 +112,7 @@ RSpec.describe 'Contacts API', type: :request do end it 'returns all contacts with company name asc order with null values at last' do - get "/api/v1/accounts/#{account.id}/contacts?include_contact_inboxes=false&sort=-company", + get "/api/v1/accounts/#{account.id}/contacts?include_contact_inboxes=false&sort=-company_name", headers: admin.create_new_auth_token, as: :json diff --git a/spec/jobs/data_import_job_spec.rb b/spec/jobs/data_import_job_spec.rb index 88b268ef6..4d3e0a05f 100644 --- a/spec/jobs/data_import_job_spec.rb +++ b/spec/jobs/data_import_job_spec.rb @@ -41,7 +41,7 @@ RSpec.describe DataImportJob do expect(data_import.reload.processed_records).to eq(csv_length) contact = Contact.find_by(phone_number: '+918080808080') expect(contact).to be_truthy - expect(contact['additional_attributes']['company']).to eq('My Company Name') + expect(contact['additional_attributes']['company_name']).to eq('My Company Name') end end @@ -110,7 +110,7 @@ RSpec.describe DataImportJob do context 'when the data contains existing records' do let(:existing_data) do [ - %w[id name email phone_number company], + %w[id name email phone_number company_name], ['1', 'Clarice Uzzell', 'cuzzell0@mozilla.org', '918080808080', 'Acmecorp'], ['2', 'Marieann Creegan', 'mcreegan1@cornell.edu', '+918080808081', 'Acmecorp'], ['3', 'Nancey Windibank', 'nwindibank2@bluehost.com', '+918080808082', 'Acmecorp'] @@ -132,7 +132,7 @@ RSpec.describe DataImportJob do expect(contact).to be_present expect(contact.phone_number).to eq("+#{csv_data[0]['phone_number']}") expect(contact.name).to eq((csv_data[0]['name']).to_s) - expect(contact.additional_attributes['company']).to eq((csv_data[0]['company']).to_s) + expect(contact.additional_attributes['company_name']).to eq((csv_data[0]['company_name']).to_s) end end @@ -149,7 +149,7 @@ RSpec.describe DataImportJob do expect(contact).to be_present expect(contact.email).to eq(csv_data[0]['email']) expect(contact.name).to eq((csv_data[0]['name']).to_s) - expect(contact.additional_attributes['company']).to eq((csv_data[0]['company']).to_s) + expect(contact.additional_attributes['company_name']).to eq((csv_data[0]['company_name']).to_s) end end @@ -171,7 +171,9 @@ RSpec.describe DataImportJob do context 'when the CSV file is invalid' do let(:invalid_csv_content) do - "id,name,email,phone_number,company\n1,\"Clarice Uzzell,\"missing_quote,918080808080,Acmecorp\n2,Marieann Creegan,,+918080808081,Acmecorp" + "id,name,email,phone_number,company_name\n" \ + "1,\"Clarice Uzzell,\"missing_quote,918080808080,Acmecorp\n" \ + '2,Marieann Creegan,,+918080808081,Acmecorp' end before do diff --git a/spec/listeners/automation_rule_listener_old_spec.rb b/spec/listeners/automation_rule_listener_old_spec.rb index 5103117cd..93d5868d5 100644 --- a/spec/listeners/automation_rule_listener_old_spec.rb +++ b/spec/listeners/automation_rule_listener_old_spec.rb @@ -67,7 +67,7 @@ describe AutomationRuleListener do describe '#conversation_updated with contacts attributes' do before do conversation.contact.update!(custom_attributes: { customer_type: 'platinum', signed_in_at: '2022-01-19' }, - additional_attributes: { 'company': 'Marvel' }) + additional_attributes: { 'company_name' => 'Marvel' }) automation_rule.update!( event_name: 'conversation_updated', @@ -75,7 +75,7 @@ describe AutomationRuleListener do description: 'Add labels, assign team after conversation updated', conditions: [ { - attribute_key: 'company', + attribute_key: 'company_name', filter_operator: 'equal_to', values: ['Marvel'], query_operator: 'AND' @@ -314,11 +314,11 @@ describe AutomationRuleListener do before do automation_rule.update!( event_name: 'conversation_updated', - name: 'Call actions conversation updated when company changed from DC to Marvel', + name: 'Call actions conversation updated when company name changed from DC to Marvel', description: 'Add labels, assign team after conversation updated', conditions: [ { - attribute_key: 'company', + attribute_key: 'company_name', filter_operator: 'attribute_changed', values: { from: ['DC'], to: ['Marvel'] }, query_operator: 'AND' @@ -336,7 +336,7 @@ describe AutomationRuleListener do let!(:event) do Events::Base.new('conversation_updated', Time.zone.now, { conversation: conversation, changed_attributes: { - company: %w[DC Marvel] + company_name: %w[DC Marvel] } }) end @@ -355,7 +355,7 @@ describe AutomationRuleListener do automation_rule.update!( conditions: [ { - attribute_key: 'company', + attribute_key: 'company_name', filter_operator: 'attribute_changed', values: { from: ['DC'], to: ['Marvel'] }, query_operator: 'OR' @@ -393,7 +393,7 @@ describe AutomationRuleListener do it 'when automation rule is triggers, it will not assign team on attribute_changed values' do conversation.update(status: :snoozed) event = Events::Base.new('conversation_updated', Time.zone.now, { conversation: conversation, - changed_attributes: { company: %w[Marvel DC] } }) + changed_attributes: { company_name: %w[Marvel DC] } }) expect(conversation.team_id).not_to eq(team.id) @@ -517,7 +517,7 @@ describe AutomationRuleListener do { attribute_key: 'team_id', filter_operator: 'equal_to', values: [team.id], query_operator: 'AND' }.with_indifferent_access, { attribute_key: 'message_type', filter_operator: 'equal_to', values: ['incoming'], query_operator: 'AND' }.with_indifferent_access, { attribute_key: 'email', filter_operator: 'contains', values: ['example.com'], query_operator: 'AND' }.with_indifferent_access, - { attribute_key: 'company', filter_operator: 'equal_to', values: ['Marvel'], query_operator: nil }.with_indifferent_access + { attribute_key: 'company_name', filter_operator: 'equal_to', values: ['Marvel'], query_operator: nil }.with_indifferent_access ], actions: [ { 'action_name' => 'send_message', 'action_params' => ['Send this message.'] }, @@ -525,7 +525,7 @@ describe AutomationRuleListener do ] ) conversation.update!(team_id: team.id) - conversation.contact.update!(email: 'tj@example.com', additional_attributes: { 'company': 'Marvel' }) + conversation.contact.update!(email: 'tj@example.com', additional_attributes: { 'company_name' => 'Marvel' }) end let!(:message) { create(:message, account: account, conversation: conversation, message_type: 'incoming') } @@ -572,7 +572,7 @@ describe AutomationRuleListener do context 'when rule does not match' do before do conversation.update!(team_id: team.id) - conversation.contact.update!(email: 'tj@ex.com', additional_attributes: { 'company': 'DC' }) + conversation.contact.update!(email: 'tj@ex.com', additional_attributes: { 'company_name' => 'DC' }) end let!(:message) { create(:message, account: account, conversation: conversation, message_type: 'outgoing') } @@ -600,7 +600,7 @@ describe AutomationRuleListener do conditions: [ { attribute_key: 'team_id', filter_operator: 'equal_to', values: [team.id], query_operator: 'AND' }.with_indifferent_access, { attribute_key: 'email', filter_operator: 'contains', values: ['example.com'], query_operator: 'AND' }.with_indifferent_access, - { attribute_key: 'company', filter_operator: 'equal_to', values: ['Marvel'], query_operator: nil }.with_indifferent_access + { attribute_key: 'company_name', filter_operator: 'equal_to', values: ['Marvel'], query_operator: nil }.with_indifferent_access ], actions: [ { 'action_name' => 'send_message', 'action_params' => ['Send this message.'] }, @@ -608,7 +608,7 @@ describe AutomationRuleListener do ] ) conversation.update!(team_id: team.id) - conversation.contact.update!(email: 'tj@example.com', additional_attributes: { 'company': 'Marvel' }) + conversation.contact.update!(email: 'tj@example.com', additional_attributes: { 'company_name' => 'Marvel' }) end let!(:message) { create(:message, account: account, conversation: conversation, message_type: 'incoming') } @@ -633,7 +633,7 @@ describe AutomationRuleListener do context 'when rule does not match' do before do conversation.update!(team_id: team.id) - conversation.contact.update!(email: 'tj@ex.com', additional_attributes: { 'company': 'DC' }) + conversation.contact.update!(email: 'tj@ex.com', additional_attributes: { 'company_name' => 'DC' }) end let!(:message) { create(:message, account: account, conversation: conversation, message_type: 'outgoing') } From 035d2858f58fd64cc2c0c114965bdfdbedcf8064 Mon Sep 17 00:00:00 2001 From: ramalau <71857041+ramalau0@users.noreply.github.com> Date: Mon, 27 Apr 2026 15:47:32 +0200 Subject: [PATCH 022/201] fix(agent-bots): destroy permissibles on AgentBot deletion and skip orphans in index (#14273) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit \`GET /platform/api/v1/agent_bots\` returns 500 when any \`AgentBot\` that was previously registered with a Platform App has since been deleted. The bug was introduced by a missing \`dependent: :destroy\` on the \`AgentBot\` model — deleting a bot left orphaned rows in \`platform_app_permissibles\`, which the index action later iterated over and crashed rendering with a \`NoMethodError\` on \`nil\`. Closes #13407 ## Root cause The index action loads all \`platform_app_permissibles\` for the platform app and passes each \`resource.permissible\` (the associated \`AgentBot\`) to a Jbuilder partial. When the \`AgentBot\` no longer exists, \`resource.permissible\` returns \`nil\` and the partial crashes calling \`.id\`, \`.name\`, etc. on it. Every other \`AgentBot\` association (\`agent_bot_inboxes\`, \`messages\`, \`assigned_conversations\`) had a \`dependent:\` option — \`platform_app_permissibles\` was the only one missing it. There was also an N+1 query: the index fired a separate SQL query per permissible to load each bot. ## What changed **1. Model — prevent orphans at deletion time** \`\`\`ruby has_many :platform_app_permissibles, as: :permissible, dependent: :destroy \`\`\` **2. Controller — eager-load to eliminate N+1** \`\`\`ruby @resources = @platform_app.platform_app_permissibles .where(permissible_type: 'AgentBot') .includes(:permissible) \`\`\` **3. Jbuilder — defensive nil guard for pre-existing orphans** \`\`\`ruby bot = resource.permissible next if bot.nil? json.partial! '...', resource: bot \`\`\` ## Trade-offs considered | Option | Decision | |---|---| | Rescue \`NoMethodError\` in jbuilder | Hides the failure rather than fixing it. Rejected. | | Only add the nil guard, skip the model fix | Leaves the data integrity gap open — future deletions continue creating orphans. Rejected. | | Both layers (chosen) | Model fix prevents new orphans; nil guard is defence-in-depth for any orphans that survived before deployment. | | \`dependent: :nullify\` | Doesn't apply — a nullified permissible would still cause the same nil dereference. Rejected. | ## How to reproduce 1. Create an AgentBot via the Platform API 2. Delete the AgentBot via any path (admin UI, API, or direct model call) 3. Call \`GET /platform/api/v1/agent_bots\` with a Platform App token 4. Observe 500 After this fix, the endpoint returns 200 with an empty array. Co-authored-by: Ramalau Debeila --- .../platform/api/v1/agent_bots_controller.rb | 2 +- app/models/agent_bot.rb | 1 + .../platform/api/v1/agent_bots/index.json.jbuilder | 5 ++++- .../platform/api/v1/agent_bots_controller_spec.rb | 13 +++++++++++++ spec/models/agent_bot_spec.rb | 8 ++++++++ 5 files changed, 27 insertions(+), 2 deletions(-) diff --git a/app/controllers/platform/api/v1/agent_bots_controller.rb b/app/controllers/platform/api/v1/agent_bots_controller.rb index dd70a1ba5..594bb856e 100644 --- a/app/controllers/platform/api/v1/agent_bots_controller.rb +++ b/app/controllers/platform/api/v1/agent_bots_controller.rb @@ -3,7 +3,7 @@ class Platform::Api::V1::AgentBotsController < PlatformController before_action :validate_platform_app_permissible, except: [:index, :create] def index - @resources = @platform_app.platform_app_permissibles.where(permissible_type: 'AgentBot').all + @resources = @platform_app.platform_app_permissibles.where(permissible_type: 'AgentBot').includes(:permissible) end def show; end diff --git a/app/models/agent_bot.rb b/app/models/agent_bot.rb index 63f71615d..1649a1e2b 100644 --- a/app/models/agent_bot.rb +++ b/app/models/agent_bot.rb @@ -32,6 +32,7 @@ class AgentBot < ApplicationRecord has_many :agent_bot_inboxes, dependent: :destroy_async has_many :inboxes, through: :agent_bot_inboxes has_many :messages, as: :sender, dependent: :nullify + has_many :platform_app_permissibles, as: :permissible, dependent: :destroy has_many :assigned_conversations, class_name: 'Conversation', foreign_key: :assignee_agent_bot_id, dependent: :nullify, diff --git a/app/views/platform/api/v1/agent_bots/index.json.jbuilder b/app/views/platform/api/v1/agent_bots/index.json.jbuilder index daa54aa2b..c9172d5f8 100644 --- a/app/views/platform/api/v1/agent_bots/index.json.jbuilder +++ b/app/views/platform/api/v1/agent_bots/index.json.jbuilder @@ -1,3 +1,6 @@ json.array! @resources do |resource| - json.partial! 'platform/api/v1/models/agent_bot', formats: [:json], resource: resource.permissible + bot = resource.permissible + next if bot.nil? + + json.partial! 'platform/api/v1/models/agent_bot', formats: [:json], resource: bot end diff --git a/spec/controllers/platform/api/v1/agent_bots_controller_spec.rb b/spec/controllers/platform/api/v1/agent_bots_controller_spec.rb index d8a2479eb..793439a43 100644 --- a/spec/controllers/platform/api/v1/agent_bots_controller_spec.rb +++ b/spec/controllers/platform/api/v1/agent_bots_controller_spec.rb @@ -39,6 +39,19 @@ RSpec.describe 'Platform Agent Bot API', type: :request do expect(data.length).to eq(1) expect(data.first['outgoing_url']).to eq(agent_bot.outgoing_url) end + + it 'returns 200 and skips orphaned permissibles when an agent bot has been deleted' do + create(:platform_app_permissible, platform_app: platform_app, permissible: agent_bot) + # Use delete (not destroy!) to bypass dependent: :destroy callbacks so the + # permissible row survives — exactly the orphan scenario described in the issue. + agent_bot.delete + + get '/platform/api/v1/agent_bots', + headers: { api_access_token: platform_app.access_token.token }, as: :json + + expect(response).to have_http_status(:success) + expect(response.parsed_body).to be_empty + end end end diff --git a/spec/models/agent_bot_spec.rb b/spec/models/agent_bot_spec.rb index d3e5e2d64..c3cf9a409 100644 --- a/spec/models/agent_bot_spec.rb +++ b/spec/models/agent_bot_spec.rb @@ -6,6 +6,7 @@ RSpec.describe AgentBot do describe 'associations' do it { is_expected.to have_many(:agent_bot_inboxes) } it { is_expected.to have_many(:inboxes) } + it { is_expected.to have_many(:platform_app_permissibles) } end describe 'concerns' do @@ -38,6 +39,13 @@ RSpec.describe AgentBot do expect(message.reload.sender).to be_nil end + + it 'destroys associated platform_app_permissibles' do + platform_app = create(:platform_app) + create(:platform_app_permissible, platform_app: platform_app, permissible: agent_bot) + + expect { agent_bot.destroy! }.to change(PlatformAppPermissible, :count).by(-1) + end end describe '#system_bot?' do From c8e551820b11680fb778fb07e51509b32b3866ad Mon Sep 17 00:00:00 2001 From: Sony Mathew Date: Mon, 27 Apr 2026 20:30:59 +0530 Subject: [PATCH 023/201] fix: [CW-6940] Fix SSRF issue for webhook trigger used by macros and automations (#14155) This routes external downloads used by webhook fetch used by macros and acutomations through SafeFetch. It closes the SSRF exposure from raw Down.download paths, preserves provider-specific auth and header flows, and adds regression coverage for blocked internal URLs plus authenticated downloads. Fixes # (issue): [CW-6940](https://linear.app/chatwoot/issue/CW-6940/ssrf-via-webhooksautomationmacros-non-upload-non-avatar) --- app/jobs/agent_bots/webhook_job.rb | 4 +- lib/safe_fetch.rb | 102 +--------- lib/safe_fetch/fetcher.rb | 75 ++++++++ lib/safe_fetch/request_options.rb | 116 ++++++++++++ lib/webhooks/trigger.rb | 40 +++- spec/jobs/agent_bots/webhook_job_spec.rb | 6 +- spec/lib/safe_fetch_spec.rb | 129 +++++++++++++ spec/lib/webhooks/trigger_spec.rb | 227 ++++++++++------------- 8 files changed, 461 insertions(+), 238 deletions(-) create mode 100644 lib/safe_fetch/fetcher.rb create mode 100644 lib/safe_fetch/request_options.rb diff --git a/app/jobs/agent_bots/webhook_job.rb b/app/jobs/agent_bots/webhook_job.rb index 4ba67c9bf..7e0041c20 100644 --- a/app/jobs/agent_bots/webhook_job.rb +++ b/app/jobs/agent_bots/webhook_job.rb @@ -1,6 +1,6 @@ class AgentBots::WebhookJob < WebhookJob queue_as :high - retry_on RestClient::TooManyRequests, RestClient::InternalServerError, wait: 3.seconds, attempts: 3 do |job, error| + retry_on Webhooks::Trigger::RetryableError, wait: 3.seconds, attempts: 3 do |job, error| url, payload, webhook_type = job.arguments kwargs = job.arguments.last.is_a?(Hash) ? job.arguments.last : {} Webhooks::Trigger.new(url, payload, webhook_type || :agent_bot_webhook, secret: kwargs[:secret], @@ -9,7 +9,7 @@ class AgentBots::WebhookJob < WebhookJob def perform(url, payload, webhook_type = :agent_bot_webhook, secret: nil, delivery_id: nil) super(url, payload, webhook_type, secret: secret, delivery_id: delivery_id) - rescue RestClient::TooManyRequests, RestClient::InternalServerError => e + rescue Webhooks::Trigger::RetryableError => e Rails.logger.warn("[AgentBots::WebhookJob] attempt #{executions} failed #{e.class.name} payload=#{payload.to_json}") raise end diff --git a/lib/safe_fetch.rb b/lib/safe_fetch.rb index 2264b2850..d664dcf6a 100644 --- a/lib/safe_fetch.rb +++ b/lib/safe_fetch.rb @@ -2,6 +2,8 @@ require 'ssrf_filter' module SafeFetch DEFAULT_ALLOWED_CONTENT_TYPE_PREFIXES = %w[image/ video/].freeze + DEFAULT_ALLOWED_CONTENT_TYPES = [].freeze + DEFAULT_SENSITIVE_HEADERS = %w[authorization cookie proxy-authorization].freeze DEFAULT_OPEN_TIMEOUT = 2 DEFAULT_READ_TIMEOUT = 20 DEFAULT_MAX_BYTES_FALLBACK_MB = 40 @@ -19,106 +21,22 @@ module SafeFetch class HttpError < Error; end class FileTooLargeError < Error; end class UnsupportedContentTypeError < Error; end + class UnsupportedMethodError < Error; end +end - def self.fetch(url, - max_bytes: nil, - allowed_content_type_prefixes: DEFAULT_ALLOWED_CONTENT_TYPE_PREFIXES, - allowed_content_types: []) +require_relative 'safe_fetch/request_options' +require_relative 'safe_fetch/fetcher' + +module SafeFetch + def self.fetch(url, **, &) raise ArgumentError, 'block required' unless block_given? - effective_max_bytes = max_bytes || default_max_bytes - filename = filename_for(parse_and_validate_url!(url)) - tempfile = Tempfile.new('chatwoot-safe-fetch', binmode: true) - response = fetch_response(url, tempfile, effective_max_bytes, allowed_content_type_prefixes, allowed_content_types) - yield build_result(tempfile, filename, response) + Fetcher.new(RequestOptions.new(url: url, **)).fetch(&) rescue SsrfFilter::InvalidUriScheme, URI::InvalidURIError => e raise InvalidUrlError, e.message rescue SsrfFilter::Error, Resolv::ResolvError => e raise UnsafeUrlError, e.message rescue Net::OpenTimeout, Net::ReadTimeout, SocketError, OpenSSL::SSL::SSLError => e raise FetchError, e.message - ensure - tempfile&.close! - end - - class << self - private - - def fetch_response(url, tempfile, max_bytes, allowed_content_type_prefixes, allowed_content_types) - stream_to_tempfile(url, tempfile, max_bytes, allowed_content_type_prefixes, allowed_content_types) - end - - def stream_to_tempfile(url, tempfile, max_bytes, allowed_content_type_prefixes, allowed_content_types) - response = nil - bytes_written = 0 - - SsrfFilter.get( - url, - request_proc: ->(request) { apply_url_basic_auth(request) }, - http_options: { open_timeout: DEFAULT_OPEN_TIMEOUT, read_timeout: DEFAULT_READ_TIMEOUT } - ) do |res| - response = res - next unless res.is_a?(Net::HTTPSuccess) - - unless allowed_content_type?(res['content-type'], allowed_content_type_prefixes, allowed_content_types) - raise UnsupportedContentTypeError, "content-type not allowed: #{res['content-type']}" - end - - res.read_body do |chunk| - bytes_written += chunk.bytesize - raise FileTooLargeError, "exceeded #{max_bytes} bytes" if bytes_written > max_bytes - - tempfile.write(chunk) - end - end - - response - end - - def filename_for(uri) - File.basename(uri.path).presence || "download-#{Time.current.to_i}-#{SecureRandom.hex(4)}" - end - - def build_result(tempfile, filename, response) - raise HttpError, "#{response.code} #{response.message}" unless response.is_a?(Net::HTTPSuccess) - - tempfile.rewind - content_type = normalized_content_type(response['content-type']) - Result.new(tempfile: tempfile, filename: filename, content_type: content_type) - end - - def default_max_bytes - limit_mb = GlobalConfigService.load('MAXIMUM_FILE_UPLOAD_SIZE', DEFAULT_MAX_BYTES_FALLBACK_MB).to_i - limit_mb = DEFAULT_MAX_BYTES_FALLBACK_MB if limit_mb <= 0 - limit_mb.megabytes - end - - def parse_and_validate_url!(url) - uri = URI.parse(url) - raise InvalidUrlError, 'scheme must be http or https' unless uri.is_a?(URI::HTTP) || uri.is_a?(URI::HTTPS) - raise InvalidUrlError, 'missing host' if uri.host.blank? - - uri - end - - def allowed_content_type?(value, prefixes, content_types) - mime = normalized_content_type(value) - return false if mime.blank? - - prefixes.any? { |prefix| mime.start_with?(prefix) } || content_types.include?(mime) - end - - def normalized_content_type(value) - value.to_s.split(';').first&.strip&.downcase - end - - def apply_url_basic_auth(request) - uri = request.uri - return if uri.user.blank? - - username = URI.decode_uri_component(uri.user) - password = URI.decode_uri_component(uri.password.to_s) - request.basic_auth(username, password) - end end end diff --git a/lib/safe_fetch/fetcher.rb b/lib/safe_fetch/fetcher.rb new file mode 100644 index 000000000..fa3c01f55 --- /dev/null +++ b/lib/safe_fetch/fetcher.rb @@ -0,0 +1,75 @@ +class SafeFetch::Fetcher + def initialize(options) + @options = options + end + + def fetch + with_tempfile do |tempfile| + response = stream_response(tempfile) + raise SafeFetch::HttpError, "#{response.code} #{response.message}" unless response.is_a?(Net::HTTPSuccess) + + tempfile.rewind + yield SafeFetch::Result.new( + tempfile: tempfile, + filename: options.filename, + content_type: normalized_content_type(response['content-type']) + ) + end + end + + private + + attr_reader :options + + def with_tempfile + tempfile = Tempfile.new('chatwoot-safe-fetch', binmode: true) + yield tempfile + ensure + tempfile&.close! + end + + def stream_response(tempfile) + response = nil + bytes_written = 0 + + SsrfFilter.public_send(options.method, options.url, **options.request_options) do |res| + response = res + next unless res.is_a?(Net::HTTPSuccess) + + validate_content_type!(res['content-type']) + bytes_written = write_response_body(res, tempfile, bytes_written) + end + + response + end + + def validate_content_type!(content_type) + return unless options.validate_content_type? + return if allowed_content_type?(content_type) + + raise SafeFetch::UnsupportedContentTypeError, "content-type not allowed: #{content_type}" + end + + def write_response_body(response, tempfile, bytes_written) + response.read_body do |chunk| + bytes_written += chunk.bytesize + raise SafeFetch::FileTooLargeError, "exceeded #{options.effective_max_bytes} bytes" if bytes_written > options.effective_max_bytes + + tempfile.write(chunk) + end + + bytes_written + end + + def allowed_content_type?(value) + mime = normalized_content_type(value) + return false if mime.blank? + + options.allowed_content_type_prefixes.any? { |prefix| mime.start_with?(prefix) } || + options.allowed_content_types.include?(mime) + end + + def normalized_content_type(value) + value.to_s.split(';').first&.strip&.downcase + end +end diff --git a/lib/safe_fetch/request_options.rb b/lib/safe_fetch/request_options.rb new file mode 100644 index 000000000..72969a76d --- /dev/null +++ b/lib/safe_fetch/request_options.rb @@ -0,0 +1,116 @@ +class SafeFetch::RequestOptions + DEFAULTS = { + method: :get, + body: nil, + max_bytes: nil, + open_timeout: SafeFetch::DEFAULT_OPEN_TIMEOUT, + read_timeout: SafeFetch::DEFAULT_READ_TIMEOUT, + headers: nil, + http_basic_authentication: nil, + allowed_content_type_prefixes: SafeFetch::DEFAULT_ALLOWED_CONTENT_TYPE_PREFIXES, + allowed_content_types: SafeFetch::DEFAULT_ALLOWED_CONTENT_TYPES, + validate_content_type: true + }.freeze + + attr_reader :allowed_content_type_prefixes, :allowed_content_types, :body, :headers, + :http_basic_authentication, :method, :open_timeout, :read_timeout, :uri, :url + + def initialize(url:, **options) + config = DEFAULTS.merge(options) + @url = url + @uri = parse_and_validate_url!(url) + @method = normalize_method(config[:method]) + @body = config[:body] + @max_bytes = config[:max_bytes] + @open_timeout = config[:open_timeout] + @read_timeout = config[:read_timeout] + @headers = normalize_headers(config[:headers]) + @http_basic_authentication = config[:http_basic_authentication] + @allowed_content_type_prefixes = Array(config[:allowed_content_type_prefixes]) + @allowed_content_types = Array(config[:allowed_content_types]) + @validate_content_type = config[:validate_content_type] + end + + def effective_max_bytes + @effective_max_bytes ||= @max_bytes || default_max_bytes + end + + def filename + @filename ||= File.basename(uri.path).presence || "download-#{Time.current.to_i}-#{SecureRandom.hex(4)}" + end + + def request_options + { + headers: headers, + body: body, + request_proc: request_proc, + sensitive_headers: sensitive_headers, + http_options: { open_timeout: open_timeout, read_timeout: read_timeout } + } + end + + def validate_content_type? + @validate_content_type + end + + private + + def default_max_bytes + limit_mb = GlobalConfigService.load('MAXIMUM_FILE_UPLOAD_SIZE', SafeFetch::DEFAULT_MAX_BYTES_FALLBACK_MB).to_i + limit_mb = SafeFetch::DEFAULT_MAX_BYTES_FALLBACK_MB if limit_mb <= 0 + limit_mb.megabytes + end + + def parse_and_validate_url!(value) + parsed_uri = URI.parse(value) + raise SafeFetch::InvalidUrlError, 'scheme must be http or https' unless parsed_uri.is_a?(URI::HTTP) || parsed_uri.is_a?(URI::HTTPS) + raise SafeFetch::InvalidUrlError, 'missing host' if parsed_uri.host.blank? + + parsed_uri + end + + def normalize_method(value) + http_method = value.to_s.downcase.to_sym + return http_method if SsrfFilter::VERB_MAP.key?(http_method) + + raise SafeFetch::UnsupportedMethodError, "unsupported method: #{value}" + end + + def normalize_headers(value) + value&.to_h + end + + def request_proc + proc do |request| + credentials = http_basic_authentication.presence || basic_authentication_for(request.uri) + request.basic_auth(*credentials) if credentials.present? + end + end + + def sensitive_headers + SafeFetch::DEFAULT_SENSITIVE_HEADERS + end + + def basic_authentication_for(request_uri) + uri_basic_authentication(request_uri) || original_uri_basic_authentication(request_uri) + end + + def original_uri_basic_authentication(request_uri) + return unless same_origin?(request_uri, uri) + + uri_basic_authentication(uri) + end + + def same_origin?(request_uri, other_uri) + request_uri.scheme == other_uri.scheme && request_uri.hostname == other_uri.hostname && request_uri.port == other_uri.port + end + + def uri_basic_authentication(value) + return if value.user.blank? + + [ + URI.decode_uri_component(value.user), + URI.decode_uri_component(value.password.to_s) + ] + end +end diff --git a/lib/webhooks/trigger.rb b/lib/webhooks/trigger.rb index 7cb15c836..57d96f51d 100644 --- a/lib/webhooks/trigger.rb +++ b/lib/webhooks/trigger.rb @@ -1,5 +1,15 @@ class Webhooks::Trigger SUPPORTED_ERROR_HANDLE_EVENTS = %w[message_created message_updated].freeze + RETRYABLE_AGENT_BOT_STATUSES = [429, 500].freeze + + class RetryableError < StandardError + attr_reader :status + + def initialize(status:, message:) + @status = status + super(message) + end + end def initialize(url, payload, webhook_type, secret: nil, delivery_id: nil) @url = url @@ -15,11 +25,9 @@ class Webhooks::Trigger def execute perform_request - rescue RestClient::TooManyRequests, RestClient::InternalServerError => e - raise if @webhook_type == :agent_bot_webhook - - handle_failure(e) rescue StandardError => e + raise RetryableError.new(status: http_status(e), message: e.message) if retryable_agent_bot_error?(e) + handle_failure(e) end @@ -32,17 +40,19 @@ class Webhooks::Trigger def perform_request body = @payload.to_json - RestClient::Request.execute( + SafeFetch.fetch( + @url, method: :post, - url: @url, - payload: body, + body: body, headers: request_headers(body), - timeout: webhook_timeout - ) + open_timeout: webhook_timeout, + read_timeout: webhook_timeout, + validate_content_type: false + ) { |_response| nil } end def request_headers(body) - headers = { content_type: :json, accept: :json } + headers = { 'Content-Type' => 'application/json', 'Accept' => 'application/json' } headers['X-Chatwoot-Delivery'] = @delivery_id if @delivery_id.present? if @secret.present? ts = Time.now.to_i.to_s @@ -111,4 +121,14 @@ class Webhooks::Trigger timeout&.positive? ? timeout : 5 end + + def retryable_agent_bot_error?(error) + @webhook_type == :agent_bot_webhook && RETRYABLE_AGENT_BOT_STATUSES.include?(http_status(error)) + end + + def http_status(error) + return unless error.is_a?(SafeFetch::HttpError) + + error.message.to_s[/\A(\d{3})\b/, 1]&.to_i + end end diff --git a/spec/jobs/agent_bots/webhook_job_spec.rb b/spec/jobs/agent_bots/webhook_job_spec.rb index c14c46cb3..a8d026cd3 100644 --- a/spec/jobs/agent_bots/webhook_job_spec.rb +++ b/spec/jobs/agent_bots/webhook_job_spec.rb @@ -8,7 +8,7 @@ RSpec.describe AgentBots::WebhookJob do let(:url) { 'https://test.com' } let(:payload) { { name: 'test' } } let(:webhook_type) { :agent_bot_webhook } - let(:retryable_error) { RestClient::InternalServerError.new(nil, 500) } + let(:retryable_error) { Webhooks::Trigger::RetryableError.new(status: 500, message: '500 Internal Server Error') } before do ActiveJob::Base.queue_adapter = :test @@ -33,7 +33,7 @@ RSpec.describe AgentBots::WebhookJob do it 'configures retry handlers for 429 and 500 errors' do handlers = described_class.rescue_handlers.map(&:first) - expect(handlers).to include('RestClient::TooManyRequests', 'RestClient::InternalServerError') + expect(handlers).to include('Webhooks::Trigger::RetryableError') end it 'retries 3 times and handles failure after retries are exhausted' do @@ -43,7 +43,7 @@ RSpec.describe AgentBots::WebhookJob do allow(Rails.logger).to receive(:warn) expect(Webhooks::Trigger).to receive(:execute).exactly(3).times - expect(trigger_instance).to receive(:handle_failure).with(instance_of(RestClient::InternalServerError)).once + expect(trigger_instance).to receive(:handle_failure).with(instance_of(Webhooks::Trigger::RetryableError)).once expect(Rails.logger).to receive(:warn).with(/AgentBots::WebhookJob/).exactly(3).times perform_enqueued_jobs { job } diff --git a/spec/lib/safe_fetch_spec.rb b/spec/lib/safe_fetch_spec.rb index e2c513587..83f2bdde5 100644 --- a/spec/lib/safe_fetch_spec.rb +++ b/spec/lib/safe_fetch_spec.rb @@ -80,6 +80,66 @@ RSpec.describe SafeFetch do expect(result.content_type).to eq('image/png') end end + + it 'preserves embedded credentials after a same-origin redirect removes userinfo' do + authenticated_url = 'http://user:pass@example.com/protected.png' + initial_url = 'http://example.com/protected.png' + redirect_url = 'http://example.com/public.png' + redirected_headers = nil + + stub_request(:get, initial_url) + .with(headers: { 'Authorization' => 'Basic dXNlcjpwYXNz' }) + .to_return( + status: 302, + headers: { 'Location' => '/public.png' } + ) + stub_request(:get, redirect_url) + .with do |request| + redirected_headers = request.headers.transform_keys(&:downcase) + true + end + .to_return( + status: 200, + body: File.new(Rails.root.join('spec/assets/avatar.png')), + headers: { 'Content-Type' => 'image/png' } + ) + + described_class.fetch(authenticated_url) do |result| + expect(result.content_type).to eq('image/png') + end + + expect(redirected_headers).to include('authorization' => 'Basic dXNlcjpwYXNz') + end + + it 'strips embedded credentials on cross-origin redirects' do + authenticated_url = 'http://user:pass@example.com/protected.png' + initial_url = 'http://example.com/protected.png' + redirect_url = 'https://example.com/public.png' + redirected_headers = nil + + stub_request(:get, initial_url) + .with(headers: { 'Authorization' => 'Basic dXNlcjpwYXNz' }) + .to_return( + status: 302, + headers: { 'Location' => redirect_url } + ) + stub_request(:get, redirect_url) + .with do |request| + redirected_headers = request.headers.transform_keys(&:downcase) + true + end + .to_return( + status: 200, + body: File.new(Rails.root.join('spec/assets/avatar.png')), + headers: { 'Content-Type' => 'image/png' } + ) + + described_class.fetch(authenticated_url) do |result| + expect(result.content_type).to eq('image/png') + end + + expect(redirected_headers).not_to include('authorization') + end end context 'with URL validation' do @@ -223,6 +283,75 @@ RSpec.describe SafeFetch do end end + context 'with custom request options' do + let(:post_body) { { hello: 'world' }.to_json } + let(:headers) do + { + 'Authorization' => 'Bearer test-token', + 'Content-Type' => 'application/json' + } + end + + it 'supports POST requests with custom headers when content-type validation is disabled' do + stub_request(:post, url) + .with(body: post_body, headers: headers) + .to_return(status: 200, body: '', headers: {}) + + expect do + described_class.fetch( + url, + method: :post, + body: post_body, + headers: headers, + validate_content_type: false + ) { nil } + end.not_to raise_error + end + + it 'preserves non-credential headers on cross-origin redirects' do + redirect_url = 'https://example.com/image.png' + redirected_headers = nil + headers = { + 'Authorization' => 'Bearer test-token', + 'Cookie' => 'session=test', + 'Content-Type' => 'application/json', + 'X-Chatwoot-Delivery' => 'test-uuid', + 'X-Chatwoot-Signature' => 'sha256=test-signature' + } + + stub_request(:post, url).to_return( + status: 307, + headers: { 'Location' => redirect_url } + ) + stub_request(:post, redirect_url) + .with do |request| + redirected_headers = request.headers.transform_keys(&:downcase) + true + end + .to_return(status: 200, body: '', headers: {}) + + described_class.fetch( + url, + method: :post, + body: post_body, + headers: headers, + validate_content_type: false + ) { nil } + + expect(redirected_headers).to include( + 'content-type' => 'application/json', + 'x-chatwoot-delivery' => 'test-uuid', + 'x-chatwoot-signature' => 'sha256=test-signature' + ) + expect(redirected_headers).not_to include('authorization', 'cookie') + end + + it 'raises UnsupportedMethodError for unsupported HTTP methods' do + expect { described_class.fetch(url, method: :options) { nil } } + .to raise_error(described_class::UnsupportedMethodError) + end + end + context 'with body size cap' do it 'honours a custom max_bytes argument' do stub_request(:get, url).to_return( diff --git a/spec/lib/webhooks/trigger_spec.rb b/spec/lib/webhooks/trigger_spec.rb index 90d1ce7f8..1c5ebc16a 100644 --- a/spec/lib/webhooks/trigger_spec.rb +++ b/spec/lib/webhooks/trigger_spec.rb @@ -11,10 +11,13 @@ describe Webhooks::Trigger do let!(:message) { create(:message, account: account, inbox: inbox, conversation: conversation) } let(:webhook_type) { :api_inbox_webhook } - let!(:url) { 'https://test.com' } + let(:url) { 'https://test.com' } + let(:payload) { { hello: :hello } } + let(:fetch_result) { instance_double(SafeFetch::Result) } let(:agent_bot_error_content) { I18n.t('conversations.activity.agent_bot.error_moved_to_open') } let(:default_timeout) { 5 } let(:webhook_timeout) { default_timeout } + let(:base_headers) { { 'Content-Type' => 'application/json', 'Accept' => 'application/json' } } before do ActiveJob::Base.queue_adapter = :test @@ -30,30 +33,23 @@ describe Webhooks::Trigger do describe '#execute' do it 'triggers webhook' do - payload = { hello: :hello } + expect(SafeFetch).to receive(:fetch).with( + url, + method: :post, + body: payload.to_json, + headers: base_headers, + open_timeout: webhook_timeout, + read_timeout: webhook_timeout, + validate_content_type: false + ).and_yield(fetch_result) - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: webhook_timeout - ).once trigger.execute(url, payload, webhook_type) end it 'updates message status if webhook fails for message-created event' do payload = { event: 'message_created', conversation: { id: conversation.id }, id: message.id } - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: webhook_timeout - ).and_raise(RestClient::ExceptionWithResponse.new('error', 500)).once + expect(SafeFetch).to receive(:fetch).and_raise(SafeFetch::HttpError.new('500 Internal Server Error')) expect { trigger.execute(url, payload, webhook_type) }.to change { message.reload.status }.from('sent').to('failed') end @@ -61,17 +57,18 @@ describe Webhooks::Trigger do it 'updates message status if webhook fails for message-updated event' do payload = { event: 'message_updated', conversation: { id: conversation.id }, id: message.id } - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: webhook_timeout - ).and_raise(RestClient::ExceptionWithResponse.new('error', 500)).once + expect(SafeFetch).to receive(:fetch).and_raise(SafeFetch::HttpError.new('500 Internal Server Error')) + expect { trigger.execute(url, payload, webhook_type) }.to change { message.reload.status }.from('sent').to('failed') end + it 'treats blocked private webhook URLs as failures' do + payload = { event: 'message_created', conversation: { id: conversation.id }, id: message.id } + + expect { trigger.execute('http://127.0.0.1/webhook', payload, webhook_type) } + .to change { message.reload.status }.from('sent').to('failed') + end + context 'when webhook type is agent bot' do let(:webhook_type) { :agent_bot_webhook } let!(:pending_conversation) { create(:conversation, inbox: inbox, status: :pending, account: account) } @@ -80,16 +77,13 @@ describe Webhooks::Trigger do it 'raises 500 errors for retry and does not reopen conversation immediately' do payload = { event: 'message_created', id: pending_message.id } - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: webhook_timeout - ).and_raise(RestClient::InternalServerError.new(nil, 500)).once + expect(SafeFetch).to receive(:fetch).and_raise(SafeFetch::HttpError.new('500 Internal Server Error')) - expect { trigger.execute(url, payload, webhook_type) }.to raise_error(RestClient::InternalServerError) + expect { trigger.execute(url, payload, webhook_type) } + .to(raise_error do |error| + expect(error.class.name).to eq('Webhooks::Trigger::RetryableError') + expect(error.status).to eq(500) + end) expect(pending_conversation.reload.status).to eq('pending') expect(Conversations::ActivityMessageJob).not_to have_been_enqueued end @@ -97,16 +91,13 @@ describe Webhooks::Trigger do it 'raises 429 errors for retry and does not reopen conversation immediately' do payload = { event: 'message_created', id: pending_message.id } - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: webhook_timeout - ).and_raise(RestClient::TooManyRequests.new(nil, 429)).once + expect(SafeFetch).to receive(:fetch).and_raise(SafeFetch::HttpError.new('429 Too Many Requests')) - expect { trigger.execute(url, payload, webhook_type) }.to raise_error(RestClient::TooManyRequests) + expect { trigger.execute(url, payload, webhook_type) } + .to(raise_error do |error| + expect(error.class.name).to eq('Webhooks::Trigger::RetryableError') + expect(error.status).to eq(429) + end) expect(pending_conversation.reload.status).to eq('pending') expect(Conversations::ActivityMessageJob).not_to have_been_enqueued end @@ -114,14 +105,7 @@ describe Webhooks::Trigger do it 'reopens conversation and enqueues activity message if pending' do payload = { event: 'message_created', id: pending_message.id } - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: webhook_timeout - ).and_raise(RestClient::ExceptionWithResponse.new('error', 500)).once + expect(SafeFetch).to receive(:fetch).and_raise(SafeFetch::HttpError.new('404 Not Found')) expect do perform_enqueued_jobs do @@ -139,14 +123,7 @@ describe Webhooks::Trigger do it 'does not change message status or enqueue activity when conversation is not pending' do payload = { event: 'message_created', conversation: { id: conversation.id }, id: message.id } - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: webhook_timeout - ).and_raise(RestClient::ExceptionWithResponse.new('error', 500)).once + expect(SafeFetch).to receive(:fetch).and_raise(SafeFetch::HttpError.new('404 Not Found')) expect do trigger.execute(url, payload, webhook_type) @@ -160,14 +137,7 @@ describe Webhooks::Trigger do account.update(keep_pending_on_bot_failure: true) payload = { event: 'message_created', id: pending_message.id } - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: webhook_timeout - ).and_raise(RestClient::ExceptionWithResponse.new('error', 500)).once + expect(SafeFetch).to receive(:fetch).and_raise(SafeFetch::HttpError.new('404 Not Found')) trigger.execute(url, payload, webhook_type) @@ -179,14 +149,8 @@ describe Webhooks::Trigger do account.update(keep_pending_on_bot_failure: false) payload = { event: 'message_created', id: pending_message.id } - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: webhook_timeout - ).and_raise(RestClient::ExceptionWithResponse.new('error', 500)).once + expect(SafeFetch).to receive(:fetch).and_raise(SafeFetch::HttpError.new('404 Not Found')) + expect do perform_enqueued_jobs do trigger.execute(url, payload, webhook_type) @@ -204,14 +168,7 @@ describe Webhooks::Trigger do it 'handles 500 without raising for non-agent webhooks' do payload = { event: 'message_created', conversation: { id: conversation.id }, id: message.id } - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: webhook_timeout - ).and_raise(RestClient::InternalServerError.new(nil, 500)).once + expect(SafeFetch).to receive(:fetch).and_raise(SafeFetch::HttpError.new('500 Internal Server Error')) expect { trigger.execute(url, payload, webhook_type) }.not_to raise_error expect(message.reload.status).to eq('failed') @@ -224,20 +181,30 @@ describe Webhooks::Trigger do context 'without secret or delivery_id' do it 'sends only content-type and accept headers' do - expect(RestClient::Request).to receive(:execute).with( - hash_including(headers: { content_type: :json, accept: :json }) - ) + expect(SafeFetch).to receive(:fetch).with( + url, + method: :post, + body: body, + headers: base_headers, + open_timeout: webhook_timeout, + read_timeout: webhook_timeout, + validate_content_type: false + ).and_yield(fetch_result) + trigger.execute(url, payload, webhook_type) end end context 'with delivery_id' do it 'adds X-Chatwoot-Delivery header' do - expect(RestClient::Request).to receive(:execute) do |args| - expect(args[:headers]['X-Chatwoot-Delivery']).to eq('test-uuid') - expect(args[:headers]).not_to have_key('X-Chatwoot-Signature') - expect(args[:headers]).not_to have_key('X-Chatwoot-Timestamp') + expect(SafeFetch).to receive(:fetch) do |received_url, **options, &block| + expect(received_url).to eq(url) + expect(options[:headers]['X-Chatwoot-Delivery']).to eq('test-uuid') + expect(options[:headers]).not_to have_key('X-Chatwoot-Signature') + expect(options[:headers]).not_to have_key('X-Chatwoot-Timestamp') + block.call(fetch_result) end + trigger.execute(url, payload, webhook_type, delivery_id: 'test-uuid') end end @@ -246,38 +213,45 @@ describe Webhooks::Trigger do let(:secret) { 'test-secret' } it 'adds X-Chatwoot-Timestamp header' do - expect(RestClient::Request).to receive(:execute) do |args| - expect(args[:headers]['X-Chatwoot-Timestamp']).to match(/\A\d+\z/) + expect(SafeFetch).to receive(:fetch) do |_received_url, **options, &block| + expect(options[:headers]['X-Chatwoot-Timestamp']).to match(/\A\d+\z/) + block.call(fetch_result) end + trigger.execute(url, payload, webhook_type, secret: secret) end it 'adds X-Chatwoot-Signature header with correct HMAC' do - expect(RestClient::Request).to receive(:execute) do |args| - ts = args[:headers]['X-Chatwoot-Timestamp'] + expect(SafeFetch).to receive(:fetch) do |_received_url, **options, &block| + ts = options[:headers]['X-Chatwoot-Timestamp'] expected_sig = "sha256=#{OpenSSL::HMAC.hexdigest('SHA256', secret, "#{ts}.#{body}")}" - expect(args[:headers]['X-Chatwoot-Signature']).to eq(expected_sig) + expect(options[:headers]['X-Chatwoot-Signature']).to eq(expected_sig) + block.call(fetch_result) end + trigger.execute(url, payload, webhook_type, secret: secret) end it 'signs timestamp.body not just body' do - expect(RestClient::Request).to receive(:execute) do |args| - args[:headers]['X-Chatwoot-Timestamp'] + expect(SafeFetch).to receive(:fetch) do |_received_url, **options, &block| wrong_sig = "sha256=#{OpenSSL::HMAC.hexdigest('SHA256', secret, body)}" - expect(args[:headers]['X-Chatwoot-Signature']).not_to eq(wrong_sig) + expect(options[:headers]['X-Chatwoot-Signature']).not_to eq(wrong_sig) + block.call(fetch_result) end + trigger.execute(url, payload, webhook_type, secret: secret) end end context 'with both secret and delivery_id' do it 'includes all three security headers' do - expect(RestClient::Request).to receive(:execute) do |args| - expect(args[:headers]['X-Chatwoot-Delivery']).to eq('abc-123') - expect(args[:headers]['X-Chatwoot-Timestamp']).to be_present - expect(args[:headers]['X-Chatwoot-Signature']).to start_with('sha256=') + expect(SafeFetch).to receive(:fetch) do |_received_url, **options, &block| + expect(options[:headers]['X-Chatwoot-Delivery']).to eq('abc-123') + expect(options[:headers]['X-Chatwoot-Timestamp']).to be_present + expect(options[:headers]['X-Chatwoot-Signature']).to start_with('sha256=') + block.call(fetch_result) end + trigger.execute(url, payload, webhook_type, secret: 'mysecret', delivery_id: 'abc-123') end end @@ -286,14 +260,7 @@ describe Webhooks::Trigger do it 'does not update message status if webhook fails for other events' do payload = { event: 'conversation_created', conversation: { id: conversation.id }, id: message.id } - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: webhook_timeout - ).and_raise(RestClient::ExceptionWithResponse.new('error', 500)).once + expect(SafeFetch).to receive(:fetch).and_raise(SafeFetch::HttpError.new('500 Internal Server Error')) expect { trigger.execute(url, payload, webhook_type) }.not_to(change { message.reload.status }) end @@ -302,16 +269,15 @@ describe Webhooks::Trigger do let(:webhook_timeout) { nil } it 'falls back to default timeout' do - payload = { hello: :hello } - - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: default_timeout - ).once + expect(SafeFetch).to receive(:fetch).with( + url, + method: :post, + body: payload.to_json, + headers: base_headers, + open_timeout: default_timeout, + read_timeout: default_timeout, + validate_content_type: false + ).and_yield(fetch_result) trigger.execute(url, payload, webhook_type) end @@ -321,16 +287,15 @@ describe Webhooks::Trigger do let(:webhook_timeout) { -1 } it 'falls back to default timeout' do - payload = { hello: :hello } - - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: default_timeout - ).once + expect(SafeFetch).to receive(:fetch).with( + url, + method: :post, + body: payload.to_json, + headers: base_headers, + open_timeout: default_timeout, + read_timeout: default_timeout, + validate_content_type: false + ).and_yield(fetch_result) trigger.execute(url, payload, webhook_type) end From b0aa844a3266604c589dff0943399a8ad31bf656 Mon Sep 17 00:00:00 2001 From: ramalau <71857041+ramalau0@users.noreply.github.com> Date: Mon, 27 Apr 2026 21:44:51 +0200 Subject: [PATCH 024/201] fix(portals): handle integer blob_id in process_attached_logo without 500 (#14274) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Updating portal settings (name, header text, page title, homepage link) on a portal that already has a logo attached returns 500. The error is \`NoMethodError: undefined method 'valid_encoding?' for an instance of Integer\`. The fix is a two-character change in \`process_attached_logo\`. Closes #13300 ## Root cause \`ActiveStorage::Blob.find_signed\` expects a signed ID string (e.g. \`"eyJfcmFpbH..."\`). Internally it calls \`valid_encoding?\` on the argument to validate the signature payload — a method that exists on \`String\` but not \`Integer\`. When a portal already has a logo, the frontend includes the blob's raw database integer ID (e.g. \`blob_id: 170\`) in the update request payload. The controller passes this integer directly to \`find_signed\`, which immediately raises \`NoMethodError\` before any database query is made. \`\`\`ruby # before, crashes when blob_id is an Integer blob_id = params[:blob_id] blob = ActiveStorage::Blob.find_signed(blob_id) # NoMethodError here @portal.logo.attach(blob) \`\`\` ## What changed \`\`\`ruby # after, safe for any input type blob = ActiveStorage::Blob.find_signed(params[:blob_id].to_s) @portal.logo.attach(blob) if blob \`\`\` \`.to_s\` on an Integer produces a plain decimal string (\`"170"\`), which is not a valid signed ID. \`find_signed\` returns \`nil\` for any invalid signature rather than raising, so the nil guard prevents a broken \`attach\` call. The existing logo remains attached and the settings update succeeds. ## Trade-offs considered | Option | Decision | |---|---| | \`find(blob_id)\` when input is an Integer | Bypasses signature verification — any authenticated user knowing a blob ID could attach arbitrary files to a portal. Security risk. Rejected. | | Raise a 422 for non-string blob_id | Overly strict — the frontend sending an integer is pre-existing behaviour this PR shouldn't break. | | Silently no-op for invalid blob_id (chosen) | Correct product behaviour: if no valid signed upload is provided, leave the logo unchanged. The settings update still succeeds. | ## Known limitation The correct long-term fix is also on the frontend: it should only send \`blob_id\` when attaching a **new** upload (using the signed ID from the direct-upload flow), not when re-submitting the existing logo's raw database integer ID. This PR makes the server robust against the current frontend behaviour without requiring a coordinated frontend change. ## How to reproduce 1. Create a Help Center portal and upload a logo 2. Update any text field via \`PUT /api/v1/accounts/:id/portals/:slug\` while including \`blob_id: \` in the payload 3. Observe 500 with \`NoMethodError: undefined method 'valid_encoding?' for an instance of Integer\` After this fix, the request returns 200, settings are updated, and the existing logo is preserved. Co-authored-by: Ramalau Debeila --- .../api/v1/accounts/portals_controller.rb | 5 ++--- .../api/v1/accounts/portals_controller_spec.rb | 12 ++++++++++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/app/controllers/api/v1/accounts/portals_controller.rb b/app/controllers/api/v1/accounts/portals_controller.rb index 972b244fa..ade83d8ec 100644 --- a/app/controllers/api/v1/accounts/portals_controller.rb +++ b/app/controllers/api/v1/accounts/portals_controller.rb @@ -61,9 +61,8 @@ class Api::V1::Accounts::PortalsController < Api::V1::Accounts::BaseController end def process_attached_logo - blob_id = params[:blob_id] - blob = ActiveStorage::Blob.find_signed(blob_id) - @portal.logo.attach(blob) + blob = ActiveStorage::Blob.find_signed(params[:blob_id].to_s) + @portal.logo.attach(blob) if blob end private diff --git a/spec/controllers/api/v1/accounts/portals_controller_spec.rb b/spec/controllers/api/v1/accounts/portals_controller_spec.rb index 19bc795c3..9c780c00a 100644 --- a/spec/controllers/api/v1/accounts/portals_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/portals_controller_spec.rb @@ -180,6 +180,18 @@ RSpec.describe 'Api::V1::Accounts::Portals', type: :request do expect(portal.archived).to be_truthy end + it 'does not raise when blob_id is an integer (existing logo re-sent by frontend)' do + portal.logo.attach(io: Rails.root.join('spec/assets/avatar.png').open, filename: 'avatar.png', content_type: 'image/png') + + put "/api/v1/accounts/#{account.id}/portals/#{portal.slug}", + params: { portal: { name: 'updated_name' }, blob_id: portal.logo.blob.id }, + headers: admin.create_new_auth_token + + expect(response).to have_http_status(:success) + expect(response.parsed_body['name']).to eq('updated_name') + expect(portal.reload.logo).to be_attached + end + it 'clears associated web widget when inbox selection is blank' do web_widget_inbox = create(:inbox, account: account) portal.update!(channel_web_widget: web_widget_inbox.channel) From 51eb626b889e49a5210735ce5f771c804081ce99 Mon Sep 17 00:00:00 2001 From: Tanmay Deep Sharma <32020192+tds-1@users.noreply.github.com> Date: Tue, 28 Apr 2026 10:09:41 +0700 Subject: [PATCH 025/201] feat: allow disabling 2FA with a backup code (#14102) ## Linear Ticket - https://linear.app/chatwoot/issue/CW-6883/allow-disabling-2fa-using-a-backup-code ## Description When a user loses access to their authenticator app, they can now disable 2FA using one of their saved backup codes (in addition to their password), so they can re-enroll a new authenticator. The disable dialog includes a toggle to switch between entering a verification code and a backup code. ## Type of change - [x] Bug fix (non-breaking change which fixes an issue) ## How Has This Been Tested? - Via UI flows Screenshot 2026-04-20 at 2 17 21 PM Screenshot 2026-04-20 at 2 17 36 PM ## Checklist: - [ ] My code follows the style guidelines of this project - [ ] I have performed a self-review of my code - [ ] I have commented on my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [ ] Any dependent changes have been merged and published in downstream modules --- .../api/v1/profile/mfa_controller.rb | 7 ++-- app/javascript/dashboard/api/mfa.js | 4 +-- .../dashboard/i18n/locale/en/mfa.json | 6 +++- .../settings/profile/MfaManagementActions.vue | 33 ++++++++++++++++++- .../settings/profile/MfaSettings.vue | 4 +-- .../api/v1/profile/mfa_controller_spec.rb | 17 ++++++++++ 6 files changed, 62 insertions(+), 9 deletions(-) diff --git a/app/controllers/api/v1/profile/mfa_controller.rb b/app/controllers/api/v1/profile/mfa_controller.rb index dd874f222..8480b64fb 100644 --- a/app/controllers/api/v1/profile/mfa_controller.rb +++ b/app/controllers/api/v1/profile/mfa_controller.rb @@ -2,8 +2,8 @@ class Api::V1::Profile::MfaController < Api::BaseController before_action :check_mfa_feature_available before_action :check_mfa_enabled, only: [:destroy, :backup_codes] before_action :check_mfa_disabled, only: [:create, :verify] - before_action :validate_otp, only: [:verify, :backup_codes, :destroy] before_action :validate_password, only: [:destroy] + before_action :validate_otp, only: [:verify, :backup_codes, :destroy] def show; end @@ -48,7 +48,8 @@ class Api::V1::Profile::MfaController < Api::BaseController def validate_otp authenticated = Mfa::AuthenticationService.new( user: current_user, - otp_code: mfa_params[:otp_code] + otp_code: mfa_params[:otp_code], + backup_code: mfa_params[:backup_code] ).authenticate return if authenticated @@ -63,6 +64,6 @@ class Api::V1::Profile::MfaController < Api::BaseController end def mfa_params - params.permit(:otp_code, :password) + params.permit(:otp_code, :backup_code, :password) end end diff --git a/app/javascript/dashboard/api/mfa.js b/app/javascript/dashboard/api/mfa.js index c18bea3e9..38cb93810 100644 --- a/app/javascript/dashboard/api/mfa.js +++ b/app/javascript/dashboard/api/mfa.js @@ -14,9 +14,9 @@ class MfaAPI extends ApiClient { return axios.post(`${this.url}/verify`, { otp_code: otpCode }); } - disable(password, otpCode) { + disable(password, { otpCode, backupCode } = {}) { return axios.delete(this.url, { - data: { password, otp_code: otpCode }, + data: { password, otp_code: otpCode, backup_code: backupCode }, }); } diff --git a/app/javascript/dashboard/i18n/locale/en/mfa.json b/app/javascript/dashboard/i18n/locale/en/mfa.json index b03917bcd..8e356aad4 100644 --- a/app/javascript/dashboard/i18n/locale/en/mfa.json +++ b/app/javascript/dashboard/i18n/locale/en/mfa.json @@ -51,10 +51,14 @@ }, "DISABLE": { "TITLE": "Disable Two-Factor Authentication", - "DESCRIPTION": "You'll need to enter your password and a verification code to disable two-factor authentication.", + "DESCRIPTION": "You'll need to enter your password and either a verification code from your authenticator app or a backup code to disable two-factor authentication.", "PASSWORD": "Password", "OTP_CODE": "Verification Code", "OTP_CODE_PLACEHOLDER": "000000", + "BACKUP_CODE": "Backup Code", + "BACKUP_CODE_PLACEHOLDER": "Enter one of your backup codes", + "USE_BACKUP_CODE": "Lost access to your authenticator? Use a backup code instead", + "USE_OTP_CODE": "Use a verification code from your authenticator app", "CONFIRM": "Disable 2FA", "CANCEL": "Cancel", "SUCCESS": "Two-factor authentication has been disabled", diff --git a/app/javascript/dashboard/routes/dashboard/settings/profile/MfaManagementActions.vue b/app/javascript/dashboard/routes/dashboard/settings/profile/MfaManagementActions.vue index caf9e2a6a..b49bae086 100644 --- a/app/javascript/dashboard/routes/dashboard/settings/profile/MfaManagementActions.vue +++ b/app/javascript/dashboard/routes/dashboard/settings/profile/MfaManagementActions.vue @@ -31,6 +31,8 @@ const backupCodesDialogRef = ref(null); // Form values const disablePassword = ref(''); const disableOtpCode = ref(''); +const disableBackupCode = ref(''); +const useBackupCodeToDisable = ref(false); const regenerateOtpCode = ref(''); // Utility functions @@ -54,10 +56,17 @@ const downloadBackupCodes = () => { const handleDisableMfa = async () => { emit('disableMfa', { password: disablePassword.value, - otpCode: disableOtpCode.value, + otpCode: useBackupCodeToDisable.value ? '' : disableOtpCode.value, + backupCode: useBackupCodeToDisable.value ? disableBackupCode.value : '', }); }; +const toggleDisableMethod = () => { + useBackupCodeToDisable.value = !useBackupCodeToDisable.value; + disableOtpCode.value = ''; + disableBackupCode.value = ''; +}; + const handleRegenerateBackupCodes = async () => { emit('regenerateBackupCodes', { otpCode: regenerateOtpCode.value, @@ -68,6 +77,8 @@ const handleRegenerateBackupCodes = async () => { const resetDisableForm = () => { disablePassword.value = ''; disableOtpCode.value = ''; + disableBackupCode.value = ''; + useBackupCodeToDisable.value = false; disableDialogRef.value?.close(); }; @@ -157,12 +168,32 @@ defineExpose({ :label="$t('MFA_SETTINGS.DISABLE.PASSWORD')" /> + +