From b5d7cf4a381323ef06c4d18dfd7fb1f4972c5e66 Mon Sep 17 00:00:00 2001
From: Graham McIntire <graham@mcintire.me>
Date: Thu, 19 Feb 2026 15:01:38 -0600
Subject: [PATCH] Replace ip-api.com geolocation with Cloudflare headers

Use CF-IPLatitude/CF-IPLongitude headers instead of making HTTP calls
to ip-api.com on every page load. Eliminates ~500ms latency, external
API dependency, and flaky tests.

Also disable test server to prevent port conflicts during development.
---
 .dialyzer_ignore.exs                          |   4 -
 config/test.exs                               |   2 +-
 lib/aprsme_web/plugs/ip_geolocation.ex        | 196 +-----------------
 test/aprsme_web/plugs/ip_geolocation_test.exs | 147 ++++++-------
 4 files changed, 86 insertions(+), 263 deletions(-)

diff --git a/.dialyzer_ignore.exs b/.dialyzer_ignore.exs
index bba37cb..e60f0e2 100644
--- a/.dialyzer_ignore.exs
+++ b/.dialyzer_ignore.exs
@@ -11,10 +11,6 @@
   # The filter_packets_by_bounds function has two clauses for map and list inputs
   ~r/lib\/aprsme_web\/live\/map_live\/index\.ex.*The pattern.*can never match/,
 
-  # IP geolocation plug - these patterns are actually reachable but dialyzer can't prove it
-  # The remote_ip can be nil or other values beyond just IPv4/IPv6 tuples
-  ~r/lib\/aprsme_web\/plugs\/ip_geolocation\.ex/,
-
   # False positive: Aprs.parse/1 does return {:ok, _} but dialyzer can't see the vendored library types
   {"lib/aprsme/is/is.ex"},
 
diff --git a/config/test.exs b/config/test.exs
index f556a35..8a7ac6a 100644
--- a/config/test.exs
+++ b/config/test.exs
@@ -35,7 +35,7 @@ config :aprsme, AprsmeWeb.Endpoint,
   adapter: Bandit.PhoenixAdapter,
   http: [ip: {127, 0, 0, 1}, port: 4002],
   secret_key_base: "IV9+ENaw9i8xjReRk4sULRvRgsmFVTGQwQGGrf4G+Q/SFMeHBCNWRlPXQ2YvT36R",
-  server: true
+  server: false
 
 # Disable cleanup scheduler in test environment
 config :aprsme, :cleanup_scheduler, enabled: false
diff --git a/lib/aprsme_web/plugs/ip_geolocation.ex b/lib/aprsme_web/plugs/ip_geolocation.ex
index 84b0293..c4a156e 100644
--- a/lib/aprsme_web/plugs/ip_geolocation.ex
+++ b/lib/aprsme_web/plugs/ip_geolocation.ex
@@ -1,14 +1,12 @@
 defmodule AprsmeWeb.Plugs.IPGeolocation do
   @moduledoc """
-  Plug that performs IP-based geolocation on initial page load.
-  Results are stored in the session to avoid repeated API calls.
+  Plug that extracts geolocation from Cloudflare's CF-IPLatitude and CF-IPLongitude
+  headers on initial page load. Results are stored in the session to avoid repeated parsing.
   """
   @behaviour Plug
 
   import Plug.Conn
 
-  require Logger
-
   @impl true
   def init(opts), do: opts
 
@@ -21,190 +19,18 @@ defmodule AprsmeWeb.Plugs.IPGeolocation do
 
   def call(conn, _opts), do: conn
 
-  defp handle_geolocation(nil, conn), do: perform_geolocation(conn)
+  defp handle_geolocation(nil, conn), do: extract_cf_headers(conn)
   defp handle_geolocation(_cached, conn), do: conn
 
-  defp perform_geolocation(conn) do
-    ip = get_client_ip(conn)
-    Logger.info("IP geolocation: Starting lookup for IP #{ip}")
-
-    if valid_ip_for_geolocation?(ip) do
-      start_time = System.monotonic_time(:millisecond)
-
-      # Run geolocation in a task with a hard timeout
-      task = Task.async(fn -> fetch_ip_location(ip) end)
-
-      case Task.yield(task, 600) || Task.shutdown(task, :brutal_kill) do
-        {:ok, {:ok, location}} ->
-          duration = System.monotonic_time(:millisecond) - start_time
-
-          Logger.info(
-            "IP geolocation: Successfully located #{ip} at #{location["lat"]}, #{location["lng"]} in #{duration}ms"
-          )
-
-          put_session(conn, :ip_geolocation, location)
-
-        {:ok, {:error, reason}} ->
-          duration = System.monotonic_time(:millisecond) - start_time
-          Logger.warning("IP geolocation: Failed for #{ip} after #{duration}ms - #{inspect(reason)}")
-          conn
-
-        nil ->
-          # Task timed out
-          duration = System.monotonic_time(:millisecond) - start_time
-          Logger.warning("IP geolocation: Task timeout for #{ip} after #{duration}ms")
-          conn
-      end
+  defp extract_cf_headers(conn) do
+    with [lat_str] <- get_req_header(conn, "cf-iplatitude"),
+         [lng_str] <- get_req_header(conn, "cf-iplongitude"),
+         {lat, ""} <- Float.parse(lat_str),
+         {lng, ""} <- Float.parse(lng_str),
+         true <- lat >= -90 and lat <= 90 and lng >= -180 and lng <= 180 do
+      put_session(conn, :ip_geolocation, %{"lat" => lat, "lng" => lng})
     else
