From 017abbff5da56485eb893f9d8000735164ceb8c2 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sun, 23 Aug 2026 11:45:58 -0700 Subject: [PATCH] fix(api): withhold internal failure messages from internal route responses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An orchestration result carrying `errorCode: 'internal'` holds whatever text the fault happened to have — `workflow-lifecycle.ts` catch-alls return `toError(error).message`, which is the driver's failed SQL. Three application helpers projected that straight into an `OrchestrationError`, and the internal route policy rendered its message into a 500 body, so raw SQL reached clients. The v2 envelope already scrubbed the same failures; internal routes did not. `messageForOrchestrationError` already encoded the rule and two sites honored it. The three that hand-rolled it disagreed, and `workflow-vfs` disagreed with itself: it defaulted the code with `?? 'internal'` but compared the raw `errorCode` against `'internal'`, so an uncoded failure was classified internal and still rendered its own message. Pair the two in `throwOrchestrationFailure` so a code and its message cannot disagree, and scrub at the internal route boundary as well, matching v2 — no call site authors a curated `internal` message, so nothing legitimate is masked, and site N+1 cannot reopen this by forgetting the rule. --- .../api/server/routes/internal-json-route.ts | 11 ++- apps/sim/lib/core/orchestration/types.test.ts | 69 ++++++++++++++++++- apps/sim/lib/core/orchestration/types.ts | 19 +++++ apps/sim/lib/knowledge/application/folders.ts | 11 +-- .../application/transition-result.test.ts | 39 +++++++++++ .../application/transition-result.ts | 7 +- .../workflows/application/workflow-folders.ts | 8 +-- .../lib/workflows/application/workflow-vfs.ts | 8 +-- 8 files changed, 150 insertions(+), 22 deletions(-) create mode 100644 apps/sim/lib/workflows/application/transition-result.test.ts diff --git a/apps/sim/lib/api/server/routes/internal-json-route.ts b/apps/sim/lib/api/server/routes/internal-json-route.ts index ae198a2684d..599fb2cbeee 100644 --- a/apps/sim/lib/api/server/routes/internal-json-route.ts +++ b/apps/sim/lib/api/server/routes/internal-json-route.ts @@ -33,7 +33,11 @@ import { InvalidInternalDelegationBindingError, } from '@/lib/auth/internal-delegation' import type { ApplicationOperation, OperationUseCase } from '@/lib/core/application' -import { asOrchestrationError, statusForOrchestrationError } from '@/lib/core/orchestration/types' +import { + asOrchestrationError, + messageForOrchestrationError, + statusForOrchestrationError, +} from '@/lib/core/orchestration/types' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' export class InternalUnauthenticatedError extends Error { @@ -142,7 +146,10 @@ export const internalOrchestrationErrorPolicy: InternalErrorPolicy = { const classified = asOrchestrationError(error) if (!classified) return null return internalErrorResponse(statusForOrchestrationError(classified.code), { - error: classified.message, + error: messageForOrchestrationError( + { error: classified.message, errorCode: classified.code }, + 'Internal server error' + ), }) }, unhandled() { diff --git a/apps/sim/lib/core/orchestration/types.test.ts b/apps/sim/lib/core/orchestration/types.test.ts index af8104841a1..843a6b7ad2b 100644 --- a/apps/sim/lib/core/orchestration/types.test.ts +++ b/apps/sim/lib/core/orchestration/types.test.ts @@ -2,7 +2,15 @@ * @vitest-environment node */ import { describe, expect, it } from 'vitest' -import { statusForOrchestrationError } from '@/lib/core/orchestration/types' +import { + messageForOrchestrationError, + OrchestrationError, + statusForOrchestrationError, + throwOrchestrationFailure, +} from '@/lib/core/orchestration/types' + +const RAW_DRIVER_MESSAGE = + 'insert into "workflow" ("id") values ($1) - duplicate key value violates unique constraint "workflow_pkey"' describe('statusForOrchestrationError', () => { it.each([ @@ -15,3 +23,62 @@ describe('statusForOrchestrationError', () => { expect(statusForOrchestrationError(code)).toBe(expected) }) }) + +describe('messageForOrchestrationError', () => { + it('withholds the message of an explicitly internal failure', () => { + expect( + messageForOrchestrationError( + { error: RAW_DRIVER_MESSAGE, errorCode: 'internal' }, + 'Failed to create workflow' + ) + ).toBe('Failed to create workflow') + }) + + it('withholds the message of a failure carrying no code', () => { + expect( + messageForOrchestrationError({ error: RAW_DRIVER_MESSAGE }, 'Failed to create workflow') + ).toBe('Failed to create workflow') + }) + + it('returns a classified failure message to the caller', () => { + expect( + messageForOrchestrationError( + { error: 'Workflow name is already taken', errorCode: 'conflict' }, + 'Failed to create workflow' + ) + ).toBe('Workflow name is already taken') + }) + + it('falls back when a classified failure carries no message', () => { + expect( + messageForOrchestrationError({ errorCode: 'conflict' }, 'Failed to create workflow') + ).toBe('Failed to create workflow') + }) +}) + +describe('throwOrchestrationFailure', () => { + it('classifies an uncoded failure as internal without rendering its message', () => { + try { + throwOrchestrationFailure({ error: RAW_DRIVER_MESSAGE }, 'Failed to update workflow') + expect.unreachable('expected throwOrchestrationFailure to throw') + } catch (error) { + expect(error).toBeInstanceOf(OrchestrationError) + expect((error as OrchestrationError).code).toBe('internal') + expect((error as OrchestrationError).message).toBe('Failed to update workflow') + } + }) + + it('preserves the code and message of a classified failure', () => { + try { + throwOrchestrationFailure( + { error: 'No such workflow', errorCode: 'not_found' }, + 'Failed to delete workflow' + ) + expect.unreachable('expected throwOrchestrationFailure to throw') + } catch (error) { + expect(error).toBeInstanceOf(OrchestrationError) + expect((error as OrchestrationError).code).toBe('not_found') + expect((error as OrchestrationError).message).toBe('No such workflow') + } + }) +}) diff --git a/apps/sim/lib/core/orchestration/types.ts b/apps/sim/lib/core/orchestration/types.ts index eb42af6bf65..f46c0a4ebbd 100644 --- a/apps/sim/lib/core/orchestration/types.ts +++ b/apps/sim/lib/core/orchestration/types.ts @@ -72,6 +72,25 @@ export class OrchestrationError extends Error { } } +/** + * Rethrows a failed orchestration result as its classified {@link OrchestrationError}. + * + * Pairs the code with the message {@link messageForOrchestrationError} permits for + * it, so the two can never disagree. Hand-rolling that pair is what let raw driver + * text reach clients: a site that defaulted the code with `?? 'internal'` but then + * compared the *raw* `errorCode` against `'internal'` classified an uncoded failure + * as internal while still rendering its own message. + */ +export function throwOrchestrationFailure( + result: { error?: string; errorCode?: OrchestrationErrorCode }, + fallback: string +): never { + throw new OrchestrationError( + result.errorCode ?? 'internal', + messageForOrchestrationError(result, fallback) + ) +} + /** * The {@link OrchestrationError} in `error`'s cause chain, or `null` when the * failure is not a classified one. diff --git a/apps/sim/lib/knowledge/application/folders.ts b/apps/sim/lib/knowledge/application/folders.ts index a3f530c25bb..b12a59c7eec 100644 --- a/apps/sim/lib/knowledge/application/folders.ts +++ b/apps/sim/lib/knowledge/application/folders.ts @@ -1,6 +1,10 @@ import { AuditAction, AuditResourceType } from '@sim/audit' import type { folder } from '@sim/db/schema' -import { OrchestrationError, type OrchestrationErrorCode } from '@/lib/core/orchestration/types' +import { + OrchestrationError, + type OrchestrationErrorCode, + throwOrchestrationFailure, +} from '@/lib/core/orchestration/types' import { createFolderAtPath, deleteFolderByPath, @@ -49,10 +53,7 @@ export interface DeleteKnowledgeFolderInput { } function throwFolderFailure(result: { error?: string; errorCode?: OrchestrationErrorCode }): never { - throw new OrchestrationError( - result.errorCode ?? 'internal', - result.error ?? 'Folder operation failed' - ) + throwOrchestrationFailure(result, 'Folder operation failed') } export const listKnowledgeFolders = defineAuthorizedKnowledgeUseCase({ diff --git a/apps/sim/lib/workflows/application/transition-result.test.ts b/apps/sim/lib/workflows/application/transition-result.test.ts new file mode 100644 index 00000000000..b4990c4d877 --- /dev/null +++ b/apps/sim/lib/workflows/application/transition-result.test.ts @@ -0,0 +1,39 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { OrchestrationError } from '@/lib/core/orchestration/types' +import { requireWorkflowTransition } from '@/lib/workflows/application/transition-result' + +describe('requireWorkflowTransition', () => { + it('returns without throwing for a successful transition', () => { + expect(() => requireWorkflowTransition({ success: true }, 'Failed')).not.toThrow() + }) + + it('withholds the raw message a failed lifecycle transition carries', () => { + expect(() => + requireWorkflowTransition( + { + success: false, + error: 'duplicate key value violates unique constraint "workflow_pkey"', + errorCode: 'internal', + }, + 'Failed to create workflow' + ) + ).toThrow('Failed to create workflow') + }) + + it('preserves a classified failure so the route maps the right status', () => { + try { + requireWorkflowTransition( + { success: false, error: 'No such workflow', errorCode: 'not_found' }, + 'Failed to delete workflow' + ) + expect.unreachable('expected requireWorkflowTransition to throw') + } catch (error) { + expect(error).toBeInstanceOf(OrchestrationError) + expect((error as OrchestrationError).code).toBe('not_found') + expect((error as OrchestrationError).message).toBe('No such workflow') + } + }) +}) diff --git a/apps/sim/lib/workflows/application/transition-result.ts b/apps/sim/lib/workflows/application/transition-result.ts index 8ad59eceafd..9a4f6da3b2a 100644 --- a/apps/sim/lib/workflows/application/transition-result.ts +++ b/apps/sim/lib/workflows/application/transition-result.ts @@ -1,8 +1,11 @@ -import { OrchestrationError, type OrchestrationErrorCode } from '@/lib/core/orchestration/types' +import { + type OrchestrationErrorCode, + throwOrchestrationFailure, +} from '@/lib/core/orchestration/types' export function requireWorkflowTransition< T extends { success: boolean; error?: string; errorCode?: OrchestrationErrorCode }, >(result: T, fallbackMessage: string): asserts result is T & { success: true } { if (result.success) return - throw new OrchestrationError(result.errorCode ?? 'internal', result.error ?? fallbackMessage) + throwOrchestrationFailure(result, fallbackMessage) } diff --git a/apps/sim/lib/workflows/application/workflow-folders.ts b/apps/sim/lib/workflows/application/workflow-folders.ts index ae1bd746e89..87dc4c029cd 100644 --- a/apps/sim/lib/workflows/application/workflow-folders.ts +++ b/apps/sim/lib/workflows/application/workflow-folders.ts @@ -2,7 +2,7 @@ import { AuditAction, AuditResourceType } from '@sim/audit' import { resolvePrincipalAttribution } from '@sim/auth/principal' import type { folder } from '@sim/db/schema' import type { OrchestrationErrorCode } from '@/lib/core/orchestration/types' -import { OrchestrationError } from '@/lib/core/orchestration/types' +import { OrchestrationError, throwOrchestrationFailure } from '@/lib/core/orchestration/types' import { MAX_FOLDERS_PER_WORKSPACE } from '@/lib/folders/constants' import { withFolderTreeLock } from '@/lib/folders/locks' import { @@ -73,11 +73,7 @@ function throwFolderMutationFailure(result: { error?: string errorCode?: OrchestrationErrorCode }): never { - const code = result.errorCode ?? 'internal' - throw new OrchestrationError( - code, - code === 'internal' ? 'Internal server error' : (result.error ?? 'Folder mutation failed') - ) + throwOrchestrationFailure(result, 'Internal server error') } export async function resolveWorkflowFolderPath( diff --git a/apps/sim/lib/workflows/application/workflow-vfs.ts b/apps/sim/lib/workflows/application/workflow-vfs.ts index deb32724d46..3d4ab0a6acc 100644 --- a/apps/sim/lib/workflows/application/workflow-vfs.ts +++ b/apps/sim/lib/workflows/application/workflow-vfs.ts @@ -13,6 +13,7 @@ import { asOrchestrationError, OrchestrationError, type OrchestrationErrorCode, + throwOrchestrationFailure, } from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' import { @@ -246,12 +247,7 @@ function resolveWorkflowSources( } function throwFolderFailure(result: { error?: string; errorCode?: OrchestrationErrorCode }): never { - throw new OrchestrationError( - result.errorCode ?? 'internal', - result.errorCode === 'internal' - ? 'Workflow folder mutation failed' - : (result.error ?? 'Folder mutation failed') - ) + throwOrchestrationFailure(result, 'Workflow folder mutation failed') } async function reloadFolderIndex(state: WorkflowVfsIndexState, workspaceId: string): Promise {