From 82cd9e8579405d6c5b3151c318c4f87da3c9c76c Mon Sep 17 00:00:00 2001 From: Ismael Bejarano Date: Tue, 27 Aug 2024 16:15:44 -0300 Subject: [PATCH 1/5] Fix invalid link --- lib/ask_web/controllers/short_link_controller.ex | 6 ++++++ test/ask_web/controllers/short_link_controller_test.exs | 8 ++++++++ 2 files changed, 14 insertions(+) diff --git a/lib/ask_web/controllers/short_link_controller.ex b/lib/ask_web/controllers/short_link_controller.ex index cf41c2575..612381742 100644 --- a/lib/ask_web/controllers/short_link_controller.ex +++ b/lib/ask_web/controllers/short_link_controller.ex @@ -9,6 +9,12 @@ defmodule AskWeb.ShortLinkController do ShortLink |> Repo.get_by(hash: hash) + unless link do + conn + |> put_status(:not_found) + |> send_resp(:not_found, "") + end + conn = conn |> assign(:skip_auth, true) diff --git a/test/ask_web/controllers/short_link_controller_test.exs b/test/ask_web/controllers/short_link_controller_test.exs index e687c1cd9..ef8a154ec 100644 --- a/test/ask_web/controllers/short_link_controller_test.exs +++ b/test/ask_web/controllers/short_link_controller_test.exs @@ -14,6 +14,14 @@ defmodule AskWeb.ShortLinkControllerTest do {:ok, conn: conn, user: user} end + test "return 404 for an invalid link", %{ + conn: conn + } do + conn = get(conn, short_link_path(conn, :access, "invalid-link")) + + assert html_response(conn, 404) + end + test "render surveys if the link specifies that endpoint even if there is no current user", %{ conn: conn, user: user From 55e315b5b51d3f00c77c8fb32b52aedac445483d Mon Sep 17 00:00:00 2001 From: Ismael Bejarano Date: Wed, 28 Aug 2024 10:35:41 -0300 Subject: [PATCH 2/5] Fix CI failure --- .../controllers/short_link_controller.ex | 51 +++++++++---------- 1 file changed, 25 insertions(+), 26 deletions(-) diff --git a/lib/ask_web/controllers/short_link_controller.ex b/lib/ask_web/controllers/short_link_controller.ex index 612381742..f5170d735 100644 --- a/lib/ask_web/controllers/short_link_controller.ex +++ b/lib/ask_web/controllers/short_link_controller.ex @@ -9,35 +9,34 @@ defmodule AskWeb.ShortLinkController do ShortLink |> Repo.get_by(hash: hash) - unless link do + if link do + conn = + conn + |> assign(:skip_auth, true) + + {path, query_string} = + case :binary.split(link.target, "?", [:global]) do + [path, query_string] -> {path, query_string} + [path] -> {path, ""} + end + + conn = %{ + conn + | request_path: path, + path_info: split_path(path), + params: %{}, + path_params: %{}, + private: %{}, + req_headers: [], + query_string: query_string, + query_params: %Plug.Conn.Unfetched{aspect: :query_params} + } + + AskWeb.Endpoint.call(conn, []) + else conn - |> put_status(:not_found) |> send_resp(:not_found, "") end - - conn = - conn - |> assign(:skip_auth, true) - - {path, query_string} = - case :binary.split(link.target, "?", [:global]) do - [path, query_string] -> {path, query_string} - [path] -> {path, ""} - end - - conn = %{ - conn - | request_path: path, - path_info: split_path(path), - params: %{}, - path_params: %{}, - private: %{}, - req_headers: [], - query_string: query_string, - query_params: %Plug.Conn.Unfetched{aspect: :query_params} - } - - AskWeb.Endpoint.call(conn, []) end # Copied from Plug.Adapters.Cowboy.Conn From 1f0a112cbff64306dfe149a560fa105fdb97799f Mon Sep 17 00:00:00 2001 From: Ismael Bejarano Date: Wed, 28 Aug 2024 12:24:14 -0300 Subject: [PATCH 3/5] Fix assertion --- test/ask_web/controllers/short_link_controller_test.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/ask_web/controllers/short_link_controller_test.exs b/test/ask_web/controllers/short_link_controller_test.exs index ef8a154ec..9507692ec 100644 --- a/test/ask_web/controllers/short_link_controller_test.exs +++ b/test/ask_web/controllers/short_link_controller_test.exs @@ -19,7 +19,7 @@ defmodule AskWeb.ShortLinkControllerTest do } do conn = get(conn, short_link_path(conn, :access, "invalid-link")) - assert html_response(conn, 404) + assert response(conn, 404) end test "render surveys if the link specifies that endpoint even if there is no current user", %{ From 2e05c07c17502d0950fee1e54c86cfff6ea5dfd6 Mon Sep 17 00:00:00 2001 From: Ismael Bejarano Date: Wed, 28 Aug 2024 12:35:23 -0300 Subject: [PATCH 4/5] Update lib/ask_web/controllers/short_link_controller.ex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Matías García Isaía --- lib/ask_web/controllers/short_link_controller.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ask_web/controllers/short_link_controller.ex b/lib/ask_web/controllers/short_link_controller.ex index f5170d735..6cd73a9b3 100644 --- a/lib/ask_web/controllers/short_link_controller.ex +++ b/lib/ask_web/controllers/short_link_controller.ex @@ -35,7 +35,7 @@ defmodule AskWeb.ShortLinkController do AskWeb.Endpoint.call(conn, []) else conn - |> send_resp(:not_found, "") + |> send_resp(:not_found, "Link not found") end end From 592da7b817d0e20e5d99aee3f6a9e98e4494fe70 Mon Sep 17 00:00:00 2001 From: Ismael Bejarano Date: Wed, 28 Aug 2024 14:21:05 -0300 Subject: [PATCH 5/5] Apply fixes from PR --- .../controllers/short_link_controller.ex | 60 ++++++++++--------- .../short_link_controller_test.exs | 2 +- 2 files changed, 33 insertions(+), 29 deletions(-) diff --git a/lib/ask_web/controllers/short_link_controller.ex b/lib/ask_web/controllers/short_link_controller.ex index 6cd73a9b3..fafd88cc8 100644 --- a/lib/ask_web/controllers/short_link_controller.ex +++ b/lib/ask_web/controllers/short_link_controller.ex @@ -9,34 +9,7 @@ defmodule AskWeb.ShortLinkController do ShortLink |> Repo.get_by(hash: hash) - if link do - conn = - conn - |> assign(:skip_auth, true) - - {path, query_string} = - case :binary.split(link.target, "?", [:global]) do - [path, query_string] -> {path, query_string} - [path] -> {path, ""} - end - - conn = %{ - conn - | request_path: path, - path_info: split_path(path), - params: %{}, - path_params: %{}, - private: %{}, - req_headers: [], - query_string: query_string, - query_params: %Plug.Conn.Unfetched{aspect: :query_params} - } - - AskWeb.Endpoint.call(conn, []) - else - conn - |> send_resp(:not_found, "Link not found") - end + resolve_link(conn, link) end # Copied from Plug.Adapters.Cowboy.Conn @@ -44,4 +17,35 @@ defmodule AskWeb.ShortLinkController do segments = :binary.split(path, "/", [:global]) for segment <- segments, segment != "", do: segment end + + defp resolve_link(conn, nil) do + conn + |> send_resp(:not_found, "Link not found") + end + + defp resolve_link(conn, link) do + conn = + conn + |> assign(:skip_auth, true) + + {path, query_string} = + case :binary.split(link.target, "?", [:global]) do + [path, query_string] -> {path, query_string} + [path] -> {path, ""} + end + + conn = %{ + conn + | request_path: path, + path_info: split_path(path), + params: %{}, + path_params: %{}, + private: %{}, + req_headers: [], + query_string: query_string, + query_params: %Plug.Conn.Unfetched{aspect: :query_params} + } + + AskWeb.Endpoint.call(conn, []) + end end diff --git a/test/ask_web/controllers/short_link_controller_test.exs b/test/ask_web/controllers/short_link_controller_test.exs index 9507692ec..481733d7b 100644 --- a/test/ask_web/controllers/short_link_controller_test.exs +++ b/test/ask_web/controllers/short_link_controller_test.exs @@ -19,7 +19,7 @@ defmodule AskWeb.ShortLinkControllerTest do } do conn = get(conn, short_link_path(conn, :access, "invalid-link")) - assert response(conn, 404) + assert response(conn, 404) == "Link not found" end test "render surveys if the link specifies that endpoint even if there is no current user", %{