Merge branch '319-img-scaler-use-detected-file-format' into 'master'

Use detected file format for image scaler

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

changelogs/unreleased/319-img-scaler-use-detected-file-format.yml

+---
+title: Don't reject image scaling requests based on file extension/format mismatch
+merge_request: 651
+author:
+type: changed

cmd/gitlab-resize-image/main.go

@@ -3,7 +3,6 @@ package main
 import (
 	"fmt"
 	"image"
-	"mime"
 	"os"
 	"strconv"
 
@@ -23,23 +22,16 @@ func _main() error {
 	if err != nil {
 		return fmt.Errorf("GL_RESIZE_IMAGE_WIDTH: %w", err)
 	}
-	contentType := os.Getenv("GL_RESIZE_IMAGE_CONTENT_TYPE")
-	if contentType == "" {
-		return fmt.Errorf("GL_RESIZE_IMAGE_CONTENT_TYPE is empty")
-	}
 
-	src, extension, err := image.Decode(os.Stdin)
+	src, formatName, err := image.Decode(os.Stdin)
 	if err != nil {
 		return fmt.Errorf("decode: %w", err)
 	}
-	if detectedType := mime.TypeByExtension("." + extension); detectedType != contentType {
-		return fmt.Errorf("MIME types do not match; requested: %s; actual: %s", contentType, detectedType)
-	}
-	format, err := imaging.FormatFromExtension(extension)
+	imagingFormat, err := imaging.FormatFromExtension(formatName)
 	if err != nil {
 		return fmt.Errorf("find imaging format: %w", err)
 	}
 
 	image := imaging.Resize(src, requestedWidth, 0, imaging.Lanczos)
-	return imaging.Encode(os.Stdout, image, format)
+	return imaging.Encode(os.Stdout, image, imagingFormat)
 }

internal/imageresizer/image_resizer.go

 package imageresizer
 
 import (
+	"bufio"
 	"context"
 	"fmt"
 	"io"
+	"math"
 	"net"
 	"net/http"
 	"os"
@@ -14,15 +16,12 @@ import (
 	"syscall"
 	"time"
 
-	"gitlab.com/gitlab-org/gitlab-workhorse/internal/config"
-
 	"github.com/prometheus/client_golang/prometheus"
-
 	"gitlab.com/gitlab-org/labkit/correlation"
 	"gitlab.com/gitlab-org/labkit/log"
-
 	"gitlab.com/gitlab-org/labkit/tracing"
 
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/config"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/senddata"
 )
@@ -131,6 +130,12 @@ var (
 	)
 )
 
+const (
+	jpegMagic   = "\xff\xd8"          // 2 bytes
+	pngMagic    = "\x89PNG\r\n\x1a\n" // 8 bytes
+	maxMagicLen = 8                   // 8 first bytes is enough to detect PNG or JPEG
+)
+
 func init() {
 	prometheus.MustRegister(imageResizeConcurrencyLimitExceeds)
 	prometheus.MustRegister(imageResizeProcesses)
@@ -270,9 +275,22 @@ func (r *Resizer) tryResizeImage(req *http.Request, reader io.Reader, errorWrite
 		r.numScalerProcs.decrement()
 	}()
 
-	resizeCmd, resizedImageReader, err := startResizeImageCommand(ctx, reader, errorWriter, params)
+	// Prevents EOF if the file is smaller than 8 bytes
+	bufferSize := int(math.Min(maxMagicLen, float64(fileSize)))
+	buffered := bufio.NewReaderSize(reader, bufferSize)
+
+	data, err := buffered.Peek(bufferSize)
+	if err != nil {
+		return buffered, nil, fmt.Errorf("ImageResizer: failed to peek into data: %v", err)
+	}
+
+	if string(data) != pngMagic && string(data[0:2]) != jpegMagic {
+		return buffered, nil, fmt.Errorf("ImageResizer: format is prohibited")
+	}
+
+	resizeCmd, resizedImageReader, err := startResizeImageCommand(ctx, buffered, errorWriter, params)
 	if err != nil {
-		return reader, nil, fmt.Errorf("ImageResizer: failed forking into scaler process: %w", err)
+		return buffered, nil, fmt.Errorf("ImageResizer: failed forking into scaler process: %w", err)
 	}
 	return resizedImageReader, resizeCmd, nil
 }
@@ -284,7 +302,6 @@ func startResizeImageCommand(ctx context.Context, imageReader io.Reader, errorWr
 	cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
 	cmd.Env = []string{
 		"GL_RESIZE_IMAGE_WIDTH=" + strconv.Itoa(int(params.Width)),
-		"GL_RESIZE_IMAGE_CONTENT_TYPE=" + params.ContentType,
 	}
 	cmd.Env = envInjector(ctx, cmd.Env)
 

internal/imageresizer/image_resizer_test.go

@@ -5,18 +5,19 @@ import (
 	"encoding/json"
 	"image"
 	"image/png"
+	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"os"
 	"testing"
 
-	"gitlab.com/gitlab-org/gitlab-workhorse/internal/config"
-
-	"gitlab.com/gitlab-org/labkit/log"
-
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/labkit/log"
 
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/config"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/testhelper"
+
+	_ "image/jpeg" // need this for image.Decode with JPEG
 )
 
 const imagePath = "../../testdata/image.png"
@@ -41,13 +42,40 @@ func requestScaledImage(t *testing.T, httpHeaders http.Header, params resizePara
 
 func TestRequestScaledImageFromPath(t *testing.T) {
 	cfg := config.DefaultImageResizerConfig
-	params := resizeParams{Location: imagePath, ContentType: "image/png", Width: 64}
 
-	resp := requestScaledImage(t, nil, params, cfg)
-	require.Equal(t, http.StatusOK, resp.StatusCode)
+	testCases := []struct {
+		desc        string
+		imagePath   string
+		contentType string
+	}{
+		{
+			desc:        "PNG",
+			imagePath:   imagePath,
+			contentType: "image/png",
+		},
+		{
+			desc:        "JPEG",
+			imagePath:   "../../testdata/image.jpg",
+			contentType: "image/jpeg",
+		},
+		{
+			desc:        "JPEG < 1kb",
+			imagePath:   "../../testdata/image_single_pixel.jpg",
+			contentType: "image/jpeg",
+		},
+	}
 
-	bounds := imageFromResponse(t, resp).Bounds()
-	require.Equal(t, int(params.Width), bounds.Size().X, "wrong width after resizing")
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			params := resizeParams{Location: tc.imagePath, ContentType: tc.contentType, Width: 64}
+
+			resp := requestScaledImage(t, nil, params, cfg)
+			require.Equal(t, http.StatusOK, resp.StatusCode)
+
+			bounds := imageFromResponse(t, resp).Bounds()
+			require.Equal(t, int(params.Width), bounds.Size().X, "wrong width after resizing")
+		})
+	}
 }
 
 func TestServeOriginalImageWhenSourceImageTooLarge(t *testing.T) {
@@ -70,12 +98,14 @@ func TestFailFastOnOpenStreamFailure(t *testing.T) {
 	require.Equal(t, http.StatusInternalServerError, resp.StatusCode)
 }
 
-func TestFailFastOnContentTypeMismatch(t *testing.T) {
+func TestIgnoreContentTypeMismatchIfImageFormatIsAllowed(t *testing.T) {
 	cfg := config.DefaultImageResizerConfig
 	params := resizeParams{Location: imagePath, ContentType: "image/jpeg", Width: 64}
 	resp := requestScaledImage(t, nil, params, cfg)
+	require.Equal(t, http.StatusOK, resp.StatusCode)
 
-	require.Equal(t, http.StatusInternalServerError, resp.StatusCode)
+	bounds := imageFromResponse(t, resp).Bounds()
+	require.Equal(t, int(params.Width), bounds.Size().X, "wrong width after resizing")
 }
 
 func TestUnpackParametersReturnsParamsInstanceForValidInput(t *testing.T) {
@@ -106,6 +136,24 @@ func TestUnpackParametersReturnsErrorWhenContentTypeBlank(t *testing.T) {
 	require.Error(t, err, "expected error when ContentType is blank")
 }
 
+func TestServeOriginalImageWhenSourceImageFormatIsNotAllowed(t *testing.T) {
+	cfg := config.DefaultImageResizerConfig
+	// SVG images are not allowed to be resized
+	svgImagePath := "../../testdata/image.svg"
+	svgImage, err := ioutil.ReadFile(svgImagePath)
+	require.NoError(t, err)
+	// ContentType is no longer used to perform the format validation.
+	// To make the test more strict, we'll use allowed, but incorrect ContentType.
+	params := resizeParams{Location: svgImagePath, ContentType: "image/png", Width: 64}
+
+	resp := requestScaledImage(t, nil, params, cfg)
+	require.Equal(t, http.StatusOK, resp.StatusCode)
+
+	responseData, err := ioutil.ReadAll(resp.Body)
+	require.NoError(t, err)
+	require.Equal(t, svgImage, responseData, "expected original image")
+}
+
 // 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 {
@@ -127,7 +175,7 @@ func testImage(t *testing.T) image.Image {
 }
 
 func imageFromResponse(t *testing.T, resp *http.Response) image.Image {
-	img, err := png.Decode(resp.Body)
+	img, _, err := image.Decode(resp.Body)
 	require.NoError(t, err, "decode resized image")
 	return img
 }