fix(web): say what actually went wrong instead of "unknown" - #448
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds ChangesException reporting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant APIv3Endpoint
participant describe_exception
participant APIResponse
APIv3Endpoint->>describe_exception: format and redact exception
describe_exception-->>APIv3Endpoint: sanitized details
APIv3Endpoint->>APIResponse: return error details and status
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 | 6 |
| 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.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/web_interface/error_handler.py`:
- Around line 25-29: Extend _REDACT_CREDENTIAL and the sanitizer used by
describe_exception() to redact Authorization header values such as “Bearer
SECRET” and URL userinfo credentials such as “user:password” before the host.
Add regression cases covering both formats, while preserving existing credential
redaction behavior and client-facing error handling.
In `@test/test_web_error_detail.py`:
- Around line 79-98: Strengthen test_no_api_v3_handler_discards_its_exception so
matching exception handlers must use error-level logging with traceback details
and call describe_exception(e). Update the AST checks to reject logger.info and
other non-error levels, require exc_info=True on the error/exception logging
call, and verify the handler’s exception variable is passed to
describe_exception.
🪄 Autofix
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 Plus
Run ID: 75c4716b-2dbb-4f16-8ec6-b62d4c69807b
📒 Files selected for processing (4)
src/web_interface/error_handler.pytest/test_web_error_detail.pyweb_interface/app.pyweb_interface/blueprints/api_v3.py
ChuckBuilds
force-pushed
the
fix/surface-web-error-detail
branch
from
August 10, 2026 15:22
471e7a6 to
0b9f5e2
Compare
Owner
Author
|
All three addressed in Auth headers and URL userinfo — fixed. Both are real: The scheme ( AST test — strengthened, and it caught me out. You're right that it only asked whether something was logged; Enforcing that immediately failed on 60 handlers — the ones I'd deliberately left alone, on the reasoning that they already logged so the detail was at least in the journal. That reasoning was wrong, and the device that prompted this PR proves it: So all 69 now carry the detail. Two extra shapes needed doing by hand: two handlers bound no exception name ( CodeQL also flagged Verification
|
Owner
Author
|
Note on the CodeQL alert ( This one needs a maintainer decision rather than a code fix, because the alert describes exactly what the PR sets out to do: exception information now reaches the HTTP response. What is and isn't exposed:
The trade-off, stated plainly: these endpoints are already unauthenticated on the local network, and an attacker who can reach them can reach far more interesting things than an errno string. Against that, the message is frequently the entire diagnosis — If you'd rather not carry the alert, there are two smaller options:
Happy to implement (2) if you'd prefer the alert closed by code rather than dismissed — say the word and I'll push it. |
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/web_interface/error_handler.py (1)
83-84: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate
max_lengthbefore truncation.If
max_lengthis zero or negative, Line 84 can return a value longer than the requested limit. Reject values below one before truncating.Proposed fix
+ if max_length < 1: + raise ValueError("max_length must be at least 1") if len(text) > max_length:🤖 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 `@src/web_interface/error_handler.py` around lines 83 - 84, Validate max_length before the truncation logic in the error-handling function: reject any value below one before evaluating the len(text) truncation branch. Preserve the existing truncation behavior for valid max_length values.web_interface/blueprints/api_v3.py (1)
466-473: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not convert Flask HTTP exceptions to 500 responses.
save_schedule_config()callsrequest.get_json()at Line 330 withoutsilent=True. Flask raises a WerkzeugBadRequestfor malformed JSON andUnsupportedMediaTypefor an invalidContent-Type. The local broadexcept Exceptionrewrites these client errors asCONFIG_SAVE_FAILEDwith status 500.
web_interface/app.pypreserves HTTP exceptions only when they escape the route handler, so this local catch skips that handling. ReraiseHTTPExceptionbefore the generic API v3 exception handler and apply the same rule around other request-parsingexcept Exceptionhandlers inweb_interface/blueprints/api_v3.py. Add integration coverage for malformed JSON and incorrectContent-Type.🤖 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 466 - 473, Update save_schedule_config() and other request-parsing exception handlers in api_v3.py to re-raise Werkzeug HTTPException instances before generic Exception handling, preserving Flask’s original 4xx responses for malformed JSON and unsupported Content-Type. Keep non-HTTP failures mapped to the existing API error responses, and add integration coverage for both request errors.
🤖 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 `@src/web_interface/error_handler.py`:
- Around line 35-39: Update _REDACT_AUTH_HEADER so it matches and redacts any
Authorization authentication scheme, not only the listed
Bearer/Basic/Digest/Token values, while preserving capture of the credential for
replacement. Add regression cases covering unsupported schemes such as ApiKey
and Negotiate and verify their secrets are absent from client responses.
In `@test/test_web_error_detail.py`:
- Around line 123-145: Update returns_the_detail() to inspect only Return
expressions in the exception handler, requiring a matching
describe_exception(exception_name) call within the returned jsonify or
error_response payload. Do not accept calls that are merely computed elsewhere
or discarded; preserve the bare except behavior where no exception name exists.
---
Outside diff comments:
In `@src/web_interface/error_handler.py`:
- Around line 83-84: Validate max_length before the truncation logic in the
error-handling function: reject any value below one before evaluating the
len(text) truncation branch. Preserve the existing truncation behavior for valid
max_length values.
In `@web_interface/blueprints/api_v3.py`:
- Around line 466-473: Update save_schedule_config() and other request-parsing
exception handlers in api_v3.py to re-raise Werkzeug HTTPException instances
before generic Exception handling, preserving Flask’s original 4xx responses for
malformed JSON and unsupported Content-Type. Keep non-HTTP failures mapped to
the existing API error responses, and add integration coverage for both request
errors.
🪄 Autofix
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 Plus
Run ID: f8221799-ee5d-474a-9294-690aecd6aed8
📒 Files selected for processing (4)
src/web_interface/error_handler.pytest/test_web_error_detail.pyweb_interface/app.pyweb_interface/blueprints/api_v3.py
Every failing endpoint returned "An error occurred; see logs for
details" and nothing else. That is survivable until the logs are the
thing you cannot reach: a device whose SD card was failing answered the
restart action, /system/status and /logs with that same sentence -- the
log viewer included, because journalctl could not be executed -- while
the exception underneath said
[Errno 5] Input/output error: 'systemctl'
which names the fault outright. The only endpoint that helped was
/health, and only because it happens to pass a subprocess's stderr
through. Diagnosis came down to guessing which endpoint leaked something.
Add describe_exception(), returning "TypeName: message" on one line, and
populate the `details` field that the response schema has always had and
nothing ever filled. The type alone carries information -- a bare
PermissionError says more than any generic sentence.
Exception text is not automatically safe to echo: a requests error
quotes the URL it failed on, and plugins that authenticate by query
string put their key there. Credential values are redacted while the
parameter name is kept, since knowing which credential was involved is
part of the diagnosis. Length is capped and newlines collapsed so a
parser's context cannot flood a JSON field.
Nine handlers in api_v3 bound the exception and never used it, so the
promised log entry was never written either -- "see logs for details"
was false, not merely unhelpful. Those now log with a traceback and
carry the detail. The other 60 already logged and are unchanged; they
can adopt the helper as they are touched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Three review findings.
The sanitizer missed two credential shapes that requests puts in its
exception text verbatim: `Authorization: Bearer <token>` and
`https://user:password@host`. Both would have gone straight into a
response. The auth-scheme name and the username are kept -- they say
which credential and whose without being the secret.
The AST test only asked whether *something* had been logged, so a
`logger.info("failed")` satisfied it while discarding the exception just
as completely. It now requires an error-level record carrying exc_info
and `describe_exception()` called on the handler's own bound exception.
Enforcing that revealed the first cut had scoped itself wrongly. I had
converted the nine handlers that logged nothing and left the sixty that
logged, reasoning their detail was at least in the journal. But
/system/status is one of the sixty, and on the failing device it told me
nothing -- the journal was exactly what could not be read. Splitting
them left most of the diagnostic surface unhelpful for the case this
change exists for, so all sixty-nine now carry the detail.
Two handlers had no bound exception name, and three passed the message
through a variable rather than a literal; both shapes needed doing by
hand. Full suite: 2383 passed, one pre-existing unrelated failure.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Werkzeug's HTTPExceptions subclass Exception, so the catch-all handler saw them too and turned every 405, 400, 413 and 415 into a 500 UNKNOWN_ERROR. A GET on a POST-only route answered "an error occurred; see logs for details", which tells the caller nothing and blames the wrong side -- found while probing a device whose POST-only config endpoints did exactly that. Hand HTTPExceptions back as themselves, with their own status and description. A genuine server fault still reports as one, with the detail this branch adds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Two review findings. The auth-header pattern listed Bearer, Basic, Digest and Token, so `Authorization: ApiKey SECRET` or `Negotiate SECRET` went to the client intact. A fixed list silently leaks whatever it does not name, and plugin APIs invent their own schemes, so match any scheme name and keep it while redacting the credential. The AST test accepted a describe_exception(e) call anywhere in the handler, which a handler could satisfy by computing the detail and dropping it before returning the generic message. It now requires the call inside every return expression, which is where it has to be to reach the caller. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
ChuckBuilds
force-pushed
the
fix/surface-web-error-detail
branch
from
August 11, 2026 17:35
83a1f92 to
6ae69df
Compare
ChuckBuilds
added a commit
that referenced
this pull request
Aug 13, 2026
CodeQL flagged four exposure paths. Two were mine and genuinely raw: the OSError from failing to spawn calendar_registration.py, which carries the interpreter path and whatever the OS chose to say, and the ImportError for the Google libraries, whose message named the missing module by interpolating the exception directly. Both now go through describe_exception, and the unredacted text goes to the log. The other two are the repo-wide pattern from PR #448 -- 67 handlers on main already return details=describe_exception(e), and these two new handlers follow it. That function is the sanitizer: it strips URL userinfo, auth headers and credential-shaped key=value pairs, collapses to one line and caps the length. CodeQL's taint tracking cannot see a sanitizer it has no model for, so it reports the flow regardless. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
ChuckBuilds
added a commit
that referenced
this pull request
Aug 13, 2026
* fix(calendar): implement the OAuth and calendar-listing endpoints The plugin's config advertised a three-step setup and only step 1 existed. Step 3's picker fetched /api/v3/plugins/calendar/list-calendars, which was never registered, so Flask fell through to the global 404 handler and the user saw "Resource not found" -- a message that names nothing and points nowhere. Step 2 had no endpoint at all, so even a working picker would have found no token to list with. Two routes, following the pattern the spotify and ytm plugins already use for their own auth scripts: POST /plugins/calendar/authenticate two-step Google OAuth GET /plugins/calendar/list-calendars calendars for the picker The authenticate route drives calendar_registration.py, which the plugin already ships and which was written expressly for this -- it reads a redirect URL on stdin and prints one JSON object. It takes two calls because a human has to visit Google in between; the script persists the PKCE verifier from the first call for the second, without which the exchange fails with "Missing code verifier". The listing route reads the token directly rather than shelling out again: the picker is interactive and a subprocess per click is slower than the API call it would wrap. It refreshes an expired token in place, sorts the primary calendar first, and drops entries with no id, which could not be selected anyway. Both name the plugin when it is not installed, rather than reproducing the anonymous 404 that started this. Verified against the live Google API on the dev rig: HTTP 200 with the account's real calendars. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * fix(calendar): one input for the auth code, and a louder warning Two things reported after testing the flow. There were two boxes and no way to tell which to use. The config template's string branch dispatches widgets from an allow-list of names, and anything missing from it falls through to a plain input type=text -- so the field rendered both the widget's own box and a stray one for the same key. google-oauth is now on that list, which is all the widget ever needed to render in place of the fallback rather than beside it. And the warning that the redirect page fails to load was small grey text under a link, which is where it is least likely to be read. It is now an amber callout that leads with "The next page will fail to load. That is expected." The failure lands at exactly the moment the user has to act on it, and it looks precisely like the flow breaking rather than working. The paste box is labelled too, rather than relying on a placeholder that vanishes on focus. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * fix(calendar): redact script diagnostics, and page the calendar list Three findings from CodeRabbit, all valid. Raw subprocess output was being returned to the client -- the script's stderr on one path, and its own error payload on another. CodeQL flagged the same line. That script handles OAuth client secrets and interpolates exceptions into its messages, so either could carry a secret or a path. Both now go to the log unredacted, where they are worth having in full, and reach the client through a redactor. That redactor already existed inside describe_exception, which only takes exceptions. Split out as redact_text: an exception is not the only thing worth returning, and a subprocess's stderr is just as capable of quoting a token. calendarList.list returns 100 entries per page by default, caps at 250, and hands back a nextPageToken when there are more. Reading one page would have hidden calendars from the picker with nothing to say the list was cut short. It now pages, asking for 250 at a time, bounded at ten pages so a malformed token cannot spin. And a test helper was a lambda where ruff wants a def. The five new tests fail against the previous commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * fix(calendar): redact the last two raw exception interpolations CodeQL flagged four exposure paths. Two were mine and genuinely raw: the OSError from failing to spawn calendar_registration.py, which carries the interpreter path and whatever the OS chose to say, and the ImportError for the Google libraries, whose message named the missing module by interpolating the exception directly. Both now go through describe_exception, and the unredacted text goes to the log. The other two are the repo-wide pattern from PR #448 -- 67 handlers on main already return details=describe_exception(e), and these two new handlers follow it. That function is the sanitizer: it strips URL userinfo, auth headers and credential-shaped key=value pairs, collapses to one line and caps the length. CodeQL's taint tracking cannot see a sanitizer it has no model for, so it reports the flow regardless. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * fix(calendar): announce status changes, name the paste box, drop a no-op Three more from the review, all valid. The status line is written after every async call -- the consent link is ready, the exchange failed -- and was a plain paragraph, so a screen reader was told none of it. It is a live region now. The paste box had a visible label that was never associated with it, so its only accessible name was the placeholder, which disappears on focus: precisely when the value is being pasted. The label now points at the input by id. And a conditional in the test helper returned the same value from both branches, which Ruff flags as RUF034. It was left over from making the fake page; one page is all those cases need, and TestPagination builds its own sequences. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * test(calendar): assert the accessibility relationships, not their parts The previous assertions searched for role="status", aria-live, a label `for` and an input `id` independently, so they passed whether or not those belonged together. Two attributes on different elements announce nothing, and a `for` that names something other than the input leaves it just as anonymous. Both attributes are now asserted on the status element itself, and the label and input are checked to go through the same identifier rather than merely both existing. Verified by mutation: a mismatched pair and a displaced aria-live are both caught. Reported by CodeRabbit, against tests I had written two commits earlier. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Found while diagnosing a device that returned this from the web UI after an update + restart:
{"error_code":"UNKNOWN_ERROR","message":"An error occurred; see logs for details","status":"error"}Why the generic message failed
The device's SD card was failing. Every endpoint answered with that same sentence — the restart action,
/system/status, and/logs, becausejournalctlcould not be executed either. So the message pointed at logs that the same fault made unreachable, and SSH was down for the same reason (sshd resetting before its banner, unable to read its host keys).The exception underneath said:
That names the fault outright —
EIOon exec is a failing block device, not a missing file or a permissions problem. The only endpoint that helped was/health, and only because it happens to pass a subprocess'sstderrthrough. Diagnosis came down to guessing which endpoint leaked something useful.The change
describe_exception()returns"TypeName: message"on one line, and it populates thedetailsfield thatWebInterfaceError.to_dict()anderror_response(details=...)have always supported and nothing ever filled. The type alone carries information — a barePermissionErrorsays more than any generic sentence./api/v3/logsunder the same failure, before and after:Exception text is not automatically safe to echo. A
requestserror quotes the URL it failed on, and plugins that authenticate by query string put their key there — so returning exceptions verbatim would hand out API keys. Credential values are redacted and the parameter name kept, since knowing which credential was involved is part of the diagnosis:Length is capped at 400 chars and newlines collapsed, so a parser's worth of context can't flood a JSON field.
The nine liars
Of 69 handlers returning the generic message, 60 log it properly. Nine bound the exception and never used it — no log line either, so "see logs for details" was false rather than merely unhelpful.
/logswas one of them, which is what nearly dead-ended the diagnosis. Those nine now log with a traceback and carry the detail; a test asserts none are left.The other 60 are untouched. They already log, so the information exists on a healthy box, and rewriting 60 call sites in the same PR is a large blast radius for a smaller gain — they can adopt the helper as they're touched.
Verification
api_v3handler discards its exception./api/v3/logswithsubprocess.runpatched to raiseEIO— returns the string above and logs the traceback.test_install_lowmem.py::TestDiskBackedTmpdir, which fails identically on a clean tree whereverTMPDIRis set).🤖 Generated with Claude Code
https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Summary by CodeRabbit