Skip to content

Commit bf6ed20

Browse files
author
Test User
committed
fix(security): sanitise agent_id and learning_id in markdown_store paths (CWE-22)
source_agent was joined directly into filesystem paths in agent_dir(), save(), save_to_shared(), load(), and delete() — enabling path traversal attacks (CWE-22) via crafted agent IDs such as "../../../etc/passwd". Add sanitise_path_component() which replaces every character that is not ASCII-alphanumeric, hyphen, underscore, or dot with an underscore, and emits a tracing::warn! when sanitisation is required (audit trail for injection attempts). Apply sanitisation at every path-construction site: - agent_dir(): agent_id - save(): learning.id in filename - save_to_shared(): both learning.source_agent and learning.id in filename - load(): learning_id in filename - delete(): learning_id in filename Add 5 regression tests covering strip of slashes, null bytes, traversal sequences, and end-to-end save/load with malicious source_agent values. Fixes #2160
1 parent d13f139 commit bf6ed20

1 file changed

Lines changed: 140 additions & 5 deletions

File tree

crates/terraphim_agent/src/shared_learning/markdown_store.rs

Lines changed: 140 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,34 @@ use serde::{Deserialize, Serialize};
1010
use thiserror::Error;
1111
use tracing::{info, warn};
1212

13+
/// Sanitise a path component, replacing dangerous characters with underscores.
14+
///
15+
/// Allows only ASCII alphanumeric characters, hyphens, underscores, and dots.
16+
/// Any other character (including `/`, `\`, null bytes, and `..` sequences made
17+
/// reachable via `/`) is replaced with `_`. Callers that receive user-supplied
18+
/// strings for agent IDs or learning IDs must pass them through this function
19+
/// before constructing filesystem paths (CWE-22 prevention).
20+
fn sanitise_path_component(component: &str) -> String {
21+
let sanitised: String = component
22+
.chars()
23+
.map(|c| {
24+
if c.is_ascii_alphanumeric() || matches!(c, '-' | '_' | '.') {
25+
c
26+
} else {
27+
'_'
28+
}
29+
})
30+
.collect();
31+
if sanitised != component {
32+
warn!(
33+
original = component,
34+
sanitised = %sanitised,
35+
"path component contained unsafe characters — sanitised to prevent path traversal"
36+
);
37+
}
38+
sanitised
39+
}
40+
1341
use crate::shared_learning::types::{LearningSource, QualityMetrics, SharedLearning, TrustLevel};
1442

1543
#[derive(Error, Debug)]
@@ -111,8 +139,12 @@ impl MarkdownLearningStore {
111139
}
112140

113141
/// Get the directory for a specific agent's learnings
142+
///
143+
/// The `agent_id` is sanitised before joining to prevent path traversal (CWE-22).
114144
pub fn agent_dir(&self, agent_id: &str) -> PathBuf {
115-
self.config.learnings_dir.join(agent_id)
145+
self.config
146+
.learnings_dir
147+
.join(sanitise_path_component(agent_id))
116148
}
117149

