Repository: slack-go/slack (go)

Issue:
Error from "Ensure" should not disclose expected signature
Hello! First of all, a big thank-you to the maintainers and contributors for this library.

I'm surfacing this issue because I wonder if the error message located [here](https://github.com/slack-go/slack/blob/master/security.go#L90) is too descriptive. Specifically, it reads:

```
return fmt.Errorf("Expected signing signature: %s, but computed: %s", hex.EncodeToString(v.signature), hex.EncodeToString(computed))
```

My concern is that since Go errors tend to get blindly passed upwards, this could be eventually returned directly to a caller making a request. Because it directly discloses the expected signature, the caller could simply update to the expected signature, and re-execute the call successfully.

Is returning the expected signature actually useful for debugging? If it's not, then perhaps we could strike it completely. If it _is_, perhaps instead we could do something like:

```
log.Debugf(""Expected signing signature: %s, but computed: %s", hex.EncodeToString(v.signature), hex.EncodeToString(computed)")
return fmt.Errorf("Computed unexpected signature of: %s", hex.EncodeToString(v.signature), hex.EncodeToString(computed))
```

Am I missing something here? Let me know. If this seems reasonable, I'm happy to open a pull request, just want to check with maintainers first. Thank you!

Pull request:
Avoid returning correct signature
Closes #849 

This PR updates the code to avoid returning the correct signature. This prevents a malicious caller from viewing the error, updating their signature to the now known correct one, and re-executing the call.

