Merge pull request #3200 from lapshin-vitaly/bufix/validate-tags-on-copy

validate tags on copy object and add regex for validating tags

5 files changed, 136 insertions, 26 deletions

diff --git a/weed/s3api/s3api_object_copy_handlers.go b/weed/s3api/s3api_object_copy_handlers.go
index 6b67ef337..950e7a8fb 100644
--- a/weed/s3api/s3api_object_copy_handlers.go
+++ b/weed/s3api/s3api_object_copy_handlers.go
@@ -45,7 +45,12 @@ func (s3a *S3ApiServer) CopyObjectHandler(w http.ResponseWriter, r *http.Request
 			s3err.WriteErrorResponse(w, r, s3err.ErrInvalidCopySource)
 			return
 		}
-		entry.Extended = processMetadataBytes(r.Header, entry.Extended, replaceMeta, replaceTagging)
+		entry.Extended, err = processMetadataBytes(r.Header, entry.Extended, replaceMeta, replaceTagging)
+		if err != nil {
+			glog.Errorf("CopyObjectHandler ValidateTags error %s: %v", r.URL, err)
+			s3err.WriteErrorResponse(w, r, s3err.ErrInvalidTag)
+			return
+		}
 		err = s3a.touch(dir, name, entry)
 		if err != nil {
 			s3err.WriteErrorResponse(w, r, s3err.ErrInvalidCopySource)
@@ -252,7 +257,7 @@ func processMetadata(reqHeader, existing http.Header, replaceMeta, replaceTaggin
 	return
 }
 
-func processMetadataBytes(reqHeader http.Header, existing map[string][]byte, replaceMeta, replaceTagging bool) (metadata map[string][]byte) {
+func processMetadataBytes(reqHeader http.Header, existing map[string][]byte, replaceMeta, replaceTagging bool) (metadata map[string][]byte, err error) {
 	metadata = make(map[string][]byte)
 
 	if sc := existing[s3_constants.AmzStorageClass]; len(sc) > 0 {
@@ -277,16 +282,18 @@ func processMetadataBytes(reqHeader http.Header, existing map[string][]byte, rep
 			}
 		}
 	}
-
 	if replaceTagging {
 		if tags := reqHeader.Get(s3_constants.AmzObjectTagging); tags != "" {
-			for _, v := range strings.Split(tags, "&") {
-				tag := strings.Split(v, "=")
-				if len(tag) == 2 {
-					metadata[s3_constants.AmzObjectTagging+"-"+tag[0]] = []byte(tag[1])
-				} else if len(tag) == 1 {
-					metadata[s3_constants.AmzObjectTagging+"-"+tag[0]] = nil
-				}
+			parsedTags, err := parseTagsHeader(tags)
+			if err != nil {
+				return nil, err
+			}
+			err = ValidateTags(parsedTags)
+			if err != nil {
+				return nil, err
+			}
+			for k, v := range parsedTags {
+				metadata[s3_constants.AmzObjectTagging+"-"+k] = []byte(v)
 			}
 		}
 	} else {
diff --git a/weed/s3api/s3api_object_copy_handlers_test.go b/weed/s3api/s3api_object_copy_handlers_test.go
index 610b29a6b..29d519c24 100644
--- a/weed/s3api/s3api_object_copy_handlers_test.go
+++ b/weed/s3api/s3api_object_copy_handlers_test.go
@@ -332,6 +332,19 @@ var processMetadataBytesTestCases = []struct {
 			"X-Amz-Tagging-type": "request",
 		},
 	},
+
+	{
+		108,
+		H{
+			"User-Agent":                           "firefox",
+			"X-Amz-Meta-My-Meta":                   "request",
+			"X-Amz-Tagging":                        "A=B&a=b&type=request*",
+			s3_constants.AmzUserMetaDirective:      DirectiveReplace,
+			s3_constants.AmzObjectTaggingDirective: DirectiveReplace,
+		},
+		H{},
+		H{},
+	},
 }
 
 func TestProcessMetadata(t *testing.T) {
@@ -339,7 +352,6 @@ func TestProcessMetadata(t *testing.T) {
 		reqHeader := transferHToHeader(tc.request)
 		existing := transferHToHeader(tc.existing)
 		replaceMeta, replaceTagging := replaceDirective(reqHeader)
-
 		err := processMetadata(reqHeader, existing, replaceMeta, replaceTagging, func(_ string, _ string) (tags map[string]string, err error) {
 			return tc.getTags, nil
 		}, "", "")
@@ -367,7 +379,7 @@ func TestProcessMetadataBytes(t *testing.T) {
 		reqHeader := transferHToHeader(tc.request)
 		existing := transferHToBytesArr(tc.existing)
 		replaceMeta, replaceTagging := replaceDirective(reqHeader)
-		extends := processMetadataBytes(reqHeader, existing, replaceMeta, replaceTagging)
+		extends, _ := processMetadataBytes(reqHeader, existing, replaceMeta, replaceTagging)
 
 		result := transferBytesArrToH(extends)
 		fmtTagging(result, tc.want)
diff --git a/weed/s3api/s3api_object_tagging_handlers.go b/weed/s3api/s3api_object_tagging_handlers.go
index 9fde0309c..1791d7dc8 100644
--- a/weed/s3api/s3api_object_tagging_handlers.go
+++ b/weed/s3api/s3api_object_tagging_handlers.go
@@ -62,23 +62,12 @@ func (s3a *S3ApiServer) PutObjectTaggingHandler(w http.ResponseWriter, r *http.R
 		return
 	}
 	tags := tagging.ToTags()
-	if len(tags) > 10 {
-		glog.Errorf("PutObjectTaggingHandler tags %s: %d tags more than 10", r.URL, len(tags))
+	err = ValidateTags(tags)
+	if err != nil {
+		glog.Errorf("PutObjectTaggingHandler ValidateTags error %s: %v", r.URL, err)
 		s3err.WriteErrorResponse(w, r, s3err.ErrInvalidTag)
 		return
 	}
-	for k, v := range tags {
-		if len(k) > 128 {
-			glog.Errorf("PutObjectTaggingHandler tags %s: tag key %s longer than 128", r.URL, k)
-			s3err.WriteErrorResponse(w, r, s3err.ErrInvalidTag)
-			return
-		}
-		if len(v) > 256 {
-			glog.Errorf("PutObjectTaggingHandler tags %s: tag value %s longer than 256", r.URL, v)
-			s3err.WriteErrorResponse(w, r, s3err.ErrInvalidTag)
-			return
-		}
-	}
 
 	if err = s3a.setTags(dir, name, tagging.ToTags()); err != nil {
 		if err == filer_pb.ErrNotFound {
diff --git a/weed/s3api/tags.go b/weed/s3api/tags.go
index 979e5a80c..d49db6894 100644
--- a/weed/s3api/tags.go
+++ b/weed/s3api/tags.go
@@ -2,6 +2,9 @@ package s3api
 
 import (
 	"encoding/xml"
+	"fmt"
+	"regexp"
+	"strings"
 )
 
 type Tag struct {
@@ -37,3 +40,40 @@ func FromTags(tags map[string]string) (t *Tagging) {
 	}
 	return
 }
+
+func parseTagsHeader(tags string) (map[string]string, error) {
+	parsedTags := make(map[string]string)
+	for _, v := range strings.Split(tags, "&") {
+		tag := strings.Split(v, "=")
+		if len(tag) == 2 {
+			parsedTags[tag[0]] = tag[1]
+		} else if len(tag) == 1 {
+			parsedTags[tag[0]] = ""
+		}
+	}
+	return parsedTags, nil
+}
+
+func ValidateTags(tags map[string]string) error {
+	if len(tags) > 10 {
+		return fmt.Errorf("validate tags: %d tags more than 10", len(tags))
+	}
+	for k, v := range tags {
+		if len(k) > 128 {
+			return fmt.Errorf("validate tags: tag key longer than 128")
+		}
+		validateKey, err := regexp.MatchString(`^([\p{L}\p{Z}\p{N}_.:/=+\-@]*)$`, k)
+		if !validateKey || err != nil {
+			return fmt.Errorf("validate tags key %s error, incorrect key", k)
+		}
+		if len(v) > 256 {
+			return fmt.Errorf("validate tags: tag value longer than 256")
+		}
+		validateValue, err := regexp.MatchString(`^([\p{L}\p{Z}\p{N}_.:/=+\-@]*)$`, v)
+		if !validateValue || err != nil {
+			return fmt.Errorf("validate tags value %s error, incorrect value", v)
+		}
+	}
+
+	return nil
+}
diff --git a/weed/s3api/tags_test.go b/weed/s3api/tags_test.go
index d8beb1922..fb464fcae 100644
--- a/weed/s3api/tags_test.go
+++ b/weed/s3api/tags_test.go
@@ -50,3 +50,65 @@ func TestXMLMarshall(t *testing.T) {
 	assert.Equal(t, expected, actual)
 
 }
+
+type TestTags map[string]string
+
+var ValidateTagsTestCases = []struct {
+	testCaseID    int
+	tags          TestTags
+	wantErrString string
+}{
+	{
+		1,
+		TestTags{"key-1": "value-1"},
+		"",
+	},
+	{
+		2,
+		TestTags{"key-1": "valueOver256R59YI9bahPwAVqvLeKCvM2S1RjzgP8fNDKluCbol0XTTFY6VcMwTBmdnqjsddilXztSGfEoZS1wDAIMBA0rW0CLNSoE2zNg4TT0vDbLHEtZBoZjdZ5E0JNIAqwb9ptIk2VizYmhWjb1G4rJ0CqDGWxcy3usXaQg6Dk6kU8N4hlqwYWeGw7uqdghcQ3ScfF02nHW9QFMN7msLR5fe90mbFBBp3Tjq34i0LEr4By2vxoRa2RqdBhEJhi23Tm"},
+		"validate tags: tag value longer than 256",
+	},
+	{
+		3,
+		TestTags{"keyLenOver128a5aUUGcPexMELsz3RyROzIzfO6BKABeApH2nbbagpOxZh2MgBWYDZtFxQaCuQeP1xR7dUJLwfFfDHguVIyxvTStGDk51BemKETIwZ0zkhR7lhfHBp2y0nFnV": "value-1"},
+		"validate tags: tag key longer than 128",
+	},
+	{
+		4,
+		TestTags{"key-1*": "value-1"},
+		"validate tags key key-1* error, incorrect key",
+	},
+	{
+		5,
+		TestTags{"key-1": "value-1?"},
+		"validate tags value value-1? error, incorrect value",
+	},
+	{
+		6,
+		TestTags{
+			"key-1":  "value",
+			"key-2":  "value",
+			"key-3":  "value",
+			"key-4":  "value",
+			"key-5":  "value",
+			"key-6":  "value",
+			"key-7":  "value",
+			"key-8":  "value",
+			"key-9":  "value",
+			"key-10": "value",
+			"key-11": "value",
+		},
+		"validate tags: 11 tags more than 10",
+	},
+}
+
+func TestValidateTags(t *testing.T) {
+	for _, testCase := range ValidateTagsTestCases {
+		err := ValidateTags(testCase.tags)
+		if testCase.wantErrString == "" {
+			assert.NoErrorf(t, err, "no error")
+		} else {
+			assert.EqualError(t, err, testCase.wantErrString)
+		}
+	}
+}