From 7be8c7bb6636ff7b662070c34bd758278634adfe Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 13 Aug 2026 12:08:53 -0700 Subject: [PATCH] fix(v2): close the correctness gaps the release audit found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An end-to-end audit of the v0.8.1 release surfaced one regression the release itself introduced and a set of filter/cursor gaps that let a caller's spelling change what a query answered. An `enrichment` workflow group stored no `workflowId`. The public contract invites a caller to omit it, the write persisted caller input through an `as WorkflowGroup` cast that hid the omission from the type checker, and the response schema still required it — so the write committed and then the outbound parse threw. Because the list presenter maps every group through that schema, one such row made GET, PATCH and DELETE on that table's groups fail from then on, with no public way to remove it. The cast is gone rather than papered over, so the same class of omission cannot recur silently. Knowledge tag filters accepted any operator string and then ignored an unrecognized one in opposite directions: the document list dropped the predicate and answered with the whole knowledge base, while search fell through to equality and answered a different question. Both now reject at the boundary. `.strict()` is applied on the v2 chain only — v1 has always stripped unrecognized keys, and the `between`-requires-`valueTo` rule already closes the mis-cased `valueTo` trap on both versions. Cursor scopes bound set-valued filters to the caller's ordering: `all`/`any` clause order and `in`/`nin` operand order in the table predicate, and the raw `resourceType` text on audit logs, whose query splits it into an `inArray`. Audit logs is canonicalized on both sides, because canonicalizing the scope alone would have given two genuinely different result sets one fingerprint. Also: the exposed-header list reached only the fallback CORS policy, so all five matched rules — including the wildcard-origin execute route, the only one that emits `X-Run-Id` — could not hand a browser the run id or a 429's `Retry-After`; a bulk row update reported an uncoercible value only when its filter happened to match; the cost and duration windows accepted an inverted pair and answered it with an empty page; and the sortless runs list advised callers to fix a `sortBy` it rejects. --- apps/docs/openapi-v2-files-audit.json | 4 +- apps/docs/openapi-v2-knowledge.json | 20 ++- apps/docs/openapi-v2-logs.json | 12 +- apps/sim/app/api/v2/audit-logs/route.test.ts | 49 +++++++ apps/sim/app/api/v2/audit-logs/route.ts | 4 +- .../[uploadId]/parts/[partNumber]/route.ts | 7 +- .../app/api/v2/uploads/[uploadId]/route.ts | 7 +- .../api/v2/workflows/[id]/runs/route.test.ts | 31 +++- .../app/api/v2/workflows/[id]/runs/route.ts | 9 +- .../lib/api/contracts/v1/knowledge/index.ts | 62 +++++++- .../contracts/v1/knowledge/tag-filter.test.ts | 62 ++++++++ .../contracts/v2/__tests__/knowledge.test.ts | 67 +++++++++ apps/sim/lib/api/contracts/v2/audit-logs.ts | 2 +- apps/sim/lib/api/contracts/v2/knowledge.ts | 17 ++- .../api/contracts/v2/log-cost-bounds.test.ts | 96 +++++++++++++ apps/sim/lib/api/contracts/v2/logs.ts | 63 ++++++++- apps/sim/lib/api/cursor-binding.ts | 21 ++- apps/sim/lib/audit-logs/query.test.ts | 55 ++++++++ apps/sim/lib/audit-logs/query.ts | 11 +- .../knowledge/tags/filter-resolution.test.ts | 83 +++++++++++ .../lib/knowledge/tags/filter-resolution.ts | 42 +++++- apps/sim/lib/table/application/groups.test.ts | 28 ++++ apps/sim/lib/table/application/groups.ts | 17 ++- .../bulk-update-patch-validation.test.ts | 133 ++++++++++++++++++ apps/sim/lib/table/rows/cursor.test.ts | 114 +++++++++++++++ apps/sim/lib/table/rows/cursor.ts | 39 ++++- apps/sim/lib/table/rows/service.ts | 35 +++++ apps/sim/proxy.test.ts | 37 +++++ apps/sim/proxy.ts | 16 ++- 29 files changed, 1095 insertions(+), 48 deletions(-) create mode 100644 apps/sim/lib/api/contracts/v1/knowledge/tag-filter.test.ts create mode 100644 apps/sim/lib/api/contracts/v2/log-cost-bounds.test.ts create mode 100644 apps/sim/lib/table/rows/__tests__/bulk-update-patch-validation.test.ts diff --git a/apps/docs/openapi-v2-files-audit.json b/apps/docs/openapi-v2-files-audit.json index 3e605aa00e1..e842423e106 100644 --- a/apps/docs/openapi-v2-files-audit.json +++ b/apps/docs/openapi-v2-files-audit.json @@ -1036,9 +1036,9 @@ "name": "resourceType", "in": "query", "required": false, - "description": "Filter by exact resource type.", + "description": "Filter by resource type. Accepts a comma-separated set; members are trimmed and deduplicated, and member order affects neither the result nor the cursor.", "schema": { - "description": "Filter by exact resource type.", + "description": "Filter by resource type. Accepts a comma-separated set; members are trimmed and deduplicated, and member order affects neither the result nor the cursor.", "type": "string" } }, diff --git a/apps/docs/openapi-v2-knowledge.json b/apps/docs/openapi-v2-knowledge.json index e0d93148561..9b598af8ae0 100644 --- a/apps/docs/openapi-v2-knowledge.json +++ b/apps/docs/openapi-v2-knowledge.json @@ -2743,9 +2743,22 @@ }, "operator": { "default": "eq", - "description": "Comparison operator; valid operators depend on the field type.", + "description": "Comparison operator; valid operators depend on the field type. Text tags accept eq, neq, contains, not_contains, starts_with, ends_with; number and date tags accept eq, neq, gt, gte, lt, lte, between; boolean tags accept eq, neq. An operator the tag's field type does not implement is rejected, never ignored.", "examples": ["eq"], - "type": "string" + "type": "string", + "enum": [ + "eq", + "neq", + "contains", + "not_contains", + "starts_with", + "ends_with", + "gt", + "gte", + "lt", + "lte", + "between" + ] }, "value": { "anyOf": [ @@ -2763,7 +2776,7 @@ "examples": ["billing"] }, "valueTo": { - "description": "Upper bound for the `between` operator.", + "description": "Upper bound for the `between` operator, and required whenever that operator is used.", "anyOf": [ { "type": "string" @@ -2775,6 +2788,7 @@ } }, "required": ["tagName", "value"], + "additionalProperties": false, "title": "Knowledge search tag filter", "description": "A structured tag filter applied to knowledge search." }, diff --git a/apps/docs/openapi-v2-logs.json b/apps/docs/openapi-v2-logs.json index eaf97c33c96..404a537a28b 100644 --- a/apps/docs/openapi-v2-logs.json +++ b/apps/docs/openapi-v2-logs.json @@ -133,20 +133,24 @@ "name": "minCost", "in": "query", "required": false, - "description": "Minimum execution cost in USD.", + "description": "Minimum execution cost in USD, from 0 to 1000000. A run is never charged a negative amount, so a negative bound is rejected rather than treated as a filter that matches every run.", "schema": { "type": "number", - "description": "Minimum execution cost in USD." + "minimum": 0, + "maximum": 1000000, + "description": "Minimum execution cost in USD, from 0 to 1000000. A run is never charged a negative amount, so a negative bound is rejected rather than treated as a filter that matches every run." } }, { "name": "maxCost", "in": "query", "required": false, - "description": "Maximum execution cost in USD.", + "description": "Maximum execution cost in USD, from 0 to 1000000. A run is never charged a negative amount, so a negative bound is rejected rather than treated as a filter that matches every run.", "schema": { "type": "number", - "description": "Maximum execution cost in USD." + "minimum": 0, + "maximum": 1000000, + "description": "Maximum execution cost in USD, from 0 to 1000000. A run is never charged a negative amount, so a negative bound is rejected rather than treated as a filter that matches every run." } }, { diff --git a/apps/sim/app/api/v2/audit-logs/route.test.ts b/apps/sim/app/api/v2/audit-logs/route.test.ts index b77f30edc06..59f7e6107bc 100644 --- a/apps/sim/app/api/v2/audit-logs/route.test.ts +++ b/apps/sim/app/api/v2/audit-logs/route.test.ts @@ -179,6 +179,55 @@ describe('v2 audit-log routes', () => { }) }) + /** + * `resourceType` is split into an `inArray` downstream, so its spelling is a + * set the query acts on rather than the exact string the caller sent. The + * cursor must bind the members, not the text. + */ + it.each([ + ['reordered', 'workflow,file'], + ['respaced', 'file,%20workflow'], + ['repeated', 'file,workflow,file'], + ])('resumes a cursor whose resourceType set is %s', async (_label, respelled) => { + const minted = await listLogs( + new NextRequest( + 'http://localhost:3000/api/v2/audit-logs?organizationId=org-1&resourceType=file,workflow' + ) + ) + const { nextCursor } = await minted.json() + expect(nextCursor).toEqual(expect.any(String)) + + mocks.list.mockClear() + const resumed = await listLogs( + new NextRequest( + `http://localhost:3000/api/v2/audit-logs?organizationId=org-1&resourceType=${respelled}&cursor=${encodeURIComponent(nextCursor)}` + ) + ) + + expect(resumed.status).toBe(200) + expect(mocks.list).toHaveBeenCalled() + }) + + it('still refuses a cursor replayed under a different resourceType set', async () => { + const minted = await listLogs( + new NextRequest( + 'http://localhost:3000/api/v2/audit-logs?organizationId=org-1&resourceType=file,workflow' + ) + ) + const { nextCursor } = await minted.json() + + mocks.list.mockClear() + const replayed = await listLogs( + new NextRequest( + `http://localhost:3000/api/v2/audit-logs?organizationId=org-1&resourceType=file,knowledge&cursor=${encodeURIComponent(nextCursor)}` + ) + ) + + expect(replayed.status).toBe(400) + expect((await replayed.json()).error.message).toBe(REFILTERED_CURSOR_MESSAGE) + expect(mocks.list).not.toHaveBeenCalled() + }) + it('projects typed admin-policy failures without leaking internals', async () => { mocks.list.mockRejectedValueOnce(new OrchestrationError('forbidden', 'Admin required')) diff --git a/apps/sim/app/api/v2/audit-logs/route.ts b/apps/sim/app/api/v2/audit-logs/route.ts index 53022dff0d0..d69532e01ae 100644 --- a/apps/sim/app/api/v2/audit-logs/route.ts +++ b/apps/sim/app/api/v2/audit-logs/route.ts @@ -1,5 +1,5 @@ import { v2ListAuditLogsContract } from '@/lib/api/contracts/v2/audit-logs' -import { cursorScopeKey, instantScopePart } from '@/lib/api/cursor-binding' +import { cursorScopeKey, instantScopePart, unorderedScopePart } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, @@ -27,7 +27,7 @@ function auditLogCursorFilters(query: { organizationId: query.organizationId, includeDeparted: query.includeDeparted, action: query.action, - resourceType: query.resourceType, + resourceType: unorderedScopePart(query.resourceType), resourceId: query.resourceId, workspaceId: query.workspaceId, actorEmail: query.actorEmail, diff --git a/apps/sim/app/api/v2/uploads/[uploadId]/parts/[partNumber]/route.ts b/apps/sim/app/api/v2/uploads/[uploadId]/parts/[partNumber]/route.ts index 91f03f278b7..dca212587dc 100644 --- a/apps/sim/app/api/v2/uploads/[uploadId]/parts/[partNumber]/route.ts +++ b/apps/sim/app/api/v2/uploads/[uploadId]/parts/[partNumber]/route.ts @@ -12,12 +12,7 @@ import { type UploadSessionRecord, verifyUploadSessionToken, } from '@/lib/uploads/upload-session/service' -import { - v2Error, - v2HttpError, - v2UploadDataPlaneError, - v2ValidationError, -} from '@/app/api/v2/lib/response' +import { v2Error, v2HttpError, v2UploadDataPlaneError } from '@/app/api/v2/lib/response' interface LocalPartRouteParams { params: Promise<{ uploadId: string; partNumber: string }> diff --git a/apps/sim/app/api/v2/uploads/[uploadId]/route.ts b/apps/sim/app/api/v2/uploads/[uploadId]/route.ts index d53034de60f..58ba867229b 100644 --- a/apps/sim/app/api/v2/uploads/[uploadId]/route.ts +++ b/apps/sim/app/api/v2/uploads/[uploadId]/route.ts @@ -8,12 +8,7 @@ import { getOwnedUploadSession, uploadSessionObjectMetadata, } from '@/lib/uploads/upload-session/service' -import { - v2Error, - v2HttpError, - v2UploadDataPlaneError, - v2ValidationError, -} from '@/app/api/v2/lib/response' +import { v2Error, v2HttpError, v2UploadDataPlaneError } from '@/app/api/v2/lib/response' interface LocalPutRouteParams { params: Promise<{ uploadId: string }> diff --git a/apps/sim/app/api/v2/workflows/[id]/runs/route.test.ts b/apps/sim/app/api/v2/workflows/[id]/runs/route.test.ts index eb11ee3c02e..0d01548f713 100644 --- a/apps/sim/app/api/v2/workflows/[id]/runs/route.test.ts +++ b/apps/sim/app/api/v2/workflows/[id]/runs/route.test.ts @@ -28,7 +28,7 @@ vi.mock('@/lib/workflows/application/list-workflow-runs', () => ({ }, })) -import { REFILTERED_CURSOR_MESSAGE } from '@/lib/api/cursor-binding' +import { REFILTERED_CURSOR_MESSAGE, UNREADABLE_CURSOR_MESSAGE } from '@/lib/api/cursor-binding' import { NoWorkspaceAccessError, PersonalApiKeysDisabledError } from '@/lib/core/application' import { GET } from '@/app/api/v2/workflows/[id]/runs/route' @@ -170,6 +170,35 @@ describe('GET /api/v2/workflows/[id]/runs', () => { expect(mocks.listRuns).not.toHaveBeenCalled() }) + /** + * This list orders by the single `order` param — its query schema is + * `.strict()` and declares no `sortBy` — so the sort-mismatch wording would + * answer one 400 with advice that earns a second. + */ + it('names a cursor with unusable keys unreadable rather than blaming sortBy', async () => { + mocks.listRuns.mockResolvedValueOnce({ + data: EXECUTIONS, + nextCursor: { startedAt: EXECUTIONS[1].startedAt, rowId: 'row-1' }, + workflowId: 'workflow-1', + order: 'desc', + }) + + const { nextCursor } = await (await callGet()).json() + const payload = JSON.parse(Buffer.from(nextCursor, 'base64').toString()) + const tampered = Buffer.from( + JSON.stringify({ ...payload, keys: ['not-a-date', 'row-1'] }) + ).toString('base64') + + mocks.listRuns.mockClear() + const response = await callGet(`?cursor=${encodeURIComponent(tampered)}`) + + expect(response.status).toBe(400) + const { error } = await response.json() + expect(error.message).toBe(UNREADABLE_CURSOR_MESSAGE) + expect(error.message).not.toMatch(/sortBy/) + expect(mocks.listRuns).not.toHaveBeenCalled() + }) + it('rejects an invalid cursor after API-key admission without calling the use case', async () => { const response = await callGet('?cursor=not-a-cursor') 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 aa1528ef178..b76eb2e88f5 100644 --- a/apps/sim/app/api/v2/workflows/[id]/runs/route.ts +++ b/apps/sim/app/api/v2/workflows/[id]/runs/route.ts @@ -3,8 +3,11 @@ import { v2ListWorkflowRunsContract, v2WorkflowRunListStatusValueSchema, } from '@/lib/api/contracts/v2/workflows' -import { cursorScopeKey, instantScopePart } from '@/lib/api/cursor-binding' -import { INVALID_CURSOR_MESSAGE } from '@/lib/api/list-query' +import { + cursorScopeKey, + instantScopePart, + UNREADABLE_CURSOR_MESSAGE, +} from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, v2RateLimits } from '@/lib/api/server/routes' import { OrchestrationError } from '@/lib/core/orchestration/types' import { v2WorkflowErrorPolicies } from '@/lib/workflows/api' @@ -53,7 +56,7 @@ export const GET = defineV2JsonRoute({ Number.isNaN(cursorDate.getTime()) || typeof cursorRowId !== 'string') ) { - throw new OrchestrationError('validation', INVALID_CURSOR_MESSAGE) + throw new OrchestrationError('validation', UNREADABLE_CURSOR_MESSAGE) } return { diff --git a/apps/sim/lib/api/contracts/v1/knowledge/index.ts b/apps/sim/lib/api/contracts/v1/knowledge/index.ts index 926786090b4..2b13594b96a 100644 --- a/apps/sim/lib/api/contracts/v1/knowledge/index.ts +++ b/apps/sim/lib/api/contracts/v1/knowledge/index.ts @@ -106,18 +106,66 @@ export const v1ListKnowledgeDocumentsQuerySchema = z.object({ sortOrder: z.enum(['asc', 'desc']).default('desc'), }) +/** + * Comparison operators the tag filter builders implement, per field type. + * + * Both builders — `lib/knowledge/documents/tag-filter.ts` for document lists and + * `lib/knowledge/search/queries.ts` for search — recognize exactly these. An + * operator outside the set used to be admitted by the boundary and then diverge: + * the document list dropped the predicate and returned the whole knowledge base, + * while search fell through to equality. Enumerating them here makes both + * `default:` arms unreachable instead of load-bearing. + */ +export const KNOWLEDGE_TAG_FILTER_OPERATORS_BY_FIELD_TYPE = { + text: ['eq', 'neq', 'contains', 'not_contains', 'starts_with', 'ends_with'], + number: ['eq', 'neq', 'gt', 'gte', 'lt', 'lte', 'between'], + date: ['eq', 'neq', 'gt', 'gte', 'lt', 'lte', 'between'], + boolean: ['eq', 'neq'], +} as const satisfies Record<'text' | 'number' | 'date' | 'boolean', readonly string[]> + +/** Every operator any field type implements; the field type narrows it further. */ +export const KNOWLEDGE_TAG_FILTER_OPERATORS = [ + 'eq', + 'neq', + 'contains', + 'not_contains', + 'starts_with', + 'ends_with', + 'gt', + 'gte', + 'lt', + 'lte', + 'between', +] as const + /** * POST `/api/v1/knowledge/search` tag filter — uses display `tagName` (not * slot) and a default operator. Distinct from the in-app * `documentTagFilterSchema`, which is slot-based and used for list filtering. + * + * Deliberately not strict. v1 has always stripped unrecognized keys, and + * rejecting them now would break shipped callers for no correctness win; v2 + * layers `.strict()` on top of this schema instead. The one stripped key that + * changed an answer — a mis-cased `valueto` leaving a `between` filter with no + * upper bound, which the document list then discarded entirely — is caught on + * both versions by the `valueTo` rule below. */ -export const v1SearchTagFilterSchema = z.object({ - tagName: z.string(), - fieldType: z.enum(['text', 'number', 'date', 'boolean']).optional(), - operator: z.string().default('eq'), - value: z.union([z.string(), z.number(), z.boolean()]), - valueTo: z.union([z.string(), z.number()]).optional(), -}) +export const v1SearchTagFilterSchema = z + .object({ + tagName: z.string(), + fieldType: z.enum(['text', 'number', 'date', 'boolean']).optional(), + operator: z + .enum(KNOWLEDGE_TAG_FILTER_OPERATORS, { + error: `operator must be one of: ${KNOWLEDGE_TAG_FILTER_OPERATORS.join(', ')}`, + }) + .default('eq'), + value: z.union([z.string(), z.number(), z.boolean()]), + valueTo: z.union([z.string(), z.number()]).optional(), + }) + .refine((filter) => filter.operator !== 'between' || filter.valueTo !== undefined, { + message: 'valueTo is required when operator is "between"', + path: ['valueTo'], + }) /** POST `/api/v1/knowledge/search` body. */ export const v1KnowledgeSearchBodySchema = z diff --git a/apps/sim/lib/api/contracts/v1/knowledge/tag-filter.test.ts b/apps/sim/lib/api/contracts/v1/knowledge/tag-filter.test.ts new file mode 100644 index 00000000000..227afd9a8cc --- /dev/null +++ b/apps/sim/lib/api/contracts/v1/knowledge/tag-filter.test.ts @@ -0,0 +1,62 @@ +import { describe, expect, it } from 'vitest' +import { v1SearchTagFilterSchema } from '@/lib/api/contracts/v1/knowledge' + +/** + * v1 is a shipped public API, so this schema stays lenient about keys it does + * not declare — that is deliberate, not an oversight, and the v2 element layers + * `.strict()` on top of it instead. What v1 does share with v2 is the operator + * enum and the `between` rule: an unrecognized operator used to reach the search + * query builder and be coerced to equality, silently answering a different + * question than the caller asked. + */ +describe('v1 knowledge search tag filter', () => { + it('still strips an unrecognized key rather than rejecting the request', () => { + const parsed = v1SearchTagFilterSchema.safeParse({ + tagName: 'category', + operator: 'eq', + value: 'billing', + unknownKey: 'ignored', + }) + + expect(parsed.success).toBe(true) + expect(parsed.data).toEqual({ tagName: 'category', operator: 'eq', value: 'billing' }) + }) + + it('rejects an operator no field type implements', () => { + expect( + v1SearchTagFilterSchema.safeParse({ + tagName: 'category', + operator: 'nosuchop', + value: 'billing', + }).success + ).toBe(false) + }) + + it('rejects "between" with no valueTo, including the mis-cased key that strips to it', () => { + expect( + v1SearchTagFilterSchema.safeParse({ tagName: 'score', operator: 'between', value: 1 }).success + ).toBe(false) + expect( + v1SearchTagFilterSchema.safeParse({ + tagName: 'score', + operator: 'between', + value: 1, + valueto: 5, + }).success + ).toBe(false) + }) + + it('accepts a bounded between filter and defaults a missing operator to eq', () => { + expect( + v1SearchTagFilterSchema.safeParse({ + tagName: 'score', + operator: 'between', + value: 1, + valueTo: 5, + }).success + ).toBe(true) + expect( + v1SearchTagFilterSchema.safeParse({ tagName: 'category', value: 'billing' }).data + ).toEqual({ tagName: 'category', operator: 'eq', value: 'billing' }) + }) +}) diff --git a/apps/sim/lib/api/contracts/v2/__tests__/knowledge.test.ts b/apps/sim/lib/api/contracts/v2/__tests__/knowledge.test.ts index a66b13d857c..6392696edb3 100644 --- a/apps/sim/lib/api/contracts/v2/__tests__/knowledge.test.ts +++ b/apps/sim/lib/api/contracts/v2/__tests__/knowledge.test.ts @@ -76,6 +76,73 @@ describe('v2 knowledge contracts', () => { }) }) +function parseTagFilter(filter: unknown) { + return v2SearchKnowledgeContract.body?.safeParse({ + workspaceId: 'workspace-1', + knowledgeBaseIds: ['kb-1'], + query: 'support', + tagFilters: [filter], + }) +} + +/** + * An operator the builders do not implement used to be admitted here and then + * diverge downstream: the document list dropped the predicate and answered with + * the whole knowledge base, while search fell through to equality. The same trap + * the search body closed with `.strict()` was left open in the array element, so + * a mis-cased `valueto` was stripped and its `between` filter lost its bound. + */ +describe('v2 knowledge search tag filter', () => { + it('rejects an operator no field type implements', () => { + expect( + parseTagFilter({ tagName: 'category', operator: 'nosuchop', value: 'billing' })?.success + ).toBe(false) + }) + + it('rejects "between" with no valueTo', () => { + const parsed = parseTagFilter({ tagName: 'score', operator: 'between', value: 1 }) + expect(parsed?.success).toBe(false) + expect(issueMessages(parsed as never)).toContain( + 'valueTo is required when operator is "between"' + ) + }) + + it('rejects a mis-cased valueTo instead of stripping it', () => { + expect( + parseTagFilter({ tagName: 'score', operator: 'between', value: 1, valueto: 5 })?.success + ).toBe(false) + }) + + it.each([ + 'eq', + 'neq', + 'contains', + 'not_contains', + 'starts_with', + 'ends_with', + 'gt', + 'gte', + 'lt', + 'lte', + ])('still accepts the implemented operator %s', (operator) => { + expect(parseTagFilter({ tagName: 'category', operator, value: 'billing' })?.success).toBe(true) + }) + + it('still accepts a bounded between filter', () => { + expect( + parseTagFilter({ tagName: 'score', operator: 'between', value: 1, valueTo: 5 })?.success + ).toBe(true) + }) + + it('still defaults a filter that names no operator', () => { + const parsed = parseTagFilter({ tagName: 'category', value: 'billing' }) + expect(parsed?.success).toBe(true) + expect( + (parsed as { data: { tagFilters: { operator: string }[] } }).data.tagFilters[0].operator + ).toBe('eq') + }) +}) + /** * The document list inherited `limit` and `search` from the v1 shape, so it was * the one v2 list whose bounds and messages diverged from every sibling. An diff --git a/apps/sim/lib/api/contracts/v2/audit-logs.ts b/apps/sim/lib/api/contracts/v2/audit-logs.ts index 916688c0c32..ebca7ba3915 100644 --- a/apps/sim/lib/api/contracts/v2/audit-logs.ts +++ b/apps/sim/lib/api/contracts/v2/audit-logs.ts @@ -78,7 +78,7 @@ export const v2ListAuditLogsQuerySchema = v1ListAuditLogsQuerySchema .extend({ action: v1ListAuditLogsQuerySchema.shape.action.describe('Filter by exact action name.'), resourceType: v1ListAuditLogsQuerySchema.shape.resourceType.describe( - 'Filter by exact resource type.' + 'Filter by resource type. Accepts a comma-separated set; members are trimmed and deduplicated, and member order affects neither the result nor the cursor.' ), resourceId: v1ListAuditLogsQuerySchema.shape.resourceId.describe( 'Filter by exact resource identifier.' diff --git a/apps/sim/lib/api/contracts/v2/knowledge.ts b/apps/sim/lib/api/contracts/v2/knowledge.ts index 85d8f794eaa..44a7c723f1a 100644 --- a/apps/sim/lib/api/contracts/v2/knowledge.ts +++ b/apps/sim/lib/api/contracts/v2/knowledge.ts @@ -9,6 +9,7 @@ import { import { noInputSchema, workspaceIdSchema } from '@/lib/api/contracts/primitives' import { defineRouteContract } from '@/lib/api/contracts/types' import { + KNOWLEDGE_TAG_FILTER_OPERATORS_BY_FIELD_TYPE, v1ChunkingConfigSchema, v1CreateKnowledgeBaseBodySchema, v1KnowledgeSearchBodySchema, @@ -789,21 +790,31 @@ export const v2DeleteKnowledgeFolderContract = defineRouteContract({ }) export const v2KnowledgeSearchTagFilterSchema = v1SearchTagFilterSchema - .extend({ + /** `safeExtend` so the base schema's `between` rule survives the redescribe. */ + .safeExtend({ tagName: v1SearchTagFilterSchema.shape.tagName .describe('Display name of the tag to filter.') .meta({ examples: ['category'] }), fieldType: v1SearchTagFilterSchema.shape.fieldType.describe('Tag field type.'), operator: v1SearchTagFilterSchema.shape.operator - .describe('Comparison operator; valid operators depend on the field type.') + .describe( + `Comparison operator; valid operators depend on the field type. Text tags accept ${KNOWLEDGE_TAG_FILTER_OPERATORS_BY_FIELD_TYPE.text.join(', ')}; number and date tags accept ${KNOWLEDGE_TAG_FILTER_OPERATORS_BY_FIELD_TYPE.number.join(', ')}; boolean tags accept ${KNOWLEDGE_TAG_FILTER_OPERATORS_BY_FIELD_TYPE.boolean.join(', ')}. An operator the tag's field type does not implement is rejected, never ignored.` + ) .meta({ examples: ['eq'] }), value: v1SearchTagFilterSchema.shape.value .describe('Tag value to compare against.') .meta({ examples: ['billing'] }), valueTo: v1SearchTagFilterSchema.shape.valueTo.describe( - 'Upper bound for the `between` operator.' + 'Upper bound for the `between` operator, and required whenever that operator is used.' ), }) + /** + * Strict for the same reason the search body is: Zod strips what it does not + * declare, so a mis-cased `valueto` left a `between` filter with no upper + * bound and the document list answered 200 with the whole knowledge base. v1 + * keeps its historical lenient parse. + */ + .strict() .meta({ id: 'V2KnowledgeSearchTagFilter', title: 'Knowledge search tag filter', diff --git a/apps/sim/lib/api/contracts/v2/log-cost-bounds.test.ts b/apps/sim/lib/api/contracts/v2/log-cost-bounds.test.ts new file mode 100644 index 00000000000..268b7976702 --- /dev/null +++ b/apps/sim/lib/api/contracts/v2/log-cost-bounds.test.ts @@ -0,0 +1,96 @@ +import { describe, expect, it } from 'vitest' +import { v2ListLogsQuerySchema } from '@/lib/api/contracts/v2/logs' + +const WORKSPACE_ID = '6fc7631d-88cd-46f8-9f0a-d4764daef7f8' + +function parseQuery(query: Record) { + return v2ListLogsQuerySchema.safeParse({ workspaceId: WORKSPACE_ID, ...query }) +} + +/** + * The cost window is the one filter pair on this read that answered a caller + * mistake with an empty page: `minCost` above `maxCost` selects no run, and a + * negative bound selects every run, so both reported a result rather than the + * error the date window already reports for the same shape of mistake. + */ +describe('v2 logs cost bounds', () => { + it('accepts fractional costs inside the published range', () => { + const parsed = parseQuery({ minCost: '0.0001', maxCost: '12.5' }) + + expect(parsed.success).toBe(true) + expect(parsed.success && parsed.data.minCost).toBe(0.0001) + expect(parsed.success && parsed.data.maxCost).toBe(12.5) + }) + + it.each([ + ['minCost', '-1'], + ['maxCost', '-1'], + ['minCost', '-1e308'], + ])('rejects a negative %s (%s)', (field, value) => { + const parsed = parseQuery({ [field]: value }) + + expect(parsed.success).toBe(false) + expect(parsed.error?.issues[0]?.message).toBe(`${field} must not be negative`) + expect(parsed.error?.issues[0]?.path).toEqual([field]) + }) + + it.each([ + ['minCost', '1000001'], + ['maxCost', '1e30'], + ])('rejects an out-of-range %s (%s)', (field, value) => { + const parsed = parseQuery({ [field]: value }) + + expect(parsed.success).toBe(false) + expect(parsed.error?.issues[0]?.message).toBe(`${field} must be at most 1000000`) + }) + + it('rejects an inverted cost window rather than answering it with no runs', () => { + const parsed = parseQuery({ minCost: '100', maxCost: '1' }) + + expect(parsed.success).toBe(false) + expect(parsed.error?.issues[0]?.message).toBe('minCost must be less than or equal to maxCost') + expect(parsed.error?.issues[0]?.path).toEqual(['minCost']) + }) + + it('accepts an equal cost window', () => { + expect(parseQuery({ minCost: '1', maxCost: '1' }).success).toBe(true) + }) + + it('leaves a one-sided cost window alone', () => { + expect(parseQuery({ minCost: '100' }).success).toBe(true) + expect(parseQuery({ maxCost: '1' }).success).toBe(true) + }) + + /** + * The duration window carries its own bounds but shared the cost window's + * missing inversion check, so an inverted pair reported "those runs do not + * exist" for what is a caller mistake. + */ + it('rejects an inverted duration window rather than answering it with no runs', () => { + const parsed = parseQuery({ minDurationMs: '100', maxDurationMs: '1' }) + + expect(parsed.success).toBe(false) + expect(parsed.error?.issues[0]?.message).toBe( + 'minDurationMs must be less than or equal to maxDurationMs' + ) + expect(parsed.error?.issues[0]?.path).toEqual(['minDurationMs']) + }) + + it('accepts an equal duration window and leaves a one-sided one alone', () => { + expect(parseQuery({ minDurationMs: '5', maxDurationMs: '5' }).success).toBe(true) + expect(parseQuery({ minDurationMs: '100' }).success).toBe(true) + expect(parseQuery({ maxDurationMs: '1' }).success).toBe(true) + }) + + it('reports both inverted windows when a caller sends both', () => { + const parsed = parseQuery({ + minCost: '100', + maxCost: '1', + minDurationMs: '100', + maxDurationMs: '1', + }) + + expect(parsed.success).toBe(false) + expect(parsed.error?.issues.map((issue) => issue.path[0])).toEqual(['minCost', 'minDurationMs']) + }) +}) diff --git a/apps/sim/lib/api/contracts/v2/logs.ts b/apps/sim/lib/api/contracts/v2/logs.ts index 58683c9e8c8..19ca8d358ea 100644 --- a/apps/sim/lib/api/contracts/v2/logs.ts +++ b/apps/sim/lib/api/contracts/v2/logs.ts @@ -238,6 +238,35 @@ function v2DurationBoundSchema( ) } +/** + * Largest run cost, in USD, a caller may bound the search by. + * + * `cost_total` is an unconstrained `numeric`, so unlike the duration bounds + * there is no storage limit to borrow; this is a policy ceiling set far above + * any cost a single run can accrue. A bound past it cannot select anything the + * caller could not select with a smaller one, so it is a mistyped value rather + * than a filter. + */ +const V2_COST_USD_MAX = 1_000_000 + +/** + * A cost bound, in the range its column can hold. + * + * Fractional values are kept — a run costs fractions of a cent — but a negative + * bound is rejected for the same reason a negative duration is: `cost_total` is + * never below zero, so `minCost=-1` is not a filter that matches everything, it + * is a caller mistake reported as a full result set. + */ +function v2CostBoundSchema(field: 'minCost' | 'maxCost', bound: 'Minimum' | 'Maximum') { + return z.coerce + .number() + .min(0, `${field} must not be negative`) + .max(V2_COST_USD_MAX, `${field} must be at most ${V2_COST_USD_MAX}`) + .describe( + `${bound} execution cost in USD, from 0 to ${V2_COST_USD_MAX}. A run is never charged a negative amount, so a negative bound is rejected rather than treated as a filter that matches every run.` + ) +} + /** * A comma-separated filter list, with an empty entry rejected rather than dropped. * @@ -275,8 +304,8 @@ export const v2ListLogsQuerySchema = v1ListLogsQuerySchema runId: runIdSchema.describe('Exact run identifier to match.').optional(), minDurationMs: v2DurationBoundSchema('minDurationMs', 'Minimum').optional(), maxDurationMs: v2DurationBoundSchema('maxDurationMs', 'Maximum').optional(), - minCost: z.coerce.number().describe('Minimum execution cost in USD.').optional(), - maxCost: z.coerce.number().describe('Maximum execution cost in USD.').optional(), + minCost: v2CostBoundSchema('minCost', 'Minimum').optional(), + maxCost: v2CostBoundSchema('maxCost', 'Maximum').optional(), model: z.string().describe('AI model used during execution.').optional(), details: z .enum(['basic', 'full']) @@ -358,6 +387,36 @@ export const v2ListLogsQuerySchema = v1ListLogsQuerySchema path: ['startDate'], } ) + /** + * The cost and duration windows get the same treatment as the date window, + * for the same reason: an inverted pair can never match a run, so answering + * it with an empty page reports "those runs do not exist" for what is a + * caller mistake. + */ + .superRefine((query, ctx) => { + if ( + query.minCost !== undefined && + query.maxCost !== undefined && + query.minCost > query.maxCost + ) { + ctx.addIssue({ + code: 'custom', + message: 'minCost must be less than or equal to maxCost', + path: ['minCost'], + }) + } + if ( + query.minDurationMs !== undefined && + query.maxDurationMs !== undefined && + query.minDurationMs > query.maxDurationMs + ) { + ctx.addIssue({ + code: 'custom', + message: 'minDurationMs must be less than or equal to maxDurationMs', + path: ['minDurationMs'], + }) + } + }) export const v2ListLogsContract = defineRouteContract({ method: 'GET', diff --git a/apps/sim/lib/api/cursor-binding.ts b/apps/sim/lib/api/cursor-binding.ts index c093ef0b68c..95fc95b98eb 100644 --- a/apps/sim/lib/api/cursor-binding.ts +++ b/apps/sim/lib/api/cursor-binding.ts @@ -86,7 +86,26 @@ export function parseUnorderedList(raw: string | undefined): string[] | undefine export function unorderedScopeOf(value: unknown): string | undefined { if (value === undefined) return undefined if (!Array.isArray(value)) return canonicalJson(value) - return `[${[...new Set(value.map(canonicalJson))].sort().join(',')}]` + return canonicalUnorderedArray(value) +} + +/** + * Canonical form of an array whose member ORDER does not change the result. + * + * The one place the set rule lives, so every position that compiles to `and`, + * `or`, or `IN (...)` binds the same way. Members are canonicalized first, then + * de-duplicated and sorted, because `A AND A` selects what `A` does. + * + * `canonicalizeMember` exists for nested set-valued shapes — a predicate tree + * whose groups contain groups needs the rule applied at every level, not just + * the outermost array. It defaults to {@link canonicalJson}, which is correct + * for a flat array of scalars. + */ +export function canonicalUnorderedArray( + members: readonly unknown[], + canonicalizeMember: (member: unknown) => string = canonicalJson +): string { + return `[${[...new Set(members.map(canonicalizeMember))].sort().join(',')}]` } /** diff --git a/apps/sim/lib/audit-logs/query.test.ts b/apps/sim/lib/audit-logs/query.test.ts index 08105b6f908..dd454b31179 100644 --- a/apps/sim/lib/audit-logs/query.test.ts +++ b/apps/sim/lib/audit-logs/query.test.ts @@ -7,7 +7,9 @@ */ import { dbChainMockFns } from '@sim/testing' import { beforeEach, describe, expect, it, vi } from 'vitest' +import { unorderedScopePart } from '@/lib/api/cursor-binding' import { + buildFilterConditions, buildOrgScopeCondition, decodeAuditLogCursor, getOrgWorkspaceIds, @@ -172,6 +174,59 @@ describe('getOrgWorkspaceIds', () => { }) }) +/** + * `resourceType` is a comma-separated set, and the v2 cursor scope fingerprints + * the same list. The query and the scope must agree on the members, or either + * two different result sets share one stamp or two spellings of one result set + * get different stamps and page 2 is refused. + */ +describe('buildFilterConditions resourceType', () => { + function resourceTypeCondition(resourceType: string): MockCondition { + const conditions = buildFilterConditions({ resourceType }) + expect(conditions).toHaveLength(1) + return asCondition(conditions[0]) + } + + it('trims members so a spaced list filters on the types it names', () => { + expect(resourceTypeCondition('file, workflow')).toMatchObject({ + type: 'inArray', + column: 'resourceType', + values: ['file', 'workflow'], + }) + }) + + it('filters identically however the caller orders, spaces, or repeats members', () => { + const canonical = resourceTypeCondition('file,workflow') + for (const spelling of ['workflow,file', 'file, workflow', ' workflow ,file,file']) { + expect(resourceTypeCondition(spelling)).toEqual(canonical) + } + }) + + it('agrees with the cursor scope on the member list', () => { + for (const spelling of ['file,workflow', 'workflow, file', 'file,workflow,file']) { + expect(asCondition(buildFilterConditions({ resourceType: spelling })[0]).values).toEqual( + unorderedScopePart(spelling)!.split(',') + ) + expect(unorderedScopePart(spelling)).toBe(unorderedScopePart('file,workflow')) + } + }) + + it('still collapses a single member to an equality check', () => { + expect(resourceTypeCondition(' workflow ')).toMatchObject({ + type: 'eq', + left: 'resourceType', + right: 'workflow', + }) + }) + + it('keeps genuinely different type sets apart', () => { + expect(resourceTypeCondition('file,workflow')).not.toEqual( + resourceTypeCondition('file,knowledge') + ) + expect(unorderedScopePart('file,workflow')).not.toBe(unorderedScopePart('file,knowledge')) + }) +}) + describe('decodeAuditLogCursor', () => { it('accepts the exact timestamp and ID cursor shape', () => { const cursor = Buffer.from( diff --git a/apps/sim/lib/audit-logs/query.ts b/apps/sim/lib/audit-logs/query.ts index 586d2e43d1a..b55605982d5 100644 --- a/apps/sim/lib/audit-logs/query.ts +++ b/apps/sim/lib/audit-logs/query.ts @@ -3,6 +3,7 @@ import { db, dbReplica } from '@sim/db' import { auditLog, workspace } from '@sim/db/schema' import type { InferSelectModel } from 'drizzle-orm' import { and, desc, eq, gte, ilike, inArray, isNull, lt, lte, or, type SQL, sql } from 'drizzle-orm' +import { parseUnorderedList } from '@/lib/api/cursor-binding' type DbAuditLog = InferSelectModel @@ -46,12 +47,20 @@ export interface AuditLogFilterParams { endDate?: string } +/** + * Compiles the caller-supplied filters into SQL conditions. + * + * `resourceType` is a comma-separated set, parsed through + * {@link parseUnorderedList} — the same parse the v2 cursor scope fingerprints + * through. Splitting it here independently is what let `file,workflow` and + * `file, workflow` mean one thing to the query and another to the cursor. + */ export function buildFilterConditions(params: AuditLogFilterParams): SQL[] { const conditions: SQL[] = [] if (params.action) conditions.push(eq(auditLog.action, params.action)) if (params.resourceType) { - const types = params.resourceType.split(',').filter(Boolean) + const types = parseUnorderedList(params.resourceType) ?? [] if (types.length === 1) conditions.push(eq(auditLog.resourceType, types[0])) else if (types.length > 1) conditions.push(inArray(auditLog.resourceType, types)) } diff --git a/apps/sim/lib/knowledge/tags/filter-resolution.test.ts b/apps/sim/lib/knowledge/tags/filter-resolution.test.ts index d0cb6f5c8e2..c1a4322cf57 100644 --- a/apps/sim/lib/knowledge/tags/filter-resolution.test.ts +++ b/apps/sim/lib/knowledge/tags/filter-resolution.test.ts @@ -90,6 +90,89 @@ describe('resolveKnowledgeTagFilters', () => { ) ).rejects.toThrow('is not mapped consistently') }) + + it('rejects an operator the resolved field type does not implement', async () => { + mockGetDocumentTagDefinitions.mockResolvedValue([definition('kb-1', 'tag1', 'category')]) + + await expect( + resolveKnowledgeTagFilters( + [{ tagName: 'category', operator: 'gt', value: 'billing' }], + ['kb-1'] + ) + ).rejects.toThrow( + 'Tag "category" is a text tag and does not support operator "gt". Supported operators: eq, neq, contains, not_contains, starts_with, ends_with' + ) + }) + + it('rejects an operator no field type implements rather than passing it through', async () => { + mockGetDocumentTagDefinitions.mockResolvedValue([definition('kb-1', 'tag1', 'category')]) + + await expect( + resolveKnowledgeTagFilters( + [{ tagName: 'category', operator: 'nosuchop', value: 'billing' }], + ['kb-1'] + ) + ).rejects.toThrow('does not support operator "nosuchop"') + }) + + it('rejects "between" with no upper bound instead of dropping the predicate', async () => { + mockGetDocumentTagDefinitions.mockResolvedValue([ + definition('kb-1', 'number1', 'score', 'number'), + ]) + + await expect( + resolveKnowledgeTagFilters([{ tagName: 'score', operator: 'between', value: '1' }], ['kb-1']) + ).rejects.toThrow('Tag "score" requires valueTo when using the "between" operator') + }) + + it('rejects an upper bound that is not of the tag field type', async () => { + mockGetDocumentTagDefinitions.mockResolvedValue([ + definition('kb-1', 'number1', 'score', 'number'), + ]) + + await expect( + resolveKnowledgeTagFilters( + [{ tagName: 'score', operator: 'between', value: '1', valueTo: 'ten' }], + ['kb-1'] + ) + ).rejects.toThrow('The "between" upper bound is invalid. Tag "score" expects a number value') + }) + + it.each([ + [ + 'text', + 'tag1', + 'category', + ['eq', 'neq', 'contains', 'not_contains', 'starts_with', 'ends_with'], + 'billing', + ], + ['number', 'number1', 'score', ['eq', 'neq', 'gt', 'gte', 'lt', 'lte', 'between'], 1], + ['date', 'date1', 'due', ['eq', 'neq', 'gt', 'gte', 'lt', 'lte', 'between'], '2025-01-10'], + ['boolean', 'boolean1', 'archived', ['eq', 'neq'], true], + ])( + 'accepts every operator a %s tag implements', + async (fieldType, tagSlot, tagName, operators, value) => { + mockGetDocumentTagDefinitions.mockResolvedValue([ + definition('kb-1', tagSlot as string, tagName as string, fieldType as string), + ]) + + for (const operator of operators as string[]) { + const valueTo = fieldType === 'number' ? 2 : '2025-02-10' + const resolved = await resolveKnowledgeTagFilters( + [ + { + tagName: tagName as string, + operator, + value: value as string | number | boolean, + ...(operator === 'between' ? { valueTo } : {}), + }, + ], + ['kb-1'] + ) + expect(resolved.structuredFilters[0].operator).toBe(operator) + } + } + ) }) describe('toKnowledgeTagFilterConditions', () => { diff --git a/apps/sim/lib/knowledge/tags/filter-resolution.ts b/apps/sim/lib/knowledge/tags/filter-resolution.ts index 5c230600c76..57b4bde8854 100644 --- a/apps/sim/lib/knowledge/tags/filter-resolution.ts +++ b/apps/sim/lib/knowledge/tags/filter-resolution.ts @@ -1,3 +1,4 @@ +import { KNOWLEDGE_TAG_FILTER_OPERATORS_BY_FIELD_TYPE } from '@/lib/api/contracts/v1/knowledge' import { OrchestrationError } from '@/lib/core/orchestration/types' import { SUPPORTED_FIELD_TYPES } from '@/lib/knowledge/constants' import type { TagFilterCondition } from '@/lib/knowledge/documents/tag-filter' @@ -21,6 +22,31 @@ export interface KnowledgeTagNameFilter { valueTo?: string | number } +/** + * Rejects an operator the resolved field type does not implement, and a + * `between` with no upper bound. + * + * Both filter builders end their operator switch with a `default:` arm — the + * document list drops the predicate and returns the whole knowledge base, while + * search falls through to equality — so an unchecked operator answered a + * different question depending on which endpoint the caller reached. The field + * type is only known here, after the tag name resolves to its definition. + */ +function validateTagOperator(filter: KnowledgeTagNameFilter, fieldType: string): string | null { + const supported = + KNOWLEDGE_TAG_FILTER_OPERATORS_BY_FIELD_TYPE[ + fieldType as keyof typeof KNOWLEDGE_TAG_FILTER_OPERATORS_BY_FIELD_TYPE + ] + if (!supported) return null + if (!(supported as readonly string[]).includes(filter.operator)) { + return `Tag "${filter.tagName}" is a ${fieldType} tag and does not support operator "${filter.operator}". Supported operators: ${supported.join(', ')}` + } + if (filter.operator === 'between' && filter.valueTo === undefined) { + return `Tag "${filter.tagName}" requires valueTo when using the "between" operator` + } + return null +} + export interface ResolvedKnowledgeTagFilters { structuredFilters: StructuredFilter[] definitionsByKnowledgeBase: Map @@ -34,7 +60,9 @@ export interface ResolvedKnowledgeTagFilters { * across them, is a validation failure telling the caller to search those * knowledge bases separately. With one knowledge base a name that resolves to no * definition is reported as an undefined tag rather than dropped, so a filter is - * never silently ignored. + * never silently ignored. The operator is held to the same guarantee: one the + * resolved field type does not implement is rejected here rather than dropped + * downstream by the document list or coerced to equality by search. * * The loaded definitions are returned alongside the filters so a caller that * also needs the slot-to-name map (to project tag values back out) does not read @@ -97,6 +125,18 @@ export async function resolveKnowledgeTagFilters( definition.fieldType ) if (validationError) typeErrors.push(validationError) + const operatorError = validateTagOperator(filter, definition.fieldType) + if (operatorError) typeErrors.push(operatorError) + if (filter.operator === 'between' && filter.valueTo !== undefined) { + const valueToError = validateTagValue( + filter.tagName, + String(filter.valueTo), + definition.fieldType + ) + if (valueToError) { + typeErrors.push(`The "between" upper bound is invalid. ${valueToError}`) + } + } } if (undefinedTags.length > 0 || typeErrors.length > 0) { throw new OrchestrationError( diff --git a/apps/sim/lib/table/application/groups.test.ts b/apps/sim/lib/table/application/groups.test.ts index b119b6a8c3f..718790a6450 100644 --- a/apps/sim/lib/table/application/groups.test.ts +++ b/apps/sim/lib/table/application/groups.test.ts @@ -75,6 +75,7 @@ vi.mock('@/lib/workflows/application/resolve-workflow-outputs', () => ({ loadResolvedWorkflowOutputs: mocks.loadWorkflowOutputs, })) +import { v2WorkflowGroupSchema } from '@/lib/api/contracts/v2/tables' import { addWorkflowTableGroupOutput, createTableEnrichmentGroup, @@ -307,6 +308,33 @@ describe('workflow and enrichment Table application commands', () => { expect(mocks.signal).not.toHaveBeenCalled() }) + it('stores an empty workflowId for a public enrichment group that omits it', async () => { + const result = await createTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + group: { + type: 'enrichment', + enrichmentId: 'company-domain', + name: 'Company Domain', + outputs: [{ blockId: '', path: '', outputId: 'domain', columnName: 'domain' }], + }, + outputColumns: [{ name: 'domain', type: 'string' }], + }, + }) + + expect(mocks.resolveWorkflowContext).not.toHaveBeenCalled() + expect(mocks.addGroup).toHaveBeenCalledWith( + expect.objectContaining({ + group: expect.objectContaining({ id: 'generated-id', workflowId: '' }), + }), + 'request-1' + ) + expect(result.group.workflowId).toBe('') + expect(v2WorkflowGroupSchema.safeParse(result.group).success).toBe(true) + }) + it('preserves the internal create contract for an invalid related workflow', async () => { mocks.resolveWorkflowContext.mockRejectedValueOnce( new OrchestrationError('not_found', 'Workflow not found') diff --git a/apps/sim/lib/table/application/groups.ts b/apps/sim/lib/table/application/groups.ts index 8252f5a939b..b842b0a60c0 100644 --- a/apps/sim/lib/table/application/groups.ts +++ b/apps/sim/lib/table/application/groups.ts @@ -214,11 +214,26 @@ export const createTableGroupUseCase = defineAuthorizedTableUseCase({ const actorUserId = attributedUserId(principal, context.billedAccountUserId) const groupId = input.group.id ?? generateId() + /** + * The public surface lets an `enrichment` group omit `workflowId`, so the + * stored blob must supply the same `''` a first-party enrichment group + * stores — a missing key fails every later read of the group. + */ + const group: WorkflowGroup = { + ...input.group, + id: groupId, + workflowId: input.group.workflowId ?? '', + outputs: input.group.outputs.map((output) => ({ + ...output, + blockId: output.blockId ?? '', + path: output.path ?? '', + })), + } const table = await addWorkflowGroup( { tableId: context.table.id, workspaceId: context.workspaceId, - group: { ...input.group, id: groupId } as WorkflowGroup, + group, outputColumns: input.outputColumns.map((column) => ({ ...column, workflowGroupId: groupId, diff --git a/apps/sim/lib/table/rows/__tests__/bulk-update-patch-validation.test.ts b/apps/sim/lib/table/rows/__tests__/bulk-update-patch-validation.test.ts new file mode 100644 index 00000000000..459eccbf41f --- /dev/null +++ b/apps/sim/lib/table/rows/__tests__/bulk-update-patch-validation.test.ts @@ -0,0 +1,133 @@ +/** + * @vitest-environment node + * + * A bulk update's patch must be judged on its own: an uncoercible value is a + * property of the request, so it has to be answered identically whether the + * filter matches rows or none. + */ +import { resetDbChainMock } from '@sim/testing' +import { sql } from 'drizzle-orm' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { RowData, TableDefinition } from '@/lib/table/types' + +const mocks = vi.hoisted(() => ({ + selectRowDataPage: vi.fn(), + mutateTableRowsWithSecretProvenance: vi.fn(), +})) + +vi.mock('@/lib/table/rows/ordering', () => ({ + selectRowDataPage: mocks.selectRowDataPage, +})) + +vi.mock('@/lib/table/rows/secret-provenance', () => ({ + mutateTableRowsWithSecretProvenance: mocks.mutateTableRowsWithSecretProvenance, +})) + +vi.mock('@/lib/table/sql', () => ({ + buildFilterClause: vi.fn(() => sql`true`), + buildPredicateClause: vi.fn(() => sql`true`), + buildSortClause: vi.fn(() => sql`true`), + escapeLikePattern: vi.fn((value: string) => value), + fieldPredicate: vi.fn(() => sql`true`), +})) + +vi.mock('@/lib/table/trigger', () => ({ + fireTableTrigger: vi.fn(), +})) + +vi.mock('@/lib/table/workflow-columns', () => ({ + cancelWorkflowGroupRuns: vi.fn(), + runWorkflowColumn: vi.fn(async () => undefined), +})) + +import { updateRowsByFilter } from '@/lib/table/rows/service' + +const TABLE: TableDefinition = { + id: 'table-1', + name: 'People', + description: null, + schema: { + columns: [ + { id: 'name', name: 'Name', type: 'string' }, + { id: 'age', name: 'Age', type: 'number' }, + ], + }, + metadata: null, + rowCount: 1, + maxRows: 10_000, + workspaceId: 'workspace-1', + createdBy: 'user-1', + locks: { schemaLocked: false, insertLocked: false, updateLocked: false, deleteLocked: false }, + archivedAt: null, + createdAt: new Date('2026-08-12T00:00:00.000Z'), + updatedAt: new Date('2026-08-12T00:00:00.000Z'), +} + +function row(id: string, data: RowData): { id: string; data: RowData } { + return { id, data } +} + +describe('bulk update patch validation', () => { + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + mocks.selectRowDataPage.mockResolvedValue([]) + mocks.mutateTableRowsWithSecretProvenance.mockImplementation( + async (_trx: unknown, options: { mutate: () => Promise<{ value: string[] }> }) => { + const outcome = await options.mutate() + return outcome.value + } + ) + }) + + it('rejects an uncoercible patch value when the filter matches nothing', async () => { + await expect( + updateRowsByFilter( + TABLE, + { filter: { name: { eq: 'nobody' } }, data: { age: 'abc' } }, + 'request-1', + { uncoercibleValues: 'reject' } + ) + ).rejects.toMatchObject({ code: 'validation' }) + }) + + it('rejects the same uncoercible patch value when the filter matches rows', async () => { + mocks.selectRowDataPage.mockResolvedValueOnce([row('row-1', { name: 'somebody', age: 30 })]) + + await expect( + updateRowsByFilter( + TABLE, + { filter: { name: { eq: 'somebody' } }, data: { age: 'abc' } }, + 'request-1', + { uncoercibleValues: 'reject' } + ) + ).rejects.toMatchObject({ code: 'validation' }) + }) + + it('still reports zero matches for a valid patch that matches nothing', async () => { + const result = await updateRowsByFilter( + TABLE, + { filter: { name: { eq: 'nobody' } }, data: { age: 41 } }, + 'request-1', + { uncoercibleValues: 'reject' } + ) + + expect(result).toEqual({ affectedCount: 0, affectedRowIds: [] }) + }) + + it('accepts a valid patch over a row whose pre-existing stored value is uncoercible', async () => { + mocks.selectRowDataPage.mockResolvedValue([ + row('row-1', { name: 'somebody', age: 'legacy junk' }), + ]) + + const result = await updateRowsByFilter( + TABLE, + { filter: { name: { eq: 'somebody' } }, data: { name: 'renamed' } }, + 'request-1', + { uncoercibleValues: 'reject' } + ) + + expect(result.affectedCount).toBe(0) + expect(mocks.mutateTableRowsWithSecretProvenance).toHaveBeenCalled() + }) +}) diff --git a/apps/sim/lib/table/rows/cursor.test.ts b/apps/sim/lib/table/rows/cursor.test.ts index 4f4d1439695..a9bbe2ae347 100644 --- a/apps/sim/lib/table/rows/cursor.test.ts +++ b/apps/sim/lib/table/rows/cursor.test.ts @@ -193,6 +193,120 @@ describe('cursor↔filter binding', () => { }) }) +/** + * A predicate tree is sets all the way down: `all`/`any` compile to + * `and(...)`/`or(...)` and an `in`/`nin` operand to an OR fan-out / `IN (...)`. + * Reordering any of them selects the same rows, so a page-2 cursor must survive + * the reorder — while genuinely different predicates must still be refused, + * which is the failure mode that silently serves the wrong rows. + */ +describe('set-valued predicate positions bind by membership, not order', () => { + const A = { field: 'status', op: 'eq', value: 'active' } as const + const B = { field: 'wins', op: 'gte', value: 10 } as const + + it('fingerprints reordered `all` clauses identically', () => { + expect(canonicalFilterKey({ predicate: { all: [A, B] } })).toBe( + canonicalFilterKey({ predicate: { all: [B, A] } }) + ) + }) + + it('fingerprints reordered `any` clauses identically', () => { + expect(canonicalFilterKey({ predicate: { any: [A, B] } })).toBe( + canonicalFilterKey({ predicate: { any: [B, A] } }) + ) + }) + + it('fingerprints a repeated clause like the single clause it selects', () => { + expect(canonicalFilterKey({ predicate: { all: [A, A, B] } })).toBe( + canonicalFilterKey({ predicate: { all: [A, B] } }) + ) + }) + + it('fingerprints reordered `in` operands identically', () => { + expect( + canonicalFilterKey({ + predicate: { all: [{ field: 'owner', op: 'in', value: ['U1', 'U2'] }] }, + }) + ).toBe( + canonicalFilterKey({ + predicate: { all: [{ field: 'owner', op: 'in', value: ['U2', 'U1'] }] }, + }) + ) + }) + + it('fingerprints reordered `nin` operands identically', () => { + expect( + canonicalFilterKey({ + predicate: { all: [{ field: 'owner', op: 'nin', value: ['U1', 'U2'] }] }, + }) + ).toBe( + canonicalFilterKey({ + predicate: { all: [{ field: 'owner', op: 'nin', value: ['U2', 'U1'] }] }, + }) + ) + }) + + it('applies the rule inside a nested group', () => { + expect( + canonicalFilterKey({ + predicate: { all: [{ any: [A, B] }, { field: 'x', op: 'in', value: ['b', 'a'] }] }, + }) + ).toBe( + canonicalFilterKey({ + predicate: { all: [{ field: 'x', op: 'in', value: ['a', 'b'] }, { any: [B, A] }] }, + }) + ) + }) + + /** + * The dangerous half. Canonicalizing too far would collapse predicates that + * select different rows onto one stamp, and a cursor would then resume a page + * of the wrong sequence without any 400 at all. + */ + it('keeps genuinely different predicates apart', () => { + const key = (predicate: TablePredicate) => canonicalFilterKey({ predicate }) + const distinct = [ + key({ all: [A, B] }), + key({ any: [A, B] }), + key({ all: [A] }), + key({ all: [{ field: 'status', op: 'ne', value: 'active' }] }), + key({ all: [{ field: 'owner', op: 'in', value: ['U1'] }] }), + key({ all: [{ field: 'owner', op: 'in', value: ['U1', 'U2'] }] }), + key({ all: [{ field: 'owner', op: 'nin', value: ['U1', 'U2'] }] }), + key({ all: [{ field: 'other', op: 'in', value: ['U1', 'U2'] }] }), + key({ all: [{ any: [A, B] }] }), + ] + expect(new Set(distinct).size).toBe(distinct.length) + }) + + /** + * An ordinary array operand is a sequence, not a set — `eq` matches a JSON + * array value by containment of that exact array, so reordering it changes + * which rows match and must change the stamp. + */ + it('leaves a non-set operand array bound to its order', () => { + expect( + canonicalFilterKey({ predicate: { all: [{ field: 'tags', op: 'eq', value: ['a', 'b'] }] } }) + ).not.toBe( + canonicalFilterKey({ predicate: { all: [{ field: 'tags', op: 'eq', value: ['b', 'a'] }] } }) + ) + }) + + it('accepts a cursor replayed under a reordered predicate, refuses a different one', () => { + const token = encodeCursor({ + lastRow: { id: 'row_1', orderKey: null }, + keysetValid: false, + nextOffset: 50, + predicate: { all: [A, B] }, + }) + const decoded = decodeCursor(token) + expect(() => assertCursorQueryBinding(decoded, { predicate: { all: [B, A] } })).not.toThrow() + expect(() => assertCursorQueryBinding(decoded, { predicate: { any: [B, A] } })).toThrow( + TableQueryValidationError + ) + }) +}) + /** * The filter stamp is additive, and the payload version is deliberately not * bumped for it (see `CURSOR_VERSION`). These pin what a token minted by the diff --git a/apps/sim/lib/table/rows/cursor.ts b/apps/sim/lib/table/rows/cursor.ts index 1350fbaa054..3ceea330316 100644 --- a/apps/sim/lib/table/rows/cursor.ts +++ b/apps/sim/lib/table/rows/cursor.ts @@ -18,7 +18,7 @@ * {@link assertCursorQueryBinding}. */ -import { canonicalJson, fingerprint } from '@/lib/api/cursor-binding' +import { canonicalJson, canonicalUnorderedArray, fingerprint } from '@/lib/api/cursor-binding' import { TableQueryValidationError } from '@/lib/table/errors' import type { Filter, Sort, TablePredicate, TableRow, TableRowsCursor } from '@/lib/table/types' @@ -65,6 +65,39 @@ export function canonicalSortKey(sort: Sort | null | undefined): string | undefi return entries.length > 0 ? JSON.stringify(entries) : undefined } +/** + * Canonical form of a predicate node, with every set-valued position sorted. + * + * `canonicalJson` sorts object keys but preserves array order, which is right + * for a sequence and wrong for a set. A predicate tree is sets all the way + * down: `all`/`any` compile to `and(...)`/`or(...)`, and an `in`/`nin` operand + * compiles to an OR fan-out / `IN (...)`. Member order changes none of those + * row sets, so binding the caller's spelling refuses a cursor for a page that + * is genuinely the next one. + * + * Non-set positions fall through to `canonicalJson` unchanged, so two + * predicates that select different rows still fingerprint differently. + */ +function canonicalPredicateNode(node: unknown): string { + if (node === null || typeof node !== 'object' || Array.isArray(node)) return canonicalJson(node) + const record = node as Record + const setValuedOperand = record.op === 'in' || record.op === 'nin' + const entries = Object.entries(record) + .filter(([, entry]) => entry !== undefined) + .sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0)) + return `{${entries + .map(([key, entry]) => { + if (Array.isArray(entry) && (key === 'all' || key === 'any')) { + return `${JSON.stringify(key)}:${canonicalUnorderedArray(entry, canonicalPredicateNode)}` + } + if (Array.isArray(entry) && key === 'value' && setValuedOperand) { + return `${JSON.stringify(key)}:${canonicalUnorderedArray(entry)}` + } + return `${JSON.stringify(key)}:${canonicalJson(entry)}` + }) + .join(',')}}` +} + /** * Fingerprint of the filters a page was produced under, or `undefined` for an * unfiltered read. Canonicalized and hashed through `lib/api/cursor-binding`, @@ -77,7 +110,9 @@ export function canonicalFilterKey( const predicate = scope.predicate ?? undefined const filter = scope.filter && Object.keys(scope.filter).length > 0 ? scope.filter : undefined if (!predicate && !filter) return undefined - return fingerprint(canonicalJson(predicate ? { predicate } : { filter })) + return fingerprint( + predicate ? `{"predicate":${canonicalPredicateNode(predicate)}}` : canonicalJson({ filter }) + ) } /** diff --git a/apps/sim/lib/table/rows/service.ts b/apps/sim/lib/table/rows/service.ts index f6c770074f1..d0491f40919 100644 --- a/apps/sim/lib/table/rows/service.ts +++ b/apps/sim/lib/table/rows/service.ts @@ -1788,6 +1788,40 @@ function bulkUpdateValidationError( return schemaValidation.valid ? null : schemaValidation.errors.join(', ') } +/** + * Validates the patch on its own, before any row is scanned, so a value the + * column type cannot store is answered the same way whether the filter matches + * rows or none. {@link validateBulkUpdateMatches} only runs once a page comes + * back, which left a zero-match filter reporting success for a value the write + * would never have accepted. + * + * Restricted to the columns the caller actually supplied a non-null value for: + * absent columns must not raise a missing-required error on a partial patch, + * and a patched null is left to the merged-row check that already polices it. + * Pre-existing stored values are not in scope here at all, so the patched-keys + * narrowing the merged check relies on is untouched. + */ +function validateBulkUpdatePatch( + table: TableDefinition, + patch: RowData, + policy: UncoercibleValuePolicy | undefined +): void { + const suppliedColumns = table.schema.columns.filter((column) => { + const value = patch[getColumnId(column)] + return value !== null && value !== undefined + }) + if (suppliedColumns.length === 0) return + + const validation = coerceRowToSchema( + { ...patch }, + { ...table.schema, columns: suppliedColumns }, + policy + ) + if (!validation.valid) { + throw new OrchestrationError('validation', validation.errors.join(', ')) + } +} + /** Validates a bounded page of rows against a bulk merge patch. */ function validateBulkUpdateMatches( table: TableDefinition, @@ -1963,6 +1997,7 @@ export async function updateRowsByFilter( ) coerceRowValues(data.data, table.schema, options.uncoercibleValues) + validateBulkUpdatePatch(table, data.data, options.uncoercibleValues) const uniqueColumns = getUniqueColumns(table.schema) const uniqueColumnsInUpdate = uniqueColumns.filter((col) => getColumnId(col) in data.data) const patchJson = JSON.stringify(data.data) diff --git a/apps/sim/proxy.test.ts b/apps/sim/proxy.test.ts index e72e879aa8a..d5ce595f8a9 100644 --- a/apps/sim/proxy.test.ts +++ b/apps/sim/proxy.test.ts @@ -11,6 +11,9 @@ vi.mock('@/lib/core/config/env', () => import { resolveApiCorsPolicy } from '@/proxy' +const EXPOSED_HEADERS = + 'Retry-After, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset, X-Request-Id, X-Run-Id' + function makeRequest(pathname: string, origin?: string): NextRequest { return { nextUrl: { pathname }, @@ -27,6 +30,7 @@ describe('resolveApiCorsPolicy', () => { credentials: false, methods: 'GET, POST, OPTIONS', headers: 'Content-Type, Authorization, Accept', + exposeHeaders: EXPOSED_HEADERS, }) }) @@ -46,6 +50,7 @@ describe('resolveApiCorsPolicy', () => { credentials: true, methods: 'GET, POST, PUT, OPTIONS', headers: 'Content-Type, X-Requested-With', + exposeHeaders: EXPOSED_HEADERS, }) } }) @@ -134,6 +139,38 @@ describe('resolveApiCorsPolicy', () => { }) }) + /** + * `X-Run-Id` is emitted by the v2 execute route alone, and that route is + * wildcard-origin so browsers can call it — a policy that omits the exposed + * headers leaves the run id, and a 429's `Retry-After`, unreadable to exactly + * the callers the route exists for. + */ + it('exposes the response headers on the v2 execute policy, not just the default one', () => { + const execute = resolveApiCorsPolicy( + makeRequest('/api/v2/workflows/workflow-123/execute', 'https://other.example') + ) + expect(execute.exposeHeaders).toBe(EXPOSED_HEADERS) + expect(execute.exposeHeaders).toContain('X-Run-Id') + expect(execute.exposeHeaders).toContain('Retry-After') + + const fallback = resolveApiCorsPolicy(makeRequest('/api/files/uploads')) + expect(fallback.exposeHeaders).toBe(execute.exposeHeaders) + }) + + it('exposes the response headers on every matched rule, so a new rule cannot drop them', () => { + const paths = [ + '/api/auth/oauth2/token', + '/api/mcp/copilot', + '/api/chat/abc', + '/api/workflows/wf/execute', + '/api/v2/workflows/wf/execute', + '/api/files/uploads', + ] + for (const path of paths) { + expect(resolveApiCorsPolicy(makeRequest(path)).exposeHeaders).toBe(EXPOSED_HEADERS) + } + }) + it('never pairs wildcard origin with credentials (CORS spec invariant)', () => { const paths = [ '/api/auth/oauth2/token', diff --git a/apps/sim/proxy.ts b/apps/sim/proxy.ts index befa9b484d9..d366d1e6adb 100644 --- a/apps/sim/proxy.ts +++ b/apps/sim/proxy.ts @@ -131,11 +131,23 @@ const CORS_RULES: readonly CorsRule[] = [ }, ] -/** Single source of truth for /api/* CORS — resolved at request time, not baked at build. */ +/** + * Single source of truth for /api/* CORS — resolved at request time, not baked at build. + * + * The exposed-header list is applied to every policy, matched rule or fallback, + * because the headers it names are set by the same shared route machinery on + * every route. A rule opts out by spelling `exposeHeaders: undefined`; carrying + * the list per rule instead is how `/api/v2/workflows/{id}/execute` — the only + * route that emits `X-Run-Id`, and wildcard-origin precisely so browsers can + * call it — ended up unable to hand a browser the run id or a 429's + * `Retry-After`. + */ export function resolveApiCorsPolicy(request: NextRequest): CorsPolicy { const { pathname } = request.nextUrl for (const rule of CORS_RULES) { - if (rule.match(pathname)) return rule.policy(request) + if (rule.match(pathname)) { + return { exposeHeaders: DEFAULT_API_EXPOSED_HEADERS, ...rule.policy(request) } + } } return { origin: getEnv('NEXT_PUBLIC_APP_URL') || 'http://localhost:3001',