App Improvement (#61)

* fix: prevent concurrent APDU processing during UI review

Introduce a `review_pending` flag to block new commands while the device is waiting for user approval. This prevents the host from mutating state or buffers during an asynchronous review window, addressing potential race conditions on transports like BLE.

* improvement: update zemu tests configuration and dependencies

Update Jest configuration to handle ESM-only dependencies like `get-port` and bump multiple devDependencies (including Zemu, Ledgerhq, and ESLint plugins) to their latest versions.

* test: update zemu snapshots

Update the main menu UI snapshots for various device models (Nano S, Nano S+, Nano X, Stax, Flex) to reflect recent changes.

* refactor: centralize review lock management in main dispatcher

Move the `review_pending` flag logic from individual command handlers into the central APDU dispatcher. This ensures the lock is consistently acquired at the start of processing and automatically released upon any command completion (success or error) or transport reset, reducing the risk of the dispatcher becoming permanently locked.

Author: chcmedeiros

Makefile

@@ -37,7 +37,7 @@ GIT_DESCRIBE=$(shell git describe --tags --abbrev=8 --always --long --dirty 2>/d
 VERSION_TAG=$(shell echo $(GIT_DESCRIBE) | sed 's/^v//g')
 APPVERSION_M=1
 APPVERSION_N=6
-APPVERSION_P=3
+APPVERSION_P=4
 APPVERSION=$(APPVERSION_M).$(APPVERSION_N).$(APPVERSION_P)
 APPNAME = "Mina"
 

src/globals.c

@@ -6,7 +6,10 @@ unsigned int ux_step;
 unsigned int ux_step_count;
 const internalStorage_t N_storage_real;
 
+bool review_pending = false;
+
 void sendResponse(uint8_t tx, bool approve) {
+    review_pending = false;
     G_io_apdu_buffer[tx++] = approve? 0x90 : 0x69;
     G_io_apdu_buffer[tx++] = approve? 0x00 : 0x85;
     // Send back the response, do not restart the event loop

src/globals.h

@@ -3,6 +3,7 @@
 #include <os.h>
 #include <ux.h>
 #include <os_io_seproxyhal.h>
+#include <stdbool.h>
 
 #define P1_FIRST 0x00
 #define P1_MORE 0x80
@@ -21,6 +22,12 @@ typedef struct internalStorage_t {
 extern const internalStorage_t N_storage_real;
 #define N_storage (*(volatile internalStorage_t*) PIC(&N_storage_real))
 
+// True while a user-confirmation UI flow is open and the device has
+// deferred its APDU reply with IO_ASYNCH_REPLY. Any new APDU arriving
+// in this state must be rejected so the host cannot mutate the bytes
+// being reviewed.
+extern bool review_pending;
+
 void sendResponse(uint8_t tx, bool approve);
 
     // type            userid    x    y   w    h  str rad fill      fg        bg      fid iid  txt   touchparams...       ]

src/main.c

@@ -48,6 +48,20 @@ void handleApdu(volatile unsigned int *flags, volatile unsigned int *tx,
                 THROW(0x6E00);
             }
 
+            // Refuse any new command while a user-confirmation screen is
+            // still waiting for approval. Without this, a second APDU can
+            // race into handler state (globals, review buffers) during the
+            // asynchronous review window — exploitable on transports that
+            // do not gate new APDUs at the SDK layer (BLE).
+            if (review_pending) {
+                THROW(0x6985);
+            }
+            // An APDU is now in flight. Non-review commands release this
+            // lock when their response is emitted (sendResponse or the
+            // THROW handled in app_main's CATCH_OTHER). Review-based
+            // commands keep it until the user approves/rejects.
+            review_pending = true;
+
             // Check user supplied command data length against the actual
             // length to make sure it's not a lie!
             uint8_t dataLength = G_io_apdu_buffer[OFFSET_LC];
@@ -172,6 +186,11 @@ void app_main(void) {
                 handleApdu(&flags, &tx, rx);
             }
             CATCH(EXCEPTION_IO_RESET) {
+              // Transport went away mid-review (BLE disconnect, USB
+              // replug). The deferred reply will never arrive, so drop
+              // the lock before we unwind — otherwise the dispatcher
+              // stays frozen after re-init.
+              review_pending = false;
               THROW(EXCEPTION_IO_RESET);
             }
             CATCH_OTHER(e) {
@@ -191,6 +210,11 @@ void app_main(void) {
                 if (e != APDU_CODE_OK) {
                     flags &= ~IO_ASYNCH_REPLY;
                 }
+                // Any THROW exits the APDU — whether a success code
+                // (e.g. GET_CONF) or an error. Handlers that genuinely
+                // own a review return normally and keep the lock; we
+                // only reach here when the APDU is done.
+                review_pending = false;
                 // Unexpected exception => report
                 G_io_apdu_buffer[tx] = sw >> 8;
                 G_io_apdu_buffer[tx + 1] = sw;

tests_zemu/jest.config.js

@@ -1,7 +1,28 @@
+// ESM-only dependencies that must be transpiled to CommonJS before Jest
+// can require() them. `get-port` went ESM-only in v6; @zondax/zemu still
+// require()s it. Add more names here if another transitive dep ships ESM.
+const esmDeps = ['get-port']
+
 module.exports = {
   preset: 'ts-jest',
   testEnvironment: 'node',
-  transformIgnorePatterns: ['^.+\\.js$'],
+  transform: {
+    '^.+\\.tsx?$': 'ts-jest',
+    '^.+\\.m?js$': [
+      'ts-jest',
+      {
+        useESM: false,
+        isolatedModules: true,
+        tsconfig: {
+          allowJs: true,
+          esModuleInterop: true,
+          module: 'CommonJS',
+          target: 'ES2020',
+        },
+      },
+    ],
+  },
+  transformIgnorePatterns: [`/node_modules/(?!(${esmDeps.join('|')})/)`],
   reporters: ['default', ['summary', { summaryThreshold: 1 }]],
   globalSetup: './globalsetup.js',
   testPathIgnorePatterns: ['/node_modules/', '/dist/'],

tests_zemu/package.json

@@ -27,34 +27,34 @@
   "dependencies": {
     "@mina-wallet-adapter/mina-ledger-js": "^1.0.9",
     "@zondax/ledger-mina-js": "^0.2.0",
-    "@zondax/zemu": "^0.65.0",
+    "@zondax/zemu": "^0.66.2",
     "bs58check": "^4.0.0",
     "mina-signer": "^3.1.0",
     "o1js": "^2.2.0"
   },
   "devDependencies": {
-    "@ledgerhq/hw-transport-node-hid": "^6.29.13",
-    "@ledgerhq/logs": "^6.13.0",
-    "@noble/curves": "^2.0.1",
+    "@ledgerhq/hw-transport-node-hid": "^6.33.0",
+    "@ledgerhq/logs": "^6.17.0",
+    "@noble/curves": "^2.2.0",
     "@trivago/prettier-plugin-sort-imports": "^5.2.2",
     "@types/jest": "^30.0.0",
     "@types/ledgerhq__hw-transport": "^6.0.0",
-    "@typescript-eslint/eslint-plugin": "^8.46.1",
-    "@typescript-eslint/parser": "^8.46.1",
+    "@typescript-eslint/eslint-plugin": "^8.58.2",
+    "@typescript-eslint/parser": "^8.58.2",
     "blakejs": "^1.2.1",
     "crypto-js": "4.2.0",
     "eslint": "^9.37.0",
     "eslint-config-prettier": "^10.1.8",
     "eslint-plugin-import": "^2.32.0",
-    "eslint-plugin-jest": "^29.0.1",
-    "eslint-plugin-prettier": "^5.5.4",
+    "eslint-plugin-jest": "^29.15.2",
+    "eslint-plugin-prettier": "^5.5.5",
     "eslint-plugin-promise": "^7.2.1",
     "eslint-plugin-tsdoc": "^0.4.0",
-    "eslint-plugin-unused-imports": "^4.2.0",
-    "jest": "^30.2.0",
+    "eslint-plugin-unused-imports": "^4.4.1",
+    "jest": "^30.3.0",
     "jssha": "^3.3.1",
-    "prettier": "^3.6.2",
-    "ts-jest": "^29.4.5",
+    "prettier": "^3.8.3",
+    "ts-jest": "^29.4.9",
     "ts-node": "^10.9.2",
     "typescript": "^5.9.3"
   }