Skip to content

Commit 4cef8ee

Browse files
waleedlatif1waleedlatif1
committed
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.
1 parent f8644cc commit 4cef8ee

32 files changed

Lines changed: 654 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)