diff --git a/apps/docs/content/docs/en/platform/self-hosting/architecture.mdx b/apps/docs/content/docs/en/platform/self-hosting/architecture.mdx index af526cb8465..012bb2804ae 100644 --- a/apps/docs/content/docs/en/platform/self-hosting/architecture.mdx +++ b/apps/docs/content/docs/en/platform/self-hosting/architecture.mdx @@ -84,9 +84,9 @@ Redis is a cache and message bus. Losing it drops in-flight live updates; it doe **Collaboration** — browser → ingress → realtime:3002 (`/socket.io`, WebSocket upgrade) → Redis pub/sub → other realtime pods. The proxy must pass upgrade headers and allow long-lived idle connections; see [Networking](/platform/self-hosting/networking). -**File upload (object storage configured)** — browser asks app for a presigned URL → browser `PUT`s **directly to object storage** → app records metadata. This is why buckets need a CORS policy naming your Sim origin. Downloads are proxied back through the app. +**File upload (object storage configured)** — browser asks the app to open an upload session → app returns signed transfer instructions → browser sends the bytes **directly to object storage** (one `PUT` up to 50 MB, multipart parts above that) → browser tells the app the session is complete and the app records metadata. This is why buckets need a CORS policy naming your Sim origin. Downloads are proxied back through the app. -**File upload (local disk)** — the presigned endpoint reports `directUploadSupported: false` and the browser uploads through the app instead. No CORS configuration is involved, and no bucket is used. +**File upload (local disk)** — the same upload session opens, but the transfer instructions point back at the app's own `/api/v2/uploads/...` endpoints instead of a bucket, so the bytes stream through the app. No CORS configuration is involved, and no bucket is used. **Workflow execution** — trigger (manual, API, webhook, or schedule) → app enqueues or runs inline → isolated-vm sandbox → results and logs to Postgres, progress markers to Redis. diff --git a/apps/docs/openapi-v2-billing.json b/apps/docs/openapi-v2-billing.json index dc39ce7bb69..05e85b78dd7 100644 --- a/apps/docs/openapi-v2-billing.json +++ b/apps/docs/openapi-v2-billing.json @@ -749,7 +749,7 @@ "type": "null" } ], - "description": "Opaque cursor for the next page, or null when no more items remain. Always null on a full-set list, which returns its whole result set in one response." + "description": "Opaque cursor for the next page: send it back as `cursor` to continue, and stop when it is null. Most v2 lists page, so null means the last page was reached. A few are full-set lists that return their whole bounded result in one response and therefore always report null; those say so in the operation description. Either way, null means there is nothing further to fetch — never construct a cursor yourself." } }, "required": ["data", "nextCursor"], diff --git a/apps/docs/openapi-v2-files-audit.json b/apps/docs/openapi-v2-files-audit.json index 6ff43b2a89c..53404f27332 100644 --- a/apps/docs/openapi-v2-files-audit.json +++ b/apps/docs/openapi-v2-files-audit.json @@ -1058,6 +1058,9 @@ "403": { "$ref": "#/components/responses/Forbidden" }, + "404": { + "$ref": "#/components/responses/NotFound" + }, "429": { "$ref": "#/components/responses/RateLimited" }, @@ -2211,7 +2214,7 @@ "type": "null" } ], - "description": "Opaque cursor for the next page, or null when no more items remain. Always null on a full-set list, which returns its whole result set in one response." + "description": "Opaque cursor for the next page: send it back as `cursor` to continue, and stop when it is null. Most v2 lists page, so null means the last page was reached. A few are full-set lists that return their whole bounded result in one response and therefore always report null; those say so in the operation description. Either way, null means there is nothing further to fetch — never construct a cursor yourself." } }, "required": ["data", "nextCursor"], @@ -2398,9 +2401,15 @@ "type": "string" }, "description": "Headers that must be included with the upload request." + }, + "expiresAt": { + "type": "string", + "format": "date-time", + "pattern": "^(?:(?:\\d\\d[2468][048]|\\d\\d[13579][26]|\\d\\d0[48]|[02468][048]00|[13579][26]00)-02-29|\\d{4}-(?:(?:0[13578]|1[02])-(?:0[1-9]|[12]\\d|3[01])|(?:0[469]|11)-(?:0[1-9]|[12]\\d|30)|(?:02)-(?:0[1-9]|1\\d|2[0-8])))T(?:(?:[01]\\d|2[0-3]):[0-5]\\d(?::[0-5]\\d(?:\\.\\d+)?)?(?:Z))$", + "description": "ISO 8601 expiration time for this signed URL. This is the URL's own expiry and is normally earlier than the upload session's expiresAt: the session stays open for later part, status, completion, and abort requests, but the bytes must be uploaded before this time. Once it passes, the storage provider rejects the upload and a new upload session must be created." } }, - "required": ["method", "url", "headers"], + "required": ["method", "url", "headers", "expiresAt"], "additionalProperties": false, "title": "Direct upload transfer", "description": "Instructions for uploading bytes to one signed URL." @@ -2999,7 +3008,7 @@ "type": "null" } ], - "description": "Opaque cursor for the next page, or null when no more items remain. Always null on a full-set list, which returns its whole result set in one response." + "description": "Opaque cursor for the next page: send it back as `cursor` to continue, and stop when it is null. Most v2 lists page, so null means the last page was reached. A few are full-set lists that return their whole bounded result in one response and therefore always report null; those say so in the operation description. Either way, null means there is nothing further to fetch — never construct a cursor yourself." } }, "required": ["data", "nextCursor"], @@ -3408,7 +3417,7 @@ "type": "null" } ], - "description": "Opaque cursor for the next page, or null when no more items remain. Always null on a full-set list, which returns its whole result set in one response." + "description": "Opaque cursor for the next page: send it back as `cursor` to continue, and stop when it is null. Most v2 lists page, so null means the last page was reached. A few are full-set lists that return their whole bounded result in one response and therefore always report null; those say so in the operation description. Either way, null means there is nothing further to fetch — never construct a cursor yourself." } }, "required": ["data", "nextCursor"], diff --git a/apps/docs/openapi-v2-knowledge.json b/apps/docs/openapi-v2-knowledge.json index fc2855680c3..37a2238c614 100644 --- a/apps/docs/openapi-v2-knowledge.json +++ b/apps/docs/openapi-v2-knowledge.json @@ -447,7 +447,7 @@ "post": { "operationId": "searchKnowledge", "summary": "Search Knowledge", - "description": "Search one or more knowledge bases with semantic vector retrieval, optional hybrid full-text retrieval, and structured tag filters.", + "description": "Search one or more knowledge bases with semantic vector retrieval, optional hybrid full-text retrieval, and structured tag filters. The request body is capped at 2 MiB; a larger body is a 413.", "tags": ["Knowledge Bases"], "requestBody": { "required": true, @@ -497,6 +497,9 @@ "404": { "$ref": "#/components/responses/NotFound" }, + "413": { + "$ref": "#/components/responses/PayloadTooLarge" + }, "429": { "$ref": "#/components/responses/RateLimited" }, @@ -2099,7 +2102,7 @@ "type": "null" } ], - "description": "Opaque cursor for the next page, or null when no more items remain. Always null on a full-set list, which returns its whole result set in one response." + "description": "Opaque cursor for the next page: send it back as `cursor` to continue, and stop when it is null. Most v2 lists page, so null means the last page was reached. A few are full-set lists that return their whole bounded result in one response and therefore always report null; those say so in the operation description. Either way, null means there is nothing further to fetch — never construct a cursor yourself." } }, "required": ["data", "nextCursor"], @@ -2610,7 +2613,7 @@ "type": "null" } ], - "description": "Opaque cursor for the next page, or null when no more items remain. Always null on a full-set list, which returns its whole result set in one response." + "description": "Opaque cursor for the next page: send it back as `cursor` to continue, and stop when it is null. Most v2 lists page, so null means the last page was reached. A few are full-set lists that return their whole bounded result in one response and therefore always report null; those say so in the operation description. Either way, null means there is nothing further to fetch — never construct a cursor yourself." } }, "required": ["data", "nextCursor"], @@ -2766,9 +2769,15 @@ "type": "string" }, "description": "Headers that must be included with the upload request." + }, + "expiresAt": { + "type": "string", + "format": "date-time", + "pattern": "^(?:(?:\\d\\d[2468][048]|\\d\\d[13579][26]|\\d\\d0[48]|[02468][048]00|[13579][26]00)-02-29|\\d{4}-(?:(?:0[13578]|1[02])-(?:0[1-9]|[12]\\d|3[01])|(?:0[469]|11)-(?:0[1-9]|[12]\\d|30)|(?:02)-(?:0[1-9]|1\\d|2[0-8])))T(?:(?:[01]\\d|2[0-3]):[0-5]\\d(?::[0-5]\\d(?:\\.\\d+)?)?(?:Z))$", + "description": "ISO 8601 expiration time for this signed URL. This is the URL's own expiry and is normally earlier than the upload session's expiresAt: the session stays open for later part, status, completion, and abort requests, but the bytes must be uploaded before this time. Once it passes, the storage provider rejects the upload and a new upload session must be created." } }, - "required": ["method", "url", "headers"], + "required": ["method", "url", "headers", "expiresAt"], "additionalProperties": false, "title": "Direct upload transfer", "description": "Instructions for uploading bytes to one signed URL." @@ -3255,7 +3264,7 @@ "type": "null" } ], - "description": "Opaque cursor for the next page, or null when no more items remain. Always null on a full-set list, which returns its whole result set in one response." + "description": "Opaque cursor for the next page: send it back as `cursor` to continue, and stop when it is null. Most v2 lists page, so null means the last page was reached. A few are full-set lists that return their whole bounded result in one response and therefore always report null; those say so in the operation description. Either way, null means there is nothing further to fetch — never construct a cursor yourself." } }, "required": ["data", "nextCursor"], diff --git a/apps/docs/openapi-v2-logs.json b/apps/docs/openapi-v2-logs.json index 07838db67f4..26c5cff0b2c 100644 --- a/apps/docs/openapi-v2-logs.json +++ b/apps/docs/openapi-v2-logs.json @@ -967,7 +967,7 @@ "type": "null" } ], - "description": "Opaque cursor for the next page, or null when no more items remain. Always null on a full-set list, which returns its whole result set in one response." + "description": "Opaque cursor for the next page: send it back as `cursor` to continue, and stop when it is null. Most v2 lists page, so null means the last page was reached. A few are full-set lists that return their whole bounded result in one response and therefore always report null; those say so in the operation description. Either way, null means there is nothing further to fetch — never construct a cursor yourself." } }, "required": ["data", "nextCursor"], diff --git a/apps/docs/openapi-v2-resources.json b/apps/docs/openapi-v2-resources.json index 0ed3b94c416..920147e91ce 100644 --- a/apps/docs/openapi-v2-resources.json +++ b/apps/docs/openapi-v2-resources.json @@ -2201,7 +2201,7 @@ "type": "null" } ], - "description": "Opaque cursor for the next page, or null when no more items remain. Always null on a full-set list, which returns its whole result set in one response." + "description": "Opaque cursor for the next page: send it back as `cursor` to continue, and stop when it is null. Most v2 lists page, so null means the last page was reached. A few are full-set lists that return their whole bounded result in one response and therefore always report null; those say so in the operation description. Either way, null means there is nothing further to fetch — never construct a cursor yourself." } }, "required": ["data", "nextCursor"], @@ -2365,7 +2365,7 @@ "type": "null" } ], - "description": "Opaque cursor for the next page, or null when no more items remain. Always null on a full-set list, which returns its whole result set in one response." + "description": "Opaque cursor for the next page: send it back as `cursor` to continue, and stop when it is null. Most v2 lists page, so null means the last page was reached. A few are full-set lists that return their whole bounded result in one response and therefore always report null; those say so in the operation description. Either way, null means there is nothing further to fetch — never construct a cursor yourself." } }, "required": ["data", "nextCursor"], @@ -2825,7 +2825,7 @@ "type": "null" } ], - "description": "Opaque cursor for the next page, or null when no more items remain. Always null on a full-set list, which returns its whole result set in one response." + "description": "Opaque cursor for the next page: send it back as `cursor` to continue, and stop when it is null. Most v2 lists page, so null means the last page was reached. A few are full-set lists that return their whole bounded result in one response and therefore always report null; those say so in the operation description. Either way, null means there is nothing further to fetch — never construct a cursor yourself." } }, "required": ["data", "nextCursor"], @@ -3203,7 +3203,7 @@ "type": "null" } ], - "description": "Opaque cursor for the next page, or null when no more items remain. Always null on a full-set list, which returns its whole result set in one response." + "description": "Opaque cursor for the next page: send it back as `cursor` to continue, and stop when it is null. Most v2 lists page, so null means the last page was reached. A few are full-set lists that return their whole bounded result in one response and therefore always report null; those say so in the operation description. Either way, null means there is nothing further to fetch — never construct a cursor yourself." } }, "required": ["data", "nextCursor"], @@ -3715,7 +3715,7 @@ "type": "null" } ], - "description": "Opaque cursor for the next page, or null when no more items remain. Always null on a full-set list, which returns its whole result set in one response." + "description": "Opaque cursor for the next page: send it back as `cursor` to continue, and stop when it is null. Most v2 lists page, so null means the last page was reached. A few are full-set lists that return their whole bounded result in one response and therefore always report null; those say so in the operation description. Either way, null means there is nothing further to fetch — never construct a cursor yourself." } }, "required": ["data", "nextCursor"], @@ -3799,7 +3799,7 @@ "type": "null" } ], - "description": "Opaque cursor for the next page, or null when no more items remain. Always null on a full-set list, which returns its whole result set in one response." + "description": "Opaque cursor for the next page: send it back as `cursor` to continue, and stop when it is null. Most v2 lists page, so null means the last page was reached. A few are full-set lists that return their whole bounded result in one response and therefore always report null; those say so in the operation description. Either way, null means there is nothing further to fetch — never construct a cursor yourself." } }, "required": ["data", "nextCursor"], diff --git a/apps/docs/openapi-v2-tables.json b/apps/docs/openapi-v2-tables.json index 3ee964390b0..20c7ec7bb62 100644 --- a/apps/docs/openapi-v2-tables.json +++ b/apps/docs/openapi-v2-tables.json @@ -4100,7 +4100,7 @@ "type": "null" } ], - "description": "Opaque cursor for the next page, or null when no more items remain. Always null on a full-set list, which returns its whole result set in one response." + "description": "Opaque cursor for the next page: send it back as `cursor` to continue, and stop when it is null. Most v2 lists page, so null means the last page was reached. A few are full-set lists that return their whole bounded result in one response and therefore always report null; those say so in the operation description. Either way, null means there is nothing further to fetch — never construct a cursor yourself." } }, "required": ["data", "nextCursor"], @@ -4682,7 +4682,7 @@ "type": "null" } ], - "description": "Opaque cursor for the next page, or null when no more items remain. Always null on a full-set list, which returns its whole result set in one response." + "description": "Opaque cursor for the next page: send it back as `cursor` to continue, and stop when it is null. Most v2 lists page, so null means the last page was reached. A few are full-set lists that return their whole bounded result in one response and therefore always report null; those say so in the operation description. Either way, null means there is nothing further to fetch — never construct a cursor yourself." } }, "required": ["data", "nextCursor"], @@ -5100,7 +5100,7 @@ "type": "null" } ], - "description": "Opaque cursor for the next page, or null when no more items remain. Always null on a full-set list, which returns its whole result set in one response." + "description": "Opaque cursor for the next page: send it back as `cursor` to continue, and stop when it is null. Most v2 lists page, so null means the last page was reached. A few are full-set lists that return their whole bounded result in one response and therefore always report null; those say so in the operation description. Either way, null means there is nothing further to fetch — never construct a cursor yourself." } }, "required": ["data", "nextCursor"], @@ -5341,7 +5341,7 @@ "type": "null" } ], - "description": "Opaque cursor for the next page, or null when no more items remain. Always null on a full-set list, which returns its whole result set in one response." + "description": "Opaque cursor for the next page: send it back as `cursor` to continue, and stop when it is null. Most v2 lists page, so null means the last page was reached. A few are full-set lists that return their whole bounded result in one response and therefore always report null; those say so in the operation description. Either way, null means there is nothing further to fetch — never construct a cursor yourself." } }, "required": ["data", "nextCursor"], @@ -5798,7 +5798,7 @@ "type": "null" } ], - "description": "Opaque cursor for the next page, or null when no more items remain. Always null on a full-set list, which returns its whole result set in one response." + "description": "Opaque cursor for the next page: send it back as `cursor` to continue, and stop when it is null. Most v2 lists page, so null means the last page was reached. A few are full-set lists that return their whole bounded result in one response and therefore always report null; those say so in the operation description. Either way, null means there is nothing further to fetch — never construct a cursor yourself." } }, "required": ["data", "nextCursor"], @@ -6838,9 +6838,15 @@ "type": "string" }, "description": "Headers that must be included with the upload request." + }, + "expiresAt": { + "type": "string", + "format": "date-time", + "pattern": "^(?:(?:\\d\\d[2468][048]|\\d\\d[13579][26]|\\d\\d0[48]|[02468][048]00|[13579][26]00)-02-29|\\d{4}-(?:(?:0[13578]|1[02])-(?:0[1-9]|[12]\\d|3[01])|(?:0[469]|11)-(?:0[1-9]|[12]\\d|30)|(?:02)-(?:0[1-9]|1\\d|2[0-8])))T(?:(?:[01]\\d|2[0-3]):[0-5]\\d(?::[0-5]\\d(?:\\.\\d+)?)?(?:Z))$", + "description": "ISO 8601 expiration time for this signed URL. This is the URL's own expiry and is normally earlier than the upload session's expiresAt: the session stays open for later part, status, completion, and abort requests, but the bytes must be uploaded before this time. Once it passes, the storage provider rejects the upload and a new upload session must be created." } }, - "required": ["method", "url", "headers"], + "required": ["method", "url", "headers", "expiresAt"], "additionalProperties": false, "title": "Direct upload transfer", "description": "Instructions for uploading bytes to one signed URL." @@ -7803,7 +7809,7 @@ "type": "null" } ], - "description": "Opaque cursor for the next page, or null when no more items remain. Always null on a full-set list, which returns its whole result set in one response." + "description": "Opaque cursor for the next page: send it back as `cursor` to continue, and stop when it is null. Most v2 lists page, so null means the last page was reached. A few are full-set lists that return their whole bounded result in one response and therefore always report null; those say so in the operation description. Either way, null means there is nothing further to fetch — never construct a cursor yourself." } }, "required": ["data", "nextCursor"], diff --git a/apps/docs/openapi-v2-workflows.json b/apps/docs/openapi-v2-workflows.json index cdc211333bd..e0ff219ba13 100644 --- a/apps/docs/openapi-v2-workflows.json +++ b/apps/docs/openapi-v2-workflows.json @@ -2348,7 +2348,7 @@ "type": "null" } ], - "description": "Opaque cursor for the next page, or null when no more items remain. Always null on a full-set list, which returns its whole result set in one response." + "description": "Opaque cursor for the next page: send it back as `cursor` to continue, and stop when it is null. Most v2 lists page, so null means the last page was reached. A few are full-set lists that return their whole bounded result in one response and therefore always report null; those say so in the operation description. Either way, null means there is nothing further to fetch — never construct a cursor yourself." } }, "required": ["data", "nextCursor"], @@ -2800,7 +2800,7 @@ "type": "null" } ], - "description": "Opaque cursor for the next page, or null when no more items remain. Always null on a full-set list, which returns its whole result set in one response." + "description": "Opaque cursor for the next page: send it back as `cursor` to continue, and stop when it is null. Most v2 lists page, so null means the last page was reached. A few are full-set lists that return their whole bounded result in one response and therefore always report null; those say so in the operation description. Either way, null means there is nothing further to fetch — never construct a cursor yourself." } }, "required": ["data", "nextCursor"], @@ -3920,8 +3920,16 @@ }, "status": { "type": "string", - "enum": ["pending", "running", "completed", "failed", "cancelled", "paused"], - "description": "Current or terminal run status." + "enum": [ + "pending", + "running", + "redacting", + "completed", + "failed", + "cancelled", + "paused" + ], + "description": "Current or terminal run status. `redacting` is transient, reported while the output of a finished run is being scrubbed." }, "trigger": { "type": "string", @@ -4008,7 +4016,7 @@ "type": "null" } ], - "description": "Opaque cursor for the next page, or null when no more items remain. Always null on a full-set list, which returns its whole result set in one response." + "description": "Opaque cursor for the next page: send it back as `cursor` to continue, and stop when it is null. Most v2 lists page, so null means the last page was reached. A few are full-set lists that return their whole bounded result in one response and therefore always report null; those say so in the operation description. Either way, null means there is nothing further to fetch — never construct a cursor yourself." } }, "required": ["data", "nextCursor"], @@ -4052,8 +4060,17 @@ }, "status": { "type": "string", - "enum": ["queued", "pending", "running", "completed", "failed", "cancelled", "paused"], - "description": "Current or terminal run status." + "enum": [ + "pending", + "running", + "redacting", + "completed", + "failed", + "cancelled", + "paused", + "queued" + ], + "description": "Current or terminal run status. `redacting` is transient, reported while the output of a finished run is being scrubbed." }, "trigger": { "anyOf": [ @@ -4547,7 +4564,7 @@ "type": "null" } ], - "description": "Opaque cursor for the next page, or null when no more items remain. Always null on a full-set list, which returns its whole result set in one response." + "description": "Opaque cursor for the next page: send it back as `cursor` to continue, and stop when it is null. Most v2 lists page, so null means the last page was reached. A few are full-set lists that return their whole bounded result in one response and therefore always report null; those say so in the operation description. Either way, null means there is nothing further to fetch — never construct a cursor yourself." } }, "required": ["data", "nextCursor"], diff --git a/apps/sim/app/api/folders/[id]/duplicate/route.test.ts b/apps/sim/app/api/folders/[id]/duplicate/route.test.ts new file mode 100644 index 00000000000..210ca44f054 --- /dev/null +++ b/apps/sim/app/api/folders/[id]/duplicate/route.test.ts @@ -0,0 +1,221 @@ +/** + * Tests for the folder duplication route (/api/folders/[id]/duplicate). + * + * Duplication is the recursive create path: one call copies a whole subtree, so it is the + * write most able to push a workspace past `MAX_FOLDERS_PER_WORKSPACE` — the ceiling every + * capped folder reader materializes under. These pin that the whole subtree is charged + * against the ceiling in one check, before anything is inserted. + * + * @vitest-environment node + */ +import { + auditMock, + authMockFns, + createMockRequest, + type MockUser, + permissionsMock, + permissionsMockFns, + queueTableRows, + resetDbChainMock, + schemaMock, +} from '@sim/testing' +import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest' +import { MAX_FOLDERS_PER_WORKSPACE } from '@/lib/folders/constants' + +const { + mockLogger, + mockNextFolderSortOrder, + mockDeduplicateFolderName, + mockDuplicateWorkflow, + mockAcquireFolderMutationLock, + mockWithFolderTreeLock, +} = vi.hoisted(() => ({ + mockLogger: { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + trace: vi.fn(), + fatal: vi.fn(), + child: vi.fn(), + }, + mockNextFolderSortOrder: vi.fn(), + mockDeduplicateFolderName: vi.fn(), + mockDuplicateWorkflow: vi.fn(), + mockAcquireFolderMutationLock: vi.fn(), + mockWithFolderTreeLock: vi.fn(), +})) + +vi.mock('@sim/audit', () => auditMock) +vi.mock('@sim/logger', () => ({ + createLogger: vi.fn().mockReturnValue(mockLogger), + runWithRequestContext: (_ctx: unknown, fn: () => T): T => fn(), + getRequestContext: () => undefined, +})) +vi.mock('@/lib/workspaces/permissions/utils', () => permissionsMock) +vi.mock('@/lib/folders/orchestration', () => ({ nextFolderSortOrder: mockNextFolderSortOrder })) +vi.mock('@/lib/folders/locks', () => ({ + acquireFolderMutationLock: mockAcquireFolderMutationLock, + withFolderTreeLock: mockWithFolderTreeLock, +})) +vi.mock('@/lib/folders/naming', () => ({ deduplicateFolderName: mockDeduplicateFolderName })) +vi.mock('@/lib/workflows/persistence/duplicate', () => ({ + duplicateWorkflow: mockDuplicateWorkflow, +})) + +import { POST } from '@/app/api/folders/[id]/duplicate/route' + +const TEST_USER: MockUser = { id: 'user-123', email: 'test@example.com', name: 'Test User' } +const WORKSPACE_ID = 'workspace-123' +const SOURCE_FOLDER_ID = 'folder-source' + +const FULL_MESSAGE = + 'This workspace has reached its limit of 10,000 workflow folders. Delete folders you no longer need before creating another one.' + +function folderRow(overrides: Record = {}) { + return { + id: SOURCE_FOLDER_ID, + resourceType: 'workflow', + name: 'Source', + userId: TEST_USER.id, + workspaceId: WORKSPACE_ID, + parentId: null, + color: '#6B7280', + isExpanded: true, + locked: false, + sortOrder: 0, + createdAt: new Date('2026-01-01T00:00:00Z'), + updatedAt: new Date('2026-01-01T00:00:00Z'), + deletedAt: null, + ...overrides, + } +} + +/** + * Queues, in the order the route reads them: the source-folder lookup, the workspace folder + * skeleton the subtree size is measured from, and the ceiling count. + * + * `skeleton` describes the SOURCE subtree; `activeFolderCount` is what the workspace already + * holds. Anything the route reads after the ceiling check is queued by the caller. + */ +function queueDuplicationReads(options: { + skeleton: Array<{ id: string; parentId: string | null }> + activeFolderCount: number +}) { + queueTableRows(schemaMock.folder, [folderRow()]) + queueTableRows(schemaMock.folder, options.skeleton) + queueTableRows(schemaMock.folder, [{ total: options.activeFolderCount }]) +} + +function duplicateRequest(body: Record = { name: 'Copy' }) { + return createMockRequest('POST', body) +} + +const routeContext = { params: Promise.resolve({ id: SOURCE_FOLDER_ID }) } + +describe('POST /api/folders/[id]/duplicate', () => { + afterAll(() => { + resetDbChainMock() + }) + + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + authMockFns.mockGetSession.mockResolvedValue({ user: TEST_USER }) + permissionsMockFns.mockGetUserEntityPermissions.mockResolvedValue('admin') + mockNextFolderSortOrder.mockResolvedValue(0) + mockDeduplicateFolderName.mockImplementation(async (_tx, _ws, _parent, name) => name) + mockDuplicateWorkflow.mockResolvedValue({ id: 'wf-copy' }) + }) + + it('duplicates a folder that still fits under the ceiling', async () => { + queueDuplicationReads({ + skeleton: [{ id: SOURCE_FOLDER_ID, parentId: null }], + activeFolderCount: MAX_FOLDERS_PER_WORKSPACE - 1, + }) + // Child-folder recursion finds nothing, then the response re-reads the new folder. + queueTableRows(schemaMock.folder, []) + queueTableRows(schemaMock.folder, [folderRow({ id: 'folder-copy', name: 'Copy' })]) + + const response = await POST(duplicateRequest(), routeContext) + + expect(response.status).toBe(201) + await expect(response.json()).resolves.toMatchObject({ folder: { name: 'Copy' } }) + }) + + it('refuses a single-folder duplicate once the workspace is at the ceiling', async () => { + queueDuplicationReads({ + skeleton: [{ id: SOURCE_FOLDER_ID, parentId: null }], + activeFolderCount: MAX_FOLDERS_PER_WORKSPACE, + }) + + const response = await POST(duplicateRequest(), routeContext) + + expect(response.status).toBe(409) + await expect(response.json()).resolves.toEqual({ error: FULL_MESSAGE }) + }) + + /** + * The multiplier case: the workspace has room for one more folder, but the subtree adds + * four. A per-folder check would happily insert the first three and only then refuse. + */ + it('refuses a recursive duplicate whose subtree would cross the ceiling, before inserting anything', async () => { + queueDuplicationReads({ + skeleton: [ + { id: SOURCE_FOLDER_ID, parentId: null }, + { id: 'child-a', parentId: SOURCE_FOLDER_ID }, + { id: 'child-b', parentId: SOURCE_FOLDER_ID }, + { id: 'grandchild', parentId: 'child-a' }, + { id: 'unrelated', parentId: null }, + ], + activeFolderCount: MAX_FOLDERS_PER_WORKSPACE - 1, + }) + + const response = await POST(duplicateRequest(), routeContext) + + expect(response.status).toBe(409) + await expect(response.json()).resolves.toEqual({ error: FULL_MESSAGE }) + // The refusal precedes every write in the copy, so no workflow was duplicated either. + expect(mockDuplicateWorkflow).not.toHaveBeenCalled() + expect(mockDeduplicateFolderName).not.toHaveBeenCalled() + }) + + /** + * The ceiling check must NOT be bought with the workspace-wide folder mutation lock. + * `pg_advisory_xact_lock` cannot be released early, and this transaction goes on to copy + * every workflow in the subtree — an unbounded per-workflow loop — so holding it would + * make an ordinary concurrent folder create fail on the lock timeout the helper installs. + * The accepted cost is a rare few-row overshoot instead; this pins that choice so + * re-adding the lock is a visible decision rather than a silent contention regression. + */ + it('does not hold the workspace folder mutation lock across the workflow copy', async () => { + queueDuplicationReads({ + skeleton: [{ id: SOURCE_FOLDER_ID, parentId: null }], + activeFolderCount: 0, + }) + queueTableRows(schemaMock.folder, []) + queueTableRows(schemaMock.folder, [folderRow({ id: 'folder-copy', name: 'Copy' })]) + + const response = await POST(duplicateRequest(), routeContext) + + expect(response.status).toBe(201) + expect(mockAcquireFolderMutationLock).not.toHaveBeenCalled() + expect(mockWithFolderTreeLock).not.toHaveBeenCalled() + }) + + it('allows a subtree that exactly fills the remaining room', async () => { + queueDuplicationReads({ + skeleton: [ + { id: SOURCE_FOLDER_ID, parentId: null }, + { id: 'child-a', parentId: SOURCE_FOLDER_ID }, + ], + activeFolderCount: MAX_FOLDERS_PER_WORKSPACE - 2, + }) + queueTableRows(schemaMock.folder, []) + queueTableRows(schemaMock.folder, [folderRow({ id: 'folder-copy', name: 'Copy' })]) + + const response = await POST(duplicateRequest(), routeContext) + + expect(response.status).toBe(201) + }) +}) diff --git a/apps/sim/app/api/folders/[id]/duplicate/route.ts b/apps/sim/app/api/folders/[id]/duplicate/route.ts index 0ffbe540e89..cd0e6fea92c 100644 --- a/apps/sim/app/api/folders/[id]/duplicate/route.ts +++ b/apps/sim/app/api/folders/[id]/duplicate/route.ts @@ -10,12 +10,15 @@ import { type NextRequest, NextResponse } from 'next/server' import { duplicateFolderContract } from '@/lib/api/contracts' import { parseRequest } from '@/lib/api/server' import { getSession } from '@/lib/auth' +import { asOrchestrationError } from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import type { DbOrTx } from '@/lib/db/types' import { deduplicateFolderName } from '@/lib/folders/naming' import { nextFolderSortOrder } from '@/lib/folders/orchestration' -import { toFolderApi } from '@/lib/folders/queries' +import { assertFolderCollectionHasRoom, toFolderApi } from '@/lib/folders/queries' +import { folderMutationStatus } from '@/lib/folders/status' +import { collectDescendantFolderIds } from '@/lib/folders/subtree' import { duplicateWorkflow } from '@/lib/workflows/persistence/duplicate' import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils' @@ -106,6 +109,32 @@ export const POST = withRouteHandler( const targetParentId = parentId ?? sourceFolder.parentId await assertTargetParentFolderMutable(tx, targetParentId, targetWorkspaceId, sourceFolderId) + /** + * Duplication is recursive, so it is the create path that can add the most rows + * at once. The whole subtree is measured up front and charged against the + * ceiling in one check: a per-insert check inside the recursion would both see + * room for one more each time and cost a query per folder. + * + * Deliberately NOT under `acquireFolderMutationLock`, unlike `createFolder`. That + * lock is transaction-scoped (`pg_advisory_xact_lock`) and cannot be released + * early, and this transaction goes on to copy every workflow in the subtree — an + * unbounded loop of per-workflow round trips. Holding a workspace-wide folder lock + * for that long would make an ordinary concurrent folder create fail on the lock + * timeout the helper installs, which is a certain contention regression on every + * large duplicate. Unlocked, the cost is instead a rare overshoot of a few rows + * when a concurrent create lands between this count and the inserts below, and + * only when the workspace is already within a handful of folders of the ceiling — + * the same bounded slack the readers already tolerate, and the same trade the + * workspace-fork copy makes. A certain regression is worse than a rare one. + */ + await assertFolderCollectionHasRoom(targetWorkspaceId, FOLDER_RESOURCE_TYPE, tx, { + additionalRows: await countDuplicatedFolderRows( + tx, + sourceFolder.workspaceId, + sourceFolderId + ), + }) + // Placement is the engine's rule (folders and workflows share one ordering space), // so it is read from there rather than recomputed here. const sortOrder = await nextFolderSortOrder( @@ -229,6 +258,24 @@ export const POST = withRouteHandler( return NextResponse.json({ error: error.publicMessage }, { status: error.status }) } + /** + * The workspace folder ceiling refuses this copy as a classified `conflict`, which + * must reach the caller as an actionable 409 rather than the generic 500 below. + * Unwrapped from the cause chain because drizzle re-wraps anything thrown inside the + * transaction callback in a `DrizzleQueryError`. + */ + const orchestrationError = asOrchestrationError(error) + if (orchestrationError) { + logger.warn(`[${requestId}] Folder duplication rejected: ${orchestrationError.message}`, { + sourceFolderId, + userId: session.user.id, + }) + return NextResponse.json( + { error: orchestrationError.message }, + { status: folderMutationStatus(orchestrationError.code) } + ) + } + const elapsed = Date.now() - startTime logger.error( `[${requestId}] Error duplicating folder ${sourceFolderId} after ${elapsed}ms:`, @@ -239,6 +286,34 @@ export const POST = withRouteHandler( } ) +/** + * How many folder rows this duplication will insert: the copy of the source folder plus one + * per active descendant. Reads the workspace's folder skeleton once and walks it in memory — + * the recursion below rediscovers the same tree level by level, but the ceiling has to be + * charged before the first insert, not during it. + * + * Deliberately unbounded: a workspace already over the ceiling must still be able to READ, + * and the count is what refuses the write. + */ +async function countDuplicatedFolderRows( + tx: DbOrTx, + sourceWorkspaceId: string, + sourceFolderId: string +): Promise { + const rows = await tx + .select({ id: folderTable.id, parentId: folderTable.parentId }) + .from(folderTable) + .where( + and( + eq(folderTable.workspaceId, sourceWorkspaceId), + eq(folderTable.resourceType, FOLDER_RESOURCE_TYPE), + isNull(folderTable.deletedAt) + ) + ) + + return collectDescendantFolderIds(rows, sourceFolderId).length + 1 +} + async function assertTargetParentFolderMutable( tx: DbOrTx, parentId: string | null, diff --git a/apps/sim/app/api/folders/[id]/route.ts b/apps/sim/app/api/folders/[id]/route.ts index 682f9f40497..d436943c2ee 100644 --- a/apps/sim/app/api/folders/[id]/route.ts +++ b/apps/sim/app/api/folders/[id]/route.ts @@ -9,9 +9,9 @@ import { parseRequest } from '@/lib/api/server' import { getSession } from '@/lib/auth' import { HttpError } from '@/lib/core/utils/http-error' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' -import { folderResourceConfig } from '@/lib/folders/config' import { deleteFolder, updateFolder } from '@/lib/folders/orchestration' import { toFolderApi } from '@/lib/folders/queries' +import { folderResourceSupportsLocking } from '@/lib/folders/resource-traits' import { folderMutationStatus } from '@/lib/folders/status' import { captureServerEvent } from '@/lib/posthog/server' import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils' @@ -85,7 +85,7 @@ export const PUT = withRouteHandler( // dropping it silently, and keep the admin gate and the lock checks behind the same // capability so a non-workflow folder can neither be 403'd by a field that has no // meaning for it nor persist a `locked` value nothing will ever read. - const supportsLocking = Boolean(folderResourceConfig(resourceType).supportsLocking) + const supportsLocking = folderResourceSupportsLocking(resourceType) if (locked !== undefined && !supportsLocking) { return NextResponse.json( @@ -184,7 +184,7 @@ export const DELETE = withRouteHandler( ) } - if (folderResourceConfig(resourceType).supportsLocking) { + if (folderResourceSupportsLocking(resourceType)) { await assertFolderMutable(id) } diff --git a/apps/sim/app/api/folders/reorder/route.ts b/apps/sim/app/api/folders/reorder/route.ts index dd644402927..bd7f89d5e68 100644 --- a/apps/sim/app/api/folders/reorder/route.ts +++ b/apps/sim/app/api/folders/reorder/route.ts @@ -10,8 +10,8 @@ import { getSession } from '@/lib/auth' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { withTransactionRetry } from '@/lib/db/transaction' -import { folderResourceConfig } from '@/lib/folders/config' import { acquireFolderMutationLock } from '@/lib/folders/locks' +import { folderResourceSupportsLocking } from '@/lib/folders/resource-traits' import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils' const logger = createLogger('FolderReorderAPI') @@ -141,7 +141,7 @@ export const PUT = withRouteHandler(async (req: NextRequest) => { } } - if (folderResourceConfig(resourceType).supportsLocking) { + if (folderResourceSupportsLocking(resourceType)) { for (const update of validUpdates) { await assertFolderMutable(update.id) if (update.parentId !== undefined) { diff --git a/apps/sim/app/api/folders/route.test.ts b/apps/sim/app/api/folders/route.test.ts index 65895a1d9b7..4c203c43fba 100644 --- a/apps/sim/app/api/folders/route.test.ts +++ b/apps/sim/app/api/folders/route.test.ts @@ -327,6 +327,33 @@ describe('Folders API Route', () => { }) }) + /** + * The bounded readers refuse a workspace above `MAX_FOLDERS_PER_WORKSPACE`, + * so this endpoint must refuse to push one there — and must say so as an + * actionable 409, not an unexplained 500. + */ + it('refuses with 409 and an actionable message at the folder ceiling', async () => { + mockAuthenticatedUser() + + mockTransaction.mockImplementationOnce( + createMockTransaction({ + selectResults: [[{ total: 10_000 }]], + // A row the insert would return, so a missing guard shows up as a 200, not a fault. + insertResult: [mockFolders[0]], + }) + ) + + const response = await POST( + createMockRequest('POST', { name: 'One More', workspaceId: 'workspace-123' }) + ) + + expect(response.status).toBe(409) + await expect(response.json()).resolves.toEqual({ + error: + 'This workspace has reached its limit of 10,000 workflow folders. Delete folders you no longer need before creating another one.', + }) + }) + it('should create folder with correct sort order', async () => { mockAuthenticatedUser() let capturedValues: CapturedFolderValues | null = null @@ -371,7 +398,13 @@ describe('Folders API Route', () => { mockTransaction.mockImplementationOnce( createMockTransaction({ - selectResults: [[{ workspaceId: 'workspace-123', archivedAt: null }], [], []], + // The first read is the collection-ceiling count, then the parent lookup. + selectResults: [ + [{ total: 1 }], + [{ workspaceId: 'workspace-123', archivedAt: null }], + [], + [], + ], insertResult: [{ ...mockFolders[1] }], }) ) diff --git a/apps/sim/app/api/folders/route.ts b/apps/sim/app/api/folders/route.ts index 1ac1c31e9f1..065d2d018aa 100644 --- a/apps/sim/app/api/folders/route.ts +++ b/apps/sim/app/api/folders/route.ts @@ -5,9 +5,9 @@ import { createFolderContract, listFoldersContract } from '@/lib/api/contracts' import { parseRequest } from '@/lib/api/server' import { getSession } from '@/lib/auth' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' -import { folderResourceConfig } from '@/lib/folders/config' import { createFolder } from '@/lib/folders/orchestration' import { listFoldersForWorkspace, toFolderApi } from '@/lib/folders/queries' +import { folderResourceSupportsLocking } from '@/lib/folders/resource-traits' import { folderMutationStatus } from '@/lib/folders/status' import { captureServerEvent } from '@/lib/posthog/server' import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils' @@ -79,7 +79,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => { } // Folder locking is a workflow-only feature; other resource types leave `locked` false. - if (folderResourceConfig(resourceType).supportsLocking) { + if (folderResourceSupportsLocking(resourceType)) { await assertFolderMutable(parentId ?? null) } diff --git a/apps/sim/app/api/skills/route.test.ts b/apps/sim/app/api/skills/route.test.ts new file mode 100644 index 00000000000..9e0f4334169 --- /dev/null +++ b/apps/sim/app/api/skills/route.test.ts @@ -0,0 +1,377 @@ +/** + * @vitest-environment node + */ +import { createMockRequest } from '@sim/testing' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mocks } = vi.hoisted(() => ({ + mocks: { + getSession: vi.fn(), + loadActiveWorkspaceContext: vi.fn(), + resolveEffectiveWorkspacePermission: vi.fn(), + recordAudit: vi.fn(), + getSkillById: vi.fn(), + upsertSkills: vi.fn(), + listSkillsForUser: vi.fn(), + listSkills: vi.fn(), + deleteSkill: vi.fn(), + getSkillActorContext: vi.fn(), + captureServerEvent: vi.fn(), + checkWorkspaceAccess: vi.fn(), + checkSessionOrInternalAuth: vi.fn(), + }, +})) + +/** + * Kept authenticated independently of which auth helper the route reaches for, + * so a failure here can only be about where the authorization decision and the + * audit entry are made. + */ +vi.mock('@/lib/auth/hybrid', () => ({ + AuthType: { SESSION: 'session', API_KEY: 'api_key', INTERNAL_JWT: 'internal_jwt' }, + checkSessionOrInternalAuth: mocks.checkSessionOrInternalAuth, +})) + +vi.mock('@/lib/auth', () => ({ + auth: { api: { getSession: vi.fn() } }, + getSession: mocks.getSession, +})) +vi.mock('@/lib/uploads/contexts/workspace', () => ({ + loadActiveWorkspaceContext: mocks.loadActiveWorkspaceContext, +})) +vi.mock('@sim/platform-authz/workspace', () => ({ + permissionSatisfies: (actual: string | null, required: string) => + actual === 'admin' || actual === required || (actual === 'write' && required === 'read'), + resolveEffectiveWorkspacePermission: mocks.resolveEffectiveWorkspacePermission, +})) +vi.mock('@sim/audit', () => ({ + AuditAction: { + SKILL_CREATED: 'skill.created', + SKILL_UPDATED: 'skill.updated', + SKILL_DELETED: 'skill.deleted', + }, + AuditResourceType: { SKILL: 'skill' }, + recordAudit: mocks.recordAudit, +})) +vi.mock('@/lib/workflows/skills/operations', () => ({ + getSkillById: mocks.getSkillById, + upsertSkills: mocks.upsertSkills, + listSkillsForUser: mocks.listSkillsForUser, + listSkills: mocks.listSkills, + deleteSkill: mocks.deleteSkill, +})) +vi.mock('@/lib/skills/access', () => ({ + getSkillActorContext: mocks.getSkillActorContext, +})) +vi.mock('@/lib/posthog/server', () => ({ + captureServerEvent: mocks.captureServerEvent, +})) +vi.mock('@/lib/workspaces/permissions/utils', () => ({ + checkWorkspaceAccess: mocks.checkWorkspaceAccess, +})) + +import { DELETE, POST } from '@/app/api/skills/route' + +const WORKSPACE_ID = 'workspace-1' +const USER_ID = 'user-1' + +const workspaceContext = { + workspaceId: WORKSPACE_ID, + workspaceOrganizationId: null, + allowPersonalApiKeys: true, + billedAccountUserId: 'owner-1', +} + +const skillRow = { + id: 'skill-1', + workspaceId: WORKSPACE_ID, + userId: USER_ID, + name: 'refund-policy', + description: 'Refund rules', + content: '# Refunds', + createdAt: new Date('2026-01-01T00:00:00Z'), + updatedAt: new Date('2026-01-02T00:00:00Z'), +} + +const otherSkillRow = { ...skillRow, id: 'skill-2', name: 'shipping-policy' } + +function upsertRequest(body: unknown) { + return createMockRequest('POST', body, {}, 'http://localhost:3000/api/skills') +} + +describe('internal /api/skills route', () => { + beforeEach(() => { + vi.clearAllMocks() + mocks.getSession.mockResolvedValue({ + user: { id: USER_ID, name: 'Ada', email: 'ada@example.com' }, + session: { id: 'session-1' }, + }) + mocks.checkSessionOrInternalAuth.mockResolvedValue({ + success: true, + userId: USER_ID, + userName: 'Ada', + userEmail: 'ada@example.com', + authType: 'session', + }) + mocks.loadActiveWorkspaceContext.mockResolvedValue(workspaceContext) + mocks.resolveEffectiveWorkspacePermission.mockResolvedValue('admin') + mocks.getSkillById.mockImplementation(async ({ skillId }: { skillId: string }) => + skillId === otherSkillRow.id ? otherSkillRow : skillRow + ) + mocks.upsertSkills.mockResolvedValue({ + touched: [{ id: skillRow.id, name: skillRow.name, operation: 'updated' }], + }) + mocks.listSkillsForUser.mockResolvedValue([{ ...skillRow, canEdit: true }]) + mocks.deleteSkill.mockResolvedValue(true) + mocks.getSkillActorContext.mockResolvedValue({ + skill: skillRow, + hasWorkspaceAccess: true, + canEdit: true, + }) + mocks.checkWorkspaceAccess.mockResolvedValue({ + exists: true, + hasAccess: true, + canWrite: true, + canAdmin: true, + workspace: { id: WORKSPACE_ID }, + permission: 'admin', + }) + }) + + /** + * The semantic audit entry is projected by the application use case and is + * tagged with the operation id. A surface that writes through the manager + * directly cannot produce it. The batch is one operation, `skills.upsert`; + * the create/update distinction stays on the audit action. + */ + it('records the skills.upsert semantic audit entry for an update', async () => { + const response = await POST( + upsertRequest({ + workspaceId: WORKSPACE_ID, + skills: [{ id: skillRow.id, content: '# Updated' }], + }) + ) + + expect(response.status).toBe(200) + expect(mocks.recordAudit).toHaveBeenCalledTimes(1) + expect(mocks.recordAudit).toHaveBeenCalledWith( + expect.objectContaining({ + workspaceId: WORKSPACE_ID, + actorId: USER_ID, + action: 'skill.updated', + resourceId: skillRow.id, + metadata: expect.objectContaining({ operation: 'skills.upsert' }), + }) + ) + }) + + it('records a skill.created audit entry for a create', async () => { + mocks.upsertSkills.mockResolvedValue({ + touched: [{ id: 'skill-2', name: 'new-skill', operation: 'created' }], + }) + + const response = await POST( + upsertRequest({ + workspaceId: WORKSPACE_ID, + skills: [{ name: 'new-skill', description: 'A skill', content: '# New' }], + }) + ) + + expect(response.status).toBe(200) + expect(mocks.recordAudit).toHaveBeenCalledWith( + expect.objectContaining({ + action: 'skill.created', + resourceId: 'skill-2', + metadata: expect.objectContaining({ operation: 'skills.upsert' }), + }) + ) + }) + + /** + * Canonical workspace context is loaded by the use case, so an update aimed + * at a workspace that no longer exists is refused before any write. + */ + it('refuses an update when the canonical workspace context is gone', async () => { + mocks.loadActiveWorkspaceContext.mockResolvedValue(null) + + const response = await POST( + upsertRequest({ + workspaceId: WORKSPACE_ID, + skills: [{ id: skillRow.id, content: '# Updated' }], + }) + ) + + expect(response.status).toBe(404) + await expect(response.json()).resolves.toEqual({ error: 'Workspace not found' }) + expect(mocks.upsertSkills).not.toHaveBeenCalled() + expect(mocks.recordAudit).not.toHaveBeenCalled() + }) + + it('refuses an update when the caller holds no workspace permission', async () => { + mocks.resolveEffectiveWorkspacePermission.mockResolvedValue(null) + + const response = await POST( + upsertRequest({ + workspaceId: WORKSPACE_ID, + skills: [{ id: skillRow.id, content: '# Updated' }], + }) + ) + + expect(response.status).toBe(403) + expect(mocks.upsertSkills).not.toHaveBeenCalled() + }) + + /** + * The batch is one operation. A rejected item must leave the items before it + * unwritten and unaudited rather than half-committing the request. + */ + it('writes and audits nothing when a later item in the batch is rejected', async () => { + mocks.getSkillActorContext.mockImplementation(async (skillId: string) => + skillId === skillRow.id + ? { skill: skillRow, hasWorkspaceAccess: true, canEdit: true } + : { skill: otherSkillRow, hasWorkspaceAccess: true, canEdit: false } + ) + + const response = await POST( + upsertRequest({ + workspaceId: WORKSPACE_ID, + skills: [ + { id: skillRow.id, content: '# Updated' }, + { id: otherSkillRow.id, content: '# Also updated' }, + ], + }) + ) + + expect(response.status).toBe(403) + expect(mocks.upsertSkills).not.toHaveBeenCalled() + expect(mocks.recordAudit).not.toHaveBeenCalled() + expect(mocks.captureServerEvent).not.toHaveBeenCalled() + }) + + it('commits a valid batch in one write with one audit entry per skill', async () => { + mocks.upsertSkills.mockResolvedValue({ + touched: [ + { id: skillRow.id, name: skillRow.name, operation: 'updated' }, + { id: 'skill-3', name: 'new-skill', operation: 'created' }, + ], + }) + + const response = await POST( + upsertRequest({ + workspaceId: WORKSPACE_ID, + skills: [ + { id: skillRow.id, content: '# Updated' }, + { name: 'new-skill', description: 'A skill', content: '# New' }, + ], + }) + ) + + expect(response.status).toBe(200) + expect(mocks.upsertSkills).toHaveBeenCalledTimes(1) + expect(mocks.upsertSkills).toHaveBeenCalledWith( + expect.objectContaining({ + skills: [ + { id: skillRow.id, content: '# Updated' }, + { name: 'new-skill', description: 'A skill', content: '# New' }, + ], + }) + ) + expect(mocks.recordAudit).toHaveBeenCalledTimes(2) + expect(mocks.recordAudit.mock.calls.map(([entry]) => [entry.action, entry.resourceId])).toEqual( + [ + ['skill.updated', skillRow.id], + ['skill.created', 'skill-3'], + ] + ) + }) + + /** + * The batch operation declares only the read floor an update needs, so a + * skill editor without workspace write keeps editing. A create in the same + * request is still gated on workspace write, before anything is written. + */ + it('lets a read-only skill editor update but refuses a create', async () => { + mocks.resolveEffectiveWorkspacePermission.mockResolvedValue('read') + + const updated = await POST( + upsertRequest({ + workspaceId: WORKSPACE_ID, + skills: [{ id: skillRow.id, content: '# Updated' }], + }) + ) + expect(updated.status).toBe(200) + + mocks.upsertSkills.mockClear() + const created = await POST( + upsertRequest({ + workspaceId: WORKSPACE_ID, + skills: [{ name: 'new-skill', description: 'A skill', content: '# New' }], + }) + ) + + expect(created.status).toBe(403) + expect(mocks.upsertSkills).not.toHaveBeenCalled() + }) + + /** + * The create escalation runs ahead of the write, so a mixed batch a + * read-only editor may not fully perform lands nothing at all. + */ + it('writes nothing when only the create half of a mixed batch is unauthorized', async () => { + mocks.resolveEffectiveWorkspacePermission.mockResolvedValue('read') + + const response = await POST( + upsertRequest({ + workspaceId: WORKSPACE_ID, + skills: [ + { id: skillRow.id, content: '# Updated' }, + { name: 'new-skill', description: 'A skill', content: '# New' }, + ], + }) + ) + + expect(response.status).toBe(403) + expect(mocks.upsertSkills).not.toHaveBeenCalled() + expect(mocks.recordAudit).not.toHaveBeenCalled() + }) + + it('records the skills.delete semantic audit entry', async () => { + const response = await DELETE( + createMockRequest( + 'DELETE', + undefined, + {}, + `http://localhost:3000/api/skills?id=${skillRow.id}&workspaceId=${WORKSPACE_ID}` + ) + ) + + expect(response.status).toBe(200) + expect(mocks.recordAudit).toHaveBeenCalledWith( + expect.objectContaining({ + action: 'skill.deleted', + metadata: expect.objectContaining({ operation: 'skills.delete' }), + }) + ) + }) + + it('still emits product analytics for a write', async () => { + await POST( + upsertRequest({ + workspaceId: WORKSPACE_ID, + skills: [{ id: skillRow.id, content: '# Updated' }], + source: 'settings', + }) + ) + + expect(mocks.captureServerEvent).toHaveBeenCalledWith( + USER_ID, + 'skill_updated', + expect.objectContaining({ + skill_id: skillRow.id, + workspace_id: WORKSPACE_ID, + source: 'settings', + }), + { groups: { workspace: WORKSPACE_ID } } + ) + }) +}) diff --git a/apps/sim/app/api/skills/route.ts b/apps/sim/app/api/skills/route.ts index 251635a37fb..456ff2b8d1d 100644 --- a/apps/sim/app/api/skills/route.ts +++ b/apps/sim/app/api/skills/route.ts @@ -1,3 +1,4 @@ +import type { SessionPrincipal } from '@sim/auth/principal' import { createLogger } from '@sim/logger' import { type NextRequest, NextResponse } from 'next/server' import { @@ -6,33 +7,79 @@ import { upsertSkillsContract, } from '@/lib/api/contracts' import { parseRequest, validationErrorResponse } from '@/lib/api/server' -import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid' +import { InternalUnauthenticatedError, internalSessionAuth } from '@/lib/api/server/routes' +import { + asOrchestrationError, + messageForOrchestrationError, + statusForOrchestrationError, +} from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' +import { captureServerEvent } from '@/lib/posthog/server' import { - performCreateSkill, - performDeleteSkill, - performUpdateSkill, - statusForSkillOrchestrationError, -} from '@/lib/skills/orchestration' + deleteSkillUseCase, + listAvailableSkillsUseCase, + upsertSkillsUseCase, +} from '@/lib/skills/application/use-cases' +import type { SkillWriteSource } from '@/lib/skills/orchestration' import { isBuiltinSkillId } from '@/lib/workflows/skills/builtin-skills' -import { listSkillsForUser } from '@/lib/workflows/skills/operations' -import { checkWorkspaceAccess } from '@/lib/workspaces/permissions/utils' const logger = createLogger('SkillsAPI') +/** + * This surface authenticates, parses, presents, and emits analytics. Every + * authorization decision and the semantic audit entry belong to the skill + * application use cases, which the v2 routes and copilot call as well. + * + * Only an interactive session can reach it: the skill operations model human + * principals (session, personal API key, copilot delegation), and the legacy + * internal executor JWT this route previously accepted has no principal that + * those policies can express. + */ +async function authenticatePrincipal(): Promise { + try { + return await internalSessionAuth.authenticate() + } catch (error) { + if (error instanceof InternalUnauthenticatedError) return null + throw error + } +} + +const unauthorized = () => NextResponse.json({ error: 'Unauthorized' }, { status: 401 }) + +/** Projects a classified use-case failure onto this surface's error body. */ +function orchestrationErrorResponse(error: unknown, fallback: string): NextResponse | null { + const classified = asOrchestrationError(error) + if (!classified) return null + return NextResponse.json( + { + error: messageForOrchestrationError( + { error: classified.message, errorCode: classified.code }, + fallback + ), + }, + { status: statusForOrchestrationError(classified.code) } + ) +} + +interface SkillListRow { + id: string +} + +const withReadOnly = (skills: T[]) => + skills.map((s) => ({ ...s, readOnly: isBuiltinSkillId(s.id) })) + /** GET - Fetch all skills for a workspace */ export const GET = withRouteHandler(async (request: NextRequest) => { const requestId = generateRequestId() try { - const authResult = await checkSessionOrInternalAuth(request, { requireWorkflowId: false }) - if (!authResult.success || !authResult.userId) { + const principal = await authenticatePrincipal() + if (!principal) { logger.warn(`[${requestId}] Unauthorized skills access attempt`) - return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }) + return unauthorized() } - const userId = authResult.userId const query = listSkillsQuerySchema.safeParse( Object.fromEntries(request.nextUrl.searchParams.entries()) ) @@ -43,19 +90,17 @@ export const GET = withRouteHandler(async (request: NextRequest) => { { status: 400 } ) } - const { workspaceId } = query.data - - const workspaceAccess = await checkWorkspaceAccess(workspaceId, userId) - if (!workspaceAccess.hasAccess) { - logger.warn(`[${requestId}] User ${userId} does not have access to workspace ${workspaceId}`) - return NextResponse.json({ error: 'Access denied' }, { status: 403 }) - } - const result = await listSkillsForUser({ workspaceId, userId, workspaceAccess }) - const data = result.map((s) => ({ ...s, readOnly: isBuiltinSkillId(s.id) })) + const { skills } = await listAvailableSkillsUseCase.execute({ + principal, + input: { workspaceId: query.data.workspaceId }, + request, + }) - return NextResponse.json({ data }, { status: 200 }) + return NextResponse.json({ data: withReadOnly(skills) }, { status: 200 }) } catch (error) { + const projected = orchestrationErrorResponse(error, 'Failed to fetch skills') + if (projected) return projected logger.error(`[${requestId}] Error fetching skills:`, error) return NextResponse.json({ error: 'Failed to fetch skills' }, { status: 500 }) } @@ -66,14 +111,12 @@ export const POST = withRouteHandler(async (req: NextRequest) => { const requestId = generateRequestId() try { - const authResult = await checkSessionOrInternalAuth(req, { requireWorkflowId: false }) - if (!authResult.success || !authResult.userId) { + const principal = await authenticatePrincipal() + if (!principal) { logger.warn(`[${requestId}] Unauthorized skills update attempt`) - return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }) + return unauthorized() } - const userId = authResult.userId - const parsed = await parseRequest( upsertSkillsContract, req, @@ -89,82 +132,41 @@ export const POST = withRouteHandler(async (req: NextRequest) => { const { skills, workspaceId, source } = parsed.data.body - const workspaceAccess = await checkWorkspaceAccess(workspaceId, userId) - if (!workspaceAccess.hasAccess) { - logger.warn(`[${requestId}] User ${userId} does not have access to workspace ${workspaceId}`) - return NextResponse.json({ error: 'Access denied' }, { status: 403 }) - } - /** - * Each item is applied through the skill orchestration, which owns the - * built-in guard, the field limits, the per-skill editor check, and the - * audit. Creating still requires workspace write; editing an existing skill - * is gated per skill inside `performUpdateSkill`. - * - * The batch is applied item by item rather than in one transaction: this - * endpoint's callers submit a single skill, and one shared authority for the - * rules is worth more than atomicity across a batch nobody sends. + * The whole batch is one semantic operation: the use case authorizes every + * item before writing any of them and commits them together, so a rejected + * item cannot leave earlier ones persisted. Analytics follows the commit, + * one event per skill actually written. */ - const actor = { - actorName: authResult.userName, - actorEmail: authResult.userEmail, - source, + const { touched } = await upsertSkillsUseCase.execute({ + principal, + input: { workspaceId, skills, source }, request: req, - } - - for (const item of skills) { - if (item.id) { - const result = await performUpdateSkill({ - workspaceId, - userId, - skillId: item.id, - name: item.name, - description: item.description, - content: item.content, - ...actor, - }) - if (!result.success) { - logger.warn(`[${requestId}] Skill update rejected`, { - skillId: item.id, - errorCode: result.errorCode, - }) - return NextResponse.json( - { error: result.error ?? 'Failed to update skill' }, - { status: statusForSkillOrchestrationError(result.errorCode) } - ) - } - continue - } - - if (!workspaceAccess.canWrite) { - logger.warn( - `[${requestId}] User ${userId} does not have write permission for workspace ${workspaceId}` - ) - return NextResponse.json({ error: 'Write permission required' }, { status: 403 }) - } + }) - const result = await performCreateSkill({ + for (const entry of touched) { + captureSkillEvent( + entry.operation === 'created' ? 'skill_created' : 'skill_updated', + principal.userId, workspaceId, - userId, - name: item.name!, - description: item.description!, - content: item.content!, - ...actor, - }) - if (!result.success) { - logger.warn(`[${requestId}] Skill create rejected`, { errorCode: result.errorCode }) - return NextResponse.json( - { error: result.error ?? 'Failed to create skill' }, - { status: statusForSkillOrchestrationError(result.errorCode) } - ) - } + source, + entry + ) } - const resultSkills = await listSkillsForUser({ workspaceId, userId, workspaceAccess }) - const data = resultSkills.map((s) => ({ ...s, readOnly: isBuiltinSkillId(s.id) })) + const { skills: resultSkills } = await listAvailableSkillsUseCase.execute({ + principal, + input: { workspaceId }, + request: req, + }) - return NextResponse.json({ success: true, data }) + return NextResponse.json({ success: true, data: withReadOnly(resultSkills) }) } catch (error) { + const projected = orchestrationErrorResponse(error, 'Failed to update skills') + if (projected) { + logger.warn(`[${requestId}] Skill write rejected`, { status: projected.status }) + return projected + } logger.error(`[${requestId}] Error updating skills`, error) return NextResponse.json({ error: 'Failed to update skills' }, { status: 500 }) } @@ -175,13 +177,12 @@ export const DELETE = withRouteHandler(async (request: NextRequest) => { const requestId = generateRequestId() try { - const authResult = await checkSessionOrInternalAuth(request, { requireWorkflowId: false }) - if (!authResult.success || !authResult.userId) { + const principal = await authenticatePrincipal() + if (!principal) { logger.warn(`[${requestId}] Unauthorized skill deletion attempt`) - return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }) + return unauthorized() } - const userId = authResult.userId const query = deleteSkillQuerySchema.safeParse( Object.fromEntries(request.nextUrl.searchParams.entries()) ) @@ -194,30 +195,47 @@ export const DELETE = withRouteHandler(async (request: NextRequest) => { } const { id: skillId, workspaceId, source } = query.data - const result = await performDeleteSkill({ - workspaceId, - userId, - skillId, - actorName: authResult.userName, - actorEmail: authResult.userEmail, - source, + const { skill } = await deleteSkillUseCase.execute({ + principal, + input: { workspaceId, skillId, source }, request, }) - if (!result.success) { - logger.warn(`[${requestId}] Skill delete rejected`, { - skillId, - errorCode: result.errorCode, - }) - return NextResponse.json( - { error: result.error ?? 'Failed to delete skill' }, - { status: statusForSkillOrchestrationError(result.errorCode) } - ) - } + + captureServerEvent( + principal.userId, + 'skill_deleted', + { skill_id: skill.id, workspace_id: workspaceId, source }, + { groups: { workspace: workspaceId } } + ) logger.info(`[${requestId}] Deleted skill: ${skillId}`) return NextResponse.json({ success: true }) } catch (error) { + const projected = orchestrationErrorResponse(error, 'Failed to delete skill') + if (projected) { + logger.warn(`[${requestId}] Skill delete rejected`, { status: projected.status }) + return projected + } logger.error(`[${requestId}] Error deleting skill:`, error) return NextResponse.json({ error: 'Failed to delete skill' }, { status: 500 }) } }) + +/** + * Analytics stays on the surface, as it does on the v2 routes: the use case + * owns audit, each adapter owns its own product telemetry. + */ +function captureSkillEvent( + event: 'skill_created' | 'skill_updated', + userId: string, + workspaceId: string, + source: SkillWriteSource | undefined, + skill: { id: string; name: string } +): void { + captureServerEvent( + userId, + event, + { skill_id: skill.id, skill_name: skill.name, workspace_id: workspaceId, source }, + { groups: { workspace: workspaceId } } + ) +} diff --git a/apps/sim/app/api/table/[tableId]/restore/route.test.ts b/apps/sim/app/api/table/[tableId]/restore/route.test.ts new file mode 100644 index 00000000000..15df8696bb4 --- /dev/null +++ b/apps/sim/app/api/table/[tableId]/restore/route.test.ts @@ -0,0 +1,112 @@ +/** + * @vitest-environment node + */ +import { hybridAuthMockFns, permissionsMock, permissionsMockFns } from '@sim/testing' +import { NextRequest } from 'next/server' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { TableDefinition } from '@/lib/table' + +const { mockGetTableById, mockPerformRestoreTable } = vi.hoisted(() => ({ + mockGetTableById: vi.fn(), + mockPerformRestoreTable: vi.fn(), +})) + +vi.mock('@/lib/table', () => ({ getTableById: mockGetTableById })) +vi.mock('@/lib/table/orchestration', () => ({ performRestoreTable: mockPerformRestoreTable })) +vi.mock('@/lib/workspaces/permissions/utils', () => permissionsMock) + +import { POST } from '@/app/api/table/[tableId]/restore/route' + +const TABLE = { + id: 'tbl_1', + name: 'People', + workspaceId: 'workspace-1', +} as unknown as TableDefinition + +function makeRequest(tableId = 'tbl_1') { + const request = new NextRequest(`http://localhost:3000/api/table/${tableId}/restore`, { + method: 'POST', + }) + return POST(request, { params: Promise.resolve({ tableId }) }) +} + +describe('POST /api/table/[tableId]/restore', () => { + beforeEach(() => { + vi.clearAllMocks() + hybridAuthMockFns.mockCheckSessionOrInternalAuth.mockResolvedValue({ + success: true, + userId: 'user-1', + authType: 'session', + }) + permissionsMockFns.mockGetUserEntityPermissions.mockResolvedValue('write') + mockGetTableById.mockResolvedValue(TABLE) + }) + + it('restores the table and returns it', async () => { + mockPerformRestoreTable.mockResolvedValue({ success: true, table: TABLE }) + + const response = await makeRequest() + const data = await response.json() + + expect(response.status).toBe(200) + expect(data.data.table.id).toBe('tbl_1') + }) + + it('keeps an unclassified failure out of the response body', async () => { + // `performRestoreTable` puts the raw fault text on `error` for the logs — for a driver + // fault that is the failed SQL and its bound parameters. A caller can do nothing with it + // and must never see it, so an unclassified failure renders the route's own wording. + mockPerformRestoreTable.mockResolvedValue({ + success: false, + errorCode: 'internal', + error: + 'update "user_table_definitions" set "archived_at" = $1 where "id" = $2 -- params: [null, "tbl_1"]', + }) + + const response = await makeRequest() + const data = await response.json() + + expect(response.status).toBe(500) + expect(data.error).toBe('Failed to restore table') + }) + + it('keeps the message of a classified failure the caller can act on', async () => { + mockPerformRestoreTable.mockResolvedValue({ + success: false, + errorCode: 'conflict', + error: 'A table named "People" already exists', + }) + + const response = await makeRequest() + const data = await response.json() + + expect(response.status).toBe(409) + expect(data.error).toBe('A table named "People" already exists') + }) + + it('maps a missing table to 404', async () => { + mockPerformRestoreTable.mockResolvedValue({ + success: false, + errorCode: 'not_found', + error: 'Table not found', + }) + + const response = await makeRequest() + + expect(response.status).toBe(404) + }) + + it('returns 401 when unauthenticated', async () => { + hybridAuthMockFns.mockCheckSessionOrInternalAuth.mockResolvedValue({ success: false }) + + expect((await makeRequest()).status).toBe(401) + expect(mockPerformRestoreTable).not.toHaveBeenCalled() + }) + + it('returns 403 without write permission', async () => { + permissionsMockFns.mockGetUserEntityPermissions.mockResolvedValue('read') + + expect((await makeRequest()).status).toBe(403) + expect(mockPerformRestoreTable).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/app/api/table/[tableId]/restore/route.ts b/apps/sim/app/api/table/[tableId]/restore/route.ts index 066b03480c1..c01a25af73c 100644 --- a/apps/sim/app/api/table/[tableId]/restore/route.ts +++ b/apps/sim/app/api/table/[tableId]/restore/route.ts @@ -1,5 +1,4 @@ import { createLogger } from '@sim/logger' -import { getErrorMessage } from '@sim/utils/errors' import { type NextRequest, NextResponse } from 'next/server' import { tableIdParamsSchema } from '@/lib/api/contracts/tables' import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid' @@ -8,6 +7,7 @@ import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { getTableById } from '@/lib/table' import { performRestoreTable } from '@/lib/table/orchestration' import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils' +import { orchestrationOutcomeErrorResponse } from '@/app/api/table/utils' const logger = createLogger('RestoreTableAPI') @@ -34,9 +34,7 @@ export const POST = withRouteHandler( const result = await performRestoreTable({ tableId, userId: auth.userId, requestId }) if (!result.success) { - const status = - result.errorCode === 'not_found' ? 404 : result.errorCode === 'conflict' ? 409 : 500 - return NextResponse.json({ error: result.error }, { status }) + return orchestrationOutcomeErrorResponse(result, 'Failed to restore table') } logger.info(`[${requestId}] Restored table ${tableId}`) @@ -47,10 +45,7 @@ export const POST = withRouteHandler( }) } catch (error) { logger.error(`[${requestId}] Error restoring table ${tableId}`, error) - return NextResponse.json( - { error: getErrorMessage(error, 'Internal server error') }, - { status: 500 } - ) + return NextResponse.json({ error: 'Failed to restore table' }, { status: 500 }) } } ) diff --git a/apps/sim/app/api/table/import-csv/route.test.ts b/apps/sim/app/api/table/import-csv/route.test.ts index a9722924755..dae8f0c3d63 100644 --- a/apps/sim/app/api/table/import-csv/route.test.ts +++ b/apps/sim/app/api/table/import-csv/route.test.ts @@ -31,9 +31,8 @@ vi.mock('@/lib/table/rows/service', () => ({ vi.mock('@/lib/table/billing', () => ({ getWorkspaceTableLimits: mockGetLimits })) vi.mock('@/app/api/table/utils', async () => { const { NextResponse } = await import('next/server') - const { asOrchestrationError, statusForOrchestrationError } = await import( - '@/lib/core/orchestration/types' - ) + const { asOrchestrationError, messageForOrchestrationError, statusForOrchestrationError } = + await import('@/lib/core/orchestration/types') return { normalizeColumn: (column: unknown) => column, csvProxyBodyCapResponse: () => null, @@ -42,6 +41,17 @@ vi.mock('@/app/api/table/utils', async () => { { error: error.message }, { status: error.code === 'FILE_TOO_LARGE' ? 413 : 400 } ), + orchestrationOutcomeErrorResponse: ( + outcome: { error?: string; errorCode?: OrchestrationErrorCode; lock?: string }, + fallback: string + ) => + NextResponse.json( + { + error: messageForOrchestrationError(outcome, fallback), + ...(outcome.lock ? { lock: outcome.lock } : {}), + }, + { status: statusForOrchestrationError(outcome.errorCode) } + ), orchestrationErrorResponse: (error: unknown) => { const classified = asOrchestrationError(error) return classified @@ -55,7 +65,8 @@ vi.mock('@/app/api/table/utils', async () => { }) vi.mock('@/lib/workspaces/permissions/utils', () => permissionsMock) -import { OrchestrationError } from '@/lib/core/orchestration/types' +import { OrchestrationError, type OrchestrationErrorCode } from '@/lib/core/orchestration/types' +import { TableLockedError } from '@/lib/table/mutation-locks' import { POST } from '@/app/api/table/import-csv/route' type Part = @@ -213,6 +224,19 @@ describe('POST /api/table/import-csv', () => { expect(mockDeleteTable).toHaveBeenCalledWith('tbl_1', expect.any(String)) }) + it('names the lock that rejected the import on a 423', async () => { + // The lock kind is the only thing that tells a client which lock to clear; rendering the + // outcome by hand is how the field gets dropped from one route and not its sibling. + mockBatchInsertRows.mockRejectedValueOnce(new TableLockedError('insert')) + + const response = await POST(makeRequest(uploadParts(csvWithRows(250)))) + const data = await response.json() + + expect(response.status).toBe(423) + expect(data.lock).toBe('insert') + expect(data.error).toMatch(/lock/i) + }) + it('returns 401 when unauthenticated', async () => { hybridAuthMockFns.mockCheckSessionOrInternalAuth.mockResolvedValue({ success: false }) const response = await POST(makeRequest(uploadParts(csvWithRows(3)))) diff --git a/apps/sim/app/api/table/import-csv/route.ts b/apps/sim/app/api/table/import-csv/route.ts index 06981c6e43b..d6cf5fb6440 100644 --- a/apps/sim/app/api/table/import-csv/route.ts +++ b/apps/sim/app/api/table/import-csv/route.ts @@ -5,7 +5,6 @@ import { csvExtensionSchema, csvImportFormSchema } from '@/lib/api/contracts/tab import { ianaTimezoneSchema } from '@/lib/api/contracts/user' import { getValidationErrorMessage } from '@/lib/api/server' import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid' -import { statusForOrchestrationError } from '@/lib/core/orchestration/types' import { isMultipartError, readMultipart } from '@/lib/core/utils/multipart' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' @@ -14,7 +13,11 @@ import { CSV_SYNC_MAX_FILE_SIZE_BYTES } from '@/lib/table' import { performCreateTableFromCsv } from '@/lib/table/orchestration' import { getUserSettings } from '@/lib/users/queries' import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils' -import { csvProxyBodyCapResponse, multipartErrorResponse } from '@/app/api/table/utils' +import { + csvProxyBodyCapResponse, + multipartErrorResponse, + orchestrationOutcomeErrorResponse, +} from '@/app/api/table/utils' const logger = createLogger('TableImportCSV') @@ -118,10 +121,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => { }) if (!outcome.success) { - return NextResponse.json( - { error: outcome.errorCode === 'internal' ? 'Failed to import CSV' : outcome.error }, - { status: statusForOrchestrationError(outcome.errorCode) } - ) + return orchestrationOutcomeErrorResponse(outcome, 'Failed to import CSV') } return NextResponse.json({ success: true, data: outcome.data }) diff --git a/apps/sim/app/api/v1/admin/workspaces/[id]/import/route.test.ts b/apps/sim/app/api/v1/admin/workspaces/[id]/import/route.test.ts new file mode 100644 index 00000000000..9b6b562fc6e --- /dev/null +++ b/apps/sim/app/api/v1/admin/workspaces/[id]/import/route.test.ts @@ -0,0 +1,142 @@ +/** + * Tests for the admin workspace import route. + * + * The import creates one folder per path segment through `ensureImportFolder`, a raw insert + * that used to bypass the `MAX_FOLDERS_PER_WORKSPACE` ceiling the capped folder readers + * materialize under. These pin that the ceiling is enforced and surfaces as a 409. + * + * @vitest-environment node + */ +import { createMockRequest, queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { MAX_FOLDERS_PER_WORKSPACE } from '@/lib/folders/constants' + +const { + mockLogger, + mockGetWorkspaceWithOwner, + mockParseWorkflowJson, + mockExtractWorkflowName, + mockPrepareWorkflowStateForPersistence, + mockSaveWorkflowToNormalizedTables, + mockDeduplicateWorkflowName, + mockNormalizeImportedVariables, +} = vi.hoisted(() => ({ + mockLogger: { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + trace: vi.fn(), + fatal: vi.fn(), + child: vi.fn(), + }, + mockGetWorkspaceWithOwner: vi.fn(), + mockParseWorkflowJson: vi.fn(), + mockExtractWorkflowName: vi.fn(), + mockPrepareWorkflowStateForPersistence: vi.fn(), + mockSaveWorkflowToNormalizedTables: vi.fn(), + mockDeduplicateWorkflowName: vi.fn(), + mockNormalizeImportedVariables: vi.fn(), +})) + +vi.mock('@sim/logger', () => ({ + createLogger: vi.fn().mockReturnValue(mockLogger), + runWithRequestContext: (_ctx: unknown, fn: () => T): T => fn(), + getRequestContext: () => undefined, +})) +vi.mock('@/app/api/v1/admin/middleware', () => ({ + withAdminAuthParams: (handler: unknown) => handler, +})) +vi.mock('@/lib/workspaces/permissions/utils', () => ({ + getWorkspaceWithOwner: mockGetWorkspaceWithOwner, +})) +vi.mock('@/lib/workflows/operations/import-export', () => ({ + parseWorkflowJson: mockParseWorkflowJson, + extractWorkflowName: mockExtractWorkflowName, + extractWorkflowsFromZip: vi.fn(), +})) +vi.mock('@/lib/workflows/persistence/prepare-state', () => ({ + prepareWorkflowStateForPersistence: mockPrepareWorkflowStateForPersistence, +})) +vi.mock('@/lib/workflows/persistence/utils', () => ({ + saveWorkflowToNormalizedTables: mockSaveWorkflowToNormalizedTables, +})) +vi.mock('@/lib/workflows/utils', () => ({ deduplicateWorkflowName: mockDeduplicateWorkflowName })) +vi.mock('@/lib/workflows/variables/parse', () => ({ + normalizeImportedVariables: mockNormalizeImportedVariables, +})) + +import { POST } from '@/app/api/v1/admin/workspaces/[id]/import/route' + +const WORKSPACE_ID = 'ws-1' +const routeContext = { params: Promise.resolve({ id: WORKSPACE_ID }) } + +const FULL_MESSAGE = + 'This workspace has reached its limit of 10,000 workflow folders. Delete folders you no longer need before creating another one.' + +function importRequest() { + return createMockRequest( + 'POST', + { workflows: [{ content: '{}', name: 'Report', folderPath: ['Reports'] }] }, + { 'content-type': 'application/json' }, + `http://localhost:3000/api/v1/admin/workspaces/${WORKSPACE_ID}/import` + ) +} + +/** + * Queues the two lookups `ensureImportFolder` runs before it inserts — the lock-free reuse + * probe and the re-check under the folder mutation lock — then the ceiling count. + */ +function queueFolderCreateReads(activeFolderCount: number) { + queueTableRows(schemaMock.folder, []) + queueTableRows(schemaMock.folder, []) + queueTableRows(schemaMock.folder, [{ total: activeFolderCount }]) +} + +describe('admin workspace import POST', () => { + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + mockGetWorkspaceWithOwner.mockResolvedValue({ id: WORKSPACE_ID, ownerId: 'owner-1' }) + mockParseWorkflowJson.mockReturnValue({ data: { blocks: {}, edges: [] }, errors: [] }) + mockExtractWorkflowName.mockReturnValue('Report') + mockPrepareWorkflowStateForPersistence.mockReturnValue({ state: {}, warnings: [] }) + mockSaveWorkflowToNormalizedTables.mockResolvedValue({ success: true }) + mockDeduplicateWorkflowName.mockResolvedValue('Report') + mockNormalizeImportedVariables.mockReturnValue({}) + }) + + it('imports a workflow whose folder still fits under the ceiling', async () => { + queueFolderCreateReads(MAX_FOLDERS_PER_WORKSPACE - 1) + + const response = await POST(importRequest(), routeContext) + + expect(response.status).toBe(200) + await expect(response.json()).resolves.toMatchObject({ imported: 1, failed: 0 }) + }) + + /** + * The whole import fails rather than recording N identical per-workflow errors behind a + * 200: a full folder tree is a property of the workspace, not of any one workflow. + */ + it('refuses the import with a 409 once the workspace is at the folder ceiling', async () => { + queueFolderCreateReads(MAX_FOLDERS_PER_WORKSPACE) + + const response = await POST(importRequest(), routeContext) + + expect(response.status).toBe(409) + await expect(response.json()).resolves.toEqual({ + error: { code: 'CONFLICT', message: FULL_MESSAGE }, + }) + }) + + /** An over-cap workspace must still be able to import into folders that already exist. */ + it('imports into an existing folder without consulting the ceiling', async () => { + queueTableRows(schemaMock.folder, [{ id: 'folder-existing' }]) + + const response = await POST(importRequest(), routeContext) + + expect(response.status).toBe(200) + await expect(response.json()).resolves.toMatchObject({ imported: 1, failed: 0 }) + }) +}) diff --git a/apps/sim/app/api/v1/admin/workspaces/[id]/import/route.ts b/apps/sim/app/api/v1/admin/workspaces/[id]/import/route.ts index 88768aa735c..2284b4e4a7e 100644 --- a/apps/sim/app/api/v1/admin/workspaces/[id]/import/route.ts +++ b/apps/sim/app/api/v1/admin/workspaces/[id]/import/route.ts @@ -35,7 +35,11 @@ import { adminV1WorkspaceImportBodySchema, } from '@/lib/api/contracts/v1/admin' import { parseJsonBody, parseRequest } from '@/lib/api/server' +import { asOrchestrationError } from '@/lib/core/orchestration/types' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' +import type { DbOrTx } from '@/lib/db/types' +import { withFolderTreeLock } from '@/lib/folders/locks' +import { assertFolderCollectionHasRoom } from '@/lib/folders/queries' import { extractWorkflowName, extractWorkflowsFromZip, @@ -49,6 +53,7 @@ import { getWorkspaceWithOwner } from '@/lib/workspaces/permissions/utils' import { withAdminAuthParams } from '@/app/api/v1/admin/middleware' import { badRequestResponse, + conflictResponse, internalErrorResponse, notFoundResponse, } from '@/app/api/v1/admin/responses' @@ -76,6 +81,29 @@ interface ParsedWorkflow { folderPath: string[] } +/** The active workflow folder named `name` under `parentId`, or undefined. */ +async function findImportFolder( + executor: DbOrTx, + workspaceId: string, + name: string, + parentId: string | null +): Promise { + const [existing] = await executor + .select({ id: folderTable.id }) + .from(folderTable) + .where( + and( + eq(folderTable.workspaceId, workspaceId), + eq(folderTable.resourceType, 'workflow'), + eq(folderTable.name, name), + parentId ? eq(folderTable.parentId, parentId) : isNull(folderTable.parentId), + isNull(folderTable.deletedAt) + ) + ) + .limit(1) + return existing?.id +} + /** * Returns the id of the active workflow folder named `name` under `parentId`, creating it if * absent — `mkdir -p` semantics. @@ -92,59 +120,50 @@ async function ensureImportFolder( name: string, parentId: string | null ): Promise { - const [existing] = await db - .select({ id: folderTable.id }) - .from(folderTable) - .where( - and( - eq(folderTable.workspaceId, workspaceId), - eq(folderTable.resourceType, 'workflow'), - eq(folderTable.name, name), - parentId ? eq(folderTable.parentId, parentId) : isNull(folderTable.parentId), - isNull(folderTable.deletedAt) - ) - ) - .limit(1) - if (existing) return existing.id + const existing = await findImportFolder(db, workspaceId, name, parentId) + if (existing) return existing - const folderId = generateId() try { - await db.insert(folderTable).values({ - id: folderId, - resourceType: 'workflow', - name, - userId, - workspaceId, - parentId, - createdAt: new Date(), - updatedAt: new Date(), + /** + * The create runs under the workspace's folder mutation lock, so the ceiling count and + * the insert are atomic against every other folder writer — the reuse lookup above is + * only a lock-free fast path and is repeated inside. Each call adds exactly one folder, + * so the default `additionalRows` of 1 is the real row count; the segments of one import + * path are separate calls that each take the lock and re-count. + */ + return await withFolderTreeLock(workspaceId, 'workflow', async (tx) => { + const concurrent = await findImportFolder(tx, workspaceId, name, parentId) + if (concurrent) return concurrent + + await assertFolderCollectionHasRoom(workspaceId, 'workflow', tx) + + const folderId = generateId() + await tx.insert(folderTable).values({ + id: folderId, + resourceType: 'workflow', + name, + userId, + workspaceId, + parentId, + createdAt: new Date(), + updatedAt: new Date(), + }) + return folderId }) } catch (error) { /** - * The SELECT above is not serialized against this INSERT, so two imports sharing a folder - * path race here. The name is server-chosen, not user-chosen, so the right resolution is - * to adopt whichever folder won rather than fail the whole import with a 500 — the same - * reuse-on-conflict `ensureWorkspaceFileFolderPath` already does. + * A writer that does not take the folder mutation lock can still take this name between + * the check and the insert. The name is server-chosen, not user-chosen, so the right + * resolution is to adopt whichever folder won rather than fail the whole import with a + * 500 — the same reuse-on-conflict `ensureWorkspaceFileFolderPath` already does. The + * recovery read runs on `db`, not the (now rolled back) transaction. */ if (getPostgresErrorCode(error) !== '23505') throw error - const [concurrent] = await db - .select({ id: folderTable.id }) - .from(folderTable) - .where( - and( - eq(folderTable.workspaceId, workspaceId), - eq(folderTable.resourceType, 'workflow'), - eq(folderTable.name, name), - parentId ? eq(folderTable.parentId, parentId) : isNull(folderTable.parentId), - isNull(folderTable.deletedAt) - ) - ) - .limit(1) + const concurrent = await findImportFolder(db, workspaceId, name, parentId) if (!concurrent) throw error - return concurrent.id + return concurrent } - return folderId } export const POST = withRouteHandler( @@ -259,6 +278,18 @@ export const POST = withRouteHandler( const response: WorkspaceImportResponse = { imported, failed, results } return NextResponse.json(response) } catch (error) { + /** + * The workspace folder ceiling refuses the import as a classified `conflict`. It is a + * whole-import failure rather than a per-workflow one: once the tree is full every + * remaining folder segment fails the same way, so the caller gets one actionable 409 + * instead of N identical per-workflow errors behind a 200. + */ + const orchestrationError = asOrchestrationError(error) + if (orchestrationError?.code === 'conflict') { + logger.warn('Admin API: Import refused by the workspace folder limit', { workspaceId }) + return conflictResponse(orchestrationError.message) + } + logger.error('Admin API: Failed to import into workspace', { error, workspaceId }) return internalErrorResponse('Failed to import workflows') } @@ -380,6 +411,10 @@ async function importSingleWorkflow( success: true, } } catch (error) { + // A full folder tree is a property of the workspace, not of this workflow: recording it + // as one of N per-workflow failures would bury it in a 200. Let it reach the route. + if (asOrchestrationError(error)?.code === 'conflict') throw error + return { workflowId: '', name: wf.name, diff --git a/apps/sim/app/api/v1/logs/executions/[executionId]/route.test.ts b/apps/sim/app/api/v1/logs/executions/[executionId]/route.test.ts new file mode 100644 index 00000000000..92ee767591e --- /dev/null +++ b/apps/sim/app/api/v1/logs/executions/[executionId]/route.test.ts @@ -0,0 +1,155 @@ +/** + * @vitest-environment node + */ +import { NextRequest, NextResponse } from 'next/server' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const mocks = vi.hoisted(() => ({ + checkRateLimit: vi.fn(), + validateWorkspaceAccess: vi.fn(), + getPublicWorkflowLog: vi.fn(), + getUserLimits: vi.fn(), +})) + +vi.mock('@/app/api/v1/middleware', () => ({ + checkRateLimit: mocks.checkRateLimit, + createRateLimitResponse: () => NextResponse.json({ error: 'Rate limit' }, { status: 429 }), + validateWorkspaceAccess: mocks.validateWorkspaceAccess, +})) + +vi.mock('@/lib/logs/public-queries', () => ({ + getPublicWorkflowLog: mocks.getPublicWorkflowLog, +})) + +vi.mock('@/app/api/v1/logs/meta', () => ({ + getUserLimits: mocks.getUserLimits, + createApiResponse: (data: T, limits: L) => ({ body: { ...data, limits }, headers: {} }), +})) + +/** + * Overrides the global stub, whose empty `subBlocks` would let the sanitizer + * no-op and make this suite pass against an unsanitized route. + */ +vi.mock('@/blocks/registry', () => ({ + getBlock: vi.fn(() => ({ + name: 'Gmail', + subBlocks: [ + { id: 'credential', type: 'oauth-input' }, + { id: 'apiKey', type: 'short-input', password: true }, + { id: 'envApiKey', type: 'short-input', password: true }, + { id: 'subject', type: 'short-input' }, + ], + outputs: {}, + })), + getAllBlocks: vi.fn(() => []), + getLatestBlock: vi.fn(() => undefined), + getBlockRegistry: vi.fn(() => ({})), + getBlockByToolName: vi.fn(() => undefined), +})) + +import { GET } from '@/app/api/v1/logs/executions/[executionId]/route' + +const rateLimit = { + allowed: true, + userId: 'user-1', + limit: 100, + remaining: 99, + resetAt: new Date('2026-08-11T00:00:00Z'), +} + +function snapshot() { + return { + blocks: { + 'block-1': { + id: 'block-1', + type: 'gmail', + subBlocks: { + credential: { id: 'credential', type: 'oauth-input', value: 'credential-row-id' }, + apiKey: { id: 'apiKey', type: 'short-input', value: 'literal-secret-value' }, + envApiKey: { id: 'envApiKey', type: 'short-input', value: '{{GMAIL_API_KEY}}' }, + subject: { id: 'subject', type: 'short-input', value: 'Weekly digest' }, + }, + }, + }, + edges: [], + } +} + +function requestFor(executionId: string) { + return { + request: new NextRequest(`http://localhost:3000/api/v1/logs/executions/${executionId}`), + context: { params: Promise.resolve({ executionId }) }, + } +} + +describe('GET /api/v1/logs/executions/[executionId]', () => { + beforeEach(() => { + vi.clearAllMocks() + mocks.checkRateLimit.mockResolvedValue(rateLimit) + mocks.validateWorkspaceAccess.mockResolvedValue(null) + mocks.getUserLimits.mockResolvedValue({ usage: { plan: 'free' } }) + mocks.getPublicWorkflowLog.mockResolvedValue({ + workflowId: 'workflow-1', + workspaceId: 'workspace-1', + workflowState: snapshot(), + trigger: 'api', + startedAt: new Date('2026-08-11T00:00:00Z'), + endedAt: new Date('2026-08-11T00:00:01Z'), + totalDurationMs: 1000, + costTotal: '0.01', + }) + }) + + it('redacts credentials from the snapshot while preserving env-var references', async () => { + const { request, context } = requestFor('execution-1') + const response = await GET(request, context) + const body = await response.json() + + expect(response.status).toBe(200) + + const subBlocks = body.workflowState.blocks['block-1'].subBlocks + expect(subBlocks.credential.value).toBeNull() + expect(subBlocks.apiKey.value).toBeNull() + expect(subBlocks.envApiKey.value).toBe('{{GMAIL_API_KEY}}') + expect(subBlocks.subject.value).toBe('Weekly digest') + expect(JSON.stringify(body)).not.toContain('literal-secret-value') + expect(JSON.stringify(body)).not.toContain('credential-row-id') + }) + + it('keeps the surrounding response shape intact', async () => { + const { request, context } = requestFor('execution-1') + const body = await (await GET(request, context)).json() + + expect(body).toMatchObject({ + executionId: 'execution-1', + workflowId: 'workflow-1', + executionMetadata: { + trigger: 'api', + startedAt: '2026-08-11T00:00:00.000Z', + endedAt: '2026-08-11T00:00:01.000Z', + totalDurationMs: 1000, + cost: { total: 0.01 }, + }, + limits: { usage: { plan: 'free' } }, + }) + }) + + it('reports a missing snapshot as not found', async () => { + mocks.getPublicWorkflowLog.mockResolvedValueOnce({ + workflowId: 'workflow-1', + workspaceId: 'workspace-1', + workflowState: null, + trigger: 'api', + startedAt: new Date('2026-08-11T00:00:00Z'), + endedAt: null, + totalDurationMs: 1000, + costTotal: null, + }) + + const { request, context } = requestFor('execution-1') + const response = await GET(request, context) + + expect(response.status).toBe(404) + expect(await response.json()).toEqual({ error: 'Workflow state snapshot not found' }) + }) +}) diff --git a/apps/sim/app/api/v1/logs/executions/[executionId]/route.ts b/apps/sim/app/api/v1/logs/executions/[executionId]/route.ts index cd2c2e5cead..5bb92f2363f 100644 --- a/apps/sim/app/api/v1/logs/executions/[executionId]/route.ts +++ b/apps/sim/app/api/v1/logs/executions/[executionId]/route.ts @@ -4,6 +4,7 @@ import { v1GetExecutionContract } from '@/lib/api/contracts/v1/logs' import { parseRequest } from '@/lib/api/server' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { getPublicWorkflowLog } from '@/lib/logs/public-queries' +import { sanitizeExecutionSnapshotState } from '@/lib/logs/snapshot-sanitizer' import { createApiResponse, getUserLimits } from '@/app/api/v1/logs/meta' import { checkRateLimit, @@ -50,14 +51,21 @@ export const GET = withRouteHandler( return NextResponse.json({ error: 'Workflow execution not found' }, { status: 404 }) } - if (!workflowLog.workflowState) { + /** + * The stored snapshot carries `password: true` sub-block values and `oauth-input` + * credential ids, so it is redacted before it reaches this public wire — the same + * treatment the v2 run detail applies. A snapshot the sanitizer cannot walk projects + * as `null`, which keeps the pre-existing "not found" outcome for an absent one. + */ + const workflowState = sanitizeExecutionSnapshotState(workflowLog.workflowState) + if (!workflowState) { return NextResponse.json({ error: 'Workflow state snapshot not found' }, { status: 404 }) } const response = { executionId, workflowId: workflowLog.workflowId, - workflowState: workflowLog.workflowState, + workflowState, executionMetadata: { trigger: workflowLog.trigger, startedAt: workflowLog.startedAt.toISOString(), @@ -70,9 +78,7 @@ export const GET = withRouteHandler( } logger.debug(`Successfully fetched execution data for: ${executionId}`) - logger.debug( - `Workflow state contains ${countWorkflowStateBlocks(workflowLog.workflowState)} blocks` - ) + logger.debug(`Workflow state contains ${countWorkflowStateBlocks(workflowState)} blocks`) // Get user's workflow execution limits and usage const limits = await getUserLimits(userId) diff --git a/apps/sim/app/api/v2/files/[fileId]/content/route.ts b/apps/sim/app/api/v2/files/[fileId]/content/route.ts index c85c5251366..48d35dd94b7 100644 --- a/apps/sim/app/api/v2/files/[fileId]/content/route.ts +++ b/apps/sim/app/api/v2/files/[fileId]/content/route.ts @@ -23,7 +23,6 @@ export const PUT = defineV2JsonRoute({ parseOptions: { invalidJsonResponse: () => v2Error('BAD_REQUEST', 'Request body must be valid JSON'), maxBodyBytes: MAX_WORKSPACE_FILE_INLINE_BODY_BYTES, - payloadTooLargeResponse: () => v2Error('PAYLOAD_TOO_LARGE', 'Request body is too large'), }, beforeParse: async ({ principal, params }) => { if (typeof params.fileId === 'string') { diff --git a/apps/sim/app/api/v2/files/route.ts b/apps/sim/app/api/v2/files/route.ts index bba97991356..9b905ca9342 100644 --- a/apps/sim/app/api/v2/files/route.ts +++ b/apps/sim/app/api/v2/files/route.ts @@ -64,7 +64,6 @@ export const POST = defineV2JsonRoute({ parseOptions: { invalidJsonResponse: () => v2Error('BAD_REQUEST', 'Request body must be valid JSON'), maxBodyBytes: MAX_WORKSPACE_FILE_INLINE_BODY_BYTES, - payloadTooLargeResponse: () => v2Error('PAYLOAD_TOO_LARGE', 'Request body is too large'), }, mapInput: ({ body }) => ({ workspaceId: body.workspaceId, diff --git a/apps/sim/app/api/v2/files/uploads/route.test.ts b/apps/sim/app/api/v2/files/uploads/route.test.ts index 27fdc5fca91..cfc19a29766 100644 --- a/apps/sim/app/api/v2/files/uploads/route.test.ts +++ b/apps/sim/app/api/v2/files/uploads/route.test.ts @@ -62,6 +62,7 @@ const AUTH = { rateLimitSubscription: null, keyType: 'workspace' as const, } +const URL_EXPIRES_AT = '2026-01-01T01:00:00.000Z' const UPLOAD_SESSION = { id: 'upload-1', uploadToken: 'signed-upload-token', @@ -69,6 +70,7 @@ const UPLOAD_SESSION = { method: 'put' as const, url: 'https://storage.example/upload', headers: { 'content-type': 'text/csv' }, + expiresAt: URL_EXPIRES_AT, }, } @@ -113,7 +115,11 @@ describe('POST /api/v2/files/uploads', () => { data: { session: { id: 'upload-1', status: 'uploading', file: null }, uploadToken: 'signed-upload-token', - transfer: { method: 'put', url: 'https://storage.example/upload' }, + transfer: { + method: 'put', + url: 'https://storage.example/upload', + expiresAt: URL_EXPIRES_AT, + }, }, }) expect(mocks.createUpload).toHaveBeenCalledWith({ diff --git a/apps/sim/app/api/v2/knowledge/[id]/documents/uploads/route.test.ts b/apps/sim/app/api/v2/knowledge/[id]/documents/uploads/route.test.ts index 652dd4404fb..3f2e59f50c0 100644 --- a/apps/sim/app/api/v2/knowledge/[id]/documents/uploads/route.test.ts +++ b/apps/sim/app/api/v2/knowledge/[id]/documents/uploads/route.test.ts @@ -80,6 +80,7 @@ const SESSION = { method: 'put' as const, url: 'https://storage.example/upload', headers: { 'content-type': 'application/pdf' }, + expiresAt: '2026-01-01T01:00:00.000Z', }, } diff --git a/apps/sim/app/api/v2/knowledge/search/route.ts b/apps/sim/app/api/v2/knowledge/search/route.ts index a8c4d5a0a45..070e1342015 100644 --- a/apps/sim/app/api/v2/knowledge/search/route.ts +++ b/apps/sim/app/api/v2/knowledge/search/route.ts @@ -25,7 +25,6 @@ export const POST = defineV2JsonRoute({ parseOptions: { maxBodyBytes: V2_KNOWLEDGE_SEARCH_MAX_BODY_BYTES, invalidJsonResponse: () => v2Error('BAD_REQUEST', 'Request body must be valid JSON'), - payloadTooLargeResponse: () => v2Error('PAYLOAD_TOO_LARGE', 'Request body is too large'), }, mapInput: ({ body }) => ({ workspaceId: body.workspaceId, diff --git a/apps/sim/app/api/v2/tables/imports/route.test.ts b/apps/sim/app/api/v2/tables/imports/route.test.ts index 90bd9e23e22..fd1f7672193 100644 --- a/apps/sim/app/api/v2/tables/imports/route.test.ts +++ b/apps/sim/app/api/v2/tables/imports/route.test.ts @@ -83,7 +83,12 @@ describe('POST /api/v2/tables/imports', () => { completedAt: null, }, uploadToken: 'signed-token', - transfer: { method: 'put', url: 'https://storage.example/upload', headers: {} }, + transfer: { + method: 'put', + url: 'https://storage.example/upload', + headers: {}, + expiresAt: '2026-01-01T01:00:00.000Z', + }, }, ], [ diff --git a/apps/sim/app/api/v2/tables/utils.ts b/apps/sim/app/api/v2/tables/utils.ts index f6c10a9c33a..04d02c3264e 100644 --- a/apps/sim/app/api/v2/tables/utils.ts +++ b/apps/sim/app/api/v2/tables/utils.ts @@ -15,12 +15,7 @@ import { predicateToStorage } from '@/lib/table/select-values' import type { Filter, TableLockKind } from '@/lib/table/types' import type { TableView } from '@/lib/table/views/service' import { getUserEmailsByIds, requireResolvedUserEmail } from '@/lib/users/queries' -import { - CSV_IMPORT_PROXY_BODY_CAP_BYTES, - normalizeColumn, - orchestrationErrorResponse, - rootErrorMessage, -} from '@/app/api/table/utils' +import { CSV_IMPORT_PROXY_BODY_CAP_BYTES, normalizeColumn } from '@/app/api/table/utils' import { v2Error, v2ErrorForOrchestration } from '@/app/api/v2/lib/response' /** @@ -286,17 +281,6 @@ export function v2TableOrchestrationError( ) } -/** - * Maps a known user-facing row-write failure (schema/size/unique/limit) to a v2 - * `BAD_REQUEST`, reusing v1's {@link orchestrationErrorResponse} classifier as the - * single source of truth for which messages are safe to surface. Returns `null` - * for unrecognized errors so the caller logs and returns a generic 500. - */ -export function v2RowWriteError(error: unknown): NextResponse | null { - if (!orchestrationErrorResponse(error)) return null - return v2Error('BAD_REQUEST', rootErrorMessage(error)) -} - /** * Adapts a failed-row validation from the shared `validateRowData` / * `validateBatchRows` helpers — which bake a v1-shaped `{ error, details }` 400 diff --git a/apps/sim/app/api/v2/workflows/[id]/deploy/route.test.ts b/apps/sim/app/api/v2/workflows/[id]/deploy/route.test.ts index e298f0d09aa..e083a771afc 100644 --- a/apps/sim/app/api/v2/workflows/[id]/deploy/route.test.ts +++ b/apps/sim/app/api/v2/workflows/[id]/deploy/route.test.ts @@ -53,14 +53,9 @@ describe('/api/v2/workflows/[id]/deploy route definitions', () => { error: { code: 'BAD_REQUEST', message: 'Request body must be valid JSON' }, }) - const payloadTooLargeResponse = Reflect.get( - Reflect.get(POST, 'parseOptions'), - 'payloadTooLargeResponse' - )() - expect(payloadTooLargeResponse.status).toBe(413) - expect(await payloadTooLargeResponse.json()).toEqual({ - error: { code: 'PAYLOAD_TOO_LARGE', message: 'Request body is too large' }, - }) + expect( + Reflect.get(Reflect.get(POST, 'parseOptions'), 'payloadTooLargeResponse') + ).toBeUndefined() }) it('presents the full declared deployment lifecycle response', () => { diff --git a/apps/sim/app/api/v2/workflows/[id]/deploy/route.ts b/apps/sim/app/api/v2/workflows/[id]/deploy/route.ts index 28ccde0f4bd..a72e3838f7f 100644 --- a/apps/sim/app/api/v2/workflows/[id]/deploy/route.ts +++ b/apps/sim/app/api/v2/workflows/[id]/deploy/route.ts @@ -23,7 +23,6 @@ export const POST = defineV2JsonRoute({ parseOptions: { optionalJsonBody: true, invalidJsonResponse: () => v2Error('BAD_REQUEST', 'Request body must be valid JSON'), - payloadTooLargeResponse: () => v2Error('PAYLOAD_TOO_LARGE', 'Request body is too large'), }, mapInput: ({ params, body }) => ({ workflowId: params.id, diff --git a/apps/sim/app/api/v2/workflows/[id]/rollback/route.test.ts b/apps/sim/app/api/v2/workflows/[id]/rollback/route.test.ts index 690d53fa3b9..caa12ab8903 100644 --- a/apps/sim/app/api/v2/workflows/[id]/rollback/route.test.ts +++ b/apps/sim/app/api/v2/workflows/[id]/rollback/route.test.ts @@ -48,14 +48,9 @@ describe('/api/v2/workflows/[id]/rollback route definition', () => { error: { code: 'BAD_REQUEST', message: 'Request body must be valid JSON' }, }) - const payloadTooLargeResponse = Reflect.get( - Reflect.get(POST, 'parseOptions'), - 'payloadTooLargeResponse' - )() - expect(payloadTooLargeResponse.status).toBe(413) - expect(await payloadTooLargeResponse.json()).toEqual({ - error: { code: 'PAYLOAD_TOO_LARGE', message: 'Request body is too large' }, - }) + expect( + Reflect.get(Reflect.get(POST, 'parseOptions'), 'payloadTooLargeResponse') + ).toBeUndefined() }) it('presents the full declared rollback lifecycle response', () => { diff --git a/apps/sim/app/api/v2/workflows/[id]/rollback/route.ts b/apps/sim/app/api/v2/workflows/[id]/rollback/route.ts index ae0e1a3519f..c0b9b6c20e0 100644 --- a/apps/sim/app/api/v2/workflows/[id]/rollback/route.ts +++ b/apps/sim/app/api/v2/workflows/[id]/rollback/route.ts @@ -19,7 +19,6 @@ export const POST = defineV2JsonRoute({ parseOptions: { optionalJsonBody: true, invalidJsonResponse: () => v2Error('BAD_REQUEST', 'Request body must be valid JSON'), - payloadTooLargeResponse: () => v2Error('PAYLOAD_TOO_LARGE', 'Request body is too large'), }, mapInput: ({ params, body }) => ({ workflowId: params.id, diff --git a/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/route.test.ts b/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/route.test.ts index 774f60af9ae..14ca04fbc5d 100644 --- a/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/route.test.ts +++ b/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/route.test.ts @@ -169,6 +169,20 @@ describe('v2 run detail and cancel adapters', () => { expect((await (await callStatus()).json()).data.status).toBe('queued') }) + it('returns the run resource while its output is still being redacted', async () => { + mocks.readRun.mockResolvedValueOnce({ + ...baseStatus, + status: 'redacting', + level: 'info', + error: null, + }) + + const response = await callStatus() + + expect(response.status).toBe(200) + expect((await response.json()).data.status).toBe('redacting') + }) + it('returns the public pause context without its internal paused-execution ID', async () => { mocks.readRun.mockResolvedValueOnce({ ...baseStatus, diff --git a/apps/sim/app/api/v2/workflows/[id]/runs/route.test.ts b/apps/sim/app/api/v2/workflows/[id]/runs/route.test.ts index 74f355d59f8..f3714d3f563 100644 --- a/apps/sim/app/api/v2/workflows/[id]/runs/route.test.ts +++ b/apps/sim/app/api/v2/workflows/[id]/runs/route.test.ts @@ -165,6 +165,45 @@ describe('GET /api/v2/workflows/[id]/runs', () => { expect(mocks.listRuns).not.toHaveBeenCalled() }) + it('serves a page containing a run whose output is still being redacted', async () => { + mocks.listRuns.mockResolvedValueOnce({ + data: [ + { + rowId: 'row-3', + executionId: 'execution-3', + workflowId: 'workflow-1', + status: 'redacting', + trigger: 'api', + startedAt: new Date('2026-08-05T00:03:00Z'), + endedAt: new Date('2026-08-05T00:03:01Z'), + durationMs: 1000, + costTotal: null, + }, + ...EXECUTIONS, + ], + nextCursor: null, + workflowId: 'workflow-1', + order: 'desc', + }) + + const response = await callGet() + + expect(response.status).toBe(200) + const body = await response.json() + expect(body.data.map((run: { status: string }) => run.status)).toEqual([ + 'redacting', + 'paused', + 'completed', + ]) + }) + + it('rejects redacting as a durable-history filter', async () => { + const response = await callGet('?status=redacting') + + expect(response.status).toBe(400) + expect(mocks.listRuns).not.toHaveBeenCalled() + }) + it('rejects queued as a durable-history filter', async () => { const response = await callGet('?status=queued') diff --git a/apps/sim/app/api/workspaces/[id]/fork/promote/route.test.ts b/apps/sim/app/api/workspaces/[id]/fork/promote/route.test.ts new file mode 100644 index 00000000000..09528e0aa10 --- /dev/null +++ b/apps/sim/app/api/workspaces/[id]/fork/promote/route.test.ts @@ -0,0 +1,106 @@ +/** + * Tests for the fork sync (promote) route's error projection. + * + * `promoteFork` returns its deliberate refusals as a `blocked` result, but a classified + * failure raised deeper in the copy — the target workspace's folder ceiling being full — + * throws. `withRouteHandler` only understands `HttpError`, so without an explicit branch + * that throw renders as an opaque 500. + * + * @vitest-environment node + */ +import { auditMock, authMockFns, createMockRequest, type MockUser } from '@sim/testing' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { FolderCollectionFullError } from '@/lib/folders/errors' + +const { mockLogger, mockPromoteFork, mockAssertCanPromote, mockRecordBackgroundWork } = vi.hoisted( + () => ({ + mockLogger: { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + trace: vi.fn(), + fatal: vi.fn(), + child: vi.fn(), + }, + mockPromoteFork: vi.fn(), + mockAssertCanPromote: vi.fn(), + mockRecordBackgroundWork: vi.fn(), + }) +) + +vi.mock('@sim/audit', () => auditMock) +vi.mock('@sim/logger', () => ({ + createLogger: vi.fn().mockReturnValue(mockLogger), + runWithRequestContext: (_ctx: unknown, fn: () => T): T => fn(), + getRequestContext: () => undefined, +})) +vi.mock('@/ee/workspace-forking/lib/promote/promote', () => ({ promoteFork: mockPromoteFork })) +vi.mock('@/ee/workspace-forking/lib/lineage/authz', () => ({ + assertCanPromote: mockAssertCanPromote, +})) +vi.mock('@/ee/workspace-forking/lib/background-work/store', () => ({ + recordBackgroundWork: mockRecordBackgroundWork, +})) + +import { POST } from '@/app/api/workspaces/[id]/fork/promote/route' + +const TEST_USER: MockUser = { id: 'user-1', email: 'a@b.com', name: 'A' } +const WORKSPACE_ID = 'ws-child' +const routeContext = { params: Promise.resolve({ id: WORKSPACE_ID }) } + +const FULL_MESSAGE = + 'This workspace has reached its limit of 10,000 workflow folders. Delete folders you no longer need before creating another one.' + +function promoteRequest() { + return createMockRequest( + 'POST', + { otherWorkspaceId: 'ws-parent', direction: 'push' }, + { 'content-type': 'application/json' }, + `http://localhost:3000/api/workspaces/${WORKSPACE_ID}/fork/promote` + ) +} + +describe('POST /api/workspaces/[id]/fork/promote', () => { + beforeEach(() => { + vi.clearAllMocks() + authMockFns.mockGetSession.mockResolvedValue({ user: TEST_USER }) + mockAssertCanPromote.mockResolvedValue({ + edge: { childWorkspaceId: WORKSPACE_ID }, + sourceWorkspaceId: WORKSPACE_ID, + targetWorkspaceId: 'ws-parent', + }) + mockRecordBackgroundWork.mockResolvedValue(undefined) + }) + + it('renders a full-folder-tree refusal as an actionable 409', async () => { + mockPromoteFork.mockRejectedValue(new FolderCollectionFullError('workflow', 10_000)) + + const response = await POST(promoteRequest(), routeContext) + + expect(response.status).toBe(409) + await expect(response.json()).resolves.toEqual({ error: FULL_MESSAGE }) + }) + + /** The copy runs inside a transaction, so drizzle wraps the throw; the cause chain matters. */ + it('classifies a refusal that drizzle wrapped in a transaction error', async () => { + mockPromoteFork.mockRejectedValue( + new Error('select "folder"."id" from "folder" ...', { + cause: new FolderCollectionFullError('workflow', 10_000), + }) + ) + + const response = await POST(promoteRequest(), routeContext) + + expect(response.status).toBe(409) + await expect(response.json()).resolves.toEqual({ error: FULL_MESSAGE }) + }) + + it('rethrows an unclassified failure instead of dressing it as a 409', async () => { + mockPromoteFork.mockRejectedValue(new Error('connection reset')) + + const response = await POST(promoteRequest(), routeContext) + + expect(response.status).toBe(500) + }) +}) diff --git a/apps/sim/app/api/workspaces/[id]/fork/promote/route.ts b/apps/sim/app/api/workspaces/[id]/fork/promote/route.ts index a06dd6300f9..23f128589d3 100644 --- a/apps/sim/app/api/workspaces/[id]/fork/promote/route.ts +++ b/apps/sim/app/api/workspaces/[id]/fork/promote/route.ts @@ -6,6 +6,7 @@ import { type NextRequest, NextResponse } from 'next/server' import { promoteForkContract } from '@/lib/api/contracts/workspace-fork' import { parseRequest } from '@/lib/api/server' import { getSession } from '@/lib/auth' +import { asOrchestrationError, statusForOrchestrationError } from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { recordBackgroundWork } from '@/ee/workspace-forking/lib/background-work/store' @@ -36,19 +37,38 @@ export const POST = withRouteHandler( const auth = await assertCanPromote(id, otherWorkspaceId, direction, session.user.id) - const result = await promoteFork({ - edge: auth.edge, - sourceWorkspaceId: auth.sourceWorkspaceId, - targetWorkspaceId: auth.targetWorkspaceId, - direction, - userId: session.user.id, - actorName: session.user.name ?? undefined, - dependentValues, - copyResources, - dropReferences, - triggerMappings, - requestId, - }) + let result: Awaited> + try { + result = await promoteFork({ + edge: auth.edge, + sourceWorkspaceId: auth.sourceWorkspaceId, + targetWorkspaceId: auth.targetWorkspaceId, + direction, + userId: session.user.id, + actorName: session.user.name ?? undefined, + dependentValues, + copyResources, + dropReferences, + triggerMappings, + requestId, + }) + } catch (error) { + /** + * `promoteFork` returns its deliberate refusals as a `blocked` result, but a + * classified failure raised deeper in the copy — the target workspace's folder + * ceiling being full, for one — throws instead. Without this branch it reaches + * `withRouteHandler`, which only understands `HttpError` and renders everything else + * as an opaque `Internal server error` 500. Unwrapped from the cause chain because + * drizzle re-wraps anything thrown inside a transaction callback. + */ + const classified = asOrchestrationError(error) + if (!classified) throw error + logger.warn(`[${requestId}] Fork sync refused: ${classified.message}`) + return NextResponse.json( + { error: classified.message }, + { status: statusForOrchestrationError(classified.code) } + ) + } const body = { promoteRunId: result.promoteRunId, diff --git a/apps/sim/app/api/workspaces/[id]/fork/route.test.ts b/apps/sim/app/api/workspaces/[id]/fork/route.test.ts new file mode 100644 index 00000000000..c23e3a45ee9 --- /dev/null +++ b/apps/sim/app/api/workspaces/[id]/fork/route.test.ts @@ -0,0 +1,111 @@ +/** + * Tests for the workspace fork route's error projection. + * + * The fork copy mirrors the source folder tree into the child and refuses when that would + * cross the child's `MAX_FOLDERS_PER_WORKSPACE` ceiling. That refusal is a classified + * `OrchestrationError`, which `withRouteHandler` alone renders as an opaque 500 — it only + * understands `HttpError`. These pin that the caller gets the actionable 409 instead. + * + * @vitest-environment node + */ +import { auditMock, authMockFns, createMockRequest, type MockUser } from '@sim/testing' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { FolderCollectionFullError } from '@/lib/folders/errors' + +const { mockLogger, mockCreateFork, mockAssertCanFork } = vi.hoisted(() => ({ + mockLogger: { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + trace: vi.fn(), + fatal: vi.fn(), + child: vi.fn(), + }, + mockCreateFork: vi.fn(), + mockAssertCanFork: vi.fn(), +})) + +vi.mock('@sim/audit', () => auditMock) +vi.mock('@sim/logger', () => ({ + createLogger: vi.fn().mockReturnValue(mockLogger), + runWithRequestContext: (_ctx: unknown, fn: () => T): T => fn(), + getRequestContext: () => undefined, +})) +vi.mock('@/ee/workspace-forking/lib/create-fork', () => ({ createFork: mockCreateFork })) +vi.mock('@/ee/workspace-forking/lib/lineage/authz', () => ({ assertCanFork: mockAssertCanFork })) + +import { POST } from '@/app/api/workspaces/[id]/fork/route' + +const TEST_USER: MockUser = { id: 'user-1', email: 'a@b.com', name: 'A' } +const SOURCE_WORKSPACE_ID = 'ws-source' +const routeContext = { params: Promise.resolve({ id: SOURCE_WORKSPACE_ID }) } + +const FULL_MESSAGE = + 'This workspace has reached its limit of 10,000 workflow folders. Delete folders you no longer need before creating another one.' + +function forkRequest() { + return createMockRequest( + 'POST', + { name: 'Child' }, + { 'content-type': 'application/json' }, + `http://localhost:3000/api/workspaces/${SOURCE_WORKSPACE_ID}/fork` + ) +} + +describe('POST /api/workspaces/[id]/fork', () => { + beforeEach(() => { + vi.clearAllMocks() + authMockFns.mockGetSession.mockResolvedValue({ user: TEST_USER }) + mockAssertCanFork.mockResolvedValue({ + source: { id: SOURCE_WORKSPACE_ID, name: 'Source' }, + policy: {}, + }) + }) + + it('renders a full-folder-tree refusal as an actionable 409', async () => { + mockCreateFork.mockRejectedValue(new FolderCollectionFullError('workflow', 10_000)) + + const response = await POST(forkRequest(), routeContext) + + expect(response.status).toBe(409) + await expect(response.json()).resolves.toEqual({ error: FULL_MESSAGE }) + }) + + /** + * Drizzle re-wraps anything thrown inside a `db.transaction` callback, and the fork copy + * runs inside one — so an `instanceof` check at this layer would miss it and fall through + * to the generic 500. The projection must walk the cause chain. + */ + it('classifies a refusal that drizzle wrapped in a transaction error', async () => { + const wrapped = new Error('select "folder"."id" from "folder" ...', { + cause: new FolderCollectionFullError('workflow', 10_000), + }) + mockCreateFork.mockRejectedValue(wrapped) + + const response = await POST(forkRequest(), routeContext) + + expect(response.status).toBe(409) + await expect(response.json()).resolves.toEqual({ error: FULL_MESSAGE }) + }) + + /** An unclassified fault must keep falling through to the handler's generic 500. */ + it('rethrows an unclassified failure instead of dressing it as a 409', async () => { + mockCreateFork.mockRejectedValue(new Error('connection reset')) + + const response = await POST(forkRequest(), routeContext) + + expect(response.status).toBe(500) + }) + + it('still returns the created fork when the copy succeeds', async () => { + mockCreateFork.mockResolvedValue({ + workspace: { id: 'ws-child', name: 'Child' }, + workflowsCopied: 2, + }) + + const response = await POST(forkRequest(), routeContext) + + expect(response.status).toBe(201) + }) +}) diff --git a/apps/sim/app/api/workspaces/[id]/fork/route.ts b/apps/sim/app/api/workspaces/[id]/fork/route.ts index 27cd8fdbd03..c4a90c30d6f 100644 --- a/apps/sim/app/api/workspaces/[id]/fork/route.ts +++ b/apps/sim/app/api/workspaces/[id]/fork/route.ts @@ -4,6 +4,7 @@ import { type NextRequest, NextResponse } from 'next/server' import { forkWorkspaceContract } from '@/lib/api/contracts/workspace-fork' import { parseRequest } from '@/lib/api/server' import { getSession } from '@/lib/auth' +import { asOrchestrationError, statusForOrchestrationError } from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { createFork } from '@/ee/workspace-forking/lib/create-fork' @@ -27,23 +28,42 @@ export const POST = withRouteHandler( if (!parsed.success) return parsed.response const copy = parsed.data.body.copy - const result = await createFork({ - source, - policy, - userId: session.user.id, - actorName: session.user.name ?? undefined, - name: parsed.data.body.name, - selection: { - files: copy?.files ?? [], - tables: copy?.tables ?? [], - knowledgeBases: copy?.knowledgeBases ?? [], - customTools: copy?.customTools ?? [], - skills: copy?.skills ?? [], - mcpServers: copy?.mcpServers ?? [], - workflowMcpServers: copy?.workflowMcpServers ?? [], - }, - requestId, - }) + let result: Awaited> + try { + result = await createFork({ + source, + policy, + userId: session.user.id, + actorName: session.user.name ?? undefined, + name: parsed.data.body.name, + selection: { + files: copy?.files ?? [], + tables: copy?.tables ?? [], + knowledgeBases: copy?.knowledgeBases ?? [], + customTools: copy?.customTools ?? [], + skills: copy?.skills ?? [], + mcpServers: copy?.mcpServers ?? [], + workflowMcpServers: copy?.workflowMcpServers ?? [], + }, + requestId, + }) + } catch (error) { + /** + * The fork copy raises classified, caller-fixable refusals — the child workspace's + * folder ceiling being full, for one. Without this branch they reach + * `withRouteHandler`, which only understands `HttpError` and renders everything else + * as an opaque `Internal server error` 500, dropping the message that tells the user + * what to do. Unwrapped from the cause chain because drizzle re-wraps anything thrown + * inside a transaction callback in a `DrizzleQueryError`. + */ + const classified = asOrchestrationError(error) + if (!classified) throw error + logger.warn(`[${requestId}] Fork of ${sourceWorkspaceId} refused: ${classified.message}`) + return NextResponse.json( + { error: classified.message }, + { status: statusForOrchestrationError(classified.code) } + ) + } recordAudit({ workspaceId: result.workspace.id, diff --git a/apps/sim/ee/workspace-forking/lib/copy/copy-workflows.test.ts b/apps/sim/ee/workspace-forking/lib/copy/copy-workflows.test.ts index 44913d6381c..abbe788a5b4 100644 --- a/apps/sim/ee/workspace-forking/lib/copy/copy-workflows.test.ts +++ b/apps/sim/ee/workspace-forking/lib/copy/copy-workflows.test.ts @@ -3,6 +3,8 @@ */ import { describe, expect, it, vi } from 'vitest' import type { DbOrTx } from '@/lib/db/types' +import { MAX_FOLDERS_PER_WORKSPACE } from '@/lib/folders/constants' +import { FolderCollectionFullError } from '@/lib/folders/errors' const { mockSaveWorkflowToNormalizedTables } = vi.hoisted(() => ({ mockSaveWorkflowToNormalizedTables: vi.fn(), @@ -109,16 +111,22 @@ function folderRow(id: string, name: string, parentId: string | null = null): Fo /** * Transaction stub for {@link resolveForkFolderMapping}: the first awaited select resolves - * the source folders, the second the target folders, and inserted rows are captured. + * the source folders, the second the target folders, the third the target's active folder + * count (the ceiling check, which only runs when the copy has folders to insert), and + * inserted rows are captured. */ -function buildFolderTx(sourceFolders: FolderRow[], targetFolders: FolderRow[] = []) { +function buildFolderTx( + sourceFolders: FolderRow[], + targetFolders: FolderRow[] = [], + targetFolderCount = 0 +) { const insertedRows: FolderRow[] = [] - const selects = [sourceFolders, targetFolders] + const selects: unknown[][] = [sourceFolders, targetFolders, [{ total: targetFolderCount }]] let selectIndex = 0 const tx = { select: () => ({ from: () => ({ - where: () => Promise.resolve(selects[selectIndex++] ?? []), + where: () => Promise.resolve((selects[selectIndex++] ?? []) as FolderRow[]), }), }), insert: () => ({ @@ -254,6 +262,55 @@ describe('resolveForkFolderMapping', () => { expect(insertedRows[0].name).toBe('Child') expect(insertedRows[0].parentId).toBe('T-parent') }) + + /** + * The fork mirrors a whole source subtree into the target in one bulk insert, so it can + * push the target past `MAX_FOLDERS_PER_WORKSPACE` — the ceiling every capped folder + * reader materializes under — and leave the target's folder list unreadable. The refusal + * is raised before the insert and inside the fork transaction, so the copy rolls back. + */ + it('refuses a fork whose new folders would cross the target workspace ceiling', async () => { + const { tx, insertedRows } = buildFolderTx( + [folderRow('A', 'Alpha'), folderRow('B', 'Beta', 'A'), folderRow('C', 'Gamma', 'B')], + [], + MAX_FOLDERS_PER_WORKSPACE - 2 + ) + + const rejection = expect(resolveMapping({ tx, contentFolderIds: ['C'] })).rejects + await rejection.toBeInstanceOf(FolderCollectionFullError) + await rejection.toMatchObject({ code: 'conflict' }) + expect(insertedRows).toHaveLength(0) + }) + + it('allows a fork whose new folders exactly fill the target workspace ceiling', async () => { + const { tx, insertedRows } = buildFolderTx( + [folderRow('A', 'Alpha'), folderRow('B', 'Beta', 'A'), folderRow('C', 'Gamma', 'B')], + [], + MAX_FOLDERS_PER_WORKSPACE - 3 + ) + + await resolveMapping({ tx, contentFolderIds: ['C'] }) + + expect(insertedRows).toHaveLength(3) + }) + + /** + * A sync that reuses every target folder adds no rows, so an already-over-cap target must + * not have it refused — the ceiling gates writes, never reads. + */ + it('does not refuse a sync into an over-cap target when it creates no folders', async () => { + const existing = { ...folderRow('T1', 'Shared'), workspaceId: 'ws-target' } + const { tx, insertedRows } = buildFolderTx( + [folderRow('G', 'Shared')], + [existing], + MAX_FOLDERS_PER_WORKSPACE + 5 + ) + + const map = await resolveMapping({ tx, contentFolderIds: ['G'] }) + + expect(insertedRows).toHaveLength(0) + expect(map.get('G')).toBe('T1') + }) }) describe('copyWorkflowStateIntoTarget folder fallback', () => { diff --git a/apps/sim/ee/workspace-forking/lib/copy/copy-workflows.ts b/apps/sim/ee/workspace-forking/lib/copy/copy-workflows.ts index 2d6fec8dbb2..3d5cbb2bf71 100644 --- a/apps/sim/ee/workspace-forking/lib/copy/copy-workflows.ts +++ b/apps/sim/ee/workspace-forking/lib/copy/copy-workflows.ts @@ -3,6 +3,7 @@ import { createLogger } from '@sim/logger' import { generateId } from '@sim/utils/id' import { and, eq, inArray, isNull } from 'drizzle-orm' import type { DbOrTx } from '@/lib/db/types' +import { assertFolderCollectionHasRoom } from '@/lib/folders/queries' import { remapConditionEdgeHandle } from '@/lib/workflows/condition-ids' import { remapConditionIdsInSubBlocks, @@ -158,6 +159,21 @@ export async function resolveForkFolderMapping({ } if (newFolders.length > 0) { + /** + * Charge the whole batch against the target workspace's folder ceiling in one check + * before writing any of it. Readers cap the active folder index at + * `MAX_FOLDERS_PER_WORKSPACE`, so a fork that mirrors a large source tree into an + * already-populated target could otherwise leave the target unreadable. + * + * Runs inside the fork transaction, after the `fork-target` advisory lock, so it is + * atomic against every other fork/promote into this target and a refusal rolls the + * whole copy back. It does NOT take the folder mutation lock: that helper resets the + * transaction's `lock_timeout`, which the fork sets deliberately, so an ordinary + * concurrent `createFolder` can still slip a row in between the count and the insert. + */ + await assertFolderCollectionHasRoom(targetWorkspaceId, 'workflow', tx, { + additionalRows: newFolders.length, + }) await tx.insert(folderTable).values(newFolders) } diff --git a/apps/sim/lib/api/contracts/knowledge/upload-sessions.ts b/apps/sim/lib/api/contracts/knowledge/upload-sessions.ts index f90cf22422d..32d3eb7440f 100644 --- a/apps/sim/lib/api/contracts/knowledge/upload-sessions.ts +++ b/apps/sim/lib/api/contracts/knowledge/upload-sessions.ts @@ -30,6 +30,7 @@ const knowledgeDocumentUploadTransferSchema = z.discriminatedUnion('method', [ method: z.literal('put'), url: z.string().url(), headers: z.record(z.string(), z.string()), + expiresAt: z.string().datetime(), }) .strict(), z diff --git a/apps/sim/lib/api/contracts/v2/__tests__/uploads.test.ts b/apps/sim/lib/api/contracts/v2/__tests__/uploads.test.ts index 5f879e60980..f9540b90e84 100644 --- a/apps/sim/lib/api/contracts/v2/__tests__/uploads.test.ts +++ b/apps/sim/lib/api/contracts/v2/__tests__/uploads.test.ts @@ -8,8 +8,9 @@ describe('v2 upload transfer contracts', () => { method: 'put', url: 'https://storage.example/upload', headers: { 'Content-Type': 'application/octet-stream' }, + expiresAt: '2026-01-01T01:00:00.000Z', }) - ).toMatchObject({ method: 'put' }) + ).toMatchObject({ method: 'put', expiresAt: '2026-01-01T01:00:00.000Z' }) expect( v2UploadTransferSchema.parse({ method: 'multipart', @@ -25,4 +26,14 @@ describe('v2 upload transfer contracts', () => { }).success ).toBe(false) }) + + it('requires a PUT transfer to advertise its own URL expiry', () => { + expect( + v2UploadTransferSchema.safeParse({ + method: 'put', + url: 'https://storage.example/upload', + headers: { 'Content-Type': 'application/octet-stream' }, + }).success + ).toBe(false) + }) }) diff --git a/apps/sim/lib/api/contracts/v2/billing.ts b/apps/sim/lib/api/contracts/v2/billing.ts index b39a33beb2d..3dcfd28b9d9 100644 --- a/apps/sim/lib/api/contracts/v2/billing.ts +++ b/apps/sim/lib/api/contracts/v2/billing.ts @@ -37,13 +37,16 @@ export const v2BillingStatusQuerySchema = z.object({ * and source analytics deliberately live outside this status resource. * * `credits` and `storage` report the resolved payer's pooled allowances, which - * are shared across every workspace that payer funds. They are populated only - * for a caller who may manage that payer's billing: the billed account holder, - * or an admin of the hosting organization. Billing authority is a property of - * a person, so an actor-less workspace API key never qualifies. Every other - * caller reads both as `null` while still seeing the plan, period, and - * standing that the workspace already surfaces to them — enough to monitor for - * `limit_exceeded` and `billing_blocked`. + * are shared across every workspace and member that payer funds. They are + * populated only for a caller who may manage that payer's billing: the billed + * account holder, or an admin of the owning organization. Billing authority is + * a property of a person, so an actor-less workspace API key never qualifies. + * This holds on both scopes — omitting `workspaceId` resolves the payer from + * the caller's own subscriptions and organization memberships, and plain + * membership is not authority over the organization's pool. Every other caller + * reads both as `null` while still seeing the plan, period, and standing of + * the payer that funds them — enough to monitor for `limit_exceeded` and + * `billing_blocked`. */ export const v2BillingStatusDataSchema = z .object({ diff --git a/apps/sim/lib/api/contracts/v2/openapi/files-audit.ts b/apps/sim/lib/api/contracts/v2/openapi/files-audit.ts index 0d6ea0ac3e9..1eaca40c670 100644 --- a/apps/sim/lib/api/contracts/v2/openapi/files-audit.ts +++ b/apps/sim/lib/api/contracts/v2/openapi/files-audit.ts @@ -460,7 +460,7 @@ const routes = [ operationId: 'listAuditLogs', summary: 'List Audit Logs', description: `List an organization audit trail with filters and opaque cursor pagination. Requires an Enterprise subscription and organization admin or owner access. ${WORKSPACE_API_KEY_DENIED}`, - errors: [...STANDARD_ERRORS, 'BadRequest', 'Forbidden'], + errors: [...VALIDATED_ERRORS, 'Forbidden', 'NotFound'], success: { description: 'A page of audit-log entries.' }, }), { diff --git a/apps/sim/lib/api/contracts/v2/openapi/knowledge.ts b/apps/sim/lib/api/contracts/v2/openapi/knowledge.ts index 0d63eb3eab7..3e86d717eb9 100644 --- a/apps/sim/lib/api/contracts/v2/openapi/knowledge.ts +++ b/apps/sim/lib/api/contracts/v2/openapi/knowledge.ts @@ -207,8 +207,8 @@ const routes = [ operationId: 'searchKnowledge', summary: 'Search Knowledge', description: - 'Search one or more knowledge bases with semantic vector retrieval, optional hybrid full-text retrieval, and structured tag filters.', - errors: [...WORKSPACE_ERRORS, 'UsageLimitExceeded', 'NotFound'], + 'Search one or more knowledge bases with semantic vector retrieval, optional hybrid full-text retrieval, and structured tag filters. The request body is capped at 2 MiB; a larger body is a 413.', + errors: [...WORKSPACE_ERRORS, 'UsageLimitExceeded', 'NotFound', 'PayloadTooLarge'], success: { description: 'Matching document chunks ordered by relevance.' }, }), { diff --git a/apps/sim/lib/api/contracts/v2/shared.ts b/apps/sim/lib/api/contracts/v2/shared.ts index 978bd54249b..8b187a34d51 100644 --- a/apps/sim/lib/api/contracts/v2/shared.ts +++ b/apps/sim/lib/api/contracts/v2/shared.ts @@ -147,7 +147,7 @@ export const v2CursorListResponse = (itemSchema: T) => .string() .nullable() .describe( - 'Opaque cursor for the next page, or null when no more items remain. Always null on a full-set list, which returns its whole result set in one response.' + 'Opaque cursor for the next page: send it back as `cursor` to continue, and stop when it is null. Most v2 lists page, so null means the last page was reached. A few are full-set lists that return their whole bounded result in one response and therefore always report null; those say so in the operation description. Either way, null means there is nothing further to fetch — never construct a cursor yourself.' ), }) diff --git a/apps/sim/lib/api/contracts/v2/uploads.ts b/apps/sim/lib/api/contracts/v2/uploads.ts index 6219eab8247..8cb2d1803eb 100644 --- a/apps/sim/lib/api/contracts/v2/uploads.ts +++ b/apps/sim/lib/api/contracts/v2/uploads.ts @@ -32,6 +32,9 @@ export const v2PutUploadTransferSchema = z headers: z .record(z.string(), z.string()) .describe('Headers that must be included with the upload request.'), + expiresAt: v2TimestampSchema.describe( + "ISO 8601 expiration time for this signed URL. This is the URL's own expiry and is normally earlier than the upload session's expiresAt: the session stays open for later part, status, completion, and abort requests, but the bytes must be uploaded before this time. Once it passes, the storage provider rejects the upload and a new upload session must be created." + ), }) .strict() .meta({ diff --git a/apps/sim/lib/api/contracts/v2/workflow-run-status.test.ts b/apps/sim/lib/api/contracts/v2/workflow-run-status.test.ts new file mode 100644 index 00000000000..55801f4f3e1 --- /dev/null +++ b/apps/sim/lib/api/contracts/v2/workflow-run-status.test.ts @@ -0,0 +1,45 @@ +import { describe, expect, it } from 'vitest' +import { + v2WorkflowRunListStatusValueSchema, + v2WorkflowRunStatusFilterSchema, + v2WorkflowRunStatusValueSchema, +} from '@/lib/api/contracts/v2/workflows' +import type { PersistedWorkflowExecutionStatus } from '@/lib/logs/types' + +/** + * The runtime mirror of the persisted union. `satisfies` keeps it honest against + * `PersistedWorkflowExecutionStatus`, and the `AssertNever` gate in the contract keeps + * that union honest against the reported enums, so a status added to the execution logger + * fails compilation in both places before it can 500 a response parse. + */ +const PERSISTED_STATUSES = [ + 'pending', + 'running', + 'redacting', + 'completed', + 'failed', + 'cancelled', +] as const satisfies readonly PersistedWorkflowExecutionStatus[] + +describe('v2 workflow run status schemas', () => { + it.each(PERSISTED_STATUSES)('reports the persisted status %s on both run endpoints', (status) => { + expect(v2WorkflowRunListStatusValueSchema.parse(status)).toBe(status) + expect(v2WorkflowRunStatusValueSchema.parse(status)).toBe(status) + }) + + it('reports the paused overlay on both run endpoints', () => { + expect(v2WorkflowRunListStatusValueSchema.parse('paused')).toBe('paused') + expect(v2WorkflowRunStatusValueSchema.parse('paused')).toBe('paused') + }) + + it('reports queued only where the job queue is consulted', () => { + expect(v2WorkflowRunStatusValueSchema.parse('queued')).toBe('queued') + expect(v2WorkflowRunListStatusValueSchema.safeParse('queued').success).toBe(false) + }) + + it('keeps the list filter narrower than the reported set', () => { + expect(v2WorkflowRunStatusFilterSchema.safeParse('redacting').success).toBe(false) + expect(v2WorkflowRunStatusFilterSchema.safeParse('queued').success).toBe(false) + expect(v2WorkflowRunStatusFilterSchema.parse('paused')).toBe('paused') + }) +}) diff --git a/apps/sim/lib/api/contracts/v2/workflows.ts b/apps/sim/lib/api/contracts/v2/workflows.ts index 27258ea444d..d5df0af5f4f 100644 --- a/apps/sim/lib/api/contracts/v2/workflows.ts +++ b/apps/sim/lib/api/contracts/v2/workflows.ts @@ -36,6 +36,7 @@ import { workflowIdParamsSchema, } from '@/lib/api/contracts/workflows' import { MAX_WORKFLOW_EXECUTION_TIMEOUT_SECONDS } from '@/lib/billing/execution-timeout-defaults' +import type { PersistedWorkflowExecutionStatus } from '@/lib/logs/types' export const V2_WORKFLOW_RUN_ID_HEADER = 'X-Run-Id' @@ -981,17 +982,56 @@ export const v2ResumeWorkflowContract = defineRouteContract({ }, }) -export const v2WorkflowRunStatusValueSchema = z.enum([ - 'queued', +/** + * Every status the execution logger can persist into `workflow_execution_logs.status`, + * including the transient `redacting` state written while a finished run's output is + * scrubbed. The column is free text and both run endpoints pass it straight through, so + * a value missing here fails the response parse — and because list validation is + * whole-page, one such row turns an entire page into a 500. `_ExhaustiveRunStatus` makes + * a future addition to the persisted union a compile error instead. + */ +const V2_PERSISTED_RUN_STATUSES = [ 'pending', 'running', + 'redacting', 'completed', 'failed', 'cancelled', - 'paused', -]) +] as const satisfies readonly PersistedWorkflowExecutionStatus[] + +type AssertNever = T +type _ExhaustiveRunStatus = AssertNever< + Exclude +> + +/** + * The list projection overlays `paused` onto the persisted status whenever the run has a + * `paused` or `partially_resumed` row in `paused_executions`. It cannot report `queued`: + * a run that is still only in the job queue has no log row to list. + */ +const V2_WORKFLOW_RUN_LIST_STATUSES = [...V2_PERSISTED_RUN_STATUSES, 'paused'] as const -export const v2WorkflowRunListStatusValueSchema = z.enum([ +const RUN_STATUS_DESCRIPTION = + 'Current or terminal run status. `redacting` is transient, reported while the output of a finished run is being scrubbed.' + +export const v2WorkflowRunListStatusValueSchema = z + .enum(V2_WORKFLOW_RUN_LIST_STATUSES) + .describe(RUN_STATUS_DESCRIPTION) + +/** + * The single-run read additionally consults the async job queue by deterministic job id, + * so a run accepted but not yet started reports `queued` rather than 404. + */ +export const v2WorkflowRunStatusValueSchema = z + .enum([...V2_WORKFLOW_RUN_LIST_STATUSES, 'queued']) + .describe(RUN_STATUS_DESCRIPTION) + +/** + * Statuses accepted by the run-list `status` filter. Narrower than the reported set on + * purpose: the filter compares against the same projection, and `redacting` is a + * sub-second window nothing can usefully page through. + */ +export const v2WorkflowRunStatusFilterSchema = z.enum([ 'pending', 'running', 'completed', @@ -1002,7 +1042,7 @@ export const v2WorkflowRunListStatusValueSchema = z.enum([ export const v2ListWorkflowRunsQuerySchema = z .object({ - status: v2WorkflowRunListStatusValueSchema.optional().describe('Filter by run status.'), + status: v2WorkflowRunStatusFilterSchema.optional().describe('Filter by run status.'), trigger: z .string() .min(1, 'trigger cannot be empty') @@ -1071,7 +1111,7 @@ export const v2WorkflowRunListItemSchema = z .object({ runId: v2WorkflowRunIdSchema, workflowId: z.string().describe('Workflow that produced the run.'), - status: v2WorkflowRunListStatusValueSchema.describe('Current or terminal run status.'), + status: v2WorkflowRunListStatusValueSchema, trigger: z.string().describe('Trigger type that started the run.'), startedAt: z .string() @@ -1120,7 +1160,7 @@ export const v2WorkflowRunStatusSchema = z .object({ runId: v2WorkflowRunIdSchema, workflowId: z.string().describe('Workflow that produced the run.'), - status: v2WorkflowRunStatusValueSchema.describe('Current or terminal run status.'), + status: v2WorkflowRunStatusValueSchema, trigger: z.string().nullable().describe('Trigger type, or null before the run is recorded.'), startedAt: z .string() diff --git a/apps/sim/lib/api/server/routes/v2-binary-route.ts b/apps/sim/lib/api/server/routes/v2-binary-route.ts index 294fd3ca77d..67cc35af1f6 100644 --- a/apps/sim/lib/api/server/routes/v2-binary-route.ts +++ b/apps/sim/lib/api/server/routes/v2-binary-route.ts @@ -12,6 +12,7 @@ import { type V2RateLimitPolicy, V2RouteInfrastructureError, type v2ApiKeyAuth, + v2PayloadTooLargeResponse, } from '@/lib/api/server/routes/v2-json-route' import { parseRequest } from '@/lib/api/server/validation' import type { ApplicationOperation } from '@/lib/core/application' @@ -58,6 +59,7 @@ export function defineV2BinaryRoute< if (!admission.success) return admission.response const parsed = await parseRequest(options.contract, request, context ?? {}, { + payloadTooLargeResponse: v2PayloadTooLargeResponse, validationErrorResponse: v2ValidationError, }) if (!parsed.success) return parsed.response diff --git a/apps/sim/lib/api/server/routes/v2-body-lifecycle-route.ts b/apps/sim/lib/api/server/routes/v2-body-lifecycle-route.ts index 996194751ca..f5e04983e58 100644 --- a/apps/sim/lib/api/server/routes/v2-body-lifecycle-route.ts +++ b/apps/sim/lib/api/server/routes/v2-body-lifecycle-route.ts @@ -13,6 +13,7 @@ import { type V2RateLimitPolicy, V2RouteInfrastructureError, type v2ApiKeyAuth, + v2PayloadTooLargeResponse, } from '@/lib/api/server/routes/v2-json-route' import { type ParsedRequest, @@ -136,6 +137,7 @@ export function defineV2BodyLifecycleRoute< if (!routeAdmission.success) return routeAdmission.response const parsed = await parseRequest(options.contract, request, context ?? {}, { + payloadTooLargeResponse: v2PayloadTooLargeResponse, ...options.parseOptions, validationErrorResponse: v2ValidationError, }) diff --git a/apps/sim/lib/api/server/routes/v2-json-route.test.ts b/apps/sim/lib/api/server/routes/v2-json-route.test.ts index 43cc24f26ca..62b6e8f6edf 100644 --- a/apps/sim/lib/api/server/routes/v2-json-route.test.ts +++ b/apps/sim/lib/api/server/routes/v2-json-route.test.ts @@ -13,7 +13,7 @@ import { NextRequest, NextResponse } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' import { z } from 'zod' import { defineRouteContract } from '@/lib/api/contracts' -import type { ParsedRequest } from '@/lib/api/server/validation' +import type { ParsedRequest, ParseRequestOptions } from '@/lib/api/server/validation' import type { OperationUseCase } from '@/lib/core/application' import { OrchestrationError } from '@/lib/core/orchestration/types' import { HttpError } from '@/lib/core/utils/http-error' @@ -88,6 +88,7 @@ interface HandlerOverrides { }) => void | Promise present?: (result: Result) => { data: { value: string } } | Promise<{ data: { value: string } }> statusForResult?: (result: Result) => number + parseOptions?: Omit } function createHandler(overrides: HandlerOverrides = {}) { @@ -111,6 +112,7 @@ function createHandler(overrides: HandlerOverrides = {}) { present: overrides.present ?? ((result) => ({ data: result })), onSuccess: overrides.onSuccess, statusForResult: overrides.statusForResult, + parseOptions: overrides.parseOptions, }) } @@ -122,6 +124,22 @@ function request(body: unknown = { value: 'ok' }): NextRequest { }) } +/** + * A body the size guard rejects on the declared `content-length` alone, which is + * how an oversized request is refused before any of it is buffered. + */ +function oversizedRequest(maxBodyBytes: number): NextRequest { + return new NextRequest('http://localhost/api/v2/widgets', { + method: 'POST', + headers: { + 'content-type': 'application/json', + 'x-api-key': 'secret', + 'content-length': String(maxBodyBytes + 1), + }, + body: JSON.stringify({ value: 'ok' }), + }) +} + describe('defineV2JsonRoute', () => { beforeEach(() => { vi.clearAllMocks() @@ -418,4 +436,36 @@ describe('defineV2JsonRoute', () => { error: { code: 'INTERNAL_ERROR', message: 'Internal server error' }, }) }) + + it('renders an oversized body in the v2 error envelope without a per-route override', async () => { + const maxBodyBytes = 64 + const response = await createHandler({ parseOptions: { maxBodyBytes } })( + oversizedRequest(maxBodyBytes) + ) + + expect(response.status).toBe(413) + await expect(response.json()).resolves.toEqual({ + error: { code: 'PAYLOAD_TOO_LARGE', message: 'Request body is too large' }, + }) + expect(response.headers.get('Cache-Control')).toBe('private, no-store') + }) + + it('lets a route override the default payload-too-large response', async () => { + const maxBodyBytes = 64 + const response = await createHandler({ + parseOptions: { + maxBodyBytes, + payloadTooLargeResponse: () => + NextResponse.json( + { error: { code: 'PAYLOAD_TOO_LARGE', message: 'Import archive is too large' } }, + { status: 413 } + ), + }, + })(oversizedRequest(maxBodyBytes)) + + expect(response.status).toBe(413) + await expect(response.json()).resolves.toEqual({ + error: { code: 'PAYLOAD_TOO_LARGE', message: 'Import archive is too large' }, + }) + }) }) diff --git a/apps/sim/lib/api/server/routes/v2-json-route.ts b/apps/sim/lib/api/server/routes/v2-json-route.ts index 109338c0cd7..434d46c9918 100644 --- a/apps/sim/lib/api/server/routes/v2-json-route.ts +++ b/apps/sim/lib/api/server/routes/v2-json-route.ts @@ -101,6 +101,16 @@ export const v2RateLimits = { } satisfies V2RateLimitPolicy, } as const +/** + * Default `413` for every v2 JSON route. `parseRequest` otherwise falls back to its + * framework-level body, a bare `{ "error": string }` that carries no `error.code`, is not + * the v2 error envelope, and omits the `Cache-Control: private, no-store` every other v2 + * response sets. Declared here so a route only has to set `parseOptions.maxBodyBytes` to + * get a correct 413; a route that supplies its own `payloadTooLargeResponse` still wins. + */ +export const v2PayloadTooLargeResponse = () => + v2Error('PAYLOAD_TOO_LARGE', 'Request body is too large') + export interface V2ErrorPolicy { render(error: unknown): NextResponse | null } @@ -240,6 +250,7 @@ export function defineV2JsonRoute< } const parsed = await parseRequest(options.contract, request, context ?? {}, { + payloadTooLargeResponse: v2PayloadTooLargeResponse, ...options.parseOptions, validationErrorResponse: v2ValidationError, }) diff --git a/apps/sim/lib/billing/application/billing-use-cases.test.ts b/apps/sim/lib/billing/application/billing-use-cases.test.ts index 08bc56447c6..af2faa89b39 100644 --- a/apps/sim/lib/billing/application/billing-use-cases.test.ts +++ b/apps/sim/lib/billing/application/billing-use-cases.test.ts @@ -25,10 +25,12 @@ const mocks = vi.hoisted(() => ({ getWorkspaceUsageLogs: vi.fn(), recordAudit: vi.fn(), canUserManageWorkspaceBilling: vi.fn(), + canUserManageBillingEntity: vi.fn(), })) vi.mock('@/lib/billing/core/workspace-billing-authority', () => ({ canUserManageWorkspaceBilling: mocks.canUserManageWorkspaceBilling, + canUserManageBillingEntity: mocks.canUserManageBillingEntity, })) vi.mock('@/lib/workspaces/application/workspace-context', () => ({ @@ -100,6 +102,7 @@ describe('billing application use cases', () => { mocks.loadWorkspace.mockResolvedValue(workspaceContext) mocks.resolvePermission.mockResolvedValue('read') mocks.canUserManageWorkspaceBilling.mockResolvedValue(false) + mocks.canUserManageBillingEntity.mockResolvedValue(false) mocks.checkUsageStatus.mockResolvedValue({ currentUsage: 1, limit: 10, isExceeded: false }) mocks.checkAttributedBlocks.mockResolvedValue({ blocked: false }) mocks.toUsageLimitSubscription.mockReturnValue(null) @@ -289,6 +292,81 @@ describe('billing application use cases', () => { expect(result.storage).not.toBeNull() }) + /** + * `getHighestPrioritySubscription` resolves an organization subscription from + * any `member` row regardless of role, so dropping `workspaceId` must not + * hand a plain member the organization-wide pool the workspace branch + * withholds. + */ + it('withholds the organization pool from an account caller who cannot manage it', async () => { + mocks.getSubscription.mockResolvedValue({ plan: 'team', referenceId: 'organization-1' }) + mocks.deriveBillingContext.mockReturnValue({ + billingEntity: { type: 'organization', id: 'organization-1' }, + billingPeriod: { + start: new Date('2026-01-01T00:00:00Z'), + end: new Date('2026-02-01T00:00:00Z'), + }, + }) + mocks.canUserManageBillingEntity.mockResolvedValue(false) + mocks.checkBillingBlocked.mockResolvedValue({ blocked: false }) + mocks.checkBillingEntityBlocked.mockResolvedValue({ blocked: false }) + + const result = await getBillingStatus.execute({ principal: personalPrincipal, input: {} }) + + expect(result.credits).toBeNull() + expect(result.storage).toBeNull() + expect(result).toMatchObject({ workspaceId: null, plan: 'team', status: 'active' }) + expect(mocks.canUserManageBillingEntity).toHaveBeenCalledWith( + { type: 'organization', id: 'organization-1' }, + 'user-1' + ) + expect(mocks.getUserStorageUsage).not.toHaveBeenCalled() + expect(mocks.getUserStorageLimit).not.toHaveBeenCalled() + }) + + it('still reports an exceeded organization limit to an account caller who cannot read it', async () => { + mocks.getSubscription.mockResolvedValue({ plan: 'team', referenceId: 'organization-1' }) + mocks.deriveBillingContext.mockReturnValue({ + billingEntity: { type: 'organization', id: 'organization-1' }, + billingPeriod: { + start: new Date('2026-01-01T00:00:00Z'), + end: new Date('2026-02-01T00:00:00Z'), + }, + }) + mocks.canUserManageBillingEntity.mockResolvedValue(false) + mocks.checkBillingBlocked.mockResolvedValue({ blocked: false }) + mocks.checkBillingEntityBlocked.mockResolvedValue({ blocked: false }) + mocks.checkUsageStatus.mockResolvedValue({ currentUsage: 40, limit: 10, isExceeded: true }) + + const result = await getBillingStatus.execute({ principal: personalPrincipal, input: {} }) + + expect(result.status).toBe('limit_exceeded') + expect(result.credits).toBeNull() + }) + + it('projects the organization pool to an account caller who administers it', async () => { + mocks.getSubscription.mockResolvedValue({ plan: 'team', referenceId: 'organization-1' }) + mocks.deriveBillingContext.mockReturnValue({ + billingEntity: { type: 'organization', id: 'organization-1' }, + billingPeriod: { + start: new Date('2026-01-01T00:00:00Z'), + end: new Date('2026-02-01T00:00:00Z'), + }, + }) + mocks.canUserManageBillingEntity.mockResolvedValue(true) + mocks.checkBillingBlocked.mockResolvedValue({ blocked: false }) + mocks.checkBillingEntityBlocked.mockResolvedValue({ blocked: false }) + + const result = await getBillingStatus.execute({ principal: personalPrincipal, input: {} }) + + expect(result.credits).toEqual({ used: 200, limit: 2_000, remaining: 1_800 }) + expect(result.storage).toEqual({ + usedBytes: 5_242_880, + limitBytes: 1_073_741_824, + percentUsed: 0.48828125, + }) + }) + it('uses the personal principal as account authority', async () => { mocks.getSubscription.mockResolvedValue({ plan: 'pro' }) mocks.deriveBillingContext.mockReturnValue({ diff --git a/apps/sim/lib/billing/application/get-billing-status.ts b/apps/sim/lib/billing/application/get-billing-status.ts index e3520a38178..4ff70ea0fc3 100644 --- a/apps/sim/lib/billing/application/get-billing-status.ts +++ b/apps/sim/lib/billing/application/get-billing-status.ts @@ -11,9 +11,11 @@ import { resolveSystemBillingAttribution, toUsageLimitSubscription, } from '@/lib/billing/core/billing-attribution' +import type { HighestPrioritySubscription } from '@/lib/billing/core/plan' import { getHighestPrioritySubscription } from '@/lib/billing/core/subscription' -import { deriveBillingContext } from '@/lib/billing/core/usage-log' +import { type BillingEntity, deriveBillingContext } from '@/lib/billing/core/usage-log' import { + canUserManageBillingEntity, canUserManageWorkspaceBilling, type WorkspaceBillingAuthorityContext, } from '@/lib/billing/core/workspace-billing-authority' @@ -45,7 +47,8 @@ export interface BillingStorageStatus { /** * `credits` and `storage` describe the resolved payer's pooled allowances, not * the caller's own consumption, so they are only projected to a caller who may - * manage that payer's billing — see {@link canReadPayerPool}. Every other + * manage that payer's billing — see {@link canReadPayerPool} on the workspace + * branch and {@link isSelfBillingEntity} on the account branch. Every other * caller reads them as `null` while still seeing the plan and standing the * workspace UI already shows them. */ @@ -105,6 +108,30 @@ async function canReadPayerPool( return canUserManageWorkspaceBilling(workspace, principal.userId) } +/** + * Whether the account-branch caller is themselves the payer. + * + * Omitting `workspaceId` selects account scope, whose operation policy is + * `personal_self`. That reads naturally as "my own billing", and it is — + * except that {@link getHighestPrioritySubscription} resolves a subscription + * from any `member` row regardless of role, so a plain non-admin member of an + * organization lands on the organization's pooled subscription. Credits then + * come from the pooled organization usage and storage from the organization's + * counter, which is the whole organization's consumption rather than the + * caller's own. + * + * The pool is therefore gated exactly as the workspace branch gates it, rather + * than the branch being forced back to the caller's personal subscription: + * plan, period, and standing stay resolved from the payer that actually funds + * the caller, so a monitor still sees `limit_exceeded` and `billing_blocked` + * truthfully, while only the pooled figures require authority over the payer. + * A caller with no organization payer is their own payer and keeps reading + * their own credits and storage unchanged. + */ +function isSelfBillingEntity(billingEntity: Readonly, userId: string): boolean { + return billingEntity.type === 'user' && billingEntity.id === userId +} + /** Only invoked once payer-pool disclosure is authorized. */ async function resolvePayerStorage(workspaceId: string): Promise { const storageContext = await resolveStorageBillingContext(workspaceId) @@ -112,6 +139,18 @@ async function resolvePayerStorage(workspaceId: string): Promise { + const [usedBytes, limitBytes] = await Promise.all([ + getUserStorageUsage(userId, subscription), + getUserStorageLimit(userId, subscription), + ]) + return storageStatus(usedBytes, limitBytes) +} + export const getBillingStatus = defineAuthorizedBillingReadUseCase({ operation: billingOperations.readStatus, requestedWorkspaceId: (input: GetBillingStatusInput) => input.workspaceId, @@ -143,14 +182,15 @@ export const getBillingStatus = defineAuthorizedBillingReadUseCase({ const subscription = await getHighestPrioritySubscription(scope.userId) const { billingEntity, billingPeriod } = deriveBillingContext(scope.userId, subscription) - const [usage, actorBlock, payerBlock, storageUsedBytes, storageLimitBytes] = await Promise.all([ + const isSelfPayer = isSelfBillingEntity(billingEntity, scope.userId) + const canViewPayerPool = isSelfPayer + ? true + : await canUserManageBillingEntity(billingEntity, scope.userId) + const [usage, actorBlock, payerBlock, storage] = await Promise.all([ checkUsageStatus(scope.userId, subscription), checkBillingBlocked(scope.userId), - billingEntity.type === 'user' && billingEntity.id === scope.userId - ? Promise.resolve({ blocked: false }) - : checkBillingEntityBlocked(billingEntity), - getUserStorageUsage(scope.userId, subscription), - getUserStorageLimit(scope.userId, subscription), + isSelfPayer ? Promise.resolve({ blocked: false }) : checkBillingEntityBlocked(billingEntity), + canViewPayerPool ? resolveAccountStorage(scope.userId, subscription) : null, ]) return { workspaceId: null, @@ -165,8 +205,8 @@ export const getBillingStatus = defineAuthorizedBillingReadUseCase({ : usage.isExceeded ? 'limit_exceeded' : 'active', - credits: creditsStatus(usage), - storage: storageStatus(storageUsedBytes, storageLimitBytes), + credits: canViewPayerPool ? creditsStatus(usage) : null, + storage, } }, }) diff --git a/apps/sim/lib/billing/core/workspace-billing-authority.ts b/apps/sim/lib/billing/core/workspace-billing-authority.ts index c536dbe8f53..c1ee6a87594 100644 --- a/apps/sim/lib/billing/core/workspace-billing-authority.ts +++ b/apps/sim/lib/billing/core/workspace-billing-authority.ts @@ -1,3 +1,4 @@ +import type { BillingEntity } from '@/lib/billing/core/usage-log' import { isOrganizationAdminOrOwner } from '@/lib/workspaces/permissions/utils' /** @@ -8,6 +9,27 @@ export interface WorkspaceBillingAuthorityContext { billedAccountUserId: string } +/** + * Resolves whether a user holds billing authority over the entity that funds a + * pool — the single predicate every payer-pool disclosure is gated on. + * + * An organization pool answers to that organization's admins and owners; a + * personal pool answers only to the account holder it belongs to. Membership + * alone is never sufficient: an organization's credit and storage allowances + * are pooled across every member and workspace it funds, so projecting them to + * a plain member discloses the whole organization's consumption. + */ +export async function canUserManageBillingEntity( + billingEntity: Readonly, + userId: string +): Promise { + if (billingEntity.type === 'organization') { + return isOrganizationAdminOrOwner(userId, billingEntity.id) + } + + return billingEntity.id === userId +} + /** * Server-side counterpart of `canManageWorkspaceBilling`, resolved from * canonical workspace state instead of a viewer-facing host context. @@ -22,9 +44,10 @@ export async function canUserManageWorkspaceBilling( context: WorkspaceBillingAuthorityContext, userId: string ): Promise { - if (context.workspaceOrganizationId) { - return isOrganizationAdminOrOwner(userId, context.workspaceOrganizationId) - } - - return context.billedAccountUserId === userId + return canUserManageBillingEntity( + context.workspaceOrganizationId + ? { type: 'organization', id: context.workspaceOrganizationId } + : { type: 'user', id: context.billedAccountUserId }, + userId + ) } diff --git a/apps/sim/lib/execution/cancel-workflow-execution.test.ts b/apps/sim/lib/execution/cancel-workflow-execution.test.ts index d630ed7e631..77d7da49e7f 100644 --- a/apps/sim/lib/execution/cancel-workflow-execution.test.ts +++ b/apps/sim/lib/execution/cancel-workflow-execution.test.ts @@ -10,6 +10,7 @@ const { mockCancelWorkflowGroupExecution, mockCaptureServerEvent, mockClearPausedCancellationIntent, + mockCompletePausedCancellation, mockGetJobQueue, mockGetPausedCancellationStatus, mockMarkExecutionCancelled, @@ -24,6 +25,7 @@ const { mockCancelWorkflowGroupExecution: vi.fn(), mockCaptureServerEvent: vi.fn(), mockClearPausedCancellationIntent: vi.fn(), + mockCompletePausedCancellation: vi.fn(), mockGetJobQueue: vi.fn(), mockGetPausedCancellationStatus: vi.fn(), mockMarkExecutionCancelled: vi.fn(), @@ -91,7 +93,7 @@ vi.mock('@/lib/workflows/executor/human-in-the-loop-manager', () => ({ getPausedCancellationStatus: mockGetPausedCancellationStatus, blockQueuedResumesForCancellation: mockBlockQueuedResumes, clearPausedCancellationIntent: mockClearPausedCancellationIntent, - completePausedCancellation: vi.fn(), + completePausedCancellation: mockCompletePausedCancellation, }, })) @@ -117,6 +119,7 @@ describe('cancelWorkflowExecution', () => { mockAbortManualExecution.mockReturnValue(false) mockBlockQueuedResumes.mockResolvedValue(undefined) mockClearPausedCancellationIntent.mockResolvedValue(undefined) + mockCompletePausedCancellation.mockResolvedValue(true) mockReleaseExecutionSlot.mockResolvedValue(undefined) mockPublishWorkflowGroupCancellationEvent.mockResolvedValue(undefined) mockGetJobQueue.mockResolvedValue({ @@ -173,18 +176,101 @@ describe('cancelWorkflowExecution', () => { it.each([ [{ kind: 'conflict' as const, status: 'completed' }, 'cannot be cancelled while completed'], [{ kind: 'not_workflow_group' as const }, 'no longer the active table execution'], - ])('reports a refused workflow-group cell claim as a conflict', async (outcome, message) => { + ])( + 'releases the reservation before reporting a refused workflow-group cell claim as a conflict', + async (outcome, message) => { + mockResolveWorkflowExecutionOwnership.mockResolvedValue({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: 'workspace-1', + }) + mockCancelWorkflowGroupExecution.mockResolvedValue(outcome) + + await expect(cancelWorkflowExecution(INPUT)).rejects.toMatchObject({ + code: 'conflict', + message: expect.stringContaining(message), + }) + expect(mockPublishWorkflowGroupCancellationEvent).not.toHaveBeenCalled() + expect(mockReleaseExecutionSlot).toHaveBeenCalledWith('execution-1') + } + ) + + it.each([ + [{ kind: 'conflict' as const, status: 'completed' }], + [{ kind: 'not_workflow_group' as const }], + ])( + 'keeps the reservation held when a refused claim follows a paused cancellation', + async (outcome) => { + mockResolveWorkflowExecutionOwnership.mockResolvedValue({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: 'workspace-1', + }) + mockBeginPausedCancellation.mockResolvedValue(true) + mockCancelWorkflowGroupExecution.mockResolvedValue(outcome) + + await expect(cancelWorkflowExecution(INPUT)).rejects.toMatchObject({ code: 'conflict' }) + expect(mockReleaseExecutionSlot).not.toHaveBeenCalled() + } + ) + + it.each([ + [{ kind: 'conflict' as const, status: 'completed' }], + [{ kind: 'not_workflow_group' as const }], + ])( + 'keeps the reservation held when a refused claim follows a failed cancellation', + async (outcome) => { + mockResolveWorkflowExecutionOwnership.mockResolvedValue({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: 'workspace-1', + }) + mockMarkExecutionCancelled.mockResolvedValue({ + durablyRecorded: false, + reason: 'redis_unavailable', + }) + mockCancelWorkflowGroupExecution.mockResolvedValue(outcome) + + await expect(cancelWorkflowExecution(INPUT)).rejects.toMatchObject({ code: 'conflict' }) + expect(mockReleaseExecutionSlot).not.toHaveBeenCalled() + } + ) + + it('releases the reservation and rethrows when the workflow-group cancel fails unexpectedly', async () => { mockResolveWorkflowExecutionOwnership.mockResolvedValue({ belongsToWorkflow: true, workflowGroupWorkspaceId: 'workspace-1', }) - mockCancelWorkflowGroupExecution.mockResolvedValue(outcome) + const failure = new Error('Workflow-group cancellation lost its locked workflow-log claim') + mockCancelWorkflowGroupExecution.mockRejectedValue(failure) - await expect(cancelWorkflowExecution(INPUT)).rejects.toMatchObject({ - code: 'conflict', - message: expect.stringContaining(message), - }) + await expect(cancelWorkflowExecution(INPUT)).rejects.toBe(failure) + expect(mockReleaseExecutionSlot).toHaveBeenCalledWith('execution-1') expect(mockPublishWorkflowGroupCancellationEvent).not.toHaveBeenCalled() + expect(mockUpdateSet).not.toHaveBeenCalled() + }) + + it('keeps the reservation held when an unexpected workflow-group failure follows a paused cancellation', async () => { + mockResolveWorkflowExecutionOwnership.mockResolvedValue({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: 'workspace-1', + }) + mockBeginPausedCancellation.mockResolvedValue(true) + mockCancelWorkflowGroupExecution.mockRejectedValue(new Error('serialization conflict')) + + await expect(cancelWorkflowExecution(INPUT)).rejects.toThrow('serialization conflict') + expect(mockReleaseExecutionSlot).not.toHaveBeenCalled() + }) + + it('keeps the reservation held when an unexpected workflow-group failure follows a failed cancellation', async () => { + mockResolveWorkflowExecutionOwnership.mockResolvedValue({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: 'workspace-1', + }) + mockMarkExecutionCancelled.mockResolvedValue({ + durablyRecorded: false, + reason: 'redis_unavailable', + }) + mockCancelWorkflowGroupExecution.mockRejectedValue(new Error('serialization conflict')) + + await expect(cancelWorkflowExecution(INPUT)).rejects.toThrow('serialization conflict') expect(mockReleaseExecutionSlot).not.toHaveBeenCalled() }) diff --git a/apps/sim/lib/execution/cancel-workflow-execution.ts b/apps/sim/lib/execution/cancel-workflow-execution.ts index 87f5837b7c7..79ed19d0823 100644 --- a/apps/sim/lib/execution/cancel-workflow-execution.ts +++ b/apps/sim/lib/execution/cancel-workflow-execution.ts @@ -287,19 +287,68 @@ export async function cancelWorkflowExecution( ) } - const groupCancellation = workflowGroupWorkspaceId - ? await cancelWorkflowGroupExecution({ + const success = + (isPausedCancellationPath + ? pausedCancelled && pausedCancellationPublished + : cancellation.durablyRecorded || queuedJobCancelled) || locallyAborted + + /** + * Frees the plan concurrency reservation once the stop-the-work effects above + * have actually taken. The paused path keeps its reservation because a paused + * run never held an in-flight slot to give back, and an unsuccessful ordinary + * cancel keeps it because the run may still be executing. + */ + const releaseSlotForStoppedExecution = async (): Promise => { + if (!success || isPausedCancellationPath) return + await releaseExecutionSlot(executionId).catch((error) => { + logger.warn('Failed to release reservation after execution cancellation', { + executionId, + error, + }) + }) + } + + /** + * The sidecar transition can fail outright — a lost claim, a serialization + * conflict, a connection blip. The stop-the-work effects above have already + * fired, so the run is going down regardless and the reservation must not be + * stranded; but the cell is left in an unknown state, so the failure is + * re-thrown rather than swallowed into a success-shaped result. + */ + let groupCancellation: Awaited> | null = null + if (workflowGroupWorkspaceId) { + try { + groupCancellation = await cancelWorkflowGroupExecution({ workspaceId: workflowGroupWorkspaceId, workflowId, executionId, }) - : null + } catch (error) { + logger.error('Workflow group execution cancellation failed unexpectedly', { + executionId, + error, + }) + await releaseSlotForStoppedExecution() + throw error + } + } + /** + * Both refusals mean the cell claim was lost, never that the run is still + * going: the sidecar conflicts only on a terminal workflow log or a terminal + * cell, and `not_workflow_group` means the log is not a group run at all. + * Every refusal is a terminal-or-absent state that carries no evidence of + * liveness, so nothing is left running to hold the reservation and it is + * released before the 409 rather than left to expire. (The Redis abort record + * is reversible — see `clearExecutionCancellation` — but a refusal gives no + * reason to reverse it.) + */ if (groupCancellation?.kind === 'conflict') { logger.warn('Workflow group execution could not be cancelled', { executionId, status: groupCancellation.status, }) + await releaseSlotForStoppedExecution() throw new OrchestrationError( 'conflict', `Workflow group execution cannot be cancelled while ${groupCancellation.status}` @@ -307,6 +356,7 @@ export async function cancelWorkflowExecution( } if (groupCancellation?.kind === 'not_workflow_group') { logger.warn('Workflow group execution is no longer the active table execution', { executionId }) + await releaseSlotForStoppedExecution() throw new OrchestrationError( 'conflict', 'Workflow group execution is no longer the active table execution' @@ -341,23 +391,11 @@ export async function cancelWorkflowExecution( } } - const success = - (isPausedCancellationPath - ? pausedCancelled && pausedCancellationPublished - : cancellation.durablyRecorded || queuedJobCancelled) || locallyAborted - if (groupCancellationToPublish && success) { await publishWorkflowGroupCancellationEvent(groupCancellationToPublish, executionId) } - if (success && !isPausedCancellationPath) { - await releaseExecutionSlot(executionId).catch((error) => { - logger.warn('Failed to release reservation after execution cancellation', { - executionId, - error, - }) - }) - } + await releaseSlotForStoppedExecution() if (success && input.captureAnalytics !== false) { captureServerEvent( diff --git a/apps/sim/lib/folders/cascade.test.ts b/apps/sim/lib/folders/cascade.test.ts index e177dd3c308..5c4af123168 100644 --- a/apps/sim/lib/folders/cascade.test.ts +++ b/apps/sim/lib/folders/cascade.test.ts @@ -14,6 +14,7 @@ import { } from '@/lib/folders/cascade' import { FOLDER_RESOURCES, type FolderResourceConfig } from '@/lib/folders/config' import { FolderCollectionLimitExceededError } from '@/lib/folders/errors' +import { folderResourceSupportsLocking } from '@/lib/folders/resource-traits' import { folderMutationStatus } from '@/lib/folders/status' interface SelectCall { @@ -440,6 +441,16 @@ describe('FOLDER_RESOURCES', () => { expect(lockable).toEqual(['workflow']) }) + it('answers the lock question identically whether asked of the config or the trait', () => { + // Routes read `folderResourceSupportsLocking` (the leaf module, so a lock check costs no + // db-schema graph) while orchestration reads `config.supportsLocking`. Declared twice they + // drift silently: a newly lockable resource would lock in orchestration but not in the + // routes that guard it. + for (const config of Object.values(FOLDER_RESOURCES)) { + expect(config.supportsLocking).toBe(folderResourceSupportsLocking(config.resourceType)) + } + }) + it('guards the delete of resources that gate their own deletion', () => { // Tables refuse deletion while delete-locked; deleting the folder around one must not // become a way around that control. diff --git a/apps/sim/lib/folders/config.ts b/apps/sim/lib/folders/config.ts index 8dea1441f48..cca02a55313 100644 --- a/apps/sim/lib/folders/config.ts +++ b/apps/sim/lib/folders/config.ts @@ -11,6 +11,10 @@ import { import { eq, type SQL } from 'drizzle-orm' import type { PgColumn, PgTable } from 'drizzle-orm/pg-core' import type { FolderResourceType } from '@/lib/api/contracts/folders' +import { + FOLDER_RESOURCE_LABELS, + FOLDER_RESOURCE_SUPPORTS_LOCKING, +} from '@/lib/folders/resource-traits' /** * Counts of cascaded resources returned by a folder delete/restore, keyed per resource @@ -97,8 +101,13 @@ export interface FolderResourceConfig { * Declared here rather than checked as `resourceType === 'workflow'` at each call site, so * every surface that touches locking asks the same question and a future lockable resource * is one flag rather than a hunt through routes. + * + * Required, and every entry composes it from {@link FOLDER_RESOURCE_SUPPORTS_LOCKING} — the + * same treatment as `label`. Routes read the trait module directly (it is a leaf, so a + * lock check costs no db-schema graph) while orchestration reads this field; declaring the + * value twice would let those two answers drift. */ - supportsLocking?: boolean + supportsLocking: boolean /** Narrows which rows of `table` participate in folder membership at all. */ scope?: SQL /** @@ -379,7 +388,7 @@ async function guardLockedTables({ export const FOLDER_RESOURCES: Record = { workflow: { resourceType: 'workflow', - label: 'workflow', + label: FOLDER_RESOURCE_LABELS.workflow, countKey: 'workflows', table: workflow, idColumn: workflow.id, @@ -440,7 +449,7 @@ export const FOLDER_RESOURCES: Record >, }, ], - supportsLocking: true, + supportsLocking: FOLDER_RESOURCE_SUPPORTS_LOCKING.workflow, archiveChildren: archiveWorkflowChildren, guardDelete: guardLastWorkflows, }, @@ -454,7 +463,8 @@ export const FOLDER_RESOURCES: Record */ file: { resourceType: 'file', - label: 'file', + label: FOLDER_RESOURCE_LABELS.file, + supportsLocking: FOLDER_RESOURCE_SUPPORTS_LOCKING.file, countKey: 'files', table: workspaceFiles, idColumn: workspaceFiles.id, @@ -472,7 +482,8 @@ export const FOLDER_RESOURCES: Record }, knowledge_base: { resourceType: 'knowledge_base', - label: 'knowledge base', + label: FOLDER_RESOURCE_LABELS.knowledge_base, + supportsLocking: FOLDER_RESOURCE_SUPPORTS_LOCKING.knowledge_base, countKey: 'knowledgeBases', table: knowledgeBase, idColumn: knowledgeBase.id, @@ -489,7 +500,8 @@ export const FOLDER_RESOURCES: Record }, table: { resourceType: 'table', - label: 'table', + label: FOLDER_RESOURCE_LABELS.table, + supportsLocking: FOLDER_RESOURCE_SUPPORTS_LOCKING.table, countKey: 'tables', table: userTableDefinitions, idColumn: userTableDefinitions.id, diff --git a/apps/sim/lib/folders/errors.ts b/apps/sim/lib/folders/errors.ts index 525a9279ef9..04c025f8ae2 100644 --- a/apps/sim/lib/folders/errors.ts +++ b/apps/sim/lib/folders/errors.ts @@ -9,3 +9,22 @@ export class FolderCollectionLimitExceededError extends OrchestrationError { this.name = 'FolderCollectionLimitExceededError' } } + +/** + * Typed refusal for a create that would push a workspace's active folder tree + * past the ceiling its readers materialize under. + * + * Distinct from {@link FolderCollectionLimitExceededError}: that one reports a + * collection that is already too large to read, which is an infrastructure + * limit (413). This is a write the current state of the workspace conflicts + * with (409), and its message names the action the user can take. + */ +export class FolderCollectionFullError extends OrchestrationError { + constructor(label: string, maxRows: number) { + super( + 'conflict', + `This workspace has reached its limit of ${maxRows.toLocaleString('en-US')} ${label} folders. Delete folders you no longer need before creating another one.` + ) + this.name = 'FolderCollectionFullError' + } +} diff --git a/apps/sim/lib/folders/orchestration.test.ts b/apps/sim/lib/folders/orchestration.test.ts index d87b037d6e3..676b2c7144e 100644 --- a/apps/sim/lib/folders/orchestration.test.ts +++ b/apps/sim/lib/folders/orchestration.test.ts @@ -11,7 +11,8 @@ import { schemaMock, } from '@sim/testing' import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest' -import { FolderCollectionLimitExceededError } from '@/lib/folders/errors' +import { FolderCollectionFullError, FolderCollectionLimitExceededError } from '@/lib/folders/errors' +import { folderMutationStatus } from '@/lib/folders/status' const { mockArchiveFolderCascade, @@ -25,6 +26,7 @@ const { mockRestoreFolderRows, mockWouldCreateFolderCycle, mockLoadActiveFolderPathIndex, + mockAssertFolderCollectionHasRoom, resourceConfig, } = vi.hoisted(() => ({ mockArchiveFolderCascade: vi.fn(), @@ -38,6 +40,7 @@ const { mockRestoreFolderRows: vi.fn(), mockWouldCreateFolderCycle: vi.fn(), mockLoadActiveFolderPathIndex: vi.fn(), + mockAssertFolderCollectionHasRoom: vi.fn(), resourceConfig: { current: {} as Record }, })) @@ -64,6 +67,7 @@ vi.mock('@/lib/folders/naming', () => ({ deduplicateFolderName: mockDeduplicateF vi.mock('@/lib/folders/queries', () => ({ wouldCreateFolderCycle: mockWouldCreateFolderCycle, loadActiveFolderPathIndex: mockLoadActiveFolderPathIndex, + assertFolderCollectionHasRoom: mockAssertFolderCollectionHasRoom, })) vi.mock('@/lib/workspaces/permissions/utils', () => ({ @@ -140,6 +144,7 @@ beforeEach(() => { resetDbChainMock() setConfig() mockWouldCreateFolderCycle.mockResolvedValue(false) + mockAssertFolderCollectionHasRoom.mockResolvedValue(undefined) mockLoadActiveFolderPathIndex.mockResolvedValue({ rowById: new Map(), pathById: new Map(), @@ -321,6 +326,28 @@ describe('createFolder', () => { }) }) + /** + * The writer must agree with the bounded readers: without this the sidebar + * create path could push a workspace past `MAX_FOLDERS_PER_WORKSPACE`, after + * which every capped read fails on a state the product allowed to exist. + */ + it('refuses a create at the collection ceiling with a typed conflict, before inserting', async () => { + mockAssertFolderCollectionHasRoom.mockRejectedValueOnce( + new FolderCollectionFullError('table', 10_000) + ) + + const result = await createFolder(baseCreate) + + expect(result).toEqual({ + success: false, + error: + 'This workspace has reached its limit of 10,000 table folders. Delete folders you no longer need before creating another one.', + errorCode: 'conflict', + }) + expect(folderMutationStatus(result.errorCode)).toBe(409) + expect(dbChainMockFns.insert).not.toHaveBeenCalled() + }) + it('reports any other insert failure as internal rather than a name conflict', async () => { queueTableRows(schemaMock.folder, [{ minSortOrder: 0 }]) dbChainMockFns.returning.mockRejectedValueOnce(new Error('connection reset')) @@ -354,6 +381,8 @@ describe('path-owned folder mutations', () => { error: 'Folder path index exceeds the 10000 row limit', errorCode: 'payload_too_large', }) + // A collection too large to materialize is an infrastructure limit, not a fault. + expect(folderMutationStatus(result.errorCode)).toBe(413) expect(mockLoadActiveFolderPathIndex).toHaveBeenCalledWith( 'ws-1', 'workflow', diff --git a/apps/sim/lib/folders/orchestration.ts b/apps/sim/lib/folders/orchestration.ts index b08a8115916..a91a8a75e3c 100644 --- a/apps/sim/lib/folders/orchestration.ts +++ b/apps/sim/lib/folders/orchestration.ts @@ -27,7 +27,11 @@ import { parentFolderPath, requireNonRootFolderPath, } from '@/lib/folders/paths' -import { loadActiveFolderPathIndex, wouldCreateFolderCycle } from '@/lib/folders/queries' +import { + assertFolderCollectionHasRoom, + loadActiveFolderPathIndex, + wouldCreateFolderCycle, +} from '@/lib/folders/queries' import type { FolderMutationErrorCode } from '@/lib/folders/status' import { notifyFolderResourceChanged } from '@/lib/realtime/notify' @@ -572,6 +576,12 @@ export async function createFolder(params: CreateFolderParams): Promise { await acquireFolderMutationLock(tx, params.workspaceId, params.resourceType) + /** + * The path-owned create enforces the same ceiling from its index; this + * one has no index to count, so it asks the collection directly. Under + * the mutation lock the count cannot be raced past the cap. + */ + await assertFolderCollectionHasRoom(params.workspaceId, params.resourceType, tx) if (parentId) { const parentError = await assertParentFolderInWorkspace( params.resourceType, @@ -644,6 +654,12 @@ export async function createFolder(params: CreateFolderParams): Promise { }) /** - * The bound stays opt-in. Folder creation does not refuse at - * `MAX_FOLDERS_PER_WORKSPACE` on every path — `POST /api/folders` passes no - * `maxFolderRows` — so a workspace already over the cap must still be - * readable. Defaulting the bound would turn every path-index consumer into - * a hard failure for a state the product allows to exist. + * The bound stays opt-in. Creation now refuses at + * `MAX_FOLDERS_PER_WORKSPACE`, but a workspace that crossed the ceiling + * before that guard existed — or through a create path that still bypasses + * the orchestration engine — must stay readable. Defaulting the bound would + * turn every path-index consumer into a hard failure for a state that + * already exists in production data. */ it('leaves the read unbounded when no maxRows is given', async () => { queueTableRows(schemaMock.folder, [ @@ -237,6 +240,84 @@ describe('folder queries', () => { }) }) + /** + * The writer half of the ceiling the bounded readers enforce. Without it a + * workspace could be driven past `MAX_FOLDERS_PER_WORKSPACE`, after which + * every capped reader fails on a state the product allowed to exist. + */ + describe('assertFolderCollectionHasRoom', () => { + it('refuses a create once the active collection is at the ceiling', async () => { + queueTableRows(schemaMock.folder, [{ total: MAX_FOLDERS_PER_WORKSPACE }]) + + const rejection = expect(assertFolderCollectionHasRoom('ws-1', 'workflow')).rejects + await rejection.toBeInstanceOf(FolderCollectionFullError) + await rejection.toMatchObject({ + code: 'conflict', + message: + 'This workspace has reached its limit of 10,000 workflow folders. Delete folders you no longer need before creating another one.', + }) + }) + + it('still refuses a workspace that is already past the ceiling', async () => { + queueTableRows(schemaMock.folder, [{ total: MAX_FOLDERS_PER_WORKSPACE + 1 }]) + + await expect(assertFolderCollectionHasRoom('ws-1', 'table')).rejects.toBeInstanceOf( + FolderCollectionFullError + ) + }) + + it('allows a create below the ceiling and counts only this resource type', async () => { + queueTableRows(schemaMock.folder, [{ total: MAX_FOLDERS_PER_WORKSPACE - 1 }]) + + await expect(assertFolderCollectionHasRoom('ws-1', 'knowledge_base')).resolves.toBeUndefined() + + const where = whereAt(0) + expect(hasMockCondition(where, (n) => n.type === 'eq' && n.right === 'knowledge_base')).toBe( + true + ) + expect( + hasMockCondition( + where, + (n) => n.type === 'isNull' && n.column === schemaMock.folder.deletedAt + ) + ).toBe(true) + }) + + /** + * The bulk writers — recursive duplication, admin import, workspace fork — insert many + * folders per call. Charging one row and then writing N is the same overflow the ceiling + * exists to prevent, so the caller declares how many rows it is about to add. + */ + it('refuses a bulk create that would cross the ceiling from below it', async () => { + queueTableRows(schemaMock.folder, [{ total: MAX_FOLDERS_PER_WORKSPACE - 3 }]) + + const rejection = expect( + assertFolderCollectionHasRoom('ws-1', 'workflow', undefined, { additionalRows: 4 }) + ).rejects + await rejection.toBeInstanceOf(FolderCollectionFullError) + await rejection.toMatchObject({ code: 'conflict' }) + }) + + it('allows a bulk create that exactly fills the ceiling', async () => { + queueTableRows(schemaMock.folder, [{ total: MAX_FOLDERS_PER_WORKSPACE - 4 }]) + + await expect( + assertFolderCollectionHasRoom('ws-1', 'workflow', undefined, { additionalRows: 4 }) + ).resolves.toBeUndefined() + }) + + /** + * A copy that creates no folders is not a create. An over-cap workspace must still be + * able to run one, so the count is not even issued. + */ + it('skips the count entirely when no rows are being added', async () => { + await expect( + assertFolderCollectionHasRoom('ws-1', 'workflow', undefined, { additionalRows: 0 }) + ).resolves.toBeUndefined() + expect(dbChainMockFns.select).not.toHaveBeenCalled() + }) + }) + describe('toFolderApi', () => { it('serializes timestamps to ISO strings and preserves a null deletedAt', () => { expect(toFolderApi(ROW)).toMatchObject({ diff --git a/apps/sim/lib/folders/queries.ts b/apps/sim/lib/folders/queries.ts index b32e4d39a56..39863653795 100644 --- a/apps/sim/lib/folders/queries.ts +++ b/apps/sim/lib/folders/queries.ts @@ -1,11 +1,13 @@ import { db } from '@sim/db' import { folder } from '@sim/db/schema' -import { and, type Column, eq, isNotNull, isNull } from 'drizzle-orm' +import { and, type Column, count, eq, isNotNull, isNull } from 'drizzle-orm' import type { FolderApi, FolderResourceType } from '@/lib/api/contracts/folders' import { type ListSortOrder, listOrderBy, searchFilter } from '@/lib/api/list-query' import type { DbOrTx } from '@/lib/db/types' -import { FolderCollectionLimitExceededError } from '@/lib/folders/errors' +import { MAX_FOLDERS_PER_WORKSPACE } from '@/lib/folders/constants' +import { FolderCollectionFullError, FolderCollectionLimitExceededError } from '@/lib/folders/errors' import { buildFolderPathIndex, type FolderPathIndex, ROOT_FOLDER_PATH } from '@/lib/folders/paths' +import { folderResourceLabel } from '@/lib/folders/resource-traits' import type { FolderQueryScope } from '@/hooks/queries/utils/folder-keys' export type FolderSortBy = 'position' | 'name' | 'createdAt' | 'updatedAt' @@ -176,8 +178,9 @@ interface ListActiveFolderRowsOptions { * pass it get a throw of `FolderCollectionLimitExceededError` rather than a * truncated index, because a partial path index resolves real folder paths to * `undefined` and re-roots resources at the workspace root. The bound is not a - * default because folder creation does not enforce the same ceiling on every - * path, so a workspace can hold more rows than the cap and must still be read. + * default because folder creation only refuses at the ceiling on the paths that + * run through the orchestration engine, so a workspace can already hold more + * rows than the cap and must still be read. */ export async function loadActiveFolderPathIndex( workspaceId: string, @@ -203,6 +206,57 @@ export async function loadActiveFolderPathIndex( return buildFolderPathIndex(rows) } +export interface FolderCollectionRoomOptions { + /** + * How many folder rows the caller is about to insert. Bulk and recursive + * creates must pass their real row count: asserting room for one row and then + * inserting a whole subtree crosses the ceiling just as surely as ignoring it. + * Defaults to 1, the single-folder create. + */ + additionalRows?: number + maxRows?: number +} + +/** + * Refuses a folder create that would push a workspace's active tree past the + * ceiling the capped readers materialize under. + * + * Counts rather than loading the index: the writer only needs the cardinality, + * and a workspace already over the ceiling must not have its creates fail as a + * read error. Callers run this inside the folder mutation lock, which is what + * makes the count authoritative against a concurrent create. + * + * One query regardless of how many rows the caller is adding — a bulk writer + * passes `additionalRows` instead of calling this per row, which would be both + * O(n) queries and wrong (each call would see room for one more). + */ +export async function assertFolderCollectionHasRoom( + workspaceId: string, + resourceType: FolderResourceType, + tx: DbOrTx = db, + options: FolderCollectionRoomOptions = {} +): Promise { + const { additionalRows = 1, maxRows = MAX_FOLDERS_PER_WORKSPACE } = options + // A copy that creates no folders is not a create; an over-cap workspace must + // still be allowed to run it. + if (additionalRows <= 0) return + + const [row] = await tx + .select({ total: count() }) + .from(folder) + .where( + and( + eq(folder.workspaceId, workspaceId), + eq(folder.resourceType, resourceType), + isNull(folder.deletedAt) + ) + ) + + if (Number(row?.total ?? 0) + additionalRows > maxRows) { + throw new FolderCollectionFullError(folderResourceLabel(resourceType), maxRows) + } +} + /** Resolves a canonical folder path to its internal id; `/` resolves to the root sentinel. */ export function resolveFolderPathFromIndex( index: FolderPathIndex, diff --git a/apps/sim/lib/folders/resource-traits.ts b/apps/sim/lib/folders/resource-traits.ts new file mode 100644 index 00000000000..4c37d710548 --- /dev/null +++ b/apps/sim/lib/folders/resource-traits.ts @@ -0,0 +1,45 @@ +import type { FolderResourceType } from '@/lib/api/contracts/folders' + +/** + * The cheap, declarative facts about a folder resource type. + * + * Deliberately a leaf module. {@link folderResourceConfig} in `./config` owns + * everything else a resource type needs — child tables, archive and restore + * sets, delete guards — and to do that it imports the db schema for every table + * it serves, which transitively reaches the executor and the tool registry. + * Pulling that graph in to read a label or a boolean puts thousands of modules + * into consumers that only ever needed a string, so those two facts live here + * and `./config` composes them into its own entries. + */ +export const FOLDER_RESOURCE_LABELS: Record = { + workflow: 'workflow', + file: 'file', + knowledge_base: 'knowledge base', + table: 'table', +} + +/** + * Resource types whose folders participate in the mutation-lock system. Only + * workflow folders can be locked; the rest have no lock semantics to honour. + * + * The single declaration of that fact: `./config` composes it into + * {@link FolderResourceConfig.supportsLocking} rather than restating it, so the + * routes that read the trait directly and the orchestration that reads it off + * the config cannot disagree about which resources lock. + */ +export const FOLDER_RESOURCE_SUPPORTS_LOCKING: Record = { + workflow: true, + file: false, + knowledge_base: false, + table: false, +} + +/** Human-readable name for `resourceType`, for messages a caller reads. */ +export function folderResourceLabel(resourceType: FolderResourceType): string { + return FOLDER_RESOURCE_LABELS[resourceType] +} + +/** Whether `resourceType`'s folders honour the folder mutation lock. */ +export function folderResourceSupportsLocking(resourceType: FolderResourceType): boolean { + return FOLDER_RESOURCE_SUPPORTS_LOCKING[resourceType] +} diff --git a/apps/sim/lib/folders/status.ts b/apps/sim/lib/folders/status.ts index 2cf6e89037b..9c5762c3a1d 100644 --- a/apps/sim/lib/folders/status.ts +++ b/apps/sim/lib/folders/status.ts @@ -21,5 +21,8 @@ export function folderMutationStatus(errorCode: FolderMutationErrorCode | undefi if (errorCode === 'not_found') return 404 if (errorCode === 'conflict') return 409 if (errorCode === 'locked') return 423 + // A folder collection too large to materialize reaches these routes as a classified + // failure too; without this arm it rendered as an unexplained 500. + if (errorCode === 'payload_too_large') return 413 return 500 } diff --git a/apps/sim/lib/logs/application/get-public-log.ts b/apps/sim/lib/logs/application/get-public-log.ts index 89b7500be2e..0accc6b76e0 100644 --- a/apps/sim/lib/logs/application/get-public-log.ts +++ b/apps/sim/lib/logs/application/get-public-log.ts @@ -4,38 +4,14 @@ import { loadActiveFolderPathIndex } from '@/lib/folders/queries' import { logOperations } from '@/lib/logs/application/operations' import { materializeExecutionDataForDisplay } from '@/lib/logs/execution/trace-store' import { getPublicWorkflowLog, getPublicWorkflowLogScope } from '@/lib/logs/public-queries' -import { sanitizeWorkflowForSharing } from '@/lib/workflows/credentials/credential-extractor' +import { sanitizeExecutionSnapshotState } from '@/lib/logs/snapshot-sanitizer' import { type ActiveWorkspaceApplicationContext, loadActiveWorkspaceApplicationContext, } from '@/lib/workspaces/application/workspace-context' -import type { WorkflowState } from '@/stores/workflows/workflow/types' type PublicWorkflowLog = NonNullable>> -/** - * Strips credentials out of the execution's graph snapshot before it leaves the process. - * - * The snapshot is the workflow graph as executed, so `blocks[].subBlocks[].value` carries - * whatever the author typed into a `password: true` field and the credential id behind an - * `oauth-input`. Redaction is unconditional: the only surface reading this run is the v2 - * public API, reachable by a read-role workspace API key. - * - * `preserveEnvVars` keeps `{{VAR}}` references, which name a workspace environment variable - * rather than carrying its value — resolution happens at execution time — so the reference is - * not a secret and is what keeps consecutive run snapshots diffable. - * - * A run with no retained snapshot projects as `null`, and so does a stored value that is not an - * object: the sanitizer can make no guarantee about a shape it cannot walk, so it is withheld - * rather than passed through. - */ -function sanitizeSnapshotState( - state: PublicWorkflowLog['workflowState'] -): Record | null { - if (typeof state !== 'object' || state === null) return null - return sanitizeWorkflowForSharing(state as Partial, { preserveEnvVars: true }) -} - interface PublicLogContext extends ActiveWorkspaceApplicationContext { executionId: string workflowId: string | null @@ -86,7 +62,7 @@ export const getPublicLog = defineAuthorizedWorkspaceUseCase({ throw new Error(`Unable to resolve workflow owner email for ${log.workflowUserId}`) } return { - log: { ...log, workflowState: sanitizeSnapshotState(log.workflowState) }, + log: { ...log, workflowState: sanitizeExecutionSnapshotState(log.workflowState) }, workflowFolderPath: log.workflowFolderId ? (folderIndex.pathById.get(log.workflowFolderId) ?? null) : null, diff --git a/apps/sim/lib/logs/snapshot-sanitizer.ts b/apps/sim/lib/logs/snapshot-sanitizer.ts new file mode 100644 index 00000000000..f086f853103 --- /dev/null +++ b/apps/sim/lib/logs/snapshot-sanitizer.ts @@ -0,0 +1,23 @@ +import { sanitizeWorkflowForSharing } from '@/lib/workflows/credentials/credential-extractor' +import type { WorkflowState } from '@/stores/workflows/workflow/types' + +/** + * Strips credentials out of an execution's graph snapshot before it leaves the process. + * + * The snapshot is the workflow graph as executed, so `blocks[].subBlocks[].value` carries + * whatever the author typed into a `password: true` field and the credential id behind an + * `oauth-input`. Redaction is unconditional: every surface that projects a run — the v1 and + * v2 public APIs — is reachable by an API key that holds no billing or credential authority. + * + * `preserveEnvVars` keeps `{{VAR}}` references, which name a workspace environment variable + * rather than carrying its value — resolution happens at execution time — so the reference is + * not a secret and is what keeps consecutive run snapshots diffable. + * + * A run with no retained snapshot projects as `null`, and so does a stored value that is not an + * object: the sanitizer can make no guarantee about a shape it cannot walk, so it is withheld + * rather than passed through. + */ +export function sanitizeExecutionSnapshotState(state: unknown): Record | null { + if (typeof state !== 'object' || state === null) return null + return sanitizeWorkflowForSharing(state as Partial, { preserveEnvVars: true }) +} diff --git a/apps/sim/lib/skills/application/operations.ts b/apps/sim/lib/skills/application/operations.ts index dc0ecbebd90..79c652eb395 100644 --- a/apps/sim/lib/skills/application/operations.ts +++ b/apps/sim/lib/skills/application/operations.ts @@ -40,6 +40,22 @@ export const skillOperations = { workspaceApiKey: 'deny', ...HUMAN_PRINCIPAL_POLICY, }), + /** + * One mixed batch of creates and updates applied as a single unit. + * + * The declared minimum role is the floor a pure-update batch needs, matching + * {@link skillOperations.update}: workspace write is not required to edit a + * skill you are an editor of. A batch that also creates is additionally + * authorized against {@link skillOperations.create} by the use case, before + * anything is written — so neither half of the batch is authorized more + * loosely than it would be on its own. + */ + upsert: defineWorkspaceOperation({ + id: 'skills.upsert', + minimumRole: 'read', + workspaceApiKey: 'deny', + ...HUMAN_PRINCIPAL_POLICY, + }), delete: defineWorkspaceOperation({ id: 'skills.delete', minimumRole: 'read', diff --git a/apps/sim/lib/skills/application/use-cases.ts b/apps/sim/lib/skills/application/use-cases.ts index 73fbf9130dc..258280e26b8 100644 --- a/apps/sim/lib/skills/application/use-cases.ts +++ b/apps/sim/lib/skills/application/use-cases.ts @@ -2,11 +2,20 @@ import { AuditAction, AuditResourceType } from '@sim/audit' import { requirePrincipalSubjectUserId, resolvePrincipalAttribution } from '@sim/auth/principal' import type { skill } from '@sim/db/schema' import type { ListSortOrder } from '@/lib/api/list-query' -import { defineAuthorizedWorkspaceUseCase } from '@/lib/core/application' +import { + authorizeWorkspaceOperation, + defineAuthorizedWorkspaceUseCase, +} from '@/lib/core/application' import { OrchestrationError } from '@/lib/core/orchestration/types' import { skillDelegationPolicy } from '@/lib/skills/application/authorization' import { skillOperations } from '@/lib/skills/application/operations' -import { createSkill, deleteSkillRecord, updateSkill } from '@/lib/skills/orchestration' +import { + createSkill, + deleteSkillRecord, + type SkillUpsertItem, + updateSkill, + upsertSkillBatch, +} from '@/lib/skills/orchestration' import { loadActiveWorkspaceContext } from '@/lib/uploads/contexts/workspace' import { getSkillById, @@ -170,6 +179,61 @@ export const updateSkillUseCase = defineAuthorizedWorkspaceUseCase({ }), }) +export interface UpsertSkillsInput { + workspaceId: string + skills: SkillUpsertItem[] + source?: SkillWriteSource +} + +/** + * Applies a mixed batch of skill creates and updates as one semantic + * operation. Every item is authorized before any of them is written, and the + * writes share one transaction, so a rejected item leaves the whole batch + * unwritten and unaudited rather than partially committing it. + * + * The audit trail is unchanged in shape: one entry per skill actually written, + * tagged `skill.created` or `skill.updated`. Only `metadata.operation` differs + * from the single-item use cases, because the semantic operation genuinely is + * the batch. + */ +export const upsertSkillsUseCase = defineAuthorizedWorkspaceUseCase({ + operation: skillOperations.upsert, + resolveContext: ({ input }: { input: UpsertSkillsInput }) => + resolveWorkspaceContext(input.workspaceId), + authorizationOptions, + async execute({ principal, input, context }) { + /** + * `skills.upsert` declares only the read floor an update needs. An item + * without an id creates, which `skills.create` gates on workspace write — + * so demand that too, still ahead of every write. + */ + if (input.skills.some((item) => !item.id)) { + await authorizeWorkspaceOperation( + principal, + skillOperations.create, + context, + authorizationOptions + ) + } + + const touched = await upsertSkillBatch({ + workspaceId: context.workspaceId, + userId: requirePrincipalSubjectUserId(principal), + skills: input.skills, + }) + return { touched } + }, + projectAudit: ({ input, result }) => + result.touched.map((entry) => ({ + action: entry.operation === 'created' ? AuditAction.SKILL_CREATED : AuditAction.SKILL_UPDATED, + resourceType: AuditResourceType.SKILL, + resourceId: entry.id, + resourceName: entry.name, + description: `${entry.operation === 'created' ? 'Created' : 'Updated'} skill "${entry.name}"`, + metadata: { source: input.source }, + })), +}) + export interface DeleteSkillInput { workspaceId: string skillId: string diff --git a/apps/sim/lib/skills/orchestration/index.ts b/apps/sim/lib/skills/orchestration/index.ts index fd07c58624d..fb69a44492c 100644 --- a/apps/sim/lib/skills/orchestration/index.ts +++ b/apps/sim/lib/skills/orchestration/index.ts @@ -1,15 +1,9 @@ export { createSkill, deleteSkillRecord, - type PerformCreateSkillParams, - type PerformDeleteSkillParams, - type PerformSkillResult, - type PerformUpdateSkillParams, - performCreateSkill, - performDeleteSkill, - performUpdateSkill, type SkillOrchestrationErrorCode, + type SkillUpsertItem, type SkillWriteSource, - statusForSkillOrchestrationError, updateSkill, + upsertSkillBatch, } from './skill-lifecycle' diff --git a/apps/sim/lib/skills/orchestration/skill-lifecycle.test.ts b/apps/sim/lib/skills/orchestration/skill-lifecycle.test.ts index 857d4fd194f..97699034e0a 100644 --- a/apps/sim/lib/skills/orchestration/skill-lifecycle.test.ts +++ b/apps/sim/lib/skills/orchestration/skill-lifecycle.test.ts @@ -21,16 +21,6 @@ vi.mock('@/lib/workflows/skills/operations', () => ({ deleteSkill: mockDeleteSkill, })) -vi.mock('@/lib/posthog/server', () => ({ - captureServerEvent: vi.fn(), -})) - -vi.mock('@sim/audit', () => ({ - AuditAction: { SKILL_CREATED: 'skill.created', SKILL_UPDATED: 'skill.updated' }, - AuditResourceType: { SKILL: 'skill' }, - recordAudit: vi.fn(), -})) - import { createSkill, updateSkill } from '@/lib/skills/orchestration/skill-lifecycle' const WORKSPACE_ID = '11111111-1111-4111-8111-111111111111' diff --git a/apps/sim/lib/skills/orchestration/skill-lifecycle.ts b/apps/sim/lib/skills/orchestration/skill-lifecycle.ts index ad2a25f413f..b7930b1a6fa 100644 --- a/apps/sim/lib/skills/orchestration/skill-lifecycle.ts +++ b/apps/sim/lib/skills/orchestration/skill-lifecycle.ts @@ -1,33 +1,30 @@ -import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit' import type { skill } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { getErrorMessage, getPostgresErrorCode } from '@sim/utils/errors' -import type { NextRequest } from 'next/server' import type { z } from 'zod' import { skillContentSchema, skillDescriptionSchema, skillNameSchema, } from '@/lib/api/contracts/skills' -import { - asOrchestrationError, - OrchestrationError, - type OrchestrationErrorCode, -} from '@/lib/core/orchestration/types' -import { captureServerEvent } from '@/lib/posthog/server' +import { OrchestrationError, type OrchestrationErrorCode } from '@/lib/core/orchestration/types' import { getSkillActorContext } from '@/lib/skills/access' import { getBuiltinSkillByName, isBuiltinSkillId } from '@/lib/workflows/skills/builtin-skills' -import { deleteSkill, getSkillById, upsertSkills } from '@/lib/workflows/skills/operations' +import { + deleteSkill, + getSkillById, + type TouchedSkill, + upsertSkills, +} from '@/lib/workflows/skills/operations' const logger = createLogger('SkillOrchestration') /** - * Shared skill manager primitives and legacy orchestration adapters. + * Shared skill manager primitives. * - * The throwing primitives own field validation, built-in guards, conflicts, + * These throwing primitives own field validation, built-in guards, conflicts, * and per-skill editor checks. Authorized application use cases own workspace - * authorization and semantic audit. The `perform*` adapters preserve internal - * route result, audit, and analytics compatibility. + * authorization and semantic audit; each surface adapter owns its own analytics. */ /** @@ -36,30 +33,12 @@ const logger = createLogger('SkillOrchestration') */ export type SkillOrchestrationErrorCode = OrchestrationErrorCode | 'forbidden' -/** HTTP status for a skill orchestration failure, shared by every route surface. */ -export function statusForSkillOrchestrationError( - code: SkillOrchestrationErrorCode | undefined -): number { - if (code === 'validation') return 400 - if (code === 'forbidden') return 403 - if (code === 'not_found') return 404 - if (code === 'conflict') return 409 - return 500 -} - type SkillRow = typeof skill.$inferSelect /** Which surface performed the write. Recorded on the audit entry and the analytics event. */ export type SkillWriteSource = 'settings' | 'tool_input' | 'api' -interface ActorMetadata { - actorName?: string | null - actorEmail?: string | null - source?: SkillWriteSource - request?: NextRequest -} - -export interface PerformCreateSkillParams extends ActorMetadata { +export interface CreateSkillParams { workspaceId: string userId: string name: string @@ -67,7 +46,7 @@ export interface PerformCreateSkillParams extends ActorMetadata { content: string } -export interface PerformUpdateSkillParams extends ActorMetadata { +export interface UpdateSkillParams { workspaceId: string userId: string skillId: string @@ -76,21 +55,20 @@ export interface PerformUpdateSkillParams extends ActorMetadata { content?: string } -export interface PerformDeleteSkillParams extends ActorMetadata { +export interface DeleteSkillParams { workspaceId: string userId: string skillId: string } -export interface PerformSkillResult { - success: boolean - error?: string - errorCode?: SkillOrchestrationErrorCode - skill?: SkillRow +/** Classified failure passed between the internal guards and {@link throwSkillFailure}. */ +interface SkillFailure { + error: string + errorCode: SkillOrchestrationErrorCode } -function validationFailure(error: string): PerformSkillResult { - return { success: false, error, errorCode: 'validation' } +function validationFailure(error: string): SkillFailure { + return { error, errorCode: 'validation' } } /** First message from a failed field parse, or null when the value is valid. */ @@ -118,7 +96,7 @@ async function resolveEditableSkill(params: { workspaceId: string userId: string skillId: string -}): Promise<{ ok: true; skill: SkillRow } | { ok: false; result: PerformSkillResult }> { +}): Promise<{ ok: true; skill: SkillRow } | { ok: false; result: SkillFailure }> { if (isBuiltinSkillId(params.skillId)) { return { ok: false, @@ -128,16 +106,12 @@ async function resolveEditableSkill(params: { const actor = await getSkillActorContext(params.skillId, params.userId) if (!actor.skill || actor.skill.workspaceId !== params.workspaceId || !actor.hasWorkspaceAccess) { - return { - ok: false, - result: { success: false, error: 'Skill not found', errorCode: 'not_found' }, - } + return { ok: false, result: { error: 'Skill not found', errorCode: 'not_found' } } } if (!actor.canEdit) { return { ok: false, result: { - success: false, error: `Skill editor access required to modify "${actor.skill.name}"`, errorCode: 'forbidden', }, @@ -155,181 +129,70 @@ async function resolveEditableSkill(params: { * by `skill_workspace_name_unique` as a raw Postgres error whose message matches * nothing here — which would otherwise surface as a 500 for what is a conflict. */ -function classifyUpsertError(error: unknown): PerformSkillResult { +function classifyUpsertError(error: unknown): SkillFailure { const message = getErrorMessage(error, 'Failed to save skill') if (getPostgresErrorCode(error) === '23505') { - return { - success: false, - error: 'That skill name is unavailable in this workspace', - errorCode: 'conflict', - } + return { error: 'That skill name is unavailable in this workspace', errorCode: 'conflict' } } if (message.includes('is unavailable')) { - return { success: false, error: message, errorCode: 'conflict' } + return { error: message, errorCode: 'conflict' } } if (message.startsWith('Skill not found')) { - return { success: false, error: 'Skill not found', errorCode: 'not_found' } + return { error: 'Skill not found', errorCode: 'not_found' } } logger.error('Skill upsert failed', { error: message }) - return { success: false, error: 'Failed to save skill', errorCode: 'internal' } + return { error: 'Failed to save skill', errorCode: 'internal' } } -type SkillLifecycleAction = 'created' | 'updated' | 'deleted' - -const AUDIT_ACTION = { - created: AuditAction.SKILL_CREATED, - updated: AuditAction.SKILL_UPDATED, - deleted: AuditAction.SKILL_DELETED, -} as const satisfies Record - -const AUDIT_VERB = { - created: 'Created', - updated: 'Updated', - deleted: 'Deleted', -} as const satisfies Record - -function recordSkillEvent(params: { - action: SkillLifecycleAction - workspaceId: string - userId: string - skillId: string - skillName: string - actor: ActorMetadata -}): void { - const { action, workspaceId, userId, skillId, skillName, actor } = params - - recordAudit({ - workspaceId, - actorId: userId, - actorName: actor.actorName ?? undefined, - actorEmail: actor.actorEmail ?? undefined, - action: AUDIT_ACTION[action], - resourceType: AuditResourceType.SKILL, - resourceId: skillId, - resourceName: skillName, - description: `${AUDIT_VERB[action]} skill "${skillName}"`, - metadata: { source: actor.source }, - request: actor.request, - }) - - // The delete event carries no skill_name — the skill no longer exists to name. - if (action === 'deleted') { - captureServerEvent( - userId, - 'skill_deleted', - { skill_id: skillId, workspace_id: workspaceId, source: actor.source }, - { groups: { workspace: workspaceId } } - ) - return - } - - captureServerEvent( - userId, - action === 'created' ? 'skill_created' : 'skill_updated', - { - skill_id: skillId, - skill_name: skillName, - workspace_id: workspaceId, - source: actor.source, - }, - { groups: { workspace: workspaceId } } - ) +function throwSkillFailure(result: SkillFailure): never { + throw new OrchestrationError(result.errorCode, result.error) } -export async function performCreateSkill( - params: PerformCreateSkillParams -): Promise { - try { - const skill = await createSkill(params) - recordSkillEvent({ - action: 'created', - workspaceId: params.workspaceId, - userId: params.userId, - skillId: skill.id, - skillName: skill.name, - actor: params, - }) - return { success: true, skill } - } catch (error) { - return skillFailureResult(error, 'Failed to create skill') - } +/** One item of a batch upsert: no `id` creates, an `id` partially updates. */ +export interface SkillUpsertItem { + id?: string + name?: string + description?: string + content?: string } -function throwSkillFailure(result: PerformSkillResult): never { - throw new OrchestrationError( - result.errorCode ?? 'internal', - result.error ?? 'Skill operation failed' - ) +export interface UpsertSkillBatchParams { + workspaceId: string + /** + * The acting subject. Gates every update through the per-skill editor check + * and is recorded as the owner of every row this batch creates. + */ + userId: string + skills: SkillUpsertItem[] } -function skillFailureResult(error: unknown, fallback: string): PerformSkillResult { - const classified = asOrchestrationError(error) - if (classified) { - return { success: false, error: classified.message, errorCode: classified.code } +/** Field validation and the built-in name guard for a create item. */ +function validateCreateItem(item: SkillUpsertItem): void { + if (item.name === undefined || item.description === undefined || item.content === undefined) { + throw new OrchestrationError( + 'validation', + 'Skill name, description, and content are required to create a skill' + ) } - logger.error(fallback, { error: getErrorMessage(error, fallback) }) - return { success: false, error: fallback, errorCode: 'internal' } -} - -export async function createSkill( - params: Omit -): Promise { const invalid = - fieldError(skillNameSchema, params.name) ?? - fieldError(skillDescriptionSchema, params.description) ?? - fieldError(skillContentSchema, params.content) ?? - builtinNameCollision(params.name) + fieldError(skillNameSchema, item.name) ?? + fieldError(skillDescriptionSchema, item.description) ?? + fieldError(skillContentSchema, item.content) ?? + builtinNameCollision(item.name) if (invalid) throw new OrchestrationError('validation', invalid) - - let created: { id: string; name: string } | undefined - try { - const { touched } = await upsertSkills({ - skills: [{ name: params.name, description: params.description, content: params.content }], - workspaceId: params.workspaceId, - userId: params.userId, - returnSkills: false, - }) - created = touched[0] - } catch (error) { - throwSkillFailure(classifyUpsertError(error)) - } - - if (!created) { - throw new Error(`Skill create returned no touched row for workspace ${params.workspaceId}`) - } - - const row = await getSkillById({ skillId: created.id, workspaceId: params.workspaceId }) - if (!row) throw new Error(`Skill ${created.id} missing after a successful create`) - return row } -export async function performUpdateSkill( - params: PerformUpdateSkillParams -): Promise { - try { - const skill = await updateSkill(params) - recordSkillEvent({ - action: 'updated', - workspaceId: params.workspaceId, - userId: params.userId, - skillId: skill.id, - skillName: skill.name, - actor: params, - }) - return { success: true, skill } - } catch (error) { - return skillFailureResult(error, 'Failed to update skill') - } -} - -export async function updateSkill( - params: Omit -): Promise { - if ( - params.name === undefined && - params.description === undefined && - params.content === undefined - ) { +/** + * Field validation, the per-skill editor check, and the rename guard for an + * update item. Reads only; the caller writes once every item has passed. + */ +async function validateUpdateItem( + workspaceId: string, + userId: string, + skillId: string, + item: SkillUpsertItem +): Promise { + if (item.name === undefined && item.description === undefined && item.content === undefined) { throw new OrchestrationError( 'validation', 'At least one of name, description, or content is required' @@ -337,69 +200,104 @@ export async function updateSkill( } const invalid = - (params.name !== undefined ? fieldError(skillNameSchema, params.name) : null) ?? - (params.description !== undefined - ? fieldError(skillDescriptionSchema, params.description) + (item.name !== undefined ? fieldError(skillNameSchema, item.name) : null) ?? + (item.description !== undefined + ? fieldError(skillDescriptionSchema, item.description) : null) ?? - (params.content !== undefined ? fieldError(skillContentSchema, params.content) : null) + (item.content !== undefined ? fieldError(skillContentSchema, item.content) : null) if (invalid) throw new OrchestrationError('validation', invalid) - const resolved = await resolveEditableSkill(params) + const resolved = await resolveEditableSkill({ workspaceId, userId, skillId }) if (!resolved.ok) throwSkillFailure(resolved.result) // Only a rename can newly shadow a built-in. Rows predating the guard may already carry a // built-in's name, and the modal always resubmits the full object, so compare against the // canonical name rather than rejecting every write that echoes it back. - if (params.name !== undefined && params.name !== resolved.skill.name) { - const collision = builtinNameCollision(params.name) + if (item.name !== undefined && item.name !== resolved.skill.name) { + const collision = builtinNameCollision(item.name) if (collision) throw new OrchestrationError('validation', collision) } +} + +/** + * Applies a mixed batch of creates and updates as one unit. + * + * Every item is validated and authorized first — no row is written until the + * whole batch has passed — and the writes then run inside the single + * `upsertSkills` transaction, so a rejected item leaves the earlier ones + * unwritten instead of half-committing the batch. + * + * Workspace-level authorization is the caller's (the application use case's) + * job. What lives here is the per-skill editor check an update needs, which + * the workspace role cannot express. + */ +export async function upsertSkillBatch( + params: UpsertSkillBatchParams +): Promise { + if (params.skills.length === 0) return [] + + for (const item of params.skills) { + if (item.id) { + await validateUpdateItem(params.workspaceId, params.userId, item.id, item) + continue + } + validateCreateItem(item) + } try { - await upsertSkills({ - skills: [ - { - id: params.skillId, - ...(params.name !== undefined ? { name: params.name } : {}), - ...(params.description !== undefined ? { description: params.description } : {}), - ...(params.content !== undefined ? { content: params.content } : {}), - }, - ], + const { touched } = await upsertSkills({ + skills: params.skills.map((item) => ({ + ...(item.id !== undefined ? { id: item.id } : {}), + ...(item.name !== undefined ? { name: item.name } : {}), + ...(item.description !== undefined ? { description: item.description } : {}), + ...(item.content !== undefined ? { content: item.content } : {}), + })), workspaceId: params.workspaceId, userId: params.userId, returnSkills: false, }) + return touched } catch (error) { throwSkillFailure(classifyUpsertError(error)) } +} - const row = await getSkillById({ skillId: params.skillId, workspaceId: params.workspaceId }) - if (!row) throw new OrchestrationError('not_found', 'Skill not found') +export async function createSkill(params: CreateSkillParams): Promise { + const [created] = await upsertSkillBatch({ + workspaceId: params.workspaceId, + userId: params.userId, + skills: [{ name: params.name, description: params.description, content: params.content }], + }) + + if (!created) { + throw new Error(`Skill create returned no touched row for workspace ${params.workspaceId}`) + } + + const row = await getSkillById({ skillId: created.id, workspaceId: params.workspaceId }) + if (!row) throw new Error(`Skill ${created.id} missing after a successful create`) return row } -export async function performDeleteSkill( - params: PerformDeleteSkillParams -): Promise { - try { - const skill = await deleteSkillRecord(params) - recordSkillEvent({ - action: 'deleted', - workspaceId: params.workspaceId, - userId: params.userId, - skillId: skill.id, - skillName: skill.name, - actor: params, - }) - return { success: true, skill } - } catch (error) { - return skillFailureResult(error, 'Failed to delete skill') - } +export async function updateSkill(params: UpdateSkillParams): Promise { + await upsertSkillBatch({ + workspaceId: params.workspaceId, + userId: params.userId, + skills: [ + { + id: params.skillId, + ...(params.name !== undefined ? { name: params.name } : {}), + ...(params.description !== undefined ? { description: params.description } : {}), + ...(params.content !== undefined ? { content: params.content } : {}), + }, + ], + }) + + const row = await getSkillById({ skillId: params.skillId, workspaceId: params.workspaceId }) + if (!row) throw new OrchestrationError('not_found', 'Skill not found') + return row } -export async function deleteSkillRecord( - params: Omit -): Promise { +export async function deleteSkillRecord(params: DeleteSkillParams): Promise { const resolved = await resolveEditableSkill(params) if (!resolved.ok) throwSkillFailure(resolved.result) diff --git a/apps/sim/lib/table/columns/retype-cell.test.ts b/apps/sim/lib/table/columns/retype-cell.test.ts new file mode 100644 index 00000000000..479563fc74b --- /dev/null +++ b/apps/sim/lib/table/columns/retype-cell.test.ts @@ -0,0 +1,44 @@ +/** + * @vitest-environment node + */ + +import { describe, expect, it } from 'vitest' +import { retypeCellRewrite } from '@/lib/table/columns/service' +import type { ColumnDefinition } from '@/lib/table/types' + +const column = (over: Partial): ColumnDefinition => + ({ name: 'col', type: 'string', ...over }) as ColumnDefinition + +describe('retypeCellRewrite', () => { + it('preserves an empty string the target type can hold', () => { + // `''` is a real stored value: `coerceRowValues` keeps it for `string`, and + // `json` accepts anything. Nulling it silently destroys the cell — and on a + // required target leaves a null behind a constraint that just passed, + // because `countEmptyCells` does not count `''` as empty. + expect(retypeCellRewrite('', column({ type: 'json' }))).toBeNull() + expect(retypeCellRewrite('', column({ type: 'json', required: true }))).toBeNull() + expect(retypeCellRewrite('', column({ type: 'string' }))).toBeNull() + }) + + it('nulls an empty string the target type cannot read', () => { + expect(retypeCellRewrite('', column({ type: 'number' }))).toEqual({ value: null }) + expect(retypeCellRewrite('', column({ type: 'boolean' }))).toEqual({ value: null }) + expect(retypeCellRewrite('', column({ type: 'date' }))).toEqual({ value: null }) + }) + + it('writes back the value the target coercion produces', () => { + expect(retypeCellRewrite('42', column({ type: 'number' }))).toEqual({ value: 42 }) + expect(retypeCellRewrite(7, column({ type: 'string' }))).toEqual({ value: '7' }) + expect(retypeCellRewrite('true', column({ type: 'boolean' }))).toEqual({ value: true }) + }) + + it('skips a cell whose stored value already matches the coercion', () => { + expect(retypeCellRewrite('kept', column({ type: 'json' }))).toBeNull() + expect(retypeCellRewrite(3, column({ type: 'json' }))).toBeNull() + }) + + it('leaves absent cells alone', () => { + expect(retypeCellRewrite(null, column({ type: 'string' }))).toBeNull() + expect(retypeCellRewrite(undefined, column({ type: 'string' }))).toBeNull() + }) +}) diff --git a/apps/sim/lib/table/columns/service.ts b/apps/sim/lib/table/columns/service.ts index 4769e627b41..f7cc0b6803d 100644 --- a/apps/sim/lib/table/columns/service.ts +++ b/apps/sim/lib/table/columns/service.ts @@ -746,6 +746,41 @@ export function applyPendingRename( return { ...column, name: newName } } +/** + * What a retype must write back for one already-compatible cell, or `null` when + * the stored value is already the value the new type should hold. + * + * A blank the target CANNOT read becomes null — the write path turns an + * unreadable value into null on an optional column, so the conversion does the + * same. A blank the target CAN read (`''` in a `string` or `json` column) is + * left exactly as stored: nulling it would silently destroy the cell, and on a + * `required` target it would leave a null behind a constraint that just passed + * (`countEmptyCells` does not treat `''` as empty). + * + * Everything else goes through the target's `coerce`, which frequently + * *transforms* the value — an epoch becomes an ISO date, `$1,234.56` becomes + * `1234.56`. Without writing the transformed value back the cell keeps its old + * bytes under the new type, and since filters and sorts apply the type's + * `jsonbCast` to whatever is stored, an epoch left in a `date` column makes + * `::timestamptz` fail on EVERY query against that column. + */ +export function retypeCellRewrite( + value: unknown, + target: ColumnDefinition +): { value: JsonValue } | null { + if (value === null || value === undefined) return null + + if (!isValueCompatibleWithColumn(value, target)) { + // Incompatible non-blanks never reach here: the compatibility scan already + // refused the whole conversion for them. + return value === '' ? { value: null } : null + } + + const coerced = columnTypeById(target.type).coerce(value as JsonValue, target) + if (coerced.ok && !Object.is(coerced.value, value)) return { value: coerced.value } + return null +} + /** * The column definition a retype produces: prior per-type metadata dropped, * then only what the TARGET type declares it owns carried forward, then that @@ -912,19 +947,11 @@ export async function updateColumnType( let incompatibleCount = 0 let blankCount = 0 /** - * Row id → the value the cell must END UP holding. - * - * Collected during the compatibility scan rather than re-derived later, so - * it reads the same `effective` value the check accepted — which for a - * `select` source is the option name, not the stored id. - * - * Load-bearing: a conversion is allowed exactly when the target type's - * `coerce` accepts the value, and `coerce` frequently *transforms* it (an - * epoch number becomes an ISO date, a formatted amount becomes a number). - * Without writing the transformed value back, the cell keeps its old bytes - * under the new type — and since filters and sorts apply the type's - * `jsonbCast` to whatever is stored, an epoch left in a `date` column makes - * `::timestamptz` fail on EVERY query against it. + * Compatibility scan, paged so a wide table cannot pull every row into + * memory at once. Only counts here — the values the cells must END UP + * holding are derived in the rewrite pass below, which reads the rows + * back after `migrationFrom` has run so a `select` source is already in + * its option-name form. See {@link retypeCellRewrite}. */ const retypeScanBatchSize = getColumnRetypeScanBatchSize() let validationAfterId: string | undefined @@ -1017,16 +1044,8 @@ export async function updateColumnType( if (rows.length === 0) break const coercedByRowId = new Map() for (const row of rows) { - const value = row.value - if (value === null || value === undefined) continue - if (value === '') { - coercedByRowId.set(row.id, null) - continue - } - const coerced = columnTypeById(data.newType).coerce(value as JsonValue, convertedColumn) - if (coerced.ok && !Object.is(coerced.value, value)) { - coercedByRowId.set(row.id, coerced.value) - } + const rewrite = retypeCellRewrite(row.value, convertedColumn) + if (rewrite) coercedByRowId.set(row.id, rewrite.value) } await writeBackCoercedCells( trx, diff --git a/apps/sim/lib/table/orchestration/import.ts b/apps/sim/lib/table/orchestration/import.ts index 49795944bd8..7a799540458 100644 --- a/apps/sim/lib/table/orchestration/import.ts +++ b/apps/sim/lib/table/orchestration/import.ts @@ -376,6 +376,13 @@ export interface PerformCreateTableFromCsvResult { success: boolean error?: string errorCode?: OrchestrationErrorCode + /** + * Which lock rejected the write. Set only when `errorCode` is `'locked'`, which + * {@link classifyImportFailure} populates for either entry point. Declared even though a + * table created by this call starts unlocked: the field is the only thing that tells a + * caller which lock to clear, and leaving it off the type is how a 423 silently loses it. + */ + lock?: TableLockKind data?: { table: CreatedTableFromCsv } } diff --git a/apps/sim/lib/uploads/client/upload-session.test.ts b/apps/sim/lib/uploads/client/upload-session.test.ts index adfe89df232..130d6b544b3 100644 --- a/apps/sim/lib/uploads/client/upload-session.test.ts +++ b/apps/sim/lib/uploads/client/upload-session.test.ts @@ -219,6 +219,61 @@ describe('uploadFileSession', () => { expect(onProgress.mock.calls.map(([event]) => event.loaded)).toEqual([8, 8, 10]) }) + it('completes a retried PUT whose create-only precondition already committed the object', async () => { + vi.useFakeTimers() + MockXhr.onSend = (xhr) => { + const attempt = MockXhr.instances.length + if (attempt === 1) { + queueMicrotask(() => xhr.dispatchEvent(new Event('error'))) + return + } + xhr.status = 412 + xhr.statusText = 'Precondition Failed' + queueMicrotask(() => xhr.dispatchEvent(new Event('load'))) + } + const complete = vi.fn(async () => 'done') + const abort = vi.fn(async () => undefined) + const onProgress = vi.fn() + + const promise = uploadFileSession({ + file: sizedFile(10), + transfer: { method: 'put', url: 'https://storage.example/upload', headers: {} }, + complete, + abort, + onProgress, + }) + await vi.runAllTimersAsync() + + await expect(promise).resolves.toBe('done') + expect(MockXhr.instances).toHaveLength(2) + expect(complete).toHaveBeenCalledWith() + expect(abort).not.toHaveBeenCalled() + expect(onProgress).toHaveBeenLastCalledWith({ loaded: 10, total: 10, percent: 100 }) + }) + + it('does not treat a first-attempt PUT conflict as a committed object', async () => { + MockXhr.onSend = (xhr) => { + xhr.status = 412 + xhr.statusText = 'Precondition Failed' + queueMicrotask(() => xhr.dispatchEvent(new Event('load'))) + } + const complete = vi.fn() + const abort = vi.fn(async () => undefined) + + await expect( + uploadFileSession({ + file: sizedFile(1), + transfer: { method: 'put', url: 'https://storage.example/upload', headers: {} }, + complete, + abort, + }) + ).rejects.toMatchObject({ status: 412 }) + + expect(MockXhr.instances).toHaveLength(1) + expect(complete).not.toHaveBeenCalled() + expect(abort).toHaveBeenCalledTimes(1) + }) + it('aborts XHR and the control session when the caller cancels', async () => { const controller = new AbortController() MockXhr.onSend = () => undefined diff --git a/apps/sim/lib/uploads/client/upload-session.ts b/apps/sim/lib/uploads/client/upload-session.ts index 52894634f29..f741670d598 100644 --- a/apps/sim/lib/uploads/client/upload-session.ts +++ b/apps/sim/lib/uploads/client/upload-session.ts @@ -109,6 +109,10 @@ async function uploadPut(params: UploadPutFileSession): Promise { reportProgress(params.file.size) return } catch (error) { + if (attempt > 0 && isCreateOnlyConflict(error)) { + reportProgress(params.file.size) + return + } if (isAbortError(error) || !isRetryableUploadError(error) || attempt >= MAX_RETRIES) { throw error } @@ -319,6 +323,20 @@ function isRetryableUploadError(error: unknown): error is UploadSessionTransport return error instanceof UploadSessionTransportError && error.transient } +/** + * Whether a retried whole-object PUT was rejected by the create-only + * precondition every provider signs into the transfer (S3/GCS `412`, Azure + * `409`). After a first attempt whose response was lost, that conflict means + * our own bytes are already committed, so the transfer is treated as finished + * and completion verifies the object's upload id, size, and content type + * server-side before anything durable is registered. + */ +function isCreateOnlyConflict(error: unknown): boolean { + return ( + error instanceof UploadSessionTransportError && (error.status === 409 || error.status === 412) + ) +} + function isRetryableStatus(status: number): boolean { return status === 408 || status === 429 || (status >= 500 && status < 600) } diff --git a/apps/sim/lib/uploads/core/storage-service.blob-connection-string.test.ts b/apps/sim/lib/uploads/core/storage-service.blob-connection-string.test.ts index b678aa6ae4a..cdfa72c4ed9 100644 --- a/apps/sim/lib/uploads/core/storage-service.blob-connection-string.test.ts +++ b/apps/sim/lib/uploads/core/storage-service.blob-connection-string.test.ts @@ -16,21 +16,14 @@ */ import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest' import * as uploadsConfig from '@/lib/uploads/config' -import { generatePresignedUploadUrl, headObject } from '@/lib/uploads/core/storage-service' +import { headObject } from '@/lib/uploads/core/storage-service' const CONNECTION_STRING = 'DefaultEndpointsProtocol=http;AccountName=devstoreaccount1;AccountKey=Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw==;BlobEndpoint=http://127.0.0.1:10000/devstoreaccount1;' -const { - mockBlobSASPermissionsParse, - mockHeadBlobObject, - mockGetBlobServiceClient, - mockGenerateBlobSASQueryParameters, -} = vi.hoisted(() => ({ - mockBlobSASPermissionsParse: vi.fn(() => 'create-only'), +const { mockHeadBlobObject, mockGetBlobServiceClient } = vi.hoisted(() => ({ mockHeadBlobObject: vi.fn(), mockGetBlobServiceClient: vi.fn(), - mockGenerateBlobSASQueryParameters: vi.fn(() => ({ toString: () => 'sig=fake' })), })) vi.mock('@/lib/uploads/providers/blob/client', () => ({ @@ -44,12 +37,6 @@ vi.mock('@/lib/uploads/providers/blob/client', () => ({ }, })) -vi.mock('@azure/storage-blob', () => ({ - StorageSharedKeyCredential: vi.fn(), - BlobSASPermissions: { parse: mockBlobSASPermissionsParse }, - generateBlobSASQueryParameters: mockGenerateBlobSASQueryParameters, -})) - const STORAGE_FLAGS = ['USE_S3_STORAGE', 'USE_BLOB_STORAGE', 'USE_GCS_STORAGE'] as const const originalFlagValues = STORAGE_FLAGS.map( @@ -105,24 +92,4 @@ describe('Azure Blob storage — connection-string-only auth', () => { }) expect(mockHeadBlobObject).toHaveBeenCalled() }) - - it('generatePresignedUploadUrl derives SAS credentials from connectionString when accountName/accountKey are absent', async () => { - const result = await generatePresignedUploadUrl({ - fileName: 'report.csv', - contentType: 'text/csv', - context: 'workspace', - fileSize: 100, - }) - - expect(mockGenerateBlobSASQueryParameters).toHaveBeenCalled() - expect(mockBlobSASPermissionsParse).toHaveBeenCalledWith('c') - expect(result.uploadHeaders).toEqual( - expect.objectContaining({ - 'If-None-Match': '*', - 'x-ms-blob-type': 'BlockBlob', - 'x-ms-meta-simuploadid': expect.any(String), - }) - ) - expect(result.url).toContain('sig=fake') - }) }) diff --git a/apps/sim/lib/uploads/core/storage-service.test.ts b/apps/sim/lib/uploads/core/storage-service.test.ts index 3ceca5b66df..d3d19f3934e 100644 --- a/apps/sim/lib/uploads/core/storage-service.test.ts +++ b/apps/sim/lib/uploads/core/storage-service.test.ts @@ -58,12 +58,7 @@ vi.mock('@/lib/uploads/server/metadata', () => ({ insertFileMetadata: mockInsertFileMetadata, })) -import { - createMultipartUpload, - generatePresignedUploadUrl, - uploadFile, - verifyPresignedUploadReceipt, -} from '@/lib/uploads/core/storage-service' +import { createMultipartUpload, uploadFile } from '@/lib/uploads/core/storage-service' const PART_SIZE = 8 * 1024 * 1024 @@ -84,31 +79,6 @@ describe('createMultipartUpload', () => { mockHeadS3Object.mockResolvedValue(null) }) - it('signs direct S3 uploads as create-only and returns the required precondition header', async () => { - const result = await generatePresignedUploadUrl({ - fileName: 'report.csv', - contentType: 'text/csv', - fileSize: 12, - context: 'workspace', - customKey: 'workspace/ws-1/report.csv', - }) - - expect(mockPutObjectCommand).toHaveBeenCalledWith( - expect.objectContaining({ - Bucket: 'b', - Key: 'workspace/ws-1/report.csv', - IfNoneMatch: '*', - Metadata: expect.objectContaining({ simuploadid: expect.any(String) }), - }) - ) - expect(result).toMatchObject({ - url: 'https://s3.example/create-only', - key: 'workspace/ws-1/report.csv', - uploadHeaders: { 'If-None-Match': '*' }, - uploadId: expect.any(String), - }) - }) - it('can upload an object without persisting generic metadata', async () => { await uploadFile({ file: Buffer.from('hello'), @@ -123,34 +93,6 @@ describe('createMultipartUpload', () => { expect(mockInsertFileMetadata).not.toHaveBeenCalled() }) - it('verifies a direct upload only when its opaque object receipt matches', async () => { - mockHeadS3Object.mockResolvedValueOnce({ - size: 12, - contentType: 'text/plain', - metadata: { simuploadid: 'receipt-1' }, - }) - - await expect( - verifyPresignedUploadReceipt({ - key: 'workspace/ws-1/report.txt', - context: 'workspace', - uploadId: 'receipt-1', - }) - ).resolves.toBe(true) - - mockHeadS3Object.mockResolvedValueOnce({ - size: 12, - metadata: { simuploadid: 'different-receipt' }, - }) - await expect( - verifyPresignedUploadReceipt({ - key: 'workspace/ws-1/report.txt', - context: 'workspace', - uploadId: 'receipt-1', - }) - ).resolves.toBe(false) - }) - it('takes the single-shot PutObject path for a payload smaller than one part', async () => { const handle = await createMultipartUpload({ key: 'k', diff --git a/apps/sim/lib/uploads/core/storage-service.ts b/apps/sim/lib/uploads/core/storage-service.ts index 7e0e511d407..499b603eec5 100644 --- a/apps/sim/lib/uploads/core/storage-service.ts +++ b/apps/sim/lib/uploads/core/storage-service.ts @@ -1,8 +1,6 @@ import type { Readable } from 'node:stream' -import { randomBytes } from 'crypto' import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' -import { generateId } from '@sim/utils/id' import { assertKnownSizeWithinLimit } from '@/lib/core/utils/stream-limits' import { getStorageConfig, @@ -17,20 +15,13 @@ import type { DeleteFileOptions, DownloadFileOptions, FileInfo, - GeneratePresignedUrlOptions, MultipartCompletionPolicy, - PresignedUrlResponse, StorageConfig, StorageContext, StoredObjectInfo, UploadFileOptions, } from '@/lib/uploads/shared/types' -import { PRESIGNED_UPLOAD_RECEIPT_METADATA_KEY } from '@/lib/uploads/shared/types' -import { - sanitizeFileKey, - sanitizeFilenameForMetadata, - sanitizeStorageMetadata, -} from '@/lib/uploads/utils/file-utils' +import { sanitizeFileKey } from '@/lib/uploads/utils/file-utils' const logger = createLogger('StorageService') @@ -688,243 +679,6 @@ export async function headObject( } } -/** Verifies that a create-only direct upload committed the object minted by one presigned URL. */ -export async function verifyPresignedUploadReceipt(options: { - key: string - context: StorageContext - uploadId: string -}): Promise { - const object = await headObject(options.key, options.context) - if (!object?.metadata) return false - - return Object.entries(object.metadata).some( - ([key, value]) => - key.toLowerCase() === PRESIGNED_UPLOAD_RECEIPT_METADATA_KEY && value === options.uploadId - ) -} - -/** - * Generate a presigned URL for direct file upload - */ -export async function generatePresignedUploadUrl( - options: GeneratePresignedUrlOptions -): Promise { - const { - fileName, - contentType, - fileSize, - context, - userId, - expirationSeconds = 3600, - metadata = {}, - customKey, - } = options - - const uploadId = generateId() - - const allMetadata = { - ...metadata, - originalName: fileName, - uploadedAt: new Date().toISOString(), - purpose: context, - ...(userId && { userId }), - [PRESIGNED_UPLOAD_RECEIPT_METADATA_KEY]: uploadId, - } - - const config = getStorageConfig(context) - - let key: string - if (customKey) { - key = customKey - } else { - const timestamp = Date.now() - const uniqueId = randomBytes(8).toString('hex') - const safeFileName = fileName.replace(/[^a-zA-Z0-9.-]/g, '_') - key = `${context}/${timestamp}-${uniqueId}-${safeFileName}` - } - - if (USE_S3_STORAGE) { - const response = await generateS3PresignedUrl( - key, - contentType, - fileSize, - allMetadata, - config, - expirationSeconds - ) - return { ...response, uploadId } - } - - if (USE_BLOB_STORAGE) { - const response = await generateBlobPresignedUrl( - key, - contentType, - allMetadata, - config, - expirationSeconds - ) - return { ...response, uploadId } - } - - if (USE_GCS_STORAGE) { - const response = await generateGcsPresignedUrl( - key, - contentType, - allMetadata, - config, - expirationSeconds - ) - return { ...response, uploadId } - } - - throw new Error('Cloud storage not configured. Cannot generate presigned URL for local storage.') -} - -/** - * Generate presigned URL for GCS - */ -async function generateGcsPresignedUrl( - key: string, - contentType: string, - metadata: Record, - config: StorageConfig, - expirationSeconds: number -): Promise { - const { getGcsPresignedUploadUrl } = await import('@/lib/uploads/providers/gcs/client') - - const { url, signedHeaders } = await getGcsPresignedUploadUrl( - key, - contentType, - metadata, - createGcsConfig(config), - expirationSeconds - ) - - return { - url, - key, - uploadHeaders: signedHeaders, - } -} - -/** - * Generate presigned URL for S3 - */ -async function generateS3PresignedUrl( - key: string, - contentType: string, - fileSize: number, - metadata: Record, - config: { bucket?: string; region?: string }, - expirationSeconds: number -): Promise { - const { getS3Client } = await import('@/lib/uploads/providers/s3/client') - const { PutObjectCommand } = await import('@aws-sdk/client-s3') - const { getSignedUrl } = await import('@aws-sdk/s3-request-presigner') - - if (!config.bucket || !config.region) { - throw new Error('S3 configuration missing bucket or region') - } - - const sanitizedMetadata = sanitizeStorageMetadata(metadata, 2000) - if (sanitizedMetadata.originalName) { - sanitizedMetadata.originalName = sanitizeFilenameForMetadata(sanitizedMetadata.originalName) - } - - const command = new PutObjectCommand({ - Bucket: config.bucket, - Key: key, - ContentType: contentType, - ContentLength: fileSize, - IfNoneMatch: '*', - Metadata: sanitizedMetadata, - }) - - const presignedUrl = await getSignedUrl(getS3Client(), command, { expiresIn: expirationSeconds }) - - return { - url: presignedUrl, - key, - uploadHeaders: { - 'If-None-Match': '*', - }, - } -} - -/** - * Generate presigned URL for Azure Blob - */ -async function generateBlobPresignedUrl( - key: string, - contentType: string, - metadata: Record, - config: { - containerName?: string - accountName?: string - accountKey?: string - connectionString?: string - }, - expirationSeconds: number -): Promise { - const { getBlobServiceClient, parseConnectionString } = await import( - '@/lib/uploads/providers/blob/client' - ) - const { BlobSASPermissions, generateBlobSASQueryParameters, StorageSharedKeyCredential } = - await import('@azure/storage-blob') - - if (!config.containerName) { - throw new Error('Blob configuration missing container name') - } - - const blobServiceClient = await getBlobServiceClient() - const containerClient = blobServiceClient.getContainerClient(config.containerName) - const blobClient = containerClient.getBlockBlobClient(key) - - const startsOn = new Date() - const expiresOn = new Date(startsOn.getTime() + expirationSeconds * 1000) - - let accountName = config.accountName - let accountKey = config.accountKey - if ((!accountName || !accountKey) && config.connectionString) { - ;({ accountName, accountKey } = parseConnectionString(config.connectionString)) - } - - if (!accountName || !accountKey) { - throw new Error( - 'Azure Blob SAS generation requires accountName/accountKey or a connectionString' - ) - } - - const sharedKeyCredential = new StorageSharedKeyCredential(accountName, accountKey) - const sasToken = generateBlobSASQueryParameters( - { - containerName: config.containerName, - blobName: key, - permissions: BlobSASPermissions.parse('c'), - startsOn, - expiresOn, - }, - sharedKeyCredential - ).toString() - - return { - url: `${blobClient.url}?${sasToken}`, - key, - uploadHeaders: { - 'If-None-Match': '*', - 'x-ms-blob-type': 'BlockBlob', - 'x-ms-blob-content-type': contentType, - ...Object.entries(metadata).reduce( - (acc, [k, v]) => { - acc[`x-ms-meta-${k}`] = encodeURIComponent(v) - return acc - }, - {} as Record - ), - }, - } -} - /** * Generate a presigned URL for downloading/accessing an existing file */ diff --git a/apps/sim/lib/uploads/providers/blob/client.test.ts b/apps/sim/lib/uploads/providers/blob/client.test.ts index 35c5feb9f20..99363a6213c 100644 --- a/apps/sim/lib/uploads/providers/blob/client.test.ts +++ b/apps/sim/lib/uploads/providers/blob/client.test.ts @@ -66,6 +66,7 @@ import { deleteFromBlob, downloadFromBlob, getBlobPresignedUploadUrl, + getMultipartPartUrls, getPresignedUrl, headBlobObject, initiateMultipartUpload, @@ -189,6 +190,19 @@ describe('Azure Blob Storage Client', () => { }) }) + it('signs multipart part URLs to the lifetime the caller passed', async () => { + const expiresOn = new Date('2026-01-01T00:02:00.000Z') + + await getMultipartPartUrls('workspace/workspace-1/file.bin', [1], customConfig, expiresOn) + + // The caller owns the lifetime and advertises the matching `expiresAt`; a window this + // function picks for itself is how the advertised expiry and the real SAS token drift. + expect(mockGenerateBlobSASQueryParameters).toHaveBeenCalledWith( + expect.objectContaining({ expiresOn }), + expect.anything() + ) + }) + it('returns only completed objects as usable upload identities', async () => { mockGetProperties.mockResolvedValueOnce({ contentLength: 3, diff --git a/apps/sim/lib/uploads/providers/blob/client.ts b/apps/sim/lib/uploads/providers/blob/client.ts index 5cd6c69e873..7cb350fda7d 100644 --- a/apps/sim/lib/uploads/providers/blob/client.ts +++ b/apps/sim/lib/uploads/providers/blob/client.ts @@ -601,12 +601,18 @@ export async function initiateMultipartUpload( } /** - * Generate presigned URLs for uploading parts + * Generate presigned URLs for uploading parts. + * + * `expiresOn` is required rather than defaulted: the caller owns the part-URL lifetime and + * advertises the matching `expiresAt` to the client, so a local default would be a second + * source of truth that silently keeps signing 1h SAS tokens after the caller's window changed. */ export async function getMultipartPartUrls( key: string, partNumbers: number[], - customConfig?: BlobConfig + customConfig: BlobConfig | undefined, + /** Absolute instant the SAS token stops being valid. */ + expiresOn: Date ): Promise { const { BlobServiceClient, @@ -659,7 +665,7 @@ export async function getMultipartPartUrls( blobName: key, permissions: BlobSASPermissions.parse('w'), // Write permission startsOn: new Date(), - expiresOn: new Date(Date.now() + 3600 * 1000), // 1 hour + expiresOn, } const sasToken = generateBlobSASQueryParameters( diff --git a/apps/sim/lib/uploads/providers/gcs/client.test.ts b/apps/sim/lib/uploads/providers/gcs/client.test.ts index ba9d7076470..99fa4f966ac 100644 --- a/apps/sim/lib/uploads/providers/gcs/client.test.ts +++ b/apps/sim/lib/uploads/providers/gcs/client.test.ts @@ -491,16 +491,25 @@ describe('GCS Client', () => { .mockResolvedValueOnce(['https://example.com/part-1']) .mockResolvedValueOnce(['https://example.com/part-2']) - const urls = await getGcsMultipartPartUrls('key.csv', 'upload-123', [1, 2]) + const expires = new Date('2026-01-01T00:02:00.000Z') + const urls = await getGcsMultipartPartUrls( + 'key.csv', + 'upload-123', + [1, 2], + undefined, + expires + ) expect(urls).toEqual([ { partNumber: 1, url: 'https://example.com/part-1' }, { partNumber: 2, url: 'https://example.com/part-2' }, ]) + // The caller owns the lifetime and advertises the matching `expiresAt`; signing to a + // window this function picked itself is how the two drift apart. expect(mockFile.getSignedUrl).toHaveBeenCalledWith({ version: 'v4', action: 'write', - expires: expect.any(Number), + expires, queryParams: { partNumber: '1', uploadId: 'upload-123' }, }) }) diff --git a/apps/sim/lib/uploads/providers/gcs/client.ts b/apps/sim/lib/uploads/providers/gcs/client.ts index fda05f2d1d8..abf0c3b5b4f 100644 --- a/apps/sim/lib/uploads/providers/gcs/client.ts +++ b/apps/sim/lib/uploads/providers/gcs/client.ts @@ -590,12 +590,18 @@ export async function uploadGcsPart( /** * Generate presigned URLs for uploading parts to GCS. The URLs sign the * `partNumber`/`uploadId` query parameters (V4), matching the S3 flow. + * + * `expires` is required rather than defaulted: the caller owns the part-URL lifetime and + * advertises the matching `expiresAt` to the client, so a local default would be a second + * source of truth that silently keeps signing 1h URLs after the caller's window changed. */ export async function getGcsMultipartPartUrls( key: string, uploadId: string, partNumbers: number[], - customConfig?: GcsConfig + customConfig: GcsConfig | undefined, + /** Absolute instant the signature stops being valid. */ + expires: Date ): Promise { const config = customConfig || { bucket: GCS_CONFIG.bucket } const storage = await getGcsClient() @@ -606,7 +612,7 @@ export async function getGcsMultipartPartUrls( const [url] = await file.getSignedUrl({ version: 'v4', action: 'write', - expires: Date.now() + 3600 * 1000, + expires, queryParams: { partNumber: String(partNumber), uploadId, diff --git a/apps/sim/lib/uploads/providers/s3/client.ts b/apps/sim/lib/uploads/providers/s3/client.ts index 7fff1d661f3..942b5fdd709 100644 --- a/apps/sim/lib/uploads/providers/s3/client.ts +++ b/apps/sim/lib/uploads/providers/s3/client.ts @@ -463,13 +463,19 @@ export async function uploadS3Part( } /** - * Generate presigned URLs for uploading parts to S3 + * Generate presigned URLs for uploading parts to S3. + * + * `expiresIn` is required rather than defaulted: the caller owns the part-URL lifetime and + * advertises the matching `expiresAt` to the client, so a local default would be a second + * source of truth that silently keeps signing 1h URLs after the caller's window changed. */ export async function getS3MultipartPartUrls( key: string, uploadId: string, partNumbers: number[], - customConfig?: S3Config + customConfig: S3Config | undefined, + /** Signature lifetime, in seconds. */ + expiresIn: number ): Promise { const config = customConfig || { bucket: S3_KB_CONFIG.bucket, region: S3_KB_CONFIG.region } const s3Client = getS3Client() @@ -483,7 +489,7 @@ export async function getS3MultipartPartUrls( UploadId: uploadId, }) - const url = await getSignedUrl(s3Client, command, { expiresIn: 3600 }) + const url = await getSignedUrl(s3Client, command, { expiresIn }) return { partNumber, url } }) ) diff --git a/apps/sim/lib/uploads/shared/types.ts b/apps/sim/lib/uploads/shared/types.ts index 96edd9cc9b7..af9625d9b7e 100644 --- a/apps/sim/lib/uploads/shared/types.ts +++ b/apps/sim/lib/uploads/shared/types.ts @@ -40,25 +40,6 @@ export type StorageContext = export type MultipartCompletionPolicy = 'create-only' | 'replace' | 'reuse-existing' -/** - * Contexts exempt from storage quota checks. Includes system-internal contexts - * (`logs` — written by the execution pipeline, not user-initiated) and small - * metadata assets (`profile-pictures`, `workspace-logos`, `og-images`). - * Mothership chat attachments are also exempt because they are not counted as - * durable workspace-file storage. - * - * The small-asset and system contexts are excluded from the multipart endpoint. - * Mothership remains available there for large chat attachments while retaining - * the same quota exemption as its single-part upload path. - */ -export const QUOTA_EXEMPT_STORAGE_CONTEXTS = new Set([ - 'mothership', - 'profile-pictures', - 'workspace-logos', - 'og-images', - 'logs', -]) - export interface FileInfo { path: string key: string @@ -102,29 +83,6 @@ export interface DeleteFileOptions { context?: StorageContext } -export interface GeneratePresignedUrlOptions { - fileName: string - contentType: string - fileSize: number - context: StorageContext - userId?: string - expirationSeconds?: number - metadata?: Record - /** - * When provided, overrides the default `${context}/${timestamp}-${id}-${name}` key derivation. - * The caller takes responsibility for uniqueness and prefix conventions. - */ - customKey?: string -} - -export interface PresignedUrlResponse { - url: string - key: string - uploadHeaders?: Record - /** Opaque per-URL receipt persisted in object metadata for safe retry verification. */ - uploadId?: string -} - export interface StoredObjectInfo { size: number contentType?: string @@ -132,5 +90,3 @@ export interface StoredObjectInfo { uploadId?: string version?: string } - -export const PRESIGNED_UPLOAD_RECEIPT_METADATA_KEY = 'simuploadid' diff --git a/apps/sim/lib/uploads/upload-session/provider.test.ts b/apps/sim/lib/uploads/upload-session/provider.test.ts index 7fc8edd9ec6..b8426003135 100644 --- a/apps/sim/lib/uploads/upload-session/provider.test.ts +++ b/apps/sim/lib/uploads/upload-session/provider.test.ts @@ -2,10 +2,12 @@ * @vitest-environment node */ import { mkdir, readdir, readFile, rm, stat } from 'node:fs/promises' -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -const { testUploadDirectory } = vi.hoisted(() => ({ +const { testUploadDirectory, mockS3Presign, mockS3PartUrls } = vi.hoisted(() => ({ testUploadDirectory: `/tmp/sim-upload-session-provider-${process.pid}`, + mockS3Presign: vi.fn(), + mockS3PartUrls: vi.fn(), })) vi.mock('@/lib/uploads/core/setup.server', () => ({ @@ -16,14 +18,22 @@ vi.mock('@/lib/uploads/config', () => ({ USE_BLOB_STORAGE: false, USE_GCS_STORAGE: false, USE_S3_STORAGE: false, - getStorageConfig: vi.fn(() => ({})), + getStorageConfig: vi.fn(() => ({ bucket: 'test-bucket', region: 'us-east-1' })), +})) + +vi.mock('@/lib/uploads/providers/s3/client', () => ({ + getS3PresignedUploadUrl: mockS3Presign, + getS3MultipartPartUrls: mockS3PartUrls, })) import { completeMultipartProviderUpload, + createPutProviderTransfer, + getMultipartProviderPartUrls, headProviderObject, LocalUploadBodyError, listMultipartProviderParts, + UPLOAD_URL_TTL_MS, writeLocalMultipartPart, writeLocalPutObject, } from '@/lib/uploads/upload-session/provider' @@ -204,6 +214,166 @@ describe('local upload-session provider', () => { }) }) +/** Mirrors `UPLOAD_SESSION_TTL_MS`, imported here as a literal so this suite + * does not pull the upload-session service's database and billing graph. */ +const SESSION_TTL_MS = 24 * 60 * 60 * 1000 + +describe('signed transfer URL lifetimes', () => { + beforeEach(() => { + vi.clearAllMocks() + vi.useFakeTimers() + vi.setSystemTime(new Date('2026-01-01T00:00:00.000Z')) + mockS3Presign.mockResolvedValue({ url: 'https://s3.example/put', headers: {} }) + mockS3PartUrls.mockImplementation( + async (_key: string, _uploadId: string, partNumbers: number[]) => + partNumbers.map((partNumber) => ({ partNumber, url: `https://s3.example/${partNumber}` })) + ) + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('signs a whole-object PUT for the bounded URL lifetime, not the session TTL', async () => { + await createPutProviderTransfer({ + provider: 's3', + key: 'workspace/workspace-1/file.bin', + contentType: 'application/octet-stream', + fileSize: 4, + context: CONTEXT, + uploadId: 'upload-1', + uploadToken: 'token-1', + expiresAt: new Date(Date.now() + SESSION_TTL_MS), + metadata: METADATA, + }) + + expect(UPLOAD_URL_TTL_MS).toBeLessThan(SESSION_TTL_MS) + expect(mockS3Presign).toHaveBeenCalledTimes(1) + expect(mockS3Presign.mock.calls[0][0]).toMatchObject({ + expiresIn: UPLOAD_URL_TTL_MS / 1000, + }) + }) + + it('advertises the clamped URL expiry on the transfer, not the session TTL', async () => { + const sessionExpiresAt = new Date(Date.now() + SESSION_TTL_MS) + + const transfer = await createPutProviderTransfer({ + provider: 's3', + key: 'workspace/workspace-1/file.bin', + contentType: 'application/octet-stream', + fileSize: 4, + context: CONTEXT, + uploadId: 'upload-1', + uploadToken: 'token-1', + expiresAt: sessionExpiresAt, + metadata: METADATA, + }) + + expect(transfer.expiresAt).toBe(new Date(Date.now() + UPLOAD_URL_TTL_MS).toISOString()) + expect(transfer.expiresAt).not.toBe(sessionExpiresAt.toISOString()) + expect(new Date(transfer.expiresAt).getTime()).toBeLessThan(sessionExpiresAt.getTime()) + }) + + it('advertises the full session lifetime for the unsigned local data plane', async () => { + const sessionExpiresAt = new Date(Date.now() + SESSION_TTL_MS) + + const transfer = await createPutProviderTransfer({ + provider: 'local', + key: 'workspace/workspace-1/file.bin', + contentType: 'application/octet-stream', + fileSize: 4, + context: CONTEXT, + uploadId: 'upload-1', + uploadToken: 'token-1', + localOrigin: 'http://localhost:3000', + expiresAt: sessionExpiresAt, + metadata: METADATA, + }) + + expect(transfer.expiresAt).toBe(sessionExpiresAt.toISOString()) + }) + + it('never signs a PUT past the end of its session', async () => { + await createPutProviderTransfer({ + provider: 's3', + key: 'workspace/workspace-1/file.bin', + contentType: 'application/octet-stream', + fileSize: 4, + context: CONTEXT, + uploadId: 'upload-1', + uploadToken: 'token-1', + expiresAt: new Date(Date.now() + 5 * 60 * 1000), + metadata: METADATA, + }) + + expect(mockS3Presign.mock.calls[0][0]).toMatchObject({ expiresIn: 5 * 60 }) + }) + + it('refuses to sign a PUT for a session that has already expired', async () => { + await expect( + createPutProviderTransfer({ + provider: 's3', + key: 'workspace/workspace-1/file.bin', + contentType: 'application/octet-stream', + fileSize: 4, + context: CONTEXT, + uploadId: 'upload-1', + uploadToken: 'token-1', + expiresAt: new Date(Date.now() - 1), + metadata: METADATA, + }) + ).rejects.toThrow('Cannot sign an expired PUT upload session') + expect(mockS3Presign).not.toHaveBeenCalled() + }) + + it('keeps multipart part URLs on the same bounded lifetime, re-signed on demand', async () => { + const first = await getMultipartProviderPartUrls({ + provider: 's3', + providerUploadId: 'provider-upload-1', + key: 'workspace/workspace-1/file.bin', + context: CONTEXT, + partNumbers: [1], + localUrl: (partNumber) => `http://local/${partNumber}`, + }) + expect(first[0].expiresAt).toBe(new Date(Date.now() + UPLOAD_URL_TTL_MS).toISOString()) + + // A multipart session that outlives its part URLs recovers by asking the + // per-surface `.../parts` endpoint for a freshly signed window. + vi.advanceTimersByTime(23 * 60 * 60 * 1000) + const second = await getMultipartProviderPartUrls({ + provider: 's3', + providerUploadId: 'provider-upload-1', + key: 'workspace/workspace-1/file.bin', + context: CONTEXT, + partNumbers: [1], + localUrl: (partNumber) => `http://local/${partNumber}`, + }) + expect(second[0].expiresAt).toBe(new Date(Date.now() + UPLOAD_URL_TTL_MS).toISOString()) + expect(new Date(second[0].expiresAt).getTime()).toBeGreaterThan( + new Date(first[0].expiresAt).getTime() + ) + }) + + it('signs multipart parts for the same lifetime it advertises', async () => { + const urls = await getMultipartProviderPartUrls({ + provider: 's3', + providerUploadId: 'provider-upload-1', + key: 'workspace/workspace-1/file.bin', + context: CONTEXT, + partNumbers: [1], + localUrl: (partNumber) => `http://local/${partNumber}`, + }) + + // The advertised `expiresAt` and the signature's own lifetime both come from + // `UPLOAD_URL_TTL_MS`. A provider that defaults its own window instead would keep signing + // 1h URLs while the advertised expiry moved — the advertise-vs-sign mismatch this ttl + // constant exists to prevent. + const signedExpiresIn = mockS3PartUrls.mock.calls[0][4] + expect(signedExpiresIn).toBe(UPLOAD_URL_TTL_MS / 1000) + expect(new Date(urls[0].expiresAt).getTime() - Date.now()).toBe(signedExpiresIn * 1000) + }) +}) + function byteStream(...chunks: string[]): ReadableStream { const encoder = new TextEncoder() return new ReadableStream({ diff --git a/apps/sim/lib/uploads/upload-session/provider.ts b/apps/sim/lib/uploads/upload-session/provider.ts index 237bae271a5..478f6780c40 100644 --- a/apps/sim/lib/uploads/upload-session/provider.ts +++ b/apps/sim/lib/uploads/upload-session/provider.ts @@ -34,6 +34,27 @@ import { sanitizeFileKey } from '@/lib/uploads/utils/file-utils' export type { UploadStorageProvider } from '@/lib/uploads/upload-session/types' +/** + * Bounded lifetime of every signed data-plane URL this module issues. + * + * A signed transfer URL is a bearer credential for writing the object's bytes, + * so it is deliberately decoupled from the upload session's own TTL. The + * session stays addressable through its control plane (part URLs, status, + * complete, abort) for as long as it lives; the credential that can write bytes + * expires an hour after it was signed, which is the lifetime the pre-session + * presign route used. + * + * Multipart parts are re-signed on demand by the per-surface `.../parts` + * endpoints, so a long-lived multipart session always outlives its part URLs + * and recovers by asking for new ones. A whole-object PUT has no such endpoint + * and needs none: it is size-capped by `UPLOAD_SESSION_PUT_MAX_BYTES`, is + * issued and used within one client call, and is not resumable — an expired PUT + * URL and an interrupted PUT have the identical recovery of starting a new + * session. Nothing durable is written in either case, because the transfer is + * signed with a create-only precondition. + */ +export const UPLOAD_URL_TTL_MS = 60 * 60 * 1000 + export interface CompletedUploadPart { partNumber: number etag?: string @@ -130,6 +151,14 @@ export async function initiateMultipartProviderUpload(params: { return { provider, providerUploadId: null } } +/** + * Signs the whole-object PUT data plane for an upload session. + * + * The returned `expiresAt` is the transfer URL's own expiry, which is not the + * session's: cloud providers get a signature bounded by {@link UPLOAD_URL_TTL_MS}, + * while the local data plane carries no signature at all and its route admits + * the upload for as long as the session is live. + */ export async function createPutProviderTransfer(params: { provider: UploadStorageProvider key: string @@ -141,8 +170,15 @@ export async function createPutProviderTransfer(params: { localOrigin?: string expiresAt: Date metadata: Record -}): Promise<{ method: 'put'; url: string; headers: Record }> { - const expiresIn = Math.floor((params.expiresAt.getTime() - Date.now()) / 1000) +}): Promise<{ + method: 'put' + url: string + headers: Record + expiresAt: string +}> { + const expiresIn = Math.floor( + Math.min(params.expiresAt.getTime() - Date.now(), UPLOAD_URL_TTL_MS) / 1000 + ) if (expiresIn < 1) throw new Error('Cannot sign an expired PUT upload session') if (params.provider === 'local') { @@ -156,9 +192,12 @@ export async function createPutProviderTransfer(params: { 'Content-Type': params.contentType, 'upload-token': params.uploadToken, }, + expiresAt: params.expiresAt.toISOString(), } } + const signedExpiresAt = new Date(Date.now() + expiresIn * 1000).toISOString() + const config = getStorageConfig(params.context) const metadata = { ...params.metadata, uploadId: params.uploadId } if (params.provider === 's3') { @@ -171,7 +210,7 @@ export async function createPutProviderTransfer(params: { customConfig: createS3Config(config), expiresIn, }) - return { method: 'put', ...transfer } + return { method: 'put', ...transfer, expiresAt: signedExpiresAt } } if (params.provider === 'blob') { const { getBlobPresignedUploadUrl } = await import('@/lib/uploads/providers/blob/client') @@ -182,7 +221,7 @@ export async function createPutProviderTransfer(params: { customConfig: createBlobConfig(config), expiresIn, }) - return { method: 'put', ...transfer } + return { method: 'put', ...transfer, expiresAt: signedExpiresAt } } const { getGcsPresignedUploadUrl } = await import('@/lib/uploads/providers/gcs/client') const transfer = await getGcsPresignedUploadUrl( @@ -192,9 +231,23 @@ export async function createPutProviderTransfer(params: { createGcsConfig(config), expiresIn ) - return { method: 'put', url: transfer.url, headers: transfer.signedHeaders } + return { + method: 'put', + url: transfer.url, + headers: transfer.signedHeaders, + expiresAt: signedExpiresAt, + } } +/** + * Signs the per-part data plane for a multipart upload session. + * + * The signature lifetime and the advertised `expiresAt` are both derived from + * {@link UPLOAD_URL_TTL_MS} and threaded into each provider in that provider's own unit — + * seconds for S3, an absolute instant for blob and GCS. Neither side may re-derive it: a + * provider defaulting the lifetime internally is how an advertised expiry and the real + * signature drift apart. + */ export async function getMultipartProviderPartUrls(params: { provider: UploadStorageProvider providerUploadId: string | null @@ -203,7 +256,9 @@ export async function getMultipartProviderPartUrls(params: { partNumbers: number[] localUrl: (partNumber: number) => string }): Promise { - const expiresAt = new Date(Date.now() + 60 * 60 * 1000).toISOString() + const expiresOn = new Date(Date.now() + UPLOAD_URL_TTL_MS) + const expiresIn = Math.floor(UPLOAD_URL_TTL_MS / 1000) + const expiresAt = expiresOn.toISOString() if (params.provider === 'local') { return params.partNumbers.map((partNumber) => ({ partNumber, @@ -221,7 +276,8 @@ export async function getMultipartProviderPartUrls(params: { params.key, params.providerUploadId, params.partNumbers, - createS3Config(config) + createS3Config(config), + expiresIn ) return urls.map(({ partNumber, url }) => ({ partNumber, @@ -235,7 +291,8 @@ export async function getMultipartProviderPartUrls(params: { const urls = await getMultipartPartUrls( params.key, params.partNumbers, - createBlobConfig(config) + createBlobConfig(config), + expiresOn ) return urls.map(({ partNumber, url }) => ({ partNumber, @@ -249,7 +306,8 @@ export async function getMultipartProviderPartUrls(params: { params.key, params.providerUploadId, params.partNumbers, - createGcsConfig(config) + createGcsConfig(config), + expiresOn ) return urls.map(({ partNumber, url }) => ({ partNumber, diff --git a/apps/sim/lib/uploads/upload-session/service.ts b/apps/sim/lib/uploads/upload-session/service.ts index 8e3bba66b82..240bbdd7981 100644 --- a/apps/sim/lib/uploads/upload-session/service.ts +++ b/apps/sim/lib/uploads/upload-session/service.ts @@ -57,7 +57,7 @@ const cleanupDb = dbFor('cleanup') export type { UploadSessionPurpose, UploadSessionStatus, UploadTransferMethod } export type UploadSessionTransfer = - | { method: 'put'; url: string; headers: Record } + | { method: 'put'; url: string; headers: Record; expiresAt: string } | { method: 'multipart'; partSize: number; partCount: number } export interface UploadSessionRecord { diff --git a/packages/testing/src/mocks/storage-service.mock.ts b/packages/testing/src/mocks/storage-service.mock.ts index 87692f6a738..b4354531f43 100644 --- a/packages/testing/src/mocks/storage-service.mock.ts +++ b/packages/testing/src/mocks/storage-service.mock.ts @@ -9,9 +9,7 @@ import { vi } from 'vitest' * import { storageServiceMockFns } from '@sim/testing' * * storageServiceMockFns.mockHasCloudStorage.mockReturnValue(true) - * storageServiceMockFns.mockGeneratePresignedUploadUrl.mockResolvedValue({ - * uploadUrl: 'https://s3/test', key: 'workspace/x/y', ... - * }) + * storageServiceMockFns.mockGeneratePresignedDownloadUrl.mockResolvedValue('https://s3/test') * ``` */ export const storageServiceMockFns = { @@ -19,8 +17,6 @@ export const storageServiceMockFns = { mockDownloadFile: vi.fn(), mockDeleteFile: vi.fn(), mockHeadObject: vi.fn(), - mockVerifyPresignedUploadReceipt: vi.fn(), - mockGeneratePresignedUploadUrl: vi.fn(), mockGeneratePresignedDownloadUrl: vi.fn(), mockHasCloudStorage: vi.fn(() => false), mockGetS3InfoForKey: vi.fn(), @@ -39,8 +35,6 @@ export const storageServiceMock = { downloadFile: storageServiceMockFns.mockDownloadFile, deleteFile: storageServiceMockFns.mockDeleteFile, headObject: storageServiceMockFns.mockHeadObject, - verifyPresignedUploadReceipt: storageServiceMockFns.mockVerifyPresignedUploadReceipt, - generatePresignedUploadUrl: storageServiceMockFns.mockGeneratePresignedUploadUrl, generatePresignedDownloadUrl: storageServiceMockFns.mockGeneratePresignedDownloadUrl, hasCloudStorage: storageServiceMockFns.mockHasCloudStorage, getS3InfoForKey: storageServiceMockFns.mockGetS3InfoForKey,