fix(http): stop leaking tracebacks in JSONL/RPC error responses#361
fix(http): stop leaking tracebacks in JSONL/RPC error responses#361RealDiligent wants to merge 1 commit into
Conversation
Unexpected exceptions in handle_request() included full stack traces in the JSON error envelope, exposing paths and internals to HTTP /rpc clients. Log server-side and return message only. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughThe JSONL server's generic exception handler was updated to use Python's logging module instead of including a traceback string in error responses. Exceptions are now logged server-side via a module-level logger, and the client-facing internal_error payload only contains a code and message. A regression test was added to verify traceback data is no longer exposed. ChangesInternal Error Response Hardening
Estimated code review effort: 1 (Trivial) | ~5 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant JsonlServer as handle_request
participant Logger as _log
Client->>JsonlServer: request (e.g. kb.status)
JsonlServer->>JsonlServer: handler raises unexpected exception
JsonlServer->>Logger: _log.exception(...)
JsonlServer->>Client: {ok: false, error: {code: internal_error, message}}
Related PRs: None identified. Suggested labels: bug, security Suggested reviewers: None identified. 🐰 A traceback once leaked through the wire, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
github-actions
Bot
added
mcp
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_jsonl_server.py (1)
239-250: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't cover message-content leakage.
Good regression coverage for the traceback field, but the test doesn't assert on
resp["error"]["message"]. Given the handler raisesRuntimeError("secret internals"), the current implementation would actually return that string inmessage— the exact class of leak the PR aims to prevent. If themessagefix above is adopted, add an assertion here (e.g.,assert "secret internals" not in resp["error"]["message"]) to lock in that behavior too.🤖 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 `@tests/test_jsonl_server.py` around lines 239 - 250, The regression test for `handle_request` only checks that `resp["error"]` omits the traceback, but it still allows the raised `RuntimeError("secret internals")` to leak through `resp["error"]["message"]`. Update `test_jsonl_internal_error_omits_traceback` to also assert that the error message does not contain the exception text, using the existing `jsonl_server.HANDLERS` monkeypatch and `handle_request` flow so the non-leak behavior is covered end to end.
🤖 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/vouch/jsonl_server.py`:
- Around line 765-773: The `except Exception as e` handler in `jsonl_server.py`
still returns handler-controlled text via `message: str(e)`, which can leak
sensitive internals to `/rpc` clients. Update the `internal_error` response to
use a generic, static message instead of the raw exception text, and keep the
detailed exception only in `_log.exception("internal error handling %s",
method)` so `req_id`/`method` stay contextual while `str(e)` is not exposed over
HTTP.
---
Nitpick comments:
In `@tests/test_jsonl_server.py`:
- Around line 239-250: The regression test for `handle_request` only checks that
`resp["error"]` omits the traceback, but it still allows the raised
`RuntimeError("secret internals")` to leak through `resp["error"]["message"]`.
Update `test_jsonl_internal_error_omits_traceback` to also assert that the error
message does not contain the exception text, using the existing
`jsonl_server.HANDLERS` monkeypatch and `handle_request` flow so the non-leak
behavior is covered end to end.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a504bf3f-e6a4-498d-96c5-d8627b8f2703
📒 Files selected for processing (2)
src/vouch/jsonl_server.pytests/test_jsonl_server.py
| except Exception as e: | ||
| _log.exception("internal error handling %s", method) | ||
| return { | ||
| "id": req_id, "ok": False, | ||
| "error": { | ||
| "code": "internal_error", | ||
| "message": str(e), | ||
| "traceback": traceback.format_exc(), | ||
| }, | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Exception message can still leak sensitive internal details.
Traceback is now correctly kept server-side, but "message": str(e) still forwards the raw exception text to the client over HTTP /rpc. Exception messages frequently embed sensitive internals (file paths, connection strings, IDs, or — as the new test itself demonstrates with RuntimeError("secret internals") — arbitrary secrets a handler author put in an error message). This only partially closes the information-disclosure gap described in the PR objectives.
Consider returning a generic, static message for the internal_error case (logging str(e) server-side alongside the traceback) so unexpected-exception responses never echo handler-controlled text to clients.
🔒 Proposed fix
except Exception as e:
- _log.exception("internal error handling %s", method)
+ _log.exception("internal error handling %s: %s", method, e)
return {
"id": req_id, "ok": False,
"error": {
"code": "internal_error",
- "message": str(e),
+ "message": "internal server error",
},
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception as e: | |
| _log.exception("internal error handling %s", method) | |
| return { | |
| "id": req_id, "ok": False, | |
| "error": { | |
| "code": "internal_error", | |
| "message": str(e), | |
| "traceback": traceback.format_exc(), | |
| }, | |
| } | |
| except Exception as e: | |
| _log.exception("internal error handling %s: %s", method, e) | |
| return { | |
| "id": req_id, "ok": False, | |
| "error": { | |
| "code": "internal_error", | |
| "message": "internal server error", | |
| }, | |
| } |
🤖 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/vouch/jsonl_server.py` around lines 765 - 773, The `except Exception as
e` handler in `jsonl_server.py` still returns handler-controlled text via
`message: str(e)`, which can leak sensitive internals to `/rpc` clients. Update
the `internal_error` response to use a generic, static message instead of the
raw exception text, and keep the detailed exception only in
`_log.exception("internal error handling %s", method)` so `req_id`/`method` stay
contextual while `str(e)` is not exposed over HTTP.
1 participant
Summary
Root cause:
handle_request()injsonl_server.pyincludedtraceback.format_exc()in the error envelope for unexpected exceptions. HTTP/rpcforwards this verbatim, so bearer-authenticated clients on public deploys receive full stack traces.Fix: Log with
logging.exception()server-side; return onlycodeandmessageto clients.Impact: Closes an information-disclosure vector without changing success paths or expected error codes.
Risk / tradeoffs
Test plan
test_jsonl_internal_error_omits_traceback(new)Summary by CodeRabbit