feat(frontend): add banned-IPs admin UI by bihius · Pull Request #284 · bihius/guard-proxy

bihius · 2026-07-22T10:40:19Z

Summary

New dashboard StatCard showing the current count of actively banned IPs (tone error).
New /banned-ips admin page: table (IP, domain, violations, expires-in) with client-side search and pagination, plus an admin-gated Unban action.
New use-banned-ips hook and banned-ips feature API layer wrapping GET/DELETE /security/banned-ips.
Nav entry and route wiring for the new page.

Note: the GET /security/banned-ips endpoint returns all tracked stick-table entries (including ones not yet past the ban threshold) without server-side pagination. The list and StatCard filter to banned === true client-side, and pagination/search are done client-side accordingly.

Test plan

pnpm run type-check
pnpm run lint
pnpm run test (124 passed, including new hook/API/page tests)
pnpm run build

Closes #277
Related to #176

Surfaces the banned-IP data from the security router: a dashboard StatCard, a dedicated /banned-ips page (table + client-side pagination and search), and manual unban action, admin-gated.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

bihius · 2026-07-22T10:47:20Z

+            <DataTable
+              columns={columns}
+              rows={items}
+              getRowKey={(row) => row.ip}


[bug] getRowKey={(row) => row.ip} is not unique. Backend list rows are per (vhost_id, ip) — the same attacker IP can appear once per ban table. Duplicate React keys cause unstable rendering (wrong domain/actions can glue to the wrong row) and make multi-domain bans hard to reason about.

Suggestion: Use a composite key such as `${row.vhost_id}:${row.ip}` (or ${row.domain}:${row.ip}). Keep Unban keyed by ip only, since the DELETE API is IP-scoped globally.

bihius · 2026-07-22T10:47:20Z


