From 4a9f0ad030d49738c3815a582b2b0d49f10f37d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Tue, 25 Aug 2026 17:43:34 +0100 Subject: [PATCH] fix(workflow): accept unresolvable move_to_step targets again (#3046) Co-authored-by: Kandev Agent Co-authored-by: Claude Opus 5 (1M context) --- .../workflow/controller/controller.go | 22 +++++++-- .../handlers/step_reference_scope_test.go | 47 +++++++++++++++---- 2 files changed, 58 insertions(+), 11 deletions(-) diff --git a/apps/backend/internal/workflow/controller/controller.go b/apps/backend/internal/workflow/controller/controller.go index 6d3716396a..eb097abd6f 100644 --- a/apps/backend/internal/workflow/controller/controller.go +++ b/apps/backend/internal/workflow/controller/controller.go @@ -317,17 +317,33 @@ func (c *Controller) validateStepReferences(ctx context.Context, step *models.Wo // with whatever it finds there — that step's prompt and agent profile — so a // target in somebody else's workflow both parks the task outside its board and // runs it under their configuration. +// +// A target that resolves to nothing is left alone, because reaching nothing is +// the whole point: `move_to_step` configs are authored with symbolic IDs +// ("review", "in-progress") that only the template applier remaps, and the +// engine skips an unresolvable target rather than failing the trigger. That +// tolerance is load-bearing product behaviour, not an oversight, so rejecting +// it here broke ordinary step edits that had nothing to do with scoping. +// +// It is also safe: an unresolvable ID reaches no other user's step, and it +// cannot be armed later, since step IDs are generated server-side on every +// write path (create, template apply, import, sync) and never accepted from a +// caller. func (c *Controller) validateEventStepTarget(ctx context.Context, step *models.WorkflowStep, targetID string) error { if targetID == step.ID { return nil // a self-transition names no other step } - if err := c.svc.AuthorizeStep(ctx, targetID); err != nil { - return err - } target, err := c.svc.GetStep(ctx, targetID) if err != nil { + if service.IsNotFound(err) { + return nil + } return fmt.Errorf("move_to_step target is invalid: %w", err) } + // Membership carries the authorization: the step being written lives in an + // already-authorized workflow, so a target inside that same workflow is + // authorized by construction, and one outside it is refused whether or not + // the caller can see it. if target.WorkflowID != step.WorkflowID { return fmt.Errorf("move_to_step must reference a step in the same workflow") } diff --git a/apps/backend/internal/workflow/handlers/step_reference_scope_test.go b/apps/backend/internal/workflow/handlers/step_reference_scope_test.go index 8ca7eb1583..108690ea72 100644 --- a/apps/backend/internal/workflow/handlers/step_reference_scope_test.go +++ b/apps/backend/internal/workflow/handlers/step_reference_scope_test.go @@ -58,7 +58,10 @@ func TestMoveToStepTargetCannotLeaveTheWorkflow(t *testing.T) { h.queries.reset() rec := doAs(t, h, asUser(userB), http.MethodPost, "/api/v1/workflow/steps", moveToStepBody("wf-b", "Smuggler", h.stepA)) - requireNotFound(t, rec) + requireStatus(t, rec, http.StatusBadRequest) + if !strings.Contains(rec.Body.String(), "same workflow") { + t.Fatalf("body = %s, want the same-workflow reason", rec.Body.String()) + } requireNoStepWrites(t, h) }) @@ -71,19 +74,47 @@ func TestMoveToStepTargetCannotLeaveTheWorkflow(t *testing.T) { {"type": "move_to_step", "config": map[string]any{"step_id": h.stepA}}, }, }}) - requireNotFound(t, rec) + requireStatus(t, rec, http.StatusBadRequest) requireNoStepWrites(t, h) }) - t.Run("a foreign target answers exactly like a nonexistent one", func(t *testing.T) { + // A target that resolves to nothing is accepted, and a target that resolves + // into another workflow is not. Those two answers differ on purpose. + // + // The alternative — one reply for both — is what shipped in #3031, and it + // broke ordinary step edits: `move_to_step` configs are routinely authored + // with symbolic IDs ("review") that only the template applier remaps, and + // the engine skips an unresolvable target instead of failing the trigger. + // Refusing those rejected a documented, load-bearing behaviour. + // + // The cost is that a caller holding a step UUID can learn it belongs to + // some other workflow. That is a UUID they already had, it stays refused + // either way, and no ID they supply can ever become valid later: step IDs + // are generated server-side on every write path. + t.Run("an unresolvable target is accepted, a foreign one is not", func(t *testing.T) { h := setupScopedRouter(t) + symbolic := doAs(t, h, asUser(userB), http.MethodPost, "/api/v1/workflow/steps", + moveToStepBody("wf-b", "Symbolic", "review")) + requireStatus(t, symbolic, http.StatusCreated) + foreign := doAs(t, h, asUser(userB), http.MethodPost, "/api/v1/workflow/steps", moveToStepBody("wf-b", "Smuggler", h.stepA)) - missing := doAs(t, h, asUser(userB), http.MethodPost, "/api/v1/workflow/steps", - moveToStepBody("wf-b", "Smuggler", "no-such-step")) - if foreign.Code != missing.Code || foreign.Body.String() != missing.Body.String() { - t.Fatalf("foreign target %d %s differs from missing target %d %s", - foreign.Code, foreign.Body.String(), missing.Code, missing.Body.String()) + requireStatus(t, foreign, http.StatusBadRequest) + }) + + // The e2e suite runs with authentication disabled and edits steps exactly + // this way (workflow-automation.spec.ts). Pinning it here means the next + // change to this validator fails in a Go test in seconds rather than in a + // browser shard an hour later. + t.Run("a symbolic target is accepted with auth disabled", func(t *testing.T) { + h := setupScopedRouter(t) + for _, ctx := range []context.Context{asSyntheticUser(), context.Background()} { + rec := doAs(t, h, ctx, http.MethodPut, "/api/v1/workflow/steps/"+h.stepB, + map[string]any{"events": map[string]any{ + "on_turn_start": []map[string]any{{"type": "move_to_next"}}, + "on_turn_complete": []map[string]any{{"type": "move_to_step", "config": map[string]any{"step_id": "review"}}}, + }}) + requireStatus(t, rec, http.StatusOK) } })