kayg/calternal
Watch
1
0
Fork
You've already forked calternal
 0
Code Issues 190 Pull requests 61 Projects Releases Packages Wiki Activity Actions 26

WIP: feat: sharing slice 1 PR 2: shared data and recovery #829

Closed
kayg wants to merge 1 commit from feat/sharing-s1-pr2 into feat/sharing-s1-pr1
pull from: feat/sharing-s1-pr2
merge into: kayg:feat/sharing-s1-pr1
Conversation 14 Commits 1 Files changed 1 +59
kayg commented 2026-11-01 13:51:31 +00:00
Owner
Copy link

Part 2 of the sharing slice, built on approved PR #827 (08876c76) and targeting feat/sharing-s1-pr1.

This draft retains the AuthorizedResource resolver, server admission fences and durable per-Installation controls, including per-note sequence fences and re-grant baselines. It stays dormant: cross-owner access is refused unconditionally and sharing actions are not registered, so a grant cannot be created through the API.

Live room delivery moved to stacked PR #835. That isolates the send-time sequence/fence check, recipient epoch, and revoke eviction while this PR remains safe without UI registration.

Changed lines: 3,463 (2,956 additions, 507 deletions).

Gates on dev-01

  • PASS: cargo fmt --all -- --check
  • PASS: cargo clippy --workspace --all-targets -j4 -- -D warnings
  • PASS: cargo test --workspace -j4 (89 successful test-result groups; perf-only tests remain ignored)
Part 2 of the sharing slice, built on approved PR #827 (`08876c76`) and targeting `feat/sharing-s1-pr1`. This draft retains the `AuthorizedResource` resolver, server admission fences and durable per-Installation controls, including per-note sequence fences and re-grant baselines. It stays dormant: cross-owner access is refused unconditionally and sharing actions are not registered, so a grant cannot be created through the API. Live room delivery moved to stacked PR #835. That isolates the send-time sequence/fence check, recipient epoch, and revoke eviction while this PR remains safe without UI registration. Changed lines: **3,463** (2,956 additions, 507 deletions). ## Gates on dev-01 - PASS: `cargo fmt --all -- --check` - PASS: `cargo clippy --workspace --all-targets -j4 -- -D warnings` - PASS: `cargo test --workspace -j4` (89 successful test-result groups; perf-only tests remain ignored)
docs(sharing): outline PR 2 data path
Some checks failed
CI / server (pull_request) Has been cancelled
Details
CI / rust (pull_request) Has been cancelled
Details
CI / web (pull_request) Has been cancelled
Details
c43dabe5f3
kayg commented 2026-11-01 15:24:27 +00:00
Author
Owner
Copy link

Independent review, round 1

Reviewed head: f00c26249872f44d4846049b978acb56e55610ce.

Findings

  • P0: none.

  • P1: Purge does not fence already-open room deliveries or a prepared response. Notes::join checks access before and after joining, then returns a room stream with no later access check. sharing.revoke commits the purge control but does not detach or filter existing room sessions. Failure scenario: a recipient joins owner/note, the owner revokes the grant, then the owner edits; the recipient's existing room still receives the update even though a new notes.get is denied. I reproduced this with the temporary dev-01 probe probe_shared_room_still_delivers_after_revoke. get_authorized also performs its final resolver check before returning the response, so a prepared read can finish after purge commits. Recheck at delivery and response completion, and fence active room sessions.

  • P1: This split does not keep sharing unavailable. The sharing plugin is registered unconditionally, the action contract exposes grant/revoke/control actions, and the new isolation test asserts that an owner can grant. notes.get accepts owner_id, and room joins accept owner/id; the probe reads the shared text through notes.get. API clients can therefore share and read before the follow-up sync, fan-out, device recovery, search, history and lifecycle work is complete. No UI does not block these server actions. Add a server-side gate for share creation and cross-owner reads until the follow-ups are complete, or keep the core unlanded.

  • P2: Durable control and fan-out storage is not bounded over time. sharing.controls limits one response to 256 rows, but there is no control/ack cleanup path. If an Installation never returns, its controls remain pending indefinitely; even after all current Installations acknowledge, the rows remain. Also, each edit on a note with a note_shares row inserts a share_fanout intent, and this branch has no worker to drain them. The response limit does not bound either table. Define retirement/compaction for absent Installations and land the fan-out worker before enabling sharing.

  • P3: Controls are not returned in per-note sequence order. The pending query sorts by created_at,id, while IDs are random and timestamps have millisecond resolution. A quick re-grant/revoke can return a baseline and purge out of sequence; applying that list order can let the older purge clear the newer baseline. The 2C client path should apply controls monotonically by note_seq and cover equal-timestamp ordering.

Split and verdict

The 2B → 2C → UI split is sensible by dependency. The core is not safe to land on its own while sharing is meant to remain unavailable: its API is already callable, and live room delivery survives revoke. Verdict: request changes before merge; gate the feature and enforce the committed fence on every delivery path before enabling it.

Probe results

  • The room probe passed while reproducing post-revoke delivery.
  • The auth restart probe confirmed the pending purge and fence survive reopening auth.db.
  • The 10 existing sharing tests passed, including randomized admission/grant/revoke interleavings and per-Installation acknowledgements.
  • The action availability test passed.
## Independent review, round 1 Reviewed head: `f00c26249872f44d4846049b978acb56e55610ce`. ### Findings - **P0: none.** - **P1: Purge does not fence already-open room deliveries or a prepared response.** `Notes::join` checks access before and after joining, then returns a room stream with no later access check. `sharing.revoke` commits the purge control but does not detach or filter existing room sessions. Failure scenario: a recipient joins `owner/note`, the owner revokes the grant, then the owner edits; the recipient's existing room still receives the update even though a new `notes.get` is denied. I reproduced this with the temporary dev-01 probe `probe_shared_room_still_delivers_after_revoke`. `get_authorized` also performs its final resolver check before returning the response, so a prepared read can finish after purge commits. Recheck at delivery and response completion, and fence active room sessions. - **P1: This split does not keep sharing unavailable.** The sharing plugin is registered unconditionally, the action contract exposes grant/revoke/control actions, and the new isolation test asserts that an owner can grant. `notes.get` accepts `owner_id`, and room joins accept `owner/id`; the probe reads the shared text through `notes.get`. API clients can therefore share and read before the follow-up sync, fan-out, device recovery, search, history and lifecycle work is complete. No UI does not block these server actions. Add a server-side gate for share creation and cross-owner reads until the follow-ups are complete, or keep the core unlanded. - **P2: Durable control and fan-out storage is not bounded over time.** `sharing.controls` limits one response to 256 rows, but there is no control/ack cleanup path. If an Installation never returns, its controls remain pending indefinitely; even after all current Installations acknowledge, the rows remain. Also, each edit on a note with a `note_shares` row inserts a `share_fanout` intent, and this branch has no worker to drain them. The response limit does not bound either table. Define retirement/compaction for absent Installations and land the fan-out worker before enabling sharing. - **P3: Controls are not returned in per-note sequence order.** The pending query sorts by `created_at,id`, while IDs are random and timestamps have millisecond resolution. A quick re-grant/revoke can return a baseline and purge out of sequence; applying that list order can let the older purge clear the newer baseline. The 2C client path should apply controls monotonically by `note_seq` and cover equal-timestamp ordering. ### Split and verdict The 2B → 2C → UI split is sensible by dependency. The core is **not safe to land on its own while sharing is meant to remain unavailable**: its API is already callable, and live room delivery survives revoke. Verdict: request changes before merge; gate the feature and enforce the committed fence on every delivery path before enabling it. ### Probe results - The room probe passed while reproducing post-revoke delivery. - The auth restart probe confirmed the pending purge and fence survive reopening `auth.db`. - The 10 existing sharing tests passed, including randomized admission/grant/revoke interleavings and per-Installation acknowledgements. - The action availability test passed.
kayg commented 2026-11-01 17:18:13 +00:00
Author
Owner
Copy link

Round-one review status at head 2bb35086 (code gates ran at e4f6a934; 2bb35086 records the gate results).

  • P1a — finding: an existing room subscription or prepared read could return content after purge. Fix: each room message checks the sequence against the committed fence and grant interval; revoke commits the fence and evicts the recipient subscription before returning; notes.get rechecks access immediately before returning. Tests: revoked_shared_room_is_evicted_and_receives_no_later_updates and prepared_shared_read_rechecks_its_sequence_before_returning pass.
  • P1b — finding: sharing actions and cross-owner reads were reachable before the remaining sharing work. Fix: the action code stays unregistered and sharing actions are absent from contracts/actions.json; the generic route inventory is unchanged. Tests: sharing_actions_are_not_registered_before_the_ui_pr and the route inventory isolation tests pass.
  • P2/P3 — finding: control order and retained work needed deterministic ordering and cleanup. Fix: controls sort by per-note note_seq; cleanup retains controls and fan-out intents for known Installations, retires work when an Installation has been inactive for 30 days, and requires a current authorized reset on return. Tests: pending_controls_use_note_sequence_when_timestamps_match and cleanup_keeps_known_work_and_resets_dormant_installations pass.

