Fix Authelia OIDC login by implementing secure state and nonce

lib/claper_web/controllers/user_oidc_auth.ex

@@ -24,56 +24,85 @@ defmodule ClaperWeb.UserOidcAuth do
     pkce_verifier = generate_pkce_verifier()
     conn = put_session(conn, :pkce_verifier, pkce_verifier)
 
+    # Generate a secure random state (at least 8 chars)
+    state = :crypto.strong_rand_bytes(16) |> Base.url_encode64(padding: false)
+    conn = put_session(conn, :oidc_state, state)
+
+    # Generate nonce for additional security
+    nonce = :crypto.strong_rand_bytes(32) |> Base.url_encode64(padding: false)
+    conn = put_session(conn, :oidc_nonce, nonce)
+
     {:ok, redirect_uri} =
       Oidcc.create_redirect_url(
         Claper.OidcProviderConfig,
         client_id(),
         client_secret(),
-        opts(pkce_verifier)
+        opts(pkce_verifier, state, nonce)
       )
 
     uri = Enum.join(redirect_uri, "")
 
     redirect(conn, external: uri)
   end
 
-  def callback(conn, %{"code" => code} = _params) do
-    # Get PKCE verifier from session
+  def callback(conn, %{"code" => code, "state" => state_param} = _params) do
+    # Get all security parameters from session
     pkce_verifier = get_session(conn, :pkce_verifier)
+    session_state = get_session(conn, :oidc_state)
+    session_nonce = get_session(conn, :oidc_nonce)
 
-    with {:ok,
-          %Oidcc.Token{
-            id: %Oidcc.Token.Id{token: id_token, claims: claims},
-            access: %Oidcc.Token.Access{token: access_token},
-            refresh: refresh_token
-          }} <-
-           Oidcc.retrieve_token(
-             code,
-             Claper.OidcProviderConfig,
-             client_id(),
-             client_secret(),
-             opts(pkce_verifier)
-           ),
-         {:ok, oidc_user} <- validate_user(id_token, access_token, refresh_token, claims) do
-      conn
-      # Clean up the verifier
-      |> delete_session(:pkce_verifier)
-      |> UserAuth.log_in_user(oidc_user.user)
-    else
-      {:error, reason} ->
+    cond do
+      is_nil(session_state) or is_nil(state_param) or session_state != state_param ->
         conn
         # Clean up the verifier even on error
         |> delete_session(:pkce_verifier)
+        |> delete_session(:oidc_state)
+        |> delete_session(:oidc_nonce)
         |> put_status(:unauthorized)
         |> put_view(ClaperWeb.ErrorView)
-        |> render("csrf_error.html", %{error: "Authentication failed: #{inspect(reason)}"})
+        |> render("csrf_error.html", %{
+          error: "Authentication failed: invalid or missing state parameter"
+        })
+
+      true ->
+        with {:ok,
+              %Oidcc.Token{
+                id: %Oidcc.Token.Id{token: id_token, claims: claims},
+                access: %Oidcc.Token.Access{token: access_token},
+                refresh: refresh_token
+              }} <-
+               Oidcc.retrieve_token(
+                 code,
+                 Claper.OidcProviderConfig,
+                 client_id(),
+                 client_secret(),
+                 opts(pkce_verifier, session_state, session_nonce)
+               ),
+             {:ok, oidc_user} <- validate_user(id_token, access_token, refresh_token, claims) do
+          conn
+          |> delete_session(:pkce_verifier)
+          |> delete_session(:oidc_state)
+          |> delete_session(:oidc_nonce)
+          |> UserAuth.log_in_user(oidc_user.user)
+        else
+          {:error, reason} ->
+            conn
+            |> delete_session(:pkce_verifier)
+            |> delete_session(:oidc_state)
+            |> delete_session(:oidc_nonce)
+            |> put_status(:unauthorized)
+            |> put_view(ClaperWeb.ErrorView)
+            |> render("csrf_error.html", %{error: "Authentication failed: #{inspect(reason)}"})
+        end
     end
   end
 
   def callback(conn, %{"error" => error} = _params) do
     conn
     # Clean up the verifier even on error
     |> delete_session(:pkce_verifier)
