Validate the user invitation HTML templates #4835

rdimitrov · 2024-10-28T09:42:04Z

Summary

The following PR:

Adds validation for the user invitation HTML templates against HTML and JS injection
Adds email validation for the user invitation feature based on RFC 5322
Adds unit tests

Change Type

Mark the type of change your PR introduces:

Bug fix (resolves an issue without affecting existing features)
Feature (adds new functionality without breaking changes)
Breaking change (may impact existing functionalities or require documentation updates)
Documentation (updates or additions to documentation)
Refactoring or test improvements (no bug fixes or new functionality)

Testing

Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.

Review Checklist:

Reviewed my own code for quality and clarity.
Added comments to complex or tricky code sections.
Updated any affected documentation.
Included tests that validate the fix or feature.
Checked that related changes are merged.

evankanderson · 2024-10-28T14:27:32Z

internal/email/email.go

 // 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
+func isValidField(str string) error {


I think this is also covered by html/template -- do we need both?

I decided to add it because html/template was not erroring if it finds html in the template input, instead it escapes it and so we end up with things like <script>

Can you add a comment that we want to detect and error, rather than escape?

Added a comment 👍

internal/email/email.go

coveralls · 2024-10-28T15:43:37Z

Pull Request Test Coverage Report for Build 11603399089

Details

67 of 89 (75.28%) changed or added relevant lines in 2 files are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.009%) to 55.216%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
internal/invites/service.go	0	2	0.0%
internal/email/email.go	67	87	77.01%

Files with Coverage Reduction	New Missed Lines	%
internal/email/email.go	2	75.0%

Totals
Change from base Build 11600690174:	0.009%
Covered Lines:	15462
Relevant Lines:	28003

💛 - Coveralls

evankanderson

Sorry, just got a chance to review this on computer rather than on phone, and I've got a few more comments.

internal/email/email.go

evankanderson · 2024-10-28T17:32:28Z

internal/email/email.go

 // 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
+func isValidField(str string) error {


Can you add a comment that we want to detect and error, rather than escape?

evankanderson · 2024-10-28T17:32:49Z

internal/email/email.go

+	}
+
+	// Check for JavaScript content separately
+	escapedJS := template.JSEscaper(str)


It looks like JSEscapeString should be simpler / faster here.

Do you mean that any potential JS code would have been wrapped into HTML tags and so this is rather obsolete/redundant or it's something else?

internal/email/email.go

internal/email/email_test.go

evankanderson · 2024-10-28T17:41:28Z

internal/invites/service.go

