diff options
-rw-r--r-- | lib/api/applications.rb | 2 | ||||
-rw-r--r-- | lib/api/project_import.rb | 3 | ||||
-rw-r--r-- | spec/requests/api/applications_spec.rb | 62 | ||||
-rw-r--r-- | spec/requests/api/project_import_spec.rb | 34 | ||||
-rw-r--r-- | workhorse/internal/artifacts/artifacts_upload_test.go | 2 | ||||
-rw-r--r-- | workhorse/internal/upload/exif/testdata/takes_lot_of_memory_to_decode.tiff | bin | 0 -> 9662 bytes | |||
-rw-r--r-- | workhorse/internal/upload/rewrite.go | 11 | ||||
-rw-r--r-- | workhorse/internal/upload/rewrite_test.go | 13 | ||||
-rw-r--r-- | workhorse/internal/upload/uploads.go | 3 | ||||
-rw-r--r-- | workhorse/internal/upload/uploads_test.go | 43 |
10 files changed, 146 insertions, 27 deletions
diff --git a/lib/api/applications.rb b/lib/api/applications.rb index be482272b20..346bd6ccfe4 100644 --- a/lib/api/applications.rb +++ b/lib/api/applications.rb @@ -15,7 +15,7 @@ module API params do requires :name, type: String, desc: 'Application name' requires :redirect_uri, type: String, desc: 'Application redirect URI' - requires :scopes, type: String, desc: 'Application scopes' + requires :scopes, type: String, desc: 'Application scopes', allow_blank: false optional :confidential, type: Boolean, default: true, desc: 'Application will be used where the client secret is confidential' diff --git a/lib/api/project_import.rb b/lib/api/project_import.rb index d43184ff75d..e7c532e2483 100644 --- a/lib/api/project_import.rb +++ b/lib/api/project_import.rb @@ -9,6 +9,8 @@ module API feature_category :importers + before { authenticate! unless route.settings[:skip_authentication] } + helpers do def import_params declared_params(include_missing: false) @@ -109,6 +111,7 @@ module API detail 'This feature was introduced in GitLab 10.6.' success Entities::ProjectImportStatus end + route_setting :skip_authentication, true get ':id/import' do present user_project, with: Entities::ProjectImportStatus end diff --git a/spec/requests/api/applications_spec.rb b/spec/requests/api/applications_spec.rb index 959e68e6a0d..022451553ee 100644 --- a/spec/requests/api/applications_spec.rb +++ b/spec/requests/api/applications_spec.rb @@ -5,13 +5,14 @@ require 'spec_helper' RSpec.describe API::Applications, :api do let(:admin_user) { create(:user, admin: true) } let(:user) { create(:user, admin: false) } - let!(:application) { create(:application, name: 'another_application', owner: nil, redirect_uri: 'http://other_application.url', scopes: '') } + let(:scopes) { 'api' } + let!(:application) { create(:application, name: 'another_application', owner: nil, redirect_uri: 'http://other_application.url', scopes: scopes) } describe 'POST /applications' do context 'authenticated and authorized user' do it 'creates and returns an OAuth application' do expect do - post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'http://application.url', scopes: '' } + post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'http://application.url', scopes: scopes } end.to change { Doorkeeper::Application.count }.by 1 application = Doorkeeper::Application.find_by(name: 'application_name', redirect_uri: 'http://application.url') @@ -22,11 +23,12 @@ RSpec.describe API::Applications, :api do expect(json_response['secret']).to eq application.secret expect(json_response['callback_url']).to eq application.redirect_uri expect(json_response['confidential']).to eq application.confidential + expect(application.scopes.to_s).to eq('api') end it 'does not allow creating an application with the wrong redirect_uri format' do expect do - post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'http://', scopes: '' } + post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'http://', scopes: scopes } end.not_to change { Doorkeeper::Application.count } expect(response).to have_gitlab_http_status(:bad_request) @@ -36,7 +38,7 @@ RSpec.describe API::Applications, :api do it 'does not allow creating an application with a forbidden URI format' do expect do - post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'javascript://alert()', scopes: '' } + post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'javascript://alert()', scopes: scopes } end.not_to change { Doorkeeper::Application.count } expect(response).to have_gitlab_http_status(:bad_request) @@ -46,7 +48,7 @@ RSpec.describe API::Applications, :api do it 'does not allow creating an application without a name' do expect do - post api('/applications', admin_user), params: { redirect_uri: 'http://application.url', scopes: '' } + post api('/applications', admin_user), params: { redirect_uri: 'http://application.url', scopes: scopes } end.not_to change { Doorkeeper::Application.count } expect(response).to have_gitlab_http_status(:bad_request) @@ -56,7 +58,7 @@ RSpec.describe API::Applications, :api do it 'does not allow creating an application without a redirect_uri' do expect do - post api('/applications', admin_user), params: { name: 'application_name', scopes: '' } + post api('/applications', admin_user), params: { name: 'application_name', scopes: scopes } end.not_to change { Doorkeeper::Application.count } expect(response).to have_gitlab_http_status(:bad_request) @@ -64,19 +66,59 @@ RSpec.describe API::Applications, :api do expect(json_response['error']).to eq('redirect_uri is missing') end - it 'does not allow creating an application without scopes' do + it 'does not allow creating an application without specifying `scopes`' do expect do post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'http://application.url' } end.not_to change { Doorkeeper::Application.count } expect(response).to have_gitlab_http_status(:bad_request) expect(json_response).to be_a Hash - expect(json_response['error']).to eq('scopes is missing') + expect(json_response['error']).to eq('scopes is missing, scopes is empty') + end + + it 'does not allow creating an application with blank `scopes`' do + expect do + post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'http://application.url', scopes: '' } + end.not_to change { Doorkeeper::Application.count } + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['error']).to eq('scopes is empty') + end + + it 'does not allow creating an application with invalid `scopes`' do + expect do + post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'http://application.url', scopes: 'non_existent_scope' } + end.not_to change { Doorkeeper::Application.count } + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['message']['scopes'][0]).to eq('doesn\'t match configured on the server.') + end + + context 'multiple scopes' do + it 'creates an application with multiple `scopes` when each scope specified is seperated by a space' do + expect do + post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'http://application.url', scopes: 'api read_user' } + end.to change { Doorkeeper::Application.count }.by 1 + + application = Doorkeeper::Application.last + + expect(response).to have_gitlab_http_status(:created) + expect(application.scopes.to_s).to eq('api read_user') + end + + it 'does not allow creating an application with multiple `scopes` when one of the scopes is invalid' do + expect do + post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'http://application.url', scopes: 'api non_existent_scope' } + end.not_to change { Doorkeeper::Application.count } + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['message']['scopes'][0]).to eq('doesn\'t match configured on the server.') + end end it 'defaults to creating an application with confidential' do expect do - post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'http://application.url', scopes: '', confidential: nil } + post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'http://application.url', scopes: scopes, confidential: nil } end.to change { Doorkeeper::Application.count }.by(1) expect(response).to have_gitlab_http_status(:created) @@ -89,7 +131,7 @@ RSpec.describe API::Applications, :api do context 'authorized user without authorization' do it 'does not create application' do expect do - post api('/applications', user), params: { name: 'application_name', redirect_uri: 'http://application.url', scopes: '' } + post api('/applications', user), params: { name: 'application_name', redirect_uri: 'http://application.url', scopes: scopes } end.not_to change { Doorkeeper::Application.count } expect(response).to have_gitlab_http_status(:forbidden) diff --git a/spec/requests/api/project_import_spec.rb b/spec/requests/api/project_import_spec.rb index d3b24eb3832..0c9e125cc90 100644 --- a/spec/requests/api/project_import_spec.rb +++ b/spec/requests/api/project_import_spec.rb @@ -16,6 +16,16 @@ RSpec.describe API::ProjectImport do namespace.add_owner(user) end + shared_examples 'requires authentication' do + let(:user) { nil } + + it 'returns 401' do + subject + + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + describe 'POST /projects/import' do subject { upload_archive(file_upload, workhorse_headers, params) } @@ -32,6 +42,8 @@ RSpec.describe API::ProjectImport do allow(ImportExportUploader).to receive(:workhorse_upload_path).and_return('/') end + it_behaves_like 'requires authentication' + it 'executes a limited number of queries' do control_count = ActiveRecord::QueryRecorder.new { subject }.count @@ -281,6 +293,10 @@ RSpec.describe API::ProjectImport do end describe 'POST /projects/remote-import' do + subject do + post api('/projects/remote-import', user), params: params + end + let(:params) do { path: 'test-import', @@ -288,10 +304,12 @@ RSpec.describe API::ProjectImport do } end + it_behaves_like 'requires authentication' + it 'returns NOT FOUND when the feature is disabled' do stub_feature_flags(import_project_from_remote_file: false) - post api('/projects/remote-import', user), params: params + subject expect(response).to have_gitlab_http_status(:not_found) end @@ -315,7 +333,7 @@ RSpec.describe API::ProjectImport do .to receive(:execute) .and_return(service_response) - post api('/projects/remote-import', user), params: params + subject expect(response).to have_gitlab_http_status(:created) expect(json_response).to include({ @@ -338,7 +356,7 @@ RSpec.describe API::ProjectImport do .to receive(:execute) .and_return(service_response) - post api('/projects/remote-import', user), params: params + subject expect(response).to have_gitlab_http_status(:bad_request) expect(json_response).to eq({ @@ -350,6 +368,14 @@ RSpec.describe API::ProjectImport do end describe 'GET /projects/:id/import' do + it 'public project accessible for an unauthenticated user' do + project = create(:project, :public) + + get api("/projects/#{project.id}/import", nil) + + expect(response).to have_gitlab_http_status(:ok) + end + it 'returns the import status' do project = create(:project, :import_started) project.add_maintainer(user) @@ -376,6 +402,8 @@ RSpec.describe API::ProjectImport do describe 'POST /projects/import/authorize' do subject { post api('/projects/import/authorize', user), headers: workhorse_headers } + it_behaves_like 'requires authentication' + it 'authorizes importing project with workhorse header' do subject diff --git a/workhorse/internal/artifacts/artifacts_upload_test.go b/workhorse/internal/artifacts/artifacts_upload_test.go index 3e8a52be1a1..df1c30dcff0 100644 --- a/workhorse/internal/artifacts/artifacts_upload_test.go +++ b/workhorse/internal/artifacts/artifacts_upload_test.go @@ -270,7 +270,7 @@ func TestUploadHandlerForMultipleFiles(t *testing.T) { require.NoError(t, s.writer.Close()) response := testUploadArtifacts(t, s.writer.FormDataContentType(), s.url, s.buffer) - require.Equal(t, http.StatusInternalServerError, response.Code) + require.Equal(t, http.StatusBadRequest, response.Code) } func TestUploadFormProcessing(t *testing.T) { diff --git a/workhorse/internal/upload/exif/testdata/takes_lot_of_memory_to_decode.tiff b/workhorse/internal/upload/exif/testdata/takes_lot_of_memory_to_decode.tiff Binary files differnew file mode 100644 index 00000000000..6935cb130db --- /dev/null +++ b/workhorse/internal/upload/exif/testdata/takes_lot_of_memory_to_decode.tiff diff --git a/workhorse/internal/upload/rewrite.go b/workhorse/internal/upload/rewrite.go index 7945fa1f34d..3dfab120188 100644 --- a/workhorse/internal/upload/rewrite.go +++ b/workhorse/internal/upload/rewrite.go @@ -25,7 +25,10 @@ import ( ) // ErrInjectedClientParam means that the client sent a parameter that overrides one of our own fields -var ErrInjectedClientParam = errors.New("injected client parameter") +var ( + ErrInjectedClientParam = errors.New("injected client parameter") + ErrMultipleFilesUploaded = errors.New("upload request contains more than one file") +) var ( multipartUploadRequests = promauto.NewCounterVec( @@ -114,6 +117,10 @@ func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, pr } func (rew *rewriter) handleFilePart(ctx context.Context, name string, p *multipart.Part, opts *filestore.SaveFileOpts) error { + if rew.filter.Count() > 0 { + return ErrMultipleFilesUploaded + } + multipartFiles.WithLabelValues(rew.filter.Name()).Inc() filename := filepath.Base(p.FileName()) @@ -226,7 +233,7 @@ func handleExifUpload(ctx context.Context, r io.Reader, filename string, imageTy } func isTIFF(r io.Reader) bool { - _, err := tiff.Decode(r) + _, err := tiff.DecodeConfig(r) if err == nil { return true } diff --git a/workhorse/internal/upload/rewrite_test.go b/workhorse/internal/upload/rewrite_test.go index 6fc41c3fefd..e3f33a02489 100644 --- a/workhorse/internal/upload/rewrite_test.go +++ b/workhorse/internal/upload/rewrite_test.go @@ -2,6 +2,7 @@ package upload import ( "os" + "runtime" "testing" "github.com/stretchr/testify/require" @@ -29,6 +30,10 @@ func TestImageTypeRecongition(t *testing.T) { filename: "exif/testdata/sample_exif_invalid.jpg", isJPEG: false, isTIFF: false, + }, { + filename: "exif/testdata/takes_lot_of_memory_to_decode.tiff", // File from https://gitlab.com/gitlab-org/gitlab/-/issues/341363 + isJPEG: false, + isTIFF: true, }, } @@ -36,8 +41,16 @@ func TestImageTypeRecongition(t *testing.T) { t.Run(test.filename, func(t *testing.T) { input, err := os.Open(test.filename) require.NoError(t, err) + + var m runtime.MemStats + runtime.ReadMemStats(&m) + start := m.TotalAlloc + require.Equal(t, test.isJPEG, isJPEG(input)) require.Equal(t, test.isTIFF, isTIFF(input)) + + runtime.ReadMemStats(&m) + require.Less(t, m.TotalAlloc-start, uint64(50000), "must take reasonable amount of memory to recognise the type") }) } } diff --git a/workhorse/internal/upload/uploads.go b/workhorse/internal/upload/uploads.go index 7a93fa4a9d8..8f4c8bb95f0 100644 --- a/workhorse/internal/upload/uploads.go +++ b/workhorse/internal/upload/uploads.go @@ -21,6 +21,7 @@ type MultipartFormProcessor interface { ProcessField(ctx context.Context, formName string, writer *multipart.Writer) error Finalize(ctx context.Context) error Name() string + Count() int } func HandleFileUploads(w http.ResponseWriter, r *http.Request, h http.Handler, preauth *api.Response, filter MultipartFormProcessor, opts *filestore.SaveFileOpts) { @@ -34,6 +35,8 @@ func HandleFileUploads(w http.ResponseWriter, r *http.Request, h http.Handler, p switch err { case ErrInjectedClientParam: helper.CaptureAndFail(w, r, err, "Bad Request", http.StatusBadRequest) + case ErrMultipleFilesUploaded: + helper.CaptureAndFail(w, r, err, "Uploading multiple files is not allowed", http.StatusBadRequest) case http.ErrNotMultipart: h.ServeHTTP(w, r) case filestore.ErrEntityTooLarge: diff --git a/workhorse/internal/upload/uploads_test.go b/workhorse/internal/upload/uploads_test.go index 29acf849e05..e82dcdcbc69 100644 --- a/workhorse/internal/upload/uploads_test.go +++ b/workhorse/internal/upload/uploads_test.go @@ -18,7 +18,6 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitlab/workhorse/internal/api" - "gitlab.com/gitlab-org/gitlab/workhorse/internal/filestore" "gitlab.com/gitlab-org/gitlab/workhorse/internal/helper" "gitlab.com/gitlab-org/gitlab/workhorse/internal/objectstore/test" "gitlab.com/gitlab-org/gitlab/workhorse/internal/proxy" @@ -28,11 +27,7 @@ import ( var nilHandler = http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) -type testFormProcessor struct{} - -func (a *testFormProcessor) ProcessFile(ctx context.Context, formName string, file *filestore.FileHandler, writer *multipart.Writer) error { - return nil -} +type testFormProcessor struct{ SavedFileTracker } func (a *testFormProcessor) ProcessField(ctx context.Context, formName string, writer *multipart.Writer) error { if formName != "token" && !strings.HasPrefix(formName, "file.") && !strings.HasPrefix(formName, "other.") { @@ -45,10 +40,6 @@ func (a *testFormProcessor) Finalize(ctx context.Context) error { return nil } -func (a *testFormProcessor) Name() string { - return "" -} - func TestUploadTempPathRequirement(t *testing.T) { apiResponse := &api.Response{} preparer := &DefaultPreparer{} @@ -259,6 +250,38 @@ func TestUploadProcessingField(t *testing.T) { require.Equal(t, 500, response.Code) } +func TestUploadingMultipleFiles(t *testing.T) { + testhelper.ConfigureSecret() + + tempPath, err := ioutil.TempDir("", "uploads") + require.NoError(t, err) + defer os.RemoveAll(tempPath) + + var buffer bytes.Buffer + + writer := multipart.NewWriter(&buffer) + _, err = writer.CreateFormFile("file", "my.file") + require.NoError(t, err) + _, err = writer.CreateFormFile("file", "my.file") + require.NoError(t, err) + require.NoError(t, writer.Close()) + + httpRequest, err := http.NewRequest("PUT", "/url/path", &buffer) + require.NoError(t, err) + httpRequest.Header.Set("Content-Type", writer.FormDataContentType()) + + response := httptest.NewRecorder() + apiResponse := &api.Response{TempPath: tempPath} + preparer := &DefaultPreparer{} + opts, _, err := preparer.Prepare(apiResponse) + require.NoError(t, err) + + HandleFileUploads(response, httpRequest, nilHandler, apiResponse, &testFormProcessor{}, opts) + + require.Equal(t, 400, response.Code) + require.Equal(t, "Uploading multiple files is not allowed\n", response.Body.String()) +} + func TestUploadProcessingFile(t *testing.T) { tempPath, err := ioutil.TempDir("", "uploads") require.NoError(t, err) |