refactor(gateway): parse AWS query bodies with url.ParseQuery (#569)

spinifex/gateway/gateway.go

@@ -606,23 +606,17 @@ func readQueryArgs(r *http.Request) (map[string]string, error) {
 // ParseAWSQueryArgs parses an AWS query-protocol body. Returns an error on
 // invalid percent-encoding so callers can surface MalformedQueryString.
 func ParseAWSQueryArgs(query string) (map[string]string, error) {
-	params := make(map[string]string)
-	pairs := strings.SplitSeq(query, "&")
-	for pair := range pairs {
-		kv := strings.SplitN(pair, "=", 2)
-		key, err := url.QueryUnescape(kv[0])
-		if err != nil {
-			return nil, fmt.Errorf("invalid URL encoding in parameter name: %w", err)
-		}
-		if len(kv) == 2 {
-			value, err := url.QueryUnescape(kv[1])
-			if err != nil {
-				return nil, fmt.Errorf("invalid URL encoding in value for %q: %w", key, err)
-			}
-			params[key] = value
-		} else {
-			params[key] = ""
-		}
+	values, err := url.ParseQuery(query)
+	if err != nil {
+		return nil, fmt.Errorf("invalid AWS query string: %w", err)
+	}
+
+	params := make(map[string]string, len(values))
+	for key, vs := range values {
+		// The query protocol indexes repeated parameters (Filter.1.Value.1), so a
+		// bare duplicate key only arrives from a non-conforming client. Take the
+		// last occurrence.
+		params[key] = vs[len(vs)-1]
 	}
 	return params, nil
 }

spinifex/gateway/gateway_test.go

@@ -417,7 +417,7 @@ func TestParseAWSQueryArgs(t *testing.T) {
 		{
 			name:     "empty string",
 			query:    "",
-			expected: map[string]string{"": ""},
+			expected: map[string]string{},
 		},
 		{
 			name:  "multiple parameters",
@@ -445,6 +445,51 @@ func TestParseAWSQueryArgs(t *testing.T) {
 				"Tag.Name": "my tag",
 			},
 		},
+		{
+			name:  "plus decodes to space",
+			query: "Name=my+volume&Description=a+b+c",
+			expected: map[string]string{
+				"Name":        "my volume",
+				"Description": "a b c",
+			},
+		},
+		{
+			name:  "duplicate key takes the last value",
+			query: "Action=DescribeInstances&Action=RunInstances",
+			expected: map[string]string{
+				"Action": "RunInstances",
+			},
+		},
+		{
+			name:  "empty value",
+			query: "Action=DescribeVolumes&NextToken=",
+			expected: map[string]string{
+				"Action":    "DescribeVolumes",
+				"NextToken": "",
+			},
+		},
+		{
+			name:  "empty segments are skipped",
+			query: "Action=DescribeInstances&&Version=2016-11-15&",
+			expected: map[string]string{
+				"Action":  "DescribeInstances",
+				"Version": "2016-11-15",
+			},
+		},
+		{
+			name:  "encoded reserved characters in a value",
+			query: "Filter.1.Value.1=a%3Bb%26c%3Dd",
+			expected: map[string]string{
+				"Filter.1.Value.1": "a;b&c=d",
+			},
+		},
+		{
+			name:  "base64 user data round-trips",
+			query: "UserData=IyEvYmluL2Jhc2gKZWNobyAiaGVsbG8i%0A",
+			expected: map[string]string{
+				"UserData": "IyEvYmluL2Jhc2gKZWNobyAiaGVsbG8i\n",
+			},
+		},
 	}
 
 	for _, tc := range tests {
@@ -466,11 +511,96 @@ func TestParseAWSQueryArgs_MalformedURLEncoding(t *testing.T) {
 		{"bad value encoding", "Action=DescribeInstances&Name=%ZZ"},
 		{"bad key encoding", "Bad%ZZKey=value"},
 		{"bad lone key encoding", "Lone%ZZ"},
+		{"truncated escape", "Action=DescribeInstances&Name=%A"},
+		// url.ParseQuery rejects a raw ";" as an ambiguous separator. Every AWS
+		// SDK and the CLI send it as %3B, so only a non-conforming client sees this.
+		{"raw semicolon separator", "Action=DescribeInstances;Version=2016-11-15"},
 	}
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
-			_, err := ParseAWSQueryArgs(tc.query)
+			args, err := ParseAWSQueryArgs(tc.query)
 			require.Error(t, err)
+			// Callers must not act on a partially decoded request.
+			assert.Nil(t, args)
+		})
+	}
+}
+
+// Request bodies captured off the wire from aws-cli v2, covering the payloads
+// most likely to break a parser: a base64 user-data blob, a percent-encoded IAM
+// policy document, and tag values holding ";", "&", "=" and "%". These are
+// verbatim, not regenerated by an encoder, so the expectations stay honest even
+// if the parser and the test were ever to share an encoding bug.
+func TestParseAWSQueryArgs_CapturedClientBodies(t *testing.T) {
+	tests := []struct {
+		name     string
+		body     string
+		expected map[string]string
+	}{
+		{
+			name: "ec2 create-tags with reserved characters in values",
+			body: "Action=CreateTags&Version=2016-11-15&ResourceId.1=i-123&Tag.1.Key=Cmd" +
+				"&Tag.1.Value=echo+hi%3B+ls&Tag.2.Key=Note&Tag.2.Value=a%3Bb%26c%3Dd+100%25",
+			expected: map[string]string{
+				"Action":       "CreateTags",
+				"Version":      "2016-11-15",
+				"ResourceId.1": "i-123",
+				"Tag.1.Key":    "Cmd",
+				// The CLI escapes ";" as %3B and a space as "+", so a semicolon in a
+				// tag value never reaches the parser as a raw separator.
+				"Tag.1.Value": "echo hi; ls",
+				"Tag.2.Key":   "Note",
+				"Tag.2.Value": "a;b&c=d 100%",
+			},
+		},
+		{
+			name: "ec2 run-instances with base64 user data",
+			body: "Action=RunInstances&Version=2016-11-15&ImageId=ami-0abc123&InstanceType=t2.micro" +
+				"&UserData=IyEvYmluL2Jhc2gKZWNobyAiaGVsbG87IHdvcmxkIgpleHBvcnQgWD0nYT1iJmM9ZCcK" +
+				"&TagSpecification.1.ResourceType=instance&TagSpecification.1.Tag.1.Key=Name" +
+				"&TagSpecification.1.Tag.1.Value=web%3B+prod&TagSpecification.1.Tag.2.Key=Cost" +
+				"&TagSpecification.1.Tag.2.Value=100%25&MinCount=1&MaxCount=1" +
+				"&ClientToken=6072fd95-41f0-4339-8099-0e68d873af82",
+			expected: map[string]string{
+				"Action":       "RunInstances",
+				"Version":      "2016-11-15",
+				"ImageId":      "ami-0abc123",
+				"InstanceType": "t2.micro",
+				// Base64 survives intact: "+" inside the blob would decode to a space,
+				// so the CLI emits a padding-free alphabet the parser must not mangle.
+				"UserData":                        "IyEvYmluL2Jhc2gKZWNobyAiaGVsbG87IHdvcmxkIgpleHBvcnQgWD0nYT1iJmM9ZCcK",
+				"TagSpecification.1.ResourceType": "instance",
+				"TagSpecification.1.Tag.1.Key":    "Name",
+				"TagSpecification.1.Tag.1.Value":  "web; prod",
+				"TagSpecification.1.Tag.2.Key":    "Cost",
+				"TagSpecification.1.Tag.2.Value":  "100%",
+				"MinCount":                        "1",
+				"MaxCount":                        "1",
+				"ClientToken":                     "6072fd95-41f0-4339-8099-0e68d873af82",
+			},
+		},
+		{
+			name: "iam put-role-policy with a JSON policy document",
+			body: "Action=PutRolePolicy&Version=2010-05-08&RoleName=TestRole&PolicyName=TestPolicy" +
+				"&PolicyDocument=%7B%22Version%22%3A%222012-10-17%22%2C%22Statement%22%3A%5B%7B" +
+				"%22Effect%22%3A%22Allow%22%2C%22Action%22%3A%5B%22s3%3AGetObject%22%5D%2C" +
+				"%22Resource%22%3A%22arn%3Aaws%3As3%3A%3A%3Abucket%2F%2A%22%7D%5D%7D%0A",
+			expected: map[string]string{
+				"Action":     "PutRolePolicy",
+				"Version":    "2010-05-08",
+				"RoleName":   "TestRole",
+				"PolicyName": "TestPolicy",
+				"PolicyDocument": `{"Version":"2012-10-17","Statement":[{"Effect":"Allow",` +
+					`"Action":["s3:GetObject"],"Resource":"arn:aws:s3:::bucket/*"}]}` + "\n",
+			},
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			parsed, err := ParseAWSQueryArgs(tc.body)
+			require.NoError(t, err)
+			assert.Equal(t, tc.expected, parsed)
 		})
 	}
 }