Skip to content

Postmark integration#22771

Closed
peterpacket wants to merge 100 commits intoTryGhost:mainfrom
madewithlove:main
Closed

Postmark integration#22771
peterpacket wants to merge 100 commits intoTryGhost:mainfrom
madewithlove:main

Conversation

@peterpacket
Copy link
Copy Markdown

@peterpacket peterpacket commented Apr 2, 2025

Why are you making it?

We use postmark to send our mails, but we are missing a postmark integration for Ghost.

What does it do?

It add postmark as a second mail service provider

Why is this something Ghost users or developers need?

For Ghost users where mailgun is not an option.

Please check your PR against these items:

  • I've read and followed the Contributor Guide
  • I've explained my change
  • I've written an automated test to prove my change works

This a copy of #20530, submitting it again because our main branch got corrupted due to a merge conflict.


Note

High Risk
Touches core newsletter delivery (sending, analytics ingestion, suppressions) and introduces new provider-specific error paths; the PR also includes a broken unit test file with merge-conflict markers.

Overview
Adds Postmark as a second bulk email provider alongside Mailgun, selectable at runtime via new settings (bulk_email_provider, postmark_api_token) and a new provider selector in Admin-X.

Backend email sending now instantiates either MailgunClient or the new @tryghost/postmark-client via EmailServiceWrapper.getMailClient(), and the email suppression service is switched to use this same client. Email analytics now registers a new @tryghost/email-analytics-provider-postmark provider, and public config exposes postmarkIsConfigured so the admin UI/publish flow gates “send email” on either provider being configured.

Notable gaps introduced in the diff: BulkEmailProvider has a TODO for Postmark error handling, and ghost/core/test/unit/server/services/email-service/bulk-email-provider.test.js contains unresolved merge conflict markers (tests will not run).

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

onEditingChange={handleEditingChange}
onSave={async () => {
handleSave();
}}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: Missing Mailgun region default value handling

The new BulkEmail component removes the special handling that existed in the old Mailgun component for setting the default Mailgun region. When a user saves Mailgun settings without explicitly selecting a region, the mailgun_base_url remains null because the Select component doesn't trigger updateSetting if the user doesn't interact with it. This breaks Mailgun configuration for new setups where users don't explicitly select a region.

Fix in Cursor Fix in Web

logging.error(error);
throw error;
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: Postmark analytics ignores event filter, only tracks opens

The fetchEvents method receives postmarkOptions (which includes the event filter for 'delivered OR opened OR failed OR unsubscribed OR complained') but completely ignores it. The method only calls getMessageOpens() which fetches only open events. Additionally, normalizeEvent hardcodes type: 'opened' for all events. This means delivery tracking, bounce tracking, unsubscribe tracking, and spam complaint tracking will not work with Postmark, causing Ghost to have incomplete email analytics data.

Additional Locations (1)

Fix in Cursor Fix in Web

