From b0fbddc051450f7d58da19576a18e4419303a8d3 Mon Sep 17 00:00:00 2001 From: Andreas Auernhammer Date: Wed, 5 Jul 2017 16:56:10 -0700 Subject: [PATCH] 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 | 7 ++++++- cmd/gateway-handlers.go | 7 ++++++- cmd/handler-utils.go | 26 ++++++++++-------------- cmd/handler-utils_test.go | 42 +++++++++++++++++++++++++++++---------- cmd/object-handlers.go | 26 ++++++++++++++++++------ cmd/web-handlers.go | 7 ++++++- 6 files changed, 81 insertions(+), 34 deletions(-) diff --git a/cmd/bucket-handlers.go b/cmd/bucket-handlers.go index 2d35d4269..ea64530bc 100644 --- a/cmd/bucket-handlers.go +++ b/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) diff --git a/cmd/gateway-handlers.go b/cmd/gateway-handlers.go index 6d023f939..f15ef10cd 100644 --- a/cmd/gateway-handlers.go +++ b/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) diff --git a/cmd/handler-utils.go b/cmd/handler-utils.go index 7a1d80f13..e8b60f7e1 100644 --- a/cmd/handler-utils.go +++ b/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 strings.HasPrefix(cKey, "X-Amz-Meta-") { - metadata[cKey] = header.Get(key) - } else if strings.HasPrefix(key, "X-Minio-Meta-") { - metadata[cKey] = header.Get(key) + if key != http.CanonicalHeaderKey(key) { + return nil, traceError(errInvalidArgument) + } + if strings.HasPrefix(key, "X-Amz-Meta-") { + metadata[key] = header.Get(key) + } + if strings.HasPrefix(key, "X-Minio-Meta-") { + metadata[key] = header.Get(key) } } - - // Success. - return metadata + return metadata, nil } // 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 diff --git a/cmd/handler-utils_test.go b/cmd/handler-utils_test.go index 51c45adf6..40b11e733 100644 --- a/cmd/handler-utils_test.go +++ b/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 - metadata map[string]string + header http.Header + 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, - metadata: nil, + header: nil, + metadata: nil, + shouldFail: true, }, } // Validate if the extracting headers. for i, testCase := range testCases { - metadata := extractMetadataFromHeader(testCase.header) - if !reflect.DeepEqual(metadata, testCase.metadata) { + metadata, err := extractMetadataFromHeader(testCase.header) + 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) } } diff --git a/cmd/object-handlers.go b/cmd/object-handlers.go index 3a1352fdf..1da5b0dd7 100644 --- a/cmd/object-handlers.go +++ b/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 { diff --git a/cmd/web-handlers.go b/cmd/web-handlers.go index bbde076f9..dced81ba2 100644 --- a/cmd/web-handlers.go +++ b/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)