Feature/1350/fix email settings not working with ssl #4294
Conversation
WalkthroughMigrate email sending from System.Net.Mail.SmtpClient to MailKit (v4.7.0). Introduce DTO and API endpoint for sending emails with attachments. Add MailKit helper class for MimeMessage construction and async/sync SMTP operations. Update existing email senders to use the new infrastructure. Changes
Sequence DiagramsequenceDiagram
actor User
participant API as EmailController
participant Sender as SheshaEmailSender
participant Helper as MailKitEmailHelper
participant MailKit as MailKit SmtpClient
participant SMTP as SMTP Server
User->>API: POST /api/Email/SendTestEmailWithAttachments<br/>(with attachments)
API->>API: Read attachments into memory<br/>Create EmailAttachment objects
API->>Sender: SendEmailAsync(email with attachments)
Sender->>Helper: ConvertToMimeMessage(mail)
Helper->>Helper: Map From/To/CC/BCC<br/>Build MimeMessage with<br/>HTML/Text bodies & attachments
Helper-->>Sender: MimeMessage
Sender->>Helper: SendAsync(mimeMessage, settings)
Helper->>MailKit: Connect & Authenticate
MailKit->>SMTP: Establish connection
Helper->>MailKit: Send(mimeMessage)
MailKit->>SMTP: Transmit email with attachments
SMTP-->>MailKit: Acknowledgement
Helper->>MailKit: Disconnect
Helper-->>Sender: Task completed
Sender-->>API: Success
API-->>User: Confirmation response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
shesha-core/src/Shesha.Application/Email/SheshaEmailSender.cs (1)
84-97: Consider propagating CancellationToken for cancellation support.The synchronous
SendEmailmethod doesn't accept or propagate aCancellationToken. While this matches the base class signature, consider if future enhancements should support cancellation for long-running SMTP operations.shesha-core/src/Shesha.Application/Notifications/Emails/EmailChannelSender.cs (1)
85-98: SMTP settings fetched multiple times - consider consolidating.
SmtpSettingsis fetched inBuildMessageWith(line 111) and again inSendEmailAsync(line 90). This creates redundant async calls and potential inconsistency if settings change between calls. Consider fetching settings once inSendAsyncand passing them through.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
shesha-core/src/Shesha.Application/Email/Dtos/EmailWithAttachmentsDto.cs(1 hunks)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)shesha-core/src/Shesha.Web.Host/Controllers/EmailController.cs(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
⏰ 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 (4)
shesha-core/src/Shesha.Application/Email/MailKitEmailHelper.cs (1)
104-120: LGTM!The async send implementation correctly handles connection, authentication (when credentials exist), sending, and disconnection with proper async/await patterns and ConfigureAwait(false).
shesha-core/src/Shesha.Application/Email/SheshaEmailSender.cs (1)
69-82: LGTM!The migration to MailKit is cleanly implemented. The flow maintains proper validation checks before conversion and correctly disposes the MimeMessage.
shesha-core/src/Shesha.Web.Host/Controllers/EmailController.cs (2)
31-42: Consider null-guarding only if using non-standard model binding or ifAttachmentsis not initialized in the DTOThe concern about
upload.Attachmentsbeingnullis unlikely with[ApiController]model binding, as ASP.NET Core typically initializes collection properties to empty collections when no files are posted. However, if the DTO does not initializeAttachmentsin its constructor (e.g.,public List<IFormFile> Attachments { get; set; } = new();), null checks would be prudent as a defensive measure.Individual
IFormFileitems are unlikely to be null within a bound collection. If null checks are added, verify whether this aligns with the DTO's actual initialization behavior and whether similar patterns exist elsewhere in the codebase.
24-27: Removeasynckeyword or addawaitto eliminate compiler warning
TestEmailAsyncis declared asasync Task<ActionResult>but the method body does not contain anyawaitexpressions. This triggers compiler warning CS1998 and misleads callers about the method's actual behavior.Either remove the
asynckeyword and rename toTestEmail:- public async Task<ActionResult> TestEmailAsync([FromForm] EmailWithAttachmentsDto upload) + public ActionResult TestEmail([FromForm] EmailWithAttachmentsDto upload)Or refactor to use async email-sending APIs (e.g.,
SendMailAsyncandCopyToAsyncinstead of synchronous variants) and addawaitcalls throughout.
| public class EmailWithAttachmentsDto: SendTestEmailInput | ||
| { | ||
| public string From { get; set; } | ||
| public List<IFormFile> Attachments { get; set; } = []; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add nullable annotation to From property.
The project has <Nullable>enable</Nullable>. The From property should be marked as string? if it can be null/empty, or initialized with a default value if required.
public class EmailWithAttachmentsDto: SendTestEmailInput
{
- public string From { get; set; }
+ public string? From { get; set; }
public List<IFormFile> Attachments { get; set; } = [];
}📝 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 class EmailWithAttachmentsDto: SendTestEmailInput | |
| { | |
| public string From { get; set; } | |
| public List<IFormFile> Attachments { get; set; } = []; | |
| } | |
| public class EmailWithAttachmentsDto: SendTestEmailInput | |
| { | |
| public string? From { get; set; } | |
| public List<IFormFile> Attachments { get; set; } = []; | |
| } |
🤖 Prompt for AI Agents
In shesha-core/src/Shesha.Application/Email/Dtos/EmailWithAttachmentsDto.cs
around lines 6 to 10, the From property lacks a nullable annotation under
Nullable enable; either change its type to string? if it can be null/empty, or
assign a non-null default (for example = string.Empty) to keep it non-nullable;
update the declaration accordingly and ensure project nullable context remains
consistent.
| using MailKit.Security; | ||
| using MimeKit; | ||
| using MimeKit.Utils; | ||
| using NUglify; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused import.
NUglify is imported but not used anywhere in this file.
-using NUglify;📝 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.
| using NUglify; |
🤖 Prompt for AI Agents
In shesha-core/src/Shesha.Application/Email/MailKitEmailHelper.cs around line 5,
the using directive for NUglify is unused; remove the line "using NUglify;" from
the top of the file and save, then rebuild or run static analysis to ensure no
other references remain.
| private static MimeEntity CreateLinkedResource(LinkedResource resource) | ||
| { | ||
| var contentType = resource.ContentType.MediaType; | ||
| var mediaType = contentType.Split('/')[0] ?? "application"; | ||
| var mediaSubType = contentType.Split('/')[1] ?? "octet-stream"; | ||
| using MemoryStream memoryStream = CopyToMemoryStream(resource.ContentStream); | ||
| var mimePart = new MimePart(mediaType, mediaSubType) | ||
| { | ||
| Content = new MimeContent(memoryStream), | ||
| ContentDisposition = new ContentDisposition(ContentDisposition.Inline), | ||
| ContentTransferEncoding = ContentEncoding.Base64, | ||
| ContentId = string.IsNullOrWhiteSpace(resource.ContentId) ? MimeUtils.GenerateMessageId() : resource.ContentId, | ||
| FileName = resource.ContentId | ||
| }; | ||
|
|
||
| // The created MemoryStream is now owned by MimeContent, which will dispose it when disposed. | ||
| return mimePart; | ||
| } |
There was a problem hiding this comment.
Critical: using disposes MemoryStream before MimeContent can use it.
The using keyword on line 171 causes the MemoryStream to be disposed when the method returns, but MimeContent requires the stream to remain open for later serialization. This will cause runtime failures when sending emails with linked resources.
private static MimeEntity CreateLinkedResource(LinkedResource resource)
{
var contentType = resource.ContentType.MediaType;
var mediaType = contentType.Split('/')[0] ?? "application";
var mediaSubType = contentType.Split('/')[1] ?? "octet-stream";
- using MemoryStream memoryStream = CopyToMemoryStream(resource.ContentStream);
+ MemoryStream memoryStream = CopyToMemoryStream(resource.ContentStream);
var mimePart = new MimePart(mediaType, mediaSubType)
{
Content = new MimeContent(memoryStream),
ContentDisposition = new ContentDisposition(ContentDisposition.Inline),
ContentTransferEncoding = ContentEncoding.Base64,
ContentId = string.IsNullOrWhiteSpace(resource.ContentId) ? MimeUtils.GenerateMessageId() : resource.ContentId,
FileName = resource.ContentId
};
// The created MemoryStream is now owned by MimeContent, which will dispose it when disposed.
return mimePart;
}📝 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.
| private static MimeEntity CreateLinkedResource(LinkedResource resource) | |
| { | |
| var contentType = resource.ContentType.MediaType; | |
| var mediaType = contentType.Split('/')[0] ?? "application"; | |
| var mediaSubType = contentType.Split('/')[1] ?? "octet-stream"; | |
| using MemoryStream memoryStream = CopyToMemoryStream(resource.ContentStream); | |
| var mimePart = new MimePart(mediaType, mediaSubType) | |
| { | |
| Content = new MimeContent(memoryStream), | |
| ContentDisposition = new ContentDisposition(ContentDisposition.Inline), | |
| ContentTransferEncoding = ContentEncoding.Base64, | |
| ContentId = string.IsNullOrWhiteSpace(resource.ContentId) ? MimeUtils.GenerateMessageId() : resource.ContentId, | |
| FileName = resource.ContentId | |
| }; | |
| // The created MemoryStream is now owned by MimeContent, which will dispose it when disposed. | |
| return mimePart; | |
| } | |
| private static MimeEntity CreateLinkedResource(LinkedResource resource) | |
| { | |
| var contentType = resource.ContentType.MediaType; | |
| var mediaType = contentType.Split('/')[0] ?? "application"; | |
| var mediaSubType = contentType.Split('/')[1] ?? "octet-stream"; | |
| MemoryStream memoryStream = CopyToMemoryStream(resource.ContentStream); | |
| var mimePart = new MimePart(mediaType, mediaSubType) | |
| { | |
| Content = new MimeContent(memoryStream), | |
| ContentDisposition = new ContentDisposition(ContentDisposition.Inline), | |
| ContentTransferEncoding = ContentEncoding.Base64, | |
| ContentId = string.IsNullOrWhiteSpace(resource.ContentId) ? MimeUtils.GenerateMessageId() : resource.ContentId, | |
| FileName = resource.ContentId | |
| }; | |
| // The created MemoryStream is now owned by MimeContent, which will dispose it when disposed. | |
| return mimePart; | |
| } |
🤖 Prompt for AI Agents
In shesha-core/src/Shesha.Application/Email/MailKitEmailHelper.cs around lines
166 to 183, the MemoryStream is wrapped in a using which disposes it before
MimeContent can use it; remove the using so the MemoryStream remains alive and
is owned/disposed by MimeContent (i.e., create the MemoryStream, pass it into
new MimeContent without disposing here), and ensure any callers rely on
MimeContent to dispose the stream.
| private static SecureSocketOptions GetSecureSocketOption(SmtpSettings settings) | ||
| { | ||
| if (!settings.EnableSsl) | ||
| return SecureSocketOptions.None; | ||
|
|
||
| return settings.Port == 465 | ||
| ? SecureSocketOptions.SslOnConnect | ||
| : SecureSocketOptions.StartTls; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding explicit SSL mode configuration for edge cases.
The port-based heuristic (465 = implicit SSL, others = StartTls) is reasonable for common configurations, but some SMTP servers may have non-standard setups. Consider adding an explicit SslMode option in SmtpSettings for full configurability.
🤖 Prompt for AI Agents
In shesha-core/src/Shesha.Application/Email/MailKitEmailHelper.cs around lines
230 to 238, the method currently infers SSL behavior solely from EnableSsl and
port which fails for non‑standard servers; add an explicit SslMode property
(enum: None, SslOnConnect, StartTls, Auto/Default) to SmtpSettings and update
GetSecureSocketOption to prefer SmtpSettings.SslMode when set, mapping the enum
to the corresponding SecureSocketOptions values and only falling back to the
existing port-based heuristic when SslMode is unspecified or set to
Auto/Default.
| using var mimeMessage = MailKitEmailHelper.ConvertToMimeMessage(mail); | ||
| var smtpSettings = await _emailSettings.SmtpSettings.GetValueAsync(); | ||
| await MailKitEmailHelper.SendAsync(mimeMessage, smtpSettings); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Reorder operations to fail fast before expensive conversion.
The MimeMessage conversion (line 89) happens before fetching SmtpSettings (line 90). If settings retrieval fails, the conversion work is wasted. Consider fetching settings first.
private async Task SendEmailAsync(MailMessage mail)
{
try
{
- using var mimeMessage = MailKitEmailHelper.ConvertToMimeMessage(mail);
var smtpSettings = await _emailSettings.SmtpSettings.GetValueAsync();
+ using var mimeMessage = MailKitEmailHelper.ConvertToMimeMessage(mail);
await MailKitEmailHelper.SendAsync(mimeMessage, smtpSettings);
}🤖 Prompt for AI Agents
In shesha-core/src/Shesha.Application/Notifications/Emails/EmailChannelSender.cs
around lines 89-91, the code converts the Mail object to a MimeMessage before
retrieving SMTP settings, causing wasted work if settings retrieval fails;
reorder the operations to first await
_emailSettings.SmtpSettings.GetValueAsync(), validate or fail early on the
settings, then call MailKitEmailHelper.ConvertToMimeMessage(mail) and finally
await MailKitEmailHelper.SendAsync(mimeMessage, smtpSettings).
| <PrivateAssets>all</PrivateAssets> | ||
| <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets> | ||
| </PackageReference> | ||
| <PackageReference Include="MailKit" Version="4.7.0" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
MailKit 4.7.0 latest version NuGet
💡 Result:
No — MailKit 4.7.0 is not the latest. The current NuGet latest version is 4.14.1 (install with: dotnet add package MailKit --version 4.14.1). [1]
Sources:
[1] NuGet: MailKit package (4.14.1).
🌐 Web query:
MailKit security vulnerabilities CVE 2024 2025
💡 Result:
Summary — notable 2024–2025 issues affecting MailKit (or its dependencies)
-
Denial‑of‑Service via vulnerable System.Formats.Asn1 (affects MimeKit used by MailKit). Patched in MimeKit 4.7.1 (upgrade MailKit/MimeKit). Sources: GitHub advisory / OSS databases. [1][2]
-
CVE‑2024‑38095 — .NET System.Formats.Asn1 / ASN.1 parsing DoS (excessive CPU). This is a .NET runtime/component vulnerability that can impact libraries (including MimeKit/MailKit S/MIME operations) until patched in .NET; apply Microsoft/.NET updates. [3]
-
MailKit repository security page: no MailKit‑specific published security advisories (report via private advisory); keep MailKit/MimeKit up to latest. [4]
If you want, I can:
- list exact vulnerable version ranges and recommended fixed versions, or
- produce upgrade/mitigation steps for your project (NuGet package commands and minimal code/workarounds).
Sources:
[1] GitLab/GHSA advisory for MimeKit (GHSA‑gmc6‑fwg3‑75m5) — fixed in MimeKit 4.7.1. (advisories.gitlab.com)
[2] OSV / GitHub advisory summary (GHSA‑gmc6‑fwg3‑75m5). (osv.dev)
[3] NVD entry for CVE‑2024‑38095 (.NET ASN.1 DoS). (nvd.nist.gov)
[4] MailKit GitHub security policy / page (no published advisories). (github.com)
Citations:
- 1: https://advisories.gitlab.com/pkg/nuget/mimekit/GHSA-gmc6-fwg3-75m5/?utm_source=openai
- 2: https://osv.dev/vulnerability/GHSA-gmc6-fwg3-75m5?utm_source=openai
- 3: https://nvd.nist.gov/vuln/detail/CVE-2024-38095?utm_source=openai
- 4: https://github.com/jstedfast/MailKit/security?utm_source=openai
Upgrade MailKit from 4.7.0 to 4.14.1 to address security vulnerabilities.
MailKit 4.7.0 is outdated; the current NuGet latest is 4.14.1. More critically, MailKit 4.7.0 depends on a vulnerable version of MimeKit that is affected by a Denial-of-Service vulnerability (GHSA-gmc6-fwg3-75m5) caused by System.Formats.Asn1 ASN.1 parsing, which was patched in MimeKit 4.7.1. Additionally, CVE-2024-38095 impacts ASN.1 operations used in S/MIME processing. Upgrade to MailKit 4.14.1 immediately.
🤖 Prompt for AI Agents
In shesha-core/src/Shesha.Application/Shesha.Application.csproj around line 169,
the project references MailKit 4.7.0 which depends on a vulnerable MimeKit;
update the PackageReference Version attribute to 4.14.1 to pull in the patched
MimeKit and mitigate ASN.1/S-MIME DoS vulnerabilities; after updating, restore
NuGet packages and run the test suite/CI to ensure no breaking changes.
| var isBodyHtml = false; | ||
| var attachments = new List<EmailAttachment>(); | ||
|
|
||
| foreach (var attachment in upload.Attachments) | ||
| { | ||
| if (attachment.Length > 0) | ||
| { | ||
| using var sourceStream = attachment.OpenReadStream(); | ||
| var memoryStream = new MemoryStream(); | ||
| sourceStream.CopyTo(memoryStream); | ||
| memoryStream.Position = 0; // Reset position for reading | ||
|
|
||
| attachments.Add(new EmailAttachment(attachment.FileName, memoryStream)); | ||
| } | ||
| } | ||
|
|
||
| _emailSender.SendMail(upload.From, upload.To, upload.Subject, upload.Body, isBodyHtml, attachments); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider async streaming, attachment size limits, and HTML flag flexibility
A few non‑blocking but important points around robustness and behavior:
- You synchronously call
sourceStream.CopyTo(memoryStream)and_emailSender.SendMail(...)on the request thread. For large attachments or slow SMTP, this can block the thread pool. If the email infrastructure exposes async APIs, preferCopyToAsyncand an asyncSendMailAsync. - Each attachment is fully buffered into memory via
MemoryStream. For large or many files this can be a memory/DoS risk. Depending on howEmailAttachmentis implemented, consider:- Imposing max per‑file and total size limits (either via ASP.NET Core
[RequestSizeLimit]/ global config or explicit checks onattachment.Length), and/or - Using streaming or temporary files instead of unbounded in‑memory buffers if large attachments are a real use case.
- Imposing max per‑file and total size limits (either via ASP.NET Core
isBodyHtmlis hardcoded tofalse. If callers sometimes send HTML bodies, exposing abool IsBodyHtmlonEmailWithAttachmentsDto(or deriving it from content) would make this endpoint more generally useful.
None of these are correctness bugs today, but addressing them would make the endpoint safer and more future‑proof.
🤖 Prompt for AI Agents
In shesha-core/src/Shesha.Web.Host/Controllers/EmailController.cs around lines
28 to 45, the controller synchronously copies uploaded attachments into
MemoryStreams and calls SendMail synchronously, uses unbounded in-memory
buffering for attachments, and hardcodes isBodyHtml=false; update to use
asynchronous IO and email sending (use CopyToAsync and SendMailAsync if
available), enforce per-file and total size limits before buffering (e.g., check
attachment.Length against configured maxs and return 400 if exceeded), avoid
unbounded memory by streaming to temporary files when attachment sizes exceed a
threshold, and add an IsBodyHtml flag to the DTO (or derive from content-type)
and pass it to the email sender so HTML bodies are handled correctly.
2 participants
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.