Repository: emersion/go-imap (go)

Issue:
FETCH FLAGS and unilateral messages: Unexpected results
IMAP servers (here: Dovecot) tend to send you some information "while you're at it". go-imap in general handles them via the `Updates` channel. I stumbled now upon an issue with FETCH FLAGS and an unilateral update:

```
a66 uid fetch 213531 flags
* 2004 FETCH (UID 213531 FLAGS (\Seen))
* 1994 FETCH (FLAGS ())
a66 OK Fetch completed (0.001 + 0.000 secs).
```

In this case, a `c.UidFetch(213531, imap.FetchFlags)` _(pseudo-code)_ will yield TWO messages as a result (the one for the message asked, and another). And this was totally unexpected on my end, leading to dead-locks, because I stopped reading from the message channel after the first result.

This is either

- an implementation bug: `Fetch.Handle` should ensure to only handle requested messages and pass the rest onto the Update handler
- a documentation ... enhancement: Make it clear, that this command can return more than one asked for.

If it is decided to be the former, I can create a PR for the fix.

Pull request description:
Fetch: Ignore unilateral updates from the server
The server can send unilateral updates as part of fetch results. Those should be ignored by Fetch and be handled by the `Updates` channel.

This fixes #430.


Patch:
diff --git a/client/cmd_selected.go b/client/cmd_selected.go
index 03616cc..a18fec0 100644
--- a/client/cmd_selected.go
+++ b/client/cmd_selected.go
@@ -152,7 +152,7 @@ func (c *Client) fetch(uid bool, seqset *imap.SeqSet, items []imap.FetchItem, ch
 		cmd = &commands.Uid{Cmd: cmd}
 	}
 
