Skip to content

Commit 9e71105

Browse files
committed
fix(api): restore v1 table error-response parity and stop internal message leak
The v1 table routes were rewritten to consume `lib/table/orchestration` results, and two response behaviors drifted from what the live API returned. Information disclosure: an unclassified failure's `outcome.error` carries whatever text the fault happened to have. Drizzle wraps a throw raised inside a transaction in an error whose own message is the failed statement and its bound parameters, so `DELETE /api/v1/tables/{tableId}` and `DELETE /api/v1/tables/{tableId}/rows/{rowId}` returned that verbatim in the 500 body to any API-key holder. Previously these returned a fixed generic string. Lost `lock` field: the 423 body used to be `{ error, lock }`. The delete, row-delete, and column-update routes (v1 and internal) dropped the lock kind the orchestration result already computes, leaving clients unable to tell which lock to clear. Both are fixed at one altitude: `orchestrationOutcomeErrorResponse` in `app/api/table/utils.ts` is now the only way a table route projects an orchestration failure onto the wire. It renders the route's fallback for an unclassified failure and the real message for a classified one (validation, not-found, conflict, locked keep their specific text), and carries `lock` on a 423. A future route cannot reintroduce either bug by hand-spelling the body. Duplicate table names on `POST /api/v1/tables` keep answering 409 rather than reverting to the previous 400. 409 is the correct semantic, and every other v1 duplicate-name surface (knowledge, files, workflow import) already answers 409; the tables 400 was the outlier. v1 tables appears in no published OpenAPI document and no in-repo client branches on the status, so the compatibility cost is limited to a caller matching 400 specifically for a name collision.
1 parent fe432f9 commit 9e71105

11 files changed

Lines changed: 409 additions & 47 deletions

File tree

apps/sim/app/api/table/[tableId]/columns/route.test.ts

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
*/
1111
import { hybridAuthMockFns } from '@sim/testing'
1212
import { getErrorMessage } from '@sim/utils/errors'
13-
import { NextRequest } from 'next/server'
13+
import { NextRequest, NextResponse } from 'next/server'
1414
import { beforeEach, describe, expect, it, vi } from 'vitest'
1515

1616
const {
@@ -53,11 +53,24 @@ vi.mock('@/app/api/table/utils', () => ({
5353
accessError: () => new Response('denied', { status: 403 }),
5454
checkAccess: mockCheckAccess,
5555
normalizeColumn: (c: unknown) => c,
56+
orchestrationOutcomeErrorResponse: (
57+
outcome: { error?: string; errorCode?: OrchestrationErrorCode },
58+
fallback: string
59+
) =>
60+
NextResponse.json(
61+
{ error: messageForOrchestrationError(outcome, fallback) },
62+
{ status: statusForOrchestrationError(outcome.errorCode) }
63+
),
5664
rootErrorMessage: (e: unknown) => getErrorMessage(e),
5765
tableLockErrorResponse: () => null,
5866
}))
5967

60-
import { OrchestrationError } from '@/lib/core/orchestration/types'
68+
import {
69+
messageForOrchestrationError,
70+
OrchestrationError,
71+
type OrchestrationErrorCode,
72+
statusForOrchestrationError,
73+
} from '@/lib/core/orchestration/types'
6174
import { PATCH } from '@/app/api/table/[tableId]/columns/route'
6275

6376
const WORKSPACE_ID = '11111111-1111-4111-8111-111111111111'

apps/sim/app/api/table/[tableId]/columns/route.ts

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import {
88
import { parseRequest } from '@/lib/api/server'
99
import { isZodError, validationErrorResponse } from '@/lib/api/server/validation'
1010
import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid'
11-
import { statusForOrchestrationError } from '@/lib/core/orchestration/types'
1211
import { generateRequestId } from '@/lib/core/utils/request'
1312
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1413
import { addTableColumn, deleteColumn } from '@/lib/table'
@@ -18,6 +17,7 @@ import {
1817
accessError,
1918
checkAccess,
2019
normalizeColumn,
20+
orchestrationOutcomeErrorResponse,
2121
rootErrorMessage,
2222
tableLockErrorResponse,
2323
} from '@/app/api/table/utils'
@@ -122,10 +122,7 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
122122
request,
123123
})
124124
if (!outcome.success || !outcome.table) {
125-
return NextResponse.json(
126-
{ error: outcome.error ?? 'Failed to update column' },
127-
{ status: statusForOrchestrationError(outcome.errorCode) }
128-
)
125+
return orchestrationOutcomeErrorResponse(outcome, 'Failed to update column')
129126
}
130127