-      Logger.info("IP geolocation: Skipping private/local IP #{ip}")
-      conn
+      _ -> conn
     end
   end
-
-  defp get_client_ip(conn) do
-    # Check for Cloudflare header first (CF-Connecting-IP)
-    cf_ip = get_req_header(conn, "cf-connecting-ip")
-
-    # Then check for standard forwarded headers
-    forwarded_for = get_req_header(conn, "x-forwarded-for")
-    real_ip = get_req_header(conn, "x-real-ip")
-
-    ip =
-      case {cf_ip, forwarded_for, real_ip} do
-        {[cf | _], _, _} ->
-          # Cloudflare header takes precedence
-          ip_from_cf = String.trim(cf)
-          Logger.info("IP geolocation: Using Cloudflare CF-Connecting-IP: #{ip_from_cf}")
-          ip_from_cf
-
-        {[], [forwarded | _], _} ->
-          # Take the first IP from the X-Forwarded-For header
-          ip_from_header =
-            forwarded
-            |> String.split(",")
-            |> List.first()
-            |> String.trim()
-
-          Logger.info("IP geolocation: Using forwarded IP from X-Forwarded-For header: #{ip_from_header}")
-          ip_from_header
-
-        {[], [], [real | _]} ->
-          # Use X-Real-IP header
-          ip_from_real = String.trim(real)
-          Logger.info("IP geolocation: Using X-Real-IP header: #{ip_from_real}")
-          ip_from_real
-
-        {[], [], []} ->
-          # Fall back to remote_ip
-          ip_from_remote =
-            case conn.remote_ip do
-              {a, b, c, d} ->
-                "#{a}.#{b}.#{c}.#{d}"
-
-              {a, b, c, d, e, f, g, h} ->
-                # Convert IPv6 tuple to proper IPv6 format
-                {a, b, c, d, e, f, g, h}
-                |> :inet.ntoa()
-                |> to_string()
-
-              _ ->
-                nil
-            end
-
-          if ip_from_remote do
-            Logger.info("IP geolocation: Using remote_ip: #{ip_from_remote}")
-          end
-
-          ip_from_remote
-      end
-
-    ip
-  end
-
-  defp valid_ip_for_geolocation?(ip) when is_binary(ip) do
-    not private_ip?(ip)
-  end
-
-  defp valid_ip_for_geolocation?(_), do: false
-
-  defp private_ip?("127." <> _), do: true
-  defp private_ip?("::1" <> _), do: true
-  defp private_ip?("10." <> _), do: true
-  defp private_ip?("192.168." <> _), do: true
-  defp private_ip?("172." <> _ = ip), do: in_172_private_range?(ip)
-  defp private_ip?(_), do: false
-
-  # Check if IP is in 172.16.0.0/12 range (172.16.0.0 - 172.31.255.255)
-  defp in_172_private_range?(ip) do
-    case String.split(ip, ".") do
-      ["172", second_octet | _] ->
-        case Integer.parse(second_octet) do
-          {num, _} when num >= 16 and num <= 31 -> true
-          _ -> false
-        end
-
-      _ ->
-        false
-    end
-  end
-
-  defp fetch_ip_location(ip) do
-    url = "http://ip-api.com/json/#{ip}"
-    Logger.info("IP geolocation: Making HTTP request to #{url}")
-
-    # Configure very short timeout to prevent blocking page load
-    req_options = [
-      # 500ms timeout - fail fast
-      receive_timeout: 500,
-      # No retries - we want fast failure
-      retry: false,
-      # Also limit connection pool wait time
-      pool_timeout: 500
-    ]
-
-    case Req.get(url, req_options) do
-      {:ok, %Req.Response{status: 200, body: body}} ->
-        Logger.info("IP geolocation: Got 200 response with body: #{inspect(body)}")
-        parse_ip_api_response(body)
-
-      {:ok, response} ->
-        Logger.warning("IP geolocation: Got non-200 response: #{response.status}")
-        {:error, :invalid_response}
-
-      {:error, reason} ->
-        Logger.error("IP geolocation: HTTP request failed: #{inspect(reason)}")
-        {:error, reason}
-    end
-  end
-
-  defp parse_ip_api_response(body) when is_map(body) do
-    case body do
-      %{"status" => "success", "lat" => lat, "lon" => lon}
-      when is_number(lat) and is_number(lon) ->
-        if lat >= -90 and lat <= 90 and lon >= -180 and lon <= 180 do
-          Logger.info("IP geolocation: Parsed coordinates lat=#{lat}, lon=#{lon}")
-          {:ok, %{"lat" => lat / 1.0, "lng" => lon / 1.0}}
-        else
-          Logger.warning("IP geolocation: Invalid coordinates lat=#{lat}, lon=#{lon}")
-          {:error, :invalid_coordinates}
-        end
-
-      %{"status" => "fail", "message" => message} ->
-        Logger.warning("IP geolocation: API returned failure: #{message}")
-        {:error, :api_failure}
-
-      data ->
-        Logger.warning("IP geolocation: Unexpected response format: #{inspect(data)}")
-        {:error, :api_failure}
-    end
-  end
-
-  defp parse_ip_api_response(body) when is_binary(body) do
-    case Jason.decode(body) do
-      {:ok, decoded} -> parse_ip_api_response(decoded)
-      {:error, _} -> {:error, :json_decode_error}
-    end
-  end
-
-  defp parse_ip_api_response(_), do: {:error, :invalid_response}
 end
