Fix for #1350: switch SMTP email sending to MailKit #4167
Conversation
WalkthroughThis change migrates email sending from System.Net.Mail.SmtpClient to MailKit, introducing a new MailKitEmailHelper class that converts MailMessage to MimeMessage and handles SMTP transmission, with corresponding updates to email sender implementations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
ihouvet
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
shesha-core/src/Shesha.Application/Email/MailKitEmailHelper.cs (2)
142-155: Consider handling null attachment name.The
attachment.Nameproperty could potentially be null or empty. While MailKit likely handles this gracefully, explicitly handling it would make the code more defensive.private static MimeEntity CreateAttachmentPart(Attachment attachment) { var mediaType = attachment.ContentType.MediaType ?? "application"; var mediaSubType = attachment.ContentType.MediaSubtype ?? "octet-stream"; var mimePart = new MimePart(mediaType, mediaSubType) { Content = new MimeContent(CopyToMemoryStream(attachment.ContentStream)), ContentDisposition = new ContentDisposition(ContentDisposition.Attachment), ContentTransferEncoding = ContentEncoding.Base64, - FileName = attachment.Name + FileName = string.IsNullOrWhiteSpace(attachment.Name) ? "attachment" : attachment.Name }; return mimePart; }
157-171: Review FileName assignment for linked resources.Using
ContentIdasFileName(line 167) may not be ideal, as ContentId is typically an identifier like "image001@domain.com" rather than a user-friendly filename. Consider using a more descriptive default name.private static MimeEntity CreateLinkedResource(LinkedResource resource) { var mediaType = resource.ContentType.MediaType ?? "application"; var mediaSubType = resource.ContentType.MediaSubtype ?? "octet-stream"; var mimePart = new MimePart(mediaType, mediaSubType) { Content = new MimeContent(CopyToMemoryStream(resource.ContentStream)), ContentDisposition = new ContentDisposition(ContentDisposition.Inline), ContentTransferEncoding = ContentEncoding.Base64, ContentId = string.IsNullOrWhiteSpace(resource.ContentId) ? MimeUtils.GenerateMessageId() : resource.ContentId, - FileName = resource.ContentId + FileName = $"linked-resource.{mediaSubType}" }; return mimePart; }shesha-core/src/Shesha.Application/Shesha.Application.csproj (1)
169-169: Consider updating MailKit to the latest stable version.MailKit 4.14.1 is the latest stable version, while the codebase is pinned to 4.7.0. No security advisories exist for MailKit, but updating would provide access to bug fixes and improvements, including NTLM mechanism re-addition, IMAP token parsing fixes, and fallback logic for specific email providers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
shesha-core/src/Shesha.Application/Email/MailKitEmailHelper.cs(1 hunks)shesha-core/src/Shesha.Application/Email/SheshaEmailSender.cs(4 hunks)shesha-core/src/Shesha.Application/Notifications/Emails/EmailChannelSender.cs(2 hunks)shesha-core/src/Shesha.Application/Shesha.Application.csproj(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-05T17:11:17.267Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 3678
File: shesha-core/src/Shesha.Application/Otp/Configuration/OtpDefaults.cs:14-15
Timestamp: 2025-08-05T17:11:17.267Z
Learning: In the Shesha framework, the DefaultEmailBodyTemplate in OtpDefaults.cs contains a hard-coded localhost URL by design. This serves as a default that gets updated/overridden in production environments through their existing configuration management process.
Applied to files:
shesha-core/src/Shesha.Application/Notifications/Emails/EmailChannelSender.cs
🧬 Code graph analysis (3)
shesha-core/src/Shesha.Application/Email/SheshaEmailSender.cs (1)
shesha-core/src/Shesha.Application/Email/MailKitEmailHelper.cs (2)
MailKitEmailHelper(17-227)Send(116-132)
shesha-core/src/Shesha.Application/Email/MailKitEmailHelper.cs (2)
shesha-core/src/Shesha.Application/Email/SheshaEmailSender.cs (2)
MailMessage(138-169)Task(69-82)shesha-core/src/Shesha.Application/Notifications/Emails/EmailChannelSender.cs (4)
MailMessage(109-169)Task(33-36)Task(38-77)Task(85-98)
shesha-core/src/Shesha.Application/Notifications/Emails/EmailChannelSender.cs (1)
shesha-core/src/Shesha.Application/Email/MailKitEmailHelper.cs (1)
MailKitEmailHelper(17-227)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-attempt
🔇 Additional comments (6)
shesha-core/src/Shesha.Application/Notifications/Emails/EmailChannelSender.cs (1)
89-91: LGTM: Clean migration to MailKit.The switch to MailKit is straightforward with proper error handling and logging in place.
shesha-core/src/Shesha.Application/Email/SheshaEmailSender.cs (2)
80-81: LGTM: Async path correctly migrated.The async email sending path now properly uses MailKit through the helper class.
95-96: LGTM: Synchronous path correctly migrated.The synchronous email sending path now properly uses MailKit through the helper class.
shesha-core/src/Shesha.Application/Email/MailKitEmailHelper.cs (3)
19-96: LGTM: Comprehensive MailMessage to MimeMessage conversion.The conversion logic correctly handles addresses, headers, alternate views, attachments, and linked resources.
218-226: LGTM: Appropriate SSL/TLS configuration logic.The secure socket options correctly handle port 465 (implicit SSL) vs. other ports (explicit STARTTLS), following common SMTP conventions.
134-140: LGTM: Helper methods are correctly implemented.The remaining helper methods properly handle linked resources, mailbox addresses with display names, alternate view reading with encoding preservation, and credential creation with domain support.
Also applies to: 184-189, 191-206, 208-216
| public static async Task SendAsync(MimeMessage message, SmtpSettings settings, CancellationToken cancellationToken = default) | ||
| { | ||
| if (settings == null) | ||
| throw new ArgumentNullException(nameof(settings)); | ||
|
|
||
| using var smtpClient = new SmtpClient(); | ||
| await smtpClient.ConnectAsync(settings.Host, settings.Port, GetSecureSocketOption(settings), cancellationToken).ConfigureAwait(false); | ||
|
|
||
| var credential = CreateCredential(settings); | ||
| if (credential != null) | ||
| { | ||
| await smtpClient.AuthenticateAsync(credential, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| await smtpClient.SendAsync(message, cancellationToken).ConfigureAwait(false); | ||
| await smtpClient.DisconnectAsync(true, cancellationToken).ConfigureAwait(false); | ||
| } |
There was a problem hiding this comment.
Add timeout configuration for SMTP operations.
The SMTP client operations (Connect, Authenticate, Send, Disconnect) have no timeout configured, which can cause indefinite hangs if the SMTP server becomes unresponsive.
Consider adding a timeout parameter or using a reasonable default:
-public static async Task SendAsync(MimeMessage message, SmtpSettings settings, CancellationToken cancellationToken = default)
+public static async Task SendAsync(MimeMessage message, SmtpSettings settings, CancellationToken cancellationToken = default, int timeoutSeconds = 30)
{
if (settings == null)
throw new ArgumentNullException(nameof(settings));
- using var smtpClient = new SmtpClient();
+ using var smtpClient = new SmtpClient
+ {
+ Timeout = timeoutSeconds * 1000
+ };
await smtpClient.ConnectAsync(settings.Host, settings.Port, GetSecureSocketOption(settings), cancellationToken).ConfigureAwait(false);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static async Task SendAsync(MimeMessage message, SmtpSettings settings, CancellationToken cancellationToken = default) | |
| { | |
| if (settings == null) | |
| throw new ArgumentNullException(nameof(settings)); | |
| using var smtpClient = new SmtpClient(); | |
| await smtpClient.ConnectAsync(settings.Host, settings.Port, GetSecureSocketOption(settings), cancellationToken).ConfigureAwait(false); | |
| var credential = CreateCredential(settings); | |
| if (credential != null) | |
| { | |
| await smtpClient.AuthenticateAsync(credential, cancellationToken).ConfigureAwait(false); | |
| } | |
| await smtpClient.SendAsync(message, cancellationToken).ConfigureAwait(false); | |
| await smtpClient.DisconnectAsync(true, cancellationToken).ConfigureAwait(false); | |
| } | |
| public static async Task SendAsync(MimeMessage message, SmtpSettings settings, CancellationToken cancellationToken = default, int timeoutSeconds = 30) | |
| { | |
| if (settings == null) | |
| throw new ArgumentNullException(nameof(settings)); | |
| using var smtpClient = new SmtpClient | |
| { | |
| Timeout = timeoutSeconds * 1000 | |
| }; | |
| await smtpClient.ConnectAsync(settings.Host, settings.Port, GetSecureSocketOption(settings), cancellationToken).ConfigureAwait(false); | |
| var credential = CreateCredential(settings); | |
| if (credential != null) | |
| { | |
| await smtpClient.AuthenticateAsync(credential, cancellationToken).ConfigureAwait(false); | |
| } | |
| await smtpClient.SendAsync(message, cancellationToken).ConfigureAwait(false); | |
| await smtpClient.DisconnectAsync(true, cancellationToken).ConfigureAwait(false); | |
| } |
🤖 Prompt for AI Agents
In shesha-core/src/Shesha.Application/Email/MailKitEmailHelper.cs around lines
98 to 114, the SMTP operations have no timeout configured and can hang
indefinitely; set an operation timeout on the SmtpClient before connecting
(e.g., smtpClient.Timeout) using a timeout value from SmtpSettings or a sensible
default, and use a CancellationTokenSource.CancelAfter to enforce per-operation
timeouts by linking it with the provided cancellationToken for
ConnectAsync/AuthenticateAsync/SendAsync/DisconnectAsync calls so each call
fails fast when the SMTP server is unresponsive.
| public static void Send(MimeMessage message, SmtpSettings settings, CancellationToken cancellationToken = default) | ||
| { | ||
| if (settings == null) | ||
| throw new ArgumentNullException(nameof(settings)); | ||
|
|
||
| using var smtpClient = new SmtpClient(); | ||
| smtpClient.Connect(settings.Host, settings.Port, GetSecureSocketOption(settings), cancellationToken); | ||
|
|
||
| var credential = CreateCredential(settings); | ||
| if (credential != null) | ||
| { | ||
| smtpClient.Authenticate(credential, cancellationToken); | ||
| } | ||
|
|
||
| smtpClient.Send(message, cancellationToken); | ||
| smtpClient.Disconnect(true, cancellationToken); | ||
| } |
There was a problem hiding this comment.
Add timeout configuration for synchronous SMTP operations.
Similar to the async path, the synchronous SMTP operations lack timeout configuration.
-public static void Send(MimeMessage message, SmtpSettings settings, CancellationToken cancellationToken = default)
+public static void Send(MimeMessage message, SmtpSettings settings, CancellationToken cancellationToken = default, int timeoutSeconds = 30)
{
if (settings == null)
throw new ArgumentNullException(nameof(settings));
- using var smtpClient = new SmtpClient();
+ using var smtpClient = new SmtpClient
+ {
+ Timeout = timeoutSeconds * 1000
+ };
smtpClient.Connect(settings.Host, settings.Port, GetSecureSocketOption(settings), cancellationToken);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static void Send(MimeMessage message, SmtpSettings settings, CancellationToken cancellationToken = default) | |
| { | |
| if (settings == null) | |
| throw new ArgumentNullException(nameof(settings)); | |
| using var smtpClient = new SmtpClient(); | |
| smtpClient.Connect(settings.Host, settings.Port, GetSecureSocketOption(settings), cancellationToken); | |
| var credential = CreateCredential(settings); | |
| if (credential != null) | |
| { | |
| smtpClient.Authenticate(credential, cancellationToken); | |
| } | |
| smtpClient.Send(message, cancellationToken); | |
| smtpClient.Disconnect(true, cancellationToken); | |
| } | |
| public static void Send(MimeMessage message, SmtpSettings settings, CancellationToken cancellationToken = default, int timeoutSeconds = 30) | |
| { | |
| if (settings == null) | |
| throw new ArgumentNullException(nameof(settings)); | |
| using var smtpClient = new SmtpClient | |
| { | |
| Timeout = timeoutSeconds * 1000 | |
| }; | |
| smtpClient.Connect(settings.Host, settings.Port, GetSecureSocketOption(settings), cancellationToken); | |
| var credential = CreateCredential(settings); | |
| if (credential != null) | |
| { | |
| smtpClient.Authenticate(credential, cancellationToken); | |
| } | |
| smtpClient.Send(message, cancellationToken); | |
| smtpClient.Disconnect(true, cancellationToken); | |
| } |
🤖 Prompt for AI Agents
In shesha-core/src/Shesha.Application/Email/MailKitEmailHelper.cs around lines
116 to 132, the synchronous Send method never sets a timeout on the MailKit
SmtpClient; set the client's Timeout property before calling Connect (same place
the async path configures timeouts) using the configured SMTP timeout value
(e.g. settings.Timeout.TotalMilliseconds cast to int or
settings.TimeoutInMilliseconds) with a sensible fallback, so
Connect/Authenticate/Send will honor the configured timeout for synchronous
operations.
Summary
Testing
dotnet build shesha-core/src/Shesha.Application/Shesha.Application.csproj(fails: dotnet CLI not available in container)Codex Task
Summary by CodeRabbit