dreampiler/kandev
1
0
Fork 0
Code Issues 43 Pull Requests Actions 6 Packages Projects Releases Wiki Activity

fix(auth): deny a workflow whose workspace no longer exists (#3040)

Browse Source 
Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
João Salavisa
2026-08-25 19:53:22 +01:00
committed by GitHub
parent e11768cbea
commit d5e4aed75e
2 changed files with 41 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files
apps/backend/internal/task/service/service_access.go
+14 -6
View File
@@ -93,16 +93,24 @@ func (s *Service) authorizeWorkflowID(ctx context.Context, workflowID string) er
if workflow.WorkspaceID == "" {
return nil
}
// A workflow whose workspace cannot be resolved has no owner to check
// against, so neither outcome below is "authorized". This used to be one
// `return nil` covering both, which handed any workflow ID that survived
// a failed lookup to whoever guessed it.
//
// The pre-auth unowned row is not this case: workspace_id == "" is
// answered above and stays visible to everyone. Here the workflow names a
// workspace that is not there — `workflows.workspace_id` carries no
// foreign key, so a deleted workspace can leave one behind — and an
// orphan belongs to nobody, which under per-user scoping means nobody
// sees it.
workspace, err := s.workspaces.GetWorkspace(ctx, workflow.WorkspaceID)
switch {
case errors.Is(err, repoerrors.ErrWorkspaceNotFound):
// A dangling workspace reference should not hide the workflow from the
// single user who can already see everything else about it.
return nil
return repoerrors.ErrWorkspaceNotFound
case err != nil:
// Anything else is a failed lookup, not an answer. Treating it as
// "authorized" would let a transient database error hand a guessed
// workflow ID to whoever asked, so it fails closed by propagating.
// A failed lookup is not an answer at all: propagate it rather than
// letting a transient database error read as either allow or deny.
return err
}
if !workspaceVisibleTo(workspace, userID) {
apps/backend/internal/task/service/service_access_test.go
+27 -4
View File
@@ -211,6 +211,17 @@ func TestAuthorizeWorkflowAccess(t *testing.T) {
if err := svc.AuthorizeWorkflowAccess(ctxAs("user-a"), "wf-legacy"); err != nil {
t.Fatalf("legacy workflow access: %v", err)
}
// So does a workflow carrying no workspace at all (schema default, never
// backfilled). This is the pre-auth row the orphan branch above must not
// swallow: it names no owner rather than naming a missing one.
if err := repo.CreateWorkflow(context.Background(), &models.Workflow{
ID: "wf-no-workspace", Name: "Pre-auth, workspaceless",
}); err != nil {
t.Fatalf("create workspaceless workflow: %v", err)
}
if err := svc.AuthorizeWorkflowAccess(ctxAs("user-a"), "wf-no-workspace"); err != nil {
t.Fatalf("workspaceless workflow access: %v", err)
}
// A failed workspace lookup is not an answer. The dangling-reference
// fallback below it returns nil, so a transient database error used to
// read as "authorized" and hand a guessed workflow ID to whoever asked.
@@ -219,11 +230,23 @@ func TestAuthorizeWorkflowAccess(t *testing.T) {
if err := svc.AuthorizeWorkflowAccess(ctxAs("user-a"), "wf-b"); !errors.Is(err, broken) {
t.Fatalf("failed workspace lookup: %v, want the lookup error", err)
}
// A workspace row that is genuinely gone keeps the documented fallback:
// the workflow stays visible rather than disappearing from its owner.
// A workspace row that is genuinely gone is a different answer from a
// failed lookup, and it is still not "authorized": the workflow names an
// owner that does not exist, so nobody sees it. (An unowned pre-auth row
// is the workspace_id == "" case below, not this one.)
svc.workspaces = &brokenWorkspaceReader{WorkspaceRepository: repo, err: repoerrors.ErrWorkspaceNotFound}
if err := svc.AuthorizeWorkflowAccess(ctxAs("user-a"), "wf-b"); err != nil {
t.Fatalf("dangling workspace reference: %v, want the visibility fallback", err)
if err := svc.AuthorizeWorkflowAccess(ctxAs("user-a"), "wf-b"); !errors.Is(err, repoerrors.ErrWorkspaceNotFound) {
t.Fatalf("orphaned workflow: %v, want the not-found denial", err)
}
if err := svc.AuthorizeWorkflowAccess(ctxAs("user-b"), "wf-b"); !errors.Is(err, repoerrors.ErrWorkspaceNotFound) {
t.Fatalf("orphaned workflow for its former owner: %v, want the not-found denial", err)
}
// Unscoped callers are unaffected: they never reach the lookup.
if err := svc.AuthorizeWorkflowAccess(context.Background(), "wf-b"); err != nil {
t.Fatalf("internal caller hit the orphan branch: %v", err)
}
if err := svc.AuthorizeWorkflowAccess(ctxSynthetic(), "wf-b"); err != nil {
t.Fatalf("synthetic caller hit the orphan branch: %v", err)
}
svc.workspaces = repo