fix(auth): scope integration config/watch routes to workspace owner (#1960)
* 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) <noreply@anthropic.com>
* 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) <noreply@anthropic.com>
---------
Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -21,6 +21,32 @@ This file covers both halves of the playbook (backend + frontend) so the pattern
|
||||
- "Auth required / reconnect" UI reuses `<IntegrationAuthErrorMessage>` (`components/integrations/auth-error-message.tsx`) — supply the integration's display name, regex check, and reconnect href.
|
||||
- Link / import popovers reuse `<ValidatedPopover>` (`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.
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user