mirror of
https://github.com/NVIDIA/OpenShell.git
synced 2026-10-19 05:50:41 +08:00
fix(providers): prevent home path escape in expand_home (!54)
Closes #61 ## Summary - harden provider discovery home expansion to reject path-escape components like dot-dot - reuse the same safe expansion logic in MockDiscoveryContext - add regression tests for escaped paths and valid home expansions - document the new config-path expansion safety rule ## Test Plan - mise run pre-commit
This commit is contained in:
@@ -123,6 +123,7 @@ Discovery behavior is split into three layers:
|
||||
|
||||
2. **Config file scan**: for each path in `spec.config_paths`:
|
||||
- expands `~/` via the context,
|
||||
- rejects `~/` expansions that contain path-escape components (for example `..`),
|
||||
- checks file existence,
|
||||
- **only parses `.json` files** (`.yml`/`.yaml` are checked for existence but not read),
|
||||
- recursively collects JSON fields whose keys match credential patterns
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
use std::path::{Path, PathBuf};
|
||||
use std::path::{Component, Path, PathBuf};
|
||||
|
||||
pub trait DiscoveryContext {
|
||||
fn env_var(&self, key: &str) -> Option<String>;
|
||||
@@ -16,10 +16,8 @@ impl DiscoveryContext for RealDiscoveryContext {
|
||||
|
||||
fn expand_home(&self, path: &str) -> Option<PathBuf> {
|
||||
if let Some(stripped) = path.strip_prefix("~/") {
|
||||
return std::env::var("HOME")
|
||||
.ok()
|
||||
.map(PathBuf::from)
|
||||
.map(|home| home.join(stripped));
|
||||
let home = std::env::var("HOME").ok()?;
|
||||
return expand_home_relative(Path::new(&home), stripped);
|
||||
}
|
||||
Some(PathBuf::from(path))
|
||||
}
|
||||
@@ -32,3 +30,36 @@ impl DiscoveryContext for RealDiscoveryContext {
|
||||
std::fs::read_to_string(path).ok()
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn expand_home_relative(home: &Path, stripped: &str) -> Option<PathBuf> {
|
||||
let relative = Path::new(stripped);
|
||||
for component in relative.components() {
|
||||
match component {
|
||||
Component::Normal(_) | Component::CurDir => {}
|
||||
Component::ParentDir | Component::RootDir | Component::Prefix(_) => return None,
|
||||
}
|
||||
}
|
||||
Some(home.join(relative))
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::expand_home_relative;
|
||||
use std::path::{Path, PathBuf};
|
||||
|
||||
#[test]
|
||||
fn rejects_parent_dir_escape_when_expanding_home() {
|
||||
assert_eq!(
|
||||
expand_home_relative(Path::new("/home/tester"), "../etc/passwd"),
|
||||
None
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn expands_safe_relative_home_path() {
|
||||
assert_eq!(
|
||||
expand_home_relative(Path::new("/home/tester"), ".config/codex/config.json"),
|
||||
Some(PathBuf::from("/home/tester/.config/codex/config.json"))
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -92,3 +92,29 @@ fn collect_credential_fields(
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::discover_with_spec;
|
||||
use crate::ProviderDiscoverySpec;
|
||||
use crate::test_helpers::{MockDiscoveryContext, home_join};
|
||||
|
||||
#[test]
|
||||
fn ignores_home_escaping_config_paths() {
|
||||
let spec = ProviderDiscoverySpec {
|
||||
id: "test",
|
||||
credential_env_vars: &[],
|
||||
config_paths: &["~/../secrets.json"],
|
||||
};
|
||||
|
||||
let ctx = MockDiscoveryContext::new()
|
||||
.with_home("/mock/home")
|
||||
.with_file(
|
||||
home_join("/mock/home", "../secrets.json"),
|
||||
r#"{"token":"secret"}"#,
|
||||
);
|
||||
|
||||
let discovered = discover_with_spec(&spec, &ctx).expect("discovery");
|
||||
assert!(discovered.is_none());
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
use crate::DiscoveryContext;
|
||||
use crate::context::expand_home_relative;
|
||||
use std::collections::{HashMap, HashSet};
|
||||
use std::path::{Path, PathBuf};
|
||||
|
||||
@@ -45,7 +46,8 @@ impl DiscoveryContext for MockDiscoveryContext {
|
||||
|
||||
fn expand_home(&self, path: &str) -> Option<PathBuf> {
|
||||
if let Some(stripped) = path.strip_prefix("~/") {
|
||||
return self.home.as_ref().map(|home| home.join(stripped));
|
||||
let home = self.home.as_deref()?;
|
||||
return expand_home_relative(home, stripped);
|
||||
}
|
||||
Some(PathBuf::from(path))
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user