diff --git a/CHANGELOG.md b/CHANGELOG.md index 144394dc1..6ebf84140 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/plausible_web/plugs/favicon.ex b/lib/plausible_web/plugs/favicon.ex index 02bce94b0..63ab9251c 100644 --- a/lib/plausible_web/plugs/favicon.ex +++ b/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 diff --git a/priv/placeholder_favicon.ico b/priv/placeholder_favicon.ico new file mode 100644 index 000000000..35aa65d28 Binary files /dev/null and b/priv/placeholder_favicon.ico differ diff --git a/test/plausible_web/plugs/favicon_test.exs b/test/plausible_web/plugs/favicon_test.exs index b15beca00..d2134c07f 100644 --- a/test/plausible_web/plugs/favicon_test.exs +++ b/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