diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2022-01-20 18:11:58 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2022-01-20 18:11:58 +0300 |
commit | c17eb7c97062d25cdf1b44573e4c0241f52aa2fe (patch) | |
tree | 5164ebf4ccf1701cce78d7b4121c414ab370db6e /workhorse/internal | |
parent | 60e09a0cef4e104aa41e20ab7a40499f3343e90f (diff) |
Add latest changes from gitlab-org/gitlab@master
Diffstat (limited to 'workhorse/internal')
-rw-r--r-- | workhorse/internal/upload/rewrite.go | 17 | ||||
-rw-r--r-- | workhorse/internal/upload/rewrite_test.go | 46 | ||||
-rw-r--r-- | workhorse/internal/upload/uploads.go | 2 | ||||
-rw-r--r-- | workhorse/internal/upload/uploads_test.go | 95 |
4 files changed, 159 insertions, 1 deletions
diff --git a/workhorse/internal/upload/rewrite.go b/workhorse/internal/upload/rewrite.go index b9324ac8b7b..8101ca2b698 100644 --- a/workhorse/internal/upload/rewrite.go +++ b/workhorse/internal/upload/rewrite.go @@ -10,6 +10,7 @@ import ( "net/http" "os" "path/filepath" + "regexp" "strings" "github.com/prometheus/client_golang/prometheus" @@ -30,8 +31,11 @@ const maxFilesAllowed = 10 var ( ErrInjectedClientParam = errors.New("injected client parameter") ErrTooManyFilesUploaded = fmt.Errorf("upload request contains more than %v files", maxFilesAllowed) + ErrUnexpectedFilePart = errors.New("Content-Disposition contains unexpected filepart") ) +var filePartRegex = regexp.MustCompile(`;\s*filename\b`) + var ( multipartUploadRequests = promauto.NewCounterVec( prometheus.CounterOpts{ @@ -107,6 +111,11 @@ func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, pr if p.FileName() != "" { err = rew.handleFilePart(r.Context(), name, p, opts) } else { + err = verifyContentDisposition(p) + if err != nil { + return err + } + err = rew.copyPart(r.Context(), name, p) } @@ -118,6 +127,14 @@ func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, pr return nil } +func verifyContentDisposition(p *multipart.Part) error { + if filePartRegex.MatchString(p.Header.Get("Content-Disposition")) { + return ErrUnexpectedFilePart + } + + return nil +} + func (rew *rewriter) handleFilePart(ctx context.Context, name string, p *multipart.Part, opts *filestore.SaveFileOpts) error { if rew.filter.Count() >= maxFilesAllowed { return ErrTooManyFilesUploaded diff --git a/workhorse/internal/upload/rewrite_test.go b/workhorse/internal/upload/rewrite_test.go index e3f33a02489..9ae83d069b5 100644 --- a/workhorse/internal/upload/rewrite_test.go +++ b/workhorse/internal/upload/rewrite_test.go @@ -1,6 +1,8 @@ package upload import ( + "mime/multipart" + "net/textproto" "os" "runtime" "testing" @@ -54,3 +56,47 @@ func TestImageTypeRecongition(t *testing.T) { }) } } + +func TestVerifyContentDisposition(t *testing.T) { + tests := []struct { + desc string + contentDisposition string + error error + }{ + { + desc: "without content disposition", + contentDisposition: "", + error: nil, + }, { + desc: "content disposition without filename", + contentDisposition: `form-data; name="filename"`, + error: nil, + }, { + desc: "with filename", + contentDisposition: `form-data; name="file"; filename=foobar`, + error: ErrUnexpectedFilePart, + }, { + desc: "with filename*", + contentDisposition: `form-data; name="file"; filename*=UTF-8''foobar`, + error: ErrUnexpectedFilePart, + }, { + desc: "filename and filename*", + contentDisposition: `form-data; name="file"; filename=foobar; filename*=UTF-8''foobar`, + error: ErrUnexpectedFilePart, + }, + } + + for _, testCase := range tests { + t.Run(testCase.desc, func(t *testing.T) { + h := make(textproto.MIMEHeader) + + if testCase.contentDisposition != "" { + h.Set("Content-Disposition", testCase.contentDisposition) + h.Set("Content-Type", "application/octet-stream") + } + + require.Equal(t, testCase.error, verifyContentDisposition(&multipart.Part{Header: h})) + }) + } + +} diff --git a/workhorse/internal/upload/uploads.go b/workhorse/internal/upload/uploads.go index b408d260379..c7daa9bbf18 100644 --- a/workhorse/internal/upload/uploads.go +++ b/workhorse/internal/upload/uploads.go @@ -35,7 +35,7 @@ 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 ErrTooManyFilesUploaded: + case ErrTooManyFilesUploaded, ErrUnexpectedFilePart: helper.CaptureAndFail(w, r, err, err.Error(), http.StatusBadRequest) case http.ErrNotMultipart: h.ServeHTTP(w, r) diff --git a/workhorse/internal/upload/uploads_test.go b/workhorse/internal/upload/uploads_test.go index 7c22b3b4e28..d238895c2bd 100644 --- a/workhorse/internal/upload/uploads_test.go +++ b/workhorse/internal/upload/uploads_test.go @@ -4,10 +4,12 @@ import ( "bytes" "context" "fmt" + "io" "io/ioutil" "mime/multipart" "net/http" "net/http/httptest" + "net/textproto" "os" "regexp" "strconv" @@ -384,6 +386,99 @@ func TestInvalidFileNames(t *testing.T) { } } +func TestContentDisposition(t *testing.T) { + testhelper.ConfigureSecret() + + tempPath, err := ioutil.TempDir("", "uploads") + require.NoError(t, err) + defer os.RemoveAll(tempPath) + + tests := []struct { + desc string + contentDisposition string + code int + body string + }{ + { + desc: "empty header", + contentDisposition: "", + code: 200, + body: "", + }, + { + desc: "without filename", + contentDisposition: `form-data; name="filename"`, + code: 200, + body: "", + }, + { + desc: "with filename", + contentDisposition: `form-data; name="file"; filename=foobar`, + code: 200, + body: "", + }, + { + desc: "with filename* supported charset", + contentDisposition: `form-data; name="file"; filename*=UTF-8''foobar`, + code: 200, + body: "", + }, + { + desc: "with both filename and filename* supported charset", + contentDisposition: `form-data; name="file"; filename=foobar; filename*=UTF-8''foobar`, + code: 200, + body: "", + }, + { + desc: "with filename and filename* unsupported charset", + contentDisposition: `form-data; name="file"; filename=foobar; filename*=UTF-16''foobar`, + code: 200, + body: "", + }, + { + desc: "unsupported charset", + contentDisposition: `form-data; name="file"; filename*=UTF-16''foobar`, + code: 400, + body: ErrUnexpectedFilePart.Error() + "\n", + }, + } + + for _, testCase := range tests { + t.Run(testCase.desc, func(t *testing.T) { + // Create custom Content-Disposition with filename* and charset + // Example: filename*=UTF-8''application.log + h := make(textproto.MIMEHeader) + + h.Set("Content-Disposition", testCase.contentDisposition) + h.Set("Content-Type", "application/octet-stream") + + buffer := &bytes.Buffer{} + writer := multipart.NewWriter(buffer) + file, err := writer.CreatePart(h) + require.NoError(t, err) + fmt.Fprint(file, "test") + writer.Close() + + httpRequest := httptest.NewRequest("POST", "/example", buffer) + 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, &SavedFileTracker{Request: httpRequest}, opts) + + body, err := io.ReadAll(response.Body) + require.NoError(t, err) + + require.Equal(t, testCase.code, response.Code) + require.Equal(t, testCase.body, string(body)) + }) + } +} + func TestUploadHandlerRemovingExif(t *testing.T) { content, err := ioutil.ReadFile("exif/testdata/sample_exif.jpg") require.NoError(t, err) |