improve emails and add preview system#500
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/web/Jordnaer/Features/Email/EmailContentBuilder.cs`:
- Around line 41-65: GroupInvite is passing encodedGroupName into
EmailTemplate.Wrap's preheaderText causing double-encoding; change the
preheaderText argument to use the raw groupName (leave
WebUtility.HtmlEncode(groupName) for the in-body bold name), so
EmailTemplate.Wrap receives the unencoded value just like GroupInviteNewUser
does, ensuring consistent encoding behavior across GroupInvite and
GroupInviteNewUser.
- Around line 32-39: PasswordResetCode inserts resetCode directly into the HTML;
defensively HTML-encode resetCode before passing it into the template to prevent
injection if format changes. Update EmailContentBuilder.PasswordResetCode to
call an HTML-encoding helper (e.g. System.Net.WebUtility.HtmlEncode) on the
resetCode and use the encoded value in the interpolated string passed to
EmailTemplate.Wrap (the Greeting call can remain unchanged).
- Around line 67-79: The preheader is being double-encoded because
ChatNotification computes encodedSenderName and passes it into
EmailTemplate.Wrap as preheaderText while Wrap will HTML-encode inputs again;
update ChatNotification to use the original senderDisplayName (not
encodedSenderName) for the preheaderText argument and keep encodedSenderName
only for the inline HTML body (and leave EmailTemplate.Wrap to perform encoding
for the preheader). Ensure references: ChatNotification, encodedSenderName,
EmailTemplate.Wrap, and the preheaderText parameter are the targets to change.
- Around line 147-162: The PartnerImageApproval method currently injects
changesListHtml directly into the <ul>, risking XSS; update PartnerImageApproval
to treat the entries as plain text by HTML-encoding each item (use
WebUtility.HtmlEncode) and wrapping them in <li> elements before joining (e.g.
map each item -> $"<li>{encoded}</li>" then string.Join), or alternatively
change the parameter name to indicate pre-sanitized HTML and document it —
ensure EmailTemplate.Wrap usage remains the same and update callers if you
change the parameter contract.
In `@src/web/Jordnaer/Features/Email/EmailService.cs`:
- Around line 257-265: The DisplayName for the EmailRecipient is being
HTML-encoded here while the email body builder
(EmailContentBuilder.PartnerWelcome) already encodes the partner name, risking
double-encoding; update the welcomeEmail construction to set
EmailRecipient.DisplayName to the raw partnerName (remove WebUtility.HtmlEncode)
and ensure any HTML encoding remains only in EmailContentBuilder.PartnerWelcome,
leaving the email header value unencoded.
🧹 Nitpick comments (2)
src/web/Jordnaer/Features/Email/EmailTemplate.cs (1)
72-86: Consider validatingbackgroundColorto prevent CSS injection.The
backgroundColorparameter is inserted directly into the style attribute without validation. While this is likely called with trusted values internally, consider adding validation to ensure it's a valid hex color or from an allowed list.💡 Optional: Add color validation
public static string Button(string href, string text, string? backgroundColor = null) { - var bgColor = backgroundColor ?? "#dbab45"; + var bgColor = IsValidHexColor(backgroundColor) ? backgroundColor : "#dbab45"; var encodedHref = WebUtility.HtmlEncode(href); var encodedText = WebUtility.HtmlEncode(text); // ... } + +private static bool IsValidHexColor(string? color) => + !string.IsNullOrEmpty(color) && + System.Text.RegularExpressions.Regex.IsMatch(color, @"^#[0-9A-Fa-f]{6}$");src/web/Jordnaer/Features/Email/EmailService.cs (1)
192-193: Redundant approvalUrl variable.The
approvalUrlvariable is constructed here but is not used directly - it's reconstructed insideEmailContentBuilder.PartnerImageApproval. Consider removing this unused variable.♻️ Suggested cleanup
- var baseUrl = options.Value.BaseUrl; - var approvalUrl = $"{baseUrl}/backoffice/partners/{partnerId}"; + var baseUrl = options.Value.BaseUrl;
No description provided.