Skip to content

fix(skills): prevent path traversal in LocalSkillSource#1228

Open
adilburaksen wants to merge 1 commit into
google:mainfrom
adilburaksen:fix/local-skill-source-path-traversal
Open

fix(skills): prevent path traversal in LocalSkillSource#1228
adilburaksen wants to merge 1 commit into
google:mainfrom
adilburaksen:fix/local-skill-source-path-traversal

Conversation

@adilburaksen
Copy link
Copy Markdown

Summary

LocalSkillSource resolves caller-supplied skillName and resourcePath
strings directly onto the file system without validating that the resulting
path remains within skillsBasePath. A crafted value such as
../../../etc/passwd passes the Files.exists() check (the OS stat call
follows .. segments) and is returned as a valid resource path, enabling
arbitrary file reads from the host.

Affected methods: findResourcePath, listResources, findSkillMdPath.

Fix

Added a private validatePathWithinBase(Path base, String component) helper
that rejects:

  • absolute path components (e.g. /etc/passwd)
  • components whose normalized, absolute resolution escapes 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:

Test Input Expected
testLoadResource_pathTraversalInSkillName skillName ../other-dir SkillSourceException "Path traversal detected"
testLoadResource_pathTraversalInResourcePath resourcePath ../../../etc/passwd SkillSourceException "Path traversal detected"
testLoadResource_absoluteResourcePath resourcePath /etc/passwd SkillSourceException "Absolute paths are not allowed"
testListResources_pathTraversalInSkillName skillName ../../etc SkillSourceException "Path traversal detected"
testListResources_pathTraversalInResourceDirectory resourceDirectory ../other-skill SkillSourceException "Path traversal detected"

@hemasekhar-p hemasekhar-p self-assigned this Jun 1, 2026
@hemasekhar-p
Copy link
Copy Markdown
Contributor

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?

@hemasekhar-p hemasekhar-p added the waiting on reporter Waiting for reaction by reporter. Failing that, maintainers will eventually closed it as stale. label Jun 1, 2026
@hemasekhar-p
Copy link
Copy Markdown
Contributor

@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?

@hemasekhar-p hemasekhar-p added waiting on reporter Waiting for reaction by reporter. Failing that, maintainers will eventually closed it as stale. and removed waiting on reporter Waiting for reaction by reporter. Failing that, maintainers will eventually closed it as stale. labels Jun 1, 2026
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.
@adilburaksen adilburaksen force-pushed the fix/local-skill-source-path-traversal branch from b0f2e30 to 8752425 Compare June 1, 2026 11:39
@adilburaksen
Copy link
Copy Markdown
Author

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.

@hemasekhar-p
Copy link
Copy Markdown
Contributor

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.

@hemasekhar-p
Copy link
Copy Markdown
Contributor

@sherryfox, Could you please review this.

@hemasekhar-p hemasekhar-p added needs review and removed waiting on reporter Waiting for reaction by reporter. Failing that, maintainers will eventually closed it as stale. labels Jun 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants