feat(identity): add a player-set anti-phishing code for outgoing email

[damianrzepka]

Summary

• Lets a player set a free-text, case-sensitive anti-phishing recognition phrase in Security settings via identity.security.setAntiPhishingCode (no reauth, set/overwrite only) and read it back raw through security.me.
• Widens EmailTemplateRenderer.render() with a 4th optional antiPhishingCode param, threaded from MailService.deliver() via AdminUserDirectory, so DefaultEmailTemplateRenderer appends it as a footer line on every outgoing platform email for a recipient who set one.
• Registers the new identity.security.anti_phishing_code.set domain event in the audit pipeline.

Openora's half of the paired BF-520 build (Jira: BF-520 "Anti-phishing code"). Betfeel's frontend (Security settings modal) and branded-email renderer overlay consume this contract; their side lands separately.

Test plan

• pnpm verify (typecheck, lint, unit + integration tests) green
• pnpm regen run, docs/catalog.json/docs/platform/system-design.md committed, check:drift clean
• New E2E: packages/testing/src/__tests__/anti-phishing-code.e2e.test.ts (happy path + round-trip via security.me, overwrite, whitespace-only rejection, non-player 403)
• Identity integration test: role check, no-reauth write, code absent from the emitted event payload
• Mail tests: renderer footer present/absent, HTML-escaping of the code, 4th render() arg threading

[jakubfilinger-b]

Overall looks good — trace is clean (session-derived userId, owner-filtered write, code kept out of the event/audit payload, safe additive migration) and tests cover the key paths. One thing I'd like fixed before merge: the missing max length on the anti-phishing code input. The rest are minor / follow-up-able.

[mp-blurify]

Code review — findings

Blocking

1. packages/core/src/contracts/schemas/identity.ts:206 — unbounded input on a value appended to every email

z.string().trim().min(1) has no .max() and no charset limit, backing an unbounded text column, and core sets no request body size limit. Every other player-supplied string in this same file is bounded (PasswordSchema 128, name 255, LanguageSchema 35).

Failure: a player POSTs a multi-megabyte code. It is stored, then appended to the body of every subsequent email for that account, so the provider rejects the oversize message and their regulatory mail (cooling-off, self-exclusion, KYC resubmission) permanently dead-letters via mail.service.ts:106 mail.regulatory_delivery.failed — a self-inflicted denial of compliance email.

Amplified because DrizzleAdminUserDirectory.list/get/lookupUsers all select() full rows, so the blob rides along on every back-office user page and every chat online-count lookup. Embedded newlines are also accepted, letting the player inject extra paragraphs into their own rendered mail (self-directed, so not separately blocking).

2. packages/core/src/mail/adapters/default-email-template-renderer.ts:153 + packages/core/src/contracts/adapters/email-template.ts:11 — the control lives in an operator-replaceable adapter

The footer is added as an optional 4th positional parameter on EmailTemplateRenderer. docs/adapters/mail.md (not touched by this PR) still documents EMAIL_TEMPLATE_RENDERER.render(template, locale) and states an overlay renderer "owns the rendered result for each key; there is no automatic fallback to the platform's English renderer" — and the stock sender never actually delivers mail, so every real operator binds their own renderer.

A custom renderer whose render takes two parameters still satisfies EmailTemplateRenderer structurally (TS allows fewer params), so there is no compile error, no test, and no doc line to warn them. Net effect in production: no email carries the code, or only some do once a partial overlay is written — the "trains players to trust codeless mail" failure mode, which defeats the point of the control.

Suggested fix: append the footer in MailService.deliver (packages/core/src/mail/service/mail.service.ts:112) after renderer.render(...) returns, so no overlay can drop it. At minimum, document the parameter in docs/adapters/mail.md.

Worth fixing

