198 lines
9.3 KiB
Markdown
198 lines
9.3 KiB
Markdown
# Bugs Found 2026-05-12
|
|
|
|
## 4. ADIF parser incorrectly identifies tags inside field values
|
|
|
|
**Severity:** Medium
|
|
**Category:** Logic / Parsing
|
|
**Status:** OPEN
|
|
|
|
The `AdifImport.parse_fields/1` function uses `Regex.scan/3` globally on the record string. ADIF field values can contain `<` and `:` characters. If a value contains something that looks like an ADIF tag (e.g., a note containing `<MODE:2>CW`), the parser will incorrectly identify it as a new tag, potentially overwriting existing fields or creating bogus ones.
|
|
|
|
**Reproduction:**
|
|
```elixir
|
|
adif = "<CALL:6>N0CALL<BAND:3>3cm<NOTES:20>Contains <MODE:2>CW inside<EOR>"
|
|
# Notes will be "Contains <MODE:2>CW "
|
|
# Mode will be "CW" (overwritten or newly added, even though it was part of notes)
|
|
```
|
|
|
|
**Suggested fix:**
|
|
Rewrite `parse_fields/1` to be a sequential parser. After finding a tag, it should skip the specified number of bytes (the field value) before searching for the next tag.
|
|
|
|
|
|
## 5. MechanismClassifier uses hardcoded Sporadic-E MUF factor
|
|
|
|
**Severity:** Low
|
|
**Category:** Logic / Consistency
|
|
**Status:** OPEN
|
|
|
|
`MechanismClassifier.try_sporadic_e/1` uses a hardcoded factor of `5.0` to estimate the Sporadic-E MUF from foEs (`muf_mhz = 5.0 * foes`). However, the `Microwaveprop.Propagation.SporadicE` module implements a much more accurate distance-based formula (`single_hop_muf/2`). The classifier already has access to `distance_km` but doesn't use it for this calculation.
|
|
|
|
**Suggested fix:**
|
|
Update `MechanismClassifier.try_sporadic_e/1` to call `SporadicE.single_hop_muf/2` using the contact's distance.
|
|
|
|
---
|
|
|
|
## 6. `Radio.ensure_positions!/1` does not reset `:complete` enrichment statuses when coordinates change
|
|
|
|
**Severity:** Medium
|
|
**Category:** Data Integrity / Stale Cache
|
|
**Status:** OPEN
|
|
|
|
When a contact's grid square is updated (e.g., from a 4-character to an 8-character grid), `Radio.ensure_positions!/1` recomputes the coordinates. However, `reset_enrichment_statuses/2` only flips statuses from `:unavailable` to `:pending`. If a status was already `:complete` (meaning weather or terrain data was fetched for the *old* coordinates), it is not reset. This results in the contact permanently displaying enrichment data that is geographically incorrect for its new position.
|
|
|
|
**Suggested fix:**
|
|
Update `Radio.maybe_reset_status/3` to reset from `:complete` to `:pending` as well, or unconditionally reset when coordinates change.
|
|
|
|
## 7. `Radio.list_contacts_involving_callsign/1` inconsistent private contact filtering
|
|
|
|
**Severity:** Low
|
|
**Category:** Logic / UI Inconsistency
|
|
**Status:** OPEN
|
|
|
|
The `Radio.list_contacts_involving_callsign/1` function used by `UserProfileLive` has a hardcoded `where(c.private == false)` filter. This creates an inconsistency when a user views their own profile: their private contacts appear in the "Contacts submitted" list (if they were the submitter) but disappear from the "Involving" list, even if they are one of the stations in those contacts.
|
|
|
|
**Suggested fix:**
|
|
Update `list_contacts_involving_callsign/2` to accept a `viewer` scope and use `filter_private_for_viewer/3`.
|
|
|
|
## 8. `HrrrNativeClient.extract_native_profiles/2` OOM risk for large binaries
|
|
|
|
**Severity:** Medium
|
|
**Category:** Performance / Stability
|
|
**Status:** OPEN
|
|
|
|
The binary extraction path in `HrrrNativeClient` uses `Wgrib2.extract_grid` (which employs the `-lola` grid extraction). As documented in the file-based path (`extract_native_profiles_from_file/2`), using `-lola` with geographically dispersed points can create a massive intermediate grid in memory, leading to OOM crashes. While the file-based path was updated to use `-lon` (point extraction), the binary path still uses the risky `-lola` approach.
|
|
|
|
**Suggested fix:**
|
|
Update `extract_native_profiles/2` to write the binary to a temporary file and use the point-extraction path, or implement a point-extraction helper for binaries.
|
|
|
|
## 9. `HrrrNativeGridWorker.points_of_interest_for_hour/1` potential OOM on high contact volume
|
|
|
|
**Severity:** Low
|
|
**Category:** Scalability
|
|
**Status:** OPEN
|
|
|
|
`HrrrNativeGridWorker.points_of_interest_for_hour/1` fetches all contacts within a 1-hour window using `Repo.all/1` without any limit or batching. If the system scales to a high volume of contacts (e.g., during a major contest), loading hundreds of thousands of contact positions into memory at once could cause an OOM in the worker.
|
|
|
|
**Suggested fix:**
|
|
Use a stream or batch the query for contact positions.
|
|
|
|
---
|
|
|
|
# Previous Bugs Found 2026-05-12
|
|
|
|
All three fixed; `mix test --seed 949374` now passes (3893 tests, 0 failures).
|
|
|
|
## 1. Public profile leaks private contacts and pending beacons [FIXED]
|
|
|
|
**Severity:** High
|
|
**Category:** Privacy / authorization bypass
|
|
**Files:**
|
|
- `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`
|
|
|
|
`/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.
|
|
|
|
`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.
|
|
|
|
`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.
|
|
|
|
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:629`
|
|
- `lib/microwaveprop/radio.ex:632`
|
|
- `lib/microwaveprop/radio.ex:633`
|
|
- `lib/microwaveprop/radio/contact.ex:109`
|
|
- `lib/microwaveprop/radio/contact.ex:173`
|
|
|
|
`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`.
|
|
|
|
Confirmed with:
|
|
|
|
```text
|
|
Radio.create_contact(valid_attrs_without_submitter_email, user.id)
|
|
#=> {:error, %Ecto.Changeset{errors: [submitter_email: {"can't be blank", []}]}}
|
|
```
|
|
|
|
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.
|
|
|
|
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`.
|
|
|
|
## 3. Full test suite is order-dependent around Valkey-backed `GridCache.clear/0` [FIXED]
|
|
|
|
**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`
|
|
|
|
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.
|
|
|
|
Failure shape:
|
|
|
|
```text
|
|
Mox.UnexpectedCallError: no expectation defined for
|
|
Microwaveprop.Valkey.MockAdapter.command/3
|
|
args: [Microwaveprop.Valkey.Conn, ["SCAN", "0", "MATCH", "prop:wg:*", "COUNT", "500"], ...]
|
|
```
|
|
|
|
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.
|