From 0407f7a8e7e8974822a79067f9a0506094b3e1c1 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Tue, 1 Sep 2026 23:19:13 -0700 Subject: [PATCH 1/5] Allow PATCH /routines/:id to replace a routine's target (CL-7353) A routine's definitionAssetId can now be retargeted through the same PATCH used for every other field, persisted in one UPDATE alongside whatever else the patch carries. The launcher already re-resolves definitionAssetId to the definition that runs at the moment of each fire, so a retarget lands atomically for the next run without any optimistic-locking machinery. --- packages/routines/src/routes.ts | 14 +++++++++ packages/routines/src/store.ts | 9 ++++++ .../routines/src/workflow-routine-routes.ts | 5 ++++ packages/routines/test/routes.test.ts | 29 +++++++++++++++++++ 4 files changed, 57 insertions(+) diff --git a/packages/routines/src/routes.ts b/packages/routines/src/routes.ts index e15afecd3d..6ee4e2d069 100644 --- a/packages/routines/src/routes.ts +++ b/packages/routines/src/routes.ts @@ -232,6 +232,9 @@ const UpdateRoutineBody = type({ "input?": "Record", "enabled?": "boolean", "deliveryWorkbenchId?": "string", + // Retargets the routine (CL-7353): a workflow asset id, same rule as + // `CreateRoutineBody`'s own field — an explicit target, never inferred. + "definitionAssetId?": "string > 0", }); const RunNowBody = type({ @@ -342,6 +345,14 @@ async function runView( * root timeline — see `RoutineLauncher`'s own doc comment for the * multi-message contract. * + * A retarget (CL-7353) is safe against an in-flight fire because the + * launcher itself re-resolves `definitionAssetId` through + * `resolveLaunchableDefinition` exactly once, at the moment this call + * launches (see `RoutineLauncher`'s doc comment and the hub's own + * `createHubRoutineLauncher`) — a run always launches against whatever + * one definition that single read named, never a definition read before + * the retarget landed spliced with one read after. + * * Exported: `./workflow-routine-routes.ts`'s "run now" reuses this exact * launch-then-correlate call, never a second launch path for Myra's own * tenant-scoped routine surface either. @@ -817,6 +828,9 @@ export function createRoutineRoutes( if (body.deliveryWorkbenchId !== undefined) { patch = { ...patch, deliveryWorkbenchId: body.deliveryWorkbenchId }; } + if (body.definitionAssetId !== undefined) { + patch = { ...patch, definitionAssetId: body.definitionAssetId }; + } const row = await deps.store.updateRoutine(tenant.id, routineId, patch); diff --git a/packages/routines/src/store.ts b/packages/routines/src/store.ts index e67fb9eb8f..dd7bf7f1e9 100644 --- a/packages/routines/src/store.ts +++ b/packages/routines/src/store.ts @@ -102,6 +102,15 @@ export interface UpdateRoutineInput { readonly input?: Record; readonly enabled?: boolean; readonly deliveryWorkbenchId?: string | null; + /** + * Retargets the routine to a different workflow asset — the same + * single UPDATE as every other field here, so a launch that reads the + * row once (`resolveLaunchableDefinition` at fire time, `./target.ts`) + * never sees a half-applied retarget. The route validates this through + * `resolveLaunchableDefinition` before it ever reaches the store (see + * `./routes.ts`'s `rejectUnlaunchableTarget`). + */ + readonly definitionAssetId?: string; } export interface RoutineRunRow { diff --git a/packages/routines/src/workflow-routine-routes.ts b/packages/routines/src/workflow-routine-routes.ts index 2cacaf1f86..a8c20aa881 100644 --- a/packages/routines/src/workflow-routine-routes.ts +++ b/packages/routines/src/workflow-routine-routes.ts @@ -153,6 +153,8 @@ const UpdateWorkflowRoutineBody = type({ "definitionAssetId?": "string > 0", "trigger?": RoutineTrigger, "input?": "Record", + // Retargets the routine (CL-7353) — same rule as `CreateWorkflowRoutineBody`. + "definitionAssetId?": "string > 0", }); const RunNowBody = type({ @@ -495,6 +497,9 @@ export function createWorkflowRoutineRoutes( if (body.trigger !== undefined) patch = { ...patch, trigger: body.trigger }; if (body.input !== undefined) patch = { ...patch, input: body.input }; if (body.enabled !== undefined) patch = { ...patch, enabled: body.enabled }; + if (body.definitionAssetId !== undefined) { + patch = { ...patch, definitionAssetId: body.definitionAssetId }; + } const row = await deps.store.updateRoutine( scope.tenantId, diff --git a/packages/routines/test/routes.test.ts b/packages/routines/test/routes.test.ts index 647aa1c3b1..14a780abc6 100644 --- a/packages/routines/test/routes.test.ts +++ b/packages/routines/test/routes.test.ts @@ -907,6 +907,35 @@ describe("createRoutineRoutes", () => { expect(seenInput).toEqual({ agent: "wfd_agent", prompt: "Do it" }); }); }); + + // CL-7353: PATCH can replace the deployed-definition target. + describe("retargeting a routine's definition (CL-7353)", () => { + test("a retarget-only PATCH changes definitionAssetId and leaves every other field untouched", async () => { + const deps = buildDeps(); + const app = mountAs(createRoutineRoutes(deps), "user_1"); + const { body: created } = await createRoutine(app, { + ...VALID_BODY, + input: { topic: "AI coding agents" }, + }); + + const response = await app.request(`/routines/${created["id"]}`, { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ definitionAssetId: "def_other" }), + }); + const body = (await response.json()) as Record; + + expect(response.status).toBe(200); + expect(body["definitionAssetId"]).toBe("def_other"); + // Everything else — name, trigger, scope, input, deliveryWorkbenchId + // — survives the retarget, a single UPDATE alongside every other + // patchable field, never a second write. + expect(body["name"]).toBe(created["name"]); + expect(body["trigger"]).toEqual(created["trigger"]); + expect(body["input"]).toEqual(created["input"]); + expect(body["deliveryWorkbenchId"]).toBe(created["deliveryWorkbenchId"]); + }); + }); }); describe("fireScheduledRoutine", () => { From 0cff5dfd5ccbc5edc531c3e35680ee0c6a414b52 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Tue, 1 Sep 2026 23:21:16 -0700 Subject: [PATCH 2/5] Enforce authorization on routine targets (CL-7354) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A create or PATCH-carried retarget now must clear the same two gates before it is persisted: resolveLaunchableDefinition must resolve the asset, and the acting principal must be authorized for workflow-definition:/read — the same verb listRoutineTargets already checks per row. A denial is a typed 403 alongside the existing 400/404/409 target envelopes, on both the tenant-session and Myra's workflow-run-authenticated routine surfaces. Also routes a pre-existing catch in postRoutineEnabledNotice through reportError, since this change's diff now touches that line and check:report-error requires it. --- apps/hub/src/index.ts | 9 ++ bun.lock | 1 + packages/routines/package.json | 1 + packages/routines/src/routes.ts | 96 ++++++++++++--- .../routines/src/workflow-routine-routes.ts | 29 +++++ packages/routines/test/routes.test.ts | 116 +++++++++++++++++- scripts/checks/report-error-baseline.txt | 1 - 7 files changed, 235 insertions(+), 18 deletions(-) diff --git a/apps/hub/src/index.ts b/apps/hub/src/index.ts index 93004eb266..ad24db67e7 100644 --- a/apps/hub/src/index.ts +++ b/apps/hub/src/index.ts @@ -2901,6 +2901,11 @@ export async function createHub(config: HubConfig) { grantStore: routineGrantStore, conditionRegistry: chatConditionRegistry, }), + // CL-7354: a create or retarget's resolved target must also + // authorize for the acting principal — same `grantStore` the grant + // check above already uses. + grantStore: routineGrantStore, + conditionRegistry: chatConditionRegistry, // A run-now or a scheduled fire's result is a message into the // routine's delivery workbench root timeline — never a pre-opened // thread; see `@corbits/routines`' `RoutineLauncher` doc comment @@ -2955,6 +2960,10 @@ export async function createHub(config: HubConfig) { }, query, ), + // CL-7354: same authorization the tenant-session surface enforces + // above, for Myra's own create/retarget path. + grantStore: routineGrantStore, + conditionRegistry: chatConditionRegistry, webhookTriggerInTenant, deliveryWorkbenchRequired: routineDeliveryWorkbenchRequired, // A routine created from inside a workbench delivers into that diff --git a/bun.lock b/bun.lock index 95ef2a9527..6ee248c3dd 100644 --- a/bun.lock +++ b/bun.lock @@ -1212,6 +1212,7 @@ "version": "0.0.1", "dependencies": { "@corbits/chat": "workspace:*", + "@corbits/error-sink": "workspace:*", "@corbits/folded-run-one-shot": "workspace:*", "@corbits/migration-runner": "workspace:*", "@corbits/slug": "workspace:*", diff --git a/packages/routines/package.json b/packages/routines/package.json index 0347a82dc7..a9ec6a7d66 100644 --- a/packages/routines/package.json +++ b/packages/routines/package.json @@ -17,6 +17,7 @@ }, "dependencies": { "@corbits/chat": "workspace:*", + "@corbits/error-sink": "workspace:*", "@corbits/folded-run-one-shot": "workspace:*", "@corbits/migration-runner": "workspace:*", "@corbits/slug": "workspace:*", diff --git a/packages/routines/src/routes.ts b/packages/routines/src/routes.ts index 6ee4e2d069..c104c36163 100644 --- a/packages/routines/src/routes.ts +++ b/packages/routines/src/routes.ts @@ -15,6 +15,9 @@ import type { RequireGrant } from "@intx/hub-api"; import { idResource } from "@intx/hub-api"; import { getLogger } from "@intx/log"; import { generateId } from "@intx/hub-common"; +import { authorize } from "@intx/authz"; +import type { ConditionRegistry, GrantStore } from "@intx/types/authz"; +import { reportError } from "@corbits/error-sink"; import { FoldedRunFailedError, FoldedRunTimedOutError, @@ -127,6 +130,17 @@ export type CreateRoutineRoutesDeps = { * `definitionId: null`. */ resolveTarget?: LaunchableDefinitionResolver; + /** + * When wired alongside `resolveTarget`, a create/retarget's resolved + * definition is also authorized (`workflow-definition:` + * / `read`, the same verb `listRoutineTargets` checks in + * `./targets.ts`) for the acting principal before the target is + * accepted — a denial is the typed 403 `rejectUnlaunchableTarget` + * returns. Both must be wired together; either omitted (the prior + * default) skips authorization, unchanged behavior. + */ + grantStore?: GrantStore; + conditionRegistry?: ConditionRegistry; /** * When provided, a `{kind: "webhook"}` trigger is rejected with 404 * unless the referenced `@corbits/webhook-triggers` row exists in the @@ -306,19 +320,51 @@ export async function resolvedRoutineView( } /** - * Refuses a create/retarget whose target does not resolve — the typed - * envelope UI and Myra branch on. `undefined` means the target is - * launchable (or no resolver is wired). Exported for - * `./workflow-routine-routes.ts`. + * Refuses a create/retarget whose target does not resolve, or that the + * acting principal is not authorized to reference — the typed envelope + * UI and Myra branch on. `undefined` means the target is launchable (or + * no resolver is wired). Exported for `./workflow-routine-routes.ts`. + * + * Authorization checks the same `workflow-definition:` / + * `read` verb `listRoutineTargets` (`./targets.ts`) checks per row, so a + * routine can never be pointed at a definition its own target listing + * would never have offered — but only when `grantStore` and + * `conditionRegistry` are both wired; either omitted skips it (CL-7351's + * prior default), matching every caller's existing tests. */ export async function rejectUnlaunchableTarget( - deps: Pick, + deps: Pick< + CreateRoutineRoutesDeps, + "resolveTarget" | "grantStore" | "conditionRegistry" + >, tenantId: string, + principalId: string, definitionAssetId: string, -): Promise | undefined> { +): Promise< + | ReturnType + | { readonly status: 403; readonly code: string; readonly userMessage: string } + | undefined +> { if (deps.resolveTarget === undefined) return undefined; const target = await deps.resolveTarget(tenantId, definitionAssetId); - return target.ok ? undefined : routineTargetRejection(target.reason); + if (!target.ok) return routineTargetRejection(target.reason); + if (deps.grantStore === undefined || deps.conditionRegistry === undefined) { + return undefined; + } + const decision = await authorize( + deps.grantStore, + principalId, + tenantId, + `workflow-definition:${target.definitionId}`, + "read", + deps.conditionRegistry, + ); + if (decision.effect === "allow") return undefined; + return { + status: 403, + code: "routine_target_forbidden", + userMessage: "You don't have access to that workflow.", + }; } async function runView( @@ -530,14 +576,11 @@ export async function postRoutineEnabledNotice( text, }); } catch (err) { - log.error( - "Failed to post routine-{verb} notice into workbench {workbenchId}", - { - verb: input.verb, - workbenchId: input.workbenchId, - err, - }, - ); + reportError(err, { + operation: "routines.postRoutineEnabledNotice", + tenantId: input.tenantId, + extra: { verb: input.verb, workbenchId: input.workbenchId }, + }); } } @@ -580,6 +623,7 @@ export function createRoutineRoutes( const rejection = await rejectUnlaunchableTarget( deps, tenant.id, + principal.id, body.definitionAssetId, ); if (rejection !== undefined) { @@ -798,6 +842,27 @@ export function createRoutineRoutes( ); } + if ( + body.definitionAssetId !== undefined && + body.definitionAssetId !== existing.definitionAssetId + ) { + const rejection = await rejectUnlaunchableTarget( + deps, + tenant.id, + principal.id, + body.definitionAssetId, + ); + if (rejection !== undefined) { + return c.json( + makeErrorEnvelope({ + code: rejection.code, + userMessage: rejection.userMessage, + }), + rejection.status, + ); + } + } + if ( body.trigger !== undefined && !(await webhookTriggerValid( @@ -1167,6 +1232,7 @@ export function createRoutineRoutes( const rejection = await rejectUnlaunchableTarget( deps, tenant.id, + principal.id, definitionAssetId, ); if (rejection !== undefined) { diff --git a/packages/routines/src/workflow-routine-routes.ts b/packages/routines/src/workflow-routine-routes.ts index a8c20aa881..f90ecdf0dd 100644 --- a/packages/routines/src/workflow-routine-routes.ts +++ b/packages/routines/src/workflow-routine-routes.ts @@ -52,6 +52,7 @@ import { type RoutineTargetsPage, type RoutineTargetsQuery, } from "./targets"; +import type { ConditionRegistry, GrantStore } from "@intx/types/authz"; const TargetsLimitParam = type("string.integer.parse").narrow( (limit) => limit >= 1 && limit <= ROUTINE_TARGETS_MAX_LIMIT, @@ -96,6 +97,12 @@ export type CreateWorkflowRoutineRoutesDeps = { listTargets: (query: RoutineTargetsQuery) => Promise; /** Same contract as `CreateRoutineRoutesDeps.resolveTarget`. */ resolveTarget?: LaunchableDefinitionResolver; + /** Same contract as `CreateRoutineRoutesDeps.grantStore` / + * `conditionRegistry` — both wired authorizes a create/retarget's + * resolved target for Myra's own acting principal, same as the + * tenant-session surface. */ + grantStore?: GrantStore; + conditionRegistry?: ConditionRegistry; /** Same contract as `CreateRoutineRoutesDeps.webhookTriggerInTenant`. */ webhookTriggerInTenant?: ( tenantId: string, @@ -254,6 +261,7 @@ export function createWorkflowRoutineRoutes( const rejection = await rejectUnlaunchableTarget( deps, scope.tenantId, + scope.principalId, definitionAssetId, ); if (rejection !== undefined) { @@ -431,6 +439,27 @@ export function createWorkflowRoutineRoutes( const effectiveDefinitionAssetId = body.definitionAssetId ?? existing.definitionAssetId; + if ( + body.definitionAssetId !== undefined && + body.definitionAssetId !== existing.definitionAssetId + ) { + const rejection = await rejectUnlaunchableTarget( + deps, + scope.tenantId, + scope.principalId, + body.definitionAssetId, + ); + if (rejection !== undefined) { + return c.json( + makeErrorEnvelope({ + code: rejection.code, + userMessage: rejection.userMessage, + }), + rejection.status, + ); + } + } + if ( body.trigger !== undefined && !(await webhookTriggerValid( diff --git a/packages/routines/test/routes.test.ts b/packages/routines/test/routes.test.ts index 14a780abc6..ad741c6c47 100644 --- a/packages/routines/test/routes.test.ts +++ b/packages/routines/test/routes.test.ts @@ -7,6 +7,7 @@ import { Hono } from "hono"; import type { MiddlewareHandler } from "hono"; import type { TenantEnv } from "@intx/hub-api"; +import type { ConditionRegistry, GrantRule, GrantStore } from "@intx/types/authz"; import { createRoutineRoutes, fireScheduledRoutine, @@ -98,6 +99,29 @@ function storeCreatingDisabled(): RoutineStore { }; } +function allowGrant(): GrantRule { + return { + id: "g_read", + resource: "workflow-definition:*", + action: "read", + effect: "allow", + origin: "system", + conditions: null, + expiresAt: null, + roleId: null, + principalId: null, + }; +} + +function fakeGrantStore(grants: readonly GrantRule[]): GrantStore { + return { + collectGrants: async () => [...grants], + collectGrantsInChain: async () => [...grants], + }; +} + +const conditionRegistry: ConditionRegistry = {}; + function mountAs( routes: Hono, principalId: string, @@ -908,8 +932,10 @@ describe("createRoutineRoutes", () => { }); }); - // CL-7353: PATCH can replace the deployed-definition target. - describe("retargeting a routine's definition (CL-7353)", () => { + // CL-7353/CL-7354: PATCH can replace the deployed-definition target, and + // every retarget (create or PATCH) is validated the same way a create's + // own target is — resolved and, when authz deps are wired, authorized. + describe("retargeting a routine's definition (CL-7353/CL-7354)", () => { test("a retarget-only PATCH changes definitionAssetId and leaves every other field untouched", async () => { const deps = buildDeps(); const app = mountAs(createRoutineRoutes(deps), "user_1"); @@ -935,6 +961,92 @@ describe("createRoutineRoutes", () => { expect(body["input"]).toEqual(created["input"]); expect(body["deliveryWorkbenchId"]).toBe(created["deliveryWorkbenchId"]); }); + + test("PATCH rejects an unresolvable retarget with the typed status, leaving the stored target unchanged", async () => { + const deps = buildDeps({ + resolveTarget: async (_tenantId, definitionAssetId) => + definitionAssetId === VALID_BODY.definitionAssetId + ? { ok: true, definitionId: "wfd_v1", wireHash: "h1" } + : { ok: false, reason: "not_deployed" }, + }); + const app = mountAs(createRoutineRoutes(deps), "user_1"); + const { body: created } = await createRoutine(app, VALID_BODY); + + const response = await app.request(`/routines/${created["id"]}`, { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ definitionAssetId: "def_not_deployed" }), + }); + const body = (await response.json()) as Record; + + expect(response.status).toBe(409); + expect((body["error"] as Record)["code"]).toBe( + "routine_target_not_deployed", + ); + + const stored = await deps.store.getRoutine( + TENANT.id, + created["id"] as string, + ); + expect(stored?.definitionAssetId).toBe(VALID_BODY.definitionAssetId); + }); + + test("a create is refused 403 when the resolved target is not authorized for the acting principal", async () => { + const deps = buildDeps({ + resolveTarget: async () => ({ + ok: true, + definitionId: "wfd_v1", + wireHash: "h1", + }), + grantStore: fakeGrantStore([]), + conditionRegistry, + }); + const app = mountAs(createRoutineRoutes(deps), "user_1"); + const { response, body } = await createRoutine(app, VALID_BODY); + + expect(response.status).toBe(403); + expect((body["error"] as Record)["code"]).toBe( + "routine_target_forbidden", + ); + expect(await deps.store.listRoutines(TENANT.id)).toEqual([]); + }); + + test("a retarget PATCH is refused 403 when the new target is not authorized, and an allowed create still succeeds", async () => { + const deps = buildDeps({ + resolveTarget: async () => ({ + ok: true, + definitionId: "wfd_v1", + wireHash: "h1", + }), + grantStore: fakeGrantStore([allowGrant()]), + conditionRegistry, + }); + const app = mountAs(createRoutineRoutes(deps), "user_1"); + const { response: createResponse, body: created } = await createRoutine( + app, + VALID_BODY, + ); + expect(createResponse.status).toBe(201); + + // Flip to a denying grant store for the retarget itself. + deps.grantStore = fakeGrantStore([]); + const response = await app.request(`/routines/${created["id"]}`, { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ definitionAssetId: "def_forbidden" }), + }); + const body = (await response.json()) as Record; + + expect(response.status).toBe(403); + expect((body["error"] as Record)["code"]).toBe( + "routine_target_forbidden", + ); + const stored = await deps.store.getRoutine( + TENANT.id, + created["id"] as string, + ); + expect(stored?.definitionAssetId).toBe(VALID_BODY.definitionAssetId); + }); }); }); diff --git a/scripts/checks/report-error-baseline.txt b/scripts/checks/report-error-baseline.txt index f0562c5baf..ade93a472b 100644 --- a/scripts/checks/report-error-baseline.txt +++ b/scripts/checks/report-error-baseline.txt @@ -231,7 +231,6 @@ packages/routines/src/cron.ts 1 return false; packages/routines/src/cron.ts 2 return false; packages/routines/src/routes.ts 1 log.error( packages/routines/src/routes.ts 1 return c.json( -packages/routines/src/routes.ts 2 log.error( packages/routines/src/schedule-language.ts 1 return null; packages/sandbox-sidecar/src/provisioner.ts 1 log.error`failed to sweep obsolete unit ${externalRef} for allocation ${allocationId}: ${ packages/sandbox-sidecar/src/provisioner.ts 1 return rejected(...classify(error, "destroy_unit_failed")); From 76b87f7cb2591784272fa679ee0094bde7a760a4 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Wed, 2 Sep 2026 00:41:33 -0700 Subject: [PATCH 3/5] Fix duplicate definitionAssetId property and rejectUnlaunchableTarget arity from rebase merge --- packages/routines/src/store.ts | 20 ++++++------- .../routines/src/workflow-routine-routes.ts | 30 +++---------------- 2 files changed, 13 insertions(+), 37 deletions(-) diff --git a/packages/routines/src/store.ts b/packages/routines/src/store.ts index dd7bf7f1e9..ef8767be95 100644 --- a/packages/routines/src/store.ts +++ b/packages/routines/src/store.ts @@ -95,22 +95,20 @@ export type CreateRoutineIfAbsentResult = export interface UpdateRoutineInput { readonly name?: string; - /** Retargets the routine at a different workflow asset (CL-7359) — - * see `CreateRoutineInput.definitionAssetId`'s own doc comment. */ + /** + * Retargets the routine to a different workflow asset (CL-7359) — the + * same single UPDATE as every other field here, so a launch that reads + * the row once (`resolveLaunchableDefinition` at fire time, + * `./target.ts`) never sees a half-applied retarget. The route + * validates this through `resolveLaunchableDefinition` before it ever + * reaches the store (see `./routes.ts`'s `rejectUnlaunchableTarget`) — + * see `CreateRoutineInput.definitionAssetId`'s own doc comment. + */ readonly definitionAssetId?: string; readonly trigger?: RoutineTriggerT; readonly input?: Record; readonly enabled?: boolean; readonly deliveryWorkbenchId?: string | null; - /** - * Retargets the routine to a different workflow asset — the same - * single UPDATE as every other field here, so a launch that reads the - * row once (`resolveLaunchableDefinition` at fire time, `./target.ts`) - * never sees a half-applied retarget. The route validates this through - * `resolveLaunchableDefinition` before it ever reaches the store (see - * `./routes.ts`'s `rejectUnlaunchableTarget`). - */ - readonly definitionAssetId?: string; } export interface RoutineRunRow { diff --git a/packages/routines/src/workflow-routine-routes.ts b/packages/routines/src/workflow-routine-routes.ts index f90ecdf0dd..656634ef61 100644 --- a/packages/routines/src/workflow-routine-routes.ts +++ b/packages/routines/src/workflow-routine-routes.ts @@ -154,14 +154,12 @@ const CreateWorkflowRoutineBody = type({ const UpdateWorkflowRoutineBody = type({ "enabled?": "boolean", "name?": "string", - /** Retargets the routine at a different workflow asset (CL-7359) — - * an explicit asset id, never a name search, same as `create`'s - * `definitionAssetId`. */ + /** Retargets the routine at a different workflow asset (CL-7359, + * CL-7353) — an explicit asset id, never a name search, same as + * `create`'s `definitionAssetId`. */ "definitionAssetId?": "string > 0", "trigger?": RoutineTrigger, "input?": "Record", - // Retargets the routine (CL-7353) — same rule as `CreateWorkflowRoutineBody`. - "definitionAssetId?": "string > 0", }); const RunNowBody = type({ @@ -423,6 +421,7 @@ export function createWorkflowRoutineRoutes( const rejection = await rejectUnlaunchableTarget( deps, scope.tenantId, + scope.principalId, body.definitionAssetId, ); if (rejection !== undefined) { @@ -439,27 +438,6 @@ export function createWorkflowRoutineRoutes( const effectiveDefinitionAssetId = body.definitionAssetId ?? existing.definitionAssetId; - if ( - body.definitionAssetId !== undefined && - body.definitionAssetId !== existing.definitionAssetId - ) { - const rejection = await rejectUnlaunchableTarget( - deps, - scope.tenantId, - scope.principalId, - body.definitionAssetId, - ); - if (rejection !== undefined) { - return c.json( - makeErrorEnvelope({ - code: rejection.code, - userMessage: rejection.userMessage, - }), - rejection.status, - ); - } - } - if ( body.trigger !== undefined && !(await webhookTriggerValid( From 4bcdc4d767b0d8df1ce127ec492bfccd1dbafaaa Mon Sep 17 00:00:00 2001 From: Sawyer Date: Wed, 2 Sep 2026 01:08:12 -0700 Subject: [PATCH 4/5] Address review findings (CL-7353) --- packages/routines/src/routes.ts | 46 +++++--- packages/routines/src/routine-operations.ts | 82 ++++++++++++++ .../src/workflow-routine-routes.test.ts | 103 ++++++++++++++++++ .../routines/src/workflow-routine-routes.ts | 37 ++----- packages/routines/test/routes.test.ts | 16 ++- 5 files changed, 236 insertions(+), 48 deletions(-) create mode 100644 packages/routines/src/routine-operations.ts diff --git a/packages/routines/src/routes.ts b/packages/routines/src/routes.ts index c104c36163..39f4de3b74 100644 --- a/packages/routines/src/routes.ts +++ b/packages/routines/src/routes.ts @@ -36,6 +36,7 @@ import { routineTargetRejection, type LaunchableDefinitionResolver, } from "./target"; +import { validateRetarget } from "./routine-operations"; import { makeErrorEnvelope } from "@workbench/hub-client"; import { MyraRoutineDraftingUnavailableError, @@ -340,11 +341,7 @@ export async function rejectUnlaunchableTarget( tenantId: string, principalId: string, definitionAssetId: string, -): Promise< - | ReturnType - | { readonly status: 403; readonly code: string; readonly userMessage: string } - | undefined -> { +): Promise | undefined> { if (deps.resolveTarget === undefined) return undefined; const target = await deps.resolveTarget(tenantId, definitionAssetId); if (!target.ok) return routineTargetRejection(target.reason); @@ -360,11 +357,12 @@ export async function rejectUnlaunchableTarget( deps.conditionRegistry, ); if (decision.effect === "allow") return undefined; - return { - status: 403, - code: "routine_target_forbidden", - userMessage: "You don't have access to that workflow.", - }; + // A denial is reported identically to "not found" — naming a + // deployed-but-ungranted definition must not let a caller distinguish + // "exists, no access" from "doesn't exist" by probing different ids, + // same rule `target.ts`'s `routineTargetRejection` states for a + // cross-tenant asset. + return routineTargetRejection("not_found"); } async function runView( @@ -842,10 +840,11 @@ export function createRoutineRoutes( ); } - if ( + const isRetarget = body.definitionAssetId !== undefined && - body.definitionAssetId !== existing.definitionAssetId - ) { + body.definitionAssetId !== existing.definitionAssetId; + + if (isRetarget && body.definitionAssetId !== undefined) { const rejection = await rejectUnlaunchableTarget( deps, tenant.id, @@ -861,15 +860,34 @@ export function createRoutineRoutes( rejection.status, ); } + + const retargetRejection = await validateRetarget( + deps, + tenant.id, + body.definitionAssetId, + existing, + ); + if (retargetRejection !== undefined) { + return c.json( + makeErrorEnvelope({ + code: retargetRejection.code, + userMessage: retargetRejection.userMessage, + }), + 400, + ); + } } + const effectiveDefinitionAssetId = + body.definitionAssetId ?? existing.definitionAssetId; + if ( body.trigger !== undefined && !(await webhookTriggerValid( deps, tenant.id, body.trigger, - existing.definitionAssetId, + effectiveDefinitionAssetId, )) ) { return c.json( diff --git a/packages/routines/src/routine-operations.ts b/packages/routines/src/routine-operations.ts new file mode 100644 index 0000000000..4647d1a9e9 --- /dev/null +++ b/packages/routines/src/routine-operations.ts @@ -0,0 +1,82 @@ +// The one place a routine retarget's non-authz validation lives, shared +// by both route surfaces that can retarget a routine: `./routes.ts`'s +// tenant-session `PATCH /routines/:id` and `./workflow-routine-routes.ts`'s +// run-authenticated mirror. `rejectUnlaunchableTarget` (./routes.ts) +// already covers whether the new target resolves and is authorized for +// the acting principal; this covers the two checks `POST /routines` +// (create) also runs against a fresh target — an unsuited delivery +// workbench and an existing `input` that no longer satisfies the new +// definition's input schema — so a retarget can't silently produce a +// routine that fails at next launch the way create never could. Greybeard +// review (CL-7353): PR 555 had to patch authz into both route factories +// independently; this module is the seam that stops the next fix from +// needing the same double patch for retarget validation specifically. +// No import from `./routes` here on purpose: `./routes.ts` imports +// `validateRetarget` from this module, so this module importing back from +// `./routes.ts` would be a cycle. `deliveryWorkbenchRequired`'s +// "omitted means required" default is duplicated from +// `routes.ts`'s `isDeliveryWorkbenchRequired` (one line, unlikely to +// drift; both are covered by the same tests via each route's PATCH). +export type RetargetValidationDeps = { + readonly deliveryWorkbenchRequired?: ( + tenantId: string, + definitionAssetId: string, + ) => Promise; + readonly validateRoutineInput?: ( + tenantId: string, + definitionAssetId: string, + input: Record, + ) => Promise< + { readonly ok: true } | { readonly ok: false; readonly message: string } + >; +}; + +export type RetargetValidationRejection = { + readonly code: "bad_request"; + readonly userMessage: string; +}; + +/** + * Re-runs create's delivery-workbench-required and input-schema checks + * against a routine being retargeted at `effectiveDefinitionAssetId`, + * using the routine's own existing `deliveryWorkbenchId`/`input` (a + * retarget-only PATCH never asks the caller to resupply fields it isn't + * changing). `undefined` means the retarget's non-target fields are still + * satisfied by the new target. + */ +export async function validateRetarget( + deps: RetargetValidationDeps, + tenantId: string, + effectiveDefinitionAssetId: string, + existing: { + readonly deliveryWorkbenchId: string | null; + readonly input: Record; + }, +): Promise { + const deliveryRequired = + deps.deliveryWorkbenchRequired === undefined + ? true + : await deps.deliveryWorkbenchRequired( + tenantId, + effectiveDefinitionAssetId, + ); + if (deliveryRequired && existing.deliveryWorkbenchId === null) { + return { + code: "bad_request", + userMessage: "deliveryWorkbenchId is required for this workflow", + }; + } + + if (deps.validateRoutineInput !== undefined) { + const validated = await deps.validateRoutineInput( + tenantId, + effectiveDefinitionAssetId, + existing.input, + ); + if (!validated.ok) { + return { code: "bad_request", userMessage: validated.message }; + } + } + + return undefined; +} diff --git a/packages/routines/src/workflow-routine-routes.test.ts b/packages/routines/src/workflow-routine-routes.test.ts index 1e51494d14..4fd1e064e8 100644 --- a/packages/routines/src/workflow-routine-routes.test.ts +++ b/packages/routines/src/workflow-routine-routes.test.ts @@ -9,6 +9,7 @@ import { expect, test } from "bun:test"; import { Hono } from "hono"; +import type { ConditionRegistry, GrantRule, GrantStore } from "@intx/types/authz"; import { createWorkflowRoutineRoutes, type CreateWorkflowRoutineRoutesDeps, @@ -62,6 +63,29 @@ function fakeWorkbenchNotice(): WorkbenchNoticePort & { }; } +function allowGrant(): GrantRule { + return { + id: "g_read", + resource: "workflow-definition:*", + action: "read", + effect: "allow", + origin: "system", + conditions: null, + expiresAt: null, + roleId: null, + principalId: null, + }; +} + +function fakeGrantStore(grants: readonly GrantRule[]): GrantStore { + return { + collectGrants: async () => [...grants], + collectGrantsInChain: async () => [...grants], + }; +} + +const conditionRegistry: ConditionRegistry = {}; + /** A store that always creates a routine already disabled — see the * identical helper in `./test/routes.test.ts` for why. */ function storeCreatingDisabled(): RoutineStore { @@ -353,6 +377,85 @@ test("PATCH /routines/:id updates enabled, name, trigger, and input", async () = expect(body["input"]).toEqual({ instruction: "Summarize today's threads" }); }); +// Mirrors `./test/routes.test.ts`'s "retargeting a routine's definition +// (CL-7353/CL-7354)" describe block on this file's own run-authenticated +// (Myra) surface — `rejectUnlaunchableTarget` and the duplicate-patch fix +// both live in this file's PATCH handler, not the tenant-session one. +test("a retarget-only PATCH changes definitionAssetId and leaves every other field untouched", async () => { + const deps = buildDeps({ + resolveTarget: async (_tenantId, definitionAssetId) => ({ + ok: true, + definitionId: `wfd_${definitionAssetId}`, + wireHash: "h1", + }), + }); + const app = buildApp(deps); + const { body: created } = await createRoutine(app, VALID_BODY); + + const response = await app.request(`/routines/${String(created["id"])}`, { + method: "PATCH", + headers: { "content-type": "application/json", ...AUTH_HEADERS }, + body: JSON.stringify({ definitionAssetId: "ast_retargeted" }), + }); + const body = (await response.json()) as Record; + + expect(response.status).toBe(200); + expect(body["definitionAssetId"]).toBe("ast_retargeted"); + expect(body["name"]).toBe(created["name"]); + expect(body["trigger"]).toEqual(created["trigger"]); + expect(body["input"]).toEqual(created["input"]); +}); + +test("a retarget PATCH is refused 404 (indistinguishable from not-found) when the new target is not authorized", async () => { + const deps = buildDeps({ + resolveTarget: async () => ({ + ok: true, + definitionId: "wfd_v1", + wireHash: "h1", + }), + grantStore: fakeGrantStore([allowGrant()]), + conditionRegistry, + }); + const app = buildApp(deps); + const { response: createResponse, body: created } = await createRoutine( + app, + VALID_BODY, + ); + expect(createResponse.status).toBe(201); + + deps.grantStore = fakeGrantStore([]); + const response = await app.request(`/routines/${String(created["id"])}`, { + method: "PATCH", + headers: { "content-type": "application/json", ...AUTH_HEADERS }, + body: JSON.stringify({ definitionAssetId: "ast_forbidden" }), + }); + const body = (await response.json()) as Record; + + expect(response.status).toBe(404); + expect((body["error"] as Record)["code"]).toBe( + "routine_target_not_found", + ); +}); + +test("a create is refused 404 (indistinguishable from not-found) when the resolved target is not authorized", async () => { + const deps = buildDeps({ + resolveTarget: async () => ({ + ok: true, + definitionId: "wfd_v1", + wireHash: "h1", + }), + grantStore: fakeGrantStore([]), + conditionRegistry, + }); + const app = buildApp(deps); + const { response, body } = await createRoutine(app, VALID_BODY); + + expect(response.status).toBe(404); + expect((body["error"] as Record)["code"]).toBe( + "routine_target_not_found", + ); +}); + test("POST /routines posts an honest notice when created enabled", async () => { const workbenchNotice = fakeWorkbenchNotice(); const app = buildApp(buildDeps({ workbenchNotice })); diff --git a/packages/routines/src/workflow-routine-routes.ts b/packages/routines/src/workflow-routine-routes.ts index 656634ef61..42478efdfb 100644 --- a/packages/routines/src/workflow-routine-routes.ts +++ b/packages/routines/src/workflow-routine-routes.ts @@ -44,6 +44,7 @@ import { type WorkbenchNoticePort, type RoutineLauncher, } from "./routes"; +import { validateRetarget } from "./routine-operations"; import type { LaunchableDefinitionResolver } from "./target"; import { InvalidRoutineTargetCursorError, @@ -458,42 +459,25 @@ export function createWorkflowRoutineRoutes( // A retarget onto a different definition can leave a routine that // fails at next launch unless the same checks `POST /routines` runs - // for these fields are re-run against the new target: an unsuited - // delivery workbench (required/not-required can flip per-definition) - // and an existing `input` that no longer satisfies the new - // definition's input schema. + // for these fields are re-run against the new target — see + // `./routine-operations.ts`'s `validateRetarget`, shared with + // `./routes.ts`'s own PATCH handler so this can't drift a second time. if (body.definitionAssetId !== undefined) { - const deliveryRequired = await isDeliveryWorkbenchRequired( + const retargetRejection = await validateRetarget( deps, scope.tenantId, effectiveDefinitionAssetId, + existing, ); - if (deliveryRequired && existing.deliveryWorkbenchId === null) { + if (retargetRejection !== undefined) { return c.json( makeErrorEnvelope({ - code: "bad_request", - userMessage: "deliveryWorkbenchId is required for this workflow", + code: retargetRejection.code, + userMessage: retargetRejection.userMessage, }), 400, ); } - - if (deps.validateRoutineInput !== undefined) { - const validated = await deps.validateRoutineInput( - scope.tenantId, - effectiveDefinitionAssetId, - existing.input, - ); - if (!validated.ok) { - return c.json( - makeErrorEnvelope({ - code: "bad_request", - userMessage: validated.message, - }), - 400, - ); - } - } } let patch: UpdateRoutineInput = {}; @@ -504,9 +488,6 @@ export function createWorkflowRoutineRoutes( if (body.trigger !== undefined) patch = { ...patch, trigger: body.trigger }; if (body.input !== undefined) patch = { ...patch, input: body.input }; if (body.enabled !== undefined) patch = { ...patch, enabled: body.enabled }; - if (body.definitionAssetId !== undefined) { - patch = { ...patch, definitionAssetId: body.definitionAssetId }; - } const row = await deps.store.updateRoutine( scope.tenantId, diff --git a/packages/routines/test/routes.test.ts b/packages/routines/test/routes.test.ts index ad741c6c47..b3cf595e2c 100644 --- a/packages/routines/test/routes.test.ts +++ b/packages/routines/test/routes.test.ts @@ -991,7 +991,7 @@ describe("createRoutineRoutes", () => { expect(stored?.definitionAssetId).toBe(VALID_BODY.definitionAssetId); }); - test("a create is refused 403 when the resolved target is not authorized for the acting principal", async () => { + test("a create is refused 404 (indistinguishable from not-found) when the resolved target is not authorized for the acting principal", async () => { const deps = buildDeps({ resolveTarget: async () => ({ ok: true, @@ -1004,14 +1004,18 @@ describe("createRoutineRoutes", () => { const app = mountAs(createRoutineRoutes(deps), "user_1"); const { response, body } = await createRoutine(app, VALID_BODY); - expect(response.status).toBe(403); + // Authz denial reports the same 404 shape as "no such definition" — + // a tenant member who lacks a fine-grained grant on one definition + // must not be able to distinguish "exists, no access" from + // "doesn't exist" by probing create/PATCH with different ids. + expect(response.status).toBe(404); expect((body["error"] as Record)["code"]).toBe( - "routine_target_forbidden", + "routine_target_not_found", ); expect(await deps.store.listRoutines(TENANT.id)).toEqual([]); }); - test("a retarget PATCH is refused 403 when the new target is not authorized, and an allowed create still succeeds", async () => { + test("a retarget PATCH is refused 404 (indistinguishable from not-found) when the new target is not authorized, and an allowed create still succeeds", async () => { const deps = buildDeps({ resolveTarget: async () => ({ ok: true, @@ -1037,9 +1041,9 @@ describe("createRoutineRoutes", () => { }); const body = (await response.json()) as Record; - expect(response.status).toBe(403); + expect(response.status).toBe(404); expect((body["error"] as Record)["code"]).toBe( - "routine_target_forbidden", + "routine_target_not_found", ); const stored = await deps.store.getRoutine( TENANT.id, From ec2b67baf4128cbc46b67137c9cc47092522fc34 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Wed, 2 Sep 2026 03:32:39 -0700 Subject: [PATCH 5/5] Fix formatting (CL-7353) --- packages/routines/src/workflow-routine-routes.test.ts | 6 +++++- packages/routines/test/routes.test.ts | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/routines/src/workflow-routine-routes.test.ts b/packages/routines/src/workflow-routine-routes.test.ts index 4fd1e064e8..1f6b4d3c03 100644 --- a/packages/routines/src/workflow-routine-routes.test.ts +++ b/packages/routines/src/workflow-routine-routes.test.ts @@ -9,7 +9,11 @@ import { expect, test } from "bun:test"; import { Hono } from "hono"; -import type { ConditionRegistry, GrantRule, GrantStore } from "@intx/types/authz"; +import type { + ConditionRegistry, + GrantRule, + GrantStore, +} from "@intx/types/authz"; import { createWorkflowRoutineRoutes, type CreateWorkflowRoutineRoutesDeps, diff --git a/packages/routines/test/routes.test.ts b/packages/routines/test/routes.test.ts index b3cf595e2c..ee58227473 100644 --- a/packages/routines/test/routes.test.ts +++ b/packages/routines/test/routes.test.ts @@ -7,7 +7,11 @@ import { Hono } from "hono"; import type { MiddlewareHandler } from "hono"; import type { TenantEnv } from "@intx/hub-api"; -import type { ConditionRegistry, GrantRule, GrantStore } from "@intx/types/authz"; +import type { + ConditionRegistry, + GrantRule, + GrantStore, +} from "@intx/types/authz"; import { createRoutineRoutes, fireScheduledRoutine,