fix: limit and sanitize public status response #31

graham · 2026-09-14T23:51:51-05:00

graham commented

2026-09-14 23:51:51 -05:00

Fixes W12 from bugs.md.

/status.json moved from the unlimited health scope onto the shared :public_api limiter (100/min per IP)
the payload is cached in the existing :query_cache ETS for 5 seconds, bounding the cluster-wide LeaderElection call
login_id and filter dropped from the response — the APRS-IS login and server-side filter are operational details, not public status
regression tests assert the leak fields are absent and the limiter returns 429 past 100 requests; cache-aware tests evict the payload key so they observe fresh data

Verification: focused controller tests passed (104), full suite passed (2237), mix credo --strict, and commit hooks including Dialyzer passed.

Fixes W12 from bugs.md. - `/status.json` moved from the unlimited health scope onto the shared `:public_api` limiter (100/min per IP) - the payload is cached in the existing `:query_cache` ETS for 5 seconds, bounding the cluster-wide `LeaderElection` call - `login_id` and `filter` dropped from the response — the APRS-IS login and server-side filter are operational details, not public status - regression tests assert the leak fields are absent and the limiter returns 429 past 100 requests; cache-aware tests evict the payload key so they observe fresh data Verification: focused controller tests passed (104), full suite passed (2237), `mix credo --strict`, and commit hooks including Dialyzer passed.

graham added 1 commit

2026-09-14 23:51:51 -05:00

fix: limit and sanitize public status response

Elixir CI / Dialyzer (pull_request) Successful in 38s

Details

Elixir CI / Build and test (pull_request) Successful in 1m41s

Details

Elixir CI / Build and Push Docker Image (pull_request) Has been skipped

Details

skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread

Details

ce50b07264

skippy-bot reviewed

2026-09-14 23:59:28 -05:00

skippy-bot left a comment

🤖 Skippy PR review

2 findings — 2 blocking before merge.

Severity	Location	Issue
🟡 Warning	`lib/aprsme_web/controllers/page_controller.ex:147`	Only the JSON path is sanitised — /status still renders login_id and filter
🟡 Warning	`lib/aprsme_web/router.ex:103`	Limiter bucket is shared with :browser and :api, so /status.json traffic eats the v1 API allowance

_{Reviewed ce50b0726491. Comment skippy review to re-run.}

### 🤖 Skippy PR review **2 findings** — 2 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟡 Warning | `lib/aprsme_web/controllers/page_controller.ex:147` | Only the JSON path is sanitised — /status still renders login_id and filter | | 🟡 Warning | `lib/aprsme_web/router.ex:103` | Limiter bucket is shared with :browser and :api, so /status.json traffic eats the v1 API allowance | <sub>Reviewed `ce50b0726491`. Comment `skippy review` to re-run.</sub>

lib/aprsme_web/controllers/page_controller.ex

					
				@ -142,0 +144,4 @@

				  # `/status.json` is public and hits the cluster-wide LeaderElection call,

				  # so the payload is cached briefly (the page polls at most every few

				  # seconds) and never includes the APRS-IS login id or server-side filter.

🟡 Warning — Only the JSON path is sanitised — /status still renders login_id and filter

The JSON no longer carries them, but the same values are still served publicly: lib/aprsme_web/live/status_live/index.ex:181 renders Login ID: {@aprs_status.login_id} and :213 renders Filter: {@aprs_status.filter}, and /status is a plain pipe_through :browser + mount_current_user route with no auth check. So curl -s https://aprs.me/status still hands out the APRS-IS login id and server-side filter that W12 wants gone — the leak is only closed on the JSON path. Either drop both fields from the LiveView (template and the get_aprs_status/0 assigns at :444-445), or state that the page is intentionally allowed to show them.

**🟡 Warning — Only the JSON path is sanitised — /status still renders login_id and filter** The JSON no longer carries them, but the same values are still served publicly: `lib/aprsme_web/live/status_live/index.ex:181` renders `Login ID: {@aprs_status.login_id}` and `:213` renders `Filter: {@aprs_status.filter}`, and `/status` is a plain `pipe_through :browser` + `mount_current_user` route with no auth check. So `curl -s https://aprs.me/status` still hands out the APRS-IS login id and server-side filter that W12 wants gone — the leak is only closed on the JSON path. Either drop both fields from the LiveView (template and the `get_aprs_status/0` assigns at `:444-445`), or state that the page is intentionally allowed to show them.

