diff --git a/docs/api.md b/docs/api.md index 234fb19..bcc778d 100644 --- a/docs/api.md +++ b/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" } ``` diff --git a/pysim_simple_server/presets.py b/pysim_simple_server/presets.py index 538b0ad..a421ca7 100644 --- a/pysim_simple_server/presets.py +++ b/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): diff --git a/pysim_simple_server/server.py b/pysim_simple_server/server.py index a35710f..efabbbd 100644 --- a/pysim_simple_server/server.py +++ b/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) - spi = _spi_from_bytes(int(spi1, 16), int(spi2, 16)) + try: + 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) diff --git a/tests/test_fetch_routes.py b/tests/test_fetch_routes.py index fc16215..bc3cc3e 100644 --- a/tests/test_fetch_routes.py +++ b/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') diff --git a/tests/test_presets.py b/tests/test_presets.py index c86cd04..a31555c 100644 --- a/tests/test_presets.py +++ b/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):