address timestamp concern

Author: Vikhyath Mondreti

apps/sim/lib/knowledge/documents/document-processing-source.test.ts

@@ -183,6 +183,31 @@ describe('knowledge document processing source', () => {
     expect(mockGenerateEmbeddings).not.toHaveBeenCalled()
   })
 
+  it('processes a legacy document when its workspace metadata row no longer exists', async () => {
+    mockGetFileMetadataByKeys.mockResolvedValue([])
+    mockGetBoundWorkspaceFileSecretProvenanceByMetadata.mockResolvedValue(new Map())
+
+    await processDocumentAsync('knowledge-base-1', 'document-1', {
+      filename: 'stale.pdf',
+      fileUrl: 'https://example.com/stale.pdf',
+      fileSize: 1,
+      mimeType: 'text/plain',
+    })
+
+    expect(mockProcessDocument).toHaveBeenCalledWith(
+      PERSISTED_CONTEXT.fileUrl,
+      PERSISTED_CONTEXT.filename,
+      PERSISTED_CONTEXT.mimeType,
+      1024,
+      200,
+      100,
+      PERSISTED_CONTEXT.uploadedBy,
+      null,
+      undefined,
+      undefined
+    )
+  })
+
   it('fails before parsing an existing document when its current source is tracked unknown', async () => {
     mockGetFileMetadataByKeys.mockImplementation(async (_keys: string[], context: string) =>
       context === 'workspace' ? [{ ...SOURCE_BINDING, secretProvenanceVersion: 1 }] : []

apps/sim/lib/knowledge/documents/service.ts

@@ -104,8 +104,8 @@ import { calculateCost } from '@/providers/utils'
 const logger = createLogger('DocumentService')
 
 /**
- * Thrown when a knowledge-base document's `fileUrl` references an internal `kb/`
- * storage object that is not owned by the target knowledge base's workspace.
+ * Thrown when a knowledge-base document's `fileUrl` references an internal
+ * knowledge-base storage object not owned by the target knowledge base's workspace.
  * Routes map this to a 403.
  */
 export class KnowledgeBaseFileOwnershipError extends Error {
@@ -117,17 +117,17 @@ export class KnowledgeBaseFileOwnershipError extends Error {
 
 /**
  * Guard document `fileUrl`s at creation time. When a URL points at an internal
- * `kb/` storage object, require that the target knowledge base owns the object,
+ * knowledge-base storage object, require that the target knowledge base owns the object,
  * resolved from the trusted `workspace_files` binding:
  *
  * - Workspace KB (`kbWorkspaceId` set): the binding's `workspaceId` must match.
  * - Personal KB (`kbWorkspaceId` null): the binding's `userId` must be the KB
  *   owner. A key bound to another tenant is rejected; an unbound key (legacy /
  *   never reserved) passes since it carries no cross-tenant ownership.
  *
- * External `http(s)`/`data:` URLs (ingestion sources) and non-`kb/` internal keys
+ * External `http(s)`/`data:` URLs (ingestion sources) and other internal keys
  * pass through unchanged. This blocks a user from asserting ownership of another
- * tenant's `kb/` key via a planted `fileUrl` — including in a personal KB, which
+ * tenant's object via a planted `fileUrl` — including in a personal KB, which
  * otherwise could be moved into a workspace to launder the binding. All
  * referenced bindings are resolved in one query (no N+1 inside the `FOR UPDATE`
  * window). Single-document callers pass a one-element array.
@@ -2557,8 +2557,7 @@ export async function deleteDocumentStorageFiles(
       entries
         .map((entry) => entry.storageKey)
         .filter(
-          (key): key is string =>
-            typeof key === 'string' && (key.startsWith('kb/') || key.startsWith('knowledge-base/'))
+          (key): key is string => typeof key === 'string' && isKnowledgeBaseOwnedStorageKey(key)
         )
     ),
   ]
@@ -2575,7 +2574,7 @@ export async function deleteDocumentStorageFiles(
       return
     }
 
-    if (!storageKey.startsWith('kb/') && !storageKey.startsWith('knowledge-base/')) {
+    if (!isKnowledgeBaseOwnedStorageKey(storageKey)) {
       return
     }
 
@@ -2598,13 +2597,20 @@ export async function deleteDocumentStorageFiles(
     }
 
     try {
-      await deleteFile({ key: storageKey, context: 'knowledge-base' })
-      await deleteFileMetadataByIdentity({
+      const metadataDeleted = await deleteFileMetadataByIdentity({
         id: binding.id,
         key: binding.key,
         context: binding.context,
         contentUpdatedAt: binding.contentUpdatedAt,
       })
+      if (!metadataDeleted) {
+        logger.warn(`[${requestId}] Skipping storage delete: ownership binding changed`, {
+          documentId: doc.id,
+          storageKey,
+        })
+        return
+      }
+      await deleteFile({ key: storageKey, context: 'knowledge-base' })
     } catch (error) {
       logger.warn(`[${requestId}] Failed to delete document storage file`, {
         documentId: doc.id,

apps/sim/lib/knowledge/documents/workspace-source-provenance.test.ts

@@ -292,4 +292,64 @@ describe('knowledge workspace source provenance', () => {
     expect(mockDeleteFile).not.toHaveBeenCalled()
     expect(mockDeleteFileMetadataByIdentity).not.toHaveBeenCalled()
   })
+
+  it.each(['kb', 'knowledge-base'])(
+    'deletes a trusted %s object only after its metadata identity is claimed',
+    async (keyPrefix) => {
+      const storageKey = `${keyPrefix}/owned.pdf`
+      const fileUrl = `/api/files/serve/${encodeURIComponent(storageKey)}?context=knowledge-base`
+      const binding = {
+        ...SOURCE_BINDING,
+        id: `binding-${keyPrefix}`,
+        key: storageKey,
+        context: 'knowledge-base',
+      }
+      mockGetFileMetadataByKeys.mockImplementation(async (_keys: string[], context: string) =>
+        context === 'knowledge-base' ? [binding] : []
+      )
+      mockDeleteFileMetadataByIdentity.mockResolvedValue(true)
+
+      await deleteDocumentStorageFiles(
+        [{ id: 'document-1', fileUrl, workspaceId: WORKSPACE_ID }],
+        'request-1'
+      )
+
+      expect(mockDeleteFileMetadataByIdentity).toHaveBeenCalledWith({
+        id: binding.id,
+        key: storageKey,
+        context: 'knowledge-base',
+        contentUpdatedAt: binding.contentUpdatedAt,
+      })
+      expect(mockDeleteFile).toHaveBeenCalledWith({
+        key: storageKey,
+        context: 'knowledge-base',
+      })
+      expect(mockDeleteFileMetadataByIdentity.mock.invocationCallOrder[0]).toBeLessThan(
+        mockDeleteFile.mock.invocationCallOrder[0]
+      )
+    }
+  )
+
+  it('keeps the object when its metadata identity changed before deletion', async () => {
+    const storageKey = 'kb/changed.pdf'
+    const fileUrl = `/api/files/serve/${encodeURIComponent(storageKey)}?context=knowledge-base`
+    const binding = {
+      ...SOURCE_BINDING,
+      id: 'changed-binding',
+      key: storageKey,
+      context: 'knowledge-base',
+    }
+    mockGetFileMetadataByKeys.mockImplementation(async (_keys: string[], context: string) =>
+      context === 'knowledge-base' ? [binding] : []
+    )
+    mockDeleteFileMetadataByIdentity.mockResolvedValue(false)
+
+    await deleteDocumentStorageFiles(
+      [{ id: 'document-1', fileUrl, workspaceId: WORKSPACE_ID }],
+      'request-1'
+    )
+
+    expect(mockDeleteFileMetadataByIdentity).toHaveBeenCalledOnce()
+    expect(mockDeleteFile).not.toHaveBeenCalled()
+  })
 })

apps/sim/lib/uploads/server/metadata.test.ts

@@ -3,7 +3,13 @@
  */
 import { workspaceFiles } from '@sim/db/schema'
 import { dbChainMockFns, queueTableRows, resetDbChainMock } from '@sim/testing'
+import type { SQL } from 'drizzle-orm'
+import { PgDialect } from 'drizzle-orm/pg-core'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+vi.unmock('@sim/db/schema')
+vi.unmock('drizzle-orm')
+
 import {
   ActiveFileMetadataKeyConflictError,
   deleteFileMetadataByIdentity,
@@ -28,6 +34,12 @@ describe('deleteFileMetadataByIdentity', () => {
     dbChainMockFns.returning.mockResolvedValueOnce([{ id: identity.id }])
 
     await expect(deleteFileMetadataByIdentity(identity)).resolves.toBe(true)
+    const predicate = dbChainMockFns.where.mock.calls[0]?.[0] as SQL
+    const query = new PgDialect().sqlToQuery(predicate)
+    expect(query.sql).toContain(
+      `date_trunc('milliseconds', "workspace_files"."content_updated_at")`
+    )
+    expect(query.params).toContain(identity.contentUpdatedAt)
 
     dbChainMockFns.returning.mockResolvedValueOnce([])
     await expect(deleteFileMetadataByIdentity(identity)).resolves.toBe(false)

apps/sim/lib/uploads/server/metadata.ts

@@ -353,7 +353,11 @@ export async function deleteFileMetadata(key: string): Promise<boolean> {
   return true
 }
 
-/** Soft-deletes only the exact active metadata version previously authorized by a caller. */
+/**
+ * Soft-deletes only the active metadata version previously authorized by a caller.
+ * Postgres timestamps are compared at JavaScript `Date` precision because a selected
+ * microsecond timestamp has already been rounded to milliseconds at this boundary.
+ */
 export async function deleteFileMetadataByIdentity(identity: {
   id: string
   key: string
@@ -368,7 +372,10 @@ export async function deleteFileMetadataByIdentity(identity: {
         eq(workspaceFiles.id, identity.id),
         eq(workspaceFiles.key, identity.key),
         eq(workspaceFiles.context, identity.context),
-        eq(workspaceFiles.contentUpdatedAt, identity.contentUpdatedAt),
+        eq(
+          sql<Date>`date_trunc('milliseconds', ${workspaceFiles.contentUpdatedAt})`,
+          identity.contentUpdatedAt
+        ),
         isNull(workspaceFiles.deletedAt)
       )
     )