skippy-bot marked this conversation as resolved

lib/aprsme_web/router.ex Outdated

					
				@ -98,0 +100,4 @@

				  # Public status endpoint — cluster-wide status call behind the shared

				  # API limiter; the payload itself is cached for a few seconds.

				  scope "/", AprsmeWeb do

				    pipe_through :public_api

🟡 Warning — Limiter bucket is shared with :browser and :api, so /status.json traffic eats the v1 API allowance

RateLimiter keys on "#{prefix}:#{key}" with prefix defaulting to "rate_limit" (lib/aprsme_web/plugs/rate_limiter.ex:31,46) and Hammer's fix_window keeps one counter per {key, window} — so :browser (200/min), :api (100/min) and :public_api (100/min) all resolve to the same entry for an IP, each only checking its own limit against that shared count. Adding /status.json here means status polling and /api/v1 traffic from one address draw on one 100/min budget: a poller behind CGNAT/office NAT burns the allowance the v1 API is measured against, and every API call from that IP starts returning 429 even though it never touched the API (and 100 routed requests in the same minute already 429 /status.json). Namespace the route — plug RateLimiter, scale: 60_000, limit: 100, prefix: "status_rate_limit" in a dedicated pipeline, or add a prefix to :public_api.

**🟡 Warning — Limiter bucket is shared with :browser and :api, so /status.json traffic eats the v1 API allowance** `RateLimiter` keys on `"#{prefix}:#{key}"` with `prefix` defaulting to `"rate_limit"` (`lib/aprsme_web/plugs/rate_limiter.ex:31,46`) and Hammer's fix_window keeps one counter per `{key, window}` — so `:browser` (200/min), `:api` (100/min) and `:public_api` (100/min) all resolve to the same entry for an IP, each only checking its own limit against that shared count. Adding `/status.json` here means status polling and `/api/v1` traffic from one address draw on one 100/min budget: a poller behind CGNAT/office NAT burns the allowance the v1 API is measured against, and every API call from that IP starts returning 429 even though it never touched the API (and 100 routed requests in the same minute already 429 `/status.json`). Namespace the route — `plug RateLimiter, scale: 60_000, limit: 100, prefix: "status_rate_limit"` in a dedicated pipeline, or add a prefix to `:public_api`.

skippy-bot marked this conversation as resolved

graham added 1 commit

2026-09-15 12:04:32 -05:00

fix: drop login/filter from status page, namespace status.json limiter

Elixir CI / Build and test (pull_request) Failing after 56s

Details

skippy-bot/review Skippy review: clean — no open findings

Details

Elixir CI / Dialyzer (pull_request) Successful in 2m42s

Details

Elixir CI / Build and Push Docker Image (pull_request) Has been skipped

Details

735ad720bd

skippy-bot commented

2026-09-15 12:07:12 -05:00

First-time contributor

Both earlier findings resolved in 735ad720 — verified against the code as it stands, nothing new in ce50b072..735ad720.

The /status LiveView no longer renders or assigns login_id / filter (template blocks and the default_status/0 keys are both gone), so the page and its LiveView payload are clean.
/status.json now runs on its own status_rate_limit bucket (router.ex:36), so status polling and /api/v1 traffic from one IP no longer share a 100/min counter.

Nothing left open from this review. 👍

**Both earlier findings resolved** in `735ad720` — verified against the code as it stands, nothing new in `ce50b072..735ad720`. - The `/status` LiveView no longer renders or assigns `login_id` / `filter` (template blocks and the `default_status/0` keys are both gone), so the page and its LiveView payload are clean. - `/status.json` now runs on its own `status_rate_limit` bucket (`router.ex:36`), so status polling and `/api/v1` traffic from one IP no longer share a 100/min counter. Nothing left open from this review. 👍

graham merged commit f42ed54569 into main

2026-09-15 16:36:00 -05:00

graham deleted branch fix/w12-status-response

2026-09-15 16:36:01 -05:00

graham referenced this pull request from a commit

2026-09-15 16:36:02 -05:00

fix: limit and sanitize public status response (#31)

graham referenced this pull request from a commit

2026-09-15 18:35:28 -05:00

test: assert login_id stays off the public status page

Sign in to join this conversation.

No reviewers