fail properly while parsing duplicate tags (#1701)

AWS S3 API expects duplicate keys to be rejected,
due to our usage of url.ParseQuery() this was skipped
since url query parser simply appends duplicate keys
as key with multiple values, this is not desirable.

Add more input validation and restrictions as per
AWS S3 API.

.github/workflows/go.yml

@@ -17,7 +17,7 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        go-version: [1.18.x, 1.19.x]
+        go-version: [1.17.x, 1.18.x, 1.19.x]
         os: [ubuntu-latest]
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}

pkg/tags/tags.go

@@ -1,5 +1,6 @@
 /*
- * MinIO Cloud Storage, (C) 2020 MinIO, Inc.
+ * MinIO Go Library for Amazon S3 Compatible Cloud Storage
+ * Copyright 2020-2022 MinIO, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -20,6 +21,8 @@ import (
 	"encoding/xml"
 	"io"
 	"net/url"
+	"regexp"
+	"sort"
 	"strings"
 	"unicode/utf8"
 )
@@ -63,17 +66,28 @@ const (
 	maxTagCount       = 50
 )
 
+// https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/Using_Tags.html#tag-restrictions
+// borrowed from this article and also testing various ASCII characters following regex
+// is supported by AWS S3 for both tags and values.
+var validTagKeyValue = regexp.MustCompile(`^[a-zA-Z0-9-+\-._:/@]+$`)
+
 func checkKey(key string) error {
-	if len(key) == 0 || utf8.RuneCountInString(key) > maxKeyLength || strings.Contains(key, "&") {
+	if len(key) == 0 {
+		return errInvalidTagKey
+	}
+
+	if utf8.RuneCountInString(key) > maxKeyLength || !validTagKeyValue.MatchString(key) {
 		return errInvalidTagKey
 	}
 
 	return nil
 }
 
 func checkValue(value string) error {
-	if utf8.RuneCountInString(value) > maxValueLength || strings.Contains(value, "&") {
-		return errInvalidTagValue
+	if value != "" {
+		if utf8.RuneCountInString(value) > maxValueLength || !validTagKeyValue.MatchString(value) {
+			return errInvalidTagValue
+		}
 	}
 
 	return nil
@@ -136,11 +150,26 @@ type tagSet struct {
 }
 
 func (tags tagSet) String() string {
-	vals := make(url.Values)
-	for key, value := range tags.tagMap {
-		vals.Set(key, value)
+	if len(tags.tagMap) == 0 {
+		return ""
 	}
-	return vals.Encode()
+	var buf strings.Builder
+	keys := make([]string, 0, len(tags.tagMap))
+	for k := range tags.tagMap {
+		keys = append(keys, k)
+	}
+	sort.Strings(keys)
+	for _, k := range keys {
+		keyEscaped := url.QueryEscape(k)
+		valueEscaped := url.QueryEscape(tags.tagMap[k])
+		if buf.Len() > 0 {
+			buf.WriteByte('&')
+		}
+		buf.WriteString(keyEscaped)
+		buf.WriteByte('=')
+		buf.WriteString(valueEscaped)
+	}
+	return buf.String()
 }
 
 func (tags *tagSet) remove(key string) {
@@ -175,7 +204,7 @@ func (tags *tagSet) set(key, value string, failOnExist bool) error {
 }
 
 func (tags tagSet) toMap() map[string]string {
-	m := make(map[string]string)
+	m := make(map[string]string, len(tags.tagMap))
 	for key, value := range tags.tagMap {
 		m[key] = value
 	}
@@ -188,6 +217,7 @@ func (tags tagSet) MarshalXML(e *xml.Encoder, start xml.StartElement) error {
 		Tags []Tag `xml:"Tag"`
 	}{}
 
+	tagList.Tags = make([]Tag, 0, len(tags.tagMap))
 	for key, value := range tags.tagMap {
 		tagList.Tags = append(tagList.Tags, Tag{key, value})
 	}
@@ -213,7 +243,7 @@ func (tags *tagSet) UnmarshalXML(d *xml.Decoder, start xml.StartElement) error {
 		return errTooManyTags
 	}
 
-	m := map[string]string{}
+	m := make(map[string]string, len(tagList.Tags))
 	for _, tag := range tagList.Tags {
 		if _, found := m[tag.Key]; found {
 			return errDuplicateTagKey
@@ -311,25 +341,58 @@ func ParseObjectXML(reader io.Reader) (*Tags, error) {
 	return unmarshalXML(reader, true)
 }
 
+// stringsCut slices s around the first instance of sep,
+// returning the text before and after sep.
+// The found result reports whether sep appears in s.
+// If sep does not appear in s, cut returns s, "", false.
+func stringsCut(s, sep string) (before, after string, found bool) {
+	if i := strings.Index(s, sep); i >= 0 {
+		return s[:i], s[i+len(sep):], true
+	}
+	return s, "", false
+}
+
+func (tags *tagSet) parseTags(tgs string) (err error) {
+	for tgs != "" {
+		var key string
+		key, tgs, _ = stringsCut(tgs, "&")
+		if key == "" {
+			continue
+		}
+		key, value, _ := stringsCut(key, "=")
+		key, err1 := url.QueryUnescape(key)
+		if err1 != nil {
+			if err == nil {
+				err = err1
+			}
+			continue
+		}
+		value, err1 = url.QueryUnescape(value)
+		if err1 != nil {
+			if err == nil {
+				err = err1
+			}
+			continue
+		}
+		if err = tags.set(key, value, true); err != nil {
+			return err
+		}
+	}
+	return err
+}
+
 // Parse decodes HTTP query formatted string into tags which is limited by isObject.
 // A query formatted string is like "key1=value1&key2=value2".
 func Parse(s string, isObject bool) (*Tags, error) {
-	values, err := url.ParseQuery(s)
-	if err != nil {
-		return nil, err
-	}
-
 	tagging := &Tags{
 		TagSet: &tagSet{
 			tagMap:   make(map[string]string),
 			isObject: isObject,
 		},
 	}
 
-	for key := range values {
-		if err := tagging.TagSet.set(key, values.Get(key), true); err != nil {
-			return nil, err
-		}
+	if err := tagging.TagSet.parseTags(s); err != nil {
+		return nil, err
 	}
 
 	return tagging, nil

pkg/tags/tags_test.go

@@ -0,0 +1,89 @@
+/*
+ * MinIO Go Library for Amazon S3 Compatible Cloud Storage
+ * Copyright 2022 MinIO, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package tags
+
+import (
+	"fmt"
+	"testing"
+)
+
+func TestParseTags(t *testing.T) {
+	testCases := []struct {
+		tags        string
+		expectedErr bool
+	}{
+		{
+			"key1=value1&key2=value2",
+			false,
+		},
+		{
+			fmt.Sprintf("%0128d=%0256d", 1, 1),
+			false,
+		},
+		// Failure cases - https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/Using_Tags.html#tag-restrictions
+		{
+			"key1=value1&key1=value2",
+			true,
+		},
+		{
+			"key$=value1",
+			true,
+		},
+		{
+			"key1=value$",
+			true,
+		},
+		{
+			fmt.Sprintf("%0128d=%0257d", 1, 1),
+			true,
+		},
+		{
+			fmt.Sprintf("%0129d=%0256d", 1, 1),
+			true,
+		},
+		{
+			fmt.Sprintf("%0129d=%0257d", 1, 1),
+			true,
+		},
+	}
+
+	for _, testCase := range testCases {
+		testCase := testCase
+		t.Run(testCase.tags, func(t *testing.T) {
+			tt, err := ParseObjectTags(testCase.tags)
+			if !testCase.expectedErr && err != nil {
+				t.Errorf("Expected success but failed with %v", err)
+			}
+			if testCase.expectedErr && err == nil {
+				t.Error("Expected failure but found success")
+			}
+			if err == nil {
+				t.Logf("%s", tt)
+			}
+		})
+	}
+}
+
+func BenchmarkParseTags(b *testing.B) {
+	b.ResetTimer()
+	b.ReportAllocs()
+
+	for i := 0; i < b.N; i++ {
+		ParseObjectTags("key1=value1&key2=value2")
+	}
+}