Skip to content

fix: never re-advertise issued OTKs to the same peer (#49) - #58

Open
chtnnh wants to merge 5 commits into
ni5arga:mainfrom
chtnnh:fix/49-no-otk-reshare
Open

fix: never re-advertise issued OTKs to the same peer (#49)#58
chtnnh wants to merge 5 commits into
ni5arga:mainfrom
chtnnh:fix/49-no-otk-reshare

Conversation

@chtnnh

@chtnnh chtnnh commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Summary

Test plan

  • npm test / npm run typecheck
  • New test: replenish after take yields a different OTK; both messages open

Re-listing an unconsumed issued OTK let a peer seal twice to one public;
the second message died after consume. Mint fresh OTKs on replenish only.

Co-authored-by: Cursor <cursoragent@cursor.com>

@commitchan commitchan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The fix is correct and I am approving it. Holding the merge button on a sequencing issue, not on this diff — it should land immediately after #59, and I have explained there why.

The fix

#49 is real and the diagnosis is exact. absorb() rebuilds its known set from row.otks, and takeAgreementPublic() shifts the taken public out of that array — so a re-advertised OTK is no longer "known" to the peer's book and gets re-queued. Take it twice, seal twice to the same public, and the second ciphertext is dead the moment we consume the secret on opening the first. Deleting the re-share loop is the right fix: an OTK's whole contract is one seal, and the only way to keep that contract without a per-peer take-ledger we do not have is to never list it twice.

Verified locally on a0e6dc9: merges clean, 230/230 pass, typecheck and lint clean. The regression test is the audit PoC and it fails on main as expected.

Why it should not go first

Removing the re-share loop changes the steady-state size of the OTK pool, and that interacts with the secretsForOpen() slice that #59 fixes. Measured on a local merge of this PR:

# bundle OTK counts across six unanswered sends
main:      8,8,8,8,8,8   pool=16
this PR:   8,8,8,0,0,0   pool=24

The pool now pins at OTK_POOL_CEILING. On today's secretsForOpen() that means 24 OTK secrets consume 24 of the 28 slots, leaving 4 for a 6-key SPK ring — so mail sealed to an SPK older than ~4h stops opening, inside the 6h window we document as retained:

pool=24  spkRing=6  secretsTried=28  agedSpkOpens=false

That is #50, and it is currently latent on main only because the re-share loop keeps the pool at 16. This PR is what makes it fire. Nothing here is wrong — #59 is the fix — they just need to land in that order. Reproduce with a scratch test if you want:

const local = new LocalPrekeys();
local.ensureReady();
for (let i = 0; i < 6; i++) console.log(local.updateForPeer(bob, 'peer', 8).oneTimePublics.length);

Separate follow-up: the zeroes in that sequence

The 0,0,0 tail is worth a tracking issue on its own, independent of #59.

Once the pool is at the ceiling and every key in it is issued-but-unconsumed, issueOtks has nothing to hand out: there are no issuedTo === null rows, and the mint loop is gated on this.otks.size < OTK_POOL_CEILING. Every subsequent replenishment carries zero OTKs until something is consumed or the 6h sweep fires. That happens after only three unanswered messages to one peer.

The consequence is a graceful degradation, not a loss — the peer's book empties, takeAgreementPublic falls through to the SPK, and the mail delivers. So it is not a merge blocker and I do not want it fixed in this PR. But it means per-message FS quietly downgrades to per-rotation FS for any peer who does not reply, and on a lossy mesh that is the common case, not the edge case: an OTK secret is only consumed when we open something sealed to it, so every message that never arrives leaves a key pinned in the pool forever.

Worth considering later: evicting the oldest issued-unconsumed OTKs when the ceiling is hit, rather than starving the bundle. @chtnnh if you want to file that under #4 I will pick it up in review; please do not expand this PR.

Approving now, merging as soon as #59 is rebased and in.

@chtnnh

chtnnh commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Filed the issued-unconsumed OTK eviction follow-up as #68 (kept out of this PR as requested).

@chtnnh chtnnh closed this Jul 23, 2026
@chtnnh
chtnnh deleted the fix/49-no-otk-reshare branch July 23, 2026 11:42
@chtnnh
chtnnh restored the fix/49-no-otk-reshare branch July 23, 2026 11:49
@chtnnh chtnnh reopened this Jul 23, 2026
chtnnh and others added 4 commits July 23, 2026 15:54
CI's `npx expo install --check` started failing after upstream SDK 57
patch bumps (expo ~57.0.8 and related packages).

Co-authored-by: Cursor <cursoragent@cursor.com>
Lock @napi-rs/wasm-runtime's @emnapi/core and @emnapi/runtime peers at
the top level so CI's npm ci does not demand an unlocked 1.11.2.

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
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.

[High][FS] In-band OTK re-share allows double-seal then silent loss

2 participants