• packages/core/src/audit/plugin.ts:817 — the record carries only after: { antiPhishingCodeSet: true } and no before. docs/standards/audit.md:5 requires "meaningful before/after state", and the sibling control does exactly that (identity.security.withdrawal_pin.set carries before: { withdrawalPinSet: wasAlreadySet }). The service already has the prior state in hand at identity.service.ts:1685. Without it, an account-takeover investigation cannot distinguish a first-time set from a silent replacement of an existing code — the single most diagnostic fact about this control.
• packages/core/src/pam/identity/service/identity.service.ts:1656 — no rate limiter and no notification on change. The sibling withdrawal-pin mutation consumes RATE_LIMIT_KEYS.WITHDRAWAL_PIN_MUTATION; this route consumes nothing, so the unbounded-length write above can also be hammered. More importantly, an attacker holding a stolen session can set their own code and the victim is never told; every later genuine platform email then carries the attacker's code, so the control certifies the attacker's phishing baseline instead of exposing it. Standard mitigation is a mailDispatch.toUser notice on change, rendered with the previous code.
• packages/core/src/contracts/adapters/admin-user-directory.ts:27 — putting the value on AdminUserRow exposes it to every consumer of ADMIN_USER_DIRECTORY (admin-console, chat, chat-commands, wallet, compliance, tag). All were traced; nothing leaks it today, but only because AdminUserSchema is a non-strict z.object and oRPC's validateOutput returns the zod-parsed value, silently stripping the extra key. BackofficeService.toAdminUser (packages/core/src/admin-console/service/backoffice.service.ts:27) spreads the whole row, so the moment anyone widens that output schema or adds a route returning the raw row, every support agent can read every player's recognition code. The in-code comment claiming it "is not treated as secret" is wrong about the threat model — the control's entire value is that nobody outside the player and the mail renderer knows it. A narrow mail-only read would keep the blast radius at one consumer.

Notes

• The authz trace is clean: the caller is resolved from the session (never from input), the write is where(eq(user.id, userId)), the raw value is kept out of the event and audit payloads, the migration is additive-nullable, and the HTML path escapes the code (with a test proving <script> is neutralized). No IDOR, no unauthenticated exposure, no log leak, no hygiene violation.
• antiPhishingCodeSetAt is written at identity.service.ts:1689 and never read or exposed anywhere.
• The e2e (packages/testing/src/__tests__/anti-phishing-code.e2e.test.ts) never asserts that a captured outgoing email contains the code — coverage is unit-level only, so blocking item 2 has no regression guard.

[damianrzepka]

Addressed the findings in ee005db. The input is capped at 255; MailService now appends the escaped footer after any renderer completes, so an overlay cannot drop it; and the raw code moved off AdminUserDirectory into a dedicated mail-recipient port. The audit event now carries only wasAlreadySet for meaningful before/after state, repeated values short-circuit, and writes are rate-limited. A security notification is queued on a real change and carries the prior code via the address-only path, so it is not stamped with the replacement code. Added coverage for the 255-character boundary, unchanged submissions, audit state, change notices, and core-owned footer delivery. pnpm -F @openora/core check:types is green.

[jakubfilinger-b]

Changes requested: resolve the BF-520 requirement conflicts, make the notification and queue rollout safe, repair the migration lineage, and regenerate the catalog because the current verify check fails check:drift.

[jakubfilinger-b]

BF-520 explicitly requires no length or character restriction, but .trim().min(1).max(255) changes leading/trailing characters and rejects whitespace-only or 256+ character values; please resolve the security/spec conflict in the ticket first, then align the schema and tests with the agreed bounded representation.

[damianrzepka]

Valid specification conflict. Core currently keeps trim, non-empty, and 255-character bounds to prevent player-controlled regulatory-email amplification, but BF-520 reportedly says no restriction. I cannot silently choose between incompatible requirements; the ticket needs an explicit security decision before schema or test changes.

[jakubfilinger-b]

The row is committed before the fallible player lookup and mail enqueue, so a queue failure returns an error after changing the code, while retrying the same value exits at the no-op branch and never restores the missing audit/notification effects; persist a retry-safe notification intent atomically with the mutation (for example via the transactional outbox) and cover enqueue failure plus retry.

[damianrzepka]

Valid. Direct enqueue is not retry-safe after the database write. The shipped transactional outbox persists domain-event envelopes; preserving this notice safely needs a transactionally persisted encrypted notification intent, a retrying consumer, and a stable mutation ID. I have not added an ad-hoc non-atomic workaround.

[jakubfilinger-b]

This producer/consumer change is not rolling-deploy safe: old mail-send workers can still consume ordinary jobs without appending the footer, and they reject the new securityAntiPhishingCodeChanged template before delivery; deploy backward-compatible worker/schema support before activating the setter, or version/route the queue so old workers cannot claim new jobs.

[damianrzepka]

Valid deployment concern. The new setter and template are not safe to activate while old mail workers may claim jobs. This needs an operator rollout gate or versioned backward-compatible worker path before activation; core has no deployment coordinator to guarantee order, so I have not claimed it is safe.

[jakubfilinger-b]

Using toAddress intentionally keeps the attacker-selected replacement code out of the change alert, which is the safer behavior, but it contradicts BF-520's literal requirement that the saved code appear in every genuine email; document this security-notification exception explicitly in BF-520 and test first-set versus overwrite semantics.

[damianrzepka]

