improve payload and error management (#20)

Author: jleni

.gitignore

@@ -1,3 +1,4 @@
 node_modules
 yarn.lock
+.idea
 dist/

src/app.test.ts

@@ -17,6 +17,7 @@ import { MockTransport } from '@ledgerhq/hw-transport-mocker'
 
 import BaseApp from './app'
 import { LedgerError } from './consts'
+import { ResponseError } from './responseError'
 
 describe('BaseApp', () => {
   const params = {
@@ -69,7 +70,7 @@ describe('BaseApp', () => {
   describe('getVersion', () => {
     it('should retrieve version information', async () => {
       const responseBuffer = Buffer.concat([
-        Buffer.from([0, 1, 2, 3]), // Version information
+        Buffer.from([0, 1, 2, 3, 0]), // Version information
         Buffer.from([0x90, 0x00]), // Status code for no errors (0x9000)
       ])
 
@@ -82,20 +83,29 @@ describe('BaseApp', () => {
         minor: 2,
         patch: 3,
         deviceLocked: false,
-        targetId: '0',
+        targetId: '',
         testMode: false,
       })
     })
 
+    it('should handle missing data', async () => {
+      const responseBuffer = Buffer.concat([
+        Buffer.from([0, 1, 2, 3]), // Version information
+        Buffer.from([0x90, 0x00]), // Status code for no errors (0x9000)
+      ])
+
+      const transport = new MockTransport(responseBuffer)
+      const app = new BaseApp(transport, params)
+
+      await expect(app.getVersion()).rejects.toEqual(new ResponseError(LedgerError.UnknownError, 'Attempt to read beyond buffer length'))
+    })
+
     it('should handle errors correctly', async () => {
       const transport = new MockTransport(Buffer.alloc(0))
       transport.exchange = jest.fn().mockRejectedValue(new Error('Unknown error'))
       const app = new BaseApp(transport, params)
 
-      await expect(app.getVersion()).rejects.toEqual({
-        returnCode: LedgerError.UnknownTransportError,
-        errorMessage: 'Unknown transport error',
-      })
+      await expect(app.getVersion()).rejects.toEqual(ResponseError.fromReturnCode(LedgerError.UnknownTransportError))
     })
   })
 
@@ -123,10 +133,7 @@ describe('BaseApp', () => {
       transport.exchange = jest.fn().mockRejectedValue(new Error('App does not seem to be open'))
       const app = new BaseApp(transport, params)
 
-      await expect(app.appInfo()).rejects.toEqual({
-        returnCode: LedgerError.UnknownTransportError,
-        errorMessage: 'Unknown transport error',
-      })
+      await expect(app.appInfo()).rejects.toEqual(ResponseError.fromReturnCode(LedgerError.UnknownTransportError))
     })
   })
 
@@ -156,10 +163,7 @@ describe('BaseApp', () => {
       transport.exchange = jest.fn().mockRejectedValue(new Error('Device is busy'))
 
       const app = new BaseApp(transport, params)
-      await expect(app.deviceInfo()).rejects.toEqual({
-        returnCode: LedgerError.UnknownTransportError,
-        errorMessage: 'Unknown transport error',
-      })
+      await expect(app.deviceInfo()).rejects.toEqual(ResponseError.fromReturnCode(LedgerError.UnknownTransportError))
     })
   })
 })

src/app.ts

@@ -17,13 +17,14 @@ import type Transport from '@ledgerhq/hw-transport'
 
 import { processErrorResponse, processResponse } from './common'
 import { HARDENED, LEDGER_DASHBOARD_CLA, LedgerError, PAYLOAD_TYPE } from './consts'
+import { ResponsePayload } from './payload'
+import { ResponseError } from './responseError'
 import {
   type ConstructorParams,
   type INSGeneric,
   type P1_VALUESGeneric,
   type ResponseAppInfo,
   type ResponseDeviceInfo,
-  ResponseError,
   type ResponseVersion,
 } from './types'
 
