Skip to content

Share a key into a realm: suspend on loss of write, swap, and delete-suspends - #6782

Merged
habdelra merged 10 commits into
mainfrom
cs-13864-shares-share-a-key-into-a-realm-suspend-on-loss-of-write
Oct 19, 2026
Merged

habdelra merged 10 commits into
mainfrom
cs-13864-shares-share-a-key-into-a-realm-suspend-on-loss-of-write

Conversation

@habdelra

@habdelra habdelra commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Note

Stacked on #6776, which requires a secure session to mint key write tickets. Sharing and swapping use those tickets. The base is that PR's branch; retarget to main once it merges.

Background and Goal

A key its owner wants other people in a realm to consume (a weather subscription, say) is shared into that realm. A share holds no secret, only a reference to the key, so one key can be shared into several realms while its value is stored once. This adds shares and every rule about them: who may share, unshare, suspend, resume and swap; that a share is suspended when its owner stops being a direct writer of the realm, resuming when they write again; that deleting a key suspends its shares instead of deleting them, while deleting a realm deletes its shares; that the realm's direct writers get a notice of each suspension; and who sees what. Nothing consumes shares yet.

Where to start

  • The migration (packages/postgres/migrations/1792187770930_add-key-shares.js) holds the tables, the state check constraint and the three triggers. Those are what keep a share's state right.
  • packages/realm-server/lib/key-shares.ts holds the rules for each action and listing. Its header comment is the whole permission table.
  • packages/realm-server/tests/key-shares-test.ts has one test per rule in the ticket, plus the read-only-role check.
  • The rest supports those: handlers/handle-key-shares.ts and routes.ts; share and swap ticket targets in lib/user-keys.ts; isDirectWriterLocked in runtime-common; the telemetry path list; and the key-shares and user-keys skills.

Key decisions and non-obvious mechanics

  • Suspension on loss of write is a trigger on realm_user_permissions, not application code. Many code paths write that table: the realm's _permissions PATCH, the admin upsert, create, publish, unpublish, delete, the registry backfill, and any database writer. A row-level trigger makes the rule hold for all of them, in the same transaction. After each change to a person's row, it suspends their active shares in that realm if the row no longer grants write, and resumes their owner-lost-write shares if it does. A share's owner is always a person, because only a direct writer can share or swap in, so their own row is the whole answer and the bot and wildcard rules of isDirectWriter don't need repeating in SQL.
  • Key deletion suspends through a BEFORE DELETE trigger on user_keys. It sets key_id to null with reason key-deleted, so the share keeps its place and label for a writer to swap a key into. GET /_key-shares?key=<id> lists the shares a delete would affect.
  • Notices are rows in key_share_notices, written by a trigger on key_shares whenever a share goes from active to suspended, or a suspended share's key is deleted, whatever made the change. Direct writers read them from GET /_key-share-notices?realm=. Showing them to people belongs to the realm config's API Keys entry and the secure window (follow-ups below).
  • Reasons don't stack. Only an active share is suspended, so one suspended for abuse stays suspended through its owner losing and regaining write. Deleting the key is the one exception: the share then holds no key, so it becomes key-deleted whatever it was suspended for. Lifting a writer's suspension while the owner can't write leaves the share as owner-lost-write, which then resumes on its own. key_shares_state_check pins each reason to its columns.
  • Races are closed with row locks, taken in the order the triggers take them: permission rows, then the key, then the share. A concurrent revoke or key delete therefore waits for a share, swap or suspend to commit, and its trigger then sees the result; taking the share first would deadlock against the trigger. The realm listings read with the writer's permission row locked too.
  • Tickets name the whole change. A share ticket names the key and the realm. A swap ticket names the key and the share, and is refused at minting for another user's key. A resume ticket names the share. key_write_tickets gains realm_url and share_id, with a check constraint widened to match. Lifting a writer's suspension puts a key back into use, so it takes a ticket: card code in a writer's app page can't undo another writer's abuse suspension. Unshare and suspend only stop a key being used, so they take none.
  • Deleting a realm deletes its shares (removeRealmDatabaseArtifacts), so a realm later made at the same URL that grants their owner write can't revive them.
  • An owner can unshare without write. Withdrawing your own key shouldn't depend on still writing the realm.
  • The schema snapshot only moves to the new timestamp. Every new table is excluded from the SQLite dump, and the triggers don't carry over to SQLite. The content is identical to the previous snapshot.

