Commit e47e5144 authored by Kerri Miller's avatar Kerri Miller Committed by Sean McGivern

Cleanup of import url sanitization patch

parent da0c0f0b
...@@ -2,12 +2,14 @@ ...@@ -2,12 +2,14 @@
module Projects module Projects
class UpdateRemoteMirrorService < BaseService class UpdateRemoteMirrorService < BaseService
include Gitlab::Utils::StrongMemoize
MAX_TRIES = 3 MAX_TRIES = 3
def execute(remote_mirror, tries) def execute(remote_mirror, tries)
return success unless remote_mirror.enabled? 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.") return error("The remote mirror URL is invalid.")
end end
...@@ -27,6 +29,12 @@ module Projects ...@@ -27,6 +29,12 @@ module Projects
private private
def normalized_url(url)
strong_memoize(:normalized_url) do
CGI.unescape(Gitlab::UrlSanitizer.sanitize(url))
end
end
def update_mirror(remote_mirror) def update_mirror(remote_mirror)
remote_mirror.update_start! remote_mirror.update_start!
remote_mirror.ensure_remote! remote_mirror.ensure_remote!
......
...@@ -2,11 +2,13 @@ ...@@ -2,11 +2,13 @@
module Projects module Projects
class UpdateMirrorService < BaseService class UpdateMirrorService < BaseService
include Gitlab::Utils::StrongMemoize
Error = Class.new(StandardError) Error = Class.new(StandardError)
UpdateError = Class.new(Error) UpdateError = Class.new(Error)
def execute 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.") return error("The import URL is invalid.")
end end
...@@ -47,6 +49,12 @@ module Projects ...@@ -47,6 +49,12 @@ module Projects
private private
def normalized_url(url)
strong_memoize(:normalized_url) do
CGI.unescape(Gitlab::UrlSanitizer.sanitize(url))
end
end
def update_branches def update_branches
local_branches = repository.branches.each_with_object({}) { |branch, branches| branches[branch.name] = branch } local_branches = repository.branches.each_with_object({}) { |branch, branches| branches[branch.name] = branch }
......
...@@ -75,27 +75,14 @@ RSpec.describe Projects::UpdateMirrorService do ...@@ -75,27 +75,14 @@ RSpec.describe Projects::UpdateMirrorService do
end end
context "when given URLs containing escaped elements" do 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 before do
allow(project).to receive(:import_url).and_return(url) allow(project).to receive(:import_url).and_return(url)
stub_fetch_mirror(project) stub_fetch_mirror(project)
end end
it "returns expected status" do
result = service.execute
expect(result[:status]).to eq(result_status)
end
end end
end end
......
...@@ -67,25 +67,12 @@ RSpec.describe Projects::UpdateRemoteMirrorService do ...@@ -67,25 +67,12 @@ RSpec.describe Projects::UpdateRemoteMirrorService do
end end
context "when given URLs containing escaped elements" do 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 before do
allow(remote_mirror).to receive(:url).and_return(url) allow(remote_mirror).to receive(:url).and_return(url)
end end
it "returns expected status" do
result = execute!
expect(result[:status]).to eq(result_status)
end
end end
end end
......
# 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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment