FIX: When following redirects before cloning, use the first git request (#19269)

This is closer to git's redirect following behaviour. We prevented git
following redirects when we clone in order to prevent SSRF attacks.

Follow-up-to: 291bbc4fb9
This commit is contained in:
Daniel Waterworth 2022-11-30 14:21:09 -06:00 committed by GitHub
parent aea492df5e
commit d9364a272e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 48 additions and 35 deletions

View File

@ -67,7 +67,7 @@ class FinalDestination
@status = :ready
@follow_canonical = @opts[:follow_canonical]
@http_verb = http_verb(@force_get_hosts, @follow_canonical)
@http_verb = @opts[:http_verb] || http_verb(@force_get_hosts, @follow_canonical)
@cookie = nil
@limited_ips = []
@verbose = @opts[:verbose] || false
@ -82,8 +82,8 @@ class FinalDestination
20
end
def self.resolve(url)
new(url).resolve
def self.resolve(url, opts = nil)
new(url, opts).resolve
end
def http_verb(force_get_hosts, follow_canonical)

View File

@ -77,6 +77,25 @@ class ThemeStore::GitImporter
protected
def redirected_uri
first_clone_uri = @uri.dup
first_clone_uri.path.gsub!(/\/\z/, "")
first_clone_uri.path += "/info/refs"
first_clone_uri.query = "service=git-upload-pack"
redirected_uri = FinalDestination.resolve(first_clone_uri.to_s, http_verb: :get)
if redirected_uri&.path.ends_with?("/info/refs")
redirected_uri.path.gsub!(/\/info\/refs\z/, "")
redirected_uri.query = nil
redirected_uri
else
@uri
end
rescue
@uri
end
def raise_import_error!
raise RemoteTheme::ImportError.new(I18n.t("themes.import_error.git"))
end
@ -117,44 +136,28 @@ class ThemeStore::GitImporter
end
def clone_http!
uris = [@uri]
@uri = redirected_uri
@url = @uri.to_s
begin
resolved_uri = FinalDestination.resolve(@uri.to_s)
if resolved_uri && resolved_uri != @uri
uris.unshift(resolved_uri)
end
rescue
# If this fails, we can stil attempt to clone using the original URI
unless ["http", "https"].include?(@uri.scheme)
raise_import_error!
end
uris.each do |uri|
@uri = uri
@url = @uri.to_s
addresses = FinalDestination::SSRFDetector.lookup_and_filter_ips(@uri.host)
unless ["http", "https"].include?(@uri.scheme)
raise_import_error!
end
unless addresses.empty?
env = { "GIT_TERMINAL_PROMPT" => "0" }
addresses = FinalDestination::SSRFDetector.lookup_and_filter_ips(@uri.host)
args = clone_args(
"http.followRedirects" => "false",
"http.curloptResolve" => "#{@uri.host}:#{@uri.port}:#{addresses.join(',')}",
)
unless addresses.empty?
env = { "GIT_TERMINAL_PROMPT" => "0" }
args = clone_args(
"http.followRedirects" => "false",
"http.curloptResolve" => "#{@uri.host}:#{@uri.port}:#{addresses.join(',')}",
)
begin
Discourse::Utils.execute_command(env, *args, timeout: COMMAND_TIMEOUT_SECONDS)
return
rescue RuntimeError
end
begin
Discourse::Utils.execute_command(env, *args, timeout: COMMAND_TIMEOUT_SECONDS)
rescue RuntimeError
end
end
raise_import_error!
end
def clone_ssh!

View File

@ -6,6 +6,7 @@ require 'theme_store/git_importer'
RSpec.describe ThemeStore::GitImporter do
describe "#import" do
let(:url) { "https://github.com/example/example.git" }
let(:first_fetch_url) { "https://github.com/example/example.git/info/refs?service=git-upload-pack" }
let(:trailing_slash_url) { "https://github.com/example/example/" }
let(:ssh_url) { "git@github.com:example/example.git" }
let(:branch) { "dev" }
@ -13,8 +14,17 @@ RSpec.describe ThemeStore::GitImporter do
before do
hex = "xxx"
SecureRandom.stubs(:hex).returns(hex)
FinalDestination.stubs(:resolve).with(url).returns(URI.parse(url))
FinalDestination::SSRFDetector.stubs(:lookup_and_filter_ips).with("github.com").returns(["192.0.2.100"])
FinalDestination::SSRFDetector
.stubs(:lookup_and_filter_ips)
.with("github.com")
.returns(["192.0.2.100"])
FinalDestination
.stubs(:resolve)
.with(first_fetch_url, http_verb: :get)
.returns(URI.parse(first_fetch_url))
@temp_folder = "#{Pathname.new(Dir.tmpdir).realpath}/discourse_theme_#{hex}"
@ssh_folder = "#{Pathname.new(Dir.tmpdir).realpath}/discourse_theme_ssh_#{hex}"
end