Describe the bug
GET /authenticate/callback raises Phoenix.ActionClauseError whenever the
request arrives without the params that one of the existing
OidcController.new/2 clauses expects. The three clauses match
%{"provider" => _, "code" => _}, %{"state" => _, "code" => _}, and
%{"error" => _, "state" => _} — so a request with no query string (or
any combination of unexpected params) reaches the action, no clause matches,
Phoenix raises ActionClauseError, and the request returns a 500 with an
unhandled stack.
In practice the bare URL gets hit fairly often by scanners and crawlers
(stale UAs, no referer, no cookies, no repeat IPs), which means a steady
trickle of unhandled exceptions in the logs that aren't user-affecting but
also shouldn't be 500s. The controller should never raise on a malformed
callback regardless of who's calling.
Version number Present on current main. The clauses haven't been
touched in a long time so it's been present across recent releases.
I have reproduced this locally on main:
To Reproduce
- Send
GET /authenticate/callback with no query parameters
(curl http://localhost:4000/authenticate/callback is enough).
- Observe 500 in the response and an
ActionClauseError pointing at
lib/lightning_web/controllers/oidc_controller.ex:32.
Expected behaviour
Never 500. Two reasonable shapes for the fallback, and they could be
combined:
- Non-browser / no-cookie / no-meaningful-headers request → plain
400 Bad Request. Most of the traffic looks like this and a clean 400 is
the right answer.
- Browser-shaped request (User-Agent + cookies + Lightning-domain
referer) → redirect to the log-in page with a flash explaining the
callback couldn't be completed, similar to the "could not find user
account" path already in the first clause of new/2.
That said, it's hard to imagine any IdP redirecting back to the callback
URL with literally no params, state, or cookies — so a plain 400 across
the board may well be enough, and is simpler. Open to either.
Additional context
- Route is
get "/authenticate/callback", OidcController, :new — the OAuth
credential-creation callback (used when connecting external services via
OAuth). The user-login OIDC callback is at
/authenticate/:provider/callback, which is unaffected because the
router enforces :provider is present.
- A catch-all
def new(conn, _params) clause handling the "no expected
fields" case would resolve this; the discussion point is the exact
response (400 vs. browser-aware redirect-with-flash).
Describe the bug
GET /authenticate/callbackraisesPhoenix.ActionClauseErrorwhenever therequest arrives without the params that one of the existing
OidcController.new/2clauses expects. The three clauses match%{"provider" => _, "code" => _},%{"state" => _, "code" => _}, and%{"error" => _, "state" => _}— so a request with no query string (orany combination of unexpected params) reaches the action, no clause matches,
Phoenix raises ActionClauseError, and the request returns a 500 with an
unhandled stack.
In practice the bare URL gets hit fairly often by scanners and crawlers
(stale UAs, no referer, no cookies, no repeat IPs), which means a steady
trickle of unhandled exceptions in the logs that aren't user-affecting but
also shouldn't be 500s. The controller should never raise on a malformed
callback regardless of who's calling.
Version number Present on current
main. The clauses haven't beentouched in a long time so it's been present across recent releases.
I have reproduced this locally on main:
To Reproduce
GET /authenticate/callbackwith no query parameters(
curl http://localhost:4000/authenticate/callbackis enough).ActionClauseErrorpointing atlib/lightning_web/controllers/oidc_controller.ex:32.Expected behaviour
Never 500. Two reasonable shapes for the fallback, and they could be
combined:
400 Bad Request. Most of the traffic looks like this and a clean 400 isthe right answer.
referer) → redirect to the log-in page with a flash explaining the
callback couldn't be completed, similar to the "could not find user
account" path already in the first clause of
new/2.That said, it's hard to imagine any IdP redirecting back to the callback
URL with literally no params, state, or cookies — so a plain
400acrossthe board may well be enough, and is simpler. Open to either.
Additional context
get "/authenticate/callback", OidcController, :new— the OAuthcredential-creation callback (used when connecting external services via
OAuth). The user-login OIDC callback is at
/authenticate/:provider/callback, which is unaffected because therouter enforces
:provideris present.def new(conn, _params)clause handling the "no expectedfields" case would resolve this; the discussion point is the exact
response (400 vs. browser-aware redirect-with-flash).