Conversation
🦋 Changeset detectedLatest commit: 8543c82 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
review/needs-review
Scope checkThis PR changes 842 lines across 5 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-moderation
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
b21e3c5 to
e142700
Compare
github-actions
Bot
added
area/admin
and removed
review/needs-review
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
The approach is sound: a built-in SMTP transport is the right way to give operators generic SMTP credentials on Cloudflare Workers, and it fits the existing exclusive email:deliver hook model alongside the dev console and Cloudflare Email providers. The PR references the approved discussion and adds a proper changeset.
Static review found several real issues, though:
- Admin UI labels are not localized. Hard-coded English provider names and security-mode labels in
EmailSettings.tsxviolate the Lingui/i18n convention. - SMTP configured via the admin UI does not become active until the runtime restarts. The SMTP built-in is registered only at cold-start when a complete config is already present. The settings route saves the DB record and sets the exclusive selection, but the current pipeline has no SMTP handler, so
isAvailable()returnstruewhilesend()throwsEmailNotConfiguredError. - The admin API allows saving port 25. The PR claims port 25 is refused with a clear error, but that refusal only lives in
loadSmtpConfigFromEnv(). The Zod schema used by the settings route accepts any port 1–65535. - SMTP MIME is not safe for non-ASCII content. Headers (
Subject,From,Reply-To) are emitted as raw UTF-8 and the body is declaredContent-Transfer-Encoding: 7bit, which will mojibake or be rejected for localized/RTL content. - Stale/narrative comments. The SMTP plugin header still says "env-only … no admin UI yet", and tests reference issue numbers in comments.
I did not run the test suite, linter, or builds; the findings below are based on reading the diff, tracing call sites, and checking the repo conventions in AGENTS.md.
| const PROVIDER_OPTIONS: { value: EmailProviderChoice; label: string }[] = [ | ||
| { value: "none", label: "None" }, | ||
| { value: "smtp", label: "SMTP" }, | ||
| { value: "cloudflare", label: "Cloudflare Email" }, |
There was a problem hiding this comment.
[needs fixing] These select labels are user-facing English strings rendered in the DOM, but they bypass Lingui. Per AGENTS.md every admin UI string must go through @lingui/react/macro / @lingui/core/macro.
Use msg descriptors at module scope and resolve them with t() when rendering the Select items:
| const PROVIDER_OPTIONS: { value: EmailProviderChoice; label: string }[] = [ | |
| { value: "none", label: "None" }, | |
| { value: "smtp", label: "SMTP" }, | |
| { value: "cloudflare", label: "Cloudflare Email" }, | |
| import { msg } from "@lingui/core/macro"; | |
| import type { MessageDescriptor } from "@lingui/core"; | |
| const PROVIDER_OPTIONS: { value: EmailProviderChoice; label: MessageDescriptor }[] = [ | |
| { value: "none", label: msg`None` }, | |
| { value: "smtp", label: msg`SMTP` }, | |
| { value: "cloudflare", label: msg`Cloudflare Email` }, | |
| ]; |
Then map with label: t(opt.label) in the <Select items={...}> call on line 263.
| { value: "starttls", label: "STARTTLS (port 587)" }, | ||
| { value: "tls", label: "Implicit TLS (port 465)" }, |
There was a problem hiding this comment.
[needs fixing] The STARTTLS / implicit TLS option labels are hard-coded English. They are user-visible strings in the security <Select> and must be localized.
| { value: "starttls", label: "STARTTLS (port 587)" }, | |
| { value: "tls", label: "Implicit TLS (port 465)" }, | |
| items={[ | |
| { value: "starttls", label: t`STARTTLS (port 587)` }, | |
| { value: "tls", label: t`Implicit TLS (port 465)` }, | |
| ]} |
| // Persist and activate SMTP as the selected provider | ||
| await optionsRepo.set(optionKey, SMTP_EMAIL_PLUGIN_ID); | ||
| emdash.hooks.setExclusiveSelection(EMAIL_DELIVER_HOOK, SMTP_EMAIL_PLUGIN_ID); | ||
|
|
There was a problem hiding this comment.
[needs fixing] This saves the SMTP config to the DB and immediately sets it as the selected exclusive provider, but nothing rebuilds the hook pipeline. EmDashRuntime only registers the built-in SMTP plugin at cold-start when loadSmtpConfig() already returns a complete config (emdash-runtime.ts:1359). If the worker started without SMTP env vars or an existing DB config, the SMTP handler is not in the current pipeline, so emdash.email.isAvailable() will report true while send() later throws EmailNotConfiguredError.
Fix by making the SMTP provider available immediately after admin configuration. The cleanest path is to register the built-in SMTP plugin unconditionally (like Cloudflare Email) and have its handler reject an incomplete config with a clear message, or expose a hook-pipeline rebuild that this route can invoke after persisting a new provider.
| const smtpConfigSchema = z.object({ | ||
| host: z.string().min(1), | ||
| port: z.number().int().min(1).max(65535), | ||
| secure: z.enum(["starttls", "tls"]), |
There was a problem hiding this comment.
[needs fixing] The PR description says port 25 is refused with a clear error because Cloudflare blocks it, but this Zod schema accepts any port 1–65535. A user can save port 25 through the admin UI, after which delivery will hang/fail at runtime.
Add a refine so the API rejects port 25 at the same place it rejects other invalid values:
| secure: z.enum(["starttls", "tls"]), | |
| const smtpConfigSchema = z.object({ | |
| host: z.string().min(1), | |
| port: z.number().int().min(1).max(65535), | |
| secure: z.enum(["starttls", "tls"]), | |
| user: z.string().min(1), | |
| pass: z.string().min(1).optional(), // undefined = keep existing password | |
| fromName: z.string().optional(), | |
| fromEmail: z.string().email().optional(), | |
| replyTo: z.string().email().optional(), | |
| }).refine((v) => v.port !== 25, { | |
| message: "Port 25 is not supported on Cloudflare Workers; use 587 (STARTTLS) or 465 (implicit TLS).", | |
| }); |
| ].join("\r\n"); | ||
| } else { | ||
| headers.push(`Content-Type: text/plain; charset=utf-8`); | ||
| headers.push(`Content-Transfer-Encoding: 7bit`); |
There was a problem hiding this comment.
[needs fixing] The MIME body is declared Content-Transfer-Encoding: 7bit and Subject / From / Reply-To headers are emitted as raw UTF-8. That violates RFC 5322/2047 and will mojibake or be rejected for any non-ASCII content, including localized site names, RTL text, or sender names.
At minimum, encode non-ASCII headers per RFC 2047 and switch the body transfer encoding to quoted-printable or base64 (or negotiate BODY=8BITMIME and use 8bit). As written, a user-visible <strong>${siteName}</strong> from the test email or any localized subject will be corrupted.
| * uses `node:net` / `node:tls`. Configuration is env-only for the first | ||
| * iteration — no admin UI yet. |
There was a problem hiding this comment.
[suggestion] This header comment is stale: it says the SMTP config is "env-only" and that "no admin UI yet" exists, but this PR adds exactly that admin UI. Update or remove the outdated line so future readers don't believe the feature is CLI/env-only.
| * uses `node:net` / `node:tls`. Configuration is env-only for the first | |
| * iteration — no admin UI yet. | |
| * via raw TCP — the one network primitive sandboxed plugins cannot use. | |
| * | |
| * Registered as a built-in `email:deliver` provider when SMTP is configured, | |
| * either via env vars or via Settings → Email in the admin UI. |
| }); | ||
| }); | ||
|
|
||
| describe("Cloudflare socket edge cases (regression #1541)", () => { |
There was a problem hiding this comment.
[suggestion] AGENTS.md says comments should never reference issues/PRs; that context belongs in commit messages. The describe block name and the inline // Regression #1541 comment both violate this. Rename the suite to something descriptive (e.g., Cloudflare socket edge cases) and delete the issue-number comment on line 462.
| } | ||
| }); | ||
|
|
||
| it("uses 25s timeout so hook timeout (30s) does not swallow the error", async () => { |
There was a problem hiding this comment.
[suggestion] The test name claims to verify that the default 25s timeout is shorter than the 30s hook timeout, but it actually passes timeoutMs: 50 and only asserts the configurable timeout is used. It cannot fail on the real regression it claims to guard against (someone changing the default 25s value). Either drop the test or mock timers and assert the actual default.
github-actions
Bot
added
review/awaiting-author
There was a problem hiding this comment.
The approach is still sound: a built-in SMTP transport is the right way to unblock generic-SMTP credentials in Cloudflare Workers, and the Settings → Email UI fits EmDash’s exclusive-hook email model.
Compared with the first review, this revision fixes the major blockers:
- The built-in SMTP and Cloudflare Email providers are always registered, so selecting them in Settings activates the handler at runtime without a restart.
- The hook handler now loads config from the database on every send (
createSmtpEmailDeliverFromDb), and the settings route persists the exclusive selection. - Port 25 is now refused both in
loadSmtpConfigFromEnv()and in the admin API Zod schema. - Success toasts and most labels are now localized.
- MIME bodies are base64-encoded (
Content-Transfer-Encoding: base64) rather than 7-bit, so non-ASCII text and RTL content survive in the body.
I read the changed files, traced calls into EmailPipeline, HookPipeline, PluginManager, and the runtime registration, and re-checked AGENTS.md. I did not run the test suite, linter, or build; the findings below are from static reading only.
Remaining blockers are small but real: several hard-coded English placeholders in the admin form, a non-compliant non-ASCII header encoding that can exceed SMTP line-length limits, a duplicated docs sentence, and a JSDoc link to a function that does not exist.
| label={t`SMTP Host`} | ||
| value={smtpHost} | ||
| onChange={(event) => setSmtpHost(event.target.value)} | ||
| placeholder="smtp-relay.brevo.com" |
There was a problem hiding this comment.
[needs fixing] Several placeholder attributes on this form are still hard-coded English example strings, which violates AGENTS.md (“No hard-coded English in JSX, attributes, or strings that end up in the DOM”). The same pattern repeats on lines 312, 331, 356, 363, 390, and 398.
| placeholder="smtp-relay.brevo.com" | |
| placeholder={t`smtp-relay.brevo.com`} |
Apply the same t\…`` wrapper to the remaining literal placeholders (port, username, sender email, reply-to, and the Cloudflare email/reply-to examples).
| const sanitized = sanitizeHeader(value); | ||
| // Only encode when non-ASCII is present — ASCII headers stay readable. | ||
| if (!NON_ASCII_REGEX.test(sanitized)) return sanitized; | ||
| return `=?UTF-8?B?${btoa(unescape(encodeURIComponent(sanitized)))}?=`; |
There was a problem hiding this comment.
[needs fixing] encodeHeader() emits non-ASCII header values as a single encoded-word with no length limit. A long RTL subject or sender name can exceed the SMTP 998-octet line limit and RFC 2047’s 75-octet encoded-word limit, so mail may be rejected or truncated. Split the base64 output into multiple encoded-words.
| return `=?UTF-8?B?${btoa(unescape(encodeURIComponent(sanitized)))}?=`; | |
| function encodeHeader(value: string): string { | |
| const sanitized = sanitizeHeader(value); | |
| if (!NON_ASCII_REGEX.test(sanitized)) return sanitized; | |
| const encoded = btoa(unescape(encodeURIComponent(sanitized))); | |
| const words: string[] = []; | |
| for (let i = 0; i < encoded.length; i += 48) { | |
| words.push(`=?UTF-8?B?${encoded.slice(i, i + 48)}?=`); | |
| } | |
| return words.join(" "); | |
| } |
| /** | ||
| * Build the email:deliver handler. | ||
| * | ||
| * Exported for testing — production code should use {@link createSmtpPlugin}. |
There was a problem hiding this comment.
[needs fixing] The JSDoc tells readers that production code should use {@link createSmtpPlugin}, but no createSmtpPlugin function exists in the repo. The production registration in emdash-runtime.ts calls definePlugin() directly. This stale reference will break generated docs and mislead future readers.
| * Exported for testing — production code should use {@link createSmtpPlugin}. | |
| * Exported for testing — production code registers the handler through | |
| * `definePlugin()` in `emdash-runtime.ts`. |
| in core and uses `cloudflare:sockets` on Workers or `node:net`/`node:tls` on | ||
| Node. | ||
|
|
||
| Configure via environment variables: |
There was a problem hiding this comment.
[suggestion] The “Built-in SMTP transport” section starts with a redundant sentence (“Configure via environment variables:”) immediately followed by the real intro sentence. Remove the stray sentence so the section flows from the admin UI/env intro into the .env example.
| Configure via environment variables: | |
| - | |
| -Configure via environment variables: | |
| - | |
| Configure it under **Settings → Email** in the admin |
…ld long encoded headers A literal cloudflare:sockets import in core broke every Node-adapter astro build. The transport now imports connect from virtual:emdash/sockets, which the integration generates per adapter like virtual:emdash/env. Non-ASCII headers are split into RFC 2047 encoded-words of at most 75 octets without splitting multi-byte characters. Remaining admin placeholders go through Lingui; stale JSDoc link and duplicated docs sentence removed.
|
Pushed 8543c82:
|
There was a problem hiding this comment.
This is a re-review of the built-in SMTP transport PR. The approach remains sound: adding core-side SMTP and a Cloudflare Email built-in is the right way to unblock generic-SMTP credentials in sandboxed workers, and the Settings → Email UI fits the existing exclusive-hook email model.
Compared with the previous review, the major blockers have been addressed:
- Admin form placeholders, labels, and helper text are now Lingui-wrapped (the previously reported hard-coded English strings are gone).
- Non-ASCII header encoding now splits into per-RFC encoded-words capped at 75 octets and the tests verify each word stays within the limit.
- The duplicated docs sentence is no longer present in the Cloudflare deployment guide.
- The dangling JSDoc
{@link}reference has been removed. - Built-in providers are always registered, config loads lazily per send, the
__none__sentinel preserves an explicit "None" choice, and port 25 is rejected at both the env/config layer and the admin API schema.
I read the changed files, traced calls through EmailPipeline, HookPipeline, PluginManager, and EmDashRuntime, and re-checked the docs, changeset, and tests. I did not run the test suite, linter, or build; the findings below are from static reading only.
Remaining issues are minor: one inaccurate JSDoc claim about how production registers the handler, and a UI polish item where the status section displays raw provider IDs instead of the friendly labels used in the provider dropdown. I also have a low-confidence observation about a couple of narrative comments in the SMTP module that could be trimmed, but I’m not treating them as blockers.
| timeoutMs?: number; | ||
| } | ||
|
|
||
| /** Load SMTP config from env; returns null if not configured. */ |
There was a problem hiding this comment.
[suggestion] The JSDoc says production registers the handler through createSmtpEmailDeliver(), but emdash-runtime.ts actually registers createSmtpEmailDeliverFromDb() so that admin-saved DB config is picked up without a restart. Update the comment so it points to the function actually used in production.
| /** Load SMTP config from env; returns null if not configured. */ | |
| /** | |
| * Build the email:deliver handler from a static config. | |
| * | |
| * Exported for testing — production code registers the handler through | |
| * `createSmtpEmailDeliverFromDb()` in `emdash-runtime.ts`. | |
| */ |
| @@ -2,28 +2,63 @@ | |||
| * Email settings page | |||
There was a problem hiding this comment.
[suggestion] The active-provider status in PipelineStatus renders the raw selectedProviderId (e.g. emdash-smtp, emdash-cloudflare-email), while the provider dropdown maps these same IDs to user-friendly labels (SMTP, Cloudflare Email). For consistency, display the friendly label here too, falling back to the plugin ID only for plugin providers that have no built-in mapping.
|
|
||
| if (!host || !port || !secure || !user || !encryptedPass) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
[suggestion] This comment and the following two explain the race/timeout design in narrative form. The code is clear enough now that these justifications ("so a blackholed host...", "connect() runs outside deliver() so TS...", "The transcript goes to the server logs only...") are mostly duplicative of the code. Consider trimming them to keep the file focused on behavior future maintainers cannot read from the code itself.
github-actions
Bot
added
needs-rebase
review/awaiting-author
1 participant
What does this PR do?
Adds a built-in
email:delivertransport that works with any standard SMTP server (Brevo relay, Office365, Google Workspace, Fastmail, Amazon SES, self-hosted Postfix) via raw TCP — the one network primitive sandboxed plugins cannot use. This unblocks operators who only have generic SMTP credentials and cannot use HTTP transactional APIs (Brevo HTTP API, SendGrid, etc.).How it works:
EMAIL_SMTP_HOST,EMAIL_SMTP_PORT,EMAIL_SMTP_USER,EMAIL_SMTP_PASS, optionalEMAIL_SMTP_FROM) or via the new Settings → Email admin UI (password stored encrypted).cloudflare:socketsand on Node vianode:net/node:tls, with automatic fallback.Cloudflare Workers specifics fixed:
WritableStreamlock: the plaintext socket's writer must be closed beforestartTls()upgrades the socket, otherwise the first write to the TLS socket throws "WritableStream is currently locked".sock.openedmust be awaited before writing — unlike Node, where the connect callback signals readiness.secureTransport: "starttls"is required during the initial connection to later allowsock.startTls().throwinsidesetTimeoutnever reaches the awaiting promise and becomes an unhandled exception.Admin UI:
__none__sentinel for exclusive hooks.send_emailbinding.Closes #1541
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output
Unit tests:
packages/core/tests/unit/plugins/email-smtp.test.ts— 21 tests covering env config parsing, port-25 refusal, full STARTTLS session flow, AUTH failure, dot-stuffing, the plugin handler contract, and Cloudflare-specific regressions (timeout race, STARTTLS upgrade flow).Live verification on Cloudflare Workers (Brevo SMTP, port 465 implicit TLS): test email delivered successfully.