From 47cc960d107853b777a0e1ad55d21b2178a11590 Mon Sep 17 00:00:00 2001 From: zafer Date: Fri, 21 Feb 2025 00:45:26 +0300 Subject: [PATCH 1/2] fix intermittent webhook conn malfunctions --- lib/algora/integrations/github/webhook.ex | 8 +++-- .../controllers/webhooks/github_controller.ex | 24 ++++++------- lib/algora_web/endpoint.ex | 29 ++------------- lib/algora_web/plugs/github_webhooks.ex | 36 +++++++++++++++++++ lib/algora_web/router.ex | 6 ---- 5 files changed, 54 insertions(+), 49 deletions(-) create mode 100644 lib/algora_web/plugs/github_webhooks.ex diff --git a/lib/algora/integrations/github/webhook.ex b/lib/algora/integrations/github/webhook.ex index 773c22277..860a0cc92 100644 --- a/lib/algora/integrations/github/webhook.ex +++ b/lib/algora/integrations/github/webhook.ex @@ -22,12 +22,14 @@ 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 diff --git a/lib/algora_web/controllers/webhooks/github_controller.ex b/lib/algora_web/controllers/webhooks/github_controller.ex index bfc9593c6..31e0fb7a4 100644 --- a/lib/algora_web/controllers/webhooks/github_controller.ex +++ b/lib/algora_web/controllers/webhooks/github_controller.ex @@ -19,32 +19,28 @@ 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 + def handle_webhook(webhook) do + case process_delivery(webhook) do + :ok -> + :ok + {:error, :bot_event} -> - conn |> put_status(:ok) |> json(%{status: "ok"}) + :ok {:error, :missing_header} -> - conn |> put_status(:bad_request) |> json(%{error: "Missing header"}) + {: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, "Signature mismatch"} error -> Logger.error("Error processing webhook: #{inspect(error)}") - conn |> put_status(:internal_server_error) |> json(%{error: "Internal server error"}) + {: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"}) + {:error, "Internal server error"} end def process_delivery(webhook) do diff --git a/lib/algora_web/endpoint.ex b/lib/algora_web/endpoint.ex index 5eceae756..03476b53a 100644 --- a/lib/algora_web/endpoint.ex +++ b/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 diff --git a/lib/algora_web/plugs/github_webhooks.ex b/lib/algora_web/plugs/github_webhooks.ex new file mode 100644 index 000000000..8d8e20805 --- /dev/null +++ b/lib/algora_web/plugs/github_webhooks.ex @@ -0,0 +1,36 @@ +defmodule AlgoraWeb.Plugs.GithubWebhooks do + @moduledoc false + @behaviour Plug + + import Plug.Conn + + alias Plug.Conn + + @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 + {:ok, webhook, conn} = Algora.Github.Webhook.new(conn) + + case AlgoraWeb.Webhooks.GithubController.handle_webhook(webhook) do + :ok -> conn |> send_resp(200, "Webhook received.") |> halt() + {:handle_error, reason} -> conn |> send_resp(400, reason) |> halt() + _ -> conn |> send_resp(400, "Bad request.") |> halt() + end + 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 diff --git a/lib/algora_web/router.ex b/lib/algora_web/router.ex index 79bef9a45..e682928f4 100644 --- a/lib/algora_web/router.ex +++ b/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] From 8a1c6d2a7c49fb9bd77be4a30a81bcf629b39676 Mon Sep 17 00:00:00 2001 From: zafer Date: Fri, 21 Feb 2025 18:32:46 +0300 Subject: [PATCH 2/2] fix some issues --- lib/algora/integrations/github/webhook.ex | 2 ++ .../controllers/webhooks/github_controller.ex | 24 -------------- lib/algora_web/plugs/github_webhooks.ex | 32 +++++++++++++++---- 3 files changed, 28 insertions(+), 30 deletions(-) diff --git a/lib/algora/integrations/github/webhook.ex b/lib/algora/integrations/github/webhook.ex index 860a0cc92..a6e233f85 100644 --- a/lib/algora/integrations/github/webhook.ex +++ b/lib/algora/integrations/github/webhook.ex @@ -34,6 +34,8 @@ defmodule Algora.Github.Webhook do end defp build_webhook(headers, payload) do + payload = Jason.decode!(payload) + params = headers |> Map.put(:payload, payload) diff --git a/lib/algora_web/controllers/webhooks/github_controller.ex b/lib/algora_web/controllers/webhooks/github_controller.ex index 31e0fb7a4..2d447daf8 100644 --- a/lib/algora_web/controllers/webhooks/github_controller.ex +++ b/lib/algora_web/controllers/webhooks/github_controller.ex @@ -19,30 +19,6 @@ defmodule AlgoraWeb.Webhooks.GithubController do # TODO: persist & alert about failed deliveries # TODO: auto-retry failed deliveries with exponential backoff - def handle_webhook(webhook) do - case process_delivery(webhook) do - :ok -> - :ok - - {:error, :bot_event} -> - :ok - - {:error, :missing_header} -> - {:error, "Missing header"} - - {:error, :signature_mismatch} -> - {:error, "Signature mismatch"} - - error -> - Logger.error("Error processing webhook: #{inspect(error)}") - {:error, "Internal server error"} - end - rescue - e -> - Logger.error(Exception.format(:error, e, __STACKTRACE__)) - {:error, "Internal server error"} - end - def process_delivery(webhook) do with :ok <- ensure_human_author(webhook), {:ok, commands} <- process_commands(webhook), diff --git a/lib/algora_web/plugs/github_webhooks.ex b/lib/algora_web/plugs/github_webhooks.ex index 8d8e20805..11cfa3f0b 100644 --- a/lib/algora_web/plugs/github_webhooks.ex +++ b/lib/algora_web/plugs/github_webhooks.ex @@ -4,8 +4,12 @@ defmodule AlgoraWeb.Plugs.GithubWebhooks do 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) @@ -17,13 +21,29 @@ defmodule AlgoraWeb.Plugs.GithubWebhooks do @impl true def call(%Conn{method: "POST", path_info: path_info} = conn, %{path_info: path_info} = _opts) do - {:ok, webhook, conn} = Algora.Github.Webhook.new(conn) - - case AlgoraWeb.Webhooks.GithubController.handle_webhook(webhook) do - :ok -> conn |> send_resp(200, "Webhook received.") |> halt() - {:handle_error, reason} -> conn |> send_resp(400, reason) |> halt() - _ -> conn |> send_resp(400, "Bad request.") |> halt() + 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