From e8190bd3d0ad87b98f920f144e098a6a2e08cca0 Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Mon, 17 Jun 2024 10:43:14 -0700
Subject: [PATCH] Skip IP lookups in validating URLs on certain conditions
Previously anytime `ApplicationSetting` were updated `UrlBlocker`
would attempt to resolve the IPs and determine whether they were
allowed by the current settings. However, in an offline environment,
services like Diagrams.net may not be resolved even if they are
enabled by default.
This commit skips the validations that require resolving IP addresses
if there are no restrictions on outgoing requests:
- Allow requests to the local network is checked
- Allow requests to the local network from system hooks is checked
- DNS rebinding attack protection is disabled
- Block all requests, except for IP addresses, IP ranges, and domain
names defined in the allowlist (`deny_all_requests_except_allowed`)
is disabled
Note that the URL validators in `ApplicationSetting` only pass in the
current `deny_all_requests_except_allowed` setting, so if that is
active then IP resolution will occur.
Relates to https://gitlab.com/gitlab-org/gitlab/-/issues/467524
Changelog: fixed
---
gems/gitlab-http/.rubocop.yml | 4 ++
gems/gitlab-http/Gemfile.lock | 23 ++++++++
gems/gitlab-http/gitlab-http.gemspec | 1 +
.../lib/gitlab/http_v2/url_blocker.rb | 4 ++
.../spec/gitlab/http_v2/url_blocker_spec.rb | 59 +++++++++++++++++++
5 files changed, 91 insertions(+)
diff --git a/gems/gitlab-http/.rubocop.yml b/gems/gitlab-http/.rubocop.yml
index 8bc6b6a4cfbed..75941091b8935 100644
--- a/gems/gitlab-http/.rubocop.yml
+++ b/gems/gitlab-http/.rubocop.yml
@@ -20,3 +20,7 @@ RSpec/MultipleMemoizedHelpers:
RSpec/FilePath:
CustomTransform:
HTTP_V2: http_v2
+
+Lint/BinaryOperatorWithIdenticalOperands:
+ Exclude:
+ - 'spec/**/*.rb'
diff --git a/gems/gitlab-http/Gemfile.lock b/gems/gitlab-http/Gemfile.lock
index afdf6f3dce282..91cd6eca94f8e 100644
--- a/gems/gitlab-http/Gemfile.lock
+++ b/gems/gitlab-http/Gemfile.lock
@@ -55,11 +55,14 @@ GEM
addressable (2.8.4)
public_suffix (>= 2.0.2, < 6.0)
ast (2.4.2)
+ binding_of_caller (1.0.1)
+ debug_inspector (>= 1.2.0)
builder (3.2.4)
concurrent-ruby (1.2.2)
crack (0.4.5)
rexml
crass (1.0.6)
+ debug_inspector (1.2.0)
diff-lcs (1.5.0)
erubi (1.12.0)
gitlab-styles (10.1.0)
@@ -91,6 +94,10 @@ GEM
parser (3.2.2.3)
ast (~> 2.4.1)
racc
+ proc_to_ast (0.2.0)
+ parser
+ rouge
+ unparser
public_suffix (5.0.1)
racc (1.7.1)
rack (2.2.7)
@@ -113,6 +120,7 @@ GEM
rake (13.0.6)
regexp_parser (2.8.1)
rexml (3.2.5)
+ rouge (4.3.0)
rspec (3.12.0)
rspec-core (~> 3.12.0)
rspec-expectations (~> 3.12.0)
@@ -125,6 +133,17 @@ GEM
rspec-mocks (3.12.5)
diff-lcs (>= 1.2.0, < 2.0)
rspec-support (~> 3.12.0)
+ rspec-parameterized (1.0.2)
+ rspec-parameterized-core (< 2)
+ rspec-parameterized-table_syntax (< 2)
+ rspec-parameterized-core (1.0.1)
+ parser
+ proc_to_ast (>= 0.2.0)
+ rspec (>= 2.13, < 4)
+ unparser
+ rspec-parameterized-table_syntax (1.0.1)
+ binding_of_caller
+ rspec-parameterized-core (< 2)
rspec-rails (6.0.3)
actionpack (>= 6.1)
activesupport (>= 6.1)
@@ -168,6 +187,9 @@ GEM
tzinfo (2.0.6)
concurrent-ruby (~> 1.0)
unicode-display_width (2.4.2)
+ unparser (0.6.8)
+ diff-lcs (~> 1.3)
+ parser (>= 3.2.0)
uri (0.13.0)
webmock (3.18.1)
addressable (>= 2.8.0)
@@ -184,6 +206,7 @@ DEPENDENCIES
gitlab-rspec!
gitlab-styles (~> 10.1.0)
gitlab-utils!
+ rspec-parameterized (~> 1.0.2)
rspec-rails (~> 6.0.3)
rubocop (~> 1.50.2)
rubocop-rspec (~> 2.22)
diff --git a/gems/gitlab-http/gitlab-http.gemspec b/gems/gitlab-http/gitlab-http.gemspec
index 2378945cbcc35..89306c0b21e2c 100644
--- a/gems/gitlab-http/gitlab-http.gemspec
+++ b/gems/gitlab-http/gitlab-http.gemspec
@@ -28,6 +28,7 @@ Gem::Specification.new do |spec|
spec.add_runtime_dependency "net-http", "= 0.4.1"
spec.add_development_dependency 'gitlab-styles', '~> 10.1.0'
+ spec.add_development_dependency 'rspec-parameterized', '~> 1.0.2'
spec.add_development_dependency 'rspec-rails', '~> 6.0.3'
spec.add_development_dependency "rubocop", "~> 1.50.2"
spec.add_development_dependency "rubocop-rspec", "~> 2.22"
diff --git a/gems/gitlab-http/lib/gitlab/http_v2/url_blocker.rb b/gems/gitlab-http/lib/gitlab/http_v2/url_blocker.rb
index 7c669a0d9c6f8..bfee1b2b40b62 100644
--- a/gems/gitlab-http/lib/gitlab/http_v2/url_blocker.rb
+++ b/gems/gitlab-http/lib/gitlab/http_v2/url_blocker.rb
@@ -81,6 +81,10 @@ def validate_url_with_proxy!(
ascii_only: ascii_only
)
+ unless deny_all_requests_except_allowed || dns_rebind_protection || !allow_local_network || !allow_localhost
+ return Result.new(uri, nil, true)
+ end
+
validate_resolved_uri(uri,
allow_localhost: allow_localhost,
allow_local_network: allow_local_network,
diff --git a/gems/gitlab-http/spec/gitlab/http_v2/url_blocker_spec.rb b/gems/gitlab-http/spec/gitlab/http_v2/url_blocker_spec.rb
index 904fed9baefca..75f57e731222b 100644
--- a/gems/gitlab-http/spec/gitlab/http_v2/url_blocker_spec.rb
+++ b/gems/gitlab-http/spec/gitlab/http_v2/url_blocker_spec.rb
@@ -1,6 +1,7 @@
# frozen_string_literal: true
require 'spec_helper'
+require 'rspec-parameterized'
RSpec.describe Gitlab::HTTP_V2::UrlBlocker, :stub_invalid_dns_only, feature_category: :shared do
let(:schemes) { %w[http https] }
@@ -37,6 +38,64 @@
end
end
end
+
+ context 'when one flag requring IP validation is enabled' do
+ using RSpec::Parameterized::TableSyntax
+
+ let(:import_url) { 'https://localhost' }
+
+ where(:deny_all_requests_except_allowed, :dns_rebind_protection, :allow_local_network, :allow_localhost) do
+ true | false | true | true
+ false | true | true | true
+ false | false | false | true
+ false | false | true | false
+ end
+
+ with_them do
+ let(:options) do
+ {
+ deny_all_requests_except_allowed: deny_all_requests_except_allowed,
+ dns_rebind_protection: dns_rebind_protection,
+ allow_local_network: allow_local_network,
+ allow_localhost: allow_localhost,
+ schemes: schemes
+ }
+ end
+
+ it 'attempts to validate IP' do
+ expect(described_class).to receive(:validate_resolved_uri).and_return(
+ described_class::Result.new(import_url, 'localhost', false))
+
+ subject
+ end
+ end
+ end
+
+ context 'when all forms of IP validation are disabled' do
+ let(:options) do
+ {
+ deny_all_requests_except_allowed: false,
+ dns_rebind_protection: false,
+ allow_local_network: true,
+ allow_localhost: true,
+ schemes: schemes
+ }
+ end
+
+ let(:import_url) { 'https://localhost' }
+ let(:expected_uri) { 'https://localhost' }
+ let(:expected_hostname) { nil }
+ let(:expected_use_proxy) { false }
+
+ it 'does not attempt to resolve IPs' do
+ expect(Addrinfo).not_to receive(:getaddrinfo)
+
+ uri, hostname = subject
+
+ expect(uri).to eq(Addressable::URI.parse(expected_uri))
+ expect(hostname).to eq(expected_hostname)
+ end
+ end
end
describe '#validate_url_with_proxy!' do
--
GitLab