fix(security,test): viewer-aware profile queries + create_contact ordering + valkey test isolation

- /u/:callsign profile no longer leaks private contacts or pending beacons;
  Radio.list_contacts_for_user/2 and Beacons.list_beacons_for_user/2 now
  filter by viewer (owner/admin see everything, others see only public).
- Radio.create_contact/2 places user_id on the struct before
  submission_changeset so validate_user_or_email/1 accepts authenticated
  submissions that omit submitter_email.
- valkey_test.exs runs async: false; its setup mutates global Application
  env and Process registry, which otherwise crashed concurrent ConnCase
  GridCache.clear/0 calls (Mox.UnexpectedCallError on SCAN).
This commit is contained in:
Graham McIntire 2026-05-12 08:49:08 -05:00
parent a3b97f9e98
commit 0704253af8
No known key found for this signature in database
GPG key ID: F4ABF488E6029E59
10 changed files with 303 additions and 104 deletions

167
bugs.md
View file

@ -1,89 +1,118 @@
# Bugs Found 2026-05-11
# Bugs Found 2026-05-12
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.
All three fixed; `mix test --seed 949374` now passes (3893 tests, 0 failures).
## A. `String.to_integer` on user-controlled `band` param crashes LiveViews
## 1. Public profile leaks private contacts and pending beacons [FIXED]
**Status:** Fixed.
**Severity:** High
**Category:** Availability / unhandled crash on user input
**Severity:** High
**Category:** Privacy / authorization bypass
**Files:**
- `lib/microwaveprop_web/live/path_live.ex` (formerly lines 227, 284)
- `lib/microwaveprop_web/live/map_live.ex` (formerly line 198)
- `lib/microwaveprop_web/live/user_profile_live.ex:21`
- `lib/microwaveprop_web/live/user_profile_live.ex:22`
- `lib/microwaveprop_web/live/user_profile_live.ex:92`
- `lib/microwaveprop_web/live/user_profile_live.ex:130`
- `lib/microwaveprop/beacons.ex:61`
`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.
`/u/:callsign` is mounted in the public LiveView session, but it loads
`Radio.list_contacts_for_user(user)` and `Beacons.list_beacons_for_user(user)`
without checking whether the visitor is the profile owner or an admin.
**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.
`list_contacts_for_user/1` returns every submitted contact for that user,
including contacts where `private == true`. The profile template then renders
those rows and links to `/contacts/:id`. The contact detail page correctly
404s for unauthorized viewers, but the public profile has already exposed the
contact timestamp, stations, band, mode, and distance.
## B. Mass assignment on `Radio.apply_owner_edit/3`
`list_beacons_for_user/1` explicitly returns approved and pending beacons for
the public profile page. The profile template renders pending beacon callsign,
frequency, grid, keying, status, and link. This contradicts the beacon visibility
model where pending beacons should only be visible to the submitter and admins.
**Status:** Fixed.
**Severity:** High
**Category:** OWASP A04 Insecure Design / mass assignment
Suggested fix: split the profile query by viewer. Anonymous/non-owner visitors
should see only non-private contacts and approved beacons. Owners/admins can see
their private contacts and pending beacons. Add LiveView tests for anonymous,
owner, and admin views.
## 2. `Radio.create_contact/2` rejects authenticated submissions unless callers also pass an email [FIXED]
**Severity:** Medium
**Category:** Broken context contract / validation ordering
**Files:**
- `lib/microwaveprop/radio.ex`
- `lib/microwaveprop_web/live/contact_live/show.ex`
- `lib/microwaveprop/radio.ex:629`
- `lib/microwaveprop/radio.ex:632`
- `lib/microwaveprop/radio.ex:633`
- `lib/microwaveprop/radio/contact.ex:109`
- `lib/microwaveprop/radio/contact.ex:173`
`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, …)`.
`Radio.create_contact(attrs, user_id)` adds `user_id` after
`Contact.submission_changeset/2` has already run `validate_user_or_email/1`.
That validation only sees fields cast from `attrs`, so a valid authenticated
create with `user_id` but no `submitter_email` returns an invalid changeset with
`submitter_email: can't be blank`.
`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.
Confirmed with:
**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`.
```text
Radio.create_contact(valid_attrs_without_submitter_email, user.id)
#=> {:error, %Ecto.Changeset{errors: [submitter_email: {"can't be blank", []}]}}
```
## C. `Accounts.revoke_api_token/2` raised `Ecto.Query.CastError` on a malformed UUID
Current web/API call sites mask this by inserting the user's email into params
before calling the context, but the public function's contract and implementation
disagree. Any future trusted caller that relies on the `user_id` argument alone
will fail unexpectedly.
**Status:** Fixed.
**Severity:** Low
**Category:** Availability / API contract
Suggested fix: put `user_id` into the struct or attrs before calling
`Contact.submission_changeset/2`, or move the ownership validation to after
`maybe_put_user_id/2`. Add a context-level regression test that passes
`user_id` without `submitter_email`.
`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.
## 3. Full test suite is order-dependent around Valkey-backed `GridCache.clear/0` [FIXED]
**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`.
**Severity:** Medium
**Category:** Test isolation / flaky suite
**Files:**
- `test/support/conn_case.ex:45`
- `test/microwaveprop/weather/grid_cache_valkey_test.exs:26`
- `test/microwaveprop/weather/grid_cache_valkey_test.exs:35`
- `lib/microwaveprop/weather/grid_cache.ex:477`
## D. `:erlang.binary_to_term/1` on disk-loaded propagation profiles
One `mix test` run failed with 3 setup failures. `ConnCase` calls
`GridCache.clear/0` for every web test. If a Valkey cache test has registered
`Microwaveprop.Valkey.Conn` and swapped `:valkey_adapter`, `GridCache.clear/0`
takes the Valkey path and calls `Valkey.scan_match/1`. In the failed run, that
reached `Microwaveprop.Valkey.MockAdapter.command/3` without a `SCAN`
expectation, crashing unrelated web tests during setup.
**Status:** Fixed (defense-in-depth — pre-existing low risk).
**Severity:** Low
**Category:** OWASP A08 Software Integrity Failures
**File:** `lib/microwaveprop/propagation/profiles_file.ex`
Failure shape:
`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.
```text
Mox.UnexpectedCallError: no expectation defined for
Microwaveprop.Valkey.MockAdapter.command/3
args: [Microwaveprop.Valkey.Conn, ["SCAN", "0", "MATCH", "prop:wg:*", "COUNT", "500"], ...]
```
**Fix:** Pass `[:safe]` so unknown atoms and external function
references are rejected on decode.
Verification:
```text
mix test --seed 949374
# 3880 tests, 3 failures, 6 skipped
mix precommit
# 3880 tests, 0 failures, 6 skipped
```
The subsequent passing `mix precommit` run makes this look order-dependent
rather than a deterministic failure.
Suggested fix: make Valkey test setup restore global process/env state before
other tests can observe it, or avoid global `Process.register/2` for `Conn`.
Also consider making `ConnCase` force the ETS fallback or installing a default
Valkey mock stub for `GridCache.clear/0`.
## Verification Notes
- `mix compile --warnings-as-errors` passed.
- `mix credo --strict` reported design/refactoring/style issues only; no bug-class findings were added from Credo.
- `mix test --seed 949374` failed as described in bug 3.
- `mix precommit` passed afterward, confirming the checked-in code still clears the normal project gate for this run.

View file

@ -59,16 +59,27 @@ defmodule Microwaveprop.Beacons do
end
@doc """
Returns every beacon (approved or pending) that the given user submitted,
newest first. Used by the public `/u/:callsign` profile page.
Returns beacons that the given user submitted, newest first. Used by
the `/u/:callsign` profile page. Pending (unapproved) beacons are
filtered out unless the viewer is the profile owner or an admin
the public profile must not leak the existence of pending drafts.
"""
@spec list_beacons_for_user(User.t()) :: [Beacon.t()]
def list_beacons_for_user(%User{id: user_id}) do
Repo.all(
from b in Beacon,
where: b.user_id == ^user_id,
order_by: [desc: b.inserted_at]
)
@spec list_beacons_for_user(User.t(), User.t() | nil) :: [Beacon.t()]
def list_beacons_for_user(%User{} = owner, viewer \\ nil) do
Beacon
|> where([b], b.user_id == ^owner.id)
|> filter_unapproved_for_viewer(owner, viewer)
|> order_by([b], desc: b.inserted_at)
|> Repo.all()
end
defp filter_unapproved_for_viewer(query, %User{id: owner_id}, %User{id: viewer_id}) when owner_id == viewer_id,
do: query
defp filter_unapproved_for_viewer(query, _owner, %User{is_admin: true}), do: query
defp filter_unapproved_for_viewer(query, _owner, _viewer) do
where(query, [b], b.approved == true)
end
@doc "Gets a single beacon. Raises if not found."

View file

@ -119,18 +119,28 @@ defmodule Microwaveprop.Radio do
end
@doc """
Returns all contacts submitted by the given user, newest first. Used
by the public `/u/:callsign` profile page; expected to return a small
list since only logged-in submissions carry a `user_id`.
Returns contacts submitted by the given user, newest first. Used by
the `/u/:callsign` profile page. Private contacts are filtered out
unless the viewer is the profile owner or an admin anonymous or
third-party viewers must not see private rows.
"""
@spec list_contacts_for_user(User.t()) :: [Contact.t()]
def list_contacts_for_user(%User{id: user_id}) do
@spec list_contacts_for_user(User.t(), User.t() | nil) :: [Contact.t()]
def list_contacts_for_user(%User{} = owner, viewer \\ nil) do
Contact
|> where([c], c.user_id == ^user_id)
|> where([c], c.user_id == ^owner.id)
|> filter_private_for_viewer(owner, viewer)
|> order_by([c], desc: c.qso_timestamp, desc: c.id)
|> Repo.all()
end
defp filter_private_for_viewer(query, %User{id: owner_id}, %User{id: viewer_id}) when owner_id == viewer_id, do: query
defp filter_private_for_viewer(query, _owner, %User{is_admin: true}), do: query
defp filter_private_for_viewer(query, _owner, _viewer) do
where(query, [c], c.private == false)
end
@doc """
Returns the 100 most recent contacts where the given callsign appears
as either `station1` or `station2`, newest first. Matching is
@ -627,10 +637,11 @@ defmodule Microwaveprop.Radio do
@spec create_contact(map(), String.t() | nil) ::
{:ok, Contact.t()} | {:error, Ecto.Changeset.t()} | {:error, :duplicate, Contact.t()}
def create_contact(attrs, user_id \\ nil) do
changeset =
%Contact{}
|> Contact.submission_changeset(normalize_band_attr(attrs))
|> maybe_put_user_id(user_id)
# Place user_id on the struct BEFORE running submission_changeset so
# validate_user_or_email/1 can see it. Otherwise an authenticated
# caller that omits submitter_email gets a spurious "can't be blank"
# error even though user_id satisfies the ownership invariant.
changeset = Contact.submission_changeset(%Contact{user_id: user_id}, normalize_band_attr(attrs))
if changeset.valid? do
insert_validated_contact(changeset)
@ -664,9 +675,6 @@ defmodule Microwaveprop.Radio do
end
end
defp maybe_put_user_id(changeset, nil), do: changeset
defp maybe_put_user_id(changeset, user_id), do: Ecto.Changeset.put_change(changeset, :user_id, user_id)
defp insert_validated_contact(changeset) do
if existing = find_duplicate_contact(changeset) do
{:error, :duplicate, existing}

View file

@ -41,7 +41,7 @@ defmodule MicrowavepropWeb.Api.V1.MeController do
def contacts(conn, _params) do
user = conn.assigns.current_api_user
contacts = Radio.list_contacts_for_user(user)
contacts = Radio.list_contacts_for_user(user, user)
json(conn, ContactJSON.index(%{contacts: contacts, viewer: user}))
end
@ -49,7 +49,7 @@ defmodule MicrowavepropWeb.Api.V1.MeController do
def beacons(conn, _params) do
user = conn.assigns.current_api_user
beacons = Beacons.list_beacons_for_user(user)
beacons = Beacons.list_beacons_for_user(user, user)
json(conn, BeaconJSON.index(%{beacons: beacons}))
end

View file

@ -20,7 +20,7 @@ defmodule MicrowavepropWeb.Api.V1.ProfileController do
user ->
contacts = Radio.list_contacts_involving_callsign(user.callsign)
beacons = user |> Beacons.list_beacons_for_user() |> Enum.filter(& &1.approved)
beacons = Beacons.list_beacons_for_user(user)
json(conn, %{
user: UserJSON.public(user).data,

View file

@ -18,8 +18,9 @@ defmodule MicrowavepropWeb.UserProfileLive do
|> push_navigate(to: ~p"/")}
user ->
contacts = Radio.list_contacts_for_user(user)
beacons = Beacons.list_beacons_for_user(user)
viewer = socket.assigns[:current_scope] && socket.assigns.current_scope.user
contacts = Radio.list_contacts_for_user(user, viewer)
beacons = Beacons.list_beacons_for_user(user, viewer)
involving = Radio.list_contacts_involving_callsign(user.callsign)
{:ok,

View file

@ -132,8 +132,8 @@ defmodule Microwaveprop.BeaconsTest do
end
end
describe "list_beacons_for_user/1" do
test "returns both approved and pending beacons owned by the user" do
describe "list_beacons_for_user/2" do
test "returns both approved and pending beacons when the viewer is the owner" do
user = user_fixture()
other = user_fixture()
@ -143,10 +143,37 @@ defmodule Microwaveprop.BeaconsTest do
pending = beacon_fixture(user, callsign: "W5PND")
_theirs = beacon_fixture(other, callsign: "W5THR")
ids = user |> Beacons.list_beacons_for_user() |> Enum.map(& &1.id) |> Enum.sort()
ids = user |> Beacons.list_beacons_for_user(user) |> Enum.map(& &1.id) |> Enum.sort()
assert ids == Enum.sort([approved.id, pending.id])
end
test "hides pending beacons from anonymous viewers" do
user = user_fixture()
{:ok, approved} = Beacons.create_beacon(user, valid_beacon_attrs())
{:ok, approved} = Beacons.approve_beacon(approved)
_pending = beacon_fixture(user, callsign: "W5PND")
assert [found] = Beacons.list_beacons_for_user(user)
assert found.id == approved.id
end
test "hides pending beacons from other authenticated viewers" do
user = user_fixture()
other = user_fixture()
_pending = beacon_fixture(user, callsign: "W5PND")
assert Beacons.list_beacons_for_user(user, other) == []
end
test "shows pending beacons when the viewer is an admin" do
user = user_fixture()
admin = %{user_fixture() | is_admin: true}
pending = beacon_fixture(user, callsign: "W5PND")
assert [found] = Beacons.list_beacons_for_user(user, admin)
assert found.id == pending.id
end
test "returns an empty list when the user owns nothing" do
user = user_fixture()
assert Beacons.list_beacons_for_user(user) == []

View file

@ -512,6 +512,19 @@ defmodule Microwaveprop.RadioTest do
assert {:error, changeset} = Radio.create_contact(attrs)
assert errors_on(changeset).grid1
end
test "accepts authenticated submission without submitter_email" do
# Regression: validate_user_or_email must see user_id, not just attrs.
# Previously user_id was applied AFTER submission_changeset, so this
# call returned `{:error, %Ecto.Changeset{errors: [submitter_email: ...]}}`
# even though the user_id satisfies the ownership invariant.
user = user_fixture()
attrs = Map.delete(@valid_submission, :submitter_email)
assert {:ok, contact} = Radio.create_contact(attrs, user.id)
assert contact.user_id == user.id
assert is_nil(contact.submitter_email)
end
end
describe "list_contacts_for_user/1" do
@ -557,11 +570,38 @@ defmodule Microwaveprop.RadioTest do
assert Radio.list_contacts_for_user(user) == []
end
test "includes private contacts owned by the user" do
test "includes private contacts when the viewer is the owner" do
user = user_fixture()
private = create_contact(%{station1: "W5PRV", user_id: user.id, private: true})
assert [found] = Radio.list_contacts_for_user(user, user)
assert found.id == private.id
end
test "hides private contacts when the viewer is anonymous" do
user = user_fixture()
_private = create_contact(%{station1: "W5PRV", user_id: user.id, private: true})
public = create_contact(%{station1: "W5PUB", user_id: user.id, private: false})
assert [found] = Radio.list_contacts_for_user(user)
assert found.id == public.id
end
test "hides private contacts from other authenticated viewers" do
user = user_fixture()
other = user_fixture()
_private = create_contact(%{station1: "W5PRV", user_id: user.id, private: true})
assert Radio.list_contacts_for_user(user, other) == []
end
test "includes private contacts when the viewer is an admin" do
user = user_fixture()
admin = user_fixture()
admin = %{admin | is_admin: true}
private = create_contact(%{station1: "W5PRV", user_id: user.id, private: true})
assert [found] = Radio.list_contacts_for_user(user, admin)
assert found.id == private.id
end
end

View file

@ -1,5 +1,10 @@
defmodule Microwaveprop.ValkeyTest do
use ExUnit.Case, async: true
# Setup registers a process under the global `Microwaveprop.Valkey.Conn`
# name and swaps `:microwaveprop, :valkey_adapter` via Application.put_env.
# Both are global state, so this case must not run concurrently with other
# tests — otherwise ConnCase setups (which call GridCache.clear/0) see the
# Mox adapter without expectations and crash unrelated tests.
use ExUnit.Case, async: false
import Mox

View file

@ -6,6 +6,7 @@ defmodule MicrowavepropWeb.UserProfileLiveTest do
import Microwaveprop.BeaconsFixtures
import Phoenix.LiveViewTest
alias Microwaveprop.Accounts.User
alias Microwaveprop.Beacons
alias Microwaveprop.Radio.Contact
alias Microwaveprop.Repo
@ -80,7 +81,21 @@ defmodule MicrowavepropWeb.UserProfileLiveTest do
assert {:error, {:live_redirect, %{to: "/"}}} = live(conn, ~p"/u/NO1SUCH")
end
test "lists both approved and pending beacons so owners see their drafts", %{conn: conn} do
test "owners see both approved and pending beacons on their own profile", %{conn: conn} do
user = user_fixture(%{callsign: "W5ISP"})
{:ok, approved} = Beacons.create_beacon(user, valid_beacon_attrs())
{:ok, _approved} = Beacons.approve_beacon(approved)
_pending = beacon_fixture(user, callsign: "W5PND")
conn = log_in_user(conn, user)
{:ok, _lv, html} = live(conn, ~p"/u/#{user.callsign}")
assert html =~ "W5PND"
assert html =~ "Pending"
assert html =~ "Approved"
end
test "anonymous visitors do not see pending beacons", %{conn: conn} do
user = user_fixture(%{callsign: "W5ISP"})
{:ok, approved} = Beacons.create_beacon(user, valid_beacon_attrs())
{:ok, _approved} = Beacons.approve_beacon(approved)
@ -88,9 +103,72 @@ defmodule MicrowavepropWeb.UserProfileLiveTest do
{:ok, _lv, html} = live(conn, ~p"/u/#{user.callsign}")
refute html =~ "W5PND"
refute html =~ "Pending"
assert html =~ "Approved"
end
test "another logged-in user does not see pending beacons", %{conn: conn} do
owner = user_fixture(%{callsign: "W5ISP"})
other = user_fixture(%{callsign: "W5OTH"})
_pending = beacon_fixture(owner, callsign: "W5PND")
conn = log_in_user(conn, other)
{:ok, _lv, html} = live(conn, ~p"/u/#{owner.callsign}")
refute html =~ "W5PND"
refute html =~ "Pending"
end
test "admins see pending beacons on any profile", %{conn: conn} do
owner = user_fixture(%{callsign: "W5ISP"})
admin = user_fixture(%{callsign: "W5ADM"})
{1, _} =
Repo.update_all(
from(u in User, where: u.id == ^admin.id),
set: [is_admin: true]
)
_pending = beacon_fixture(owner, callsign: "W5PND")
conn = log_in_user(conn, %{admin | is_admin: true})
{:ok, _lv, html} = live(conn, ~p"/u/#{owner.callsign}")
assert html =~ "W5PND"
assert html =~ "Pending"
assert html =~ "Approved"
end
test "anonymous visitors do not see private contacts", %{conn: conn} do
user = user_fixture(%{callsign: "W5ISP"})
_public = contact_for(user, %{station1: "W5PUB"})
_hidden = contact_for(user, %{station1: "W5SECRET", private: true})
{:ok, _lv, html} = live(conn, ~p"/u/#{user.callsign}")
assert html =~ "W5PUB"
refute html =~ "W5SECRET"
end
test "owners see their own private contacts", %{conn: conn} do
user = user_fixture(%{callsign: "W5ISP"})
_hidden = contact_for(user, %{station1: "W5SECRET", private: true})
conn = log_in_user(conn, user)
{:ok, _lv, html} = live(conn, ~p"/u/#{user.callsign}")
assert html =~ "W5SECRET"
end
test "another logged-in user does not see private contacts", %{conn: conn} do
owner = user_fixture(%{callsign: "W5ISP"})
other = user_fixture(%{callsign: "W5OTH"})
_hidden = contact_for(owner, %{station1: "W5SECRET", private: true})
conn = log_in_user(conn, other)
{:ok, _lv, html} = live(conn, ~p"/u/#{owner.callsign}")
refute html =~ "W5SECRET"
end
test "renders the 'Contacts involving' section with contacts that mention the callsign as a remote station",
@ -128,7 +206,7 @@ defmodule MicrowavepropWeb.UserProfileLiveTest do
{1, _} =
Repo.update_all(
from(u in Microwaveprop.Accounts.User, where: u.id == ^user.id),
from(u in User, where: u.id == ^user.id),
set: [name: ""]
)