Fix two observer-verification gaps

docs/observers.md

@@ -275,10 +275,21 @@ 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: best-effort `removeObserver(record.observerId)` on the gatekeepers
-     added in *this* pass, do **not** persist the working record, and deny the open with a clear
-     message.
+   - 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 that synchronously scrubs the failed gatekeeper from the
+     *persisted* record, so commit-time re-checks (`assertCollaboratorStillVerified`) fail closed
+     immediately, 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 are handled by which kind of verification
+     this was: a *first-ever* verification (no record at call start) best-effort-removes the
+     registrations it added or invalidated and persists no record — that collaborator was never
+     admitted and their minted id would otherwise linger unresolvable — while a *re-verification*
+     deliberately keeps them, because the registration is what preserves forward exclusion for
+     the collaborator's still-live sessions (coverage was already scrubbed, so nothing vouches
+     for them; the id keeps resolving so `excludeObservers` keeps naming them). One exception: if
+     a racing teardown deleted the record mid-call, the just-re-asserted registrations reference
+     an id no record resolves, so the full in-scope set is removed.
 
 6. **Persist the observer record** (with merged `accountChoices` and `observerId`) only after all
    `addObserver` calls succeed. Storing/creating the record is the canonical moment the user
@@ -329,7 +340,14 @@ observation proceed is when the named observer has *already lost access* in the
 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.
