fix(ook): address upstream PR review — SDR tracking, validation, cleanup, XSS

Critical:
- Pass sdr_type_str to claim/release_sdr_device (was missing 3rd arg)
- Add ook_active_sdr_type module-level var for proper device registry tracking
- Add server-side range validation on all timing params via validate_positive_int

Major:
- Extract cleanup_ook() function for full teardown (stop_event, pipes, process,
  SDR release) — called from both stop_ook() and kill_all()
- Replace Popen monkey-patching with module-level _ook_stop_event/_ook_parser_thread
- Fix XSS: define local _esc() fallback in ook.js, never use raw innerHTML
- Remove dead inversion code path in utils/ook.py (bytes.fromhex on same
  string that already failed decode — could never produce a result)

Minor:
- Status event key 'status' → 'text' for consistency with other modules
- Parser thread logging: debug → warning for missing code field and errors
- Parser thread emits status:stopped on exit (normal EOF or crash)
- Add cache-busting ?v={{ version }}&r=ook1 to ook.js script include
- Fix gain/ppm comparison: != '0' (string) → != 0 (number)

Tests: 22 → 33 (added start success, stop with process, SSE stream,
timing range validation, stopped-on-exit event)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

app.py

@@ -896,12 +896,16 @@ def kill_all() -> Response:
     with morse_lock:
         morse_process = None
 
-    # Reset OOK state
+    # Reset OOK state (full cleanup: parser thread, pipes, SDR release)
     with ook_lock:
-        if ook_process:
+        try:
-            safe_terminate(ook_process)
+            from routes.ook import cleanup_ook
-            unregister_process(ook_process)
+            cleanup_ook(emit_status=False)
-        ook_process = None
+        except Exception:
+            if ook_process:
+                safe_terminate(ook_process)
+                unregister_process(ook_process)
+            ook_process = None
 
     # Reset APRS state
     with aprs_lock:

routes/ook.py

