Fix email template loading and conditional TLS configuration#3134
Fix email template loading and conditional TLS configuration#3134gorkem-bwl wants to merge 1 commit intodevelopfrom
Conversation
## Changes ### Server-side (emailService.js) - Fixed template path resolution: Changed default from `templates/` to `src/templates/` to correctly locate email templates in the project structure - Added EMAIL_TEMPLATE_PATH environment variable support for Docker deployments - Added input validation (validateEmailParams, validateFromAddress) to catch missing fields before attempting to send - Implemented conditional TLS configuration - TLS settings only applied when systemEmailSecure is enabled, allowing services like Resend to work properly - Added stronger content validation to prevent empty emails from being sent - Added transporter.verify() before sending to catch configuration issues early - Improved error logging with more context for debugging ### Client-side (SettingsEmail.jsx) - Refactored settings preview to use buildEmailConfigPreview() builder function - TLS fields now only shown in JSON preview when secure mode is enabled Fixes #3054
gorkem-bwl
requested review from
Br0wnHammer,
Owaiseimdad,
ajhollid and
karenvicent
as code owners
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Email Settings UI client/src/Pages/Settings/SettingsEmail.jsx |
Extracted email configuration preview logic into a new buildEmailConfigPreview() helper function that conditionally includes TLS-related fields (ignoreTLS, requireTLS, tls sub-object) only when relevant settings exist; replaced inline JSON.stringify with builder-based approach. |
Email Service Infrastructure server/src/service/infrastructure/emailService.js |
Added three validation/builder methods: validateEmailParams(), validateFromAddress(), buildTLSConfig(). Enhanced template loading with configurable EMAIL_TEMPLATE_PATH env var, preloading validation, and detailed error logging. Refactored email sending workflow to include pre-flight validation and compose TLS config via dedicated builder instead of inline object. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
A rabbit hops through email streams,
With validations, templates, TLS dreams,
Configuration preview builds with care,
No more silent errors floating in the air! 🐰📧✨
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title accurately summarizes the main changes: fixing email template loading and implementing conditional TLS configuration. |
| Description check | ✅ Passed | The description provides clear summaries of changes, issue references, and test plans, though the description template checklist items are not marked. |
| Linked Issues check | ✅ Passed | The PR directly addresses issue #3054 by fixing template path resolution, adding error handling, validation, and conditional TLS support to resolve empty infrastructure alert emails. |
| Out of Scope Changes check | ✅ Passed | All changes are focused on email service functionality: server-side template loading, validation, and TLS configuration; client-side preview builder. No unrelated changes detected. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
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.
![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
🤖 Fix all issues with AI agents
In `@server/src/service/infrastructure/emailService.js`:
- Around line 41-74: The async init() is called without await in the constructor
while loadTemplate() now throws, causing unhandled rejections and possibly
leaving templateLookup unset; fix by making initialization synchronous or
properly awaiting init in the constructor: change init() to a synchronous method
(remove async/await) that calls loadTemplate() inside a try/catch and rethrows
on failure, or keep init async but await this.init() in the constructor so
failures propagate; ensure templateLookup (the field populated by loadTemplate
calls) is always set on success and that any loadTemplate() throw is allowed to
propagate (or is caught and rethrown) so the service never finishes construction
in a half-initialized state.
- Around line 214-256: The buildTLSConfig function currently gates all TLS
options behind systemEmailSecure; instead, remove the outer if
(systemEmailSecure) and set tlsConfig.ignoreTLS and tlsConfig.requireTLS
whenever systemEmailIgnoreTLS/systemEmailRequireTLS are explicitly provided, and
build tlsConfig.tls (with rejectUnauthorized and servername from
systemEmailRejectUnauthorized and systemEmailTLSServername) whenever those
values are set — this ensures ignoreTLS/requireTLS apply for STARTTLS (secure:
false) and tls settings apply for both implicit TLS and STARTTLS; keep the
existing debug logging when tlsConfig.tls is populated and reference
buildTLSConfig, systemEmailIgnoreTLS, systemEmailRequireTLS,
systemEmailRejectUnauthorized, and systemEmailTLSServername to locate the
changes.
🧹 Nitpick comments (2)
client/src/Pages/Settings/SettingsEmail.jsx (2)
104-143: Clean implementation of conditional TLS preview.The builder pattern here improves readability over nested conditional spreads. A couple of minor simplifications:
- Line 130: The
!== undefinedcheck is redundant sincesystemEmailRejectUnauthorizedhas a default value oftrue(line 45).- Line 133: The empty string check is redundant—a truthy check on a string already excludes empty strings.
🔧 Optional simplification
const tlsSettings = {}; - if (systemEmailRejectUnauthorized !== undefined) { - tlsSettings.rejectUnauthorized = systemEmailRejectUnauthorized; - } - if (systemEmailTLSServername && systemEmailTLSServername !== "") { + tlsSettings.rejectUnauthorized = systemEmailRejectUnauthorized; + if (systemEmailTLSServername) { tlsSettings.servername = systemEmailTLSServername; } + + if (Object.keys(tlsSettings).length > 0) { + config.tls = tlsSettings; + }
256-281: Consider hiding TLS switches when secure mode is disabled.The TLS-related switches (
ignoreTLS,requireTLS,rejectUnauthorized) are always displayed, but their values only appear in the preview whensystemEmailSecureistrue. This matches the server-side conditional TLS behavior per the PR objectives, but users might be confused when they toggle these switches and see no change in the preview.Consider conditionally rendering these switches only when secure mode is enabled, or adding a hint that they only take effect when secure mode is on.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/src/Pages/Settings/SettingsEmail.jsxserver/src/service/infrastructure/emailService.js
🧰 Additional context used
🧬 Code graph analysis (2)
client/src/Pages/Settings/SettingsEmail.jsx (1)
server/src/config/index.ts (1)
config(16-25)
server/src/service/infrastructure/emailService.js (1)
server/src/config/index.ts (1)
config(16-25)
🔇 Additional comments (4)
client/src/Pages/Settings/SettingsEmail.jsx (1)
315-315: LGTM!The preview now correctly reflects the conditional TLS configuration, aligning with the server-side changes where TLS settings are only applied when
systemEmailSecureis enabled.server/src/service/infrastructure/emailService.js (3)
101-159: Template existence/type checks and empty-content guards look solid.Good defensive validation and logging around missing templates and empty MJML/HTML outputs.
162-207: Validation helpers are concise and improve failure visibility.Clear input checks with targeted logging improve debuggability without cluttering the send flow.
258-333: Pre-flight validation and verify-before-send flow looks good.Early parameter/from validation plus
transporter.verify()beforesendMail()makes failures explicit and easier to diagnose.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| this.loadTemplate = (templateName) => { | ||
| try { | ||
| const templatePath = this.path.join(__dirname, `../../../templates/${templateName}.mjml`); | ||
| // Use EMAIL_TEMPLATE_PATH environment variable or default to src/templates/ in cwd | ||
| const templateBase = process.env.EMAIL_TEMPLATE_PATH || this.path.join(process.cwd(), "src", "templates"); | ||
| const templatePath = this.path.join(templateBase, `${templateName}.mjml`); | ||
|
|
||
| this.logger.debug({ | ||
| message: `Loading template: ${templateName}`, | ||
| service: SERVICE_NAME, | ||
| method: "loadTemplate", | ||
| templatePath: templatePath, | ||
| }); | ||
|
|
||
| const templateContent = this.fs.readFileSync(templatePath, "utf8"); | ||
| return this.compile(templateContent); | ||
| const compiled = this.compile(templateContent); | ||
|
|
||
| this.logger.debug({ | ||
| message: `Template loaded successfully: ${templateName}`, | ||
| service: SERVICE_NAME, | ||
| method: "loadTemplate", | ||
| }); | ||
| return compiled; | ||
| } catch (error) { | ||
| this.logger.error({ | ||
| message: error.message, | ||
| message: `Failed to load template '${templateName}': ${error.message}`, | ||
| service: SERVICE_NAME, | ||
| method: "loadTemplate", | ||
| templateName: templateName, | ||
| error: error.message, | ||
| stack: error.stack, | ||
| }); | ||
| // Fail fast - throw error instead of returning empty function | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Avoid unhandled init failures after loadTemplate now throws.
init is async and invoked without await in the constructor. With loadTemplate now throwing, this creates an unhandled rejection and can leave templateLookup unset while the service still constructs. Consider making init synchronous (it currently doesn’t await) so failures propagate immediately.
🔧 Proposed fix
- init = async () => {
+ init = () => {
/**
* Loads an email template from the filesystem.
*
* `@param` {string} templateName - The name of the template to load.
* `@returns` {Function} A compiled template function that can be used to generate HTML email content.
*/🤖 Prompt for AI Agents
In `@server/src/service/infrastructure/emailService.js` around lines 41 - 74, The
async init() is called without await in the constructor while loadTemplate() now
throws, causing unhandled rejections and possibly leaving templateLookup unset;
fix by making initialization synchronous or properly awaiting init in the
constructor: change init() to a synchronous method (remove async/await) that
calls loadTemplate() inside a try/catch and rethrows on failure, or keep init
async but await this.init() in the constructor so failures propagate; ensure
templateLookup (the field populated by loadTemplate calls) is always set on
success and that any loadTemplate() throw is allowed to propagate (or is caught
and rethrown) so the service never finishes construction in a half-initialized
state.
| buildTLSConfig(config) { | ||
| const { systemEmailSecure, systemEmailIgnoreTLS, systemEmailRequireTLS, systemEmailRejectUnauthorized, systemEmailTLSServername } = config; | ||
|
|
||
| const tlsConfig = {}; | ||
|
|
||
| // Only apply TLS settings if secure is enabled | ||
| if (systemEmailSecure) { | ||
| // Top-level Nodemailer options (control STARTTLS behavior): | ||
| // - ignoreTLS: If true, the connection will not attempt to use STARTTLS | ||
| // - requireTLS: If true, connection will fail if STARTTLS is not available | ||
| if (systemEmailIgnoreTLS !== undefined) { | ||
| tlsConfig.ignoreTLS = systemEmailIgnoreTLS; | ||
| } | ||
| if (systemEmailRequireTLS !== undefined) { | ||
| tlsConfig.requireTLS = systemEmailRequireTLS; | ||
| } | ||
|
|
||
| // Node.js TLS options (go inside the 'tls' object): | ||
| // - rejectUnauthorized: If false, accepts self-signed certificates | ||
| // - servername: Overrides the hostname for SNI (Server Name Indication) | ||
| const tlsSettings = {}; | ||
|
|
||
| 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) { | ||
| tlsConfig.tls = tlsSettings; | ||
| this.logger.debug({ | ||
| message: `TLS settings applied to email config`, | ||
| service: SERVICE_NAME, | ||
| method: "buildTLSConfig", | ||
| tlsSettings: Object.keys(tlsSettings), | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| return tlsConfig; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Nodemailer ignoreTLS requireTLS tls options STARTTLS secure false behavior
💡 Result:
Short summary — how Nodemailer handles TLS / STARTTLS and the relevant options:
-
secure (default false): if true, connect with implicit TLS (port 465). If false, connection starts plaintext and may be upgraded to TLS with STARTTLS. Nodemailer will automatically use STARTTLS when offered unless you override it. [1][2]
-
ignoreTLS (default false): when true, Nodemailer will not attempt STARTTLS even if the server advertises it — the connection stays unencrypted (unless you used secure:true/port465). Use this to force plaintext. [1][2]
-
requireTLS (default false): when true and secure is false, Nodemailer requires a successful STARTTLS upgrade. If the server does not support STARTTLS or the upgrade fails, sending fails. Use this to enforce encryption on submission ports (e.g., 587). [1][2]
-
opportunisticTLS (SMTP-connection option): attempts STARTTLS but will continue unencrypted if STARTTLS fails (useful when you prefer TLS but accept fallback). [2]
-
tls (object): forwarded to Node.js tls.connect()/createSecureContext (e.g., { rejectUnauthorized: false }, servername, ca, key, cert, ciphers). Use it to control certificate validation, SNI, client certs, etc. These options apply to the TLS socket created by STARTTLS or implicit TLS. [2][3]
Practical notes:
- secure:false does NOT mean “never encrypted” — STARTTLS will normally upgrade the connection. Set ignoreTLS:true to prevent that. Use requireTLS:true to fail when STARTTLS is unavailable. [1][2]
- If connecting to port 465, set secure:true; for port 587 use secure:false plus requireTLS if you want encryption required. [1][2]
Sources:
- Nodemailer SMTP docs (options.secure, ignoreTLS, requireTLS). [1]
- Nodemailer SMTP-Connection reference (opportunisticTLS, tls object, detailed behaviors). [2]
- npm package notes summarizing the same options. [3]
🌐 Web query:
Nodemailer secure false STARTTLS ignoreTLS requireTLS rejectUnauthorized when applied
💡 Result:
Summary of how these Nodemailer options interact:
- secure: true — use TLS from the start (implicit TLS, normally port 465). (nodemailer.com)
- secure: false — connect unencrypted initially (typical for port 587 or 25). Nodemailer will attempt STARTTLS to upgrade to TLS if the server advertises it (unless disabled). (nodemailer.com)
- ignoreTLS: true — do not attempt STARTTLS at all; connection remains unencrypted when secure: false. (Has no effect when secure: true.) (nodemailer.com)
- requireTLS: true — require a successful STARTTLS upgrade when secure: false; fail the send if the server does not support STARTTLS or the upgrade fails. (nodemailer.com)
- opportunisticTLS (related) — attempt STARTTLS but continue unencrypted if upgrade fails (opposite of requireTLS). (nodemailer.com)
- tls.rejectUnauthorized (aka rejectUnauthorized) — Node.js TLS option passed in transporter tls: { rejectUnauthorized: }. If false, certificate validation is skipped (useful for self-signed certs in development; insecure for production). Use tls.servername when connecting by IP to validate the certificate name. (deepwiki.com)
Practical combinations:
- secure: true + tls.rejectUnauthorized: false — connect over TLS immediately and skip cert validation. (nodemailer.com)
- secure: false + requireTLS: true — plain connect then MUST upgrade with STARTTLS or fail. (nodemailer.com)
- secure: false + ignoreTLS: true — force an unencrypted session (no STARTTLS). (nodemailer.com)
References: Nodemailer SMTP docs and TLS/connection docs. (nodemailer.com)
Citations:
- 1: https://nodemailer.com/smtp?utm_source=openai
- 2: https://nodemailer.com/smtp?utm_source=openai
- 3: https://nodemailer.com/smtp?utm_source=openai
- 4: https://nodemailer.com/smtp?utm_source=openai
- 5: https://nodemailer.com/extras/smtp-connection?utm_source=openai
- 6: https://deepwiki.com/nodemailer/nodemailer/5.4-tlsssl-options?utm_source=openai
- 7: https://nodemailer.com/errors?utm_source=openai
- 8: https://nodemailer.com/extras/smtp-connection?utm_source=openai
- 9: https://nodemailer.com/smtp?utm_source=openai
- 10: https://nodemailer.com/smtp?utm_source=openai
TLS options are incorrectly gated by systemEmailSecure and should be applied independently.
In Nodemailer, ignoreTLS/requireTLS control STARTTLS behavior and are meaningful when secure is false. The tls object (rejectUnauthorized, servername, etc.) applies to both implicit TLS (secure: true) and STARTTLS (secure: false) connections. Gating them all behind systemEmailSecure silently ignores TLS requirements set by the administrator — for example, requireTLS: true won't be applied when secure: false, defeating enforcement of required encryption on submission ports.
Remove the outer if (systemEmailSecure) guard and apply each TLS option whenever explicitly set.
🤖 Prompt for AI Agents
In `@server/src/service/infrastructure/emailService.js` around lines 214 - 256,
The buildTLSConfig function currently gates all TLS options behind
systemEmailSecure; instead, remove the outer if (systemEmailSecure) and set
tlsConfig.ignoreTLS and tlsConfig.requireTLS whenever
systemEmailIgnoreTLS/systemEmailRequireTLS are explicitly provided, and build
tlsConfig.tls (with rejectUnauthorized and servername from
systemEmailRejectUnauthorized and systemEmailTLSServername) whenever those
values are set — this ensures ignoreTLS/requireTLS apply for STARTTLS (secure:
false) and tls settings apply for both implicit TLS and STARTTLS; keep the
existing debug logging when tlsConfig.tls is populated and reference
buildTLSConfig, systemEmailIgnoreTLS, systemEmailRequireTLS,
systemEmailRejectUnauthorized, and systemEmailTLSServername to locate the
changes.
![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 fixes critical email template loading issues but introduces breaking changes in path resolution and TLS configuration that could silently fail existing deployments.
🌟 Strengths
- Adds enhanced validation and error handling for more reliable email delivery.
- Improves configurability with environment variable support for Docker deployments.
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P1 | server/.../emailService.js | Architecture | Breaks template loading without EMAIL_TEMPLATE_PATH env var. | |
| P1 | server/.../emailService.js | Architecture | Silently ignores TLS settings when secure=false, breaking STARTTLS. | method:buildTLSConfig, path:client/src/Utils/NetworkService.js |
| P2 | server/.../emailService.js | Maintainability | Merges validation and send failures, obscuring error causes. | method:sendEmail, path:server/src/service/infrastructure/notificationService.js |
| P2 | server/.../emailService.js | Performance | Adds network handshake per email, increasing latency. | method:sendEmail |
| P2 | client/.../SettingsEmail.jsx | Testing | Preview shows incorrect TLS logic, misleading configuration validation. |
🔍 Notable Themes
- Architectural breaking changes: Modifications to path resolution and TLS gating alter fundamental assumptions, potentially impacting existing deployments without clear migration paths.
- Error handling side effects: While validation is improved, it introduces merged failure modes and performance overheads that could hinder debugging and scalability.
📈 Risk Diagram
This diagram illustrates the risk of silent TLS configuration failure when systemEmailSecure is false.
sequenceDiagram
participant C as Client (SettingsEmail.jsx)
participant ES as EmailService
participant S as SMTP Server
C->>ES: Configure email with secure=false, ignoreTLS=false
ES->>ES: Build config (ignores TLS settings due to secure=false)
note over ES: R2(P1): TLS settings ignored, risking STARTTLS failure
ES->>S: Send email without proper TLS negotiation
S-->>ES: Connection failure or insecure transmission
⚠️ **Unanchored Suggestions (Manual Review Recommended)**
The following suggestions could not be precisely anchored to a specific line in the diff. This can happen if the code is outside the changed lines, has been significantly refactored, or if the suggestion is a general observation. Please review them carefully in the context of the full file.
📁 File: server/src/service/infrastructure/emailService.js
The PR adds validation that returns false on failure, changing the method's error handling contract. The related_context shows notificationService.js calls sendEmail and checks if (!messageId) return false;. This pattern already handles the false return. However, the new validation returns false for validation failures, while the original method returned false only for transporter send failures (after the try-catch). This merges two semantically different failure modes (invalid input vs. network failure) into the same sentinel value, making debugging harder. Callers cannot distinguish between "bad configuration" and "send failed." The validation errors are logged but not propagated.
Suggestion:
// Option A: Throw validation errors (breaking but clear)
if (!this.validateEmailParams(to, subject, html)) {
throw new Error('Invalid email parameters');
}
// Option B: Return a structured error object
const validationError = this.validateEmailParamsStructured(to, subject, html);
if (validationError) {
return { success: false, reason: 'validation', error: validationError };
}
// Later in notificationService: check result.successRelated Code:
// Validate email parameters
if (!this.validateEmailParams(to, subject, html)) {
return false;
}
// ... later in the method ...
// Validate from address
if (!this.validateFromAddress(systemEmailAddress)) {
return false;
}💡 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.
| try { | ||
| const templatePath = this.path.join(__dirname, `../../../templates/${templateName}.mjml`); | ||
| // Use EMAIL_TEMPLATE_PATH environment variable or default to src/templates/ in cwd | ||
| const templateBase = process.env.EMAIL_TEMPLATE_PATH || this.path.join(process.cwd(), "src", "templates"); | ||
| const templatePath = this.path.join(templateBase, `${templateName}.mjml`); |
There was a problem hiding this comment.
P1 | Confidence: High
The PR changes the default template search path from using __dirname with a relative path (../../../templates/) to using process.cwd() with src/templates. This changes the resolution behavior relative to the server's working directory instead of the file location. The related_context shows the server is started via server/src/index.js, which sets __dirname relative to that entry point. If the server is started from a different directory (common in Docker containers), process.cwd() will point elsewhere. While the PR adds EMAIL_TEMPLATE_PATH env var for flexibility, the change is a breaking architectural assumption about the execution context. Consumers relying on the previous implicit path resolution (relative to the source file) will fail unless they set the new environment variable, which is not backward-compatible.
| try { | |
| const templatePath = this.path.join(__dirname, `../../../templates/${templateName}.mjml`); | |
| // Use EMAIL_TEMPLATE_PATH environment variable or default to src/templates/ in cwd | |
| const templateBase = process.env.EMAIL_TEMPLATE_PATH || this.path.join(process.cwd(), "src", "templates"); | |
| const templatePath = this.path.join(templateBase, `${templateName}.mjml`); | |
| // Option 1: Keep __dirname-based path but fix the relative depth | |
| const projectRoot = this.path.join(__dirname, '../../..'); | |
| const templateBase = process.env.EMAIL_TEMPLATE_PATH || this.path.join(projectRoot, "src", "templates"); |
|
|
||
| const tlsConfig = {}; | ||
|
|
||
| // Only apply TLS settings if secure is enabled |
There was a problem hiding this comment.
P1 | Confidence: High
The PR gates TLS configuration (including ignoreTLS, requireTLS, and tls object) exclusively when systemEmailSecure is true. This is a breaking change for SMTP servers that use STARTTLS (common on port 587). systemEmailSecure=false typically means "use unencrypted connection initially, but upgrade via STARTTLS." The related_context shows the client's NetworkService (client/src/Utils/NetworkService.js) sends all TLS fields (including ignoreTLS, requireTLS) regardless of the secure flag. If a user has systemEmailSecure=false but depends on ignoreTLS=false (to enforce STARTTLS) or requireTLS=true, their configuration will silently stop working—the TLS control fields will be ignored. The PR's client-side preview change (buildEmailConfigPreview) mirrors this incorrect logic, providing false configuration validation.
Code Suggestion:
// Build TLS configuration that respects both secure and STARTTLS scenarios
const tlsConfig = {};
// These are top-level Nodemailer options for STARTTLS behavior, relevant even when secure=false
if (systemEmailIgnoreTLS !== undefined) {
tlsConfig.ignoreTLS = systemEmailIgnoreTLS;
}
if (systemEmailRequireTLS !== undefined) {
tlsConfig.requireTLS = systemEmailRequireTLS;
}
// Node.js TLS socket options (inside 'tls' object) should be applied when using TLS (secure OR STARTTLS)
// Determine if TLS will be used: either secure connection OR STARTTLS is not ignored
const willUseTLS = systemEmailSecure || (systemEmailIgnoreTLS === false);
if (willUseTLS) {
const tlsSettings = {};
if (systemEmailRejectUnauthorized !== undefined) {
tlsSettings.rejectUnauthorized = systemEmailRejectUnauthorized;
}
if (systemEmailTLSServername) {
tlsSettings.servername = systemEmailTLSServername;
}
if (Object.keys(tlsSettings).length > 0) {
tlsConfig.tls = tlsSettings;
}
}
return tlsConfig;Evidence: method:buildTLSConfig, path:client/src/Utils/NetworkService.js
|
|
||
| this.transporter = this.nodemailer.createTransport(emailConfig); | ||
|
|
||
| try { |
There was a problem hiding this comment.
P2 | Confidence: Medium
Adding transporter.verify() before every email send introduces a synchronous network handshake with the SMTP server, doubling the latency for each email. For high-volume alerting or bulk operations, this could create performance bottlenecks. The verification is valuable for catching configuration errors early, but it should be performed once during service initialization or connection pooling setup, not per-send. The PR's approach adds robustness at the cost of runtime performance without considering the operational impact.
Code Suggestion:
// In init() or a dedicated connect() method
async initializeTransporter(config) {
this.transporter = this.nodemailer.createTransport(emailConfig);
try {
await this.transporter.verify();
this.transporterVerified = true;
} catch (error) {
this.logger.warn({ message: 'Transporter verification failed', error: error.message });
this.transporterVerified = false;
// Optionally still keep transporter but log warning
}
}
// In sendEmail, skip verify() if already verified, or add a retry/refresh mechanism.Evidence: method:sendEmail
| * Build email configuration object for code preview | ||
| * Constructs the config incrementally instead of using nested conditional spreads | ||
| */ | ||
| const buildEmailConfigPreview = () => { |
There was a problem hiding this comment.
P2 | Confidence: Medium
The refactored buildEmailConfigPreview function replicates the same incorrect TLS conditional logic as the server-side buildTLSConfig. This creates a validation mismatch: the client preview will show a configuration that differs from what the server will actually use when systemEmailSecure=false but TLS options are set. This defeats the purpose of the preview, as users will see an incomplete config and not realize their TLS settings are being ignored. The client preview should accurately reflect the server's actual configuration logic.
Code Suggestion:
const buildEmailConfigPreview = () => {
const config = { /* base config */ };
// Mirror the corrected server-side logic
if (systemEmailIgnoreTLS !== undefined) {
config.ignoreTLS = systemEmailIgnoreTLS;
}
if (systemEmailRequireTLS !== undefined) {
config.requireTLS = systemEmailRequireTLS;
}
const willUseTLS = systemEmailSecure || (systemEmailIgnoreTLS === false);
if (willUseTLS) {
const tlsSettings = {};
// ... add rejectUnauthorized, servername
if (Object.keys(tlsSettings).length > 0) {
config.tls = tlsSettings;
}
}
return config;
};
1 participant
Summary
Changes
Server-side (emailService.js)
templates/tosrc/templates/to correctly locate email templatesEMAIL_TEMPLATE_PATHfor Docker deploymentsvalidateEmailParams()andvalidateFromAddress()methodssystemEmailSecureis enabledtransporter.verify()before sendingClient-side (SettingsEmail.jsx)
buildEmailConfigPreview()builder functionTest plan
Fixes #3054
Supersedes #3066
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.