Make design filename patter stricter

The pattern which detects possible design filenames should
be stricter to make sure that we don't match any special characters
or quoted strings which could lead to XSS.

Change: security

app/models/design_management/design.rb

@@ -169,7 +169,7 @@ module DesignManagement
       @link_reference_pattern ||= begin
         path_segment = %r{issues/#{Gitlab::Regex.issue}/designs}
         ext = Regexp.new(Regexp.union(SAFE_IMAGE_EXT + DANGEROUS_IMAGE_EXT).source, Regexp::IGNORECASE)
-        valid_char = %r{[^/\s]} # any char that is not a forward slash or whitespace
+        valid_char = %r{[[:word:]\.\-\+]}
         filename_pattern = %r{
           (?<url_filename> #{valid_char}+ \. #{ext})
         }x

lib/gitlab/markdown_cache.rb

@@ -11,7 +11,7 @@ module Gitlab
     # this if the change to the renderer output is a new feature or a
     # minor bug fix.
     # See: https://gitlab.com/gitlab-org/gitlab/-/issues/330313
-    CACHE_COMMONMARK_VERSION        = 28
+    CACHE_COMMONMARK_VERSION        = 29
     CACHE_COMMONMARK_VERSION_START  = 10
 
     BaseError = Class.new(StandardError)

spec/lib/banzai/filter/references/design_reference_filter_spec.rb

@@ -90,11 +90,8 @@ RSpec.describe Banzai::Filter::References::DesignReferenceFilter do
       [
         ['simple.png'],
         ['SIMPLE.PNG'],
-        ['has spaces.png'],
         ['has-hyphen.jpg'],
-        ['snake_case.svg'],
-        ['has "quotes".svg'],
-        ['has <special> characters [o].svg']
+        ['snake_case.svg']
       ]
     end
 
@@ -138,40 +135,25 @@ RSpec.describe Banzai::Filter::References::DesignReferenceFilter do
     end
   end
 
-  context 'a design with a quoted filename' do
-    let(:filename) { %q{A "very" good file.png} }
-    let(:design) { create(:design, :with_versions, issue: issue, filename: filename) }
-
-    it 'links to the design' do
-      expect(doc.css('a').first.attr('href'))
-        .to eq url_for_design(design)
-    end
-  end
-
   context 'internal reference' do
     it_behaves_like 'a reference containing an element node'
 
-    context 'the reference is valid' do
-      it_behaves_like 'a good link reference'
+    it_behaves_like 'a good link reference'
 
-      context 'the filename needs to be escaped' do
-        where(:filename) do
-          [
-            ['with some spaces.png'],
-            ['with <script>console.log("pwded")<%2Fscript>.png']
-          ]
-        end
+    context 'the filename contains invalid characters' do
+      where(:filename) do
+        [
+          ['with some spaces.png'],
+          ['with <script>console.log("pwded")<%2Fscript>.png'],
+          ['foo"bar.png'],
+          ['A "very" good file.png']
+        ]
+      end
 
-        with_them do
-          let(:design) { create(:design, :with_versions, filename: filename, issue: issue) }
-          let(:link) { doc.css('a').first }
+      with_them do
+        let(:design) { create(:design, :with_versions, filename: filename, issue: issue) }
 
-          it 'replaces the content with the reference, but keeps the link', :aggregate_failures do
-            expect(doc.text).to eq(CGI.unescapeHTML("Added #{design.to_reference}"))
-            expect(link.attr('title')).to eq(design.filename)
-            expect(link.attr('href')).to eq(design_url)
-          end
-        end
+        it_behaves_like 'a no-op filter'
       end
     end
 

spec/models/design_management/design_spec.rb

@@ -572,6 +572,12 @@ RSpec.describe DesignManagement::Design do
       expect(described_class.link_reference_pattern).not_to match(url_for_designs(issue))
     end
 
+    it 'intentionally ignores filenames with any special character' do
+      design = build(:design, issue: issue, filename: '"invalid')
+
+      expect(described_class.link_reference_pattern).not_to match(url_for_design(design))
+    end
+
     where(:ext) do
       (described_class::SAFE_IMAGE_EXT + described_class::DANGEROUS_IMAGE_EXT).flat_map do |ext|
         [[ext], [ext.upcase]]
@@ -593,14 +599,6 @@ RSpec.describe DesignManagement::Design do
         )
       end
 
-      context 'the file needs to be encoded' do
-        let(:filename) { "my file.#{ext}" }
-
-        it 'extracts the encoded filename' do
-          expect(captures).to include('url_filename' => 'my%20file.' + ext)
-        end
-      end
-
       context 'the file is all upper case' do
         let(:filename) { "file.#{ext}".upcase }