Extract the BigQuery observer probe out of google.ts - #372
Draft
Maximo-Guk wants to merge 30 commits into
Draft
Conversation
Preview:
|
Maximo-Guk
force-pushed
the
restricted-data-followups
branch
from
August 28, 2026 01:52
0be8366 to
97da468
Compare
Maximo-Guk
force-pushed
the
restricted-data-bigquery
branch
from
August 28, 2026 02:01
27034aa to
50a5904
Compare
The persisted observer record is the standing claim that a collaborator was verified for a producer -- `ensureObserver` reads it back on their next open and re-registers them off the account choice it holds, and `authorizeObservation` reads it from other turns. So a collaborator whose live re-verification just failed must not keep an entry saying they are covered: until now a revoked collaborator stayed "verified" until their next *successful* open. `fail()` now drops the failed gatekeeper from the persisted `accountChoices` synchronously with the failure determination, `getVerifier` moves inside the per-gatekeeper `try` so a verifier-acquisition rejection scrubs like any other refusal (and surfaces the descriptive denial rather than the raw RPC error, with no mid-flight `Promise.all` rejection to stale the rollback snapshot), and the terminal catch de-registers invalidated registrations alongside newly-added ones. That last part is a fail-open regression for a *returning* observer, marked with a TODO here and fixed in the next commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The persisted observer record is meant to state what a collaborator's most recent open verified, and `ensureObserver` re-registers them off the account choices it holds. But a choice for a gatekeeper outside their current verification scope survived every open that could not check it: a "use" collaborator who opens while a connection is unbound from every gadget verifies nothing against it, yet their stale entry stays -- and the moment the connection is rebound (rebinding keeps the same gatekeeper id) the next open silently re-registers them off a choice made for a scope the workspace no longer has, instead of asking them again. Step 2 now drops every account choice for a gatekeeper outside the collaborator's live verification scope, even when the remaining scope is empty (that is exactly the everything-unbound open). The gatekeeper-side registration is deliberately kept: it preserves forward exclusion via `byObserverId`, and the next successful open's `addObserver` overwrites the verifier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…n fails. `ensureObserver`'s rollback removed the gatekeeper registrations of every binding that failed the call (`invalidated`), not just the ones the call created (`newlyAdded`). For a collaborator who was already an admitted observer, that de-registered them from a gatekeeper they had previously been verified against -- and a de-registered observer is one the gatekeeper stops naming in `ObservationDescription.excludeObservers`, so an observation it would have excluded them from is admitted with nothing left to block it. The coverage scrub this rollback accompanies is not a substitute for the registration. They cover different sets: `gatekeeper-confluence` -- the only in-repo producer of `excludeObservers` -- never marks an observation `prohibitAllSharing`, so for it the scrub covers none of the affected reads. The reachable sequence is a collaborator whose Confluence access is revoked upstream, whose re-open therefore fails, and whose pre-existing live session then watches the owner's agent read a page they cannot access. So roll back `invalidated` only on a first-ever verification, where the minted observerId is discarded along with the unpersisted record and a registration left behind would linger unresolvable. A returning observer's id is already persisted, so keeping their registrations is fail-closed (a registration can only add exclusion names) and the next successful open's `addObserver` overwrites the verifier. This restores the invariant `registeredBeforeCall` was introduced to state: roll back only what this call added. Coverage is still scrubbed either way, so a revoked collaborator's record stops claiming they were verified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ens.
Authorization and observer verification run only at open(). Nothing re-ran
them when the set of gatekeepers a collaborator must be verified against
*grew* mid-session -- adding a connection, or binding one into a gadget --
so a collaborator who opened before the growth kept a live session holding
access they were never verified for.
Fix it with the mechanism already used to revoke a collaborator: generalize
scheduleRevocationRestart() to scheduleAccessRestart(reason) and add
#restartIfShared(), which flushes, waits 100ms and aborts the DO so every
client reconnects and re-opens against the new scope. It is a no-op when the
workspace has no collaborators, so a solo workspace is never disturbed.
Four sites widen scope and now restart: addGatekeeper, a permanent
bindWorkpiece, a merge that promotes a binding edge into "use" scope, and a
denied re-verification that scrubbed a persisted account choice. The merge
case compares the effective account-requiring "use" scope before and after
promotion rather than restarting on any promotion, since most merges promote
neither a gadget with bindings nor an edge to a connection anyone is verified
against. It reads the scope through the non-throwing gatekeeperVendorId()
rather than #inScopeGatekeepers("use"), whose observerVendorId() throws on a
legacy record with no creationSpec -- an unrelated legacy connection must not
turn an accepted merge into an error.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`addGatekeeper` published the gatekeeper record before awaiting the gatekeeper's `describe()`, because `getGatekeeperFacet(id)` resolved the class from that record. The DO's input gate is open across the await and ids are allocated sequentially, so a live `build` session could guess the id and `getGatekeeperById()`/`openSession()` on the owner's brand-new connection -- which gates on nothing but record existence -- for as long as `describe()` took, all of it before `#restartIfShared` severed it. `getGatekeeperFacet` now optionally takes the class directly, so the record is published exactly once, after `describe()` resolves. Nothing a gatekeeper's `describe()` can reach calls back into the overseer to resolve itself by record, so no caller needs the early put. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
receiveExternalMessage() checked only the caller's role. Observer verification -- which is how a collaborator earns the right to see what the workspace has read -- runs at open(), so a "build" collaborator who never opened the workspace, or whose upstream access was since revoked, could still drive the agent and have it answer out of chat history and gadget storage. Extract the gate open() applies into authorizeCollaborator(): resolve the effective role from the permission graph, then run ensureObserver for that role. Both entry points call it. The external path passes requireRole: "build", so an insufficient role is denied before verification runs -- a "use" collaborator would otherwise be verified, or told to go fix a verification failure, for access this path can never grant them. It also passes no configureCb, since there is no channel to prompt on: an unverified caller is told to open the workspace in a browser, which is where configuration happens. roleRank is exported for the requireRole comparison, so it ranks rather than string-compares. Also has open() await ambient reconciliation before authorizing rather than between the role check and verification, which is where the two halves now join. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
docs/observers.md gains a "Restarting when verification scope widens" subsection: the four triggers, why the merge trigger compares scopes rather than firing on any promotion, why shrinking scope and role rises are deliberately not triggers, why addGatekeeper's publication order is load-bearing under the restart, and where the enforcement moment actually falls for each trigger. Step 3 gains the record prune, the scrub-and-restart failure path and the returning-observer rollback rule; edge cases 3 and 5 are rewritten around them, and Step 6's justification for an orphaned entry is corrected -- a registration is what admits an open, so a stale one grants nothing on its own. docs/sharing.md renames scheduleRevocationRestart and documents the abort's second purpose, whose trigger is a grant rather than a revocation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A schema property name is also the KV key it maps to, so renaming a property
in code is a storage migration. Give a singleton slot somewhere to say
otherwise: `singleton(defaultValue, {storageKey})` declares the key on disk
explicitly, and a bare default value stays the shorthand for the common case
and behaves exactly as before. Collections get the same option as
`storageName`, which prefixes the records and every index alike.
This is the schema-level version of what would otherwise be a special case at
each call site, and it keeps the old name on disk with no migration.
The flag's real meaning is "this observation contains restricted data". What the platform does about that is policy, which shouldn't be baked into the name -- the next commits replace the all-or-nothing lockdown with per-collaborator observer verification. ObservationDescription.prohibitAllSharing and GadgetMetadata.sharingProhibited both become containsRestrictedData. No alias: this is a hard rename, so the gatekeeper call sites move in the same commit. The overseer's durable singleton is renamed too, and declares its old name as its `storageKey` so nothing on disk moves. Without that, every workspace that has already observed restricted data would silently unlatch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Restate what `ObservationDescription.containsRestrictedData` means now that the enforcement is per-collaborator observer verification rather than an all-or-nothing sharing lockdown, and state the two limits of the model plainly: verification is held to the collaborator's role scope, and enforcement is at admission rather than at each read. No functional change; the implementation follows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ion. Reading restricted data no longer locks the workspace down. The old model blocked the observation outright if the workspace was shared and then refused all future sharing, which made every sensitive data source unusable the moment a workspace had a single collaborator. The observer verification machinery already answers the real question -- does this collaborator have access to the same data? -- at every open, and a widening of that scope now restarts every live session, so admission is a sound enforcement point. So: drop the `hasAnyShares()` block in `authorizeObservation` and the three guards on the sharing mutators. Keep the two guards that are about leaking data back out rather than about who may see it -- no actions and no public web fetches once the latch is set. What replaces them is narrower. A producer nobody can ever be verified against (a vendorless connection, or a legacy record with no `creationSpec`) is still refused while the workspace is shared, because `#inScopeGatekeepers` skips it and so admission cannot see it at all. Removing a producer's record is blocked while the workspace is shared, since that record is what verification runs against. And a new grant -- a collaborator, a share link, another key for one, or a redemption -- is refused if some producer can no longer verify anyone. Each of those checks runs in the same synchronous block as the write it gates, after every await, so a concurrent change cannot slip between check and write. `sharing.ts` loses `hasAnyShares()` and gains an optional `assertGrantAllowed` on each grant-writing method, invoked at that write. Two smaller things fall out. `getSharingManager()` moves inside the `containsRestrictedData` branch, so an ordinary observation on a cold DO no longer pays for an owner User DO round trip; the producer record is then read after that await, since latching against a stale record would permanently brick sharing. And the restart on a terminal re-verification failure is hoisted ahead of the best-effort rollback, taking a gatekeeper RPC fan-out off the path between determining the denial and the abort. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Covers the latch (what sets it, and the cases that must refuse the read rather than latch), the producer-removal guard and its exemptions, the grant checks on each sharing mutator, and the tolerance for action records written before the flag's rename. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Drives the model end to end through the test gatekeeper: a restricted read on a shared workspace, the unverifiable-producer refusal, the removal guard, the action and web-fetch blocks, and the restart that forces re-verification when scope widens. `TestSession.readThing()` takes an optional `restricted` flag so a test can trip the latch through the same `ApprovalQueue` funnel a shipping gatekeeper uses. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rewrites the observer document's model section around admission-time enforcement, states the two limits (role-scoped verification, and enforcement at admission rather than at each read) as edge cases with their reasoning, and records the design under plans/restricted-data-sharing.md -- including the known risk of a producer no gadget binds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Share modal no longer replaces itself with a "can't be shared" view when the workspace has read restricted data. Sharing controls stay live and a notice explains that collaborators must be able to see the data themselves. The server allows sharing after the restricted latch (assertNewSharingAllowed refuses only unverifiable producers) and GadgetMetadata.containsRestrictedData documents that such a workspace can still be shared, so the modal's job is to warn and to surface a server refusal verbatim -- which the existing toast catches already do. Regression tests pin both: with the flag set, the banner renders in place of the wall and every management affordance (invite, link creation/copying, collaborator removal, link revocation) stays reachable; and a server-side "can no longer be shared" rejection reaches the user as an error toast. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Opening with a #share= fragment strips the key from the URL, so an open that failed while the recipient's access was still being verified had nothing left to retry with. The key is now held and replayed on the next attempt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The retained key moves to sessionStorage so a reload can still retry, which means it can outlive the session that captured it. Each entry is therefore stamped with the capturing user's id and ignored -- and swept -- when the current session's id doesn't match, so one user's pending share key can never be redeemed under the next user's account in the same tab. logout() sweeps the whole prefix as well, including malformed and older unstamped entries. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ility. The retained-storage path awaits identity resolution before openGadget. An attempt superseded while parked there had already run its cleanup -- with nothing yet to dispose -- so on resuming it minted a stub its cleanup can never reach and published it over the replacement attempt's state: a stale capability, or the wrong workspace's when the id changed. Bail after the await, before any capability is created, like the checks the later awaits already have. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The in-memory retention tier carried no identity and was replayed on whatever authenticated stub the effect ran with. Its safety rested on a rendering invariant two files away -- that an identity change always unmounts the editor -- which nothing local enforced; an account switcher or soft logout would have silently turned it into a cross-user key replay. The ref now records the stub that captured it and is replayed only on that stub. Any other stub falls through to the sessionStorage tier, whose entries are identity-stamped and checked. Stub identity rather than an async userId keeps the common same-session retry pipelined. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The capture path's identity stamp is asynchronous, gated by a flag local to one load attempt -- but the storage it writes is global. A stamp resolving after a *different* attempt succeeded (or after logout swept the tier) wrote the entry back, resurrecting a key that could silently re-redeem the still-active link after an owner removes the collaborator. Invalidation now lives in retainedShareKeys.ts as generation counters: a capture takes a write token, and clearing a workspace's entry (or the logout sweep) voids every token taken before it. The per-attempt flag is deleted -- its scope was the defect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The server confirms a share-key redemption inside open(), before the client holds the capability -- but retention was discarded only after subscribeToMetadata resolved. That call has real post-open failure modes for exactly the keyed audience (the non-owner whoami round trip, a WS drop), and every error path keeps the key by design, so a confirmed-then-failed subscribe left all three retry paths (retry button, reconnect stub swap, remount) armed with a live key -- and a re-redemption after an owner removal silently re-grants access, since links are multi-redeemable and owner removal wipes edges but not the link. Keyed opens now await the open promise (one extra round trip, keyed opens only -- the pipelined RpcPromise stays usable as the stub) and discard both retention tiers the moment success is knowable; an open failure rejects there and keeps retention, matching the server's reverted redemption. The tail clear stays for the keyless corner where a retained entry existed but was not attached (the identity-unknown path). The denial tests now model the denial where it really lands -- openGadget's promise rejecting -- rather than as a subscribeToMetadata throw. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ained key. The finding-7 fix awaited the keyed open and then unconditionally discarded both retention tiers -- with no cancelled check, unlike every other side-effect site in the hook. The await can park across the attempt's cancellation, and a superseded attempt no longer owns the retention state: a newer attempt may have captured its own key -- possibly another user's, on a swapped stub -- into the very ref and sessionStorage entry the late clear wipes, and clearRetainedShareKey's write-token bump also permanently voids that attempt's still-in-flight identity stamp, so its failed open dead-ends unretryable. Bail before the clears when cancelled, matching the hook's invariant everywhere else. Skipping the clear loses nothing: replaying the superseded attempt's confirmed key later is a server-side no-op (a confirmed edge skips redemption), and whichever attempt next succeeds clears retention itself. The stub was assigned before the await, so the cleanup already disposed it and a plain return is correct. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The round-2 fix's blanket cancelled-bail before the post-open clears discarded positive knowledge: reaching that line means the open *resolved*, i.e. the server durably confirmed the redemption (nothing in disposal reverts it). A confirmed-then-cancelled attempt (unmount, stub swap, retry) left the identity-stamped sessionStorage entry behind -- the stamp is deliberately not cancelled-gated -- and every replay path later re-redeemed the still-live link, silently restoring access after an owner removal. Clearing is now attempt-owned: clearRetainedShareKey takes an `onlyKey` and no-ops (no removal, no generation bump) when the stored entry carries a different key, so a newer capture's retention and in-flight stamp survive -- which is what keeps the round-2 superseded-attempt test passing unchanged -- while a matching or absent entry is cleared and its pending stamp voided even after cancellation. The absent-entry bump is deliberate fail-toward-security; its residual (voiding a concurrent newer attempt's in-flight stamp) is documented with the recovery being a re-click of the invite link. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… other keys'. The round-3 attempt-owned clear (clearRetainedShareKey with onlyKey) returned without any invalidation when a different key occupied the entry -- so a superseded attempt A whose open the server confirmed, but whose identity stamp was still parked in whoami(), never voided that stamp: it landed late, overwrote the newer attempt B's entry with A's *confirmed* key, and a later mount replayed it -- re-redeeming the still-live link after an owner removal. The generations were per-workspace, so A's stamp could not be voided without also voiding B's. Add a per-(workspace, key) generation tier: beginRetainedShareKeyWrite now records the key it will stamp, commitRetainedShareKeyWrite checks all three tiers, and an onlyKey clear always bumps exactly its own key's generation -- voiding the calling attempt's stamp even when a different key occupies the entry -- while removing the entry only when it is absent or matching, and leaving the workspace generation alone. That last part also retires round 3's documented absent-entry residual: an attempt-owned clear can no longer void a concurrent newer attempt's in-flight stamp. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
In the retained-storage path, a cancelled attempt parked in whoami() resumed and mutated retention before the pre-open cancelled check: the identity-match branch re-armed the in-memory ref over a newer attempt's capture, and the mismatch branch called the unscoped clearRetainedShareKey(id) -- sweeping a newer attempt's entry and, via the workspace-generation bump, permanently voiding its in-flight identity stamp. Bail immediately after the identity resolves: a cancelled attempt no longer owns retention, so it must neither re-arm the ref nor judge an entry that may have been replaced while it was parked. And scope the identity-mismatch sweep to the key this branch actually read and judged (clearRetainedShareKey(id, retained.key), from the previous commit's key-scoped clears), so a newer capture's different-key entry and stamp survive even if the branch is ever reached with stale data. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Attempt-owned clears (the post-open success clear and the identity- mismatch sweep) identified their retention by (workspaceId, raw key), so two captures of the *same* invite key collided: after a same-tab user switch, user A's disposed open of key K resolving late would remove user B's freshly captured entry for the same K and permanently void B's in-flight identity stamp (the per-(workspace, key) write generation was shared), dead-ending B's retry on the access-denied page. An availability bug only -- clearing is the fail-safe direction. Each fragment capture now gets a unique captureId, stored in the entry, the in-memory ref, and the write token. Attempt-owned clears bump that capture's own generation and remove the entry only when it carries the same captureId, so a same-key successor capture survives both the removal and the stamp-voiding. Workspace-scoped and global clears are unchanged. The stored entry shape gains a required captureId with no migration: the v2 format exists only on this branch, so entries without one simply read as absent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Browsers copy sessionStorage into a duplicated tab, so a retained share-key entry cleared in the original tab lives on in the copy: after a successful redemption and a later collaborator removal, the duplicate's revocation-restart reconnect would silently re-redeem the still-live link, undoing the removal. Two frontend mitigations bound this without touching the kernel API surface. Entries now expire 15 minutes after their identity stamp is written (the legitimate failed-open retry/reload fits well inside that; a copy replaying after a later removal does not), and clears propagate across same-origin tabs over a BroadcastChannel -- capture-scoped clears, which a duplicate's copied entry answers to because it shares the original's captureId, and the logout sweep, since tabs share the login session. Workspace-scoped clears name no capture and deliberately stay local, so an independent sibling capture still legitimately retrying is never blanket-cleared. Documented residual: a duplicate discarded or unloaded at broadcast time that reactivates within the TTL can still replay once. The link itself stays multi-use server-side (docs/sharing.md already carries the matching manual re-redeem residual); a single-use server-side retry capability would close both and remains a possible kernel-side follow-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comments and the plan doc still described two-phase redemption: a failed open reverting the redemption server-side, so a retry had to re-send the key, and success "confirming" it. Under one-step redemption (#340) the edge is real the moment the server redeems, so a failure after that point retries keylessly and nothing is ever reverted or confirmed. The mechanism is unchanged and still earns its keep: an open can fail *before* the redemption lands (a transport failure, a server throw ahead of the redemption, an attempt superseded before issuing), and the client cannot distinguish that from a post-redemption failure, so it retains the key on every failure -- replaying a key whose edge already exists is a server-side no-op. Prose only; no behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
hasDatasetAccess passes on any 2xx from datasets.get, which requires only bigquery.datasets.get -- dataset *metadata* access (e.g. roles/bigquery.metadataViewer). It proves neither bigquery.tables.getData on the tables the workspace's queries actually read, nor row-level security, nor column policy tags, so an observer admitted by this probe may see query rows they could not query themselves. Decision: document only, no behavior change. Record the limitation and the tightening direction (per-table tracking via the dry-run's referencedTables, which is already in hand at both query call sites but currently reduced to dataset prefixes, plus a table-data probe) at the probe, in the observers decision table, and in the user-facing README. Full row/column-policy parity is unprovable via any Google API regardless: row-level security filters rows without ever denying access. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A table-free SELECT (`SELECT SESSION_USER()`, `@@project_id`) dry-runs with empty referencedTables, was allowed for project-only scope, tracked zero datasets, and executed with the owner's token -- returning owner/session context no dataset probe can verify an observer against. An empty tracked set admits any verifier without a single probe. INFORMATION_SCHEMA reads that dry-run with empty refs rode the same path. now refuses empty referencedTables on every binding shape, with a message pointing the agent at gadget code for table-free computations. The scalar-function-with-a-table variant (SELECT SESSION_USER() FROM t LIMIT 1) is unfixable at the referencedTables level and is documented as part of the existing metadata-class residual. BigQuerySessionImpl moves to its own module so Node vitest can construct it directly (google.ts imports workerd-only modules and bundled assets); the tests stub `cloudflare:workers` and `capnweb-validate`, the same pattern as mcp-shared. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Maximo-Guk
force-pushed
the
restricted-data-followups
branch
from
August 28, 2026 20:16
97da468 to
85082a7
Compare
Maximo-Guk
force-pushed
the
restricted-data-bigquery
branch
from
August 28, 2026 20:16
50a5904 to
8f18b17
Compare
|
LGTM! |
Maximo-Guk
force-pushed
the
restricted-data-followups
branch
2 times, most recently
from
September 11, 2026 15:17
c85f513 to
741f9cd
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WIP