fix(api): stop broad handlers masking faults in the webhook/admin surface

[cbcoutinho]

Context

Follow-up to the reviewer's observation on #1092: a broad except Exception → return error response swallowed a real fault in payload_backfill.py, so its test failed for a reason unrelated to the actual error. I audited the whole admin/webhook surface (28 broad-catch sites) to see how far the pattern ran.

Full inventory + evidence: Nextcloud note 390881. Card: Deck #679.

Audit tally: 12 masking / 12 too broad / 4 legitimate fail-open. Zero carried a comment justifying the breadth. Zero used logger.exception — no traceback was emitted anywhere in this surface, including at catch-all 500s.

This PR fixes the reachable bugs plus the reason none of them were diagnosable. Narrowing the merely-too-broad catches, the 401 conflation, and the 503-vs-500 mapping are left as follow-ups on #679.

Changes

1. _sanitize_error_for_client: logger.error → logger.exception (management.py:242)

This helper backs every catch-all 500 in the management/access surface, and the client deliberately receives only a generic message — so this log line is the sole record of what failed, and it was discarding the traceback. A 500 from a ~45-line try block gave no way to tell a Qdrant outage from a TypeError in response building.

I verified all 28 call sites are inside except blocks, so the traceback is always live (logger.exception outside a handler would log NoneType: None). This is a one-line change that reaches all 28.

2. create_webhook: malformed body is 400, not 500 (webhooks.py)

await request.json() sat inside the catch-all, so bad JSON returned 500 "Internal error" — telling the client the server is broken when the client sent bad JSON, and burying a routine 400 in the error logs. A non-dict body ([]) hit body.get → AttributeError → also 500.

Not a stylistic point: delete_webhook already guards its int() parse exactly this way (webhooks.py:292-298), so this was an internal inconsistency, not house style.

3. _register_preset_webhooks: roll back on failure (webhook_routes.py)

The helper returns ids only after every event registers. A failure on event N left events 1..N-1 live in Nextcloud while the caller's store_webhook never ran — orphaned registrations, invisible to the UI, still delivering — while the handler reported "Failed to enable preset", i.e. that nothing happened. Now the already-created webhooks are deleted before re-raising, so the reported end state matches reality. Rollback is best-effort and never replaces the original error (that's the one that explains the failure) — there's a test for that.

4. _get_enabled_presets: propagate instead of returning {} (webhook_routes.py)

{} is indistinguishable from the truthful "no presets are enabled", and both callers act on the difference:

• the pane rendered "Not Enabled" for presets that are live → an admin clicking Enable double-registers every event;
• the storage-less disable path no-op'd its delete loop and still returned the success card.

Both callers already wrap this in a handler that surfaces an error, so failing honestly is strictly better than answering wrongly. Behavior change: a transient listing failure now shows an error in the pane instead of a silently wrong state.

Two audit findings I did NOT fix (and why)

access.py:172 (scope update) and management.py:636 (revoke) look like post-write state divergences — a durable write followed by a cache invalidation under one catch, which would report a committed write as a 500 failure. I initially flagged both as security-adjacent bugs. On checking reachability, both are false positives: the post-write steps are _scope_cache.pop(user_id, None) on a plain dict, and an awaited in-memory del guarded by an if user_id in self._cache. Neither can realistically raise, and CancelledError is a BaseException so it isn't caught either. If the step before them raises, nothing was committed and the 500 is correct.

The structure is fragile and worth tidying, but there is no live bug — recorded as structural nits on #679 rather than churned here.

Verification

Every fix has a regression test proven to fail against master's code:

Test	Failure on master
test_malformed_json_returns_400_not_500	assert 500 == 400
test_non_object_json_body_returns_400_not_500	assert 500 == 400
test_register_rolls_back_already_created_webhooks_on_failure	assert [] == [42, 43]
test_enabled_presets_failure_propagates_...	DID NOT RAISE
test_sanitizer_logs_traceback	traceback was not captured

The JSON tests drive a real request through a Starlette TestClient against the actual handler, not a mock.

• Exact CI selection green: uv run pytest -m unit -o "addopts=-p no:asyncio" → 2387 passed (2379 baseline + 8 new), no regressions.
• ruff / ruff format / ty / commitizen clean; sonar analyze secrets clean.

Test coverage (ADR-029 gate)

This changes the behavior of POST /api/v1/webhooks (malformed input now 400 rather than 500). Per CLAUDE.md I'm stating the coverage position explicitly rather than leaving it silent:

