fix: v3.8.0 preset-store review findings

- /api/send-ota no longer 500s on a counter-less body: the plain `sp` path
  reports final_cntr only when the counter is usable (_counter_valid, 1-10 hex
  digits) - before, the new plain-path report ran int('') and the request died
  in the generic handler.  The failure path is guarded the same way.
- _decode_por wraps the keyset/SPI construction in the same "never raise" rule
  as the decode: a malformed counter or missing keys return None instead of
  escaping the handler.
- presets.add() never accepts a taken id (a fresh uuid is assigned, like the
  import path), the counter is validated as 1-10 hex digits instead of being
  silently masked/kept, and set_counter() ignores a malformed value with a log
  line instead of writing it.
- the preset endpoints answer a JSON 500 when the store write fails (read-only
  home, disk full) instead of dropping the connection.
- tests: counter-less send-ota, store-write 500, duplicate id, counter
  validation (add/update/set_counter), and the temp-file check no longer
  matches the audit file; docs/api.md documents final_cntr for the plain path
  and preset_id.

docs/api.md

@@ -351,6 +351,13 @@ packets consume counters).  Without the key the command data is sent
 byte-exact: applet-directed payloads (HTTP OTA triggers, §9 push, expanded
 scripts) must not be wrapped.
 
+`final_cntr` is reported for the plain `sp` path too (a pre-built packet):
+the counter that was sent, advanced by one when the card accepted the packet.
+A request without a `cntr` (nothing to advance) simply reports no
+`final_cntr` — it never fails the send.  With `preset_id` the server persists
+the counter into the card preset store (see *Card presets*), so a lost
+response or a closed tab cannot lose the increment.
+
 **Request body:**
 ```json
 {
@@ -363,6 +370,7 @@ scripts) must not be wrapped.
   "cntr": "0000000001",
   "kicKey": "D6FCC023...",
   "kidKey": "1B07E7E0...",
+  "preset_id": "7ee0367f3a444b28988fea5fbc50de9f",
   "ram_format": "auto"
 }
 ```

pysim_simple_server/presets.py

@@ -208,7 +208,9 @@ class PresetStore:
             out[k] = v
         out['spi1'] = out['spi1'] or '16'
         out['spi2'] = out['spi2'] or '01'
-        out['cntr'] = counter_hex(out['cntr'] or '1')
+        # the counter is kept as typed here and validated + padded in
+        # _validate(): a bad value must be refused, never silently masked
+        out['cntr'] = out['cntr'] or '1'
         for k, d in TAR_DEFAULTS.items():
             out[k] = out[k] or d
         pid = str(fields.get('id') or '').strip().lower()
@@ -225,6 +227,10 @@ class PresetStore:
             raise PresetError('PSK identity and PSK key must be set together')
         if p['pskKey'] and not re.fullmatch(r'[0-9A-F]{32}', p['pskKey']):
             raise PresetError('PSK key must be 32 hex characters')
+        if not re.fullmatch(r'[0-9A-F]{1,10}', p['cntr']):
+            raise PresetError('counter must be 1-10 hex digits')
+        # validated: store the protocol's fixed-width form
+        p['cntr'] = counter_hex(p['cntr'])
         norm = normalize_iccid(p['iccid'])
         if norm:
             for other in self._presets:
@@ -267,6 +273,10 @@ class PresetStore:
     def add(self, fields):
         with self._lock:
             p = self._normalise(fields or {})
+            if self._find(p['id']):
+                # a caller-supplied id that is already taken never wins: ids
+                # must stay unique (the same guard the import uses)
+                p['id'] = uuid.uuid4().hex
             self._validate(p)
             self._presets.append(p)
             self._save()
@@ -315,7 +325,12 @@ class PresetStore:
             p = self._find(pid)
             if p is None:
                 return None
-            new = counter_hex(cntr)
+            new = str(cntr or '').strip().upper()
+            if not re.fullmatch(r'[0-9A-F]{1,10}', new):
+                sys.stderr.write('PRESETS: ignoring malformed counter %r (%s)\n'
+                                 % (cntr, source))
+                return dict(p)
+            new = counter_hex(new)
             if new == p['cntr']:
                 return dict(p)
             if not counter_ahead(p['cntr'], new):

pysim_simple_server/server.py

@@ -1412,6 +1412,13 @@ def _ram_next_cntr(cntr, advance):
     return '%010X' % ((int(cntr, 16) + 1) % (2 ** 40))
 
 
