Merge branch 'mk/strict-filetype-check' into 'master'

Use strict filetype checks when calling `gm`

See merge request gitlab-org/gitlab-workhorse!564

changelogs/unreleased/mk-strict-filetype-check.yml

+---
+title: 'Experimental: Use strict content checks when resizing images'
+merge_request: 564
+author:
+type: changed

internal/imageresizer/image_resizer.go

@@ -20,8 +20,6 @@ import (
 
 	"gitlab.com/gitlab-org/labkit/tracing"
 
-	"github.com/sirupsen/logrus"
-
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/senddata"
 )
@@ -31,9 +29,9 @@ type resizer struct{ senddata.Prefix }
 var SendScaledImage = &resizer{"send-scaled-img:"}
 
 type resizeParams struct {
-	Location string
-	Width    uint
-	Format   string
+	Location    string
+	ContentType string
+	Width       uint
 }
 
 type processCounter struct {
@@ -111,26 +109,45 @@ func (r *resizer) Inject(w http.ResponseWriter, req *http.Request, paramsData st
 	}
 	defer sourceImageReader.Close()
 
-	// Past this point we attempt to rescale the image; if this should fail for any reason, we
+	logFields := func(bytesWritten int64) *log.Fields {
+		return &log.Fields{
+			"bytes_written":     bytesWritten,
+			"duration_s":        time.Since(start).Seconds(),
+			"target_width":      params.Width,
+			"content_type":      params.ContentType,
+			"original_filesize": filesize,
+		}
+	}
+
+	// We first attempt to rescale the image; if this should fail for any reason, we
 	// simply fail over to rendering out the original image unchanged.
-	imageReader, resizeCmd := tryResizeImage(req.Context(), sourceImageReader, params.Width, logger)
+	imageReader, resizeCmd, err := tryResizeImage(req, sourceImageReader, logger.Writer(), params)
+	if err != nil {
+		// something failed, but we can still write out the original image, do don't return early
+		helper.LogErrorWithFields(req, err, *logFields(0))
+	}
 	defer helper.CleanUpProcessGroup(resizeCmd)
 	imageResizeCompleted.Inc()
 
 	w.Header().Del("Content-Length")
 	bytesWritten, err := io.Copy(w, imageReader)
 	if err != nil {
-		helper.Fail500(w, req, err)
-		return
+		handleFailedCommand(w, req, bytesWritten, err, logFields(bytesWritten))
+	} else if err = resizeCmd.Wait(); err != nil {
+		handleFailedCommand(w, req, bytesWritten, err, logFields(bytesWritten))
+	} else {
+		logger.WithFields(*logFields(bytesWritten)).Printf("ImageResizer: Success")
 	}
+}
 
-	logger.WithFields(log.Fields{
-		"bytes_written":     bytesWritten,
-		"duration_s":        time.Since(start).Seconds(),
-		"target_width":      params.Width,
-		"format":            params.Format,
-		"original_filesize": filesize,
-	}).Printf("ImageResizer: Success")
+func handleFailedCommand(w http.ResponseWriter, req *http.Request, bytesWritten int64, err error, logFields *log.Fields) {
+	if err != nil {
+		if bytesWritten <= 0 {
+			helper.Fail500(w, req, err)
+		} else {
+			helper.LogErrorWithFields(req, err, *logFields)
+		}
+	}
 }
 
 func (r *resizer) unpackParameters(paramsData string) (*resizeParams, error) {
@@ -143,30 +160,51 @@ func (r *resizer) unpackParameters(paramsData string) (*resizeParams, error) {
 		return nil, fmt.Errorf("ImageResizer: Location is empty")
 	}
 
+	if params.ContentType == "" {
+		return nil, fmt.Errorf("ImageResizer: Image MIME type must be set")
+	}
+
 	return &params, nil
 }
 
 // Attempts to rescale the given image data, or in case of errors, falls back to the original image.
