Merge branch 'sh-loosen-multiline-block' into 'master'

Allow encoded newlines in HTTP URLs

See merge request gitlab-org/gitlab!72655

lib/gitlab/url_blocker.rb

@@ -164,15 +164,21 @@ module Gitlab
       end
 
       def parse_url(url)
-        raise Addressable::URI::InvalidURIError if multiline?(url)
-
-        Addressable::URI.parse(url)
+        Addressable::URI.parse(url).tap do |parsed_url|
+          raise Addressable::URI::InvalidURIError if multiline_blocked?(parsed_url)
+        end
       rescue Addressable::URI::InvalidURIError, URI::InvalidURIError
         raise BlockedUrlError, 'URI is invalid'
       end
 
-      def multiline?(url)
-        CGI.unescape(url.to_s) =~ /\n|\r/
+      def multiline_blocked?(parsed_url)
+        url = parsed_url.to_s
+
+        return true if url =~ /\n|\r/
+        # Google Cloud Storage uses a multi-line, encoded Signature query string
+        return false if %w(http https).include?(parsed_url.scheme&.downcase)
+
+        CGI.unescape(url) =~ /\n|\r/
       end
 
       def validate_port(port, ports)

spec/models/project_spec.rb

@@ -424,8 +424,9 @@ RSpec.describe Project, factory_default: :keep do
       end
 
       include_context 'invalid urls'
+      include_context 'valid urls with CRLF'
 
-      it 'does not allow urls with CR or LF characters' do
+      it 'does not allow URLs with unencoded CR or LF characters' do
         project = build(:project)
 
         aggregate_failures do
@@ -437,6 +438,19 @@ RSpec.describe Project, factory_default: :keep do
           end
         end
       end
+
+      it 'allow URLs with CR or LF characters' do
+        project = build(:project)
+
+        aggregate_failures do
+          valid_urls_with_CRLF.each do |url|
+            project.import_url = url
+
+            expect(project).to be_valid
+            expect(project.errors).to be_empty
+          end
+        end
+      end
     end
 
     describe 'project pending deletion' do

spec/support/shared_contexts/url_shared_context.rb

 # frozen_string_literal: true
 
+RSpec.shared_context 'valid urls with CRLF' do
+  let(:valid_urls_with_CRLF) do
+    [
+      "http://example.com/pa%0dth",
+      "http://example.com/pa%0ath",
+      "http://example.com/pa%0d%0th",
+      "http://example.com/pa%0D%0Ath",
+      "http://gitlab.com/path?param=foo%0Abar",
+      "https://gitlab.com/path?param=foo%0Dbar",
+      "http://example.org:1024/path?param=foo%0D%0Abar",
+      "https://storage.googleapis.com/bucket/import_export_upload/import_file/57265/express.tar.gz?GoogleAccessId=hello@example.org&Signature=ABCD%0AEFGHik&Expires=1634663304"
+    ]
+  end
+end
+
 RSpec.shared_context 'invalid urls' do
   let(:urls_with_CRLF) do
-    ["http://127.0.0.1:333/pa\rth",
-     "http://127.0.0.1:333/pa\nth",
-     "http://127.0a.0.1:333/pa\r\nth",
-     "http://127.0.0.1:333/path?param=foo\r\nbar",
-     "http://127.0.0.1:333/path?param=foo\rbar",
-     "http://127.0.0.1:333/path?param=foo\nbar",
-     "http://127.0.0.1:333/pa%0dth",
-     "http://127.0.0.1:333/pa%0ath",
-     "http://127.0a.0.1:333/pa%0d%0th",
-     "http://127.0.0.1:333/pa%0D%0Ath",
-     "http://127.0.0.1:333/path?param=foo%0Abar",
-     "http://127.0.0.1:333/path?param=foo%0Dbar",
-     "http://127.0.0.1:333/path?param=foo%0D%0Abar"]
+    [
+      "git://example.com/pa%0dth",
+      "git://example.com/pa%0ath",
+      "git://example.com/pa%0d%0th",
+      "http://example.com/pa\rth",
+      "http://example.com/pa\nth",
+      "http://example.com/pa\r\nth",
+      "http://example.com/path?param=foo\r\nbar",
+      "http://example.com/path?param=foo\rbar",
+      "http://example.com/path?param=foo\nbar"
+    ]
   end
 end

spec/validators/addressable_url_validator_spec.rb

@@ -14,6 +14,7 @@ RSpec.describe AddressableUrlValidator do
 
   describe 'validations' do
     include_context 'invalid urls'
+    include_context 'valid urls with CRLF'
 
     let(:validator) { described_class.new(attributes: [:link_url]) }
 
@@ -27,9 +28,20 @@ RSpec.describe AddressableUrlValidator do
       expect(badge.errors.added?(:link_url, validator.options.fetch(:message))).to be true
     end
 
+    it 'allows urls with encoded CR or LF characters' do
+      aggregate_failures do
+        valid_urls_with_CRLF.each do |url|
+          validator.validate_each(badge, :link_url, url)
+
+          expect(badge.errors).to be_empty
+        end
+      end
+    end
+
     it 'does not allow urls with CR or LF characters' do
       aggregate_failures do
         urls_with_CRLF.each do |url|
+          badge = build(:badge, link_url: 'http://www.example.com')
           validator.validate_each(badge, :link_url, url)
 
           expect(badge.errors.added?(:link_url, 'is blocked: URI is invalid')).to be true