Follow-ups

  • Limits: the limits column is reserved, and the owner-only rule for changing it lands with the limits work: CS-13877.
  • Swap as an approval of a share's bound operations: CS-13867.
  • Surfacing shares and notices to people: CS-13873 (realm config) and CS-13866 (secure window sharing mode). Doing the same from the terminal: CS-13875.

🤖 Generated with Claude Code

…suspends

Adds `key_shares` and `key_share_notices`, with triggers on
`realm_user_permissions` and `user_keys` so a share is suspended when its
owner stops being a direct writer (resuming when they write again) and when
its key is deleted, however those rows change. Each suspension records a
notice for the realm's direct writers.

The `/_key-shares` routes let a key's owner share it into a realm they
directly write (with a `share` key write ticket), unshare it, and let a
realm's direct writers list its shares and notices, suspend and resume them,
and swap one of their own keys into a suspended share (with a `swap` ticket).

Adds the `key-shares` operator skill and updates the `user-keys` skill.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@habdelra habdelra self-assigned this Oct 16, 2026
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-16T22:14:42.496754Z 9ce6261 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9ce6261742

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/realm-server/lib/key-shares.ts Outdated
Comment on lines +423 to +424
await assertDirectWriter(dbAdapter, realmURL, user);
let rows = await query(dbAdapter, [

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Make realm-share authorization atomic with the read

If the user's direct-write permission is revoked after this standalone check succeeds but before the following SELECT, the request still returns the realm's share metadata to the former writer. listRealmShareNotices has the same check-then-read window. Fold the permission predicate into each query or perform the check and read in a transaction that locks the permission row so revocation cannot interleave.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Fixed in 3ad4d94a52. listRealmShares and listRealmShareNotices run the direct-writer check and the read in one transaction, with the writer's permission row locked FOR SHARE, so a revoke waits for the read to finish.

Comment thread packages/realm-server/lib/user-keys.ts Outdated
Comment on lines +129 to +133
if (action === 'swap') {
if (typeof shareId !== 'string') {
throw new UserKeyError(400, 'a ticket to swap a key names the share');
}
return { action, keyId, realmURL: null, shareId };

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject non-UUID swap targets before insertion

For action: 'swap', any string is accepted as shareId. A ticket request with a valid owned key but a value such as "not-a-uuid" therefore reaches mintKeyWriteTicket, where inserting it into the UUID share_id column raises PostgreSQL error 22P02; keysHandler converts that client input into a 500 response. Validate the UUID format here and return a 400/404 response instead.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Fixed in 3ad4d94a52. validateKeyWriteTarget refuses a shareId that isn't a UUID with 404 before anything reaches the uuid column, for both swap and the new resume ticket.

@github-actions

github-actions Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Preview deployments

Host Test Results

    1 files  ±0      1 suites  ±0   2h 9m 54s ⏱️ + 13m 4s
5 708 tests ±0  5 697 ✅ ±0  11 💤 ±0  0 ❌ ±0 
5 723 runs  ±0  5 712 ✅ ±0  11 💤 ±0  0 ❌ ±0 

Results for commit 54c52e6. ± Comparison against earlier commit 7e485b8.

Realm Server Test Results

    1 files  ±0    345 suites  ±0   2h 7m 8s ⏱️ - 4m 4s
5 832 tests ±0  5 832 ✅ ±0  0 💤 ±0  0 ❌ ±0 
5 895 runs  ±0  5 895 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 54c52e6. ± Comparison against earlier commit 7e485b8.

@habdelra habdelra left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] This review covered the share triggers and check constraints, the locking in lib/key-shares.ts, the authorization of each action, the widened key write tickets, whether the tests would fail if their rules broke, and how accurate the comments and skills are. I didn't review the secure-session base this PR stacks on. The realm-server suite wasn't run. I applied the migrations to a throwaway Postgres and checked the trigger, deadlock and down/up claims there.

No blocking issues. One lock-order deadlock is worth fixing in this PR, and there is one design decision to make: shares outlive realm deletion and resume in a realm recreated at the same URL.

  1. Deadlock between an owner suspending or unsharing their own share and the removal of their write. Lock the permission row before the share (see the lockVisibleShare thread in lib/key-shares.ts).
  2. Delete a realm's shares when the realm is deleted, or make them unable to resume (see the key_shares_follow_owner_write thread in the migration).
  3. Check UUIDs before consuming a ticket in swapKeyIntoShare and shareKey, and in validateKeyWriteTarget for shareId, so a malformed id doesn't answer 500 (see the swapKeyIntoShare thread).
  4. Deleting a key overwrites suspended-by-writer and records no notice. Either add the carve-out to the skill or record a notice (see the key_shares_on_key_delete thread).
  5. Add a test for spending a swap ticket on a different share, and a lock test if the harness allows one (see the thread in tests/key-shares-test.ts).
  6. Two comment fixes: listKeyShares should be listOwnShares, and the migration header's "a bot account owns no key".

A decision, not a defect: suspend and resume take no ticket, so card code running in any direct writer's app page can resume a share that another writer suspended for abuse, in every realm that writer writes. The description reasons that resuming "restores what the owner already consented to". That covers the owner. It doesn't cover the writer who made the suspension. If a writer's suspension should only be lifted from the secure origin, resume needs a ticket. Otherwise the key-shares skill should say plainly that any direct writer can undo it from anywhere.

CI was still pending when I reviewed, with no failures so far.

Comment thread packages/realm-server/lib/key-shares.ts Outdated
Comment on lines +191 to +203
let [row] = await txQuery([
`SELECT ${SHARE_COLUMNS} FROM key_shares WHERE id = `,
param(shareId),
` FOR UPDATE`,
]);
if (!row) {
throw noSuchShare();
}
let isWriter = await isDirectWriterLocked(
txQuery,
new URL(row.realm_url as string),
user,
);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] lockVisibleShare locks the share row and then the caller's permission row. The permission trigger takes those locks in the opposite order: the row first, then the shares. So when an owner suspends or unshares their own active share while their write is being removed, the two transactions deadlock.

The steps: T1 (suspendShare by the owner) runs SELECT … FROM key_shares … FOR UPDATE. T2 (UPDATE realm_user_permissions SET write = false for that owner) takes the row lock, and its key_shares_follow_owner_write UPDATE waits on the share. T1's isDirectWriterLocked then waits on T2. I ran this against a scratch Postgres with this branch's migrations. Postgres answered deadlock detected and aborted the permission UPDATE, so the _permissions PATCH that removes the write fails with a 500 and the owner keeps write. A swap by the owner into their own owner-lost-write share, racing a grant of write to them, takes the same cycle.

The fix is to take the permission row first, the same order the trigger and shareKey use. realm_url never changes on a share, so read it without a lock, call isDirectWriterLocked, and only then SELECT … FOR UPDATE the share. The owner-row lock that resumeShare takes after the share doesn't form a cycle: the trigger's UPDATEs only match active and owner-lost-write rows, never a suspended-by-writer one.

Regression. I'd fix it in this PR, because the description says these races are closed. It isn't blocking on its own, since both sides roll back cleanly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Fixed in 3ad4d94a52. lockVisibleShare reads the share's realm and owner without a lock, then locks the permission rows (the caller's, and the owner's too when resuming, in username order), then the swapped-in key, then the share FOR UPDATE. That's the order the permission trigger and the key-delete trigger take them in. If a swap changed the owner between the read and the lock, it takes the locks again. On a scratch database, a revoke against an owner's suspend now waits for the suspend to commit; the old order fails with deadlock detected.

Comment thread packages/realm-server/lib/key-shares.ts Outdated
Comment on lines +356 to +365
if (typeof keyId !== 'string') {
throw new UserKeyError(400, 'a swap names the key it puts in');
}
return await dbAdapter.withTransaction(async (txQuery) => {
await consumeKeyWriteTicket(txQuery, user, ticket, {
action: 'swap',
keyId,
realmURL: null,
shareId,
});

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Swap and share consume the ticket before checking that the ids are UUIDs, so an id that isn't one returns a 500 instead of a 404. consumeKeyWriteTicket compares share_id/key_id (uuid columns) to the raw string, and Postgres raises invalid input syntax for type uuid (checked on a scratch DB). That happens for POST /_key-shares/not-a-uuid/swap, for a swap or share body whose keyId isn't a UUID, and when minting a swap ticket with a non-UUID shareId (the INSERT fails the same way). replaceUserKeyValue and deleteUserKey call assertKeyId before consuming.

Fix: in swapKeyIntoShare, check UUID.test(shareId) (noSuchShare) and UUID.test(keyId) (noSuchKey) before consumeKeyWriteTicket. Do the same keyId check in shareKey. In validateKeyWriteTarget, answer a non-UUID shareId with a 400.

Regression, non-blocking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Fixed in 3ad4d94a52. Share, swap and resume check the key and share ids before spending the ticket, and an id that isn't a UUID answers 404 (no such key / no such share). Minting a swap or resume ticket with a malformed shareId also answers 404. ids that are not UUIDs name nothing covers each route and confirms the refusals leave the ticket unspent.

Comment on lines +108 to +122
IF EXISTS (
SELECT 1 FROM realm_user_permissions
WHERE realm_url = share_realm_url
AND username = share_owner
AND write
) THEN
UPDATE key_shares
SET state = 'active',
suspension_reason = NULL,
suspended_at = NULL,
updated_at = NOW()
WHERE realm_url = share_realm_url
AND owner = share_owner
AND state = 'suspended'
AND suspension_reason = 'owner-lost-write';

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Shares survive deleting their realm, and they come back on their own if a realm is later created at the same URL. Deleting a realm runs removeRealmPermissions, which suspends every share there as owner-lost-write, and nothing deletes them. If the realm's owner recreates it under the same slug, create-realm inserts their write row and this branch resumes their old shares into the new realm. Any other writer's share in the old realm resumes too, as soon as the new realm grants that person write, even though they never shared their key into the new realm. The old realm's notices also show up for the new realm's writers. I checked the delete-then-recreate sequence against a scratch DB: the share went back to active.

handle-delete-realm.ts already hard-deletes unlisted_realm_paths "so a realm later recreated at the same endpoint can't reuse" them, and the same reasoning applies here. My suggestion is DELETE FROM key_shares WHERE realm_url = … in removeRealmDatabaseArtifacts, inside the delete transaction (notices go with it through the cascade). If shares should outlive their realm, the alternative is a terminal reason that doesn't resume on a write row. Either way, the key-shares skill's "nothing but its owner's unshare deletes a share" changes with the decision.

Regression (it comes with the new table). Non-blocking while nothing consumes shares, but please decide it in this PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Fixed in 3ad4d94a52: deleting a realm deletes its shares. removeRealmDatabaseArtifacts deletes the realm's key_shares rows, and their notices cascade, so a realm later made at the same URL gets none of them back. deleting a realm deletes its shares covers it, and the key-shares skill says so.

Comment on lines +163 to +170
UPDATE key_shares
SET state = 'suspended',
suspension_reason = 'key-deleted',
suspended_by = NULL,
suspended_at = COALESCE(suspended_at, NOW()),
key_id = NULL,
updated_at = NOW()
WHERE key_id = OLD.id;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Deleting a key overwrites the reason on a share a writer already suspended, and the realm gets no notice of it. The UPDATE matches every share holding the key, so a suspended-by-writer share becomes key-deleted with suspended_by cleared. The notice trigger fires only on active → suspended, so it records nothing (checked on a scratch DB). That contradicts the key-shares skill's "a share suspended for one reason keeps that reason when another applies too". It also loses the record of who suspended the share for abuse.

key_shares_state_check needs key_id null for key-deleted, so the overwrite may be what you want. If so, either add a carve-out to the skill sentence and the migration header (a deleted key replaces any reason), or record a notice when a key is deleted out of an already-suspended share.

Regression (doc accuracy), non-blocking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Fixed in 3ad4d94a52. Deleting the key still turns any suspension into key-deleted, because the share then holds no key and only a swap can bring it back. The notice trigger now also fires when a suspended share's reason becomes key-deleted, so the writers learn the key is gone. The skill states this as the one exception to "reasons don't stack". Covered by deleting the key of a share a writer suspended records a notice.

Comment on lines +26 to +28
// Only a direct writer (see `isDirectWriter`) can share or swap a key in,
// and a wildcard or bot account owns no key, so a share's owner is always a
// person and their own row is the whole answer.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] The justification here doesn't hold as written: a bot account can own keys in user_keys. What makes a share's owner always a person is that isDirectWriter refuses wildcard and bot rows, so neither can share or swap in. Non-blocking wording fix:

Suggested change
// Only a direct writer (see `isDirectWriter`) can share or swap a key in,
// and a wildcard or bot account owns no key, so a share's owner is always a
// person and their own row is the whole answer.
// Only a direct writer (see `isDirectWriter`) can share or swap a key in,
// and a wildcard or bot account is never one, so a share's owner is always
// a person and their own row is the whole answer.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Fixed in 3ad4d94a52 with your rewording: a share's owner is always a person whose own row granted write when they shared or swapped in, because only a direct writer can do either, and isDirectWriter refuses wildcard rows and bots.

Comment thread packages/realm-server/lib/user-keys.ts Outdated
Comment on lines +429 to +430
// by a trigger on `user_keys`, and keeps its place in its realm for a direct
// writer to swap another key into; `listKeyShares` with the key's id lists

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] There's no listKeyShares; the function is listOwnShares (lib/key-shares.ts). Non-blocking:

Suggested change
// by a trigger on `user_keys`, and keeps its place in its realm for a direct
// writer to swap another key into; `listKeyShares` with the key's id lists
// writer to swap another key into; `listOwnShares` with the key's id lists
// the shares a delete would suspend.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Fixed in 3ad4d94a52: the comment now names listOwnShares.

Comment on lines +589 to +599
let writerTicket = await ticketFor(WRITER, {
action: 'swap',
keyId: await keyFor(WRITER),
shareId,
});
let withOwnTicket = await swap(WRITER, shareId, readerKey, writerTicket);
assert.strictEqual(
withOwnTicket.status,
403,
`a ticket for the writer's key doesn't swap in another`,
);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] No test covers a swap ticket's binding to its share. Every swap here spends a ticket minted for the share it targets, so deleting ...matches('share_id', shareId) from consumeKeyWriteTicket would leave the whole file green. That binding is the only thing keeping a swap ticket for share A from being spent on share B in the same realm. Please add the case: mint a swap for share A, spend it on another suspended share, expect 403 and the second share unchanged. The realm binding of share tickets already has its equivalent (otherRealmTicket).

The row-lock claims (isDirectWriterLocked, the key's FOR SHARE) are also untested. Removing either lock passes this file. A two-connection test is the only thing that would pin them: hold the share transaction open, revoke from a second connection, and assert the share ends up suspended. That test would also have caught the deadlock in the lockVisibleShare thread.

Regression in coverage, non-blocking. The lock test can be a follow-up if it's awkward in this harness.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Fixed in 3ad4d94a52. a non-writer can't swap, and nobody swaps in another user's key now spends a swap ticket minted for another suspended share and expects 403. The resume test does the same for a resume ticket. I checked the lock-order claim by hand on a scratch database, as described in the lockVisibleShare thread, rather than with a two-connection test: the realm-server harness has no way to hold one transaction open while another runs.

…kets

- Take locks in the order the triggers do (permission rows, then the key,
  then the share), so an owner suspending or unsharing while their write is
  removed no longer deadlocks the revoke.
- Delete a realm's shares, with their notices, when the realm is deleted,
  so a realm later made at the same URL gets none of them back.
- Answer a share or key id that isn't a UUID with 404 instead of a 500, at
  minting and at every route.
- Require a `resume` key write ticket to lift a writer's suspension, so card
  code in a writer's app page can't put a suspended key back into use.
- Record a notice when deleting a key changes why a suspended share is
  suspended, and say so in the skill.
- Read a realm's shares and notices with the writer's permission row locked.
- Test a swap or resume ticket spent on another share, the malformed ids,
  realm deletion, and the key-deleted notice; fix two inaccurate comments.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@habdelra

Copy link
Copy Markdown
Contributor Author

[Claude Code 🤖] I've addressed every finding from the review and the Codex comments in 3ad4d94a52, and replied in each thread. On the review body's design question: lifting a writer's suspension now needs a resume key write ticket, so only the secure window or the CLI can do it, not card code. The description is updated to match.

@habdelra
habdelra requested a review from a team October 16, 2026 22:28
@habdelra

Copy link
Copy Markdown
Contributor Author

[Claude Code 🤖] One change to the migration is still to come: the readonly_role revoke block in 1792187770930_add-key-shares.js. On Postgres 16, the role that created readonly_role is an ADMIN-only member of it. The loop that revokes SELECT from every direct member would then also revoke the table owner's own SELECT on key_shares and key_share_notices. A grant from another role would also survive. The fix counts a member only when its membership inherits or can SET the role, skips the table owner, and raises if any reader can still SELECT a column. It's being settled for the sealing migrations first, and this migration will take the same block, with tests.

…wner

The revoke loop skips a member that holds only the ADMIN option on
readonly_role (the role that creates it holds that membership on Postgres
16) and the table's owner, so it no longer revokes the owner's own SELECT.
After it, `up` stops if readonly_role, or any role reading through it, can
still read a column of either table, as the check for the sealed tables does.
Tests cover a PUBLIC grant, a grant through another role, an ADMIN-only
member and an owning member, and remove the memberships they make.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@habdelra

Copy link
Copy Markdown
Contributor Author

[Claude Code 🤖] The readonly_role change in 1792187770930_add-key-shares.js is in 988fec01c4. The revoke loop skips ADMIN-only members and the table owner, and up then stops if any role that reads through readonly_role can still read a column of either table. Five tests in key-shares-test.ts cover it.

…-in-on-its-own-a-credential-card-code' into cs-13864-shares-share-a-key-into-a-realm-suspend-on-loss-of-write

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A REVOKE removes only the grants its grantor made, so a SELECT another role
granted to readonly_role or one of its readers would survive the revoke loop
(and the read check would then stop the migration). The loop now finds every
SELECT grant held by readonly_role or a role reading through it, recursively,
and revokes it acting as its grantor with CASCADE, so grants passed on from it
go too, as the sealing migrations' loop does. A test passes a grant-option
grant from an outside role to a member and on to readonly_role.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@habdelra

Copy link
Copy Markdown
Contributor Author

[Claude Code 🤖] 6f4f16eb95 makes the readonly_role revoke loop in 1792187770930_add-key-shares.js work the way the sealing migrations' loop does. It finds every SELECT grant that readonly_role holds, directly or through a role that reads through it, and revokes each one as the role that granted it, with CASCADE. The read check after the loop is unchanged.

@FadhlanR FadhlanR left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] This review went after the share triggers and their interaction with the row locks in lib/key-shares.ts, ticket binding, realm deletion, and the skill text. I didn't re-review the secure-session base this PR stacks on, or the readonly_role revoke block beyond reading it, and the realm-server suite wasn't run.

No blocking issues. One race is still open: a writer lifting a suspension at the moment the share's owner is re-added to the realm can leave the share stuck as owner-lost-write while its owner writes.

  1. Lock the person's shares at the top of key_shares_follow_owner_write, so a re-grant that inserts the owner's permission row waits for an in-flight resume (see the thread on that function in the migration).
  2. Optional cleanup: lib/key-shares.ts and lib/user-keys.ts each define the same UUID regex. Exporting one from user-keys.ts keeps the two 404 rules from drifting.

The failing Percy check reports a host visual change. This diff touches no host template, so it comes from the base branch or the snapshot baseline, not from this PR.

Comment on lines +128 to +134
BEGIN
IF EXISTS (
SELECT 1 FROM realm_user_permissions
WHERE realm_url = share_realm_url
AND username = share_owner
AND write
) THEN

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] A resume can still leave a share stuck as owner-lost-write while its owner writes. The case is an owner whose permission row was deleted (removed from the realm) and is re-added while a writer lifts their suspension. resumeShare locks the owner's row FOR SHARE, but there's no row to lock, so it reads ownerIsWriter = false and sets the reason to owner-lost-write. The re-grant's INSERT doesn't wait on anything. Its trigger's resume UPDATE evaluates the share as still suspended-by-writer (the resume hasn't committed) and skips it without blocking. After both commit, the owner writes and the share stays suspended until their row changes again. That breaks the resumeShare comment's "resumes when they are one again".

I reproduced it on a scratch database with these triggers: a transaction doing the resume's statements and holding for 3s, plus a concurrent INSERT of the owner's write row. The INSERT committed straight away, and the share ended as suspended / owner-lost-write with the owner's row granting write.

Fix: lock the person's shares in the realm before deciding. Once that waits on the resume's share lock, the UPDATE that follows (a new statement, so a new snapshot) sees the committed owner-lost-write and resumes it. With this change the same race ended active, and it keeps the lock order the description sets out (permission row, then shares):

BEGIN
  PERFORM 1 FROM key_shares
    WHERE realm_url = share_realm_url AND owner = share_owner
    FOR UPDATE;
  IF EXISTS (

Non-blocking regression: the window is narrow, and a swap or another change to the owner's row recovers the share. But it's the one place where the PR's "races are closed with row locks" claim doesn't hold.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Fixed in b94a5778b5. key_shares_follow_owner_write now locks the person's shares in the realm with FOR UPDATE before it checks their permission row, as you suggested. That keeps the lock order the same: permission row, then shares. The comment above lockDirectWriters in lib/key-shares.ts now says the trigger waits this way and why.

I ran your race on a scratch Postgres 16 with the triggers taken from the migration. A transaction runs the resume's locks and its owner-lost-write UPDATE, then holds for 3s while the owner's write row is INSERTed concurrently. With the previous commit's function the share ended suspended|owner-lost-write. With this commit's function the INSERT waited on the share lock and the share ended active. key-shares-test still passes locally (22/22).

habdelra and others added 5 commits October 19, 2026 00:02
A resume that finds no permission row for the share's owner has nothing to
lock there, so a concurrent grant of the owner's write could skip the share
as still writer-suspended and leave it as owner-lost-write while the owner
writes. The trigger now locks the person's shares in the realm first, so it
waits on the resume and its UPDATE sees the committed reason.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…loop

Grants a table's owner made go first, so CASCADE takes what a reader passed
on from one without acting as that reader. Column grants go before table
grants, because CASCADE on a table grant leaves the column grants passed on
from it, and once their grantor has lost the table grant nobody can revoke
them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-in-on-its-own-a-credential-card-code' into cs-13864-shares-share-a-key-into-a-realm-suspend-on-loss-of-write
…-in-on-its-own-a-credential-card-code' into cs-13864-shares-share-a-key-into-a-realm-suspend-on-loss-of-write
…-a-key-into-a-realm-suspend-on-loss-of-write
@habdelra
habdelra changed the base branch from cs-13996-the-secure-window-signs-in-on-its-own-a-credential-card-code to main October 19, 2026 14:55
@habdelra
habdelra merged commit 8ee13e3 into main Oct 19, 2026
92 of 109 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants