feat: troppa roba davvero
This commit is contained in:
gabdevele
@@ -0,0 +1,239 @@
|
||||
# thePlug Codebase Audit
|
||||
|
||||
Date: 2026-05-06
|
||||
|
||||
Checks run:
|
||||
- `cd backend && uv run pytest -q`
|
||||
- `cd backend && uv run ruff check .`
|
||||
- `cd backend && uv run basedpyright app`
|
||||
- `npm --prefix frontend run build`
|
||||
|
||||
Summary:
|
||||
- Functional checks currently pass: backend tests are green, Ruff is clean, frontend builds.
|
||||
- Structural quality is much weaker than the green CI surface suggests.
|
||||
- The biggest risks are runtime mutation safety around Docker compose rewrites, false confidence in the backend test suite, packaging/type-check fragility, and a very large amount of stateful coordination concentrated in single files.
|
||||
|
||||
## Findings
|
||||
|
||||
### Critical
|
||||
|
||||
#### 1. Docker port rewrite is not exception-safe and can leave challenge services mutated on disk
|
||||
- Files:
|
||||
- `backend/app/integrations/docker/editor.py:14-51`
|
||||
- `backend/app/services/mitmproxy/manager.py:195-236`
|
||||
- Why it matters:
|
||||
- `change_port()` rewrites the compose file in place, then runs `docker compose up -d`.
|
||||
- There is no `try/finally` around the file mutation path.
|
||||
- If subprocess startup, YAML serialization, file writes, or any unexpected exception happens after the compose file has been rewritten, the original file is not guaranteed to be restored.
|
||||
- In practice, a failed proxy start/stop/recovery can leave the challenge service permanently remapped until manual cleanup.
|
||||
- Recommended fix:
|
||||
- Make compose rewrite transactional: read once, write temp file, atomically replace, and always restore in `finally` on failure.
|
||||
- Treat backup creation/restoration as part of a single transaction, not best effort.
|
||||
- Add integration tests that force failures between rewrite and `docker compose up`.
|
||||
|
||||
#### 2. The backend test suite gives false confidence because most API behavior is mocked at the router seam
|
||||
- Files:
|
||||
- `backend/tests/conftest.py:40-69`
|
||||
- `backend/tests/api/test_proxy.py`
|
||||
- Why it matters:
|
||||
- The API suite mounts a bespoke `FastAPI()` app instead of the real application lifecycle.
|
||||
- Authentication is bypassed through dependency overrides.
|
||||
- Core router behavior is tested by monkeypatching repository/service functions directly on the module under test.
|
||||
- This catches serialization and status codes, but misses real dependency wiring, startup/shutdown behavior, Mongo access, WebSocket/auth interplay, and real process/runtime interactions.
|
||||
- Recommended fix:
|
||||
- Split tests into unit, integration, and system/perf layers.
|
||||
- Keep monkeypatch for narrow fault injection only.
|
||||
- Add real-app integration tests with actual lifespan, real Mongo-backed repositories, and realistic temp runtime directories.
|
||||
|
||||
### High
|
||||
|
||||
#### 3. Log streaming drops data after truncation or rotation
|
||||
- File:
|
||||
- `backend/app/api/proxy/router.py:188-213`
|
||||
- Why it matters:
|
||||
- On truncation, the stream sets `offset = size`.
|
||||
- If the log file is rotated or recreated with fresh content smaller than the old offset, the new bytes already present in the new file are skipped forever.
|
||||
- This is exactly the kind of subtle bug that makes operators distrust live logs.
|
||||
- Recommended fix:
|
||||
- When `size < offset`, reset to `0` or resend a fresh snapshot and continue from the new file start.
|
||||
- Add a regression test that simulates truncation/recreation with immediate new log bytes.
|
||||
|
||||
#### 4. The health monitor interval drifts under load
|
||||
- File:
|
||||
- `backend/app/services/mitmproxy/health_monitor.py:30-49`
|
||||
- Why it matters:
|
||||
- The loop sleeps after the cycle completes.
|
||||
- Real interval becomes `check duration + HEALTHCHECK_INTERVAL_SECONDS`.
|
||||
- As proxy count or timeout frequency rises, checks become slower and the dashboard silently becomes stale.
|
||||
- Recommended fix:
|
||||
- Switch to fixed-schedule cadence using monotonic deadlines.
|
||||
- Track cycle duration and lag explicitly for observability.
|
||||
|
||||
#### 5. Port reassignment in reverse mode has a race window and no reservation mechanism
|
||||
- Files:
|
||||
- `backend/app/integrations/docker/ports.py:4-13`
|
||||
- `backend/app/services/mitmproxy/manager.py:195-215`
|
||||
- Why it matters:
|
||||
- `get_random_port()` discovers a free port and returns it, but does not reserve it.
|
||||
- Another process can claim the port before Docker rebinds the service.
|
||||
- The failure mode is intermittent and hard to reproduce, which makes it a good candidate for production-only flakiness.
|
||||
- Recommended fix:
|
||||
- Move to a reservation strategy or let Docker select the port and then discover the assigned binding.
|
||||
- At minimum, retry on bind collision and record the failure reason explicitly.
|
||||
|
||||
#### 6. Backend packaging/import structure is fragile and currently fails full static analysis badly
|
||||
- Files:
|
||||
- Broadly across `backend/app/**`
|
||||
- Evidence:
|
||||
- `uv run basedpyright app` reports `86 errors` and `243 warnings`.
|
||||
- Why it matters:
|
||||
- The codebase relies on `pythonpath = ["app"]` and implicit absolute imports such as `from core.config import settings`.
|
||||
- This is brittle outside the current dev/test invocation shape and blocks meaningful type-checking from being trustworthy.
|
||||
- Static type health is effectively unusable right now.
|
||||
- Recommended fix:
|
||||
- Convert backend imports to package-consistent absolute imports from `app...` or explicit relative imports.
|
||||
- Add a real basedpyright target/config to CI once import hygiene is fixed.
|
||||
|
||||
#### 7. The frontend control plane is a God object that now owns too many concerns
|
||||
- File:
|
||||
- `frontend/src/composables/useControlPlane.ts` (`1534` LOC)
|
||||
- Why it matters:
|
||||
- One composable currently owns auth, routing, discovery, drafts, editor save flow, templates, logs, health, websocket reconnection, notices, modal state, and error routing.
|
||||
- This is a high-risk shape for regressions because unrelated fixes share the same reactive graph and watcher set.
|
||||
- State restoration and race-condition fixes will keep getting harder, not easier.
|
||||
- Recommended fix:
|
||||
- Split into focused composables:
|
||||
- session/auth
|
||||
- proxy workspace state
|
||||
- discovery state
|
||||
- logs stream state
|
||||
- health stream state
|
||||
- wizard/settings draft state
|
||||
- Keep one thin composition root that wires them together.
|
||||
|
||||
#### 8. Full refresh is all-or-nothing, so one failing endpoint blocks unrelated UI surfaces
|
||||
- File:
|
||||
- `frontend/src/composables/useControlPlane.ts:648-673`
|
||||
- Why it matters:
|
||||
- `refreshAll()` uses one `Promise.all()` for proxies, services, options, and templates.
|
||||
- If one endpoint fails, none of the others update.
|
||||
- A temporary failure in options or templates can block the basic proxy/discovery view.
|
||||
- Recommended fix:
|
||||
- Separate critical and non-critical refreshes.
|
||||
- Fetch proxies/services independently from options/templates and degrade per panel.
|
||||
|
||||
### Medium
|
||||
|
||||
#### 9. Template API and UI contract are inconsistent
|
||||
- Files:
|
||||
- `backend/app/models/template.py:22-39`
|
||||
- `frontend/src/composables/useControlPlane.ts:1098-1126`
|
||||
- `frontend/src/composables/useControlPlane.ts:483-516`
|
||||
- Why it matters:
|
||||
- Backend models allow arbitrary `protocol` values and default to `"custom"`.
|
||||
- The UI only exposes `http` and `tcp` lists.
|
||||
- A custom template persisted with any other protocol value becomes effectively invisible in the current UI.
|
||||
- Recommended fix:
|
||||
- Validate template protocol/category on the backend to match supported UI semantics.
|
||||
- Or explicitly support additional categories in the UI instead of silently hiding them.
|
||||
|
||||
#### 10. Template behavior is inconsistent with a predictable “block” contract
|
||||
- File:
|
||||
- `backend/app/services/mitmproxy/templates.py`
|
||||
- Why it matters:
|
||||
- Built-in “block” templates do not behave consistently:
|
||||
- some `flow.kill()`
|
||||
- some return empty `200`
|
||||
- some return `404`
|
||||
- some return `413` or `405`
|
||||
- This may be intentional per challenge type, but as a product surface it is hard to reason about and document.
|
||||
- Recommended fix:
|
||||
- Decide whether built-in templates are “semantic examples” or “operational policies”.
|
||||
- If they are operational policies, define a small set of standard outcomes and label deviations explicitly.
|
||||
|
||||
#### 11. Synchronous filesystem work happens on the event loop in hot backend paths
|
||||
- Files:
|
||||
- `backend/app/services/mitmproxy/manager.py:287-303`
|
||||
- `backend/app/api/proxy/router.py:177-180`
|
||||
- `backend/app/integrations/docker/editor.py:39-51`
|
||||
- Why it matters:
|
||||
- `read_bytes`, `write_text`, YAML dump/write, and similar disk operations happen directly inside async request/runtime paths.
|
||||
- This is acceptable at tiny scale, but it will block the event loop during heavier operator usage or larger logs/files.
|
||||
- Recommended fix:
|
||||
- Move blocking file I/O behind `asyncio.to_thread()` or a thin async wrapper for runtime/log/config file operations.
|
||||
|
||||
#### 12. Health history limit is caller-controlled without a server-side cap
|
||||
- File:
|
||||
- `backend/app/api/proxy/router.py:387-397`
|
||||
- Why it matters:
|
||||
- `limit` is accepted directly from query params and forwarded to Mongo.
|
||||
- A large limit can create unnecessary memory and serialization pressure.
|
||||
- Recommended fix:
|
||||
- Clamp the value server-side, for example `1..200` or similar.
|
||||
|
||||
#### 13. `app.state.services` is stale mutable state with no real consumer value
|
||||
- Files:
|
||||
- `backend/app/main.py:47-56`
|
||||
- `backend/app/api/proxy/router.py:221-230`
|
||||
- Why it matters:
|
||||
- Service discovery results are written into `app.state.services`, but the state is not used as a trusted cache anywhere meaningful.
|
||||
- This adds mutable global state without clear ownership and can mislead future maintainers.
|
||||
- Recommended fix:
|
||||
- Remove it or turn it into an intentional cache with invalidation and a real read path.
|
||||
|
||||
#### 14. Docker compose editor leaves backups behind and does not manage them as lifecycle artifacts
|
||||
- File:
|
||||
- `backend/app/integrations/docker/editor.py:7-44`
|
||||
- Why it matters:
|
||||
- Backup files are created every time a port rewrite occurs.
|
||||
- They are never cleaned up or versioned intentionally.
|
||||
- Over time this pollutes challenge folders and complicates manual debugging.
|
||||
- Recommended fix:
|
||||
- Either manage backups explicitly as runtime artifacts or stop generating long-lived `.bak` files per rewrite.
|
||||
|
||||
### Low
|
||||
|
||||
#### 15. Static Python highlighting in template preview is intentionally lightweight but very naive
|
||||
- File:
|
||||
- `frontend/src/components/TemplateCodePreview.vue:1-40`
|
||||
- Why it matters:
|
||||
- Regex-based highlighting on escaped HTML will mis-handle more complex Python constructs.
|
||||
- This is mostly cosmetic today, but it limits confidence in template readability as the library grows.
|
||||
- Recommended fix:
|
||||
- Accept it as a lightweight preview or replace it with a read-only CodeMirror/Lezer-based renderer if template complexity grows.
|
||||
|
||||
#### 16. Oversized files are now a maintenance signal on their own
|
||||
- Evidence:
|
||||
- `frontend/src/composables/useControlPlane.ts`: `1534` LOC
|
||||
- `frontend/src/styles.css`: `1798` LOC
|
||||
- `backend/app/api/proxy/router.py`: `549` LOC
|
||||
- `backend/app/services/mitmproxy/manager.py`: `443` LOC
|
||||
- `backend/tests/api/test_proxy.py`: `859` LOC
|
||||
- Why it matters:
|
||||
- Even when behavior is correct, files of this size hide coupling and slow review/debug cycles.
|
||||
- Recommended fix:
|
||||
- Split by responsibility before the next major feature wave.
|
||||
|
||||
## Roadmap
|
||||
|
||||
### P0 — Correctness and operator safety
|
||||
- Make Docker compose port rewrites transactional and exception-safe.
|
||||
- Fix log stream truncation/rotation handling.
|
||||
- Add a server-side cap for health history limits.
|
||||
|
||||
### P1 — Confidence and hardening
|
||||
- Restructure backend tests into real app/integration layers and reduce router-level monkeypatching.
|
||||
- Fix import/package hygiene and make basedpyright a meaningful signal.
|
||||
- Add explicit tests for reverse-mode port reassignment failure paths and orphan recovery.
|
||||
|
||||
### P2 — Maintainability and resilience
|
||||
- Split `useControlPlane.ts` into smaller composables.
|
||||
- Separate `refreshAll()` into panel-aware refreshes.
|
||||
- Remove or formalize `app.state.services`.
|
||||
- Move blocking file I/O off the event loop.
|
||||
|
||||
### P3 — Product quality and performance
|
||||
- Add external load/perf harness for backend APIs, websockets, and lifecycle operations.
|
||||
- Normalize built-in template contract and document intended operator semantics.
|
||||
- Clean up runtime artifact and backup-file lifecycle policy.
|
||||
|
||||
Reference in New Issue
Block a user