fix(skills): prevent path traversal in LocalSkillSource#1228
fix(skills): prevent path traversal in LocalSkillSource#1228adilburaksen wants to merge 1 commit into
Conversation
|
Hi @adilburaksen, thank you for your contribution! We appreciate you taking the time to submit this pull request. I noticed some formatting inconsistencies in the test cases. could you please address these issues and run the full Maven build without skipping the test cases to ensure everything is compliant? |
|
@adilburaksen, Thank you for your quick response. As per contribution policy, please ensure your PR consists of a single commit. Could you please change your commits accordingly? |
Add input validation to LocalSkillSource to ensure skill names and resource paths cannot escape the skills base directory via path traversal sequences (e.g. "../../../etc/passwd") or absolute paths (e.g. "/etc/passwd"). The new validatePathWithinBase() helper normalizes and resolves each caller-supplied path component against its parent directory, then checks that the result still starts with that parent. This mirrors the boundary check already present in the Go implementation (filesystem_source.go). Affected methods: findResourcePath, listResources, findSkillMdPath. Corresponding tests added for all traversal and absolute-path cases.
b0f2e30 to
8752425
Compare
|
Thanks for the review @hemasekhar-p. I've applied google-java-format to the test file so it matches the project style now, and I squashed everything down into a single commit as requested. I also ran the full build with tests enabled (no skips) and it passes, including LocalSkillSourceTest (16 tests, all green). Let me know if anything else is needed. |
|
Thank you for your update @adilburaksen, Currently this PR is under review by our team, we will keep you posted if any additional information is required. thank you. |
|
@sherryfox, Could you please review this. |
Summary
LocalSkillSourceresolves caller-suppliedskillNameandresourcePathstrings directly onto the file system without validating that the resulting
path remains within
skillsBasePath. A crafted value such as../../../etc/passwdpasses theFiles.exists()check (the OSstatcallfollows
..segments) and is returned as a valid resource path, enablingarbitrary file reads from the host.
Affected methods:
findResourcePath,listResources,findSkillMdPath.Fix
Added a private
validatePathWithinBase(Path base, String component)helperthat rejects:
/etc/passwd)base(e.g.
../../secret)The check mirrors the boundary enforcement already present in the Go
implementation (
filesystem_source.go).Tests
Five new test cases in
LocalSkillSourceTest:testLoadResource_pathTraversalInSkillName../other-dirSkillSourceException"Path traversal detected"testLoadResource_pathTraversalInResourcePath../../../etc/passwdSkillSourceException"Path traversal detected"testLoadResource_absoluteResourcePath/etc/passwdSkillSourceException"Absolute paths are not allowed"testListResources_pathTraversalInSkillName../../etcSkillSourceException"Path traversal detected"testListResources_pathTraversalInResourceDirectory../other-skillSkillSourceException"Path traversal detected"