Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 42 additions & 23 deletions client/src/Pages/Settings/SettingsEmail.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,47 @@ const SettingsEmail = ({
return null;
}

/**
* Build email configuration object for code preview
* Constructs the config incrementally instead of using nested conditional spreads
*/
const buildEmailConfigPreview = () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
};

const config = {
host: systemEmailHost,
port: systemEmailPort,
secure: systemEmailSecure,
auth: {
user: systemEmailUser || systemEmailAddress,
pass: "<your_password>",
},
name: systemEmailConnectionHost || "localhost",
pool: systemEmailPool,
};

if (systemEmailSecure) {
if (systemEmailIgnoreTLS) {
config.ignoreTLS = systemEmailIgnoreTLS;
}
if (systemEmailRequireTLS) {
config.requireTLS = systemEmailRequireTLS;
}

const tlsSettings = {};
if (systemEmailRejectUnauthorized !== undefined) {
tlsSettings.rejectUnauthorized = systemEmailRejectUnauthorized;
}
if (systemEmailTLSServername && systemEmailTLSServername !== "") {
tlsSettings.servername = systemEmailTLSServername;
}

if (Object.keys(tlsSettings).length > 0) {
config.tls = tlsSettings;
}
}

return config;
};

return (
<ConfigBox>
<Box>
Expand Down Expand Up @@ -271,29 +312,7 @@ const SettingsEmail = ({
overflow: "auto",
}}
>
<code>
{JSON.stringify(
{
host: systemEmailHost,
port: systemEmailPort,
secure: systemEmailSecure,
auth: {
user: systemEmailUser || systemEmailAddress,
pass: "<your_password>",
},
name: systemEmailConnectionHost || "localhost",
pool: systemEmailPool,
tls: {
rejectUnauthorized: systemEmailRejectUnauthorized,
ignoreTLS: systemEmailIgnoreTLS,
requireTLS: systemEmailRequireTLS,
servername: systemEmailTLSServername,
},
},
null,
2
)}
</code>
<code>{JSON.stringify(buildEmailConfigPreview(), null, 2)}</code>
</Box>
</Box>

Expand Down
212 changes: 189 additions & 23 deletions server/src/service/infrastructure/emailService.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,3 @@
import { fileURLToPath } from "url";
import path from "path";

const __filename = fileURLToPath(import.meta.url);
const __dirname = path.dirname(__filename);

const SERVICE_NAME = "EmailService";

/**
Expand Down Expand Up @@ -46,16 +40,37 @@ class EmailService {
*/
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`);
Comment on lines 42 to +45

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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");


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;
}
Comment on lines 41 to 74

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

};

Expand Down Expand Up @@ -83,20 +98,169 @@ class EmailService {

buildEmail = async (template, context) => {
try {
if (!this.templateLookup[template]) {
this.logger.error({
message: `Template '${template}' not found in templateLookup`,
service: SERVICE_NAME,
method: "buildEmail",
availableTemplates: Object.keys(this.templateLookup),
});
throw new Error(`Template '${template}' not found`);
}
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`);
}
const mjml = this.templateLookup[template](context);

// Check if MJML is empty (template failed to load)
if (!mjml || mjml.trim() === "") {
const msg = `Template '${template}' returned empty MJML content. Template may have failed to load.`;
this.logger.error({
message: msg,
service: SERVICE_NAME,
method: "buildEmail",
template: template,
});
throw new Error(msg);
}

const html = await this.mjml2html(mjml);

// Check if HTML is empty
if (!html || !html.html) {
const msg = `MJML conversion failed for template '${template}'. No HTML output.`;
this.logger.error({
message: msg,
service: SERVICE_NAME,
method: "buildEmail",
template: template,
mjmlLength: mjml.length,
});
throw new Error(msg);
}

return html.html;
} catch (error) {
this.logger.error({
message: error.message,
message: `Failed to build email for template '${template}': ${error.message}`,
service: SERVICE_NAME,
method: "buildEmail",
template: template,
error: error.message,
stack: error.stack,
});
throw error;
}
};

/**
* Validates email parameters.
* @param {string} to - The recipient email address.
* @param {string} subject - The email subject.
* @param {string} html - The email HTML content.
* @returns {boolean} True if valid, false otherwise.
*/
validateEmailParams(to, subject, html) {
if (!to || !subject) {
this.logger.error({
message: "Invalid email parameters: missing 'to' or 'subject'",
service: SERVICE_NAME,
method: "validateEmailParams",
});
return false;
}

if (!html || html.trim() === "") {
this.logger.error({
message: "Cannot send email: HTML content is empty",
service: SERVICE_NAME,
method: "validateEmailParams",
});
return false;
}

return true;
}

/**
* Validates the from email address.
* @param {string} systemEmailAddress - The system email address.
* @returns {boolean} True if valid, false otherwise.
*/
validateFromAddress(systemEmailAddress) {
const fromAddress = systemEmailAddress;
if (!fromAddress || !fromAddress.includes("@")) {
this.logger.error({
message: "Missing or invalid systemEmailAddress - a valid email address is required",
service: SERVICE_NAME,
method: "validateFromAddress",
});
return false;
}
return true;
}

/**
* Builds the TLS configuration for the email transporter.
* @param {Object} config - The email configuration object.
* @returns {Object} The TLS configuration object.
*/
buildTLSConfig(config) {
const { systemEmailSecure, systemEmailIgnoreTLS, systemEmailRequireTLS, systemEmailRejectUnauthorized, systemEmailTLSServername } = config;

const tlsConfig = {};

// Only apply TLS settings if secure is enabled

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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;
}
Comment on lines +214 to +256

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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:


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.


sendEmail = async (to, subject, html, transportConfig) => {
// Validate email parameters
if (!this.validateEmailParams(to, subject, html)) {
return false;
}

let config;
if (typeof transportConfig !== "undefined") {
config = transportConfig;
Expand All @@ -107,35 +271,35 @@ class EmailService {
systemEmailHost,
systemEmailPort,
systemEmailSecure,
systemEmailPool,
systemEmailUser,
systemEmailAddress,
systemEmailPassword,
systemEmailConnectionHost,
systemEmailTLSServername,
systemEmailIgnoreTLS,
systemEmailRequireTLS,
systemEmailRejectUnauthorized,
} = config;

// Validate from address
if (!this.validateFromAddress(systemEmailAddress)) {
return false;
}

// Build base email config
const emailConfig = {
host: systemEmailHost,
port: Number(systemEmailPort),
secure: systemEmailSecure,
pool: config.systemEmailPool ?? false,
auth: {
user: systemEmailUser || systemEmailAddress,
pass: systemEmailPassword,
},
name: systemEmailConnectionHost || "localhost",
connectionTimeout: 5000,
pool: systemEmailPool,
tls: {
rejectUnauthorized: systemEmailRejectUnauthorized,
ignoreTLS: systemEmailIgnoreTLS,
requireTLS: systemEmailRequireTLS,
servername: systemEmailTLSServername,
},
};

// Apply TLS configuration
const tlsConfig = this.buildTLSConfig(config);
Object.assign(emailConfig, tlsConfig);

this.transporter = this.nodemailer.createTransport(emailConfig);

try {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand All @@ -144,7 +308,8 @@ class EmailService {
this.logger.warn({
message: "Email transporter verification failed",
service: SERVICE_NAME,
method: "verifyTransporter",
method: "sendEmail",
error: error.message,
});
return false;
}
Expand All @@ -164,6 +329,7 @@ class EmailService {
method: "sendEmail",
stack: error.stack,
});
return false;
}
};
}
Expand Down