diff --git a/changelogs/unreleased/authorize-upload-filesize.yml b/changelogs/unreleased/authorize-upload-filesize.yml
new file mode 100644
index 0000000000000000000000000000000000000000..24bcfce458e76eb43e33cf940b889c17c68b7b0f
--- /dev/null
+++ b/changelogs/unreleased/authorize-upload-filesize.yml
@@ -0,0 +1,5 @@
+---
+title: Reject upload when filesize exceeds MaximumSize returned by authorize endpoint
+merge_request:
+author:
+type: added
diff --git a/internal/api/api.go b/internal/api/api.go
index 9fa45267d84133ea2ad49ff96506a0b8a63fc975..5db947a157f82aa90a8215081ec088370219e632 100644
--- a/internal/api/api.go
+++ b/internal/api/api.go
@@ -148,6 +148,8 @@ type Response struct {
ProcessLsif bool
// Detects whether LSIF artifact will be parsed with references
ProcessLsifReferences bool
+ // The maximum accepted size in bytes of the upload
+ MaximumSize int64
}
// singleJoiningSlash is taken from reverseproxy.go:NewSingleHostReverseProxy
diff --git a/internal/artifacts/artifacts_store_test.go b/internal/artifacts/artifacts_store_test.go
index 2da3b7535654e30b60044d19fc3de795fc90969b..784acb1428a9e028480508eda8e335bd5c125dad 100644
--- a/internal/artifacts/artifacts_store_test.go
+++ b/internal/artifacts/artifacts_store_test.go
@@ -260,13 +260,12 @@ func TestUploadHandlerMultipartUploadSizeLimit(t *testing.T) {
objectURL := server.URL + test.ObjectPath
- partSize := int64(1)
uploadSize := 10
preauth := api.Response{
RemoteObject: api.RemoteObject{
ID: "store-id",
MultipartUpload: &api.MultipartUploadParams{
- PartSize: partSize,
+ PartSize: 1,
PartURLs: []string{objectURL + "?partNumber=1"},
AbortURL: objectURL, // DELETE
CompleteURL: objectURL, // POST
@@ -292,3 +291,47 @@ func TestUploadHandlerMultipartUploadSizeLimit(t *testing.T) {
assert.False(t, os.IsMultipartUpload(test.ObjectPath), "MultipartUpload should not be in progress anymore")
assert.Empty(t, os.GetObjectMD5(test.ObjectPath), "upload should have failed, so the object should not exists")
}
+
+func TestUploadHandlerMultipartUploadMaximumSizeFromApi(t *testing.T) {
+ os, server := test.StartObjectStore()
+ defer server.Close()
+
+ err := os.InitiateMultipartUpload(test.ObjectPath)
+ require.NoError(t, err)
+
+ objectURL := server.URL + test.ObjectPath
+
+ uploadSize := int64(10)
+ maxSize := uploadSize - 1
+ preauth := api.Response{
+ MaximumSize: maxSize,
+ RemoteObject: api.RemoteObject{
+ ID: "store-id",
+ MultipartUpload: &api.MultipartUploadParams{
+ PartSize: uploadSize,
+ PartURLs: []string{objectURL + "?partNumber=1"},
+ AbortURL: objectURL, // DELETE
+ CompleteURL: objectURL, // POST
+ },
+ },
+ }
+
+ responseProcessor := func(w http.ResponseWriter, r *http.Request) {
+ t.Fatal("it should not be called")
+ }
+
+ ts := testArtifactsUploadServer(t, preauth, responseProcessor)
+ defer ts.Close()
+
+ contentBuffer, contentType := createTestMultipartForm(t, make([]byte, uploadSize))
+ response := testUploadArtifacts(t, contentType, ts.URL+Path, &contentBuffer)
+ require.Equal(t, http.StatusRequestEntityTooLarge, response.Code)
+
+ testhelper.Retry(t, 5*time.Second, func() error {
+ if os.GetObjectMD5(test.ObjectPath) == "" {
+ return nil
+ }
+
+ return fmt.Errorf("file is still present")
+ })
+}
diff --git a/internal/filestore/file_handler.go b/internal/filestore/file_handler.go
index 581ccda168e12245f3cb3036a02fe7c7e55b691b..3f345642131e8a74022691d8091e0077f025045f 100644
--- a/internal/filestore/file_handler.go
+++ b/internal/filestore/file_handler.go
@@ -166,7 +166,14 @@ func SaveFileFromReader(ctx context.Context, reader io.Reader, size int64, opts
if err != nil {
return nil, err
}
+
writers = append(writers, remoteWriter)
+
+ defer func() {
+ if err != nil {
+ remoteWriter.CloseWithError(err)
+ }
+ }()
} else {
clientMode = "local"
@@ -182,12 +189,26 @@ func SaveFileFromReader(ctx context.Context, reader io.Reader, size int64, opts
return nil, errors.New("missing upload destination")
}
+ if opts.MaximumSize > 0 {
+ if size > opts.MaximumSize {
+ return nil, SizeError(fmt.Errorf("the upload size %d is over maximum of %d bytes", size, opts.MaximumSize))
+ }
+
+ // We allow to read an extra byte to check later if we exceed the max size
+ reader = &io.LimitedReader{R: reader, N: opts.MaximumSize + 1}
+ }
+
multiWriter := io.MultiWriter(writers...)
fh.Size, err = io.Copy(multiWriter, reader)
if err != nil {
return nil, err
}
+ if opts.MaximumSize > 0 && fh.Size > opts.MaximumSize {
+ // An extra byte was read thus exceeding the max size
+ return nil, ErrEntityTooLarge
+ }
+
if size != -1 && size != fh.Size {
return nil, SizeError(fmt.Errorf("expected %d bytes but got only %d", size, fh.Size))
}
diff --git a/internal/filestore/file_handler_test.go b/internal/filestore/file_handler_test.go
index 8a096013506026f26ee0c7b91de2bc104248d146..1e11a68aaba7423535d73fbaba609671d99dfca7 100644
--- a/internal/filestore/file_handler_test.go
+++ b/internal/filestore/file_handler_test.go
@@ -69,6 +69,36 @@ func TestSaveFileWrongSize(t *testing.T) {
assert.Nil(t, fh)
}
+func TestSaveFileWithKnownSizeExceedLimit(t *testing.T) {
+ ctx, cancel := context.WithCancel(context.Background())
+ defer cancel()
+
+ tmpFolder, err := ioutil.TempDir("", "workhorse-test-tmp")
+ require.NoError(t, err)
+ defer os.RemoveAll(tmpFolder)
+
+ opts := &filestore.SaveFileOpts{LocalTempPath: tmpFolder, TempFilePrefix: "test-file", MaximumSize: test.ObjectSize - 1}
+ fh, err := filestore.SaveFileFromReader(ctx, strings.NewReader(test.ObjectContent), test.ObjectSize, opts)
+ assert.Error(t, err)
+ _, isSizeError := err.(filestore.SizeError)
+ assert.True(t, isSizeError, "Should fail with SizeError")
+ assert.Nil(t, fh)
+}
+
+func TestSaveFileWithUnknownSizeExceedLimit(t *testing.T) {
+ ctx, cancel := context.WithCancel(context.Background())
+ defer cancel()
+
+ tmpFolder, err := ioutil.TempDir("", "workhorse-test-tmp")
+ require.NoError(t, err)
+ defer os.RemoveAll(tmpFolder)
+
+ opts := &filestore.SaveFileOpts{LocalTempPath: tmpFolder, TempFilePrefix: "test-file", MaximumSize: test.ObjectSize - 1}
+ fh, err := filestore.SaveFileFromReader(ctx, strings.NewReader(test.ObjectContent), -1, opts)
+ assert.Equal(t, err, filestore.ErrEntityTooLarge)
+ assert.Nil(t, fh)
+}
+
func TestSaveFromDiskNotExistingFile(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
diff --git a/internal/filestore/save_file_opts.go b/internal/filestore/save_file_opts.go
index 337a20b50333c380de6e5f692abf359ac54fb2b7..1eb708c3f55cc2a6e6a379997ae356b93040b26e 100644
--- a/internal/filestore/save_file_opts.go
+++ b/internal/filestore/save_file_opts.go
@@ -51,6 +51,8 @@ type SaveFileOpts struct {
ObjectStorageConfig ObjectStorageConfig
// Deadline it the S3 operation deadline, the upload will be aborted if not completed in time
Deadline time.Time
+ // The maximum accepted size in bytes of the upload
+ MaximumSize int64
//MultipartUpload parameters
// PartSize is the exact size of each uploaded part. Only the last one can be smaller
@@ -95,6 +97,7 @@ func GetOpts(apiResponse *api.Response) (*SaveFileOpts, error) {
UseWorkhorseClient: apiResponse.RemoteObject.UseWorkhorseClient,
RemoteTempObjectID: apiResponse.RemoteObject.RemoteTempObjectID,
Deadline: time.Now().Add(timeout),
+ MaximumSize: apiResponse.MaximumSize,
}
if opts.LocalTempPath != "" && opts.RemoteID != "" {
diff --git a/internal/objectstore/gocloud_object_test.go b/internal/objectstore/gocloud_object_test.go
index 4f79a429a377058fa1814e78b6b3da789376a77f..35b4b84f1a0db017a20b1b8f636bc59fa70879a0 100644
--- a/internal/objectstore/gocloud_object_test.go
+++ b/internal/objectstore/gocloud_object_test.go
@@ -49,22 +49,18 @@ func TestGoCloudObjectUpload(t *testing.T) {
require.Equal(t, []byte(test.ObjectContent), received)
cancel()
- deleted := false
- retry(3, time.Second, func() error {
+ testhelper.Retry(t, 5*time.Second, func() error {
exists, err := bucket.Exists(ctx, objectName)
require.NoError(t, err)
if exists {
- return fmt.Errorf("file %s is still present, retrying", objectName)
+ return fmt.Errorf("file %s is still present", objectName)
} else {
- deleted = true
return nil
}
})
- require.True(t, deleted)
-
// Verify no log noise when deleting a file that already is gone
object.Delete()
entries := logHook.AllEntries()
diff --git a/internal/objectstore/s3_object_test.go b/internal/objectstore/s3_object_test.go
index f265fe26ca2aeb5fc1e6992d2478092271bae0ce..1f4e530321b6d893a9865f5eb5679c7131f8a3ed 100644
--- a/internal/objectstore/s3_object_test.go
+++ b/internal/objectstore/s3_object_test.go
@@ -19,6 +19,7 @@ import (
"gitlab.com/gitlab-org/gitlab-workhorse/internal/config"
"gitlab.com/gitlab-org/gitlab-workhorse/internal/objectstore"
"gitlab.com/gitlab-org/gitlab-workhorse/internal/objectstore/test"
+ "gitlab.com/gitlab-org/gitlab-workhorse/internal/testhelper"
)
func TestS3ObjectUpload(t *testing.T) {
@@ -59,18 +60,14 @@ func TestS3ObjectUpload(t *testing.T) {
test.CheckS3Metadata(t, sess, config, objectName)
cancel()
- deleted := false
- retry(3, time.Second, func() error {
+ testhelper.Retry(t, 5*time.Second, func() error {
if test.S3ObjectDoesNotExist(t, sess, config, objectName) {
- deleted = true
return nil
- } else {
- return fmt.Errorf("file is still present, retrying")
}
- })
- require.True(t, deleted)
+ return fmt.Errorf("file is still present")
+ })
})
}
}
@@ -153,23 +150,3 @@ func TestS3ObjectUploadCancel(t *testing.T) {
_, err = io.Copy(object, strings.NewReader(test.ObjectContent))
require.Error(t, err)
}
-
-func retry(attempts int, sleep time.Duration, fn func() error) error {
- if err := fn(); err != nil {
- if s, ok := err.(stop); ok {
- // Return the original error for later checking
- return s.error
- }
-
- if attempts--; attempts > 0 {
- time.Sleep(sleep)
- return retry(attempts, 2*sleep, fn)
- }
- return err
- }
- return nil
-}
-
-type stop struct {
- error
-}
diff --git a/internal/objectstore/uploader.go b/internal/objectstore/uploader.go
index 93a09208ab517b78077b60e3e4e62df65a879192..8fbedf5cc863ad9d35f5122c714fb9ead412dbc8 100644
--- a/internal/objectstore/uploader.go
+++ b/internal/objectstore/uploader.go
@@ -8,6 +8,7 @@ import (
"hash"
"io"
"strings"
+ "sync"
"time"
"gitlab.com/gitlab-org/labkit/log"
@@ -16,6 +17,7 @@ import (
// Upload represents an upload to an ObjectStorage provider
type Upload interface {
io.WriteCloser
+ CloseWithError(error) error
ETag() string
}
@@ -28,7 +30,6 @@ type uploader struct {
md5 hash.Hash
w io.Writer
- c io.Closer
// uploadError is the last error occourred during upload
uploadError error
@@ -39,25 +40,34 @@ type uploader struct {
pw *io.PipeWriter
strategy uploadStrategy
metrics bool
+
+ // closeOnce is used to prevent multiple calls to pw.Close
+ // which may result to Close overriding the error set by CloseWithError
+ // Bug fixed in v1.14: https://github.com/golang/go/commit/f45eb9ff3c96dfd951c65d112d033ed7b5e02432
+ closeOnce sync.Once
}
func newUploader(strategy uploadStrategy) uploader {
pr, pw := io.Pipe()
- return uploader{w: pw, c: pw, pr: pr, pw: pw, strategy: strategy, metrics: true}
+ return uploader{w: pw, pr: pr, pw: pw, strategy: strategy, metrics: true}
}
func newMD5Uploader(strategy uploadStrategy, metrics bool) uploader {
pr, pw := io.Pipe()
hasher := md5.New()
mw := io.MultiWriter(pw, hasher)
- return uploader{w: mw, c: pw, pr: pr, pw: pw, md5: hasher, strategy: strategy, metrics: metrics}
+ return uploader{w: mw, pr: pr, pw: pw, md5: hasher, strategy: strategy, metrics: metrics}
}
// Close implements the standard io.Closer interface: it closes the http client request.
// This method will also wait for the connection to terminate and return any error occurred during the upload
func (u *uploader) Close() error {
- if err := u.c.Close(); err != nil {
- return err
+ var closeError error
+ u.closeOnce.Do(func() {
+ closeError = u.pw.Close()
+ })
+ if closeError != nil {
+ return closeError
}
<-u.ctx.Done()
@@ -69,6 +79,14 @@ func (u *uploader) Close() error {
return u.uploadError
}
+func (u *uploader) CloseWithError(err error) error {
+ u.closeOnce.Do(func() {
+ u.pw.CloseWithError(err)
+ })
+
+ return nil
+}
+
func (u *uploader) Write(p []byte) (int, error) {
return u.w.Write(p)
}
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index ab4e7e658f619ce6c21521cdfc235d555cb4cca4..67b72e8d2082c330e2055400ca9190c47fbad705 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -161,3 +161,16 @@ func WaitForLogEvent(hook *test.Hook) bool {
return false
}
+
+func Retry(t testing.TB, timeout time.Duration, fn func() error) {
+ t.Helper()
+ start := time.Now()
+ var err error
+ for ; time.Since(start) < timeout; time.Sleep(time.Millisecond) {
+ err = fn()
+ if err == nil {
+ return
+ }
+ }
+ t.Fatalf("test timeout after %v; last error: %v", timeout, err)
+}