+
+// isEmail is used to validate if the email is a valid email address according to RFC 5322
+func isEmail(emailString string) bool {
+	_, err := mail.ParseAddress(emailString)


Should we be using only the address part for the rest of the function? I'm not sure we've thought about "Joe Bob <[email protected]>" and "Oh gosh, I really dislike you <[email protected]>" in the rest of our code.

(If so, we could just return string or "")

We can, but do we want to support such kind of inputs? For now at least I assume it's much cleaner to fail if the input is not strictly an email.

My point is that mail.ParseAddress will accept "An unpleasant joke from recent politics <[email protected]>", and the Name field will be set to "An unpleasant joke from recent politics".

(And I was suggesting we should return just the parsed.Address part of the ParseAddress result.

See https://github.com/bufbuild/protovalidate-go/blob/1446f1e1d576e67382edcb4863f76414e2eb2256/celext/lib.go#L426 for fairly-complete email address validation.

(And, I'm okay removing this bit of code in favor of #4840 )

I decided to remove it in favour of the validation checks we'll introduce 👍

evankanderson

A few nits (which you can ignore) aligning with what I think of as idiomatic go, and one bit on isEmail that we probably do want to fix.

evankanderson · 2024-10-30T13:38:35Z

internal/email/email.go

+	err = data.Validate()
+	if err != nil {


Suggested change

err = data.Validate()

if err != nil {

if err := data.Validate(); err != nil {

Applied 👍

evankanderson · 2024-10-30T13:39:19Z

internal/email/email.go

+		return nil, err
+	}
+	// Create the HTML and text bodies
+	strBodyHTML, err := data.GetEmailBodyHTML(ctx)


Oh, these are really nice as methods on the data struct!

internal/email/email.go

internal/email/email_test.go

evankanderson · 2024-10-30T13:45:22Z

internal/email/email_test.go

+
+	tests := []struct {
+		input          bodyData
+		expectedErr    bool


Do you need expectedErr separately from expectedErrMsg != ""?

Thanks 👍 Removed the obsolete bool for expected error 👍

evankanderson · 2024-10-30T13:48:45Z

internal/invites/service.go

+
+// isEmail is used to validate if the email is a valid email address according to RFC 5322
+func isEmail(emailString string) bool {
+	_, err := mail.ParseAddress(emailString)


My point is that mail.ParseAddress will accept "An unpleasant joke from recent politics <[email protected]>", and the Name field will be set to "An unpleasant joke from recent politics".

(And I was suggesting we should return just the parsed.Address part of the ParseAddress result.

Signed-off-by: Radoslav Dimitrov <[email protected]>

…idation Signed-off-by: Radoslav Dimitrov <[email protected]>

rdimitrov requested a review from a team as a code owner October 28, 2024 09:42

rdimitrov self-assigned this Oct 28, 2024

rdimitrov force-pushed the validate-invite-html branch 3 times, most recently from 0594a87 to 062e537 Compare October 28, 2024 10:50

evankanderson reviewed Oct 28, 2024

View reviewed changes

rdimitrov force-pushed the validate-invite-html branch 3 times, most recently from b9869c6 to 91f14ac Compare October 29, 2024 23:51

evankanderson reviewed Oct 30, 2024

View reviewed changes

rdimitrov added 7 commits October 31, 2024 01:30

Validate the user invitation HTML templates

5806216

Signed-off-by: Radoslav Dimitrov <[email protected]>

Add email validation for the authz service

2cbbba4

Signed-off-by: Radoslav Dimitrov <[email protected]>

Add an additional validation for the template data source

040242a

Signed-off-by: Radoslav Dimitrov <[email protected]>

Simplify the data template validation

21139bc

Signed-off-by: Radoslav Dimitrov <[email protected]>

Add error handling for getting the generated text body

fd2f366

Signed-off-by: Radoslav Dimitrov <[email protected]>

Fix unit tests, remove unnecessary empty string check

f0dc02e

Signed-off-by: Radoslav Dimitrov <[email protected]>

Add length validation checks and remove duplicated error logs

1146641

Signed-off-by: Radoslav Dimitrov <[email protected]>

rdimitrov force-pushed the validate-invite-html branch from 91f14ac to 2e3ae2f Compare October 30, 2024 23:40

Reformat code and remove email check in favour of upcomming proto val…

1b26070

…idation Signed-off-by: Radoslav Dimitrov <[email protected]>

rdimitrov force-pushed the validate-invite-html branch from 2e3ae2f to 1b26070 Compare October 30, 2024 23:42

rdimitrov requested a review from evankanderson October 31, 2024 10:27

Merge branch 'main' into validate-invite-html

9cce531

evankanderson approved these changes Oct 31, 2024

View reviewed changes

rdimitrov merged commit df4a1ea into main Oct 31, 2024
25 checks passed

rdimitrov deleted the validate-invite-html branch October 31, 2024 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate the user invitation HTML templates #4835

Validate the user invitation HTML templates #4835

rdimitrov commented Oct 28, 2024 •

edited

Loading

evankanderson Oct 28, 2024

rdimitrov Oct 28, 2024

evankanderson Oct 28, 2024

rdimitrov Oct 29, 2024

coveralls commented Oct 28, 2024 •

edited

Loading

evankanderson left a comment

evankanderson Oct 28, 2024

evankanderson Oct 28, 2024

rdimitrov Oct 29, 2024

evankanderson Oct 28, 2024

evankanderson Oct 28, 2024

rdimitrov Oct 29, 2024

evankanderson Oct 30, 2024

evankanderson Oct 30, 2024

evankanderson Oct 30, 2024

rdimitrov Oct 31, 2024

evankanderson left a comment

evankanderson Oct 30, 2024

rdimitrov Oct 31, 2024

evankanderson Oct 30, 2024

evankanderson Oct 30, 2024

rdimitrov Oct 31, 2024

evankanderson Oct 30, 2024

	err = data.Validate()
	if err != nil {
	if err := data.Validate(); err != nil {

Validate the user invitation HTML templates #4835

Validate the user invitation HTML templates #4835

Conversation

rdimitrov commented Oct 28, 2024 • edited Loading

Summary

Change Type

Testing

Review Checklist:

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

coveralls commented Oct 28, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11603399089

Details

💛 - Coveralls

evankanderson 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

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

evankanderson 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdimitrov commented Oct 28, 2024 •

edited

Loading

coveralls commented Oct 28, 2024 •

edited

Loading