-	res := &responses.Fetch{Messages: ch}
+	res := &responses.Fetch{Messages: ch, SeqSet: seqset, Uid: uid}
 
 	status, err := c.execute(cmd, res)
 	if err != nil {
@@ -211,7 +211,7 @@ func (c *Client) store(uid bool, seqset *imap.SeqSet, item imap.StoreItem, value
 
 	var h responses.Handler
 	if ch != nil {
-		h = &responses.Fetch{Messages: ch}
+		h = &responses.Fetch{Messages: ch, SeqSet: seqset, Uid: uid}
 	}
 
 	status, err := c.execute(cmd, h)
diff --git a/responses/fetch.go b/responses/fetch.go
index a1ac52e..691ebcb 100644
--- a/responses/fetch.go
+++ b/responses/fetch.go
@@ -10,6 +10,8 @@ const fetchName = "FETCH"
 // See RFC 3501 section 7.4.2
 type Fetch struct {
 	Messages chan *imap.Message
+	SeqSet   *imap.SeqSet
+	Uid      bool
 }
 
 func (r *Fetch) Handle(resp imap.Resp) error {
@@ -31,6 +33,27 @@ func (r *Fetch) Handle(resp imap.Resp) error {
 		return err
 	}
 
+	if r.Uid && msg.Uid == 0 {
+		// we requested UIDs and got a message without one --> unilateral update --> ignore
+		return ErrUnhandled
+	}
+
+	var num uint32
+	if r.Uid {
+		num = msg.Uid
+	} else {
+		num = seqNum
+	}
+
+	// Check whether we obtained a result we requested with our SeqSet
+	// If the result is not contained in our SeqSet we have to handle an additional special case:
+	// In case we requested UIDs with a dynamic sequence (i.e. * or n:*) and the maximum UID of the mailbox
+	// is less then our n, the server will supply us with the max UID (cf. RFC 3501 §6.4.8 and §9 `seq-range`).
+	// Thus, such a result is correct and has to be returned by us.
+	if !r.SeqSet.Contains(num) && (!r.Uid || !r.SeqSet.Dynamic()) {
+		return ErrUnhandled
+	}
+
 	r.Messages <- msg
 	return nil
 }


Test patch:
diff --git a/client/cmd_selected_test.go b/client/cmd_selected_test.go
index c38318e..67ff942 100644
--- a/client/cmd_selected_test.go
+++ b/client/cmd_selected_test.go
@@ -378,6 +378,129 @@ func TestClient_Fetch_Uid(t *testing.T) {
 	}
 }
 
+func TestClient_Fetch_Unilateral(t *testing.T) {
+	c, s := newTestClient(t)
+	defer s.Close()
+
+	setClientState(c, imap.SelectedState, nil)
+
+	seqset, _ := imap.ParseSeqSet("1:4")
+	fields := []imap.FetchItem{imap.FetchFlags}
+
+	done := make(chan error, 1)
+	messages := make(chan *imap.Message, 3)
+	go func() {
+		done <- c.Fetch(seqset, fields, messages)
+	}()
+
+	tag, cmd := s.ScanCmd()
+	if cmd != "FETCH 1:4 (FLAGS)" {
+		t.Fatalf("client sent command %v, want %v", cmd, "FETCH 1:4 (FLAGS)")
+	}
+
+	s.WriteString("* 2 FETCH (FLAGS (\\Seen))\r\n")
+	s.WriteString("* 123 FETCH (FLAGS (\\Deleted))\r\n")
+	s.WriteString("* 4 FETCH (FLAGS (\\Seen))\r\n")
+	s.WriteString(tag + " OK FETCH completed\r\n")
+
+	if err := <-done; err != nil {
+		t.Fatalf("c.Fetch() = %v", err)
+	}
+
+	msg := <-messages
+	if msg.SeqNum != 2 {
+		t.Errorf("First message has bad sequence number: %v", msg.SeqNum)
+	}
+	msg = <-messages
+	if msg.SeqNum != 4 {
+		t.Errorf("Second message has bad sequence number: %v", msg.SeqNum)
+	}
+
+	_, ok := <-messages
+	if ok {
+		t.Errorf("More than two messages")
+	}
+}
+
+func TestClient_Fetch_Unilateral_Uid(t *testing.T) {
+	c, s := newTestClient(t)
+	defer s.Close()
+
+	setClientState(c, imap.SelectedState, nil)
+
+	seqset, _ := imap.ParseSeqSet("1:4")
+	fields := []imap.FetchItem{imap.FetchFlags}
+
+	done := make(chan error, 1)
+	messages := make(chan *imap.Message, 3)
+	go func() {
+		done <- c.UidFetch(seqset, fields, messages)
+	}()
+
+	tag, cmd := s.ScanCmd()
+	if cmd != "UID FETCH 1:4 (FLAGS)" {
+		t.Fatalf("client sent command %v, want %v", cmd, "UID FETCH 1:4 (FLAGS)")
+	}
+
+	s.WriteString("* 23 FETCH (UID 2 FLAGS (\\Seen))\r\n")
+	s.WriteString("* 123 FETCH (FLAGS (\\Deleted))\r\n")
+	s.WriteString("* 49 FETCH (UID 4 FLAGS (\\Seen))\r\n")
+	s.WriteString(tag + " OK FETCH completed\r\n")
+
+	if err := <-done; err != nil {
+		t.Fatalf("c.Fetch() = %v", err)
+	}
+
+	msg := <-messages
+	if msg.Uid != 2 {
+		t.Errorf("First message has bad UID: %v", msg.Uid)
+	}
+	msg = <-messages
+	if msg.Uid != 4 {
+		t.Errorf("Second message has bad UID: %v", msg.Uid)
+	}
+
+	_, ok := <-messages
+	if ok {
+		t.Errorf("More than two messages")
+	}
+}
+
+func TestClient_Fetch_Uid_Dynamic(t *testing.T) {
+	c, s := newTestClient(t)
+	defer s.Close()
+
+	setClientState(c, imap.SelectedState, nil)
+
+	seqset, _ := imap.ParseSeqSet("4:*")
+	fields := []imap.FetchItem{imap.FetchFlags}
+
+	done := make(chan error, 1)
+	messages := make(chan *imap.Message, 1)
+	go func() {
+		done <- c.UidFetch(seqset, fields, messages)
+	}()
+
+	tag, cmd := s.ScanCmd()
+	if cmd != "UID FETCH 4:* (FLAGS)" {
+		t.Fatalf("client sent command %v, want %v", cmd, "UID FETCH 4:* (FLAGS)")
+	}
+
+	s.WriteString("* 23 FETCH (UID 2 FLAGS (\\Seen))\r\n")
+	s.WriteString(tag + " OK FETCH completed\r\n")
+
+	if err := <-done; err != nil {
+		t.Fatalf("c.Fetch() = %v", err)
+	}
+
+	msg, ok := <-messages
+	if !ok {
+		t.Errorf("No message supplied")
+	} else if msg.Uid != 2 {
+		t.Errorf("First message has bad UID: %v", msg.Uid)
+	}
+}
+
 func TestClient_Store(t *testing.T) {
 	c, s := newTestClient(t)
 	defer s.Close()