fix(files): read a file nobody vouched for, as an untracked one already is - #6986
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview Absence vs taint is now stored separately. Sidecar status Writes without a registry persist Reviewed by Cursor Bugbot for commit 01468e0. Configure here. |
Greptile SummaryThe PR distinguishes unrecorded workspace-file provenance from deliberate taint and makes only the unrecorded state eligible for policy-controlled reads.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/uploads/contexts/workspace/workspace-file-secret-provenance.ts | Adds distinct exact, unrecorded, and unknown semantics across merge, persistence, copying, classification, and governed read boundaries. |
| packages/db/script-migrations/0007_repair_unknown_workspace_file_provenance.ts | Repairs eligible legacy unknown rows using current-binding guards and parent-first deterministic locking. |
| packages/db/migrations/0302_fat_sentry.sql | Widens the workspace-file provenance status constraint to persist unrecorded separately from unknown. |
| apps/sim/lib/execution/durable-secret-provenance-enforcement.ts | Registers workspace files as a configurable durable provenance surface with shared audit behavior. |
| apps/sim/lib/copilot/tools/server/media/ffmpeg.ts | Keeps all non-exact input provenance fail-closed for media derivation and opaque error projection. |
| apps/sim/lib/knowledge/documents/service.ts | Continues mapping every non-exact workspace-file provenance state to unknown at knowledge-ingestion boundaries. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
W[Workspace file write] --> C{Provenance decision}
C -->|Exact| E[Store exact entries]
C -->|No recorder| U[Store unrecorded]
C -->|Deliberate refusal| T[Store unknown taint]
E --> R[Read boundary]
U --> P{Workspace-file policy enforced?}
T --> D[Refuse read]
P -->|No| A[Audit and allow]
P -->|Yes| D
R --> E2{Entries empty?}
E2 -->|Yes| A2[Allow]
E2 -->|No| I[Import provenance controls]
Reviews (6): Last reviewed commit: "fix(files): copy a recorded absence as a..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
0b3fc79 to
e1a2f1f
Compare
…dy is A workspace file whose secret provenance could not be recorded was refused by every model and runtime boundary at once — attachments, mounts, and the tool routes that parse, transcribe or describe a file. A file that was never tracked at all returned exact-empty and worked fine. Both say exactly as much about their contents: nothing. So the refusal was a permanent penalty for having tried to record provenance and failed, with no way back, since nothing rewrites a file's provenance but another content write. Workspace files now do what the other durable surfaces do — proceed and record an audit entry naming the surface and the count, so the people who own the secrets are told. The state is distinguished from the four that really are unknown (missing row, version mismatch, stale binding, malformed), which stay refused; only a current sidecar that says so reads as unrecorded. Absence survives a merge rather than dropping through to exact, so a derived file cannot launder an unrecorded input into a positive claim. Script migration 0007 clears the files already stranded in the old state, taking the parent lock first in id order to match the live writer, with the unknown status re-checked under it so a concurrent write is never undone. Nothing changes for a legacy file: no sidecar row still means untracked, still reads exact-empty. The enforcement flag stays in place.
e1a2f1f to
dff6bc7
Compare
|
Rebased onto staging now that #6982 has merged — the conflict was the stacked parent's commits appearing twice after its squash-merge. This is now a single commit on top of staging, Both review findings were real and are fixed; replies on the threads. @greptileai review |
|
bugbot run |
The union carries three states; the sidecar's CHECK constraint accepts two. Only the replace path discriminated — initialize forwarded the status verbatim, so an 'unrecorded' would have reached the database as a value it rejects, aborting the enclosing transaction rather than writing a bad row. Nothing passes one today, which is why it belongs here and not in a caller. Also names the storage mismatch on the type itself: a stored 'unknown' reads back as 'unrecorded', not as the union's 'unknown'. Same word, two layers, different meanings.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Fourth round — one more real defect, found auditing every consumer of the widened union rather than reported.
Also confirmed the other direction: no consumer treats "not unknown" as safe. Every downstream branch is @greptileai review |
The repair selected on status alone, which is wider than the branch it was meant to undo. The reader answers 'unrecorded' only for a sidecar that is version 1, bound to the file's current bytes, and holding a well-formed entries array; anything else is a fault it refuses and no policy relaxes. Clearing a fault was not a smaller claim but a larger one. Deleting the sidecar sets secret_provenance_version to NULL, and a NULL version reads as exact-empty — so a file refused because its provenance described bytes it no longer holds would have come back positively vouched for, silently, with no audit entry. Four such rows exist in production today. Both the candidate query and the delete now carry every condition, so a concurrent write cannot lose the distinction between an absence and a fault. Stale, unversioned and malformed sidecars stay refused exactly as the surface refuses them, and recover as they always have, on the next content write.
Relaxing a stored `unknown` assumed it always meant "nobody recorded this". It did not. The sidecar collapsed two opposite claims into one value: bytes nobody recorded, and bytes a writer refused on purpose — a child of an archive whose parent provably held secrets, a generated asset whose safety decision came back false, a transcode whose scanner knew secrets were present and could not locate them. Relaxing the second would have walked known-secret-bearing content through every model boundary at once. The decision type already told them apart: a registry that never ran is `safe: true`, while every refusal is `safe: false`. Only storage lost it. Writers now persist `unrecorded` and `unknown` separately, readers relax the first and refuse the second, and the constraint is widened to accept it. Only workspace files store this distinction, deliberately. Every other durable surface produces a non-exact sidecar from one condition — an incomplete incoming bundle or registry — which is always an absence. Files are the only surface that derives one stored object from another, so they are the only one that can refuse on purpose. The shared policy is unchanged and uniform: read an absence and audit it, refuse a taint. Also aligns the metadata batch classifier, which answered `unknown` where the single-file reader answers `unrecorded`, leaving two classifiers describing one policy differently.
|
Fifth round. The high-severity finding was right and it invalidated this PR's premise, not just an edge case — details on the thread and in the body. Short version: the sidecar collapsed "nobody recorded this" and "a writer refused this on purpose" into one stored value, so relaxing the former relaxed the latter. Storage now keeps them apart (0302 widens the CHECK), writers persist the distinction the decision type already made, and readers relax only the absence. Also confirmed with production data that the stale-binding finding was reachable: 4 rows sit Cross-surface check, since this adds a status to one of five sidecars: memory, document, embedding and table_row each produce a non-exact sidecar from exactly one condition — an incomplete bundle or registry — which is always an absence. Files are the only surface deriving one stored object from another, so the only one able to refuse on purpose. The invariant is documented on the shared surface list so the asymmetry reads as intent. @greptileai review |
|
bugbot run |
The test that pins "closing the surface refuses an unrecorded file again" used a stored unknown, which the split had just turned into a taint. A taint is refused whatever the flag says, so the assertion held with enforcement off too and proved nothing about the switch this posture rests on. It now uses a recorded absence, and fails if the flag is ignored.
|
bugbot run |
|
bugbot run |
The copy path required an exact source, so a stored unrecorded one fell in with the refusals and the target was written as a taint the source never carried. A workspace fork or a chat file copy therefore turned a readable file into a permanently refused one — the pathology this surface exists to undo, reached by copying — and nothing rewrites a file's provenance but another content write. Safe past the scope checks it now precedes: those exist to stop one workspace's secret entries landing in another's file, and a recorded absence has no entries to carry. The legacy Function export marker keeps writing unknown. Its record had resolved secret names in scope and an unreadable bundle, so whether that is an absence is not something this change can establish, and guessing in the readable direction is what caused this round.
|
bugbot run |
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 01468e0. Configure here.
1 participant
Summary
workspace-fileis now a real surface inDURABLE_SECRET_PROVENANCE_SURFACES, where the env documentation has always offered it — setting it previously logged "unrecognized" and silently did nothing. The four governed boundaries read an unrecorded file and record the same workspace-visiblesecret_provenance.unrecordedaudit entry the other surfaces record.unknownthat a recorded absence produces. It now separates them, so the policy cannot reach the four that are not absences. Consumers that have not opted into the surface's policy (ffmpeg, knowledge ingest, the file-manage accumulator) keep refusing exactly as before.secret_provenance_version === nullstill returns exact-empty on the branch above, unchanged.0007clears the files already in the state, which would otherwise report on every read forever.Review fixes
Two real defects, both caught in review, both now fixed and pinned by tests that fail against the previous code.
An unrecorded contribution was laundered into an exact one.
mergeWorkspaceFileSecretProvenancechecked only forunknown, sounrecordedfell through to the exact branch — combining bytes nobody vouched for with bytes that were vouched for produced a positive claim that neither input made, and a derived file could hand a later boundary an exact-empty it had no basis for. Absence now survives the merge:unknown>unrecorded>exact.0007reversed the live writer's lock order. The content-update path updatesworkspace_filesand then replaces the sidecar, one transaction, parent-first. The repair deleted the sidecar and then updated the parent — opposite order, so an overlapping upload deadlocks and Postgres aborts either the deployment or a tenant's file write. It now takes the parent lock first inidorder, matching the writer and0005beside it, which is also what makes thestatus = 'unknown're-check decisive rather than racy.An initialization could send the database a value it rejects. The union carries three states; the sidecar's CHECK constraint accepts
('exact', 'unknown'). Only the replace path discriminated —initializeWorkspaceFileSecretProvenanceInTxforwarded the status verbatim, so an'unrecorded'would have hit a constraint violation and aborted the enclosing transaction. Unreachable today (both callers pass exact-empty), but the signature permitted a value the column does not, which is a landmine rather than a bug. Found while auditing every consumer of the widened union.One reported concern was checked and rejected: archive extraction flattens
'unrecorded'to'unknown'before persisting, which sounded like it would strand extracted files in the unrelaxable state. It does not —replacewrites'unknown'for any non-exact input and that reads back as'unrecorded', so both values persist identically and both round-trip to the relaxable state.The doc comment claiming there was nothing here to deadlock against was false — written without checking, one PR after the same bug was caught on the table-row repair — and is gone. The ordering is now pinned by a test over both repairs, since reasoning about it has failed twice.
The premise this PR started from was wrong
Relaxing a stored
unknownassumed it always meant "nobody recorded this". It did not. The sidecar collapsed two opposite claims into one stored value:archive.ts:327), a generated asset whosedecision.safecame back false, a transcode whose scanner hadhasSecrets: trueand could not locate them.Relaxing the second would have walked known-secret-bearing content through every model boundary at once. Both reviewers caught this independently.
The decision type already separated them — a registry that never ran returns
safe: true, every refusal returnssafe: false— and only storage lost it. Migration0302widens the CHECK to accept a third status; writers persistunrecordedandunknownseparately; readers relax the first and refuse the second. The attachment test asserts both directions on one call.Only workspace files store this distinction, and that is deliberate. On every other durable surface the non-exact sidecar comes from a single condition — an incomplete incoming bundle or registry — which is always an absence, so two statuses say everything there is to say. Files are the only surface that derives one stored object from another, so they are the only one that can refuse on purpose. The shared policy is unchanged and uniform across all five: read an absence and audit it, refuse a taint. That invariant is now written down on
DURABLE_SECRET_PROVENANCE_SURFACESso the asymmetry is not mistaken for drift in either direction.Type of Change
Testing
Tested manually.
bun run lint, all 24 CI audits,check:migrations, and type-check pass. 5,836 tests pass across the uploads, provenance, executor, knowledge, copilot and file-tool suites;packages/db60.Three pre-existing tests encoded the old fail-closed decision and were updated deliberately — the unrecorded attachment is now kept, an unrecorded mount now proceeds without importing, and the mixed egress test keeps refusing tainted, stale and cross-workspace keys with only the unrecorded row removed. Two others were failing wrongly mid-change and caught a real bug: the first cut of this applied the policy to a content-version mismatch and a missing row, which it must not.
Each of the two review fixes has a test verified to fail against the reverted fix before being kept — the merge one on precedence, the migration one on statement order across both repairs.
Checklist