@@ -128,7 +129,7 @@ export default class BaseApp {
    * @returns A promise that resolves to the processed response from the device.
    * @throws {ResponseError} If the response from the device indicates an error.
    */
-  async signSendChunk(ins: number, chunkIdx: number, chunkNum: number, chunk: Buffer): Promise<Buffer> {
+  async signSendChunk(ins: number, chunkIdx: number, chunkNum: number, chunk: Buffer): Promise<ResponsePayload> {
     let payloadType = PAYLOAD_TYPE.ADD
 
     if (chunkIdx === 1) {
@@ -146,6 +147,7 @@ export default class BaseApp {
 
     return response
   }
+
   /**
    * Retrieves the version information from the device.
    * @returns A promise that resolves to the version information.
@@ -156,21 +158,27 @@ export default class BaseApp {
       const responseBuffer = await this.transport.send(this.CLA, this.INS.GET_VERSION, 0, 0)
       const response = processResponse(responseBuffer)
 
-      let targetId = 0
+      const testMode = response.readBytes(1).readUInt8() !== 0
+      const major = response.readBytes(1).readUInt8()
+      const minor = response.readBytes(1).readUInt8()
+      const patch = response.readBytes(1).readUInt8()
+
+      const deviceLocked = response.readBytes(1).readUInt8() === 1
 
-      if (response.length >= 9) {
-        targetId = response.readUInt32BE(5)
+      let targetId = ''
+      if (response.length() >= 4) {
+        targetId = response.readBytes(4).readUInt32BE().toString(16)
       }
 
       // FIXME: Add support for devices with multibyte version numbers
 
       return {
-        testMode: response[0] !== 0,
-        major: response[1],
-        minor: response[2],
-        patch: response[3],
-        deviceLocked: response[4] === 1,
-        targetId: targetId.toString(16),
+        testMode,
+        major,
+        minor,
+        patch,
+        deviceLocked,
+        targetId,
       }
     } catch (error) {
       throw processErrorResponse(error)
@@ -187,23 +195,23 @@ export default class BaseApp {
       const responseBuffer = await this.transport.send(LEDGER_DASHBOARD_CLA, 0x01, 0, 0)
       const response = processResponse(responseBuffer)
 
-      if (response[0] !== 1) {
+      const formatId = response.readBytes(1).readUInt8()
+
+      if (formatId !== 1) {
         throw {
           returnCode: 0x9001,
           errorMessage: 'Format ID not recognized',
         } as ResponseError
       }
 
-      const appNameLen = response[1]
-      const appName = response.subarray(2, 2 + appNameLen).toString('ascii')
-      let idx = 2 + appNameLen
-      const appVersionLen = response[idx]
-      idx += 1
-      const appVersion = response.subarray(idx, idx + appVersionLen).toString('ascii')
-      idx += appVersionLen
-      const flagLen = response[idx]
-      idx += 1
-      const flagsValue = response[idx]
+      const appNameLen = response.readBytes(1).readUInt8()
+      const appName = response.readBytes(appNameLen).toString('ascii')
+
+      const appVersionLen = response.readBytes(1).readUInt8()
+      const appVersion = response.readBytes(appVersionLen).toString('ascii')
+
+      const flagLen = response.readBytes(1).readUInt8()
+      const flagsValue = response.readBytes(flagLen).readUInt8()
 
       return {
         appName,
@@ -230,25 +238,22 @@ export default class BaseApp {
       const responseBuffer = await this.transport.send(0xe0, 0x01, 0, 0, Buffer.from([]), [LedgerError.NoErrors, 0x6e00])
       const response = processResponse(responseBuffer)
 
-      const targetId = response.subarray(0, 4).toString('hex')
+      const targetId = response.readBytes(4).toString('hex')
+
+      const secureElementVersionLen = response.readBytes(1).readUInt8()
+      const seVersion = response.readBytes(secureElementVersionLen).toString()
 
-      let pos = 4
-      const secureElementVersionLen = response[pos]
-      pos += 1
-      const seVersion = response.subarray(pos, pos + secureElementVersionLen).toString()
-      pos += secureElementVersionLen
+      const flagsLen = response.readBytes(1).readUInt8()
+      const flag = response.readBytes(flagsLen).toString('hex')
 
-      const flagsLen = response[pos]
-      pos += 1
-      const flag = response.subarray(pos, pos + flagsLen).toString('hex')
-      pos += flagsLen
+      const mcuVersionLen = response.readBytes(1).readUInt8()
+      let tmp = response.readBytes(mcuVersionLen)
 
-      const mcuVersionLen = response[pos]
-      pos += 1
       // Patch issue in mcu version
-      let tmp = response.subarray(pos, pos + mcuVersionLen)
-      if (tmp[mcuVersionLen - 1] === 0) {
-        tmp = response.subarray(pos, pos + mcuVersionLen - 1)
+      // Find the first zero byte and trim the buffer up to that point
+      const firstZeroIndex = tmp.indexOf(0)
+      if (firstZeroIndex !== -1) {
+        tmp = tmp.subarray(0, firstZeroIndex)
       }
       const mcuVersion = tmp.toString()
 

src/common.test.ts

@@ -15,6 +15,7 @@
  *****************************************************************************/
 import { errorCodeToString, processErrorResponse } from './common'
 import { LedgerError } from './consts'
+import { ResponseError } from './responseError'
 
 describe('errorCodeToString', () => {
   it('should return the correct error message for a known error code', () => {
@@ -33,10 +34,7 @@ describe('errorCodeToString', () => {
 describe('processErrorResponse', () => {
   it('should return correct response object when statusCode is present', () => {
     const response = { statusCode: 0x9000 }
-    const expectedResponse = {
-      returnCode: 0x9000,
-      errorMessage: 'No errors',
-    }
+    const expectedResponse = new ResponseError(0x9000, 'No errors')
     expect(processErrorResponse(response)).toEqual(expectedResponse)
   })
 
@@ -47,10 +45,7 @@ describe('processErrorResponse', () => {
 
   it('should return a default error response when neither statusCode nor returnCode/errorMessage are present', () => {
     const response = { someOtherKey: 123 }
-    const expectedResponse = {
-      returnCode: LedgerError.UnknownTransportError,
-      errorMessage: 'Unknown transport error',
-    }
+    const expectedResponse = ResponseError.fromReturnCode(LedgerError.UnknownTransportError)
     expect(processErrorResponse(response)).toEqual(expectedResponse)
   })
 })

src/common.ts

@@ -14,7 +14,8 @@
  *  limitations under the License.
  *****************************************************************************/
 import { ERROR_DESCRIPTION_OVERRIDE, LedgerError } from './consts'
-import { ResponseError, ResponsePayload, type ResponseReturnCode } from './types'
+import { ResponsePayload } from './payload'
+import { ResponseError } from './responseError'
 
 export function errorCodeToString(returnCode: LedgerError): string {
   const returnCodeStr = returnCode.toString(16).toUpperCase()
@@ -27,13 +28,6 @@ export function errorCodeToString(returnCode: LedgerError): string {
   return errDescription
 }
 
-function responseBaseFrom(retCode: number): ResponseReturnCode {
-  return {
-    returnCode: retCode,
-    errorMessage: errorCodeToString(retCode),
-  }
-}
-
 function isDict(v: any): boolean {
   return typeof v === 'object' && v !== null && !(v instanceof Array) && !(v instanceof Date)
 }
@@ -51,7 +45,7 @@ function isDict(v: any): boolean {
 export function processResponse(responseRaw: Buffer): ResponsePayload {
   // Ensure the buffer is large enough to contain a return code
   if (responseRaw.length < 2) {
-    throw responseBaseFrom(LedgerError.EmptyBuffer) as ResponseError
+    throw ResponseError.fromReturnCode(LedgerError.EmptyBuffer)
   }
 
   // Determine the return code from the last two bytes of the response
@@ -63,7 +57,7 @@ export function processResponse(responseRaw: Buffer): ResponsePayload {
 
   // Directly return the payload if there are no errors
   if (returnCode === LedgerError.NoErrors) {
-    return payload
+    return new ResponsePayload(payload)
   }
 
   // Append additional error message from payload if available
@@ -84,10 +78,10 @@ export function processResponse(responseRaw: Buffer): ResponsePayload {
  * @param response - The raw response object that may contain error details.
  * @returns A standardized error response object.
  */
-export function processErrorResponse(response: any): ResponseReturnCode {
+export function processErrorResponse(response: any): ResponseError {
   if (isDict(response)) {
     if (Object.prototype.hasOwnProperty.call(response, 'statusCode')) {
-      return responseBaseFrom(response.statusCode)
+      return ResponseError.fromReturnCode(response.statusCode)
     }
 
     if (Object.prototype.hasOwnProperty.call(response, 'returnCode') && Object.prototype.hasOwnProperty.call(response, 'errorMessage')) {
@@ -96,5 +90,5 @@ export function processErrorResponse(response: any): ResponseReturnCode {
   }
 
   // If response is not a dictionary or does not contain the expected properties, handle as unknown error
-  return responseBaseFrom(LedgerError.UnknownTransportError)
+  return ResponseError.fromReturnCode(LedgerError.UnknownTransportError)
 }

src/payload.test.ts

@@ -0,0 +1,49 @@
+import { Buffer } from 'buffer'
+
+import { LedgerError } from './consts'
+import { ResponsePayload } from './payload'
+import { ResponseError } from './responseError'
+
+describe('ResponsePayload', () => {
+  let payload: Buffer
+  let responsePayload: ResponsePayload
+
+  beforeEach(() => {
+    payload = Buffer.from([0x01, 0x02, 0x03, 0x04, 0x05])
+    responsePayload = new ResponsePayload(payload)
+  })
+
+  test('getCompleteBuffer should return a complete buffer', () => {
+    expect(responsePayload.getCompleteBuffer()).toEqual(Buffer.from([0x01, 0x02, 0x03, 0x04, 0x05]))
+  })
+
+  test('getAvailableBuffer should return the available buffer', () => {
+    responsePayload.readBytes(3)
+    expect(responsePayload.getAvailableBuffer()).toEqual(Buffer.from([0x04, 0x05]))
+  })
+
+  test('readBytes should return the correct bytes and increase offset', () => {
+    const readBuffer = responsePayload.readBytes(2)
+    expect(readBuffer).toEqual(Buffer.from([0x01, 0x02]))
+    expect(responsePayload.readBytes(1)).toEqual(Buffer.from([0x03]))
+  })
+
+  test('skipBytes should increase the offset correctly', () => {
+    responsePayload.skipBytes(2)
+    expect(responsePayload.readBytes(1)).toEqual(Buffer.from([0x03]))
+  })
+
+  test('resetOffset should reset the offset to zero', () => {
+    responsePayload.readBytes(3)
+    responsePayload.resetOffset()
+    expect(responsePayload.readBytes(2)).toEqual(Buffer.from([0x01, 0x02]))
+  })
+
+  test('readBytes should throw an error when reading beyond the buffer length', () => {
+    expect(() => responsePayload.readBytes(10)).toThrow(new ResponseError(LedgerError.UnknownError, 'Attempt to read beyond buffer length'))
+  })
+
+  test('skipBytes should throw an error when skipping beyond the buffer length', () => {
+    expect(() => responsePayload.skipBytes(10)).toThrow(new ResponseError(LedgerError.UnknownError, 'Attempt to skip beyond buffer length'))
+  })
+})