+   no record, the id is not an active observer → ignore it — **unless** the id belongs to a
+   first-time verification still in flight: `ensureObserver` registers a freshly minted id with
+   gatekeepers (`addObserver`) before the record is persisted, and that window spans awaits
+   (sibling verifier RPCs, even the configuration modal). The overseer tracks such ids in an
+   in-memory pending map and **blocks** an observation naming one (fail closed, with a distinct
+   "collaborator currently being verified" message) rather than reading it as unknown — otherwise
+   the observation would proceed and the collaborator be admitted moments later with the data
+   already in chat history.
 2. Check sharing-graph reachability for that `profileId`
    (`SharingManager.getEffectiveRole` / `computeEffectiveRoles`).
    - **Still authorized → throw**, blocking the observation (degrade to per-observation
@@ -340,6 +358,15 @@ For each id in `description.excludeObservers`:
      to observe; if they ever regain access they reconfigure from scratch (Step 3).
 3. If, after evaluating all excluded ids, none are still-authorized, allow the observation.
 
+Additionally, `authorizeObservation` fails **every** observation closed — not just those naming
+excluded observers — whenever a revocation's restart is still pending
+(`#revocationRestartPending`, set synchronously with the sever in `tearDownLostObservers`): the
+DO abort that actually ends the removed user's live sessions runs only after the awaited teardown
+and listing-refresh phases, and once the teardown's `removeObserver` fan-out de-registers the
+observer, gatekeepers stop naming them in `excludeObservers` at all — so no per-gate check can
+see the removed user, and only blocking everything covers their still-live sessions. See
+`OverseerImpl.scheduleRevocationRestart`.
+
 This is the runtime counterpart of `addObserver`: `addObserver` covers observers configured
 *after* data was read; `excludeObservers` covers data read *after* observers were configured.
 Persisting the observation record itself is unchanged; we only gate it.
@@ -357,9 +384,12 @@ methods wrapping `SharingManager` mutations (`removeCollaborator`, `revokeShareL
 downgrades — see the matching methods on `OverseerClientInterface` and `SharingManager`):
 
 - 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. Any
+  non-empty affected set also sets `#revocationRestartPending` synchronously with the sever, so
+  `authorizeObservation` fails every observation closed until the revocation restart disconnects
+  the affected users' still-live sessions (the awaited fan-out here is part of why that restart
+  is not immediate).
 - 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
@@ -406,7 +436,14 @@ already in the JSDoc in `gatekeeper.ts`; add anything missing there rather than
    can open at all (`overseer.ts:2770`). Observer checks only matter when sharing is allowed.
 5. **Owner adds a new binding after sharing** — existing observers see an incremental modal for
    just the new binding on their next open, and may be denied if they lack access to the new
-   resource (inherent to the security model).
+   resource (inherent to the security model). Until that next open, their already-live sessions
+   watch the new connection's (non-restricted) observations unverified — the accepted residual of
+   verifying at open time. A connection added *while a collaborator's verification is parked* on
+   an await (the modal, verifier RPCs) is part of this same residual, not a bypass: the committed
+   record simply lacks an entry for it, and every consumer fails closed on absence —
+   `assertCollaboratorStillVerified` recomputes the in-scope set live against the persisted
+   record, and the next open re-verifies the uncovered binding — while their live session watches
+   like any other until re-open.
 6. **Performance** — `ensureObserver` does one `getVerifier` + one `addObserver` per in-scope
    gatekeeper per open. Parallelize with `Promise.all` and pipe the verifier promise straight into
    `addObserver`. Expensive gatekeepers cache on their side.
@@ -428,6 +465,9 @@ already in the JSDoc in `gatekeeper.ts`; add anything missing there rather than
     opens do not (record covers them) but still re-run `addObserver`.
   - a thrown `addObserver` denies the open and triggers best-effort `removeObserver` rollback on
     bindings added in the same pass, and does not persist the record.
+  - a failure against an *already-covered* binding scrubs that binding from the persisted record,
+    so commit-time re-checks (`assertCollaboratorStillVerified`) fail closed after a revocation
+    (edge case 3) instead of trusting coverage the live check just refused.
   - missing account → binding reported as a need to the callback; callback rejection denies open.
 - **`authorizeObservation` exclusion:** observation naming a still-authorized observer throws;
   observation naming an observer who lost access proceeds and deletes that observer record (+

packages/integration-tests/__tests__/external-message-verification.test.ts

@@ -0,0 +1,205 @@
+// Tests for the external-message authorization gate (authorizeCollaborator in overseer.ts):
+// receiveExternalMessage() must hold a collaborator to the same observer verification open()
+// applies -- non-interactively, since this path has no way to prompt for account configuration --
+// and must deny an insufficient role *before* verification runs.
+//
+// These live in their own file -- with their own harness, like every suite here -- so the suite
+// stays self-contained as the observer suites around it grow.
+
+import { afterAll, beforeAll, describe, expect, it } from "vitest";
+import type { RpcStub } from "capnweb";
+import type { AuthenticatedApi, PublicApi } from "@gadgets/workshop-shared/api";
+import type {
+  SubmitExternalMessageResult,
+} from "@gadgets/workshop-shared/external-message-gateway";
+import {
+  startTestGatekeeperHarness, TEST_GATEKEEPER_WORKER, TEST_VENDOR_ID, type Harness,
+} from "../src/harness.js";
+import {
+  accountLabel, connect, listConnectedAccounts, MAX_OBSERVER_PROMPTS, nextUsernames,
+  ObserverConfigRecorder, signUp, stubFor, waitFor, type ConnectedAccount,
+} from "../src/rpc-client.js";
+import { NetworkInterceptor } from "../src/network-interceptor.js";
+
+// Reason text shaped like what a gatekeeper actually reports on a settled denial. Its appearance
+// in the gateway's reply below is what proves a live verification round trip happened.
+const DENIED_REASON = "You do not have access to this thing.";
+
+let harness: Harness;
+let interceptor: NetworkInterceptor;
+
+beforeAll(async () => {
+  interceptor = new NetworkInterceptor();
+  interceptor.install();
+  harness = await startTestGatekeeperHarness();
+});
+
+afterAll(async () => {
+  const unmocked = interceptor.getUnmockedCalls();
+  await harness?.server.close();
+  interceptor.uninstall();
+  interceptor.reset();
+  expect(unmocked).toEqual([]);
+});
+
+async function withSession<T>(body: (api: RpcStub<PublicApi>) => Promise<T>): Promise<T> {
+  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<AuthenticatedApi>): Promise<ConnectedAccount> {
+  await api.provisionAmbientAccount(TEST_VENDOR_ID);
+  return waitFor("the test account to be provisioned", async () => {
+    const accounts = await listConnectedAccounts(api);
+    return accounts.find(a => a.vendorId === TEST_VENDOR_ID) ?? null;
+  });
+}
+
+/**
+ * Submit an external chat message as `callerEmail`, through the fixture worker's control surface
+ * (and so through the Workshop's real ExternalMessageGateway entrypoint).
+ */
+async function submitExternalMessage(input: {
+  callerEmail: string; gadgetKey: string; prompt: string;
+}): Promise<SubmitExternalMessageResult> {
+  const res = await harness.fetchWorker(
+    TEST_GATEKEEPER_WORKER, "http://gatekeeper-test.test/control/submit-external-message",
+    { method: "POST", body: JSON.stringify({
+        chatKey: `chat-${input.gadgetKey}`, messageKey: crypto.randomUUID(),
+        gadgetTitle: input.gadgetKey, ...input }) });
+  if (res.status !== 200) {
+    throw new Error(`submit-external-message failed with ${res.status}: ${await res.text()}`);
+  }
+  return await res.json() as SubmitExternalMessageResult;
+}
+
+/** Tell the gatekeeper what to do the next time it's asked to admit `label` as an observer. */
+async function setVerifyOutcome(
+    label: string, outcome: { allow: true } | { allow: false; reason: string }): Promise<void> {
+  const res = await harness.fetchWorker(
+    TEST_GATEKEEPER_WORKER, "http://gatekeeper-test.test/control/verify-outcome",
+    { method: "POST", body: JSON.stringify({ label, ...outcome }) });
+  if (res.status !== 204) {
+    throw new Error(`Setting the verify outcome failed with ${res.status}: ${await res.text()}`);
+  }
+}
+
+/** The workspace id behind an external gadgetKey -- the DO id the gateway derives from it. */
+async function externalGadgetId(gadgetKey: string): Promise<string> {
+  const res = await harness.fetchWorker(
+    TEST_GATEKEEPER_WORKER, "http://gatekeeper-test.test/control/external-gadget-id",
+    { method: "POST", body: JSON.stringify({ gadgetKey }) });
+  if (res.status !== 200) {
+    throw new Error(`external-gadget-id failed with ${res.status}: ${await res.text()}`);
+  }
+  return (await res.json() as { gadgetId: string }).gadgetId;
+}
+
+describe("external-message verification", () => {
+  it.concurrent("the external-message path verifies collaborators like open() does", async () => {
+    await withSession(async publicApi => {
+      const [alice, bob, carol] = nextUsernames("alice", "bob", "carol");
+      const aliceApi = await signUp(publicApi, alice);
+      const aliceAccount = await provisionAccount(aliceApi);
+      const gadgetKey = `external-${crypto.randomUUID()}`;
+
+      // Alice creates the workspace through the external channel. No test user has an AI model,
+      // so a submission that passes the authorization gate is rejected with the model message --
+      // which is what tells "passed the gate" apart from a gate denial below.
+      await expect(submitExternalMessage({ callerEmail: alice, gadgetKey, prompt: "hello" }))
+          .resolves.toMatchObject({
+            accepted: false, message: expect.stringMatching(/AI model/i) });
+
+      // Wire the workspace up over the web API: connect a Thing (an account-requiring connection,
+      // so collaborators must be observer-verified against it) and add Bob.
+      const gadgetId = await externalGadgetId(gadgetKey);
+      using overseer = await aliceApi.openGadget(gadgetId);
+      const gatekeeper = await overseer.newGatekeeper(aliceAccount.id, thingUrl("external"));
+      if (!gatekeeper) throw new Error("Failed to create the test connection");
+
+      const bobApi = await signUp(publicApi, bob);
+      const bobAccount = await provisionAccount(bobApi);
+      await overseer.addCollaborator(bob, "build");
+
+      // A stranger is turned away by role, before verification is ever attempted.
+      await signUp(publicApi, carol);
+      await expect(submitExternalMessage({ callerEmail: carol, gadgetKey, prompt: "hi" }))
+          .resolves.toMatchObject({
+            accepted: false, message: expect.stringMatching(/do not have access/i) });
+
+      // Bob has build access but has never opened, so he was never observer-verified -- and this
+      // path has no configuration channel to fix that. The agent's reply could surface anything
+      // the workspace has already read, so the external path must refuse him rather than fall
+      // through to the model check.
+      await expect(submitExternalMessage({ callerEmail: bob, gadgetKey, prompt: "hi" }))
+          .resolves.toMatchObject({
+            accepted: false, message: expect.stringMatching(/could not be verified/i) });
+
+      // Opening the workspace verifies him; the same submission now passes the gate and fails
+      // only on the missing AI model, exactly like the owner's did.
+      const callback = stubFor(
+          new ObserverConfigRecorder().alwaysChoose(bobAccount.id, MAX_OBSERVER_PROMPTS));
+      try {
+        (await bobApi.openGadget(gadgetId, undefined, callback))[Symbol.dispose]();
+      } finally {
+        callback[Symbol.dispose]();
+      }
+      await expect(submitExternalMessage({ callerEmail: bob, gadgetKey, prompt: "hi" }))
+          .resolves.toMatchObject({
+            accepted: false, message: expect.stringMatching(/AI model/i) });
+
+      // The gatekeeper now revokes Bob's underlying access. His persisted observer record is
+      // untouched, so only a live addObserver re-verification on this submission can notice --
+      // and the gatekeeper's own refusal reason appearing in the reply is the proof that round
+      // trip happened, since nothing persisted in the Workshop contains it. An implementation
+      // that merely checked the record would keep accepting him here.
+      await setVerifyOutcome(accountLabel(bobAccount), { allow: false, reason: DENIED_REASON });
+      const revoked = await submitExternalMessage({ callerEmail: bob, gadgetKey, prompt: "hi" });
+      if (revoked.accepted) throw new Error("The revoked submission was accepted");
+      expect(revoked.message).toMatch(/could not be verified/i);
+      expect(revoked.message).toContain(DENIED_REASON);
+    });
+  });
+
+  it.concurrent("the external-message path denies a use collaborator by role, not verification",
+      async () => {
+    await withSession(async publicApi => {
+      const [alice, dave] = nextUsernames("alice", "dave");
+      const aliceApi = await signUp(publicApi, alice);
+      const aliceAccount = await provisionAccount(aliceApi);
+      const gadgetKey = `external-use-${crypto.randomUUID()}`;
+
+      // Alice creates the workspace through the external channel (the AI-model rejection means
+      // her submission passed the gate), then binds its connection to a gadget so it falls in
+      // "use" verification scope.
+      await expect(submitExternalMessage({ callerEmail: alice, gadgetKey, prompt: "hello" }))
+          .resolves.toMatchObject({
+            accepted: false, message: expect.stringMatching(/AI model/i) });
+      const gadgetId = await externalGadgetId(gadgetKey);
+      using overseer = await aliceApi.openGadget(gadgetId);
+      const gatekeeper = await overseer.newGatekeeper(aliceAccount.id, thingUrl("external-use"));
+      if (!gatekeeper) throw new Error("Failed to create the test connection");
+      using gadget = await overseer.createGadget("Test Gadget", undefined, "TEST_GADGET");
+      await gadget.bind("TEST_THING", await gatekeeper.getId());
+
+      // Dave is in verification scope and unverified, but this path can never grant a "use"
+      // collaborator agent access, so his role is checked before verification runs: he gets the
+      // plain denial, not a verification failure he has no reason to go fix.
+      await signUp(publicApi, dave);
+      if (!await overseer.addCollaborator(dave, "use")) {
+        throw new Error(`Failed to share the gadget with ${dave}`);
+      }
+      await expect(submitExternalMessage({ callerEmail: dave, gadgetKey, prompt: "hi" }))
+          .resolves.toMatchObject({
+            accepted: false, message: expect.stringMatching(/do not have access/i) });
+    });
+  });
+});

packages/integration-tests/fixtures/gatekeeper-test/src/env.d.ts

@@ -12,6 +12,17 @@ declare namespace Cloudflare {
     // Storage classes exposed as DO namespaces on ctx.exports.
     durableNamespaces: "TestGatekeeper" | "TestControl";
   }
+
+  interface Env {
+    // The Workshop's external-message gateway entrypoint (see wrangler.jsonc). The contract
+    // interface is not entrypoint-branded (the shipping class implements it), so brand it here to
+    // satisfy Fetcher's constraint.
+    WORKSHOP_EXTERNAL_MESSAGES: Fetcher<
+        import("@gadgets/workshop-shared/external-message-gateway").ExternalMessageGateway &
+        Rpc.WorkerEntrypointBranded>;
+    // The Workshop's Overseer DO namespace (see wrangler.jsonc); used only to derive ids.
+    WORKSHOP_OVERSEER: DurableObjectNamespace;
+  }
 }
 
 interface ExecutionContext<Props = unknown> {