+def _counter_valid(cntr):
+    """True when an SCP80 counter is usable (1-10 hex digits).  A caller
+    sending a pre-built packet may have no counter at all - the empty value
+    must never reach int() (v3.8.0 review fix)."""
+    return bool(re.fullmatch(r'[0-9A-Fa-f]{1,10}', str(cntr or '')))
+
+
 def _preset_counter_persist(server, preset_id, cntr, source):
     """Persist the counter an operation accepted into the server-side preset
     store (v3.8.0).  The counter is card state, so the server writes it with
@@ -1736,8 +1743,13 @@ def _decode_por(spi1, spi2, kic, kid, cntr_hex, kic_key_hex, kid_key_hex, respon
     from osmocom.utils import h2b, b2h
     if not response_hex:
         return None
-    otak = _ota_keyset(spi1, spi2, kic, kid, cntr_hex, kic_key_hex, kid_key_hex)
+    try:
-    spi = _spi_from_bytes(int(spi1, 16), int(spi2, 16))
+        otak = _ota_keyset(spi1, spi2, kic, kid, cntr_hex, kic_key_hex, kid_key_hex)
+        spi = _spi_from_bytes(int(spi1, 16), int(spi2, 16))
+    except Exception:
+        # a malformed counter/key (e.g. a hand-made request) is not a PoR
+        # either - the same rule as the decode below: never raise here.
+        return None
     try:
         data = h2b(response_hex)
         if not data or data[0] != 0x02:
@@ -5634,6 +5646,13 @@ class PysimHandler(BaseHTTPRequestHandler):
             return None
         return store
 
+    def _preset_store_error(self, e):
+        """A store I/O failure (read-only home, disk full, ...) answers a JSON
+        500 instead of escaping the handler as a dropped connection."""
+        resp = {'error': 'preset store write failed: %s' % e}
+        self._send_json(resp, 500)
+        self._log_resp(resp)
+
     def _log_req(self, body=None):
         if self.server.log_requests:
             if body is not None:
@@ -6777,6 +6796,7 @@ class PysimHandler(BaseHTTPRequestHandler):
                         resp = result
                         if ram_format is not None:
                             resp['ram_format'] = ram_format
+                        if _counter_valid(cntr):
                             resp['final_cntr'] = cntr
                         # A warning ENVELOPE may still carry the PoR (the
                         # card's counter verdict): decode it so the caller
@@ -6853,7 +6873,10 @@ class PysimHandler(BaseHTTPRequestHandler):
                             resp['ram_format'] = ram_format
                         # The counter to use next, reported for the RAM and the
                         # plain SCP80 path alike; the server persists it (v3.8.0).
-                        resp['final_cntr'] = _ram_next_cntr(cntr, accepted)
+                        # A pre-built packet may come without a counter - then
+                        # nothing is reported (and int('') never happens).
+                        if _counter_valid(cntr):
+                            resp['final_cntr'] = _ram_next_cntr(cntr, accepted)
                 finally:
                     if submit_handler and hasattr(scc, '_tp'):
                         scc._tp.proactive_handler = old_proactive
@@ -7384,6 +7407,9 @@ class PysimHandler(BaseHTTPRequestHandler):
                 self._send_json(resp, 400)
                 self._log_resp(resp)
                 return
+            except OSError as e:
+                self._preset_store_error(e)
+                return
             resp = {'ok': True, 'preset': preset}
             self._send_json(resp)
             self._log_resp(resp)
@@ -7403,6 +7429,9 @@ class PysimHandler(BaseHTTPRequestHandler):
                 self._send_json(resp, 400)
                 self._log_resp(resp)
                 return
+            except OSError as e:
+                self._preset_store_error(e)
+                return
             if preset is None:
                 resp = {'error': 'unknown preset id'}
                 self._send_json(resp, 404)
@@ -7417,7 +7446,11 @@ class PysimHandler(BaseHTTPRequestHandler):
             store = self._preset_store_or_503()
             if store is None:
                 return
-            removed = store.remove(body.get('id'))
+            try:
+                removed = store.remove(body.get('id'))
+            except OSError as e:
+                self._preset_store_error(e)
+                return
             resp = {'ok': True, 'removed': removed}
             self._send_json(resp)
             self._log_resp(resp)
@@ -7434,7 +7467,11 @@ class PysimHandler(BaseHTTPRequestHandler):
                 self._log_resp(resp)
                 return
             mode = body.get('mode') if body.get('mode') in ('merge', 'replace') else 'merge'
-            resp = store.import_presets(items, mode)
+            try:
+                resp = store.import_presets(items, mode)
+            except OSError as e:
+                self._preset_store_error(e)
+                return
             resp['ok'] = True
             self._send_json(resp)
             self._log_resp(resp)

tests/test_fetch_routes.py

@@ -559,6 +559,36 @@ class PresetStoreHttpTests(unittest.TestCase):
         self.assertEqual(resp['final_cntr'], '0000000002')
         self.assertEqual(self.store.get(p['id'])['cntr'], '0000000002')
 
+    def test_a_counter_less_send_ota_body_still_answers(self):
+        # a pre-built packet without a counter: no final_cntr, no persist,
+        # and above all no int('') crash (v3.8.0 review fix)
+        p = self.store.add(self._preset(iccid='8970119000004600098'))
+        patches = [
+            mock.patch.object(self.srv, '_send_secured_packet', lambda *a, **k: {
+                'success': True, 'sw': '9000', 'bytes': 34, 'segments': 1,
+                'response_data': ''}),
+            mock.patch.object(self.srv, '_decode_por', lambda *a, **k: None),
+        ]
+        self.server.app = object()
+        for patch in patches:
+            patch.start()
+        try:
+            status, resp = self._post('/api/send-ota', {
+                'sp': '00', 'spi1': '16', 'spi2': '01', 'kic': '15', 'kid': '15',
+                'tar': '000000', 'preset_id': p['id']})
+        finally:
+            for patch in reversed(patches):
+                patch.stop()
+        self.assertEqual(status, 200, resp)
+        self.assertNotIn('final_cntr', resp)
+        self.assertEqual(self.store.get(p['id'])['cntr'], '0000000001')
+
+    def test_a_store_write_failure_answers_a_json_500(self):
+        with mock.patch.object(self.store, 'add', side_effect=OSError('read-only file system')):
+            status, resp = self._post('/api/presets', self._preset())
+        self.assertEqual(status, 500, resp)
+        self.assertIn('preset store write failed', resp['error'])
+
     def test_the_store_endpoints_answer_503_without_a_store(self):
         self.server.card_presets = None
         status, resp = self._get('/api/presets')

tests/test_presets.py

@@ -117,6 +117,27 @@ class StoreCrudTests(unittest.TestCase):
         self.assertFalse(self.store.remove(p['id']))
         self.assertEqual(self.store.list(), [])
 
+    def test_a_caller_supplied_id_never_collides(self):
+        first = self.store.add(preset())
+        second = self.store.add(dict(preset(name='B'), id=first['id']))
+        self.assertNotEqual(second['id'], first['id'])
+        self.assertEqual(len({p['id'] for p in self.store.list()}), 2)
+
+    def test_the_counter_must_be_hex_and_at_most_ten_digits(self):
+        for bad in ('ZZ', '00000000000000000000'):
+            with self.assertRaises(presets.PresetError) as ctx:
+                self.store.add(preset(cntr=bad))
+            self.assertIn('counter', str(ctx.exception))
+        # whitespace is cleaned and a short value is stored fixed-width
+        self.assertEqual(self.store.add(preset(cntr='12 34'))['cntr'], '0000001234')
+        self.assertEqual(self.store.add(preset(cntr='abc'))['cntr'], '0000000ABC')
+
+    def test_update_validates_the_counter_too(self):
+        p = self.store.add(preset())
+        with self.assertRaises(presets.PresetError):
+            self.store.update(p['id'], {'cntr': 'ZZ'})
+        self.assertEqual(self.store.get(p['id'])['cntr'], '0000000001')
+
     def test_find_by_iccid(self):
         p = self.store.add(preset(iccid=DIGITS))
         self.assertEqual(self.store.find_by_iccid(RAW_HEX)['id'], p['id'])
@@ -160,6 +181,14 @@ class StoreCounterTests(unittest.TestCase):
     def test_set_counter_on_unknown_id_is_none(self):
         self.assertIsNone(self.store.set_counter('nope', '0000000002'))
 
+    def test_set_counter_ignores_a_malformed_value(self):
+        self.store.set_counter(self.p['id'], 'ZZ', 'send-ota')
+        self.assertEqual(self.store.get(self.p['id'])['cntr'], '0000000001')
+        self.store.set_counter(self.p['id'], '00000000000000000000', 'send-ota')
+        self.assertEqual(self.store.get(self.p['id'])['cntr'], '0000000001')
+        self.store.set_counter(self.p['id'], '', 'send-ota')
+        self.assertEqual(self.store.get(self.p['id'])['cntr'], '0000000001')
+
     def test_counter_changes_are_audited(self):
         self.store.set_counter(self.p['id'], '0000000005', 'send-ota')
         lines = self.store.audit_path.read_text(encoding='utf-8').strip().splitlines()
@@ -189,10 +218,11 @@ class StorePersistenceTests(unittest.TestCase):
 
     def test_no_temporary_files_are_left_behind(self):
         store = make_store(self.tmp.name)
-        store.add(preset())
+        p = store.add(preset())
         store.add(preset(name='Second'))
+        store.set_counter(p['id'], '0000000005', 'send-ota')   # writes the audit file
         leftovers = [f.name for f in pathlib.Path(self.tmp.name).iterdir()
-                     if '.json.' in f.name]
+                     if f.name not in ('card_presets.json', 'card_presets.json.audit.jsonl')]
         self.assertEqual(leftovers, [])
 
     def test_a_corrupt_store_is_kept_and_the_store_starts_empty(self):