Commit 9c08f8f9 authored by dcouture's avatar dcouture Committed by Dominic Couture

Add a check for path traversal in manifest_file_name

parent a63cb9b7
...@@ -120,7 +120,7 @@ class Groups::DependencyProxyForContainersController < ::Groups::DependencyProxy ...@@ -120,7 +120,7 @@ class Groups::DependencyProxyForContainersController < ::Groups::DependencyProxy
end end
def manifest_file_name def manifest_file_name
@manifest_file_name ||= "#{image}:#{tag}.json" @manifest_file_name ||= Gitlab::Utils.check_path_traversal!("#{image}:#{tag}.json")
end end
def group def group
......
...@@ -47,6 +47,24 @@ RSpec.describe Groups::DependencyProxyForContainersController do ...@@ -47,6 +47,24 @@ RSpec.describe Groups::DependencyProxyForContainersController do
end end
end end
shared_examples 'with invalid path' do
context 'with invalid image' do
let(:image) { '../path_traversal' }
it 'raises an error' do
expect { subject }.to raise_error(Gitlab::Utils::PathTraversalAttackError, 'Invalid path')
end
end
context 'with invalid tag' do
let(:tag) { 'latest%2f..%2f..%2fpath_traversal' }
it 'raises an error' do
expect { subject }.to raise_error(Gitlab::Utils::PathTraversalAttackError, 'Invalid path')
end
end
end
shared_examples 'without permission' do shared_examples 'without permission' do
context 'with invalid user' do context 'with invalid user' do
before do before do
...@@ -164,8 +182,10 @@ RSpec.describe Groups::DependencyProxyForContainersController do ...@@ -164,8 +182,10 @@ RSpec.describe Groups::DependencyProxyForContainersController do
end end
describe 'GET #manifest' do describe 'GET #manifest' do
let_it_be(:image) { 'alpine' }
let_it_be(:tag) { 'latest' } let_it_be(:tag) { 'latest' }
let_it_be(:manifest) { create(:dependency_proxy_manifest, file_name: "alpine:#{tag}.json", group: group) } let_it_be(:file_name) { "#{image}:#{tag}.json" }
let_it_be(:manifest) { create(:dependency_proxy_manifest, file_name: file_name, group: group) }
let(:pull_response) { { status: :success, manifest: manifest, from_cache: false } } let(:pull_response) { { status: :success, manifest: manifest, from_cache: false } }
...@@ -235,6 +255,8 @@ RSpec.describe Groups::DependencyProxyForContainersController do ...@@ -235,6 +255,8 @@ RSpec.describe Groups::DependencyProxyForContainersController do
context 'with workhorse response' do context 'with workhorse response' do
let(:pull_response) { { status: :success, manifest: nil, from_cache: false } } let(:pull_response) { { status: :success, manifest: nil, from_cache: false } }
it_behaves_like 'with invalid path'
it 'returns Workhorse send-dependency instructions', :aggregate_failures do it 'returns Workhorse send-dependency instructions', :aggregate_failures do
subject subject
...@@ -246,7 +268,7 @@ RSpec.describe Groups::DependencyProxyForContainersController do ...@@ -246,7 +268,7 @@ RSpec.describe Groups::DependencyProxyForContainersController do
"Authorization" => ["Bearer abcd1234"], "Authorization" => ["Bearer abcd1234"],
"Accept" => ::ContainerRegistry::Client::ACCEPTED_TYPES "Accept" => ::ContainerRegistry::Client::ACCEPTED_TYPES
) )
expect(url).to eq(DependencyProxy::Registry.manifest_url('alpine', tag)) expect(url).to eq(DependencyProxy::Registry.manifest_url(image, tag))
expect(response.headers['Content-Type']).to eq('application/gzip') expect(response.headers['Content-Type']).to eq('application/gzip')
expect(response.headers['Content-Disposition']).to eq( expect(response.headers['Content-Disposition']).to eq(
ActionDispatch::Http::ContentDisposition.format(disposition: 'attachment', filename: manifest.file_name) ActionDispatch::Http::ContentDisposition.format(disposition: 'attachment', filename: manifest.file_name)
...@@ -277,7 +299,7 @@ RSpec.describe Groups::DependencyProxyForContainersController do ...@@ -277,7 +299,7 @@ RSpec.describe Groups::DependencyProxyForContainersController do
it_behaves_like 'not found when disabled' it_behaves_like 'not found when disabled'
def get_manifest(tag) def get_manifest(tag)
get :manifest, params: { group_id: group.to_param, image: 'alpine', tag: tag } get :manifest, params: { group_id: group.to_param, image: image, tag: tag }
end end
end end
...@@ -440,6 +462,7 @@ RSpec.describe Groups::DependencyProxyForContainersController do ...@@ -440,6 +462,7 @@ RSpec.describe Groups::DependencyProxyForContainersController do
end end
it_behaves_like 'a package tracking event', described_class.name, 'pull_manifest' it_behaves_like 'a package tracking event', described_class.name, 'pull_manifest'
it_behaves_like 'with invalid path'
context 'with no existing manifest' do context 'with no existing manifest' do
it 'creates a manifest' do it 'creates a manifest' 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