refactoring-backend #3
@@ -72,13 +72,8 @@ Supporting documentation you must know and respect:
|
||||
|
||||
Repeat the following cycle for every task. Do not skip steps.
|
||||
|
||||
### Step 1 — Pick a Task
|
||||
|
||||
- Open `tasks.md` and pick the next unfinished task (highest priority first).
|
||||
- Mark the task as **in progress**.
|
||||
- Read the task description thoroughly. Understand the expected outcome before proceeding.
|
||||
|
||||
### Step 2 — Plan Your Steps
|
||||
### Step 1 — Plan Your Steps
|
||||
|
||||
- Break the task into concrete implementation steps.
|
||||
- Identify which files need to be created, modified, or deleted.
|
||||
@@ -86,7 +81,7 @@ Repeat the following cycle for every task. Do not skip steps.
|
||||
- Identify edge cases and error scenarios.
|
||||
- Write down your plan before touching any code.
|
||||
|
||||
### Step 3 — Write Code
|
||||
### Step 2 — Write Code
|
||||
|
||||
- Implement the feature or fix following the plan.
|
||||
- Follow all rules from the relevant development docs:
|
||||
@@ -97,14 +92,14 @@ Repeat the following cycle for every task. Do not skip steps.
|
||||
- Write clean, well-structured, fully typed code.
|
||||
- Keep commits atomic — one logical change per commit.
|
||||
|
||||
### Step 4 — Add Logging
|
||||
### Step 3 — Add Logging
|
||||
|
||||
- Add structured log statements at key points in new or modified code.
|
||||
- Backend: use **structlog** with contextual key-value pairs — never `print()`.
|
||||
- Log at appropriate levels: `info` for operational events, `warning` for recoverable issues, `error` for failures.
|
||||
- Never log sensitive data (passwords, tokens, session IDs).
|
||||
|
||||
### Step 5 — Write Tests
|
||||
### Step 4 — Write Tests
|
||||
|
||||
- Write tests for every new or changed piece of functionality.
|
||||
- Backend: use `pytest` + `pytest-asyncio` + `httpx.AsyncClient`. See [Backend-Development.md § 9](Backend-Development.md).
|
||||
@@ -113,24 +108,24 @@ Repeat the following cycle for every task. Do not skip steps.
|
||||
- Mock external dependencies — tests must never touch real infrastructure.
|
||||
- Follow the naming pattern: `test_<unit>_<scenario>_<expected>`.
|
||||
|
||||
### Step 6 — Review Your Code
|
||||
### Step 5 — Review Your Code
|
||||
|
||||
Run a thorough self-review before considering the task done. Check **all** of the following:
|
||||
|
||||
#### 6.1 — Warnings and Errors
|
||||
#### 5.1 — Warnings and Errors
|
||||
|
||||
- Backend: run `ruff check` and `mypy --strict` (or `pyright --strict`). Fix every warning and error.
|
||||
- Frontend: run `tsc --noEmit` and `eslint`. Fix every warning and error.
|
||||
- Zero warnings, zero errors — no exceptions.
|
||||
|
||||
#### 6.2 — Test Coverage
|
||||
#### 5.2 — Test Coverage
|
||||
|
||||
- Run the test suite with coverage enabled.
|
||||
- Aim for **>80 % line coverage** overall.
|
||||
- Critical paths (auth, banning, scheduling, API endpoints) must be **100 %** covered.
|
||||
- If coverage is below the threshold, write additional tests before proceeding.
|
||||
|
||||
#### 6.3 — Coding Principles
|
||||
#### 5.3 — Coding Principles
|
||||
|
||||
Verify your code against the coding principles defined in [Backend-Development.md § 13](Backend-Development.md) and [Web-Development.md](Web-Development.md):
|
||||
|
||||
@@ -141,7 +136,7 @@ Verify your code against the coding principles defined in [Backend-Development.m
|
||||
- [ ] **KISS** — The simplest correct solution is used. No over-engineering.
|
||||
- [ ] **Type Safety** — All types are explicit. No `any` / `Any`. No `# type: ignore` without justification.
|
||||
|
||||
#### 6.4 — Architecture Compliance
|
||||
#### 5.4 — Architecture Compliance
|
||||
|
||||
Verify against [Architekture.md](Architekture.md) and the project structure rules:
|
||||
|
||||
@@ -153,7 +148,7 @@ Verify against [Architekture.md](Architekture.md) and the project structure rule
|
||||
- [ ] Pydantic models separate request, response, and domain shapes.
|
||||
- [ ] Frontend types live in `types/`, not scattered across components.
|
||||
|
||||
### Step 7 — Update Documentation
|
||||
### Step 6 — Update Documentation
|
||||
|
||||
- If your change introduces new features, new endpoints, new components, or changes existing behaviour, update the relevant docs:
|
||||
- [Features.md](Features.md) — if feature behaviour changed.
|
||||
@@ -161,51 +156,6 @@ Verify against [Architekture.md](Architekture.md) and the project structure rule
|
||||
- [Backend-Development.md](Backend-Development.md) or [Web-Development.md](Web-Development.md) — if new conventions were established.
|
||||
- Keep documentation accurate and in sync with the code. Outdated docs are worse than no docs.
|
||||
|
||||
### Step 8 — Mark Task Complete
|
||||
|
||||
- Open `tasks.md` and mark the task as **done**.
|
||||
- Add a brief summary of what was implemented or changed.
|
||||
|
||||
### Step 9 — Commit
|
||||
|
||||
- Stage all changed files.
|
||||
- Write a commit message in **imperative tense**, max 72 characters for the subject line.
|
||||
- Good: `Add jail reload endpoint`
|
||||
- Bad: `added stuff` / `WIP` / `fix`
|
||||
- If the change is large, include a body explaining **why**, not just **what**.
|
||||
- Branch naming: `feature/<short-description>`, `fix/<short-description>`, `chore/<short-description>`.
|
||||
- Ensure the commit passes: linter, type checker, all tests.
|
||||
|
||||
### Step 10 — Next Task
|
||||
|
||||
- Return to **Step 1** and pick the next task.
|
||||
|
||||
---
|
||||
|
||||
## 4. Workflow Summary
|
||||
|
||||
```
|
||||
┌─────────────────────────────────────────┐
|
||||
│ 1. Pick task from tasks.md │
|
||||
│ 2. Plan your steps │
|
||||
│ 3. Write code │
|
||||
│ 4. Add logging │
|
||||
│ 5. Write tests │
|
||||
│ 6. Review your code │
|
||||
│ ├── 6.1 Check warnings & errors │
|
||||
│ ├── 6.2 Check test coverage │
|
||||
│ ├── 6.3 Check coding principles │
|
||||
│ └── 6.4 Check architecture │
|
||||
│ 7. Update documentation if needed │
|
||||
│ 8. Mark task complete in tasks.md │
|
||||
│ 9. Git commit │
|
||||
│ 10. Pick next task ──────── loop ───┐ │
|
||||
│ ▲ │ │
|
||||
│ └───────────────────────────┘ │
|
||||
└─────────────────────────────────────────┘
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 5. When You Are Stuck
|
||||
|
||||
|
||||
@@ -16,4 +16,5 @@ This document catalogues architecture violations, code smells, and structural is
|
||||
- Moved config file exceptions (`ConfigDirError`, `ConfigFileNotFoundError`, `ConfigFileExistsError`, `ConfigFileWriteError`, `ConfigFileNameError`) from `backend/app/services/raw_config_io_service.py` into `backend/app/exceptions.py`. Updated router and tests to import the shared domain exceptions from `app.exceptions`.
|
||||
- Added global domain exception handlers to `backend/app/main.py` so domain exceptions like `JailNotFoundError`, `ConfigValidationError`, and `ConfigWriteError` map consistently to 404, 400, and 500 responses.
|
||||
- Fixed stale activation tracking in `backend/app/routers/jail_config.py` by recording `last_activation` only after a successful jail activation and preventing a failed activation attempt from leaving a stale runtime state record.
|
||||
- Fixed infinite re-fetch loop in `frontend/src/hooks/useJailConfigs.ts` by wrapping the `onSuccess` callback in `useCallback` with empty dependencies. The bug occurred because `useListData` includes `onSuccess` in its internal `refresh` function's dependency array; an inline callback created a new reference on each render, causing `refresh` to be recreated, which triggered the `useEffect` again, leading to an unbounded fetch loop. Callers of `useListData` must always wrap `onSuccess` callbacks in `useCallback` to maintain reference stability.
|
||||
|
||||
|
||||
@@ -1,48 +1,12 @@
|
||||
|
||||
### TASK-SEC-01 — Open Redirect in LoginPage via `?next=` Parameter
|
||||
### ✅ TASK-BUG-01 — Infinite Re-Fetch Loop in `useJailConfigs` — DONE
|
||||
|
||||
**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.
|
||||
**Fix Summary**
|
||||
Wrapped the `onSuccess` callback in `useCallback` with empty dependencies in `frontend/src/hooks/useJailConfigs.ts` (lines 33-35). The inline callback was creating a new reference on every render, which caused `useListData`'s internal `refresh` function to be recreated (since `onSuccess` is in its deps), which triggered the `useEffect` again, causing an infinite fetch loop.
|
||||
|
||||
**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.
|
||||
Added comprehensive test coverage in `frontend/src/hooks/__tests__/useJailConfigs.test.ts` to verify the hook no longer triggers infinite refetches. Updated `Docs/Refactoring.md` with documentation explaining the `onSuccess` stability requirement for all `useListData` callers.
|
||||
|
||||
**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.
|
||||
Commit: `de8af09a3da36dbf24b56fa28656673b232b5e91`
|
||||
|
||||
---
|
||||
|
||||
|
||||
@@ -89,8 +89,9 @@ async Task<string> RunCopilot(IEnumerable<string> extraArgs, string prompt)
|
||||
}
|
||||
|
||||
// ── Main loop ─────────────────────────────────────────────────────────────────
|
||||
foreach (var item in items)
|
||||
for (int i = 0; i < items.Count; i++)
|
||||
{
|
||||
var item = items[i];
|
||||
if (cts.IsCancellationRequested) break;
|
||||
|
||||
Console.WriteLine();
|
||||
@@ -122,6 +123,11 @@ foreach (var item in items)
|
||||
Console.WriteLine("\n[runner] Task confirmed. Making git commit...\n");
|
||||
await RunCopilot(Enumerable.Empty<string>(), "make git commit");
|
||||
if (cts.IsCancellationRequested) break;
|
||||
|
||||
// Step 5 — remove completed task from Tasks.md
|
||||
var remaining = items.Skip(i + 1).ToList();
|
||||
File.WriteAllText(tasksFile, string.Join("\n\n---\n\n", remaining));
|
||||
Console.WriteLine("[runner] Removed completed task from Tasks.md");
|
||||
}
|
||||
|
||||
Console.WriteLine("\n[runner] Finished.");
|
||||
|
||||
88
frontend/src/hooks/__tests__/useJailConfigs.test.ts
Normal file
88
frontend/src/hooks/__tests__/useJailConfigs.test.ts
Normal file
@@ -0,0 +1,88 @@
|
||||
import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||
import { renderHook, act } from "@testing-library/react";
|
||||
import * as configApi from "../../api/config";
|
||||
import { useJailConfigs } from "../useJailConfigs";
|
||||
|
||||
vi.mock("../../api/config");
|
||||
|
||||
describe("useJailConfigs", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
});
|
||||
|
||||
it("calls fetchJailConfigs only once on mount", async () => {
|
||||
vi.mocked(configApi.fetchJailConfigs).mockResolvedValue({
|
||||
jails: [
|
||||
{
|
||||
name: "sshd",
|
||||
enabled: true,
|
||||
active: true,
|
||||
backend: "systemd",
|
||||
filename: "/etc/fail2ban/jail.d/sshd.conf",
|
||||
source_file: "/etc/fail2ban/jail.d/sshd.conf",
|
||||
last_activation: "2024-01-01T00:00:00Z",
|
||||
bantime: 600,
|
||||
findtime: 600,
|
||||
maxretry: 5,
|
||||
action: [],
|
||||
logpath: [],
|
||||
port: [],
|
||||
ignoreself: false,
|
||||
filter: "sshd",
|
||||
destemail: "",
|
||||
sendername: "",
|
||||
action_d_files: [],
|
||||
ignoreip: [],
|
||||
ignorecommand: "",
|
||||
banaction: "",
|
||||
banaction_allports: false,
|
||||
},
|
||||
],
|
||||
total: 1,
|
||||
});
|
||||
|
||||
const { result } = renderHook(() => useJailConfigs());
|
||||
|
||||
expect(result.current.loading).toBe(true);
|
||||
|
||||
await act(async () => {
|
||||
await Promise.resolve();
|
||||
});
|
||||
|
||||
expect(configApi.fetchJailConfigs).toHaveBeenCalledTimes(1);
|
||||
expect(result.current.jails).toHaveLength(1);
|
||||
expect(result.current.total).toBe(1);
|
||||
expect(result.current.loading).toBe(false);
|
||||
});
|
||||
|
||||
it("does not trigger infinite refetch with stable onSuccess", async () => {
|
||||
vi.mocked(configApi.fetchJailConfigs).mockResolvedValue({
|
||||
jails: [],
|
||||
total: 0,
|
||||
});
|
||||
|
||||
const { rerender } = renderHook(() => useJailConfigs());
|
||||
|
||||
await act(async () => {
|
||||
await Promise.resolve();
|
||||
});
|
||||
|
||||
const callCountAfterMount = vi.mocked(configApi.fetchJailConfigs).mock.calls.length;
|
||||
|
||||
// Re-render multiple times to ensure no additional fetches are triggered
|
||||
rerender();
|
||||
await act(async () => {
|
||||
await Promise.resolve();
|
||||
});
|
||||
|
||||
rerender();
|
||||
await act(async () => {
|
||||
await Promise.resolve();
|
||||
});
|
||||
|
||||
const callCountAfterRerender = vi.mocked(configApi.fetchJailConfigs).mock.calls.length;
|
||||
|
||||
// Should still be called only once (on mount)
|
||||
expect(callCountAfterRerender).toBe(callCountAfterMount);
|
||||
});
|
||||
});
|
||||
@@ -30,13 +30,15 @@ export function useJailConfigs(): UseJailConfigsResult {
|
||||
|
||||
const selector = useCallback((response: JailConfigListResponse) => response.jails, []);
|
||||
|
||||
const onSuccess = useCallback((response: JailConfigListResponse) => {
|
||||
setTotal(response.total);
|
||||
}, []);
|
||||
|
||||
const { items: jails, loading, error, refresh } = useListData<JailConfigListResponse, JailConfig>({
|
||||
fetcher,
|
||||
selector,
|
||||
errorMessage: "Failed to fetch jail configs",
|
||||
onSuccess: (response) => {
|
||||
setTotal(response.total);
|
||||
},
|
||||
onSuccess,
|
||||
});
|
||||
|
||||
const updateJail = useCallback(
|
||||
|
||||
Reference in New Issue
Block a user