118150
/// Get the shared directory for cross-agent learnings
@@ -127,7 +159,7 @@ impl MarkdownLearningStore {
127159
let agent_dir = self.agent_dir(&learning.source_agent);
128160
tokio::fs::create_dir_all(&agent_dir).await?;
129161

130-
let file_path = agent_dir.join(format!("{}.md", learning.id));
162+
let file_path = agent_dir.join(format!("{}.md", sanitise_path_component(&learning.id)));
131163
let content = Self::to_markdown(learning)?;
132164

133165
tokio::fs::write(&file_path, content).await?;
@@ -144,7 +176,11 @@ impl MarkdownLearningStore {
144176
let shared_dir = self.shared_dir();
145177
tokio::fs::create_dir_all(&shared_dir).await?;
146178

147-
let file_path = shared_dir.join(format!("{}-{}.md", learning.source_agent, learning.id));
179+
let file_path = shared_dir.join(format!(
180+
"{}-{}.md",
181+
sanitise_path_component(&learning.source_agent),
182+
sanitise_path_component(&learning.id)
183+
));
148184
let content = Self::to_markdown(learning)?;
149185

150186
tokio::fs::write(&file_path, content).await?;
@@ -163,7 +199,9 @@ impl MarkdownLearningStore {
163199
agent_id: &str,
164200
learning_id: &str,
165201
) -> Result<SharedLearning, MarkdownStoreError> {
166-
let file_path = self.agent_dir(agent_id).join(format!("{}.md", learning_id));
202+
let file_path = self
203+
.agent_dir(agent_id)
204+
.join(format!("{}.md", sanitise_path_component(learning_id)));
167205
self.load_from_path(&file_path).await
168206
}
169207

@@ -241,7 +279,9 @@ impl MarkdownLearningStore {
241279
agent_id: &str,
242280
learning_id: &str,
243281
) -> Result<(), MarkdownStoreError> {
244-
let file_path = self.agent_dir(agent_id).join(format!("{}.md", learning_id));
282+
let file_path = self
283+
.agent_dir(agent_id)
284+
.join(format!("{}.md", sanitise_path_component(learning_id)));
245285
tokio::fs::remove_file(&file_path).await?;
246286
info!("Deleted learning {} from agent {}", learning_id, agent_id);
247287
Ok(())
@@ -638,4 +678,99 @@ This is content from an old learning.
638678
let all = store.list_all().await.unwrap();
639679
assert!(all.is_empty());
640680
}
681+
682+
// -- Path traversal regression tests (CWE-22) --
683+
684+
#[test]
685+
fn test_sanitise_path_component_strips_slashes() {
686+
assert_eq!(sanitise_path_component("../../../etc"), ".._.._.._etc");
687+
assert_eq!(sanitise_path_component("/etc/passwd"), "_etc_passwd");
688+
assert_eq!(sanitise_path_component("a\\b"), "a_b");
689+
}
690+
691+
#[test]
692+
fn test_sanitise_path_component_preserves_safe_chars() {
693+
assert_eq!(sanitise_path_component("agent-1"), "agent-1");
694+
assert_eq!(sanitise_path_component("my_agent.v2"), "my_agent.v2");
695+
assert_eq!(
696+
sanitise_path_component("abc123-ABC_def.0"),
697+
"abc123-ABC_def.0"
698+
);
699+
}
700+
701+
#[test]
702+
fn test_sanitise_path_component_strips_null_bytes() {
703+
let with_null = "agent\x00id";
704+
let sanitised = sanitise_path_component(with_null);
705+
assert!(!sanitised.contains('\x00'));
706+
assert_eq!(sanitised, "agent_id");
707+
}
708+
709+
#[tokio::test]
710+
async fn test_agent_dir_is_contained_when_id_has_traversal() {
711+
let temp_dir = TempDir::new().unwrap();
712+
let config = MarkdownStoreConfig {
713+
learnings_dir: temp_dir.path().to_path_buf(),
714+
shared_dir_name: "shared".to_string(),
715+
};
716+
let store = MarkdownLearningStore::with_config(config);
717+
718+
let dir = store.agent_dir("../../../etc");
719+
assert!(
720+
dir.starts_with(temp_dir.path()),
721+
"agent_dir must remain within learnings_dir after sanitisation, got: {:?}",
722+
dir
723+
);
724+
}
725+
726+
#[tokio::test]
727+
async fn test_save_with_traversal_agent_stays_inside_learnings_dir() {
728+
let temp_dir = TempDir::new().unwrap();
729+
let config = MarkdownStoreConfig {
730+
learnings_dir: temp_dir.path().to_path_buf(),
731+
shared_dir_name: "shared".to_string(),
732+
};
733+
let store = MarkdownLearningStore::with_config(config);
734+
735+
let learning = SharedLearning::new(
736+
"Traversal Test".to_string(),
737+
"Body".to_string(),
738+
LearningSource::AutoExtract,
739+
"../../../etc".to_string(),
740+
);
741+
742+
store.save(&learning).await.unwrap();
743+
744+
// The file must live inside temp_dir, not escape to /etc
745+
let agent_dir = store.agent_dir("../../../etc");
746+
assert!(
747+
agent_dir.starts_with(temp_dir.path()),
748+
"Saved file must stay inside learnings_dir: {:?}",
749+
agent_dir
750+
);
751+
}
752+
753+
#[tokio::test]
754+
async fn test_save_to_shared_with_traversal_stays_inside_shared_dir() {
755+
let temp_dir = TempDir::new().unwrap();
756+
let config = MarkdownStoreConfig {
757+
learnings_dir: temp_dir.path().to_path_buf(),
758+
shared_dir_name: "shared".to_string(),
759+
};
760+
let store = MarkdownLearningStore::with_config(config);
761+
762+
let learning = SharedLearning::new(
763+
"Shared Traversal Test".to_string(),
764+
"Body".to_string(),
765+
LearningSource::AutoExtract,
766+
"../../passwd".to_string(),
767+
);
768+
769+
store.save_to_shared(&learning).await.unwrap();
770+
771+
let shared = store.list_shared().await.unwrap();
772+
assert_eq!(shared.len(), 1);
773+
// The sanitised source_agent must appear in the title match, confirming the file loaded
774+
assert_eq!(shared[0].title, "Shared Traversal Test");
775+
}
641776
}

0 commit comments

Comments
 (0)