-func tryResizeImage(ctx context.Context, r io.Reader, width uint, logger *logrus.Entry) (io.Reader, *exec.Cmd) {
+func tryResizeImage(req *http.Request, r io.Reader, errorWriter io.Writer, params *resizeParams) (io.Reader, *exec.Cmd, error) {
 	if !numScalerProcs.tryIncrement() {
-		return r, nil
+		return r, nil, fmt.Errorf("ImageResizer: too many running scaler processes")
 	}
 
+	ctx := req.Context()
 	go func() {
 		<-ctx.Done()
 		numScalerProcs.decrement()
 	}()
 
-	resizeCmd, resizedImageReader, err := startResizeImageCommand(ctx, r, logger.Writer(), width)
+	width := params.Width
+	gmFileSpec := determineFilePrefix(params.ContentType)
+	if gmFileSpec == "" {
+		return r, nil, fmt.Errorf("ImageResizer: unexpected MIME type: %s", params.ContentType)
+	}
+
+	resizeCmd, resizedImageReader, err := startResizeImageCommand(ctx, r, errorWriter, width, gmFileSpec)
 	if err != nil {
-		logger.WithError(err).Error("ImageResizer: failed forking into graphicsmagick")
-		return r, nil
+		return r, nil, fmt.Errorf("ImageResizer: failed forking into graphicsmagick")
+	}
+	return resizedImageReader, resizeCmd, nil
+}
+
+func determineFilePrefix(contentType string) string {
+	switch contentType {
+	case "image/png":
+		return "png:"
+	case "image/jpeg":
+		return "jpg:"
+	default:
+		return ""
 	}
-	return resizedImageReader, resizeCmd
 }
 
-func startResizeImageCommand(ctx context.Context, imageReader io.Reader, errorWriter io.Writer, width uint) (*exec.Cmd, io.ReadCloser, error) {
-	cmd := exec.CommandContext(ctx, "gm", "convert", "-resize", fmt.Sprintf("%dx", width), "-", "-")
+func startResizeImageCommand(ctx context.Context, imageReader io.Reader, errorWriter io.Writer, width uint, gmFileSpec string) (*exec.Cmd, io.ReadCloser, error) {
+	cmd := exec.CommandContext(ctx, "gm", "convert", "-resize", fmt.Sprintf("%dx", width), gmFileSpec+"-", "-")
 	cmd.Stdin = imageReader
 	cmd.Stderr = errorWriter
 	cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}

internal/imageresizer/image_resizer_test.go

+package imageresizer
+
+import (
+	"encoding/base64"
+	"encoding/json"
+	"net/http"
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+var r = resizer{}
+
+func TestUnpackParametersReturnsParamsInstanceForValidInput(t *testing.T) {
+	inParams := resizeParams{Location: "/path/to/img", Width: 64, ContentType: "image/png"}
+
+	outParams, err := r.unpackParameters(encodeParams(t, &inParams))
+
+	require.NoError(t, err, "unexpected error when unpacking params")
+	require.Equal(t, inParams, *outParams)
+}
+
+func TestUnpackParametersReturnsErrorWhenLocationBlank(t *testing.T) {
+	inParams := resizeParams{Location: "", Width: 64, ContentType: "image/jpg"}
+
+	_, err := r.unpackParameters(encodeParams(t, &inParams))
+
+	require.Error(t, err, "expected error when Location is blank")
+}
+
+func TestUnpackParametersReturnsErrorWhenContentTypeBlank(t *testing.T) {
+	inParams := resizeParams{Location: "/path/to/img", Width: 64, ContentType: ""}
+
+	_, err := r.unpackParameters(encodeParams(t, &inParams))
+
+	require.Error(t, err, "expected error when ContentType is blank")
+}
+
+func TestDetermineFilePrefixFromMimeType(t *testing.T) {
+	require.Equal(t, "png:", determineFilePrefix("image/png"))
+	require.Equal(t, "jpg:", determineFilePrefix("image/jpeg"))
+	require.Equal(t, "", determineFilePrefix("unsupported"))
+}
+
+func TestTryResizeImageSuccess(t *testing.T) {
+	inParams := resizeParams{Location: "/path/to/img", Width: 64, ContentType: "image/png"}
+	inFile := testImage(t)
+	req, err := http.NewRequest("GET", "/foo", nil)
+	require.NoError(t, err)
+
+	reader, cmd, err := tryResizeImage(req, inFile, os.Stderr, &inParams)
+
+	require.NoError(t, err)
+	require.NotNil(t, cmd)
+	require.NotNil(t, reader)
+	require.NotEqual(t, inFile, reader)
+}
+
+func TestTryResizeImageFailsOverToOriginalImageWhenContentTypeNotSupported(t *testing.T) {
+	inParams := resizeParams{Location: "/path/to/img", Width: 64, ContentType: "not supported"}
+	inFile := testImage(t)
+	req, err := http.NewRequest("GET", "/foo", nil)
+	require.NoError(t, err)
+
+	reader, cmd, err := tryResizeImage(req, inFile, os.Stderr, &inParams)
+
+	require.Error(t, err)
+	require.Nil(t, cmd)
+	require.Equal(t, inFile, reader)
+}
+
+func TestGraphicsMagickFailsWhenContentTypeNotMatchingFileContents(t *testing.T) {
+	inParams := resizeParams{Location: "/path/to/img", Width: 64, ContentType: "image/jpeg"}
+	inFile := testImage(t) // this is PNG file; gm should fail fast in this case
+	req, err := http.NewRequest("GET", "/foo", nil)
+	require.NoError(t, err)
+
+	_, cmd, err := tryResizeImage(req, inFile, os.Stderr, &inParams)
+
+	require.NoError(t, err)
+	require.Error(t, cmd.Wait())
+}
+
+// The Rails applications sends a Base64 encoded JSON string carrying
+// these parameters in an HTTP response header
+func encodeParams(t *testing.T, p *resizeParams) string {
+	json, err := json.Marshal(*p)
+	if err != nil {
+		require.NoError(t, err, "JSON encoder encountered unexpected error")
+	}
+	return base64.StdEncoding.EncodeToString(json)
+}
+
+func testImage(t *testing.T) *os.File {
+	f, err := os.Open("../../testdata/image.png")
+	require.NoError(t, err)
+	return f
+}

main_test.go

@@ -372,7 +372,8 @@ func TestArtifactsGetSingleFile(t *testing.T) {
 func TestImageResizing(t *testing.T) {
 	imageLocation := `testdata/image.png`
 	requestedWidth := 40
-	jsonParams := fmt.Sprintf(`{"Location":"%s","Width":%d}`, imageLocation, requestedWidth)
+	imageFormat := "image/png"
+	jsonParams := fmt.Sprintf(`{"Location":"%s","Width":%d, "ContentType":"%s"}`, imageLocation, requestedWidth, imageFormat)
 	resourcePath := "/uploads/-/system/user/avatar/123/avatar.png?width=40"
 
 	resp, body, err := doSendDataRequest(resourcePath, "send-scaled-img", jsonParams)