Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions cmux-tui/crates/acpmux/src/agent_tools.rs
Original file line number Diff line number Diff line change
Expand Up @@ -781,6 +781,9 @@ impl AgentTools {
.map(|(k, v)| (k.to_owned(), v)),
);
}
if server.name == crate::cloud_repo_mcp::SERVER_NAME {
server.env.push(("ACPMUX_SESSION_ID".to_owned(), session_id.to_owned()));
}
server.env.extend(repo_route);
}
self
Expand Down
26 changes: 26 additions & 0 deletions cmux-tui/crates/acpmux/src/agent_tools_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,3 +197,29 @@ fn a_launch_without_servers_removes_the_last_mcp_config() {
assert!(!file.exists(), "the last launch's config stayed for the hook to read");
let _ = std::fs::remove_dir_all(&root);
}

#[test]
fn cloud_repo_tools_receive_complete_protected_caller_identity() {
let tools = super::AgentTools {
servers: vec![super::McpServer {
name: crate::cloud_repo_mcp::SERVER_NAME.to_owned(),
command: "/bin/acpmux".into(),
args: vec!["repo-mcp".into()],
env: vec![
("ACPMUX_SESSION_ID".into(), "foreign".into()),
("CMUX_BIN".into(), "/bin/cmux".into()),
],
}],
..Default::default()
}
.for_session("own-chat");
let env: std::collections::HashMap<_, _> = tools.servers[0].env.iter().cloned().collect();
assert_eq!(env["ACPMUX_SESSION_ID"], "own-chat");
assert_eq!(env[crate::cloud_repo_mcp::SESSION_ENV], "own-chat");
assert_eq!(
env[crate::hub::person::CALLER_TOKEN_ENV],
crate::hub::person::caller_token("own-chat")
);
assert_eq!(env[crate::hub::person::CALLER_SOCKET_ENV], env["ACPMUX_SOCKET"]);
assert_eq!(env["CMUX_BIN"], "/bin/cmux");
}
13 changes: 13 additions & 0 deletions cmux-tui/crates/acpmux/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ use tokio::sync::{Mutex, mpsc, oneshot};
pub struct Client {
/// Build id the daemon reported at initialize, for mismatch hints.
daemon_build: std::sync::Mutex<Option<String>>,
/// The session this connection proved at initialize, never the caller's claim.
proven_caller_session: std::sync::OnceLock<String>,
/// Absolute path to the CLI bundled with the app that started the daemon,
/// when the daemon was launched by an app. A standalone acpmux CLI does
/// not inherit the app's environment, so this is how `--group` finds the
Expand Down Expand Up @@ -106,6 +108,7 @@ impl Client {
}
let client = Arc::new(Self {
daemon_build: std::sync::Mutex::new(None),
proven_caller_session: std::sync::OnceLock::new(),
daemon_cli_path: std::sync::Mutex::new(None),
daemon_features: std::sync::Mutex::new(Vec::new()),
person_challenge: std::sync::Mutex::new(None),
Expand Down Expand Up @@ -138,6 +141,10 @@ impl Client {
hello.set("_meta", json!({"acpmux": meta}));
}
let init = client.request(method::INITIALIZE, hello).await?;
if let Some(session) = init.pointer("/_meta/acpmux/callerSessionId").and_then(Value::as_str)
{
let _ = client.proven_caller_session.set(session.to_owned());
}
*client.daemon_build.lock_or_recover() =
init.pointer("/_meta/acpmux/build").and_then(Value::as_str).map(str::to_owned);
*client.daemon_cli_path.lock_or_recover() = init
Expand All @@ -159,6 +166,12 @@ impl Client {
Ok(client)
}

/// The session the daemon authenticated on this connection. Older daemons
/// report none, so consumers requiring a caller binding fail closed.
pub fn proven_caller_session(&self) -> Option<&str> {
self.proven_caller_session.get().map(String::as_str)
}

/// Proves this connection is the person (`_acpmux/person_prove`) with the
/// key the daemon was started with; must be the request right after
/// initialize. True when the daemon accepted the proof.
Expand Down
67 changes: 62 additions & 5 deletions cmux-tui/crates/acpmux/src/cloud_repo_mcp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
//!
//! The agent and the web UI use the same daemon actions. This stdio server
//! forwards the chat ID supplied by acpmux; it never trusts a session id
//! supplied by tool arguments.
//! supplied by tool arguments. The acpmux connection must also prove the
//! same session with its caller token before any CLI operation runs.

use crate::client::Client;
use crate::secret_mcp::error_result;
Expand Down Expand Up @@ -115,7 +116,7 @@ fn result_text(result: &Value) -> String {
/// it without a shell keeps the chat ID in an argv value only
/// (never interpolated into a command string), and makes this bridge work with
/// the same socket as `cmux cloud repo`.
async fn run_cli(action: &str, request: Value) -> Result<Value, String> {
async fn run_cli(action: &'static str, request: Value) -> Result<Value, String> {
let binary = std::env::var_os("CMUX_BIN").unwrap_or_else(|| "cmux".into());
run_cli_with(&binary, std::env::var_os("CMUX_SOCKET_PATH").as_deref(), action, request).await
}
Expand Down Expand Up @@ -197,7 +198,25 @@ fn args_object(params: &Value) -> Result<Value, Value> {
}
}

pub async fn call(_daemon: &Arc<Client>, session: &str, params: &Value) -> Value {
pub async fn call(daemon: &Arc<Client>, session: &str, params: &Value) -> Value {
call_for_caller(daemon.proven_caller_session(), session, params, run_cli).await
}

async fn call_for_caller<F, Fut>(
caller: Option<&str>,
session: &str,
params: &Value,
run: F,
) -> Value
where
F: Fn(&'static str, Value) -> Fut,
Fut: std::future::Future<Output = Result<Value, String>>,
{
if caller != Some(session) {
return error_result(
"Repository tools could not verify this chat. Restart the chat session, then retry Add repository.".to_owned(),
);
}
let args = match args_object(params) {
Ok(v) => v,
Err(v) => return v,
Expand All @@ -211,7 +230,7 @@ pub async fn call(_daemon: &Arc<Client>, session: &str, params: &Value) -> Value
{
req.set("query", json!(query));
}
match run_cli(REPO_LIST_ACTION, req).await {
match run(REPO_LIST_ACTION, req).await {
Ok(result) => ok(result),
Err(e) => error_result(e),
}
Expand All @@ -228,7 +247,7 @@ pub async fn call(_daemon: &Arc<Client>, session: &str, params: &Value) -> Value
{
req.set("ref", json!(reference));
}
match run_cli(REPO_ADD_ACTION, req).await {
match run(REPO_ADD_ACTION, req).await {
Ok(mut result) => {
enrich_retry(&mut result, session, repo, args.get("ref"));
ok(result)
Expand Down Expand Up @@ -268,6 +287,44 @@ fn enrich_retry(result: &mut Value, session: &str, repo: &str, reference: Option
#[cfg(test)]
mod tests {
use super::*;
#[tokio::test]
async fn cloud_repo_missing_or_foreign_caller_never_invokes_cli() {
for caller in [None, Some("other-chat")] {
for tool in ["list_repos", "add_repo"] {
let result = call_for_caller(
caller,
"own-chat",
&json!({"name":tool,"arguments":{"repo":"org/repo"}}),
|_, _| async { panic!("unauthenticated caller reached the CLI") },
)
.await;
assert_eq!(result["isError"], true);
assert!(
result["content"][0]["text"]
.as_str()
.unwrap()
.contains("Restart the chat session")
);
}
}
}

#[tokio::test]
async fn cloud_repo_proven_caller_uses_protected_session() {
let result = call_for_caller(
Some("own-chat"),
"own-chat",
&json!({"name":"add_repo","arguments":{"repo":"org/repo","session":"other-chat"}}),
|action, request| async move {
assert_eq!(action, REPO_ADD_ACTION);
assert_eq!(request["session"], "own-chat");
Ok(json!({"state":"done","repo":"org/repo","folder":"/home/chats/own-chat/repo"}))
},
)
.await;
assert_eq!(result["isError"], false);
}

#[test]
fn retry_shape_is_stable() {
let mut result = json!({"code":"repo.needs_github","message":"Connect GitHub."});
Expand Down
2 changes: 2 additions & 0 deletions cmux-tui/crates/acpmux/src/server/requests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -910,6 +910,8 @@ async fn dispatch_request(
// This connection is the person (`hub/person.rs`): never yet,
// as the proof comes after this reply.
"person": conn.is_person(),
// Only a verified first-initialize caller token supplies this ID.
"callerSessionId": conn.caller(),
// The challenge a local connection proves the person key against.
"personChallenge": if conn.challengeable() {
json!({"nonce": conn.nonce, "connection": conn.id})
Expand Down
23 changes: 23 additions & 0 deletions cmux-tui/crates/acpmux/src/server/team_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,10 @@ impl Rpc {
}
let result = client.call("initialize", hello).await;
assert!(result.get("error").is_none(), "{result}");
let expected =
caller.filter(|(_, valid)| *valid && origin == Origin::Local).map(|(id, _)| id);
assert_eq!(result["result"]["_meta"]["acpmux"]["callerSessionId"].as_str(), expected);

client
}

Expand Down Expand Up @@ -494,3 +498,22 @@ async fn team_receipt_origin_survives_restart_and_gates_owner_sends_without_wire
assert_eq!(reply["error"]["data"]["reason"], "team_messaging_disabled", "{reply}");
std::fs::remove_dir_all(dir).unwrap();
}

#[tokio::test]
async fn cloud_repo_initialize_reports_only_the_verified_local_caller() {
let (hub, _) = setup(false);
for caller in [None, Some((OWN, true)), Some((OWN, false))] {
let _client = Rpc::connect(&hub, Origin::Local, caller).await;
}
// A later initialize cannot replace the first connection binding.
let mut client = Rpc::connect(&hub, Origin::Local, Some((OWN, true))).await;
let reply = client
.call(
"initialize",
json!({"_meta":{"acpmux":{"callerSession":{
"sessionId":OTHER,"token":crate::hub::person::caller_token(OTHER)
}}}}),
)
.await;
assert_eq!(reply["result"]["_meta"]["acpmux"]["callerSessionId"], OWN);
}
41 changes: 37 additions & 4 deletions cmux-tui/crates/cmux-tui-core/src/cloud_repos/engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,39 @@ impl Failure {
Self(json!({"code":code,"message":message}))
}
pub fn needs_github(url: &str) -> Self {
if !valid_install_url(url) {
return install_link_required();
}
Self(
json!({"code":"repo.needs_github", "message":"Connect GitHub in the card. This add will retry automatically when connected; no new message is needed.", "install_url":url}),
)
}
}

/// All installation-link sources share this origin check. URL parsing strips
/// some controls, so reject them before parsing rather than trusting its output.
pub(super) fn valid_install_url(value: &str) -> bool {
value.trim() == value
&& !value.chars().any(char::is_control)
&& value.split_once("://").is_some_and(|(_, rest)| {
!rest.split(['/', '?', '#']).next().unwrap_or_default().contains('@')
})
&& url::Url::parse(value).is_ok_and(|url| {
url.scheme() == "https"
&& url.host_str() == Some("github.com")
&& url.username().is_empty()
&& url.password().is_none()
&& url.port().is_none()
})
}

pub(super) fn install_link_required() -> Failure {
Failure::new(
"seed.bad_request",
"Open Add repository on your Mac to refresh the GitHub installation link, then retry.",
)
}

pub(crate) trait Backend {
fn sources(&self) -> Result<Vec<Repo>, Failure>;
fn connector(&self, query: Option<&str>) -> Result<Catalog, Failure>;
Expand Down Expand Up @@ -133,6 +160,9 @@ pub(crate) fn list(backend: &impl Backend, query: Option<&str>) -> Result<Value,
let catalog = backend.connector(query)?;
let mut result = json!({"repos":merge(sources.into_iter().chain(catalog.repos), query)});
if let Some(url) = catalog.install_url {
if !valid_install_url(&url) {
return Err(install_link_required());
}
result.set("connect", json!({"install_url":url}));
}
Ok(result)
Expand Down Expand Up @@ -264,10 +294,13 @@ pub(crate) fn seed_failure(value: &Value) -> Failure {
"repo.no_access",
"Install GitHub access for this organization, or request access from its owner, then retry Add repository.",
);
for key in ["org", "install_url"] {
if let Some(v) = value.get(key) {
result.0.set(key, v.clone());
}
if let Some(org) = value.get("org").and_then(Value::as_str) {
result.0.set("org", json!(org));
}
if let Some(url) =
value.get("install_url").and_then(Value::as_str).filter(|url| valid_install_url(url))
{
result.0.set("install_url", json!(url));
}
return result;
}
Expand Down
50 changes: 26 additions & 24 deletions cmux-tui/crates/cmux-tui-core/src/cloud_repos/runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,19 +73,20 @@ impl<'a> Runtime<'a> {

fn seed_failure(&self, value: &Value, repo: &str) -> Result<Failure, Failure> {
let mut failure = super::engine::seed_failure(value);
if failure.0.get("code").and_then(Value::as_str) == Some("repo.needs_github") {
// Read the raw seed code: the engine may reject its unsafe/missing
// URL, but the connector and the Mac can still supply a trusted one.
let code = value
.get("error_code")
.and_then(Value::as_str)
.or_else(|| value.get("code").and_then(Value::as_str));
if matches!(code, Some("seed.needs_github" | "repo.needs_github")) {
// A linked installation can still exclude this organization or
// repo. Its recovery is access, not reconnecting the same account.
let catalog = self.connector(Some(repo))?;
if let Some(url) = catalog.install_url {
failure = Failure::needs_github(&url);
} else {
let url = value
.get("install_url")
.and_then(Value::as_str)
.filter(|s| !s.is_empty())
.map(str::to_owned)
.or(store::install_url(self.mux)?);
let url = self.optional_install_url(value)?;
failure = Failure::new(
"repo.no_access",
"Install GitHub access for this organization, or request access from its owner, then retry Add repository.",
Expand All @@ -97,6 +98,10 @@ impl<'a> Runtime<'a> {
failure.0.set("install_url", json!(url));
}
}
} else if failure.0.get("code").and_then(Value::as_str) == Some("repo.no_access")
&& let Some(url) = self.optional_install_url(value)?
{
failure.0.set("install_url", json!(url));
}
Ok(failure)
}
Expand All @@ -114,19 +119,20 @@ impl<'a> Runtime<'a> {
}
}

fn optional_install_url(&self, response: &Value) -> Result<Option<String>, Failure> {
// The Mac obtained this link as the signed-in user; prefer it to
// links carried by a connector or seed error. Both paths are checked.
Ok(store::install_url(self.mux)?.or_else(|| {
response
.get("install_url")
.and_then(Value::as_str)
.filter(|url| super::engine::valid_install_url(url))
.map(str::to_owned)
}))
}

fn install_url(&self, response: &Value) -> Result<String, Failure> {
response
.get("install_url")
.and_then(Value::as_str)
.filter(|url| !url.is_empty())
.map(str::to_owned)
.or(store::install_url(self.mux)?)
.ok_or_else(|| {
Failure::new(
"seed.bad_request",
"Open Add repository on your Mac to connect GitHub, then retry.",
)
})
self.optional_install_url(response)?.ok_or_else(super::engine::install_link_required)
}
}
impl Backend for Runtime<'_> {
Expand Down Expand Up @@ -174,11 +180,7 @@ impl Backend for Runtime<'_> {
},
);
if code == "repo.no_access"
&& let Some(url) = error
.get("install_url")
.and_then(Value::as_str)
.map(str::to_owned)
.or(store::install_url(self.mux)?)
&& let Some(url) = self.optional_install_url(error)?
{
failure.0.set("install_url", json!(url));
}
Expand Down
Loading
Loading