From 5e4fc9c1dd69ceb42eea67e3d39848351814f67a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Sun, 31 May 2026 11:47:25 +0100 Subject: [PATCH] fix(workflow): exclude office workflows from settings export (#1184) --- .../internal/office/testharness/routes.go | 42 +++++++++++++++ .../office/testharness/routes_test.go | 43 +++++++++++++++ .../workflow/controller/controller.go | 7 +-- .../internal/workflow/handlers/handlers.go | 20 ++++++- .../internal/workflow/service/service.go | 36 +++++++++++-- .../internal/workflow/service/service_test.go | 52 ++++++++++++++++++- apps/web/app/actions/workspaces.test.ts | 39 +++++++++++++- apps/web/app/actions/workspaces.ts | 18 ++++++- .../workspace/workspace-workflows-client.tsx | 18 +++++-- apps/web/e2e/helpers/api-client.ts | 19 +++++++ .../workflow/workflow-import-export.spec.ts | 24 +++++++++ 11 files changed, 304 insertions(+), 14 deletions(-) diff --git a/apps/backend/internal/office/testharness/routes.go b/apps/backend/internal/office/testharness/routes.go index fd4db186c8..3e12ff2614 100644 --- a/apps/backend/internal/office/testharness/routes.go +++ b/apps/backend/internal/office/testharness/routes.go @@ -76,6 +76,7 @@ func RegisterRoutes( }) g.POST("/task-sessions", seedTaskSessionHandler(repo, eventBus, log)) g.POST("/messages", seedMessageHandler(repo, eventBus, log)) + g.POST("/workflows", seedWorkflowHandler(repo, log)) if officeRepo != nil { g.POST("/comments", seedCommentHandler(officeRepo, eventBus, log)) g.POST("/agent-failures", seedAgentFailureHandler(officeRepo, eventBus, log)) @@ -162,6 +163,47 @@ func seedTaskSessionHandler(repo *sqliterepo.Repository, eventBus bus.EventBus, } } +type seedWorkflowRequest struct { + WorkspaceID string `json:"workspace_id"` + Name string `json:"name"` + // Style is persisted as-is (kanban / office / custom). Production has no + // HTTP path that creates an office-style workflow — the normal create + // endpoint always normalises to kanban — so this seed route lets the + // Playwright suite stand up an office workflow to exercise the + // "exclude office from settings export" behaviour (issue #1109). + Style string `json:"style"` +} + +type seedWorkflowResponse struct { + WorkflowID string `json:"workflow_id"` +} + +func seedWorkflowHandler(repo *sqliterepo.Repository, log *logger.Logger) gin.HandlerFunc { + return func(c *gin.Context) { + var req seedWorkflowRequest + if err := c.ShouldBindJSON(&req); err != nil { + c.JSON(http.StatusBadRequest, gin.H{"error": "invalid JSON: " + err.Error()}) + return + } + if req.WorkspaceID == "" || req.Name == "" { + c.JSON(http.StatusBadRequest, gin.H{"error": "workspace_id and name are required"}) + return + } + workflow := &models.Workflow{ + ID: uuid.New().String(), + WorkspaceID: req.WorkspaceID, + Name: req.Name, + Style: req.Style, + } + if err := repo.CreateWorkflow(c.Request.Context(), workflow); err != nil { + log.Error("test harness: create workflow failed", zap.Error(err)) + c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + return + } + c.JSON(http.StatusOK, seedWorkflowResponse{WorkflowID: workflow.ID}) + } +} + func validateSeedTaskSessionRequest(req *seedTaskSessionRequest) error { if req.TaskID == "" { return errBadField("task_id is required") diff --git a/apps/backend/internal/office/testharness/routes_test.go b/apps/backend/internal/office/testharness/routes_test.go index 010db4f84c..0eda3d43fb 100644 --- a/apps/backend/internal/office/testharness/routes_test.go +++ b/apps/backend/internal/office/testharness/routes_test.go @@ -291,6 +291,49 @@ func TestSeedMessageRejectsUnknownSession(t *testing.T) { } } +func TestSeedWorkflowPersistsStyle(t *testing.T) { + repo, _ := newTestRepo(t) + r := newRouter(t, repo, nil) + + body := mustJSON(t, map[string]interface{}{ + "workspace_id": "ws-1", + "name": "Office Only Workflow", + "style": models.WorkflowStyleOffice, + }) + w := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/api/v1/_test/workflows", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + r.ServeHTTP(w, req) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + + workflows, err := repo.ListWorkflows(context.Background(), "ws-1", true) + if err != nil { + t.Fatalf("list workflows: %v", err) + } + if len(workflows) != 1 { + t.Fatalf("expected 1 workflow, got %d", len(workflows)) + } + if got := workflows[0].Style; got != models.WorkflowStyleOffice { + t.Fatalf("expected style %q, got %q", models.WorkflowStyleOffice, got) + } +} + +func TestSeedWorkflowRejectsMissingFields(t *testing.T) { + repo, _ := newTestRepo(t) + r := newRouter(t, repo, nil) + + body := mustJSON(t, map[string]interface{}{"style": "office"}) + w := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/api/v1/_test/workflows", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + r.ServeHTTP(w, req) + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400 for missing workspace_id/name, got %d", w.Code) + } +} + func mustJSON(t *testing.T, v interface{}) []byte { t.Helper() b, err := json.Marshal(v) diff --git a/apps/backend/internal/workflow/controller/controller.go b/apps/backend/internal/workflow/controller/controller.go index 603b9c8a86..dcf14081cb 100644 --- a/apps/backend/internal/workflow/controller/controller.go +++ b/apps/backend/internal/workflow/controller/controller.go @@ -236,9 +236,10 @@ func (c *Controller) ExportWorkflow(ctx context.Context, workflowID string) (*mo return c.svc.ExportWorkflow(ctx, workflowID) } -// ExportWorkflows exports all workflows for a workspace. -func (c *Controller) ExportWorkflows(ctx context.Context, workspaceID string) (*models.WorkflowExport, error) { - return c.svc.ExportWorkflows(ctx, workspaceID) +// ExportWorkflows exports workflows for a workspace. A nil workflowIDs exports +// every workflow; a non-nil slice restricts the export to that set of IDs. +func (c *Controller) ExportWorkflows(ctx context.Context, workspaceID string, workflowIDs []string) (*models.WorkflowExport, error) { + return c.svc.ExportWorkflows(ctx, workspaceID, workflowIDs) } // ImportWorkflows imports workflows into a workspace. diff --git a/apps/backend/internal/workflow/handlers/handlers.go b/apps/backend/internal/workflow/handlers/handlers.go index f4b2dc0ec9..10977af926 100644 --- a/apps/backend/internal/workflow/handlers/handlers.go +++ b/apps/backend/internal/workflow/handlers/handlers.go @@ -4,6 +4,7 @@ import ( "context" "io" "net/http" + "strings" "github.com/gin-gonic/gin" "go.uber.org/zap" @@ -240,7 +241,7 @@ func (h *Handlers) httpExportWorkflow(c *gin.Context) { } func (h *Handlers) httpExportWorkflows(c *gin.Context) { - resp, err := h.controller.ExportWorkflows(c.Request.Context(), c.Param("id")) + resp, err := h.controller.ExportWorkflows(c.Request.Context(), c.Param("id"), parseExportIDs(c)) if err != nil { h.logger.Error("failed to export workflows", zap.Error(err)) c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to export workflows"}) @@ -249,6 +250,23 @@ func (h *Handlers) httpExportWorkflows(c *gin.Context) { h.respondYAML(c, resp) } +// parseExportIDs reads the optional comma-separated `ids` query param. It +// returns nil when the param is absent (export all, back-compat) and a non-nil +// slice (possibly empty) when present, restricting the export to those IDs. +func parseExportIDs(c *gin.Context) []string { + raw, ok := c.GetQuery("ids") + if !ok { + return nil + } + ids := []string{} + for _, part := range strings.Split(raw, ",") { + if trimmed := strings.TrimSpace(part); trimmed != "" { + ids = append(ids, trimmed) + } + } + return ids +} + func (h *Handlers) httpImportWorkflows(c *gin.Context) { const maxImportSize = 1 << 20 // 1 MB body, err := io.ReadAll(io.LimitReader(c.Request.Body, maxImportSize)) diff --git a/apps/backend/internal/workflow/service/service.go b/apps/backend/internal/workflow/service/service.go index 0ccde832b5..920b7ade07 100644 --- a/apps/backend/internal/workflow/service/service.go +++ b/apps/backend/internal/workflow/service/service.go @@ -412,12 +412,26 @@ func (s *Service) ExportWorkflow(ctx context.Context, workflowID string) (*model return models.BuildWorkflowExport([]*taskmodels.Workflow{wf}, stepMap, s.resolveProfile), nil } -// ExportWorkflows exports all workflows for a workspace. -func (s *Service) ExportWorkflows(ctx context.Context, workspaceID string) (*models.WorkflowExport, error) { - workflows, err := s.workflowProvider.ListWorkflows(ctx, workspaceID, true) +// ExportWorkflows exports workflows for a workspace. When workflowIDs is nil, +// every (non-hidden) workflow is exported (back-compat). When workflowIDs is +// non-nil, only workflows whose ID is in the set are exported — an empty set +// exports nothing. Filtering is by ID membership only: the backend MUST NOT +// branch on a workflow's style (see internal/workflow/models/phase2.go), so the +// frontend decides which workflows (e.g. kanban-only) to include and passes +// their IDs. +// +// Hidden/system workflows (e.g. improve-kandev) are only listed when the caller +// passes explicit IDs; the back-compat "export everything" path leaves them out +// so a bare GET of the export endpoint can't leak system flows. +func (s *Service) ExportWorkflows(ctx context.Context, workspaceID string, workflowIDs []string) (*models.WorkflowExport, error) { + includeHidden := workflowIDs != nil + workflows, err := s.workflowProvider.ListWorkflows(ctx, workspaceID, includeHidden) if err != nil { return nil, fmt.Errorf("failed to list workflows: %w", err) } + if workflowIDs != nil { + workflows = filterWorkflowsByID(workflows, workflowIDs) + } stepMap := make(map[string][]*models.WorkflowStep, len(workflows)) for _, wf := range workflows { steps, stepsErr := s.repo.ListStepsByWorkflow(ctx, wf.ID) @@ -429,6 +443,22 @@ func (s *Service) ExportWorkflows(ctx context.Context, workspaceID string) (*mod return models.BuildWorkflowExport(workflows, stepMap, s.resolveProfile), nil } +// filterWorkflowsByID returns the subset of workflows whose ID is in ids, +// preserving the input order. +func filterWorkflowsByID(workflows []*taskmodels.Workflow, ids []string) []*taskmodels.Workflow { + want := make(map[string]bool, len(ids)) + for _, id := range ids { + want[id] = true + } + filtered := make([]*taskmodels.Workflow, 0, len(workflows)) + for _, wf := range workflows { + if want[wf.ID] { + filtered = append(filtered, wf) + } + } + return filtered +} + // ImportWorkflows imports workflows into a workspace. Deduplicates by name. func (s *Service) ImportWorkflows(ctx context.Context, workspaceID string, export *models.WorkflowExport) (*ImportResult, error) { if err := export.Validate(); err != nil { diff --git a/apps/backend/internal/workflow/service/service_test.go b/apps/backend/internal/workflow/service/service_test.go index 587524222a..1398237d98 100644 --- a/apps/backend/internal/workflow/service/service_test.go +++ b/apps/backend/internal/workflow/service/service_test.go @@ -492,7 +492,7 @@ func TestExportWorkflows(t *testing.T) { createStep(t, svc, &models.WorkflowStep{WorkflowID: "wf-1", Name: "Step1", Position: 0}) createStep(t, svc, &models.WorkflowStep{WorkflowID: "wf-2", Name: "Step2", Position: 0}) - export, err := svc.ExportWorkflows(ctx, "ws-1") + export, err := svc.ExportWorkflows(ctx, "ws-1", nil) require.NoError(t, err) require.Len(t, export.Workflows, 2) @@ -500,6 +500,56 @@ func TestExportWorkflows(t *testing.T) { assert.Contains(t, names, "Alpha") assert.Contains(t, names, "Beta") }) + + t.Run("filters to the requested workflow IDs", func(t *testing.T) { + svc, _, mock := setupTestServiceWithProvider(t) + ctx := context.Background() + + mock.addWorkflow("wf-1", "ws-1", "Alpha") + mock.addWorkflow("wf-2", "ws-1", "Beta") + mock.addWorkflow("wf-3", "ws-1", "Gamma") + + export, err := svc.ExportWorkflows(ctx, "ws-1", []string{"wf-1", "wf-3"}) + require.NoError(t, err) + require.Len(t, export.Workflows, 2) + + names := []string{export.Workflows[0].Name, export.Workflows[1].Name} + assert.Contains(t, names, "Alpha") + assert.Contains(t, names, "Gamma") + assert.NotContains(t, names, "Beta") + }) + + t.Run("empty (non-nil) ID list exports nothing", func(t *testing.T) { + svc, _, mock := setupTestServiceWithProvider(t) + ctx := context.Background() + + mock.addWorkflow("wf-1", "ws-1", "Alpha") + + export, err := svc.ExportWorkflows(ctx, "ws-1", []string{}) + require.NoError(t, err) + assert.Empty(t, export.Workflows) + }) + + t.Run("nil ID list omits hidden workflows but explicit IDs include them", func(t *testing.T) { + svc, _, mock := setupTestServiceWithProvider(t) + ctx := context.Background() + + mock.addWorkflow("wf-1", "ws-1", "Alpha") + mock.addWorkflow("hidden-1", "ws-1", "System Flow") + mock.workflows[1].Hidden = true + + // Back-compat path (nil) must not leak the hidden system workflow. + all, err := svc.ExportWorkflows(ctx, "ws-1", nil) + require.NoError(t, err) + require.Len(t, all.Workflows, 1) + assert.Equal(t, "Alpha", all.Workflows[0].Name) + + // Explicitly requesting a hidden workflow's ID still exports it. + explicit, err := svc.ExportWorkflows(ctx, "ws-1", []string{"hidden-1"}) + require.NoError(t, err) + require.Len(t, explicit.Workflows, 1) + assert.Equal(t, "System Flow", explicit.Workflows[0].Name) + }) } func TestImportWorkflows(t *testing.T) { diff --git a/apps/web/app/actions/workspaces.test.ts b/apps/web/app/actions/workspaces.test.ts index 578b2a8f25..b7600ea782 100644 --- a/apps/web/app/actions/workspaces.test.ts +++ b/apps/web/app/actions/workspaces.test.ts @@ -4,7 +4,44 @@ vi.mock("@/lib/config", () => ({ getBackendConfig: () => ({ apiBaseUrl: "http://backend.test" }), })); -import { deleteWorkspaceAction } from "./workspaces"; +import { deleteWorkspaceAction, exportAllWorkflowsAction } from "./workspaces"; + +describe("exportAllWorkflowsAction", () => { + beforeEach(() => { + vi.stubGlobal( + "fetch", + vi.fn(async () => new Response("workflows: []", { status: 200 })), + ); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + }); + + const requestedUrl = () => { + const fetchMock = fetch as unknown as ReturnType; + expect(fetchMock).toHaveBeenCalledTimes(1); + return new URL(String(fetchMock.mock.calls[0][0])); + }; + + it("omits the ids param when no workflow IDs are passed (export all)", async () => { + await exportAllWorkflowsAction("ws-1"); + expect(requestedUrl().searchParams.has("ids")).toBe(false); + }); + + it("restricts the export to the provided workflow IDs", async () => { + await exportAllWorkflowsAction("ws-1", ["wf-1", "wf-3"]); + expect(requestedUrl().searchParams.get("ids")).toBe("wf-1,wf-3"); + }); + + it("sends an empty ids param so nothing is exported when the set is empty", async () => { + await exportAllWorkflowsAction("ws-1", []); + const url = requestedUrl(); + expect(url.searchParams.has("ids")).toBe(true); + expect(url.searchParams.get("ids")).toBe(""); + }); +}); describe("deleteWorkspaceAction", () => { beforeEach(() => { diff --git a/apps/web/app/actions/workspaces.ts b/apps/web/app/actions/workspaces.ts index 44a3eadf1c..c0c9e5c36c 100644 --- a/apps/web/app/actions/workspaces.ts +++ b/apps/web/app/actions/workspaces.ts @@ -555,8 +555,22 @@ export async function exportWorkflowAction(workflowId: string): Promise return response.text(); } -export async function exportAllWorkflowsAction(workspaceId: string): Promise { - const response = await fetch(`${apiBaseUrl}/api/v1/workspaces/${workspaceId}/workflows/export`); +export async function exportAllWorkflowsAction( + workspaceId: string, + workflowIds?: string[], +): Promise { + // When workflowIds is provided, restrict the export to exactly that set + // (the settings UI passes its kanban workflows, excluding office ones). An + // empty list is sent intentionally as `ids=` so nothing is exported, rather + // than omitting the param and falling back to exporting every workflow. + // Encode the user-provided values that flow into the request URL (workspace + // ID in the path, workflow IDs in the query) so they can't alter the request + // target — clears CodeQL's js/request-forgery (SSRF) check on this path. + const query = + workflowIds !== undefined ? `?ids=${encodeURIComponent(workflowIds.join(","))}` : ""; + const response = await fetch( + `${apiBaseUrl}/api/v1/workspaces/${encodeURIComponent(workspaceId)}/workflows/export${query}`, + ); if (!response.ok) throw new Error(`Export failed: ${response.statusText}`); return response.text(); } diff --git a/apps/web/app/settings/workspace/workspace-workflows-client.tsx b/apps/web/app/settings/workspace/workspace-workflows-client.tsx index ec885ea65f..ee878e50bb 100644 --- a/apps/web/app/settings/workspace/workspace-workflows-client.tsx +++ b/apps/web/app/settings/workspace/workspace-workflows-client.tsx @@ -95,6 +95,7 @@ function buildWorkflowSteps(workflow: Workflow, definitions: StepDefinition[]): function useWorkflowImportExport( workspace: Workspace | null, + workflowItems: Workflow[], router: ReturnType, toast: ReturnType["toast"], ) { @@ -108,7 +109,11 @@ function useWorkflowImportExport( const handleExportAll = async () => { if (!workspace) return; try { - const yamlText = await exportAllWorkflowsAction(workspace.id); + // Export only the workflows shown in this settings view (kanban-only — + // office workflows are filtered out upstream) and skip unsaved drafts. + // Workflow import/export is kanban-only by design (ADR-0004). + const exportIds = workflowItems.filter((wf) => !wf.id.startsWith("temp-")).map((wf) => wf.id); + const yamlText = await exportAllWorkflowsAction(workspace.id, exportIds); setExportYaml(yamlText); setIsExportDialogOpen(true); } catch (error) { @@ -531,10 +536,17 @@ function useWorkspaceWorkflowsPage( ) { const router = useRouter(); const { toast } = useToast(); + // Workflow settings is kanban-only by design (ADR-0004): office-style + // workflows are managed from the Office surface and are not importable / + // exportable here, so we keep them out of this view entirely. + const kanbanWorkflows = useMemo( + () => workflows.filter((wf) => wf.style !== "office"), + [workflows], + ); const { workflowItems, setWorkflowItems, setSavedWorkflowItems, isWorkflowDirty } = - useWorkflowSettings(workflows, workspace?.id); + useWorkflowSettings(kanbanWorkflows, workspace?.id); - const importExport = useWorkflowImportExport(workspace, router, toast); + const importExport = useWorkflowImportExport(workspace, workflowItems, router, toast); const { isExportDialogOpen, setIsExportDialogOpen, diff --git a/apps/web/e2e/helpers/api-client.ts b/apps/web/e2e/helpers/api-client.ts index 10aa2069e3..b1d0c28993 100644 --- a/apps/web/e2e/helpers/api-client.ts +++ b/apps/web/e2e/helpers/api-client.ts @@ -197,6 +197,25 @@ export class ApiClient { }); } + /** + * Seed a workflow with an explicit style (kanban / office / custom) via the + * KANDEV_E2E_MOCK test harness. Production has no HTTP path that creates an + * office-style workflow (the normal create endpoint always normalises to + * kanban), so this is the only way to stand up an office workflow for the + * "exclude office from settings export" coverage (issue #1109). + */ + async seedWorkflow( + workspaceId: string, + name: string, + style: "kanban" | "office" | "custom", + ): Promise<{ workflow_id: string }> { + return this.request("POST", "/api/v1/_test/workflows", { + workspace_id: workspaceId, + name, + style, + }); + } + async reorderWorkflows( workspaceId: string, workflowIds: string[], diff --git a/apps/web/e2e/tests/workflow/workflow-import-export.spec.ts b/apps/web/e2e/tests/workflow/workflow-import-export.spec.ts index d2a395113a..8933b34af7 100644 --- a/apps/web/e2e/tests/workflow/workflow-import-export.spec.ts +++ b/apps/web/e2e/tests/workflow/workflow-import-export.spec.ts @@ -30,6 +30,30 @@ test.describe("Workflow import/export", () => { await expect(dialog).not.toBeVisible(); }); + test("export all excludes office-style workflows", async ({ testPage, apiClient, seedData }) => { + // Workflow settings is kanban-only by design (ADR-0004). Seed an + // office-style workflow alongside the kanban "E2E Workflow" and confirm + // Export All omits it (regression for issue #1109, where Export All + // dumped every workflow — office triggers would silently drop on import). + await apiClient.seedWorkflow(seedData.workspaceId, "Office Only Workflow", "office"); + + const page = new WorkflowSettingsPage(testPage); + await page.goto(seedData.workspaceId); + + // The office workflow must not even appear in the kanban settings list. + await expect(testPage.getByText("Office Only Workflow")).toHaveCount(0); + + await testPage.getByRole("button", { name: "Export All" }).click(); + + const dialog = testPage.getByRole("dialog"); + await expect(dialog).toBeVisible(); + const yamlContent = await dialog.locator("textarea").inputValue(); + expect(yamlContent).toContain("E2E Workflow"); + expect(yamlContent).not.toContain("Office Only Workflow"); + + await dialog.getByRole("button", { name: "Close" }).first().click(); + }); + test("per-workflow export button shows that workflow's YAML", async ({ testPage, seedData }) => { const page = new WorkflowSettingsPage(testPage); await page.goto(seedData.workspaceId);