fix(http): sanitize HTTP error responses to avoid leaking internals

[peilii]

Overview

Sanitizes error responses across the HTTP service so internal details (file
paths, library versions, panic/stack text, raw backend exception messages,
context IDs) are no longer returned to clients. Addresses CWE-209 (Generation
of Error Message Containing Sensitive Information).

The pattern throughout: log the full detail server-side (tracing), and
return a static, sanitized message to the client. Client-facing 4xx
messages (validation, model-not-found, etc.) are intentionally preserved.

Surfaces closed

• Non-streaming OpenAI — from_anyhow fallback, ResourceExhausted (503),
  from_http_error (5xx / out-of-range), fold/parse handlers, JoinError sites,
  MJPEG builder, and check_for_backend_error for non-4xx events.
• Streaming SSE — monitor_for_disconnects emitted the raw backend error
  in its error frame, leaking across every streaming endpoint (OpenAI
  chat/completions/videos and Anthropic messages). Single shared sink, fixed
  once.
• Anthropic 500s — await-task JoinError, generate failure, and
  fold-stream failure handlers.
• Cancellation (HTTP 499) — from_anyhow and the Anthropic generate path
  echoed raw backend cancellation text (context IDs, "stopped or killed"); now
  return a static "Request cancelled".
• Realtime WebSocket — the outbound engine-error frame and the
  engine-lookup failure handler echoed err.to_string() into server_error
  frames.

Tests

• Reworked the streaming fault-contract tests (disconnect.rs) — they
  previously asserted the raw backend message reached the client (i.e. pinned
  the leak). They now assert the sanitized static message and that the
  backend detail is absent.
• Added test_mid_stream_error_does_not_leak_internal_details driving a
  path/panic/ValueError payload through the stream and asserting none of it
  reaches the client.
• Updated test_cancelled_error_response_from_anyhow to assert the static
  message and absence of the context ID / cancellation text.
• Existing non-streaming sanitization tests retained.

Verification

• cargo clippy -p dynamo-llm --lib -- -D warnings — clean
• cargo test -p dynamo-llm --lib http::service — 99 passed, 0 failed

🤖 Generated with Claude Code

---

Summary by CodeRabbit

Release Notes

• Bug Fixes
  • Improved error message consistency across API endpoints and WebSocket connections by standardizing internal error responses with generic messages instead of exposing backend implementation details.
  • Enhanced error logging to capture full diagnostic information server-side while keeping client-facing messages concise and uniform.

[coderabbitai]

Walkthrough

This PR sanitizes error handling across HTTP service handlers to prevent internal error details from leaking to clients. A new OpenAI helper logs full backend errors server-side while returning static public messages. All service handlers (OpenAI, Anthropic, Realtime, Disconnect) are updated to apply consistent sanitization patterns at spawn failures, generation errors, stream folding, and mid-stream backend events. Tests verify no sensitive details appear in client responses.

Changes

Error Sanitization Across HTTP Service Handlers

Layer / File(s)	Summary
OpenAI error sanitization infrastructure lib/llm/src/http/service/openai.rs	Introduces ErrorMessage::internal_server_error_with_details helper that logs server-side details while returning static public messages. Refactors from_anyhow to apply sanitization for admission-control rejections, cancellations, and non-HttpError chains. Updates from_http_error to sanitize out-of-range status codes.
OpenAI HTTP handler integration lib/llm/src/http/service/openai.rs	Updates error handling at completions, chat-completions, and responses handlers: tokio::spawn join failures log exceptions and return generic errors; stream folding failures use static messages; check_for_backend_error validates only documented 4xx codes are forwarded, sanitizing all others; MJPEG builder errors are sanitized.
OpenAI test coverage for sanitization lib/llm/src/http/service/openai.rs	Tests verify sanitization for out-of-range HttpError codes, resource exhaustion, cancellation, backend error events, JSON payloads with 500 and non-client codes, and backend comment error signals. Client messages are asserted to not contain leaked backend strings.
Anthropic error sanitization lib/llm/src/http/service/anthropic.rs	Spawn task join failures return generic internal error; generation errors distinguish cancellation (499 request_cancelled with debug log) from other failures (500 api_error with error log and static message); stream folding failures use static message.
Disconnect SSE mid-stream error sanitization lib/llm/src/http/service/disconnect.rs	Streaming errors are logged server-side and structured error JSON is constructed with static "Internal server error" message instead of echoing runtime error text, followed by data: [DONE].
Disconnect test infrastructure and leak detection lib/llm/src/http/service/disconnect.rs	Updates assertion helpers to verify static error message is emitted before [DONE] and that raw backend details do not appear in SSE body. Modifies worker-kill and Python consumer drop tests, adds new test for sensitive internal text fragments (paths, panic, exception) to confirm no leakage.
Realtime WebSocket error sanitization lib/llm/src/http/service/realtime.rs	Engine-produced annotated errors are logged server-side and mapped to static "Internal server error" message. Model manager lookup errors similarly use static message instead of formatting underlying error.

