From 16ebbdb01293d8ae4e57d79c10ca401b96bfa3c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Tue, 25 Aug 2026 11:36:16 +0100 Subject: [PATCH] fix(auth): guard quick-chat listing on workspace ownership (#3026) Co-authored-by: Kandev Agent Co-authored-by: Claude Opus 5 (1M context) --- .../boot_state_quick_chat_authz_test.go | 87 +++++++++++ .../handlers/quick_chat_list_authz_test.go | 123 +++++++++++++++ .../task/service/quick_chat_sessions.go | 9 ++ .../quick_chat_sessions_access_test.go | 145 ++++++++++++++++++ 4 files changed, 364 insertions(+) create mode 100644 apps/backend/internal/backendapp/boot_state_quick_chat_authz_test.go create mode 100644 apps/backend/internal/task/handlers/quick_chat_list_authz_test.go create mode 100644 apps/backend/internal/task/service/quick_chat_sessions_access_test.go diff --git a/apps/backend/internal/backendapp/boot_state_quick_chat_authz_test.go b/apps/backend/internal/backendapp/boot_state_quick_chat_authz_test.go new file mode 100644 index 0000000000..3627823783 --- /dev/null +++ b/apps/backend/internal/backendapp/boot_state_quick_chat_authz_test.go @@ -0,0 +1,87 @@ +package backendapp + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/kandev/kandev/internal/auth/authn" + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/webapp" +) + +// seedOwnedQuickChat gives userID a workspace holding one restorable quick chat +// whose title is the thing that must not cross an ownership boundary. +func seedOwnedQuickChat(t *testing.T, harness bootStateTestHarness, workspaceID, userID, title string) { + t.Helper() + ctx := context.Background() + if err := harness.taskRepo.CreateWorkspace(ctx, &models.Workspace{ + ID: workspaceID, Name: workspaceID, OwnerID: userID, + }); err != nil { + t.Fatalf("CreateWorkspace(%s): %v", workspaceID, err) + } + taskID := workspaceID + "-chat" + if err := harness.taskRepo.CreateTask(ctx, &models.Task{ + ID: taskID, WorkspaceID: workspaceID, Title: title, + State: "todo", Priority: "medium", IsEphemeral: true, + }); err != nil { + t.Fatalf("CreateTask(%s): %v", taskID, err) + } + if err := harness.taskRepo.CreateTaskSession(ctx, &models.TaskSession{ + ID: taskID + "-session", TaskID: taskID, + State: models.TaskSessionStateCompleted, IsPrimary: true, + }); err != nil { + t.Fatalf("CreateTaskSession(%s): %v", taskID, err) + } +} + +func bootQuickChatNames(t *testing.T, params routeParams, userID, workspaceID string) []string { + t.Helper() + ctx := authn.WithIdentity(t.Context(), authn.Identity{UserID: userID, Role: authn.RoleMember}) + req := httptest.NewRequest(http.MethodGet, "/?workspaceId="+workspaceID, nil).WithContext(ctx) + state := bootInitialState(ctx, req, params, webapp.RouteClassification{Route: webapp.RouteHome}) + + quickChat, ok := state["quickChat"].(map[string]any) + if !ok { + return nil + } + sessions, _ := quickChat["sessions"].([]map[string]any) + names := make([]string, 0, len(sessions)) + for _, session := range sessions { + name, _ := session["name"].(string) + names = append(names, name) + } + return names +} + +// TestBootStateQuickChatIsScopedToTheCaller is the regression guard for the +// second ListQuickChatSessions caller. Its main job is the owner case: the boot +// payload must still carry the caller's tabs now that the service can refuse. +// +// The foreign case documents rather than detects. resolveQuickChatWorkspaceID +// validates every candidate against the caller's own visible workspace list, so +// a foreign ID never reaches the service at all -- removing the service guard +// entirely leaves this assertion green. That is the property being pinned: the +// boot path fails closed one layer up, so a future refactor that starts +// trusting a request-supplied workspace ID here has to break this test first. +func TestBootStateQuickChatIsScopedToTheCaller(t *testing.T) { + harness := newBootStateTestHarness(t) + params := routeParams{taskSvc: harness.taskSvc, userCtrl: harness.userCtrl} + seedOwnedQuickChat(t, harness, "ws-boot-a", "user-a", "A's boot chat") + seedOwnedQuickChat(t, harness, "ws-boot-b", "user-b", "B's boot chat") + + owned := bootQuickChatNames(t, params, "user-a", "ws-boot-a") + if len(owned) != 1 || owned[0] != "A's boot chat" { + t.Fatalf("owner boot payload quick chats = %v; want [A's boot chat]", owned) + } + + // Asking for user-b's workspace must not put user-b's tab in user-a's boot + // payload -- neither directly nor by falling back to it. + foreign := bootQuickChatNames(t, params, "user-a", "ws-boot-b") + for _, name := range foreign { + if name == "B's boot chat" { + t.Fatalf("boot payload leaked a foreign quick chat: %v", foreign) + } + } +} diff --git a/apps/backend/internal/task/handlers/quick_chat_list_authz_test.go b/apps/backend/internal/task/handlers/quick_chat_list_authz_test.go new file mode 100644 index 0000000000..11d1482098 --- /dev/null +++ b/apps/backend/internal/task/handlers/quick_chat_list_authz_test.go @@ -0,0 +1,123 @@ +package handlers + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/kandev/kandev/internal/auth/authn" + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/repository/repoerrors" + "github.com/kandev/kandev/internal/task/service" +) + +// quickChatAuthzRepo owns two workspaces belonging to different users and +// serves user-a's quick chat regardless of the workspace asked for, so any tab +// reaching a foreign caller is a leak rather than an artifact of the fixture. +type quickChatAuthzRepo struct { + quickChatListRepo + workspaces map[string]*models.Workspace +} + +// GetWorkspace mirrors the sqlite repository's error shape exactly: a missing +// row wraps the sentinel with the requested ID (workspace.go:351). Returning the +// bare sentinel here would make the byte-identity assertion below pass for the +// wrong reason -- the point is that handleNotFound substitutes its own fallback +// message, so the ID the caller probed for never reaches the response. +func (r *quickChatAuthzRepo) GetWorkspace(_ context.Context, id string) (*models.Workspace, error) { + workspace, ok := r.workspaces[id] + if !ok { + return nil, fmt.Errorf("%w: %s", repoerrors.ErrWorkspaceNotFound, id) + } + return workspace, nil +} + +func newQuickChatAuthzHandler(t *testing.T) *TaskHandlers { + t.Helper() + gin.SetMode(gin.TestMode) + log := newTestLogger(t) + repo := &quickChatAuthzRepo{ + quickChatListRepo: quickChatListRepo{ + tasks: []*models.Task{{ + ID: "task-chat", WorkspaceID: "ws-a", Title: "A's secret chat", IsEphemeral: true, + Metadata: map[string]interface{}{models.MetaKeyAgentProfileID: "agent-1"}, + }}, + sessions: map[string]*models.TaskSession{ + "task-chat": {ID: "session-chat", TaskID: "task-chat"}, + }, + }, + workspaces: map[string]*models.Workspace{ + "ws-a": {ID: "ws-a", Name: "A's", OwnerID: "user-a"}, + "ws-b": {ID: "ws-b", Name: "B's", OwnerID: "user-b"}, + }, + } + svc := service.NewService(service.Repos{ + Workspaces: repo, Tasks: repo, TaskRepos: repo, + Workflows: repo, Messages: repo, Turns: repo, + Sessions: repo, GitSnapshots: repo, RepoEntities: repo, + Executors: repo, Environments: repo, TaskEnvironments: repo, + Reviews: repo, + }, nil, log, service.RepositoryDiscoveryConfig{}) + return &TaskHandlers{service: svc, logger: log} +} + +func listQuickChats(t *testing.T, h *TaskHandlers, identity *authn.Identity, workspaceID string) *httptest.ResponseRecorder { + t.Helper() + rec := httptest.NewRecorder() + c, _ := gin.CreateTestContext(rec) + req := httptest.NewRequest(http.MethodGet, "/workspaces/"+workspaceID+"/quick-chats", nil) + if identity != nil { + req = req.WithContext(authn.WithIdentity(req.Context(), *identity)) + } + c.Request = req + c.Params = gin.Params{{Key: "id", Value: workspaceID}} + h.httpListQuickChatSessions(c) + return rec +} + +// TestHTTPListQuickChatSessionsDeniesForeignWorkspace verifies end to end that +// the service denial reaches the wire as a plain 404 -- the whole +// no-existence-leak guarantee rests on the response, not on the sentinel. +func TestHTTPListQuickChatSessionsDeniesForeignWorkspace(t *testing.T) { + h := newQuickChatAuthzHandler(t) + userB := &authn.Identity{UserID: "user-b", Role: authn.RoleMember} + + foreign := listQuickChats(t, h, userB, "ws-a") + missing := listQuickChats(t, h, userB, "ws-nonexistent") + + assert.Equal(t, http.StatusNotFound, foreign.Code) + assert.NotContains(t, foreign.Body.String(), "A's secret chat") + assert.NotContains(t, foreign.Body.String(), "session-chat") + + // The probed ID must not survive into either body; that is what lets the + // two cases collapse into one response. + assert.Equal(t, http.StatusNotFound, missing.Code) + assert.NotContains(t, missing.Body.String(), "ws-nonexistent") + + // A workspace the caller may not see must be indistinguishable from one + // that does not exist, byte for byte. + assert.Equal(t, missing.Body.String(), foreign.Body.String()) +} + +// TestHTTPListQuickChatSessionsOwnerAndUnscopedCallers keeps the legitimate +// paths intact: the owner still gets their tabs, and with authentication +// disabled (synthetic identity, or no identity at all) nothing changes. +func TestHTTPListQuickChatSessionsOwnerAndUnscopedCallers(t *testing.T) { + h := newQuickChatAuthzHandler(t) + + for name, identity := range map[string]*authn.Identity{ + "owner": {UserID: "user-a", Role: authn.RoleMember}, + "auth disabled": {UserID: "default-user", Role: authn.RoleAdmin, Synthetic: true}, + "no identity": nil, + } { + rec := listQuickChats(t, h, identity, "ws-a") + require.Equal(t, http.StatusOK, rec.Code, "%s: body %s", name, rec.Body.String()) + assert.Contains(t, rec.Body.String(), "A's secret chat", "%s", name) + } +} diff --git a/apps/backend/internal/task/service/quick_chat_sessions.go b/apps/backend/internal/task/service/quick_chat_sessions.go index 28e937c1b6..50882f6a0e 100644 --- a/apps/backend/internal/task/service/quick_chat_sessions.go +++ b/apps/backend/internal/task/service/quick_chat_sessions.go @@ -40,7 +40,16 @@ type QuickChatSession struct { // most recently active first. It is the single source of truth for the quick // chat tab strip: the boot payload and the runtime resync endpoint both read it, // so a client that reloads and a client that resyncs converge on the same list. +// +// Quick-chat names are user-authored text, so the workspace check belongs here +// rather than in the callers: the resync endpoint takes its workspace ID +// straight from the URL. Today listQuickChatTasks also happens to authorize on +// the way through ListTasksByWorkspace, but that is an implementation detail of +// how this list is assembled, not a guarantee this function should lean on. func (s *Service) ListQuickChatSessions(ctx context.Context, workspaceID string) ([]QuickChatSession, error) { + if err := s.authorizeWorkspaceID(ctx, workspaceID); err != nil { + return nil, err + } tasks, err := s.listQuickChatTasks(ctx, workspaceID) if err != nil { return nil, err diff --git a/apps/backend/internal/task/service/quick_chat_sessions_access_test.go b/apps/backend/internal/task/service/quick_chat_sessions_access_test.go new file mode 100644 index 0000000000..529f6e6738 --- /dev/null +++ b/apps/backend/internal/task/service/quick_chat_sessions_access_test.go @@ -0,0 +1,145 @@ +package service + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/jmoiron/sqlx" + + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/repository" + "github.com/kandev/kandev/internal/task/repository/repoerrors" +) + +// countingTaskRepo records whether the quick-chat listing ever reached the task +// store. A denial must be decided before the query runs, so the counter staying +// at zero is what proves the guard is an authorization check and not a filter +// applied to rows we already read. +type countingTaskRepo struct { + repository.TaskRepository + listCalls int +} + +func (r *countingTaskRepo) ListTasksByWorkspace( + ctx context.Context, + workspaceID, workflowID, repositoryID, query string, + page, pageSize int, + sort string, + includeArchived, includeEphemeral, onlyEphemeral, excludeConfig bool, +) ([]*models.Task, int, error) { + r.listCalls++ + return r.TaskRepository.ListTasksByWorkspace( + ctx, workspaceID, workflowID, repositoryID, query, + page, pageSize, sort, includeArchived, includeEphemeral, onlyEphemeral, excludeConfig, + ) +} + +// seedScopedQuickChats gives user-a and user-b one owned workspace each, with a +// single restorable quick chat apiece. +func seedScopedQuickChats(t *testing.T, svc *Service, sqlxDB *sqlx.DB) { + t.Helper() + ctx := context.Background() + for _, ws := range []*models.Workspace{ + {ID: "ws-qc-a", Name: "A's", OwnerID: "user-a"}, + {ID: "ws-qc-b", Name: "B's", OwnerID: "user-b"}, + } { + if err := svc.workspaces.CreateWorkspace(ctx, ws); err != nil { + t.Fatalf("CreateWorkspace(%s): %v", ws.ID, err) + } + } + base := time.Date(2026, 8, 25, 12, 0, 0, 0, time.UTC) + seedQuickChatTask(t, svc, sqlxDB, quickChatFixture{ + id: "qc-task-a", workspaceID: "ws-qc-a", title: "A's secret chat", + agentProfileID: "agent-a", updatedAt: base, + }) + seedQuickChatTask(t, svc, sqlxDB, quickChatFixture{ + id: "qc-task-b", workspaceID: "ws-qc-b", title: "B's secret chat", + agentProfileID: "agent-b", updatedAt: base, + }) +} + +// TestListQuickChatSessionsDeniesForeignWorkspace is the core regression: quick +// chat names are user-authored text, so a foreign listing must fail closed and +// must fail before the task query runs. +func TestListQuickChatSessionsDeniesForeignWorkspace(t *testing.T) { + svc, sqlxDB := createOfficeIntegrationServiceWithDB(t) + seedScopedQuickChats(t, svc, sqlxDB) + + counter := &countingTaskRepo{TaskRepository: svc.tasks} + svc.tasks = counter + + items, err := svc.ListQuickChatSessions(ctxAs("user-b"), "ws-qc-a") + if !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Fatalf("foreign quick-chat listing: got (%v, %v); want ErrWorkspaceNotFound", items, err) + } + if len(items) != 0 { + t.Fatalf("denied listing leaked %d tabs", len(items)) + } + if counter.listCalls != 0 { + t.Fatalf("quick-chat query ran %d time(s) for a denied caller; want 0", counter.listCalls) + } +} + +// TestListQuickChatSessionsForeignMatchesNonexistent pins the no-existence-leak +// guarantee at the service boundary: both cases resolve to the same sentinel, +// which is what makes them collapse into one response. The sentinels carry +// different detail text (the repository names the missing ID), so the byte-level +// identity of the two responses is pinned at the HTTP boundary instead, in +// TestHTTPListQuickChatSessionsDeniesForeignWorkspace. +func TestListQuickChatSessionsForeignMatchesNonexistent(t *testing.T) { + svc, sqlxDB := createOfficeIntegrationServiceWithDB(t) + seedScopedQuickChats(t, svc, sqlxDB) + + _, foreignErr := svc.ListQuickChatSessions(ctxAs("user-b"), "ws-qc-a") + _, missingErr := svc.ListQuickChatSessions(ctxAs("user-b"), "ws-does-not-exist") + + if !errors.Is(foreignErr, repoerrors.ErrWorkspaceNotFound) { + t.Fatalf("foreign workspace: got %v; want ErrWorkspaceNotFound", foreignErr) + } + if !errors.Is(missingErr, repoerrors.ErrWorkspaceNotFound) { + t.Fatalf("nonexistent workspace: got %v; want ErrWorkspaceNotFound", missingErr) + } +} + +// TestListQuickChatSessionsOwnerStillSeesTabs guards against the fix turning +// into a denial for the legitimate owner. +func TestListQuickChatSessionsOwnerStillSeesTabs(t *testing.T) { + svc, sqlxDB := createOfficeIntegrationServiceWithDB(t) + seedScopedQuickChats(t, svc, sqlxDB) + + items, err := svc.ListQuickChatSessions(ctxAs("user-a"), "ws-qc-a") + if err != nil { + t.Fatalf("owner listing: %v", err) + } + if len(items) != 1 || items[0].Name != "A's secret chat" { + t.Fatalf("owner listing = %+v; want one tab named %q", items, "A's secret chat") + } +} + +// TestListQuickChatSessionsUnscopedCallersUnchanged pins single-user behavior: +// the synthetic identity (auth disabled) and identity-less internal callers see +// every workspace's tabs exactly as before. +func TestListQuickChatSessionsUnscopedCallersUnchanged(t *testing.T) { + svc, sqlxDB := createOfficeIntegrationServiceWithDB(t) + seedScopedQuickChats(t, svc, sqlxDB) + + for name, ctx := range map[string]context.Context{ + "auth disabled": ctxSynthetic(), + "internal": context.Background(), + } { + for _, ws := range []struct{ id, want string }{ + {"ws-qc-a", "A's secret chat"}, + {"ws-qc-b", "B's secret chat"}, + } { + items, err := svc.ListQuickChatSessions(ctx, ws.id) + if err != nil { + t.Fatalf("%s caller listing %s: %v", name, ws.id, err) + } + if len(items) != 1 || items[0].Name != ws.want { + t.Fatalf("%s caller listing %s = %+v; want one tab named %q", name, ws.id, items, ws.want) + } + } + } +}