From f69cbf4755b974de0303731327180bb51ed244fc Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Thu, 20 Dec 2018 13:41:30 +0300 Subject: [PATCH] [#114] Added :user_id component to email confirmation path to improve the security. Added tests for `confirm_email` action. --- lib/pleroma/emails/user_email.ex | 1 + lib/pleroma/user.ex | 4 ---- lib/pleroma/web/router.ex | 7 ++++++- .../web/twitter_api/twitter_api_controller.ex | 6 ++++-- .../twitter_api_controller_test.exs | 18 +++++++++++++++--- 5 files changed, 26 insertions(+), 10 deletions(-) diff --git a/lib/pleroma/emails/user_email.ex b/lib/pleroma/emails/user_email.ex index 856816386..8f916f470 100644 --- a/lib/pleroma/emails/user_email.ex +++ b/lib/pleroma/emails/user_email.ex @@ -70,6 +70,7 @@ def account_confirmation_email(user) do Router.Helpers.confirm_email_url( Endpoint, :confirm_email, + user.id, to_string(user.info.confirmation_token) ) diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index 7e3a342f1..ad50cf558 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -396,10 +396,6 @@ def get_or_fetch_by_nickname(nickname) do end end - def get_by_confirmation_token(token) do - Repo.one(from(u in User, where: fragment("? ->> 'confirmation_token' = ?", u.info, ^token))) - end - def get_followers_query(%User{id: id, follower_address: follower_address}) do from( u in User, diff --git a/lib/pleroma/web/router.ex b/lib/pleroma/web/router.ex index ca069ab99..d1c3b34f6 100644 --- a/lib/pleroma/web/router.ex +++ b/lib/pleroma/web/router.ex @@ -282,7 +282,12 @@ defmodule Pleroma.Web.Router do post("/account/register", TwitterAPI.Controller, :register) post("/account/password_reset", TwitterAPI.Controller, :password_reset) - get("/account/confirm_email/:token", TwitterAPI.Controller, :confirm_email, as: :confirm_email) + get( + "/account/confirm_email/:user_id/:token", + TwitterAPI.Controller, + :confirm_email, + as: :confirm_email + ) post("/account/resend_confirmation_email", TwitterAPI.Controller, :resend_confirmation_email) diff --git a/lib/pleroma/web/twitter_api/twitter_api_controller.ex b/lib/pleroma/web/twitter_api/twitter_api_controller.ex index 46dc9b12c..c644681b0 100644 --- a/lib/pleroma/web/twitter_api/twitter_api_controller.ex +++ b/lib/pleroma/web/twitter_api/twitter_api_controller.ex @@ -382,9 +382,11 @@ def password_reset(conn, params) do end end - def confirm_email(conn, %{"token" => token}) do - with %User{} = user <- User.get_by_confirmation_token(token), + def confirm_email(conn, %{"user_id" => uid, "token" => token}) do + with %User{} = user <- Repo.get(User, uid), true <- user.local, + true <- user.info.confirmation_pending, + true <- user.info.confirmation_token == token, info_change <- User.Info.confirmation_changeset(user.info, :confirmed), changeset <- Changeset.change(user) |> Changeset.put_embed(:info, info_change), {:ok, _} <- User.update_and_set_cache(changeset) do diff --git a/test/web/twitter_api/twitter_api_controller_test.exs b/test/web/twitter_api/twitter_api_controller_test.exs index 53b390793..16422c35a 100644 --- a/test/web/twitter_api/twitter_api_controller_test.exs +++ b/test/web/twitter_api/twitter_api_controller_test.exs @@ -873,7 +873,7 @@ test "it returns 500 when user is not local", %{conn: conn, user: user} do end end - describe "GET /api/account/confirm_email/:token" do + describe "GET /api/account/confirm_email/:id/:token" do setup do user = insert(:user) info_change = User.Info.confirmation_changeset(user.info, :unconfirmed) @@ -890,19 +890,31 @@ test "it returns 500 when user is not local", %{conn: conn, user: user} do end test "it redirects to root url", %{conn: conn, user: user} do - conn = get(conn, "/api/account/confirm_email/#{user.info.confirmation_token}") + conn = get(conn, "/api/account/confirm_email/#{user.id}/#{user.info.confirmation_token}") assert 302 == conn.status end test "it confirms the user account", %{conn: conn, user: user} do - get(conn, "/api/account/confirm_email/#{user.info.confirmation_token}") + get(conn, "/api/account/confirm_email/#{user.id}/#{user.info.confirmation_token}") user = Repo.get(User, user.id) refute user.info.confirmation_pending refute user.info.confirmation_token end + + test "it returns 500 if user cannot be found by id", %{conn: conn, user: user} do + conn = get(conn, "/api/account/confirm_email/0/#{user.info.confirmation_token}") + + assert 500 == conn.status + end + + test "it returns 500 if token is invalid", %{conn: conn, user: user} do + conn = get(conn, "/api/account/confirm_email/#{user.id}/wrong_token") + + assert 500 == conn.status + end end describe "POST /api/account/resend_confirmation_email" do