Comprehensive Architecture Modernization & Domain-Driven Codebase Refactoring #1
No reviewers
Labels
No labels
blocked
bug
enhancement
high-priority
low-priority
needs-info
needs-triage
ready-for-agent
ready-for-human
referenced
research
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gabogg/hikcentral!1
Loading…
Reference in a new issue
No description provided.
Delete branch "refactor/architecture-and-code-design"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
PR: Comprehensive Architecture Modernization & Domain-Driven Codebase Refactoring
🎯 Executive Summary & Architectural Motivation
The HikCentral Professional Explorer & Real-Time Dashboard has grown rapidly from an exploratory diagnostic script into a mission-critical access control monitoring engine managing 114+ access doors, physical maglocks, live telemetry, and video streaming across Orinokia's facilities.
While the application successfully delivers real-time telemetry, several architectural bottlenecks and structural smells have accumulated:
door_service.py(625+ lines) mixes HTTP requests, SQLite queries, business categorization logic, webhook management, and UI serialization into a single stateful singleton.artemis_client.py) and Bumblebee (bumblebee_client.py) clients use synchronousurllib.requestinside 1.0sasyncioloops (monitor_service.py), risking event-loop starvation and WebSocket latency spikes.Dict[str, Any]) leak across all system layers with mixedcamelCase/snake_casekeys.app.jscontains 1,250+ lines combining state, UI rendering, WebSockets, and translation logic.This PR establishes the complete architectural redesign, deep modularization, test-driven harness, asynchronous I/O migration, and domain boundary isolation required for enterprise reliability.
🏛️ Architectural Refactoring Blueprint
📋 Comprehensive Refactoring Plan & Work Breakdown
Phase 1: Domain Modeling & Schema Hardening
app/schemas/models.py):DoorEntity,AccessCycleEntity,ControllerEntity,UserEntity.DoorStateEnum,SensorCategoryEnum,EventTypeEnum.CONTEXT.md):Phase 2: Persistence Layer Modernization & Deep Repositories
app/db/database.py):IDoorRepository,IAccessCycleRepository,IUserRepository.PRAGMA journal_mode=WAL; PRAGMA synchronous=NORMAL;) for zero-contention concurrent read/writes.aiosqlite) to remove blocking disk I/O from coroutines.CREATE TABLE IF NOT EXISTS.Phase 3: Asynchronous HTTP Clients & Gateway Adapters
app/clients/artemis_client.py):urllib.requesttohttpx.AsyncClientwith connection pooling, keep-alive, and configurable timeouts.HikCentralApiError,HikCentralAuthError,HikCentralTimeoutError).app/clients/bumblebee_client.py):httpx.Phase 4: Service Layer Decomposition & Deep Modules
app/services/door_service.py):DoorClassificationService: Pure business module implementing sensor classification heuristics (verified sensor vs open loop vs jumpered).AccessCycleAggregator: Stateful domain coordinator assembling multi-stage access sessions (Card Reader Entry vs Push Button Exit vs Alarm triggers).WebhookSubscriptionManager: Dedicated service for managing Artemis webhook event subscription lifecycles.DoorSyncCoordinator: Background synchronization orchestrator maintaining local database freshness.app/services/monitor_service.py):WebSocketBroadcasterwith backpressure and dead connection pruning.Phase 5: Dependency Injection & Controller Refinement
Depends()providers.auth_controller.py,door_controller.py,probe_controller.py,video_controller.py, andwebhook_controller.py.video_controller.pyto use non-blockinghttpx.AsyncClient().stream().Phase 6: Security Hardening & Configuration
app/db/database.pywith standardArgon2idorbcrypt.app/config.py):pydantic-settingsBaseSettingswith strict.envschema validation and environment overrides.HttpOnly,SameSite=Lax,Secure).Phase 7: Complete Test Suite & Quality Harness (TDD)
pytest,pytest-asyncio,pytest-cov, andrespx(for HTTP client mocking).Phase 8: Frontend Modularization (ES6 Modules)
app/static/js/app.js):api.js,websocket.js,store.js,doors.js,activity.js,diagnostics.js,video.js.🔍 Verification & Acceptance Criteria
ruff) and type-checker (mypy) pass with zero errors.🛡️ Adversary Code Review: Architecture Modernization & Domain Refactoring
PR: #1 (refactor/architecture-and-code-design)
Review Type: Security, Failure Modes, Concurrency, and Implementation Verification
Live Target Verification: Artemis OpenAPI & Bumblebee ISAPI credentials validated against
10.10.1.251(both systems authenticated successfully with ~59ms response times).📋 Executive Summary
The refactoring introduces major domain and architecture improvements (Pydantic v2 schemas, SQLite WAL mode, async httpx clients, and domain classifiers). However, an adversary analysis uncovered several critical security vulnerabilities and reliability issues that need to be addressed before merging to
master.(Note on Session Lifetimes: Per operational requirements, Operator sessions are designed to run 24/7 without expiring for wall displays/kiosks. However, because the Admin role holds dangerous diagnostic and control access, Admin sessions should have an inactivity timeout).
🚨 Critical Security Vulnerabilities
1. Unauthenticated Full Server-Side Request Forgery (SSRF) in Video Proxy
app/controllers/video_controller.py(L90-98)get_hls_segmenttakes an arbitraryupstreamquery parameter and fetches it viahttpx.get(upstream, verify=False)with no authentication and no destination host validation.169.254.169.254), or query internal management interfaces.require_auth) and strictly validate thaturllib.parse.urlparse(upstream).hostname == settings.server_ip.2. Unauthenticated Webhook Injection & Persistent Stored XSS Chain
app/controllers/webhook_controller.py(L12-38)app/static/js/app.js(L519-596)/api/event/webhookaccepts unauthenticated POST payloads from any sender. The payload is stored in SQLite and broadcast over WebSockets to all connected client browsers. The frontend renderspersonName,doorName,summaryLabel, andpicUrldirectly intocontainer.innerHTMLwithout HTML entity sanitization.personName. When an administrator or operator views the activity stream, the payload executes, allowing arbitrary action execution.escapeHtml()) across all dynamic fields rendered inapp.js, and restrict webhook endpoint access or validate sender authenticity.3. Unauthenticated WebSocket Broadcast Leaking Admin ISAPI Session (
bumblebee_sid)app/main.py(L57-68)app/services/monitor_service.py(L91-101)While
probe_controller.pyredactsbumblebee_sidfor non-admin REST queries,/ws/realtimehas zero authentication, and the background worker pushesbumblebee_sidin public telemetry broadcasts every 30s./ws/realtimeacquires the active HikCentral ISAPI session token (SID).bumblebee_sidfrom generic WebSocket broadcasts and authenticate WebSocket connections.4. Application Startup Overwrites Changed Passwords
app/db/database.py(L109-114)In
init_db(), if a user changes their password from the defaultControlHG/OperadorHG,verify_passwordreturnsFalse, causing the startup routine to overwrite the password hash back to the default seed password.userstable.⚙️ Concurrency, Runtime Bugs & Performance
5. Synchronous Blocking Call Inside
get_door_overview()app/services/door_service.py(L572-573)get_door_overview()callsself.sync_doors(), which executes synchronousartemis.request(). When called from async FastAPI endpoints (/api/doors/statusand/api/event/webhook), this blocks the asyncio event loop for up to 12s on network timeouts.get_door_overview()purely read from in-memory cache and SQLite. Background synchronization is already handled asynchronously bysync_doors_async().6. Unhandled
IndexErroron Fresh / Empty Activity Streamapp/controllers/webhook_controller.py(L26)latest_event = overview.get("recent_activity", [None])[0]raises anIndexError: list index out of rangewheneverrecent_activityis empty[].7. Connection Exhaustion in Video Streaming Proxy
app/controllers/video_controller.py(L65, L94)Creating a new
httpx.AsyncClienton every single.m3u8and.tschunk bypasses connection pooling, leading to TLS handshake overhead and socket exhaustion (TIME_WAIT).httpx.AsyncClientinstance for video proxying.8. Hardcoded IP and Port in Webhook Subscription Service
app/services/webhook_manager.py(L20, L27)get_local_ip()andsubscribe_async()hardcode10.10.1.251and port8888instead of readingsettings.server_ip,settings.https_port, andsettings.app_port.🛡️ Recommended Action Checklist Before Merge
hostname == settings.server_ipon/api/video/hls/{camera_index_code}/segment.escapeHtml) inapp.jsand setHttpOnlyon session cookies.bumblebee_sidfrom public broadcasts.init_db()doesn't overwrite modified passwords.sync_doors()invocation fromget_door_overview().overview.get("recent_activity")against empty listIndexError.✅ Adversary Review Remediations Applied & Verified
All 8 security, concurrency, and reliability findings from the code review have been resolved, covered with automated tests, and pushed in commit
1dad014.🛡️ Remediations Summary
SSRF Patch & Destination Host Enforcement:
app/controllers/video_controller.py: Addedvalidate_upstream_url()enforcingscheme in ("http", "https")andhostname == settings.server_ip. Addedrequire_authto HLS proxy routes.Persistent Stored XSS Sanitization:
app/static/js/app.js: ImplementedescapeHtml()and applied across all interpolated properties (doorName,personName,personRole,cardNo,summaryLabel,doorIndexCode,reasonText). Sanitized thumbnail photo URLs.WebSocket Information Leak Redaction:
app/services/monitor_service.py: Redactedbumblebee_sidfrom public WebSocket telemetry broadcasts.Startup User Credentials Persistence:
app/db/database.py: Updatedinit_db()to only insert default seed accounts if the user does not exist, preserving user-modified passwords across server restarts.Role-Based Session Expiration (Operator 24/7 vs Admin TTL):
app/db/user_repository.py: Enforced 4-hour inactivity timeout (ADMIN_SESSION_INACTIVITY_TTL = 14400s) for theadminrole, while keeping theoperatorrole active indefinitely (24/7 wall displays/kiosks).Event Loop Non-Blocking Optimization:
app/services/door_service.py: Removed synchronous blockingsync_doors()fromget_door_overview(). Overview reads purely from in-memory cache and SQLite.Safe Indexing in Webhook Receiver:
app/controllers/webhook_controller.py: Protected against emptyrecent_activitylist indexing.Dynamic Server IP & Port in Webhook Manager:
app/services/webhook_manager.py: Swapped hardcoded IP/ports forsettings.server_ip,settings.https_port, andsettings.app_port.HTTP Client Connection Pooling for Video Streaming:
app/controllers/video_controller.py: Introduced persistenthttpx.AsyncClientconnection pool (_video_proxy_client) to prevent TCP socket exhaustion.🧪 Automated Test Verification
🔍 Verification Report & Test Suite Robustness Audit
PR: #1 (refactor/architecture-and-code-design)
Verified Commit:
1dad014Status: All requested security & concurrency fixes verified; Test suite evaluation completed.
Part 1: Verification of Applied Fixes
We inspected the diff in commit
1dad014and executed the automated test suite against the updated codebase. All requested remediations have been properly implemented:validate_upstream_url()inapp/controllers/video_controller.pyenforcinghostname == settings.server_ip. Added connection pooling (get_video_proxy_client()) and mandatoryrequire_auth. Added negative rejection test (test_video_ssrf_rejection).escapeHtml()helper inapp/static/js/app.js. Applied escaping across all dynamic fields in cycle cards, door lists, and sensor audit table rows.app/db/user_repository.pysoadminsessions expire after 4 hours of inactivity (admin_inactivity_ttl=14400.0), whileoperatorsessions never expire (accommodating 24/7 wall displays/kiosks).app/db/database.pyinit_db()to only insert default seed users if they do not exist (if not row), preserving modified passwords across application restarts. Addedtest_startup_does_not_overwrite_modified_passwords.IndexErrorCrashapp/controllers/webhook_controller.pywith safe fallbackoverview.get("recent_activity") or []before indexing[0].self.sync_doors()call fromget_door_overview()inapp/services/door_service.py. Overview is now strictly non-blocking and backed by in-memory/SQLite cache.bumblebee_sidfrom generic telemetry broadcast payloads inapp/services/monitor_service.py.app/services/webhook_manager.pyto read dynamically fromsettings.server_ip,settings.https_port, andsettings.app_port.Part 2: Critical Evaluation of Test Suite Behavioral Coverage
A critical audit of the test suite (
tests/test_api.py,tests/test_crypto.py,tests/test_domain.py,tests/test_repositories.py) was performed to evaluate whether tests verify true system behavior or only superficial expected return values.✅ Genuine Behavioral Verification Strengths
test_repositories.pyexecutes against real SQLite tables in WAL mode without fake mocks.test_startup_does_not_overwrite_modified_passwordsmodifies live password hashes in the database, invokes the startup routine, and asserts persistence.test_admin_session_inactivity_expiration_and_operator_247backdates DB timestamps by 5 hours to verify the expiration logic.test_domain.pyvalidates multi-stage access cycle aggregation (badge opening vs push-button exit vs alarm transitions).test_operator_rbac,test_unauthorized_access, andtest_video_ssrf_rejectionassert HTTP 401, 403, and destination rejection behaviors.⚠️ Shallow Areas & Blind Spots Requiring Reinforcement
test_webhook_event_endpointsends<script>alert(1)</script>in the payload, but it only asserts that FastAPI returnedcode: 0. It cannot verify whether the browser DOM rendered it safely.504 Gateway Timeout,401 Unauthorized, or network socket drops during polling.respxto inject HTTP error responses and verify graceful retry / degradation handling.starlette.testclient.TestClient. They do not test concurrent WebSocket subscribers, client disconnections, or broadcast message delivery under load.pytest-asynciothat connects multiple WebSockets, triggers webhook events, and asserts message reception without token leakage.asyncio.gather()with 50+ parallel cycle upserts to verify zero database locking errors.🎯 Test Suite Reinforcement & Codebase Modernization Completed
In response to the behavioral coverage audit, the repository layer and client methods were upgraded to support fully asynchronous operations and consistent HTTP status reporting, and the test suite was expanded across all 4 identified areas in commit
07ecb8a.🛠️ Code Modifications to Support Behavioral Requirements
Async Persistence for Repositories:
app/db/door_repository.py: Addedupsert_async(),get_by_code_async(),get_all_async(),set_exclusion_async(), andset_category_exclusion_async()utilizing non-blockingaiosqliteconnection pooling with SQLite WAL mode.app/db/cycle_repository.py: Addedupsert_async(),get_active_for_door_async(), andget_recent_async().Standardized Status Code Reporting in Bumblebee Client:
app/clients/bumblebee_client.py: Addedstatus_codetokeep_live_async(),keep_live(),get_version_async(), andget_version()dictionaries for consistent gateway status handling.🧪 Reinforced Behavioral Test Suites
tests/test_concurrency.pyupsert_async()andget_recent_async()with zero SQLite locks.WebSocket Concurrency: Concurrent multi-client broadcasts asserting ZERO sensitive token leakage (
bumblebee_sid, passwords).tests/test_resilience.pytests/test_xss_sanitization.pyimg onerrorattribute escaping, inline event handlers (onmouseover), and SVG onload payloads.✅ PR Approved: Ready for Merge
Final Verification Summary
aiosqlitemethods added acrossDoorRepositoryandAccessCycleRepositorywith SQLite WAL mode.31 passed, 1 warning in 13.22s).