Restricted data UI - share modal stays usable for restricted workspaces - #308
Restricted data UI - share modal stays usable for restricted workspaces#308Maximo-Guk wants to merge 16 commits into
Conversation
github-actions
Bot
added
workshop/frontend
Maximo-Guk
changed the title
Preview:
|
e176ac9 to
5fed7a7
Compare
557dc7d to
62e4ee7
Compare
62e4ee7 to
e0ceacb
Compare
e0ceacb to
b7b46ab
Compare
b7b46ab to
80e155d
Compare
80e155d to
2ef65ed
Compare
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>
85082a7 to
f611270
Compare
|
Maximo-Guk
changed the title
This is true, but I mean, the user could easily just copy the share key to there notepad or whatever. The workspace owner should be revoking the share url if they don't want people using it...
Fixing
They can just re-click the link if this happens, and this was an issue before anyways.
Fixing |
![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.
…er capture. Write tokens tracked clears but not newer captures, so two captures for one workspace with identity stamps in flight raced on the entry: when the older capture's whoami() resolved last, its stamp overwrote the newer capture's entry. Nothing had cleared, so no generation moved -- the older key (spent or not) became what a reload replayed under the newer capture's session. Starting a capture now bumps the workspace generation before taking its token, so every older pending stamp for that workspace fails its workspace check at commit time. The newest capture owns the slot outright, whether or not the older attempt ever succeeded or cleared. Other workspaces' pending stamps are untouched, and the per-capture tier keeps its job: it is still what lets the newest capture's own success clear void its stamp without voiding a later capture's. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…emory key. The cross-tab clear broadcast swept a duplicated tab's sessionStorage copy but not the two other places the same tab could still hold the key: the hook's retainedShareKeyRef, re-armed from the copied entry (same captureId), and a local the reload path had read the entry into before parking in its identity await. "Try again" on the same stub replayed the ref, and the resumed await attached the stale local -- so a live duplicate re-redeemed the cleared key exactly as if nothing had been broadcast. Clears now notify subscribers (subscribeToRetainedShareKeyClears): capture-scoped clears and the logout sweep, local and received alike, with a capture-scoped clear reported whether or not a stored entry matched, since the ref is a separate tier. Workspace-scoped clears stay unreported: they name no capture, are never broadcast, and the hook drops its own ref before issuing them. A listener's throw cannot break the clear. The hook subscribes and drops its ref on a clear naming its capture (or on the sweep), leaving a newer local capture with a different id untouched; and the reload path re-reads storage after the identity await, proceeding keylessly when the entry it judged has since been swept (sibling broadcast, logout, or TTL). The residual shrinks to what the module header already documents: only a duplicate unloaded at broadcast time that reactivates within the TTL can still replay once. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Both fixes are pushed, one commit each:
Residual is unchanged from what the module header and docs/sharing.md already state: a duplicate that was unloaded at broadcast time and reactivates within the 15-minute TTL can still replay once. Closing that needs the single-use server-side retry capability, which stays a kernel-side follow-up. |
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
|
|
||
| function applyCaptureClear(workspaceId: string, captureId: string): void { | ||
| // The generation is bumped before the removal so no in-flight commit can land between the two. | ||
| captureGenerations.set(captureId, (captureGenerations.get(captureId) ?? 0) + 1) |
There was a problem hiding this comment.
🟡 Cleared capture generations accumulate forever
Each successful share-link open adds its random captureId to captureGenerations. Nothing removes settled captures, so the SPA retains every identifier indefinitely.
Learn more
captureGenerations stores tombstones so a late identity-stamp commit cannot resurrect a cleared key. Each fragment capture uses a fresh UUID, and successful keyed opens call clearRetainedShareKey, which reaches this insertion. The tombstone cannot simply be deleted immediately because an outstanding write token would then see the default generation again and become valid.
Example: A long-lived tab opens 10,000 distinct share links. All opens finish, but captureGenerations still contains 10,000 UUID keys although no retained entry uses them.
Recommended fix: Track outstanding write tokens per capture. Remove a capture’s generation after its final pending commit is consumed or invalidated, while preserving a tombstone until no stale token can still arrive.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
1 participant
Frontend follow-ups to the restricted-data series (#380, #381, #382).
The modal no longer replaces itself with a "can't be shared" wall when the workspace has read restricted data.
Opening a
#share=link strips the key from the URL. If the open then failed, for example while the recipient's access was still being verified, there was nothing left to retry with. The key is now retained and replayed on the next attempt.