Repository navigation
feat(mobile): back up nsec to org builderlab account - #9029
Conversation
…account The Block tenant binds a member's key on its first relay assertion, but mobile never uploaded a backup of that key, so turning on Block backups would leave mobile-only keys unrecoverable. After each organization assertion, read the account's backup status and, when the bound key is not listed, enroll, seal and upload it through HostedBackupService, now scoped to its account client's origin. A tenant without backups (404) or a definitive refusal stops attempts for that account and key until restart; other failures retry at the next assertion. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Tom Brow <tomb@squareup.com>
…ures - Pass the already-derived pubkey from assertion issuance, so the backup never re-parses the key on the connection path. - Treat a failing native sealing check and identity errors as recorded failures instead of uncaught async errors. - Stop retrying a key after three consecutive failures in a launch: a relay refusal re-issues at once, so retries need their own end. - Hold the backup in its own provider, so a relay-auth rebuild cannot start a duplicate attempt. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Tom Brow <tomb@squareup.com>
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 783914fdbc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Three transient failures settled the key for the rest of the launch, so a long-running app stopped retrying until restart. Retries now wait a delay that doubles from one minute up to one hour, and continue at later assertions until the tenant lists the key. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Tom Brow <tomb@squareup.com>
|
@codex review |
🔐 Codex Security Review
Review SummaryOverall Risk: NONE
FindingsNo concrete security, correctness, or reliability findings were identified. Notes
Generated by Codex Security Review | |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.
No blockers at fe6209a698. Four of us reviewed it, covering integration, persistence, security/protocol and retry/tests. The key, origin and subject checks hold. Hosted pending-envelope keys and receipts stay byte-compatible, so nothing needs migrating. Unknown upload outcomes replay the same envelope. Every throw path lands in one of the four caught failure types, so the unawaited hook can't raise an unhandled error or affect the relay connection.
The three inline notes are optional.
Not verified here: native HPKE on device, and whether the tenant rejects an envelope for another account. Both were source-only; we made no tenant requests.
Product call: the PR notes that desktop asks before backing up while mobile does it automatically. That's for the humans to decide, not a code finding.
| current = 'f' * 64; | ||
| paths.clear(); | ||
| var delay = OrganizationKeyBackup.firstRetryDelay; | ||
| for (var i = 0; i < 10; i++) { |
There was a problem hiding this comment.
🤖 Optional (test strength): this loop advances now by at least the scheduled delay, so it only proves the delay is no longer than the schedule. Mutation check: a constant 1-minute backoff (firstRetryDelay * 1) passes all six tests, and so does a 30-minute cap. To pin both growth and the 1h cap, in each round connect at delay - 1s and expect no status request, then connect at delay and expect one. Run enough rounds to reach the cap.
There was a problem hiding this comment.
🤖 Agreed. Addressed in 9f3810c64d: each round now connects a second before its delay (expect no status request) and exactly at it (expect one), for 9 rounds that reach the 1h cap. A constant 1-minute backoff and a 30-minute cap both fail it now.
| final BackupIdentity identity; | ||
| final HpkeBackup _hpke; | ||
| final FlutterSecureStorage _storage; | ||
| static final _queue = SerialQueue(); |
There was a problem hiding this comment.
🤖 Optional: _queue is static, so an organization backup that stalls on enrollment or upload now blocks the hosted backup that onboarding waits on (hosted_flow.dart:629). Each request can take up to its 30s bound, and the rejected→re-enroll path makes several. Pending keys are already scoped by origin and the organization receipt is in memory, so keying the queue by account.accountOrigin.origin removes the cross-account wait without losing serialization. Separately, organization backup requests also take turns in the organization client's _serial queue. A sign-out started during an in-flight upload waited for it to finish in a local probe. That's acceptable if it's intended, but worth a comment.
There was a problem hiding this comment.
🤖 Agreed. Addressed in 32b31c7784: the queue is keyed by account origin, with a test where a stalled organization enrollment no longer blocks a hosted backup (it times out with the shared queue). The sign-out wait behind an in-flight organization request is intended, and is now noted in a comment in c6474c4fae.
| _CommunityKey(nsec, pubkey), | ||
| hpke: _hpke, | ||
| storage: _storage, | ||
| ).backup(isCurrent: () => account.account?.subject == subject); |
There was a problem hiding this comment.
🤖 Optional hardening: isCurrent here repeats the service's own checkAccount(), and removing it fails no test. Neither check covers the gap between checkAccount() and when the queued enrollment/upload account.post actually runs, because those calls pass no expectedSubject. We couldn't show this is reachable today, since OrganizationSignIn._begin awaits clearLocalSession() before starting login. Still, passing expectedSubject: subject on both posts in HostedBackupService would close it at the API boundary for hosted and organization alike.
There was a problem hiding this comment.
🤖 Agreed. Addressed in c6474c4fae: enrollment and upload pass expectedSubject: subject, and the redundant isCurrent from the organization backup is removed.
Each retry is checked a second before and exactly at its delay, so the test fails if the delay stops doubling or the one-hour cap changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Tom Brow <tomb@squareup.com>
A stalled organization backup no longer holds up the hosted backup that onboarding waits on. Pending envelopes were already scoped by origin. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Tom Brow <tomb@squareup.com>
Enrollment and upload now pass the expected subject, so a session that changes between the service's checks and the queued request is refused at the API boundary. The organization backup's own subject check was redundant and is removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Tom Brow <tomb@squareup.com>
…ge (#9085) ## Summary [#8965 native key derivation](#8965) removed `AccountIdentityStore._keys`. `keysForNsec`, added by [#9029 nsec backup](#9029), which merged first, still called it. The two PRs didn't conflict textually, so `main` now fails `flutter analyze` (`_keys` isn't defined at `account_identity_store.dart:356`) and mobile CI is red. This builds the keys through `keysForHex(_nsecHex(nsec))`, the same path `withKeys` uses since #8965. It applies the same bech32 decoding and secp256k1 scalar validation that `_keys` did. ## Testing - `just mobile-check`: no issues found. - `flutter test` (full mobile suite): 4460 passed, 49 skipped. No UI change, so no app run. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Tom Brow <tomb@squareup.com> Co-authored-by: Tom Brow <tomb@squareup.com> Co-authored-by: studio <dc2fc481343672df31a689840c09299972e704dac592e0da6f18486646a7ee3e@buzz.block.builderlab.xyz> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…9225, #9228 to 0.27) (#9232) Backport to `mobile-v0.27` of the mobile fixes that let iOS release builds turn on hosted NIP-FI (`BUZZ_HOSTED_RELAY_AUTH`) for the Nov 10 launch. [squareup/buzz-releases#142](squareup/buzz-releases#142) turns the flag on for 0.27.1 candidates after this lands. ## Picks Each is `git cherry-pick -x --signoff` of the squash commit on main, in this order: - #9056 (6b8a72e): send hosted relay auth with iOS custom emoji downloads. - #9069 (97ede70): send the hosted relay assertion from native media and tombstone paths. - #9134 (1b9b307): renew expired assertions for native media and drop retired-session avatars. - #9225 (b58ee08): request a hosted relay assertion only when the relay's NIP-11 document requires NIP-FI. Without it, the flag-on build is denied by every hosted community that hasn't enabled NIP-FI. - #9228 (794ee64): the notification extension mints hosted assertions after a relay's NIP-FI challenge, so hosted pushes keep their text once a community enforces. One 0.27-only commit, `test(mobile): drop org key backup checks from the #9069 backport`: 0.27 has no organization key backup (#9029), so the backported organization relay tests keep only their assertion and cache checks. No production code differs from main's versions. All picks applied without conflicts. ## Risk Hosted assertions are sent only when the build sets `BUZZ_HOSTED_RELAY_AUTH`, which current App Store builds don't. With the flag off, the one new behavior from #9228 is clearing a hosted session an earlier build might have shared with the notification extension. Organization relays keep their issuer. Each PR's description on main covers its own testing. ## Testing - `dart format --set-exit-if-changed`, `flutter analyze`: clean. - Full `flutter test` with CI's `BUZZ_PUSH_GATEWAY_URL` define: all pass (4,386). - CI's configured-auth suite (`BUZZ_AUTH_DOMAINS=buzz.xyz,builderlab.xyz,builderlabstaging.xyz`) plus `test/shared/hosted`, `test/features/onboarding` and `test/shared/verified_profiles`: all pass (355). - `swift test` in `ios/BuzzPushKit`: all pass (110 XCTest, 70 Swift Testing). - Automated app run: an iOS simulator build of this branch with the App Store defines plus `BUZZ_HOSTED_RELAY_AUTH=true`, installed over a signed-in hosted account. The hosted community connects with no denial banner, the channel list loads, and a channel's history, avatars and media load. The notification extension can't be exercised on the simulator; BuzzPushKit tests cover it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Tom Brow <tomb@squareup.com>
The Block tenant links a member's existing key to their account the first time it issues them a relay assertion (#8700). Mobile never backed that key up. Desktop FI builds back up to the tenant, but on mobile a Block key that lives on just one phone can't be recovered once Block turns backups on.
Mobile now backs up the organization relay's key to the organization account:
/v1/buzz/nsec-backups/status. If the key is already listed, there's nothing to do.HostedBackupService. That service now uses its account client's own origin, so organization and hosted backups never share a pending envelope. Hosted storage keys and receipts don't change.There's no UI. As in mobile's hosted onboarding, the backup happens automatically. Desktop asks for an explicit backup choice first, so whether mobile should too is a product call.
Validation
Added tests (
organization_key_backup_test.dart), which run through realHostedRelayAuthassertion issuance with only HTTP faked:Removing the assertion hook fails 4 of the 6 tests. Removing the shared attempt or the retry delay fails its test. Full
mobile-checkandmobile-testpass.Device (iPhone 12 mini, release build) against bb-block staging, where backups are configured and the relay enforces NIP-FI, with an existing Block SSO session:
The device's staging community was added in an earlier run, because adding an enforcing organization relay by link is still blocked (#8791).
An independent agent review ran; its should-fix items are addressed in the second commit. Codex's retry finding is addressed in the third, which changes only failure-path timing; the device-tested success path is unchanged. Wes's optional review notes are addressed in the last three: an exact-timing retry test, backup queues per account origin (a stalled organization backup no longer delays hosted onboarding's backup, with a test), and a subject check on each enrollment and upload request. Full
mobile-checkandmobile-testpass at the head. Human testing is waived under the mobile automated-testing policy; the device run above covers the at-risk flows (organization assertion, relay connection, backup status, enrollment, sealing and upload).buzz-review-completed
🤖 Generated with Claude Code