fix(server): remove internal error details from client responses

[chefsale]

Description

This PR removes sensitive error details from internal error responses sent to clients. Instead of including serialization error information in the InternalError response body, we now:

1. Log the error details server-side with appropriate context (connection ID, session ID, etc.)
2. Send a sanitized response to clients with err: None

This improves security by preventing internal implementation details from leaking to clients while maintaining observability through server-side logging.

Changes affect:

• WebSocket event handling (crates/server/src/ws.rs)
• WebSocket response serialization (crates/server/src/ws.rs)
• WebSocket execution errors (crates/server/src/ws/execute.rs)
• SSE event handling (crates/server/src/sse/events.rs)
• SSE session handler (crates/server/src/sse/handlers.rs)
• JSON-RPC error handling (crates/server/src/jsonrpc.rs)

Test plan

Existing error handling tests should continue to pass. The change is backward compatible from a response structure perspective (the err field is simply None instead of containing error details). Server-side logging can be verified by checking logs when serialization errors occur.

Wire contract (SDK gate)

• No wire DTO changes — the InternalError structure remains the same, only the err field value changes from Some(error) to None
• No route changes

https://claude.ai/code/session_015NhVDDRxUBHVMQpfG1Y7Y7

[cursor]

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

[meroreviewer]

🤖 MeroReviewer

Reviewed by 1 agents | Quality score: 85% | Review time: 42.6s

---

✅ No Issues Found

All agents reviewed the code and found no issues. LGTM! 🎉

---

_{🤖 Generated by MeroReviewer | Review ID: review-7fa4eee8}

[meroreviewer]

Documentation Review

The following documentation may need updates based on the changes in this PR:

