diff --git a/internal/desktop/bind_sync.go b/internal/desktop/bind_sync.go index a779469..1ea7b13 100644 --- a/internal/desktop/bind_sync.go +++ b/internal/desktop/bind_sync.go @@ -124,10 +124,48 @@ func (t *accountTransmitter) Transmit(ctx context.Context, m outbox.Message) err if err != nil { return err } - sender := psmtp.NewSender(cfg, psmtp.WithLogger(t.app.log)) + sender := psmtp.NewSender(cfg, + psmtp.WithLogger(t.app.log), + psmtp.WithSentAppender(func(raw []byte) (string, error) { + return t.app.appendToSent(*account, raw) + }), + ) return sender.Transmit(ctx, m) } +// appendToSent puts a copy of a message that has just been sent in the +// account's Sent folder, so it is there in webmail and in every other client, +// not only in Pelton (#451). +// +// It opens its own imap session rather than borrowing the sync engine's. That +// one is parked in IDLE almost all the time, and interrupting it to append +// would cost more than a second connection does for something that happens +// once per sent message. +// +// The copy is appended to the server and not written locally: the folder's next +// sync pulls it down like any other message, which keeps one path for how mail +// arrives in the store. +func (a *App) appendToSent(account storage.Account, raw []byte) (string, error) { + cfg, err := a.resolveIMAP(account) + if err != nil { + return "", err + } + // every other imap session in the package takes this, so a send during a + // sync waits its turn rather than opening a second login for the account. + syncMu.Lock() + defer syncMu.Unlock() + client, err := a.connectIMAP(cfg) + if err != nil { + return "", err + } + defer func() { _ = client.Close() }() + if err := client.Login(); err != nil { + return "", err + } + defer func() { _ = client.Logout() }() + return client.AppendToSent(raw) +} + // runInitialSyncAndIdle syncs every account once, then parks each on idle. func (a *App) runInitialSyncAndIdle() { accounts, err := a.store.ListAccounts(a.ctx) diff --git a/internal/desktop/bind_sync_sent_test.go b/internal/desktop/bind_sync_sent_test.go new file mode 100644 index 0000000..29c20c6 --- /dev/null +++ b/internal/desktop/bind_sync_sent_test.go @@ -0,0 +1,198 @@ +package desktop + +import ( + "bufio" + "context" + "crypto/tls" + "errors" + "log/slog" + "net" + "net/http" + "net/http/httptest" + "path/filepath" + "strconv" + "strings" + "testing" + + "github.com/peltonapp/Pelton/internal/certtrust" + "github.com/peltonapp/Pelton/internal/credentials" + pimap "github.com/peltonapp/Pelton/internal/imap" + "github.com/peltonapp/Pelton/internal/outbox" + "github.com/peltonapp/Pelton/internal/storage" +) + +// submissionServer is enough of a submission server to carry one message: +// greeting, EHLO, AUTH, MAIL, RCPT, DATA, QUIT. It runs on implicit TLS with +// httptest's self-signed certificate, and returns its port and the certificate +// fingerprint so the account under test can pin it. +// +// The whole point of these tests is the step that happens after a send +// succeeds, so the send has to actually succeed. +func submissionServer(t *testing.T) (port int, fingerprint string) { + t.Helper() + certServer := httptest.NewTLSServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + certServer.Close() + tlsConfig := certServer.TLS.Clone() + tlsConfig.NextProtos = nil + + ln, err := tls.Listen("tcp", "127.0.0.1:0", tlsConfig) + if err != nil { + t.Fatalf("listen: %v", err) + } + t.Cleanup(func() { _ = ln.Close() }) + go func() { + for { + conn, err := ln.Accept() + if err != nil { + return + } + go speakSubmission(conn) + } + }() + + _, p, _ := net.SplitHostPort(ln.Addr().String()) + n, _ := strconv.Atoi(p) + return n, certtrust.Fingerprint(certServer.Certificate()) +} + +func speakSubmission(conn net.Conn) { + defer func() { _ = conn.Close() }() + r := bufio.NewReader(conn) + write := func(s string) { _, _ = conn.Write([]byte(s + "\r\n")) } + + write("220 localhost ESMTP") + for { + line, err := r.ReadString('\n') + if err != nil { + return + } + cmd := strings.ToUpper(strings.TrimSpace(line)) + switch { + case strings.HasPrefix(cmd, "EHLO"), strings.HasPrefix(cmd, "HELO"): + write("250-localhost") + write("250 AUTH PLAIN") + case strings.HasPrefix(cmd, "AUTH"): + write("235 2.7.0 authenticated") + case strings.HasPrefix(cmd, "DATA"): + write("354 go ahead") + // read to the end-of-data marker and accept whatever arrived. + for { + body, err := r.ReadString('\n') + if err != nil { + return + } + if strings.TrimSpace(body) == "." { + break + } + } + write("250 2.0.0 queued") + case strings.HasPrefix(cmd, "QUIT"): + write("221 bye") + return + default: + write("250 2.0.0 ok") + } + } +} + +// sendingAccount builds an app and an account whose smtp points at a live test +// server, with the server's certificate pinned and a password stored, and +// points the imap side at client. +func sendingAccount(t *testing.T, client *fakeIMAP) (*App, storage.Account) { + t.Helper() + ctx, stopBackground := testContext(t) + db, err := storage.Open(filepath.Join(t.TempDir(), "test.db")) + if err != nil { + t.Fatalf("open: %v", err) + } + t.Cleanup(func() { db.Close() }) + t.Cleanup(stopBackground) + if err := db.RunMigrations(ctx); err != nil { + t.Fatalf("migrate: %v", err) + } + + port, fingerprint := submissionServer(t) + account := storage.Account{ + Email: "me@example.com", + Username: "me@example.com", + SMTPHost: "127.0.0.1", + SMTPPort: port, + SMTPTLS: "ssl", + IMAPHost: "127.0.0.1", + IMAPPort: 993, + IMAPTLS: "ssl", + TrustedCerts: []string{fingerprint}, + } + id, err := db.CreateAccount(ctx, &account) + if err != nil { + t.Fatalf("create account: %v", err) + } + account.ID = id + + if err := credentials.Store(id, credentials.Secret{ + Method: credentials.MethodPassword, + Password: "hunter2", + }); err != nil { + t.Fatalf("store credentials: %v", err) + } + t.Cleanup(func() { _ = credentials.Delete(id) }) + + app := &App{ctx: ctx, store: db, log: slog.New(slog.DiscardHandler)} + app.newIMAPClient = func(pimap.Config) (mailClient, error) { return client, nil } + return app, account +} + +// The bug this exists for (#451): the append-to-Sent step was wired through an +// option the one place that builds the sender never passed, so every message +// sent correctly and no copy was ever written. Nothing failed, because the +// missing appender is a debug log and a send that returns nil. +// +// So the assertion is on the transmit path as the outbox worker drives it, not +// on the helper underneath. A test of the helper alone would still have passed +// the whole time the feature was dead. +func TestTransmitAppendsACopyToSent(t *testing.T) { + client := &fakeIMAP{} + app, account := sendingAccount(t, client) + + raw := []byte("From: me@example.com\r\nTo: you@example.com\r\nSubject: hi\r\n\r\nbody\r\n") + transmitter := &accountTransmitter{app: app} + err := transmitter.Transmit(context.Background(), outbox.Message{ + AccountID: account.ID, + EnvelopeFrom: "me@example.com", + Recipients: []string{"you@example.com"}, + Raw: raw, + }) + if err != nil { + t.Fatalf("Transmit: %v", err) + } + + if len(client.appended) != 1 { + t.Fatalf("appended %d messages to Sent, want 1", len(client.appended)) + } + if string(client.appended[0]) != string(raw) { + t.Errorf("appended %q, want the message that was sent", client.appended[0]) + } + if !client.loggedOut { + t.Error("the imap session opened for the append was left open") + } +} + +// The message has already left the building by the time the append runs, so a +// server that refuses it must not turn a delivered message into a failed one. +// The copy is lost, which is worth a log, but reporting failure would have the +// outbox retry and send it twice. +func TestTransmitSucceedsWhenTheSentCopyFails(t *testing.T) { + client := &fakeIMAP{failAppend: errors.New("permission denied")} + app, account := sendingAccount(t, client) + + transmitter := &accountTransmitter{app: app} + err := transmitter.Transmit(context.Background(), outbox.Message{ + AccountID: account.ID, + EnvelopeFrom: "me@example.com", + Recipients: []string{"you@example.com"}, + Raw: []byte("Subject: hi\r\n\r\nbody\r\n"), + }) + if err != nil { + t.Errorf("Transmit reported failure for a message that was sent: %v", err) + } +} diff --git a/internal/desktop/imapseam.go b/internal/desktop/imapseam.go index 2fc7e97..adb8a83 100644 --- a/internal/desktop/imapseam.go +++ b/internal/desktop/imapseam.go @@ -31,6 +31,10 @@ type mailClient interface { RenameFolder(path, newPath string) error DeleteFolder(path string) error + // AppendToSent puts a copy of a message that has just been sent in the + // account's Sent folder, and reports which folder it used. + AppendToSent(raw []byte) (string, error) + FetchMessage(uid imap.UID) (*pimap.Message, error) FetchMessages(uids []imap.UID, fn func(uid imap.UID, msg *pimap.Message, err error) error) error FetchRawMessage(uid imap.UID) ([]byte, error) diff --git a/internal/desktop/imapseam_test.go b/internal/desktop/imapseam_test.go index 460e5ed..f5758c7 100644 --- a/internal/desktop/imapseam_test.go +++ b/internal/desktop/imapseam_test.go @@ -43,6 +43,11 @@ type fakeIMAP struct { // refusing the next login. loggedOut bool + // appended is every message handed to AppendToSent, in order, and + // failAppend makes the append fail so a test can check a send still + // succeeds without one. + appended [][]byte + failAppend error // failMove makes MOVE fail, for the paths that have to leave the local cache // alone when the server says no. failMove error @@ -53,6 +58,14 @@ type movedMessage struct { dest string } +func (f *fakeIMAP) AppendToSent(raw []byte) (string, error) { + if f.failAppend != nil { + return "", f.failAppend + } + f.appended = append(f.appended, raw) + return "Sent", nil +} + func (f *fakeIMAP) Login() error { return nil } func (f *fakeIMAP) Logout() error { f.loggedOut = true; return nil } func (f *fakeIMAP) Close() error { return nil }