fix: keep permissions profiles forward compatible (#14107)

## Summary
- preserve unknown `:special_path` tokens, including nested entries, so
older Codex builds warn and ignore instead of failing config load
- fail closed with a startup warning when a permissions profile has
missing or empty filesystem entries instead of aborting profile
compilation
- normalize Windows verbatim paths like `\?\C:\...` before absolute-path
validation while keeping explicit errors for truly invalid paths

## Testing
- just fmt
- cargo test -p codex-core permissions_profiles_allow
- cargo test -p codex-core
normalize_absolute_path_for_platform_simplifies_windows_verbatim_paths
- cargo test -p codex-protocol
unknown_special_paths_are_ignored_by_legacy_bridge
- cargo clippy -p codex-core -p codex-protocol --all-targets -- -D
warnings
- cargo clean
This commit is contained in:
viyatb-oai 2026-03-09 18:43:38 -07:00 committed by GitHub
parent b0cbc25a48
commit 1165a16e6f
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 377 additions and 49 deletions

View file

@ -574,6 +574,156 @@ fn permissions_profiles_reject_nested_entries_for_non_project_roots() -> std::io
Ok(())
}
fn load_workspace_permission_profile(profile: PermissionProfileToml) -> std::io::Result<Config> {
let codex_home = TempDir::new()?;
let cwd = TempDir::new()?;
std::fs::write(cwd.path().join(".git"), "gitdir: nowhere")?;
Config::load_from_base_config_with_overrides(
ConfigToml {
default_permissions: Some("workspace".to_string()),
permissions: Some(PermissionsToml {
entries: BTreeMap::from([("workspace".to_string(), profile)]),
}),
..Default::default()
},
ConfigOverrides {
cwd: Some(cwd.path().to_path_buf()),
..Default::default()
},
codex_home.path().to_path_buf(),
)
}
#[test]
fn permissions_profiles_allow_unknown_special_paths() -> std::io::Result<()> {
let config = load_workspace_permission_profile(PermissionProfileToml {
filesystem: Some(FilesystemPermissionsToml {
entries: BTreeMap::from([(
":future_special_path".to_string(),
FilesystemPermissionToml::Access(FileSystemAccessMode::Read),
)]),
}),
network: None,
})?;
assert_eq!(
config.permissions.file_system_sandbox_policy,
FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry {
path: FileSystemPath::Special {
value: FileSystemSpecialPath::unknown(":future_special_path", None),
},
access: FileSystemAccessMode::Read,
}]),
);
assert_eq!(
config.permissions.sandbox_policy.get(),
&SandboxPolicy::ReadOnly {
access: ReadOnlyAccess::Restricted {
include_platform_defaults: false,
readable_roots: Vec::new(),
},
network_access: false,
}
);
assert!(
config.startup_warnings.iter().any(|warning| warning.contains(
"Configured filesystem path `:future_special_path` is not recognized by this version of Codex and will be ignored."
)),
"{:?}",
config.startup_warnings
);
Ok(())
}
#[test]
fn permissions_profiles_allow_unknown_special_paths_with_nested_entries() -> std::io::Result<()> {
let config = load_workspace_permission_profile(PermissionProfileToml {
filesystem: Some(FilesystemPermissionsToml {
entries: BTreeMap::from([(
":future_special_path".to_string(),
FilesystemPermissionToml::Scoped(BTreeMap::from([(
"docs".to_string(),
FileSystemAccessMode::Read,
)])),
)]),
}),
network: None,
})?;
assert_eq!(
config.permissions.file_system_sandbox_policy,
FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry {
path: FileSystemPath::Special {
value: FileSystemSpecialPath::unknown(":future_special_path", Some("docs".into())),
},
access: FileSystemAccessMode::Read,
}]),
);
assert!(
config.startup_warnings.iter().any(|warning| warning.contains(
"Configured filesystem path `:future_special_path` with nested entry `docs` is not recognized by this version of Codex and will be ignored."
)),
"{:?}",
config.startup_warnings
);
Ok(())
}
#[test]
fn permissions_profiles_allow_missing_filesystem_with_warning() -> std::io::Result<()> {
let config = load_workspace_permission_profile(PermissionProfileToml {
filesystem: None,
network: None,
})?;
assert_eq!(
config.permissions.file_system_sandbox_policy,
FileSystemSandboxPolicy::restricted(Vec::new())
);
assert_eq!(
config.permissions.sandbox_policy.get(),
&SandboxPolicy::ReadOnly {
access: ReadOnlyAccess::Restricted {
include_platform_defaults: false,
readable_roots: Vec::new(),
},
network_access: false,
}
);
assert!(
config.startup_warnings.iter().any(|warning| warning.contains(
"Permissions profile `workspace` does not define any recognized filesystem entries for this version of Codex."
)),
"{:?}",
config.startup_warnings
);
Ok(())
}
#[test]
fn permissions_profiles_allow_empty_filesystem_with_warning() -> std::io::Result<()> {
let config = load_workspace_permission_profile(PermissionProfileToml {
filesystem: Some(FilesystemPermissionsToml {
entries: BTreeMap::new(),
}),
network: None,
})?;
assert_eq!(
config.permissions.file_system_sandbox_policy,
FileSystemSandboxPolicy::restricted(Vec::new())
);
assert!(
config.startup_warnings.iter().any(|warning| warning.contains(
"Permissions profile `workspace` does not define any recognized filesystem entries for this version of Codex."
)),
"{:?}",
config.startup_warnings
);
Ok(())
}
#[test]
fn permissions_profiles_reject_project_root_parent_traversal() -> std::io::Result<()> {
let codex_home = TempDir::new()?;

View file

@ -1971,7 +1971,11 @@ impl Config {
let configured_network_proxy_config =
network_proxy_config_from_profile_network(profile.network.as_ref());
let (mut file_system_sandbox_policy, network_sandbox_policy) =
compile_permission_profile(permissions, default_permissions)?;
compile_permission_profile(
permissions,
default_permissions,
&mut startup_warnings,
)?;
let mut sandbox_policy = file_system_sandbox_policy
.to_legacy_sandbox_policy(network_sandbox_policy, &resolved_cwd)?;
if matches!(sandbox_policy, SandboxPolicy::WorkspaceWrite { .. }) {

View file

@ -1,3 +1,4 @@
use std::borrow::Cow;
use std::collections::BTreeMap;
use std::io;
use std::path::Component;
@ -158,30 +159,27 @@ pub(crate) fn resolve_permission_profile<'a>(
pub(crate) fn compile_permission_profile(
permissions: &PermissionsToml,
profile_name: &str,
startup_warnings: &mut Vec<String>,
) -> io::Result<(FileSystemSandboxPolicy, NetworkSandboxPolicy)> {
let profile = resolve_permission_profile(permissions, profile_name)?;
let filesystem = profile.filesystem.as_ref().ok_or_else(|| {
io::Error::new(
io::ErrorKind::InvalidInput,
format!(
"permissions profile `{profile_name}` must define a `[permissions.{profile_name}.filesystem]` table"
),
)
})?;
if filesystem.is_empty() {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
format!(
"permissions profile `{profile_name}` must define at least one filesystem entry"
),
));
}
let mut entries = Vec::new();
for (path, permission) in &filesystem.entries {
compile_filesystem_permission(path, permission, &mut entries)?;
if let Some(filesystem) = profile.filesystem.as_ref() {
if filesystem.is_empty() {
push_warning(
startup_warnings,
missing_filesystem_entries_warning(profile_name),
);
} else {
for (path, permission) in &filesystem.entries {
compile_filesystem_permission(path, permission, &mut entries, startup_warnings)?;
}
}
} else {
push_warning(
startup_warnings,
missing_filesystem_entries_warning(profile_name),
);
}
let network_sandbox_policy = compile_network_sandbox_policy(profile.network.as_ref());
@ -207,16 +205,17 @@ fn compile_filesystem_permission(
path: &str,
permission: &FilesystemPermissionToml,
entries: &mut Vec<FileSystemSandboxEntry>,
startup_warnings: &mut Vec<String>,
) -> io::Result<()> {
match permission {
FilesystemPermissionToml::Access(access) => entries.push(FileSystemSandboxEntry {
path: compile_filesystem_path(path)?,
path: compile_filesystem_path(path, startup_warnings)?,
access: *access,
}),
FilesystemPermissionToml::Scoped(scoped_entries) => {
for (subpath, access) in scoped_entries {
entries.push(FileSystemSandboxEntry {
path: compile_scoped_filesystem_path(path, subpath)?,
path: compile_scoped_filesystem_path(path, subpath, startup_warnings)?,
access: *access,
});
}
@ -225,8 +224,12 @@ fn compile_filesystem_permission(
Ok(())
}
fn compile_filesystem_path(path: &str) -> io::Result<FileSystemPath> {
if let Some(special) = parse_special_path(path)? {
fn compile_filesystem_path(
path: &str,
startup_warnings: &mut Vec<String>,
) -> io::Result<FileSystemPath> {
if let Some(special) = parse_special_path(path) {
maybe_push_unknown_special_path_warning(&special, startup_warnings);
return Ok(FileSystemPath::Special { value: special });
}
@ -234,21 +237,33 @@ fn compile_filesystem_path(path: &str) -> io::Result<FileSystemPath> {
Ok(FileSystemPath::Path { path })
}
fn compile_scoped_filesystem_path(path: &str, subpath: &str) -> io::Result<FileSystemPath> {
fn compile_scoped_filesystem_path(
path: &str,
subpath: &str,
startup_warnings: &mut Vec<String>,
) -> io::Result<FileSystemPath> {
if subpath == "." {
return compile_filesystem_path(path);
return compile_filesystem_path(path, startup_warnings);
}
if let Some(special) = parse_special_path(path)? {
if !matches!(special, FileSystemSpecialPath::ProjectRoots { .. }) {
return Err(io::Error::new(
if let Some(special) = parse_special_path(path) {
let subpath = parse_relative_subpath(subpath)?;
let special = match special {
FileSystemSpecialPath::ProjectRoots { .. } => Ok(FileSystemPath::Special {
value: FileSystemSpecialPath::project_roots(Some(subpath)),
}),
FileSystemSpecialPath::Unknown { path, .. } => Ok(FileSystemPath::Special {
value: FileSystemSpecialPath::unknown(path, Some(subpath)),
}),
_ => Err(io::Error::new(
io::ErrorKind::InvalidInput,
format!("filesystem path `{path}` does not support nested entries"),
));
)),
}?;
if let FileSystemPath::Special { value } = &special {
maybe_push_unknown_special_path_warning(value, startup_warnings);
}
return Ok(FileSystemPath::Special {
value: FileSystemSpecialPath::project_roots(Some(parse_relative_subpath(subpath)?)),
});
return Ok(special);
}
let subpath = parse_relative_subpath(subpath)?;
@ -257,33 +272,90 @@ fn compile_scoped_filesystem_path(path: &str, subpath: &str) -> io::Result<FileS
Ok(FileSystemPath::Path { path })
}
fn parse_special_path(path: &str) -> io::Result<Option<FileSystemSpecialPath>> {
let special = match path {
// WARNING: keep this parser forward-compatible.
// Adding a new `:special_path` must not make older Codex versions reject the
// config. Unknown values intentionally round-trip through
// `FileSystemSpecialPath::Unknown` so they can be surfaced as warnings and
// ignored, rather than aborting config load.
fn parse_special_path(path: &str) -> Option<FileSystemSpecialPath> {
match path {
":root" => Some(FileSystemSpecialPath::Root),
":minimal" => Some(FileSystemSpecialPath::Minimal),
":project_roots" => Some(FileSystemSpecialPath::project_roots(None)),
":tmpdir" => Some(FileSystemSpecialPath::Tmpdir),
_ if path.starts_with(':') => {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
format!("unknown filesystem special path `{path}`"),
));
}
_ if path.starts_with(':') => Some(FileSystemSpecialPath::unknown(path, None)),
_ => None,
};
Ok(special)
}
}
fn parse_absolute_path(path: &str) -> io::Result<AbsolutePathBuf> {
let path_ref = Path::new(path);
if !path_ref.is_absolute() && path != "~" && !path.starts_with("~/") {
parse_absolute_path_for_platform(path, cfg!(windows))
}
fn parse_absolute_path_for_platform(path: &str, is_windows: bool) -> io::Result<AbsolutePathBuf> {
let path_ref = normalize_absolute_path_for_platform(path, is_windows);
if !is_absolute_path_for_platform(path, path_ref.as_ref(), is_windows)
&& path != "~"
&& !path.starts_with("~/")
{
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
format!("filesystem path `{path}` must be absolute, use `~/...`, or start with `:`"),
));
}
AbsolutePathBuf::from_absolute_path(path_ref)
AbsolutePathBuf::from_absolute_path(path_ref.as_ref())
}
fn is_absolute_path_for_platform(path: &str, normalized_path: &Path, is_windows: bool) -> bool {
if is_windows {
is_windows_absolute_path(path)
|| is_windows_absolute_path(&normalized_path.to_string_lossy())
} else {
normalized_path.is_absolute()
}
}
fn normalize_absolute_path_for_platform(path: &str, is_windows: bool) -> Cow<'_, Path> {
if !is_windows {
return Cow::Borrowed(Path::new(path));
}
match normalize_windows_device_path(path) {
Some(normalized) => Cow::Owned(PathBuf::from(normalized)),
None => Cow::Borrowed(Path::new(path)),
}
}
fn normalize_windows_device_path(path: &str) -> Option<String> {
if let Some(unc) = path.strip_prefix(r"\\?\UNC\") {
return Some(format!(r"\\{unc}"));
}
if let Some(unc) = path.strip_prefix(r"\\.\UNC\") {
return Some(format!(r"\\{unc}"));
}
if let Some(path) = path.strip_prefix(r"\\?\")
&& is_windows_drive_absolute_path(path)
{
return Some(path.to_string());
}
if let Some(path) = path.strip_prefix(r"\\.\")
&& is_windows_drive_absolute_path(path)
{
return Some(path.to_string());
}
None
}
fn is_windows_absolute_path(path: &str) -> bool {
is_windows_drive_absolute_path(path) || path.starts_with(r"\\")
}
fn is_windows_drive_absolute_path(path: &str) -> bool {
let bytes = path.as_bytes();
bytes.len() >= 3
&& bytes[0].is_ascii_alphabetic()
&& bytes[1] == b':'
&& matches!(bytes[2], b'\\' | b'/')
}
fn parse_relative_subpath(subpath: &str) -> io::Result<PathBuf> {
@ -304,3 +376,48 @@ fn parse_relative_subpath(subpath: &str) -> io::Result<PathBuf> {
),
))
}
fn push_warning(startup_warnings: &mut Vec<String>, message: String) {
tracing::warn!("{message}");
startup_warnings.push(message);
}
fn missing_filesystem_entries_warning(profile_name: &str) -> String {
format!(
"Permissions profile `{profile_name}` does not define any recognized filesystem entries for this version of Codex. Filesystem access will remain restricted. Upgrade Codex if this profile expects filesystem permissions."
)
}
fn maybe_push_unknown_special_path_warning(
special: &FileSystemSpecialPath,
startup_warnings: &mut Vec<String>,
) {
let FileSystemSpecialPath::Unknown { path, subpath } = special else {
return;
};
push_warning(
startup_warnings,
match subpath.as_deref() {
Some(subpath) => format!(
"Configured filesystem path `{path}` with nested entry `{}` is not recognized by this version of Codex and will be ignored. Upgrade Codex if this path is required.",
subpath.display()
),
None => format!(
"Configured filesystem path `{path}` is not recognized by this version of Codex and will be ignored. Upgrade Codex if this path is required."
),
},
);
}
#[cfg(test)]
mod tests {
use super::*;
use pretty_assertions::assert_eq;
#[test]
fn normalize_absolute_path_for_platform_simplifies_windows_verbatim_paths() {
let parsed =
normalize_absolute_path_for_platform(r"\\?\D:\c\x\worktrees\2508\swift-base", true);
assert_eq!(parsed, PathBuf::from(r"D:\c\x\worktrees\2508\swift-base"));
}
}

View file

@ -67,12 +67,33 @@ pub enum FileSystemSpecialPath {
},
Tmpdir,
SlashTmp,
/// WARNING: `:special_path` tokens are part of config compatibility.
/// Do not make older runtimes reject newly introduced tokens.
/// New parser support should be additive, while unknown values must stay
/// representable so config from a newer Codex degrades to warn-and-ignore
/// instead of failing to load. Codex 0.112.0 rejected unknown values here,
/// which broke forward compatibility for newer config.
/// Preserves future special-path tokens so older runtimes can ignore them
/// without rejecting config authored by a newer release.
Unknown {
path: String,
#[serde(default, skip_serializing_if = "Option::is_none")]
#[ts(optional)]
subpath: Option<PathBuf>,
},
}
impl FileSystemSpecialPath {
pub fn project_roots(subpath: Option<PathBuf>) -> Self {
Self::ProjectRoots { subpath }
}
pub fn unknown(path: impl Into<String>, subpath: Option<PathBuf>) -> Self {
Self::Unknown {
path: path.into(),
subpath,
}
}
}
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema, TS)]
@ -437,6 +458,7 @@ impl FileSystemSandboxPolicy {
readable_roots.push(path);
}
}
FileSystemSpecialPath::Unknown { .. } => {}
},
}
}
@ -634,7 +656,9 @@ fn resolve_file_system_special_path(
cwd: Option<&AbsolutePathBuf>,
) -> Option<AbsolutePathBuf> {
match value {
FileSystemSpecialPath::Root | FileSystemSpecialPath::Minimal => None,
FileSystemSpecialPath::Root
| FileSystemSpecialPath::Minimal
| FileSystemSpecialPath::Unknown { .. } => None,
FileSystemSpecialPath::CurrentWorkingDirectory => {
let cwd = cwd?;
Some(cwd.clone())
@ -779,3 +803,36 @@ fn resolve_gitdir_from_file(dot_git: &AbsolutePathBuf) -> Option<AbsolutePathBuf
}
Some(gitdir_path)
}
#[cfg(test)]
mod tests {
use super::*;
use pretty_assertions::assert_eq;
#[test]
fn unknown_special_paths_are_ignored_by_legacy_bridge() -> std::io::Result<()> {
let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry {
path: FileSystemPath::Special {
value: FileSystemSpecialPath::unknown(":future_special_path", None),
},
access: FileSystemAccessMode::Write,
}]);
let sandbox_policy = policy.to_legacy_sandbox_policy(
NetworkSandboxPolicy::Restricted,
Path::new("/tmp/workspace"),
)?;
assert_eq!(
sandbox_policy,
SandboxPolicy::ReadOnly {
access: ReadOnlyAccess::Restricted {
include_platform_defaults: false,
readable_roots: Vec::new(),
},
network_access: false,
}
);
Ok(())
}
}