fix(security): strip BYOK nsec from Keycast OAuth wire (#3359)

[realmeylisdev]

Description

Phase 1 emergency block for #3359 — strips the user's nsec from the
Keycast OAuth registration wire on both channels:

• body['nsec'] field in POST /api/headless/register
• <random>.<nsec1...> suffix embedded in the PKCE code_verifier
  (later POSTed as code_verifier to /api/oauth/token)

The fix removes the parameters from the keycast_flutter package API
entirely so a future caller cannot re-introduce the leak. A POSIX-grep
CI guard locks the rule in.

Related Issue: Closes #3359

Cross-repo coordination: divinevideo/keycast#197 tracks the
server-side proof-of-possession redesign that Phase 2 depends on.
This PR ships without preserving BYOK identity binding — that is
restored once Phase 2 lands.

Type of Change

• 🛠️ Bug fix (non-breaking change which fixes an issue)
• ❌ Breaking change (the public KeycastOAuth.headlessRegister and KeycastOAuth.getAuthorizationUrl APIs no longer accept nsec — intentional, for security)

Trade-off

Until Phase 2 ships, BYOK secure-account upgrades fall through to the
server's auto-generate path. The user's existing local pubkey identity
is not preserved across the upgrade, but the local nsec stays
on-device and never crosses the network. This is the intentional
emergency-block trade-off — stop the leak today, restore identity
preservation in Phase 2 (depends on divinevideo/keycast#197).

What's in this PR

File	Change
mobile/packages/keycast_flutter/lib/src/oauth/pkce.dart	generateVerifier no longer takes nsec; verifier is always pure-random base64url
mobile/packages/keycast_flutter/lib/src/oauth/oauth_client.dart	getAuthorizationUrl & headlessRegister drop nsec/byokPubkey parameters; body['nsec'] removed; byok_pubkey URL param removed
mobile/packages/keycast_flutter/test/{pkce,oauth_client}_test.dart	Leak-as-feature tests flipped to negative regression guards
mobile/lib/services/pending_verification_service.dart	load() discards any persisted verifier containing .nsec1 and clears storage (defense-in-depth migration)
mobile/lib/blocs/email_verification/email_verification_cubit.dart	Error log no longer interpolates _pendingVerifier value
mobile/lib/screens/auth/secure_account_screen.dart	Removed authService.exportNsec() and the nsec: arg
mobile/test/services/pending_verification_service_test.dart	New — three tests for the migration
mobile/tools/ci/grep_for_nsec_payload.sh	New — POSIX-grep guard for production code
.github/workflows/mobile_ci.yaml	New nsec-leak-guard job
mobile/integration_test/auth/session_expired_banner_test.dart	markTestSkipped until Phase 2 — depends on BYOK preservation

Test plan

• flutter analyze lib test integration_test packages/keycast_flutter/{lib,test} — no issues
• flutter test packages/keycast_flutter/ — 167/167 passing
• flutter test test/services/pending_verification_service_test.dart test/screens/auth/secure_account_screen_test.dart test/blocs/email_verification/email_verification_cubit_test.dart — 38/38 passing
• flutter test test/blocs/divine_auth/ — 64/64 passing (regression-safety on the path that never carried nsec)
• bash mobile/tools/ci/grep_for_nsec_payload.sh — exits 0 on this branch
• Guard regex verified: matches body['nsec'] + 'nsec':; ignores containsKey('nsec') + any(named: 'nsec')
• Manual: e2e secure-account upgrade flow on a local dev stack (post-merge or staging)
• Coordinate with infra/security on retention review for any pre-fix request bodies that infra-layer components may have captured (separate workstream)

Follow-ups (not in this PR)

• feat(byok): add proof-of-possession registration to support divine-mobile#3359 keycast#197 — server-side proof-of-possession contract (Phase 2A)
• A separate Phase 2 mobile PR — restores BYOK identity preservation via the new server contract (depends on keycast#197 being in production)
• Orphaned ARB key authUnableToAccessKeys (no longer referenced from Dart) — left in place to avoid touching 16 locale files; safe to remove in a follow-up

[dcadenas]

Moving this to draft so it does not merge while we discuss the product implications in the issue.

This changes the current Keycast BYOK contract and breaks the secure-account flow's "backup your key" behavior.

I left more context here: #3359 (comment)

Let's settle that first.

[NotThatKindOfDrLiz]

@realmeylisdev @dcadenas I took a fresh pass on #3784 and on the current main code path.

My read is:

• the security bug is real and still present on main
• this PR does actually remove the leak channels
• but it also intentionally changes the current secure-account / BYOK contract by no longer preserving the user’s existing key identity
• under our normal no tech debt bar, that makes this hard to merge as-is, because it is explicitly a Phase 1 stopgap that depends on a later Phase 2 to restore the behavior

So I don’t think the right outcomes here are either merge this now as normal or close it as unnecessary.

I think we need one explicit decision in #3359:

1. Emergency security exception: accept the temporary BYOK regression in order to stop the nsec leak immediately, and merge this knowingly as a security stopgap.
2. No-tech-debt path: keep this in draft and only merge once we have the coordinated mobile/server design that removes the leak without breaking secure-account identity preservation.

If it would help move this forward, @rabble and I can help turn that into a concrete go/no-go decision in #3359, but I don’t think this PR should just sit in ambiguous draft state.

[rabble]

Draft review notes

The core security fix is directionally strong: removing nsec from the Keycast OAuth API surface and no longer embedding nsec1... inside PKCE code_verifier closes the dangerous leak path.

Top risks before marking ready

• Skipped integration test / dead-code workaround needs explicit acceptance. The skipped BYOK identity-preservation integration test has issue references, but repo rules strongly discourage skipped tests and dead code. For an emergency security PR this may be acceptable temporarily, but I would not call it ready until the team explicitly accepts the exception or replaces the assertion with a Phase-1-compatible path.

• Product impact needs verification. Secure-account upgrade no longer preserves the user’s existing local pubkey identity until Phase 2. That may be acceptable for the security fix, but the UX/product impact should be verified and clearly accepted.

• Pending verifier migration detection is narrow. verifier.contains('.nsec1') covers the documented legacy shape. If any persisted value used a variant format, it would not be cleared. Probably acceptable if the legacy format is known and fixed.

• PII in warning log. PendingVerificationService.load() logs the email address when discarding pre-fix pending verification. Safer to omit or redact the email unless auth email logging is already explicitly accepted.

• CI guard dependency churn. The workflow installs ripgrep, but if the guard script is POSIX grep-based and does not use rg, this adds unnecessary CI setup.

Readiness

Keep draft until manual/staging secure-account upgrade behavior is verified or explicitly waived, the skipped-test exception is resolved/accepted, and the PII/logging nit is addressed.

[realmeylisdev]

Thanks for the review @rabble. Items (1), (3), and (5) are addressed in b0cb1ee:

• (1) Skipped test / dead-code workaround — replaced the markTestSkipped + return + // ignore: dead_code shape with a markTestSkipped-only body, deleting the 180-line unreachable section. The pre-skip body is preserved in commit feb110b and can be restored when Phase 2 lands. Eliminates the // ignore: dead_code lint suppression and the now-unused imports (−195 lines).
• (3) Narrow .nsec1 match — broadened the migration guard from verifier.contains('.nsec1') to verifier.contains('nsec1'). Random base64url verifiers contain the 5-char literal nsec1 only at ~1e-7 probability, so the false-positive cost is negligible against the one-time-only migration value. Added a test case for the bare-prefix shape.
• (5) Ripgrep install step — removed; verified grep_for_nsec_payload.sh uses POSIX find | grep -E only and contains no rg invocation. Saves ~5–10s of CI setup per run.

Two open items below.

---

Product impact needs verification. Secure-account upgrade no longer preserves the user's existing local pubkey identity until Phase 2. That may be acceptable for the security fix, but the UX/product impact should be verified and clearly accepted.

Confirmed from an end-to-end trace:

• secure_account_screen.dart:130 calls headlessRegister(email, password, scope) with no nsec.
• The server auto-generates NEW_PUBKEY, surfaced via session.userPubkey.
• auth_service.dart:2974–3008: the local nsec lookup keyed by NEW_PUBKEY fails (the on-device key is for OLD_PUBKEY), so signInWithDivineOAuth falls back to SecureKeyContainer.fromPublicKey(NEW_PUBKEY) → signing routes through Keycast RPC (200–500ms) until Phase 2 lands the BYOK path.
• _setupUserSession → shouldClearDataForUser(NEW_PUBKEY) compares against the stored OLD_PUBKEY and returns true → user-specific cached data (follows, bookmarks, viewing history) is cleared on the upgrade.

Concrete user-visible impact for an anonymous user who upgrades:

• Old local nsec stays on-device but is orphaned for the OAuth account.
• Videos published under OLD_PUBKEY remain on relays but aren't associated with the new authenticated account.
• Cached per-user data is cleared at the identity transition.
• Every sign/decrypt round-trips through Keycast RPC until Phase 2.

The test plan's Manual: e2e secure-account upgrade flow on a local dev stack checkbox is still unchecked. I'll run that on staging before flipping out of draft and capture the result in the PR description; if staging access isn't viable for this iteration, I'll ask for an explicit waiver here citing the security/UX trade-off.

---

PII in warning log. PendingVerificationService.load() logs the email address when discarding pre-fix pending verification. Safer to omit or redact the email unless auth email logging is already explicitly accepted.

Repo-wide audit — email is logged at info/warning level in the same flow today at 5 other sites:

• lib/blocs/divine_auth/divine_auth_cubit.dart:275
• lib/blocs/email_verification/email_verification_cubit.dart:75
• lib/services/pending_verification_service.dart:68 (save)
• lib/services/pending_verification_service.dart:139 (expired/clearing)
• lib/services/pending_verification_service.dart:148 (loaded)

The new warning at line 113 is the same shape as the 4 sibling logs in the same load() function. Redacting only the security-migration line — while leaving the routine info logs that fire on every successful load on the same code path — would create inconsistency without security benefit.

Two coherent options:

1. Leave the line as-is (matches established convention).
2. Systematic redaction across all 6 sites + a new redactEmailForLogs(email) helper colocated with lib/utils/sensitive_uri_for_logs.dart (current redaction utilities). Out of scope for an emergency security patch but happy to file a follow-up issue if you'd prefer that.

Defaulting to (1) unless you'd rather see (2) shipped in this PR.

[realmeylisdev]

PII in warning log. `PendingVerificationService.load()` logs the email address when discarding pre-fix pending verification. Safer to omit or redact the email unless auth email logging is already explicitly accepted.

Filed #4349 for the systematic option-2 path (new `redactEmailForLogs` helper colocated with `lib/utils/sensitive_uri_for_logs.dart`, applied across all 6 audit sites + orphaned ARB key removal + lessons capture).

Defaulting to option 1 in this PR per the existing-convention rationale upthread; the cleanup will land in a small focused follow-up after #3784 merges.

[dcadenas]

@rabble I think we need your direct read before this leaves draft.

Core concern

Proof-of-possession proves control of a pubkey, but it does not give Keycast the key material needed for managed RPC/Nostr signing.
So it can restore account/pubkey binding, but not the current Keycast contract:

“backup my key and sign for me later”

Why binding is a big change

If the replacement is account/pubkey binding to an autogenerated Keycast key, that opens the identity-binding can of worms.
A human can have multiple Nostr keys. That is fine. The issue is when the product wants those keys to behave as one continuous Divine identity.
Example:

• Half of a user’s videos were posted by nsecA.
• The other half were posted by nsecB.
• Nostr clients will see those as different pubkey identities unless they implement our binding/migration rules.
• This is not only for “me”; it applies when viewing everyone else too.
  That implies:
• binding-mapping caches
• profile/follow/mention resolution
• membership and permission resolution
• recovery rules
• backfill/sync behavior
• edge cases when mappings are missing, stale, or conflicting
  If instead we rewrite/backfill old events under nsecB, that becomes a sync/backfill system with its own edge cases.

Questions

1. After Phase 2, should Keycast be able to sign as the user’s original mobile pubkey from a different device using only Keycast email/password login?
2. If yes, how does Keycast get durable signing capability for that original pubkey without receiving the user’s nsec?
3. If no, are we intentionally changing secure-account/BYOK from “backup/import my existing key into Keycast” into “bind my account to a local-only key”?
4. Is the security rule here “nsec must never cross the network,” or only “the current PKCE/request transport is not safe enough”?
5. If nsec must never cross the network, should Keycast also remove/manual-block all other nsec import paths, including paste/import flows?
6. If the fix is only about the current transport, should Phase 2 be a hardened key-import/custody flow instead of proof-of-possession-only binding?
7. Are we okay creating a second autogenerated Keycast pubkey for users who already have a mobile pubkey, knowing that identity continuity then depends on our own binding/migration layer?
8. If the answer to 7 is yes, who owns the identity-migration/aliasing work across clients, caches, memberships, profiles, follows, mentions, permissions, and backfills?

Prior context

This is the identity divergence problem I was trying to call out earlier:
#3359 (comment)

Simpler product alternative

The only practical alternative I see is to explicitly choose a simpler product rule:

• Divine/Keycast account: Keycast owns the managed signing key from the start.
• Local key or external NIP-46/bunker: signing stays local/external.
• No “backup/import my existing local key into Keycast” path.
  But if that is the decision, we should remove/reword all “Backup your key” / secure-account upgrade flows instead of treating Phase 2 as restoring the current contract.

Summary

Proof-of-possession-only Phase 2 may preserve account/pubkey binding, but it does not restore Keycast managed signing/backup.
If we avoid importing the nsec, we either lose managed signing for the user’s existing identity or introduce a second-key/binding/backfill system.
That is a big product and protocol change.

[realmeylisdev]

Proof-of-possession proves control of a pubkey, but it does not give Keycast the key material needed for managed RPC/Nostr signing.

Agreed — that reframing (pubkey-binding ≠ managed signing) is the right one. Phase 2 as currently scoped in #3786 / divinevideo/keycast#197 is PoP-style pubkey-binding, not key custody. For users with an existing local nsec, that does not restore the "backup my key and sign for me later" contract; it changes it.

What I can speak to as PR author:

• Q4 (scope of the rule): this PR operates on the narrower of your two framings — "the current PKCE/request transport is not safe enough". It closes the two known leak channels (body['nsec'] field + <random>.<nsec1...> PKCE-verifier suffix) and removes the package API surface that allowed them. Other nsec import / paste paths aren't touched.
• Q5 (also block paste/import?): out of scope for fix(security): strip BYOK nsec from Keycast OAuth wire (#3359) #3784 — separate decision, neither made nor foreclosed by what's here.
• Status: PR stays in DRAFT. CI is green and the technical fix is done, but I'm not flipping to Ready for Review until the Phase 2 shape question — your three alternatives (PoP-binding + identity-migration/aliasing system, hardened key-import-into-custody flow, or the simpler "no nsec import path at all") — has a direct answer.

Q1, Q2, Q3, Q6, Q7, Q8 are product/protocol-contract calls about what BYOK should be after Phase 2 lands. Deferring to @rabble — agree this needs his direct read before #3784 leaves draft.