Skip to content
Snippets Groups Projects
Verified Commit 5ad5acc6 authored by Emma Park's avatar Emma Park Committed by GitLab
Browse files

Fix 500 error on IP restrictions when checking snippets

Previously, PostgreSQL's CIDR type was used for IP range checks
which caused 500 errors when ranges didn't conform to
CIDR format requirements
(like having bits set to the right of mask, e.g. 1.1.1.1/22)

This change:
- Uses PostgreSQL's more lenient INET type instead of CIDR
- Adds comprehensive test coverage for different IP scenarios:
  - IPs within/outside allowed ranges
  - Non-CIDR-compliant ranges
  - Multiple IP restrictions per group
  - Specific IP matching

Changelog: changed
EE: true
parent 912dd8ac
No related branches found
No related tags found
2 merge requests!180727Resolve "Extend job archival mechanism to the whole pipeline",!180331Fix 500 error on IP restrictions when displaying snippet names
---
name: snippet_ip_restrictions
feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/511506
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/180331
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/517917
milestone: '17.9'
type: development
group: group::source code
default_enabled: false
......@@ -23,7 +23,18 @@ def all_snippets
override :filter_snippets
def filter_snippets
by_repository_storage(super)
if ::Feature.enabled?(:snippet_ip_restrictions, current_user)
valid_snippets = filter_unauthorized_snippets(super)
by_repository_storage(valid_snippets)
else
by_repository_storage(super)
end
end
def filter_unauthorized_snippets(snippets)
current_ip = ::Gitlab::IpAddressState.current
snippets.allowed_for_ip(current_ip)
end
# This method returns snippets from a more restrictive scope.
......
......@@ -13,6 +13,24 @@ module Snippet
scope :by_repository_storage, ->(storage) do
joins(snippet_repository: :shard).where(shards: { name: storage })
end
scope :with_ip_restrictions, -> do
only_project_snippets.joins(project: { group: :ip_restrictions })
end
scope :allowed_for_ip, ->(current_ip) do
out_of_ip_range = <<-SQL.squish
NOT EXISTS (
SELECT 1
FROM ip_restrictions
WHERE group_id = namespaces.id
AND inet(?) <<= range::inet
)
SQL
restricted_group_snippets = with_ip_restrictions.where(out_of_ip_range, current_ip)
where.not(id: restricted_group_snippets.select(:id))
end
end
override :repository_size_checker
......
......@@ -245,4 +245,53 @@
end
end
end
context 'filter by restricted IPs' do
subject { described_class.new(guest, author: owner).execute }
let_it_be(:private_group) { create(:group, :private) }
let_it_be(:group_without_ip_restriction) { create(:group, :private) }
let_it_be(:owner) { create(:user, owner_of: private_group) }
let_it_be(:guest) { create(:user, guest_of: private_group) }
let_it_be(:restricted_group_ip) { '10.0.0.0/8' }
let_it_be(:ip_restriction) { create(:ip_restriction, group: private_group, range: restricted_group_ip) }
let_it_be(:snippet_with_ip_restriction) do
create(:project_snippet, project: create(:project, group: private_group), author: owner)
end
let_it_be(:snippet_without_ip_restriction) do
create(:project_snippet, project: create(:project, group: group_without_ip_restriction), author: owner)
end
let(:user_ip) { '10.0.0.0/8' }
before do
stub_licensed_features(group_ip_restriction: true)
allow(Gitlab::IpAddressState).to receive(:current).and_return(user_ip)
stub_application_setting(globally_allowed_ips: "")
group_without_ip_restriction.add_guest(guest)
end
it 'returns all snippets' do
expect(subject).to contain_exactly(snippet_with_ip_restriction, snippet_without_ip_restriction)
end
context 'when the user has no access to the group because of the restricted IP rules' do
let(:user_ip) { '127.0.0.1' }
it 'returns only the accessible snippets' do
expect(subject).to contain_exactly(snippet_without_ip_restriction)
end
end
context 'when feature flag "snippet_ip_restrictions" is disabled' do
before do
stub_feature_flags(snippet_ip_restrictions: false)
end
it 'returns all snippets' do
expect(subject).to contain_exactly(snippet_with_ip_restriction, snippet_without_ip_restriction)
end
end
end
end
......@@ -36,4 +36,100 @@
expect(snippets).to eq([snippet_in_default_storage])
end
end
describe '.allowed_for_ip' do
let_it_be(:current_ip) { '2.2.2.2' }
# Group with valid IP range
let_it_be(:valid_group) { create(:group) }
let_it_be(:valid_ip_restriction) { create(:ip_restriction, group: valid_group, range: '10.0.0.0/22') }
let_it_be(:snippet_with_valid_restriction) do
create(:project_snippet, project: create(:project, group: valid_group))
end
# Group with PostgreSQL CIDR-invalid IP range
# This range (1.1.1.1/22) is valid in networking terms and works with inet type,
# but PostgreSQL considers it invalid for cidr type because it has bits set to right of mask.
# We test this to ensure the inet casting handles it correctly.
let_it_be(:non_cidr_compliant_group) { create(:group) }
let_it_be(:non_cidr_compliant_restriction) do
create(:ip_restriction, group: non_cidr_compliant_group, range: '1.1.1.1/22')
end
let_it_be(:snippet_with_non_cidr_restriction) do
create(:project_snippet, project: create(:project, group: non_cidr_compliant_group))
end
# Group with multiple IP restrictions
let_it_be(:multi_group) { create(:group) }
let_it_be(:multi_restriction_1) { create(:ip_restriction, group: multi_group, range: '20.0.0.0/22') }
let_it_be(:multi_restriction_2) { create(:ip_restriction, group: multi_group, range: '30.0.0.0/22') }
let_it_be(:multi_restriction_3) { create(:ip_restriction, group: multi_group, range: '40.0.0.0/22') }
let_it_be(:snippet_with_multi_restrictions) do
create(:project_snippet, project: create(:project, group: multi_group))
end
# Group with specific IP
let_it_be(:specific_group) { create(:group) }
let_it_be(:specific_ip_restriction) { create(:ip_restriction, group: specific_group, range: '50.0.0.1') }
let_it_be(:snippet_with_specific_ip) do
create(:project_snippet, project: create(:project, group: specific_group))
end
# Snippet without any IP restrictions
let_it_be(:snippet_without_restriction) { create(:project_snippet) }
let_it_be(:personal_snippet) { create(:personal_snippet) }
subject(:allowed_snippets) { described_class.allowed_for_ip(current_ip) }
context 'when IP does not fall within any group range' do
let(:current_ip) { '2.2.2.2' }
it { is_expected.to contain_exactly(snippet_without_restriction, personal_snippet) }
end
context 'when IP falls within only non_cidr_restriction group range' do
let(:current_ip) { '1.1.1.100' }
it 'returns the expected snippets' do
is_expected.to contain_exactly(snippet_without_restriction, personal_snippet, snippet_with_non_cidr_restriction)
end
end
context 'when IP falls within only valid group range' do
let(:current_ip) { '10.0.1.100' }
it 'returns the expected snippets' do
is_expected.to contain_exactly(snippet_without_restriction, personal_snippet, snippet_with_valid_restriction)
end
end
context 'when IP falls within some of multiple restriction group range' do
let(:current_ip) { '30.0.1.100' }
it 'returns the expected snippets' do
is_expected.to contain_exactly(snippet_without_restriction, personal_snippet, snippet_with_multi_restrictions)
end
end
context 'when IP exactly matches specific IP restriction' do
let(:current_ip) { '50.0.0.1' }
it { is_expected.to contain_exactly(snippet_without_restriction, personal_snippet, snippet_with_specific_ip) }
end
context 'when the user IP falls within both non_cidr and valid group restrictions' do
let_it_be(:valid_ip_restriction) { create(:ip_restriction, group: valid_group, range: '1.1.0.0/22') }
let(:current_ip) { '1.1.1.100' }
it 'returns the expected snippets' do
expect(allowed_snippets).to include(snippet_with_non_cidr_restriction)
expect(allowed_snippets).to include(snippet_with_valid_restriction)
expect(allowed_snippets).not_to include(snippet_with_multi_restrictions)
expect(allowed_snippets).not_to include(snippet_with_specific_ip)
expect(allowed_snippets).to include(snippet_without_restriction)
expect(allowed_snippets).to include(personal_snippet)
end
end
end
end
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment