fix: Unify CORS header handling (#1874)

Fixes #1824

Moves the CORS header logic to a utility and at the router level so we
can define the allowed methods for each endpoint.

Defining it in each endpoint is counterintuitive as for all
`/v1/shape/:root_table` endpoints we should be returning the same
allowed headers.

.changeset/flat-melons-attack.md

@@ -0,0 +1,5 @@
+---
+"@core/sync-service": patch
+---
+
+Unify CORS header handling to ensure they are always present

packages/sync-service/lib/electric/plug/options_shape_plug.ex

@@ -2,18 +2,13 @@ defmodule Electric.Plug.OptionsShapePlug do
   require Logger
   use Plug.Builder
 
-  plug :add_allowed_methods_header
   plug :add_cache_max_age_header
   plug :filter_cors_headers
-  plug :add_allow_origin_header
   plug :add_keep_alive
   plug :send_options_response
 
   @allowed_headers ["if-none-match"]
 
-  defp add_allowed_methods_header(conn, _),
-    do: Plug.Conn.put_resp_header(conn, "access-control-allow-methods", "GET, OPTIONS, DELETE")
-
   defp add_cache_max_age_header(conn, _) do
     conn
     |> Plug.Conn.put_resp_header("access-control-max-age", "86400")
@@ -39,14 +34,6 @@ defmodule Electric.Plug.OptionsShapePlug do
     end
   end
 
-  # Electric currently allows any origin to access the API
-  defp add_allow_origin_header(conn, _) do
-    case Plug.Conn.get_req_header(conn, "origin") do
-      [origin] -> Plug.Conn.put_resp_header(conn, "access-control-allow-origin", origin)
-      _ -> conn
-    end
-  end
-
   defp add_keep_alive(conn, _), do: Plug.Conn.put_resp_header(conn, "connection", "keep-alive")
 
   defp send_options_response(conn, _), do: send_resp(conn, 204, "")

packages/sync-service/lib/electric/plug/router.ex

@@ -1,5 +1,6 @@
 defmodule Electric.Plug.Router do
   use Plug.Router, copy_opts_to_assign: :config
+  alias Electric.Plug.Utils.CORSHeaderPlug
 
   plug Plug.RequestId, assign_as: :plug_request_id
   plug :server_header, Electric.version()
@@ -9,6 +10,7 @@ defmodule Electric.Plug.Router do
   plug Electric.Plug.LabelProcessPlug
   plug Plug.Telemetry, event_prefix: [:electric, :routing]
   plug Plug.Logger
+  plug :put_cors_headers
   plug :dispatch
 
   match "/", via: [:get, :head], do: send_resp(conn, 200, "")
@@ -19,12 +21,15 @@ defmodule Electric.Plug.Router do
 
   get "/v1/health", to: Electric.Plug.HealthCheckPlug
 
-  match _ do
-    send_resp(conn, 404, "Not found")
-  end
+  match _,
+    do: send_resp(conn, 404, "Not found")
 
-  def server_header(conn, version) do
-    conn
-    |> Plug.Conn.put_resp_header("server", "ElectricSQL/#{version}")
-  end
+  def server_header(conn, version),
+    do: conn |> Plug.Conn.put_resp_header("server", "ElectricSQL/#{version}")
+
+  def put_cors_headers(%Plug.Conn{path_info: ["v1", "shape", _ | _]} = conn, _opts),
+    do: CORSHeaderPlug.call(conn, %{methods: ["GET", "HEAD", "DELETE", "OPTIONS"]})
+
+  def put_cors_headers(conn, _opts),
+    do: CORSHeaderPlug.call(conn, %{methods: ["GET", "HEAD"]})
 end

packages/sync-service/lib/electric/plug/serve_shape_plug.ex

@@ -173,8 +173,6 @@ defmodule Electric.Plug.ServeShapePlug do
 
   # start_telemetry_span needs to always be the first plug after fetching query params.
   plug :start_telemetry_span
-
-  plug :cors
   plug :put_resp_content_type, "application/json"
   plug :validate_query_params
   plug :load_shape_info
@@ -417,13 +415,6 @@ defmodule Electric.Plug.ServeShapePlug do
         "public, max-age=#{config[:max_age]}, stale-while-revalidate=#{config[:stale_age]}"
       )
 
