From 8d9d3856832c9edb70fcc3e98229644d753e25f4 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Thu, 12 Feb 2026 12:53:32 -0600 Subject: [PATCH] fix: prevent AlreadySentError in BruteForceProtection plug The plug was attempting to register a before_send callback after blocking a banned IP and sending a 403 response. This caused Plug.Conn.AlreadySentError in production. Fixed by checking conn.halted before attempting to register the 404 tracking callback. If the request was already blocked and sent, we skip the callback registration. Also updated the test to verify proper behavior without the workaround for AlreadySentError. --- .../plugs/brute_force_protection.ex | 12 ++++++-- .../plugs/brute_force_protection_test.exs | 29 ++++--------------- 2 files changed, 15 insertions(+), 26 deletions(-) diff --git a/lib/towerops_web/plugs/brute_force_protection.ex b/lib/towerops_web/plugs/brute_force_protection.ex index 61d354c5..13d960de 100644 --- a/lib/towerops_web/plugs/brute_force_protection.ex +++ b/lib/towerops_web/plugs/brute_force_protection.ex @@ -40,9 +40,15 @@ defmodule ToweropsWeb.Plugs.BruteForceProtection do ip_address = get_remote_ip(conn) # Check whitelist and ban status before routing - conn - |> check_and_block(ip_address) - |> maybe_track_404(ip_address) + conn = check_and_block(conn, ip_address) + + # Only register 404 tracking if we didn't already halt the request + # (halted requests have already been sent, can't register before_send) + if conn.halted do + conn + else + maybe_track_404(conn, ip_address) + end end defp check_and_block(conn, ip_address) do diff --git a/test/towerops_web/plugs/brute_force_protection_test.exs b/test/towerops_web/plugs/brute_force_protection_test.exs index 4fda1dee..cd14efe1 100644 --- a/test/towerops_web/plugs/brute_force_protection_test.exs +++ b/test/towerops_web/plugs/brute_force_protection_test.exs @@ -87,37 +87,20 @@ defmodule ToweropsWeb.Plugs.BruteForceProtectionTest do end describe "call/2 with banned IPs" do - # NOTE: The BruteForceProtection plug has a design issue where block_request/2 - # sends the response (setting conn.state to :sent) and then maybe_track_404/2 - # attempts register_before_send/2 which raises AlreadySentError on sent conns. - # - # This test verifies the blocking behavior by catching the AlreadySentError and - # inspecting the conn state after the block_request step completed. test "blocks requests from banned IPs with 403 and retry-after header" do ip = "198.51.100.#{[:positive] |> System.unique_integer() |> rem(255)}" # Create a ban for the IP (5 minute ban for first offense) {:ok, _block} = BruteForce.create_or_escalate_ban(ip, "Test ban") - # The plug will send 403, then raise AlreadySentError when trying to - # register before_send on the already-sent conn. conn = - try do - build_conn() - |> put_req_header("x-forwarded-for", ip) - |> BruteForceProtection.call([]) - rescue - Plug.Conn.AlreadySentError -> - # When AlreadySentError is raised, the response has been sent but - # we lose the conn reference. Verify via BruteForce context instead. - nil - end + build_conn() + |> put_req_header("x-forwarded-for", ip) + |> BruteForceProtection.call([]) - if conn do - # If no error was raised (future fix), verify directly - assert conn.halted - assert conn.status == 403 - end + assert conn.halted + assert conn.status == 403 + assert get_resp_header(conn, "retry-after") != [] # Verify the IP is actually banned via the context assert {:blocked, _banned_until} = BruteForce.check_ban_status(ip)