improvement(files): make the row the documented owner of module context - #6803
Conversation
Follows the serve fix by stating the contract it relies on, so the next reader does not re-derive module ownership from the key prefix. The prefix is authoritative for where the bytes live — bucket and tenant — and nothing more. Which module owns an object is `workspace_files.context`, which is server-authored like the key but, unlike the key, mutable: a chat attachment becomes a workspace file when `materialize_file` flips that column, and encoding a mutable fact in an immutable key would mean copying the bytes on every such transition just to restate them. `resolveTrustedFileContext` claimed the prefix was flatly authoritative. That claim is what made routing on it look safe. It is now scoped to what it actually defends — a caller-supplied context can still never relabel a private key — and `resolveStoredFileContext` is documented as the sanctioned way to ask who owns an object rather than as a workaround. `verifyWorkspaceFileAccess` resolved its binding with the lookup filtered to `context = 'workspace'`, so an attachment missed the row and fell through to object metadata, which cannot see a soft delete. It now matches either workspace-scoped context, which is also what every caller already wanted: the LLM-attachment and presigned-URL paths pass 'workspace' for attachment keys today. A soft-deleted attachment is now denied on all of them. The parse route carried the same gate and labelled parsed attachments with the raw storage segment instead of the uploaded filename. Module-scoped filters are deliberately untouched: the Files module, its folder manager, forking and the workspace-file use cases all match `context = 'workspace'` because they mean the Files module, not the bucket.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview Authorization and parse no longer look up bindings with
New authorization tests mock Reviewed by Cursor Bugbot for commit 4965459. Configure here. |
Greptile SummaryThis PR clarifies that storage-key prefixes identify bucket and tenancy while the persisted row identifies module ownership. It also broadens workspace-scoped authorization and parse metadata lookups to handle both Files-module files and mothership attachments, including consistent denial of soft-deleted attachments.
Confidence Score: 5/5The PR appears safe to merge, with no concrete changed-code failure remaining after checking metadata uniqueness, authorization reachability, deletion handling, and module-routing behavior. Active metadata is unique per storage key, the broadened lookups accept only the two explicitly workspace-scoped contexts, and both contexts continue through the owning workspace permission and soft-delete checks.
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/files/authorization.ts | Broadens binding lookup to both allowlisted workspace-scoped contexts while retaining workspace membership and soft-delete checks. |
| apps/sim/app/api/files/authorization.test.ts | Adds focused tests for workspace and mothership bindings, deletion denial, tenant isolation, and unsupported contexts. |
| apps/sim/app/api/files/parse/route.ts | Recovers the stored original filename for either workspace-scoped context after access authorization. |
| apps/sim/lib/uploads/server/metadata.ts | Documents and implements persisted-row ownership resolution for ambiguous workspace-prefixed keys. |
| apps/sim/lib/uploads/shared/types.ts | Introduces the shared workspace-scoped context tuple, derived type, and runtime type guard. |
| apps/sim/app/api/files/serve/[...path]/route.ts | Clarifies why serving dispatch must use stored module ownership instead of the key prefix. |
| apps/sim/lib/uploads/utils/file-utils.ts | Documents that prefix inference establishes storage tenancy rather than mutable module ownership. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
K[Workspace-prefixed storage key] --> R[Read active workspace_files row]
R --> C{Stored context}
C -->|workspace| F[Files-module handling]
C -->|mothership| M[Mothership attachment handling]
C -->|missing or unsupported| I[Use prefix-inferred workspace context]
F --> A[Authorize owning workspace membership]
M --> A
I --> A
A --> D{Soft-deleted binding?}
D -->|yes| X[Deny access]
D -->|no| S[Serve or parse file]
Reviews (1): Last reviewed commit: "improvement(files): make the row the doc..." | Re-trigger Greptile
Summary
Follow-up to #6789. That PR fixed the 404 by resolving a file's module context from its stored row instead of its key prefix; this states the contract that fix relies on, so the next reader doesn't re-derive module ownership from the prefix the same way #6654 did.
workspace_files.contextis server-authored like the key, but unlike the key it is mutable —materialize_filepromotes a chat attachment to a workspace file by flipping that column. Encoding a mutable fact in an immutable key would mean copying the bytes on every transition just to restate them, which is why the prefix can't own this andexecuteSavestays a metadata-only flip.resolveTrustedFileContextclaimed the prefix was flatly "authoritative" — the claim that made routing on it look safe. Now scoped to what it actually defends: a caller-supplied context still can never relabel a private key.resolveStoredFileContextis documented as the sanctioned reader rather than a workaround.verifyWorkspaceFileAccessfiltered its binding lookup tocontext = 'workspace', so attachments missed the row and fell through to object metadata, which can't see a soft delete. It now matches either workspace-scoped context — which is what callers already wanted, since the LLM-attachment and presigned-URL paths pass'workspace'for attachment keys today. A soft-deleted attachment is now denied on all of them.context = 'workspace'because they mean the Files module, not the bucket.Considered and rejected: giving mothership its own
mothership/prefix. It would forceexecuteSaveto copy bytes to aworkspace/key on every promotion, turning an atomic metadata transaction into copy → commit → delete with orphan/stale failure modes on both sides — a data migration to restate something a column already says.Type of Change
Testing
The new authorization tests use a
getFileMetadataByKeymock that honors thecontext/includeDeletedarguments, so they actually exercise the filter — 2 fail against the old authorizer, while the workspace-context cases still pass, showing no Files-module regression. (A mock that ignored those args would have passed either way, which is how the original gate went unnoticed.)bun run test app/api/files lib/uploads lib/workspace-files— 895 pass (79 files).type-check,lint, and all 29 audits clean. Not exercised against a running app.Checklist