A maintainer stated that if we were to remove the explicitly correct signature, it would be nice to also log the correct one at the debug level. To do that without changing the signature of the `NewSecretsVerifier` method, I added a `WithDebug` method that could be optionally called when creating the verifier, which follows [the Builder pattern](https://en.wikipedia.org/wiki/Builder_pattern). It would have been strange to take an argument that was an unexported interface, so I also exported the `Debug` interface. That makes this change is 100% backwards compatible. 

Happy to change my approach, add tests, or anything else that might help. A big thank-you to this repo's maintainers! 😄 

Patch:
diff --git a/attachments.go b/attachments.go
index b5b79f9..5f388e9 100644
--- a/attachments.go
+++ b/attachments.go
@@ -75,7 +75,7 @@ type Attachment struct {
 	Title     string `json:"title,omitempty"`
 	TitleLink string `json:"title_link,omitempty"`
 	Pretext   string `json:"pretext,omitempty"`
-	Text      string `json:"text,omitempty"`
+	Text      string `json:"text"` // Required
 
 	ImageURL string `json:"image_url,omitempty"`
 	ThumbURL string `json:"thumb_url,omitempty"`
diff --git a/block_action.go b/block_action.go
index c15e4a3..fe46a95 100644
--- a/block_action.go
+++ b/block_action.go
@@ -6,7 +6,7 @@ package slack
 type ActionBlock struct {
 	Type     MessageBlockType `json:"type"`
 	BlockID  string           `json:"block_id,omitempty"`
-	Elements *BlockElements   `json:"elements"`
+	Elements BlockElements    `json:"elements"`
 }
 
 // BlockType returns the type of the block
@@ -19,7 +19,7 @@ func NewActionBlock(blockID string, elements ...BlockElement) *ActionBlock {
 	return &ActionBlock{
 		Type:    MBTAction,
 		BlockID: blockID,
-		Elements: &BlockElements{
+		Elements: BlockElements{
 			ElementSet: elements,
 		},
 	}
diff --git a/logger.go b/logger.go
index 6a3533a..90cb3ca 100644
--- a/logger.go
+++ b/logger.go
@@ -18,7 +18,7 @@ type ilogger interface {
 	Println(...interface{})
 }
 
-type debug interface {
+type Debug interface {
 	Debug() bool
 
 	// Debugf print a formatted debug line.
diff --git a/misc.go b/misc.go
index a5ca51b..336f0af 100644
--- a/misc.go
+++ b/misc.go
@@ -88,7 +88,7 @@ func fileUploadReq(ctx context.Context, path string, values url.Values, r io.Rea
 	return req, nil
 }
 
-func downloadFile(client httpClient, token string, downloadURL string, writer io.Writer, d debug) error {
+func downloadFile(client httpClient, token string, downloadURL string, writer io.Writer, d Debug) error {
 	if downloadURL == "" {
 		return fmt.Errorf("received empty download URL")
 	}
@@ -142,7 +142,7 @@ func jsonReq(endpoint string, body interface{}) (req *http.Request, err error) {
 	return req, nil
 }
 
-func parseResponseBody(body io.ReadCloser, intf interface{}, d debug) error {
+func parseResponseBody(body io.ReadCloser, intf interface{}, d Debug) error {
 	response, err := ioutil.ReadAll(body)
 	if err != nil {
 		return err
@@ -155,7 +155,7 @@ func parseResponseBody(body io.ReadCloser, intf interface{}, d debug) error {
 	return json.Unmarshal(response, intf)
 }
 
-func postLocalWithMultipartResponse(ctx context.Context, client httpClient, method, fpath, fieldname string, values url.Values, intf interface{}, d debug) error {
+func postLocalWithMultipartResponse(ctx context.Context, client httpClient, method, fpath, fieldname string, values url.Values, intf interface{}, d Debug) error {
 	fullpath, err := filepath.Abs(fpath)
 	if err != nil {
 		return err
@@ -169,7 +169,7 @@ func postLocalWithMultipartResponse(ctx context.Context, client httpClient, meth
 	return postWithMultipartResponse(ctx, client, method, filepath.Base(fpath), fieldname, values, file, intf, d)
 }
 
-func postWithMultipartResponse(ctx context.Context, client httpClient, path, name, fieldname string, values url.Values, r io.Reader, intf interface{}, d debug) error {
+func postWithMultipartResponse(ctx context.Context, client httpClient, path, name, fieldname string, values url.Values, r io.Reader, intf interface{}, d Debug) error {
 	pipeReader, pipeWriter := io.Pipe()
 	wr := multipart.NewWriter(pipeWriter)
 	errc := make(chan error)
@@ -216,7 +216,7 @@ func postWithMultipartResponse(ctx context.Context, client httpClient, path, nam
 	}
 }
 
-func doPost(ctx context.Context, client httpClient, req *http.Request, parser responseParser, d debug) error {
+func doPost(ctx context.Context, client httpClient, req *http.Request, parser responseParser, d Debug) error {
 	req = req.WithContext(ctx)
 	resp, err := client.Do(req)
 	if err != nil {
@@ -233,7 +233,7 @@ func doPost(ctx context.Context, client httpClient, req *http.Request, parser re
 }
 
 // post JSON.
-func postJSON(ctx context.Context, client httpClient, endpoint, token string, json []byte, intf interface{}, d debug) error {
+func postJSON(ctx context.Context, client httpClient, endpoint, token string, json []byte, intf interface{}, d Debug) error {
 	reqBody := bytes.NewBuffer(json)
 	req, err := http.NewRequest("POST", endpoint, reqBody)
 	if err != nil {
@@ -246,7 +246,7 @@ func postJSON(ctx context.Context, client httpClient, endpoint, token string, js
 }
 
 // post a url encoded form.
-func postForm(ctx context.Context, client httpClient, endpoint string, values url.Values, intf interface{}, d debug) error {
+func postForm(ctx context.Context, client httpClient, endpoint string, values url.Values, intf interface{}, d Debug) error {
 	reqBody := strings.NewReader(values.Encode())
 	req, err := http.NewRequest("POST", endpoint, reqBody)
 	if err != nil {
@@ -256,7 +256,7 @@ func postForm(ctx context.Context, client httpClient, endpoint string, values ur
 	return doPost(ctx, client, req, newJSONParser(intf), d)
 }
 
-func getResource(ctx context.Context, client httpClient, endpoint string, values url.Values, intf interface{}, d debug) error {
+func getResource(ctx context.Context, client httpClient, endpoint string, values url.Values, intf interface{}, d Debug) error {
 	req, err := http.NewRequest("GET", endpoint, nil)
 	if err != nil {
 		return err
@@ -267,12 +267,12 @@ func getResource(ctx context.Context, client httpClient, endpoint string, values
 	return doPost(ctx, client, req, newJSONParser(intf), d)
 }
 
-func parseAdminResponse(ctx context.Context, client httpClient, method string, teamName string, values url.Values, intf interface{}, d debug) error {
+func parseAdminResponse(ctx context.Context, client httpClient, method string, teamName string, values url.Values, intf interface{}, d Debug) error {
 	endpoint := fmt.Sprintf(WEBAPIURLFormat, teamName, method, time.Now().Unix())
 	return postForm(ctx, client, endpoint, values, intf, d)
 }
 
-func logResponse(resp *http.Response, d debug) error {
+func logResponse(resp *http.Response, d Debug) error {
 	if d.Debug() {
 		text, err := httputil.DumpResponse(resp, true)
 		if err != nil {
@@ -300,7 +300,7 @@ func timerReset(t *time.Timer, d time.Duration) {
 	t.Reset(d)
 }
 
-func checkStatusCode(resp *http.Response, d debug) error {
+func checkStatusCode(resp *http.Response, d Debug) error {
 	if resp.StatusCode == http.StatusTooManyRequests {
 		retry, err := strconv.ParseInt(resp.Header.Get("Retry-After"), 10, 64)
 		if err != nil {
diff --git a/security.go b/security.go
index dbe8fb2..4510352 100644
--- a/security.go
+++ b/security.go
@@ -20,6 +20,7 @@ const (
 
 // SecretsVerifier contains the information needed to verify that the request comes from Slack
 type SecretsVerifier struct {
+	d         Debug
 	signature []byte
 	hmac      hash.Hash
 }
@@ -75,6 +76,11 @@ func NewSecretsVerifier(header http.Header, secret string) (sv SecretsVerifier,
 	return sv, err
 }
 
+func (v *SecretsVerifier) WithDebug(d Debug) *SecretsVerifier {
+	v.d = d
+	return v
+}
+
 func (v *SecretsVerifier) Write(body []byte) (n int, err error) {
 	return v.hmac.Write(body)
 }
@@ -86,8 +92,10 @@ func (v SecretsVerifier) Ensure() error {
 	if hmac.Equal(computed, v.signature) {
 		return nil
 	}
-
-	return fmt.Errorf("Expected signing signature: %s, but computed: %s", hex.EncodeToString(v.signature), hex.EncodeToString(computed))
+	if v.d != nil && v.d.Debug() {
+		v.d.Debugln(fmt.Sprintf("Expected signing signature: %s, but computed: %s", hex.EncodeToString(v.signature), hex.EncodeToString(computed)))
+	}
+	return fmt.Errorf("Computed unexpected signature of: %s", hex.EncodeToString(computed))
 }
 
 func abs64(n int64) int64 {


Test patch:
diff --git a/attachments_test.go b/attachments_test.go
index 4b951f1..24e2254 100644
--- a/attachments_test.go
+++ b/attachments_test.go
@@ -12,6 +12,7 @@ func TestAttachment_UnmarshalMarshalJSON_WithBlocks(t *testing.T) {
 
 	originalAttachmentJson := `{
     "id": 1,
+    "text":"",
     "blocks": [
       {
         "type": "section",
diff --git a/chat_test.go b/chat_test.go
index 450cae7..a038728 100644
--- a/chat_test.go
+++ b/chat_test.go
@@ -101,7 +101,7 @@ func TestPostMessage(t *testing.T) {
 					}),
 			},
 			expected: url.Values{
-				"attachments": []string{`[{"blocks":` + blockStr + `}]`},
+				"attachments": []string{`[{"text":"","blocks":` + blockStr + `}]`},
 				"channel":     []string{"CXXX"},
 				"token":       []string{"testing-token"},
 			},