Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 1 addition & 15 deletions lib/plausible/stats/dashboard_query_serializer.ex
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
77 changes: 68 additions & 9 deletions lib/plausible_web/controllers/stats_controller.ex
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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? =
Expand Down
2 changes: 1 addition & 1 deletion lib/plausible_web/router.ex
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
17 changes: 17 additions & 0 deletions lib/plausible_web/uri_encoding.ex
Original file line number Diff line number Diff line change
@@ -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
86 changes: 51 additions & 35 deletions test/plausible_web/controllers/stats_controller_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -1500,7 +1500,7 @@
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
Expand All @@ -1520,7 +1520,7 @@
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"
Expand Down Expand Up @@ -1550,13 +1550,13 @@
)

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"
end

test "preserves query parameters during password authentication", %{conn: conn} do

Check failure on line 1559 in test/plausible_web/controllers/stats_controller_test.exs

View workflow job for this annotation

GitHub Actions / Build and test (ce_test, postgres:16)

test POST /share/:slug/authenticate preserves query parameters during password authentication (PlausibleWeb.StatsControllerTest)
site = new_site()

link =
Expand All @@ -1567,68 +1567,84 @@
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!"}
)

html = html_response(conn, 200)
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

Expand Down
16 changes: 16 additions & 0 deletions test/plausible_web/uri_encoding_test.exs
Original file line number Diff line number Diff line change
@@ -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
Loading