diff --git a/apps/backend/internal/backendapp/services.go b/apps/backend/internal/backendapp/services.go index 88ad98e1f0..074966639a 100644 --- a/apps/backend/internal/backendapp/services.go +++ b/apps/backend/internal/backendapp/services.go @@ -615,6 +615,7 @@ func initWorkflowSyncService(dbPool *db.Pool, githubSvc *github.Service, workflo log.Warn("workflow sync service initialization failed (non-fatal)", zap.Error(err)) return nil } + svc.SetWorkspaceAuthorizer(taskSvc.AuthorizeWorkspaceAccess) return svc } diff --git a/apps/backend/internal/backendapp/workflowsync_wiring_test.go b/apps/backend/internal/backendapp/workflowsync_wiring_test.go new file mode 100644 index 0000000000..c0d8aa9faa --- /dev/null +++ b/apps/backend/internal/backendapp/workflowsync_wiring_test.go @@ -0,0 +1,85 @@ +package backendapp + +import ( + "context" + "errors" + "path/filepath" + "testing" + + "github.com/jmoiron/sqlx" + + "github.com/kandev/kandev/internal/auth/authn" + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/db" + "github.com/kandev/kandev/internal/github" + "github.com/kandev/kandev/internal/task/repository/repoerrors" + taskservice "github.com/kandev/kandev/internal/task/service" +) + +// TestInitWorkflowSyncService_WiresRealWorkspaceAuthorization calls the real +// initWorkflowSyncService boot function — not a hand-copied re-wiring — with a +// real taskservice.Service backed by SQLite, so a regression that removes or +// breaks the SetWorkspaceAuthorizer(taskSvc.AuthorizeWorkspaceAccess) line +// inside it fails this test. The workflowsync package's own tests only ever +// install a hand-written fake authorizer and cannot detect that class of +// regression. +func TestInitWorkflowSyncService_WiresRealWorkspaceAuthorization(t *testing.T) { + harness := newBootStateTestHarness(t) + ctx := context.Background() + + ownerCtx := authn.WithIdentity(ctx, authn.Identity{UserID: "owner-1", Role: authn.RoleMember}) + workspace, err := harness.taskSvc.CreateWorkspace(ownerCtx, &taskservice.CreateWorkspaceRequest{ + Name: "victim workspace", + }) + if err != nil { + t.Fatalf("create workspace: %v", err) + } + if workspace.OwnerID != "owner-1" { + t.Fatalf("workspace owner = %q, want owner-1 (authenticated caller owns what it creates)", workspace.OwnerID) + } + + dbConn, err := db.OpenSQLite(filepath.Join(t.TempDir(), "workflowsync.db")) + if err != nil { + t.Fatalf("OpenSQLite: %v", err) + } + sqlxDB := sqlx.NewDb(dbConn, "sqlite3") + t.Cleanup(func() { + if err := sqlxDB.Close(); err != nil { + t.Errorf("close db: %v", err) + } + }) + log, err := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "console"}) + if err != nil { + t.Fatalf("logger: %v", err) + } + + // A non-nil GitHub service is required for initWorkflowSyncService to + // wire anything at all (see its early "GitHub unavailable" return); the + // client itself is never exercised by this test. + githubSvc := github.NewService(nil, github.AuthMethodNone, nil, nil, nil, log) + + svc := initWorkflowSyncService(db.NewPool(sqlxDB, sqlxDB), githubSvc, harness.workflowSvc, harness.taskSvc, log) + if svc == nil { + t.Fatal("initWorkflowSyncService returned nil") + } + + if _, err := svc.GetConfigForWorkspace(ownerCtx, workspace.ID); err != nil { + t.Fatalf("owner GetConfigForWorkspace: %v", err) + } + + foreignCtx := authn.WithIdentity(ctx, authn.Identity{UserID: "attacker-1", Role: authn.RoleMember}) + _, err = svc.GetConfigForWorkspace(foreignCtx, workspace.ID) + if !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Fatalf("foreign identity GetConfigForWorkspace err = %v, want ErrWorkspaceNotFound", err) + } + + // Internal caller (no identity in context — matches the periodic poller) + // and the pre-auth synthetic identity are both unscoped. + if _, err := svc.GetConfigForWorkspace(ctx, workspace.ID); err != nil { + t.Fatalf("identity-free GetConfigForWorkspace: %v", err) + } + syntheticCtx := authn.WithIdentity(ctx, authn.Identity{UserID: "single-user", Role: authn.RoleAdmin, Synthetic: true}) + if _, err := svc.GetConfigForWorkspace(syntheticCtx, workspace.ID); err != nil { + t.Fatalf("synthetic identity GetConfigForWorkspace: %v", err) + } +} diff --git a/apps/backend/internal/workflowsync/handlers.go b/apps/backend/internal/workflowsync/handlers.go index a7b69b177d..e9ec6c0de9 100644 --- a/apps/backend/internal/workflowsync/handlers.go +++ b/apps/backend/internal/workflowsync/handlers.go @@ -9,8 +9,18 @@ import ( "go.uber.org/zap" "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/task/repository/repoerrors" ) +// 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 — the same convention used by every other integration in this +// codebase (see internal/jira/handlers.go). +func workspaceDenied(err error) bool { + return errors.Is(err, repoerrors.ErrWorkspaceNotFound) +} + // Controller holds HTTP route handlers for workflow sync. type Controller struct { service *Service @@ -46,6 +56,10 @@ func (c *Controller) httpGetConfig(ctx *gin.Context) { return } cfg, err := c.service.GetConfigForWorkspace(ctx.Request.Context(), workspaceID) + if workspaceDenied(err) { + ctx.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"}) + return + } if err != nil { c.internalError(ctx, "failed to load workflow sync config", err) return @@ -75,6 +89,10 @@ func (c *Controller) httpSetConfig(ctx *gin.Context) { return } cfg, err := c.service.SetConfigForWorkspace(ctx.Request.Context(), workspaceID, &req) + if workspaceDenied(err) { + ctx.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"}) + return + } if errors.Is(err, ErrInvalidConfig) { ctx.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) return @@ -91,7 +109,12 @@ func (c *Controller) httpDeleteConfig(ctx *gin.Context) { if !ok { return } - if err := c.service.DeleteConfigForWorkspace(ctx.Request.Context(), workspaceID); err != nil { + err := c.service.DeleteConfigForWorkspace(ctx.Request.Context(), workspaceID) + if workspaceDenied(err) { + ctx.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"}) + return + } + if err != nil { c.internalError(ctx, "failed to remove workflow sync config", err) return } @@ -108,11 +131,19 @@ func (c *Controller) httpForceSync(ctx *gin.Context) { return } result, syncErr := c.service.SyncWorkspace(ctx.Request.Context(), workspaceID) + if workspaceDenied(syncErr) { + ctx.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"}) + return + } if errors.Is(syncErr, ErrNotConfigured) { ctx.JSON(http.StatusNotFound, gin.H{"error": syncErr.Error()}) return } cfg, err := c.service.GetConfigForWorkspace(ctx.Request.Context(), workspaceID) + if workspaceDenied(err) { + ctx.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"}) + return + } if err != nil { c.internalError(ctx, "failed to load workflow sync config", err) return diff --git a/apps/backend/internal/workflowsync/handlers_test.go b/apps/backend/internal/workflowsync/handlers_test.go new file mode 100644 index 0000000000..3c037c26ab --- /dev/null +++ b/apps/backend/internal/workflowsync/handlers_test.go @@ -0,0 +1,210 @@ +package workflowsync + +import ( + "bytes" + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/kandev/kandev/internal/auth/authn" + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/task/repository/repoerrors" +) + +const ( + victimWorkspace = "ws-victim" + victimOwnerID = "owner-1" + attackerID = "attacker-1" + victimRepoOwner = "victim-org" + victimRepoName = "victim-repo" + victimBranch = "victim-branch" + victimPath = "victim/path" +) + +// ownerOnlyAuthorizer mirrors the real callerScope semantics documented in +// apps/backend/AGENTS.md: no identity in context, or a synthetic identity +// (auth disabled), is unscoped; a real identity may only reach the workspace +// it owns. +func ownerOnlyAuthorizer(ownerID, workspaceID string) func(context.Context, string) error { + return func(ctx context.Context, wsID string) error { + identity, ok := authn.IdentityFromContext(ctx) + if !ok || identity.Synthetic { + return nil + } + if wsID == workspaceID && identity.UserID == ownerID { + return nil + } + return repoerrors.ErrWorkspaceNotFound + } +} + +func newTestRouter(t *testing.T, svc *Service) *gin.Engine { + t.Helper() + gin.SetMode(gin.TestMode) + router := gin.New() + log, err := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "console"}) + require.NoError(t, err) + RegisterRoutes(router, svc, log) + return router +} + +func withIdentity(req *http.Request, identity authn.Identity) *http.Request { + return req.WithContext(authn.WithIdentity(req.Context(), identity)) +} + +// configOp is one of the four workflow-sync HTTP entry points, parameterized +// by workspace ID so every test in this file can drive all four uniformly. +type configOp struct { + name string + method string + path func(workspaceID string) string + body []byte +} + +var allConfigOps = []configOp{ + { + name: "get", method: http.MethodGet, + path: func(ws string) string { return "/api/v1/workflow-sync/config?workspace_id=" + ws }, + }, + { + name: "post", method: http.MethodPost, + path: func(ws string) string { return "/api/v1/workflow-sync/config?workspace_id=" + ws }, + body: []byte(`{"repo_owner":"attacker","repo_name":"evil"}`), + }, + { + name: "delete", method: http.MethodDelete, + path: func(ws string) string { return "/api/v1/workflow-sync/config?workspace_id=" + ws }, + }, + { + name: "sync", method: http.MethodPost, + path: func(ws string) string { return "/api/v1/workflow-sync/sync?workspace_id=" + ws }, + }, +} + +func doOp(t *testing.T, router *gin.Engine, op configOp, workspaceID string, identity authn.Identity) *httptest.ResponseRecorder { + t.Helper() + var body *bytes.Reader + if op.body != nil { + body = bytes.NewReader(op.body) + } else { + body = bytes.NewReader(nil) + } + req := withIdentity(httptest.NewRequest(op.method, op.path(workspaceID), body), identity) + resp := httptest.NewRecorder() + router.ServeHTTP(resp, req) + return resp +} + +// newOwnedWorkspace builds a fresh service + router with one configured +// workspace and the given owner wired as the sole authorized identity. Each +// op-level test gets its own instance so DELETE/POST attempts in one subtest +// can't affect another's assertions. +func newOwnedWorkspace(t *testing.T) (*gin.Engine, string) { + t.Helper() + svc, _ := setupTestService(t, seededMockClient()) + configureWorkspace(t, svc, victimWorkspace) + svc.SetWorkspaceAuthorizer(ownerOnlyAuthorizer(victimOwnerID, victimWorkspace)) + return newTestRouter(t, svc), victimWorkspace +} + +func TestHTTPHandlers_OwnerSucceedsOnAllRoutes(t *testing.T) { + owner := authn.Identity{UserID: victimOwnerID, Role: authn.RoleMember} + for _, op := range allConfigOps { + t.Run(op.name, func(t *testing.T) { + router, ws := newOwnedWorkspace(t) + resp := doOp(t, router, op, ws, owner) + assert.Equal(t, http.StatusOK, resp.Code, "response body: %s", resp.Body.String()) + }) + } +} + +func TestHTTPHandlers_SyntheticIdentitySucceedsOnAllRoutes(t *testing.T) { + synthetic := authn.Identity{UserID: "single-user", Role: authn.RoleAdmin, Synthetic: true} + for _, op := range allConfigOps { + t.Run(op.name, func(t *testing.T) { + router, ws := newOwnedWorkspace(t) + resp := doOp(t, router, op, ws, synthetic) + assert.Equal(t, http.StatusOK, resp.Code, "response body: %s", resp.Body.String()) + }) + } +} + +func TestHTTPHandlers_ForeignMemberDeniedOnAllRoutesWithoutLeaking(t *testing.T) { + svc, applier := setupTestService(t, seededMockClient()) + configureWorkspace(t, svc, victimWorkspace) + require.NoError(t, updateVictimRepo(t, svc)) + before, err := svc.store.GetConfigForWorkspace(context.Background(), victimWorkspace) + require.NoError(t, err) + require.NotNil(t, before) + + svc.SetWorkspaceAuthorizer(ownerOnlyAuthorizer(victimOwnerID, victimWorkspace)) + router := newTestRouter(t, svc) + attacker := authn.Identity{UserID: attackerID, Role: authn.RoleMember} + + for _, op := range allConfigOps { + t.Run(op.name, func(t *testing.T) { + resp := doOp(t, router, op, victimWorkspace, attacker) + assert.Equal(t, http.StatusNotFound, resp.Code, "response body: %s", resp.Body.String()) + for _, secret := range []string{victimRepoOwner, victimRepoName, victimBranch, victimPath} { + assert.NotContains(t, resp.Body.String(), secret) + } + }) + } + + after, err := svc.store.GetConfigForWorkspace(context.Background(), victimWorkspace) + require.NoError(t, err) + require.NotNil(t, after) + assert.Equal(t, *before, *after, "denied requests must not change any field of the victim's config") + assert.Empty(t, applier.released, "denied delete must not release synced workflows") + assert.Zero(t, applier.callCount(), "denied sync must never reach the applier") +} + +// TestHTTPHandlers_ForceSyncDeniesSecondLookupAfterAccessRevoked covers the +// narrow race in httpForceSync: SyncWorkspace and the follow-up +// GetConfigForWorkspace (used to build the response) are two separate +// authorization checks. If access is revoked between them — say a workspace +// is deleted, or reassigned, mid-request — the second call must still map to +// a sanitized 404, not fall through to the generic 500 path. +func TestHTTPHandlers_ForceSyncDeniesSecondLookupAfterAccessRevoked(t *testing.T) { + svc, _ := setupTestService(t, seededMockClient()) + configureWorkspace(t, svc, victimWorkspace) + + calls := 0 + svc.SetWorkspaceAuthorizer(func(context.Context, string) error { + calls++ + if calls == 1 { + return nil + } + return repoerrors.ErrWorkspaceNotFound + }) + router := newTestRouter(t, svc) + owner := authn.Identity{UserID: victimOwnerID, Role: authn.RoleMember} + + req := withIdentity(httptest.NewRequest(http.MethodPost, "/api/v1/workflow-sync/sync?workspace_id="+victimWorkspace, nil), owner) + resp := httptest.NewRecorder() + router.ServeHTTP(resp, req) + + assert.Equal(t, http.StatusNotFound, resp.Code, "response body: %s", resp.Body.String()) + assert.NotContains(t, resp.Body.String(), "acme") + assert.GreaterOrEqual(t, calls, 2, "test requires both the sync and the follow-up config lookup to run") +} + +// updateVictimRepo overwrites the seeded config with distinctive identity +// (repo, branch, and path) so leak assertions aren't relying on the shared +// default fixture values used elsewhere in this package's tests. +func updateVictimRepo(t *testing.T, svc *Service) error { + t.Helper() + _, err := svc.store.UpsertConfigForWorkspace(context.Background(), victimWorkspace, &SetConfigRequest{ + RepoOwner: victimRepoOwner, + RepoName: victimRepoName, + Branch: victimBranch, + Path: victimPath, + IntervalSeconds: DefaultIntervalSeconds, + }) + return err +} diff --git a/apps/backend/internal/workflowsync/service.go b/apps/backend/internal/workflowsync/service.go index ce286318ee..7cab8cc868 100644 --- a/apps/backend/internal/workflowsync/service.go +++ b/apps/backend/internal/workflowsync/service.go @@ -51,6 +51,27 @@ type Service struct { // delete for that workspace waits (bounded by the HTTP client timeout) // rather than racing an in-flight apply. locks sync.Map // workspaceID → *sync.Mutex + + // workspaceAuthorizer enforces per-user workspace scoping. Nil (unit + // tests, or a caller with no identity in context — internal callers like + // the periodic poller) means unscoped, matching every other integration + // service's default before auth is wired up. + workspaceAuthorizer func(context.Context, string) error +} + +// SetWorkspaceAuthorizer installs the per-user workspace access boundary +// applied before every user-facing config read/write and force sync. +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) } func (s *Service) workspaceLock(workspaceID string) *sync.Mutex { @@ -75,11 +96,17 @@ func (s *Service) Store() *Store { // GetConfigForWorkspace returns the workspace's config, or nil when unset. func (s *Service) GetConfigForWorkspace(ctx context.Context, workspaceID string) (*Config, error) { + if err := s.authorizeWorkspaceAccess(ctx, workspaceID); err != nil { + return nil, err + } return s.store.GetConfigForWorkspace(ctx, workspaceID) } // SetConfigForWorkspace validates and stores the workspace's config. func (s *Service) SetConfigForWorkspace(ctx context.Context, workspaceID string, req *SetConfigRequest) (*Config, error) { + if err := s.authorizeWorkspaceAccess(ctx, workspaceID); err != nil { + return nil, err + } if err := req.Normalize(); err != nil { return nil, err } @@ -93,6 +120,9 @@ func (s *Service) SetConfigForWorkspace(ctx context.Context, workspaceID string, // workflows are released back to manual ownership first so they become // editable again — a failed release keeps the config so the user can retry. func (s *Service) DeleteConfigForWorkspace(ctx context.Context, workspaceID string) error { + if err := s.authorizeWorkspaceAccess(ctx, workspaceID); err != nil { + return err + } lock := s.workspaceLock(workspaceID) lock.Lock() defer lock.Unlock() @@ -133,6 +163,9 @@ type fetchedFile struct { // silent. The outcome (including failures) is recorded on the config row so // the UI can surface it. func (s *Service) SyncWorkspace(ctx context.Context, workspaceID string) (*SyncResult, error) { + if err := s.authorizeWorkspaceAccess(ctx, workspaceID); err != nil { + return nil, err + } lock := s.workspaceLock(workspaceID) lock.Lock() defer lock.Unlock() diff --git a/apps/backend/internal/workflowsync/service_authorization_test.go b/apps/backend/internal/workflowsync/service_authorization_test.go new file mode 100644 index 0000000000..8de20af508 --- /dev/null +++ b/apps/backend/internal/workflowsync/service_authorization_test.go @@ -0,0 +1,117 @@ +package workflowsync + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/kandev/kandev/internal/github" + "github.com/kandev/kandev/internal/task/repository/repoerrors" +) + +// denyingAuthorizer asserts the exact workspace ID reaches the authorizer, +// then denies access — simulating a caller who does not own the workspace. +func denyingAuthorizer(t *testing.T, wantWorkspaceID string) func(context.Context, string) error { + t.Helper() + return func(_ context.Context, workspaceID string) error { + if workspaceID != wantWorkspaceID { + t.Fatalf("authorizer workspace = %q, want %q", workspaceID, wantWorkspaceID) + } + return repoerrors.ErrWorkspaceNotFound + } +} + +func TestGetConfigForWorkspace_DeniesForeignWorkspace(t *testing.T) { + svc, _ := setupTestService(t, github.NewMockClient()) + configureWorkspace(t, svc, "victim") + svc.SetWorkspaceAuthorizer(denyingAuthorizer(t, "victim")) + + cfg, err := svc.GetConfigForWorkspace(context.Background(), "victim") + assert.ErrorIs(t, err, repoerrors.ErrWorkspaceNotFound) + assert.Nil(t, cfg) +} + +func TestSetConfigForWorkspace_DeniesForeignWorkspaceAndLeavesConfigUnchanged(t *testing.T) { + svc, _ := setupTestService(t, github.NewMockClient()) + configureWorkspace(t, svc, "victim") + before, err := svc.store.GetConfigForWorkspace(context.Background(), "victim") + require.NoError(t, err) + require.NotNil(t, before) + + svc.SetWorkspaceAuthorizer(denyingAuthorizer(t, "victim")) + _, err = svc.SetConfigForWorkspace(context.Background(), "victim", &SetConfigRequest{ + RepoOwner: "attacker", + RepoName: "evil", + }) + assert.ErrorIs(t, err, repoerrors.ErrWorkspaceNotFound) + + after, err := svc.store.GetConfigForWorkspace(context.Background(), "victim") + require.NoError(t, err) + require.NotNil(t, after) + assert.Equal(t, before.RepoOwner, after.RepoOwner) + assert.Equal(t, before.RepoName, after.RepoName) +} + +func TestDeleteConfigForWorkspace_DeniesForeignWorkspaceAndLeavesConfigInPlace(t *testing.T) { + svc, applier := setupTestService(t, github.NewMockClient()) + configureWorkspace(t, svc, "victim") + + svc.SetWorkspaceAuthorizer(denyingAuthorizer(t, "victim")) + err := svc.DeleteConfigForWorkspace(context.Background(), "victim") + assert.ErrorIs(t, err, repoerrors.ErrWorkspaceNotFound) + + cfg, err := svc.store.GetConfigForWorkspace(context.Background(), "victim") + require.NoError(t, err) + assert.NotNil(t, cfg) + assert.Empty(t, applier.released, "denied delete must not release synced workflows") +} + +func TestSyncWorkspace_DeniesForeignWorkspaceAndNeverApplies(t *testing.T) { + svc, applier := setupTestService(t, seededMockClient()) + configureWorkspace(t, svc, "victim") + + svc.SetWorkspaceAuthorizer(denyingAuthorizer(t, "victim")) + _, err := svc.SyncWorkspace(context.Background(), "victim") + assert.ErrorIs(t, err, repoerrors.ErrWorkspaceNotFound) + assert.Zero(t, applier.callCount(), "denied sync must never reach the applier") +} + +// The internal periodic poller (SyncDueConfigs) and any caller that never +// wires an authorizer must keep working exactly as before this feature — +// authorization is opt-in scoping, not a default-deny gate. +func TestServiceMethods_SucceedWhenNoAuthorizerWired(t *testing.T) { + svc, _ := setupTestService(t, seededMockClient()) + configureWorkspace(t, svc, "ws-1") + + cfg, err := svc.GetConfigForWorkspace(context.Background(), "ws-1") + require.NoError(t, err) + assert.NotNil(t, cfg) + + _, err = svc.SyncWorkspace(context.Background(), "ws-1") + require.NoError(t, err) + + err = svc.DeleteConfigForWorkspace(context.Background(), "ws-1") + require.NoError(t, err) +} + +func TestServiceMethods_SucceedWhenAuthorizerAllows(t *testing.T) { + svc, _ := setupTestService(t, seededMockClient()) + configureWorkspace(t, svc, "ws-1") + svc.SetWorkspaceAuthorizer(func(context.Context, string) error { return nil }) + + cfg, err := svc.GetConfigForWorkspace(context.Background(), "ws-1") + require.NoError(t, err) + assert.NotNil(t, cfg) + + _, err = svc.SetConfigForWorkspace(context.Background(), "ws-1", &SetConfigRequest{ + RepoOwner: "acme", RepoName: "flows", + }) + require.NoError(t, err) + + _, err = svc.SyncWorkspace(context.Background(), "ws-1") + require.NoError(t, err) + + require.NoError(t, svc.DeleteConfigForWorkspace(context.Background(), "ws-1")) +} diff --git a/docs/plans/workflow-sync-workspace-authz/plan.md b/docs/plans/workflow-sync-workspace-authz/plan.md new file mode 100644 index 0000000000..b9e926bc1c --- /dev/null +++ b/docs/plans/workflow-sync-workspace-authz/plan.md @@ -0,0 +1,141 @@ +--- +spec: docs/specs/workflow-sync-workspace-authz/spec.md +created: 2026-08-06 +status: building +--- + +# Implementation Plan: Workflow Sync — Per-User Workspace Authorization + +## Overview +Close the workspace-scoping gap in `internal/workflowsync` by reusing the exact pattern already +used by every other per-workspace integration service in this codebase (GitHub, Jira, Linear, +Slack, Azure DevOps, Automation): a nil-safe `workspaceAuthorizer` field on `workflowsync.Service`, +set post-construction from `backendapp` with `taskSvc.AuthorizeWorkspaceAccess`, checked first in +every user-facing service method. Handlers map the resulting `ErrWorkspaceNotFound` to a sanitized +404. Two tasks, sequential (the second's handler test depends on the first's service change): +service-layer authorization + wiring, then handler-layer error mapping + HTTP-level regression +tests. + +--- + +## Backend + +### Service layer (`internal/workflowsync/service.go`) +Add, mirroring `internal/github/service.go:137,169-181` exactly: +```go +type Service struct { + // ...existing fields... + workspaceAuthorizer func(context.Context, string) error +} + +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) +} +``` +Call `if err := s.authorizeWorkspaceAccess(ctx, workspaceID); err != nil { return nil, err }` +(or `return err` for the delete path) as the first line of the body in: +- `GetConfigForWorkspace` +- `SetConfigForWorkspace` (before `req.Normalize()` — no need to validate an attacker's payload) +- `DeleteConfigForWorkspace` +- `SyncWorkspace` (covers `httpForceSync`, and is a no-op for the internal poller's identity-free + calls since `authorizeWorkspaceID`'s `callerScope` already treats no-identity as unscoped) + +### Wiring (`internal/backendapp/services.go`) +In `initWorkflowSyncService` (already receives `taskSvc *taskservice.Service` — currently used only +for `workflowSvc.SetSyncWorkflowOps(taskSvc)`), add immediately after `workflowsync.Provide` +succeeds, matching the azuredevops in-function style at services.go:609-612: +```go +svc.SetWorkspaceAuthorizer(taskSvc.AuthorizeWorkspaceAccess) +``` + +### Handler layer (`internal/workflowsync/handlers.go`) +Add a `workspaceDenied` helper (mirrors `internal/jira/handlers.go:18-22`) and use it in all four +handlers to return 404 instead of falling into the generic `internalError` 500 path: +```go +func workspaceDenied(err error) bool { + return errors.Is(err, repoerrors.ErrWorkspaceNotFound) +} +``` +`httpGetConfig`, `httpSetConfig`, `httpDeleteConfig`, `httpForceSync`: check +`workspaceDenied(err)` before the generic error branch and respond +`ctx.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"})` (no config/repo data in the +body). Import `github.com/kandev/kandev/internal/task/repository/repoerrors`. + +--- + +## Tests + +- **What:** foreign-workspace denial at the service layer for all four entry points. + **File:** `apps/backend/internal/workflowsync/service_authorization_test.go` (new). + **How:** unit test constructing `Service` directly (no HTTP), installing a fake + `SetWorkspaceAuthorizer` that asserts the identity/workspaceID it receives (pattern: + `internal/github/handlers_authorization_test.go`'s + `TestReviewWatchWebSocketDeniesForeignWorkspaceListAndMutations`), returning + `repoerrors.ErrWorkspaceNotFound`; assert each of `GetConfigForWorkspace`, + `SetConfigForWorkspace`, `DeleteConfigForWorkspace`, `SyncWorkspace` returns that sentinel and + performs no store mutation (re-read the config afterward to confirm it's unchanged). +- **What:** unscoped callers (nil authorizer, and an authorizer that itself no-ops on missing + identity) are unaffected. **File:** same new file. **How:** call the same four methods with no + authorizer wired and confirm they succeed as before (regression guard against a nil-safety + mistake in `authorizeWorkspaceAccess`). +- **What:** HTTP-level 404 + no-leak behavior end to end. + **File:** `apps/backend/internal/workflowsync/handlers_test.go` (new — this package currently has + zero handler-level tests). **How:** `gin.New()` + `RegisterRoutes`, `httptest.NewRequest` + + `authn.WithIdentity` on the request context (pattern: + `internal/github/controller_auth_identity_test.go`), covering: + - Owner identity succeeds on GET/POST/DELETE/sync against their own workspace. + - Foreign member identity gets 404 on all four routes against a seeded victim workspace's config, + and the response body never contains the victim's repo owner/name/branch/path. + - Victim's config is unchanged after the denied POST/DELETE attempts. + - Synthetic identity (auth disabled) still succeeds — no regression for the default (auth-off) + path. +- **Regression test that must fail before the fix and pass after:** the foreign-member GET/POST + cases above — today they succeed and leak/accept a foreign workspace's config; after the fix they + 404. + +Backend-only change; no frontend/E2E surface (no UI, WS, or client-visible contract changes). + +--- + +## Verification Results + +Both tasks done; see each task file's `## Results` for exact per-task output, including two +Codex-review-driven follow-up fixes (untested production wiring, force-sync's second error path) +and expanded test coverage. Whole-package summary: +- `go build ./...`: clean. +- `gofmt -l internal/workflowsync internal/backendapp`: no output (already formatted). +- `go vet ./internal/workflowsync/... ./internal/backendapp/...`: clean. +- `go test -race ./internal/workflowsync/... ./internal/backendapp/...`: both `ok`. +- `golangci-lint run ./internal/workflowsync/... ./internal/backendapp/... --new-from-rev=main --timeout=5m`: 0 issues. +- Full `go build ./... && go test -race ./...` (whole backend): all packages `ok` except a + pre-existing, unrelated flake in `internal/repoclone` + (`TestEnsureWorkspaceClonedWithBasicAuthKeepsCredentialScopedToGitChild/context_cancellation`, + a subprocess-timing test) — confirmed unrelated by re-running it in isolation (passes) and by this + branch touching neither that package nor anything it depends on. + +**Severity correction:** Codex review found that `origin/main` already has a global +`integrationWorkspaceScopeMiddleware` (`internal/backendapp/main.go:1807`) that path-matches +`/api/v1/workflow-sync/` and authorizes before any handler runs — the HTTP-level exploit originally +described in this spec's first draft was already blocked. See spec.md's "Correction" note; this fix +closes a real but narrower gap (service-layer defense-in-depth, consistency with every sibling +integration, protection for any future non-HTTP caller), not a currently-open HTTP vulnerability. + +--- + +## Implementation Waves And Parallel Candidates + +Sequential (2 tasks; task 2's handler tests depend on task 1's service-layer change): +- [x] [task-01-service-authorization](task-01-service-authorization.md) +- [x] [task-02-handler-error-mapping](task-02-handler-error-mapping.md) + +## Open Questions +(none) diff --git a/docs/plans/workflow-sync-workspace-authz/task-01-service-authorization.md b/docs/plans/workflow-sync-workspace-authz/task-01-service-authorization.md new file mode 100644 index 0000000000..5a2f3e6cfa --- /dev/null +++ b/docs/plans/workflow-sync-workspace-authz/task-01-service-authorization.md @@ -0,0 +1,66 @@ +--- +id: "01-service-authorization" +title: "Service-layer workspace authorization" +status: pending +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/workflow-sync-workspace-authz/spec.md" +--- + +# Task 01: Service-layer workspace authorization + +Add the `workspaceAuthorizer` boundary to `workflowsync.Service` and wire it at app boot, following +the exact pattern already used by `internal/github`, `internal/jira`, `internal/linear`, +`internal/slack`, `internal/azuredevops`, and `internal/automation`. + +## Changes + +1. `apps/backend/internal/workflowsync/service.go`: + - Add `workspaceAuthorizer func(context.Context, string) error` field to `Service`. + - Add `SetWorkspaceAuthorizer` (nil-safe on `s`) and `authorizeWorkspaceAccess` (nil-safe on `s` + and the field) methods — copy the shape from `internal/github/service.go:169-181`. + - Call `s.authorizeWorkspaceAccess(ctx, workspaceID)` first in `GetConfigForWorkspace`, + `SetConfigForWorkspace`, `DeleteConfigForWorkspace`, `SyncWorkspace`, returning the error + immediately (before touching `s.store` or `s.applier`). +2. `apps/backend/internal/backendapp/services.go`: in `initWorkflowSyncService`, after + `workflowsync.Provide` succeeds, call `svc.SetWorkspaceAuthorizer(taskSvc.AuthorizeWorkspaceAccess)` + before returning `svc`. + +## Acceptance + +- A caller whose identity does not own the target workspace gets `repoerrors.ErrWorkspaceNotFound` + from all four `Service` methods, and no store read/write or applier call happens for that call. +- A caller with no identity in context, or a synthetic identity, or a `Service` with no authorizer + wired, behaves exactly as before this change (fully unscoped, no denial). +- `go vet` / build clean; no new lint findings under the changed-file thresholds in + `apps/backend/.golangci.yml`. + +## Verification + +```bash +cd apps/backend && go test ./internal/workflowsync/... -run 'Authoriz|SyncWorkspace|ConfigForWorkspace' -v +cd apps/backend && go build ./... +cd apps/backend && golangci-lint run ./internal/workflowsync/... ./internal/backendapp/... --new-from-rev=main --timeout=5m +``` + +## Results + +- Status: **done**. +- `go test ./internal/workflowsync/... -run 'Authoriz|DeniesForeign|SucceedWhenNoAuthorizer|SucceedWhenAuthorizerAllows' -v`: 6/6 pass (`TestGetConfigForWorkspace_DeniesForeignWorkspace`, + `TestSetConfigForWorkspace_DeniesForeignWorkspaceAndLeavesConfigUnchanged`, + `TestDeleteConfigForWorkspace_DeniesForeignWorkspaceAndLeavesConfigInPlace`, + `TestSyncWorkspace_DeniesForeignWorkspaceAndNeverApplies`, + `TestServiceMethods_SucceedWhenNoAuthorizerWired`, `TestServiceMethods_SucceedWhenAuthorizerAllows`). +- `go build ./...`: clean. +- `go test -race ./internal/workflowsync/... ./internal/backendapp/...`: both `ok`. +- `golangci-lint run ./internal/workflowsync/... ./internal/backendapp/... --new-from-rev=main --timeout=5m`: 0 issues. +- **Codex review finding (blocker, fixed):** the initial service-layer tests only ever installed a + hand-written fake authorizer, so a regression removing the real + `svc.SetWorkspaceAuthorizer(taskSvc.AuthorizeWorkspaceAccess)` wiring line in + `initWorkflowSyncService` would leave every test green. Added + `apps/backend/internal/backendapp/workflowsync_wiring_test.go` + (`TestInitWorkflowSyncService_WiresRealWorkspaceAuthorization`), which calls + `initWorkflowSyncService` itself with a real `taskservice.Service` backed by SQLite. Verified by + temporarily commenting out the wiring line: the new test failed for the expected reason; restoring + the line made it pass again (confirmed via `go build`/full suite afterward). diff --git a/docs/plans/workflow-sync-workspace-authz/task-02-handler-error-mapping.md b/docs/plans/workflow-sync-workspace-authz/task-02-handler-error-mapping.md new file mode 100644 index 0000000000..bf4245e799 --- /dev/null +++ b/docs/plans/workflow-sync-workspace-authz/task-02-handler-error-mapping.md @@ -0,0 +1,75 @@ +--- +id: "02-handler-error-mapping" +title: "Handler error mapping + HTTP regression tests" +status: pending +wave: 1 +depends_on: ["01-service-authorization"] +plan: "plan.md" +spec: "../../specs/workflow-sync-workspace-authz/spec.md" +--- + +# Task 02: Handler error mapping + HTTP regression tests + +Map the new `ErrWorkspaceNotFound` denial to a sanitized 404 in the HTTP layer, and add the +HTTP-level regression coverage this package currently lacks entirely (`handlers_test.go` does not +exist yet). + +## Changes + +1. `apps/backend/internal/workflowsync/handlers.go`: + - Import `errors` (already imported) and + `github.com/kandev/kandev/internal/task/repository/repoerrors`. + - Add `func workspaceDenied(err error) bool { return errors.Is(err, repoerrors.ErrWorkspaceNotFound) }` + (mirror `internal/jira/handlers.go:18-22`). + - In `httpGetConfig`, `httpSetConfig`, `httpDeleteConfig`, `httpForceSync`: check + `workspaceDenied(err)` before the existing generic-error branch and respond + `ctx.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"})`. Do not include the + underlying service error or any config fields in this response. +2. New file `apps/backend/internal/workflowsync/handlers_test.go` — HTTP-level tests using + `gin.New()` + `RegisterRoutes` + `httptest.NewRequest` + `authn.WithIdentity` on the request + context (pattern: `internal/github/controller_auth_identity_test.go`). + +## Acceptance (from spec Scenarios) + +- Owner identity: GET/POST/DELETE/sync all succeed against their own workspace, unchanged from + pre-fix behavior. +- Foreign member identity against a seeded victim workspace's config: all four routes return 404, + and the response body contains none of the victim's repo owner/name/branch/path/project_path. +- The victim's config is byte-for-byte unchanged after the denied POST/DELETE attempts (re-read via + the service and compare). +- Synthetic identity (auth disabled) succeeds on all four routes — no regression for the default + path. +- The two "foreign member GET/POST" cases are the regression tests: written to fail against + pre-task-01 code (they'd get 200 + leaked/accepted data) and pass after task 01 + this task. + +## Verification + +```bash +cd apps/backend && go test ./internal/workflowsync/... -run 'TestHTTP|TestConfig|TestForceSync' -v +cd apps/backend && go test ./internal/workflowsync/... -v +cd apps/backend && golangci-lint run ./internal/workflowsync/... --new-from-rev=main --timeout=5m +cd apps/backend && go test -race ./internal/workflowsync/... +``` + +## Results + +- Status: **done**, including two follow-up fixes from an independent Codex review round. +- `go test ./internal/workflowsync/... -run 'TestHTTPHandlers' -v`: all pass + (`TestHTTPHandlers_OwnerSucceedsOnAllRoutes`, `TestHTTPHandlers_SyntheticIdentitySucceedsOnAllRoutes`, + `TestHTTPHandlers_ForeignMemberDeniedOnAllRoutesWithoutLeaking`, each table-driven across + get/post/delete/sync, plus `TestHTTPHandlers_ForceSyncDeniesSecondLookupAfterAccessRevoked`). +- Regression confirmed: before the task-01 service change, the get/post/delete/sync subtests failed + with 500s and leaked/accepted the foreign workspace's config (verified during TDD). +- **Codex review finding (blocker, fixed):** `httpForceSync`'s second call + (`GetConfigForWorkspace`, used to build the response after a successful sync) did not map + `ErrWorkspaceNotFound` to 404 — a race where access is revoked between the sync and the follow-up + read fell through to a generic 500. Fixed with the same `workspaceDenied` check; covered by + `TestHTTPHandlers_ForceSyncDeniesSecondLookupAfterAccessRevoked` (verified to fail pre-fix, pass + post-fix). +- **Codex review finding (suggestion, fixed):** owner/synthetic-identity coverage was GET-only and + the leak/mutation assertions didn't cover branch/path or the full config. Rewritten as + table-driven tests across all four operations (`TestHTTPHandlers_OwnerSucceedsOnAllRoutes`, + `TestHTTPHandlers_SyntheticIdentitySucceedsOnAllRoutes`), with distinctive branch/path fixture + values and a full-struct `before`/`after` config comparison in the denial test. +- `go test -race ./internal/workflowsync/... ./internal/backendapp/...`: both `ok`. +- `golangci-lint run ./internal/workflowsync/... --new-from-rev=main --timeout=5m`: 0 issues. diff --git a/docs/specs/workflow-sync-workspace-authz/spec.md b/docs/specs/workflow-sync-workspace-authz/spec.md new file mode 100644 index 0000000000..5f8dedddc3 --- /dev/null +++ b/docs/specs/workflow-sync-workspace-authz/spec.md @@ -0,0 +1,96 @@ +--- +status: building +created: 2026-08-06 +owner: nova28 +--- + +# Workflow Sync — Per-User Workspace Authorization + +## Why +Every other per-workspace integration service (GitHub, Jira, Linear, Slack, Azure DevOps, +Automation) enforces that a caller-supplied `workspace_id` belongs to the requesting user *inside +the service itself* — `SetWorkspaceAuthorizer` + a check at the top of every user-facing method — +in addition to the global `integrationWorkspaceScopeMiddleware` +(`internal/backendapp/main.go`/`helpers.go`) that already gates their HTTP routes by path prefix. +`internal/workflowsync` is the one integration service that never adopted the service-layer half of +that pattern: its `Service` methods take a caller-supplied `workspace_id` straight from the caller +with no ownership check of their own. + +**Correction (found during review):** an earlier draft of this spec claimed the HTTP endpoints +(`/api/v1/workflow-sync/config`, `/api/v1/workflow-sync/sync`) had no ownership check at all and were +directly exploitable by an authenticated member. That was wrong — those paths are already listed in +`integrationWorkspacePrefixes` and covered by the global middleware, which authorizes before any +workflow-sync handler runs, exactly like `/api/v1/jira/`, `/api/v1/github/`, etc. The actual gap this +spec closes is narrower: `workflowsync.Service` has no authorization boundary of its own, unlike +every sibling integration service, so (a) it is the one integration whose safety depends entirely on +the route-level middleware never being bypassed, misconfigured, or missing this path prefix, with no +independent second layer, and (b) any future non-HTTP caller of the service (a WS handler, an MCP +tool, a plugin, a different mount point) would inherit no protection at all, unlike every sibling +service where the constructor-level check still applies regardless of how the call arrived. This is +a defense-in-depth and consistency fix, not a closure of a currently open HTTP exploit. + +## What +- The workflow-sync config read endpoint (`GET /api/v1/workflow-sync/config`) SHALL deny a request + for a `workspace_id` the caller does not own. +- The workflow-sync config write endpoint (`POST /api/v1/workflow-sync/config`) SHALL deny writing a + config for a `workspace_id` the caller does not own. +- The workflow-sync config delete endpoint (`DELETE /api/v1/workflow-sync/config`) SHALL deny + deleting a config for a `workspace_id` the caller does not own. +- The force-sync endpoint (`POST /api/v1/workflow-sync/sync`) SHALL deny triggering a sync for a + `workspace_id` the caller does not own. +- A denied request SHALL be indistinguishable from a request against a nonexistent workspace (404, + no existence leak) — consistent with every other per-user-scoped integration in this codebase. +- Requests from an **unscoped caller** — no identity in context (internal callers: the periodic sync + poller, the event bus) or a synthetic identity (auth disabled) — SHALL continue to work exactly as + before this fix; this feature only closes the gap for caller-supplied `workspace_id` values + reaching these endpoints from an authenticated HTTP request. +- Workspaces with no owner (`owner_id = ''`, pre-auth rows not yet claimed by the setup wizard) + SHALL remain visible to any authenticated member, matching the existing workspace-visibility rule + used everywhere else. + +## Permissions +Reuses the existing per-user workspace scoping rule documented in +`apps/backend/AGENTS.md` ("Opt-in authentication & per-user scoping"): +- No identity in context, or a synthetic identity → unscoped (today's pre-auth behavior). +- Real identity → the workspace is visible only if its `owner_id` is empty or matches the caller. +- Denial uses the `ErrWorkspaceNotFound` sentinel — the same value the task service, GitHub, Jira, + Linear, Slack, and Azure DevOps services already return for this class of denial — so a foreign + workspace and a missing workspace produce the same observable response. + +This spec does not introduce a new permission concept; it applies the existing one to a service that +missed it. + +## Failure modes +- **Caller lacks access to the target workspace:** the request is denied with the sanitized + not-found response before the config store (or the sync/apply pipeline) is touched. No config + contents, repo identity, or sync status for the foreign workspace are ever included in the denial. +- **Workspace authorizer not wired** (e.g. a unit test constructing `workflowsync.Service` directly + without calling `SetWorkspaceAuthorizer`): the service behaves as unscoped, matching every other + integration service's nil-safe default — this preserves existing tests and call sites that don't + go through the full app-boot wiring. + +## Scenarios +- **GIVEN** two workspaces A (owned by user `alice`) and B (owned by user `bob`) each with their own + workflow-sync config, **WHEN** `bob` calls `GET /api/v1/workflow-sync/config?workspace_id=`, + **THEN** the response is 404 and contains no data about workspace A's config. +- **GIVEN** the same setup, **WHEN** `bob` calls + `POST /api/v1/workflow-sync/config?workspace_id=` with an attacker-controlled repo payload, + **THEN** the request is denied (404) and workspace A's stored config is unchanged. +- **GIVEN** the same setup, **WHEN** `bob` calls `DELETE /api/v1/workflow-sync/config?workspace_id=` + or `POST /api/v1/workflow-sync/sync?workspace_id=`, **THEN** both are denied (404) and workspace + A's config/sync state is unaffected. +- **GIVEN** workspace A owned by `alice`, **WHEN** `alice` calls any of the four endpoints against her + own workspace, **THEN** the request succeeds exactly as before this fix. +- **GIVEN** `features.auth` is disabled (synthetic identity) or the caller is an internal caller (no + identity in context, e.g. the periodic sync poller), **WHEN** any workflow-sync entry point runs, + **THEN** it is unaffected — no denial, same behavior as before this fix. + +## Out of scope +- Changing the periodic poller (`SyncDueConfigs`/`SyncWorkspace` invoked internally) to carry or + require an identity — it is a correct internal caller today and stays unscoped by the existing + `callerScope` rule. +- Any change to GitHub or GitLab repository-level authorization (App installation scope, PAT scope) + — this spec is strictly about the `workspace_id` ownership boundary on the workflow-sync HTTP + surface. +- Adding workspace authorization to any other integration package — all of them already have it. +- New data model or schema changes — no persistent state changes.