Repository: slack-go/slack (go)

Issue:
OAuth Slack Token Logged In Clear
**Describe the bug**

While using Argo Event which leverage `slack-go/slack` for Slack triggers, it was noticed that the OAuth Slack token is logged in the clear. Specifically, https://github.com/slack-go/slack/blob/686c209f9525a78313cfe54cbc07cd86bd677384/chat.go#L217 is logging the whole API request resulting in also logging a clear version of the OAuth slack token in the log files. The following is an example:

```
sensor-qrlrb-58d497d595-q9dmh main 2020-11-09T10:11:50.147597554Z slack-go/slack2020/11/09 10:11:50 chat.go:217: Sending request: channel=random&text=webhook+triggered%21&token=xoxb-xxxx-xxx-xxx
```

**Expected behavior**

OAuth Slack token not never be logged in the clear in the log files.

_**Argo Events related issue:**_ https://github.com/argoproj/argo-events/issues/944

Pull request:
Redact tokens in SendMessage debug log
This fixes #844.

Instead of changing the request itself as in PR #1102 (which was closed because of this), this PR merely sanitizes the logging.

Added a unit test to ensure the redaction doesn't touch anything other that the token: even if it did it wouldn't have any functional impact, but it would reduce the usefulness of the debug log.

Also added a tiny and unrelated lint fix in `interactions_test.go` _en passant_.

I ran `make pr-prep` as requested - all unit tests passed.

Patch:
diff --git a/chat.go b/chat.go
index 4448e9f..35ffbbc 100644
--- a/chat.go
+++ b/chat.go
@@ -7,6 +7,7 @@ import (
 	"io/ioutil"
 	"net/http"
 	"net/url"
+	"regexp"
 	"strconv"
 
 	"github.com/slack-go/slack/slackutilsx"
@@ -29,9 +30,9 @@ const (
 
 type chatResponseFull struct {
 	Channel            string `json:"channel"`
-	Timestamp          string `json:"ts"`                             //Regular message timestamp
-	MessageTimeStamp   string `json:"message_ts"`                     //Ephemeral message timestamp
-	ScheduledMessageID string `json:"scheduled_message_id,omitempty"` //Scheduled message id
+	Timestamp          string `json:"ts"`                             // Regular message timestamp
+	MessageTimeStamp   string `json:"message_ts"`                     // Ephemeral message timestamp
+	ScheduledMessageID string `json:"scheduled_message_id,omitempty"` // Scheduled message id
 	Text               string `json:"text"`
 	SlackResponse
 }
@@ -224,7 +225,7 @@ func (api *Client) SendMessageContext(ctx context.Context, channelID string, opt
 			return "", "", "", err
 		}
 		req.Body = ioutil.NopCloser(bytes.NewBuffer(reqBody))
-		api.Debugf("Sending request: %s", string(reqBody))
+		api.Debugf("Sending request: %s", redactToken(reqBody))
 	}
 
 	if err = doPost(ctx, api.httpclient, req, parser(&response), api); err != nil {
@@ -234,6 +235,20 @@ func (api *Client) SendMessageContext(ctx context.Context, channelID string, opt
 	return response.Channel, response.getMessageTimestamp(), response.Text, response.Err()
 }
 
+func redactToken(b []byte) []byte {
+	// See https://api.slack.com/authentication/token-types
+	// and https://api.slack.com/authentication/rotation
+	re, err := regexp.Compile(`(token=x[a-z.]+)-[0-9A-Za-z-]+`)
+	if err != nil {
+		// The regular expression above should never result in errors,
+		// but just in case, do no harm.
+		return b
+	}
+	// Keep "token=" and the first element of the token, which identifies its type
+	// (this could be useful for debugging, e.g. when using a wrong token).
+	return re.ReplaceAll(b, []byte("$1-REDACTED"))
+}
+
 // UnsafeApplyMsgOptions utility function for debugging/testing chat requests.
 // NOTE: USE AT YOUR OWN RISK: No issues relating to the use of this function
 // will be supported by the library.


Test patch:
diff --git a/chat_test.go b/chat_test.go
index 7a549fa..fb37225 100644
--- a/chat_test.go
+++ b/chat_test.go
@@ -1,11 +1,14 @@
 package slack
 
 import (
+	"bytes"
 	"encoding/json"
 	"io/ioutil"
+	"log"
 	"net/http"
 	"net/url"
 	"reflect"
+	"regexp"
 	"testing"
 )
 
@@ -39,7 +42,6 @@ func TestGetPermalink(t *testing.T) {
 	timeStamp := "p135854651500008"
 
 	http.HandleFunc("/chat.getPermalink", func(rw http.ResponseWriter, r *http.Request) {
-
 		if got, want := r.Header.Get("Content-Type"), "application/x-www-form-urlencoded"; got != want {
 			t.Errorf("request uses unexpected content type: got %s, want %s", got, want)
 		}
@@ -296,3 +298,50 @@ func TestPostMessageWhenMsgOptionDeleteOriginalApplied(t *testing.T) {
 
 	_, _, _ = api.PostMessage("CXXX", MsgOptionDeleteOriginal(responseURL))
 }
+
+func TestSendMessageContextRedactsTokenInDebugLog(t *testing.T) {
+	tests := []struct {
+		name  string
+		token string
+		want  string
+	}{
+		{
+			name:  "regular token",
+			token: "xtest-token-1234-abcd",
+			want:  "xtest-REDACTED",
+		},
+		{
+			name:  "refresh token",
+			token: "xoxe.xtest-token-1234-abcd",
+			want:  "xoxe.xtest-REDACTED",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			once.Do(startServer)
+			buf := bytes.NewBufferString("")
+
+			opts := []Option{
+				OptionAPIURL("http://" + serverAddr + "/"),
+				OptionLog(log.New(buf, "", log.Lshortfile)),
+				OptionDebug(true),
+			}
+			api := New(tt.token, opts...)
+			// Why send the token in the message text too? To test that we're not
+			// redacting substrings in the request which look like a token but aren't.
+			api.SendMessage("CXXX", MsgOptionText(token, false))
+			s := buf.String()
+
+			re := regexp.MustCompile(`token=[\w.-]*`)
+			want := "token=" + tt.want
+			if got := re.FindString(s); got != want {
+				t.Errorf("Logged token in SendMessageContext(): got %q, want %q", got, want)
+			}
+			re = regexp.MustCompile(`text=[\w.-]*`)
+			want = "text=" + token
+			if got := re.FindString(s); got != want {
+				t.Errorf("Logged text in SendMessageContext(): got %q, want %q", got, want)
+			}
+		})
+	}
+}
diff --git a/interactions_test.go b/interactions_test.go
index f492b1f..989690e 100644
--- a/interactions_test.go
+++ b/interactions_test.go
@@ -286,14 +286,14 @@ func TestViewSubmissionCallback(t *testing.T) {
 			},
 			State: &ViewState{
 				Values: map[string]map[string]BlockAction{
-					"multi-line": map[string]BlockAction{
-						"ml-value": BlockAction{
+					"multi-line": {
+						"ml-value": {
 							Type:  "plain_text_input",
 							Value: "No onions",
 						},
 					},
-					"target_channel": map[string]BlockAction{
-						"target_select": BlockAction{
+					"target_channel": {
+						"target_select": {
 							Type:  "conversations_select",
 							Value: "C1AB2C3DE",
 						},