diff --git a/agent/cmd/chat.go b/agent/cmd/chat.go index a21852e20..8e3faef1b 100644 --- a/agent/cmd/chat.go +++ b/agent/cmd/chat.go @@ -99,15 +99,37 @@ func registerChat(api *callable.Server, s *chatRoutes) { default: return nil, invalid("invalid provider") } - // A provider rate limit is the user acting too fast, not a server error. - var status *clients.HTTPError - if errors.As(err, &status) && status.Status == http.StatusTooManyRequests { - return nil, &callable.Error{Code: "resource-exhausted", Message: "too many requests, try again shortly"} + if rejection := providerRejection(r.Provider, name, err); rejection != nil { + return nil, rejection } return result, err }) } } + +// providerRejection classifies a provider's HTTP error that is about the user's +// request rather than the agent. Every chat action and provider shares it, so a +// new refusal wording never surfaces as an internal error. The request shapes +// these actions send are pinned by tests, so an explained rejection is a refusal. +func providerRejection(p, action string, err error) error { + var status *clients.HTTPError + if !errors.As(err, &status) { + return nil + } + switch status.Status { + case http.StatusTooManyRequests: + // A provider rate limit is the user acting too fast, not a server error. + return &callable.Error{Code: "resource-exhausted", Message: "too many requests, try again shortly"} + case http.StatusBadRequest, http.StatusNotFound, http.StatusConflict, http.StatusUnprocessableEntity: + if status.Explained { + return &callable.Error{Code: "failed-precondition", Message: providerNames[p] + " rejected this " + action + " request."} + } + } + return nil +} + +var providerNames = map[string]string{"twitch": "Twitch", "kick": "Kick", "youtube": "YouTube"} + func (r chatRequest) validate(action string) error { if r.Provider == "" { return invalid("missing provider") diff --git a/agent/cmd/chat_moderation_test.go b/agent/cmd/chat_moderation_test.go index ff2641ec1..8042e8fd9 100644 --- a/agent/cmd/chat_moderation_test.go +++ b/agent/cmd/chat_moderation_test.go @@ -22,20 +22,18 @@ func TestTwitchModerationRejectionReporting(t *testing.T) { {"protected user", "ban", "user may not be banned", 400, 400, "FAILED_PRECONDITION", "Twitch does not allow banning this user."}, {"protected timeout", "timeout", "user may not be put in a timeout", 400, 400, "FAILED_PRECONDITION", "Twitch does not allow timing out this user."}, {"already unbanned", "unban", "user is not banned", 400, 400, "FAILED_PRECONDITION", "This user is not banned."}, + {"unban refusal naming its parameter", "unban", "The user specified in the user_id query parameter is not banned or timed out.", 400, 400, "FAILED_PRECONDITION", "Twitch rejected this unban request."}, {"unrecognized unban rejection", "unban", "private-provider-detail", 400, 400, "FAILED_PRECONDITION", "Twitch rejected this unban request."}, {"empty unban rejection", "unban", "", 400, 500, "INTERNAL", "INTERNAL"}, {"blank unban rejection", "unban", " ", 400, 500, "INTERNAL", "INTERNAL"}, {"unban server failure", "unban", "user is not banned", 503, 500, "INTERNAL", "INTERNAL"}, {"unban forbidden", "unban", "private-provider-detail", 403, 500, "INTERNAL", "INTERNAL"}, {"documented wording", "ban", "The user specified in the user_id field is already banned.", 400, 400, "FAILED_PRECONDITION", "This user is already banned."}, - {"missing provider parameter", "ban", "user_id is required", 400, 500, "INTERNAL", "INTERNAL"}, {"unrecognized ban rejection", "ban", "private-provider-detail", 400, 400, "FAILED_PRECONDITION", "Twitch rejected this ban request."}, {"unrecognized timeout rejection", "timeout", "private-provider-detail", 400, 400, "FAILED_PRECONDITION", "Twitch rejected this timeout request."}, {"not a matching reason", "ban", "user is already banned private-provider-detail", 400, 400, "FAILED_PRECONDITION", "Twitch rejected this ban request."}, {"empty ban rejection", "ban", "", 400, 500, "INTERNAL", "INTERNAL"}, - {"missing query parameter", "ban", `Missing required parameter "moderator_id"`, 400, 500, "INTERNAL", "INTERNAL"}, - {"malformed parameter", "timeout", "The value in the duration field is not valid.", 400, 500, "INTERNAL", "INTERNAL"}, - {"malformed unban", "unban", "The user_id query parameter is required.", 400, 500, "INTERNAL", "INTERNAL"}, + {"ban refusal naming its field", "ban", "The user_id field may not be banned while required by policy.", 400, 400, "FAILED_PRECONDITION", "Twitch rejected this ban request."}, {"forbidden", "ban", "private-provider-detail", 403, 500, "INTERNAL", "INTERNAL"}, {"server failure with known text", "ban", "user is already banned", 503, 500, "INTERNAL", "INTERNAL"}, {"rate limited", "ban", "private-provider-detail", 429, 429, "RESOURCE_EXHAUSTED", "too many requests, try again shortly"}, @@ -145,3 +143,48 @@ func TestTwitchTimeoutBoundsCheckedLocally(t *testing.T) { }) } } + +// Every chat action shares one rejection policy, so a refusal Twitch or Kick +// explains is a user-facing rejection on any endpoint, while an unexplained +// rejection or a server failure still alerts. +func TestProviderRejectionsAcrossEndpoints(t *testing.T) { + for _, tc := range []struct { + name, action, provider string + status int + body string + wantStatus int + wantMessage string + }{ + {"twitch delete refusal", "deleteMessage", "twitch", 400, `{"message":"private-provider-detail"}`, 400, "Twitch rejected this deleteMessage request."}, + {"twitch delete missing message", "deleteMessage", "twitch", 404, `{"message":"private-provider-detail"}`, 400, "Twitch rejected this deleteMessage request."}, + {"twitch clear refusal", "clear", "twitch", 400, `{"message":"private-provider-detail"}`, 400, "Twitch rejected this clear request."}, + {"kick ban refusal", "ban", "kick", 400, `{"message":"private-provider-detail"}`, 400, "Kick rejected this ban request."}, + {"kick unban refusal", "unban", "kick", 422, `{"message":"private-provider-detail"}`, 400, "Kick rejected this unban request."}, + {"kick delete conflict", "deleteMessage", "kick", 409, `{"message":"private-provider-detail"}`, 400, "Kick rejected this deleteMessage request."}, + {"unexplained rejection", "deleteMessage", "twitch", 400, `{"message":" "}`, 500, "INTERNAL"}, + {"non-JSON rejection", "ban", "kick", 400, `private-provider-detail`, 500, "INTERNAL"}, + {"non-string message", "clear", "twitch", 400, `{"message":42}`, 500, "INTERNAL"}, + {"explained server failure", "deleteMessage", "twitch", 502, `{"message":"private-provider-detail"}`, 500, "INTERNAL"}, + } { + t.Run(tc.name, func(t *testing.T) { + core, logs := observer.New(zap.InfoLevel) + defer zap.ReplaceGlobals(zap.New(observability.Wrap(core)))() + s, _ := newMock(func(r *http.Request) (*http.Response, error) { + return respond(tc.status, tc.body), nil + }) + code, body := call(t, s, tc.action, map[string]any{"provider": tc.provider, "channelId": "123", "userId": "456", "messageId": tc.provider + ":message"}, true) + if code != tc.wantStatus || object(body["error"])["message"] != tc.wantMessage { + t.Fatal(code, body) + } + entries := logs.FilterMessage("http request").All() + if len(entries) != 1 || (tc.wantStatus < 500) != (entries[0].Level == zap.InfoLevel) { + t.Fatalf("rejection alerting changed: %v", entries) + } + logged, _ := json.Marshal(entries[0].ContextMap()) + encoded, _ := json.Marshal(body) + if strings.Contains(string(logged)+string(encoded), "private-provider-detail") { + t.Fatal("provider details leaked") + } + }) + } +} diff --git a/agent/internal/clients/client.go b/agent/internal/clients/client.go index 3197d6649..cbb45aa11 100644 --- a/agent/internal/clients/client.go +++ b/agent/internal/clients/client.go @@ -26,17 +26,32 @@ type Response struct { } // HTTPError deliberately excludes response bodies, which may contain credentials. -type HTTPError struct{ Status int } +// Explained records only whether the provider gave a reason for the rejection. +type HTTPError struct { + Status int + Explained bool +} func (e *HTTPError) Error() string { return fmt.Sprintf("provider returned HTTP %d", e.Status) } func (r *Response) Err() error { if r.StatusCode < 200 || r.StatusCode >= 300 { - return &HTTPError{Status: r.StatusCode} + return &HTTPError{Status: r.StatusCode, Explained: explained(r.Body)} } return nil } +// explained reports whether a JSON error body carries a non-blank "message", +// the field Twitch and Kick use to say why they rejected a request. +func explained(body []byte) bool { + var result struct{ Message any } + if json.Unmarshal(body, &result) != nil { + return false + } + message, _ := result.Message.(string) + return strings.TrimSpace(message) != "" +} + func (r *Response) Decode(out any) error { if out != nil && json.Unmarshal(r.Body, out) != nil { return errors.New("invalid provider response") diff --git a/agent/internal/clients/twitch/chat.go b/agent/internal/clients/twitch/chat.go index a01c56432..7e237b53c 100644 --- a/agent/internal/clients/twitch/chat.go +++ b/agent/internal/clients/twitch/chat.go @@ -3,7 +3,6 @@ package twitch import ( "context" "net/url" - "regexp" "strings" "time" ) @@ -68,28 +67,20 @@ func (c *Client) Ban(ctx context.Context, channel, moderator, user string, durat if duration != 0 { data["duration"], data["reason"] = duration, reason } - action := "ban" - if duration != 0 { - action = "timeout" - } - return c.writeBan(ctx, action, "POST", moderationPath("/moderation/bans", channel, moderator, nil), map[string]any{"data": data}) + return c.writeBan(ctx, "POST", moderationPath("/moderation/bans", channel, moderator, nil), map[string]any{"data": data}) } func (c *Client) Unban(ctx context.Context, channel, moderator, user string) error { - return c.writeBan(ctx, "unban", "DELETE", moderationPath("/moderation/bans", channel, moderator, url.Values{"user_id": {user}}), nil) + return c.writeBan(ctx, "DELETE", moderationPath("/moderation/bans", channel, moderator, url.Values{"user_id": {user}}), nil) } -// ModerationRejected is a refusal of a moderation action by Twitch. -// Other provider failures retain their redacted HTTP error for reporting. +// ModerationRejected is a recognized refusal of a moderation action by Twitch. +// Other refusals keep their redacted HTTP error and are classified by the caller. type ModerationRejected struct{ Message string } func (e *ModerationRejected) Error() string { return e.Message } -// Helix describes a malformed request in terms of its parameters, while a -// refusal describes the target user. Malformed requests are agent bugs. -var malformedRequest = regexp.MustCompile(`parameter|required|malformed|valid`) - -func (c *Client) writeBan(ctx context.Context, action, method, path string, body any) error { +func (c *Client) writeBan(ctx context.Context, method, path string, body any) error { response, err := c.Do(ctx, method, path, body) if err != nil { return err @@ -108,13 +99,6 @@ func (c *Client) writeBan(ctx context.Context, action, method, path string, body case "user may not be put in a timeout", "the user specified in the user_id field may not be put in a timeout": return &ModerationRejected{Message: "Twitch does not allow timing out this user."} } - // The request carries only resolved IDs and locally validated timeout - // fields, so any other rejection is Twitch refusing the action, even - // when it changes its wording. Never expose raw text. - message := strings.ToLower(strings.TrimSpace(result.Message)) - if message != "" && !malformedRequest.MatchString(message) { - return &ModerationRejected{Message: "Twitch rejected this " + action + " request."} - } } } return response.Err()