From 4891dc6fd7e3a61d9616f4a8d4f9c1ae6b6171e3 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 13 Aug 2026 15:35:45 -0700 Subject: [PATCH 1/3] fix(v2): close the defects live probing found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Staging finally deployed the merged release, so the surface could be exercised for real. Every fix already shipped held up. These are the defects only live traffic surfaced, plus the ones a static sweep had found and left. A cursor named a position in a sequence without naming the sequence. `cursorScopeKey` hashed only the caller's filters, so any two lists filtering on nothing but `workspaceId` produced one fingerprint and accepted each other's tokens: a tables cursor replayed against the knowledge list answered 200 and silently skipped a row. Table rows never reached that check at all, so a cursor from one table paged another. Identity now comes from the route's own contract — method plus resolved path — because a hand-written name is the step an author forgets, and forgetting it is invisible. An unresolved path placeholder throws rather than fingerprinting the template, so a misconfigured route fails on every request instead of an unlucky one. Every token minted before this is refused with an accurate message; they are single-walk and unpersisted. Knowledge search and the document list answered different questions. Search grouped same-tag filters by slot and joined them with OR while the list conjoined every filter, so `gte 9` and `lte 2` on one tag returned nothing from the list and a full billed page from search. Search now conjoins. The OR grouping replaced an explicit `|OR|` mechanism that was deleted outright, was never documented in any contract, and cost the ability to express a range on a single tag; the union it gave is still reachable as separate searches. The search body also accepted an unbounded query that was billed and then silently truncated to the embedding model's window, and ignored the tag-filter cap the list enforces. A body over ten mebibytes was reported as malformed JSON. Next's proxy truncates there, well under this app's fifty-megabyte ceiling, so the parse failed on a body the caller sent whole and the size branch was unreachable. The ceiling is now clamped to what the proxy will pass. Also: a group's output columns accepted a `workflowGroupId` and discarded it; an enrichment group could never gain an output, because a new output coordinate demanded workflow metadata a group with no workflow cannot have; `newOutputColumns` alone reported success and created nothing; a saved view stored layout references to columns that do not exist while refusing the same name in a filter; an MCP server stored `retries: 0` as three and overrode an explicit auth type; a disabled server answered tool discovery with an unclassified fault; rotating a header server's headers left it reading connected; and a run whose workflow was deleted reported the root folder path while also reporting the workflow deleted. Where the honest fix was out of reach, the contract was corrected instead of half-fixing the code: the polled run resource rebuilds `error.code` by matching the persisted message, so it can never report the two codes that need block attribution, and now says so. `OUTPUT_TOO_LARGE` is removed — no path ever emitted it. `triggers` was left alone deliberately. It reads as a closed enum but production holds 43 distinct values, because a webhook run stores its provider id; pinning the enum would refuse legitimate history a log search exists to find. The description now says the vocabulary is open. --- apps/docs/openapi-v2-knowledge.json | 12 +- apps/docs/openapi-v2-logs.json | 4 +- apps/docs/openapi-v2-resources.json | 8 +- apps/docs/openapi-v2-tables.json | 6 +- apps/docs/openapi-v2-workflows.json | 25 ++- apps/sim/app/api/v2/audit-logs/route.ts | 9 +- .../sim/app/api/v2/billing/logs/route.test.ts | 5 +- apps/sim/app/api/v2/billing/logs/route.ts | 4 +- apps/sim/app/api/v2/credentials/route.ts | 4 +- apps/sim/app/api/v2/custom-tools/route.ts | 4 +- apps/sim/app/api/v2/files/route.ts | 4 +- .../api/v2/knowledge/[id]/documents/route.ts | 5 +- apps/sim/app/api/v2/knowledge/route.ts | 4 +- apps/sim/app/api/v2/lib/response.ts | 76 +++---- apps/sim/app/api/v2/logs/route.test.ts | 5 +- apps/sim/app/api/v2/logs/route.ts | 3 +- apps/sim/app/api/v2/mcp-servers/route.ts | 4 +- apps/sim/app/api/v2/secrets/route.ts | 4 +- apps/sim/app/api/v2/skills/route.test.ts | 5 +- apps/sim/app/api/v2/skills/route.ts | 7 +- .../v2/tables/[tableId]/query/route.test.ts | 29 ++- .../api/v2/tables/[tableId]/query/route.ts | 22 +- .../v2/tables/[tableId]/rows/route.test.ts | 38 +++- .../app/api/v2/tables/[tableId]/rows/route.ts | 22 +- apps/sim/app/api/v2/tables/route.ts | 4 +- .../app/api/v2/workflows/[id]/runs/route.ts | 4 +- apps/sim/app/api/v2/workflows/route.ts | 4 +- .../executor/utils/errors.classify.test.ts | 35 ++++ apps/sim/executor/utils/errors.ts | 11 +- .../api/contracts/v2/__tests__/tables.test.ts | 47 +++++ .../v2/knowledge-search-bounds.test.ts | 106 ++++++++++ apps/sim/lib/api/contracts/v2/knowledge.ts | 38 +++- apps/sim/lib/api/contracts/v2/logs.ts | 20 +- apps/sim/lib/api/contracts/v2/mcp-servers.ts | 21 +- apps/sim/lib/api/contracts/v2/tables.ts | 15 +- apps/sim/lib/api/contracts/v2/workflows.ts | 93 +++++++-- apps/sim/lib/api/cursor-binding.test.ts | 193 +++++++++++++++--- apps/sim/lib/api/cursor-binding.ts | 101 +++++++-- apps/sim/lib/api/server/validation.test.ts | 74 +++++++ apps/sim/lib/api/server/validation.ts | 27 ++- apps/sim/lib/knowledge/search/queries.test.ts | 79 ++++++- apps/sim/lib/knowledge/search/queries.ts | 50 ++--- .../lib/logs/application/get-public-log.ts | 17 +- .../application/public-log-use-cases.test.ts | 31 +++ .../sim/lib/mcp/application/use-cases.test.ts | 18 ++ apps/sim/lib/mcp/application/use-cases.ts | 14 ++ .../orchestration/server-lifecycle.test.ts | 133 ++++++++++++ .../lib/mcp/orchestration/server-lifecycle.ts | 38 +++- apps/sim/lib/table/application/groups.test.ts | 104 ++++++++++ apps/sim/lib/table/application/groups.ts | 37 +++- apps/sim/lib/table/views/service.test.ts | 36 ++++ apps/sim/lib/table/views/service.ts | 66 +++++- 52 files changed, 1473 insertions(+), 252 deletions(-) create mode 100644 apps/sim/lib/api/contracts/v2/knowledge-search-bounds.test.ts create mode 100644 apps/sim/lib/api/server/validation.test.ts diff --git a/apps/docs/openapi-v2-knowledge.json b/apps/docs/openapi-v2-knowledge.json index 9b598af8ae0..24157c46f4e 100644 --- a/apps/docs/openapi-v2-knowledge.json +++ b/apps/docs/openapi-v2-knowledge.json @@ -728,9 +728,9 @@ "name": "tagFilters", "in": "query", "required": false, - "description": "A JSON-encoded array of at most 10 tag filters, using the same display-name shape as knowledge search: `[{\"tagName\":\"category\",\"operator\":\"eq\",\"value\":\"billing\"}]`. A name that is not defined in this knowledge base is rejected, never ignored.", + "description": "A JSON-encoded array of at most 10 tag filters, using the same display-name shape as knowledge search: `[{\"tagName\":\"category\",\"operator\":\"eq\",\"value\":\"billing\"}]`. Every filter must hold, including two that name the same tag. A name that is not defined in this knowledge base is rejected, never ignored.", "schema": { - "description": "A JSON-encoded array of at most 10 tag filters, using the same display-name shape as knowledge search: `[{\"tagName\":\"category\",\"operator\":\"eq\",\"value\":\"billing\"}]`. A name that is not defined in this knowledge base is rejected, never ignored.", + "description": "A JSON-encoded array of at most 10 tag filters, using the same display-name shape as knowledge search: `[{\"tagName\":\"category\",\"operator\":\"eq\",\"value\":\"billing\"}]`. Every filter must hold, including two that name the same tag. A name that is not defined in this knowledge base is rejected, never ignored.", "examples": [ "[{\"tagName\":\"category\",\"operator\":\"eq\",\"value\":\"billing\"}]" ], @@ -2820,9 +2820,10 @@ "examples": [["7c9e6679-7425-40de-944b-e07fc1f90ae7"]] }, "query": { - "description": "Natural-language query; required when tag filters are omitted.", + "description": "Natural-language query; required when tag filters are omitted. At most 32768 characters — longer text exceeds the embedding model's per-input token ceiling and would be truncated before the billed search ran.", "examples": ["How do I reset my password?"], - "type": "string" + "type": "string", + "maxLength": 32768 }, "topK": { "default": 10, @@ -2832,7 +2833,8 @@ "maximum": 100 }, "tagFilters": { - "description": "Structured tag filters. Each filtered tag must resolve to the same slot and field type in every knowledge base selected; one missing from any of them, or defined inconsistently across them, is rejected rather than ignored, and those knowledge bases must be searched separately. List the available names with `GET /api/v2/knowledge/{id}/tags`.", + "description": "Structured tag filters, at most 10 of them. Every filter must hold, including two that name the same tag: repeating one tag narrows the result rather than widening it, matching `GET /api/v2/knowledge/{id}/documents`. To match either of two values for one tag, issue a search per value. Each filtered tag must resolve to the same slot and field type in every knowledge base selected; one missing from any of them, or defined inconsistently across them, is rejected rather than ignored, and those knowledge bases must be searched separately. List the available names with `GET /api/v2/knowledge/{id}/tags`.", + "maxItems": 10, "type": "array", "items": { "$ref": "#/components/schemas/V2KnowledgeSearchTagFilter" diff --git a/apps/docs/openapi-v2-logs.json b/apps/docs/openapi-v2-logs.json index 404a537a28b..0a64c9f581f 100644 --- a/apps/docs/openapi-v2-logs.json +++ b/apps/docs/openapi-v2-logs.json @@ -64,10 +64,10 @@ "name": "triggers", "in": "query", "required": false, - "description": "Comma-separated trigger types to include. An empty entry is rejected. The literal value `all` is a sentinel that disables this filter entirely, so a list containing it returns runs of every trigger type; no real trigger type is named `all`.", + "description": "Comma-separated trigger types to include. An empty entry is rejected. Values are matched exactly and are case-sensitive — every recorded trigger is lowercase, so `API` matches nothing while `api` matches. The vocabulary is open: it covers the core trigger types (`manual`, `api`, `schedule`, `chat`, `webhook`, `mcp`, `copilot`, `workflow`, `custom_block`) and the provider id of any webhook trigger (`slack`, `gmail`, `github`, …), so an unrecognized member is not rejected — it selects no runs. The literal value `all` is a sentinel that disables this filter entirely, so a list containing it returns runs of every trigger type; no real trigger type is named `all`.", "schema": { "type": "string", - "description": "Comma-separated trigger types to include. An empty entry is rejected. The literal value `all` is a sentinel that disables this filter entirely, so a list containing it returns runs of every trigger type; no real trigger type is named `all`." + "description": "Comma-separated trigger types to include. An empty entry is rejected. Values are matched exactly and are case-sensitive — every recorded trigger is lowercase, so `API` matches nothing while `api` matches. The vocabulary is open: it covers the core trigger types (`manual`, `api`, `schedule`, `chat`, `webhook`, `mcp`, `copilot`, `workflow`, `custom_block`) and the provider id of any webhook trigger (`slack`, `gmail`, `github`, …), so an unrecognized member is not rejected — it selects no runs. The literal value `all` is a sentinel that disables this filter entirely, so a list containing it returns runs of every trigger type; no real trigger type is named `all`." } }, { diff --git a/apps/docs/openapi-v2-resources.json b/apps/docs/openapi-v2-resources.json index 28792358443..51e85c8125b 100644 --- a/apps/docs/openapi-v2-resources.json +++ b/apps/docs/openapi-v2-resources.json @@ -2492,7 +2492,7 @@ "description": "Whether the server tools are available to workflows." }, "connectionStatus": { - "description": "Result of the most recent connection attempt. Registration and re-registration store a configuration without contacting the endpoint, so a server begins — and returns to — `disconnected` until a tool discovery runs.", + "description": "Result of the most recent connection attempt. Registration and re-registration establish no connection — the auth-type probe they may send does not count as one — so a server begins, and returns to, `disconnected` until a tool discovery runs.", "type": "string", "enum": ["connected", "disconnected", "error"] }, @@ -2694,8 +2694,7 @@ "description": "Absolute HTTP or HTTPS endpoint URL without `{{ENV_VAR}}` references. It determines server identity and is immutable: delete and recreate the server to change endpoints." }, "authType": { - "description": "Authentication method. Applied server-side as `headers` when omitted; registration never contacts the server, so an omitted value is never detected from it.", - "default": "headers", + "description": "Authentication method. When omitted, and no `headers` are sent, registration probes the endpoint once to classify it, falling back to `headers` when the probe fails or the server does not advertise OAuth. A server publishing RFC 9728 metadata is therefore stored as `oauth`, and headers configured afterwards will not authenticate — send this field explicitly to pin the method.", "type": "string", "enum": ["none", "headers", "oauth"] }, @@ -2881,8 +2880,7 @@ "maxLength": 2048 }, "authType": { - "description": "Authentication method. Applied server-side as `headers` when omitted; registration never contacts the server, so an omitted value is never detected from it.", - "default": "headers", + "description": "Authentication method. When omitted, and no `headers` are sent, registration probes the endpoint once to classify it, falling back to `headers` when the probe fails or the server does not advertise OAuth. A server publishing RFC 9728 metadata is therefore stored as `oauth`, and headers configured afterwards will not authenticate — send this field explicitly to pin the method.", "type": "string", "enum": ["none", "headers", "oauth"] }, diff --git a/apps/docs/openapi-v2-tables.json b/apps/docs/openapi-v2-tables.json index 173c373bc16..a8215eec631 100644 --- a/apps/docs/openapi-v2-tables.json +++ b/apps/docs/openapi-v2-tables.json @@ -6611,7 +6611,8 @@ "type": "boolean" } }, - "required": ["name", "type"] + "required": ["name", "type"], + "additionalProperties": false }, "description": "Columns created for producer outputs." }, @@ -6750,7 +6751,8 @@ "type": "boolean" } }, - "required": ["name", "type"] + "required": ["name", "type"], + "additionalProperties": false } }, "mappingUpdates": { diff --git a/apps/docs/openapi-v2-workflows.json b/apps/docs/openapi-v2-workflows.json index c93a2b3352e..6d9fdd9da01 100644 --- a/apps/docs/openapi-v2-workflows.json +++ b/apps/docs/openapi-v2-workflows.json @@ -1423,9 +1423,9 @@ "name": "includeOutput", "in": "query", "required": false, - "description": "Include final and block outputs when true.", + "description": "Include the final workflow output when true. It does not gate `blockOutputs`, which `selectedOutputs` selects on its own.", "schema": { - "description": "Include final and block outputs when true.", + "description": "Include the final workflow output when true. It does not gate `blockOutputs`, which `selectedOutputs` selects on its own.", "type": "boolean" } }, @@ -1433,9 +1433,9 @@ "name": "selectedOutputs", "in": "query", "required": false, - "description": "Comma-separated block output references to include.", + "description": "Comma-separated block output references to include, as `blockId` or `blockId.path`. Block *names* are not resolved here — unlike the execute request, this resource reads a recorded run and matches ids only, so a name selects nothing and yields an empty `blockOutputs`.", "schema": { - "description": "Comma-separated block output references to include.", + "description": "Comma-separated block output references to include, as `blockId` or `blockId.path`. Block *names* are not resolved here — unlike the execute request, this resource reads a recorded run and matches ids only, so a name selects nothing and yields an empty `blockOutputs`.", "type": "string" } } @@ -3882,21 +3882,20 @@ "INVALID_INPUT", "BLOCK_EXECUTION_FAILED", "CHILD_WORKFLOW_FAILED", - "OUTPUT_TOO_LARGE", "EXECUTION_FAILED" ], - "description": "Stable machine-readable execution failure code." + "description": "Stable machine-readable execution failure code. `BLOCK_EXECUTION_FAILED` and `CHILD_WORKFLOW_FAILED` are reported only where block attribution is available; elsewhere a block-level failure is reported as `EXECUTION_FAILED`." }, "blockId": { - "description": "Identifier of the failing block, when attributable.", + "description": "Identifier of the failing block. Present on the synchronous execute response only; the polled run resource and the resume response cannot attribute a block.", "type": "string" }, "blockName": { - "description": "Display name of the failing block.", + "description": "Display name of the failing block. Present on the synchronous execute response only.", "type": "string" }, "blockType": { - "description": "Integration or block type that failed.", + "description": "Integration or block type that failed. Present on the synchronous execute response only.", "type": "string" } }, @@ -4292,7 +4291,7 @@ "type": "null" } ], - "description": "Trigger type, or null before the run is recorded." + "description": "Trigger type that started the run. Backfilled as `api` for a run that is still queued, so it is populated from the first poll." }, "startedAt": { "anyOf": [ @@ -4303,7 +4302,7 @@ "type": "null" } ], - "description": "ISO 8601 start timestamp, or null while queued.", + "description": "ISO 8601 start timestamp. A queued run reports the time it was enqueued, so it is populated from the first poll.", "format": "date-time" }, "endedAt": { @@ -4453,7 +4452,7 @@ "type": "null" } ], - "description": "Structured execution failure, or null when none occurred." + "description": "Structured execution failure, or null when none occurred. Reclassified from the persisted error message, so `blockId`/`blockName`/`blockType` are absent and a block-level failure reports `EXECUTION_FAILED` here even when the same run reported `BLOCK_EXECUTION_FAILED` on its synchronous execute response." }, "output": { "anyOf": [ @@ -4481,7 +4480,7 @@ "type": "null" } ], - "description": "Selected block outputs when requested, otherwise null." + "description": "Outputs of the blocks named by `selectedOutputs`, or null when none were requested. Gated by `selectedOutputs` alone — `includeOutput` governs `output` only." } }, "required": [ diff --git a/apps/sim/app/api/v2/audit-logs/route.ts b/apps/sim/app/api/v2/audit-logs/route.ts index d69532e01ae..207251b3145 100644 --- a/apps/sim/app/api/v2/audit-logs/route.ts +++ b/apps/sim/app/api/v2/audit-logs/route.ts @@ -1,5 +1,10 @@ import { v2ListAuditLogsContract } from '@/lib/api/contracts/v2/audit-logs' -import { cursorScopeKey, instantScopePart, unorderedScopePart } from '@/lib/api/cursor-binding' +import { + cursorRoute, + cursorScopeKey, + instantScopePart, + unorderedScopePart, +} from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, @@ -23,7 +28,7 @@ function auditLogCursorFilters(query: { startDate?: string endDate?: string }) { - return cursorScopeKey({ + return cursorScopeKey(cursorRoute(v2ListAuditLogsContract), { organizationId: query.organizationId, includeDeparted: query.includeDeparted, action: query.action, diff --git a/apps/sim/app/api/v2/billing/logs/route.test.ts b/apps/sim/app/api/v2/billing/logs/route.test.ts index f1534458b47..c929a42215a 100644 --- a/apps/sim/app/api/v2/billing/logs/route.test.ts +++ b/apps/sim/app/api/v2/billing/logs/route.test.ts @@ -24,7 +24,8 @@ vi.mock('@/lib/billing/application/list-billing-logs', () => ({ listBillingLogs: { operation: { id: 'billing.logs.list' }, execute: mocks.execute }, })) -import { cursorScopeKey } from '@/lib/api/cursor-binding' +import { v2ListBillingLogsContract } from '@/lib/api/contracts/v2/billing' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { UNKNOWN_CURSOR_MESSAGE } from '@/lib/billing/core/usage-log' import { OrchestrationError } from '@/lib/core/orchestration/types' import { GET } from '@/app/api/v2/billing/logs/route' @@ -35,7 +36,7 @@ function ledgerCursor( inner: string, filters: { source?: string; workspaceId?: string; period?: string } ): string { - return encodeScopedCursor(cursorScopeKey(filters), inner) + return encodeScopedCursor(cursorScopeKey(cursorRoute(v2ListBillingLogsContract), filters), inner) } const auth = { diff --git a/apps/sim/app/api/v2/billing/logs/route.ts b/apps/sim/app/api/v2/billing/logs/route.ts index f306ff40682..61f1e4acd80 100644 --- a/apps/sim/app/api/v2/billing/logs/route.ts +++ b/apps/sim/app/api/v2/billing/logs/route.ts @@ -1,5 +1,5 @@ import { v2ListBillingLogsContract } from '@/lib/api/contracts/v2/billing' -import { cursorScopeKey, instantScopePart } from '@/lib/api/cursor-binding' +import { cursorRoute, cursorScopeKey, instantScopePart } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, v2RateLimits } from '@/lib/api/server/routes' import { v2BillingErrorPolicies } from '@/lib/billing/api/route-policies' import { listBillingLogs } from '@/lib/billing/application/list-billing-logs' @@ -32,7 +32,7 @@ function billingLogCursorFilters(query: { startDate?: string endDate?: string }) { - return cursorScopeKey({ + return cursorScopeKey(cursorRoute(v2ListBillingLogsContract), { source: query.source, workspaceId: query.workspaceId, period: query.period, diff --git a/apps/sim/app/api/v2/credentials/route.ts b/apps/sim/app/api/v2/credentials/route.ts index bccfffccbe2..b0dde7a39ab 100644 --- a/apps/sim/app/api/v2/credentials/route.ts +++ b/apps/sim/app/api/v2/credentials/route.ts @@ -1,6 +1,6 @@ import type { V2Credential } from '@/lib/api/contracts/v2/credentials' import { v2ListCredentialsContract } from '@/lib/api/contracts/v2/credentials' -import { cursorScopeKey } from '@/lib/api/cursor-binding' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, @@ -42,7 +42,7 @@ function credentialCursorFilters(query: { providerId?: string search?: string }) { - return cursorScopeKey({ + return cursorScopeKey(cursorRoute(v2ListCredentialsContract), { workspaceId: query.workspaceId, type: query.type, providerId: query.providerId, diff --git a/apps/sim/app/api/v2/custom-tools/route.ts b/apps/sim/app/api/v2/custom-tools/route.ts index ca027ae013a..4cbfafb5492 100644 --- a/apps/sim/app/api/v2/custom-tools/route.ts +++ b/apps/sim/app/api/v2/custom-tools/route.ts @@ -2,7 +2,7 @@ import { v2CreateCustomToolContract, v2ListCustomToolsContract, } from '@/lib/api/contracts/v2/custom-tools' -import { cursorScopeKey } from '@/lib/api/cursor-binding' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, @@ -22,7 +22,7 @@ export const revalidate = 0 /** Every param that changes which custom tools, in which order, this list returns. */ function customToolCursorFilters(query: { workspaceId: string; search?: string }) { - return cursorScopeKey({ + return cursorScopeKey(cursorRoute(v2ListCustomToolsContract), { workspaceId: query.workspaceId, search: query.search, }) diff --git a/apps/sim/app/api/v2/files/route.ts b/apps/sim/app/api/v2/files/route.ts index 31930cf1e60..e544f4bf231 100644 --- a/apps/sim/app/api/v2/files/route.ts +++ b/apps/sim/app/api/v2/files/route.ts @@ -3,7 +3,7 @@ import { v2CreateFileContract, v2ListFilesContract, } from '@/lib/api/contracts/v2/files' -import { cursorScopeKey } from '@/lib/api/cursor-binding' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, v2RateLimits } from '@/lib/api/server/routes' import { getFileExtension, getMimeTypeFromExtension } from '@/lib/uploads/utils/file-utils' import { v2FileErrorPolicies } from '@/lib/workspace-files/api' @@ -24,7 +24,7 @@ function fileCursorFilters(query: { folderPath?: string search?: string }) { - return cursorScopeKey({ + return cursorScopeKey(cursorRoute(v2ListFilesContract), { workspaceId: query.workspaceId, scope: query.scope, folderPath: query.folderPath, diff --git a/apps/sim/app/api/v2/knowledge/[id]/documents/route.ts b/apps/sim/app/api/v2/knowledge/[id]/documents/route.ts index c6440f890de..436567fe845 100644 --- a/apps/sim/app/api/v2/knowledge/[id]/documents/route.ts +++ b/apps/sim/app/api/v2/knowledge/[id]/documents/route.ts @@ -5,7 +5,7 @@ import { v2ListKnowledgeDocumentsContract, v2UploadKnowledgeDocumentContract, } from '@/lib/api/contracts/v2/knowledge' -import { cursorScopeKey, unorderedScopeOf } from '@/lib/api/cursor-binding' +import { cursorRoute, cursorScopeKey, unorderedScopeOf } from '@/lib/api/cursor-binding' import { defineV2BodyLifecycleRoute, defineV2JsonRoute, @@ -59,8 +59,7 @@ function documentCursorFilters( query: { workspaceId: string; enabledFilter?: string; search?: string; tagFilters?: string } ) { const parsed = parseV2KnowledgeTagFiltersParam(query.tagFilters) - return cursorScopeKey({ - knowledgeBaseId, + return cursorScopeKey(cursorRoute(v2ListKnowledgeDocumentsContract, { id: knowledgeBaseId }), { workspaceId: query.workspaceId, enabledFilter: query.enabledFilter, search: query.search, diff --git a/apps/sim/app/api/v2/knowledge/route.ts b/apps/sim/app/api/v2/knowledge/route.ts index 07d9be95f16..8bfaabf2fcd 100644 --- a/apps/sim/app/api/v2/knowledge/route.ts +++ b/apps/sim/app/api/v2/knowledge/route.ts @@ -2,7 +2,7 @@ import { v2CreateKnowledgeBaseContract, v2ListKnowledgeBasesContract, } from '@/lib/api/contracts/v2/knowledge' -import { cursorScopeKey } from '@/lib/api/cursor-binding' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, @@ -28,7 +28,7 @@ function knowledgeCursorFilters(query: { folderPath?: string search?: string }) { - return cursorScopeKey({ + return cursorScopeKey(cursorRoute(v2ListKnowledgeBasesContract), { workspaceId: query.workspaceId, folderPath: query.folderPath, search: query.search, diff --git a/apps/sim/app/api/v2/lib/response.ts b/apps/sim/app/api/v2/lib/response.ts index 7ffd9fc9f88..8f5f61215b5 100644 --- a/apps/sim/app/api/v2/lib/response.ts +++ b/apps/sim/app/api/v2/lib/response.ts @@ -251,22 +251,14 @@ export function decodeCursor>(cursor: string): T | n interface OffsetCursorPayload { /** The ordering the offset counts positions within. */ sort: string - /** Fingerprint of the filters the offset counts positions within. */ - filter?: string + /** Fingerprint of the list and filters the offset counts positions within. */ + filter: string offset: number } -/** An offset cursor stamped with the sort and filters that produced it. */ -export function encodeOffsetCursor( - sort: string, - filter: string | undefined, - offset: number -): string { - return encodeCursor({ - sort, - ...(filter ? { filter } : {}), - offset, - } satisfies OffsetCursorPayload) +/** An offset cursor stamped with the sort and scope that produced it. */ +export function encodeOffsetCursor(sort: string, filter: string, offset: number): string { + return encodeCursor({ sort, filter, offset } satisfies OffsetCursorPayload) } /** @@ -288,14 +280,15 @@ export function encodeOffsetCursor( export function decodeOffsetCursor( cursor: string | undefined, sort: string, - filter?: string | undefined + filter: string ): number { if (!cursor) return 0 const decoded = decodeCursor>(cursor) - if (!decoded || decoded.sort !== sort) { + if (!decoded) throw new OrchestrationError('validation', UNREADABLE_CURSOR_MESSAGE) + if (decoded.sort !== sort) { throw new OrchestrationError('validation', INVALID_CURSOR_MESSAGE) } - if ((decoded.filter ?? undefined) !== (filter || undefined)) { + if ((decoded.filter ?? undefined) !== filter) { throw new OrchestrationError('validation', REFILTERED_CURSOR_MESSAGE) } const { offset } = decoded @@ -317,29 +310,27 @@ export function cursorSortKey(sortBy: string, sortOrder: string): string { interface SortedCursorPayload { sort: string keys: CursorKey[] - /** Fingerprint of the filters the page was read under; absent = unfiltered. */ - filter?: string + /** Fingerprint of the list and filters the page was read under. */ + filter: string } /** - * A keyset cursor stamped with the sort AND the filters that produced it. The + * A keyset cursor stamped with the sort AND the scope that produced it. The * keys are only meaningful under that exact ordering, and only name a useful * position within that exact row set, so both stamps travel with them. */ -export function encodeSortedCursor( - sort: string, - keys: CursorKey[], - filter?: string | undefined -): string { - return encodeCursor({ sort, keys, ...(filter ? { filter } : {}) } satisfies SortedCursorPayload) +export function encodeSortedCursor(sort: string, keys: CursorKey[], filter: string): string { + return encodeCursor({ sort, keys, filter } satisfies SortedCursorPayload) } type DecodedSortedCursor = | { status: 'absent' } | { status: 'ok'; keys: CursorKey[] } - /** Malformed, or minted under a different sort — the page cannot be resumed. */ + /** Not a pagination cursor at all — it does not decode into one. */ + | { status: 'unreadable' } + /** Minted under a different sort — the keys compare the wrong column. */ | { status: 'invalid' } - /** Minted under different filters — the position names another sequence. */ + /** Minted under a different list or different filters — another sequence. */ | { status: 'refiltered' } /** @@ -365,14 +356,15 @@ type DecodedSortedCursor = export function decodeSortedCursor( cursor: string | undefined, sort: string, - filter?: string | undefined + filter: string ): DecodedSortedCursor { if (!cursor) return { status: 'absent' } const decoded = decodeCursor>(cursor) - if (!decoded || decoded.sort !== sort || !Array.isArray(decoded.keys)) { - return { status: 'invalid' } + if (!decoded || typeof decoded.sort !== 'string' || !Array.isArray(decoded.keys)) { + return { status: 'unreadable' } } - if ((decoded.filter ?? undefined) !== (filter || undefined)) return { status: 'refiltered' } + if (decoded.sort !== sort) return { status: 'invalid' } + if ((decoded.filter ?? undefined) !== filter) return { status: 'refiltered' } return { status: 'ok', keys: decoded.keys } } @@ -392,9 +384,12 @@ export function readSortedCursor( cursor: string | undefined, sortBy: string, sortOrder: string, - filter?: string | undefined + filter: string ): CursorKey[] | undefined { const decoded = decodeSortedCursor(cursor, cursorSortKey(sortBy, sortOrder), filter) + if (decoded.status === 'unreadable') { + throw new OrchestrationError('validation', UNREADABLE_CURSOR_MESSAGE) + } if (decoded.status === 'invalid') { throw new OrchestrationError('validation', INVALID_CURSOR_MESSAGE) } @@ -416,14 +411,14 @@ export function writeSortedCursor( keys: CursorKey[] | null | undefined, sortBy: string, sortOrder: string, - filter?: string | undefined + filter: string ): string | null { return keys ? encodeSortedCursor(cursorSortKey(sortBy, sortOrder), keys, filter) : null } interface ScopedCursorPayload { - /** Fingerprint of the filters and sort the inner token was minted under. */ - scope?: string + /** Fingerprint of the list, filters, and sort the inner token was minted under. */ + scope: string /** The domain codec's own opaque token, passed through untouched. */ inner: string } @@ -438,8 +433,8 @@ interface ScopedCursorPayload { * lists the same binding as the rest of the surface — one rule for v2 callers * rather than "some lists notice, some don't". */ -export function encodeScopedCursor(scope: string | undefined, inner: string): string { - return encodeCursor({ ...(scope ? { scope } : {}), inner } satisfies ScopedCursorPayload) +export function encodeScopedCursor(scope: string, inner: string): string { + return encodeCursor({ scope, inner } satisfies ScopedCursorPayload) } /** @@ -456,16 +451,13 @@ export function encodeScopedCursor(scope: string | undefined, inner: string): st * reached through the wrapper instead of through the token, and it slipped past * the unresolvable-cursor 400 that exists to stop it. */ -export function readScopedCursor( - cursor: string | undefined, - scope: string | undefined -): string | undefined { +export function readScopedCursor(cursor: string | undefined, scope: string): string | undefined { if (!cursor) return undefined const decoded = decodeCursor>(cursor) if (!decoded || typeof decoded.inner !== 'string' || decoded.inner.length === 0) { throw new OrchestrationError('validation', UNREADABLE_CURSOR_MESSAGE) } - if ((decoded.scope ?? undefined) !== (scope || undefined)) { + if ((decoded.scope ?? undefined) !== scope) { throw new OrchestrationError('validation', REFILTERED_CURSOR_MESSAGE) } return decoded.inner diff --git a/apps/sim/app/api/v2/logs/route.test.ts b/apps/sim/app/api/v2/logs/route.test.ts index 277f05cd1f1..36917bedbab 100644 --- a/apps/sim/app/api/v2/logs/route.test.ts +++ b/apps/sim/app/api/v2/logs/route.test.ts @@ -24,7 +24,8 @@ vi.mock('@/lib/logs/application/list-public-logs', () => ({ listPublicLogs: { operation: { id: 'logs.list' }, execute: mocks.execute }, })) -import { cursorScopeKey, UNREADABLE_CURSOR_MESSAGE } from '@/lib/api/cursor-binding' +import { v2ListLogsContract } from '@/lib/api/contracts/v2/logs' +import { cursorRoute, cursorScopeKey, UNREADABLE_CURSOR_MESSAGE } from '@/lib/api/cursor-binding' import { OrchestrationError } from '@/lib/core/orchestration/types' import { encodeScopedCursor } from '@/app/api/v2/lib/response' import { GET } from '@/app/api/v2/logs/route' @@ -202,7 +203,7 @@ describe('GET /api/v2/logs', () => { */ it('rejects a cursor whose inner token is empty instead of restarting at page one', async () => { const cursor = encodeScopedCursor( - cursorScopeKey({ workspaceId: WORKSPACE_ID, order: 'desc' }), + cursorScopeKey(cursorRoute(v2ListLogsContract), { workspaceId: WORKSPACE_ID, order: 'desc' }), '' ) diff --git a/apps/sim/app/api/v2/logs/route.ts b/apps/sim/app/api/v2/logs/route.ts index 5ecf1a474f3..74123a88266 100644 --- a/apps/sim/app/api/v2/logs/route.ts +++ b/apps/sim/app/api/v2/logs/route.ts @@ -5,6 +5,7 @@ import { v2LogStatusSchema, } from '@/lib/api/contracts/v2/logs' import { + cursorRoute, cursorScopeKey, instantScopePart, parseUnorderedList, @@ -45,7 +46,7 @@ function logCursorFilters(query: { folderPaths?: string order?: string }) { - return cursorScopeKey({ + return cursorScopeKey(cursorRoute(v2ListLogsContract), { workspaceId: query.workspaceId, workflowIds: unorderedScopePart(query.workflowIds), triggers: unorderedScopePart(query.triggers), diff --git a/apps/sim/app/api/v2/mcp-servers/route.ts b/apps/sim/app/api/v2/mcp-servers/route.ts index 37757f7824b..91ea160afc2 100644 --- a/apps/sim/app/api/v2/mcp-servers/route.ts +++ b/apps/sim/app/api/v2/mcp-servers/route.ts @@ -2,7 +2,7 @@ import { v2CreateMcpServerContract, v2ListMcpServersContract, } from '@/lib/api/contracts/v2/mcp-servers' -import { cursorScopeKey } from '@/lib/api/cursor-binding' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, @@ -20,7 +20,7 @@ export const revalidate = 0 /** Every param that changes which MCP servers, in which order, this list returns. */ function mcpServerCursorFilters(query: { workspaceId: string; search?: string }) { - return cursorScopeKey({ + return cursorScopeKey(cursorRoute(v2ListMcpServersContract), { workspaceId: query.workspaceId, search: query.search, }) diff --git a/apps/sim/app/api/v2/secrets/route.ts b/apps/sim/app/api/v2/secrets/route.ts index 591508f8be1..bd9a44c1d39 100644 --- a/apps/sim/app/api/v2/secrets/route.ts +++ b/apps/sim/app/api/v2/secrets/route.ts @@ -1,5 +1,5 @@ import { v2ListSecretsContract } from '@/lib/api/contracts/v2/secrets' -import { cursorScopeKey } from '@/lib/api/cursor-binding' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, @@ -16,7 +16,7 @@ export const revalidate = 0 /** Every param that changes which secrets, in which order, this list returns. */ function secretCursorFilters(query: { workspaceId: string; scope?: string; search?: string }) { - return cursorScopeKey({ + return cursorScopeKey(cursorRoute(v2ListSecretsContract), { workspaceId: query.workspaceId, scope: query.scope, search: query.search, diff --git a/apps/sim/app/api/v2/skills/route.test.ts b/apps/sim/app/api/v2/skills/route.test.ts index e84de0dfcdb..40b8668b169 100644 --- a/apps/sim/app/api/v2/skills/route.test.ts +++ b/apps/sim/app/api/v2/skills/route.test.ts @@ -50,7 +50,8 @@ vi.mock('@/lib/skills/application/use-cases', () => ({ createSkillUseCase: { operation: { id: 'skills.create' }, execute: mocks.create }, })) -import { cursorScopeKey } from '@/lib/api/cursor-binding' +import { v2ListSkillsContract } from '@/lib/api/contracts/v2/skills' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { PrincipalKindAuthorizationError } from '@/lib/core/application' import { cursorSortKey, encodeOffsetCursor } from '@/app/api/v2/lib/response' import { GET, POST } from '@/app/api/v2/skills/route' @@ -75,7 +76,7 @@ function skillCursor({ }): string { return encodeOffsetCursor( cursorSortKey(sortBy, sortOrder), - cursorScopeKey({ workspaceId: WORKSPACE_ID, search }), + cursorScopeKey(cursorRoute(v2ListSkillsContract), { workspaceId: WORKSPACE_ID, search }), offset ) } diff --git a/apps/sim/app/api/v2/skills/route.ts b/apps/sim/app/api/v2/skills/route.ts index a0bf8acdeb2..42de2765968 100644 --- a/apps/sim/app/api/v2/skills/route.ts +++ b/apps/sim/app/api/v2/skills/route.ts @@ -1,5 +1,5 @@ import { v2CreateSkillContract, v2ListSkillsContract } from '@/lib/api/contracts/v2/skills' -import { cursorScopeKey } from '@/lib/api/cursor-binding' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, @@ -14,7 +14,10 @@ import { toV2Skill, toV2SkillSummary } from '@/app/api/v2/skills/utils' /** Every param that changes which skills, in which order, this list returns. */ function skillCursorFilters(query: { workspaceId: string; search?: string }) { - return cursorScopeKey({ workspaceId: query.workspaceId, search: query.search }) + return cursorScopeKey(cursorRoute(v2ListSkillsContract), { + workspaceId: query.workspaceId, + search: query.search, + }) } export const dynamic = 'force-dynamic' diff --git a/apps/sim/app/api/v2/tables/[tableId]/query/route.test.ts b/apps/sim/app/api/v2/tables/[tableId]/query/route.test.ts index d32782c9bb2..e271d8eee6c 100644 --- a/apps/sim/app/api/v2/tables/[tableId]/query/route.test.ts +++ b/apps/sim/app/api/v2/tables/[tableId]/query/route.test.ts @@ -39,8 +39,16 @@ vi.mock('@/lib/table/application/rows', () => ({ queryTableRows: { operation: { id: 'tables.rows.query' }, execute: mocks.queryRows }, })) +import { v2QueryRowsContract } from '@/lib/api/contracts/v2/tables' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' +import { encodeScopedCursor } from '@/app/api/v2/lib/response' import { POST } from '@/app/api/v2/tables/[tableId]/query/route' +/** A query cursor exactly as the route mints one, for the table given. */ +function queryCursor(tableId: string, inner: string): string { + return encodeScopedCursor(cursorScopeKey(cursorRoute(v2QueryRowsContract, { tableId })), inner) +} + const WORKSPACE_ID = 'workspace-1' const PRINCIPAL = { kind: 'workspace_api_key' as const, @@ -152,12 +160,31 @@ describe('POST /api/v2/tables/[tableId]/query', () => { new MockTableRowsValidationError('Invalid cursor', { code: 'INVALID_CURSOR' }) ) - const response = await call({ workspaceId: WORKSPACE_ID, cursor: 'malformed' }).response + const response = await call({ + workspaceId: WORKSPACE_ID, + cursor: queryCursor('table-1', 'malformed'), + }).response expect(response.status).toBe(400) expect((await response.json()).error.details).toEqual({ code: 'INVALID_CURSOR' }) }) + /** + * The row codec binds the predicate and sort a page was produced under but + * carries no table identity, so an unfiltered token from one table decoded + * cleanly against another and answered 200 with that other table's rows. + */ + it('refuses a query cursor minted on a different table', async () => { + const response = await call({ + workspaceId: WORKSPACE_ID, + cursor: queryCursor('table-2', 'native-row-cursor'), + }).response + + expect(response.status).toBe(400) + expect((await response.json()).error.message).toMatch(/requested filters/) + expect(mocks.queryRows).not.toHaveBeenCalled() + }) + it('enforces the one MiB body cap before delegation', async () => { const response = await call({ workspaceId: WORKSPACE_ID, cursor: 'x'.repeat(1024 * 1024) }) .response diff --git a/apps/sim/app/api/v2/tables/[tableId]/query/route.ts b/apps/sim/app/api/v2/tables/[tableId]/query/route.ts index 76805afa9b5..5b9d0516056 100644 --- a/apps/sim/app/api/v2/tables/[tableId]/query/route.ts +++ b/apps/sim/app/api/v2/tables/[tableId]/query/route.ts @@ -1,15 +1,29 @@ import { TABLE_QUERY_MAX_BODY_BYTES } from '@/lib/api/contracts/tables' import { V2_DEFAULT_ROW_LIMIT, v2QueryRowsContract } from '@/lib/api/contracts/v2/tables' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, v2RateLimits } from '@/lib/api/server/routes' import { v2TableRowsErrorPolicy } from '@/lib/table/api/row-route-policies' import { tableOperations } from '@/lib/table/application/operations' import { queryTableRows } from '@/lib/table/application/rows' import { namedRowMapper } from '@/lib/table/cell-format' +import { encodeScopedCursor, readScopedCursor } from '@/app/api/v2/lib/response' import { toApiRow } from '@/app/api/v2/tables/utils' export const dynamic = 'force-dynamic' export const revalidate = 0 +/** + * The sequence a query cursor names a position in: this list, on THIS table. + * + * The row codec binds the predicate and sort a page was produced under, but not + * the table — so an unfiltered token from one table decoded cleanly against + * another and answered 200 with that other table's rows. The table id lives in + * the path, so the route is the only place that knows it. + */ +function queryRowCursorScope(tableId: string): string { + return cursorScopeKey(cursorRoute(v2QueryRowsContract, { tableId })) +} + export const POST = defineV2JsonRoute({ contract: v2QueryRowsContract, operation: tableOperations.queryRows, @@ -22,17 +36,19 @@ export const POST = defineV2JsonRoute({ assertedWorkspaceId: body.workspaceId, predicate: body.predicate, sort: body.sort, - cursor: body.cursor, + cursor: readScopedCursor(body.cursor, queryRowCursorScope(params.tableId)), limit: body.limit === undefined ? V2_DEFAULT_ROW_LIMIT : body.limit === 0 ? undefined : body.limit, includeTotal: false, }), useCase: queryTableRows, - present: ({ table, rows, nextCursor }) => { + present: ({ table, rows, nextCursor }, { params }) => { const toNamedRow = namedRowMapper(table.schema.columns) return { data: rows.map((row) => toApiRow(row, toNamedRow)), - nextCursor, + nextCursor: nextCursor + ? encodeScopedCursor(queryRowCursorScope(params.tableId), nextCursor) + : null, } }, }) diff --git a/apps/sim/app/api/v2/tables/[tableId]/rows/route.test.ts b/apps/sim/app/api/v2/tables/[tableId]/rows/route.test.ts index d36a43f1516..1f0b8a3b5e7 100644 --- a/apps/sim/app/api/v2/tables/[tableId]/rows/route.test.ts +++ b/apps/sim/app/api/v2/tables/[tableId]/rows/route.test.ts @@ -38,8 +38,19 @@ vi.mock('@/lib/table/application/rows', () => ({ deleteTableRows: { operation: { id: 'tables.rows.delete_many' }, execute: mocks.deleteRows }, })) +import { v2ListTableRowsContract } from '@/lib/api/contracts/v2/tables' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' +import { encodeScopedCursor } from '@/app/api/v2/lib/response' import { DELETE, GET, PATCH, POST } from '@/app/api/v2/tables/[tableId]/rows/route' +/** A row cursor exactly as the route mints one, for the table given. */ +function rowCursor(tableId: string, inner: string): string { + return encodeScopedCursor( + cursorScopeKey(cursorRoute(v2ListTableRowsContract, { tableId })), + inner + ) +} + const WORKSPACE_ID = 'workspace-1' const PRINCIPAL = { kind: 'workspace_api_key' as const, @@ -102,7 +113,7 @@ describe('/api/v2/tables/[tableId]/rows', () => { }) it('passes the opaque native row cursor through the route unchanged', async () => { - const cursor = 'native-row-cursor' + const cursor = rowCursor('table-1', 'native-row-cursor') mocks.listRows.mockResolvedValue({ table: TABLE, rows: [ROW], @@ -122,11 +133,32 @@ describe('/api/v2/tables/[tableId]/rows', () => { tableId: 'table-1', assertedWorkspaceId: WORKSPACE_ID, limit: 25, - cursor, + cursor: 'native-row-cursor', }, request: req, }) - expect((await response.json()).nextCursor).toBe('next-native-cursor') + expect((await response.json()).nextCursor).toBe(rowCursor('table-1', 'next-native-cursor')) + }) + + /** + * The row codec binds the sort and predicate a page was produced under but + * carries no table identity, so an unfiltered token from one table decoded + * cleanly against another and answered 200 with that other table's rows. + */ + it('refuses a row cursor minted on a different table', async () => { + const foreign = rowCursor('table-2', 'native-row-cursor') + const response = await GET( + request( + 'GET', + undefined, + `?workspaceId=${WORKSPACE_ID}&limit=25&cursor=${encodeURIComponent(foreign)}` + ), + CONTEXT + ) + + expect(response.status).toBe(400) + expect((await response.json()).error.message).toMatch(/requested filters/) + expect(mocks.listRows).not.toHaveBeenCalled() }) it('rejects an unauthenticated request', async () => { diff --git a/apps/sim/app/api/v2/tables/[tableId]/rows/route.ts b/apps/sim/app/api/v2/tables/[tableId]/rows/route.ts index e309607496f..4727a7bba6a 100644 --- a/apps/sim/app/api/v2/tables/[tableId]/rows/route.ts +++ b/apps/sim/app/api/v2/tables/[tableId]/rows/route.ts @@ -4,6 +4,7 @@ import { v2ListTableRowsContract, v2UpdateRowsByFilterContract, } from '@/lib/api/contracts/v2/tables' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, v2RateLimits } from '@/lib/api/server/routes' import { v2TableRowsErrorPolicy } from '@/lib/table/api/row-route-policies' import { tableOperations } from '@/lib/table/application/operations' @@ -14,11 +15,24 @@ import { updateTableRows, } from '@/lib/table/application/rows' import { namedRowMapper } from '@/lib/table/cell-format' +import { encodeScopedCursor, readScopedCursor } from '@/app/api/v2/lib/response' import { toApiRow } from '@/app/api/v2/tables/utils' export const dynamic = 'force-dynamic' export const revalidate = 0 +/** + * The sequence a row cursor names a position in: this list, on THIS table. + * + * The row codec binds a token to the sort and predicate it was minted under but + * carries no table identity, so an unfiltered token from one table decoded + * cleanly against another and answered 200 with that other table's rows. The + * table id lives in the path, so the route is the only place that knows it. + */ +function tableRowCursorScope(tableId: string): string { + return cursorScopeKey(cursorRoute(v2ListTableRowsContract, { tableId })) +} + export const GET = defineV2JsonRoute({ contract: v2ListTableRowsContract, operation: tableOperations.listRows, @@ -29,14 +43,16 @@ export const GET = defineV2JsonRoute({ tableId: params.tableId, assertedWorkspaceId: query.workspaceId, limit: query.limit, - cursor: query.cursor, + cursor: readScopedCursor(query.cursor, tableRowCursorScope(params.tableId)), }), useCase: listTableRows, - present: ({ table, rows, nextCursor }) => { + present: ({ table, rows, nextCursor }, { params }) => { const toNamedRow = namedRowMapper(table.schema.columns) return { data: rows.map((row) => toApiRow(row, toNamedRow)), - nextCursor, + nextCursor: nextCursor + ? encodeScopedCursor(tableRowCursorScope(params.tableId), nextCursor) + : null, } }, }) diff --git a/apps/sim/app/api/v2/tables/route.ts b/apps/sim/app/api/v2/tables/route.ts index d4d715fd7fd..a91b9c4ef61 100644 --- a/apps/sim/app/api/v2/tables/route.ts +++ b/apps/sim/app/api/v2/tables/route.ts @@ -1,5 +1,5 @@ import { v2CreateTableContract, v2ListTablesContract } from '@/lib/api/contracts/v2/tables' -import { cursorScopeKey } from '@/lib/api/cursor-binding' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, v2RateLimits } from '@/lib/api/server/routes' import { v2TableErrorPolicies } from '@/lib/table/api' import { tableOperations } from '@/lib/table/application/operations' @@ -12,7 +12,7 @@ export const revalidate = 0 /** Every param that changes which tables, in which order, this list returns. */ function tableCursorFilters(query: { workspaceId: string; folderPath?: string; search?: string }) { - return cursorScopeKey({ + return cursorScopeKey(cursorRoute(v2ListTablesContract), { workspaceId: query.workspaceId, folderPath: query.folderPath, search: query.search, diff --git a/apps/sim/app/api/v2/workflows/[id]/runs/route.ts b/apps/sim/app/api/v2/workflows/[id]/runs/route.ts index b76eb2e88f5..e504143b335 100644 --- a/apps/sim/app/api/v2/workflows/[id]/runs/route.ts +++ b/apps/sim/app/api/v2/workflows/[id]/runs/route.ts @@ -4,6 +4,7 @@ import { v2WorkflowRunListStatusValueSchema, } from '@/lib/api/contracts/v2/workflows' import { + cursorRoute, cursorScopeKey, instantScopePart, UNREADABLE_CURSOR_MESSAGE, @@ -23,8 +24,7 @@ function runCursorFilters( workflowId: string, query: { status?: string; trigger?: string; startDate?: string; endDate?: string } ) { - return cursorScopeKey({ - workflowId, + return cursorScopeKey(cursorRoute(v2ListWorkflowRunsContract, { id: workflowId }), { status: query.status, trigger: query.trigger, startDate: instantScopePart(query.startDate), diff --git a/apps/sim/app/api/v2/workflows/route.ts b/apps/sim/app/api/v2/workflows/route.ts index c8095d27ce2..d291c231a6c 100644 --- a/apps/sim/app/api/v2/workflows/route.ts +++ b/apps/sim/app/api/v2/workflows/route.ts @@ -1,6 +1,6 @@ import type { V2WorkflowListItem } from '@/lib/api/contracts/v2/workflows' import { v2CreateWorkflowContract, v2ListWorkflowsContract } from '@/lib/api/contracts/v2/workflows' -import { cursorScopeKey } from '@/lib/api/cursor-binding' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, @@ -22,7 +22,7 @@ function workflowCursorFilters(query: { deployedOnly: boolean search?: string }) { - return cursorScopeKey({ + return cursorScopeKey(cursorRoute(v2ListWorkflowsContract), { workspaceId: query.workspaceId, folderPath: query.folderPath, deployedOnly: query.deployedOnly, diff --git a/apps/sim/executor/utils/errors.classify.test.ts b/apps/sim/executor/utils/errors.classify.test.ts index 3051cf6a52c..0e91f223679 100644 --- a/apps/sim/executor/utils/errors.classify.test.ts +++ b/apps/sim/executor/utils/errors.classify.test.ts @@ -2,11 +2,13 @@ * @vitest-environment node */ import { describe, expect, it } from 'vitest' +import { v2ExecutionErrorSchema } from '@/lib/api/contracts/v2/workflows' import type { ExecutionResult } from '@/executor/types' import { attachExecutionResult, buildBlockExecutionError, classifyExecutionError, + type WorkflowExecutionErrorCode, } from '@/executor/utils/errors' function failedResult(partial?: Partial): ExecutionResult { @@ -146,4 +148,37 @@ describe('classifyExecutionError', () => { blockType: undefined, }) }) + + /** + * The published enum promises callers a code to route on, so a member this + * function cannot produce is a branch no response ever takes — the state + * `OUTPUT_TOO_LARGE` was in until it was retired, since an oversize run + * response is an HTTP 413 and never reaches classification. Pinning the two + * together makes the next unreachable member fail here instead of shipping + * into SDK types. + */ + it('produces every code the public execution-error contract publishes', () => { + const producedBy: Record = { + TIMEOUT: new Error('Execution timed out after 5 minutes'), + CANCELLED: new Error('Run was cancelled'), + USAGE_LIMIT_EXCEEDED: new Error('Usage limit exceeded for this billing period'), + INVALID_INPUT: new Error('Invalid input format for the workflow'), + BLOCK_EXECUTION_FAILED: buildBlockExecutionError({ + block: { id: 'block-1', metadata: { name: 'Send Email', id: 'gmail' } } as never, + error: new Error('Invalid credentials'), + }), + CHILD_WORKFLOW_FAILED: buildBlockExecutionError({ + block: { id: 'block-2', metadata: { name: 'Child', id: 'workflow' } } as never, + error: new Error('Child run failed'), + }), + EXECUTION_FAILED: new Error('something odd'), + } + + for (const [code, error] of Object.entries(producedBy)) { + expect(classifyExecutionError(error).code).toBe(code) + } + expect(new Set(v2ExecutionErrorSchema.shape.code.options)).toEqual( + new Set(Object.keys(producedBy)) + ) + }) }) diff --git a/apps/sim/executor/utils/errors.ts b/apps/sim/executor/utils/errors.ts index 90bd6a11b1b..7bbef22da0e 100644 --- a/apps/sim/executor/utils/errors.ts +++ b/apps/sim/executor/utils/errors.ts @@ -138,6 +138,16 @@ export function normalizeError(error: unknown): string { * substring-matching messages; this module is the single place raw errors are * interpreted, so the executor can later attach codes natively at throw sites * without a wire change. + * + * Append-only binds what a caller can observe, and an oversize run response has + * never been observable here: it is not an in-band failure at all. The execution + * service short-circuits it into an HTTP 413 carrying `workflow_response_too_large` + * in the error envelope, so no run ever reaches classification with an oversize + * output. `OUTPUT_TOO_LARGE` was therefore a code no caller could ever have + * routed on — publishing it only invited a branch that never runs, and an + * exhaustive `switch` in a generated SDK to claim coverage it does not have. + * Retiring an unreachable member narrows the published type without changing a + * single response; adding a reachable member later is still the append-only path. */ export type WorkflowExecutionErrorCode = | 'TIMEOUT' @@ -146,7 +156,6 @@ export type WorkflowExecutionErrorCode = | 'INVALID_INPUT' | 'BLOCK_EXECUTION_FAILED' | 'CHILD_WORKFLOW_FAILED' - | 'OUTPUT_TOO_LARGE' | 'EXECUTION_FAILED' export interface StructuredExecutionError { diff --git a/apps/sim/lib/api/contracts/v2/__tests__/tables.test.ts b/apps/sim/lib/api/contracts/v2/__tests__/tables.test.ts index 0022839b939..481cf479b6e 100644 --- a/apps/sim/lib/api/contracts/v2/__tests__/tables.test.ts +++ b/apps/sim/lib/api/contracts/v2/__tests__/tables.test.ts @@ -11,6 +11,7 @@ import { V2_SEARCH_MAX_LENGTH } from '@/lib/api/contracts/v2/shared' import * as tableContracts from '@/lib/api/contracts/v2/tables' import { V2_TABLE_IMPORT_OPTIONS_MAX_BYTES, + v2AddWorkflowGroupBodySchema, v2ApiTableSchema, v2CreateTableBodySchema, v2CreateTableColumnBodySchema, @@ -25,6 +26,7 @@ import { v2TableImportStatusSchema, v2TableUploadImportSourceSchema, v2UpdateTableColumnBodySchema, + v2UpdateWorkflowGroupBodySchema, } from '@/lib/api/contracts/v2/tables' import { getValidationErrorMessage } from '@/lib/api/server/validation' import { MAX_RUN_TARGET_ROW_IDS, TABLE_LIMITS } from '@/lib/table/constants' @@ -80,6 +82,51 @@ describe('v2 table column contracts', () => { expect(issueCodes(result.error?.issues ?? [])).toContain('unrecognized_keys') }) + /** + * Same field, same reason, one level down: the group body's `.strict()` binds + * its own level, so an `outputColumns` entry that kept `workflowGroupId` was + * stripped, overwritten with the server-minted id, and answered 201. + */ + it('refuses a workflow group id on a group output column', () => { + const result = v2AddWorkflowGroupBodySchema.safeParse({ + workspaceId: WORKSPACE_ID, + group: { + type: 'enrichment', + enrichmentId: 'company-domain', + outputs: [{ blockId: '', path: 'domain', columnName: 'zz_y' }], + }, + outputColumns: [{ name: 'zz_y', type: 'string', workflowGroupId: 'wfg_does_not_exist' }], + }) + + expect(result.success).toBe(false) + expect(issueCodes(result.error?.issues ?? [])).toContain('unrecognized_keys') + }) + + it('refuses a workflow group id on an added output column', () => { + const result = v2UpdateWorkflowGroupBodySchema.safeParse({ + workspaceId: WORKSPACE_ID, + groupId: 'group-1', + newOutputColumns: [{ name: 'zz_z', type: 'string', workflowGroupId: 'wfg_does_not_exist' }], + }) + + expect(result.success).toBe(false) + expect(issueCodes(result.error?.issues ?? [])).toContain('unrecognized_keys') + }) + + it('accepts a group output column that leaves the group id to the server', () => { + expect( + v2AddWorkflowGroupBodySchema.safeParse({ + workspaceId: WORKSPACE_ID, + group: { + type: 'enrichment', + enrichmentId: 'company-domain', + outputs: [{ blockId: '', path: 'domain', columnName: 'zz_y' }], + }, + outputColumns: [{ name: 'zz_y', type: 'string' }], + }).success + ).toBe(true) + }) + it('keeps required in table responses for existing stored schemas', () => { expect( v2ApiTableSchema.safeParse({ diff --git a/apps/sim/lib/api/contracts/v2/knowledge-search-bounds.test.ts b/apps/sim/lib/api/contracts/v2/knowledge-search-bounds.test.ts new file mode 100644 index 00000000000..ea79ec8c3d3 --- /dev/null +++ b/apps/sim/lib/api/contracts/v2/knowledge-search-bounds.test.ts @@ -0,0 +1,106 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { + MAX_V2_KNOWLEDGE_DOCUMENT_TAG_FILTERS, + MAX_V2_KNOWLEDGE_SEARCH_QUERY_LENGTH, + parseV2KnowledgeTagFiltersParam, + v2KnowledgeSearchBodySchema, +} from '@/lib/api/contracts/v2/knowledge' + +const workspaceId = '7a6cce2b-78b8-40bc-b8d3-0a2a6dfd9023' +const knowledgeBaseIds = ['ae814592-a730-4ad6-8741-b51e48843300'] + +function tagFilters(count: number) { + return Array.from({ length: count }, (_, index) => ({ + tagName: 'Messages in Thread', + operator: 'gte' as const, + value: String(index), + })) +} + +function issueMessages(result: ReturnType) { + return result.success ? [] : result.error.issues.map((issue) => issue.message) +} + +/** + * Search is billed per call, so an input it cannot honour must be rejected at the + * boundary rather than accepted and quietly reshaped. + */ +describe('v2 knowledge search body bounds', () => { + it('accepts a query at the maximum length', () => { + const result = v2KnowledgeSearchBodySchema.safeParse({ + workspaceId, + knowledgeBaseIds, + query: 'a'.repeat(MAX_V2_KNOWLEDGE_SEARCH_QUERY_LENGTH), + }) + expect(result.success).toBe(true) + }) + + it('rejects a query one character past the maximum, naming the limit', () => { + const result = v2KnowledgeSearchBodySchema.safeParse({ + workspaceId, + knowledgeBaseIds, + query: 'a'.repeat(MAX_V2_KNOWLEDGE_SEARCH_QUERY_LENGTH + 1), + }) + expect(result.success).toBe(false) + expect(issueMessages(result)).toContain( + `query cannot exceed ${MAX_V2_KNOWLEDGE_SEARCH_QUERY_LENGTH} characters` + ) + }) + + it('bounds the query by the embedding model per-input token ceiling', () => { + expect(MAX_V2_KNOWLEDGE_SEARCH_QUERY_LENGTH).toBe(8192 * 4) + }) + + it('accepts tag filters at the cap', () => { + const result = v2KnowledgeSearchBodySchema.safeParse({ + workspaceId, + knowledgeBaseIds, + tagFilters: tagFilters(MAX_V2_KNOWLEDGE_DOCUMENT_TAG_FILTERS), + }) + expect(result.success).toBe(true) + }) + + /** + * The document list has always rejected an eleventh filter. Search shares the + * tag vocabulary, so it must share the policy — the same request cannot be a + * 400 on one surface and an applied 200 on the other. + */ + it('rejects one tag filter past the cap with the same message the document list uses', () => { + const overLimit = tagFilters(MAX_V2_KNOWLEDGE_DOCUMENT_TAG_FILTERS + 1) + const expected = `tagFilters cannot contain more than ${MAX_V2_KNOWLEDGE_DOCUMENT_TAG_FILTERS} filters` + + const searchResult = v2KnowledgeSearchBodySchema.safeParse({ + workspaceId, + knowledgeBaseIds, + tagFilters: overLimit, + }) + expect(searchResult.success).toBe(false) + expect(issueMessages(searchResult)).toContain(expected) + + const listResult = parseV2KnowledgeTagFiltersParam(JSON.stringify(overLimit)) + expect(listResult.success).toBe(false) + expect(listResult.success === false && listResult.message).toContain(expected) + }) + + it('still accepts an ordinary single filter of each field type', () => { + const result = v2KnowledgeSearchBodySchema.safeParse({ + workspaceId, + knowledgeBaseIds, + query: 'How do I reset my password?', + tagFilters: [ + { tagName: 'From', operator: 'contains', value: 'brex' }, + { tagName: 'Messages in Thread', operator: 'gte', value: 9 }, + { + tagName: 'Last Message', + operator: 'between', + value: '2026-01-01', + valueTo: '2026-12-31', + }, + ], + }) + expect(result.success).toBe(true) + }) +}) diff --git a/apps/sim/lib/api/contracts/v2/knowledge.ts b/apps/sim/lib/api/contracts/v2/knowledge.ts index 44a7c723f1a..52cdf284756 100644 --- a/apps/sim/lib/api/contracts/v2/knowledge.ts +++ b/apps/sim/lib/api/contracts/v2/knowledge.ts @@ -821,6 +821,21 @@ export const v2KnowledgeSearchTagFilterSchema = v1SearchTagFilterSchema description: 'A structured tag filter applied to knowledge search.', }) +/** Maximum tag filters accepted on one document-list or search request. */ +export const MAX_V2_KNOWLEDGE_DOCUMENT_TAG_FILTERS = 10 + +/** + * Maximum `query` length accepted by knowledge search. + * + * Every knowledge-base-eligible embedding model caps a single input at 8192 + * tokens, and the embedding client silently truncates anything longer, so a + * caller paid for a billed search whose query was mostly discarded. The bound is + * that ceiling expressed in characters using the four-characters-per-token + * conversion the tokenizer's own fallback uses, which is generous enough that + * nothing that could have been embedded whole is rejected. + */ +export const MAX_V2_KNOWLEDGE_SEARCH_QUERY_LENGTH = 8192 * 4 + export const v2KnowledgeSearchBodySchema = v1KnowledgeSearchBodySchema .safeExtend({ workspaceId: v1KnowledgeSearchBodySchema.shape.workspaceId.describe( @@ -829,17 +844,29 @@ export const v2KnowledgeSearchBodySchema = v1KnowledgeSearchBodySchema knowledgeBaseIds: v1KnowledgeSearchBodySchema.shape.knowledgeBaseIds .describe('One knowledge base identifier or an array of up to 20 identifiers.') .meta({ examples: [['7c9e6679-7425-40de-944b-e07fc1f90ae7']] }), - query: v1KnowledgeSearchBodySchema.shape.query - .describe('Natural-language query; required when tag filters are omitted.') + query: z + .string() + .max( + MAX_V2_KNOWLEDGE_SEARCH_QUERY_LENGTH, + `query cannot exceed ${MAX_V2_KNOWLEDGE_SEARCH_QUERY_LENGTH} characters` + ) + .optional() + .describe( + `Natural-language query; required when tag filters are omitted. At most ${MAX_V2_KNOWLEDGE_SEARCH_QUERY_LENGTH} characters — longer text exceeds the embedding model's per-input token ceiling and would be truncated before the billed search ran.` + ) .meta({ examples: ['How do I reset my password?'] }), topK: v1KnowledgeSearchBodySchema.shape.topK.describe( 'Maximum number of search results to return. Must be a whole number between 1 and 100; the boundary schema only bounds the range, so a fractional value is admitted here and then rejected with 400 during search.' ), tagFilters: z .array(v2KnowledgeSearchTagFilterSchema) + .max( + MAX_V2_KNOWLEDGE_DOCUMENT_TAG_FILTERS, + `tagFilters cannot contain more than ${MAX_V2_KNOWLEDGE_DOCUMENT_TAG_FILTERS} filters` + ) .optional() .describe( - 'Structured tag filters. Each filtered tag must resolve to the same slot and field type in every knowledge base selected; one missing from any of them, or defined inconsistently across them, is rejected rather than ignored, and those knowledge bases must be searched separately. List the available names with `GET /api/v2/knowledge/{id}/tags`.' + `Structured tag filters, at most ${MAX_V2_KNOWLEDGE_DOCUMENT_TAG_FILTERS} of them. Every filter must hold, including two that name the same tag: repeating one tag narrows the result rather than widening it, matching \`GET /api/v2/knowledge/{id}/documents\`. To match either of two values for one tag, issue a search per value. Each filtered tag must resolve to the same slot and field type in every knowledge base selected; one missing from any of them, or defined inconsistently across them, is rejected rather than ignored, and those knowledge bases must be searched separately. List the available names with \`GET /api/v2/knowledge/{id}/tags\`.` ), searchMode: v1KnowledgeSearchBodySchema.shape.searchMode.describe( 'Retrieval strategy: vector is semantic-only, while hybrid also runs full-text search.' @@ -895,9 +922,6 @@ export const v2SearchKnowledgeContract = defineRouteContract({ }, }) -/** Maximum tag filters accepted on one document-list request. */ -export const MAX_V2_KNOWLEDGE_DOCUMENT_TAG_FILTERS = 10 - const v2KnowledgeDocumentTagFiltersSchema = z .array(v2KnowledgeSearchTagFilterSchema) .max( @@ -974,7 +998,7 @@ export const v2ListKnowledgeDocumentsQuerySchema = v1ListKnowledgeDocumentsQuery .string() .optional() .describe( - `A JSON-encoded array of at most ${MAX_V2_KNOWLEDGE_DOCUMENT_TAG_FILTERS} tag filters, using the same display-name shape as knowledge search: \`[{"tagName":"category","operator":"eq","value":"billing"}]\`. A name that is not defined in this knowledge base is rejected, never ignored.` + `A JSON-encoded array of at most ${MAX_V2_KNOWLEDGE_DOCUMENT_TAG_FILTERS} tag filters, using the same display-name shape as knowledge search: \`[{"tagName":"category","operator":"eq","value":"billing"}]\`. Every filter must hold, including two that name the same tag. A name that is not defined in this knowledge base is rejected, never ignored.` ) .meta({ examples: ['[{"tagName":"category","operator":"eq","value":"billing"}]'] }), }) diff --git a/apps/sim/lib/api/contracts/v2/logs.ts b/apps/sim/lib/api/contracts/v2/logs.ts index 19ca8d358ea..669969b01d4 100644 --- a/apps/sim/lib/api/contracts/v2/logs.ts +++ b/apps/sim/lib/api/contracts/v2/logs.ts @@ -294,9 +294,27 @@ export const v2ListLogsQuerySchema = v1ListLogsQuerySchema 'workflowIds', 'Comma-separated workflow identifiers to include. An empty entry is rejected.' ).optional(), + /** + * Not a closed enum, which is why an unrecognized member is not a 400. + * `workflow_execution_logs.trigger` holds the core trigger types *and* the + * webhook provider id a run arrived on — `executeWebhookJobInternal` passes + * `payload.provider` straight through as the trigger — so the live + * vocabulary is the union of the core set and every webhook provider that + * has ever fired, including spellings retired since (`microsoft-teams` + * alongside `microsoftteams`). Pinning an enum here would reject the + * historical values a diagnostic search exists to find, so the filter states + * that an unmatched member simply selects nothing rather than pretending to + * validate one. + * + * Matching is exact and case-sensitive because the column is: every value + * ever written is lowercase, so `API` and `ALL` name nothing. They are + * caller mistakes, but the boundary cannot tell them apart from an unknown + * provider id, and normalizing case here would silently repair one class of + * typo while leaving the rest — so the case rule is documented instead. + */ triggers: v2CommaListSchema( 'triggers', - 'Comma-separated trigger types to include. An empty entry is rejected. The literal value `all` is a sentinel that disables this filter entirely, so a list containing it returns runs of every trigger type; no real trigger type is named `all`.' + 'Comma-separated trigger types to include. An empty entry is rejected. Values are matched exactly and are case-sensitive — every recorded trigger is lowercase, so `API` matches nothing while `api` matches. The vocabulary is open: it covers the core trigger types (`manual`, `api`, `schedule`, `chat`, `webhook`, `mcp`, `copilot`, `workflow`, `custom_block`) and the provider id of any webhook trigger (`slack`, `gmail`, `github`, …), so an unrecognized member is not rejected — it selects no runs. The literal value `all` is a sentinel that disables this filter entirely, so a list containing it returns runs of every trigger type; no real trigger type is named `all`.' ).optional(), level: z.enum(['info', 'error']).describe('Severity level to include.').optional(), startDate: v2RunWindowBoundSchema('startDate').optional(), diff --git a/apps/sim/lib/api/contracts/v2/mcp-servers.ts b/apps/sim/lib/api/contracts/v2/mcp-servers.ts index 2ce5c3369d0..ce89b5e1bf1 100644 --- a/apps/sim/lib/api/contracts/v2/mcp-servers.ts +++ b/apps/sim/lib/api/contracts/v2/mcp-servers.ts @@ -117,12 +117,13 @@ export const v2McpServerSchema = z 'Whether the server tools are available to workflows.' ), /** - * These three are written only by a real discovery. Registration stores a - * configuration without contacting the endpoint, so it leaves all three at - * their defaults rather than asserting a connection nothing has verified. + * These three are written only by a real discovery. Registration's one + * outbound touch is the auth-type probe, which classifies the endpoint and + * never records a connection, so registration leaves all three at their + * defaults rather than asserting a connection nothing has verified. */ connectionStatus: mcpServerSchema.shape.connectionStatus.describe( - 'Result of the most recent connection attempt. Registration and re-registration store a configuration without contacting the endpoint, so a server begins — and returns to — `disconnected` until a tool discovery runs.' + 'Result of the most recent connection attempt. Registration and re-registration establish no connection — the auth-type probe they may send does not count as one — so a server begins, and returns to, `disconnected` until a tool discovery runs.' ), lastError: mcpServerSchema.shape.lastError.describe( 'Message from the most recent failed connection, or null when absent. A re-registration clears it, since the configuration it described no longer applies.' @@ -221,12 +222,18 @@ export const v2CreateMcpServerBodySchema = z ) .meta({ default: 'streamable-http' }), url: v2McpServerUrlSchema, + /** + * No published `default`: when the field is omitted the stored value is + * detected, not defaulted, so a `default` here would be wrong on create and + * — inherited by the `.partial()` update body — would make an SDK that + * materializes JSON-Schema defaults revoke a stored OAuth grant on every + * unrelated PATCH. + */ authType: mcpAuthTypeSchema .optional() .describe( - 'Authentication method. Applied server-side as `headers` when omitted; registration never contacts the server, so an omitted value is never detected from it.' - ) - .meta({ default: 'headers' }), + 'Authentication method. When omitted, and no `headers` are sent, registration probes the endpoint once to classify it, falling back to `headers` when the probe fails or the server does not advertise OAuth. A server publishing RFC 9728 metadata is therefore stored as `oauth`, and headers configured afterwards will not authenticate — send this field explicitly to pin the method.' + ), /** Write-only. Reads expose `hasHeaders` and `headerNames` instead. */ headers: v2McpServerHeadersSchema .optional() diff --git a/apps/sim/lib/api/contracts/v2/tables.ts b/apps/sim/lib/api/contracts/v2/tables.ts index 38ff366b4b4..3708be6662f 100644 --- a/apps/sim/lib/api/contracts/v2/tables.ts +++ b/apps/sim/lib/api/contracts/v2/tables.ts @@ -1274,10 +1274,19 @@ export const v2ListWorkflowGroupsContract = defineRouteContract({ * shape carries `workflowGroupId` because the client mints the group id before * posting; v2 server-generates it, so the field is stamped from the group being * written rather than being a caller's to supply (and get wrong). + * + * `.strict()` is what makes that omission observable. `omit()` yields an + * ordinary object schema, and the enclosing body's `.strict()` binds its own + * level only — so a caller who kept the first-party `workflowGroupId` had it + * stripped, silently overwritten with the server-minted id, and answered 201. + * The same key is now a 400 naming the field, matching how `POST /v2/tables` + * refuses it on initial columns. */ -const v2WorkflowGroupOutputColumnSchema = workflowGroupOutputColumnSchema.omit({ - workflowGroupId: true, -}) +const v2WorkflowGroupOutputColumnSchema = workflowGroupOutputColumnSchema + .omit({ + workflowGroupId: true, + }) + .strict() /** * A group names its producer two mutually exclusive ways, and the underlying diff --git a/apps/sim/lib/api/contracts/v2/workflows.ts b/apps/sim/lib/api/contracts/v2/workflows.ts index eb075e9ddc7..8d0aa30a245 100644 --- a/apps/sim/lib/api/contracts/v2/workflows.ts +++ b/apps/sim/lib/api/contracts/v2/workflows.ts @@ -787,6 +787,15 @@ export const v2RollbackWorkflowContract = defineRouteContract({ * `@/executor/utils/errors` (duplicated literally: contracts are * client-importable and must not pull executor modules). APPEND-ONLY: callers * route on these instead of substring-matching messages. + * + * `OUTPUT_TOO_LARGE` was retired rather than kept for the append-only promise: + * an oversize run response is not an in-band failure and never reached + * classification, so the code was never emitted on any response. See + * `WorkflowExecutionErrorCode` for the full reasoning; the oversize case is an + * HTTP 413 carrying `workflow_response_too_large` in the error envelope. + * + * Block attribution is NOT uniform across the operations that carry this + * object — see `blockId` below. */ export const v2ExecutionErrorSchema = z .object({ @@ -799,14 +808,41 @@ export const v2ExecutionErrorSchema = z 'INVALID_INPUT', 'BLOCK_EXECUTION_FAILED', 'CHILD_WORKFLOW_FAILED', - 'OUTPUT_TOO_LARGE', 'EXECUTION_FAILED', ]) - .describe('Stable machine-readable execution failure code.'), - /** Failing block, when attributable. Deliberately crosses the workspace boundary for shared/child workflows — the runId + block context is the reproducible handle a caller hands the workflow provider. */ - blockId: z.string().optional().describe('Identifier of the failing block, when attributable.'), - blockName: z.string().optional().describe('Display name of the failing block.'), - blockType: z.string().optional().describe('Integration or block type that failed.'), + .describe( + 'Stable machine-readable execution failure code. `BLOCK_EXECUTION_FAILED` and `CHILD_WORKFLOW_FAILED` are reported only where block attribution is available; elsewhere a block-level failure is reported as `EXECUTION_FAILED`.' + ), + /** + * Failing block, when attributable. Deliberately crosses the workspace boundary for shared/child workflows — the runId + block context is the reproducible handle a caller hands the workflow provider. + * + * Attribution is produced at execution time from the failing block's throw + * site, so it reaches the caller only on the synchronous execute response. + * The polled run resource and the resume response reclassify a persisted + * error *string* — the log row keeps no structured error — so these three + * fields are absent there and the code collapses to `EXECUTION_FAILED`. + * That is a known asymmetry, documented rather than silently promised: + * a caller that needs the failing block reads the run's trace spans, which + * do carry `blockId`, `name`, and `type`. + */ + blockId: z + .string() + .optional() + .describe( + 'Identifier of the failing block. Present on the synchronous execute response only; the polled run resource and the resume response cannot attribute a block.' + ), + blockName: z + .string() + .optional() + .describe( + 'Display name of the failing block. Present on the synchronous execute response only.' + ), + blockType: z + .string() + .optional() + .describe( + 'Integration or block type that failed. Present on the synchronous execute response only.' + ), }) .meta({ id: 'ExecutionError', @@ -1191,19 +1227,36 @@ export const v2ListWorkflowRunsContract = defineRouteContract({ /** * The polled run resource. `queued` is backfilled from the async job * queue before the worker writes the durable log row — v1's jobs endpoint 404 - * window doesn't exist here. `error` is the same structured object the execute - * response carries. + * window doesn't exist here. `error` is the same *shape* the execute response + * carries, but not the same content: this resource reclassifies the persisted + * error string, so it never attributes a block. See `v2ExecutionErrorSchema`. */ export const v2WorkflowRunStatusSchema = z .object({ runId: v2WorkflowRunIdSchema, workflowId: z.string().describe('Workflow that produced the run.'), status: v2WorkflowRunStatusValueSchema, - trigger: z.string().nullable().describe('Trigger type, or null before the run is recorded.'), + /** + * Kept nullable on the wire while never being null in practice: every + * projection this resource has — the queued job, the queued resume, and the + * durable log row — backfills both fields (`api` and the job's creation time + * when the run is not yet recorded), so no caller has observed a null here. + * The nullability is the schema's tolerance for a future projection, not a + * state a caller needs to branch on, which is why neither description + * promises a null that never arrives. + */ + trigger: z + .string() + .nullable() + .describe( + 'Trigger type that started the run. Backfilled as `api` for a run that is still queued, so it is populated from the first poll.' + ), startedAt: z .string() .nullable() - .describe('ISO 8601 start timestamp, or null while queued.') + .describe( + 'ISO 8601 start timestamp. A queued run reports the time it was enqueued, so it is populated from the first poll.' + ) .meta({ format: 'date-time' }), endedAt: z .string() @@ -1224,7 +1277,9 @@ export const v2WorkflowRunStatusSchema = z .describe('Credit cost, or null when unavailable.'), error: v2ExecutionErrorSchema .nullable() - .describe('Structured execution failure, or null when none occurred.'), + .describe( + 'Structured execution failure, or null when none occurred. Reclassified from the persisted error message, so `blockId`/`blockName`/`blockType` are absent and a block-level failure reports `EXECUTION_FAILED` here even when the same run reported `BLOCK_EXECUTION_FAILED` on its synchronous execute response.' + ), /** Populated only with `includeOutput=true` on completed runs. */ output: z .unknown() @@ -1234,7 +1289,9 @@ export const v2WorkflowRunStatusSchema = z blockOutputs: z .record(z.string(), z.unknown().describe('Output value produced by one workflow block.')) .nullable() - .describe('Selected block outputs when requested, otherwise null.'), + .describe( + 'Outputs of the blocks named by `selectedOutputs`, or null when none were requested. Gated by `selectedOutputs` alone — `includeOutput` governs `output` only.' + ), }) .meta({ id: 'WorkflowRunStatus', @@ -1257,11 +1314,19 @@ export const v2GetWorkflowRunContract = defineRouteContract({ * working identically; it only widens what parses. */ includeOutput: booleanQueryFlagSchema - .describe('Include final and block outputs when true.') + .describe( + 'Include the final workflow output when true. It does not gate `blockOutputs`, which `selectedOutputs` selects on its own.' + ) .optional() .default(false), + /** + * Block *ids*, unlike the execute request's `selectedOutputs`, which also + * accepts `BlockName.path` and resolves it against the live workflow. This + * resource reads a recorded run and never loads the workflow's blocks, so + * a name has no id to resolve to and selects nothing. + */ selectedOutputs: workflowExecutionStatusQuerySchema.shape.selectedOutputs.describe( - 'Comma-separated block output references to include.' + 'Comma-separated block output references to include, as `blockId` or `blockId.path`. Block *names* are not resolved here — unlike the execute request, this resource reads a recorded run and matches ids only, so a name selects nothing and yields an empty `blockOutputs`.' ), }) .strict() diff --git a/apps/sim/lib/api/cursor-binding.test.ts b/apps/sim/lib/api/cursor-binding.test.ts index 84caf533a45..2afdb65ccf7 100644 --- a/apps/sim/lib/api/cursor-binding.test.ts +++ b/apps/sim/lib/api/cursor-binding.test.ts @@ -1,9 +1,12 @@ /** * @vitest-environment node */ +import { globSync, readFileSync } from 'node:fs' +import { join } from 'node:path' import { describe, expect, it } from 'vitest' import { parseV2KnowledgeTagFiltersParam } from '@/lib/api/contracts/v2/knowledge' import { + cursorRoute, cursorScopeKey, instantScopePart, parseUnorderedList, @@ -14,6 +17,7 @@ import { cursorSortKey, decodeOffsetCursor, decodeSortedCursor, + encodeCursor, encodeOffsetCursor, encodeScopedCursor, encodeSortedCursor, @@ -33,11 +37,18 @@ import { * `GET /skills` and `GET /knowledge/{id}/documents`, the keyset used by the nine * SQL-ordered lists, and the wrapper that binds the domain-minted tokens on * `GET /logs`, `GET /audit-logs`, and `GET /billing/logs` — to that one rule. + * + * The scope also carries the list's own identity, because two lists that filter + * identically are still two sequences — see the `list identity` block. */ + +/** Stand-in for one v2 list route, as its contract declares it. */ +const LIST = cursorRoute({ method: 'GET', path: '/api/v2/files' }) + describe('v2 cursor binding', () => { const sort = cursorSortKey('name', 'asc') const filters = { workspaceId: 'ws-1', search: undefined as string | undefined } - const scope = cursorScopeKey(filters) + const scope = cursorScopeKey(LIST, filters) describe('offset cursor', () => { it('resumes a cursor replayed under the same query state', () => { @@ -59,10 +70,10 @@ describe('v2 cursor binding', () => { const cursor = encodeOffsetCursor(sort, scope, 40) expect(() => - decodeOffsetCursor(cursor, sort, cursorScopeKey({ ...filters, search: 'deploy' })) + decodeOffsetCursor(cursor, sort, cursorScopeKey(LIST, { ...filters, search: 'deploy' })) ).toThrow(/requested filters/) expect(() => - decodeOffsetCursor(cursor, sort, cursorScopeKey({ ...filters, workspaceId: 'ws-2' })) + decodeOffsetCursor(cursor, sort, cursorScopeKey(LIST, { ...filters, workspaceId: 'ws-2' })) ).toThrow(/requested filters/) }) @@ -100,7 +111,7 @@ describe('v2 cursor binding', () => { */ it('rejects a cursor replayed under a different filter', () => { const cursor = encodeSortedCursor(sort, keys, scope) - const narrowed = cursorScopeKey({ ...filters, search: 'deploy' }) + const narrowed = cursorScopeKey(LIST, { ...filters, search: 'deploy' }) expect(decodeSortedCursor(cursor, sort, narrowed)).toEqual({ status: 'refiltered' }) expect(() => readSortedCursor(cursor, 'name', 'asc', narrowed)).toThrow(/requested filters/) @@ -117,13 +128,10 @@ describe('v2 cursor binding', () => { ).toThrow(/sortBy\/sortOrder/) }) - it('refuses an unfiltered cursor replayed under a filter, and the reverse', () => { - const unfiltered = encodeSortedCursor(sort, keys, undefined) + it('refuses a cursor minted with no scope stamp at all', () => { + const unstamped = encodeCursor({ sort, keys }) - expect(() => readSortedCursor(unfiltered, 'name', 'asc', scope)).toThrow(/requested filters/) - expect(() => - readSortedCursor(encodeSortedCursor(sort, keys, scope), 'name', 'asc', undefined) - ).toThrow(/requested filters/) + expect(() => readSortedCursor(unstamped, 'name', 'asc', scope)).toThrow(/requested filters/) }) it('treats an absent cursor as page one', () => { @@ -142,7 +150,7 @@ describe('v2 cursor binding', () => { const cursor = encodeScopedCursor(scope, 'domain-token') expect(() => - readScopedCursor(cursor, cursorScopeKey({ ...filters, search: 'deploy' })) + readScopedCursor(cursor, cursorScopeKey(LIST, { ...filters, search: 'deploy' })) ).toThrow(/requested filters/) }) @@ -173,15 +181,20 @@ describe('v2 cursor binding', () => { }) it('does not depend on the order the parts are written', () => { - expect(cursorScopeKey({ b: '2', a: '1' })).toBe(cursorScopeKey({ a: '1', b: '2' })) + expect(cursorScopeKey(LIST, { b: '2', a: '1' })).toBe( + cursorScopeKey(LIST, { a: '1', b: '2' }) + ) }) it('treats an omitted part and an undefined part as the same scope', () => { - expect(cursorScopeKey({ a: '1', b: undefined })).toBe(cursorScopeKey({ a: '1' })) + expect(cursorScopeKey(LIST, { a: '1', b: undefined })).toBe(cursorScopeKey(LIST, { a: '1' })) }) - it('has no fingerprint at all when nothing is filtered', () => { - expect(cursorScopeKey({ a: undefined })).toBeUndefined() + it('still fingerprints the list when nothing is filtered', () => { + const bare = cursorScopeKey(LIST, { a: undefined }) + + expect(bare).toHaveLength(22) + expect(bare).toBe(cursorScopeKey(LIST)) }) /** @@ -189,27 +202,119 @@ describe('v2 cursor binding', () => { * and `{a:'1|2'}` are different reads and must fingerprint differently. */ it('separates parts rather than concatenating their values', () => { - expect(cursorScopeKey({ a: '1', b: '2' })).not.toBe(cursorScopeKey({ a: '1|2' })) - expect(cursorScopeKey({ a: '1' })).not.toBe(cursorScopeKey({ b: '1' })) + expect(cursorScopeKey(LIST, { a: '1', b: '2' })).not.toBe(cursorScopeKey(LIST, { a: '1|2' })) + expect(cursorScopeKey(LIST, { a: '1' })).not.toBe(cursorScopeKey(LIST, { b: '1' })) }) it('stays short enough to sit inside an opaque token', () => { - expect(cursorScopeKey({ search: 'x'.repeat(200) })).toHaveLength(22) + expect(cursorScopeKey(LIST, { search: 'x'.repeat(200) })).toHaveLength(22) + }) + }) + + /** + * The whole point of the scope: two lists that filter identically are still + * two sequences. Every v2 workspace list shares a `workspaceId`-only filter + * set and the same `{sort,keys,filter}` payload shape, so a fingerprint over + * filters alone made each of them accept the others' tokens and answer 200 + * with a page that silently skipped rows. + */ + describe('list identity', () => { + const workspace = { workspaceId: 'ws-1' } + + it('separates two lists that filter identically', () => { + const tables = cursorRoute({ method: 'GET', path: '/api/v2/tables' }) + const knowledge = cursorRoute({ method: 'GET', path: '/api/v2/knowledge' }) + const credentials = cursorRoute({ method: 'GET', path: '/api/v2/credentials' }) + + const keys = new Set( + [tables, knowledge, credentials].map((route) => cursorScopeKey(route, workspace)) + ) + + expect(keys.size).toBe(3) + }) + + it('refuses a cursor minted by another list', () => { + const tablesScope = cursorScopeKey(cursorRoute({ method: 'GET', path: '/api/v2/tables' }), { + workspaceId: 'ws-1', + }) + const knowledgeScope = cursorScopeKey( + cursorRoute({ method: 'GET', path: '/api/v2/knowledge' }), + { workspaceId: 'ws-1' } + ) + const fromTables = encodeSortedCursor( + sort, + ['2026-03-17T10:09:31.755Z', 'tbl_1'], + tablesScope + ) + + expect(() => readSortedCursor(fromTables, 'name', 'asc', knowledgeScope)).toThrow( + /requested filters/ + ) + expect(readSortedCursor(fromTables, 'name', 'asc', tablesScope)).toEqual([ + '2026-03-17T10:09:31.755Z', + 'tbl_1', + ]) + }) + + it('separates two parents of the same nested list', () => { + const rowsOf = (tableId: string) => + cursorScopeKey( + cursorRoute({ method: 'GET', path: '/api/v2/tables/[tableId]/rows' }, { tableId }) + ) + + const fromX = encodeScopedCursor(rowsOf('tbl_x'), 'domain-token') + + expect(() => readScopedCursor(fromX, rowsOf('tbl_y'))).toThrow(/requested filters/) + expect(readScopedCursor(fromX, rowsOf('tbl_x'))).toBe('domain-token') + }) + + /** + * A template binds every parent to one scope, which is the defect this + * exists to prevent — so an unresolved placeholder is a hard failure rather + * than a fingerprint of the literal `[tableId]`. + */ + it('refuses to fingerprint an unresolved path param', () => { + expect(() => + cursorScopeKey(cursorRoute({ method: 'GET', path: '/api/v2/tables/[tableId]/rows' })) + ).toThrow(/tableId/) + }) + }) + + /** + * A token that does not decode says nothing about the sort, and the sort + * message tells a caller who changed nothing to go re-read the sort docs. + */ + describe('undecodable cursor', () => { + it('reports that the token is unreadable, not that the sort changed', () => { + expect(() => readSortedCursor('GARBAGE', 'name', 'asc', scope)).toThrow( + /not a valid pagination cursor/ + ) + expect(() => readSortedCursor('GARBAGE', 'name', 'asc', scope)).not.toThrow(/sortBy/) + expect(() => decodeOffsetCursor('GARBAGE', sort, scope)).toThrow( + /not a valid pagination cursor/ + ) + expect(() => decodeOffsetCursor('GARBAGE', sort, scope)).not.toThrow(/sortBy/) + }) + + it('still names the sort when the token decodes and the sort is what changed', () => { + expect(() => + readSortedCursor(encodeSortedCursor(sort, ['a', 'b'], scope), 'createdAt', 'asc', scope) + ).toThrow(/sortBy\/sortOrder/) }) }) }) describe('unordered filter scope parts', () => { it('fingerprints a reordered set identically', () => { - const a = cursorScopeKey({ workflowIds: unorderedScopePart('A,B') }) - const b = cursorScopeKey({ workflowIds: unorderedScopePart('B,A') }) + const a = cursorScopeKey(LIST, { workflowIds: unorderedScopePart('A,B') }) + const b = cursorScopeKey(LIST, { workflowIds: unorderedScopePart('B,A') }) expect(a).toBe(b) }) it('fingerprints a duplicate-bearing set identically', () => { // The filters compile to `inArray`, which is set membership, so A,A,B // selects exactly what A,B does and must resume the same page. - expect(cursorScopeKey({ workflowIds: unorderedScopePart('A,A,B') })).toBe( - cursorScopeKey({ workflowIds: unorderedScopePart('A,B') }) + expect(cursorScopeKey(LIST, { workflowIds: unorderedScopePart('A,A,B') })).toBe( + cursorScopeKey(LIST, { workflowIds: unorderedScopePart('A,B') }) ) expect(unorderedScopePart('B,A,B')).toBe('A,B') }) @@ -292,8 +397,8 @@ describe('unordered filter scope parts', () => { }) it('still separates genuinely different sets', () => { - expect(cursorScopeKey({ workflowIds: unorderedScopePart('A,B') })).not.toBe( - cursorScopeKey({ workflowIds: unorderedScopePart('A,C') }) + expect(cursorScopeKey(LIST, { workflowIds: unorderedScopePart('A,B') })).not.toBe( + cursorScopeKey(LIST, { workflowIds: unorderedScopePart('A,C') }) ) }) it('treats an all-empty list as absent, matching the parsers', () => { @@ -301,3 +406,43 @@ describe('unordered filter scope parts', () => { expect(unorderedScopePart('A,,B')).toBe('A,B') }) }) + +/** + * The declaration sweep in `contracts/v2/__tests__/list-pagination.test.ts` + * reconciles each list's cursor binding against its CONTRACT, and never looks at + * the `cursorScopeKey` call the route actually makes — which is precisely where + * the missing list and parent identity sat. This closes that half: the scope a + * route builds must start from its own route identity, and a nested list must + * resolve the path params that name its parent. + */ +describe('every v2 route binds its cursor to its route identity', () => { + const routeFiles = globSync('app/api/v2/**/route.ts', { + cwd: join(import.meta.dirname, '..', '..'), + absolute: true, + }) + + it('finds the v2 route tree', () => { + expect(routeFiles.length).toBeGreaterThan(50) + }) + + it.each(routeFiles.filter((file) => readFileSync(file, 'utf8').includes('cursorScopeKey(')))( + '%s', + (file) => { + const source = readFileSync(file, 'utf8') + const calls = [...source.matchAll(/cursorScopeKey\(\s*([A-Za-z_][\w.]*)/g)].map( + (match) => match[1] + ) + + expect(calls.length).toBeGreaterThan(0) + for (const first of calls) { + expect(first).toBe('cursorRoute') + } + + if (file.includes('[')) { + for (const call of source.matchAll(/cursorRoute\(([^)]*)\)/g)) { + expect(call[1]).toContain(',') + } + } + } + ) +}) diff --git a/apps/sim/lib/api/cursor-binding.ts b/apps/sim/lib/api/cursor-binding.ts index 95fc95b98eb..920cc72886e 100644 --- a/apps/sim/lib/api/cursor-binding.ts +++ b/apps/sim/lib/api/cursor-binding.ts @@ -158,24 +158,87 @@ export function fingerprint(canonical: string): string { } /** - * The fingerprint of a list's sequence-affecting params, or `undefined` when - * the caller supplied none of them. - * - * A cursor names a position in *one* sequence, so everything that reorders or - * re-filters that sequence has to travel with it — otherwise replaying the token - * against a re-filtered read silently answers from a sequence the caller never - * asked for. Pass every param that changes *which rows, in which order*. Keep - * `limit` out: it selects how much of the sequence to return, not what the - * sequence is, so a caller may change page size mid-walk. Response-shaping - * params (whether to inline trace spans, say) stay out for the same reason. - * - * `undefined` is a real state rather than an empty hash: an unstamped cursor is - * an unfiltered one, so it stays short, and replaying it under a filter still - * mismatches (`undefined !== `). Params whose value is `undefined` are - * dropped, so omitting a filter and never having sent it are the same scope. + * The list a cursor names a position in: the route that mints it, with the path + * params that pick out *which* parent resource resolved into the path. + * + * Identity is taken from the contract rather than a per-route literal on + * purpose. A hand-written name is a step an author can forget, and forgetting it + * is invisible: two lists whose only filter is `workspaceId` fingerprint + * identically and silently accept each other's tokens. The contract already + * carries the one string that is unique per list and impossible to omit. + */ +export interface CursorScopeRoute { + method: string + path: string + /** Resolved values for the path's `[placeholders]`. */ + params?: Record +} + +/** + * The route identity half of a cursor scope, built from the route's own + * contract so it cannot drift from the endpoint it binds. + * + * Pass `params` for every `[placeholder]` in the contract path — those name the + * parent resource, and a nested list bound only to its filters accepts a sibling + * parent's cursor and answers with the wrong rows. + */ +export function cursorRoute( + contract: { method: string; path: string }, + params?: Record +): CursorScopeRoute { + return { method: contract.method, path: contract.path, params } +} + +const PATH_PARAM_PATTERN = /\[([^\]]+)\]/g + +/** + * `METHOD /concrete/path`, with every `[placeholder]` replaced by its value. + * + * An unresolved placeholder throws rather than fingerprinting the template: a + * template binds every parent resource to one scope, which is the defect this + * exists to prevent, and a route whose params never reach here is misconfigured + * for every request rather than for an unlucky one. + */ +function resolveRouteIdentity(route: CursorScopeRoute): string { + const path = route.path.replace(PATH_PARAM_PATTERN, (_match, name: string) => { + const value = route.params?.[name] + if (value === undefined || value === '') { + throw new Error( + `cursorScopeKey: ${route.method} ${route.path} has no value for path param "${name}"` + ) + } + return encodeURIComponent(value) + }) + return `${route.method} ${path}` +} + +/** + * The fingerprint of the exact sequence a cursor names a position in: the list + * itself, plus the params that reorder or re-filter it. + * + * A cursor names a position in *one* sequence, so everything that decides which + * sequence that is has to travel with it — otherwise replaying the token + * silently answers from a sequence the caller never asked for. That includes the + * list's own identity, not only its filters: `GET /v2/tables` and + * `GET /v2/knowledge` share both a `{sort,keys,filter}` payload shape and a + * `workspaceId`-only filter set, so a fingerprint over filters alone made every + * such pair accept each other's tokens and answer 200 with a page that silently + * skipped rows. + * + * Pass every param that changes *which rows, in which order*. Keep `limit` out: + * it selects how much of the sequence to return, not what the sequence is, so a + * caller may change page size mid-walk. Response-shaping params (whether to + * inline trace spans, say) stay out for the same reason. + * + * Always a string, never `undefined`: the route identity is always present, so + * there is no "unstamped" cursor to represent. Params whose value is `undefined` + * are dropped, so omitting a filter and never having sent it are the same scope. */ -export function cursorScopeKey(parts: Record): string | undefined { - const present = filterUndefined(parts) - if (Object.keys(present).length === 0) return undefined - return fingerprint(canonicalJson(present)) +export function cursorScopeKey( + route: CursorScopeRoute, + parts: Record = {} +): string { + return fingerprint( + canonicalJson({ route: resolveRouteIdentity(route), filters: filterUndefined(parts) }) + ) } diff --git a/apps/sim/lib/api/server/validation.test.ts b/apps/sim/lib/api/server/validation.test.ts new file mode 100644 index 00000000000..61ca823b438 --- /dev/null +++ b/apps/sim/lib/api/server/validation.test.ts @@ -0,0 +1,74 @@ +/** + * @vitest-environment node + */ +import { NextRequest } from 'next/server' +import { describe, expect, it } from 'vitest' +import { DEFAULT_MAX_JSON_BODY_BYTES, parseJsonBody } from '@/lib/api/server/validation' + +/** + * Next.js truncates a proxied client body past `experimental.proxyClientMaxBodySize` + * without signalling it, so this is the largest body a handler can actually receive. + */ +const PROXY_CLIENT_MAX_BODY_BYTES = 10 * 1024 * 1024 + +/** + * Declares `content-length` independently of the bytes actually attached, which is + * how the guard sees an oversized request without buffering one in the test. + */ +function requestDeclaring(contentLength: number, body: string): NextRequest { + return new NextRequest('http://localhost/api/v2/widgets', { + method: 'POST', + headers: { + 'content-type': 'application/json', + 'content-length': String(contentLength), + }, + body, + }) +} + +describe('DEFAULT_MAX_JSON_BODY_BYTES', () => { + it('never exceeds the body size the proxy will forward intact', () => { + expect(DEFAULT_MAX_JSON_BODY_BYTES).toBeLessThanOrEqual(PROXY_CLIENT_MAX_BODY_BYTES) + }) +}) + +describe('parseJsonBody default size boundary', () => { + it('accepts a body at the proxy cap', async () => { + const result = await parseJsonBody( + requestDeclaring(PROXY_CLIENT_MAX_BODY_BYTES, JSON.stringify({ value: 'ok' })) + ) + + expect(result.success).toBe(true) + }) + + it('accepts a body just under the proxy cap', async () => { + const result = await parseJsonBody( + requestDeclaring(PROXY_CLIENT_MAX_BODY_BYTES - 1, JSON.stringify({ value: 'ok' })) + ) + + expect(result.success).toBe(true) + }) + + it('rejects a body just over the proxy cap as too large, not as malformed', async () => { + const result = await parseJsonBody( + requestDeclaring(PROXY_CLIENT_MAX_BODY_BYTES + 1, JSON.stringify({ value: 'ok' })) + ) + + expect(result.success).toBe(false) + if (result.success) return + expect(result.reason).toBe('too_large') + expect(result.response.status).toBe(413) + }) + + it('still reports a genuinely malformed body as malformed', async () => { + const result = await parseJsonBody(requestDeclaring(7, '{"a": ')) + + expect(result.success).toBe(false) + if (result.success) return + expect(result.reason).toBe('invalid_json') + expect(result.response.status).toBe(400) + await expect(result.response.json()).resolves.toEqual({ + error: 'Request body must be valid JSON', + }) + }) +}) diff --git a/apps/sim/lib/api/server/validation.ts b/apps/sim/lib/api/server/validation.ts index 9e9fdb50286..42a6f55ec15 100644 --- a/apps/sim/lib/api/server/validation.ts +++ b/apps/sim/lib/api/server/validation.ts @@ -20,16 +20,31 @@ import { readStreamToBufferWithLimit, } from '@/lib/core/utils/stream-limits' +/** + * Next.js buffers the client body for the proxy and *silently truncates* anything + * past `experimental.proxyClientMaxBodySize` (default 10 MB), and `apps/sim/proxy.ts` + * matches `/api/:path*`. A larger body therefore reaches the handler as a truncated + * prefix, which fails JSON parsing — so an oversized request has to be rejected on + * its declared size before it is read, or the truncation gets misreported as a + * malformed body. + */ +const PROXY_CLIENT_MAX_BODY_BYTES = 10 * 1024 * 1024 + /** * Default upper bound on the JSON request body that contract routes will read - * and parse into memory. Next.js App Router imposes no body cap, so without - * this an unauthenticated caller could buffer an arbitrarily large body before - * schema validation runs. Override per-route via `ParseRequestOptions.maxBodyBytes`. + * and parse into memory. Without a cap an unauthenticated caller could buffer a + * large body before schema validation runs. Override per-route via + * `ParseRequestOptions.maxBodyBytes`. + * * Falls back to 50 MB if the env value is missing or non-numeric so a misconfig - * can never silently disable the cap (a NaN limit would never reject). + * can never silently disable the cap (a NaN limit would never reject), then + * clamps to {@link PROXY_CLIENT_MAX_BODY_BYTES} because the app can never + * actually receive more than the proxy forwards. */ -export const DEFAULT_MAX_JSON_BODY_BYTES = - Number.parseInt(env.API_MAX_JSON_BODY_BYTES, 10) || 50 * 1024 * 1024 +export const DEFAULT_MAX_JSON_BODY_BYTES = Math.min( + Number.parseInt(env.API_MAX_JSON_BODY_BYTES, 10) || 50 * 1024 * 1024, + PROXY_CLIENT_MAX_BODY_BYTES +) export interface ValidationErrorBody { error: string diff --git a/apps/sim/lib/knowledge/search/queries.test.ts b/apps/sim/lib/knowledge/search/queries.test.ts index 6186d4f5a0c..dda82f335c0 100644 --- a/apps/sim/lib/knowledge/search/queries.test.ts +++ b/apps/sim/lib/knowledge/search/queries.test.ts @@ -2,6 +2,7 @@ * @vitest-environment node */ import { describe, expect, it } from 'vitest' +import { buildTagFilterCondition } from '@/lib/knowledge/documents/tag-filter' import { getStructuredTagFilters } from '@/lib/knowledge/search/queries' import type { StructuredFilter } from '@/lib/knowledge/types' @@ -20,10 +21,14 @@ const embeddingTable = { * The global `drizzle-orm` mock renders `sql` fragments to a `?`-placeholder * string via `toSQL()`, so we can assert the exact predicate each filter builds. */ +function render(condition: unknown) { + return (condition as { toSQL: () => { sql: string; params: unknown[] } }).toSQL() +} + function renderOne(filters: StructuredFilter[]) { const conditions = getStructuredTagFilters(filters, embeddingTable) expect(conditions).toHaveLength(1) - return (conditions[0] as unknown as { toSQL: () => { sql: string; params: unknown[] } }).toSQL() + return render(conditions[0]) } describe('getStructuredTagFilters', () => { @@ -152,8 +157,17 @@ describe('getStructuredTagFilters', () => { expect(sql).toBe('? != ?') expect(params).toEqual(['boolean1', false]) }) + }) - it('ORs two filters on the same slot and keeps them one condition', () => { + /** + * The callers spread the returned conditions into `and(...)`, so one condition + * per filter is what makes the whole array conjunctive. Grouping same-slot + * filters into a single OR'd condition made search answer an impossible + * predicate with a full page while the document list, which ANDs the same + * filters, answered with nothing. + */ + describe('every filter is a conjunct, including two naming the same tag', () => { + it('emits one condition per filter for two filters on the same slot', () => { const conditions = getStructuredTagFilters( [ { tagSlot: 'tag1', fieldType: 'text', operator: 'eq', value: 'a' }, @@ -161,11 +175,62 @@ describe('getStructuredTagFilters', () => { ], embeddingTable ) - expect(conditions).toHaveLength(1) - const joined = (conditions[0] as unknown as { values: unknown[] }).values[0] as { - toSQL: () => { sql: string; params: unknown[] } - } - expect(joined.toSQL().params).toEqual(['tag1', 'a', 'tag1', 'b']) + expect(conditions).toHaveLength(2) + expect(render(conditions[0]).params).toEqual(['tag1', 'a']) + expect(render(conditions[1]).params).toEqual(['tag1', 'b']) + }) + + it('keeps an impossible same-tag range as two conditions rather than a union', () => { + const conditions = getStructuredTagFilters( + [ + { tagSlot: 'number1', fieldType: 'number', operator: 'gte', value: '9' }, + { tagSlot: 'number1', fieldType: 'number', operator: 'lte', value: '2' }, + ], + embeddingTable + ) + expect(conditions).toHaveLength(2) + expect(render(conditions[0]).sql).toBe('? >= ?') + expect(render(conditions[1]).sql).toBe('? <= ?') + expect(conditions.every((condition) => !render(condition).sql.includes('OR'))).toBe(true) + }) + + it('still emits one condition per filter across different slots', () => { + const conditions = getStructuredTagFilters( + [ + { tagSlot: 'tag1', fieldType: 'text', operator: 'eq', value: 'a' }, + { tagSlot: 'number1', fieldType: 'number', operator: 'gte', value: '9' }, + { tagSlot: 'boolean1', fieldType: 'boolean', operator: 'eq', value: 'true' }, + ], + embeddingTable + ) + expect(conditions).toHaveLength(3) + }) + }) + + /** + * The document list builds one predicate per filter and pushes each into a + * single `and(...)`. Search must yield the same number of conjuncts for the + * same filters, or the two surfaces answer different questions over one tag + * vocabulary. + */ + describe('agreement with the document-list surface', () => { + it('produces the same number of conjuncts as the document-list builder', () => { + const filters: StructuredFilter[] = [ + { tagSlot: 'number1', fieldType: 'number', operator: 'gte', value: '9' }, + { tagSlot: 'number1', fieldType: 'number', operator: 'lte', value: '2' }, + ] + + const listConditions = filters.map((filter) => + buildTagFilterCondition({ + tagSlot: filter.tagSlot, + fieldType: 'number', + operator: filter.operator, + value: filter.value, + }) + ) + + expect(listConditions.every((condition) => condition !== undefined)).toBe(true) + expect(getStructuredTagFilters(filters, embeddingTable)).toHaveLength(listConditions.length) }) }) }) diff --git a/apps/sim/lib/knowledge/search/queries.ts b/apps/sim/lib/knowledge/search/queries.ts index 1b380f1ac4e..a28a2e78536 100644 --- a/apps/sim/lib/knowledge/search/queries.ts +++ b/apps/sim/lib/knowledge/search/queries.ts @@ -282,9 +282,18 @@ function buildFilterCondition(filter: StructuredFilter, embeddingTable: any) { } /** - * Build SQL conditions from structured filters with operator support - * - Same tag multiple times: OR logic - * - Different tags: AND logic + * Build SQL conditions from structured filters with operator support. Every + * filter is a conjunct, including two that name the same tag. + * + * Search used to group filters by slot and OR same-slot conditions together, + * which made the two surfaces over the same tag vocabulary answer different + * questions: the document list ANDs every filter, so `gte 9` plus `lte 2` on one + * number tag returned nothing there and a full page of results from search — + * a widening on the billed endpoint, the same failure mode as dropping a filter. + * OR also made a range on a single text tag (`contains A` and `contains B`) + * inexpressible, while the union it produced stays reachable as separate + * searches. Neither contract ever documented the OR, so no caller could have + * been relying on it deliberately. * * Every filter reaching here has already been validated, so one that fails to * compile is a defect rather than a predicate to skip. Skipping it dropped the @@ -293,36 +302,11 @@ function buildFilterCondition(filter: StructuredFilter, embeddingTable: any) { * paid for the widened scan. It is reported as a validation failure instead. */ export function getStructuredTagFilters(filters: StructuredFilter[], embeddingTable: any) { - // Group filters by tagSlot - const filtersBySlot = new Map() - for (const filter of filters) { - const slot = filter.tagSlot - if (!filtersBySlot.has(slot)) { - filtersBySlot.set(slot, []) - } - filtersBySlot.get(slot)!.push(filter) - } - - // Build conditions: OR within same slot, AND across different slots - const conditions: ReturnType[] = [] - - for (const [, slotFilters] of filtersBySlot) { - const slotConditions = slotFilters.map((f) => { - const condition = buildFilterCondition(f, embeddingTable) - if (condition === null) throw uncompilableTagFilterError(f) - return condition - }) - - if (slotConditions.length === 1) { - // Single condition for this slot - conditions.push(slotConditions[0]) - } else { - // Multiple conditions for same slot - OR them together - conditions.push(sql`(${sql.join(slotConditions, sql` OR `)})`) - } - } - - return conditions + return filters.map((filter) => { + const condition = buildFilterCondition(filter, embeddingTable) + if (condition === null) throw uncompilableTagFilterError(filter) + return condition + }) } /** diff --git a/apps/sim/lib/logs/application/get-public-log.ts b/apps/sim/lib/logs/application/get-public-log.ts index c16fa10dc3b..cf40838c5c2 100644 --- a/apps/sim/lib/logs/application/get-public-log.ts +++ b/apps/sim/lib/logs/application/get-public-log.ts @@ -48,11 +48,20 @@ export interface GetPublicLogResult { * folder means the caller's own tree is inconsistent; it is wrong for a * diagnostic log read, where the run may long outlive the folder it ran in and a * 500 would withhold the whole run over one unresolvable field. + * + * `workflowExists` is the join, not the folder: the log's `workflow_id` is set + * null when the workflow is deleted, so the left join yields a null `folderId` + * that is indistinguishable from a workflow sitting at the workspace root. Left + * unseparated, a run whose workflow is gone reports the root path next to + * `deleted: true` — a path the caller can hand back to `folderPaths` as a filter + * for a workflow that is no longer in any folder at all. */ function publicLogFolderPath( pathById: ReadonlyMap, - folderId: string | null + folderId: string | null, + workflowExists: boolean ): string | null { + if (!workflowExists) return null if (!folderId) return ROOT_FOLDER_PATH return pathById.get(folderId) ?? null } @@ -90,7 +99,11 @@ export const getPublicLog = defineAuthorizedWorkspaceUseCase({ } return { log: { ...log, workflowState: sanitizeExecutionSnapshotState(log.workflowState) }, - workflowFolderPath: publicLogFolderPath(folderIndex.pathById, log.workflowFolderId), + workflowFolderPath: publicLogFolderPath( + folderIndex.pathById, + log.workflowFolderId, + log.workflowName !== null + ), executionData, } }, diff --git a/apps/sim/lib/logs/application/public-log-use-cases.test.ts b/apps/sim/lib/logs/application/public-log-use-cases.test.ts index f0584ff6169..de575537efc 100644 --- a/apps/sim/lib/logs/application/public-log-use-cases.test.ts +++ b/apps/sim/lib/logs/application/public-log-use-cases.test.ts @@ -101,6 +101,8 @@ const log = { executionId: 'run-1', workspaceId: 'workspace-1', workflowId: 'workflow-1', + /** Null exactly when the left join found no workflow row — the delete signal. */ + workflowName: 'Support triage', workflowFolderId: 'folder-1', workflowUserId: 'owner-1', workflowOwnerEmail: 'owner@example.com', @@ -196,6 +198,35 @@ describe('public log application use cases', () => { expect(result.workflowFolderPath).toBeNull() }) + /** + * A deleted workflow nulls the log's `workflow_id`, so the left join returns a + * null folder that is shape-identical to a workflow sitting at the root. Read + * as the root, the run reports `/` beside `deleted: true` and hands the caller + * a `folderPaths` value for a workflow that is in no folder at all. + */ + it('reports no folder path for a run whose workflow has been deleted', async () => { + mocks.getLogScope.mockResolvedValueOnce({ + executionId: 'run-1', + workspaceId: 'workspace-1', + workflowId: null, + }) + mocks.getLog.mockResolvedValueOnce({ + ...log, + workflowId: null, + workflowName: null, + workflowFolderId: null, + workflowUserId: null, + workflowOwnerEmail: null, + }) + + const result = await getPublicLog.execute({ + principal: workspacePrincipal, + input: { runId: 'run-1' }, + }) + + expect(result.workflowFolderPath).toBeNull() + }) + it('redacts credential values from the run snapshot', async () => { mocks.getLog.mockResolvedValueOnce({ ...log, diff --git a/apps/sim/lib/mcp/application/use-cases.test.ts b/apps/sim/lib/mcp/application/use-cases.test.ts index 73eb5404a19..9f59727b7a2 100644 --- a/apps/sim/lib/mcp/application/use-cases.test.ts +++ b/apps/sim/lib/mcp/application/use-cases.test.ts @@ -257,6 +257,24 @@ describe('MCP server application use cases', () => { expect(mocks.discoverServerTools).not.toHaveBeenCalled() }) + it('answers a disabled server with a conflict instead of an untyped fault', async () => { + mocks.getServer.mockResolvedValueOnce({ ...server, enabled: false }) + + await expect( + discoverMcpServerToolsUseCase.execute({ + principal: { kind: 'personal_api_key', userId: 'user-1', keyId: 'key-1' }, + input: { workspaceId: workspace.workspaceId, serverId: server.id }, + }) + ).rejects.toMatchObject({ code: 'conflict' }) + + /** + * Discovery loads its configuration through a query that filters on + * `enabled`, so reaching it raised a plain `Error` — rendered as a 500 for + * a documented registration value — and stamped a bogus failure on the row. + */ + expect(mocks.discoverServerTools).not.toHaveBeenCalled() + }) + it('discovers one server tools for the acting subject, honouring refresh', async () => { const tools = [ { diff --git a/apps/sim/lib/mcp/application/use-cases.ts b/apps/sim/lib/mcp/application/use-cases.ts index 3189a7ef481..dc7d3433516 100644 --- a/apps/sim/lib/mcp/application/use-cases.ts +++ b/apps/sim/lib/mcp/application/use-cases.ts @@ -159,6 +159,20 @@ export const discoverMcpServerToolsUseCase = defineAuthorizedWorkspaceUseCase({ resolveServerContext(input.workspaceId, input.serverId), authorizationOptions, async execute({ principal, input, context }) { + /** + * `enabled: false` is a documented registration value, but discovery loads + * its configuration through a query that filters on `enabled`, so a + * disabled server surfaced as an untyped "not accessible" fault — rendered + * as a Sim-side 500 — and stamped a bogus failure on the row on the way + * out. It is a state conflict the caller resolves by enabling the server. + */ + if (!context.server.enabled) { + throw new OrchestrationError( + 'conflict', + 'The MCP server is disabled; enable it before listing its tools' + ) + } + const tools = await mcpService.discoverServerTools( requirePrincipalSubjectUserId(principal), context.server.id, diff --git a/apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts b/apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts index f89751d23db..b6df53b6b12 100644 --- a/apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts +++ b/apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts @@ -167,6 +167,81 @@ describe('MCP server lifecycle orchestration', () => { ) }) + it('resets the connection when a headers server rotates the headers that authenticate it', async () => { + dbChainMockFns.limit.mockResolvedValueOnce([ + { + url: 'https://example.com/mcp', + authType: 'headers', + headers: { authorization: 'Bearer original' }, + oauthClientId: null, + oauthClientSecret: null, + }, + ]) + dbChainMockFns.returning.mockResolvedValueOnce([ + { + id: 'server-1', + workspaceId: 'workspace-1', + name: 'Example', + transport: 'streamable-http', + url: 'https://example.com/mcp', + authType: 'headers', + }, + ]) + + const result = await performUpdateMcpServer({ + workspaceId: 'workspace-1', + userId: 'user-1', + serverId: 'server-1', + headers: { authorization: 'Bearer rotated' }, + }) + + expect(result.success).toBe(true) + expect(dbChainMockFns.set).toHaveBeenCalledWith( + expect.objectContaining({ + headers: { authorization: 'Bearer rotated' }, + connectionStatus: 'disconnected', + lastConnected: null, + lastError: null, + }) + ) + // Rotating headers invalidates nothing OAuth holds, so the grant survives. + expect(mockRevokeOauthTokens).not.toHaveBeenCalled() + }) + + it('leaves the connection alone when a headers rewrite changes nothing', async () => { + dbChainMockFns.limit.mockResolvedValueOnce([ + { + url: 'https://example.com/mcp', + authType: 'headers', + headers: { authorization: 'Bearer original' }, + oauthClientId: null, + oauthClientSecret: null, + }, + ]) + dbChainMockFns.returning.mockResolvedValueOnce([ + { + id: 'server-1', + workspaceId: 'workspace-1', + name: 'Example', + transport: 'streamable-http', + url: 'https://example.com/mcp', + authType: 'headers', + }, + ]) + + const result = await performUpdateMcpServer({ + workspaceId: 'workspace-1', + userId: 'user-1', + serverId: 'server-1', + headers: { authorization: 'Bearer original' }, + }) + + expect(result.success).toBe(true) + expect(dbChainMockFns.set).not.toHaveBeenCalledWith( + expect.objectContaining({ connectionStatus: 'disconnected' }) + ) + }) + it('audits only the columns an edit wrote, not the params it was handed', async () => { dbChainMockFns.limit.mockResolvedValueOnce([ { @@ -278,6 +353,64 @@ describe('MCP server lifecycle orchestration', () => { ) }) + it('stores an explicit retries of 0 rather than folding it into the default', async () => { + mockGenerateMcpServerId.mockReturnValue('server-1') + dbChainMockFns.limit.mockResolvedValueOnce([]) + dbChainMockFns.limit.mockResolvedValueOnce([ + { + id: 'server-1', + workspaceId: 'workspace-1', + name: 'Example', + transport: 'streamable-http', + url: 'https://example.com/anything', + authType: 'headers', + }, + ]) + + const result = await performCreateMcpServer({ + workspaceId: 'workspace-1', + userId: 'user-1', + name: 'Example', + url: 'https://example.com/anything', + authType: 'headers', + retries: 0, + }) + + expect(result.success).toBe(true) + expect(dbChainMockFns.values).toHaveBeenCalledWith(expect.objectContaining({ retries: 0 })) + }) + + it('keeps an explicit auth type when an OAuth client ID is also supplied', async () => { + mockGenerateMcpServerId.mockReturnValue('server-1') + dbChainMockFns.limit.mockResolvedValueOnce([]) + dbChainMockFns.limit.mockResolvedValueOnce([ + { + id: 'server-1', + workspaceId: 'workspace-1', + name: 'Example', + transport: 'streamable-http', + url: 'https://example.com/anything', + authType: 'headers', + }, + ]) + + const result = await performCreateMcpServer({ + workspaceId: 'workspace-1', + userId: 'user-1', + name: 'Example', + url: 'https://example.com/anything', + authType: 'headers', + oauthClientId: 'client-1', + oauthClientIdProvided: true, + }) + + expect(result.success).toBe(true) + expect(result.authType).toBe('headers') + expect(dbChainMockFns.values).toHaveBeenCalledWith( + expect.objectContaining({ authType: 'headers', oauthClientId: 'client-1' }) + ) + }) + it('leaves a re-registered server disconnected until discovery re-runs', async () => { mockGenerateMcpServerId.mockReturnValue('server-1') dbChainMockFns.limit.mockResolvedValueOnce([ diff --git a/apps/sim/lib/mcp/orchestration/server-lifecycle.ts b/apps/sim/lib/mcp/orchestration/server-lifecycle.ts index ca9ecde3730..f0a7cc64f34 100644 --- a/apps/sim/lib/mcp/orchestration/server-lifecycle.ts +++ b/apps/sim/lib/mcp/orchestration/server-lifecycle.ts @@ -142,7 +142,13 @@ export async function createMcpServer( const transport = params.transport || 'streamable-http' const timeout = params.timeout || 30000 - const retries = params.retries || 3 + /** + * `0` is a published, in-range value meaning "no retry", so the default may + * only apply when the field is absent. `||` folded it into `3`, which is the + * opposite of what the caller asked for; the update path already guards on + * `!== undefined`. + */ + const retries = params.retries ?? 3 const enabled = params.enabled !== false const serverId = params.url ? generateMcpServerId(params.workspaceId, params.url) : generateId() @@ -195,7 +201,14 @@ export async function createMcpServer( } } } - if (params.oauthClientId) resolvedAuthType = 'oauth' + /** + * An OAuth client id only *implies* an auth type; it may not overrule one + * the caller stated. Unconditional promotion turned an explicit + * `authType: 'headers'` into `oauth`, so the caller's own header + * configuration was never used to authenticate. The update path already + * promotes only when `authType` is absent. + */ + if (!params.authType && params.oauthClientId) resolvedAuthType = 'oauth' if (existingServer) { const credsChanged = await oauthCredsChanged({ @@ -378,6 +391,7 @@ export async function updateMcpServer( .select({ url: mcpServers.url, authType: mcpServers.authType, + headers: mcpServers.headers, oauthClientId: mcpServers.oauthClientId, oauthClientSecret: mcpServers.oauthClientSecret, }) @@ -415,8 +429,26 @@ export async function updateMcpServer( // Turning OAuth off must revoke and delete its now-orphaned tokens, not just reset the connection. const oauthDisabled = currentServer.authType === 'oauth' && resolvedAuthType !== 'oauth' const shouldClearOauth = urlChanged || credsChanged || oauthDisabled + /** + * On a `headers` server the headers *are* the credential, so rotating them + * invalidates the connection exactly as an OAuth credential change does — + * and the registration path already counts headers as a connection input. + * The reset is scoped to that auth type: under `oauth` (or `none`) the + * headers authenticate nothing, and clearing an OAuth server's status + * strands it, since discovery only reruns for an OAuth row that is + * `connected`. Header revocation never revokes the OAuth grant, so this + * stays out of `shouldClearOauth`. + */ + const headersInvalidateAuth = + resolvedAuthType === 'headers' && + params.headers !== undefined && + !isEqual(currentServer.headers ?? {}, params.headers) // An auth-type flip (either direction) or OAuth creds/URL change invalidates the connection: reset and clear stale state. - if (authTypeChanged || (shouldClearOauth && resolvedAuthType === 'oauth')) { + if ( + authTypeChanged || + headersInvalidateAuth || + (shouldClearOauth && resolvedAuthType === 'oauth') + ) { updateData.connectionStatus = 'disconnected' updateData.lastConnected = null updateData.lastError = null diff --git a/apps/sim/lib/table/application/groups.test.ts b/apps/sim/lib/table/application/groups.test.ts index 718790a6450..8a8fd6f6ef6 100644 --- a/apps/sim/lib/table/application/groups.test.ts +++ b/apps/sim/lib/table/application/groups.test.ts @@ -111,6 +111,29 @@ const table: TableDefinition = { createdAt: new Date('2026-08-01T00:00:00.000Z'), updatedAt: new Date('2026-08-01T00:00:00.000Z'), } +/** An enrichment group stores `workflowId: ''` — there is no workflow to resolve. */ +const enrichmentGroup: WorkflowGroup = { + id: 'group-enrichment', + workflowId: '', + enrichmentId: 'company-domain', + type: 'enrichment', + outputs: [{ blockId: '', path: 'domain', columnName: 'column-domain' }], +} +const enrichmentTable: TableDefinition = { + ...table, + schema: { + columns: [ + { id: 'column-name', name: 'name', type: 'string' }, + { + id: 'column-domain', + name: 'domain', + type: 'string', + workflowGroupId: 'group-enrichment', + }, + ], + workflowGroups: [enrichmentGroup], + }, +} const principal = { kind: 'delegated' as const, serviceId: 'copilot' as const, @@ -406,6 +429,87 @@ describe('workflow and enrichment Table application commands', () => { ) }) + it('extends an enrichment group with a new output without resolving a workflow', async () => { + mocks.resolveContext.mockResolvedValueOnce({ + tableId: table.id, + table: enrichmentTable, + workspaceId: table.workspaceId, + workspaceOrganizationId: null, + allowPersonalApiKeys: true, + billedAccountUserId: 'billing-owner-1', + }) + mocks.updateGroup.mockImplementation(async (input) => ({ + ...enrichmentTable, + schema: { + ...enrichmentTable.schema, + workflowGroups: [{ ...enrichmentGroup, outputs: input.outputs ?? enrichmentGroup.outputs }], + }, + })) + + await updateTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + groupId: enrichmentGroup.id, + outputs: [...enrichmentGroup.outputs, { blockId: '', path: 'name', columnName: 'zz_z' }], + newOutputColumns: [{ name: 'zz_z', type: 'string' }], + }, + }) + + expect(mocks.resolveWorkflowContext).not.toHaveBeenCalled() + expect(mocks.updateGroup).toHaveBeenCalledWith( + expect.objectContaining({ + groupId: enrichmentGroup.id, + newOutputColumns: [{ name: 'zz_z', type: 'string', workflowGroupId: enrichmentGroup.id }], + }), + 'request-1' + ) + }) + + it('still resolves the workflow for a new output coordinate on a manual group', async () => { + await updateTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + groupId: group.id, + outputs: [...group.outputs, { blockId: 'block-2', path: 'score', columnName: 'score' }], + newOutputColumns: [{ name: 'score', type: 'number' }], + }, + }) + + expect(mocks.resolveWorkflowContext).toHaveBeenCalledWith({ + workflowId: 'workflow-1', + assertedWorkspaceId: 'workspace-1', + }) + expect(mocks.updateGroup).toHaveBeenCalledWith( + expect.objectContaining({ + newOutputColumns: [{ name: 'score', type: 'number', workflowGroupId: group.id }], + }), + 'request-1' + ) + }) + + it('refuses an output column no output names instead of dropping it', async () => { + await expect( + updateTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + groupId: group.id, + newOutputColumns: [{ name: 'zz_w', type: 'string' }], + }, + }) + ).rejects.toMatchObject({ + code: 'validation', + message: 'newOutputColumns entry "zz_w" has no matching outputs[].columnName', + }) + + expect(mocks.updateGroup).not.toHaveBeenCalled() + }) + it('does not start auto-run when the generic update saves a legacy enabled group', async () => { await updateTableGroupUseCase.execute({ principal, diff --git a/apps/sim/lib/table/application/groups.ts b/apps/sim/lib/table/application/groups.ts index b842b0a60c0..aac1fbf971d 100644 --- a/apps/sim/lib/table/application/groups.ts +++ b/apps/sim/lib/table/application/groups.ts @@ -567,15 +567,44 @@ export const updateTableGroupUseCase = defineAuthorizedTableUseCase({ const previousGroup = (context.table.schema.workflowGroups ?? []).find( (group) => group.id === input.groupId ) + /** + * An enrichment group's outputs come from the registry, not from a workflow, + * and it stores `workflowId: ''` — so there is nothing to resolve a new + * output coordinate against. Validating one anyway resolved the empty id and + * answered `404 Workflow not found`, which made an enrichment group's output + * set permanently unextendable. Only a body that supplies a `workflowId` + * converts the group to workflow-backed and needs workflow metadata. + */ + const producerIsEnrichment = + input.workflowId === undefined && + previousGroup !== undefined && + (previousGroup.type === 'enrichment' || !previousGroup.workflowId) + /** + * A `newOutputColumns` entry that no resulting output names is dropped by the + * writer, so a caller asking for a column got a 200 and no column. Refuse it + * the way group creation refuses an orphan `outputColumns` entry. + */ + if (input.newOutputColumns?.length) { + const requestedOutputNames = new Set((input.outputs ?? []).map((output) => output.columnName)) + const orphan = input.newOutputColumns.find((column) => !requestedOutputNames.has(column.name)) + if (orphan) { + throw new OrchestrationError( + 'validation', + `newOutputColumns entry "${orphan.name}" has no matching outputs[].columnName` + ) + } + } const previousOutputKeys = new Set( previousGroup?.outputs.map((output) => `${output.blockId}::${output.path}`) ?? [] ) const workflowChanged = input.workflowId !== undefined && input.workflowId !== previousGroup?.workflowId - const outputCoordinatesToValidate = - input.outputs?.filter( - (output) => workflowChanged || !previousOutputKeys.has(`${output.blockId}::${output.path}`) - ) ?? [] + const outputCoordinatesToValidate = producerIsEnrichment + ? [] + : (input.outputs?.filter( + (output) => + workflowChanged || !previousOutputKeys.has(`${output.blockId}::${output.path}`) + ) ?? []) const workflowMetadataRequired = input.workflowId !== undefined || outputCoordinatesToValidate.length > 0 || diff --git a/apps/sim/lib/table/views/service.test.ts b/apps/sim/lib/table/views/service.test.ts index 1cc5c7ec5c0..33a1cc5d7e9 100644 --- a/apps/sim/lib/table/views/service.test.ts +++ b/apps/sim/lib/table/views/service.test.ts @@ -408,6 +408,42 @@ describe('view config column-reference normalization', () => { expect(dbChainMockFns.insert).not.toHaveBeenCalled() }) + /** + * The layout half used to answer 200 and store a name no read ever shows, + * while the same unknown name in `filter` answered 400 — one request, two + * policies. + */ + it('refuses a hidden column that does not exist for a strict caller', async () => { + queueTableRows(tableViews, [{ total: 0 }]) + dbChainMockFns.returning.mockResolvedValueOnce([storedRow]) + + await expect(create({ hiddenColumns: ['ghost'] })).rejects.toMatchObject({ + name: 'TableViewValidationError', + }) + expect(dbChainMockFns.insert).not.toHaveBeenCalled() + }) + + it('refuses an unknown column in every other layout key too', async () => { + for (const config of [ + { columnOrder: ['ghost'] }, + { pinnedColumns: ['ghost'] }, + { columnWidths: { ghost: 120 } }, + ]) { + queueTableRows(tableViews, [{ total: 0 }]) + await expect(create(config)).rejects.toMatchObject({ name: 'TableViewValidationError' }) + } + expect(dbChainMockFns.insert).not.toHaveBeenCalled() + }) + + it('keeps storing a first-party layout reference that no longer resolves', async () => { + queueTableRows(tableViews, [{ total: 0 }]) + dbChainMockFns.returning.mockResolvedValueOnce([storedRow]) + + await create({ hiddenColumns: ['col_gone'] }, false) + + expect(insertedConfig().hiddenColumns).toEqual(['col_gone']) + }) + it('refuses a sort on a column that does not exist for a strict caller', async () => { queueTableRows(tableViews, [{ total: 0 }]) dbChainMockFns.returning.mockResolvedValueOnce([storedRow]) diff --git a/apps/sim/lib/table/views/service.ts b/apps/sim/lib/table/views/service.ts index 1cd7462091e..19e10cb8b1e 100644 --- a/apps/sim/lib/table/views/service.ts +++ b/apps/sim/lib/table/views/service.ts @@ -177,10 +177,19 @@ function tolerantColumns( * * `filter` and `sort` are then validated: a predicate or sort naming no column * is one the query routes answer 400 for, so storing it would save a view that - * can never load. Column LAYOUT is deliberately not validated — it auto-saves as + * can never load. Column LAYOUT is not validated by default — it auto-saves as * the user drags, and racing a concurrent column delete must self-heal through * {@link pruneViewConfig}, not fail the drag. * + * `strictLayoutRefs` extends the same refusal to the LAYOUT keys, for the same + * caller that gets it on `filter` and `sort`: one request naming an unknown + * column twice answered 400 for the filter leaf and 200 for the hidden column, + * whose name was then stored in a blob no read ever shows (`pruneViewConfig` + * drops it) and would silently take effect if a column of that name were later + * created. It stays off for the first-party grid, whose layout auto-saves as the + * user drags and must self-heal past a concurrent column delete. A v2 caller + * cannot carry a dangling layout ref forward: the read it echoes is pruned. + * * `carriedForward` names the references that are exempt from that refusal. * Deleting a column leaves every view that filtered on it dangling — * `pruneViewConfig` deliberately does not prune a filter — so without the @@ -195,7 +204,8 @@ function tolerantColumns( export function normalizeViewConfigForStorage( config: TableViewConfig, columns: ColumnDefinition[], - carriedForward: readonly string[] = [] + carriedForward: readonly string[] = [], + strictLayoutRefs = false ): TableViewConfig { const stored = remapViewConfigColumnRefs(config, viewConfigRefMap(columns)) const known = tolerantColumns(columns, carriedForward) @@ -208,9 +218,38 @@ export function normalizeViewConfigForStorage( } throw error } + if (strictLayoutRefs) assertLayoutRefsResolve(stored, known) return stored } +/** The layout keys, paired with the references each one holds. */ +function layoutRefEntries(config: TableViewConfig): Array<[string, readonly string[]]> { + return [ + ['columnOrder', config.columnOrder ?? []], + ['pinnedColumns', config.pinnedColumns ?? []], + ['hiddenColumns', config.hiddenColumns ?? []], + ['columnWidths', Object.keys(config.columnWidths ?? {})], + ] +} + +/** + * Refuses a layout reference naming no column. Mirrors what the query + * validators do for `filter` and `sort`, including the message shape, so one + * config gets one answer whichever half the unknown name landed in. + */ +function assertLayoutRefsResolve(config: TableViewConfig, known: ColumnDefinition[]): void { + const live = new Set(known.map(getColumnId)) + for (const [key, refs] of layoutRefEntries(config)) { + for (const ref of refs) { + if (!live.has(ref)) { + throw new TableViewValidationError( + `Unknown ${key} column "${ref}". It is not a column on this table.` + ) + } + } + } +} + /** * Migrates a config stored before the grammar switch. The feature never * released, so legacy-shaped rows exist only from pre-refactor testing: a @@ -371,9 +410,9 @@ export interface CreateTableViewData { userId: string columns: ColumnDefinition[] /** - * Whether to refuse a filter or sort reference naming no live column. Set by - * the `/api/v2` surface only, whose caller authored the config in this request - * and can be told which reference was wrong. + * Whether to refuse a filter, sort, or column-layout reference naming no live + * column. Set by the `/api/v2` surface only, whose caller authored the config + * in this request and can be told which reference was wrong. * * Absent — the first-party grid, which does not author these refs so much as * carry them: a view filtered on a since-deleted column keeps the dangling @@ -405,7 +444,8 @@ export async function createTableView(data: CreateTableViewData): Promise { @@ -500,11 +540,21 @@ export async function updateTableView(data: UpdateTableViewData): Promise = { updatedAt: new Date() } if (data.name !== undefined) patch.name = normalizeName(data.name) From 02dd242fe3a8db640f204369d18a31ca32c3341d Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 13 Aug 2026 15:47:52 -0700 Subject: [PATCH 2/3] fix(v2): clamp explicit body caps to the proxy ceiling too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit clamped the default JSON body cap but left explicit per-route overrides alone, so a route declaring a larger `maxBodyBytes` still fell into the truncation it was meant to report: the four inline workspace-file routes at 70 MB and the deployed-chat route at 220 MB. Next attaches `proxyClientMaxBodySize` to every request and clones the body unconditionally for any non-GET method on a matched path, pushing EOF at ten mebibytes with only a warning, so the handler reads a truncated prefix. Those routes therefore already fail above that size — as a malformed-JSON 400. Clamping the effective limit inside the two body readers makes the same request fail as payload-too-large, quoting the limit actually in force. One existing test asserted the unreachable case, allowing a sixty-mebibyte base64 body; it now asserts what the proxy will forward intact. The inline-file path still advertises fifty mebibytes and cannot exceed the proxy ceiling until that ceiling is raised, which changes buffering for every route and belongs in its own change. --- .../[id]/files/[fileId]/content/route.test.ts | 13 ++-- apps/sim/lib/api/server/validation.test.ts | 71 ++++++++++++++++++- apps/sim/lib/api/server/validation.ts | 36 ++++++++-- 3 files changed, 109 insertions(+), 11 deletions(-) diff --git a/apps/sim/app/api/workspaces/[id]/files/[fileId]/content/route.test.ts b/apps/sim/app/api/workspaces/[id]/files/[fileId]/content/route.test.ts index 698f524a4ce..5be59775eea 100644 --- a/apps/sim/app/api/workspaces/[id]/files/[fileId]/content/route.test.ts +++ b/apps/sim/app/api/workspaces/[id]/files/[fileId]/content/route.test.ts @@ -135,9 +135,9 @@ describe('PUT /api/workspaces/[id]/files/[fileId]/content', () => { }) }) - it('allows JSON bodies above the default 50 MiB cap for base64 expansion', async () => { + it('allows a base64 JSON body up to what the proxy forwards intact', async () => { const response = await PUT( - createRequest({ content: 'TQ==', encoding: 'base64' }, 60 * 1024 * 1024), + createRequest({ content: 'TQ==', encoding: 'base64' }, 10 * 1024 * 1024), routeContext ) @@ -145,8 +145,13 @@ describe('PUT /api/workspaces/[id]/files/[fileId]/content', () => { expect(mocks.updateContent).toHaveBeenCalled() }) - it('rejects a JSON body above the inline-content cap after admission', async () => { - const response = await PUT(createRequest({ content: '' }, 70 * 1024 * 1024 + 1), routeContext) + /** + * The route declares a 70 MB inline cap, but Next's proxy truncates a client + * body past 10 MiB, so the parser clamps to that ceiling and answers 413 + * rather than letting a truncated prefix surface as malformed JSON. + */ + it('rejects a JSON body above the proxy ceiling after admission', async () => { + const response = await PUT(createRequest({ content: '' }, 10 * 1024 * 1024 + 1), routeContext) expect(response.status).toBe(413) expect(mocks.admit).toHaveBeenCalled() diff --git a/apps/sim/lib/api/server/validation.test.ts b/apps/sim/lib/api/server/validation.test.ts index 61ca823b438..68b050e9e8e 100644 --- a/apps/sim/lib/api/server/validation.test.ts +++ b/apps/sim/lib/api/server/validation.test.ts @@ -3,7 +3,11 @@ */ import { NextRequest } from 'next/server' import { describe, expect, it } from 'vitest' -import { DEFAULT_MAX_JSON_BODY_BYTES, parseJsonBody } from '@/lib/api/server/validation' +import { + DEFAULT_MAX_JSON_BODY_BYTES, + parseJsonBody, + parseOptionalJsonBody, +} from '@/lib/api/server/validation' /** * Next.js truncates a proxied client body past `experimental.proxyClientMaxBodySize` @@ -11,6 +15,12 @@ import { DEFAULT_MAX_JSON_BODY_BYTES, parseJsonBody } from '@/lib/api/server/val */ const PROXY_CLIENT_MAX_BODY_BYTES = 10 * 1024 * 1024 +/** Mirrors `MAX_WORKSPACE_FILE_INLINE_BODY_BYTES` — an explicit override above the ceiling. */ +const INLINE_FILE_BODY_BYTES = 70 * 1024 * 1024 + +/** Mirrors the knowledge-search override — below the ceiling, so the clamp must not touch it. */ +const BELOW_CEILING_BODY_BYTES = 2 * 1024 * 1024 + /** * Declares `content-length` independently of the bytes actually attached, which is * how the guard sees an oversized request without buffering one in the test. @@ -60,6 +70,65 @@ describe('parseJsonBody default size boundary', () => { expect(result.response.status).toBe(413) }) + it('rejects an explicit over-ceiling override the same way, quoting the enforced limit', async () => { + const result = await parseJsonBody( + requestDeclaring(PROXY_CLIENT_MAX_BODY_BYTES + 1, JSON.stringify({ value: 'ok' })), + 'response', + INLINE_FILE_BODY_BYTES + ) + + expect(result.success).toBe(false) + if (result.success) return + expect(result.reason).toBe('too_large') + expect(result.response.status).toBe(413) + await expect(result.response.json()).resolves.toEqual({ + error: `Request body exceeds the maximum allowed size of ${PROXY_CLIENT_MAX_BODY_BYTES} bytes`, + }) + }) + + it('still accepts a body at the ceiling under an over-ceiling override', async () => { + const result = await parseJsonBody( + requestDeclaring(PROXY_CLIENT_MAX_BODY_BYTES, JSON.stringify({ value: 'ok' })), + 'response', + INLINE_FILE_BODY_BYTES + ) + + expect(result.success).toBe(true) + }) + + it('leaves an override below the ceiling exactly as declared', async () => { + const atLimit = await parseJsonBody( + requestDeclaring(BELOW_CEILING_BODY_BYTES, JSON.stringify({ value: 'ok' })), + 'response', + BELOW_CEILING_BODY_BYTES + ) + expect(atLimit.success).toBe(true) + + const overLimit = await parseJsonBody( + requestDeclaring(BELOW_CEILING_BODY_BYTES + 1, JSON.stringify({ value: 'ok' })), + 'response', + BELOW_CEILING_BODY_BYTES + ) + expect(overLimit.success).toBe(false) + if (overLimit.success) return + expect(overLimit.reason).toBe('too_large') + await expect(overLimit.response.json()).resolves.toEqual({ + error: `Request body exceeds the maximum allowed size of ${BELOW_CEILING_BODY_BYTES} bytes`, + }) + }) + + it('applies the same clamp to an optional body', async () => { + const result = await parseOptionalJsonBody( + requestDeclaring(PROXY_CLIENT_MAX_BODY_BYTES + 1, JSON.stringify({ value: 'ok' })), + INLINE_FILE_BODY_BYTES + ) + + expect(result.success).toBe(false) + if (result.success) return + expect(result.reason).toBe('too_large') + expect(result.response.status).toBe(413) + }) + it('still reports a genuinely malformed body as malformed', async () => { const result = await parseJsonBody(requestDeclaring(7, '{"a": ')) diff --git a/apps/sim/lib/api/server/validation.ts b/apps/sim/lib/api/server/validation.ts index 42a6f55ec15..096b1a63656 100644 --- a/apps/sim/lib/api/server/validation.ts +++ b/apps/sim/lib/api/server/validation.ts @@ -46,6 +46,27 @@ export const DEFAULT_MAX_JSON_BODY_BYTES = Math.min( PROXY_CLIENT_MAX_BODY_BYTES ) +/** + * Clamps a per-route body cap to {@link PROXY_CLIENT_MAX_BODY_BYTES}. + * + * A route that raises `maxBodyBytes` above the proxy ceiling cannot actually + * receive a body that large: the proxy truncates the stream, the handler parses + * a prefix, and the caller gets `400 "Request body must be valid JSON"` for a + * request whose only fault was its size. Clamping at the point of use turns that + * into an accurate `413`; nothing that succeeds today changes, because a body + * over the ceiling already fails — just less honestly. + * + * Consequence worth keeping in view: `MAX_WORKSPACE_FILE_INLINE_BODY_BYTES` + * (70 MB) exists so a 50 MiB file can be sent inline as base64, and that ceiling + * stays unreachable until `experimental.proxyClientMaxBodySize` is raised in + * `apps/sim/next.config.ts`. Raising it changes the memory profile of every + * `/api` route, so it is a separate decision — this clamp only makes the limit + * that is actually in force report itself correctly. + */ +function clampToProxyLimit(maxBytes: number): number { + return Math.min(maxBytes, PROXY_CLIENT_MAX_BODY_BYTES) +} + export interface ValidationErrorBody { error: string details: z.core.$ZodIssue[] @@ -76,7 +97,8 @@ export interface ParseRequestOptions { /** * Maximum number of bytes to read for the JSON body before rejecting with a * 413. Defaults to {@link DEFAULT_MAX_JSON_BODY_BYTES}. Raise this only for - * routes that legitimately accept large JSON payloads (e.g. inline file uploads). + * routes that legitimately accept large JSON payloads (e.g. inline file uploads); + * a value above what the proxy forwards is clamped — see {@link clampToProxyLimit}. */ maxBodyBytes?: number /** Treat an absent or whitespace-only body as `undefined` before contract validation. */ @@ -163,8 +185,9 @@ export async function parseJsonBody( response: NextResponse<{ error: string }> } > { + const limit = clampToProxyLimit(maxBytes) try { - return { success: true, data: await readJsonBodyWithLimit(request, maxBytes) } + return { success: true, data: await readJsonBodyWithLimit(request, limit) } } catch (error) { if (invalidJson === 'throw') throw error if (isPayloadSizeLimitError(error)) { @@ -172,7 +195,7 @@ export async function parseJsonBody( success: false, reason: 'too_large', response: NextResponse.json( - { error: `Request body exceeds the maximum allowed size of ${maxBytes} bytes` }, + { error: `Request body exceeds the maximum allowed size of ${limit} bytes` }, { status: 413 } ), } @@ -203,13 +226,14 @@ export async function parseOptionalJsonBody( response: NextResponse<{ error: string }> } > { + const limit = clampToProxyLimit(maxBytes) try { - assertContentLengthWithinLimit(request.headers, maxBytes, REQUEST_BODY_LABEL) + assertContentLengthWithinLimit(request.headers, limit, REQUEST_BODY_LABEL) const stream = request.body const text = stream ? new TextDecoder().decode( - await readStreamToBufferWithLimit(stream, { maxBytes, label: REQUEST_BODY_LABEL }) + await readStreamToBufferWithLimit(stream, { maxBytes: limit, label: REQUEST_BODY_LABEL }) ) : await request.text() @@ -223,7 +247,7 @@ export async function parseOptionalJsonBody( success: false, reason: 'too_large', response: NextResponse.json( - { error: `Request body exceeds the maximum allowed size of ${maxBytes} bytes` }, + { error: `Request body exceeds the maximum allowed size of ${limit} bytes` }, { status: 413 } ), } From 6dafb52a0b28aed402117f39d788afc09501d077 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 13 Aug 2026 15:55:20 -0700 Subject: [PATCH 3/3] fix(v2): close the two holes the first review round found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both are places where a fix in this branch shut one door and left a smaller one open in the same wall. Letting an enrichment group gain an output meant skipping workflow resolution — but that resolution was the only thing validating a new output, so a PATCH began storing coordinates the runner can never fill. It fills a cell from `result[out.outputId]` and skips an output with no `outputId` at all, while the writer diffs on that same id and the sidebar reads and writes by it; the contract leaves it optional. The regression test added with that fix was itself asserting such a dead coordinate. Create's registry checks are now two shared helpers both paths call, and on update an output is exempt only when an identical binding already existed, so renaming a group whose enrichment has since changed still works while anything added or repointed must name a real output. `mappingUpdates` on an enrichment group now says it is inexpressible rather than resolving an empty workflow id into a missing workflow. The layout-reference check was handed the tolerant column set, so a placeholder minted to keep a dangling filter writable also whitelisted a brand-new layout reference — storing an entry the next read discards, which is the inconsistency the check was added to remove. Layout now resolves against the live columns, which is exactly what pruning keeps, while filters and sorts keep the exemption they need. --- apps/sim/lib/table/application/groups.test.ts | 148 +++++++++++++++--- apps/sim/lib/table/application/groups.ts | 86 ++++++++-- apps/sim/lib/table/views/service.test.ts | 55 +++++++ apps/sim/lib/table/views/service.ts | 10 +- 4 files changed, 264 insertions(+), 35 deletions(-) diff --git a/apps/sim/lib/table/application/groups.test.ts b/apps/sim/lib/table/application/groups.test.ts index 8a8fd6f6ef6..0375eb0c290 100644 --- a/apps/sim/lib/table/application/groups.test.ts +++ b/apps/sim/lib/table/application/groups.test.ts @@ -117,7 +117,7 @@ const enrichmentGroup: WorkflowGroup = { workflowId: '', enrichmentId: 'company-domain', type: 'enrichment', - outputs: [{ blockId: '', path: 'domain', columnName: 'column-domain' }], + outputs: [{ blockId: '', path: '', outputId: 'domain', columnName: 'column-domain' }], } const enrichmentTable: TableDefinition = { ...table, @@ -174,6 +174,37 @@ function tableWithGroup(nextGroup: WorkflowGroup, columns = table.schema.columns } } +/** + * Points the command at the enrichment table and a registry entry that defines a + * second output, so an extension has something valid to ask for. + */ +function useEnrichmentTable(): void { + mocks.resolveContext.mockResolvedValue({ + tableId: table.id, + table: enrichmentTable, + workspaceId: table.workspaceId, + workspaceOrganizationId: null, + allowPersonalApiKeys: true, + billedAccountUserId: 'billing-owner-1', + }) + mocks.getEnrichment.mockReturnValue({ + id: 'company-domain', + name: 'Company Domain', + inputs: [{ id: 'company', name: 'Company', type: 'string', required: true }], + outputs: [ + { id: 'domain', name: 'domain', type: 'string' }, + { id: 'company_name', name: 'company name', type: 'string' }, + ], + }) + mocks.updateGroup.mockImplementation(async (input) => ({ + ...enrichmentTable, + schema: { + ...enrichmentTable.schema, + workflowGroups: [{ ...enrichmentGroup, outputs: input.outputs ?? enrichmentGroup.outputs }], + }, + })) +} + describe('workflow and enrichment Table application commands', () => { beforeEach(() => { vi.clearAllMocks() @@ -429,22 +460,8 @@ describe('workflow and enrichment Table application commands', () => { ) }) - it('extends an enrichment group with a new output without resolving a workflow', async () => { - mocks.resolveContext.mockResolvedValueOnce({ - tableId: table.id, - table: enrichmentTable, - workspaceId: table.workspaceId, - workspaceOrganizationId: null, - allowPersonalApiKeys: true, - billedAccountUserId: 'billing-owner-1', - }) - mocks.updateGroup.mockImplementation(async (input) => ({ - ...enrichmentTable, - schema: { - ...enrichmentTable.schema, - workflowGroups: [{ ...enrichmentGroup, outputs: input.outputs ?? enrichmentGroup.outputs }], - }, - })) + it('extends an enrichment group with a registry output without resolving a workflow', async () => { + useEnrichmentTable() await updateTableGroupUseCase.execute({ principal, @@ -452,7 +469,10 @@ describe('workflow and enrichment Table application commands', () => { tableId: table.id, workspaceId: table.workspaceId, groupId: enrichmentGroup.id, - outputs: [...enrichmentGroup.outputs, { blockId: '', path: 'name', columnName: 'zz_z' }], + outputs: [ + ...enrichmentGroup.outputs, + { blockId: '', path: '', outputId: 'company_name', columnName: 'zz_z' }, + ], newOutputColumns: [{ name: 'zz_z', type: 'string' }], }, }) @@ -467,6 +487,97 @@ describe('workflow and enrichment Table application commands', () => { ) }) + it('refuses an enrichment output the registry does not define', async () => { + useEnrichmentTable() + + await expect( + updateTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + groupId: enrichmentGroup.id, + outputs: [ + ...enrichmentGroup.outputs, + { blockId: '', path: '', outputId: 'invented', columnName: 'zz_z' }, + ], + newOutputColumns: [{ name: 'zz_z', type: 'string' }], + }, + }) + ).rejects.toMatchObject({ + code: 'validation', + message: 'Enrichment "Company Domain" has no output "invented"', + }) + + expect(mocks.updateGroup).not.toHaveBeenCalled() + expect(mocks.audit).not.toHaveBeenCalled() + }) + + it('refuses an enrichment output coordinate that carries no registry output id', async () => { + useEnrichmentTable() + + await expect( + updateTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + groupId: enrichmentGroup.id, + outputs: [...enrichmentGroup.outputs, { blockId: '', path: 'name', columnName: 'zz_z' }], + newOutputColumns: [{ name: 'zz_z', type: 'string' }], + }, + }) + ).rejects.toMatchObject({ + code: 'validation', + message: 'Enrichment "Company Domain" has no output ""', + }) + + expect(mocks.updateGroup).not.toHaveBeenCalled() + }) + + it('leaves an untouched enrichment binding alone while renaming the group', async () => { + useEnrichmentTable() + + await updateTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + groupId: enrichmentGroup.id, + name: 'Renamed enrichment', + outputs: enrichmentGroup.outputs, + }, + }) + + expect(mocks.getEnrichment).not.toHaveBeenCalled() + expect(mocks.updateGroup).toHaveBeenCalledWith( + expect.objectContaining({ name: 'Renamed enrichment' }), + 'request-1' + ) + }) + + it('names the enrichment instead of a missing workflow for a mapping update', async () => { + useEnrichmentTable() + + await expect( + updateTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + groupId: enrichmentGroup.id, + mappingUpdates: [{ columnName: 'column-domain', blockId: 'block-1', path: 'content' }], + }, + }) + ).rejects.toMatchObject({ + code: 'validation', + message: 'Mapping updates are not supported for an enrichment group; send outputs[] instead', + }) + + expect(mocks.resolveWorkflowContext).not.toHaveBeenCalled() + expect(mocks.updateGroup).not.toHaveBeenCalled() + }) + it('still resolves the workflow for a new output coordinate on a manual group', async () => { await updateTableGroupUseCase.execute({ principal, @@ -483,6 +594,7 @@ describe('workflow and enrichment Table application commands', () => { workflowId: 'workflow-1', assertedWorkspaceId: 'workspace-1', }) + expect(mocks.getEnrichment).not.toHaveBeenCalled() expect(mocks.updateGroup).toHaveBeenCalledWith( expect.objectContaining({ newOutputColumns: [{ name: 'score', type: 'number', workflowGroupId: group.id }], diff --git a/apps/sim/lib/table/application/groups.ts b/apps/sim/lib/table/application/groups.ts index aac1fbf971d..a9e692d6c10 100644 --- a/apps/sim/lib/table/application/groups.ts +++ b/apps/sim/lib/table/application/groups.ts @@ -37,6 +37,7 @@ import { resolveActiveWorkflowApplicationContext } from '@/lib/workflows/applica import type { ResolveWorkflowOutputsResult } from '@/lib/workflows/application/resolve-workflow-outputs' import { loadResolvedWorkflowOutputs } from '@/lib/workflows/application/resolve-workflow-outputs' import { getEnrichment } from '@/enrichments/registry' +import type { EnrichmentConfig } from '@/enrichments/types' const logger = createLogger('TableGroupApplication') @@ -120,6 +121,38 @@ function validateRequestedOutputs( ) } +/** Resolves the registry enrichment a group is bound to, refusing an unknown id. */ +function requireEnrichment(enrichmentId: string | undefined): EnrichmentConfig { + const enrichment = getEnrichment(enrichmentId) + if (!enrichment) { + throw new OrchestrationError( + 'validation', + `Unknown enrichment "${enrichmentId ?? ''}". Call list_enrichments to see available ids.` + ) + } + return enrichment +} + +/** + * Refuses an output id the enrichment registry does not define. A run fills a + * cell by reading `result[outputId]`, so a coordinate carrying an unknown — or + * absent — output id names a column no run can ever write. + */ +function requireKnownEnrichmentOutputIds( + enrichment: EnrichmentConfig, + outputIds: Array +): void { + const known = new Set(enrichment.outputs.map((output) => output.id)) + for (const outputId of outputIds) { + if (!outputId || !known.has(outputId)) { + throw new OrchestrationError( + 'validation', + `Enrichment "${enrichment.name}" has no output "${outputId ?? ''}"` + ) + } + } +} + function workflowOutputColumnType( requestedType: string | undefined, resolvedLeafType: string | undefined @@ -413,13 +446,7 @@ export const createTableEnrichmentGroup = defineAuthorizedTableUseCase({ `Enrichment output names cannot exceed ${TABLE_LIMITS.MAX_COLUMNS_PER_TABLE} entries` ) } - const enrichment = getEnrichment(input.enrichmentId) - if (!enrichment) { - throw new OrchestrationError( - 'validation', - `Unknown enrichment "${input.enrichmentId}". Call list_enrichments to see available ids.` - ) - } + const enrichment = requireEnrichment(input.enrichmentId) const enrichmentInputIds = new Set( enrichment.inputs.map((enrichmentInput) => enrichmentInput.id) @@ -440,15 +467,7 @@ export const createTableEnrichmentGroup = defineAuthorizedTableUseCase({ } mappingByInput.set(mapping.inputName, mapping.columnName) } - const enrichmentOutputIds = new Set(enrichment.outputs.map((output) => output.id)) - for (const outputId of Object.keys(input.outputColumnNames ?? {})) { - if (!enrichmentOutputIds.has(outputId)) { - throw new OrchestrationError( - 'validation', - `Enrichment "${enrichment.name}" has no output "${outputId}"` - ) - } - } + requireKnownEnrichmentOutputIds(enrichment, Object.keys(input.outputColumnNames ?? {})) const existingColumns = new Set(context.table.schema.columns.map((column) => column.name)) for (const enrichmentInput of enrichment.inputs) { const mapped = mappingByInput.get(enrichmentInput.id) @@ -579,6 +598,41 @@ export const updateTableGroupUseCase = defineAuthorizedTableUseCase({ input.workflowId === undefined && previousGroup !== undefined && (previousGroup.type === 'enrichment' || !previousGroup.workflowId) + /** + * Skipping workflow resolution must not mean skipping validation. An + * enrichment run fills a column by registry `outputId`, so a coordinate the + * registry does not define is a column nothing can ever populate. Hold an + * added — or repointed — output to the same check enrichment creation + * applies, and leave an untouched existing binding alone so a group whose + * enrichment has since changed stays editable. + */ + if (producerIsEnrichment && input.outputs?.length) { + const boundOutputKeys = new Set( + previousGroup?.outputs.map((output) => `${output.columnName}::${output.outputId ?? ''}`) ?? + [] + ) + const addedOutputs = input.outputs.filter( + (output) => !boundOutputKeys.has(`${output.columnName}::${output.outputId ?? ''}`) + ) + if (addedOutputs.length > 0) { + requireKnownEnrichmentOutputIds( + requireEnrichment(previousGroup?.enrichmentId), + addedOutputs.map((output) => output.outputId) + ) + } + } + /** + * `mappingUpdates` repoints a column at a new `(blockId, path)` — coordinates + * an enrichment output does not have and the writer cannot translate into an + * `outputId`. Say that, rather than resolving the group's empty workflow id + * and answering `404 Workflow not found`. + */ + if (producerIsEnrichment && input.mappingUpdates?.length) { + throw new OrchestrationError( + 'validation', + 'Mapping updates are not supported for an enrichment group; send outputs[] instead' + ) + } /** * A `newOutputColumns` entry that no resulting output names is dropped by the * writer, so a caller asking for a column got a 200 and no column. Refuse it diff --git a/apps/sim/lib/table/views/service.test.ts b/apps/sim/lib/table/views/service.test.ts index 33a1cc5d7e9..a76c568d310 100644 --- a/apps/sim/lib/table/views/service.test.ts +++ b/apps/sim/lib/table/views/service.test.ts @@ -547,6 +547,61 @@ describe('view config column-reference normalization', () => { ).rejects.toMatchObject({ name: 'TableViewValidationError' }) }) + /** + * The carried-forward exemption exists so a dangling FILTER ref stays + * writable, not so it becomes a valid target for a NEW layout ref. Without + * scoping, a strict caller could store `hiddenColumns: ['col_gone']` purely + * because `col_gone` survives in the stored filter — a layout entry the very + * next read drops, which is the asymmetry the strict check closes. + */ + it('refuses a NEW layout reference that resolves only via a carried-forward filter ref', async () => { + const stale = { all: [{ field: 'col_gone', op: 'eq' as const, value: 'x' }] } + queueTableRows(tableViews, [{ ...storedRow, config: { filter: stale } }]) + dbChainMockFns.returning.mockResolvedValueOnce([storedRow]) + + await expect( + updateTableView({ + viewId: 'view-1', + tableId: 'table-1', + config: { filter: stale, hiddenColumns: ['col_gone'] }, + columns, + strictRefs: true, + }) + ).rejects.toMatchObject({ name: 'TableViewValidationError' }) + expect(dbChainMockFns.update).not.toHaveBeenCalled() + }) + + it('still lets a strict save carry the stale filter reference forward with a valid layout', async () => { + const stale = { all: [{ field: 'col_gone', op: 'eq' as const, value: 'x' }] } + queueTableRows(tableViews, [{ ...storedRow, config: { filter: stale } }]) + dbChainMockFns.returning.mockResolvedValueOnce([storedRow]) + + await expect( + updateTableView({ + viewId: 'view-1', + tableId: 'table-1', + config: { filter: stale, hiddenColumns: ['col_a'] }, + columns, + strictRefs: true, + }) + ).resolves.not.toBeNull() + }) + + it('leaves the non-strict grid path free to save that same layout reference', async () => { + const stale = { all: [{ field: 'col_gone', op: 'eq' as const, value: 'x' }] } + queueTableRows(tableViews, [{ ...storedRow, config: { filter: stale } }]) + dbChainMockFns.returning.mockResolvedValueOnce([storedRow]) + + await expect( + updateTableView({ + viewId: 'view-1', + tableId: 'table-1', + config: { filter: stale, hiddenColumns: ['col_gone'] }, + columns, + }) + ).resolves.not.toBeNull() + }) + it('accepts that same new reference from a first-party caller', async () => { const stale = { all: [{ field: 'col_gone', op: 'eq' as const, value: 'x' }] } queueTableRows(tableViews, [{ ...storedRow, config: { filter: stale } }]) diff --git a/apps/sim/lib/table/views/service.ts b/apps/sim/lib/table/views/service.ts index 19e10cb8b1e..68a0ddce1e2 100644 --- a/apps/sim/lib/table/views/service.ts +++ b/apps/sim/lib/table/views/service.ts @@ -200,6 +200,14 @@ function tolerantColumns( * already held, so a reference the caller INTRODUCES is refused; a first-party * caller exempts its own refs too, which is the behavior the grid has always * had — see {@link CreateTableViewData.strictRefs}. + * + * The exemption is scoped to `filter` and `sort`, the only halves it exists for: + * the layout check runs against the LIVE columns, not the tolerant set. A + * carried-forward placeholder standing in for a deleted column named in the + * stored filter must not also make that name a valid target for a new layout + * ref, which `pruneViewConfig` would drop on the very next read — the same + * write-accepts / read-discards asymmetry `strictLayoutRefs` closes. Validating + * against `columns` makes the strict path accept exactly what a read keeps. */ export function normalizeViewConfigForStorage( config: TableViewConfig, @@ -218,7 +226,7 @@ export function normalizeViewConfigForStorage( } throw error } - if (strictLayoutRefs) assertLayoutRefsResolve(stored, known) + if (strictLayoutRefs) assertLayoutRefsResolve(stored, columns) return stored }