-  def cors(conn, _opts) do
-    conn
-    |> put_resp_header("access-control-allow-origin", "*")
-    |> put_resp_header("access-control-expose-headers", "*")
-    |> put_resp_header("access-control-allow-methods", "GET, POST, OPTIONS")
-  end
-
   # If offset is -1, we're serving a snapshot
   defp serve_log_or_snapshot(%Conn{assigns: %{offset: @before_all_offset}} = conn, _) do
     OpenTelemetry.with_span("shape_get.plug.serve_snapshot", [], fn -> serve_snapshot(conn) end)

packages/sync-service/lib/electric/plug/utils.ex

@@ -46,4 +46,30 @@ defmodule Electric.Plug.Utils do
       end
     end)
   end
+
+  defmodule CORSHeaderPlug do
+    @behaviour Plug
+    import Plug.Conn
+    def init(opts), do: opts
+
+    def call(conn, opts),
+      do:
+        conn
+        |> put_resp_header("access-control-allow-origin", get_allowed_origin(conn, opts))
+        |> put_resp_header("access-control-expose-headers", "*")
+        |> put_resp_header("access-control-allow-methods", get_allowed_methods(conn, opts))
+
+    defp get_allowed_methods(_conn, opts), do: Access.get(opts, :methods, []) |> Enum.join(", ")
+
+    defp get_allowed_origin(conn, opts) do
+      Access.get(
+        opts,
+        :origin,
+        case Plug.Conn.get_req_header(conn, "origin") do
+          [origin] -> origin
+          [] -> "*"
+        end
+      )
+    end
+  end
 end

packages/sync-service/test/electric/plug/options_shape_plug_test.exs

@@ -4,30 +4,20 @@ defmodule Electric.Plug.OptionsShapePlugTest do
   alias Electric.Plug.OptionsShapePlug
 
   @registry Registry.OptionsShapePlugTest
-  @expected_allowed_methods MapSet.new(["GET", "OPTIONS", "DELETE"])
 
   setup do
     start_link_supervised!({Registry, keys: :duplicate, name: @registry})
     :ok
   end
 
   describe "OptionsShapePlug" do
-    test "returns allowed methods" do
+    test "returns relevant headers" do
       conn =
         Plug.Test.conn("OPTIONS", "/?root_table=foo")
         |> OptionsShapePlug.call([])
 
       assert conn.status == 204
 
-      allowed_methods =
-        conn
-        |> Plug.Conn.get_resp_header("access-control-allow-methods")
-        |> List.first("")
-        |> String.split(",")
-        |> Enum.map(&String.trim/1)
-        |> MapSet.new()
-
-      assert allowed_methods == @expected_allowed_methods
       assert Plug.Conn.get_resp_header(conn, "access-control-max-age") == ["86400"]
       assert Plug.Conn.get_resp_header(conn, "connection") == ["keep-alive"]
     end
@@ -43,18 +33,5 @@ defmodule Electric.Plug.OptionsShapePlugTest do
       assert conn.status == 204
       assert Plug.Conn.get_resp_header(conn, "access-control-allow-headers") == [header]
     end
-
-    test "handles origin header" do
-      origin = "https://example.com"
-
-      conn =
-        Plug.Test.conn("OPTIONS", "/?root_table=foo")
-        # also checks that it is case insensitive
-        |> Plug.Conn.put_req_header("origin", origin)
-        |> OptionsShapePlug.call([])
-
-      assert conn.status == 204
-      assert Plug.Conn.get_resp_header(conn, "access-control-allow-origin") == [origin]
-    end
   end
 end

packages/sync-service/test/electric/plug/router_test.exs

@@ -861,7 +861,27 @@ defmodule Electric.Plug.RouterTest do
         |> Enum.map(&String.trim/1)
         |> MapSet.new()
 
-      assert allowed_methods == MapSet.new(["GET", "OPTIONS", "DELETE"])
+      assert allowed_methods == MapSet.new(["GET", "HEAD", "OPTIONS", "DELETE"])
+    end
+  end
+
+  describe "404" do
+    test "GET on invalid path returns 404", _ do
+      conn =
+        conn("GET", "/invalidpath")
+        |> Router.call([])
+
+      assert %{status: 404} = conn
+
+      allowed_methods =
+        conn
+        |> Plug.Conn.get_resp_header("access-control-allow-methods")
+        |> List.first("")
+        |> String.split(",")
+        |> Enum.map(&String.trim/1)
+        |> MapSet.new()
+
+      assert allowed_methods == MapSet.new(["GET", "HEAD"])
     end
   end