From 05901aa303139443043a51fd4f0df1966adb0c28 Mon Sep 17 00:00:00 2001
From: Clement Sam <clementsam75@gmail.com>
Date: Tue, 3 Oct 2023 13:19:17 +0000
Subject: [PATCH] cmd/tools/tag-signer: fail for comma separated tags

This change allows tag-signer to fail when key-value pairs
provided as arguments are comma-separated.
However, for cases where a value is expected to contain a comma,
we validate the value only if --confirm flag is specified

Resolves https://github.com/storj/storj/issues/6336

Change-Id: Ib6a100ee3adf529f44c8b3ca620a3c0b4f953a17
---
 cmd/tools/tag-signer/main.go      | 43 ++++++++++----
 cmd/tools/tag-signer/main_test.go | 99 +++++++++++++++++++++++++++++++
 2 files changed, 129 insertions(+), 13 deletions(-)
 create mode 100644 cmd/tools/tag-signer/main_test.go

diff --git a/cmd/tools/tag-signer/main.go b/cmd/tools/tag-signer/main.go
index 99a9be3db..ef490cdc5 100644
--- a/cmd/tools/tag-signer/main.go
+++ b/cmd/tools/tag-signer/main.go
@@ -68,6 +68,7 @@ var (
 type Config struct {
 	IdentityDir string `help:"location if the identity files" path:"true"`
 	NodeID      string `help:"the ID of the node, which will used this tag "`
+	Confirm     bool   `help:"enable comma in tag values" default:"false"`
 }
 
 func init() {
@@ -107,19 +108,9 @@ func signTags(ctx context.Context, cfg Config, tagPairs []string) (string, error
 		SignedAt: time.Now().Unix(),
 	}
 
-	for _, tag := range tagPairs {
-		tag = strings.TrimSpace(tag)
-		if len(tag) == 0 {
-			continue
-		}
-		parts := strings.SplitN(tag, "=", 2)
-		if len(parts) != 2 {
-			return "", errs.New("tags should be in KEY=VALUE format, but it was %s", tag)
-		}
-		tagSet.Tags = append(tagSet.Tags, &pb.Tag{
-			Name:  parts[0],
-			Value: []byte(parts[1]),
-		})
+	tagSet.Tags, err = parseTagPairs(tagPairs, cfg.Confirm)
+	if err != nil {
+		return "", err
 	}
 
 	signedMessage, err := nodetag.Sign(ctx, tagSet, signer)
@@ -183,6 +174,32 @@ func inspect(ctx context.Context, s string) error {
 	return nil
 }
 
+func parseTagPairs(tagPairs []string, allowCommaValues bool) ([]*pb.Tag, error) {
+	tags := make([]*pb.Tag, 0, len(tagPairs))
+
+	for _, tag := range tagPairs {
+		tag = strings.TrimSpace(tag)
+		if len(tag) == 0 {
+			continue
+		}
+
+		if !allowCommaValues && strings.ContainsRune(tag, ',') {
+			return nil, errs.New("multiple tags should be separated by spaces instead of commas, or specify --confirm to enable commas in tag values")
+		}
+
+		parts := strings.SplitN(tag, "=", 2)
+		if len(parts) != 2 {
+			return nil, errs.New("tags should be in KEY=VALUE format, but it was %s", tag)
+		}
+		tags = append(tags, &pb.Tag{
+			Name:  parts[0],
+			Value: []byte(parts[1]),
+		})
+	}
+
+	return tags, nil
+}
+
 func main() {
 	process.ExecWithCustomOptions(rootCmd, process.ExecOptions{
 		LoadConfig: func(cmd *cobra.Command, vip *viper.Viper) error {
diff --git a/cmd/tools/tag-signer/main_test.go b/cmd/tools/tag-signer/main_test.go
new file mode 100644
index 000000000..4df6263f6
--- /dev/null
+++ b/cmd/tools/tag-signer/main_test.go
@@ -0,0 +1,99 @@
+// Copyright (C) 2023 Storj Labs, Inc.
+// See LICENSE for copying information.
+
+package main
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"storj.io/common/pb"
+)
+
+func Test_parseTagPairs(t *testing.T) {
+	tests := []struct {
+		name          string
+		args          []string
+		confirm       bool
+		expected      []*pb.Tag
+		expectedError string
+	}{
+		{
+			name:          "comma separated tag pairs without confirm flag",
+			args:          []string{"key1=value1,key2=value2"},
+			expectedError: "multiple tags should be separated by spaces instead of commas, or specify --confirm to enable commas in tag values",
+		},
+		{
+			name:    "comma separated tag pairs with confirm flag",
+			args:    []string{"key1=value1,key2=value2"},
+			confirm: true,
+			expected: []*pb.Tag{
+				{
+					Name:  "key1",
+					Value: []byte("value1,key2=value2"),
+				},
+			},
+		},
+		{
+			name:    "single tag pair",
+			args:    []string{"key1=value1"},
+			confirm: true,
+			expected: []*pb.Tag{
+				{
+					Name:  "key1",
+					Value: []byte("value1"),
+				},
+			},
+		},
+		{
+			name:    "multiple tag pairs",
+			args:    []string{"key1=value1", "key2=value2"},
+			confirm: true,
+			expected: []*pb.Tag{
+				{
+					Name:  "key1",
+					Value: []byte("value1"),
+				},
+				{
+					Name:  "key2",
+					Value: []byte("value2"),
+				},
+			},
+		},
+		{
+			name:    "multiple tag pairs with with comma values and confirm flag",
+			args:    []string{"key1=value1", "key2=value2,value3"},
+			confirm: true,
+			expected: []*pb.Tag{
+				{
+					Name:  "key1",
+					Value: []byte("value1"),
+				},
+				{
+					Name:  "key2",
+					Value: []byte("value2,value3"),
+				},
+			},
+		},
+		{
+			name:          "multiple tag pairs with with comma values without confirm flag",
+			args:          []string{"key1=value1", "key2=value2,value3"},
+			expectedError: "multiple tags should be separated by spaces instead of commas, or specify --confirm to enable commas in tag values",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := parseTagPairs(tt.args, tt.confirm)
+			if tt.expectedError != "" {
+				require.Error(t, err)
+				require.Contains(t, err.Error(), tt.expectedError)
+				return
+			}
+
+			require.NoError(t, err)
+			require.Equal(t, tt.expected, got)
+		})
+	}
+}