Skip to content

fix(sdk-core): return clear 401 for wrong wallet passphrase - #9365

Draft
shubham-damkondwar wants to merge 1 commit into
masterfrom
COINS-1257
Draft

fix(sdk-core): return clear 401 for wrong wallet passphrase#9365
shubham-damkondwar wants to merge 1 commit into
masterfrom
COINS-1257

Conversation

@shubham-damkondwar

Copy link
Copy Markdown
Contributor

Summary

  • Return IncorrectPasswordError (HTTP 401, wallet_passphrase_incorrect) for wrong wallet passphrase instead of a generic 500 / Internal Server Error
  • Stop string-wrapping TSS signing failures so original API/status/details are preserved
  • Fix Express retryPromise and decryptPrivKey to keep actionable passphrase vs malformed-ciphertext errors

Ticket: COINS-1257

Note: Separate from BitGo Internal Agent PR #9344 (ICP broadcast certificate parsing), which addresses a different failure mode than this ticket’s remaining ask.

Test plan

  • Express unit tests for wrong passphrase decrypt → 401 (coinSign, generateShareTSS, ofcExtSignPayload)
  • retryPromise preserves original error without wrapping
  • signPayload passphrase vs malformed ciphertext cases
  • Manual: POST /api/v2/icp/wallet/:id/signtxtss with wrong walletPassphrase returns 401 + code: wallet_passphrase_incorrect

Preserve signing error details instead of wrapping them into generic
Internal Server Error responses.

Ticket: COINS-1257
@linear-code

linear-code Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

COINS-1257

@shubham-damkondwar
shubham-damkondwar marked this pull request as draft July 27, 2026 16:37
@shubham-damkondwar

Copy link
Copy Markdown
Contributor Author

@claude

@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished @shubham-damkondwar's task —— View job


Code Review for PR #9365

This PR effectively addresses incorrect wallet passphrase error handling by returning proper 401 HTTP responses with clear error codes instead of generic 500 errors. The changes are well-structured and focused.

Key Improvements

✅ Proper Error Classification

  • IncorrectPasswordError: New error class in modules/sdk-core/src/bitgo/errors.ts:170-178 with code: 'wallet_passphrase_incorrect' and status: 401
  • Consistent Usage: Applied across wallet operations in modules/sdk-core/src/bitgo/wallet/wallet.ts:2565 and modules/sdk-core/src/bitgo/wallet/wallet.ts:5362

✅ Enhanced Express Error Handling

  • Smart Passphrase Detection: modules/express/src/clientRoutes.ts:437-440 distinguishes between malformed ciphertext and wrong passphrase using regex pattern matching
  • Error Code Propagation: Added ...(err.code ? { code: err.code } : {}) in modules/express/src/clientRoutes.ts:1710 to preserve error codes in API responses

✅ TSS Signing Improvements

  • Removed Error Wrapping: Multiple TSS signing functions in wallet.ts now preserve original error details instead of wrapping them in generic messages
  • Clean Implementation: Eliminated unnecessary try-catch blocks that were obscuring actual error information

✅ RetryPromise Fix

  • Error Preservation: modules/express/src/retryPromise.ts:36 now throws original errors instead of wrapping them with new Error(err)

✅ Comprehensive Test Updates

  • Status Code Validation: Updated tests expect 401 instead of 500 for wrong passphrase scenarios
  • Message Validation: Tests verify specific error messages match new IncorrectPasswordError behavior
  • New Test Coverage: Added modules/express/test/unit/retryPromise.ts to verify error preservation

Technical Quality

Code Pattern Consistency: The implementation follows a clear pattern of distinguishing between system errors (malformed data) and user errors (wrong credentials).

Error Message Clarity: The regex pattern /not valid JSON|unknown envelope version|salt must be|iv must be/i effectively identifies structural decryption issues vs. authentication failures.

Backwards Compatibility: Changes preserve existing error handling for non-passphrase related failures while improving user experience for authentication errors.

Minor Considerations

  1. Test Coverage Gap: The PR mentions one unchecked manual test item for ICP signtxtss endpoint - consider adding automated coverage if possible.

  2. Error Message Consistency: Ensure all client-facing error messages follow consistent formatting and language standards across the SDK.

Overall, this is a well-implemented improvement that enhances error handling precision and user experience. The changes are focused, well-tested, and maintain backward compatibility while providing clearer feedback to API consumers.

Recommendation: ✅ Approve - This PR effectively addresses the stated objectives with clean, maintainable code and comprehensive test coverage.


PR #9365 | Branch: COINS-1257

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants