Graham McIntire
a3b97f9e98
- Radio.normalize_proposed/1 now Map.take/2 the allowed-edit keys
before normalization. Owner / admin / pending-edit submit paths all
funnel through this boundary, so a crafted form can no longer
mass-assign user_id, flagged_invalid, inserted_at, etc. on a
contact via String.to_existing_atom -> Ecto.Changeset.change.
- Accounts.revoke_api_token/2 rescues Ecto.Query.CastError and
returns {:error, :not_found} so a malformed UUID in
DELETE /api/v1/me/api-tokens/:id renders the API's clean 404
problem+json instead of a 500.
- ProfilesFile.read_etf decodes with :erlang.binary_to_term(bin, [:safe]).
Defense-in-depth against tampered on-disk profiles (atom-table
exhaustion via untrusted ETF).
- Regression tests: contact_edit_test.exs covers the rejected
mass-assignment fields; accounts_api_token_test.exs covers the
malformed-UUID path.
89 lines
4 KiB
Markdown
89 lines
4 KiB
Markdown
# Bugs Found 2026-05-11
|
|
|
|
The first batch (7 items: pending-beacon access control, per_page contract,
|
|
rate-limiter ETS lifecycle, unbounded grid endpoints, NEXRAD PNG quadratic
|
|
unfilter, LiveTableFooter parser, ScoresFile parser) was fixed and removed
|
|
from this file as each was resolved. The items below are additional bugs
|
|
surfaced by a follow-up audit.
|
|
|
|
## A. `String.to_integer` on user-controlled `band` param crashes LiveViews
|
|
|
|
**Status:** Fixed.
|
|
**Severity:** High
|
|
**Category:** Availability / unhandled crash on user input
|
|
**Files:**
|
|
- `lib/microwaveprop_web/live/path_live.ex` (formerly lines 227, 284)
|
|
- `lib/microwaveprop_web/live/map_live.ex` (formerly line 198)
|
|
|
|
`PathLive.handle_event("calculate", …)` and the auto-calculate path both
|
|
called `String.to_integer(params["band"])` directly on the `band` form
|
|
parameter. `MapLive.handle_event("select_band", %{"value" => band}, …)`
|
|
did the same. A non-numeric `band` value — supplied via a hand-crafted
|
|
URL (`/path?band=abc`) or a misbehaving JS hook — raised `ArgumentError`
|
|
inside the LiveView process and crashed the channel.
|
|
|
|
**Fix:** Use the existing `LiveHelpers.parse_int/2` (PathLive) and the
|
|
existing `parse_band_param/1` plus a small `normalize_band_event/1` helper
|
|
(MapLive). Unknown bands now fall back to a safe default or a no-op
|
|
instead of crashing the process.
|
|
|
|
## B. Mass assignment on `Radio.apply_owner_edit/3`
|
|
|
|
**Status:** Fixed.
|
|
**Severity:** High
|
|
**Category:** OWASP A04 Insecure Design / mass assignment
|
|
**Files:**
|
|
- `lib/microwaveprop/radio.ex`
|
|
- `lib/microwaveprop_web/live/contact_live/show.ex`
|
|
|
|
`apply_owner_edit` and `apply_admin_edit` accept the LiveView form's
|
|
`params` map, run it through `normalize_proposed/1`, then `diff_against_contact/2`,
|
|
then `apply_edit_to_contact/2`. The final stage calls
|
|
`String.to_existing_atom(key)` for every key in the diff and pipes the
|
|
resulting `{atom, value}` pairs into `Ecto.Changeset.change(contact, …)`.
|
|
|
|
`normalize_proposed/1` did not whitelist keys, so an owner could craft
|
|
a form POST with arbitrary fields — `user_id`, `flagged_invalid`,
|
|
`inserted_at`, etc. — and have them persisted onto their own contact.
|
|
|
|
**Fix:** Tighten `Radio.normalize_proposed/1` to `Map.take/2` only the
|
|
known editable keys (`station1/2`, `grid1/2`, `band`, `mode`,
|
|
`qso_timestamp`, `height1/2_ft`, `private`) before any normalization /
|
|
diffing. All three call sites (owner edit, admin edit, pending edit)
|
|
share this boundary, so no other change was needed. Regression test
|
|
in `contact_edit_test.exs`.
|
|
|
|
## C. `Accounts.revoke_api_token/2` raised `Ecto.Query.CastError` on a malformed UUID
|
|
|
|
**Status:** Fixed.
|
|
**Severity:** Low
|
|
**Category:** Availability / API contract
|
|
|
|
`Repo.get_by(UserApiToken, id: token_id, …)` raises `Ecto.Query.CastError`
|
|
when `token_id` does not parse as a UUID. Phoenix.Ecto's
|
|
`Plug.Exception` impl converts this to a 400 in the browser pipeline,
|
|
but the API path (`DELETE /api/v1/me/api-tokens/:id`) bypasses the
|
|
Plug.Exception fallback for actions that already return tagged tuples,
|
|
so the malformed-UUID path leaked a 500 with a generic error body
|
|
instead of the API's structured `:not_found` problem+json.
|
|
|
|
**Fix:** Rescue `Ecto.Query.CastError` in `revoke_api_token/2` and
|
|
return `{:error, :not_found}` — same shape that propagates as a clean
|
|
404 through the existing API fallback. Regression test in
|
|
`accounts_api_token_test.exs`.
|
|
|
|
## D. `:erlang.binary_to_term/1` on disk-loaded propagation profiles
|
|
|
|
**Status:** Fixed (defense-in-depth — pre-existing low risk).
|
|
**Severity:** Low
|
|
**Category:** OWASP A08 Software Integrity Failures
|
|
**File:** `lib/microwaveprop/propagation/profiles_file.ex`
|
|
|
|
`read_etf/1` decoded `.etf` files from `/data/profiles/...` with
|
|
`:erlang.binary_to_term(binary)` (no `:safe` option). Files are written
|
|
by the propagation pipeline, so the realistic threat is filesystem
|
|
tampering — by which point the box is already compromised. Still, the
|
|
unsafe variant allows atom-table exhaustion DoS.
|
|
|
|
**Fix:** Pass `[:safe]` so unknown atoms and external function
|
|
references are rejected on decode.
|