fix(backend): enforce per-user workspace authorization in workflow sync (#2311)
This commit is contained in:
@@ -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
|
||||
}
|
||||
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
}
|
||||
@@ -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()
|
||||
|
||||
@@ -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"))
|
||||
}
|
||||
@@ -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)
|
||||
@@ -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).
|
||||
@@ -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.
|
||||
@@ -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=<A>`,
|
||||
**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=<A>` 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=<A>`
|
||||
or `POST /api/v1/workflow-sync/sync?workspace_id=<A>`, **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.
|
||||
Reference in New Issue
Block a user