add SNS receiver subject validation by qinxx108 · Pull Request #2829 · prometheus/alertmanager

qinxx108 · 2022-02-08T19:21:08Z

Add validation for subject

if not ASCII, we will replace it with error message
if longer than 100 characters, we will truncate the subject

Signed-off-by: Yijie Qin <qinyijie@amazon.com>

…sns-subject-validation

Signed-off-by: Yijie Qin <qinyijie@amazon.com>

…empty subject Signed-off-by: Soon-Ping Phang <soonping@amazon.com>

Signed-off-by: Soon-Ping Phang <soonping@amazon.com>

roidelapluie · 2022-04-11T17:11:25Z

Note to myself: are we already doing Trimspaces to remove extra spaces before or after the subject?

roidelapluie · 2022-05-03T09:14:43Z

+
+	if !isASCIINonControl(subject) {
+		*modifiedReasons = append(*modifiedReasons, fmt.Sprintf(ComponentAndModifiedReason, Subject, SubjectContainsIllegalChars))
+		level.Info(logger).Log("msg", "subject has been modified because it contains control- or non-ASCII characters", "originalSubject", subject)


Suggested change

level.Info(logger).Log("msg", "subject has been modified because it contains control- or non-ASCII characters", "originalSubject", subject)

level.Warn(logger).Log("msg", "Subject has been modified because it contains control- or non-ASCII characters.", "originalSubject", subject)

roidelapluie · 2022-05-03T09:15:01Z

+	}
+
+	// If the message is larger than our specified size we have to truncate.
+	level.Info(logger).Log("msg", "subject has been truncated because of size limit exceeded", "originalSubject", subject)


Suggested change

level.Info(logger).Log("msg", "subject has been truncated because of size limit exceeded", "originalSubject", subject)

level.Warn(logger).Log("msg", "Subject has been truncated because of size limit exceeded.", "originalSubject", subject)

We should maybe not output the full message if its' very long

roidelapluie · 2022-05-03T09:15:36Z

 	"github.com/stretchr/testify/require"
 )

+var logger = log.NewNopLogger()


can we avoid this as a top level variable?

roidelapluie · 2022-05-03T09:15:51Z

