mirror of
https://github.com/openai/codex.git
synced 2026-09-17 12:23:33 +00:00
Reject paths in project documentation fallback filenames (#45865)
## Why Probing Windows network paths can send ambient credentials, even during metadata checks. Entries in `project_doc_fallback_filenames` must be validated before filesystem probes. ## What changed Ignore entries containing path syntax according to the executor's OS, rather than the host's. Reject `.` and `..`, slashes, and NUL characters on all executors; also reject backslashes and colons on Windows. Log a warning for rejected entries and continue considering valid filenames. ## Testing Add tests that verify invalid entries never reach metadata probes under POSIX and Windows path conventions, preserve backslashes and colons in POSIX filenames, and confirm valid fallback instructions still load alongside invalid entries. GitOrigin-RevId: 4369e9b97ae6a3560d09e79efd8c85adac3d1d90
This commit is contained in:
@@ -2,6 +2,8 @@
|
||||
//!
|
||||
//! Project-level documentation is primarily stored in files named `AGENTS.md`.
|
||||
//! Additional fallback filenames can be configured via `project_doc_fallback_filenames`.
|
||||
//! Fallback entries containing path syntax for the executor's OS are ignored
|
||||
//! before any filesystem probes use them.
|
||||
//! We include the concatenation of all files found along the path from the
|
||||
//! project root to the current working directory as follows:
|
||||
//!
|
||||
@@ -30,6 +32,7 @@ use codex_file_system::FileSystemSandboxContext;
|
||||
use codex_file_system::FindUpErrorPolicy;
|
||||
use codex_file_system::find_nearest_ancestor_with_markers;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_path_uri::PathConvention;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use futures::StreamExt;
|
||||
use std::io;
|
||||
@@ -236,7 +239,7 @@ async fn agents_md_paths(
|
||||
vec![dir]
|
||||
};
|
||||
|
||||
let candidate_filenames = candidate_filenames(config);
|
||||
let candidate_filenames = candidate_filenames(config, cwd);
|
||||
let candidate_filenames = &candidate_filenames;
|
||||
let mut results = futures::stream::iter(search_dirs)
|
||||
.map(|directory| async move {
|
||||
@@ -266,7 +269,7 @@ async fn agents_md_paths(
|
||||
Ok(found)
|
||||
}
|
||||
|
||||
fn candidate_filenames(config: &Config) -> Vec<&str> {
|
||||
fn candidate_filenames<'a>(config: &'a Config, cwd: &PathUri) -> Vec<&'a str> {
|
||||
let mut names: Vec<&str> = Vec::with_capacity(2 + config.project_doc_fallback_filenames.len());
|
||||
names.push(LOCAL_AGENTS_MD_FILENAME);
|
||||
names.push(DEFAULT_AGENTS_MD_FILENAME);
|
||||
@@ -275,6 +278,16 @@ fn candidate_filenames(config: &Config) -> Vec<&str> {
|
||||
if candidate.is_empty() {
|
||||
continue;
|
||||
}
|
||||
// Use the executor's path convention, not the host's: resolving a Windows
|
||||
// network path can send ambient credentials even during metadata probes.
|
||||
if matches!(candidate, "." | "..")
|
||||
|| candidate.contains(['/', '\0'])
|
||||
|| cwd.infer_path_convention() == Some(PathConvention::Windows)
|
||||
&& candidate.contains(['\\', ':'])
|
||||
{
|
||||
tracing::warn!("ignoring project_doc_fallback_filenames entry that is not a filename");
|
||||
continue;
|
||||
}
|
||||
if !names.contains(&candidate) {
|
||||
names.push(candidate);
|
||||
}
|
||||
|
||||
@@ -51,6 +51,7 @@ use tokio::sync::Semaphore;
|
||||
|
||||
#[derive(Clone, Copy)]
|
||||
enum InjectedFailure {
|
||||
MetadataNotFound,
|
||||
Metadata(io::ErrorKind),
|
||||
MetadataBlocked,
|
||||
MetadataBlockedByFilenamePrefix(&'static str),
|
||||
@@ -128,13 +129,16 @@ impl FailingFileSystem {
|
||||
options: GetMetadataOptions,
|
||||
sandbox: Option<&FileSystemSandboxContext>,
|
||||
) -> io::Result<FileMetadata> {
|
||||
let path_abs = path.to_abs_path()?;
|
||||
self.metadata_calls
|
||||
.paths
|
||||
.lock()
|
||||
.expect("metadata paths lock")
|
||||
.push(path.clone());
|
||||
self.metadata_calls.started.notify_one();
|
||||
if matches!(self.failure, InjectedFailure::MetadataNotFound) {
|
||||
return Err(io::Error::from(io::ErrorKind::NotFound));
|
||||
}
|
||||
let path_abs = path.to_abs_path()?;
|
||||
match self.failure {
|
||||
InjectedFailure::Metadata(kind) if path_abs == self.path => {
|
||||
Err(io::Error::new(kind, "injected metadata failure"))
|
||||
@@ -165,7 +169,8 @@ impl FailingFileSystem {
|
||||
InjectedFailure::MetadataPending if path_abs == self.path => {
|
||||
std::future::pending().await
|
||||
}
|
||||
InjectedFailure::Metadata(_)
|
||||
InjectedFailure::MetadataNotFound
|
||||
| InjectedFailure::Metadata(_)
|
||||
| InjectedFailure::MetadataBlocked
|
||||
| InjectedFailure::MetadataBlockedByFilenamePrefix(_)
|
||||
| InjectedFailure::MetadataPending
|
||||
@@ -1566,6 +1571,72 @@ async fn agents_local_md_preferred() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn fallback_paths_are_rejected_before_filesystem_probes() {
|
||||
let tmp = tempfile::tempdir().expect("tempdir");
|
||||
let mut cfg = make_config_with_project_root_markers(
|
||||
&tmp,
|
||||
/*limit*/ 4096,
|
||||
/*instructions*/ None,
|
||||
&[],
|
||||
)
|
||||
.await;
|
||||
let windows_paths = [
|
||||
r"..\AGENTS.md",
|
||||
r"nested\AGENTS.md",
|
||||
r"\AGENTS.md",
|
||||
r"C:\AGENTS.md",
|
||||
"C:AGENTS.md",
|
||||
r"\\server\share\AGENTS.md",
|
||||
r"\\?\UNC\server\share\AGENTS.md",
|
||||
r"\\.\pipe\instructions",
|
||||
"AGENTS.md:stream",
|
||||
];
|
||||
cfg.project_doc_fallback_filenames = [
|
||||
"",
|
||||
".",
|
||||
"..",
|
||||
"/AGENTS.md",
|
||||
"../AGENTS.md",
|
||||
"nested/AGENTS.md",
|
||||
"//server/share/AGENTS.md",
|
||||
"AGENTS\0.md",
|
||||
]
|
||||
.into_iter()
|
||||
.chain(windows_paths)
|
||||
.chain(["WORKFLOW.md", "WORKFLOW.md", ".instructions.md"])
|
||||
.map(str::to_owned)
|
||||
.collect();
|
||||
|
||||
// Backslashes and colons are ordinary filename characters on POSIX executors.
|
||||
for (cwd, extra_filenames) in [
|
||||
("file:///repo", windows_paths.as_slice()),
|
||||
("file:///C:/repo", &[][..]),
|
||||
] {
|
||||
let cwd: PathUri = cwd.parse().expect("cwd URI");
|
||||
let metadata_calls = Arc::new(MetadataCallCounts::default());
|
||||
let filesystem = FailingFileSystem {
|
||||
path: tmp.abs(),
|
||||
failure: InjectedFailure::MetadataNotFound,
|
||||
metadata_calls: Arc::clone(&metadata_calls),
|
||||
};
|
||||
let paths = super::agents_md_paths(&cfg, &cwd, &filesystem, /*sandbox*/ None)
|
||||
.await
|
||||
.expect("discover paths");
|
||||
|
||||
assert_eq!(paths, Vec::<PathUri>::new());
|
||||
assert_eq!(
|
||||
*metadata_calls.paths.lock().expect("metadata paths lock"),
|
||||
["AGENTS.override.md", "AGENTS.md"]
|
||||
.into_iter()
|
||||
.chain(extra_filenames.iter().copied())
|
||||
.chain(["WORKFLOW.md", ".instructions.md"])
|
||||
.map(|name| cwd.join(name).expect("filename"))
|
||||
.collect::<Vec<_>>()
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// When AGENTS.md is absent but a configured fallback exists, the fallback is used.
|
||||
#[tokio::test]
|
||||
async fn uses_configured_fallback_when_agents_missing() {
|
||||
|
||||
@@ -374,6 +374,58 @@ async fn configured_fallback_is_used_when_agents_candidate_is_directory() -> Res
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn invalid_fallback_paths_do_not_prevent_loading_valid_filenames() -> Result<()> {
|
||||
let server = start_mock_server().await;
|
||||
let response_mock = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![ev_response_created("resp1"), ev_completed("resp1")]),
|
||||
)
|
||||
.await;
|
||||
let test = test_codex()
|
||||
.with_config(|config| {
|
||||
config.project_doc_fallback_filenames =
|
||||
[".", "..", "nested/WORKFLOW.md", "WORKFLOW.md"]
|
||||
.map(str::to_owned)
|
||||
.to_vec();
|
||||
})
|
||||
.with_workspace_setup(|cwd, fs| async move {
|
||||
let nested = executor_path_uri(cwd.join("nested"))?;
|
||||
fs.create_directory(
|
||||
&nested,
|
||||
CreateDirectoryOptions {
|
||||
recursive: false,
|
||||
follow_symlinks: true,
|
||||
},
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
.await?;
|
||||
for (path, contents) in [
|
||||
(nested.join("WORKFLOW.md")?, b"nested instructions".to_vec()),
|
||||
(
|
||||
executor_path_uri(cwd.join("WORKFLOW.md"))?,
|
||||
b"local instructions".to_vec(),
|
||||
),
|
||||
] {
|
||||
fs.write_file(&path, contents, Default::default(), /*sandbox*/ None)
|
||||
.await?;
|
||||
}
|
||||
Ok::<(), anyhow::Error>(())
|
||||
})
|
||||
.build_with_auto_env(&server)
|
||||
.await?;
|
||||
test.submit_turn("hello").await?;
|
||||
|
||||
assert_single_instruction_fragment(
|
||||
&response_mock.single_request(),
|
||||
&expected_instruction_fragment(
|
||||
&test.executor_environment().selection().cwd,
|
||||
"local instructions",
|
||||
),
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn agents_docs_are_concatenated_from_project_root_to_cwd() -> Result<()> {
|
||||
let instructions = agents_instructions(
|
||||
|
||||
Reference in New Issue
Block a user