Fix raw action config endpoint shadowed by config router

Rename GET/PUT /api/config/actions/{name} to /actions/{name}/raw in
file_config.py to eliminate the route-shadowing conflict with config.py,
which registers its own GET /actions/{name} returning ActionConfig.

Add configActionRaw endpoint helper in endpoints.ts and update
fetchActionFile/updateActionFile in config.ts to use it. Add
TestGetActionFileRaw and TestUpdateActionFileRaw test classes.
This commit is contained in:
2026-03-15 14:09:37 +01:00
parent 41dcd60225
commit b81e0cdbb4
7 changed files with 219 additions and 40 deletions

View File

@@ -4,60 +4,142 @@ This document breaks the entire BanGUI project into development stages, ordered
---
## ✅ Task: Add "Deactivate Jail" Button for Inactive Jails in the Config View
## Bug Fix: "Raw Action Configuration" always empty — DONE
**Context:**
In `frontend/src/components/config/JailsTab.tsx`, the "Deactivate Jail" button is currently only rendered when a jail is active. When a jail is inactive but has an existing `jail.d/{name}.local` file (i.e. it was previously configured and has `enabled = false`), there is no action button to clean up that override file.
**Summary:** Renamed `GET /actions/{name}` and `PUT /actions/{name}` in `file_config.py` to `GET /actions/{name}/raw` and `PUT /actions/{name}/raw` to eliminate the route-shadowing conflict with `config.py`. Added `configActionRaw` endpoint helper in `endpoints.ts` and updated `fetchActionFile` / `updateActionFile` in `config.ts` to call it. Added `TestGetActionFileRaw` and `TestUpdateActionFileRaw` test classes.
**Goal:**
Add a "Deactivate Jail" (or "Remove Config") button to the jail config view for inactive jails that have a `.local` file. Clicking it should delete the `jail.d/{name}.local` file via the existing deactivate endpoint (`POST /api/config/jails/{name}/deactivate`) or a dedicated delete-override endpoint, making the UI consistent with the active-jail view. If no `.local` file exists for the inactive jail, the button should not be shown (there is nothing to clean up).
**Problem**
When a user opens the *Actions* tab in the Config screen, selects any action, and expands the "Raw Action Configuration" accordion, the text area is always blank. The `fetchContent` callback makes a `GET /api/config/actions/{name}` request expecting a `ConfFileContent` response (`{ content: string, name: string, filename: string }`), but the backend returns an `ActionConfig` (the fully-parsed structured model) instead. The `content` field is therefore `undefined` in the browser, which the `RawConfigSection` component renders as an empty string.
**Acceptance criteria:**
- Inactive jails that own a `.local` file show a "Deactivate Jail" button in their config panel.
- Calling the button removes or neutralises the `.local` file and refreshes the jail list.
- Inactive jails without a `.local` file are unaffected and show no extra button.
**Root cause**
Both `backend/app/routers/config.py` and `backend/app/routers/file_config.py` are mounted with the prefix `/api/config` (see lines 107 and 63 respectively). Both define a `GET /actions/{name}` route:
- `config.py` → returns `ActionConfig` (parsed detail)
- `file_config.py` → returns `ConfFileContent` (raw file text)
In `backend/app/main.py`, `config.router` is registered on line 402 and `file_config.router` on line 403. FastAPI matches the first registered route, so the raw-content endpoint is permanently shadowed.
The filters feature already solved the same conflict by using distinct paths (`/filters/{name}/raw` for raw and `/filters/{name}` for parsed). Actions must follow the same pattern.
**Fix — backend (`backend/app/routers/file_config.py`)**
Rename the two action raw-file routes:
| Old path | New path |
|---|---|
| `GET /actions/{name}` | `GET /actions/{name}/raw` |
| `PUT /actions/{name}` | `PUT /actions/{name}/raw` |
Update the module-level docstring comment block at the top of `file_config.py` to reflect the new paths.
**Fix — frontend (`frontend/src/api/endpoints.ts`)**
Add a new helper alongside the existing `configAction` entry:
```ts
configActionRaw: (name: string): string => `/config/actions/${encodeURIComponent(name)}/raw`,
```
**Fix — frontend (`frontend/src/api/config.ts`)**
Change `fetchActionFile` and `updateActionFile` to call `ENDPOINTS.configActionRaw(name)` instead of `ENDPOINTS.configAction(name)`.
**No changes needed elsewhere.** `ActionsTab.tsx` already passes `fetchActionFile` / `updateActionFile` into `RawConfigSection` via `fetchRaw` / `saveRaw`; the resolved URL is the only thing that needs to change.
---
## ✅ Task: Remove the "fail2ban Stopped After Jail Activation" Recovery Banner
## Rename dev jail `bangui-sim` → `manual-Jail` — DONE
**Context:**
`frontend/src/components/common/RecoveryBanner.tsx` renders a full-page banner with the heading *"fail2ban Stopped After Jail Activation"* and the body *"fail2ban stopped responding after activating jail `{name}`. The jail's configuration may be invalid."* together with "Disable & Restart" and "View Logs" action buttons. This banner interrupts the UI even when the backend has already handled the rollback automatically.
**Summary:** Renamed `jail.d/bangui-sim.conf``manual-Jail.conf` and `filter.d/bangui-sim.conf``manual-Jail.conf` (via `git mv`), updated all internal references. Updated `check_ban_status.sh`, `simulate_failed_logins.sh`, and `fail2ban-dev-config/README.md` to replace all `bangui-sim` references with `manual-Jail`.
**Goal:**
Remove the `RecoveryBanner` component and all its mount points from the application. Any state that was used exclusively to drive this banner (e.g. a `fail2banStopped` flag or related context) should also be removed. If the underlying crash-detection logic is still needed for other features, keep that logic but detach it from the banner render path.
**Scope**
This is purely a Docker development-environment change. The frontend never hardcodes jail names; it reads them dynamically from the API. Only the files listed below need editing.
**Acceptance criteria:**
- The full-page banner no longer appears under any circumstances.
- No dead code or orphaned state references remain after the removal.
- All existing tests that reference `RecoveryBanner` are updated or removed accordingly.
**Files to update**
1. **`Docker/fail2ban-dev-config/fail2ban/jail.d/bangui-sim.conf`**
- Rename the file to `manual-Jail.conf`.
- Change the section header from `[bangui-sim]` to `[manual-Jail]`.
- Change `filter = bangui-sim` to `filter = manual-Jail`.
- Update the file-header comment ("BanGUI — Simulated authentication failure jail" line and any other references to `bangui-sim`).
2. **`Docker/fail2ban-dev-config/fail2ban/filter.d/bangui-sim.conf`**
- Rename the file to `manual-Jail.conf`.
- Update any internal comments that mention `bangui-sim`.
3. **`Docker/check_ban_status.sh`**
- Change `readonly JAIL="bangui-sim"` to `readonly JAIL="manual-Jail"`.
- Update the file-header comment block that references `bangui-sim`.
4. **`Docker/simulate_failed_logins.sh`**
- Update all comments that mention `bangui-sim` or `bangui-auth` to refer to `manual-Jail` instead.
- Do **not** change the log-line format string (`bangui-auth: authentication failure from <IP>`) unless the filter's `failregex` in the renamed `manual-Jail.conf` is also updated to match the new prefix; keep them in sync.
5. **`Docker/fail2ban-dev-config/README.md`**
- Replace every occurrence of `bangui-sim` with `manual-Jail`.
After renaming, run `docker compose -f Docker/compose.debug.yml restart fail2ban` and verify with `bash Docker/check_ban_status.sh` that the jail is active under its new name.
---
## ✅ Task: Fix Activation Failure Rollback — Actually Delete the `.local` File
## Bug Fix: Config screen content pane does not update when switching jails — DONE
**Context:**
When jail activation fails after the `jail.d/{name}.local` file has already been written (i.e. fail2ban reloaded but the jail never came up, or fail2ban became unresponsive), `_rollback_activation_async()` in `backend/app/services/config_file_service.py` is supposed to restore the pre-activation state. The frontend then displays *"Activation Failed — System Recovered"* with the message *"Activation of jail `{name}` failed. The server has been automatically recovered."*
**Summary:** Added `key={selectedActiveJail.name}` to `JailConfigDetail` and `key={selectedInactiveJail.name}` to `InactiveJailDetail` in `JailsTab.tsx`, forcing React to unmount and remount the detail component on jail selection changes.
In practice, recovery does not happen: the `.local` file remains on disk with `enabled = true`, leaving fail2ban in a broken state on next restart. The frontend misleadingly reports success.
**Problem**
In the *Jails* tab of the Config screen, clicking a jail name in the left-hand list correctly highlights the new selection, but the right-hand content pane continues to show the previously selected jail (e.g. selecting `blocklist-import` after `manual-Jail` still displays `manual-Jail`'s configuration).
**Goal:**
Fix `_rollback_activation_async()` so that it reliably removes (or reverts) the `.local` file whenever activation fails:
**Root cause**
In `frontend/src/components/config/JailsTab.tsx`, the child components rendered by `ConfigListDetail` are not given a `key` prop:
1. If the `.local` file did not exist before activation, **delete** `jail.d/{name}.local` outright.
2. If it existed before activation (e.g. previously had `enabled = false`), **restore** its original content atomically (temp-file rename pattern already used elsewhere in the service).
3. After deleting/restoring the file, attempt a `reload_all` socket command so fail2ban picks up the reverted state.
4. Only set `recovered = true` in the `JailActivationResponse` once all three steps above have actually succeeded. If any step fails, set `recovered = false` and log the error.
5. On the frontend side, the *"Activation Failed — System Recovered"* `MessageBar` in `ActivateJailDialog.tsx` should only be shown when the backend actually returns `recovered = true`. The current misleading message should be replaced with a more actionable one when `recovered = false`.
```tsx
{selectedActiveJail !== undefined ? (
<JailConfigDetail
jail={selectedActiveJail} // no key prop
...
/>
) : selectedInactiveJail !== undefined ? (
<InactiveJailDetail
jail={selectedInactiveJail} // no key prop
...
/>
) : null}
```
**Acceptance criteria:**
- After a failed activation, `jail.d/{name}.local` is either absent or contains its pre-activation content.
- `recovered: true` is only returned when the rollback fully succeeded.
- The UI message accurately reflects the actual recovery state.
- A test in `backend/tests/test_services/` covers the rollback path, asserting the file is absent/reverted and the response flag is correct.
When the user switches between two jails of the same type (both active or both inactive), React reuses the existing component instance and only updates its props. Any internal state derived from the previous jail — including the `loadedRef` guard inside every nested `RawConfigSection` — is never reset. As a result, forms still show the old jail's values and the raw-config section refuses to re-fetch because `loadedRef.current` is already `true`.
Compare with `ActionsTab.tsx`, where `ActionDetail` correctly uses `key={selectedAction.name}`:
```tsx
<ActionDetail
key={selectedAction.name} // ← forces remount on action change
action={selectedAction}
...
/>
```
**Fix — `frontend/src/components/config/JailsTab.tsx`**
Add `key` props to both detail components so React unmounts and remounts them whenever the selected jail changes:
```tsx
{selectedActiveJail !== undefined ? (
<JailConfigDetail
key={selectedActiveJail.name}
jail={selectedActiveJail}
onSave={updateJail}
onDeactivate={() => { handleDeactivate(selectedActiveJail.name); }}
/>
) : selectedInactiveJail !== undefined ? (
<InactiveJailDetail
key={selectedInactiveJail.name}
jail={selectedInactiveJail}
onActivate={() => { setActivateTarget(selectedInactiveJail); }}
onDeactivate={
selectedInactiveJail.has_local_override
? (): void => { handleDeactivateInactive(selectedInactiveJail.name); }
: undefined
}
/>
) : null}
```
No other files need changing. The `key` change is the minimal, isolated fix.
---

View File

@@ -14,8 +14,8 @@ Endpoints:
* ``GET /api/config/filters/{name}/parsed`` — parse a filter file into a structured model
* ``PUT /api/config/filters/{name}/parsed`` — update a filter file from a structured model
* ``GET /api/config/actions`` — list all action files
* ``GET /api/config/actions/{name}`` — get one action file (with content)
* ``PUT /api/config/actions/{name}`` — update an action file
* ``GET /api/config/actions/{name}/raw`` — get one action file (raw content)
* ``PUT /api/config/actions/{name}/raw`` — update an action file (raw content)
* ``POST /api/config/actions`` — create a new action file
* ``GET /api/config/actions/{name}/parsed`` — parse an action file into a structured model
* ``PUT /api/config/actions/{name}/parsed`` — update an action file from a structured model
@@ -460,7 +460,7 @@ async def list_action_files(
@router.get(
"/actions/{name}",
"/actions/{name}/raw",
response_model=ConfFileContent,
summary="Return an action definition file with its content",
)
@@ -496,7 +496,7 @@ async def get_action_file(
@router.put(
"/actions/{name}",
"/actions/{name}/raw",
status_code=status.HTTP_204_NO_CONTENT,
summary="Update an action definition file",
)

View File

@@ -377,6 +377,102 @@ class TestCreateActionFile:
assert resp.json()["name"] == "myaction"
# ---------------------------------------------------------------------------
# GET /api/config/actions/{name}/raw
# ---------------------------------------------------------------------------
class TestGetActionFileRaw:
"""Tests for ``GET /api/config/actions/{name}/raw``."""
async def test_200_returns_content(self, file_config_client: AsyncClient) -> None:
with patch(
"app.routers.file_config.file_config_service.get_action_file",
AsyncMock(return_value=_conf_file_content("iptables")),
):
resp = await file_config_client.get("/api/config/actions/iptables/raw")
assert resp.status_code == 200
assert resp.json()["name"] == "iptables"
async def test_404_not_found(self, file_config_client: AsyncClient) -> None:
with patch(
"app.routers.file_config.file_config_service.get_action_file",
AsyncMock(side_effect=ConfigFileNotFoundError("missing")),
):
resp = await file_config_client.get("/api/config/actions/missing/raw")
assert resp.status_code == 404
async def test_503_on_config_dir_error(
self, file_config_client: AsyncClient
) -> None:
with patch(
"app.routers.file_config.file_config_service.get_action_file",
AsyncMock(side_effect=ConfigDirError("no dir")),
):
resp = await file_config_client.get("/api/config/actions/iptables/raw")
assert resp.status_code == 503
# ---------------------------------------------------------------------------
# PUT /api/config/actions/{name}/raw
# ---------------------------------------------------------------------------
class TestUpdateActionFileRaw:
"""Tests for ``PUT /api/config/actions/{name}/raw``."""
async def test_204_on_success(self, file_config_client: AsyncClient) -> None:
with patch(
"app.routers.file_config.file_config_service.write_action_file",
AsyncMock(return_value=None),
):
resp = await file_config_client.put(
"/api/config/actions/iptables/raw",
json={"content": "[Definition]\nactionban = iptables -I INPUT -s <ip> -j DROP\n"},
)
assert resp.status_code == 204
async def test_400_write_error(self, file_config_client: AsyncClient) -> None:
with patch(
"app.routers.file_config.file_config_service.write_action_file",
AsyncMock(side_effect=ConfigFileWriteError("disk full")),
):
resp = await file_config_client.put(
"/api/config/actions/iptables/raw",
json={"content": "x"},
)
assert resp.status_code == 400
async def test_404_not_found(self, file_config_client: AsyncClient) -> None:
with patch(
"app.routers.file_config.file_config_service.write_action_file",
AsyncMock(side_effect=ConfigFileNotFoundError("missing")),
):
resp = await file_config_client.put(
"/api/config/actions/missing/raw",
json={"content": "x"},
)
assert resp.status_code == 404
async def test_400_invalid_name(self, file_config_client: AsyncClient) -> None:
with patch(
"app.routers.file_config.file_config_service.write_action_file",
AsyncMock(side_effect=ConfigFileNameError("bad/../name")),
):
resp = await file_config_client.put(
"/api/config/actions/escape/raw",
json={"content": "x"},
)
assert resp.status_code == 400
# ---------------------------------------------------------------------------
# POST /api/config/jail-files
# ---------------------------------------------------------------------------

View File

@@ -263,14 +263,14 @@ export async function fetchActionFiles(): Promise<ConfFilesResponse> {
}
export async function fetchActionFile(name: string): Promise<ConfFileContent> {
return get<ConfFileContent>(ENDPOINTS.configAction(name));
return get<ConfFileContent>(ENDPOINTS.configActionRaw(name));
}
export async function updateActionFile(
name: string,
req: ConfFileUpdateRequest
): Promise<void> {
await put<undefined>(ENDPOINTS.configAction(name), req);
await put<undefined>(ENDPOINTS.configActionRaw(name), req);
}
export async function createActionFile(

View File

@@ -104,6 +104,7 @@ export const ENDPOINTS = {
`/config/jails/${encodeURIComponent(jailName)}/action/${encodeURIComponent(actionName)}`,
configActions: "/config/actions",
configAction: (name: string): string => `/config/actions/${encodeURIComponent(name)}`,
configActionRaw: (name: string): string => `/config/actions/${encodeURIComponent(name)}/raw`,
configActionParsed: (name: string): string =>
`/config/actions/${encodeURIComponent(name)}/parsed`,