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

@@ -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

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.
+
+  <div class="mermaid">
+  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
+  </div>
+
+  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,15 +145,60 @@ 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
+    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)
+
+      :not_found ->
+        render_error(conn, 404)
+    end
+  end
+
+  @old_format_deprecation_date ~N[2022-01-01 00:00:00]
+  def shared_link(conn, %{"domain" => slug}) do
+    shared_link =
+      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
+      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
+  end
+
+  def shared_link(conn, _) 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(auth)),
+         {: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)
@@ -126,26 +211,27 @@ defmodule PlausibleWeb.StatsController do
           layout: {PlausibleWeb.LayoutView, "focus.html"}
         )
     end
-      else
-        render_shared_link(conn, shared_link)
-      end
-    end
   end
 
-  def shared_link(conn, %{"slug" => slug}) do
-    shared_link =
-      Repo.get_by(Plausible.Site.SharedLink, slug: slug)
-      |> Repo.preload(:site)
+  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]
 
-    if shared_link do
-      redirect(conn, to: "/share/#{URI.encode_www_form(shared_link.site.domain)}?auth=#{slug}")
-    else
-      render_error(conn, 404)
-    end
-  end
+    case Repo.one(link_query) do
+      %Plausible.Site.SharedLink{password_hash: hash} = link when not is_nil(hash) ->
+        {:password_protected, link}
 
-  def shared_link(conn, _) do
-    render_error(conn, 400)
+      %Plausible.Site.SharedLink{} = link ->
+        {:unlisted, link}
+
+      nil ->
+        :not_found
+    end
   end
 
   def authenticate_shared_link(conn, %{"slug" => slug, "password" => password}) do

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 ->
+          """
+          <script src="https://cdn.jsdelivr.net/npm/mermaid/dist/mermaid.min.js"></script>
+          <script>mermaid.initialize({startOnLoad: true})</script>
+          """
+
+        _ ->
+          ""
+      end
     ]
   end
 end

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

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