The address-only exception is documented in code and tests cover first-set versus overwrite notification payloads. The requested BF-520 wording is ticket and product-record work; it should explicitly allow this security notice to carry the previous code rather than the newly saved code.

[jakubfilinger-b]

Review — BF-520 core side

The port design here is genuinely good: MailRecipientDirectory is deliberately narrow rather than widening AdminUserDirectory, with the reasoning in the docstring. securityControlsFor selects explicit columns, textToHtml escapes so the free-text code cannot inject HTML, and the rate limit plus unauthorized_access event on the non-player path are right. Findings below, most severe first.

1. [High] Orphaned duplicate migration 0012_reflective_hellcat.sql

The PR adds two new migrations that both add anti_phishing_code:

• 0012_reflective_hellcat.sql — not in _journal.json (idx 12 is 0012_worried_nuke), collides on the 0012_ prefix with an already-applied migration, and adds anti_phishing_code_set_at, a column the schema never declares.
• 0014_spicy_mephistopheles.sql — the real one, correctly journalled.

The first looks like a leftover from an earlier attempt. Drizzle reads the journal so it will not execute, but it will confuse anyone reading the folder and any tooling that globs *.sql. Please delete it.

2. [High] The footer is appended after </html>, so branded emails lose the code

appendAntiPhishingCode post-processes the finished render in packages/core/src/mail/service/mail.service.ts:

html: `${rendered.html}\n<p>${escapeHtml(footer)}</p>`,

betfeel's BetfeelEmailTemplateRenderer delegates to @react-email, whose shell.tsx emits a complete <Html>…</Html> document. The footer therefore lands outside the document, where Gmail and Outlook routinely strip it; where it does survive it renders as unstyled text below a fully branded email.

The failure mode is the bad one: an anti-phishing code silently missing from a genuine email trains the player to ignore its absence, which defeats the feature. The renderer should own placement.

3. [High] The PR description does not match the implementation

The body states EmailTemplateRenderer.render() was widened "with a 4th optional antiPhishingCode param, threaded from MailService.deliver()", and the test plan claims coverage of "4th render() arg threading".

In fact packages/core/src/contracts/adapters/email-template.ts is not in this diff at all — render() still takes three params (template, locale, recipientName?), MailService calls it with three arguments, and the new tests assert render was called with (verify, 'fr', 'Ada').

Note that betfeel !374 has already implemented the described 4-param design end to end (BetfeelEmailTemplateRenderer → renderEmailTemplate → EmailRenderContext → Footer). Because the 4th param is optional the mismatch compiles silently, so core simply never passes the code and betfeel's branded footer is dead code. The port version is the better design — it is the one that can place the code inside the branded layout. Suggest changing this PR to match !374 rather than the reverse, which also resolves finding #2.

4. [Medium] New mail template key breaks consumer typechecks

securityAntiPhishingCodeChanged joins MAIL_TEMPLATE_KEYS. betfeel's buildElement switch in packages/emails/src/render.tsx is exhaustive with no default arm, so betfeel's check:types breaks on the next @openora/* bump, and until a layout exists the change-notification email has nothing branded to render. Worth calling out as a required consumer change.

5. [Low] .update(user) does not verify the write landed

No .returning() — if the user row vanished between the read and the write, the update silently no-ops yet still emits the event and sends the confirmation mail.

6. [Low] The toAddress choice is load-bearing but unexplained

Dispatching the change notification via toAddress rather than toUser is correct and subtle: toUser would stamp the footer with the new code, so an attacker who just changed it would send the victim an email bearing the attacker's own code. Because toAddress skips the footer, the body's previousAntiPhishingCode is what makes the email recognisable. That reasoning deserves a comment — a future refactor to toUser would silently defeat the notification.

[mp-blurify]

Blocker: docs/catalog.json and docs/platform/system-design.md were not regenerated after the new adapter port landed, so pnpm check:drift fails CI on this PR. Beyond that, the anti-phishing footer is assembled after the renderer returns, which puts it outside the localization seam and breaks when a renderer returns a full HTML document - details inline. The points already raised on this PR read as answered by the current diff.

[damianrzepka]

Addressed the new review in c18fa7e. Regenerated the catalog (46 adapter ports; check:drift passes), restored the optional fourth EmailTemplateRenderer argument so branded renderers own translated in-document placement, and removed the orphaned 0012_reflective_hellcat migration. The anti-phishing-code change notification remains a new mail-template contract: its branded consumer renderer needs to add that template before adopting this core version; that work belongs in the consumer overlay, not core. The address-only dispatch now has an explicit comment explaining why it must not become toUser. Targeted mail tests pass; the full verify command began successfully but was terminated by this environment before completion.