chore: full review hardening — bind-IP validation, TLS e2e test, doc fixes (v2.6.2)#22
Merged
Conversation
…fixes (v2.6.2) Multi-dimension review (engine correctness, web/security, config/packaging, design/docs/tests). Conclusion: no critical bugs — the engine's _clients reads are race-free (synchronous comprehensions, no await mid-iteration), the auth/RBAC/CSRF/ SSRF model is sound, config round-trips losslessly, and edit-preservation already guards match/RS-485/open-close strings. Concrete fixes: - config: validate admin_ui.bind_ip in AppConfig.validate() (a hand-edited bad value now fails with a clear ConfigError instead of crashing uvicorn at bind time). - tests: add test_tls_bridge.py — per-mapping TLS data bridge e2e (asserts the channel is encrypted and a plaintext client is rejected); this was the top coverage gap. Add an admin bind-IP validation case to test_config_validation.py. Suite is now 33 green. - docs: clarify that RS-485 is set via JSON import / REST (no web UI yet) and that the global logs are size-rotated (distinct from per-mapping 100 MB/15-day trim). - ROADMAP: record the review outcome + refreshed backlog (cluster hardening, RS-485 UI, Modbus write, Node-24 CI bump as a non-breaking maintenance item).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A full multi-dimension review of the project (engine correctness, web/security, config/runtime/packaging, design/docs/tests), then the concrete fixes it surfaced.
Review conclusion: healthy, no critical bugs
Four focused passes. The headline "critical" candidates did not hold up on verification:
_clientsiteration "races" — false._pick_victim/_refresh_clientsuse synchronous comprehensions (noawaitmid-iteration), so in single-threaded asyncio they're atomic. NoRuntimeError, no snapshot needed.writer.close()withoutwait_closed()"leak" — not a leak;close()schedules the transport close. (Production shutdown already aborts + boundswait_closed.)_preserve_unmanaged_fields(match / RS-485 / open-close strings), and already covered bytest_preserve_unmanaged_fields_on_edit.control-peer"missing CSRF" — it's not in the CSRF-exempt set, so the middleware already enforces theX-CSRF-Tokenheader on it.Auth / RBAC / session / CSRF / SSRF-allowlist / scrypt / LDAP+OIDC escaping / CSP all reviewed as sound. Config round-trips losslessly (incl.
cluster.peers,instance_id); atomic write + validation-before-save confirmed.Fixes applied
config: validateadmin_ui.bind_ipinAppConfig.validate()— the one genuine gap. A hand-edited bad value now fails with a clearConfigErrorinstead of crashing uvicorn at bind time.tests: addtest_tls_bridge.py— per-mapping TLS data-bridge e2e (brings up a TLS server mapping with a self-signed cert, asserts the channel is encrypted and a plaintext client is rejected). This was the top coverage gap. Plus an admin bind-IP validation case. Suite now 33 green.docs: RS-485 is settable via JSON import / REST today (web-UI control is planned) — the README's "full serial config" line was slightly overstated; globalall.log/audit.logare size-rotated (distinct from the per-mapping 100 MB / 15-day trim).ROADMAP: records the review outcome and a refreshed backlog (cluster hardening, RS-485 UI, Modbus write/Sparkplug, and the Node-24 GitHub-Actions bump — noted as non-breaking: GitHub force-runs the current actions on Node 24 from 2026-06-16, so it's cosmetic).Verification
ser2net --version→ 2.6.2 · ruff clean · 33/33 tests pass (Linux+Windows × py3.10–3.13 in CI).