mirror of
https://github.com/openai/codex.git
synced 2026-10-18 17:18:52 +00:00
Avoid exposing config values in codex doctor errors (#46962)
## Why Configuration load errors can echo configuration values, including credentials, into diagnostic reports. ## What changed Replace raw error messages in `config.load` failures with typed metadata. Report the file, line, and column when a `ConfigLoadError` is available, including when wrapped in an I/O error. Otherwise, report only the I/O error kind or a generic configuration load failure. ## Testing Add regression tests and snapshots for malformed user and project configuration, an unknown model provider, and invalid header configuration. Verify that both tested JSON reporting modes omit the test credentials while retaining the expected failure details. GitOrigin-RevId: bec53313888dbfc8c0869f3cb410b1c3e5496630
This commit is contained in:
@@ -32,6 +32,7 @@ use codex_api::ApiError;
|
||||
use codex_api::ResponsesWebsocketClient;
|
||||
use codex_api::is_azure_responses_provider;
|
||||
use codex_arg0::Arg0DispatchPaths;
|
||||
use codex_config::ConfigLoadError;
|
||||
use codex_config::types::McpServerConfig;
|
||||
use codex_config::types::McpServerTransportConfig;
|
||||
use codex_core::config::Config;
|
||||
@@ -525,14 +526,38 @@ async fn build_report(
|
||||
) = tokio::join!(
|
||||
async {
|
||||
run_sync_check("config", progress.clone(), || {
|
||||
DoctorCheck::new(
|
||||
let check = DoctorCheck::new(
|
||||
"config.load",
|
||||
"config",
|
||||
CheckStatus::Fail,
|
||||
"config could not be loaded",
|
||||
)
|
||||
.detail(err.to_string())
|
||||
.remediation("Fix the reported config error, then rerun codex doctor.")
|
||||
.remediation("Fix the reported config error, then rerun codex doctor.");
|
||||
// Error messages can echo config values. Report only typed metadata,
|
||||
// including errors wrapped by io::Error, whose source skips the wrapper.
|
||||
let config_error = err.chain().find_map(|cause| {
|
||||
cause.downcast_ref::<ConfigLoadError>().or_else(|| {
|
||||
cause
|
||||
.downcast_ref::<std::io::Error>()?
|
||||
.get_ref()?
|
||||
.downcast_ref::<ConfigLoadError>()
|
||||
})
|
||||
});
|
||||
if let Some(error) = config_error {
|
||||
let error = error.config_error();
|
||||
return check
|
||||
.detail("error: invalid configuration")
|
||||
.detail(format!("file: {}", error.path.display()))
|
||||
.detail(format!("line: {}", error.range.start.line))
|
||||
.detail(format!("column: {}", error.range.start.column));
|
||||
}
|
||||
let io_error = err
|
||||
.chain()
|
||||
.find_map(|cause| cause.downcast_ref::<std::io::Error>());
|
||||
match io_error {
|
||||
Some(error) => check.detail(format!("error: {}", error.kind())),
|
||||
None => check.detail("error: configuration load failed"),
|
||||
}
|
||||
})
|
||||
},
|
||||
async {
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
//! Black-box coverage for diagnostics under a repository-controlled PATH.
|
||||
//! Black-box coverage for safe diagnostic execution and config error reporting.
|
||||
|
||||
use std::ffi::OsString;
|
||||
use std::path::PathBuf;
|
||||
@@ -197,6 +197,79 @@ fn non_interactive_dumb_terminal_preserves_other_doctor_failures() -> Result<()>
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn doctor_reports_only_safe_config_error_metadata() -> Result<()> {
|
||||
let fixture = Fixture::new()?;
|
||||
let user_config_file = fixture.home.join("config.toml");
|
||||
let original = std::fs::read_to_string(&user_config_file)?;
|
||||
let project_key = toml::Value::String(fixture.workspace.display().to_string());
|
||||
let original = format!("{original}\n[projects.{project_key}]\ntrust_level = \"trusted\"\n");
|
||||
let project_config_dir = fixture.workspace.join(".codex");
|
||||
std::fs::create_dir(&project_config_dir)?;
|
||||
for (snapshot_name, config_file, config) in [
|
||||
(
|
||||
"doctor_config_error_location",
|
||||
user_config_file.clone(),
|
||||
"custom_header = \"doctor-test-credential\" trailing\n".to_string(),
|
||||
),
|
||||
(
|
||||
"doctor_config_invalid_data",
|
||||
project_config_dir.join("config.toml"),
|
||||
"custom_header = \"doctor-test-credential\" trailing\n".to_string(),
|
||||
),
|
||||
(
|
||||
"doctor_config_not_found",
|
||||
user_config_file.clone(),
|
||||
original.replace(
|
||||
"model_provider = \"local\"",
|
||||
"model_provider = \"doctor-test-credential\"",
|
||||
),
|
||||
),
|
||||
(
|
||||
"doctor_config_invalid_data",
|
||||
user_config_file.clone(),
|
||||
original.replace(
|
||||
"wire_api = \"responses\"",
|
||||
"wire_api = \"responses\"\nhttp_headers = \"sk-proj-ABC123example\"",
|
||||
),
|
||||
),
|
||||
] {
|
||||
std::fs::write(&user_config_file, &original)?;
|
||||
std::fs::write(&config_file, config)?;
|
||||
for args in [
|
||||
vec!["doctor", "--json"],
|
||||
vec!["doctor", "--json", "--feedback"],
|
||||
] {
|
||||
let output = fixture.command()?.args(args).output()?;
|
||||
assert_eq!(output.status.code(), Some(1));
|
||||
let report: Value = serde_json::from_slice(&output.stdout)?;
|
||||
let mut check = report["checks"]["config.load"].clone();
|
||||
check
|
||||
.as_object_mut()
|
||||
.expect("config check")
|
||||
.remove("durationMs");
|
||||
if let Some(file) = check["details"]["file"].as_str() {
|
||||
check["details"]["file"] = Value::String(
|
||||
file.replace(
|
||||
&fixture.root.path().canonicalize()?.display().to_string(),
|
||||
"FIXTURE",
|
||||
)
|
||||
.replace(&fixture.root.path().display().to_string(), "FIXTURE")
|
||||
.replace('\\', "/"),
|
||||
);
|
||||
}
|
||||
let stdout = String::from_utf8_lossy(&output.stdout);
|
||||
assert!(!stdout.contains("doctor-test-credential"));
|
||||
assert!(!stdout.contains("sk-proj-ABC123example"));
|
||||
// Keep snapshots stable across serde_json feature sets in Cargo and Bazel.
|
||||
check.sort_all_objects();
|
||||
insta::assert_snapshot!(snapshot_name, serde_json::to_string_pretty(&check)?);
|
||||
}
|
||||
std::fs::remove_file(config_file)?;
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn doctor_reports_configured_filesystem_paths() -> Result<()> {
|
||||
let fixture = Fixture::new()?;
|
||||
|
||||
@@ -0,0 +1,17 @@
|
||||
---
|
||||
source: cli/tests/doctor_path_safety.rs
|
||||
expression: "serde_json::to_string_pretty(&check)?"
|
||||
---
|
||||
{
|
||||
"category": "config",
|
||||
"details": {
|
||||
"column": "42",
|
||||
"error": "invalid configuration",
|
||||
"file": "FIXTURE/home/config.toml",
|
||||
"line": "1"
|
||||
},
|
||||
"id": "config.load",
|
||||
"remediation": "Fix the reported config error, then rerun codex doctor.",
|
||||
"status": "fail",
|
||||
"summary": "config could not be loaded"
|
||||
}
|
||||
@@ -0,0 +1,14 @@
|
||||
---
|
||||
source: cli/tests/doctor_path_safety.rs
|
||||
expression: "serde_json::to_string_pretty(&check)?"
|
||||
---
|
||||
{
|
||||
"category": "config",
|
||||
"details": {
|
||||
"error": "invalid data"
|
||||
},
|
||||
"id": "config.load",
|
||||
"remediation": "Fix the reported config error, then rerun codex doctor.",
|
||||
"status": "fail",
|
||||
"summary": "config could not be loaded"
|
||||
}
|
||||
@@ -0,0 +1,14 @@
|
||||
---
|
||||
source: cli/tests/doctor_path_safety.rs
|
||||
expression: "serde_json::to_string_pretty(&check)?"
|
||||
---
|
||||
{
|
||||
"category": "config",
|
||||
"details": {
|
||||
"error": "entity not found"
|
||||
},
|
||||
"id": "config.load",
|
||||
"remediation": "Fix the reported config error, then rerun codex doctor.",
|
||||
"status": "fail",
|
||||
"summary": "config could not be loaded"
|
||||
}
|
||||
Reference in New Issue
Block a user