diff --git a/lib/pleroma/web/o_auth/o_auth_controller.ex b/lib/pleroma/web/o_auth/o_auth_controller.ex index 29cbd6aa6..29aa8c10e 100644 --- a/lib/pleroma/web/o_auth/o_auth_controller.ex +++ b/lib/pleroma/web/o_auth/o_auth_controller.ex @@ -39,6 +39,7 @@ defmodule Pleroma.Web.OAuth.OAuthController do action_fallback(Pleroma.Web.OAuth.FallbackController) @oob_token_redirect_uri "urn:ietf:wg:oauth:2.0:oob" + @state_cookie_name "akkoma_oauth_state" # Note: this definition is only called from error-handling methods with `conn.params` as 2nd arg def authorize(%Plug.Conn{} = conn, %{"authorization" => _} = params) do @@ -445,7 +446,7 @@ def prepare_request(%Plug.Conn{} = conn, %{ # Handing the request to Ueberauth conn - |> put_resp_cookie("akkoma_oauth_state", state) + |> put_resp_cookie(@state_cookie_name, state) |> redirect(to: ~p"/oauth/#{provider}") end @@ -469,12 +470,18 @@ def callback(%Plug.Conn{assigns: %{ueberauth_failure: failure}} = conn, params) messages = for e <- Map.get(failure, :errors, []), do: e.message message = Enum.join(messages, "; ") - conn - |> put_flash( - :error, - dgettext("errors", "Failed to authenticate: %{message}.", message: message) - ) - |> redirect(external: redirect_uri(conn, params["redirect_uri"])) + error_message = dgettext("errors", "Failed to authenticate: %{message}.", message: message) + + if params["redirect_uri"] do + conn + |> put_flash( + :error, + error_message + ) + |> redirect(external: redirect_uri(conn, params["redirect_uri"])) + else + send_resp(conn, :bad_request, error_message) + end end def callback(%Plug.Conn{} = conn, params) do @@ -510,7 +517,7 @@ def callback(%Plug.Conn{} = conn, params) do defp callback_params(%Plug.Conn{} = conn, params) do fetch_cookies(conn) - Map.merge(params, Jason.decode!(Map.get(conn.req_cookies, "akkoma_oauth_state", "{}"))) + Map.merge(params, Jason.decode!(Map.get(conn.req_cookies, @state_cookie_name, "{}"))) end def registration_details(%Plug.Conn{} = conn, %{"authorization" => auth_attrs}) do diff --git a/test/pleroma/web/o_auth/o_auth_controller_test.exs b/test/pleroma/web/o_auth/o_auth_controller_test.exs index e0ba339db..d0703f44c 100644 --- a/test/pleroma/web/o_auth/o_auth_controller_test.exs +++ b/test/pleroma/web/o_auth/o_auth_controller_test.exs @@ -81,9 +81,7 @@ test "GET /oauth/prepare_request encodes parameters as `state` and redirects", % assert html_response(conn, 302) - redirect_query = URI.parse(redirected_to(conn)).query - assert %{"state" => state_param} = URI.decode_query(redirect_query) - assert {:ok, state_components} = Jason.decode(state_param) + assert {:ok, state_components} = Jason.decode(conn.resp_cookies["akkoma_oauth_state"].value) expected_client_id = app.client_id expected_redirect_uri = app.redirect_uris @@ -97,7 +95,7 @@ test "GET /oauth/prepare_request encodes parameters as `state` and redirects", % end test "with user-bound registration, GET /oauth//callback redirects to `redirect_uri` with `code`", - %{app: app, conn: conn} do + %{app: app, conn: _} do registration = insert(:registration) redirect_uri = OAuthController.default_redirect_uri(app) @@ -109,15 +107,17 @@ test "with user-bound registration, GET /oauth//callback redirects to } conn = - conn + build_conn() + |> put_req_cookie("akkoma_oauth_state", Jason.encode!(state_params)) + |> Plug.Session.call(Plug.Session.init(@session_opts)) + |> fetch_session() |> assign(:ueberauth_auth, %{provider: registration.provider, uid: registration.uid}) |> get( "/oauth/twitter/callback", %{ "oauth_token" => "G-5a3AAAAAAAwMH9AAABaektfSM", "oauth_verifier" => "QZl8vUqNvXMTKpdmUnGejJxuHG75WWWs", - "provider" => "twitter", - "state" => Jason.encode!(state_params) + "provider" => "twitter" } ) @@ -162,15 +162,42 @@ test "with user-unbound registration, GET /oauth//callback renders reg test "on authentication error, GET /oauth//callback redirects to `redirect_uri`", %{ app: app, - conn: conn + conn: _ } do state_params = %{ "scope" => Enum.join(app.scopes, " "), "client_id" => app.client_id, - "redirect_uri" => OAuthController.default_redirect_uri(app), - "state" => "" + "redirect_uri" => OAuthController.default_redirect_uri(app) } + conn = + build_conn() + |> put_req_cookie("akkoma_oauth_state", Jason.encode!(state_params)) + |> Plug.Session.call(Plug.Session.init(@session_opts)) + |> fetch_session() + |> assign(:ueberauth_failure, %{errors: [%{message: "(error description)"}]}) + |> get( + "/oauth/twitter/callback", + %{ + "oauth_token" => "G-5a3AAAAAAAwMH9AAABaektfSM", + "oauth_verifier" => "QZl8vUqNvXMTKpdmUnGejJxuHG75WWWs", + "provider" => "twitter", + "state" => "" + } + ) + + assert html_response(conn, 302) + assert redirected_to(conn) == app.redirect_uris + + assert Phoenix.Flash.get(conn.assigns.flash, :error) == + "Failed to authenticate: (error description)." + end + + test "on authentication error with no prior state, GET /oauth//callback returns 400", + %{ + app: _, + conn: conn + } do conn = conn |> assign(:ueberauth_failure, %{errors: [%{message: "(error description)"}]}) @@ -180,15 +207,11 @@ test "on authentication error, GET /oauth//callback redirects to `redi "oauth_token" => "G-5a3AAAAAAAwMH9AAABaektfSM", "oauth_verifier" => "QZl8vUqNvXMTKpdmUnGejJxuHG75WWWs", "provider" => "twitter", - "state" => Jason.encode!(state_params) + "state" => "" } ) - assert html_response(conn, 302) - assert redirected_to(conn) == app.redirect_uris - - assert Phoenix.Flash.get(conn.assigns.flash, :error) == - "Failed to authenticate: (error description)." + assert response(conn, 400) end test "GET /oauth/registration_details renders registration details form", %{