Allow newlines in HTTP URLs

We saw in
https://gitlab.com/gitlab-com/gl-infra/production/-/issues/5756 that
restricting newlines in query strings blocks Google Cloud Storage (GCS)
URLs from working since GCS uses a multi-line `Signature` query string.

The original check was introduced to prevent CRLF injection in the Git
protocol (https://gitlab.com/gitlab-org/gitlab/-/issues/8438). Git has
since added protection against newlines in the URL
(https://github.com/git/git/commit/a02ea577174ab8ed18f847cf1693f213e0b9c473),
but they haven't blocked the carriage return (CR) case.

To ensure defense in depth, we continue to block Git requests with CRLF,
but allow multi-line HTTP queries.

Changelog: fixed

lib/gitlab/url_blocker.rb

@@ -164,15 +164,15 @@ 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 git_multiline?(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 git_multiline?(parsed_url)
+        parsed_url.scheme == 'git' && CGI.unescape(parsed_url.to_s) =~ /\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 CR or LF characters with git:// scheme' 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\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",
+      "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=VBJkDX2MV4gi5wcARkvEmSVrNxRgbumVYcH6xw615IbIyT9kDw/+NXmHEqg7\n1mSTRypOjr2IkqaCgHJeyIF4mdOsII/XdgYomdV6zRSqrVmAD0BXg6jfCCCk&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\rth",
+      "git://example.com/pa\nth",
+      "git://example.com/pa\r\nth",
+      "git://example.com/path?param=foo\r\nbar",
+      "git://example.com/path?param=foo\rbar",
+      "git://example.com/path?param=foo\nbar",
+      "git://example.com/pa%0dth",
+      "git://example.com/pa%0ath",
+      "git://127.0a.0.1/pa%0d%0th",
+      "git://example.com/pa%0D%0Ath",
+      "git://gitlab.com/project%0Dpath",
+      "git://gitlab.com/path?param=foo%0Abar"
+    ]
   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,6 +28,16 @@ RSpec.describe AddressableUrlValidator do
       expect(badge.errors.added?(:link_url, validator.options.fetch(:message))).to be true
     end
 
+    it 'allows urls with CR or LF characters in query strings' 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|