From 42892078e5dfb582d8da2ba9a8024fa0451092ec Mon Sep 17 00:00:00 2001 From: Jack Zhuang <50353452+hotlong@users.noreply.github.com> Date: Wed, 2 Sep 2026 10:14:40 +0800 Subject: [PATCH 1/2] fix(plugin-email): validate the default sender at configuration time and record pre-delivery rejections in sys_email An unsendable OS_EMAIL_FROM was first judged inside normalizeMessage on the first send, which for a fresh deployment is the first user's sign-up -- and better-auth's runInBackgroundOrAwait logs and swallows that throw, so the UI reported a verification email nobody had sent. EmailServicePlugin.init() now refuses a declared defaultFrom no message could be sent from; the mail settings channel refuses the value, keeps the previous sender and says so at error. EmailService also writes a sys_email row at status='failed' for a message rejected before delivery -- the one window that previously produced no record. Co-Authored-By: Claude Fable 5.1 --- ...-sender-validated-at-configuration-time.md | 55 ++++ ...fication-email-failure-propagation.test.ts | 111 +++++++ .../plugins/plugin-email/src/email-plugin.ts | 67 +++- .../plugins/plugin-email/src/email-service.ts | 132 +++++++- .../src/sender-address-validation.test.ts | 285 ++++++++++++++++++ 5 files changed, 646 insertions(+), 4 deletions(-) create mode 100644 .changeset/email-sender-validated-at-configuration-time.md create mode 100644 packages/plugins/plugin-auth/src/verification-email-failure-propagation.test.ts create mode 100644 packages/plugins/plugin-email/src/sender-address-validation.test.ts diff --git a/.changeset/email-sender-validated-at-configuration-time.md b/.changeset/email-sender-validated-at-configuration-time.md new file mode 100644 index 0000000000..d1f179d2ef --- /dev/null +++ b/.changeset/email-sender-validated-at-configuration-time.md @@ -0,0 +1,55 @@ +--- +"@objectstack/plugin-email": patch +--- + +fix(plugin-email): judge the default sender where it is configured, and record a send rejected before delivery (#14318) + +Measured on a local rig with `OS_EMAIL_FROM="ObjectOS Local "`, +`OS_EMAIL_PROVIDER=log` and `OS_AUTH_REQUIRE_EMAIL_VERIFICATION=true`: the server +booted clean, the first sign-up answered `200`, the UI said a verification email +had been sent, and nothing had been. `EMAIL_REGEX` requires a dotted domain and +correctly refuses `noreply@localhost` — but it refused it inside +`normalizeMessage`, on the **first send**, which for a fresh deployment is the +first user's sign-up. better-auth runs `sendVerificationEmail` through +`runInBackgroundOrAwait`, which logs `Failed to run background task` and returns +normally, so the account was created and the user was parked on a verify screen +whose Resend repeated the whole sequence. `sys_email` held no row for any of it: +the throw happened *before* the row insert, and every other failure shape in the +service is a row at `status:'failed'`. + +Two changes, one per half. + +**The address is judged where it is configured.** `EmailServicePlugin.init()` +now refuses a declared `defaultFrom` that no message could ever be sent from, so +a deployment that names an unsendable sender fails its boot instead of failing +every send — the same trade `resolveTransport` already makes for an SMTP +provider with no host, and the error names the consequence and the fix +(`OS_EMAIL_FROM` / `config.email.defaultFrom`). An **absent** sender is still +accepted: callers that always pass `input.from` are a complete configuration, +and `normalizeMessage` already refuses a send that has neither. + +The `mail` settings channel takes that method's opposite, stated trade — a save +must not kill a running server — so an unsendable saved From address is +**refused, the previous sender kept**, and the consequence stated at `error`. +`error` and not `warn` because nothing looks broken afterwards: the save +succeeds and the settings page shows the address the operator typed. + +**A send rejected before delivery now leaves a `sys_email` row.** The +`normalizeMessage` window was the one path on which a send produced no record at +all. It now writes `status:'failed'` with the reason, prefixed +`rejected before delivery:` so the column distinguishes a message that never +reached a transport from one an SMTP host refused. The envelope columns carry +what the caller actually passed (never re-canonicalised — canonicalisation is +what threw), `(none)` where the input named nothing, since `from_address` / +`to_addresses` / `subject` are required. The row is safe by construction: both +re-delivery paths — the `afterInsert` outbox drain hook and the boot outbox +sweep — gate on `status === 'queued'`, so a rejection record can never be +mistaken for an outbox entry. Persisting it is best-effort and never replaces +the caller's error. + +Unchanged: `formatAddress`, `EMAIL_REGEX` and `normalizeMessage` keep their +exact verdicts (the new `isSendableAddress` predicate shares the one regex, so a +boot cannot pass a check the send path then fails), `send()` still throws on +validation failure rather than answering `failed`, and the auth layer's +propagation is as it was — `sendVerificationEmail` already rejects on both a +throw and a returned `status:'failed'`, which is now pinned by a test. diff --git a/packages/plugins/plugin-auth/src/verification-email-failure-propagation.test.ts b/packages/plugins/plugin-auth/src/verification-email-failure-propagation.test.ts new file mode 100644 index 0000000000..002423805e --- /dev/null +++ b/packages/plugins/plugin-auth/src/verification-email-failure-propagation.test.ts @@ -0,0 +1,111 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #14318 — the `sendVerificationEmail` callback must hand a send failure back + * to whoever called it. + * + * Why this is pinned rather than assumed. The reported symptom was a sign-up + * that answered 200 while the verification mail was never sent, and the + * obvious reading is "the auth layer swallows the failure". It does not: both + * failure shapes `IEmailService` can produce — a THROW (template resolution, + * or `normalizeMessage` refusing an unsendable `from`) and a returned + * `status:'failed'` (transport error) — leave this callback as a rejection. + * + * What swallows it is one layer further out, and it is not ours: + * better-auth's sign-up route invokes the callback through + * `runInBackgroundOrAwait`, which awaits the promise inside a `try/catch` that + * logs `Failed to run background task` and returns normally + * (`better-auth/dist/context/create-context.mjs`). The `/send-verification-email` + * route does NOT — it awaits `sendVerificationEmailFn` directly and rethrows — + * so the resend path is honest today and must stay that way. + * + * Hence these assertions: they are the contract the cloud verify-email screen + * reads through the resend endpoint, and the reason the *first* half of #14318 + * is a configuration-time refusal in plugin-email rather than another layer of + * error plumbing here. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { AuthManager } from './auth-manager'; + +vi.mock('better-auth', () => ({ + betterAuth: vi.fn(() => ({ handler: vi.fn(), api: {} })), +})); +vi.mock('better-auth/plugins/organization', () => ({ + organization: vi.fn((opts: any) => ({ id: 'organization', _opts: opts })), +})); +vi.mock('better-auth/plugins/magic-link', () => ({ + magicLink: vi.fn((opts: any) => ({ id: 'magic-link', _opts: opts })), +})); +vi.mock('better-auth/plugins/two-factor', () => ({ + twoFactor: vi.fn((opts: any) => ({ id: 'two-factor', _opts: opts })), +})); +vi.mock('better-auth/plugins/custom-session', () => ({ + customSession: vi.fn((fn: any) => ({ id: 'custom-session', _fn: fn })), +})); +vi.mock('better-auth/plugins/haveibeenpwned', () => ({ + haveIBeenPwned: vi.fn((opts: any) => ({ id: 'have-i-been-pwned', _opts: opts })), +})); + +const USER = { id: 'u1', email: 'ada@example.com', name: 'Ada' }; + +/** Boot an AuthManager whose `sendTemplate` behaves as `sendTemplate` says. */ +async function boot(sendTemplate: (input: any) => Promise) { + const { betterAuth } = await import('better-auth'); + let capturedConfig: any; + (betterAuth as any).mockImplementation((config: any) => { + capturedConfig = config; + return { handler: vi.fn(), api: {} }; + }); + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const manager = new AuthManager({ + secret: 'test-secret-at-least-32-chars-long', + baseUrl: 'http://localhost:3000', + emailAndPassword: { enabled: true }, + emailVerification: { sendOnSignUp: true }, + } as never); + manager.setEmailService({ + async send() { return { id: 'e', status: 'sent' }; }, + sendTemplate, + } as never); + await manager.getAuthInstance(); + warnSpy.mockRestore(); + return capturedConfig; +} + +const drive = (config: any) => config.emailVerification.sendVerificationEmail({ + user: USER, + url: 'http://x/verify', + token: 't', +}); + +describe('sendVerificationEmail — failure reaches the caller', () => { + it('rejects when the send THROWS (the unsendable-from shape)', async () => { + // Exactly what `EmailService.send` does with + // OS_EMAIL_FROM="ObjectOS Local ": `normalizeMessage` + // refuses the sender and the throw travels out of `sendTemplate`. + const config = await boot(async () => { + throw new Error('Invalid email address: noreply@localhost'); + }); + await expect(drive(config)).rejects.toThrow(/Invalid email address: noreply@localhost/); + }); + + it('rejects when the send RETURNS status:failed, naming the recipient and the cause', async () => { + const config = await boot(async () => ({ id: 'e1', status: 'failed', error: 'smtp 421' })); + // Both facts matter to whoever reads the resend response: which address + // was not reached, and why. + await expect(drive(config)).rejects.toThrow(/ada@example\.com/); + await expect(drive(config)).rejects.toThrow(/smtp 421/); + }); + + it('resolves on a successful send — the control', async () => { + const sent: any[] = []; + const config = await boot(async (input: any) => { + sent.push(input); + return { id: 'e1', status: 'sent' }; + }); + await expect(drive(config)).resolves.toBeUndefined(); + expect(sent).toHaveLength(1); + expect(sent[0]).toMatchObject({ template: 'auth.verify_email' }); + }); +}); diff --git a/packages/plugins/plugin-email/src/email-plugin.ts b/packages/plugins/plugin-email/src/email-plugin.ts index 11b7ac288f..d9a384b401 100644 --- a/packages/plugins/plugin-email/src/email-plugin.ts +++ b/packages/plugins/plugin-email/src/email-plugin.ts @@ -14,6 +14,7 @@ import { EmailService, LogTransport, EMAIL_SEND_QUEUE, + isSendableAddress, type EmailPersistence, type EmailQueueDelivery, type TemplateLoader, @@ -245,6 +246,48 @@ export function resolveDurableQueue(getService: (name: string) => unknown): IQue return queue as IQueueService; } +/** How the settings page and the deployment channel name the same address. */ +const DEFAULT_FROM_FIX = + 'Fix: set a sender with a dotted domain — OS_EMAIL_FROM="Name " or ' + + 'config.email.defaultFrom (Settings → Mail → From address on the settings channel). ' + + 'Bare hostnames such as "noreply@localhost" are not deliverable addresses.'; + +/** + * Refuse a **declared** default sender no message could ever be sent from + * (#14318). + * + * This is the constructor / CLI channel, so it THROWS, exactly as an + * unbuildable transport does in {@link EmailServicePlugin.resolveTransport}: a + * deployment that names a sender is declaring one, and a boot that cannot + * honour the declaration must fail loudly rather than start half-configured. + * The alternative was measured — `OS_EMAIL_FROM="ObjectOS Local + * "` booted clean, and the address was first judged inside + * `normalizeMessage` on the first real send, which for a fresh deployment is + * the first user's sign-up: better-auth ran that send through + * `runInBackgroundOrAwait`, logged the throw and swallowed it, so the account + * was created, the UI reported "verification email sent", and nothing had + * been. + * + * `undefined` is NOT rejected: a service with no default sender is a + * complete, working configuration for callers that always pass `input.from`, + * and `normalizeMessage` already refuses a send that has neither. + * + * The settings channel takes the opposite trade — see + * {@link EmailServicePlugin.applyMailSettings}: one bad save must not stop a + * running server, so there the value is refused, the previous sender kept, + * and the consequence stated at `error`. + */ +function assertSendableDefaultFrom(from: EmailAddress | undefined): void { + if (from === undefined || isSendableAddress(from)) return; + const shown = typeof from === 'string' ? from : (from?.address ?? ''); + throw new Error( + `EmailServicePlugin: the configured default sender '${String(shown) || '(empty)'}' is not a valid email ` + + 'address, so EVERY message this deployment sends would be rejected before it reached the transport — ' + + 'including the sign-up verification mail, which is discarded in a background task and would leave users ' + + `told their mail was sent. ${DEFAULT_FROM_FIX}`, + ); +} + /** * EmailServicePlugin — registers the `email` service. * @@ -363,6 +406,10 @@ export class EmailServicePlugin implements Plugin { } async init(ctx: PluginContext): Promise { + // The declared sender has to be sendable, and this is where that is + // knowable — before a single message depends on it (#14318). + assertSendableDefaultFrom(this.options.defaultFrom); + // Register sys_email + sys_email_template via manifest service. ctx.getService<{ register(m: any): void }>('manifest').register({ id: 'com.objectstack.service.email', @@ -1404,7 +1451,23 @@ export class EmailServicePlugin implements Plugin { const fromEmail = typeof values.from_email === 'string' ? values.from_email : undefined; const fromName = typeof values.from_name === 'string' ? values.from_name : undefined; - if (fromEmail) this.service.setDefaultFrom({ address: fromEmail, name: fromName }); + let appliedFrom: string | undefined; + if (fromEmail && !isSendableAddress({ address: fromEmail, name: fromName })) { + // #14318 — the settings twin of the constructor assertion, taking this + // method's stated trade: refuse the value, KEEP the previous sender, + // never throw. `error` and not `warn` because nothing looks broken + // afterwards — the save succeeds, the page shows the address the + // operator typed — while every send made with it would be rejected + // before it reached the transport. + ctx.logger.error( + `EmailServicePlugin: the saved From address '${fromEmail}' is not a valid email address — it is ` + + 'NOT applied (the previous sender is kept) because every message sent from it would be rejected ' + + `before reaching the transport, silently on the sign-up path. ${DEFAULT_FROM_FIX}`, + ); + } else if (fromEmail) { + this.service.setDefaultFrom({ address: fromEmail, name: fromName }); + appliedFrom = fromEmail; + } const provider = String(values.provider ?? 'smtp'); @@ -1449,7 +1512,7 @@ export class EmailServicePlugin implements Plugin { if (provider === 'log') { ctx.logger.info( - `EmailServicePlugin: mail settings applied (provider=log, from=${fromEmail ?? '∅'}); ` + `EmailServicePlugin: mail settings applied (provider=log, from=${appliedFrom ?? '∅'}); ` + 'transport unchanged — messages are logged and recorded in sys_email, never delivered.', ); return; diff --git a/packages/plugins/plugin-email/src/email-service.ts b/packages/plugins/plugin-email/src/email-service.ts index e031b07a83..0198f7acae 100644 --- a/packages/plugins/plugin-email/src/email-service.ts +++ b/packages/plugins/plugin-email/src/email-service.ts @@ -207,13 +207,41 @@ function assertInsertConfirmedRowId(res: { id: string } | string | undefined, ro // backtracking (ReDoS) of the naive `[^\s@]+\.[^\s@]+` shape. const EMAIL_REGEX = /^[^\s@]+@[^\s@.]+(?:\.[^\s@.]+)+$/; +/** The bare `addr-spec` half of an {@link EmailAddress}, trimmed. */ +function addressPart(addr: EmailAddress): string { + const obj = typeof addr === 'string' ? { address: addr } : (addr ?? { address: undefined }); + return String(obj.address ?? '').trim(); +} + +/** + * Would {@link formatAddress} accept this address — i.e. can a message ever + * be sent with it? + * + * The predicate exists so a CONFIGURED sender can be judged where it is + * configured (`EmailServicePlugin.init`, the `mail` settings apply) instead of + * on the first send. `OS_EMAIL_FROM="ObjectOS Local "` is + * the measured shape: `EMAIL_REGEX` requires a dotted domain, so every send + * threw `Invalid email address` deep inside `normalizeMessage` — and on the + * sign-up path that throw landed in better-auth's `runInBackgroundOrAwait`, + * which logs and swallows, so the account was created, the UI said "we sent + * you a verification email", and nothing had been sent. + * + * It shares {@link EMAIL_REGEX} with `formatAddress` on purpose: a second + * spelling of "valid enough to send" would let a boot pass a check the send + * path then fails. + */ +export function isSendableAddress(addr: EmailAddress | undefined | null): boolean { + if (addr == null) return false; + return EMAIL_REGEX.test(addressPart(addr)); +} + /** * Format an EmailAddress (string or {name,address}) into the canonical * `"Display" ` form. Throws if address is malformed. */ export function formatAddress(addr: EmailAddress): string { const obj = typeof addr === 'string' ? { address: addr } : addr; - const address = String(obj.address ?? '').trim(); + const address = addressPart(addr); if (!EMAIL_REGEX.test(address)) { throw new Error(`Invalid email address: ${address || '(empty)'}`); } @@ -274,6 +302,35 @@ export function normalizeMessage( return msg; } +/** + * What a `sys_email` envelope column says when the rejected input carried + * nothing usable for it. Non-empty on purpose: `from_address`, `to_addresses` + * and `subject` are `required: true` on the object, so an empty string would + * make the rejection record itself unwritable — and "the caller supplied no + * recipient" is exactly the fact the row exists to preserve. + */ +const UNSTATED_COLUMN = '(none)'; + +/** + * Render an address for the REJECTION record — never for delivery. + * + * Deliberately not {@link formatAddress}: the message is here precisely + * because canonicalization threw, so the row records what the caller actually + * passed, verbatim, rather than throwing a second time while trying to + * describe the first throw. + */ +function rawAddressText(v: EmailAddress | EmailAddress[] | undefined): string { + if (v == null) return ''; + const one = (a: EmailAddress): string => { + if (a == null) return ''; + if (typeof a === 'string') return a.trim(); + const address = String(a.address ?? '').trim(); + const name = a.name?.trim(); + return name ? `${name} <${address}>` : address; + }; + return (Array.isArray(v) ? v : [v]).map(one).filter(Boolean).join(', '); +} + /** Split a persisted comma-separated address column back into a list. */ function splitAddresses(v: unknown): string[] { if (v == null) return []; @@ -607,7 +664,17 @@ export class EmailService implements IEmailService { try { normalized = normalizeMessage(input, this.options.defaultFrom); } catch (err: any) { - // Validation failures must surface to the caller. + // Validation failures must surface to the caller — AND leave a record. + // + // This is the ONE window in which a send could fail without producing a + // `sys_email` row at all: everything below persists first and finalizes + // the same row, so a transport failure is already an audit row at + // `status:'failed'`. A message rejected HERE used to vanish, and the + // caller's own error handling is not a substitute — better-auth runs + // `sendVerificationEmail` through `runInBackgroundOrAwait`, which logs + // the throw and swallows it, so the only durable evidence a deployment + // has that its verification mail never left is the row written here. + await this.recordRejectedMessage(input, err); throw err; } @@ -757,6 +824,67 @@ export class EmailService implements IEmailService { } } + /** + * Write the `sys_email` record for a message REJECTED before delivery — + * a `normalizeMessage` throw (bad/absent `from`, no recipient, no subject, + * no body). + * + * ## Why a row for something that was never sent + * `sys_email` is documented as the log of "every email the platform has + * tried to deliver", and until this existed the one class it did NOT record + * was the class nobody can otherwise see: the caller's error handling may be + * a `catch` that logs (better-auth's background-task runner is exactly + * that), so the attempt left no trace an operator could query. `status` and + * `error` say what happened; the envelope columns say which message it was. + * + * ## Why the row is safe to insert + * It lands at `status:'failed'`, and BOTH re-delivery paths gate on + * `status === 'queued'` — the `afterInsert` outbox drain hook and the boot + * `sweepStrandedOutbox`. So a rejection record can never be picked up and + * "re-delivered": the message was never acceptable in the first place. + * + * ## Why it never throws + * It runs on a path that is already failing and whose error belongs to the + * caller. A persistence failure here is reported at `warn` and swallowed — + * the same non-fatal treatment the happy path gives a failed insert — so + * this can never replace the caller's real error with a storage one. + */ + private async recordRejectedMessage(input: SendEmailInput, err: unknown): Promise { + const persistence = this.options.persistence; + if (!persistence) return; + const reason = String((err as any)?.message ?? err ?? 'rejected').slice(0, 900); + const raw = (input ?? {}) as Partial; + const row: Record = { + id: newId(), + from_address: rawAddressText(raw.from ?? this.options.defaultFrom) || UNSTATED_COLUMN, + to_addresses: rawAddressText(raw.to) || UNSTATED_COLUMN, + ...(rawAddressText(raw.cc) ? { cc_addresses: rawAddressText(raw.cc) } : {}), + ...(rawAddressText(raw.bcc) ? { bcc_addresses: rawAddressText(raw.bcc) } : {}), + ...(rawAddressText(raw.replyTo) ? { reply_to: rawAddressText(raw.replyTo) } : {}), + subject: String(raw.subject ?? '').trim() || UNSTATED_COLUMN, + ...(typeof raw.text === 'string' ? { body_text: raw.text } : {}), + ...(typeof raw.html === 'string' ? { body_html: raw.html } : {}), + ...(raw.relatedObject ? { related_object: raw.relatedObject } : {}), + ...(raw.relatedId ? { related_id: raw.relatedId } : {}), + ...(raw.sentBy ? { sent_by: raw.sentBy } : {}), + ...(raw.organizationId ? { organization_id: raw.organizationId } : {}), + status: 'failed', + // Prefixed so the column distinguishes the two ways a send fails: this + // message never reached a transport, so reading it as "the SMTP host + // rejected us" would send an operator to the wrong system. + error: `rejected before delivery: ${reason}`, + attempt_count: 0, + }; + try { + await persistence.insert(row); + } catch (persistErr: any) { + this.options.logger?.warn( + 'EmailService: sys_email rejection record could not be persisted (non-fatal)', + { error: persistErr?.message, rejection: reason }, + ); + } + } + /** * Resolve the queue to publish THIS message to, or `undefined` to deliver * it inline. diff --git a/packages/plugins/plugin-email/src/sender-address-validation.test.ts b/packages/plugins/plugin-email/src/sender-address-validation.test.ts new file mode 100644 index 0000000000..473d4c9910 --- /dev/null +++ b/packages/plugins/plugin-email/src/sender-address-validation.test.ts @@ -0,0 +1,285 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #14318 — an unsendable `from` address must be judged where it is +// CONFIGURED, and a send rejected before delivery must still leave a record. +// +// The measured shape: `OS_EMAIL_FROM="ObjectOS Local "` +// booted clean (nothing looks at the address until something sends), so the +// first judgement happened on the first user's sign-up — inside +// `normalizeMessage`, on a promise better-auth runs through +// `runInBackgroundOrAwait`, which logs the throw and swallows it. Sign-up +// answered 200, the UI said "we sent you a verification email", `sys_email` +// held no failed row, and "resend" repeated the whole thing. +// +// Two halves are pinned here, one per defect: +// 1. the address is refused at configuration time — a THROW on the +// constructor/CLI channel, an `error` + "previous sender kept" on the +// settings channel (a save must not kill a running server); +// 2. a message rejected before delivery writes a `sys_email` row at +// `status:'failed'` carrying the reason. + +import { describe, it, expect, vi } from 'vitest'; +import { EmailServicePlugin } from './email-plugin.js'; +import { EmailService, type EmailPersistence } from './email-service.js'; + +// ── harness ──────────────────────────────────────────────────────────────── + +interface Resolved { value: unknown; source?: string } + +function fakeSettings(values: Record) { + return { + createClient: () => ({}), + getNamespace: async () => ({ values }), + registerAction: () => {}, + }; +} + +function fakeEngine() { + const inserted: Array<{ object: string; row: any }> = []; + return { + inserted, + async insert(object: string, row: any) { inserted.push({ object, row }); return { id: row.id }; }, + async update() { /* no-op */ }, + async find() { return []; }, + }; +} + +function fakeCtx(services: Record) { + const hooks: Record Promise | void>> = {}; + const logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }; + return { + logger, + getService: (name: string): T => { + if (!(name in services)) throw new Error(`service '${name}' not registered`); + return services[name] as T; + }, + registerService: (name: string, svc: unknown) => { services[name] = svc; }, + hook: (name: string, fn: () => Promise | void) => { (hooks[name] ??= []).push(fn); }, + fire: async (name: string) => { for (const fn of hooks[name] ?? []) await fn(); }, + }; +} + +/** Boot the plugin (provider=log, so no transport is built) with `mail` settings. */ +async function boot(mailValues: Record, opts: any = {}) { + const services: Record = { + manifest: { register: () => {} }, + objectql: fakeEngine(), + settings: fakeSettings({ + provider: { value: 'log', source: 'default' }, + ...mailValues, + }), + }; + const ctx = fakeCtx(services); + const plugin = new EmailServicePlugin({ provider: 'log', seedTemplates: false, ...opts }); + await plugin.init(ctx as never); + await plugin.start(ctx as never); + await ctx.fire('kernel:ready'); + return { plugin, ctx, service: services.email as EmailService }; +} + +const errorLines = (ctx: { logger: { error: ReturnType } }): string[] => + ctx.logger.error.mock.calls.map((c: unknown[]) => String(c[0])); + +// ── 1. configuration-time refusal — the constructor / CLI channel ────────── + +describe('EmailServicePlugin.init — declared default sender', () => { + const ctxFor = () => fakeCtx({ manifest: { register: () => {} } }); + + it('refuses to boot with the exact OS_EMAIL_FROM shape that shipped the defect', async () => { + const plugin = new EmailServicePlugin({ + provider: 'log', + seedTemplates: false, + defaultFrom: { name: 'ObjectOS Local', address: 'noreply@localhost' }, + }); + await expect(plugin.init(ctxFor() as never)).rejects.toThrow(/noreply@localhost/); + }); + + it('names the consequence and the fix, not just the address', async () => { + const plugin = new EmailServicePlugin({ + provider: 'log', + seedTemplates: false, + defaultFrom: 'noreply@localhost', + }); + const err = await plugin.init(ctxFor() as never).then( + () => { throw new Error('init resolved — the invalid sender was accepted'); }, + (e: Error) => e, + ); + // The consequence (every send rejected, silently on sign-up) and the fix + // (OS_EMAIL_FROM / config.email.defaultFrom) are what make the boot + // failure actionable; the address alone is not. + expect(err.message).toMatch(/EVERY message/); + expect(err.message).toMatch(/OS_EMAIL_FROM/); + }); + + it('refuses an address with no domain at all', async () => { + const plugin = new EmailServicePlugin({ provider: 'log', seedTemplates: false, defaultFrom: 'noreply' }); + await expect(plugin.init(ctxFor() as never)).rejects.toThrow(/not a valid email address/); + }); + + it('boots with a deliverable sender', async () => { + const ctx = ctxFor(); + const plugin = new EmailServicePlugin({ + provider: 'log', + seedTemplates: false, + defaultFrom: { name: 'ObjectOS Local', address: 'no-reply@objectstack.local' }, + }); + await expect(plugin.init(ctx as never)).resolves.toBeUndefined(); + }); + + it('boots with NO declared sender — callers may always pass input.from', async () => { + const plugin = new EmailServicePlugin({ provider: 'log', seedTemplates: false }); + await expect(plugin.init(ctxFor() as never)).resolves.toBeUndefined(); + }); +}); + +// ── 2. configuration-time refusal — the settings channel ─────────────────── + +describe('applyMailSettings — saved From address', () => { + it('refuses an unsendable saved address, keeps the previous sender, and says so at error', async () => { + const { ctx, service } = await boot( + { from_email: { value: 'noreply@localhost', source: 'global' } }, + { defaultFrom: { name: 'Boot', address: 'no-reply@objectstack.local' } }, + ); + + // The running service still sends from the address that WORKS. + expect(service.options.defaultFrom).toEqual({ name: 'Boot', address: 'no-reply@objectstack.local' }); + + // `error`, not `warn`: the save succeeded and the page shows what the + // operator typed, so nothing looks broken from the outside. + const lines = errorLines(ctx); + expect(lines.some((m) => m.includes('noreply@localhost') && m.includes('NOT applied'))).toBe(true); + expect(lines.some((m) => m.includes('OS_EMAIL_FROM'))).toBe(true); + }); + + it('does not report the refused address as applied', async () => { + const { ctx } = await boot( + { from_email: { value: 'noreply@localhost', source: 'global' } }, + { defaultFrom: 'no-reply@objectstack.local' }, + ); + const applied = ctx.logger.info.mock.calls.map((c: unknown[]) => String(c[0])) + .filter((m) => m.includes('mail settings applied')); + expect(applied.length).toBeGreaterThan(0); + expect(applied.some((m) => m.includes('noreply@localhost'))).toBe(false); + }); + + it('applies a deliverable saved address', async () => { + const { ctx, service } = await boot({ + from_email: { value: 'no-reply@example.test', source: 'global' }, + from_name: { value: 'Acme', source: 'global' }, + }); + expect(service.options.defaultFrom).toEqual({ address: 'no-reply@example.test', name: 'Acme' }); + expect(errorLines(ctx)).toEqual([]); + }); + + it('never throws out of the settings path — a save must not kill the server', async () => { + await expect( + boot({ from_email: { value: 'noreply@localhost', source: 'global' } }), + ).resolves.toBeDefined(); + }); +}); + +// ── 3. a message rejected before delivery leaves a sys_email row ─────────── + +describe('EmailService — rejection is recorded in sys_email', () => { + function makePersistence() { + const rows: Array> = []; + const p: EmailPersistence = { + async insert(row) { rows.push({ ...row }); return { id: row.id }; }, + async update() { /* no-op */ }, + }; + return { p, rows }; + } + + const transport = () => ({ send: vi.fn(async () => ({ messageId: '' })) }); + + it('writes status=failed + the reason when the default sender is unsendable', async () => { + const t = transport(); + const { p, rows } = makePersistence(); + const svc = new EmailService({ + transport: t, + defaultFrom: { name: 'ObjectOS Local', address: 'noreply@localhost' }, + persistence: p, + }); + + await expect( + svc.send({ to: 'user@example.test', subject: 'Verify your email', text: 'click here' }), + ).rejects.toThrow(/Invalid email address: noreply@localhost/); + + // The caller still gets the error (auth propagates it), AND the attempt is + // now queryable — which it was not: this window inserted no row at all. + expect(t.send).not.toHaveBeenCalled(); + expect(rows).toHaveLength(1); + expect(rows[0]).toMatchObject({ + status: 'failed', + from_address: 'ObjectOS Local ', + to_addresses: 'user@example.test', + subject: 'Verify your email', + attempt_count: 0, + }); + expect(String(rows[0].error)).toContain('Invalid email address: noreply@localhost'); + }); + + it('distinguishes a pre-delivery rejection from a transport failure in `error`', async () => { + const { p, rows } = makePersistence(); + const svc = new EmailService({ transport: transport(), defaultFrom: 'noreply@localhost', persistence: p }); + await expect(svc.send({ to: 'a@b.test', subject: 'Hi', text: 'x' })).rejects.toThrow(); + // An operator reading `error` must not be sent to the SMTP host for a + // message that never reached one. + expect(String(rows[0].error)).toMatch(/^rejected before delivery: /); + }); + + it('records a rejection whose envelope is itself incomplete', async () => { + const { p, rows } = makePersistence(); + const svc = new EmailService({ transport: transport(), defaultFrom: 'no-reply@example.test', persistence: p }); + await expect(svc.send({ to: [], subject: '', text: 'x' } as never)).rejects.toThrow(/VALIDATION_FAILED/); + // `from_address` / `to_addresses` / `subject` are required columns, so the + // record has to stay writable when the input names none of them. + expect(rows[0]).toMatchObject({ status: 'failed', to_addresses: '(none)', subject: '(none)' }); + }); + + it('carries the linkage the send asked for, so the failure is findable from the user record', async () => { + const { p, rows } = makePersistence(); + const svc = new EmailService({ transport: transport(), defaultFrom: 'noreply@localhost', persistence: p }); + await expect(svc.send({ + to: 'user@example.test', + subject: 'Verify your email', + text: 'x', + relatedObject: 'sys_user', + relatedId: 'usr_1', + })).rejects.toThrow(); + expect(rows[0]).toMatchObject({ related_object: 'sys_user', related_id: 'usr_1' }); + }); + + it('never replaces the caller\'s error with a persistence one', async () => { + const warn = vi.fn(); + const persistence: EmailPersistence = { async insert() { throw new Error('db down'); } }; + const svc = new EmailService({ + transport: transport(), + defaultFrom: 'noreply@localhost', + persistence, + logger: { info: vi.fn(), warn }, + }); + await expect(svc.send({ to: 'a@b.test', subject: 'Hi', text: 'x' })) + .rejects.toThrow(/Invalid email address/); + expect(warn).toHaveBeenCalledWith( + expect.stringContaining('rejection record could not be persisted'), + expect.any(Object), + ); + }); + + it('writes nothing when the service has no persistence wired', async () => { + const svc = new EmailService({ transport: transport(), defaultFrom: 'noreply@localhost' }); + await expect(svc.send({ to: 'a@b.test', subject: 'Hi', text: 'x' })).rejects.toThrow(); + }); + + it('leaves the row where no re-delivery path can pick it up', async () => { + // Both consumers of a stranded row gate on `status === 'queued'` — the + // afterInsert outbox drain hook and the boot sweep. A rejection record + // must never be mistaken for an outbox entry and "re-delivered". + const { p, rows } = makePersistence(); + const svc = new EmailService({ transport: transport(), defaultFrom: 'noreply@localhost', persistence: p }); + await expect(svc.send({ to: 'a@b.test', subject: 'Hi', text: 'x' })).rejects.toThrow(); + expect(rows[0].status).not.toBe('queued'); + expect(rows[0].message_id).toBeUndefined(); + }); +}); From 5170e7a85c80db628cfb9fe9133a9dd5e13d5fc7 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <50353452+hotlong@users.noreply.github.com> Date: Wed, 2 Sep 2026 10:42:53 +0800 Subject: [PATCH 2/2] test(plugin-email): route the new fake engine's update() through assertEngineUpdateDispatch check:engine-double-contract flagged the double in sender-address-validation.test.ts: a fake looser than ObjectQL.update is how a dead route once shipped with a green suite. Routed through the predicate (the shape email-plugin.template-runtime-write.test.ts already pins) and recorded the new pinned coverage in the ledger with --write. Co-Authored-By: Claude Fable 5.1 --- .../src/sender-address-validation.test.ts | 21 +++++++++++++++---- scripts/engine-double-contract.pinned.json | 5 +++++ 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/packages/plugins/plugin-email/src/sender-address-validation.test.ts b/packages/plugins/plugin-email/src/sender-address-validation.test.ts index 473d4c9910..ba24028c14 100644 --- a/packages/plugins/plugin-email/src/sender-address-validation.test.ts +++ b/packages/plugins/plugin-email/src/sender-address-validation.test.ts @@ -19,6 +19,7 @@ // `status:'failed'` carrying the reason. import { describe, it, expect, vi } from 'vitest'; +import { assertEngineUpdateDispatch } from '@objectstack/objectql'; import { EmailServicePlugin } from './email-plugin.js'; import { EmailService, type EmailPersistence } from './email-service.js'; @@ -34,12 +35,24 @@ function fakeSettings(values: Record) { }; } +/** + * The slice of ObjectQL the plugin's `sys_email` persistence seam touches. + * `update` routes through `assertEngineUpdateDispatch` — the pinned shape in + * this package (`email-plugin.template-runtime-write.test.ts`) — so the double + * cannot accept a dispatch the real engine would refuse. + */ function fakeEngine() { - const inserted: Array<{ object: string; row: any }> = []; + const rows: Array<{ object: string; row: any }> = []; return { - inserted, - async insert(object: string, row: any) { inserted.push({ object, row }); return { id: row.id }; }, - async update() { /* no-op */ }, + rows, + async insert(object: string, row: any) { rows.push({ object, row }); return { id: row.id }; }, + async update(_object: string, data: any, options?: any) { + const dispatch = assertEngineUpdateDispatch(data, options); + if (dispatch.kind !== 'by-id') throw new Error(`unexpected update dispatch: ${dispatch.kind}`); + const target = rows.find((r) => r.row.id === dispatch.id); + if (target) Object.assign(target.row, data); + return { affected: target ? 1 : 0 }; + }, async find() { return []; }, }; } diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 0dd09cc3ad..1658a13117 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -2361,6 +2361,11 @@ "verb": "update", "pinned": 1 }, + { + "file": "packages/plugins/plugin-email/src/sender-address-validation.test.ts", + "verb": "update", + "pinned": 1 + }, { "file": "packages/plugins/plugin-security/src/authored-row-write-verdict.test.ts", "verb": "delete",