From 6baf442c28c94d9c3e7eea6333ea7019170217a0 Mon Sep 17 00:00:00 2001 From: Sony Mathew Date: Fri, 24 Jul 2026 13:59:28 +0530 Subject: [PATCH] fix: enforce email limits for agent invitations (#15082) # Pull Request Template ## Description Prevents Chatwoot Cloud accounts from exceeding their daily non-channel email allowance through agent invitations. New-user invitations atomically reserve email capacity before mail is queued; when the budget is exhausted, agent creation rolls back and returns HTTP 429. This covers single and bulk agent creation. Self-hosted installations remain unaffected, and adding an existing user does not consume capacity when no invitation is sent. Related to [CW-7637](https://linear.app/chatwoot/issue/CW-7637/prevent-agent-invitation-email-abuse-after-july-20-incident). ## Type of change - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality not to work as expected) - [ ] This change requires a documentation update ## How Has This Been Tested? Verified single and bulk creation at an exhausted budget, successful invitation enqueueing below the limit, no capacity usage for existing users, and no enforcement on self-hosted installations. A concurrent Redis probe admitted exactly five of twenty simultaneous reservations against a limit of five. ## 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 --- app/builders/agent_builder.rb | 19 ++++- .../api/v1/accounts/agents_controller.rb | 10 ++- .../concerns/request_exception_handler.rb | 4 +- .../concerns/account_email_rate_limitable.rb | 36 ++++++++- config/locales/en.yml | 1 + lib/custom_exceptions/account.rb | 14 ++++ spec/builders/agent_builder_spec.rb | 31 +++++++ .../api/v1/accounts/agents_controller_spec.rb | 81 +++++++++++++++++++ .../account_email_rate_limitable_spec.rb | 47 +++++++++++ 9 files changed, 237 insertions(+), 6 deletions(-) diff --git a/app/builders/agent_builder.rb b/app/builders/agent_builder.rb index af68eefc5..fd26d523e 100644 --- a/app/builders/agent_builder.rb +++ b/app/builders/agent_builder.rb @@ -25,11 +25,13 @@ class AgentBuilder account.with_lock do raise LimitExceededError unless can_add_agent? - ActiveRecord::Base.transaction do + ActiveRecord::Base.transaction(requires_new: true) do @user = find_or_create_user create_account_user + reserve_invitation_email_capacity if user_needs_confirmation? end end + @user.send_confirmation_instructions if user_needs_confirmation? @user end @@ -42,18 +44,29 @@ class AgentBuilder # Finds a user by email or creates a new one with a temporary password. # @return [User] the found or created user. def find_or_create_user + @new_user = false 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.new(email: email, name: @name, password: temp_password, password_confirmation: temp_password).tap do |new_user| + new_user.skip_confirmation_notification! + new_user.save! + @new_user = true + end end # Checks if the user needs confirmation. # @return [Boolean] true if the user is persisted and not confirmed, false otherwise. def user_needs_confirmation? - @user.persisted? && !@user.confirmed? + @new_user && @user.persisted? && !@user.confirmed? + end + + def reserve_invitation_email_capacity + return if account.reserve_email_send_capacity + + raise CustomExceptions::Account::EmailLimitExceeded.new({}) end # Creates an account user linking the user to the current account. diff --git a/app/controllers/api/v1/accounts/agents_controller.rb b/app/controllers/api/v1/accounts/agents_controller.rb index 864c50bb4..7399b92fd 100644 --- a/app/controllers/api/v1/accounts/agents_controller.rb +++ b/app/controllers/api/v1/accounts/agents_controller.rb @@ -76,11 +76,19 @@ class Api::V1::Accounts::AgentsController < Api::V1::Accounts::BaseController end def bulk_create_agents(emails) + email_limit_error = nil + Current.account.with_lock do raise AgentBuilder::LimitExceededError if emails.count > available_agent_count - emails.each { |email| create_agent_from_email(email) } + emails.each do |email| + create_agent_from_email(email) + rescue CustomExceptions::Account::EmailLimitExceeded => e + email_limit_error = e + end end + + raise email_limit_error if email_limit_error end def create_agent_from_email(email) diff --git a/app/controllers/concerns/request_exception_handler.rb b/app/controllers/concerns/request_exception_handler.rb index 43d6edf1f..b132f7eeb 100644 --- a/app/controllers/concerns/request_exception_handler.rb +++ b/app/controllers/concerns/request_exception_handler.rb @@ -9,7 +9,9 @@ module RequestExceptionHandler included do rescue_from ActiveRecord::RecordInvalid, with: :render_record_invalid - rescue_from CustomExceptions::Inbox::LimitExceeded, with: :render_error_response + rescue_from CustomExceptions::Inbox::LimitExceeded, + CustomExceptions::Account::EmailLimitExceeded, + with: :render_error_response end private diff --git a/app/models/concerns/account_email_rate_limitable.rb b/app/models/concerns/account_email_rate_limitable.rb index 5f69e22ee..6a59df241 100644 --- a/app/models/concerns/account_email_rate_limitable.rb +++ b/app/models/concerns/account_email_rate_limitable.rb @@ -20,7 +20,7 @@ module AccountEmailRateLimitable return true unless ChatwootApp.chatwoot_cloud? return true if emails_sent_today < email_rate_limit - Rails.logger.warn("Account #{id} reached daily email rate limit of #{email_rate_limit}. Sent: #{emails_sent_today}") + log_email_limit_reached false end @@ -30,8 +30,42 @@ module AccountEmailRateLimitable end end + def reserve_email_send_capacity(count = 1) + return true unless ChatwootApp.chatwoot_cloud? + + loop do + reservation = attempt_email_capacity_reservation(count) + if reservation == :limit_exceeded + log_email_limit_reached + return false + end + return true if reservation.present? + end + end + private + def attempt_email_capacity_reservation(count) + Redis::Alfred.with do |redis| + redis.watch(email_count_cache_key) do + current_count = redis.get(email_count_cache_key).to_i + if current_count + count > email_rate_limit + redis.unwatch + next :limit_exceeded + end + + redis.multi do |transaction| + transaction.incrby(email_count_cache_key, count) + transaction.expire(email_count_cache_key, OUTBOUND_EMAIL_TTL) if current_count.zero? + end + end + end + end + + def log_email_limit_reached + Rails.logger.warn("Account #{id} reached daily email rate limit of #{email_rate_limit}. Sent: #{emails_sent_today}") + end + def email_count_cache_key @email_count_cache_key ||= format( Redis::Alfred::ACCOUNT_OUTBOUND_EMAIL_COUNT_KEY, diff --git a/config/locales/en.yml b/config/locales/en.yml index a1f1077be..96c823f6a 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -76,6 +76,7 @@ en: errors: account: not_authorized: You are not authorized to access this account + email_limit_exceeded: The daily email limit for this account has been reached reporting_timezone: invalid: is not a valid timezone support_email: diff --git a/lib/custom_exceptions/account.rb b/lib/custom_exceptions/account.rb index 08c6f0ddb..92d49b282 100644 --- a/lib/custom_exceptions/account.rb +++ b/lib/custom_exceptions/account.rb @@ -42,4 +42,18 @@ module CustomExceptions::Account I18n.t 'errors.plan_upgrade_required.failed' end end + + class EmailLimitExceeded < CustomExceptions::Base + def message + I18n.t('errors.account.email_limit_exceeded') + end + + def to_hash + { error: message } + end + + def http_status + :too_many_requests + end + end end diff --git a/spec/builders/agent_builder_spec.rb b/spec/builders/agent_builder_spec.rb index 69cacb22b..3bde0ed09 100644 --- a/spec/builders/agent_builder_spec.rb +++ b/spec/builders/agent_builder_spec.rb @@ -30,6 +30,8 @@ RSpec.describe AgentBuilder, type: :model do end context 'when user does not exist' do + before { clear_enqueued_jobs } + it 'creates a new user' do expect { agent_builder.perform }.to change(User, :count).by(1) end @@ -41,6 +43,28 @@ RSpec.describe AgentBuilder, type: :model do it 'returns a user' do expect(agent_builder.perform).to be_a(User) end + + it 'reserves email capacity and enqueues the invitation' do + allow(ChatwootApp).to receive(:chatwoot_cloud?).and_return(true) + + expect { agent_builder.perform }.to have_enqueued_mail(Devise::Mailer, :confirmation_instructions) + expect(account.emails_sent_today).to eq(1) + end + + context 'when the account email limit is exhausted' do + before do + allow(ChatwootApp).to receive(:chatwoot_cloud?).and_return(true) + account.update!(limits: { 'emails' => 0 }) + end + + it 'does not create the user or enqueue an invitation' do + expect { agent_builder.perform }.to raise_error(CustomExceptions::Account::EmailLimitExceeded) + expect(User.from_email(email)).to be_nil + expect(AccountUser.find_by(account: account, user: User.from_email(email))).to be_nil + mail_jobs = enqueued_jobs.select { |job| job[:job].to_s == 'ActionMailer::MailDeliveryJob' } + expect(mail_jobs).to be_empty + end + end end context 'when user exists' do @@ -55,6 +79,13 @@ RSpec.describe AgentBuilder, type: :model do it 'creates a new account user' do expect { agent_builder.perform }.to change(AccountUser, :count).by(1) end + + it 'does not consume email capacity or enqueue another invitation' do + clear_enqueued_jobs + + expect { agent_builder.perform }.not_to have_enqueued_mail(Devise::Mailer, :confirmation_instructions) + expect(account.emails_sent_today).to eq(0) + end end context 'when only email is provided' do diff --git a/spec/controllers/api/v1/accounts/agents_controller_spec.rb b/spec/controllers/api/v1/accounts/agents_controller_spec.rb index 46b38677a..328d9ce60 100644 --- a/spec/controllers/api/v1/accounts/agents_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/agents_controller_spec.rb @@ -177,6 +177,22 @@ RSpec.describe 'Agents API', type: :request do expect(response.parsed_body['email']).to eq(params[:email]) expect(account.users.last.name).to eq('NewUser') end + + context 'when the account email limit is exhausted' do + before do + allow(ChatwootApp).to receive(:chatwoot_cloud?).and_return(true) + account.update!(limits: { 'emails' => 0 }) + end + + it 'does not create an agent' do + expect do + post "/api/v1/accounts/#{account.id}/agents", params: params, headers: admin.create_new_auth_token, as: :json + end.not_to change(User, :count) + + expect(response).to have_http_status(:too_many_requests) + expect(response.parsed_body['error']).to eq('The daily email limit for this account has been reached') + end + end end end @@ -211,6 +227,71 @@ RSpec.describe 'Agents API', type: :request do expect(response).to have_http_status(:ok) end + + context 'when the account email limit is exhausted' do + before do + allow(ChatwootApp).to receive(:chatwoot_cloud?).and_return(true) + account.update!(limits: { 'emails' => 0 }) + end + + it 'does not create new agents' do + expect do + post "/api/v1/accounts/#{account.id}/agents/bulk_create", params: bulk_create_params, headers: admin.create_new_auth_token + end.not_to change(User, :count) + + expect(response).to have_http_status(:too_many_requests) + expect(response.parsed_body['error']).to eq('The daily email limit for this account has been reached') + end + + it 'adds existing users and continues processing after a rejected invitation' do + first_existing_user = create(:user, email: 'first-existing@example.com') + second_existing_user = create(:user, email: 'second-existing@example.com') + params = { + emails: [ + first_existing_user.email, + 'rejected-invitation@example.com', + second_existing_user.email + ] + } + + expect do + post "/api/v1/accounts/#{account.id}/agents/bulk_create", params: params, headers: admin.create_new_auth_token + end.not_to change(User, :count) + + expect(response).to have_http_status(:too_many_requests) + expect(account.reload.users).to include(first_existing_user, second_existing_user) + expect(User.from_email('rejected-invitation@example.com')).to be_nil + expect(account.emails_sent_today).to eq(0) + end + + it 'keeps the onboarding step when an invitation is rejected' do + account.update!(custom_attributes: { onboarding_step: 'completed' }) + + post "/api/v1/accounts/#{account.id}/agents/bulk_create", params: bulk_create_params, headers: admin.create_new_auth_token + + expect(response).to have_http_status(:too_many_requests) + expect(account.reload.custom_attributes['onboarding_step']).to eq('completed') + end + end + + context 'when the account has capacity for only part of the batch' do + before do + allow(ChatwootApp).to receive(:chatwoot_cloud?).and_return(true) + account.update!(limits: { 'emails' => 1 }) + end + + it 'persists the successful invitation without leaking email capacity' do + expect do + post "/api/v1/accounts/#{account.id}/agents/bulk_create", params: bulk_create_params, headers: admin.create_new_auth_token + end.to change(User, :count).by(1) + + expect(response).to have_http_status(:too_many_requests) + expect(User.from_email(emails.first)).to be_present + expect(User.from_email(emails.second)).to be_nil + expect(User.from_email(emails.third)).to be_nil + expect(account.emails_sent_today).to eq(1) + end + end end end end diff --git a/spec/models/concerns/account_email_rate_limitable_spec.rb b/spec/models/concerns/account_email_rate_limitable_spec.rb index fb9a86144..acef32a50 100644 --- a/spec/models/concerns/account_email_rate_limitable_spec.rb +++ b/spec/models/concerns/account_email_rate_limitable_spec.rb @@ -83,4 +83,51 @@ RSpec.describe AccountEmailRateLimitable do expect(Redis::Alfred).not_to have_received(:expire) end end + + describe '#reserve_email_send_capacity' do + context 'when chatwoot cloud' do + before do + allow(ChatwootApp).to receive(:chatwoot_cloud?).and_return(true) + account.update!(limits: { 'emails' => 2 }) + end + + it 'atomically reserves capacity without exceeding the limit' do + expect(account.reserve_email_send_capacity).to be true + expect(account.reserve_email_send_capacity).to be true + expect(account.reserve_email_send_capacity).to be false + expect(account.emails_sent_today).to eq(2) + end + + it 'does not partially reserve a batch that exceeds the remaining capacity' do + expect(account.reserve_email_send_capacity(2)).to be true + expect(account.reserve_email_send_capacity(2)).to be false + expect(account.emails_sent_today).to eq(2) + end + + it 'unwatches the counter when capacity is exhausted' do + redis = instance_double(Redis) + key = format(Redis::Alfred::ACCOUNT_OUTBOUND_EMAIL_COUNT_KEY, account_id: account.id, date: Time.zone.today.to_s) + allow(Redis::Alfred).to receive(:with).and_yield(redis) + allow(account).to receive(:emails_sent_today).and_return(2) + allow(redis).to receive(:watch).with(key).and_yield + allow(redis).to receive(:get).with(key).and_return('2') + expect(redis).to receive(:unwatch) + expect(redis).not_to receive(:multi) + + expect(account.reserve_email_send_capacity).to be false + end + end + + context 'when self-hosted' do + before do + allow(ChatwootApp).to receive(:chatwoot_cloud?).and_return(false) + account.update!(limits: { 'emails' => 1 }) + end + + it 'does not reserve or track email capacity' do + expect(account.reserve_email_send_capacity(2)).to be true + expect(account.emails_sent_today).to eq(0) + end + end + end end