Fix email template loading and conditional TLS configuration #3066
Conversation
# Conflicts: # server/src/service/v1/infrastructure/emailService.js
sameh0
requested review from
Br0wnHammer,
Owaiseimdad,
ajhollid and
karenvicent
as code owners
|
Warning Rate limit exceeded@sameh0 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 37 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note
|
| Cohort / File(s) | Summary |
|---|---|
Email service core server/src/service/v1/infrastructure/emailService.js |
Multi-path template discovery (dev → prod) with compile-first-found and no-op fallback; enhanced error logging and template validation; validate MJML → HTML non-empty; sendEmail input validation (to, subject), compute/validate fromAddress, fallback HTML; transporter.verify() before send; TLS options moved to top-level (ignoreTLS, requireTLS) and applied only when systemEmailSecure true; preloads templates into templateLookup. |
Client settings UI client/src/Pages/v1/Settings/SettingsEmail.jsx |
JSON display now conditionally includes TLS-related fields: only emits tls and its rejectUnauthorized/servername when systemEmailSecure is true and values exist; ignoreTLS/requireTLS included only when defined in secure mode. |
Sequence Diagram(s)
sequenceDiagram
participant Caller
participant EmailSvc as emailService
participant Loader as TemplateLoader
participant Builder as EmailBuilder
participant MJML as MJMLCompiler
participant SMTP as SMTPTransporter
Caller->>EmailSvc: sendEmail(to, subject, templateName, data)
rect rgb(240,248,255)
Note over EmailSvc: Input validation & fromAddress
EmailSvc->>EmailSvc: validate to/subject, compute fromAddress
alt invalid input
EmailSvc-->>Caller: return false
end
end
rect rgb(255,250,240)
Note over Loader: Template discovery (dev → prod)
EmailSvc->>Loader: loadTemplate(templateName)
Loader->>Loader: probe dev paths → probe prod/dist paths
alt found
Loader-->>EmailSvc: compiled template
else not found
Loader-->>EmailSvc: no-op template
end
end
rect rgb(240,255,240)
Note over Builder: Build & validate content
EmailSvc->>Builder: buildEmail(template, data)
Builder->>MJML: compile MJML -> HTML
alt empty/invalid
Builder-->>EmailSvc: throw / return error
else valid HTML
Builder-->>EmailSvc: HTML ready
end
end
rect rgb(255,245,240)
Note over SMTP: Verify & send
EmailSvc->>SMTP: transporter.verify()
alt verify fails
SMTP-->>EmailSvc: error
EmailSvc-->>Caller: return false
else verify OK
EmailSvc->>SMTP: sendMail({from: fromAddress, ...})
alt send succeeds
SMTP-->>EmailSvc: success
EmailSvc-->>Caller: return true
else send fails
SMTP-->>EmailSvc: error
EmailSvc-->>Caller: return false
end
end
end
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
- Focus areas:
- multi-path template discovery correctness and no-op fallback
- MJML -> HTML validation and error handling
- conditional TLS application when
systemEmailSecureis true - transporter.verify() handling and consistent false returns
- client UI conditional inclusion of TLS fields
Poem
🐇
I hopped through folders, found each file,
Compiled the mail with careful smile.
Verified the post, checked every TLS seam,
Now alerts arrive and emails gleam. ✉️🎉
Pre-merge checks and finishing touches
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title accurately captures the main changes: email template loading fixes and conditional TLS configuration, matching the primary objectives. |
| Description check | ✅ Passed | Description comprehensively addresses all major changes with clear problem statements, solutions, and fixes; all required checklist items are marked complete. |
| Linked Issues check | ✅ Passed | Code changes directly address issue #3054: template loading across multiple paths fixes ENOENT errors, validation prevents empty emails, and conditional TLS enables services like Resend. |
| Out of Scope Changes check | ✅ Passed | All changes focus on email service improvements and UI display of email configuration; no unrelated modifications detected across the two modified files. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
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 help to get the list of available commands and usage tips.
|
Image exist for testing at ghcr.io/sameh0/checkmate-backend:latest |
![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 (1)
server/src/service/v1/infrastructure/emailService.js (1)
49-62: Template multi-path loading and error handling look solid, minor logging nit onlyThe multi-path search with early exit and structured debug/error logging is a clear improvement and should resolve the Docker vs. dev template path issues as intended. The no-op fallback function combined with the later empty-MJML guard in
buildEmailis a reasonable way to avoid hard startup failures while still surfacing problems when a template is actually used.One small optional improvement: the ENOENT error message currently logs the full list of absolute
possiblePaths(includingprocess.cwd()-based ones), which may expose internal filesystem layout. You could consider logging them as paths relative toprocess.cwd()or__dirnameinstead, using something likethis.path.relative(process.cwd(), p)rather than.replace(__dirname, ".").Also applies to: 64-66, 68-82, 84-99, 101-105, 108-109
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/service/v1/infrastructure/emailService.js(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/service/v1/infrastructure/emailService.js (1)
server/src/index.js (2)
__dirname(19-19)SERVICE_NAME(13-13)
🔇 Additional comments (2)
server/src/service/v1/infrastructure/emailService.js (2)
137-145: Stronger template and MJML/HTML validation aligns with the PR goalsThe additional checks in
buildEmail(template existence, function type, non-empty MJML, and non-empty HTML) plus the structured error logging and rethrow are all good improvements. They should prevent silent empty emails and make template/config issues much easier to track down, while keeping behavior explicit (exceptions instead of returning bad content).No issues spotted with this block.
Also applies to: 146-154, 158-167, 171-182, 187-191, 194-195
310-311: Using validatedfromAddressand returningfalseon send errors looks goodSwitching the
fromfield to use the validatedfromAddressand normalizing send failures to returnfalsegives a clearer and more predictable contract to callers. This is consistent with the earlier input validation and should make downstream handling simpler.No issues here.
Also applies to: 322-323
sameh0
changed the title
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR improves email validation and template loading but introduces breaking changes in TLS configuration and from address validation, along with silent template failures that could lead to empty emails.
📄 Documentation Diagram
This diagram documents the refactored email sending workflow with improved validation and conditional TLS.
sequenceDiagram
participant U as User
participant ES as EmailService
participant SS as SMTP Server
U->>ES: sendEmail(to, subject, html)
ES->>ES: Validate parameters
note over ES: PR #35;3066 added validation<br/>for empty content and email format
ES->>ES: Build email config
note over ES: Conditional TLS settings<br/>only if systemEmailSecure true
ES->>SS: Send email with config
SS-->>ES: Response
🌟 Strengths
- Enhanced template loading with multiple fallback paths for better reliability in different environments.
- Improved error handling and logging throughout the email service.
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P1 | server/.../emailService.js | Architecture | Breaking change in TLS config could cause connection failures. | settingsController.js |
| P1 | server/.../emailService.js | Bug | From address validation may incorrectly fail valid SMTP setups. | settingsController.js |
| P1 | server/.../emailService.js | Bug | Silent template failures lead to empty emails without clear errors. | notificationService.js, settingsController.js |
| P2 | server/.../emailService.js | Bug | Empty HTML validation breaks notification flows with degraded functionality. | |
| P2 | server/.../emailService.js | Maintainability | Complex template path resolution increases code maintenance burden. | |
| P2 | server/.../emailService.js | Maintainability | Template validation exposes implementation details in logs. |
🔍 Notable Themes
- Configuration Breaking Changes: TLS and from address validations may disrupt existing SMTP setups without clear migration paths.
- Error Handling Consistency: Mixed approaches to failures—some propagated, some silenced—could confuse debugging efforts.
📈 Risk Diagram
This diagram illustrates the risks in TLS configuration changes and validation logic.
sequenceDiagram
participant SC as Settings Controller
participant ES as EmailService
participant NS as Notification Service
SC->>ES: Provide email config<br/>(includes TLS settings)
note over SC,ES: R1(P1): TLS settings ignored<br/>if systemEmailSecure false
ES->>ES: Validate from address
note over ES: R2(P1): Incorrect validation<br/>may fail valid setups
ES->>ES: Load template
note over ES: R3(P1): Silent failure<br/>returns empty function
ES->>NS: Send email (may fail or send empty)
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
| if (systemEmailRequireTLS !== undefined) { | ||
| tlsSettings.requireTLS = systemEmailRequireTLS; | ||
| } | ||
| if (systemEmailTLSServername !== undefined && systemEmailTLSServername !== null && systemEmailTLSServername !== '') { |
There was a problem hiding this comment.
P1 | Confidence: High
This change conditionally applies TLS settings only when systemEmailSecure is enabled. However, the related context shows that settingsController.js passes all TLS-related settings (including systemEmailRejectUnauthorized, systemEmailIgnoreTLS, etc.) regardless of the secure flag. This creates a potential breaking change where users with systemEmailSecure: false but explicit TLS settings will lose their TLS configuration, potentially causing connection failures or security issues with SMTP servers that require specific TLS behavior even in non-secure mode.
Code Suggestion:
// Apply TLS settings if any TLS-related config is present
const tlsSettings = {};
if (systemEmailRejectUnauthorized !== undefined) {
tlsSettings.rejectUnauthorized = systemEmailRejectUnauthorized;
}
if (systemEmailIgnoreTLS !== undefined) {
tlsSettings.ignoreTLS = systemEmailIgnoreTLS;
}
if (systemEmailRequireTLS !== undefined) {
tlsSettings.requireTLS = systemEmailRequireTLS;
}
if (systemEmailTLSServername !== undefined && systemEmailTLSServername !== null && systemEmailTLSServername !== '') {
tlsSettings.servername = systemEmailTLSServername;
}
if (Object.keys(tlsSettings).length > 0) {
emailConfig.tls = tlsSettings;
}Evidence: path:server/src/controllers/v1/settingsController.js
| // Validate from address | ||
| const fromAddress = systemEmailAddress || systemEmailUser; | ||
| if (!fromAddress || !fromAddress.includes('@')) { | ||
| this.logger.error({ | ||
| message: `Invalid from email address: ${fromAddress}`, | ||
| service: SERVICE_NAME, | ||
| method: "sendEmail", | ||
| }); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
P1 | Confidence: High
The new validation rejects emails if the from address doesn't contain '@'. However, the related context shows that settingsController.js passes systemEmailUser separately from systemEmailAddress. Some SMTP configurations may use systemEmailUser as a username (not necessarily an email) while systemEmailAddress is the actual from address. This validation could incorrectly fail valid configurations where systemEmailUser is a non-email username and systemEmailAddress is properly set.
Code Suggestion:
const fromAddress = systemEmailAddress;
if (!fromAddress || !fromAddress.includes('@')) {
this.logger.error({
message: `Invalid from email address: ${fromAddress}`,
service: SERVICE_NAME,
method: "sendEmail",
});
return false;
}Evidence: path:server/src/controllers/v1/settingsController.js
| // Return a no-op function that returns empty string to prevent runtime errors | ||
| return () => ""; |
There was a problem hiding this comment.
P1 | Confidence: High
When template loading fails, the code returns a function that produces empty content. This creates a silent failure mode where emails are sent with empty content instead of proper error handling. The related context shows that both notificationService.js and settingsController.js rely on sendEmail returning a messageId for success, but they won't receive clear errors about template failures, making debugging difficult.
Code Suggestion:
// Re-throw the error to let callers handle template failures properly
throw new Error(`Failed to load template '${templateName}': ${error.message}`);Evidence: path:server/src/service/v1/infrastructure/notificationService.js, path:server/src/controllers/v1/settingsController.js
| return this.compile(templateContent); | ||
| // Try multiple possible paths for template files | ||
| // to support both development and production environments | ||
| const possiblePaths = [ |
There was a problem hiding this comment.
P2 | Confidence: High
The template path resolution now tries 6 different locations, which increases complexity and makes the code harder to maintain. The paths mix development, production, and build artifacts locations, creating potential confusion about which path should be used in different environments. This approach may mask configuration issues rather than solving them properly.
Code Suggestion:
const templateBase = process.env.EMAIL_TEMPLATE_PATH || this.path.join(process.cwd(), 'templates');
const templatePath = this.path.join(templateBase, `${templateName}.mjml`);| return false; | ||
| } | ||
|
|
||
| // Validate HTML content |
There was a problem hiding this comment.
P2 | Confidence: Medium
The new validation rejects empty HTML content, but the related context shows notificationService.js calls sendEmail without checking the HTML content first. This could break notification flows where template rendering fails silently. While preventing empty emails is good, the change introduces a hard failure where previously notifications might have continued with degraded functionality.
Code Suggestion:
if (!html || html.trim() === "") {
this.logger.warn({
message: "Email HTML content is empty, using fallback text",
service: SERVICE_NAME,
method: "sendEmail",
});
html = "<p>Email content unavailable</p>";
}| if (typeof this.templateLookup[template] !== 'function') { | ||
| this.logger.error({ | ||
| message: `Template '${template}' is not a function. Type: ${typeof this.templateLookup[template]}`, | ||
| service: SERVICE_NAME, | ||
| method: "buildEmail", | ||
| templateValue: this.templateLookup[template], | ||
| }); | ||
| throw new Error(`Template '${template}' is not a function`); | ||
| } |
There was a problem hiding this comment.
P2 | Confidence: Medium
This validation exposes internal implementation details (template functions) in error messages. The templateValue logging could leak sensitive template content or create overly verbose logs. The check assumes all templates must be functions, which couples the validation to the current implementation rather than the interface contract.
Code Suggestion:
if (typeof this.templateLookup[template] !== 'function') {
this.logger.error({
message: `Template '${template}' is not properly initialized`,
service: SERVICE_NAME,
method: "buildEmail",
});
throw new Error(`Template '${template}' is not available`);
}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
server/src/service/v1/infrastructure/emailService.js (4)
146-154: Remove or redacttemplateValuefrom error logs.Logging
templateValue(line 151) could leak sensitive template content or create overly verbose logs. The type check and available templates list provide sufficient debugging context.if (typeof this.templateLookup[template] !== 'function') { this.logger.error({ message: `Template '${template}' is not a function. Type: ${typeof this.templateLookup[template]}`, service: SERVICE_NAME, method: "buildEmail", - templateValue: this.templateLookup[template], }); throw new Error(`Template '${template}' is not a function`); }
51-62: Simplify template path resolution with environment variable.Trying 6 hardcoded paths adds complexity and makes the code harder to maintain. The paths mix development, production, and build artifact locations, which can mask configuration issues rather than solve them properly.
Consider using a single environment variable to specify the template directory, with a sensible default:
- const possiblePaths = [ - // Production/Docker path - templates in dist/templates (from dist/src/service/v1/infrastructure) - this.path.join(__dirname, `../../../templates/${templateName}.mjml`), - // Alternative production path - templates in dist/templates (from dist/service/v1/infrastructure) - this.path.join(__dirname, `../../templates/${templateName}.mjml`), - // If running from dist, templates might be in parent src directory - this.path.join(__dirname, `../../../../src/templates/${templateName}.mjml`), - // Development path - from the project root - this.path.join(process.cwd(), `templates/${templateName}.mjml`), - this.path.join(process.cwd(), `src/templates/${templateName}.mjml`), - this.path.join(process.cwd(), `dist/templates/${templateName}.mjml`), - ]; - - let templatePath; - let templateContent; - - // Try each path until we find one that works - for (const tryPath of possiblePaths) { - try { - if (this.fs.existsSync(tryPath)) { - templatePath = tryPath; - templateContent = this.fs.readFileSync(templatePath, "utf8"); - break; - } - } catch (e) { - // Continue to next path - } - } - - if (!templateContent) { - throw new Error(`Template file not found in any of: ${possiblePaths.map(p => p.replace(__dirname, '.')).join(', ')}`); - } + const templateBase = process.env.EMAIL_TEMPLATE_PATH || this.path.join(process.cwd(), 'templates'); + const templatePath = this.path.join(templateBase, `${templateName}.mjml`); + + if (!this.fs.existsSync(templatePath)) { + throw new Error(`Template file not found at: ${templatePath}`); + } + + const templateContent = this.fs.readFileSync(templatePath, "utf8");Then configure
EMAIL_TEMPLATE_PATHin your Docker environment to point to the correct location.
108-109: Throw error on template load failure instead of returning empty function.Returning a no-op function that produces empty content creates a silent failure mode. Emails will be sent with empty content instead of proper error handling, making debugging difficult.
The validation added in
buildEmail(lines 158-167) will catch this and throw, but it's better to fail fast at template loading time:- // Return a no-op function that returns empty string to prevent runtime errors - return () => ""; + // Re-throw the error to let callers handle template failures properly + throw error;This ensures template loading issues are detected during initialization rather than at email-send time.
199-219: Remove PII from error logs and document HTML-only requirement.Two concerns with the validation logic:
- PII exposure (lines 215-216): Logging
toandsubjectdirectly exposes email addresses in logs, which may violate compliance/privacy requirements. Remove these fields or redact them:if (!html || html.trim() === "") { this.logger.error({ message: "Cannot send email: HTML content is empty", service: SERVICE_NAME, method: "sendEmail", - to: to, - subject: subject, }); return false; }
- HTML-only enforcement (line 210): The validation blocks any text-only emails. If this is intentional, document it; if text-only support should remain, accept either
htmlortextparameters. Currently no callers use text-only, but this silently prevents future usage.
🧹 Nitpick comments (1)
server/src/service/v1/infrastructure/emailService.js (1)
241-250: From address validation looks correct, but consider clearer error messaging.The logic correctly prioritizes
systemEmailAddressoversystemEmailUser, so the concern from past reviews about username vs email is addressed. The validation will only fail if neither value contains an '@' symbol.However, the error message could be more helpful by indicating which configuration field needs to be fixed:
const fromAddress = systemEmailAddress || systemEmailUser; if (!fromAddress || !fromAddress.includes('@')) { this.logger.error({ - message: "Invalid from email address", + message: "Invalid from email address: systemEmailAddress or systemEmailUser must be a valid email", service: SERVICE_NAME, method: "sendEmail", }); return false; }Good practice: The error log doesn't expose the actual email address, avoiding PII leakage.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/service/v1/infrastructure/emailService.js(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/service/v1/infrastructure/emailService.js (1)
server/src/index.js (1)
__dirname(19-19)
🔇 Additional comments (1)
server/src/service/v1/infrastructure/emailService.js (1)
296-326: Email sending logic is well-structured.The transporter verification and email sending flow is correctly implemented:
- Transporter is verified before use (lines 298-307)
- Uses the validated
fromAddress(line 312)- Consistent error handling that returns
falseon failure (lines 306, 324)- Returns
messageIdon success (line 316)This provides clear success/failure signaling to callers.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
server/src/service/v1/infrastructure/emailService.js (3)
265-295: TLS configuration inconsistency:ignoreTLSandrequireTLSshould also be conditional.Lines 266-271 unconditionally apply
ignoreTLSandrequireTLS, while lines 274-295 only applyrejectUnauthorizedandservernamewhensystemEmailSecureis true. This is inconsistent with the PR objective: "only add TLS settings when systemEmailSecure is enabled; previously TLS flags were always passed, breaking services (e.g., Resend)."A previous review comment on these exact lines provided the fix. All TLS-related options should only be added when
systemEmailSecureis true.Apply this diff to make all TLS settings conditional:
// Build base email config const emailConfig = { host: systemEmailHost, port: Number(systemEmailPort), secure: systemEmailSecure, auth: { user: systemEmailUser || systemEmailAddress, pass: systemEmailPassword, }, name: systemEmailConnectionHost || "localhost", connectionTimeout: 5000, }; - // Add top-level TLS options (must be outside tls object) - if (systemEmailIgnoreTLS !== undefined) { - emailConfig.ignoreTLS = systemEmailIgnoreTLS; - } - if (systemEmailRequireTLS !== undefined) { - emailConfig.requireTLS = systemEmailRequireTLS; - } - // Conditionally add TLS settings only if secure is enabled if (systemEmailSecure) { const tlsSettings = {}; + // Add top-level TLS options (must be outside tls object) + if (systemEmailIgnoreTLS !== undefined) { + emailConfig.ignoreTLS = systemEmailIgnoreTLS; + } + if (systemEmailRequireTLS !== undefined) { + emailConfig.requireTLS = systemEmailRequireTLS; + } + // Only add TLS settings that are explicitly configured if (systemEmailRejectUnauthorized !== undefined) { tlsSettings.rejectUnauthorized = systemEmailRejectUnauthorized; } if (systemEmailTLSServername !== undefined && systemEmailTLSServername !== null && systemEmailTLSServername !== '') { tlsSettings.servername = systemEmailTLSServername; } // Only add tls property if we have TLS settings if (Object.keys(tlsSettings).length > 0) { emailConfig.tls = tlsSettings; this.logger.debug({ message: `TLS settings applied to email config`, service: SERVICE_NAME, method: "sendEmail", tlsSettings: Object.keys(tlsSettings), }); } }
209-219: PII exposure: email addresses and subjects still logged directly.Despite a previous review comment (Major, marked as "Addressed in commit 0b3e023") flagging PII exposure in error logs, lines 215-216 still log the recipient email address (
to) andsubjectdirectly. Email addresses are PII and subjects may contain sensitive information.Apply this diff to remove PII from logs:
// Validate HTML content if (!html || html.trim() === "") { this.logger.warn({ message: "Email HTML content is empty, using fallback text", service: SERVICE_NAME, method: "sendEmail", - to: to, - subject: subject, }); html = "<p>Email content unavailable</p>"; }
108-109: Silent failure still masks template loading errors.Returning an empty-string function when template loading fails creates a silent failure mode where emails are sent with empty content instead of surfacing the error to callers. This contradicts the PR's goal of improved error handling and makes debugging difficult.
A previous review comment (P1, High confidence) on these lines recommended re-throwing the error to let callers handle template failures properly. The current implementation still suppresses the error.
Apply this diff to surface template loading failures:
} catch (error) { this.logger.error({ message: `Failed to load template '${templateName}': ${error.message}`, service: SERVICE_NAME, method: "loadTemplate", templateName: templateName, error: error.message, stack: error.stack, }); - // Return a no-op function that returns empty string to prevent runtime errors - return () => ""; + // Re-throw to let callers handle the failure + throw new Error(`Failed to load template '${templateName}': ${error.message}`); }
🧹 Nitpick comments (1)
server/src/service/v1/infrastructure/emailService.js (1)
146-154: RemovetemplateValuefrom error logs to avoid exposing template internals.A previous review comment (P2, Medium confidence) flagged that logging
templateValuecould leak sensitive template content or create overly verbose logs. The type and template name provide sufficient debugging context.Apply this diff:
if (typeof this.templateLookup[template] !== 'function') { this.logger.error({ message: `Template '${template}' is not a function. Type: ${typeof this.templateLookup[template]}`, service: SERVICE_NAME, method: "buildEmail", - templateValue: this.templateLookup[template], }); throw new Error(`Template '${template}' is not a function`); }
|
@sameh0 your PR has a minor CI/CD check issue: |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/service/v1/infrastructure/emailService.js (1)
198-250: UndefinedredactEmail/redactSubjectwill throw on empty-HTML pathsIn
sendEmail, the warning branch for empty HTML (Lines 210–218) callsthis.redactEmail(to)andthis.redactSubject(subject), but neither helper is defined onEmailService. This will cause aTypeError: this.redactEmail is not a functionthe first time an email is built with empty HTML, preventing the fallback HTML from being used and short‑circuiting the send.Given the privacy concerns around logging PII, the simplest safe fix is to drop these fields from the log for now:
if (!html || html.trim() === "") { this.logger.warn({ message: "Email HTML content is empty, using fallback text", service: SERVICE_NAME, method: "sendEmail", - to: this.redactEmail(to), - subject: this.redactSubject(subject), }); html = "<p>Email content unavailable</p>"; }If you do want redacted
to/subjectin logs, please add concrete helper implementations on the class (or inject a masking utility) and keep them thoroughly unit‑tested.
🧹 Nitpick comments (3)
server/src/service/v1/infrastructure/emailService.js (3)
47-110: Template multi-path loading is robust; consider surfacing non‑ENOENT fs errorsThe multi-path resolution and logging of all attempted locations should help a lot with dev vs Docker discrepancies, and the final error message is clear. However, the inner
try { ... } catch (e) { /* ignore */ }aroundexistsSync/readFileSyncmeans real I/O problems (e.g., permission errors, partial reads) get silently swallowed and later misreported as “file not found”.A small refactor would be to only suppress
ENOENTand let other errors bubble to the outercatch, for example:- for (const tryPath of possiblePaths) { - try { - if (this.fs.existsSync(tryPath)) { - templatePath = tryPath; - templateContent = this.fs.readFileSync(templatePath, "utf8"); - break; - } - } catch (e) { - // Continue to next path - } - } + for (const tryPath of possiblePaths) { + try { + if (this.fs.existsSync(tryPath)) { + templatePath = tryPath; + templateContent = this.fs.readFileSync(templatePath, "utf8"); + break; + } + } catch (e) { + // Only ignore "file not found" errors; surface everything else + if (!e || e.code !== "ENOENT") { + throw e; + } + } + }This preserves the fallback behavior while keeping error diagnostics accurate when something is genuinely wrong with the file system.
252-296: TLS option gating may renderignoreTLS/requireTLSineffective—confirm this matches intentThe TLS block currently applies all TLS-related options only when
systemEmailSecureis true:if (systemEmailSecure) { if (systemEmailIgnoreTLS !== undefined) { emailConfig.ignoreTLS = systemEmailIgnoreTLS; } if (systemEmailRequireTLS !== undefined) { emailConfig.requireTLS = systemEmailRequireTLS; } // ... }Per nodemailer’s docs,
ignoreTLSandrequireTLSspecifically control STARTTLS behavior and only have effect whensecureisfalse(STARTTLS mode). With the current wiring:
- When
systemEmailSecureisfalse(typical for port 587 STARTTLS), these flags are never applied, so admin-configuredignoreTLS/requireTLSvalues become no-ops.- When
systemEmailSecureistrue(implicit TLS), the flags are set but effectively irrelevant.If the product intentionally wants to ignore
ignoreTLS/requireTLSwheneversecureis false (to protect providers like Resend), consider documenting that in code comments and/or UI so operators don’t expect these flags to do anything in that configuration.If you do want these options to behave as in nodemailer’s documentation while still protecting non‑TLS providers, you may want to adjust the conditions—for example, only applying them when admins explicitly enable them and accepting that this opt‑in might break misconfigured Resend setups, or splitting “TLS enabled” from “advanced TLS tuning” into separate config flags.
299-325: Consider logging error details on transporter verification failuresThe
verifystep is a good addition, but the catch block only logs a generic"Email transporter verification failed"message and dropserror.message/ connection details. That can make SMTP misconfigurations (wrong host, port, TLS mismatch) hard to troubleshoot from logs alone.A small, non‑breaking improvement:
try { await this.transporter.verify(); } catch (error) { this.logger.warn({ - message: "Email transporter verification failed", + message: `Email transporter verification failed: ${error.message}`, service: SERVICE_NAME, method: "verifyTransporter", + host: emailConfig.host, + port: emailConfig.port, + secure: emailConfig.secure, + stack: error.stack, }); return false; }This keeps verification as a soft failure (
falsereturn) but makes diagnosing configuration issues much easier.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/service/v1/infrastructure/emailService.js(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/service/v1/infrastructure/emailService.js (1)
server/src/index.js (1)
__dirname(19-19)
🔇 Additional comments (1)
server/src/service/v1/infrastructure/emailService.js (1)
135-195: Stronger template / MJML validation looks goodThe additional guards in
buildEmail(missing template entry, non-function, empty MJML, and missing HTML frommjml2html) plus rethrowing after logging eliminate the previous “silent empty email” failure mode and propagate clear errors to callers. This aligns well with the goal of avoiding empty infrastructure alert emails.No issues from my side here.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/src/Pages/v1/Settings/SettingsEmail.jsx (1)
294-307: Inner TLS field conditions are also affected by default values.While the outer condition correctly checks if
systemEmailSecureis truthy, the inner conditions at lines 299-301 also check!== undefined, which will always be true forsystemEmailRejectUnauthorized(defaults totrueat line 45).This means when
systemEmailSecureistrue, thetlsobject will always includerejectUnauthorized, even if you intended to make it conditional.Consider the same fix approaches as the previous comment:
...(systemEmailSecure && - (systemEmailRejectUnauthorized !== undefined || - (systemEmailTLSServername && - systemEmailTLSServername !== "")) && { + (systemEmailTLSServername && systemEmailTLSServername !== "") && { tls: { - ...(systemEmailRejectUnauthorized !== undefined && { - rejectUnauthorized: systemEmailRejectUnauthorized, - }), + rejectUnauthorized: systemEmailRejectUnauthorized, ...(systemEmailTLSServername && systemEmailTLSServername !== "" && { servername: systemEmailTLSServername, }), }, }),Or if you want both fields to be conditional, remove the default for
systemEmailRejectUnauthorizedin the destructuring (line 45) and keep the!== undefinedchecks.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
server/src/service/v1/infrastructure/emailService.js (3)
137-195: Stronger build-time validation is good; templateValue logging may be overly verboseThe additional checks for missing/non-function templates, empty MJML, and failed MJML→HTML conversion are solid and make failures much clearer. When the template isn’t a function, logging
templateValueitself could dump large or noisy data; logging just its type or a summarized descriptor (e.g., constructor name) would keep logs leaner without losing signal.
235-250: From-address validation assumes an email-style value; ensure config matches thatDeriving
fromAddressfromsystemEmailAddress || systemEmailUserand requiring an@is reasonable if you always expect a real email in at least one of those fields. Please double-check that all supported SMTP configurations in your deployments setsystemEmailAddress(or otherwise use an email-form username); otherwise, this guard will cause valid non-email usernames to be rejected.Also applies to: 313-313
199-219: Empty-HTML fallback helps, but warning log still exposes PIIThe guard for missing
to/subjectand the HTML fallback avoid sending completely empty emails, which aligns with the infra-alert objective. However, the warning on empty HTML currently logs fulltoandsubject, which can leak PII into log streams; consider removing these fields or redacting/masking them before logging.
🧹 Nitpick comments (2)
server/src/service/v1/infrastructure/emailService.js (2)
47-110: Multi-path template resolution looks robust; consider making base path configurableThe multi-path search plus logging should resolve ENOENT issues across dev and Docker layouts, and the no-op fallback combined with later MJML checks prevents silent runtime crashes. As a future improvement, you could allow an env/config-provided base template directory to reduce the need to hardcode multiple paths and simplify maintenance.
299-326: Transporter verification is helpful; log more detail on failuresVerifying the transporter before sending is a good safety net, and returning
falseon failures keeps the contract simple. To make diagnosing connectivity/TLS/auth issues easier, consider includingerror.message(and perhaps host/port) in the verification warning log while still avoiding credentials, and optionally reusing a transporter instead of recreating and verifying it on every send if email volume grows.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/service/v1/infrastructure/emailService.js(6 hunks)
🔇 Additional comments (2)
server/src/service/v1/infrastructure/emailService.js (2)
252-296: Conditional TLS wiring and top-level flags align with the stated behaviorBuilding a minimal base
emailConfigand only attaching TLS-related options whensystemEmailSecureis true should prevent TLS flags from being passed to providers like Resend, while still honoring explicitly configured TLS behavior when secure mode is enabled. The separation of top-level flags andtlssub-options looks consistent within this file.
1-330: Remember to re-run Prettier on this file to satisfy CICI previously failed on Prettier for this file; after your latest edits, please re-run the project’s Prettier command (likely with
--write) so the formatting check passes.
|
@gorkem-bwl I believe it's all looking good now, could you please run the check flow |
Sure running, lets see how it goes. |
Owaiseimdad
left a comment
Owaiseimdad
left a comment
There was a problem hiding this comment.
The PR includes good improvements but introduces several behavior changes that could affect production email delivery. Before merging, we should clarify the intention behind TLS changes, pooling removal, and silent template/HTML fallbacks. These should either be reverted for backward compatibility or documented as deliberate changes.
| requireTLS: systemEmailRequireTLS, | ||
| servername: systemEmailTLSServername, | ||
| }, | ||
| ...(systemEmailSecure && { |
There was a problem hiding this comment.
@sameh0 --- thanks for the update! I had a question and a suggestion regarding the new logic that builds the transport config.
The current version uses multiple nested spread operators, and while it works, it becomes quite hard to read and maintain — especially for someone new who is trying to contribute. Nested conditional spreads are clever, but they make the intention of the code harder to understand at a glance.
Another point of confusion: systemEmailRequireTLS and systemEmailIgnoreTLS were previously under the tls object, but in the PR they are now moved to the top-level. Is there a specific reason for that? Just trying to understand the intention — since mixing some TLS fields at the root and others inside tls can make the structure inconsistent.
| return this.compile(templateContent); | ||
| // Try multiple possible paths for template files | ||
| // to support both development and production environments | ||
| const possiblePaths = [ |
There was a problem hiding this comment.
@sameh0 thanks for the contributions, I have some certain points here.
- Very long list of paths = harder to maintain
- Performance overhead for checking 6 paths per template
Why do we need all these fallback paths?
Can we standardize template paths instead of searching everywhere?
| } catch (error) { | ||
| this.logger.error({ | ||
| message: error.message, | ||
| message: `Failed to load template '${templateName}': ${error.message}`, |
There was a problem hiding this comment.
This is good. Better clarity.
| } = config; | ||
|
|
||
| // Validate from address | ||
| const fromAddress = systemEmailAddress || systemEmailUser; |
There was a problem hiding this comment.
@sameh0 any reason why we are considering the user and email address is same here??
| } | ||
| }; | ||
|
|
||
| sendEmail = async (to, subject, html, transportConfig) => { |
There was a problem hiding this comment.
@sameh0 , honestly the sendEmail is becoming bigger and bigger now, harder to maintain then.
Possible to have a clean code, like:
`sendEmail = async (to, subject, html, transportConfig) => {
// 1. Validate incoming email data
html = this.validateEmailParams(to, subject, html);
if (html === false) return false;
// 2. Load config
const config = await this.getTransportConfig(transportConfig);
// 3. Validate from address
const from = this.validateFromAddress(config.systemEmailAddress, config.systemEmailUser);
if (!from) return false;
// 4. Build TLS options
const tlsRelated = this.buildTLSConfig({
secure: config.systemEmailSecure,
systemEmailIgnoreTLS: config.systemEmailIgnoreTLS,
systemEmailRequireTLS: config.systemEmailRequireTLS,
systemEmailRejectUnauthorized: config.systemEmailRejectUnauthorized,
systemEmailTLSServername: config.systemEmailTLSServername,
});
// 5. Build full nodemailer config
const emailConfig = this.buildTransport({ ...config, ...tlsRelated });
// 6. Send
return await this.sendWithTransporter(from, to, subject, html, emailConfig);
};
`
This is just a snapshot which explains what I am thinking, clearly breaking the responsibilities and maintaining it will become much easier.
| }, | ||
| name: systemEmailConnectionHost || "localhost", | ||
| connectionTimeout: 5000, | ||
| pool: systemEmailPool, |
There was a problem hiding this comment.
Can you clarify the reasoning behind that?
pool controls whether Nodemailer keeps SMTP connections open and reuses them, which can significantly improve performance for multiple emails or batch sending.
| to: to, | ||
| subject: subject, | ||
| }); | ||
| html = "<p>Email content unavailable</p>"; |
There was a problem hiding this comment.
why are we sending the email if the content is not available??
|
@Owaiseimdad Thanks for your comments. Yes, it’s indeed getting complex. I do see some of the points you raised, and I’ll reconsider them. However, I just want to clarify that the current implementation on The main thing I’m addressing is getting setups without TLS (like this one) to work. Still, I need to consider pooling and other things. I’ll rework it and get back to you. |
Thanks for the contribution. And yes you are right, the code is getting bigger everyday. Let me know if you have anything in mind. I can jump on and contribute as well if required. |
3 participants
Describe your changes
Fixes Docker template loading and stops TLS flags from being passed when TLS isn't needed.
What Changed
Improved errors: Clearer logs instead of cryptic nodemailer failures
Fixes:
Write your issue number after "Fixes "
Fixes #3054
Please ensure all items are checked off before requesting a review. "Checked off" means you need to add an "x" character between brackets so they turn into checkmarks.
<div>Add</div>, use):npm run formatin server and client directories, which automatically formats your code.Summary by CodeRabbit
Bug Fixes
UI