Security and DB fixes#1535
Open
kizniche wants to merge 7 commits into
Open
Conversation
- Remove Remote Admin feature entirely (routes, model, templates,
utils_remote_host, RemoteSetup form, /newremote/ /remote_login /auth/
endpoints); add Alembic migration to drop remote table and
displayorder.remote_host column
- Bind Pyro5 IPC daemon to 127.0.0.1 instead of 0.0.0.0
- Add server-side IP-based login ban (login_security.py) that survives
cookie clearing; resolve real client IP from X-Forwarded-For safely,
skipping loopback/private addresses
- Fix controller_deactivate returning bare tuple `1,` instead of `1, message`
- Fix check_daemon() missing explicit `return "OK"` on success
- Fix trigger_action() mutable default argument `value={}` in daemon and actions.py
- Fix initialize_variables() exception leaving ready.wait() hung; set
running=False and ready.set() before returning
- Replace stop_all_controllers() hard-coded sleep(2.5) with join(timeout=10)
per thread; log controllers that fail to stop in time
- Cache SQLAlchemy engine per URI in session_scope(); fix bare except on
config_override import → except ImportError
- Fix determine_controller_type() to short-circuit on first match and
guard against None unique_id
- Replace bare except: with except Exception: in daemon (3 sites)
- Add 21 unit tests for login_security module
- databases/utils.py: cache SQLAlchemy engine and sessionmaker per URI so session_scope() no longer creates a new engine on every call - utils/database.py: raise RuntimeError after 5 consecutive OperationalError retries in db_retrieve_table_daemon() instead of silently retrying forever - utils/influx.py: cache InfluxDB client per connection settings; add refresh_influx_client() to force reconnection without a daemon restart; fix ts_str Flux filter using InfluxQL syntax (AND time =) — replaced with valid Flux time() syntax (r["_time"] == time(v: "...")) - mycodo_client.py: cache Pyro5 proxy in DaemonControl and add _reset_proxy(); restore proxy._pyroTimeout to default after module_function() calls with a custom timeout; fix mutable default argument in trigger_action() - controllers/base_controller.py: add ready.set() safety net in run() so that callers are never blocked indefinitely if initialize_variables() returns without setting the event - mycodo_daemon.py: guard determine_controller_type() returning None in controller_activate(), controller_deactivate(), and controller_is_active() to return structured errors instead of raising - tests: add test_framework_fixes.py covering all of the above changes
Contributor
There was a problem hiding this comment.
Pull request overview
This PR bundles multiple security hardening and database/IPC reliability fixes across Mycodo’s daemon, Flask app, and persistence layers.
Changes:
- Remove the Remote Admin feature end-to-end (routes, templates, model references, and DB schema).
- Add caching for SQLAlchemy engines/sessionmakers and InfluxDB clients to reduce per-call connection overhead.
- Improve security posture (Pyro5 binds to localhost; add IP-based login brute-force tracking) and fix several runtime robustness issues (mutable defaults, controller readiness waits).
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| mycodo/utils/influx.py | Adds module-level InfluxDB client caching, refresh hook, and fixes Flux timestamp filtering / exception logging. |
| mycodo/utils/database.py | Makes db_retrieve_table() Flask-context aware with safe outside-Flask fallback; db_retrieve_table_daemon() now raises after exhausted retries. |
| mycodo/utils/actions.py | Fixes mutable default argument in trigger_action(). |
| mycodo/tests/software_tests/test_mycodo_flask/test_login_security.py | Adds unit tests for the new login brute-force helper module. |
| mycodo/tests/software_tests/test_mycodo_flask/test_endpoints.py | Removes Remote Admin endpoint expectations. |
| mycodo/tests/software_tests/test_framework_fixes.py | Adds regression tests for DB/session caching, Influx caching, IPC/client fixes, and controller readiness behavior. |
| mycodo/mycodo_flask/utils/utils_remote_host.py | Removes Remote Admin remote-host utility code. |
| mycodo/mycodo_flask/templates/remote/setup.html | Removes Remote Admin setup UI template. |
| mycodo/mycodo_flask/templates/remote/input.html | Removes Remote Admin input UI template. |
| mycodo/mycodo_flask/templates/layout-remote.html | Removes Remote Admin layout template. |
| mycodo/mycodo_flask/routes_remote_admin.py | Removes Remote Admin blueprint and endpoints. |
| mycodo/mycodo_flask/routes_authentication.py | Removes Remote Admin auth endpoints and integrates IP-based login failure tracking. |
| mycodo/mycodo_flask/login_security.py | Adds request IP resolution + in-memory IP ban tracking helpers. |
| mycodo/mycodo_flask/forms/forms_authentication.py | Removes Remote Admin form definitions. |
| mycodo/mycodo_flask/app.py | Stops registering Remote Admin blueprint; clarifies DB session usage at startup. |
| mycodo/mycodo_daemon.py | Improves controller activation/deactivation robustness, refreshes Influx client on settings refresh, binds Pyro server to localhost, and enables PyroMonitor. |
| mycodo/mycodo_client.py | Adds cached Pyro Proxy behavior, timeout restoration, and fixes mutable default. |
| mycodo/databases/utils.py | Adds engine/sessionmaker caching for session_scope(). |
| mycodo/databases/models/display_order.py | Drops remote_host column from model. |
| mycodo/databases/models/init.py | Removes Remote model export. |
| mycodo/databases/init.py | Makes CRUD helpers context-aware (Flask vs daemon) and avoids Flask import inversion. |
| mycodo/controllers/base_controller.py | Ensures ready.set() is always signaled even on initialization failures. |
| alembic_db/alembic/versions/a1b2c3d4e5f6_remove_remote_admin.py | Migrates DB schema to remove Remote Admin table/column. |
| CHANGELOG.md | Documents Remote Admin removal, Pyro bind change, and key bugfixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Owner
Author
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/kizniche/Mycodo/sessions/75d5350d-9977-45c0-9009-56617bfc1654 Co-authored-by: kizniche <838427+kizniche@users.noreply.github.com>
Agent-Logs-Url: https://github.com/kizniche/Mycodo/sessions/75d5350d-9977-45c0-9009-56617bfc1654 Co-authored-by: kizniche <838427+kizniche@users.noreply.github.com>
Agent-Logs-Url: https://github.com/kizniche/Mycodo/sessions/75d5350d-9977-45c0-9009-56617bfc1654 Co-authored-by: kizniche <838427+kizniche@users.noreply.github.com>
Contributor
Implemented the review-thread fixes and pushed them in commit ad4fc84 (plus follow-up commits 80f5d72 and fb8fa11): SQLite-only engine connect args, Flask-session rollback hardening in CRUD save/delete, trusted-proxy gating for X-Forwarded-For, safer test FileHandler patch lifecycle, proxy error re-raise, and explicit Influx import handling/doc updates with test updates. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
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
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.
No description provided.