Add fallback for favicon (#2279)

* Add fallback for favicon

* Add Favicon tests

* Changelog

* Move placeholder icon to priv folder

CHANGELOG.md

@@ -62,6 +62,7 @@ All notable changes to this project will be documented in this file.
 - `DISABLE_REGISTRATIONS` configuration parameter can now accept `invite_only` to allow invited users to register an account while keeping regular registrations disabled plausible/analytics#1841
 - New and improved Session tracking module for higher throughput and lower latency. [PR#1934](https://github.com/plausible/analytics#1934)
 - Do not display ZZ country code in countries report [PR#1934](https://github.com/plausible/analytics#2223)
+- Add fallback icon for when DDG favicon cannot be fetched [PR#2279](https://github.com/plausible/analytics#2279)
 
 ## v1.4.1
 

lib/plausible_web/plugs/favicon.ex

@@ -1,34 +1,37 @@
 defmodule PlausibleWeb.Favicon do
   @referer_domains_file "priv/referer_favicon_domains.json"
   @moduledoc """
-    A Plug that fetches favicon images from DuckDuckGo and returns them
-    to the Plausible frontend.
+  A Plug that fetches favicon images from DuckDuckGo and returns them
+  to the Plausible frontend.
 
-    The proxying is there so we can reduce the number of third-party domains that
-    the browser clients need to connect to. Our goal is to have 0 third-party domain
-    connections on the website for privacy reasons.
+  The proxying is there so we can reduce the number of third-party domains that
+  the browser clients need to connect to. Our goal is to have 0 third-party domain
+  connections on the website for privacy reasons.
 
-    This module also maps between categorized sources and their respective URLs for favicons.
-    What does that mean exactly? During ingestion we use `PlausibleWeb.RefInspector.parse/1` to
-    categorize our referrer sources like so:
+  This module also maps between categorized sources and their respective URLs for favicons.
+  What does that mean exactly? During ingestion we use `PlausibleWeb.RefInspector.parse/1` to
+  categorize our referrer sources like so:
 
-    google.com -> Google
-    google.co.uk -> Google
-    google.com.au -> Google
+  google.com -> Google
+  google.co.uk -> Google
+  google.com.au -> Google
 
-    So when we show Google as a source in the dashboard, the request to this plug will come as:
-    https://plausible/io/favicon/sources/Google
+  So when we show Google as a source in the dashboard, the request to this plug will come as:
+  https://plausible/io/favicon/sources/Google
 
-    Now, when we want to show a favicon for Google, we need to convert Google -> google.com or
-    some other hostname owned by Google:
-    https://icons.duckduckgo.com/ip3/google.com.ico
+  Now, when we want to show a favicon for Google, we need to convert Google -> google.com or
+  some other hostname owned by Google:
+  https://icons.duckduckgo.com/ip3/google.com.ico
 
-    The mapping from source category -> source hostname is stored in "#{@referer_domains_file}" and
-    managed by `Mix.Tasks.GenerateReferrerFavicons.run/1`
+  The mapping from source category -> source hostname is stored in "#{@referer_domains_file}" and
+  managed by `Mix.Tasks.GenerateReferrerFavicons.run/1`
   """
   import Plug.Conn
   alias Plausible.HTTPClient
 
+  @placeholder_icon_location "priv/placeholder_favicon.ico"
+  @placeholder_icon File.read!(@placeholder_icon_location)
+
   def init(_) do
     domains =
       File.read!(Application.app_dir(:plausible, @referer_domains_file))
@@ -37,9 +40,19 @@ defmodule PlausibleWeb.Favicon do
     [favicon_domains: domains]
   end
 
+  @ddg_broken_icon <<137, 80, 78, 71, 13, 10, 26, 10>>
   @doc """
-    Proxies HTTP request to DuckDuckGo favicon service. Swallows hop-by-hop HTTP headers that
-    should not be forwarded as defined in RFC 2616 (https://www.rfc-editor.org/rfc/rfc2616#section-13.5.1)
+  Proxies HTTP request to DuckDuckGo favicon service. Swallows hop-by-hop HTTP headers that
+  should not be forwarded as defined in RFC 2616 (https://www.rfc-editor.org/rfc/rfc2616#section-13.5.1)
+
+  Cases where we show a placeholder icon instead:
+  * In case of network error to DuckDuckGo
+  * In case of non-2xx status code from DuckDuckGo
+  * In case of broken image response body from DuckDuckGo
+
+  I'm not sure why DDG sometimes returns a broken PNG image in their response but we filter that out.
+  When the icon request fails, we show a placeholder favicon instead. The placeholder is an emoji
+  from https://favicon.io/emoji-favicons/
   """
   def call(conn, favicon_domains: favicon_domains) do
     case conn.path_info do
@@ -48,14 +61,14 @@ defmodule PlausibleWeb.Favicon do
         domain = Map.get(favicon_domains, clean_source, clean_source)
 
         case HTTPClient.impl().get("https://icons.duckduckgo.com/ip3/#{domain}.ico") do
-          {:ok, res} ->
+          {:ok, %Finch.Response{body: body, headers: headers}} when body != @ddg_broken_icon ->
             conn
-            |> forward_headers(res)
-            |> send_resp(200, res.body)
+            |> forward_headers(headers)
+            |> send_resp(200, body)
             |> halt
 
           _ ->
-            send_resp(conn, 503, "") |> halt
+            send_placeholder(conn)
         end
 
       _ ->
@@ -63,9 +76,17 @@ defmodule PlausibleWeb.Favicon do
     end
   end
 
+  defp send_placeholder(conn) do
+    conn
+    |> put_resp_content_type("image/x-icon")
+    |> put_resp_header("cache-control", "public, max-age=2592000")
+    |> send_resp(200, @placeholder_icon)
+    |> halt
+  end
+
   @forwarded_headers ["content-type", "cache-control", "expires"]
-  defp forward_headers(conn, response) do
-    headers = Enum.filter(response.headers, fn {k, _} -> k in @forwarded_headers end)
-    %Plug.Conn{conn | resp_headers: headers}
+  defp forward_headers(conn, headers) do
+    headers_to_forward = Enum.filter(headers, fn {k, _} -> k in @forwarded_headers end)
+    %Plug.Conn{conn | resp_headers: headers_to_forward}
   end
 end

test/plausible_web/plugs/favicon_test.exs

@@ -84,4 +84,65 @@ defmodule PlausibleWeb.FaviconTest do
              {"content-type", "should-pass-through"}
            ]
   end
+
+  describe "Fallback to placeholder icon" do
+    @placholder_icon File.read!("priv/placeholder_favicon.ico")
+
+    test "falls back to placeholder when DDG returns a non-2xx response", %{plug_opts: plug_opts} do
+      expect(
+        Plausible.HTTPClient.Mock,
+        :get,
+        fn "https://icons.duckduckgo.com/ip3/plausible.io.ico" ->
+          res = %Finch.Response{status: 503, body: "bad gateway"}
+          {:error, Plausible.HTTPClient.Non200Error.new(res)}
+        end
+      )
+
+      conn =
+        conn(:get, "/favicon/sources/plausible.io")
+        |> Favicon.call(plug_opts)
+
+      assert conn.halted
+      assert conn.status == 200
+      assert conn.resp_body == @placholder_icon
+    end
+
+    test "falls back to placeholder in case of a network error", %{plug_opts: plug_opts} do
+      expect(
+        Plausible.HTTPClient.Mock,
+        :get,
+        fn "https://icons.duckduckgo.com/ip3/plausible.io.ico" ->
+          {:error, %Mint.TransportError{reason: :closed}}
+        end
+      )
+
+      conn =
+        conn(:get, "/favicon/sources/plausible.io")
+        |> Favicon.call(plug_opts)
+
+      assert conn.halted
+      assert conn.status == 200
+      assert conn.resp_body == @placholder_icon
+    end
+
+    test "falls back to placeholder when DDG returns a broken image response", %{
+      plug_opts: plug_opts
+    } do
+      expect(
+        Plausible.HTTPClient.Mock,
+        :get,
+        fn "https://icons.duckduckgo.com/ip3/plausible.io.ico" ->
+          {:ok, %Finch.Response{status: 200, body: <<137, 80, 78, 71, 13, 10, 26, 10>>}}
+        end
+      )
+
+      conn =
+        conn(:get, "/favicon/sources/plausible.io")
+        |> Favicon.call(plug_opts)
+
+      assert conn.halted
+      assert conn.status == 200
+      assert conn.resp_body == @placholder_icon
+    end
+  end
 end