refactoring-backend #3
@@ -1,68 +1,3 @@
|
||||
### 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**
|
||||
|
||||
@@ -6,7 +6,7 @@
|
||||
* raw-config editor.
|
||||
*/
|
||||
|
||||
import { useCallback, useEffect, useMemo, useState } from "react";
|
||||
import { useCallback, useEffect, useMemo, useRef, useState } from "react";
|
||||
import {
|
||||
Badge,
|
||||
Button,
|
||||
@@ -249,6 +249,78 @@ function JailConfigDetail({
|
||||
const { status: saveStatus, errorText: saveErrorText, retry: retrySave } =
|
||||
useAutoSave(autoSavePayload, saveCurrent);
|
||||
|
||||
// Ref to track the last jail object we synced from. When the incoming jail
|
||||
// prop is a new object (indicating a server refresh), and autoSave is idle
|
||||
// with no pending changes, we reset the form to the new server state.
|
||||
const lastSyncedJailRef = useRef<JailConfig>(jail);
|
||||
|
||||
// Resync form fields when the jail prop changes identity (server refresh)
|
||||
// but only if autoSave has completed and there are no pending changes.
|
||||
useEffect(() => {
|
||||
if (
|
||||
lastSyncedJailRef.current !== jail &&
|
||||
saveStatus === "idle" &&
|
||||
!saveErrorText
|
||||
) {
|
||||
// Payload is equal to the server state, so we can safely reset.
|
||||
if (
|
||||
JSON.stringify(autoSavePayload) === JSON.stringify({
|
||||
ban_time: lastSyncedJailRef.current.ban_time,
|
||||
find_time: lastSyncedJailRef.current.find_time,
|
||||
max_retry: lastSyncedJailRef.current.max_retry,
|
||||
fail_regex: lastSyncedJailRef.current.fail_regex,
|
||||
ignore_regex: lastSyncedJailRef.current.ignore_regex,
|
||||
date_pattern: lastSyncedJailRef.current.date_pattern ?? null,
|
||||
dns_mode: lastSyncedJailRef.current.use_dns,
|
||||
log_encoding: lastSyncedJailRef.current.log_encoding,
|
||||
prefregex: lastSyncedJailRef.current.prefregex,
|
||||
bantime_escalation: {
|
||||
increment: lastSyncedJailRef.current.bantime_escalation?.increment ?? false,
|
||||
factor: lastSyncedJailRef.current.bantime_escalation?.factor ?? null,
|
||||
formula: lastSyncedJailRef.current.bantime_escalation?.formula ?? null,
|
||||
multipliers: lastSyncedJailRef.current.bantime_escalation?.multipliers ?? null,
|
||||
max_time: lastSyncedJailRef.current.bantime_escalation?.max_time ?? null,
|
||||
rnd_time: lastSyncedJailRef.current.bantime_escalation?.rnd_time ?? null,
|
||||
overall_jails: lastSyncedJailRef.current.bantime_escalation?.overall_jails ?? false,
|
||||
},
|
||||
})
|
||||
) {
|
||||
// Reset all form state to new jail prop values.
|
||||
setBanTime(String(jail.ban_time));
|
||||
setFindTime(String(jail.find_time));
|
||||
setMaxRetry(String(jail.max_retry));
|
||||
setFailRegex(jail.fail_regex);
|
||||
setIgnoreRegex(jail.ignore_regex);
|
||||
setLogPaths(jail.log_paths);
|
||||
setDatePattern(jail.date_pattern ?? "");
|
||||
setDnsMode(jail.use_dns);
|
||||
setBackend(jail.backend);
|
||||
setLogEncoding(jail.log_encoding);
|
||||
setPrefRegex(jail.prefregex);
|
||||
setEscEnabled(jail.bantime_escalation?.increment ?? false);
|
||||
setEscFactor(
|
||||
jail.bantime_escalation?.factor != null
|
||||
? String(jail.bantime_escalation.factor)
|
||||
: "",
|
||||
);
|
||||
setEscFormula(jail.bantime_escalation?.formula ?? "");
|
||||
setEscMultipliers(jail.bantime_escalation?.multipliers ?? "");
|
||||
setEscMaxTime(
|
||||
jail.bantime_escalation?.max_time != null
|
||||
? String(jail.bantime_escalation.max_time)
|
||||
: "",
|
||||
);
|
||||
setEscRndTime(
|
||||
jail.bantime_escalation?.rnd_time != null
|
||||
? String(jail.bantime_escalation.rnd_time)
|
||||
: "",
|
||||
);
|
||||
setEscOverallJails(jail.bantime_escalation?.overall_jails ?? false);
|
||||
}
|
||||
lastSyncedJailRef.current = jail;
|
||||
}
|
||||
}, [jail, saveStatus, saveErrorText, autoSavePayload]);
|
||||
|
||||
// Raw config file fetch/save helpers — uses jail.d/<name>.conf convention.
|
||||
const fetchRaw = useCallback(async (): Promise<string> => {
|
||||
return await fetchRawContent();
|
||||
|
||||
Reference in New Issue
Block a user