@@ -26,6 +26,7 @@ from utils.validation import (
     validate_device_index,
     validate_frequency,
     validate_gain,
+    validate_positive_int,
     validate_ppm,
     validate_rtl_tcp_host,
     validate_rtl_tcp_port,
@@ -33,8 +34,13 @@ from utils.validation import (
 
 ook_bp = Blueprint('ook', __name__)
 
-# Track which device is being used
+# Track which device / SDR type is being used
 ook_active_device: int | None = None
+ook_active_sdr_type: str | None = None
+
+# Parser thread state (avoids monkey-patching subprocess.Popen)
+_ook_stop_event: threading.Event | None = None
+_ook_parser_thread: threading.Thread | None = None
 
 # Supported modulation schemes → rtl_433 flex decoder modulation string
 _MODULATION_MAP = {
@@ -53,7 +59,7 @@ def _validate_encoding(value: Any) -> str:
 
 @ook_bp.route('/ook/start', methods=['POST'])
 def start_ook() -> Response:
-    global ook_active_device
+    global ook_active_device, ook_active_sdr_type, _ook_stop_event, _ook_parser_thread
 
     with app_module.ook_lock:
         if app_module.ook_process:
@@ -74,24 +80,36 @@ def start_ook() -> Response:
         except ValueError as e:
             return jsonify({'status': 'error', 'message': str(e)}), 400
 
-        # OOK flex decoder timing parameters
+        # OOK flex decoder timing parameters (server-side range validation)
         try:
-            short_pulse = int(data.get('short_pulse', 300))
+            short_pulse = validate_positive_int(data.get('short_pulse', 300), 'short_pulse', max_val=100000)
-            long_pulse = int(data.get('long_pulse', 600))
+            long_pulse = validate_positive_int(data.get('long_pulse', 600), 'long_pulse', max_val=100000)
-            reset_limit = int(data.get('reset_limit', 8000))
+            reset_limit = validate_positive_int(data.get('reset_limit', 8000), 'reset_limit', max_val=1000000)
-            gap_limit = int(data.get('gap_limit', 5000))
+            gap_limit = validate_positive_int(data.get('gap_limit', 5000), 'gap_limit', max_val=1000000)
-            tolerance = int(data.get('tolerance', 150))
+            tolerance = validate_positive_int(data.get('tolerance', 150), 'tolerance', max_val=50000)
-            min_bits = int(data.get('min_bits', 8))
+            min_bits = validate_positive_int(data.get('min_bits', 8), 'min_bits', max_val=4096)
-        except (ValueError, TypeError) as e:
+        except ValueError as e:
             return jsonify({'status': 'error', 'message': f'Invalid timing parameter: {e}'}), 400
+        if min_bits < 1:
+            return jsonify({'status': 'error', 'message': 'min_bits must be >= 1'}), 400
+        if short_pulse < 1 or long_pulse < 1:
+            return jsonify({'status': 'error', 'message': 'Pulse widths must be >= 1'}), 400
         deduplicate = bool(data.get('deduplicate', False))
 
+        # Parse SDR type early — needed for device claim
+        sdr_type_str = data.get('sdr_type', 'rtlsdr')
+        try:
+            sdr_type = SDRType(sdr_type_str)
+        except ValueError:
+            sdr_type = SDRType.RTL_SDR
+            sdr_type_str = 'rtlsdr'
+
         rtl_tcp_host = data.get('rtl_tcp_host') or None
         rtl_tcp_port = data.get('rtl_tcp_port', 1234)
 
         if not rtl_tcp_host:
             device_int = int(device)
-            error = app_module.claim_sdr_device(device_int, 'ook')
+            error = app_module.claim_sdr_device(device_int, 'ook', sdr_type_str)
             if error:
                 return jsonify({
                     'status': 'error',
@@ -99,6 +117,7 @@ def start_ook() -> Response:
                     'message': error,
                 }), 409
             ook_active_device = device_int
+            ook_active_sdr_type = sdr_type_str
 
         while not app_module.ook_queue.empty():
             try:
@@ -106,12 +125,6 @@ def start_ook() -> Response:
             except queue.Empty:
                 break
 
-        sdr_type_str = data.get('sdr_type', 'rtlsdr')
-        try:
-            sdr_type = SDRType(sdr_type_str)
-        except ValueError:
-            sdr_type = SDRType.RTL_SDR
-
         if rtl_tcp_host:
             try:
                 rtl_tcp_host = validate_rtl_tcp_host(rtl_tcp_host)
@@ -130,8 +143,8 @@ def start_ook() -> Response:
         cmd = builder.build_ism_command(
             device=sdr_device,
             frequency_mhz=freq,
-            gain=float(gain) if gain and gain != '0' else None,
+            gain=float(gain) if gain and gain != 0 else None,
-            ppm=int(ppm) if ppm and ppm != '0' else None,
+            ppm=int(ppm) if ppm and ppm != 0 else None,
             bias_t=bias_t,
         )
 
@@ -195,11 +208,11 @@ def start_ook() -> Response:
             parser_thread.start()
 
             app_module.ook_process = rtl_process
-            app_module.ook_process._stop_parser = stop_event
+            _ook_stop_event = stop_event
-            app_module.ook_process._parser_thread = parser_thread
+            _ook_parser_thread = parser_thread
 
             try:
-                app_module.ook_queue.put_nowait({'type': 'status', 'status': 'started'})
+                app_module.ook_queue.put_nowait({'type': 'status', 'text': 'started'})
             except queue.Full:
                 logger.warning("OOK 'started' status dropped — queue full")
 
@@ -214,8 +227,9 @@ def start_ook() -> Response:
 
         except FileNotFoundError as e:
             if ook_active_device is not None:
-                app_module.release_sdr_device(ook_active_device)
+                app_module.release_sdr_device(ook_active_device, ook_active_sdr_type or 'rtlsdr')
                 ook_active_device = None
+                ook_active_sdr_type = None
             return jsonify({'status': 'error', 'message': f'Tool not found: {e.filename}'}), 400
 
         except Exception as e:
@@ -227,8 +241,9 @@ def start_ook() -> Response:
                     rtl_process.kill()
             unregister_process(rtl_process)
             if ook_active_device is not None:
-                app_module.release_sdr_device(ook_active_device)
+                app_module.release_sdr_device(ook_active_device, ook_active_sdr_type or 'rtlsdr')
                 ook_active_device = None
+                ook_active_sdr_type = None
             return jsonify({'status': 'error', 'message': str(e)}), 500
 
 
@@ -239,40 +254,54 @@ def _close_pipe(pipe_obj) -> None:
             pipe_obj.close()
 
 
+def cleanup_ook(*, emit_status: bool = True) -> None:
+    """Full OOK cleanup: stop parser, terminate process, release SDR device.
+
+    Safe to call from ``stop_ook()`` and ``kill_all()``.  Caller must hold
+    ``app_module.ook_lock``.
+    """
+    global ook_active_device, ook_active_sdr_type, _ook_stop_event, _ook_parser_thread
+
+    proc = app_module.ook_process
+    if not proc:
+        return
+
+    # Signal parser thread to stop
+    if _ook_stop_event:
+        _ook_stop_event.set()
+
+    # Close pipes so parser thread unblocks from readline()
+    _close_pipe(getattr(proc, 'stdout', None))
+    _close_pipe(getattr(proc, 'stderr', None))
+
+    safe_terminate(proc)
+    unregister_process(proc)
+    app_module.ook_process = None
+
+    # Join parser thread with timeout
+    if _ook_parser_thread:
+        _ook_parser_thread.join(timeout=0.5)
+
+    _ook_stop_event = None
+    _ook_parser_thread = None
+
+    if ook_active_device is not None:
+        app_module.release_sdr_device(ook_active_device, ook_active_sdr_type or 'rtlsdr')
+        ook_active_device = None
+        ook_active_sdr_type = None
+
+    if emit_status:
+        try:
+            app_module.ook_queue.put_nowait({'type': 'status', 'text': 'stopped'})
+        except queue.Full:
+            logger.warning("OOK 'stopped' status dropped — queue full")
+
+
 @ook_bp.route('/ook/stop', methods=['POST'])
 def stop_ook() -> Response:
-    global ook_active_device
-
     with app_module.ook_lock:
         if app_module.ook_process:
-            proc = app_module.ook_process
+            cleanup_ook()
-            stop_event = getattr(proc, '_stop_parser', None)
-            parser_thread = getattr(proc, '_parser_thread', None)
-
-            # Signal parser thread to stop
-            if stop_event:
-                stop_event.set()
-
-            # Close pipes so parser thread unblocks from readline()
-            _close_pipe(getattr(proc, 'stdout', None))
-            _close_pipe(getattr(proc, 'stderr', None))
-
-            safe_terminate(proc)
-            unregister_process(proc)
-            app_module.ook_process = None
-
-            # Join parser thread with timeout
-            if parser_thread:
-                parser_thread.join(timeout=0.5)
-
-            if ook_active_device is not None:
-                app_module.release_sdr_device(ook_active_device)
-                ook_active_device = None
-
-            try:
-                app_module.ook_queue.put_nowait({'type': 'status', 'status': 'stopped'})
-            except queue.Full:
-                logger.warning("OOK 'stopped' status dropped — queue full")
             return jsonify({'status': 'stopped'})
 
         return jsonify({'status': 'not_running'})

static/js/modes/ook.js

@@ -12,6 +12,13 @@ var OokMode = (function () {
     var DEFAULT_FREQ_PRESETS = ['433.920', '315.000', '868.000', '915.000'];
     var MAX_FRAMES = 5000;
 
+    // Local XSS-safe escape — never fall back to raw innerHTML
+    var _esc = typeof escapeHtml === 'function' ? escapeHtml : function (s) {
+        var d = document.createElement('div');
+        d.textContent = s;
+        return d.innerHTML;
+    };
+
     var state = {
         running: false,
         initialized: false,
@@ -147,7 +154,7 @@ var OokMode = (function () {
         if (msg.type === 'ook_frame') {
             handleFrame(msg);
         } else if (msg.type === 'status') {
-            if (msg.status === 'stopped') {
+            if (msg.text === 'stopped') {
                 state.running = false;
                 updateUI(false);
                 disconnectSSE();
@@ -245,7 +252,7 @@ var OokMode = (function () {
             '</span>' +
             '<br>' +
             '<span style="padding-left:8em; color:' + (hasPrintable ? '#aaffcc' : '#555') + '; font-family:var(--font-mono); font-size:10px">' +
-            'ascii: ' + (typeof escapeHtml === 'function' ? escapeHtml(interp.ascii) : interp.ascii) +
+            'ascii: ' + _esc(interp.ascii) +
             '</span>';
 
         div.style.cssText = 'font-size:11px; padding: 4px 0; border-bottom: 1px solid #1a1a1a; line-height:1.6;';

templates/index.html

@@ -3393,7 +3393,7 @@
     <script src="{{ url_for('static', filename='js/modes/bt_locate.js') }}?v={{ version }}&r=btlocate4"></script>
     <script src="{{ url_for('static', filename='js/modes/wefax.js') }}"></script>
     <script src="{{ url_for('static', filename='js/modes/morse.js') }}?v={{ version }}&r=morse_iq12"></script>
-    <script src="{{ url_for('static', filename='js/modes/ook.js') }}"></script>
+    <script src="{{ url_for('static', filename='js/modes/ook.js') }}?v={{ version }}&r=ook1"></script>
     <script src="{{ url_for('static', filename='js/modes/space-weather.js') }}"></script>
     <script src="{{ url_for('static', filename='js/modes/system.js') }}"></script>
     <script src="{{ url_for('static', filename='js/modes/meteor.js') }}"></script>

tests/test_ook.py

@@ -6,12 +6,12 @@ import io
 import json
 import queue
 import threading
+import unittest.mock
 
 import pytest
 
 from utils.ook import decode_ook_frame, ook_parser_thread
 
-
 # ---------------------------------------------------------------------------
 # Helpers
 # ---------------------------------------------------------------------------
@@ -250,3 +250,112 @@ class TestOokRoutes:
                            json={'frequency': '-5'},
                            content_type='application/json')
         assert resp.status_code == 400
+
+    def test_start_rejects_out_of_range_timing(self, client):
+        """Timing params that exceed server-side max should be rejected."""
+        _login_session(client)
+        resp = client.post('/ook/start',
+                           json={'short_pulse': 999999},
+                           content_type='application/json')
+        assert resp.status_code == 400
+
+    def test_start_rejects_negative_timing(self, client):
+        _login_session(client)
+        resp = client.post('/ook/start',
+                           json={'min_bits': -1},
+                           content_type='application/json')
+        assert resp.status_code == 400
+
+    def test_start_success_mocked(self, client, monkeypatch):
+        """start_ook with mocked Popen should return 'started'."""
+        import subprocess
+
+        import app as app_module
+
+        _login_session(client)
+
+        mock_proc = unittest.mock.MagicMock()
+        mock_proc.poll.return_value = None
+        mock_proc.stdout = io.BytesIO(b'')
+        mock_proc.stderr = io.BytesIO(b'')
+        mock_proc.pid = 12345
+
+        monkeypatch.setattr(subprocess, 'Popen', lambda *a, **kw: mock_proc)
+        monkeypatch.setattr(app_module, 'claim_sdr_device', lambda *a, **kw: None)
+        monkeypatch.setattr(app_module, 'release_sdr_device', lambda *a, **kw: None)
+
+        resp = client.post('/ook/start',
+                           json={'frequency': '433.920'},
+                           content_type='application/json')
+        data = resp.get_json()
+        assert resp.status_code == 200
+        assert data['status'] == 'started'
+        assert 'command' in data
+
+        # Cleanup
+        with app_module.ook_lock:
+            app_module.ook_process = None
+
+    def test_stop_with_running_process(self, client, monkeypatch):
+        """stop_ook should clean up a running process."""
+        import app as app_module
+
+        _login_session(client)
+
+        mock_proc = unittest.mock.MagicMock()
+        mock_proc.poll.return_value = None
+        mock_proc.stdout = None
+        mock_proc.stderr = None
+        mock_proc.pid = 12345
+
+        # Inject a fake running process
+        import routes.ook as ook_module
+        app_module.ook_process = mock_proc
+        ook_module._ook_stop_event = threading.Event()
+        ook_module._ook_parser_thread = None
+        ook_module.ook_active_device = 0
+        ook_module.ook_active_sdr_type = 'rtlsdr'
+
+        monkeypatch.setattr(app_module, 'release_sdr_device', lambda *a, **kw: None)
+        monkeypatch.setattr('utils.process.safe_terminate', lambda p: None)
+        monkeypatch.setattr('utils.process.unregister_process', lambda p: None)
+
+        resp = client.post('/ook/stop')
+        data = resp.get_json()
+        assert resp.status_code == 200
+        assert data['status'] == 'stopped'
+        assert app_module.ook_process is None
+        assert ook_module.ook_active_device is None
+
+    def test_stream_endpoint(self, client):
+        """SSE stream endpoint should return text/event-stream."""
+        _login_session(client)
+        resp = client.get('/ook/stream')
+        assert resp.content_type.startswith('text/event-stream')
+        assert resp.headers.get('Cache-Control') == 'no-cache'
+
+
+# ---------------------------------------------------------------------------
+# Parser thread — stopped status on exit
+# ---------------------------------------------------------------------------
+
+class TestOokParserStoppedEvent:
+    def test_emits_stopped_on_normal_exit(self):
+        """Parser thread should emit a status: stopped event when stream ends."""
+        stdout = io.BytesIO(b'')
+        output_queue = queue.Queue()
+        stop_event = threading.Event()
+
+        t = threading.Thread(
+            target=ook_parser_thread,
+            args=(stdout, output_queue, stop_event),
+        )
+        t.start()
+        t.join(timeout=2)
+
+        events = []
+        while not output_queue.empty():
+            events.append(output_queue.get_nowait())
+        status_events = [e for e in events if e.get('type') == 'status']
+        assert len(status_events) == 1
+        assert status_events[0]['text'] == 'stopped'

utils/ook.py

@@ -77,11 +77,8 @@ def ook_parser_thread(
     """Thread function: reads rtl_433 JSON output and emits OOK frame events.
 
     Handles the three rtl_433 hex-output field names (``codes``, ``code``,
-    ``data``) and, if the initial hex decoding fails, retries with an
+    ``data``).  Emits a ``status: stopped`` event when the parser exits
-    inverted bit interpretation.  This inversion fallback is only applied
+    (normal EOF or unexpected crash) so the frontend can update its UI.
-    when the primary parse yields no usable hex; it does not attempt to
-    reinterpret successfully decoded frames that merely swap the short/long
-    pulse mapping.
 
     Args:
         rtl_stdout: rtl_433 stdout pipe.
@@ -144,7 +141,7 @@ def ook_parser_thread(
                     break
 
             if not codes:
-                logger.debug(
+                logger.warning(
                     f'[rtl_433/ook] no code field — keys: {list(data.keys())}'
                 )
                 try:
@@ -165,21 +162,7 @@ def ook_parser_thread(
                     if brace_end >= 0:
                         hex_str = hex_str[brace_end + 1:]
 
-                inverted = False
                 frame = decode_ook_frame(hex_str)
-                if frame is None:
-                    # Some transmitters use long=0, short=1 (inverted ratio).
-                    try:
-                        inv_bytes = bytes(
-                            b ^ 0xFF
-                            for b in bytes.fromhex(hex_str.replace(' ', ''))
-                        )
-                        frame = decode_ook_frame(inv_bytes.hex())
-                        if frame is not None:
-                            inverted = True
-                    except ValueError:
-                        pass
-
                 if frame is None:
                     continue
 
@@ -199,7 +182,7 @@ def ook_parser_thread(
                         'bits': frame['bits'],
                         'byte_count': frame['byte_count'],
                         'bit_count': frame['bit_count'],
-                        'inverted': inverted,
+                        'inverted': False,
                         'encoding': encoding,
                         'timestamp': timestamp,
                     }
@@ -210,8 +193,15 @@ def ook_parser_thread(
                     pass
 
     except Exception as e:
-        logger.debug(f'OOK parser thread error: {e}')
+        logger.warning(f'OOK parser thread error: {e}')
         try:
             output_queue.put_nowait({'type': 'error', 'text': str(e)})
         except queue.Full:
             pass
+
+    # Notify frontend that the parser has stopped (covers both normal exit
+    # and unexpected rtl_433 crashes so the UI doesn't stay in "Listening").
+    try:
+        output_queue.put_nowait({'type': 'status', 'text': 'stopped'})
+    except queue.Full:
+        pass