From e16e357dd2050e1b5317f62f8f6e678497c2c538 Mon Sep 17 00:00:00 2001 From: Uku Taht Date: Tue, 20 Sep 2022 15:37:18 +0300 Subject: [PATCH] Fix shared link with bad auth (#2225) * Render 404 when shared link cannot be found * Add documentation for StatsController and shared link rendering * Refactor shared_link/2 for more clarity * Add changelog entry * Use mermaid graph for sequence diagram * Use more accurate return value in sequence diagram * Refactor Ecto query to be more idiomatic * Remove order dependence in test * Restore backwards compatibility for older shared links * Add changelog entry --- CHANGELOG.md | 2 + .../controllers/stats_controller.ex | 138 ++++++++++++++---- mix.exs | 14 +- .../breakdown_test.exs | 9 +- .../controllers/stats_controller_test.exs | 38 ++++- 5 files changed, 162 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e6077ebbf..36fa84ef1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,8 @@ All notable changes to this project will be documented in this file. - Fix a bug where city, region and country filters were filtering stats but not the location list - Fix a bug where regions were not being saved - Timezone offset labels now update with time changes +- Render 404 if shared link auth cannot be verified [plausible/analytics#2225](https://github.com/plausible/analytics/pull/2225) +- Restore compatibility with older format of shared links [plausible/analytics#2225](https://github.com/plausible/analytics/pull/2225) ### Changed - Cache the tracking script for 24 hours diff --git a/lib/plausible_web/controllers/stats_controller.ex b/lib/plausible_web/controllers/stats_controller.ex index 5c3ddbb48..3e0c5bec6 100644 --- a/lib/plausible_web/controllers/stats_controller.ex +++ b/lib/plausible_web/controllers/stats_controller.ex @@ -1,4 +1,44 @@ defmodule PlausibleWeb.StatsController do + @moduledoc """ + This controller is responsible for rendering stats dashboards. + + The stats dashboards are currently the only part of the app that uses client-side + rendering. Since the dashboards are heavily interactive, they are built with React + which is an appropraite choice for highly interactive browser UIs. + +
+ sequenceDiagram + Browser->>StatsController: GET /mydomain.com + StatsController-->>Browser: StatsView.render("stats.html") + Note left of Browser: ReactDom.render(Dashboard) + + Browser -) Api.StatsController: GET /api/stats/mydomain.com/top-stats + Api.StatsController --) Browser: {"top_stats": [...]} + Note left of Browser: TopStats.render() + + Browser -) Api.StatsController: GET /api/stats/mydomain.com/main-graph + Api.StatsController --) Browser: [{"plot": [...], "labels": [...]}, ...] + Note left of Browser: VisitorGraph.render() + + Browser -) Api.StatsController: GET /api/stats/mydomain.com/sources + Api.StatsController --) Browser: [{"name": "Google", "visitors": 292150}, ...] + Note left of Browser: Sources.render() + + Note over Browser,StatsController: And so on, for all reports in the viewport +
+ + This reasoning for this sequence is as follows: + 1. First paint is fast because it doesn't do any data aggregation yet - good UX + 2. The basic structure of the dashboard is rendered with spinners before reports are ready - good UX + 2. Rendering on the frontend allows for maximum interactivity. Re-rendering and re-fetching can be as granular as needed. + 3. Routing on the frontend allows the user to navigate the dashboard without reloading the page and losing context + 4. Rendering on the frontend allows caching results in the browser to reduce pressure on backends and storage + 3.1 No client-side caching has been implemented yet. This is still theoretical. See https://github.com/plausible/analytics/discussions/1278 + 3.2 This is a big potential opportunity, because analytics data is mostly immutable. Clients can cache all historical data. + 5. Since frontend rendering & navigation is harder to build and maintain than regular server-rendered HTML, we don't use SPA-style rendering anywhere else + .The only place currently where the benefits outweight the costs is the dashboard. + """ + use PlausibleWeb, :controller use Plausible.Repo alias PlausibleWeb.Api @@ -105,40 +145,48 @@ defmodule PlausibleWeb.StatsController do |> send_resp(200, zip_content) end - def shared_link(conn, %{"domain" => domain, "auth" => auth}) do - shared_link = - Repo.get_by(Plausible.Site.SharedLink, slug: auth) - |> Repo.preload(:site) + @doc """ + Authorizes and renders a shared link: + 1. Shared link with no password protection: needs to just make sure the shared link entry is still + in our database. This check makes sure shared link access can be revoked by the site admins. If the + shared link exists, render it directly. - if shared_link && shared_link.site.domain == domain do - if shared_link.password_hash do - with conn <- Plug.Conn.fetch_cookies(conn), - {:ok, token} <- Map.fetch(conn.req_cookies, shared_link_cookie_name(auth)), - {:ok, %{slug: token_slug}} <- Plausible.Auth.Token.verify_shared_link(token), - true <- token_slug == shared_link.slug do - render_shared_link(conn, shared_link) - else - _e -> - conn - |> assign(:skip_plausible_tracking, true) - |> render("shared_link_password.html", - link: shared_link, - layout: {PlausibleWeb.LayoutView, "focus.html"} - ) - end - else + 2. Shared link with password protection: Same checks as without the password, but an extra step is taken to + protect the page with a password. When the user passes the password challenge, a cookie is set with Plausible.Auth.Token.sign_shared_link(). + The cookie allows the user to access the dashboard for 24 hours without entering the password again. + + ### Backwards compatibility + + The URL format for shared links was changed in [this pull request](https://github.com/plausible/analytics/pull/752) in order + to make the URLs easier to bookmark. The old format is supported along with the new in order to not break old links. + + See: https://plausible.io/docs/shared-links + """ + def shared_link(conn, %{"domain" => domain, "auth" => auth}) do + case find_shared_link(domain, auth) do + {:password_protected, shared_link} -> + render_password_protected_shared_link(conn, shared_link) + + {:unlisted, shared_link} -> render_shared_link(conn, shared_link) - end + + :not_found -> + render_error(conn, 404) end end - def shared_link(conn, %{"slug" => slug}) do + @old_format_deprecation_date ~N[2022-01-01 00:00:00] + def shared_link(conn, %{"domain" => slug}) do shared_link = - Repo.get_by(Plausible.Site.SharedLink, slug: slug) - |> Repo.preload(:site) + Repo.one( + from l in Plausible.Site.SharedLink, + where: l.slug == ^slug and l.inserted_at < ^@old_format_deprecation_date, + preload: :site + ) if shared_link do - redirect(conn, to: "/share/#{URI.encode_www_form(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) end @@ -148,6 +196,44 @@ defmodule PlausibleWeb.StatsController do render_error(conn, 400) end + defp render_password_protected_shared_link(conn, shared_link) do + with conn <- Plug.Conn.fetch_cookies(conn), + {:ok, token} <- Map.fetch(conn.req_cookies, shared_link_cookie_name(shared_link.slug)), + {:ok, %{slug: token_slug}} <- Plausible.Auth.Token.verify_shared_link(token), + true <- token_slug == shared_link.slug do + render_shared_link(conn, shared_link) + else + _e -> + conn + |> assign(:skip_plausible_tracking, true) + |> render("shared_link_password.html", + link: shared_link, + layout: {PlausibleWeb.LayoutView, "focus.html"} + ) + end + end + + defp find_shared_link(domain, auth) do + link_query = + from link in Plausible.Site.SharedLink, + inner_join: site in assoc(link, :site), + where: link.slug == ^auth, + where: site.domain == ^domain, + limit: 1, + preload: [site: site] + + case Repo.one(link_query) do + %Plausible.Site.SharedLink{password_hash: hash} = link when not is_nil(hash) -> + {:password_protected, link} + + %Plausible.Site.SharedLink{} = link -> + {:unlisted, link} + + nil -> + :not_found + end + end + def authenticate_shared_link(conn, %{"slug" => slug, "password" => password}) do shared_link = Repo.get_by(Plausible.Site.SharedLink, slug: slug) diff --git a/mix.exs b/mix.exs index e540c7e05..ace345cd1 100644 --- a/mix.exs +++ b/mix.exs @@ -115,7 +115,7 @@ defmodule Plausible.MixProject do {:telemetry, "~> 1.0", override: true}, {:timex, "~> 3.7"}, {:ua_inspector, "~> 3.0"}, - {:ex_doc, "~> 0.28"} + {:ex_doc, "~> 0.28", only: :dev, runtime: false} ] end @@ -140,7 +140,17 @@ defmodule Plausible.MixProject do ], groups_for_extras: [ Features: Path.wildcard("guides/features/*.md") - ] + ], + before_closing_body_tag: fn + :html -> + """ + + + """ + + _ -> + "" + end ] end end diff --git a/test/plausible_web/controllers/api/external_stats_controller/breakdown_test.exs b/test/plausible_web/controllers/api/external_stats_controller/breakdown_test.exs index 308d750a0..cffdc7c8c 100644 --- a/test/plausible_web/controllers/api/external_stats_controller/breakdown_test.exs +++ b/test/plausible_web/controllers/api/external_stats_controller/breakdown_test.exs @@ -976,13 +976,6 @@ defmodule PlausibleWeb.Api.ExternalStatsController.BreakdownTest do domain: site.domain, timestamp: ~N[2021-01-01 00:00:00] ), - build(:event, - name: "Purchase", - "meta.key": ["cost"], - "meta.value": ["16"], - domain: site.domain, - timestamp: ~N[2021-01-01 00:00:00] - ), build(:event, name: "Purchase", "meta.key": ["cost"], @@ -1019,7 +1012,7 @@ defmodule PlausibleWeb.Api.ExternalStatsController.BreakdownTest do assert json_response(conn, 200) == %{ "results" => [ %{"cost" => "14", "visitors" => 2}, - %{"cost" => "16", "visitors" => 2} + %{"cost" => "16", "visitors" => 1} ] } end diff --git a/test/plausible_web/controllers/stats_controller_test.exs b/test/plausible_web/controllers/stats_controller_test.exs index 54a4a1845..138deefa6 100644 --- a/test/plausible_web/controllers/stats_controller_test.exs +++ b/test/plausible_web/controllers/stats_controller_test.exs @@ -223,7 +223,7 @@ defmodule PlausibleWeb.StatsControllerTest do end end - describe "GET /share/:slug" do + describe "GET /share/:domain?auth=:auth" do test "prompts a password for a password-protected link", %{conn: conn} do site = insert(:site) @@ -264,9 +264,41 @@ defmodule PlausibleWeb.StatsControllerTest do refute String.contains?(html_response(conn, 200), "Back to my sites") end - test "renders bad request when no auth parameter supplied", %{conn: conn} do + test "renders 404 not found when no auth parameter supplied", %{conn: conn} do conn = get(conn, "/share/example.com") - assert response(conn, 400) =~ "Bad Request" + assert response(conn, 404) =~ "nothing here" + end + + test "renders 404 not found when non-existent auth parameter is supplied", %{conn: conn} do + conn = get(conn, "/share/example.com?auth=bad-token") + assert response(conn, 404) =~ "nothing here" + end + + test "renders 404 not found when auth parameter for another site is supplied", %{conn: conn} do + site1 = insert(:site, domain: "test-site-1.com") + site2 = insert(:site, domain: "test-site-2.com") + site1_link = insert(:shared_link, site: site1) + + conn = get(conn, "/share/#{site2.domain}/?auth=#{site1_link.slug}") + assert response(conn, 404) =~ "nothing here" + end + end + + describe "GET /share/:slug - backwards compatibility" do + test "it redirects to new shared link format for historical links", %{conn: conn} do + site = insert(:site, domain: "test-site.com") + 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}" + end + + test "it does nothing for newer links", %{conn: conn} do + site = insert(:site, domain: "test-site.com") + site_link = insert(:shared_link, site: site, inserted_at: ~N[2022-01-01 00:00:00]) + + conn = get(conn, "/share/#{site_link.slug}") + assert response(conn, 404) =~ "nothing here" end end