Skip to content

Commit f3b2559

Browse files
committed
fix(knowledge): close review findings on the sim-native connectors
Pre-landing review surfaced three real defects, two of them found independently by more than one reviewer. Stop PATCH wiping tagSlotMapping. Update replaces sourceConfig wholesale, and sanitizing the caller payload stripped the server-derived slot mapping that is written once at creation and never re-sent. Every connector that declares tagDefinitions would have silently stopped writing tags after any edit, not just the new ones. Server-owned keys are now carried forward from the stored row, with the never-persisted keys kept distinct from the server-owned-and-persisted one. Gate both connectors on secret provenance. The manual knowledge-base upload path refuses a workspace file whose provenance is unavailable, but the sync engine re-uploads extracted text under a fresh kb/ key, so the processor's check resolved zero rows and passed vacuously. Agent memory had no check at all, while every other reader of memory.data pairs it with its provenance sidecar. Both now skip unsafe items visibly instead of indexing them. Also: cover resolveConnectorAuth, which rewrote credential resolution for every connector and had no tests; cover sanitizeConnectorSourceConfig, a stated tenancy control with none; assert the LIKE-escaping wiring rather than only the helper, verified by mutation; drop two vacuous assertions that could not fail; make collectsCredential exhaustive like the other four branch points; remove a provably dead branch in normalizeExt.
1 parent 4fc1ac5 commit f3b2559

9 files changed

Lines changed: 381 additions & 17 deletions

File tree

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

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,10 @@ import { hasWorkspaceLiveSyncAccess } from '@/lib/billing/core/subscription'
1212
import { generateRequestId } from '@/lib/core/utils/request'
1313
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1414
import { resolveCredentialTokenIdentity } from '@/lib/credentials/access'
15-
import { sanitizeConnectorSourceConfig } from '@/lib/knowledge/connectors/source-config'
15+
import {
16+
preserveServerOwnedSourceConfig,
17+
sanitizeConnectorSourceConfig,
18+
} from '@/lib/knowledge/connectors/source-config'
1619
import { deleteDocumentStorageFiles } from '@/lib/knowledge/documents/service'
1720
import { cleanupUnusedTagDefinitions } from '@/lib/knowledge/tags/service'
1821
import { captureServerEvent } from '@/lib/posthog/server'
@@ -110,6 +113,8 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Rout
110113
*/
111114
const sourceConfigUpdate =
112115
body.sourceConfig === undefined ? undefined : sanitizeConnectorSourceConfig(body.sourceConfig)
116+
/** Sanitized update plus the server-owned keys carried over from the stored row. */
117+
let sourceConfigToPersist: Record<string, unknown> | undefined
113118

114119
if (
115120
body.syncIntervalMinutes !== undefined &&
@@ -151,6 +156,10 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Rout
151156
}
152157

153158
const existing = existingRows[0]
159+
sourceConfigToPersist = preserveServerOwnedSourceConfig(
160+
sourceConfigUpdate,
161+
existing.sourceConfig
162+
)
154163
const connectorConfig = CONNECTOR_REGISTRY[existing.connectorType]
155164

156165
if (!connectorConfig) {
@@ -247,8 +256,8 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Rout
247256
}
248257

