fix: intermittent webhook conn malfunctions (#53)

Author: zcesur

lib/algora/integrations/github/webhook.ex

@@ -22,16 +22,20 @@ defmodule Algora.Github.Webhook do
 
   defstruct @enforce_keys
 
-  def new(conn, payload) do
+  def new(conn) do
     secret = Algora.Github.webhook_secret()
 
     with {:ok, headers} <- parse_headers(conn),
-         {:ok, _} <- verify_signature(headers.signature_256, conn.assigns[:raw_body], secret) do
-      build_webhook(headers, payload)
+         {:ok, payload, conn} = Plug.Conn.read_body(conn),
+         {:ok, _} <- verify_signature(headers.signature_256, payload, secret),
+         {:ok, webhook} <- build_webhook(headers, payload) do
+      {:ok, webhook, conn}
     end
   end
 
   defp build_webhook(headers, payload) do
+    payload = Jason.decode!(payload)
+
     params =
       headers
       |> Map.put(:payload, payload)

lib/algora_web/controllers/webhooks/github_controller.ex

@@ -19,34 +19,6 @@ defmodule AlgoraWeb.Webhooks.GithubController do
   # TODO: persist & alert about failed deliveries
   # TODO: auto-retry failed deliveries with exponential backoff
 
-  def new(conn, payload) do
-    with {:ok, webhook} <- Webhook.new(conn, payload),
-         :ok <- process_delivery(webhook) do
-      conn |> put_status(:accepted) |> json(%{status: "ok"})
-    else
-      {:error, :bot_event} ->
-        conn |> put_status(:ok) |> json(%{status: "ok"})
-
-      {:error, :missing_header} ->
-        conn |> put_status(:bad_request) |> json(%{error: "Missing header"})
-
-      {:error, :signature_mismatch} ->
-        conn |> put_status(:unauthorized) |> json(%{error: "Signature mismatch"})
-
-      {:error, reason} ->
-        Logger.error("Error processing webhook: #{inspect(reason)}")
-        conn |> put_status(:internal_server_error) |> json(%{error: "Internal server error"})
-
-      error ->
-        Logger.error("Error processing webhook: #{inspect(error)}")
-        conn |> put_status(:internal_server_error) |> json(%{error: "Internal server error"})
-    end
-  rescue
-    e ->
-      Logger.error(Exception.format(:error, e, __STACKTRACE__))
-      conn |> put_status(:internal_server_error) |> json(%{error: "Internal server error"})
-  end
-
   def process_delivery(webhook) do
     with :ok <- ensure_human_author(webhook),
          {:ok, commands} <- process_commands(webhook),

lib/algora_web/endpoint.ex

@@ -46,32 +46,9 @@ defmodule AlgoraWeb.Endpoint do
     handler: AlgoraWeb.Webhooks.StripeController,
     secret: {Algora, :config, [[:stripe, :webhook_secret]]}
 
-  plug :parse_body
-
-  opts = [
-    parsers: [:urlencoded, :multipart, :json],
-    pass: ["*/*"],
-    json_decoder: Phoenix.json_library()
-  ]
-
-  defmodule BodyReader do
-    @moduledoc false
-    def cache_raw_body(conn, opts) do
-      with {:ok, body, conn} <- Plug.Conn.read_body(conn, opts) do
-        conn = update_in(conn.assigns[:raw_body], &[body | &1 || []])
-
-        {:ok, body, conn}
-      end
-    end
-  end
-
-  @parser_without_cache Plug.Parsers.init(opts)
-  @parser_with_cache Plug.Parsers.init([body_reader: {BodyReader, :cache_raw_body, []}] ++ opts)
-
-  # All endpoints that start with "webhooks" have their body cached.
-  defp parse_body(%{path_info: ["webhooks" | _]} = conn, _), do: Plug.Parsers.call(conn, @parser_with_cache)
-
-  defp parse_body(conn, _), do: Plug.Parsers.call(conn, @parser_without_cache)
+  plug AlgoraWeb.Plugs.GithubWebhooks,
+    at: "/webhooks/github",
+    handler: AlgoraWeb.Webhooks.GithubController
 
   plug Plug.MethodOverride
   plug Plug.Head

lib/algora_web/plugs/github_webhooks.ex

@@ -0,0 +1,56 @@
+defmodule AlgoraWeb.Plugs.GithubWebhooks do
+  @moduledoc false
+  @behaviour Plug
+
+  import Plug.Conn
+
+  alias Algora.Github.Webhook
+  alias AlgoraWeb.Webhooks.GithubController
+  alias Plug.Conn
+
+  require Logger
+
+  @impl true
+  def init(opts) do
+    path_info = String.split(opts[:at], "/", trim: true)
+
+    opts
+    |> Map.new()
+    |> Map.put_new(:path_info, path_info)
+  end
+
+  @impl true
+  def call(%Conn{method: "POST", path_info: path_info} = conn, %{path_info: path_info} = _opts) do
+    with {:ok, webhook, conn} <- Webhook.new(conn),
+         :ok <- GithubController.process_delivery(webhook) do
+      conn |> send_resp(200, "Webhook received.") |> halt()
+    else
+      {:error, :bot_event} ->
+        conn |> send_resp(200, "Webhook received.") |> halt()
+
+      {:error, :missing_header} ->
+        Logger.error("Missing header")
+        conn |> send_resp(400, "Bad request.") |> halt()
+
+      {:error, :signature_mismatch} ->
+        Logger.error("Signature mismatch")
+        conn |> send_resp(400, "Bad request.") |> halt()
+
+      error ->
+        Logger.error("Bad request: #{inspect(error)}")
+        conn |> send_resp(400, "Bad request.") |> halt()
+    end
+  rescue
+    e ->
+      Logger.error(Exception.format(:error, e, __STACKTRACE__))
+      conn |> send_resp(400, "Bad request.") |> halt()
+  end
+
+  @impl true
+  def call(%Conn{path_info: path_info} = conn, %{path_info: path_info}) do
+    conn |> send_resp(400, "Bad request.") |> halt()
+  end
+
+  @impl true
+  def call(conn, _), do: conn
+end

lib/algora_web/router.ex

@@ -20,12 +20,6 @@ defmodule AlgoraWeb.Router do
     plug :accepts, ["json"]
   end
 
-  scope "/webhooks", AlgoraWeb do
-    pipe_through :api
-
-    post "/github", Webhooks.GithubController, :new
-  end
-
   scope "/", AlgoraWeb do
     pipe_through [:browser]