fix(backup): address CodeRabbit and CodeQL findings on PR #439 - #440
Conversation
- first_time_install.sh: verify chown/chmod succeed and the final owner/group/mode on config_secrets.json before reporting success; exit with a clear error otherwise instead of swallowing failures. - api_v3.py: replace the predictable .writetest probe with an exclusive NamedTemporaryFile to avoid a race with concurrent resolvers; log the preferred/fallback export path and OSError when falling back to the reinstall-deleted directory. - api_v3.py: mark a restore as failed when plugin reinstalls fail, even if file restoration itself succeeded, so the endpoint no longer reports HTTP 200 success on a partial restore. - api_v3.py: stringify plugin IDs before joining them into the error message so a malformed backup's non-string plugin_id can't raise a TypeError and mask the detailed response. - backup_manager.py / api_v3.py: stop putting raw exception text (originating from a user-controlled backup file) into restore results returned to the client; log full details server-side instead. Addresses the CodeQL "stack trace information exposure" alert. - test coverage: add a test for get_plugin_info() resolving a manifest id, and assert the disabled restore_wifi path also skips and omits ytm_auth.json.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 1 high |
🟢 Metrics 0 complexity · 0 duplication
Metric Results Complexity 0 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.
|
Investigated the Codacy "1 high" security alert. The bot's summary comment and the check-run output don't include the specific file/line/rule — only the count — and I don't have access to the Codacy dashboard itself ( To narrow it down anyway: If someone with dashboard access can grab the specific rule/file/line from https://app.codacy.com/gh/ChuckBuilds/LEDMatrix/pull-requests/440/issues, I'll fix it directly rather than guess further. I'll keep watching this PR for the next update either way. Generated by Claude Code |
2 participants
Addresses the actionable CodeRabbit review comments and the GitHub Advanced Security (CodeQL) alert posted on #439.
Changes
first_time_install.shchown/chmodcalls onconfig_secrets.jsonno longer swallow failures with|| true. Both operations are checked, the final owner/group/mode are verified withstat, and the script exits with a clear troubleshooting message if anything is off — instead of printing "✓ Secrets file owned by..." regardless of whether it actually happened.web_interface/blueprints/api_v3.py_resolve_backup_export_dir()'s writability probe (preferred / ".writetest") was a predictable, shared path — a concurrent resolver could truncate/delete another process's probe. Replaced withtempfile.NamedTemporaryFile, which is exclusive to the call that created it.config/backups/exports, that's now logged (preferred path, fallback path, and theOSError) so a Raspberry Pi operator can tell their backups aren't safe from the next reinstall.backup_restore()now setsresult.success = Falsewhenresult.plugins_failedis non-empty. Previously, a restore whose files all landed but whose plugin reinstalls failed still returned HTTP 200 withstatus: success.plugin_idvalues (sourced from an uploadedplugins.json) are stringified before being joined into the error message, so a malformed backup with a non-string id can't raiseTypeErrorand replace the detailed partial-restore response with the generic 500 handler.src/backup_manager.py+api_v3.py(CodeQL: Information exposure through an exception)RestoreResult.errors/plugins_failed[].error, which round-trip into the JSON response. Each failure now logs the full exception server-side ([Backup] ...,exc_info=True) and reports a category-level message to the client (e.g."Failed to restore config.json"instead of embedding the rawOSErrorstring/path).Tests
test/test_registry_id_resolution.py: addedtest_get_plugin_info_resolves_manifest_id, covering theget_plugin_info()delegation path (network-free,fetch_latest_from_github=False) that the existing tests only exercised viaget_registry_info().test/test_backup_manager.py:test_restore_honors_optionsnow also asserts that disablingrestore_wifiskipsytm_authand leaves noytm_auth.jsonbehind, since it rides on the same flag.Test plan
pytest test/test_backup_manager.py test/test_registry_id_resolution.py— 21 passedbash -n first_time_install.sh— syntax OKpython -m py_compileon all touched Python filesGenerated by Claude Code