From 9ad50c30ec49ea07549284d85e55e37221525375 Mon Sep 17 00:00:00 2001
From: "Tyler A. Young" <tyler@tylerayoung.com>
Date: Thu, 5 Sep 2024 10:01:46 -0500
Subject: [PATCH] Convert string coordinates to floats, or raise an error
 (#218)

* Fix broken test

* Convert string coordinates to floats, or raise an error

This fixes an issue where we were silently accepting non-numeric coordinates in the GeoJSON decoder, such that you could wind up doing things like decoding a point like `%Geo.Point{coordinates: {"100.0", "-10.0"}}`. This would obviously not have gone well for you later in your processing pipeline.

The fix here, suggested by @LostKobrakai, is to convert those strings to numbers where we can do so unambiguously. While such inputs are clearly invalid, it's easy enough to handle them in the way that the user was hoping that we should probably just do it. In cases where there's any ambiguity at all, we raise an ArgumentError.

Resolves #175

* Also handle non-numeric, non-list, non-string coords
---
 lib/geo/json/decoder.ex | 54 ++++++++++++++++++++++++++++++++++--
 test/geo/json_test.exs  | 61 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 112 insertions(+), 3 deletions(-)

diff --git a/lib/geo/json/decoder.ex b/lib/geo/json/decoder.ex
index b42999d..673bc6a 100644
--- a/lib/geo/json/decoder.ex
+++ b/lib/geo/json/decoder.ex
@@ -46,7 +46,7 @@ defmodule Geo.JSON.Decoder do
           Enum.map(Map.get(geo_json, "geometries"), fn x ->
             do_decode(
               Map.get(x, "type"),
-              Map.get(x, "coordinates"),
+              ensure_numeric(x["coordinates"]),
               Map.get(x, "properties", %{}),
               crs
             )
@@ -62,7 +62,7 @@ defmodule Geo.JSON.Decoder do
 
         do_decode(
           Map.get(geo_json, "type"),
-          Map.get(geo_json, "coordinates"),
+          ensure_numeric(geo_json["coordinates"]),
           Map.get(geo_json, "properties", %{}),
           crs
         )
@@ -205,7 +205,12 @@ defmodule Geo.JSON.Decoder do
         properties: properties
       }
     else
-      do_decode(Map.get(geometry, "type"), Map.get(geometry, "coordinates"), properties, nil)
+      do_decode(
+        Map.get(geometry, "type"),
+        ensure_numeric(geometry["coordinates"]),
+        properties,
+        nil
+      )
     end
   end
 
@@ -231,4 +236,47 @@ defmodule Geo.JSON.Decoder do
   defp get_srid(nil) do
     nil
   end
+
+  # Fast paths for the common (correct) cases
+  defp ensure_numeric(num) when is_number(num), do: num
+  defp ensure_numeric([x, y] = l) when is_number(x) and is_number(y), do: l
+
+  defp ensure_numeric([x, y, z] = l)
+       when is_number(x) and is_number(y) and (is_number(z) or is_nil(z)) do
+    l
+  end
+
+  defp ensure_numeric([x, y, z, m] = l)
+       when is_number(x) and is_number(y) and (is_number(z) or is_nil(z)) and
+              (is_number(m) or is_nil(z)) do
+    l
+  end
+
+  defp ensure_numeric(l) when is_list(l) do
+    Enum.map(l, fn
+      num when is_number(num) ->
+        num
+
+      str when is_binary(str) ->
+        try do
+          String.to_float(str)
+        catch
+          ArgumentError ->
+            raise ArgumentError, "expected a numeric coordinate, got the string #{inspect(str)}"
+        end
+
+      nil ->
+        nil
+
+      l when is_list(l) ->
+        Enum.map(l, &ensure_numeric/1)
+
+      other ->
+        raise ArgumentError, "expected a numeric coordinate, got: #{inspect(other)}"
+    end)
+  end
+
+  defp ensure_numeric(other) do
+    raise ArgumentError, "expected a numeric coordinate, got: #{inspect(other)}"
+  end
 end
diff --git a/test/geo/json_test.exs b/test/geo/json_test.exs
index 0dbbf5b..193be51 100644
--- a/test/geo/json_test.exs
+++ b/test/geo/json_test.exs
@@ -387,6 +387,67 @@ defmodule Geo.JSON.Test do
     assert geom.geometries == []
   end
 
+  test "Decode seamlessly converts coordinates that are numbers-as-strings" do
+    check all(
+            x <- float(),
+            y <- float()
+          ) do
+      json = """
+        {
+          "properties": {},
+          "geometry": {
+            "type": "Point",
+            "coordinates": ["#{x}", "#{y}"]
+          },
+          "type": "Feature"
+        }
+      """
+
+      assert %Geo.Point{coordinates: {^x, ^y}} = Jason.decode!(json) |> Geo.JSON.decode!()
+    end
+  end
+
+  test "Decode rejects geometries with non-numeric coordinates" do
+    for {bad_x, bad_y} <- [
+          {" 100.0", "0.0"},
+          {"100.0", "0.0?"},
+          {"100.", "0.0"},
+          {"100.0", nil, "0.0"}
+        ] do
+      json = """
+        {
+          "properties": {},
+          "geometry": {
+            "type": "Point",
+            "coordinates": [#{inspect(bad_x)}, #{inspect(bad_y)}]
+          },
+          "type": "Feature"
+        }
+      """
+
+      assert_raise ArgumentError, fn ->
+        Jason.decode!(json) |> Geo.JSON.decode!()
+      end
+    end
+  end
+
+  test "Decode rejects geometries with garbage coordinates" do
+    json = """
+      {
+        "properties": {},
+        "geometry": {
+          "type": "Point",
+          "coordinates": {"x": 1.0, "y": 2.0}
+        },
+        "type": "Feature"
+      }
+    """
+
+    assert_raise ArgumentError, fn ->
+      Jason.decode!(json) |> Geo.JSON.decode!()
+    end
+  end
+
   property "encodes and decodes back to the correct Point struct" do
     check all(
             x <- float(),