---

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: sanitizing HTTP error responses to prevent leaking internal details, which is the primary objective of all file changes.
Description check	✅ Passed	The description follows the template structure with Overview, Details (Surfaces closed), and Tests sections. It comprehensively explains the changes, affected components, and verification steps.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

lib/llm/src/http/service/openai.rs (2)

348-370: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don't forward backend 499 bodies unchanged.

from_anyhow() now sanitizes cancelled requests, but this helper still echoes HttpError { code: 499, message } verbatim. If an adapter/backend encodes cancellation that way, context IDs or backend cancellation text will leak again.

💡 Suggested fix

 pub fn from_http_error(err: HttpError) -> ErrorResponse {
+    if err.code == 499 {
+        let code = StatusCode::from_u16(499).unwrap();
+        tracing::debug!("Request cancelled before response: {}", err.message);
+        return (
+            code,
+            Json(ErrorMessage {
+                message: "Request cancelled".to_string(),
+                error_type: map_error_code_to_error_type(code),
+                code: code.as_u16(),
+            }),
+        );
+    }
+
     // Backend-supplied messages are only forwarded for the documented 4xx
     // range; for 5xx or codes outside the HTTP space the message may
     // contain internal paths/details and is kept server-side only.
     if err.code < 400 || err.code >= 500 {

Please mirror the same carve-out in check_for_backend_error() and add a regression test for a backend HttpError/error event with code 499.

🤖 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 `@lib/llm/src/http/service/openai.rs` around lines 348 - 370, The helper
from_http_error currently forwards HttpError with code 499 verbatim; change it
to treat 499 like other non-forwarded codes by not echoing backend message
(i.e., return ErrorMessage::internal_server_error_with_details or otherwise
sanitize the message) so cancellation details don’t leak; apply the same
carve-out in check_for_backend_error() so it also treats 499 as
non-forwarded/sanitized; add a regression test that constructs an HttpError (or
backend error event) with code 499 and asserts the response does not include the
original backend message or context IDs.

---

1182-1224: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Keep scanning past leading annotations before declaring the stream clean.

chat_completions() and responses() prepend annotation frames before calling this helper. When the first item is an annotation like request_id, this function reconstructs the stream immediately and never sees an immediate backend event:error right after it. On non-streaming requests that turns intended backend 4xxs into the later generic fold/parse 500 path.

💡 Suggested fix

-    // Peek at the first event
-    if let Some(first_event) = stream.next().await {
-        // Check if it's an error event
-        if let Some((error_msg, status_code)) = extract_backend_error_if_present(&first_event) {
+    let mut buffered = Vec::new();
+    while let Some(event) = stream.next().await {
+        if let Some((error_msg, status_code)) = extract_backend_error_if_present(&event) {
             // existing error handling...
         }
 
-        // Not an error - reconstruct stream with first event
-        let reconstructed_stream = futures::stream::iter(vec![first_event]).chain(stream);
-        Ok(reconstructed_stream)
-    } else {
-        // Empty stream - this shouldn't happen but handle gracefully
-        Ok(futures::stream::iter(vec![]).chain(stream))
+        let is_annotation = event.data.is_none()
+            && event.comment.is_none()
+            && event.event.as_deref() != Some("error");
+
+        buffered.push(event);
+        if !is_annotation {
+            break;
+        }
     }
+
+    Ok(futures::stream::iter(buffered).chain(stream))

A regression test with ANNOTATION_REQUEST_ID followed by a backend 4xx error frame would lock this down.

🤖 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 `@lib/llm/src/http/service/openai.rs` around lines 1182 - 1224,
check_for_backend_error currently only inspects the very first stream item and
will miss an immediate backend error if the stream begins with annotation frames
(e.g., ANNOTATION_REQUEST_ID); change it to consume and buffer any leading
annotation frames (using whatever predicate identifies annotations in Annotated
items), then inspect the first non-annotation event with
extract_backend_error_if_present; if it’s an error return the appropriate
ErrorResponse, otherwise reconstruct the stream by chaining the buffered
annotation frames back in front of the remaining stream and return that
reconstructed stream (update logic around extract_backend_error_if_present, the
reconstructed_stream creation, and preserve behavior for empty streams).

🤖 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.

Outside diff comments:
In `@lib/llm/src/http/service/openai.rs`:
- Around line 348-370: The helper from_http_error currently forwards HttpError
with code 499 verbatim; change it to treat 499 like other non-forwarded codes by
not echoing backend message (i.e., return
ErrorMessage::internal_server_error_with_details or otherwise sanitize the
message) so cancellation details don’t leak; apply the same carve-out in
check_for_backend_error() so it also treats 499 as non-forwarded/sanitized; add
a regression test that constructs an HttpError (or backend error event) with
code 499 and asserts the response does not include the original backend message
or context IDs.
- Around line 1182-1224: check_for_backend_error currently only inspects the
very first stream item and will miss an immediate backend error if the stream
begins with annotation frames (e.g., ANNOTATION_REQUEST_ID); change it to
consume and buffer any leading annotation frames (using whatever predicate
identifies annotations in Annotated items), then inspect the first
non-annotation event with extract_backend_error_if_present; if it’s an error
return the appropriate ErrorResponse, otherwise reconstruct the stream by
chaining the buffered annotation frames back in front of the remaining stream
and return that reconstructed stream (update logic around
extract_backend_error_if_present, the reconstructed_stream creation, and
preserve behavior for empty streams).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 625a2610-4766-4c18-b187-57c35b4aedf5

📥 Commits

Reviewing files that changed from the base of the PR and between 7ca60df and 5887e83.

📒 Files selected for processing (4)

• lib/llm/src/http/service/anthropic.rs
• lib/llm/src/http/service/disconnect.rs
• lib/llm/src/http/service/openai.rs
• lib/llm/src/http/service/realtime.rs

[nnshah1]

generally looks good - I think we can potentially refactor to simplify and also streamline comments - lets review the dynamo ops comments as well.

[peilii]

Closing both coderabbit outside-diff items:

• from_http_error previously forwarded HttpError { code: 499, message: ... } verbatim because 499 sits in the 4xx range. Added a 499 carve-out routed through SanitizedError::Cancelled + test test_from_http_error_sanitizes_499_message.
• check_for_backend_error only inspected the first stream item, so the request_id annotation prepended by chat_completions/responses shadowed any immediate backend error event. Now buffers leading annotation frames (predicate: no data, no error, event tag is some non-"error" value) via the new is_annotation_frame helper, inspects the first non-annotation event, and replays the buffer in original order if no error is found. Tests: test_check_for_backend_error_skips_leading_annotation_frames, test_check_for_backend_error_replays_leading_annotation_frames.

[peilii]

Self-review (effort=high, 7 finder agents over the http::service diff) surfaced four contained issues fixed in f0d3139:

• SanitizedError::PreserveServerError(503).anthropic_type() returned "api_error" instead of the spec-correct "overloaded_error" (same for 529). Anthropic SDKs branching on error.type for overload backoff would have misclassified backend 503s. anthropic_type() and openai_type_slug() now consult the inner status. Unit tests added in error.rs (preserve_server_error_503_maps_to_overload_types, preserve_server_error_529_maps_to_overload_types, preserve_server_error_500_remains_generic).
• check_for_backend_error buffered leading annotation frames in an unbounded Vec — a backend that emits only annotations could pin per-request memory. Capped at MAX_LEADING_ANNOTATIONS = 16.
• PreserveServerError(StatusCode) had no enforcement of its 5xx invariant. Added a debug_assert in status() and documented that the variant should be constructed via SanitizedError::for_backend_status.
• The four-way status-classification ladder (499 / 4xx / 5xx / else) was duplicated in check_for_backend_error and the anthropic.rs rewrap. Both now go through the new SanitizedError::for_backend_status(StatusCode) -> Option<Self> helper — one source of truth.

Deferred (intentional, not regressions): the InvalidArgument 4xx branch still forwards backend messages (protocol contract — same trade-off the PR already makes for 400s); the three 500-helper variants (internal_server_error / _with_details / sanitized_with_details) coexist for different roles; is_annotation_frame would ideally live on Annotated<T> but that's a cross-crate move and worth a separate PR.

115 tests pass, clippy + fmt clean.