Validate the user invitation HTML templates (#4835)

* Validate the user invitation HTML templates Signed-off-by: Radoslav Dimitrov <[email protected]> * Add email validation for the authz service Signed-off-by: Radoslav Dimitrov <[email protected]> * Add an additional validation for the template data source Signed-off-by: Radoslav Dimitrov <[email protected]> * Simplify the data template validation Signed-off-by: Radoslav Dimitrov <[email protected]> * Add error handling for getting the generated text body Signed-off-by: Radoslav Dimitrov <[email protected]> * Fix unit tests, remove unnecessary empty string check Signed-off-by: Radoslav Dimitrov <[email protected]> * Add length validation checks and remove duplicated error logs Signed-off-by: Radoslav Dimitrov <[email protected]> * Reformat code and remove email check in favour of upcomming proto validation Signed-off-by: Radoslav Dimitrov <[email protected]> --------- Signed-off-by: Radoslav Dimitrov <[email protected]>
mindersec · Oct 31, 2024 · df4a1ea · df4a1ea
1 parent 393e01f
commit df4a1ea
Show file tree

Hide file tree

Showing 5 changed files with 312 additions and 52 deletions.
diff --git a/go.mod b/go.mod
@@ -281,7 +281,7 @@ require (
 	github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect
 	github.com/digitorus/pkcs7 v0.0.0-20230818184609-3a137a874352 // indirect
 	github.com/digitorus/timestamp v0.0.0-20231217203849-220c5c2851b7 // indirect
-	github.com/docker/cli v27.3.1+incompatible
+	github.com/docker/cli v27.3.1+incompatible // indirect
 	github.com/docker/distribution v2.8.3+incompatible // indirect
 	github.com/docker/docker v27.3.1+incompatible // indirect
 	github.com/docker/docker-credential-helpers v0.8.2 // indirect

diff --git a/internal/email/email.go b/internal/email/email.go
@@ -8,14 +8,15 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"html/template"
+	"reflect"
 	"strings"
 
 	"github.com/ThreeDotsLabs/watermill/message"
-	"github.com/docker/cli/templates"
 	"github.com/google/uuid"
-	"github.com/rs/zerolog"
 
 	"github.com/mindersec/minder/internal/authz"
+	"github.com/mindersec/minder/internal/util"
 )
 
 const (
@@ -25,6 +26,10 @@ const (
 	DefaultMinderTermsURL = "https://stacklok.com/stacklok-terms-of-service"
 	// DefaultMinderPrivacyURL is the default privacy URL for minder
 	DefaultMinderPrivacyURL = "https://stacklok.com/privacy-policy/"
+	// BodyMaxLength is the maximum length of the email body
+	BodyMaxLength = 10000
+	// MaxFieldLength is the maximum length of a string field
+	MaxFieldLength = 200
 )
 
 // MailEventPayload is the event payload for sending an invitation email
@@ -35,6 +40,19 @@ type MailEventPayload struct {
 	BodyText string `json:"body_text"`
 }
 
+type bodyData struct {
+	AdminName        string
+	OrganizationName string
+	InvitationURL    string
+	RecipientEmail   string
+	MinderURL        string
+	TermsURL         string
+	PrivacyURL       string
+	SignInURL        string
+	RoleName         string
+	RoleVerb         string
+}
+
 // NewMessage creates a new message for sending an invitation email
 func NewMessage(
 	ctx context.Context,
@@ -43,15 +61,45 @@ func NewMessage(
 	// Generate a new message UUID
 	id, err := uuid.NewUUID()
 	if err != nil {
-		return nil, fmt.Errorf("error generating UUID: %w", err)
+		return nil, err
 	}
 
+	// Populate the template data source
+	data := bodyData{
+		AdminName:        sponsorDisplay,
+		OrganizationName: projectDisplay,
+		InvitationURL:    inviteURL,
+		RecipientEmail:   inviteeEmail,
+		MinderURL:        minderURLBase,
+		TermsURL:         DefaultMinderTermsURL,
+		PrivacyURL:       DefaultMinderPrivacyURL,
+		SignInURL:        minderURLBase,
+		RoleName:         role,
+		RoleVerb:         authz.AllRolesVerbs[authz.Role(role)],
+	}
+	// Validate the data source template for HTML injection attacks or empty fields
+	if err = data.Validate(); err != nil {
+		return nil, err
+	}
+	// Create the HTML and text bodies
+	strBodyHTML, err := data.GetEmailBodyHTML(ctx)
+	if err != nil {
+		return nil, err
+	}
+	strBodyText, err := data.GetEmailBodyText(ctx)
+	if err != nil {
+		return nil, err
+	}
+	strSubject, err := data.GetEmailSubject()
+	if err != nil {
+		return nil, err
+	}
 	// Create the payload
 	payload, err := json.Marshal(MailEventPayload{
 		Address:  inviteeEmail,
-		Subject:  getEmailSubject(projectDisplay),
-		BodyHTML: getEmailBodyHTML(ctx, inviteURL, minderURLBase, sponsorDisplay, projectDisplay, role, inviteeEmail),
-		BodyText: getEmailBodyText(inviteURL, sponsorDisplay, projectDisplay, role),
+		Subject:  strSubject,
+		BodyHTML: strBodyHTML,
+		BodyText: strBodyText,
 	})
 	if err != nil {
 		return nil, fmt.Errorf("error marshalling payload for email event: %w", err)
@@ -61,56 +109,84 @@ func NewMessage(
 	return message.NewMessage(id.String(), payload), nil
 }
 
-// getBodyHTML returns the HTML body for the email based on the message payload
-func getEmailBodyHTML(ctx context.Context, inviteURL, minderURL, sponsor, project, role, inviteeEmail string) string {
-	data := struct {
-		AdminName        string
-		OrganizationName string
-		InvitationURL    string
-		RecipientEmail   string
-		MinderURL        string
-		TermsURL         string
-		PrivacyURL       string
-		SignInURL        string
-		RoleName         string
-		RoleVerb         string
-	}{
-		AdminName:        sponsor,
-		OrganizationName: project,
-		InvitationURL:    inviteURL,
-		RecipientEmail:   inviteeEmail,
-		MinderURL:        minderURL,
-		TermsURL:         DefaultMinderTermsURL,
-		PrivacyURL:       DefaultMinderPrivacyURL,
-		SignInURL:        minderURL,
-		RoleName:         role,
-		RoleVerb:         authz.AllRolesVerbs[authz.Role(role)],
+// GetEmailBodyHTML returns the HTML body for the email based on the message payload.
+// If there is an error creating the HTML body, it will try to return the text body instead
+func (b *bodyData) GetEmailBodyHTML(ctx context.Context) (string, error) {
+	var builder strings.Builder
+	bHTML := bodyHTML
+
+	bodyHTMLTmpl, err := util.NewSafeHTMLTemplate(&bHTML, "body-invite-html")
+	if err != nil {
+		return "", err
 	}
+	err = bodyHTMLTmpl.Execute(ctx, &builder, b, BodyMaxLength)
+	if err != nil {
+		return "", err
+	}
+	return builder.String(), nil
+}
 
-	// TODO: Load the email template from elsewhere
+// GetEmailBodyText returns the text body for the email based on the message payload
+func (b *bodyData) GetEmailBodyText(ctx context.Context) (string, error) {
+	var builder strings.Builder
+	bText := bodyText
 
-	// Parse the template
-	tmpl, err := templates.Parse(bodyHTML)
+	bodyTextTmpl, err := util.NewSafeHTMLTemplate(&bText, "body-invite-text")
 	if err != nil {
-		zerolog.Ctx(ctx).Error().Err(err).Msg("error parsing the HTML template for email invitations")
-		// Default to the text body
-		return getEmailBodyText(inviteURL, sponsor, project, role)
+		return "", err
 	}
-	// Execute the template
-	var b strings.Builder
-	if err := tmpl.Execute(&b, data); err != nil {
-		return ""
+	err = bodyTextTmpl.Execute(ctx, &builder, b, BodyMaxLength)
+	if err != nil {
+		return "", err
 	}
-	return b.String()
+	return builder.String(), nil
 }
 
-// getEmailBodyText returns the text body for the email based on the message payload
-func getEmailBodyText(inviteURL, sponsor, project, role string) string {
-	return fmt.Sprintf("You have been invited to join %s as %s by %s. Visit %s to accept the invitation.",
-		project, role, sponsor, inviteURL)
+// GetEmailSubject returns the subject for the email based on the message payload
+func (b *bodyData) GetEmailSubject() (string, error) {
+	err := isValidField(b.OrganizationName)
+	if err != nil {
+		return "", err
+	}
+	return fmt.Sprintf("You have been invited to join the %s organization in Minder", b.OrganizationName), nil
 }
 
-// getEmailSubject returns the subject for the email based on the message payload
-func getEmailSubject(project string) string {
-	return fmt.Sprintf("You have been invited to join the %s organization in Minder", project)
+// isValidField checks if the string is empty or contains HTML or JavaScript injection
+// If we detect any HTML or JavaScript injection, we want to return an error rather than escaping the content
+func isValidField(str string) error {
+	// Check string length
+	if len(str) > MaxFieldLength {
+		return fmt.Errorf("field value %s is more than %d characters", str, MaxFieldLength)
+	}
+	// Check for HTML content
+	escapedHTML := template.HTMLEscapeString(str)
+	if escapedHTML != str {
+		return fmt.Errorf("string %s contains HTML injection", str)
+	}
+
+	// Check for JavaScript content separately
+	escapedJS := template.JSEscaper(str)
+	if escapedJS != str {
+		return fmt.Errorf("string %s contains JavaScript injection", str)
+	}
+
+	return nil
+}
+
+// Validate validates the template data source for HTML injection attacks
+func (b *bodyData) Validate() error {
+	v := reflect.ValueOf(b).Elem()
+	// Iterate over the fields of the struct
+	for i := 0; i < v.NumField(); i++ {
+		field := v.Field(i)
+		// Check if the field is settable and of kind and type string
+		if !field.CanSet() || field.Kind() != reflect.String || field.Type() != reflect.TypeOf("") {
+			return fmt.Errorf("field %s is not settable or a string", v.Type().Field(i).Name)
+		}
+		err := isValidField(field.String())
+		if err != nil {
+			return fmt.Errorf("field %s failed validation - %s", v.Type().Field(i).Name, field.String())
+		}
+	}
+	return nil
 }
diff --git a/internal/email/email_test.go b/internal/email/email_test.go
@@ -0,0 +1,159 @@
+// SPDX-FileCopyrightText: Copyright 2024 The Minder Authors
+// SPDX-License-Identifier: Apache-2.0
+
+package email
+
+import (
+	"fmt"
+	"strings"
+	"testing"
+)
+
+func TestIsValidField(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		input          string
+		expectedErrMsg string
+	}{
+		// Test case 1: Valid plain text
+		{"Just plain text", ""},
+
+		// Test case 2: String with HTML tags
+		{"<b>Bold Text</b>", "string <b>Bold Text</b> contains HTML injection"},
+
+		// Test case 3: String with HTML entity
+		{"This is a test &amp; example.", "string This is a test &amp; example. contains HTML injection"},
+
+		// Test case 4: String with multiple HTML entities
+		{"This &amp; that &lt; should &gt; work.", "string This &amp; that &lt; should &gt; work. contains HTML injection"},
+
+		// Test case 5: Valid URL (no HTML or JavaScript injection)
+		{"https://example.com", ""},
+
+		// Test case 6: Mixed content with HTML and JS
+		{"Hello <b>World</b> onload=alert('test');", "string Hello <b>World</b> onload=alert('test'); contains HTML injection"},
+
+		// Test case 7: HTML-style comment
+		{"<!-- This is a comment -->", "string <!-- This is a comment --> contains HTML injection"},
+
+		// Test case 8: ensure allowed length is less than 200 characters
+		{strings.Repeat("a", MaxFieldLength+1), fmt.Sprintf("field value %s is more than %d characters", strings.Repeat("a", MaxFieldLength+1), MaxFieldLength)},
+	}
+
+	for _, tt := range tests {
+		tt := tt // capture range variable for parallel execution
+		t.Run(tt.input, func(t *testing.T) {
+			t.Parallel()
+			err := isValidField(tt.input)
+			if err != nil && err.Error() != tt.expectedErrMsg {
+				t.Errorf("isValidField(%q) got error message: %v, expected message: %v", tt.input, err.Error(), tt.expectedErrMsg)
+			}
+		})
+	}
+}
+
+func TestValidateDataSourceTemplate(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		input          bodyData
+		expectedErrMsg string
+	}{
+		// Test case 1: All fields are valid plain text
+		{
+			bodyData{
+				AdminName:        "John Doe",
+				OrganizationName: "Acme Corp",
+				InvitationURL:    "https://invitation.com",
+				RecipientEmail:   "[email protected]",
+				MinderURL:        "https://minder.com",
+				TermsURL:         "https://terms.com",
+				PrivacyURL:       "https://privacy.com",
+				SignInURL:        "https://signin.com",
+				RoleName:         "Administrator",
+				RoleVerb:         "manage",
+			},
+			"",
+		},
+
+		// Test case 2: AdminName contains HTML tags
+		{
+			bodyData{
+				AdminName:        "John <b>Doe</b>",
+				OrganizationName: "Acme Corp",
+				InvitationURL:    "https://invitation.com",
+				RecipientEmail:   "[email protected]",
+				MinderURL:        "https://minder.com",
+				TermsURL:         "https://terms.com",
+				PrivacyURL:       "https://privacy.com",
+				SignInURL:        "https://signin.com",
+				RoleName:         "Administrator",
+				RoleVerb:         "manage",
+			},
+			"field AdminName failed validation - John <b>Doe</b>",
+		},
+
+		// Test case 3: OrganizationName contains HTML content
+		{
+			bodyData{
+				AdminName:        "John Doe",
+				OrganizationName: "<script>alert('Hack');</script>",
+				InvitationURL:    "https://invitation.com",
+				RecipientEmail:   "[email protected]",
+				MinderURL:        "https://minder.com",
+				TermsURL:         "https://terms.com",
+				PrivacyURL:       "https://privacy.com",
+				SignInURL:        "https://signin.com",
+				RoleName:         "Administrator",
+				RoleVerb:         "manage",
+			},
+			"field OrganizationName failed validation - <script>alert('Hack');</script>",
+		},
+
+		// Test case 4: AdminName contains JavaScript code
+		{
+			bodyData{
+				AdminName:        "onload=alert('test')",
+				OrganizationName: "Acme Corp",
+				InvitationURL:    "https://invitation.com",
+				RecipientEmail:   "[email protected]",
+				MinderURL:        "https://minder.com",
+				TermsURL:         "https://terms.com",
+				PrivacyURL:       "https://privacy.com",
+				SignInURL:        "https://signin.com",
+				RoleName:         "Administrator",
+				RoleVerb:         "manage",
+			},
+			"field AdminName failed validation - onload=alert('test')",
+		},
+
+		// Test case 5: All fields contain valid plain text with some URLs
+		{
+			bodyData{
+				AdminName:        "Plain Text User",
+				OrganizationName: "No HTML Corp",
+				InvitationURL:    "https://example.com",
+				RecipientEmail:   "[email protected]",
+				MinderURL:        "https://example.com/minder",
+				TermsURL:         "https://example.com/terms",
+				PrivacyURL:       "https://example.com/privacy",
+				SignInURL:        "https://example.com/signin",
+				RoleName:         "User",
+				RoleVerb:         "view",
+			},
+			"",
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt // capture range variable for parallel execution
+		t.Run(tt.input.AdminName, func(t *testing.T) {
+			t.Parallel()
+			err := tt.input.Validate()
+			if err != nil && err.Error() != tt.expectedErrMsg {
+				t.Errorf("validateDataSourceTemplate(%+v) got error message: %v, expected message: %v", tt.input, err.Error(), tt.expectedErrMsg)
+			}
+		})
+	}
+}