kvitt/docs/validation/security-review.md

> Taken at `18c527f` (main, 2026-10-18, SPEC v0.17); committed verbatim with SPEC v0.18. Status of each finding: SPEC §11.4.

# Security review of Kvitt at `18c527f` (2026-10-18)

Read-only review of the main checkout against SPEC v0.17 §8, §10, §14.3, §14.4, §14.6 and general practice. Scope: `server/crates/**`, `android/**`, `infra/**`, `contracts/openapi.yaml`, `.gitea/workflows/**`. No build, no phone, no network, no repository change. Threat model as in SPEC §8.1: one public port (8443) on a home connection, two trusted users, attacker is an internet stranger or a lost phone. Aggregates only; no secret values were read.

## 1. Verdict

**No critical finding. One High: the in-memory rate limiter can be made to grow without bound and to bypass the per-IP limit by an unauthenticated stranger (DoS of the Pi). Fix it before port 8443 is opened.** Everything else is Medium or below, mostly hardening and owner decisions. The design (hashed 256-bit device tokens, 60-bit single-use invite codes with a 10-per-hour failure lockout, Keystore-sealed token on the phone, no backup/extraction, one exported activity, parameterised SQL everywhere, path-safe APK and picture files, read-only non-root server container behind Caddy) matches the spec and is sound.

Counts: Critical 0, High 1, Medium 3, Low 10, Info 8.

## 2. Findings

### High

**H-1. Rate limiter: a random bearer token becomes "accepted" on any non-401 response, bypasses the IP limit and is never pruned.**
Location: `server/crates/kvitt-server/src/api/rate_limit.rs:273-287` (admit), `:305-314` (record), `:143-144` (prune), with `src/router.rs:102` (`require_protocol` runs before the token check) and `src/auth.rs:65-76`.
Issue: for an authenticated route the limiter keys by the hash of any token-shaped bearer (`Key::Token`). `admit` only *checks* the IP window for a not-yet-accepted token, it does not count against it; `record` counts the IP only when the response is 401, and otherwise marks the token `accepted = true`. A request with a random `kvt_` token and **no `X-Kvitt-Protocol` header** gets 426 from `require_protocol` (before the database is touched), so the random token is marked accepted; `prune` keeps accepted tokens forever. The same happens with 405 (wrong method on a known path).
Exploit (internet stranger, no credentials): send `GET /me` with a fresh random token and no protocol header in a loop. No IP counter is ever incremented, so there is no rate limit at all on this path, and every request adds a permanent `HashMap` entry (about 150 bytes) to a container limited to 160 MiB: a few hundred thousand requests, well within an hour on a home uplink, exhaust the server's memory (OOM kill, restart loop). Secondary effect: each such "accepted" token then gets its own 600/min bucket of database lookups on later requests with the protocol header, so the per-IP 60/min limit of §8.6 no longer bounds the database load from one IP.
Smallest fix (server): in `record`, mark a token accepted only when `status.is_success()` (2xx), not on every non-401/429; in `admit`, for a not-yet-accepted token *hit* the IP window (count it) instead of only checking it; in `prune`, also drop accepted tokens whose window has expired (re-marking costs one successful request). Add a test: random token + no protocol header × N leaves `tokens.len()` bounded and returns 429 after 60.

### Medium

**M-1. Backups on Google Drive are unencrypted and the rclone token has full-Drive scope.**
Location: `infra/backup/backup.sh:55-72`; SPEC §8.7 last bullet vs §8.1 ("a compromised backup location" is in scope); SPEC §10.9 (`scope = drive`).
Issue: the daily `kvitt-*.sqlite.gz` (all expenses, notes, names) and pictures go to Drive in clear; the mounted `rclone.conf` carries an OAuth refresh token with the `drive` scope, i.e. read/write to the owner's entire Drive, not just the backup folder. A compromise of the backup container or of `/opt/kvitt/secrets/rclone.conf` exposes the whole Drive; a compromise of the Google account exposes every backup. The spec accepts the first half (§8.7) while listing the second half as a threat (§8.1).
Smallest fix (infra + owner decision): wrap the remote in an rclone `crypt` remote (`[gdrive-crypt] type = crypt, remote = gdrive:<folder>`; the two passwords live in the already-secret `rclone.conf`, and `RESTORE.md` gains one `rclone copy` through the crypt remote); re-authorise with scope `drive.file` so the token reaches only files rclone created. Needs a decision entry and a §8.7/§10.9 edit.

**M-2. The cert-sync SSH key may be a root login on the Home Assistant Pi.**
Location: `infra/cert-sync/sync.sh:36-42`; SPEC §10.5.1 ("variant B: added as a plain key"), `.env.example` ("the Home Assistant SSH add-ons log in as root").
Issue: cert-sync holds `/secrets/ha_ssh_key`. With variant B there is no `restrict,command=` on the HA side, so whoever holds the key (a compromised cert-sync container, or the key copied around on the Mac and the Pi) gets an unrestricted root shell on the Home Assistant Pi, which also owns ports 80/443 and the house's automation. The script's own fixed `tar` command does not restrict the server side.
Smallest fix (infra + owner): make variant A the only accepted variant (`restrict,command="tar -C <dir> -cf - fullchain.pem privkey.pem"`, plus `from="<Kvitt Pi LAN address>"`), and if the add-on cannot do that, use a dedicated non-root user that can read `/ssl` only. Record the variant in `infra/README.md` as §10.5.1 already asks.

> **Note 2026-10-19 (D-120, SPEC v0.21):** moot. Caddy and cert-sync are removed; TLS terminates in the owner's Nginx Proxy Manager on the Home Assistant Pi, and no SSH key for the Home Assistant Pi exists in the stack any more (`install.sh` deletes `ha_ssh_key` and `ha_known_hosts` left on the Kvitt Pi). Closed.

**M-3. Picture decoding can exceed the server's memory limit (8000 × 8000 px vs 160 MiB).**
Location: `server/crates/kvitt-server/src/api/pictures.rs:39` (`MAX_DIMENSION`), `:66-92` (`process`), `infra/pi/compose.yaml` (`mem_limit: 160m`); limit from SPEC AC-4.4.
Issue: the dimension check happens before decoding (good), but an 8000 × 8000 image is allowed: decoding to RGBA is 256 MB, plus `to_rgba8`/`to_rgb8` copies, on a container limited to 160 MiB. A 5 MiB PNG or WebP easily encodes that size (a flat colour compresses to a few KB). Only an authenticated device can call `PUT /me/picture`, so in this threat model the attacker is a lost phone with a live token (until revoked) or a bug in the app, not a stranger; the effect is a crash and restart of the server (and of the sync for both phones) on each upload.
Smallest fix (server, plus a spec number): decode through `image::ImageDecoder` limits (`Limits { max_image_width/height, max_alloc ~ 64 MiB }`) or lower the limit to a pixel count (for example 16 M pixels, 4000 × 4000) and refuse larger with `image_invalid`. The number is a spec value: `TODO(spec)` / decision entry.

### Low

**L-1. Caddy's access log records invite codes and the admin's display name.**
Location: `infra/pi/Caddyfile:11-14`; SPEC §8.7 (the Rust server logs the route template, by design, `src/router.rs:134-142`).
Issue: Caddy logs `request.uri` in full: `/i/<CODE>?from=<name>` and the `?from=` value, together with the client IP and User-Agent, into the json-file logs on the Pi (10 MB × 3). Codes live 72 h and are single-use, so the risk is low (an attacker with Pi log access is out of scope), but it is personal data and a secret in a log that §8.7 says must not hold either.
Fix (infra): add `log_skip` (Caddy ≥ 2.8) for the `/i/*` matcher, or `log { format filter { request>uri delete } ... }` — one line in the Caddyfile; or drop the access log entirely (the server already logs every request).

> **Note 2026-10-19 (D-120, SPEC v0.21):** Caddy is gone; the redaction moved to NPM's Custom Nginx Configuration (SPEC §10.5.2): `location ^~ /i/` has `access_log off` and `error_log /dev/null crit`, so neither the code nor the `?from=` name reaches NPM's logs, a 502 included (rehearsed on the Mac). Other paths are logged with their query strings, which hold no personal content; NPM logs no request headers.

