refactor: consolidate CI, clean up docs and MCP framework#75
refactor: consolidate CI, clean up docs and MCP framework#75sparesparrow merged 1 commit intomainfrom
Conversation
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
There was a problem hiding this comment.
Trivy found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
| context = ssl.create_default_context() | ||
|
|
||
| with socket.create_connection((target_host, port), timeout=10) as sock: | ||
| with context.wrap_socket(sock, server_hostname=target_host) as ssock: |
Check failure
Code scanning / CodeQL
Use of insecure SSL/TLS version High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 14 days ago
To fix the problem, the TLS context should be configured to reject TLS 1.0 and 1.1 explicitly instead of relying on create_default_context() defaults. Modern Python provides SSLContext.minimum_version, which can be set to ssl.TLSVersion.TLSv1_2 to ensure only TLS 1.2+ are used. On slightly older but still supported versions, disabling OP_NO_TLSv1 and OP_NO_TLSv1_1 flags can achieve the same effect.
The best single fix here, without changing the existing behavior of the scanner, is:
- Keep using
ssl.create_default_context()so that certificates, CA handling, etc., remain as before. - Immediately after creating the context, explicitly restrict the minimum protocol version.
- Prefer
context.minimum_version = ssl.TLSVersion.TLSv1_2whenssl.TLSVersionis available (Python 3.7+). - Fall back to setting
context.options |= ssl.OP_NO_TLSv1 | ssl.OP_NO_TLSv1_1otherwise.
- Prefer
This can be implemented directly in modules/security-scanner/automotive_security.py within _check_ssl_security, right after line 550 where context = ssl.create_default_context() is called. No new external libraries are required; only the standard ssl module (already imported) is used.
| @@ -548,6 +548,15 @@ | ||
| for port in ssl_ports: | ||
| try: | ||
| context = ssl.create_default_context() | ||
| # Enforce modern TLS versions (TLS 1.2 and above) | ||
| if hasattr(ssl, "TLSVersion") and hasattr(context, "minimum_version"): | ||
| context.minimum_version = ssl.TLSVersion.TLSv1_2 | ||
| else: | ||
| # Fallback for older Python versions: disable TLS 1.0 and 1.1 explicitly | ||
| if hasattr(ssl, "OP_NO_TLSv1"): | ||
| context.options |= ssl.OP_NO_TLSv1 | ||
| if hasattr(ssl, "OP_NO_TLSv1_1"): | ||
| context.options |= ssl.OP_NO_TLSv1_1 | ||
|
|
||
| with socket.create_connection((target_host, port), timeout=10) as sock: | ||
| with context.wrap_socket(sock, server_hostname=target_host) as ssock: |
| """Start the OBD-II simulator server""" | ||
| server = socket.socket(socket.AF_INET, socket.SOCK_STREAM) | ||
| server.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) | ||
| server.bind(('0.0.0.0', self.port)) |
Check warning
Code scanning / CodeQL
Binding a socket to all network interfaces Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 14 days ago
In general, to fix this issue you should avoid binding server sockets to 0.0.0.0 unless you truly need to accept connections from all network interfaces. For a local development simulator that is advertised as being used via localhost, the safest default is to bind to the loopback interface (127.0.0.1). If there is a legitimate need to expose the simulator beyond localhost (e.g., to a physical Android device), that should be an explicit, configurable choice instead of an unchangeable default.
The best fix here, without changing the simulator’s core behavior, is to (1) add a configurable host/interface argument to the CLI (with a secure default of 127.0.0.1), (2) store that host value in the ELM327Simulator instance, and (3) replace the hard-coded ('0.0.0.0', self.port) with (self.host, self.port). This preserves current functionality when a user deliberately passes --host 0.0.0.0, while making the out-of-the-box behavior bind only to localhost. The usage text should also be updated to accurately reflect how to connect.
Concretely:
- Modify
ELM327Simulator.__init__(not shown, but within the same file) to accept and store ahostargument (e.g.,self.host). - Update
start()so thatserver.bind(('0.0.0.0', self.port))becomesserver.bind((self.host, self.port))and tweak the printed messages to include the host. - In
main(), add a--host(or--bind) argument with default"127.0.0.1", pass it intoELM327Simulator(host=args.host, port=args.port), and adjust the help/usage text to mention the host.
| @@ -291,15 +291,15 @@ | ||
| """Start the OBD-II simulator server""" | ||
| server = socket.socket(socket.AF_INET, socket.SOCK_STREAM) | ||
| server.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) | ||
| server.bind(('0.0.0.0', self.port)) | ||
| server.bind((self.host, self.port)) | ||
| server.listen(5) | ||
|
|
||
| print("=" * 60) | ||
| print(" ELM327 OBD-II Simulator") | ||
| print("=" * 60) | ||
| print(f" Listening on port {self.port}") | ||
| print(f" Listening on {self.host}:{self.port}") | ||
| print(f" Connect your OBD diagnostic software to:") | ||
| print(f" TCP: localhost:{self.port}") | ||
| print(f" TCP: {self.host}:{self.port}") | ||
| print("") | ||
| print(" Simulating: Toyota Auris Hybrid (default vehicle)") | ||
| print(" Supported PIDs: RPM, Speed, Coolant Temp, Fuel Level, etc.") | ||
| @@ -327,9 +321,11 @@ | ||
| parser = argparse.ArgumentParser(description="ELM327 OBD-II Simulator") | ||
| parser.add_argument("--port", "-p", type=int, default=35000, | ||
| help="TCP port (default: 35000)") | ||
| parser.add_argument("--host", "-H", default="127.0.0.1", | ||
| help="Host/interface to bind to (default: 127.0.0.1)") | ||
| args = parser.parse_args() | ||
|
|
||
| simulator = ELM327Simulator(port=args.port) | ||
| simulator = ELM327Simulator(port=args.port, host=args.host) | ||
| simulator.start() | ||
|
|
||
|
|
| active_connections.append(websocket) | ||
| logger.info(f"WebSocket client connected. Total connections: {len(active_connections)}") | ||
|
|
||
| try: | ||
| while True: |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 14 days ago
In general, to fix information exposure through exceptions, do not return raw exception details (str(e) or stack traces) to clients. Instead, log the detailed error server-side and send a generic, non-sensitive message in the API response (for example, "Internal server error" or a generic "status": "error" with no internal details).
For this codebase, the best minimal fix is to keep the existing logging for e in the except block of get_status, but change the response body so it no longer includes str(e). We can replace the "error": str(e) field with a generic message like "error": "Internal server error while fetching status" that does not depend on the exception object. This keeps behavior largely the same (client still learns that an error occurred) without leaking internal details. No new imports or helpers are needed.
Concretely, in api/main.py around lines 562–567, update the except block of get_status so that:
-
logger.error(f"Error getting status: {e}")remains unchanged, preserving server-side diagnostics. -
The returned dict is changed from:
return { "status": "error", "error": str(e), "timestamp": datetime.now().isoformat() }
to:
return { "status": "error", "error": "Internal server error while retrieving system status.", "timestamp": datetime.now().isoformat() }
This avoids exposing the raw exception content while preserving API shape and HTTP behavior.
| @@ -563,7 +563,7 @@ | ||
| logger.error(f"Error getting status: {e}") | ||
| return { | ||
| "status": "error", | ||
| "error": str(e), | ||
| "error": "Internal server error while retrieving system status.", | ||
| "timestamp": datetime.now().isoformat() | ||
| } | ||
|
|
| return { | ||
| "success": False, | ||
| "error": str(e), | ||
| "led_state": led_state, | ||
| "timestamp": datetime.now().isoformat() | ||
| } |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 14 days ago
In general, to fix this kind of problem you should avoid returning raw exception messages or stack traces to clients. Instead, log the full exception on the server (including stack trace) and return a generic, non-sensitive error message to the client. Optionally, include a simple, non-sensitive indicator (like a boolean or generic code) that something went wrong.
For this file, the best fix that preserves functionality is:
- Keep logging the actual exception with as much detail as needed (using the existing
logger). - Change the HTTP responses in the
exceptblocks ofset_led_stateandsend_led_commandto:- Remove
str(e)from the JSON payload. - Replace it with a generic message such as
"An internal error occurred while updating LED state."or"An internal error occurred while sending LED command.".
- Remove
- Do not modify other behavior (success paths, structure of the response, or other fields like
led_state,command, ortimestamp).
The required changes are confined to rpi/api/main.py in the two except blocks starting at lines 518 and 549. No new imports or helper methods are necessary because logging is already available via logger.error(...).
| @@ -519,7 +519,7 @@ | ||
| logger.error(f"Error updating LED state: {e}") | ||
| return { | ||
| "success": False, | ||
| "error": str(e), | ||
| "error": "An internal error occurred while updating LED state.", | ||
| "led_state": led_state, | ||
| "timestamp": datetime.now().isoformat() | ||
| } | ||
| @@ -550,7 +550,7 @@ | ||
| logger.error(f"Error sending LED command: {e}") | ||
| return { | ||
| "success": False, | ||
| "error": str(e), | ||
| "error": "An internal error occurred while sending LED command.", | ||
| "command": command.command, | ||
| "timestamp": datetime.now().isoformat() | ||
| } |
| return { | ||
| "success": False, | ||
| "error": str(e), | ||
| "command": command.command, | ||
| "timestamp": datetime.now().isoformat() | ||
| } |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 14 days ago
In general, to fix this issue you should avoid sending raw exception messages back to the client. Instead, log the detailed exception (optionally including the stack trace) on the server side and return a generic, user-safe error message or a stable error code in the HTTP response.
For this specific file, the best fix without changing existing functionality is:
- Keep logging the detailed error with
logger.error(...)(we can enhance it to include the full traceback usingexc_info=True). - Replace
"error": str(e)in API responses with a generic error message that does not reveal internal information, e.g."error": "Failed to send LED command"or"error": "Internal server error". - Do this consistently for all similar handlers (
set_led_state,send_led_command, andget_status) inrpi/api/main.py.
We only need to change the JSON response dictionaries in the exception blocks around lines 518–525, 549–556, and 590–596. No new methods or external dependencies are required; we can rely on the existing logger and FastAPI setup. Optionally, we can change logger.error(f"...: {e}") to logger.exception("...") or logger.error("...", exc_info=True) to include stack traces in the server logs while keeping responses generic.
| @@ -516,10 +516,10 @@ | ||
| "timestamp": datetime.now().isoformat() | ||
| } | ||
| except Exception as e: | ||
| logger.error(f"Error updating LED state: {e}") | ||
| logger.error(f"Error updating LED state: {e}", exc_info=True) | ||
| return { | ||
| "success": False, | ||
| "error": str(e), | ||
| "error": "Failed to update LED state.", | ||
| "led_state": led_state, | ||
| "timestamp": datetime.now().isoformat() | ||
| } | ||
| @@ -547,10 +545,10 @@ | ||
| "timestamp": datetime.now().isoformat() | ||
| } | ||
| except Exception as e: | ||
| logger.error(f"Error sending LED command: {e}") | ||
| logger.error(f"Error sending LED command: {e}", exc_info=True) | ||
| return { | ||
| "success": False, | ||
| "error": str(e), | ||
| "error": "Failed to send LED command.", | ||
| "command": command.command, | ||
| "timestamp": datetime.now().isoformat() | ||
| } | ||
| @@ -588,10 +584,10 @@ | ||
| "timestamp": datetime.now().isoformat() | ||
| } | ||
| except Exception as e: | ||
| logger.error(f"Error getting status: {e}") | ||
| logger.error(f"Error getting status: {e}", exc_info=True) | ||
| return { | ||
| "status": "error", | ||
| "error": str(e), | ||
| "error": "Failed to retrieve system status.", | ||
| "timestamp": datetime.now().isoformat() | ||
| } | ||
|
|
| return { | ||
| "status": "error", | ||
| "error": str(e), | ||
| "timestamp": datetime.now().isoformat() | ||
| } |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 14 days ago
In general, to fix information exposure through exceptions, you should avoid returning raw exception objects or messages to clients. Instead, log detailed error information on the server (including stack traces if needed) and return a generic, user-safe error message. Only non-sensitive, high-level status indicators should be sent back in API responses.
For this codebase, the best minimal fix is to keep the logging as-is (so developers still see detailed error info) but change the JSON payload returned by the /status endpoint so it no longer includes str(e). Instead, return a generic error string like "Internal server error" or "Failed to fetch system status". We only need to modify the except block in get_status in rpi/api/main.py (lines 590–595). No new methods or imports are required since logging is already configured and used.
Concretely:
- Leave
logger.error(f"Error getting status: {e}")unchanged so that internal logs retain details. - Replace
"error": str(e)with a generic message such as"error": "Failed to retrieve system status".
| @@ -591,7 +591,7 @@ | ||
| logger.error(f"Error getting status: {e}") | ||
| return { | ||
| "status": "error", | ||
| "error": str(e), | ||
| "error": "Failed to retrieve system status", | ||
| "timestamp": datetime.now().isoformat() | ||
| } | ||
|
|
bcbf874 to
a4be509
Compare
- Consolidate 20 CI workflows into 3 (ci.yml, security.yml, deploy.yml) - Remove 30+ root-level status/report/plan markdown files - Consolidate mcp_framework.py into modules/shared/ - Add voice-learning modules, agents, and prompts - Add car assistant config, hardware MCP server, and deploy scripts - Add MCP JSON-RPC endpoint and fix CORS middleware in core orchestrator - Update C++ FlatBuffers API (response writer pattern, request reader ctor) - Add Conan ARM64 profile fixes - Add Android BLE/ANPR/WebSocket/Permission improvements and tests - Update .gitignore for dev artifacts - Add CLAUDE.md project instructions - Add ESP32 firmware stubs and automotive MCP bridge modules Cherry-picked from: feature/car-assistant-workflow, fix/conan-arm64-compilation-errors, feature/android-todo-implementation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
a4be509 to
e675d1b
Compare
Summary
ci.yml,security.yml,deploy.yml)mcp_framework.pycopies intomodules/shared/pytest.skipusage, float comparison).gitignorefor dev artifacts and generated Conan cmake filesCLAUDE.mdproject instructionsValidation
modules.shared.mcp_frameworkTest plan
pytest -m 'not hardware')🤖 Generated with Claude Code