Commit 822d06fa authored by GitLab Release Tools Bot's avatar GitLab Release Tools Bot

Merge branch 'security-262724-pypi-sha256-validation-14-10' into '14-10-stable-ee'

Add validation to pypi file sha256 values

See merge request gitlab-org/security/gitlab!2415
parents e9d059fa afc796f4
...@@ -35,6 +35,7 @@ class Packages::PackageFile < ApplicationRecord ...@@ -35,6 +35,7 @@ class Packages::PackageFile < ApplicationRecord
validates :file_name, presence: true validates :file_name, presence: true
validates :file_name, uniqueness: { scope: :package }, if: -> { !pending_destruction? && package&.pypi? } validates :file_name, uniqueness: { scope: :package }, if: -> { !pending_destruction? && package&.pypi? }
validates :file_sha256, format: { with: Gitlab::Regex.sha256_regex }, if: -> { package&.pypi? }, allow_nil: true
scope :recent, -> { order(id: :desc) } scope :recent, -> { order(id: :desc) }
scope :limit_recent, ->(limit) { recent.limit(limit) } scope :limit_recent, ->(limit) { recent.limit(limit) }
......
...@@ -174,7 +174,7 @@ module API ...@@ -174,7 +174,7 @@ module API
requires :version, type: String requires :version, type: String
optional :requires_python, type: String optional :requires_python, type: String
optional :md5_digest, type: String optional :md5_digest, type: String
optional :sha256_digest, type: String optional :sha256_digest, type: String, regexp: Gitlab::Regex.sha256_regex
end end
route_setting :authentication, deploy_token_allowed: true, basic_auth_personal_access_token: true, job_token_allowed: :basic_auth route_setting :authentication, deploy_token_allowed: true, basic_auth_personal_access_token: true, job_token_allowed: :basic_auth
......
...@@ -237,6 +237,10 @@ module Gitlab ...@@ -237,6 +237,10 @@ module Gitlab
generic_package_name_regex generic_package_name_regex
end end
def sha256_regex
@sha256_regex ||= /\A[0-9a-f]{64}\z/i.freeze
end
private private
def conan_name_regex def conan_name_regex
......
...@@ -1005,4 +1005,19 @@ RSpec.describe Gitlab::Regex do ...@@ -1005,4 +1005,19 @@ RSpec.describe Gitlab::Regex do
it { is_expected.not_to match('.xt.est_') } it { is_expected.not_to match('.xt.est_') }
it { is_expected.not_to match('0test1') } it { is_expected.not_to match('0test1') }
end end
describe '.sha256_regex' do
subject { described_class.sha256_regex }
it { is_expected.to match('a' * 64) }
it { is_expected.to match('abcdefABCDEF1234567890abcdefABCDEF1234567890abcdefABCDEF12345678') }
it { is_expected.not_to match('a' * 63) }
it { is_expected.not_to match('a' * 65) }
it { is_expected.not_to match('a' * 63 + 'g') }
it { is_expected.not_to match('a' * 63 + '{') }
it { is_expected.not_to match('a' * 63 + '%') }
it { is_expected.not_to match('a' * 63 + '*') }
it { is_expected.not_to match('a' * 63 + '#') }
it { is_expected.not_to match('') }
end
end end
...@@ -29,19 +29,48 @@ RSpec.describe Packages::PackageFile, type: :model do ...@@ -29,19 +29,48 @@ RSpec.describe Packages::PackageFile, type: :model do
let(:package_file) { package.package_files.first } let(:package_file) { package.package_files.first }
let(:status) { :default } let(:status) { :default }
let(:file_name) { 'foo' }
let(:file) { fixture_file_upload('spec/fixtures/dk.png') } let(:file) { fixture_file_upload('spec/fixtures/dk.png') }
let(:params) { { file: file, file_name: file_name, status: status } }
subject { package.package_files.create!(file: file, file_name: package_file.file_name, status: status) } subject { package.package_files.create!(params) }
it 'can not save a duplicated file' do context 'file_name' do
expect { subject }.to raise_error(ActiveRecord::RecordInvalid, "Validation failed: File name has already been taken") let(:file_name) { package_file.file_name }
it 'can not save a duplicated file' do
expect { subject }.to raise_error(ActiveRecord::RecordInvalid, "Validation failed: File name has already been taken")
end
context 'with a pending destruction package duplicated file' do
let(:status) { :pending_destruction }
it 'can save it' do
expect { subject }.to change { package.package_files.count }.from(1).to(2)
end
end
end end
context 'with a pending destruction package duplicated file' do context 'file_sha256' do
let(:status) { :pending_destruction } where(:sha256_value, :expected_success) do
'a' * 64 | true
nil | true
'a' * 63 | false
'a' * 65 | false
'a' * 63 + '%' | false
'' | false
end
with_them do
let(:params) { super().merge({ file_sha256: sha256_value }) }
it 'can save it' do it 'does not allow invalid sha256 characters' do
expect { subject }.to change { package.package_files.count }.from(1).to(2) if expected_success
expect { subject }.not_to raise_error
else
expect { subject }.to raise_error(ActiveRecord::RecordInvalid, "Validation failed: File sha256 is invalid")
end
end
end end
end end
end end
......
...@@ -136,7 +136,7 @@ RSpec.describe API::PypiPackages do ...@@ -136,7 +136,7 @@ RSpec.describe API::PypiPackages do
let(:url) { "/projects/#{project.id}/packages/pypi" } let(:url) { "/projects/#{project.id}/packages/pypi" }
let(:headers) { {} } let(:headers) { {} }
let(:requires_python) { '>=3.7' } let(:requires_python) { '>=3.7' }
let(:base_params) { { requires_python: requires_python, version: '1.0.0', name: 'sample-project', sha256_digest: '123' } } let(:base_params) { { requires_python: requires_python, version: '1.0.0', name: 'sample-project', sha256_digest: '1' * 64 } }
let(:params) { base_params.merge(content: temp_file(file_name)) } let(:params) { base_params.merge(content: temp_file(file_name)) }
let(:send_rewritten_field) { true } let(:send_rewritten_field) { true }
let(:snowplow_gitlab_standard_context) { { project: project, namespace: project.namespace, user: user } } let(:snowplow_gitlab_standard_context) { { project: project, namespace: project.namespace, user: user } }
...@@ -221,6 +221,19 @@ RSpec.describe API::PypiPackages do ...@@ -221,6 +221,19 @@ RSpec.describe API::PypiPackages do
it_behaves_like 'returning response status', :bad_request it_behaves_like 'returning response status', :bad_request
end end
context 'with an invalid sha256' do
let(:token) { personal_access_token.token }
let(:user_headers) { basic_auth_header(user.username, token) }
let(:headers) { user_headers.merge(workhorse_headers) }
before do
params[:sha256_digest] = 'a' * 63 + '%'
project.add_developer(user)
end
it_behaves_like 'returning response status', :bad_request
end
it_behaves_like 'deploy token for package uploads' it_behaves_like 'deploy token for package uploads'
it_behaves_like 'job token for package uploads' it_behaves_like 'job token for package uploads'
......
...@@ -7,6 +7,9 @@ RSpec.describe Packages::Pypi::CreatePackageService, :aggregate_failures do ...@@ -7,6 +7,9 @@ RSpec.describe Packages::Pypi::CreatePackageService, :aggregate_failures do
let_it_be(:project) { create(:project) } let_it_be(:project) { create(:project) }
let_it_be(:user) { create(:user) } let_it_be(:user) { create(:user) }
let(:sha256) { '1' * 64 }
let(:md5) { '567' }
let(:requires_python) { '>=2.7' } let(:requires_python) { '>=2.7' }
let(:params) do let(:params) do
{ {
...@@ -14,8 +17,8 @@ RSpec.describe Packages::Pypi::CreatePackageService, :aggregate_failures do ...@@ -14,8 +17,8 @@ RSpec.describe Packages::Pypi::CreatePackageService, :aggregate_failures do
version: '1.0', version: '1.0',
content: temp_file('foo.tgz'), content: temp_file('foo.tgz'),
requires_python: requires_python, requires_python: requires_python,
sha256_digest: '123', sha256_digest: sha256,
md5_digest: '567' md5_digest: md5
} }
end end
...@@ -34,8 +37,8 @@ RSpec.describe Packages::Pypi::CreatePackageService, :aggregate_failures do ...@@ -34,8 +37,8 @@ RSpec.describe Packages::Pypi::CreatePackageService, :aggregate_failures do
expect(created_package.pypi_metadatum.required_python).to eq '>=2.7' expect(created_package.pypi_metadatum.required_python).to eq '>=2.7'
expect(created_package.package_files.size).to eq 1 expect(created_package.package_files.size).to eq 1
expect(created_package.package_files.first.file_name).to eq 'foo.tgz' expect(created_package.package_files.first.file_name).to eq 'foo.tgz'
expect(created_package.package_files.first.file_sha256).to eq '123' expect(created_package.package_files.first.file_sha256).to eq sha256
expect(created_package.package_files.first.file_md5).to eq '567' expect(created_package.package_files.first.file_md5).to eq md5
end end
end end
...@@ -74,8 +77,8 @@ RSpec.describe Packages::Pypi::CreatePackageService, :aggregate_failures do ...@@ -74,8 +77,8 @@ RSpec.describe Packages::Pypi::CreatePackageService, :aggregate_failures do
context 'with an existing file' do context 'with an existing file' do
before do before do
params[:content] = temp_file('foo.tgz') params[:content] = temp_file('foo.tgz')
params[:sha256_digest] = 'abc' params[:sha256_digest] = sha256
params[:md5_digest] = 'def' params[:md5_digest] = md5
end end
it 'throws an error' do it 'throws an error' do
...@@ -101,8 +104,8 @@ RSpec.describe Packages::Pypi::CreatePackageService, :aggregate_failures do ...@@ -101,8 +104,8 @@ RSpec.describe Packages::Pypi::CreatePackageService, :aggregate_failures do
expect(created_package.pypi_metadatum.required_python).to eq '>=2.7' expect(created_package.pypi_metadatum.required_python).to eq '>=2.7'
expect(created_package.package_files.size).to eq 1 expect(created_package.package_files.size).to eq 1
expect(created_package.package_files.first.file_name).to eq 'foo.tgz' expect(created_package.package_files.first.file_name).to eq 'foo.tgz'
expect(created_package.package_files.first.file_sha256).to eq 'abc' expect(created_package.package_files.first.file_sha256).to eq sha256
expect(created_package.package_files.first.file_md5).to eq 'def' expect(created_package.package_files.first.file_md5).to eq md5
end end
end end
end end
......
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