Add latest changes from gitlab-org/security/gitlab@14-4-stable-ee

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
new 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)