fix(miner): fail closed when a chat-action handler throws#7041
fix(miner): fail closed when a chat-action handler throws#7041shin-core wants to merge 1 commit into
Conversation
dispatchChatAction wrapped its paramsValidator call in a try/catch
("fail closed") but left the next line, await registered.handler(request),
unguarded -- a throwing handler became an unhandled rejection instead of
the module's typed { ok, status } contract every other failure path
returns.
Wrap the handler call in try/catch, returning
{ ok: false, status: "handler_error", action, error } on throw, mirroring
the paramsValidator catch above. The distinct handler_error status lets a
caller tell an execution failure from a validation failure.
Closes JSONbored#6989
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
| // A handler that throws fails closed with the module's typed result shape (#6989), consistent with the | ||
| // paramsValidator catch above -- a distinct "handler_error" status lets a caller tell an execution | ||
| // failure apart from a params-validation failure. | ||
| return { ok: false, status: "handler_error", action, error: error instanceof Error ? error.message : String(error) }; |
There was a problem hiding this comment.
P2: Handler error messages returned to caller may leak sensitive internal details
Raw handler error messages are returned to the caller, risking internal detail leakage.
Log the error internally and return a generic error code instead of the raw message.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="packages/loopover-miner/lib/chat-action-dispatch.js">
<violation number="1" location="packages/loopover-miner/lib/chat-action-dispatch.js:81">
<priority>P2</priority>
<title>Handler error messages returned to caller may leak sensitive internal details</title>
<evidence>The catch block for registered.handler(request) returns `error: error instanceof Error ? error.message : String(error)` directly in the typed result object. If this function is invoked from a REST endpoint, internal error details such as database connection strings, file paths, or secrets could leak to external clients.</evidence>
<recommendation>Log the full error internally and return a generic error identifier (e.g., an error code or trace ID) to the caller instead of the raw error message.</recommendation>
</violation>
</file>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7041 +/- ##
===========================================
+ Coverage 62.24% 93.74% +31.50%
===========================================
Files 692 692
Lines 68748 68750 +2
Branches 18769 18769
===========================================
+ Hits 42793 64453 +21660
+ Misses 23075 3302 -19773
+ Partials 2880 995 -1885
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Important 🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪 🔍 LoopOver is reviewing…AI analysis is in progress. This comment will update when the review is complete. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟪 Reviewing |
JSONbored
left a comment
There was a problem hiding this comment.
Outstanding superagent issue, closing.
What
dispatchChatAction(packages/loopover-miner/lib/chat-action-dispatch.js) wraps itsparamsValidatorcall in a try/catch — explicitly "fail closed" — but the next line,await registered.handler(request), was unguarded. A handler that throws (e.g. a network error from a governor action) would surface as an unhandled rejection instead of the module's own typed{ ok, status }contract that every other failure path already returns.This is latent today (the dashboard-chat REST endpoint that invokes it is still an open epic, #6839), but wiring it live would turn any handler throw into an unhandled rejection.
How
Wrap
registered.handler(request)in try/catch, returning{ ok: false, status: "handler_error", action, error }on throw — mirroring theparamsValidatorcatch immediately above. The distinct"handler_error"status (vs"invalid_params") lets a caller tell an execution failure apart from a validation failure. This matches the fail-closed catch pattern used by the package's other must-never-crash paths (pretooluse-hook.js:32,sentry.js:37,47).Validation
Tests added to the existing
test/unit/miner-chat-action-dispatch.test.ts: a handler throwing anErroryieldshandler_errorwith the message, and a non-Errorthrow is stringified — covering both sides of theinstanceof Errorternary. Confirmed both fail against the unfixed code (unhandled rejection) and pass with the fix; 100% branch coverage on the file.Closes #6989