Dev-01 gates: cargo fmt --all -- --check, cargo clippy --workspace --all-targets -j4 -- -D warnings, the targeted migration regression, cargo test -j4 -p calternal-auth --test migration -- --nocapture, and cargo test --workspace -j4 pass. Workspace result: 91 groups, 1,278 passed, 0 failed, 20 ignored. Focused auth, prepared-read and isolation gates pass.

PR #829 remains at the server core (3,229 changed lines against PR #827). Direct bytes, attachments, sync fan-out, offline recovery, search, history and lifecycle remain in the sequenced follow-ups. The owner-to-recipient app-password API flow stays unavailable until PR 3 registers sharing actions.

Round-one review status at head `2bb35086` (code gates ran at `e4f6a934`; `2bb35086` records the gate results). - **P1a — finding:** an existing room subscription or prepared read could return content after purge. **Fix:** each room message checks the sequence against the committed fence and grant interval; revoke commits the fence and evicts the recipient subscription before returning; `notes.get` rechecks access immediately before returning. **Tests:** `revoked_shared_room_is_evicted_and_receives_no_later_updates` and `prepared_shared_read_rechecks_its_sequence_before_returning` pass. - **P1b — finding:** sharing actions and cross-owner reads were reachable before the remaining sharing work. **Fix:** the action code stays unregistered and sharing actions are absent from `contracts/actions.json`; the generic route inventory is unchanged. **Tests:** `sharing_actions_are_not_registered_before_the_ui_pr` and the route inventory isolation tests pass. - **P2/P3 — finding:** control order and retained work needed deterministic ordering and cleanup. **Fix:** controls sort by per-note `note_seq`; cleanup retains controls and fan-out intents for known Installations, retires work when an Installation has been inactive for 30 days, and requires a current authorized reset on return. **Tests:** `pending_controls_use_note_sequence_when_timestamps_match` and `cleanup_keeps_known_work_and_resets_dormant_installations` pass. **Dev-01 gates:** `cargo fmt --all -- --check`, `cargo clippy --workspace --all-targets -j4 -- -D warnings`, the targeted migration regression, `cargo test -j4 -p calternal-auth --test migration -- --nocapture`, and `cargo test --workspace -j4` pass. Workspace result: 91 groups, 1,278 passed, 0 failed, 20 ignored. Focused auth, prepared-read and isolation gates pass. PR #829 remains at the server core (3,229 changed lines against PR #827). Direct bytes, attachments, sync fan-out, offline recovery, search, history and lifecycle remain in the sequenced follow-ups. The owner-to-recipient app-password API flow stays unavailable until PR 3 registers sharing actions.
kayg commented 2026-11-01 17:52:48 +00:00
Author
Owner
Copy link

Independent review, round 2

Reviewed head: 2bb3508623.

Area Status Evidence
Round-one revoke and prepared-read checks Partial The sequential room-eviction and prepared-read regressions pass. An old resource sequence is refused when its final check runs after purge and re-grant. A frame already dequeued by the server room pump can still be forwarded after revoke.
Edit-to-Read and re-grant room behavior Pass An open room keeps delivering readable edits after downgrade, write admission is refused, and a fresh join after re-grant receives the new baseline while the old room stays closed.
Unregistered surfaces Partial Sharing grant/list/revoke/control actions are absent, and the route/action inventory test passes. No grant-creation path was found in sync or DAV. The generic notes.get action still reads another owner's note when an active grant already exists; the inventory does not guard that owner-specific behavior.
30-day cleanup Fail An Installation seen 29 days ago keeps its pending control. After a dormant reset is prepared, a revoke before reset acknowledgement causes cleanup to retire the new purge; acknowledging the older reset then clears the marker.
Size and scope Acceptable The delta is 2,706 additions and 523 deletions across 32 files. Most growth is sharing authorization, control storage, room-delivery plumbing, tests and the plan. I found no unrelated feature. Direct bytes, attachments, sync fan-out, device recovery, search, history, lifecycle and UI remain follow-ups.

Findings

  • P0: none.
  • P1: A dequeued room frame can cross the committed revoke fence. notes/src/room/sync.rs:295-311 releases its permit after queueing to the room receiver. The server pump can then dequeue the bytes, block in events.send, and later pass the current-generation RoomEvent::Data to the websocket without another fence check (server/src/rooms.rs:187-193; server/src/live.rs:211-212). The in-flight probe held a dequeued frame across revoke and forwarded it afterward. Eviction only drops bytes still in the room queue. Carry cancellation or a fence check through the final server send, and add a deterministic regression.
  • P1: Cross-owner reads are not fully disabled by unregistering sharing actions. notes.get accepts owner_id, and the generic action route returns the note for an existing grant. The probe got an error without a grant and note text after a direct grant. If grant rows can remain from the stacked base, recipients can still read them on this head. Keep this route owner-only until PR 3, or add a direct server-side gate. The current inventory test checks route/action classification and the fixed sharing-action names; it does not check cross-owner reads.
  • P2: A reset is not bound to the control snapshot it acknowledges. prepare_full_reset returns current authorized resources, but acknowledge_full_reset acknowledges every control present later and removes the reset marker. While a reset marker is pending, cleanup can also retire a newly created purge as though the older snapshot covered it. The probe left no purge pending after reset acknowledgement. Bind acknowledgement to a reset generation or cursor and retain newer controls or require another reset.
  • P3: No additional scope finding. The room queue and save-path changes propagate per-delivery authorization checks; the other deferred work is already named in the plan.

Verification

The focused gates ran on dev-01 through ~/bin/slot nix develop -c bash -c:

  • cargo test -j4 -p calternal-auth --lib sharing::tests -- --nocapture — 13 passed.
  • cargo test -j4 -p calternal --bin calternal isolation::sharing_actions_are_not_registered_before_the_ui_pr -- --nocapture — passed.
  • cargo test -j4 -p calternal --bin calternal isolation::every_registered_action_and_direct_route_has_a_capability_classification -- --nocapture — passed.
  • cargo test -j4 -p calternal --bin calternal isolation::revoked_shared_room_is_evicted_and_receives_no_later_updates -- --nocapture — passed.
  • cargo test -j4 -p calternal-notes --lib prepared_shared_read_rechecks_its_sequence_before_returning -- --nocapture — passed.
  • cargo test -j4 -p calternal-core room_queue::tests::abort_discards_messages_waiting_for_a_room_client -- --nocapture — passed.
  • Temporary probes reproduced the in-flight room and reset-control gaps. Downgrade, fresh re-grant join, post-re-grant old-sequence refusal, and conditional notes.get(owner_id) authorization behaved as described. Probe code was removed; the PR worktree is clean.

Verdict: request changes.

