refactor(admin): remove dead SMTP admin form + clean up stale rows (LC-77-SMTP-SEAL, #200) #204

Merged

longjacksonle merged 1 commit from feat/lc-77-smtp-seal into main

2026-05-25 22:38:16 +02:00

longjacksonle commented

2026-05-25 22:29:51 +02:00

Owner

Resolves #200 (LC-77-SMTP-SEAL).

Investigation finding rerouted the scope

The original ticket plan was "migrate SMTP password from plaintext to AES-256-GCM-sealed storage" mirroring the LC-77 imap_inbox_config pattern. The first thing this branch did was grep for actual readers of the password column. Result:

routes::admin::post_settings writes smtp_pass (line 474).
routes::admin::get_settings reads it back, just to re-display in the admin form.
Nothing else reads it. mail::Mailer::from_env (line 29) reads SMTP config exclusively from the SMTP_* env vars, as documented in its 14-line module doc-comment.

The settings.db SMTP rows are dead storage. The plaintext password sitting there was the only real consequence of the form; sealing the column would have added crypto plumbing around a value no code consults.

The real fix is to stop populating the column. This PR removes the SMTP admin form, drops the dead model, and ships a migration that DELETEs the legacy smtp_* rows. Existing operator deployments get their stale plaintext password dropped at next startup, and new deployments never accumulate the row in the first place.

What changed

routes/admin.rs: SettingsForm reduced to an empty struct (the form has no inputs anymore); get_settings stops reading smtp_*; SettingsPage construction drops the smtp_* fields; post_settings becomes a no-op redirect retained so a stale browser POST doesn't 404. Comment block at the struct site names the finding so a future contributor doesn't reintroduce the pattern.
views/admin.rs: SettingsPage struct drops smtp_host / smtp_port / smtp_user / smtp_from / saved.
templates/admin/settings.html: SMTP form section replaced with an informational banner: "SMTP is configured via environment variables, not the admin UI. Set SMTP_HOST ... and restart. See README.md."
models/settings.rs: deleted. SiteSettings was defined and re-exported but never constructed or read anywhere - pure dead code unrelated to this PR but unrelated cleanup it costs zero to fold in.
models/mod.rs: drop the SiteSettings re-export.
migrations/settings/0006_drop_smtp_settings.sql (new): idempotent DELETE FROM settings WHERE key IN ('smtp_host', 'smtp_port', 'smtp_user', 'smtp_pass', 'smtp_from').

Test plan

New test tests/db_smtp_settings_cleanup.rs::migration_drops_pre_existing_smtp_settings_rows: inserts the five legacy keys, runs the migration SQL, asserts gone; also asserts unrelated rows survive; also asserts idempotent re-run.
Full server test suite green under both --features standalone (default) and --no-default-features --features saas.
cargo clippy --tests -- -D warnings clean.
cargo fmt --check clean.
Test drift cleanup: 7 array-form test files get the new settings/0006 migration appended (CLAUDE.md test-maintenance category 2). No drift expected from this PR going forward; future migrations follow the same pattern.

Anti-scope

Does not change the SMTP_* env var loader in mail.rs. That is the actual SMTP config surface and stays as-is.
Does not add a "test SMTP" button or any new admin UI affordance for outbound mail. The README already documents the env vars; the admin UI now points at them rather than pretending to be the config surface.
Does not migrate any other column to sealed storage. VAPID and IMAP-poll already use the sealed pattern. SMTP doesn't need it because there is no longer any SMTP storage to seal.

Closes

#200 (LC-77-SMTP-SEAL).

Resolves #200 (LC-77-SMTP-SEAL). ## Investigation finding rerouted the scope The original ticket plan was "migrate SMTP password from plaintext to AES-256-GCM-sealed storage" mirroring the LC-77 `imap_inbox_config` pattern. The first thing this branch did was grep for actual readers of the password column. Result: - `routes::admin::post_settings` writes `smtp_pass` (line 474). - `routes::admin::get_settings` reads it back, just to re-display in the admin form. - **Nothing else reads it.** `mail::Mailer::from_env` (line 29) reads SMTP config exclusively from the `SMTP_*` env vars, as documented in its 14-line module doc-comment. The `settings.db` SMTP rows are dead storage. The plaintext password sitting there was the only real consequence of the form; sealing the column would have added crypto plumbing around a value no code consults. The real fix is to stop populating the column. This PR removes the SMTP admin form, drops the dead model, and ships a migration that DELETEs the legacy `smtp_*` rows. Existing operator deployments get their stale plaintext password dropped at next startup, and new deployments never accumulate the row in the first place. ## What changed - `routes/admin.rs`: `SettingsForm` reduced to an empty struct (the form has no inputs anymore); `get_settings` stops reading `smtp_*`; `SettingsPage` construction drops the `smtp_*` fields; `post_settings` becomes a no-op redirect retained so a stale browser POST doesn't 404. Comment block at the struct site names the finding so a future contributor doesn't reintroduce the pattern. - `views/admin.rs`: `SettingsPage` struct drops `smtp_host` / `smtp_port` / `smtp_user` / `smtp_from` / `saved`. - `templates/admin/settings.html`: SMTP form section replaced with an informational banner: "SMTP is configured via environment variables, not the admin UI. Set `SMTP_HOST` ... and restart. See `README.md`." - `models/settings.rs`: deleted. `SiteSettings` was defined and re-exported but never constructed or read anywhere - pure dead code unrelated to this PR but unrelated cleanup it costs zero to fold in. - `models/mod.rs`: drop the `SiteSettings` re-export. - `migrations/settings/0006_drop_smtp_settings.sql` (new): idempotent `DELETE FROM settings WHERE key IN ('smtp_host', 'smtp_port', 'smtp_user', 'smtp_pass', 'smtp_from')`. ## Test plan - [x] New test `tests/db_smtp_settings_cleanup.rs::migration_drops_pre_existing_smtp_settings_rows`: inserts the five legacy keys, runs the migration SQL, asserts gone; also asserts unrelated rows survive; also asserts idempotent re-run. - [x] Full server test suite green under both `--features standalone` (default) and `--no-default-features --features saas`. - [x] `cargo clippy --tests -- -D warnings` clean. - [x] `cargo fmt --check` clean. - [x] Test drift cleanup: 7 array-form test files get the new `settings/0006` migration appended (CLAUDE.md test-maintenance category 2). No drift expected from this PR going forward; future migrations follow the same pattern. ## Anti-scope - Does not change the `SMTP_*` env var loader in `mail.rs`. That is the actual SMTP config surface and stays as-is. - Does not add a "test SMTP" button or any new admin UI affordance for outbound mail. The README already documents the env vars; the admin UI now points at them rather than pretending to be the config surface. - Does not migrate any other column to sealed storage. VAPID and IMAP-poll already use the sealed pattern. SMTP doesn't need it because there is no longer any SMTP storage to seal. ## Closes #200 (LC-77-SMTP-SEAL).

longjacksonle added 1 commit

2026-05-25 22:29:51 +02:00

refactor(admin): remove dead SMTP admin form + clean up stale settings rows (LC-77-SMTP-SEAL)

check-secrets / Nosey parker (push) Successful in 3s

Details

check-secrets / Kingfisher (push) Successful in 4s

Details

check-secrets / TruffleHog (push) Successful in 5s

Details

check-secrets / TruffleHog (pull_request) Successful in 7s

Details

check-secrets / Nosey parker (pull_request) Successful in 7s

Details

check-secrets / Kingfisher (pull_request) Successful in 10s

Details

Check / clippy + fmt + tests (pull_request) Successful in 4m36s

Details

Create release / Create release from merged PR (pull_request) Has been skipped

Details

7d071e8c7a

Investigation finding rerouted the ticket scope. The original LC-77-SMTP-SEAL plan was "migrate SMTP password from plaintext to AES-256-GCM-sealed storage" mirroring the LC-77 IMAP-config pattern. But grepping the actual code revealed `mail::Mailer::from_env` reads SMTP creds exclusively from the SMTP_* env vars; the `settings.db` rows the admin form was writing have NEVER been read by the mailer. Sealing a column nothing consults adds crypto plumbing around dead data.

The real fix is to stop populating the column. This commit removes the SMTP admin form, drops the dead model, and ships a migration that DELETEs any pre-existing smtp_* rows. The plaintext-leak vector goes away because the column stops being populated, AND existing operator deployments get their stale plaintext password row dropped on the next startup.

Changes:

- routes/admin.rs: SettingsForm reduced to an empty struct (form has no inputs anymore); get_settings stops reading smtp_*; SettingsPage construction drops the smtp_* fields; post_settings becomes a no-op redirect retained so a stale browser POST does not 404. A comment block names the finding so a future contributor doesn't reintroduce the pattern.
- views/admin.rs: SettingsPage struct drops smtp_host/smtp_port/smtp_user/smtp_from/saved fields.
- templates/admin/settings.html: replace the SMTP form section with an informational banner pointing at the SMTP_* env vars + README.md.
- models/settings.rs: deleted. The SiteSettings struct was defined and re-exported but never constructed or read anywhere. Pure dead code.
- models/mod.rs: drop the SiteSettings re-export + the settings module.
- migrations/settings/0006_drop_smtp_settings.sql (new): idempotent DELETE of the five legacy smtp_* keys.

New test: tests/db_smtp_settings_cleanup.rs::migration_drops_pre_existing_smtp_settings_rows. Inserts the five legacy keys, runs the migration SQL inline, asserts they are gone; also asserts unrelated rows are untouched. Idempotent re-run also asserted.

Test drift cleanup: 7 array-form test files (admin_uploads, db_vapid, push_dispatch, routes_analytics, routes_reconnect, routes_uploads, scheduled_dispatcher) get the new settings/0006 migration appended via the same Python script the LC-77 commit 2 used. Documented in CLAUDE.md as category 2 drift.

Verification: cargo check, cargo test --tests, and cargo test --no-default-features --features saas --tests all clean. cargo clippy --tests -- -D warnings clean. cargo fmt --check clean.

The investigation finding closes the LC-77-SMTP-SEAL ticket more cleanly than the original sealed-storage plan would have: there's no longer a plaintext-in-settings.db weakness because there's no longer any SMTP storage in settings.db at all. README.md already documents SMTP as env-var-only; this commit removes the admin UI scaffolding that contradicted that.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>