Skip to content

fix: reject OTKs on QR contact-code decode (#51) - #60

Open
chtnnh wants to merge 5 commits into
ni5arga:mainfrom
chtnnh:fix/51-qr-reject-otks
Open

fix: reject OTKs on QR contact-code decode (#51)#60
chtnnh wants to merge 5 commits into
ni5arga:mainfrom
chtnnh:fix/51-qr-reject-otks

Conversation

@chtnnh

@chtnnh chtnnh commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Summary

Test plan

  • npm test / npm run typecheck
  • Hand-crafted n=3 QR payload rejected; encoder throws on OTK bundles

SPK-only was an encoder policy; parsers still accepted OTK-bearing plaques
and reintroduced multi-scan silent loss. Encode throws and decode returns
null when n !== 0.

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

Copy link
Copy Markdown
Collaborator

Reviewed the substance and I agree with it. Holding merge only because the branch is conflictingmain has moved (#48, #63/#64/#65/#66). Please rebase onto latest main and force-push. Also note #59 touches the same two files (prekeys.ts, forward-secrecy.test.ts) and the same docs/FORWARD-SECRECY.md table, so land order matters: #59#58 → this one, and this is the one to rebase last.

The fix

Making SPK-only a wire rule rather than an encoder policy is the right instinct, and the distinction is the whole point of the issue. bundleForQr already passed QR_OTK_COUNT = 0, so we were never generating OTK-bearing plaques — but the decoder happily parsed one, which means the property held only as long as every producer in the wild was our current build. It is not a property until the parser enforces it.

Worth being precise about what this does and does not buy, since the issue title reads scarier than the actual threat:

  • It does not stop key substitution. [High][FS] Unsigned OTKs can redirect decryption to a third party #48 already binds the OTK list into the SPK signature, so an attacker cannot graft OTKs onto someone else's plaque — verifyReceiveKey rejects it.
  • What it stops is a plaque whose owner legitimately signs OTKs into it: an older or modified client, or someone who simply does not know better. Every person who scans that plaque seals their first message to the same OTK, and everyone after the first gets consume-on-open silent loss with no error surface anywhere. That is the failure mode a QR uniquely produces, because a QR is inherently multi-scan, and it is exactly the reasoning already written at the top of contact-code.ts. This makes the code enforce the comment.

Backward compatibility checked: every v2 code we have ever emitted has n = 0, so the stricter decoder rejects nothing that exists in the field.

One observation, not a change request

encodeBundle now throws, and encodeContactCode does not catch it, and the only caller is app-state.tsx:123 on the identity-setup path. That is unreachable today because bundleForQr hardcodes an empty list — but it does mean a future caller who passes a peer bundle takes down the QR screen rather than degrading.

I think throwing is the correct choice here and I would not change it: silently emitting a plaque that will eat other people's messages is strictly worse than a crash we would notice in testing. Just flagging that the safety of it rests on bundleForQr staying SPK-only, which is now asserted in three places (QR_OTK_COUNT, the encoder guard, the decoder guard) — that redundancy is the good kind.

Test note

The hand-crafted n=3 payload is the right test to write — it proves the wire rule rather than the encoder policy, and it is the only way to cover a producer we do not control. Good that it asserts through decodeContactCode too and not just decodeBundle.

@chtnnh chtnnh closed this Jul 23, 2026
@chtnnh
chtnnh deleted the fix/51-qr-reject-otks branch July 23, 2026 11:42
@chtnnh
chtnnh restored the fix/51-qr-reject-otks branch July 23, 2026 11:49
@chtnnh chtnnh reopened this Jul 23, 2026
chtnnh and others added 4 commits July 23, 2026 15:56
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>
@chtnnh
chtnnh force-pushed the fix/51-qr-reject-otks branch from 5c14435 to 1d2f720 Compare July 23, 2026 12:07
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.

[Medium][FS] Contact-code parser still accepts OTKs on QR intros

2 participants