mirror of
https://github.com/openai/codex.git
synced 2026-09-17 12:23:33 +00:00
Support gRPC code-mode hosts in app server (#38288)
## What changed - Accept root `http://` and `https://` URLs in `--code-mode-host` and use the shared gRPC session provider for those endpoints. - Keep `ws://` and `wss://` URLs on the existing WebSocket transport. - Reject paths, queries, fragments, and credentials where unsupported, without exposing gRPC URL credentials in command-line validation errors. ## Testing - Cover argument parsing and transport selection for both remote protocols. - Exercise a gRPC host shared across app-server threads and verify credential rejection does not disclose usernames or passwords. GitOrigin-RevId: b6516a85cf76db5c4cea620f89ef866d8af30cf0
This commit is contained in:
committed by
copyberry
parent
e766f75989
commit
5104cb649e
@@ -36,7 +36,7 @@ When running with `--listen ws://IP:PORT`, the same listener also serves basic H
|
||||
|
||||
Websocket transport is currently experimental and unsupported. Do not rely on it for production workloads.
|
||||
|
||||
Pass `--code-mode-host wss://HOST/PATH` to connect this app-server process to a remote code-mode host instead of starting a local host. This outbound connection is independent of `--listen` and is shared by the process's threads. Use `ws://` for a local code-mode host.
|
||||
Pass `--code-mode-host URL` to connect this app-server process to a remote code-mode host instead of starting a local host. Use `ws://` or `wss://` for the WebSocket protocol, or a root `http://` or `https://` URL without a path or query for gRPC. Remote hosts require the `code_mode_host` feature. This outbound connection is independent of `--listen` and is shared by the process's threads.
|
||||
|
||||
The unix socket transport is intended for local app-server control-plane clients. `codex app-server proxy`
|
||||
opens exactly one raw stream connection to `$CODEX_HOME/app-server-control/app-server-control.sock`
|
||||
|
||||
@@ -1,4 +1,8 @@
|
||||
use std::ffi::OsStr;
|
||||
|
||||
use clap::Args;
|
||||
use clap::builder::TypedValueParser;
|
||||
use clap::error::ErrorKind;
|
||||
use url::Url;
|
||||
|
||||
/// Selects the code-mode host for a single app-server process.
|
||||
@@ -7,8 +11,8 @@ pub struct AppServerCodeModeHostArgs {
|
||||
/// Connect to a remote code-mode host instead of starting a local host.
|
||||
#[arg(
|
||||
long = "code-mode-host",
|
||||
value_name = "WS_URL",
|
||||
value_parser = parse_websocket_url
|
||||
value_name = "URL",
|
||||
value_parser = RedactedHostUrlParser
|
||||
)]
|
||||
pub code_mode_host: Option<Url>,
|
||||
}
|
||||
@@ -21,25 +25,64 @@ pub enum CodeModeHostTransport {
|
||||
Local,
|
||||
/// Share a connection to the specified remote code-mode host.
|
||||
WebSocket(Url),
|
||||
/// Share an HTTP/2 gRPC connection to the specified remote code-mode host.
|
||||
Grpc(Url),
|
||||
}
|
||||
|
||||
impl From<AppServerCodeModeHostArgs> for CodeModeHostTransport {
|
||||
fn from(args: AppServerCodeModeHostArgs) -> Self {
|
||||
match args.code_mode_host {
|
||||
Some(url) if matches!(url.scheme(), "http" | "https") => Self::Grpc(url),
|
||||
Some(url) => Self::WebSocket(url),
|
||||
None => Self::Local,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn parse_websocket_url(value: &str) -> Result<Url, String> {
|
||||
let url = Url::parse(value).map_err(|error| format!("invalid websocket URL: {error}"))?;
|
||||
if !matches!(url.scheme(), "ws" | "wss") || url.host_str().is_none() {
|
||||
return Err("code-mode host URL must use ws:// or wss:// with a host".to_string());
|
||||
#[derive(Clone)]
|
||||
struct RedactedHostUrlParser;
|
||||
|
||||
impl TypedValueParser for RedactedHostUrlParser {
|
||||
type Value = Url;
|
||||
|
||||
fn parse_ref(
|
||||
&self,
|
||||
command: &clap::Command,
|
||||
_argument: Option<&clap::Arg>,
|
||||
value: &OsStr,
|
||||
) -> Result<Self::Value, clap::Error> {
|
||||
let value = value.to_str().ok_or_else(|| {
|
||||
clap::Error::raw(
|
||||
ErrorKind::InvalidUtf8,
|
||||
"code-mode host URL must contain valid UTF-8",
|
||||
)
|
||||
.with_cmd(command)
|
||||
})?;
|
||||
|
||||
parse_host_url(value)
|
||||
.map_err(|error| clap::Error::raw(ErrorKind::ValueValidation, error).with_cmd(command))
|
||||
}
|
||||
}
|
||||
|
||||
fn parse_host_url(value: &str) -> Result<Url, String> {
|
||||
let url = Url::parse(value).map_err(|error| format!("invalid code-mode host URL: {error}"))?;
|
||||
if !matches!(url.scheme(), "ws" | "wss" | "http" | "https") || url.host_str().is_none() {
|
||||
return Err(
|
||||
"code-mode host URL must use ws://, wss://, http://, or https:// with a host"
|
||||
.to_string(),
|
||||
);
|
||||
}
|
||||
if url.fragment().is_some() {
|
||||
return Err("code-mode host URL must not contain a fragment".to_string());
|
||||
}
|
||||
if matches!(url.scheme(), "http" | "https") {
|
||||
if !url.username().is_empty() || url.password().is_some() {
|
||||
return Err("gRPC code-mode host URL must not contain credentials".to_string());
|
||||
}
|
||||
if url.path() != "/" || url.query().is_some() {
|
||||
return Err("gRPC code-mode host URL must not contain a path or query".to_string());
|
||||
}
|
||||
}
|
||||
Ok(url)
|
||||
}
|
||||
|
||||
|
||||
@@ -1,30 +1,63 @@
|
||||
use super::AppServerCodeModeHostArgs;
|
||||
use super::CodeModeHostTransport;
|
||||
use super::parse_websocket_url;
|
||||
use super::parse_host_url;
|
||||
use pretty_assertions::assert_eq;
|
||||
use url::Url;
|
||||
|
||||
#[test]
|
||||
fn websocket_host_accepts_local_and_secure_endpoints() {
|
||||
for endpoint in ["ws://127.0.0.1:8765", "wss://example.test/code-mode"] {
|
||||
for endpoint in [
|
||||
"ws://127.0.0.1:8765",
|
||||
"wss://example.test/code-mode",
|
||||
"ws://alice:secret@example.test/code-mode",
|
||||
"wss://alice:secret@example.test/code-mode",
|
||||
] {
|
||||
assert_eq!(
|
||||
parse_websocket_url(endpoint),
|
||||
parse_host_url(endpoint),
|
||||
Ok(Url::parse(endpoint).expect("test endpoint should parse"))
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn websocket_host_rejects_invalid_endpoints() {
|
||||
fn grpc_host_accepts_local_and_secure_endpoints() {
|
||||
for endpoint in ["http://127.0.0.1:8765", "https://example.test"] {
|
||||
assert_eq!(
|
||||
parse_host_url(endpoint),
|
||||
Ok(Url::parse(endpoint).expect("test endpoint should parse"))
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn grpc_host_rejects_credentials_without_disclosing_them() {
|
||||
for endpoint in [
|
||||
"http://127.0.0.1:8765",
|
||||
"https://example.test/code-mode",
|
||||
"http://alice:secret@example.test",
|
||||
"https://alice:secret@example.test",
|
||||
"https://alice@example.test",
|
||||
"https://:secret@example.test",
|
||||
] {
|
||||
let error = parse_host_url(endpoint).expect_err("gRPC credentials should be rejected");
|
||||
|
||||
assert!(error.contains("must not contain credentials"));
|
||||
assert!(!error.contains("alice"));
|
||||
assert!(!error.contains("secret"));
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn code_mode_host_rejects_invalid_endpoints() {
|
||||
for endpoint in [
|
||||
"ftp://127.0.0.1:8765",
|
||||
"ws://",
|
||||
"not a websocket",
|
||||
"not a host endpoint",
|
||||
"wss://example.test/code-mode#fragment",
|
||||
"https://example.test/code-mode#fragment",
|
||||
"https://example.test/code-mode",
|
||||
"http://example.test/?token=secret",
|
||||
] {
|
||||
assert!(
|
||||
parse_websocket_url(endpoint).is_err(),
|
||||
parse_host_url(endpoint).is_err(),
|
||||
"invalid code-mode host endpoint should be rejected: {endpoint}"
|
||||
);
|
||||
}
|
||||
@@ -49,3 +82,15 @@ fn explicit_websocket_host_selects_remote_transport() {
|
||||
CodeModeHostTransport::WebSocket(url)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn explicit_grpc_host_selects_remote_transport() {
|
||||
let url = Url::parse("https://example.test").expect("test endpoint should parse");
|
||||
|
||||
assert_eq!(
|
||||
CodeModeHostTransport::from(AppServerCodeModeHostArgs {
|
||||
code_mode_host: Some(url.clone()),
|
||||
}),
|
||||
CodeModeHostTransport::Grpc(url)
|
||||
);
|
||||
}
|
||||
|
||||
@@ -3,6 +3,7 @@
|
||||
|
||||
use codex_arg0::Arg0DispatchPaths;
|
||||
use codex_code_mode::CodeModeSessionProvider;
|
||||
use codex_code_mode::GrpcCodeModeSessionProvider;
|
||||
use codex_code_mode::WebSocketCodeModeSessionProvider;
|
||||
use codex_config::LoaderOverrides;
|
||||
use codex_config::NoopThreadConfigLoader;
|
||||
@@ -558,6 +559,20 @@ pub async fn run_main_with_transport_options(
|
||||
),
|
||||
))
|
||||
}
|
||||
CodeModeHostTransport::Grpc(url) => {
|
||||
if !config.features.enabled(Feature::CodeModeHost) {
|
||||
return Err(std::io::Error::new(
|
||||
ErrorKind::InvalidInput,
|
||||
"remote code-mode host requires the code_mode_host feature to be enabled",
|
||||
));
|
||||
}
|
||||
Some(Arc::new(
|
||||
GrpcCodeModeSessionProvider::with_http_client_factory(
|
||||
url.to_string(),
|
||||
config.http_client_factory(),
|
||||
),
|
||||
))
|
||||
}
|
||||
};
|
||||
let environment_manager = if ignore_user_config {
|
||||
EnvironmentManager::from_env(Some(local_runtime_paths), config.http_client_factory()).await
|
||||
|
||||
@@ -57,17 +57,42 @@ fn app_server_accepts_process_scoped_code_mode_host() {
|
||||
assert_eq!(args.config_overrides.raw_overrides, Vec::<String>::new());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn app_server_accepts_process_scoped_grpc_code_mode_host() {
|
||||
let args = AppServerArgs::try_parse_from([
|
||||
"codex-app-server",
|
||||
"--code-mode-host",
|
||||
"https://example.test",
|
||||
"--listen",
|
||||
"off",
|
||||
])
|
||||
.expect("parse gRPC app-server args");
|
||||
|
||||
assert_eq!(
|
||||
args.code_mode_host.code_mode_host,
|
||||
Some(Url::parse("https://example.test").expect("test endpoint should parse"))
|
||||
);
|
||||
assert_eq!(args.listen, AppServerTransport::Off);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn app_server_rejects_invalid_code_mode_host() {
|
||||
for endpoint in [
|
||||
"http://127.0.0.1:8765",
|
||||
"ftp://127.0.0.1:8765",
|
||||
"ws://",
|
||||
"wss://example.test/code-mode#fragment",
|
||||
"https://example.test/code-mode",
|
||||
"http://alice:secret@example.test",
|
||||
"https://alice:secret@example.test",
|
||||
"http://example.test/?token=secret",
|
||||
] {
|
||||
let error =
|
||||
AppServerArgs::try_parse_from(["codex-app-server", "--code-mode-host", endpoint])
|
||||
.expect_err("invalid code-mode host endpoint should fail startup argument parsing");
|
||||
|
||||
assert_eq!(error.kind(), clap::error::ErrorKind::ValueValidation);
|
||||
let rendered_error = error.to_string();
|
||||
assert!(!rendered_error.contains("alice"));
|
||||
assert!(!rendered_error.contains("secret"));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -26,24 +26,33 @@ const DEFAULT_READ_TIMEOUT: Duration = Duration::from_secs(/*secs*/ 10);
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn app_server_shares_flag_selected_code_mode_host_across_threads() -> Result<()> {
|
||||
assert_shared_remote_code_mode_host("ws://127.0.0.1:0").await
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn app_server_shares_flag_selected_grpc_code_mode_host_across_threads() -> Result<()> {
|
||||
assert_shared_remote_code_mode_host("grpc://127.0.0.1:0").await
|
||||
}
|
||||
|
||||
async fn assert_shared_remote_code_mode_host(listen_url: &str) -> Result<()> {
|
||||
let host_program = codex_utils_cargo_bin::cargo_bin("codex-code-mode-host")?;
|
||||
let mut websocket_host = Command::new(host_program)
|
||||
.args(["--listen", "ws://127.0.0.1:0"])
|
||||
let mut code_mode_host = Command::new(host_program)
|
||||
.args(["--listen", listen_url])
|
||||
.stdin(Stdio::null())
|
||||
.stdout(Stdio::piped())
|
||||
.stderr(Stdio::inherit())
|
||||
.kill_on_drop(true)
|
||||
.spawn()
|
||||
.context("failed to start websocket code-mode host")?;
|
||||
let stdout = websocket_host
|
||||
.context("failed to start remote code-mode host")?;
|
||||
let stdout = code_mode_host
|
||||
.stdout
|
||||
.take()
|
||||
.context("websocket code-mode host stdout was not captured")?;
|
||||
.context("remote code-mode host stdout was not captured")?;
|
||||
let mut lines = BufReader::new(stdout).lines();
|
||||
let websocket_url = timeout(DEFAULT_READ_TIMEOUT, lines.next_line())
|
||||
let host_url = timeout(DEFAULT_READ_TIMEOUT, lines.next_line())
|
||||
.await
|
||||
.context("timed out waiting for websocket code-mode host URL")??
|
||||
.context("websocket code-mode host exited before publishing its URL")?;
|
||||
.context("timed out waiting for remote code-mode host URL")??
|
||||
.context("remote code-mode host exited before publishing its URL")?;
|
||||
|
||||
let model_server = responses::start_mock_server().await;
|
||||
let response_mock = responses::mount_sse_sequence(
|
||||
@@ -86,7 +95,7 @@ async fn app_server_shares_flag_selected_code_mode_host_across_threads() -> Resu
|
||||
let original_config = std::fs::read_to_string(codex_home.path().join("config.toml"))?;
|
||||
let mut app_server = TestAppServer::builder()
|
||||
.with_codex_home(codex_home.path())
|
||||
.with_args(&["--code-mode-host", &websocket_url])
|
||||
.with_args(&["--code-mode-host", &host_url])
|
||||
.build_initialized_with_timeout(DEFAULT_READ_TIMEOUT)
|
||||
.await?;
|
||||
|
||||
|
||||
@@ -4070,18 +4070,45 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn app_server_grpc_code_mode_host_url_parses_independently_of_listen_transport() {
|
||||
let app_server = app_server_from_args(
|
||||
[
|
||||
"codex",
|
||||
"app-server",
|
||||
"--code-mode-host",
|
||||
"https://example.test",
|
||||
"--listen",
|
||||
"ws://127.0.0.1:4500",
|
||||
]
|
||||
.as_ref(),
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
app_server.code_mode_host.code_mode_host,
|
||||
Some(url::Url::parse("https://example.test").expect("test endpoint should parse"))
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn app_server_rejects_invalid_code_mode_host_urls() {
|
||||
for endpoint in [
|
||||
"http://127.0.0.1:8765",
|
||||
"ftp://127.0.0.1:8765",
|
||||
"ws://",
|
||||
"wss://example.test/code-mode#fragment",
|
||||
"https://example.test/code-mode",
|
||||
"http://alice:secret@example.test",
|
||||
"https://alice:secret@example.test",
|
||||
"http://example.test/?token=secret",
|
||||
] {
|
||||
let error =
|
||||
MultitoolCli::try_parse_from(["codex", "app-server", "--code-mode-host", endpoint])
|
||||
.expect_err("invalid code-mode host endpoint should fail argument parsing");
|
||||
|
||||
assert_eq!(error.kind(), clap::error::ErrorKind::ValueValidation);
|
||||
let rendered_error = error.to_string();
|
||||
assert!(!rendered_error.contains("alice"));
|
||||
assert!(!rendered_error.contains("secret"));
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -175,6 +175,27 @@ async fn start_active_wait(
|
||||
Ok(wait)
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn grpc_endpoints_reject_credentials_without_disclosing_them() {
|
||||
for endpoint in [
|
||||
"http://alice:secret@host.example",
|
||||
"https://alice:secret@host.example",
|
||||
"https://alice@host.example",
|
||||
"https://:secret@host.example",
|
||||
] {
|
||||
let provider = GrpcCodeModeSessionProvider::new(endpoint);
|
||||
let error = provider
|
||||
.create_session(Arc::new(NoopCodeModeSessionDelegate))
|
||||
.await
|
||||
.err()
|
||||
.expect("gRPC credentials should be rejected");
|
||||
|
||||
assert!(error.contains("must not include credentials"));
|
||||
assert!(!error.contains("alice"));
|
||||
assert!(!error.contains("secret"));
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn tcp_session_persists_values_and_forwards_tools_notifications_and_closure() -> Result<()> {
|
||||
let host = HostHarness::start("grpc://127.0.0.1:0").await?;
|
||||
|
||||
@@ -72,6 +72,11 @@ impl SharedTransport {
|
||||
if !matches!(target.scheme(), "http" | "https") {
|
||||
return Err("gRPC code-mode host URL must use http or https".to_string());
|
||||
}
|
||||
if !target.username().is_empty() || target.password().is_some() {
|
||||
return Err(
|
||||
"gRPC code-mode host URL must not include credentials".to_string(),
|
||||
);
|
||||
}
|
||||
if target.path() != "/"
|
||||
|| target.query().is_some()
|
||||
|| target.fragment().is_some()
|
||||
|
||||
Reference in New Issue
Block a user