Code quality & test-coverage audit: web-logbook XSS/CSRF, error handling, untested protocol code

[Reid-n0rc]

Code quality & test-coverage audit

Audit of the current codebase (~95k LOC production across 383 files; ~29k LOC tests across 255 files, 2,195 @Test methods). The codebase is bimodal — legacy Java (com.k1af.ft8af.*, from the FT8CN lineage) and newer Kotlin/Compose (radio.ks3ckc.ft8af.*). Most defects and gaps cluster in the legacy Java half.

Line references are against the branch audited (docs/security-policy) and may drift.

---

Test suite: overall healthy

• 2,195 test methods, zero @Ignore/@Disabled, zero assertion-free test files — no rot, no silently-disabled tests.
• Core classes are exercised indirectly even without a dedicated FooTest: GeneralVariables (35 test files), MainViewModel (13), DatabaseOpr (11), FlexRadio (6).
• The "extract logic out of @Composable, test the plain function" convention is being followed — Compose screens dominate the zero-reference list because their logic lives in tested helpers.

No action needed on the suite itself; the gaps below are specific untested modules.

---

Findings

1. Reflected XSS in the web-logbook HTTP server — highest severity

LogHttpServer (NanoHTTPD, port 7050) echoes untrusted query params into HTML with no or incomplete escaping:

• showQslCallsigns — LogHttpServer.java:384-388 puts the raw callsign param into value="%s" with no escaping. ?callsign="><script>… breaks out of the attribute.
• getCallsignQTH — LogHttpServer.java:433-434 — same pattern for callsign and grid.
• Escaping elsewhere is inconsistent and incomplete: several spots do .replace("<", "&lt;") (e.g. :1151, :1155) which misses ", >, and &, so attribute-context breakout via " still works.

The server does super(port) (LogHttpServer.java:50) → NanoHTTPD binds all interfaces with no authentication, so any host on the LAN can reach it.

Note: SQL is correctly parameterized (bound ?, e.g. :396, :428), so this is HTML/XSS injection, not SQLi.

Recommended fix: add a single central htmlEscape() helper (escaping & < > " ') and apply it to every user-supplied value before it enters markup. Remove the ad-hoc partial .replace("<", …) calls in favor of it. Add unit tests covering attribute-context and element-context payloads.

2. State-changing deletes over GET (CSRF)

Destructive actions are triggered by GET requests rendered as plain <a href> links:

• /delfollow/<call>, /delQslCallsign/<id>, and month-delete are dispatched on GET at LogHttpServer.java:167,172,193 and rendered as links at :499, :572, :1873.

Any page a user opens while the server is running can silently delete log entries via an <img>/link (classic CSRF), and browsers/prefetchers can trigger them unintentionally.

Recommended fix: move all state-changing operations to POST, and require a per-session CSRF token on those forms. At minimum, gate deletes behind POST.

3. printStackTrace() as the error strategy — 74 sites

Concentrated in flex/ (4), wave/ (3), database/ (3), plus icom/, x6100/, connector/. On Android these go to stderr and are invisible in the field. The app already has a structured fileLog() writing to debug.log.

Recommended fix: route these through fileLog() (or a shared logging helper) so CAT/audio/DB failures are diagnosable from the on-device log instead of being swallowed to stderr.

4. Empty catch blocks — 34 sites (mostly benign, a few not)

Most are legitimate cleanup swallows (request.close(), unregisterReceiver, cancel() in UsbAudioDevice) and are acceptable. Worth revisiting:

• MainViewModel.java:156 — silent catch (Exception ignored).
• flex/FlexMeterInfos.java:82,86 — NumberFormatException swallowed; discarding the parse failure can hide malformed rig telemetry.

Recommended fix: for the non-cleanup cases, log the swallowed exception via fileLog() (even at debug level) so malformed input is observable.

5. God-classes / duplicated legacy fragment

• java/…/ui/ConfigFragment.java (1,944 LOC), MainViewModel.java (2,214), DatabaseOpr.java (3,105) are monoliths.

These are the files most likely to accrue the next bug and the hardest to test.

Recommended fix (long-term): incrementally extract cohesive responsibilities (e.g. DB access grouped by table/feature, config sections) into smaller units with their own tests. Not urgent; do it opportunistically when touching these files.

---

Test-coverage gaps (highest-value, currently zero coverage)

Of 205 classes with no test references, most are pure UI (acceptable per the extract-and-test convention). These carry real, testable, non-UI logic and have none:

File	~LOC	Why it matters
serialport/ProlificSerialDriver.java	584	USB serial protocol byte-packing — pure logic
serialport/CdcAcmSerialDriver.java	359	same
serialport/Ch34xSerialDriver.java	393	same
serialport/FtdiSerialDriver.java	434	same
serialport/Cp21xxSerialDriver.java	~360	same
icom/IComPacketTypes.java	1,045	CI-V packet construction (byte layout)
pota/PotaAuth.kt, pota/PotaOAuthLogin.kt	388 / 361	OAuth token flow — security-relevant
ui/ConfigFragment.java	1,944	Legacy settings fragment

The serial drivers and IComPacketTypes are the best targets: pure byte/protocol logic (no Android UI), cheap to test, and a single wrong byte silently breaks CAT control.

Recommended fix: add unit tests for the five USB serial drivers and IComPacketTypes (encode/decode round-trips and control-transfer byte layouts); add tests for the POTA OAuth flow.

---

Suggested priority

1. Central HTML-escape + move deletes to POST in LogHttpServer (findings 1 & 2) — the only network-facing attack surface.
2. Unit tests for the 5 USB serial drivers + IComPacketTypes (pure logic, high value, cheap).
3. Route the 74 printStackTrace() calls (and the non-cleanup empty catches) through fileLog().
4. Long-term: chip away at the god-classes as they're touched.

Each of these is a natural standalone PR against dev.