fix(web): keep exception messages out of API responses (py/stack-trace-exposure) - #778
Open
ChuckBuilds wants to merge 1 commit into
Open
ChuckBuilds wants to merge 1 commit into
ChuckBuilds wants to merge 1 commit into
Conversation
…e-exposure)
CodeQL had ~40 open py/stack-trace-exposure alerts on main. Almost all
flowed through describe_exception(), which returned "TypeName: message"
(redacted, capped); the rest through _run_systemctl_command's str(err),
WiFiManager's `return False, str(e)`, unit_refresh's f-strings and two
str(e)/f"{err}" messages in api_v3/__init__.py.
describe_exception() now returns a reason code -- the type, plus the
errno symbol for an OSError ("OSError:EIO", "PermissionError:EACCES") --
and logs the redacted message itself. That keeps what #538 wanted (a
failing disk still says EIO in the response) without quoting paths,
URLs or library internals, and fixes every call site at once; the
test_no_api_v3_handler_discards_its_exception policy still holds.
Service results: _get_display_service_status returns active/returncode
only, and the on-demand start/stop `service` result keeps
returncode/active/started/status but drops systemctl stdout/stderr
(logged on failure). Nothing in web_interface/static, the templates or
the MQTT bridge reads those fields. The Starlark SIGKILL-restart error
no longer returns systemctl stderr as `details`.
WiFi, unit-refresh, config-save and plugin-removal failures now say
what failed with the reason code and point at the log. display.py is
untouched (draft #773 edits it).
Tests: test_api_v3_no_exception_text.py drives one route per affected
file with a marker in the exception message and asserts it never
reaches the body; all 13 fail on origin/main, and targeted mutations
(drop the service filter, put stderr back, str(e) in WiFiManager,
{e} in unit_refresh, {install_err} in system.py, message back in
describe_exception) each fail at least one. Tests that asserted the old
message-in-details contract now assert the reason code.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (14)
💤 Files with no reviewable changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change removes exception messages and systemctl output from selected Web API responses and failure messages. Exception details now use the exception type and, for valid ChangesAPI Error Reporting
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The configuration-save failure retains a diagnostic log, and no unresolved issue identified here prevents merging after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The inspected changes reduce diagnostic information in ordinary API responses without changing command privileges or state-transition controls. Detailed logs remain accessible through existing log views, and coverage of all error paths is incomplete. No material security regression was established. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 61.54% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 65 functions across 12 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 | 7 |
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.
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.
Closes the ~40 open CodeQL
py/stack-trace-exposurealerts on main. I took the source → sink flows from the main CodeQL SARIF and fixed each source, not each sink.Sources fixed
describe_exception()(src/web_interface/error_handler.py): this fed nearly every alert, in starlark, misc, plugins, plugin_calendar, the system catch-all, wifi, andcreate_error_responsevia config.py. It now returns a reason code: the exception type plus the errno symbol for anOSError(OSError:EIO,PermissionError:EACCES,RuntimeError). The redacted message is logged instead. A failing disk still saysEIOin the response, which is the reason feat(starlark,on-demand): the third-party fixes worth taking, plus a Home Assistant MQTT bridge #538 added the helper. Every call site is fixed at once, andtest_no_api_v3_handler_discards_its_exceptionstill holds.api_v3/__init__.py):_get_display_service_statusnow returnsactive/returncodeonly._ensure_display_service_runningand_stop_display_servicekeepreturncode/active/started/statusbut dropstdout/stderr. They log stderr on failure._run_systemctl_commandno longer putsstr(err)in stderr.web_interface/static, the templates and the MQTT bridge. None of them readstdout/stderr. The bridge readsservice.activeandmessage.WiFiManager: 9return False, str(e)became e.g."Could not enable AP mode (OSError); see logs for details".unit_refresh: the two{e}interpolations are gone. Same change for_save_config'sstr(e)and the plugin-removal{remove_err}inapi_v3/__init__.py.install_base_requirementsoutput uses the reason code. The sudo hint is still matched againststr(e)but returns only its fixed text.details.display.py is untouched, because draft #773 edits it. Its alerts close through the source fixes in
__init__.py.Tests
test/test_api_v3_no_exception_text.py: one route per affected file (display, wifi + WiFiManager, system ×2, plugin_calendar, misc, config → error_handler, plugins, starlark, unit_refresh). Each puts a marker in the exception message and asserts it never reaches the body.str(e)back in WiFiManager,{e}in unit_refresh,{install_err}in system.py, message back indescribe_exception.detailscontract now assert the reason code. The redaction and bounds suites now testredact_text, which still carries subprocess output.🤖 Generated with Claude Code
Summary by CodeRabbit