[IAmJSD]

Fixes the four internal/email items of the 2026-07-28 review tracker: the plain-text renderer item plus A06-005, the raw-address log item plus A06-012, the mute fail-open item, and the SES/SMTP no-recipient item plus A06-014. Each was re-checked against main at 7b86e386 and still applied.

Refs #111

Root cause and fix

1. Plain-text bodies rendered through html/template (internal/email/templates.go)
RenderPurchaseScheduledDelayEmail and RenderPurchaseExecutedNotificationEmail called renderTemplate, the HTML renderer whose own doc comment says never to use it for plain text. Both now call renderTextTemplate, like every other plain-text renderer. The executed notice was already broken on main, beyond the latent & case the issue describes: its plain-text revoke URL rendered the url-escaped token through HTML escaping (?token=tok+en...). The copy-paste fallback link therefore carried an HTML entity. executed_notification_test.go had pinned that escaped form for the text half; it now expects the working URL (tok+en%2F...). The HTML half is unchanged and still pinned to +, which mail clients decode.

2. Raw recipient addresses in logs (internal/email/sender.go, internal/email/smtp_sender.go)
createVerificationRequest (SES sandbox path, Info level) and the four Debugf lines in SMTPSender.SendToEmailWithCC / SendToEmailWithCCMultipart now log redactEmail(toEmail), matching the other log lines on these paths. The full address is still used in the user-facing sandbox error, where it is intentional.

3. Mute check failed open for every scope (internal/email/mute.go)
On a mute-store error, isRecipientMuted now asks muteCheckFailsOpen(scope):

