Bugfix around message attributes on Publish(SNS), ReceiveMessages(SQS) #339

kojisaikiAtSony · 2024-11-13T11:42:42Z

We found 3 bugs around message attributes, as shown below:

Received Message Attribute via XML is not decoded from base 64.
DataType value was broken by caser unexpectedly.
- The exact conditions have not been identified, but at a certain point when publish/receive messages are repeated, the DataType value String becomes a string like \u0000\u0000\u0000\u0000\u0000\u0000 due to the caser.
MD5OfMessageAttributes is missing on the message with message attributes from SQS-ReceiveMessages.

kojisaikiAtSony · 2024-11-13T11:59:27Z

app/models/requests.go

-		binaryValue := values.Get(fmt.Sprintf("Entries.%d.MessageAttributes.%d.Value.BinaryValue", entryIndex, attributeIndex))
+		encodedBinaryValue := values.Get(fmt.Sprintf("Entries.%d.MessageAttributes.%d.Value.BinaryValue", entryIndex, attributeIndex))

 		if r.Entries[entryIndex].MessageAttributes == nil {
 			r.Entries[entryIndex].MessageAttributes = make(map[string]MessageAttribute)
 		}

+		binaryValue, err := base64.StdEncoding.DecodeString(encodedBinaryValue)
+		if err != nil {
+			log.Warnf("Failed to base64 decode. %s may not have been base64 encoded.", encodedBinaryValue)
+			continue
+		}
+
 		r.Entries[entryIndex].MessageAttributes[name] = MessageAttribute{
 			DataType:    dataType,
 			StringValue: stringValue,
-			BinaryValue: []byte(binaryValue),
+			BinaryValue: binaryValue,


This is the same fix for bug 1.

kojisaikiAtSony · 2024-11-13T11:59:35Z

app/models/requests.go

-		binaryValue := values.Get(fmt.Sprintf("MessageAttributes.entry.%d.Value.BinaryValue", i))
+		encodedBinaryValue := values.Get(fmt.Sprintf("MessageAttributes.entry.%d.Value.BinaryValue", i))
+		binaryValue, err := base64.StdEncoding.DecodeString(encodedBinaryValue)
+		if err != nil {
+			log.Warnf("Failed to base64 decode. %s may not have been base64 encoded.", encodedBinaryValue)
+			continue
+		}

 		if r.MessageAttributes == nil {
 			r.MessageAttributes = make(map[string]MessageAttribute)
 		}
+
 		attributes[name] = MessageAttribute{
-			DataType:    caser.String(dataType), // capitalize
+			DataType:    cases.Title(language.AmericanEnglish).String(dataType), // capitalize
 			StringValue: stringValue,
-			BinaryValue: []byte(binaryValue),
+			BinaryValue: binaryValue,


This is the same fix for bug 1.

kojisaikiAtSony · 2024-11-13T12:00:22Z

app/models/requests.go

-		binaryValue := values.Get(fmt.Sprintf("MessageAttribute.%d.Value.BinaryValue", i))
+		encodedBinaryValue := values.Get(fmt.Sprintf("MessageAttribute.%d.Value.BinaryValue", i))
+
+		binaryValue, err := base64.StdEncoding.DecodeString(encodedBinaryValue)
+		if err != nil {
+			log.Warnf("Failed to base64 decode. %s may not have been base64 encoded.", encodedBinaryValue)
+			continue
+		}

 		r.MessageAttributes[name] = MessageAttribute{
 			DataType:    dataType,
 			StringValue: stringValue,
-			BinaryValue: []byte(binaryValue),
+			BinaryValue: binaryValue,


This fixes Bug 1 "Received Message Attribute via XML is not decoded from base 64".
JSON requests has decoded from base 64 on JSON marshaller, but for XML, here SetAttributesFromForm does not have decoding step. So internal BinaryValue was still base 64 string (as binary) value.

I don't really love this. I know it's more true to life, since that input is supposed to be encoded, but I'd prefer to make the decoding optional. It's a testing tool after all, and we're doing our best to cover the needs of all. It's not like we're throwing errors for invalid attributes or anything.

So maybe instead of your continue, the log is fine, but take the value as is and stuff it in the resulting attribute?

OK! I will update it as the below 👍

Try decoding

Success: store it as the decoded raw value

Failure: log and store it as original requested value

Sorry @kojisaikiAtSony - I'm looking at this again, and more of my previous thoughts are coming back to me. Looks to me like this change is decoding it, storing it internally, and then upon receiving the message, encoding it, and returning it. Double check me, is that what you're doing here?

If so, why do this at all? That's why I undid all this decoding/encoding business not long ago. It didn't seem like there was a point. Are you having trouble with the checksum or something because it's encoded? Where are you seeing the "Received Message Attribute via XML is not decoded from base 64" error, and how are you recreating it?

kojisaikiAtSony · 2024-11-13T12:01:08Z

app/models/requests_test.go

-	assert.Equal(t, []uint8("VmFsdWUy"), attr2.BinaryValue)
+	assert.Equal(t, []uint8("Value2"), attr2.BinaryValue)


Internal value should be raw (not base 64 encoded) value.

kojisaikiAtSony · 2024-11-13T12:06:54Z

app/models/responses.go

+		if value.DataType == "Binary" {
+			value.BinaryValue = []byte(base64.StdEncoding.EncodeToString(value.BinaryValue))
+		}


Here is the related fix for Bug 1.

In the AWS query protocol, blobs are sent and received after being base64 encoded , so the binary value should be base64 encoded.

I don't know about this. I understand that's part of the query protocol, but that whole thing is deprecated in AWS. Based on my tests this works, as is, for the existing sdks I've tried. What exactly isn't working for you?

As you said this part is for the query protocol. The latest version of AWS SDK for Java also works without this part.

I thought that future versions of GOAWS still keep support the query protocol, so I added it.
Sorry, I don't remember clearly, but Is the policy that "users who need to use the query protocol should use older versions of GOAWS"? (It also makes sense)

Well, no, that's not the policy per say. I hadn't given it that much thought if I'm honest. But if you think about it like this - this is a tool for folks to run their own apps against instead AWS directly. So my main goal is to make it work in a performant/lightweight way and replicate as much of the AWS SQS/SNS "contract" as I can.

So with that in mind - we don't NEED to match what AWS does, as long as we respond the same way that it would. We just need to match their contracts right? At any rate we can't know what they're doing all the times as their docs are generally garbage in my opinion.

So, I don't care about things like security, and base64 encoding much, because, it's really up to the user here to worry about their input and output - because it's an app running where a user puts it. "Do what you will with it." so to speak, as long as an SDK doesn't fail on it - "garbage in garbage" out.

Here's what I need:
So all that said - this change is doing a couple of things. I need to know why we need them. The way I had it lately was working for me, based on the contract I had researched. What I'd like to see is what you're failing with, how you're recreating it, and why we think these changes will fix them.

Sorry - this is a lot for me to juggle long term and I need a little grooming/refining (in agile terms), before I let it in. Otherwise I'll be adrift if someone else complains about it or something similar later.

As always thank you for giving it so much work and thought. I really appreciate it and without you I wouldn't be able to keep this tool as current and viable as it is.

kojisaikiAtSony · 2024-11-13T12:08:57Z

app/models/responses_test.go

-	entry = "<MessageAttribute><Name>attr2</Name><Value><BinaryValue>binary-value</BinaryValue><DataType>Binary</DataType></Value></MessageAttribute>"
+	entry = "<MessageAttribute><Name>attr2</Name><Value><BinaryValue>YmluYXJ5LXZhbHVl</BinaryValue><DataType>Binary</DataType></Value></MessageAttribute>"


Assertion update for blobs are sent and received after being base64 encoded.

kojisaikiAtSony · 2024-11-13T12:10:46Z

smoke_tests/sns_publish_test.go

+
+	snsClient.SetSubscriptionAttributes(context.TODO(), &sns.SetSubscriptionAttributesInput{
+		SubscriptionArn: subscribeResult.SubscriptionArn,
+		AttributeName:   aws.String("RawMessageDelivery"),


This new test pattern is for "RawMessageDelivery".

kojisaikiAtSony · 2024-11-13T12:11:58Z

smoke_tests/sqs_receive_message_test.go

@@ -268,7 +268,7 @@ func Test_ReceiveMessageV1_xml_with_attributes(t *testing.T) {
 	assert.Equal(t, 1, len(receiveMessageResponse.Result.Messages))
 	assert.Equal(t, "MyTestMessage", receiveMessageResponse.Result.Messages[0].Body)
 	assert.Equal(t, "ad4883a84ad41c79aa3a373698c0d4e9", receiveMessageResponse.Result.Messages[0].MD5OfBody)
-	assert.Equal(t, "", receiveMessageResponse.Result.Messages[0].MD5OfMessageAttributes)
+	assert.Equal(t, "ae8770938aee44bc548cf65ac377e3bf", receiveMessageResponse.Result.Messages[0].MD5OfMessageAttributes)


xml/json both should have hash for message attribute if the message attributes is not empty.

kojisaikiAtSony · 2024-11-13T12:13:00Z

smoke_tests/sqs_send_message_batch_test.go

@@ -370,7 +372,7 @@ func TestSendMessageBatchV1_Xml_Success_including_attributes(t *testing.T) {
 		WithFormField("Entries.1.MessageBody", messageBody2).
 		WithFormField("Entries.1.MessageAttributes.1.Name", binaryAttributeKey).
 		WithFormField("Entries.1.MessageAttributes.1.Value.DataType", binaryType).
-		WithFormField("Entries.1.MessageAttributes.1.Value.BinaryValue", binaryValue).
+		WithFormField("Entries.1.MessageAttributes.1.Value.BinaryValue", binaryValueEncodeString).


Here is also for blobs are sent and received after being base64 encoded.
BinaryValue in XML should be base 64 encoded.

kojisaikiAtSony · 2024-11-13T12:18:15Z

app/models/requests.go

-			DataType:    caser.String(dataType), // capitalize
+			DataType:    cases.Title(language.AmericanEnglish).String(dataType), // capitalize


Here is the fix for Bug 2 "DataType value was broken by caser unexpectedly".

As I wrote in the description, I don't know the exact conditions under which this occurs, but since there was no need to make it a global variable, I changed it to a local definition and the problem was resolved. If you have any ideas, I would appreciate your comments.

I think we need to understand more about why you want to change this. If you don't know the exact conditions does that mean you can't recreate it? If so, what are we fixing, and beyond that how can we create a test that makes sure the issue is fixed?

In the short term, I like the pattern of it being global. If I need to uppercase anything else, I want to use the same instance of caser instead of creating new ones all over the place. That's why I made it global.

Can you provide a little more information before we move forward with this?

Yep, I thought you said so 😅. It's a little creepy when the behavior isn't clear. Let's dive into the caser code and take a closer look 👍

Admiral-Piett

I need some more information on this one before we move forward. Having just done some significant work on this, I want to understand more about what's causing you problems before we change it. Also, I think there are a couple opportunities to be the "path of least resistance" here, and I think that's helpful. See my individualized comments for the specifics.

Finally - #338 @toastwaffle did some good work on combining the logic around parsing incoming MessageAttributes. It'll definitely affect your change, since you'd only need to do it once. Want to take a look at what they've got first? I think there's is pretty close and I think it makes sense to get that in before this, so we can just fix the bugs in 1 area, if you're ok with that.

Thanks for looking at this, as always!

Admiral-Piett · 2024-11-15T20:46:46Z

app/models/requests.go

-		binaryValue := values.Get(fmt.Sprintf("MessageAttribute.%d.Value.BinaryValue", i))
+		encodedBinaryValue := values.Get(fmt.Sprintf("MessageAttribute.%d.Value.BinaryValue", i))
+
+		binaryValue, err := base64.StdEncoding.DecodeString(encodedBinaryValue)
+		if err != nil {
+			log.Warnf("Failed to base64 decode. %s may not have been base64 encoded.", encodedBinaryValue)
+			continue
+		}

 		r.MessageAttributes[name] = MessageAttribute{
 			DataType:    dataType,
 			StringValue: stringValue,
-			BinaryValue: []byte(binaryValue),
+			BinaryValue: binaryValue,


I don't really love this. I know it's more true to life, since that input is supposed to be encoded, but I'd prefer to make the decoding optional. It's a testing tool after all, and we're doing our best to cover the needs of all. It's not like we're throwing errors for invalid attributes or anything.

So maybe instead of your continue, the log is fine, but take the value as is and stuff it in the resulting attribute?

Admiral-Piett · 2024-11-15T20:51:09Z

app/models/responses.go

+		if value.DataType == "Binary" {
+			value.BinaryValue = []byte(base64.StdEncoding.EncodeToString(value.BinaryValue))
+		}


I don't know about this. I understand that's part of the query protocol, but that whole thing is deprecated in AWS. Based on my tests this works, as is, for the existing sdks I've tried. What exactly isn't working for you?

Admiral-Piett · 2024-11-15T20:54:43Z

app/models/responses.go

+	if r.MessageAttributes != nil {
+		e.EncodeElement(r.MD5OfMessageAttributes, xml.StartElement{Name: xml.Name{Local: "MD5OfMessageAttributes"}})
+	}


I'm not sure why I left this out. I think there was a reason though. A lot of this had to do with the raw vs regular message handling. Can you tell me more about what your issues are on this?

kojisaikiAtSony · 2024-11-19T03:31:07Z

Finally - #338 @toastwaffle did some good work on combining the logic around parsing incoming MessageAttributes.

The PR is very helpful! We'll rebase this once it is merged 👍

kojisaikiAtSony · 2024-11-25T01:20:55Z

@Admiral-Piett
I'm really sorry for creating such a big and confusing PR without considering your burden 🙇
I will create separated, small PRs on each fixes.

bugfix message attributes

7b3ec1a

kojisaikiAtSony commented Nov 13, 2024

View reviewed changes

Admiral-Piett reviewed Nov 15, 2024

View reviewed changes

kojisaikiAtSony closed this Nov 25, 2024

kojisaikiAtSony mentioned this pull request Nov 25, 2024

Move caser from a global variable to a local variable #340

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix around message attributes on Publish(SNS), ReceiveMessages(SQS) #339

Bugfix around message attributes on Publish(SNS), ReceiveMessages(SQS) #339

kojisaikiAtSony commented Nov 13, 2024 •

edited

Loading

kojisaikiAtSony Nov 13, 2024

kojisaikiAtSony Nov 13, 2024

kojisaikiAtSony Nov 13, 2024

Admiral-Piett Nov 15, 2024

kojisaikiAtSony Nov 19, 2024

Admiral-Piett Nov 22, 2024

kojisaikiAtSony Nov 13, 2024

kojisaikiAtSony Nov 13, 2024

Admiral-Piett Nov 15, 2024

kojisaikiAtSony Nov 19, 2024 •

edited

Loading

Admiral-Piett Nov 22, 2024

kojisaikiAtSony Nov 13, 2024

kojisaikiAtSony Nov 13, 2024

kojisaikiAtSony Nov 13, 2024

kojisaikiAtSony Nov 13, 2024

kojisaikiAtSony Nov 13, 2024

Admiral-Piett Nov 15, 2024

kojisaikiAtSony Nov 19, 2024

Admiral-Piett left a comment

Admiral-Piett Nov 15, 2024

Admiral-Piett Nov 15, 2024

Admiral-Piett Nov 15, 2024

kojisaikiAtSony commented Nov 19, 2024 •

edited

Loading

kojisaikiAtSony commented Nov 25, 2024 •

edited

Loading

		assert.Equal(t, []uint8("VmFsdWUy"), attr2.BinaryValue)
		assert.Equal(t, []uint8("Value2"), attr2.BinaryValue)

		entry = "<MessageAttribute><Name>attr2</Name><Value><BinaryValue>binary-value</BinaryValue><DataType>Binary</DataType></Value></MessageAttribute>"
		entry = "<MessageAttribute><Name>attr2</Name><Value><BinaryValue>YmluYXJ5LXZhbHVl</BinaryValue><DataType>Binary</DataType></Value></MessageAttribute>"

		DataType: caser.String(dataType), // capitalize
		DataType: cases.Title(language.AmericanEnglish).String(dataType), // capitalize

Bugfix around message attributes on Publish(SNS), ReceiveMessages(SQS) #339

Bugfix around message attributes on Publish(SNS), ReceiveMessages(SQS) #339

Conversation

kojisaikiAtSony commented Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kojisaikiAtSony Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Admiral-Piett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kojisaikiAtSony commented Nov 19, 2024 • edited Loading

kojisaikiAtSony commented Nov 25, 2024 • edited Loading

kojisaikiAtSony commented Nov 13, 2024 •

edited

Loading

kojisaikiAtSony Nov 19, 2024 •

edited

Loading

kojisaikiAtSony commented Nov 19, 2024 •

edited

Loading

kojisaikiAtSony commented Nov 25, 2024 •

edited

Loading