Rename prohibitAllSharing to containsRestrictedData - #234
Conversation
github-actions
Bot
added
workshop/frontend
c04d5e3 to
aa71300
Compare
fd04b0d to
42aefbf
Compare
cf3449d to
f633a1a
Compare
5f1ff10 to
e4e661c
Compare
f633a1a to
bc6f07a
Compare
Preview:
|
413e955 to
b2c3e58
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
cd22d16 to
b7c9372
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
41ae6a0 to
740374a
Compare
|
|
||
| const isOwner = !metadata.owner | ||
| const sharingProhibited = metadata.sharingProhibited === true | ||
| const containsRestrictedData = metadata.containsRestrictedData === true |
There was a problem hiding this comment.
🟡 Kernel and UI changes are shipped in the same commits, against the repository's review rule
The change edits the kernel and the browser UI together in single commits (packages/workshop-frontend/src/ShareModal.tsx:374 alongside packages/workshop-backend/src/overseer.ts), so the kernel cannot be reviewed apart from the UI as the repository requires.
Impact: Reviewers cannot review the security-critical kernel diff independently, which the project's review policy mandates.
Rule and commit layout
REVIEW.md ("High-scrutiny areas"): "A large kernel change should be split by concern into separate PRs, and at minimum grouped into commits that let workshop-backend/workshop-shared be reviewed apart from UI. Flag PRs that bundle both."
Commits 6b778a1 and 1991473 each touch packages/workshop-backend/src/overseer.ts, packages/workshop-shared/src/api.ts, packages/workshop-shared/src/gatekeeper.ts and packages/workshop-frontend/src/ShareModal.tsx in one commit.
Prompt for agents
Per REVIEW.md, kernel changes (packages/workshop-backend, packages/workshop-shared) must be reviewable apart from UI changes. Restructure the history so the ShareModal.tsx (workshop-frontend) edits live in their own commit(s) separate from the overseer.ts/api.ts/gatekeeper.ts changes, or split the frontend change into its own PR.
Was this helpful? React with 👍 or 👎 to provide feedback.
| vendorId !== null && this.#inScopeGatekeepers("use").some(gk => gk.id === gatekeeperId); | ||
|
|
||
| for (let collaborator of collaborators) { | ||
| // A verifiable gatekeeper outside a "use" collaborator's scope is one the UI can't invoke | ||
| // and ensureObserver can't cover; only the unverifiable case above blocks regardless of | ||
| // role. An absent role means "build" (see CollaboratorInfo), which fails safe here. | ||
| if (vendorId && (collaborator.role ?? "build") === "use" && !inUseScope) continue; | ||
| let observer = vendorId ? this.storage.observers.get(collaborator.profile.id) : undefined; | ||
| if (!observer || !(gatekeeperId in observer.accountChoices)) { | ||
| throw new Error( | ||
| "This observation was blocked because it contains sensitive data, but this " + | ||
| `workspace is shared with ${collaborator.profile.name} (${collaborator.profile.id}), ` + | ||
| "who has not been verified to have access to that data. They must re-open the " + | ||
| "workspace (which verifies their access) or be removed from it before this data " + | ||
| "can be read."); | ||
| } |
There was a problem hiding this comment.
🟨 A "use" collaborator can now open a workspace holding restricted data without being verified against the connection that read it
Removing the wholesale block in open() (packages/workshop-backend/src/overseer.ts:6751-6756) means a non-owner may now open a workspace that has observed containsRestrictedData, and verification is only run for gatekeepers in their role's scope. For a use collaborator, #inScopeGatekeepers("use") (packages/workshop-backend/src/overseer.ts:6091-6112) only includes connections bound by a non-pending gadget, so a connection that read restricted data through the agent's chat bindings only (never bound to a gadget) is never verified against them, and the observation-time coverage guard explicitly skips them for that connection (packages/workshop-backend/src/overseer.ts:3002). If the agent persists that restricted data into gadget code/storage, the use collaborator renders it through the gadget UI with no access check at all — previously impossible, since a latched workspace refused every non-owner open.
Was this helpful? React with 👍 or 👎 to provide feedback.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| throw new Error( | ||
| "This observation was blocked because it contains sensitive data, but this " + | ||
| `workspace is shared with ${collaborator.profile.name}, ` + | ||
| "who has not been verified to have access to that data. They must re-open the " + | ||
| "workspace (which verifies their access) or be removed from it before this data " + | ||
| "can be read."); | ||
| } |
There was a problem hiding this comment.
🟨 Blocked sensitive read names a collaborator to sandboxed gadget and agent output
The coverage-guard error embeds collaborator.profile.name (packages/workshop-backend/src/overseer.ts:3020) and, as its own comment notes, this message is surfaced to sandboxed gadget code and agent output — an audience that cannot otherwise enumerate collaborators. The comment claims the profile id is omitted because it is "often an email address", but profile.name is the user-supplied display name and, for password signups, is set to the username/email (see createAccount(username, username, ...) usage), so the identifier the guard intends to withhold can still be exposed.
Was this helpful? React with 👍 or 👎 to provide feedback.
f9c4f0e to
f53bdac
Compare
Rename prohibitAllSharing to containsRestrictedData: the flag now means "this observation carries data whose sharing is governed by observer verification" rather than a wholesale ban on sharing the workspace. The legacy flag is still read on persisted records and converted on load. Instead of refusing to share a flagged workspace outright, sharing is admitted when every collaborator is verified as an observer of the producing gatekeeper: - open() verifies each collaborator against every gatekeeper in their role's verification scope (ensureObserver), and the coverage guard (#assertSensitiveObservationCoverage) blocks a sensitive observation naming any current collaborator not yet verified against its producer. Coverage is held to each collaborator's role scope, since a "use" collaborator can never be verified against a gatekeeper no gadget binds. - Share-key redemption writes a *pending* edge that grants no interim authority: the redeeming open() verifies the recipient at the role the edge would grant, confirms only on success (capped at the verified role, denied if the connection/binding topology changed mid-verification, merged if a concurrent redemption landed first), and rolls the edge back on refusal or a null effective role. - Removing the producing connection no longer lifts the restriction for existing collaborators, and assertNewSharingAllowed refuses new grants inside the grant write's synchronous block. - The coverage-guard error reaches sandboxed gadget/agent output, so it names the collaborator but omits their profile id (the full email on OAuth/CF Access deployments). The accepted residuals (formerly-bound and never-bound producers outside "use" scope) are documented in docs/observers.md edge case 4 and docs/sharing.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e scope New sensitive-observations and observer-role-scope suites, with the harness, RPC client, and gatekeeper-test fixture support they need (the fixture gains a configurable observer hook and sensitive reads). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Update gatekeeper-google to the renamed flag, and revise the gatekeeper-mcp README and write-gatekeeper skill guidance to describe observer-verified sharing rather than a wholesale sharing prohibition. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The modal no longer refuses to share a workspace with restricted data; it explains that collaborators must be verified against the producing connection at their next open. The share key is retained across failures and reloads (sessionStorage tier) so a recipient who is denied verification can retry, and is discarded at the first successful open so a later removal isn't undone by an automatic re-redemption. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
f53bdac to
a3f45c3
Compare
ensureObserver re-runs each gatekeeper's addObserver on every open, but a failure only denied the open: the collaborator's persisted observer record (and the gatekeeper-side registration, being preConfigured) survived untouched. #assertSensitiveObservationCoverage reads that record from other turns, so a collaborator whose provider-side access was revoked kept admitting the producer's restricted observations to their still-live older session indefinitely. Now fail() scrubs the failed gatekeeper from the persisted record synchronously with the failure determination -- covering both failure sites (verifier gone, addObserver threw) and cancel-after-reprompt -- and the terminal catch best-effort de-registers the invalidated gatekeepers alongside the newly-added ones (removeObserver is idempotent). The scrub is scoped to the failed gatekeeper; a repaired pass re-persists full coverage at step 6. A first-pass modal cancel never scrubs (no failure was determined). Known tradeoff, by design: an operational failure (vendor outage, expired credential) scrubs the same way -- the overseer cannot tell it from a settled denial -- so that producer's restricted reads are blocked workspace-wide until the affected collaborator re-opens successfully. Fail-closed. The fixture gatekeeper gains an optional per-resource verify outcome (resource-specific key, label-wide fallback) so the new integration test can prove the scrub is per-producer rather than account-wide. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Share-key redemption is two-phase: assertNewSharingAllowed runs synchronously with the *pending* edge write in redeemShareKey, but the granting write is confirmShareKeyRedemption, separated from it by the redeeming open()'s await windows (ensureCapsules, ensureObserver). The topology fingerprint is taken after those awaits, so a restricted-data producer removed in the window between the redemption gate and the snapshot -- an unverifiable legacy producer's remove() skips the share-link guard entirely -- appears in neither fingerprint, and the confirm admitted a recipient nobody can verify for the restricted data. confirmShareKeyRedemption now takes the same optional assertGrantAllowed callback the other grant-writing mutators use, invoked in the confirm's synchronous block before the pending flag is cleared or the missing edge re-added; an already-confirmed edge (an existing grant) skips it, matching redeemShareKey. The overseer passes assertNewSharingAllowed, and a throw lands in open()'s catch, which severs the pending edge. No integration repro: the window (the ensureCapsules await) has no fixture hook and needs an unverifiable legacy producer the fixture cannot mint. The callback pattern keeps the check unit-testable in the sharing.test.ts harness; existing redemption integration tests guard regressions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The accepted wart in confirmShareKeyRedemption (an owner removal racing a mid-verification redemption is quietly re-granted by the confirm) was documented, but not why it grants nothing an attacker could not already take: a pending-only recipient is invisible to listCollaborators, so the removal UI cannot target one mid-verification (a racing removal necessarily aimed at a previously confirmed edge); the re-add carries no incremental authority, since the recipient holds the live link and can re-redeem it manually; and the durable exclusion is revoking the link, which computeEffectiveRoles already inerts. Spell those three points out in the docblock and in docs/sharing.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The sessionStorage tier of share-key retention was keyed only by workspace id
and auto-resent on a fresh mount, while logout cleared only the auth token --
so user B opening the same workspace in A's tab silently redeemed A's
retained key under B's account.
Two complementary fixes, in a new retainedShareKeys.ts module (so useAuth
does not import the workspace hook):
- Entries are identity-stamped (v2 format, JSON {key, userId}) with the
capturing session's whoami, resolved from the same stub the open is issued
on. The retained-read path honors an entry only when the current session's
identity matches; a definite mismatch sweeps it, a transport failure leaves
it but does not attach the key. The common keyless open stays fully
pipelined -- identity is only resolved when a fragment key is captured
(async, gated against the success-discard racing it) or a stored entry
exists (rare: only after a reload mid-retry). v1 and malformed entries read
as absent.
- logout() sweeps the whole retention prefix (all format versions), before
the CF Access navigate-away.
The in-memory ref tier needs no stamp: it is bounded by the editor's
lifetime (logout unmounts it; CF Access logout navigates away). Residual: a
reload before the identity stamp lands loses retention, recovered by
re-clicking the invite link.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Old model: prohibitAllSharing blocked all sharing the moment sensitive data was observed. No one other than the owner could access a latched workspace.
New model: containsRestrictedData allows sharing but requires per-collaborator observer verification against each gatekeeper that read sensitive data. The workspace still goes into a restricted mode (no actions against gatekeepers, no web fetches), but collaborators can open it if they pass verification.
WIP