Skip to content

Commit e2b7335

Browse files
authored
fix(v2): stop telling callers something the server did not do (#6676)
A works-as-advertised sweep of the v2 surface found one defect class in five places: input is validated for shape, then its meaning is re-derived independently by each consumer — so a filter compiles differently than it validated, or a write commits and the response then reports failure. Knowledge tag filters were validated once and re-parsed three times. The document list read `Number()`, search read `parseFloat()`; the list matched booleans case-insensitively, search compared against the literal `'true'`; the list escaped LIKE metacharacters, search did not; and the date pattern was tested against the untrimmed string the validator had already trimmed. Two of those paths dropped the predicate entirely and answered 200 with the whole knowledge base — on a billed endpoint. Values are now coerced once, where the resolved field type is known, and both builders consume the result. A builder that cannot compile an already-validated filter now raises instead of silently widening the result set. `PUT /api/v2/secrets/{name}` with `scope: personal` committed the secret and then answered 500, because a user-global write was reported through a workspace-scoped mirror lookup, and an org admin's inherited access has no `permissions` row for the fan-out to find. The personal path no longer decides success from a per-workspace mirror. The `workspaceId` descriptions said a personal secret lives in one workspace; it does not, and they now say so. A custom tool could be stored with a schema the read path cannot serialize — `POST /workflows/import` and Copilot both wrote through name-only checks — so one row made the whole workspace list 500, and a title-only PATCH committed, audited, then reported failure. Every write now passes the same guard the response schema is derived from. `POST /api/v2/tables` accepted `workflowGroupId` on an initial column. Nothing can populate it legitimately, and it made every later column-add and group-add fail with no way to clear it. The key is refused at the boundary, and `createTable` now runs the invariant every later mutation already runs, closing the internal and v1 ingresses too. Those invariants moved to a leaf module: reaching them through `workflow-columns` pulled the executable tool registry into the tables page graph, taking it from 1,767 modules to 6,999. An out-of-range upload part number answered 500 rather than the 400 its published contract promises, because the throw happened above the route's try/catch and was not an `HttpError`.
1 parent 0758df3 commit e2b7335

33 files changed

Lines changed: 1518 additions & 344 deletions

File tree

apps/docs/openapi-v2-resources.json

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1965,22 +1965,22 @@
19651965
"name": "workspaceId",
19661966
"in": "query",
19671967
"required": true,
1968-
"description": "Workspace in which the secret is available.",
1968+
"description": "Workspace the request is authorized against. A workspace secret is deleted from it; a personal secret is deleted for the caller in all of their workspaces.",
19691969
"schema": {
19701970
"type": "string",
19711971
"minLength": 1,
1972-
"description": "Workspace in which the secret is available."
1972+
"description": "Workspace the request is authorized against. A workspace secret is deleted from it; a personal secret is deleted for the caller in all of their workspaces."
19731973
}
19741974
},
19751975
{
19761976
"name": "scope",
19771977
"in": "query",
19781978
"required": true,
1979-
"description": "Whether the secret belongs to the workspace or the caller.",
1979+
"description": "Whether the secret belongs to the workspace or to the caller. A personal secret belongs to the caller across every workspace, not to one workspace.",
19801980
"schema": {
19811981
"type": "string",
19821982
"enum": ["workspace", "personal"],
1983-
"description": "Whether the secret belongs to the workspace or the caller."
1983+
"description": "Whether the secret belongs to the workspace or to the caller. A personal secret belongs to the caller across every workspace, not to one workspace."
19841984
}
19851985
}
19861986
],
@@ -4088,7 +4088,7 @@
40884088
"scope": {
40894089
"type": "string",
40904090
"enum": ["workspace", "personal"],
4091-
"description": "Whether the secret belongs to the workspace or the caller."
4091+
"description": "Whether the secret belongs to the workspace or to the caller. A personal secret belongs to the caller across every workspace, not to one workspace."
40924092
},
40934093
"role": {
40944094
"type": "string",
@@ -4184,12 +4184,12 @@
41844184
"workspaceId": {
41854185
"type": "string",
41864186
"minLength": 1,
4187-
"description": "Workspace in which the secret is available."
4187+
"description": "Workspace the request is authorized against. A workspace secret is written to it; a personal secret is written to the caller and is available in all of their workspaces."
41884188
},
41894189
"scope": {
41904190
"type": "string",
41914191
"enum": ["workspace", "personal"],
4192-
"description": "Whether the secret belongs to the workspace or the caller."
4192+
"description": "Whether the secret belongs to the workspace or to the caller. A personal secret belongs to the caller across every workspace, not to one workspace."
41934193
},
41944194
"value": {
41954195
"type": "string",
@@ -4224,7 +4224,7 @@
42244224
"scope": {
42254225
"type": "string",
42264226
"enum": ["workspace", "personal"],
4227-
"description": "Whether the secret belongs to the workspace or the caller."
4227+
"description": "Whether the secret belongs to the workspace or to the caller. A personal secret belongs to the caller across every workspace, not to one workspace."
42284228
},
42294229
"deleted": {
42304230
"type": "boolean",

apps/docs/openapi-v2-tables.json

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4370,10 +4370,6 @@
43704370
"description": "ISO 4217 code for currency columns.",
43714371
"type": "string",
43724372
"pattern": "^[A-Za-z]{3}$"
4373-
},
4374-
"workflowGroupId": {
4375-
"description": "Workflow group initially associated with the column.",
4376-
"type": "string"
43774373
}
43784374
},
43794375
"required": ["name", "type"],

apps/sim/app/api/v2/uploads/[uploadId]/parts/[partNumber]/route.test.ts

Lines changed: 49 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ vi.mock('@/lib/uploads/upload-session/service', () => ({
2929
verifyUploadSessionToken: mockVerifyUploadSessionToken,
3030
}))
3131

32+
import { OrchestrationError } from '@/lib/core/orchestration/types'
3233
import { PUT } from '@/app/api/v2/uploads/[uploadId]/parts/[partNumber]/route'
3334

3435
const SESSION = {
@@ -37,6 +38,8 @@ const SESSION = {
3738
method: 'multipart',
3839
status: 'uploading',
3940
expiresAt: new Date('2999-01-01T00:00:00.000Z'),
41+
partSize: 3,
42+
partCount: 2,
4043
} as const
4144

4245
describe('PUT /api/v2/uploads/[uploadId]/parts/[partNumber]', () => {
@@ -84,6 +87,41 @@ describe('PUT /api/v2/uploads/[uploadId]/parts/[partNumber]', () => {
8487
expect(mockWriteLocalMultipartPart).not.toHaveBeenCalled()
8588
})
8689

90+
/**
91+
* The part number is a path segment of a session-scoped signed URL, so any
92+
* holder of a legitimate part URL can address a part the session does not
93+
* have. `expectedUploadPartSize` classifies that as a validation failure;
94+
* `service.test.ts` pins that classification on the real implementation,
95+
* which this suite mocks away.
96+
*/
97+
it('maps an out-of-range part number to the documented 400', async () => {
98+
mockExpectedUploadPartSize.mockImplementation(() => {
99+
throw new OrchestrationError('validation', 'partNumber must be between 1 and 2')
100+
})
101+
102+
const response = await request({ partNumber: '99' })
103+
104+
expect(response.status).toBe(400)
105+
await expect(response.json()).resolves.toEqual({
106+
error: { code: 'BAD_REQUEST', message: 'partNumber must be between 1 and 2' },
107+
})
108+
expect(mockWriteLocalMultipartPart).not.toHaveBeenCalled()
109+
})
110+
111+
it('still renders an unclassified part-size failure as a generic 500', async () => {
112+
mockExpectedUploadPartSize.mockImplementation(() => {
113+
throw new Error('unexpected')
114+
})
115+
116+
const response = await request()
117+
118+
expect(response.status).toBe(500)
119+
await expect(response.json()).resolves.toEqual({
120+
error: { code: 'INTERNAL_ERROR', message: 'Internal server error' },
121+
})
122+
expect(mockWriteLocalMultipartPart).not.toHaveBeenCalled()
123+
})
124+
87125
it('rejects expired upload sessions before writing the part', async () => {
88126
mockVerifyUploadSessionToken.mockReturnValue({
89127
...SESSION,
@@ -101,17 +139,21 @@ describe('PUT /api/v2/uploads/[uploadId]/parts/[partNumber]', () => {
101139
})
102140
})
103141

104-
function request(options?: { contentLength?: string | null }) {
142+
function request(options?: { contentLength?: string | null; partNumber?: string }) {
105143
const headers = new Headers({ 'Content-Type': 'application/octet-stream' })
106144
if (options?.contentLength !== null) {
107145
headers.set('Content-Length', options?.contentLength ?? '3')
108146
}
147+
const partNumber = options?.partNumber ?? '1'
109148
return PUT(
110-
new NextRequest('http://localhost:3000/api/v2/uploads/upload-1/parts/1?token=signed-token', {
111-
method: 'PUT',
112-
headers,
113-
body: new Uint8Array([1, 2, 3]),
114-
}),
115-
{ params: Promise.resolve({ uploadId: 'upload-1', partNumber: '1' }) }
149+
new NextRequest(
150+
`http://localhost:3000/api/v2/uploads/upload-1/parts/${partNumber}?token=signed-token`,
151+
{
152+
method: 'PUT',
153+
headers,
154+
body: new Uint8Array([1, 2, 3]),
155+
}
156+
),
157+
{ params: Promise.resolve({ uploadId: 'upload-1', partNumber }) }
116158
)
117159
}

apps/sim/app/api/v2/uploads/[uploadId]/parts/[partNumber]/route.ts

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,12 @@ import {
1212
type UploadSessionRecord,
1313
verifyUploadSessionToken,
1414
} from '@/lib/uploads/upload-session/service'
15-
import { v2Error, v2HttpError, v2UploadDataPlaneError } from '@/app/api/v2/lib/response'
15+
import {
16+
v2CaughtOrchestrationError,
17+
v2Error,
18+
v2HttpError,
19+
v2UploadDataPlaneError,
20+
} from '@/app/api/v2/lib/response'
1621

1722
interface LocalPartRouteParams {
1823
params: Promise<{ uploadId: string; partNumber: string }>
@@ -60,7 +65,18 @@ export const PUT = withRouteHandler(
6065
}
6166

6267
const { partNumber } = parsed.data.params
63-
const expectedSize = expectedUploadPartSize(session, partNumber)
68+
let expectedSize: number
69+
try {
70+
expectedSize = expectedUploadPartSize(session, partNumber)
71+
} catch (error) {
72+
// The part number is a path segment of a session-scoped signed URL, so a
73+
// caller can address a part this session does not have. That refusal is a
74+
// classified domain failure, and the data plane's generic 500 tail would
75+
// otherwise render it as an internal error.
76+
const classified = v2CaughtOrchestrationError(error)
77+
if (classified) return classified
78+
throw error
79+
}
6480
const contentLength = request.headers.get('content-length')
6581
if (contentLength !== null && Number(contentLength) !== expectedSize) {
6682
return v2Error('BAD_REQUEST', `Part ${partNumber} must contain exactly ${expectedSize} bytes`)

apps/sim/lib/api/contracts/v2/__tests__/tables.test.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,27 @@ describe('v2 table column contracts', () => {
5959
).toMatchObject({ success: true, data: { updates: { required: true } } })
6060
})
6161

62+
/**
63+
* v2 mints workflow group ids server-side and has no way to declare a group
64+
* on the create body, so any id a caller supplied would name a group that
65+
* does not exist. `createTable` does not check that, but every later schema
66+
* mutation does — accepting the field made the created table's columns and
67+
* groups permanently unaddable, with nothing on the update body able to clear
68+
* it.
69+
*/
70+
it('refuses a workflow group id on an initial column', () => {
71+
const result = v2CreateTableBodySchema.safeParse({
72+
workspaceId: WORKSPACE_ID,
73+
name: 'contacts',
74+
schema: {
75+
columns: [{ name: 'email', type: 'string', workflowGroupId: 'wfg_does_not_exist' }],
76+
},
77+
})
78+
79+
expect(result.success).toBe(false)
80+
expect(issueCodes(result.error?.issues ?? [])).toContain('unrecognized_keys')
81+
})
82+
6283
it('keeps required in table responses for existing stored schemas', () => {
6384
expect(
6485
v2ApiTableSchema.safeParse({

apps/sim/lib/api/contracts/v2/secrets.ts

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,9 @@ const SECRET_NAME_REGEX = /^[A-Za-z0-9_]+$/
1515

1616
export const v2SecretScopeSchema = z
1717
.enum(['workspace', 'personal'])
18-
.describe('Whether the secret belongs to the workspace or the caller.')
18+
.describe(
19+
'Whether the secret belongs to the workspace or to the caller. A personal secret belongs to the caller across every workspace, not to one workspace.'
20+
)
1921
export type V2SecretScope = z.output<typeof v2SecretScopeSchema>
2022

2123
export const v2SecretNameSchema = z
@@ -76,7 +78,9 @@ export type V2SecretParams = z.output<typeof v2SecretParamsSchema>
7678

7779
export const v2SetSecretBodySchema = z
7880
.object({
79-
workspaceId: workspaceIdSchema.describe('Workspace in which the secret is available.'),
81+
workspaceId: workspaceIdSchema.describe(
82+
'Workspace the request is authorized against. A workspace secret is written to it; a personal secret is written to the caller and is available in all of their workspaces.'
83+
),
8084
scope: v2SecretScopeSchema,
8185
value: z
8286
.string()
@@ -90,7 +94,9 @@ export type V2SetSecretBody = z.input<typeof v2SetSecretBodySchema>
9094

9195
export const v2DeleteSecretQuerySchema = z
9296
.object({
93-
workspaceId: workspaceIdSchema.describe('Workspace in which the secret is available.'),
97+
workspaceId: workspaceIdSchema.describe(
98+
'Workspace the request is authorized against. A workspace secret is deleted from it; a personal secret is deleted for the caller in all of their workspaces.'
99+
),
94100
scope: v2SecretScopeSchema,
95101
})
96102
.strict()

apps/sim/lib/api/contracts/v2/tables.ts

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -390,24 +390,23 @@ export const v2TableColumnInputSchema = z
390390
.strict()
391391
.superRefine(refineColumnOptions)
392392

393-
const v2InitialTableColumnInputSchema = z
394-
.object({
395-
...v2TableColumnInputShape,
396-
workflowGroupId: z
397-
.string()
398-
.optional()
399-
.describe('Workflow group initially associated with the column.'),
400-
})
401-
.strict()
402-
.superRefine(refineColumnOptions)
403-
393+
/**
394+
* Initial columns take the same shape as every other v2 column input.
395+
*
396+
* They deliberately cannot name a workflow group: v2 has no way to declare one
397+
* on this body and mints group ids server-side, so any id a caller supplied
398+
* would necessarily dangle. A dangling `workflowGroupId` is a schema invariant
399+
* violation, and `createTable` does not check it while every later schema
400+
* mutation does — so accepting the field made the table's own columns and
401+
* groups permanently unaddable, with no update body field able to clear it.
402+
*/
404403
export const v2CreateTableBodySchema = v1CreateTableBodySchema
405404
.omit({ folderId: true, schema: true })
406405
.extend({
407406
schema: z
408407
.object({
409408
columns: z
410-
.array(v2InitialTableColumnInputSchema)
409+
.array(v2TableColumnInputSchema)
411410
.min(1, 'Table must have at least one column')
412411
.max(
413412
TABLE_LIMITS.MAX_COLUMNS_PER_TABLE,

apps/sim/lib/credentials/environment.ts

Lines changed: 48 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { credential, credentialMember, permissions, workspace } from '@sim/db/sc
33
import { permissionSatisfies } from '@sim/platform-authz/workspace'
44
import { chunkArray } from '@sim/utils/helpers'
55
import { generateId } from '@sim/utils/id'
6-
import { and, eq, inArray, isNotNull, isNull, notInArray, or, sql } from 'drizzle-orm'
6+
import { and, asc, eq, inArray, isNotNull, isNull, notInArray, or, sql } from 'drizzle-orm'
77
import { acquireUserBillingIdentityLock } from '@/lib/billing/organizations/billing-identity-lock'
88
import type { DbOrTx } from '@/lib/db/types'
99
import {
@@ -552,7 +552,53 @@ export async function upsertPersonalEnvCredentialForUser(params: {
552552
await db.transaction(upsert)
553553
}
554554

555-
/** Deletes one caller-owned personal secret's credential metadata in every workspace. */
555+
export interface PersonalEnvCredentialMetadata {
556+
id: string
557+
createdAt: Date
558+
updatedAt: Date
559+
}
560+
561+
/**
562+
* Reads one caller-owned personal secret's credential metadata without scoping to
563+
* a workspace.
564+
*
565+
* A personal secret is stored once per user; the `env_personal` credential rows
566+
* are per-workspace mirrors, so a reader that needs the secret's own timestamps
567+
* must not require a mirror in one particular workspace. The earliest mirror is
568+
* the authoritative creation time — later ones are written when the caller joins
569+
* another workspace, long after the secret itself was created.
570+
*/
571+
export async function getPersonalEnvCredentialMetadata(params: {
572+
userId: string
573+
envKey: string
574+
}): Promise<PersonalEnvCredentialMetadata | null> {
575+
const [row] = await db
576+
.select({
577+
id: credential.id,
578+
createdAt: credential.createdAt,
579+
updatedAt: credential.updatedAt,
580+
})
581+
.from(credential)
582+
.where(
583+
and(
584+
eq(credential.type, 'env_personal'),
585+
eq(credential.envOwnerUserId, params.userId),
586+
eq(credential.envKey, params.envKey)
587+
)
588+
)
589+
.orderBy(asc(credential.createdAt))
590+
.limit(1)
591+
592+
return row ?? null
593+
}
594+
595+
/**
596+
* Deletes one caller-owned personal secret's credential metadata in every workspace.
597+
*
598+
* Deliberately unscoped by workspace: the value being removed alongside it lives
599+
* in the user-global `environment` row, so leaving mirrors behind in other
600+
* workspaces would advertise a secret that no longer exists.
601+
*/
556602
export async function deletePersonalEnvCredentialForUser(params: {
557603
userId: string
558604
envKey: string

0 commit comments

Comments
 (0)