Commit 61d45d5c authored by Yorick Peterse's avatar Yorick Peterse

Merge branch 'security-2767-verify-lfs-finalize-from-workhorse' into 'master'

[master] Verify that LFS upload requests are genuine

Closes #2767

See merge request gitlab/gitlabhq!2767
parents 610247e9 87c5abfc
8.1.0 8.1.1
\ No newline at end of file
...@@ -5,7 +5,7 @@ class Projects::LfsStorageController < Projects::GitHttpClientController ...@@ -5,7 +5,7 @@ class Projects::LfsStorageController < Projects::GitHttpClientController
include WorkhorseRequest include WorkhorseRequest
include SendFileUpload include SendFileUpload
skip_before_action :verify_workhorse_api!, only: [:download, :upload_finalize] skip_before_action :verify_workhorse_api!, only: :download
def download def download
lfs_object = LfsObject.find_by_oid(oid) lfs_object = LfsObject.find_by_oid(oid)
......
---
title: Verify that LFS upload requests are genuine
merge_request:
author:
type: security
...@@ -1086,6 +1086,12 @@ describe 'Git LFS API and storage' do ...@@ -1086,6 +1086,12 @@ describe 'Git LFS API and storage' do
end end
end end
context 'and request to finalize the upload is not sent by gitlab-workhorse' do
it 'fails with a JWT decode error' do
expect { put_finalize(lfs_tmp_file, verified: false) }.to raise_error(JWT::DecodeError)
end
end
context 'and workhorse requests upload finalize for a new lfs object' do context 'and workhorse requests upload finalize for a new lfs object' do
before do before do
lfs_object.destroy lfs_object.destroy
...@@ -1347,9 +1353,13 @@ describe 'Git LFS API and storage' do ...@@ -1347,9 +1353,13 @@ describe 'Git LFS API and storage' do
context 'when pushing the same lfs object to the second project' do context 'when pushing the same lfs object to the second project' do
before do before do
finalize_headers = headers
.merge('X-Gitlab-Lfs-Tmp' => lfs_tmp_file)
.merge(workhorse_internal_api_request_header)
put "#{second_project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}", put "#{second_project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}",
params: {}, params: {},
headers: headers.merge('X-Gitlab-Lfs-Tmp' => lfs_tmp_file).compact headers: finalize_headers
end end
it 'responds with status 200' do it 'responds with status 200' do
...@@ -1370,7 +1380,7 @@ describe 'Git LFS API and storage' do ...@@ -1370,7 +1380,7 @@ describe 'Git LFS API and storage' do
put "#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}/authorize", params: {}, headers: authorize_headers put "#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}/authorize", params: {}, headers: authorize_headers
end end
def put_finalize(lfs_tmp = lfs_tmp_file, with_tempfile: false, args: {}) def put_finalize(lfs_tmp = lfs_tmp_file, with_tempfile: false, verified: true, args: {})
upload_path = LfsObjectUploader.workhorse_local_upload_path upload_path = LfsObjectUploader.workhorse_local_upload_path
file_path = upload_path + '/' + lfs_tmp if lfs_tmp file_path = upload_path + '/' + lfs_tmp if lfs_tmp
...@@ -1384,11 +1394,14 @@ describe 'Git LFS API and storage' do ...@@ -1384,11 +1394,14 @@ describe 'Git LFS API and storage' do
'file.name' => File.basename(file_path) 'file.name' => File.basename(file_path)
} }
put_finalize_with_args(args.merge(extra_args).compact) put_finalize_with_args(args.merge(extra_args).compact, verified: verified)
end end
def put_finalize_with_args(args) def put_finalize_with_args(args, verified:)
put "#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}", params: args, headers: headers finalize_headers = headers
finalize_headers.merge!(workhorse_internal_api_request_header) if verified
put "#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}", params: args, headers: finalize_headers
end end
def lfs_tmp_file def lfs_tmp_file
......
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