From e54fe220e5878d1cb328cb7e1e7401bcac45e1a7 Mon Sep 17 00:00:00 2001 From: Santiago Date: Thu, 21 Oct 2021 03:12:15 -0300 Subject: [PATCH] Fix panic when Slack API sends unexpected response (#40721) * panic when unexpected Slack response fixed, tests added * fix linting errors, add test --- pkg/services/alerting/notifiers/slack.go | 14 +-- pkg/services/alerting/notifiers/slack_test.go | 102 ++++++++++++++++++ .../ngalert/notifier/channels/slack.go | 14 +-- .../ngalert/notifier/channels/slack_test.go | 92 ++++++++++++++++ 4 files changed, 210 insertions(+), 12 deletions(-) diff --git a/pkg/services/alerting/notifiers/slack.go b/pkg/services/alerting/notifiers/slack.go index 7b3032c068f..057227e79f7 100644 --- a/pkg/services/alerting/notifiers/slack.go +++ b/pkg/services/alerting/notifiers/slack.go @@ -382,15 +382,17 @@ func (sn *SlackNotifier) sendRequest(ctx context.Context, data []byte) error { return fmt.Errorf("failed to read response body: %w", err) } - if resp.StatusCode/100 == 2 { - var rslt map[string]interface{} + if resp.StatusCode >= http.StatusOK && resp.StatusCode < http.StatusMultipleChoices { // Slack responds to some requests with a JSON document, that might contain an error + rslt := struct { + Ok bool `json:"ok"` + Err string `json:"error"` + }{} if err := json.Unmarshal(body, &rslt); err == nil { - if !rslt["ok"].(bool) { - errMsg := rslt["error"].(string) + if !rslt.Ok && rslt.Err != "" { sn.log.Warn("Sending Slack API request failed", "url", sn.url.String(), "statusCode", resp.Status, - "err", errMsg) - return fmt.Errorf("failed to make Slack API request: %s", errMsg) + "err", rslt.Err) + return fmt.Errorf("failed to make Slack API request: %s", rslt.Err) } } diff --git a/pkg/services/alerting/notifiers/slack_test.go b/pkg/services/alerting/notifiers/slack_test.go index dde7cd6c334..64be9af4e32 100644 --- a/pkg/services/alerting/notifiers/slack_test.go +++ b/pkg/services/alerting/notifiers/slack_test.go @@ -2,6 +2,9 @@ package notifiers import ( "context" + "fmt" + "net/http" + "net/http/httptest" "testing" "github.com/grafana/grafana/pkg/components/simplejson" @@ -222,3 +225,102 @@ func TestSlackNotifier(t *testing.T) { assert.Equal(t, "1ABCDE", slackNotifier.recipient) }) } + +func TestSendSlackRequest(t *testing.T) { + tests := []struct { + name string + slackResponse string + statusCode int + expectError bool + }{ + { + name: "Example error", + slackResponse: `{ + "ok": false, + "error": "too_many_attachments" + }`, + statusCode: http.StatusBadRequest, + expectError: true, + }, + { + name: "Non 200 status code, no response body", + statusCode: http.StatusMovedPermanently, + expectError: true, + }, + { + name: "Success case, normal response body", + slackResponse: `{ + "ok": true, + "channel": "C1H9RESGL", + "ts": "1503435956.000247", + "message": { + "text": "Here's a message for you", + "username": "ecto1", + "bot_id": "B19LU7CSY", + "attachments": [ + { + "text": "This is an attachment", + "id": 1, + "fallback": "This is an attachment's fallback" + } + ], + "type": "message", + "subtype": "bot_message", + "ts": "1503435956.000247" + } + }`, + statusCode: http.StatusOK, + expectError: false, + }, + { + name: "Success case, no response body", + statusCode: http.StatusOK, + expectError: false, + }, + { + name: "Success case, unexpected response body", + statusCode: http.StatusOK, + slackResponse: "{}", + expectError: false, + }, + { + name: "Success case, ok: true", + statusCode: http.StatusOK, + slackResponse: "{\"ok\": true}", + expectError: false, + }, + { + name: "200 status code, error in body", + statusCode: http.StatusOK, + slackResponse: `{"ok": false, "error": "test error"}`, + expectError: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(tt *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(test.statusCode) + _, err := w.Write([]byte(test.slackResponse)) + require.NoError(tt, err) + })) + + settingsJSON, err := simplejson.NewJson([]byte(fmt.Sprintf(`{"url": %q}`, server.URL))) + require.NoError(t, err) + model := &models.AlertNotification{ + Settings: settingsJSON, + } + + not, err := NewSlackNotifier(model, ossencryption.ProvideService().GetDecryptedValue) + require.NoError(t, err) + slackNotifier := not.(*SlackNotifier) + + err = slackNotifier.sendRequest(context.TODO(), []byte("test")) + if !test.expectError { + require.NoError(tt, err) + } else { + require.Error(tt, err) + } + }) + } +} diff --git a/pkg/services/ngalert/notifier/channels/slack.go b/pkg/services/ngalert/notifier/channels/slack.go index d209b1ffd95..9831e093f08 100644 --- a/pkg/services/ngalert/notifier/channels/slack.go +++ b/pkg/services/ngalert/notifier/channels/slack.go @@ -219,19 +219,21 @@ var sendSlackRequest = func(request *http.Request, logger log.Logger) error { return fmt.Errorf("failed to read response body: %w", err) } - if resp.StatusCode/100 != 2 { + if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { logger.Warn("Slack API request failed", "url", request.URL.String(), "statusCode", resp.Status, "body", string(body)) return fmt.Errorf("request to Slack API failed with status code %d", resp.StatusCode) } - var rslt map[string]interface{} // Slack responds to some requests with a JSON document, that might contain an error + rslt := struct { + Ok bool `json:"ok"` + Err string `json:"error"` + }{} if err := json.Unmarshal(body, &rslt); err == nil { - if !rslt["ok"].(bool) { - errMsg := rslt["error"].(string) + if !rslt.Ok && rslt.Err != "" { logger.Warn("Sending Slack API request failed", "url", request.URL.String(), "statusCode", resp.Status, - "err", errMsg) - return fmt.Errorf("failed to make Slack API request: %s", errMsg) + "err", rslt.Err) + return fmt.Errorf("failed to make Slack API request: %s", rslt.Err) } } diff --git a/pkg/services/ngalert/notifier/channels/slack_test.go b/pkg/services/ngalert/notifier/channels/slack_test.go index d35ec92af7e..cf2714dbbd5 100644 --- a/pkg/services/ngalert/notifier/channels/slack_test.go +++ b/pkg/services/ngalert/notifier/channels/slack_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "io" "net/http" + "net/http/httptest" "net/url" "testing" @@ -221,3 +222,94 @@ func TestSlackNotifier(t *testing.T) { }) } } + +func TestSendSlackRequest(t *testing.T) { + tests := []struct { + name string + slackResponse string + statusCode int + expectError bool + }{ + { + name: "Example error", + slackResponse: `{ + "ok": false, + "error": "too_many_attachments" + }`, + statusCode: http.StatusBadRequest, + expectError: true, + }, + { + name: "Non 200 status code, no response body", + statusCode: http.StatusMovedPermanently, + expectError: true, + }, + { + name: "Success case, normal response body", + slackResponse: `{ + "ok": true, + "channel": "C1H9RESGL", + "ts": "1503435956.000247", + "message": { + "text": "Here's a message for you", + "username": "ecto1", + "bot_id": "B19LU7CSY", + "attachments": [ + { + "text": "This is an attachment", + "id": 1, + "fallback": "This is an attachment's fallback" + } + ], + "type": "message", + "subtype": "bot_message", + "ts": "1503435956.000247" + } + }`, + statusCode: http.StatusOK, + expectError: false, + }, + { + name: "Success case, no response body", + statusCode: http.StatusOK, + expectError: false, + }, + { + name: "Success case, unexpected response body", + statusCode: http.StatusOK, + slackResponse: "{}", + expectError: false, + }, + { + name: "Success case, ok: true", + statusCode: http.StatusOK, + slackResponse: "{\"ok\": true}", + expectError: false, + }, + { + name: "200 status code, error in body", + statusCode: http.StatusOK, + slackResponse: `{"ok": false, "error": "test error"}`, + expectError: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(tt *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(test.statusCode) + _, err := w.Write([]byte(test.slackResponse)) + require.NoError(tt, err) + })) + req, err := http.NewRequest(http.MethodGet, server.URL, nil) + require.NoError(tt, err) + + err = sendSlackRequest(req, log.New("test")) + if !test.expectError { + require.NoError(tt, err) + } else { + require.Error(tt, err) + } + }) + } +}