131128
// Live-collab: tell open viewers the change landed so they refetch.

apps/sim/app/api/table/[tableId]/route.ts

Lines changed: 9 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import { getTableQuerySchema, updateTableContract } from '@/lib/api/contracts/ta
55
import { isZodError, parseRequest, validationErrorResponse } from '@/lib/api/server/validation'
66
import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid'
77
import { isFeatureEnabled } from '@/lib/core/config/feature-flags'
8-
import { statusForOrchestrationError } from '@/lib/core/orchestration/types'
98
import { generateRequestId } from '@/lib/core/utils/request'
109
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1110
import { findActiveFolder } from '@/lib/folders/queries'
@@ -24,6 +23,7 @@ import {
2423
accessError,
2524
checkAccess,
2625
normalizeColumn,
26+
orchestrationOutcomeErrorResponse,
2727
tableLockErrorResponse,
2828
} from '@/app/api/table/utils'
2929

@@ -187,10 +187,7 @@ export const PATCH = withRouteHandler(
187187
request,
188188
})
189189
if (!lockOutcome.success) {
190-
return NextResponse.json(
191-
{ error: lockOutcome.error ?? 'Failed to update table locks' },
192-
{ status: statusForOrchestrationError(lockOutcome.errorCode) }
193-
)
190+
return orchestrationOutcomeErrorResponse(lockOutcome, 'Failed to update table locks')
194191
}
195192
}
196193

@@ -203,10 +200,7 @@ export const PATCH = withRouteHandler(
203200
request,
204201
})
205202
if (!renameOutcome.success) {
206-
return NextResponse.json(
207-
{ error: renameOutcome.error ?? 'Failed to rename table' },
208-
{ status: statusForOrchestrationError(renameOutcome.errorCode) }
209-
)
203+
return orchestrationOutcomeErrorResponse(renameOutcome, 'Failed to rename table')
210204
}
211205
}
212206

@@ -229,11 +223,11 @@ export const PATCH = withRouteHandler(
229223
request,
230224
})
231225
if (!moveOutcome.success) {
232-
return NextResponse.json(
233-
{
234-
error: moveOutcome.errorCode === 'not_found' ? 'Table not found' : moveOutcome.error,
235-
},
236-
{ status: statusForOrchestrationError(moveOutcome.errorCode) }
226+
return orchestrationOutcomeErrorResponse(
227+
moveOutcome.errorCode === 'not_found'
228+
? { ...moveOutcome, error: 'Table not found' }
229+
: moveOutcome,
230+
'Failed to move table'
237231
)
238232
}
239233
}
@@ -302,10 +296,7 @@ export const DELETE = withRouteHandler(
302296
request,
303297
})
304298
if (!outcome.success) {
305-
return NextResponse.json(
306-
{ error: outcome.error ?? 'Failed to delete table' },
307-
{ status: statusForOrchestrationError(outcome.errorCode) }
308-
)
299+
return orchestrationOutcomeErrorResponse(outcome, 'Failed to delete table')
309300
}
310301

