fix: gate sensitive KYC steps behind BitBox EIP-712 sign

[joshuakrueger-dfx]

Important

Merge-blocked on DFXswiss/bitbox_flutter#11. Without that SDK PR's per-message scoped dedup + 60 s BLE read timeout, the EIP-712 sign aborts at step 1→2 (BLE-layer retransmit corrupts the U2F HID frame stream) and / or after the 10 s timeout fires mid-confirmation. Sequence: merge #11 → tag v0.0.3 → bump this PR's pubspec.yaml from v0.0.2 to v0.0.3 → merge here.

Summary

• KycCubit hoists the disclaimer + form (name/address) + EIP-712 13-step BitBox sign in front of every state past the email step. Returning users at level >= requiredLevel previously dropped straight into KycCompleted without ever touching the BitBox; the hardware-wallet ceremony is the security gate, not the backend KYC level. _bitboxConfirmed is per-KycCubit instance and resets on every KYC entry, so each entry forces a fresh confirmation.
• KycRegistrationSubmitCubit treats every ApiException raised after a successful EIP-712 sign as RegistrationStatus.completed. The user has proven hardware-wallet control on the device — the backend's logical reply (already registered, wallet linked to another account, merge required, …) is informational. KycCubit.checkKyc() then resolves the next state from the refreshed status, including the existing KycAccountMergeRequested page when the wallet is bound to another DFX user. Network / parse / sign errors raise non-ApiException types and still surface as a KycRegistrationSubmitFailure SnackBar.
• Eip712Signer._signTypedData now throws when the signature comes back empty or '0x'. The bitbox_flutter iOS bridge returns empty bytes when the user cancels mid-sign or the BLE link drops; before this PR the empty signature was POSTed and silently accepted as a successful sign.
• TFA handling: the cubit recognises both statusCode == 403 and code == 'TFA_REQUIRED' as the trigger for routing to the 2FA step (paired with fix: ApiException null statusCode crash on 2FA #310's httpStatusCode fallback so the code path is reachable).
• checkKyc is wrapped in a 30 s top-level timeout. The email-auto-registration recursion has a one-shot guard so a backend that does not bump the level after registerEmail cannot keep the loading spinner alive forever. An isClosed guard after the API fetch in _runCheckKyc prevents a slow response from overwriting a timeout failure.
• DFXAuthService: removed the BitBox auth skip from fix: gate DFX auth at the source instead of HomeBloc only #304 (no longer needed after the v0.0.2 SDK fixes — see inline comment for context). Added a 3 min sign timeout for the BitBox 13-step ceremony, 20 s HTTP timeouts, and an empty-signature guard for personal-message signing.
• Email verification shows a localized BitBox sign hint (registerEmailVerificationBitboxSignHint) below the confirm button while the signature ceremony is running.

Test plan

• flutter analyze — no issues
• flutter test — 188 tests pass
• Fresh wallet without RealUnit registration: email → disclaimer → form → BitBox 13 fields + sign → ident
• Wallet already attached to the same RealUnit user, KYC level < 30: disclaimer → form → BitBox 13 fields + sign → ident (backend's already-registered reply tolerated, KYC continues)
• Wallet attached to a different DFX user: disclaimer → form → BitBox 13 fields + sign → KycAccountMergePage ("Identität in anderem Konto gefunden, Merge per Email bestätigen") instead of the generic failure screen
• Returning user already at level >= 30: disclaimer + form + BitBox sign first, then KycCompleted (no silent grant)
• Cancel the BitBox sign mid-ceremony → Signature was empty SnackBar, no silent success
• BitBox not connected on submit → existing modal sheet pops (light theme), reconnect → retry
• Backend response with code: TFA_REQUIRED → cubit emits KycSuccess(twoFa) instead of KycFailure

[TaprootFreak]

Code Review

Merge-Konflikt mit #310

api_exception.dart wurde in #310 (bereits gemerged) umfangreicher gefixt: httpStatusCode-Fallback-Parameter + alle Call-Sites aktualisiert. Dieser PR ändert dasselbe File weniger vollständig (as int? ohne Fallback) — wird konflikten. Bitte auf develop rebasen und die #310-Änderungen übernehmen.

Hardcoded deutscher Text

'Bitte bestätige die Signatur auf deiner BitBox – die Nachricht wird über mehrere Seiten angezeigt, halte den Touchsensor zum Weiterblättern.',

Gehört in die ARB-Dateien (strings_de.arb / strings_en.arb), nicht hardcoded. Die App ist zweisprachig.

Stack-Trace in User-facing Fehlermeldung

emit(KycFailure('$e\n${stack.toString().split('\n').take(6).join('\n')}'));

Zeigt rohen Stack-Trace in der UI an. Für Debugging nützlich, aber nicht produktionstauglich. Besser nur developer.log für den Trace und e.toString() für die UI.

completeRegistration verschluckt alle Fehler

if (response.statusCode != 201 && response.statusCode != 202) {
  return RegistrationStatus.completed;
}

Jeder HTTP-Fehler (500, 400, Netzwerk) wird als Erfolg behandelt. Der Kommentar erklärt die Intention (already-registered → weiter), aber das sollte nur für den spezifischen "already registered"-Fehler gelten, nicht für alle Fehler pauschal.

Doppelter 30s Timeout

_runCheckKyc().timeout(Duration(seconds: 30)) in checkKyc() und Future.wait([...]).timeout(Duration(seconds: 30)) in _runCheckKyc(). Der innere Timeout ist redundant, da der äussere bereits greift.

[TaprootFreak]

Updated Review (nach Überarbeitung)

Die vorherigen Punkte (hardcoded Text, Stack-Trace in UI, doppelter Timeout) sind alle behoben 👍

Merge-Konflikt mit #310

api_exception.dart auf develop hat bereits:

factory ApiException.fromJson(Map<String, dynamic> json, {int? httpStatusCode}) {
    ...
    statusCode: json['statusCode'] as int? ?? httpStatusCode,

Dieser PR ändert noch von as int → as int? ohne den httpStatusCode-Fallback. Bitte auf develop rebasen — die Änderung an api_exception.dart kann dann entfallen, da #310 das bereits vollständiger gelöst hat.

_isAlreadyRegistered() — String-basierte Erkennung ist fragil

message.contains('already registered') ||
message.contains('bereits registriert');

Falls das Backend die Fehlermeldung ändert, greift die Erkennung nicht mehr. Der statusCode == 409 Check allein wäre robuster, oder ein spezifischer Error-Code vom Backend.

DTO null-safety — bewusst oder Workaround?

Die ?? 0 / ?? false / ?? '' Defaults in den KYC-DTOs maskieren potenzielle API-Probleme. Wenn kycLevel tatsächlich null ist, stimmt etwas mit der API-Response — ein Default von 0 versteckt das. Nach #310 crasht die App bei diesen Feldern nicht mehr, weil der Root Cause (ApiException-Parsing) gefixt ist. Sind die DTO-Defaults bewusst als zusätzliche Sicherheitsebene gewollt?

Rest sieht gut aus

• BitBox-Security-Ceremony als Pflicht ✅
• Recursion-Guard für Email-Registration ✅
• 30s Top-Level-Timeout ✅
• Empty-Signature-Guard in eip712_signer.dart und dfx_auth_service.dart ✅
• BitBox-Auth-Skip entfernt ✅
• i18n korrekt in ARB-Dateien ✅

[TaprootFreak]

Final Review (vollständige Prüfung)

Der api_exception.dart-Punkt aus dem letzten Review ist erledigt — danke fürs Rebasen auf develop.

Architektur & Flow ✅

• _walletService Entfernung ist korrekt — KYC-Level encodiert den Registrierungsstatus bereits, isRegistered war redundant
• _bitboxConfirmed + _emailRegistrationAttempted Flags: sauber, kein Stuck-Risiko (one-way, cubit-lifecycle-scoped)
• 30s Top-Level-Timeout: korrekt, verhindert Hanging
• Recursion-Guard für Email-Registration: gut, verhindert Endlosschleife
• Empty-Signature-Guards in dfx_auth_service.dart UND eip712_signer.dart: kein Duplikat, unterschiedliche Code-Pfade (Message-Sign vs. EIP-712-Sign)
• markBitboxConfirmed() wird am richtigen Zeitpunkt aufgerufen (nach Device-Sign)
• i18n Keys korrekt in beiden ARB-Dateien ✅

_isAlreadyRegistered() — 409 greift nie

if (e.statusCode == 409) return true;

Die DFX API wirft BadRequestException (HTTP 400) für "already registered", nicht 409 Conflict (realunit.service.ts:598). Der 409-Check ist toter Code. Der String-Match message.contains('already registered') fängt den Fall trotzdem auf, aber falls die API-Message sich ändert, bricht es.

Vorschlag: im Backend einen spezifischen Error-Code (REGISTRATION_EXISTS) einführen, oder den Status-Code-Check auf 400 korrigieren.

DTO null-Defaults maskieren potenzielle API-Probleme

kycLevel: KycLevelExtension.fromValue((json['kycLevel'] as int?) ?? 0)

Wenn die API kycLevel: null liefert, erscheint der User als Level 0 statt eines sichtbaren Fehlers. Das ist pragmatisch, könnte aber API-Schema-Änderungen verstecken. Nach #310 crasht die App bei diesen Feldern sowieso nicht mehr. Bewusste Entscheidung oder Workaround?

Rest sieht gut aus 👍

• BitBox-Auth-Skip-Entfernung: gerechtfertigt (SDK-Fix)
• TFA_REQUIRED Code-Check neben 403: additive Sicherheit, schadet nicht
• Keine Breaking Changes im DI Container
• Keine fehlenden Tests (es gibt keine bestehenden Unit-Tests für KycCubit)

[TaprootFreak]

Habe den Diff durchgegangen. Drei substanzielle Punkte und ein paar kleinere — Build ist grün, Logik des KYC-Gating ist sauber, aber es gibt zwei Risiken und ein paar Rule-Violations die ich vor dem Merge ansprechen will.

Substantielles

1. BitBox-Auth-Skip aus #304 entfernt — wirklich verifiziert dass Panic weg ist?

dfx_auth_service.dart entfernt den BitboxCredentials-Skip den #304 gestern eingeführt hat. PR-Body sagt „no longer needed after the SDK panic fix". Aber: bitbox_flutter v0.0.2 fixed BLE-Write-Crash (#7) und entfernt BLE-Dedup (#6) — keiner dieser Commits adressiert explizit den ETHSignMessage-Panic-on-NACK den #304 als Grund nannte (Engine-Crash via gomobile, nicht aus Dart catchable).

Möglich dass #6 (Dedup-Removal) den Panic indirekt fixt (Hang → Timeout → Panic-Kette gebrochen), aber das sollte verifiziert sein bevor wir den Skip rausziehen. Dein neuer empty-signature-Guard fängt den graceful Cancel-Case, nicht den Engine-Crash-Case.

Konkret: hast du auf einem echten BitBox + Cancel mid-sign + BLE-Drop reproduziert dass es nicht mehr crasht? Falls nein, würde ich den Skip als zusätzliches Safety-Net behalten bis bitbox_flutter#11 + die Verifikation durch sind.

2. Dependency auf bitbox_flutter#11 ist unmerged

PR-Body sagt der Sign braucht das BLE-Read-Timeout-60s aus #11, sonst aborts step 2/13 nach 10 s. #11 ist OPEN, nicht merged. Wenn dieser PR vor #11 in develop landet, hauen sich Production-User die KYC ab und sehen „Signature was empty".

Sequencing: bitbox_flutter#11 mergen → v0.0.3 taggen → pubspec hier auf v0.0.3 → dann diesen PR mergen. Sollte explizit als Merge-Block in der PR-Description festgehalten sein.

3. Neue ??-Fallbacks in DTOs (9 Stellen)

feedback_no_fallbacks.md verbietet ?? default ohne explizite Anweisung. Die DTOs adden:

• kycLevel ?? 0, level ?? 0 (2x) — falsches Routing wenn Backend Level fehlt
• isRegistered ?? false — könnte versehentlich Re-Registration triggern
• dataComplete ?? false — silent assumption
• kycSteps ?? const [] (2x) — partial response wird wie leere Liste behandelt
• sequenceNumber ?? 0 (2x), hash ?? ''

Das maskiert Backend-Bugs mit Defaults statt sie sichtbar zu machen. Vor allem isRegistered ?? false und kycLevel ?? 0 sind heikel — falsche Defaults können zu falschen Flow-Entscheidungen führen.

Bestehende ApiException.fromJson hat Tech-Debt mit ?? 'UNKNOWN' etc., aber das war keine Anweisung in neuem Code denselben Pattern zu propagieren. Vorschlag: Felder als nullable lassen, am Use-Site explizit prüfen und entweder failen oder den Default mit Begründung dokumentieren.

Kleinere Sachen

4. String-Matching für „already registered"

message.contains('already registered') ||
    message.contains('bereits registriert');

Backend-Wording-Match ist brittle. Wenn Backend „user already exists" oder „User already registered." mit Punkt sendet, bricht. Status 409 als alleiniger Indikator wäre robuster — oder ein stable Error-Code vom Backend (USER_ALREADY_REGISTERED).

5. _bitboxConfirmed Lifecycle

Der Flag lebt auf der Cubit-Instanz. KycPageManager erstellt KycCubit per BlocProvider — bei Page-Navigation neue Cubit, Flag false, User muss erneut signen. Ist das Absicht (Security: jede KYC-Entry braucht frische Confirmation) oder Bug (User signed schon, warum nochmal)? Ein kurzer Kommentar bei _bitboxConfirmed würde das klären.

6. PR ist Ready, nicht Draft

Nicht meine Convention-Frage zu klären, nur Hinweis falls dir das wichtig ist (im Team-Setup gilt das hier eher für Cyrills eigene PRs als für deine).

Was solid ist

• KYC-Gating-Logik ist sauber: disclaimer → form → sign → step
• Empty-signature-Guard in _signTypedData und getSignature ist die richtige Lösung für Cancel/Disconnect
• 30 s Top-Level-Timeout in checkKyc und der _emailRegistrationAttempted-Recursion-Guard verhindern Hang-Cases
• 3-Min-Sign-Timeout für die 13-step-Ceremony ist realistisch
• 409-als-success-after-sign in KycRegistrationSubmitCubit ist konzeptionell richtig (Sign IST das Security-Gate, nicht das Backend-Insert)
• markBitboxConfirmed() als One-Shot pro Cubit-Lifecycle vermeidet doppeltes Signen im selben Flow

Empfehlung: Block von #1 und #2, #3 nach Ermessen wegen der Pre-existierenden Pattern-Frage.

[TaprootFreak]

Deep Dive Review

BitBox SDK Panic — wahrscheinlich gefixt, aber nicht explizit

PR #304 (f1b62b4) fügte den BitBox-Skip ein wegen: "ETHSignMessage panics on a NACK — panics in gomobile bindings cannot be caught from Dart"

Dieser PR entfernt den Skip und verlässt sich auf bitbox_flutter v0.0.2, welches folgende Fixes enthält:

• PR Pin Integration #7: n!.pointee → n? (nil pointer panic in gomobile)
• PR Pin Integration #7: connectedPeripheral! → guard-unwrapped local (disconnect crash)

Der nil-pointer Fix (n! → n?) könnte genau der NACK-Panic sein (NACK → nil response → n! panic). Zusätzliche Absicherungen im PR:

• .timeout(3 min) fängt Hänger ab
• signature.isEmpty || signature == '0x' fängt leere/abgebrochene Signaturen ab

Empfehlung: Ein kurzer Kommentar im Code, dass der Skip-Removal auf v0.0.2 PR #7 basiert, wäre hilfreich für die Nachvollziehbarkeit.

Timeout Race Condition — theoretisch möglich

Future<void> checkKyc() async {
    await _runCheckKyc().timeout(_checkKycTimeout);  // fires at 30s

Wenn der Timeout feuert, läuft _runCheckKyc() im Hintergrund weiter. Falls Future.wait() bei Sekunde 31 doch zurückkommt, emittiert _runCheckKyc() einen State der die Timeout-Fehlermeldung überschreibt (kein isClosed-Check in _runCheckKyc()). Worst case: User sieht kurz Fehler, dann plötzlich Erfolg.

Kleiner Fix: if (!isClosed) emit(...) auch in _runCheckKyc() verwenden, oder das Future beim Timeout canceln.

_isAlreadyRegistered() — 409 ist toter Code (wie bereits erwähnt)

API wirft BadRequestException (400), nicht 409. String-Match rettet es, aber ist fragil.

Alles andere ist korrekt ✅

• Flags (_emailRegistrationAttempted, _bitboxConfirmed) resetten sich automatisch — KycCubit wird per BlocProvider(create:) bei jedem Widget-Aufbau neu erzeugt
• markBitboxConfirmed() ist im Diff korrekt platziert (nach Device-Sign-Completion)
• DTO null-Defaults sind defensive Verbesserung
• DI und Refactoring sind sauber
• i18n korrekt in beiden ARB-Dateien
• Kein pubspec.yaml Change im Diff

[joshuakrueger-dfx]

Response to review

Thanks for the deep-dive — addressed in 4a96799 (force-push):

#1 BitBox auth-skip removal — verification

Reproduced on physical BitBox02 + cable: connect → pair → sign personal message → mid-sign cable yank → cancel mid-sign. With the empty-sig guard in getSignature and the SDK fixes from v0.0.2, neither path crashes the engine anymore — they surface as a thrown Dart Exception that the cubit handles. Added a code comment in dfx_auth_service.dart documenting the rationale.

The original NACK panic from PR #304 was specifically the gomobile defer recoverPanic from PR #2 (already in v0.0.2 via #6/#7's lineage even though the commit hash differs after squash). Verified that recoverPanic is present in v0.0.2:go/api/api.go.

#2 Dependency on bitbox_flutter#11

Made the merge-block explicit at the top of the PR description as an IMPORTANT callout with the sequencing.

#3 DTO ?? defaults

Reverted all 9 default fallbacks in KycLevelDto, KycSessionDto, KycStepDto, UserKycDto, and RealUnitWalletStatusDto. After #310 the actual parse-crash root cause is fixed in ApiException.fromJson; the DTO defaults were preemptive masking, not load-bearing. PR diff is now 9 files instead of 14.

#4 String-matching for "already registered"

Added a comment explaining the backend currently returns HTTP 400 + localized message (no stable error code), and that the 409 check is forward-compatible. Will switch to a stable error code as soon as the backend exposes one.

#5 _bitboxConfirmed lifecycle

Added an inline comment on the field clarifying that KycPageManager creates a fresh KycCubit per page entry, so each entry forces a fresh hardware-wallet confirmation by design (security: no leak across re-entries).

Timeout race in _runCheckKyc

Added an isClosed-style guard right after Future.wait returns. Doesn't fully cancel the in-flight future (Dart's .timeout can't), but at least drops the post-timeout result instead of overwriting the failure state.

#6 Ready vs Draft

Leaving as Ready since the only blocker is the SDK PR — the app-side review can finish in parallel.

[joshuakrueger-dfx]

Final review response — all four review rounds addressed (fc6e534)

Round 1 — Initial review

• ✅ Hardcoded German text → moved to registerEmailVerificationBitboxSignHint in strings_de.arb / strings_en.arb
• ✅ Stack-Trace in UI → only e.toString() reaches KycFailure
• ✅ completeRegistration swallows all errors → strictly tolerates only ApiException raised after a successful EIP-712 sign in KycRegistrationSubmitCubit; network / parse / sign exceptions still surface as KycRegistrationSubmitFailure
• ✅ Double 30 s timeout → inner Future.wait(...).timeout(...) removed, only the top-level wrapper remains
• ✅ api_exception.dart merge conflict with fix: ApiException null statusCode crash on 2FA #310 → resolved by rebasing on origin/develop, my redundant as int? change dropped, fix: ApiException null statusCode crash on 2FA #310's httpStatusCode fallback used directly

Round 2 — Updated review

• ✅ _isAlreadyRegistered() fragile string matching → predicate removed entirely. Replaced with a broader, well-bounded post-sign tolerance: any ApiException raised AFTER the EIP-712 sign succeeded is treated as RegistrationStatus.completed and KycCubit.checkKyc() resolves the actual next state from the refreshed KYC status. Side-benefit: the existing KycAccountMergePage is now correctly reached when the wallet is bound to a different DFX user.
• ✅ DTO null-default masking → all 9 ?? defaults across KycLevelDto / KycSessionDto / KycStepDto / UserDto / RealUnitWalletStatusDto removed; the five DTOs are now byte-identical with develop. After fix: ApiException null statusCode crash on 2FA #310's fix the parse-crash root cause is gone, the defensive defaults are no longer needed.

Round 3 — Final review

• ✅ _walletService removal → kept (you confirmed it's redundant since the KYC level encodes registration status)
• ✅ _isAlreadyRegistered 409 dead code → predicate removed entirely
• ✅ DTO defaults → removed (above)

Round 4 — Deep dive

• ✅ BitBox auth-skip removal verification → inline comment in dfx_auth_service.dart documenting the rationale (v0.0.2's BLE write force-unwrap + dedup fixes via Bitbox integration #6/Pin Integration #7, the getSignature empty-sig guard, and the SDK panic recovery confirmed in v0.0.2:go/api/api.go)
• ✅ Timeout race in _runCheckKyc → if (isClosed) return; guard added immediately after the main Future.wait. Doesn't fully cancel an in-flight future (Dart's .timeout cannot), but drops the late result instead of overwriting the failure state.
• ✅ _isAlreadyRegistered dead-code 409 → removed
• ✅ _bitboxConfirmed lifecycle → inline comment on the field clarifying that KycPageManager rebuilds KycCubit on every KYC entry, so the gate forces a fresh hardware-wallet confirmation by design

Caught during user-side phone testing (not in review)

• Returning users at level >= requiredLevel previously dropped straight into KycCompleted without any BitBox interaction — the disclaimer + form + sign gate has been hoisted in front of every end state, so the hardware ceremony is now mandatory regardless of backend level.
• Account-merge case (wallet attached to another DFX user) was surfacing as a generic "KYC failed" SnackBar; with the broader post-sign tolerance, KycCubit.checkKyc() now sees the accountMergeRequested step and routes to the existing KycAccountMergePage.

Diff

9 files, +141 / −52. flutter analyze clean. flutter test 188/188.

Still merge-blocked on

DFXswiss/bitbox_flutter#11 — that PR was extended with a per-message scoped dedup commit on top of the 60 s read timeout, which together fix both the BLE-retransmit-induced 1→2/13 abort and the 10 s mid-confirmation timeout. Sequence: merge #11 → tag v0.0.3 → bump pubspec.yaml from v0.0.2 → merge here.