Merge branch 'security-discord-integration-regex' into 'master'

Update Integrations::Discord::ATTACHMENT_REGEX regex

See merge request https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/3953



Merged-by: GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com>
Approved-by: Nikhil George <ngeorge@gitlab.com>
Approved-by: Luke Duncalfe <lduncalfe@gitlab.com>
Reviewed-by: Luke Duncalfe <lduncalfe@gitlab.com>
Reviewed-by: James Nutt <jnutt@gitlab.com>
Co-authored-by: George Koltsov <gkoltsov@gitlab.com>

app/models/integrations/discord.rb

@@ -4,7 +4,7 @@
 
 module Integrations
   class Discord < BaseChatNotification
-    ATTACHMENT_REGEX = /: (?<entry>.*?)\n - (?<name>.*)\n*/
+    ATTACHMENT_REGEX = Gitlab::UntrustedRegexp.new(': (?<entry>[^\n]*)\n - (?<name>[^\n]*)\n*')
 
     field :webhook,
       section: SECTION_TYPE_CONNECTION,
@@ -77,7 +77,14 @@ def notify(message, opts)
       client.execute do |builder|
         builder.add_embed do |embed|
           embed.author = Discordrb::Webhooks::EmbedAuthor.new(name: message.user_name, icon_url: message.user_avatar)
-          embed.description = (message.pretext + "\n" + Array.wrap(message.attachments).join("\n")).gsub(ATTACHMENT_REGEX, " \\k<entry> - \\k<name>\n")
+          embed.description = (message.pretext + "\n" + Array.wrap(message.attachments).join("\n"))
+
+          if ATTACHMENT_REGEX.match?(embed.description)
+            embed.description = ATTACHMENT_REGEX.replace_gsub(embed.description) do |match|
+              " #{match[:entry]} - #{match[:name]}\n"
+            end
+          end
+
           embed.colour = embed_color(message)
           embed.timestamp = Time.now.utc
         end

spec/models/integrations/discord_spec.rb

@@ -25,7 +25,7 @@
   describe 'validations' do
     let_it_be(:project) { create(:project) }
 
-    subject { integration }
+    subject(:discord_integration) { integration }
 
     describe 'only allows one channel on events' do
       context 'when given more than one channel' do
@@ -53,8 +53,10 @@
       Gitlab::DataBuilder::Push.build_sample(project, user)
     end
 
+    subject(:discord_integration) { described_class.new }
+
     before do
-      allow(subject).to receive_messages(
+      allow(discord_integration).to receive_messages(
         project: project,
         project_id: project.id,
         webhook: webhook_url
@@ -71,7 +73,7 @@
       end
 
       freeze_time do
-        subject.execute(sample_data)
+        discord_integration.execute(sample_data)
 
         expect(builder.to_json_hash[:embeds].first).to include(
           description: start_with("#{user.name} pushed to branch [master](http://localhost/#{project.namespace.path}/#{project.path}/-/commits/master) of"),
@@ -85,13 +87,53 @@
       end
     end
 
+    context 'when description references attachments' do
+      let(:builder) { Discordrb::Webhooks::Builder.new }
+      let(:attachments) { ": foo\n - bar" }
+
+      before do
+        allow_next_instance_of(Discordrb::Webhooks::Client) do |client|
+          allow(client).to receive(:execute).and_yield(builder)
+        end
+
+        allow_next_instance_of(Integrations::ChatMessage::PushMessage) do |message|
+          allow(message).to receive(:pretext).and_return('pretext')
+          allow(message).to receive(:attachments).and_return(attachments)
+        end
+      end
+
+      it 'updates attachment format' do
+        freeze_time do
+          discord_integration.execute(sample_data)
+
+          expect(builder.to_json_hash[:embeds].first)
+            .to include(description: "pretext\n foo - bar\n")
+        end
+      end
+
+      context 'when description is large' do
+        let(:attachments) { "#{': -' * 20_000}: foo\n - bar" }
+
+        it 'updates attachment format' do
+          Timeout.timeout(1) do
+            freeze_time do
+              discord_integration.execute(sample_data)
+
+              expect(builder.to_json_hash[:embeds].first)
+                .to include(description: "pretext\n#{' -:' * 20_000} foo - bar\n")
+            end
+          end
+        end
+      end
+    end
+
     context 'DNS rebind to local address' do
       before do
         stub_dns(webhook_url, ip_address: '192.168.2.120')
       end
 
       it 'does not allow DNS rebinding' do
-        expect { subject.execute(sample_data) }.to raise_error(ArgumentError, /is blocked/)
+        expect { discord_integration.execute(sample_data) }.to raise_error(ArgumentError, /is blocked/)
       end
     end
 
@@ -101,8 +143,8 @@
       end
 
       it 'logs an error and returns false' do
-        expect(subject).to receive(:log_error).with('400 Bad Request')
-        expect(subject.execute(sample_data)).to be(false)
+        expect(discord_integration).to receive(:log_error).with('400 Bad Request')
+        expect(discord_integration.execute(sample_data)).to be(false)
       end
     end
   end