fix: added path validation for skill names
This commit is contained in:
@@ -80,6 +80,21 @@ pub fn skill_file(name: &str) -> PathBuf {
|
|||||||
skill_dir(name).join("SKILL.md")
|
skill_dir(name).join("SKILL.md")
|
||||||
}
|
}
|
||||||
|
|
||||||
|
pub fn validate_skill_name(name: &str) -> Result<()> {
|
||||||
|
if name.is_empty() {
|
||||||
|
bail!("Skill name cannot be empty");
|
||||||
|
}
|
||||||
|
if !name
|
||||||
|
.chars()
|
||||||
|
.all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_')
|
||||||
|
{
|
||||||
|
bail!(
|
||||||
|
"Invalid skill name '{name}': only letters, digits, '-', and '_' are allowed"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
|
||||||
pub fn macros_dir() -> PathBuf {
|
pub fn macros_dir() -> PathBuf {
|
||||||
match env::var(get_env_name("macros_dir")) {
|
match env::var(get_env_name("macros_dir")) {
|
||||||
Ok(value) => PathBuf::from(value),
|
Ok(value) => PathBuf::from(value),
|
||||||
@@ -286,3 +301,42 @@ pub fn local_models_override() -> Result<Vec<ProviderModels>> {
|
|||||||
}
|
}
|
||||||
Ok(models_override.list)
|
Ok(models_override.list)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[cfg(test)]
|
||||||
|
mod tests {
|
||||||
|
use super::*;
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn validate_skill_name_accepts_alphanumerics_and_dashes() {
|
||||||
|
assert!(validate_skill_name("git-master").is_ok());
|
||||||
|
assert!(validate_skill_name("code_review").is_ok());
|
||||||
|
assert!(validate_skill_name("Skill1").is_ok());
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn validate_skill_name_rejects_empty() {
|
||||||
|
let err = validate_skill_name("").unwrap_err();
|
||||||
|
assert!(err.to_string().contains("cannot be empty"));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn validate_skill_name_rejects_path_traversal() {
|
||||||
|
for bad in ["../escape", "..", "foo/bar", "foo\\bar", "./hidden"] {
|
||||||
|
let err = validate_skill_name(bad).unwrap_err();
|
||||||
|
assert!(
|
||||||
|
err.to_string().contains("Invalid skill name"),
|
||||||
|
"expected rejection for {bad:?}, got: {err}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn validate_skill_name_rejects_other_special_chars() {
|
||||||
|
for bad in ["with space", "null\0byte", "weird?char", "dot.name"] {
|
||||||
|
assert!(
|
||||||
|
validate_skill_name(bad).is_err(),
|
||||||
|
"expected rejection for {bad:?}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -2584,6 +2584,7 @@ impl RequestContext {
|
|||||||
}
|
}
|
||||||
|
|
||||||
pub fn upsert_skill(&self, app: &AppConfig, name: &str) -> Result<()> {
|
pub fn upsert_skill(&self, app: &AppConfig, name: &str) -> Result<()> {
|
||||||
|
paths::validate_skill_name(name)?;
|
||||||
let path = paths::skill_file(name);
|
let path = paths::skill_file(name);
|
||||||
ensure_parent_exists(&path)?;
|
ensure_parent_exists(&path)?;
|
||||||
let is_new = !path.exists();
|
let is_new = !path.exists();
|
||||||
@@ -2602,6 +2603,7 @@ impl RequestContext {
|
|||||||
}
|
}
|
||||||
|
|
||||||
pub async fn load_skill_repl(&mut self, name: &str, abort_signal: AbortSignal) -> Result<()> {
|
pub async fn load_skill_repl(&mut self, name: &str, abort_signal: AbortSignal) -> Result<()> {
|
||||||
|
paths::validate_skill_name(name)?;
|
||||||
if !self.app.config.function_calling_support {
|
if !self.app.config.function_calling_support {
|
||||||
bail!(
|
bail!(
|
||||||
"Skills require function calling, which is disabled. Enable function calling in your config then try again."
|
"Skills require function calling, which is disabled. Enable function calling in your config then try again."
|
||||||
|
|||||||
@@ -116,6 +116,7 @@ impl Skill {
|
|||||||
}
|
}
|
||||||
|
|
||||||
pub fn load(name: &str) -> Result<Self> {
|
pub fn load(name: &str) -> Result<Self> {
|
||||||
|
paths::validate_skill_name(name)?;
|
||||||
let path = paths::skill_file(name);
|
let path = paths::skill_file(name);
|
||||||
let content = read_to_string(&path)
|
let content = read_to_string(&path)
|
||||||
.with_context(|| format!("Failed to read skill '{name}' at {}", path.display()))?;
|
.with_context(|| format!("Failed to read skill '{name}' at {}", path.display()))?;
|
||||||
|
|||||||
Reference in New Issue
Block a user