+    |> delete_session(:oidc_state)
+    |> delete_session(:oidc_nonce)
     |> put_status(:unauthorized)
     |> put_view(ClaperWeb.ErrorView)
     |> render("csrf_error.html", %{error: "Authentication failed: #{error}"})
@@ -103,7 +132,7 @@ defmodule ClaperWeb.UserOidcAuth do
     Application.get_env(:claper, ClaperWeb.Endpoint)[:base_url]
   end
 
-  defp opts(pkce_verifier) do
+  defp opts(pkce_verifier, state \\ nil, nonce \\ nil) do
     url = base_url()
 
     base_opts = %{
@@ -113,6 +142,20 @@ defmodule ClaperWeb.UserOidcAuth do
       require_pkce: true
     }
 
+    base_opts =
+      if state do
+        Map.put(base_opts, :state, state)
+      else
+        base_opts
+      end
+
+    base_opts =
+      if nonce do
+        Map.put(base_opts, :nonce, nonce)
+      else
+        base_opts
+      end
+
     if pkce_verifier do
       Map.merge(base_opts, %{
         pkce_verifier: pkce_verifier,

test/claper_web/controllers/user_oidc_auth_test.exs

@@ -0,0 +1,60 @@
+defmodule ClaperWeb.UserOidcAuthTest do
+  use ClaperWeb.ConnCase, async: true
+
+  describe "new/2" do
+    test "redirects to OIDC provider with state and nonce in session", %{conn: conn} do
+      conn = get(conn, ~p"/users/oidc")
+
+      assert redirected_to(conn) =~ "/authorization" # Assuming redirection URL contains /authorization or similar, checking generated URL structure might be hard without knowing provider config fully.
+      # Actually, since we can't easily check the exact URL without mocking provider config, we can check the session.
+
+      assert get_session(conn, :oidc_state)
+      assert get_session(conn, :oidc_nonce)
+      assert get_session(conn, :pkce_verifier)
+
+      state = get_session(conn, :oidc_state)
+      nonce = get_session(conn, :oidc_nonce)
+
+      # Check lengths (base64 encoded 16 bytes is approx 22 chars, 32 bytes approx 43 chars)
+      assert String.length(state) >= 20
+      assert String.length(nonce) >= 40
+    end
+  end
+
+  describe "callback/2" do
+    test "fails when state is missing in params", %{conn: conn} do
+      conn = 
+        conn
+        |> put_session(:oidc_state, "valid_state")
+        |> get(~p"/users/oidc/callback", %{code: "some_code"})
+
+      assert html_response(conn, 401) =~ "Authentication failed: invalid or missing state parameter"
+      refute get_session(conn, :oidc_state)
+    end
+
+    test "fails when state is missing in session", %{conn: conn} do
+      conn = 
+        conn
+        |> get(~p"/users/oidc/callback", %{code: "some_code", state: "some_state"})
+
+      assert html_response(conn, 401) =~ "Authentication failed: invalid or missing state parameter"
+    end
+
+    test "fails when state mismatch", %{conn: conn} do
+      conn = 
+        conn
+        |> put_session(:oidc_state, "valid_state")
+        |> get(~p"/users/oidc/callback", %{code: "some_code", state: "invalid_state"})
+
+      assert html_response(conn, 401) =~ "Authentication failed: invalid or missing state parameter"
+      refute get_session(conn, :oidc_state)
+    end
+
+    test "fails when error param is present", %{conn: conn} do
+      conn = get(conn, ~p"/users/oidc/callback", %{error: "access_denied"})
+
+      assert html_response(conn, 401) =~ "Authentication failed: access_denied"
+      refute get_session(conn, :oidc_state)
+    end
+  end
+end