## Independent review, round 2 Reviewed head: 2bb3508623c0bf5c1fe8ee2016567764322ce01c. | Area | Status | Evidence | |---|---|---| | Round-one revoke and prepared-read checks | Partial | The sequential room-eviction and prepared-read regressions pass. An old resource sequence is refused when its final check runs after purge and re-grant. A frame already dequeued by the server room pump can still be forwarded after revoke. | | Edit-to-Read and re-grant room behavior | Pass | An open room keeps delivering readable edits after downgrade, write admission is refused, and a fresh join after re-grant receives the new baseline while the old room stays closed. | | Unregistered surfaces | Partial | Sharing grant/list/revoke/control actions are absent, and the route/action inventory test passes. No grant-creation path was found in sync or DAV. The generic notes.get action still reads another owner's note when an active grant already exists; the inventory does not guard that owner-specific behavior. | | 30-day cleanup | Fail | An Installation seen 29 days ago keeps its pending control. After a dormant reset is prepared, a revoke before reset acknowledgement causes cleanup to retire the new purge; acknowledging the older reset then clears the marker. | | Size and scope | Acceptable | The delta is 2,706 additions and 523 deletions across 32 files. Most growth is sharing authorization, control storage, room-delivery plumbing, tests and the plan. I found no unrelated feature. Direct bytes, attachments, sync fan-out, device recovery, search, history, lifecycle and UI remain follow-ups. | ### Findings - **P0: none.** - **P1: A dequeued room frame can cross the committed revoke fence.** notes/src/room/sync.rs:295-311 releases its permit after queueing to the room receiver. The server pump can then dequeue the bytes, block in events.send, and later pass the current-generation RoomEvent::Data to the websocket without another fence check (server/src/rooms.rs:187-193; server/src/live.rs:211-212). The in-flight probe held a dequeued frame across revoke and forwarded it afterward. Eviction only drops bytes still in the room queue. Carry cancellation or a fence check through the final server send, and add a deterministic regression. - **P1: Cross-owner reads are not fully disabled by unregistering sharing actions.** notes.get accepts owner_id, and the generic action route returns the note for an existing grant. The probe got an error without a grant and note text after a direct grant. If grant rows can remain from the stacked base, recipients can still read them on this head. Keep this route owner-only until PR 3, or add a direct server-side gate. The current inventory test checks route/action classification and the fixed sharing-action names; it does not check cross-owner reads. - **P2: A reset is not bound to the control snapshot it acknowledges.** prepare_full_reset returns current authorized resources, but acknowledge_full_reset acknowledges every control present later and removes the reset marker. While a reset marker is pending, cleanup can also retire a newly created purge as though the older snapshot covered it. The probe left no purge pending after reset acknowledgement. Bind acknowledgement to a reset generation or cursor and retain newer controls or require another reset. - **P3: No additional scope finding.** The room queue and save-path changes propagate per-delivery authorization checks; the other deferred work is already named in the plan. ### Verification The focused gates ran on dev-01 through ~/bin/slot nix develop -c bash -c: - <code>cargo test -j4 -p calternal-auth --lib sharing::tests -- --nocapture</code> — 13 passed. - <code>cargo test -j4 -p calternal --bin calternal isolation::sharing_actions_are_not_registered_before_the_ui_pr -- --nocapture</code> — passed. - <code>cargo test -j4 -p calternal --bin calternal isolation::every_registered_action_and_direct_route_has_a_capability_classification -- --nocapture</code> — passed. - <code>cargo test -j4 -p calternal --bin calternal isolation::revoked_shared_room_is_evicted_and_receives_no_later_updates -- --nocapture</code> — passed. - <code>cargo test -j4 -p calternal-notes --lib prepared_shared_read_rechecks_its_sequence_before_returning -- --nocapture</code> — passed. - <code>cargo test -j4 -p calternal-core room_queue::tests::abort_discards_messages_waiting_for_a_room_client -- --nocapture</code> — passed. - Temporary probes reproduced the in-flight room and reset-control gaps. Downgrade, fresh re-grant join, post-re-grant old-sequence refusal, and conditional notes.get(owner_id) authorization behaved as described. Probe code was removed; the PR worktree is clean. **Verdict: request changes.**
kayg commented 2026-11-01 19:25:26 +00:00
Author
Owner
Copy link

Round 3 status at 7857ace57184b51eacb92691531a85315215a39c.

  • P1a Finding: a room frame dequeued before revoke could still be written after the fence committed. Fix: each frame carries its note sequence and recipient epoch; the socket writer rechecks the current grant interval, committed server fence, and epoch in one SQL snapshot immediately before writing, while holding the per-note permit through the write. A refused frame is dropped and closes the socket; revoke awaits room eviction. Test: dequeued_room_frame_is_checked_after_revoke_under_random_schedules covers 16 seeded yield schedules, and dequeued_room_frame_rechecks_committed_fence_and_grant_epoch checks the real auth database across purge and re-grant.
  • P1b Finding: notes.get(owner_id) could read through a stored grant. Fix: cross-owner resolution remains disabled by the compile-time CROSS_OWNER_SHARING_ENABLED constant, and sharing actions remain unregistered for PR 2. Test: a_direct_grant_row_does_not_enable_cross_owner_read_paths inserts a grant row, verifies Bob's valid Notes Read app password can read his own note, then verifies Alice's note is refused through notes.get, direct bytes, the resolver, room join, and sync.
  • P2 Finding: an older prepared reset could acknowledge a newer purge. Fix: reset preparation records per-note sequence cursors, and cleanup/ack only covers controls and fan-out at or below those cursors. Test: reset_ack_keeps_controls_and_fanout_after_its_note_cursor.

Dev-01 gates passed under Nix/~/bin/slot: cargo fmt --all --check, cargo clippy --workspace --all-targets -- -D warnings, and cargo test -j4 --workspace (1,280 passed, 0 failed, 20 ignored).

Round 3 status at `7857ace57184b51eacb92691531a85315215a39c`. - **P1a** Finding: a room frame dequeued before revoke could still be written after the fence committed. Fix: each frame carries its note sequence and recipient epoch; the socket writer rechecks the current grant interval, committed server fence, and epoch in one SQL snapshot immediately before writing, while holding the per-note permit through the write. A refused frame is dropped and closes the socket; revoke awaits room eviction. Test: `dequeued_room_frame_is_checked_after_revoke_under_random_schedules` covers 16 seeded yield schedules, and `dequeued_room_frame_rechecks_committed_fence_and_grant_epoch` checks the real auth database across purge and re-grant. - **P1b** Finding: `notes.get(owner_id)` could read through a stored grant. Fix: cross-owner resolution remains disabled by the compile-time `CROSS_OWNER_SHARING_ENABLED` constant, and sharing actions remain unregistered for PR 2. Test: `a_direct_grant_row_does_not_enable_cross_owner_read_paths` inserts a grant row, verifies Bob's valid Notes Read app password can read his own note, then verifies Alice's note is refused through `notes.get`, direct bytes, the resolver, room join, and sync. - **P2** Finding: an older prepared reset could acknowledge a newer purge. Fix: reset preparation records per-note sequence cursors, and cleanup/ack only covers controls and fan-out at or below those cursors. Test: `reset_ack_keeps_controls_and_fanout_after_its_note_cursor`. Dev-01 gates passed under Nix/`~/bin/slot`: `cargo fmt --all --check`, `cargo clippy --workspace --all-targets -- -D warnings`, and `cargo test -j4 --workspace` (1,280 passed, 0 failed, 20 ignored).
kayg commented 2026-11-01 20:12:16 +00:00
Author
Owner
Copy link

Independent review, round 3 (final)

Reviewed head: 7857ace57184b51eacb92691531a85315215a39c.

Area Status Evidence
Round-2 P1a, dequeued room frame Closed A frame captured before revoke was rejected after the committed fence. The real auth permit held the per-note lock through a blocked send; the server recheck refused the stale frame.
Round-2 P1b, direct grant read Closed With a grant row inserted directly into note_shares/share_grants, notes.get, direct note bytes, resolver, room join and sync returned no owner data. List, search, backlinks and history also returned no owner ID or marker. A Notes Read scope was refused by the DAV gate.
Round-2 P2, reset/purge order Closed A reset cursor was captured, then a later purge was created. Cleanup retained it, and acknowledging the older reset left the purge pending.
Dormant state Pass CROSS_OWNER_SHARING_ENABLED is false, sharing actions remain unregistered, and the existing unregistered-actions test passes.
Size Over plan The current PR is 4,276 changed lines across 38 files, versus the roughly 2,500-line plan. The description still reports 1,241 lines.

Findings

  • P0: none.
  • P1: none.
  • P2: The room delivery permit uses writer_lock(owner, note_id), shared by every recipient, and holds it through socket.send (up to the existing 15-second write timeout). A slow peer can serialize same-note deliveries and delay revoke, which uses the same lock. This path is unreachable while cross-owner access stays disabled; resolve it before PR 3 removes the gate.
  • P3: The committed direct-grant regression test does not cover list, search, backlinks, history or DAV. Those passed in review-only probes, but the coverage should be committed so the matrix remains a gate. Update the stale PR description with the current size and scope.

Split recommendation

Yes, split into two stacked PRs. Keep #829 focused on the resolver and hard-off gate, action registration remaining off, per-note admission fences, durable controls and reset ordering, plus the committed direct-grant matrix. Put the room frame sequence/epoch plumbing and final websocket send check in a new PR stacked on #829. Keep the gate false until that room PR is merged; resolve the P2 before enabling sharing in PR 3.

Verdict

The reviewed head is safe to land only as a dormant slice. I recommend the split before merge because the PR is 1.7 times the planned size and combines auth/control storage with room transport. No P0/P1 remains.

Verification

On dev-01 through ~/bin/slot nix develop -c bash -c, these focused probes passed. Probe bodies were temporary and restored after the run:

  • cargo test -j4 -p calternal-auth --lib review_probe_dequeued_frame_is_refused_after_db_revoke -- --nocapture
  • cargo test -j4 -p calternal-auth --lib review_probe_reset_cursor_preserves_a_later_purge -- --nocapture
  • cargo test -j4 -p calternal-server --lib review_probe_dequeued_frame_holds_fence_through_socket_write -- --nocapture
  • cargo test -j4 -p calternal --bin calternal isolation::a_direct_grant_row_does_not_enable_cross_owner_read_paths -- --nocapture
  • cargo test -j4 -p calternal --bin calternal isolation::sharing_actions_are_not_registered_before_the_ui_pr -- --nocapture
