fix: strengthen API CSRF protection contract #32

graham · 2026-09-14T23:54:59-05:00

graham commented

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

Fixes W15 from bugs.md.

ApiCSRF no longer accepts a bare X-Requested-With: XMLHttpRequest header as sufficient for mutating requests — only an x-csrf-token that validates against the session token via Plug.CSRFProtection passes. The header carries no session binding and would have been a silent bypass on the first mutating API endpoint; since every API route is GET-only today, nothing breaks. Tests updated to assert the header alone now gets 403.

Verification: focused plug tests passed (11), full suite passed (2235), mix credo --strict, and commit hooks including Dialyzer passed.

Fixes W15 from bugs.md. `ApiCSRF` no longer accepts a bare `X-Requested-With: XMLHttpRequest` header as sufficient for mutating requests — only an `x-csrf-token` that validates against the session token via `Plug.CSRFProtection` passes. The header carries no session binding and would have been a silent bypass on the first mutating API endpoint; since every API route is GET-only today, nothing breaks. Tests updated to assert the header alone now gets 403. Verification: focused plug tests passed (11), full suite passed (2235), `mix credo --strict`, and commit hooks including Dialyzer passed.

graham added 1 commit

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

fix: strengthen API CSRF protection contract

Elixir CI / Dialyzer (pull_request) Successful in 39s

Details

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

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

72401e054f

skippy-bot reviewed

2026-09-14 23:56:40 -05:00

skippy-bot left a comment

🤖 Skippy PR review

3 findings — 2 blocking before merge.

Severity	Location	Issue
🟠 High	`lib/aprsme_web/plugs/api_csrf.ex:33`	The token path can never pass: the :api pipeline never fetches the session
🟡 Warning	`test/aprsme_web/plugs/api_csrf_test.exs:15`	Accept path now has no real coverage — a reject-everything plug stays green
🔵 Suggestion	`lib/aprsme_web/plugs/api_csrf.ex:13`	API docs still advertise the header this PR now rejects

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

### 🤖 Skippy PR review **3 findings** — 2 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟠 High | `lib/aprsme_web/plugs/api_csrf.ex:33` | The token path can never pass: the :api pipeline never fetches the session | | 🟡 Warning | `test/aprsme_web/plugs/api_csrf_test.exs:15` | Accept path now has no real coverage — a reject-everything plug stays green | | 🔵 Suggestion | `lib/aprsme_web/plugs/api_csrf.ex:13` | API docs still advertise the header this PR now rejects | <sub>Reviewed `72401e054f8b`. Comment `skippy review` to re-run.</sub>

lib/aprsme_web/plugs/api_csrf.ex

					
				@ -4,0 +10,4 @@

				  sufficient. Custom headers do block simple cross-site form posts, but the

				  header carries no session binding: anything that can script the origin

				  (or a future permissive CORS response) would satisfy it. Since every API

				  route is currently GET-only, requiring a real token breaks no caller and

🔵 Suggestion — API docs still advertise the header this PR now rejects

docs/web/api-rest.md:46-52 says API requests must include X-Requested-With: XMLHttpRequest or a valid CSRF token; after this change the first option is a hard 403. Update that section in the same PR — and note it can no longer promise 'valid CSRF token' for a session-less API client.

**🔵 Suggestion — API docs still advertise the header this PR now rejects** `docs/web/api-rest.md:46-52` says API requests must include `X-Requested-With: XMLHttpRequest` *or* a valid CSRF token; after this change the first option is a hard 403. Update that section in the same PR — and note it can no longer promise 'valid CSRF token' for a session-less API client.

skippy-bot marked this conversation as resolved

lib/aprsme_web/plugs/api_csrf.ex

					
				@ -26,1 +31,3 @@

				        conn

				    case get_req_header(conn, "x-csrf-token") do

				      [token] when byte_size(token) > 0 ->

				        validate_csrf_token(conn, token)

🟠 High — The token path can never pass: the :api pipeline never fetches the session

pipeline :api (router.ex:43-47) is :accepts + RateLimiter + ApiCSRF — no plug :fetch_session. The endpoint's plug Plug.Session only registers a fetch function; conn.private[:plug_session] is populated by fetch_session/2 alone. So get_session(conn, "_csrf_token") at line 41 raises ArgumentError "session not fetched, call fetch_session/2", the blanket rescue _ -> at lines 52-54 swallows it, and every non-GET request to /api/v1/* gets 403 — valid token or not. Fail-closed, so nothing is exploitable today, but the 'real token' contract this PR advertises is unreachable, and the first mutating endpoint 403s every legitimate caller with a token that is correct. Fix by adding plug :fetch_session to the :api pipeline ahead of ApiCSRF (and giving token holders a way to obtain one), or — if the JSON API is meant to be session-less — drop the session-bound scheme here instead of shipping a guard that can only deny.