+func validateAndTruncateSubject(logger log.Logger, subject string, modifiedReasons *[]string) string {
+	if subject == "" {
+		*modifiedReasons = append(*modifiedReasons, fmt.Sprintf(ComponentAndModifiedReason, Subject, SubjectEmpty))
+		level.Info(logger).Log("msg", "subject has been modified because it is empty", "originalSubject", subject)


Suggested change

level.Info(logger).Log("msg", "subject has been modified because it is empty", "originalSubject", subject)

level.Warn(logger).Log("msg", "Subject has been modified because it is empty.", "originalSubject", subject)

roidelapluie · 2022-05-03T09:16:40Z

 )

+const (
+	// Message components


Suggested change

// Message components

// Message components.

roidelapluie · 2022-05-03T09:16:56Z

+	// Message components
+	Subject = "Subject"
+
+	// Modified Message attribute value format


Suggested change

// Modified Message attribute value format

// Modified message attribute format.

roidelapluie · 2022-05-03T09:17:11Z

+	// Modified Message attribute value format
+	ComponentAndModifiedReason = "%s: %s"
+
+	// The errors


Suggested change

// The errors

// Errors messages.

Signed-off-by: Yijie Qin <qinyijie@amazon.com>

roidelapluie

cc @gotjosh for a 2nd look

gotjosh

I've dropped a couple of very correctable fixes on the code, but...

My biggest worry is about direction - what we're doing is communicating errors via SNS as opposed to failing to deliver the notification. This seems to go against the approach taken in other receivers where we prefer to fail the notification as opposed to delivering incorrect ones. (I don't have the context, but I presume it might be due to increased costs for incorrect alerts)

Correct me if I'm wrong, but the problem you're trying to solve here is probably the biggest pain point I've seen from Alertmanager: notification failures are hard to communicate back to the user.

If it is, then I'd rather us focus on a more permanent solution that applies to all receivers and not just SNS.

That being said, I just want to hear from @roidelapluie given he already gave a 👍 to this approach. If he thinks we should proceed, I won't block this change and would only expect my changes to be addressed.

gotjosh · 2022-06-13T13:19:36Z

+	return nil
 }

 func validateAndTruncateMessage(message string, maxMessageSizeInBytes int) (string, bool, error) {


In the error message below we have spelt utf8 as the spelling of UTF-8, can quickly correct that? (I can't comment on that line because it wasn't modified here.

gotjosh · 2022-06-13T13:21:20Z

@@ -43,3 +51,172 @@ func TestValidateAndTruncateMessage(t *testing.T) {
 	_, _, err = validateAndTruncateMessage(invalidUtf8String, 100)


Suggested change

_, _, err = validateAndTruncateMessage(invalidUtf8String, 100)

const maxMessageSize = 100

_, _, err = validateAndTruncateMessage(invalidUtf8String, maxMessageSize)

And move maxMessageSize to the top of the file.

this is testing variables, add some comments

gotjosh · 2022-06-13T13:23:59Z

 	"github.com/prometheus/alertmanager/types"
 )

+const (


nit: Is there any reason why these are public, can we make them private?

gotjosh · 2022-06-13T13:42:26Z

 	}

+	templatedSubject := tmpl(n.conf.Subject)
+	if n.conf.Subject != "" || templatedSubject != "" {


I'm a bit confused by this validation - you're only using templatedSubject, but your conditional says that if either of - why do we care about n.conf.Subject?

I'm thinking that it might be because tmpl(n.conf.Subject) can fail but in that case templatedSubject should be n.config.Subject as opposed to "" as it would be here.

You want to inspect err here to know whether it succeeded or not, but you can't because you haven't passed it over to createPublishInput - so I think we're gonna have to re-think the approach.

gotjosh · 2022-06-13T14:09:09Z

+func validateAndTruncateSubject(logger log.Logger, subject string, modifiedReasons *[]string) string {
+	if subject == "" {
+		*modifiedReasons = append(*modifiedReasons, fmt.Sprintf(ComponentAndModifiedReason, Subject, SubjectEmpty))
+		level.Warn(logger).Log("msg", "Subject has been modified because it is empty.", "originalSubject", subject)


Suggested change

level.Warn(logger).Log("msg", "Subject has been modified because it is empty.", "originalSubject", subject)

level.Warn(logger).Log("msg", "subject has been modified because it is empty")

No need to show an empty subject.

gotjosh · 2022-06-13T14:16:19Z

+
+	if !isASCIINonControl(subject) {
+		*modifiedReasons = append(*modifiedReasons, fmt.Sprintf(ComponentAndModifiedReason, Subject, SubjectContainsIllegalChars))
+		level.Warn(logger).Log("msg", "Subject has been modified because it contains control- or non-ASCII characters.", "originalSubject", subject)


Suggested change

level.Warn(logger).Log("msg", "Subject has been modified because it contains control- or non-ASCII characters.", "originalSubject", subject)

level.Warn(logger).Log("msg", "subject has been modified because it contains control- or non-ASCII characters", "subject", subject)

gotjosh · 2022-06-13T14:22:16Z

+	*modifiedReasons = append(*modifiedReasons, fmt.Sprintf(ComponentAndModifiedReason, Subject, fmt.Sprintf(SubjectSizeExceeded, charactersInSubject)))
+	return subject[:subjectSizeLimitInCharacters]


Suggested change

*modifiedReasons = append(*modifiedReasons, fmt.Sprintf(ComponentAndModifiedReason, Subject, fmt.Sprintf(SubjectSizeExceeded, charactersInSubject)))

return subject[:subjectSizeLimitInCharacters]

*modifiedReasons = append(*modifiedReasons, fmt.Sprintf(ComponentAndModifiedReason, Subject, fmt.Sprintf(SubjectSizeExceeded, charactersInSubject)))

truncated := subject[:subjectSizeLimitInCharacters]

level.Warn(logger).Log("msg", "subject has been modified because it exceeds the number of characters allowed", "subject", subject, "truncated", truncated)

SuperQ · 2025-10-31T14:14:58Z

Hello from the Alertmanager bug scrub. This change needs a rebase.

add SNS receiver subject validation

4fea185

Signed-off-by: Yijie Qin <qinyijie@amazon.com>

qinxx108 force-pushed the sns-subject-validation branch from 79e2447 to 4fea185 Compare February 8, 2022 19:21

qinxx108 and others added 4 commits February 8, 2022 11:27

Merge branch 'main' of https://github.com/qinxx108/alertmanager into …

605cffc

…sns-subject-validation

fix merge conflict

895f511

Signed-off-by: Yijie Qin <qinyijie@amazon.com>

Added validation of ASCII control characters and improve handling of …

7ab84c7

…empty subject Signed-off-by: Soon-Ping Phang <soonping@amazon.com>

Fix for lint error

02f78a7

Signed-off-by: Soon-Ping Phang <soonping@amazon.com>

roidelapluie reviewed May 3, 2022

View reviewed changes

qinxx108 added 3 commits May 9, 2022 12:09

Merge branch 'prometheus:main' into sns-subject-validation

d9fb51f

fix comments

98887af

Signed-off-by: Yijie Qin <qinyijie@amazon.com>

trigger re-run

5eca050

Signed-off-by: Yijie Qin <qinyijie@amazon.com>

roidelapluie approved these changes May 23, 2022

View reviewed changes

gotjosh reviewed Jun 13, 2022

View reviewed changes

gotjosh mentioned this pull request Jun 14, 2022

add SNS receiver validation #2808

Closed

SoloJacobs added the component/notify label Dec 12, 2025

siavashs added the stale label Feb 11, 2026

	level.Info(logger).Log("msg", "subject has been modified because it contains control- or non-ASCII characters", "originalSubject", subject)
	level.Warn(logger).Log("msg", "Subject has been modified because it contains control- or non-ASCII characters.", "originalSubject", subject)

	// Modified Message attribute value format
	// Modified message attribute format.

		@@ -43,3 +51,172 @@ func TestValidateAndTruncateMessage(t *testing.T) {
		_, _, err = validateAndTruncateMessage(invalidUtf8String, 100)

	_, _, err = validateAndTruncateMessage(invalidUtf8String, 100)
	const maxMessageSize = 100
	_, _, err = validateAndTruncateMessage(invalidUtf8String, maxMessageSize)

		modifiedReasons = append(modifiedReasons, fmt.Sprintf(ComponentAndModifiedReason, Subject, fmt.Sprintf(SubjectSizeExceeded, charactersInSubject)))
		return subject[:subjectSizeLimitInCharacters]

Conversation

qinxx108 commented Feb 8, 2022

Uh oh!

roidelapluie commented Apr 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roidelapluie left a comment

Choose a reason for hiding this comment

Uh oh!

gotjosh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qinxx108 Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SuperQ commented Oct 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

qinxx108 Dec 14, 2022 •

edited

Loading