Make local dev-mailbox delivery opt-in and stop advertising /dev/mailbox

[timujinne]

Addresses the actionable half of #687: items 1 and 4 as filed, plus the design decision the issue left open — instead of an install-time option, local-mailbox delivery becomes a runtime opt-in, which also covers the delegation-mode hosts the installer can never reach.

The four changes

1. Config.mailer_local?/0 resolves the way deliver_email/2 actually sends (item 1). New PhoenixKit.Mailer.resolved_send_path/0 — default send integration → delegated host mailer (adapter read from the parent app's env) → built-in — and mailer_local?/0 derives from it. Fixes the delegation-mode false negative from the issue and the false positive @ddon added (installer-written Local block + delegating real mailer). Guarded with rescue/catch :exit: it renders on public pages, a dead pool means false, not a crash.

2. Local mailbox is opt-in. New setting dev_mailbox_enabled (default "false"). When the resolved adapter is Swoosh.Adapters.Local and the switch is off, deliver_email/2 does not hand the message to the mailbox: the recipient, subject and body (where the one-time links live; falls back to html_body for HTML-only mail) go to the server log with a one-line pointer at the switch, and the call returns {:ok, %{suppressed: true, ...}} so register/reset flows stay green. The gate sits before the tracking pipeline — a message never handed to an adapter is not recorded as sent. The integration path is untouched (no Local adapter exists among providers).

3. Toggle + warning on the existing /admin/settings/email-sending page. The section renders only when the resolved path is Local; switching the mailbox on shows the consequence spelled out ("tokens readable by anyone who can reach this server").

4. DevNotice stops advertising the inbox (item 4, the "drop the anchor" option). Gate closed → "outgoing email is written to the server log"; gate open → the old copy without the /dev/mailbox link. Its render condition (mailer_local?/0) is now truthful, which fixes the backwards visibility ddon described — the notice no longer shows only on the installs that were configured the ordinary way while staying silent on the actually-blind delegation hosts.

The cost, named

An upgraded dev install stops filling the mailbox until the operator flips the switch. Every suppressed send says exactly that in the log, the DevNotice copy changes visibly, and the admin page carries the banner — the failure mode is loud, and the tokens stay recoverable from the log.

Items 2 (router introspection in install/doctor) and 3 stay open: 2 as the agreed follow-up, 3 superseded — the safe default no longer depends on what the installer asks.

Evidence

• Full suite via the real test DB: 3288 tests, 2 failures — both the pre-existing stale chain_hash manifest items (see the Close the remaining review findings from the #694 round #695 review), untouched by this branch.
• mix precommit clean (compile with warnings-as-errors, format, credo --strict, dialyzer).
• External review (GLM-5.2, elixir-review persona): APPROVE; its four minor findings (test cache leak, HTML-only log fallback, one unguarded resolution call, a get_parent_app/0 doc note) are all fixed in the last commit.

[timujinne]

Code review — GLM-5.2 (elixir-review persona), verified against the branch

I have verified every claim against the actual code. Here is my review.

Summary

The core security fix is correct and complete for the intended threat model. I verified the key claims:

• Gate coverage: All PhoenixKit auth mail routes through Mailer.deliver_email/2 → deliver_via_configured_mailer/2 (user_notifier.ex:48, mailer.ex:632), where the gate sits. The integration path (deliver_via_integration/3) can never resolve to Swoosh.Adapters.Local (swoosh_config_for/1 only returns SES/SMTP/Brevo), so there's no bypass there. ✓
• Resolution consistency:resolved_send_path/0 branches on exactly the same default_send_integration_uuid/0 as deliver_email/2, and the Dev installs can route auth mail into an unauthenticated /dev/mailbox — detect it and offer the integration send path instead #687 false-positive/false-negative cases are genuinely fixed (delegated mailer now read from parent-app env; installer Local block no longer answers for a delegated real mailer). ✓
• DB-read robustness: The gate's dev_mailbox_enabled?/0 → get_boolean_setting → get_setting_cached chain is fully protected (get_setting_cached has both rescue and catch :exit, settings.ex:380-392, falling back to get_setting/1 which also has both) — so a dead pool cannot crash delivery. ✓
• Safe default:"dev_mailbox_enabled" => "false"; checkbox param shape (hidden false + checkbox true → "true"/"false") matches the handler. ✓

No correctness or security hole in the main flow. The findings below are all minor / test-hygiene / edge-case.

---

MINOR — Test isolation: dev_mailbox_enabled leaks into the global settings cache

File:test/integration/email/dev_mailbox_gate_test.exs:60-61, test/integration/email/dev_mailbox_toggle_test.exs

Both new tests write "dev_mailbox_enabled" => "true" and never reset/invalidate it. update_setting invalidates the cache key (settings.ex:1326), but the subsequent dev_mailbox_enabled?() read re-warms the global, app-supervised:settings cache with "true" (query_and_cache_setting, settings.ex:1859). The Ecto sandbox rolls back the DB row on exit, but the ETS cache write is not transactional, and the cache runs with a 5-minute TTL (supervisor.ex:66) — longer than a suite run.

Failing scenario:dev_mailbox_gate_test.exs (or the toggle test) runs and leaves "true" cached. dev_notice_test.exs's "gate closed" test then renders the notice: the outer :if={Config.mailer_local?()} is true (it sets Local), but mailbox_enabled?() reads the leaked "true", so it renders the "check the local mailbox" branch instead of "written to the server log". The assertion html =~ "written to the server log" fails. ExUnit's cross-module ordering is seed-dependent, so this surfaces as intermittent flakiness.

Fix: add on_exit(fn -> PhoenixKit.Cache.invalidate(:settings, "dev_mailbox_enabled") end) (or reset via update_setting(..., "false")) in each test that sets the key.

---

MINOR — Suppression fallback silently drops the token for HTML-only mail

File:lib/phoenix_kit/mailer.ex:267-276

log_suppressed_local_delivery/1 is the developer's only way to recover a single-use token when the mailbox is off, yet it logs only email.text_body (nil → interpolated as blank). All current core auth mail sets a text body (send_magic_link_email, UserNotifier.deliver/4), so this is latent — but UserNotifier is explicitly documented as overridable ("Override this module in your application"), and a host that sends HTML-only auth mail loses the token from the log with no signal.

Fix: also log email.html_body, or derive a text part (e.g. keep the body that contains a URL/token, preferring text and falling back to a stripped html_body).

---

NIT — resolved_send_path/0 is called unguarded in the admin LV

File:lib/phoenix_kit_web/live/settings/email_sending.ex:275

mailer_local?/0 deliberately wraps resolved_send_path/0 in rescue/catch :exit (public auth pages). The only other caller — assign_dev_mailbox/1 in the admin Email-Sending mount — calls it bare. resolved_send_path → Integrations.connected?/1 (integrations.ex:341) has no rescue/catch of its own; on a dead pool it can propagate an exit and 500 the page mount. Impact is low (the page needs a live DB to authenticate to anyway), but it's an inconsistency with the guarding the branch added elsewhere. Either guard the LV call or push the protection down into resolved_send_path/0.

---

LOW — Gate's adapter view can diverge from actual delivery for a delegated non-SES Local mailer

File:lib/phoenix_kit/mailer.ex:94-97 vs lib/phoenix_kit/mailer.ex:287-296

For a delegated, non-AWS mailer, do_deliver_via_configured_mailer/3 delivers via mailer.deliver/1 (Swoosh reads the mailer's baked-in otp_app), while the gate's configured_adapter/1 reads Application.get_env(get_parent_app(), mailer). If get_parent_app() (the unreliable fallback when :parent_app_name is unset and the mailer's otp_app ≠ the repo-derived app) disagrees with the mailer's real otp_app, the gate can see "not Local" while delivery actually goes to Swoosh.Adapters.Local — bypassing suppression. This is a pre-existing pattern (the SES detection reads the adapter the same way) and requires an unusual misconfiguration, so likelihood is low, but it's the one path where the "opt-in" guarantee could silently fail to hold. Worth a one-line note in the resolved_send_path doc that it assumes get_parent_app() matches the delegated mailer's otp_app.

---

Verdict: APPROVE — the opt-in gate is correctly placed, defaults safe, and the resolution logic faithfully matches deliver_email/2; the findings are minor test-hygiene and latent-robustness items, none of which block the security fix.

All four findings are addressed in the branch tip (Fix review findings: test cache leak, HTML-only token log, guarded LV mailbox check).

[timujinne]

Code review, round 2 — GLM-5.2 (elixir-review persona), cascaded self-verification pass

I've now verified every claim against the actual code. Let me do my final cascade pass by confirming the two remaining structural points: that configured_adapter in the gate reads exactly what delivery uses (no bypass), and that no migration seeds dev_mailbox_enabled to a conflicting value.

I confirmed:

• The gate's configured_adapter(__MODULE__) reads PhoenixKit.Config.get(__MODULE__, [])[:adapter] — identical to what deliver_with_runtime_config/3 uses on the built-in path; for delegated mailers both read Application.get_env(get_parent_app(), mailer, [])[:adapter]. No mismatch is possible within a single delivery.
• No migration references dev_mailbox_enabled (or remember_me_enabled); both rely on the runtime default in get_boolean_setting/2, so absence → false holds for existing installs with no backfill needed.
• The route is under the /admin auth scope (admin-only), and every auth-mail caller (UserNotifier.deliver/4, send_magic_link_email/2, notifications channel, test-send) routes through the gated deliver_email/2.

Here is my consolidated review.

---

Review — PR #697 (dev-mailbox opt-in, issue #687)

Summary

The PR makes local-mailbox (Swoosh.Adapters.Local + unauthenticated /dev/mailbox) delivery opt-in, defaulting closed, so that single-use auth tokens (confirmation, reset, magic-link, org-invite) are no longer routed to a page any visitor can read. The mechanism is sound and well-targeted:

• mailer_local?/0 now derives from Mailer.resolved_send_path/0, which mirrors deliver_email/2's actual routing (default integration → delegated host mailer → built-in). This correctly kills both the false-negative (delegated Local host mailer) and the false-positive (installer-written Local block coexisting with a real delegated mailer) from Dev installs can route auth mail into an unauthenticated /dev/mailbox — detect it and offer the integration send path instead #687.
• The delivery gate lives in deliver_via_configured_mailer/2 — the exact function reached only when there is no integration path — and reads configured_adapter(mailer) from the same source the real delivery uses, so there is no steady-state scenario where the gate and the actual hand-off disagree.
• Suppression returns beforeintercept_and_offer_queue/handle_after_send, so a never-delivered message is never recorded as sent. Correct.
• Dead-database safety is consistent everywhere it matters: mailer_local?/0 (rescue+catch → false), DevNotice.mailbox_enabled?/0 (same), and the gate's dev_mailbox_enabled?/0 all degrade to "closed" because get_boolean_setting/2 → get_setting_cached/2 returns the default on an unreachable pool.
• All token-bearing mail routes through the gate: every UserNotifier function goes through the private deliver/4 → Mailer.deliver_email/2, as do send_magic_link_email/2, the notifications email channel, and the admin test-send.

I found no critical, major, or minor behavioral defect. The findings below are a documentation gap and two low-severity observations.

Findings

MINOR — deliver_email/2 moduledoc does not document the new suppressed return shape

lib/phoenix_kit/mailer.ex:246 (moduledoc) vs. the new branch at lib/phoenix_kit/mailer.ex:256-262.

The "Return shape" section of deliver_email/2 documents {:ok, %{id: …}} and {:ok, %{id: ref, queued: true}}, but the PR introduces a third live shape: {:ok, %{id: "dev-mailbox-suppressed", suppressed: true}}. Callers in this codebase all match {:ok, _} (verified: UserNotifier.deliver/4, notifications/channels/email.ex:59, email_sending.ex:162), so nothing breaks today. But deliver_email/2 is a documented public API of a published library, and a caller that inspects metadata (e.g. treating queued/id as authoritative) now has an undocumented case that looks like success for a message that was never handed to an adapter. Suggested fix: add a one-line note to the "Return shape" section describing the suppressed: true case and when it occurs.

OBSERVATION (no fix required) — dormant toggle state after a default integration is added then removed

If an operator enables dev_mailbox_enabled, then later configures a default send integration, mailer_local?/0 becomes false, the admin section hides, and the toggle is left set to "true" in the DB. If the integration is subsequently removed, the section reappears with the toggle still on, and Local delivery resumes to the mailbox without a fresh confirmation. This preserves admin intent (reasonable for a dev-only control), and it can only occur on a Swoosh.Adapters.Local install, so it is not a vulnerability — flagged only so it's a conscious choice rather than an accident.

OBSERVATION (no fix required) — mailer_local?/0 is no longer a pure config read

mailer_local?/0 previously did one Application.get_env/3; it now resolves the integration path, which (only when default_email_integration_uuid is set) performs an Integrations.connected?/1 lookup. Its sole public-page caller is DevNotice (login/register/forgot-password renders), which is low-frequency, so the cost is negligible. Noting it because the function's contract silently widened, and a future caller assuming "cheap config read" would be surprised. No change needed for the current callers.

What I verified explicitly (not findings — confidence checks)

• No gate bypass via the integration path:deliver_via_integration/3 is reached only when default_send_integration_uuid/0 returns {:ok, _}, and integrations resolve to SES/SMTP/Brevo (never Local); mailer_local?/0 returns false on that branch, so UI and delivery agree.
• Default-off holds without a row:get_boolean_setting("dev_mailbox_enabled", false) returns false on row-absence, and no migration seeds the key, so existing installs are closed with no backfill. test/integration/email/dev_mailbox_gate_test.exs:72 pins "false" in get_defaults/0.
• Toggle works both directions:Core.Checkbox emits the hidden value="false" fallback (checkbox.ex:105), so unchecking submits "false"; the handler's if enabled == "true" (email_sending.ex:117) handles both states.
• Test isolation: both integration tests are async: false, restore the mutated :phoenix_kit mailer env, and PhoenixKit.Cache.invalidate(:settings, "dev_mailbox_enabled") in on_exit closes the non-transactional ETS cache leak the previous round fixed.
• Suppressed result is safe for callers: it short-circuits before tracking, and UserNotifier.deliver/4's with {:ok, _metadata} <- treats it as success — the correct dev semantic ("send didn't error; token is in the server log").

Verdict

APPROVE — the fix correctly and completely closes #687 with no behavioral defect; the only actionable item is adding the suppressed-return shape to the deliver_email/2 moduledoc.

[timujinne]

The round-2 actionable item landed: 0bb52401 documents the {:ok, %{id: "dev-mailbox-suppressed", suppressed: true}} return in the deliver_email/2 "Return shape" section. The two observations stay as recorded — the dormant-toggle behavior is a deliberate choice (admin intent survives an integration round-trip), and mailer_local?/0's widened contract is documented in its moduledoc.

[timujinne]

Added a second core security fix to this branch (58826b0e) — surfaced by the same production-surface review that produced the mailbox work, and the more urgent one because it's a live unauthenticated write, not a dev-only read.

POST /api/upload was an anonymous write with an attacker-controlled owner. The route sits in the [:browser, :phoenix_kit_auto_setup] scope (fetches a user, never requires one), and get_current_user_uuid/2 fell through to params["user_uuid"] under the comment "Verify admin permission here if needed" — the check was never written. An anonymous client could attribute a 100 MB upload, and the ProcessFileJob it enqueues, to any account.

GET /api/files/:uuid/info handed out capability URLs. Same scope; for any file uuid it returned metadata plus freshly-minted URLSigner URLs for every variant, defeating the signing scheme outright, and its 200-vs-404 split was a file-existence oracle.

Fixes:

• Upload authorizes before touching the body: unauthenticated → 401; the user_uuid override is honored only for an admin (Scope.can_access_admin_area?), otherwise the upload is attributed to the uploader; the action is rate-limited per account (30/min, new RateLimiter.check_upload_rate_limit/1).
• File-info requires auth and scopes to the owner (or admin); a foreign or missing file returns an identical :not_found, so it is no longer an oracle.
• No in-tree caller of either endpoint exists (checked across core + app + publishing + catalogue), so requiring auth breaks nothing.

Tests: storage_api_authz_test.exs covers all branches (anonymous-with-user_uuid → refused; non-admin override ignored; admin override honored; owner/non-owner/admin file reads). mix precommit green; full suite unaffected.

Deliberately not in this PR (need their own design, not a rushed graft): widening the 16-bit URLSigner token / adding expiry (breaks already-issued URLs — needs a transition), the unauthenticated POST /context/:id session write, and the magic-link IP-rate-limit gap. Happy to split the storage commit into its own PR if you'd rather keep this one strictly about the mailbox — say the word.

[timujinne]

Final pre-publication review — Kimi K3 (elixir-review persona), verified against 58826b0e

Independent second-opinion review, given full context of the two-part scope and the earlier fixed findings. Reproduced verbatim.

All claims verified against the branch code (HEAD = 58826b0e, merge-base with main = 49bdccd9). I read every changed function plus the surrounding infrastructure (router pipelines, Scope, Settings cache guards, Swoosh storage API, queue-worker path) and ran what's runnable without a DB. Findings:

Findings

MEDIUM — Rate limit is keyed on the attributed owner, not the uploader

lib/phoenix_kit_web/controllers/upload_controller.ex:64-65

with{:ok,user_uuid}<-resolve_upload_user(...),:ok<-RateLimiter.check_upload_rate_limit(user_uuid),

resolve_upload_user/2 returns the params["user_uuid"]override for an admin-capable caller, and that override is what the rate limiter keys on. Concrete failure scenario: a user holding any single permission (see next finding) loops POST /api/upload rotating user_uuid=victim-1…victim-N. Each victim key gets a fresh 30/min window, the attacker's own account key is never touched, and 100 MB per request lands on disk attributed to victims (plus an Oban ProcessFileJob each) at unbounded aggregate rate. The limiter's stated purpose — "bounds storage-exhaustion and Oban-queue abuse by an authenticated user" — is defeated for exactly the only callers who can pass an override. Fix: rate-limit the authenticated actor's uuid (resolve actor first, limit actor, then resolve owner attribution).

MEDIUM — "admin" here means any permission holder, not Admin/Owner

lib/phoenix_kit_web/controllers/upload_controller.ex:181 and lib/phoenix_kit_web/controllers/file_controller.ex:152

Both gates use Scope.can_access_admin_area?(Scope.for_user(user)), which is true for Owner, Admin, or any user whose permission set is non-empty (scope.ex:341-347: MapSet.size(perms) > 0). Consequences: a host that grants a user one modest permission (e.g. a single module key) silently grants them (a) the ability to attribute uploads — 100 MB + a processing job — to any account, and (b) read access to every file's metadata plus freshly-minted signed variant URLs via /api/files/:uuid/info. The docs/comments say "admin", which is misleading; for a published library this is a latent trap (a consumer will read "admin only" and not audit further). Either gate on the Admin/Owner roles explicitly, or rename/document the semantics in the endpoint docs. Not necessarily a blocker if deliberate — but it must be a decision, not an accident.

LOW — Stale security-contract doc now contradicts the code

lib/modules/storage/storage.ex:2472 — the store_file/2 moduledoc still states "/api/files/:uuid/info is unauthenticated and hands out a valid signed URL for any uuid it is given." This PR changes exactly that contract; the doc (which consumers read to decide what the signing scheme provides) now describes the pre-fix behavior.

LOW — The headline resolution tests are excluded in DB-less runs

test/phoenix_kit/mailer_test.exs:556-625 — the new resolved_send_path/0 and mailer_local?/0 describe lives inside PhoenixKit.MailerTest, which uses PhoenixKit.DataCase → auto-tagged :integration → excluded when postgres is unreachable (and the run still exits 0, per AGENTS.md). The logic under test doesn't need the DB (default_send_integration_uuid short-circuits/rescues to :error). Part A's core fix ships with zero executable coverage in a DB-less environment. Move that describe to a plain ExUnit.Case module. (Verified locally: only the 2 DevNoticeTest tests ran; all 32 DataCase tests were excluded.)

LOW — Behavior-breaking changes with no CHANGELOG entry on the branch

The branch contains no CHANGELOG.md/mix.exs changes. Both parts are breaking for consumers: dev installs on Swoosh.Adapters.Local stop receiving mail in /dev/mailbox after upgrade (log-only), and the two storage endpoints now 401 anonymous callers. If CHANGELOG is written at release time per repo convention, fine — but these two items must be in it explicitly.

NIT — Suppressed path skips check_recipient_allowed/1

lib/phoenix_kit/mailer.ex:260-273 returns before the with chain, so a blocklisted recipient gets {:ok, %{suppressed: true}} instead of {:error, {:blocked, _}}. Dev-only, defensible, but it means blocklist behavior can't be exercised against the Local adapter with the gate off. Worth one comment line, not code.

NIT — Comment overstates the protection

upload_controller.ex:61-63: "refused without the server hashing, validating or storing 100 MB on its behalf" — Plug has already parsed the multipart body and spooled the temp file before the controller runs; the ordering saves hashing/storage/enqueue, not the bandwidth/temp-write. The ordering itself is correct (authz → limit → extract → validate).

Verified sound (the previously-fixed items and the main claims)

• Resolution ≡ delivery: gate source (configured_adapter/1) matches the delivery source for both built-in (PhoenixKit.Config.get(mailer, [])) and delegated (parent-app env, SES special-case) paths; the integration path can never resolve to Local; the queue worker re-enters via deliver_email so the gate re-fires at dequeue time. No bypass found.
• Dead-DB safety: get_boolean_setting → get_setting_cached has rescue andcatch :exit; mailer_local?/0 and DevNotice.mailbox_enabled?/0 add their own guards. The mailer adds no new unguarded Settings read (default_send_integration_uuid already read Settings pre-PR).
• Suppressed return shape: all 5 in-tree deliver_email callers match {:ok, _} — none inspects metadata fields. Safe.
• Owner-scoping on file-info: nil and foreign file both → identical :not_found (no oracle beyond a negligible timing difference); Storage.get_file/1 returns struct-or-nil and the function heads match that; malformed-UUID CastError behavior is unchanged from the old private get_file/1.
• Order in create: authz → rate-limit → extract → validate → store is correct; {:error, :no_file} clause is new but previously a missing file fell into the 500 catch-all, so this is an improvement, not a regression.
• Test-infra claims: Cache.invalidate/2 exists with rescue/catch; Swoosh Memory.start/1/stop/0/delete_all/0 match the test usage; ConnCase aliases Repo and provides log_in_user/2; the Owner-insert with on_conflict: :nothing composes with first-user auto-Owner; the storage_api_authz_test seed user correctly defeats auto-Owner.
• No legitimate in-tree flow broken: zero functional callers of either endpoint in core (only docs/sitemap-exclusion references).
• Compile passes with --warnings-as-errors; the 2 runnable new tests pass.

Verdict

GO-WITH-FIXES — the two endpoint gates and the mailbox gate are correctly fail-closed, but key the upload rate limit on the authenticated actor rather than the attributed owner, and make an explicit decision (or doc fix) on "any permission holder counts as admin" for the owner-override and file-info read before publishing.

[timujinne]

Applied the review's actionable items in 48b0a456:

• MEDIUM (rate-limit keying) — the upload limit now keys on the authenticated uploader (current_user.uuid), not the resolved owner, so an admin rotating user_uuid=victim-N no longer gets a fresh window per victim.
• MEDIUM (admin semantics) — both gates (upload override, file-info cross-user read) now use Scope.system_role?/1 (strictly Owner/Admin) instead of can_access_admin_area?/1, which was also true for any single-permission holder. Added two tests proving a permission-holder-but-not-Owner/Admin user is refused both — with an inline assert can_access_admin_area? / refute system_role? so the test pins exactly the distinction (a regression to the broad predicate goes red).
• LOW (stale doc) — store_file/2's signing-contract note updated: /api/files/:uuid/info is no longer an anonymous handout.
• LOW (DB-less coverage) — moved the resolved_send_path/0 + mailer_local?/0 tests into a plain ExUnit.Case module so Part A's core fix keeps executable coverage on a database-less run.
• Both NITs addressed (suppressed-path blocklist note; corrected the upload-ordering comment — Plug has already spooled the temp file).

CHANGELOG (LOW) is maintainer-owned in this repo, so it's left for the release cut — the two consumer-visible breaks (dev Local installs stop filling /dev/mailbox; the two storage endpoints 401 anonymous callers) belong in the 2.0.0 entry.

mix precommit green; 155 tests across the touched suites, 0 failures. This should close the GO-WITH-FIXES items.