From a0f3856e4005095378e6b1155945957272f9822c Mon Sep 17 00:00:00 2001 From: Mechiel Lukkien Date: Fri, 22 Sep 2023 15:43:25 +0200 Subject: [PATCH] when moving a message out of a Rejects mailbox, mark the message as "not seen" so stands out in the destination mailbox (e.g. inbox) we set the flag both for move in imap and in webmail. this also ensures the "MailboxDestinedID", used for per-mailbox reputation analysis, is set in more reject-situations. before this change, some rejects (such as based on DMARC reject) wouldn't result in reputation being used after having been moved the message out of the rejects mailbox. in the future, we need more tests for scenario's like this... for issue #63 reported by x8x may also help with issue #64 --- imapserver/server.go | 6 ++-- smtpserver/analyze.go | 64 +++++++++++++++++++++++++++++-------------- webmail/api.go | 1 + 3 files changed, 48 insertions(+), 23 deletions(-) diff --git a/imapserver/server.go b/imapserver/server.go index 6381a37..85385e6 100644 --- a/imapserver/server.go +++ b/imapserver/server.go @@ -3378,9 +3378,7 @@ func (c *conn) cmdxMove(isUID bool, tag, cmd string, p *parser) { xserverErrorf("internal error: got uid %d, expected %d, for index %d", m.UID, uids[i], i) } - mc := m.MailboxCounts() - mbSrc.Sub(mc) - mbDst.Add(mc) + mbSrc.Sub(m.MailboxCounts()) // Copy of message record that we'll insert when UID is freed up. om := *m @@ -3394,7 +3392,9 @@ func (c *conn) cmdxMove(isUID bool, tag, cmd string, p *parser) { // is used for reputation calculation during future deliveries. m.MailboxOrigID = m.MailboxDestinedID m.IsReject = false + m.Seen = false } + mbDst.Add(m.MailboxCounts()) m.UID = uidnext m.ModSeq = modseq m.JunkFlagsForMailbox(mbDst, conf) diff --git a/smtpserver/analyze.go b/smtpserver/analyze.go index 294e010..94988a7 100644 --- a/smtpserver/analyze.go +++ b/smtpserver/analyze.go @@ -116,7 +116,49 @@ func analyze(ctx context.Context, log *mlog.Log, resolver dns.Resolver, d delive log.Info("forwarded message, clearing identifying signals of forwarding mail server") } + assignMailbox := func(tx *bstore.Tx) error { + // Set message MailboxID to which mail will be delivered. Reputation is + // per-mailbox. If referenced mailbox is not found (e.g. does not yet exist), we + // can still determine a reputation because we also base it on outgoing + // messages and those are account-global. + mb, err := d.acc.MailboxFind(tx, mailbox) + if err != nil { + return fmt.Errorf("finding destination mailbox: %w", err) + } + if mb != nil { + // We want to deliver to mb.ID, but this message may be rejected and sent to the + // Rejects mailbox instead, with MailboxID overwritten. Record the ID in + // MailboxDestinedID too. If the message is later moved out of the Rejects mailbox, + // we'll adjust the MailboxOrigID so it gets taken into account during reputation + // calculating in future deliveries. If we end up delivering to the intended + // mailbox (i.e. not rejecting), MailboxDestinedID is cleared during delivery so we + // don't store it unnecessarily. + d.m.MailboxID = mb.ID + d.m.MailboxDestinedID = mb.ID + } else { + log.Debug("mailbox not found in database", mlog.Field("mailbox", mailbox)) + } + return nil + } + reject := func(code int, secode string, errmsg string, err error, reason string) analysis { + // We may have set MailboxDestinedID below already while we had a transaction. If + // not, do it now. This makes it possible to use the per-mailbox reputation when a + // user moves the message out of the Rejects mailbox to the intended mailbox + // (typically Inbox). + if d.m.MailboxDestinedID == 0 { + var mberr error + d.acc.WithRLock(func() { + mberr = d.acc.DB.Read(ctx, func(tx *bstore.Tx) error { + return assignMailbox(tx) + }) + }) + if mberr != nil { + return analysis{false, mailbox, smtp.C451LocalErr, smtp.SeSys3Other0, false, "error processing", err, nil, nil, reasonReputationError} + } + d.m.MailboxID = 0 // We plan to reject, no need to set intended MailboxID. + } + accept := false if rs != nil && rs.AcceptRejectsToMailbox != "" { accept = true @@ -203,26 +245,8 @@ func analyze(ctx context.Context, log *mlog.Log, resolver dns.Resolver, d delive var err error d.acc.WithRLock(func() { err = d.acc.DB.Read(ctx, func(tx *bstore.Tx) error { - // Set message MailboxID to which mail will be delivered. Reputation is - // per-mailbox. If referenced mailbox is not found (e.g. does not yet exist), we - // can still determine a reputation because we also base it on outgoing - // messages and those are account-global. - mb, err := d.acc.MailboxFind(tx, mailbox) - if err != nil { - return fmt.Errorf("finding destination mailbox: %w", err) - } - if mb != nil { - // We want to deliver to mb.ID, but this message may be rejected and sent to the - // Rejects mailbox instead, with MailboxID overwritten. Record the ID in - // MailboxDestinedID too. If the message is later moved out of the Rejects mailbox, - // we'll adjust the MailboxOrigID so it gets taken into account during reputation - // calculating in future deliveries. If we end up delivering to the intended - // mailbox (i.e. not rejecting), MailboxDestinedID is cleared during delivery so we - // don't store it unnecessarily. - d.m.MailboxID = mb.ID - d.m.MailboxDestinedID = mb.ID - } else { - log.Debug("mailbox not found in database", mlog.Field("mailbox", mailbox)) + if err := assignMailbox(tx); err != nil { + return err } isjunk, conclusive, method, err = reputation(tx, log, d.m) diff --git a/webmail/api.go b/webmail/api.go index a0e4c84..e5f0948 100644 --- a/webmail/api.go +++ b/webmail/api.go @@ -844,6 +844,7 @@ func (Webmail) MessageMove(ctx context.Context, messageIDs []int64, mailboxID in // is used for reputation calculation during future deliveries. m.MailboxOrigID = m.MailboxDestinedID m.IsReject = false + m.Seen = false } m.UID = mbDst.UIDNext m.ModSeq = modseq