Restricted data: govern restricted reads by observer verification - #382
Conversation
Preview:
|
|
cdd6c1f to
4a3f383
Compare
|
Restate what `ObservationDescription.containsRestrictedData` means now that the enforcement is per-collaborator observer verification rather than an all-or-nothing sharing lockdown, and state the two limits of the model plainly: verification is held to the collaborator's role scope, and enforcement is at admission rather than at each read. No functional change; the implementation follows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
4a3f383 to
a323357
Compare
|
|
…ion. Reading restricted data no longer locks the workspace down. The old model blocked the observation outright if the workspace was shared and then refused all future sharing, which made every sensitive data source unusable the moment a workspace had a single collaborator. The observer verification machinery already answers the real question -- does this collaborator have access to the same data? -- at every open, and a widening of that scope now restarts every live session, so admission is a sound enforcement point. So: drop the `hasAnyShares()` block in `authorizeObservation` and the three guards on the sharing mutators. Keep the two guards that are about leaking data back out rather than about who may see it -- no actions and no public web fetches once the latch is set. What replaces them is narrower. A producer nobody can ever be verified against (a vendorless connection, or a legacy record with no `creationSpec`) is still refused while the workspace is shared, because `#inScopeGatekeepers` skips it and so admission cannot see it at all. Removing a producer's record is blocked while the workspace is shared, since that record is what verification runs against. And a new grant -- a collaborator, a share link, another key for one, or a redemption -- is refused if some producer can no longer verify anyone. Each of those checks runs in the same synchronous block as the write it gates, after every await, so a concurrent change cannot slip between check and write. `sharing.ts` loses `hasAnyShares()` and gains an optional `assertGrantAllowed` on each grant-writing method, invoked at that write. Two smaller things fall out. `getSharingManager()` moves inside the `containsRestrictedData` branch, so an ordinary observation on a cold DO no longer pays for an owner User DO round trip; the producer record is then read after that await, since latching against a stale record would permanently brick sharing. And the restart on a terminal re-verification failure is hoisted ahead of the best-effort rollback, taking a gatekeeper RPC fan-out off the path between determining the denial and the abort. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Covers the latch (what sets it, and the cases that must refuse the read rather than latch), the producer-removal guard and its exemptions, the grant checks on each sharing mutator, and the tolerance for action records written before the flag's rename. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Drives the model end to end through the test gatekeeper: a restricted read on a shared workspace, the unverifiable-producer refusal, the removal guard, the action and web-fetch blocks, and the restart that forces re-verification when scope widens. `TestSession.readThing()` takes an optional `restricted` flag so a test can trip the latch through the same `ApprovalQueue` funnel a shipping gatekeeper uses. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rewrites the observer document's model section around admission-time enforcement, states the two limits (role-scoped verification, and enforcement at admission rather than at each read) as edge cases with their reasoning, and records the design under plans/restricted-data-sharing.md -- including the known risk of a producer no gadget binds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
a323357 to
4a88685
Compare
|
This is the lazy-revocation we already document explicitly, at the throw site, in docs/sharing.md docs/observers.md edge case 3, and in the Known limitations of plans/restricted-data-sharing.md. The integration test "a failed re-verification denies that open and nothing else" also pins it. |
This is the "never-bound producers" item under Accepted tradeoffs in plans/restricted-data-sharing.md, described as a known security risk and marked at the site by the TODO(known-risk) |
Known it's the mid-registration item under Known limitations in both docs/observers.md and the plan doc, marked by the TODO(observer-races)
This is the Step 5 gap in docs/observers.md (the "left their scope does not yet imply cannot reach" paragraph)
Fixed on the #assertUnverifiableProducerUnshared now treats any outstanding share link as "shared", the same predicate
The UI changes land in a follow up PR |
Known, race documented in known limitations in observer.md doc, marked by the TODO at the site. ( pre-existing won't fix in this PR )
Gap is documented at receiveExternalMessage (overseer.ts and in docs/observers.md under "Known gap — the agent turn an external message starts". Nothing yet calls the external chat endpoint, so I'm continuing to defer this. And platform-gateway mode should be used if you want to restrict what AI providers your deployment uses.
In ensureAmbientCapsules the only await after the accounts snapshot is getSharingManager(), which this branch added so that the guard could run synchronously: the gatekeeper records are snapshotted after it, and the loop from snapshot through removalBlockedByRestrictedData to removeGatekeeper has no await at all |
This is pre-existing same root cause as the resumed-turn finding above as the comment in overseer.ts already says. The window is narrower than "any time after disconnect", deferring and on top of that if the deployment admin wants to restrict model egress they can just use platform gateway mode
Documented in docs/observers.md "Known gap — enableHook neither counts nor aborts gadget-minted children", deferring |
…pping no-op re-grants. The overseer called assertNewSharingAllowed() unconditionally before SharingManager.addCollaborator() could learn whether the caller already had an edge to this profile, so a same-or-lower re-grant (a note update or a pure no-op) was refused once the workspace became permanently owner-only. Every other grant mutator takes an assertGrantAllowed hook and runs it at the write; redeemShareKey skips it for an existing edge. addCollaborator now takes the same hook and invokes it only when a grant is created: a new record, a new edge from this sharer, or a role rise on the existing edge. The check still runs in the same synchronous block as the storage write, after every await. maxRole is gone with the rewrite. Unreachable in practice (the removal guard refuses to remove a producer while any reachable collaborator exists), fixed for consistency with the documented design in plans/restricted-data-sharing.md §4. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
LGTM! |
kentonv
left a comment
There was a problem hiding this comment.
I think this change should be significantly simplified.
After simplification the change should be mostly (entirely?) deletions.
Per review: the second layer part 2 stacked on top of observer verification is gone. Deleted `#assertUnverifiableProducerUnshared`, `restrictedProducerIds` (which scanned the whole action log), `removalBlockedByRestrictedData`, `assertNewSharingAllowed`, the removal guard in `GatekeeperClientImpl.remove()` and ambient reconciliation, the missing-producer refusal in `authorizeObservation`, the legacy `prohibitAllSharing` read shim, and the `assertGrantAllowed` hook plumbing in `SharingManager` that only existed to carry the assertion. `authorizeObservation` now just latches. What remains is the whole model: a collaborator is verified against every in-scope gatekeeper at admission, and the latch blocks actions and public-web fetches. Removing a connection is not guarded; when a removal UI is built, it will ask the owner to certify that no sensitive data from that connection has been retained in the workspace, for any connection. Legacy records with no creationSpec are not worth the complexity: the owner starts a new workspace. Tests and docs for the deleted machinery are removed, and every comment the branch added is cut or trimmed to what the code still does. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
|
LGTM! |
Upstream sync plus two fork fix commits. `pnpm fork:sync --verify` is clean on the merge. ## Upstream (foundation/main → 5af356e, 11 commits) - **cloudflare#464 / cloudflare#473 — connect handoff**: adopted wholesale, with the fork's initiator guards kept in front. Completion tickets are bound to the initiating browser and redeemed from the popup; BroadcastChannel transport dropped. - **cloudflare#381 / cloudflare#382 / cloudflare#308 — restricted data**: observer-verification model adopted; the fork's owner-only tier is retained alongside it. Share modal stays usable for restricted workspaces. - **cloudflare#460 — gatekeeper-kit overhaul**: replayable credential runs, declared action fences, conformance consumer. Reviewed against gatekeeper-ai-executor: no wiring needed (it uses zero kit modules; no provider credentials, single action kind, strategy-A observers). - **cloudflare#450 — multi-tab Google Docs**; **cloudflare#433 — xlsx export** in workspace-sheets; **cloudflare#478** integration test; **cloudflare#470** vitest bump; **cloudflare#454** sidebar CSS. ## Fork fixes (config fallout from ae28b29) - `b23db94d`: manifest-lib reads and drops `limits`/`placement` (no v1-contract field; golden regenerated); staging strips dev-only `browser.remote`; integration harness drops backend assets for test boots (inline configs resolve `assets.directory` against the harness root). - `a8815836`: removes `browser.remote` (verified dev-only in wrangler's own source; prod Browser Rendering comes from the binding, now pinned in the deploy-contract test); run-dev-server drops backend assets in normal dev mode (run-local unchanged). ## Verification - scripts 378/378, backend 881, integration 82/82, frontend 474, ai-executor 57+6 — all green, no workarounds. - `types:scripts`, package `tsc`, lint (0 errors), `fork:sync --verify` clean. ## Open (not in this PR) - Context `artifacts` prod namespace flows verbatim into preview configs — needs a product call (cut vs per-preview namespace). - Customer manifests now ship frontend assets on the backend too (consequence of the fork's backend-assets decision).
Stacked on #381.
Why:
Before this PR: Once a user has observed a sensitive data source, we block sharing the resource altogether. We decided we can lift this restriction now that we have mechanisms in place to verify the observers permissions.
What:
Makes it so that now when restricted data is observed, the workspace can still be shared by verifying the observers access against the resource by reusing the same mechanisms we had already introduced in the original observer verification PR.
Frontend behavior is unchanged here, so the UI still declines to share a workspace even though the server now permits it. The UI will be done in #308