fix(recovery): roll back confirm record to 'confirmed' on failure#306
Open
ety001 wants to merge 1 commit into
Open
fix(recovery): roll back confirm record to 'confirmed' on failure#306ety001 wants to merge 1 commit into
ety001 wants to merge 1 commit into
Conversation
The CAS in recovery/confirm sets status to 'processing' but only the success path transitions to 'closed'. Every failure after the claim — owner-key mismatch, conveyor config missing (503), requestAccountRecovery RPC error — left the record permanently stuck in 'processing', so the user could never retry and the subsequent recover-account broadcast (which requires status='closed') was wedged. A single transient RPC error would brick a recovery request. Add a rollbackToConfirmed() helper that resets 'processing' → 'confirmed' (CAS-guarded on status='processing' to avoid clobbering concurrent success) and call it on every failure path: owner-key mismatch, conveyor preflight, and the catch block. The catch-block rollback is best-effort. Tests: add rollback assertions for the RPC-error and conveyor-missing paths; extend setupUpdateMocks to chain a third update (rollback).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes audit V2 finding #1 (High): the
recovery/confirmroute's CAS claim setsstatus = 'processing', but only the success path transitions to'closed'. Every failure path left the record permanently stuck in'processing'— the user could never retry, and the subsequentrecover-accountbroadcast (which requiresstatus = 'closed') was wedged. A single transient Steem RPC error would permanently brick a recovery request.Changes
src/app/api/recovery/confirm/route.ts: addrollbackToConfirmed()helper that resets'processing' → 'confirmed'(CAS-guarded onstatus = 'processing'to avoid clobbering a concurrent success). Call it on every failure path after the CAS claim:requestAccountRecoveryRPC error / any catch (500; best-effort)tests/unit/recovery-confirm-route.test.ts: add rollback regression tests for the RPC-error and conveyor-missing paths; extendsetupUpdateMocksto support the third update chain (rollback).Root cause
The CAS at the top of the try-block atomically claims the record (
confirmed → processing) to prevent TOCTOU races. But the original code's comment even admitted: "If this fails, the record stays in 'processing' and won't be re-processed." — with no rollback or janitor to reset it.Test plan
pnpm type-check— cleanpnpm test— 467 passed (includes 2 new rollback regression tests)