From 2949028dcb0cf7c3c904fc9c8d66d34b3c2a0c49 Mon Sep 17 00:00:00 2001 From: Jakub Domeracki Date: Wed, 6 May 2026 10:25:12 +0200 Subject: [PATCH] fix(coderd): enforce chat owner check on processing handlers (#24921) --- coderd/exp_chats.go | 62 +++++++++++++ coderd/exp_chats_test.go | 186 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 248 insertions(+) diff --git a/coderd/exp_chats.go b/coderd/exp_chats.go index 4f7f9eb7f4..94a80188c4 100644 --- a/coderd/exp_chats.go +++ b/coderd/exp_chats.go @@ -2822,6 +2822,19 @@ func (api *API) postChatMessages(rw http.ResponseWriter, r *http.Request) { return } + // Only the chat owner may send messages. Org admins pass the + // RBAC check above (org-level ActionUpdate), but chat + // processing forwards the *owner's* credentials (OIDC tokens, + // provider API keys) to external services. Allowing a + // non-owner to trigger processing would leak the owner's + // tokens to MCP servers the caller controls. + if apiKey.UserID != chat.OwnerID { + httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ + Message: "Only the chat owner may send messages.", + }) + return + } + if chat.Archived { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Cannot send messages to an archived chat.", @@ -2999,6 +3012,15 @@ func (api *API) patchChatMessage(rw http.ResponseWriter, r *http.Request) { apiKey := httpmw.APIKey(r) chat := httpmw.ChatParam(r) + // Only the chat owner may edit messages. See postChatMessages + // for the security rationale. + if apiKey.UserID != chat.OwnerID { + httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ + Message: "Only the chat owner may edit messages.", + }) + return + } + if chat.Archived { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Cannot edit messages in an archived chat.", @@ -3137,6 +3159,15 @@ func (api *API) promoteChatQueuedMessage(rw http.ResponseWriter, r *http.Request return } + // Only the chat owner may promote messages. See + // postChatMessages for the security rationale. + if apiKey.UserID != chat.OwnerID { + httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ + Message: "Only the chat owner may promote queued messages.", + }) + return + } + if chat.Archived { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Cannot promote queued messages in an archived chat.", @@ -3423,12 +3454,23 @@ func (api *API) interruptChat(rw http.ResponseWriter, r *http.Request) { //nolint:revive // HTTP handler writes to ResponseWriter. func (api *API) regenerateChatTitle(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() + apiKey := httpmw.APIKey(r) chat := httpmw.ChatParam(r) if !api.Authorize(r, policy.ActionUpdate, chat.RBACObject()) { httpapi.ResourceNotFound(rw) return } + + // Only the chat owner may regenerate titles. See + // postChatMessages for the security rationale. + if apiKey.UserID != chat.OwnerID { + httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ + Message: "Only the chat owner may regenerate the title.", + }) + return + } + if api.chatDaemon == nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Chat processor is unavailable.", @@ -3465,12 +3507,23 @@ func (api *API) regenerateChatTitle(rw http.ResponseWriter, r *http.Request) { //nolint:revive // HTTP handler writes to ResponseWriter. func (api *API) proposeChatTitle(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() + apiKey := httpmw.APIKey(r) chat := httpmw.ChatParam(r) if !api.Authorize(r, policy.ActionUpdate, chat.RBACObject()) { httpapi.ResourceNotFound(rw) return } + + // Only the chat owner may propose titles. See + // postChatMessages for the security rationale. + if apiKey.UserID != chat.OwnerID { + httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ + Message: "Only the chat owner may propose a title.", + }) + return + } + if api.chatDaemon == nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Chat processor is unavailable.", @@ -7945,6 +7998,15 @@ func (api *API) postChatToolResults(rw http.ResponseWriter, r *http.Request) { return } + // Only the chat owner may submit tool results. See + // postChatMessages for the security rationale. + if apiKey.UserID != chat.OwnerID { + httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ + Message: "Only the chat owner may submit tool results.", + }) + return + } + if chat.Archived { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Cannot submit tool results to an archived chat.", diff --git a/coderd/exp_chats_test.go b/coderd/exp_chats_test.go index d15083d6b7..d9c36fc6e8 100644 --- a/coderd/exp_chats_test.go +++ b/coderd/exp_chats_test.go @@ -13471,3 +13471,189 @@ func requireSDKError(t *testing.T, err error, expectedStatus int) *codersdk.Erro require.Equal(t, expectedStatus, sdkErr.StatusCode()) return sdkErr } + +// TestChatOwnerOnlyWriteHandlers verifies that only the chat owner can +// call handlers that trigger chat processing. Org admins pass the RBAC +// ActionUpdate check (org-level permission) but must still be blocked +// because processing forwards the *owner's* credentials to external +// services. +func TestChatOwnerOnlyWriteHandlers(t *testing.T) { + t.Parallel() + + // setupOrgAdminAndOwnerChat creates an org-admin user and a chat + // owned by the first (site-admin) user. Returns both clients, + // the chat, and the DB handle. + setupOrgAdminAndOwnerChat := func(t *testing.T) ( + ownerClient *codersdk.ExperimentalClient, + adminClient *codersdk.ExperimentalClient, + chat codersdk.Chat, + db database.Store, + ) { + t.Helper() + + ctx := testutil.Context(t, testutil.WaitLong) + ownerClient, db = newChatClientWithDatabase(t) + firstUser := coderdtest.CreateFirstUser(t, ownerClient.Client) + _ = createChatModelConfig(t, ownerClient) + + // Create a chat owned by the first user. + var err error + chat, err = ownerClient.CreateChat(ctx, codersdk.CreateChatRequest{ + OrganizationID: firstUser.OrganizationID, + Content: []codersdk.ChatInputPart{{ + Type: codersdk.ChatInputPartTypeText, + Text: "owner chat for authz test", + }}, + }) + require.NoError(t, err) + + // Create an org admin in the same org. + orgAdminRaw, _ := coderdtest.CreateAnotherUser( + t, + ownerClient.Client, + firstUser.OrganizationID, + rbac.ScopedRoleOrgAdmin(firstUser.OrganizationID), + rbac.ScopedRoleAgentsAccess(firstUser.OrganizationID), + ) + adminClient = codersdk.NewExperimentalClient(orgAdminRaw) + return ownerClient, adminClient, chat, db + } + + t.Run("PostChatMessages", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + _, adminClient, chat, _ := setupOrgAdminAndOwnerChat(t) + + _, err := adminClient.CreateChatMessage(ctx, chat.ID, codersdk.CreateChatMessageRequest{ + Content: []codersdk.ChatInputPart{{ + Type: codersdk.ChatInputPartTypeText, + Text: "org admin should not be able to send this", + }}, + }) + sdkErr := requireSDKError(t, err, http.StatusForbidden) + require.Contains(t, sdkErr.Message, "Only the chat owner") + }) + + t.Run("PatchChatMessage", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + ownerClient, adminClient, chat, _ := setupOrgAdminAndOwnerChat(t) + + // Fetch the first user message to get a valid message ID. + messagesResult, err := ownerClient.GetChatMessages(ctx, chat.ID, nil) + require.NoError(t, err) + var userMessageID int64 + for _, msg := range messagesResult.Messages { + if msg.Role == codersdk.ChatMessageRoleUser { + userMessageID = msg.ID + break + } + } + require.NotZero(t, userMessageID) + + _, err = adminClient.EditChatMessage(ctx, chat.ID, userMessageID, codersdk.EditChatMessageRequest{ + Content: []codersdk.ChatInputPart{{ + Type: codersdk.ChatInputPartTypeText, + Text: "org admin should not be able to edit this", + }}, + }) + sdkErr := requireSDKError(t, err, http.StatusForbidden) + require.Contains(t, sdkErr.Message, "Only the chat owner") + }) + + t.Run("PromoteChatQueuedMessage", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + _, adminClient, chat, db := setupOrgAdminAndOwnerChat(t) + + // Insert a queued message directly in the DB. + queuedContent, err := json.Marshal([]codersdk.ChatMessagePart{ + codersdk.ChatMessageText("queued"), + }) + require.NoError(t, err) + queuedMessage, err := db.InsertChatQueuedMessage( + dbauthz.AsSystemRestricted(ctx), + database.InsertChatQueuedMessageParams{ + ChatID: chat.ID, + Content: queuedContent, + }, + ) + require.NoError(t, err) + + // Org admin tries to promote. + promoteRes, err := adminClient.Request( + ctx, + http.MethodPost, + fmt.Sprintf("/api/experimental/chats/%s/queue/%d/promote", chat.ID, queuedMessage.ID), + nil, + ) + require.NoError(t, err) + defer promoteRes.Body.Close() + require.Equal(t, http.StatusForbidden, promoteRes.StatusCode) + }) + + t.Run("SubmitToolResults", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + _, adminClient, chat, _ := setupOrgAdminAndOwnerChat(t) + + err := adminClient.SubmitToolResults(ctx, chat.ID, codersdk.SubmitToolResultsRequest{ + Results: []codersdk.ToolResult{{ + ToolCallID: "call_forbidden", + Output: json.RawMessage(`"forbidden"`), + }}, + }) + sdkErr := requireSDKError(t, err, http.StatusForbidden) + require.Contains(t, sdkErr.Message, "Only the chat owner") + }) + + t.Run("RegenerateChatTitle", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + _, adminClient, chat, _ := setupOrgAdminAndOwnerChat(t) + + _, err := adminClient.RegenerateChatTitle(ctx, chat.ID) + sdkErr := requireSDKError(t, err, http.StatusForbidden) + require.Contains(t, sdkErr.Message, "Only the chat owner") + }) + + t.Run("ProposeChatTitle", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + _, adminClient, chat, _ := setupOrgAdminAndOwnerChat(t) + + _, err := adminClient.ProposeChatTitle(ctx, chat.ID) + sdkErr := requireSDKError(t, err, http.StatusForbidden) + require.Contains(t, sdkErr.Message, "Only the chat owner") + }) + + // Verify the owner can still operate normally. + t.Run("OwnerCanSendMessages", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + ownerClient, _, chat, _ := setupOrgAdminAndOwnerChat(t) + + _, err := ownerClient.CreateChatMessage(ctx, chat.ID, codersdk.CreateChatMessageRequest{ + Content: []codersdk.ChatInputPart{{ + Type: codersdk.ChatInputPartTypeText, + Text: "owner should succeed", + }}, + }) + // The message is accepted (no 403). It may fail downstream + // (e.g. no running LLM) but that is not a 403. + if err != nil { + var sdkErr *codersdk.Error + if xerrors.As(err, &sdkErr) { + require.NotEqual(t, http.StatusForbidden, sdkErr.StatusCode(), + "owner must not receive 403") + } + } + }) +}