Sulkta-Coop/clawdforge
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

clients/rust: apply audit findings — UTF-8 panic + Debug redaction + path-traversal (062d405 → next)

Browse source 
HIGH:
- H1: truncate() uses floor_char_boundary (was panicking on multibyte boundaries)
- H2: hand-written Debug for Client/ClientBuilder/AppToken redacts bearer (was leaking via dbg!()/tracing)
- H3: revoke_token validates name client-side (rejects path traversal sequences)

MEDIUM:
- M1: From<reqwest::Error> maps timeouts to Error::Timeout (was always Transport)
- M2: revoke_token accepts 2xx empty body (was rejecting RFC-correct 204 No Content)
- M3: tests use assert!(matches!) instead of matches!().then_some().unwrap()
- M4: ClientBuilder.max_upload_bytes optional cap
- M5: lib.rs deny(missing_docs)

LOW:
- L1: cargo fmt
- L2: drop dead AUTHORIZATION import

Audit: memory/clawdforge-audits/rust-062d405.md
This commit is contained in:
Kayos 2026-04-28 23:26:01 -07:00
parent 70f4dcc2a4
commit ebbd7cc553
6 changed files with 293 additions and 40 deletions
Download patch file Download diff file Expand all files Collapse all files

19
clients/rust/examples/basic.rs
View file

