Improve user input validation (#2291)

* Make pagination params parsing ignore bad input

* Remove unused binding

* Don't crash on filter parse error

* Sanitize input date on internal stats API

* Revert Query module changes (ref 55645734)

* Implement simplistic input date validation in stats controller

* Mute bad request logging

lib/plausible/stats/filter_parser.ex

@@ -39,32 +39,41 @@ defmodule Plausible.Stats.FilterParser do
     filters = String.split(str, ";")
 
     Enum.map(filters, &parse_single_filter/1)
+    |> Enum.reject(fn parsed -> parsed == :error end)
     |> Enum.into(%{})
   end
 
   @non_escaped_pipe_regex ~r/(?<!\\)\|/
   defp parse_single_filter(str) do
-    [key, raw_value] =
-      String.trim(str)
-      |> String.split(["==", "!="], trim: true)
-      |> Enum.map(&String.trim/1)
+    case to_kv(str) do
+      [key, raw_value] ->
+        is_negated = String.contains?(str, "!=")
+        is_list = Regex.match?(@non_escaped_pipe_regex, raw_value)
+        is_wildcard = String.contains?(raw_value, "*")
 
-    is_negated = String.contains?(str, "!=")
-    is_list = Regex.match?(@non_escaped_pipe_regex, raw_value)
-    is_wildcard = String.contains?(raw_value, "*")
+        final_value = remove_escape_chars(raw_value)
 
-    final_value = remove_escape_chars(raw_value)
+        cond do
+          key == "event:goal" -> {key, parse_goal_filter(final_value)}
+          is_wildcard && is_negated -> {key, {:does_not_match, raw_value}}
+          is_wildcard -> {key, {:matches, raw_value}}
+          is_list -> {key, {:member, parse_member_list(raw_value)}}
+          is_negated -> {key, {:is_not, final_value}}
+          true -> {key, {:is, final_value}}
+        end
 
-    cond do
-      key == "event:goal" -> {key, parse_goal_filter(final_value)}
-      is_wildcard && is_negated -> {key, {:does_not_match, raw_value}}
-      is_wildcard -> {key, {:matches, raw_value}}
-      is_list -> {key, {:member, parse_member_list(raw_value)}}
-      is_negated -> {key, {:is_not, final_value}}
-      true -> {key, {:is, final_value}}
+      _ ->
+        :error
     end
   end
 
+  defp to_kv(str) do
+    str
+    |> String.trim()
+    |> String.split(["==", "!="], trim: true)
+    |> Enum.map(&String.trim/1)
+  end
+
   defp parse_goal_filter("Visit " <> page), do: {:is, :page, page}
   defp parse_goal_filter(event), do: {:is, :event, event}
 

lib/plausible_web/controllers/api/stats_controller.ex

@@ -7,54 +7,68 @@ defmodule PlausibleWeb.Api.StatsController do
 
   def main_graph(conn, params) do
     site = conn.assigns[:site]
-    query = Query.from(site, params) |> Filters.add_prefix()
 
-    selected_metric =
-      if !params["metric"] || params["metric"] == "conversions" do
-        "visitors"
-      else
-        params["metric"]
-      end
+    with :ok <- validate_params(params) do
+      query = Query.from(site, params) |> Filters.add_prefix()
 
-    timeseries_query =
-      if query.period == "realtime" do
-        %Query{query | period: "30m"}
-      else
-        query
-      end
+      selected_metric =
+        if !params["metric"] || params["metric"] == "conversions" do
+          "visitors"
+        else
+          params["metric"]
+        end
 
-    timeseries_result =
-      Stats.timeseries(site, timeseries_query, [String.to_existing_atom(selected_metric)])
+      timeseries_query =
+        if query.period == "realtime" do
+          %Query{query | period: "30m"}
+        else
+          query
+        end
 
-    plot =
-      Enum.map(timeseries_result, fn row -> row[String.to_existing_atom(selected_metric)] || 0 end)
+      timeseries_result =
+        Stats.timeseries(site, timeseries_query, [String.to_existing_atom(selected_metric)])
 
-    labels = Enum.map(timeseries_result, fn row -> row[:date] end)
-    present_index = present_index_for(site, query, labels)
+      plot =
+        Enum.map(timeseries_result, fn row ->
+          row[String.to_existing_atom(selected_metric)] || 0
+        end)
 
-    json(conn, %{
-      plot: plot,
-      labels: labels,
-      present_index: present_index,
-      interval: query.interval,
-      with_imported: query.include_imported,
-      imported_source: site.imported_data && site.imported_data.source
-    })
+      labels = Enum.map(timeseries_result, fn row -> row[:date] end)
+      present_index = present_index_for(site, query, labels)
+
+      json(conn, %{
+        plot: plot,
+        labels: labels,
+        present_index: present_index,
+        interval: query.interval,
+        with_imported: query.include_imported,
+        imported_source: site.imported_data && site.imported_data.source
+      })
+    else
+      _ ->
+        bad_request(conn)
+    end
   end
 
   def top_stats(conn, params) do
     site = conn.assigns[:site]
-    query = Query.from(site, params) |> Filters.add_prefix()
 
-    {top_stats, sample_percent} = fetch_top_stats(site, query)
+    with :ok <- validate_params(params) do
+      query = Query.from(site, params) |> Filters.add_prefix()
 
-    json(conn, %{
-      top_stats: top_stats,
-      interval: query.interval,
-      sample_percent: sample_percent,
-      with_imported: query.include_imported,
-      imported_source: site.imported_data && site.imported_data.source
-    })
+      {top_stats, sample_percent} = fetch_top_stats(site, query)
+
+      json(conn, %{
+        top_stats: top_stats,
+        interval: query.interval,
+        sample_percent: sample_percent,
+        with_imported: query.include_imported,
+        imported_source: site.imported_data && site.imported_data.source
+      })
+    else
+      _ ->
+        bad_request(conn)
+    end
   end
 
   defp present_index_for(site, query, dates) do
@@ -909,9 +923,15 @@ defmodule PlausibleWeb.Api.StatsController do
 
   def filter_suggestions(conn, params) do
     site = conn.assigns[:site]
-    query = Query.from(site, params) |> Filters.add_prefix()
 
-    json(conn, Stats.filter_suggestions(site, query, params["filter_name"], params["q"]))
+    with :ok <- validate_params(params) do
+      query = Query.from(site, params) |> Filters.add_prefix()
+
+      json(conn, Stats.filter_suggestions(site, query, params["filter_name"], params["q"]))
+    else
+      _ ->
+        bad_request(conn)
+    end
   end
 
   defp transform_keys(results, keys_to_replace) do
@@ -924,11 +944,23 @@ defmodule PlausibleWeb.Api.StatsController do
   end
 
   defp parse_pagination(params) do
-    limit = if params["limit"], do: String.to_integer(params["limit"]), else: 9
-    page = if params["page"], do: String.to_integer(params["page"]), else: 1
+    limit = to_int(params["limit"], 9)
+    page = to_int(params["page"], 1)
     {limit, page}
   end
 
+  defp to_int(string, default) when is_binary(string) do
+    case Integer.parse(string) do
+      {i, ""} when is_integer(i) ->
+        i
+
+      _ ->
+        default
+    end
+  end
+
+  defp to_int(_, default), do: default
+
   defp maybe_add_percentages(stat_list, query) do
     if Map.has_key?(query.filters, "event:goal") do
       stat_list
@@ -1007,4 +1039,20 @@ defmodule PlausibleWeb.Api.StatsController do
         country
     end
   end
+
+  defp validate_params(%{"date" => date}) do
+    with {:ok, _} <- Date.from_iso8601(date) do
+      :ok
+    end
+  end
+
+  defp validate_params(_) do
+    :ok
+  end
+
+  defp bad_request(conn) do
+    conn
+    |> put_status(400)
+    |> json(%{error: "input validation error"})
+  end
 end

test/plausible/stats/filter_parser_test.exs

@@ -76,5 +76,10 @@ defmodule Plausible.Stats.FilterParserTest do
       "event:page==/**\\|page|/other/page"
       |> assert_parsed(%{"event:page" => {:matches, "/**\\|page|/other/page"}})
     end
+
+    test "gracefully fails to parse garbage" do
+      "bfg10309\uff1cs1\ufe65s2\u02bas3\u02b9hjl10309"
+      |> assert_parsed(%{})
+    end
   end
 end

test/plausible_web/controllers/api/stats_controller/suggestions_test.exs

@@ -271,5 +271,36 @@ defmodule PlausibleWeb.Api.StatsController.SuggestionsTest do
 
       assert json_response(conn, 200) |> Enum.sort() == ["Uku Taht"]
     end
+
+    test "when date is borked, bad request is returned", %{
+      conn: conn,
+      site: site
+    } do
+      today = (Date.utc_today() |> Date.to_iso8601()) <> " 00:00:00"
+      naive_today = NaiveDateTime.from_iso8601!(today)
+
+      populate_stats(site, [
+        build(:pageview,
+          "meta.key": ["author"],
+          "meta.value": ["Alice Bob"],
+          timestamp: naive_today
+        ),
+        build(:pageview,
+          "meta.key": ["author"],
+          "meta.value": ["Cecil"],
+          timestamp: ~N[2022-01-01 00:00:00]
+        )
+      ])
+
+      filters = Jason.encode!(%{props: %{author: "!(none)"}})
+
+      conn =
+        get(
+          conn,
+          "/api/stats/#{site.domain}/suggestions/prop_value?period=all&date=CLEVER_SECURITY_RESEARCH&filters=#{filters}"
+        )
+
+      assert json_response(conn, 400) == %{"error" => "input validation error"}
+    end
   end
 end

test/plausible_web/controllers/auth_controller_test.exs

@@ -585,7 +585,7 @@ defmodule PlausibleWeb.AuthControllerTest do
       assert redirected_to(conn, 302) == "/settings"
     end
 
-    test "renders form with error if form validations fail", %{conn: conn, user: user} do
+    test "renders form with error if form validations fail", %{conn: conn} do
       conn = put(conn, "/settings", %{"user" => %{"name" => ""}})
 
       assert html_response(conn, 200) =~ "can't be blank"