diff --git a/api/v4/source/actions.yaml b/api/v4/source/actions.yaml index b96cd5b5f16..8bca616dbcc 100644 --- a/api/v4/source/actions.yaml +++ b/api/v4/source/actions.yaml @@ -155,6 +155,28 @@ $ref: "#/components/responses/Unauthorized" "403": $ref: "#/components/responses/Forbidden" + "429": + description: The upstream integration rate-limited the request. The + original status code is preserved so clients can honor retry + semantics. + content: + application/json: + schema: + $ref: "#/components/schemas/AppError" + "502": + description: The upstream integration returned a 5xx (other than 503). + Surfaced as Bad Gateway because the failure is upstream of Mattermost. + content: + application/json: + schema: + $ref: "#/components/schemas/AppError" + "503": + description: The upstream integration is unavailable. The original + status code is preserved so clients can honor retry semantics. + content: + application/json: + schema: + $ref: "#/components/schemas/AppError" /api/v4/actions/dialogs/lookup: post: tags: @@ -227,6 +249,28 @@ $ref: "#/components/responses/Unauthorized" "403": $ref: "#/components/responses/Forbidden" + "429": + description: The upstream integration rate-limited the request. The + original status code is preserved so clients can honor retry + semantics. + content: + application/json: + schema: + $ref: "#/components/schemas/AppError" + "502": + description: The upstream integration returned a 5xx (other than 503). + Surfaced as Bad Gateway because the failure is upstream of Mattermost. + content: + application/json: + schema: + $ref: "#/components/schemas/AppError" + "503": + description: The upstream integration is unavailable. The original + status code is preserved so clients can honor retry semantics. + content: + application/json: + schema: + $ref: "#/components/schemas/AppError" /api/v4/actions/dialogs/execute: post: tags: diff --git a/api/v4/source/posts.yaml b/api/v4/source/posts.yaml index e4494565c29..354a8c2a4ea 100644 --- a/api/v4/source/posts.yaml +++ b/api/v4/source/posts.yaml @@ -1000,6 +1000,28 @@ $ref: "#/components/responses/Unauthorized" "403": $ref: "#/components/responses/Forbidden" + "429": + description: The upstream integration rate-limited the request. The + original status code is preserved so clients can honor retry + semantics. + content: + application/json: + schema: + $ref: "#/components/schemas/AppError" + "502": + description: The upstream integration returned a 5xx (other than 503). + Surfaced as Bad Gateway because the failure is upstream of Mattermost. + content: + application/json: + schema: + $ref: "#/components/schemas/AppError" + "503": + description: The upstream integration is unavailable. The original + status code is preserved so clients can honor retry semantics. + content: + application/json: + schema: + $ref: "#/components/schemas/AppError" "/api/v4/posts/ids": post: tags: diff --git a/server/channels/api4/integration_action_test.go b/server/channels/api4/integration_action_test.go index caff6f58829..7601d0addde 100644 --- a/server/channels/api4/integration_action_test.go +++ b/server/channels/api4/integration_action_test.go @@ -1485,8 +1485,8 @@ func TestExecuteDialogAction(t *testing.T) { resp, err := client.DoAPIPost(context.Background(), route, string(body)) require.Error(t, err) require.NotNil(t, resp) - // DoActionRequest returns a 400 AppError on non-200 upstream responses; + // DoActionRequest maps upstream 5xx (other than 429/503) to 502 Bad Gateway; // the handler propagates it unchanged. - assert.Equal(t, http.StatusBadRequest, resp.StatusCode) + assert.Equal(t, http.StatusBadGateway, resp.StatusCode) }) } diff --git a/server/channels/app/integration_action.go b/server/channels/app/integration_action.go index fe9688e0c2d..91aac9cb95c 100644 --- a/server/channels/app/integration_action.go +++ b/server/channels/app/integration_action.go @@ -162,7 +162,19 @@ func (a *App) DoActionRequest(rctx request.CTX, rawURL string, body []byte) (*ht } if resp.StatusCode != http.StatusOK { - return resp, model.NewAppError("DoActionRequest", "api.post.do_action.action_integration.app_error", nil, fmt.Sprintf("status=%v", resp.StatusCode), http.StatusBadRequest) + // Preserve 429 and 503 because they carry retry semantics (RFC 6585, + // RFC 7231) that downstream HTTP clients legitimately need. Map other + // 5xx responses to 502 Bad Gateway since the failure is upstream of + // MM. Sanitize all other non-200 responses to 400 so plugin-specific + // 4xx codes do not leak misleading semantics to MM API clients. + status := http.StatusBadRequest + switch { + case resp.StatusCode == http.StatusTooManyRequests, resp.StatusCode == http.StatusServiceUnavailable: + status = resp.StatusCode + case resp.StatusCode >= 500: + status = http.StatusBadGateway + } + return resp, model.NewAppError("DoActionRequest", "api.post.do_action.action_integration.app_error", nil, fmt.Sprintf("status=%v", resp.StatusCode), status) } return resp, nil diff --git a/server/channels/app/integration_action_test.go b/server/channels/app/integration_action_test.go index 786ff071392..f0e9cc767a0 100644 --- a/server/channels/app/integration_action_test.go +++ b/server/channels/app/integration_action_test.go @@ -1636,6 +1636,74 @@ func TestDoActionRequest(t *testing.T) { resp.Body.Close() }) + t.Run("should preserve 429 status code from plugin", func(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusTooManyRequests) + _, _ = w.Write([]byte("Rate limited")) + })) + defer ts.Close() + + requestBody := []byte(`{"test": "data"}`) + resp, err := th.App.DoActionRequest(th.Context, ts.URL, requestBody) + require.NotNil(t, err) + require.NotNil(t, resp) + assert.Equal(t, http.StatusTooManyRequests, resp.StatusCode) + assert.Equal(t, http.StatusTooManyRequests, err.StatusCode) + assert.Contains(t, err.Error(), "status=429") + resp.Body.Close() + }) + + t.Run("should preserve 503 status code from plugin", func(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusServiceUnavailable) + _, _ = w.Write([]byte("Service unavailable")) + })) + defer ts.Close() + + requestBody := []byte(`{"test": "data"}`) + resp, err := th.App.DoActionRequest(th.Context, ts.URL, requestBody) + require.NotNil(t, err) + require.NotNil(t, resp) + assert.Equal(t, http.StatusServiceUnavailable, resp.StatusCode) + assert.Equal(t, http.StatusServiceUnavailable, err.StatusCode) + assert.Contains(t, err.Error(), "status=503") + resp.Body.Close() + }) + + t.Run("should map other 5xx status codes to 502 Bad Gateway", func(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte("Internal error")) + })) + defer ts.Close() + + requestBody := []byte(`{"test": "data"}`) + resp, err := th.App.DoActionRequest(th.Context, ts.URL, requestBody) + require.NotNil(t, err) + require.NotNil(t, resp) + assert.Equal(t, http.StatusInternalServerError, resp.StatusCode) + assert.Equal(t, http.StatusBadGateway, err.StatusCode) + assert.Contains(t, err.Error(), "status=500") + resp.Body.Close() + }) + + t.Run("should sanitize other 4xx status codes to 400", func(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte("Not found")) + })) + defer ts.Close() + + requestBody := []byte(`{"test": "data"}`) + resp, err := th.App.DoActionRequest(th.Context, ts.URL, requestBody) + require.NotNil(t, err) + require.NotNil(t, resp) + assert.Equal(t, http.StatusNotFound, resp.StatusCode) + assert.Equal(t, http.StatusBadRequest, err.StatusCode) + assert.Contains(t, err.Error(), "status=404") + resp.Body.Close() + }) + t.Run("should handle invalid URL", func(t *testing.T) { requestBody := []byte(`{"test": "data"}`) resp, err := th.App.DoActionRequest(th.Context, "invalid-url", requestBody) @@ -3994,7 +4062,7 @@ func TestExecuteDialogAction(t *testing.T) { _, appErr := th.App.ExecuteDialogAction(th.Context, th.BasicUser.Id, req) require.NotNil(t, appErr) - // DoActionRequest maps non-200 upstream responses to a 400 AppError. - assert.Equal(t, http.StatusBadRequest, appErr.StatusCode) + // DoActionRequest maps upstream 5xx (other than 429/503) to 502 Bad Gateway. + assert.Equal(t, http.StatusBadGateway, appErr.StatusCode) }) }