From 3e3578f4d8af8b25d8fd27c067cb0268c0c2ea43 Mon Sep 17 00:00:00 2001 From: Lukas Date: Wed, 22 Apr 2026 21:05:41 +0200 Subject: [PATCH] Update task list and add runner script - Updated Tasks.md with refined task tracking format - Added runner.csx script for automated task processing with Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Docs/Tasks.md | 1132 ++++++++++++++++++++++++----------------------- Docs/runner.csx | 127 ++++++ 2 files changed, 717 insertions(+), 542 deletions(-) create mode 100644 Docs/runner.csx diff --git a/Docs/Tasks.md b/Docs/Tasks.md index 0649266..55a4aab 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,631 +1,679 @@ -# BanGUI — Task List -This document breaks the entire BanGUI project into development stages, ordered so that each stage builds on the previous one. Every task is described in prose with enough detail for a developer to begin work. References point to the relevant documentation. +### TASK-SEC-01 — Open Redirect in LoginPage via `?next=` Parameter -Reference: `Docs/Refactoring.md` for full analysis of each issue. +**Where found** +`frontend/src/pages/LoginPage.tsx` lines 77–103. After a successful login the code does `navigate(nextPath, { replace: true })` where `nextPath = searchParams.get("next") ?? "/"` with no validation of the value. `RequireAuth.tsx` sets the redirect with `to={`/login?next=${encodeURIComponent(...)}`}`, which only ever produces relative paths, but the parameter can be set by an attacker in a crafted link. + +**Goal** +Validate that `nextPath` is an internal path before using it. The check should verify the value starts with `/` and does not start with `//` (which browsers interpret as a protocol-relative URL). If the value fails validation fall back to `"/"`. A one-liner guard such as `const safePath = /^\/(?!\/)/.test(next) ? next : "/";` is sufficient. + +**Possible traps and issues** +- `//evil.com` passes a leading-slash check unless the double-slash case is excluded explicitly. +- `encodeURIComponent` in `RequireAuth` means the decoded value will always be a relative path under normal operation, so this fix will not break the legitimate redirect flow. +- If the app ever navigates to an external URL intentionally that logic must bypass this guard through a separate explicit code path, not the `?next=` parameter. + +**Docs changes needed** +Note the fix in `Docs/Refactoring.md` under a new "Security Fixes" section. + +**Why this is needed** +An attacker can send a user a link like `/login?next=https://evil.com`. After the user enters their password they are transparently redirected to an attacker-controlled site. This is a textbook open-redirect vulnerability and is listed in the OWASP Top 10 (A01:2021 Broken Access Control). --- -## Open Issues +### TASK-BUG-01 — Infinite Re-Fetch Loop in `useJailConfigs` -Issues are grouped by category and ordered roughly by severity. Each entry describes the current state, the desired end state, pitfalls to watch for, and which documentation needs updating when the task is done. +**Where found** +`frontend/src/hooks/useJailConfigs.ts` lines 33–40. The hook calls `useListData` with an inline `onSuccess` callback: `onSuccess: (response) => { setTotal(response.total); }`. This arrow function is a new object reference on every render. `useListData` lists `onSuccess` in the `useCallback` dependency array of its internal `refresh` function, so a new `onSuccess` → new `refresh` → `useEffect([refresh])` fires → fetch completes → re-render → new `onSuccess` → infinite loop. ---- - -## Bug Fixes / Correctness - ---- - -### TASK-001 — Race condition in `useJailBannedIps`: missing AbortController (done) - -**Where fixed:** `frontend/src/hooks/useJailBannedIps.ts`, `frontend/src/api/jails.ts` - -**Summary:** Added an `AbortController` ref to cancel stale fetches, passed the signal into `fetchJailBannedIps`, and abort on unmount. - -**Where found:** `frontend/src/hooks/useJailBannedIps.ts` — the `load` callback is `async` and calls `fetchJailBannedIps` with no AbortSignal. - -**Goal:** Add a `useRef` to `useJailBannedIps`. Before each fetch, abort the previous controller and create a new one. Pass `signal` to `fetchJailBannedIps`. In the `useEffect` cleanup return, abort the controller. After every `await`, check `signal.aborted` before calling any state setter. - -**Possible traps:** -- `fetchJailBannedIps` in `api/jails.ts` does not yet accept a signal — that parameter must be added first (see TASK-005). -- The hook uses a debounced search value; the debounce timer itself does not need to be cancelled, only the in-flight request. -- Setting state after abort causes a React warning in some versions; always guard with `if (!ctrl.signal.aborted)`. - -**Docs changes needed:** None. - -**Why:** Without cancellation, rapidly changing the search or page can cause an older, slower response to overwrite the result of a newer request. The user sees stale data with no indication it is stale. - ---- - -### TASK-002 — `HistoryPage` filter effect has a stale `appliedQuery` dependency (done) - -**Where found:** `frontend/src/pages/HistoryPage.tsx` lines 214–230. The `useEffect` lists `appliedQuery` as a dependency, reads it inside the effect, and then calls `setAppliedQuery` — which triggers the same effect again immediately. - -**Goal:** Remove `appliedQuery` from the effect dependency array and use a `useRef` to hold the last-applied query for comparison. The ref must be updated synchronously when the query changes so the comparison is always accurate without creating a circular dependency. - -**Possible traps:** -- The `areHistoryQueriesEqual` utility must still be called to avoid redundant fetches; moving it to a ref pattern requires care not to skip the equality check. -- Removing a dependency while keeping `// eslint-disable-next-line` comments is tempting but wrong; restructure so the lint rule is satisfied naturally. - -**Docs changes needed:** None. - -**Why:** The current code runs the effect one extra time on every filter change — once when filters change, and once because `appliedQuery` just changed. This causes two API calls per user interaction instead of one. - ---- - -### TASK-003 — `BanUnbanForm` floating promises and no double-submit guard (done) - -**Where fixed:** `frontend/src/pages/jails/BanUnbanForm.tsx`, `frontend/src/pages/jails/__tests__/BanUnbanForm.test.tsx` - -**Summary:** Converted ban and unban handlers to async functions with separate submit states and disabled submit buttons while requests are in flight. - -**Where found:** `frontend/src/pages/jails/BanUnbanForm.tsx` — `handleBan` and `handleUnban` are synchronous functions that call `onBan(…).then(…).catch(…)`. The returned promise is not awaited and is not assigned to anything. - -**Goal:** Convert `handleBan` and `handleUnban` to `async` functions. Add an `isSubmitting` state variable. Set it to `true` before the API call and `false` in a `finally` block. Disable both submit buttons while `isSubmitting` is true. The promise chain can then become a simple `try/catch/finally`. - -**Possible traps:** -- If the outer handler is passed to an `onClick` that is typed as `() => void`, TypeScript will not warn about the lost promise; ESLint rule `@typescript-eslint/no-misused-promises` (see TASK-030) would catch this. -- Two separate actions (ban and unban) share the same `isSubmitting` flag; use two flags (`isBanning`, `isUnbanning`) to allow one to proceed while the other is loading. - -**Docs changes needed:** None. - -**Why:** Without a double-submit guard, clicking "Ban" quickly fires multiple identical API requests. If the `.catch()` branch itself throws, the error is an unhandled rejection that surfaces as a console warning and is invisible to the user. - ---- - -### TASK-004 — `KVEditor` key rename silently overwrites duplicate keys (done) - -**Where fixed:** `frontend/src/components/config/KVEditor.tsx`, `frontend/src/components/config/configStyles.ts`, `frontend/src/components/config/__tests__/KVEditor.test.tsx` - -**Goal:** Before applying a key rename, check whether `newKey` already exists in `entries`. If it does, show a validation error inline (a `MessageBar` beneath the affected input row or a red border via `validationState="error"` on the Fluent UI `Input`). Block the `onChange` call until the conflict is resolved. - -**Possible traps:** -- The component is fully controlled (parent owns the `entries` object), so the error state must be local to `KVEditor` — adding it to the parent's state would be over-engineering. -- While the user is mid-edit (typing the new key name), partial names that match existing keys should not be flagged until the input loses focus, to avoid premature errors. - -**Docs changes needed:** None. - -**Why:** Silently dropping the value of an existing key when the user renames another key to the same name is destructive data loss with no warning. Config settings that share a key name are semantically invalid in fail2ban anyway. - ---- - -### TASK-005 — `SetupGuard` redirects to `/setup` when the backend is temporarily unreachable (done) - -**Where fixed:** `frontend/src/components/SetupGuard.tsx` - -**Where found:** `frontend/src/components/SetupGuard.tsx`. When `useSetup` returns `{ loading: false, status: null }` due to a network error, the guard treats this the same as "setup not completed" and redirects to `/setup`. - -**Goal:** Destructure `error` from `useSetup` in addition to `status` and `loading`. Add an error branch: if `error` is non-null and `status` is null, render an error card (Fluent UI `MessageBar` with a Retry button) instead of redirecting. Only redirect to `/setup` when `status` is definitively `{ completed: false }`. - -**Possible traps:** -- `useSetup` may need to be updated to expose an `error` field if it does not already. -- There is a brief window on initial load where both `loading` and `error` are false and `status` is null; this state must not trigger a redirect. - -**Docs changes needed:** None. - -**Why:** A transient network hiccup during app startup causes a setup-complete user to be dropped into the setup wizard and potentially overwrite their configuration. This is a silent data-integrity risk. - ---- - -## Security - ---- - -### TASK-006 — No 401 interceptor: expired sessions show broken pages instead of redirecting (done) - -**Where found:** `frontend/src/api/client.ts`, `request` function. All non-2xx responses including 401 are thrown as a generic `ApiError`. Consumers render "Failed to load…" messages instead of redirecting. - -**Goal:** After the `if (!response.ok)` check, add a dedicated branch: if `response.status === 401`, dispatch a custom DOM event (`bangui:session-expired`) before throwing. In `AuthProvider`, listen for this event with `window.addEventListener` and call the existing logout cleanup logic, then navigate to `/login`. The event approach avoids coupling the API client to React context. - -**Possible traps:** -- The event listener must be added and removed inside a `useEffect` in `AuthProvider` to avoid leaks. -- Pages that show a brief error flash before the redirect fires should have their error messages suppressed for `ApiError` with status 401 — wrap this in a shared utility (`isAuthError(err)`). -- If the backend returns 403 (forbidden) rather than 401 for an expired token, the interceptor must also handle that status. - -**Docs changes needed:** Update `Docs/Backend-Development.md` to document that 401 responses trigger client-side logout. - -**Why:** Currently a user with an expired session sees multiple "Failed to load" error boxes and must manually navigate to `/login`. This is confusing and the broken state can be mistaken for a backend outage. - ---- - -### TASK-007 — Setup page password validation too weak (done) - -**Where found:** `frontend/src/pages/SetupPage.tsx`, `validate()` function. Only `masterPassword.length < 8` is checked. - -**Goal:** Add minimum complexity rules: at least one uppercase letter, one number, and one special character (e.g. `!@#$%^&*()`). Show a password-strength indicator (a simple four-step progress bar is sufficient) that updates as the user types. Add a validation message per unmet rule rather than a single generic string. - -**Possible traps:** -- The complexity rules must match whatever the backend enforces; check `backend/app/routers/setup.py` for server-side validation and keep the two in sync. -- The "confirm password" field comparison must run after the strength check, not instead of it, so both errors can be shown simultaneously. - -**Docs changes needed:** Document the password policy in `Docs/Instructions.md`. - -**Why:** An 8-character minimum allows trivially weak passwords such as `12345678`. Because this is the single master credential for the entire application, it warrants stronger client-side guidance. - ---- - -## Performance - ---- - -### TASK-008 — `buildBanColumns` and `HISTORY_COLUMNS` recreated on every render (done) - -**Where fixed:** `frontend/src/components/BanTable.tsx`, `frontend/src/pages/HistoryPage.tsx` - -**Where found:** -- `frontend/src/components/BanTable.tsx` — `buildBanColumns(styles)` called unconditionally in the render body. -- `frontend/src/pages/HistoryPage.tsx` — `HISTORY_COLUMNS(onClickIp, styles)` called unconditionally in the render body. - -**Goal:** Wrap both column-definition arrays in `useMemo`. In `BanTable`, the dependency is `[styles]`. In `HistoryPage`, the dependency is `[styles]` — the `onClickIp` callback should be wrapped in `useCallback` first so it has a stable reference. - -**Possible traps:** -- Fluent UI `createTableColumn` creates objects that are compared by reference inside `DataGrid`; if the array is recreated on every render, `DataGrid` will re-render all cells even when data has not changed. -- The `styles` object from `makeStyles` is stable across renders (Fluent UI guarantees this), so `[styles]` as a dependency is effectively `[]` in practice. - -**Docs changes needed:** None. - -**Why:** Recreating column definitions every render triggers unnecessary cell re-renders in `DataGrid`, which is one of the heaviest Fluent UI components. On a page with 100 rows and 7 columns, this means 700 unnecessary cell renders per parent re-render. - ---- - -### TASK-009 — `resolveFluentToken` calls `getComputedStyle` on every render (done) - -**Where fixed:** `frontend/src/components/BanTrendChart.tsx`, `frontend/src/components/TopCountriesPieChart.tsx`, `frontend/src/components/TopCountriesBarChart.tsx`, `frontend/src/components/JailDistributionChart.tsx` - -**Summary:** Wrapped Fluent token resolution calls in `useMemo([])` in each chart component and added tests verifying token resolution is memoized across rerenders. - -**Where found:** `frontend/src/utils/chartTheme.ts`, `resolveFluentToken` function. Called 2–3 times per render in `BanTrendChart`, `TopCountriesPieChart`, `TopCountriesBarChart`, and `JailDistributionChart`. - -**Goal:** In each chart component that calls `resolveFluentToken`, wrap the calls in `useMemo` with an empty dependency array `[]` (the theme never changes at runtime in the current implementation). When dark mode support is added (TASK-015), the dependency should change to the active theme object. - -**Possible traps:** -- `resolveFluentToken` reads a CSS custom property from a DOM element; calling it at module level (outside a component) would read before the `FluentProvider` has injected its tokens, returning an empty string. -- The function itself should not be memoized globally — only the results per component, since each component may be mounted in a different theming context in tests. - -**Docs changes needed:** None. - -**Why:** `getComputedStyle` on every render is a forced style recalculation. On low-end hardware or when many charts are visible simultaneously, this measurably degrades frame rate. - ---- - -### TASK-010 — No code splitting: all pages bundled into the main chunk (done) - -**Where found:** `frontend/src/App.tsx` — all page imports are static (`import { DashboardPage } from "./pages/DashboardPage"`). `frontend/vite.config.ts` has no `build.rollupOptions.manualChunks`. - -**Goal:** Convert all seven page imports in `App.tsx` to `React.lazy(() => import(…))`. Wrap the `` block in `}>`. In `vite.config.ts`, add `manualChunks` to split `react`/`react-dom`, Fluent UI, Recharts, and the D3/TopoJSON geo stack into separate vendor chunks. - -**Possible traps:** -- `React.lazy` requires the module to have a default export; all current pages use named exports. Either add a re-export default in each page file, or use `import(…).then(m => ({ default: m.PageName }))` in the lazy call. -- The `` fallback renders during navigation; place it inside `` but outside `` so the spinner is not blocked by auth context loading. -- The D3/TopoJSON data (`world-atlas/countries-110m.json`) is a large JSON file (~100 KB gzipped) and should be placed in the `geo` chunk alongside `d3-geo` and `topojson-client`. - -**Docs changes needed:** None. - -**Why:** Every user who opens the dashboard must download, parse, and execute JavaScript for the map, config editor, and blocklist pages — even if they never visit those routes. Code splitting allows the initial load to be significantly smaller. - ---- - -### TASK-011 — No `React.memo` on any heavy component (done) - -**Where found:** Every component in `frontend/src/components/` — zero uses of `React.memo` exist in the codebase. - -**Goal:** Apply `React.memo` to the components whose props rarely change but whose render is expensive: `WorldMap`, `BanTrendChart`, `JailDistributionChart`, `TopCountriesPieChart`, `TopCountriesBarChart`, `BanTable`, and `ServerStatusBar`. Use the default shallow-equality comparator for props that are primitives. For `WorldMap`, provide a custom comparator that compares the `countries` record by key count and values (shallow object comparison). - -**Possible traps:** -- Inline function props (e.g. `onSelectCountry={() => …}`) will always be a new reference and defeat `React.memo`. Ensure all callback props passed to memoized components are wrapped in `useCallback` at the call site. -- Inline `style={{…}}` objects passed as props also defeat memoization; these must be moved to `makeStyles` first (see TASK-018). -- `React.memo` does not help if the component itself calls an expensive hook that always returns a new object. - -**Docs changes needed:** None. - -**Why:** `DashboardPage` re-renders whenever the 30-second server status poll fires, causing all five chart components and the ban table to re-render even though the filter state has not changed. - ---- - -### TASK-012 — `useMapData` sets `loading=true` before the debounce fires (done) - -**Where fixed:** `frontend/src/hooks/useMapData.ts`, `frontend/src/hooks/__tests__/useMapData.test.ts` - -**Where found:** `frontend/src/hooks/useMapData.ts`, `load` callback — `setLoading(true)` is called at the top of `load`, but the actual fetch is deferred inside a `setTimeout` of 300 ms. - -**Goal:** Move `setLoading(true)` inside the `setTimeout` callback, immediately before the `abortRef.current?.abort()` call. This ensures the spinner only appears when a real network request is about to start. - -**Possible traps:** -- The caller (`MapPage`) uses `loading` to fade the map table with `opacity: 0.5`. There will now be a 300 ms window where filters have changed but the table is still showing old data at full opacity; this is acceptable and more correct than showing a spinner immediately. -- If the component unmounts during the debounce window, the `clearTimeout` in the cleanup must run; verify the existing `useEffect` cleanup correctly returns `clearTimeout`. - -**Docs changes needed:** None. - -**Why:** Users see a loading spinner for 300 ms before any network activity has started, which makes the UI feel slower than it is. - ---- - -## Architecture / Code Quality - ---- - -### TASK-013 — Consolidate `api/config.ts` and `api/file_config.ts` duplicate functions (done) - -**Where fixed:** `frontend/src/api/config.ts`, `frontend/src/hooks/useFilterRawFile.ts`, `frontend/src/hooks/useActionRawFile.ts`, `frontend/src/components/config/JailFilesTab.tsx`, `frontend/src/components/config/ExportTab.tsx` - -**Where found:** Both `frontend/src/api/config.ts` and `frontend/src/api/file_config.ts` export identical functions: `fetchJailConfigFiles`, `createJailConfigFile`, `fetchJailConfigFileContent`, `updateJailConfigFile`, `setJailConfigFileEnabled`, `fetchFilterFiles`, `fetchFilterFile`, `updateFilterFile`, `createFilterFile`, `fetchActionFiles`, `fetchActionFile`, `updateActionFile`, `createActionFile`. - -**Goal:** Delete `api/file_config.ts`. Update the four files that import from it (`hooks/useFilterRawFile.ts`, `hooks/useActionRawFile.ts`, `components/config/JailFilesTab.tsx`, `components/config/ExportTab.tsx`) to import the same functions from `api/config.ts` instead. Verify that both modules currently export functions with identical signatures before the merge; resolve any differences first. - -**Possible traps:** -- `file_config.ts` lacks `AbortSignal` parameters on all its functions, while the corresponding functions in `config.ts` may have them. After migration, all callers must be updated to pass the signal. -- Running a global search for `from.*file_config` after the migration should return zero results; add this as a CI check via ESLint `no-restricted-imports` if desired. - -**Docs changes needed:** None. - -**Why:** Two modules exporting the same API functions will drift apart over time. Any bug fix or new feature applied to one will silently not apply to the other. - ---- - -### TASK-014 — Add `AbortSignal` to all API functions missing it (done) - -**Where found:** -- `frontend/src/api/dashboard.ts` — `fetchServerStatus`, `fetchBansByJail`, `fetchBanTrend` have no signal parameter. -- `frontend/src/api/jails.ts` — `fetchJailBannedIps` (and others) have no signal parameter. -- `frontend/src/api/blocklist.ts` — `fetchSchedule` (used by the polling hook) has no signal parameter. - -**Summary:** Added optional `signal` parameters to dashboard API functions and updated `useServerStatus`, `useBanTrend`, `useJailDistribution`, and `useBlocklistStatus` to pass abort signals from refs. Added tests to verify server status and blocklist schedule polling use `AbortSignal`. - -**Goal:** Add `signal?: AbortSignal` as the last parameter to every `get`/`post`/`put`/`del` wrapper call in these modules. The pattern is already established in `api/config.ts` and `api/map.ts` and should be replicated uniformly. After adding the parameters, update the consuming hooks to pass their `abortRef.current.signal`. - -**Possible traps:** -- Functions used by polling hooks (`fetchServerStatus` in `useServerStatus`, `fetchSchedule` in `useBlocklistStatus`) must pass the signal from a `useRef` rather than from a `useEffect`-local controller, because the poll fires outside the effect's lifecycle. -- Adding a signal parameter is non-breaking (it is optional), so no consumers need to change immediately; they can be updated incrementally. - -**Docs changes needed:** None. - -**Why:** Without a signal, requests fired by polling hooks cannot be cancelled when the component unmounts, leaking network activity and potentially updating state on unmounted components (React warning). - ---- - -### TASK-015 — Standardise AbortController pattern across all hooks (done) - -**Where fixed:** `frontend/src/hooks/useSetup.ts`, `frontend/src/hooks/useServerHealth.ts`, `frontend/src/api/setup.ts`, `frontend/src/api/config.ts`, `frontend/src/hooks/README.md`, `frontend/src/hooks/__tests__/useSetupAndServerHealth.test.ts` - -**Where found:** Three different patterns exist in `frontend/src/hooks/`: -1. `useRef` with manual abort before each fetch (correct — used in `useActiveBans`, `useActionList`). -2. Inline `AbortController` created inside `useEffect` and aborted in cleanup (acceptable for single-fetch effects — used in `useSchedule`). -3. No AbortController at all (incorrect — used in `useJailBannedIps`, `useBlocklistStatus`). - -**Goal:** Hooks that expose a manual `refresh()` function must use pattern 1 (the `useRef` approach), because the abort must survive across effect re-runs. Hooks that only fetch once on mount and never need manual refresh may use pattern 2. Remove all instances of pattern 3. Document the chosen conventions in a comment block at the top of a new file `frontend/src/hooks/README.md` (or inline in `fetchError.ts`). - -**Possible traps:** -- When migrating `useBlocklistStatus` from a boolean `cancelled` flag to an `AbortController`, the underlying `fetchSchedule` call must first accept a signal (TASK-014). -- Some hooks use both a debounce timer and an abort controller; the abort must cancel the in-flight request, while the timer cancellation prevents a new request from starting. These are independent concerns and should not be conflated. - -**Docs changes needed:** Add an ADR (Architecture Decision Record) or hook convention note to `Docs/Backend-Development.md` or a new `Docs/Frontend-Development.md`. - -**Why:** Inconsistent patterns make it hard for a new developer to know which approach to follow. The presence of pattern 3 causes memory leaks and stale-state React warnings in production. - ---- - -### TASK-016 — Extract generic `useListData` hook to eliminate duplicated fetch-list pattern (done) - -**Where fixed:** `frontend/src/hooks/useListData.ts`, `frontend/src/hooks/useActionList.ts`, `frontend/src/hooks/useFilterList.ts`, `frontend/src/hooks/useJailConfigs.ts`, `frontend/src/hooks/useBlocklists.ts`, `frontend/src/api/config.ts`, `frontend/src/api/blocklist.ts`, `frontend/src/hooks/__tests__/useListData.test.ts` - -**Where found:** `frontend/src/hooks/useActionList.ts`, `useFilterList.ts`, `useJailConfigs.ts`, and `useBlocklists.ts` each contain ~40 lines of nearly identical code: `useState` for data/loading/error, a `useRef`, a `refresh` callback with abort-create-set-loading-fetch-set logic, and a `useEffect` that calls `refresh` on mount. - -**Goal:** Create `frontend/src/hooks/useListData.ts` that exports a generic hook: +**Goal** +Wrap the `onSuccess` callback in `useCallback` inside `useJailConfigs` so its reference is stable across renders: ```ts -function useListData(options: { - fetcher: (signal: AbortSignal) => Promise; - selector: (response: TResponse) => TItem[]; - errorMessage: string; -}): { items: TItem[]; loading: boolean; error: string | null; refresh: () => void } +const onSuccess = useCallback((response: JailConfigListResponse) => { + setTotal(response.total); +}, []); ``` -Rewrite the four hooks listed above as thin wrappers around `useListData`. +Then pass `onSuccess` to `useListData`. The loop will break because `onSuccess` only changes when its own deps change (none here), so `refresh` is stable, so the effect fires only once on mount. -**Possible traps:** -- Some hooks (e.g. `useJailConfigs`) expose additional operations like `reload` and `update` beyond just listing. These should remain in their own hook and call `useListData` only for the list-loading portion. -- The `fetcher` function must accept a signal; ensure all underlying API functions have been updated (TASK-014) before replacing the hook internals. -- Generic hooks with complex type parameters can confuse TypeScript's inference; provide explicit type arguments at each call site to avoid `unknown` leaking out. +**Possible traps and issues** +- `useListData` itself would also need to ensure `onSuccess` is in its `useCallback` deps, which it already is — the fix is entirely in `useJailConfigs`. +- Adding `useCallback` to `onSuccess` but forgetting to keep it dep-free (empty array) would still cause the loop if any state value is incorrectly added to the array. -**Docs changes needed:** None. +**Docs changes needed** +Add a note to `Docs/Refactoring.md` explaining the `onSuccess` stability rule for `useListData` callers. -**Why:** Four copies of identical logic means bug fixes must be applied four times. The pattern is stable enough to abstract — every list hook has the same contract. +**Why this is needed** +The current code causes every visit to the Config → Jails tab to hammer the backend with an unbounded sequence of `GET /api/config/jails` requests until the user navigates away. --- -### TASK-017 — Move `source` derivation out of page components (done) +### TASK-BUG-02 — `ConfigPage` Tab Switch Destroys All Form State -**Where fixed:** `frontend/src/utils/queryUtils.ts`, `frontend/src/pages/DashboardPage.tsx`, `frontend/src/pages/MapPage.tsx` +**Where found** +`frontend/src/pages/ConfigPage.tsx`. The active tab content is wrapped in a `
`. Changing `key` forces React to unmount the old subtree and mount a brand-new one. `JailConfigDetail` alone has 20+ `useState` fields (banTime, findTime, maxRetry, failRegex, ignoreRegex, logPaths, datePattern, dnsMode, backend, logEncoding, prefRegex, plus all ban-time escalation fields). All of these are reset when the user switches tabs and switches back. -**Summary:** Added `getDataSource` to `frontend/src/utils/queryUtils.ts` and replaced inline `timeRange === "24h"` ternaries with the shared function in both dashboard and map pages. +**Goal** +Remove the `key={tab}` from the wrapping `
` and instead conditionally render each tab panel using `display: none` / `display: block` (or Fluent UI `TabPanel` visibility) so components stay mounted throughout the page lifetime. Alternatively, lift the shared state up to `ConfigPage` or a context so it survives remounts. The simplest fix is removing the key and using CSS visibility. -**Where found:** `frontend/src/pages/DashboardPage.tsx` and `frontend/src/pages/MapPage.tsx` both contain the identical line: +**Possible traps and issues** +- Hidden tabs still run their hooks (data fetches, effects). This is acceptable because the tabs are already mounted when the page loads; the behaviour becomes consistent rather than worse. +- If tab-specific effects must fire on "tab activation" they need to be converted from key-based remount triggers to explicit activation flags. +- Auto-save in `JailConfigDetail` uses `useAutoSave` which has its own pending-save debounce. Keeping the component mounted means a pending save from one session survives correctly across tab switches, which is the desired behaviour. + +**Docs changes needed** +Update `Docs/Web-Development.md` to note that tab panels must not use `key` for tab identity. + +**Why this is needed** +Users editing a jail config (filling in ban time, regex patterns, log paths) lose all unsaved changes if they accidentally click another tab and return. The auto-save timer is also cancelled, silently dropping work. + +--- + +### TASK-BUG-03 — MapPage Pagination Resets on Every Data Refresh + +**Where found** +`frontend/src/pages/MapPage.tsx` lines ~350–352: ```ts -const source = timeRange === "24h" ? "fail2ban" : "archive"; +useEffect(() => { + setPage(1); +}, [range, originFilter, selectedCountry, bans, pageSize]); +``` +`bans` is the array returned by `useMapData`. Every time `useMapData` completes a fetch it produces a new array reference. This causes `setPage(1)` to fire after every single background refresh, not just when the user changes a filter. + +**Goal** +Remove `bans` from the dependency array. Page should only reset to 1 when the user changes a *filter* (range, originFilter, selectedCountry, pageSize), not when the underlying data refreshes. + +**Possible traps and issues** +- After removing `bans`, if `bans.length` shrinks below the current page offset (e.g. the user is on page 3, data refreshes with fewer results), the pagination will show an empty page. Add a separate effect that clamps `page` to `totalPages` when `totalPages` changes: `if (page > totalPages) setPage(totalPages)`. +- `visibleBans` is already derived from `bans` via `useMemo`, so the table stays correct without `bans` in the reset effect. + +**Docs changes needed** +None required. + +**Why this is needed** +Users browsing the per-country ban table on MapPage are returned to page 1 every time the background auto-refresh fires (typically every 30 seconds), which makes the table unusable for pagination beyond the first page. + +--- + +### TASK-BUG-04 — `autoSavePayload` Silently Drops Intentional Zero Values + +**Where found** +`frontend/src/components/config/JailsTab.tsx` lines 213–215: +```ts +ban_time: Number(banTime) || jail.ban_time, +find_time: Number(findTime) || jail.find_time, +max_retry: Number(maxRetry) || jail.max_retry, +``` +The `||` operator treats `0` as falsy, so when a user sets `ban_time` to `0` (which in fail2ban means "ban permanently") the payload silently falls back to the server's current value. The user's intent is discarded without any error. + +**Goal** +Replace the `||` fallback with an explicit `NaN` guard: +```ts +ban_time: Number.isNaN(Number(banTime)) ? jail.ban_time : Number(banTime), +find_time: Number.isNaN(Number(findTime)) ? jail.find_time : Number(findTime), +max_retry: Number.isNaN(Number(maxRetry)) ? jail.max_retry : Number(maxRetry), +``` +This preserves `0` and other valid numeric values while still falling back when the field contains non-numeric text. + +**Possible traps and issues** +- An empty string converts to `0` via `Number("")`, which would then be sent to the API. If empty input is invalid, add a separate guard: only fall back if the trimmed string is empty or `NaN`. +- `max_retry` of `0` is meaningless in fail2ban (would never ban). Consider adding UI validation that shows a warning for `max_retry < 1` rather than silently correcting it. + +**Docs changes needed** +None required. + +**Why this is needed** +`ban_time = 0` in fail2ban sets a permanent ban — a common use case for admin hardening. The current code makes it impossible to save this value through the UI, with no error shown to the user. + +--- + +### TASK-BUG-05 — `InactiveJailDetail.handleValidate` Swallows Network Failures + +**Where found** +`frontend/src/components/config/JailsTab.tsx` inside `InactiveJailDetail`, the `handleValidate` callback: +```ts +onValidate() + .then((result) => { setValidationResult(result); }) + .catch(() => { /* validation call failed — ignore */ }) + .finally(() => { setValidating(false); }); +``` +If the API call fails (network error, 500, timeout), the spinner stops, `validationResult` remains `null`, and there is zero user feedback. The user cannot distinguish a clean "no issues" result from a silent failure. + +**Goal** +Add error state to `InactiveJailDetail` and render a `MessageBar intent="error"` when validation fails: +```ts +.catch((err: unknown) => { + setValidationError(err instanceof Error ? err.message : "Validation request failed."); +}) +``` +Clear `validationError` when the user clicks Validate again. + +**Possible traps and issues** +- The existing `validationResult` display logic handles the empty-issues case with a success banner. Ensure the new error state renders instead of the success banner when set. +- `handleFetchError` should be used (or replicated) so that auth errors don't show a generic error banner — they should trigger the session-expiry flow instead. + +**Docs changes needed** +None required. + +**Why this is needed** +Silent failure is worse than a visible error. A user who validates a jail config and sees nothing has no idea whether validation passed or the server is unreachable. + +--- + +### TASK-BUG-06 — `JailConfigDetail` Form State Never Re-syncs After Background Refresh + +**Where found** +`frontend/src/components/config/JailsTab.tsx`. `JailConfigDetail` initialises all 20+ form fields from `jail` prop in `useState` calls (lines 126–161). The component uses `key={selectedActiveJail.name}`, which forces remount only when the *selected jail changes*, not when the data for the already-selected jail is refreshed by the parent. If `useJailConfigs` does a background refresh and delivers updated server data for the currently-selected jail, the form continues displaying the stale locally-edited values. + +**Goal** +Add a `useEffect` that resets form fields when the incoming `jail` prop changes identity (i.e. when a server refresh delivers a new object for the same jail name). The effect must only run when the user is not mid-edit. The cleanest approach is to track a `lastSavedJail` ref and compare it to the incoming `jail`; if the auto-save has no pending changes and `jail` has changed, reset the fields. + +Alternatively, expose a `resetToServer` button that lets the user explicitly pull the latest server state without relying on automatic detection. + +**Possible traps and issues** +- Automatically resetting a form a user is actively editing is hostile. The reset must only happen when `autoSave` reports no pending changes and no dirty state. +- Comparing the full `jail` object on every render is expensive; use a ref to track the last-applied server version by comparing a stable property like `jail.name + JSON.stringify(jail)` (hashed or shallow-compared field by field). +- This issue is partially mitigated by `key={selectedActiveJail.name}` forcing remount on jail selection change. + +**Docs changes needed** +None required. + +**Why this is needed** +If fail2ban reloads externally (e.g. another admin makes a change), the GUI background-refreshes the config but the currently-open form silently shows stale data. A save action would overwrite the external change. + +--- + +### TASK-BUG-07 — `useJails()` Called Twice on `JailsPage` (Double HTTP Request) + +**Where found** +`frontend/src/pages/JailsPage.tsx` line 11: `const { jails } = useJails();` — used only to extract `jailNames` for `useIpLookup`. `frontend/src/pages/jails/JailOverviewSection.tsx` line 55: `const { jails, ... } = useJails();` — the full feature hook. Both components are rendered simultaneously on `JailsPage`, causing two parallel `GET /api/jails` requests on every page load. + +**Goal** +Remove the `useJails()` call from `JailsPage`. Pass `jailNames` to `JailsPage`'s children as a prop from `JailOverviewSection`, or lift `useJails()` to `JailsPage` and thread `jails` down as a prop to `JailOverviewSection`. Since `JailOverviewSection` already owns all the jail operations, the simplest fix is to accept an optional `onJailNamesLoaded` callback or have `JailsPage` access jail names directly from `JailOverviewSection` via a ref or by reading from the single hook call. + +**Possible traps and issues** +- `JailsPage` currently passes `jailNames` to a separate `IpLookupSection` or similar. After consolidating to one `useJails()` call the prop-drilling path needs to be updated. +- `JailOverviewSection` is the authoritative consumer of `useJails`; making `JailsPage` the single call site and passing the result down as props is the cleanest structural change. + +**Docs changes needed** +None required. + +**Why this is needed** +Every visit to the Jails page sends two identical requests to the backend. At scale with many jails this doubles the serialization and deserialization cost for no benefit. + +--- + +### TASK-BUG-08 — `AssignActionDialog` and `AssignFilterDialog` Call `useJails()` When Closed + +**Where found** +`frontend/src/components/config/AssignActionDialog.tsx` line 71 and `frontend/src/components/config/AssignFilterDialog.tsx` line 71. Both components call `useJails()` unconditionally at the top of the component body. The parent mounts these dialogs regardless of their `open` prop (so they can animate in), meaning `GET /api/jails` is fired every time the Config page tab containing these dialogs renders, even when the dialogs are never opened. + +**Goal** +Gate the `useJails()` call behind the `open` prop. Because React hooks cannot be called conditionally, the fix is to extract the dialog body into a separate inner component that is only rendered when `open` is true: +```tsx +export function AssignActionDialog({ open, ... }) { + return open ? : null; +} +function AssignActionDialogInner({ ... }) { + const { jails, ... } = useJails(); + ... +} +``` +This way `useJails()` only mounts (and fetches) when the dialog is actually open. + +**Possible traps and issues** +- Fluent UI Dialog animations may require the wrapper element to always exist for the open/close animation to work. In that case keep the `` wrapper in the outer component and only render the dialog content conditionally. +- Since `useJails()` is already called on `JailsPage` and `JailOverviewSection`, ideally jail data would be passed as a prop rather than fetched again in the dialog. Longer term, a jail context or shared data store would eliminate redundant fetches. + +**Docs changes needed** +None required. + +**Why this is needed** +Config tab loads currently trigger `GET /api/jails` from up to four independent call sites simultaneously: `JailsPage`, `JailOverviewSection`, `AssignActionDialog`, and `AssignFilterDialog`. This creates unnecessary backend load. + +--- + +### TASK-BUG-09 — `linesCount` Input in `ServerHealthSection` Fires Fetch on Every Keystroke + +**Where found** +`frontend/src/components/config/ServerHealthSection.tsx` line 410: `onChange={(_e, d) => { setLinesCount(Number(d.value)); }}`. The `linesCount` value is passed directly to `useServerHealth(linesCount, filterValue)`. `useServerHealth` re-creates its `refresh` callback when `linesCount` changes, which rebuilds `fetchData`, which triggers `useEffect([fetchData])`, firing a `GET /api/config/server/log` for every digit typed. Unlike `filterValue` (which is debounced at 500ms), `linesCount` has no debounce. + +**Goal** +Introduce a `debouncedLinesCount` state mirroring the existing `filterValue` / `filterRaw` pattern already in the component. Update the `onChange` handler to set a raw state, and apply a debounce (300–500ms) before committing to `linesCount` passed to `useServerHealth`. + +**Possible traps and issues** +- The debounce ref pattern (`filterDebounceRef`) is already present in the component; the linesCount debounce should reuse the same approach to avoid introducing a second debounce timer ref. +- `Number(d.value)` on an empty field produces `0`. Guard against `0` or negative values before passing to the API, or the backend may reject them. + +**Docs changes needed** +None required. + +**Why this is needed** +Typing `"500"` in the Lines field currently fires three HTTP requests (`"5"`, `"50"`, `"500"`). Each request fetches potentially hundreds of log lines and serializes them, adding unnecessary backend load. + + +--- + +### TASK-ABORT-01 — Missing `signal` Parameter on Multiple API Functions + +**Where found** +The following API functions accept no `signal` parameter and therefore cannot be cancelled regardless of what the calling hook does: + +| File | Function | +|------|----------| +| `frontend/src/api/history.ts` | `fetchHistory`, `fetchIpHistory` | +| `frontend/src/api/map.ts` | `fetchBansByCountry` | +| `frontend/src/api/jails.ts` | `fetchActiveBans`, `fetchJails` | +| `frontend/src/api/config.ts` | `fetchJailConfig`, `fetchInactiveJails`, `fetchJailConfigFiles`, `fetchFilterFiles` (partially — calls `fetchFilters` which accepts signal but discards it), `fetchActionFiles`, `fetchFilterFile`, `fetchActionFile` | +| `frontend/src/api/blocklist.ts` | `fetchImportLog`, `previewBlocklist` | + +**Goal** +Add an optional `signal?: AbortSignal` parameter to each function and forward it to the underlying `get()` call. Example: +```ts +export async function fetchHistory(query: HistoryQuery, signal?: AbortSignal): Promise { + return get(`${ENDPOINTS.history}?${buildQuery(query)}`, signal); +} +``` +Then update every calling hook to forward its controller's signal. + +**Possible traps and issues** +- `fetchFilterFiles` is an adapter that calls `fetchFilters()` internally. The correct fix is to accept `signal` and thread it into `fetchFilters(signal)`, then map the result. +- `previewBlocklist` is called from `useBlocklists.previewSource` which has no abort machinery. That hook should also be updated to add an `AbortController` if unmount cancellation is desired. +- Search for all call sites after adding the parameter; TypeScript's optional `?` means existing callers will not break but they should be updated to pass the signal where available. + +**Docs changes needed** +Add a coding convention to `Docs/Web-Development.md`: "All API functions that perform a `GET` request must accept an optional `signal?: AbortSignal` parameter and forward it to the HTTP client." + +**Why this is needed** +Without signals, navigating away from a page mid-fetch does not cancel the in-flight HTTP request. The response arrives, the callback fires, and React attempts a state update on an unmounted component. In development mode React warns about this; in production it causes silent no-ops and wastes server resources. + +--- + +### TASK-ABORT-02 — Hooks Create `AbortController` but Never Forward Signal to API + +**Where found** +Several hooks correctly create a controller and check `signal.aborted` in callbacks, but then call the API function without passing the signal — so the fetch is never actually cancelled: + +| Hook | API call missing signal | +|------|------------------------| +| `frontend/src/hooks/useConfigActiveStatus.ts` | `fetchJails()`, `fetchJailConfigs()` | +| `frontend/src/hooks/useMapData.ts` | `fetchBansByCountry(range, origin, source, countryCode)` | +| `frontend/src/hooks/useDashboardCountryData.ts` | `fetchBansByCountry(timeRange, origin, source)` | +| `frontend/src/hooks/useImportLog.ts` | `fetchImportLog(page, pageSize, sourceId)` | +| `frontend/src/hooks/useJailAdmin.ts` | `fetchInactiveJails()` | + +**Goal** +After completing TASK-ABORT-01, update each hook listed above to pass `controller.signal` (or `ctrl.signal`) to the API call. For example in `useMapData`: +```ts +fetchBansByCountry(range, origin, source, countryCode, controller.signal) ``` -**Goal:** Create a utility function `getDataSource(timeRange: TimeRange): "fail2ban" | "archive"` in `frontend/src/utils/constants.ts` (or a new `queryUtils.ts` entry). Import and call it in both pages. Remove the inline ternary. +**Possible traps and issues** +- This task is a dependency of TASK-ABORT-01: the API functions must accept `signal` first. +- `useConfigActiveStatus` makes two sequential fetches; both need the same signal forwarded. +- After adding the signal parameter, ensure the abort check in `.then()` / `.catch()` callbacks uses `controller.signal.aborted` (the local capture), not `abortRef.current?.signal.aborted` (the mutable ref). -**Possible traps:** -- This rule ("24h uses live fail2ban data, everything else uses the archive") is a backend contract, not a UI preference. If the backend ever adds a new time range or changes the source mapping, both places must be updated in sync; centralising it makes that change a one-liner. -- The function name should make the business rule obvious — `getLiveDataSource` is more descriptive than `getDataSource`. +**Docs changes needed** +None beyond TASK-ABORT-01. -**Docs changes needed:** Document the time range → source mapping in `Docs/Backend-Development.md` if not already described. - -**Why:** Duplicated business logic in UI components is fragile. If the rule changes or a new time range is added, it is easy to update one page and miss the other. +**Why this is needed** +The abort guard (`if (controller.signal.aborted) return;`) is present but pointless when the HTTP request itself was never aborted. The network request still completes and consumes server resources; only the state update is skipped. --- -### TASK-018 — Move inline `style={{…}}` objects to `makeStyles` (done) +### TASK-ABORT-03 — Stale `abortRef` Read in `.finally()` in Three Hooks -**Where fixed:** `frontend/src/pages/MapPage.tsx`, `frontend/src/pages/DashboardPage.tsx`, `frontend/src/pages/jail/PatternsSection.tsx`, `frontend/src/pages/jails/BanUnbanForm.tsx`, `frontend/src/layouts/MainLayout.tsx`, `frontend/src/pages/jails/jailsPageStyles.ts` - -**Summary:** Moved constant inline style objects into `makeStyles` classes and used `mergeClasses` for dynamic button layout in `MainLayout`. Kept runtime-dependent opacity and collapse state as conditional class application. - -**Where found:** 30+ instances across `frontend/src/pages/MapPage.tsx`, `frontend/src/pages/map/MapBansTable.tsx`, `frontend/src/pages/DashboardPage.tsx`, `frontend/src/pages/jail/PatternsSection.tsx`, `frontend/src/pages/jails/BanUnbanForm.tsx`, `frontend/src/layouts/MainLayout.tsx`, and others. - -**Goal:** For each instance of `style={{…}}` that is not dynamic (i.e. the style values are constants or token references, not computed from props), move the declaration into the nearest `makeStyles` call. Dynamic styles that depend on runtime values (e.g. `style={{ opacity: loading ? 0.5 : 1 }}`) should remain inline or be converted to conditional `mergeClasses` with two separate class definitions. - -**Possible traps:** -- The `MainLayout` logout button uses `style={{ width: "100%", justifyContent: collapsed ? "center" : "flex-start" }}` — this is dynamic and must become two `makeStyles` entries toggled with `mergeClasses`. -- The `WorldMap` country `` uses CSS custom properties (`--country-fill`) as inline styles for CSS variable injection; this is intentional and should not be changed. -- Running ESLint with `@typescript-eslint/no-unsafe-assignment` can catch some object literals, but a manual pass is more reliable. - -**Docs changes needed:** None. - -**Why:** Inline style objects create a new object reference on every render, defeating `React.memo` (TASK-011) and preventing the browser from reusing cached styles. Fluent UI's `makeStyles` uses atomic CSS which is far more cache-efficient. - ---- - -### TASK-019 — Replace index keys with stable keys in editable lists (done) - -**Where fixed:** `frontend/src/components/config/StringListEditor.tsx`, `frontend/src/components/config/RegexList.tsx`, `frontend/src/components/config/JailsTab.tsx`, `frontend/src/components/config/stableListEntries.ts` - -**Summary:** Added stable internal IDs for editable string lists, replaced index-based React keys with stable entry IDs, and updated the jail log path list to use the path value as its key. - -**Where found:** 19 instances identified, including: -- `frontend/src/components/config/StringListEditor.tsx` line 34 — `key={index}` on editable `Input` rows. -- `frontend/src/components/config/RegexList.tsx` line 66 — `key={i}` on editable regex rows. -- `frontend/src/components/config/JailsTab.tsx` lines 384, 714, 726 — `key={i}` and `key={idx}`. -- `frontend/src/components/blocklist/BlocklistScheduleSection.tsx` line 147 — `key={i}` on `