## Independent review, round 3 (final) Reviewed head: `7857ace57184b51eacb92691531a85315215a39c`. | Area | Status | Evidence | |---|---|---| | Round-2 P1a, dequeued room frame | Closed | A frame captured before revoke was rejected after the committed fence. The real auth permit held the per-note lock through a blocked send; the server recheck refused the stale frame. | | Round-2 P1b, direct grant read | Closed | With a grant row inserted directly into `note_shares`/`share_grants`, `notes.get`, direct note bytes, resolver, room join and sync returned no owner data. List, search, backlinks and history also returned no owner ID or marker. A Notes Read scope was refused by the DAV gate. | | Round-2 P2, reset/purge order | Closed | A reset cursor was captured, then a later purge was created. Cleanup retained it, and acknowledging the older reset left the purge pending. | | Dormant state | Pass | `CROSS_OWNER_SHARING_ENABLED` is false, sharing actions remain unregistered, and the existing unregistered-actions test passes. | | Size | Over plan | The current PR is 4,276 changed lines across 38 files, versus the roughly 2,500-line plan. The description still reports 1,241 lines. | ### Findings - **P0:** none. - **P1:** none. - **P2:** The room delivery permit uses `writer_lock(owner, note_id)`, shared by every recipient, and holds it through `socket.send` (up to the existing 15-second write timeout). A slow peer can serialize same-note deliveries and delay revoke, which uses the same lock. This path is unreachable while cross-owner access stays disabled; resolve it before PR 3 removes the gate. - **P3:** The committed direct-grant regression test does not cover list, search, backlinks, history or DAV. Those passed in review-only probes, but the coverage should be committed so the matrix remains a gate. Update the stale PR description with the current size and scope. ### Split recommendation **Yes, split into two stacked PRs.** Keep #829 focused on the resolver and hard-off gate, action registration remaining off, per-note admission fences, durable controls and reset ordering, plus the committed direct-grant matrix. Put the room frame sequence/epoch plumbing and final websocket send check in a new PR stacked on #829. Keep the gate false until that room PR is merged; resolve the P2 before enabling sharing in PR 3. ### Verdict The reviewed head is safe to land only as a dormant slice. I recommend the split before merge because the PR is 1.7 times the planned size and combines auth/control storage with room transport. No P0/P1 remains. ### Verification On dev-01 through `~/bin/slot nix develop -c bash -c`, these focused probes passed. Probe bodies were temporary and restored after the run: - `cargo test -j4 -p calternal-auth --lib review_probe_dequeued_frame_is_refused_after_db_revoke -- --nocapture` - `cargo test -j4 -p calternal-auth --lib review_probe_reset_cursor_preserves_a_later_purge -- --nocapture` - `cargo test -j4 -p calternal-server --lib review_probe_dequeued_frame_holds_fence_through_socket_write -- --nocapture` - `cargo test -j4 -p calternal --bin calternal isolation::a_direct_grant_row_does_not_enable_cross_owner_read_paths -- --nocapture` - `cargo test -j4 -p calternal --bin calternal isolation::sharing_actions_are_not_registered_before_the_ui_pr -- --nocapture`
kayg commented 2026-11-01 21:31:03 +00:00
Author
Owner
Copy link

Split complete: live room send-time checks, recipient epochs, revoke eviction, and their regressions moved to stacked #835 so the socket-delivery boundary can be gated separately. This PR retains the resolver, admission fences, and durable controls; cross-owner reads remain refused and sharing actions remain unregistered. dev-01 fmt, Clippy with warnings denied, and workspace tests pass.

Split complete: live room send-time checks, recipient epochs, revoke eviction, and their regressions moved to stacked #835 so the socket-delivery boundary can be gated separately. This PR retains the resolver, admission fences, and durable controls; cross-owner reads remain refused and sharing actions remain unregistered. dev-01 fmt, Clippy with warnings denied, and workspace tests pass.
kayg commented 2026-11-03 05:41:24 +00:00
Author
Owner
Copy link

BM14 follow-up: both auth failures are deterministic integration mismatches, not timing failures. Dev added the tests after this branch diverged; #829's CROSS_OWNER_SHARING_ENABLED=false rejects the recipient operations they still unwrap as successful. The baseline has no dormant gate. I merged the pinned BM14 base 41f2efa3 into this owning branch to bring those tests into scope.

The fix preserves the dormant gate and every cache/sequence check. It additionally requires recipient admission to return NotFound without consuming a sequence, then verifies owner admission still receives sequence 6. The replay test polls the authorization future under the held per-note mutex, requiring Pending for both owner and recipient, then requires owner success and recipient refusal after release. No timeout or access restriction is relaxed. Before/after 30-run merged-tree auth results under one ordinary slot build will follow here. No train rerun or merge into dev.

BM14 follow-up: both auth failures are deterministic integration mismatches, not timing failures. Dev added the tests after this branch diverged; #829's `CROSS_OWNER_SHARING_ENABLED=false` rejects the recipient operations they still unwrap as successful. The baseline has no dormant gate. I merged the pinned BM14 base `41f2efa3` into this owning branch to bring those tests into scope. The fix preserves the dormant gate and every cache/sequence check. It additionally requires recipient admission to return NotFound without consuming a sequence, then verifies owner admission still receives sequence 6. The replay test polls the authorization future under the held per-note mutex, requiring Pending for both owner and recipient, then requires owner success and recipient refusal after release. No timeout or access restriction is relaxed. Before/after 30-run merged-tree auth results under one ordinary slot build will follow here. No train rerun or merge into dev.
kayg commented 2026-11-03 05:45:09 +00:00
Author
Owner
Copy link

BM14 auth fix pushed: 97d790f6 (pinned dev integration merge d95b705c is its parent). Both original failures reproduced in 30/30 runs, and both pass in 30/30 after the fix on BM14's merged source tree. Each run executes the entire auth library suite with its normal parallel test threads, not just the named case. Before: 62 passed / 2 failed per run. After: 64 passed / 0 failed per run.

Cause: the dev baseline admits recipient writes, but #829 deliberately hard-disables cross-owner resolution. The new dev tests' success unwraps therefore fail consistently when the branches are combined. There is no evidence of a production access race in these two failures.

Fix: retain the dormant access gate; verify recipient admission returns NotFound and consumes no sequence, then verify the next owner write receives sequence 6. All owner cache and DB-operation assertions remain. The writer-lock regression now polls the actual authorization future under the held per-note mutex, requires Pending for both identities, then requires owner success and recipient refusal after release. No timeout is extended and no access assertion is removed to bypass a race.

Dev-01 commands, under Nix and ~/bin/slot, CARGO_PROFILE_DEV_DEBUG=0 CARGO_PROFILE_TEST_DEBUG=0 CARGO_INCREMENTAL=0:

cargo test -p calternal-auth --lib -j4 --no-run
# Before: retained the exact test executable, then invoked it 30 times:
for i in $(seq 1 30); do ~/bm14-fix-logs/auth-before --nocapture; done
# After:
for i in $(seq 1 30); do cargo test -p calternal-auth --lib -j4 -- --nocapture; done

Before loops overlapped the ordinary ~/bin/slot ... cargo build -j4 preparation job. After loops overlapped one ordinary ~/bin/slot env CARGO_TARGET_DIR=$PWD/target/load cargo build -p calternal-auth -j4 job. Both use normal slots, no train lock or artificial CPU load. cargo fmt --all ran on dev-01. Browser work remains in progress. No full train or merge into dev was run.

BM14 auth fix pushed: `97d790f6` (pinned dev integration merge `d95b705c` is its parent). Both original failures reproduced in **30/30** runs, and both pass in **30/30** after the fix on BM14's merged source tree. Each run executes the entire auth library suite with its normal parallel test threads, not just the named case. Before: 62 passed / 2 failed per run. After: 64 passed / 0 failed per run. Cause: the dev baseline admits recipient writes, but #829 deliberately hard-disables cross-owner resolution. The new dev tests' success unwraps therefore fail consistently when the branches are combined. There is no evidence of a production access race in these two failures. Fix: retain the dormant access gate; verify recipient admission returns NotFound and consumes no sequence, then verify the next owner write receives sequence 6. All owner cache and DB-operation assertions remain. The writer-lock regression now polls the actual authorization future under the held per-note mutex, requires Pending for both identities, then requires owner success and recipient refusal after release. No timeout is extended and no access assertion is removed to bypass a race. Dev-01 commands, under Nix and `~/bin/slot`, `CARGO_PROFILE_DEV_DEBUG=0 CARGO_PROFILE_TEST_DEBUG=0 CARGO_INCREMENTAL=0`: ```sh cargo test -p calternal-auth --lib -j4 --no-run # Before: retained the exact test executable, then invoked it 30 times: for i in $(seq 1 30); do ~/bm14-fix-logs/auth-before --nocapture; done # After: for i in $(seq 1 30); do cargo test -p calternal-auth --lib -j4 -- --nocapture; done ``` Before loops overlapped the ordinary `~/bin/slot ... cargo build -j4` preparation job. After loops overlapped one ordinary `~/bin/slot env CARGO_TARGET_DIR=$PWD/target/load cargo build -p calternal-auth -j4` job. Both use normal slots, no train lock or artificial CPU load. `cargo fmt --all` ran on dev-01. Browser work remains in progress. No full train or merge into dev was run.
kayg commented 2026-11-03 06:08:03 +00:00
Author
Owner
Copy link

