acp: Support raw agent registry binaries (#58579)
It is valid to have raw binary paths in the registry but we didn't handle them. Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Release Notes: - acp: Fix for certain ACP Registry agent downloads not starting
This commit is contained in:
1 parent
b45344df51
commit
21df82d5a4
2 files changed
+349
-48
No files matched your search
@@ -6,7 +6,7 @@ use std::{
|
||||
|
||||
use anyhow::{Context, Result};
|
||||
use async_compression::futures::bufread::{BzDecoder, GzipDecoder};
|
||||
use futures::{AsyncRead, AsyncSeek, AsyncSeekExt, AsyncWrite, io::BufReader};
|
||||
use futures::{AsyncRead, AsyncSeek, AsyncSeekExt, AsyncWrite, AsyncWriteExt, io::BufReader};
|
||||
use sha2::{Digest, Sha256};
|
||||
|
||||
use crate::{HttpClient, github::AssetKind};
|
||||
@@ -68,6 +68,64 @@ pub async fn download_server_binary(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub async fn download_server_raw_binary(
|
||||
http_client: &dyn HttpClient,
|
||||
url: &str,
|
||||
digest: Option<&str>,
|
||||
destination_path: &Path,
|
||||
binary_file_name: &str,
|
||||
) -> Result<(), anyhow::Error> {
|
||||
log::info!("downloading raw binary from {url}");
|
||||
let Some(destination_parent) = destination_path.parent() else {
|
||||
anyhow::bail!("destination path has no parent: {destination_path:?}");
|
||||
};
|
||||
|
||||
let staging_path = staging_dir_path(destination_parent)?;
|
||||
let result = async {
|
||||
let mut response = http_client
|
||||
.get(url, Default::default(), true)
|
||||
.await
|
||||
.with_context(|| format!("downloading release from {url}"))?;
|
||||
|
||||
let binary_path = staging_path.join(binary_file_name);
|
||||
let mut writer = HashingWriter {
|
||||
writer: async_fs::File::create(&binary_path)
|
||||
.await
|
||||
.with_context(|| format!("creating a file {binary_path:?} for {url}"))?,
|
||||
hasher: Sha256::new(),
|
||||
};
|
||||
futures::io::copy(&mut BufReader::new(response.body_mut()), &mut writer)
|
||||
.await
|
||||
.with_context(|| format!("saving binary contents from {url}"))?;
|
||||
let asset_sha_256 = writer
|
||||
.finish()
|
||||
.await
|
||||
.with_context(|| format!("flushing binary contents for {url}"))?;
|
||||
|
||||
if let Some(expected_sha_256) = digest {
|
||||
anyhow::ensure!(
|
||||
asset_sha_256 == expected_sha_256,
|
||||
"{url} asset got SHA-256 mismatch. Expected: {expected_sha_256}, Got: {asset_sha_256}",
|
||||
);
|
||||
}
|
||||
|
||||
util::fs::make_file_executable(&binary_path)
|
||||
.await
|
||||
.with_context(|| format!("marking {binary_path:?} as executable"))?;
|
||||
finalize_download(&staging_path, destination_path).await
|
||||
}
|
||||
.await;
|
||||
|
||||
if let Err(err) = result {
|
||||
if let Err(err) = async_fs::remove_dir_all(&staging_path).await {
|
||||
log::warn!("failed to remove staging directory {staging_path:?}: {err:?}");
|
||||
}
|
||||
return Err(err);
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn extract_to_staging(
|
||||
body: impl AsyncRead + Unpin,
|
||||
digest: Option<&str>,
|
||||
@@ -117,15 +175,17 @@ async fn extract_to_staging(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn staging_dir_path(parent: &Path) -> Result<PathBuf> {
|
||||
let dir = tempfile::Builder::new()
|
||||
.prefix(".tmp-github-download-")
|
||||
.tempdir_in(parent)
|
||||
.with_context(|| format!("creating staging directory in {parent:?}"))?;
|
||||
Ok(dir.keep())
|
||||
}
|
||||
|
||||
fn staging_path(parent: &Path, asset_kind: AssetKind) -> Result<PathBuf> {
|
||||
match asset_kind {
|
||||
AssetKind::TarGz | AssetKind::TarBz2 | AssetKind::Zip => {
|
||||
let dir = tempfile::Builder::new()
|
||||
.prefix(".tmp-github-download-")
|
||||
.tempdir_in(parent)
|
||||
.with_context(|| format!("creating staging directory in {parent:?}"))?;
|
||||
Ok(dir.keep())
|
||||
}
|
||||
AssetKind::TarGz | AssetKind::TarBz2 | AssetKind::Zip => staging_dir_path(parent),
|
||||
AssetKind::Gz => {
|
||||
let path = tempfile::Builder::new()
|
||||
.prefix(".tmp-github-download-")
|
||||
@@ -261,6 +321,22 @@ struct HashingWriter<W: AsyncWrite + Unpin> {
|
||||
hasher: Sha256,
|
||||
}
|
||||
|
||||
impl<W: AsyncWrite + Unpin> HashingWriter<W> {
|
||||
/// Closes and drops the inner writer, returning the hex SHA-256 digest of
|
||||
/// everything written.
|
||||
///
|
||||
/// Taking `self` by value guarantees the writer is dropped before this
|
||||
/// returns. For file writers this releases the OS handle, which Windows
|
||||
/// requires before an ancestor directory can be renamed or deleted; note
|
||||
/// that closing alone is not enough, as `async_fs::File` holds its handle
|
||||
/// until dropped.
|
||||
async fn finish(mut self) -> std::io::Result<String> {
|
||||
self.writer.close().await?;
|
||||
drop(self.writer);
|
||||
Ok(format!("{:x}", self.hasher.finalize()))
|
||||
}
|
||||
}
|
||||
|
||||
impl<W: AsyncWrite + Unpin> AsyncWrite for HashingWriter<W> {
|
||||
fn poll_write(
|
||||
mut self: Pin<&mut Self>,
|
||||
@@ -290,3 +366,101 @@ impl<W: AsyncWrite + Unpin> AsyncWrite for HashingWriter<W> {
|
||||
Pin::new(&mut self.writer).poll_close(cx)
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::{AsyncBody, Response};
|
||||
use futures::future::BoxFuture;
|
||||
use http::HeaderValue;
|
||||
use url::Url;
|
||||
|
||||
struct StaticResponseClient {
|
||||
body: Vec<u8>,
|
||||
}
|
||||
|
||||
impl HttpClient for StaticResponseClient {
|
||||
fn send(
|
||||
&self,
|
||||
_req: http::Request<AsyncBody>,
|
||||
) -> BoxFuture<'static, anyhow::Result<Response<AsyncBody>>> {
|
||||
let body = self.body.clone();
|
||||
Box::pin(async move {
|
||||
Ok(Response::builder()
|
||||
.status(200)
|
||||
.body(AsyncBody::from(body))
|
||||
.unwrap())
|
||||
})
|
||||
}
|
||||
|
||||
fn user_agent(&self) -> Option<&HeaderValue> {
|
||||
None
|
||||
}
|
||||
|
||||
fn proxy(&self) -> Option<&Url> {
|
||||
None
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn downloads_raw_binary_into_destination_dir() {
|
||||
futures::executor::block_on(async {
|
||||
let temp_dir = tempfile::tempdir().unwrap();
|
||||
let destination_path = temp_dir.path().join("v_1");
|
||||
let contents = b"#!/bin/sh\necho hello\n".to_vec();
|
||||
let expected_sha_256 = format!("{:x}", Sha256::digest(&contents));
|
||||
let client = StaticResponseClient { body: contents };
|
||||
|
||||
download_server_raw_binary(
|
||||
&client,
|
||||
"https://example.com/agent-binary",
|
||||
Some(&expected_sha_256),
|
||||
&destination_path,
|
||||
"agent-binary",
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
let binary_path = destination_path.join("agent-binary");
|
||||
assert_eq!(
|
||||
std::fs::read(&binary_path).unwrap(),
|
||||
b"#!/bin/sh\necho hello\n"
|
||||
);
|
||||
#[cfg(unix)]
|
||||
{
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
let mode = std::fs::metadata(&binary_path)
|
||||
.unwrap()
|
||||
.permissions()
|
||||
.mode();
|
||||
assert_eq!(mode & 0o111, 0o111, "binary should be executable");
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn raw_binary_digest_mismatch_cleans_up_staging() {
|
||||
futures::executor::block_on(async {
|
||||
let temp_dir = tempfile::tempdir().unwrap();
|
||||
let destination_path = temp_dir.path().join("v_1");
|
||||
let client = StaticResponseClient {
|
||||
body: b"some binary".to_vec(),
|
||||
};
|
||||
|
||||
let error = download_server_raw_binary(
|
||||
&client,
|
||||
"https://example.com/agent-binary",
|
||||
Some("0000000000000000000000000000000000000000000000000000000000000000"),
|
||||
&destination_path,
|
||||
"agent-binary",
|
||||
)
|
||||
.await
|
||||
.unwrap_err();
|
||||
|
||||
assert!(error.to_string().contains("SHA-256 mismatch"));
|
||||
assert!(!destination_path.exists());
|
||||
let leftover_entries = std::fs::read_dir(temp_dir.path()).unwrap().count();
|
||||
assert_eq!(leftover_entries, 0, "staging directory should be removed");
|
||||
});
|
||||
}
|
||||
}
|
||||
@@ -891,23 +891,82 @@ impl ExternalAgentServer for RemoteExternalAgentServer {
|
||||
}
|
||||
}
|
||||
|
||||
fn asset_kind_for_archive_url(archive_url: &str) -> Result<AssetKind> {
|
||||
#[derive(Debug, PartialEq, Eq)]
|
||||
enum RegistryArchiveKind {
|
||||
Archive(AssetKind),
|
||||
/// The archive URL points directly at an executable, per the ACP registry
|
||||
/// schema: "URL to download archive (.zip, .tar.gz, .tgz, .tar.bz2, .tbz2,
|
||||
/// or raw binary)".
|
||||
RawBinary {
|
||||
file_name: String,
|
||||
},
|
||||
}
|
||||
|
||||
fn registry_archive_kind_for_url(archive_url: &str) -> Result<RegistryArchiveKind> {
|
||||
const UNSUPPORTED_SUFFIXES: &[&str] = &[
|
||||
// Installer formats explicitly rejected by the registry schema.
|
||||
".dmg",
|
||||
".pkg",
|
||||
".deb",
|
||||
".rpm",
|
||||
".msi",
|
||||
".appimage",
|
||||
// Archive formats we cannot extract; treating them as raw binaries
|
||||
// would produce a broken install.
|
||||
".tar.xz",
|
||||
".txz",
|
||||
".tar",
|
||||
".gz",
|
||||
".bz2",
|
||||
".xz",
|
||||
".7z",
|
||||
];
|
||||
|
||||
let archive_path = Url::parse(archive_url)
|
||||
.ok()
|
||||
.map(|url| url.path().to_string())
|
||||
.unwrap_or_else(|| archive_url.to_string());
|
||||
let lowercase_path = archive_path.to_lowercase();
|
||||
|
||||
if archive_path.ends_with(".zip") {
|
||||
Ok(AssetKind::Zip)
|
||||
} else if archive_path.ends_with(".tar.gz") || archive_path.ends_with(".tgz") {
|
||||
Ok(AssetKind::TarGz)
|
||||
} else if archive_path.ends_with(".tar.bz2") || archive_path.ends_with(".tbz2") {
|
||||
Ok(AssetKind::TarBz2)
|
||||
if lowercase_path.ends_with(".zip") {
|
||||
Ok(RegistryArchiveKind::Archive(AssetKind::Zip))
|
||||
} else if lowercase_path.ends_with(".tar.gz") || lowercase_path.ends_with(".tgz") {
|
||||
Ok(RegistryArchiveKind::Archive(AssetKind::TarGz))
|
||||
} else if lowercase_path.ends_with(".tar.bz2") || lowercase_path.ends_with(".tbz2") {
|
||||
Ok(RegistryArchiveKind::Archive(AssetKind::TarBz2))
|
||||
} else if let Some(suffix) = UNSUPPORTED_SUFFIXES
|
||||
.iter()
|
||||
.find(|suffix| lowercase_path.ends_with(*suffix))
|
||||
{
|
||||
bail!("unsupported archive type {suffix} in URL: {archive_url}");
|
||||
} else {
|
||||
bail!("unsupported archive type in URL: {archive_url}");
|
||||
let file_name = raw_binary_file_name(&archive_path)
|
||||
.with_context(|| format!("determining binary file name from URL: {archive_url}"))?;
|
||||
Ok(RegistryArchiveKind::RawBinary { file_name })
|
||||
}
|
||||
}
|
||||
|
||||
fn raw_binary_file_name(archive_path: &str) -> Result<String> {
|
||||
let last_segment = archive_path
|
||||
.rsplit('/')
|
||||
.next()
|
||||
.filter(|segment| !segment.is_empty())
|
||||
.context("URL has no file name")?;
|
||||
let file_name = percent_decode_str(last_segment)
|
||||
.decode_utf8()
|
||||
.context("file name is not valid UTF-8")?
|
||||
.into_owned();
|
||||
anyhow::ensure!(
|
||||
!file_name.is_empty()
|
||||
&& file_name != "."
|
||||
&& file_name != ".."
|
||||
&& !file_name.contains(['/', '\\'])
|
||||
&& !file_name.contains('\0'),
|
||||
"invalid binary file name: {file_name}"
|
||||
);
|
||||
Ok(file_name)
|
||||
}
|
||||
|
||||
struct GithubReleaseArchive {
|
||||
repo_name_with_owner: String,
|
||||
tag: String,
|
||||
@@ -1187,16 +1246,28 @@ impl ExternalAgentServer for LocalRegistryArchiveAgent {
|
||||
None
|
||||
};
|
||||
|
||||
let asset_kind = asset_kind_for_archive_url(archive_url)?;
|
||||
|
||||
::http_client::github_download::download_server_binary(
|
||||
&*http_client,
|
||||
archive_url,
|
||||
sha256.as_deref(),
|
||||
&version_dir,
|
||||
asset_kind,
|
||||
)
|
||||
.await?;
|
||||
match registry_archive_kind_for_url(archive_url)? {
|
||||
RegistryArchiveKind::Archive(asset_kind) => {
|
||||
::http_client::github_download::download_server_binary(
|
||||
&*http_client,
|
||||
archive_url,
|
||||
sha256.as_deref(),
|
||||
&version_dir,
|
||||
asset_kind,
|
||||
)
|
||||
.await?;
|
||||
}
|
||||
RegistryArchiveKind::RawBinary { file_name } => {
|
||||
::http_client::github_download::download_server_raw_binary(
|
||||
&*http_client,
|
||||
archive_url,
|
||||
sha256.as_deref(),
|
||||
&version_dir,
|
||||
&file_name,
|
||||
)
|
||||
.await?;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
let cmd = &target_config.cmd;
|
||||
@@ -1703,45 +1774,84 @@ mod tests {
|
||||
#[test]
|
||||
fn detects_supported_archive_suffixes() {
|
||||
assert!(matches!(
|
||||
asset_kind_for_archive_url("https://example.com/agent.zip"),
|
||||
Ok(AssetKind::Zip)
|
||||
registry_archive_kind_for_url("https://example.com/agent.zip"),
|
||||
Ok(RegistryArchiveKind::Archive(AssetKind::Zip))
|
||||
));
|
||||
assert!(matches!(
|
||||
asset_kind_for_archive_url("https://example.com/agent.zip?download=1"),
|
||||
Ok(AssetKind::Zip)
|
||||
registry_archive_kind_for_url("https://example.com/agent.zip?download=1"),
|
||||
Ok(RegistryArchiveKind::Archive(AssetKind::Zip))
|
||||
));
|
||||
assert!(matches!(
|
||||
asset_kind_for_archive_url("https://example.com/agent.tar.gz"),
|
||||
Ok(AssetKind::TarGz)
|
||||
registry_archive_kind_for_url("https://example.com/agent.tar.gz"),
|
||||
Ok(RegistryArchiveKind::Archive(AssetKind::TarGz))
|
||||
));
|
||||
assert!(matches!(
|
||||
asset_kind_for_archive_url("https://example.com/agent.tar.gz?download=1#latest"),
|
||||
Ok(AssetKind::TarGz)
|
||||
registry_archive_kind_for_url("https://example.com/agent.tar.gz?download=1#latest"),
|
||||
Ok(RegistryArchiveKind::Archive(AssetKind::TarGz))
|
||||
));
|
||||
assert!(matches!(
|
||||
asset_kind_for_archive_url("https://example.com/agent.tgz"),
|
||||
Ok(AssetKind::TarGz)
|
||||
registry_archive_kind_for_url("https://example.com/agent.tgz"),
|
||||
Ok(RegistryArchiveKind::Archive(AssetKind::TarGz))
|
||||
));
|
||||
assert!(matches!(
|
||||
asset_kind_for_archive_url("https://example.com/agent.tgz#download"),
|
||||
Ok(AssetKind::TarGz)
|
||||
registry_archive_kind_for_url("https://example.com/agent.tgz#download"),
|
||||
Ok(RegistryArchiveKind::Archive(AssetKind::TarGz))
|
||||
));
|
||||
assert!(matches!(
|
||||
asset_kind_for_archive_url("https://example.com/agent.tar.bz2"),
|
||||
Ok(AssetKind::TarBz2)
|
||||
registry_archive_kind_for_url("https://example.com/agent.tar.bz2"),
|
||||
Ok(RegistryArchiveKind::Archive(AssetKind::TarBz2))
|
||||
));
|
||||
assert!(matches!(
|
||||
asset_kind_for_archive_url("https://example.com/agent.tar.bz2?download=1"),
|
||||
Ok(AssetKind::TarBz2)
|
||||
registry_archive_kind_for_url("https://example.com/agent.tar.bz2?download=1"),
|
||||
Ok(RegistryArchiveKind::Archive(AssetKind::TarBz2))
|
||||
));
|
||||
assert!(matches!(
|
||||
asset_kind_for_archive_url("https://example.com/agent.tbz2"),
|
||||
Ok(AssetKind::TarBz2)
|
||||
registry_archive_kind_for_url("https://example.com/agent.tbz2"),
|
||||
Ok(RegistryArchiveKind::Archive(AssetKind::TarBz2))
|
||||
));
|
||||
assert!(matches!(
|
||||
asset_kind_for_archive_url("https://example.com/agent.tbz2#download"),
|
||||
Ok(AssetKind::TarBz2)
|
||||
registry_archive_kind_for_url("https://example.com/agent.tbz2#download"),
|
||||
Ok(RegistryArchiveKind::Archive(AssetKind::TarBz2))
|
||||
));
|
||||
assert!(matches!(
|
||||
registry_archive_kind_for_url("https://example.com/agent.ZIP"),
|
||||
Ok(RegistryArchiveKind::Archive(AssetKind::Zip))
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn detects_raw_binary_archive_urls() {
|
||||
assert_eq!(
|
||||
registry_archive_kind_for_url("https://x.ai/cli/grok-0.2.20-macos-aarch64").unwrap(),
|
||||
RegistryArchiveKind::RawBinary {
|
||||
file_name: "grok-0.2.20-macos-aarch64".to_string()
|
||||
},
|
||||
);
|
||||
assert_eq!(
|
||||
registry_archive_kind_for_url("https://x.ai/cli/grok-0.2.20-windows-x86_64.exe")
|
||||
.unwrap(),
|
||||
RegistryArchiveKind::RawBinary {
|
||||
file_name: "grok-0.2.20-windows-x86_64.exe".to_string()
|
||||
},
|
||||
);
|
||||
assert_eq!(
|
||||
registry_archive_kind_for_url("https://example.com/agent-binary?download=1#latest")
|
||||
.unwrap(),
|
||||
RegistryArchiveKind::RawBinary {
|
||||
file_name: "agent-binary".to_string()
|
||||
},
|
||||
);
|
||||
assert_eq!(
|
||||
registry_archive_kind_for_url("https://example.com/agent%20binary").unwrap(),
|
||||
RegistryArchiveKind::RawBinary {
|
||||
file_name: "agent binary".to_string()
|
||||
},
|
||||
);
|
||||
// No file name to install the binary as.
|
||||
assert!(registry_archive_kind_for_url("https://example.com/").is_err());
|
||||
// Percent-decoding must not allow path traversal in the file name.
|
||||
assert!(registry_archive_kind_for_url("https://example.com/a%2F..%2Fevil").is_err());
|
||||
assert!(registry_archive_kind_for_url("https://example.com/%2E%2E").is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -1758,14 +1868,31 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn rejects_unsupported_archive_suffixes() {
|
||||
let error = asset_kind_for_archive_url("https://example.com/agent.tar.xz")
|
||||
let error = registry_archive_kind_for_url("https://example.com/agent.tar.xz")
|
||||
.err()
|
||||
.map(|error| error.to_string());
|
||||
|
||||
assert_eq!(
|
||||
error,
|
||||
Some("unsupported archive type in URL: https://example.com/agent.tar.xz".to_string()),
|
||||
Some(
|
||||
"unsupported archive type .tar.xz in URL: https://example.com/agent.tar.xz"
|
||||
.to_string()
|
||||
),
|
||||
);
|
||||
|
||||
for installer_url in [
|
||||
"https://example.com/agent.dmg",
|
||||
"https://example.com/agent.pkg",
|
||||
"https://example.com/agent.deb",
|
||||
"https://example.com/agent.rpm",
|
||||
"https://example.com/agent.msi",
|
||||
"https://example.com/agent.AppImage",
|
||||
] {
|
||||
assert!(
|
||||
registry_archive_kind_for_url(installer_url).is_err(),
|
||||
"expected {installer_url} to be rejected"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
Reference in new issue
Block a user