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.
2026-06-02 20:48:20 +00:00 · 2026-04-20 04:33:35 -07:00
parent 615be176b8
commit ea00d2d396
3 changed files with 268 additions and 24 deletions
@@ -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
 	}

@@ -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()

@@ -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.