From 116f7557dde6fb214964dea9f9ec4a1a76a55fe2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Sun, 26 Jul 2026 16:29:58 +0100 Subject: [PATCH] fix(auth): scope integration config/watch routes to workspace owner (#1960) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(auth): scope integration config/watch routes to workspace owner The third-party integration route group is guarded by its own query-only middleware (integrationWorkspaceScopeMiddleware), which authorizes only the workspace_id query param and falls through when it is absent. Two shapes bypassed it entirely under enforced auth: - POST /{jira,linear,slack}/config/copy takes the copy TARGET from the request body (targetWorkspaceId), which no layer checked. A caller could copy their own connection settings + credential into another user's workspace by naming its ID, after which that workspace's identity-less poller would run against attacker-controlled config (Jira/Linear issue watchers auto-creating tasks from attacker issues; Slack bot token/webhook). CopyConfigToWorkspace now authorizes BOTH source and target up front, before the pre-write UpdateAuthHealth side effect. Mirrors github/copy.go. - GET /{jira,linear}/watches/issue with workspace_id omitted returned ListAllIssueWatches across every workspace (JQL/filters, repo, agent and executor profile IDs, spawn prompt). It now filters to the caller's workspaces; an identity-less internal caller still sees all, preserving poller use. - The config Get/Set/Delete entry points resolved an unowned global "default" workspace when workspace_id was omitted; they now resolve+authorize via resolveWorkspaceID. Each service gains SetWorkspaceAuthorizer(taskSvc.AuthorizeWorkspaceAccess), wired in backendapp; nil (unit tests, auth disabled) stays unscoped. Denials surface repoerrors.ErrWorkspaceNotFound, mapped to 404 by the handlers (no existence leak). Recorded the query-only-middleware rule in integrations/AGENTS.md, including that Sentry and GitLab's ListAllIssueWatches still need the same filter. Regression tests added per package and mutation-verified: dropping only the copy up-front guard fails on the victim health-flip witness; dropping both guards fails on the config/secret witness. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(auth): scope integration watch-write and data-plane paths too Review of the prior commit (Codex + Claude, converging) found the same body/query workspace-bypass class in surfaces the first pass missed: - CreateIssueWatch / UpdateIssueWatch (Jira, Linear) never authorized the workspace. CreateIssueWatch takes WorkspaceID from the request body, so a caller could plant a persistent watch in another user's workspace that repeatedly spawns tasks with that user's credentials and an attacker-supplied filter/prompt. Now authorize req.WorkspaceID (create) and the loaded row's workspace (update). The by-ID mutation/trigger/reset handlers were already guarded by assertWatchInWorkspace + the query-authorizing middleware; the service-layer guard adds defense in depth and covers non-HTTP callers. - TestConnectionForWorkspace (Jira, Linear, Slack) resolved the default workspace with no auth, so an omitted workspace_id + attacker-supplied siteUrl could exfiltrate that workspace's stored token. Now authorizes after normalize and returns a hard error (404), not a swallowed {OK:false}. - Data-plane reads funnel through clientFor, which now resolves+authorizes the workspace — scoping search/projects/statuses in one chokepoint. - filterIssueWatchesByAccess treated ANY authorizer error as a denial, silently dropping watches on a transient failure and returning a truncated 200. It now drops only on ErrWorkspaceNotFound and propagates everything else. Denials map to 404 via the shared writeClientError / writeIssueWatchError sinks and the two list/test handlers. Renamed the test helper denyExcept -> denyOnly (it denies exactly the named workspaces). Added per-package regression tests for create/update/list denial, authorizer error propagation, connection-test denial, and data-plane denial; mutation-verified the CreateIssueWatch guard. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Kandev Agent Co-authored-by: Claude Opus 4.8 (1M context) --- apps/backend/internal/backendapp/helpers.go | 5 + apps/backend/internal/integrations/AGENTS.md | 26 +++ apps/backend/internal/jira/copy.go | 9 +- apps/backend/internal/jira/handlers.go | 38 ++++- apps/backend/internal/jira/scope_test.go | 149 ++++++++++++++++++ .../backend/internal/jira/scope_watch_test.go | 115 ++++++++++++++ apps/backend/internal/jira/service.go | 114 +++++++++++++- apps/backend/internal/linear/copy.go | 9 +- apps/backend/internal/linear/handlers.go | 34 +++- .../internal/linear/handlers_issue_watch.go | 4 + apps/backend/internal/linear/scope_test.go | 141 +++++++++++++++++ .../internal/linear/scope_watch_test.go | 108 +++++++++++++ apps/backend/internal/linear/service.go | 56 ++++++- .../internal/linear/service_issue_watch.go | 58 ++++++- apps/backend/internal/slack/copy.go | 9 +- apps/backend/internal/slack/handlers.go | 30 +++- .../backend/internal/slack/scope_conn_test.go | 37 +++++ apps/backend/internal/slack/scope_test.go | 110 +++++++++++++ apps/backend/internal/slack/service.go | 55 ++++++- 19 files changed, 1082 insertions(+), 25 deletions(-) create mode 100644 apps/backend/internal/jira/scope_test.go create mode 100644 apps/backend/internal/jira/scope_watch_test.go create mode 100644 apps/backend/internal/linear/scope_test.go create mode 100644 apps/backend/internal/linear/scope_watch_test.go create mode 100644 apps/backend/internal/slack/scope_conn_test.go create mode 100644 apps/backend/internal/slack/scope_test.go diff --git a/apps/backend/internal/backendapp/helpers.go b/apps/backend/internal/backendapp/helpers.go index ae9fd44296..59beee5966 100644 --- a/apps/backend/internal/backendapp/helpers.go +++ b/apps/backend/internal/backendapp/helpers.go @@ -569,10 +569,15 @@ func registerRoutes(p routeParams) { if p.services.Jira != nil { p.services.Jira.SetTaskDeleter(handoffSvc) p.services.Jira.SetRepositoryLookup(repoLookup) + p.services.Jira.SetWorkspaceAuthorizer(p.taskSvc.AuthorizeWorkspaceAccess) } if p.services.Linear != nil { p.services.Linear.SetTaskDeleter(handoffSvc) p.services.Linear.SetRepositoryLookup(repoLookup) + p.services.Linear.SetWorkspaceAuthorizer(p.taskSvc.AuthorizeWorkspaceAccess) + } + if p.services.Slack != nil { + p.services.Slack.SetWorkspaceAuthorizer(p.taskSvc.AuthorizeWorkspaceAccess) } if p.services.Sentry != nil { p.services.Sentry.SetTaskDeleter(handoffSvc) diff --git a/apps/backend/internal/integrations/AGENTS.md b/apps/backend/internal/integrations/AGENTS.md index caf002fcfc..6f247327ca 100644 --- a/apps/backend/internal/integrations/AGENTS.md +++ b/apps/backend/internal/integrations/AGENTS.md @@ -21,6 +21,32 @@ This file covers both halves of the playbook (backend + frontend) so the pattern - "Auth required / reconnect" UI reuses `` (`components/integrations/auth-error-message.tsx`) — supply the integration's display name, regex check, and reconnect href. - Link / import popovers reuse `` (`components/integrations/validated-popover.tsx`) — supply the icon, label, key regex, fetch function, and success callback. +## Per-user scoping (the integration route group is query-only) + +The global `integrationWorkspaceScopeMiddleware` (`backendapp/helpers.go`) only +authorizes the workspace named in the **`workspace_id` query param**, and it +**falls through when that param is absent**. So two shapes bypass it and MUST +guard themselves at the service layer: + +- **A body-supplied workspace** — e.g. `POST /config/copy`'s `targetWorkspaceId`. + The middleware authorizes the source (query) but never the target (body), so a + copy must call `authorizeWorkspaceAccess` on **both** source and target, up + front — before any per-workspace side effect (the pre-write `UpdateAuthHealth` + flip counts). Mirror `github/copy.go`. +- **An omitted `workspace_id`** — `normalizeWorkspaceID("")` resolves a global + *default* workspace not owned by the caller. User-facing config entry points + resolve+authorize in one step via `resolveWorkspaceID(ctx, id)` instead of bare + `normalizeWorkspaceID`. The unscoped `ListAllIssueWatches` (reached by omitting + `workspace_id`) filters to the caller's workspaces; an identity-less internal + caller (nil authorizer) still sees all, preserving poller use. + +Wire the boundary with `SetWorkspaceAuthorizer(taskSvc.AuthorizeWorkspaceAccess)` +in `backendapp/helpers.go`; nil (unit tests, auth disabled) means unscoped. +Denials surface `repoerrors.ErrWorkspaceNotFound`, which handlers map to 404 (no +existence leak). Jira/Linear/Slack follow this; **Sentry and GitLab's +`ListAllIssueWatches` still need the same filter** — the WS gateway backstop does +not read `workspace_id`, so GitLab's `workspace_id`-keyed WS list is unscoped too. + ## Where Jira and Linear deliberately diverge - **Issue model:** Jira uses transitions + JQL; Linear uses state IDs + structured filters. Don't merge these schemas — the upstream APIs are genuinely different. diff --git a/apps/backend/internal/jira/copy.go b/apps/backend/internal/jira/copy.go index 468e05c73b..2189c7cf97 100644 --- a/apps/backend/internal/jira/copy.go +++ b/apps/backend/internal/jira/copy.go @@ -18,11 +18,16 @@ var ErrNothingToCopy = errors.New("jira: source workspace has no configuration t // from sourceWorkspaceID to targetWorkspaceID. Watchers are intentionally out // of scope — only the connection settings and secret are duplicated. func (s *Service) CopyConfigToWorkspace(ctx context.Context, sourceWorkspaceID, targetWorkspaceID string) (*JiraConfig, error) { - sourceWorkspaceID, err := s.normalizeWorkspaceID(sourceWorkspaceID) + // Authorize BOTH workspaces up front. The integration middleware only + // authorizes the source (workspace_id query param); the target arrives in the + // request body and is otherwise unchecked, so a caller could copy their config + // and credential into another user's workspace by naming its ID here. Guard + // before the target health-row write below, not just before the config write. + sourceWorkspaceID, err := s.resolveWorkspaceID(ctx, sourceWorkspaceID) if err != nil { return nil, err } - targetWorkspaceID, err = s.normalizeWorkspaceID(targetWorkspaceID) + targetWorkspaceID, err = s.resolveWorkspaceID(ctx, targetWorkspaceID) if err != nil { return nil, err } diff --git a/apps/backend/internal/jira/handlers.go b/apps/backend/internal/jira/handlers.go index 637b1380bd..20ae50e31f 100644 --- a/apps/backend/internal/jira/handlers.go +++ b/apps/backend/internal/jira/handlers.go @@ -11,9 +11,17 @@ import ( "go.uber.org/zap" "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/task/repository/repoerrors" ws "github.com/kandev/kandev/pkg/websocket" ) +// workspaceDenied reports whether err is the per-user workspace access denial +// surfaced by the workspace authorizer. Handlers map it to 404 so a workspace +// the caller may not access is indistinguishable from a missing one. +func workspaceDenied(err error) bool { + return errors.Is(err, repoerrors.ErrWorkspaceNotFound) +} + // RegisterRoutes wires the Jira HTTP and WebSocket handlers. func RegisterRoutes(router *gin.Engine, dispatcher *ws.Dispatcher, svc *Service, log *logger.Logger) { ctrl := &Controller{service: svc, logger: log} @@ -55,6 +63,10 @@ func (c *Controller) RegisterHTTPRoutes(router *gin.Engine) { func (c *Controller) httpGetConfig(ctx *gin.Context) { cfg, err := c.service.GetConfigForWorkspace(ctx.Request.Context(), c.workspaceID(ctx)) if err != nil { + if workspaceDenied(err) { + ctx.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"}) + return + } ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return } @@ -74,8 +86,11 @@ func (c *Controller) httpSetConfig(ctx *gin.Context) { cfg, err := c.service.SetConfigForWorkspace(ctx.Request.Context(), c.workspaceID(ctx), &req) if err != nil { status := http.StatusInternalServerError - if errors.Is(err, ErrInvalidConfig) { + switch { + case errors.Is(err, ErrInvalidConfig): status = http.StatusBadRequest + case workspaceDenied(err): + status = http.StatusNotFound } ctx.JSON(status, gin.H{"error": err.Error()}) return @@ -85,6 +100,10 @@ func (c *Controller) httpSetConfig(ctx *gin.Context) { func (c *Controller) httpDeleteConfig(ctx *gin.Context) { if err := c.service.DeleteConfigForWorkspace(ctx.Request.Context(), c.workspaceID(ctx)); err != nil { + if workspaceDenied(err) { + ctx.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"}) + return + } ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return } @@ -99,6 +118,10 @@ func (c *Controller) httpTestConfig(ctx *gin.Context) { } result, err := c.service.TestConnectionForWorkspace(ctx.Request.Context(), c.workspaceID(ctx), &req) if err != nil { + if workspaceDenied(err) { + ctx.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"}) + return + } ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return } @@ -214,6 +237,8 @@ func (c *Controller) httpCopyConfig(ctx *gin.Context) { switch { case errors.Is(err, ErrSameWorkspace), errors.Is(err, ErrNothingToCopy), errors.Is(err, ErrInvalidConfig): status = http.StatusBadRequest + case workspaceDenied(err): + status = http.StatusNotFound } ctx.JSON(status, gin.H{"error": err.Error()}) return @@ -239,6 +264,10 @@ func (c *Controller) httpListIssueWatches(ctx *gin.Context) { watches, err = c.service.ListIssueWatches(ctx.Request.Context(), workspaceID) } if err != nil { + if workspaceDenied(err) { + ctx.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"}) + return + } ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return } @@ -398,6 +427,13 @@ const errCodeJiraNotConfigured = "JIRA_NOT_CONFIGURED" // surfaces as 503 so the UI can prompt the user to configure Jira; upstream // API errors propagate their status codes. func (c *Controller) writeClientError(ctx *gin.Context, err error) { + if workspaceDenied(err) { + // A workspace the caller may not access is indistinguishable from a + // missing one — 404, not 403/500. Covers data-plane reads (via clientFor) + // and watch create/update (via writeIssueWatchError's fall-through). + ctx.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"}) + return + } if errors.Is(err, ErrNotConfigured) { ctx.JSON(http.StatusServiceUnavailable, gin.H{ "error": "Jira is not configured", diff --git a/apps/backend/internal/jira/scope_test.go b/apps/backend/internal/jira/scope_test.go new file mode 100644 index 0000000000..71285d8c64 --- /dev/null +++ b/apps/backend/internal/jira/scope_test.go @@ -0,0 +1,149 @@ +package jira + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/kandev/kandev/internal/task/repository/repoerrors" +) + +// denyOnly returns a workspace authorizer that allows every workspace except +// the named ones, which it denies with the same ErrWorkspaceNotFound the real +// task-service authorizer returns. +func denyOnly(denied ...string) func(context.Context, string) error { + blocked := make(map[string]bool, len(denied)) + for _, ws := range denied { + blocked[ws] = true + } + return func(_ context.Context, ws string) error { + if blocked[ws] { + return repoerrors.ErrWorkspaceNotFound + } + return nil + } +} + +// TestCopyConfigToWorkspace_DeniesForeignTarget is the regression for the HIGH +// finding: the copy target arrives in the request body, which the query-only +// integration middleware never authorizes, so without a service-layer check a +// caller could copy their config + credential into another user's workspace. +func TestCopyConfigToWorkspace_DeniesForeignTarget(t *testing.T) { + f := newSvcFixture(t) + ctx := context.Background() + const src, victim = "ws-src", "ws-victim" + + // Seed the attacker's own source config while unscoped, then turn on scoping. + if _, err := f.svc.SetConfigForWorkspace(ctx, src, &SetConfigRequest{ + SiteURL: "https://attacker.atlassian.net", + Email: "a@example.com", + AuthMethod: AuthMethodAPIToken, + InstanceType: InstanceTypeCloud, + Secret: "tok-attacker", + }); err != nil { + t.Fatalf("seed source: %v", err) + } + drainProbe(t, f) + + // The victim already has a healthy Jira connection of their own. A denied + // copy must leave it byte-for-byte untouched — including its health status, + // which the up-front guard protects from the pre-write UpdateAuthHealth flip. + if err := f.store.UpsertConfigForWorkspace(ctx, victim, &JiraConfig{ + SiteURL: "https://victim.atlassian.net", Email: "v@example.com", + AuthMethod: AuthMethodAPIToken, InstanceType: InstanceTypeCloud, + }); err != nil { + t.Fatalf("seed victim config: %v", err) + } + if err := f.secrets.Set(ctx, SecretKeyForWorkspace(victim), "Jira token", "tok-victim"); err != nil { + t.Fatalf("seed victim secret: %v", err) + } + if err := f.store.UpdateAuthHealthForWorkspace(ctx, victim, true, "", time.Now().UTC()); err != nil { + t.Fatalf("seed victim health: %v", err) + } + + f.svc.SetWorkspaceAuthorizer(denyOnly(victim)) + + _, err := f.svc.CopyConfigToWorkspace(ctx, src, victim) + if !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Fatalf("expected ErrWorkspaceNotFound, got %v", err) + } + got, cfgErr := f.store.GetConfigForWorkspace(ctx, victim) + if cfgErr != nil || got == nil { + t.Fatalf("victim config vanished: cfg=%+v err=%v", got, cfgErr) + } + if got.SiteURL != "https://victim.atlassian.net" { + t.Errorf("victim config overwritten by copy: %q", got.SiteURL) + } + if !got.LastOk { + t.Errorf("victim health flipped to unhealthy by unauthorized copy") + } + if v, _ := f.secrets.Reveal(ctx, SecretKeyForWorkspace(victim)); v != "tok-victim" { + t.Errorf("victim secret overwritten by copy: %q", v) + } +} + +// TestConfigForWorkspace_DeniesForeignWorkspace is the regression for the LOW +// finding: a scoped caller must not read, write, or delete a workspace's config +// it does not own (including via the resolved default when workspace_id is +// omitted — resolveWorkspaceID authorizes whatever normalize resolves). +func TestConfigForWorkspace_DeniesForeignWorkspace(t *testing.T) { + f := newSvcFixture(t) + ctx := context.Background() + const foreign = "ws-foreign" + f.svc.SetWorkspaceAuthorizer(denyOnly(foreign)) + + if _, err := f.svc.GetConfigForWorkspace(ctx, foreign); !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Errorf("Get: expected ErrWorkspaceNotFound, got %v", err) + } + if _, err := f.svc.SetConfigForWorkspace(ctx, foreign, &SetConfigRequest{ + SiteURL: "https://x.atlassian.net", Email: "x@example.com", + AuthMethod: AuthMethodAPIToken, InstanceType: InstanceTypeCloud, Secret: "t", + }); !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Errorf("Set: expected ErrWorkspaceNotFound, got %v", err) + } + if err := f.svc.DeleteConfigForWorkspace(ctx, foreign); !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Errorf("Delete: expected ErrWorkspaceNotFound, got %v", err) + } +} + +// TestListAllIssueWatches_FiltersToCallerWorkspaces is the regression for the +// MEDIUM finding: the unscoped list-all form (reached by omitting workspace_id) +// leaked every workspace's watch config. A scoped caller now sees only their +// own; an identity-less internal caller (nil authorizer) still sees all. +func TestListAllIssueWatches_FiltersToCallerWorkspaces(t *testing.T) { + f := newSvcFixture(t) + ctx := context.Background() + + seed := func(ws, jql string) { + if err := f.store.CreateIssueWatch(ctx, &IssueWatch{ + WorkspaceID: ws, WorkflowID: "wf", WorkflowStepID: "step", + JQL: jql, AgentProfileID: "ap", Enabled: true, + }); err != nil { + t.Fatalf("seed watch: %v", err) + } + } + seed("ws-mine", "project = MINE") + seed("ws-mine", "project = MINE2") + seed("ws-other", "project = OTHER") + + // Unscoped (internal caller): every watch is visible, as before auth. + if all, err := f.svc.ListAllIssueWatches(ctx); err != nil || len(all) != 3 { + t.Fatalf("unscoped: want 3 watches, got %d (err=%v)", len(all), err) + } + + // Scoped: only the caller's own workspace watches survive the filter. + f.svc.SetWorkspaceAuthorizer(denyOnly("ws-other")) + mine, err := f.svc.ListAllIssueWatches(ctx) + if err != nil { + t.Fatalf("scoped list: %v", err) + } + if len(mine) != 2 { + t.Fatalf("scoped: want 2 watches, got %d", len(mine)) + } + for _, w := range mine { + if w.WorkspaceID != "ws-mine" { + t.Errorf("leaked foreign watch from %q", w.WorkspaceID) + } + } +} diff --git a/apps/backend/internal/jira/scope_watch_test.go b/apps/backend/internal/jira/scope_watch_test.go new file mode 100644 index 0000000000..4c023a29fc --- /dev/null +++ b/apps/backend/internal/jira/scope_watch_test.go @@ -0,0 +1,115 @@ +package jira + +import ( + "context" + "errors" + "testing" + + "github.com/kandev/kandev/internal/task/repository/repoerrors" +) + +// TestCreateIssueWatch_DeniesForeignWorkspace is the regression for the review's +// P1: CreateIssueWatch takes WorkspaceID from the request body, which the +// query-only integration middleware never authorizes. +func TestCreateIssueWatch_DeniesForeignWorkspace(t *testing.T) { + f := newSvcFixture(t) + ctx := context.Background() + f.svc.SetWorkspaceAuthorizer(denyOnly("ws-victim")) + + _, err := f.svc.CreateIssueWatch(ctx, &CreateIssueWatchRequest{ + WorkspaceID: "ws-victim", WorkflowID: "wf", WorkflowStepID: "step", + JQL: "project = X", AgentProfileID: "ap", + }) + if !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Fatalf("expected ErrWorkspaceNotFound, got %v", err) + } + if all, _ := f.store.ListAllIssueWatches(ctx); len(all) != 0 { + t.Fatalf("watch was created in a foreign workspace: %d rows", len(all)) + } +} + +// TestUpdateIssueWatch_DeniesForeignWorkspace guards the by-ID mutation path: +// the watch ID alone reveals nothing about ownership. +func TestUpdateIssueWatch_DeniesForeignWorkspace(t *testing.T) { + f := newSvcFixture(t) + ctx := context.Background() + w := &IssueWatch{ + WorkspaceID: "ws-victim", WorkflowID: "wf", WorkflowStepID: "step", + JQL: "project = X", AgentProfileID: "ap", Enabled: true, + } + if err := f.store.CreateIssueWatch(ctx, w); err != nil { + t.Fatalf("seed watch: %v", err) + } + f.svc.SetWorkspaceAuthorizer(denyOnly("ws-victim")) + + if _, err := f.svc.UpdateIssueWatch(ctx, w.ID, &UpdateIssueWatchRequest{}); !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Fatalf("expected ErrWorkspaceNotFound, got %v", err) + } +} + +// TestListIssueWatches_DeniesForeignWorkspace covers the single-workspace list +// path (the scoped form the settings page uses with an explicit workspace_id). +func TestListIssueWatches_DeniesForeignWorkspace(t *testing.T) { + f := newSvcFixture(t) + ctx := context.Background() + f.svc.SetWorkspaceAuthorizer(denyOnly("ws-victim")) + + if _, err := f.svc.ListIssueWatches(ctx, "ws-victim"); !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Fatalf("expected ErrWorkspaceNotFound, got %v", err) + } +} + +// TestListAllIssueWatches_PropagatesAuthorizerError is the regression for the +// review's P2: a non-denial authorizer error (e.g. a transient DB failure) must +// surface, not be treated as a silent per-watch denial that returns a truncated +// 200. +func TestListAllIssueWatches_PropagatesAuthorizerError(t *testing.T) { + f := newSvcFixture(t) + ctx := context.Background() + if err := f.store.CreateIssueWatch(ctx, &IssueWatch{ + WorkspaceID: "ws-1", WorkflowID: "wf", WorkflowStepID: "step", + JQL: "project = X", AgentProfileID: "ap", Enabled: true, + }); err != nil { + t.Fatalf("seed watch: %v", err) + } + boom := errors.New("authorizer backend down") + f.svc.SetWorkspaceAuthorizer(func(context.Context, string) error { return boom }) + + if _, err := f.svc.ListAllIssueWatches(ctx); !errors.Is(err, boom) { + t.Fatalf("expected the authorizer error to propagate, got %v", err) + } +} + +// TestTestConnection_DeniesForeignWorkspace is the regression for the review's +// P1 credential-exfiltration path: a caller-supplied siteUrl must not cause the +// default/foreign workspace's stored token to be sent anywhere. +func TestTestConnection_DeniesForeignWorkspace(t *testing.T) { + f := newSvcFixture(t) + ctx := context.Background() + f.svc.SetWorkspaceAuthorizer(denyOnly("ws-foreign")) + + result, err := f.svc.TestConnectionForWorkspace(ctx, "ws-foreign", &SetConfigRequest{ + SiteURL: "https://attacker.example", Email: "a@example.com", + AuthMethod: AuthMethodAPIToken, InstanceType: InstanceTypeCloud, Secret: "x", + }) + if !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Fatalf("expected ErrWorkspaceNotFound, got %v (result=%+v)", err, result) + } + // The client factory (which would perform the outbound auth call) must never + // have been reached. + if f.factoryHit.Load() != 0 { + t.Fatalf("client was built for a denied workspace: %d hits", f.factoryHit.Load()) + } +} + +// TestDataPlane_DeniesForeignWorkspace covers the clientFor chokepoint that all +// data-plane reads (search, projects, statuses) funnel through. +func TestDataPlane_DeniesForeignWorkspace(t *testing.T) { + f := newSvcFixture(t) + ctx := context.Background() + f.svc.SetWorkspaceAuthorizer(denyOnly("ws-foreign")) + + if _, err := f.svc.SearchTicketsForWorkspace(ctx, "ws-foreign", "project = X", "", 10); !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Fatalf("expected ErrWorkspaceNotFound, got %v", err) + } +} diff --git a/apps/backend/internal/jira/service.go b/apps/backend/internal/jira/service.go index 045b253106..750beb2a41 100644 --- a/apps/backend/internal/jira/service.go +++ b/apps/backend/internal/jira/service.go @@ -14,6 +14,7 @@ import ( "github.com/kandev/kandev/internal/common/securityutil" "github.com/kandev/kandev/internal/events" "github.com/kandev/kandev/internal/events/bus" + "github.com/kandev/kandev/internal/task/repository/repoerrors" "github.com/kandev/kandev/internal/watchreset" ) @@ -59,6 +60,43 @@ type Service struct { // (KANDEV_MOCK_JIRA=true). Exposed via MockClient() so the e2e control routes // can drive the same instance the clientFn returns. mockClient *MockClient + // workspaceAuthorizer is the per-user workspace access boundary, wired + // post-construction via SetWorkspaceAuthorizer. Nil (unit tests, auth + // disabled) means unscoped — every workspace is visible, as before auth. + workspaceAuthorizer func(context.Context, string) error +} + +// SetWorkspaceAuthorizer installs the per-user workspace access boundary applied +// before user-facing Jira config reads/mutations. Wired to +// taskSvc.AuthorizeWorkspaceAccess so a caller cannot reach a workspace it does +// not own by naming its ID (e.g. the copy target, or the resolved default). +func (s *Service) SetWorkspaceAuthorizer(authorizer func(context.Context, string) error) { + if s != nil { + s.workspaceAuthorizer = authorizer + } +} + +func (s *Service) authorizeWorkspaceAccess(ctx context.Context, workspaceID string) error { + if s == nil || s.workspaceAuthorizer == nil { + return nil + } + return s.workspaceAuthorizer(ctx, workspaceID) +} + +// resolveWorkspaceID normalizes an optional workspace ID (empty → default) and +// authorizes the caller against the resolved workspace. Using it instead of +// normalizeWorkspaceID on user-facing entry points closes the default-workspace +// fallback: a scoped caller that omits workspace_id cannot land on a global +// default it does not own. +func (s *Service) resolveWorkspaceID(ctx context.Context, workspaceID string) (string, error) { + workspaceID, err := s.normalizeWorkspaceID(workspaceID) + if err != nil { + return "", err + } + if err := s.authorizeWorkspaceAccess(ctx, workspaceID); err != nil { + return "", err + } + return workspaceID, nil } // SetTaskDeleter wires the cascade-delete dependency used by ResetIssueWatch. @@ -131,7 +169,7 @@ func (s *Service) GetConfig(ctx context.Context) (*JiraConfig, error) { // flag so the UI can distinguish "configured but empty" from "needs // credentials". func (s *Service) GetConfigForWorkspace(ctx context.Context, workspaceID string) (*JiraConfig, error) { - workspaceID, err := s.normalizeWorkspaceID(workspaceID) + workspaceID, err := s.resolveWorkspaceID(ctx, workspaceID) if err != nil { return nil, err } @@ -176,7 +214,7 @@ func (s *Service) SetConfig(ctx context.Context, req *SetConfigRequest) (*JiraCo // SetConfigForWorkspace is upsert for one workspace. func (s *Service) SetConfigForWorkspace(ctx context.Context, workspaceID string, req *SetConfigRequest) (*JiraConfig, error) { - workspaceID, err := s.normalizeWorkspaceID(workspaceID) + workspaceID, err := s.resolveWorkspaceID(ctx, workspaceID) if err != nil { return nil, err } @@ -223,7 +261,7 @@ func (s *Service) DeleteConfig(ctx context.Context) error { // DeleteConfigForWorkspace removes both the config row and the stored secret. func (s *Service) DeleteConfigForWorkspace(ctx context.Context, workspaceID string) error { - workspaceID, err := s.normalizeWorkspaceID(workspaceID) + workspaceID, err := s.resolveWorkspaceID(ctx, workspaceID) if err != nil { return err } @@ -257,6 +295,14 @@ func (s *Service) TestConnectionForWorkspace(ctx context.Context, workspaceID st if err != nil { return &TestConnectionResult{OK: false, Error: err.Error()}, nil } + // Authorize before revealing/using the workspace's stored token: an omitted + // workspace_id resolves the global default, and a caller-supplied siteUrl in + // req would otherwise exfiltrate that workspace's token to an attacker host. + // Return a hard error (not a swallowed {OK:false} result) so the denial is a + // 404 rather than a probeable "test failed". + if err := s.authorizeWorkspaceAccess(ctx, workspaceID); err != nil { + return nil, err + } cfg, secret, err := s.resolveCredentials(ctx, workspaceID, req) if err != nil { return &TestConnectionResult{OK: false, Error: err.Error()}, nil @@ -464,8 +510,11 @@ func (s *Service) SearchTicketsForWorkspace(ctx context.Context, workspaceID, jq // clientFor returns the cached client, creating it if needed. The cache is // invalidated whenever the config changes so stale credentials never linger. +// It is the single chokepoint for every data-plane read (projects, statuses, +// tickets, transitions), so authorizing the resolved workspace here scopes all +// of them — an omitted workspace_id must not resolve another user's default. func (s *Service) clientFor(ctx context.Context, workspaceID string) (Client, error) { - workspaceID, err := s.normalizeWorkspaceID(workspaceID) + workspaceID, err := s.resolveWorkspaceID(ctx, workspaceID) if err != nil { return nil, err } @@ -740,6 +789,13 @@ func (s *Service) CreateIssueWatch(ctx context.Context, req *CreateIssueWatchReq if err := validateIssueWatchCreate(req); err != nil { return nil, err } + // WorkspaceID comes from the request body, which the query-only integration + // middleware never authorizes. Without this a caller could plant a watch in + // another user's workspace that repeatedly spawns tasks with that user's + // credentials and an attacker-supplied JQL/prompt. + if err := s.authorizeWorkspaceAccess(ctx, req.WorkspaceID); err != nil { + return nil, err + } repositoryID, baseBranch, err := s.resolveRepositoryBinding(ctx, req.WorkspaceID, req.RepositoryID, req.BaseBranch) if err != nil { return nil, err @@ -769,12 +825,53 @@ func (s *Service) CreateIssueWatch(ctx context.Context, req *CreateIssueWatchReq // ListIssueWatches returns the watches configured for a workspace. func (s *Service) ListIssueWatches(ctx context.Context, workspaceID string) ([]*IssueWatch, error) { + if err := s.authorizeWorkspaceAccess(ctx, workspaceID); err != nil { + return nil, err + } return s.store.ListIssueWatches(ctx, workspaceID) } -// ListAllIssueWatches returns every watch across all workspaces. +// ListAllIssueWatches returns every watch the caller may see. For a scoped +// caller that is only their own workspaces' watches; for an identity-less +// internal caller (unscoped) it is every watch, as before auth. The unscoped +// list form is only reachable when workspace_id is omitted, which the query-only +// integration middleware does not authorize — without this filter it leaked +// every workspace's watch config (JQL, repo/agent/profile IDs, spawn prompt). func (s *Service) ListAllIssueWatches(ctx context.Context) ([]*IssueWatch, error) { - return s.store.ListAllIssueWatches(ctx) + watches, err := s.store.ListAllIssueWatches(ctx) + if err != nil { + return nil, err + } + return s.filterIssueWatchesByAccess(ctx, watches) +} + +// filterIssueWatchesByAccess keeps only watches whose workspace the caller may +// access. Access decisions are memoized per workspace so a long list costs one +// authorize call per distinct workspace, not one per watch. Only an +// ErrWorkspaceNotFound denial drops a watch; any other authorizer error (a +// transient DB failure, say) is propagated so the caller sees the failure +// rather than a silently truncated 200. +func (s *Service) filterIssueWatchesByAccess(ctx context.Context, watches []*IssueWatch) ([]*IssueWatch, error) { + decision := make(map[string]bool) + visible := make([]*IssueWatch, 0, len(watches)) + for _, w := range watches { + allowed, seen := decision[w.WorkspaceID] + if !seen { + switch err := s.authorizeWorkspaceAccess(ctx, w.WorkspaceID); { + case err == nil: + allowed = true + case errors.Is(err, repoerrors.ErrWorkspaceNotFound): + allowed = false + default: + return nil, err + } + decision[w.WorkspaceID] = allowed + } + if allowed { + visible = append(visible, w) + } + } + return visible, nil } // GetIssueWatch returns a single watch by ID or ErrIssueWatchNotFound. @@ -796,6 +893,11 @@ func (s *Service) UpdateIssueWatch(ctx context.Context, id string, req *UpdateIs if err != nil { return nil, err } + // Authorize off the loaded row's workspace — the watch ID alone reveals + // nothing about ownership, so a caller must not mutate another user's watch. + if err := s.authorizeWorkspaceAccess(ctx, w.WorkspaceID); err != nil { + return nil, err + } prevRepositoryID, prevBaseBranch := w.RepositoryID, w.BaseBranch applyIssueWatchPatch(w, req) // Empty-string PATCH writes (`{"workflowId": ""}` etc.) bypass the nil diff --git a/apps/backend/internal/linear/copy.go b/apps/backend/internal/linear/copy.go index af5107a01f..e0c656cbd6 100644 --- a/apps/backend/internal/linear/copy.go +++ b/apps/backend/internal/linear/copy.go @@ -18,11 +18,16 @@ var ErrNothingToCopy = errors.New("linear: source workspace has no configuration // key) from sourceWorkspaceID to targetWorkspaceID. Watchers are intentionally // out of scope — only the connection settings and secret are duplicated. func (s *Service) CopyConfigToWorkspace(ctx context.Context, sourceWorkspaceID, targetWorkspaceID string) (*LinearConfig, error) { - sourceWorkspaceID, err := s.normalizeWorkspaceID(sourceWorkspaceID) + // Authorize BOTH workspaces up front. The integration middleware only + // authorizes the source (workspace_id query param); the target arrives in the + // request body and is otherwise unchecked, so a caller could copy their config + // and credential into another user's workspace by naming its ID here. Guard + // before the target health-row write below, not just before the config write. + sourceWorkspaceID, err := s.resolveWorkspaceID(ctx, sourceWorkspaceID) if err != nil { return nil, err } - targetWorkspaceID, err = s.normalizeWorkspaceID(targetWorkspaceID) + targetWorkspaceID, err = s.resolveWorkspaceID(ctx, targetWorkspaceID) if err != nil { return nil, err } diff --git a/apps/backend/internal/linear/handlers.go b/apps/backend/internal/linear/handlers.go index 06e96e73a3..95abd2b7ba 100644 --- a/apps/backend/internal/linear/handlers.go +++ b/apps/backend/internal/linear/handlers.go @@ -13,9 +13,17 @@ import ( "go.uber.org/zap" "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/task/repository/repoerrors" ws "github.com/kandev/kandev/pkg/websocket" ) +// workspaceDenied reports whether err is the per-user workspace access denial +// surfaced by the workspace authorizer. Handlers map it to 404 so a workspace +// the caller may not access is indistinguishable from a missing one. +func workspaceDenied(err error) bool { + return errors.Is(err, repoerrors.ErrWorkspaceNotFound) +} + // RegisterRoutes wires the Linear HTTP and WebSocket handlers. func RegisterRoutes(router *gin.Engine, dispatcher *ws.Dispatcher, svc *Service, log *logger.Logger) { ctrl := &Controller{service: svc, logger: log} @@ -59,6 +67,10 @@ func (c *Controller) RegisterHTTPRoutes(router *gin.Engine) { func (c *Controller) httpGetConfig(ctx *gin.Context) { cfg, err := c.service.GetConfigForWorkspace(ctx.Request.Context(), c.workspaceID(ctx)) if err != nil { + if workspaceDenied(err) { + ctx.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"}) + return + } ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return } @@ -78,8 +90,11 @@ func (c *Controller) httpSetConfig(ctx *gin.Context) { cfg, err := c.service.SetConfigForWorkspace(ctx.Request.Context(), c.workspaceID(ctx), &req) if err != nil { status := http.StatusInternalServerError - if errors.Is(err, ErrInvalidConfig) { + switch { + case errors.Is(err, ErrInvalidConfig): status = http.StatusBadRequest + case workspaceDenied(err): + status = http.StatusNotFound } ctx.JSON(status, gin.H{"error": err.Error()}) return @@ -89,6 +104,10 @@ func (c *Controller) httpSetConfig(ctx *gin.Context) { func (c *Controller) httpDeleteConfig(ctx *gin.Context) { if err := c.service.DeleteConfigForWorkspace(ctx.Request.Context(), c.workspaceID(ctx)); err != nil { + if workspaceDenied(err) { + ctx.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"}) + return + } ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return } @@ -103,6 +122,10 @@ func (c *Controller) httpTestConfig(ctx *gin.Context) { } result, err := c.service.TestConnectionForWorkspace(ctx.Request.Context(), c.workspaceID(ctx), &req) if err != nil { + if workspaceDenied(err) { + ctx.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"}) + return + } ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return } @@ -240,6 +263,8 @@ func (c *Controller) httpCopyConfig(ctx *gin.Context) { switch { case errors.Is(err, ErrSameWorkspace), errors.Is(err, ErrNothingToCopy), errors.Is(err, ErrInvalidConfig): status = http.StatusBadRequest + case workspaceDenied(err): + status = http.StatusNotFound } ctx.JSON(status, gin.H{"error": err.Error()}) return @@ -338,6 +363,13 @@ const ( // writeClientError maps service-level errors to HTTP responses. func (c *Controller) writeClientError(ctx *gin.Context, err error) { + if workspaceDenied(err) { + // A workspace the caller may not access is indistinguishable from a + // missing one — 404, not 403/500. Covers data-plane reads (via clientFor) + // and watch create/update (via writeIssueWatchError's fall-through). + ctx.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"}) + return + } if errors.Is(err, ErrNotConfigured) { ctx.JSON(http.StatusServiceUnavailable, gin.H{ "error": "Linear is not configured", diff --git a/apps/backend/internal/linear/handlers_issue_watch.go b/apps/backend/internal/linear/handlers_issue_watch.go index 523e6adc77..bde2c48fbe 100644 --- a/apps/backend/internal/linear/handlers_issue_watch.go +++ b/apps/backend/internal/linear/handlers_issue_watch.go @@ -22,6 +22,10 @@ func (c *Controller) httpListIssueWatches(ctx *gin.Context) { watches, err = c.service.ListIssueWatches(ctx.Request.Context(), workspaceID) } if err != nil { + if workspaceDenied(err) { + ctx.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"}) + return + } ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return } diff --git a/apps/backend/internal/linear/scope_test.go b/apps/backend/internal/linear/scope_test.go new file mode 100644 index 0000000000..a1985abf6b --- /dev/null +++ b/apps/backend/internal/linear/scope_test.go @@ -0,0 +1,141 @@ +package linear + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/kandev/kandev/internal/task/repository/repoerrors" +) + +// denyOnly returns a workspace authorizer that allows every workspace except +// the named ones, which it denies with the same ErrWorkspaceNotFound the real +// task-service authorizer returns. +func denyOnly(denied ...string) func(context.Context, string) error { + blocked := make(map[string]bool, len(denied)) + for _, ws := range denied { + blocked[ws] = true + } + return func(_ context.Context, ws string) error { + if blocked[ws] { + return repoerrors.ErrWorkspaceNotFound + } + return nil + } +} + +// TestCopyConfigToWorkspace_DeniesForeignTarget is the regression for the HIGH +// finding: the copy target arrives in the request body, which the query-only +// integration middleware never authorizes, so without a service-layer check a +// caller could copy their config + credential into another user's workspace. +func TestCopyConfigToWorkspace_DeniesForeignTarget(t *testing.T) { + f := newSvcFixture(t) + ctx := context.Background() + const src, victim = "ws-src", "ws-victim" + + if _, err := f.svc.SetConfigForWorkspace(ctx, src, &SetConfigRequest{ + AuthMethod: AuthMethodAPIKey, + DefaultTeamKey: "ENG", + Secret: "lin-attacker", + }); err != nil { + t.Fatalf("seed source: %v", err) + } + drainProbe(t, f) + + // The victim already has a healthy Linear connection of their own. A denied + // copy must leave it byte-for-byte untouched — including its health status, + // which the up-front guard protects from the pre-write UpdateAuthHealth flip. + if err := f.store.UpsertConfigForWorkspace(ctx, victim, &LinearConfig{ + AuthMethod: AuthMethodAPIKey, DefaultTeamKey: "VIC", + }); err != nil { + t.Fatalf("seed victim config: %v", err) + } + if err := f.secrets.Set(ctx, SecretKeyForWorkspace(victim), "Linear API key", "lin-victim"); err != nil { + t.Fatalf("seed victim secret: %v", err) + } + if err := f.store.UpdateAuthHealthForWorkspace(ctx, victim, true, "", "", time.Now().UTC()); err != nil { + t.Fatalf("seed victim health: %v", err) + } + + f.svc.SetWorkspaceAuthorizer(denyOnly(victim)) + + _, err := f.svc.CopyConfigToWorkspace(ctx, src, victim) + if !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Fatalf("expected ErrWorkspaceNotFound, got %v", err) + } + got, cfgErr := f.store.GetConfigForWorkspace(ctx, victim) + if cfgErr != nil || got == nil { + t.Fatalf("victim config vanished: cfg=%+v err=%v", got, cfgErr) + } + if got.DefaultTeamKey != "VIC" { + t.Errorf("victim config overwritten by copy: %q", got.DefaultTeamKey) + } + if !got.LastOk { + t.Errorf("victim health flipped to unhealthy by unauthorized copy") + } + if v, _ := f.secrets.Reveal(ctx, SecretKeyForWorkspace(victim)); v != "lin-victim" { + t.Errorf("victim secret overwritten by copy: %q", v) + } +} + +// TestConfigForWorkspace_DeniesForeignWorkspace is the regression for the LOW +// finding: a scoped caller must not read, write, or delete a workspace's config +// it does not own. +func TestConfigForWorkspace_DeniesForeignWorkspace(t *testing.T) { + f := newSvcFixture(t) + ctx := context.Background() + const foreign = "ws-foreign" + f.svc.SetWorkspaceAuthorizer(denyOnly(foreign)) + + if _, err := f.svc.GetConfigForWorkspace(ctx, foreign); !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Errorf("Get: expected ErrWorkspaceNotFound, got %v", err) + } + if _, err := f.svc.SetConfigForWorkspace(ctx, foreign, &SetConfigRequest{ + AuthMethod: AuthMethodAPIKey, DefaultTeamKey: "ENG", Secret: "t", + }); !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Errorf("Set: expected ErrWorkspaceNotFound, got %v", err) + } + if err := f.svc.DeleteConfigForWorkspace(ctx, foreign); !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Errorf("Delete: expected ErrWorkspaceNotFound, got %v", err) + } +} + +// TestListAllIssueWatches_FiltersToCallerWorkspaces is the regression for the +// MEDIUM finding: the unscoped list-all form (reached by omitting workspace_id) +// leaked every workspace's watch config. A scoped caller now sees only their +// own; an identity-less internal caller (nil authorizer) still sees all. +func TestListAllIssueWatches_FiltersToCallerWorkspaces(t *testing.T) { + f := newSvcFixture(t) + ctx := context.Background() + + seed := func(ws string) { + if err := f.store.CreateIssueWatch(ctx, &IssueWatch{ + WorkspaceID: ws, WorkflowID: "wf", WorkflowStepID: "step", + AgentProfileID: "ap", Enabled: true, + }); err != nil { + t.Fatalf("seed watch: %v", err) + } + } + seed("ws-mine") + seed("ws-mine") + seed("ws-other") + + if all, err := f.svc.ListAllIssueWatches(ctx); err != nil || len(all) != 3 { + t.Fatalf("unscoped: want 3 watches, got %d (err=%v)", len(all), err) + } + + f.svc.SetWorkspaceAuthorizer(denyOnly("ws-other")) + mine, err := f.svc.ListAllIssueWatches(ctx) + if err != nil { + t.Fatalf("scoped list: %v", err) + } + if len(mine) != 2 { + t.Fatalf("scoped: want 2 watches, got %d", len(mine)) + } + for _, w := range mine { + if w.WorkspaceID != "ws-mine" { + t.Errorf("leaked foreign watch from %q", w.WorkspaceID) + } + } +} diff --git a/apps/backend/internal/linear/scope_watch_test.go b/apps/backend/internal/linear/scope_watch_test.go new file mode 100644 index 0000000000..860e8e5618 --- /dev/null +++ b/apps/backend/internal/linear/scope_watch_test.go @@ -0,0 +1,108 @@ +package linear + +import ( + "context" + "errors" + "testing" + + "github.com/kandev/kandev/internal/task/repository/repoerrors" +) + +// TestCreateIssueWatch_DeniesForeignWorkspace is the regression for the review's +// P1: CreateIssueWatch takes WorkspaceID from the request body, which the +// query-only integration middleware never authorizes. +func TestCreateIssueWatch_DeniesForeignWorkspace(t *testing.T) { + f := newSvcFixture(t) + ctx := context.Background() + f.svc.SetWorkspaceAuthorizer(denyOnly("ws-victim")) + + _, err := f.svc.CreateIssueWatch(ctx, &CreateIssueWatchRequest{ + WorkspaceID: "ws-victim", WorkflowID: "wf", WorkflowStepID: "step", + Filter: SearchFilter{Query: "bug"}, AgentProfileID: "ap", + }) + if !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Fatalf("expected ErrWorkspaceNotFound, got %v", err) + } + if all, _ := f.store.ListAllIssueWatches(ctx); len(all) != 0 { + t.Fatalf("watch was created in a foreign workspace: %d rows", len(all)) + } +} + +// TestUpdateIssueWatch_DeniesForeignWorkspace guards the by-ID mutation path: +// the watch ID alone reveals nothing about ownership. +func TestUpdateIssueWatch_DeniesForeignWorkspace(t *testing.T) { + f := newSvcFixture(t) + ctx := context.Background() + w := &IssueWatch{ + WorkspaceID: "ws-victim", WorkflowID: "wf", WorkflowStepID: "step", + Filter: SearchFilter{Query: "bug"}, AgentProfileID: "ap", Enabled: true, + } + if err := f.store.CreateIssueWatch(ctx, w); err != nil { + t.Fatalf("seed watch: %v", err) + } + f.svc.SetWorkspaceAuthorizer(denyOnly("ws-victim")) + + if _, err := f.svc.UpdateIssueWatch(ctx, w.ID, &UpdateIssueWatchRequest{}); !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Fatalf("expected ErrWorkspaceNotFound, got %v", err) + } +} + +// TestListIssueWatches_DeniesForeignWorkspace covers the single-workspace list +// path (the scoped form the settings page uses with an explicit workspace_id). +func TestListIssueWatches_DeniesForeignWorkspace(t *testing.T) { + f := newSvcFixture(t) + ctx := context.Background() + f.svc.SetWorkspaceAuthorizer(denyOnly("ws-victim")) + + if _, err := f.svc.ListIssueWatches(ctx, "ws-victim"); !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Fatalf("expected ErrWorkspaceNotFound, got %v", err) + } +} + +// TestListAllIssueWatches_PropagatesAuthorizerError is the regression for the +// review's P2: a non-denial authorizer error (e.g. a transient DB failure) must +// surface, not be treated as a silent per-watch denial that returns a truncated +// 200. +func TestListAllIssueWatches_PropagatesAuthorizerError(t *testing.T) { + f := newSvcFixture(t) + ctx := context.Background() + if err := f.store.CreateIssueWatch(ctx, &IssueWatch{ + WorkspaceID: "ws-1", WorkflowID: "wf", WorkflowStepID: "step", + Filter: SearchFilter{Query: "bug"}, AgentProfileID: "ap", Enabled: true, + }); err != nil { + t.Fatalf("seed watch: %v", err) + } + boom := errors.New("authorizer backend down") + f.svc.SetWorkspaceAuthorizer(func(context.Context, string) error { return boom }) + + if _, err := f.svc.ListAllIssueWatches(ctx); !errors.Is(err, boom) { + t.Fatalf("expected the authorizer error to propagate, got %v", err) + } +} + +// TestTestConnection_DeniesForeignWorkspace is the regression for the review's +// P1 credential-exfiltration path. +func TestTestConnection_DeniesForeignWorkspace(t *testing.T) { + f := newSvcFixture(t) + ctx := context.Background() + f.svc.SetWorkspaceAuthorizer(denyOnly("ws-foreign")) + + result, err := f.svc.TestConnectionForWorkspace(ctx, "ws-foreign", &SetConfigRequest{ + AuthMethod: AuthMethodAPIKey, DefaultTeamKey: "ENG", Secret: "x", + }) + if !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Fatalf("expected ErrWorkspaceNotFound, got %v (result=%+v)", err, result) + } +} + +// TestDataPlane_DeniesForeignWorkspace covers the clientFor chokepoint that all +// data-plane reads funnel through. +func TestDataPlane_DeniesForeignWorkspace(t *testing.T) { + f := newSvcFixture(t) + ctx := context.Background() + f.svc.SetWorkspaceAuthorizer(denyOnly("ws-foreign")) + + if _, err := f.svc.SearchIssuesForWorkspace(ctx, "ws-foreign", SearchFilter{Query: "bug"}, "", 10); !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Fatalf("expected ErrWorkspaceNotFound, got %v", err) + } +} diff --git a/apps/backend/internal/linear/service.go b/apps/backend/internal/linear/service.go index 7ead0e93ba..4c92b2b87d 100644 --- a/apps/backend/internal/linear/service.go +++ b/apps/backend/internal/linear/service.go @@ -54,6 +54,43 @@ type Service struct { // (KANDEV_MOCK_LINEAR=true). Exposed via MockClient() so the e2e control // routes can drive the same instance the clientFn returns. mockClient *MockClient + // workspaceAuthorizer is the per-user workspace access boundary, wired + // post-construction via SetWorkspaceAuthorizer. Nil (unit tests, auth + // disabled) means unscoped — every workspace is visible, as before auth. + workspaceAuthorizer func(context.Context, string) error +} + +// SetWorkspaceAuthorizer installs the per-user workspace access boundary applied +// before user-facing Linear config reads/mutations. Wired to +// taskSvc.AuthorizeWorkspaceAccess so a caller cannot reach a workspace it does +// not own by naming its ID (e.g. the copy target, or the resolved default). +func (s *Service) SetWorkspaceAuthorizer(authorizer func(context.Context, string) error) { + if s != nil { + s.workspaceAuthorizer = authorizer + } +} + +func (s *Service) authorizeWorkspaceAccess(ctx context.Context, workspaceID string) error { + if s == nil || s.workspaceAuthorizer == nil { + return nil + } + return s.workspaceAuthorizer(ctx, workspaceID) +} + +// resolveWorkspaceID normalizes an optional workspace ID (empty → default) and +// authorizes the caller against the resolved workspace. Using it instead of +// normalizeWorkspaceID on user-facing entry points closes the default-workspace +// fallback: a scoped caller that omits workspace_id cannot land on a global +// default it does not own. +func (s *Service) resolveWorkspaceID(ctx context.Context, workspaceID string) (string, error) { + workspaceID, err := s.normalizeWorkspaceID(workspaceID) + if err != nil { + return "", err + } + if err := s.authorizeWorkspaceAccess(ctx, workspaceID); err != nil { + return "", err + } + return workspaceID, nil } // SetTaskDeleter wires the cascade-delete dependency used by ResetIssueWatch. @@ -120,7 +157,7 @@ func (s *Service) GetConfig(ctx context.Context) (*LinearConfig, error) { // GetConfigForWorkspace returns a workspace config enriched with a HasSecret flag. func (s *Service) GetConfigForWorkspace(ctx context.Context, workspaceID string) (*LinearConfig, error) { - workspaceID, err := s.normalizeWorkspaceID(workspaceID) + workspaceID, err := s.resolveWorkspaceID(ctx, workspaceID) if err != nil { return nil, err } @@ -154,7 +191,7 @@ func (s *Service) SetConfig(ctx context.Context, req *SetConfigRequest) (*Linear // SetConfigForWorkspace is upsert for one workspace. An empty Secret on update // keeps the existing token. func (s *Service) SetConfigForWorkspace(ctx context.Context, workspaceID string, req *SetConfigRequest) (*LinearConfig, error) { - workspaceID, err := s.normalizeWorkspaceID(workspaceID) + workspaceID, err := s.resolveWorkspaceID(ctx, workspaceID) if err != nil { return nil, err } @@ -192,7 +229,7 @@ func (s *Service) DeleteConfig(ctx context.Context) error { // DeleteConfigForWorkspace removes both the config row and the stored secret. func (s *Service) DeleteConfigForWorkspace(ctx context.Context, workspaceID string) error { - workspaceID, err := s.normalizeWorkspaceID(workspaceID) + workspaceID, err := s.resolveWorkspaceID(ctx, workspaceID) if err != nil { return err } @@ -224,6 +261,13 @@ func (s *Service) TestConnectionForWorkspace(ctx context.Context, workspaceID st if err != nil { return &TestConnectionResult{OK: false, Error: err.Error()}, nil } + // Authorize before revealing/using the workspace's stored key: an omitted + // workspace_id resolves the global default. Return a hard error (not a + // swallowed {OK:false} result) so the denial is a 404 rather than a + // probeable "test failed". + if err := s.authorizeWorkspaceAccess(ctx, workspaceID); err != nil { + return nil, err + } cfg, secret, err := s.resolveCredentials(ctx, workspaceID, req) if err != nil { return &TestConnectionResult{OK: false, Error: err.Error()}, nil @@ -435,7 +479,11 @@ func (s *Service) SearchIssuesForWorkspace(ctx context.Context, workspaceID stri // clientFor returns the cached client, creating it if needed. func (s *Service) clientFor(ctx context.Context, workspaceID string) (Client, error) { - workspaceID, err := s.normalizeWorkspaceID(workspaceID) + // clientFor is the single chokepoint for every data-plane read (teams, + // states, labels, issues), so authorizing the resolved workspace here scopes + // all of them — an omitted workspace_id must not resolve another user's + // default workspace. + workspaceID, err := s.resolveWorkspaceID(ctx, workspaceID) if err != nil { return nil, err } diff --git a/apps/backend/internal/linear/service_issue_watch.go b/apps/backend/internal/linear/service_issue_watch.go index 17b6d1fffc..9fa1c83ee4 100644 --- a/apps/backend/internal/linear/service_issue_watch.go +++ b/apps/backend/internal/linear/service_issue_watch.go @@ -13,6 +13,7 @@ import ( "github.com/kandev/kandev/internal/common/securityutil" "github.com/kandev/kandev/internal/events" "github.com/kandev/kandev/internal/events/bus" + "github.com/kandev/kandev/internal/task/repository/repoerrors" "github.com/kandev/kandev/internal/watchreset" ) @@ -33,6 +34,13 @@ func (s *Service) CreateIssueWatch(ctx context.Context, req *CreateIssueWatchReq if err := validateIssueWatchCreate(req); err != nil { return nil, err } + // WorkspaceID comes from the request body, which the query-only integration + // middleware never authorizes. Without this a caller could plant a watch in + // another user's workspace that repeatedly spawns tasks with that user's + // credentials and an attacker-supplied filter/prompt. + if err := s.authorizeWorkspaceAccess(ctx, req.WorkspaceID); err != nil { + return nil, err + } repositoryID, baseBranch, err := s.resolveRepositoryBinding(ctx, req.WorkspaceID, req.RepositoryID, req.BaseBranch) if err != nil { return nil, err @@ -63,12 +71,53 @@ func (s *Service) CreateIssueWatch(ctx context.Context, req *CreateIssueWatchReq // ListIssueWatches returns the watches configured for a workspace. func (s *Service) ListIssueWatches(ctx context.Context, workspaceID string) ([]*IssueWatch, error) { + if err := s.authorizeWorkspaceAccess(ctx, workspaceID); err != nil { + return nil, err + } return s.store.ListIssueWatches(ctx, workspaceID) } -// ListAllIssueWatches returns every watch across all workspaces. +// ListAllIssueWatches returns every watch the caller may see. For a scoped +// caller that is only their own workspaces' watches; for an identity-less +// internal caller (unscoped) it is every watch, as before auth. The unscoped +// list form is only reachable when workspace_id is omitted, which the query-only +// integration middleware does not authorize — without this filter it leaked +// every workspace's watch config (filters, repo/agent/profile IDs, spawn prompt). func (s *Service) ListAllIssueWatches(ctx context.Context) ([]*IssueWatch, error) { - return s.store.ListAllIssueWatches(ctx) + watches, err := s.store.ListAllIssueWatches(ctx) + if err != nil { + return nil, err + } + return s.filterIssueWatchesByAccess(ctx, watches) +} + +// filterIssueWatchesByAccess keeps only watches whose workspace the caller may +// access. Access decisions are memoized per workspace so a long list costs one +// authorize call per distinct workspace, not one per watch. Only an +// ErrWorkspaceNotFound denial drops a watch; any other authorizer error (a +// transient DB failure, say) is propagated so the caller sees the failure +// rather than a silently truncated 200. +func (s *Service) filterIssueWatchesByAccess(ctx context.Context, watches []*IssueWatch) ([]*IssueWatch, error) { + decision := make(map[string]bool) + visible := make([]*IssueWatch, 0, len(watches)) + for _, w := range watches { + allowed, seen := decision[w.WorkspaceID] + if !seen { + switch err := s.authorizeWorkspaceAccess(ctx, w.WorkspaceID); { + case err == nil: + allowed = true + case errors.Is(err, repoerrors.ErrWorkspaceNotFound): + allowed = false + default: + return nil, err + } + decision[w.WorkspaceID] = allowed + } + if allowed { + visible = append(visible, w) + } + } + return visible, nil } // GetIssueWatch returns a single watch by ID or ErrIssueWatchNotFound. @@ -90,6 +139,11 @@ func (s *Service) UpdateIssueWatch(ctx context.Context, id string, req *UpdateIs if err != nil { return nil, err } + // Authorize off the loaded row's workspace — the watch ID alone reveals + // nothing about ownership, so a caller must not mutate another user's watch. + if err := s.authorizeWorkspaceAccess(ctx, w.WorkspaceID); err != nil { + return nil, err + } prevRepositoryID, prevBaseBranch := w.RepositoryID, w.BaseBranch applyIssueWatchPatch(w, req) if filterIsEmpty(w.Filter) { diff --git a/apps/backend/internal/slack/copy.go b/apps/backend/internal/slack/copy.go index 5039535a6a..8587b7b477 100644 --- a/apps/backend/internal/slack/copy.go +++ b/apps/backend/internal/slack/copy.go @@ -22,11 +22,16 @@ var ErrNothingToCopy = errors.New("slack: source workspace has no configuration // secrets are duplicated. The target's health probe re-runs so runtime fields // (team/user id) repopulate from the copied credentials. func (s *Service) CopyConfigToWorkspace(ctx context.Context, sourceWorkspaceID, targetWorkspaceID string) (*SlackConfig, error) { - sourceWorkspaceID, err := s.normalizeWorkspaceID(sourceWorkspaceID) + // Authorize BOTH workspaces up front. The integration middleware only + // authorizes the source (workspace_id query param); the target arrives in the + // request body and is otherwise unchecked, so a caller could copy their config + // and credentials into another user's workspace by naming its ID here. Guard + // before the target health-row write below, not just before the config write. + sourceWorkspaceID, err := s.resolveWorkspaceID(ctx, sourceWorkspaceID) if err != nil { return nil, err } - targetWorkspaceID, err = s.normalizeWorkspaceID(targetWorkspaceID) + targetWorkspaceID, err = s.resolveWorkspaceID(ctx, targetWorkspaceID) if err != nil { return nil, err } diff --git a/apps/backend/internal/slack/handlers.go b/apps/backend/internal/slack/handlers.go index 4c01cd71e1..f3009cc160 100644 --- a/apps/backend/internal/slack/handlers.go +++ b/apps/backend/internal/slack/handlers.go @@ -10,9 +10,17 @@ import ( "go.uber.org/zap" "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/task/repository/repoerrors" ws "github.com/kandev/kandev/pkg/websocket" ) +// workspaceDenied reports whether err is the per-user workspace access denial +// surfaced by the workspace authorizer. Handlers map it to 404 so a workspace +// the caller may not access is indistinguishable from a missing one. +func workspaceDenied(err error) bool { + return errors.Is(err, repoerrors.ErrWorkspaceNotFound) +} + // RegisterRoutes wires the Slack HTTP and WebSocket handlers. func RegisterRoutes(router *gin.Engine, dispatcher *ws.Dispatcher, svc *Service, log *logger.Logger) { ctrl := &Controller{service: svc, logger: log} @@ -65,6 +73,8 @@ func (c *Controller) httpCopyConfig(ctx *gin.Context) { switch { case errors.Is(err, ErrSameWorkspace), errors.Is(err, ErrNothingToCopy), errors.Is(err, ErrInvalidConfig): status = http.StatusBadRequest + case workspaceDenied(err): + status = http.StatusNotFound } ctx.JSON(status, gin.H{"error": err.Error()}) return @@ -77,6 +87,10 @@ func (c *Controller) httpCopyConfig(ctx *gin.Context) { func (c *Controller) httpGetConfig(ctx *gin.Context) { cfg, err := c.service.GetConfigForWorkspace(ctx.Request.Context(), c.workspaceID(ctx)) if err != nil { + if workspaceDenied(err) { + ctx.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"}) + return + } ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return } @@ -96,8 +110,11 @@ func (c *Controller) httpSetConfig(ctx *gin.Context) { cfg, err := c.service.SetConfigForWorkspace(ctx.Request.Context(), c.workspaceID(ctx), &req) if err != nil { status := http.StatusInternalServerError - if errors.Is(err, ErrInvalidConfig) { + switch { + case errors.Is(err, ErrInvalidConfig): status = http.StatusBadRequest + case workspaceDenied(err): + status = http.StatusNotFound } ctx.JSON(status, gin.H{"error": err.Error()}) return @@ -107,6 +124,10 @@ func (c *Controller) httpSetConfig(ctx *gin.Context) { func (c *Controller) httpDeleteConfig(ctx *gin.Context) { if err := c.service.DeleteConfigForWorkspace(ctx.Request.Context(), c.workspaceID(ctx)); err != nil { + if workspaceDenied(err) { + ctx.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"}) + return + } ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return } @@ -136,6 +157,13 @@ func (c *Controller) workspaceID(ctx *gin.Context) string { const errCodeSlackNotConfigured = "SLACK_NOT_CONFIGURED" func (c *Controller) writeClientError(ctx *gin.Context, err error) { + if workspaceDenied(err) { + // A workspace the caller may not access is indistinguishable from a + // missing one — 404, not 403/500. Covers TestConnection and data-plane + // reads (via clientFor). + ctx.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"}) + return + } if errors.Is(err, ErrNotConfigured) { ctx.JSON(http.StatusServiceUnavailable, gin.H{ "error": "Slack is not configured", diff --git a/apps/backend/internal/slack/scope_conn_test.go b/apps/backend/internal/slack/scope_conn_test.go new file mode 100644 index 0000000000..095a3360d1 --- /dev/null +++ b/apps/backend/internal/slack/scope_conn_test.go @@ -0,0 +1,37 @@ +package slack + +import ( + "context" + "errors" + "testing" + + "github.com/kandev/kandev/internal/task/repository/repoerrors" +) + +// TestTestConnection_DeniesForeignWorkspace is the regression for the review's +// P1 credential-exfiltration path: a caller must not cause a foreign/default +// workspace's stored Slack credentials to be used. +func TestTestConnection_DeniesForeignWorkspace(t *testing.T) { + ctx := context.Background() + svc := newCopyTestService(t, newFakeSecrets()) + svc.SetWorkspaceAuthorizer(denyOnly("ws-foreign")) + + result, err := svc.TestConnectionForWorkspace(ctx, "ws-foreign", &SetConfigRequest{ + AuthMethod: AuthMethodCookie, Token: "t", Cookie: "c", + }) + if !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Fatalf("expected ErrWorkspaceNotFound, got %v (result=%+v)", err, result) + } +} + +// TestClientFor_DeniesForeignWorkspace covers the clientFor chokepoint that all +// data-plane reads funnel through. +func TestClientFor_DeniesForeignWorkspace(t *testing.T) { + ctx := context.Background() + svc := newCopyTestService(t, newFakeSecrets()) + svc.SetWorkspaceAuthorizer(denyOnly("ws-foreign")) + + if _, err := svc.ClientForWorkspace(ctx, "ws-foreign"); !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Fatalf("expected ErrWorkspaceNotFound, got %v", err) + } +} diff --git a/apps/backend/internal/slack/scope_test.go b/apps/backend/internal/slack/scope_test.go new file mode 100644 index 0000000000..2daf24bce5 --- /dev/null +++ b/apps/backend/internal/slack/scope_test.go @@ -0,0 +1,110 @@ +package slack + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/kandev/kandev/internal/task/repository/repoerrors" +) + +// denyOnly returns a workspace authorizer that allows every workspace except +// the named ones, which it denies with the same ErrWorkspaceNotFound the real +// task-service authorizer returns. +func denyOnly(denied ...string) func(context.Context, string) error { + blocked := make(map[string]bool, len(denied)) + for _, ws := range denied { + blocked[ws] = true + } + return func(_ context.Context, ws string) error { + if blocked[ws] { + return repoerrors.ErrWorkspaceNotFound + } + return nil + } +} + +// TestCopyConfigToWorkspace_DeniesForeignTarget is the regression for the HIGH +// finding: the copy target arrives in the request body, which the query-only +// integration middleware never authorizes, so without a service-layer check a +// caller could copy their config + credentials into another user's workspace. +func TestCopyConfigToWorkspace_DeniesForeignTarget(t *testing.T) { + ctx := context.Background() + secrets := newFakeSecrets() + svc := newCopyTestService(t, secrets) + done := make(chan struct{}, 4) + svc.SetProbeHook(func() { done <- struct{}{} }) + + const src, victim = "ws-src", "ws-victim" + if _, err := svc.SetConfigForWorkspace(ctx, src, &SetConfigRequest{ + AuthMethod: AuthMethodCookie, + Token: "xoxc-attacker", + Cookie: "d-attacker", + }); err != nil { + t.Fatalf("seed source: %v", err) + } + <-done + + // The victim already has a healthy Slack connection of their own. A denied + // copy must leave it byte-for-byte untouched — including its health status, + // which the up-front guard protects from the pre-write UpdateAuthHealth flip. + if err := svc.store.UpsertConfigForWorkspace(ctx, victim, &SlackConfig{ + AuthMethod: AuthMethodCookie, CommandPrefix: "!vic", UtilityAgentID: "ua-vic", + }); err != nil { + t.Fatalf("seed victim config: %v", err) + } + if err := secrets.Set(ctx, SecretKeyForToken(victim), "Slack token", "xoxc-victim"); err != nil { + t.Fatalf("seed victim token: %v", err) + } + if err := secrets.Set(ctx, SecretKeyForCookie(victim), "Slack cookie", "d-victim"); err != nil { + t.Fatalf("seed victim cookie: %v", err) + } + if err := svc.store.UpdateAuthHealthForWorkspace(ctx, victim, true, "", "", "", time.Now().UTC()); err != nil { + t.Fatalf("seed victim health: %v", err) + } + + svc.SetWorkspaceAuthorizer(denyOnly(victim)) + + if _, err := svc.CopyConfigToWorkspace(ctx, src, victim); !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Fatalf("expected ErrWorkspaceNotFound, got %v", err) + } + got, cfgErr := svc.store.GetConfigForWorkspace(ctx, victim) + if cfgErr != nil || got == nil { + t.Fatalf("victim config vanished: cfg=%+v err=%v", got, cfgErr) + } + if got.CommandPrefix != "!vic" { + t.Errorf("victim config overwritten by copy: %q", got.CommandPrefix) + } + if !got.LastOk { + t.Errorf("victim health flipped to unhealthy by unauthorized copy") + } + if v := secrets.get(SecretKeyForToken(victim)); v != "xoxc-victim" { + t.Errorf("victim token overwritten by copy: %q", v) + } + if v := secrets.get(SecretKeyForCookie(victim)); v != "d-victim" { + t.Errorf("victim cookie overwritten by copy: %q", v) + } +} + +// TestConfigForWorkspace_DeniesForeignWorkspace is the regression for the LOW +// finding: a scoped caller must not read, write, or delete a workspace's config +// it does not own. +func TestConfigForWorkspace_DeniesForeignWorkspace(t *testing.T) { + ctx := context.Background() + svc := newCopyTestService(t, newFakeSecrets()) + const foreign = "ws-foreign" + svc.SetWorkspaceAuthorizer(denyOnly(foreign)) + + if _, err := svc.GetConfigForWorkspace(ctx, foreign); !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Errorf("Get: expected ErrWorkspaceNotFound, got %v", err) + } + if _, err := svc.SetConfigForWorkspace(ctx, foreign, &SetConfigRequest{ + AuthMethod: AuthMethodCookie, Token: "t", Cookie: "c", + }); !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Errorf("Set: expected ErrWorkspaceNotFound, got %v", err) + } + if err := svc.DeleteConfigForWorkspace(ctx, foreign); !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Errorf("Delete: expected ErrWorkspaceNotFound, got %v", err) + } +} diff --git a/apps/backend/internal/slack/service.go b/apps/backend/internal/slack/service.go index 802e85ec57..52c02628b2 100644 --- a/apps/backend/internal/slack/service.go +++ b/apps/backend/internal/slack/service.go @@ -36,6 +36,43 @@ type Service struct { clientFn ClientFactory clients map[string]Client probeHook func() + // workspaceAuthorizer is the per-user workspace access boundary, wired + // post-construction via SetWorkspaceAuthorizer. Nil (unit tests, auth + // disabled) means unscoped — every workspace is visible, as before auth. + workspaceAuthorizer func(context.Context, string) error +} + +// SetWorkspaceAuthorizer installs the per-user workspace access boundary applied +// before user-facing Slack config reads/mutations. Wired to +// taskSvc.AuthorizeWorkspaceAccess so a caller cannot reach a workspace it does +// not own by naming its ID (e.g. the copy target, or the resolved default). +func (s *Service) SetWorkspaceAuthorizer(authorizer func(context.Context, string) error) { + if s != nil { + s.workspaceAuthorizer = authorizer + } +} + +func (s *Service) authorizeWorkspaceAccess(ctx context.Context, workspaceID string) error { + if s == nil || s.workspaceAuthorizer == nil { + return nil + } + return s.workspaceAuthorizer(ctx, workspaceID) +} + +// resolveWorkspaceID normalizes an optional workspace ID (empty → default) and +// authorizes the caller against the resolved workspace. Using it instead of +// normalizeWorkspaceID on user-facing entry points closes the default-workspace +// fallback: a scoped caller that omits workspace_id cannot land on a global +// default it does not own. +func (s *Service) resolveWorkspaceID(ctx context.Context, workspaceID string) (string, error) { + workspaceID, err := s.normalizeWorkspaceID(workspaceID) + if err != nil { + return "", err + } + if err := s.authorizeWorkspaceAccess(ctx, workspaceID); err != nil { + return "", err + } + return workspaceID, nil } // AgentRunner runs the configured utility agent for a Slack match. Defined as @@ -101,7 +138,7 @@ func (s *Service) GetConfig(ctx context.Context) (*SlackConfig, error) { // GetConfigForWorkspace returns a workspace config enriched with HasToken/HasCookie. func (s *Service) GetConfigForWorkspace(ctx context.Context, workspaceID string) (*SlackConfig, error) { - workspaceID, err := s.normalizeWorkspaceID(workspaceID) + workspaceID, err := s.resolveWorkspaceID(ctx, workspaceID) if err != nil { return nil, err } @@ -165,7 +202,7 @@ func (s *Service) SetConfig(ctx context.Context, req *SetConfigRequest) (*SlackC // SetConfigForWorkspace is upsert. Empty Token/Cookie on update keeps existing values. func (s *Service) SetConfigForWorkspace(ctx context.Context, workspaceID string, req *SetConfigRequest) (*SlackConfig, error) { - workspaceID, err := s.normalizeWorkspaceID(workspaceID) + workspaceID, err := s.resolveWorkspaceID(ctx, workspaceID) if err != nil { return nil, err } @@ -202,7 +239,7 @@ func (s *Service) DeleteConfig(ctx context.Context) error { // DeleteConfigForWorkspace removes both the row and the stored secrets. func (s *Service) DeleteConfigForWorkspace(ctx context.Context, workspaceID string) error { - workspaceID, err := s.normalizeWorkspaceID(workspaceID) + workspaceID, err := s.resolveWorkspaceID(ctx, workspaceID) if err != nil { return err } @@ -240,6 +277,13 @@ func (s *Service) TestConnectionForWorkspace(ctx context.Context, workspaceID st if err != nil { return &TestConnectionResult{OK: false, Error: err.Error()}, nil } + // Authorize before revealing/using the workspace's stored credentials: an + // omitted workspace_id resolves the global default. Return a hard error (not + // a swallowed {OK:false} result) so the denial is a 404 rather than a + // probeable "test failed". + if err := s.authorizeWorkspaceAccess(ctx, workspaceID); err != nil { + return nil, err + } cfg, token, cookie, err := s.resolveCredentials(ctx, workspaceID, req) if err != nil { return &TestConnectionResult{OK: false, Error: err.Error()}, nil @@ -363,7 +407,10 @@ func (s *Service) ClientForWorkspace(ctx context.Context, workspaceID string) (C } func (s *Service) clientFor(ctx context.Context, workspaceID string) (Client, error) { - workspaceID, err := s.normalizeWorkspaceID(workspaceID) + // clientFor is the single chokepoint for data-plane reads, so authorizing the + // resolved workspace here scopes them — an omitted workspace_id must not + // resolve another user's default workspace. + workspaceID, err := s.resolveWorkspaceID(ctx, workspaceID) if err != nil { return nil, err }