**L-2. The Caddy container is not hardened like the other three.**
Location: `infra/pi/compose.yaml` (`caddy` service: no `user`, no `cap_drop`, no `read_only`); SPEC §8.9 hardens only the server.
Issue: Caddy runs as root with every default capability and a writable root filesystem, and its admin API is bound to `0.0.0.0:2019` for every container on the `kvitt` network (only cert-sync needs it). This is the process that terminates TLS from the internet.
Fix (infra): `cap_drop: [ALL]`, `cap_add: [NET_BIND_SERVICE]` (or none: 8443 is unprivileged), `read_only: true` with `tmpfs: [/tmp]` and `caddy_data`/a `caddy_config` volume writable; keep `no-new-privileges`. Admin on `0.0.0.0` is required for cert-sync; acceptable on the internal network.

> **Note 2026-10-19 (D-120, SPEC v0.21):** moot. There is no Caddy container; the TLS-terminating process is the owner's existing NPM install on the Home Assistant Pi, outside this repository. On the Kvitt Pi only `server` publishes a port (8080 on the LAN), with the hardening of SPEC §8.9.

**L-3. APK download is unauthenticated and 11 MB per request at 60 requests per minute per IP.**
Location: `server/crates/kvitt-server/src/api/apk.rs:76-101`, `src/api/rate_limit.rs:56-63`; SPEC §8.6, §10.6.
Issue: the only large unauthenticated response. One IP may pull about 660 MB/min, several IPs more, saturating the home uplink (and the Pi's SD card) for as long as they like. The file is public by design (the landing page links it).
Fix (server; the number is a spec value): a separate, lower window for `/apk/files/{file_name}` (for example 5 per minute per IP) — `TODO(spec)`.

**L-4. `fcm_token` has no length limit.**
Location: `server/crates/kvitt-server/src/api/devices.rs:95-117`; SPEC §9.6 has no limit for it.
Issue: an authenticated device can store a string of up to 1 MiB, which is then sent to FCM in every push body for that device (and kept in the database and backups). Trusted users only; a Firebase Installation ID is 22 characters.
Fix (server, spec number): refuse more than, say, 256 characters with `validation_failed` (`TODO(spec)`).

**L-5. The development stack mounts every secret into the server container.**
Location: `infra/dev/compose.yaml` (`${KVITT_SECRETS_DIR:-../secrets}:/secrets:ro`); SPEC §10 (accepted as the dev layout).
Issue: the dev server container sees the HA SSH key, `rclone.conf`, the Gitea push key and `google-services.json`, although it needs only `fcm-service-account.json`. The Pi stack mounts the single file (correct).
Fix (infra): mount `${KVITT_SECRETS_DIR}/fcm-service-account.json:/secrets/fcm-service-account.json:ro` as on the Pi; a missing file is already handled (push disabled).

**L-6. Any link can prefill any server URL on the pairing screen.**
Location: `android/app/src/main/kotlin/se/hagstedt/kvitt/link/AppLinks.kt:53-56`, `android/feature/onboarding/.../InviteLink.kt:36-52` (`fromAppLink`, `fromLandingLink`), manifest intent filters `android/app/src/main/AndroidManifest.xml:26-47`; SPEC AC-2.6, D-084.
Issue: `kvitt://invite?server=https://attacker&code=…` from any app or web page, or `https://<anyhost>/i/<CODE>`, opens S-1 with the attacker's server in the field. An unpaired phone (first install, or after a revoke) could be talked into pairing with a rogue server, which then receives everything the phone pushes. Mitigations in place: a paired phone ignores the link (D-084, the session gate only shows S-1 when unpaired); the URL is visible and editable; release builds require `https://`.
Fix (android, small): on S-1 show a warning line when the prefilled server is not the default `https://icehack.asuscomm.com:8443` ("Unusual server"); no server change.

**L-7. Notification text (description, amount) shows on the lock screen.**
Location: `android/app/src/main/kotlin/se/hagstedt/kvitt/push/Notifications.kt:82-84` (no `setVisibility`/`setPublicVersion`).
Issue: with Android's default "show sensitive content" setting a bystander reads expense descriptions and amounts from a locked phone. Spec §13.3 defines the copy; it does not say what the lock screen shows.
Fix (android): `setVisibility(VISIBILITY_PRIVATE)` with `setPublicVersion(...)` carrying the generic text already defined for the sync-failed case.

**L-8. The Gitea workflow expands the registry token inside a shell line.**
Location: `.gitea/workflows/build-images.yaml` ("Log in to Gitea Container Registry": `echo "${{ secrets.kvitt_cicd }}" | docker login …`).
Issue: `${{ }}` is substituted into the script text before the shell runs it: a token containing a quote or `$` breaks the line (and could inject), and the value appears in the step's process arguments on the runner. The token also needs `write:repository` (for the tag push) on top of `write:package`.
Fix (infra): pass it as `env: { REGISTRY_TOKEN: ${{ secrets.kvitt_cicd }} }` and use `printf '%s' "$REGISTRY_TOKEN" | docker login …`; keep the scopes minimal (the tag push can use the checkout token if Gitea provides one).

> **Note 2026-10-19 (D-120, SPEC v0.21):** the fix of `e70fe0d` is reversed (`4f69ae0`). The env-based form came through empty on act_runner 0.6.1 with a user-level secret ("password is empty" on the first run), so the login step interpolates the secret in the `run` line again, as `printf '%s' "${{ secrets.kvitt_cicd }}" | docker login … --password-stdin` (the owner's proven form); act_runner masks the value in the log. The quoting risk remains for a token containing `"` or `$`; Gitea tokens are hex. The scope stays `write:package` only.

**L-9. Images are pulled by tag over plain HTTP from the LAN registry.**
Location: `infra/pi/compose.yaml` (`${KVITT_REGISTRY}/kvitt-server:${KVITT_TAG}`), `infra/pi/install.sh` (`insecure-registries`), `infra/pi/deploy.sh`; SPEC §10.3, D-001/D-104 (LAN is trusted).
Issue: anyone on the home Wi-Fi who can answer for `gitea.lan` (or the Gitea Pi itself, if compromised) can hand the Kvitt Pi a tampered server image that sees every token hash and all data. Accepted by the spec's trust in the LAN; noted because the server is the one component that holds everything.
Fix (infra, optional): have `deploy.sh` pin the digest the workflow printed (`image@sha256:…` in `.env`), or enable TLS on Gitea later.

**L-10. A member can tell whether a device id belongs to the other user.**
Location: `server/crates/kvitt-server/src/api/devices.rs:57-63` (404 for unknown, 403 for another user's device).
Issue: trivial information leak between the two trusted users; listed for completeness.
Fix (server): return 404 in both cases, or leave as is.

### Info

**I-1.** `X-Kvitt-Server-Version` on every response and `GET /health` (`server_version`, `schema_version`, `db_ok`) are visible to unauthenticated scanners (`src/router.rs:125-131`, `src/api/health.rs`). Spec-mandated; the version is not a secret. Accept.

**I-2.** Room database and Coil's picture cache are unencrypted at rest (`android/data/.../di/DataModule.kt`, `repo/Pictures.kt`); the token key is Keystore AES-GCM without `setUnlockedDeviceRequired`/StrongBox (`data/auth/TokenCipher.kt:50-58`). This relies on Android file-based encryption and the lock screen, as §8.2 intends; the lost-phone case is covered by revocation (AC-3.3). Accept.

**I-3.** The landing page sends no `Content-Security-Policy`/`X-Content-Type-Options` (`src/api/landing.rs:196-210`). Output is escaped and the page is static with inline CSS only; `Cache-Control: no-store` and `Referrer-Policy: no-referrer` are set. Optional one-liner: `Content-Security-Policy: default-src 'none'; style-src 'unsafe-inline'`.

**I-4.** No vulnerability scan of Rust dependencies is part of the server gate (no `cargo audit`/`cargo deny` in `tools/` or the hooks); `tools/dependency-report/latest.py` covers the Android catalog only. This review did not run one (offline). Suggest `cargo audit` as a periodic owner step or in the gate.

**I-5.** Client-IP correctness to verify after deploy: the rate limiter trusts the last `X-Forwarded-For` entry from Caddy (`rate_limit.rs:203-227`). If Docker's userland proxy ever fronts port 8443 (IPv6 without `ip6tables`, or `userland-proxy` quirks), Caddy sees the bridge gateway for every client and all internet clients share one 60/min bucket and one 10-failed-accepts lockout. Check once in Caddy's log that `request.remote_ip` is a public address.

> **Note 2026-10-19 (D-120, SPEC v0.21):** the trusted proxy is now NPM (`KVITT_TRUSTED_PROXY` = `HA_HOST`, its LAN IPv4 address) and the server publishes 8080 on the LAN; the one-off check is a `conntrack -L -p tcp --orig-port-dst 8080` on the Kvitt Pi showing `HA_HOST` as the source and no `172.*` address in the reply half (SPEC §10.10), instead of Caddy's `remote_ip`.

**I-6.** `bootstrap` and `pair-device` print the invite code to stdout (`src/cli/bootstrap.rs:41-62`). Through `docker compose run --rm` the line also sits in that one-off container's json log until the container is removed. Expected; the owner reads it and it expires in 72 h.

**I-7.** The Ktor/OkHttp client keeps default redirect handling (`data/api/KvittApi.kt:246-248`). Neither the server nor Caddy redirects, and TLS excludes a third party, so the bearer cannot be bounced to another host. Fine.

**I-8.** `infra/secrets/` on the Mac: directory mode 0700; private key files and the service-account, rclone and `google-services.json` files mode 0600; two `.pub` files 0644. One file (`gitea_push_ed25519`) is not among the secrets SPEC §10 lists; presence only, not read.

## 3. Checked and found fine

* **Tokens (§8.2):** 32 CSPRNG bytes via `getrandom`, `kvt_` + base64url, stored only as SHA-256 hex (`api/invites.rs:79-86`, `auth.rs:46-53`); lookup by `UNIQUE token_hash` (`migrations/0001_init.sql:29`); revoked → `device_revoked` before any work; token never logged (log line carries route template, status, latency, device id only, `router.rs:134-162`); the hourly touch is in a `BEGIN IMMEDIATE` transaction.
* **Invite codes (§8.3, AC-2.3, AC-2.12):** 60 random bits from the CSPRNG, Crockford alphabet, normalisation and hashing as specified; single-use/expiry/cancel checked inside the write transaction; every failure is the same `invite_invalid`; 10 failed accepts per IP per hour then 429 (`rate_limit.rs:46-49, 251-265`); `admin_setup` refused once a user exists; `household_full` enforced.
* **Authorization (§8.4):** `AdminDevice` on create/list/cancel invites; devices listing filtered for members; revoke rules and `cannot_revoke_current_device`; `/me` edits only the caller; pushed expenses/settlements must name household users (`sync/check.rs:92-110, 185, 204-222`).
* **SQL:** every query uses `sqlx` bind parameters; no string-built SQL found in `server/crates`; path ids must be canonical hyphenated UUIDs (`api/mod.rs:95-113`).
* **Files and paths:** APK names must match `^kvitt-[0-9]+\.apk$` and be regular files (`apk.rs:22-26, 80-92`); picture paths are built from a parsed UUID and an integer version; writes are atomic (temp + rename); `latest.json` is read on each request, never cached stale.
* **Body limits:** 1 MiB default and 5 MiB for `PUT /me/picture` in the router; Caddy `max_size 6MB`; sync batch 1 to 100 mutations, pull page ≤ 1000; notes/description/name limits applied after trimming; control characters refused in notes.
* **Error leakage:** `internal` returns only a request id to the client and logs the detail (`error.rs:225-234`); 404 for unknown paths; serde messages reach the client only for the caller's own malformed JSON.
* **Panics/unsafe:** `#![cfg_attr(not(test), deny(clippy::unwrap_used, clippy::expect_used))]` in `lib.rs` and `main.rs`; `unsafe_code = "forbid"` in the workspace; mutex poisoning handled (`rate_limit.rs:164-169`).
* **Push (§14.6):** data-only payloads with ids only; service-account `Debug` never prints the key (`push/fcm.rs:64-71`); access token cached in memory only; `UNREGISTERED` clears the token only if unchanged; log lines carry device id and push type, never the FCM token.
* **Server container (§8.9):** uid 10001, `read_only`, `cap_drop: [ALL]`, `no-new-privileges`, no published port, distroless runtime pinned by digest, single secret file mounted read-only; only Caddy publishes 8443; TLS from files, `auto_https off`, admin API not published; Caddy ≥ 2.11 defaults give TLS 1.2+ and the `Authorization` header is redacted (verified by the owner 2026-10-18 per §10.5.2).
* **cert-sync:** `StrictHostKeyChecking=yes` with a dedicated known_hosts, `BatchMode`, `IdentitiesOnly`, fixed remote command, PEM/key-match/expiry checks before install, atomic rename, uid 1000, `cap_drop: [ALL]`.
* **Backups:** online `.backup` + `integrity_check`; copy-only with `--immutable`, confirmed by `lsf` before local deletion; `rclone.conf` used through a 0600 `mktemp` copy that is removed on exit; log lines carry names and sizes only.
* **Secrets in git:** `.gitignore` covers `local.properties`, `*.jks`, `infra/secrets/`, `*.pem`, `*service-account*.json`, `google-services.json`, `*.sqlite*`, `infra/pi/.env`, `infra/pi/out/`, `infra/pi/secrets/`; the pre-commit hook refuses those patterns; `git ls-files` and the add-history of all branches contain no secret-looking file; `.env.example` files hold placeholders only (the Drive folder id is not a secret).
* **Android (§8.5, §8.8):** release network config: cleartext off, system anchors; pinned-root variant limited to one domain and only with `kvitt.tls.pinnedRoot=true`; debug cleartext confined by `normaliseServerUrl` to `https://`, `localhost`, `10.0.2.2` and the three private ranges (`SessionRepositoryImpl.kt:102-124`); `allowBackup="false"` and extraction rules excluding every domain for both cloud backup and device transfer; exported components: `MainActivity` only; `KvittMessagingService` and `InstallResultReceiver` not exported; WorkManager default initializer removed in favour of the Hilt factory.
* **Android token storage (§8.2):** Keystore AES-256-GCM, alias `kvitt_token_key`, 128-bit tag, Keystore-generated IV, ciphertext+IV in `auth.preferences_pb`; `Credentials.toString` redacts the token; on `device_revoked` the preferences are cleared, the key deleted, and database, prefs and picture caches wiped (`SessionManager.kt`, `DataModule.kt:166-170`); on `unauthorized` only the token is dropped.
* **Android network:** bearer only on authenticated calls and, for Coil, only on URLs under the paired base URL (`PictureAuth.kt`); the token is never put in a URL; the accept call sends no bearer; log statements in `app`, `data` and `adapter` carry status codes and exception class names only.
* **In-app update (FR-16):** release-only; file name validated against the same regex as the server; download to app-private cache; size bounded during download and SHA-256 + size verified before `PackageInstaller`; file deleted on any failure; Android enforces signing-key continuity on the update, so a tampered APK from a compromised server could not replace the installed app; `canRequestPackageInstalls` explained once; R8 enabled; `mapping.txt` never served.
* **Push handling on the phone:** untrusted FCM data mapped to a typed event with defaults (`PushMapping.kt`), unknown `type`/`v` → sync only; ids are passed to WorkManager as strings and looked up in Room, never rendered raw.
* **CSV import:** runs only as an operator CLI against a file path given on the command line, inside one transaction, with header and row validation and a reconciliation gate; not reachable over HTTP.
* **OpenAPI:** top-level `security: [device]`; exactly the six public operations (E-1 to E-6) declare `security: []`; the server router applies `AuthDevice`/`AdminDevice` on every other route.

## 4. Owner questions

1. **Backups (M-1):** encrypt the Drive backups with an rclone `crypt` remote (passwords in `rclone.conf`, restore documented), and re-authorise rclone with the `drive.file` scope? This changes §8.7's last bullet and §10.9 and needs a decision entry.
2. **HA SSH key (M-2):** which `authorized_keys` variant did the Home Assistant add-on accept at checklist step 3? If B (plain key), is a dedicated non-root user possible on that Pi?
3. **Picture limit (M-3):** may AC-4.4's 8000 × 8000 become a pixel-count limit (for example 16 M pixels) or a decoder allocation limit, given the 160 MiB container?
4. **APK download rate (L-3) and `fcm_token` length (L-4):** are a separate per-IP limit for `/apk/files/*` and a 256-character cap acceptable as new §8.6/§9.6 numbers?
5. **After the first deploy (I-5):** one check of Caddy's access log that `request.remote_ip` shows public client addresses, so the per-IP limits key on real clients.