• purchase_approvals and ri_exchange_approvals fail open (send), so a DB blip can't strand a purchase or exchange waiting on its approval email.
• Every other scope, including any scope added later, fails closed (don't send), so the blip can't re-mail someone who used the one-click unsubscribe link.

filterMutedRecipients (CC list) and prepareMuteAwareDelivery follow the same rule. The MuteChecker == nil behavior (not configured, so nothing is muted) is unchanged. Today only the two approval scopes exist (common.MuteNotifScope), so behavior on current senders does not change. The rule is now explicit, and a new informational scope gets the safe default.

4. SMTP fell back to NOTIFY_EMAIL for token-bearing mail (internal/email/smtp_sender.go)
SMTPSender.SendRIExchangePendingApproval, SendPurchaseApprovalRequest, SendPurchaseScheduledNotification and SendPurchaseExecutedNotification now return ErrNoRecipient when data.RecipientEmail is empty, as the SES Sender does. Before, they mailed the approve, revoke or exchange-approval link to s.notifyEmail, which always resolves because it defaults to FROM_EMAIL. That made the ErrNoRecipient branch in internal/api/handler_purchases.go (approvalSendFailure) unreachable on Azure and GCP. Doc comments were updated to match.

Verification

All of the following is unit, mock and fixture evidence (mock SES client and an in-process mock SMTP server on 127.0.0.1). It includes no real SES, SMTP or cloud calls and no integration tests: none of these changes touch the DB.

Environment: Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true.

Fail-before: I ran the new and extended tests against unmodified main (7b86e386) with only the test files added:

go test ./internal/email/ -count=1 -run 'TestPlainTextTemplates_NoHTMLEscaping|TestIsRecipientMuted_|TestSenders_NoRecipientParity|TestCreateVerificationRequest_RedactsAddressInLog|TestSMTPSender_SendLogsRedactRecipient'
--- FAIL: TestCreateVerificationRequest_RedactsAddressInLog   ("Created email verification request for customer.person@example.com")
--- FAIL: TestSMTPSender_SendLogsRedactRecipient              ("Sent email via SMTP to customer.person@example.com", text + multipart, with and without CC)
--- FAIL: TestIsRecipientMuted_StoreErrorDirectionPerScope     (informational scopes: expected muted=true, got false; CC kept)
--- FAIL: TestSenders_NoRecipientParity                       (SMTP: expected ErrNoRecipient, got nil, for all four methods)
--- FAIL: TestPlainTextTemplates_NoHTMLEscaping               ("execution=exec-3&source=email", "O'Brien <a&b@example.com>")
FAIL

Pass-after, at 6d422e20:

go test ./internal/email/ -count=1                       ok
go test ./internal/server/ -count=1                      ok
go test ./internal/mocks/ -count=1                       ok
go build ./... ; go vet ./... ; go vet -tags integration ./...   clean
golangci-lint v2.10.1 run --allow-parallel-runners ./internal/email/...   0 issues
gocyclo -over 10 -ignore "_test\.go" internal/email/{mute,smtp_sender,sender,templates}.go   no output
gofmt -l internal/email                                   no output

Mutation probe: making muteCheckFailsOpen return false for the approval scopes fails TestIsRecipientMuted_StoreErrorDirectionPerScope and the existing TestSendPurchaseApprovalRequest_MuteCheckError_FailOpen. The opposite direction is the fail-before run above.

New and changed tests:

• TestPlainTextTemplates_NoHTMLEscaping (extended): scheduled-delay revoke URL with a second query parameter and </> in the window time; executed notice with O'Brien / a&b@example.com and a url-escaped token. The HTML half of the executed notice is also checked to still escape <script>.
• TestIsRecipientMuted_StoreErrorDirectionPerScope: each approval scope fails open; purchase_executed, new_recommendations and the empty scope fail closed. Covers isRecipientMuted, filterMutedRecipients and prepareMuteAwareDelivery.
• TestIsRecipientMuted_NoErrorFollowsStore: the store answer is passed through unchanged when there is no error; a nil checker is not muted.
• TestSenders_NoRecipientParity (new file no_recipient_parity_test.go): runs all four methods against both the SES Sender and the SMTPSender, with notifyEmail set and a live mock SMTP server. Both must return ErrNoRecipient; SES SendEmail must not be called and the SMTP server must see no session.
• TestCreateVerificationRequest_RedactsAddressInLog, TestSMTPSender_SendLogsRedactRecipient (new file log_redaction_test.go): capture the default logger through logging.SetOutput.
• TestSMTPSender_SendRIExchangePendingApproval_WithNotifyEmail was replaced by ..._NotifyEmailIsNotAFallback. The old test asserted nothing once the fallback was gone, and it dialed smtp.example.com:587.
• executed_notification_test.go: the text half now expects the unescaped working token (see fix 1).

Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree: queue/platform @ 400d256b = origin/main 7b86e386 plus these heads, merged in this order:

Branch	Tested head
fix/issue-243-rest	007c163
fix/issue-1206	312072f
fix/issue-111-email	6d422e2
fix/issue-111-fail-closed	3817e29
fix/issue-712-age-cap	a91120b
fix/issue-712-fargate	aeaf0d4
fix/issue-613-legacy-ids	350e837
fix/issue-120-guards	6f38ebc
fix/issue-120-shared-predicate	cb25735
fix/issue-151-account-update-keep	d20cf01
fix/issue-713-upcoming-filter	e73fdfb
fix/issue-263-bastion-opts	7861f32
fix/issue-43-fixture-ids	8cee203
fix/issue-177-makefile-walk	365a9d6
fix/issue-27-seller-not-registered	d845f66
fix/issue-313-user-columns	6d759dd
fix/issue-353-mock-panics	aac2edf
fix/issue-8-config-stamps	b548ae5
fix/issue-30-monthly-totals	f637921

Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers. The full suite ran on f7c48020, which differs from this tree only by test-only commits on fix/issue-30, fix/issue-713 and fix/issue-263 that replace placeholder account-number literals with named constants:

• Passed: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests, every changed package that has them, one package at a time (-tags integration -count=1): internal/analytics, internal/api, internal/auth, internal/config, internal/database/postgres/migrations, internal/purchase, internal/scheduler and internal/server all pass. Migrations 000149, 000150 and 000151 apply in order on this tree.
• Frontend: the changed jest files (approval-details, settings-accounts, api) pass (221 tests).
• Secrets: the first run flagged 9 new fixture lines (placeholder account numbers next to an account key); fixed with named constants. Re-run on this tree: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged. scripts/test-git-secrets-allowlist.sh: 183 PASS, 0 FAIL.
• Terraform .tftest.hcl files were not run locally (CI runs them). Nothing was applied to any cloud.

Behavior change for operators (please review)

On SMTP deployments (Azure ACS, GCP SendGrid), an RI-exchange approval run in manual mode with no Settings > General > notification email now logs failed to send RI exchange notification email: email: no recipient address instead of mailing the approval links to NOTIFY_EMAIL. AWS (SES) already behaves this way. The purchase approval, scheduled-delay and executed paths already resolve a recipient (or skip) before calling the sender, so they are unaffected in practice. This is the change the issue asks for, but it should be called out in release notes.

Out of scope

• The other items of chore(server/scheduler): medium/low findings from the 2026-07-28 full review #111 (federation .tfvars escaping, suppression-read failure direction, HTTP scheduled-path validation, IsLambda discriminator, A06-013 Azure bicep script escaping, A01-016 suppression config error). Scheduler/API items 1, 2 and 7 are being handled separately. The issue stays open.
• No email copy changes.
• Unrelated, not fixed: SMTPSender.SendScheduledPurchaseNotification (the pre-purchase reminder, which also carries approve/pause/cancel links) still sends to s.notifyEmail, while SES requires data.RecipientEmail. It was not one of the four methods named in A06-014, so I left it. It may deserve its own issue.

Labels

A maintainer please mirror the issue's triage labels onto this PR: priority/p3, severity/low, urgency/eventually, impact/many, effort/l, type/bug, triaged.

🤖 Generated with Claude Code

[coderabbitai]

Caution

Review failed

The pull request is closed.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 5cff3e1a8b28cce5a458599117961c1327db1dd8

Blocking:

• none

Non-blocking:

• internal/email/mute.go muteCheckFailsOpen: the issue asks for fail-open on "approval and action scopes". Any new scope defaults to fail-closed, so a future token-bearing scope (for example a revoke or ladder-approval scope) would quietly drop its email on a mute-store blip unless whoever adds it also updates this switch. Today only the two approval scopes exist, so nothing changes now. A one-line pointer next to the common.MuteNotifScope constants, or a test that enumerates every defined scope, would stop this from drifting.
• internal/email/smtp_sender.go SendRIExchangePendingApproval: as the body says, an SMTP (Azure/GCP-hosted) deployment with no Settings > notification email now sends the manual-mode RI exchange approval mail nowhere, and sendExchangeNotification only log.Printfs a warning. The issue asks for exactly this (A06-014), and the pending exchanges are still reachable in the dashboard. Still, it's an operator-visible behaviour change and belongs in the release notes, as the body says.
• SMTPSender.SendScheduledPurchaseNotification (the pre-purchase reminder with approve/pause/cancel links) still falls back to s.notifyEmail. The body correctly calls it out of scope. It has the same token-to-shared-inbox shape, so it's worth its own issue.

Attacked:

• The head moved from 6d422e20 to 5cff3e1a while I was reviewing. The only new commit is the golang.org/x/net v0.61.0 to v0.62.0 bump, and its diff is byte-identical to fix(deps): bump golang.org/x/net to v0.62.0 (GO-2026-6860, GO-2026-6866) #1564's. On 5cff3e1a I re-ran go build ./..., go vet ./... and go test -race -short -count=1 ./internal/email/, and all pass. Everything below was run on 6d422e20, which has the same non-dependency diff.
• Checked that the two renderers moved to text/template feed only plain-text bodies. The SES scheduled-delay path uses SendToEmailWithCCMultipart(..., body, ""), which degrades to SendToEmailWithCC (text). The executed notice's HTML half still uses html/template. So the switch adds no XSS sink, and urlquery is in both func maps.
• Checked every prepareMuteAwareDelivery caller (smtp_sender.go:532/575, templates.go:589/856). They all pass an approval scope, so the per-scope change has no effect on current senders. Read sendExchangeNotification to confirm what the ErrNoRecipient change means for RI exchange.
• Ran go build ./...; go vet with and without -tags integration on internal/email; golangci-lint v2.10.1 with and without --build-tags integration (0 issues); gocyclo -over 10 on the four changed files (silent); go test -race -short -count=1 ./internal/email/ ./internal/server/ (ok). internal/email has no integration tests.
• Fail-before: I reset mute.go, sender.go, smtp_sender.go and templates.go to origin/main and kept the new tests. TestCreateVerificationRequest_RedactsAddressInLog, TestSMTPSender_SendLogsRedactRecipient, TestIsRecipientMuted_StoreErrorDirectionPerScope, TestSenders_NoRecipientParity and TestPlainTextTemplates_NoHTMLEscaping all FAIL. I restored the files afterwards.

Body-stale:

• none

_{Reviewer: rG1. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at 5cff3e1a8b28cce5a458599117961c1327db1dd8 merged onto main (d05a1726), together with #1547, #1555, #1558, #1559, #1561, #1562 in that order (combined tree eea0132d, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 1 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 1 approving independent review(s) on 5cff3e1a, green CI, and the combined-tree verification above. Squash-merged at exactly 5cff3e1a8b28cce5a458599117961c1327db1dd8.