Skip to content

Commit f6ab995

Browse files
NathanFlurryclaude
andcommitted
feat: US-070 - Fix IPC binary frame length guards (Rust)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent ef4580b commit f6ab995

4 files changed

Lines changed: 152 additions & 45 deletions

File tree

crates/v8-runtime/src/bridge.rs

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -410,16 +410,6 @@ fn async_bridge_callback(
410410
rv.set(promise.into());
411411
}
412412

413-
/// Serialize V8 function arguments as a V8 Array via ValueSerializer.
414-
fn serialize_v8_args(scope: &mut v8::HandleScope, args: &v8::FunctionCallbackArguments) -> Result<Vec<u8>, String> {
415-
let count = args.length();
416-
let array = v8::Array::new(scope, count);
417-
for i in 0..count {
418-
array.set_index(scope, i as u32, args.get(i));
419-
}
420-
serialize_v8_value(scope, array.into())
421-
}
422-
423413
/// Serialize V8 function arguments into a pre-allocated buffer.
424414
/// The buffer is cleared and reused across calls (grows to high-water mark).
425415
fn serialize_v8_args_into(scope: &mut v8::HandleScope, args: &v8::FunctionCallbackArguments, buf: &mut Vec<u8>) -> Result<(), String> {

crates/v8-runtime/src/ipc_binary.rs

Lines changed: 118 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ pub fn encode_frame_into(buf: &mut Vec<u8>, frame: &BinaryFrame) -> io::Result<(
121121
buf.clear();
122122
// Reserve 4 bytes for the length prefix (filled after body)
123123
buf.extend_from_slice(&[0, 0, 0, 0]);
124-
encode_body(buf, frame);
124+
encode_body(buf, frame)?;
125125

126126
let total_len = buf.len() - 4;
127127
if total_len > MAX_FRAME_SIZE as usize {
@@ -192,7 +192,7 @@ pub fn extract_session_id(raw: &[u8]) -> io::Result<Option<&str>> {
192192

193193
// -- Internal encode/decode --
194194

195-
fn encode_body(buf: &mut Vec<u8>, frame: &BinaryFrame) {
195+
fn encode_body(buf: &mut Vec<u8>, frame: &BinaryFrame) -> io::Result<()> {
196196
match frame {
197197
BinaryFrame::Authenticate { token } => {
198198
buf.push(MSG_AUTHENTICATE);
@@ -206,17 +206,17 @@ fn encode_body(buf: &mut Vec<u8>, frame: &BinaryFrame) {
206206
cpu_time_limit_ms,
207207
} => {
208208
buf.push(MSG_CREATE_SESSION);
209-
write_session_id(buf, session_id);
209+
write_session_id(buf, session_id)?;
210210
buf.extend_from_slice(&heap_limit_mb.to_be_bytes());
211211
buf.extend_from_slice(&cpu_time_limit_ms.to_be_bytes());
212212
}
213213
BinaryFrame::DestroySession { session_id } => {
214214
buf.push(MSG_DESTROY_SESSION);
215-
write_session_id(buf, session_id);
215+
write_session_id(buf, session_id)?;
216216
}
217217
BinaryFrame::InjectGlobals { session_id, payload } => {
218218
buf.push(MSG_INJECT_GLOBALS);
219-
write_session_id(buf, session_id);
219+
write_session_id(buf, session_id)?;
220220
buf.extend_from_slice(payload);
221221
}
222222
BinaryFrame::Execute {
@@ -227,12 +227,10 @@ fn encode_body(buf: &mut Vec<u8>, frame: &BinaryFrame) {
227227
user_code,
228228
} => {
229229
buf.push(MSG_EXECUTE);
230-
write_session_id(buf, session_id);
230+
write_session_id(buf, session_id)?;
231231
buf.push(*mode);
232232
// file_path length (u16 BE)
233-
let fp_bytes = file_path.as_bytes();
234-
buf.extend_from_slice(&(fp_bytes.len() as u16).to_be_bytes());
235-
buf.extend_from_slice(fp_bytes);
233+
write_len_prefixed_u16(buf, file_path)?;
236234
// bridge_code length (u32 BE)
237235
let bc_bytes = bridge_code.as_bytes();
238236
buf.extend_from_slice(&(bc_bytes.len() as u32).to_be_bytes());
@@ -247,7 +245,7 @@ fn encode_body(buf: &mut Vec<u8>, frame: &BinaryFrame) {
247245
payload,
248246
} => {
249247
buf.push(MSG_BRIDGE_RESPONSE);
250-
write_session_id(buf, session_id);
248+
write_session_id(buf, session_id)?;
251249
buf.extend_from_slice(&call_id.to_be_bytes());
252250
buf.push(*status);
253251
buf.extend_from_slice(payload);
@@ -258,15 +256,13 @@ fn encode_body(buf: &mut Vec<u8>, frame: &BinaryFrame) {
258256
payload,
259257
} => {
260258
buf.push(MSG_STREAM_EVENT);
261-
write_session_id(buf, session_id);
262-
let et_bytes = event_type.as_bytes();
263-
buf.extend_from_slice(&(et_bytes.len() as u16).to_be_bytes());
264-
buf.extend_from_slice(et_bytes);
259+
write_session_id(buf, session_id)?;
260+
write_len_prefixed_u16(buf, event_type)?;
265261
buf.extend_from_slice(payload);
266262
}
267263
BinaryFrame::TerminateExecution { session_id } => {
268264
buf.push(MSG_TERMINATE_EXECUTION);
269-
write_session_id(buf, session_id);
265+
write_session_id(buf, session_id)?;
270266
}
271267
BinaryFrame::WarmSnapshot { bridge_code } => {
272268
buf.push(MSG_WARM_SNAPSHOT);
@@ -282,11 +278,9 @@ fn encode_body(buf: &mut Vec<u8>, frame: &BinaryFrame) {
282278
payload,
283279
} => {
284280
buf.push(MSG_BRIDGE_CALL);
285-
write_session_id(buf, session_id);
281+
write_session_id(buf, session_id)?;
286282
buf.extend_from_slice(&call_id.to_be_bytes());
287-
let m_bytes = method.as_bytes();
288-
buf.extend_from_slice(&(m_bytes.len() as u16).to_be_bytes());
289-
buf.extend_from_slice(m_bytes);
283+
write_len_prefixed_u16(buf, method)?;
290284
buf.extend_from_slice(payload);
291285
}
292286
BinaryFrame::ExecutionResult {
@@ -296,7 +290,7 @@ fn encode_body(buf: &mut Vec<u8>, frame: &BinaryFrame) {
296290
error,
297291
} => {
298292
buf.push(MSG_EXECUTION_RESULT);
299-
write_session_id(buf, session_id);
293+
write_session_id(buf, session_id)?;
300294
buf.extend_from_slice(&exit_code.to_be_bytes());
301295
let mut flags: u8 = 0;
302296
if exports.is_some() {
@@ -311,10 +305,10 @@ fn encode_body(buf: &mut Vec<u8>, frame: &BinaryFrame) {
311305
buf.extend_from_slice(exp);
312306
}
313307
if let Some(err) = error {
314-
write_len_prefixed_u16(buf, &err.error_type);
315-
write_len_prefixed_u16(buf, &err.message);
316-
write_len_prefixed_u16(buf, &err.stack);
317-
write_len_prefixed_u16(buf, &err.code);
308+
write_len_prefixed_u16(buf, &err.error_type)?;
309+
write_len_prefixed_u16(buf, &err.message)?;
310+
write_len_prefixed_u16(buf, &err.stack)?;
311+
write_len_prefixed_u16(buf, &err.code)?;
318312
}
319313
}
320314
BinaryFrame::Log {
@@ -323,7 +317,7 @@ fn encode_body(buf: &mut Vec<u8>, frame: &BinaryFrame) {
323317
message,
324318
} => {
325319
buf.push(MSG_LOG);
326-
write_session_id(buf, session_id);
320+
write_session_id(buf, session_id)?;
327321
buf.push(*channel);
328322
buf.extend_from_slice(message.as_bytes());
329323
}
@@ -333,13 +327,12 @@ fn encode_body(buf: &mut Vec<u8>, frame: &BinaryFrame) {
333327
payload,
334328
} => {
335329
buf.push(MSG_STREAM_CALLBACK);
336-
write_session_id(buf, session_id);
337-
let ct_bytes = callback_type.as_bytes();
338-
buf.extend_from_slice(&(ct_bytes.len() as u16).to_be_bytes());
339-
buf.extend_from_slice(ct_bytes);
330+
write_session_id(buf, session_id)?;
331+
write_len_prefixed_u16(buf, callback_type)?;
340332
buf.extend_from_slice(payload);
341333
}
342334
}
335+
Ok(())
343336
}
344337

345338
fn decode_body(buf: &[u8]) -> io::Result<BinaryFrame> {
@@ -493,16 +486,30 @@ fn decode_body(buf: &[u8]) -> io::Result<BinaryFrame> {
493486

494487
// -- Primitive read/write helpers --
495488

496-
fn write_session_id(buf: &mut Vec<u8>, sid: &str) {
489+
fn write_session_id(buf: &mut Vec<u8>, sid: &str) -> io::Result<()> {
497490
let bytes = sid.as_bytes();
491+
if bytes.len() > 255 {
492+
return Err(io::Error::new(
493+
io::ErrorKind::InvalidInput,
494+
format!("session ID byte length {} exceeds u8 max 255", bytes.len()),
495+
));
496+
}
498497
buf.push(bytes.len() as u8);
499498
buf.extend_from_slice(bytes);
499+
Ok(())
500500
}
501501

502-
fn write_len_prefixed_u16(buf: &mut Vec<u8>, s: &str) {
502+
fn write_len_prefixed_u16(buf: &mut Vec<u8>, s: &str) -> io::Result<()> {
503503
let bytes = s.as_bytes();
504+
if bytes.len() > 0xFFFF {
505+
return Err(io::Error::new(
506+
io::ErrorKind::InvalidInput,
507+
format!("string byte length {} exceeds u16 max 65535", bytes.len()),
508+
));
509+
}
504510
buf.extend_from_slice(&(bytes.len() as u16).to_be_bytes());
505511
buf.extend_from_slice(bytes);
512+
Ok(())
506513
}
507514

508515
fn read_u8(buf: &[u8], pos: &mut usize) -> io::Result<u8> {
@@ -1263,4 +1270,84 @@ mod tests {
12631270
encode_frame_into(&mut buf, &small).expect("encode");
12641271
assert_eq!(buf.capacity(), large_cap, "capacity should stay at high-water mark");
12651272
}
1273+
1274+
// -- Overflow guard tests --
1275+
1276+
#[test]
1277+
fn write_session_id_rejects_oversized() {
1278+
// Session ID > 255 bytes must be rejected
1279+
let long_sid = "x".repeat(256);
1280+
let frame = BinaryFrame::DestroySession {
1281+
session_id: long_sid,
1282+
};
1283+
let result = frame_to_bytes(&frame);
1284+
assert!(result.is_err());
1285+
let err = result.unwrap_err();
1286+
assert_eq!(err.kind(), io::ErrorKind::InvalidInput);
1287+
assert!(err.to_string().contains("session ID byte length"));
1288+
assert!(err.to_string().contains("255"));
1289+
}
1290+
1291+
#[test]
1292+
fn write_session_id_accepts_max() {
1293+
// Session ID of exactly 255 bytes must succeed
1294+
let max_sid = "a".repeat(255);
1295+
let frame = BinaryFrame::DestroySession {
1296+
session_id: max_sid.clone(),
1297+
};
1298+
let bytes = frame_to_bytes(&frame).expect("should accept 255-byte session ID");
1299+
let decoded = read_frame(&mut std::io::Cursor::new(&bytes)).expect("decode");
1300+
assert_eq!(decoded, frame);
1301+
}
1302+
1303+
#[test]
1304+
fn write_len_prefixed_u16_rejects_oversized() {
1305+
// String > 65535 bytes in a u16-prefixed field must be rejected
1306+
let long_method = "m".repeat(65536);
1307+
let frame = BinaryFrame::BridgeCall {
1308+
session_id: "s".into(),
1309+
call_id: 1,
1310+
method: long_method,
1311+
payload: vec![],
1312+
};
1313+
let result = frame_to_bytes(&frame);
1314+
assert!(result.is_err());
1315+
let err = result.unwrap_err();
1316+
assert_eq!(err.kind(), io::ErrorKind::InvalidInput);
1317+
assert!(err.to_string().contains("string byte length"));
1318+
assert!(err.to_string().contains("65535"));
1319+
}
1320+
1321+
#[test]
1322+
fn write_len_prefixed_u16_accepts_max() {
1323+
// String of exactly 65535 bytes in a u16-prefixed field must succeed
1324+
let max_method = "m".repeat(65535);
1325+
let frame = BinaryFrame::BridgeCall {
1326+
session_id: "s".into(),
1327+
call_id: 1,
1328+
method: max_method.clone(),
1329+
payload: vec![],
1330+
};
1331+
let bytes = frame_to_bytes(&frame).expect("should accept 65535-byte method");
1332+
let decoded = read_frame(&mut std::io::Cursor::new(&bytes)).expect("decode");
1333+
assert_eq!(decoded, frame);
1334+
}
1335+
1336+
#[test]
1337+
fn execute_file_path_rejects_oversized() {
1338+
// file_path > 65535 bytes must be rejected (encoded as u16)
1339+
let long_path = "/".repeat(65536);
1340+
let frame = BinaryFrame::Execute {
1341+
session_id: "s".into(),
1342+
mode: 0,
1343+
file_path: long_path,
1344+
bridge_code: "".into(),
1345+
user_code: "".into(),
1346+
};
1347+
let result = frame_to_bytes(&frame);
1348+
assert!(result.is_err());
1349+
let err = result.unwrap_err();
1350+
assert_eq!(err.kind(), io::ErrorKind::InvalidInput);
1351+
assert!(err.to_string().contains("65535"));
1352+
}
12661353
}

progress.txt

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2755,3 +2755,33 @@ PRD: ralph/kernel-hardening (46 stories)
27552755
- packages/secure-exec-core/dist/ is gitignored — don't try to stage files from there
27562756
- The generated isolate-runtime.ts is auto-regenerated by `pnpm run --filter @secure-exec/core build` (specifically build:isolate-runtime)
27572757
---
2758+
2759+
## 2026-03-19 - US-069
2760+
- What was implemented: Fixed 4 correctness bugs in TypeScript IPC binary codec and runtime hash function
2761+
- readLenPrefixedU16: returns { value, bytesRead } struct; pos advances by wire length (u16 value), not Buffer.byteLength(decodedString)
2762+
- encodeSessionId: throws if session ID UTF-8 byte length exceeds 255 (was silently truncating via byte assignment)
2763+
- writeLenPrefixedU16: throws if string UTF-8 byte length exceeds 65535 (explicit guard before writeUInt16BE)
2764+
- fnv1aHash: hashes over Buffer.from(str, 'utf8') bytes instead of str.charCodeAt(i) UTF-16 code units
2765+
- Exported fnv1aHash from runtime.ts and index.ts for testability
2766+
- Added 14 new tests: overflow guards (6), multi-byte UTF-8 round-trips (3), fnv1aHash (4 including UTF-8 vs UTF-16 divergence proof), boundary tests (1)
2767+
- Files changed: packages/secure-exec-v8/src/ipc-binary.ts, packages/secure-exec-v8/src/runtime.ts, packages/secure-exec-v8/src/index.ts, packages/secure-exec-v8/test/ipc-binary.test.ts
2768+
- **Learnings for future iterations:**
2769+
- readLenPrefixedU16 wire length vs re-encoded length diverges on malformed UTF-8 (replacement char U+FFFD re-encodes to 3 bytes per invalid sequence)
2770+
- Node.js writeUInt16BE already throws RangeError for >0xFFFF but error message is opaque — explicit guard gives clearer diagnostics
2771+
- FNV-1a over charCodeAt gives different results from FNV-1a over UTF-8 bytes for any non-ASCII string — always hash bytes for cross-language compatibility
2772+
---
2773+
2774+
## 2026-03-19 - US-070
2775+
- Fixed overflow guards in Rust IPC binary encoder to prevent silent truncation
2776+
- `write_session_id` now returns `io::Result<()>` and rejects session IDs > 255 bytes
2777+
- `write_len_prefixed_u16` now returns `io::Result<()>` and rejects strings > 65535 bytes
2778+
- `encode_body` changed to return `io::Result<()>` to propagate errors from helpers
2779+
- Execute frame's `file_path` now uses `write_len_prefixed_u16` instead of inline `as u16` cast
2780+
- All inline `as u16` casts for StreamEvent, BridgeCall, StreamCallback replaced with `write_len_prefixed_u16`
2781+
- Removed dead `serialize_v8_args` function from bridge.rs (replaced by `serialize_v8_args_into`)
2782+
- Added tests: write_session_id rejects >255 byte sid, accepts 255; write_len_prefixed_u16 rejects >65535, accepts 65535; Execute file_path rejects >65535
2783+
- Files changed: crates/v8-runtime/src/ipc_binary.rs, crates/v8-runtime/src/bridge.rs
2784+
- **Learnings for future iterations:**
2785+
- encode_body was infallible (returned nothing) so callers like encode_frame_into had to handle errors only from the size check — now encode_body itself can fail on field overflow
2786+
- All u16/u8 truncation points in encode_body now use the guarded helper functions
2787+
---

scripts/ralph/prd.json

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1005,8 +1005,8 @@
10051005
"Tests pass"
10061006
],
10071007
"priority": 59,
1008-
"passes": false,
1009-
"notes": "PR review finding: Critical correctness bugs. readLenPrefixedU16 diverges on malformed UTF-8. fnv1aHash mismatch can cause stale bridge code on Rust side."
1008+
"passes": true,
1009+
"notes": "Fixed: readLenPrefixedU16 returns bytesRead from wire length instead of re-encoded string length; encodeSessionId throws on >255 bytes; writeLenPrefixedU16 throws on >65535 bytes; fnv1aHash hashes over UTF-8 bytes via Buffer.from()."
10101010
},
10111011
{
10121012
"id": "US-070",
@@ -1023,8 +1023,8 @@
10231023
"Typecheck passes"
10241024
],
10251025
"priority": 60,
1026-
"passes": false,
1027-
"notes": "PR review finding: Silent truncation via 'as u8'/'as u16' casts corrupts frames."
1026+
"passes": true,
1027+
"notes": "Fixed: write_session_id returns error on >255 bytes; write_len_prefixed_u16 returns error on >65535 bytes; encode_body propagates errors; Execute file_path uses write_len_prefixed_u16; removed dead serialize_v8_args."
10281028
},
10291029
{
10301030
"id": "US-071",

0 commit comments

Comments
 (0)