mirror of
https://github.com/openai/codex.git
synced 2026-11-03 08:22:30 +00:00
Escape turn metadata headers as ASCII JSON (#19620)
## Why `x-codex-turn-metadata` is sent as an HTTP/WebSocket header, but Codex was serializing the metadata JSON with raw UTF-8 string contents. When a workspace path contains non-ASCII characters, common HTTP stacks can reject or corrupt that header before the request reaches the provider. Fixes #17468. Also addresses the duplicate WebSocket report in #19581. ## What changed - Added `codex_utils_string::to_ascii_json_string`, a shared helper that serializes JSON normally while escaping non-ASCII string content as `\uXXXX`. - Switched turn metadata header serialization, including merged Responses API client metadata, to use the ASCII-safe JSON helper. - Added coverage for non-ASCII workspace paths and non-ASCII client metadata while preserving the same parsed JSON values. ## Verification - `cargo test -p codex-utils-string` - `cargo test -p codex-core turn_metadata` - `just bazel-lock-check`
This commit is contained in:
1 parent
b1546008fc
commit
4241df4d79
6 files changed
+146
-3
No files matched your search
Generated
+2
@@ -3856,6 +3856,8 @@ version = "0.0.0"
|
|||||||
dependencies = [
|
dependencies = [
|
||||||
"pretty_assertions",
|
"pretty_assertions",
|
||||||
"regex-lite",
|
"regex-lite",
|
||||||
|
"serde",
|
||||||
|
"serde_json",
|
||||||
]
|
]
|
||||||
|
|
||||||
[[package]]
|
[[package]]
|
||||||
|
|||||||
@@ -4,6 +4,7 @@ use std::sync::Arc;
|
|||||||
use std::sync::Mutex;
|
use std::sync::Mutex;
|
||||||
use std::sync::RwLock;
|
use std::sync::RwLock;
|
||||||
|
|
||||||
|
use codex_utils_string::to_ascii_json_string;
|
||||||
use serde::Serialize;
|
use serde::Serialize;
|
||||||
use serde_json::Value;
|
use serde_json::Value;
|
||||||
use tokio::task::JoinHandle;
|
use tokio::task::JoinHandle;
|
||||||
@@ -71,7 +72,7 @@ pub(crate) struct TurnMetadataBag {
|
|||||||
|
|
||||||
impl TurnMetadataBag {
|
impl TurnMetadataBag {
|
||||||
fn to_header_value(&self) -> Option<String> {
|
fn to_header_value(&self) -> Option<String> {
|
||||||
serde_json::to_string(self).ok()
|
to_ascii_json_string(self).ok()
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -101,7 +102,7 @@ fn merge_turn_metadata(
|
|||||||
.or_insert_with(|| Value::String(value.clone()));
|
.or_insert_with(|| Value::String(value.clone()));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
serde_json::to_string(&metadata).ok()
|
to_ascii_json_string(&metadata).ok()
|
||||||
}
|
}
|
||||||
|
|
||||||
fn build_turn_metadata_bag(
|
fn build_turn_metadata_bag(
|
||||||
|
|||||||
@@ -7,6 +7,7 @@ use codex_protocol::protocol::SessionSource;
|
|||||||
use codex_protocol::protocol::SubAgentSource;
|
use codex_protocol::protocol::SubAgentSource;
|
||||||
use core_test_support::PathBufExt;
|
use core_test_support::PathBufExt;
|
||||||
use core_test_support::PathExt;
|
use core_test_support::PathExt;
|
||||||
|
use pretty_assertions::assert_eq;
|
||||||
use serde_json::Value;
|
use serde_json::Value;
|
||||||
use std::collections::HashMap;
|
use std::collections::HashMap;
|
||||||
use tempfile::TempDir;
|
use tempfile::TempDir;
|
||||||
@@ -15,7 +16,7 @@ use tokio::process::Command;
|
|||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn build_turn_metadata_header_includes_has_changes_for_clean_repo() {
|
async fn build_turn_metadata_header_includes_has_changes_for_clean_repo() {
|
||||||
let temp_dir = TempDir::new().expect("temp dir");
|
let temp_dir = TempDir::new().expect("temp dir");
|
||||||
let repo_path = temp_dir.path().join("repo").abs();
|
let repo_path = temp_dir.path().join("repo-東京").abs();
|
||||||
std::fs::create_dir_all(&repo_path).expect("create repo");
|
std::fs::create_dir_all(&repo_path).expect("create repo");
|
||||||
|
|
||||||
Command::new("git")
|
Command::new("git")
|
||||||
@@ -54,7 +55,16 @@ async fn build_turn_metadata_header_includes_has_changes_for_clean_repo() {
|
|||||||
let header = build_turn_metadata_header(&repo_path, Some("none"))
|
let header = build_turn_metadata_header(&repo_path, Some("none"))
|
||||||
.await
|
.await
|
||||||
.expect("header");
|
.expect("header");
|
||||||
|
assert!(header.is_ascii());
|
||||||
|
assert!(!header.contains("東京"));
|
||||||
let parsed: Value = serde_json::from_str(&header).expect("valid json");
|
let parsed: Value = serde_json::from_str(&header).expect("valid json");
|
||||||
|
let expected_repo_path = repo_path.to_string_lossy().into_owned();
|
||||||
|
let actual_repo_path = parsed
|
||||||
|
.get("workspaces")
|
||||||
|
.and_then(Value::as_object)
|
||||||
|
.and_then(|workspaces| workspaces.keys().next())
|
||||||
|
.expect("workspace path");
|
||||||
|
assert_eq!(actual_repo_path, &expected_repo_path);
|
||||||
let workspace = parsed
|
let workspace = parsed
|
||||||
.get("workspaces")
|
.get("workspaces")
|
||||||
.and_then(Value::as_object)
|
.and_then(Value::as_object)
|
||||||
@@ -191,6 +201,7 @@ fn turn_metadata_state_merges_client_metadata_without_replacing_reserved_fields(
|
|||||||
);
|
);
|
||||||
state.set_responsesapi_client_metadata(HashMap::from([
|
state.set_responsesapi_client_metadata(HashMap::from([
|
||||||
("fiber_run_id".to_string(), "fiber-123".to_string()),
|
("fiber_run_id".to_string(), "fiber-123".to_string()),
|
||||||
|
("origin".to_string(), "東京".to_string()),
|
||||||
("session_id".to_string(), "client-supplied".to_string()),
|
("session_id".to_string(), "client-supplied".to_string()),
|
||||||
("thread_source".to_string(), "client-supplied".to_string()),
|
("thread_source".to_string(), "client-supplied".to_string()),
|
||||||
(
|
(
|
||||||
@@ -201,9 +212,12 @@ fn turn_metadata_state_merges_client_metadata_without_replacing_reserved_fields(
|
|||||||
state.set_turn_started_at_unix_ms(/*turn_started_at_unix_ms*/ 1_700_000_000_123);
|
state.set_turn_started_at_unix_ms(/*turn_started_at_unix_ms*/ 1_700_000_000_123);
|
||||||
|
|
||||||
let header = state.current_header_value().expect("header");
|
let header = state.current_header_value().expect("header");
|
||||||
|
assert!(header.is_ascii());
|
||||||
|
assert!(!header.contains("東京"));
|
||||||
let json: Value = serde_json::from_str(&header).expect("json");
|
let json: Value = serde_json::from_str(&header).expect("json");
|
||||||
|
|
||||||
assert_eq!(json["fiber_run_id"].as_str(), Some("fiber-123"));
|
assert_eq!(json["fiber_run_id"].as_str(), Some("fiber-123"));
|
||||||
|
assert_eq!(json["origin"].as_str(), Some("東京"));
|
||||||
assert_eq!(json["session_id"].as_str(), Some("session-a"));
|
assert_eq!(json["session_id"].as_str(), Some("session-a"));
|
||||||
assert_eq!(json["thread_source"].as_str(), Some("user"));
|
assert_eq!(json["thread_source"].as_str(), Some("user"));
|
||||||
assert_eq!(json["turn_id"].as_str(), Some("turn-a"));
|
assert_eq!(json["turn_id"].as_str(), Some("turn-a"));
|
||||||
|
|||||||
@@ -9,6 +9,8 @@ workspace = true
|
|||||||
|
|
||||||
[dependencies]
|
[dependencies]
|
||||||
regex-lite = { workspace = true }
|
regex-lite = { workspace = true }
|
||||||
|
serde = { workspace = true }
|
||||||
|
serde_json = { workspace = true }
|
||||||
|
|
||||||
[dev-dependencies]
|
[dev-dependencies]
|
||||||
pretty_assertions = { workspace = true }
|
pretty_assertions = { workspace = true }
|
||||||
@@ -0,0 +1,122 @@
|
|||||||
|
//! JSON serialization helpers for output that must remain parseable as JSON
|
||||||
|
//! while staying safe for ASCII-only transports.
|
||||||
|
|
||||||
|
use std::io;
|
||||||
|
|
||||||
|
use serde::Serialize;
|
||||||
|
|
||||||
|
struct AsciiJsonFormatter;
|
||||||
|
|
||||||
|
impl serde_json::ser::Formatter for AsciiJsonFormatter {
|
||||||
|
// serde_json has no ensure_ascii flag; this formatter keeps its serializer
|
||||||
|
// in charge and only escapes non-ASCII string fragments.
|
||||||
|
fn write_string_fragment<W>(&mut self, writer: &mut W, fragment: &str) -> io::Result<()>
|
||||||
|
where
|
||||||
|
W: ?Sized + io::Write,
|
||||||
|
{
|
||||||
|
let mut start = 0;
|
||||||
|
for (index, ch) in fragment.char_indices() {
|
||||||
|
if ch.is_ascii() {
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
|
if start < index {
|
||||||
|
writer.write_all(&fragment.as_bytes()[start..index])?;
|
||||||
|
}
|
||||||
|
|
||||||
|
let mut utf16 = [0; 2];
|
||||||
|
for code_unit in ch.encode_utf16(&mut utf16) {
|
||||||
|
write!(writer, "\\u{code_unit:04x}")?;
|
||||||
|
}
|
||||||
|
start = index + ch.len_utf8();
|
||||||
|
}
|
||||||
|
|
||||||
|
if start < fragment.len() {
|
||||||
|
writer.write_all(&fragment.as_bytes()[start..])?;
|
||||||
|
}
|
||||||
|
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Serialize JSON while escaping non-ASCII string content as `\uXXXX`.
|
||||||
|
///
|
||||||
|
/// This is useful when JSON needs to remain parseable as JSON but must be
|
||||||
|
/// carried through ASCII-safe transports such as HTTP headers.
|
||||||
|
pub fn to_ascii_json_string<T>(value: &T) -> serde_json::Result<String>
|
||||||
|
where
|
||||||
|
T: Serialize + ?Sized,
|
||||||
|
{
|
||||||
|
let mut bytes = Vec::new();
|
||||||
|
let mut serializer = serde_json::Serializer::with_formatter(&mut bytes, AsciiJsonFormatter);
|
||||||
|
value.serialize(&mut serializer)?;
|
||||||
|
String::from_utf8(bytes)
|
||||||
|
.map_err(|err| serde_json::Error::io(io::Error::new(io::ErrorKind::InvalidData, err)))
|
||||||
|
}
|
||||||
|
|
||||||
|
#[cfg(test)]
|
||||||
|
mod tests {
|
||||||
|
use std::collections::BTreeMap;
|
||||||
|
|
||||||
|
use pretty_assertions::assert_eq;
|
||||||
|
use serde::Serialize;
|
||||||
|
use serde::ser::SerializeStruct;
|
||||||
|
use serde_json::Value;
|
||||||
|
use serde_json::json;
|
||||||
|
|
||||||
|
use super::to_ascii_json_string;
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn to_ascii_json_string_escapes_non_ascii_strings() {
|
||||||
|
struct TestPayload;
|
||||||
|
|
||||||
|
impl Serialize for TestPayload {
|
||||||
|
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
|
||||||
|
where
|
||||||
|
S: serde::Serializer,
|
||||||
|
{
|
||||||
|
let workspaces = BTreeMap::from([("/tmp/東京", TestWorkspace)]);
|
||||||
|
let mut state = serializer.serialize_struct("TestPayload", 1)?;
|
||||||
|
state.serialize_field("workspaces", &workspaces)?;
|
||||||
|
state.end()
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
struct TestWorkspace;
|
||||||
|
|
||||||
|
impl Serialize for TestWorkspace {
|
||||||
|
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
|
||||||
|
where
|
||||||
|
S: serde::Serializer,
|
||||||
|
{
|
||||||
|
let mut state = serializer.serialize_struct("TestWorkspace", 2)?;
|
||||||
|
state.serialize_field("label", "Agentlarım")?;
|
||||||
|
state.serialize_field("emoji", "🚀")?;
|
||||||
|
state.end()
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
let value = TestPayload;
|
||||||
|
let expected_value = json!({
|
||||||
|
"workspaces": {
|
||||||
|
"/tmp/東京": {
|
||||||
|
"label": "Agentlarım",
|
||||||
|
"emoji": "🚀"
|
||||||
|
}
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
let serialized = to_ascii_json_string(&value).expect("serialize ascii json");
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
serialized,
|
||||||
|
r#"{"workspaces":{"/tmp/\u6771\u4eac":{"label":"Agentlar\u0131m","emoji":"\ud83d\ude80"}}}"#
|
||||||
|
);
|
||||||
|
assert!(serialized.is_ascii());
|
||||||
|
assert!(!serialized.contains("東京"));
|
||||||
|
assert!(!serialized.contains("Agentlarım"));
|
||||||
|
assert!(!serialized.contains("🚀"));
|
||||||
|
let parsed: Value = serde_json::from_str(&serialized).expect("serialized json");
|
||||||
|
assert_eq!(parsed, expected_value);
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -1,5 +1,7 @@
|
|||||||
|
mod json;
|
||||||
mod truncate;
|
mod truncate;
|
||||||
|
|
||||||
|
pub use json::to_ascii_json_string;
|
||||||
pub use truncate::approx_bytes_for_tokens;
|
pub use truncate::approx_bytes_for_tokens;
|
||||||
pub use truncate::approx_token_count;
|
pub use truncate::approx_token_count;
|
||||||
pub use truncate::approx_tokens_from_byte_count;
|
pub use truncate::approx_tokens_from_byte_count;
|
||||||
|
|||||||
Reference in new issue
Block a user