fix confusing code for http.Header handling (#4623)

Fixed header-to-metadat extraction. The extractMetadataFromHeader function should return an error if the http.Header contains a non-canonicalized key. The reason is that the keys can be manually set (through a map access) which can lead to ugly bugs.
Also fixed header-to-metadata extraction. Return a InternalError if a non-canonicalized key is found in a http.Header. Also log the error.

cmd/bucket-handlers.go

@@ -525,7 +525,12 @@ func (api objectAPIHandlers) PostPolicyBucketHandler(w http.ResponseWriter, r *h
 	}
 
 	// Extract metadata to be saved from received Form.
-	metadata := extractMetadataFromForm(formValues)
+	metadata, err := extractMetadataFromHeader(formValues)
+	if err != nil {
+		errorIf(err, "found invalid http request header")
+		writeErrorResponse(w, ErrInternalError, r.URL)
+		return
+	}
 	sha256sum := ""
 
 	objectLock := globalNSMutex.NewNSLock(bucket, object)

cmd/gateway-handlers.go

@@ -231,7 +231,12 @@ func (api gatewayAPIHandlers) PutObjectHandler(w http.ResponseWriter, r *http.Re
 	}
 
 	// Extract metadata to be saved from incoming HTTP header.
-	metadata := extractMetadataFromHeader(r.Header)
+	metadata, err := extractMetadataFromHeader(r.Header)
+	if err != nil {
+		errorIf(err, "found invalid http request header")
+		writeErrorResponse(w, ErrInternalError, r.URL)
+		return
+	}
 	if reqAuthType == authTypeStreamingSigned {
 		if contentEncoding, ok := metadata["content-encoding"]; ok {
 			contentEncoding = trimAwsChunkedContentEncoding(contentEncoding)

cmd/handler-utils.go

@@ -98,9 +98,9 @@ func path2BucketAndObject(path string) (bucket, object string) {
 }
 
 // extractMetadataFromHeader extracts metadata from HTTP header.
-func extractMetadataFromHeader(header http.Header) map[string]string {
+func extractMetadataFromHeader(header http.Header) (map[string]string, error) {
 	if header == nil {
-		return nil
+		return nil, traceError(errInvalidArgument)
 	}
 	metadata := make(map[string]string)
 	// Save standard supported headers.
@@ -116,16 +116,17 @@ func extractMetadataFromHeader(header http.Header) map[string]string {
 	}
 	// Go through all other headers for any additional headers that needs to be saved.
 	for key := range header {
-		cKey := http.CanonicalHeaderKey(key)
+		if key != http.CanonicalHeaderKey(key) {
-		if strings.HasPrefix(cKey, "X-Amz-Meta-") {
+			return nil, traceError(errInvalidArgument)
-			metadata[cKey] = header.Get(key)
+		}
-		} else if strings.HasPrefix(key, "X-Minio-Meta-") {
+		if strings.HasPrefix(key, "X-Amz-Meta-") {
-			metadata[cKey] = header.Get(key)
+			metadata[key] = header.Get(key)
+		}
+		if strings.HasPrefix(key, "X-Minio-Meta-") {
+			metadata[key] = header.Get(key)
 		}
 	}
-
+	return metadata, nil
-	// Success.
-	return metadata
 }
 
 // The Query string for the redirect URL the client is
@@ -168,11 +169,6 @@ func trimAwsChunkedContentEncoding(contentEnc string) (trimmedContentEnc string)
 	return strings.Join(newEncs, ",")
 }
 
-// extractMetadataFromForm extracts metadata from Post Form.
-func extractMetadataFromForm(formValues http.Header) map[string]string {
-	return extractMetadataFromHeader(formValues)
-}
-
 // Validate form field size for s3 specification requirement.
 func validateFormFieldSize(formValues http.Header) error {
 	// Iterate over form values

cmd/handler-utils_test.go

@@ -123,8 +123,9 @@ func TestValidateFormFieldSize(t *testing.T) {
 // Tests validate metadata extraction from http headers.
 func TestExtractMetadataHeaders(t *testing.T) {
 	testCases := []struct {
-		header   http.Header
+		header     http.Header
-		metadata map[string]string
+		metadata   map[string]string
+		shouldFail bool
 	}{
 		// Validate if there a known 'content-type'.
 		{
@@ -134,15 +135,17 @@ func TestExtractMetadataHeaders(t *testing.T) {
 			metadata: map[string]string{
 				"content-type": "image/png",
 			},
+			shouldFail: false,
 		},
 		// Validate if there are no keys to extract.
 		{
 			header: http.Header{
-				"test-1": []string{"123"},
+				"Test-1": []string{"123"},
 			},
-			metadata: map[string]string{},
+			metadata:   map[string]string{},
+			shouldFail: false,
 		},
-		// Validate if there are no keys to extract.
+		// Validate that there are all headers extracted
 		{
 			header: http.Header{
 				"X-Amz-Meta-Appid":   []string{"amz-meta"},
@@ -150,19 +153,38 @@ func TestExtractMetadataHeaders(t *testing.T) {
 			},
 			metadata: map[string]string{
 				"X-Amz-Meta-Appid":   "amz-meta",
-				"X-Minio-Meta-Appid": "minio-meta"},
+				"X-Minio-Meta-Appid": "minio-meta",
+			},
+			shouldFail: false,
+		},
+		// Fail if header key is not in canonicalized form
+		{
+			header: http.Header{
+				"x-amz-meta-appid": []string{"amz-meta"},
+			},
+			metadata: map[string]string{
+				"X-Amz-Meta-Appid": "amz-meta",
+			},
+			shouldFail: true,
 		},
 		// Empty header input returns empty metadata.
 		{
-			header:   nil,
+			header:     nil,
-			metadata: nil,
+			metadata:   nil,
+			shouldFail: true,
 		},
 	}
 
 	// Validate if the extracting headers.
 	for i, testCase := range testCases {
-		metadata := extractMetadataFromHeader(testCase.header)
+		metadata, err := extractMetadataFromHeader(testCase.header)
-		if !reflect.DeepEqual(metadata, testCase.metadata) {
+		if err != nil && !testCase.shouldFail {
+			t.Fatalf("Test %d failed to extract metadata: %v", i+1, err)
+		}
+		if err == nil && testCase.shouldFail {
+			t.Fatalf("Test %d should fail, but it passed", i+1)
+		}
+		if err == nil && !reflect.DeepEqual(metadata, testCase.metadata) {
 			t.Fatalf("Test %d failed: Expected \"%#v\", got \"%#v\"", i+1, testCase.metadata, metadata)
 		}
 	}

cmd/object-handlers.go

@@ -260,7 +260,7 @@ func (api objectAPIHandlers) HeadObjectHandler(w http.ResponseWriter, r *http.Re
 
 // Extract metadata relevant for an CopyObject operation based on conditional
 // header values specified in X-Amz-Metadata-Directive.
-func getCpObjMetadataFromHeader(header http.Header, defaultMeta map[string]string) map[string]string {
+func getCpObjMetadataFromHeader(header http.Header, defaultMeta map[string]string) (map[string]string, error) {
 	// if x-amz-metadata-directive says REPLACE then
 	// we extract metadata from the input headers.
 	if isMetadataReplace(header) {
@@ -270,11 +270,11 @@ func getCpObjMetadataFromHeader(header http.Header, defaultMeta map[string]strin
 	// if x-amz-metadata-directive says COPY then we
 	// return the default metadata.
 	if isMetadataCopy(header) {
-		return defaultMeta
+		return defaultMeta, nil
 	}
 
 	// Copy is default behavior if not x-amz-metadata-directive is set.
-	return defaultMeta
+	return defaultMeta, nil
 }
 
 // CopyObjectHandler - Copy Object
@@ -363,7 +363,11 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re
 	// Make sure to remove saved etag, CopyObject calculates a new one.
 	delete(defaultMeta, "etag")
 
-	newMetadata := getCpObjMetadataFromHeader(r.Header, defaultMeta)
+	newMetadata, err := getCpObjMetadataFromHeader(r.Header, defaultMeta)
+	if err != nil {
+		errorIf(err, "found invalid http request header")
+		writeErrorResponse(w, ErrInternalError, r.URL)
+	}
 	// Check if x-amz-metadata-directive was not set to REPLACE and source,
 	// desination are same objects.
 	if !isMetadataReplace(r.Header) && cpSrcDstSame {
@@ -457,7 +461,12 @@ func (api objectAPIHandlers) PutObjectHandler(w http.ResponseWriter, r *http.Req
 	}
 
 	// Extract metadata to be saved from incoming HTTP header.
-	metadata := extractMetadataFromHeader(r.Header)
+	metadata, err := extractMetadataFromHeader(r.Header)
+	if err != nil {
+		errorIf(err, "found invalid http request header")
+		writeErrorResponse(w, ErrInternalError, r.URL)
+		return
+	}
 	if rAuthType == authTypeStreamingSigned {
 		if contentEncoding, ok := metadata["content-encoding"]; ok {
 			contentEncoding = trimAwsChunkedContentEncoding(contentEncoding)
@@ -579,7 +588,12 @@ func (api objectAPIHandlers) NewMultipartUploadHandler(w http.ResponseWriter, r
 	}
 
 	// Extract metadata that needs to be saved.
-	metadata := extractMetadataFromHeader(r.Header)
+	metadata, err := extractMetadataFromHeader(r.Header)
+	if err != nil {
+		errorIf(err, "found invalid http request header")
+		writeErrorResponse(w, ErrInternalError, r.URL)
+		return
+	}
 
 	uploadID, err := objectAPI.NewMultipartUpload(bucket, object, metadata)
 	if err != nil {

cmd/web-handlers.go

@@ -497,7 +497,12 @@ func (web *webAPIHandlers) Upload(w http.ResponseWriter, r *http.Request) {
 	}
 
 	// Extract incoming metadata if any.
-	metadata := extractMetadataFromHeader(r.Header)
+	metadata, err := extractMetadataFromHeader(r.Header)
+	if err != nil {
+		errorIf(err, "found invalid http request header")
+		writeErrorResponse(w, ErrInternalError, r.URL)
+		return
+	}
 
 	// Lock the object.
 	objectLock := globalNSMutex.NewNSLock(bucket, object)