Protect against custom props payloads breaking event insertions (#3617)

* Protect against custom props payloads breaking event insertions

* Use two traversals instead of three

* Reorganize invalid props filtering

lib/plausible/clickhouse_event_v2.ex

@@ -78,8 +78,7 @@ defmodule Plausible.ClickhouseEventV2 do
         :revenue_source_currency,
         :revenue_reporting_amount,
         :revenue_reporting_currency
-      ],
-      empty_values: [nil, ""]
+      ]
     )
     |> validate_required([:name, :site_id, :hostname, :pathname, :user_id, :timestamp])
   end

lib/plausible/ingestion/event.ex

@@ -213,9 +213,12 @@ defmodule Plausible.Ingestion.Event do
   end
 
   defp put_props(%__MODULE__{request: %{props: %{} = props}} = event) do
+    # defensive: ensuring the keys/values are always in the same order
+    {keys, values} = Enum.unzip(props)
+
     update_attrs(event, %{
-      "meta.key": Map.keys(props),
-      "meta.value": Enum.map(props, fn {_, v} -> to_string(v) end)
+      "meta.key": keys,
+      "meta.value": values
     })
   end
 

lib/plausible/ingestion/request.ex

@@ -206,7 +206,7 @@ defmodule Plausible.Ingestion.Request do
     props =
       (request_body["m"] || request_body["meta"] || request_body["p"] || request_body["props"])
       |> Plausible.Helpers.JSON.decode_or_fallback()
-      |> Enum.reject(fn {_k, v} -> is_nil(v) || is_list(v) || is_map(v) || v == "" end)
+      |> Enum.reduce([], &filter_bad_props/2)
       |> Enum.take(@max_props)
       |> Map.new()
 
@@ -215,6 +215,14 @@ defmodule Plausible.Ingestion.Request do
     |> validate_props()
   end
 
+  defp filter_bad_props({k, v}, acc) do
+    cond do
+      Enum.any?([k, v], &(is_list(&1) or is_map(&1))) -> acc
+      Enum.any?([k, v], &(String.trim_leading(to_string(&1)) == "")) -> acc
+      true -> [{to_string(k), to_string(v)} | acc]
+    end
+  end
+
   @max_prop_key_length Plausible.Props.max_prop_key_length()
   @max_prop_value_length Plausible.Props.max_prop_value_length()
   defp validate_props(changeset) do

test/plausible_web/controllers/api/external_controller_test.exs

@@ -566,6 +566,30 @@ defmodule PlausibleWeb.Api.ExternalControllerTest do
       assert Map.get(event, :"meta.value") == ["true", "12"]
     end
 
+    test "filters out bad props", %{conn: conn, site: site} do
+      params = %{
+        name: "Signup",
+        url: "http://gigride.live/",
+        domain: site.domain,
+        props: %{
+          false: nil,
+          nil: false,
+          good: true,
+          "      ": "    ",
+          "    ": "value",
+          key: "    "
+        }
+      }
+
+      conn
+      |> post("/api/event", params)
+
+      event = get_event(site)
+
+      assert Map.get(event, :"meta.key") == ["good"]
+      assert Map.get(event, :"meta.value") == ["true"]
+    end
+
     test "ignores malformed custom props", %{conn: conn, site: site} do
       params = %{
         name: "Signup",