Lukas
3e3578f4d8
- 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>
680 lines
40 KiB
Markdown
680 lines
40 KiB
Markdown
|
||
### TASK-SEC-01 — Open Redirect in LoginPage via `?next=` Parameter
|
||
|
||
**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).
|
||
|
||
---
|
||
|
||
### TASK-BUG-01 — Infinite Re-Fetch Loop in `useJailConfigs`
|
||
|
||
**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.
|
||
|
||
**Goal**
|
||
Wrap the `onSuccess` callback in `useCallback` inside `useJailConfigs` so its reference is stable across renders:
|
||
```ts
|
||
const onSuccess = useCallback((response: JailConfigListResponse) => {
|
||
setTotal(response.total);
|
||
}, []);
|
||
```
|
||
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 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**
|
||
Add a note to `Docs/Refactoring.md` explaining the `onSuccess` stability rule for `useListData` callers.
|
||
|
||
**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-BUG-02 — `ConfigPage` Tab Switch Destroys All Form State
|
||
|
||
**Where found**
|
||
`frontend/src/pages/ConfigPage.tsx`. The active tab content is wrapped in a `<div key={tab}>`. 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.
|
||
|
||
**Goal**
|
||
Remove the `key={tab}` from the wrapping `<div>` 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.
|
||
|
||
**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
|
||
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 ? <AssignActionDialogInner ... /> : 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 `<Dialog open={open}>` 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<HistoryResponse> {
|
||
return get<HistoryResponse>(`${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)
|
||
```
|
||
|
||
**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).
|
||
|
||
**Docs changes needed**
|
||
None beyond TASK-ABORT-01.
|
||
|
||
**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-ABORT-03 — Stale `abortRef` Read in `.finally()` in Three Hooks
|
||
|
||
**Where found**
|
||
`frontend/src/hooks/useGlobalConfig.ts`, `frontend/src/hooks/useServerSettings.ts`, and `frontend/src/hooks/useJailConfigDetail.ts`. In each hook the `.finally()` block reads:
|
||
```ts
|
||
.finally(() => {
|
||
if (!abortRef.current?.signal.aborted) {
|
||
setLoading(false);
|
||
}
|
||
});
|
||
```
|
||
`abortRef.current` is a mutable ref. If `load()` is called a second time before the first fetch completes, `abortRef.current` is replaced with a new controller. The first fetch's `.finally()` then reads the *new* controller's signal (which is not aborted) and calls `setLoading(false)` while the second fetch is still in flight.
|
||
|
||
**Goal**
|
||
Capture the local controller reference before the async operation and use that in `.finally()`:
|
||
```ts
|
||
const ctrl = new AbortController();
|
||
abortRef.current = ctrl;
|
||
// ...
|
||
.finally(() => {
|
||
if (!ctrl.signal.aborted) { // use the captured local variable
|
||
setLoading(false);
|
||
}
|
||
});
|
||
```
|
||
This is the pattern already used correctly in `useListData`, `useBanTrend`, and most other hooks.
|
||
|
||
**Possible traps and issues**
|
||
- This is a low-frequency race condition (only observable if `load()` is called in rapid succession), but it produces a spinner that disappears prematurely while a fetch is still in flight.
|
||
- After this fix verify that the existing checks in `.then()` and `.catch()` also use the locally-captured `ctrl` (not `abortRef.current`) — they typically already do, but confirm.
|
||
|
||
**Docs changes needed**
|
||
Add a code convention note in `Docs/Web-Development.md`: "Always capture the `AbortController` in a local `const` and use that local variable in all callbacks — never read `abortRef.current` from inside an async callback."
|
||
|
||
**Why this is needed**
|
||
The loading spinner is the primary feedback mechanism for data fetches. Prematurely clearing it while a request is still in flight misleads the user and can cause partial renders.
|
||
|
||
---
|
||
|
||
### TASK-ABORT-04 — `useIpLookup` Has No AbortController or Unmount Guard
|
||
|
||
**Where found**
|
||
`frontend/src/hooks/useIpLookup.ts`. The hook performs an async fetch but has no `AbortController`, no `useEffect` cleanup, and no unmount guard. If the component that calls this hook unmounts before the fetch completes (e.g. the user navigates away), the `.then()` callback still fires and calls `setResult` / `setLoading` on an unmounted component.
|
||
|
||
**Goal**
|
||
Add a standard `AbortController` pattern:
|
||
```ts
|
||
const abortRef = useRef<AbortController | null>(null);
|
||
const lookup = useCallback(async (ip: string): Promise<void> => {
|
||
abortRef.current?.abort();
|
||
const ctrl = new AbortController();
|
||
abortRef.current = ctrl;
|
||
setLoading(true);
|
||
try {
|
||
const result = await fetchIpInfo(ip, ctrl.signal);
|
||
if (ctrl.signal.aborted) return;
|
||
setResult(result);
|
||
} catch (err) {
|
||
if (ctrl.signal.aborted) return;
|
||
// handle error
|
||
} finally {
|
||
if (!ctrl.signal.aborted) setLoading(false);
|
||
}
|
||
}, []);
|
||
```
|
||
Add a `useEffect` cleanup that calls `abortRef.current?.abort()` on unmount.
|
||
|
||
**Possible traps and issues**
|
||
- `fetchIpInfo` (in `api/jails.ts` or equivalent) must accept `signal` for the abort to actually cancel the HTTP request. Check whether it already does; if not, add it as part of TASK-ABORT-01.
|
||
- This hook is likely called from a popover or panel that can close while a lookup is running; unmount cancellation is the primary concern here.
|
||
|
||
**Docs changes needed**
|
||
None required.
|
||
|
||
**Why this is needed**
|
||
Calling React state setters on unmounted components is a React antipattern that produces console warnings in development and can cause subtle state corruption if the component re-mounts quickly.
|
||
|
||
---
|
||
|
||
## State Management
|
||
|
||
---
|
||
|
||
### TASK-STATE-01 — Auth Errors Silently Swallowed in `useRegexTester` and `useLogPreview`
|
||
|
||
**Where found**
|
||
`frontend/src/hooks/useRegexTester.ts` and `frontend/src/hooks/useLogPreview.ts`. Each hook's catch block handles errors only as `err instanceof Error`, which catches `ApiError` (a subclass of `Error`), but does not call `handleFetchError`. The `handleFetchError` utility calls `isAuthError` and returns early without setting UI error text, allowing the global `SESSION_EXPIRED_EVENT` listener in `AuthProvider` to handle the redirect. By bypassing `handleFetchError`, auth errors (401/403) are displayed as UI error text (`err.message`) instead of triggering the session expiry flow.
|
||
|
||
**Goal**
|
||
Replace the ad-hoc catch blocks in both hooks with `handleFetchError`:
|
||
```ts
|
||
} catch (err: unknown) {
|
||
if (signal.aborted) return;
|
||
handleFetchError(err, setError, "Regex test failed");
|
||
}
|
||
```
|
||
`handleFetchError` already imports `isAuthError` and returns early for auth errors, allowing the global handler to take over.
|
||
|
||
**Possible traps and issues**
|
||
- Confirm that `SESSION_EXPIRED_EVENT` is being listened to in `AuthProvider` and that it triggers navigation to `/login`. It is — this was verified in the architecture review.
|
||
- After the fix, a 401 during a regex test will show no error text (the auth handler navigates away), which is correct behaviour.
|
||
|
||
**Docs changes needed**
|
||
Add a convention to `Docs/Web-Development.md`: "All hook catch blocks must use `handleFetchError` rather than directly calling `setError`, so auth errors are routed to the session-expiry flow."
|
||
|
||
**Why this is needed**
|
||
When a session expires while the user is in the Regex Tester, they see a confusing "Unauthorized" error message in the tester UI instead of being cleanly redirected to the login page.
|
||
|
||
---
|
||
|
||
### TASK-STATE-02 — `withRefresh` in `useJailList` Creates New Function References Every Render
|
||
|
||
**Where found**
|
||
`frontend/src/hooks/useJailList.ts`. The `withRefresh` helper is defined inside the hook body without `useCallback`. It wraps an operation and calls `refresh()` after it completes. Because it is recreated every render, `startJail`, `stopJail`, `setIdle`, and `reloadJail` — which are all produced by `withRefresh(...)` — are also new references every render. Any child component receiving these as props will re-render even when nothing has changed.
|
||
|
||
**Goal**
|
||
Wrap `withRefresh` in `useCallback` with `[refresh]` as dependency, or inline `useCallback` directly for each operation:
|
||
```ts
|
||
const startJail = useCallback(async (name: string) => {
|
||
await apiStartJail(name);
|
||
refresh();
|
||
}, [refresh]);
|
||
```
|
||
This ensures the function references are stable between renders.
|
||
|
||
**Possible traps and issues**
|
||
- If `withRefresh` is used to produce many operations, it may be cleaner to keep `withRefresh` as a `useCallback` that captures `refresh`, rather than wrapping each call individually.
|
||
- After stabilising these references, `JailOverviewSection` or any child wrapped in `React.memo` will correctly skip re-renders when only unrelated state changes.
|
||
|
||
**Docs changes needed**
|
||
None required.
|
||
|
||
**Why this is needed**
|
||
Unstable function references passed as props defeat `React.memo` optimisations and cause unnecessary child re-renders. On the Jails page, which renders a list of potentially many jails, this compounds with every parent state change.
|
||
|
||
---
|
||
|
||
### TASK-STATE-03 — `DashboardFilterBar` Has Dual State Source
|
||
|
||
**Where found**
|
||
`frontend/src/components/DashboardFilterBar.tsx`. The component reads from both `DashboardFilterProvider` context and from props using `??` as a fallback. `HistoryPage` passes filter values via props without mounting a `DashboardFilterProvider`, so the context values are `undefined` and the `??` fallback silently provides context defaults. `DashboardPage` uses context, so props are `undefined` and context values apply. Both pages render the same component but through different code paths.
|
||
|
||
**Goal**
|
||
Choose one data source and use it consistently. The recommended approach is to use props everywhere: `DashboardPage` should read from context via `useDashboardFilter()` and pass the values explicitly to `DashboardFilterBar` as props, exactly like `HistoryPage` does. This makes the component's behaviour predictable — it always reads from props, never from context.
|
||
|
||
**Possible traps and issues**
|
||
- `DashboardFilterProvider` may be used by other components on the dashboard. Audit all consumers of `useDashboardFilter()` before removing the context read from `DashboardFilterBar`.
|
||
- The `??` fallback chain must be fully removed; otherwise the dual-source behaviour can creep back.
|
||
|
||
**Docs changes needed**
|
||
None required.
|
||
|
||
**Why this is needed**
|
||
Silent dual-source components are a debugging hazard. A developer adding a new consumer of `DashboardFilterBar` has no obvious signal about which data source is active, leading to subtle bugs when one source overrides the other.
|
||
|
||
---
|
||
|
||
### TASK-STATE-04 — Token in `sessionStorage` Is Never Sent (Misleading Auth Model)
|
||
|
||
**Where found**
|
||
`frontend/src/providers/AuthProvider.tsx`. After login, a JWT token and `expires_at` timestamp are stored in `sessionStorage`. `isAuthenticated` is derived entirely from the `expires_at` check against the local clock. However, `frontend/src/api/client.ts` uses `credentials: "include"` (cookie auth) for every request and never reads the token from `sessionStorage`. The stored token is purely decorative.
|
||
|
||
**Goal**
|
||
Remove the `sessionStorage` token storage entirely. `isAuthenticated` should instead be determined by whether the backend considers the session valid. The practical check is: the user is authenticated when the last request did not return 401/403. A simple approach is to set `isAuthenticated = true` on successful login, set it to `false` when `SESSION_EXPIRED_EVENT` fires, and persist only a boolean (or nothing at all) in `sessionStorage` for page-reload continuity.
|
||
|
||
If the token is stored intentionally for future use (e.g. switching to Bearer token auth), add a clear comment explaining this, and add a `TODO` so it is not silently misleading.
|
||
|
||
**Possible traps and issues**
|
||
- If `expires_at` is used to proactively redirect the user to login before a request fails, removing it changes the UX: the user will stay on the page until the next request fails with 401. This is generally acceptable since the server is the authority on session validity.
|
||
- Test that `SetupGuard` and `RequireAuth` still work correctly after removing the `sessionStorage` check.
|
||
|
||
**Docs changes needed**
|
||
Update `Docs/Web-Development.md` auth section to accurately describe the cookie-based auth model.
|
||
|
||
**Why this is needed**
|
||
The misleading code makes the auth model appear to be token-based when it is actually cookie-based. This causes confusion during development and maintenance, and the local clock `expires_at` check can cause premature logouts if there is clock skew between client and server.
|
||
|
||
---
|
||
|
||
### TASK-PERF-01 — `ConfigListDetail` Calls `sortItems()` on Every Render
|
||
|
||
**Where found**
|
||
`frontend/src/components/config/ConfigListDetail.tsx`. The `sortItems()` function is called directly in the render path without `useMemo`. For config lists with many items (e.g. all filters in a system with many services) this performs an O(n log n) sort on every render, including renders triggered by unrelated state changes.
|
||
|
||
**Goal**
|
||
Wrap the `sortItems()` call in `useMemo` with the items array as dependency:
|
||
```ts
|
||
const sortedItems = useMemo(() => sortItems(items), [items]);
|
||
```
|
||
Replace all uses of the raw `items` in the render with `sortedItems`.
|
||
|
||
**Possible traps and issues**
|
||
- Verify that `items` array reference is stable (not recreated on every parent render). If the parent passes a new array each time, the `useMemo` will re-sort every render anyway. Trace `items` back to its source hook to confirm stability.
|
||
- `sortItems` must be a pure function with no side effects for `useMemo` to be safe. Verify this.
|
||
|
||
**Docs changes needed**
|
||
None required.
|
||
|
||
**Why this is needed**
|
||
The Config page is already re-rendering due to multiple concurrent hooks. Adding an O(n log n) sort to each render cycle adds unnecessary CPU work, visible as jank when navigating long filter or action lists.
|
||
|
||
---
|
||
|
||
### TASK-PERF-02 — `useSchedule` Exposes No `refresh` Function
|
||
|
||
**Where found**
|
||
`frontend/src/hooks/useSchedule.ts`. The hook fetches the import schedule on mount and exposes no way to re-fetch. After a `PUT /api/blocklists/schedule` from a different component or tab, the displayed schedule data stays stale until the user navigates away and back.
|
||
|
||
**Goal**
|
||
Expose a `refresh` callback from `useSchedule`, following the same pattern as `useListData` and other hooks. The `BlocklistsPage` (or whichever component saves schedule changes) should call `refresh()` after a successful save, and after `runImportNow()` completes.
|
||
|
||
**Possible traps and issues**
|
||
- `useSchedule` currently uses a simple `useEffect` on mount. Adding `refresh` means converting the internal fetch into a `useCallback` and calling it from the effect.
|
||
- Ensure the `AbortController` pattern is applied correctly when adding `refresh`.
|
||
|
||
**Docs changes needed**
|
||
None required.
|
||
|
||
**Why this is needed**
|
||
After saving a new schedule the user sees the old schedule until they reload the page. This makes the save feel broken even when it succeeded.
|
||
|
||
---
|
||
|
||
### TASK-QUALITY-01 — `KVEditor` Uses `entryKeys.join(",")` as Effect Dependency
|
||
|
||
**Where found**
|
||
`frontend/src/components/config/KVEditor.tsx`. An effect dependency is computed as `entryKeys.join(",")`. This works for most key values but produces incorrect results (false equality) when any key contains a comma character — two different key sets could produce the same joined string.
|
||
|
||
**Goal**
|
||
Replace the join-based comparison with a stable serialisation that cannot produce false equality. The simplest correct option is `JSON.stringify(entryKeys)`, which handles commas, empty strings, and special characters correctly. Alternatively, use `useDeepCompareEffect` from a utility library, or maintain a counter that increments whenever keys change.
|
||
|
||
**Possible traps and issues**
|
||
- `JSON.stringify` on a large array is marginally more expensive than `join`. For a config editor with typically fewer than 50 keys this cost is negligible.
|
||
- Ensure the dependency is the full keys array (not the joined string) and let React's referential equality handle the common case; only reach for `JSON.stringify` if the array reference itself is not stable.
|
||
|
||
**Docs changes needed**
|
||
None required.
|
||
|
||
**Why this is needed**
|
||
A KV entry key containing a comma (e.g. `"a,b"` vs separate keys `"a"` and `"b"`) would cause the effect to not fire when it should, silently failing to update derived state.
|
||
|
||
---
|
||
|
||
### TASK-QUALITY-02 — `useConfigItem.save()` Briefly Shows Session-Expiry as Save Error
|
||
|
||
**Where found**
|
||
`frontend/src/hooks/useConfigItem.ts` lines 70–80. The `save()` function's catch block calls `setSaveError(err.message)` for all errors including `ApiError(401)` and `ApiError(403)`. The HTTP client layer dispatches `SESSION_EXPIRED_EVENT` on those status codes, which triggers auth handling, but `setSaveError` still runs first and may briefly display an "Unauthorized" or similar message before the navigation occurs.
|
||
|
||
**Goal**
|
||
Check for auth errors before setting save error state:
|
||
```ts
|
||
} catch (err: unknown) {
|
||
if (isAuthError(err)) throw err; // let auth handler deal with it
|
||
const message = err instanceof Error ? err.message : "Failed to save data";
|
||
setSaveError(message);
|
||
throw err;
|
||
}
|
||
```
|
||
|
||
**Possible traps and issues**
|
||
- Rethrowing auth errors is correct here since the caller might also have error handling. Confirm that all callers of `save()` handle the re-thrown auth error gracefully (typically by not doing anything — the session expiry flow handles navigation).
|
||
- Import `isAuthError` from `../api/client`.
|
||
|
||
**Docs changes needed**
|
||
None required.
|
||
|
||
**Why this is needed**
|
||
Briefly flashing "Unauthorized" in a form's save-error field is confusing UX when the correct outcome is a redirect to the login page.
|
||
|
||
---
|
||
|
||
### TASK-QUALITY-03 — `useHistory` Object Identity Dependency Footgun
|
||
|
||
**Where found**
|
||
`frontend/src/hooks/useHistory.ts`. The hook accepts a `query` object as a parameter and lists it directly in the `useCallback` dependency array for the internal `load` function. If a caller passes an inline object literal on every render (e.g. `useHistory({ page: 1, jail: selectedJail })`), `query` is a new reference every render, causing a new `load` callback, which causes `useEffect([load])` to fire, triggering an infinite re-fetch.
|
||
|
||
**Goal**
|
||
Document this constraint prominently in the hook's JSDoc and in `Docs/Web-Development.md`. Alternatively, change the hook to accept individual primitive parameters instead of an object, eliminating the reference-stability requirement:
|
||
```ts
|
||
export function useHistory(page: number, pageSize: number, jail?: string, ...): UseHistoryResult
|
||
```
|
||
This is the safest fix because it makes incorrect usage a compile-time error.
|
||
|
||
**Possible traps and issues**
|
||
- Changing the signature is a breaking change for all callers. Audit all call sites before changing the signature.
|
||
- The interim documentation fix (a clear JSDoc warning) is a lower-risk option if refactoring callers is out of scope.
|
||
|
||
**Docs changes needed**
|
||
Add a note to `Docs/Web-Development.md`: "Hooks that accept objects as parameters must either destructure to primitives internally or require the caller to provide a stable reference (e.g. via `useMemo`)."
|
||
|
||
**Why this is needed**
|
||
This is a silent footgun. The hook works correctly in all current call sites only because callers happen to use `useMemo` or stable state references. A future caller passing an inline literal will introduce an infinite re-fetch with no obvious diagnostic.
|
||
|
||
---
|
||
|
||
### TASK-QUALITY-04 — `pendingSaveRef as boolean` Redundant Cast in `useAutoSave`
|
||
|
||
**Where found**
|
||
`frontend/src/hooks/useAutoSave.ts`. The code contains `if (pendingSaveRef.current as boolean)` where `pendingSaveRef` is already typed as `React.MutableRefObject<boolean>`. The `as boolean` cast is redundant and suggests the author was uncertain about the type.
|
||
|
||
**Goal**
|
||
Remove the cast: `if (pendingSaveRef.current)`. Run TypeScript type-check to confirm no error is introduced.
|
||
|
||
**Possible traps and issues**
|
||
- None. This is a one-line cleanup.
|
||
|
||
**Docs changes needed**
|
||
None required.
|
||
|
||
**Why this is needed**
|
||
Redundant type assertions are noise that makes reviewers second-guess the type system. They also suppress TypeScript errors in cases where the cast is actually incorrect.
|
||
|
||
---
|
||
|
||
### TASK-QUALITY-05 — `console.warn` in `MapPage` Provides No User Feedback for Threshold Errors
|
||
|
||
**Where found**
|
||
`frontend/src/pages/MapPage.tsx` lines ~148–151:
|
||
```ts
|
||
useEffect(() => {
|
||
if (mapThresholdError) {
|
||
console.warn("Failed to load map color thresholds:", mapThresholdError);
|
||
}
|
||
}, [mapThresholdError]);
|
||
```
|
||
When the threshold fetch fails the map silently falls back to hardcoded defaults. The user has no indication that their custom thresholds are not being applied.
|
||
|
||
**Goal**
|
||
Replace the `console.warn` with a small inline `MessageBar` or tooltip near the map legend that indicates thresholds could not be loaded and defaults are in use. The `console.warn` should be removed from production-facing code.
|
||
|
||
**Possible traps and issues**
|
||
- The fallback behaviour (using hardcoded defaults) is correct and the map should still render. The notification should be non-blocking (not a modal or full-page error).
|
||
- If the threshold fetch failing is expected in certain deployment configurations (e.g. feature not configured), an info-level message rather than a warning may be more appropriate.
|
||
|
||
**Docs changes needed**
|
||
None required.
|
||
|
||
**Why this is needed**
|
||
`console.warn` is invisible to end users. If a custom threshold configuration is silently not applied, the map colour coding may be misleading with no indication of why.
|
||
|
||
---
|
||
|
||
### TASK-QUALITY-06 — `console.log` Leaked in `HistoryPage.test.tsx`
|
||
|
||
**Where found**
|
||
`frontend/src/pages/__tests__/HistoryPage.test.tsx` line 8. A `console.log` statement was left in the test file, likely from a debugging session.
|
||
|
||
**Goal**
|
||
Remove the `console.log` call.
|
||
|
||
**Possible traps and issues**
|
||
- None.
|
||
|
||
**Docs changes needed**
|
||
None required.
|
||
|
||
**Why this is needed**
|
||
Debug logs in test files pollute the test runner output and make it harder to spot real failures or warnings.
|