Lukas
e08a16c7dd
Extracted the monolithic import_source() function (776 lines) into focused, testable components with clear single responsibilities: - BlocklistDownloader: HTTP download with exponential backoff retry logic * Handles transient failures (429, 5xx errors, timeouts) * Configurable retry attempts and backoff strategy * 93% test coverage - BlocklistParser: Parse and validate IP addresses * Extract valid IPv4/IPv6 addresses from text * Skip CIDRs and malformed entries gracefully * Separate parsing from validation concerns * 100% test coverage - BanExecutor: Ban execution with error handling * Ban IPs via fail2ban socket * Stop on JailNotFoundError (jail doesn't exist) * Continue on JailOperationError (individual ban failures) * 100% test coverage - BlocklistImportWorkflow: Thin orchestrator * Coordinates the download → parse → ban → log flow * Pre-warms geo cache with newly banned IPs * 96% test coverage - blocklist_service.py: Maintains public API * Source CRUD (create, read, update, delete) * URL validation and preview functionality * Scheduling configuration and import triggers * 92% test coverage Benefits: * Each component is independently testable with mock dependencies * Error handling is explicit and localized * Components can evolve independently * Logging is contextual and clear * Retry and transient error handling are isolated Testing: * All 36 existing blocklist_service tests pass * All 13 blocklist import task tests pass * Added 17 comprehensive component unit tests * Combined 96%+ coverage on new modules * Zero type errors in new code Documentation: * Updated Refactoring.md with detailed architecture notes * Added component architecture diagram to Architekture.md * Documented ownership and responsibilities of each component Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
23 lines
3.8 KiB
Markdown
23 lines
3.8 KiB
Markdown
# BanGUI — Architecture Issues & Refactoring Plan
|
|
|
|
This document catalogues architecture violations, code smells, and structural issues found during a full project review. Issues are grouped by category and prioritised.
|
|
|
|
---
|
|
|
|
## Security Fixes
|
|
|
|
- Fixed open redirect vulnerability in `frontend/src/pages/LoginPage.tsx` by validating the `?next=` parameter to ensure it is a relative path (starts with `/` but not `//`). The validation regex `/^\/(?!\/)/.test(next)` prevents protocol-relative URLs and external redirects. Invalid paths fall back to `"/"`.
|
|
|
|
---
|
|
|
|
## Completed Refactors
|
|
|
|
- Moved `Fail2BanConnectionError` and `Fail2BanProtocolError` from `backend/app/utils/fail2ban_client.py` into `backend/app/exceptions.py`. Updated all router, service, and test call sites to import these domain exceptions from `app.exceptions` and retained backward compatibility through re-exporting in `app.utils.fail2ban_client`.
|
|
- 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.
|
|
- **T-11 — Repository module-as-Protocol structural type-safety:** Resolved the fragile `cast()` pattern where repository modules were loosely typed against Protocol interfaces. Created a **validation script** (`backend/scripts/validate_repository_protocols.py`) that runs at CI time to ensure all repository modules satisfy their Protocol interfaces. Fixed signature mismatches in `protocols.py` to match actual implementations in `session_repo`, `settings_repo`, `blocklist_repo`, `import_log_repo`, `geo_cache_repo`, `history_archive_repo`, and `fail2ban_db_repo` (correcting return types like `dict[str, Any]` vs `dict[str, object]`, `Sequence` vs `Iterable`, and typed models). Updated `backend/app/dependencies.py` with explicit documentation linking each repository provider to the pattern explained in Backend-Development.md § 13.7.1. **Option B (minimal):** Instead of refactoring to class-based repositories (Option A), the pattern is now formally documented and validated, preventing silent breakage.
|
|
- **T-3 — Blocklist import flow refactoring:** Extracted the monolithic `import_source()` function (776 lines with mixed responsibilities) into focused, testable components. Created `BlocklistDownloader` (HTTP download with retry logic), `BlocklistParser` (parsing and validation), `BanExecutor` (ban execution with error handling), and `BlocklistImportWorkflow` (thin orchestrator). This separation improves testability, evolution, and error handling. Each component has a single responsibility and clear boundaries. All 53 existing tests pass; added 17 new component unit tests achieving 96%+ coverage on new modules.
|
|
|