Additional targeted validation on the merged tree with #829 fix 97d790f6:

  • cargo fmt --all --check: PASS.
  • cargo clippy -p calternal-auth --lib --tests -j4 -- -D warnings: PASS (dev-01, Nix, normal slot).
    The recorded 30/30 before failures and 30/30 after passes remain unchanged. No full train was run.
Additional targeted validation on the merged tree with #829 fix `97d790f6`: - `cargo fmt --all --check`: PASS. - `cargo clippy -p calternal-auth --lib --tests -j4 -- -D warnings`: PASS (dev-01, Nix, normal slot). The recorded 30/30 before failures and 30/30 after passes remain unchanged. No full train was run.
kayg commented 2026-11-03 06:17:58 +00:00
Author
Owner
Copy link

bm14 follow-up complete. head 97d790f69259b0308598ca0aafd2407ef1a1226e. both named rust tests: 0/30 before, 30/30 after, from 30 complete 64-test auth-library runs with a concurrent ordinary slot build. fmt and targeted auth clippy pass. detailed rust evidence: #829 (comment)

final cross-case results, commands, limitations and cleanup: #850 (comment)

no train run or merge. own VM jobs finished and large temporary artifacts removed.

bm14 follow-up complete. head `97d790f69259b0308598ca0aafd2407ef1a1226e`. both named rust tests: 0/30 before, 30/30 after, from 30 complete 64-test auth-library runs with a concurrent ordinary slot build. fmt and targeted auth clippy pass. detailed rust evidence: https://git.kayg.org/kayg/calternal/pulls/829#issuecomment-76545 final cross-case results, commands, limitations and cleanup: https://git.kayg.org/kayg/calternal/pulls/850#issuecomment-76594 no train run or merge. own VM jobs finished and large temporary artifacts removed.
kayg commented 2026-11-04 00:41:13 +00:00
Author
Owner
Copy link

Perf fix pushed to this branch (head 72eb0eb1): the room-delivery check for an owner's own note no longer reads the sharing db per frame.

  • e859b19c auth: authorize_room_delivery for the owner with a session/development credential runs only the note-ID size and read-scope checks (what resolve_resource_in reduces to for an owner). App passwords (revocable) keep the full check.
  • 08d11203 room: Home::owner_delivery answers the owner's connection in place (no caller clone, no sharing call).
  • aeda1a4f Notes::resolve_resource answers owner Read/Edit with the same size + scope checks and an Owner result (join used to make two reads); sequence is passed through as given.
  • 95c6ed00 test: the shared-read recheck probe now reads as a recipient (an owner never needs that second read); same assertions.
    Access logic touched, stated plainly: owner Read/Edit of own note only; same allow/deny outcomes (size, scope, Forbidden); recipients, app passwords, Snapshot and ManageSharing take the unchanged path. No check was relaxed. Gates: cargo test -p calternal-auth -p calternal-notes -p calternal-core -p calternal-server --lib pass (65/551/17/16), cargo fmt --all --check clean.

Method: release binaries built on dev-01, relayed to the perf VMs with matching sha256 (dev de99cfbf…, train+fixes 68dd30a0…, same dev calternal-perf for every run). Every run under ~/bin/perf-lock, seed 1, 10k Home, alternating pairs. Medians of per-run p50 / p95, [min–max of p50]. The perf VMs were shared with other agents' long lock holds, so runs spread over several windows; dev-vs-dev spread alone is 0.20–0.37 ms on round trip.

sync.round_trip bisect (10k, perf-02, ms)

Build p50 p95 runs
dev 41f2efa3 0.302–0.327 0.41–0.43 5
train through #829 (ef284146) 0.418 (+34%) 0.547 (+33%) 5
train through #835 (efa2bf37) 0.548 (+77%) 0.766 (+87%) 4
#829 + fix (72eb0eb1) vs dev same window 0.217 vs 0.235 0.337 vs 0.344 5
#835 + owner-delivery fixes only (F3) 0.351 vs 0.327 (+7%) 0.451 vs 0.417 5
train + all fixes (TF2) vs dev, 10k 0.286 vs 0.290 0.388 vs 0.401 5

Culprits: #829 (Auth::authorize_room_delivery, called per delivered frame, does a sharing-db read even for the owner's own note) and #835 (the same check per queued frame in the room, again per written frame in live.rs, plus two more reads per room join and a clone of the caller). Both are the same cause: an owner reading their own note never depends on a grant row, but every frame paid a database hop.

Perf fix pushed to this branch (head `72eb0eb1`): the room-delivery check for an owner's own note no longer reads the sharing db per frame. - `e859b19c` auth: `authorize_room_delivery` for the owner with a session/development credential runs only the note-ID size and read-scope checks (what `resolve_resource_in` reduces to for an owner). App passwords (revocable) keep the full check. - `08d11203` room: `Home::owner_delivery` answers the owner's connection in place (no caller clone, no sharing call). - `aeda1a4f` `Notes::resolve_resource` answers owner Read/Edit with the same size + scope checks and an `Owner` result (join used to make two reads); sequence is passed through as given. - `95c6ed00` test: the shared-read recheck probe now reads as a recipient (an owner never needs that second read); same assertions. **Access logic touched, stated plainly:** owner Read/Edit of own note only; same allow/deny outcomes (size, scope, Forbidden); recipients, app passwords, Snapshot and ManageSharing take the unchanged path. No check was relaxed. Gates: `cargo test -p calternal-auth -p calternal-notes -p calternal-core -p calternal-server --lib` pass (65/551/17/16), `cargo fmt --all --check` clean. Method: release binaries built on dev-01, relayed to the perf VMs with matching sha256 (dev `de99cfbf…`, train+fixes `68dd30a0…`, same dev `calternal-perf` for every run). Every run under `~/bin/perf-lock`, seed 1, 10k Home, alternating pairs. Medians of per-run p50 / p95, [min–max of p50]. The perf VMs were shared with other agents' long lock holds, so runs spread over several windows; dev-vs-dev spread alone is 0.20–0.37 ms on round trip. ### sync.round_trip bisect (10k, perf-02, ms) | Build | p50 | p95 | runs | |---|---|---|---| | dev `41f2efa3` | 0.302–0.327 | 0.41–0.43 | 5 | | train through #829 (`ef284146`) | 0.418 (+34%) | 0.547 (+33%) | 5 | | train through #835 (`efa2bf37`) | 0.548 (+77%) | 0.766 (+87%) | 4 | | #829 + fix (`72eb0eb1`) vs dev same window | 0.217 vs 0.235 | 0.337 vs 0.344 | 5 | | #835 + owner-delivery fixes only (F3) | 0.351 vs 0.327 (+7%) | 0.451 vs 0.417 | 5 | | **train + all fixes (TF2) vs dev, 10k** | **0.286 vs 0.290** | **0.388 vs 0.401** | 5 | Culprits: **#829** (`Auth::authorize_room_delivery`, called per delivered frame, does a sharing-db read even for the owner's own note) and **#835** (the same check per queued frame in the room, again per written frame in `live.rs`, plus two more reads per room join and a clone of the caller). Both are the same cause: an owner reading their own note never depends on a grant row, but every frame paid a database hop.
kayg commented 2026-11-04 00:46:49 +00:00
Author
Owner
Copy link

