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
ericksoa pushed a commit to NVIDIA/NemoClaw that referenced this pull request Apr 23, 2026
## Summary
Bumps the pinned OpenShell version range from `0.0.29` → `0.0.32` so
fresh NemoClaw installs pick up sandbox hardening and TLS improvements
from the last three OpenShell releases.

## Notable upstream changes

**0.0.30**
([NVIDIA/OpenShell@v0.0.29...v0.0.30](NVIDIA/OpenShell@v0.0.29...v0.0.30))
- Network policy deny rules
([OpenShell#822](NVIDIA/OpenShell#822))
- Preserve ownership on existing `read_write` paths
([OpenShell#827](NVIDIA/OpenShell#827))
- Disable child core dumps
([OpenShell#821](NVIDIA/OpenShell#821))
- Escape control characters in SSE error formatting
([OpenShell#842](NVIDIA/OpenShell#842))
- Fix silent truncation of large streaming inference responses
([OpenShell#834](NVIDIA/OpenShell#834))

**0.0.31**
([NVIDIA/OpenShell@v0.0.30...v0.0.31](NVIDIA/OpenShell@v0.0.30...v0.0.31))
- Inference routed-request header allowlist
([OpenShell#826](NVIDIA/OpenShell#826))

**0.0.32**
([NVIDIA/OpenShell@v0.0.31...v0.0.32](NVIDIA/OpenShell@v0.0.31...v0.0.32))
- **Load system CA certificates for upstream TLS connections**
([OpenShell#862](NVIDIA/OpenShell#862))
- Publish standalone `openshell-gateway` binaries
([OpenShell#853](NVIDIA/OpenShell#853))

## Changes
- `nemoclaw-blueprint/blueprint.yaml`: `min_openshell_version` and
`max_openshell_version` → `0.0.32`
- `scripts/install-openshell.sh`: `MIN_VERSION` and `MAX_VERSION` →
`0.0.32` (`PIN_VERSION` follows `MAX`)
- `scripts/brev-launchable-ci-cpu.sh`: default `OPENSHELL_VERSION` →
`v0.0.32`
- `src/lib/onboard.ts`: blueprint-fallback min version → `0.0.32`
- `test/onboard.test.ts`,
`test/install-openshell-version-check.test.ts`: fixtures updated; "above
MAX" test case moved from `0.0.30` to `0.0.33`

Historical `m-dev` comments referencing `0.0.29` left in place — they
describe a self-report quirk the sidecar fallback still handles.

## Why not 0.0.33+?
`0.0.34` introduced incremental sandbox policy updates and L7
request-target canonicalization — changes with larger surface area
against how NemoClaw delivers policy via gRPC. Worth a follow-up PR
rather than bundling here. `0.0.35` released hours before this PR was
cut — too fresh.

## Type of Change
- [x] Code change for a new feature, bug fix, or refactor.

## Testing
- [x] `npx vitest run test/install-openshell-version-check.test.ts` — 9
passed
- [x] pre-commit hooks (prek) clean: shellcheck, commitlint, gitleaks,
YAML validator, CLI test suite
- [ ] Nightly E2E on this branch — will be kicked off after PR opens

## Notes
- No user-facing CLI behavior changes — just the pinned version range.
- Two pre-existing failures in `test/onboard.test.ts` reproduce on clean
`main` and are unrelated to this bump.

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>

🤖 Generated with [Claude Code](https://claude.com/claude-code)

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Chores**
* Updated OpenShell version constraints and default pinned version to
v0.0.32 across configuration, install, and onboarding flows.

* **Tests**
* Updated test fixtures and expectations to match the new OpenShell
version (v0.0.32).
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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