fix(mobile/ios): secure push relay resolution#2732
Conversation
… vectors Add tests/vectors/app_attest_transcripts.json — deterministic known-answer vectors (exact transcript bytes + SHA-256) for all five NIP-PL App Attest operations: enroll, delegate, rotate_endpoint, revoke_delegation, revoke_installation — generated by the gateway's own transcript() encoder, plus in-crate tests asserting the encoder reproduces the fixture byte-for-byte. The fixture is shared ground truth for client-side canonical encoders (the Swift NIP-PL iOS client): a client that fails to reproduce these bytes exactly fails every attested call with invalid_attestation and no diagnostic (Swift JSONSerialization's \/ escaping is the known trap). Any future change to the Rust encoder now breaks a test instead of silently stranding clients. Co-authored-by: Tyler Longwell <tlongwell@block.xyz> Signed-off-by: Tyler Longwell <tlongwell@block.xyz>
BuzzPushKit SwiftPM package with a hand-rolled ordered-compact-JSON transcript encoder for all five App Attest domains (enroll / delegate / rotate-endpoint / revoke-delegation / revoke-installation), verified byte-for-byte + SHA-256 against the gateway-generated known-answer vectors in crates/buzz-push-gateway/tests/vectors/app_attest_transcripts.json. JSONSerialization is deliberately not used anywhere near these bytes: it escapes '/' as '\/' and does not guarantee member order, either of which silently strands every iOS client with 401 invalid_attestation. Non-ASCII input fails closed per the NIP-PL ASCII profile. The 'audience' member is a fixed NIP-PL protocol constant (https://push.buzz.xyz/v1/...) hardcoded by the gateway for cross-route domain separation — clients must never derive it from a discovered gateway base URL or relay host. Co-authored-by: Tyler Longwell <tlongwell@block.xyz> Signed-off-by: Tyler Longwell <tlongwell@block.xyz>
Co-authored-by: npub12gtutshhh76rx0jx697f32f9tffd4hhp3hx58fp4x6u4uemkm7sqf8f757 <5217c5c2f7bfb4333e46d17c98a9255a52dadee18dcd43a43536b95e6776dfa0@sprout-oss.stage.blox.sqprod.co> Signed-off-by: npub12gtutshhh76rx0jx697f32f9tffd4hhp3hx58fp4x6u4uemkm7sqf8f757 <5217c5c2f7bfb4333e46d17c98a9255a52dadee18dcd43a43536b95e6776dfa0@sprout-oss.stage.blox.sqprod.co>
Co-authored-by: npub102wg7q285p64ch2fjvstmf2ntn2sz3c4u5hmwatalc76mhsuauysftjtfj <7a9c8f0147a0755c5d499320bda5535cd5014715e52fb7757dfe3dadde1cef09@buzz.block.builderlab.xyz> Signed-off-by: npub102wg7q285p64ch2fjvstmf2ntn2sz3c4u5hmwatalc76mhsuauysftjtfj <7a9c8f0147a0755c5d499320bda5535cd5014715e52fb7757dfe3dadde1cef09@buzz.block.builderlab.xyz>
Co-authored-by: npub102wg7q285p64ch2fjvstmf2ntn2sz3c4u5hmwatalc76mhsuauysftjtfj <7a9c8f0147a0755c5d499320bda5535cd5014715e52fb7757dfe3dadde1cef09@buzz.block.builderlab.xyz> Signed-off-by: npub102wg7q285p64ch2fjvstmf2ntn2sz3c4u5hmwatalc76mhsuauysftjtfj <7a9c8f0147a0755c5d499320bda5535cd5014715e52fb7757dfe3dadde1cef09@buzz.block.builderlab.xyz>
Co-authored-by: npub15uvq03jgm52chkxa5x7f2ud28xh9v48287f5fj5fsuu0wqpm63ns8hn2er <a71807c648dd158bd8dda1bc9571aa39ae5654ea3f9344ca898738f7003bd467@buzz.block.builderlab.xyz> Signed-off-by: npub15uvq03jgm52chkxa5x7f2ud28xh9v48287f5fj5fsuu0wqpm63ns8hn2er <a71807c648dd158bd8dda1bc9571aa39ae5654ea3f9344ca898738f7003bd467@buzz.block.builderlab.xyz>
brow
left a comment
There was a problem hiding this comment.
🤖 Independent review of the delta above #1924's base (e4b1e81) at head e067a39.
What I verified locally at this exact head: BuzzPushKit swift test 14 passed; full flutter test 549 passed, 1 skipped; flutter analyze clean; the merge commit 14a529d resolves the project.pbxproj conflict correctly (keeps both MediaSanitizer.swift and PushNativeState.swift, verified against git merge-tree). I also checked the NIP-01 canonical serialization: JSONSerialization with .withoutEscapingSlashes produces the required escaping (raw UTF-8, \uXXXX control chars, unescaped slashes) for the tag/content shapes used here, and the NIP-98 signing/verification round-trip is covered by tests.
Two should-fix findings inline: an auth lockout path introduced by making the snapshot export load-bearing in AuthNotifier.build(), and NSE watermark poisoning via attacker-controlled created_at.
Nits not worth threads: the cross-community newest-candidate tie-break prefers the larger event id while the per-community pick prefers the smaller; watermark keys for removed communities are never cleaned from app-group defaults; and the auth paths plus the communityListProvider invalidation they trigger produce redundant back-to-back snapshot/Keychain writes.
Co-authored-by: npub15uvq03jgm52chkxa5x7f2ud28xh9v48287f5fj5fsuu0wqpm63ns8hn2er <a71807c648dd158bd8dda1bc9571aa39ae5654ea3f9344ca898738f7003bd467@buzz.block.builderlab.xyz> Signed-off-by: npub15uvq03jgm52chkxa5x7f2ud28xh9v48287f5fj5fsuu0wqpm63ns8hn2er <a71807c648dd158bd8dda1bc9571aa39ae5654ea3f9344ca898738f7003bd467@buzz.block.builderlab.xyz>
brow
left a comment
There was a problem hiding this comment.
🤖 Re-reviewed the fix commit 8912fdc (delta over e067a39). Both should-fix findings from my previous review are correctly addressed, and all three nits were fixed as well:
- Auth lockout: every snapshot export now routes through
syncCommunitySnapshot, which catches failures, records them inpushCommunitySnapshotError, and never fails the auth path. The dedup fingerprint only advances on successful writes, so failed exports stay retryable. Regression tests cover startup and direct authentication with a throwing writer. - Watermark poisoning: persisted watermarks are clamped to now + 300s, out-of-skew events are rejected from resolution, already-poisoned stored values self-repair to now before querying, and
sinceis now inclusive. Covered by the newPushWatermarkTests. - Nits: cross-community tie-break now picks the smaller event id on equal
created_at, matching the per-community sort; stale watermark keys are pruned against the current snapshot (prefix-guarded against unrelated defaults keys); redundant snapshot writes are deduplicated across provider invalidation, with a test.
Verified locally at this exact head: BuzzPushKit swift test 20 passed; full flutter test 552 passed, 1 skipped; flutter analyze clean. CI at this head is still running; my approval is based on local verification.
|
Closing: opened from the wrong GitHub account. Reopening from the correct account. |
brow
left a comment
There was a problem hiding this comment.
🤖 Independent review at 8912fdc (cumulative delta e4b1e81..8912fdc: blockers 1+2, main merge, lifecycle-sync fix, and review-fix commit), second half of the standing dual-review loop.
Blockers-1/2 core (4c99a83 + merge 14a529d):
- NIP-98 client matches the relay contract: kind 27235,
u/method/payloadtags, canonical NIP-01 serialization (JSONSerialization+.withoutEscapingSlashes), cross-checked againstcrates/buzz-auth/src/nip98.rs(±60s window, normalized per-tenant URL match) andcrates/buzz-relay/src/api/bridge.rs/queryenforcement (auth + replay + membership + p-gated filters). Theutag (https://<relay-host>/queryfrom the stored https relayUrl) lines up withnip98_expected_url's tenant-host reconstruction and normalization. - Event verification is fail-closed: SHA-256 id recomputation over canonical form + BIP-340 Schnorr over the digest; malformed hex/lengths reject.
- Keychain transfer is symmetric: Runner writes
buzz.push.nse.signingwith the access group from Info.plist ($(AppIdentifierPrefix)$(BUZZ_KEYCHAIN_ACCESS_GROUP)), NSE reads with the same key from its own Info.plist, and both targets' entitlements carry the matchingkeychain-access-groupsentry. Fail-closed on partial writes (delete-all on SecItemAdd failure). - Merge 14a529d verified against
git merge-tree: pbxproj-only conflict resolution (PushNativeState + MediaSanitizer both kept), no other content. - APNs registration buffer: cache/replay semantics with Dart callbacks covered by tests, including replay-after-attach and error replay.
Review-fix commit 8912fdc (both prior findings addressed):
- Auth lockout: every snapshot export routes through
syncCommunitySnapshot— best-effort, observable (pushCommunitySnapshotError+ debug log), never gatesauthProvider. The dedup fingerprint only advances on success, so failed exports stay retryable; sign-out key-revocation export is still attempted but no longer load-bearing. Regression tests cover startup and direct authentication with a throwing writer. - Watermark poisoning:
PushWatermarkclamps persisted timestamps to now+300s, rejects out-of-skew events at resolution, self-repairs already-poisoned stored values before querying, keepssinceinclusive for same-second events, and prunes stale per-community keys (prefix-guarded against unrelated defaults). Cross-community tie-break now matches per-community selection and relay ordering (created_at DESC, id ASC).
Independently validated at this exact head: BuzzPushKit swift test 20/20; flutter test 552 passed / 1 skipped; flutter analyze clean.
Non-blocking: the BuzzPushKit suite does not run in CI — the Mobile job is ubuntu-only and the package needs CryptoKit/Security — so the new watermark/auth-header tests are local-only signal. A small macOS CI step running swift test in mobile/ios/BuzzPushKit would close that gap as a follow-up. One inline nit on the snapshot fingerprint.
Approving.
| community.nsec, | ||
| ].join('\u0000'), | ||
| ) | ||
| .join('\u0001'); |
There was a problem hiding this comment.
🤖 Non-blocking: this fingerprint concatenates every community's nsec and is retained for the provider's lifetime in _lastSuccessfulSnapshot. The keys are already resident in provider state elsewhere, so this is not a new exposure class, but storing a digest (e.g. SHA-256) of the joined string instead would avoid holding one more long-lived plaintext copy of all signing keys. Fine as a follow-up.
Summary
Builds on #1924 and addresses review blockers 1 and 2 for the NIP-PL iOS push notification foundation.
Blocker 1: authenticated and verified NSE relay resolution
/queryrequests with NIP-98Blocker 2: APNs callback replay
ValueNotifierstateTests
swift test: 14 passed../bin/flutter test: 487 passed, 1 skippedgit diff --check: cleanStacking note
This branch is one commit on top of the exact #1924 head used during implementation (
e4b1e81312f57edb43fbcf1bfa5fb26eec8aef2e). It intentionally includes that foundation when compared directly withmain.