From ea00d2d39632c90883bdbbd991a1afe403198e8c Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Mon, 20 Apr 2026 04:33:35 -0700 Subject: [PATCH] fix(coderd): enforce workspace authz on watchChatGit (#24477) `watchChatGit` proxies a live websocket to the workspace agent's git watcher (`/api/v0/git/watch`), streaming repository diffs back through the chat stream. Before this change it only enforced `chat:read` (via `ExtractChatParam`) plus an implicit `workspace:read` from the dbauthz wrapper on `GetWorkspaceAgentsInLatestBuildByWorkspaceID`. The sibling `watchChatDesktop` handler already fetches the workspace and requires `policy.ActionApplicationConnect` or `policy.ActionSSH` before dialing. Built-in roles like **Template Admin** and **Org Admin** grant `workspace:read` without SSH/ApplicationConnect, and **Owner** also loses both under `DisableOwnerWorkspaceExec`. A chat owner whose exec-level workspace access was revoked *after* the chat was bound could therefore keep streaming repository content from the workspace agent through the chat's git-watch endpoint. Mirror `watchChatDesktop`: fetch the workspace and require `ApplicationConnect || SSH` before any agent-tunnel activity. Adds one real-coderdtest regression test (`TestWatchChatGitAuthz`) that demotes the chat's owner to template-admin after binding and asserts the git-watch endpoint returns 403; the mock-based `TestWatchChatGit` in `coderd/workspaceagents_internal_test.go` continues to cover the no-workspace / disconnected-agent / websocket-proxy paths. Fixes CODAGT-184. --- coderd/exp_chats.go | 74 ++++++++---- coderd/exp_chats_test.go | 74 ++++++++++++ coderd/workspaceagents_internal_test.go | 144 +++++++++++++++++++++++- 3 files changed, 268 insertions(+), 24 deletions(-) diff --git a/coderd/exp_chats.go b/coderd/exp_chats.go index b782c0e48b..464ee12eb4 100644 --- a/coderd/exp_chats.go +++ b/coderd/exp_chats.go @@ -1634,6 +1634,57 @@ func (api *API) getChatMessages(rw http.ResponseWriter, r *http.Request) { }) } +// authorizeChatWorkspaceExec enforces the workspace-level permissions +// shared by the chat stream endpoints that proxy a live websocket into +// the workspace agent (currently /stream/git and /stream/desktop). +// +// The chat row only authorizes the chat owner, so callers also need +// exec-level access (ApplicationConnect or SSH) to the bound workspace. +// The chat owner's workspace permissions may have been revoked after +// the chat was bound; skipping this check enabled CODAGT-184. +// +// On any failure the response is written and ok=false is returned. +// +//nolint:revive // HTTP handler writes to ResponseWriter. +func (api *API) authorizeChatWorkspaceExec( + rw http.ResponseWriter, + r *http.Request, + chat database.Chat, + noWorkspaceMessage string, +) (database.Workspace, bool) { + ctx := r.Context() + + if !chat.WorkspaceID.Valid { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: noWorkspaceMessage, + }) + return database.Workspace{}, false + } + + workspace, err := api.Database.GetWorkspaceByID(ctx, chat.WorkspaceID.UUID) + if httpapi.Is404Error(err) { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Chat workspace not found.", + }) + return database.Workspace{}, false + } + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching chat workspace.", + Detail: err.Error(), + }) + return database.Workspace{}, false + } + + if !api.Authorize(r, policy.ActionApplicationConnect, workspace) && + !api.Authorize(r, policy.ActionSSH, workspace) { + httpapi.Forbidden(rw) + return database.Workspace{}, false + } + + return workspace, true +} + // EXPERIMENTAL: this endpoint is experimental and is subject to change. // //nolint:revive // HTTP handler writes to ResponseWriter. @@ -1644,10 +1695,7 @@ func (api *API) watchChatGit(rw http.ResponseWriter, r *http.Request) { logger = api.Logger.Named("chat_git_watcher").With(slog.F("chat_id", chat.ID)) ) - if !chat.WorkspaceID.Valid { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Chat has no workspace to watch.", - }) + if _, ok := api.authorizeChatWorkspaceExec(rw, r, chat, "Chat has no workspace to watch."); !ok { return } @@ -1792,23 +1840,7 @@ func (api *API) watchChatDesktop(rw http.ResponseWriter, r *http.Request) { logger = api.Logger.Named("chat_desktop").With(slog.F("chat_id", chat.ID)) ) - if !chat.WorkspaceID.Valid { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Chat has no workspace.", - }) - return - } - - workspace, err := api.Database.GetWorkspaceByID(ctx, chat.WorkspaceID.UUID) - if err != nil { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Chat workspace not found.", - }) - return - } - if !api.Authorize(r, policy.ActionApplicationConnect, workspace) && - !api.Authorize(r, policy.ActionSSH, workspace) { - httpapi.Forbidden(rw) + if _, ok := api.authorizeChatWorkspaceExec(rw, r, chat, "Chat has no workspace."); !ok { return } diff --git a/coderd/exp_chats_test.go b/coderd/exp_chats_test.go index c53defb087..2db2f9670d 100644 --- a/coderd/exp_chats_test.go +++ b/coderd/exp_chats_test.go @@ -8254,6 +8254,80 @@ func TestWatchChatDesktop(t *testing.T) { }) } +// TestWatchChatGitAuthz is the regression test for CODAGT-184. The +// git-watcher handler opens a bidirectional websocket into the +// workspace agent and streams repository diffs; before the fix it only +// enforced chat:read, so a chat owner who lost workspace SSH / +// application-connect access (e.g. by being demoted from owner to +// template-admin after the chat was bound) could keep exfiltrating +// repository contents. +// +// Other behaviors (no-workspace 400, websocket proxy plumbing, +// disconnected-agent 400) are covered by the mock-based TestWatchChatGit +// in coderd/workspaceagents_internal_test.go. +func TestWatchChatGitAuthz(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + + // adminClient = first user (site: owner). Creates the chat below + // and is demoted after the chat is bound. + adminClient, db := newChatClientWithDatabase(t) + firstUser := coderdtest.CreateFirstUser(t, adminClient.Client) + _ = createChatModelConfig(t, adminClient) + + // A second owner is needed to run UpdateUserRoles on the first + // user, since the server refuses self-demotion. + secondAdminClient, _ := coderdtest.CreateAnotherUser(t, adminClient.Client, firstUser.OrganizationID, rbac.RoleOwner()) + + // The workspace owner is a distinct user so that stripping + // adminClient's site roles fully removes its workspace + // SSH/ApplicationConnect. If the workspace were owned by + // adminClient, the user would retain SSH via the org-member role + // regardless of site-role demotion. + _, workspaceOwner := coderdtest.CreateAnotherUser(t, adminClient.Client, firstUser.OrganizationID) + + workspaceBuild := dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{ + OrganizationID: firstUser.OrganizationID, + OwnerID: workspaceOwner.ID, + }).WithAgent().Do() + + chat, err := adminClient.CreateChat(ctx, codersdk.CreateChatRequest{ + OrganizationID: firstUser.OrganizationID, + Content: []codersdk.ChatInputPart{ + {Type: codersdk.ChatInputPartTypeText, Text: "codagt-184"}, + }, + }) + require.NoError(t, err) + + // Bind the chat to the workspace while adminClient still has + // site-wide workspace:ssh via the owner role. + err = adminClient.UpdateChat(ctx, chat.ID, codersdk.UpdateChatRequest{ + WorkspaceID: &workspaceBuild.Workspace.ID, + }) + require.NoError(t, err) + + // Demote adminClient via the second owner. template-admin grants + // workspace:read (site) but not workspace:ssh or + // workspace:application_connect; agents-access preserves + // chat:read|create|update|delete on chats the user owns, so the + // demoted user still passes ExtractChatParam for their own chat. + _, err = secondAdminClient.UpdateUserRoles(ctx, firstUser.UserID.String(), codersdk.UpdateRoles{ + Roles: []string{rbac.RoleTemplateAdmin().String(), rbac.RoleAgentsAccess().String()}, + }) + require.NoError(t, err) + + res, err := adminClient.Request( + ctx, + http.MethodGet, + fmt.Sprintf("/api/experimental/chats/%s/stream/git", chat.ID), + nil, + ) + require.NoError(t, err) + defer res.Body.Close() + require.Equal(t, http.StatusForbidden, res.StatusCode) +} + func createChatModelConfig(t *testing.T, client *codersdk.ExperimentalClient) codersdk.ChatModelConfig { t.Helper() diff --git a/coderd/workspaceagents_internal_test.go b/coderd/workspaceagents_internal_test.go index a2203cdf6d..ccbfff4330 100644 --- a/coderd/workspaceagents_internal_test.go +++ b/coderd/workspaceagents_internal_test.go @@ -18,13 +18,17 @@ import ( "github.com/google/uuid" "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" + "golang.org/x/xerrors" "cdr.dev/slog/v3" "cdr.dev/slog/v3/sloggers/slogtest" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbmock" "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/httpmw" + "github.com/coder/coder/v2/coderd/rbac" + "github.com/coder/coder/v2/coderd/rbac/policy" "github.com/coder/coder/v2/coderd/workspaceapps/appurl" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/codersdk/workspacesdk" @@ -69,6 +73,93 @@ func (c *channelCloser) Close() error { return nil } +// mockAuthorizer is a permissive rbac.Authorizer used by the mock-based +// handler tests in this file. Authorization behavior is tested +// separately in coderd/exp_chats_test.go against a real coderdtest +// server. +type mockAuthorizer struct{} + +func (*mockAuthorizer) Authorize(context.Context, rbac.Subject, policy.Action, rbac.Object) error { + return nil +} + +func (*mockAuthorizer) Prepare(context.Context, rbac.Subject, policy.Action, string) (rbac.PreparedAuthorized, error) { + //nolint:nilnil + return nil, nil +} + +var _ rbac.Authorizer = (*mockAuthorizer)(nil) + +// injectSystemActor is a test-only middleware that seeds an RBAC actor +// into the request context so handlers using api.Authorize do not panic +// via httpmw.UserAuthorization. Pair it with mockAuthorizer to +// short-circuit authorization in tests that focus on plumbing rather +// than RBAC. +func injectSystemActor(next http.Handler) http.Handler { + return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + next.ServeHTTP(rw, r.WithContext(dbauthz.AsSystemRestricted(r.Context()))) + }) +} + +// runWatchChatGitWorkspaceLookupTest exercises the GetWorkspaceByID +// error branches in authorizeChatWorkspaceExec. The chat middleware +// always succeeds; the workspace lookup returns workspaceErr, and the +// handler is expected to respond with wantStatus. +func runWatchChatGitWorkspaceLookupTest(t *testing.T, workspaceErr error, wantStatus int) { + t.Helper() + + var ( + ctx = testutil.Context(t, testutil.WaitShort) + logger = slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug).Named("coderd") + + mCtrl = gomock.NewController(t) + mDB = dbmock.NewMockStore(mCtrl) + + chatID = uuid.New() + workspaceID = uuid.New() + + r = chi.NewMux() + + api = API{ + ctx: ctx, + Options: &Options{ + AgentInactiveDisconnectTimeout: testutil.WaitShort, + Database: mDB, + Logger: logger, + DeploymentValues: &codersdk.DeploymentValues{}, + }, + HTTPAuth: &HTTPAuthorizer{ + Authorizer: &mockAuthorizer{}, + Logger: logger, + }, + } + ) + + mDB.EXPECT().GetChatByID(gomock.Any(), chatID).Return(database.Chat{ + ID: chatID, + OwnerID: uuid.New(), + WorkspaceID: uuid.NullUUID{UUID: workspaceID, Valid: true}, + }, nil) + + mDB.EXPECT().GetWorkspaceByID(gomock.Any(), workspaceID).Return(database.Workspace{}, workspaceErr) + + r.With(injectSystemActor, httpmw.ExtractChatParam(mDB)). + Get("/chats/{chat}/stream/git", api.watchChatGit) + + srv := httptest.NewServer(r) + defer srv.Close() + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, + fmt.Sprintf("%s/chats/%s/stream/git", srv.URL, chatID), nil) + require.NoError(t, err) + + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + + require.Equal(t, wantStatus, resp.StatusCode) +} + func TestWatchChatGit(t *testing.T) { t.Parallel() @@ -128,6 +219,23 @@ func TestWatchChatGit(t *testing.T) { require.Equal(t, http.StatusBadRequest, resp.StatusCode) }) + t.Run("WorkspaceLookupErrors", func(t *testing.T) { + t.Parallel() + + // Covers the GetWorkspaceByID branches in + // authorizeChatWorkspaceExec: 404-class errors return 400, + // other errors return 500. + t.Run("NotFound", func(t *testing.T) { + t.Parallel() + runWatchChatGitWorkspaceLookupTest(t, sql.ErrNoRows, http.StatusBadRequest) + }) + + t.Run("InternalError", func(t *testing.T) { + t.Parallel() + runWatchChatGitWorkspaceLookupTest(t, xerrors.New("simulated db failure"), http.StatusInternalServerError) + }) + }) + t.Run("UnauthorizedUsersCannotWatch", func(t *testing.T) { t.Parallel() @@ -215,6 +323,10 @@ func TestWatchChatGit(t *testing.T) { DeploymentValues: &codersdk.DeploymentValues{}, TailnetCoordinator: tailnettest.NewFakeCoordinator(), }, + HTTPAuth: &HTTPAuthorizer{ + Authorizer: &mockAuthorizer{}, + Logger: logger, + }, } ) @@ -228,6 +340,12 @@ func TestWatchChatGit(t *testing.T) { WorkspaceID: uuid.NullUUID{UUID: workspaceID, Valid: true}, }, nil) + // And: Return the workspace so the handler's + // workspace-level authz check can run. + mDB.EXPECT().GetWorkspaceByID(gomock.Any(), workspaceID).Return(database.Workspace{ + ID: workspaceID, + }, nil) + // And: Return an agent that is disconnected (no // FirstConnectedAt). mDB.EXPECT().GetWorkspaceAgentsInLatestBuildByWorkspaceID(gomock.Any(), workspaceID). @@ -241,7 +359,7 @@ func TestWatchChatGit(t *testing.T) { mCoordinator.EXPECT().Node(gomock.Any()).Return(nil) // And: We mount the HTTP handler. - r.With(httpmw.ExtractChatParam(mDB)). + r.With(injectSystemActor, httpmw.ExtractChatParam(mDB)). Get("/chats/{chat}/stream/git", api.watchChatGit) // Given: We create the HTTP server. @@ -300,6 +418,10 @@ func TestWatchChatGit(t *testing.T) { DeploymentValues: &codersdk.DeploymentValues{}, TailnetCoordinator: tailnettest.NewFakeCoordinator(), }, + HTTPAuth: &HTTPAuthorizer{ + Authorizer: &mockAuthorizer{}, + Logger: logger, + }, } ) @@ -341,6 +463,12 @@ func TestWatchChatGit(t *testing.T) { WorkspaceID: uuid.NullUUID{UUID: workspaceID, Valid: true}, }, nil) + // And: Return the workspace so the handler's + // workspace-level authz check can run. + mDB.EXPECT().GetWorkspaceByID(gomock.Any(), workspaceID).Return(database.Workspace{ + ID: workspaceID, + }, nil) + // And: Return a connected agent. mDB.EXPECT().GetWorkspaceAgentsInLatestBuildByWorkspaceID(gomock.Any(), workspaceID). Return([]database.WorkspaceAgent{{ @@ -375,7 +503,7 @@ func TestWatchChatGit(t *testing.T) { return s, nil }) // And: We mount the HTTP handler. - r.With(httpmw.ExtractChatParam(mDB)). + r.With(injectSystemActor, httpmw.ExtractChatParam(mDB)). Get("/chats/{chat}/stream/git", api.watchChatGit) // Given: We create the HTTP server. @@ -468,6 +596,10 @@ func TestWatchChatGit(t *testing.T) { DeploymentValues: &codersdk.DeploymentValues{}, TailnetCoordinator: tailnettest.NewFakeCoordinator(), }, + HTTPAuth: &HTTPAuthorizer{ + Authorizer: &mockAuthorizer{}, + Logger: logger, + }, } ) @@ -506,6 +638,12 @@ func TestWatchChatGit(t *testing.T) { WorkspaceID: uuid.NullUUID{UUID: workspaceID, Valid: true}, }, nil) + // And: Return the workspace so the handler's + // workspace-level authz check can run. + mDB.EXPECT().GetWorkspaceByID(gomock.Any(), workspaceID).Return(database.Workspace{ + ID: workspaceID, + }, nil) + // And: Return a connected agent. mDB.EXPECT().GetWorkspaceAgentsInLatestBuildByWorkspaceID(gomock.Any(), workspaceID). Return([]database.WorkspaceAgent{{ @@ -537,7 +675,7 @@ func TestWatchChatGit(t *testing.T) { return s, nil }) // And: We mount the HTTP handler. - r.With(httpmw.ExtractChatParam(mDB)). + r.With(injectSystemActor, httpmw.ExtractChatParam(mDB)). Get("/chats/{chat}/stream/git", api.watchChatGit) // Given: We create the HTTP server.