• 🟡 architecture/crates/server.html: Files matching crates/server/** were changed but architecture/crates/server.html was not updated (per source_to_docs_mapping).

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 2 agents | Quality score: 52% | Review time: 225.2s

---

🟡 Warning (1)

1. Serialization error message leaked to client in SSE command stream

File: crates/server/src/sse/handlers.rs (line 456-462) | Consensus: 1/2 agents

In the Command::Send match arm of the SSE command stream (inside sse_handler), when to_json_string(&response) fails, the error is not logged server-side and the raw err is formatted into a JSON payload that is sent directly to the client as {"type": "error", "message": "Failed to serialize SseResponse"}. While the message string itself is hardcoded, the err value from serde_json is silently discarded with no server-side log, making this failure invisible in observability. This is inconsistent with the rest of the PR's pattern of logging errors server-side. Additionally, the error data is constructed inline as a raw JSON string sent over SSE without going through the standard ResponseBody::Error(ResponseBodyError::ServerError(ServerResponseError::InternalError { err: None })) envelope, meaning clients receive a structurally different error format here than everywhere else.

Suggested fix:

Add `error!(%err, "Failed to serialize SseResponse");` before constructing the error_data, and consider routing through the standard `ServerResponseError::InternalError { err: None }` envelope for consistency with the rest of the PR.

Found by: security-reviewer

💡 Suggestion (1)

1. Serialization errors for MethodCallError are silently collapsed into InternalError without logging the original handler error type

File: crates/server/src/jsonrpc.rs (line 211-215) | Consensus: 1/2 agents

In ToResponseBody::to_res_body for the JSON-RPC path, when serde_json::to_value(err) fails for a MethodCallError, the original err value (the handler error) is discarded — only the serde error is logged. This means the handler error type and value are permanently lost. The WS path has the same pattern. While this is an edge case (a Serialize-derived type failing serialization is unusual), when it does happen the log gives no hint of what the handler error was.

Suggested fix:

Before calling `serde_json::to_value(err)`, consider logging the debug representation of `err` (e.g. `error!(?err, "Failed to serialize handler error")`) so the original error is preserved in logs even if serialization fails.

Found by: patterns-reviewer

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-16bcb31c}

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 2 agents | Quality score: 50% | Review time: 114.9s

---

🟡 Warning (2)

1. Error details logged before the err: None change but missing here

File: crates/server/src/sse/handlers.rs (line 512-519) | Consensus: 1/2 agents

In get_session_handler, the Err(err) branch at line 512 now returns InternalError { err: None } (the PR's change), but unlike every other call-site in this PR, there is no error!(%session_id, %err, ...) log call added before the response is built. The error is silently swallowed: it won't appear in server logs, making storage failures in this path invisible to operators.

Suggested fix:

Add `error!(%session_id, %err, "Failed to load session from storage");` immediately before the `ResponseBody::Error(...)` construction, consistent with the pattern used in `sse/events.rs`, `ws.rs`, and `ws/execute.rs`.

Found by: patterns-reviewer

2. Serialization error message leaked to client in SSE command stream

File: crates/server/src/sse/handlers.rs (line 449-457) | Consensus: 1/2 agents

When to_json_string(&response) fails at line ~449 inside the Command::Send arm of the SSE command stream, the error message is embedded directly into the SSE event data as "message": "Failed to serialize SseResponse". While this is a static string (not the actual serde_json::Error), the surrounding error_data JSON object is sent verbatim to the client over the SSE stream. This is inconsistent with the rest of the PR's approach of sanitizing error responses. More importantly, the err variable (the actual serde_json::Error) is only logged via error!("Failed to serialize SseResponse: {}", err) — the format string uses {} which is fine — but the client-visible JSON still exposes internal framing details. This path was not changed by the PR and remains a minor inconsistency, but it is within the changed file scope and worth noting for consistency with the PR's stated goal.

Suggested fix:

Log the error server-side (already done) and send a sanitized `InternalError` response body serialized as a proper `SseResponse` JSON, consistent with the other error paths changed in this PR. Avoid sending ad-hoc JSON objects that bypass the standard response envelope.

Found by: security-reviewer

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-6839be40}

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 2 agents | Quality score: 50% | Review time: 212.7s

---

🟡 Warning (1)

1. Fallback error JSON does not match the SseResponse wire format

File: crates/server/src/sse/handlers.rs (line 343-356) | Consensus: 1/2 agents

When to_json_string(&error_response) itself fails (the .unwrap_or_else branch), the hardcoded fallback string {"error":{"type":"InternalError","data":{}}} is sent to the client. This shape does not match the actual SseResponse / ResponseBody wire format used everywhere else (which serialises as {"body":{"error":{...}}}). A client that parses SSE frames uniformly will fail to deserialise this frame, potentially causing a crash or silent data loss in the client. The fallback should either match the real schema or be a plain-text sentinel that the client is documented to handle.

Suggested fix:

Use a fallback that matches the real wire shape, e.g. `r#"{\"body\":{\"error\":{\"type\":\"InternalError\",\"data\":null}}}"#.to_owned()`, or document the divergence explicitly with a comment explaining why the schema differs here.

Found by: patterns-reviewer

💡 Suggestion (2)

1. Control flow relies on implicit fall-through after logging in match arms

File: crates/server/src/jsonrpc.rs (line 199-218) | Consensus: 1/2 agents

The refactored to_res_body uses error!(...) as the expression in match arms that previously returned early or produced a value. The fall-through to the ResponseBody::Error(...) at the bottom is correct but subtle: a reader must notice that none of the three non-returning arms produce a value used by the outer match, and that execution always continues past the match block to the shared error return. Adding a brief comment (e.g. // fall through to InternalError response) above the shared return would make the intent immediately clear and prevent a future refactor from accidentally breaking the invariant.

Suggested fix:

Add a comment before `ResponseBody::Error(ResponseBodyError::ServerError(...))` explaining that all three logging arms fall through here intentionally.

Found by: patterns-reviewer

2. Control flow relies on implicit fall-through after logging

File: crates/server/src/jsonrpc.rs (line 199-215) | Consensus: 1/2 agents

After the match block in to_res_body, all three non-returning arms (serialization failure, handler-error serialization failure, and InternalError) fall through to the same ResponseBody::Error(…InternalError { err: None }) return. This is correct, but the structure is subtle: the match expression is used as a statement purely for its side-effects (logging), and the actual return value is constructed below. A future maintainer adding a new arm might accidentally omit the fall-through or add an early return, silently changing behaviour. The ws.rs version uses the same pattern. Consider extracting a small log_and_internal_error helper or restructuring with explicit return statements to make the intent clearer.

Suggested fix:

fn log_and_internal_error(err: impl std::fmt::Display, ctx: &str) -> ResponseBody {
    error!(%err, "{ctx}");
    ResponseBody::Error(ResponseBodyError::ServerError(
        ServerResponseError::InternalError { err: None },
    ))
}
// Then each arm returns directly: `Err(err) => return log_and_internal_error(err, "Failed to serialize response")`

Found by: security-reviewer

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-b9158518}