311302
return NextResponse.json({

apps/sim/app/api/table/[tableId]/rows/[rowId]/route.ts

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import {
1010
} from '@/lib/api/contracts/tables'
1111
import { isZodError, parseRequest, validationErrorResponse } from '@/lib/api/server/validation'
1212
import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid'
13-
import { statusForOrchestrationError } from '@/lib/core/orchestration/types'
1413
import { generateRequestId } from '@/lib/core/utils/request'
1514
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1615
import type { RowData, TableSchema } from '@/lib/table'
@@ -27,6 +26,7 @@ import {
2726
accessError,
2827
checkAccess,
2928
orchestrationErrorResponse,
29+
orchestrationOutcomeErrorResponse,
3030
rowWriteErrorResponse,
3131
tableLockErrorResponse,
3232
} from '@/app/api/table/utils'
@@ -248,10 +248,7 @@ export const DELETE = withRouteHandler(async (request: NextRequest, context: Row
248248

249249
const outcome = await performDeleteTableRow({ table, rowId, requestId })
250250
if (!outcome.success) {
251-
return NextResponse.json(
252-
{ error: outcome.error ?? 'Failed to delete row' },
253-
{ status: statusForOrchestrationError(outcome.errorCode) }
254-
)
251+
return orchestrationOutcomeErrorResponse(outcome, 'Failed to delete row')
255252
}
256253

257254
// Live-collab: tell open viewers the change landed so they refetch.

apps/sim/app/api/table/utils.test.ts

Lines changed: 63 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,12 @@ import { describe, expect, it } from 'vitest'
55
import { OrchestrationError } from '@/lib/core/orchestration/types'
66
import { TableRowLimitError } from '@/lib/table/billing'
77
import type { ColumnDefinition } from '@/lib/table/types'
8-
import { rootErrorMessage, rowWriteErrorResponse, tableFilterError } from '@/app/api/table/utils'
8+
import {
9+
orchestrationOutcomeErrorResponse,
10+
rootErrorMessage,
11+
rowWriteErrorResponse,
12+
tableFilterError,
13+
} from '@/app/api/table/utils'
914

1015
/** Mimics drizzle's DrizzleQueryError: message is the failed SQL, real error on `cause`. */
1116
function wrapLikeDrizzle(cause: Error): Error {
@@ -117,3 +122,60 @@ describe('tableFilterError', () => {
117122
expect(tableFilterError({ col_status: { $regex: 'x' } } as never, columns)?.status).toBe(400)
118123
})
119124
})
125+
126+
describe('orchestrationOutcomeErrorResponse', () => {
127+
/**
128+
* Shaped like a driver fault surfacing verbatim — a statement plus its bound
129+
* parameters — so the assertion proves none of it reaches the response body.
130+
*/
131+
const leakyMessage =
132+
'Failed query: delete from "user_table" where "user_table"."id" = $1 params: tbl-1'
133+
134+
it('replaces an unclassified failure message with the fallback', async () => {
135+
const response = orchestrationOutcomeErrorResponse(
136+
{ success: false, error: leakyMessage, errorCode: 'internal' },
137+
'Failed to delete table'
138+
)
139+
140+
expect(response.status).toBe(500)
141+
const body = await response.json()
142+
expect(body).toEqual({ error: 'Failed to delete table' })
143+
expect(JSON.stringify(body)).not.toContain('Failed query')
144+
expect(JSON.stringify(body)).not.toContain('params:')
145+
})
146+
147+
it('replaces an unclassified failure with no error code too', async () => {
148+
const response = orchestrationOutcomeErrorResponse(
149+
{ error: leakyMessage },
150+
'Failed to delete table'
151+
)
152+
153+
expect(response.status).toBe(500)
154+
expect(await response.json()).toEqual({ error: 'Failed to delete table' })
155+
})
156+
157+
it('keeps the message of a classified failure', async () => {
158+
const response = orchestrationOutcomeErrorResponse(
159+
{ error: 'A table named "Orders" already exists in this workspace', errorCode: 'conflict' },
160+
'Failed to rename table'
161+
)
162+
163+
expect(response.status).toBe(409)
164+
expect(await response.json()).toEqual({
165+
error: 'A table named "Orders" already exists in this workspace',
166+
})
167+
})
168+
169+
it('carries the rejecting lock kind on a 423', async () => {
170+
const response = orchestrationOutcomeErrorResponse(
171+
{ error: 'Table is locked against deletion', errorCode: 'locked', lock: 'delete' },
172+
'Failed to delete table'
173+
)
174+
175+
expect(response.status).toBe(423)
176+
expect(await response.json()).toEqual({
177+
error: 'Table is locked against deletion',
178+
lock: 'delete',
179+
})
180+
})
181+
})

apps/sim/app/api/table/utils.ts

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,12 @@ import {
88
updateTableColumnBodySchema,
99
} from '@/lib/api/contracts/tables'
1010
import { isFeatureEnabled } from '@/lib/core/config/feature-flags'
11-
import { asOrchestrationError, statusForOrchestrationError } from '@/lib/core/orchestration/types'
11+
import {
12+
asOrchestrationError,
13+
messageForOrchestrationError,
14+
type OrchestrationErrorCode,
15+
statusForOrchestrationError,
16+
} from '@/lib/core/orchestration/types'
1217
import type { MultipartError } from '@/lib/core/utils/multipart'
1318
import type { ColumnDefinition, Filter, TableDefinition, TablePredicate } from '@/lib/table'
1419
import { buildFilterClause, getTableById, TableQueryValidationError } from '@/lib/table'
@@ -17,6 +22,7 @@ import { USER_TABLE_ROWS_SQL_NAME } from '@/lib/table/constants'
1722
import { TableLockedError } from '@/lib/table/mutation-locks'
1823
import { isTablePredicate } from '@/lib/table/query-builder/converters'
1924
import { validateStoragePredicate } from '@/lib/table/query-builder/validate'
25+
import type { TableLockKind } from '@/lib/table/types'
2026
import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils'
2127
import { getWorkspaceOrganizationId } from '@/lib/workspaces/utils'
2228

@@ -132,6 +138,45 @@ export function orchestrationErrorResponse(error: unknown): NextResponse | null
132138
)
133139
}
134140

141+
/**
142+
* The failure half of a `lib/table/orchestration` result. Every `perform*`
143+
* function returns this shape, so one projection serves all of them.
144+
*/
145+
export interface TableOrchestrationFailure {
146+
error?: string
147+
errorCode?: OrchestrationErrorCode
148+
/** Which lock rejected the write. Set only when `errorCode` is `'locked'`. */
149+
lock?: TableLockKind
150+
}
151+
152+
/**
153+
* Projects an orchestration failure RESULT onto its HTTP response, the
154+
* counterpart of {@link orchestrationErrorResponse} for the functions that
155+
* return a failure instead of throwing one.
156+
*
157+
* Every table route must go through this rather than reading `outcome.error`
158+
* itself, for two reasons the per-route spellings kept getting wrong:
159+
*
160+
* - An unclassified failure carries whatever text the fault happened to have —
161+
* a driver's failed SQL and its bound parameters — so it renders `fallback`
162+
* instead. Only a classified, caller-fixable failure keeps its own message.
163+
* - A `'locked'` failure answers 423 with `{ error, lock }`. The lock kind is
164+
* the only thing that tells a client which lock to clear, and it is computed
165+
* by every `perform*` function already.
166+
*/
167+
export function orchestrationOutcomeErrorResponse(
168+
outcome: TableOrchestrationFailure,
169+
fallback: string
170+
): NextResponse {
171+
return NextResponse.json(
172+
{
173+
error: messageForOrchestrationError(outcome, fallback),
174+
...(outcome.lock ? { lock: outcome.lock } : {}),
175+
},
176+
{ status: statusForOrchestrationError(outcome.errorCode) }
177+
)
178+
}
179+
135180
/**
136181
* {@link orchestrationErrorResponse} under the name the row-write routes call
137182
* it by. Row writes have no classification rules of their own any more.

apps/sim/app/api/v1/tables/[tableId]/columns/route.ts

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import {
77
v1UpdateTableColumnContract,
88
} from '@/lib/api/contracts/v1/tables'
99
import { parseRequest } from '@/lib/api/server'
10-
import { statusForOrchestrationError } from '@/lib/core/orchestration/types'
1110
import { generateRequestId } from '@/lib/core/utils/request'
1211
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1312
import { addTableColumn, deleteColumn } from '@/lib/table'
@@ -18,6 +17,7 @@ import {
1817
checkAccess,
1918
normalizeColumn,
2019
orchestrationErrorResponse,
20+
orchestrationOutcomeErrorResponse,
2121
tableLockErrorResponse,
2222
} from '@/app/api/table/utils'
2323
import {
@@ -143,10 +143,7 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
143143
request,
144144
})
145145
if (!outcome.success || !outcome.table) {
146-
return NextResponse.json(
147-
{ error: outcome.error ?? 'Failed to update column' },
148-
{ status: statusForOrchestrationError(outcome.errorCode) }
149-
)
146+
return orchestrationOutcomeErrorResponse(outcome, 'Failed to update column')
150147
}
151148

152149
// Live-collab: tell open viewers the change landed so they refetch.

0 commit comments

Comments
 (0)