diff --git a/internal/httpapi/mailbox_handlers.go b/internal/httpapi/mailbox_handlers.go index 9b05b6a..8c1b135 100644 --- a/internal/httpapi/mailbox_handlers.go +++ b/internal/httpapi/mailbox_handlers.go @@ -147,7 +147,7 @@ func (s *Server) SyncMailbox(ctx context.Context, user *store.User, m *store.Mai } defer client.Close() - messages, err := client.Since(m.LastUID, 25) + messages, searched, err := client.Since(m.LastUID, 25) if err != nil { return 0, err } @@ -197,6 +197,14 @@ func (s *Server) SyncMailbox(ctx context.Context, user *store.User, m *store.Mai s.hub.Publish(projectID, "task.created", toTaskJSON(*task)) } + // A run that got all the way through moves the marker to the whole of what the + // search considered, not merely to the last message it happened to read. That + // is what stops the same mail becoming a task again on the next run when the + // server did not put a uid in the fetch reply. + if ctx.Err() == nil && searched > highest { + highest = searched + } + // Written even when the loop was cut short, and outside the request's context // so a cancelled request still records what it did. if highest > m.LastUID { diff --git a/internal/imap/imap.go b/internal/imap/imap.go index ba4b6e8..fa27d22 100644 --- a/internal/imap/imap.go +++ b/internal/imap/imap.go @@ -16,15 +16,18 @@ package imap import ( + "bytes" "crypto/tls" "crypto/x509" "fmt" + "io" "net" "strings" "time" "github.com/emersion/go-imap/v2" "github.com/emersion/go-imap/v2/imapclient" + "github.com/emersion/go-message" ) // Message is one mail, reduced to what a task needs. @@ -108,9 +111,13 @@ func (c *Client) Close() error { // that consumed it would be taking something away. Flagging a mail is a deliberate // act with no other meaning, which is exactly what is wanted — the same reason the // Gmail side reads stars. -func (c *Client) Since(uid uint32, limit int) ([]Message, error) { +// The second return is the highest uid the search considered. A caller that got +// through every message can move its marker to that, and then it does not matter +// whether the server put a uid in each fetch reply — which some do not, and which +// is how the same mail became a task again on every run. +func (c *Client) Since(uid uint32, limit int) ([]Message, uint32, error) { if _, err := c.c.Select(c.folder, &imap.SelectOptions{ReadOnly: true}).Wait(); err != nil { - return nil, fmt.Errorf("open %s: %w", c.folder, err) + return nil, 0, fmt.Errorf("open %s: %w", c.folder, err) } // From uid+1 to the end. A mailbox with nothing new answers with no uids @@ -124,12 +131,12 @@ func (c *Client) Since(uid uint32, limit int) ([]Message, error) { } found, err := c.c.UIDSearch(criteria, nil).Wait() if err != nil { - return nil, fmt.Errorf("search %s: %w", c.folder, err) + return nil, 0, fmt.Errorf("search %s: %w", c.folder, err) } uids := found.AllUIDs() if len(uids) == 0 { - return nil, nil + return nil, uid, nil } if limit > 0 && len(uids) > limit { // The newest ones. A mailbox flagged over years would otherwise make the @@ -139,29 +146,51 @@ func (c *Client) Since(uid uint32, limit int) ([]Message, error) { want := imap.UIDSetNum(uids...) fetch := &imap.FetchOptions{ - Envelope: true, - UID: true, - BodySection: []*imap.FetchItemBodySection{{Specifier: imap.PartSpecifierText, Peek: true}}, + Envelope: true, + UID: true, + // The whole message, headers included, so the MIME structure can be walked. + // TEXT alone gives the body as bytes — boundaries and part headers and all — + // which is what put "--=_be71cb39… Content-Transfer-Encoding: 8bit" at the + // front of every task made from a multipart mail. + BodySection: []*imap.FetchItemBodySection{{Peek: true}}, } + var highest uint32 + for _, u := range uids { + if uint32(u) > highest { + highest = uint32(u) + } + } + msgs, err := c.c.Fetch(want, fetch).Collect() if err != nil { - return nil, fmt.Errorf("fetch from %s: %w", c.folder, err) + return nil, 0, fmt.Errorf("fetch from %s: %w", c.folder, err) } out := make([]Message, 0, len(msgs)) - for _, m := range msgs { + for i, m := range msgs { if m.Envelope == nil { continue } + + // The uid comes from the search when the fetch reply does not carry one. + // A missing uid is not cosmetic: it is the marker the next run starts from, + // and a marker stuck at zero makes every message arrive again on every run. + // That is what happened against a real server while the tests stayed green, + // because the server they run against always answers with it. + msgUID := uint32(m.UID) + if msgUID == 0 && i < len(uids) { + msgUID = uint32(uids[i]) + } + out = append(out, Message{ - UID: uint32(m.UID), + UID: msgUID, From: address(m.Envelope.From), Subject: strings.TrimSpace(m.Envelope.Subject), Snippet: snippet(m), Date: m.Envelope.Date, }) } - return out, nil + return out, highest, nil } // address renders the first sender as "Name ", or just the address. @@ -177,23 +206,92 @@ func address(list []imap.Address) string { return a.Name + " <" + addr + ">" } -// snippet is the first readable line or so of the body. +// snippet is the first readable text in the mail. // -// No MIME walk and no HTML stripping yet: this is what shows under a task's title, -// and a wrong-but-short line is a smaller problem than a parser that has to be -// right about every mail ever sent. The full mail stays where it is. +// It walks the MIME structure rather than taking the body as bytes. text/plain is +// preferred and HTML is the fallback with its tags stripped — crudely, because +// this is the line under a task's title and not a rendering engine. The mail +// itself stays where it is. const snippetLimit = 400 func snippet(m *imapclient.FetchMessageBuffer) string { for _, body := range m.BodySection { - text := strings.TrimSpace(string(body.Bytes)) - if text == "" { - continue + if text := readable(body.Bytes); text != "" { + return cut(text) } - if len(text) > snippetLimit { - text = text[:snippetLimit] + "…" - } - return text } return "" } + +func readable(raw []byte) string { + entity, err := message.Read(bytes.NewReader(raw)) + if err != nil { + // Not something this parser recognises. The raw bytes read badly but read, + // and a mail is not worth losing over its own headers. + return strings.TrimSpace(string(raw)) + } + return strings.TrimSpace(fromEntity(entity, 0)) +} + +// fromEntity descends into a multipart mail. The depth guard is for the mail that +// nests parts inside parts inside parts — rare, and cheaper to bound than to trust. +func fromEntity(entity *message.Entity, depth int) string { + if depth > 4 { + return "" + } + + if parts := entity.MultipartReader(); parts != nil { + var html string + for { + part, err := parts.NextPart() + if err != nil { + break + } + if text := fromEntity(part, depth+1); text != "" { + kind, _, _ := part.Header.ContentType() + if kind != "text/html" { + return text + } + if html == "" { + html = text + } + } + } + return html + } + + body, _ := io.ReadAll(entity.Body) + kind, _, _ := entity.Header.ContentType() + if kind == "text/html" { + return stripTags(string(body)) + } + return strings.TrimSpace(string(body)) +} + +// stripTags removes markup without pretending to understand it. Enough for one +// line of preview, and honest about being no more than that. +func stripTags(html string) string { + var b strings.Builder + depth := 0 + for _, r := range html { + switch { + case r == '<': + depth++ + case r == '>': + if depth > 0 { + depth-- + } + case depth == 0: + b.WriteRune(r) + } + } + return strings.Join(strings.Fields(b.String()), " ") +} + +func cut(text string) string { + runes := []rune(text) + if len(runes) > snippetLimit { + return string(runes[:snippetLimit]) + "…" + } + return text +} diff --git a/internal/imap/roundtrip_test.go b/internal/imap/roundtrip_test.go index 5eea409..d96046a 100644 --- a/internal/imap/roundtrip_test.go +++ b/internal/imap/roundtrip_test.go @@ -37,7 +37,7 @@ func TestFlaggedMailComesBackAsMessages(t *testing.T) { } defer client.Close() - msgs, err := client.Since(0, 25) + msgs, _, err := client.Since(0, 25) if err != nil { t.Fatal(err) } @@ -59,7 +59,7 @@ func TestFlaggedMailComesBackAsMessages(t *testing.T) { // And reading on from a marker returns only what is newer, which is the whole // of the deduplication. - rest, err := client.Since(msgs[0].UID, 25) + rest, _, err := client.Since(msgs[0].UID, 25) if err != nil { t.Fatal(err) } @@ -92,6 +92,9 @@ func TestAnUntrustedCertificateIsRefused(t *testing.T) { type mail struct { subject, from, body string flagged bool + // raw replaces the whole message when set, which is the only way to get a + // genuinely multipart one in. + raw string } // serveMailbox starts an in-process IMAP server over TLS and returns its address @@ -112,8 +115,11 @@ func serveMailbox(t *testing.T, mails []mail) (string, *x509.CertPool) { if m.flagged { flags = append(flags, imap.FlagFlagged) } - raw := "From: " + m.from + "\r\nSubject: " + m.subject + - "\r\nDate: Tue, 18 Aug 2026 10:00:00 +0200\r\n\r\n" + m.body + "\r\n" + raw := m.raw + if raw == "" { + raw = "From: " + m.from + "\r\nSubject: " + m.subject + + "\r\nDate: Tue, 18 Aug 2026 10:00:00 +0200\r\n\r\n" + m.body + "\r\n" + } if _, err := user.Append("INBOX", strings.NewReader(raw), &imap.AppendOptions{ Flags: flags, }); err != nil { @@ -181,3 +187,78 @@ func selfSigned(t *testing.T) (tls.Certificate, *x509.CertPool) { pool.AppendCertsFromPEM(certPEM) return cert, pool } + +// The bug this was written for: every press of "Fetch now" made the same mail +// into a task again. The marker the next run starts from is the uid, and if it +// comes back as zero the marker never moves. +func TestEveryMessageCarriesAUID(t *testing.T) { + addr, pool := serveMailbox(t, []mail{ + {subject: "En", from: "a@example.dk", body: "en", flagged: true}, + {subject: "To", from: "b@example.dk", body: "to", flagged: true}, + }) + client, err := Dial(Account{Host: addr, Username: "kw", Password: "hemmelig", RootCAs: pool}) + if err != nil { + t.Fatal(err) + } + defer client.Close() + + msgs, _, err := client.Since(0, 25) + if err != nil { + t.Fatal(err) + } + for _, m := range msgs { + if m.UID == 0 { + t.Fatalf("%q came back without a uid; the marker would never move and "+ + "every run would make it into a task again", m.Subject) + } + } + if msgs[0].UID == msgs[1].UID { + t.Error("two messages share a uid") + } +} + +// The other half of the same report: the snippet under a task's title read +// "--=_be71cb39… Content-Transfer-Encoding: 8bit Content-Type: text/plain". +func TestAMultipartMailReadsAsItsText(t *testing.T) { + const boundary = "=_be71cb3975bdfbc60a16327cb2f49c23" + body := "This is a multi-part message.\r\n" + + "--" + boundary + "\r\n" + + "Content-Transfer-Encoding: 8bit\r\n" + + "Content-Type: text/plain; charset=UTF-8; format=flowed\r\n\r\n" + + "Hej igen,\r\n\r\nVi har normalt 3 slags. :)\r\n" + + "--" + boundary + "\r\n" + + "Content-Type: text/html; charset=UTF-8\r\n\r\n" + + "

Hej igen

\r\n" + + "--" + boundary + "--\r\n" + + addr, pool := serveMailbox(t, []mail{{ + flagged: true, + raw: "Content-Type: multipart/alternative; boundary=\"" + boundary + "\"\r\n" + + "From: kontakt@mikrobagt.dk\r\nSubject: Re: Kaffebestilling\r\n" + + "Date: Tue, 18 Aug 2026 10:00:00 +0200\r\n\r\n" + body, + }}) + + client, err := Dial(Account{Host: addr, Username: "kw", Password: "hemmelig", RootCAs: pool}) + if err != nil { + t.Fatal(err) + } + defer client.Close() + + msgs, _, err := client.Since(0, 25) + if err != nil { + t.Fatal(err) + } + if len(msgs) != 1 { + t.Fatalf("got %d messages", len(msgs)) + } + + got := msgs[0].Snippet + for _, junk := range []string{boundary, "Content-Transfer-Encoding", "Content-Type", "", "

"} { + if strings.Contains(got, junk) { + t.Errorf("the snippet still carries %q: %q", junk, got) + } + } + if !strings.HasPrefix(got, "Hej igen") { + t.Errorf("the snippet does not start with the text of the mail: %q", got) + } +}