diff --git a/test/aprsme_web/plugs/ip_geolocation_test.exs b/test/aprsme_web/plugs/ip_geolocation_test.exs
index f813e7c..cc92b82 100644
--- a/test/aprsme_web/plugs/ip_geolocation_test.exs
+++ b/test/aprsme_web/plugs/ip_geolocation_test.exs
@@ -3,11 +3,7 @@ defmodule AprsmeWeb.Plugs.IPGeolocationTest do
 
   alias AprsmeWeb.Plugs.IPGeolocation
 
-  # Test IP address
-  @test_ip {204, 110, 191, 254}
-
   setup %{conn: conn} do
-    # Initialize session for all tests
     conn =
       conn
       |> Plug.Session.call(Plug.Session.init(store: :cookie, key: "_aprs_key", signing_salt: "test"))
@@ -16,98 +12,48 @@ defmodule AprsmeWeb.Plugs.IPGeolocationTest do
     {:ok, conn: conn}
   end
 
-  describe "call/2" do
-    @tag :external_api
-    test "adds geolocation data to session when successful", %{conn: conn} do
+  describe "call/2 with Cloudflare headers" do
+    test "sets session from CF-IPLatitude and CF-IPLongitude headers on root GET", %{conn: conn} do
       conn =
         conn
-        |> Map.put(:remote_ip, @test_ip)
+        |> put_req_header("cf-iplatitude", "37.7749")
+        |> put_req_header("cf-iplongitude", "-122.4194")
         |> IPGeolocation.call([])
 
       geo = get_session(conn, :ip_geolocation)
-      assert is_map(geo)
-      assert is_number(geo["lat"])
-      assert is_number(geo["lng"])
-      # Test IP should geolocate to somewhere in the US
-      assert geo["lat"] >= 24 and geo["lat"] <= 49
-      assert geo["lng"] >= -125 and geo["lng"] <= -66
+      assert geo == %{"lat" => 37.7749, "lng" => -122.4194}
     end
 
-    test "skips geolocation for local IP addresses", %{conn: conn} do
+    test "skips when CF headers are missing", %{conn: conn} do
+      conn = IPGeolocation.call(conn, [])
+
+      assert get_session(conn, :ip_geolocation) == nil
+    end
+
+    test "skips when only latitude header is present", %{conn: conn} do
       conn =
         conn
-        |> Map.put(:remote_ip, {127, 0, 0, 1})
+        |> put_req_header("cf-iplatitude", "37.7749")
         |> IPGeolocation.call([])
 
       assert get_session(conn, :ip_geolocation) == nil
     end
 
-    test "skips geolocation for private IP addresses", %{conn: _conn} do
-      # Test various private IP ranges
-      private_ips = [
-        {10, 0, 0, 1},
-        {172, 16, 0, 1},
-        {192, 168, 1, 1}
-      ]
-
-      for ip <- private_ips do
-        conn =
-          build_conn()
-          |> Plug.Session.call(Plug.Session.init(store: :cookie, key: "_aprs_key", signing_salt: "test"))
-          |> fetch_session()
-          |> Map.put(:remote_ip, ip)
-          |> IPGeolocation.call([])
-
-        assert get_session(conn, :ip_geolocation) == nil
-      end
-    end
-
-    @tag :external_api
-    test "handles IPv6 addresses", %{conn: conn} do
+    test "skips when only longitude header is present", %{conn: conn} do
       conn =
         conn
