Security hardening + code-review fixes
Outcome of a full multi-agent code review. Touches TLS, credential storage, API versioning, networking correctness, concurrency/lifecycle, and the release build. See CHANGELOG.md for the full list and the items to port to the iOS app. Security - Remove untrusted-TLS support: delete ServerTrustPolicy.AllowSelfSignedForHost and its trust-all X509TrustManager; drop the self-signed Settings toggle. - Enforce HTTPS in normalizedBaseUrl (reject http + embedded credentials); add network_security_config (no cleartext, system CAs only) + allowBackup=false. - Never persist passwords at rest: remember-me stores email only; no password pre-fill; clear creds on sign-out / when remember-me is off. - Match CSRF host lookup to OkHttp's host parsing; stop leaking raw exception text. Correctness - Fix API versioning: hardcode v1/v3 per endpoint (docs Sec.4) instead of one broken global apiVersion; remove the version plumbing + Settings field. - Treat "warning" status as success (committed write); fail-fast on a missing pagination cursor; close Response on cancellation; tolerant JSON parse; User-Agent. - Firmware isCompatible fails closed + extra acknowledgment for a non-compatible image. Concurrency / lifecycle - Run network I/O on Dispatchers.IO; stale-result generation guards + rethrow CancellationException in all loads; host AppState in a ViewModel (survive config change); thread-safe cookie jar; reset/upgrade re-entry guard; wire list Retry. Build - Enable R8 minify+shrink with kotlinx.serialization keep rules; drop unused navigation-compose; add the missing .gitignore. NOTE: built/verified by static review only (no Android SDK on the dev machine) — run :app:assembleDebug and a release build before shipping. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
ca3d7d05e0
commit
41098831ae
20 changed files with 505 additions and 191 deletions
103
CHANGELOG.md
Normal file
103
CHANGELOG.md
Normal file
|
|
@ -0,0 +1,103 @@
|
|||
# Changelog — PON Go (Android)
|
||||
|
||||
All notable changes to this app. The **★ Port to iOS** section in each entry calls
|
||||
out changes that affect shared REST behaviour / security / device-safety and most
|
||||
likely have an equivalent in the sibling iOS app (`../mcms_ios`) — the two share the
|
||||
same MCMS field paths and request semantics (see `docs/MCMS_API.md`).
|
||||
|
||||
---
|
||||
|
||||
## [Unreleased] — Security hardening & code-review fixes (2026-05-31)
|
||||
|
||||
Outcome of a full code review. No `versionCode` bump yet — bump before release.
|
||||
Release build now uses R8; **run `./gradlew :app:assembleDebug` and a `release`
|
||||
build to verify** (the dev machine for this change had no Android SDK).
|
||||
|
||||
### Security
|
||||
|
||||
- **Removed untrusted-TLS support entirely.** Deleted `ServerTrustPolicy.AllowSelfSignedForHost`
|
||||
and its all-accepting `X509TrustManager` (it accepted *any* certificate for the
|
||||
configured host — a full MITM hole on a tool that can reset gear and push firmware).
|
||||
Removed the "Allow self-signed certificate" toggle from Settings. Only system-CA
|
||||
trust remains, plus the still-available `ServerTrustPolicy.PinPublicKeySha256` for
|
||||
pinning a known box's key. *(`core/Networking.kt`, `core/ApiConfiguration.kt`, `ui/AuthScreens.kt`)*
|
||||
- **Enforced HTTPS.** `normalizedBaseUrl` now defaults a missing scheme to `https`,
|
||||
rejects any non-`https` URL, and rejects embedded credentials. Added
|
||||
`res/xml/network_security_config.xml` (cleartext forbidden, system CAs only — blocks
|
||||
the user-installed-CA MITM vector) + `usesCleartextTraffic="false"`. *(`core/ApiConfiguration.kt`, `AndroidManifest.xml`)*
|
||||
- **Hardened stored credentials.** Passwords are no longer persisted at rest:
|
||||
"Remember me" stores the **email only**; the password field is never pre-filled and
|
||||
is kept out of the saved-instance `Bundle` (plain `remember`, not `rememberSaveable`).
|
||||
Credentials are cleared on sign-out and when "Remember me" is unchecked; any legacy
|
||||
stored password is purged. `allowBackup="false"`. *(`ui/AppState.kt`, `ui/AuthScreens.kt`, `AndroidManifest.xml`)*
|
||||
- **CSRF cookie lookup** now resolves the host via OkHttp's parser (matching the cookie
|
||||
jar) so the `X-CSRFToken` lookup can't silently miss on host-normalisation edge cases.
|
||||
- Unexpected (non-`ApiError`) throwables no longer leak raw library text to the UI. *(`ui/Types.kt`)*
|
||||
|
||||
### Correctness
|
||||
|
||||
- **Per-endpoint API versioning.** Endpoint paths are now fixed at `v1`/`v3` per
|
||||
`docs/MCMS_API.md` §4 instead of one global `apiVersion` (which was wrong for *every*
|
||||
config: lists/states need `v3`, single-doc/login/logs need `v1` — no single value
|
||||
worked). Removed the global `apiVersion` config field, the `version` override plumbing,
|
||||
and the API-version Settings input. Versions for undocumented endpoints (controllers,
|
||||
`olts/configs`, reset, `ponmgr/version`) are inferred and marked `// inferred`. *(`core/McmsEndpoint.kt`, `core/ApiClient.kt`)*
|
||||
- **`"warning"` status treated as success.** The `{status,data}` unwrap now accepts
|
||||
`warning` as well as `success` (a `warning` means the write committed; only a schema
|
||||
mismatch is reported) — previously a committed firmware/config write was reported as
|
||||
failed. *(`core/ApiClient.kt`)*
|
||||
- **Pagination fails loud, not silent.** `getAll` now throws instead of returning a
|
||||
truncated fleet when a full page yields no usable cursor (e.g. a projection drops
|
||||
`_id`). *(`core/ApiClient.kt`)*
|
||||
- **`Response` no longer leaks on coroutine cancellation** (resume-with-onCancellation
|
||||
closes a dropped response — prevents connection-pool exhaustion on navigate-away). *(`core/ApiClient.kt`)*
|
||||
- **Firmware compatibility now fails closed.** `FirmwareFile.isCompatible` returns
|
||||
compatible only when vendor + equipment id are known and the file positively matches
|
||||
(was: unknown/missing metadata ⇒ "compatible"). Staging a non-compatible image now
|
||||
requires an explicit extra acknowledgment in the dialog. Mis-flashing can brick an ONU. *(`core/Firmware.kt`, `ui/OnuScreens.kt`)*
|
||||
- **JSON parse is tolerant.** A non-JSON 2xx body yields a typed `ApiError.Decoding`
|
||||
instead of crashing. A required non-empty `User-Agent` header is now sent (§3.1). *(`core/ApiClient.kt`)*
|
||||
|
||||
### Concurrency / lifecycle
|
||||
|
||||
- **Network I/O off the main thread** (`Dispatchers.IO`) — fixes ANR/jank on large
|
||||
fleets; the `onAuthExpired` Compose-state write stays on the main thread. *(`core/ApiClient.kt`)*
|
||||
- **Stale-result generation guards** in every screen `load()`/`reload()`: a slow earlier
|
||||
load can no longer overwrite newer data, and `CancellationException` is rethrown (a
|
||||
cancelled load is no longer shown as an error). Fixes the ONU-detail tab-switch race. *(`ui/*Screens.kt`)*
|
||||
- **`AppState` hosted in a `ViewModel`** so rotation / dark-mode no longer rebuilds the
|
||||
connection and bounces the user to login. *(`MainActivity.kt`)*
|
||||
- Thread-safe `SessionCookieJar` (synchronised; OkHttp calls it from background threads). *(`core/Networking.kt`)*
|
||||
- Re-entry guard on `reset()`/`upgrade()`; list-screen "Retry" buttons wired up; the
|
||||
dashboard keeps its last-good data when a background refresh fails. *(`ui/*Screens.kt`)*
|
||||
|
||||
### Build / Android
|
||||
|
||||
- Release build enables R8 `isMinifyEnabled` + `isShrinkResources` with correct
|
||||
kotlinx.serialization keep rules (so config (de)serialization survives shrinking). *(`app/build.gradle.kts`, `app/proguard-rules.pro`)*
|
||||
- Dropped the unused `navigation-compose` dependency. Added the missing `.gitignore`. *(`app/build.gradle.kts`, `.gitignore`)*
|
||||
|
||||
### ★ Port to iOS (`mcms_ios`)
|
||||
|
||||
These touch shared REST/security/safety behaviour and very likely have an iOS analogue —
|
||||
review and port:
|
||||
|
||||
1. **Per-endpoint v1/v3 versioning** — if iOS uses a single global API version it has the
|
||||
same "no value works for everything" defect. Pin versions per endpoint per `docs/MCMS_API.md` §4.
|
||||
2. **`"warning"` = success** in the `{status,data}` unwrap — a committed write must not be
|
||||
reported as a failure.
|
||||
3. **Firmware `isCompatible` fail-closed** (+ extra acknowledgment for a non-compatible
|
||||
image) — this is a device-brick safety fix, not cosmetic.
|
||||
4. **Remove the self-signed "escape hatch."** HANDOFF notes the Android trust-all path
|
||||
"matches the iOS self-signed escape hatch," so iOS almost certainly has the same MITM
|
||||
hole. Remove it; rely on ATS + system trust, or pin the leaf key for a known box.
|
||||
5. **Credential hardening** — store email only (never the password) in the Keychain;
|
||||
clear on sign-out / when remember-me is off; don't pre-fill the password field.
|
||||
6. **Reject `http://` at config time** even though iOS ATS blocks cleartext at the
|
||||
transport layer — fail early with a clear message rather than a confusing transport error.
|
||||
7. **Pagination fail-fast** when a full page has no cursor, rather than silently truncating.
|
||||
8. **Send a non-empty `User-Agent`** (an empty UA is rejected by MCMS, §3.1).
|
||||
|
||||
iOS-specific equivalents worth a glance but not 1:1 ports: cancel-safe request teardown
|
||||
(don't leak the response on task cancellation), and not surfacing stale results when a
|
||||
newer load is in flight.
|
||||
Loading…
Add table
Add a link
Reference in a new issue