diff --git a/lib/plausible/stats/dashboard_query_serializer.ex b/lib/plausible/stats/dashboard_query_serializer.ex index f04bf60c24a0..55d134c7b237 100644 --- a/lib/plausible/stats/dashboard_query_serializer.ex +++ b/lib/plausible/stats/dashboard_query_serializer.ex @@ -51,7 +51,7 @@ defmodule Plausible.Stats.DashboardQuerySerializer do defp get_serialized_fields({:filters, [_ | _] = filters}) do filters |> Enum.map(fn [operator, dimension, clauses] -> - clauses = Enum.map_join(clauses, ",", &uri_encode_permissive/1) + clauses = Enum.map_join(clauses, ",", &PlausibleWeb.URIEncoding.uri_encode_permissive/1) dimension = String.split(dimension, ":", parts: 2) |> List.last() {"f", "#{operator},#{dimension},#{clauses}"} end) @@ -68,18 +68,4 @@ defmodule Plausible.Stats.DashboardQuerySerializer do defp get_serialized_fields(_) do [] end - - # These characters are not URL encoded to have more readable URLs. - # Browsers seem to handle this just fine. `?f=is,page,/my/page/:some_param` - # vs `?f=is,page,%2Fmy%2Fpage%2F%3Asome_param` - @do_not_url_encode [":", "/"] - @do_not_url_encode_map Enum.into(@do_not_url_encode, %{}, fn char -> - {URI.encode_www_form(char), char} - end) - - defp uri_encode_permissive(input) do - input - |> URI.encode_www_form() - |> String.replace(Map.keys(@do_not_url_encode_map), &@do_not_url_encode_map[&1]) - end end diff --git a/lib/plausible_web/controllers/stats_controller.ex b/lib/plausible_web/controllers/stats_controller.ex index 94f0c9a2e4c8..82b971ccad71 100644 --- a/lib/plausible_web/controllers/stats_controller.ex +++ b/lib/plausible_web/controllers/stats_controller.ex @@ -282,7 +282,9 @@ defmodule PlausibleWeb.StatsController do ) if shared_link do - new_link_format = Routes.stats_path(conn, :shared_link, shared_link.site.domain, auth: slug) + new_link_format = + Routes.stats_path(conn, :shared_link, shared_link.site.domain, [], auth: slug) + redirect(conn, to: new_link_format) else render_error(conn, 404) @@ -306,6 +308,15 @@ defmodule PlausibleWeb.StatsController do defp render_password_protected_shared_link(conn, shared_link) do conn = Plug.Conn.fetch_cookies(conn) + star_path = conn.path_params["path"] + + star_path_fragment = serialize_star_path_as_query_string_fragment(star_path) + + query_string = + [conn.query_string, star_path_fragment] + |> Enum.filter(fn v -> is_binary(v) and String.length(v) > 0 end) + |> Enum.join("&") + case validate_shared_link_password(conn, shared_link) do {:ok, shared_link} -> render_shared_link(conn, shared_link) @@ -314,7 +325,7 @@ defmodule PlausibleWeb.StatsController do conn |> render("shared_link_password.html", link: shared_link, - query_string: conn.query_string, + query_string: query_string, dogfood_page_path: "/share/:dashboard" ) end @@ -349,12 +360,25 @@ defmodule PlausibleWeb.StatsController do if Plausible.Auth.Password.match?(password, shared_link.password_hash) do token = Plausible.Auth.Token.sign_shared_link(slug) + star_path = parse_star_path(conn) + + query_string_fragment = + get_rest_of_query_string(conn) + |> omit_from_query_string("return_to") + |> omit_from_query_string("auth") + conn |> put_resp_cookie(shared_link_cookie_name(slug), token) |> redirect( to: - Routes.stats_path(conn, :shared_link, shared_link.site.domain, auth: slug) <> - qs_appendix(conn) + Routes.stats_path( + conn, + :shared_link, + shared_link.site.domain, + star_path, + auth: slug + ) <> + query_string_fragment ) else conn @@ -370,12 +394,47 @@ defmodule PlausibleWeb.StatsController do end end - def qs_appendix(conn) - when is_nil(conn.query_string) or - (is_binary(conn.query_string) and byte_size(conn.query_string)) == 0, - do: "" + defp serialize_star_path_as_query_string_fragment(star_path) do + if length(star_path) > 0 do + # make the path start with a / + # to be able to reject values that don't start with a / + serialized_value = + "/#{Enum.join(star_path, "/")}" + |> PlausibleWeb.URIEncoding.uri_encode_permissive() - def qs_appendix(conn), do: "&#{conn.query_string}" + "return_to=#{serialized_value}" + else + nil + end + end + + defp parse_star_path(conn) do + return_to_value = conn.query_params["return_to"] + + if not is_nil(return_to_value) and String.starts_with?(return_to_value, "/") do + # get rid of the / character that we added + "/" <> trimmed_return_to_value = return_to_value + String.split(trimmed_return_to_value, "/") + else + [] + end + end + + defp get_rest_of_query_string(conn) + when is_nil(conn.query_string) or + (is_binary(conn.query_string) and byte_size(conn.query_string)) == 0, + do: "" + + defp get_rest_of_query_string(conn), do: "&#{conn.query_string}" + + defp omit_from_query_string(query_string, key) do + query_string + |> String.split("&") + |> Enum.reject(fn key_and_value -> + key_and_value == key || String.starts_with?(key_and_value, "#{key}=") + end) + |> Enum.join("&") + end defp render_shared_link(conn, shared_link) do shared_links_feature_access? = diff --git a/lib/plausible_web/router.ex b/lib/plausible_web/router.ex index 420f03ae7b7f..e9f5e8307b02 100644 --- a/lib/plausible_web/router.ex +++ b/lib/plausible_web/router.ex @@ -468,7 +468,7 @@ defmodule PlausibleWeb.Router do scope "/", PlausibleWeb do pipe_through [:shared_link] - get "/share/:domain", StatsController, :shared_link + get "/share/:domain/*path", StatsController, :shared_link post "/share/:slug/authenticate", StatsController, :authenticate_shared_link end diff --git a/lib/plausible_web/uri_encoding.ex b/lib/plausible_web/uri_encoding.ex new file mode 100644 index 000000000000..4a0c2ac3834e --- /dev/null +++ b/lib/plausible_web/uri_encoding.ex @@ -0,0 +1,17 @@ +defmodule PlausibleWeb.URIEncoding do + @moduledoc false + + # These characters are not URL encoded to have more readable URLs. + # Browsers seem to handle this just fine. `?f=is,page,/my/page/:some_param` + # vs `?f=is,page,%2Fmy%2Fpage%2F%3Asome_param` + @do_not_url_encode [":", "/"] + @do_not_url_encode_map Enum.into(@do_not_url_encode, %{}, fn char -> + {URI.encode_www_form(char), char} + end) + + def uri_encode_permissive(input) do + input + |> URI.encode_www_form() + |> String.replace(Map.keys(@do_not_url_encode_map), &@do_not_url_encode_map[&1]) + end +end diff --git a/test/plausible_web/controllers/stats_controller_test.exs b/test/plausible_web/controllers/stats_controller_test.exs index eb9fe48401a4..a3e1c51570a4 100644 --- a/test/plausible_web/controllers/stats_controller_test.exs +++ b/test/plausible_web/controllers/stats_controller_test.exs @@ -1500,7 +1500,7 @@ defmodule PlausibleWeb.StatsControllerTest do site_link = insert(:shared_link, site: site, inserted_at: ~N[2021-12-31 00:00:00]) conn = get(conn, "/share/#{site_link.slug}") - assert redirected_to(conn, 302) == "/share/#{site.domain}?auth=#{site_link.slug}" + assert redirected_to(conn, 302) == "/share/#{site.domain}/?auth=#{site_link.slug}" end test "it does nothing for newer links", %{conn: conn} do @@ -1520,7 +1520,7 @@ defmodule PlausibleWeb.StatsControllerTest do insert(:shared_link, site: site, password_hash: Plausible.Auth.Password.hash("password")) conn = post(conn, "/share/#{link.slug}/authenticate", %{password: "password"}) - assert redirected_to(conn, 302) == "/share/#{site.domain}?auth=#{link.slug}" + assert redirected_to(conn, 302) == "/share/#{site.domain}/?auth=#{link.slug}" conn = get(conn, "/share/#{site.domain}?auth=#{link.slug}") assert html_response(conn, 200) =~ "stats-react-container" @@ -1550,7 +1550,7 @@ defmodule PlausibleWeb.StatsControllerTest do ) conn = post(conn, "/share/#{link.slug}/authenticate", %{password: "password"}) - assert redirected_to(conn, 302) == "/share/#{site.domain}?auth=#{link.slug}" + assert redirected_to(conn, 302) == "/share/#{site.domain}/?auth=#{link.slug}" conn = get(conn, "/share/#{site2.domain}?auth=#{link2.slug}") assert html_response(conn, 200) =~ "Enter password" @@ -1567,33 +1567,21 @@ defmodule PlausibleWeb.StatsControllerTest do conn = get( conn, - "/share/#{site.domain}?auth=#{link.slug}&#{filters}" + "/share/#{URI.encode_www_form(site.domain)}?auth=#{link.slug}&#{filters}" ) assert html_response(conn, 200) =~ "Enter password" html = html_response(conn, 200) - assert html =~ ~s(action="/share/#{link.slug}/authenticate?) - assert html =~ "f=is,browser,Firefox" - assert html =~ "f=is,country,EE" - assert html =~ "l=EE,Estonia" + expected_action_string = + "/share/#{URI.encode_www_form(link.slug)}/authenticate?auth=#{link.slug}&#{filters}" - conn = - post( - conn, - "/share/#{link.slug}/authenticate?#{filters}", - %{password: "password"} - ) - - expected_redirect = - "/share/#{URI.encode_www_form(site.domain)}?auth=#{link.slug}&#{filters}" - - assert redirected_to(conn, 302) == expected_redirect + assert text_of_attr(html, "form", "action") == expected_action_string conn = post( conn, - "/share/#{link.slug}/authenticate?#{filters}", + expected_action_string, %{password: "WRONG!"} ) @@ -1601,34 +1589,62 @@ defmodule PlausibleWeb.StatsControllerTest do assert html =~ "Enter password" assert html =~ "Incorrect password" - assert text_of_attr(html, "form", "action") =~ "?#{filters}" + assert text_of_attr(html, "form", "action") == expected_action_string conn = post( conn, - "/share/#{link.slug}/authenticate?#{filters}", + expected_action_string, %{password: "password"} ) - redirected_url = redirected_to(conn, 302) - assert redirected_url =~ filters - - conn = - post( - conn, - "/share/#{link.slug}/authenticate?#{filters}", - %{password: "password"} - ) + expected_redirect = + "/share/#{URI.encode_www_form(site.domain)}?auth=#{link.slug}&#{filters}" - redirect_path = redirected_to(conn, 302) + assert redirected_to(conn, 302) == expected_redirect - conn = get(conn, redirect_path) + conn = get(conn, expected_redirect) assert html_response(conn, 200) =~ "stats-react-container" - assert redirect_path =~ filters - assert redirect_path =~ "auth=#{link.slug}" end end + test "handles return_to during password authentication", %{conn: conn} do + site = new_site() + + link = + insert(:shared_link, site: site, password_hash: Plausible.Auth.Password.hash("password")) + + filters = "f=is,country,EE&l=EE,Estonia&f=is,browser,Firefox" + + deep_path = "/filter/source" + + conn = + get( + conn, + "/share/#{URI.encode_www_form(site.domain)}#{deep_path}?auth=#{link.slug}&#{filters}" + ) + + assert html_response(conn, 200) =~ "Enter password" + html = html_response(conn, 200) + + expected_action_string = + "/share/#{link.slug}/authenticate?auth=#{link.slug}&#{filters}&return_to=#{deep_path}" + + assert text_of_attr(html, "form", "action") == expected_action_string + + conn = + post( + conn, + expected_action_string, + %{password: "password"} + ) + + expected_redirect = + "/share/#{URI.encode_www_form(site.domain)}#{deep_path}?auth=#{link.slug}&#{filters}" + + assert redirected_to(conn, 302) == expected_redirect + end + describe "dogfood tracking" do @describetag :ee_only diff --git a/test/plausible_web/uri_encoding_test.exs b/test/plausible_web/uri_encoding_test.exs new file mode 100644 index 000000000000..c66cec63a98c --- /dev/null +++ b/test/plausible_web/uri_encoding_test.exs @@ -0,0 +1,16 @@ +defmodule PlausibleWeb.URIEncodingTest do + use ExUnit.Case, async: true + + for {value, expected} <- [ + {"/:dashboard/settings", "/:dashboard/settings"}, + {"&", "%26"}, + {"=", "%3D"}, + {",", "%2C"}, + # should be {"hello world", "hello%20world"}, is + {"hello world", "hello+world"} + ] do + test "permissively uri-encoding value #{inspect(value)} yields #{inspect(expected)}" do + assert PlausibleWeb.URIEncoding.uri_encode_permissive(unquote(value)) == unquote(expected) + end + end +end