diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-04-14 18:09:04 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-04-14 18:09:04 +0300 |
commit | a3dfd311f4660fc81e929058abd6e136ac884ed3 (patch) | |
tree | 4d7087cac6e2ca89df3adea98f92b9eddffa7790 /workhorse/internal | |
parent | 7f408a3831590a1f98a1e1a04b63acba8937e36b (diff) |
Add latest changes from gitlab-org/gitlab@master
Diffstat (limited to 'workhorse/internal')
-rw-r--r-- | workhorse/internal/upload/exif/exif.go | 27 | ||||
-rw-r--r-- | workhorse/internal/upload/exif/exif_test.go | 36 | ||||
-rw-r--r-- | workhorse/internal/upload/exif/testdata/sample_exif.tiff | bin | 0 -> 1039916 bytes | |||
-rw-r--r-- | workhorse/internal/upload/exif/testdata/sample_exif_corrupted.jpg | bin | 0 -> 2182 bytes | |||
-rw-r--r-- | workhorse/internal/upload/exif/testdata/sample_exif_invalid.jpg | 1 | ||||
-rw-r--r-- | workhorse/internal/upload/rewrite.go | 72 | ||||
-rw-r--r-- | workhorse/internal/upload/rewrite_test.go | 43 | ||||
-rw-r--r-- | workhorse/internal/upload/uploads_test.go | 77 |
8 files changed, 205 insertions, 51 deletions
diff --git a/workhorse/internal/upload/exif/exif.go b/workhorse/internal/upload/exif/exif.go index a9307b1ca90..db3c45431c0 100644 --- a/workhorse/internal/upload/exif/exif.go +++ b/workhorse/internal/upload/exif/exif.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "io" + "os" "os/exec" "regexp" @@ -22,6 +23,14 @@ type cleaner struct { eof bool } +type FileType int + +const ( + TypeUnknown FileType = iota + TypeJPEG + TypeTIFF +) + func NewCleaner(ctx context.Context, stdin io.Reader) (io.ReadCloser, error) { c := &cleaner{ctx: ctx} @@ -100,8 +109,20 @@ func (c *cleaner) startProcessing(stdin io.Reader) error { return nil } -func IsExifFile(filename string) bool { - filenameMatch := regexp.MustCompile(`(?i)\.(jpg|jpeg|tiff)$`) +func FileTypeFromSuffix(filename string) FileType { + if os.Getenv("SKIP_EXIFTOOL") == "1" { + return TypeUnknown + } + + jpegMatch := regexp.MustCompile(`(?i)^[^\n]*\.(jpg|jpeg)$`) + if jpegMatch.MatchString(filename) { + return TypeJPEG + } + + tiffMatch := regexp.MustCompile(`(?i)^[^\n]*\.tiff$`) + if tiffMatch.MatchString(filename) { + return TypeTIFF + } - return filenameMatch.MatchString(filename) + return TypeUnknown } diff --git a/workhorse/internal/upload/exif/exif_test.go b/workhorse/internal/upload/exif/exif_test.go index 373d97f7fce..ee5883d9e08 100644 --- a/workhorse/internal/upload/exif/exif_test.go +++ b/workhorse/internal/upload/exif/exif_test.go @@ -11,39 +11,57 @@ import ( "github.com/stretchr/testify/require" ) -func TestIsExifFile(t *testing.T) { +func TestFileTypeFromSuffix(t *testing.T) { tests := []struct { name string - expected bool + expected FileType }{ { name: "/full/path.jpg", - expected: true, + expected: TypeJPEG, }, { name: "path.jpeg", - expected: true, + expected: TypeJPEG, }, { name: "path.tiff", - expected: true, + expected: TypeTIFF, }, { name: "path.JPG", - expected: true, + expected: TypeJPEG, }, { name: "path.tar", - expected: false, + expected: TypeUnknown, }, { name: "path", - expected: false, + expected: TypeUnknown, + }, + { + name: "something.jpg.py", + expected: TypeUnknown, + }, + { + name: "something.py.jpg", + expected: TypeJPEG, + }, + { + name: `something.jpg + .py`, + expected: TypeUnknown, + }, + { + name: `something.something + .jpg`, + expected: TypeUnknown, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - require.Equal(t, test.expected, IsExifFile(test.name)) + require.Equal(t, test.expected, FileTypeFromSuffix(test.name)) }) } } diff --git a/workhorse/internal/upload/exif/testdata/sample_exif.tiff b/workhorse/internal/upload/exif/testdata/sample_exif.tiff Binary files differnew file mode 100644 index 00000000000..6671d818edb --- /dev/null +++ b/workhorse/internal/upload/exif/testdata/sample_exif.tiff diff --git a/workhorse/internal/upload/exif/testdata/sample_exif_corrupted.jpg b/workhorse/internal/upload/exif/testdata/sample_exif_corrupted.jpg Binary files differnew file mode 100644 index 00000000000..3b5c692de54 --- /dev/null +++ b/workhorse/internal/upload/exif/testdata/sample_exif_corrupted.jpg diff --git a/workhorse/internal/upload/exif/testdata/sample_exif_invalid.jpg b/workhorse/internal/upload/exif/testdata/sample_exif_invalid.jpg new file mode 100644 index 00000000000..9f8a284c64f --- /dev/null +++ b/workhorse/internal/upload/exif/testdata/sample_exif_invalid.jpg @@ -0,0 +1 @@ +invalid data diff --git a/workhorse/internal/upload/rewrite.go b/workhorse/internal/upload/rewrite.go index b8c9194f180..29c3e54f4f2 100644 --- a/workhorse/internal/upload/rewrite.go +++ b/workhorse/internal/upload/rewrite.go @@ -8,6 +8,7 @@ import ( "io/ioutil" "mime/multipart" "net/http" + "os" "path/filepath" "strings" @@ -15,6 +16,8 @@ import ( "github.com/prometheus/client_golang/prometheus/promauto" "gitlab.com/gitlab-org/labkit/log" + "golang.org/x/image/tiff" + "gitlab.com/gitlab-org/gitlab-workhorse/internal/api" "gitlab.com/gitlab-org/gitlab-workhorse/internal/filestore" "gitlab.com/gitlab-org/gitlab-workhorse/internal/lsif_transformer/parser" @@ -127,9 +130,11 @@ func (rew *rewriter) handleFilePart(ctx context.Context, name string, p *multipa var inputReader io.ReadCloser var err error + + imageType := exif.FileTypeFromSuffix(filename) switch { - case exif.IsExifFile(filename): - inputReader, err = handleExifUpload(ctx, p, filename) + case imageType != exif.TypeUnknown: + inputReader, err = handleExifUpload(ctx, p, filename, imageType) if err != nil { return err } @@ -169,12 +174,48 @@ func (rew *rewriter) handleFilePart(ctx context.Context, name string, p *multipa return rew.filter.ProcessFile(ctx, name, fh, rew.writer) } -func handleExifUpload(ctx context.Context, r io.Reader, filename string) (io.ReadCloser, error) { +func handleExifUpload(ctx context.Context, r io.Reader, filename string, imageType exif.FileType) (io.ReadCloser, error) { + tmpfile, err := ioutil.TempFile("", "exifremove") + if err != nil { + return nil, err + } + go func() { + <-ctx.Done() + tmpfile.Close() + }() + if err := os.Remove(tmpfile.Name()); err != nil { + return nil, err + } + + _, err = io.Copy(tmpfile, r) + if err != nil { + return nil, err + } + + tmpfile.Seek(0, io.SeekStart) + isValidType := false + switch imageType { + case exif.TypeJPEG: + isValidType = isJPEG(tmpfile) + case exif.TypeTIFF: + isValidType = isTIFF(tmpfile) + } + + tmpfile.Seek(0, io.SeekStart) + if !isValidType { + log.WithContextFields(ctx, log.Fields{ + "filename": filename, + "imageType": imageType, + }).Print("invalid content type, not running exiftool") + + return tmpfile, nil + } + log.WithContextFields(ctx, log.Fields{ "filename": filename, }).Print("running exiftool to remove any metadata") - cleaner, err := exif.NewCleaner(ctx, r) + cleaner, err := exif.NewCleaner(ctx, tmpfile) if err != nil { return nil, err } @@ -182,6 +223,29 @@ func handleExifUpload(ctx context.Context, r io.Reader, filename string) (io.Rea return cleaner, nil } +func isTIFF(r io.Reader) bool { + _, err := tiff.Decode(r) + if err == nil { + return true + } + + if _, unsupported := err.(tiff.UnsupportedError); unsupported { + return true + } + + return false +} + +func isJPEG(r io.Reader) bool { + // Only the first 512 bytes are used to sniff the content type. + buf, err := ioutil.ReadAll(io.LimitReader(r, 512)) + if err != nil { + return false + } + + return http.DetectContentType(buf) == "image/jpeg" +} + func handleLsifUpload(ctx context.Context, reader io.Reader, tempPath, filename string, preauth *api.Response) (io.ReadCloser, error) { parserConfig := parser.Config{ TempPath: tempPath, diff --git a/workhorse/internal/upload/rewrite_test.go b/workhorse/internal/upload/rewrite_test.go new file mode 100644 index 00000000000..6fc41c3fefd --- /dev/null +++ b/workhorse/internal/upload/rewrite_test.go @@ -0,0 +1,43 @@ +package upload + +import ( + "os" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestImageTypeRecongition(t *testing.T) { + tests := []struct { + filename string + isJPEG bool + isTIFF bool + }{ + { + filename: "exif/testdata/sample_exif.jpg", + isJPEG: true, + isTIFF: false, + }, { + filename: "exif/testdata/sample_exif.tiff", + isJPEG: false, + isTIFF: true, + }, { + filename: "exif/testdata/sample_exif_corrupted.jpg", + isJPEG: true, + isTIFF: false, + }, { + filename: "exif/testdata/sample_exif_invalid.jpg", + isJPEG: false, + isTIFF: false, + }, + } + + for _, test := range tests { + t.Run(test.filename, func(t *testing.T) { + input, err := os.Open(test.filename) + require.NoError(t, err) + require.Equal(t, test.isJPEG, isJPEG(input)) + require.Equal(t, test.isTIFF, isTIFF(input)) + }) + } +} diff --git a/workhorse/internal/upload/uploads_test.go b/workhorse/internal/upload/uploads_test.go index 29282a6f88d..d77d73b5f48 100644 --- a/workhorse/internal/upload/uploads_test.go +++ b/workhorse/internal/upload/uploads_test.go @@ -366,26 +366,28 @@ func TestInvalidFileNames(t *testing.T) { } func TestUploadHandlerRemovingExif(t *testing.T) { - tempPath, err := ioutil.TempDir("", "uploads") - require.NoError(t, err) - defer os.RemoveAll(tempPath) - - var buffer bytes.Buffer - content, err := ioutil.ReadFile("exif/testdata/sample_exif.jpg") require.NoError(t, err) - writer := multipart.NewWriter(&buffer) - file, err := writer.CreateFormFile("file", "test.jpg") - require.NoError(t, err) + runUploadTest(t, content, "sample_exif.jpg", 200, func(w http.ResponseWriter, r *http.Request) { + err := r.ParseMultipartForm(100000) + require.NoError(t, err) - _, err = file.Write(content) - require.NoError(t, err) + size, err := strconv.Atoi(r.FormValue("file.size")) + require.NoError(t, err) + require.True(t, size < len(content), "Expected the file to be smaller after removal of exif") + require.True(t, size > 0, "Expected to receive not empty file") - err = writer.Close() + w.WriteHeader(200) + fmt.Fprint(w, "RESPONSE") + }) +} + +func TestUploadHandlerRemovingExifTiff(t *testing.T) { + content, err := ioutil.ReadFile("exif/testdata/sample_exif.tiff") require.NoError(t, err) - ts := testhelper.TestServerWithHandler(regexp.MustCompile(`/url/path\z`), func(w http.ResponseWriter, r *http.Request) { + runUploadTest(t, content, "sample_exif.tiff", 200, func(w http.ResponseWriter, r *http.Request) { err := r.ParseMultipartForm(100000) require.NoError(t, err) @@ -397,30 +399,36 @@ func TestUploadHandlerRemovingExif(t *testing.T) { w.WriteHeader(200) fmt.Fprint(w, "RESPONSE") }) - defer ts.Close() +} - httpRequest, err := http.NewRequest("POST", ts.URL+"/url/path", &buffer) +func TestUploadHandlerRemovingExifInvalidContentType(t *testing.T) { + content, err := ioutil.ReadFile("exif/testdata/sample_exif_invalid.jpg") require.NoError(t, err) - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() + runUploadTest(t, content, "sample_exif_invalid.jpg", 200, func(w http.ResponseWriter, r *http.Request) { + err := r.ParseMultipartForm(100000) + require.NoError(t, err) - httpRequest = httpRequest.WithContext(ctx) - httpRequest.ContentLength = int64(buffer.Len()) - httpRequest.Header.Set("Content-Type", writer.FormDataContentType()) - response := httptest.NewRecorder() + output, err := ioutil.ReadFile(r.FormValue("file.path")) + require.NoError(t, err) + require.Equal(t, content, output, "Expected the file to be same as before") - handler := newProxy(ts.URL) - apiResponse := &api.Response{TempPath: tempPath} - preparer := &DefaultPreparer{} - opts, _, err := preparer.Prepare(apiResponse) + w.WriteHeader(200) + fmt.Fprint(w, "RESPONSE") + }) +} + +func TestUploadHandlerRemovingExifCorruptedFile(t *testing.T) { + content, err := ioutil.ReadFile("exif/testdata/sample_exif_corrupted.jpg") require.NoError(t, err) - HandleFileUploads(response, httpRequest, handler, apiResponse, &testFormProcessor{}, opts) - require.Equal(t, 200, response.Code) + runUploadTest(t, content, "sample_exif_corrupted.jpg", 422, func(w http.ResponseWriter, r *http.Request) { + err := r.ParseMultipartForm(100000) + require.Error(t, err) + }) } -func TestUploadHandlerRemovingInvalidExif(t *testing.T) { +func runUploadTest(t *testing.T, image []byte, filename string, httpCode int, tsHandler func(http.ResponseWriter, *http.Request)) { tempPath, err := ioutil.TempDir("", "uploads") require.NoError(t, err) defer os.RemoveAll(tempPath) @@ -428,17 +436,16 @@ func TestUploadHandlerRemovingInvalidExif(t *testing.T) { var buffer bytes.Buffer writer := multipart.NewWriter(&buffer) - file, err := writer.CreateFormFile("file", "test.jpg") + file, err := writer.CreateFormFile("file", filename) + require.NoError(t, err) + + _, err = file.Write(image) require.NoError(t, err) - fmt.Fprint(file, "this is not valid image data") err = writer.Close() require.NoError(t, err) - ts := testhelper.TestServerWithHandler(regexp.MustCompile(`/url/path\z`), func(w http.ResponseWriter, r *http.Request) { - err := r.ParseMultipartForm(100000) - require.Error(t, err) - }) + ts := testhelper.TestServerWithHandler(regexp.MustCompile(`/url/path\z`), tsHandler) defer ts.Close() httpRequest, err := http.NewRequest("POST", ts.URL+"/url/path", &buffer) @@ -459,7 +466,7 @@ func TestUploadHandlerRemovingInvalidExif(t *testing.T) { require.NoError(t, err) HandleFileUploads(response, httpRequest, handler, apiResponse, &testFormProcessor{}, opts) - require.Equal(t, 422, response.Code) + require.Equal(t, httpCode, response.Code) } func newProxy(url string) *proxy.Proxy { |