-        |> Map.put(:remote_ip, {0x2001, 0x4860, 0x4860, 0, 0, 0, 0, 0x8888})
+        |> put_req_header("cf-iplongitude", "-122.4194")
         |> IPGeolocation.call([])
 
-      geo = get_session(conn, :ip_geolocation)
-      assert is_map(geo)
-      assert is_number(geo["lat"])
-      assert is_number(geo["lng"])
-    end
-
-    @tag :external_api
-    test "uses forwarded IP when behind proxy", %{conn: conn} do
-      conn =
-        conn
-        |> Map.put(:remote_ip, {10, 0, 0, 1})
-        |> put_req_header("x-forwarded-for", "204.110.191.254, 10.0.0.1")
-        |> IPGeolocation.call([])
-
-      geo = get_session(conn, :ip_geolocation)
-      assert is_map(geo)
-      assert is_number(geo["lat"])
-      assert is_number(geo["lng"])
-    end
-
-    test "caches geolocation in session", %{conn: conn} do
-      # Mock the geolocation data by setting it in session first
-      geo_data = %{"lat" => 37.4056, "lng" => -122.0775}
-
-      conn =
-        conn
-        |> put_session(:ip_geolocation, geo_data)
-        |> Map.put(:remote_ip, @test_ip)
-        |> IPGeolocation.call([])
-
-      # Should return the cached data without making an API call
-      assert get_session(conn, :ip_geolocation) == geo_data
+      assert get_session(conn, :ip_geolocation) == nil
     end
 
     test "skips non-root paths", %{conn: conn} do
       conn =
         conn
-        |> Map.put(:remote_ip, @test_ip)
         |> Map.put(:request_path, "/about")
+        |> put_req_header("cf-iplatitude", "37.7749")
+        |> put_req_header("cf-iplongitude", "-122.4194")
         |> IPGeolocation.call([])
 
       assert get_session(conn, :ip_geolocation) == nil
@@ -116,11 +62,66 @@ defmodule AprsmeWeb.Plugs.IPGeolocationTest do
     test "skips non-GET requests", %{conn: conn} do
       conn =
         conn
-        |> Map.put(:remote_ip, @test_ip)
         |> Map.put(:method, "POST")
+        |> put_req_header("cf-iplatitude", "37.7749")
+        |> put_req_header("cf-iplongitude", "-122.4194")
         |> IPGeolocation.call([])
 
       assert get_session(conn, :ip_geolocation) == nil
     end
+
+    test "uses cached session data without re-reading headers", %{conn: conn} do
+      geo_data = %{"lat" => 40.7128, "lng" => -74.006}
+
+      conn =
+        conn
+        |> put_session(:ip_geolocation, geo_data)
+        |> put_req_header("cf-iplatitude", "37.7749")
+        |> put_req_header("cf-iplongitude", "-122.4194")
+        |> IPGeolocation.call([])
+
+      assert get_session(conn, :ip_geolocation) == geo_data
+    end
+
+    test "rejects latitude out of range", %{conn: conn} do
+      conn =
+        conn
+        |> put_req_header("cf-iplatitude", "91.0")
+        |> put_req_header("cf-iplongitude", "-122.4194")
+        |> IPGeolocation.call([])
+
+      assert get_session(conn, :ip_geolocation) == nil
+    end
+
+    test "rejects longitude out of range", %{conn: conn} do
+      conn =
+        conn
+        |> put_req_header("cf-iplatitude", "37.7749")
+        |> put_req_header("cf-iplongitude", "181.0")
+        |> IPGeolocation.call([])
+
+      assert get_session(conn, :ip_geolocation) == nil
+    end
+
+    test "rejects non-numeric header values", %{conn: conn} do
+      conn =
+        conn
+        |> put_req_header("cf-iplatitude", "not_a_number")
+        |> put_req_header("cf-iplongitude", "-122.4194")
+        |> IPGeolocation.call([])
+
+      assert get_session(conn, :ip_geolocation) == nil
+    end
+
+    test "handles boundary coordinate values", %{conn: conn} do
+      conn =
+        conn
+        |> put_req_header("cf-iplatitude", "-90.0")
+        |> put_req_header("cf-iplongitude", "180.0")
+        |> IPGeolocation.call([])
+
+      geo = get_session(conn, :ip_geolocation)
+      assert geo == %{"lat" => -90.0, "lng" => 180.0}
+    end
   end
 end