diff --git a/Docs/Instructions.md b/Docs/Instructions.md index 0ebf211..76b0745 100644 --- a/Docs/Instructions.md +++ b/Docs/Instructions.md @@ -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___`. -### 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/`, `fix/`, `chore/`. -- 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 diff --git a/Docs/Refactoring.md b/Docs/Refactoring.md index 58df3f9..b4ce2a8 100644 --- a/Docs/Refactoring.md +++ b/Docs/Refactoring.md @@ -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. diff --git a/Docs/Tasks.md b/Docs/Tasks.md index 55a4aab..25ef749 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -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` --- diff --git a/Docs/runner.csx b/Docs/runner.csx index ec03273..02125c3 100644 --- a/Docs/runner.csx +++ b/Docs/runner.csx @@ -89,8 +89,9 @@ async Task RunCopilot(IEnumerable 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(), "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."); diff --git a/frontend/src/hooks/__tests__/useJailConfigs.test.ts b/frontend/src/hooks/__tests__/useJailConfigs.test.ts new file mode 100644 index 0000000..93f98fb --- /dev/null +++ b/frontend/src/hooks/__tests__/useJailConfigs.test.ts @@ -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); + }); +}); diff --git a/frontend/src/hooks/useJailConfigs.ts b/frontend/src/hooks/useJailConfigs.ts index 69bc1b3..28293a6 100644 --- a/frontend/src/hooks/useJailConfigs.ts +++ b/frontend/src/hooks/useJailConfigs.ts @@ -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({ fetcher, selector, errorMessage: "Failed to fetch jail configs", - onSuccess: (response) => { - setTotal(response.total); - }, + onSuccess, }); const updateJail = useCallback(