fix(sandbox): Box::leak memory leak in rewrite_forward_request

[areporeporepo]

Agent Diagnostic

• Read crates/openshell-sandbox/src/proxy.rs, traced rewrite_forward_request function (line 1615)
• Found Box::leak(new_line.into_boxed_str()) at line 1634 — leaks one String per forward proxy request
• Wrote fix (replace with direct output.extend_from_slice writes), compiled with cargo check -p openshell-sandbox — clean
• Ran cargo test -p openshell-sandbox -- proxy — 84/84 tests pass including all rewrite tests

Description

rewrite_forward_request in proxy.rs:1634 uses Box::leak to extend the lifetime of a rewritten HTTP request line:

*first_line = Box::leak(new_line.into_boxed_str()); // safe: short-lived

Box::leak permanently leaks heap memory — it converts an owned allocation into a &'static str that is never freed. The comment "safe: short-lived" is incorrect; the memory is leaked for the lifetime of the process.

Every HTTP forward proxy request that hits this path leaks one String. On long-running sandboxes with many outbound HTTP requests, this causes unbounded RSS growth and eventual OOM.

Reproduction Steps

1. Start a sandbox with egress policy allowing HTTP forward proxy
2. Send N forward proxy requests (e.g. GET http://example.com/path HTTP/1.1) through the sandbox proxy
3. Observe RSS of the sandbox process growing monotonically — memory is never reclaimed

Environment

• Affects all platforms — this is a logic bug in Rust source
• crates/openshell-sandbox/src/proxy.rs line 1634 on current main

Logs

N/A — memory leak, not a crash. Observable via RSS / heap profiling.

Suggested Fix

Move the request-line rewrite into the output-building loop, writing directly to output instead of mutating the borrowed &str vec. This eliminates the need for Box::leak entirely:

-    let mut lines = header_str.split("\r\n").collect::<Vec<_>>();
-
-    // Rewrite request line: METHOD absolute-uri HTTP/1.1 → METHOD path HTTP/1.1
-    if let Some(first_line) = lines.first_mut() {
-        let parts: Vec<&str> = first_line.splitn(3, ' ').collect();
-        if parts.len() == 3 {
-            let new_line = format!("{} {} {}", parts[0], path, parts[2]);
-            *first_line = Box::leak(new_line.into_boxed_str()); // safe: short-lived
-        }
-    }
-
-    // Rebuild headers, stripping hop-by-hop and adding proxy headers
-    let mut output = Vec::with_capacity(header_end + 128);
+    let lines = header_str.split("\r\n").collect::<Vec<_>>();
+
+    // Rebuild headers, stripping hop-by-hop and adding proxy headers
+    let mut output = Vec::with_capacity(header_end + 128);
     ...
     for (i, line) in lines.iter().enumerate() {
         if i == 0 {
-            // Request line — already rewritten
-            output.extend_from_slice(line.as_bytes());
+            // Rewrite request line: METHOD absolute-uri HTTP/1.1 → METHOD path HTTP/1.1
+            let parts: Vec<&str> = line.splitn(3, ' ').collect();
+            if parts.len() == 3 {
+                output.extend_from_slice(parts[0].as_bytes());
+                output.push(b' ');
+                output.extend_from_slice(path.as_bytes());
+                output.push(b' ');
+                output.extend_from_slice(parts[2].as_bytes());
+            } else {
+                output.extend_from_slice(line.as_bytes());
+            }
             output.extend_from_slice(b"\r\n");
             continue;
         }

Verified: compiles clean, all 84 proxy unit tests pass.

[areporeporepo]

I could open a PR based on the suggested fix above, also open for suggestions

[johntmyers]

Thanks, we will take a look and open a PR after investigation.

[areporeporepo]

Sg!