logging.error(error);
metrics.metric('postmark-send-mail', {
value: Date.now() - startTime,
statusCode: error.code
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

startTime may be undefined in error metrics

In the send method's catch block, Date.now() - startTime is used to calculate the metric value. However, startTime is only assigned at line 95, after building emailMessages. If an error occurs during the message building phase (lines 51-93), startTime will be undefined and the metric value will be NaN.

Fix in Cursor Fix in Web

Object.keys(recipientData[recipient]).forEach((key) => {
messageData.HtmlBody = messageData.HtmlBody.replaceAll(`%recipient.${key}%`, recipientData[recipient][key]);
messageData.TextBody = messageData.TextBody.replaceAll(`%recipient.${key}%`, recipientData[recipient][key]);
messageData.Subject = messageData.Subject.replaceAll(`%recipient.${key}%`, recipientData[recipient][key]);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

replaceAll on potentially null HtmlBody or TextBody crashes

The send method calls .replaceAll() on messageData.HtmlBody, messageData.TextBody, and messageData.Subject without null checks. These values come from messageContent.html and messageContent.plaintext. If the message content is null or undefined (e.g., if an email has no HTML or plaintext version), calling .replaceAll() will throw a TypeError, crashing email sending.

Fix in Cursor Fix in Web

const section = page.getByTestId('bulk-email');

await expect(section.getByText('Mailgun is not set up')).toHaveCount(1);
await expect(section.getByText('Email provider is not set up')).toHaveCount(1);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Test expects wrong success text after UI change

The test asserts that the text 'Mailgun is set up' is visible after saving, but the new BulkEmail.tsx component displays 'The email provider is set up' instead. This test will fail because the expected text doesn't match the actual UI output.

Fix in Cursor Fix in Web

Added 'string-width', 'strip-ansi', and 'wrap-ansi' packages to dependencies in package.json. These may be required for improved string formatting or terminal output handling.
if (e.error && e.messageData) {
const {error, messageData} = e;

// REF: possible mailgun errors https://documentation.mailgun.com/en/latest/api-intro.html#status-codes
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

BulkEmailProvider crashes when PostmarkClient returns null

When PostmarkClient.send() is called but Postmark is not configured (no API token), it returns null at line 38-39 of PostmarkClient.js. The BulkEmailProvider.send() method then attempts to access response.id.trim() on this null value, causing a TypeError: Cannot read properties of null. This can occur if a user selects Postmark as provider but doesn't configure the API token.

Fix in Cursor Fix in Web

danielraffel added a commit to danielraffel/Ghost that referenced this pull request Feb 19, 2026
ref TryGhost#22771

This establishes the foundation for multi-provider email support using
Ghost's existing AdapterManager pattern. The EmailProviderBase class
defines the contract that all email provider adapters must follow,
enabling community-developed providers to be published as npm packages.

- Created EmailProviderBase with required send() method
- Registered email adapter type in AdapterManager
- Added comprehensive tests validating base class contract
danielraffel added a commit to danielraffel/Ghost that referenced this pull request Feb 19, 2026
fixes TryGhost#22771

PR6 (SES Analytics):
- Fixed partial SQS message deletion: When maxEvents limit is reached mid-message, don't delete the message so next run can process remaining recipients
- Added fullyProcessed flag to track whether all events from a message were processed

PR7 (Email Personalization):
- Fixed hasPersonalization detection: Now ignores required system tokens (list_unsubscribe) that are always present
- Bulk BCC path now actually runs for non-personalized newsletters
- Only sends personalized emails when actual member data is used (name, email, etc)

Technical Details:
1. SQS Message Handling:
   - Only marks messages for deletion if all events processed
   - Partially processed messages remain in queue for next analytics run
   - Prevents losing recipients in bulk sends when hitting maxEvents limit

2. Personalization Detection:
   - Checks for replacements beyond REQUIRED_TOKEN_IDS array
   - list_unsubscribe is always present (required: true in EmailRenderer)
   - Bulk path triggers when only system tokens present
   - Personalized path triggers when name, email, first_name, etc used

Example:
- Newsletter with no %%{name}%%: Uses bulk BCC (1 API call per 50 recipients)
- Newsletter with %%{name}%%: Uses personalized (1 API call per recipient)
Resolved conflicts from upstream refactoring:
- Renamed MailgunEmailProvider to BulkEmailProvider (provider-agnostic)
- Updated email service wrapper to use getMailClient() method
- Added PostmarkProvider to email analytics service
- Renamed Mailgun component to BulkEmail in admin settings
- Merged package.json dependencies (@tryghost/postmark-client + @tryghost/nql)
- Regenerated yarn.lock with both dependencies

All Postmark integration code has been preserved while adopting
upstream's provider-agnostic architecture changes.
=======
try {
const response = await bulkEmailProvider.send({
>>>>>>> main:ghost/core/test/unit/server/services/email-service/bulk-email-provider.test.js
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unresolved Git Merge Conflicts in Test File

High Severity

The test file contains three unresolved git merge conflict markers (<<<<<<<, =======, >>>>>>>). These are raw conflict markers committed into the source, which makes the file invalid JavaScript — the test runner will fail to parse and execute it, breaking the entire test suite for the BulkEmailProvider.

Additional Locations (2)

Fix in Cursor Fix in Web

return this.#fetchAnalytics(postmarkOptions, batchHandler, {
maxEvents: options.maxEvents
});
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Postmark Send Errors Silently Swallowed

High Severity

EmailAnalyticsProviderPostmark.fetchLatest carefully builds a postmarkOptions object with begin, end, limit, event filter, and tags, then passes it to PostmarkClient.fetchEvents. However, fetchEvents ignores the postmarkOptions parameter entirely — it hardcodes a 5-minute window (Date.now() - 5 * 60 * 1000) and only calls getMessageOpens, making it impossible to fetch delivered, failed, unsubscribed, or complained events or respect the caller's time range.

Fix in Cursor Fix in Web

// Mail client instance for email provider
let mailClient = this.getMailClient(settingsCache, configService, labs);

const i18nLanguage = labs.isSet('i18n') ? settingsCache.get('locale') || 'en' : 'en';
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Email Locale Gated Behind Unexpected Labs Feature Flag

High Severity

The i18n language initialisation was changed from settingsCache.get('locale') || 'en' to labs.isSet('i18n') ? settingsCache.get('locale') || 'en' : 'en'. This means the site's configured locale is now only used for email rendering when the i18n labs feature flag is enabled. Sites with a non-English locale that haven't opted into the i18n lab will silently receive all newsletter emails in English, regardless of their locale setting. This is a global regression unrelated to the Postmark integration and the only occurrence of this pattern in the codebase.

Fix in Cursor Fix in Web

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (1)
AGENTS.md (1)

257-268: Option 2 (config file) silently requires a manual DB step — make this more prominent.

The current layout presents Option 2 as a self-contained alternative, but the crucial caveat—that bulk_email_provider = 'postmark' must also be set in the DB—is only mentioned as a post-hoc footnote on line 268. An operator who follows only the JSON snippet will configure the token correctly but leave bulk_email_provider on its 'mailgun' default, silently sending all mail via Mailgun while believing Postmark is active. Consider elevating this to a > ⚠️ Warning callout directly inside the Option 2 block.

📝 Proposed documentation clarification
 **Option 2 — Config file (takes precedence over DB settings):**
+> ⚠️ **Important:** The config file alone is not sufficient. You must also set
+> `bulk_email_provider = 'postmark'` via the Admin UI or API (see Option 1 step 2),
+> because `email-service-wrapper.js` routes sends based on that DB setting.
 ```json
 {
   "bulkEmail": {
     "postmark": {
       "apiToken": "your-postmark-server-api-token",
       "streamId": "broadcast"
     }
   }
 }

-When using a config file, also set bulk_email_provider = 'postmark' via the Admin UI or API, because email-service-wrapper.js routes sends based on that DB setting.

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @AGENTS.md around lines 257 - 268, Update the Option 2 documentation block to
prominently warn that configuring the Postmark JSON alone is not sufficient and
the DB setting bulk_email_provider must also be set to 'postmark'; add a clear
warning callout inside the Option 2 section (near the JSON snippet) that
instructs operators to set bulk_email_provider = 'postmark' via the Admin UI or
API because email-service-wrapper.js routes sends based on that DB value.
Reference the JSON example and the DB setting name bulk_email_provider in the
warning so readers cannot miss this manual step.


</details>

</blockquote></details>

</blockquote></details>

<details>
<summary>🤖 Prompt for all review comments with AI agents</summary>

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @AGENTS.md:

  • Around line 270-276: The table under "Settings Added to DB
    (default-settings.json)" incorrectly lists postmark_stream_id as a DB setting;
    move the postmark_stream_id row out of that table and into a new clearly
    labeled section (e.g., "Config-file-only keys" or a subsection beneath a
    horizontal rule) that states it is read from settings but not present in
    default-settings.json; update the documentation text to reference
    postmark_stream_id explicitly in that new section and remove it from the DB
    settings table so readers aren't misled.
  • Around line 317-323: The Known Issues list contains three blockers: remove the
    git conflict markers from the test file referenced by
    bulk-email-provider.test.js so the test parser can run (locate and resolve the
    <<<<<<< / ======= / >>>>>>> blocks and restore a valid test), update
    BulkEmailProvider.js to stop silently dropping Postmark errors by catching
    Postmark client errors and either rethrowing or returning a surfaced failure
    with proper processLogger.error and optional retry logic (modify the
    send/dispatch method that has the "@todo: handle postmark errors" marker), and
    fix EmailAnalyticsProviderPostmark.fetchLatest() to respect the passed begin/end
    parameters (replace the hardcoded "last 5 minutes" window with logic that
    computes the query window from the begin/end args and falls back to a sensible
    default only when those args are absent); additionally, file issues in the
    tracker for the Postmark error handling and analytics-window bug if not fixing
    both immediately.
  • Line 282: Replace the plain fenced code block marker with a language specifier
    to satisfy markdownlint MD040; update the fence that wraps the pseudo-code line
    "email-service-wrapper.js :: getMailClient()" to use text instead of just
    so the block is treated as plain text.

Nitpick comments:
In @AGENTS.md:

  • Around line 257-268: Update the Option 2 documentation block to prominently
    warn that configuring the Postmark JSON alone is not sufficient and the DB
    setting bulk_email_provider must also be set to 'postmark'; add a clear warning
    callout inside the Option 2 section (near the JSON snippet) that instructs
    operators to set bulk_email_provider = 'postmark' via the Admin UI or API
    because email-service-wrapper.js routes sends based on that DB value. Reference
    the JSON example and the DB setting name bulk_email_provider in the warning so
    readers cannot miss this manual step.

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

Comment on lines +270 to +276
### Settings Added to DB (`default-settings.json`)

| Key | Group | Default | Notes |
|-----|-------|---------|-------|
| `bulk_email_provider` | `email` | `'mailgun'` | `'mailgun'` or `'postmark'` |
| `postmark_api_token` | `email` | `null` | Postmark Server API Token |
| `postmark_stream_id` | — | `'broadcast'` | Read from settings but **not** in `default-settings.json`; config-file only |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

postmark_stream_id row placement is contradictory — move it out of the DB settings table.

The table is titled "Settings Added to DB (default-settings.json)" yet the postmark_stream_id row is annotated "config-file only" and explicitly "not in default-settings.json". Including it in a DB-settings table is misleading. Consider either moving it to a separate "Config-file-only keys" subsection or adding a horizontal rule and a clear header distinguishing it.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@AGENTS.md` around lines 270 - 276, The table under "Settings Added to DB
(`default-settings.json`)" incorrectly lists postmark_stream_id as a DB setting;
move the `postmark_stream_id` row out of that table and into a new clearly
labeled section (e.g., "Config-file-only keys" or a subsection beneath a
horizontal rule) that states it is read from settings but not present in
`default-settings.json`; update the documentation text to reference
`postmark_stream_id` explicitly in that new section and remove it from the DB
settings table so readers aren't misled.


### Architecture: How Postmark Plugs In

```
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Add a language specifier to the architecture fenced code block.

The plain ``` fence at line 282 triggers markdownlint MD040. Since this block is a custom pseudo-code diagram, using text keeps linters quiet without implying a runnable language.

📝 Proposed fix
-```
+```text
 email-service-wrapper.js :: getMailClient()
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)

[warning] 282-282: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@AGENTS.md` at line 282, Replace the plain fenced code block marker with a
language specifier to satisfy markdownlint MD040; update the fence that wraps
the pseudo-code line "email-service-wrapper.js :: getMailClient()" to use
```text instead of just ``` so the block is treated as plain text.

Comment on lines +317 to +323
### Known Issues / TODOs

- `BulkEmailProvider.js` has an explicit `@TODO: handle postmark errors` — Postmark errors are currently silently dropped.
- `postmark_stream_id` cannot be set via the Admin UI; config-file only.
- `ghost/core/test/unit/server/services/email-service/bulk-email-provider.test.js` has **unresolved git merge conflict markers** and will not run.
- `ghost/postmark-client/test/postmark-client.test.js` and `ghost/email-analytics-provider-postmark/test/provider-postmark.test.js` are **empty stubs**.
- `EmailAnalyticsProviderPostmark` always fetches only the last 5 minutes of open events regardless of the `begin`/`end` options passed to it.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Three of the five documented "known issues" are hard blockers — they should be resolved before any production merge.

The Known Issues section honestly lists these, but they represent material defects:

  1. Line 321 — Unresolved merge-conflict markers in bulk-email-provider.test.js: Git conflict markers (e.g. <<<<<<<, =======, >>>>>>>) cause Node's test runner to throw a parse error, so the entire test suite for the most critical new code path simply does not execute. This is a CI/build blocker.

  2. Line 319 — Postmark errors silently dropped: The explicit @TODO: handle postmark errors in BulkEmailProvider.js means any Postmark send failure is swallowed with no exception propagation, no retry, and no user-visible error — subscribers are silently skipped with no operational signal.

  3. Line 323 — Hardcoded 5-minute analytics window: EmailAnalyticsProviderPostmark.fetchLatest() ignores the begin/end parameters it receives. For analytics catch-up runs (e.g., after downtime or a cold start), this means open/click events older than 5 minutes are permanently lost.

Items 2 and 3 should ideally be tracked in the issue tracker rather than only in AGENTS.md, and item 1 must be resolved before the test suite can be considered green.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@AGENTS.md` around lines 317 - 323, The Known Issues list contains three
blockers: remove the git conflict markers from the test file referenced by
bulk-email-provider.test.js so the test parser can run (locate and resolve the
<<<<<<< / ======= / >>>>>>> blocks and restore a valid test), update
BulkEmailProvider.js to stop silently dropping Postmark errors by catching
Postmark client errors and either rethrowing or returning a surfaced failure
with proper processLogger.error and optional retry logic (modify the
send/dispatch method that has the "@TODO: handle postmark errors" marker), and
fix EmailAnalyticsProviderPostmark.fetchLatest() to respect the passed begin/end
parameters (replace the hardcoded "last 5 minutes" window with logic that
computes the query window from the begin/end args and falls back to a sensible
default only when those args are absent); additionally, file issues in the
tracker for the Postmark error handling and analytics-window bug if not fixing
both immediately.

// send the email using Mailgun
// uses empty replacements array as we've already replaced all tokens with Mailgun variables
const response = await this.#mailgunClient.send(
const response = await this.#mailClient.send(
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Email ID not passed to PostmarkClient, breaking analytics tracking

High Severity

BulkEmailProvider.send() destructures emailId from data but never includes it in messageData that gets passed to #mailClient.send(). PostmarkClient.send() reads message.id (which is undefined) to build the Tag field, resulting in 'ghost-email|undefined'. This means normalizeEvent later extracts the string 'undefined' as emailId, which won't match any email record — completely breaking the link between Postmark analytics events and Ghost emails.

Additional Locations (1)

Fix in Cursor Fix in Web

const endDate = new Date();

try {
let page = await this.getEventsFromPostmark(postmarkInstance, startDate);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Metrics timer uses query window start instead of request start

Low Severity

fetchEvents passes startDate (a Date set to 5 minutes in the past) as the startTime argument to getEventsFromPostmark. The metric calculation Date.now() - startTime then always reports ~300000ms instead of the actual API call duration, making the postmark-get-events metric useless for performance monitoring.

Additional Locations (1)

Fix in Cursor Fix in Web

if (!postmarkInstance) {
logging.warn(`Postmark is not configured`);
return null;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

PostmarkClient returning null crashes BulkEmailProvider with TypeError

High Severity

PostmarkClient.send() returns null when Postmark is not configured (no API token). BulkEmailProvider.send() then attempts response.id.trim() on the null return value, causing an unhandled TypeError. This happens when the bulk_email_provider setting is 'postmark' but no API token has been provided — the mail client is instantiated unconditionally by getMailClient, and configuration is only checked lazily inside send().

Additional Locations (1)

Fix in Cursor Fix in Web

const section = page.getByTestId('bulk-email');

await expect(section.getByText('Mailgun is not set up')).toHaveCount(1);
await expect(section.getByText('Email provider is not set up')).toHaveCount(1);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Test fails because Mailgun fields hidden without provider selection

Medium Severity

The test clicks Edit and immediately tries to fill 'Mailgun domain' and 'Mailgun private API key' fields. However, the new BulkEmail component only renders Mailgun settings when emailProvider === 'mailgun', and the test fixture sets bulk_email_provider to null. The test is missing a step to select "Mailgun" from the provider dropdown first, so the Mailgun fields aren't rendered and the test will fail.

Fix in Cursor Fix in Web

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 5 potential issues.

Bugbot Free Tier Details

Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.

To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

Bugbot Autofix is kicking off a free cloud agent to fix these issues. This run is complimentary, but you can enable autofix for all future PRs in the Cursor dashboard.

});
}
} else if (this.#mailClient.constructor.name === 'PostmarkClient') {
// @TODO: handle postmark errors
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Postmark errors throw undefined crashing error handling

High Severity

When PostmarkClient throws an error during send(), the ghostError variable is declared but never assigned in the Postmark branch (the @TODO comment is the only content). Execution then reaches throw ghostError which throws undefined, producing a confusing, undebuggable crash instead of a proper EmailError with context. Every Postmark send failure will result in this broken error path.

Fix in Cursor Fix in Web

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bugbot Autofix determined this is a false positive.

This branch has no Postmark bulk email provider implementation, so the referenced BulkEmailProvider.js error path does not exist here.

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

onEditingChange={handleEditingChange}
onSave={async () => {
handleSave();
}}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing Mailgun region default on save causes null base URL

Medium Severity

The old mailgun.tsx had special logic in onSave to set mailgun_base_url to the US region default when it was unset. The new BulkEmail.tsx omits this, just calling handleSave() directly. When a user configures Mailgun for the first time without explicitly changing the region dropdown, mailgun_base_url remains null, causing MailgunClient.#getConfig() to treat Mailgun as unconfigured.

Fix in Cursor Fix in Web

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bugbot Autofix determined this is a false positive.

The current settings UI already sets a default Mailgun region on save in mailgun.tsx when mailgun_base_url is unset.

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

});
const i18nLanguage = settingsCache.get('locale') || 'en';
// Mail client instance for email provider
let mailClient = this.getMailClient(settingsCache, configService, labs);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Mail client fixed at init, runtime provider switching broken

Medium Severity

getMailClient() is called once during init() and the resulting client is permanently bound to BulkEmailProvider. The admin UI lets users switch bulk_email_provider between Mailgun and Postmark, but this change is never picked up — the original client object continues to be used until Ghost restarts. The same issue exists in service.js where the suppression-list client is fixed at module load time.

Additional Locations (1)

Fix in Cursor Fix in Web

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bugbot Autofix determined this is a false positive.

This codebase does not support runtime bulk_email_provider switching and only uses Mailgun, whose client reads settings dynamically per call.

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.


if (config.get('bulkEmail:mailgun:tag')) {
this.tags.push(config.get('bulkEmail:mailgun:tag'));
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Postmark analytics provider reads Mailgun config key

Medium Severity

The EmailAnalyticsProviderPostmark constructor reads config.get('bulkEmail:mailgun:tag') — this is a copy-paste from the Mailgun analytics provider. It needs to read bulkEmail:postmark:tag instead. A search confirms bulkEmail:postmark:tag is never referenced anywhere in the codebase, while this file duplicates the Mailgun key.

Fix in Cursor Fix in Web

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bugbot Autofix determined this is a false positive.

There is no Postmark analytics provider file in this branch, so the reported copy-paste config key issue is not present.

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

type: 'opened',
severity: 'permanent',
recipientEmail: event.Recipient,
emailId: event.Tag.split('|')[1] ?? null,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

normalizeEvent crashes when event Tag is null

Medium Severity

event.Tag.split('|')[1] in normalizeEvent throws a TypeError if event.Tag is null or undefined. Since getMessageOpens fetches all open events from the Postmark account (not filtered by tag), any event from a non-Ghost email or one without a tag will crash the entire analytics processing loop.

Fix in Cursor Fix in Web

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bugbot Autofix determined this is a false positive.

This repository snapshot contains no Postmark client implementation, so the referenced normalizeEvent logic does not exist.

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

@JohnONolan
Copy link
Copy Markdown
Member

JohnONolan commented Mar 27, 2026

Thanks for sharing! This is really cool

Closing for now because we're not going to merge this. The core team doesn't have the capacity to support multiple bulk mail providers at present - but ofc you're welcome to keep using it in your fork if it's working well for you

@JohnONolan JohnONolan closed this Mar 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community [triage] Community features and bugs migration [pull request] Includes migration for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants