Commit 7560d2dc authored by Stan Hu's avatar Stan Hu

Fix URL blocker when object storage enabled but type is disabled

This commit fixes two problems:

1. If the object type is disabled but object storage is enabled (as it
is in Cloud Native GitLab), previously the URL blocker would attempt
to drill into the endpoint. We now check that the type is also enabled.

2. Due to a Settingslogic bug
(https://gitlab.com/gitlab-org/gitlab/-/issues/286873), the use of
`default` anywhere causes `Hash#default` to be overriden for the
`Settingslogic` class. If `connection` field `connection.endpoint` is
omittted from object storage config, this previously would result in
an error:

```
ArgumentError: wrong number of arguments (given 1, expected 0)>
```

We need to call `to_h` because we define `default` as a Gitaly storage
name in GitLab Charts.

Relates to https://gitlab.com/gitlab-org/gitlab/-/issues/357340

Changelog: fixed
parent dfc03562
...@@ -289,9 +289,10 @@ module Gitlab ...@@ -289,9 +289,10 @@ module Gitlab
ObjectStoreSettings::SUPPORTED_TYPES.collect do |type| ObjectStoreSettings::SUPPORTED_TYPES.collect do |type|
section_setting = config.try(type) section_setting = config.try(type)
next unless section_setting next unless section_setting && section_setting['enabled']
object_store_setting = section_setting['object_store'] # Use #to_h to avoid Settingslogic bug: https://gitlab.com/gitlab-org/gitlab/-/issues/286873
object_store_setting = section_setting['object_store']&.to_h
next unless object_store_setting && object_store_setting['enabled'] next unless object_store_setting && object_store_setting['enabled']
......
...@@ -167,6 +167,7 @@ RSpec.describe Gitlab::ImportExport::CommandLineUtil do ...@@ -167,6 +167,7 @@ RSpec.describe Gitlab::ImportExport::CommandLineUtil do
context 'for object_storage uri' do context 'for object_storage uri' do
let(:enabled_object_storage_setting) do let(:enabled_object_storage_setting) do
{ {
'enabled' => true,
'object_store' => 'object_store' =>
{ {
'enabled' => true, 'enabled' => true,
......
...@@ -43,6 +43,7 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only do ...@@ -43,6 +43,7 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only do
let(:import_url) { "#{host}/external-diffs/merge_request_diffs/mr-1/diff-1" } let(:import_url) { "#{host}/external-diffs/merge_request_diffs/mr-1/diff-1" }
let(:enabled_object_storage_setting) do let(:enabled_object_storage_setting) do
{ {
'enabled' => true,
'object_store' => 'object_store' =>
{ {
'enabled' => true, 'enabled' => true,
...@@ -81,6 +82,49 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only do ...@@ -81,6 +82,49 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only do
let(:expected_hostname) { nil } let(:expected_hostname) { nil }
end end
end end
context 'when LFS object storage is enabled' do
let(:lfs_config) do
{
'enabled' => lfs_enabled,
# This nesting of Settingslogic is necessary to trigger the bug
'object_store' => Settingslogic.new({ 'enabled' => true })
}
end
let(:config) do
{
'gitlab' => Gitlab.config.gitlab,
'repositories' => { 'storages' => { 'default' => 'test' } },
'lfs' => Settingslogic.new(lfs_config)
}
end
let(:host) { 'http://127.0.0.1:9000' }
let(:settings) { Settingslogic.new(config) }
before do
allow(Gitlab).to receive(:config).and_return(settings)
# Triggers Settingslogic bug: https://gitlab.com/gitlab-org/gitlab/-/issues/286873
settings.repositories.storages.default
end
context 'when LFS is disabled' do
let(:lfs_enabled) { false }
it 'raises an error' do
expect { subject }.to raise_error(described_class::BlockedUrlError)
end
end
context 'when LFS is enabled with no connection endpoint' do
let(:lfs_enabled) { true }
it 'raises an error' do
expect { subject }.to raise_error(described_class::BlockedUrlError)
end
end
end
end end
context 'when allow_object_storage is false' do context 'when allow_object_storage is false' do
......
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