Repository: emersion/go-imap
Pull request #358 (2020-05-08 12:42:08)

server: Terminate SASL exchange with proper response on "*"

codecov[bot]: # [Codecov](https://codecov.io/gh/emersion/go-imap/pull/358?src=pr&el=h1) Report
> Merging [#358](https://codecov.io/gh/emersion/go-imap/pull/358?src=pr&el=desc) into [master](https://codecov.io/gh/emersion/go-imap/commit/89df427d2794007b4669df962b60251632ba835e&el=desc) will **not change** coverage.
> The diff coverage is `50.00%`.

[![Impacted file tree graph](https://codecov.io/gh/emersion/go-imap/pull/358/graphs/tree.svg?width=650&height=150&src=pr&token=toR3ygEJFD)](https://codecov.io/gh/emersion/go-imap/pull/358?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master     #358   +/-   ##
=======================================
  Coverage   73.55%   73.55%           
=======================================
  Files          32       32           
  Lines        3524     3524           
=======================================
  Hits         2592     2592           
  Misses        643      643           
  Partials      289      289           
```


| [Impacted Files](https://codecov.io/gh/emersion/go-imap/pull/358?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [status.go](https://codecov.io/gh/emersion/go-imap/pull/358/diff?src=pr&el=tree#diff-c3RhdHVzLmdv) | `32.00% <0.00%> (-2.79%)` | :arrow_down: |
| [server/server.go](https://codecov.io/gh/emersion/go-imap/pull/358/diff?src=pr&el=tree#diff-c2VydmVyL3NlcnZlci5nbw==) | `50.26% <50.00%> (+0.53%)` | :arrow_up: |
| [server/conn.go](https://codecov.io/gh/emersion/go-imap/pull/358/diff?src=pr&el=tree#diff-c2VydmVyL2Nvbm4uZ28=) | `72.58% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/emersion/go-imap/pull/358?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/emersion/go-imap/pull/358?src=pr&el=footer). Last update [89df427...5f5acf2](https://codecov.io/gh/emersion/go-imap/pull/358?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

emersion: Can you split the changes in logical commits, removing the fixup commits?
foxcpp: > Can you split the changes in logical commits, removing the fixup commits?

Done.

Patch:
diff --git a/commands/authenticate.go b/commands/authenticate.go
index ec39a41..b66f21f 100644
--- a/commands/authenticate.go
+++ b/commands/authenticate.go
@@ -109,6 +109,12 @@ func (cmd *Authenticate) Handle(mechanisms map[string]sasl.Server, conn Authenti
 
 		encoded = scanner.Text()
 		if encoded != "" {
+			if encoded == "*" {
+				return &imap.ErrStatusResp{Resp: &imap.StatusResp{
+					Type: imap.StatusRespBad,
+					Info: "negotiation cancelled",
+				}}
+			}
 			response, err = base64.StdEncoding.DecodeString(encoded)
 			if err != nil {
 				return err
diff --git a/server/conn.go b/server/conn.go
index 7cad37b..7da57a1 100644
--- a/server/conn.go
+++ b/server/conn.go
@@ -377,8 +377,8 @@ func (c *conn) handleCommand(cmd *imap.Command) (res *imap.StatusResp, up Upgrad
 	}
 
 	hdlrErr := hdlr.Handle(c.conn)
-	if statusErr, ok := hdlrErr.(*errStatusResp); ok {
-		res = statusErr.resp
+	if statusErr, ok := hdlrErr.(*imap.ErrStatusResp); ok {
+		res = statusErr.Resp
 	} else if hdlrErr != nil {
 		res = &imap.StatusResp{
 			Type: imap.StatusRespNo,
diff --git a/server/server.go b/server/server.go
index 204f432..1058c0c 100644
--- a/server/server.go
+++ b/server/server.go
@@ -27,7 +27,7 @@ type Handler interface {
 	// Handle this command for a given connection.
 	//
 	// By default, after this function has returned a status response is sent. To
-	// prevent this behavior handlers can use ErrStatusResp or ErrNoStatusResp.
+	// prevent this behavior handlers can use imap.ErrStatusResp.
 	Handle(conn Conn) error
 }
 
@@ -65,26 +65,22 @@ type ConnExtension interface {
 	NewConn(c Conn) Conn
 }
 
-type errStatusResp struct {
-	resp *imap.StatusResp
-}
-
-func (err *errStatusResp) Error() string {
-	return ""
-}
-
 // ErrStatusResp can be returned by a Handler to replace the default status
 // response. The response tag must be empty.
 //
-// To disable the default status response, use ErrNoStatusResp instead.
+// Deprecated: Use imap.ErrStatusResp{res} instead.
+//
+// To disable the default status response, use imap.ErrStatusResp{nil} instead.
 func ErrStatusResp(res *imap.StatusResp) error {
-	return &errStatusResp{res}
+	return &imap.ErrStatusResp{res}
 }
 
 // ErrNoStatusResp can be returned by a Handler to prevent the default status
 // response from being sent.
+//
+// Deprecated: Use imap.ErrStatusResp{nil} instead
 func ErrNoStatusResp() error {
-	return &errStatusResp{nil}
+	return &imap.ErrStatusResp{nil}
 }
 
 // An IMAP server.
diff --git a/status.go b/status.go
index 86a95b7..81ffd1b 100644
--- a/status.go
+++ b/status.go
@@ -118,3 +118,19 @@ func (r *StatusResp) WriteTo(w *Writer) error {
 
 	return w.writeCrlf()
 }
+
+// ErrStatusResp can be returned by a server.Handler to replace the default status
+// response. The response tag must be empty.
+//
+// To suppress default response, Resp should be set to nil.
+type ErrStatusResp struct {
+	// Response to send instead of default.
+	Resp *StatusResp
+}
+
+func (err *ErrStatusResp) Error() string {
+	if err.Resp == nil {
+		return "imap: suppressed response"
+	}
+	return err.Resp.Info
+}


Test patch:
diff --git a/server/cmd_noauth_test.go b/server/cmd_noauth_test.go
index 65ba0e6..cd507b8 100644
--- a/server/cmd_noauth_test.go
+++ b/server/cmd_noauth_test.go
@@ -157,6 +157,26 @@ func TestAuthenticate_Plain_Ok(t *testing.T) {
 	}
 }
 
+func TestAuthenticate_Plain_Cancel(t *testing.T) {
+	s, c, scanner := testServerGreeted(t)
+	defer s.Close()
+	defer c.Close()
+
+	io.WriteString(c, "a001 AUTHENTICATE PLAIN\r\n")
+
+	scanner.Scan()
+	if scanner.Text() != "+" {
+		t.Fatal("Bad continuation request:", scanner.Text())
+	}
+
+	io.WriteString(c, "*\r\n")
+
+	scanner.Scan()
+	if !strings.HasPrefix(scanner.Text(), "a001 BAD negotiation cancelled") {
+		t.Fatal("Bad status response:", scanner.Text())
+	}
+}
+
 func TestAuthenticate_Plain_InitialResponse(t *testing.T) {
 	s, c, scanner := testServerGreeted(t)
 	defer s.Close()