249258
const updates: Record<string, unknown> = { updatedAt: new Date() }
250-
if (sourceConfigUpdate !== undefined) {
251-
updates.sourceConfig = sourceConfigUpdate
259+
if (sourceConfigToPersist !== undefined) {
260+
updates.sourceConfig = sourceConfigToPersist
252261
}
253262
if (body.syncIntervalMinutes !== undefined) {
254263
updates.syncIntervalMinutes = body.syncIntervalMinutes

apps/sim/connectors/sim-conversations/sim-conversations.test.ts

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,12 @@ import {
1414
renderTranscript,
1515
} from '@/connectors/sim-conversations/sim-conversations'
1616

17+
/** Shape the drizzle `sql` mock produces (see packages/testing database.mock). */
18+
interface SqlFragment {
19+
strings?: readonly string[]
20+
values?: unknown[]
21+
}
22+
1723
const BASE_ROW: ConversationRow = {
1824
id: 'mem-1',
1925
key: 'support-123',
@@ -57,13 +63,16 @@ describe('escapeLikePrefix', () => {
5763
})
5864

5965
describe('buildConversationListingFilters', () => {
60-
/** The tenancy invariant: only the engine-supplied workspace can scope the query. */
61-
it('always binds the supplied workspace, and nothing else can widen it', () => {
66+
/**
67+
* The builder takes no `sourceConfig`, so this proves only that the supplied
68+
* workspace is bound. That the connector never READS `sourceConfig.workspaceId`
69+
* is proven end to end by the sync harness, not here.
70+
*/
71+
it('binds the supplied workspace', () => {
6272
const nodes = conditionsOf({ workspaceId: 'ws-real', prefix: '' })
6373
const workspaceClause = nodes.find((node) => node.left === 'workspaceId')
6474

6575
expect(workspaceClause).toMatchObject({ type: 'eq', right: 'ws-real' })
66-
expect(JSON.stringify(nodes)).not.toContain('victim-ws')
6776
})
6877

6978
/**
@@ -80,6 +89,22 @@ describe('buildConversationListingFilters', () => {
8089
expect(conditionsOf({ workspaceId: 'ws-1', prefix: 'support-' }).length).toBeGreaterThan(2)
8190
})
8291

92+
/**
93+
* Asserts the WIRING, not just that `escapeLikePrefix` works in isolation.
94+
* Without this, deleting the escape call from the builder still passes every
95+
* other test while a prefix of `%` exports every conversation in the workspace.
96+
*/
97+
it('binds the ESCAPED prefix as the LIKE parameter', () => {
98+
const filters = buildConversationListingFilters({ workspaceId: 'ws-1', prefix: '100%_done' })
99+
const bound = JSON.stringify(filters.map((f) => (f as unknown as SqlFragment).values ?? null))
100+
101+
expect(bound).toContain('100\\\\%\\\\_done%')
102+
expect(bound).not.toContain('"100%_done%"')
103+
expect(
104+
JSON.stringify(filters.map((f) => (f as unknown as SqlFragment).strings ?? null))
105+
).toContain('ESCAPE')
106+
})
107+
83108
it('adds a keyset clause only when paginating', () => {
84109
const first = conditionsOf({ workspaceId: 'ws-1', prefix: '' })
85110
const next = conditionsOf({

apps/sim/connectors/sim-conversations/sim-conversations.ts

Lines changed: 38 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { db } from '@sim/db'
2-
import { memory } from '@sim/db/schema'
2+
import { memory, memorySecretProvenance } from '@sim/db/schema'
33
import { and, asc, eq, gt, isNull, or, type SQL, sql } from 'drizzle-orm'
4+
import { readBoundMemorySecretProvenance } from '@/lib/memory/secret-provenance'
45
import { simConversationsConnectorMeta } from '@/connectors/sim-conversations/meta'
56
import type {
67
ConnectorConfig,
@@ -283,9 +284,18 @@ export const simConversationsConnector: ConnectorConfig = {
283284
const workspaceId = syncContext.workspaceId
284285

285286
// Re-read through the same predicates: never fetch by external id alone.
287+
// Left-joins the provenance sidecar so the secret check below has its inputs.
286288
const rows = await db
287-
.select({ ...CONVERSATION_ROW_COLUMNS, data: memory.data })
289+
.select({
290+
...CONVERSATION_ROW_COLUMNS,
291+
data: memory.data,
292+
secretProvenanceVersion: memory.secretProvenanceVersion,
293+
provenanceContentHash: memorySecretProvenance.contentHash,
294+
provenanceStatus: memorySecretProvenance.status,
295+
provenanceEntries: memorySecretProvenance.entries,
296+
})
288297
.from(memory)
298+
.leftJoin(memorySecretProvenance, eq(memorySecretProvenance.memoryId, memory.id))
289299
.where(
290300
and(
291301
eq(memory.id, externalId),
@@ -299,6 +309,32 @@ export const simConversationsConnector: ConnectorConfig = {
299309
if (!row) return null
300310

301311
const stub = conversationToStub(row)
312+
313+
/**
314+
* Agent memory is where resolved credentials and env values land in message
315+
* text, so every other reader of `memory.data` pairs it with this sidecar
316+
* (see `app/api/memory/route.ts`). Indexing a transcript copies it into KB
317+
* chunks and embeddings, which are readable by anyone with *any* permission on
318+
* the workspace — a wider audience than the write/admin needed to create the
319+
* connector — so only a provably secret-free conversation is indexed.
320+
*
321+
* `readBoundMemorySecretProvenance` returns exact-empty for untracked legacy
322+
* rows and `unknown` for malformed ones, so this fails closed.
323+
*/
324+
const provenance = readBoundMemorySecretProvenance({
325+
secretProvenanceVersion: row.secretProvenanceVersion,
326+
data: row.data,
327+
provenanceContentHash: row.provenanceContentHash,
328+
status: row.provenanceStatus,
329+
entries: row.provenanceEntries,
330+
})
331+
if (provenance.status !== 'exact' || provenance.entries.length > 0) {
332+
return markSkipped(
333+
stub,
334+
'Conversation contains secret-derived values or its provenance is unavailable, so it was not indexed'
335+
)
336+
}
337+
302338
const content = renderTranscript(
303339
{
304340
conversationId: row.key,

apps/sim/connectors/sim-files/sim-files.test.ts

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -137,16 +137,15 @@ describe('fileRowToStub', () => {
137137

138138
describe('buildFileListingFilters', () => {
139139
/**
140-
* The tenancy invariant of the whole connector: the workspace comes from the sync
141-
* engine, and a `sourceConfig` carrying its own `workspaceId` must be inert. This
142-
* asserts the filters are built solely from the passed workspace.
140+
* The builder takes no `sourceConfig`, so this proves only that the supplied
141+
* workspace is bound. That the connector never READS `sourceConfig.workspaceId`
142+
* is proven end to end by the sync harness, not here.
143143
*/
144-
it('always binds the supplied workspace, and nothing else can widen it', () => {
144+
it('binds the supplied workspace', () => {
145145
const nodes = conditionsOf({ workspaceId: 'ws-real', folderIds: null, rootOnly: false })
146146
const workspaceClause = nodes.find((node) => node.left === 'workspaceId')
147147

148148
expect(workspaceClause).toMatchObject({ type: 'eq', right: 'ws-real' })
149-
expect(JSON.stringify(nodes)).not.toContain('victim-ws')
150149
})
151150

152151
/**

apps/sim/connectors/sim-files/sim-files.ts

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,10 @@ import {
1010
fetchServableWorkspaceFileBuffer,
1111
getWorkspaceFile,
1212
} from '@/lib/uploads/contexts/workspace/workspace-file-manager'
13+
import {
14+
isModelSafeWorkspaceFileKey,
15+
MODEL_UNSAFE_WORKSPACE_FILE_ERROR_MESSAGE,
16+
} from '@/lib/uploads/contexts/workspace/workspace-file-secret-provenance'
1317
import { simFilesConnectorMeta } from '@/connectors/sim-files/meta'
1418
import type {
1519
ConnectorConfig,
@@ -93,8 +97,7 @@ export function decodeCursor(cursor: string): Cursor {
9397
export function normalizeExt(value: string): string {
9498
const trimmed = value.trim().toLowerCase()
9599
const dot = trimmed.lastIndexOf('.')
96-
const ext = dot === -1 ? trimmed : trimmed.slice(dot + 1)
97-
return ext === trimmed && dot === -1 ? trimmed.replace(/^\.+/, '') : ext
100+
return dot === -1 ? trimmed : trimmed.slice(dot + 1)
98101
}
99102

100103
/**
@@ -410,6 +413,22 @@ export const simFilesConnector: ConnectorConfig = {
410413
const fileRecord = await getWorkspaceFile(workspaceId, externalId, { throwOnError: true })
411414
if (!fileRecord) return null
412415

416+
/**
417+
* The same gate the manual knowledge-base upload path enforces
418+
* (`assertDocumentFileModelSafe` in `documents/document-processor.ts`).
419+
*
420+
* It has to run HERE rather than being inherited: the sync engine re-uploads the
421+
* extracted text under a fresh `kb/...` key, and that key has no `workspace_files`
422+
* row — so the processor's own check resolves zero rows and passes vacuously. A
423+
* file whose provenance is unknown would otherwise be laundered into embeddings.
424+
*
425+
* Skipped rather than dropped so it surfaces as a visible failed document.
426+
*/
427+
const provenanceSafe = await isModelSafeWorkspaceFileKey(fileRecord.key, { workspaceId })
428+
if (!provenanceSafe) {
429+
return markSkipped(stub, MODEL_UNSAFE_WORKSPACE_FILE_ERROR_MESSAGE)
430+
}
431+
413432
let buffer: Buffer
414433
try {
415434
/**

apps/sim/connectors/types.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,17 @@ export type ConnectorAuthConfig =
3232
* field's readiness gate, so the two can never disagree.
3333
*/
3434
export function collectsCredential(auth: ConnectorAuthConfig): boolean {
35-
return auth.mode !== 'sim'
35+
switch (auth.mode) {
36+
case 'sim':
37+
return false
38+
case 'apiKey':
39+
case 'oauth':
40+
return true
41+
default: {
42+
const _exhaustive: never = auth
43+
return true
44+
}
45+
}
3646
}
3747

3848
/**
Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { describe, expect, it } from 'vitest'
5+
import {
6+
preserveServerOwnedSourceConfig,
7+
RESERVED_SOURCE_CONFIG_KEYS,
8+
sanitizeConnectorSourceConfig,
9+
} from '@/lib/knowledge/connectors/source-config'
10+
11+
describe('sanitizeConnectorSourceConfig', () => {
12+
/**
13+
* The tenancy control for `sim`-mode connectors: the engine derives the workspace
14+
* from the knowledge_base row, so a caller-supplied one must never be persisted
15+
* where a connector could read it back.
16+
*/
17+
it('strips every reserved key a caller could use to widen scope', () => {
18+
expect(
19+
sanitizeConnectorSourceConfig({
20+
workspaceId: 'victim-ws',
21+
knowledgeBaseId: 'victim-kb',
22+
tagSlotMapping: { folderPath: 'tag7' },
23+
folderId: 'f-1',
24+
recursive: 'false',
25+
})
26+
).toEqual({ folderId: 'f-1', recursive: 'false' })
27+
})
28+
29+
it('covers the whole declared reserved list', () => {
30+
const everyReserved = Object.fromEntries(RESERVED_SOURCE_CONFIG_KEYS.map((k) => [k, 'x']))
31+
expect(sanitizeConnectorSourceConfig(everyReserved)).toEqual({})
32+
})
33+
34+
it('leaves unreserved keys untouched, including falsy values', () => {
35+
const input = { folderId: '', recursive: 'false', maxFiles: 0 }
36+
expect(sanitizeConnectorSourceConfig(input)).toEqual(input)
37+
})
38+
39+
it('does not mutate the caller object', () => {
40+
const input = { workspaceId: 'victim-ws', folderId: 'f-1' }
41+
sanitizeConnectorSourceConfig(input)
42+
expect(input.workspaceId).toBe('victim-ws')
43+
})
44+
})
45+
46+
describe('preserveServerOwnedSourceConfig', () => {
47+
/**
48+
* Update replaces `sourceConfig` wholesale. Without this, sanitizing would drop
49+
* `tagSlotMapping` on every edit and the connector would silently stop writing
50+
* tags — for every connector that declares tagDefinitions, not just the sim ones.
51+
*/
52+
it('carries the stored tagSlotMapping across an edit that does not resend it', () => {
53+
expect(
54+
preserveServerOwnedSourceConfig(
55+
{ folderId: 'new-folder' },
56+
{ folderId: 'old-folder', tagSlotMapping: { folderPath: 'tag1' } }
57+
)
58+
).toEqual({ folderId: 'new-folder', tagSlotMapping: { folderPath: 'tag1' } })
59+
})
60+
61+
/** The stored mapping wins: a caller cannot claim slots it was not allocated. */
62+
it('prefers the stored mapping over anything left in the update', () => {
63+
const result = preserveServerOwnedSourceConfig(
64+
{ tagSlotMapping: { folderPath: 'tag7' } } as Record<string, unknown>,
65+
{ tagSlotMapping: { folderPath: 'tag1' } }
66+
)
67+
expect(result.tagSlotMapping).toEqual({ folderPath: 'tag1' })
68+
})
69+
70+
/** workspaceId/knowledgeBaseId are never persisted, so nothing should resurrect them. */
71+
it('does not resurrect keys that are never persisted', () => {
72+
const result = preserveServerOwnedSourceConfig(
73+
{ folderId: 'f-1' },
74+
{ workspaceId: 'victim-ws', knowledgeBaseId: 'victim-kb' }
75+
)
76+
expect(result).toEqual({ folderId: 'f-1' })
77+
})
78+
79+
it('tolerates a connector row with no stored config', () => {
80+
expect(preserveServerOwnedSourceConfig({ folderId: 'f-1' }, null)).toEqual({ folderId: 'f-1' })
81+
expect(preserveServerOwnedSourceConfig({ folderId: 'f-1' }, undefined)).toEqual({
82+
folderId: 'f-1',
83+
})
84+
})
85+
})

apps/sim/lib/knowledge/connectors/source-config.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,3 +27,34 @@ export function sanitizeConnectorSourceConfig(
2727
): Record<string, unknown> {
2828
return omit(sourceConfig, [...RESERVED_SOURCE_CONFIG_KEYS])
2929
}
30+
31+
/**
32+
* The reserved keys that are legitimately *persisted*, just never by the caller.
33+
*
34+
* `workspaceId` and `knowledgeBaseId` are stripped and never stored at all — the
35+
* engine derives them per run. `tagSlotMapping` is different: it is computed once
36+
* during creation from the knowledge base's free slots and must survive edits.
37+
*/
38+
const SERVER_OWNED_PERSISTED_KEYS = ['tagSlotMapping'] as const
39+
40+
/**
41+
* Re-applies server-owned keys from the stored row onto a sanitized update.
42+
*
43+
* Update replaces `sourceConfig` wholesale, so sanitizing alone would drop
44+
* `tagSlotMapping` — which the client never re-sends. Losing it makes
45+
* `resolveTagMapping` return undefined and the connector silently stops writing
46+
* tags on every later sync, for every connector that declares `tagDefinitions`.
47+
* Reading it back from the stored row rather than the request keeps the key
48+
* server-owned while still surviving an edit.
49+
*/
50+
export function preserveServerOwnedSourceConfig(
51+
sanitizedUpdate: Record<string, unknown>,
52+
storedSourceConfig: unknown
53+
): Record<string, unknown> {
54+
const stored = (storedSourceConfig ?? {}) as Record<string, unknown>
55+
const preserved: Record<string, unknown> = {}
56+
for (const key of SERVER_OWNED_PERSISTED_KEYS) {
57+
if (stored[key] !== undefined) preserved[key] = stored[key]
58+
}
59+
return { ...sanitizedUpdate, ...preserved }
60+
}

0 commit comments

Comments
 (0)