• Unit/e2e-ish: the new tests exercise the real handler end-to-end via TestClient.
• Contract: the authenticated /api/v1/* surface — including webhooks CRUD — is not pact-verified today; provider verification covers only the public endpoints pending the ADR-029 phase-4 Bearer-token hook. That gap is pre-existing and already tracked on Deck board 11; this PR does not widen it. The status-code change is in the direction of the HTTP contract (client fault → 4xx), so a future pact should encode 400.

Refs: Deck #679

---

This PR was generated with the help of AI, and reviewed by a Human

[claude]

Claude finished @cbcoutinho's task in 2m 19s —— View job

---

Review: fix(api): stop broad handlers masking faults in the webhook/admin surface

• Gather context (read diff, changed files)
• Understand the request (code review requested per repo custom instructions)
• Review management.py change
• Review webhooks.py change
• Review webhook_routes.py change
• Review test coverage additions
• Post final review

Summary

This is a well-scoped, well-evidenced fix. I traced all four changes against their call sites and the two "investigated but not fixed" audit findings, and they hold up.

Findings

1. _sanitize_error_for_client → logger.exception (management.py:242) ✅
Correct and low-risk. I confirmed via grep -rn "_sanitize_error_for_client(" nextcloud_mcp_server/ that every call site sits inside an except block, so logger.exception's implicit sys.exc_info() pickup is always valid — no risk of the "NoneType: None" footgun the PR body calls out.

2. create_webhook malformed-body handling (webhooks.py:191-227) ✅
Verified the final state (post bc8469d) is clean: except ValueError (not the redundant (ValueError, UnicodeDecodeError) tuple — good call resolving that in favor of the Sonar finding over house-style consistency), the isinstance(body, dict) guard for non-object JSON, and the shared _BAD_REQUEST constant. The parse-then-validate-then-try restructuring correctly moves request.json() and the .get() calls outside the catch-all, so a malformed body can no longer fall into the generic 500 path. I confirmed the rest of the handler (OAuth/provisioning/503-for-no-secret branches) is untouched by the restructuring.

3. _register_preset_webhooks rollback (webhook_routes.py:166-222) ✅
Traced both callers (enable_webhook_preset:520 and its test double). The rollback loop is correctly scoped: webhook_auth_pair() (which can raise WebhookSecretNotConfigured) runs before the try, so a missing-secret failure doesn't trigger a spurious rollback of zero webhooks — matches the docstring. The nested except Exception: logger.exception(...) around the cleanup delete_webhook call correctly avoids masking the original error while still logging a traceback for the cleanup failure itself (an operator now has enough signal to find and manually delete the orphan). One subtlety worth noting for future readers: because webhook_data["id"] (line 196) is now inside the same try, a malformed response missing "id" also triggers rollback of prior events — that's a behavior change from before this PR (previously that line sat outside any try), but it's strictly more correct, not a regression.

4. _get_enabled_presets propagates instead of swallowing to {} (webhook_routes.py:281-350) ✅
Confirmed both call sites (webhook_management_pane:400, enable_webhook_preset via disable_webhook_preset:623) already sit inside their own enclosing try/except Exception blocks that render an error state, so the propagation change can't produce an unhandled 500 outside Starlette's default handler. The updated docstring's Raises: section is accurate and gives future editors the "why" so the swallow doesn't get reintroduced.

Audit findings correctly left unfixed — I checked both by hand:

• access.py:172 (invalidate_scope_cache) → auth/scope_authorization.py:580-582 is a synchronous dict.pop(user_id, None), cannot raise.
• management.py:636 (token_broker.cache.invalidate) → TokenCache.invalidate (token_broker.py:82-86) is an async with self._lock: if user_id in self._cache: del self._cache[user_id]. Only a cancellation could interrupt it, and CancelledError is BaseException, not caught by except Exception.

Both write-ups match the PR body's reasoning exactly — good discipline not to churn code for a non-bug.

Test coverage

The five new/extended test files are strong: test_malformed_json_returns_400_not_500 and test_non_object_json_body_returns_400_not_500 drive a real Starlette TestClient against the actual handler (not mocked), test_register_rolls_back_already_created_webhooks_on_failure asserts rollback ordering via delete_webhook.call_args_list, and test_register_rollback_failure_does_not_mask_original_error specifically pins the "cleanup failure must not replace the original error" contract — exactly the kind of case that's easy to regress silently. test_sanitizer_logs_traceback correctly asserts on record.exc_info rather than message content, which is the only way to actually verify logger.exception vs logger.error behavior.

Minor / non-blocking observations

• webhooks.py's new pre-try JSON parsing doesn't have its own catch-all for non-ValueError failures during body reading (e.g. a mid-request client disconnect), so such an error would now propagate past this function's handlers to Starlette's default 500 instead of being logged via _sanitize_error_for_client. This mirrors the pre-existing idiom in access.py/passwords.py rather than introducing a new gap, so I don't think it needs fixing here — just flagging for awareness given it's now one more call site with the same shape.
• Nothing else stood out — no security, performance, or style issues in the diff itself.

Verification note

I was unable to execute uv run pytest / ruff / ty in this sandbox (Bash tool required approval that wasn't available in this run), so I relied on careful reading of the diff, call-site tracing, and cross-checking the PR's own stated results (2387 passed locally, green CI unit-test job, SonarCloud quality gate passed per the bot comment above). If you'd like, re-run /pre-push-review or the CI status can be pulled directly to double-check.

Overall: solid fix, correctly scoped, well-tested, and the "what we didn't fix and why" section shows real rigor rather than reflexive over-fixing. No blocking issues found.

[cbcoutinho]

Round-1 findings addressed in 9ec9d25d. Took two of the three:

• Observation 2 (adjacent catch-alls still on logger.error): fixed rather than deferred. You're right that webhook_management_pane (:464), enable_webhook_preset (:566) and disable_webhook_preset (:658) discard the traceback — that's the same defect this PR fixes for the _sanitize_error_for_client sites, they just don't route through that helper so the one-line fix missed them. They're in a file this PR already touches, so folding them in beats losing them to a follow-up. All five catch-alls in webhook_routes.py now capture the stack.
• Observation 3 (_get_enabled_presets docstring): fixed — documented both the new Raises contract and why it raises, so the swallow-and-return-{} pattern doesn't get reintroduced by someone reading only the signature.

Left as-is — Observation 1 (except (ValueError, UnicodeDecodeError) is redundant): correct on the facts (UnicodeDecodeError subclasses ValueError; I verified), but the tuple is the established idiom in this codebase — access.py:108 and passwords.py:402 both spell it out around request.json(). Matching the surrounding modules seems more valuable than the shorter spelling, and it documents which inputs are expected. Happy to collapse it to except ValueError if you'd rather have the terser form.

On your "Not run" note — fair, and worth having a number rather than my say-so: exact CI selection uv run pytest -m unit -o "addopts=-p no:asyncio" → 2387 passed locally after this change, and CI's own unit-test job is green on the PR. ruff/format/ty/commitizen clean.

One correction to your review for the record: you found 9 _sanitize_error_for_client call sites; the actual count is 28 (grep -rn "_sanitize_error_for_client(" nextcloud_mcp_server/ | grep -v "def _sanitize" | wc -l). Doesn't change the conclusion — I checked every one of the 28 is inside an except block, so the traceback is live at each — but the one-liner reaches rather more of the surface than the review implies.

[cbcoutinho]

Round-2 + SonarCloud addressed in bc8469d7.

SonarCloud flagged 2 issues on the new code (gate passed, but per CLAUDE.md these get fixed in the loop, not after):

• python:S5713 — except (ValueError, UnicodeDecodeError) is redundant. You were right and I was wrong: I defended the tuple on repo-precedent grounds last round, but with you and Sonar flagging it independently, "matches the neighbours" doesn't outweigh "it's redundant." Collapsed to except ValueError, keeping a comment that names what it covers — that's the non-obvious part worth writing down, not the tuple. (access.py:108 / passwords.py:402 presumably trip the same rule; they're pre-existing so they don't count as new code — noted for fix: support cloud OAuth clients and graceful DCR fallback #679.)
• python:S1192 — my new 400s pushed the "Bad request" literal to 5 occurrences. Extracted _BAD_REQUEST.

Behavior is unchanged — verified json.JSONDecodeError and UnicodeDecodeError are both ValueError subclasses, so the same inputs still yield 400, and the 15 tests in test_webhooks_api_auth.py still pass.

On your two observations:

• Rollback skipped on BaseException/cancellation — correct, and I'd rather leave it. Catching BaseException to run cleanup would mean swallowing cancellation semantics for a case where the event loop is already tearing down; the codebase convention is not to, and a finally-based rollback would fire on the success path too. Logged against fix: support cloud OAuth clients and graceful DCR fallback #679 alongside the other BaseException-adjacent notes rather than solved here.
• Two log lines per failure at the catch-all sites (one without exc_info from the handler, one with from the helper) — agreed it's redundant, and it's the flip side of the double-logging I flagged in the audit. Deliberately out of scope: deduping means touching every catch-all in the surface, which is exactly the fix: support cloud OAuth clients and graceful DCR fallback #679 follow-up.

On "what I could not run" — CI has since finished: all non-review checks green, unit-test job green, SonarCloud Quality Gate passed. Locally uv run pytest -m unit -o "addopts=-p no:asyncio" → 2387 passed; ruff/format/ty/commitizen clean; sonar analyze secrets clean.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud