bxctl: escape frame errors; say the outcome is unknown after a timeout
From the independent review of task 23. serde quotes a bad frame's text after decoding, so a compromised peer could put escape sequences in it. A timed-out admin request now says whether brokerd acted is unknown, since it may have. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -40,7 +40,17 @@ pub fn request_with_timeout(
|
|||||||
msg,
|
msg,
|
||||||
};
|
};
|
||||||
write_frame(&mut stream, &env).map_err(AdminError::Frame)?;
|
write_frame(&mut stream, &env).map_err(AdminError::Frame)?;
|
||||||
let answer = read_frame(&mut stream).map_err(AdminError::Frame)?;
|
let answer = read_frame(&mut stream).map_err(|e| match e {
|
||||||
|
FrameError::Io(io)
|
||||||
|
if matches!(
|
||||||
|
io.kind(),
|
||||||
|
io::ErrorKind::WouldBlock | io::ErrorKind::TimedOut
|
||||||
|
) =>
|
||||||
|
{
|
||||||
|
AdminError::NoAnswer(timeout)
|
||||||
|
}
|
||||||
|
other => AdminError::Frame(other),
|
||||||
|
})?;
|
||||||
if answer.id != 1 || !answer.r#final {
|
if answer.id != 1 || !answer.r#final {
|
||||||
return Err(AdminError::Protocol(
|
return Err(AdminError::Protocol(
|
||||||
"expected an answer for request 1".to_string(),
|
"expected an answer for request 1".to_string(),
|
||||||
@@ -70,19 +80,27 @@ pub enum AdminError {
|
|||||||
Protocol(String),
|
Protocol(String),
|
||||||
/// Writing the output failed; the caller stops rather than write again.
|
/// Writing the output failed; the caller stops rather than write again.
|
||||||
Io(std::io::Error),
|
Io(std::io::Error),
|
||||||
|
/// The request was sent and no answer came in time, so what brokerd did is not known.
|
||||||
|
NoAnswer(std::time::Duration),
|
||||||
}
|
}
|
||||||
|
|
||||||
impl std::fmt::Display for AdminError {
|
impl std::fmt::Display for AdminError {
|
||||||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
|
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
|
||||||
match self {
|
match self {
|
||||||
AdminError::Connect(p, e) => write!(f, "cannot reach brokerd at {}: {e}", p.display()),
|
AdminError::Connect(p, e) => write!(f, "cannot reach brokerd at {}: {e}", p.display()),
|
||||||
AdminError::Frame(e) => write!(f, "{e}"),
|
// serde quotes the offending text of a bad frame, decoded: escape it.
|
||||||
|
AdminError::Frame(e) => write!(f, "{}", escape_json_text(&e.to_string())),
|
||||||
// The detail may carry text from the inference server or a tool: escape it.
|
// The detail may carry text from the inference server or a tool: escape it.
|
||||||
AdminError::Refused(w) => {
|
AdminError::Refused(w) => {
|
||||||
write!(f, "{}: {}", code_name(w.code), escape_json_text(&w.detail))
|
write!(f, "{}: {}", code_name(w.code), escape_json_text(&w.detail))
|
||||||
}
|
}
|
||||||
AdminError::Protocol(s) => write!(f, "{s}"),
|
AdminError::Protocol(s) => write!(f, "{s}"),
|
||||||
AdminError::Io(e) => write!(f, "{e}"),
|
AdminError::Io(e) => write!(f, "{e}"),
|
||||||
|
AdminError::NoAnswer(t) => write!(
|
||||||
|
f,
|
||||||
|
"brokerd did not answer within {} s; whether it acted is unknown: check `bxctl approvals` and `bxctl audit verify`",
|
||||||
|
t.as_secs_f64()
|
||||||
|
),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -24,7 +24,8 @@ impl std::fmt::Display for ChatError {
|
|||||||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
|
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
|
||||||
match self {
|
match self {
|
||||||
ChatError::Connect(e) => write!(f, "{e}"),
|
ChatError::Connect(e) => write!(f, "{e}"),
|
||||||
ChatError::Frame(e) => write!(f, "{e}"),
|
// serde quotes the offending text of a bad frame, decoded: escape it.
|
||||||
|
ChatError::Frame(e) => write!(f, "{}", escape_json_text(&e.to_string())),
|
||||||
// The detail may carry the inference server's body: escape it like model text.
|
// The detail may carry the inference server's body: escape it like model text.
|
||||||
ChatError::Refused(w) => {
|
ChatError::Refused(w) => {
|
||||||
write!(f, "{}: {}", code_name(w.code), escape_json_text(&w.detail))
|
write!(f, "{}: {}", code_name(w.code), escape_json_text(&w.detail))
|
||||||
|
|||||||
@@ -31,7 +31,11 @@ fn a_silent_brokerd_is_an_error_after_the_timeout() {
|
|||||||
Duration::from_millis(200),
|
Duration::from_millis(200),
|
||||||
);
|
);
|
||||||
let took = started.elapsed();
|
let took = started.elapsed();
|
||||||
assert!(got.is_err(), "{got:?}");
|
let text = got.expect_err("a silent brokerd is an error").to_string();
|
||||||
|
assert!(
|
||||||
|
text.contains("whether it acted is unknown"),
|
||||||
|
"brokerd may have approved and run the call before going quiet: {text}"
|
||||||
|
);
|
||||||
assert!(took < Duration::from_millis(1_500), "waited {took:?}");
|
assert!(took < Duration::from_millis(1_500), "waited {took:?}");
|
||||||
held.join().unwrap();
|
held.join().unwrap();
|
||||||
let _ = std::fs::remove_dir_all(&dir);
|
let _ = std::fs::remove_dir_all(&dir);
|
||||||
|
|||||||
@@ -55,3 +55,21 @@ fn an_error_detail_is_escaped_in_chat_and_admin_errors() {
|
|||||||
assert!(text.contains("\\u202e"), "{text}");
|
assert!(text.contains("\\u202e"), "{text}");
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// A frame that does not decode is reported with serde's message, which quotes the offending
|
||||||
|
/// text after JSON decoding: from a compromised peer that can be raw escape sequences.
|
||||||
|
#[test]
|
||||||
|
fn a_frame_error_is_escaped() {
|
||||||
|
let bad = r#"{"v":1,"id":1,"final":true,"msg":{"kind":"\u001b[2J\u202e"}}"#;
|
||||||
|
let error = serde_json::from_str::<proto::Envelope>(bad).unwrap_err();
|
||||||
|
for text in [
|
||||||
|
ChatError::Frame(proto::FrameError::Json(
|
||||||
|
serde_json::from_str::<proto::Envelope>(bad).unwrap_err(),
|
||||||
|
))
|
||||||
|
.to_string(),
|
||||||
|
AdminError::Frame(proto::FrameError::Json(error)).to_string(),
|
||||||
|
] {
|
||||||
|
clean(&text);
|
||||||
|
assert!(text.contains("\\u001b"), "{text}");
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user