@ -18,16 +18,12 @@ struct Hello {
#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
let url = std::env::var("CLAWDFORGE_URL")
.unwrap_or_else(|_| "http://localhost:8800".to_string());
let token = std::env::var("CLAWDFORGE_TOKEN").map_err(|_| {
"set CLAWDFORGE_TOKEN to a cf_... bearer minted via /admin/tokens"
})?;
let url =
std::env::var("CLAWDFORGE_URL").unwrap_or_else(|_| "http://localhost:8800".to_string());
let token = std::env::var("CLAWDFORGE_TOKEN")
.map_err(|_| "set CLAWDFORGE_TOKEN to a cf_... bearer minted via /admin/tokens")?;
let client = Client::builder()
.base_url(url)
.token(token)
.build()?;
let client = Client::builder().base_url(url).token(token).build()?;
// 1) liveness
let h = client.healthz().await?;
@ -46,7 +42,10 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
})
.await?;
println!("duration_ms={} stop_reason={:?}", r.duration_ms, r.stop_reason);
println!(
"duration_ms={} stop_reason={:?}",
r.duration_ms, r.stop_reason
);
match r.as_json::<Hello>() {
Ok(v) => println!("parsed: hello = {}", v.hello),

118
clients/rust/src/client.rs
View file

@ -3,7 +3,7 @@
use std::path::Path;
use std::time::Duration;
use reqwest::header::{HeaderMap, HeaderValue, AUTHORIZATION};
use reqwest::header::{HeaderMap, HeaderValue};
use reqwest::multipart::{Form, Part};
use reqwest::{Body, Method, Response, StatusCode};
use serde::de::DeserializeOwned;
@ -24,7 +24,10 @@ const DEFAULT_TIMEOUT: Duration = Duration::from_secs(120);
///
/// Construct via [`Client::builder`]. `Client` is cheap to clone — internally
/// it wraps an `Arc`-backed `reqwest::Client`.
#[derive(Debug, Clone)]
///
/// `Debug` is hand-written to redact bearer tokens — `format!("{:?}", client)`
/// will never expose `app_token` or `admin_token` plaintext.
#[derive(Clone)]
pub struct Client {
inner: reqwest::Client,
base: Url,
@ -33,10 +36,28 @@ pub struct Client {
app_token: Option<String>,
/// Admin bootstrap token (used for `/admin/*`). Optional.
admin_token: Option<String>,
/// Optional cap on `upload_file` size in bytes — `None` = no cap.
max_upload_bytes: Option<u64>,
}
impl std::fmt::Debug for Client {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("Client")
.field("base_url", &self.base.as_str())
.field("app_token", &self.app_token.as_ref().map(|_| "<redacted>"))
.field(
"admin_token",
&self.admin_token.as_ref().map(|_| "<redacted>"),
)
.field("max_upload_bytes", &self.max_upload_bytes)
.finish_non_exhaustive()
}
}
/// Builder for [`Client`].
#[derive(Debug, Default)]
///
/// `Debug` is hand-written to redact bearer tokens.
#[derive(Default)]
pub struct ClientBuilder {
base_url: Option<String>,
app_token: Option<String>,
@ -44,6 +65,27 @@ pub struct ClientBuilder {
timeout: Option<Duration>,
user_agent: Option<String>,
danger_accept_invalid_certs: bool,
max_upload_bytes: Option<u64>,
}
impl std::fmt::Debug for ClientBuilder {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("ClientBuilder")
.field("base_url", &self.base_url)
.field("app_token", &self.app_token.as_ref().map(|_| "<redacted>"))
.field(
"admin_token",
&self.admin_token.as_ref().map(|_| "<redacted>"),
)
.field("timeout", &self.timeout)
.field("user_agent", &self.user_agent)
.field(
"danger_accept_invalid_certs",
&self.danger_accept_invalid_certs,
)
.field("max_upload_bytes", &self.max_upload_bytes)
.finish_non_exhaustive()
}
}
impl Client {
@ -99,6 +141,10 @@ impl Client {
///
/// `ttl_secs` defaults to the server's 3600 if `None`. Server clamps to
/// `60..=86400`.
///
/// If [`ClientBuilder::max_upload_bytes`] was set and the file's size on
/// disk exceeds it, returns [`Error::Config`] before opening any network
/// connection.
pub async fn upload_file(
&self,
path: impl AsRef<Path>,
@ -118,6 +164,14 @@ impl Client {
let file = tokio::fs::File::open(path).await?;
let len = file.metadata().await?.len();
if let Some(max) = self.max_upload_bytes {
if len > max {
return Err(Error::Config(format!(
"file size {len} bytes exceeds max_upload_bytes={max}"
)));
}
}
let stream = ReaderStream::new(file);
let body = Body::wrap_stream(stream);
@ -143,10 +197,7 @@ impl Client {
}
/// `POST /admin/tokens`. Requires an admin token on the client.
pub async fn create_token(
&self,
body: TokenCreateRequest,
) -> Result<AppToken, Error> {
pub async fn create_token(&self, body: TokenCreateRequest) -> Result<AppToken, Error> {
let token = self.require_admin()?;
let url = self.url("/admin/tokens")?;
let resp = self
@ -163,27 +214,38 @@ impl Client {
pub async fn list_tokens(&self) -> Result<TokenList, Error> {
let token = self.require_admin()?;
let url = self.url("/admin/tokens")?;
let resp = self
.inner
.get(url)
.bearer_auth(token)
.send()
.await?;
let resp = self.inner.get(url).bearer_auth(token).send().await?;
json_or_error(resp).await
}
/// `DELETE /admin/tokens/{name}`. Requires an admin token on the client.
/// Returns `Ok(())` on success, [`Error::Api`] with status 404 if the
/// token does not exist.
///
/// `name` is validated client-side to match the server's
/// `[a-z0-9][a-z0-9_-]{0,63}` constraint — anything containing `/`, `?`,
/// `#`, `..`, or empty short-circuits with [`Error::Config`] before a
/// request is sent. This is defense-in-depth against path traversal via
/// `Url::join` (which honors RFC 3986 `..` resolution).
pub async fn revoke_token(&self, name: &str) -> Result<(), Error> {
if name.is_empty()
|| name.contains('/')
|| name.contains('?')
|| name.contains('#')
|| name.contains("..")
{
return Err(Error::Config(format!("invalid token name: {name:?}")));
}
let token = self.require_admin()?;
let url = self.url(&format!("/admin/tokens/{name}"))?;
let resp = self
.inner
.delete(url)
.bearer_auth(token)
.send()
.await?;
let resp = self.inner.delete(url).bearer_auth(token).send().await?;
// 2xx is success regardless of body — RFC-correct DELETE may return
// 204 No Content with no body.
if resp.status().is_success() {
return Ok(());
}
// Non-2xx: route through json_or_error to get Auth/Api mapping.
// Discard the (already-non-success) deserialization slot.
let _: serde_json::Value = json_or_error(resp).await?;
Ok(())
}
@ -245,6 +307,15 @@ impl ClientBuilder {
self
}
/// Maximum file size (in bytes) accepted by [`Client::upload_file`].
/// Files exceeding this cap fail with [`Error::Config`] before any
/// network I/O. Default `None` = no client-side cap (the server's own
/// limit still applies).
pub fn max_upload_bytes(mut self, max: u64) -> Self {
self.max_upload_bytes = Some(max);
self
}
/// Finalize. Errors if `base_url` is missing or unparseable.
pub fn build(self) -> Result<Client, Error> {
let base_raw = self
@ -276,7 +347,6 @@ impl ClientBuilder {
);
// We don't preset Authorization here — per-call helpers do it because
// the right token depends on which endpoint is being hit.
let _ = AUTHORIZATION; // silence unused-import lint in some configs
let inner = reqwest::Client::builder()
.timeout(self.timeout.unwrap_or(DEFAULT_TIMEOUT))
@ -290,6 +360,7 @@ impl ClientBuilder {
base,
app_token: self.app_token,
admin_token: self.admin_token,
max_upload_bytes: self.max_upload_bytes,
})
}
}
@ -314,10 +385,15 @@ async fn json_or_error<T: DeserializeOwned>(resp: Response) -> Result<T, Error>
}
}
/// Truncate `s` to at most `max` bytes, snapping down to the nearest UTF-8
/// codepoint boundary so we never panic on multibyte sequences. Appends `…`
/// to the truncated form. `str::floor_char_boundary` (stable 1.80+) does the
/// boundary math.
fn truncate(s: &str, max: usize) -> String {
if s.len() <= max {
s.to_string()
} else {
format!("{}…", &s[..max])
let safe = s.floor_char_boundary(max);
format!("{}…", &s[..safe])
}
}

16
clients/rust/src/error.rs
View file

@ -25,7 +25,7 @@ pub enum Error {
/// Request never completed: DNS, connect, TLS, body-read, etc.
#[error("transport error: {0}")]
Transport(#[from] reqwest::Error),
Transport(reqwest::Error),
/// JSON decode failed on a successful HTTP response.
#[error("json error: {0}")]
@ -37,8 +37,8 @@ pub enum Error {
Io(#[from] std::io::Error),
/// The configured request timeout elapsed before the server replied.
/// `reqwest` also surfaces timeouts via [`Error::Transport`]; this variant
/// is reserved for explicit deadlines in the client itself.
/// Mapped from `reqwest::Error::is_timeout()` so callers can match on
/// timeouts specifically without inspecting the inner transport error.
#[error("timeout: {0}")]
Timeout(String),
@ -47,6 +47,16 @@ pub enum Error {
Config(String),
}
impl From<reqwest::Error> for Error {
fn from(e: reqwest::Error) -> Self {
if e.is_timeout() {
Self::Timeout(e.to_string())
} else {
Self::Transport(e)
}
}
}
impl Error {
/// Build an [`Error::Api`] from a status code and body string.
pub(crate) fn api(status: u16, body: impl Into<String>) -> Self {

2
clients/rust/src/lib.rs
View file

@ -42,7 +42,7 @@
//! container attribute.
#![deny(rust_2018_idioms)]
#![warn(missing_docs)]
#![deny(missing_docs)]
mod client;
mod error;

15
clients/rust/src/types.rs
View file

@ -124,7 +124,10 @@ pub struct FileToken {
/// Response body from `POST /admin/tokens`.
///
/// `token` is the plaintext bearer — only returned at creation time.
#[derive(Debug, Clone, Deserialize, Serialize)]
///
/// `Debug` is hand-written to redact `token` (the plaintext bearer); `name`
/// and `ip_cidrs` print verbatim.
#[derive(Clone, Deserialize, Serialize)]
pub struct AppToken {
/// App / consumer name.
pub name: String,
@ -136,6 +139,16 @@ pub struct AppToken {
pub ip_cidrs: Vec<String>,
}
impl std::fmt::Debug for AppToken {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("AppToken")
.field("name", &self.name)
.field("token", &"<redacted>")
.field("ip_cidrs", &self.ip_cidrs)
.finish()
}
}
/// Request body for `POST /admin/tokens`.
#[derive(Debug, Clone, Default, Serialize, Deserialize)]
pub struct TokenCreateRequest {

163
clients/rust/tests/client.rs
View file

@ -106,7 +106,10 @@ async fn run_success_with_text_result() {
.unwrap();
assert_eq!(r.as_text(), Some("plain string reply"));
let json_attempt: Result<serde_json::Map<String, serde_json::Value>, _> = r.as_json();
assert!(json_attempt.is_err(), "string should not deserialize as map");
assert!(
json_attempt.is_err(),
"string should not deserialize as map"
);
}
#[tokio::test]
@ -293,7 +296,7 @@ async fn unauthorized_response_maps_to_auth_error() {
})
.await
.expect_err("should fail");
matches!(err, Error::Auth(_)).then_some(()).unwrap();
assert!(matches!(err, Error::Auth(_)));
}
#[tokio::test]
@ -320,7 +323,7 @@ async fn missing_app_token_short_circuits_run() {
}
#[tokio::test]
async fn transport_timeout_surfaces_as_transport_error() {
async fn error_timeout_constructed_on_reqwest_timeout() {
let server = MockServer::start().await;
Mock::given(method("GET"))
.and(path("/healthz"))
@ -343,7 +346,7 @@ async fn transport_timeout_surfaces_as_transport_error() {
.build()
.unwrap();
let err = c.healthz().await.expect_err("should time out");
assert!(matches!(err, Error::Transport(_)), "got {err:?}");
assert!(matches!(err, Error::Timeout(_)), "got {err:?}");
}
#[tokio::test]
@ -360,3 +363,155 @@ async fn builder_rejects_bad_scheme() {
.expect_err("should fail");
assert!(matches!(err, Error::Config(_)));
}
// ---- audit-driven regression tests --------------------------------------
/// H1: 4xx body with multibyte char straddling the truncation cutoff must
/// not panic. Build a 503-byte string where `ü` (2 bytes UTF-8) lands at
/// offset 499..501, so byte 500 is mid-codepoint.
#[tokio::test]
async fn truncate_handles_multibyte_boundary() {
let server = MockServer::start().await;
let mut body = String::new();
for _ in 0..499 {
body.push('a');
}
body.push('ü'); // bytes 499 and 500
for _ in 0..2 {
body.push('b');
}
assert_eq!(body.len(), 503);
assert!(!body.is_char_boundary(500));
Mock::given(method("POST"))
.and(path("/run"))
.respond_with(ResponseTemplate::new(401).set_body_string(body.clone()))
.mount(&server)
.await;
let c = make_client(&server);
let err = c
.run(RunRequest {
prompt: "x".into(),
..Default::default()
})
.await
.expect_err("should fail");
// Just having reached this line — without panicking — is the assertion.
assert!(matches!(err, Error::Auth(_)), "got {err:?}");
}
/// H2: `Debug` on `Client` must not leak app or admin tokens.
#[tokio::test]
async fn client_debug_redacts_bearer() {
let server = MockServer::start().await;
let c = Client::builder()
.base_url(server.uri())
.token("cf_super_secret_app_bearer")
.admin_token("admin_super_secret_bearer")
.build()
.unwrap();
let dbg = format!("{c:?}");
assert!(
!dbg.contains("cf_super_secret_app_bearer"),
"app token leaked: {dbg}"
);
assert!(
!dbg.contains("admin_super_secret_bearer"),
"admin token leaked: {dbg}"
);
assert!(dbg.contains("<redacted>"), "no redaction marker: {dbg}");
// ClientBuilder Debug also redacts.
let builder = Client::builder()
.base_url("http://x")
.token("cf_builder_secret");
let bdbg = format!("{builder:?}");
assert!(
!bdbg.contains("cf_builder_secret"),
"builder token leaked: {bdbg}"
);
assert!(bdbg.contains("<redacted>"), "no redaction marker: {bdbg}");
}
/// H2: `Debug` on `AppToken` must not leak the plaintext `token` field.
#[test]
fn app_token_debug_redacts_token() {
let t = clawdforge::AppToken {
name: "cauldron".into(),
token: "cf_should_not_appear".into(),
ip_cidrs: vec!["172.24.0.0/16".into()],
};
let dbg = format!("{t:?}");
assert!(!dbg.contains("cf_should_not_appear"), "leaked: {dbg}");
assert!(dbg.contains("<redacted>"), "no marker: {dbg}");
// name + ip_cidrs are non-secret and should still print.
assert!(dbg.contains("cauldron"));
assert!(dbg.contains("172.24.0.0/16"));
}
/// H3: `revoke_token` must reject path-traversal sequences before issuing
/// any HTTP request.
#[tokio::test]
async fn revoke_token_rejects_path_traversal() {
let server = MockServer::start().await;
// No mock — if a request escaped client-side validation, wiremock would
// 404 and we'd see Error::Api, not Error::Config.
let c = make_client(&server);
for bad in [
"../foo", "..", "foo/bar", "foo?x=1", "foo#frag", "", "a/../b",
] {
let err = c
.revoke_token(bad)
.await
.expect_err(&format!("revoke_token({bad:?}) should reject"));
assert!(
matches!(err, Error::Config(_)),
"{bad:?} produced wrong variant: {err:?}"
);
}
}
/// M2: a 204 No Content response from `revoke_token` must Ok-out.
#[tokio::test]
async fn revoke_token_accepts_204_no_content() {
let server = MockServer::start().await;
Mock::given(method("DELETE"))
.and(path_regex(r"^/admin/tokens/.+"))
.and(header("authorization", "Bearer admin_test_token"))
.respond_with(ResponseTemplate::new(204))
.mount(&server)
.await;
let c = make_client(&server);
c.revoke_token("cauldron")
.await
.expect("204 No Content should be Ok");
}
/// M4: `upload_file` with a `max_upload_bytes` cap rejects oversized files
/// before any network I/O.
#[tokio::test]
async fn upload_file_respects_max_upload_bytes() {
let server = MockServer::start().await;
// No /files mock — if the cap fails to short-circuit, the test will see
// a 404 from wiremock instead of Error::Config.
let mut tmp = tempfile::NamedTempFile::new().unwrap();
// Write 1024 bytes; cap at 512.
write!(tmp, "{}", "x".repeat(1024)).unwrap();
tmp.flush().unwrap();
let c = Client::builder()
.base_url(server.uri())
.token("cf_test_token")
.max_upload_bytes(512)
.build()
.unwrap();
let err = c
.upload_file(tmp.path(), Some(1800))
.await
.expect_err("should reject oversize");
assert!(matches!(err, Error::Config(_)), "got {err:?}");
}