**🟠 High — The token path can never pass: the :api pipeline never fetches the session** `pipeline :api` (router.ex:43-47) is `:accepts` + `RateLimiter` + `ApiCSRF` — no `plug :fetch_session`. The endpoint's `plug Plug.Session` only registers a fetch *function*; `conn.private[:plug_session]` is populated by `fetch_session/2` alone. So `get_session(conn, "_csrf_token")` at line 41 raises `ArgumentError "session not fetched, call fetch_session/2"`, the blanket `rescue _ ->` at lines 52-54 swallows it, and **every** non-GET request to `/api/v1/*` gets 403 — valid token or not. Fail-closed, so nothing is exploitable today, but the 'real token' contract this PR advertises is unreachable, and the first mutating endpoint 403s every legitimate caller with a token that is correct. Fix by adding `plug :fetch_session` to the `:api` pipeline ahead of `ApiCSRF` (and giving token holders a way to obtain one), or — if the JSON API is meant to be session-less — drop the session-bound scheme here instead of shipping a guard that can only deny.

skippy-bot marked this conversation as resolved

test/aprsme_web/plugs/api_csrf_test.exs

					
				@ -13,3 +13,3 @@

				      |> ApiCSRF.call([])

				    refute conn.halted

				    assert conn.halted

🟡 Warning — Accept path now has no real coverage — a reject-everything plug stays green

This PR converts the only test that exercised acceptance (refute conn.halted) into a 403 assertion, and the remaining "allows JSON request with valid CSRF token" test asserts the tautology conn.halted in [true, false] — so the suite passes even if the plug rejects 100% of valid requests (which is what the :api pipeline does; see the other finding). That test can't pass as written anyway: it puts get_csrf_token/0 into the session, but valid_state_and_csrf_token?/2 requires the 24-byte state, and the masked client token is 56 bytes, so the with falls through to false. Pair them correctly:

token = Plug.CSRFProtection.get_csrf_token()
state = Plug.CSRFProtection.dump_state()

conn
|> Map.put(:method, "POST")
|> init_test_session(%{"_csrf_token" => state})
|> put_req_header("x-csrf-token", token)
|> ApiCSRF.call([])

refute conn.halted

and assert refute conn.halted.

**🟡 Warning — Accept path now has no real coverage — a reject-everything plug stays green** This PR converts the only test that exercised acceptance (`refute conn.halted`) into a 403 assertion, and the remaining "allows JSON request with valid CSRF token" test asserts the tautology `conn.halted in [true, false]` — so the suite passes even if the plug rejects 100% of valid requests (which is what the `:api` pipeline does; see the other finding). That test can't pass as written anyway: it puts `get_csrf_token/0` into the session, but `valid_state_and_csrf_token?/2` requires the 24-byte state, and the masked client token is 56 bytes, so the `with` falls through to `false`. Pair them correctly: ```elixir token = Plug.CSRFProtection.get_csrf_token() state = Plug.CSRFProtection.dump_state() conn |> Map.put(:method, "POST") |> init_test_session(%{"_csrf_token" => state}) |> put_req_header("x-csrf-token", token) |> ApiCSRF.call([]) refute conn.halted ``` and assert `refute conn.halted`.

skippy-bot marked this conversation as resolved

graham added 1 commit

2026-09-15 12:06:57 -05:00

fix: fetch session in api pipeline, pair csrf test correctly, update docs

Elixir CI / Build and test (pull_request) Successful in 57s

Details

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

Details

Elixir CI / Dialyzer (pull_request) Successful in 2m43s

Details

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

Details

5abd3d292c

skippy-bot commented

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

First-time contributor

Resolved all 3 earlier findings at 5abd3d29:

session: the :api pipeline now runs plug :fetch_session ahead of ApiCSRF (router.ex:50), so the token comparison no longer raises and a correct token can pass.
test: api_csrf_test.exs:110 pairs dump_state/0 with get_csrf_token/0 and asserts refute conn.halted, which matches Plug 1.20.3's 24-byte state / 56-byte masked-token split.
docs: docs/web/api-rest.md now states the token-only contract and the 403 response.

Nothing new in 72401e05..5abd3d29.

Resolved all 3 earlier findings at `5abd3d29`: - session: the `:api` pipeline now runs `plug :fetch_session` ahead of `ApiCSRF` (router.ex:50), so the token comparison no longer raises and a correct token can pass. - test: `api_csrf_test.exs:110` pairs `dump_state/0` with `get_csrf_token/0` and asserts `refute conn.halted`, which matches Plug 1.20.3's 24-byte state / 56-byte masked-token split. - docs: `docs/web/api-rest.md` now states the token-only contract and the 403 response. Nothing new in `72401e05..5abd3d29`.

graham merged commit 40a5b0a15c into main

2026-09-15 16:33:14 -05:00

graham deleted branch fix/w15-api-csrf-contract

2026-09-15 16:33:14 -05:00

graham referenced this pull request from a commit

2026-09-15 16:33:14 -05:00

fix: strengthen API CSRF protection contract (#32)

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project