CVE-2022-0527: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting') (CWE-79)
value: Cross-site Scripting (XSS) - Stored in GitHub repository chatwoot/chatwoot prior to 2.2.0.
CWE description: The product does not neutralize or incorrectly neutralizes user-controllable input before it is placed in output that is used as a web page that is served to other users.
CVSS v3 base score: 6.1
Published: 2022-02-09T05:15Z
Repository: https://github.com/chatwoot/chatwoot
Language: Ruby
Files changed: app/controllers/api/v1/accounts/campaigns_controller.rb, app/models/campaign.rb, spec/builders/campaigns/campaign_conversation_builder_spec.rb, spec/controllers/api/v1/accounts/campaigns_controller_spec.rb, spec/controllers/api/v1/accounts/inboxes_controller_spec.rb, spec/controllers/api/v1/widget/campaigns_controller_spec.rb, spec/listeners/campaign_listener_spec.rb, spec/models/campaign_spec.rb
Commit date: 2022-02-04 14:38:18 +0530
Commit message: fix: Ongoing campaign URL validation (#3890)


Diff: diff --git a/app/controllers/api/v1/accounts/campaigns_controller.rb b/app/controllers/api/v1/accounts/campaigns_controller.rb
index 11b563d81..18d0998c8 100644
--- a/app/controllers/api/v1/accounts/campaigns_controller.rb
+++ b/app/controllers/api/v1/accounts/campaigns_controller.rb
@@ -16,11 +16,11 @@ class Api::V1::Accounts::CampaignsController < Api::V1::Accounts::BaseController
   end
 
   def show; end
 
   def update
-    @campaign.update(campaign_params)
+    @campaign.update!(campaign_params)
   end
 
   private
 
   def campaign
diff --git a/app/models/campaign.rb b/app/models/campaign.rb
index 0093342d6..304d270e7 100644
--- a/app/models/campaign.rb
+++ b/app/models/campaign.rb
@@ -31,16 +31,18 @@
 # Foreign Keys
 #
 #  fk_rails_...  (account_id => accounts.id) ON DELETE => cascade
 #  fk_rails_...  (inbox_id => inboxes.id) ON DELETE => cascade
 #
+require 'uri'
 class Campaign < ApplicationRecord
   validates :account_id, presence: true
   validates :inbox_id, presence: true
   validates :title, presence: true
   validates :message, presence: true
   validate :validate_campaign_inbox
+  validate :validate_url
   validate :prevent_completed_campaign_from_update, on: :update
   belongs_to :account
   belongs_to :inbox
   belongs_to :sender, class_name: 'User', optional: true
 
@@ -84,10 +86,25 @@ class Campaign < ApplicationRecord
       self.campaign_type = 'ongoing'
       self.scheduled_at = nil
     end
   end
 
+  def validate_url
+    return unless trigger_rules['url']
+
+    errors.add(:url, 'invalid') if inbox.inbox_type == 'Website' && !url_valid?(trigger_rules['url'])
+  end
+
+  def url_valid?(url)
+    url = begin
+      URI.parse(url)
+    rescue StandardError
+      false
+    end
+    url.is_a?(URI::HTTP) || url.is_a?(URI::HTTPS)
+  end
+
   def prevent_completed_campaign_from_update
     errors.add :status, 'The campaign is already completed' if !campaign_status_changed? && completed?
   end
 
   # creating db triggers
diff --git a/spec/builders/campaigns/campaign_conversation_builder_spec.rb b/spec/builders/campaigns/campaign_conversation_builder_spec.rb
index 076aeab67..3735c0e66 100644
--- a/spec/builders/campaigns/campaign_conversation_builder_spec.rb
+++ b/spec/builders/campaigns/campaign_conversation_builder_spec.rb
@@ -3,11 +3,11 @@ require 'rails_helper'
 describe ::Campaigns::CampaignConversationBuilder do
   let(:account) { create(:account) }
   let(:inbox) { create(:inbox, account: account) }
   let(:contact) { create(:contact, account: account, identifier: '123') }
   let(:contact_inbox) { create(:contact_inbox, contact: contact, inbox: inbox) }
-  let(:campaign) { create(:campaign, inbox: inbox, account: account) }
+  let(:campaign) { create(:campaign, inbox: inbox, account: account, trigger_rules: { url: 'https://test.com' }) }
 
   describe '#perform' do
     it 'creates a conversation with campaign id and message with campaign message' do
       campaign_conversation = described_class.new(
         contact_inbox_id: contact_inbox.id,
diff --git a/spec/controllers/api/v1/accounts/campaigns_controller_spec.rb b/spec/controllers/api/v1/accounts/campaigns_controller_spec.rb
index 4a9cfece0..9f2175007 100644
--- a/spec/controllers/api/v1/accounts/campaigns_controller_spec.rb
+++ b/spec/controllers/api/v1/accounts/campaigns_controller_spec.rb
@@ -13,11 +13,11 @@ RSpec.describe 'Campaigns API', type: :request do
     end
 
     context 'when it is an authenticated user' do
       let(:agent) { create(:user, account: account, role: :agent) }
       let(:administrator) { create(:user, account: account, role: :administrator) }
-      let!(:campaign) { create(:campaign, account: account) }
+      let!(:campaign) { create(:campaign, account: account, trigger_rules: { url: 'https://test.com' }) }
 
       it 'returns unauthorized for agents' do
         get "/api/v1/accounts/#{account.id}/campaigns",
             headers: agent.create_new_auth_token,
             as: :json
@@ -36,11 +36,11 @@ RSpec.describe 'Campaigns API', type: :request do
       end
     end
   end
 
   describe 'GET /api/v1/accounts/{account.id}/campaigns/:id' do
-    let(:campaign) { create(:campaign, account: account) }
+    let(:campaign) { create(:campaign, account: account, trigger_rules: { url: 'https://test.com' }) }
 
     context 'when it is an unauthenticated user' do
       it 'returns unauthorized' do
         get "/api/v1/accounts/#{account.id}/campaigns/#{campaign.display_id}"
 
@@ -105,10 +105,29 @@ RSpec.describe 'Campaigns API', type: :request do
 
         expect(response).to have_http_status(:success)
         expect(JSON.parse(response.body, symbolize_names: true)[:title]).to eq('test')
       end
 
+      it 'creates a new ongoing campaign' do
+        post "/api/v1/accounts/#{account.id}/campaigns",
+             params: { inbox_id: inbox.id, title: 'test', message: 'test message', trigger_rules: { url: 'https://test.com' } },
+             headers: administrator.create_new_auth_token,
+             as: :json
+
+        expect(response).to have_http_status(:success)
+        expect(JSON.parse(response.body, symbolize_names: true)[:title]).to eq('test')
+      end
+
+      it 'throws error when invalid url provided for ongoing campaign' do
+        post "/api/v1/accounts/#{account.id}/campaigns",
+             params: { inbox_id: inbox.id, title: 'test', message: 'test message', trigger_rules: { url: 'javascript' } },
+             headers: administrator.create_new_auth_token,
+             as: :json
+
+        expect(response).to have_http_status(:unprocessable_entity)
+      end
+
       it 'creates a new oneoff campaign' do
         twilio_sms = create(:channel_twilio_sms, account: account)
         twilio_inbox = create(:inbox, channel: twilio_sms)
         label1 = create(:label, account: account)
         label2 = create(:label, account: account)
@@ -131,11 +150,11 @@ RSpec.describe 'Campaigns API', type: :request do
     end
   end
 
   describe 'PATCH /api/v1/accounts/{account.id}/campaigns/:id' do
     let(:inbox) { create(:inbox, account: account) }
-    let!(:campaign) { create(:campaign, account: account) }
+    let!(:campaign) { create(:campaign, account: account, trigger_rules: { url: 'https://test.com' }) }
 
     context 'when it is an unauthenticated user' do
       it 'returns unauthorized' do
         patch "/api/v1/accounts/#{account.id}/campaigns/#{campaign.display_id}",
               params: { inbox_id: inbox.id, title: 'test', message: 'test message' },
@@ -170,11 +189,11 @@ RSpec.describe 'Campaigns API', type: :request do
     end
   end
 
   describe 'DELETE /api/v1/accounts/{account.id}/campaigns/:id' do
     let(:inbox) { create(:inbox, account: account) }
-    let!(:campaign) { create(:campaign, account: account) }
+    let!(:campaign) { create(:campaign, account: account, trigger_rules: { url: 'https://test.com' }) }
 
     context 'when it is an unauthenticated user' do
       it 'returns unauthorized' do
         delete "/api/v1/accounts/#{account.id}/campaigns/#{campaign.display_id}",
                as: :json
diff --git a/spec/controllers/api/v1/accounts/inboxes_controller_spec.rb b/spec/controllers/api/v1/accounts/inboxes_controller_spec.rb
index 8ccf667ac..5dc184b8c 100644
--- a/spec/controllers/api/v1/accounts/inboxes_controller_spec.rb
+++ b/spec/controllers/api/v1/accounts/inboxes_controller_spec.rb
@@ -131,11 +131,11 @@ RSpec.describe 'Inboxes API', type: :request do
 
     context 'when it is an authenticated user' do
       let(:agent) { create(:user, account: account, role: :agent) }
       let(:administrator) { create(:user, account: account, role: :administrator) }
 
-      let!(:campaign) { create(:campaign, account: account, inbox: inbox) }
+      let!(:campaign) { create(:campaign, account: account, inbox: inbox, trigger_rules: { url: 'https://test.com' }) }
 
       it 'returns unauthorized for agents' do
         get "/api/v1/accounts/#{account.id}/inboxes/#{inbox.id}/campaigns",
             headers: agent.create_new_auth_token,
             as: :json
@@ -143,11 +143,11 @@ RSpec.describe 'Inboxes API', type: :request do
         expect(response).to have_http_status(:unauthorized)
       end
 
       it 'returns all campaigns belonging to the inbox to administrators' do
         # create a random campaign
-        create(:campaign, account: account)
+        create(:campaign, account: account, trigger_rules: { url: 'https://test.com' })
         get "/api/v1/accounts/#{account.id}/inboxes/#{inbox.id}/campaigns",
             headers: administrator.create_new_auth_token,
             as: :json
 
         expect(response).to have_http_status(:success)
diff --git a/spec/controllers/api/v1/widget/campaigns_controller_spec.rb b/spec/controllers/api/v1/widget/campaigns_controller_spec.rb
index 8a938b667..11248769f 100644
--- a/spec/controllers/api/v1/widget/campaigns_controller_spec.rb
+++ b/spec/controllers/api/v1/widget/campaigns_controller_spec.rb
@@ -1,12 +1,12 @@
 require 'rails_helper'
 
 RSpec.describe '/api/v1/widget/campaigns', type: :request do
   let(:account) { create(:account) }
   let(:web_widget) { create(:channel_widget, account: account) }
-  let!(:campaign_1) { create(:campaign, inbox: web_widget.inbox, enabled: true, account: account) }
-  let!(:campaign_2) { create(:campaign, inbox: web_widget.inbox, enabled: false, account: account) }
+  let!(:campaign_1) { create(:campaign, inbox: web_widget.inbox, enabled: true, account: account, trigger_rules: { url: 'https://test.com' }) }
+  let!(:campaign_2) { create(:campaign, inbox: web_widget.inbox, enabled: false, account: account, trigger_rules: { url: 'https://test.com' }) }
 
   describe 'GET /api/v1/widget/campaigns' do
     let(:params) { { website_token: web_widget.website_token } }
 
     context 'with correct website token' do
diff --git a/spec/listeners/campaign_listener_spec.rb b/spec/listeners/campaign_listener_spec.rb
index bfc5df08f..b9f693055 100644
--- a/spec/listeners/campaign_listener_spec.rb
+++ b/spec/listeners/campaign_listener_spec.rb
@@ -3,11 +3,11 @@ describe CampaignListener do
   let(:listener) { described_class.instance }
   let(:account) { create(:account) }
   let(:inbox) { create(:inbox, account: account) }
   let(:contact) { create(:contact, account: account, identifier: '123') }
   let(:contact_inbox) { create(:contact_inbox, contact: contact, inbox: inbox) }
-  let(:campaign) { create(:campaign, inbox: inbox, account: account) }
+  let(:campaign) { create(:campaign, inbox: inbox, account: account, trigger_rules: { url: 'https://test.com' }) }
 
   let!(:event) do
     Events::Base.new('campaign_triggered', Time.zone.now,
                      contact_inbox: contact_inbox, event_info: { campaign_id: campaign.display_id })
   end
diff --git a/spec/models/campaign_spec.rb b/spec/models/campaign_spec.rb
index 54936ac2b..95cac1caf 100644
--- a/spec/models/campaign_spec.rb
+++ b/spec/models/campaign_spec.rb
@@ -7,11 +7,14 @@ RSpec.describe Campaign, type: :model do
     it { is_expected.to belong_to(:account) }
     it { is_expected.to belong_to(:inbox) }
   end
 
   describe '.before_create' do
-    let(:campaign) { build(:campaign, display_id: nil) }
+    let(:account) { create(:account) }
+    let(:website_channel) { create(:channel_widget, account: account) }
+    let(:website_inbox) { create(:inbox, channel: website_channel, account: account) }
+    let(:campaign) { build(:campaign, inbox: website_inbox, display_id: nil, trigger_rules: { url: 'https://test.com' }) }
 
     before do
       campaign.save
       campaign.reload
     end
@@ -35,11 +38,13 @@ RSpec.describe Campaign, type: :model do
       expect(campaign.errors.full_messages.first).to eq 'Inbox Unsupported Inbox type'
     end
   end
 
   context 'when a campaign is completed' do
-    let!(:campaign) { create(:campaign, campaign_status: :completed) }
+    let(:account) { create(:account) }
+    let(:web_widget) { create(:channel_widget, account: account) }
+    let!(:campaign) { create(:campaign, inbox: web_widget.inbox, campaign_status: :completed, trigger_rules: { url: 'https://test.com' }) }
 
     it 'would prevent further updates' do
       campaign.title = 'new name'
       expect(campaign.save).to eq false
       expect(campaign.errors.full_messages.first).to eq 'Status The campaign is already completed'

Vulnerable code:     @campaign.update(campaign_params)
  let(:campaign) { create(:campaign, inbox: inbox, account: account) }
      let!(:campaign) { create(:campaign, account: account) }
    let(:campaign) { create(:campaign, account: account) }
    let!(:campaign) { create(:campaign, account: account) }
    let!(:campaign) { create(:campaign, account: account) }
      let!(:campaign) { create(:campaign, account: account, inbox: inbox) }
        create(:campaign, account: account)
  let!(:campaign_1) { create(:campaign, inbox: web_widget.inbox, enabled: true, account: account) }
  let!(:campaign_2) { create(:campaign, inbox: web_widget.inbox, enabled: false, account: account) }
  let(:campaign) { create(:campaign, inbox: inbox, account: account) }
    let(:campaign) { build(:campaign, display_id: nil) }
    let!(:campaign) { create(:campaign, campaign_status: :completed) }
Fixed code:     @campaign.update!(campaign_params)
require 'uri'
  validate :validate_url
  def validate_url
    return unless trigger_rules['url']

    errors.add(:url, 'invalid') if inbox.inbox_type == 'Website' && !url_valid?(trigger_rules['url'])
  end

  def url_valid?(url)
    url = begin
      URI.parse(url)
    rescue StandardError
      false
    end
    url.is_a?(URI::HTTP) || url.is_a?(URI::HTTPS)
  end

  let(:campaign) { create(:campaign, inbox: inbox, account: account, trigger_rules: { url: 'https://test.com' }) }
      let!(:campaign) { create(:campaign, account: account, trigger_rules: { url: 'https://test.com' }) }
    let(:campaign) { create(:campaign, account: account, trigger_rules: { url: 'https://test.com' }) }
      it 'creates a new ongoing campaign' do
        post "/api/v1/accounts/#{account.id}/campaigns",
             params: { inbox_id: inbox.id, title: 'test', message: 'test message', trigger_rules: { url: 'https://test.com' } },
             headers: administrator.create_new_auth_token,
             as: :json

        expect(response).to have_http_status(:success)
        expect(JSON.parse(response.body, symbolize_names: true)[:title]).to eq('test')
      end

      it 'throws error when invalid url provided for ongoing campaign' do
        post "/api/v1/accounts/#{account.id}/campaigns",
             params: { inbox_id: inbox.id, title: 'test', message: 'test message', trigger_rules: { url: 'javascript' } },
             headers: administrator.create_new_auth_token,
             as: :json

        expect(response).to have_http_status(:unprocessable_entity)
      end

    let!(:campaign) { create(:campaign, account: account, trigger_rules: { url: 'https://test.com' }) }
    let!(:campaign) { create(:campaign, account: account, trigger_rules: { url: 'https://test.com' }) }
      let!(:campaign) { create(:campaign, account: account, inbox: inbox, trigger_rules: { url: 'https://test.com' }) }
        create(:campaign, account: account, trigger_rules: { url: 'https://test.com' })
  let!(:campaign_1) { create(:campaign, inbox: web_widget.inbox, enabled: true, account: account, trigger_rules: { url: 'https://test.com' }) }
  let!(:campaign_2) { create(:campaign, inbox: web_widget.inbox, enabled: false, account: account, trigger_rules: { url: 'https://test.com' }) }
  let(:campaign) { create(:campaign, inbox: inbox, account: account, trigger_rules: { url: 'https://test.com' }) }
    let(:account) { create(:account) }
    let(:website_channel) { create(:channel_widget, account: account) }
    let(:website_inbox) { create(:inbox, channel: website_channel, account: account) }
    let(:campaign) { build(:campaign, inbox: website_inbox, display_id: nil, trigger_rules: { url: 'https://test.com' }) }
    let(:account) { create(:account) }
    let(:web_widget) { create(:channel_widget, account: account) }
    let!(:campaign) { create(:campaign, inbox: web_widget.inbox, campaign_status: :completed, trigger_rules: { url: 'https://test.com' }) }