diff --git a/spinifex/gateway/gateway.go b/spinifex/gateway/gateway.go index 32b10002e..f0aafdc5b 100644 --- a/spinifex/gateway/gateway.go +++ b/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 } diff --git a/spinifex/gateway/gateway_test.go b/spinifex/gateway/gateway_test.go index 8e8a9e966..4b04944e3 100644 --- a/spinifex/gateway/gateway_test.go +++ b/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) }) } }