diff --git a/.agents/skills/v2-api-conventions/SKILL.md b/.agents/skills/v2-api-conventions/SKILL.md index 52ab14450ad..01f9e8f29d3 100644 --- a/.agents/skills/v2-api-conventions/SKILL.md +++ b/.agents/skills/v2-api-conventions/SKILL.md @@ -62,7 +62,16 @@ Two of these carry real design weight: **404 is deliberately overloaded.** A workspace the caller cannot reach answers `404 "Workspace not found"`, never 403 — a 403 would confirm the resource exists. `createV2ResourceConcealmentPolicy` does this by mapping a cross-tenant authorization failure to `v2Error('NOT_FOUND', ...)`. The rollout gate answers the same way for the same reason (`gate.ts`: "an ungated caller cannot distinguish 'not in the rollout cohort' from 'no such endpoint'"), and so does the unknown-path catch-all at `app/api/v2/[[...segments]]/route.ts` — its body is byte-identical to the gate's on purpose. -**500 is never caller-reachable.** Any input a caller can send must be rejected at the contract boundary with a 400. If you can construct a query string or body that produces a 500, that is a bug in the contract, not something to wrap in a `try`/`catch`. `v2ErrorForOrchestration` also replaces the message on an unclassified failure with a generic one, so internal detail never leaks. +**500 is never caller-reachable.** Any input a caller can send must be rejected at the contract boundary with a 400. If you can construct a query string or body that produces a 500, that is a bug in the contract, not something to wrap in a `try`/`catch`. `v2ErrorForOrchestration` also replaces the message on an unclassified failure with a generic one, so internal detail never leaks. A caller-reachable 500 has shipped twice — a fractional `limit` reaching `LIMIT 2.5`, and a plain `HEAD` tripping the builder's method guard — so treat "a well-formed request produced a 500" as the highest-severity class of defect on this surface. + +**Which of 403 and 404 an operation documents follows from its authorization, not from whether it is a read.** `requirePermission` throws two different failures: no workspace access at all is `NoWorkspaceAccessError`, which `createV2ResourceConcealmentPolicy` conceals as 404; access below the operation's `minimumRole` is `InsufficientWorkspacePermissionsError`, which stays a 403. So: + +- An operation whose `minimumRole` is `write` or `admin` can always 403 — a member with a lower role hits it. Document 403. +- An operation whose `minimumRole` is `read` cannot 403 *that* way, because `read` is the floor of the `read < write < admin` ordering and anyone without access is concealed as 404 instead. It can still 403 through `PersonalApiKeysDisabledError` (a personal API key against a workspace whose organization disabled them) or `WorkspaceApiKeyAuthorizationError` (`workspaceApiKey: 'deny'`), and every v2 operation is reachable by a personal API key. **So in practice every workspace-scoped v2 operation documents 403**, and the reads that omitted it were wrong, not principled. + +Use the shared sets in `contracts/v2/openapi/shared.ts` — `RESOURCE_ERRORS`, `RESOURCE_CONFLICT_ERRORS`, `RESOURCE_MUTATION_ERRORS` — rather than assembling a per-operation list; all three already include `Forbidden`, and hand-assembled lists are how three knowledge reads and three upload operations quietly lost it. + +**HEAD is answered by the `GET` handler, not rejected.** Next aliases a missing `HEAD` export onto `GET` and drops the body when sending, so a route's `GET` legitimately runs with `request.method === 'HEAD'`. The builders' method guard accepts that pairing via `methodMatchesContract`; any other mismatch stays a hard error. Never hand-write a `HEAD` export to "fix" this. ## Rule 3 — a collection that returns `nextCursor` must accept `limit` + `cursor`, and must apply them diff --git a/.claude/commands/v2-api-conventions.md b/.claude/commands/v2-api-conventions.md index c7740489850..6ad148230c2 100644 --- a/.claude/commands/v2-api-conventions.md +++ b/.claude/commands/v2-api-conventions.md @@ -61,7 +61,16 @@ Two of these carry real design weight: **404 is deliberately overloaded.** A workspace the caller cannot reach answers `404 "Workspace not found"`, never 403 — a 403 would confirm the resource exists. `createV2ResourceConcealmentPolicy` does this by mapping a cross-tenant authorization failure to `v2Error('NOT_FOUND', ...)`. The rollout gate answers the same way for the same reason (`gate.ts`: "an ungated caller cannot distinguish 'not in the rollout cohort' from 'no such endpoint'"), and so does the unknown-path catch-all at `app/api/v2/[[...segments]]/route.ts` — its body is byte-identical to the gate's on purpose. -**500 is never caller-reachable.** Any input a caller can send must be rejected at the contract boundary with a 400. If you can construct a query string or body that produces a 500, that is a bug in the contract, not something to wrap in a `try`/`catch`. `v2ErrorForOrchestration` also replaces the message on an unclassified failure with a generic one, so internal detail never leaks. +**500 is never caller-reachable.** Any input a caller can send must be rejected at the contract boundary with a 400. If you can construct a query string or body that produces a 500, that is a bug in the contract, not something to wrap in a `try`/`catch`. `v2ErrorForOrchestration` also replaces the message on an unclassified failure with a generic one, so internal detail never leaks. A caller-reachable 500 has shipped twice — a fractional `limit` reaching `LIMIT 2.5`, and a plain `HEAD` tripping the builder's method guard — so treat "a well-formed request produced a 500" as the highest-severity class of defect on this surface. + +**Which of 403 and 404 an operation documents follows from its authorization, not from whether it is a read.** `requirePermission` throws two different failures: no workspace access at all is `NoWorkspaceAccessError`, which `createV2ResourceConcealmentPolicy` conceals as 404; access below the operation's `minimumRole` is `InsufficientWorkspacePermissionsError`, which stays a 403. So: + +- An operation whose `minimumRole` is `write` or `admin` can always 403 — a member with a lower role hits it. Document 403. +- An operation whose `minimumRole` is `read` cannot 403 *that* way, because `read` is the floor of the `read < write < admin` ordering and anyone without access is concealed as 404 instead. It can still 403 through `PersonalApiKeysDisabledError` (a personal API key against a workspace whose organization disabled them) or `WorkspaceApiKeyAuthorizationError` (`workspaceApiKey: 'deny'`), and every v2 operation is reachable by a personal API key. **So in practice every workspace-scoped v2 operation documents 403**, and the reads that omitted it were wrong, not principled. + +Use the shared sets in `contracts/v2/openapi/shared.ts` — `RESOURCE_ERRORS`, `RESOURCE_CONFLICT_ERRORS`, `RESOURCE_MUTATION_ERRORS` — rather than assembling a per-operation list; all three already include `Forbidden`, and hand-assembled lists are how three knowledge reads and three upload operations quietly lost it. + +**HEAD is answered by the `GET` handler, not rejected.** Next aliases a missing `HEAD` export onto `GET` and drops the body when sending, so a route's `GET` legitimately runs with `request.method === 'HEAD'`. The builders' method guard accepts that pairing via `methodMatchesContract`; any other mismatch stays a hard error. Never hand-write a `HEAD` export to "fix" this. ## Rule 3 — a collection that returns `nextCursor` must accept `limit` + `cursor`, and must apply them diff --git a/.cursor/commands/v2-api-conventions.md b/.cursor/commands/v2-api-conventions.md index a19ab57e6aa..ea45560bd5b 100644 --- a/.cursor/commands/v2-api-conventions.md +++ b/.cursor/commands/v2-api-conventions.md @@ -56,7 +56,16 @@ Two of these carry real design weight: **404 is deliberately overloaded.** A workspace the caller cannot reach answers `404 "Workspace not found"`, never 403 — a 403 would confirm the resource exists. `createV2ResourceConcealmentPolicy` does this by mapping a cross-tenant authorization failure to `v2Error('NOT_FOUND', ...)`. The rollout gate answers the same way for the same reason (`gate.ts`: "an ungated caller cannot distinguish 'not in the rollout cohort' from 'no such endpoint'"), and so does the unknown-path catch-all at `app/api/v2/[[...segments]]/route.ts` — its body is byte-identical to the gate's on purpose. -**500 is never caller-reachable.** Any input a caller can send must be rejected at the contract boundary with a 400. If you can construct a query string or body that produces a 500, that is a bug in the contract, not something to wrap in a `try`/`catch`. `v2ErrorForOrchestration` also replaces the message on an unclassified failure with a generic one, so internal detail never leaks. +**500 is never caller-reachable.** Any input a caller can send must be rejected at the contract boundary with a 400. If you can construct a query string or body that produces a 500, that is a bug in the contract, not something to wrap in a `try`/`catch`. `v2ErrorForOrchestration` also replaces the message on an unclassified failure with a generic one, so internal detail never leaks. A caller-reachable 500 has shipped twice — a fractional `limit` reaching `LIMIT 2.5`, and a plain `HEAD` tripping the builder's method guard — so treat "a well-formed request produced a 500" as the highest-severity class of defect on this surface. + +**Which of 403 and 404 an operation documents follows from its authorization, not from whether it is a read.** `requirePermission` throws two different failures: no workspace access at all is `NoWorkspaceAccessError`, which `createV2ResourceConcealmentPolicy` conceals as 404; access below the operation's `minimumRole` is `InsufficientWorkspacePermissionsError`, which stays a 403. So: + +- An operation whose `minimumRole` is `write` or `admin` can always 403 — a member with a lower role hits it. Document 403. +- An operation whose `minimumRole` is `read` cannot 403 *that* way, because `read` is the floor of the `read < write < admin` ordering and anyone without access is concealed as 404 instead. It can still 403 through `PersonalApiKeysDisabledError` (a personal API key against a workspace whose organization disabled them) or `WorkspaceApiKeyAuthorizationError` (`workspaceApiKey: 'deny'`), and every v2 operation is reachable by a personal API key. **So in practice every workspace-scoped v2 operation documents 403**, and the reads that omitted it were wrong, not principled. + +Use the shared sets in `contracts/v2/openapi/shared.ts` — `RESOURCE_ERRORS`, `RESOURCE_CONFLICT_ERRORS`, `RESOURCE_MUTATION_ERRORS` — rather than assembling a per-operation list; all three already include `Forbidden`, and hand-assembled lists are how three knowledge reads and three upload operations quietly lost it. + +**HEAD is answered by the `GET` handler, not rejected.** Next aliases a missing `HEAD` export onto `GET` and drops the body when sending, so a route's `GET` legitimately runs with `request.method === 'HEAD'`. The builders' method guard accepts that pairing via `methodMatchesContract`; any other mismatch stays a hard error. Never hand-write a `HEAD` export to "fix" this. ## Rule 3 — a collection that returns `nextCursor` must accept `limit` + `cursor`, and must apply them diff --git a/apps/docs/openapi-v2-files-audit.json b/apps/docs/openapi-v2-files-audit.json index 702da1a9924..47c9020d248 100644 --- a/apps/docs/openapi-v2-files-audit.json +++ b/apps/docs/openapi-v2-files-audit.json @@ -370,6 +370,9 @@ "401": { "$ref": "#/components/responses/Unauthorized" }, + "403": { + "$ref": "#/components/responses/Forbidden" + }, "404": { "$ref": "#/components/responses/NotFound" }, @@ -468,6 +471,9 @@ "401": { "$ref": "#/components/responses/Unauthorized" }, + "403": { + "$ref": "#/components/responses/Forbidden" + }, "404": { "$ref": "#/components/responses/NotFound" }, @@ -555,6 +561,9 @@ "401": { "$ref": "#/components/responses/Unauthorized" }, + "403": { + "$ref": "#/components/responses/Forbidden" + }, "404": { "$ref": "#/components/responses/NotFound" }, diff --git a/apps/docs/openapi-v2-knowledge.json b/apps/docs/openapi-v2-knowledge.json index 2ec9e730198..fac622ee6b5 100644 --- a/apps/docs/openapi-v2-knowledge.json +++ b/apps/docs/openapi-v2-knowledge.json @@ -295,6 +295,9 @@ "401": { "$ref": "#/components/responses/Unauthorized" }, + "403": { + "$ref": "#/components/responses/Forbidden" + }, "404": { "$ref": "#/components/responses/NotFound" }, @@ -672,6 +675,9 @@ "401": { "$ref": "#/components/responses/Unauthorized" }, + "403": { + "$ref": "#/components/responses/Forbidden" + }, "404": { "$ref": "#/components/responses/NotFound" }, @@ -1252,6 +1258,9 @@ "401": { "$ref": "#/components/responses/Unauthorized" }, + "403": { + "$ref": "#/components/responses/Forbidden" + }, "404": { "$ref": "#/components/responses/NotFound" }, diff --git a/apps/sim/app/api/v2/skills/route.test.ts b/apps/sim/app/api/v2/skills/route.test.ts index 6e193ae8d11..6324aee87dc 100644 --- a/apps/sim/app/api/v2/skills/route.test.ts +++ b/apps/sim/app/api/v2/skills/route.test.ts @@ -90,7 +90,7 @@ const skill = { updatedAt: new Date('2026-01-02T00:00:00Z'), } -function request(method: 'GET' | 'POST', url: string, body?: unknown) { +function request(method: 'GET' | 'POST' | 'HEAD', url: string, body?: unknown) { return new NextRequest(`http://localhost:3000${url}`, { method, headers: { @@ -179,6 +179,17 @@ describe('/api/v2/skills', () => { expect(mocks.list).not.toHaveBeenCalled() }) + /** + * The guard itself is unit-tested in `definition.test.ts`; this proves the + * pairing end-to-end, on a real v2 read that used to reply 500 to a plain HEAD. + */ + it('serves HEAD through the GET handler instead of throwing', async () => { + const response = await GET(request('HEAD', `/api/v2/skills?workspaceId=${WORKSPACE_ID}`)) + + expect(response.status).toBe(200) + expect(mocks.list).toHaveBeenCalled() + }) + it('rejects a malformed cursor rather than silently restarting at page one', async () => { const response = await GET( request('GET', `/api/v2/skills?workspaceId=${WORKSPACE_ID}&cursor=not-a-cursor`) diff --git a/apps/sim/lib/api/contracts/v2/openapi/files-audit.ts b/apps/sim/lib/api/contracts/v2/openapi/files-audit.ts index 1eaca40c670..b94bc2d4ce4 100644 --- a/apps/sim/lib/api/contracts/v2/openapi/files-audit.ts +++ b/apps/sim/lib/api/contracts/v2/openapi/files-audit.ts @@ -26,16 +26,14 @@ import { ERROR_RESPONSES, type ErrorResponseId, RATE_LIMIT_HEADERS, + RESOURCE_CONFLICT_ERRORS, RESOURCE_ERRORS, - STANDARD_ERRORS, V2_API_KEY_SECURITY, V2_API_KEY_SECURITY_SCHEMES, V2_COMMON_HEADERS, V2_ERROR_SCHEMA, - VALIDATED_ERRORS, WORKSPACE_API_KEY_DENIED, WORKSPACE_API_KEY_DENIED_AS_NOT_FOUND, - WORKSPACE_ERRORS, } from '@/lib/api/contracts/v2/openapi/shared' import { defineOpenApiDocument, @@ -122,7 +120,7 @@ const routes = [ summary: 'List Files', description: 'List workspace files with search, sorting, folder filtering, and opaque cursor pagination.', - errors: [...WORKSPACE_ERRORS, 'NotFound'], + errors: RESOURCE_ERRORS, success: { description: 'A page of workspace files.' }, }), { @@ -148,7 +146,7 @@ const routes = [ summary: 'Create File', description: 'Create a workspace file from inline UTF-8 or base64 content. Use an upload session for streamed or larger files.', - errors: [...WORKSPACE_ERRORS, 'NotFound', 'Conflict', 'PayloadTooLarge'], + errors: [...RESOURCE_CONFLICT_ERRORS, 'PayloadTooLarge'], success: { description: 'The created file.' }, }), { @@ -181,7 +179,7 @@ const routes = [ summary: 'Create File Upload', description: 'Create a resumable upload session and receive either a signed PUT URL or multipart instructions.', - errors: [...WORKSPACE_ERRORS, 'NotFound'], + errors: RESOURCE_ERRORS, success: { description: 'The created upload session and transfer instructions.' }, }), { @@ -213,7 +211,7 @@ const routes = [ operationId: 'abortFileUpload', summary: 'Abort File Upload', description: 'Abort an active upload session and release provider-side multipart state.', - errors: [...VALIDATED_ERRORS, 'NotFound', 'Conflict'], + errors: RESOURCE_CONFLICT_ERRORS, success: { description: 'The aborted upload session.' }, }), { @@ -249,7 +247,7 @@ const routes = [ operationId: 'createFileUploadPartUrls', summary: 'Create File Upload Part URLs', description: 'Create signed URLs for a bounded set of multipart upload part numbers.', - errors: [...STANDARD_ERRORS, 'BadRequest', 'NotFound', 'Conflict'], + errors: RESOURCE_CONFLICT_ERRORS, success: { description: 'Signed URLs for the requested upload parts.' }, }), { @@ -293,7 +291,7 @@ const routes = [ summary: 'Complete File Upload', description: 'Finalize uploaded bytes, verify provider state, and begin atomic workspace-file registration.', - errors: [...STANDARD_ERRORS, 'BadRequest', 'NotFound', 'Conflict'], + errors: RESOURCE_CONFLICT_ERRORS, success: { description: 'The completed or finalizing upload session.' }, }), { @@ -330,7 +328,7 @@ const routes = [ summary: 'Download File', description: 'Download the current file bytes from a workspace. A generated document is served as its compiled artifact, so it returns `409` while that artifact is still compiling and `413` if it renders past the size ceiling.', - errors: [...WORKSPACE_ERRORS, 'NotFound', 'Conflict', 'PayloadTooLarge'], + errors: [...RESOURCE_CONFLICT_ERRORS, 'PayloadTooLarge'], success: { description: 'The file bytes.', headers: ['Content-Type', 'Content-Disposition', 'Content-Length'], @@ -359,7 +357,7 @@ const routes = [ summary: 'Delete File', description: 'Archive a workspace file. This is a soft delete: the row is retained with a deletion timestamp, the file stops appearing in listings and is no longer readable through the API, and its stored bytes are never removed. An archived file can be restored from the workspace Recently Deleted settings; the v2 API exposes no restore operation.', - errors: [...WORKSPACE_ERRORS, 'NotFound', 'Conflict'], + errors: RESOURCE_CONFLICT_ERRORS, success: { description: 'Deletion confirmation.' }, }), { @@ -389,7 +387,7 @@ const routes = [ operationId: 'renameFile', summary: 'Rename File', description: 'Rename a workspace file without changing its containing folder.', - errors: [...WORKSPACE_ERRORS, 'NotFound', 'Conflict'], + errors: RESOURCE_CONFLICT_ERRORS, success: { description: 'The renamed file.' }, }), { @@ -426,7 +424,7 @@ const routes = [ operationId: 'getFile', summary: 'Get File Metadata', description: 'Return file metadata together with the nullable current public-share state.', - errors: [...WORKSPACE_ERRORS, 'NotFound'], + errors: RESOURCE_ERRORS, success: { description: 'File metadata and public-share state.' }, }), { @@ -460,7 +458,7 @@ const routes = [ operationId: 'listAuditLogs', summary: 'List Audit Logs', description: `List an organization audit trail with filters and opaque cursor pagination. Requires an Enterprise subscription and organization admin or owner access. ${WORKSPACE_API_KEY_DENIED}`, - errors: [...VALIDATED_ERRORS, 'Forbidden', 'NotFound'], + errors: RESOURCE_ERRORS, success: { description: 'A page of audit-log entries.' }, }), { @@ -485,7 +483,7 @@ const routes = [ operationId: 'getAuditLog', summary: 'Get Audit Log', description: `Return one organization audit-log entry. Requires an Enterprise subscription and organization admin or owner access. ${WORKSPACE_API_KEY_DENIED}`, - errors: [...VALIDATED_ERRORS, 'Forbidden', 'NotFound'], + errors: RESOURCE_ERRORS, success: { description: 'The requested audit-log entry.' }, }), { @@ -516,7 +514,7 @@ const routes = [ operationId: 'moveFileItems', summary: 'Move Files', description: 'Move up to 1,000 files to a canonical folder path or the workspace root.', - errors: [...WORKSPACE_ERRORS, 'NotFound', 'Conflict'], + errors: RESOURCE_CONFLICT_ERRORS, success: { description: 'Count of moved files.' }, }), { @@ -549,7 +547,7 @@ const routes = [ summary: 'Get File Share', description: 'Return the nullable current public-share configuration for a file. A file that has never been shared returns `data: null` rather than a 404; a share that was created and later disabled is still returned, with `isActive: false`.', - errors: [...WORKSPACE_ERRORS, 'NotFound'], + errors: RESOURCE_ERRORS, success: { description: 'Current nullable file-share state.' }, }), { @@ -580,7 +578,7 @@ const routes = [ operationId: 'upsertFileShare', summary: 'Enable or Disable File Share', description: `Create or partially update a server-tokenized public share. Only isActive is required, and an omitted authType keeps the stored auth mode. What happens to password and allowedEmails depends on the resulting mode, because enabling a share always rewrites the credentials the chosen mode does not use: 'public' clears the stored password and empties allowedEmails; 'password' keeps the stored password when password is omitted but empties allowedEmails; 'email' and 'sso' clear the stored password and keep the stored allowedEmails when the field is omitted. Only disabling with isActive false preserves the whole access configuration untouched — it also retains the token, so re-enabling restores the share as it was. Two enabling combinations are rejected outright with a 400 instead of being partially applied: 'password' when neither a password is supplied nor one is already stored, and 'email' or 'sso' when the resulting allowedEmails would be empty because none was supplied and none is stored. On a file that has never been shared there is nothing stored to fall back on, so enabling any mode other than 'public' must carry its credential in the same request. ${WORKSPACE_API_KEY_DENIED_AS_NOT_FOUND}`, - errors: [...WORKSPACE_ERRORS, 'NotFound'], + errors: RESOURCE_ERRORS, success: { description: 'The updated file share.' }, }), { @@ -622,7 +620,7 @@ const routes = [ operationId: 'updateFileContent', summary: 'Replace File Content', description: 'Replace the complete contents of an existing file from UTF-8 or base64 input.', - errors: [...WORKSPACE_ERRORS, 'NotFound', 'PayloadTooLarge'], + errors: [...RESOURCE_ERRORS, 'PayloadTooLarge'], success: { description: 'The updated file.' }, }), { @@ -659,7 +657,7 @@ const routes = [ operationId: 'bulkDeleteFiles', summary: 'Delete Files', description: 'Delete up to 1,000 workspace files in one operation.', - errors: [...WORKSPACE_ERRORS, 'NotFound', 'Conflict'], + errors: RESOURCE_CONFLICT_ERRORS, success: { description: 'Count of deleted files.' }, }), { @@ -715,7 +713,7 @@ const routes = [ operationId: 'createFilesFolder', summary: 'Create Folder', description: 'Create a canonical folder path in a workspace.', - errors: [...WORKSPACE_ERRORS, 'NotFound', 'Conflict'], + errors: RESOURCE_CONFLICT_ERRORS, success: { description: 'The created folder.' }, }), { @@ -745,7 +743,7 @@ const routes = [ operationId: 'relocateFilesFolder', summary: 'Rename or Move Folder', description: 'Rename or move a folder and atomically rewrite descendant canonical paths.', - errors: [...WORKSPACE_ERRORS, 'NotFound', 'Conflict'], + errors: RESOURCE_CONFLICT_ERRORS, success: { description: 'The relocated folder.' }, }), { @@ -776,7 +774,7 @@ const routes = [ operationId: 'deleteFilesFolder', summary: 'Delete Folder', description: 'Delete a folder, optionally including every nested file and folder.', - errors: [...WORKSPACE_ERRORS, 'NotFound', 'Conflict'], + errors: RESOURCE_CONFLICT_ERRORS, success: { description: 'Folder deletion confirmation and deleted item counts.' }, }), { diff --git a/apps/sim/lib/api/contracts/v2/openapi/knowledge.ts b/apps/sim/lib/api/contracts/v2/openapi/knowledge.ts index 4e4b7a1f68b..dd4ecf3f022 100644 --- a/apps/sim/lib/api/contracts/v2/openapi/knowledge.ts +++ b/apps/sim/lib/api/contracts/v2/openapi/knowledge.ts @@ -31,7 +31,6 @@ import { V2_API_KEY_SECURITY_SCHEMES, V2_COMMON_HEADERS, V2_ERROR_SCHEMA, - VALIDATED_ERRORS, WORKSPACE_ERRORS, } from '@/lib/api/contracts/v2/openapi/shared' import { @@ -116,7 +115,7 @@ const routes = [ operationId: 'getKnowledgeBase', summary: 'Get Knowledge Base', description: `Retrieve a knowledge base by identifier. Inaccessible knowledge bases are reported as not found. ${FOLDER_TREE_TOO_LARGE}`, - errors: [...VALIDATED_ERRORS, 'NotFound', 'PayloadTooLarge'], + errors: [...RESOURCE_ERRORS, 'PayloadTooLarge'], success: { description: 'The requested knowledge base.' }, }), { @@ -177,7 +176,7 @@ const routes = [ operationId: 'deleteKnowledgeBase', summary: 'Delete Knowledge Base', description: 'Delete a knowledge base and its documents.', - errors: [...WORKSPACE_ERRORS, 'NotFound'], + errors: RESOURCE_ERRORS, success: { description: 'Knowledge base deletion acknowledgement.' }, }), { @@ -241,7 +240,7 @@ const routes = [ summary: 'List Documents', description: 'List documents in a knowledge base with filename search, state filtering, sorting, and opaque cursor pagination.', - errors: [...VALIDATED_ERRORS, 'NotFound'], + errors: RESOURCE_ERRORS, success: { description: 'A page of knowledge documents.' }, }), { @@ -362,7 +361,7 @@ const routes = [ operationId: 'abortKnowledgeDocumentUpload', summary: 'Abort Document Upload', description: 'Abort an incomplete upload and discard provider-side multipart state.', - errors: [...WORKSPACE_ERRORS, 'NotFound', 'Conflict'], + errors: RESOURCE_CONFLICT_ERRORS, success: { description: 'The aborted upload session.' }, }), { @@ -398,7 +397,7 @@ const routes = [ operationId: 'createKnowledgeDocumentUploadPartUrls', summary: 'Create Document Upload Part URLs', description: 'Issue short-lived signed PUT URLs for up to 100 multipart part numbers.', - errors: [...WORKSPACE_ERRORS, 'NotFound', 'Conflict'], + errors: RESOURCE_CONFLICT_ERRORS, success: { description: 'Signed URLs for the requested upload parts.' }, }), { @@ -478,7 +477,7 @@ const routes = [ operationId: 'getKnowledgeDocument', summary: 'Get Document', description: 'Retrieve document detail, processing state, and connector provenance.', - errors: [...VALIDATED_ERRORS, 'NotFound'], + errors: RESOURCE_ERRORS, success: { description: 'The requested knowledge document.' }, }), { @@ -509,7 +508,7 @@ const routes = [ summary: 'Delete Document', description: 'Remove one document from a knowledge base. What that means depends on the document. A directly uploaded document is deleted outright along with its indexed chunks. A connector-backed document is instead excluded: its row survives, marked excluded and disabled so it stops being searchable and a later connector sync does not re-add it, and its embeddings are not deleted. Either way the document no longer appears in listings or search results.', - errors: [...WORKSPACE_ERRORS, 'NotFound'], + errors: RESOURCE_ERRORS, success: { description: 'Knowledge document deletion acknowledgement.' }, }), { diff --git a/apps/sim/lib/api/contracts/v2/openapi/shared.ts b/apps/sim/lib/api/contracts/v2/openapi/shared.ts index 473def99c60..ec42635525e 100644 --- a/apps/sim/lib/api/contracts/v2/openapi/shared.ts +++ b/apps/sim/lib/api/contracts/v2/openapi/shared.ts @@ -12,13 +12,6 @@ export const RATE_LIMIT_HEADERS = [ 'X-RateLimit-Reset', ] as const -export const STANDARD_ERRORS = [ - 'Unauthorized', - 'RateLimited', - 'InternalError', - 'ServiceUnavailable', -] as const - export const WORKSPACE_ERRORS = [ 'BadRequest', 'Unauthorized', @@ -74,17 +67,7 @@ export const ERROR_RESPONSES = { export type ErrorResponseId = keyof typeof ERROR_RESPONSES /** - * {@link STANDARD_ERRORS} plus the 400 that any operation parsing required path, - * query, header, or body input returns when that input fails contract validation. - * `STANDARD_ERRORS` alone is only correct for an operation with nothing to parse. - */ -export const VALIDATED_ERRORS = [ - 'BadRequest', - ...STANDARD_ERRORS, -] as const satisfies readonly ErrorResponseId[] - -/** - * The three sets below are the only shapes every workspace-scoped resource + * The three sets below are the base shapes every workspace-scoped resource * operation in the v2 API actually emits, so they live here once rather than * being re-derived per domain. Eight per-domain aliases previously denoted these * same three sets under names that implied distinctions the generated spec never diff --git a/apps/sim/lib/api/server/routes/definition.test.ts b/apps/sim/lib/api/server/routes/definition.test.ts index dce09b578d0..046c903c74a 100644 --- a/apps/sim/lib/api/server/routes/definition.test.ts +++ b/apps/sim/lib/api/server/routes/definition.test.ts @@ -6,6 +6,7 @@ import { describe, expect, it } from 'vitest' import { z } from 'zod' import { defineRouteContract } from '@/lib/api/contracts' import { + methodMatchesContract, requireBinaryRouteDefinition, requireJsonRouteDefinition, } from '@/lib/api/server/routes/definition' @@ -112,3 +113,26 @@ describe('declarative route definition invariants', () => { ).toThrow('does not match') }) }) + +describe('methodMatchesContract', () => { + it.each(['GET', 'POST', 'PUT', 'PATCH', 'DELETE'] as const)( + 'accepts %s against its own contract', + (method) => { + expect(methodMatchesContract(method, method)).toBe(true) + } + ) + + it('accepts HEAD against a GET contract, which is how Next serves it', () => { + expect(methodMatchesContract('HEAD', 'GET')).toBe(true) + }) + + it.each([ + ['HEAD', 'POST'], + ['HEAD', 'DELETE'], + ['POST', 'GET'], + ['GET', 'DELETE'], + ['PATCH', 'PUT'], + ] as const)('rejects %s against a %s contract', (requestMethod, contractMethod) => { + expect(methodMatchesContract(requestMethod, contractMethod)).toBe(false) + }) +}) diff --git a/apps/sim/lib/api/server/routes/definition.ts b/apps/sim/lib/api/server/routes/definition.ts index 9a08a13128a..248eb1b8557 100644 --- a/apps/sim/lib/api/server/routes/definition.ts +++ b/apps/sim/lib/api/server/routes/definition.ts @@ -64,3 +64,25 @@ export function requireBinaryRouteDefinition( } return { successStatus, successStatuses: [successStatus] } } + +/** + * Whether an incoming request's method is the one its contract declares. + * + * `HEAD` satisfies a `GET` contract because Next aliases a missing `HEAD` + * export straight to the `GET` handler + * (`auto-implement-methods.ts`: `methods.HEAD = handlers.GET`) and then drops + * the body when sending (`send-response.ts` skips the stream when + * `req.method === 'HEAD'`). So the handler legitimately runs with + * `request.method === 'HEAD'` against a `GET` contract, and rejecting that made + * every v2 read answer 500 to a plain `HEAD` — the request health checkers, + * uptime monitors, and link checkers send. RFC 9110 §9.3.2 makes HEAD identical + * to GET but for the body, which is exactly what running the GET path and + * letting the framework strip the body produces. + * + * Everything else stays a hard error: a handler exported under the wrong verb is + * a wiring mistake that should fail loudly rather than serve the wrong contract. + */ +export function methodMatchesContract(requestMethod: string, contractMethod: string): boolean { + if (requestMethod === contractMethod) return true + return requestMethod === 'HEAD' && contractMethod === 'GET' +} diff --git a/apps/sim/lib/api/server/routes/internal-binary-route.ts b/apps/sim/lib/api/server/routes/internal-binary-route.ts index 58ec51ec4f1..385b21ebc45 100644 --- a/apps/sim/lib/api/server/routes/internal-binary-route.ts +++ b/apps/sim/lib/api/server/routes/internal-binary-route.ts @@ -1,7 +1,10 @@ import type { Principal, SessionPrincipal } from '@sim/auth/principal' import type { NextRequest } from 'next/server' import { NextResponse } from 'next/server' -import { requireBinaryRouteDefinition } from '@/lib/api/server/routes/definition' +import { + methodMatchesContract, + requireBinaryRouteDefinition, +} from '@/lib/api/server/routes/definition' import { type InternalErrorPolicy, InternalUnauthenticatedError, @@ -72,7 +75,7 @@ export function defineInternalBinaryRoute< const wrapped = withRouteHandler( async (request, context) => { - if (request.method !== options.contract.method) { + if (!methodMatchesContract(request.method, options.contract.method)) { throw new Error( `Route received ${request.method} for ${options.contract.method} contract ${options.contract.path}` ) diff --git a/apps/sim/lib/api/server/routes/internal-json-route.ts b/apps/sim/lib/api/server/routes/internal-json-route.ts index 70d6bb0601c..bc0af861a23 100644 --- a/apps/sim/lib/api/server/routes/internal-json-route.ts +++ b/apps/sim/lib/api/server/routes/internal-json-route.ts @@ -7,7 +7,10 @@ import type { import type { NextRequest } from 'next/server' import { NextResponse } from 'next/server' import type { ContractJsonResponse } from '@/lib/api/contracts' -import { requireJsonRouteDefinition } from '@/lib/api/server/routes/definition' +import { + methodMatchesContract, + requireJsonRouteDefinition, +} from '@/lib/api/server/routes/definition' import { responseWithRequestId, withRequestId } from '@/lib/api/server/routes/request-id' import type { JsonApiRouteContract, @@ -292,7 +295,7 @@ export function defineInternalJsonRoute< const wrapped = withRouteHandler( async (request, context) => { - if (request.method !== options.contract.method) { + if (!methodMatchesContract(request.method, options.contract.method)) { throw new Error( `Route received ${request.method} for ${options.contract.method} contract ${options.contract.path}` ) diff --git a/apps/sim/lib/api/server/routes/v2-binary-route.ts b/apps/sim/lib/api/server/routes/v2-binary-route.ts index 1802816a19a..6709d65e48d 100644 --- a/apps/sim/lib/api/server/routes/v2-binary-route.ts +++ b/apps/sim/lib/api/server/routes/v2-binary-route.ts @@ -1,5 +1,8 @@ import type { NextRequest } from 'next/server' -import { requireBinaryRouteDefinition } from '@/lib/api/server/routes/definition' +import { + methodMatchesContract, + requireBinaryRouteDefinition, +} from '@/lib/api/server/routes/definition' import type { BinaryApiRouteContract, BinaryRouteDefinition, @@ -44,7 +47,7 @@ export function defineV2BinaryRoute< const wrapped = withRouteHandler( async (request: NextRequest, context) => { - if (request.method !== options.contract.method) { + if (!methodMatchesContract(request.method, options.contract.method)) { throw new Error( `Route received ${request.method} for ${options.contract.method} contract ${options.contract.path}` ) diff --git a/apps/sim/lib/api/server/routes/v2-body-lifecycle-route.ts b/apps/sim/lib/api/server/routes/v2-body-lifecycle-route.ts index 79767ef4641..b8c8e84466c 100644 --- a/apps/sim/lib/api/server/routes/v2-body-lifecycle-route.ts +++ b/apps/sim/lib/api/server/routes/v2-body-lifecycle-route.ts @@ -1,7 +1,10 @@ import type { NextRequest } from 'next/server' import { NextResponse } from 'next/server' import type { ContractJsonResponse } from '@/lib/api/contracts' -import { requireJsonRouteDefinition } from '@/lib/api/server/routes/definition' +import { + methodMatchesContract, + requireJsonRouteDefinition, +} from '@/lib/api/server/routes/definition' import type { JsonApiRouteContract, JsonNextRouteHandler, @@ -114,7 +117,7 @@ export function defineV2BodyLifecycleRoute< const wrapped = withRouteHandler( async (request, context) => { - if (request.method !== options.contract.method) { + if (!methodMatchesContract(request.method, options.contract.method)) { throw new Error( `Route received ${request.method} for ${options.contract.method} contract ${options.contract.path}` ) diff --git a/apps/sim/lib/api/server/routes/v2-json-route.ts b/apps/sim/lib/api/server/routes/v2-json-route.ts index 960aa46415c..d186e17f796 100644 --- a/apps/sim/lib/api/server/routes/v2-json-route.ts +++ b/apps/sim/lib/api/server/routes/v2-json-route.ts @@ -1,7 +1,10 @@ import type { NextRequest } from 'next/server' import { NextResponse } from 'next/server' import { recordRateLimitSnapshot } from '@/lib/api/server/rate-limit-context' -import { requireJsonRouteDefinition } from '@/lib/api/server/routes/definition' +import { + methodMatchesContract, + requireJsonRouteDefinition, +} from '@/lib/api/server/routes/definition' import type { JsonApiRouteContract, JsonNextRouteHandler, @@ -250,7 +253,7 @@ export function defineV2JsonRoute< const wrapped = withRouteHandler( async (request, context) => { - if (request.method !== options.contract.method) { + if (!methodMatchesContract(request.method, options.contract.method)) { throw new Error( `Route received ${request.method} for ${options.contract.method} contract ${options.contract.path}` ) diff --git a/apps/sim/proxy.test.ts b/apps/sim/proxy.test.ts index 27ee25c511f..e72e879aa8a 100644 --- a/apps/sim/proxy.test.ts +++ b/apps/sim/proxy.test.ts @@ -127,7 +127,9 @@ describe('resolveApiCorsPolicy', () => { expect(policy).toEqual({ origin: 'https://app.sim.test', credentials: true, - methods: 'GET,POST,OPTIONS,PUT,DELETE', + methods: 'GET,HEAD,POST,PUT,PATCH,DELETE,OPTIONS', + exposeHeaders: + 'Retry-After, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset, X-Request-Id, X-Run-Id', headers: expect.stringContaining('Authorization'), }) }) diff --git a/apps/sim/proxy.ts b/apps/sim/proxy.ts index bda4fdf7b5b..befa9b484d9 100644 --- a/apps/sim/proxy.ts +++ b/apps/sim/proxy.ts @@ -15,8 +15,39 @@ export interface CorsPolicy { credentials: boolean methods: string headers: string + /** Response headers a browser client may read; omitted leaves the CORS default. */ + exposeHeaders?: string } +/** + * Every method the `/api` surface actually answers, for the default CORS policy. + * + * Hand-written rather than derived from the contract registry because this + * module is edge middleware: importing `lib/api/contracts` would pull Zod and + * the whole contract tree into the middleware bundle. Nothing enforces the + * correspondence — the per-route `CORS_RULES` entries below are unenforced the + * same way — so a contract that introduces a new method must add it here in the + * same change. This list previously omitted `PATCH` while 17 v2 operations used + * it, so a browser preflight for any of them failed. + * + * `HEAD` is included because Next answers it from each route's `GET` handler, + * which the route builders permit via `methodMatchesContract`. + */ +const DEFAULT_API_ALLOWED_METHODS = 'GET,HEAD,POST,PUT,PATCH,DELETE,OPTIONS' + +/** + * Response headers the `/api` surface sets that a browser client must be able to read. + * + * Without `Access-Control-Expose-Headers` a browser can read only the six + * CORS-safelisted response headers, so everything here is on the wire but + * invisible to `fetch()` — the rate-limit budget, the retry delay a 429 or 503 + * asks the caller to observe, and the ids needed to correlate a run or a support + * report. Server-to-server callers are unaffected, which is why the gap is easy + * to miss. + */ +const DEFAULT_API_EXPOSED_HEADERS = + 'Retry-After, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset, X-Request-Id, X-Run-Id' + const DEFAULT_API_ALLOWED_HEADERS = 'X-CSRF-Token, X-Requested-With, Accept, Accept-Version, Content-Length, Content-MD5, Content-Type, Date, X-Api-Version, X-API-Key, Authorization' @@ -109,8 +140,9 @@ export function resolveApiCorsPolicy(request: NextRequest): CorsPolicy { return { origin: getEnv('NEXT_PUBLIC_APP_URL') || 'http://localhost:3001', credentials: true, - methods: 'GET,POST,OPTIONS,PUT,DELETE', + methods: DEFAULT_API_ALLOWED_METHODS, headers: DEFAULT_API_ALLOWED_HEADERS, + exposeHeaders: DEFAULT_API_EXPOSED_HEADERS, } } @@ -121,6 +153,9 @@ function applyCorsHeaders(response: NextResponse, policy: CorsPolicy): void { response.headers.set('Access-Control-Allow-Credentials', String(policy.credentials)) response.headers.set('Access-Control-Allow-Methods', policy.methods) response.headers.set('Access-Control-Allow-Headers', policy.headers) + if (policy.exposeHeaders) { + response.headers.set('Access-Control-Expose-Headers', policy.exposeHeaders) + } if (policy.origin !== '*') { response.headers.set('Vary', 'Origin') } diff --git a/scripts/check-route-verbs.ts b/scripts/check-route-verbs.ts index a1d069bcf2d..277309de866 100644 --- a/scripts/check-route-verbs.ts +++ b/scripts/check-route-verbs.ts @@ -5,10 +5,11 @@ * * All five declarative route builders — `defineV2JsonRoute`, * `defineV2BinaryRoute`, `defineV2BodyLifecycleRoute`, `defineInternalJsonRoute` - * and `defineInternalBinaryRoute` — compare `request.method` against - * `contract.method` and throw when they differ. That check only fires at - * RUNTIME, on a real request, and Next.js routes purely by the exported symbol - * name. So a half-finished rename — `export const PUT` still holding a contract + * and `defineInternalBinaryRoute` — throw when `request.method` is not the one + * `contract.method` declares, `methodMatchesContract` allowing only a `HEAD` + * request against a `GET` contract. That check only fires at RUNTIME, on a real + * request, and Next.js routes purely by the exported symbol name. So a + * half-finished rename — `export const PUT` still holding a contract * that declares `PATCH` — produces a 500 on the verb clients actually call and * a 405 on the one they do not, while type-check, tests and every existing * audit stay green. The mismatch is invisible until production traffic hits it.