Skip to content

Commit 6fdb145

Browse files
authored
fix(v2-api): standardization (#6542)
* fix(v2-api): stop leaking resolved secrets in logs and serving doc source Two regressions shipped with the v2 API (#5273) where v2 diverged from the v1 path it replaced, plus the hardening that fell out of auditing them. **v2 logs bypassed secret redaction.** `getPublicLog` and `listPublicLogs` called raw `materializeExecutionData`, while every other reader — v1 list and detail, CSV export, `fetch-log-detail`, both data-drain sources — calls `materializeExecutionDataForDisplay`, which applies the resolved-secret provenance projection. Both v2 routes then serialize `traceSpans` and `finalOutput` straight onto the wire, so unredacted secrets could reach the public API. Swapped to the display projection and threaded the principal's subject user into the read context. **v2 file download served generation source.** `GET /api/v2/files/{fileId}` streamed `file.key` raw. AI-generated docs store their generation source as the primary file, so a raw download yields source text under a `.pdf` name — a file the recipient cannot open. Generated docs now resolve to their compiled artifact; ordinary uploads still stream and are never materialized, gated on the recorded generation-source type rather than the extension. The resolve is capped at MAX_RENDERED_DOCUMENT_BYTES, and a still-compiling artifact returns a retryable 409 rather than a 500. Also in this change: - Reconcile the two v2 verbs that used PUT for PATCH semantics: `PUT /v2/knowledge/{id}` and `PUT /v2/tables/{tableId}/rows` are both all-optional partial updates. Breaking for API-key clients, but the surface is dark-launched behind the `v2-api` gate and no in-repo caller issues PUT. - Close the OpenAPI coverage blind spot that hid two routes: contract discovery was a non-recursive read of the flat `contracts/v2/` directory, so a contract in a subdirectory — or beside its non-v2 siblings, which is where the uploads contracts live — escaped the gate. The sweep is now recursive over the whole contracts tree, and the two upload data-plane routes are named in an explicit allowlist with reasons and staleness guards. - Extract `needsRenderedArtifact` so the "recorded type is authoritative, extension is fallback" rule has one home instead of being duplicated. - Extract `DocCompileUserError` into a leaf module so recognizing it no longer drags `app/api/**` and `next/server` into application modules. - Correct the stale pagination docstring in `contracts/v2/shared.ts` and pin the paged/full-set split in a test so it cannot drift again. * fix(v2-api): absolute imports for the extracted doc-compile error Review follow-up. - Use the `@/lib/...` alias for `doc-compile-error` in the three modules that imported it relatively. The repo requires absolute imports, and having all four consumers share one specifier also removes any chance of two module instances resolving apart and breaking `instanceof`. - Memoize the v2 list-pagination sweep. It re-imported the whole contracts tree once per test and timed out against the default 10s limit under load; it now sweeps once and declares an explicit timeout. Its failure message also still pointed at an enumeration in `v2/shared.ts` that this branch replaced with a pointer to the test itself. * fix(v2-api): correct three inaccurate claims found in verification None of these change behavior; each is a comment or test-config assertion that was not true as written. - The artifact resolver's TSDoc implied the byte cap prevents an oversized artifact being materialized. It does not: the artifact-store fetch is not streaming-bounded, so the bytes are resident before the ceiling rejects them. Say what it actually guarantees. - `v2/shared.ts` pointed at per-contract documentation for the two lists that still filter in memory. Neither contract documents it, so name the two lists and what they do inline instead of pointing at a page that does not exist. - The knowledge update contract said "every field of the body is optional"; `workspaceId` is required. Narrow the claim to mutable fields. - Scope the pagination sweep's extended timeout to the one test that pays for it, so a genuine hang in the other two surfaces in 10s rather than 60s.
1 parent 71ea7e5 commit 6fdb145

32 files changed

Lines changed: 682 additions & 126 deletions

apps/docs/openapi-v2-files-audit.json

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -566,7 +566,7 @@
566566
"get": {
567567
"operationId": "downloadFile",
568568
"summary": "Download File",
569-
"description": "Download the current file bytes from a workspace.",
569+
"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.",
570570
"tags": ["Files"],
571571
"parameters": [
572572
{
@@ -638,6 +638,12 @@
638638
"404": {
639639
"$ref": "#/components/responses/NotFound"
640640
},
641+
"409": {
642+
"$ref": "#/components/responses/Conflict"
643+
},
644+
"413": {
645+
"$ref": "#/components/responses/PayloadTooLarge"
646+
},
641647
"429": {
642648
"$ref": "#/components/responses/RateLimited"
643649
},

apps/docs/openapi-v2-knowledge.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -273,7 +273,7 @@
273273
}
274274
}
275275
},
276-
"put": {
276+
"patch": {
277277
"operationId": "updateKnowledgeBase",
278278
"summary": "Update Knowledge Base",
279279
"description": "Update a knowledge base name, description, chunking configuration, or folder placement.",

apps/docs/openapi-v2-tables.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -864,7 +864,7 @@
864864
}
865865
}
866866
},
867-
"put": {
867+
"patch": {
868868
"operationId": "updateTableRows",
869869
"summary": "Update Rows by Filter",
870870
"description": "Apply the same partial data patch to every row matching a non-empty predicate.",

apps/sim/app/api/files/serve/[...path]/route.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,8 @@ import type { NextRequest } from 'next/server'
44
import { NextResponse } from 'next/server'
55
import { fileServeParamsSchema, fileServeQuerySchema } from '@/lib/api/contracts/storage-transfer'
66
import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid'
7-
import {
8-
DocCompileUserError,
9-
resolveServableDocBytes,
10-
} from '@/lib/copilot/tools/server/files/doc-compile'
7+
import { resolveServableDocBytes } from '@/lib/copilot/tools/server/files/doc-compile'
8+
import { DocCompileUserError } from '@/lib/copilot/tools/server/files/doc-compile-error'
119
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1210
import { CopilotFiles, isUsingCloudStorage } from '@/lib/uploads'
1311
import type { StorageContext } from '@/lib/uploads/config'

apps/sim/app/api/v1/files/[fileId]/route.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ vi.mock('@sim/audit', () => ({
3737
}))
3838
vi.mock('@/lib/posthog/server', () => ({ captureServerEvent: vi.fn() }))
3939

40-
import { DocCompileUserError } from '@/lib/copilot/tools/server/files/doc-compile'
40+
import { DocCompileUserError } from '@/lib/copilot/tools/server/files/doc-compile-error'
4141
import { GET } from '@/app/api/v1/files/[fileId]/route'
4242

4343
const WORKSPACE_ID = 'ws-1'

apps/sim/app/api/v1/files/[fileId]/route.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import {
1010
fetchServableWorkspaceFileBuffer,
1111
getWorkspaceFile,
1212
} from '@/lib/uploads/contexts/workspace'
13-
import { docNotReadyMessage, isDocNotReadyError } from '@/lib/uploads/utils/servable-file-response'
13+
import { docNotReadyMessage, isDocNotReadyError } from '@/lib/uploads/utils/doc-not-ready'
1414
import { performDeleteWorkspaceFileItems } from '@/lib/workspace-files/orchestration'
1515
import {
1616
checkRateLimit,

apps/sim/app/api/v2/files/[fileId]/route.test.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,8 @@ describe('v2 single-file routes', () => {
111111
mocks.download.mockResolvedValue({
112112
file: fileRecord(),
113113
stream: new Blob(['id,name\n']).stream(),
114+
contentType: 'text/csv',
115+
contentLength: 'id,name\n'.length,
114116
})
115117
mocks.rename.mockResolvedValue({ file: fileRecord({ name: 'renamed.csv' }) })
116118
mocks.deleteFile.mockResolvedValue({

apps/sim/app/api/v2/files/[fileId]/route.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ export const revalidate = 0
2525
* The response carries no JSON envelope, so rate-limit state is surfaced via
2626
* `X-RateLimit-*` headers. Errors still render the canonical v2 JSON error body.
2727
* Lookups are workspace-scoped (IDOR-safe): a file in another workspace 404s.
28+
*
29+
* A generated doc whose artifact is still compiling renders `CONFLICT`; retry.
2830
*/
2931
export const GET = defineV2BinaryRoute({
3032
contract: v2DownloadFileContract,
@@ -37,11 +39,11 @@ export const GET = defineV2BinaryRoute({
3739
assertedWorkspaceId: query.workspaceId,
3840
}),
3941
useCase: downloadWorkspaceFileStream,
40-
present: ({ file, stream }) => ({
42+
present: ({ file, stream, contentType, contentLength }) => ({
4143
body: stream,
42-
contentType: file.type || 'application/octet-stream',
44+
contentType,
4345
contentDisposition: `attachment; filename="${file.name.replace(/[^\w.-]/g, '_')}"; filename*=UTF-8''${encodeURIComponent(file.name)}`,
44-
contentLength: file.size,
46+
contentLength,
4547
}),
4648
})
4749

apps/sim/app/api/v2/knowledge/[id]/route.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,8 @@ export const GET = defineV2JsonRoute({
3535
}),
3636
})
3737

38-
/** PUT /api/v2/knowledge/[id] — Update a knowledge base. */
39-
export const PUT = defineV2JsonRoute({
38+
/** PATCH /api/v2/knowledge/[id] — Partially update a knowledge base. */
39+
export const PATCH = defineV2JsonRoute({
4040
contract: v2UpdateKnowledgeBaseContract,
4141
auth: v2ApiKeyAuth,
4242
operation: knowledgeOperations.update,

apps/sim/app/api/v2/tables/[tableId]/rows/route.test.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ vi.mock('@/lib/table/application/rows', () => ({
4242
deleteTableRows: { operation: { id: 'tables.rows.delete_many' }, execute: mocks.deleteRows },
4343
}))
4444

45-
import { DELETE, GET, POST, PUT } from '@/app/api/v2/tables/[tableId]/rows/route'
45+
import { DELETE, GET, PATCH, POST } from '@/app/api/v2/tables/[tableId]/rows/route'
4646

4747
const WORKSPACE_ID = 'workspace-1'
4848
const PRINCIPAL = {
@@ -76,7 +76,7 @@ const ROW = {
7676
}
7777
const CONTEXT = { params: Promise.resolve({ tableId: 'table-1' }) }
7878

79-
function request(method: 'GET' | 'POST' | 'PUT' | 'DELETE', body?: unknown, query = '') {
79+
function request(method: 'GET' | 'POST' | 'PATCH' | 'DELETE', body?: unknown, query = '') {
8080
return new NextRequest(`http://localhost/api/v2/tables/table-1/rows${query}`, {
8181
method,
8282
headers: {
@@ -159,12 +159,12 @@ describe('/api/v2/tables/[tableId]/rows', () => {
159159

160160
it('preserves authoritative bulk update counts including a zero-match result', async () => {
161161
mocks.updateRows.mockResolvedValue({ table: TABLE, affectedCount: 0, affectedRowIds: [] })
162-
const req = request('PUT', {
162+
const req = request('PATCH', {
163163
workspaceId: WORKSPACE_ID,
164164
filter: { all: [{ field: 'name', op: 'eq', value: 'missing' }] },
165165
data: { name: 'Grace' },
166166
})
167-
const response = await PUT(req, CONTEXT)
167+
const response = await PATCH(req, CONTEXT)
168168

169169
expect(response.status).toBe(200)
170170
expect(await response.json()).toEqual({ data: { updatedCount: 0, updatedRowIds: [] } })

0 commit comments

Comments
 (0)