66 lines
8.2 KiB
Markdown
66 lines
8.2 KiB
Markdown
# htracker — revisión de código, rama `feature/test500unidades` @ `2cdcb5e` (18-sep-2026)
|
||
|
||
Archivos revisados: `web_monitor.py` (6 286 líneas), `retranslator.py` (1 466), `device_metadata.py`, `web_monitor_ctl.sh`,
|
||
`device_config.json`. No revisados (no estaban en el set): `auth.py`, `gps_logger.py` — ver «Pendiente de verificar».
|
||
Todos los hallazgos están verificados en el código (archivo:línea). Dueño del backend: Gibran.
|
||
|
||
## Resumen ejecutivo (ES)
|
||
|
||
**No es seguro seguir desplegando tal cual.** Tres cosas primero, en este orden:
|
||
|
||
1. **XSS almacenado sin autenticación por UDP (C1) y entre clientes (C2).** `cell_id` llega del `$GPTXT` sin validar y se
|
||
inyecta con `innerHTML` en el panel de desconectadas, el mapa y los reportes; los nombres de unidad y de geocerca tampoco
|
||
se escapan. Cualquiera en internet puede ejecutar JavaScript en el navegador del administrador con un datagrama a UDP 5010.
|
||
Arreglo: `re.fullmatch(r'H\d{4}', cell_id)` en el retranslator + `esc()` en todos los sitios de inyección (una tarde).
|
||
2. **Secretos y datos en git (H1)** — `.session_secret`, `users.json`, `web_monitor.py.bak`, `telemetry_history.db`:
|
||
`git rm --cached`, `.gitignore`, rotar `.session_secret` (invalida sesiones), `git filter-repo`. Y limitar la edición de
|
||
unidades/geocercas a las `cell_id` permitidas del usuario (H4).
|
||
3. **El reenvío a Wialon bloquea por paquete (H2) y reporta «ok» sin leer la respuesta (H3).** Con 500 unidades a 1 Hz se
|
||
descartan posiciones de forma sostenida mientras el tablero dice sano. Rehacer el forwarder (no esperar ACK síncrono,
|
||
validar `#ASD#1`) **antes** del despliegue de 500.
|
||
|
||
Para las 15 unidades v5 de esta semana: C1 es el riesgo real hoy; H2/H3 no muerden a 30 unidades.
|
||
|
||
---
|
||
## Detailed findings (EN, ranked)
|
||
|
||
### CRITICAL
|
||
**C1. Unauthenticated stored XSS via UDP: `cell_id` is never validated and is rendered unescaped.** `retranslator.py:691-695` returns `parts[2]` of any `$GPTXT` datagram verbatim; it becomes a key in every state dict and is emitted in `/tmp/htracker_status.json`. `web_monitor.py:3780` (disconnected panel), `5555`, `5607` (map InfoWindow) interpolate it into `innerHTML` without `esc()`. After `DEVICE_TTL_S` (300 s) the unit lands in `disconnected` and every open dashboard renders it; persists in `htracker_device_state.json` across restarts. Fix: validate (`H\d{4}`), drop and count otherwise; `esc()` every interpolated field.
|
||
|
||
**C2. Authenticated cross-tenant stored XSS through unit names and geofence names.** `web_monitor.py:785-809` lets any logged-in user set `unit_name` for any cell_id (only `.strip()`); `952-960` same for geofence `name`. Rendered unescaped at `4963`, `5069`, `5155`, `5141`, `6115`, `6128`, `5556`, `5608`, `5653`. Fix: escape at every sink; restrict `unit_name` to `[\w \-\.]{1,40}`; gate `POST /api/units/{cell_id}` on `_user_allowed_cell_ids`.
|
||
|
||
### HIGH
|
||
**H1. Secrets/data committed** (`.session_secret`, `users.json`, `web_monitor.py.bak`, `telemetry_history.db`). Session cookies forgeable, hashes crackable offline. `web_monitor_ctl.sh:38,68,69,98` hard-codes the public IP.
|
||
**H2. Wialon forwarder head-of-line blocks per worker.** `retranslator.py:511-516`: `sendall` then blocking `recv` (15 s) per packet. Throughput per worker = 1/RTT; 64 workers ≈ 320–640 pkt/s; 500 units at 1 Hz = 500 pkt/s → sustained oldest-drop (`386-402`). One stalled socket freezes ~7 other units on that worker. Fix: write then drain ACKs non-blocking (`select` across the worker's sockets); add `queue_depth` stat.
|
||
**H3. "acked" counted without checking the response.** `515-516`: `b''`, `#ASD#0`, `#ASD#-1` all count as ack → `wialon_state='ok'` while Wialon rejects. Fix: parse; only `#ASD#1`/`#AD#1` = ok; `b''` = disconnect.
|
||
**H4. Cross-tenant writes by design** (`769-824`, "decisión de producto 2026-08-17") — with C2 this is privilege escalation. Gate on allowed cell_ids; vehicle-type icons full-access only.
|
||
**H5. SVG uploads served same-origin from `/static` unauthenticated** (`372`, `611`) = stored XSS. Drop SVG or serve with `Content-Disposition: attachment` + CSP sandbox.
|
||
|
||
### MEDIUM
|
||
**M1.** Units with no fix are invisible: `_parse_gga` returns `None` on quality 0 (`647`), `_RMC_RE` needs `A` (`217`); `NoFix` chip unreachable. Mark seen + `fix_mode='NoFix'`, skip only forward/odometer.
|
||
**M2.** Hard speed gate ≥ 57.6 km/h (`1169-1173`) drops from Wialon *and* the local DB, not toggleable, not counted — the "healthy unit locked out" case of invariant #5. Make it `GW_MAX_SPEED_KMH`, count `speed_rejects`, keep `_log_to_sql`.
|
||
**M3.** Unbounded state growth: `_cleanup_thread` (`1033-1071`) never prunes `device_first_connected`, `device_total_online_s`, `device_last_seen_ever`, `device_km_*`, `device_disconnected`; written every 5 s, reloaded on boot. Prune by `last_seen_ever` age (+ C1 validation).
|
||
**M4.** `km_24h` resets at 00:00 UTC (`570, 612, 1197` use `utcnow()`); everything else uses `_TZ_CDMX`.
|
||
**M5.** KM reset offset (`web_monitor.py:2596-2599`, `140-142`) corrupts `km_24h` on following days; store `reset_at` and apply only same local day.
|
||
**M6.** Availability/disconnection reports bucket by UTC day (`1090-1093`); trips/CO2/track by local day → tabs disagree, "Hoy" misses 6 h.
|
||
**M7.** SSE parses the whole status file per client every 2 s on the event loop (`2629-2640`); ~450 KB at 500 units × 20 dashboards = 10 parses/s stalling all handlers. One cached background reader; slim payload.
|
||
**M8.** `web_monitor_ctl.sh restart` hangs while any dashboard is open (`/events` never completes; single SIGTERM at `79`; port-in-use at `43-48`). Add `--timeout-graceful-shutdown 5`, poll then `kill -9`.
|
||
**M9.** Web monitor not under systemd, binds 0.0.0.0, cookie without `secure` (`2755-2758`). systemd unit, `--host 127.0.0.1`, `secure=True` behind TLS.
|
||
**M10.** No `logging` in web_monitor; `except … pass`/`return []` at `298-301`, `313-314`, `1106-1107`, `1539-1540`, `338-339`, `2508-2509` → a DB permission/WAL problem shows as "0 % / no data" with HTTP 200.
|
||
**M11.** CGNAT port reuse mis-attributes positions: addr→cell_id kept 300 s (`1026-1029`, `1353-1360`). Expire at ~2× the `$GPTXT` period; refuse GGA if the addr's last `$GPTXT` is older.
|
||
|
||
### LOW
|
||
L1 reset link `?email=` unencoded (`2800`). L2 `/logout` is GET (`2766`). L3 `Content-Disposition` filename from unvalidated ids (`2264, 2304, 2354`). L4 Wialon packet timestamp = receipt time, not fix time (`332`). L5 CWD-relative paths (`133, 149`; `web_monitor.py:57, 78`) vs `__file__`-anchored ones; `PrivateTmp=yes` would hide `/tmp/htracker_status.json`. L6 one missed 5-min sample = "gap" (`1371, 1405`). L7 `telemetry_history.db` ≈ 30 MB/day at 500 units, no retention. L8 per-device INFO/ERROR lines (`1335, 449, 499, 519`) flood the collector at scale.
|
||
|
||
### Verified OK
|
||
All SQL parametrized. Upload paths safe. Server-side f-string HTML escapes. CSRF mitigated by `samesite=lax`. Status JSON write atomic (tmp + rename). Per-cell_id **read** authorization enforced on every read endpoint. Anti-jump filter re-anchors after 3 rejects and forgets anchors on TTL — no permanent lockout from that filter. `INSERT OR IGNORE` + `UNIQUE(cell_id, captured_at)` dedupes.
|
||
|
||
### Must verify in `auth.py` / `gps_logger.py`
|
||
HMAC session token compared with `hmac.compare_digest`; bcrypt/argon2/scrypt (not sha256); constant-time on unknown emails; reset tokens `secrets.token_urlsafe(32)`, single-use, ≤ 1 h, stored hashed; lockout persistence. Index on `gps_positions(device_cell_id, received_at)` — without it fleet CO2/trips views do 60 full scans per request.
|
||
|
||
### Improvements (not bugs)
|
||
1. Cached SSE reader + delta payloads. 2. `/run/htracker/` for the status file, passed explicitly to both processes. 3. `web_monitor` under systemd (`Requires=htracker.service`), 127.0.0.1, `--proxy-headers`, graceful timeout. 4. `/healthz` (status-file age, DB readable, forwarder drop rate). 5. Retention for `telemetry_history.db`, pruning of `htracker_device_state.json`. 6. 429/503 + `Retry-After` on heavy report ranges. 7. Structured logs; ship only `[SUMMARY]` + WARN. 8. UDP replay test harness with a fake Wialon returning `#ASD#0` / closing / stalling.
|
||
|
||
### Related
|
||
Patch already prepared for the 0.5.1 firmware fields and `$PHEV` events: `02-Architecture-Software/htracker-patches/`.
|