+    listBannedIps(accessToken, controller.signal)
+      .then((response) =>
+        guard(setBannedIps)(ok(response.items.filter((item) => item.banned).length)),


[suggestion] The StatCard counts response.items.filter((item) => item.banned).length, i.e. ban entries (one per vhost stick-table row), not unique source IPs. Label is "Banned IPs". If one IP is banned on three vhosts, the card shows 3 while operators may read that as three distinct attackers. The list page's "N banned IPs" footer has the same semantics.

Suggestion: Either count unique ip values for the card/footer, or rename labels to "Ban entries" / "Active bans" so the metric matches the data model.

bihius · 2026-07-22T10:47:20Z

+    setIsLoading(true);
+    setError(null);
+
+    listBannedIps(accessToken, controller.signal)


[bug] listBannedIps hits GET /security/banned-ips, which is require_admin on the backend (viewers get 403 — see src/backend/app/routers/security.py and test_list_banned_ips_forbidden_for_viewer). The new page, nav item, and dashboard StatCard are still exposed to all authenticated users. Viewers always land on the error state for /banned-ips, and the dashboard "Banned IPs" card always resolves to "—". The page test that mocks a successful list for a non-admin (hasRole false) further hides this contract mismatch.

Suggestion: Either (a) gate nav + route (and optionally the StatCard) to hasRole("admin") and redirect others to Forbidden, matching the backend today, or (b) open GET /security/banned-ips to any authenticated user in a follow-up backend change if product wants viewers to read the list. Until one of those lands, do not present an always-broken surface to viewers.

bihius · 2026-07-22T10:47:20Z

+    const response = await unbanIp("token", "203.0.113.10");
+
+    expect(response).toEqual({ ip: "203.0.113.10", cleared: 1 });
+    expect(fetchMock.mock.calls[0]?.[0]).toBe("/api/v1/security/banned-ips/203.0.113.10");


[nit] Encoding coverage only asserts a simple IPv4 path segment. unbanIp correctly uses encodeURIComponent, which matters for IPv6 (: → %3A), but there is no regression test for that case.

Suggestion: Add one case with an IPv6 literal (e.g. 2001:db8::1) expecting the encoded path. Low priority.

bihius · 2026-07-22T10:47:20Z

+      <p className="text-sm text-foreground">
+        Are you sure you want to unban{" "}
+        <span className="font-mono font-semibold text-foreground">{bannedIp.ip}</span>{" "}
+        on <span className="font-semibold text-foreground">{bannedIp.domain}</span>?


[bug] The confirmation copy says the IP will be unbanned "on {domain}", but DELETE /security/banned-ips/{ip} clears that key from every active ban stick-table (BanListService.unban loops all auto-ban-enabled vhosts). An admin who unbans from a row for app.example.com will also clear the same IP on other domains. That is a correctness/UX safety issue: the dialog understates the blast radius.

Suggestion: Reword to match the API, e.g. "Unban {ip} across all virtual hosts?" Optionally mention the source row's domain as context ("seen on {domain}") without implying scope. If product ever needs per-vhost unban, the backend contract would need to change first.

bihius · 2026-07-22T10:47:20Z

+    await waitFor(() => expect(screen.getByText(/no banned ips/i)).toBeInTheDocument());
+  });
+
+  it("admin sees the Unban action, viewer does not", async () => {


[suggestion] The test "admin sees the Unban action, viewer does not" mocks a successful listBannedIps for a non-admin. That encodes the product assumption that viewers can read the ban list, which contradicts the current admin-only GET. The suite will keep passing even though production viewers never reach a populated table.

Suggestion: Align the test with the chosen RBAC fix from Issue 3: either assert a Forbidden/redirect/error path for viewers, or (if GET is opened to viewers) add an integration/backend change and keep this UI assertion.

bihius · 2026-07-22T10:47:20Z

+              : String(stats.bannedIps.count)
+          }
+          tone="error"
+          icon={<ShieldIcon />}


[nit] "Banned IPs" reuses ShieldIcon, which is already used for "Active policies". Five error/success-adjacent cards with a repeated icon reduce scanability on the denser xl:grid-cols-5 row.

Suggestion: Prefer a distinct icon (e.g. ban/slash-circle style) if one exists in the local icon set, or accept as-is for MVP.

bihius · 2026-07-22T10:47:20Z

+
+  const total = filtered.length;
+  const start = (page - 1) * PAGE_SIZE;
+  const items = filtered.slice(start, start + PAGE_SIZE);


[suggestion] After unban/refresh, page is not clamped when the filtered list shrinks. With PAGE_SIZE = 50 this is uncommon, but if the user is on page 2+ and the last items on that page are unbanned (or expire), items becomes [] while total > 0 on earlier pages — the table shows the empty state ("No banned IPs") even though bans still exist, and the footer can show "Page 2 of 1".

Suggestion: After computing total / totalPages, clamp page (e.g. Math.min(page, totalPages) via effect or when applying the slice). Mirror the search path that already resets to page 1.

- Gate the /banned-ips route, nav entry, and dashboard StatCard to admins (RequireAdmin guard), matching the admin-only backend endpoint. - Use a composite (vhost_id:ip) row key since an IP can be banned per vhost. - Reword the unban dialog to reflect that DELETE clears the IP across all virtual hosts, not just the row's domain. - Count unique source IPs in the StatCard; label the list footer as entries. - Clamp the client-side page so a shrinking list can't strand an empty page. - Add a distinct BanIcon and an IPv6 unban-encoding test.

seed_runtime_config() never recorded a RuntimeOperation(reload) row, so after any backend restart /runtime/status had nothing to compare the freshly generated checksum against. The frontend's Apply-config gate (checksum mismatch => show button) read that as "always pending", showing the button even with zero unapplied changes. Reconcile the operation log against whatever config is actually active in `current` on every startup, not just the first, and consolidate the checksum calculation (previously duplicated) into one function in config_apply.

Enabling auto-ban+DDoS protection on a policy assigned to a vhost creates a DB record immediately, but the corresponding HAProxy stick-table only exists after "Apply config" runs. Until then, the Runtime API replies "No such table", which was indistinguishable from a genuinely unreachable socket and hard-failed the whole Banned IPs page with a 502. Split RuntimeApiError into RuntimeSocketUnreachableError (real outage) and RuntimeTableNotProvisionedError (expected transient state), and skip the latter per-table without counting it as a failure.

useRuntimeStatus only fetched once when the navbar mounted, so saving a policy or vhost elsewhere in the app left the Apply-config gate looking at stale data — the button only appeared after a full page reload. Add a ConfigChangedProvider (same pub/sub pattern as ApplyNoticeProvider) that lets every policy/vhost/rule mutation site notify the navbar to refresh its runtime status immediately.

…abled Rate limit and ban fields were always rendered (just grayed out) in the policy form, which read as clutter and implied settings that weren't actually active. They now only render once DDoS protection / automatic IP banning is checked. Disabling DDoS protection also resets the auto-ban toggle so it can't stay enabled invisibly.

feat(frontend): add banned-IPs admin UI

476148c

Surfaces the banned-IP data from the security router: a dashboard StatCard, a dedicated /banned-ips page (table + client-side pagination and search), and manual unban action, admin-gated.

Copilot AI review requested due to automatic review settings July 22, 2026 10:40

Copilot AI reviewed Jul 22, 2026

bihius commented Jul 22, 2026

View reviewed changes

bihius added 5 commits July 22, 2026 13:25

bihius merged commit bfb89ba into main Jul 27, 2026
3 checks passed

bihius deleted the feat/banned-ips-admin-ui branch July 27, 2026 19:45

Navigation Menu

feat(frontend): add banned-IPs admin UI - #284

feat(frontend): add banned-IPs admin UI#284
bihius merged 6 commits into
mainfrom
feat/banned-ips-admin-ui

bihius commented Jul 22, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

bihius Jul 22, 2026

Uh oh!

bihius Jul 22, 2026

Uh oh!

bihius Jul 22, 2026

Uh oh!

bihius Jul 22, 2026

Uh oh!

bihius Jul 22, 2026

Uh oh!

bihius Jul 22, 2026

Uh oh!

bihius Jul 22, 2026

Uh oh!

bihius Jul 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

bihius commented Jul 22, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants