Remove self-signed TLS option (system trust only); add session changelog
Parity with the Android client: drop the "Allow self-signed certificate" escape hatch. The app now performs system TLS trust only — a self-signed appliance must have its CA installed on the device (MDM/profile). A code-only public-key pinning policy remains (no UI). - ServerTrustEvaluator: remove ServerTrustPolicy.allowSelfSignedForHost and its challenge handler; keep .system (default) and .pinPublicKeySHA256. - SettingsView: remove the self-signed toggle/state/prefill; makeConfig uses the default .system policy. - MCMSConnection / APIConfiguration: update usage-sketch + doc comments. - README / GUI-NOTES / MCMS_API.md: document system-trust-only; self-signed boxes need their CA installed on the device. - Add CHANGELOG.md for this session, flagged for the Android port. Note: configs previously persisted with the self-signed policy fail to decode and reset to defaults (one-time re-entry of the server URL). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
7d8f1c7b2d
commit
6bb65d2aa3
8 changed files with 142 additions and 70 deletions
100
CHANGELOG.md
Normal file
100
CHANGELOG.md
Normal file
|
|
@ -0,0 +1,100 @@
|
|||
# Changelog
|
||||
|
||||
## 2026-05-31 — Code-review pass + TLS parity
|
||||
|
||||
A multi-agent code review of the iOS app, the confirmed fixes applied, and the
|
||||
self-signed TLS option removed for parity with the Android client. This entry is
|
||||
written for cross-pollination: the two apps share REST behaviour and field paths
|
||||
(see `MCMS_API.md`), so items below are grouped by how likely they apply to Android.
|
||||
|
||||
### 🔁 Check on Android — shared REST logic / behaviour
|
||||
|
||||
- **Pagination cursor must be the real `_id`, not a synthesized id.**
|
||||
`getAll` followed the `next` cursor using each model's `id`, which fell back to a
|
||||
random value when `_id` was absent → the cursor became meaningless and could
|
||||
re-serve or skip pages (silently wrong fleet list). Fixed to drive the cursor off
|
||||
the actual `_id` and stop paging when it's absent.
|
||||
→ *Android:* if the paginator derives `next` from a model `id` that has any
|
||||
non-`_id` fallback, it has the same bug. Use the document `_id` (or stop).
|
||||
|
||||
- **Firmware image with empty compatibility metadata was shown as "compatible for
|
||||
every ONU."** `isCompatible` returned true when the image declared no
|
||||
`Compatible Manufacturer` / `Compatible Model`. For a destructive flash that
|
||||
invites pushing the wrong image. Now requires a positive model match; un-typed
|
||||
images appear only under the "Show all" escape hatch.
|
||||
→ *Android:* mirror the same rule in the firmware picker's compatibility filter.
|
||||
|
||||
- **Per-ONU alarm filter may return the WHOLE fleet's alarms.** The ONU-detail
|
||||
Alarms tab filters the fleet-wide alarm-history endpoint with
|
||||
`query = {"_id": onuId}`, but that key is unverified — if it's wrong the server
|
||||
returns 200 with every ONU's alarms (no error). Left as-is (couldn't verify the
|
||||
key off a live box) with a loud in-code warning.
|
||||
→ *Android:* same endpoint + key risk. Verify the real filter key against
|
||||
`openapi.json` / a live response, or use the dedicated per-id alarm endpoint.
|
||||
|
||||
- **Optical RX thresholds were inconsistent between screens.** Dashboard flagged
|
||||
abnormal at `< -28 || > -10` dBm; the detail row tinted green at `>= -28` but its
|
||||
footer legend said "green ≥ -30". Unified to one source of truth
|
||||
(`OpticalThreshold`: green floor -28, red floor -30, high -10) and corrected the
|
||||
legend.
|
||||
→ *Android:* if it shows optical levels with color thresholds, align dashboard
|
||||
and detail to one constant and check the legend text matches the actual rule.
|
||||
|
||||
- **Dashboard best-effort sections went stale silently on a transient failure.**
|
||||
During the 30s auto-refresh, if the alarm-histories fetch failed, the
|
||||
alarm-by-severity counts kept the previous cycle's values while the dashboard
|
||||
still showed "loaded" (and controller count masked a failed fetch as `0`). Now
|
||||
keeps last-good explicitly and doesn't report a failed count as zero.
|
||||
→ *Android:* if the dashboard auto-refreshes with best-effort sections, decide
|
||||
explicitly between keep-last-good and show-empty rather than whatever the control
|
||||
flow produces.
|
||||
|
||||
- **JSON number → Int coercion could crash on a malformed/huge value.** Swift's
|
||||
`Int(double)` traps on NaN/±∞/out-of-range; a JSON literal like `1e400` decodes
|
||||
to `∞` and crashed an accessor. Guarded to return nil.
|
||||
→ *Android:* Kotlin's `Double.toInt()` won't trap the same way, but confirm
|
||||
out-of-range / NaN counters (FW Bank Ptr, DHCP lease seconds, util %, temps)
|
||||
don't yield garbage Ints in the equivalent accessors.
|
||||
|
||||
### 🔒 Security parity (the point of this round)
|
||||
|
||||
- **Removed the "Allow self-signed certificate" option entirely** — the
|
||||
`ServerTrustPolicy.allowSelfSignedForHost` case, the Settings toggle, and its
|
||||
wiring. The app now uses **system TLS trust only** (a code-only public-key pinning
|
||||
option remains, unused by the UI). Consequence: a self-signed appliance (e.g. the
|
||||
lab box `mcms.lab.svorka.net`) is unreachable until its CA is installed on the
|
||||
device (MDM/configuration profile) or the box gets a CA-trusted cert. This matches
|
||||
the Android client. Old persisted configs that stored the self-signed policy will
|
||||
fail to decode and reset to defaults (one-time; just re-enter the server URL).
|
||||
|
||||
- **Public-key pin was hashed over the raw key, not the SubjectPublicKeyInfo** —
|
||||
so a standard `openssl … pkey -pubin -outform der | dgst -sha256` pin never
|
||||
matched (pinning failed closed). Now prepends the algorithm-specific ASN.1 SPKI
|
||||
header and hashes the real SPKI.
|
||||
→ *Android:* if it pins, OkHttp `CertificatePinner` already uses SPKI
|
||||
`sha256/…` — no change needed; just don't reintroduce a raw-key hash.
|
||||
|
||||
### ❓ Analogous patterns worth a look (not certain to apply)
|
||||
|
||||
- **Overlapping load / stale-commit races.** iOS view models let a pull-to-refresh
|
||||
overlap a tab-switch / auto-refresh load and commit stale or wrong-tab data; fixed
|
||||
with a per-load generation guard + cancellation check before committing.
|
||||
→ *Android:* if `load()` / refresh use coroutines without single-flight or
|
||||
job-cancel, the same stale-commit / wrong-tab race can occur.
|
||||
|
||||
- **List-row identity fallback.** iOS list rows fell back to a random id when `_id`
|
||||
was missing, breaking diffing/scroll/animation.
|
||||
→ *Android:* if DiffUtil keys fall back similarly, use a stable/content-derived
|
||||
key.
|
||||
|
||||
- **Networking client lifecycle.** iOS leaked a `URLSession` per connection/probe
|
||||
(created with a delegate, never invalidated).
|
||||
→ *Android:* ensure OkHttp clients are shared/closed rather than created and
|
||||
dropped per connection test.
|
||||
|
||||
### 📱 iOS-only (listed for completeness; not relevant to Android)
|
||||
|
||||
- Swift closure type-inference build fix (annotate list-VM merge locals).
|
||||
- `MongoDocument.id` SwiftUI identity specifics; SwiftUI `ForEach` keying.
|
||||
- `Format` byte/bit-rate rounding rollover at the unit boundary.
|
||||
- Dead-code, stale-docstring, and helper-dedup cleanups.
|
||||
Loading…
Add table
Add a link
Reference in a new issue