diff --git a/.agents/skills/write-gatekeeper/SKILL.md b/.agents/skills/write-gatekeeper/SKILL.md index 1e470f73c..0d8a0b49b 100644 --- a/.agents/skills/write-gatekeeper/SKILL.md +++ b/.agents/skills/write-gatekeeper/SKILL.md @@ -243,7 +243,7 @@ async getVerifier(): Promise> { Strategy is chosen **per `Gatekeeper` DO class / binding**, not per package — one package may use several (e.g. Google: Gmail=A, Doc=B, BigQuery=C). -- **A — Private-only.** `addObserver()` always throws; `removeObserver()` is a no-op. `getVerifier()` must still exist (the overseer mints it) but is never consulted. Use when the resource is too sensitive to share and there is no per-observer access oracle (e.g. a personal Gmail mailbox). For truly sensitive data, also mark each observation with `ObservationDescription.containsRestrictedData: true`: the workspace then refuses sensitive observations while any unverified collaborator has access (with strategy A that is every collaborator) and latches into a restricted mode that blocks all actions and public web fetches, so the data cannot leak back out through other gatekeepers. +- **A — Private-only.** `addObserver()` always throws; `removeObserver()` is a no-op. `getVerifier()` must still exist (the overseer mints it) but is never consulted. Use when the resource is too sensitive to share and there is no per-observer access oracle (e.g. a personal Gmail mailbox). For truly sensitive data, also mark each observation with `ObservationDescription.containsRestrictedData: true`: because nobody can open the workspace without passing `addObserver()`, and with strategy A nobody ever does, the first such observation makes the workspace effectively unshareable — and it latches into a restricted mode that blocks all actions and public web fetches, so the data cannot leak back out through other gatekeepers. - **B — ACL check (single unit).** The binding is one atomic resource; sub-resources inherit its ACL. `addObserver()` calls a verifier method to confirm the observer can access it and throws otherwise; `removeObserver()` is a no-op; nothing is tracked and no `excludeObservers` is ever needed. Use for repo / document / page / team / single-project bindings. - **C — Data-set tracking.** The binding spans sub-resources with **distinct ACLs**, and there is a **per-observer access oracle** for each. The DO logs the data sets actually observed and the current observers; `addObserver()` verifies the observer against **every** logged set (plus a coarse membership baseline) and **stores their verifier**; each later observation that first touches a **new** set re-checks all stored observers and sets `excludeObservers` for any who fail. Use for workspace / organization / dataset-spanning bindings. - **D — Low-stakes.** `addObserver()` / `removeObserver()` are no-ops; `getVerifier()` returns a trivial verifier with a no-op public method such as `verify(): void {}` (an empty `WorkerEntrypoint` is not registered in `ctx.exports`). Use when any collaborator may observe (personal, low-stakes services). diff --git a/docs/observers.md b/docs/observers.md index 0dc8db05e..7b7a32be7 100644 --- a/docs/observers.md +++ b/docs/observers.md @@ -30,14 +30,15 @@ Gadgets enforce a core security invariant (see `overview.md` §"Security Model") > able to read that information will also be prohibited from interacting with the Gadget, > to prevent data leaks. -Today the only mechanism enforcing this is the blunt **`containsRestrictedData`** flag -(`packages/workshop-shared/src/gatekeeper.ts`, `ObservationDescription.containsRestrictedData`). -When a gatekeeper marks an observation as maximally sensitive, the Gadget can no longer be -shared with *anyone*, and it drops into "lockdown" (no further actions, no web fetches). This -is a deliberate stopgap — it cannot express "this data may be shared, but only with people who -*also* have access to it." +The mechanism is a per-user, gatekeeper-mediated check — "this data may be shared, but only with +people who *also* have access to it". (Maximally sensitive data gets an extra layer: an +observation marked **`containsRestrictedData`** +(`ObservationDescription.containsRestrictedData` in `packages/workshop-shared/src/gatekeeper.ts`) +latches the workspace into a restricted mode — no actions, no web fetches. Its coverage rests on +admission: nobody can open the workspace without being verified against the producing gatekeeper, +and anything that widens what they must be verified against restarts every live session.) -This feature replaces that all-or-nothing posture with a per-user, gatekeeper-mediated check: +The check works as follows: - **Observers.** Every non-owner who can see data the Gadget read is an *observer*. When a user becomes an observer, each relevant gatekeeper is asked — via `Gatekeeper.addObserver()` — to @@ -56,7 +57,7 @@ This feature replaces that all-or-nothing posture with a per-user, gatekeeper-me `ObservationDescription.excludeObservers`. The overseer must then guarantee those observers never see it, or block the observation. -**The API is already committed** (commit `e2f1707`). The relevant interfaces are +**The API is already committed.** The relevant interfaces are `GatekeeperUser.getVerifier()`, `GatekeeperUserVerifier`, `Gatekeeper.addObserver()` / `removeObserver()`, and `ObservationDescription.excludeObservers`, all in `packages/workshop-shared/src/gatekeeper.ts`. @@ -68,8 +69,8 @@ This feature replaces that all-or-nothing posture with a per-user, gatekeeper-me - **Role-based breadth of verification:** - **`build`** collaborators (full access — chat + code + all bindings) must be verified against **every** gatekeeper the Gadget has. - - **`use`** collaborators (UI only, no chat access — see `UseOverseerInterface`, - `overseer.ts:2816`) must be verified only against gatekeepers their sessions can actually + - **`use`** collaborators (UI only, no chat access — see `UseOverseerInterface` in + `overseer.ts`) must be verified only against gatekeepers their sessions can actually reach: those **bound by some gadget** (the UI can invoke them), those with an **enabled hook** (a hook is a live write channel into a gadget they can open, delivering the connection's data regardless of binding edges), plus — transitively — every **env target of a @@ -94,20 +95,20 @@ This feature replaces that all-or-nothing posture with a per-user, gatekeeper-me | Concern | Location | |---|---| | Gatekeeper RPC API (the committed surface) | `packages/workshop-shared/src/gatekeeper.ts` | -| Overseer DO, `open()` auth entry point | `packages/workshop-backend/src/overseer.ts:2714` | +| Overseer DO, `open()` auth entry point | `packages/workshop-backend/src/overseer.ts` | | Authorization gate shared by `open()` and `receiveExternalMessage()` | `overseer.ts` `authorizeCollaborator()` | | Session restart when verification scope widens | `overseer.ts` (`#restartIfSessionsAffected`, `joinSession`, `scheduleAccessRestart`) | -| Server `openGadget` path | `packages/workshop-backend/src/server.ts:206` | -| Role resolution / permission graph | `packages/workshop-backend/src/sharing.ts` (`getEffectiveRole`, `computeEffectiveRoles`, `hasAnyShares`) | -| `containsRestrictedData` enforcement | `overseer.ts:1171` (`authorizeObservation`), `:1207` (web fetch), `:1258` (`submitAction`) | -| Observation recording | `overseer.ts:1169` `authorizeObservation()`; `ApprovalQueueImpl` `overseer.ts:4856` | -| Gatekeeper storage record | `overseer.ts:110` `GatekeeperRecord` (has `creationSpec.vendorId`) | -| `GatekeeperCreationSpec` | `packages/workshop-shared/src/api.ts:1345` | -| Gatekeeper facet access | `overseer.ts:1079` `getGatekeeperFacet()` | -| Overseer storage collections | `overseer.ts:316` (`gatekeepers`, with `byBindingName` index — template for a new collection) | -| Connected accounts (User DO) | `packages/workshop-backend/src/user.ts:12` `ConnectedAccountRecord` (`account: Fetcher`, `vendorId`) | -| List connected accounts | `user.ts:890` `subscribeConnectedAccounts()`; subscriber type `api.ts:116` | -| Account → gatekeeper class | `user.ts:1136` `getGatekeeperClassFor()` | +| Server `openGadget` path | `packages/workshop-backend/src/server.ts` | +| Role resolution / permission graph | `packages/workshop-backend/src/sharing.ts` (`getEffectiveRole`, `computeEffectiveRoles`) | +| `containsRestrictedData` enforcement | `overseer.ts` (`authorizeObservation`'s latch, `getWebFetchEnv`, `submitAction`) | +| Observation recording | `overseer.ts` `authorizeObservation()`; `ApprovalQueueImpl` | +| Gatekeeper storage record | `overseer.ts` `GatekeeperRecord` (has `creationSpec.vendorId`) | +| `GatekeeperCreationSpec` | `packages/workshop-shared/src/api.ts` | +| Gatekeeper facet access | `overseer.ts` `getGatekeeperFacet()` | +| Overseer storage collections | `overseer.ts` (`gatekeepers`, with `byBindingName` index — template for a new collection) | +| Connected accounts (User DO) | `packages/workshop-backend/src/user.ts` `ConnectedAccountRecord` (`account: Fetcher`, `vendorId`) | +| List connected accounts | `user.ts` `subscribeConnectedAccounts()`; subscriber type in `api.ts` | +| Account → gatekeeper class | `user.ts` `getGatekeeperClassFor()` | --- @@ -140,8 +141,8 @@ This feature replaces that all-or-nothing posture with a per-user, gatekeeper-me ### New overseer storage collection: `observers` -Add an `observers` collection to `OverseerStorage` (mirror the `gatekeepers` collection at -`overseer.ts:316`, including a secondary index for reverse lookup): +Add an `observers` collection to `OverseerStorage` (mirror the `gatekeepers` collection in +`overseer.ts`, including a secondary index for reverse lookup): ```ts type ObserverRecord = { @@ -173,7 +174,7 @@ log) lives inside each gatekeeper's own DO and is out of scope here. ### Step 1 — User DO: mint a verifier for a chosen account Add a method to the User DO (`packages/workshop-backend/src/user.ts`), near -`getGatekeeperClassFor` (`user.ts:1136`): +`getGatekeeperClassFor`: ```ts // Mint a verifier from one of THIS user's connected accounts, identified by accountId. @@ -203,7 +204,7 @@ invoked **only** when the opening user needs to configure gatekeeper accounts. I without an extra round trip. Add to the RPC API (`packages/workshop-shared/src/api.ts`) and thread through -`server.ts:206` → `overseer.open()` (`overseer.ts:2714`): +`server.ts` `openGadget` → `overseer.open()`: ```ts // Provided by the client when opening a gadget. Invoked by the overseer only if the opening @@ -233,11 +234,19 @@ type ObserverAccountChoice = { ### Step 3 — Overseer: observer configuration & re-verification at `open()` Hook into `open()` in the non-owner branch, after `effectiveRole` is confirmed and before -constructing the client interface. Keep the existing `containsRestrictedData` short-circuit ahead of -this -- lockdown still wins. The `NeedsConnections` signal is produced only *after* a valid role is +constructing the client interface. (Observer verification *is* the open()-time enforcement for +sensitive data; no `containsRestrictedData` check precedes it.) +The `NeedsConnections` signal is produced only *after* a valid role is confirmed, so it never reveals a workspace's gatekeeper or resource metadata to an unauthorized user. +Role resolution plus verification is one shared gate, `OverseerImpl.authorizeCollaborator`, and +every non-owner entry point that can surface workspace data runs it — `open()` interactively, and +`receiveExternalMessage()` non-interactively (no configuration channel, so an unverified caller is +told to open the workspace, which is where verification happens). An agent reply on the external +path can surface anything the workspace already read, so it must not admit a collaborator with +less verification than `open()` would demand. + Add a private helper on `OverseerImpl`, roughly: ```ts @@ -286,13 +295,14 @@ Logic: **throws** on a mismatch. This server-side check is what guarantees a gatekeeper only receives a verifier minted by its own vendor; filtering account choices in the client is only a user-interface convenience. - - If any `addObserver` **throws** (or `getVerifier` throws on vendor mismatch), the user is not - (or no longer) allowed. Every such failure goes through one `fail()` path, and the user is - offered a bounded number of re-prompts to repair (e.g. re-authenticate an expired account). On - terminal failure the open is denied with a message naming each refused binding, and the - registrations this call added are best-effort-removed while no record is persisted. The - persisted `accountChoices` are left as they are: an entry records the choice the user made so - they are not asked again, and asserts nothing about whether the gatekeeper still admits them. + - If any `addObserver` **throws** (or `getVerifier` throws on vendor mismatch, or returns null + for a disconnected account), the user is not (or no longer) allowed. Every such failure goes + through one `fail()` path, and the user is offered a bounded number of re-prompts to repair + (e.g. re-authenticate an expired account). On terminal failure the open is denied with a + message naming each refused binding, and the registrations this call added are + best-effort-removed while no record is persisted. The persisted `accountChoices` are left as + they are: an entry records the choice the user made so they are not asked again, and asserts + nothing about whether the gatekeeper still admits them. - Only a *first-ever* verification rolls anything back (fully — nothing referenced its registrations before the call, and the minted id would otherwise linger unresolvable). A returning observer's registrations are **all** kept, including ones this call added: their @@ -325,13 +335,6 @@ Notes: stored account choices. The modal is only for genuinely uncovered bindings (first open, a binding the owner added after this user last configured, or an ambient binding without a matching provided account). -- **Role resolution and verification belong together.** Both live behind one - `authorizeCollaborator(profileId, clientUser, {configureCb?, requireRole?})`, so every non-owner - entry point applies the same gate. `receiveExternalMessage()` — the chat-integration path, whose - agent reply can surface anything the workspace has already read — passes `requireRole: "build"` - and no `configureCb`: it has no channel to prompt on, so an unverified caller is told to open the - workspace in a browser, and an insufficient role is denied *before* verification runs rather than - being sent to fix a failure that could never grant them access anyway. #### Restarting when verification scope widens @@ -497,12 +500,16 @@ restart-check, and mark share one synchronous block, so no request can interleav change appearing and the block taking effect; marks are only ever set when a restart is scheduled, since nothing else would clear them. +Enforcement is therefore at admission, within the ~100 ms abort delay of the moment the change is +determined — the change itself, for every trigger — and held to the collaborator's role scope +(edge case 4 below). + ### Step 4 — Frontend: the configuration modal Implement the `ObserverConfigCallback` on the client. When the overseer calls `configure(needs)`: 1. For each `ObserverBindingNeed`, find the user's candidate accounts by filtering the existing - `subscribeConnectedAccounts()` results (`user.ts:890`) by `need.vendorId`. + `subscribeConnectedAccounts()` results by `need.vendorId`. 2. If one or more accounts match, pre-select one arbitrarily as the default; let the user change it via a dropdown. (Most users have one account per vendor and will just click "OK".) 3. Include forced auto-provisioned accounts in the subscription. If **no** account matches, use @@ -518,7 +525,7 @@ you're allowed to see the data it uses." ### Step 5 — Overseer: forward exclusion in `authorizeObservation()` -Extend `authorizeObservation()` (`overseer.ts:1169`) to honor `description.excludeObservers`. +Extend `authorizeObservation()` (in `overseer.ts`) to honor `description.excludeObservers`. Because v1 has no per-thread hiding, an excluded-but-named observation can only proceed when the named observer cannot reach it at all: either they have *already lost access* in the sharing graph, or the connection that produced it has left their role's verification scope. @@ -527,6 +534,12 @@ For each id in `description.excludeObservers`: 1. Map the opaque `observerId` → `profileId` via the `observers.byObserverId` index. If there is no record, the id is not an active observer → ignore it. + **Known gap: "no record" does not yet imply "not an active observer".** A first-time + `ensureObserver` registers a freshly minted id with the gatekeepers *before* the record (and + so `byObserverId`) is persisted, so an id named during that window — the fan-out, which can + park on the config modal — reads as unknown here and the observation is admitted to the very + collaborator it names. The required fix is an in-memory map of pending ids consulted here, + failing closed; it lands with `observer-verification-fixes`. 2. Check sharing-graph reachability for that `profileId` (`SharingManager.getEffectiveRole` / `computeEffectiveRoles`). - **Still authorized, and the producing gatekeeper is still in that role's scope → throw**, @@ -574,11 +587,14 @@ This is the runtime counterpart of `addObserver`: `addObserver` covers observers *after* data was read; `excludeObservers` covers data read *after* observers were configured. Persisting the observation record itself is unchanged; we only gate it. -> Why not also worry about authorized-but-not-yet-configured users here? They cannot be named in -> `excludeObservers` because no gatekeeper knows their id yet. The invariant still holds from the -> other direction: when such a user later opens and configures, `addObserver` re-checks them -> against *all* past observations (including any restricted one) and throws, denying them. So -> forward exclusion only needs to handle already-configured observers. +> Why not also worry about authorized-but-not-yet-configured users here? A user with no +> registration in flight cannot be named in `excludeObservers` because no gatekeeper knows their +> id yet. The invariant still holds from the other direction: when such a user later opens and +> configures, `addObserver` re-checks them against *all* past observations (including any +> restricted one) and throws, denying them. So forward exclusion only needs to handle +> already-configured observers — plus the one case in between: a user *mid*-registration is +> already known to every gatekeeper whose `addObserver` has returned, and can be named before +> their record exists. That is the Step 5 item 1 known gap above. ### Step 6 — Overseer: remove observers on sharing changes @@ -594,18 +610,17 @@ downgrades — see the matching methods on `OverseerClientInterface` and `Sharin stale cached workspace listing just yields a denied open). - After a mutation, use the returned `AffectedCollaborator[]` to find users who **lost access**. - For each who is now unreachable, if they have an observer record: best-effort - `removeObserver(record.observerId)` on **all** gatekeeper facets, then delete the observer - record. + For each who is now unreachable, if they have an observer record: delete the observer record, + then best-effort `removeObserver(record.observerId)` on **all** gatekeeper facets. - For a **`build` → `use` downgrade**, optionally `removeObserver` (and drop the corresponding `accountChoices` entries) for the now-out-of-scope bindings (those without a `bindingName`). Safe to defer — an over-broad observer set only ever errs toward stricter future checks — but it keeps gatekeeper state tidy. - All these calls are best-effort: log and continue on error. An orphaned observer entry only - causes superfluous future checks, never a data leak: a registration is what *admits* an open, and - every open re-runs `addObserver`, so a stale one grants nothing on its own — while + causes superfluous future checks, never a data leak: a registration is what *admits* an open, + and every open re-runs `addObserver`, so a stale one grants nothing on its own — while `authorizeObservation`'s exclusion gate re-checks the live sharing graph for any id a gatekeeper - still names. + still names. A record is only ever persisted for a party who passed full verification. > Multi-gatekeeper sequencing/atomicity is an overseer implementation detail, not part of the > shared interface. Because `addObserver` is re-run every open and `removeObserver` is idempotent, @@ -648,8 +663,29 @@ already in the JSDoc in `gatekeeper.ts`; add anything missing there rather than operational failure (vendor outage, expired credential) is treated the same way — the overseer cannot tell it from a settled denial — and the collaborator gets back in as soon as a repaired open re-verifies them. -4. **`containsRestrictedData` interaction** — unchanged and still authoritative: if set, no non-owner - can open at all (`overseer.ts:2770`). Observer checks only matter when sharing is allowed. +4. **`containsRestrictedData` interaction** — coverage is enforced at *admission*, not per + observation: `ensureObserver` re-verifies each collaborator against every in-scope gatekeeper + at every `open()`, so nobody can be in the workspace without having passed the producing + gatekeeper's `addObserver()`, and anything that widens what they must pass restarts every live + session (see "Restarting when verification scope widens"). The flag also latches the workspace + into a restricted mode that blocks actions and web fetches. + Verification is held to each collaborator's own role scope, because `ensureObserver` can + never verify beyond it: a `use` collaborator can't be covered for a gatekeeper outside their + scope (one no gadget binds and no enabled hook feeds — see `#useScopeGatekeeperIds`). + Unbinding shrinks `use` scope silently, so a formerly-bound producer's later restricted reads + are not covered for `use` collaborators added after the unbind. Accepted because `use` + sessions cannot read chat history or the action log, and a rebind restores coverage at the + next open. + The *never*-bound flavor of the same skip is broader: a producer reachable only through + chat bindings (an ambient singleton the agent reads in chat) was never in any `use` + collaborator's scope — the agent can persist its restricted data into gadget code or storage + without any `use` collaborator ever having been verified against it, and there is no prior + binding for "re-bind" to restore. + Accepted on the same grounds: coverage there is unverifiable by construction (the + liveness argument above), `use` sessions still cannot read chat history or the action + log, so the exposure is limited to what the agent chose to persist, and the forward + remedy is binding the producer to a gadget — that puts it in `use` scope, so every + collaborator is verified against it at their next open. 5. **Owner adds a new binding after sharing** — existing observers see an incremental modal for just the new binding on their next open, and may be denied if they lack access to the new resource (inherent to the security model). Because that next open is what verifies them, the @@ -672,6 +708,11 @@ already in the JSDoc in `gatekeeper.ts`; add anything missing there rather than (unbound, with no enabled hook keeping it reachable) is the different case Step 5's scope test handles: the gatekeeper still knows the id, but the observer can no longer reach what it produces, so they are de-registered from it instead of blocking. +8. **Removing a connection that read restricted data** — removal is not guarded by the + restricted-data latch. The record is what observer verification runs against, so removing a + producer drops the check for data that outlives it in chat history and storage. The intended + remedy is that a future connection-removal UI asks the owner to certify that no sensitive data + from that connection has been retained in the workspace, for any connection. --- @@ -733,15 +774,17 @@ gatekeeper package — a single package (e.g. `gatekeeper-google`) may use sever its resource types. - **A — Private-only.** Non-owner observers are refused: `addObserver()` unconditionally throws. - This is the replacement for today's reliance on `containsRestrictedData` for these resources (the - `containsRestrictedData` lockdown mechanism itself is unchanged and remains available separately). + For data that must additionally never leak back out, the `containsRestrictedData` restricted + mode (no actions, no web fetches) is available separately; combined with strategy A it makes + the workspace effectively private once sensitive data is observed. `getVerifier()` must still exist (the overseer mints one on every open) but is never consulted. - **B — ACL check (single unit).** The resource is treated as one atomic unit. `getVerifier()` mints a verifier exposing the observer's vendor identity (via the - "non-standard method on the verifier" pattern, `gatekeeper.ts:456-461`). `addObserver()` resolves - that identity and checks it against the bound resource's ACL, throwing on failure. Gatekeepers - should cache per-open to bound cost (`gatekeeper.ts:511-516`). No `excludeObservers` is needed: + "non-standard method on the verifier" pattern; see `GatekeeperUserVerifier` in `gatekeeper.ts`). + `addObserver()` resolves that identity and checks it against the bound resource's ACL, throwing + on failure. Gatekeepers should cache per-open to bound cost (see the note on `addObserver()`). + No `excludeObservers` is needed: the whole unit is covered up front, so nothing read later could be invisible to a verified observer. @@ -750,8 +793,9 @@ its resource types. plus the set of current observers. `addObserver()` verifies the observer against **every** logged set so far. When a later observation first touches a **new** set, the gatekeeper re-verifies all current observers and sets `excludeObservers` for any who fail (the overseer then blocks the - observation per `gatekeeper.ts:751-774`). `removeObserver()` drops the observer from the tracked - set. Each per-set check reuses the same ACL primitive the corresponding narrow (B) binding uses. + observation per the `excludeObservers` contract). `removeObserver()` drops the observer from + the tracked set. Each per-set check reuses the same ACL primitive the corresponding narrow (B) + binding uses. - **D — Low-stakes.** No information-flow tracking. `addObserver()` / `removeObserver()` are no-ops; any collaborator may observe. `getVerifier()` returns a trivial verifier (the overseer @@ -779,8 +823,8 @@ its resource types. | **linear** | Workspace | **C** | Track accessed teams; verify the observer against each (reusing the Team B check). | | **notion** | Page / Database | **B** | Check the observer's Notion access to the bound page/database. | | **notion** | Workspace | **C** | Track accessed pages/databases; verify the observer's access to each. | -| **supabase** | Project | **B** | Verify the observer's own `listProjects()` (`supabase-api.ts:306`) includes the bound project ref. Within a project, arbitrary read-only SQL spans the whole DB, so the project is the atomic unit (no per-table tracking). | -| **supabase** | Organization | **C** | Track accessed project refs (the org session reaches them via `openProject` / `listProjects`, `supabase.ts:1015`/`:1037`); verify the observer's `listProjects()` includes each, reusing the Project B check. | +| **supabase** | Project | **B** | Verify the observer's own `listProjects()` (`supabase-api.ts`) includes the bound project ref. Within a project, arbitrary read-only SQL spans the whole DB, so the project is the atomic unit (no per-table tracking). | +| **supabase** | Organization | **C** | Track accessed project refs (the org session reaches them via `openProject` / `listProjects` in `supabase.ts`); verify the observer's `listProjects()` includes each, reusing the Project B check. | | **confluence** | Site | **C** | Verify site access; track observed spaces and content because both can have narrower permissions. | | **confluence** | Space | **C** | Verify space access; track observed pages and blog posts because content restrictions may be narrower. | | **confluence** | Page / Blog Post | **C** | Verify bound-content access; track observed child pages because they may have stricter restrictions than their parent. | @@ -812,3 +856,20 @@ This is why the broad bindings split the way they do: - **Decomposition deliberately deferred → A:** Gmail Mailbox — could in principle decompose into mailing lists the observer belongs to, but that is the out-of-scope "advanced" case, so it stays fully private for now. + +--- + +## Known limitations + +Revocations and role changes take effect within seconds -- the revocation restart lands in +~100ms -- and read-side races inside that envelope are accepted by design: a guard earns its +place here only if its failure mode is *persistent* wrong state that outlives the window. + +The observer-side deferrals are ledgered in `plans/restricted-data-sharing.md`, each marked at +its site in the code by a matching `TODO` comment: + +- Observer verification is not serialized per profile, so concurrent opens by one collaborator + can overwrite each other's records. +- A mid-registration observer named in `excludeObservers` is read as unknown and the observation + admitted; persistent, since it lands in chat history (Step 5, item 1). Fix: a pending-id map, + `observer-verification-fixes`. diff --git a/docs/sharing.md b/docs/sharing.md index a90d77b5e..e86e39e78 100644 --- a/docs/sharing.md +++ b/docs/sharing.md @@ -33,7 +33,7 @@ Authorization is capability-based: `open()` computes the caller's effective role There are two ways to grant someone collaborator access: -**Direct add.** The owner or an existing collaborator enters a username (email address) in the Share modal. The system looks up the corresponding user account; if it exists, a collaborator record is created. The target user does not receive an in-product notification -- the sharer is expected to send them a link or tell them out of band. +**Direct add.** The owner or an existing collaborator enters a username (an email address on OAuth/CF Access deployments; a normalized alphanumeric handle on password deployments) in the Share modal. The system looks up the corresponding user account; if it exists, a collaborator record is created. The target user does not receive an in-product notification -- the sharer is expected to send them a link or tell them out of band. **Share link.** Any collaborator (or the owner) can create a share link, which encodes a secret key in the URL as a `#share=` fragment. Anyone who opens this link is automatically added as a collaborator. A link is a durable handle that owns one or more keys: creating it mints its first key, and "copying" the link later mints another key for the same link. The raw key is shown to the creator only once at mint time and is never stored server-side, so re-copying can't reproduce an old key -- it mints a new one. Any of a link's keys can be redeemed by multiple people, or the same person multiple times, until the link is revoked, which invalidates every key minted for it. @@ -43,6 +43,8 @@ Storage shape: a link is its first key. The `shareKeys` table holds one row per Share key redemption and gadget opening happen atomically in a single RPC call (`openGadget(id, shareKey)`), which allows subsequent calls to be pipelined on the returned `Overseer` stub without waiting for a separate redemption step. +Redemption is **one-step**: redeeming a key writes the recipient's `shareKey` edge immediately, making them a collaborator like any other before the redeeming open()'s observer verification runs. A recipient whose verification then fails keeps the edge -- see Known limitations. + ### Home page behavior A shared gadget does not appear on a collaborator's home page until they first open it. At that point, a record is created in the collaborator's user account (via `UserDurableObject.recordSharedGadgetOpen()`), storing a cached copy of the gadget's title and the owner's profile. The `lastActive` timestamp is updated each time they open the gadget. @@ -97,22 +99,20 @@ This does mean removed collaborators and revoked links accumulate in storage. Li ### Effective-role algorithm -The core is a **fixed-point role-propagation computation** implemented in `SharingManager.computeEffectiveRoles()`. It computes the effective role of every collaborator (given an optional hypothetical change), returning a map from profile ID to effective role (absence from the map means no access). It is the single source of truth: `open()`, `hasAnyShares()`, the listing RPCs, and the preview methods all derive from it. +The core is a **fixed-point role-propagation computation** implemented in `SharingManager.computeEffectiveRoles()`. It computes the effective role of every collaborator (given an optional hypothetical change), returning a map from profile ID to effective role (absence from the map means no access). It is the single source of truth: `open()`, the listing RPCs, and the preview methods all derive from it. -Inputs (all optional; used to model a hypothetical change in preview): +Inputs (all optional; used to model a hypothetical change): - `removedUser` -- a profile ID to treat as removed (excluded from the graph). - `removedEdge` -- a single user edge (`{target, sharer}`) to treat as removed. Used to preview a non-owner removing only their own edge. - `revokedLinkId` -- a share link ID to treat as revoked. -- `overrides` -- profile IDs pinned to at least a given role regardless of their edges. The algorithm: 1. **Build the candidate set.** Load all collaborators except the (hypothetically) removed user. 2. **Collect share-link metadata.** Build a map from link ID to `{creator, role}`, skipping links that are `revoked` (or the hypothetical `revokedLinkId`). -3. **Initialize** the role map with any `overrides`. -4. **Iterate to fixed point.** Repeatedly scan all collaborators. For each edge, compute the role it grants -- `min(edge role, sharer's effective role)`, where the sharer (or share link creator) is the owner (always `build`) or another collaborator's current effective role -- and raise the collaborator's role to the maximum across their valid edges. Raising one collaborator's role may unlock or raise others on the next pass. -5. **Converge.** Roles only ever increase, so the loop terminates when a full pass changes nothing. -6. **Return the role map.** Collaborators absent from the map have no access; collaborators present with a lower role than before have been downgraded. +3. **Iterate to fixed point.** Repeatedly scan all collaborators. For each edge, compute the role it grants -- `min(edge role, sharer's effective role)`, where the sharer (or share link creator) is the owner (always `build`) or another collaborator's current effective role -- and raise the collaborator's role to the maximum across their valid edges. Raising one collaborator's role may unlock or raise others on the next pass. +4. **Converge.** Roles only ever increase, so the loop terminates when a full pass changes nothing. +5. **Return the role map.** Collaborators absent from the map have no access; collaborators present with a lower role than before have been downgraded. This handles arbitrary graph shapes: diamonds (a user reachable via two independent paths), cycles (mutual adds), and deep chains. @@ -150,13 +150,17 @@ Authorization is enforced at `open()`: the method computes the caller's effectiv Because the role is recomputed from the graph on every `open()`, the live computation is the *sole* source of truth for access -- there is no eager cleanup whose bugs could grant access to an unreachable user. This is what makes lazy revocation safe: severing an edge is enough to deny access, even though the unreachable records linger in storage. +A share-key redemption goes through the same gate. The redeeming open() then verifies the recipient as an observer like any other collaborator; a recipient whose verification fails persists as an unverified collaborator until removed (see Known limitations). + ### Terminating live sessions on revocation or scope growth Authorization is only checked at `open()`, so a session that is *already* open is not re-checked per message. Without intervention, a collaborator who was just removed or downgraded could keep using their live session until something else disconnected them. To close this gap, `removeCollaborator`/`revokeShareLink` proactively restart the gadget's Overseer DO via `ctx.abort()` whenever the change actually removed or downgraded someone (i.e. the returned `AffectedCollaborator[]` is non-empty; pure no-op removals don't restart). Aborting forcibly disconnects every client; each reconnects and re-runs `open()`, which re-evaluates the now-changed permission graph -- sending removed users to the terminal access-denied page and handing downgraded users their reduced capability (the editor swaps to the `use` view automatically based on `metadata.role`). Since removals are rare (and DOs restart unpredictably anyway, so reconnects are already cheap), the disruption is acceptable. -Two precautions surround the abort (`OverseerImpl.scheduleAccessRestart`): the severed edge is flushed with `ctx.storage.sync()` first (because `ctx.abort()` does not respect the output gate, a restart could otherwise come back with the change lost), and the abort is delayed ~100ms so the triggering RPC's response reaches the caller -- typically the owner, who is also connected -- before their own connection drops. The disconnect reaches the browser through the existing `notifyClosed` plumbing: when the Overseer DO aborts, the per-session `notifyClosed` stub is disposed without being called, which `AuthenticatedApiImpl` treats as a lost connection and reacts to by killing the browser WebSocket, forcing a reconnect. +Two precautions surround the abort (`OverseerImpl.scheduleAccessRestart`): the severed edge is flushed with `ctx.storage.sync()` first (because `ctx.abort()` does not respect the output gate, a restart could otherwise come back with the change lost), and the abort is delayed ~100ms so the triggering RPC's response reaches the caller -- typically the owner, who is also connected -- before their own connection drops. The disconnect reaches the browser through the existing `notifyClosed` plumbing: when the Overseer DO aborts, the per-session `notifyClosed` stub is disposed without being called, which `AuthenticatedApiImpl` treats as a lost connection and reacts to by killing the browser WebSocket, forcing a reconnect. The client discards its retained share key on the first successful open, so this forced reconnect after a removal is keyless and lands the removed collaborator on the access-denied page rather than silently re-redeeming the still-active link (which would undo the removal and break the assumption stated above). The residual is unchanged: the *link* itself survives a collaborator removal under the lazy model, so a recipient who kept the URL can still re-redeem it manually until the owner revokes it -- the discard removes only the client's automatic re-grant. -Granting or raising access never strands anyone: a live session's capability is fixed at open, so a `use` collaborator promoted to `build` in the graph still holds `UseOverseerInterface` until they re-open, and nobody is newly excluded from anything. `containsRestrictedData` cannot strand a session either: an observation that would set that flag is *blocked* (rather than applied) if the gadget is already shared, so the flag only ever flips to true on a gadget with no other sessions to evict. +The abort also lands later than the ~100ms delay alone suggests: the revocation handlers first await the observer teardown (`tearDownLostObservers`, a per-collaborator `removeObserver` fan-out) and the listing refresh (`refreshAffectedCollaboratorListings`, chunked cross-DO round trips), so the removed users' sessions stay live and watching for a window that scales with collaborator and gatekeeper count. + +Granting or raising access never strands anyone: a live session's capability is fixed at open, so a `use` collaborator promoted to `build` in the graph still holds `UseOverseerInterface` until they re-open, and nobody is newly excluded from anything. The same abort serves a second purpose, though, and there the trigger is a *grant*: observer verification (see docs/observers.md) also runs only at `open()`, so widening the set of gatekeepers a collaborator must be verified against leaves their live session holding access they were never verified for. `OverseerImpl.#restartIfSessionsAffected` restarts the workspace whenever that happens -- a connection is added, one is bound into a gadget, or a merge promotes such a binding -- so every client re-opens and re-runs `ensureObserver` at the new scope. It is a no-op unless a collaborator session of the affected role is live -- severing sessions is all a restart does -- so a solo workspace, or one whose collaborators are all disconnected, is never disturbed. See docs/observers.md, "Restarting when verification scope widens", for the full trigger list and the reasoning about what deliberately does *not* trigger it. @@ -169,3 +173,20 @@ The same abort serves a second purpose, though, and there the trigger is a *gran - **Un-revoking share links.** Revocation is non-destructive (the `revoked` flag), but there is no UI or RPC to list revoked links or clear the flag, so link revocation is currently one-way in practice. - **Garbage-collecting dead records.** Removed collaborators and revoked links accumulate in storage under the lazy model; a background sweep could reclaim entries that have been unreachable for a long time. - **Notifications.** Currently there are no in-product notifications for access grants or revocations. + +## Known limitations + +Revocations and role changes take effect within seconds -- the revocation restart lands in +~100ms -- and read-side races inside that envelope are accepted by design; only guards against +*persistent* wrong state remain. Each item below is marked at its site in the code by a matching +`TODO` comment. + +- **An unverified redeemer persists as a collaborator.** Redemption writes a real edge before the + redeeming open's observer verification runs, so from the moment a recipient clicks the link they + appear in `listCollaborators` whether or not they ever complete the open. They cannot reach the + workspace -- verification denies them at open. Remedies: they verify (complete the open), the + owner removes them, or the link is revoked. Two-phase redemption (a pending edge granting + nothing until verification confirms it) is the planned fix. +- **A refused recipient persists.** A recipient whose verification is refused keeps their edge: + they appear in `listCollaborators` until the owner removes them (or revokes the link), with the + same consequences as the previous item, and covered by the same planned fix. diff --git a/packages/integration-tests/__tests__/observer-role-scope.test.ts b/packages/integration-tests/__tests__/observer-role-scope.test.ts index 2100cae66..0b040d753 100644 --- a/packages/integration-tests/__tests__/observer-role-scope.test.ts +++ b/packages/integration-tests/__tests__/observer-role-scope.test.ts @@ -5,10 +5,10 @@ // that widening restarts the workspace: each client's next open re-verifies against the new scope. // The external-message gate's role scoping is covered by external-message-verification.test.ts. // -// This lives in its own file -- with its own harness, like every suite here -- and stays small on -// purpose: a DO reset makes the shared local harness briefly drop unrelated in-flight requests, so -// the concurrent tests of any suite that restarts a workspace pass on their current timing, and -// growing the file re-rolls those dice. +// This lives in its own file -- with its own harness, like every suite here -- rather than in +// sensitive-observations.test.ts, because both suites restart the workspace, and a DO reset makes +// the shared local harness briefly drop unrelated in-flight requests; their concurrent tests pass +// with their current timing, but growing either file re-rolls those dice. import { afterAll, beforeAll, describe, expect, it } from "vitest"; import type { RpcStub } from "capnweb"; @@ -162,8 +162,9 @@ describe("role-scoped observer enforcement", () => { const carolSession = await holdSession(ws, carol); try { // Carol holds no coverage for the connection and never will while it stays unbound, but - // that is enforced against her open, not against the owner's own use of the connection. - await expect(ws.session.readValue()).resolves.toBe(42); + // that is enforced against her open, not against the owner's reads: this restricted read + // goes through. + await expect(ws.session.readValue(true)).resolves.toBe(42); // Binding the connection to a gadget (pure storage writes; no gadget code runs) brings it // into "use" scope. That widens what Carol's live session must be verified against, and a @@ -176,9 +177,10 @@ describe("role-scoped observer enforcement", () => { const reopened = await reopenAfterRestart(ws); try { - // The owner is back on the workspace with a working session: the restart is a re-open for - // everyone, not a lockout. - await expect(reopened.session.readValue()).resolves.toBe(42); + // The owner is back on the workspace with a working session, and their restricted read is + // undisturbed by the widening: the restart is a re-open for everyone, not a lockout, and + // nothing about Carol's coverage gates the owner's reads. + await expect(reopened.session.readValue(true)).resolves.toBe(42); // Carol's forced re-open is where the newly in-scope connection gets verified, and she is // asked about exactly it -- the one connection her role's scope just gained. diff --git a/packages/integration-tests/__tests__/sensitive-observations.test.ts b/packages/integration-tests/__tests__/sensitive-observations.test.ts new file mode 100644 index 000000000..6a4dda76b --- /dev/null +++ b/packages/integration-tests/__tests__/sensitive-observations.test.ts @@ -0,0 +1,518 @@ +// Tests for the sensitive-data (`containsRestrictedData`) observation policy. +// +// Coverage is enforced at admission, not at the read: every collaborator passes the producing +// gatekeeper's `addObserver` at their most recent open and cannot open without passing it, and +// anything that widens what they must pass restarts the workspace so every live session re-opens +// against the new scope. So sensitive observations are not blocked by an unverified collaborator, +// and sharing stays available. The observation also latches the workspace into a restricted mode: +// once latched, the workspace may not perform actions (nor fetch from the web, which has no +// client-reachable surface to assert here). +// +// The fixture gatekeeper's session drives all of this through the real ApprovalQueue funnel: +// `readValue(true)` records a `containsRestrictedData` observation, `writeValue()` submits an +// action (held for the owner's approval, so a test that wants it to go through approves it via +// the overseer). + +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import type { RpcStub } from "capnweb"; +import type { + AuthenticatedApi, Overseer, PublicApi, +} from "@gadgets/workshop-shared/api"; +import { + startTestGatekeeperHarness, TEST_GATEKEEPER_WORKER, TEST_VENDOR_ID, type Harness, +} from "../src/harness.js"; +import type { TestSession } from "../fixtures/gatekeeper-test/src/test-gatekeeper.js"; +import { + accountLabel, connect, listConnectedAccounts, logIn, MAX_OBSERVER_PROMPTS, nextUsernames, + ObserverConfigRecorder, signUp, stubFor, waitFor, type ConnectedAccount, +} from "../src/rpc-client.js"; +import { NetworkInterceptor } from "../src/network-interceptor.js"; + +let harness: Harness; +let interceptor: NetworkInterceptor; + +beforeAll(async () => { + interceptor = new NetworkInterceptor(); + interceptor.install(); + harness = await startTestGatekeeperHarness(); +}); + +afterAll(async () => { + const unmocked = interceptor.getUnmockedCalls(); + await harness?.server.close(); + interceptor.uninstall(); + interceptor.reset(); + expect(unmocked).toEqual([]); +}); + +async function withSession(body: (api: RpcStub) => Promise): Promise { + const publicApi = connect(harness.url); + try { + return await body(publicApi); + } finally { + publicApi[Symbol.dispose](); + } +} + +function thingUrl(name: string): string { + return `https://gadgets-test.example/things/${name}`; +} + +async function provisionAccount(api: RpcStub): Promise { + await api.provisionAmbientAccount(TEST_VENDOR_ID); + return waitFor("the test account to be provisioned", async () => { + const accounts = await listConnectedAccounts(api); + return accounts.find(a => a.vendorId === TEST_VENDOR_ID) ?? null; + }); +} + +/** + * Tell the fixture gatekeeper whether to admit `label` as an observer -- everywhere, or (with + * `resourceUrl`) at one bound resource only, which wins over the account-wide outcome. + */ +async function setVerifyOutcome( + label: string, outcome: { allow: true } | { allow: false; reason: string }, + resourceUrl?: string): Promise { + const res = await harness.fetchWorker( + TEST_GATEKEEPER_WORKER, "http://gatekeeper-test.test/control/verify-outcome", + { method: "POST", body: JSON.stringify({ label, resourceUrl, ...outcome }) }); + if (res.status !== 204) { + throw new Error(`Setting the verify outcome failed with ${res.status}: ${await res.text()}`); + } +} + +type Workspace = { + gadgetId: string; + overseer: RpcStub; + alice: string; + aliceApi: RpcStub; + /** The fixture session bound to the workspace's (first) gatekeeper. */ + session: RpcStub; + gatekeeperId: number; +}; + +// Alice creates a workspace bound to one Test Thing and opens a session on its gatekeeper. Every +// test starts here; collaborators and links are layered on per test. +async function newWorkspace(publicApi: RpcStub, thingName: string): Promise { + const [alice] = nextUsernames("alice"); + const aliceApi = await signUp(publicApi, alice); + const account = await provisionAccount(aliceApi); + + const overseer = await aliceApi.newGadget(); + const gatekeeper = await overseer.newGatekeeper(account.id, thingUrl(thingName)); + if (!gatekeeper) throw new Error("Failed to create the test connection"); + const gatekeeperId = await gatekeeper.getId(); + const session = await gatekeeper.openSession() as RpcStub; + const { id: gadgetId } = await overseer.getMetadata(); + return { gadgetId, overseer, alice, aliceApi, session, gatekeeperId }; +} + +type Bob = { + bob: string; + bobProfileId: string; + bobApi: RpcStub; + bobAccount: ConnectedAccount; + bobLabel: string; +}; + +// Sign Bob up, add him as a collaborator, and give him his own fixture account. +async function addBob(publicApi: RpcStub, ws: Workspace): Promise { + const [bob] = nextUsernames("bob"); + const bobApi = await signUp(publicApi, bob); + const bobAccount = await provisionAccount(bobApi); + const collaborator = await ws.overseer.addCollaborator(bob, "build"); + if (!collaborator) throw new Error(`Failed to share the gadget with ${bob}`); + return { + bob, bobProfileId: collaborator.profile.id, bobApi, bobAccount, + bobLabel: accountLabel(bobAccount), + }; +} + +// Bob opens the workspace, answering observer prompts with his own account. This is what writes +// his observer record, i.e. verifies him against every in-scope gatekeeper. Pass a `recorder` to +// assert *which* connections the open asked him about. +async function bobOpens(gadgetId: string, bobApi: RpcStub, + bobAccount: ConnectedAccount, + recorder?: ObserverConfigRecorder): Promise> { + const callback = stubFor( + recorder ?? new ObserverConfigRecorder().alwaysChoose(bobAccount.id, MAX_OBSERVER_PROMPTS)); + try { + return await bobApi.openGadget(gadgetId, undefined, callback); + } finally { + callback[Symbol.dispose](); + } +} + +// Wait out a restart and come back on a fresh connection, returning the owner's re-opened +// workspace and a session on `gatekeeperId`. +// +// A restart aborts the DO shortly after the triggering call returns, killing every stub from the +// connection that made it. A probe on a fresh connection can only detect a DO that is *already* +// dead -- never one about to die -- so a reopen attempted inside the pre-abort window can fully +// succeed against the doomed instance and then lose its session under the assertions that follow. +// Hence two steps: watch the pre-restart session die, then reopen with retries. +async function reopenAfterRestart(ws: Workspace, gatekeeperId = ws.gatekeeperId): Promise<{ + publicApi: RpcStub; + overseer: RpcStub; + session: RpcStub; +}> { + await waitFor("the restart to fell the old workspace instance", () => + ws.session.readValue().then(() => null, () => true)); + + return waitFor("the workspace to come back after the restart", async () => { + const publicApi = connect(harness.url); + try { + const aliceApi = await logIn(publicApi, ws.alice); + const overseer = await aliceApi.openGadget(ws.gadgetId); + const gatekeeper = await overseer.getGatekeeperById(gatekeeperId); + const session = await gatekeeper.openSession() as RpcStub; + // Probe with a benign read, so a session felled by the abort retries here rather than + // failing an assertion below. + await session.readValue(); + return { publicApi, overseer, session }; + } catch { + publicApi[Symbol.dispose](); + return null; + } + }); +} + +// Bob's forced re-open, on the fresh connection his browser would reconnect with. The restart +// killed the whole session his `bobApi` came from -- every client of the workspace loses its +// connection, not just its workspace stubs -- so reusing it here would fail on a dead socket +// rather than exercising the re-verification this asserts. +async function bobReopens( + ws: Workspace, bob: Bob, recorder: ObserverConfigRecorder): Promise { + const publicApi = connect(harness.url); + try { + const bobApi = await logIn(publicApi, bob.bob); + (await bobOpens(ws.gadgetId, bobApi, bob.bobAccount, recorder))[Symbol.dispose](); + } finally { + publicApi[Symbol.dispose](); + } +} + +// Bob's session, opened on its own connection (the one his browser holds) and kept live until +// close(). What a widening restarts is a live session: a collaborator who is only named in the +// sharing table, or who opened and left, has nothing to sever -- so a test that expects the +// restart must have Bob connected when the widening lands. +type HeldSession = { overseer: RpcStub, close: () => void }; + +async function bobHolds(ws: Workspace, bob: Bob): Promise { + const publicApi = connect(harness.url); + try { + const bobApi = await logIn(publicApi, bob.bob); + const overseer = await bobOpens(ws.gadgetId, bobApi, bob.bobAccount); + return { + overseer, + close: () => { + overseer[Symbol.dispose](); + publicApi[Symbol.dispose](); + }, + }; + } catch (error) { + publicApi[Symbol.dispose](); + throw error; + } +} + +describe("sensitive observations", () => { + it.concurrent("latch restricted mode: actions are blocked and metadata reports it", async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "latch"); + + // Before the latch, actions submit fine -- held for the owner's approval rather than + // refused, and applied once approved -- and metadata is clean. + const write = ws.session.writeValue(7); + const [held] = await waitFor("the write to be held for approval", async () => { + const { entries } = await ws.overseer.listActions({ filter: "pending" }); + return entries.length > 0 ? entries : null; + }); + await ws.overseer.approveAction(held.id); + await expect(write).resolves.toEqual(expect.any(Number)); + expect((await ws.overseer.getMetadata()).containsRestrictedData).toBeFalsy(); + + await expect(ws.session.readValue(true)).resolves.toBe(42); + + expect((await ws.overseer.getMetadata()).containsRestrictedData).toBe(true); + await expect(ws.session.writeValue(8)).rejects.toThrow(/prohibited from performing actions/i); + // Reads -- sensitive or not -- keep working. + await expect(ws.session.readValue()).resolves.toBe(42); + await expect(ws.session.readValue(true)).resolves.toBe(42); + }); + }); + + it.concurrent("an unredeemed share link does not block a sensitive observation", async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "unredeemed"); + await ws.overseer.createShareLink("build", "never redeemed"); + + // An outstanding link grants nobody anything until it is redeemed, and redemption happens + // inside open() -- where verification runs -- so the observation proceeds. + await expect(ws.session.readValue(true)).resolves.toBe(42); + }); + }); + + it.concurrent("sharing stays available after the latch", async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "share-after"); + await expect(ws.session.readValue(true)).resolves.toBe(42); + + // Sharing stays available after the latch, across every sharing RPC. + const [carol] = nextUsernames("carol"); + await signUp(publicApi, carol); + await expect(ws.overseer.addCollaborator(carol, "build")).resolves.toMatchObject({ + profile: expect.objectContaining({ id: expect.any(String) }), + }); + const { linkId } = await ws.overseer.createShareLink("use", "post-latch"); + await expect(ws.overseer.newShareLinkKey(linkId)).resolves.toMatchObject({ + key: expect.any(String), + }); + }); + }); + + it.concurrent("an unverified collaborator does not block a sensitive observation", async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "unverified"); + const bob = await addBob(publicApi, ws); + + // Bob has access but has never opened, so he holds no observer record for this gatekeeper + // -- and no session either, because verification is a precondition of getting one. There is + // nothing for the read to fail closed against. + await expect(ws.session.readValue(true)).resolves.toBe(42); + + // Admission is where the coverage requirement bites: the gatekeeper refuses him, so his + // open is denied and he never reaches the workspace, let alone the observation. + await setVerifyOutcome(bob.bobLabel, { allow: false, reason: "You do not have access." }); + await expect(bobOpens(ws.gadgetId, bob.bobApi, bob.bobAccount)) + .rejects.toThrow(/could not confirm/i); + + // His refusal costs the owner nothing: only his open is denied, so nothing was severed and + // reads keep flowing. + await expect(ws.session.readValue(true)).resolves.toBe(42); + }); + }); + + it.concurrent("a verified collaborator allows the sensitive observation through", async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "verified"); + const bob = await addBob(publicApi, ws); + (await bobOpens(ws.gadgetId, bob.bobApi, bob.bobAccount))[Symbol.dispose](); + + await expect(ws.session.readValue(true)).resolves.toBe(42); + }); + }); + + it.concurrent("adding a connection restarts the workspace so collaborators re-verify", + async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "covered"); + const bob = await addBob(publicApi, ws); + + // Bob verifies against the one connection and stays connected. + const bobSession = await bobHolds(ws, bob); + let lateId: number; + try { + // A second connection Bob has never been verified against. It is in his verification + // scope the moment it exists -- a "build" session can open a session on it with no + // observer check -- and his live session was admitted without it, so adding it severs + // every session. + const accounts = await listConnectedAccounts(ws.aliceApi); + const account = accounts.find(a => a.vendorId === TEST_VENDOR_ID)!; + const late = await ws.overseer.newGatekeeper(account.id, thingUrl("late")); + if (!late) throw new Error("Failed to create the second test connection"); + lateId = await late.getId(); + } finally { + bobSession.close(); + } + + const reopened = await reopenAfterRestart(ws, lateId); + try { + // Nothing is blocked: the owner reads restricted data through the new connection... + await expect(reopened.session.readValue(true)).resolves.toBe(42); + + // ...and Bob's forced re-open is where it gets verified. He is asked about exactly it, + // since his coverage for the connections that predate it survived. + const recorder = new ObserverConfigRecorder() + .alwaysChoose(bob.bobAccount.id, MAX_OBSERVER_PROMPTS); + await bobReopens(ws, bob, recorder); + expect(recorder.callCount).toBe(1); + expect(recorder.calls[0].map(need => need.gatekeeperId)).toEqual([lateId]); + } finally { + reopened.publicApi[Symbol.dispose](); + } + }); + }); + + it.concurrent("a collaborator can open a workspace that latched before they were added", + async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "open-after"); + await expect(ws.session.readValue(true)).resolves.toBe(42); + + // Bob's open runs observer verification, which the fixture admits by default, so the latch + // does not shut him out. + const bob = await addBob(publicApi, ws); + using bobOverseer = await bobOpens(ws.gadgetId, bob.bobApi, bob.bobAccount); + await expect(bobOverseer.getMetadata()).resolves.toMatchObject({ + id: ws.gadgetId, + containsRestrictedData: true, + }); + }); + }); + + it.concurrent("a collaborator the gatekeeper refuses is denied at open, with its reason", + async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "refused"); + await expect(ws.session.readValue(true)).resolves.toBe(42); + + const bob = await addBob(publicApi, ws); + const reason = "You do not have access to this thing."; + await setVerifyOutcome(bob.bobLabel, { allow: false, reason }); + + // This is the strategy-A shape: enforcement lives in the gatekeeper's addObserver(), so + // the user sees the gatekeeper's own message. + const error = await bobOpens(ws.gadgetId, bob.bobApi, bob.bobAccount).then( + overseer => { overseer[Symbol.dispose](); return null; }, + (err: unknown) => err as Error); + expect(error).not.toBeNull(); + expect(error!.message).toMatch(/could not confirm/i); + expect(error!.message).toContain(reason); + }); + }); + + it.concurrent("a failed re-verification denies that open and nothing else", async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "revoked"); + // A second producer, so the test can prove the denial is scoped to the one that refused. + // Added before Bob, so it widens nobody's scope and restarts nothing. + const accounts = await listConnectedAccounts(ws.aliceApi); + const account = accounts.find(a => a.vendorId === TEST_VENDOR_ID)!; + const second = await ws.overseer.newGatekeeper(account.id, thingUrl("revoked-2")); + if (!second) throw new Error("Failed to create the second test connection"); + const secondSession = await second.openSession() as RpcStub; + + // Bob verifies against both producers. + const bob = await addBob(publicApi, ws); + (await bobOpens(ws.gadgetId, bob.bobApi, bob.bobAccount))[Symbol.dispose](); + await expect(ws.session.readValue(true)).resolves.toBe(42); + await expect(secondSession.readValue(true)).resolves.toBe(42); + + // Bob's access to the first producer's resource is revoked; his next open is denied... + await setVerifyOutcome( + bob.bobLabel, { allow: false, reason: "Access revoked." }, thingUrl("revoked")); + await expect(bobOpens(ws.gadgetId, bob.bobApi, bob.bobAccount)) + .rejects.toThrow(/could not confirm/i); + + // ...and only that open. Nothing is severed and the owner's reads keep flowing through both + // producers: Bob cannot be admitted again without re-verifying, which is the whole of the + // enforcement (the lazy-revocation residual documented in docs/observers.md). + await expect(ws.session.readValue(true)).resolves.toBe(42); + await expect(secondSession.readValue(true)).resolves.toBe(42); + + // A returning observer's coverage survives the failure, so once repaired his re-open + // re-verifies both producers from his persisted choices without prompting (the recorder has + // no queued responses, so an unexpected prompt throws). + await setVerifyOutcome(bob.bobLabel, { allow: true }, thingUrl("revoked")); + const recorder = new ObserverConfigRecorder(); + (await bobOpens(ws.gadgetId, bob.bobApi, bob.bobAccount, recorder))[Symbol.dispose](); + expect(recorder.callCount).toBe(0); + }); + }); + + it.concurrent("a refused share-link recipient persists as a collaborator without blocking reads", + async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "refused-link"); + await expect(ws.session.readValue(true)).resolves.toBe(42); + + const { key } = await ws.overseer.createShareLink("build", "refused recipient"); + + const [dave] = nextUsernames("dave"); + const daveApi = await signUp(publicApi, dave); + const daveAccount = await provisionAccount(daveApi); + await setVerifyOutcome( + accountLabel(daveAccount), { allow: false, reason: "You do not have access." }); + + // Dave's open redeems the key -- writing a real edge -- and observer verification then + // refuses him. One-step redemption accepts the residue: he persists as an unverified + // collaborator (see the TODO on redeemShareKey). + const recorder = + new ObserverConfigRecorder().alwaysChoose(daveAccount.id, MAX_OBSERVER_PROMPTS); + const callback = stubFor(recorder); + try { + await expect(daveApi.openGadget(ws.gadgetId, key, callback)) + .rejects.toThrow(/could not confirm/i); + } finally { + callback[Symbol.dispose](); + } + + // The residue is a collaborator row, not access: he never opened, and he cannot open + // without passing the same check. So the owner's reads are untouched -- and nothing was + // severed either, since only his own open was denied. + const collaborators = await ws.overseer.listCollaborators(); + expect(collaborators).toHaveLength(1); + await expect(ws.session.readValue(true)).resolves.toBe(42); + }); + }); + + it.concurrent("concurrent redemptions of the same key both verify", async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "raced"); + const { key } = await ws.overseer.createShareLink("build", "raced"); + + const [dave] = nextUsernames("dave"); + const daveApi = await signUp(publicApi, dave); + const daveAccount = await provisionAccount(daveApi); + + const callbacks = [0, 1].map(() => stubFor( + new ObserverConfigRecorder().alwaysChoose(daveAccount.id, MAX_OBSERVER_PROMPTS))); + try { + // Each open redeems the same key; the edges deduplicate, so neither open is turned away + // and the grants collapse to one edge. + const overseers = await Promise.all( + callbacks.map(cb => daveApi.openGadget(ws.gadgetId, key, cb))); + for (const overseer of overseers) overseer[Symbol.dispose](); + } finally { + for (const cb of callbacks) cb[Symbol.dispose](); + } + + const collaborators = await ws.overseer.listCollaborators(); + expect(collaborators).toHaveLength(1); + expect(collaborators[0].addedBy).toHaveLength(1); + }); + }); + + it.concurrent("removal restarts the workspace and tears down the observer record", async () => { + await withSession(async publicApi => { + const ws = await newWorkspace(publicApi, "removal"); + const bob = await addBob(publicApi, ws); + (await bobOpens(ws.gadgetId, bob.bobApi, bob.bobAccount))[Symbol.dispose](); + await expect(ws.session.readValue(true)).resolves.toBe(42); + + // Removing Bob triggers the revocation restart: the DO aborts shortly after this call + // returns, killing every stub from this connection -- including the session Bob holds, + // which is the point. Everything past here runs on a fresh connection. + await ws.overseer.removeCollaborator(bob.bobProfileId, []); + const reopened = await reopenAfterRestart(ws); + + try { + // Bob's collaborator record lingers in storage (lazy revocation), and the owner's reads + // are unaffected either way. + await expect(reopened.session.readValue(true)).resolves.toBe(42); + + // Removal also tore down his observer record, so re-adding him must not silently restore + // his coverage: his next open has to name an account for the producer and pass + // addObserver again. + await reopened.overseer.addCollaborator(bob.bob, "build"); + const recorder = new ObserverConfigRecorder() + .alwaysChoose(bob.bobAccount.id, MAX_OBSERVER_PROMPTS); + await bobReopens(ws, bob, recorder); + expect(recorder.calls[0].map(need => need.gatekeeperId)).toContain(ws.gatekeeperId); + } finally { + reopened.publicApi[Symbol.dispose](); + } + }); + }); +}); diff --git a/packages/integration-tests/fixtures/gatekeeper-test/src/test-gatekeeper.ts b/packages/integration-tests/fixtures/gatekeeper-test/src/test-gatekeeper.ts index 97273789a..6709a2b16 100644 --- a/packages/integration-tests/fixtures/gatekeeper-test/src/test-gatekeeper.ts +++ b/packages/integration-tests/fixtures/gatekeeper-test/src/test-gatekeeper.ts @@ -302,7 +302,8 @@ export class TestVerifier // Gatekeeper (one per bound resource, running as a facet under the gadget's Overseer) export interface TestSession { - readValue(): Promise; + /** `restricted` marks the observation `containsRestrictedData`. */ + readValue(restricted?: boolean): Promise; writeValue(value: number): Promise; writeValues(values: number[]): Promise; } @@ -319,10 +320,11 @@ class TestSessionTarget extends RpcTarget implements TestSession { this.approvalQueue = approvalQueue.dup(); } - async readValue(): Promise { + async readValue(restricted?: boolean): Promise { await this.approvalQueue.authorizeObservation({ title: "Read the test value", description: "Read the deterministic value exposed by the integration-test gatekeeper.", + ...(restricted ? { containsRestrictedData: true } : {}), }); return 42; } diff --git a/packages/workshop-backend/__tests__/observer-scope-restart.test.ts b/packages/workshop-backend/__tests__/observer-scope-restart.test.ts index e5769b212..2d42bc948 100644 --- a/packages/workshop-backend/__tests__/observer-scope-restart.test.ts +++ b/packages/workshop-backend/__tests__/observer-scope-restart.test.ts @@ -495,8 +495,8 @@ describe("restarting sessions when verification scope widens", () => { expect(restarts).toEqual([]); })); - // A pre-creationSpec record, which observerVendorId() refuses to classify: sharing anything - // that reaches it requires the owner to reconnect it first. + // A pre-creationSpec record, which observerVendorId() refuses to classify: nobody can be + // verified against it, so sharing anything that reaches it requires the owner to remove it first. function seedLegacyGatekeeper(impl: any, id: number): void { impl.storage.gatekeepers.put({ id, resourceTitle: `Legacy ${id}`, class: {} as any }); } @@ -518,8 +518,8 @@ describe("restarting sessions when verification scope widens", () => { seedLegacyGatekeeper(impl, 1); // "Build" scope is everything, so the record is in scope and the intended fail-closed error - // still fires: the owner must reconnect it before the workspace can be shared at that level. - expect(() => impl.listObserverRequirements("build")).toThrow(/reconnected/); + // still fires: the owner must remove it before the workspace can be shared at that level. + expect(() => impl.listObserverRequirements("build")).toThrow(/cannot verify collaborators/); })); it("...and still blocks a use collaborator once a gadget binds it", @@ -530,7 +530,7 @@ describe("restarting sessions when verification scope widens", () => { // Bound, the record is genuinely in "use" scope, and verification can't proceed without // knowing what to verify against -- same fail-closed refusal as "build". impl.bindWorkpiece(100, "DB", 1); - expect(() => impl.listObserverRequirements("use")).toThrow(/reconnected/); + expect(() => impl.listObserverRequirements("use")).toThrow(/cannot verify collaborators/); })); it("binding a legacy connection restarts a connected use collaborator and quarantines it", @@ -542,7 +542,8 @@ describe("restarting sessions when verification scope widens", () => { // A legacy record has no vendor, so nobody CAN be verified against it -- but that must make // it count on the widening diff, not vanish from both sides of it: binding it is still the // moment live use sessions gain a route to it. The restart severs them and the quarantine - // holds until the reset; fresh use opens then fail closed (/reconnected/, above). + // holds until the reset; fresh use opens then fail closed (/cannot verify collaborators/, + // above). impl.bindWorkpiece(100, "DB", 1); expect(restarts).toHaveLength(1); diff --git a/packages/workshop-backend/__tests__/observer-verification-failure.test.ts b/packages/workshop-backend/__tests__/observer-verification-failure.test.ts index 8ef27cba5..b7982fee1 100644 --- a/packages/workshop-backend/__tests__/observer-verification-failure.test.ts +++ b/packages/workshop-backend/__tests__/observer-verification-failure.test.ts @@ -80,6 +80,14 @@ describe("a binding that fails verification", () => { // nothing for the rollback to remove. Her registrations are what make gatekeepers name her // in `excludeObservers`, so dropping one would be fail-open. expect(removed).toEqual([]); + + // Neither producer's restricted reads are blocked, though -- both are verifiable, so + // admission is the whole enforcement and nobody unverified can be watching. + let restricted = { title: "t", description: "d", containsRestrictedData: true }; + await expect(impl.authorizeObservation(1, restricted, { from: "user" })) + .resolves.toBeUndefined(); + await expect(impl.authorizeObservation(2, restricted, { from: "user" })) + .resolves.toBeUndefined(); }); }); diff --git a/packages/workshop-backend/__tests__/restricted-observation-latch.test.ts b/packages/workshop-backend/__tests__/restricted-observation-latch.test.ts new file mode 100644 index 000000000..ea84de7a6 --- /dev/null +++ b/packages/workshop-backend/__tests__/restricted-observation-latch.test.ts @@ -0,0 +1,118 @@ +// authorizeObservation's restricted-data latch is one-way and is set only once the observation is +// actually delivered. The exclusion gate is decided first, across an awaited cross-worker fan-out, +// so an observation the exclusion blocks must leave no trace -- no latch, no record -- and one it +// admits latches and records in the same synchronous block after the teardown completes. +// +// Runs against a real OverseerDurableObject (the TEST_OVERSEER binding); the gatekeeper facet is +// the only fake. + +import { describe, expect, it } from "vitest"; +import { env } from "cloudflare:workers"; +import { runInDurableObject } from "cloudflare:test"; +import type { OverseerDurableObject } from "../src/overseer.js"; + +declare module "cloudflare:workers" { + interface ProvidedEnv { + TEST_OVERSEER: DurableObjectNamespace; + } +} + +const OWNER = "alice"; + +function deferred(): { promise: Promise; resolve: () => void } { + let resolve!: () => void; + let promise = new Promise(r => { resolve = r; }); + return { promise, resolve }; +} + +const tick = () => new Promise(resolve => setTimeout(resolve, 0)); + +function getImpl(instance: OverseerDurableObject): any { + let impl = (instance as unknown as { impl: any }).impl; + // The sharing manager resolves collaborator reachability from the owner; seed the cached + // profile id so no User DO round trip is attempted. + impl.ownerProfileId = OWNER; + return impl; +} + +function seedGatekeeper(impl: any, id: number): void { + impl.storage.gatekeepers.put({ + id, + resourceTitle: `Connection ${id}`, + class: {} as any, + }); +} + +const RESTRICTED_EXCLUDING_MALLORY = { + title: "Read a thing", + description: "The test read a thing.", + containsRestrictedData: true, + excludeObservers: ["obs-m"], +}; + +describe("authorizeObservation's restricted-data latch", () => { + it("latches and records only after the exclusion teardown admits the observation", async () => { + let stub = env.TEST_OVERSEER.getByName("restricted-latch-teardown-window"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = getImpl(instance); + seedGatekeeper(impl, 1); + // Mallory holds an observer record but no reachable role: the named exclusion admits the + // observation and schedules her teardown. + impl.storage.observers.put( + { profileId: "mallory", observerId: "obs-m", accountChoices: { 1: 10 } }); + + // The cross-worker teardown parks, holding the observation mid-flight before any decision + // the delivery rests on has been made. + let held = deferred(); + impl.getGatekeeperFacet = () => ({ + removeObserver: async () => { await held.promise; }, + }); + + let observation = impl.authorizeObservation( + 1, RESTRICTED_EXCLUDING_MALLORY, { from: "user" }); + await tick(); + + // Nothing is delivered while the teardown is in flight, so nothing has latched: a teardown + // that ends in refusal must leave no trace. + expect(impl.storage.containsRestrictedData.get()).toBe(false); + + held.resolve(); + await expect(observation).resolves.toBeUndefined(); + + // Delivery: the latch and the record landed together. + expect(impl.storage.containsRestrictedData.get()).toBe(true); + + // The teardown still ran (mallory is no longer set up to observe). + expect(impl.storage.observers.get("mallory")).toBeUndefined(); + let records = [...impl.storage.actions.list()]; + expect(records).toHaveLength(1); + expect(records[0].type).toBe("observation"); + }); + }); + + it("leaves no trace when the exclusion gate blocks the observation", async () => { + let stub = env.TEST_OVERSEER.getByName("restricted-latch-exclusion-blocked"); + await runInDurableObject(stub, async (instance: OverseerDurableObject) => { + let impl = getImpl(instance); + seedGatekeeper(impl, 1); + // Mallory is a current collaborator: the exclusion gate is the only thing blocking this + // observation. + impl.storage.collaborators.put({ + profile: { id: "mallory", name: "Mallory" }, + addedBy: [{ type: "user", sharer: OWNER, created: new Date(), role: "build" }], + }); + impl.storage.observers.put( + { profileId: "mallory", observerId: "obs-m", accountChoices: { 1: 10 } }); + + await expect(impl.authorizeObservation( + 1, RESTRICTED_EXCLUDING_MALLORY, { from: "user" })) + .rejects.toThrow(/not permitted to see/); + + // The blocked observation delivered no data, so the workspace is not restricted: no latch, + // no action record -- and mallory, still authorized, was not torn down. + expect(impl.storage.containsRestrictedData.get()).toBe(false); + expect([...impl.storage.actions.list()]).toHaveLength(0); + expect(impl.storage.observers.get("mallory")).toBeDefined(); + }); + }); +}); diff --git a/packages/workshop-backend/__tests__/sharing.test.ts b/packages/workshop-backend/__tests__/sharing.test.ts index 720908f02..cb1bf7872 100644 --- a/packages/workshop-backend/__tests__/sharing.test.ts +++ b/packages/workshop-backend/__tests__/sharing.test.ts @@ -96,27 +96,6 @@ describe("authorization", () => { expect(mgr.getEffectiveRole("a")).toBe("use"); expect(mgr.getEffectiveRole("b")).toBe("build"); }); - - it("hasAnyShares reflects current reachability, not table membership", () => { - let { storage, mgr } = makeManager(); - expect(mgr.hasAnyShares()).toBe(false); - - // An active share link counts as a share. - seedLink(storage, "k1", OWNER); - expect(mgr.hasAnyShares()).toBe(true); - - // A revoked link does not. - storage.shareKeys.put({ id: "k1", created: new Date(), createdBy: OWNER, revoked: true }); - expect(mgr.hasAnyShares()).toBe(false); - - // A reachable collaborator counts. - seedCollaborator(storage, "a", [userEdge(OWNER)]); - expect(mgr.hasAnyShares()).toBe(true); - - // A collaborator whose record lingers but is unreachable does not. - storage.collaborators.put({ profile: profile("a"), addedBy: [] }); - expect(mgr.hasAnyShares()).toBe(false); - }); }); describe("redeemShareKey", () => { diff --git a/packages/workshop-backend/src/overseer.ts b/packages/workshop-backend/src/overseer.ts index f52790ebb..67c4a4a8c 100644 --- a/packages/workshop-backend/src/overseer.ts +++ b/packages/workshop-backend/src/overseer.ts @@ -494,7 +494,9 @@ function fallbackBindingName(base: string, isTaken: (name: string) => boolean): function observerVendorId(record: GatekeeperRecord): string | null { if (!record.creationSpec) { throw new Error( - "This workspace has a legacy connection that must be reconnected by its owner before it can be shared."); + "This workspace has a legacy connection that cannot verify collaborators' access. Its " + + "owner must remove the connection before the workspace can be shared, or start a new " + + "workspace."); } return "vendorId" in record.creationSpec ? record.creationSpec.vendorId : null; } @@ -5509,17 +5511,6 @@ class OverseerImpl implements AgentHooks { async authorizeObservation(gatekeeperId: number, description: ObservationDescription, caller: GatekeeperCaller): Promise { - if (description.containsRestrictedData) { - if ((await this.getSharingManager()).hasAnyShares()) { - throw new Error( - "This observation was blocked because it contains sensitive data that must only be " + - "shown to the account owner, but this workspace is shared with other users. Try again " + - "from a workspace that is not shared."); - } - - this.storage.containsRestrictedData.put(true); - } - // Forward exclusion: the gatekeeper may name observers who must not see this observation. Since // v1 has no per-thread hiding, the only way to let such an observation proceed is if no named // observer could reach it -- either they have lost access in the sharing graph, or this @@ -5529,6 +5520,10 @@ class OverseerImpl implements AgentHooks { await this.#enforceExcludeObservers(gatekeeperId, description.excludeObservers); } + if (description.containsRestrictedData) { + this.storage.containsRestrictedData.put(true); + } + let actionId = this.storage.nextActionId.get(); this.storage.nextActionId.put(actionId + 1); @@ -5683,6 +5678,10 @@ class OverseerImpl implements AgentHooks { let outOfScope: string[] = []; for (let observerId of observerIds) { let observer = this.storage.observers.byObserverId.get(observerId); + // TODO(observer-races): a first-time ensureObserver registers its observerId with the + // gatekeepers before the record is persisted, so an id named here in that window reads as + // unknown and the observation is admitted. Fix: an in-memory pending-id map consulted + // here, failing closed. if (!observer) continue; // not an active observer -> ignore let role = sharing.getEffectiveRole(observer.profileId); if (!role) { @@ -9167,6 +9166,11 @@ class OverseerImpl implements AgentHooks { // creationSpec: an unrelated legacy connection outside the caller's scope must not block their // open, since nothing they can reach needs verification against it. An in-scope one still // throws, fail-closed (and "build" scope is everything, so it always throws there). + // + // TODO(known-risk): a "use" collaborator is never verified against a producer outside their + // scope, yet restricted data read from it can reach gadget state they see, because provenance + // is not tracked past the observation. Accepted for v1; see "Known security risk -- never-bound + // producers" in plans/restricted-data-sharing.md. #inScopeGatekeepers(role: CollaboratorRole): GatekeeperRecord[] { let boundIds = role === "use" ? this.#useScopeGatekeeperIds() : undefined; @@ -9319,6 +9323,10 @@ class OverseerImpl implements AgentHooks { // resource access promptly. Returns when fully verified; throws to deny access. // // See observers-implementation-plan.md §5 Step 3. + // + // TODO: Concurrent opens by the same profile race this method -- two calls mint two observerIds + // and the last-written record forgets the other's gatekeeper registrations -- so verification + // needs to be serialized per profile. async ensureObserver( profileId: string, clientUser: DurableObjectStub, @@ -9803,13 +9811,6 @@ export class OverseerDurableObject extends DurableObject { let role: CollaboratorRole = "build"; if (!isOwner) { - if (this.impl.storage.containsRestrictedData.get()) { - // `containsRestrictedData` can only have been set when the gadget had no shares (see - // `authorizeObservation`), and no new shares can be created while it's set, so any - // non-owner reaching here is necessarily unauthorized. - throw createOpenGadgetError(OPEN_GADGET_ERROR_CODES.workspaceAccessDenied); - } - let sharing = await this.impl.getSharingManager(); // If a share key was provided, redeem it. The owner already has full access and should not @@ -9830,7 +9831,7 @@ export class OverseerDurableObject extends DurableObject { // verify they may observe everything this Gadget has read through its in-scope gatekeepers, // configuring their connected accounts if needed. Observer verification runs only after a // valid role is confirmed, so it never reveals gatekeeper or resource metadata to an - // unauthorized user; the containsRestrictedData short-circuit above still wins over both. + // unauthorized user. // // An unauthorized caller (no effective role -- never had access, or was removed) gets a // distinct denial without workspace metadata. A removed collaborator who reconnects after @@ -9948,12 +9949,6 @@ export class OverseerDurableObject extends DurableObject { // denial below rather than being verified (or told to fix a verification failure) for access // this path can never grant them. if (ownerId !== callerId) { - if (this.impl.storage.containsRestrictedData.get()) { - return { - accepted: false, - message: "This workspace has sharing disabled, so only its owner can access it.", - }; - } let role: CollaboratorRole | null; try { role = await this.impl.authorizeCollaborator( @@ -11976,8 +11971,7 @@ class OverseerClientInterface extends RpcTarget implements Overseer { // --- Collaborator management --- // // The sharing/permission logic lives in SharingManager (./sharing). These methods handle only - // the RPC-bound pieces (resolving profiles via User DOs, the `containsRestrictedData` policy) and - // delegate the rest. + // the RPC-bound pieces (resolving profiles via User DOs) and delegate the rest. async listObserverRequirements( role: CollaboratorRole): Promise { @@ -11998,12 +11992,6 @@ class OverseerClientInterface extends RpcTarget implements Overseer { return null; } - if (this.impl.storage.containsRestrictedData.get()) { - throw new Error( - "This workspace has observed sensitive data. To prevent leaks, the workspace cannot be " + - "shared."); - } - return (await this.impl.getSharingManager()).addCollaborator({ caller: this.#sharingCaller(), profile, @@ -12061,23 +12049,11 @@ class OverseerClientInterface extends RpcTarget implements Overseer { async createShareLink(role: CollaboratorRole, note?: string) : Promise<{ key: string; linkId: string }> { - if (this.impl.storage.containsRestrictedData.get()) { - throw new Error( - "This workspace has observed sensitive data. To prevent leaks, the workspace cannot be " + - "shared."); - } - return (await this.impl.getSharingManager()) .createShareLink({ caller: this.#sharingCaller(), role, note }); } async newShareLinkKey(linkId: string): Promise<{ key: string }> { - if (this.impl.storage.containsRestrictedData.get()) { - throw new Error( - "This workspace has observed sensitive data. To prevent leaks, the workspace cannot be " + - "shared."); - } - return (await this.impl.getSharingManager()) .newShareLinkKey({ caller: this.#sharingCaller(), linkId }); } diff --git a/packages/workshop-backend/src/sharing.ts b/packages/workshop-backend/src/sharing.ts index e3fbe48d8..e3acb20d8 100644 --- a/packages/workshop-backend/src/sharing.ts +++ b/packages/workshop-backend/src/sharing.ts @@ -15,11 +15,8 @@ // re-adding a removed collaborator restores them and, transitively, everyone they had shared with. // (Records and revoked keys accumulate in storage; a future GC could reclaim long-dead entries.) // -// NOTE: The `containsRestrictedData` policy flag intentionally does NOT live here. It is a broader -// "is this gadget allowed to communicate with anyone other than the owner?" policy (it also -// gates gatekeeper writes and web fetches) and is expected to grow into a separate policy engine. -// The Overseer enforces that flag; this module only exposes `hasAnyShares()` so the policy can -// ask about the current sharing state. +// NOTE: The sensitive-data (`containsRestrictedData`) policy intentionally does NOT live here; the +// Overseer enforces it. This module only answers questions about the sharing graph. import { AiChatAuthorInfo, CollaboratorInfo, PermissionEdge, CollaboratorRole, AffectedCollaborator } from "@gadgets/workshop-shared/api"; @@ -160,26 +157,6 @@ export class SharingManager { */ constructor(private storage: SharingStorage, private ownerProfileId: string) {} - // --------------------------------------------------------------------------------------- - // Sharing-state queries - - /** - * True if anyone other than the owner can currently access the gadget. Used by the Overseer's - * `containsRestrictedData` policy to decide whether a sensitive observation must be blocked. - * - * Because removed collaborators and revoked links linger in storage (the lazy revocation model; - * see the module header and removeCollaborator/revokeShareLink), this must reflect *current* - * reachability, not mere table membership: a collaborator with a live path from the owner, or - * an un-revoked share link whose keys anyone could still redeem. - */ - hasAnyShares(): boolean { - if (this.computeEffectiveRoles().size > 0) return true; - for (let link of this.#listLinks()) { - if (!link.revoked) return true; - } - return false; - } - // Every share link, revoked or not. Aliases are skipped. *#listLinks(): Generator { for (let record of this.storage.shareKeys.list()) { @@ -219,6 +196,10 @@ export class SharingManager { * collaborators are redeemed without any RPC. * * A key whose link is revoked behaves like an unknown key (it cannot be redeemed). + * + * TODO: The edge is written before the redeeming open()'s observer verification runs, so a + * recipient whose verification fails lingers in listCollaborators until removed or the link is + * revoked. */ async redeemShareKey(opts: { rawKey: string; @@ -292,8 +273,8 @@ export class SharingManager { /** * Add a collaborator with a `user` edge from the caller, granting `role`. The caller is - * responsible for resolving `profile` (via RPC) and for any policy checks (e.g. - * `containsRestrictedData`). The caller may not grant a role higher than their own effective role. + * responsible for resolving `profile` (via RPC) and for any policy checks. The caller may not + * grant a role higher than their own effective role. */ addCollaborator(opts: { caller: SharingCaller; diff --git a/packages/workshop-shared/src/api.ts b/packages/workshop-shared/src/api.ts index 0422c0eaf..24d4b1410 100644 --- a/packages/workshop-shared/src/api.ts +++ b/packages/workshop-shared/src/api.ts @@ -1307,8 +1307,9 @@ export type GadgetMetadata = { role?: CollaboratorRole; /** - * True when the gadget has observed data marked as share-prohibited. Such gadgets can no longer - * be shared with additional users or links. + * True when the gadget has observed data marked `containsRestrictedData` (see + * `ObservationDescription`). It can still be shared, with collaborators verified per + * gatekeeper, but can no longer perform actions or fetch from the public web. */ containsRestrictedData?: boolean; diff --git a/packages/workshop-shared/src/gatekeeper.ts b/packages/workshop-shared/src/gatekeeper.ts index f2dc3eebb..7dbde6eca 100644 --- a/packages/workshop-shared/src/gatekeeper.ts +++ b/packages/workshop-shared/src/gatekeeper.ts @@ -1211,19 +1211,17 @@ export type ObservationDescription = { // can help detect situations where the gadget could leak information. /** - * If true, then this observation contains sensitive information that MUST NOT be shared with - * ANYONE except the account owner. This means: - * - If the gadget is shared already, authorizeObservation() must throw an exception to block - * the observation. - * - All future sharing of the gadget is prohibited. - * - Once observed, the gadget goes into "lockdown mode" where it can no longer perform any - * actions, only make observations. This prevents the gadget from leaking data through other - * gatekeepers. - * - * TODO(someday): This was added as a stopgap in order to be able to make certain sensitive data - * sources available to internal users. In the longer-term, it should be possible to share - * sensitive data as long as the recipients also have access to that same data, but this - * requires a more complex policy framework to compute. + * If true, then this observation contains sensitive information that must only be shown to + * people who are verified to have access to the same data. This means: + * - Every collaborator must pass this gatekeeper's `addObserver()` to open the gadget, so a + * gatekeeper whose `addObserver()` always throws makes the gadget effectively unshareable + * once it has made one of these observations. + * - Once observed, the gadget enters a restricted mode: no more actions or public-web fetches, + * only observations, so the gadget cannot leak the data through other gatekeepers. + * + * TODO(someday): The restricted mode is a blunt instrument. It should be possible to perform + * actions whose visibility is limited to people verified to have access to the same data, + * but this requires a more complex policy framework to compute. */ containsRestrictedData?: boolean; diff --git a/plans/restricted-data-sharing.md b/plans/restricted-data-sharing.md new file mode 100644 index 000000000..82cbf3b5a --- /dev/null +++ b/plans/restricted-data-sharing.md @@ -0,0 +1,256 @@ +# Plan: Govern sharing of restricted data by observer verification + +## Goal + +Replace the all-or-nothing sharing lockdown that a restricted-data observation imposes +with a per-collaborator check: a workspace that has read restricted data stays +shareable, and each collaborator is admitted only while they are verified as an +observer of the gatekeeper that produced the data. + +**Known security limitation:** that guarantee is currently scoped by collaborator role. +A `use` collaborator is verified only against gatekeepers in their scope — those bound to a +gadget or fed by an enabled hook (`#useScopeGatekeeperIds`). The workspace +agent can nevertheless read an unbound gatekeeper through a chat binding (including an +ambient singleton), persist its restricted result into gadget storage or UI state, and +thereby expose it to an unverified `use` collaborator. This plan accepts that risk for the +current implementation; "Never-bound producers" below records the exact boundary and the +required future remedies. + +Delivered as **one PR, split into reviewable commits** (see "Commit sequence" at the +end). The kernel packages (`workshop-backend`, `workshop-shared`) get the small, +separated diffs; the rename, the UI, and the frontend share-key work ride in their own +commits. + +## Locked decisions + +- **The flag is renamed, not aliased.** `ObservationDescription.prohibitAllSharing` + becomes `containsRestrictedData`, and `GadgetMetadata.sharingProhibited` becomes the + same name. The flag states a fact about the data ("this observation contains + restricted data"); what the platform does about that is policy and does not belong in + the name. A hard rename means every gatekeeper call site moves in the same commit — + TypeScript's excess-property check on the object literals passed to + `authorizeObservation` will not tolerate a staged one. +- **The durable storage key keeps its old name.** Typed-storage keys *are* property names, + so renaming the overseer's singleton would silently unlatch every workspace that has + already observed restricted data. The property is renamed anyway, and declares the old + key explicitly: `containsRestrictedData: singleton(false, {storageKey: + "prohibitAllSharing"})`. `storageKey` is a typed-storage schema option added for this, + so the exception lives in the schema rather than as a special case at each call site. +- **Admission is per-collaborator, checked continuously.** Not at grant time: at every + `open()`, so revocation of a collaborator's underlying resource access is caught + promptly. Nobody is in the workspace without having passed the producer's + `addObserver()`, and anything that widens what they must pass restarts every live + session so it re-opens at the new scope (`#restartIfSessionsAffected`). +- **Coverage is held to each collaborator's own role scope.** `ensureObserver` never + verifies a `use` collaborator against a gatekeeper outside their scope. This is a liveness + tradeoff, not a security guarantee: restricted data can flow from that gatekeeper + through the agent into gadget-visible state. The exception for an unbound producer and + a `use` collaborator is the known security risk stated above. +- **Share-key redemption stays one-step.** Redeeming a key writes a real edge + immediately, as on main. The redeeming open then verifies the recipient like any other + collaborator. Two consequences are accepted on the ledger below: an unverified + redeemer, and a refused recipient, both persist in `listCollaborators` until removed. + Two-phase redemption (a pending edge granting nothing until verification confirms it) + is the planned follow-up fix for both. +- **One authorization gate for every non-owner entry point.** `authorizeCollaborator` + resolves the effective role and runs `ensureObserver`. Both `open()` and + `receiveExternalMessage()` pass through it; the latter non-interactively, since there + is no way to configure connected accounts from an inbound message. +- **Removing the producing connection is not guarded.** The latch stays set, but nothing + stops the removal even though the record is what collaborators are verified against. + There is no UI to remove a connection today; when one is built, it will require the + owner to certify that no sensitive data from the connection has been retained in the + workspace, for any connection. +- **Fail closed everywhere.** An operational failure — provider outage, expired + credential — is treated exactly like a refusal. + +## Current-state anchors (for orientation) + +- `authorizeObservation` (overseer.ts) is where a gatekeeper's observation is admitted + or refused, and where the durable restricted-mode flag latches. +- `ensureObserver` (overseer.ts) brings a non-owner into compliance for their role: + selects in-scope gatekeepers, prompts for unconfigured account choices via + `configureCb`, calls `addObserver` on each gatekeeper facet, and persists an + `ObserverRecord` only after all of them succeed. Re-runs on every open. Throws to deny. +- `SharingManager` (sharing.ts) owns the permission graph: collaborator records, their + `addedBy` edges, share links and keys, and `computeEffectiveRoles`' fixed-point + resolution. The module header states that sharing *policy* deliberately lives outside + it. +- `#inScopeGatekeepers(role)` derives what a collaborator must be verified against. + `use` scope is live gadget-binding state; `build` scope is broader. + +## Design + +### 1. Admission + +Coverage is enforced by admission rather than per observation. `ensureObserver` verifies +each collaborator against every in-scope gatekeeper at every `open()`, and +`#restartIfSessionsAffected` aborts the DO whenever that scope widens (a connection added, +one bound into a gadget, a merge promoting such a binding, a hook enabled), so no live +session outlives the scope it was verified at. It is a no-op unless a collaborator session +of the widened role is live — severing sessions is all a restart does. + +### 2. One-step share-key redemption (sharing.ts) + +`redeemShareKey` keeps main's shape: hash the key, resolve the link, and write a real +`shareKey` edge (creating the collaborator record if they're new), deduplicating against +an existing edge for the same link. + +The edge is real before the redeeming open's observer verification runs; the two +resulting windows (an unverified redeemer and a refused recipient, each persisting in +`listCollaborators` until removed) are the accepted consequences on the ledger, marked by +the TODO at `redeemShareKey`. + +### 3. The unified gate (`authorizeCollaborator`) + +Resolves the effective role, denies below `requireRole` *before* verification runs, then +calls `ensureObserver`. This PR introduces the gate with both non-owner entry points as +callers: `open()` interactively and `receiveExternalMessage` non-interactively (the +latter previously checked only the role). + +Denying early matters: without it a `use` collaborator reaching `receiveExternalMessage` +would be verified (real `addObserver` calls, a persisted record) only to be turned away, +or worse, told to fix a verification failure that could never grant them access. + +Because redemption writes a real edge, a redeemer mid-verification is visible to the +revocation affected-set like any collaborator: a link revoked (or a removal landing) +while their open is parked triggers the revocation restart, which severs their session +and re-runs `open()` against the live graph. + +### 4. Observer records on a failed live check + +An earlier draft scrubbed the failed gatekeeper from the collaborator's persisted +`accountChoices` and restarted the workspace on the failure. The observer machinery +landed on main (#380) without either: an `accountChoices` entry records the account the +collaborator chose so they are not asked again, and asserts nothing about whether the +gatekeeper still admits them — every open re-runs `addObserver`, so a revoked +collaborator is denied at their next open regardless, and only that open is denied (the +lazy-revocation residual in `docs/observers.md` edge case 3). Nothing in this model reads +`accountChoices` to admit a restricted read, so the scrub is not a precondition of it. + +### 5. Frontend + +- **Share modal**: no longer replaces itself with a "can't be shared" view. Controls stay + live behind a notice. +- **Retained share keys** (`retainedShareKeys.ts`, new): the `#share=` fragment is + stripped from the URL on open, so a failed open had nothing to retry with. The key is + held in `sessionStorage` under a versioned, per-workspace key, and replayed on the next + attempt. (Note: under one-step redemption a failed open leaves a real edge, so the + retry is keyless — revisit this rationale when the follow-up branch rebases.) +- **Identity stamping**: because `sessionStorage` outlives the session that wrote it, each + entry records the capturing user's id. A read by a different identity ignores *and* + sweeps it, and `logout()` sweeps the whole prefix including malformed and older + unstamped entries. Without this, one user's pending share key could be auto-redeemed + under the next user's account in the same tab. +- **The in-memory tier is bound to its capturing stub**: it is replayed only on the same + `authenticatedApi` that captured it; any other stub falls through to the + identity-checked storage tier. This removes the reliance on the rendering invariant + that an identity change unmounts the editor -- true today, but enforced two files away. +- **Stamps are generation-gated**: the async identity stamp commits through a write token + taken at capture; clearing a workspace's entry (a successful open) or the logout sweep + voids every earlier token, so a stamp resolving late cannot resurrect a cleared key. + The invalidation lives in `retainedShareKeys.ts` because the storage outlives any one + attempt -- a per-attempt flag guards only its own attempt's writes. +- **A superseded open bails after its identity await**, before creating any capability: + its cleanup already ran with nothing to dispose, so proceeding would mint a stub + nothing can reach and publish a stale (or wrong-workspace) capability. + +## Commit sequence (one PR) + +Ordered so the kernel-critical diffs are isolated. Every commit type-checks green across +`workshop-shared`, `workshop-backend`, `workshop-frontend` and `gatekeeper-google`. + +This PR is built directly on main and carries the model change alone. The observer +machinery it builds on has a set of preexisting concurrency races (and this PR's own +model adds atomicity hardening on top); those fixes are deferred to follow-up work. +Each deferred fix is acknowledged at its site with a `TODO` comment; the docs collect +the same items in their Known-limitations sections. The observer-side groundwork this +plan once carried — the unified `authorizeCollaborator` gate on the external-message +path, and the scope-widening restart — landed separately in #380. + +1. **Refactor — the rename.** Mechanical, no behavior change, spanning + `workshop-shared`, `workshop-backend`, `workshop-frontend`, `gatekeeper-google`, + `gatekeeper-mcp` and the gatekeeper-authoring skill doc. Atomic by necessity. +2. **Part 1 — API.** The restated contract on `containsRestrictedData`. Server still + implements the old behavior. +3. **Part 2 — core server implementation.** The latch, the removal of `hasAnyShares` + and the sharing checks, and the TODO ledger. +4. **Part 3 — backend tests.** +5. **Part 4 — integration tests.** Over real Durable Objects, through the test + gatekeeper fixture's `readValue(restricted)` and its controllable verification + outcome. +6. **Part 5 — documentation.** `docs/observers.md` coverage rules and residuals; + `docs/sharing.md` one-step redemption; this plan. +7. **Part 6 — drop the producer guards per review.** Deletes the unverifiable-producer + refusal, the producer-removal guard, `assertNewSharingAllowed` and the + `assertGrantAllowed` plumbing, the action-log scan, and the legacy flag shim. + +The deferred items are collected in the Known-limitations section below. The Share +modal unblock and the retained-share-key frontend work live in +`restricted-data-followups`. + +## Known limitations + +Revocations and role changes take effect within seconds (the revocation restart lands in +~100ms), and read-side races inside that envelope are accepted by design: a deferred fix +stays on this ledger only if its failure mode is *persistent* wrong state that outlives +the window. Each item is marked in the code by a matching `TODO` comment; this ledger is +the follow-up worklist. + +- Observer verification is not serialized per profile, so concurrent opens by one + collaborator can overwrite each other's records and registrations. +- A mid-registration observer named in `excludeObservers` is read as unknown and the + observation admitted: a first-time `ensureObserver` registers the id with the + gatekeepers before the record is persisted, so `#enforceExcludeObservers` cannot map it + back during that window. Persistent (the observation lands in chat history). Fix: an + in-memory pending-id map consulted there, failing closed — `observer-verification-fixes`. +- An unverified redeemer persists as a collaborator: redemption writes a real edge + before the redeeming open's verification runs, so from click onward the recipient is + visible in `listCollaborators` whether or not they ever complete the open (remedies: + verify, remove, or revoke the link). Two-phase redemption is the planned fix. +- A refused recipient persists: a recipient whose verification is refused keeps their + edge and stays in `listCollaborators` until removed; the same planned fix. +- A failed re-verification denies only the open being attempted. Sessions the + collaborator already holds keep the access their own opens verified until they next + re-open — the lazy-revocation residual `docs/observers.md` edge case 3 already accepts, + so it grants no access they do not already hold. + +## Known edge cases / watch-fors + +- **Operational failures deny like refusals.** An outage or expired credential denies the + collaborator's open exactly as a revocation does (the overseer cannot tell them apart); + they get back in as soon as a repaired open re-verifies them. Fail-closed by design. +- **Role increases do not ride out on a redeeming open.** An owner grant landing while + verification waited takes effect at the recipient's next open, exactly as for an + ordinary keyless open. + +## Accepted tradeoffs / future work + +- **Formerly-bound producers.** Unbinding shrinks `use` scope with no guard, so a + formerly-bound producer's sensitive reads stop requiring `use` collaborators' + coverage. Accepted because `use` sessions cannot read chat history or the action log; + the data entered gadget storage while the producer *was* bound, when every `use` + collaborator was verified against it or could not open the workspace; and re-binding + restores verifiability at the next open. The residual is `use` grants created after the + unbind. The chat-history argument does not cover a binding loopback retained across the + unbind, which returns the read directly as an RPC result and stays callable until + `removeGatekeeper`; that is the `docs/observers.md` Step 5 known gap, with `#assertBindingEdgeLive` as the + named fix. +- **Known security risk — never-bound producers.** A producer reachable only through chat + bindings (including an ambient singleton) is never in a `use` collaborator's verification + scope. The agent can read restricted data from it, persist the result into gadget code, + storage, or UI state, and the collaborator can then read that state through the deployed + gadget despite never passing the producer's `addObserver()` check. Role-scoped + verification deliberately never asks this collaborator about that producer, so + `containsRestrictedData` does not prevent this disclosure. Binding the producer makes + future opens verifiable but does not retract data already exposed. Accepted temporarily + to avoid making the read permanently unavailable + under the current role-scoped model. The required fix is either workspace-wide observer + verification for `use` collaborators or enforceable provenance that prevents data from an + unverified producer reaching their gadget-visible state. Both this and the formerly-bound + residual are documented at `docs/observers.md` edge case 4. +- **`calculate()`-style aggregates are out of scope here.** This plan governs *who* may + see restricted data, not what an aggregate over it discloses. +- **Verification remains interactive-only.** `receiveExternalMessage` can verify but + cannot configure, so a caller with unconfigured account choices is told to open the + workspace. A non-interactive configuration path is future work.