From e2f5cba3822609876ca3f78a1a63d5cd19fd754e Mon Sep 17 00:00:00 2001 From: Maximo Guk <62088388+Maximo-Guk@users.noreply.github.com> Date: Tue, 25 Aug 2026 17:35:08 -0500 Subject: [PATCH 01/14] Bugfix: Scrub persisted observer coverage on a failed live check. The persisted observer record is the standing claim that a collaborator was verified for a producer -- `ensureObserver` reads it back on their next open and re-registers them off the account choice it holds, and `authorizeObservation` reads it from other turns. So a collaborator whose live re-verification just failed must not keep an entry saying they are covered: until now a revoked collaborator stayed "verified" until their next *successful* open. `fail()` now drops the failed gatekeeper from the persisted `accountChoices` synchronously with the failure determination, `getVerifier` moves inside the per-gatekeeper `try` so a verifier-acquisition rejection scrubs like any other refusal (and surfaces the descriptive denial rather than the raw RPC error, with no mid-flight `Promise.all` rejection to stale the rollback snapshot), and the terminal catch de-registers invalidated registrations alongside newly-added ones. That last part is a fail-open regression for a *returning* observer, marked with a TODO here and fixed in the next commit. Co-Authored-By: Claude Opus 5 --- .../__tests__/observer-coverage-scrub.test.ts | 116 ++++++++++++++++++ packages/workshop-backend/src/overseer.ts | 46 +++++-- 2 files changed, 152 insertions(+), 10 deletions(-) create mode 100644 packages/workshop-backend/__tests__/observer-coverage-scrub.test.ts diff --git a/packages/workshop-backend/__tests__/observer-coverage-scrub.test.ts b/packages/workshop-backend/__tests__/observer-coverage-scrub.test.ts new file mode 100644 index 000000000..21fa93781 --- /dev/null +++ b/packages/workshop-backend/__tests__/observer-coverage-scrub.test.ts @@ -0,0 +1,116 @@ +// A failed live check (a gatekeeper's addObserver refusing, or the verifier failing to resolve) +// must scrub that gatekeeper from the collaborator's *persisted* observer record synchronously +// with the failure determination: the record is the standing claim that this collaborator was +// verified for that producer, and this open is not going to renew it. +// +// Runs against a real OverseerDurableObject (the TEST_OVERSEER binding, like +// git-migration-do.test.ts); the gatekeeper facet and the client's User DO are the only fakes. + +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; + } +} + +function seedGatekeepers(impl: any): void { + for (let id of [1, 2]) { + impl.storage.gatekeepers.put({ + id, + resourceTitle: `Connection ${id}`, + class: {} as any, + creationSpec: { + type: "gatekeeper", + vendorId: "testvendor", + resourceUrl: `https://example.com/${id}`, + typeUrlPattern: "https://*", + }, + }); + } +} + +// A client User DO that always has the account and always mints a verifier. +const fakeClientUser = { + getVerifier: async () => ({}), + describeConnectedAccount: async () => null, +} as any; + +describe("observer coverage scrub on a failed live check", () => { + it("a refused re-verification drops the entry", async () => { + let stub = env.TEST_OVERSEER.getByName("observer-coverage-scrub-refused"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = (instance as unknown as { impl: any }).impl; + seedGatekeepers(impl); + impl.ownerProfileId = "owner"; + // Alice is a reachable collaborator whose previous successful open left coverage for both + // gatekeepers. + impl.storage.collaborators.put({ + profile: { type: "user", id: "alice", name: "Alice" }, + addedBy: [{ type: "user", sharer: "owner", created: new Date(), role: "build" }], + }); + impl.storage.observers.put( + { profileId: "alice", observerId: "obs-1", accountChoices: { 1: 10, 2: 20 } }); + + impl.getGatekeeperFacet = (id: number) => ({ + addObserver: async () => { + if (id === 1) throw new Error("access revoked upstream"); + }, + removeObserver: async () => {}, + }); + + // No repair channel, so gatekeeper 1's refusal is terminal -- and descriptive. + await expect(impl.ensureObserver("alice", fakeClientUser, "build")) + .rejects.toThrow(/could not confirm/); + + // The refused gatekeeper's coverage is scrubbed; the other's survives. + let record = impl.storage.observers.get("alice"); + expect(1 in record.accountChoices).toBe(false); + expect(record.accountChoices[2]).toBe(20); + }); + }); + + it("a getVerifier rejection scrubs that gatekeeper's persisted coverage", async () => { + let stub = env.TEST_OVERSEER.getByName("observer-coverage-scrub-getverifier"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = (instance as unknown as { impl: any }).impl; + seedGatekeepers(impl); + // Already-configured coverage for both gatekeepers, as a previous successful open left it. + impl.storage.observers.put( + { profileId: "alice", observerId: "obs-1", accountChoices: { 1: 10, 2: 20 } }); + + let removed: number[] = []; + impl.getGatekeeperFacet = (id: number) => ({ + addObserver: async () => {}, + removeObserver: async () => { removed.push(id); }, + }); + + // Gatekeeper 1's verifier never materializes: the client's User DO *rejects* (the + // deterministic vendor-mismatch throw, or any cross-worker transport failure) rather than + // returning null. + let failingClientUser = { + getVerifier: async (accountId: number) => { + if (accountId === 10) throw new Error("account is for a different vendor"); + return {}; + }, + describeConnectedAccount: async () => null, + } as any; + + // No repair channel, so the failure is terminal -- and descriptive, not the raw RPC error. + await expect(impl.ensureObserver("alice", failingClientUser, "build")) + .rejects.toThrow(/could not confirm/); + + // The rejection went through fail(): gatekeeper 1's persisted coverage is scrubbed -- so + // the record no longer claims this collaborator was verified for it -- while gatekeeper 2's + // survives. The invalidated registration is also rolled back (see the TODO in + // ensureObserver's catch about keeping re-asserted registrations for admitted observers). + let record = impl.storage.observers.get("alice"); + expect(1 in record.accountChoices).toBe(false); + expect(record.accountChoices[2]).toBe(20); + expect(removed).toEqual([1]); + }); + }); +}); diff --git a/packages/workshop-backend/src/overseer.ts b/packages/workshop-backend/src/overseer.ts index 889573a16..39a350454 100644 --- a/packages/workshop-backend/src/overseer.ts +++ b/packages/workshop-backend/src/overseer.ts @@ -7880,6 +7880,9 @@ class OverseerImpl implements AgentHooks { let observerId = record?.observerId ?? crypto.randomUUID(); // Gatekeepers we successfully registered the observer with during this call. let newlyAdded = new Set(); + // Gatekeepers that refused (or whose account was gone) during this call and have not verified + // since + let invalidated = new Set(); // Failures from the previous pass, keyed by gatekeeper id: an already-configured binding whose // chosen account was disconnected, or which the gatekeeper refused. @@ -7970,25 +7973,41 @@ class OverseerImpl implements AgentHooks { let fail = (reason: string, err?: unknown) => { failures.set(gk.id, {accountId, reason}); + // The persisted record is what asserts this collaborator was verified for this + // producer, so scrub the failed gatekeeper from it: this open is not going to renew + // that assertion. Scrub synchronously with the failure determination, re-reading the + // record since the awaits since load may have let a concurrent open update it. + // Scoped to the failed gatekeeper: coverage elsewhere stays intact, and a repaired + // pass re-persists full coverage at step 6. + invalidated.add(gk.id); + let persisted = this.storage.observers.get(profileId); + if (persisted && gk.id in persisted.accountChoices) { + delete persisted.accountChoices[gk.id]; + this.storage.observers.put(persisted); + } this.logger.warn("observer verification failed", { event: "gatekeeper.observer.verify.failed", gatekeeperId: gk.id, vendorId, accountId, observerId, error: err, }); }; - let verifier = await clientUser.getVerifier(accountId, vendorId); - if (!verifier) { - // Account gone -> the overseer authors the reason. (Wrong vendor throws above.) - fail("This account is no longer connected."); - return; - } - try { + let verifier = await clientUser.getVerifier(accountId, vendorId); + if (!verifier) { + // Account gone -> the overseer authors the reason. (Wrong vendor throws above.) + fail("This account is no longer connected."); + return; + } await this.getGatekeeperFacet(gk.id).addObserver(observerId, verifier); if (!registeredBeforeCall.has(gk.id)) newlyAdded.add(gk.id); + // Keep `invalidated` meaning "failed and has not verified since": this binding just + // verified, so it no longer rests on a scrubbed choice. Its persisted coverage stays + // scrubbed until step 6, so an open that never gets there leaves the record claiming + // less than it did before -- never more. + invalidated.delete(gk.id); } catch (err) { // Either a settled denial or an operational failure (expired credentials, upstream - // outage). Treat every failure as repairable and let the user try again. + // outage) fail(stringifyError(err), err); } })); @@ -8024,8 +8043,15 @@ class OverseerImpl implements AgentHooks { } } catch (err) { // Best-effort remove all the observers that were newly-added since we didn't persist the - // user's observer record. - await this.#removeObserverFromGatekeepers(observerId, [...newlyAdded]); + // user's observer record -- and the invalidated ones, whose coverage fail() just scrubbed: + // their registrations reference a choice that is no longer persisted anywhere. + // + // TODO: For a *returning* collaborator whose re-verification + // failed, this rollback removes registrations that preserve forward exclusion -- once + // de-registered, gatekeepers stop naming the observer in excludeObservers -- so only a + // first-ever verification should roll back fully. + await this.#removeObserverFromGatekeepers( + observerId, [...new Set([...newlyAdded, ...invalidated])]); throw err; } From 37c5c4d3b62fda4c1fb285229ec6030beca94057 Mon Sep 17 00:00:00 2001 From: Maximo Guk <62088388+Maximo-Guk@users.noreply.github.com> Date: Fri, 28 Aug 2026 14:03:55 -0500 Subject: [PATCH 02/14] Bugfix: Prune out-of-scope observer coverage at every open. The persisted observer record is meant to state what a collaborator's most recent open verified, and `ensureObserver` re-registers them off the account choices it holds. But a choice for a gatekeeper outside their current verification scope survived every open that could not check it: a "use" collaborator who opens while a connection is unbound from every gadget verifies nothing against it, yet their stale entry stays -- and the moment the connection is rebound (rebinding keeps the same gatekeeper id) the next open silently re-registers them off a choice made for a scope the workspace no longer has, instead of asking them again. Step 2 now drops every account choice for a gatekeeper outside the collaborator's live verification scope, even when the remaining scope is empty (that is exactly the everything-unbound open). The gatekeeper-side registration is deliberately kept: it preserves forward exclusion via `byObserverId`, and the next successful open's `addObserver` overwrites the verifier. Co-Authored-By: Claude Opus 5 --- .../__tests__/observer-scope-prune.test.ts | 169 ++++++++++++++++++ packages/workshop-backend/src/overseer.ts | 27 ++- 2 files changed, 191 insertions(+), 5 deletions(-) create mode 100644 packages/workshop-backend/__tests__/observer-scope-prune.test.ts diff --git a/packages/workshop-backend/__tests__/observer-scope-prune.test.ts b/packages/workshop-backend/__tests__/observer-scope-prune.test.ts new file mode 100644 index 000000000..757b01aa7 --- /dev/null +++ b/packages/workshop-backend/__tests__/observer-scope-prune.test.ts @@ -0,0 +1,169 @@ +// ensureObserver must prune out-of-scope account choices from the observer record at every open, +// keeping the record an accurate statement of what this collaborator's most recent open verified. +// Rebinding a connection keeps the same gatekeeper id, so a stale entry left from before an unbind +// would otherwise silently re-register them off an account choice made for a scope the workspace +// no longer has, instead of asking them again. +// +// Runs against a real OverseerDurableObject (the TEST_OVERSEER binding, like +// git-migration-do.test.ts); the gatekeeper facet and the client's User DO are the only fakes. + +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; + } +} + +function seedGatekeepers(impl: any): void { + for (let id of [1, 2]) { + impl.storage.gatekeepers.put({ + id, + resourceTitle: `Connection ${id}`, + class: {} as any, + creationSpec: { + type: "gatekeeper", + vendorId: "testvendor", + resourceUrl: `https://example.com/${id}`, + typeUrlPattern: "https://*", + }, + }); + } +} + +// A gadget that binds only gatekeeper 1, leaving gatekeeper 2 out of "use" scope. +function seedGadgetBindingGk1(impl: any): void { + impl.storage.gadgets.put({ + id: 100, + title: "G", + created: new Date(), + bindingName: "G", + bindings: { DB: { target: 1 } }, + }); +} + +// A client User DO that always has the account and always mints a verifier. +const fakeClientUser = { + getVerifier: async () => ({}), +} as any; + +describe("ensureObserver out-of-scope coverage pruning", () => { + it("prunes an unbound gatekeeper's entry at a use-role open", async () => { + let stub = env.TEST_OVERSEER.getByName("observer-scope-prune-use"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = (instance as unknown as { impl: any }).impl; + seedGatekeepers(impl); + seedGadgetBindingGk1(impl); + impl.ownerProfileId = "owner"; + impl.storage.observers.put( + { profileId: "alice", observerId: "obs-1", accountChoices: { 1: 10, 2: 20 } }); + + let verified: number[] = []; + impl.getGatekeeperFacet = (id: number) => ({ + addObserver: async () => { verified.push(id); }, + }); + + await impl.ensureObserver("alice", fakeClientUser, "use"); + + // Gatekeeper 2 is outside "use" scope: its stale entry is gone, and nothing re-verified it. + expect(verified).toEqual([1]); + expect(impl.storage.observers.get("alice").accountChoices).toEqual({ 1: 10 }); + }); + }); + + it("prunes everything at an empty-scope open, keeping the record", async () => { + let stub = env.TEST_OVERSEER.getByName("observer-scope-prune-empty"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = (instance as unknown as { impl: any }).impl; + seedGatekeepers(impl); + // No gadgets at all: a "use" collaborator's verification scope is empty. + impl.ownerProfileId = "owner"; + impl.storage.observers.put( + { profileId: "alice", observerId: "obs-1", accountChoices: { 1: 10, 2: 20 } }); + + // No configureCb: the open must still resolve (nothing in scope to configure), and it must + // still prune -- this is exactly the everything-unbound open the fix exists for. + await impl.ensureObserver("alice", fakeClientUser, "use"); + + let record = impl.storage.observers.get("alice"); + expect(record).toBeDefined(); + expect(record.accountChoices).toEqual({}); + }); + }); + + it("keeps unbound gatekeepers' entries at a build-role open", async () => { + let stub = env.TEST_OVERSEER.getByName("observer-scope-prune-build"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = (instance as unknown as { impl: any }).impl; + seedGatekeepers(impl); + seedGadgetBindingGk1(impl); + impl.ownerProfileId = "owner"; + impl.storage.observers.put( + { profileId: "alice", observerId: "obs-1", accountChoices: { 1: 10, 2: 20 } }); + + let verified: number[] = []; + impl.getGatekeeperFacet = (id: number) => ({ + addObserver: async () => { verified.push(id); }, + }); + + // "build" scope is every account-requiring gatekeeper regardless of gadget bindings, so + // both entries are in scope and nothing may be pruned (guards against over-pruning). + await impl.ensureObserver("alice", fakeClientUser, "build"); + + expect(verified.toSorted()).toEqual([1, 2]); + expect(impl.storage.observers.get("alice").accountChoices).toEqual({ 1: 10, 2: 20 }); + }); + }); + + it("makes the re-open after a rebind re-verify the pruned producer", async () => { + let stub = env.TEST_OVERSEER.getByName("observer-scope-prune-rebind"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = (instance as unknown as { impl: any }).impl; + seedGatekeepers(impl); + seedGadgetBindingGk1(impl); + impl.ownerProfileId = "owner"; + // Alice is a "use" collaborator with stale coverage for gatekeeper 2, left over from before + // it was unbound from every gadget. + impl.storage.collaborators.put({ + profile: { type: "user", id: "alice", name: "Alice" }, + addedBy: [{ type: "user", sharer: "owner", created: new Date(), role: "use" }], + }); + impl.storage.observers.put( + { profileId: "alice", observerId: "obs-1", accountChoices: { 1: 10, 2: 20 } }); + + let verified: number[] = []; + impl.getGatekeeperFacet = (id: number) => ({ + addObserver: async () => { verified.push(id); }, + }); + + // Alice opens during the unbound window: gatekeeper 2 is out of her scope, so this open + // verifies nothing against it -- and prunes her stale entry. + await impl.ensureObserver("alice", fakeClientUser, "use"); + + expect(verified).toEqual([1]); + expect(impl.storage.observers.get("alice").accountChoices).toEqual({ 1: 10 }); + + // Rebind gatekeeper 2 (same gatekeeper id -- only the gadget's binding edges change). + let gadget = impl.storage.gadgets.get(100); + gadget.bindings.DB2 = { target: 2 }; + impl.storage.gadgets.put(gadget); + + // Her next open is where gatekeeper 2 gets verified again -- and since the prune left no + // entry to reuse, she is asked to choose an account for it rather than being re-registered + // off the choice she made before it was unbound. + let asked: number[] = []; + let configureCb = { configure: async (needs: { gatekeeperId: number }[]) => { + asked.push(...needs.map(need => need.gatekeeperId)); + return needs.map(need => ({ gatekeeperId: need.gatekeeperId, accountId: 30 })); + } } as any; + await impl.ensureObserver("alice", fakeClientUser, "use", configureCb); + + expect(asked).toEqual([2]); + expect(verified.toSorted()).toEqual([1, 1, 2]); + expect(impl.storage.observers.get("alice").accountChoices).toEqual({ 1: 10, 2: 30 }); + }); + }); +}); diff --git a/packages/workshop-backend/src/overseer.ts b/packages/workshop-backend/src/overseer.ts index 39a350454..1d0f98cd8 100644 --- a/packages/workshop-backend/src/overseer.ts +++ b/packages/workshop-backend/src/overseer.ts @@ -7861,14 +7861,31 @@ class OverseerImpl implements AgentHooks { clientUser: DurableObjectStub, role: CollaboratorRole, configureCb?: RpcStub): Promise { - // 1. Select in-scope gatekeepers. If none require an account, there is nothing to verify and - // no observer record is needed (built-in gatekeepers never name observers in - // excludeObservers). + // 1. Select in-scope gatekeepers. If none require an account, there is nothing to verify + // (built-in gatekeepers never name observers in excludeObservers). let inScope = this.#inScopeGatekeepers(role); - if (inScope.length === 0) return; - // 2. Load any existing observer record, and build a working copy of its account choices. + // 2. Load any existing observer record, and prune every account choice for a gatekeeper now + // outside this collaborator's verification scope, keeping the record an accurate statement + // of what their most recent open verified: entry present => verified at that open. + // Rebinding a connection keeps its gatekeeper id, so a stale entry from before an unbind + // would otherwise re-register them off a choice made for a scope the workspace no longer + // has, instead of asking them again. let record = this.storage.observers.get(profileId); + if (record) { + let inScopeIds = new Set(inScope.map(gk => gk.id)); + let pruned = false; + for (let key of Object.keys(record.accountChoices)) { + if (!inScopeIds.has(Number(key))) { + delete record.accountChoices[Number(key)]; + pruned = true; + } + } + if (pruned) this.storage.observers.put(record); + } + if (inScope.length === 0) return; + + // Build a working copy of the (pruned) account choices. let accountChoices: {[gatekeeperId: number]: number} = {...record?.accountChoices}; // Gatekeeper ids registered before this call (their account choice came from the persisted From 01e1228493d111f9401e7ee070ae9fb177f912aa Mon Sep 17 00:00:00 2001 From: Maximo Guk <62088388+Maximo-Guk@users.noreply.github.com> Date: Fri, 28 Aug 2026 14:04:57 -0500 Subject: [PATCH 03/14] Bugfix: Keep a returning observer's registrations when re-verification fails. `ensureObserver`'s rollback removed the gatekeeper registrations of every binding that failed the call (`invalidated`), not just the ones the call created (`newlyAdded`). For a collaborator who was already an admitted observer, that de-registered them from a gatekeeper they had previously been verified against -- and a de-registered observer is one the gatekeeper stops naming in `ObservationDescription.excludeObservers`, so an observation it would have excluded them from is admitted with nothing left to block it. The coverage scrub this rollback accompanies is not a substitute for the registration. They cover different sets: `gatekeeper-confluence` -- the only in-repo producer of `excludeObservers` -- never marks an observation `prohibitAllSharing`, so for it the scrub covers none of the affected reads. The reachable sequence is a collaborator whose Confluence access is revoked upstream, whose re-open therefore fails, and whose pre-existing live session then watches the owner's agent read a page they cannot access. So roll back `invalidated` only on a first-ever verification, where the minted observerId is discarded along with the unpersisted record and a registration left behind would linger unresolvable. A returning observer's id is already persisted, so keeping their registrations is fail-closed (a registration can only add exclusion names) and the next successful open's `addObserver` overwrites the verifier. This restores the invariant `registeredBeforeCall` was introduced to state: roll back only what this call added. Coverage is still scrubbed either way, so a revoked collaborator's record stops claiming they were verified. Co-Authored-By: Claude Opus 5 --- .../__tests__/observer-coverage-scrub.test.ts | 72 ++++++++++++++++++- packages/workshop-backend/src/overseer.ts | 24 ++++--- 2 files changed, 84 insertions(+), 12 deletions(-) diff --git a/packages/workshop-backend/__tests__/observer-coverage-scrub.test.ts b/packages/workshop-backend/__tests__/observer-coverage-scrub.test.ts index 21fa93781..1d5f18747 100644 --- a/packages/workshop-backend/__tests__/observer-coverage-scrub.test.ts +++ b/packages/workshop-backend/__tests__/observer-coverage-scrub.test.ts @@ -105,12 +105,78 @@ describe("observer coverage scrub on a failed live check", () => { // The rejection went through fail(): gatekeeper 1's persisted coverage is scrubbed -- so // the record no longer claims this collaborator was verified for it -- while gatekeeper 2's - // survives. The invalidated registration is also rolled back (see the TODO in - // ensureObserver's catch about keeping re-asserted registrations for admitted observers). + // survives. let record = impl.storage.observers.get("alice"); expect(1 in record.accountChoices).toBe(false); expect(record.accountChoices[2]).toBe(20); - expect(removed).toEqual([1]); + + // Alice was already an admitted observer, so the failure de-registers her from nothing: the + // registrations are what make gatekeepers name her in `excludeObservers`, and the scrub does + // not cover the same observations (it gates `prohibitAllSharing` only). + expect(removed).toEqual([]); + }); + }); + + it("keeps a returning observer's registration so forward exclusion survives the failure", + async () => { + let stub = env.TEST_OVERSEER.getByName("observer-coverage-scrub-keeps-registration"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = (instance as unknown as { impl: any }).impl; + seedGatekeepers(impl); + // Alice's previous open covered gatekeeper 1 only; gatekeeper 2 is a binding added since, + // which she has never been verified against. + impl.storage.observers.put( + { profileId: "alice", observerId: "obs-1", accountChoices: { 1: 10 } }); + + let removed: number[] = []; + impl.getGatekeeperFacet = (id: number) => ({ + // Gatekeeper 1 has revoked her access upstream: the binding she *was* admitted for is the + // one that now refuses, which is exactly the case that used to drop her registration. + addObserver: async () => { if (id === 1) throw new Error("access revoked upstream"); }, + removeObserver: async () => { removed.push(id); }, + }); + + let configureCb = { configure: async (needs: {gatekeeperId: number}[]) => + needs.map(need => ({ gatekeeperId: need.gatekeeperId, accountId: 20 })) } as any; + + await expect(impl.ensureObserver("alice", fakeClientUser, "build", configureCb)) + .rejects.toThrow(/could not confirm/); + + // The two registrations are treated differently, which is the whole point. Gatekeeper 1's + // predates this call, so it survives and keeps naming her in `excludeObservers`. Gatekeeper + // 2's was created by this call, so rolling it back merely restores the pre-call state -- + // there was no prior registration whose exclusions could be lost. + expect(removed).toEqual([2]); + // Coverage is still scrubbed regardless, so her next open must re-verify gatekeeper 1. + expect(1 in impl.storage.observers.get("alice").accountChoices).toBe(false); + }); + }); + + it("a first-ever verification failure still rolls its registrations back", async () => { + let stub = env.TEST_OVERSEER.getByName("observer-coverage-scrub-first-ever"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = (instance as unknown as { impl: any }).impl; + seedGatekeepers(impl); + // No observer record: Alice has never been admitted, so the observerId minted for this call + // is discarded with the unpersisted record and anything registered under it would linger + // unresolvable. + let removed: number[] = []; + impl.getGatekeeperFacet = (id: number) => ({ + addObserver: async () => { if (id === 2) throw new Error("no access"); }, + removeObserver: async () => { removed.push(id); }, + }); + + let configureCb = { configure: async (needs: {gatekeeperId: number}[]) => + needs.map(need => ({ gatekeeperId: need.gatekeeperId, accountId: need.gatekeeperId * 10 })) + } as any; + + await expect(impl.ensureObserver("alice", fakeClientUser, "build", configureCb)) + .rejects.toThrow(/could not confirm/); + + // Both the one that verified and the one that refused are rolled back, and no record is + // persisted. + expect(removed.toSorted()).toEqual([1, 2]); + expect(impl.storage.observers.get("alice")).toBeUndefined(); }); }); }); diff --git a/packages/workshop-backend/src/overseer.ts b/packages/workshop-backend/src/overseer.ts index 1d0f98cd8..40dd079d8 100644 --- a/packages/workshop-backend/src/overseer.ts +++ b/packages/workshop-backend/src/overseer.ts @@ -7895,6 +7895,10 @@ class OverseerImpl implements AgentHooks { inScope.filter(gk => gk.id in accountChoices).map(gk => gk.id)); let observerId = record?.observerId ?? crypto.randomUUID(); + // Whether this collaborator was already an admitted observer when the call began. A returning + // observer's `observerId` is already persisted, so it stays resolvable no matter how this call + // ends -- which is what makes keeping their registrations on a failure safe (see the catch). + let returningObserver = record !== undefined; // Gatekeepers we successfully registered the observer with during this call. let newlyAdded = new Set(); // Gatekeepers that refused (or whose account was gone) during this call and have not verified @@ -8059,16 +8063,18 @@ class OverseerImpl implements AgentHooks { break; } } catch (err) { - // Best-effort remove all the observers that were newly-added since we didn't persist the - // user's observer record -- and the invalidated ones, whose coverage fail() just scrubbed: - // their registrations reference a choice that is no longer persisted anywhere. + // Best-effort remove the observers we registered during *this* call, since we didn't persist + // the user's observer record. // - // TODO: For a *returning* collaborator whose re-verification - // failed, this rollback removes registrations that preserve forward exclusion -- once - // de-registered, gatekeepers stop naming the observer in excludeObservers -- so only a - // first-ever verification should roll back fully. - await this.#removeObserverFromGatekeepers( - observerId, [...new Set([...newlyAdded, ...invalidated])]); + // A first-ever verification rolls back the invalidated ones too: nothing referenced those + // registrations before this call, and the freshly-minted observerId is discarded with the + // unpersisted record, so anything left behind lingers unresolvable. + // + // A *returning* observer's registrations are kept instead. De-registering one is fail-open: + // the gatekeeper stops naming that observer in `excludeObservers`, so an observation it + // would have excluded them from is admitted with nothing left to block it. + let rollback = returningObserver ? newlyAdded : new Set([...newlyAdded, ...invalidated]); + await this.#removeObserverFromGatekeepers(observerId, [...rollback]); throw err; } From 0ae8f0ca558ec73bedf273048e9f3c7e155d14fb Mon Sep 17 00:00:00 2001 From: Maximo Guk <62088388+Maximo-Guk@users.noreply.github.com> Date: Fri, 28 Aug 2026 14:16:07 -0500 Subject: [PATCH 04/14] Bugfix: Restart sessions when a collaborator's verification scope widens. Authorization and observer verification run only at open(). Nothing re-ran them when the set of gatekeepers a collaborator must be verified against *grew* mid-session -- adding a connection, or binding one into a gadget -- so a collaborator who opened before the growth kept a live session holding access they were never verified for. Fix it with the mechanism already used to revoke a collaborator: generalize scheduleRevocationRestart() to scheduleAccessRestart(reason) and add #restartIfShared(), which flushes, waits 100ms and aborts the DO so every client reconnects and re-opens against the new scope. It is a no-op when the workspace has no collaborators, so a solo workspace is never disturbed. Four sites widen scope and now restart: addGatekeeper, a permanent bindWorkpiece, a merge that promotes a binding edge into "use" scope, and a denied re-verification that scrubbed a persisted account choice. The merge case compares the effective account-requiring "use" scope before and after promotion rather than restarting on any promotion, since most merges promote neither a gadget with bindings nor an edge to a connection anyone is verified against. It reads the scope through the non-throwing gatekeeperVendorId() rather than #inScopeGatekeepers("use"), whose observerVendorId() throws on a legacy record with no creationSpec -- an unrelated legacy connection must not turn an accepted merge into an error. Co-Authored-By: Claude Opus 5 --- .../__tests__/observer-coverage-scrub.test.ts | 28 +- .../__tests__/observer-scope-prune.test.ts | 20 +- .../__tests__/observer-scope-restart.test.ts | 248 ++++++++++++++++++ packages/workshop-backend/src/overseer.ts | 169 +++++++++--- 4 files changed, 420 insertions(+), 45 deletions(-) create mode 100644 packages/workshop-backend/__tests__/observer-scope-restart.test.ts diff --git a/packages/workshop-backend/__tests__/observer-coverage-scrub.test.ts b/packages/workshop-backend/__tests__/observer-coverage-scrub.test.ts index 1d5f18747..8cac5d5b0 100644 --- a/packages/workshop-backend/__tests__/observer-coverage-scrub.test.ts +++ b/packages/workshop-backend/__tests__/observer-coverage-scrub.test.ts @@ -1,10 +1,12 @@ // A failed live check (a gatekeeper's addObserver refusing, or the verifier failing to resolve) // must scrub that gatekeeper from the collaborator's *persisted* observer record synchronously // with the failure determination: the record is the standing claim that this collaborator was -// verified for that producer, and this open is not going to renew it. +// verified for that producer, and this open is not going to renew it. Because the claim is what +// admitted them, the shrink also severs their still-live sessions (see observer-scope-restart). // // Runs against a real OverseerDurableObject (the TEST_OVERSEER binding, like -// git-migration-do.test.ts); the gatekeeper facet and the client's User DO are the only fakes. +// git-migration-do.test.ts); the gatekeeper facet, the client's User DO, and the restart are the +// only fakes -- a real ctx.abort() would kill the test DO. import { describe, expect, it } from "vitest"; import { env } from "cloudflare:workers"; @@ -17,6 +19,15 @@ declare module "cloudflare:workers" { } } +// Seed the owner profile id (so the sharing manager needs no User DO round trip) and record the +// restart a coverage shrink schedules instead of performing it. +function recordRestarts(impl: any): string[] { + impl.ownerProfileId = "owner"; + let restarts: string[] = []; + impl.scheduleAccessRestart = async (reason: string) => { restarts.push(reason); }; + return restarts; +} + function seedGatekeepers(impl: any): void { for (let id of [1, 2]) { impl.storage.gatekeepers.put({ @@ -40,12 +51,13 @@ const fakeClientUser = { } as any; describe("observer coverage scrub on a failed live check", () => { - it("a refused re-verification drops the entry", async () => { + it("a refused re-verification drops the entry and severs the collaborator's sessions", + async () => { let stub = env.TEST_OVERSEER.getByName("observer-coverage-scrub-refused"); await runInDurableObject(stub, async (instance: OverseerDurableObject) => { let impl = (instance as unknown as { impl: any }).impl; seedGatekeepers(impl); - impl.ownerProfileId = "owner"; + let restarts = recordRestarts(impl); // Alice is a reachable collaborator whose previous successful open left coverage for both // gatekeepers. impl.storage.collaborators.put({ @@ -70,6 +82,11 @@ describe("observer coverage scrub on a failed live check", () => { let record = impl.storage.observers.get("alice"); expect(1 in record.accountChoices).toBe(false); expect(record.accountChoices[2]).toBe(20); + + // The point of the scrub: alice's coverage shrank, so every live session is severed and + // must re-open against what the record now claims. + await new Promise(resolve => setTimeout(resolve, 0)); + expect(restarts).toHaveLength(1); }); }); @@ -78,6 +95,7 @@ describe("observer coverage scrub on a failed live check", () => { await runInDurableObject(stub, async (instance: OverseerDurableObject) => { let impl = (instance as unknown as { impl: any }).impl; seedGatekeepers(impl); + recordRestarts(impl); // Already-configured coverage for both gatekeepers, as a previous successful open left it. impl.storage.observers.put( { profileId: "alice", observerId: "obs-1", accountChoices: { 1: 10, 2: 20 } }); @@ -123,6 +141,7 @@ describe("observer coverage scrub on a failed live check", () => { await runInDurableObject(stub, async (instance: OverseerDurableObject) => { let impl = (instance as unknown as { impl: any }).impl; seedGatekeepers(impl); + recordRestarts(impl); // Alice's previous open covered gatekeeper 1 only; gatekeeper 2 is a binding added since, // which she has never been verified against. impl.storage.observers.put( @@ -157,6 +176,7 @@ describe("observer coverage scrub on a failed live check", () => { await runInDurableObject(stub, async (instance: OverseerDurableObject) => { let impl = (instance as unknown as { impl: any }).impl; seedGatekeepers(impl); + recordRestarts(impl); // No observer record: Alice has never been admitted, so the observerId minted for this call // is discarded with the unpersisted record and anything registered under it would linger // unresolvable. diff --git a/packages/workshop-backend/__tests__/observer-scope-prune.test.ts b/packages/workshop-backend/__tests__/observer-scope-prune.test.ts index 757b01aa7..317cd67fb 100644 --- a/packages/workshop-backend/__tests__/observer-scope-prune.test.ts +++ b/packages/workshop-backend/__tests__/observer-scope-prune.test.ts @@ -5,7 +5,8 @@ // no longer has, instead of asking them again. // // Runs against a real OverseerDurableObject (the TEST_OVERSEER binding, like -// git-migration-do.test.ts); the gatekeeper facet and the client's User DO are the only fakes. +// git-migration-do.test.ts); the gatekeeper facet, the client's User DO, and the restart are the +// only fakes -- a real ctx.abort() would kill the test DO. import { describe, expect, it } from "vitest"; import { env } from "cloudflare:workers"; @@ -118,13 +119,15 @@ describe("ensureObserver out-of-scope coverage pruning", () => { }); }); - it("makes the re-open after a rebind re-verify the pruned producer", async () => { + it("restarts on a rebind, and the re-open re-verifies the pruned producer", async () => { let stub = env.TEST_OVERSEER.getByName("observer-scope-prune-rebind"); await runInDurableObject(stub, async (instance: OverseerDurableObject) => { let impl = (instance as unknown as { impl: any }).impl; seedGatekeepers(impl); seedGadgetBindingGk1(impl); impl.ownerProfileId = "owner"; + let restarts: string[] = []; + impl.scheduleAccessRestart = async (reason: string) => { restarts.push(reason); }; // Alice is a "use" collaborator with stale coverage for gatekeeper 2, left over from before // it was unbound from every gadget. impl.storage.collaborators.put({ @@ -146,13 +149,14 @@ describe("ensureObserver out-of-scope coverage pruning", () => { expect(verified).toEqual([1]); expect(impl.storage.observers.get("alice").accountChoices).toEqual({ 1: 10 }); - // Rebind gatekeeper 2 (same gatekeeper id -- only the gadget's binding edges change). - let gadget = impl.storage.gadgets.get(100); - gadget.bindings.DB2 = { target: 2 }; - impl.storage.gadgets.put(gadget); + // Rebind gatekeeper 2 (same gatekeeper id -- only the gadget's binding edges change). That + // widens every "use" collaborator's scope, so it severs Alice's live session. + impl.bindWorkpiece(100, "DB2", 2); + await new Promise(resolve => setTimeout(resolve, 0)); + expect(restarts).toHaveLength(1); - // Her next open is where gatekeeper 2 gets verified again -- and since the prune left no - // entry to reuse, she is asked to choose an account for it rather than being re-registered + // Her forced re-open is where gatekeeper 2 gets verified again -- and since the prune left + // no entry to reuse, she is asked to choose an account for it rather than being re-registered // off the choice she made before it was unbound. let asked: number[] = []; let configureCb = { configure: async (needs: { gatekeeperId: number }[]) => { diff --git a/packages/workshop-backend/__tests__/observer-scope-restart.test.ts b/packages/workshop-backend/__tests__/observer-scope-restart.test.ts new file mode 100644 index 000000000..422549b08 --- /dev/null +++ b/packages/workshop-backend/__tests__/observer-scope-restart.test.ts @@ -0,0 +1,248 @@ +// Authorization and observer verification run only at open(), so widening what a collaborator must +// be verified against would otherwise leave their live session holding access nobody checked. Each +// widening restarts the workspace (scheduleAccessRestart), forcing every client to re-open and +// re-verify against the new scope -- and a workspace with no collaborators is never disturbed, +// since the owner is never an observer. +// +// Runs against a real OverseerDurableObject (the TEST_OVERSEER binding, like +// observer-coverage-scrub.test.ts). scheduleAccessRestart is replaced with a recorder: a real +// ctx.abort() would kill the test DO. + +import { describe, expect, it } from "vitest"; +import { env } from "cloudflare:workers"; +import { runInDurableObject } from "cloudflare:test"; +import type { AiChatAuthorInfo } from "@gadgets/workshop-shared/api"; +import type { OverseerDurableObject } from "../src/overseer.js"; + +declare module "cloudflare:workers" { + interface ProvidedEnv { + TEST_OVERSEER: DurableObjectNamespace; + } +} + +const OWNER = "owner"; +const AGENT: AiChatAuthorInfo = { type: "agent", id: "some-model", name: "Agent" }; +const USER_META = { profile: { type: "user", id: OWNER, name: "Owner" } as AiChatAuthorInfo }; + +// #restartIfShared is fire-and-forget over an async getSharingManager(), so let its continuation +// run before asserting. +const settle = () => new Promise(resolve => setTimeout(resolve, 0)); + +let doCounter = 0; + +async function withImpl(fn: (impl: any, restarts: string[]) => Promise): Promise { + let stub = env.TEST_OVERSEER.getByName(`observer-scope-restart-${++doCounter}`); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = (instance as unknown as { impl: any }).impl; + // Seed the cached owner profile id so the sharing manager needs no User DO round trip. + impl.ownerProfileId = OWNER; + let restarts: string[] = []; + impl.scheduleAccessRestart = async (reason: string) => { restarts.push(reason); }; + await fn(impl, restarts); + }); +} + +function addCollaborator(impl: any, role: "build" | "use" = "build"): void { + impl.storage.collaborators.put({ + profile: { type: "user", id: "alice", name: "Alice" }, + addedBy: [{ type: "user", sharer: OWNER, created: new Date(), role }], + }); +} + +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://*", + }, + }); +} + +// A vendorless connection (an AI model), which no collaborator is ever verified against. +function seedVendorlessGatekeeper(impl: any, id: number): void { + impl.storage.gatekeepers.put({ + id, + resourceTitle: `Model ${id}`, + class: {} as any, + creationSpec: { + type: "aiModel", modelId: `m${id}`, provider: "anthropic", modelName: "claude", + }, + }); +} + +function seedGadget(impl: any, id: number): void { + impl.storage.gadgets.put( + { id, title: "G", created: new Date(0), bindingName: "G", bindings: {} }); +} + +// A facet that lets addGatekeeper's describe() succeed. +function stubFacets(impl: any): void { + impl.getGatekeeperFacet = () => ({ + describe: async () => ({ title: "Test", url: "https://example.com/new" }), + }); +} + +const CONNECTION_SPEC = { + type: "gatekeeper" as const, + vendorId: "testvendor", + resourceUrl: "https://example.com/new", + typeUrlPattern: "https://*", +}; + +describe("restarting sessions when verification scope widens", () => { + it("adding a connection restarts a shared workspace", () => withImpl(async (impl, restarts) => { + addCollaborator(impl); + stubFacets(impl); + + // A new account-requiring connection is immediately in every "build" collaborator's scope, + // and their live session was never verified against it. + await impl.addGatekeeper({} as any, CONNECTION_SPEC); + await settle(); + + expect(restarts).toHaveLength(1); + })); + + it("adding a connection to a solo workspace disturbs nobody", + () => withImpl(async (impl, restarts) => { + stubFacets(impl); + + // The owner is never an observer, so there is nobody to re-verify -- and the one session that + // exists is the one that asked for the connection. + await impl.addGatekeeper({} as any, CONNECTION_SPEC); + await settle(); + + expect(restarts).toEqual([]); + })); + + it("adding a vendorless connection widens nothing", () => withImpl(async (impl, restarts) => { + addCollaborator(impl); + stubFacets(impl); + + // #inScopeGatekeepers skips a spec with no vendorId, so no collaborator is ever verified + // against an AI model binding and adding one cannot leave anyone under-verified. + await impl.addGatekeeper({} as any, { + type: "aiModel", modelId: "m", provider: "anthropic", modelName: "claude", + }); + await settle(); + + expect(restarts).toEqual([]); + })); + + it("binding a connection into a gadget restarts a shared workspace", + () => withImpl(async (impl, restarts) => { + addCollaborator(impl, "use"); + seedGatekeeper(impl, 1); + seedGadget(impl, 100); + + // A permanent edge puts the connection into "use" scope: the gadget UI the collaborator drives + // can now invoke it. + impl.bindWorkpiece(100, "DB", 1); + await settle(); + + expect(restarts).toHaveLength(1); + })); + + it("a pending bind is invisible to collaborators, so it restarts nothing", + () => withImpl(async (impl, restarts) => { + addCollaborator(impl, "use"); + seedGatekeeper(impl, 1); + seedGadget(impl, 100); + + // An edge provisional to a chat isn't in #gadgetBoundGatekeeperIds until it's promoted, which + // is what restarts (see the merge case below). + impl.bindWorkpiece(100, "DB", 1, 7); + await settle(); + + expect(restarts).toEqual([]); + })); + + it("promoting a pending bind at merge restarts a shared workspace", + () => withImpl(async (impl, restarts) => { + addCollaborator(impl, "use"); + seedGatekeeper(impl, 1); + seedGadget(impl, 100); + impl.storage.chatMeta.put( + { id: 1, title: "Chat", started: new Date(0), lastActive: new Date(0) }); + + impl.bindWorkpiece(100, "DB", 1, 1); + await impl.commitAgentStep(1, AGENT, [{ type: "message", message: "bound a connection" }], { + changes: [], + createdGadgets: [], + addedBindings: [{ gadgetId: 100, name: "DB", target: 1 }], + }); + await settle(); + expect(restarts).toEqual([]); + + expect(await impl.mergeChanges(1, USER_META, "owner-user-do")) + .toEqual({ outcome: "merged" }); + await settle(); + + // Accepting the change is the moment the edge becomes visible to "use" collaborators. + expect(impl.storage.gadgets.get(100).bindings.DB.pending).toBeUndefined(); + expect(restarts).toHaveLength(1); + })); + + it("a merge that promotes only a vendorless edge restarts nothing", + () => withImpl(async (impl, restarts) => { + addCollaborator(impl, "use"); + seedVendorlessGatekeeper(impl, 1); + seedGadget(impl, 100); + impl.storage.chatMeta.put( + { id: 1, title: "Chat", started: new Date(0), lastActive: new Date(0) }); + + impl.bindWorkpiece(100, "MODEL", 1, 1); + await impl.commitAgentStep(1, AGENT, [{ type: "message", message: "bound a model" }], { + changes: [], + createdGadgets: [], + addedBindings: [{ gadgetId: 100, name: "MODEL", target: 1 }], + }); + + expect(await impl.mergeChanges(1, USER_META, "owner-user-do")) + .toEqual({ outcome: "merged" }); + await settle(); + + // The edge is promoted, but a vendorless connection is in nobody's verification scope, so the + // effective scope is unchanged and no collaborator's session is interrupted. (The trigger + // compares scopes rather than restarting on any promotion, which most merges are.) + expect(impl.storage.gadgets.get(100).bindings.MODEL.pending).toBeUndefined(); + expect(restarts).toEqual([]); + })); + + it("a failed re-verification severs the collaborator's other sessions", + () => withImpl(async (impl, restarts) => { + addCollaborator(impl); + seedGatekeeper(impl, 1); + // Alice's previous open left coverage that her still-live sessions rest on. + impl.storage.observers.put( + { profileId: "alice", observerId: "obs-1", accountChoices: { 1: 10 } }); + impl.getGatekeeperFacet = () => ({ + addObserver: async () => { throw new Error("access revoked upstream"); }, + removeObserver: async () => {}, + }); + let fakeClientUser = + { getVerifier: async () => ({}), describeConnectedAccount: async () => null } as any; + // Answers every prompt with the same account, so each attempt fails the same way. + let configureCb = { configure: async (needs: { gatekeeperId: number }[]) => + needs.map(need => ({ gatekeeperId: need.gatekeeperId, accountId: 10 })) } as any; + + await expect(impl.ensureObserver("alice", fakeClientUser, "build", configureCb)) + .rejects.toThrow(/could not confirm/); + await settle(); + + // The scrub only rewrites what the record claims; the restart is what reaches the sessions. + expect(1 in impl.storage.observers.get("alice").accountChoices).toBe(false); + expect(restarts).toHaveLength(1); + + // A second identical failure finds the entry already scrubbed, so nothing shrank and nothing + // restarts: the trigger cannot loop against a collaborator who simply keeps failing. + await expect(impl.ensureObserver("alice", fakeClientUser, "build", configureCb)) + .rejects.toThrow(/could not confirm/); + await settle(); + expect(restarts).toHaveLength(1); + })); +}); diff --git a/packages/workshop-backend/src/overseer.ts b/packages/workshop-backend/src/overseer.ts index 40dd079d8..832d1e9f2 100644 --- a/packages/workshop-backend/src/overseer.ts +++ b/packages/workshop-backend/src/overseer.ts @@ -2210,7 +2210,8 @@ class OverseerImpl implements AgentHooks { } throw new Error(`There is already a binding named "${name}".`); } - if (!this.storage.gatekeepers.get(target)) { + let targetRecord = this.storage.gatekeepers.get(target); + if (!targetRecord) { if (this.storage.gadgets.get(target)) { throw new Error(`Gadget-to-gadget bindings are not supported yet.`); } @@ -2221,6 +2222,14 @@ class OverseerImpl implements AgentHooks { // The gadget's env changed, so its code must reload. this.bumpVersion([gadgetId]); + + // A permanent edge puts an account-requiring connection into every "use" collaborator's + // verification scope (#gadgetBoundGatekeeperIds), since the gadget UI they drive can now + // invoke it. A pending edge is invisible to them until it's promoted, which restarts then. + if (chatId === undefined && targetRecord.creationSpec && + "vendorId" in targetRecord.creationSpec) { + this.#restartIfShared("Gadget restarted because a connection was bound to a gadget."); + } } // Remove the named binding edge from the gadget. The target gatekeeper itself survives, @@ -3644,6 +3653,12 @@ class OverseerImpl implements AgentHooks { throw new Error("The chat's code is being actively edited; please retry."); } + // Promotion below can widen every "use" collaborator's verification scope, so snapshot the + // scope first and compare after. Comparing the effective scope rather than restarting on any + // promotion matters because most merges promote neither: a gadget with no bindings, or an edge + // to a vendorless connection, is in nobody's verification scope. + let useScopeBefore = this.#accountRequiringUseScope(); + // Promote provisional gadgets whose creation is covered by this merge: accepting the chat's // changes through `mergeThrough` makes them permanent workspace members. Each covered // creation sits on an unmerged, unreverted "changes" message at `pending.sequence` (a @@ -3676,6 +3691,10 @@ class OverseerImpl implements AgentHooks { } } + // Did the promotions above actually bring an account-requiring connection into "use" scope? + let widenedUseScope = + [...this.#accountRequiringUseScope()].some(id => !useScopeBefore.has(id)); + // Fast-forward each committed gadget's head. for (let {gadgetId, commitId} of commits) { let record = this.storage.gadgets.get(gadgetId)!; @@ -3764,6 +3783,13 @@ class OverseerImpl implements AgentHooks { interaction_type: "code_merged", }); + // Sever live sessions whose verification scope the promotions widened, now that the writes + // above have landed: a "use" collaborator's session was admitted against the narrower scope, + // and the gadget UI they drive can now invoke a connection nobody verified them against. + if (widenedUseScope) { + this.#restartIfShared("Gadget restarted because accepted changes added gadget bindings."); + } + return {outcome: "merged"}; } @@ -4353,6 +4379,14 @@ class OverseerImpl implements AgentHooks { throw error; } + // A new account-requiring connection is in every "build" collaborator's verification scope + // immediately -- a live build session can getGatekeeperById() and openSession() on it with no + // observer check -- so sever those sessions. A vendorless spec (aiModel/agentSpawner) is in + // nobody's scope (#inScopeGatekeepers skips it), so it widens nothing. + if (creationSpec && "vendorId" in creationSpec) { + this.#restartIfShared("Gadget restarted because a new connection was added."); + } + return new GatekeeperClientImpl(this, id, facet); } @@ -4848,7 +4882,7 @@ class OverseerImpl implements AgentHooks { // User DO ids whose outputs index this workspace is keeping live, one token per open session. // // In memory, not persisted, which is what makes fanning out to collaborators safe: revoking - // access aborts the DO (see scheduleRevocationRestart()), so this is destroyed with the sessions + // access aborts the DO (see scheduleAccessRestart()), so this is destroyed with the sessions // it describes and can only be rebuilt by an open() that re-checks the permission graph. #connectedIndexes = new Map>(); @@ -4975,30 +5009,61 @@ class OverseerImpl implements AgentHooks { return codeVersion; } - // Force every client to disconnect and re-authenticate after a collaborator has been removed or - // downgraded, so that someone who just lost access can't keep using a session that's already - // open. Authorization is only checked at open() (see the sharing docs), so without this a stale - // session would survive until something else happened to disconnect it. + // Force every client to disconnect and re-authenticate, so that no session outlives a change to + // what its holder is entitled to. Both checks that gate a session run only at open() (see the + // sharing docs), so without this a stale session would survive until something else happened to + // disconnect it. Two kinds of change need it: + // - Access removed or downgraded (removeCollaborator, revokeShareLink, workspace deletion): + // someone who just lost access could keep using the session they already have. + // - Verification scope widened (see #restartIfShared): a collaborator's live session was + // verified against a smaller set of gatekeepers than the workspace now holds. // // We restart by aborting the whole DO. Aborting propagates to clients: the `notifyClosed` stub // handed to each session is disposed without being called, which AuthenticatedApiImpl detects // and reacts to by killing the browser WebSocket, forcing a reconnect that re-runs open() and - // re-checks the (now-changed) permission graph. Removing/downgrading collaborators is rare, so - // the disruption is acceptable -- and DOs restart unpredictably anyway, so reconnects need to - // be made as painless as possible regardless. + // re-checks the (now-changed) permission graph. These events are rare, so the disruption is + // acceptable -- and DOs restart unpredictably anyway, so reconnects need to be made as painless + // as possible regardless. // // Two precautions before the abort: - // - `ctx.abort()` does not respect the output gate, so we explicitly flush the severed edge to - // disk with `ctx.storage.sync()`. Otherwise a restart could come back with the change lost, - // leaving the removed user still authorized. + // - `ctx.abort()` does not respect the output gate, so we explicitly flush the triggering change + // to disk with `ctx.storage.sync()`. Otherwise a restart could come back with the change lost, + // leaving the removed user still authorized (or the widened scope unrecorded). By the same + // token, callers must schedule the restart *after* the write that triggered it, never before + // further writes in the same turn -- those would be racing the abort. // - We delay the abort briefly so the triggering RPC's response can reach the caller (typically // the owner, who is also connected and will be disconnected) before their connection drops. // Without the delay their own removeCollaborator()/revokeShareLink() call might reject with a // connection error even though it succeeded. - async scheduleRevocationRestart(): Promise { + async scheduleAccessRestart(reason: string): Promise { await this.ctx.storage.sync(); await scheduler.wait(100); - this.ctx.abort("Gadget restarted to revoke access for a removed collaborator."); + this.ctx.abort(reason); + } + + // Sessions are authorized and verified only at open(), so widening what a live session's holder + // must be verified against leaves that session holding unverified access. Restart everyone -- + // the same mechanism used when access is revoked -- so each client's next open() re-runs + // authorizeCollaborator/ensureObserver against the new scope. No-op when the workspace has no + // collaborators: the owner is never an observer, so there is nobody to re-verify and no reason + // to disturb the one session that exists. + // + // Fire-and-forget: the callers are synchronous (bindWorkpiece) or already past their last write, + // and getSharingManager() is async, so failures are logged rather than left as an unhandled + // rejection. Failing to restart is fail-open for the widened scope, hence the `error` level. + // + // Note that ensureAmbientCapsules() calls addGatekeeper() from inside open(), so on a shared + // workspace the first open after an ambient capsule appears bounces itself once; the capsule + // exists by then, so the client's retry is clean. + #restartIfShared(reason: string): void { + this.getSharingManager().then(sharing => { + if (sharing.listCollaborators().length === 0) return; + return this.scheduleAccessRestart(reason); + }).catch(error => { + this.logger.error("failed to restart sessions after verification scope widened", { + event: "workspace.scope.restart.failed", error, + }); + }); } // Last timestamp generated by getChatTimestamp(), if it has been called during this session. @@ -7757,23 +7822,41 @@ class OverseerImpl implements AgentHooks { } } + // Gatekeeper ids bound by some non-provisional gadget -- everything the gadget UI can invoke, + // and therefore all of a "use" collaborator's verification scope. + #gadgetBoundGatekeeperIds(): Set { + let boundIds = new Set(); + for (let gadget of this.storage.gadgets.list()) { + // Provisional gadgets and binding edges aren't visible to "use" collaborators, so they + // don't bring gatekeepers into scope. + if (gadget.pending) continue; + for (let [, edge] of this.visibleBindings(gadget)) { + boundIds.add(edge.target); + } + } + return boundIds; + } + + // The account-requiring subset of #gadgetBoundGatekeeperIds(): exactly what a "use" collaborator + // is verified against, as an id set two states can be compared by (see mergeChatChanges). + // + // Uses the non-throwing gatekeeperVendorId() rather than #inScopeGatekeepers("use"), whose + // observerVendorId() throws on a legacy record with no creationSpec: an unrelated legacy + // connection must not turn a caller's ordinary bookkeeping into an error. + #accountRequiringUseScope(): Set { + let ids = new Set(); + for (let id of this.#gadgetBoundGatekeeperIds()) { + if (gatekeeperVendorId(this.storage.gatekeepers.get(id))) ids.add(id); + } + return ids; + } + // Selects the gatekeepers a non-owner observer with the given `role` must be verified against: // - "build" collaborators (full access): every account-requiring gatekeeper. // - "use" collaborators (UI only): only account-requiring gatekeepers bound by some gadget, // since that is all the UI can invoke. #inScopeGatekeepers(role: CollaboratorRole): GatekeeperRecord[] { - let boundIds: Set | undefined; - if (role === "use") { - boundIds = new Set(); - for (let gadget of this.storage.gadgets.list()) { - // Provisional gadgets and binding edges aren't visible to "use" collaborators, so they - // don't bring gatekeepers into scope. - if (gadget.pending) continue; - for (let [, edge] of this.visibleBindings(gadget)) { - boundIds.add(edge.target); - } - } - } + let boundIds = role === "use" ? this.#gadgetBoundGatekeeperIds() : undefined; let result: GatekeeperRecord[] = []; for (let gk of this.storage.gatekeepers.list()) { @@ -7790,8 +7873,8 @@ class OverseerImpl implements AgentHooks { // Best-effort `removeObserver(observerId)` across the given gatekeeper ids. Never throws; logs // and continues on error. An orphaned observer entry only ever causes superfluous future checks, - // never a data leak (the leak-relevant gate is authorizeObservation, which keys off the live - // sharing graph). + // 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. async #removeObserverFromGatekeepers(observerId: string, gatekeeperIds: number[]): Promise { await Promise.all(gatekeeperIds.map(async id => { try { @@ -7808,8 +7891,8 @@ class OverseerImpl implements AgentHooks { // For each affected collaborator who is now fully unauthorized (newRole === null) and has an // observer record: best-effort removeObserver on all gatekeeper facets, then delete the record. // All calls are best-effort -- an orphaned observer entry only causes superfluous future checks, - // never a data leak (the leak-relevant gate is authorizeObservation, keyed off the live sharing - // graph). See observers-implementation-plan.md §5 Step 6. + // 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. See observers-implementation-plan.md §5 Step 6. async tearDownLostObservers(affected: AffectedCollaborator[]): Promise { let gatekeeperIds = [...this.storage.gatekeepers.list()].map(gk => gk.id); for (let entry of affected) { @@ -7904,6 +7987,9 @@ class OverseerImpl implements AgentHooks { // Gatekeepers that refused (or whose account was gone) during this call and have not verified // since let invalidated = new Set(); + // Whether a failure scrubbed a previously-persisted account choice, i.e. this collaborator's + // verified coverage shrank. See the catch below. + let scrubbedCoverage = false; // Failures from the previous pass, keyed by gatekeeper id: an already-configured binding whose // chosen account was disconnected, or which the gatekeeper refused. @@ -8005,6 +8091,10 @@ class OverseerImpl implements AgentHooks { if (persisted && gk.id in persisted.accountChoices) { delete persisted.accountChoices[gk.id]; this.storage.observers.put(persisted); + // The scrub only rewrites what the record claims; the collaborator's other + // sessions are still open and still hold the access it used to justify. Note the + // shrink so the catch below can sever them. + scrubbedCoverage = true; } this.logger.warn("observer verification failed", { event: "gatekeeper.observer.verify.failed", @@ -8075,6 +8165,17 @@ class OverseerImpl implements AgentHooks { // would have excluded them from is admitted with nothing left to block it. let rollback = returningObserver ? newlyAdded : new Set([...newlyAdded, ...invalidated]); await this.#removeObserverFromGatekeepers(observerId, [...rollback]); + + // This open is being denied, but the collaborator may hold other sessions that opened while + // the scrubbed choice still verified them. Sever every session so they must re-verify -- + // whoever can't will simply be denied their next open. This cannot loop: a second identical + // failure finds the entry already scrubbed, so no flag and no restart. A re-prompt that + // repairs the failure never reaches here, and step 6 re-persists full coverage. + if (scrubbedCoverage) { + this.#restartIfShared( + "Gadget restarted because a collaborator failed to re-verify their access."); + } + throw err; } @@ -9356,7 +9457,7 @@ class OverseerClientInterface extends RpcTarget implements Overseer { await this.impl.ctx.blockConcurrencyWhile(async () => { await this.#owner.deleteGadget(this.impl.ctx.id.toString()); await this.impl.ctx.storage.deleteAll(); - this.impl.scheduleRevocationRestart(); + this.impl.scheduleAccessRestart("Gadget restarted because the workspace was deleted."); this.impl.ownerId = undefined; }); @@ -10485,7 +10586,8 @@ class OverseerClientInterface extends RpcTarget implements Overseer { // excluded). A no-op removal -- e.g. severing a share-link edge nobody relied on -- shouldn't // disconnect everyone. if (affected.length > 0) { - this.impl.scheduleRevocationRestart(); + this.impl.scheduleAccessRestart( + "Gadget restarted to revoke access for a removed collaborator."); } return affected; } @@ -10504,7 +10606,8 @@ class OverseerClientInterface extends RpcTarget implements Overseer { await this.impl.refreshAffectedCollaboratorListings(affected); // Only restart if someone actually lost access or was downgraded (see removeCollaborator). if (affected.length > 0) { - this.impl.scheduleRevocationRestart(); + this.impl.scheduleAccessRestart( + "Gadget restarted to revoke access for a revoked share link."); } return affected; } From 790cf7c3779dd89c082edf59b807f7d194180443 Mon Sep 17 00:00:00 2001 From: Maximo Guk <62088388+Maximo-Guk@users.noreply.github.com> Date: Fri, 28 Aug 2026 11:27:25 -0500 Subject: [PATCH 05/14] Bugfix: Close the addGatekeeper publication window. `addGatekeeper` published the gatekeeper record before awaiting the gatekeeper's `describe()`, because `getGatekeeperFacet(id)` resolved the class from that record. The DO's input gate is open across the await and ids are allocated sequentially, so a live `build` session could guess the id and `getGatekeeperById()`/`openSession()` on the owner's brand-new connection -- which gates on nothing but record existence -- for as long as `describe()` took, all of it before `#restartIfShared` severed it. `getGatekeeperFacet` now optionally takes the class directly, so the record is published exactly once, after `describe()` resolves. Nothing a gatekeeper's `describe()` can reach calls back into the overseer to resolve itself by record, so no caller needs the early put. Co-Authored-By: Claude Opus 5 --- .../__tests__/observer-scope-restart.test.ts | 35 +++++++++++++++++++ packages/workshop-backend/src/overseer.ts | 21 +++++++---- 2 files changed, 50 insertions(+), 6 deletions(-) diff --git a/packages/workshop-backend/__tests__/observer-scope-restart.test.ts b/packages/workshop-backend/__tests__/observer-scope-restart.test.ts index 422549b08..5d162ea33 100644 --- a/packages/workshop-backend/__tests__/observer-scope-restart.test.ts +++ b/packages/workshop-backend/__tests__/observer-scope-restart.test.ts @@ -13,6 +13,7 @@ import { env } from "cloudflare:workers"; import { runInDurableObject } from "cloudflare:test"; import type { AiChatAuthorInfo } from "@gadgets/workshop-shared/api"; import type { OverseerDurableObject } from "../src/overseer.js"; +import { openFakeOverseer } from "./fixtures.js"; declare module "cloudflare:workers" { interface ProvidedEnv { @@ -107,6 +108,40 @@ describe("restarting sessions when verification scope widens", () => { expect(restarts).toHaveLength(1); })); + it("a connection under construction is unreachable until the restart is scheduled", + () => withImpl(async (impl, restarts) => { + addCollaborator(impl); + let releaseDescribe!: () => void; + let describing = new Promise(resolve => { releaseDescribe = resolve; }); + impl.getGatekeeperFacet = () => ({ + describe: async () => { + await describing; + return { title: "Test", url: "https://example.com/new" }; + }, + }); + // A "build" client interface over this DO's real gatekeeper table, which is all + // getGatekeeperById consults. + let client = await openFakeOverseer({ gatekeepers: impl.storage.gatekeepers }); + + // Ids are allocated sequentially, so a client can simply guess the next one. + let id = impl.storage.nextGatekeeperId.get(); + let added = impl.addGatekeeper({} as any, CONNECTION_SPEC); + await settle(); + + // The DO's input gate is open across describe(), so a live build session gets a turn here -- + // before #restartIfShared has severed it. Nothing is published for it to find. + expect(impl.storage.gatekeepers.get(id)).toBeUndefined(); + await expect(client.getGatekeeperById(id)).rejects.toThrow(/No such gatekeeper id/); + expect(restarts).toEqual([]); + + releaseDescribe(); + await added; + await settle(); + + expect(impl.storage.gatekeepers.get(id).resourceTitle).toBe("Test"); + expect(restarts).toHaveLength(1); + })); + it("adding a connection to a solo workspace disturbs nobody", () => withImpl(async (impl, restarts) => { stubFacets(impl); diff --git a/packages/workshop-backend/src/overseer.ts b/packages/workshop-backend/src/overseer.ts index 832d1e9f2..7c0f93e0b 100644 --- a/packages/workshop-backend/src/overseer.ts +++ b/packages/workshop-backend/src/overseer.ts @@ -4284,13 +4284,15 @@ class OverseerImpl implements AgentHooks { } } - getGatekeeperFacet(id: number): Fetcher> { + // `cls` is for the one caller that has the class in hand but has deliberately not published the + // record yet (`addGatekeeper`); everyone else resolves it from the record. + getGatekeeperFacet(id: number, cls?: GatekeeperClass): Fetcher> { return this.ctx.facets.get(`gatekeeper${id}`, async () => { - let cls = this.storage.gatekeepers.get(id)?.class; - if (!cls) { + let resolved = cls ?? this.storage.gatekeepers.get(id)?.class; + if (!resolved) { throw new Error("no such gatekeeper?"); } - return {class: cls}; + return {class: resolved}; }); } @@ -4365,9 +4367,14 @@ class OverseerImpl implements AgentHooks { class: cls, creationSpec, }; - this.storage.gatekeepers.put(gatekeeperRecord); - let facet = this.getGatekeeperFacet(id); + // The record is published only once, below, after describe() resolves -- the facet takes the + // class directly so it needs no record to exist yet. Publishing it before the await instead + // would expose the connection for as long as describe() takes, which is entirely before + // #restartIfShared severs the sessions that were never verified against it: the DO's input gate + // is open across the await, ids are allocated sequentially, so a live build session can guess + // this one, and getGatekeeperById (OverseerClientInterface) gates on nothing but existence. + let facet = this.getGatekeeperFacet(id, cls); try { let description = await facet.describe(); gatekeeperRecord.resourceTitle = description.title; @@ -4375,6 +4382,8 @@ class OverseerImpl implements AgentHooks { gatekeeperRecord.hasSlashCommands = description.hasSlashCommands; this.storage.gatekeepers.put(gatekeeperRecord); } catch (error) { + // Still the right teardown with nothing published: it deletes the facet we just created, and + // deleting an unwritten record is a no-op. this.removeGatekeeper(id); throw error; } From b1e825f4302ef1f756aa9836f997eaccf2394e24 Mon Sep 17 00:00:00 2001 From: Maximo Guk <62088388+Maximo-Guk@users.noreply.github.com> Date: Fri, 28 Aug 2026 14:28:19 -0500 Subject: [PATCH 06/14] Bugfix: Verify collaborators on the external-message path too. receiveExternalMessage() checked only the caller's role. Observer verification -- which is how a collaborator earns the right to see what the workspace has read -- runs at open(), so a "build" collaborator who never opened the workspace, or whose upstream access was since revoked, could still drive the agent and have it answer out of chat history and gadget storage. Extract the gate open() applies into authorizeCollaborator(): resolve the effective role from the permission graph, then run ensureObserver for that role. Both entry points call it. The external path passes requireRole: "build", so an insufficient role is denied before verification runs -- a "use" collaborator would otherwise be verified, or told to go fix a verification failure, for access this path can never grant them. It also passes no configureCb, since there is no channel to prompt on: an unverified caller is told to open the workspace in a browser, which is where configuration happens. roleRank is exported for the requireRole comparison, so it ranks rather than string-compares. Also has open() await ambient reconciliation before authorizing rather than between the role check and verification, which is where the two halves now join. Co-Authored-By: Claude Opus 5 --- .../external-message-verification.test.ts | 205 ++++++++++++++++++ .../__tests__/observer-reverification.test.ts | 14 ++ .../__tests__/observer-role-scope.test.ts | 181 ++++++++++++++++ .../fixtures/gatekeeper-test/src/env.d.ts | 11 + .../gatekeeper-test/src/test-gatekeeper.ts | 98 ++++++++- .../fixtures/gatekeeper-test/wrangler.jsonc | 27 ++- packages/integration-tests/src/harness.ts | 6 + packages/integration-tests/src/rpc-client.ts | 8 + .../workshop-backend/__tests__/fixtures.ts | 6 +- packages/workshop-backend/src/overseer.ts | 72 ++++-- packages/workshop-backend/src/sharing.ts | 8 +- 11 files changed, 610 insertions(+), 26 deletions(-) create mode 100644 packages/integration-tests/__tests__/external-message-verification.test.ts create mode 100644 packages/integration-tests/__tests__/observer-role-scope.test.ts diff --git a/packages/integration-tests/__tests__/external-message-verification.test.ts b/packages/integration-tests/__tests__/external-message-verification.test.ts new file mode 100644 index 000000000..66aa3eafa --- /dev/null +++ b/packages/integration-tests/__tests__/external-message-verification.test.ts @@ -0,0 +1,205 @@ +// Tests for the external-message authorization gate (authorizeCollaborator in overseer.ts): +// receiveExternalMessage() must hold a collaborator to the same observer verification open() +// applies -- non-interactively, since this path has no way to prompt for account configuration -- +// and must deny an insufficient role *before* verification runs. +// +// These live in their own file -- with their own harness, like every suite here -- so the suite +// stays self-contained as the observer suites around it grow. + +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import type { RpcStub } from "capnweb"; +import type { AuthenticatedApi, PublicApi } from "@gadgets/workshop-shared/api"; +import type { + SubmitExternalMessageResult, +} from "@gadgets/workshop-shared/external-message-gateway"; +import { + startTestGatekeeperHarness, TEST_GATEKEEPER_WORKER, TEST_VENDOR_ID, type Harness, +} from "../src/harness.js"; +import { + accountLabel, connect, listConnectedAccounts, MAX_OBSERVER_PROMPTS, nextUsernames, + ObserverConfigRecorder, signUp, stubFor, waitFor, type ConnectedAccount, +} from "../src/rpc-client.js"; +import { NetworkInterceptor } from "../src/network-interceptor.js"; + +// Reason text shaped like what a gatekeeper actually reports on a settled denial. Its appearance +// in the gateway's reply below is what proves a live verification round trip happened. +const DENIED_REASON = "You do not have access to this thing."; + +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; + }); +} + +/** + * Submit an external chat message as `callerEmail`, through the fixture worker's control surface + * (and so through the Workshop's real ExternalMessageGateway entrypoint). + */ +async function submitExternalMessage(input: { + callerEmail: string; gadgetKey: string; prompt: string; +}): Promise { + const res = await harness.fetchWorker( + TEST_GATEKEEPER_WORKER, "http://gatekeeper-test.test/control/submit-external-message", + { method: "POST", body: JSON.stringify({ + chatKey: `chat-${input.gadgetKey}`, messageKey: crypto.randomUUID(), + gadgetTitle: input.gadgetKey, ...input }) }); + if (res.status !== 200) { + throw new Error(`submit-external-message failed with ${res.status}: ${await res.text()}`); + } + return await res.json() as SubmitExternalMessageResult; +} + +/** Tell the gatekeeper what to do the next time it's asked to admit `label` as an observer. */ +async function setVerifyOutcome( + label: string, outcome: { allow: true } | { allow: false; reason: string }): Promise { + const res = await harness.fetchWorker( + TEST_GATEKEEPER_WORKER, "http://gatekeeper-test.test/control/verify-outcome", + { method: "POST", body: JSON.stringify({ label, ...outcome }) }); + if (res.status !== 204) { + throw new Error(`Setting the verify outcome failed with ${res.status}: ${await res.text()}`); + } +} + +/** The workspace id behind an external gadgetKey -- the DO id the gateway derives from it. */ +async function externalGadgetId(gadgetKey: string): Promise { + const res = await harness.fetchWorker( + TEST_GATEKEEPER_WORKER, "http://gatekeeper-test.test/control/external-gadget-id", + { method: "POST", body: JSON.stringify({ gadgetKey }) }); + if (res.status !== 200) { + throw new Error(`external-gadget-id failed with ${res.status}: ${await res.text()}`); + } + return (await res.json() as { gadgetId: string }).gadgetId; +} + +describe("external-message verification", () => { + it.concurrent("the external-message path verifies collaborators like open() does", async () => { + await withSession(async publicApi => { + const [alice, bob, carol] = nextUsernames("alice", "bob", "carol"); + const aliceApi = await signUp(publicApi, alice); + const aliceAccount = await provisionAccount(aliceApi); + const gadgetKey = `external-${crypto.randomUUID()}`; + + // Alice creates the workspace through the external channel. No test user has an AI model, + // so a submission that passes the authorization gate is rejected with the model message -- + // which is what tells "passed the gate" apart from a gate denial below. + await expect(submitExternalMessage({ callerEmail: alice, gadgetKey, prompt: "hello" })) + .resolves.toMatchObject({ + accepted: false, message: expect.stringMatching(/AI model/i) }); + + // Wire the workspace up over the web API: connect a Thing (an account-requiring connection, + // so collaborators must be observer-verified against it) and add Bob. + const gadgetId = await externalGadgetId(gadgetKey); + using overseer = await aliceApi.openGadget(gadgetId); + const gatekeeper = await overseer.newGatekeeper(aliceAccount.id, thingUrl("external")); + if (!gatekeeper) throw new Error("Failed to create the test connection"); + + const bobApi = await signUp(publicApi, bob); + const bobAccount = await provisionAccount(bobApi); + await overseer.addCollaborator(bob, "build"); + + // A stranger is turned away by role, before verification is ever attempted. + await signUp(publicApi, carol); + await expect(submitExternalMessage({ callerEmail: carol, gadgetKey, prompt: "hi" })) + .resolves.toMatchObject({ + accepted: false, message: expect.stringMatching(/do not have access/i) }); + + // Bob has build access but has never opened, so he was never observer-verified -- and this + // path has no configuration channel to fix that. The agent's reply could surface anything + // the workspace has already read, so the external path must refuse him rather than fall + // through to the model check. + await expect(submitExternalMessage({ callerEmail: bob, gadgetKey, prompt: "hi" })) + .resolves.toMatchObject({ + accepted: false, message: expect.stringMatching(/could not be verified/i) }); + + // Opening the workspace verifies him; the same submission now passes the gate and fails + // only on the missing AI model, exactly like the owner's did. + const callback = stubFor( + new ObserverConfigRecorder().alwaysChoose(bobAccount.id, MAX_OBSERVER_PROMPTS)); + try { + (await bobApi.openGadget(gadgetId, undefined, callback))[Symbol.dispose](); + } finally { + callback[Symbol.dispose](); + } + await expect(submitExternalMessage({ callerEmail: bob, gadgetKey, prompt: "hi" })) + .resolves.toMatchObject({ + accepted: false, message: expect.stringMatching(/AI model/i) }); + + // The gatekeeper now revokes Bob's underlying access. His persisted observer record is + // untouched, so only a live addObserver re-verification on this submission can notice -- + // and the gatekeeper's own refusal reason appearing in the reply is the proof that round + // trip happened, since nothing persisted in the Workshop contains it. An implementation + // that merely checked the record would keep accepting him here. + await setVerifyOutcome(accountLabel(bobAccount), { allow: false, reason: DENIED_REASON }); + const revoked = await submitExternalMessage({ callerEmail: bob, gadgetKey, prompt: "hi" }); + if (revoked.accepted) throw new Error("The revoked submission was accepted"); + expect(revoked.message).toMatch(/could not be verified/i); + expect(revoked.message).toContain(DENIED_REASON); + }); + }); + + it.concurrent("the external-message path denies a use collaborator by role, not verification", + async () => { + await withSession(async publicApi => { + const [alice, dave] = nextUsernames("alice", "dave"); + const aliceApi = await signUp(publicApi, alice); + const aliceAccount = await provisionAccount(aliceApi); + const gadgetKey = `external-use-${crypto.randomUUID()}`; + + // Alice creates the workspace through the external channel (the AI-model rejection means + // her submission passed the gate), then binds its connection to a gadget so it falls in + // "use" verification scope. + await expect(submitExternalMessage({ callerEmail: alice, gadgetKey, prompt: "hello" })) + .resolves.toMatchObject({ + accepted: false, message: expect.stringMatching(/AI model/i) }); + const gadgetId = await externalGadgetId(gadgetKey); + using overseer = await aliceApi.openGadget(gadgetId); + const gatekeeper = await overseer.newGatekeeper(aliceAccount.id, thingUrl("external-use")); + if (!gatekeeper) throw new Error("Failed to create the test connection"); + using gadget = await overseer.createGadget("Test Gadget", undefined, "TEST_GADGET"); + await gadget.bind("TEST_THING", await gatekeeper.getId()); + + // Dave is in verification scope and unverified, but this path can never grant a "use" + // collaborator agent access, so his role is checked before verification runs: he gets the + // plain denial, not a verification failure he has no reason to go fix. + await signUp(publicApi, dave); + if (!await overseer.addCollaborator(dave, "use")) { + throw new Error(`Failed to share the gadget with ${dave}`); + } + await expect(submitExternalMessage({ callerEmail: dave, gadgetKey, prompt: "hi" })) + .resolves.toMatchObject({ + accepted: false, message: expect.stringMatching(/do not have access/i) }); + }); + }); +}); diff --git a/packages/integration-tests/__tests__/observer-reverification.test.ts b/packages/integration-tests/__tests__/observer-reverification.test.ts index 09b8b5d4a..57941325b 100644 --- a/packages/integration-tests/__tests__/observer-reverification.test.ts +++ b/packages/integration-tests/__tests__/observer-reverification.test.ts @@ -172,6 +172,15 @@ async function bobOpens( } } +// A denied re-verification scrubs the account choice it just failed against, so the overseer +// severs every session on the workspace -- Bob may hold others that opened while that choice still +// verified him. The sever is a ctx.abort() ~100ms after the open rejects, i.e. after the test body +// has returned. Wait it out before withSession() drops the connection: an abort that lands with no +// client left on the workspace crashes the local workerd, and these tests share one harness, so the +// crash fails whichever siblings are mid-flight rather than this test. +const RESTART_SETTLE_MS = 400; +const settleRestart = () => new Promise(resolve => setTimeout(resolve, RESTART_SETTLE_MS)); + /** Open once and answer the prompt, which is what persists Bob's account choice. */ async function bobOpensAndCloses(shared: SharedGadget): Promise { const recorder = @@ -272,6 +281,7 @@ describe("observer re-verification", () => { expect(need.failure).toBeDefined(); expect(need.failure!.accountId).toBe(shared.bobAccount.id); expect(need.failure!.reason).toContain(EXPIRED_REASON); + await settleRestart(); }); }); @@ -295,6 +305,7 @@ describe("observer re-verification", () => { expect(error!.message).toContain(EXPIRED_REASON); // One line per failed binding, so a single failure must not introduce stray newlines. expect(error!.message.split("\n").filter(l => l.includes(shared.bobLabel))).toHaveLength(1); + await settleRestart(); }); }); @@ -326,6 +337,7 @@ describe("observer re-verification", () => { expect(error!.message).toContain("Test Thing multi-a"); expect(error!.message).toContain("Test Thing multi-b"); expect(error!.message.split("\n").filter(l => l.includes(shared.bobLabel))).toHaveLength(2); + await settleRestart(); }); }); @@ -362,6 +374,7 @@ describe("observer re-verification", () => { expect(events.filter(e => e.type === "remove")).toEqual([]); // Both successful verifications registered it: the first open and the pass-2 repair. expect(events.filter(e => e.type === "add")).toHaveLength(2); + await settleRestart(); }); }); @@ -382,6 +395,7 @@ describe("observer re-verification", () => { expect(error).not.toBeNull(); expect(error!.message).toMatch(/could not confirm/i); expect(error!.message).toContain(DENIED_REASON); + await settleRestart(); }); }); }); diff --git a/packages/integration-tests/__tests__/observer-role-scope.test.ts b/packages/integration-tests/__tests__/observer-role-scope.test.ts new file mode 100644 index 000000000..d699e7fa4 --- /dev/null +++ b/packages/integration-tests/__tests__/observer-role-scope.test.ts @@ -0,0 +1,181 @@ +// Tests for role-scoped observer enforcement: a collaborator is held only to what their role's +// verification scope can actually cover ("use" collaborators are verified only against +// gadget-bound connections; see #inScopeGatekeepers in overseer.ts). So binding a connection into +// a gadget widens every "use" collaborator's scope, and since sessions are verified only at open(), +// 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 abort 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. + +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_VENDOR_ID, type Harness, +} from "../src/harness.js"; +import type { TestSession } from "../fixtures/gatekeeper-test/src/test-gatekeeper.js"; +import { + 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; + }); +} + +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. +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 }; +} + +// The owner's own reconnect after a restart, on a fresh connection: the abort fells every client of +// the workspace, so `ws`'s stubs -- and the whole session they came from -- are dead afterwards. +async function reopenAfterRestart(ws: Workspace): Promise<{ + publicApi: RpcStub; + session: RpcStub; +}> { + await waitFor("the restart to fell the old workspace instance", () => + ws.session.readThing().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(ws.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.readThing(); + return { publicApi, session }; + } catch { + publicApi[Symbol.dispose](); + return null; + } + }); +} + +// Carol's forced re-open, on the fresh connection her browser would reconnect with. +async function carolReopens( + ws: Workspace, carol: string, recorder: ObserverConfigRecorder): Promise { + const publicApi = connect(harness.url); + try { + const carolApi = await logIn(publicApi, carol); + const callback = stubFor(recorder); + try { + (await carolApi.openGadget(ws.gadgetId, undefined, callback))[Symbol.dispose](); + } finally { + callback[Symbol.dispose](); + } + } finally { + publicApi[Symbol.dispose](); + } +} + +describe("role-scoped observer enforcement", () => { + it.concurrent("a use collaborator is verified only against connections in their scope", + async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "use-scope"); + const [carol] = nextUsernames("carol"); + const carolApi = await signUp(publicApi, carol); + const carolAccount = await provisionAccount(carolApi); + const collaborator = await ws.overseer.addCollaborator(carol, "use"); + if (!collaborator) throw new Error(`Failed to share the gadget with ${carol}`); + + // No gadget binds the connection, so Carol's "use" verification scope is empty: her open + // must not prompt (the recorder has no queued responses, so an unexpected prompt throws). + const emptyCallback = stubFor(new ObserverConfigRecorder()); + try { + (await carolApi.openGadget(ws.gadgetId, undefined, emptyCallback))[Symbol.dispose](); + } finally { + emptyCallback[Symbol.dispose](); + } + + // 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.readThing()).resolves.toContain("use-scope"); + + // 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 + // live session is never re-verified in place -- so the workspace restarts instead. + using gadget = await ws.overseer.createGadget("Test Gadget", undefined, "TEST_GADGET"); + await gadget.bind("TEST_THING", ws.gatekeeperId); + + 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.readThing()).resolves.toContain("use-scope"); + + // 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. + const recorder = + new ObserverConfigRecorder().alwaysChoose(carolAccount.id, MAX_OBSERVER_PROMPTS); + await carolReopens(ws, carol, recorder); + expect(recorder.callCount).toBe(1); + expect(recorder.calls[0].map(need => need.gatekeeperId)).toEqual([ws.gatekeeperId]); + } finally { + reopened.publicApi[Symbol.dispose](); + } + }); + }); +}); diff --git a/packages/integration-tests/fixtures/gatekeeper-test/src/env.d.ts b/packages/integration-tests/fixtures/gatekeeper-test/src/env.d.ts index b5b372a78..8a9715af0 100644 --- a/packages/integration-tests/fixtures/gatekeeper-test/src/env.d.ts +++ b/packages/integration-tests/fixtures/gatekeeper-test/src/env.d.ts @@ -12,6 +12,17 @@ declare namespace Cloudflare { // Storage classes exposed as DO namespaces on ctx.exports. durableNamespaces: "TestGatekeeper" | "TestControl"; } + + interface Env { + // The Workshop's external-message gateway entrypoint (see wrangler.jsonc). The contract + // interface is not entrypoint-branded (the shipping class implements it), so brand it here to + // satisfy Fetcher's constraint. + WORKSHOP_EXTERNAL_MESSAGES: Fetcher< + import("@gadgets/workshop-shared/external-message-gateway").ExternalMessageGateway & + Rpc.WorkerEntrypointBranded>; + // The Workshop's Overseer DO namespace (see wrangler.jsonc); used only to derive ids. + WORKSHOP_OVERSEER: DurableObjectNamespace; + } } interface ExecutionContext { 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 db72750eb..53fa145cf 100644 --- a/packages/integration-tests/fixtures/gatekeeper-test/src/test-gatekeeper.ts +++ b/packages/integration-tests/fixtures/gatekeeper-test/src/test-gatekeeper.ts @@ -20,12 +20,15 @@ // is one control knob here, `allow`, and the reason string is what carries the distinction to the // user. Tests exercise both narratives by choosing reason text. -import { DurableObject, WorkerEntrypoint, type RpcStub } from "cloudflare:workers"; +import { DurableObject, RpcTarget, WorkerEntrypoint, type RpcStub } from "cloudflare:workers"; import type { AccountDescription, ActionKind, ApprovalQueue, Gatekeeper, GatekeeperConnectCallback, GatekeeperUser, GatekeeperUserVerifier, ResourceDescription, ResourceConfiguratorFrame, SupportedResource, VendorDescription, } from "@gadgets/workshop-shared/gatekeeper"; +import type { + ChatGatewayRpcTarget, GadgetResponse, +} from "@gadgets/workshop-shared/external-message-gateway"; // Nothing but classes and the default handler may be exported from a Worker entry module: workerd // treats every named export as an entrypoint and rejects anything that isn't one. @@ -243,8 +246,44 @@ export class TestVerifier // --------------------------------------------------------------------------- // Gatekeeper (one per bound resource, running as a facet under the gadget's Overseer) -/** No operations: these tests never open a gadget's session, only verify observers. */ -export type TestSession = Record; +/** + * A live session against a Test Thing, opened via `GatekeeperClient.openSession()`. + * + * The two methods exist so tests can drive the overseer's observation/action policy through the + * same `ApprovalQueue` funnel a shipping gatekeeper uses: `readThing()` records an observation and + * `doThing()` submits an action. + */ +export class TestSession extends RpcTarget { + #queue: RpcStub; + #title: string; + + constructor(queue: RpcStub, title: string) { + super(); + this.#queue = queue; + this.#title = title; + } + + async readThing(): Promise { + await this.#queue.authorizeObservation({ + title: `Read ${this.#title}`, + description: `The test read ${this.#title}.`, + }); + return `the contents of ${this.#title}`; + } + + async doThing(): Promise { + await this.#queue.submitAction(0, { + title: `Poke ${this.#title}`, + description: `The test poked ${this.#title}.`, + implementsRevert: false, + }); + } + + /** The session owns the queue stub dup'd in startSession(); release it with the session. */ + [Symbol.dispose]() { + this.#queue[Symbol.dispose](); + } +} export class TestGatekeeper extends DurableObject implements Gatekeeper { @@ -277,8 +316,15 @@ export class TestGatekeeper return []; } - async startSession(_approvalQueue: RpcStub): Promise { - return {}; + async startSession(approvalQueue: RpcStub): Promise { + // The session calls the queue after startSession() returns, so it owns a duplicate. + let queue = approvalQueue.dup(); + try { + return new TestSession(queue, (await this.describe()).title); + } catch (err) { + queue[Symbol.dispose]?.(); + throw err; + } } /** @@ -338,8 +384,16 @@ function isNonEmptyString(value: unknown): value is string { return typeof value === "string" && value.length > 0; } +/** + * Discards Gadget responses. The control endpoint below only asserts on the submission result, + * and the rejection paths under test return before any response is produced. + */ +class DevNullChatGateway extends RpcTarget implements ChatGatewayRpcTarget { + async onGadgetResponse(_response: GadgetResponse): Promise {} +} + export default { - async fetch(req: Request, _env: Cloudflare.Env, ctx: ExecutionContext): Promise { + async fetch(req: Request, env: Cloudflare.Env, ctx: ExecutionContext): Promise { const url = new URL(req.url); let body: unknown; @@ -392,6 +446,38 @@ export default { return Response.json({ count: await control(ctx.exports).getAmbientVerificationCount(label) }); } + // Submit an external chat message through the Workshop's ExternalMessageGateway entrypoint, + // the way a chat-integration worker would, so tests can drive receiveExternalMessage(). + // Body: {"callerEmail", "gadgetKey", "chatKey", "messageKey", "gadgetTitle", "prompt"} + // -> SubmitExternalMessageResult + if (url.pathname === "/control/submit-external-message" && req.method === "POST") { + const fields = + ["callerEmail", "gadgetKey", "chatKey", "messageKey", "gadgetTitle", "prompt"] as const; + const input = {} as Record<(typeof fields)[number], string>; + for (const field of fields) { + const value = (body as Record)[field]; + if (!isNonEmptyString(value)) return badRequest(`\`${field}\` must be a non-empty string`); + input[field] = value; + } + // The instance becomes a stub when it crosses the RPC boundary; the parameter type can only + // name the stub side of that. + const chatGatewayRpcTarget = + new DevNullChatGateway() as unknown as RpcStub; + return Response.json(await env.WORKSHOP_EXTERNAL_MESSAGES.submitExternalMessage( + { ...input, chatGatewayRpcTarget })); + } + + // Map an external gadgetKey to the Overseer id the gateway targets -- the DO named + // ":", where "test" is the `source` prop on WORKSHOP_EXTERNAL_MESSAGES -- + // so a test can open the same workspace over the web API, which addresses by DO id string. + // Body: {"gadgetKey": "..."} -> {"gadgetId": "..."} + if (url.pathname === "/control/external-gadget-id" && req.method === "POST") { + const { gadgetKey } = body as Record; + if (!isNonEmptyString(gadgetKey)) return badRequest("`gadgetKey` must be a non-empty string"); + return Response.json( + { gadgetId: env.WORKSHOP_OVERSEER.idFromName(`test:${gadgetKey}`).toString() }); + } + // Make this Worker issue a subrequest, so a test can prove that Worker-originated fetches really // do route through the interceptor rather than out to the internet. // diff --git a/packages/integration-tests/fixtures/gatekeeper-test/wrangler.jsonc b/packages/integration-tests/fixtures/gatekeeper-test/wrangler.jsonc index d4ca3ff2d..96167a8a4 100644 --- a/packages/integration-tests/fixtures/gatekeeper-test/wrangler.jsonc +++ b/packages/integration-tests/fixtures/gatekeeper-test/wrangler.jsonc @@ -12,7 +12,32 @@ "compatibility_date": "2026-02-02", "compatibility_flags": ["experimental", "allow_irrevocable_stub_storage"], - // DO classes are reached via ctx.exports; no durable_objects binding needed. + // Lets the control surface submit external chat messages through the Workshop's gateway + // entrypoint the way a real chat-integration worker (bound with its own `source` prop) would. + // The harness always boots workshop-backend as the primary worker, so the name resolves. + "services": [ + { + "binding": "WORKSHOP_EXTERNAL_MESSAGES", + "service": "workshop-backend", + "entrypoint": "ExternalMessageGateway", + "props": { "source": "test" } + } + ], + + // The Workshop's Overseer namespace, so the control surface can derive the DO id behind an + // external gadgetKey -- the same name-derived id the gateway targets -- for tests to open the + // workspace over the web API. The binding only derives ids; it never reaches an instance. + "durable_objects": { + "bindings": [ + { + "name": "WORKSHOP_OVERSEER", + "class_name": "OverseerDurableObject", + "script_name": "workshop-backend" + } + ] + }, + + // This worker's own DO classes are reached via ctx.exports; no durable_objects binding needed. "migrations": [ { "tag": "v0", diff --git a/packages/integration-tests/src/harness.ts b/packages/integration-tests/src/harness.ts index 199facdeb..da1df1514 100644 --- a/packages/integration-tests/src/harness.ts +++ b/packages/integration-tests/src/harness.ts @@ -76,6 +76,12 @@ function readWorkerConfig(dir: string): WorkerConfig { const config = parsed.data; config.build = { ...config.build, cwd: dir }; config.main = join(dir, config.main); + + // Local-dev var files (.dev.vars/.env at the harness root) must not leak into tests: a + // developer's local settings (say CF_AI_GATEWAY_*) would make suites behave differently on + // their machine than in CI -- up to sending real AI traffic. Declaring an empty required-secrets + // list makes wrangler exclude every such key that is not already a config var. + config.secrets = { required: [] }; return config; } diff --git a/packages/integration-tests/src/rpc-client.ts b/packages/integration-tests/src/rpc-client.ts index b88b0c012..37934819a 100644 --- a/packages/integration-tests/src/rpc-client.ts +++ b/packages/integration-tests/src/rpc-client.ts @@ -79,6 +79,14 @@ export async function signUp( return (await api.authenticate(token)) as unknown as RpcStub; } +/** Log back into an account created by signUp(), e.g. from a fresh connection. */ +export async function logIn( + api: RpcStub, username: string): Promise> { + const token = await api.login(username, passwordHashFor(username)); + if (!token) throw new Error(`Login failed for "${username}"`); + return (await api.authenticate(token)) as unknown as RpcStub; +} + export type ConnectedAccount = { id: number; vendorId: string; diff --git a/packages/workshop-backend/__tests__/fixtures.ts b/packages/workshop-backend/__tests__/fixtures.ts index 9afac58d0..b8e15b0e6 100644 --- a/packages/workshop-backend/__tests__/fixtures.ts +++ b/packages/workshop-backend/__tests__/fixtures.ts @@ -86,7 +86,11 @@ export async function openFakeOverseer( joinOutputsFanout: () => () => {}, ensureObserver: async () => {}, syncOutputsTo: async () => {}, - getSharingManager: async () => ({ getEffectiveRole: () => role }), + // What open() consults for a non-owner's role: the permission-graph lookup and observer + // verification in one. The sharing manager is still reached, but only to redeem a share key, + // which these tests never pass. + authorizeCollaborator: async () => role, + getSharingManager: async () => ({}), ctx: { id: { toString: () => "workspace-id" }, exports: opts.exports ?? {} }, users: { idFromString: (id: string) => id, diff --git a/packages/workshop-backend/src/overseer.ts b/packages/workshop-backend/src/overseer.ts index 7c0f93e0b..54ecbb86d 100644 --- a/packages/workshop-backend/src/overseer.ts +++ b/packages/workshop-backend/src/overseer.ts @@ -42,7 +42,8 @@ import type { ProductAnalyticsConnectionType, ProductAnalyticsGadgetInput } from import { checkUsageAndBalance } from "./ai-gateway-billing/limits/usage-checker"; import { completeAgentCatalogSnapshot, normalizeAgentCatalog } from "./agent-catalog"; import { refreshCachedBalance } from "./ai-gateway-billing/cloudflare/connection-service"; -import { SharingManager, SharingCaller, CollaboratorRecord, ShareKeyRecord } from "./sharing"; +import { SharingManager, SharingCaller, CollaboratorRecord, ShareKeyRecord, roleRank } + from "./sharing"; import { AutoApprovalDrainer } from "./auto-approval"; import { collectSlashCommands, invokeSlashCommand } from "./slash-commands"; import { createWorkshopLogger, obsContext, traced } from "./observability"; @@ -7941,6 +7942,30 @@ class OverseerImpl implements AgentHooks { } } + // The authorization gate every non-owner entry point (open(), receiveExternalMessage()) must + // pass through: resolve the caller's effective role, then verify them as an observer of + // everything this workspace has read. Returns null for no access; verification failures throw. + // A caller that requires at least `requireRole` (e.g. receiveExternalMessage needs "build") + // passes it so an insufficient role is denied *before* verification runs -- otherwise the caller + // would be verified (real addObserver calls, a persisted observer record) only to be turned + // away, or worse, told to fix a verification failure that can never grant them access. + // `configureCb` is forwarded to ensureObserver to prompt for unconfigured account choices; + // without it, verification is non-interactive and an unconfigured binding denies access. + async authorizeCollaborator( + profileId: string, + clientUser: DurableObjectStub, + opts: { + configureCb?: RpcStub; + requireRole?: CollaboratorRole; + } = {}): Promise { + let sharing = await this.getSharingManager(); + let role = sharing.getEffectiveRole(profileId); + if (!role || (opts.requireRole && roleRank(role) < roleRank(opts.requireRole))) return null; + + await this.ensureObserver(profileId, clientUser, role, opts.configureCb); + return role; + } + // Bring a non-owner `profileId` into compliance as an observer for their `role`, so that they may // open the Gadget. May invoke `configureCb` to ask the user to choose connected accounts for // gatekeeper bindings they haven't configured yet. Re-runs `addObserver` (re-verification) for @@ -8496,28 +8521,26 @@ export class OverseerDurableObject extends DurableObject { }); } - // Check authorization. Compute the caller's effective role from the permission graph; this - // both authorizes the session and determines which capability we hand back. + // Ambient reconciliation may attach Gatekeepers after open() starts. Finish it before taking + // the observer snapshot so every capability exposed to this collaborator has an observer. + await ensureCapsules; + + // Check authorization: compute the caller's effective role from the permission graph, then + // 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 prohibitAllSharing short-circuit above still wins over both. // // 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 // their session is force-restarted lands here and sees the terminal access-denied page. - let effectiveRole = sharing.getEffectiveRole(profileId); + let effectiveRole = await this.impl.authorizeCollaborator( + profileId, clientUser, {configureCb: configureObservers}); if (!effectiveRole) { throw createOpenGadgetError(OPEN_GADGET_ERROR_CODES.workspaceAccessDenied); } role = effectiveRole; - // Ambient reconciliation may attach Gatekeepers after open() starts. Finish it before taking - // the observer snapshot so every capability exposed to this collaborator has an observer. - await ensureCapsules; - - // Verify the caller may observe everything this Gadget has read through its in-scope - // gatekeepers, configuring their connected accounts if needed. This runs only after a valid - // role is confirmed, so it never reveals gatekeeper or resource metadata to an unauthorized - // user. The prohibitAllSharing short-circuit above still wins -- lockdown takes precedence. - await this.impl.ensureObserver(profileId, clientUser, role, configureObservers); - // Fire-and-forget a call to the collaborator's user DO so the gadget appears on // (or is refreshed on) their home page. let title = this.impl.storage.title.get(); @@ -8589,7 +8612,13 @@ export class OverseerDurableObject extends DurableObject { ownerId = callerId; } - // Caller must be the owner or a build collaborator. + // Caller must be the owner or a build collaborator. The agent's reply can surface anything + // the workspace has already read (chat history, gadget storage), so a collaborator passes the + // same authorization gate as open() -- but non-interactively: with no way to configure + // accounts here, an unverified caller is sent to open the workspace, which is where + // verification happens. Requiring "build" up front means a "use" collaborator gets the plain + // 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.prohibitAllSharing.get()) { return { @@ -8597,7 +8626,18 @@ export class OverseerDurableObject extends DurableObject { message: "This workspace has sharing disabled, so only its owner can access it.", }; } - let role = (await this.impl.getSharingManager()).getEffectiveRole(callerProfile.id); + let role: CollaboratorRole | null; + try { + role = await this.impl.authorizeCollaborator( + callerProfile.id, caller, {requireRole: "build"}); + } catch (err) { + return { + accepted: false, + message: "Your access to the data this workspace has read could not be verified. Open " + + "the workspace in your browser to verify your access, then try again. " + + `(${stringifyError(err)})`, + }; + } if (role !== "build") { return { accepted: false, diff --git a/packages/workshop-backend/src/sharing.ts b/packages/workshop-backend/src/sharing.ts index ff0cb9e18..f11123922 100644 --- a/packages/workshop-backend/src/sharing.ts +++ b/packages/workshop-backend/src/sharing.ts @@ -25,8 +25,12 @@ import { AiChatAuthorInfo, CollaboratorInfo, PermissionEdge, CollaboratorRole, A from "@gadgets/workshop-shared/api"; import { Collection, NonUniqueIndex } from "@gadgets/typed-storage"; -// Roles are totally ordered: build > use. Higher rank means strictly more access. -function roleRank(role: CollaboratorRole): number { +/** + * Roles are totally ordered: build > use. Higher rank means strictly more access. Exported so + * role comparisons elsewhere (e.g. the Overseer's `requireRole` floor) rank rather than + * string-compare, which stays correct if a role is ever added between the two. + */ +export function roleRank(role: CollaboratorRole): number { return role === "build" ? 2 : 1; } From db563f7e16da7c843305f9eeb7499258fa8896d5 Mon Sep 17 00:00:00 2001 From: Maximo Guk <62088388+Maximo-Guk@users.noreply.github.com> Date: Fri, 28 Aug 2026 14:31:11 -0500 Subject: [PATCH 07/14] Docs: Document the session-restart model for observer verification. docs/observers.md gains a "Restarting when verification scope widens" subsection: the four triggers, why the merge trigger compares scopes rather than firing on any promotion, why shrinking scope and role rises are deliberately not triggers, why addGatekeeper's publication order is load-bearing under the restart, and where the enforcement moment actually falls for each trigger. Step 3 gains the record prune, the scrub-and-restart failure path and the returning-observer rollback rule; edge cases 3 and 5 are rewritten around them, and Step 6's justification for an orphaned entry is corrected -- a registration is what admits an open, so a stale one grants nothing on its own. docs/sharing.md renames scheduleRevocationRestart and documents the abort's second purpose, whose trigger is a grant rather than a revocation. Co-Authored-By: Claude Opus 5 --- docs/observers.md | 103 ++++++++++++++++++++++++++++++++++++++++++---- docs/sharing.md | 8 ++-- 2 files changed, 100 insertions(+), 11 deletions(-) diff --git a/docs/observers.md b/docs/observers.md index d6c5534c9..9e6fb3355 100644 --- a/docs/observers.md +++ b/docs/observers.md @@ -91,6 +91,8 @@ This feature replaces that all-or-nothing posture with a per-user, gatekeeper-me |---|---| | 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` | +| Authorization gate shared by `open()` and `receiveExternalMessage()` | `overseer.ts` `authorizeCollaborator()` | +| Session restart when verification scope widens | `overseer.ts` (`#restartIfShared`, `scheduleAccessRestart`) | | Server `openGadget` path | `packages/workshop-backend/src/server.ts:206` | | Role resolution / permission graph | `packages/workshop-backend/src/sharing.ts` (`getEffectiveRole`, `computeEffectiveRoles`, `hasAnyShares`) | | `prohibitAllSharing` enforcement | `overseer.ts:1171` (`authorizeObservation`), `:1207` (web fetch), `:1258` (`submitAction`) | @@ -254,7 +256,11 @@ Logic: - A `creationSpec` with a `vendorId` requires an account; other specs need no verifier or account choice. -2. **Load the observer record** for `profileId` (may be absent). +2. **Load the observer record** for `profileId` (may be absent), and **prune** any + `accountChoices` entry that is no longer in scope. The record must state only what this open + verifies: `use` scope is live binding state, so an entry left from before a connection was + unbound would otherwise silently re-register the collaborator off a choice made for a scope the + workspace no longer has, instead of asking them again after a rebind. 3. **Determine uncovered bindings**: in-scope account-requiring gatekeepers with no `accountChoices` entry in the record. Before prompting, automatically fill ambient bindings from @@ -276,9 +282,20 @@ Logic: 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: best-effort `removeObserver(record.observerId)` on the gatekeepers - added in *this* pass, do **not** persist the working record, and deny the open with a clear - message. + (or no longer) allowed. Every such failure goes through one `fail()` path that synchronously + scrubs the failed gatekeeper from the *persisted* record, so the record stops claiming a + verification that no longer holds, 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. + - Roll back only what *this call* added. A returning observer's registrations are kept: their + `observerId` is already persisted, a registration can only ever add exclusion names (so + keeping it is fail-closed), and the next successful open's `addObserver` overwrites its + verifier. Only a *first-ever* verification rolls back fully, since that collaborator was never + admitted and the minted id would otherwise linger unresolvable. + - A terminal failure that scrubbed a *previously-persisted* choice also restarts the workspace + (see "Restarting when verification scope widens" below), because the collaborator may hold + other sessions that opened while that choice still verified them. 6. **Persist the observer record** (with merged `accountChoices` and `observerId`) only after all `addObserver` calls succeed. Storing/creating the record is the canonical moment the user @@ -300,6 +317,56 @@ 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 + +Verification runs at `open()` and nowhere else, so a live session is only ever as verified as the +scope that existed when it opened. When that scope **widens**, the overseer restarts the workspace +rather than trying to re-verify sessions in place: `#restartIfShared(reason)` delegates to +`scheduleAccessRestart(reason)` — the same DO abort used to revoke a collaborator (see +`docs/sharing.md`) — so every client's browser reconnects and re-runs +`authorizeCollaborator`/`ensureObserver` against the new scope. It is a no-op when the workspace +has no collaborators: the owner is never an observer, so there is nobody to re-verify. + +Four events trigger it: + +| Event | What grows | +|---|---| +| `addGatekeeper()` with a vendor-backed `creationSpec` | **build** scope — a live `build` session can `getGatekeeperById()`/`openSession()` on it with no observer check | +| `bindWorkpiece()` for a permanent (non-`chatId`) edge onto a vendor-backed connection | **use** scope — the gadget UI a `use` session drives can now invoke it | +| A merge that promotes a pending gadget or a pending binding edge into `use` scope | **use** scope, same reason | +| A terminal `ensureObserver()` failure that scrubbed a previously-persisted account choice | Coverage *shrank*: the collaborator's other sessions still hold access the scrubbed choice used to justify. Scheduled when the failure becomes terminal, which a re-prompt the failing client never answers can defer (edge case 3) | + +The merge trigger compares the effective `use` scope before and after promotion rather than firing +on any promotion: most merges promote something, and a promoted gadget with no bindings — or an edge +onto a vendorless connection nobody is verified against — widens nothing and must not sever a +shared workspace for nothing. + +Shrinking scope needs no restart (`unbindWorkpiece`, `removeGatekeeper`): the prune in step 2 +handles it at the next open, and a narrower scope can never under-verify. Role *rises* +(`addCollaborator`, share-key redemption) are deliberately not triggers either — a live session's +capability set is fixed at open, so raising someone's graph role does not widen the session they +already hold. + +The restart is what makes `addGatekeeper()`'s publication order load-bearing. The DO's input gate +is open across the gatekeeper's `describe()` and ids are allocated sequentially, so publishing the +record before that await would let a live `build` session guess the id and `openSession()` on the +owner's brand-new connection — which gates on nothing but record existence — for as long as +`describe()` took, all of it before the restart severed it. The record is therefore published +exactly once, after `describe()` resolves; `getGatekeeperFacet(id, cls?)` takes the class directly +so nothing needs the early put. + +Enforcement is therefore at admission, within the ~100 ms abort delay of the moment the change is +determined. For the three widening triggers that moment is the change itself; for the scrub trigger +it is the point at which the failure becomes terminal, which the failing collaborator can defer by +leaving a re-prompt unanswered — worth no more to them than never re-opening at all (edge case 3). ### Step 4 — Frontend: the configuration modal @@ -365,8 +432,10 @@ downgrades — see the matching methods on `OverseerClientInterface` and `Sharin 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 (the leak-relevant gate is - `authorizeObservation`, which keys off the live sharing graph). + 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. > 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, @@ -401,12 +470,30 @@ already in the JSDoc in `gatekeeper.ts`; add anything missing there rather than throws and denies the open. 3. **Underlying resource access revoked** — caught at the next open because `addObserver` re-runs the live check and throws; the open is denied. Consistent with the lazy-revocation - model in `sharing.ts`. + model in `sharing.ts`. The denial also scrubs each failed gatekeeper from the collaborator's + persisted observer record, so the record stops claiming a verification that no longer holds; and + because the collaborator may hold *other* sessions that opened while it did, the scrub restarts + the workspace when the denial is determined (see "Restarting when verification scope widens"), + forcing every session on it to re-open and re-verify. "When determined" is later than the scrub + itself: the failing client is offered a re-prompt first, and one it never answers defers the + restart for as long as it stays unanswered — the same residual as never re-opening. The residual + under the lazy model is otherwise unchanged: a collaborator who never opens again is never + asked, so nothing detects their revocation and nothing severs the session they already hold. An + 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. **`prohibitAllSharing` 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. 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). + resource (inherent to the security model). Because that next open is what verifies them, the + addition restarts a shared workspace (see "Restarting when verification scope widens"): every + client reconnects within ~100 ms and re-opens at the new scope, so no session keeps watching a + connection its holder was never verified against. A connection added *while a collaborator's + verification is parked* on an await (the modal, verifier RPCs) is covered by the same restart: + their committed record lacks an entry for the new connection, and the restart forces the open + that adds one. The residual is the ~100 ms window itself, which is inside the revocation window + the sharing model already accepts. 6. **Performance** — `ensureObserver` does one `getVerifier` + one `addObserver` per in-scope gatekeeper per open. Parallelize with `Promise.all` and pipe the verifier promise straight into `addObserver`. Expensive gatekeepers cache on their side. diff --git a/docs/sharing.md b/docs/sharing.md index 8bb4fc467..dacdd02d6 100644 --- a/docs/sharing.md +++ b/docs/sharing.md @@ -150,13 +150,15 @@ 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. -### Terminating live sessions on revocation +### 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.scheduleRevocationRestart`): 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. -Note this is only needed for removals/downgrades. Granting or raising access never strands anyone, and `prohibitAllSharing` 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. +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. `prohibitAllSharing` 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 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.#restartIfShared` restarts the workspace whenever that happens -- a connection is added, one is bound into a gadget, a merge promotes such a binding, or a re-verification failure scrubs a previously-persisted account choice -- so every client re-opens and re-runs `ensureObserver` at the new scope. It is a no-op when the workspace has no collaborators, so a solo workspace 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. ## Future work From 5cbabf2f89e1ed9dce8bf2146534fcd610a7ab9e Mon Sep 17 00:00:00 2001 From: Maximo Guk <62088388+Maximo-Guk@users.noreply.github.com> Date: Fri, 28 Aug 2026 14:40:21 -0500 Subject: [PATCH 08/14] typed-storage: Let a schema declare the storage key it lives under. A schema property name is also the KV key it maps to, so renaming a property in code is a storage migration. Give a singleton slot somewhere to say otherwise: `singleton(defaultValue, {storageKey})` declares the key on disk explicitly, and a bare default value stays the shorthand for the common case and behaves exactly as before. Collections get the same option as `storageName`, which prefixes the records and every index alike. This is the schema-level version of what would otherwise be a special case at each call site, and it keeps the old name on disk with no migration. --- .../typed-storage/__tests__/index.test.ts | 109 +++++++++++++++++- packages/typed-storage/src/index.ts | 56 +++++++-- 2 files changed, 156 insertions(+), 9 deletions(-) diff --git a/packages/typed-storage/__tests__/index.test.ts b/packages/typed-storage/__tests__/index.test.ts index 78d816dd3..fb98d81f0 100644 --- a/packages/typed-storage/__tests__/index.test.ts +++ b/packages/typed-storage/__tests__/index.test.ts @@ -1,5 +1,6 @@ import { expect, it, describe } from "vitest" -import { createTypedStorage, collection, UniqueIndex, NonUniqueIndex } from "../src/index.js"; +import { createTypedStorage, collection, singleton, UniqueIndex, NonUniqueIndex } + from "../src/index.js"; import { DurableObjectListOptions, DurableObjectStorage } from "@cloudflare/workers-types/experimental"; // We mock out DurableObjectStorage becaues otherwise we'd have to run the tests inside a @@ -137,6 +138,112 @@ describe("singletons", () => { storage.counter.put(555); expect(subscriber.lastValue).toStrictEqual(321); }); + + it("uses the property name as the storage key by default", () => { + let mockStorage = makeMockStorage(); + let storage = createTypedStorage(mockStorage, { + singletons: { + counter: singleton(0), + } + }); + + storage.counter.put(123); + + // Declaring a singleton with no options must be byte-identical on disk to a bare default. + expect(mockStorage.kv.get("counter")).toStrictEqual(123); + }); + + it("reads and writes a legacy storage key", () => { + let mockStorage = makeMockStorage(); + + // Data written by an earlier version of the schema, when the property was called `oldName`. + mockStorage.kv.put("oldName", 42); + + let storage = createTypedStorage(mockStorage, { + singletons: { + newName: singleton(0, {storageKey: "oldName"}), + } + }); + + expect(storage.newName.get()).toStrictEqual(42); + + storage.newName.put(43); + + expect(storage.newName.get()).toStrictEqual(43); + expect(mockStorage.kv.get("oldName")).toStrictEqual(43); + expect(mockStorage.kv.get("newName")).toBeUndefined(); + }); + + it("falls back to the default when the legacy key was never written", () => { + let mockStorage = makeMockStorage(); + let storage = createTypedStorage(mockStorage, { + singletons: { + newName: singleton(false, {storageKey: "oldName"}), + } + }); + + expect(storage.newName.get()).toStrictEqual(false); + + storage.newName.put(true); + + expect(mockStorage.kv.get("oldName")).toStrictEqual(true); + }); + + it("notifies subscribers for a legacy storage key", () => { + let mockStorage = makeMockStorage(); + let storage = createTypedStorage(mockStorage, { + singletons: { + newName: singleton(0, {storageKey: "oldName"}), + } + }); + + let subscriber = { + lastValue: -1, + update(value: number) { + this.lastValue = value; + } + }; + storage.newName.subscribe(subscriber); + + storage.newName.put(7); + + expect(subscriber.lastValue).toStrictEqual(7); + expect(mockStorage.kv.get("oldName")).toStrictEqual(7); + }); +}); + +describe("collections with a legacy storage name", () => { + it("stores records and indexes under the legacy prefix", () => { + let mockStorage = makeMockStorage(); + let storage = createTypedStorage(mockStorage, { + collections: { + people: collection()({ + storageName: "users", + primaryKey: "name", + uniqueIndexes: { + byUid: (user: User) => user.uid + }, + nonUniqueIndexes: { + byLevel: (user: User) => user.level + } + }) + } + }); + + storage.people.put(ALICE); + + expect(storage.people.get("alice")).toStrictEqual(ALICE); + expect(storage.people.byUid.get(45)).toStrictEqual(ALICE); + expect([...storage.people.byLevel.list(8)]).toStrictEqual([ALICE]); + + // Every key -- the record and both indexes -- lives under the legacy name, so a collection + // renamed in code reads data written before the rename. + let keys = [...mockStorage.kv.list({})].map(([key]) => key); + expect(keys.some(key => key.startsWith("users:"))).toStrictEqual(true); + expect(keys.some(key => key.startsWith("users.byUid:"))).toStrictEqual(true); + expect(keys.some(key => key.startsWith("users.byLevel:"))).toStrictEqual(true); + expect(keys.some(key => key.startsWith("people"))).toStrictEqual(false); + }); }); type User = { diff --git a/packages/typed-storage/src/index.ts b/packages/typed-storage/src/index.ts index 757a73d2c..f55be9f60 100644 --- a/packages/typed-storage/src/index.ts +++ b/packages/typed-storage/src/index.ts @@ -172,6 +172,7 @@ interface CollectionSchema< primaryKey: PrimaryKey; uniqueIndexes?: UniqueIndexes; nonUniqueIndexes?: NonUniqueIndexes; + storageName?: string; } export function collection() { @@ -182,12 +183,46 @@ export function collection() { primaryKey: PrimaryKey, uniqueIndexes?: UniqueIndexes, nonUniqueIndexes?: NonUniqueIndexes, + /** + * The name this collection's keys (records and indexes alike) are prefixed with, + * overriding the schema property name. Like `SingletonOptions.storageKey`, this lets the + * code be renamed without migrating what is already on disk. + */ + storageName?: string, }) : CollectionSchema { return options as (CollectionSchemaBrand & typeof options); } } +/** Options for a singleton slot declared with `singleton()` rather than a bare default value. */ +export interface SingletonOptions { + /** + * The KV key this slot lives under, overriding the schema property name. Renaming a schema + * property is otherwise a storage migration, since the property name *is* the key; declaring the + * old key here renames the code without touching what is already on disk. + */ + storageKey?: string; +} + +/** + * A singleton slot declared with options. Returned by `singleton()`; a class rather than a plain + * branded object so `createTypedStorage` can tell it apart at runtime from a default value that + * happens to be an object. + */ +export class SingletonSchema { + constructor(readonly defaultValue: T, readonly options: SingletonOptions) {} +} + +/** + * Declares a singleton slot that needs options. A bare default value stays the shorthand for the + * common case (`{singletons: {count: 0}}`) and behaves identically. + */ +export function singleton( + defaultValue: T, options: SingletonOptions = {}): SingletonSchema { + return new SingletonSchema(defaultValue, options); +} + // ======================================================================================= type CollectionImpl = TypedStorage ? CollectionImpl : never } & { - [K in keyof Singletons]: Singleton; + [K in keyof Singletons]: Singletons[K] extends SingletonSchema + ? Singleton : Singleton; }; export function keyString(key: Key): string { @@ -681,15 +717,19 @@ export function createTypedStoragecolSchema); + let storageName = (>colSchema).storageName; + result[colName] = createCollection(storage, storageName ?? colName, colSchema); } - for (let [key, defaultValue] of Object.entries(schema.singletons || {})) { + for (let [key, slotSchema] of Object.entries(schema.singletons || {})) { + let defaultValue = slotSchema instanceof SingletonSchema ? slotSchema.defaultValue : slotSchema; + let storageKey = slotSchema instanceof SingletonSchema + ? slotSchema.options.storageKey ?? key : key; let subscribers = new Set>(); - let singleton: Singleton = { + let slot: Singleton = { get(): any { - let result = storage.kv.get(key); + let result = storage.kv.get(storageKey); if (result === undefined) { result = defaultValue; } @@ -698,13 +738,13 @@ export function createTypedStorage { for (let subscriber of subscribers) { subscriber.update(value); } - storage.kv.put(key, value); + storage.kv.put(storageKey, value); }); } }, @@ -718,7 +758,7 @@ export function createTypedStorage Date: Fri, 28 Aug 2026 14:44:09 -0500 Subject: [PATCH 09/14] Refactor: Rename prohibitAllSharing -> containsRestrictedData. The flag's real meaning is "this observation contains restricted data". What the platform does about that is policy, which shouldn't be baked into the name -- the next commits replace the all-or-nothing lockdown with per-collaborator observer verification. ObservationDescription.prohibitAllSharing and GadgetMetadata.sharingProhibited both become containsRestrictedData. No alias: this is a hard rename, so the gatekeeper call sites move in the same commit. The overseer's durable singleton is renamed too, and declares its old name as its `storageKey` so nothing on disk moves. Without that, every workspace that has already observed restricted data would silently unlatch. Co-Authored-By: Claude Opus 5 --- .agents/skills/write-gatekeeper/SKILL.md | 2 +- docs/observers.md | 14 +++--- docs/sharing.md | 2 +- .../__tests__/drive-session.test.ts | 2 +- packages/gatekeeper-google/src/google.ts | 18 ++++---- packages/gatekeeper-mcp/README.md | 11 ++--- .../workshop-backend/__tests__/fixtures.ts | 2 +- .../__tests__/observer-coverage-scrub.test.ts | 2 +- packages/workshop-backend/src/overseer.ts | 44 +++++++++---------- packages/workshop-backend/src/sharing.ts | 6 +-- packages/workshop-frontend/src/ShareModal.tsx | 26 +++++------ packages/workshop-shared/src/api.ts | 2 +- packages/workshop-shared/src/gatekeeper.ts | 2 +- 13 files changed, 67 insertions(+), 66 deletions(-) diff --git a/.agents/skills/write-gatekeeper/SKILL.md b/.agents/skills/write-gatekeeper/SKILL.md index 7416bd12d..1e470f73c 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). +- **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. - **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 9e6fb3355..38264bd3e 100644 --- a/docs/observers.md +++ b/docs/observers.md @@ -30,8 +30,8 @@ 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 **`prohibitAllSharing`** flag -(`packages/workshop-shared/src/gatekeeper.ts`, `ObservationDescription.prohibitAllSharing`). +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 @@ -95,7 +95,7 @@ This feature replaces that all-or-nothing posture with a per-user, gatekeeper-me | Session restart when verification scope widens | `overseer.ts` (`#restartIfShared`, `scheduleAccessRestart`) | | Server `openGadget` path | `packages/workshop-backend/src/server.ts:206` | | Role resolution / permission graph | `packages/workshop-backend/src/sharing.ts` (`getEffectiveRole`, `computeEffectiveRoles`, `hasAnyShares`) | -| `prohibitAllSharing` enforcement | `overseer.ts:1171` (`authorizeObservation`), `:1207` (web fetch), `:1258` (`submitAction`) | +| `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` | @@ -229,7 +229,7 @@ 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 `prohibitAllSharing` short-circuit ahead of +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 confirmed, so it never reveals a workspace's gatekeeper or resource metadata to an unauthorized user. @@ -482,7 +482,7 @@ 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. **`prohibitAllSharing` interaction** — unchanged and still authoritative: if set, no non-owner +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. 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 @@ -561,8 +561,8 @@ 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 `prohibitAllSharing` for these resources (the - `prohibitAllSharing` lockdown mechanism itself is unchanged and remains available separately). + This is the replacement for today's reliance on `containsRestrictedData` for these resources (the + `containsRestrictedData` lockdown mechanism itself is unchanged and remains available separately). `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. diff --git a/docs/sharing.md b/docs/sharing.md index dacdd02d6..652f72fd6 100644 --- a/docs/sharing.md +++ b/docs/sharing.md @@ -156,7 +156,7 @@ Authorization is only checked at `open()`, so a session that is *already* open i 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. -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. `prohibitAllSharing` 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. +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 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.#restartIfShared` restarts the workspace whenever that happens -- a connection is added, one is bound into a gadget, a merge promotes such a binding, or a re-verification failure scrubs a previously-persisted account choice -- so every client re-opens and re-runs `ensureObserver` at the new scope. It is a no-op when the workspace has no collaborators, so a solo workspace 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. diff --git a/packages/gatekeeper-google/__tests__/drive-session.test.ts b/packages/gatekeeper-google/__tests__/drive-session.test.ts index 2d0933809..934ccd902 100644 --- a/packages/gatekeeper-google/__tests__/drive-session.test.ts +++ b/packages/gatekeeper-google/__tests__/drive-session.test.ts @@ -121,7 +121,7 @@ describe("Drive session scope", () => { description: expect.stringContaining('name starts with "missing"'), excludeObservers: ["excluded"], })]); - expect(authorizations[0]).not.toHaveProperty("prohibitAllSharing"); + expect(authorizations[0]).not.toHaveProperty("containsRestrictedData"); expect(authorizations[0].description).not.toContain("0"); expect(events).toEqual(["authorize"]); }); diff --git a/packages/gatekeeper-google/src/google.ts b/packages/gatekeeper-google/src/google.ts index 7acef83f6..603675905 100644 --- a/packages/gatekeeper-google/src/google.ts +++ b/packages/gatekeeper-google/src/google.ts @@ -1735,7 +1735,7 @@ export class GmailGatekeeperImpl extends DurableObject): Promise { @@ -3396,7 +3396,7 @@ class BigQuerySessionImpl extends RpcTarget implements BigQuerySession { `Referenced tables: ${estimate.referencedTables.join(", ")}\n` + `Estimated bytes processed: ${estimate.bytesProcessed.toLocaleString()}\n` + `Maximum bytes billed: ${maxBytes.toLocaleString()}.`, - prohibitAllSharing: true, + containsRestrictedData: true, }); let result = await this.#api.query(billingProject, sql, { @@ -3428,7 +3428,7 @@ class BigQuerySessionImpl extends RpcTarget implements BigQuerySession { description: `Estimated bytes processed: ${estimate.bytesProcessed.toLocaleString()}\n` + `Referenced tables: ${estimate.referencedTables.join(", ") || "(none)"}`, - prohibitAllSharing: true, + containsRestrictedData: true, }); return estimate; @@ -3440,7 +3440,7 @@ class BigQuerySessionImpl extends RpcTarget implements BigQuerySession { await this.#authorizeDatasets([], { title: "Get BigQuery project", description: `Returned the scoped project: \`${this.#scopedProjectId}\`.`, - prohibitAllSharing: true, + containsRestrictedData: true, }); return result; } @@ -3461,7 +3461,7 @@ class BigQuerySessionImpl extends RpcTarget implements BigQuerySession { await this.#authorizeDatasets([{ projectId: p, datasetId: this.#scopedDatasetId }], { title: `List datasets in ${p}`, description: `Returned scoped dataset \`${p}.${this.#scopedDatasetId}\` (1 dataset).`, - prohibitAllSharing: true, + containsRestrictedData: true, }); return [dataset]; } @@ -3471,7 +3471,7 @@ class BigQuerySessionImpl extends RpcTarget implements BigQuerySession { await this.#authorizeDatasets(result.map(ds => ({ projectId: p, datasetId: ds.datasetId })), { title: `List datasets in ${p}`, description: `Listed ${result.length} dataset(s) in \`${p}\`.`, - prohibitAllSharing: true, + containsRestrictedData: true, }); return result; } @@ -3497,7 +3497,7 @@ class BigQuerySessionImpl extends RpcTarget implements BigQuerySession { await this.#authorizeDatasets([{ projectId: p, datasetId: d }], { title: `List tables in ${p}.${d}`, description: `Returned scoped table \`${p}.${d}.${this.#scopedTableId}\` (1 table).`, - prohibitAllSharing: true, + containsRestrictedData: true, }); return [table]; } @@ -3506,7 +3506,7 @@ class BigQuerySessionImpl extends RpcTarget implements BigQuerySession { await this.#authorizeDatasets([{ projectId: p, datasetId: d }], { title: `List tables in ${p}.${d}`, description: `Listed ${result.length} table(s) in \`${p}.${d}\`.`, - prohibitAllSharing: true, + containsRestrictedData: true, }); return result; } @@ -3543,7 +3543,7 @@ class BigQuerySessionImpl extends RpcTarget implements BigQuerySession { title: `Describe ${p}.${d}.${t}`, description: `Described table \`${p}.${d}.${t}\` (${result.schema.length} columns).`, - prohibitAllSharing: true, + containsRestrictedData: true, }); return result; } diff --git a/packages/gatekeeper-mcp/README.md b/packages/gatekeeper-mcp/README.md index 93317ca88..1e993073d 100644 --- a/packages/gatekeeper-mcp/README.md +++ b/packages/gatekeeper-mcp/README.md @@ -177,8 +177,8 @@ rules. A Gadget bound to an MCP server can only be opened by its owner: `addObserver` refuses unconditionally. Being able to authenticate to a server is not evidence of being allowed to see what the *owner* read from it, and the Gadget runs on the owner's credentials throughout. Writes still -work — the alternative, marking every observation `prohibitAllSharing`, would latch a lockdown that -blocks every action for the rest of the session. See +work — the alternative, marking every observation `containsRestrictedData`, would latch a +restricted mode that blocks every action for the rest of the session. See [`sharing-policy.ts`](../mcp-shared/src/sharing-policy.ts). To share the work rather than the binding, publish the Gadget as a blueprint and let each person @@ -206,9 +206,10 @@ connect their own server. compatibility flag in `wrangler.jsonc`, which makes workerd reject reserved IP ranges after resolution on every request and redirect hop. It does not apply under `wrangler dev`, which is what keeps `MCP_ALLOW_INSECURE` usable locally. -- **Sharing UI reports late.** `GadgetMetadata.sharingProhibited` derives only from - `prohibitAllSharing`, so creating a share key appears to succeed and fails when the recipient - opens it. Fixing this needs a kernel change. +- **Sharing UI reports late.** `GadgetMetadata.containsRestrictedData` derives only from + `ObservationDescription.containsRestrictedData`, so creating a share key appears to succeed and + fails when the recipient opens it (their observer verification is refused). Fixing this needs a + kernel change. ## Layout diff --git a/packages/workshop-backend/__tests__/fixtures.ts b/packages/workshop-backend/__tests__/fixtures.ts index b8e15b0e6..b27ed5025 100644 --- a/packages/workshop-backend/__tests__/fixtures.ts +++ b/packages/workshop-backend/__tests__/fixtures.ts @@ -100,7 +100,7 @@ export async function openFakeOverseer( }), }, storage: Object.assign(storage, { - prohibitAllSharing: { get: () => false }, + containsRestrictedData: { get: () => false }, title: { get: () => "Test Workspace" }, }), }, diff --git a/packages/workshop-backend/__tests__/observer-coverage-scrub.test.ts b/packages/workshop-backend/__tests__/observer-coverage-scrub.test.ts index 8cac5d5b0..9fc09ffec 100644 --- a/packages/workshop-backend/__tests__/observer-coverage-scrub.test.ts +++ b/packages/workshop-backend/__tests__/observer-coverage-scrub.test.ts @@ -130,7 +130,7 @@ describe("observer coverage scrub on a failed live check", () => { // Alice was already an admitted observer, so the failure de-registers her from nothing: the // registrations are what make gatekeepers name her in `excludeObservers`, and the scrub does - // not cover the same observations (it gates `prohibitAllSharing` only). + // not cover the same observations (it gates `containsRestrictedData` only). expect(removed).toEqual([]); }); }); diff --git a/packages/workshop-backend/src/overseer.ts b/packages/workshop-backend/src/overseer.ts index 54ecbb86d..072286fcb 100644 --- a/packages/workshop-backend/src/overseer.ts +++ b/packages/workshop-backend/src/overseer.ts @@ -9,7 +9,7 @@ import { DurableObject, WorkerEntrypoint, RpcStub as NativeRpcStub, RpcTarget as NativeRpcTarget, restore, } from "cloudflare:workers"; -import { createTypedStorage, collection, keyString } from "@gadgets/typed-storage"; +import { createTypedStorage, collection, singleton, keyString } from "@gadgets/typed-storage"; import type { ListOptions } from "@gadgets/typed-storage"; import { GitStore, commitIdentityForAuthor, filesEqual, gitObjectsCollection, threeWayMerge } from "./git-store"; @@ -1026,9 +1026,9 @@ export function makeOverseerStorage(storage: DurableObjectStorage) { nextChatId: 0, nextHookId: 0, - // True if any past observation was authorized that had the `prohibitAllSharing` flag set - // in its `ObservationDescription`. - prohibitAllSharing: false, + // 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. + containsRestrictedData: singleton(false, {storageKey: "prohibitAllSharing"}), }, collections: { @@ -4488,7 +4488,7 @@ class OverseerImpl implements AgentHooks { async authorizeObservation(gatekeeperId: number, description: ObservationDescription, caller: GatekeeperCaller): Promise { - if (description.prohibitAllSharing) { + if (description.containsRestrictedData) { if ((await this.getSharingManager()).hasAnyShares()) { throw new Error( "This observation was blocked because it contains sensitive data that must only be " + @@ -4496,7 +4496,7 @@ class OverseerImpl implements AgentHooks { "from a workspace that is not shared."); } - this.storage.prohibitAllSharing.put(true); + this.storage.containsRestrictedData.put(true); } // Forward exclusion: the gatekeeper may name observers who must not see this observation. Since @@ -4660,7 +4660,7 @@ class OverseerImpl implements AgentHooks { // Provides web-fetch with the Workers AI binding and AI Gateway config it needs to call // `env.WORKERS_AI.toMarkdown()`. The initiator is needed for AI Gateway metadata. getWebFetchEnv(): WebFetchEnv { - if (this.storage.prohibitAllSharing.get()) { + if (this.storage.containsRestrictedData.get()) { // TODO: Disallwing fetches is a bit draconian. Ideally, we would have some way to detect // if a URL is well-known, and therefore not a leak problem. E.g. if the URL is already in // a search index, then it's not leaking anything. If we had a search provider we could @@ -4709,7 +4709,7 @@ class OverseerImpl implements AgentHooks { async submitAction(gatekeeperId: number, action: number, description: ActionDescription, caller: GatekeeperCaller) : Promise { - if (this.storage.prohibitAllSharing.get()) { + if (this.storage.containsRestrictedData.get()) { throw new Error( "This workspace has observed sensitive data. To prevent leaks, the workspace is prohibited " + "from performing actions."); @@ -8502,8 +8502,8 @@ export class OverseerDurableObject extends DurableObject { let role: CollaboratorRole = "build"; if (!isOwner) { - if (this.impl.storage.prohibitAllSharing.get()) { - // `prohibitAllSharing` can only have been set when the gadget had no shares (see + 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); @@ -8529,7 +8529,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 prohibitAllSharing short-circuit above still wins over both. + // unauthorized user; the containsRestrictedData short-circuit above still wins over both. // // 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 @@ -8620,7 +8620,7 @@ 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.prohibitAllSharing.get()) { + if (this.impl.storage.containsRestrictedData.get()) { return { accepted: false, message: "This workspace has sharing disabled, so only its owner can access it.", @@ -9315,7 +9315,7 @@ class OverseerClientInterface extends RpcTarget implements Overseer { id: this.impl.ctx.id.toString(), title: this.impl.storage.title.get(), totalCost: this.impl.storage.totalCost.get(), - sharingProhibited: this.impl.storage.prohibitAllSharing.get(), + containsRestrictedData: this.impl.storage.containsRestrictedData.get(), role: "build", defaultGadgetId: this.impl.defaultGadgetId, }; @@ -9334,7 +9334,7 @@ class OverseerClientInterface extends RpcTarget implements Overseer { id: this.impl.ctx.id.toString(), title: this.impl.storage.title.get(), totalCost: this.impl.storage.totalCost.get(), - sharingProhibited: this.impl.storage.prohibitAllSharing.get(), + containsRestrictedData: this.impl.storage.containsRestrictedData.get(), role: "build", defaultGadgetId: this.impl.defaultGadgetId, }; @@ -9356,9 +9356,9 @@ class OverseerClientInterface extends RpcTarget implements Overseer { callback(metadata).catch(unsubscribe); } }; - let sharingProhibitedSubscriber = { + let restrictedDataSubscriber = { update(value: boolean | undefined) { - metadata.sharingProhibited = value; + metadata.containsRestrictedData = value; callback(metadata).catch(unsubscribe); } }; @@ -9366,13 +9366,13 @@ class OverseerClientInterface extends RpcTarget implements Overseer { let unsubscribe = () => { this.impl.storage.title.unsubscribe(titleSubscriber); this.impl.storage.totalCost.unsubscribe(costSubscriber); - this.impl.storage.prohibitAllSharing.unsubscribe(sharingProhibitedSubscriber); + this.impl.storage.containsRestrictedData.unsubscribe(restrictedDataSubscriber); callback[Symbol.dispose](); }; this.impl.storage.title.subscribe(titleSubscriber); this.impl.storage.totalCost.subscribe(costSubscriber); - this.impl.storage.prohibitAllSharing.subscribe(sharingProhibitedSubscriber); + this.impl.storage.containsRestrictedData.subscribe(restrictedDataSubscriber); callback(metadata).catch(unsubscribe); @@ -10582,7 +10582,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, the `prohibitAllSharing` policy) and + // the RPC-bound pieces (resolving profiles via User DOs, the `containsRestrictedData` policy) and // delegate the rest. async listObserverRequirements( @@ -10604,7 +10604,7 @@ class OverseerClientInterface extends RpcTarget implements Overseer { return null; } - if (this.impl.storage.prohibitAllSharing.get()) { + if (this.impl.storage.containsRestrictedData.get()) { throw new Error( "This workspace has observed sensitive data. To prevent leaks, the workspace cannot be " + "shared."); @@ -10665,7 +10665,7 @@ class OverseerClientInterface extends RpcTarget implements Overseer { async createShareLink(role: CollaboratorRole, note?: string) : Promise<{ key: string; linkId: string }> { - if (this.impl.storage.prohibitAllSharing.get()) { + if (this.impl.storage.containsRestrictedData.get()) { throw new Error( "This workspace has observed sensitive data. To prevent leaks, the workspace cannot be " + "shared."); @@ -10676,7 +10676,7 @@ class OverseerClientInterface extends RpcTarget implements Overseer { } async newShareLinkKey(linkId: string): Promise<{ key: string }> { - if (this.impl.storage.prohibitAllSharing.get()) { + if (this.impl.storage.containsRestrictedData.get()) { throw new Error( "This workspace has observed sensitive data. To prevent leaks, the workspace cannot be " + "shared."); diff --git a/packages/workshop-backend/src/sharing.ts b/packages/workshop-backend/src/sharing.ts index f11123922..e3fbe48d8 100644 --- a/packages/workshop-backend/src/sharing.ts +++ b/packages/workshop-backend/src/sharing.ts @@ -15,7 +15,7 @@ // 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 `prohibitAllSharing` policy flag intentionally does NOT live here. It is a broader +// 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 @@ -165,7 +165,7 @@ export class SharingManager { /** * True if anyone other than the owner can currently access the gadget. Used by the Overseer's - * `prohibitAllSharing` policy to decide whether a sensitive observation must be blocked. + * `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* @@ -293,7 +293,7 @@ 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. - * `prohibitAllSharing`). The caller may not grant a role higher than their own effective role. + * `containsRestrictedData`). The caller may not grant a role higher than their own effective role. */ addCollaborator(opts: { caller: SharingCaller; diff --git a/packages/workshop-frontend/src/ShareModal.tsx b/packages/workshop-frontend/src/ShareModal.tsx index b6212442d..e395e289e 100644 --- a/packages/workshop-frontend/src/ShareModal.tsx +++ b/packages/workshop-frontend/src/ShareModal.tsx @@ -372,7 +372,7 @@ export default function ShareModal({ open, onClose, overseer, metadata, currentU }, []) const isOwner = !metadata.owner - const sharingProhibited = metadata.sharingProhibited === true + const containsRestrictedData = metadata.containsRestrictedData === true const loadData = useCallback(async () => { try { @@ -559,7 +559,7 @@ export default function ShareModal({ open, onClose, overseer, metadata, currentU const handleAddCollaborator = async () => { const username = addUsername.trim() - if (!username || sharingProhibited || addingRef.current) return + if (!username || containsRestrictedData || addingRef.current) return addingRef.current = true setAdding(true) @@ -585,7 +585,7 @@ export default function ShareModal({ open, onClose, overseer, metadata, currentU } const handleCreateShareLink = async () => { - if (sharingProhibited || creatingLinkRef.current) return + if (containsRestrictedData || creatingLinkRef.current) return creatingLinkRef.current = true setCreatingLink(true) try { @@ -611,7 +611,7 @@ export default function ShareModal({ open, onClose, overseer, metadata, currentU // Copy a share link again. Secrets are never stored, so the previously-shown URL can't be // re-displayed. We mint a new secret for the same logical link and copy that. const handleCopyShareLink = async (linkId: string) => { - if (sharingProhibited || copyingLinkRef.current) return + if (containsRestrictedData || copyingLinkRef.current) return copyingLinkRef.current = true setCopyingLinkId(linkId) try { @@ -775,7 +775,7 @@ export default function ShareModal({ open, onClose, overseer, metadata, currentU className="chat-panel min-h-0 flex-1 overflow-y-auto overscroll-contain px-4 pb-6 sm:px-6" onScroll={(e) => setScrolled(e.currentTarget.scrollTop > 0)} > - {sharingProhibited ? ( + {containsRestrictedData ? (
@@ -821,20 +821,20 @@ export default function ShareModal({ open, onClose, overseer, metadata, currentU data-bwignore="true" data-form-type="other" className="h-9 min-w-0 flex-1 appearance-none border-0 bg-transparent p-0 text-[14px] leading-5 tracking-[-0.25px] text-kumo-default outline-none placeholder:text-kumo-inactive disabled:cursor-not-allowed [&::-webkit-search-cancel-button]:hidden" - disabled={sharingProhibited} + disabled={containsRestrictedData} /> {adding ? 'Inviting…' : 'Invite'} @@ -911,16 +911,16 @@ export default function ShareModal({ open, onClose, overseer, metadata, currentU placeholder="Name this link (optional)…" aria-label="Share link name (optional)" className="h-9 min-w-0 flex-1 border-0 bg-transparent p-0 text-[14px] leading-5 tracking-[-0.25px] text-kumo-default outline-none placeholder:text-kumo-inactive" - disabled={creatingLink || sharingProhibited} + disabled={creatingLink || containsRestrictedData} /> - + {creatingLink ? 'Creating…' : 'Create link'} setShowLinkComposer(false)}> @@ -932,7 +932,7 @@ export default function ShareModal({ open, onClose, overseer, metadata, currentU