Independent review: owner fast path (#829 head 72eb0eb1, #835 head 46be23e3)

Scope: e859b19c, 08d11203, aeda1a4f, 95c6ed00 on #829, plus fd01b6d8, a7d8cac3, 85d02dfb, 231e4457 and the merges on #835. I only read code. Nothing was built or run.

Verdict: safe to train. A non-owner can't reach the fast path. Neither can an app password. Snapshot and ManageSharing still take the full path. There are no P1s. The two P2s are hardening and test gaps, not open holes today. Security is not the merge gate, though: the 100k sync.round_trip (+17% p50) and the 10k open.note numbers on #835 are not shown flat yet, so under the no-regression rule those still decide the merge.

P1

None.

P2

  1. The fast path excludes one credential kind instead of allowing named ones. The check is kind != AppPassword at crates/notes/src/home.rs:233 and :263, crates/auth/src/sharing.rs:996 (#829) and :984 (#835). Credential::has_scope (crates/core/src/action.rs:82) is true for every kind except AppPassword, so the "scope check" on the fast path does nothing. Today only Session, AppPassword and Development exist, so nothing is exposed now. The problem comes with the next kind. If a scoped Cal or assistant key (#839) or a link token is added as a new CredentialKind, it gets owner Read and Edit with no scope check and no revocation check, and no test will fail. Fix: one helper, e.g. Credential::is_session_like(), written as an exhaustive match with no _ arm (Session | Development => true, AppPassword => false). Use it at all four places and at the cacheable_owner check (sharing.rs:884 on #829, :1069 on #835). Adding a kind then forces someone to decide.
  2. Nothing tests a revoked owner app password through the real call sites. other_people_and_app_passwords_take_the_full_path and owner_resource_answers_owner_for_read_and_edit_only only show that the routing function returns None. No test wires Auth into Notes, revokes an owner's app password, and then checks that Notes::get_authorized, Notes::join, room send_to_authorized and the live writer (Notes::authorize_delivery, lib.rs:965 on #835) all refuse. The four call sites now pick their path separately (lib.rs:522/540, sync.rs send_to_authorized, home.rs:469, lib.rs:965). If one of them later calls the fast path without the kind check, nothing catches it. I found no such test under crates/*/tests either (app_passwords.rs covers the session store only). Suggested test: owner app password with notes.read/notes.write joins and receives one frame, then gets revoked. The next frame, get_authorized and a new join are all refused.

P3

  1. owner_room_delivery_keeps_size_and_scope_checks_without_a_db_hop (sharing.rs:2211 on #835) never asserts that auth.inner.db.run_count() stays unchanged, even though its name says so. That assertion is the cheap guard against this perf regression coming back. owner_unshared_admission_is_cached… already shows the pattern.
  2. The same precheck exists three times: owner_read_precheck (sharing.rs:451/:470), home::owner_delivery (home.rs:228) and home::owner_resource (home.rs:255), each with its own copy of the 256-byte limit and error strings. They could drift. Keep one copy in calternal_core and call it from all three.
  3. Owner Read now returns the sequence the caller passed (None on join) instead of last_note_seq, so an owner join no longer raises room.delivery_sequence (sync.rs:90). I checked and it does no harm. The frame sequence never reaches the client (live.rs:235 sends only the bytes). The owner's writer check ignores it. A recipient's own join still resolves the current sequence and raises it, and so do admitted edits. Worth one comment at Notes::join.
  4. On #835 the owner fast path in Notes::authorize_delivery (lib.rs:965) runs before self.enter(owner). An owner frame that is already queued is now written even while that Home is shutting down. Only the owner gets their own bytes, so nothing leaks. It is a small behaviour change.

Checked, no finding

  • Spoofed identity: caller.user comes only from the authenticator. DevUser is a single fixed user (server/src/auth.rs:78-87). In a owner/id reference the owner is parsed, and the fast path only runs when it equals caller.user.
  • Another user's note: the note's name space is the owner's Home (self.home.user in the room, owner == caller.user in Notes), so an id can only name the caller's own note. watch::valid_id and the size check still run first.
  • Case-insensitive paths, symlinks, renames: the fast path works on note ids, not paths, and doesn't change how paths are resolved. The old owner path didn't read note rows either (access_in returns Owner when owner == person, sharing.rs:378).
  • Owner changed or account removed: the repo has no note transfer and no account removal code, and Home.user is fixed when the Home opens. I found no "A13" in code or docs.
  • Caching across revocation: nothing new is cached. Each connection keeps the caller it joined with, as before. An app-password owner is still rechecked against the db on every frame (ensure_credential). A session owner was never rechecked on the old path either (ensure_credential returns Ok for anything except app passwords), so session sign-out on a live socket is not a regression here.
  • Recipients: every recipient path still takes the full check. isolation.rs:380-420 refuses bob through the action, get_authorized, resolve_resource and join. The fence, epoch, slow-recipient, revoke and regrant tests in sharing.rs use real Auth with recipient callers, so the change doesn't touch them.
  • 95c6ed00 (the test the author asked about): coverage is not weaker. The probe now drives a real recipient through get_authorized, and the second read is still refused with the same assertions. The mock only lets a non-owner in when reject_second_read is set (pipeline.rs memory_tests), so the other probes keep their owner-only behaviour. The owner case is covered by owner_resource_answers_owner_for_read_and_edit_only and owner_session_passes_and_keeps_size_checks. Those are unit-level, which is what P2.2 is about.
## Independent review: owner fast path (#829 head `72eb0eb1`, #835 head `46be23e3`) Scope: `e859b19c`, `08d11203`, `aeda1a4f`, `95c6ed00` on #829, plus `fd01b6d8`, `a7d8cac3`, `85d02dfb`, `231e4457` and the merges on #835. I only read code. Nothing was built or run. **Verdict: safe to train.** A non-owner can't reach the fast path. Neither can an app password. Snapshot and ManageSharing still take the full path. There are no P1s. The two P2s are hardening and test gaps, not open holes today. Security is not the merge gate, though: the 100k `sync.round_trip` (+17% p50) and the 10k `open.note` numbers on #835 are not shown flat yet, so under the no-regression rule those still decide the merge. ### P1 None. ### P2 1. **The fast path excludes one credential kind instead of allowing named ones.** The check is `kind != AppPassword` at `crates/notes/src/home.rs:233` and `:263`, `crates/auth/src/sharing.rs:996` (#829) and `:984` (#835). `Credential::has_scope` (`crates/core/src/action.rs:82`) is `true` for every kind except AppPassword, so the "scope check" on the fast path does nothing. Today only Session, AppPassword and Development exist, so nothing is exposed now. The problem comes with the next kind. If a scoped Cal or assistant key (#839) or a link token is added as a new `CredentialKind`, it gets owner Read and Edit with no scope check and no revocation check, and no test will fail. Fix: one helper, e.g. `Credential::is_session_like()`, written as an exhaustive `match` with no `_` arm (`Session | Development => true, AppPassword => false`). Use it at all four places and at the `cacheable_owner` check (`sharing.rs:884` on #829, `:1069` on #835). Adding a kind then forces someone to decide. 2. **Nothing tests a revoked owner app password through the real call sites.** `other_people_and_app_passwords_take_the_full_path` and `owner_resource_answers_owner_for_read_and_edit_only` only show that the routing function returns `None`. No test wires `Auth` into `Notes`, revokes an owner's app password, and then checks that `Notes::get_authorized`, `Notes::join`, room `send_to_authorized` and the live writer (`Notes::authorize_delivery`, `lib.rs:965` on #835) all refuse. The four call sites now pick their path separately (`lib.rs:522/540`, `sync.rs` `send_to_authorized`, `home.rs:469`, `lib.rs:965`). If one of them later calls the fast path without the kind check, nothing catches it. I found no such test under `crates/*/tests` either (`app_passwords.rs` covers the session store only). Suggested test: owner app password with `notes.read`/`notes.write` joins and receives one frame, then gets revoked. The next frame, `get_authorized` and a new `join` are all refused. ### P3 3. `owner_room_delivery_keeps_size_and_scope_checks_without_a_db_hop` (`sharing.rs:2211` on #835) never asserts that `auth.inner.db.run_count()` stays unchanged, even though its name says so. That assertion is the cheap guard against this perf regression coming back. `owner_unshared_admission_is_cached…` already shows the pattern. 4. The same precheck exists three times: `owner_read_precheck` (`sharing.rs:451`/`:470`), `home::owner_delivery` (`home.rs:228`) and `home::owner_resource` (`home.rs:255`), each with its own copy of the 256-byte limit and error strings. They could drift. Keep one copy in `calternal_core` and call it from all three. 5. Owner Read now returns the sequence the caller passed (`None` on join) instead of `last_note_seq`, so an owner join no longer raises `room.delivery_sequence` (`sync.rs:90`). I checked and it does no harm. The frame sequence never reaches the client (`live.rs:235` sends only the bytes). The owner's writer check ignores it. A recipient's own join still resolves the current sequence and raises it, and so do admitted edits. Worth one comment at `Notes::join`. 6. On #835 the owner fast path in `Notes::authorize_delivery` (`lib.rs:965`) runs before `self.enter(owner)`. An owner frame that is already queued is now written even while that Home is shutting down. Only the owner gets their own bytes, so nothing leaks. It is a small behaviour change. ### Checked, no finding - **Spoofed identity:** `caller.user` comes only from the authenticator. `DevUser` is a single fixed user (`server/src/auth.rs:78-87`). In a `owner/id` reference the owner is parsed, and the fast path only runs when it equals `caller.user`. - **Another user's note:** the note's name space is the owner's Home (`self.home.user` in the room, `owner == caller.user` in Notes), so an id can only name the caller's own note. `watch::valid_id` and the size check still run first. - **Case-insensitive paths, symlinks, renames:** the fast path works on note ids, not paths, and doesn't change how paths are resolved. The old owner path didn't read note rows either (`access_in` returns `Owner` when `owner == person`, `sharing.rs:378`). - **Owner changed or account removed:** the repo has no note transfer and no account removal code, and `Home.user` is fixed when the Home opens. I found no "A13" in code or docs. - **Caching across revocation:** nothing new is cached. Each connection keeps the caller it joined with, as before. An app-password owner is still rechecked against the db on every frame (`ensure_credential`). A session owner was never rechecked on the old path either (`ensure_credential` returns `Ok` for anything except app passwords), so session sign-out on a live socket is not a regression here. - **Recipients:** every recipient path still takes the full check. `isolation.rs:380-420` refuses bob through the action, `get_authorized`, `resolve_resource` and `join`. The fence, epoch, slow-recipient, revoke and regrant tests in `sharing.rs` use real `Auth` with recipient callers, so the change doesn't touch them. - **`95c6ed00` (the test the author asked about):** coverage is not weaker. The probe now drives a real recipient through `get_authorized`, and the second read is still refused with the same assertions. The mock only lets a non-owner in when `reject_second_read` is set (`pipeline.rs` memory_tests), so the other probes keep their owner-only behaviour. The owner case is covered by `owner_resource_answers_owner_for_read_and_edit_only` and `owner_session_passes_and_keeps_size_checks`. Those are unit-level, which is what P2.2 is about.
kayg commented 2026-11-04 10:04:54 +00:00
Author
Owner
Copy link

Security/access review: #829 (72eb0eb1) + #835 (46be23e3), follow-up #909 (98afda51), against dev 41f2efa3

Review only, nothing changed. Read: crates/auth/src/{sharing,db,app_password,session,lib}.rs, crates/core/src/{sharing,action}.rs, crates/notes/src/{lib,actions,home}.rs, crates/notes/src/room/{sync,pipeline,actor,mod}.rs, crates/server/src/{live,rooms,auth}.rs, crates/calternal/src/isolation.rs, plan 0409. Line numbers are from #835 unless marked #909.

Prod exposure in BM14e: cross-owner access is off in two places, CROSS_OWNER_SHARING_ENABLED=false (sharing.rs:15, enforced at sharing.rs:492 and :962), and sharing.* is unregistered (removed from contracts/actions.json, test isolation.rs:267). So no grant row can exist and no recipient path can be reached. In prod, what changes is the owner path (fast path, per-frame check, notes.get resolver) and the auth migrations v12/v13.

P1: none

P2: fix before PR 3 enables sharing (not reachable in BM14e)

  1. cleanup_sharing sweeps every user's sharing tables inside each shared-note write. sharing.rs:762-878 runs five global INSERT…SELECT/DELETE statements over installations, recipient_control, share_fanout and share_fanout_acks for all owners. It runs inside an IMMEDIATE auth.db transaction from admit (:1121), grant (:1680), revoke (:1813), acknowledge_control (:1290, :1326) and acknowledge_fanout (:1379, :1400). Sequence after PR 3: A shares a note with B; every room edit batch from A calls admit, which runs cleanup_sharing. Cost is O(all sharing rows on the server) per keystroke batch, and it holds the auth.db writer, so other auth writes (sign-in, session/app-password touch) wait behind it. Today admit returns at the !shared check before reaching it. Ask: move it to a bounded periodic task keyed by share, and add a perf-VM gate for "many shares + one typing owner".
  2. Room actor authorizes recipients one at a time, per update. room/sync.rs:300-349: broadcast_update awaits authorize_room_delivery for each non-owner connection, which takes the note writer lock and does one auth.db run (sharing.rs:1006-1023), all inside the room actor. With 32 recipients, each update costs 32 sequential DB hops before the actor takes its next Cmd, including the owner's next keystroke. The server then rechecks every dequeued frame again (live.rs:257-283, sharing.rs:1009), so that is about 2 hops per frame per recipient. The owner-only path is unaffected (home::owner_delivery). Ask: read the room state once per update for all recipients, and measure on perf VMs before PR 3 (no-regression rule).
  3. One flag opens only two of the read paths. Flipping CROSS_OWNER_SHARING_ENABLED enables notes.get{owner_id} and room join owner/id. History, attachments, search, backlinks, link previews, export and sync still read only caller.user's Home (plan 2B/2C). Nothing leaks today. The risk is that someone flips the const in PR 3 before 2B/2C land, or a new entry point that accepts an owner reference skips resolve_resource. Ask: an inventory test (like the action/route inventory) that lists every Notes entry point taking a note ID and asserts it either goes through Notes::resolve_resource or rejects owner references. Make it a precondition for the flip.

P3

  1. #909 doc claim is wrong. core/src/action.rs CredentialKind::owner_fast_path (#909) says Session/Development "cannot be revoked mid-session". Sessions can be revoked. The fast path is still equivalent to the full path for two reasons. First, ensure_credential returns Ok for every non-app-password credential (sharing.rs:264-267), so the full path never rechecked sessions either. Second, live sockets close on account.installation-revoked (live.rs:150, :419). Please reword to "kinds whose access the sharing layer never rechecks; revocation is enforced by authentication and the socket sign-out". Otherwise a future scoped key or link token may be marked true by analogy. The same wording appears in home.rs:222-250.
  2. #909 shutdown guard overclaims. In notes/src/lib.rs (authorize_delivery, #909), _work is dropped on return, before live.rs writes the frame. It refuses frames once shutdown has started but does not hold shutdown off during the write. The non-fast path behaves the same, so this is a comment fix only.
  3. notes.get with an owner app password now makes 2 extra auth.db round trips. get_authorized calls resolve_resource (full path, ensure_credential query) and then authorize_room_delivery, which for app passwords repeats resolve_resource_in (sharing.rs:986-1001). One check is enough because the read already happened under the first. No perf-VM numbers were posted for app-password notes.get. Collapse it, or measure it per the no-regression rule.
  4. Revoke can wait up to 15 s. The frame writer holds the recipient delivery lock across socket.send (live.rs:276-280, sharing.rs:1009-1012), and revoke takes the same lock first (:1756). A stalled recipient socket delays that recipient's revoke by up to WRITE_TIMEOUT = 15 s (live.rs:40). This is bounded and tested for other recipients (:2341). It is a PR 3 UX note: the revoke button must not look stuck.
  5. grant doesn't check the note exists in the owner's Home (sharing.rs:1556-1560). Delete/purge (Q14) doesn't remove note_shares either. Effect: orphan share rows in the owner's own namespace only. No leak.

Checked and OK

  • Owner references: notes.get{owner_id} (actions.rs:67-79) and room owner/id (lib.rs:883-895) give the same NotFound "no such note" for a bad UUID, another user, a missing note or no grant. The old "no note {id:?}" echo is gone. The resolver runs before enter(owner) in both join and get_authorized, so no other user's Home is opened. watch::valid_id excludes /, so the split can't change what an existing ID means.
  • What a shared user can learn: access_in never reads note existence, titles or counts. Every refusal is the same not_found() with no extra work, so there's no timing difference between "note exists" and "no grant".
  • Fast path allow-list (#909): the match is exhaustive (no _ arm), AppPassword takes the full path, and check_owner_note keeps one size/scope rule. Development is reachable only through --dev-user (main.rs:106, server/auth.rs:78). Owner app passwords are rechecked on every call, with installation binding plus installation_notes_scopes (sharing.rs:264-291). Test isolation.rs:1777 (#909) covers the real call sites.
  • Revocation design (for PR 3): revoke commits fence + epoch under the note lock, evicts after commit, rechecks each dequeued frame against interval/fence/epoch (sharing.rs:570-641, live.rs:257), and rechecks prepared reads (get_authorized). Recipient app passwords are rechecked in the same transaction. Offline devices get durable controls with per-Installation acks, plus a reset for installations idle more than 30 days. I found no lock cycle: grant/revoke take recipient lock then note lock, the frame writer takes only the recipient lock, the room actor takes only the note lock, and eviction runs after the note lock is dropped.
  • IDOR: acknowledge_control filters on rc.recipient_id=caller (:1305), acknowledge_fanout runs access_in first, and a non-owner revoke returns the same not_found as a missing grant.
  • Limits: event ID ≤200, note ID ≤256, control ID ≤200, ≤32 active grants per note, deny_unknown_fields on every input, roles::can_see before granting.
  • Renamed/moved notes keep their grants because grants are keyed by the stable note ID.

Not in these PRs, so not reviewed

Named/link tokens and QR codes (no token generation, hashing, compare, expiry or Referer handling exists yet), folder/tag-scoped grants, and the web share UI. Search, backlinks, previews/unfurls, history, trash, attachments by URL, export, IMAP, webhooks and notifications are not shared-aware yet; they still use only caller.user, so there is nothing to bypass today. All of these need their own review in 2B/2C/PR 3. Link tokens especially need: ≥128-bit random, stored hashed, constant-time lookup by hash, expiry, Referrer-Policy: no-referrer, and redaction from logs.

Verdict: P1 0 / P2 3 / P3 5. Safe to ship in BM14e: yes (P2s are unreachable while the flag is off and actions are unregistered). #909 is fine for the next train after the comment fixes in P3 4–5.

## Security/access review: #829 (72eb0eb1) + #835 (46be23e3), follow-up #909 (98afda51), against dev 41f2efa3 Review only, nothing changed. Read: `crates/auth/src/{sharing,db,app_password,session,lib}.rs`, `crates/core/src/{sharing,action}.rs`, `crates/notes/src/{lib,actions,home}.rs`, `crates/notes/src/room/{sync,pipeline,actor,mod}.rs`, `crates/server/src/{live,rooms,auth}.rs`, `crates/calternal/src/isolation.rs`, plan 0409. Line numbers are from #835 unless marked #909. **Prod exposure in BM14e:** cross-owner access is off in two places, `CROSS_OWNER_SHARING_ENABLED=false` (`sharing.rs:15`, enforced at `sharing.rs:492` and `:962`), and `sharing.*` is unregistered (removed from `contracts/actions.json`, test `isolation.rs:267`). So no grant row can exist and no recipient path can be reached. In prod, what changes is the owner path (fast path, per-frame check, `notes.get` resolver) and the auth migrations v12/v13. ### P1: none ### P2: fix before PR 3 enables sharing (not reachable in BM14e) 1. **`cleanup_sharing` sweeps every user's sharing tables inside each shared-note write.** `sharing.rs:762-878` runs five global `INSERT…SELECT`/`DELETE` statements over `installations`, `recipient_control`, `share_fanout` and `share_fanout_acks` for all owners. It runs inside an `IMMEDIATE` auth.db transaction from `admit` (`:1121`), `grant` (`:1680`), `revoke` (`:1813`), `acknowledge_control` (`:1290`, `:1326`) and `acknowledge_fanout` (`:1379`, `:1400`). Sequence after PR 3: A shares a note with B; every room edit batch from A calls `admit`, which runs `cleanup_sharing`. Cost is O(all sharing rows on the server) per keystroke batch, and it holds the auth.db writer, so other auth writes (sign-in, session/app-password `touch`) wait behind it. Today `admit` returns at the `!shared` check before reaching it. Ask: move it to a bounded periodic task keyed by share, and add a perf-VM gate for "many shares + one typing owner". 2. **Room actor authorizes recipients one at a time, per update.** `room/sync.rs:300-349`: `broadcast_update` awaits `authorize_room_delivery` for each non-owner connection, which takes the note writer lock and does one auth.db `run` (`sharing.rs:1006-1023`), all inside the room actor. With 32 recipients, each update costs 32 sequential DB hops before the actor takes its next `Cmd`, including the owner's next keystroke. The server then rechecks every dequeued frame again (`live.rs:257-283`, `sharing.rs:1009`), so that is about 2 hops per frame per recipient. The owner-only path is unaffected (`home::owner_delivery`). Ask: read the room state once per update for all recipients, and measure on perf VMs before PR 3 (no-regression rule). 3. **One flag opens only two of the read paths.** Flipping `CROSS_OWNER_SHARING_ENABLED` enables `notes.get{owner_id}` and room join `owner/id`. History, attachments, search, backlinks, link previews, export and sync still read only `caller.user`'s Home (plan 2B/2C). Nothing leaks today. The risk is that someone flips the const in PR 3 before 2B/2C land, or a new entry point that accepts an owner reference skips `resolve_resource`. Ask: an inventory test (like the action/route inventory) that lists every Notes entry point taking a note ID and asserts it either goes through `Notes::resolve_resource` or rejects owner references. Make it a precondition for the flip. ### P3 4. **#909 doc claim is wrong.** `core/src/action.rs` `CredentialKind::owner_fast_path` (#909) says Session/Development "cannot be revoked mid-session". Sessions can be revoked. The fast path is still equivalent to the full path for two reasons. First, `ensure_credential` returns `Ok` for every non-app-password credential (`sharing.rs:264-267`), so the full path never rechecked sessions either. Second, live sockets close on `account.installation-revoked` (`live.rs:150`, `:419`). Please reword to "kinds whose access the sharing layer never rechecks; revocation is enforced by authentication and the socket sign-out". Otherwise a future scoped key or link token may be marked `true` by analogy. The same wording appears in `home.rs:222-250`. 5. **#909 shutdown guard overclaims.** In `notes/src/lib.rs` (`authorize_delivery`, #909), `_work` is dropped on return, before `live.rs` writes the frame. It refuses frames once shutdown has started but does not hold shutdown off during the write. The non-fast path behaves the same, so this is a comment fix only. 6. **`notes.get` with an owner app password now makes 2 extra auth.db round trips.** `get_authorized` calls `resolve_resource` (full path, `ensure_credential` query) and then `authorize_room_delivery`, which for app passwords repeats `resolve_resource_in` (`sharing.rs:986-1001`). One check is enough because the read already happened under the first. No perf-VM numbers were posted for app-password `notes.get`. Collapse it, or measure it per the no-regression rule. 7. **Revoke can wait up to 15 s.** The frame writer holds the recipient delivery lock across `socket.send` (`live.rs:276-280`, `sharing.rs:1009-1012`), and `revoke` takes the same lock first (`:1756`). A stalled recipient socket delays that recipient's revoke by up to `WRITE_TIMEOUT` = 15 s (`live.rs:40`). This is bounded and tested for *other* recipients (`:2341`). It is a PR 3 UX note: the revoke button must not look stuck. 8. **`grant` doesn't check the note exists in the owner's Home** (`sharing.rs:1556-1560`). Delete/purge (Q14) doesn't remove `note_shares` either. Effect: orphan share rows in the owner's own namespace only. No leak. ### Checked and OK - **Owner references:** `notes.get{owner_id}` (`actions.rs:67-79`) and room `owner/id` (`lib.rs:883-895`) give the same `NotFound "no such note"` for a bad UUID, another user, a missing note or no grant. The old `"no note {id:?}"` echo is gone. The resolver runs before `enter(owner)` in both join and `get_authorized`, so no other user's Home is opened. `watch::valid_id` excludes `/`, so the split can't change what an existing ID means. - **What a shared user can learn:** `access_in` never reads note existence, titles or counts. Every refusal is the same `not_found()` with no extra work, so there's no timing difference between "note exists" and "no grant". - **Fast path allow-list (#909):** the match is exhaustive (no `_` arm), AppPassword takes the full path, and `check_owner_note` keeps one size/scope rule. `Development` is reachable only through `--dev-user` (`main.rs:106`, `server/auth.rs:78`). Owner app passwords are rechecked on every call, with installation binding plus `installation_notes_scopes` (`sharing.rs:264-291`). Test `isolation.rs:1777` (#909) covers the real call sites. - **Revocation design (for PR 3):** revoke commits fence + epoch under the note lock, evicts after commit, rechecks each dequeued frame against interval/fence/epoch (`sharing.rs:570-641`, `live.rs:257`), and rechecks prepared reads (`get_authorized`). Recipient app passwords are rechecked in the same transaction. Offline devices get durable controls with per-Installation acks, plus a reset for installations idle more than 30 days. I found no lock cycle: grant/revoke take recipient lock then note lock, the frame writer takes only the recipient lock, the room actor takes only the note lock, and eviction runs after the note lock is dropped. - **IDOR:** `acknowledge_control` filters on `rc.recipient_id=caller` (`:1305`), `acknowledge_fanout` runs `access_in` first, and a non-owner `revoke` returns the same `not_found` as a missing grant. - **Limits:** event ID ≤200, note ID ≤256, control ID ≤200, ≤32 active grants per note, `deny_unknown_fields` on every input, `roles::can_see` before granting. - Renamed/moved notes keep their grants because grants are keyed by the stable note ID. ### Not in these PRs, so not reviewed Named/link tokens and QR codes (no token generation, hashing, compare, expiry or Referer handling exists yet), folder/tag-scoped grants, and the web share UI. Search, backlinks, previews/unfurls, history, trash, attachments by URL, export, IMAP, webhooks and notifications are not shared-aware yet; they still use only `caller.user`, so there is nothing to bypass today. All of these need their own review in 2B/2C/PR 3. Link tokens especially need: ≥128-bit random, stored hashed, constant-time lookup by hash, expiry, `Referrer-Policy: no-referrer`, and redaction from logs. **Verdict:** P1 0 / P2 3 / P3 5. Safe to ship in BM14e: **yes** (P2s are unreachable while the flag is off and actions are unregistered). #909 is fine for the next train after the comment fixes in P3 4–5.
kayg commented 2026-11-07 17:53:17 +00:00
Author
Owner
Copy link

Landed in dev via 12103f1c97 — closing.

Landed in dev via 12103f1c973f1e810dd4a9a3bff5a705514d70b0 — closing.
kayg closed this pull request 2026-11-07 17:53:18 +00:00
Some checks failed
CI / server (pull_request) Has been cancelled
Details
CI / rust (pull_request) Has been cancelled
Details
CI / web (pull_request) Has been cancelled
Details

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
backend
Rust server

  
design
Mockups, UI and TASTE

  
docs
ADRs, CONTEXT, TASTE

  
idea
Parked idea, never worked until promoted

  
infra
Deploy, CI, VMs

  
perf
Performance and budgets

  
reliability
Data integrity, corruption traps

  
web
Web client

No labels
backend
design
docs
idea
infra
perf
reliability
web
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
kayg/calternal!829
No description provided.