Skip to content

feat(email): negotiate SMTP AUTH mechanism, add Office 365 LOGIN support - #121

Open
jackyhowh wants to merge 1 commit into
bscott:mainfrom
jackyhowh:main
Open

feat(email): negotiate SMTP AUTH mechanism, add Office 365 LOGIN support#121
jackyhowh wants to merge 1 commit into
bscott:mainfrom
jackyhowh:main

Conversation

@jackyhowh

@jackyhowh jackyhowh commented Jun 12, 2026

Copy link
Copy Markdown

Summary

Go's net/smtp only ships PLAIN and CRAM-MD5. Office 365 / Outlook
(smtp.office365.com) advertises only LOGIN and XOAUTH2, and rejects an
AUTH PLAIN attempt with 504 5.7.4 Unrecognized authentication type.

SMTPAuth negotiates PLAIN or LOGIN from the server's advertised mechanism
list at handshake time, falling back to LOGIN when PLAIN isn't offered. The
LOGIN implementation refuses to send credentials over a connection that
hasn't negotiated TLS.

Changes

  • internal/service/smtp_auth.go (new): SMTPAuth with PLAIN/LOGIN
    auto-negotiation, and a loginAuth implementation of the AUTH LOGIN
    mechanism.
  • internal/service/email.go: outgoing notification emails use the
    negotiated auth instead of a hardcoded smtp.PlainAuth.
  • internal/handlers/settings.go: the SMTP connection test uses the same
    negotiated auth.
  • internal/service/smtp_auth_test.go: unit tests covering mechanism
    selection (PLAIN preferred, LOGIN fallback, default-to-PLAIN on unknown
    mechanisms) and the TLS-refusal guard.

Test plan

  • go test ./... passes
  • Verified Test Connection authenticates via Office 365 (587/STARTTLS)

Note

This PR previously bundled a "real test email" feature and multi-recipient
support along with this LOGIN auth fix. Both have been pulled out: I found a
bug in the multi-recipient parsing after this was originally submitted, and
want to fix that properly before resubmitting it (along with the test-email
change) as its own PR. This PR is now scoped to the Office 365 LOGIN auth
negotiation only.

@bscott bscott self-assigned this Jun 23, 2026
@bscott

bscott commented Jul 2, 2026

Copy link
Copy Markdown
Owner

Thanks for this contribution — the SMTP work is genuinely solid. I reviewed it in depth: the Office 365 LOGIN auth is correctly implemented (refuses non-TLS, negotiates post-STARTTLS), the multi-recipient parsing is safe against header injection, and sending a real test email is a clear UX improvement over the old dial-only check.

That said, this PR bundles three unrelated changes, and I'd like to merge them on their own merits. Could you split it up?

  1. SMTP improvements (test email, LOGIN auth, multi-recipient) — ready to merge essentially as-is once separated. Two small nits if you're touching it anyway: the 15s timeout covers dial only (a conn.SetDeadline would also cover a stalling server), and the button label still says "Test Connection" while it now emails every configured recipient.
  2. Logout button — I'm open to this, but it isn't mentioned anywhere in the PR description, so I'd like it as its own PR with the reasoning. What motivated it for your setup? Related: I'm considering idle-session timeout as a complement (sessions currently hard-expire at 24h / 30d with remember-me, but there's no inactivity logout) — happy to discuss the right shape in that PR. One note from review: the existing logout route is a GET, so a nav link to it is CSRF-triggerable; a POST form would be the better shape.
  3. CLAUDE.md expansion — nice writeup and largely accurate, but I'd like to review doc changes to the maintainer instruction file separately. Please drop it from the SMTP PR; feel free to open it as a docs PR.

Also, for transparency: the claude-review CI failure on this PR isn't your code — GitHub doesn't expose repo secrets to workflows triggered from forks, so the review action had no API key. test-build passed fine.

A tip for the split: branch each piece off current main rather than PRing from your fork's main — it keeps each PR reviewable in isolation. Looking forward to merging the SMTP piece!

Go's net/smtp only ships PLAIN and CRAM-MD5. Office 365 / Outlook
(smtp.office365.com) advertises only LOGIN and XOAUTH2, and rejects an
AUTH PLAIN attempt with "504 5.7.4 Unrecognized authentication type".

SMTPAuth negotiates PLAIN or LOGIN from the server's advertised
mechanism list at handshake time, falling back to LOGIN when PLAIN
isn't offered. The LOGIN implementation refuses to send credentials
over a connection that hasn't negotiated TLS.

Wires the negotiated auth into both the SMTP connection test and
outgoing notification emails, replacing the hardcoded PlainAuth call
in each.
@jackyhowh jackyhowh changed the title feat: SMTP test sends a real email, Office 365 LOGIN auth, and multiple recipients feat(email): negotiate SMTP AUTH mechanism, add Office 365 LOGIN support Jul 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants