mirror of
https://github.com/openai/codex.git
synced 2026-11-03 19:53:25 +00:00
login: use route-aware HTTP client abstraction
This commit is contained in:
Generated
+2
@@ -3350,6 +3350,7 @@ dependencies = [
|
||||
"codex-terminal-detection",
|
||||
"codex-utils-template",
|
||||
"core_test_support",
|
||||
"http 1.4.0",
|
||||
"jsonwebtoken",
|
||||
"keyring",
|
||||
"once_cell",
|
||||
@@ -3367,6 +3368,7 @@ dependencies = [
|
||||
"tiny_http",
|
||||
"tokio",
|
||||
"tracing",
|
||||
"tracing-subscriber",
|
||||
"url",
|
||||
"urlencoding",
|
||||
"webbrowser",
|
||||
|
||||
@@ -16,11 +16,26 @@ use tracing_opentelemetry::OpenTelemetrySpanExt;
|
||||
#[derive(Clone, Debug)]
|
||||
pub struct HttpClient {
|
||||
inner: reqwest::Client,
|
||||
request_logging: RequestLogging,
|
||||
}
|
||||
|
||||
impl HttpClient {
|
||||
pub fn new(inner: reqwest::Client) -> Self {
|
||||
Self { inner }
|
||||
Self {
|
||||
inner,
|
||||
request_logging: RequestLogging::Enabled,
|
||||
}
|
||||
}
|
||||
|
||||
/// Creates a client that suppresses request URL and response-header diagnostics.
|
||||
///
|
||||
/// Use this for authentication endpoints whose URLs or headers may contain credentials that
|
||||
/// are redacted by the caller above the HTTP transport boundary.
|
||||
pub(crate) fn new_without_request_logging(inner: reqwest::Client) -> Self {
|
||||
Self {
|
||||
inner,
|
||||
request_logging: RequestLogging::Disabled,
|
||||
}
|
||||
}
|
||||
|
||||
pub fn get<U>(&self, url: U) -> RequestBuilder
|
||||
@@ -42,24 +57,42 @@ impl HttpClient {
|
||||
U: IntoUrl,
|
||||
{
|
||||
let url_str = url.as_str().to_string();
|
||||
RequestBuilder::new(self.inner.request(method.clone(), url), method, url_str)
|
||||
RequestBuilder::new(
|
||||
self.inner.request(method.clone(), url),
|
||||
method,
|
||||
url_str,
|
||||
self.request_logging,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
|
||||
enum RequestLogging {
|
||||
Enabled,
|
||||
Disabled,
|
||||
}
|
||||
|
||||
#[must_use = "requests are not sent unless `send` is awaited"]
|
||||
#[derive(Debug)]
|
||||
pub struct RequestBuilder {
|
||||
builder: reqwest::RequestBuilder,
|
||||
method: Method,
|
||||
url: String,
|
||||
request_logging: RequestLogging,
|
||||
}
|
||||
|
||||
impl RequestBuilder {
|
||||
fn new(builder: reqwest::RequestBuilder, method: Method, url: String) -> Self {
|
||||
fn new(
|
||||
builder: reqwest::RequestBuilder,
|
||||
method: Method,
|
||||
url: String,
|
||||
request_logging: RequestLogging,
|
||||
) -> Self {
|
||||
Self {
|
||||
builder,
|
||||
method,
|
||||
url,
|
||||
request_logging,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -68,6 +101,7 @@ impl RequestBuilder {
|
||||
builder: f(self.builder),
|
||||
method: self.method,
|
||||
url: self.url,
|
||||
request_logging: self.request_logging,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -115,26 +149,30 @@ impl RequestBuilder {
|
||||
|
||||
match self.builder.headers(headers).send().await {
|
||||
Ok(response) => {
|
||||
tracing::debug!(
|
||||
method = %self.method,
|
||||
url = %self.url,
|
||||
status = %response.status(),
|
||||
headers = ?response.headers(),
|
||||
version = ?response.version(),
|
||||
"Request completed"
|
||||
);
|
||||
if self.request_logging == RequestLogging::Enabled {
|
||||
tracing::debug!(
|
||||
method = %self.method,
|
||||
url = %self.url,
|
||||
status = %response.status(),
|
||||
headers = ?response.headers(),
|
||||
version = ?response.version(),
|
||||
"Request completed"
|
||||
);
|
||||
}
|
||||
|
||||
Ok(response)
|
||||
}
|
||||
Err(error) => {
|
||||
let status = error.status();
|
||||
tracing::debug!(
|
||||
method = %self.method,
|
||||
url = %self.url,
|
||||
status = status.map(|s| s.as_u16()),
|
||||
error = %error,
|
||||
"Request failed"
|
||||
);
|
||||
if self.request_logging == RequestLogging::Enabled {
|
||||
let status = error.status();
|
||||
tracing::debug!(
|
||||
method = %self.method,
|
||||
url = %self.url,
|
||||
status = status.map(|s| s.as_u16()),
|
||||
error = %error,
|
||||
"Request failed"
|
||||
);
|
||||
}
|
||||
Err(error)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -15,6 +15,7 @@ use std::time::Instant;
|
||||
|
||||
use crate::custom_ca::BuildCustomCaTransportError;
|
||||
use crate::custom_ca::build_reqwest_client_with_custom_ca;
|
||||
use crate::default_client::HttpClient;
|
||||
#[cfg(any(target_os = "windows", target_os = "macos"))]
|
||||
use sha2::Digest;
|
||||
#[cfg(any(target_os = "windows", target_os = "macos"))]
|
||||
@@ -158,6 +159,26 @@ impl HttpClientFactory {
|
||||
)
|
||||
}
|
||||
|
||||
/// Builds an HTTP client for a concrete outbound route.
|
||||
pub fn build_client(
|
||||
&self,
|
||||
request_url: &str,
|
||||
route_class: ClientRouteClass,
|
||||
) -> Result<HttpClient, BuildRouteAwareHttpClientError> {
|
||||
self.build_reqwest_client(reqwest::Client::builder(), request_url, route_class)
|
||||
.map(HttpClient::new)
|
||||
}
|
||||
|
||||
/// Builds a route-aware client without request URL or response-header diagnostics.
|
||||
pub fn build_client_without_request_logging(
|
||||
&self,
|
||||
request_url: &str,
|
||||
route_class: ClientRouteClass,
|
||||
) -> Result<HttpClient, BuildRouteAwareHttpClientError> {
|
||||
self.build_reqwest_client(reqwest::Client::builder(), request_url, route_class)
|
||||
.map(HttpClient::new_without_request_logging)
|
||||
}
|
||||
|
||||
/// Builds a reqwest client for a concrete outbound route.
|
||||
pub fn build_reqwest_client(
|
||||
&self,
|
||||
|
||||
@@ -20,6 +20,7 @@ codex-protocol = { workspace = true }
|
||||
codex-secrets = { workspace = true }
|
||||
codex-terminal-detection = { workspace = true }
|
||||
codex-utils-template = { workspace = true }
|
||||
http = { workspace = true }
|
||||
once_cell = { workspace = true }
|
||||
os_info = { workspace = true }
|
||||
rand = { workspace = true }
|
||||
@@ -50,6 +51,7 @@ pretty_assertions = { workspace = true }
|
||||
regex-lite = { workspace = true }
|
||||
serial_test = { workspace = true }
|
||||
tempfile = { workspace = true }
|
||||
tracing-subscriber = { workspace = true }
|
||||
wiremock = { workspace = true }
|
||||
|
||||
[lib]
|
||||
|
||||
@@ -295,16 +295,13 @@ fn default_reqwest_client_builder() -> reqwest::ClientBuilder {
|
||||
with_chatgpt_cloudflare_cookie_store(builder)
|
||||
}
|
||||
|
||||
/// Builds a raw reqwest client for an auth endpoint without Codex default headers.
|
||||
pub(crate) fn build_raw_auth_reqwest_client(
|
||||
/// Builds an HTTP client for an auth endpoint without Codex default headers.
|
||||
pub(crate) fn create_raw_auth_client(
|
||||
endpoint: &str,
|
||||
auth_route_config: Option<&AuthRouteConfig>,
|
||||
) -> Result<reqwest::Client, BuildRouteAwareHttpClientError> {
|
||||
auth_http_client_factory(auth_route_config).build_reqwest_client(
|
||||
reqwest::Client::builder(),
|
||||
endpoint,
|
||||
ClientRouteClass::Auth,
|
||||
)
|
||||
) -> Result<HttpClient, BuildRouteAwareHttpClientError> {
|
||||
auth_http_client_factory(auth_route_config)
|
||||
.build_client_without_request_logging(endpoint, ClientRouteClass::Auth)
|
||||
}
|
||||
|
||||
/// Builds the default Codex reqwest client for an auth endpoint.
|
||||
|
||||
@@ -2,6 +2,41 @@ use super::sanitize_user_agent;
|
||||
use super::*;
|
||||
use core_test_support::skip_if_no_network;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::io;
|
||||
use std::io::Write;
|
||||
use std::sync::Arc;
|
||||
use std::sync::Mutex;
|
||||
use tracing_subscriber::layer::SubscriberExt;
|
||||
|
||||
#[derive(Clone)]
|
||||
struct TestLogWriter {
|
||||
buffer: Arc<Mutex<Vec<u8>>>,
|
||||
}
|
||||
|
||||
struct TestLogSink {
|
||||
buffer: Arc<Mutex<Vec<u8>>>,
|
||||
}
|
||||
|
||||
impl<'a> tracing_subscriber::fmt::MakeWriter<'a> for TestLogWriter {
|
||||
type Writer = TestLogSink;
|
||||
|
||||
fn make_writer(&'a self) -> Self::Writer {
|
||||
TestLogSink {
|
||||
buffer: Arc::clone(&self.buffer),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl Write for TestLogSink {
|
||||
fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
|
||||
self.buffer.lock().expect("log buffer lock").extend(buf);
|
||||
Ok(buf.len())
|
||||
}
|
||||
|
||||
fn flush(&mut self) -> io::Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_get_codex_user_agent() {
|
||||
@@ -89,6 +124,89 @@ async fn test_create_client_sets_default_headers() {
|
||||
set_default_client_residency_requirement(/*enforce_residency*/ None);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn raw_auth_client_does_not_log_sensitive_request_or_response_data() {
|
||||
use wiremock::Mock;
|
||||
use wiremock::MockServer;
|
||||
use wiremock::ResponseTemplate;
|
||||
use wiremock::matchers::method;
|
||||
use wiremock::matchers::path;
|
||||
|
||||
let server = MockServer::start().await;
|
||||
Mock::given(method("POST"))
|
||||
.and(path("/token"))
|
||||
.respond_with(
|
||||
ResponseTemplate::new(200)
|
||||
.insert_header("x-sensitive-response", "response-secret-value"),
|
||||
)
|
||||
.expect(1)
|
||||
.mount(&server)
|
||||
.await;
|
||||
let authority = server
|
||||
.uri()
|
||||
.strip_prefix("http://")
|
||||
.expect("wiremock URI should use HTTP")
|
||||
.to_string();
|
||||
let endpoint = format!(
|
||||
"http://auth-user:password-secret-value@{authority}/token?client_secret=query-secret-value"
|
||||
);
|
||||
let client = create_raw_auth_client(&endpoint, /*auth_route_config*/ None)
|
||||
.expect("raw auth client should build");
|
||||
let buffer = Arc::new(Mutex::new(Vec::new()));
|
||||
let subscriber = tracing_subscriber::registry().with(
|
||||
tracing_subscriber::fmt::layer()
|
||||
.with_ansi(false)
|
||||
.with_writer(TestLogWriter {
|
||||
buffer: Arc::clone(&buffer),
|
||||
}),
|
||||
);
|
||||
let _guard = tracing::subscriber::set_default(subscriber);
|
||||
tracing::debug!("log capture sentinel");
|
||||
|
||||
let response = client
|
||||
.post(&endpoint)
|
||||
.header("x-sensitive-request", "request-header-secret-value")
|
||||
.body("request-body-secret-value")
|
||||
.send()
|
||||
.await
|
||||
.expect("raw auth request should succeed");
|
||||
assert!(response.status().is_success());
|
||||
|
||||
let unresponsive_listener = std::net::TcpListener::bind("127.0.0.1:0")
|
||||
.expect("unresponsive local listener should bind");
|
||||
let unresponsive_addr = unresponsive_listener
|
||||
.local_addr()
|
||||
.expect("unresponsive local address should be available");
|
||||
let unresponsive_endpoint = format!(
|
||||
"http://auth-user:failure-password-secret-value@{unresponsive_addr}/token?client_secret=failure-query-secret-value"
|
||||
);
|
||||
let unresponsive_client =
|
||||
create_raw_auth_client(&unresponsive_endpoint, /*auth_route_config*/ None)
|
||||
.expect("raw auth client should build");
|
||||
let error = unresponsive_client
|
||||
.post(&unresponsive_endpoint)
|
||||
.header("x-sensitive-request", "failure-request-header-secret-value")
|
||||
.body("failure-request-body-secret-value")
|
||||
.timeout(std::time::Duration::from_secs(1))
|
||||
.send()
|
||||
.await
|
||||
.expect_err("request to an unresponsive local listener should time out");
|
||||
assert!(error.is_timeout());
|
||||
|
||||
let logs = String::from_utf8(buffer.lock().expect("log buffer lock").clone())
|
||||
.expect("logs should be UTF-8");
|
||||
assert!(logs.contains("log capture sentinel"));
|
||||
assert!(!logs.contains("password-secret-value"));
|
||||
assert!(!logs.contains("query-secret-value"));
|
||||
assert!(!logs.contains("request-header-secret-value"));
|
||||
assert!(!logs.contains("request-body-secret-value"));
|
||||
assert!(!logs.contains("response-secret-value"));
|
||||
assert!(!logs.contains("failure-password-secret-value"));
|
||||
assert!(!logs.contains("failure-query-secret-value"));
|
||||
assert!(!logs.contains("failure-request-header-secret-value"));
|
||||
assert!(!logs.contains("failure-request-body-secret-value"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_invalid_suffix_is_sanitized() {
|
||||
let prefix = "codex_cli_rs/0.0.0";
|
||||
|
||||
@@ -155,6 +155,9 @@ fn derive_revoke_token_endpoint(refresh_endpoint: &str) -> Option<String> {
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use codex_http_client::ClientRouteClass;
|
||||
use codex_http_client::HttpClientFactory;
|
||||
use codex_http_client::OutboundProxyPolicy;
|
||||
use core_test_support::skip_if_no_network;
|
||||
use wiremock::Mock;
|
||||
use wiremock::MockServer;
|
||||
@@ -181,8 +184,10 @@ mod tests {
|
||||
.mount(&server)
|
||||
.await;
|
||||
|
||||
let client = HttpClient::new(reqwest::Client::new());
|
||||
let endpoint = format!("{}/oauth/revoke", server.uri());
|
||||
let client = HttpClientFactory::new(OutboundProxyPolicy::ReqwestDefault)
|
||||
.build_client(&endpoint, ClientRouteClass::Auth)
|
||||
.expect("test HTTP client should build");
|
||||
let error = revoke_oauth_token(
|
||||
&client,
|
||||
endpoint.as_str(),
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
use reqwest::StatusCode;
|
||||
use codex_http_client::HttpClient;
|
||||
use http::StatusCode;
|
||||
use serde::Deserialize;
|
||||
use serde::Serialize;
|
||||
use serde::de::Deserializer;
|
||||
@@ -6,7 +7,7 @@ use serde::de::{self};
|
||||
use std::time::Duration;
|
||||
use std::time::Instant;
|
||||
|
||||
use crate::default_client::build_raw_auth_reqwest_client;
|
||||
use crate::default_client::create_raw_auth_client;
|
||||
use crate::pkce::PkceCodes;
|
||||
use crate::server::ServerOptions;
|
||||
use std::io;
|
||||
@@ -60,7 +61,7 @@ struct CodeSuccessResp {
|
||||
|
||||
/// Request the user code and polling interval.
|
||||
async fn request_user_code(
|
||||
client: &reqwest::Client,
|
||||
client: &HttpClient,
|
||||
auth_base_url: &str,
|
||||
client_id: &str,
|
||||
) -> std::io::Result<UserCodeResp> {
|
||||
@@ -97,7 +98,7 @@ async fn request_user_code(
|
||||
|
||||
/// Poll token endpoint until a code is issued or timeout occurs.
|
||||
async fn poll_for_token(
|
||||
client: &reqwest::Client,
|
||||
client: &HttpClient,
|
||||
auth_base_url: &str,
|
||||
device_auth_id: &str,
|
||||
user_code: &str,
|
||||
@@ -165,7 +166,7 @@ pub async fn request_device_code(opts: &ServerOptions) -> std::io::Result<Device
|
||||
let base_url = opts.issuer.trim_end_matches('/');
|
||||
// The route selected for the issuer is reused for all device-auth endpoint paths; the endpoint
|
||||
// paths are not resolved separately.
|
||||
let client = build_raw_auth_reqwest_client(base_url, opts.auth_route_config.as_ref())?;
|
||||
let client = create_raw_auth_client(base_url, opts.auth_route_config.as_ref())?;
|
||||
let api_base_url = format!("{base_url}/api/accounts");
|
||||
let uc = request_user_code(&client, &api_base_url, &opts.client_id).await?;
|
||||
|
||||
@@ -182,7 +183,7 @@ pub async fn complete_device_code_login(
|
||||
device_code: DeviceCode,
|
||||
) -> std::io::Result<()> {
|
||||
let base_url = opts.issuer.trim_end_matches('/');
|
||||
let client = build_raw_auth_reqwest_client(base_url, opts.auth_route_config.as_ref())?;
|
||||
let client = create_raw_auth_client(base_url, opts.auth_route_config.as_ref())?;
|
||||
let api_base_url = format!("{base_url}/api/accounts");
|
||||
|
||||
let code_resp = poll_for_token(
|
||||
|
||||
@@ -27,7 +27,7 @@ use std::time::Duration;
|
||||
use crate::auth::AuthDotJson;
|
||||
use crate::auth::AuthKeyringBackendKind;
|
||||
use crate::auth::save_auth;
|
||||
use crate::default_client::build_raw_auth_reqwest_client;
|
||||
use crate::default_client::create_raw_auth_client;
|
||||
use crate::default_client::originator;
|
||||
use crate::outbound_proxy::AuthRouteConfig;
|
||||
use crate::pkce::PkceCodes;
|
||||
@@ -798,7 +798,7 @@ pub(crate) async fn exchange_code_for_tokens(
|
||||
|
||||
// The route selected for the issuer is reused for token exchange; the token endpoint path is
|
||||
// not resolved separately.
|
||||
let client = build_raw_auth_reqwest_client(issuer.trim_end_matches('/'), auth_route_config)?;
|
||||
let client = create_raw_auth_client(issuer.trim_end_matches('/'), auth_route_config)?;
|
||||
let token_endpoint = format!("{}/oauth/token", issuer.trim_end_matches('/'));
|
||||
info!(
|
||||
issuer = %sanitize_url_for_logging(issuer),
|
||||
@@ -1120,7 +1120,7 @@ pub(crate) async fn obtain_api_key(
|
||||
access_token: String,
|
||||
}
|
||||
let token_endpoint = format!("{}/oauth/token", issuer.trim_end_matches('/'));
|
||||
let client = build_raw_auth_reqwest_client(&token_endpoint, auth_route_config)?;
|
||||
let client = create_raw_auth_client(&token_endpoint, auth_route_config)?;
|
||||
let resp = client
|
||||
.post(token_endpoint)
|
||||
.header("Content-Type", "application/x-www-form-urlencoded")
|
||||
|
||||
Reference in New Issue
Block a user