Merge branch 'kerrizor/cleanup-of-import-url-sanitization-patch' into 'master'

Cleanup of import url sanitization patch

See merge request gitlab-org/gitlab!42539

app/services/projects/update_remote_mirror_service.rb

@@ -2,12 +2,14 @@
 
 module Projects
   class UpdateRemoteMirrorService < BaseService
+    include Gitlab::Utils::StrongMemoize
+
     MAX_TRIES = 3
 
     def execute(remote_mirror, tries)
       return success unless remote_mirror.enabled?
 
-      if Gitlab::UrlBlocker.blocked_url?(CGI.unescape(Gitlab::UrlSanitizer.sanitize(remote_mirror.url)))
+      if Gitlab::UrlBlocker.blocked_url?(normalized_url(remote_mirror.url))
         return error("The remote mirror URL is invalid.")
       end
 
@@ -27,6 +29,12 @@ module Projects
 
     private
 
+    def normalized_url(url)
+      strong_memoize(:normalized_url) do
+        CGI.unescape(Gitlab::UrlSanitizer.sanitize(url))
+      end
+    end
+
     def update_mirror(remote_mirror)
       remote_mirror.update_start!
       remote_mirror.ensure_remote!

ee/app/services/projects/update_mirror_service.rb

@@ -2,11 +2,13 @@
 
 module Projects
   class UpdateMirrorService < BaseService
+    include Gitlab::Utils::StrongMemoize
+
     Error = Class.new(StandardError)
     UpdateError = Class.new(Error)
 
     def execute
-      if project.import_url && Gitlab::UrlBlocker.blocked_url?(CGI.unescape(Gitlab::UrlSanitizer.sanitize(project.import_url)))
+      if project.import_url && Gitlab::UrlBlocker.blocked_url?(normalized_url(project.import_url))
         return error("The import URL is invalid.")
       end
 
@@ -47,6 +49,12 @@ module Projects
 
     private
 
+    def normalized_url(url)
+      strong_memoize(:normalized_url) do
+        CGI.unescape(Gitlab::UrlSanitizer.sanitize(url))
+      end
+    end
+
     def update_branches
       local_branches = repository.branches.each_with_object({}) { |branch, branches| branches[branch.name] = branch }
 

ee/spec/services/projects/update_mirror_service_spec.rb

@@ -75,27 +75,14 @@ RSpec.describe Projects::UpdateMirrorService do
     end
 
     context "when given URLs containing escaped elements" do
-      using RSpec::Parameterized::TableSyntax
+      it_behaves_like "URLs containing escaped elements return expected status" do
+        let(:result) { service.execute }
 
-      where(:url, :result_status) do
-        "https://user:0a%23@test.example.com/project.git"                               | :success
-        "https://git.example.com:1%2F%2F@source.developers.google.com/project.git"      | :success
-        CGI.escape("git://localhost:1234/some-path?some-query=some-val\#@example.com/") | :error
-        CGI.escape(CGI.escape("https://user:0a%23@test.example.com/project.git"))       | :error
-      end
-
-      with_them do
         before do
           allow(project).to receive(:import_url).and_return(url)
 
           stub_fetch_mirror(project)
         end
-
-        it "returns expected status" do
-          result = service.execute
-
-          expect(result[:status]).to eq(result_status)
-        end
       end
     end
 

spec/services/projects/update_remote_mirror_service_spec.rb

@@ -68,25 +68,12 @@ RSpec.describe Projects::UpdateRemoteMirrorService do
     end
 
     context "when given URLs containing escaped elements" do
-      using RSpec::Parameterized::TableSyntax
+      it_behaves_like "URLs containing escaped elements return expected status" do
+        let(:result) { execute! }
 
-      where(:url, :result_status) do
-        "https://user:0a%23@test.example.com/project.git"                               | :success
-        "https://git.example.com:1%2F%2F@source.developers.google.com/project.git"      | :success
-        CGI.escape("git://localhost:1234/some-path?some-query=some-val\#@example.com/") | :error
-        CGI.escape(CGI.escape("https://user:0a%23@test.example.com/project.git"))       | :error
-      end
-
-      with_them do
         before do
           allow(remote_mirror).to receive(:url).and_return(url)
         end
-
-        it "returns expected status" do
-          result = execute!
-
-          expect(result[:status]).to eq(result_status)
-        end
       end
     end
 

spec/support/shared_examples/services/projects/urls_with_escaped_elements_shared_example.rb

+# frozen_string_literal: true
+
+# Shared examples that test requests against URLs with escaped elements
+#
+RSpec.shared_examples "URLs containing escaped elements return expected status" do
+  using RSpec::Parameterized::TableSyntax
+
+  where(:url, :result_status) do
+    "https://user:0a%23@test.example.com/project.git"                               | :success
+    "https://git.example.com:1%2F%2F@source.developers.google.com/project.git"      | :success
+    CGI.escape("git://localhost:1234/some-path?some-query=some-val\#@example.com/") | :error
+    CGI.escape(CGI.escape("https://user:0a%23@test.example.com/project.git"))       | :error
+  end
+
+  with_them do
+    it "returns expected status" do
+      expect(result[:status]).to eq(result_status)
+    end
+  end
+end