notify-mailer: Improve error checking during template execution (letsencrypt#5932)

beautifulentropy · web-flow · commit ecd6d0118c4d · 2022-02-08T09:53:51.000-08:00
- Break message body construction out into a testable method. - Ensure that in the event of a missing key, an informative error is returned instead of allowing the message to be populated with the zero value of the key. - Add message body construction tests for success, empty map, and missing key. - Comment the `recipient` struct and it's `Data` field to make it clear that SRE must be informed of any modifications. Fixes letsencrypt#5921
diff --git a/cmd/notify-mailer/main.go b/cmd/notify-mailer/main.go
@@ -102,6 +102,25 @@ func sortAddresses(input addressToRecipientMap) []string {
 	return addresses
 }
 
+// makeMessageBody is a helper for mailer.run() that's split out for the
+// purposes of testing.
+func (m *mailer) makeMessageBody(recipients []recipient) (string, error) {
+	var messageBody strings.Builder
+
+	// Ensure that in the event of a missing key, an informative error is
+	// returned.
+	m.emailTemplate.Option("missingkey=error")
+	err := m.emailTemplate.Execute(&messageBody, recipients)
+	if err != nil {
+		return "", err
+	}
+
+	if messageBody.Len() == 0 {
+		return "", errors.New("templating resulted in an empty message body")
+	}
+	return messageBody.String(), nil
+}
+
 func (m *mailer) run() error {
 	err := m.ok()
 	if err != nil {
@@ -161,17 +180,13 @@ func (m *mailer) run() error {
 		recipients := addressToRecipient[address]
 		m.logStatus(address, i+1, totalAddresses, startTime)
 
-		var messageBody strings.Builder
-		err = m.emailTemplate.Execute(&messageBody, recipients)
+		messageBody, err := m.makeMessageBody(recipients)
 		if err != nil {
-			return err
-		}
-
-		if messageBody.Len() == 0 {
-			return errors.New("message body was empty after interpolation")
+			m.log.Errf("Skipping %q due to templating error: %s", address, err)
+			continue
 		}
 
-		err = m.mailer.SendMail([]string{address}, m.subject, messageBody.String())
+		err = m.mailer.SendMail([]string{address}, m.subject, messageBody)
 		if err != nil {
 			var badAddrErr bmail.BadAddressSMTPError
 			if errors.As(err, &badAddrErr) {
@@ -253,11 +268,20 @@ func getAddressForID(id int64, dbMap dbSelector) ([]string, error) {
 	return addresses, nil
 }
 
-// recipient represents a single record from the recipient list file.
+// recipient represents a single record from the recipient list file. The 'id'
+// column is parsed to the 'id' field, all additional data will be parsed to a
+// mapping of column name to value in the 'Data' field. Please inform SRE if you
+// make any changes to the exported fields of this struct. These fields are
+// referenced in operationally critical e-mail templates used to notify
+// subscribers during incident response.
 type recipient struct {
+	// id is the subscriber's ID.
 	id int64
 
-	// Data is exported so the contents can be referenced from message template.
+	// Data is a mapping of column name to value parsed from a single record in
+	// the provided recipient list file. It's exported so the contents can be
+	// accessed by the the template package. Please inform SRE if you make any
+	// changes to this field.
 	Data map[string]string
 }
 
@@ -384,7 +408,7 @@ fields to be interpolated into the email template:
 The additional fields will be interpolated with Golang templating, e.g.:
 
   Your last issuance on each account was:
-		{{ range . }} {{ .Extra.lastIssuance }}
+		{{ range . }} {{ .Data.lastIssuance }}
 		{{ end }}
 
 To help the operator gain confidence in the mailing run before committing fully
diff --git a/cmd/notify-mailer/main_test.go b/cmd/notify-mailer/main_test.go
@@ -225,6 +225,53 @@ func TestReadRecipientListWithNoHeaderOrRecords(t *testing.T) {
 	test.AssertErrorIs(t, err, io.EOF)
 }
 
+func TestMakeMessageBody(t *testing.T) {
+	emailTemplate := `{{range . }}
+{{ .Data.date }}
+{{ .Data.domainName }}
+{{end}}`
+
+	m := &mailer{
+		log:           blog.UseMock(),
+		mailer:        &mocks.Mailer{},
+		emailTemplate: template.Must(template.New("email").Parse(emailTemplate)),
+		sleepInterval: 0,
+		targetRange:   interval{end: "\xFF"},
+		clk:           newFakeClock(t),
+		recipients:    nil,
+		dbMap:         mockEmailResolver{},
+	}
+
+	recipients := []recipient{
+		{id: 10, Data: map[string]string{"date": "2018-11-21", "domainName": "example.com"}},
+		{id: 23, Data: map[string]string{"date": "2018-11-22", "domainName": "example.net"}},
+	}
+
+	expectedMessageBody := `
+2018-11-21
+example.com
+
+2018-11-22
+example.net
+`
+
+	// Ensure that a very basic template with 2 recipients can be successfully
+	// executed.
+	messageBody, err := m.makeMessageBody(recipients)
+	test.AssertNotError(t, err, "failed to execute a valid template")
+	test.AssertEquals(t, messageBody, expectedMessageBody)
+
+	// With no recipients we should get an empty body error.
+	recipients = []recipient{}
+	_, err = m.makeMessageBody(recipients)
+	test.AssertError(t, err, "should have errored on empty body")
+
+	// With a missing key we should get an informative templating error.
+	recipients = []recipient{{id: 10, Data: map[string]string{"domainName": "example.com"}}}
+	_, err = m.makeMessageBody(recipients)
+	test.AssertEquals(t, err.Error(), "template: email:2:8: executing \"email\" at <.Data.date>: map has no entry for key \"date\"")
+}
+
 func TestSleepInterval(t *testing.T) {
 	const sleepLen = 10
 	mc := &mocks.Mailer{}
