Skip to content

Commit 123202b

Browse files
authored
fix(builtins): prevent JSON injection in HTTP build_json_body (#1007)
## Summary - Replaced manual string formatting in `build_json_body` with `serde_json` for proper JSON escaping - Values containing `"`, `\`, newlines, or other special characters are now safely escaped - Added threat model entry TM-NET-018 ## What & Why `build_json_body` constructed JSON via `format!("\"{}\"", v)` without escaping, allowing injection of arbitrary JSON fields (e.g., `name='test","admin":true'` would inject an `admin` field). Now uses `serde_json::Value::String` which handles all escaping correctly. ## Tests Added - `test_json_body_escapes_quotes` — verifies injection attempt is neutralized - `test_json_body_escapes_backslash_and_newline` — verifies control chars are escaped - `test_json_body_raw_field_unchanged` — verifies raw fields still work Closes #1000
1 parent 04d8aa1 commit 123202b

File tree

2 files changed

+49
-6
lines changed

2 files changed

+49
-6
lines changed

crates/bashkit/src/builtins/http.rs

Lines changed: 48 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -203,24 +203,28 @@ fn build_url_with_query(base_url: &str, items: &[ItemType]) -> String {
203203
format!("{}{}{}", base_url, sep, params.join("&"))
204204
}
205205

206-
/// Build JSON body from items.
206+
/// Build JSON body from items using serde_json for proper escaping.
207+
// THREAT[TM-NET-018]: serde_json prevents JSON injection via special characters in values
207208
fn build_json_body(items: &[ItemType]) -> String {
208-
let mut fields = Vec::new();
209+
let mut map = serde_json::Map::new();
209210
for item in items {
210211
match item {
211212
ItemType::JsonField(k, v) => {
212-
fields.push(format!(" \"{}\": \"{}\"", k, v));
213+
map.insert(k.clone(), serde_json::Value::String(v.clone()));
213214
}
214215
ItemType::JsonRawField(k, v) => {
215-
fields.push(format!(" \"{}\": {}", k, v));
216+
// Raw values are pre-formatted JSON (numbers, booleans, etc.)
217+
let raw: serde_json::Value =
218+
serde_json::from_str(v).unwrap_or(serde_json::Value::String(v.clone()));
219+
map.insert(k.clone(), raw);
216220
}
217221
_ => {}
218222
}
219223
}
220-
if fields.is_empty() {
224+
if map.is_empty() {
221225
return String::new();
222226
}
223-
format!("{{\n{}\n}}", fields.join(",\n"))
227+
serde_json::to_string_pretty(&serde_json::Value::Object(map)).unwrap_or_default()
224228
}
225229

226230
/// Build form body from items.
@@ -587,4 +591,42 @@ mod tests {
587591
))
588592
);
589593
}
594+
595+
#[test]
596+
fn test_json_body_escapes_quotes() {
597+
let items = vec![ItemType::JsonField(
598+
"name".to_string(),
599+
r#"test","admin":true,"x":"y"#.to_string(),
600+
)];
601+
let body = build_json_body(&items);
602+
// Must produce valid JSON — injected field must NOT appear as separate key
603+
let parsed: serde_json::Value = serde_json::from_str(&body).unwrap();
604+
assert_eq!(
605+
parsed["name"].as_str().unwrap(),
606+
r#"test","admin":true,"x":"y"#
607+
);
608+
assert!(parsed.get("admin").is_none());
609+
}
610+
611+
#[test]
612+
fn test_json_body_escapes_backslash_and_newline() {
613+
let items = vec![ItemType::JsonField(
614+
"msg".to_string(),
615+
"line1\nline2\\end".to_string(),
616+
)];
617+
let body = build_json_body(&items);
618+
let parsed: serde_json::Value = serde_json::from_str(&body).unwrap();
619+
assert_eq!(parsed["msg"].as_str().unwrap(), "line1\nline2\\end");
620+
}
621+
622+
#[test]
623+
fn test_json_body_raw_field_unchanged() {
624+
let items = vec![ItemType::JsonRawField(
625+
"count".to_string(),
626+
"42".to_string(),
627+
)];
628+
let body = build_json_body(&items);
629+
let parsed: serde_json::Value = serde_json::from_str(&body).unwrap();
630+
assert_eq!(parsed["count"].as_i64().unwrap(), 42);
631+
}
590632
}

specs/006-threat-model.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -605,6 +605,7 @@ allowlist.allow("https://api.example.com");
605605
| TM-NET-012 | Chunked encoding bomb | Infinite chunked response | Response size limit (streaming) | **MITIGATED** |
606606
| TM-NET-013 | Gzip bomb / Zip bomb | 10KB gzip → 10GB decompressed | Auto-decompression disabled | **MITIGATED** |
607607
| TM-NET-014 | DNS rebind via redirect | Redirect to rebinded IP | Manual redirect requires allowlist check | **MITIGATED** |
608+
| TM-NET-018 | JSON body injection | `http POST url name='x","admin":true'` via unescaped string formatting | Use `serde_json` for JSON construction | **MITIGATED** |
608609

609610
**Current Risk**: LOW - Multiple mitigations in place
610611

0 commit comments

Comments
 (0)