From 0943f9af9b7556cf8b815887298e8d169ea679e0 Mon Sep 17 00:00:00 2001 From: Maximo Guk <62088388+Maximo-Guk@users.noreply.github.com> Date: Fri, 28 Aug 2026 14:45:16 -0500 Subject: [PATCH 1/7] Restricted data part 1: Extend the API. 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 --- packages/workshop-shared/src/api.ts | 6 ++-- packages/workshop-shared/src/gatekeeper.ts | 33 +++++++++++++--------- 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/packages/workshop-shared/src/api.ts b/packages/workshop-shared/src/api.ts index 0422c0eaf..e4b90c183 100644 --- a/packages/workshop-shared/src/api.ts +++ b/packages/workshop-shared/src/api.ts @@ -1307,8 +1307,10 @@ export type GadgetMetadata = { role?: CollaboratorRole; /** - * True when the gadget has observed data marked as share-prohibited. Such gadgets can no longer - * be shared with additional users or links. + * True when the gadget has observed data marked as containing restricted data (see + * `ObservationDescription.containsRestrictedData`). Such gadgets can still be shared, but + * collaborators must be verified (per gatekeeper) to have access to the same data, and the + * workspace can no longer perform actions or fetch from the public web. */ containsRestrictedData?: boolean; diff --git a/packages/workshop-shared/src/gatekeeper.ts b/packages/workshop-shared/src/gatekeeper.ts index f2dc3eebb..b44abbe84 100644 --- a/packages/workshop-shared/src/gatekeeper.ts +++ b/packages/workshop-shared/src/gatekeeper.ts @@ -1211,19 +1211,26 @@ export type ObservationDescription = { // can help detect situations where the gadget could leak information. /** - * If true, then this observation contains sensitive information that MUST NOT be shared with - * ANYONE except the account owner. This means: - * - If the gadget is shared already, authorizeObservation() must throw an exception to block - * the observation. - * - All future sharing of the gadget is prohibited. - * - Once observed, the gadget goes into "lockdown mode" where it can no longer perform any - * actions, only make observations. This prevents the gadget from leaking data through other - * gatekeepers. - * - * TODO(someday): This was added as a stopgap in order to be able to make certain sensitive data - * sources available to internal users. In the longer-term, it should be possible to share - * sensitive data as long as the recipients also have access to that same data, but this - * requires a more complex policy framework to compute. + * If true, then this observation contains sensitive information that must only be shown to + * people who are verified to have access to the same data. This means: + * - Access to the gadget is conditioned on verification: every collaborator passed this + * gatekeeper's `addObserver()` at their most recent open and cannot open without passing it, + * so a gatekeeper whose `addObserver()` always throws is unshareable once it has made one of + * these observations. Anything that widens what a collaborator must be verified against -- + * adding a connection, binding one into a gadget -- restarts the workspace, so every live + * session re-opens and re-verifies against the new scope. + * - Once observed, the gadget enters a restricted mode: no more actions or public-web fetches, + * only observations, so the gadget cannot leak the data through other gatekeepers. + * + * Two limits are worth stating plainly. Verification is held to the collaborator's role scope, + * so a gatekeeper the agent reads only through a chat binding is in no "use" collaborator's + * scope and they are never verified against it. And enforcement is at admission rather than at + * each read, so a session whose holder should no longer be admitted is severed within ~100ms of + * the change rather than instantaneously. + * + * TODO(someday): The restricted mode is a blunt instrument. It should be possible to perform + * actions whose visibility is limited to people verified to have access to the same data, + * but this requires a more complex policy framework to compute. */ containsRestrictedData?: boolean; From dbc97c663540420784d221817a41b046a6a4bb37 Mon Sep 17 00:00:00 2001 From: Maximo Guk <62088388+Maximo-Guk@users.noreply.github.com> Date: Fri, 28 Aug 2026 14:48:33 -0500 Subject: [PATCH 2/7] Restricted data part 2: Govern restricted reads by observer verification. 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 --- .../__tests__/observer-scope-restart.test.ts | 13 +- packages/workshop-backend/src/overseer.ts | 286 ++++++++++++++---- packages/workshop-backend/src/sharing.ts | 69 +++-- 3 files changed, 279 insertions(+), 89 deletions(-) diff --git a/packages/workshop-backend/__tests__/observer-scope-restart.test.ts b/packages/workshop-backend/__tests__/observer-scope-restart.test.ts index e5769b212..2d42bc948 100644 --- a/packages/workshop-backend/__tests__/observer-scope-restart.test.ts +++ b/packages/workshop-backend/__tests__/observer-scope-restart.test.ts @@ -495,8 +495,8 @@ describe("restarting sessions when verification scope widens", () => { expect(restarts).toEqual([]); })); - // A pre-creationSpec record, which observerVendorId() refuses to classify: sharing anything - // that reaches it requires the owner to reconnect it first. + // A pre-creationSpec record, which observerVendorId() refuses to classify: nobody can be + // verified against it, so sharing anything that reaches it requires the owner to remove it first. function seedLegacyGatekeeper(impl: any, id: number): void { impl.storage.gatekeepers.put({ id, resourceTitle: `Legacy ${id}`, class: {} as any }); } @@ -518,8 +518,8 @@ describe("restarting sessions when verification scope widens", () => { seedLegacyGatekeeper(impl, 1); // "Build" scope is everything, so the record is in scope and the intended fail-closed error - // still fires: the owner must reconnect it before the workspace can be shared at that level. - expect(() => impl.listObserverRequirements("build")).toThrow(/reconnected/); + // still fires: the owner must remove it before the workspace can be shared at that level. + expect(() => impl.listObserverRequirements("build")).toThrow(/cannot verify collaborators/); })); it("...and still blocks a use collaborator once a gadget binds it", @@ -530,7 +530,7 @@ describe("restarting sessions when verification scope widens", () => { // Bound, the record is genuinely in "use" scope, and verification can't proceed without // knowing what to verify against -- same fail-closed refusal as "build". impl.bindWorkpiece(100, "DB", 1); - expect(() => impl.listObserverRequirements("use")).toThrow(/reconnected/); + expect(() => impl.listObserverRequirements("use")).toThrow(/cannot verify collaborators/); })); it("binding a legacy connection restarts a connected use collaborator and quarantines it", @@ -542,7 +542,8 @@ describe("restarting sessions when verification scope widens", () => { // A legacy record has no vendor, so nobody CAN be verified against it -- but that must make // it count on the widening diff, not vanish from both sides of it: binding it is still the // moment live use sessions gain a route to it. The restart severs them and the quarantine - // holds until the reset; fresh use opens then fail closed (/reconnected/, above). + // holds until the reset; fresh use opens then fail closed (/cannot verify collaborators/, + // above). impl.bindWorkpiece(100, "DB", 1); expect(restarts).toHaveLength(1); diff --git a/packages/workshop-backend/src/overseer.ts b/packages/workshop-backend/src/overseer.ts index f52790ebb..914a67086 100644 --- a/packages/workshop-backend/src/overseer.ts +++ b/packages/workshop-backend/src/overseer.ts @@ -493,8 +493,13 @@ function fallbackBindingName(base: string, isTaken: (name: string) => boolean): function observerVendorId(record: GatekeeperRecord): string | null { if (!record.creationSpec) { + // There is no reconnect affordance for a legacy record (it never persisted its vendor + // identity), so the message points at the two real remedies: the owner removing the + // connection (allowed only while unshared), or moving the work to a new workspace. throw new Error( - "This workspace has a legacy connection that must be reconnected by its owner before it can be shared."); + "This workspace has a legacy connection that cannot verify collaborators' access. Its " + + "owner must remove the connection before the workspace can be shared, or start a new " + + "workspace."); } return "vendorId" in record.creationSpec ? record.creationSpec.vendorId : null; } @@ -1152,7 +1157,8 @@ export function makeOverseerStorage(storage: DurableObjectStorage) { deadWorktreeIds: [], // True if any past observation was authorized that had the `containsRestrictedData` flag - // set in its `ObservationDescription`. The key on disk predates the flag's rename. + // set in its `ObservationDescription`. While set, the workspace may not perform actions or + // fetch from the public web. The key on disk predates the flag's rename. containsRestrictedData: singleton(false, {storageKey: "prohibitAllSharing"}), }, @@ -1536,6 +1542,21 @@ export function sanitizeMessageFormatRefs( // collaborator roles. type SessionKind = CollaboratorRole | "owner"; +// Action records that predate the flag's rename carry `containsRestrictedData` under its old +// name, `prohibitAllSharing`. Records are data at rest and are never rewritten, so the +// tolerance can never be removed. +type LegacyObservationDescription = ObservationDescription & { prohibitAllSharing?: boolean }; + +/** + * Whether a persisted observation description carries the restricted-data flag, under either its + * current name or the pre-rename one still present on older records. Exported for its unit test; + * every read of the flag off a persisted record must go through this. + */ +export function observationContainsRestrictedData(description: ObservationDescription): boolean { + let d: LegacyObservationDescription = description; + return (d.containsRestrictedData ?? d.prohibitAllSharing) === true; +} + class OverseerImpl implements AgentHooks { public storage: OverseerStorage; readonly logger: ReturnType; @@ -5509,17 +5530,6 @@ class OverseerImpl implements AgentHooks { async authorizeObservation(gatekeeperId: number, description: ObservationDescription, caller: GatekeeperCaller): Promise { - if (description.containsRestrictedData) { - if ((await this.getSharingManager()).hasAnyShares()) { - throw new Error( - "This observation was blocked because it contains sensitive data that must only be " + - "shown to the account owner, but this workspace is shared with other users. Try again " + - "from a workspace that is not shared."); - } - - this.storage.containsRestrictedData.put(true); - } - // Forward exclusion: the gatekeeper may name observers who must not see this observation. Since // v1 has no per-thread hiding, the only way to let such an observation proceed is if no named // observer could reach it -- either they have lost access in the sharing graph, or this @@ -5529,6 +5539,31 @@ class OverseerImpl implements AgentHooks { await this.#enforceExcludeObservers(gatekeeperId, description.excludeObservers); } + if (description.containsRestrictedData) { + // Resolved here rather than up front: on a cold DO this is an RPC to the owner's User DO, + // and an ordinary unrestricted observation must not pay for it. The producer record is read + // *after* that await, so the check below and the latch are one synchronous block -- a record + // read before the await could be stale by the time it is checked, and latching against a + // stale one permanently bricks sharing. + let sharing = await this.getSharingManager(); + let producer = this.storage.gatekeepers.get(gatekeeperId); + + // An in-flight facet RPC can outlive removeGatekeeper, so a restricted observation can + // arrive naming a connection this workspace no longer has. Latching a missing producer id + // permanently bricks sharing (assertNewSharingAllowed's missing-record branch), so refuse + // the read instead -- including on an unshared workspace, where nothing else would stop it. + // This same read is what refuses a connection removed during the exclusion awaits above, + // where the latch is not yet set and so removalBlockedByRestrictedData does not yet protect + // the producer. + if (!producer) { + throw new Error( + "This observation was blocked because it contains sensitive data, but the " + + "connection it was read through has been removed from this workspace."); + } + this.#assertUnverifiableProducerUnshared(producer, sharing); + this.storage.containsRestrictedData.put(true); + } + let actionId = this.storage.nextActionId.get(); this.storage.nextActionId.put(actionId + 1); @@ -5657,6 +5692,114 @@ class OverseerImpl implements AgentHooks { }); } + // Refuse a restricted observation from a producer nobody can ever be verified against: a + // gatekeeper with no vendor account behind it (aiModel/agentSpawner) or a legacy record with no + // creationSpec. Every *other* producer is enforced at admission -- a collaborator cannot open + // the workspace without passing addObserver() for it, and anything that widens what they must + // pass restarts every live session (see #restartIfSessionsAffected) -- but #inScopeGatekeepers skips + // these, so no collaborator is ever asked about them and admission cannot see them at all. + // Consistent with assertNewSharingAllowed(), which treats the same case as unshareable. + // + // "Shared" means any collaborator *or* any outstanding share link -- the same predicate as + // removalBlockedByRestrictedData (and what the pre-verification hasAnyShares() refusal counted). + // Links matter because their keys never expire and are multi-redeemable: if this read were + // admitted, the latch would make assertNewSharingAllowed() refuse every later redemption, so a + // link the owner has already handed out would be permanently unredeemable with no way back. + // + // Deliberately synchronous (the sharing manager is a parameter, not an internal await) so the + // caller can check and latch in one synchronous block -- see authorizeObservation. + #assertUnverifiableProducerUnshared(gatekeeper: GatekeeperRecord, sharing: SharingManager): void { + if (sharing.listCollaborators().length === 0 && + sharing.listShareLinkRecords().length === 0) { + return; + } + + let vendorId: string | null = null; + try { + vendorId = observerVendorId(gatekeeper); + } catch { + // Legacy connection with no creationSpec: treat as unverifiable. + } + if (vendorId !== null) return; + + // The message reaches sandboxed gadget code and agent output -- an audience that can't + // otherwise list collaborators -- so it reports only that the workspace is shared, naming + // neither the collaborators nor their profile ids (the full email on OAuth/CF Access + // deployments). + throw new Error( + "This observation was blocked because it contains sensitive data, but it was read " + + "through a connection that cannot verify anyone's access to that data, and this " + + "workspace is shared. Its collaborators must be removed and its share links revoked " + + "before this data can be read."); + } + + // The connection ids this workspace has read restricted data through: the producers the latch + // guards. Derived by scanning the action log for observations whose description carries + // `containsRestrictedData`, since nothing else records which connection a latched read came + // through. + restrictedProducerIds(): Set { + let producers = new Set(); + for (let record of this.storage.actions.list()) { + if (record.type === "observation" && + observationContainsRestrictedData(record.description) && + record.gatekeeperId !== BUILTIN_TOOL_GATEKEEPER_ID) { + producers.add(record.gatekeeperId); + } + } + return producers; + } + + // True if removing gatekeeper `id` is blocked because it anchors restricted-data verification: + // the workspace is latched, `id` is a restricted producer (or the producer set is unexpectedly + // empty -- see below), and the sharing graph still has collaborators or outstanding share + // links. Shared by GatekeeperClientImpl.remove() and the ambient reconciliation in + // ensureAmbientCapsules(): while the workspace is shared, deleting a producer's record would + // let a never-verified party see the data -- the record is what observer verification runs + // against at every open, and for an unverifiable record it is what refuses the producer's reads + // outright -- even though the restricted data outlives it in chat history and storage. + // + // Deliberately synchronous (the sharing manager is a parameter, not an internal await) so each + // caller can check and delete in one synchronous block -- see GatekeeperClientImpl.remove(). + removalBlockedByRestrictedData(id: WorkpieceId, sharing: SharingManager): boolean { + if (!this.storage.containsRestrictedData.get()) return false; + // An empty producer set with the latch set should be impossible: the latch and the action + // record are written in one synchronous block, built-in observations never latch, and + // records that predate the flag's rename still read correctly (see + // observationContainsRestrictedData). If it ever happens anyway, fall back to guarding + // every connection rather than none. + let producers = this.restrictedProducerIds(); + if (producers.size > 0 && !producers.has(id)) return false; + return sharing.listCollaborators().length > 0 || sharing.listShareLinkRecords().length > 0; + } + + // Refuse a new sharing grant once the workspace has read restricted data through a connection + // that can no longer verify a recipient's access to it -- one that has since been removed, or + // that never had a vendor account behind it. Every other producer verifies its collaborators at + // each open, so sharing stays available. + assertNewSharingAllowed(): void { + if (!this.storage.containsRestrictedData.get()) return; + for (let id of this.restrictedProducerIds()) { + let producer = this.storage.gatekeepers.get(id); + if (!producer) { + throw new Error( + "This workspace can no longer be shared: it read sensitive data through a connection " + + "that has since been removed, so new collaborators can no longer be verified for " + + "access to that data."); + } + let vendorId: string | null = null; + try { + vendorId = observerVendorId(producer); + } catch { + // Legacy connection with no creationSpec: treat as unverifiable. + } + if (vendorId === null) { + throw new Error( + "This workspace can no longer be shared: it read sensitive data through a connection " + + "that cannot verify collaborators' access to that data."); + } + } + } + // Enforce an observation's `excludeObservers`, named by the gatekeeper `gatekeeperId` produced // it. For each named opaque observerId: // - Map it back to a profileId via the byObserverId index. An unknown id is not an active @@ -5683,6 +5826,11 @@ class OverseerImpl implements AgentHooks { let outOfScope: string[] = []; for (let observerId of observerIds) { let observer = this.storage.observers.byObserverId.get(observerId); + // TODO(observer-races): a first-time ensureObserver registers a freshly minted id with the + // gatekeepers *before* the record (and so byObserverId) is persisted, so an id named here + // during that window -- which can park on the config modal -- reads as unknown and the + // observation is admitted to a collaborator it names. Fix: an in-memory map of pending ids + // consulted here, failing closed. Lands with observer-verification-fixes (7ba93821). if (!observer) continue; // not an active observer -> ignore let role = sharing.getEffectiveRole(observer.profileId); if (!role) { @@ -7442,11 +7590,14 @@ class OverseerImpl implements AgentHooks { // single round trip both provisions them and reads them back before we wire up capsules. let accounts = (await ownerDo.listProvidedAccounts()) .filter(account => account.description.singleton?.tsType); + let sharing = await this.getSharingManager(); // Reconcile existing ambient capsule records against the owner's current singleton accounts. Each // record is keyed to a specific accountId; if that account is gone (disconnected) or was replaced // (an optional account removed and re-added with a new accountId), the record is stale and would - // point the capsule at a deleted account — so remove it. Snapshot the list since we mutate it. + // point the capsule at a deleted account — so remove it. With the sharing manager fetched above, + // the loop is fully synchronous: each removal-blocked check runs in the same synchronous block as + // the delete it gates, and the snapshot below cannot go stale mid-iteration. let currentAccountId = new Map(accounts.map(account => [account.vendorId, account.accountId])); let bound = new Set(); // Snapshot before iterating, since removeGatekeeper() mutates the collection. @@ -7455,6 +7606,18 @@ class OverseerImpl implements AgentHooks { if (gk.creationSpec?.type !== "ambient") continue; if (currentAccountId.get(gk.creationSpec.vendorId) === gk.creationSpec.accountId) { bound.add(gk.creationSpec.vendorId); + } else if (this.removalBlockedByRestrictedData(gk.id, sharing)) { + // A stale ambient record that anchors restricted-data verification must survive until + // the owner unshares -- deleting it here would be the same unchecked readmission + // GatekeeperClientImpl.remove() guards against, minus the user intent. Not added to + // `bound`, so a replacement account still gets a fresh capsule record; + // prepareChatBindings tolerates the duplicate vendor (names dedupe via the fallback + // binding name, and the dead record's session just fails). + this.logger.warn("skipping removal of stale ambient restricted producer", { + event: "singleton.capsules.reconcile.blocked", + gatekeeperId: gk.id, + vendorId: gk.creationSpec.vendorId, + }); } else { this.removeGatekeeper(gk.id); } @@ -9167,6 +9330,14 @@ class OverseerImpl implements AgentHooks { // creationSpec: an unrelated legacy connection outside the caller's scope must not block their // open, since nothing they can reach needs verification against it. An in-scope one still // throws, fail-closed (and "build" scope is everything, so it always throws there). + // + // TODO(known-risk): scoping by role means a "use" collaborator is never verified against a + // producer outside their scope (one no gadget binds and no enabled hook feeds) -- yet + // restricted data read from such a producer can reach gadget state and the UI they drive, + // because provenance is not tracked past the observation. Deliberately accepted for v1; the + // required fix (verify every restricted producer, or isolate restricted data by provenance) is + // recorded under "Known security risk -- never-bound producers" in + // plans/restricted-data-sharing.md, and worked through in docs/observers.md edge case 4. #inScopeGatekeepers(role: CollaboratorRole): GatekeeperRecord[] { let boundIds = role === "use" ? this.#useScopeGatekeeperIds() : undefined; @@ -9319,6 +9490,10 @@ class OverseerImpl implements AgentHooks { // resource access promptly. Returns when fully verified; throws to deny access. // // See observers-implementation-plan.md §5 Step 3. + // + // TODO: Concurrent opens by the same profile race this method -- two calls mint two observerIds + // and the last-written record forgets the other's gatekeeper registrations -- so verification + // needs to be serialized per profile. async ensureObserver( profileId: string, clientUser: DurableObjectStub, @@ -9803,13 +9978,6 @@ export class OverseerDurableObject extends DurableObject { let role: CollaboratorRole = "build"; if (!isOwner) { - if (this.impl.storage.containsRestrictedData.get()) { - // `containsRestrictedData` can only have been set when the gadget had no shares (see - // `authorizeObservation`), and no new shares can be created while it's set, so any - // non-owner reaching here is necessarily unauthorized. - throw createOpenGadgetError(OPEN_GADGET_ERROR_CODES.workspaceAccessDenied); - } - let sharing = await this.impl.getSharingManager(); // If a share key was provided, redeem it. The owner already has full access and should not @@ -9819,6 +9987,11 @@ export class OverseerDurableObject extends DurableObject { rawKey: shareKey, profileId, fetchProfile: () => clientUser.whoami(), + // An outstanding key is a new grant vector, so redemption is policy-gated like the + // grant-creating mutators. Without this, keys minted before an exempted + // (unverifiable-producer) removal -- or on a legacy-latched workspace whose producer is + // gone -- would still admit unverified recipients. + assertGrantAllowed: () => this.impl.assertNewSharingAllowed(), }); } @@ -9830,7 +10003,7 @@ export class OverseerDurableObject extends DurableObject { // verify they may observe everything this Gadget has read through its in-scope gatekeepers, // configuring their connected accounts if needed. Observer verification runs only after a // valid role is confirmed, so it never reveals gatekeeper or resource metadata to an - // unauthorized user; the containsRestrictedData short-circuit above still wins over both. + // unauthorized user. // // An unauthorized caller (no effective role -- never had access, or was removed) gets a // distinct denial without workspace metadata. A removed collaborator who reconnects after @@ -9948,12 +10121,6 @@ export class OverseerDurableObject extends DurableObject { // denial below rather than being verified (or told to fix a verification failure) for access // this path can never grant them. if (ownerId !== callerId) { - if (this.impl.storage.containsRestrictedData.get()) { - return { - accepted: false, - message: "This workspace has sharing disabled, so only its owner can access it.", - }; - } let role: CollaboratorRole | null; try { role = await this.impl.authorizeCollaborator( @@ -11976,8 +12143,10 @@ class OverseerClientInterface extends RpcTarget implements Overseer { // --- Collaborator management --- // // The sharing/permission logic lives in SharingManager (./sharing). These methods handle only - // the RPC-bound pieces (resolving profiles via User DOs, the `containsRestrictedData` policy) and - // delegate the rest. + // the RPC-bound pieces (resolving profiles via User DOs) and delegate the rest. Sharing stays + // available even after the workspace observes sensitive data (`containsRestrictedData`): + // access to that data is enforced per-gatekeeper by observer verification, not by blocking + // sharing wholesale. async listObserverRequirements( role: CollaboratorRole): Promise { @@ -11998,13 +12167,12 @@ class OverseerClientInterface extends RpcTarget implements Overseer { return null; } - if (this.impl.storage.containsRestrictedData.get()) { - throw new Error( - "This workspace has observed sensitive data. To prevent leaks, the workspace cannot be " + - "shared."); - } - - return (await this.impl.getSharingManager()).addCollaborator({ + let sharing = await this.impl.getSharingManager(); + // Asserted in the same synchronous block as the grant's storage write (after every await): a + // check ahead of the awaits above could pass, a concurrent producer-connection removal land + // during the yield, and the grant still be written past it. + this.impl.assertNewSharingAllowed(); + return sharing.addCollaborator({ caller: this.#sharingCaller(), profile, role, @@ -12061,25 +12229,20 @@ class OverseerClientInterface extends RpcTarget implements Overseer { async createShareLink(role: CollaboratorRole, note?: string) : Promise<{ key: string; linkId: string }> { - if (this.impl.storage.containsRestrictedData.get()) { - throw new Error( - "This workspace has observed sensitive data. To prevent leaks, the workspace cannot be " + - "shared."); - } - - return (await this.impl.getSharingManager()) - .createShareLink({ caller: this.#sharingCaller(), role, note }); + return (await this.impl.getSharingManager()).createShareLink({ + caller: this.#sharingCaller(), role, note, + assertGrantAllowed: () => this.impl.assertNewSharingAllowed(), + }); } async newShareLinkKey(linkId: string): Promise<{ key: string }> { - if (this.impl.storage.containsRestrictedData.get()) { - throw new Error( - "This workspace has observed sensitive data. To prevent leaks, the workspace cannot be " + - "shared."); - } - - return (await this.impl.getSharingManager()) - .newShareLinkKey({ caller: this.#sharingCaller(), linkId }); + return (await this.impl.getSharingManager()).newShareLinkKey({ + caller: this.#sharingCaller(), linkId, + // A fresh key is a new grant vector even though the link already exists: it is reachable + // here when an unverifiable producer was removed while links were outstanding (which the + // removal guard deliberately allows as a remedy). + assertGrantAllowed: () => this.impl.assertNewSharingAllowed(), + }); } async listShareLinks(): Promise { @@ -12800,7 +12963,24 @@ class GatekeeperClientImpl> } async remove(): Promise { + // A connection that has read restricted data is the anchor observer verification runs + // against: while the workspace is shared, deleting its record would let a never-verified + // collaborator open unchecked even though the data persists in chat history and storage. + // Outstanding share links count as shared too: redemption is gated at open() only while the + // record exists. Only the producers themselves are guarded -- a non-producer connection + // anchors no restricted-data verification, so it stays removable while shared. + let sharing = await this.impl.getSharingManager(); + // Checked in the same synchronous block as the delete, after the only await (cf. + // addCollaborator): a check ahead of the yield could pass, a concurrent grant land during + // it, and the delete still run past it. let record = this.impl.storage.gatekeepers.get(this.id); + if (record && this.impl.removalBlockedByRestrictedData(this.id, sharing)) { + throw new Error( + "This connection cannot be removed: it has read sensitive data into this " + + "workspace, and the workspace is shared. Collaborators are verified against this " + + "connection before they may see that data, so remove all collaborators and revoke " + + "all share links first."); + } this.impl.removeGatekeeper(this.id); this.impl.recordGadgetAnalytics({ event_name: "connection_removed", diff --git a/packages/workshop-backend/src/sharing.ts b/packages/workshop-backend/src/sharing.ts index e3fbe48d8..88039e236 100644 --- a/packages/workshop-backend/src/sharing.ts +++ b/packages/workshop-backend/src/sharing.ts @@ -15,11 +15,11 @@ // re-adding a removed collaborator restores them and, transitively, everyone they had shared with. // (Records and revoked keys accumulate in storage; a future GC could reclaim long-dead entries.) // -// NOTE: The `containsRestrictedData` policy flag intentionally does NOT live here. It is a broader -// "is this gadget allowed to communicate with anyone other than the owner?" policy (it also -// gates gatekeeper writes and web fetches) and is expected to grow into a separate policy engine. -// The Overseer enforces that flag; this module only exposes `hasAnyShares()` so the policy can -// ask about the current sharing state. +// NOTE: The sensitive-data (`containsRestrictedData`) policy intentionally does NOT live here. +// It is a broader "what may this gadget do after reading restricted data?" policy (it gates +// gatekeeper writes and web fetches, and requires per-gatekeeper observer verification of +// collaborators) and is expected to grow into a separate policy engine. The Overseer enforces +// it; this module only answers questions about the sharing graph. import { AiChatAuthorInfo, CollaboratorInfo, PermissionEdge, CollaboratorRole, AffectedCollaborator } from "@gadgets/workshop-shared/api"; @@ -160,26 +160,6 @@ export class SharingManager { */ constructor(private storage: SharingStorage, private ownerProfileId: string) {} - // --------------------------------------------------------------------------------------- - // Sharing-state queries - - /** - * True if anyone other than the owner can currently access the gadget. Used by the Overseer's - * `containsRestrictedData` policy to decide whether a sensitive observation must be blocked. - * - * Because removed collaborators and revoked links linger in storage (the lazy revocation model; - * see the module header and removeCollaborator/revokeShareLink), this must reflect *current* - * reachability, not mere table membership: a collaborator with a live path from the owner, or - * an un-revoked share link whose keys anyone could still redeem. - */ - hasAnyShares(): boolean { - if (this.computeEffectiveRoles().size > 0) return true; - for (let link of this.#listLinks()) { - if (!link.revoked) return true; - } - return false; - } - // Every share link, revoked or not. Aliases are skipped. *#listLinks(): Generator { for (let record of this.storage.shareKeys.list()) { @@ -219,11 +199,21 @@ export class SharingManager { * collaborators are redeemed without any RPC. * * A key whose link is revoked behaves like an unknown key (it cannot be redeemed). + * + * TODO: Redemption is one-step: the edge written here is real before the redeeming open()'s + * observer verification runs. Two accepted consequences, both fail-closed (availability, not + * confidentiality): an unverified redeemer is a current collaborator, so restricted reads + * block from redemption until they verify (or are removed, or the link is revoked); and a + * recipient whose verification is refused keeps the edge -- visible in listCollaborators, + * blocking restricted reads until removed. Two-phase redemption (a pending edge that grants + * nothing until verification confirms it) is the planned fix for both. */ async redeemShareKey(opts: { rawKey: string; profileId: string; fetchProfile: () => Promise; + /** See createShareLink: run synchronously with the put, a throw persists nothing. */ + assertGrantAllowed?: () => void; }): Promise { let hash = await hashShareKey(opts.rawKey); let keyRecord = this.storage.shareKeys.get(hash); @@ -240,10 +230,12 @@ export class SharingManager { let existing = this.storage.collaborators.get(opts.profileId); if (existing) { // User is already a collaborator. Only add an edge if they don't already have one for this - // link (redeeming a second key of the same link is a no-op). + // link (redeeming a second key of the same link is a no-op, so no new grant and no policy + // check). let alreadyHasEdge = existing.addedBy.some( e => e.type === "shareKey" && e.keyId === linkId); if (!alreadyHasEdge) { + opts.assertGrantAllowed?.(); existing.addedBy.push({ type: "shareKey", keyId: linkId, @@ -255,6 +247,7 @@ export class SharingManager { } else { // New collaborator -- need full profile from their user DO. let profile = await opts.fetchProfile(); + opts.assertGrantAllowed?.(); this.storage.collaborators.put({ profile, addedBy: [{ @@ -292,8 +285,8 @@ export class SharingManager { /** * Add a collaborator with a `user` edge from the caller, granting `role`. The caller is - * responsible for resolving `profile` (via RPC) and for any policy checks (e.g. - * `containsRestrictedData`). The caller may not grant a role higher than their own effective role. + * responsible for resolving `profile` (via RPC) and for any policy checks. The caller may not + * grant a role higher than their own effective role. */ addCollaborator(opts: { caller: SharingCaller; @@ -436,7 +429,16 @@ export class SharingManager { } async createShareLink( - opts: { caller: SharingCaller; role: CollaboratorRole; note?: string }) + opts: { + caller: SharingCaller; + role: CollaboratorRole; + note?: string; + /** + * Optional policy check invoked synchronously with the grant's storage write, after + * every await, so a policy change cannot slip between check and grant. + */ + assertGrantAllowed?: () => void; + }) : Promise<{ key: string; linkId: string }> { let callerRole = this.#requireCallerRole(opts.caller); if (roleRank(opts.role) > roleRank(callerRole)) { @@ -445,6 +447,7 @@ export class SharingManager { // The link is stored as its first key: the record is keyed by that key's hash. let { key, hash } = await this.#mintKey(); + opts.assertGrantAllowed?.(); this.storage.shareKeys.put({ id: hash, note: opts.note, @@ -456,7 +459,12 @@ export class SharingManager { } /** Mints another key for an existing link. */ - async newShareLinkKey(opts: { caller: SharingCaller; linkId: string }): Promise<{ key: string }> { + async newShareLinkKey(opts: { + caller: SharingCaller; + linkId: string; + /** See createShareLink: run synchronously with the put, a throw persists nothing. */ + assertGrantAllowed?: () => void; + }): Promise<{ key: string }> { let link = this.#requireLink(opts.linkId); if (link.revoked) { throw new Error("Share link not found."); @@ -471,6 +479,7 @@ export class SharingManager { } let { key, hash } = await this.#mintKey(); + opts.assertGrantAllowed?.(); this.storage.shareKeys.put({ id: hash, alias: link.id }); return { key }; } From a7cf2f82f6a3bfae394e494b16f751b867d6f0e3 Mon Sep 17 00:00:00 2001 From: Maximo Guk <62088388+Maximo-Guk@users.noreply.github.com> Date: Fri, 28 Aug 2026 14:49:48 -0500 Subject: [PATCH 3/7] Restricted data part 3: Backend tests. 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 --- .../observer-verification-failure.test.ts | 8 + .../__tests__/restricted-data.test.ts | 39 ++ .../restricted-observation-latch.test.ts | 281 ++++++++++++++ .../restricted-producer-removal.test.ts | 367 ++++++++++++++++++ .../__tests__/sharing.test.ts | 104 ++++- 5 files changed, 778 insertions(+), 21 deletions(-) create mode 100644 packages/workshop-backend/__tests__/restricted-data.test.ts create mode 100644 packages/workshop-backend/__tests__/restricted-observation-latch.test.ts create mode 100644 packages/workshop-backend/__tests__/restricted-producer-removal.test.ts diff --git a/packages/workshop-backend/__tests__/observer-verification-failure.test.ts b/packages/workshop-backend/__tests__/observer-verification-failure.test.ts index 8ef27cba5..b7982fee1 100644 --- a/packages/workshop-backend/__tests__/observer-verification-failure.test.ts +++ b/packages/workshop-backend/__tests__/observer-verification-failure.test.ts @@ -80,6 +80,14 @@ describe("a binding that fails verification", () => { // nothing for the rollback to remove. Her registrations are what make gatekeepers name her // in `excludeObservers`, so dropping one would be fail-open. expect(removed).toEqual([]); + + // Neither producer's restricted reads are blocked, though -- both are verifiable, so + // admission is the whole enforcement and nobody unverified can be watching. + let restricted = { title: "t", description: "d", containsRestrictedData: true }; + await expect(impl.authorizeObservation(1, restricted, { from: "user" })) + .resolves.toBeUndefined(); + await expect(impl.authorizeObservation(2, restricted, { from: "user" })) + .resolves.toBeUndefined(); }); }); diff --git a/packages/workshop-backend/__tests__/restricted-data.test.ts b/packages/workshop-backend/__tests__/restricted-data.test.ts new file mode 100644 index 000000000..233761fb3 --- /dev/null +++ b/packages/workshop-backend/__tests__/restricted-data.test.ts @@ -0,0 +1,39 @@ +// The restricted-data flag on persisted observation records must be readable under both its +// current name and its pre-rename one: records written before the rename carry +// `prohibitAllSharing`, are never rewritten, and anchor the producer-scoped removal guard and +// assertNewSharingAllowed -- a legacy record read as unflagged would let a legacy-latched +// workspace share past a removed producer. + +import { describe, it, expect } from "vitest"; +import { observationContainsRestrictedData } from "../src/overseer.js"; +import type { ObservationDescription } from "@gadgets/workshop-shared/gatekeeper"; + +function description(flags: Record): ObservationDescription { + return { title: "t", description: "d", ...flags } as ObservationDescription; +} + +describe("observationContainsRestrictedData", () => { + it("reads the current field name", () => { + expect(observationContainsRestrictedData(description({ containsRestrictedData: true }))) + .toBe(true); + }); + + it("reads the pre-rename field name on legacy records", () => { + expect(observationContainsRestrictedData(description({ prohibitAllSharing: true }))) + .toBe(true); + }); + + it("is false when neither name is present", () => { + expect(observationContainsRestrictedData(description({}))).toBe(false); + }); + + it("is true when both names are present", () => { + expect(observationContainsRestrictedData( + description({ containsRestrictedData: true, prohibitAllSharing: true }))).toBe(true); + }); + + it("is false for an explicit legacy false", () => { + expect(observationContainsRestrictedData(description({ prohibitAllSharing: false }))) + .toBe(false); + }); +}); diff --git a/packages/workshop-backend/__tests__/restricted-observation-latch.test.ts b/packages/workshop-backend/__tests__/restricted-observation-latch.test.ts new file mode 100644 index 000000000..e5ce5ca36 --- /dev/null +++ b/packages/workshop-backend/__tests__/restricted-observation-latch.test.ts @@ -0,0 +1,281 @@ +// authorizeObservation's restricted-data gates. The exclusion gate is decided before anything +// else: the restricted-mode latch is one-way, so an observation the exclusion blocks must leave +// no trace -- no latch, no record, sharing untouched. The decisions the delivery rests on (the +// removed-connection refusal, the unverifiable-producer refusal, the latch, the record) all run +// *after* the exclusion teardown's awaited cross-worker fan-out, in one synchronous block, so a +// removal landing mid-teardown refuses the observation rather than slipping past a pre-latched +// producer. And a restricted observation arriving through an already-removed connection (an +// in-flight facet RPC can outlive removeGatekeeper) is refused rather than latched: with zero +// collaborators nothing else would stop it, and latching a missing producer id would permanently +// brick sharing via assertNewSharingAllowed's missing-record branch. +// +// The last case covers the one producer admission cannot enforce: a connection with no vendor +// account behind it is in nobody's verification scope, so no collaborator is ever asked about it. +// +// Runs against a real OverseerDurableObject (the TEST_OVERSEER binding, like +// restricted-producer-removal.test.ts); the gatekeeper facet is the only fake. + +import { describe, expect, it } from "vitest"; +import { env } from "cloudflare:workers"; +import { runInDurableObject } from "cloudflare:test"; +import type { OverseerDurableObject } from "../src/overseer.js"; + +declare module "cloudflare:workers" { + interface ProvidedEnv { + TEST_OVERSEER: DurableObjectNamespace; + } +} + +const OWNER = "alice"; + +function deferred(): { promise: Promise; resolve: () => void } { + let resolve!: () => void; + let promise = new Promise(r => { resolve = r; }); + return { promise, resolve }; +} + +const tick = () => new Promise(resolve => setTimeout(resolve, 0)); + +function getImpl(instance: OverseerDurableObject): any { + let impl = (instance as unknown as { impl: any }).impl; + // The sharing manager resolves collaborator reachability from the owner; seed the cached + // profile id so no User DO round trip is attempted. + impl.ownerProfileId = OWNER; + return impl; +} + +function seedGatekeeper(impl: any, id: number): void { + impl.storage.gatekeepers.put({ + id, + resourceTitle: `Connection ${id}`, + class: {} as any, + creationSpec: { + type: "gatekeeper", + vendorId: "testvendor", + resourceUrl: `https://example.com/${id}`, + typeUrlPattern: "https://*", + }, + }); +} + +const RESTRICTED_EXCLUDING_MALLORY = { + title: "Read a thing", + description: "The test read a thing.", + containsRestrictedData: true, + excludeObservers: ["obs-m"], +}; + +describe("authorizeObservation's restricted-data gates", () => { + it("latches and records only after the exclusion teardown admits the observation", async () => { + let stub = env.TEST_OVERSEER.getByName("restricted-latch-teardown-window"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = getImpl(instance); + seedGatekeeper(impl, 1); + // An outstanding share link keeps the workspace "shared" for + // removalBlockedByRestrictedData. + impl.storage.shareKeys.put({ + id: "link-1", created: new Date(), createdBy: OWNER, role: "build", + }); + // Mallory holds an observer record but no reachable role: the named exclusion admits the + // observation and schedules her teardown. + impl.storage.observers.put( + { profileId: "mallory", observerId: "obs-m", accountChoices: { 1: 10 } }); + + // The cross-worker teardown parks, holding the observation mid-flight before any decision + // the delivery rests on has been made. + let held = deferred(); + impl.getGatekeeperFacet = () => ({ + removeObserver: async () => { await held.promise; }, + }); + + let observation = impl.authorizeObservation( + 1, RESTRICTED_EXCLUDING_MALLORY, { from: "user" }); + await tick(); + + // Nothing is delivered while the teardown is in flight, so nothing has latched: a teardown + // that ends in refusal must leave no trace. + expect(impl.storage.containsRestrictedData.get()).toBe(false); + + held.resolve(); + await expect(observation).resolves.toBeUndefined(); + + // Delivery: the latch and the record landed together, and everything keyed on the latch + // now holds. + expect(impl.storage.containsRestrictedData.get()).toBe(true); + expect(impl.removalBlockedByRestrictedData(1, await impl.getSharingManager())).toBe(true); + + // The teardown still ran (mallory is no longer set up to observe). + expect(impl.storage.observers.get("mallory")).toBeUndefined(); + let records = [...impl.storage.actions.list()]; + expect(records).toHaveLength(1); + expect(records[0].type).toBe("observation"); + }); + }); + + it("leaves no trace when the exclusion gate blocks the observation", async () => { + let stub = env.TEST_OVERSEER.getByName("restricted-latch-exclusion-blocked"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = getImpl(instance); + seedGatekeeper(impl, 1); + // Mallory is a current collaborator, verified against the producer: nothing else stands in + // this observation's way, so the exclusion gate is the only thing blocking it. + impl.storage.collaborators.put({ + profile: { id: "mallory", name: "Mallory" }, + addedBy: [{ type: "user", sharer: OWNER, created: new Date(), role: "build" }], + }); + impl.storage.observers.put( + { profileId: "mallory", observerId: "obs-m", accountChoices: { 1: 10 } }); + + await expect(impl.authorizeObservation( + 1, RESTRICTED_EXCLUDING_MALLORY, { from: "user" })) + .rejects.toThrow(/not permitted to see/); + + // The blocked observation delivered no data, so the workspace is not restricted: no latch, + // sharing still grantable, no action record -- and mallory, still authorized, was not torn + // down. + expect(impl.storage.containsRestrictedData.get()).toBe(false); + expect(() => impl.assertNewSharingAllowed()).not.toThrow(); + expect([...impl.storage.actions.list()]).toHaveLength(0); + expect(impl.storage.observers.get("mallory")).toBeDefined(); + }); + }); + + it("refuses the observation when the connection is removed mid-teardown", async () => { + let stub = env.TEST_OVERSEER.getByName("restricted-latch-removed-mid-teardown"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = getImpl(instance); + seedGatekeeper(impl, 1); + impl.storage.observers.put( + { profileId: "mallory", observerId: "obs-m", accountChoices: { 1: 10 } }); + + let held = deferred(); + impl.getGatekeeperFacet = () => ({ + removeObserver: async () => { await held.promise; }, + }); + + let observation = impl.authorizeObservation( + 1, RESTRICTED_EXCLUDING_MALLORY, { from: "user" }); + await tick(); + + // The latch isn't set during the teardown, so removalBlockedByRestrictedData doesn't + // protect the producer in this window; the connection is removed out from under the + // in-flight observation. + impl.storage.gatekeepers.delete(1); + + held.resolve(); + // The post-teardown re-read catches the removal: refused, and nothing latched or recorded. + await expect(observation).rejects.toThrow(/has been removed/); + expect(impl.storage.containsRestrictedData.get()).toBe(false); + expect([...impl.storage.actions.list()]).toHaveLength(0); + }); + }); + + it("refuses restricted data through a removed connection instead of bricking sharing", async () => { + let stub = env.TEST_OVERSEER.getByName("restricted-latch-missing-producer"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = getImpl(instance); + // No gatekeeper record: the in-flight facet RPC outlived removeGatekeeper. With zero + // collaborators nothing else refuses it, so only this guard stands between the observation + // and latching a missing producer id. + await expect(impl.authorizeObservation(1, { + title: "Read a thing", + description: "The test read a thing.", + containsRestrictedData: true, + }, { from: "user" })).rejects.toThrow(/has been removed/); + + // A blocked observation delivered no data: the workspace must not be left restricted -- + // and above all must not be left permanently unshareable by latching a missing producer. + expect(impl.storage.containsRestrictedData.get()).toBe(false); + expect(() => impl.assertNewSharingAllowed()).not.toThrow(); + }); + }); + + it("refuses an unverifiable producer's restricted data on a shared workspace", async () => { + let stub = env.TEST_OVERSEER.getByName("restricted-latch-unverifiable-shared"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = getImpl(instance); + // An AI model binding has no vendor account behind it, so #inScopeGatekeepers skips it and + // no collaborator is ever asked to verify against it -- the one producer admission cannot + // enforce, and the only one this check still refuses. + impl.storage.gatekeepers.put({ + id: 1, + resourceTitle: "Claude", + class: {} as any, + creationSpec: { + type: "aiModel", modelId: "m", provider: "anthropic", modelName: "claude", + }, + }); + impl.storage.collaborators.put({ + profile: { id: "mallory", name: "Mallory" }, + addedBy: [{ type: "user", sharer: OWNER, created: new Date(), role: "build" }], + }); + + await expect(impl.authorizeObservation(1, { + title: "Read a thing", + description: "The test read a thing.", + containsRestrictedData: true, + }, { from: "user" })).rejects.toThrow(/cannot verify anyone's access/); + + // Refused, so nothing latched: the owner can still unshare and read it. + expect(impl.storage.containsRestrictedData.get()).toBe(false); + expect([...impl.storage.actions.list()]).toHaveLength(0); + }); + }); + + it("refuses an unverifiable producer's restricted data while a share link is outstanding", async () => { + let stub = env.TEST_OVERSEER.getByName("restricted-latch-unverifiable-link-only"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = getImpl(instance); + impl.storage.gatekeepers.put({ + id: 1, + resourceTitle: "Claude", + class: {} as any, + creationSpec: { + type: "aiModel", modelId: "m", provider: "anthropic", modelName: "claude", + }, + }); + // No collaborators, but a link the owner has already handed out. Its key never expires and + // can be redeemed any number of times; had this read latched, assertNewSharingAllowed would + // refuse every redemption, stranding the link. Same predicate as + // removalBlockedByRestrictedData. + impl.storage.shareKeys.put({ + id: "link-1", created: new Date(), createdBy: OWNER, role: "build", + }); + + await expect(impl.authorizeObservation(1, { + title: "Read a thing", + description: "The test read a thing.", + containsRestrictedData: true, + }, { from: "user" })).rejects.toThrow(/cannot verify anyone's access/); + + expect(impl.storage.containsRestrictedData.get()).toBe(false); + expect([...impl.storage.actions.list()]).toHaveLength(0); + }); + }); + + it("admits an unverifiable producer's restricted data on a solo workspace", async () => { + let stub = env.TEST_OVERSEER.getByName("restricted-latch-unverifiable-solo"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = getImpl(instance); + impl.storage.gatekeepers.put({ + id: 1, + resourceTitle: "Claude", + class: {} as any, + creationSpec: { + type: "aiModel", modelId: "m", provider: "anthropic", modelName: "claude", + }, + }); + + // Nobody to under-verify: the owner reads their own data, and the latch is what keeps it + // that way. + await expect(impl.authorizeObservation(1, { + title: "Read a thing", + description: "The test read a thing.", + containsRestrictedData: true, + }, { from: "user" })).resolves.toBeUndefined(); + + expect(impl.storage.containsRestrictedData.get()).toBe(true); + expect(() => impl.assertNewSharingAllowed()).toThrow(); + }); + }); +}); diff --git a/packages/workshop-backend/__tests__/restricted-producer-removal.test.ts b/packages/workshop-backend/__tests__/restricted-producer-removal.test.ts new file mode 100644 index 000000000..8127507b6 --- /dev/null +++ b/packages/workshop-backend/__tests__/restricted-producer-removal.test.ts @@ -0,0 +1,367 @@ +// removalBlockedByRestrictedData() is the single predicate behind the producer-removal guard: +// GatekeeperClientImpl.remove() refuses on it, and ensureAmbientCapsules()'s reconciliation skips +// stale records on it. It must block exactly when deleting the record would readmit an unverified +// party -- the workspace is latched, the record is a restricted producer (verifiable or not), and +// the sharing graph still has collaborators or outstanding share links. +// +// Runs against a real OverseerDurableObject (the TEST_OVERSEER binding, like +// git-migration-do.test.ts) so the predicate reads real storage; records are seeded directly +// through the impl. + +import { describe, expect, it } from "vitest"; +import { env } from "cloudflare:workers"; +import { runInDurableObject } from "cloudflare:test"; +import type { OverseerDurableObject } from "../src/overseer.js"; + +declare module "cloudflare:workers" { + interface ProvidedEnv { + TEST_OVERSEER: DurableObjectNamespace; + } +} + +const OWNER = "alice"; + +function getImpl(instance: OverseerDurableObject): any { + let impl = (instance as unknown as { impl: any }).impl; + // The sharing manager resolves collaborator reachability from the owner; seed the cached + // profile id so no User DO round trip is attempted. + impl.ownerProfileId = OWNER; + return impl; +} + +// A verifiable connection record, or (without `creationSpec`) a legacy one -- unverifiable, and +// guarded all the same. +function seedGatekeeper(impl: any, id: number, creationSpec = true): void { + impl.storage.gatekeepers.put({ + id, + resourceTitle: `Connection ${id}`, + class: {} as any, + ...(creationSpec ? { + creationSpec: { + type: "gatekeeper", + vendorId: "testvendor", + resourceUrl: `https://example.com/${id}`, + typeUrlPattern: "https://*", + }, + } : {}), + }); +} + +// A restricted observation attributed to `gatekeeperId`, which is what makes it a producer +// (restrictedProducerIds scans the action log for exactly these). +function seedRestrictedObservation(impl: any, gatekeeperId: number, actionId: number): void { + impl.storage.actions.put({ + id: actionId, + gatekeeperId, + caller: { from: "user" }, + createdAt: new Date(), + state: "approved", + type: "observation", + description: { + title: "Read a thing", + description: "The test read a thing.", + containsRestrictedData: true, + }, + }); +} + +function seedCollaborator(impl: any): void { + impl.storage.collaborators.put({ + profile: { id: "bob", name: "Bob" }, + addedBy: [{ type: "user", sharer: OWNER, created: new Date(), role: "build" }], + }); +} + +function seedShareLink(impl: any): void { + impl.storage.shareKeys.put({ + id: "link-1", + created: new Date(), + createdBy: OWNER, + role: "build", + }); +} + +describe("removalBlockedByRestrictedData", () => { + it("does not block while the workspace is unlatched", async () => { + let stub = env.TEST_OVERSEER.getByName("producer-removal-unlatched"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = getImpl(instance); + seedGatekeeper(impl, 1); + seedRestrictedObservation(impl, 1, 100); + seedCollaborator(impl); + + expect(impl.removalBlockedByRestrictedData(1, await impl.getSharingManager())).toBe(false); + }); + }); + + it("does not block a latched non-producer, even while shared", async () => { + let stub = env.TEST_OVERSEER.getByName("producer-removal-non-producer"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = getImpl(instance); + seedGatekeeper(impl, 1); + seedGatekeeper(impl, 2); + seedRestrictedObservation(impl, 1, 100); + impl.storage.containsRestrictedData.put(true); + seedCollaborator(impl); + + let sharing = await impl.getSharingManager(); + expect(impl.removalBlockedByRestrictedData(2, sharing)).toBe(false); + expect(impl.removalBlockedByRestrictedData(1, sharing)).toBe(true); + }); + }); + + it("blocks a legacy (unverifiable) producer while a collaborator exists", async () => { + let stub = env.TEST_OVERSEER.getByName("producer-removal-legacy"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = getImpl(instance); + seedGatekeeper(impl, 1, /* creationSpec */ false); + seedRestrictedObservation(impl, 1, 100); + impl.storage.containsRestrictedData.put(true); + seedCollaborator(impl); + + // The legacy record is what denies every non-owner open (#inScopeGatekeepers throws on + // it), so removing it while shared would readmit the collaborator unverified. + expect(impl.removalBlockedByRestrictedData(1, await impl.getSharingManager())).toBe(true); + }); + }); + + it("blocks on an outstanding share link alone", async () => { + let stub = env.TEST_OVERSEER.getByName("producer-removal-link-only"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = getImpl(instance); + seedGatekeeper(impl, 1); + seedRestrictedObservation(impl, 1, 100); + impl.storage.containsRestrictedData.put(true); + seedShareLink(impl); + + // No collaborator yet, but the link's keys are multi-redeemable and redemption is gated + // only while the record exists. + expect(impl.removalBlockedByRestrictedData(1, await impl.getSharingManager())).toBe(true); + }); + }); + + it("does not block a latched producer while the workspace is unshared", async () => { + let stub = env.TEST_OVERSEER.getByName("producer-removal-unshared"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = getImpl(instance); + seedGatekeeper(impl, 1); + seedRestrictedObservation(impl, 1, 100); + impl.storage.containsRestrictedData.put(true); + + expect(impl.removalBlockedByRestrictedData(1, await impl.getSharingManager())).toBe(false); + }); + }); + + it("falls back to guarding every connection when the latch is set with no producer", async () => { + let stub = env.TEST_OVERSEER.getByName("producer-removal-empty-producers"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = getImpl(instance); + seedGatekeeper(impl, 1); + // Should be impossible (the latch and its action record are written together), so fail + // closed: with the latch set and no derivable producer set, everything is guarded. + impl.storage.containsRestrictedData.put(true); + seedCollaborator(impl); + + expect(impl.removalBlockedByRestrictedData(1, await impl.getSharingManager())).toBe(true); + }); + }); +}); + +// GatekeeperClientImpl.remove() must make its decision against sharing state read *after* its +// only real yield (the cold sharing manager's whoami RPC) and in the same synchronous block as +// the delete: a grant landing during the yield is seen by the check, and nothing can land +// between the check and the delete. Pinned by parking whoami on a deferred so the yield is a +// real in-test suspension point. +describe("GatekeeperClientImpl.remove ordering", () => { + it("sees a collaborator granted while the sharing manager is being fetched", async () => { + let stub = env.TEST_OVERSEER.getByName("producer-removal-mid-yield-grant"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + // Not getImpl(): impl.ownerProfileId stays unset so getSharingManager() must fetch the + // owner profile through the (stubbed) owner User DO -- the parked deferred below. + let impl = (instance as unknown as { impl: any }).impl; + let releaseWhoami!: (profile: { id: string; name: string }) => void; + let whoami = new Promise<{ id: string; name: string }>(resolve => { + releaseWhoami = resolve; + }); + impl.ownerId = "owner-do-id"; + impl.users = { + idFromString: (id: string) => id, + get: () => ({ whoami: () => whoami }), + }; + impl.getGatekeeperFacet = () => ({ + describe: async () => ({ title: "Producer", url: "test://producer" }), + }); + + // A real client for a real record, so the test drives the actual remove() path. + let client = await impl.addGatekeeper({} as any, { + type: "gatekeeper", + vendorId: "testvendor", + resourceUrl: "https://example.com/producer", + typeUrlPattern: "https://*", + }); + let id = await client.getId(); + seedRestrictedObservation(impl, id, 100); + impl.storage.containsRestrictedData.put(true); + + // Start the removal: it runs synchronously up to the parked whoami. Grant a collaborator + // mid-park, then release -- the decision must see the grant and refuse. + let removal = client.remove(); + seedCollaborator(impl); + releaseWhoami({ id: OWNER, name: "Alice" }); + + await expect(removal).rejects.toThrow(/cannot be removed/); + expect(impl.storage.gatekeepers.get(id)).toBeDefined(); + }); + }); +}); + +// The ambient reconciliation removes a capsule record whose account is gone or was replaced -- +// an internal removal that must consult the same guard: a stale record that is a restricted +// producer still anchors collaborator verification. +describe("ensureAmbientCapsules reconciliation", () => { + const AMBIENT_ID = 1; + + // Seeds a stale ambient producer (record bound to accountId 10, owner now holding accountId + // 20) plus the latch, and fakes the owner's User DO and the gatekeeper facet so + // ensureAmbientCapsules can run without any real cross-DO call. + function seedStaleAmbientProducer(impl: any): void { + impl.storage.gatekeepers.put({ + id: AMBIENT_ID, + resourceTitle: "Test Ambient", + class: {} as any, + creationSpec: { type: "ambient", vendorId: "testvendor", accountId: 10 }, + }); + // Keep freshly-provisioned records clear of the seeded id. + impl.storage.nextGatekeeperId.put(10); + seedRestrictedObservation(impl, AMBIENT_ID, 100); + impl.storage.containsRestrictedData.put(true); + + impl.ownerId = "owner-do-id"; + impl.users = { + idFromString: (id: string) => id, + get: () => ({ + listProvidedAccounts: async () => [{ + vendorId: "testvendor", + accountId: 20, + description: { singleton: { tsType: "TestThing" } }, + }], + getSingletonGatekeeperClass: async () => ({} as any), + }), + }; + impl.getGatekeeperFacet = () => ({ + describe: async () => ({ title: "Test Ambient", url: "test://ambient" }), + }); + } + + function ambientRecords(impl: any): { id: number; accountId: number }[] { + return [...impl.storage.gatekeepers.list()] + .filter((gk: any) => gk.creationSpec?.type === "ambient") + .map((gk: any) => ({ id: gk.id, accountId: gk.creationSpec.accountId })); + } + + it("keeps a guarded stale producer and still provisions the replacement", async () => { + let stub = env.TEST_OVERSEER.getByName("ambient-reconcile-guarded"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = getImpl(instance); + seedStaleAmbientProducer(impl); + seedCollaborator(impl); + + await impl.ensureAmbientCapsules(); + + // The stale record anchors the collaborator's verification, so it survives; the + // replacement account still gets its own fresh capsule record. + let records = ambientRecords(impl); + expect(records).toContainEqual({ id: AMBIENT_ID, accountId: 10 }); + expect(records.filter(r => r.accountId === 20)).toHaveLength(1); + }); + }); + + it("still reconciles a stale producer away while the workspace is unshared", async () => { + let stub = env.TEST_OVERSEER.getByName("ambient-reconcile-unshared"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = getImpl(instance); + seedStaleAmbientProducer(impl); + + await impl.ensureAmbientCapsules(); + + let records = ambientRecords(impl); + expect(records.find(r => r.id === AMBIENT_ID)).toBeUndefined(); + expect(records.filter(r => r.accountId === 20)).toHaveLength(1); + }); + }); +}); + +// assertNewSharingAllowed() must refuse every new grant once a restricted producer cannot verify +// collaborators -- whether its record is gone, is legacy (no creationSpec; observerVendorId +// throws, so recipients hard-deny at open while the grant blocks producer removal), or is an +// aiModel/agentSpawner producer with no vendor account (filtered out of every verification scope, +// so recipients would open completely unverified and read the restricted history in chat). +describe("assertNewSharingAllowed", () => { + it("allows sharing while a verifiable producer's record survives", async () => { + let stub = env.TEST_OVERSEER.getByName("sharing-allowed-verifiable"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = getImpl(instance); + seedGatekeeper(impl, 1); + seedRestrictedObservation(impl, 1, 100); + impl.storage.containsRestrictedData.put(true); + + expect(() => impl.assertNewSharingAllowed()).not.toThrow(); + }); + }); + + it("refuses when a producer's record has been removed", async () => { + let stub = env.TEST_OVERSEER.getByName("sharing-allowed-removed"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = getImpl(instance); + seedRestrictedObservation(impl, 1, 100); + impl.storage.containsRestrictedData.put(true); + + expect(() => impl.assertNewSharingAllowed()).toThrow(/has since been removed/); + }); + }); + + it("refuses a legacy producer that cannot verify collaborators", async () => { + let stub = env.TEST_OVERSEER.getByName("sharing-allowed-legacy"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = getImpl(instance); + seedGatekeeper(impl, 1, /* creationSpec */ false); + seedRestrictedObservation(impl, 1, 100); + impl.storage.containsRestrictedData.put(true); + + expect(() => impl.assertNewSharingAllowed()).toThrow(/cannot verify collaborators/); + }); + }); + + it("refuses an aiModel producer that cannot verify collaborators", async () => { + let stub = env.TEST_OVERSEER.getByName("sharing-allowed-ai-model"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = getImpl(instance); + impl.storage.gatekeepers.put({ + id: 1, + resourceTitle: "AI model", + class: {} as any, + creationSpec: { + type: "aiModel", modelId: "m-1", provider: "anthropic", modelName: "claude-sonnet-5", + }, + }); + seedRestrictedObservation(impl, 1, 100); + impl.storage.containsRestrictedData.put(true); + + // No vendor account stands behind the producer (observerVendorId returns null), so no + // recipient could ever be verified against it. + expect(() => impl.assertNewSharingAllowed()).toThrow(/cannot verify collaborators/); + }); + }); + + it("never refuses while the workspace is unlatched", async () => { + let stub = env.TEST_OVERSEER.getByName("sharing-allowed-unlatched"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = getImpl(instance); + // Even a restricted-looking observation through a missing record does not refuse without + // the latch: the latch and the record are written together, so unlatched means none. + seedRestrictedObservation(impl, 1, 100); + + expect(() => impl.assertNewSharingAllowed()).not.toThrow(); + }); + }); +}); diff --git a/packages/workshop-backend/__tests__/sharing.test.ts b/packages/workshop-backend/__tests__/sharing.test.ts index 720908f02..a55b9f782 100644 --- a/packages/workshop-backend/__tests__/sharing.test.ts +++ b/packages/workshop-backend/__tests__/sharing.test.ts @@ -96,27 +96,6 @@ describe("authorization", () => { expect(mgr.getEffectiveRole("a")).toBe("use"); expect(mgr.getEffectiveRole("b")).toBe("build"); }); - - it("hasAnyShares reflects current reachability, not table membership", () => { - let { storage, mgr } = makeManager(); - expect(mgr.hasAnyShares()).toBe(false); - - // An active share link counts as a share. - seedLink(storage, "k1", OWNER); - expect(mgr.hasAnyShares()).toBe(true); - - // A revoked link does not. - storage.shareKeys.put({ id: "k1", created: new Date(), createdBy: OWNER, revoked: true }); - expect(mgr.hasAnyShares()).toBe(false); - - // A reachable collaborator counts. - seedCollaborator(storage, "a", [userEdge(OWNER)]); - expect(mgr.hasAnyShares()).toBe(true); - - // A collaborator whose record lingers but is unreachable does not. - storage.collaborators.put({ profile: profile("a"), addedBy: [] }); - expect(mgr.hasAnyShares()).toBe(false); - }); }); describe("redeemShareKey", () => { @@ -190,6 +169,52 @@ describe("redeemShareKey", () => { }); expect(storage.collaborators.get("a")).toBeUndefined(); }); + + it("a throwing assertGrantAllowed rejects a new recipient with nothing persisted", async () => { + let { storage, mgr } = makeManager(); + let { key } = await mgr.createShareLink({ caller: owner, role: "build" }); + + await expect(mgr.redeemShareKey({ + rawKey: key, profileId: "a", fetchProfile: async () => profile("a"), + assertGrantAllowed: () => { throw new Error("sharing is closed"); }, + })).rejects.toThrow(/sharing is closed/); + + // No collaborator record and no edge were written. + expect(storage.collaborators.get("a")).toBeUndefined(); + }); + + it("does not invoke assertGrantAllowed for an already-existing edge", async () => { + let { mgr } = makeManager(); + let { key } = await mgr.createShareLink({ caller: owner, role: "build" }); + await mgr.redeemShareKey({ + rawKey: key, profileId: "a", fetchProfile: async () => profile("a"), + }); + + // An existing edge is an existing grant, not a new one: the redemption stays a no-op even + // when policy forbids new sharing (a collaborator re-opening with a retained key). + await expect(mgr.redeemShareKey({ + rawKey: key, profileId: "a", fetchProfile: async () => profile("a"), + assertGrantAllowed: () => { throw new Error("sharing is closed"); }, + })).resolves.toBeUndefined(); + expect(mgr.getEffectiveRole("a")).toBe("build"); + }); + + it("invokes a passing assertGrantAllowed once and writes the edge", async () => { + let { storage, mgr } = makeManager(); + let { key, linkId } = await mgr.createShareLink({ caller: owner, role: "build" }); + + let calls = 0; + await mgr.redeemShareKey({ + rawKey: key, profileId: "a", fetchProfile: async () => profile("a"), + assertGrantAllowed: () => { calls++; }, + }); + + expect(calls).toBe(1); + expect(storage.collaborators.get("a")!.addedBy).toEqual([ + expect.objectContaining({ type: "shareKey", keyId: linkId }), + ]); + expect(mgr.getEffectiveRole("a")).toBe("build"); + }); }); describe("addCollaborator", () => { @@ -501,6 +526,26 @@ describe("createShareLink", () => { expect(() => mgr.createShareLink({ caller: collab("a"), role: "build" })) .rejects.toThrow(/higher than your own/); }); + + it("a throwing assertGrantAllowed aborts with nothing persisted", async () => { + let { storage, mgr } = makeManager(); + await expect(mgr.createShareLink({ + caller: owner, role: "build", + assertGrantAllowed: () => { throw new Error("sharing is closed"); }, + })).rejects.toThrow(/sharing is closed/); + // The minted key was discarded, never stored. + expect([...storage.shareKeys.list()]).toEqual([]); + }); + + it("invokes assertGrantAllowed once and persists the grant when it passes", async () => { + let { mgr } = makeManager(); + let calls = 0; + let { linkId } = await mgr.createShareLink({ + caller: owner, role: "use", assertGrantAllowed: () => { calls++; }, + }); + expect(calls).toBe(1); + expect(mgr.listShareLinkRecords().map(r => r.id)).toEqual([linkId]); + }); }); describe("newShareLinkKey", () => { @@ -562,6 +607,23 @@ describe("newShareLinkKey", () => { .rejects.toThrow(/higher than your own/); }); + it("a throwing assertGrantAllowed aborts the copy with nothing persisted", async () => { + let { storage, mgr } = makeManager(); + let { linkId } = await mgr.createShareLink({ caller: owner, role: "build" }); + + await expect(mgr.newShareLinkKey({ + caller: owner, linkId, + assertGrantAllowed: () => { throw new Error("sharing is closed"); }, + })).rejects.toThrow(/sharing is closed/); + // Only the original link record remains; the aborted copy's key was never stored. + expect([...storage.shareKeys.list()].map(r => r.id)).toEqual([linkId]); + + let calls = 0; + await mgr.newShareLinkKey({ caller: owner, linkId, assertGrantAllowed: () => { calls++; } }); + expect(calls).toBe(1); + expect([...storage.shareKeys.list()]).toHaveLength(2); + }); + it("cannot manage a link through the id of one of its copies", async () => { let { storage, mgr } = makeManager(); await mgr.createShareLink({ caller: owner, role: "build" }); From d15aff7a591b4ce4cf8362fad17442597f6f87ea Mon Sep 17 00:00:00 2001 From: Maximo Guk <62088388+Maximo-Guk@users.noreply.github.com> Date: Fri, 28 Aug 2026 14:51:29 -0500 Subject: [PATCH 4/7] Restricted data part 4: Integration tests. 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 --- .../__tests__/observer-role-scope.test.ts | 20 +- .../__tests__/sensitive-observations.test.ts | 676 ++++++++++++++++++ .../gatekeeper-test/src/test-gatekeeper.ts | 10 +- 3 files changed, 695 insertions(+), 11 deletions(-) create mode 100644 packages/integration-tests/__tests__/sensitive-observations.test.ts diff --git a/packages/integration-tests/__tests__/observer-role-scope.test.ts b/packages/integration-tests/__tests__/observer-role-scope.test.ts index 2100cae66..0b040d753 100644 --- a/packages/integration-tests/__tests__/observer-role-scope.test.ts +++ b/packages/integration-tests/__tests__/observer-role-scope.test.ts @@ -5,10 +5,10 @@ // that widening restarts the workspace: each client's next open re-verifies against the new scope. // The external-message gate's role scoping is covered by external-message-verification.test.ts. // -// This lives in its own file -- with its own harness, like every suite here -- and stays small on -// purpose: a DO reset makes the shared local harness briefly drop unrelated in-flight requests, so -// the concurrent tests of any suite that restarts a workspace pass on their current timing, and -// growing the file re-rolls those dice. +// This lives in its own file -- with its own harness, like every suite here -- rather than in +// sensitive-observations.test.ts, because both suites restart the workspace, and a DO reset makes +// the shared local harness briefly drop unrelated in-flight requests; their concurrent tests pass +// with their current timing, but growing either file re-rolls those dice. import { afterAll, beforeAll, describe, expect, it } from "vitest"; import type { RpcStub } from "capnweb"; @@ -162,8 +162,9 @@ describe("role-scoped observer enforcement", () => { const carolSession = await holdSession(ws, carol); try { // Carol holds no coverage for the connection and never will while it stays unbound, but - // that is enforced against her open, not against the owner's own use of the connection. - await expect(ws.session.readValue()).resolves.toBe(42); + // that is enforced against her open, not against the owner's reads: this restricted read + // goes through. + await expect(ws.session.readValue(true)).resolves.toBe(42); // Binding the connection to a gadget (pure storage writes; no gadget code runs) brings it // into "use" scope. That widens what Carol's live session must be verified against, and a @@ -176,9 +177,10 @@ describe("role-scoped observer enforcement", () => { const reopened = await reopenAfterRestart(ws); try { - // The owner is back on the workspace with a working session: the restart is a re-open for - // everyone, not a lockout. - await expect(reopened.session.readValue()).resolves.toBe(42); + // The owner is back on the workspace with a working session, and their restricted read is + // undisturbed by the widening: the restart is a re-open for everyone, not a lockout, and + // nothing about Carol's coverage gates the owner's reads. + await expect(reopened.session.readValue(true)).resolves.toBe(42); // Carol's forced re-open is where the newly in-scope connection gets verified, and she is // asked about exactly it -- the one connection her role's scope just gained. diff --git a/packages/integration-tests/__tests__/sensitive-observations.test.ts b/packages/integration-tests/__tests__/sensitive-observations.test.ts new file mode 100644 index 000000000..8dbba0d24 --- /dev/null +++ b/packages/integration-tests/__tests__/sensitive-observations.test.ts @@ -0,0 +1,676 @@ +// Tests for the sensitive-data (`containsRestrictedData`) observation policy. +// +// Coverage is enforced at admission, not at the read: every collaborator passes the producing +// gatekeeper's `addObserver` at their most recent open and cannot open without passing it, and +// anything that widens what they must pass restarts the workspace so every live session re-opens +// against the new scope. So sensitive observations are not blocked by an unverified collaborator, +// and sharing stays available. The observation also latches the workspace into a restricted mode: +// once latched, the workspace may not perform actions (nor fetch from the web, which has no +// client-reachable surface to assert here). +// +// The fixture gatekeeper's session drives all of this through the real ApprovalQueue funnel: +// `readValue(true)` records a `containsRestrictedData` observation, `writeValue()` submits an +// action (held for the owner's approval, so a test that wants it to go through approves it via +// the overseer). + +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import type { RpcStub } from "capnweb"; +import type { + AuthenticatedApi, Overseer, PublicApi, +} from "@gadgets/workshop-shared/api"; +import { + startTestGatekeeperHarness, TEST_GATEKEEPER_WORKER, TEST_VENDOR_ID, type Harness, +} from "../src/harness.js"; +import type { TestSession } from "../fixtures/gatekeeper-test/src/test-gatekeeper.js"; +import { + accountLabel, connect, listConnectedAccounts, logIn, MAX_OBSERVER_PROMPTS, nextUsernames, + ObserverConfigRecorder, signUp, stubFor, waitFor, type ConnectedAccount, +} from "../src/rpc-client.js"; +import { NetworkInterceptor } from "../src/network-interceptor.js"; + +let harness: Harness; +let interceptor: NetworkInterceptor; + +beforeAll(async () => { + interceptor = new NetworkInterceptor(); + interceptor.install(); + harness = await startTestGatekeeperHarness(); +}); + +afterAll(async () => { + const unmocked = interceptor.getUnmockedCalls(); + await harness?.server.close(); + interceptor.uninstall(); + interceptor.reset(); + expect(unmocked).toEqual([]); +}); + +async function withSession(body: (api: RpcStub) => Promise): Promise { + const publicApi = connect(harness.url); + try { + return await body(publicApi); + } finally { + publicApi[Symbol.dispose](); + } +} + +function thingUrl(name: string): string { + return `https://gadgets-test.example/things/${name}`; +} + +async function provisionAccount(api: RpcStub): Promise { + await api.provisionAmbientAccount(TEST_VENDOR_ID); + return waitFor("the test account to be provisioned", async () => { + const accounts = await listConnectedAccounts(api); + return accounts.find(a => a.vendorId === TEST_VENDOR_ID) ?? null; + }); +} + +/** + * Tell the fixture gatekeeper whether to admit `label` as an observer -- everywhere, or (with + * `resourceUrl`) at one bound resource only, which wins over the account-wide outcome. + */ +async function setVerifyOutcome( + label: string, outcome: { allow: true } | { allow: false; reason: string }, + resourceUrl?: string): Promise { + const res = await harness.fetchWorker( + TEST_GATEKEEPER_WORKER, "http://gatekeeper-test.test/control/verify-outcome", + { method: "POST", body: JSON.stringify({ label, resourceUrl, ...outcome }) }); + if (res.status !== 204) { + throw new Error(`Setting the verify outcome failed with ${res.status}: ${await res.text()}`); + } +} + +type Workspace = { + gadgetId: string; + overseer: RpcStub; + alice: string; + aliceApi: RpcStub; + /** The fixture session bound to the workspace's (first) gatekeeper. */ + session: RpcStub; + gatekeeperId: number; +}; + +// Alice creates a workspace bound to one Test Thing and opens a session on its gatekeeper. Every +// test starts here; collaborators and links are layered on per test. +async function newWorkspace(publicApi: RpcStub, thingName: string): Promise { + const [alice] = nextUsernames("alice"); + const aliceApi = await signUp(publicApi, alice); + const account = await provisionAccount(aliceApi); + + const overseer = await aliceApi.newGadget(); + const gatekeeper = await overseer.newGatekeeper(account.id, thingUrl(thingName)); + if (!gatekeeper) throw new Error("Failed to create the test connection"); + const gatekeeperId = await gatekeeper.getId(); + const session = await gatekeeper.openSession() as RpcStub; + const { id: gadgetId } = await overseer.getMetadata(); + return { gadgetId, overseer, alice, aliceApi, session, gatekeeperId }; +} + +type Bob = { + bob: string; + bobProfileId: string; + bobApi: RpcStub; + bobAccount: ConnectedAccount; + bobLabel: string; +}; + +// Sign Bob up, add him as a collaborator, and give him his own fixture account. +async function addBob(publicApi: RpcStub, ws: Workspace): Promise { + const [bob] = nextUsernames("bob"); + const bobApi = await signUp(publicApi, bob); + const bobAccount = await provisionAccount(bobApi); + const collaborator = await ws.overseer.addCollaborator(bob, "build"); + if (!collaborator) throw new Error(`Failed to share the gadget with ${bob}`); + return { + bob, bobProfileId: collaborator.profile.id, bobApi, bobAccount, + bobLabel: accountLabel(bobAccount), + }; +} + +// Bob opens the workspace, answering observer prompts with his own account. This is what writes +// his observer record, i.e. verifies him against every in-scope gatekeeper. Pass a `recorder` to +// assert *which* connections the open asked him about. +async function bobOpens(gadgetId: string, bobApi: RpcStub, + bobAccount: ConnectedAccount, + recorder?: ObserverConfigRecorder): Promise> { + const callback = stubFor( + recorder ?? new ObserverConfigRecorder().alwaysChoose(bobAccount.id, MAX_OBSERVER_PROMPTS)); + try { + return await bobApi.openGadget(gadgetId, undefined, callback); + } finally { + callback[Symbol.dispose](); + } +} + +// Wait out a restart and come back on a fresh connection, returning the owner's re-opened +// workspace and a session on `gatekeeperId`. +// +// A restart aborts the DO shortly after the triggering call returns, killing every stub from the +// connection that made it. A probe on a fresh connection can only detect a DO that is *already* +// dead -- never one about to die -- so a reopen attempted inside the pre-abort window can fully +// succeed against the doomed instance and then lose its session under the assertions that follow. +// Hence two steps: watch the pre-restart session die, then reopen with retries. +async function reopenAfterRestart(ws: Workspace, gatekeeperId = ws.gatekeeperId): Promise<{ + publicApi: RpcStub; + overseer: RpcStub; + session: RpcStub; +}> { + await waitFor("the restart to fell the old workspace instance", () => + ws.session.readValue().then(() => null, () => true)); + + return waitFor("the workspace to come back after the restart", async () => { + const publicApi = connect(harness.url); + try { + const aliceApi = await logIn(publicApi, ws.alice); + const overseer = await aliceApi.openGadget(ws.gadgetId); + const gatekeeper = await overseer.getGatekeeperById(gatekeeperId); + const session = await gatekeeper.openSession() as RpcStub; + // Probe with a benign read, so a session felled by the abort retries here rather than + // failing an assertion below. + await session.readValue(); + return { publicApi, overseer, session }; + } catch { + publicApi[Symbol.dispose](); + return null; + } + }); +} + +// Bob's forced re-open, on the fresh connection his browser would reconnect with. The restart +// killed the whole session his `bobApi` came from -- every client of the workspace loses its +// connection, not just its workspace stubs -- so reusing it here would fail on a dead socket +// rather than exercising the re-verification this asserts. +async function bobReopens( + ws: Workspace, bob: Bob, recorder: ObserverConfigRecorder): Promise { + const publicApi = connect(harness.url); + try { + const bobApi = await logIn(publicApi, bob.bob); + (await bobOpens(ws.gadgetId, bobApi, bob.bobAccount, recorder))[Symbol.dispose](); + } finally { + publicApi[Symbol.dispose](); + } +} + +// Bob's session, opened on its own connection (the one his browser holds) and kept live until +// close(). What a widening restarts is a live session: a collaborator who is only named in the +// sharing table, or who opened and left, has nothing to sever -- so a test that expects the +// restart must have Bob connected when the widening lands. +type HeldSession = { overseer: RpcStub, close: () => void }; + +async function bobHolds(ws: Workspace, bob: Bob): Promise { + const publicApi = connect(harness.url); + try { + const bobApi = await logIn(publicApi, bob.bob); + const overseer = await bobOpens(ws.gadgetId, bobApi, bob.bobAccount); + return { + overseer, + close: () => { + overseer[Symbol.dispose](); + publicApi[Symbol.dispose](); + }, + }; + } catch (error) { + publicApi[Symbol.dispose](); + throw error; + } +} + +describe("sensitive observations", () => { + it.concurrent("latch restricted mode: actions are blocked and metadata reports it", async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "latch"); + + // Before the latch, actions submit fine -- held for the owner's approval rather than + // refused, and applied once approved -- and metadata is clean. + const write = ws.session.writeValue(7); + const [held] = await waitFor("the write to be held for approval", async () => { + const { entries } = await ws.overseer.listActions({ filter: "pending" }); + return entries.length > 0 ? entries : null; + }); + await ws.overseer.approveAction(held.id); + await expect(write).resolves.toEqual(expect.any(Number)); + expect((await ws.overseer.getMetadata()).containsRestrictedData).toBeFalsy(); + + await expect(ws.session.readValue(true)).resolves.toBe(42); + + expect((await ws.overseer.getMetadata()).containsRestrictedData).toBe(true); + await expect(ws.session.writeValue(8)).rejects.toThrow(/prohibited from performing actions/i); + // Reads -- sensitive or not -- keep working. + await expect(ws.session.readValue()).resolves.toBe(42); + await expect(ws.session.readValue(true)).resolves.toBe(42); + }); + }); + + it.concurrent("an unredeemed share link does not block a sensitive observation", async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "unredeemed"); + await ws.overseer.createShareLink("build", "never redeemed"); + + // An outstanding link grants nobody anything until it is redeemed, and redemption happens + // inside open() -- where verification runs -- so the observation proceeds. + await expect(ws.session.readValue(true)).resolves.toBe(42); + }); + }); + + it.concurrent("sharing stays available after the latch", async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "share-after"); + await expect(ws.session.readValue(true)).resolves.toBe(42); + + // Sharing stays available after the latch, across every sharing RPC. + const [carol] = nextUsernames("carol"); + await signUp(publicApi, carol); + await expect(ws.overseer.addCollaborator(carol, "build")).resolves.toMatchObject({ + profile: expect.objectContaining({ id: expect.any(String) }), + }); + const { linkId } = await ws.overseer.createShareLink("use", "post-latch"); + await expect(ws.overseer.newShareLinkKey(linkId)).resolves.toMatchObject({ + key: expect.any(String), + }); + }); + }); + + it.concurrent("an unverified collaborator does not block a sensitive observation", async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "unverified"); + const bob = await addBob(publicApi, ws); + + // Bob has access but has never opened, so he holds no observer record for this gatekeeper + // -- and no session either, because verification is a precondition of getting one. There is + // nothing for the read to fail closed against. + await expect(ws.session.readValue(true)).resolves.toBe(42); + + // Admission is where the coverage requirement bites: the gatekeeper refuses him, so his + // open is denied and he never reaches the workspace, let alone the observation. + await setVerifyOutcome(bob.bobLabel, { allow: false, reason: "You do not have access." }); + await expect(bobOpens(ws.gadgetId, bob.bobApi, bob.bobAccount)) + .rejects.toThrow(/could not confirm/i); + + // His refusal costs the owner nothing: only his open is denied, so nothing was severed and + // reads keep flowing. + await expect(ws.session.readValue(true)).resolves.toBe(42); + }); + }); + + it.concurrent("a verified collaborator allows the sensitive observation through", async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "verified"); + const bob = await addBob(publicApi, ws); + (await bobOpens(ws.gadgetId, bob.bobApi, bob.bobAccount))[Symbol.dispose](); + + await expect(ws.session.readValue(true)).resolves.toBe(42); + }); + }); + + it.concurrent("adding a connection restarts the workspace so collaborators re-verify", + async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "covered"); + const bob = await addBob(publicApi, ws); + + // Bob verifies against the one connection and stays connected. + const bobSession = await bobHolds(ws, bob); + let lateId: number; + try { + // A second connection Bob has never been verified against. It is in his verification + // scope the moment it exists -- a "build" session can open a session on it with no + // observer check -- and his live session was admitted without it, so adding it severs + // every session. + const accounts = await listConnectedAccounts(ws.aliceApi); + const account = accounts.find(a => a.vendorId === TEST_VENDOR_ID)!; + const late = await ws.overseer.newGatekeeper(account.id, thingUrl("late")); + if (!late) throw new Error("Failed to create the second test connection"); + lateId = await late.getId(); + } finally { + bobSession.close(); + } + + const reopened = await reopenAfterRestart(ws, lateId); + try { + // Nothing is blocked: the owner reads restricted data through the new connection... + await expect(reopened.session.readValue(true)).resolves.toBe(42); + + // ...and Bob's forced re-open is where it gets verified. He is asked about exactly it, + // since his coverage for the connections that predate it survived. + const recorder = new ObserverConfigRecorder() + .alwaysChoose(bob.bobAccount.id, MAX_OBSERVER_PROMPTS); + await bobReopens(ws, bob, recorder); + expect(recorder.callCount).toBe(1); + expect(recorder.calls[0].map(need => need.gatekeeperId)).toEqual([lateId]); + } finally { + reopened.publicApi[Symbol.dispose](); + } + }); + }); + + it.concurrent("a collaborator can open a workspace that latched before they were added", + async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "open-after"); + await expect(ws.session.readValue(true)).resolves.toBe(42); + + // Bob's open runs observer verification, which the fixture admits by default, so the latch + // does not shut him out. + const bob = await addBob(publicApi, ws); + using bobOverseer = await bobOpens(ws.gadgetId, bob.bobApi, bob.bobAccount); + await expect(bobOverseer.getMetadata()).resolves.toMatchObject({ + id: ws.gadgetId, + containsRestrictedData: true, + }); + }); + }); + + it.concurrent("a collaborator the gatekeeper refuses is denied at open, with its reason", + async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "refused"); + await expect(ws.session.readValue(true)).resolves.toBe(42); + + const bob = await addBob(publicApi, ws); + const reason = "You do not have access to this thing."; + await setVerifyOutcome(bob.bobLabel, { allow: false, reason }); + + // This is the strategy-A shape: enforcement lives in the gatekeeper's addObserver(), so + // the user sees the gatekeeper's own message. + const error = await bobOpens(ws.gadgetId, bob.bobApi, bob.bobAccount).then( + overseer => { overseer[Symbol.dispose](); return null; }, + (err: unknown) => err as Error); + expect(error).not.toBeNull(); + expect(error!.message).toMatch(/could not confirm/i); + expect(error!.message).toContain(reason); + }); + }); + + it.concurrent("a failed re-verification denies that open and nothing else", async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "revoked"); + // A second producer, so the test can prove the denial is scoped to the one that refused. + // Added before Bob, so it widens nobody's scope and restarts nothing. + const accounts = await listConnectedAccounts(ws.aliceApi); + const account = accounts.find(a => a.vendorId === TEST_VENDOR_ID)!; + const second = await ws.overseer.newGatekeeper(account.id, thingUrl("revoked-2")); + if (!second) throw new Error("Failed to create the second test connection"); + const secondSession = await second.openSession() as RpcStub; + + // Bob verifies against both producers. + const bob = await addBob(publicApi, ws); + (await bobOpens(ws.gadgetId, bob.bobApi, bob.bobAccount))[Symbol.dispose](); + await expect(ws.session.readValue(true)).resolves.toBe(42); + await expect(secondSession.readValue(true)).resolves.toBe(42); + + // Bob's access to the first producer's resource is revoked; his next open is denied... + await setVerifyOutcome( + bob.bobLabel, { allow: false, reason: "Access revoked." }, thingUrl("revoked")); + await expect(bobOpens(ws.gadgetId, bob.bobApi, bob.bobAccount)) + .rejects.toThrow(/could not confirm/i); + + // ...and only that open. Nothing is severed and the owner's reads keep flowing through both + // producers: Bob cannot be admitted again without re-verifying, which is the whole of the + // enforcement (the lazy-revocation residual documented in docs/observers.md). + await expect(ws.session.readValue(true)).resolves.toBe(42); + await expect(secondSession.readValue(true)).resolves.toBe(42); + + // A returning observer's coverage survives the failure, so once repaired his re-open + // re-verifies both producers from his persisted choices without prompting (the recorder has + // no queued responses, so an unexpected prompt throws). + await setVerifyOutcome(bob.bobLabel, { allow: true }, thingUrl("revoked")); + const recorder = new ObserverConfigRecorder(); + (await bobOpens(ws.gadgetId, bob.bobApi, bob.bobAccount, recorder))[Symbol.dispose](); + expect(recorder.callCount).toBe(0); + }); + }); + + it.concurrent("a refused share-link recipient persists as a collaborator without blocking reads", + async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "refused-link"); + await expect(ws.session.readValue(true)).resolves.toBe(42); + + const { key } = await ws.overseer.createShareLink("build", "refused recipient"); + + const [dave] = nextUsernames("dave"); + const daveApi = await signUp(publicApi, dave); + const daveAccount = await provisionAccount(daveApi); + await setVerifyOutcome( + accountLabel(daveAccount), { allow: false, reason: "You do not have access." }); + + // Dave's open redeems the key -- writing a real edge -- and observer verification then + // refuses him. One-step redemption accepts the residue: he persists as an unverified + // collaborator (see the TODO on redeemShareKey). + const recorder = + new ObserverConfigRecorder().alwaysChoose(daveAccount.id, MAX_OBSERVER_PROMPTS); + const callback = stubFor(recorder); + try { + await expect(daveApi.openGadget(ws.gadgetId, key, callback)) + .rejects.toThrow(/could not confirm/i); + } finally { + callback[Symbol.dispose](); + } + + // The residue is a collaborator row, not access: he never opened, and he cannot open + // without passing the same check. So the owner's reads are untouched -- and nothing was + // severed either, since only his own open was denied. + const collaborators = await ws.overseer.listCollaborators(); + expect(collaborators).toHaveLength(1); + await expect(ws.session.readValue(true)).resolves.toBe(42); + }); + }); + + it.concurrent("concurrent redemptions of the same key both verify", async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "raced"); + const { key } = await ws.overseer.createShareLink("build", "raced"); + + const [dave] = nextUsernames("dave"); + const daveApi = await signUp(publicApi, dave); + const daveAccount = await provisionAccount(daveApi); + + const callbacks = [0, 1].map(() => stubFor( + new ObserverConfigRecorder().alwaysChoose(daveAccount.id, MAX_OBSERVER_PROMPTS))); + try { + // Each open redeems the same key; the edges deduplicate, so neither open is turned away + // and the grants collapse to one edge. + const overseers = await Promise.all( + callbacks.map(cb => daveApi.openGadget(ws.gadgetId, key, cb))); + for (const overseer of overseers) overseer[Symbol.dispose](); + } finally { + for (const cb of callbacks) cb[Symbol.dispose](); + } + + const collaborators = await ws.overseer.listCollaborators(); + expect(collaborators).toHaveLength(1); + expect(collaborators[0].addedBy).toHaveLength(1); + }); + }); + + it.concurrent("a latched connection cannot be removed while the workspace is shared", + async () => { + await withSession(async publicApi => { + // Latched but unshared: removal proceeds. (The latch itself persists; there is nobody + // whose verification the record anchors.) + const solo = await newWorkspace(publicApi, "remove-solo"); + await expect(solo.session.readValue(true)).resolves.toBe(42); + const soloGatekeeper = await solo.overseer.getGatekeeperById(solo.gatekeeperId); + await expect(soloGatekeeper.remove()).resolves.toBeUndefined(); + + // Latched and shared: the record is what Bob's verification runs against, so removing it + // would let him open unchecked while the restricted data persists. + const ws = await newWorkspace(publicApi, "remove-shared"); + await expect(ws.session.readValue(true)).resolves.toBe(42); + await addBob(publicApi, ws); + const gatekeeper = await ws.overseer.getGatekeeperById(ws.gatekeeperId); + await expect(gatekeeper.remove()).rejects.toThrow(/remove all collaborators/i); + // The refused removal left the connection intact. + await expect(ws.session.readValue()).resolves.toBe(42); + }); + }); + + it.concurrent("a latched connection cannot be removed while a share link is outstanding", + async () => { + await withSession(async publicApi => { + // An unredeemed link creates no collaborator state, but its keys are multi-redeemable and + // never expire: redemption is gated at open() only while the gatekeeper record exists, so + // removing the record now would let a later recipient open unchecked. + const ws = await newWorkspace(publicApi, "remove-linked"); + await expect(ws.session.readValue(true)).resolves.toBe(42); + const { linkId } = await ws.overseer.createShareLink("build", "outstanding"); + + const gatekeeper = await ws.overseer.getGatekeeperById(ws.gatekeeperId); + await expect(gatekeeper.remove()).rejects.toThrow(/revoke all share links/i); + // The refused removal left the connection intact. + await expect(ws.session.readValue()).resolves.toBe(42); + + // Nobody redeemed the link, so revoking it affects no collaborator (no revocation restart) + // and unblocks the removal. + await expect(ws.overseer.revokeShareLink(linkId, [])).resolves.toEqual([]); + await expect(gatekeeper.remove()).resolves.toBeUndefined(); + }); + }); + + it.concurrent("the removal guard is scoped to the connection that read the sensitive data", + async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "scoped-producer"); + // A second connection that never reads anything sensitive. + const accounts = await listConnectedAccounts(ws.aliceApi); + const account = accounts.find(a => a.vendorId === TEST_VENDOR_ID)!; + const bystander = await ws.overseer.newGatekeeper(account.id, thingUrl("scoped-bystander")); + if (!bystander) throw new Error("Failed to create the second test connection"); + + // Only the first connection reads restricted data; share after the latch. + await expect(ws.session.readValue(true)).resolves.toBe(42); + await addBob(publicApi, ws); + + // The latch is workspace-wide, but only the producer anchors verification: the bystander + // stays removable while shared, the producer does not. + await expect(bystander.remove()).resolves.toBeUndefined(); + const producer = await ws.overseer.getGatekeeperById(ws.gatekeeperId); + await expect(producer.remove()).rejects.toThrow(/remove all collaborators/i); + await expect(ws.session.readValue()).resolves.toBe(42); + }); + }); + + it.concurrent("a workspace whose sensitive-data producer was removed can no longer be shared", + async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "unshareable"); + await expect(ws.session.readValue(true)).resolves.toBe(42); + + // Unshared, so removal is allowed -- but the restricted data (and the latch) outlive it. + const gatekeeper = await ws.overseer.getGatekeeperById(ws.gatekeeperId); + await expect(gatekeeper.remove()).resolves.toBeUndefined(); + + // With the producer's record gone there is nothing to verify a new collaborator against, + // so the grant-creating mutators refuse. + const [carol] = nextUsernames("carol"); + await signUp(publicApi, carol); + await expect(ws.overseer.addCollaborator(carol, "build")) + .rejects.toThrow(/can no longer be shared/i); + await expect(ws.overseer.createShareLink("use", "too late")) + .rejects.toThrow(/can no longer be shared/i); + }); + }); + + it.concurrent("removal restarts the workspace and tears down the observer record", async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "removal"); + const bob = await addBob(publicApi, ws); + (await bobOpens(ws.gadgetId, bob.bobApi, bob.bobAccount))[Symbol.dispose](); + await expect(ws.session.readValue(true)).resolves.toBe(42); + + // Removing Bob triggers the revocation restart: the DO aborts shortly after this call + // returns, killing every stub from this connection -- including the session Bob holds, + // which is the point. Everything past here runs on a fresh connection. + await ws.overseer.removeCollaborator(bob.bobProfileId, []); + const reopened = await reopenAfterRestart(ws); + + try { + // Bob's collaborator record lingers in storage (lazy revocation), and the owner's reads + // are unaffected either way. + await expect(reopened.session.readValue(true)).resolves.toBe(42); + + // Removal also tore down his observer record, so re-adding him must not silently restore + // his coverage: his next open has to name an account for the producer and pass + // addObserver again. + await reopened.overseer.addCollaborator(bob.bob, "build"); + const recorder = new ObserverConfigRecorder() + .alwaysChoose(bob.bobAccount.id, MAX_OBSERVER_PROMPTS); + await bobReopens(ws, bob, recorder); + expect(recorder.calls[0].map(need => need.gatekeeperId)).toContain(ws.gatekeeperId); + } finally { + reopened.publicApi[Symbol.dispose](); + } + }); + }); + + it.concurrent("ambient reconciliation preserves a shared restricted producer", async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "ambient-reconcile"); + + // Ambient capsule records aren't published to clients, but their workpiece ids are small + // sequential integers, so probe for the one ensureAmbientCapsules provisioned at first open. + const findAmbientIds = async (overseer: RpcStub) => { + const found: number[] = []; + for (let id = 0; id < ws.gatekeeperId + 4; id++) { + try { + const gatekeeper = await overseer.getGatekeeperById(id); + if ((await gatekeeper.getTitle()) === "Test Ambient") found.push(id); + } catch { + // Not a gatekeeper workpiece. + } + } + return found; + }; + const [ambientId] = await findAmbientIds(ws.overseer); + expect(ambientId).toBeDefined(); + + // Latch through the ambient capsule, so it -- not the pasted connection -- is the producer. + const ambient = await ws.overseer.getGatekeeperById(ambientId); + const ambientSession = await ambient.openSession() as RpcStub; + await expect(ambientSession.readValue(true)).resolves.toBe(42); + const bob = await addBob(publicApi, ws); + + // Bob verifies (against the capsule too) and stays connected, held open until the restart + // below has been observed: the reconcile runs in the background after the owner's open + // returns, so closing him any earlier could leave it nothing to sever. + const bobSession = await bobHolds(ws, bob); + try { + // Replace the owner's singleton account: disconnecting and re-provisioning mints a new + // accountId, so the existing capsule record is stale at the next reconcile. + const accounts = await listConnectedAccounts(ws.aliceApi); + const oldAccount = accounts.find(a => a.vendorId === TEST_VENDOR_ID)!; + await ws.aliceApi.disconnectAccount(oldAccount.id); + const newAccount = await provisionAccount(ws.aliceApi); + expect(newAccount.id).not.toBe(oldAccount.id); + + // Reopen. Later opens run the capsule reconcile in the background, and provisioning the + // replacement is itself a connection Bob has never been verified against -- so the + // reconcile restarts the workspace out from under this connection (possibly severing this + // very open, which is fine: the reconcile has run either way). Come back on a fresh one, + // then wait for the replacement's record to appear (proof the reconcile has run). + await ws.aliceApi.openGadget(ws.gadgetId).then( + overseer => overseer[Symbol.dispose](), () => {}); + const reopened = await reopenAfterRestart(ws); + try { + const ids = await waitFor("the replacement ambient capsule to be provisioned", + async () => { + const found = await findAmbientIds(reopened.overseer); + return found.some(id => id !== ambientId) ? found : null; + }); + + // The stale record anchors Bob's verification, so the reconcile must have skipped it: + // the record survives, and sharing -- which refuses once any producer's record is gone + // -- still works. + expect(ids).toContain(ambientId); + await expect(reopened.overseer.createShareLink("build", "still shareable")) + .resolves.toMatchObject({ key: expect.any(String) }); + } finally { + reopened.publicApi[Symbol.dispose](); + } + } finally { + bobSession.close(); + } + }); + }); +}); diff --git a/packages/integration-tests/fixtures/gatekeeper-test/src/test-gatekeeper.ts b/packages/integration-tests/fixtures/gatekeeper-test/src/test-gatekeeper.ts index 97273789a..a8d5dbafd 100644 --- a/packages/integration-tests/fixtures/gatekeeper-test/src/test-gatekeeper.ts +++ b/packages/integration-tests/fixtures/gatekeeper-test/src/test-gatekeeper.ts @@ -302,7 +302,12 @@ export class TestVerifier // Gatekeeper (one per bound resource, running as a facet under the gadget's Overseer) export interface TestSession { - readValue(): Promise; + /** + * Records an observation through the same `ApprovalQueue` funnel a shipping gatekeeper uses. + * `restricted` marks it `containsRestrictedData`, to trip the restricted-mode latch and the + * unverifiable-producer guard. + */ + readValue(restricted?: boolean): Promise; writeValue(value: number): Promise; writeValues(values: number[]): Promise; } @@ -319,10 +324,11 @@ class TestSessionTarget extends RpcTarget implements TestSession { this.approvalQueue = approvalQueue.dup(); } - async readValue(): Promise { + async readValue(restricted?: boolean): Promise { await this.approvalQueue.authorizeObservation({ title: "Read the test value", description: "Read the deterministic value exposed by the integration-test gatekeeper.", + ...(restricted ? { containsRestrictedData: true } : {}), }); return 42; } From 4a886852edb623c10b3b08747a2334e0b8cb7422 Mon Sep 17 00:00:00 2001 From: Maximo Guk <62088388+Maximo-Guk@users.noreply.github.com> Date: Fri, 28 Aug 2026 14:52:39 -0500 Subject: [PATCH 5/7] Restricted data part 5: Document the model. 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 --- .agents/skills/write-gatekeeper/SKILL.md | 2 +- docs/observers.md | 238 +++++++++++++----- docs/sharing.md | 43 +++- plans/restricted-data-sharing.md | 302 +++++++++++++++++++++++ 4 files changed, 507 insertions(+), 78 deletions(-) create mode 100644 plans/restricted-data-sharing.md diff --git a/.agents/skills/write-gatekeeper/SKILL.md b/.agents/skills/write-gatekeeper/SKILL.md index 1e470f73c..0d8a0b49b 100644 --- a/.agents/skills/write-gatekeeper/SKILL.md +++ b/.agents/skills/write-gatekeeper/SKILL.md @@ -243,7 +243,7 @@ async getVerifier(): Promise> { Strategy is chosen **per `Gatekeeper` DO class / binding**, not per package — one package may use several (e.g. Google: Gmail=A, Doc=B, BigQuery=C). -- **A — Private-only.** `addObserver()` always throws; `removeObserver()` is a no-op. `getVerifier()` must still exist (the overseer mints it) but is never consulted. Use when the resource is too sensitive to share and there is no per-observer access oracle (e.g. a personal Gmail mailbox). For truly sensitive data, also mark each observation with `ObservationDescription.containsRestrictedData: true`: the workspace then refuses sensitive observations while any unverified collaborator has access (with strategy A that is every collaborator) and latches into a restricted mode that blocks all actions and public web fetches, so the data cannot leak back out through other gatekeepers. +- **A — Private-only.** `addObserver()` always throws; `removeObserver()` is a no-op. `getVerifier()` must still exist (the overseer mints it) but is never consulted. Use when the resource is too sensitive to share and there is no per-observer access oracle (e.g. a personal Gmail mailbox). For truly sensitive data, also mark each observation with `ObservationDescription.containsRestrictedData: true`: because nobody can open the workspace without passing `addObserver()`, and with strategy A nobody ever does, the first such observation makes the workspace effectively unshareable — and it latches into a restricted mode that blocks all actions and public web fetches, so the data cannot leak back out through other gatekeepers. - **B — ACL check (single unit).** The binding is one atomic resource; sub-resources inherit its ACL. `addObserver()` calls a verifier method to confirm the observer can access it and throws otherwise; `removeObserver()` is a no-op; nothing is tracked and no `excludeObservers` is ever needed. Use for repo / document / page / team / single-project bindings. - **C — Data-set tracking.** The binding spans sub-resources with **distinct ACLs**, and there is a **per-observer access oracle** for each. The DO logs the data sets actually observed and the current observers; `addObserver()` verifies the observer against **every** logged set (plus a coarse membership baseline) and **stores their verifier**; each later observation that first touches a **new** set re-checks all stored observers and sets `excludeObservers` for any who fail. Use for workspace / organization / dataset-spanning bindings. - **D — Low-stakes.** `addObserver()` / `removeObserver()` are no-ops; `getVerifier()` returns a trivial verifier with a no-op public method such as `verify(): void {}` (an empty `WorkerEntrypoint` is not registered in `ctx.exports`). Use when any collaborator may observe (personal, low-stakes services). diff --git a/docs/observers.md b/docs/observers.md index 0dc8db05e..c1cea0a84 100644 --- a/docs/observers.md +++ b/docs/observers.md @@ -30,14 +30,18 @@ Gadgets enforce a core security invariant (see `overview.md` §"Security Model") > able to read that information will also be prohibited from interacting with the Gadget, > to prevent data leaks. -Today the only mechanism enforcing this is the blunt **`containsRestrictedData`** flag -(`packages/workshop-shared/src/gatekeeper.ts`, `ObservationDescription.containsRestrictedData`). -When a gatekeeper marks an observation as maximally sensitive, the Gadget can no longer be -shared with *anyone*, and it drops into "lockdown" (no further actions, no web fetches). This -is a deliberate stopgap — it cannot express "this data may be shared, but only with people who -*also* have access to it." - -This feature replaces that all-or-nothing posture with a per-user, gatekeeper-mediated check: +The mechanism is a per-user, gatekeeper-mediated check — "this data may be shared, but only with +people who *also* have access to it". (Maximally sensitive data gets an extra layer: an +observation marked **`containsRestrictedData`** +(`ObservationDescription.containsRestrictedData` in `packages/workshop-shared/src/gatekeeper.ts`) +latches the workspace into a restricted mode — no actions, no web fetches. Its coverage rests on +admission: nobody can open the workspace without being verified against the producing gatekeeper, +and anything that widens what they must be verified against restarts every live session. The one +producer admission cannot see — one with no vendor account behind it — is refused outright while +the workspace is shared; see `#assertUnverifiableProducerUnshared` in `overseer.ts` and edge case +4 below.) + +The check works as follows: - **Observers.** Every non-owner who can see data the Gadget read is an *observer*. When a user becomes an observer, each relevant gatekeeper is asked — via `Gatekeeper.addObserver()` — to @@ -56,7 +60,7 @@ This feature replaces that all-or-nothing posture with a per-user, gatekeeper-me `ObservationDescription.excludeObservers`. The overseer must then guarantee those observers never see it, or block the observation. -**The API is already committed** (commit `e2f1707`). The relevant interfaces are +**The API is already committed.** The relevant interfaces are `GatekeeperUser.getVerifier()`, `GatekeeperUserVerifier`, `Gatekeeper.addObserver()` / `removeObserver()`, and `ObservationDescription.excludeObservers`, all in `packages/workshop-shared/src/gatekeeper.ts`. @@ -68,8 +72,8 @@ This feature replaces that all-or-nothing posture with a per-user, gatekeeper-me - **Role-based breadth of verification:** - **`build`** collaborators (full access — chat + code + all bindings) must be verified against **every** gatekeeper the Gadget has. - - **`use`** collaborators (UI only, no chat access — see `UseOverseerInterface`, - `overseer.ts:2816`) must be verified only against gatekeepers their sessions can actually + - **`use`** collaborators (UI only, no chat access — see `UseOverseerInterface` in + `overseer.ts`) must be verified only against gatekeepers their sessions can actually reach: those **bound by some gadget** (the UI can invoke them), those with an **enabled hook** (a hook is a live write channel into a gadget they can open, delivering the connection's data regardless of binding edges), plus — transitively — every **env target of a @@ -94,20 +98,20 @@ This feature replaces that all-or-nothing posture with a per-user, gatekeeper-me | Concern | Location | |---|---| | Gatekeeper RPC API (the committed surface) | `packages/workshop-shared/src/gatekeeper.ts` | -| Overseer DO, `open()` auth entry point | `packages/workshop-backend/src/overseer.ts:2714` | +| Overseer DO, `open()` auth entry point | `packages/workshop-backend/src/overseer.ts` | | Authorization gate shared by `open()` and `receiveExternalMessage()` | `overseer.ts` `authorizeCollaborator()` | | Session restart when verification scope widens | `overseer.ts` (`#restartIfSessionsAffected`, `joinSession`, `scheduleAccessRestart`) | -| Server `openGadget` path | `packages/workshop-backend/src/server.ts:206` | -| Role resolution / permission graph | `packages/workshop-backend/src/sharing.ts` (`getEffectiveRole`, `computeEffectiveRoles`, `hasAnyShares`) | -| `containsRestrictedData` enforcement | `overseer.ts:1171` (`authorizeObservation`), `:1207` (web fetch), `:1258` (`submitAction`) | -| Observation recording | `overseer.ts:1169` `authorizeObservation()`; `ApprovalQueueImpl` `overseer.ts:4856` | -| Gatekeeper storage record | `overseer.ts:110` `GatekeeperRecord` (has `creationSpec.vendorId`) | -| `GatekeeperCreationSpec` | `packages/workshop-shared/src/api.ts:1345` | -| Gatekeeper facet access | `overseer.ts:1079` `getGatekeeperFacet()` | -| Overseer storage collections | `overseer.ts:316` (`gatekeepers`, with `byBindingName` index — template for a new collection) | -| Connected accounts (User DO) | `packages/workshop-backend/src/user.ts:12` `ConnectedAccountRecord` (`account: Fetcher`, `vendorId`) | -| List connected accounts | `user.ts:890` `subscribeConnectedAccounts()`; subscriber type `api.ts:116` | -| Account → gatekeeper class | `user.ts:1136` `getGatekeeperClassFor()` | +| Server `openGadget` path | `packages/workshop-backend/src/server.ts` | +| Role resolution / permission graph | `packages/workshop-backend/src/sharing.ts` (`getEffectiveRole`, `computeEffectiveRoles`) | +| `containsRestrictedData` enforcement | `overseer.ts` (`authorizeObservation`'s `#assertUnverifiableProducerUnshared`, `getWebFetchEnv`, `submitAction`) | +| Observation recording | `overseer.ts` `authorizeObservation()`; `ApprovalQueueImpl` | +| Gatekeeper storage record | `overseer.ts` `GatekeeperRecord` (has `creationSpec.vendorId`) | +| `GatekeeperCreationSpec` | `packages/workshop-shared/src/api.ts` | +| Gatekeeper facet access | `overseer.ts` `getGatekeeperFacet()` | +| Overseer storage collections | `overseer.ts` (`gatekeepers`, with `byBindingName` index — template for a new collection) | +| Connected accounts (User DO) | `packages/workshop-backend/src/user.ts` `ConnectedAccountRecord` (`account: Fetcher`, `vendorId`) | +| List connected accounts | `user.ts` `subscribeConnectedAccounts()`; subscriber type in `api.ts` | +| Account → gatekeeper class | `user.ts` `getGatekeeperClassFor()` | --- @@ -140,8 +144,8 @@ This feature replaces that all-or-nothing posture with a per-user, gatekeeper-me ### New overseer storage collection: `observers` -Add an `observers` collection to `OverseerStorage` (mirror the `gatekeepers` collection at -`overseer.ts:316`, including a secondary index for reverse lookup): +Add an `observers` collection to `OverseerStorage` (mirror the `gatekeepers` collection in +`overseer.ts`, including a secondary index for reverse lookup): ```ts type ObserverRecord = { @@ -173,7 +177,7 @@ log) lives inside each gatekeeper's own DO and is out of scope here. ### Step 1 — User DO: mint a verifier for a chosen account Add a method to the User DO (`packages/workshop-backend/src/user.ts`), near -`getGatekeeperClassFor` (`user.ts:1136`): +`getGatekeeperClassFor`: ```ts // Mint a verifier from one of THIS user's connected accounts, identified by accountId. @@ -203,7 +207,7 @@ invoked **only** when the opening user needs to configure gatekeeper accounts. I without an extra round trip. Add to the RPC API (`packages/workshop-shared/src/api.ts`) and thread through -`server.ts:206` → `overseer.open()` (`overseer.ts:2714`): +`server.ts` `openGadget` → `overseer.open()`: ```ts // Provided by the client when opening a gadget. Invoked by the overseer only if the opening @@ -233,11 +237,19 @@ type ObserverAccountChoice = { ### Step 3 — Overseer: observer configuration & re-verification at `open()` Hook into `open()` in the non-owner branch, after `effectiveRole` is confirmed and before -constructing the client interface. Keep the existing `containsRestrictedData` short-circuit ahead of -this -- lockdown still wins. The `NeedsConnections` signal is produced only *after* a valid role is +constructing the client interface. (Observer verification *is* the open()-time enforcement for +sensitive data; no `containsRestrictedData` check precedes it.) +The `NeedsConnections` signal is produced only *after* a valid role is confirmed, so it never reveals a workspace's gatekeeper or resource metadata to an unauthorized user. +Role resolution plus verification is one shared gate, `OverseerImpl.authorizeCollaborator`, and +every non-owner entry point that can surface workspace data runs it — `open()` interactively, and +`receiveExternalMessage()` non-interactively (no configuration channel, so an unverified caller is +told to open the workspace, which is where verification happens). An agent reply on the external +path can surface anything the workspace already read, so it must not admit a collaborator with +less verification than `open()` would demand. + Add a private helper on `OverseerImpl`, roughly: ```ts @@ -286,13 +298,14 @@ Logic: **throws** on a mismatch. This server-side check is what guarantees a gatekeeper only receives a verifier minted by its own vendor; filtering account choices in the client is only a user-interface convenience. - - If any `addObserver` **throws** (or `getVerifier` throws on vendor mismatch), the user is not - (or no longer) allowed. Every such failure goes through one `fail()` path, and the user is - offered a bounded number of re-prompts to repair (e.g. re-authenticate an expired account). On - terminal failure the open is denied with a message naming each refused binding, and the - registrations this call added are best-effort-removed while no record is persisted. The - persisted `accountChoices` are left as they are: an entry records the choice the user made so - they are not asked again, and asserts nothing about whether the gatekeeper still admits them. + - If any `addObserver` **throws** (or `getVerifier` throws on vendor mismatch, or returns null + for a disconnected account), the user is not (or no longer) allowed. Every such failure goes + through one `fail()` path, and the user is offered a bounded number of re-prompts to repair + (e.g. re-authenticate an expired account). On terminal failure the open is denied with a + message naming each refused binding, and the registrations this call added are + best-effort-removed while no record is persisted. The persisted `accountChoices` are left as + they are: an entry records the choice the user made so they are not asked again, and asserts + nothing about whether the gatekeeper still admits them. - Only a *first-ever* verification rolls anything back (fully — nothing referenced its registrations before the call, and the minted id would otherwise linger unresolvable). A returning observer's registrations are **all** kept, including ones this call added: their @@ -325,13 +338,6 @@ Notes: stored account choices. The modal is only for genuinely uncovered bindings (first open, a binding the owner added after this user last configured, or an ambient binding without a matching provided account). -- **Role resolution and verification belong together.** Both live behind one - `authorizeCollaborator(profileId, clientUser, {configureCb?, requireRole?})`, so every non-owner - entry point applies the same gate. `receiveExternalMessage()` — the chat-integration path, whose - agent reply can surface anything the workspace has already read — passes `requireRole: "build"` - and no `configureCb`: it has no channel to prompt on, so an unverified caller is told to open the - workspace in a browser, and an insufficient role is denied *before* verification runs rather than - being sent to fix a failure that could never grant them access anyway. #### Restarting when verification scope widens @@ -497,12 +503,16 @@ restart-check, and mark share one synchronous block, so no request can interleav change appearing and the block taking effect; marks are only ever set when a restart is scheduled, since nothing else would clear them. +Enforcement is therefore at admission, within the ~100 ms abort delay of the moment the change is +determined — the change itself, for every trigger — and held to the collaborator's role scope +(edge case 4 below). + ### Step 4 — Frontend: the configuration modal Implement the `ObserverConfigCallback` on the client. When the overseer calls `configure(needs)`: 1. For each `ObserverBindingNeed`, find the user's candidate accounts by filtering the existing - `subscribeConnectedAccounts()` results (`user.ts:890`) by `need.vendorId`. + `subscribeConnectedAccounts()` results by `need.vendorId`. 2. If one or more accounts match, pre-select one arbitrarily as the default; let the user change it via a dropdown. (Most users have one account per vendor and will just click "OK".) 3. Include forced auto-provisioned accounts in the subscription. If **no** account matches, use @@ -518,7 +528,7 @@ you're allowed to see the data it uses." ### Step 5 — Overseer: forward exclusion in `authorizeObservation()` -Extend `authorizeObservation()` (`overseer.ts:1169`) to honor `description.excludeObservers`. +Extend `authorizeObservation()` (in `overseer.ts`) to honor `description.excludeObservers`. Because v1 has no per-thread hiding, an excluded-but-named observation can only proceed when the named observer cannot reach it at all: either they have *already lost access* in the sharing graph, or the connection that produced it has left their role's verification scope. @@ -527,6 +537,12 @@ For each id in `description.excludeObservers`: 1. Map the opaque `observerId` → `profileId` via the `observers.byObserverId` index. If there is no record, the id is not an active observer → ignore it. + **Known gap: "no record" does not yet imply "not an active observer".** A first-time + `ensureObserver` registers a freshly minted id with the gatekeepers *before* the record (and + so `byObserverId`) is persisted, so an id named during that window — the fan-out, which can + park on the config modal — reads as unknown here and the observation is admitted to the very + collaborator it names. The required fix is an in-memory map of pending ids consulted here, + failing closed; it lands with `observer-verification-fixes`. 2. Check sharing-graph reachability for that `profileId` (`SharingManager.getEffectiveRole` / `computeEffectiveRoles`). - **Still authorized, and the producing gatekeeper is still in that role's scope → throw**, @@ -574,11 +590,14 @@ This is the runtime counterpart of `addObserver`: `addObserver` covers observers *after* data was read; `excludeObservers` covers data read *after* observers were configured. Persisting the observation record itself is unchanged; we only gate it. -> Why not also worry about authorized-but-not-yet-configured users here? They cannot be named in -> `excludeObservers` because no gatekeeper knows their id yet. The invariant still holds from the -> other direction: when such a user later opens and configures, `addObserver` re-checks them -> against *all* past observations (including any restricted one) and throws, denying them. So -> forward exclusion only needs to handle already-configured observers. +> Why not also worry about authorized-but-not-yet-configured users here? A user with no +> registration in flight cannot be named in `excludeObservers` because no gatekeeper knows their +> id yet. The invariant still holds from the other direction: when such a user later opens and +> configures, `addObserver` re-checks them against *all* past observations (including any +> restricted one) and throws, denying them. So forward exclusion only needs to handle +> already-configured observers — plus the one case in between: a user *mid*-registration is +> already known to every gatekeeper whose `addObserver` has returned, and can be named before +> their record exists. That is the Step 5 item 1 known gap above. ### Step 6 — Overseer: remove observers on sharing changes @@ -594,18 +613,17 @@ downgrades — see the matching methods on `OverseerClientInterface` and `Sharin stale cached workspace listing just yields a denied open). - After a mutation, use the returned `AffectedCollaborator[]` to find users who **lost access**. - For each who is now unreachable, if they have an observer record: best-effort - `removeObserver(record.observerId)` on **all** gatekeeper facets, then delete the observer - record. + For each who is now unreachable, if they have an observer record: delete the observer record, + then best-effort `removeObserver(record.observerId)` on **all** gatekeeper facets. - For a **`build` → `use` downgrade**, optionally `removeObserver` (and drop the corresponding `accountChoices` entries) for the now-out-of-scope bindings (those without a `bindingName`). Safe to defer — an over-broad observer set only ever errs toward stricter future checks — but it keeps gatekeeper state tidy. - All these calls are best-effort: log and continue on error. An orphaned observer entry only - causes superfluous future checks, never a data leak: a registration is what *admits* an open, and - every open re-runs `addObserver`, so a stale one grants nothing on its own — while + causes superfluous future checks, never a data leak: a registration is what *admits* an open, + and every open re-runs `addObserver`, so a stale one grants nothing on its own — while `authorizeObservation`'s exclusion gate re-checks the live sharing graph for any id a gatekeeper - still names. + still names. A record is only ever persisted for a party who passed full verification. > Multi-gatekeeper sequencing/atomicity is an overseer implementation detail, not part of the > shared interface. Because `addObserver` is re-run every open and `removeObserver` is idempotent, @@ -648,8 +666,50 @@ already in the JSDoc in `gatekeeper.ts`; add anything missing there rather than operational failure (vendor outage, expired credential) is treated the same way — the overseer cannot tell it from a settled denial — and the collaborator gets back in as soon as a repaired open re-verifies them. -4. **`containsRestrictedData` interaction** — unchanged and still authoritative: if set, no non-owner - can open at all (`overseer.ts:2770`). Observer checks only matter when sharing is allowed. +4. **`containsRestrictedData` interaction** — coverage is enforced at *admission*, not per + observation: `ensureObserver` re-verifies each collaborator against every in-scope gatekeeper + at every `open()`, so nobody can be in the workspace without having passed the producing + gatekeeper's `addObserver()`, and anything that widens what they must pass restarts every live + session (see "Restarting when verification scope widens"). The flag also latches the workspace + into a restricted mode that blocks actions and web fetches. + One producer admission structurally cannot cover: one with no vendor account behind it — an + `aiModel`/`agentSpawner` binding, or a legacy record with no `creationSpec`. + `#inScopeGatekeepers` skips those, so no collaborator is ever asked about them, and + `#assertUnverifiableProducerUnshared` in `authorizeObservation` therefore refuses their + restricted observations outright while the workspace is shared (any collaborator or + outstanding share link — the same test `removalBlockedByRestrictedData` applies; a link's key + never expires, so admitting the read would strand a link the owner has already handed out). + (This matches `assertNewSharingAllowed()`, which already treats the same case as unshareable, + and the message names no collaborator: it reaches sandboxed gadget code and agent output.) + Verification is also held to each collaborator's own role scope, because `ensureObserver` can + never verify beyond it: a `use` collaborator can't be covered for a gatekeeper outside their + scope (one no gadget binds and no enabled hook feeds — see `#useScopeGatekeeperIds`). + `use` scope is *live* binding state, with a transition case in each direction. Adding a + binding grows it, which restarts every live session (edge case 5). Unbinding shrinks it + silently: a formerly-bound producer drops out of `use` verification scope, so its sensitive + reads stop requiring `use` collaborators' coverage — the same skip as a never-bound producer, + though the liveness argument above doesn't apply to it. Accepted because (i) `use` sessions + cannot read chat history or the action log, so the exposure is limited to state the gadget + persisted, served through the gadget's own UI or export; (ii) that data entered gadget + storage while the producer *was* bound, when every `use` collaborator was verified against + it or could not open the workspace; (iii) the residual is `use` grants created after the + unbind, who view that persisted state unverified — and re-binding the connection restores their + verifiability at their next open. Coverage does not go stale across the unbind/rebind either: + a `use` collaborator who opened only during the unbound window verified nothing against the + producer and so holds no entry for it, and the rebind restarts the workspace, so their forced + re-open asks them about it; one who had verified before the unbind keeps their entry — it + records the account they chose, not an admission — and their re-open re-runs `addObserver` + against it. + The *never*-bound flavor of the same skip is broader: a producer reachable only through + chat bindings (an ambient singleton the agent reads in chat) was never in any `use` + collaborator's scope, so premise (ii) does not hold for it — the agent can persist its + restricted data into gadget code or storage without any `use` collaborator ever having + been verified against it, and there is no prior binding for "re-bind" to restore. + Accepted on the same grounds: coverage there is unverifiable by construction (the + liveness argument above), `use` sessions still cannot read chat history or the action + log, so the exposure is limited to what the agent chose to persist, and the forward + remedy is binding the producer to a gadget — that puts it in `use` scope, so every + collaborator is verified against it at their next open. 5. **Owner adds a new binding after sharing** — existing observers see an incremental modal for just the new binding on their next open, and may be denied if they lack access to the new resource (inherent to the security model). Because that next open is what verifies them, the @@ -672,6 +732,30 @@ already in the JSDoc in `gatekeeper.ts`; add anything missing there rather than (unbound, with no enabled hook keeping it reachable) is the different case Step 5's scope test handles: the gatekeeper still knows the id, but the observer can no longer reach what it produces, so they are de-registered from it instead of blocking. +8. **Removing a connection that read restricted data** — while the workspace is latched + (`containsRestrictedData`) *and* shared, `GatekeeperClient.remove()` refuses for the + *producer* connections — those through which restricted data was actually read, derived from + the permanent action log (`restrictedProducerIds`); non-producers stay removable. The record + is what observer verification runs against, and the restricted data outlives it in chat + history and storage, so deleting it would let a never-verified collaborator open unchecked. + Outstanding share links block removal the same way: their keys never expire, and redemption + is gated at open() only while the record exists. The remedy is to remove collaborators and + revoke share links first. + Unverifiable producers (a legacy record with no creation spec, or an aiModel/agentSpawner + backed by no vendor account) are guarded the same way: a legacy record denies every open whose + scope includes it (every `build` open — `observerVendorId` throws on it) while it exists, so + removing it while shared would readmit every existing collaborator with the restricted history + still in chat. It cannot be migrated (it never persisted the vendor identity), so the recovery + for an owner who wants to share such a workspace is to start a new one. Internal removals need + no guard: the creation-failure rollback removes a record too new to be a producer, and ambient + reconciliation skips — and logs — a stale record the guard protects. + The complementary rule (`assertNewSharingAllowed`): once latched, if any producer is gone or + can never verify a collaborator, everything that would admit a new party refuses — the + grant-creating mutators (`addCollaborator`, `createShareLink`, `newShareLinkKey`) and + `redeemShareKey` at open() — leaving the workspace permanently owner-only. Each check runs + synchronously with its storage write, so a producer removed in any await window still refuses + the grant; a redemption whose edge already exists skips the check, so an existing + collaborator's re-open is untouched. --- @@ -733,15 +817,17 @@ gatekeeper package — a single package (e.g. `gatekeeper-google`) may use sever its resource types. - **A — Private-only.** Non-owner observers are refused: `addObserver()` unconditionally throws. - This is the replacement for today's reliance on `containsRestrictedData` for these resources (the - `containsRestrictedData` lockdown mechanism itself is unchanged and remains available separately). + For data that must additionally never leak back out, the `containsRestrictedData` restricted + mode (no actions, no web fetches) is available separately; combined with strategy A it makes + the workspace effectively private once sensitive data is observed. `getVerifier()` must still exist (the overseer mints one on every open) but is never consulted. - **B — ACL check (single unit).** The resource is treated as one atomic unit. `getVerifier()` mints a verifier exposing the observer's vendor identity (via the - "non-standard method on the verifier" pattern, `gatekeeper.ts:456-461`). `addObserver()` resolves - that identity and checks it against the bound resource's ACL, throwing on failure. Gatekeepers - should cache per-open to bound cost (`gatekeeper.ts:511-516`). No `excludeObservers` is needed: + "non-standard method on the verifier" pattern; see `GatekeeperUserVerifier` in `gatekeeper.ts`). + `addObserver()` resolves that identity and checks it against the bound resource's ACL, throwing + on failure. Gatekeepers should cache per-open to bound cost (see the note on `addObserver()`). + No `excludeObservers` is needed: the whole unit is covered up front, so nothing read later could be invisible to a verified observer. @@ -750,8 +836,9 @@ its resource types. plus the set of current observers. `addObserver()` verifies the observer against **every** logged set so far. When a later observation first touches a **new** set, the gatekeeper re-verifies all current observers and sets `excludeObservers` for any who fail (the overseer then blocks the - observation per `gatekeeper.ts:751-774`). `removeObserver()` drops the observer from the tracked - set. Each per-set check reuses the same ACL primitive the corresponding narrow (B) binding uses. + observation per the `excludeObservers` contract). `removeObserver()` drops the observer from + the tracked set. Each per-set check reuses the same ACL primitive the corresponding narrow (B) + binding uses. - **D — Low-stakes.** No information-flow tracking. `addObserver()` / `removeObserver()` are no-ops; any collaborator may observe. `getVerifier()` returns a trivial verifier (the overseer @@ -779,8 +866,8 @@ its resource types. | **linear** | Workspace | **C** | Track accessed teams; verify the observer against each (reusing the Team B check). | | **notion** | Page / Database | **B** | Check the observer's Notion access to the bound page/database. | | **notion** | Workspace | **C** | Track accessed pages/databases; verify the observer's access to each. | -| **supabase** | Project | **B** | Verify the observer's own `listProjects()` (`supabase-api.ts:306`) includes the bound project ref. Within a project, arbitrary read-only SQL spans the whole DB, so the project is the atomic unit (no per-table tracking). | -| **supabase** | Organization | **C** | Track accessed project refs (the org session reaches them via `openProject` / `listProjects`, `supabase.ts:1015`/`:1037`); verify the observer's `listProjects()` includes each, reusing the Project B check. | +| **supabase** | Project | **B** | Verify the observer's own `listProjects()` (`supabase-api.ts`) includes the bound project ref. Within a project, arbitrary read-only SQL spans the whole DB, so the project is the atomic unit (no per-table tracking). | +| **supabase** | Organization | **C** | Track accessed project refs (the org session reaches them via `openProject` / `listProjects` in `supabase.ts`); verify the observer's `listProjects()` includes each, reusing the Project B check. | | **confluence** | Site | **C** | Verify site access; track observed spaces and content because both can have narrower permissions. | | **confluence** | Space | **C** | Verify space access; track observed pages and blog posts because content restrictions may be narrower. | | **confluence** | Page / Blog Post | **C** | Verify bound-content access; track observed child pages because they may have stricter restrictions than their parent. | @@ -812,3 +899,20 @@ This is why the broad bindings split the way they do: - **Decomposition deliberately deferred → A:** Gmail Mailbox — could in principle decompose into mailing lists the observer belongs to, but that is the out-of-scope "advanced" case, so it stays fully private for now. + +--- + +## Known limitations + +Revocations and role changes take effect within seconds -- the revocation restart lands in +~100ms -- and read-side races inside that envelope are accepted by design: a guard earns its +place here only if its failure mode is *persistent* wrong state that outlives the window. + +The observer-side deferrals are ledgered in `plans/restricted-data-sharing.md`, each marked at +its site in the code by a matching `TODO` comment: + +- Observer verification is not serialized per profile, so concurrent opens by one collaborator + can overwrite each other's records. +- A mid-registration observer named in `excludeObservers` is read as unknown and the observation + admitted; persistent, since it lands in chat history (Step 5, item 1). Fix: a pending-id map, + `observer-verification-fixes`. diff --git a/docs/sharing.md b/docs/sharing.md index a90d77b5e..abb8d783c 100644 --- a/docs/sharing.md +++ b/docs/sharing.md @@ -33,7 +33,7 @@ Authorization is capability-based: `open()` computes the caller's effective role There are two ways to grant someone collaborator access: -**Direct add.** The owner or an existing collaborator enters a username (email address) in the Share modal. The system looks up the corresponding user account; if it exists, a collaborator record is created. The target user does not receive an in-product notification -- the sharer is expected to send them a link or tell them out of band. +**Direct add.** The owner or an existing collaborator enters a username (an email address on OAuth/CF Access deployments; a normalized alphanumeric handle on password deployments) in the Share modal. The system looks up the corresponding user account; if it exists, a collaborator record is created. The target user does not receive an in-product notification -- the sharer is expected to send them a link or tell them out of band. **Share link.** Any collaborator (or the owner) can create a share link, which encodes a secret key in the URL as a `#share=` fragment. Anyone who opens this link is automatically added as a collaborator. A link is a durable handle that owns one or more keys: creating it mints its first key, and "copying" the link later mints another key for the same link. The raw key is shown to the creator only once at mint time and is never stored server-side, so re-copying can't reproduce an old key -- it mints a new one. Any of a link's keys can be redeemed by multiple people, or the same person multiple times, until the link is revoked, which invalidates every key minted for it. @@ -43,6 +43,8 @@ Storage shape: a link is its first key. The `shareKeys` table holds one row per Share key redemption and gadget opening happen atomically in a single RPC call (`openGadget(id, shareKey)`), which allows subsequent calls to be pipelined on the returned `Overseer` stub without waiting for a separate redemption step. +Redemption is **one-step**: redeeming a key writes the recipient's `shareKey` edge immediately, making them a collaborator like any other before the redeeming open()'s observer verification runs. Redemption is policy-gated like every grant-creating mutator (`assertNewSharingAllowed` runs synchronously with the write; a re-redemption whose edge already exists is a no-op that skips the gate). A recipient whose verification then fails keeps the edge -- see Known limitations. + ### Home page behavior A shared gadget does not appear on a collaborator's home page until they first open it. At that point, a record is created in the collaborator's user account (via `UserDurableObject.recordSharedGadgetOpen()`), storing a cached copy of the gadget's title and the owner's profile. The `lastActive` timestamp is updated each time they open the gadget. @@ -97,22 +99,20 @@ This does mean removed collaborators and revoked links accumulate in storage. Li ### Effective-role algorithm -The core is a **fixed-point role-propagation computation** implemented in `SharingManager.computeEffectiveRoles()`. It computes the effective role of every collaborator (given an optional hypothetical change), returning a map from profile ID to effective role (absence from the map means no access). It is the single source of truth: `open()`, `hasAnyShares()`, the listing RPCs, and the preview methods all derive from it. +The core is a **fixed-point role-propagation computation** implemented in `SharingManager.computeEffectiveRoles()`. It computes the effective role of every collaborator (given an optional hypothetical change), returning a map from profile ID to effective role (absence from the map means no access). It is the single source of truth: `open()`, the listing RPCs, and the preview methods all derive from it. -Inputs (all optional; used to model a hypothetical change in preview): +Inputs (all optional; used to model a hypothetical change): - `removedUser` -- a profile ID to treat as removed (excluded from the graph). - `removedEdge` -- a single user edge (`{target, sharer}`) to treat as removed. Used to preview a non-owner removing only their own edge. - `revokedLinkId` -- a share link ID to treat as revoked. -- `overrides` -- profile IDs pinned to at least a given role regardless of their edges. The algorithm: 1. **Build the candidate set.** Load all collaborators except the (hypothetically) removed user. 2. **Collect share-link metadata.** Build a map from link ID to `{creator, role}`, skipping links that are `revoked` (or the hypothetical `revokedLinkId`). -3. **Initialize** the role map with any `overrides`. -4. **Iterate to fixed point.** Repeatedly scan all collaborators. For each edge, compute the role it grants -- `min(edge role, sharer's effective role)`, where the sharer (or share link creator) is the owner (always `build`) or another collaborator's current effective role -- and raise the collaborator's role to the maximum across their valid edges. Raising one collaborator's role may unlock or raise others on the next pass. -5. **Converge.** Roles only ever increase, so the loop terminates when a full pass changes nothing. -6. **Return the role map.** Collaborators absent from the map have no access; collaborators present with a lower role than before have been downgraded. +3. **Iterate to fixed point.** Repeatedly scan all collaborators. For each edge, compute the role it grants -- `min(edge role, sharer's effective role)`, where the sharer (or share link creator) is the owner (always `build`) or another collaborator's current effective role -- and raise the collaborator's role to the maximum across their valid edges. Raising one collaborator's role may unlock or raise others on the next pass. +4. **Converge.** Roles only ever increase, so the loop terminates when a full pass changes nothing. +5. **Return the role map.** Collaborators absent from the map have no access; collaborators present with a lower role than before have been downgraded. This handles arbitrary graph shapes: diamonds (a user reachable via two independent paths), cycles (mutual adds), and deep chains. @@ -150,13 +150,17 @@ Authorization is enforced at `open()`: the method computes the caller's effectiv Because the role is recomputed from the graph on every `open()`, the live computation is the *sole* source of truth for access -- there is no eager cleanup whose bugs could grant access to an unreachable user. This is what makes lazy revocation safe: severing an edge is enough to deny access, even though the unreachable records linger in storage. +A share-key redemption goes through the same gate: the redemption is a grant like any other, policy-gated by `assertNewSharingAllowed` synchronously with the edge write. The redeeming open() then verifies the recipient as an observer like any other collaborator; a recipient whose verification fails persists as an unverified collaborator until removed (see Known limitations). + ### Terminating live sessions on revocation or scope growth Authorization is only checked at `open()`, so a session that is *already* open is not re-checked per message. Without intervention, a collaborator who was just removed or downgraded could keep using their live session until something else disconnected them. To close this gap, `removeCollaborator`/`revokeShareLink` proactively restart the gadget's Overseer DO via `ctx.abort()` whenever the change actually removed or downgraded someone (i.e. the returned `AffectedCollaborator[]` is non-empty; pure no-op removals don't restart). Aborting forcibly disconnects every client; each reconnects and re-runs `open()`, which re-evaluates the now-changed permission graph -- sending removed users to the terminal access-denied page and handing downgraded users their reduced capability (the editor swaps to the `use` view automatically based on `metadata.role`). Since removals are rare (and DOs restart unpredictably anyway, so reconnects are already cheap), the disruption is acceptable. -Two precautions surround the abort (`OverseerImpl.scheduleAccessRestart`): the severed edge is flushed with `ctx.storage.sync()` first (because `ctx.abort()` does not respect the output gate, a restart could otherwise come back with the change lost), and the abort is delayed ~100ms so the triggering RPC's response reaches the caller -- typically the owner, who is also connected -- before their own connection drops. The disconnect reaches the browser through the existing `notifyClosed` plumbing: when the Overseer DO aborts, the per-session `notifyClosed` stub is disposed without being called, which `AuthenticatedApiImpl` treats as a lost connection and reacts to by killing the browser WebSocket, forcing a reconnect. +Two precautions surround the abort (`OverseerImpl.scheduleAccessRestart`): the severed edge is flushed with `ctx.storage.sync()` first (because `ctx.abort()` does not respect the output gate, a restart could otherwise come back with the change lost), and the abort is delayed ~100ms so the triggering RPC's response reaches the caller -- typically the owner, who is also connected -- before their own connection drops. The disconnect reaches the browser through the existing `notifyClosed` plumbing: when the Overseer DO aborts, the per-session `notifyClosed` stub is disposed without being called, which `AuthenticatedApiImpl` treats as a lost connection and reacts to by killing the browser WebSocket, forcing a reconnect. The client discards its retained share key on the first successful open, so this forced reconnect after a removal is keyless and lands the removed collaborator on the access-denied page rather than silently re-redeeming the still-active link (which would undo the removal and break the assumption stated above). The residual is unchanged: the *link* itself survives a collaborator removal under the lazy model, so a recipient who kept the URL can still re-redeem it manually until the owner revokes it -- the discard removes only the client's automatic re-grant. -Granting or raising access never strands anyone: a live session's capability is fixed at open, so a `use` collaborator promoted to `build` in the graph still holds `UseOverseerInterface` until they re-open, and nobody is newly excluded from anything. `containsRestrictedData` cannot strand a session either: an observation that would set that flag is *blocked* (rather than applied) if the gadget is already shared, so the flag only ever flips to true on a gadget with no other sessions to evict. +The abort also lands later than the ~100ms delay alone suggests: the revocation handlers first await the observer teardown (`tearDownLostObservers`, a per-collaborator `removeObserver` fan-out) and the listing refresh (`refreshAffectedCollaboratorListings`, chunked cross-DO round trips), so the removed users' sessions stay live and watching for a window that scales with collaborator and gatekeeper count. + +Granting or raising access never strands anyone: a live session's capability is fixed at open, so a `use` collaborator promoted to `build` in the graph still holds `UseOverseerInterface` until they re-open, and nobody is newly excluded from anything. The same abort serves a second purpose, though, and there the trigger is a *grant*: observer verification (see docs/observers.md) also runs only at `open()`, so widening the set of gatekeepers a collaborator must be verified against leaves their live session holding access they were never verified for. `OverseerImpl.#restartIfSessionsAffected` restarts the workspace whenever that happens -- a connection is added, one is bound into a gadget, or a merge promotes such a binding -- so every client re-opens and re-runs `ensureObserver` at the new scope. It is a no-op unless a collaborator session of the affected role is live -- severing sessions is all a restart does -- so a solo workspace, or one whose collaborators are all disconnected, is never disturbed. See docs/observers.md, "Restarting when verification scope widens", for the full trigger list and the reasoning about what deliberately does *not* trigger it. @@ -169,3 +173,22 @@ The same abort serves a second purpose, though, and there the trigger is a *gran - **Un-revoking share links.** Revocation is non-destructive (the `revoked` flag), but there is no UI or RPC to list revoked links or clear the flag, so link revocation is currently one-way in practice. - **Garbage-collecting dead records.** Removed collaborators and revoked links accumulate in storage under the lazy model; a background sweep could reclaim entries that have been unreachable for a long time. - **Notifications.** Currently there are no in-product notifications for access grants or revocations. + +## Known limitations + +Revocations and role changes take effect within seconds -- the revocation restart lands in +~100ms -- and read-side races inside that envelope are accepted by design; only guards against +*persistent* wrong state remain. Each item below is marked at its site in the code by a matching +`TODO` comment. + +- **An unverified redeemer persists as a collaborator.** Redemption writes a real edge before the + redeeming open's observer verification runs, so from the moment a recipient clicks the link they + appear in `listCollaborators` whether or not they ever complete the open. They cannot reach the + workspace -- verification denies them at open -- but the workspace counts as *shared* for the + checks that ask only whether anyone else is on it: removing a restricted producer is blocked, and + an unverifiable producer's restricted reads are refused. Remedies: they verify (complete the + open), the owner removes them, or the link is revoked. Two-phase redemption (a pending edge + granting nothing until verification confirms it) is the planned fix. +- **A refused recipient persists.** A recipient whose verification is refused keeps their edge: + they appear in `listCollaborators` until the owner removes them (or revokes the link), with the + same consequences as the previous item, and covered by the same planned fix. diff --git a/plans/restricted-data-sharing.md b/plans/restricted-data-sharing.md new file mode 100644 index 000000000..80330e45d --- /dev/null +++ b/plans/restricted-data-sharing.md @@ -0,0 +1,302 @@ +# Plan: Govern sharing of restricted data by observer verification + +## Goal + +Replace the all-or-nothing sharing lockdown that a restricted-data observation imposes +with a per-collaborator check: a workspace that has read restricted data stays +shareable, and each collaborator is admitted only while they are verified as an +observer of the gatekeeper that produced the data. + +**Known security limitation:** that guarantee is currently scoped by collaborator role. +A `use` collaborator is verified only against gatekeepers in their scope — those bound to a +gadget or fed by an enabled hook (`#useScopeGatekeeperIds`). The workspace +agent can nevertheless read an unbound gatekeeper through a chat binding (including an +ambient singleton), persist its restricted result into gadget storage or UI state, and +thereby expose it to an unverified `use` collaborator. This plan accepts that risk for the +current implementation; "Never-bound producers" below records the exact boundary and the +required future remedies. + +Delivered as **one PR, split into reviewable commits** (see "Commit sequence" at the +end). The kernel packages (`workshop-backend`, `workshop-shared`) get the small, +separated diffs; the rename, the UI, and the frontend share-key work ride in their own +commits. + +## Locked decisions + +- **The flag is renamed, not aliased.** `ObservationDescription.prohibitAllSharing` + becomes `containsRestrictedData`, and `GadgetMetadata.sharingProhibited` becomes the + same name. The flag states a fact about the data ("this observation contains + restricted data"); what the platform does about that is policy and does not belong in + the name. A hard rename means every gatekeeper call site moves in the same commit — + TypeScript's excess-property check on the object literals passed to + `authorizeObservation` will not tolerate a staged one. +- **The durable storage key keeps its old name.** Typed-storage keys *are* property names, + so renaming the overseer's singleton would silently unlatch every workspace that has + already observed restricted data. The property is renamed anyway, and declares the old + key explicitly: `containsRestrictedData: singleton(false, {storageKey: + "prohibitAllSharing"})`. `storageKey` is a typed-storage schema option added for this, + so the exception lives in the schema rather than as a special case at each call site. +- **Persisted records are read through a legacy shim.** Old action-log entries still + carry `prohibitAllSharing` in their recorded `ObservationDescription`. + `observationContainsRestrictedData()` (with a local `LegacyObservationDescription` + type) reads either spelling. This is a read-side shim only — no producer may write the + old name. +- **Admission is per-collaborator, checked continuously.** Not at grant time: at every + `open()`, so revocation of a collaborator's underlying resource access is caught + promptly. Nobody is in the workspace without having passed the producer's + `addObserver()`, and anything that widens what they must pass restarts every live + session so it re-opens at the new scope (`#restartIfSessionsAffected`). + `authorizeObservation` itself only has to refuse the one producer admission cannot see: + an unverifiable one (`#assertUnverifiableProducerUnshared`). +- **Coverage is held to each collaborator's own role scope.** `ensureObserver` never + verifies a `use` collaborator against a gatekeeper outside their scope. This is a liveness + tradeoff, not a security guarantee: restricted data can flow from that gatekeeper + through the agent into gadget-visible state. The exception for an unbound producer and + a `use` collaborator is the known security risk stated above. +- **Share-key redemption stays one-step.** Redeeming a key writes a real edge + immediately, as on main, gated by `assertNewSharingAllowed` synchronously with the + write. The redeeming open then verifies the recipient like any other collaborator. + Two consequences are accepted on the ledger below: an unverified redeemer, and a + refused recipient, both persist in `listCollaborators` until removed, which is enough + to make the workspace count as shared. Two-phase redemption (a pending edge granting + nothing until verification confirms it) is the planned follow-up fix for both. +- **One authorization gate for every non-owner entry point.** `authorizeCollaborator` + resolves the effective role and runs `ensureObserver`. Both `open()` and + `receiveExternalMessage()` pass through it; the latter non-interactively, since there + is no way to configure connected accounts from an inbound message. +- **Removing the producing connection does not lift the restriction** for existing + collaborators. It does close the workspace to *new* grants + (`assertNewSharingAllowed`), since there is no longer an anchor to verify a newcomer + against. +- **Fail closed everywhere.** An operational failure — provider outage, expired + credential — is treated exactly like a refusal. + +## Current-state anchors (for orientation) + +- `authorizeObservation` (overseer.ts) is where a gatekeeper's observation is admitted + or refused, and where the durable restricted-mode flag latches. +- `ensureObserver` (overseer.ts) brings a non-owner into compliance for their role: + selects in-scope gatekeepers, prompts for unconfigured account choices via + `configureCb`, calls `addObserver` on each gatekeeper facet, and persists an + `ObserverRecord` only after all of them succeed. Re-runs on every open. Throws to deny. +- `SharingManager` (sharing.ts) owns the permission graph: collaborator records, their + `addedBy` edges, share links and keys, and `computeEffectiveRoles`' fixed-point + resolution. The module header states that sharing *policy* deliberately lives outside + it — this plan keeps that boundary by passing policy in as `assertGrantAllowed` + callbacks. +- `#inScopeGatekeepers(role)` derives what a collaborator must be verified against. + `use` scope is live gadget-binding state; `build` scope is broader. + +## Design + +### 1. Admission, and the residual guard (`#assertUnverifiableProducerUnshared`) + +Coverage is enforced by admission rather than per observation. `ensureObserver` verifies +each collaborator against every in-scope gatekeeper at every `open()`, and +`#restartIfSessionsAffected` aborts the DO whenever that scope widens (a connection added, +one bound into a gadget, a merge promoting such a binding, a hook enabled), so no live +session outlives the scope it was verified at. It is a no-op unless a collaborator session +of the widened role is live — severing sessions is all a restart does. + +What survives in `authorizeObservation` is the one producer admission structurally cannot +see: one with no vendor account behind it (`aiModel`/`agentSpawner`, or a legacy record +with no `creationSpec`). `#inScopeGatekeepers` skips those, so no collaborator is ever +asked about them, and its restricted observations are refused outright while the +workspace has any collaborator or outstanding share link — consistent with +`assertNewSharingAllowed`, which already treats the same case as unshareable. + +The error reaches sandboxed gadget code and agent output — an audience that cannot +otherwise enumerate collaborators — so it reports only that the workspace is shared, +naming neither the collaborators nor their profile ids (the full email on OAuth and CF +Access deployments). + +### 2. One-step share-key redemption (sharing.ts) + +`redeemShareKey` keeps main's shape: hash the key, resolve the link, and write a real +`shareKey` edge (creating the collaborator record if they're new), deduplicating against +an existing edge for the same link. The one delta vs main is the `assertGrantAllowed` +policy gate, invoked synchronously before the write and only when an edge is actually +added — a no-op re-redemption skips it, so an existing collaborator's re-open with a +retained key is untouched by a latched policy. + +The edge is real before the redeeming open's observer verification runs; the two +resulting windows (an unverified redeemer blocking restricted reads; a refused recipient +persisting until removed) are the accepted consequences on the ledger, marked by the +TODO at `redeemShareKey`. + +### 3. The unified gate (`authorizeCollaborator`) + +Resolves the effective role, denies below `requireRole` *before* verification runs, then +calls `ensureObserver`. This PR introduces the gate with both non-owner entry points as +callers: `open()` interactively and `receiveExternalMessage` non-interactively (the +latter previously checked only the role). + +Denying early matters: without it a `use` collaborator reaching `receiveExternalMessage` +would be verified (real `addObserver` calls, a persisted record) only to be turned away, +or worse, told to fix a verification failure that could never grant them access. + +Because redemption writes a real edge, a redeemer mid-verification is visible to the +revocation affected-set like any collaborator: a link revoked (or a removal landing) +while their open is parked triggers the revocation restart, which severs their session +and re-runs `open()` against the live graph. + +### 4. Policy hooks, not policy in `SharingManager` + +`addCollaborator`, `createShareLink`, `newShareLinkKey` and `redeemShareKey` all take an +optional `assertGrantAllowed` callback, invoked synchronously with the granting write. +The overseer passes `assertNewSharingAllowed`. A throw persists nothing. + +### 5. Observer records on a failed live check + +An earlier draft scrubbed the failed gatekeeper from the collaborator's persisted +`accountChoices` and restarted the workspace on the failure. The observer machinery +landed on main (#380) without either: an `accountChoices` entry records the account the +collaborator chose so they are not asked again, and asserts nothing about whether the +gatekeeper still admits them — every open re-runs `addObserver`, so a revoked +collaborator is denied at their next open regardless, and only that open is denied (the +lazy-revocation residual in `docs/observers.md` edge case 3). Nothing in this model reads +`accountChoices` to admit a restricted read, so the scrub is not a precondition of it. + +### 6. Frontend + +- **Share modal**: no longer replaces itself with a "can't be shared" view. Controls stay + live behind a notice. +- **Retained share keys** (`retainedShareKeys.ts`, new): the `#share=` fragment is + stripped from the URL on open, so a failed open had nothing to retry with. The key is + held in `sessionStorage` under a versioned, per-workspace key, and replayed on the next + attempt. (Note: under one-step redemption a failed open leaves a real edge, so the + retry is keyless — revisit this rationale when the follow-up branch rebases.) +- **Identity stamping**: because `sessionStorage` outlives the session that wrote it, each + entry records the capturing user's id. A read by a different identity ignores *and* + sweeps it, and `logout()` sweeps the whole prefix including malformed and older + unstamped entries. Without this, one user's pending share key could be auto-redeemed + under the next user's account in the same tab. +- **The in-memory tier is bound to its capturing stub**: it is replayed only on the same + `authenticatedApi` that captured it; any other stub falls through to the + identity-checked storage tier. This removes the reliance on the rendering invariant + that an identity change unmounts the editor -- true today, but enforced two files away. +- **Stamps are generation-gated**: the async identity stamp commits through a write token + taken at capture; clearing a workspace's entry (a successful open) or the logout sweep + voids every earlier token, so a stamp resolving late cannot resurrect a cleared key. + The invalidation lives in `retainedShareKeys.ts` because the storage outlives any one + attempt -- a per-attempt flag guards only its own attempt's writes. +- **A superseded open bails after its identity await**, before creating any capability: + its cleanup already ran with nothing to dispose, so proceeding would mint a stub + nothing can reach and publish a stale (or wrong-workspace) capability. + +## Commit sequence (one PR) + +Ordered so the kernel-critical diffs are isolated. Every commit type-checks green across +`workshop-shared`, `workshop-backend`, `workshop-frontend` and `gatekeeper-google`. + +This PR is built directly on main and carries the model change alone. The observer +machinery it builds on has a set of preexisting concurrency races (and this PR's own +model adds atomicity hardening on top); those fixes are deferred to follow-up work. +Each deferred fix is acknowledged at its site with a `TODO` comment; the docs collect +the same items in their Known-limitations sections. The observer-side groundwork this +plan once carried — the unified `authorizeCollaborator` gate on the external-message +path, and the scope-widening restart — landed separately in #380. + +1. **Refactor — the rename.** Mechanical, no behavior change, spanning + `workshop-shared`, `workshop-backend`, `workshop-frontend`, `gatekeeper-google`, + `gatekeeper-mcp` and the gatekeeper-authoring skill doc. Atomic by necessity. +2. **Part 1 — API.** The restated contract on `containsRestrictedData`. Server still + implements the old behavior. +3. **Part 2 — core server implementation.** The restricted-observation guard, the + redemption policy gate, `restrictedProducerIds`/`assertNewSharingAllowed`, the + producer-removal guard, the legacy flag shim, and removal of `hasAnyShares`. Places + the TODO ledger for the deferred fixes. +4. **Part 3 — backend tests.** +5. **Part 4 — integration tests.** Over real Durable Objects, through the test + gatekeeper fixture's `readValue(restricted)` and its controllable verification + outcome. +6. **Part 5 — documentation.** `docs/observers.md` coverage rules and residuals; + `docs/sharing.md` one-step redemption and the policy hooks; this plan. + +The deferred items are collected in the Known-limitations section below. The Share +modal unblock and the retained-share-key frontend work live in +`restricted-data-followups`. + +## Known limitations + +Revocations and role changes take effect within seconds (the revocation restart lands in +~100ms), and read-side races inside that envelope are accepted by design: a deferred fix +stays on this ledger only if its failure mode is *persistent* wrong state that outlives +the window. Each item is marked in the code by a matching `TODO` comment; this ledger is +the follow-up worklist. + +- Observer verification is not serialized per profile, so concurrent opens by one + collaborator can overwrite each other's records and registrations. +- A mid-registration observer named in `excludeObservers` is read as unknown and the + observation admitted: a first-time `ensureObserver` registers the id with the + gatekeepers before the record is persisted, so `#enforceExcludeObservers` cannot map it + back during that window. Persistent (the observation lands in chat history). Fix: an + in-memory pending-id map consulted there, failing closed — `observer-verification-fixes`. +- An unverified redeemer persists as a collaborator: redemption writes a real edge + before the redeeming open's verification runs, so from click onward the recipient is + visible in `listCollaborators` whether or not they ever complete the open, which is + enough to make the workspace count as shared (remedies: verify, remove, or revoke the + link). Two-phase redemption is the planned fix. +- A refused recipient persists: a recipient whose verification is refused keeps their + edge, with the same consequence as the previous item, and the same planned fix. +- A failed re-verification denies only the open being attempted. Sessions the + collaborator already holds keep the access their own opens verified until they next + re-open — the lazy-revocation residual `docs/observers.md` edge case 3 already accepts, + so it grants no access they do not already hold. + +## Known edge cases / watch-fors + +- **A producer removed mid-redemption cannot slip a grant through.** `remove()` refuses + every restricted producer (unverifiable ones included) while any share link is + outstanding, and the redemption policy gate runs synchronously with the edge write. +- **Operational failures deny like refusals.** An outage or expired credential denies the + collaborator's open exactly as a revocation does (the overseer cannot tell them apart); + they get back in as soon as a repaired open re-verifies them. Fail-closed by design. +- **Role increases do not ride out on a redeeming open.** An owner grant landing while + verification waited takes effect at the recipient's next open, exactly as for an + ordinary keyless open. +- **Removing an unverifiable restricted producer — implemented: guarded like any other.** + `remove()`'s producer guard used to exempt unverifiable records ("removing one is + itself a remedy"), which was backwards once the data had been read: the record is the + *blocker* -- `#inScopeGatekeepers` throws on it, so no collaborator can open -- and + removing it let every existing collaborator open unverified while the restricted data + persists in chat history, gadget storage and code (`assertNewSharingAllowed` only stops + *new* grants). Decided and implemented: fail closed -- unverifiable producers are + guarded like any other (the owner must remove all collaborators and revoke all share + links first), after which the workspace is permanently owner-only + (`restrictedProducerIds()` reads the action log, which never forgets the producer). + Deliberately no migration or reconnect flow: an automatic migration is impossible + (legacy records never persisted `vendorId`, and the class stub is opaque), and an + owner-driven reconnect flow was considered and rejected as scope. The documented + recovery for an owner who wants to share such a workspace is to start a new workspace. + +## Accepted tradeoffs / future work + +- **Formerly-bound producers.** Unbinding shrinks `use` scope with no guard, so a + formerly-bound producer's sensitive reads stop requiring `use` collaborators' + coverage. Accepted because `use` sessions cannot read chat history or the action log; + the data entered gadget storage while the producer *was* bound, when every `use` + collaborator was verified against it or could not open the workspace; and re-binding + restores verifiability at the next open. The residual is `use` grants created after the + unbind. Note that (i) does not cover a binding loopback retained across the unbind, which + returns the read directly as an RPC result and stays callable until `removeGatekeeper`; + that is the `docs/observers.md` Step 5 known gap, with `#assertBindingEdgeLive` as the + named fix. +- **Known security risk — never-bound producers.** A producer reachable only through chat + bindings (including an ambient singleton) is never in a `use` collaborator's verification + scope. The agent can read restricted data from it, persist the result into gadget code, + storage, or UI state, and the collaborator can then read that state through the deployed + gadget despite never passing the producer's `addObserver()` check. Role-scoped + verification deliberately never asks this collaborator about that producer, so + `containsRestrictedData` does not prevent this disclosure. Binding the producer makes + future opens verifiable but does not retract data already exposed. Accepted temporarily + to avoid making the read permanently unavailable + under the current role-scoped model. The required fix is either workspace-wide observer + verification for `use` collaborators or enforceable provenance that prevents data from an + unverified producer reaching their gadget-visible state. Both this and the formerly-bound + residual are documented at `docs/observers.md` edge case 4. +- **`calculate()`-style aggregates are out of scope here.** This plan governs *who* may + see restricted data, not what an aggregate over it discloses. +- **Verification remains interactive-only.** `receiveExternalMessage` can verify but + cannot configure, so a caller with unconfigured account choices is told to open the + workspace. A non-interactive configuration path is future work. From 546bc3ea0d7dd2be87cacd3d32a482fce1e11212 Mon Sep 17 00:00:00 2001 From: Maximo Guk <62088388+Maximo-Guk@users.noreply.github.com> Date: Thu, 10 Sep 2026 20:51:20 -0500 Subject: [PATCH 6/7] Restricted data: gate addCollaborator inside the sharing manager, skipping no-op re-grants. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/observers.md | 5 +- .../__tests__/sharing.test.ts | 62 +++++++++++++++++++ packages/workshop-backend/src/overseer.ts | 10 +-- packages/workshop-backend/src/sharing.ts | 25 +++++--- plans/restricted-data-sharing.md | 5 +- 5 files changed, 93 insertions(+), 14 deletions(-) diff --git a/docs/observers.md b/docs/observers.md index c1cea0a84..2a5baaf58 100644 --- a/docs/observers.md +++ b/docs/observers.md @@ -754,8 +754,9 @@ already in the JSDoc in `gatekeeper.ts`; add anything missing there rather than grant-creating mutators (`addCollaborator`, `createShareLink`, `newShareLinkKey`) and `redeemShareKey` at open() — leaving the workspace permanently owner-only. Each check runs synchronously with its storage write, so a producer removed in any await window still refuses - the grant; a redemption whose edge already exists skips the check, so an existing - collaborator's re-open is untouched. + the grant; a redemption whose edge already exists, or an `addCollaborator` that creates no + new edge and raises no role, skips the check, so an existing collaborator's re-open or + re-grant is untouched. --- diff --git a/packages/workshop-backend/__tests__/sharing.test.ts b/packages/workshop-backend/__tests__/sharing.test.ts index a55b9f782..a8d28a591 100644 --- a/packages/workshop-backend/__tests__/sharing.test.ts +++ b/packages/workshop-backend/__tests__/sharing.test.ts @@ -270,6 +270,68 @@ describe("addCollaborator", () => { mgr.addCollaborator({ caller: owner, profile: profile("a"), role: "use" }); expect(mgr.getEffectiveRole("a")).toBe("build"); }); + + it("does not invoke assertGrantAllowed for a same-or-lower re-grant", () => { + let { storage, mgr } = makeManager(); + seedCollaborator(storage, "a", [userEdge(OWNER, "build")]); + let closed = () => { throw new Error("sharing is closed"); }; + + // Same role: an existing grant, not a new one, so only the note changes even when policy + // forbids new sharing. + expect(() => mgr.addCollaborator({ + caller: owner, profile: profile("a"), role: "build", note: "updated", + assertGrantAllowed: closed, + })).not.toThrow(); + let record = storage.collaborators.get("a")!; + expect(record.addedBy).toHaveLength(1); + expect(record.addedBy[0]).toEqual(expect.objectContaining({ role: "build", note: "updated" })); + expect(mgr.getEffectiveRole("a")).toBe("build"); + + // Lower role: never downgrades, and creates no grant either. + expect(() => mgr.addCollaborator({ + caller: owner, profile: profile("a"), role: "use", assertGrantAllowed: closed, + })).not.toThrow(); + expect(storage.collaborators.get("a")!.addedBy).toHaveLength(1); + expect(mgr.getEffectiveRole("a")).toBe("build"); + }); + + it("invokes assertGrantAllowed for a new collaborator, a new edge, and a role rise, and a throw persists nothing", () => { + let { storage, mgr } = makeManager(); + let closed = () => { throw new Error("sharing is closed"); }; + + // New collaborator: no record written. + expect(() => mgr.addCollaborator({ + caller: owner, profile: profile("a"), role: "build", assertGrantAllowed: closed, + })).toThrow(/sharing is closed/); + expect(storage.collaborators.get("a")).toBeUndefined(); + + // New edge from a different sharer onto an existing collaborator: addedBy unchanged. + seedCollaborator(storage, "b", [userEdge(OWNER, "build")]); + seedCollaborator(storage, "a", [userEdge("b", "use")]); + expect(() => mgr.addCollaborator({ + caller: owner, profile: profile("a"), role: "build", assertGrantAllowed: closed, + })).toThrow(/sharing is closed/); + expect(storage.collaborators.get("a")!.addedBy).toHaveLength(1); + expect(mgr.getEffectiveRole("a")).toBe("use"); + + // Role rise on the existing same-sharer edge: role unchanged. + expect(() => mgr.addCollaborator({ + caller: collab("b"), profile: profile("a"), role: "build", assertGrantAllowed: closed, + })).toThrow(/sharing is closed/); + expect(storage.collaborators.get("a")!.addedBy[0]).toEqual(expect.objectContaining({ + type: "user", sharer: "b", role: "use", + })); + expect(mgr.getEffectiveRole("a")).toBe("use"); + + // A passing check is invoked exactly once and the grant is written. + let calls = 0; + mgr.addCollaborator({ + caller: owner, profile: profile("a"), role: "build", assertGrantAllowed: () => { calls++; }, + }); + expect(calls).toBe(1); + expect(storage.collaborators.get("a")!.addedBy).toHaveLength(2); + expect(mgr.getEffectiveRole("a")).toBe("build"); + }); }); describe("computeEffectiveRoles", () => { diff --git a/packages/workshop-backend/src/overseer.ts b/packages/workshop-backend/src/overseer.ts index 914a67086..04a509403 100644 --- a/packages/workshop-backend/src/overseer.ts +++ b/packages/workshop-backend/src/overseer.ts @@ -12168,15 +12168,17 @@ class OverseerClientInterface extends RpcTarget implements Overseer { } let sharing = await this.impl.getSharingManager(); - // Asserted in the same synchronous block as the grant's storage write (after every await): a - // check ahead of the awaits above could pass, a concurrent producer-connection removal land - // during the yield, and the grant still be written past it. - this.impl.assertNewSharingAllowed(); return sharing.addCollaborator({ caller: this.#sharingCaller(), profile, role, note, + // Run by the manager in the same synchronous block as the grant's storage write (after + // every await): a check ahead of the awaits above could pass, a concurrent + // producer-connection removal land during the yield, and the grant still be written past + // it. The manager also skips it when no new grant is created (a same-or-lower re-grant + // over an existing edge), matching redeemShareKey's already-existing-edge case. + assertGrantAllowed: () => this.impl.assertNewSharingAllowed(), }); } diff --git a/packages/workshop-backend/src/sharing.ts b/packages/workshop-backend/src/sharing.ts index 88039e236..fc847465f 100644 --- a/packages/workshop-backend/src/sharing.ts +++ b/packages/workshop-backend/src/sharing.ts @@ -40,10 +40,6 @@ function edgeGrantedRole(edge: PermissionEdge): CollaboratorRole { return edge.role ?? "build"; } -function maxRole(a: CollaboratorRole, b: CollaboratorRole): CollaboratorRole { - return roleRank(a) >= roleRank(b) ? a : b; -} - function minRole(a: CollaboratorRole, b: CollaboratorRole): CollaboratorRole { return roleRank(a) <= roleRank(b) ? a : b; } @@ -285,14 +281,22 @@ export class SharingManager { /** * Add a collaborator with a `user` edge from the caller, granting `role`. The caller is - * responsible for resolving `profile` (via RPC) and for any policy checks. The caller may not - * grant a role higher than their own effective role. + * responsible for resolving `profile` (via RPC) and supplies the policy hook; the manager + * decides whether the call actually creates a grant (a new record, a new edge from this + * sharer, or a role rise on the existing edge) and invokes the hook only then, so a + * same-or-lower re-grant (which at most updates the edge's note) is never refused by policy. + * The caller may not grant a role higher than their own effective role. */ addCollaborator(opts: { caller: SharingCaller; profile: AiChatAuthorInfo; role: CollaboratorRole; note?: string; + /** + * See createShareLink: run synchronously with the put, a throw persists nothing. Skipped when + * no new grant is created (same-or-lower re-grant over an existing edge from this sharer). + */ + assertGrantAllowed?: () => void; }): CollaboratorInfo { // Don't add the owner as a collaborator. if (opts.profile.id === this.ownerProfileId) { @@ -319,9 +323,15 @@ export class SharingManager { let existingEdge = existing.addedBy.find( e => e.type === "user" && e.sharer === opts.caller.profileId); if (existingEdge && existingEdge.type === "user") { - existingEdge.role = maxRole(edgeGrantedRole(existingEdge), opts.role); + // A role rise widens the grant; a same-or-lower role leaves it untouched (only the note + // may change), so no policy check. + if (roleRank(opts.role) > roleRank(edgeGrantedRole(existingEdge))) { + opts.assertGrantAllowed?.(); + existingEdge.role = opts.role; + } if (opts.note !== undefined) existingEdge.note = opts.note; } else { + opts.assertGrantAllowed?.(); existing.addedBy.push(edge); } this.storage.collaborators.put(existing); @@ -336,6 +346,7 @@ export class SharingManager { profile: opts.profile, addedBy: [edge], }; + opts.assertGrantAllowed?.(); this.storage.collaborators.put(record); return { profile: record.profile, diff --git a/plans/restricted-data-sharing.md b/plans/restricted-data-sharing.md index 80330e45d..b5a6d8ebf 100644 --- a/plans/restricted-data-sharing.md +++ b/plans/restricted-data-sharing.md @@ -144,7 +144,10 @@ and re-runs `open()` against the live graph. `addCollaborator`, `createShareLink`, `newShareLinkKey` and `redeemShareKey` all take an optional `assertGrantAllowed` callback, invoked synchronously with the granting write. -The overseer passes `assertNewSharingAllowed`. A throw persists nothing. +The overseer passes `assertNewSharingAllowed`. A throw persists nothing. The manager invokes +the hook only when a grant is actually created (a new record, a new edge, or a role rise on +an existing edge); a same-or-lower `addCollaborator` re-grant or a redemption whose edge +already exists skips it. ### 5. Observer records on a failed live check From cd8c45d1557c1a0d6828464a858486fc19cff40f Mon Sep 17 00:00:00 2001 From: Maximo Guk <62088388+Maximo-Guk@users.noreply.github.com> Date: Fri, 11 Sep 2026 09:22:20 -0500 Subject: [PATCH 7/7] Restricted data part 6: Drop the producer guards. 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 --- docs/observers.md | 74 +--- docs/sharing.md | 12 +- .../__tests__/sensitive-observations.test.ts | 158 -------- .../gatekeeper-test/src/test-gatekeeper.ts | 6 +- .../__tests__/restricted-data.test.ts | 39 -- .../restricted-observation-latch.test.ts | 185 +-------- .../restricted-producer-removal.test.ts | 367 ------------------ .../__tests__/sharing.test.ts | 145 ------- packages/workshop-backend/src/overseer.ts | 238 +----------- packages/workshop-backend/src/sharing.ts | 69 +--- packages/workshop-shared/src/api.ts | 7 +- packages/workshop-shared/src/gatekeeper.ts | 15 +- plans/restricted-data-sharing.md | 109 ++---- 13 files changed, 99 insertions(+), 1325 deletions(-) delete mode 100644 packages/workshop-backend/__tests__/restricted-data.test.ts delete mode 100644 packages/workshop-backend/__tests__/restricted-producer-removal.test.ts diff --git a/docs/observers.md b/docs/observers.md index 2a5baaf58..7b7a32be7 100644 --- a/docs/observers.md +++ b/docs/observers.md @@ -36,10 +36,7 @@ observation marked **`containsRestrictedData`** (`ObservationDescription.containsRestrictedData` in `packages/workshop-shared/src/gatekeeper.ts`) latches the workspace into a restricted mode — no actions, no web fetches. Its coverage rests on admission: nobody can open the workspace without being verified against the producing gatekeeper, -and anything that widens what they must be verified against restarts every live session. The one -producer admission cannot see — one with no vendor account behind it — is refused outright while -the workspace is shared; see `#assertUnverifiableProducerUnshared` in `overseer.ts` and edge case -4 below.) +and anything that widens what they must be verified against restarts every live session.) The check works as follows: @@ -103,7 +100,7 @@ The check works as follows: | Session restart when verification scope widens | `overseer.ts` (`#restartIfSessionsAffected`, `joinSession`, `scheduleAccessRestart`) | | Server `openGadget` path | `packages/workshop-backend/src/server.ts` | | Role resolution / permission graph | `packages/workshop-backend/src/sharing.ts` (`getEffectiveRole`, `computeEffectiveRoles`) | -| `containsRestrictedData` enforcement | `overseer.ts` (`authorizeObservation`'s `#assertUnverifiableProducerUnshared`, `getWebFetchEnv`, `submitAction`) | +| `containsRestrictedData` enforcement | `overseer.ts` (`authorizeObservation`'s latch, `getWebFetchEnv`, `submitAction`) | | Observation recording | `overseer.ts` `authorizeObservation()`; `ApprovalQueueImpl` | | Gatekeeper storage record | `overseer.ts` `GatekeeperRecord` (has `creationSpec.vendorId`) | | `GatekeeperCreationSpec` | `packages/workshop-shared/src/api.ts` | @@ -672,39 +669,18 @@ already in the JSDoc in `gatekeeper.ts`; add anything missing there rather than gatekeeper's `addObserver()`, and anything that widens what they must pass restarts every live session (see "Restarting when verification scope widens"). The flag also latches the workspace into a restricted mode that blocks actions and web fetches. - One producer admission structurally cannot cover: one with no vendor account behind it — an - `aiModel`/`agentSpawner` binding, or a legacy record with no `creationSpec`. - `#inScopeGatekeepers` skips those, so no collaborator is ever asked about them, and - `#assertUnverifiableProducerUnshared` in `authorizeObservation` therefore refuses their - restricted observations outright while the workspace is shared (any collaborator or - outstanding share link — the same test `removalBlockedByRestrictedData` applies; a link's key - never expires, so admitting the read would strand a link the owner has already handed out). - (This matches `assertNewSharingAllowed()`, which already treats the same case as unshareable, - and the message names no collaborator: it reaches sandboxed gadget code and agent output.) - Verification is also held to each collaborator's own role scope, because `ensureObserver` can + Verification is held to each collaborator's own role scope, because `ensureObserver` can never verify beyond it: a `use` collaborator can't be covered for a gatekeeper outside their scope (one no gadget binds and no enabled hook feeds — see `#useScopeGatekeeperIds`). - `use` scope is *live* binding state, with a transition case in each direction. Adding a - binding grows it, which restarts every live session (edge case 5). Unbinding shrinks it - silently: a formerly-bound producer drops out of `use` verification scope, so its sensitive - reads stop requiring `use` collaborators' coverage — the same skip as a never-bound producer, - though the liveness argument above doesn't apply to it. Accepted because (i) `use` sessions - cannot read chat history or the action log, so the exposure is limited to state the gadget - persisted, served through the gadget's own UI or export; (ii) that data entered gadget - storage while the producer *was* bound, when every `use` collaborator was verified against - it or could not open the workspace; (iii) the residual is `use` grants created after the - unbind, who view that persisted state unverified — and re-binding the connection restores their - verifiability at their next open. Coverage does not go stale across the unbind/rebind either: - a `use` collaborator who opened only during the unbound window verified nothing against the - producer and so holds no entry for it, and the rebind restarts the workspace, so their forced - re-open asks them about it; one who had verified before the unbind keeps their entry — it - records the account they chose, not an admission — and their re-open re-runs `addObserver` - against it. + Unbinding shrinks `use` scope silently, so a formerly-bound producer's later restricted reads + are not covered for `use` collaborators added after the unbind. Accepted because `use` + sessions cannot read chat history or the action log, and a rebind restores coverage at the + next open. The *never*-bound flavor of the same skip is broader: a producer reachable only through chat bindings (an ambient singleton the agent reads in chat) was never in any `use` - collaborator's scope, so premise (ii) does not hold for it — the agent can persist its - restricted data into gadget code or storage without any `use` collaborator ever having - been verified against it, and there is no prior binding for "re-bind" to restore. + collaborator's scope — the agent can persist its restricted data into gadget code or storage + without any `use` collaborator ever having been verified against it, and there is no prior + binding for "re-bind" to restore. Accepted on the same grounds: coverage there is unverifiable by construction (the liveness argument above), `use` sessions still cannot read chat history or the action log, so the exposure is limited to what the agent chose to persist, and the forward @@ -732,31 +708,11 @@ already in the JSDoc in `gatekeeper.ts`; add anything missing there rather than (unbound, with no enabled hook keeping it reachable) is the different case Step 5's scope test handles: the gatekeeper still knows the id, but the observer can no longer reach what it produces, so they are de-registered from it instead of blocking. -8. **Removing a connection that read restricted data** — while the workspace is latched - (`containsRestrictedData`) *and* shared, `GatekeeperClient.remove()` refuses for the - *producer* connections — those through which restricted data was actually read, derived from - the permanent action log (`restrictedProducerIds`); non-producers stay removable. The record - is what observer verification runs against, and the restricted data outlives it in chat - history and storage, so deleting it would let a never-verified collaborator open unchecked. - Outstanding share links block removal the same way: their keys never expire, and redemption - is gated at open() only while the record exists. The remedy is to remove collaborators and - revoke share links first. - Unverifiable producers (a legacy record with no creation spec, or an aiModel/agentSpawner - backed by no vendor account) are guarded the same way: a legacy record denies every open whose - scope includes it (every `build` open — `observerVendorId` throws on it) while it exists, so - removing it while shared would readmit every existing collaborator with the restricted history - still in chat. It cannot be migrated (it never persisted the vendor identity), so the recovery - for an owner who wants to share such a workspace is to start a new one. Internal removals need - no guard: the creation-failure rollback removes a record too new to be a producer, and ambient - reconciliation skips — and logs — a stale record the guard protects. - The complementary rule (`assertNewSharingAllowed`): once latched, if any producer is gone or - can never verify a collaborator, everything that would admit a new party refuses — the - grant-creating mutators (`addCollaborator`, `createShareLink`, `newShareLinkKey`) and - `redeemShareKey` at open() — leaving the workspace permanently owner-only. Each check runs - synchronously with its storage write, so a producer removed in any await window still refuses - the grant; a redemption whose edge already exists, or an `addCollaborator` that creates no - new edge and raises no role, skips the check, so an existing collaborator's re-open or - re-grant is untouched. +8. **Removing a connection that read restricted data** — removal is not guarded by the + restricted-data latch. The record is what observer verification runs against, so removing a + producer drops the check for data that outlives it in chat history and storage. The intended + remedy is that a future connection-removal UI asks the owner to certify that no sensitive data + from that connection has been retained in the workspace, for any connection. --- diff --git a/docs/sharing.md b/docs/sharing.md index abb8d783c..e86e39e78 100644 --- a/docs/sharing.md +++ b/docs/sharing.md @@ -43,7 +43,7 @@ Storage shape: a link is its first key. The `shareKeys` table holds one row per Share key redemption and gadget opening happen atomically in a single RPC call (`openGadget(id, shareKey)`), which allows subsequent calls to be pipelined on the returned `Overseer` stub without waiting for a separate redemption step. -Redemption is **one-step**: redeeming a key writes the recipient's `shareKey` edge immediately, making them a collaborator like any other before the redeeming open()'s observer verification runs. Redemption is policy-gated like every grant-creating mutator (`assertNewSharingAllowed` runs synchronously with the write; a re-redemption whose edge already exists is a no-op that skips the gate). A recipient whose verification then fails keeps the edge -- see Known limitations. +Redemption is **one-step**: redeeming a key writes the recipient's `shareKey` edge immediately, making them a collaborator like any other before the redeeming open()'s observer verification runs. A recipient whose verification then fails keeps the edge -- see Known limitations. ### Home page behavior @@ -150,7 +150,7 @@ Authorization is enforced at `open()`: the method computes the caller's effectiv Because the role is recomputed from the graph on every `open()`, the live computation is the *sole* source of truth for access -- there is no eager cleanup whose bugs could grant access to an unreachable user. This is what makes lazy revocation safe: severing an edge is enough to deny access, even though the unreachable records linger in storage. -A share-key redemption goes through the same gate: the redemption is a grant like any other, policy-gated by `assertNewSharingAllowed` synchronously with the edge write. The redeeming open() then verifies the recipient as an observer like any other collaborator; a recipient whose verification fails persists as an unverified collaborator until removed (see Known limitations). +A share-key redemption goes through the same gate. The redeeming open() then verifies the recipient as an observer like any other collaborator; a recipient whose verification fails persists as an unverified collaborator until removed (see Known limitations). ### Terminating live sessions on revocation or scope growth @@ -184,11 +184,9 @@ Revocations and role changes take effect within seconds -- the revocation restar - **An unverified redeemer persists as a collaborator.** Redemption writes a real edge before the redeeming open's observer verification runs, so from the moment a recipient clicks the link they appear in `listCollaborators` whether or not they ever complete the open. They cannot reach the - workspace -- verification denies them at open -- but the workspace counts as *shared* for the - checks that ask only whether anyone else is on it: removing a restricted producer is blocked, and - an unverifiable producer's restricted reads are refused. Remedies: they verify (complete the - open), the owner removes them, or the link is revoked. Two-phase redemption (a pending edge - granting nothing until verification confirms it) is the planned fix. + workspace -- verification denies them at open. Remedies: they verify (complete the open), the + owner removes them, or the link is revoked. Two-phase redemption (a pending edge granting + nothing until verification confirms it) is the planned fix. - **A refused recipient persists.** A recipient whose verification is refused keeps their edge: they appear in `listCollaborators` until the owner removes them (or revokes the link), with the same consequences as the previous item, and covered by the same planned fix. diff --git a/packages/integration-tests/__tests__/sensitive-observations.test.ts b/packages/integration-tests/__tests__/sensitive-observations.test.ts index 8dbba0d24..6a4dda76b 100644 --- a/packages/integration-tests/__tests__/sensitive-observations.test.ts +++ b/packages/integration-tests/__tests__/sensitive-observations.test.ts @@ -484,94 +484,6 @@ describe("sensitive observations", () => { }); }); - it.concurrent("a latched connection cannot be removed while the workspace is shared", - async () => { - await withSession(async publicApi => { - // Latched but unshared: removal proceeds. (The latch itself persists; there is nobody - // whose verification the record anchors.) - const solo = await newWorkspace(publicApi, "remove-solo"); - await expect(solo.session.readValue(true)).resolves.toBe(42); - const soloGatekeeper = await solo.overseer.getGatekeeperById(solo.gatekeeperId); - await expect(soloGatekeeper.remove()).resolves.toBeUndefined(); - - // Latched and shared: the record is what Bob's verification runs against, so removing it - // would let him open unchecked while the restricted data persists. - const ws = await newWorkspace(publicApi, "remove-shared"); - await expect(ws.session.readValue(true)).resolves.toBe(42); - await addBob(publicApi, ws); - const gatekeeper = await ws.overseer.getGatekeeperById(ws.gatekeeperId); - await expect(gatekeeper.remove()).rejects.toThrow(/remove all collaborators/i); - // The refused removal left the connection intact. - await expect(ws.session.readValue()).resolves.toBe(42); - }); - }); - - it.concurrent("a latched connection cannot be removed while a share link is outstanding", - async () => { - await withSession(async publicApi => { - // An unredeemed link creates no collaborator state, but its keys are multi-redeemable and - // never expire: redemption is gated at open() only while the gatekeeper record exists, so - // removing the record now would let a later recipient open unchecked. - const ws = await newWorkspace(publicApi, "remove-linked"); - await expect(ws.session.readValue(true)).resolves.toBe(42); - const { linkId } = await ws.overseer.createShareLink("build", "outstanding"); - - const gatekeeper = await ws.overseer.getGatekeeperById(ws.gatekeeperId); - await expect(gatekeeper.remove()).rejects.toThrow(/revoke all share links/i); - // The refused removal left the connection intact. - await expect(ws.session.readValue()).resolves.toBe(42); - - // Nobody redeemed the link, so revoking it affects no collaborator (no revocation restart) - // and unblocks the removal. - await expect(ws.overseer.revokeShareLink(linkId, [])).resolves.toEqual([]); - await expect(gatekeeper.remove()).resolves.toBeUndefined(); - }); - }); - - it.concurrent("the removal guard is scoped to the connection that read the sensitive data", - async () => { - await withSession(async publicApi => { - const ws = await newWorkspace(publicApi, "scoped-producer"); - // A second connection that never reads anything sensitive. - const accounts = await listConnectedAccounts(ws.aliceApi); - const account = accounts.find(a => a.vendorId === TEST_VENDOR_ID)!; - const bystander = await ws.overseer.newGatekeeper(account.id, thingUrl("scoped-bystander")); - if (!bystander) throw new Error("Failed to create the second test connection"); - - // Only the first connection reads restricted data; share after the latch. - await expect(ws.session.readValue(true)).resolves.toBe(42); - await addBob(publicApi, ws); - - // The latch is workspace-wide, but only the producer anchors verification: the bystander - // stays removable while shared, the producer does not. - await expect(bystander.remove()).resolves.toBeUndefined(); - const producer = await ws.overseer.getGatekeeperById(ws.gatekeeperId); - await expect(producer.remove()).rejects.toThrow(/remove all collaborators/i); - await expect(ws.session.readValue()).resolves.toBe(42); - }); - }); - - it.concurrent("a workspace whose sensitive-data producer was removed can no longer be shared", - async () => { - await withSession(async publicApi => { - const ws = await newWorkspace(publicApi, "unshareable"); - await expect(ws.session.readValue(true)).resolves.toBe(42); - - // Unshared, so removal is allowed -- but the restricted data (and the latch) outlive it. - const gatekeeper = await ws.overseer.getGatekeeperById(ws.gatekeeperId); - await expect(gatekeeper.remove()).resolves.toBeUndefined(); - - // With the producer's record gone there is nothing to verify a new collaborator against, - // so the grant-creating mutators refuse. - const [carol] = nextUsernames("carol"); - await signUp(publicApi, carol); - await expect(ws.overseer.addCollaborator(carol, "build")) - .rejects.toThrow(/can no longer be shared/i); - await expect(ws.overseer.createShareLink("use", "too late")) - .rejects.toThrow(/can no longer be shared/i); - }); - }); - it.concurrent("removal restarts the workspace and tears down the observer record", async () => { await withSession(async publicApi => { const ws = await newWorkspace(publicApi, "removal"); @@ -603,74 +515,4 @@ describe("sensitive observations", () => { } }); }); - - it.concurrent("ambient reconciliation preserves a shared restricted producer", async () => { - await withSession(async publicApi => { - const ws = await newWorkspace(publicApi, "ambient-reconcile"); - - // Ambient capsule records aren't published to clients, but their workpiece ids are small - // sequential integers, so probe for the one ensureAmbientCapsules provisioned at first open. - const findAmbientIds = async (overseer: RpcStub) => { - const found: number[] = []; - for (let id = 0; id < ws.gatekeeperId + 4; id++) { - try { - const gatekeeper = await overseer.getGatekeeperById(id); - if ((await gatekeeper.getTitle()) === "Test Ambient") found.push(id); - } catch { - // Not a gatekeeper workpiece. - } - } - return found; - }; - const [ambientId] = await findAmbientIds(ws.overseer); - expect(ambientId).toBeDefined(); - - // Latch through the ambient capsule, so it -- not the pasted connection -- is the producer. - const ambient = await ws.overseer.getGatekeeperById(ambientId); - const ambientSession = await ambient.openSession() as RpcStub; - await expect(ambientSession.readValue(true)).resolves.toBe(42); - const bob = await addBob(publicApi, ws); - - // Bob verifies (against the capsule too) and stays connected, held open until the restart - // below has been observed: the reconcile runs in the background after the owner's open - // returns, so closing him any earlier could leave it nothing to sever. - const bobSession = await bobHolds(ws, bob); - try { - // Replace the owner's singleton account: disconnecting and re-provisioning mints a new - // accountId, so the existing capsule record is stale at the next reconcile. - const accounts = await listConnectedAccounts(ws.aliceApi); - const oldAccount = accounts.find(a => a.vendorId === TEST_VENDOR_ID)!; - await ws.aliceApi.disconnectAccount(oldAccount.id); - const newAccount = await provisionAccount(ws.aliceApi); - expect(newAccount.id).not.toBe(oldAccount.id); - - // Reopen. Later opens run the capsule reconcile in the background, and provisioning the - // replacement is itself a connection Bob has never been verified against -- so the - // reconcile restarts the workspace out from under this connection (possibly severing this - // very open, which is fine: the reconcile has run either way). Come back on a fresh one, - // then wait for the replacement's record to appear (proof the reconcile has run). - await ws.aliceApi.openGadget(ws.gadgetId).then( - overseer => overseer[Symbol.dispose](), () => {}); - const reopened = await reopenAfterRestart(ws); - try { - const ids = await waitFor("the replacement ambient capsule to be provisioned", - async () => { - const found = await findAmbientIds(reopened.overseer); - return found.some(id => id !== ambientId) ? found : null; - }); - - // The stale record anchors Bob's verification, so the reconcile must have skipped it: - // the record survives, and sharing -- which refuses once any producer's record is gone - // -- still works. - expect(ids).toContain(ambientId); - await expect(reopened.overseer.createShareLink("build", "still shareable")) - .resolves.toMatchObject({ key: expect.any(String) }); - } finally { - reopened.publicApi[Symbol.dispose](); - } - } finally { - bobSession.close(); - } - }); - }); }); diff --git a/packages/integration-tests/fixtures/gatekeeper-test/src/test-gatekeeper.ts b/packages/integration-tests/fixtures/gatekeeper-test/src/test-gatekeeper.ts index a8d5dbafd..6709a2b16 100644 --- a/packages/integration-tests/fixtures/gatekeeper-test/src/test-gatekeeper.ts +++ b/packages/integration-tests/fixtures/gatekeeper-test/src/test-gatekeeper.ts @@ -302,11 +302,7 @@ export class TestVerifier // Gatekeeper (one per bound resource, running as a facet under the gadget's Overseer) export interface TestSession { - /** - * Records an observation through the same `ApprovalQueue` funnel a shipping gatekeeper uses. - * `restricted` marks it `containsRestrictedData`, to trip the restricted-mode latch and the - * unverifiable-producer guard. - */ + /** `restricted` marks the observation `containsRestrictedData`. */ readValue(restricted?: boolean): Promise; writeValue(value: number): Promise; writeValues(values: number[]): Promise; diff --git a/packages/workshop-backend/__tests__/restricted-data.test.ts b/packages/workshop-backend/__tests__/restricted-data.test.ts deleted file mode 100644 index 233761fb3..000000000 --- a/packages/workshop-backend/__tests__/restricted-data.test.ts +++ /dev/null @@ -1,39 +0,0 @@ -// The restricted-data flag on persisted observation records must be readable under both its -// current name and its pre-rename one: records written before the rename carry -// `prohibitAllSharing`, are never rewritten, and anchor the producer-scoped removal guard and -// assertNewSharingAllowed -- a legacy record read as unflagged would let a legacy-latched -// workspace share past a removed producer. - -import { describe, it, expect } from "vitest"; -import { observationContainsRestrictedData } from "../src/overseer.js"; -import type { ObservationDescription } from "@gadgets/workshop-shared/gatekeeper"; - -function description(flags: Record): ObservationDescription { - return { title: "t", description: "d", ...flags } as ObservationDescription; -} - -describe("observationContainsRestrictedData", () => { - it("reads the current field name", () => { - expect(observationContainsRestrictedData(description({ containsRestrictedData: true }))) - .toBe(true); - }); - - it("reads the pre-rename field name on legacy records", () => { - expect(observationContainsRestrictedData(description({ prohibitAllSharing: true }))) - .toBe(true); - }); - - it("is false when neither name is present", () => { - expect(observationContainsRestrictedData(description({}))).toBe(false); - }); - - it("is true when both names are present", () => { - expect(observationContainsRestrictedData( - description({ containsRestrictedData: true, prohibitAllSharing: true }))).toBe(true); - }); - - it("is false for an explicit legacy false", () => { - expect(observationContainsRestrictedData(description({ prohibitAllSharing: false }))) - .toBe(false); - }); -}); diff --git a/packages/workshop-backend/__tests__/restricted-observation-latch.test.ts b/packages/workshop-backend/__tests__/restricted-observation-latch.test.ts index e5ce5ca36..ea84de7a6 100644 --- a/packages/workshop-backend/__tests__/restricted-observation-latch.test.ts +++ b/packages/workshop-backend/__tests__/restricted-observation-latch.test.ts @@ -1,19 +1,10 @@ -// authorizeObservation's restricted-data gates. The exclusion gate is decided before anything -// else: the restricted-mode latch is one-way, so an observation the exclusion blocks must leave -// no trace -- no latch, no record, sharing untouched. The decisions the delivery rests on (the -// removed-connection refusal, the unverifiable-producer refusal, the latch, the record) all run -// *after* the exclusion teardown's awaited cross-worker fan-out, in one synchronous block, so a -// removal landing mid-teardown refuses the observation rather than slipping past a pre-latched -// producer. And a restricted observation arriving through an already-removed connection (an -// in-flight facet RPC can outlive removeGatekeeper) is refused rather than latched: with zero -// collaborators nothing else would stop it, and latching a missing producer id would permanently -// brick sharing via assertNewSharingAllowed's missing-record branch. +// authorizeObservation's restricted-data latch is one-way and is set only once the observation is +// actually delivered. The exclusion gate is decided first, across an awaited cross-worker fan-out, +// so an observation the exclusion blocks must leave no trace -- no latch, no record -- and one it +// admits latches and records in the same synchronous block after the teardown completes. // -// The last case covers the one producer admission cannot enforce: a connection with no vendor -// account behind it is in nobody's verification scope, so no collaborator is ever asked about it. -// -// Runs against a real OverseerDurableObject (the TEST_OVERSEER binding, like -// restricted-producer-removal.test.ts); the gatekeeper facet is the only fake. +// Runs against a real OverseerDurableObject (the TEST_OVERSEER binding); the gatekeeper facet is +// the only fake. import { describe, expect, it } from "vitest"; import { env } from "cloudflare:workers"; @@ -49,12 +40,6 @@ function seedGatekeeper(impl: any, id: number): void { id, resourceTitle: `Connection ${id}`, class: {} as any, - creationSpec: { - type: "gatekeeper", - vendorId: "testvendor", - resourceUrl: `https://example.com/${id}`, - typeUrlPattern: "https://*", - }, }); } @@ -65,17 +50,12 @@ const RESTRICTED_EXCLUDING_MALLORY = { excludeObservers: ["obs-m"], }; -describe("authorizeObservation's restricted-data gates", () => { +describe("authorizeObservation's restricted-data latch", () => { it("latches and records only after the exclusion teardown admits the observation", async () => { let stub = env.TEST_OVERSEER.getByName("restricted-latch-teardown-window"); await runInDurableObject(stub, async (instance: OverseerDurableObject) => { let impl = getImpl(instance); seedGatekeeper(impl, 1); - // An outstanding share link keeps the workspace "shared" for - // removalBlockedByRestrictedData. - impl.storage.shareKeys.put({ - id: "link-1", created: new Date(), createdBy: OWNER, role: "build", - }); // Mallory holds an observer record but no reachable role: the named exclusion admits the // observation and schedules her teardown. impl.storage.observers.put( @@ -99,10 +79,8 @@ describe("authorizeObservation's restricted-data gates", () => { held.resolve(); await expect(observation).resolves.toBeUndefined(); - // Delivery: the latch and the record landed together, and everything keyed on the latch - // now holds. + // Delivery: the latch and the record landed together. expect(impl.storage.containsRestrictedData.get()).toBe(true); - expect(impl.removalBlockedByRestrictedData(1, await impl.getSharingManager())).toBe(true); // The teardown still ran (mallory is no longer set up to observe). expect(impl.storage.observers.get("mallory")).toBeUndefined(); @@ -117,8 +95,8 @@ describe("authorizeObservation's restricted-data gates", () => { await runInDurableObject(stub, async (instance: OverseerDurableObject) => { let impl = getImpl(instance); seedGatekeeper(impl, 1); - // Mallory is a current collaborator, verified against the producer: nothing else stands in - // this observation's way, so the exclusion gate is the only thing blocking it. + // Mallory is a current collaborator: the exclusion gate is the only thing blocking this + // observation. impl.storage.collaborators.put({ profile: { id: "mallory", name: "Mallory" }, addedBy: [{ type: "user", sharer: OWNER, created: new Date(), role: "build" }], @@ -131,151 +109,10 @@ describe("authorizeObservation's restricted-data gates", () => { .rejects.toThrow(/not permitted to see/); // The blocked observation delivered no data, so the workspace is not restricted: no latch, - // sharing still grantable, no action record -- and mallory, still authorized, was not torn - // down. + // no action record -- and mallory, still authorized, was not torn down. expect(impl.storage.containsRestrictedData.get()).toBe(false); - expect(() => impl.assertNewSharingAllowed()).not.toThrow(); expect([...impl.storage.actions.list()]).toHaveLength(0); expect(impl.storage.observers.get("mallory")).toBeDefined(); }); }); - - it("refuses the observation when the connection is removed mid-teardown", async () => { - let stub = env.TEST_OVERSEER.getByName("restricted-latch-removed-mid-teardown"); - await runInDurableObject(stub, async (instance: OverseerDurableObject) => { - let impl = getImpl(instance); - seedGatekeeper(impl, 1); - impl.storage.observers.put( - { profileId: "mallory", observerId: "obs-m", accountChoices: { 1: 10 } }); - - let held = deferred(); - impl.getGatekeeperFacet = () => ({ - removeObserver: async () => { await held.promise; }, - }); - - let observation = impl.authorizeObservation( - 1, RESTRICTED_EXCLUDING_MALLORY, { from: "user" }); - await tick(); - - // The latch isn't set during the teardown, so removalBlockedByRestrictedData doesn't - // protect the producer in this window; the connection is removed out from under the - // in-flight observation. - impl.storage.gatekeepers.delete(1); - - held.resolve(); - // The post-teardown re-read catches the removal: refused, and nothing latched or recorded. - await expect(observation).rejects.toThrow(/has been removed/); - expect(impl.storage.containsRestrictedData.get()).toBe(false); - expect([...impl.storage.actions.list()]).toHaveLength(0); - }); - }); - - it("refuses restricted data through a removed connection instead of bricking sharing", async () => { - let stub = env.TEST_OVERSEER.getByName("restricted-latch-missing-producer"); - await runInDurableObject(stub, async (instance: OverseerDurableObject) => { - let impl = getImpl(instance); - // No gatekeeper record: the in-flight facet RPC outlived removeGatekeeper. With zero - // collaborators nothing else refuses it, so only this guard stands between the observation - // and latching a missing producer id. - await expect(impl.authorizeObservation(1, { - title: "Read a thing", - description: "The test read a thing.", - containsRestrictedData: true, - }, { from: "user" })).rejects.toThrow(/has been removed/); - - // A blocked observation delivered no data: the workspace must not be left restricted -- - // and above all must not be left permanently unshareable by latching a missing producer. - expect(impl.storage.containsRestrictedData.get()).toBe(false); - expect(() => impl.assertNewSharingAllowed()).not.toThrow(); - }); - }); - - it("refuses an unverifiable producer's restricted data on a shared workspace", async () => { - let stub = env.TEST_OVERSEER.getByName("restricted-latch-unverifiable-shared"); - await runInDurableObject(stub, async (instance: OverseerDurableObject) => { - let impl = getImpl(instance); - // An AI model binding has no vendor account behind it, so #inScopeGatekeepers skips it and - // no collaborator is ever asked to verify against it -- the one producer admission cannot - // enforce, and the only one this check still refuses. - impl.storage.gatekeepers.put({ - id: 1, - resourceTitle: "Claude", - class: {} as any, - creationSpec: { - type: "aiModel", modelId: "m", provider: "anthropic", modelName: "claude", - }, - }); - impl.storage.collaborators.put({ - profile: { id: "mallory", name: "Mallory" }, - addedBy: [{ type: "user", sharer: OWNER, created: new Date(), role: "build" }], - }); - - await expect(impl.authorizeObservation(1, { - title: "Read a thing", - description: "The test read a thing.", - containsRestrictedData: true, - }, { from: "user" })).rejects.toThrow(/cannot verify anyone's access/); - - // Refused, so nothing latched: the owner can still unshare and read it. - expect(impl.storage.containsRestrictedData.get()).toBe(false); - expect([...impl.storage.actions.list()]).toHaveLength(0); - }); - }); - - it("refuses an unverifiable producer's restricted data while a share link is outstanding", async () => { - let stub = env.TEST_OVERSEER.getByName("restricted-latch-unverifiable-link-only"); - await runInDurableObject(stub, async (instance: OverseerDurableObject) => { - let impl = getImpl(instance); - impl.storage.gatekeepers.put({ - id: 1, - resourceTitle: "Claude", - class: {} as any, - creationSpec: { - type: "aiModel", modelId: "m", provider: "anthropic", modelName: "claude", - }, - }); - // No collaborators, but a link the owner has already handed out. Its key never expires and - // can be redeemed any number of times; had this read latched, assertNewSharingAllowed would - // refuse every redemption, stranding the link. Same predicate as - // removalBlockedByRestrictedData. - impl.storage.shareKeys.put({ - id: "link-1", created: new Date(), createdBy: OWNER, role: "build", - }); - - await expect(impl.authorizeObservation(1, { - title: "Read a thing", - description: "The test read a thing.", - containsRestrictedData: true, - }, { from: "user" })).rejects.toThrow(/cannot verify anyone's access/); - - expect(impl.storage.containsRestrictedData.get()).toBe(false); - expect([...impl.storage.actions.list()]).toHaveLength(0); - }); - }); - - it("admits an unverifiable producer's restricted data on a solo workspace", async () => { - let stub = env.TEST_OVERSEER.getByName("restricted-latch-unverifiable-solo"); - await runInDurableObject(stub, async (instance: OverseerDurableObject) => { - let impl = getImpl(instance); - impl.storage.gatekeepers.put({ - id: 1, - resourceTitle: "Claude", - class: {} as any, - creationSpec: { - type: "aiModel", modelId: "m", provider: "anthropic", modelName: "claude", - }, - }); - - // Nobody to under-verify: the owner reads their own data, and the latch is what keeps it - // that way. - await expect(impl.authorizeObservation(1, { - title: "Read a thing", - description: "The test read a thing.", - containsRestrictedData: true, - }, { from: "user" })).resolves.toBeUndefined(); - - expect(impl.storage.containsRestrictedData.get()).toBe(true); - expect(() => impl.assertNewSharingAllowed()).toThrow(); - }); - }); }); diff --git a/packages/workshop-backend/__tests__/restricted-producer-removal.test.ts b/packages/workshop-backend/__tests__/restricted-producer-removal.test.ts deleted file mode 100644 index 8127507b6..000000000 --- a/packages/workshop-backend/__tests__/restricted-producer-removal.test.ts +++ /dev/null @@ -1,367 +0,0 @@ -// removalBlockedByRestrictedData() is the single predicate behind the producer-removal guard: -// GatekeeperClientImpl.remove() refuses on it, and ensureAmbientCapsules()'s reconciliation skips -// stale records on it. It must block exactly when deleting the record would readmit an unverified -// party -- the workspace is latched, the record is a restricted producer (verifiable or not), and -// the sharing graph still has collaborators or outstanding share links. -// -// Runs against a real OverseerDurableObject (the TEST_OVERSEER binding, like -// git-migration-do.test.ts) so the predicate reads real storage; records are seeded directly -// through the impl. - -import { describe, expect, it } from "vitest"; -import { env } from "cloudflare:workers"; -import { runInDurableObject } from "cloudflare:test"; -import type { OverseerDurableObject } from "../src/overseer.js"; - -declare module "cloudflare:workers" { - interface ProvidedEnv { - TEST_OVERSEER: DurableObjectNamespace; - } -} - -const OWNER = "alice"; - -function getImpl(instance: OverseerDurableObject): any { - let impl = (instance as unknown as { impl: any }).impl; - // The sharing manager resolves collaborator reachability from the owner; seed the cached - // profile id so no User DO round trip is attempted. - impl.ownerProfileId = OWNER; - return impl; -} - -// A verifiable connection record, or (without `creationSpec`) a legacy one -- unverifiable, and -// guarded all the same. -function seedGatekeeper(impl: any, id: number, creationSpec = true): void { - impl.storage.gatekeepers.put({ - id, - resourceTitle: `Connection ${id}`, - class: {} as any, - ...(creationSpec ? { - creationSpec: { - type: "gatekeeper", - vendorId: "testvendor", - resourceUrl: `https://example.com/${id}`, - typeUrlPattern: "https://*", - }, - } : {}), - }); -} - -// A restricted observation attributed to `gatekeeperId`, which is what makes it a producer -// (restrictedProducerIds scans the action log for exactly these). -function seedRestrictedObservation(impl: any, gatekeeperId: number, actionId: number): void { - impl.storage.actions.put({ - id: actionId, - gatekeeperId, - caller: { from: "user" }, - createdAt: new Date(), - state: "approved", - type: "observation", - description: { - title: "Read a thing", - description: "The test read a thing.", - containsRestrictedData: true, - }, - }); -} - -function seedCollaborator(impl: any): void { - impl.storage.collaborators.put({ - profile: { id: "bob", name: "Bob" }, - addedBy: [{ type: "user", sharer: OWNER, created: new Date(), role: "build" }], - }); -} - -function seedShareLink(impl: any): void { - impl.storage.shareKeys.put({ - id: "link-1", - created: new Date(), - createdBy: OWNER, - role: "build", - }); -} - -describe("removalBlockedByRestrictedData", () => { - it("does not block while the workspace is unlatched", async () => { - let stub = env.TEST_OVERSEER.getByName("producer-removal-unlatched"); - await runInDurableObject(stub, async (instance: OverseerDurableObject) => { - let impl = getImpl(instance); - seedGatekeeper(impl, 1); - seedRestrictedObservation(impl, 1, 100); - seedCollaborator(impl); - - expect(impl.removalBlockedByRestrictedData(1, await impl.getSharingManager())).toBe(false); - }); - }); - - it("does not block a latched non-producer, even while shared", async () => { - let stub = env.TEST_OVERSEER.getByName("producer-removal-non-producer"); - await runInDurableObject(stub, async (instance: OverseerDurableObject) => { - let impl = getImpl(instance); - seedGatekeeper(impl, 1); - seedGatekeeper(impl, 2); - seedRestrictedObservation(impl, 1, 100); - impl.storage.containsRestrictedData.put(true); - seedCollaborator(impl); - - let sharing = await impl.getSharingManager(); - expect(impl.removalBlockedByRestrictedData(2, sharing)).toBe(false); - expect(impl.removalBlockedByRestrictedData(1, sharing)).toBe(true); - }); - }); - - it("blocks a legacy (unverifiable) producer while a collaborator exists", async () => { - let stub = env.TEST_OVERSEER.getByName("producer-removal-legacy"); - await runInDurableObject(stub, async (instance: OverseerDurableObject) => { - let impl = getImpl(instance); - seedGatekeeper(impl, 1, /* creationSpec */ false); - seedRestrictedObservation(impl, 1, 100); - impl.storage.containsRestrictedData.put(true); - seedCollaborator(impl); - - // The legacy record is what denies every non-owner open (#inScopeGatekeepers throws on - // it), so removing it while shared would readmit the collaborator unverified. - expect(impl.removalBlockedByRestrictedData(1, await impl.getSharingManager())).toBe(true); - }); - }); - - it("blocks on an outstanding share link alone", async () => { - let stub = env.TEST_OVERSEER.getByName("producer-removal-link-only"); - await runInDurableObject(stub, async (instance: OverseerDurableObject) => { - let impl = getImpl(instance); - seedGatekeeper(impl, 1); - seedRestrictedObservation(impl, 1, 100); - impl.storage.containsRestrictedData.put(true); - seedShareLink(impl); - - // No collaborator yet, but the link's keys are multi-redeemable and redemption is gated - // only while the record exists. - expect(impl.removalBlockedByRestrictedData(1, await impl.getSharingManager())).toBe(true); - }); - }); - - it("does not block a latched producer while the workspace is unshared", async () => { - let stub = env.TEST_OVERSEER.getByName("producer-removal-unshared"); - await runInDurableObject(stub, async (instance: OverseerDurableObject) => { - let impl = getImpl(instance); - seedGatekeeper(impl, 1); - seedRestrictedObservation(impl, 1, 100); - impl.storage.containsRestrictedData.put(true); - - expect(impl.removalBlockedByRestrictedData(1, await impl.getSharingManager())).toBe(false); - }); - }); - - it("falls back to guarding every connection when the latch is set with no producer", async () => { - let stub = env.TEST_OVERSEER.getByName("producer-removal-empty-producers"); - await runInDurableObject(stub, async (instance: OverseerDurableObject) => { - let impl = getImpl(instance); - seedGatekeeper(impl, 1); - // Should be impossible (the latch and its action record are written together), so fail - // closed: with the latch set and no derivable producer set, everything is guarded. - impl.storage.containsRestrictedData.put(true); - seedCollaborator(impl); - - expect(impl.removalBlockedByRestrictedData(1, await impl.getSharingManager())).toBe(true); - }); - }); -}); - -// GatekeeperClientImpl.remove() must make its decision against sharing state read *after* its -// only real yield (the cold sharing manager's whoami RPC) and in the same synchronous block as -// the delete: a grant landing during the yield is seen by the check, and nothing can land -// between the check and the delete. Pinned by parking whoami on a deferred so the yield is a -// real in-test suspension point. -describe("GatekeeperClientImpl.remove ordering", () => { - it("sees a collaborator granted while the sharing manager is being fetched", async () => { - let stub = env.TEST_OVERSEER.getByName("producer-removal-mid-yield-grant"); - await runInDurableObject(stub, async (instance: OverseerDurableObject) => { - // Not getImpl(): impl.ownerProfileId stays unset so getSharingManager() must fetch the - // owner profile through the (stubbed) owner User DO -- the parked deferred below. - let impl = (instance as unknown as { impl: any }).impl; - let releaseWhoami!: (profile: { id: string; name: string }) => void; - let whoami = new Promise<{ id: string; name: string }>(resolve => { - releaseWhoami = resolve; - }); - impl.ownerId = "owner-do-id"; - impl.users = { - idFromString: (id: string) => id, - get: () => ({ whoami: () => whoami }), - }; - impl.getGatekeeperFacet = () => ({ - describe: async () => ({ title: "Producer", url: "test://producer" }), - }); - - // A real client for a real record, so the test drives the actual remove() path. - let client = await impl.addGatekeeper({} as any, { - type: "gatekeeper", - vendorId: "testvendor", - resourceUrl: "https://example.com/producer", - typeUrlPattern: "https://*", - }); - let id = await client.getId(); - seedRestrictedObservation(impl, id, 100); - impl.storage.containsRestrictedData.put(true); - - // Start the removal: it runs synchronously up to the parked whoami. Grant a collaborator - // mid-park, then release -- the decision must see the grant and refuse. - let removal = client.remove(); - seedCollaborator(impl); - releaseWhoami({ id: OWNER, name: "Alice" }); - - await expect(removal).rejects.toThrow(/cannot be removed/); - expect(impl.storage.gatekeepers.get(id)).toBeDefined(); - }); - }); -}); - -// The ambient reconciliation removes a capsule record whose account is gone or was replaced -- -// an internal removal that must consult the same guard: a stale record that is a restricted -// producer still anchors collaborator verification. -describe("ensureAmbientCapsules reconciliation", () => { - const AMBIENT_ID = 1; - - // Seeds a stale ambient producer (record bound to accountId 10, owner now holding accountId - // 20) plus the latch, and fakes the owner's User DO and the gatekeeper facet so - // ensureAmbientCapsules can run without any real cross-DO call. - function seedStaleAmbientProducer(impl: any): void { - impl.storage.gatekeepers.put({ - id: AMBIENT_ID, - resourceTitle: "Test Ambient", - class: {} as any, - creationSpec: { type: "ambient", vendorId: "testvendor", accountId: 10 }, - }); - // Keep freshly-provisioned records clear of the seeded id. - impl.storage.nextGatekeeperId.put(10); - seedRestrictedObservation(impl, AMBIENT_ID, 100); - impl.storage.containsRestrictedData.put(true); - - impl.ownerId = "owner-do-id"; - impl.users = { - idFromString: (id: string) => id, - get: () => ({ - listProvidedAccounts: async () => [{ - vendorId: "testvendor", - accountId: 20, - description: { singleton: { tsType: "TestThing" } }, - }], - getSingletonGatekeeperClass: async () => ({} as any), - }), - }; - impl.getGatekeeperFacet = () => ({ - describe: async () => ({ title: "Test Ambient", url: "test://ambient" }), - }); - } - - function ambientRecords(impl: any): { id: number; accountId: number }[] { - return [...impl.storage.gatekeepers.list()] - .filter((gk: any) => gk.creationSpec?.type === "ambient") - .map((gk: any) => ({ id: gk.id, accountId: gk.creationSpec.accountId })); - } - - it("keeps a guarded stale producer and still provisions the replacement", async () => { - let stub = env.TEST_OVERSEER.getByName("ambient-reconcile-guarded"); - await runInDurableObject(stub, async (instance: OverseerDurableObject) => { - let impl = getImpl(instance); - seedStaleAmbientProducer(impl); - seedCollaborator(impl); - - await impl.ensureAmbientCapsules(); - - // The stale record anchors the collaborator's verification, so it survives; the - // replacement account still gets its own fresh capsule record. - let records = ambientRecords(impl); - expect(records).toContainEqual({ id: AMBIENT_ID, accountId: 10 }); - expect(records.filter(r => r.accountId === 20)).toHaveLength(1); - }); - }); - - it("still reconciles a stale producer away while the workspace is unshared", async () => { - let stub = env.TEST_OVERSEER.getByName("ambient-reconcile-unshared"); - await runInDurableObject(stub, async (instance: OverseerDurableObject) => { - let impl = getImpl(instance); - seedStaleAmbientProducer(impl); - - await impl.ensureAmbientCapsules(); - - let records = ambientRecords(impl); - expect(records.find(r => r.id === AMBIENT_ID)).toBeUndefined(); - expect(records.filter(r => r.accountId === 20)).toHaveLength(1); - }); - }); -}); - -// assertNewSharingAllowed() must refuse every new grant once a restricted producer cannot verify -// collaborators -- whether its record is gone, is legacy (no creationSpec; observerVendorId -// throws, so recipients hard-deny at open while the grant blocks producer removal), or is an -// aiModel/agentSpawner producer with no vendor account (filtered out of every verification scope, -// so recipients would open completely unverified and read the restricted history in chat). -describe("assertNewSharingAllowed", () => { - it("allows sharing while a verifiable producer's record survives", async () => { - let stub = env.TEST_OVERSEER.getByName("sharing-allowed-verifiable"); - await runInDurableObject(stub, async (instance: OverseerDurableObject) => { - let impl = getImpl(instance); - seedGatekeeper(impl, 1); - seedRestrictedObservation(impl, 1, 100); - impl.storage.containsRestrictedData.put(true); - - expect(() => impl.assertNewSharingAllowed()).not.toThrow(); - }); - }); - - it("refuses when a producer's record has been removed", async () => { - let stub = env.TEST_OVERSEER.getByName("sharing-allowed-removed"); - await runInDurableObject(stub, async (instance: OverseerDurableObject) => { - let impl = getImpl(instance); - seedRestrictedObservation(impl, 1, 100); - impl.storage.containsRestrictedData.put(true); - - expect(() => impl.assertNewSharingAllowed()).toThrow(/has since been removed/); - }); - }); - - it("refuses a legacy producer that cannot verify collaborators", async () => { - let stub = env.TEST_OVERSEER.getByName("sharing-allowed-legacy"); - await runInDurableObject(stub, async (instance: OverseerDurableObject) => { - let impl = getImpl(instance); - seedGatekeeper(impl, 1, /* creationSpec */ false); - seedRestrictedObservation(impl, 1, 100); - impl.storage.containsRestrictedData.put(true); - - expect(() => impl.assertNewSharingAllowed()).toThrow(/cannot verify collaborators/); - }); - }); - - it("refuses an aiModel producer that cannot verify collaborators", async () => { - let stub = env.TEST_OVERSEER.getByName("sharing-allowed-ai-model"); - await runInDurableObject(stub, async (instance: OverseerDurableObject) => { - let impl = getImpl(instance); - impl.storage.gatekeepers.put({ - id: 1, - resourceTitle: "AI model", - class: {} as any, - creationSpec: { - type: "aiModel", modelId: "m-1", provider: "anthropic", modelName: "claude-sonnet-5", - }, - }); - seedRestrictedObservation(impl, 1, 100); - impl.storage.containsRestrictedData.put(true); - - // No vendor account stands behind the producer (observerVendorId returns null), so no - // recipient could ever be verified against it. - expect(() => impl.assertNewSharingAllowed()).toThrow(/cannot verify collaborators/); - }); - }); - - it("never refuses while the workspace is unlatched", async () => { - let stub = env.TEST_OVERSEER.getByName("sharing-allowed-unlatched"); - await runInDurableObject(stub, async (instance: OverseerDurableObject) => { - let impl = getImpl(instance); - // Even a restricted-looking observation through a missing record does not refuse without - // the latch: the latch and the record are written together, so unlatched means none. - seedRestrictedObservation(impl, 1, 100); - - expect(() => impl.assertNewSharingAllowed()).not.toThrow(); - }); - }); -}); diff --git a/packages/workshop-backend/__tests__/sharing.test.ts b/packages/workshop-backend/__tests__/sharing.test.ts index a8d28a591..cb1bf7872 100644 --- a/packages/workshop-backend/__tests__/sharing.test.ts +++ b/packages/workshop-backend/__tests__/sharing.test.ts @@ -169,52 +169,6 @@ describe("redeemShareKey", () => { }); expect(storage.collaborators.get("a")).toBeUndefined(); }); - - it("a throwing assertGrantAllowed rejects a new recipient with nothing persisted", async () => { - let { storage, mgr } = makeManager(); - let { key } = await mgr.createShareLink({ caller: owner, role: "build" }); - - await expect(mgr.redeemShareKey({ - rawKey: key, profileId: "a", fetchProfile: async () => profile("a"), - assertGrantAllowed: () => { throw new Error("sharing is closed"); }, - })).rejects.toThrow(/sharing is closed/); - - // No collaborator record and no edge were written. - expect(storage.collaborators.get("a")).toBeUndefined(); - }); - - it("does not invoke assertGrantAllowed for an already-existing edge", async () => { - let { mgr } = makeManager(); - let { key } = await mgr.createShareLink({ caller: owner, role: "build" }); - await mgr.redeemShareKey({ - rawKey: key, profileId: "a", fetchProfile: async () => profile("a"), - }); - - // An existing edge is an existing grant, not a new one: the redemption stays a no-op even - // when policy forbids new sharing (a collaborator re-opening with a retained key). - await expect(mgr.redeemShareKey({ - rawKey: key, profileId: "a", fetchProfile: async () => profile("a"), - assertGrantAllowed: () => { throw new Error("sharing is closed"); }, - })).resolves.toBeUndefined(); - expect(mgr.getEffectiveRole("a")).toBe("build"); - }); - - it("invokes a passing assertGrantAllowed once and writes the edge", async () => { - let { storage, mgr } = makeManager(); - let { key, linkId } = await mgr.createShareLink({ caller: owner, role: "build" }); - - let calls = 0; - await mgr.redeemShareKey({ - rawKey: key, profileId: "a", fetchProfile: async () => profile("a"), - assertGrantAllowed: () => { calls++; }, - }); - - expect(calls).toBe(1); - expect(storage.collaborators.get("a")!.addedBy).toEqual([ - expect.objectContaining({ type: "shareKey", keyId: linkId }), - ]); - expect(mgr.getEffectiveRole("a")).toBe("build"); - }); }); describe("addCollaborator", () => { @@ -270,68 +224,6 @@ describe("addCollaborator", () => { mgr.addCollaborator({ caller: owner, profile: profile("a"), role: "use" }); expect(mgr.getEffectiveRole("a")).toBe("build"); }); - - it("does not invoke assertGrantAllowed for a same-or-lower re-grant", () => { - let { storage, mgr } = makeManager(); - seedCollaborator(storage, "a", [userEdge(OWNER, "build")]); - let closed = () => { throw new Error("sharing is closed"); }; - - // Same role: an existing grant, not a new one, so only the note changes even when policy - // forbids new sharing. - expect(() => mgr.addCollaborator({ - caller: owner, profile: profile("a"), role: "build", note: "updated", - assertGrantAllowed: closed, - })).not.toThrow(); - let record = storage.collaborators.get("a")!; - expect(record.addedBy).toHaveLength(1); - expect(record.addedBy[0]).toEqual(expect.objectContaining({ role: "build", note: "updated" })); - expect(mgr.getEffectiveRole("a")).toBe("build"); - - // Lower role: never downgrades, and creates no grant either. - expect(() => mgr.addCollaborator({ - caller: owner, profile: profile("a"), role: "use", assertGrantAllowed: closed, - })).not.toThrow(); - expect(storage.collaborators.get("a")!.addedBy).toHaveLength(1); - expect(mgr.getEffectiveRole("a")).toBe("build"); - }); - - it("invokes assertGrantAllowed for a new collaborator, a new edge, and a role rise, and a throw persists nothing", () => { - let { storage, mgr } = makeManager(); - let closed = () => { throw new Error("sharing is closed"); }; - - // New collaborator: no record written. - expect(() => mgr.addCollaborator({ - caller: owner, profile: profile("a"), role: "build", assertGrantAllowed: closed, - })).toThrow(/sharing is closed/); - expect(storage.collaborators.get("a")).toBeUndefined(); - - // New edge from a different sharer onto an existing collaborator: addedBy unchanged. - seedCollaborator(storage, "b", [userEdge(OWNER, "build")]); - seedCollaborator(storage, "a", [userEdge("b", "use")]); - expect(() => mgr.addCollaborator({ - caller: owner, profile: profile("a"), role: "build", assertGrantAllowed: closed, - })).toThrow(/sharing is closed/); - expect(storage.collaborators.get("a")!.addedBy).toHaveLength(1); - expect(mgr.getEffectiveRole("a")).toBe("use"); - - // Role rise on the existing same-sharer edge: role unchanged. - expect(() => mgr.addCollaborator({ - caller: collab("b"), profile: profile("a"), role: "build", assertGrantAllowed: closed, - })).toThrow(/sharing is closed/); - expect(storage.collaborators.get("a")!.addedBy[0]).toEqual(expect.objectContaining({ - type: "user", sharer: "b", role: "use", - })); - expect(mgr.getEffectiveRole("a")).toBe("use"); - - // A passing check is invoked exactly once and the grant is written. - let calls = 0; - mgr.addCollaborator({ - caller: owner, profile: profile("a"), role: "build", assertGrantAllowed: () => { calls++; }, - }); - expect(calls).toBe(1); - expect(storage.collaborators.get("a")!.addedBy).toHaveLength(2); - expect(mgr.getEffectiveRole("a")).toBe("build"); - }); }); describe("computeEffectiveRoles", () => { @@ -588,26 +480,6 @@ describe("createShareLink", () => { expect(() => mgr.createShareLink({ caller: collab("a"), role: "build" })) .rejects.toThrow(/higher than your own/); }); - - it("a throwing assertGrantAllowed aborts with nothing persisted", async () => { - let { storage, mgr } = makeManager(); - await expect(mgr.createShareLink({ - caller: owner, role: "build", - assertGrantAllowed: () => { throw new Error("sharing is closed"); }, - })).rejects.toThrow(/sharing is closed/); - // The minted key was discarded, never stored. - expect([...storage.shareKeys.list()]).toEqual([]); - }); - - it("invokes assertGrantAllowed once and persists the grant when it passes", async () => { - let { mgr } = makeManager(); - let calls = 0; - let { linkId } = await mgr.createShareLink({ - caller: owner, role: "use", assertGrantAllowed: () => { calls++; }, - }); - expect(calls).toBe(1); - expect(mgr.listShareLinkRecords().map(r => r.id)).toEqual([linkId]); - }); }); describe("newShareLinkKey", () => { @@ -669,23 +541,6 @@ describe("newShareLinkKey", () => { .rejects.toThrow(/higher than your own/); }); - it("a throwing assertGrantAllowed aborts the copy with nothing persisted", async () => { - let { storage, mgr } = makeManager(); - let { linkId } = await mgr.createShareLink({ caller: owner, role: "build" }); - - await expect(mgr.newShareLinkKey({ - caller: owner, linkId, - assertGrantAllowed: () => { throw new Error("sharing is closed"); }, - })).rejects.toThrow(/sharing is closed/); - // Only the original link record remains; the aborted copy's key was never stored. - expect([...storage.shareKeys.list()].map(r => r.id)).toEqual([linkId]); - - let calls = 0; - await mgr.newShareLinkKey({ caller: owner, linkId, assertGrantAllowed: () => { calls++; } }); - expect(calls).toBe(1); - expect([...storage.shareKeys.list()]).toHaveLength(2); - }); - it("cannot manage a link through the id of one of its copies", async () => { let { storage, mgr } = makeManager(); await mgr.createShareLink({ caller: owner, role: "build" }); diff --git a/packages/workshop-backend/src/overseer.ts b/packages/workshop-backend/src/overseer.ts index 04a509403..67c4a4a8c 100644 --- a/packages/workshop-backend/src/overseer.ts +++ b/packages/workshop-backend/src/overseer.ts @@ -493,9 +493,6 @@ function fallbackBindingName(base: string, isTaken: (name: string) => boolean): function observerVendorId(record: GatekeeperRecord): string | null { if (!record.creationSpec) { - // There is no reconnect affordance for a legacy record (it never persisted its vendor - // identity), so the message points at the two real remedies: the owner removing the - // connection (allowed only while unshared), or moving the work to a new workspace. throw new Error( "This workspace has a legacy connection that cannot verify collaborators' access. Its " + "owner must remove the connection before the workspace can be shared, or start a new " + @@ -1157,8 +1154,7 @@ export function makeOverseerStorage(storage: DurableObjectStorage) { deadWorktreeIds: [], // True if any past observation was authorized that had the `containsRestrictedData` flag - // set in its `ObservationDescription`. While set, the workspace may not perform actions or - // fetch from the public web. The key on disk predates the flag's rename. + // set in its `ObservationDescription`. The key on disk predates the flag's rename. containsRestrictedData: singleton(false, {storageKey: "prohibitAllSharing"}), }, @@ -1542,21 +1538,6 @@ export function sanitizeMessageFormatRefs( // collaborator roles. type SessionKind = CollaboratorRole | "owner"; -// Action records that predate the flag's rename carry `containsRestrictedData` under its old -// name, `prohibitAllSharing`. Records are data at rest and are never rewritten, so the -// tolerance can never be removed. -type LegacyObservationDescription = ObservationDescription & { prohibitAllSharing?: boolean }; - -/** - * Whether a persisted observation description carries the restricted-data flag, under either its - * current name or the pre-rename one still present on older records. Exported for its unit test; - * every read of the flag off a persisted record must go through this. - */ -export function observationContainsRestrictedData(description: ObservationDescription): boolean { - let d: LegacyObservationDescription = description; - return (d.containsRestrictedData ?? d.prohibitAllSharing) === true; -} - class OverseerImpl implements AgentHooks { public storage: OverseerStorage; readonly logger: ReturnType; @@ -5540,27 +5521,6 @@ class OverseerImpl implements AgentHooks { } if (description.containsRestrictedData) { - // Resolved here rather than up front: on a cold DO this is an RPC to the owner's User DO, - // and an ordinary unrestricted observation must not pay for it. The producer record is read - // *after* that await, so the check below and the latch are one synchronous block -- a record - // read before the await could be stale by the time it is checked, and latching against a - // stale one permanently bricks sharing. - let sharing = await this.getSharingManager(); - let producer = this.storage.gatekeepers.get(gatekeeperId); - - // An in-flight facet RPC can outlive removeGatekeeper, so a restricted observation can - // arrive naming a connection this workspace no longer has. Latching a missing producer id - // permanently bricks sharing (assertNewSharingAllowed's missing-record branch), so refuse - // the read instead -- including on an unshared workspace, where nothing else would stop it. - // This same read is what refuses a connection removed during the exclusion awaits above, - // where the latch is not yet set and so removalBlockedByRestrictedData does not yet protect - // the producer. - if (!producer) { - throw new Error( - "This observation was blocked because it contains sensitive data, but the " + - "connection it was read through has been removed from this workspace."); - } - this.#assertUnverifiableProducerUnshared(producer, sharing); this.storage.containsRestrictedData.put(true); } @@ -5692,114 +5652,6 @@ class OverseerImpl implements AgentHooks { }); } - // Refuse a restricted observation from a producer nobody can ever be verified against: a - // gatekeeper with no vendor account behind it (aiModel/agentSpawner) or a legacy record with no - // creationSpec. Every *other* producer is enforced at admission -- a collaborator cannot open - // the workspace without passing addObserver() for it, and anything that widens what they must - // pass restarts every live session (see #restartIfSessionsAffected) -- but #inScopeGatekeepers skips - // these, so no collaborator is ever asked about them and admission cannot see them at all. - // Consistent with assertNewSharingAllowed(), which treats the same case as unshareable. - // - // "Shared" means any collaborator *or* any outstanding share link -- the same predicate as - // removalBlockedByRestrictedData (and what the pre-verification hasAnyShares() refusal counted). - // Links matter because their keys never expire and are multi-redeemable: if this read were - // admitted, the latch would make assertNewSharingAllowed() refuse every later redemption, so a - // link the owner has already handed out would be permanently unredeemable with no way back. - // - // Deliberately synchronous (the sharing manager is a parameter, not an internal await) so the - // caller can check and latch in one synchronous block -- see authorizeObservation. - #assertUnverifiableProducerUnshared(gatekeeper: GatekeeperRecord, sharing: SharingManager): void { - if (sharing.listCollaborators().length === 0 && - sharing.listShareLinkRecords().length === 0) { - return; - } - - let vendorId: string | null = null; - try { - vendorId = observerVendorId(gatekeeper); - } catch { - // Legacy connection with no creationSpec: treat as unverifiable. - } - if (vendorId !== null) return; - - // The message reaches sandboxed gadget code and agent output -- an audience that can't - // otherwise list collaborators -- so it reports only that the workspace is shared, naming - // neither the collaborators nor their profile ids (the full email on OAuth/CF Access - // deployments). - throw new Error( - "This observation was blocked because it contains sensitive data, but it was read " + - "through a connection that cannot verify anyone's access to that data, and this " + - "workspace is shared. Its collaborators must be removed and its share links revoked " + - "before this data can be read."); - } - - // The connection ids this workspace has read restricted data through: the producers the latch - // guards. Derived by scanning the action log for observations whose description carries - // `containsRestrictedData`, since nothing else records which connection a latched read came - // through. - restrictedProducerIds(): Set { - let producers = new Set(); - for (let record of this.storage.actions.list()) { - if (record.type === "observation" && - observationContainsRestrictedData(record.description) && - record.gatekeeperId !== BUILTIN_TOOL_GATEKEEPER_ID) { - producers.add(record.gatekeeperId); - } - } - return producers; - } - - // True if removing gatekeeper `id` is blocked because it anchors restricted-data verification: - // the workspace is latched, `id` is a restricted producer (or the producer set is unexpectedly - // empty -- see below), and the sharing graph still has collaborators or outstanding share - // links. Shared by GatekeeperClientImpl.remove() and the ambient reconciliation in - // ensureAmbientCapsules(): while the workspace is shared, deleting a producer's record would - // let a never-verified party see the data -- the record is what observer verification runs - // against at every open, and for an unverifiable record it is what refuses the producer's reads - // outright -- even though the restricted data outlives it in chat history and storage. - // - // Deliberately synchronous (the sharing manager is a parameter, not an internal await) so each - // caller can check and delete in one synchronous block -- see GatekeeperClientImpl.remove(). - removalBlockedByRestrictedData(id: WorkpieceId, sharing: SharingManager): boolean { - if (!this.storage.containsRestrictedData.get()) return false; - // An empty producer set with the latch set should be impossible: the latch and the action - // record are written in one synchronous block, built-in observations never latch, and - // records that predate the flag's rename still read correctly (see - // observationContainsRestrictedData). If it ever happens anyway, fall back to guarding - // every connection rather than none. - let producers = this.restrictedProducerIds(); - if (producers.size > 0 && !producers.has(id)) return false; - return sharing.listCollaborators().length > 0 || sharing.listShareLinkRecords().length > 0; - } - - // Refuse a new sharing grant once the workspace has read restricted data through a connection - // that can no longer verify a recipient's access to it -- one that has since been removed, or - // that never had a vendor account behind it. Every other producer verifies its collaborators at - // each open, so sharing stays available. - assertNewSharingAllowed(): void { - if (!this.storage.containsRestrictedData.get()) return; - for (let id of this.restrictedProducerIds()) { - let producer = this.storage.gatekeepers.get(id); - if (!producer) { - throw new Error( - "This workspace can no longer be shared: it read sensitive data through a connection " + - "that has since been removed, so new collaborators can no longer be verified for " + - "access to that data."); - } - let vendorId: string | null = null; - try { - vendorId = observerVendorId(producer); - } catch { - // Legacy connection with no creationSpec: treat as unverifiable. - } - if (vendorId === null) { - throw new Error( - "This workspace can no longer be shared: it read sensitive data through a connection " + - "that cannot verify collaborators' access to that data."); - } - } - } - // Enforce an observation's `excludeObservers`, named by the gatekeeper `gatekeeperId` produced // it. For each named opaque observerId: // - Map it back to a profileId via the byObserverId index. An unknown id is not an active @@ -5826,11 +5678,10 @@ class OverseerImpl implements AgentHooks { let outOfScope: string[] = []; for (let observerId of observerIds) { let observer = this.storage.observers.byObserverId.get(observerId); - // TODO(observer-races): a first-time ensureObserver registers a freshly minted id with the - // gatekeepers *before* the record (and so byObserverId) is persisted, so an id named here - // during that window -- which can park on the config modal -- reads as unknown and the - // observation is admitted to a collaborator it names. Fix: an in-memory map of pending ids - // consulted here, failing closed. Lands with observer-verification-fixes (7ba93821). + // TODO(observer-races): a first-time ensureObserver registers its observerId with the + // gatekeepers before the record is persisted, so an id named here in that window reads as + // unknown and the observation is admitted. Fix: an in-memory pending-id map consulted + // here, failing closed. if (!observer) continue; // not an active observer -> ignore let role = sharing.getEffectiveRole(observer.profileId); if (!role) { @@ -7590,14 +7441,11 @@ class OverseerImpl implements AgentHooks { // single round trip both provisions them and reads them back before we wire up capsules. let accounts = (await ownerDo.listProvidedAccounts()) .filter(account => account.description.singleton?.tsType); - let sharing = await this.getSharingManager(); // Reconcile existing ambient capsule records against the owner's current singleton accounts. Each // record is keyed to a specific accountId; if that account is gone (disconnected) or was replaced // (an optional account removed and re-added with a new accountId), the record is stale and would - // point the capsule at a deleted account — so remove it. With the sharing manager fetched above, - // the loop is fully synchronous: each removal-blocked check runs in the same synchronous block as - // the delete it gates, and the snapshot below cannot go stale mid-iteration. + // point the capsule at a deleted account — so remove it. Snapshot the list since we mutate it. let currentAccountId = new Map(accounts.map(account => [account.vendorId, account.accountId])); let bound = new Set(); // Snapshot before iterating, since removeGatekeeper() mutates the collection. @@ -7606,18 +7454,6 @@ class OverseerImpl implements AgentHooks { if (gk.creationSpec?.type !== "ambient") continue; if (currentAccountId.get(gk.creationSpec.vendorId) === gk.creationSpec.accountId) { bound.add(gk.creationSpec.vendorId); - } else if (this.removalBlockedByRestrictedData(gk.id, sharing)) { - // A stale ambient record that anchors restricted-data verification must survive until - // the owner unshares -- deleting it here would be the same unchecked readmission - // GatekeeperClientImpl.remove() guards against, minus the user intent. Not added to - // `bound`, so a replacement account still gets a fresh capsule record; - // prepareChatBindings tolerates the duplicate vendor (names dedupe via the fallback - // binding name, and the dead record's session just fails). - this.logger.warn("skipping removal of stale ambient restricted producer", { - event: "singleton.capsules.reconcile.blocked", - gatekeeperId: gk.id, - vendorId: gk.creationSpec.vendorId, - }); } else { this.removeGatekeeper(gk.id); } @@ -9331,13 +9167,10 @@ class OverseerImpl implements AgentHooks { // open, since nothing they can reach needs verification against it. An in-scope one still // throws, fail-closed (and "build" scope is everything, so it always throws there). // - // TODO(known-risk): scoping by role means a "use" collaborator is never verified against a - // producer outside their scope (one no gadget binds and no enabled hook feeds) -- yet - // restricted data read from such a producer can reach gadget state and the UI they drive, - // because provenance is not tracked past the observation. Deliberately accepted for v1; the - // required fix (verify every restricted producer, or isolate restricted data by provenance) is - // recorded under "Known security risk -- never-bound producers" in - // plans/restricted-data-sharing.md, and worked through in docs/observers.md edge case 4. + // TODO(known-risk): a "use" collaborator is never verified against a producer outside their + // scope, yet restricted data read from it can reach gadget state they see, because provenance + // is not tracked past the observation. Accepted for v1; see "Known security risk -- never-bound + // producers" in plans/restricted-data-sharing.md. #inScopeGatekeepers(role: CollaboratorRole): GatekeeperRecord[] { let boundIds = role === "use" ? this.#useScopeGatekeeperIds() : undefined; @@ -9987,11 +9820,6 @@ export class OverseerDurableObject extends DurableObject { rawKey: shareKey, profileId, fetchProfile: () => clientUser.whoami(), - // An outstanding key is a new grant vector, so redemption is policy-gated like the - // grant-creating mutators. Without this, keys minted before an exempted - // (unverifiable-producer) removal -- or on a legacy-latched workspace whose producer is - // gone -- would still admit unverified recipients. - assertGrantAllowed: () => this.impl.assertNewSharingAllowed(), }); } @@ -12143,10 +11971,7 @@ class OverseerClientInterface extends RpcTarget implements Overseer { // --- Collaborator management --- // // The sharing/permission logic lives in SharingManager (./sharing). These methods handle only - // the RPC-bound pieces (resolving profiles via User DOs) and delegate the rest. Sharing stays - // available even after the workspace observes sensitive data (`containsRestrictedData`): - // access to that data is enforced per-gatekeeper by observer verification, not by blocking - // sharing wholesale. + // the RPC-bound pieces (resolving profiles via User DOs) and delegate the rest. async listObserverRequirements( role: CollaboratorRole): Promise { @@ -12167,18 +11992,11 @@ class OverseerClientInterface extends RpcTarget implements Overseer { return null; } - let sharing = await this.impl.getSharingManager(); - return sharing.addCollaborator({ + return (await this.impl.getSharingManager()).addCollaborator({ caller: this.#sharingCaller(), profile, role, note, - // Run by the manager in the same synchronous block as the grant's storage write (after - // every await): a check ahead of the awaits above could pass, a concurrent - // producer-connection removal land during the yield, and the grant still be written past - // it. The manager also skips it when no new grant is created (a same-or-lower re-grant - // over an existing edge), matching redeemShareKey's already-existing-edge case. - assertGrantAllowed: () => this.impl.assertNewSharingAllowed(), }); } @@ -12231,20 +12049,13 @@ class OverseerClientInterface extends RpcTarget implements Overseer { async createShareLink(role: CollaboratorRole, note?: string) : Promise<{ key: string; linkId: string }> { - return (await this.impl.getSharingManager()).createShareLink({ - caller: this.#sharingCaller(), role, note, - assertGrantAllowed: () => this.impl.assertNewSharingAllowed(), - }); + return (await this.impl.getSharingManager()) + .createShareLink({ caller: this.#sharingCaller(), role, note }); } async newShareLinkKey(linkId: string): Promise<{ key: string }> { - return (await this.impl.getSharingManager()).newShareLinkKey({ - caller: this.#sharingCaller(), linkId, - // A fresh key is a new grant vector even though the link already exists: it is reachable - // here when an unverifiable producer was removed while links were outstanding (which the - // removal guard deliberately allows as a remedy). - assertGrantAllowed: () => this.impl.assertNewSharingAllowed(), - }); + return (await this.impl.getSharingManager()) + .newShareLinkKey({ caller: this.#sharingCaller(), linkId }); } async listShareLinks(): Promise { @@ -12965,24 +12776,7 @@ class GatekeeperClientImpl> } async remove(): Promise { - // A connection that has read restricted data is the anchor observer verification runs - // against: while the workspace is shared, deleting its record would let a never-verified - // collaborator open unchecked even though the data persists in chat history and storage. - // Outstanding share links count as shared too: redemption is gated at open() only while the - // record exists. Only the producers themselves are guarded -- a non-producer connection - // anchors no restricted-data verification, so it stays removable while shared. - let sharing = await this.impl.getSharingManager(); - // Checked in the same synchronous block as the delete, after the only await (cf. - // addCollaborator): a check ahead of the yield could pass, a concurrent grant land during - // it, and the delete still run past it. let record = this.impl.storage.gatekeepers.get(this.id); - if (record && this.impl.removalBlockedByRestrictedData(this.id, sharing)) { - throw new Error( - "This connection cannot be removed: it has read sensitive data into this " + - "workspace, and the workspace is shared. Collaborators are verified against this " + - "connection before they may see that data, so remove all collaborators and revoke " + - "all share links first."); - } this.impl.removeGatekeeper(this.id); this.impl.recordGadgetAnalytics({ event_name: "connection_removed", diff --git a/packages/workshop-backend/src/sharing.ts b/packages/workshop-backend/src/sharing.ts index fc847465f..e3acb20d8 100644 --- a/packages/workshop-backend/src/sharing.ts +++ b/packages/workshop-backend/src/sharing.ts @@ -15,11 +15,8 @@ // re-adding a removed collaborator restores them and, transitively, everyone they had shared with. // (Records and revoked keys accumulate in storage; a future GC could reclaim long-dead entries.) // -// NOTE: The sensitive-data (`containsRestrictedData`) policy intentionally does NOT live here. -// It is a broader "what may this gadget do after reading restricted data?" policy (it gates -// gatekeeper writes and web fetches, and requires per-gatekeeper observer verification of -// collaborators) and is expected to grow into a separate policy engine. The Overseer enforces -// it; this module only answers questions about the sharing graph. +// NOTE: The sensitive-data (`containsRestrictedData`) policy intentionally does NOT live here; the +// Overseer enforces it. This module only answers questions about the sharing graph. import { AiChatAuthorInfo, CollaboratorInfo, PermissionEdge, CollaboratorRole, AffectedCollaborator } from "@gadgets/workshop-shared/api"; @@ -40,6 +37,10 @@ function edgeGrantedRole(edge: PermissionEdge): CollaboratorRole { return edge.role ?? "build"; } +function maxRole(a: CollaboratorRole, b: CollaboratorRole): CollaboratorRole { + return roleRank(a) >= roleRank(b) ? a : b; +} + function minRole(a: CollaboratorRole, b: CollaboratorRole): CollaboratorRole { return roleRank(a) <= roleRank(b) ? a : b; } @@ -196,20 +197,14 @@ export class SharingManager { * * A key whose link is revoked behaves like an unknown key (it cannot be redeemed). * - * TODO: Redemption is one-step: the edge written here is real before the redeeming open()'s - * observer verification runs. Two accepted consequences, both fail-closed (availability, not - * confidentiality): an unverified redeemer is a current collaborator, so restricted reads - * block from redemption until they verify (or are removed, or the link is revoked); and a - * recipient whose verification is refused keeps the edge -- visible in listCollaborators, - * blocking restricted reads until removed. Two-phase redemption (a pending edge that grants - * nothing until verification confirms it) is the planned fix for both. + * TODO: The edge is written before the redeeming open()'s observer verification runs, so a + * recipient whose verification fails lingers in listCollaborators until removed or the link is + * revoked. */ async redeemShareKey(opts: { rawKey: string; profileId: string; fetchProfile: () => Promise; - /** See createShareLink: run synchronously with the put, a throw persists nothing. */ - assertGrantAllowed?: () => void; }): Promise { let hash = await hashShareKey(opts.rawKey); let keyRecord = this.storage.shareKeys.get(hash); @@ -226,12 +221,10 @@ export class SharingManager { let existing = this.storage.collaborators.get(opts.profileId); if (existing) { // User is already a collaborator. Only add an edge if they don't already have one for this - // link (redeeming a second key of the same link is a no-op, so no new grant and no policy - // check). + // link (redeeming a second key of the same link is a no-op). let alreadyHasEdge = existing.addedBy.some( e => e.type === "shareKey" && e.keyId === linkId); if (!alreadyHasEdge) { - opts.assertGrantAllowed?.(); existing.addedBy.push({ type: "shareKey", keyId: linkId, @@ -243,7 +236,6 @@ export class SharingManager { } else { // New collaborator -- need full profile from their user DO. let profile = await opts.fetchProfile(); - opts.assertGrantAllowed?.(); this.storage.collaborators.put({ profile, addedBy: [{ @@ -281,22 +273,14 @@ export class SharingManager { /** * Add a collaborator with a `user` edge from the caller, granting `role`. The caller is - * responsible for resolving `profile` (via RPC) and supplies the policy hook; the manager - * decides whether the call actually creates a grant (a new record, a new edge from this - * sharer, or a role rise on the existing edge) and invokes the hook only then, so a - * same-or-lower re-grant (which at most updates the edge's note) is never refused by policy. - * The caller may not grant a role higher than their own effective role. + * responsible for resolving `profile` (via RPC) and for any policy checks. The caller may not + * grant a role higher than their own effective role. */ addCollaborator(opts: { caller: SharingCaller; profile: AiChatAuthorInfo; role: CollaboratorRole; note?: string; - /** - * See createShareLink: run synchronously with the put, a throw persists nothing. Skipped when - * no new grant is created (same-or-lower re-grant over an existing edge from this sharer). - */ - assertGrantAllowed?: () => void; }): CollaboratorInfo { // Don't add the owner as a collaborator. if (opts.profile.id === this.ownerProfileId) { @@ -323,15 +307,9 @@ export class SharingManager { let existingEdge = existing.addedBy.find( e => e.type === "user" && e.sharer === opts.caller.profileId); if (existingEdge && existingEdge.type === "user") { - // A role rise widens the grant; a same-or-lower role leaves it untouched (only the note - // may change), so no policy check. - if (roleRank(opts.role) > roleRank(edgeGrantedRole(existingEdge))) { - opts.assertGrantAllowed?.(); - existingEdge.role = opts.role; - } + existingEdge.role = maxRole(edgeGrantedRole(existingEdge), opts.role); if (opts.note !== undefined) existingEdge.note = opts.note; } else { - opts.assertGrantAllowed?.(); existing.addedBy.push(edge); } this.storage.collaborators.put(existing); @@ -346,7 +324,6 @@ export class SharingManager { profile: opts.profile, addedBy: [edge], }; - opts.assertGrantAllowed?.(); this.storage.collaborators.put(record); return { profile: record.profile, @@ -440,16 +417,7 @@ export class SharingManager { } async createShareLink( - opts: { - caller: SharingCaller; - role: CollaboratorRole; - note?: string; - /** - * Optional policy check invoked synchronously with the grant's storage write, after - * every await, so a policy change cannot slip between check and grant. - */ - assertGrantAllowed?: () => void; - }) + opts: { caller: SharingCaller; role: CollaboratorRole; note?: string }) : Promise<{ key: string; linkId: string }> { let callerRole = this.#requireCallerRole(opts.caller); if (roleRank(opts.role) > roleRank(callerRole)) { @@ -458,7 +426,6 @@ export class SharingManager { // The link is stored as its first key: the record is keyed by that key's hash. let { key, hash } = await this.#mintKey(); - opts.assertGrantAllowed?.(); this.storage.shareKeys.put({ id: hash, note: opts.note, @@ -470,12 +437,7 @@ export class SharingManager { } /** Mints another key for an existing link. */ - async newShareLinkKey(opts: { - caller: SharingCaller; - linkId: string; - /** See createShareLink: run synchronously with the put, a throw persists nothing. */ - assertGrantAllowed?: () => void; - }): Promise<{ key: string }> { + async newShareLinkKey(opts: { caller: SharingCaller; linkId: string }): Promise<{ key: string }> { let link = this.#requireLink(opts.linkId); if (link.revoked) { throw new Error("Share link not found."); @@ -490,7 +452,6 @@ export class SharingManager { } let { key, hash } = await this.#mintKey(); - opts.assertGrantAllowed?.(); this.storage.shareKeys.put({ id: hash, alias: link.id }); return { key }; } diff --git a/packages/workshop-shared/src/api.ts b/packages/workshop-shared/src/api.ts index e4b90c183..24d4b1410 100644 --- a/packages/workshop-shared/src/api.ts +++ b/packages/workshop-shared/src/api.ts @@ -1307,10 +1307,9 @@ export type GadgetMetadata = { role?: CollaboratorRole; /** - * True when the gadget has observed data marked as containing restricted data (see - * `ObservationDescription.containsRestrictedData`). Such gadgets can still be shared, but - * collaborators must be verified (per gatekeeper) to have access to the same data, and the - * workspace can no longer perform actions or fetch from the public web. + * True when the gadget has observed data marked `containsRestrictedData` (see + * `ObservationDescription`). It can still be shared, with collaborators verified per + * gatekeeper, but can no longer perform actions or fetch from the public web. */ containsRestrictedData?: boolean; diff --git a/packages/workshop-shared/src/gatekeeper.ts b/packages/workshop-shared/src/gatekeeper.ts index b44abbe84..7dbde6eca 100644 --- a/packages/workshop-shared/src/gatekeeper.ts +++ b/packages/workshop-shared/src/gatekeeper.ts @@ -1213,21 +1213,12 @@ export type ObservationDescription = { /** * If true, then this observation contains sensitive information that must only be shown to * people who are verified to have access to the same data. This means: - * - Access to the gadget is conditioned on verification: every collaborator passed this - * gatekeeper's `addObserver()` at their most recent open and cannot open without passing it, - * so a gatekeeper whose `addObserver()` always throws is unshareable once it has made one of - * these observations. Anything that widens what a collaborator must be verified against -- - * adding a connection, binding one into a gadget -- restarts the workspace, so every live - * session re-opens and re-verifies against the new scope. + * - Every collaborator must pass this gatekeeper's `addObserver()` to open the gadget, so a + * gatekeeper whose `addObserver()` always throws makes the gadget effectively unshareable + * once it has made one of these observations. * - Once observed, the gadget enters a restricted mode: no more actions or public-web fetches, * only observations, so the gadget cannot leak the data through other gatekeepers. * - * Two limits are worth stating plainly. Verification is held to the collaborator's role scope, - * so a gatekeeper the agent reads only through a chat binding is in no "use" collaborator's - * scope and they are never verified against it. And enforcement is at admission rather than at - * each read, so a session whose holder should no longer be admitted is severed within ~100ms of - * the change rather than instantaneously. - * * TODO(someday): The restricted mode is a blunt instrument. It should be possible to perform * actions whose visibility is limited to people verified to have access to the same data, * but this requires a more complex policy framework to compute. diff --git a/plans/restricted-data-sharing.md b/plans/restricted-data-sharing.md index b5a6d8ebf..82cbf3b5a 100644 --- a/plans/restricted-data-sharing.md +++ b/plans/restricted-data-sharing.md @@ -36,38 +36,31 @@ commits. key explicitly: `containsRestrictedData: singleton(false, {storageKey: "prohibitAllSharing"})`. `storageKey` is a typed-storage schema option added for this, so the exception lives in the schema rather than as a special case at each call site. -- **Persisted records are read through a legacy shim.** Old action-log entries still - carry `prohibitAllSharing` in their recorded `ObservationDescription`. - `observationContainsRestrictedData()` (with a local `LegacyObservationDescription` - type) reads either spelling. This is a read-side shim only — no producer may write the - old name. - **Admission is per-collaborator, checked continuously.** Not at grant time: at every `open()`, so revocation of a collaborator's underlying resource access is caught promptly. Nobody is in the workspace without having passed the producer's `addObserver()`, and anything that widens what they must pass restarts every live session so it re-opens at the new scope (`#restartIfSessionsAffected`). - `authorizeObservation` itself only has to refuse the one producer admission cannot see: - an unverifiable one (`#assertUnverifiableProducerUnshared`). - **Coverage is held to each collaborator's own role scope.** `ensureObserver` never verifies a `use` collaborator against a gatekeeper outside their scope. This is a liveness tradeoff, not a security guarantee: restricted data can flow from that gatekeeper through the agent into gadget-visible state. The exception for an unbound producer and a `use` collaborator is the known security risk stated above. - **Share-key redemption stays one-step.** Redeeming a key writes a real edge - immediately, as on main, gated by `assertNewSharingAllowed` synchronously with the - write. The redeeming open then verifies the recipient like any other collaborator. - Two consequences are accepted on the ledger below: an unverified redeemer, and a - refused recipient, both persist in `listCollaborators` until removed, which is enough - to make the workspace count as shared. Two-phase redemption (a pending edge granting - nothing until verification confirms it) is the planned follow-up fix for both. + immediately, as on main. The redeeming open then verifies the recipient like any other + collaborator. Two consequences are accepted on the ledger below: an unverified + redeemer, and a refused recipient, both persist in `listCollaborators` until removed. + Two-phase redemption (a pending edge granting nothing until verification confirms it) + is the planned follow-up fix for both. - **One authorization gate for every non-owner entry point.** `authorizeCollaborator` resolves the effective role and runs `ensureObserver`. Both `open()` and `receiveExternalMessage()` pass through it; the latter non-interactively, since there is no way to configure connected accounts from an inbound message. -- **Removing the producing connection does not lift the restriction** for existing - collaborators. It does close the workspace to *new* grants - (`assertNewSharingAllowed`), since there is no longer an anchor to verify a newcomer - against. +- **Removing the producing connection is not guarded.** The latch stays set, but nothing + stops the removal even though the record is what collaborators are verified against. + There is no UI to remove a connection today; when one is built, it will require the + owner to certify that no sensitive data from the connection has been retained in the + workspace, for any connection. - **Fail closed everywhere.** An operational failure — provider outage, expired credential — is treated exactly like a refusal. @@ -82,14 +75,13 @@ commits. - `SharingManager` (sharing.ts) owns the permission graph: collaborator records, their `addedBy` edges, share links and keys, and `computeEffectiveRoles`' fixed-point resolution. The module header states that sharing *policy* deliberately lives outside - it — this plan keeps that boundary by passing policy in as `assertGrantAllowed` - callbacks. + it. - `#inScopeGatekeepers(role)` derives what a collaborator must be verified against. `use` scope is live gadget-binding state; `build` scope is broader. ## Design -### 1. Admission, and the residual guard (`#assertUnverifiableProducerUnshared`) +### 1. Admission Coverage is enforced by admission rather than per observation. `ensureObserver` verifies each collaborator against every in-scope gatekeeper at every `open()`, and @@ -98,31 +90,16 @@ one bound into a gadget, a merge promoting such a binding, a hook enabled), so n session outlives the scope it was verified at. It is a no-op unless a collaborator session of the widened role is live — severing sessions is all a restart does. -What survives in `authorizeObservation` is the one producer admission structurally cannot -see: one with no vendor account behind it (`aiModel`/`agentSpawner`, or a legacy record -with no `creationSpec`). `#inScopeGatekeepers` skips those, so no collaborator is ever -asked about them, and its restricted observations are refused outright while the -workspace has any collaborator or outstanding share link — consistent with -`assertNewSharingAllowed`, which already treats the same case as unshareable. - -The error reaches sandboxed gadget code and agent output — an audience that cannot -otherwise enumerate collaborators — so it reports only that the workspace is shared, -naming neither the collaborators nor their profile ids (the full email on OAuth and CF -Access deployments). - ### 2. One-step share-key redemption (sharing.ts) `redeemShareKey` keeps main's shape: hash the key, resolve the link, and write a real `shareKey` edge (creating the collaborator record if they're new), deduplicating against -an existing edge for the same link. The one delta vs main is the `assertGrantAllowed` -policy gate, invoked synchronously before the write and only when an edge is actually -added — a no-op re-redemption skips it, so an existing collaborator's re-open with a -retained key is untouched by a latched policy. +an existing edge for the same link. The edge is real before the redeeming open's observer verification runs; the two -resulting windows (an unverified redeemer blocking restricted reads; a refused recipient -persisting until removed) are the accepted consequences on the ledger, marked by the -TODO at `redeemShareKey`. +resulting windows (an unverified redeemer and a refused recipient, each persisting in +`listCollaborators` until removed) are the accepted consequences on the ledger, marked by +the TODO at `redeemShareKey`. ### 3. The unified gate (`authorizeCollaborator`) @@ -140,16 +117,7 @@ revocation affected-set like any collaborator: a link revoked (or a removal land while their open is parked triggers the revocation restart, which severs their session and re-runs `open()` against the live graph. -### 4. Policy hooks, not policy in `SharingManager` - -`addCollaborator`, `createShareLink`, `newShareLinkKey` and `redeemShareKey` all take an -optional `assertGrantAllowed` callback, invoked synchronously with the granting write. -The overseer passes `assertNewSharingAllowed`. A throw persists nothing. The manager invokes -the hook only when a grant is actually created (a new record, a new edge, or a role rise on -an existing edge); a same-or-lower `addCollaborator` re-grant or a redemption whose edge -already exists skips it. - -### 5. Observer records on a failed live check +### 4. Observer records on a failed live check An earlier draft scrubbed the failed gatekeeper from the collaborator's persisted `accountChoices` and restarted the workspace on the failure. The observer machinery @@ -160,7 +128,7 @@ collaborator is denied at their next open regardless, and only that open is deni lazy-revocation residual in `docs/observers.md` edge case 3). Nothing in this model reads `accountChoices` to admit a restricted read, so the scrub is not a precondition of it. -### 6. Frontend +### 5. Frontend - **Share modal**: no longer replaces itself with a "can't be shared" view. Controls stay live behind a notice. @@ -205,16 +173,17 @@ path, and the scope-widening restart — landed separately in #380. `gatekeeper-mcp` and the gatekeeper-authoring skill doc. Atomic by necessity. 2. **Part 1 — API.** The restated contract on `containsRestrictedData`. Server still implements the old behavior. -3. **Part 2 — core server implementation.** The restricted-observation guard, the - redemption policy gate, `restrictedProducerIds`/`assertNewSharingAllowed`, the - producer-removal guard, the legacy flag shim, and removal of `hasAnyShares`. Places - the TODO ledger for the deferred fixes. +3. **Part 2 — core server implementation.** The latch, the removal of `hasAnyShares` + and the sharing checks, and the TODO ledger. 4. **Part 3 — backend tests.** 5. **Part 4 — integration tests.** Over real Durable Objects, through the test gatekeeper fixture's `readValue(restricted)` and its controllable verification outcome. 6. **Part 5 — documentation.** `docs/observers.md` coverage rules and residuals; - `docs/sharing.md` one-step redemption and the policy hooks; this plan. + `docs/sharing.md` one-step redemption; this plan. +7. **Part 6 — drop the producer guards per review.** Deletes the unverifiable-producer + refusal, the producer-removal guard, `assertNewSharingAllowed` and the + `assertGrantAllowed` plumbing, the action-log scan, and the legacy flag shim. The deferred items are collected in the Known-limitations section below. The Share modal unblock and the retained-share-key frontend work live in @@ -237,11 +206,10 @@ the follow-up worklist. in-memory pending-id map consulted there, failing closed — `observer-verification-fixes`. - An unverified redeemer persists as a collaborator: redemption writes a real edge before the redeeming open's verification runs, so from click onward the recipient is - visible in `listCollaborators` whether or not they ever complete the open, which is - enough to make the workspace count as shared (remedies: verify, remove, or revoke the - link). Two-phase redemption is the planned fix. + visible in `listCollaborators` whether or not they ever complete the open (remedies: + verify, remove, or revoke the link). Two-phase redemption is the planned fix. - A refused recipient persists: a recipient whose verification is refused keeps their - edge, with the same consequence as the previous item, and the same planned fix. + edge and stays in `listCollaborators` until removed; the same planned fix. - A failed re-verification denies only the open being attempted. Sessions the collaborator already holds keep the access their own opens verified until they next re-open — the lazy-revocation residual `docs/observers.md` edge case 3 already accepts, @@ -249,29 +217,12 @@ the follow-up worklist. ## Known edge cases / watch-fors -- **A producer removed mid-redemption cannot slip a grant through.** `remove()` refuses - every restricted producer (unverifiable ones included) while any share link is - outstanding, and the redemption policy gate runs synchronously with the edge write. - **Operational failures deny like refusals.** An outage or expired credential denies the collaborator's open exactly as a revocation does (the overseer cannot tell them apart); they get back in as soon as a repaired open re-verifies them. Fail-closed by design. - **Role increases do not ride out on a redeeming open.** An owner grant landing while verification waited takes effect at the recipient's next open, exactly as for an ordinary keyless open. -- **Removing an unverifiable restricted producer — implemented: guarded like any other.** - `remove()`'s producer guard used to exempt unverifiable records ("removing one is - itself a remedy"), which was backwards once the data had been read: the record is the - *blocker* -- `#inScopeGatekeepers` throws on it, so no collaborator can open -- and - removing it let every existing collaborator open unverified while the restricted data - persists in chat history, gadget storage and code (`assertNewSharingAllowed` only stops - *new* grants). Decided and implemented: fail closed -- unverifiable producers are - guarded like any other (the owner must remove all collaborators and revoke all share - links first), after which the workspace is permanently owner-only - (`restrictedProducerIds()` reads the action log, which never forgets the producer). - Deliberately no migration or reconnect flow: an automatic migration is impossible - (legacy records never persisted `vendorId`, and the class stub is opaque), and an - owner-driven reconnect flow was considered and rejected as scope. The documented - recovery for an owner who wants to share such a workspace is to start a new workspace. ## Accepted tradeoffs / future work @@ -281,9 +232,9 @@ the follow-up worklist. the data entered gadget storage while the producer *was* bound, when every `use` collaborator was verified against it or could not open the workspace; and re-binding restores verifiability at the next open. The residual is `use` grants created after the - unbind. Note that (i) does not cover a binding loopback retained across the unbind, which - returns the read directly as an RPC result and stays callable until `removeGatekeeper`; - that is the `docs/observers.md` Step 5 known gap, with `#assertBindingEdgeLive` as the + unbind. The chat-history argument does not cover a binding loopback retained across the + unbind, which returns the read directly as an RPC result and stays callable until + `removeGatekeeper`; that is the `docs/observers.md` Step 5 known gap, with `#assertBindingEdgeLive` as the named fix. - **Known security risk — never-bound producers.** A producer reachable only through chat bindings (including an ambient singleton) is never in a `use` collaborator's verification