feat(web): add Tools tab and row address type display setting#373
Open
ChuckBuilds wants to merge 7 commits into
Open
feat(web): add Tools tab and row address type display setting#373ChuckBuilds wants to merge 7 commits into
ChuckBuilds wants to merge 7 commits into
Conversation
…lures set +e alone doesn't suppress bash's ERR trap, so any non-zero exit from first_time_install.sh inside the one-shot installer immediately triggered the outer on_error handler with a generic "Main installation, line 370" message — before the script could report the real exit code or point to logs/. Suspend the trap for that block so the existing if/else handling runs instead.
install_dependencies_apt.py previously reported only which packages failed, not why - the actual apt/pip error was discarded (apt) or could scroll out of the on_error log tail (pip), leaving "Step 7: Install web interface dependencies (line 915)" as the only visible detail. Capture command output for each install attempt and print a compact DEPENDENCY INSTALLATION FAILURES summary with the last lines of error output per package. Also run the installer with `python3 -u` for real-time, correctly-ordered logging, and widen the on_error tail from 50 to 100 lines so the summary isn't cut off.
- wait_for_apt_lock: apt_update/apt_install now wait (up to 3min) for unattended-upgrades to release the dpkg lock instead of failing outright with "Command failed after 3 attempts" right after first boot. - check_disk_space: new pre-flight check (Step 1) so a full SD card fails fast with a clear message instead of a cryptic mid-build error. - Step 6: wrap rpi-rgb-led-matrix git clone/submodule operations in retry for resilience to transient network issues. - Step 6: capture `pip install .` build output and print the last 50 lines on failure, so the actual cmake/compiler error is visible instead of just "Failed to install rpi-rgb-led-matrix Python package".
…ency installer Address coderabbitai review on PR #369: - _run() now streams combined stdout/stderr to a temp file and returns only the last ERROR_TAIL_LINES lines, instead of buffering full output in memory (Codacy also flagged the previous capture_output call as a subprocess-without-static-string security issue; the new call is annotated as safe since cmd is built from hardcoded args). - `apt update` now runs once in main() instead of once per package needing an apt fallback.
Codacy's Semgrep-based check still flagged the cmd-built subprocess.run call as "without a static string" even with the Bandit nosec applied. Add a nosemgrep marker alongside it - cmd is always a hardcoded apt/pip argument list, never user input.
…lient Address remaining coderabbitai findings on PR #369: - check_package_installed() did __import__(package_name) directly, but python-dateutil and websocket-client import as dateutil/websocket. Both always failed the "already installed" check and were reinstalled on every run. Add an IMPORT_NAME_MAP for the mismatched names. - _run() still read the entire temp file into memory before slicing the tail. Stream it line-by-line into a deque(maxlen=ERROR_TAIL_LINES) instead so memory use stays bounded for very chatty commands.
Adds a Tools/Utilities tab to the web interface with one-click maintenance buttons that previously required SSH: - Git status panel (branch, dirty state, recent commits) - Pull latest (rebase) and force reset to origin/main - Reinstall base requirements (pip, with output) - Reinstall per-plugin requirements (pass/fail per plugin) - Clear __pycache__ directories - Quick-access restart for display and web services Also exposes the hzeller row_address_type option (0–4) in the Display settings tab. The backend already read this value from config; the UI, API field list, and validation were missing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR hardens shell installers with APT lock waiting, disk space checks, retry-wrapped git/pip steps, and structured failure output captured to temp files. It refactors ChangesInstaller Robustness
Web Interface Additions
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Flask as api_v3 Blueprint
participant Shell as subprocess
Browser->>Flask: GET /v3/partials/tools (HTMX loadtab)
Flask-->>Browser: tools.html partial + JS
Note over Browser: loadGitInfo() fires on render
Browser->>Flask: GET /api/v3/system/git-info
Flask->>Shell: git branch / git status / git log / git remote
Shell-->>Flask: branch, dirty, commits, remote URL
Flask-->>Browser: JSON {branch, dirty, log, remote}
Browser->>Flask: POST /api/v3/system/action {action: "force_git_reset"}
Flask->>Shell: git fetch origin + git reset --hard origin/main
Shell-->>Flask: stdout+stderr
Flask-->>Browser: JSON {status, message, output}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 13 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web_interface/blueprints/api_v3.py`:
- Around line 1674-1684: The git subprocess commands (branch, status, log, and
remote) are not checking return codes before using their output. If any command
fails, the empty stdout is silently treated as a clean state (dirty: false,
empty strings). Check the returncode attribute of each subprocess.run() result
and if any command fails (returncode != 0), return an error response with a
clear message explaining which git command failed, rather than proceeding with
empty/blank values that misrepresent the repository state.
- Around line 1591-1599: The subprocess output from pip operations is unbounded
and can consume significant RAM on Raspberry Pi, especially with verbose
dependency resolution or build failures. Create a shared helper function that
captures only the last N bytes or lines of the combined stdout and stderr, and
includes the command, return code, and timeout context in the output. Apply this
helper to limit the output in the jsonify response's output field in both the
pip install operation at lines 1591-1599 (where subprocess.run is called and the
output field is populated in the jsonify response) and the similar pip operation
at lines 1607-1615 (which has an equivalent unbounded output field in its
jsonify response), replacing the direct concatenation of result.stdout and
result.stderr with calls to this new helper function.
- Around line 1600-1602: In the elif block for the 'install_plugin_requirements'
action, change the reference from the module-level plugin_manager to
api_v3.plugin_manager in the ternary expression on line 1601. This ensures
consistency with how plugin managers are accessed throughout the rest of the
blueprint and prevents inadvertently falling back to the PROJECT_ROOT /
'plugin-repos' default when only the blueprint attribute is initialized. Replace
plugin_manager with api_v3.plugin_manager in the condition that determines which
plugins directory to use.
- Around line 1677-1683: The remote_url field in the jsonify return statement is
including the raw output from git remote get-url, which may contain embedded
credentials in HTTPS URLs (e.g., https://user:password@github.com/repo.git).
Before including remote.stdout.strip() as the remote_url value in the response,
parse the URL using a library like urllib.parse to remove any user
authentication information, or extract only the safe components (protocol, host,
path) and reconstruct the URL without credentials. This prevents accidental
credential exposure in the API response.
- Around line 1640-1646: The `clear_pycache` action increments the `cleared`
counter unconditionally even though `shutil.rmtree()` is called with
`ignore_errors=True`, meaning failed deletions are still counted as successful.
Fix this by removing `ignore_errors=True` from the shutil.rmtree call in the
rglob loop and wrapping it in a try-except block. Only increment the `cleared`
counter when deletion succeeds, and track any failures in a separate counter.
Return both the cleared count and failed count (or a descriptive error message)
in the jsonify response so users get accurate feedback about what was actually
deleted versus what failed.
- Around line 1625-1633: The git commands in the subprocess.run calls rely on
PATH lookup without verifying git is available, which is inconsistent with the
existing pattern used for _SUDO and _JOURNALCTL. Add _GIT = shutil.which('git')
near the top of the module alongside the other tool resolution constants, then
validate that _GIT is not None at the start of both execute_system_action() and
get_git_info() functions by returning a jsonify response with status 503 if the
git command is unavailable, and finally replace all instances of 'git' strings
in the subprocess.run() calls within these endpoints with the resolved _GIT
variable path.
In `@web_interface/blueprints/pages_v3.py`:
- Around line 457-459: The exception handler for the Tools partial uses an
overly broad `except Exception` clause and lacks debugging context for remote
Raspberry Pi environments. Replace the broad `except Exception` with specific
template-related exceptions (such as TemplateNotFound, TemplateError, or other
Jinja2/Flask template exceptions) to catch only relevant errors. Update the
logger.error call to include the `[Pages V3][Tools]` context prefix in the log
message to provide structured logging context for troubleshooting. Additionally,
update the returned error message string to also include the `[Pages V3][Tools]`
prefix for consistency in error responses, ensuring both logs and client-facing
messages provide clear context about which component failed.
In `@web_interface/templates/v3/base.html`:
- Around line 1298-1308: The Tools tab in the template relies solely on HTMX
loading via hx-get without any fallback mechanism, whereas other tabs have
fallback paths in the loadTabContent function. Add a non-HTMX fallback branch in
the loadTabContent function to handle the Tools tab (when activeTab === 'tools')
so that if HTMX fails or is disabled, the tab content can still load gracefully
instead of remaining blank. Ensure the fallback mimics how other tabs in
loadTabContent degrade gracefully.
In `@web_interface/templates/v3/partials/tools.html`:
- Around line 222-246: The toolsAction function makes unauthenticated requests
to the /api/v3/system/action endpoint which performs dangerous state-changing
operations. Add authentication to protect this endpoint by modifying the fetch
call in toolsAction to include an authentication token or credential in the
request headers, and update the corresponding backend execute_system_action
endpoint to validate this authentication before allowing any system action to
proceed. This ensures only authorized clients can trigger destructive operations
like git reset, pip install, or service restarts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68218690-be81-4288-811d-2645ea1c279f
📒 Files selected for processing (8)
first_time_install.shscripts/install/one-shot-install.shscripts/install_dependencies_apt.pyweb_interface/blueprints/api_v3.pyweb_interface/blueprints/pages_v3.pyweb_interface/templates/v3/base.htmlweb_interface/templates/v3/partials/display.htmlweb_interface/templates/v3/partials/tools.html
Comment on lines
+1591
to
+1599
| result = subprocess.run( | ||
| [sys.executable, '-m', 'pip', 'install', '--break-system-packages', '-r', str(req_file)], | ||
| capture_output=True, text=True, timeout=120, cwd=str(PROJECT_ROOT) | ||
| ) | ||
| return jsonify({ | ||
| 'status': 'success' if result.returncode == 0 else 'error', | ||
| 'message': 'Base requirements installed successfully' if result.returncode == 0 else 'pip install failed', | ||
| 'output': (result.stdout + result.stderr).strip() | ||
| }) |
There was a problem hiding this comment.
Bound pip output before returning it from the web request.
Both pip actions buffer and return full stdout/stderr. On Raspberry Pi, verbose dependency resolution or build failures can consume significant RAM and make the web interface unresponsive; capture a tail-limited log instead, ideally via a small shared helper that records only the last N bytes/lines and includes the command, return code, and timeout context.
As per coding guidelines, “Optimize code for Raspberry Pi's limited RAM and CPU capabilities” and “Provide clear error messages for troubleshooting”.
Also applies to: 1607-1615
🧰 Tools
🪛 Ruff (0.15.17)
[error] 1591-1591: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web_interface/blueprints/api_v3.py` around lines 1591 - 1599, The subprocess
output from pip operations is unbounded and can consume significant RAM on
Raspberry Pi, especially with verbose dependency resolution or build failures.
Create a shared helper function that captures only the last N bytes or lines of
the combined stdout and stderr, and includes the command, return code, and
timeout context in the output. Apply this helper to limit the output in the
jsonify response's output field in both the pip install operation at lines
1591-1599 (where subprocess.run is called and the output field is populated in
the jsonify response) and the similar pip operation at lines 1607-1615 (which
has an equivalent unbounded output field in its jsonify response), replacing the
direct concatenation of result.stdout and result.stderr with calls to this new
helper function.
Source: Coding guidelines
Comment on lines
+1600
to
+1602
| elif action == 'install_plugin_requirements': | ||
| plugins_dir = Path(plugin_manager.plugins_dir) if plugin_manager else PROJECT_ROOT / 'plugin-repos' | ||
| results = [] |
There was a problem hiding this comment.
Use the initialized blueprint plugin manager for plugin requirements.
Line 1601 reads the module-level plugin_manager, but the rest of this blueprint routes through api_v3.plugin_manager. If only the blueprint attribute is initialized, this falls back to PROJECT_ROOT / 'plugin-repos' and processes the wrong plugin directory.
Proposed fix
elif action == 'install_plugin_requirements':
- plugins_dir = Path(plugin_manager.plugins_dir) if plugin_manager else PROJECT_ROOT / 'plugin-repos'
+ active_plugin_manager = getattr(api_v3, 'plugin_manager', None)
+ plugins_dir = Path(active_plugin_manager.plugins_dir) if active_plugin_manager else PROJECT_ROOT / 'plugin-repos'
results = []🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web_interface/blueprints/api_v3.py` around lines 1600 - 1602, In the elif
block for the 'install_plugin_requirements' action, change the reference from
the module-level plugin_manager to api_v3.plugin_manager in the ternary
expression on line 1601. This ensures consistency with how plugin managers are
accessed throughout the rest of the blueprint and prevents inadvertently falling
back to the PROJECT_ROOT / 'plugin-repos' default when only the blueprint
attribute is initialized. Replace plugin_manager with api_v3.plugin_manager in
the condition that determines which plugins directory to use.
Comment on lines
+1625
to
+1633
| fetch = subprocess.run( | ||
| ['git', 'fetch', 'origin'], | ||
| capture_output=True, text=True, timeout=30, cwd=project_dir | ||
| ) | ||
| if fetch.returncode != 0: | ||
| return jsonify({'status': 'error', 'message': 'git fetch failed', 'output': fetch.stderr.strip()}) | ||
| reset = subprocess.run( | ||
| ['git', 'reset', '--hard', 'origin/main'], | ||
| capture_output=True, text=True, timeout=30, cwd=project_dir |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 "\[['\"]git['\"]" web_interface/blueprints/api_v3.pyRepository: ChuckBuilds/LEDMatrix
Length of output: 3405
🏁 Script executed:
#!/bin/bash
# Check for _SUDO, _JOURNALCTL, _GIT definitions
rg -n "_SUDO|_JOURNALCTL|_GIT\s*=" web_interface/blueprints/api_v3.py | head -20
# Check the top of the file for imports and module-level constants
head -100 web_interface/blueprints/api_v3.py | cat -n
# Get context around lines 1625-1633 to see if it's part of a new section/feature
sed -n '1600,1650p' web_interface/blueprints/api_v3.py | cat -nRepository: ChuckBuilds/LEDMatrix
Length of output: 7385
🏁 Script executed:
#!/bin/bash
# Check if _GIT is already defined anywhere
rg -n "_GIT\s*=" web_interface/blueprints/api_v3.py
# Get the function names containing the reviewed lines
sed -n '1600,1640p' web_interface/blueprints/api_v3.py | grep -E "^(def|@)" | head -5
# Check around 1674-1677
sed -n '1665,1680p' web_interface/blueprints/api_v3.py | cat -nRepository: ChuckBuilds/LEDMatrix
Length of output: 1190
Resolve git once instead of relying on PATH lookup.
These request-triggered commands invoke git by name without checking if it's available. Follow the existing _SUDO/_JOURNALCTL pattern: add _GIT = shutil.which('git') near the top of the module, validate it at the start of both execute_system_action() and get_git_info(), return a 503 error if unavailable, and pass _GIT to all subprocess.run() calls in these endpoints.
🧰 Tools
🪛 ast-grep (0.43.0)
[error] 1630-1633: Command coming from incoming request
Context: subprocess.run(
['git', 'reset', '--hard', 'origin/main'],
capture_output=True, text=True, timeout=30, cwd=project_dir
)
Note: [CWE-20].
(subprocess-from-request)
🪛 Ruff (0.15.17)
[error] 1626-1626: Starting a process with a partial executable path
(S607)
[error] 1632-1632: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web_interface/blueprints/api_v3.py` around lines 1625 - 1633, The git
commands in the subprocess.run calls rely on PATH lookup without verifying git
is available, which is inconsistent with the existing pattern used for _SUDO and
_JOURNALCTL. Add _GIT = shutil.which('git') near the top of the module alongside
the other tool resolution constants, then validate that _GIT is not None at the
start of both execute_system_action() and get_git_info() functions by returning
a jsonify response with status 503 if the git command is unavailable, and
finally replace all instances of 'git' strings in the subprocess.run() calls
within these endpoints with the resolved _GIT variable path.
Source: Linters/SAST tools
Comment on lines
+1640
to
+1646
| elif action == 'clear_pycache': | ||
| cleared = 0 | ||
| for d in PROJECT_ROOT.rglob('__pycache__'): | ||
| if d.is_dir(): | ||
| shutil.rmtree(d, ignore_errors=True) | ||
| cleared += 1 | ||
| return jsonify({'status': 'success', 'message': f'Cleared {cleared} __pycache__ directories'}) |
There was a problem hiding this comment.
Do not count failed __pycache__ deletions as cleared.
ignore_errors=True hides failures, but Line 1645 still increments cleared. Return a failure count or message so the Tools tab does not report a successful cleanup when permissions or races prevented deletion.
Proposed fix
elif action == 'clear_pycache':
cleared = 0
+ failed = 0
for d in PROJECT_ROOT.rglob('__pycache__'):
if d.is_dir():
- shutil.rmtree(d, ignore_errors=True)
- cleared += 1
- return jsonify({'status': 'success', 'message': f'Cleared {cleared} __pycache__ directories'})
+ try:
+ shutil.rmtree(d)
+ cleared += 1
+ except OSError as err:
+ failed += 1
+ logger.warning("[Tools] Failed to remove %s: %s", d, err)
+ status = 'success' if failed == 0 else 'error'
+ return jsonify({
+ 'status': status,
+ 'message': f'Cleared {cleared} __pycache__ directories; {failed} failed'
+ })As per coding guidelines, “Provide clear error messages for troubleshooting”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web_interface/blueprints/api_v3.py` around lines 1640 - 1646, The
`clear_pycache` action increments the `cleared` counter unconditionally even
though `shutil.rmtree()` is called with `ignore_errors=True`, meaning failed
deletions are still counted as successful. Fix this by removing
`ignore_errors=True` from the shutil.rmtree call in the rglob loop and wrapping
it in a try-except block. Only increment the `cleared` counter when deletion
succeeds, and track any failures in a separate counter. Return both the cleared
count and failed count (or a descriptive error message) in the jsonify response
so users get accurate feedback about what was actually deleted versus what
failed.
Source: Coding guidelines
Comment on lines
+1674
to
+1684
| branch = subprocess.run(['git', 'branch', '--show-current'], capture_output=True, text=True, timeout=10, cwd=d) | ||
| status = subprocess.run(['git', 'status', '--short', '--untracked-files=no'], capture_output=True, text=True, timeout=15, cwd=d) | ||
| log = subprocess.run(['git', 'log', '--oneline', '-5'], capture_output=True, text=True, timeout=10, cwd=d) | ||
| remote = subprocess.run(['git', 'remote', 'get-url', 'origin'], capture_output=True, text=True, timeout=10, cwd=d) | ||
| return jsonify({ | ||
| 'branch': branch.stdout.strip(), | ||
| 'dirty': bool(status.stdout.strip()), | ||
| 'status': status.stdout.strip(), | ||
| 'recent_commits': log.stdout.strip(), | ||
| 'remote_url': remote.stdout.strip(), | ||
| }) |
There was a problem hiding this comment.
Check each git command before reporting a clean repo state.
If git status fails, status.stdout is empty and this reports dirty: false. Check return codes and return a clear error/degraded payload instead of silently converting command failures into blank/clean values.
As per coding guidelines, “Validate inputs and handle errors early (Fail Fast principle)” and “Provide user-friendly error messages that explain what went wrong and potential solutions”.
🧰 Tools
🪛 ast-grep (0.43.0)
[error] 1674-1674: Command coming from incoming request
Context: subprocess.run(['git', 'status', '--short', '--untracked-files=no'], capture_output=True, text=True, timeout=15, cwd=d)
Note: [CWE-20].
(subprocess-from-request)
[error] 1675-1675: Command coming from incoming request
Context: subprocess.run(['git', 'log', '--oneline', '-5'], capture_output=True, text=True, timeout=10, cwd=d)
Note: [CWE-20].
(subprocess-from-request)
[error] 1676-1676: Command coming from incoming request
Context: subprocess.run(['git', 'remote', 'get-url', 'origin'], capture_output=True, text=True, timeout=10, cwd=d)
Note: [CWE-20].
(subprocess-from-request)
🪛 Ruff (0.15.17)
[error] 1674-1674: Starting a process with a partial executable path
(S607)
[error] 1675-1675: Starting a process with a partial executable path
(S607)
[error] 1676-1676: Starting a process with a partial executable path
(S607)
[error] 1677-1677: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web_interface/blueprints/api_v3.py` around lines 1674 - 1684, The git
subprocess commands (branch, status, log, and remote) are not checking return
codes before using their output. If any command fails, the empty stdout is
silently treated as a clean state (dirty: false, empty strings). Check the
returncode attribute of each subprocess.run() result and if any command fails
(returncode != 0), return an error response with a clear message explaining
which git command failed, rather than proceeding with empty/blank values that
misrepresent the repository state.
Source: Coding guidelines
Comment on lines
+1677
to
+1683
| remote = subprocess.run(['git', 'remote', 'get-url', 'origin'], capture_output=True, text=True, timeout=10, cwd=d) | ||
| return jsonify({ | ||
| 'branch': branch.stdout.strip(), | ||
| 'dirty': bool(status.stdout.strip()), | ||
| 'status': status.stdout.strip(), | ||
| 'recent_commits': log.stdout.strip(), | ||
| 'remote_url': remote.stdout.strip(), |
There was a problem hiding this comment.
Redact credentials before returning the remote URL.
git remote get-url origin can return an HTTPS URL with embedded credentials. Sanitize remote_url before JSON serialization, or return only host/path metadata.
Proposed fix
+ remote_url = remote.stdout.strip()
+ if '://' in remote_url:
+ from urllib.parse import urlsplit, urlunsplit
+ parts = urlsplit(remote_url)
+ host = parts.hostname or ''
+ if parts.port:
+ host = f"{host}:{parts.port}"
+ remote_url = urlunsplit((parts.scheme, host, parts.path, '', ''))
return jsonify({
'branch': branch.stdout.strip(),
'dirty': bool(status.stdout.strip()),
'status': status.stdout.strip(),
'recent_commits': log.stdout.strip(),
- 'remote_url': remote.stdout.strip(),
+ 'remote_url': remote_url,
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| remote = subprocess.run(['git', 'remote', 'get-url', 'origin'], capture_output=True, text=True, timeout=10, cwd=d) | |
| return jsonify({ | |
| 'branch': branch.stdout.strip(), | |
| 'dirty': bool(status.stdout.strip()), | |
| 'status': status.stdout.strip(), | |
| 'recent_commits': log.stdout.strip(), | |
| 'remote_url': remote.stdout.strip(), | |
| remote = subprocess.run(['git', 'remote', 'get-url', 'origin'], capture_output=True, text=True, timeout=10, cwd=d) | |
| remote_url = remote.stdout.strip() | |
| if '://' in remote_url: | |
| from urllib.parse import urlsplit, urlunsplit | |
| parts = urlsplit(remote_url) | |
| host = parts.hostname or '' | |
| if parts.port: | |
| host = f"{host}:{parts.port}" | |
| remote_url = urlunsplit((parts.scheme, host, parts.path, '', '')) | |
| return jsonify({ | |
| 'branch': branch.stdout.strip(), | |
| 'dirty': bool(status.stdout.strip()), | |
| 'status': status.stdout.strip(), | |
| 'recent_commits': log.stdout.strip(), | |
| 'remote_url': remote_url, | |
| }) |
🧰 Tools
🪛 Ruff (0.15.17)
[error] 1677-1677: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web_interface/blueprints/api_v3.py` around lines 1677 - 1683, The remote_url
field in the jsonify return statement is including the raw output from git
remote get-url, which may contain embedded credentials in HTTPS URLs (e.g.,
https://user:password@github.com/repo.git). Before including
remote.stdout.strip() as the remote_url value in the response, parse the URL
using a library like urllib.parse to remove any user authentication information,
or extract only the safe components (protocol, host, path) and reconstruct the
URL without credentials. This prevents accidental credential exposure in the API
response.
Comment on lines
+457
to
+459
| except Exception: | ||
| logger.error("Error loading partial", exc_info=True) | ||
| return "Error loading partial", 500 |
There was a problem hiding this comment.
Improve exception specificity and troubleshooting context for Tools partial errors.
At Line 457 and Line 458, the broad except Exception + generic log/message makes remote Raspberry Pi debugging harder. Catch template-related exceptions explicitly and include [Pages V3][Tools] context in logs and response text.
As per coding guidelines, "Implement comprehensive logging for remote debugging on Raspberry Pi", "Use structured logging with context", and "Provide clear error messages for troubleshooting".
Suggested patch
+from jinja2 import TemplateNotFound
@@
def _load_tools_partial():
"""Load tools/utilities partial."""
try:
return render_template('v3/partials/tools.html')
- except Exception:
- logger.error("Error loading partial", exc_info=True)
- return "Error loading partial", 500
+ except TemplateNotFound:
+ logger.error("[Pages V3][Tools] Template not found: v3/partials/tools.html", exc_info=True)
+ return "Tools panel is unavailable: template is missing.", 500
+ except OSError as exc:
+ logger.error("[Pages V3][Tools] I/O error while loading tools partial: %s", exc, exc_info=True)
+ return "Tools panel failed to load due to a file system error. Check logs.", 500🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web_interface/blueprints/pages_v3.py` around lines 457 - 459, The exception
handler for the Tools partial uses an overly broad `except Exception` clause and
lacks debugging context for remote Raspberry Pi environments. Replace the broad
`except Exception` with specific template-related exceptions (such as
TemplateNotFound, TemplateError, or other Jinja2/Flask template exceptions) to
catch only relevant errors. Update the logger.error call to include the `[Pages
V3][Tools]` context prefix in the log message to provide structured logging
context for troubleshooting. Additionally, update the returned error message
string to also include the `[Pages V3][Tools]` prefix for consistency in error
responses, ensuring both logs and client-facing messages provide clear context
about which component failed.
Source: Coding guidelines
Comment on lines
+1298
to
+1308
| <!-- Tools tab --> | ||
| <div x-show="activeTab === 'tools'" x-transition> | ||
| <div id="tools-content" hx-get="/v3/partials/tools" hx-trigger="loadtab" hx-swap="innerHTML"> | ||
| <div class="animate-pulse"> | ||
| <div class="bg-white rounded-lg shadow p-6"> | ||
| <div class="h-4 bg-gray-200 rounded w-1/4 mb-4"></div> | ||
| <div class="h-32 bg-gray-200 rounded"></div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Add a non-HTMX fallback path for the Tools tab content.
Line 1300 relies on HTMX-only loading, but the existing fallback flow in loadTabContent does not include a Tools branch. If HTMX fails, this tab can stay blank indefinitely while other tabs degrade gracefully.
💡 Suggested fix
} else if (++tries > 100) { // ~10s
clearInterval(timer);
contentEl.removeAttribute('data-loading');
if (tab === 'overview' && typeof loadOverviewDirect === 'function') loadOverviewDirect();
else if (tab === 'wifi' && typeof loadWifiDirect === 'function') loadWifiDirect();
else if (tab === 'plugins' && typeof loadPluginsDirect === 'function') loadPluginsDirect();
+ else if (tab === 'tools') {
+ fetch('/v3/partials/tools')
+ .then(r => {
+ if (!r.ok) throw new Error(r.status + ' ' + r.statusText);
+ return r.text();
+ })
+ .then(html => {
+ contentEl.innerHTML = html;
+ contentEl.setAttribute('data-loaded', 'true');
+ if (window.Alpine) window.Alpine.initTree(contentEl);
+ })
+ .catch(err => {
+ console.error('Failed to load tools content:', err);
+ contentEl.innerHTML = '<div class="bg-red-50 border border-red-200 rounded-lg p-4"><p class="text-red-800">Failed to load Tools. Please refresh the page.</p></div>';
+ });
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web_interface/templates/v3/base.html` around lines 1298 - 1308, The Tools tab
in the template relies solely on HTMX loading via hx-get without any fallback
mechanism, whereas other tabs have fallback paths in the loadTabContent
function. Add a non-HTMX fallback branch in the loadTabContent function to
handle the Tools tab (when activeTab === 'tools') so that if HTMX fails or is
disabled, the tab content can still load gracefully instead of remaining blank.
Ensure the fallback mimics how other tabs in loadTabContent degrade gracefully.
Comment on lines
+222
to
+246
| window.toolsAction = function(action, btnId, resultId, showOutput, showPluginDetails) { | ||
| setBusy(btnId, true); | ||
| const el = document.getElementById(resultId); | ||
| if (el) el.classList.add('hidden'); | ||
|
|
||
| fetch('/api/v3/system/action', { | ||
| method: 'POST', | ||
| headers: {'Content-Type': 'application/json'}, | ||
| body: JSON.stringify({action}) | ||
| }) | ||
| .then(r => r.json()) | ||
| .then(data => { | ||
| const ok = data.status === 'success'; | ||
| showResult( | ||
| resultId, ok, | ||
| data.message || (ok ? 'Done' : 'Failed'), | ||
| showOutput ? (data.output || '') : '', | ||
| showPluginDetails ? (data.details || []) : null | ||
| ); | ||
| }) | ||
| .catch(err => { | ||
| showResult(resultId, false, 'Request failed: ' + err.message); | ||
| }) | ||
| .finally(() => setBusy(btnId, false)); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Flask-WTF or similar CSRF protection is configured
# Look for CSRF configuration in Flask app
rg -n 'csrf|CSRF|WTFCsrfProtect|CSRFProtect' web_interface/
# Check if the execute_system_action endpoint has CSRF exemption
rg -n 'csrf_exempt|execute_system_action' web_interface/blueprints/api_v3.pyRepository: ChuckBuilds/LEDMatrix
Length of output: 1339
🏁 Script executed:
# Check README for deployment/usage context
head -150 web_interface/README.md | grep -A 5 -B 5 -i "local\|network\|internet\|deployment\|security"Repository: ChuckBuilds/LEDMatrix
Length of output: 322
🏁 Script executed:
# Check if there are any deployment or security documentation files
fd -e md -e txt | xargs grep -l "deployment\|production\|internet\|network" | head -20Repository: ChuckBuilds/LEDMatrix
Length of output: 647
🏁 Script executed:
# Look for any application initialization or binding configuration that might expose it to network
rg -n "host|bind|0.0.0.0|listen" web_interface/app.py web_interface/main.py web_interface/*.py | head -30Repository: ChuckBuilds/LEDMatrix
Length of output: 1687
🏁 Script executed:
# Search for the execute_system_action endpoint and its authentication
rg -n -A 20 "def execute_system_action" web_interface/blueprints/api_v3.pyRepository: ChuckBuilds/LEDMatrix
Length of output: 934
🏁 Script executed:
# Check for login_required or auth decorators on this endpoint
rg -B 5 "execute_system_action" web_interface/blueprints/api_v3.py | head -40Repository: ChuckBuilds/LEDMatrix
Length of output: 587
🏁 Script executed:
# Look for session-based auth checking in the application
rg -n "login_required|`@auth`|session\[" web_interface/ | head -20Repository: ChuckBuilds/LEDMatrix
Length of output: 47
🏁 Script executed:
# Check if there's any authentication mechanism (login required, API tokens, etc.)
rg -n "login_required|`@require_auth`|session\['user" web_interface/blueprints/api_v3.py | head -20Repository: ChuckBuilds/LEDMatrix
Length of output: 47
🏁 Script executed:
# Look for any authentication-related imports or decorators in the API blueprint
head -50 web_interface/blueprints/api_v3.pyRepository: ChuckBuilds/LEDMatrix
Length of output: 1483
🏁 Script executed:
# Check the SECURITY.md file for documented security model
cat web_interface/SECURITY.md 2>/dev/null || cat SECURITY.md 2>/dev/null || echo "File not found"Repository: ChuckBuilds/LEDMatrix
Length of output: 3329
No authentication on dangerous system action endpoints.
The execute_system_action endpoint at /api/v3/system/action (invoked by toolsAction) performs state-changing operations (git reset, pip install, service restarts) without any authentication checks. This is a broader and more critical issue than CSRF. While the application intentionally disables CSRF protection as documented in app.py for a "local network only" design (per SECURITY.md), the application binds to 0.0.0.0:5000, making it accessible to any network client—not just localhost. Any network-accessible attacker can trigger destructive operations.
Add authentication to the execute_system_action endpoint. Options:
- Require a shared API token passed in headers
- Implement a simple session-based auth check
- Document the network isolation requirement and validate with firewall rules
The CSRF token proposal in the original approach would not mitigate this issue without authentication in place first.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web_interface/templates/v3/partials/tools.html` around lines 222 - 246, The
toolsAction function makes unauthenticated requests to the /api/v3/system/action
endpoint which performs dangerous state-changing operations. Add authentication
to protect this endpoint by modifying the fetch call in toolsAction to include
an authentication token or credential in the request headers, and update the
corresponding backend execute_system_action endpoint to validate this
authentication before allowing any system action to proceed. This ensures only
authorized clients can trigger destructive operations like git reset, pip
install, or service restarts.
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.
Summary
row_address_typeoption (0–4) in the Display settings tab; the backend already consumed this value from config but there was no UI or API handling for itTools tab features
Row address type
Added a dropdown to the Display → Hardware Configuration section with all five hzeller options:
`display_manager.py` already read `hardware_config.get('row_address_type', 0)` — this PR wires up the missing UI field, API field list, and 0–4 range validation.
Files changed
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements