Skip to content

fix(sandbox): escape control characters in format_sse_error#842

Merged
johntmyers merged 1 commit intoNVIDIA:mainfrom
mjamiv:fix/format-sse-error-escaping
Apr 15, 2026
Merged

fix(sandbox): escape control characters in format_sse_error#842
johntmyers merged 1 commit intoNVIDIA:mainfrom
mjamiv:fix/format-sse-error-escaping

Conversation

@mjamiv
Copy link
Copy Markdown
Contributor

@mjamiv mjamiv commented Apr 15, 2026

Summary

Fixes format_sse_error (added in #834) so dynamic reason strings containing control characters or \n\n sequences still produce a valid, single SSE event. Replaces the manual \ / " escape with serde_json::to_writer — already a workspace dep of openshell-sandbox, so no new dependency.

Related Issue

Closes #840

Changes

  • crates/openshell-sandbox/src/l7/inference.rs::format_sse_error: replace manual escape with serde_json::to_writer. Control characters (\u0000-\u001F) and " / \ are now escaped per JSON spec; the helper can no longer be made to emit more than one SSE event boundary regardless of input.
  • Unit test format_sse_error_escapes_control_characters_in_reason — covers \n, \r, \t in reason.
  • Unit test format_sse_error_does_not_inject_extra_sse_events — proves a \n\n inside reason cannot split a single error event into two frames.

No call-site changes. The four existing in-tree callers in proxy.rs::route_inference_request pass static strings today, so this fix is behaviorally invisible to them — but it closes the latent footgun for any future caller that wants to pass a dynamic upstream error message (e.g. a reqwest error display), which the function's own docstring already invites.

Testing

  • cargo fmt --package openshell-sandbox -- --check clean
  • cargo test --package openshell-sandbox --lib passes (500 tests: 498 pre-existing + 2 new)
  • cargo test --package openshell-sandbox --lib format_sse_error shows the two new tests go red on master 355d845 and green with this patch (verified locally by applying on top of master, running both states)
  • cargo clippy --package openshell-sandbox --lib clean on the touched file (pre-existing warnings in openshell-core are unrelated and not introduced here)
  • mise run pre-commitmise not installed in this environment; ran cargo fmt --check + cargo test + cargo clippy for the affected crate by hand

Repro against master (before this patch):

$ cargo test -p openshell-sandbox --lib format_sse_error
test l7::inference::tests::format_sse_error_escapes_control_characters_in_reason ... FAILED
  panicked: control character (\u0000-\u001F) found while parsing a string
test l7::inference::tests::format_sse_error_does_not_inject_extra_sse_events ... FAILED
  assertion `left == right` failed   left: 2   right: 1

After this patch, same command: all 4 format_sse_error tests pass.

Checklist

Scope

Tightly scoped to format_sse_error and its unit tests, per AGENTS.md "Scope changes to the issue at hand." No unrelated cleanup. Pre-existing warnings in the touched file are left alone.

format_sse_error only escaped `\` and `"`, leaving two problems:

1. Control characters (`\n`, `\r`, `\t`, and all `\u0000-\u001F`) in
   `reason` produce output that fails `serde_json::from_str` — defeating
   NVIDIA#834's goal of giving clients a parseable SSE truncation signal.

2. An unescaped `\n\n` inside `reason` splits the single error event
   into two SSE frames, letting a misbehaving upstream inject a forged
   frame (e.g. a fake tool_calls delta) into the client's stream. Latent
   today since all in-tree callers pass static strings, but a footgun
   for any future caller passing upstream error text, and the function's
   docstring already invites dynamic reasons.

Replace the manual escape with `serde_json::to_writer` (already a
workspace dep of `openshell-sandbox`). Add unit tests for control
character escaping and SSE event-boundary injection.

Closes NVIDIA#840

Signed-off-by: mjamiv <michael.commack@gmail.com>
@mjamiv mjamiv requested a review from a team as a code owner April 15, 2026 05:25
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot bot commented Apr 15, 2026

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 15, 2026

All contributors have signed the DCO ✍️ ✅
Posted by the DCO Assistant Lite bot.

@johntmyers johntmyers self-assigned this Apr 15, 2026
@mjamiv
Copy link
Copy Markdown
Contributor Author

mjamiv commented Apr 15, 2026

I have read the DCO document and I hereby sign the DCO.

@johntmyers
Copy link
Copy Markdown
Collaborator

recheck

@johntmyers johntmyers merged commit 1a57519 into NVIDIA:main Apr 15, 2026
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

inference proxy: format_sse_error escapes are incomplete (control chars + SSE event injection)

2 participants