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',