Make the transport proxy throw exceptions on every sw except 0x9000 (…#571
Make the transport proxy throw exceptions on every sw except 0x9000 (…#571chcmedeiros merged 1 commit intomainfrom
Conversation
WalkthroughA new dependency, Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Zemu
participant Transport
participant Errors
Caller->>Zemu: getTransport().send(...)
Zemu->>Transport: send(...)
Transport-->>Zemu: responseBuffer
Zemu->>Zemu: Extract SW from responseBuffer
alt SW == NoError (0x9000)
Zemu-->>Caller: Return responseBuffer
else SW != NoError
Zemu->>Errors: Throw TransportStatusError(SW)
Zemu-->>Caller: Exception thrown
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Zemu.ts (2)
70-72: ApduError enum design is minimal but functional.The enum contains only the success status code. Consider adding other common APDU error codes for better maintainability and documentation.
enum ApduError { NoError = 0x9000, + // Common APDU error codes + ConditionsNotSatisfied = 0x6985, + SecurityStatusNotSatisfied = 0x6982, + WrongData = 0x6a80, + WrongLength = 0x6700, + ClaNotSupported = 0x6e00, + InsNotSupported = 0x6d00, }
462-485: Consider extracting status word validation into a separate method.The status word validation logic is repeated and could be extracted into a reusable method for better maintainability.
+ private validateStatusWord(result: Buffer, statusList?: number[]): void { + const sw = result.readUInt16BE(result.length - 2) + if (sw !== ApduError.NoError && (!statusList || !statusList.includes(sw))) { + throw new TransportStatusError(sw) + } + } + // Return a proxy that intercepts send() calls return new Proxy(originalTransport, { get(target, prop, receiver) { if (prop === 'send') { return async function (cla: number, ins: number, p1: number, p2: number, data?: Buffer, statusList?: number[]) { try { self.lastTransportError = null // Clear previous error const result = await target.send(cla, ins, p1, p2, data, statusList) - const sw = result.readUInt16BE(result.length - 2) - - if (sw !== ApduError.NoError) { - throw new TransportStatusError(sw) - } + self.validateStatusWord(result, statusList) return result
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
package.json(1 hunks)src/Zemu.ts(3 hunks)tests/error-handling.test.ts(1 hunks)
🔇 Additional comments (4)
package.json (1)
45-45: Dependency addition confirmed.The
@ledgerhq/errorsdependency at version^6.23.0is the latest stable release (published July 23, 2025) and aligns with the PR’s error handling enhancements. No further changes needed.tests/error-handling.test.ts (2)
32-34: Test parameters correctly updated for new error handling.The addition of an empty data buffer and explicit status list aligns with the new transport behavior that checks status words after APDU responses.
41-41: No changes needed: test behavior matches implementationThe
sendwrapper insrc/Zemu.tsalways throws aTransportStatusErrorfor any status word other thanApduError.NoError(0x9000). Therefore, passing astatusListthat includes 0x6e00 will still result in an exception on 0x6e00, which is exactly what the test asserts.src/Zemu.ts (1)
18-18: Import statement is correct.The
TransportStatusErrorimport from@ledgerhq/errorsaligns with the new dependency added inpackage.json.
| const sw = result.readUInt16BE(result.length - 2) | ||
|
|
||
| if (sw !== ApduError.NoError) { | ||
| throw new TransportStatusError(sw) | ||
| } |
There was a problem hiding this comment.
Status word validation logic is correct but breaks existing transport behavior.
The implementation correctly extracts the 2-byte status word from the end of the APDU response and throws TransportStatusError for non-success codes. However, this fundamentally changes the transport behavior by ignoring the statusList parameter that was previously used to accept specific error codes.
The current implementation ignores the statusList parameter and throws exceptions for ANY non-0x9000 status word. This breaks backward compatibility for code that intentionally accepts specific error status codes.
Consider this alternative approach:
- const sw = result.readUInt16BE(result.length - 2)
-
- if (sw !== ApduError.NoError) {
- throw new TransportStatusError(sw)
- }
+ const sw = result.readUInt16BE(result.length - 2)
+
+ // Only throw if status word is not in the accepted list
+ if (sw !== ApduError.NoError && (!statusList || !statusList.includes(sw))) {
+ throw new TransportStatusError(sw)
+ }This preserves the original behavior where specific error codes can be accepted via the statusList parameter.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const sw = result.readUInt16BE(result.length - 2) | |
| if (sw !== ApduError.NoError) { | |
| throw new TransportStatusError(sw) | |
| } | |
| const sw = result.readUInt16BE(result.length - 2) | |
| // Only throw if status word is not in the accepted list | |
| if (sw !== ApduError.NoError && (!statusList || !statusList.includes(sw))) { | |
| throw new TransportStatusError(sw) | |
| } |
🤖 Prompt for AI Agents
In src/Zemu.ts around lines 466 to 470, the current code throws a
TransportStatusError for any status word not equal to ApduError.NoError,
ignoring the statusList parameter. To fix this, modify the logic to check if the
extracted status word is included in the statusList array before throwing an
error. If the status word is not in statusList, then throw the
TransportStatusError; otherwise, allow the response to pass. This preserves
backward compatibility by respecting the statusList parameter.
2 participants
…#566)
Summary by CodeRabbit
Bug Fixes
Chores
Tests