Skip to content

Conversation

@hodorogandrei
Copy link
Contributor

@hodorogandrei hodorogandrei commented Dec 3, 2025

Summary

This PR fixes a bug in GhostMailer.js where transactional emails fail to send when no explicit from address is provided.

The Problem

In getFromAddress() (line 32-51), when requestedFromAddress is not provided, the code sets:

requestedFromAddress = emailAddress.service.defaultFromEmail;

However, defaultFromEmail returns an object {address: string, name: string}, not a string.

This object is then passed to getAddressFromString(requestedFromAddress, ...) which internally calls EmailAddressParser.parse(from). The parse() function expects a string input and returns null when given an object:

// Testing EmailAddressParser.parse() behavior:
EmailAddressParser.parse('test@example.com')  // => {address: 'test@example.com', name: undefined}
EmailAddressParser.parse({address: 'test@example.com'})  // => null

This causes downstream failures when trying to access addresses.from.name on a null value.

Impact

This bug affects all transactional emails that don't explicitly specify a from address:

  • Staff device verification (2FA) emails - Users cannot log in when staffDeviceVerification is enabled
  • Password reset emails - Users cannot reset their passwords
  • Member magic link emails - Members cannot sign in
  • Any other system emails using the default sender

The error message shown to users is unhelpful: "Failed to send email. Please check your site configuration and try again." - even when the SMTP configuration is correct.

The Fix

Convert the email object to a string using EmailAddressParser.stringify() before passing it to getAddressFromString():

// Before (buggy):
requestedFromAddress = emailAddress.service.defaultFromEmail;

// After (fixed):
const defaultEmail = emailAddress.service.defaultFromEmail;
requestedFromAddress = EmailAddressParser.stringify(defaultEmail);

Test Added

Added a test case in GhostMailer.test.js that verifies emails are sent correctly when no from address is provided, which exercises the defaultFromEmail code path.

Test Plan

  1. Enable staffDeviceVerification in config
  2. Configure SMTP settings (any provider)
  3. Attempt to log in to Ghost Admin
  4. Verify that the verification code email is sent successfully
  5. Also test password reset and member magic link emails

Reproduction Steps (for the bug)

  1. Set up Ghost with SMTP configured (not Mailgun)
  2. Enable staffDeviceVerification: true in config
  3. Try to log in to Ghost Admin
  4. Observe the "Failed to send email" error even though SMTP credentials are correct

Note

Fixes sender handling by stringifying defaultFromEmail when no from is provided; adds unit coverage and updates e2e expectations for welcome email sender.

  • Mail Service:
    • Fix getFromAddress in ghost/core/core/server/services/mail/GhostMailer.js to stringify emailAddress.service.defaultFromEmail via EmailAddressParser.stringify when from is missing.
  • Tests:
    • Unit: Add test in ghost/core/test/unit/server/services/mail/GhostMailer.test.js verifying correct default sender ("<site title>" <noreply@<domain>>) when no from is provided.
    • E2E: Update e2e/tests/public/member-signup.test.ts welcome email assertions to expect From.Name contains site title and From.Address contains noreply@.

Written by Cursor Bugbot for commit 831a6bd. This will update automatically on new commits. Configure here.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 3, 2025

Walkthrough

getFromAddress in GhostMailer.js now converts the defaultFromEmail object to a string using EmailAddressParser.stringify when no explicit from address is provided, so getAddressFromString receives a string. A unit test was added to verify default-from behavior. E2E test expectations were updated to assert From.Name matches the site title and From.Address uses the derived noreply@... address.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Files to inspect closely:
    • ghost/core/core/server/services/mail/GhostMailer.js — verify EmailAddressParser.stringify usage and that parsing behavior is correct.
    • ghost/core/test/unit/server/services/mail/GhostMailer.test.js — confirm the new test is deterministic and covers the intended case.
    • e2e/tests/public/member-signup.test.ts — ensure updated expectations match intended behavior and are not flaky.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and specifically describes the main bug fix: stringifying defaultFromEmail before passing to getAddressFromString, which is the core change across all modified files.
Description check ✅ Passed The description is detailed and directly related to the changeset, explaining the bug, its impact on transactional emails, the fix, and tests added.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c4d657f and 831a6bd.

📒 Files selected for processing (3)
  • e2e/tests/public/member-signup.test.ts (1 hunks)
  • ghost/core/core/server/services/mail/GhostMailer.js (1 hunks)
  • ghost/core/test/unit/server/services/mail/GhostMailer.test.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • ghost/core/core/server/services/mail/GhostMailer.js
  • e2e/tests/public/member-signup.test.ts
  • ghost/core/test/unit/server/services/mail/GhostMailer.test.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: Ghost-CLI tests
  • GitHub Check: Legacy tests (Node 22.18.0, mysql8)
  • GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
  • GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
  • GitHub Check: Unit tests (Node 22.18.0)
  • GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
  • GitHub Check: Lint
  • GitHub Check: Build & Push
  • GitHub Check: Cursor Bugbot

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

…mString

When no from address is provided (e.g., when sending staff device verification
emails), getFromAddress() sets requestedFromAddress to
emailAddress.service.defaultFromEmail which returns an object {address, name}.

This object was then passed directly to getAddressFromString() which calls
EmailAddressParser.parse(from) internally. However, parse() expects a string
input and returns null when given an object, causing email sends to fail.

This bug affects all transactional emails that dont explicitly specify a from
address, including staff device verification (2FA) emails, password reset
emails, and member magic link emails.

The fix converts the email object to a string using EmailAddressParser.stringify()
before passing it to getAddressFromString().

Also adds a test case to verify this behavior.
The test was checking for 'Ghost' in From.Name and 'test@example.com' in
From.Address, but these expectations don't match the actual code behavior:

1. From.Name should be the site title ('Test Blog' set via UserFactory),
   not 'Ghost'. The name 'Ghost at [domain]' is only used as a fallback
   when no site title is configured.

2. From.Address should be the noreply address derived from the site URL
   (noreply@localhost for local tests), not 'test@example.com' which
   is the admin user's email, not the mail.from config.

Updated the test to check for the correct expected values.
@hodorogandrei hodorogandrei force-pushed the fix/ghostmailer-default-from-address-v2 branch from c4d657f to 831a6bd Compare December 3, 2025 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants