Merge branch 'sh-support-encoded-content-disposition' into 'master'

Support encoded Content-Disposition fields

Closes #207

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

CHANGELOG

@@ -2,6 +2,10 @@
 
 Formerly known as 'gitlab-git-http-server'.
 
+v 8.3.0
+
+- Support encoded Content-Disposition fields !360
+
 v 8.2.0
 
 - Sign LFS upload requests that have been handled by workhorse

internal/headers/content_headers.go

 package headers
 
 import (
-	"mime"
 	"net/http"
 	"regexp"
 
@@ -17,6 +16,7 @@ var (
 	VideoTypeRegex = regexp.MustCompile(`^video/*`)
 
 	AttachmentRegex = regexp.MustCompile(`^attachment`)
+	InlineRegex     = regexp.MustCompile(`^inline`)
 )
 
 // Mime types that can't be inlined. Usually subtypes of main types
@@ -54,61 +54,52 @@ func safeContentType(data []byte) string {
 }
 
 func safeContentDisposition(contentType string, contentDisposition string) string {
-	existingDisposition, file := extractContentDispositionFile(contentDisposition)
-
 	// If the existing disposition is attachment we return that. This allow us
 	// to force a download from GitLab (ie: RawController)
-	if AttachmentRegex.MatchString(existingDisposition) {
-		return attachmentDisposition(file)
+	if AttachmentRegex.MatchString(contentDisposition) {
+		return contentDisposition
 	}
 
 	// Checks for mime types that are forbidden to be inline
 	for _, element := range forbiddenInlineTypes {
 		if isType(contentType, element) {
-			return attachmentDisposition(file)
+			return attachmentDisposition(contentDisposition)
 		}
 	}
 
 	// Checks for mime types allowed to be inline
 	for _, element := range allowedInlineTypes {
 		if isType(contentType, element) {
-			return inlineDisposition(file)
+			return inlineDisposition(contentDisposition)
 		}
 	}
 
 	// Anything else is set to attachment
-	return attachmentDisposition(file)
+	return attachmentDisposition(contentDisposition)
 }
 
-func extractContentDispositionFile(disposition string) (string, string) {
-	if disposition == "" {
-		return "", ""
+func attachmentDisposition(contentDisposition string) string {
+	if contentDisposition == "" {
+		return "attachment"
 	}
 
-	existingDisposition, params, err := mime.ParseMediaType(disposition)
-	if err != nil {
-		return "", ""
+	if InlineRegex.MatchString(contentDisposition) {
+		return InlineRegex.ReplaceAllString(contentDisposition, "attachment")
 	}
 
-	return existingDisposition, params["filename"]
-}
-
-func attachmentDisposition(file string) string {
-	return disposition("attachment", file)
+	return contentDisposition
 }
 
-func inlineDisposition(file string) string {
-	return disposition("inline", file)
-}
-
-func disposition(disposition string, file string) string {
-	params := map[string]string{}
+func inlineDisposition(contentDisposition string) string {
+	if contentDisposition == "" {
+		return "inline"
+	}
 
-	if file != "" {
-		params["filename"] = file
+	if AttachmentRegex.MatchString(contentDisposition) {
+		return AttachmentRegex.ReplaceAllString(contentDisposition, "inline")
 	}
 
-	return mime.FormatMediaType(disposition, params)
+	return contentDisposition
 }
 
 func isType(contentType string, mimeType *regexp.Regexp) bool {

internal/senddata/contentprocessor/contentprocessor_test.go

@@ -21,7 +21,7 @@ func TestFailSetContentTypeAndDisposition(t *testing.T) {
 		require.NoError(t, err)
 	})
 
-	resp := makeRequest(t, h, testCaseBody)
+	resp := makeRequest(t, h, testCaseBody, "")
 
 	require.Equal(t, "", resp.Header.Get(headers.ContentDispositionHeader))
 	require.Equal(t, "", resp.Header.Get(headers.ContentTypeHeader))
@@ -30,7 +30,7 @@ func TestFailSetContentTypeAndDisposition(t *testing.T) {
 func TestSuccessSetContentTypeAndDispositionFeatureEnabled(t *testing.T) {
 	testCaseBody := "Hello world!"
 
-	resp := makeRequest(t, nil, testCaseBody)
+	resp := makeRequest(t, nil, testCaseBody, "")
 
 	require.Equal(t, "inline", resp.Header.Get(headers.ContentDispositionHeader))
 	require.Equal(t, "text/plain; charset=utf-8", resp.Header.Get(headers.ContentTypeHeader))
@@ -145,11 +145,17 @@ func TestSetProperContentTypeAndDisposition(t *testing.T) {
 			contentDisposition: "attachment",
 			body:               testhelper.LoadFile(t, "testdata/file.sketch"),
 		},
+		{
+			desc:               "PDF file with non-ASCII characters in filename",
+			contentType:        "application/pdf",
+			contentDisposition: `attachment; filename="file-ä.pdf"; filename*=UTF-8''file-%c3.pdf`,
+			body:               testhelper.LoadFile(t, "testdata/file-ä.pdf"),
+		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
-			resp := makeRequest(t, nil, tc.body)
+			resp := makeRequest(t, nil, tc.body, tc.contentDisposition)
 
 			require.Equal(t, tc.contentType, resp.Header.Get(headers.ContentTypeHeader))
 			require.Equal(t, tc.contentDisposition, resp.Header.Get(headers.ContentDispositionHeader))
@@ -174,7 +180,7 @@ func TestFailOverrideContentType(t *testing.T) {
 		require.NoError(t, err)
 	})
 
-	resp := makeRequest(t, h, testCase.body)
+	resp := makeRequest(t, h, testCase.body, "")
 
 	require.Equal(t, testCase.contentType, resp.Header.Get(headers.ContentTypeHeader))
 }
@@ -190,7 +196,7 @@ func TestSuccessOverrideContentDispositionFromInlineToAttachment(t *testing.T) {
 		require.NoError(t, err)
 	})
 
-	resp := makeRequest(t, h, testCaseBody)
+	resp := makeRequest(t, h, testCaseBody, "")
 
 	require.Equal(t, "attachment", resp.Header.Get(headers.ContentDispositionHeader))
 }
@@ -206,7 +212,7 @@ func TestFailOverrideContentDispositionFromAttachmentToInline(t *testing.T) {
 		require.NoError(t, err)
 	})
 
-	resp := makeRequest(t, h, testCaseBody)
+	resp := makeRequest(t, h, testCaseBody, "")
 
 	require.Equal(t, "attachment", resp.Header.Get(headers.ContentDispositionHeader))
 }
@@ -240,11 +246,12 @@ func TestWriteHeadersCalledOnce(t *testing.T) {
 	require.Equal(t, 400, rw.status)
 }
 
-func makeRequest(t *testing.T, handler http.HandlerFunc, body string) *http.Response {
+func makeRequest(t *testing.T, handler http.HandlerFunc, body string, disposition string) *http.Response {
 	if handler == nil {
 		handler = http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
 			// We are pretending to be upstream
 			w.Header().Set(headers.GitlabWorkhorseDetectContentTypeHeader, "true")
+			w.Header().Set(headers.ContentDispositionHeader, disposition)
 			_, err := io.WriteString(w, body)
 			require.NoError(t, err)
 		})