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)