improvement(kbs): ownership bindings#4833
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryHigh Risk Overview Read/serve path: Upload & binding lifecycle: KB presigned (single/batch), multipart completion, and related flows require Schema: Migration adds Reviewed by Cursor Bugbot for commit 54fcedf. Configure here. |
Greptile SummaryThis PR introduces a trusted ownership layer for knowledge-base file authorization. Instead of granting KB file access by parsing
Confidence Score: 3/5The core ownership-binding logic is well-structured and correct for workspace KBs, but personal-KB documents have a storage leak on deletion and remain inaccessible for reads after this deployment. Two issues compound: (1) prior review threads confirmed null-workspace KB files lose read access permanently because hasActiveKbDocumentForKey and verifyKBFileAccess both treat null workspaceId as a denial; (2) deleteDocumentStorageFiles now also silently skips storage cleanup for those same files, so every personal-KB document deletion leaves the object in S3/Blob indefinitely with no cleanup path. Together these mean personal-KB users lose both read access and the ability to reclaim storage after deletion. apps/sim/lib/knowledge/documents/service.ts (deleteDocumentStorageFiles regression for personal-KB files) and apps/sim/app/api/files/authorization.ts (verifyKBFileAccess denial for null-workspace KBs — covered in prior threads but unresolved) Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant PresignedRoute as /api/files/presigned
participant MultipartRoute as /api/files/multipart
participant S3 as S3 / Blob
participant WorkspaceFiles as workspace_files (binding)
participant AuthZ as verifyKBFileAccess
participant Document as document.storageKey
Note over Client,Document: Upload Flow (single presign — binding written before PUT)
Client->>PresignedRoute: "POST ?type=knowledge-base&workspaceId=ws-1"
PresignedRoute->>WorkspaceFiles: insertFileMetadata (key, ws-1)
PresignedRoute-->>Client: presigned URL + key
Client->>S3: PUT object
Note over Client,Document: Upload Flow (multipart — binding written AFTER complete)
Client->>MultipartRoute: "POST action=complete"
MultipartRoute->>S3: completeMultipartUpload
S3-->>MultipartRoute: ok
MultipartRoute->>WorkspaceFiles: recordKnowledgeBaseOwnership
MultipartRoute-->>Client: "{key, location}"
Note over Client,Document: Read Authorization
Client->>AuthZ: verifyKBFileAccess(key, userId)
AuthZ->>WorkspaceFiles: getFileMetadataByKey(key)
WorkspaceFiles-->>AuthZ: "binding {workspaceId}"
AuthZ->>AuthZ: getUserEntityPermissions(userId, workspaceId)
AuthZ->>Document: hasActiveKbDocumentForKey(key, workspaceId)
Document-->>AuthZ: "rows.length > 0"
AuthZ-->>Client: true / false
Note over Client,Document: Delete Authorization
Client->>AuthZ: verifyKBFileWriteAccess(key, userId)
AuthZ->>WorkspaceFiles: getFileMetadataByKey(key)
AuthZ->>AuthZ: "permission === write|admin?"
AuthZ-->>Client: true / false
Reviews (3): Last reviewed commit: "more comments" | Re-trigger Greptile |
|
bugbot run |
|
@greptile |
|
bugbot run |
|
@greptile |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 54fcedf. Configure here.
Summary
Authorize knowledge-base file access via trusted storage-key→workspace ownership bindings (workspace_files + document.storage_key)
Type of Change
Testing
Tested manually
Checklist