From aac8aa078711f47fac5555e04a59189e1800388c Mon Sep 17 00:00:00 2001 From: adilburaksen Date: Fri, 3 Jul 2026 20:52:27 +0300 Subject: [PATCH] fix(skills): prevent path traversal in LocalSkillSource LocalSkillSource resolved skill names and resource paths from caller input directly against skillsBasePath without validating containment, so a name or path containing ".." or an absolute path could escape the skills base directory (arbitrary read of files outside the configured skills root, e.g. credentials). Add validatePathWithinBase, which rejects absolute components and any component that normalizes outside its base directory, and route listResources, findResourcePath and findSkillMdPath through it. The helper returns the validated, normalized path so callers reuse it instead of re-resolving skillsBasePath.resolve(skillName). Component parsing uses base.getFileSystem() so it matches the filesystem base.resolve() uses when skillsBasePath comes from a non-default provider, and the error code is passed in so resource-path checks report RESOURCE_NOT_FOUND while skill-name checks report SKILL_NOT_FOUND. Add tests for skill-name traversal, resource-path traversal, findSkillMdPath (loadFrontmatter) traversal, and absolute-path rejection, each asserting on the specific error message. --- .../google/adk/skills/LocalSkillSource.java | 43 +++++++++- .../adk/skills/LocalSkillSourceTest.java | 79 +++++++++++++++++++ 2 files changed, 118 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/com/google/adk/skills/LocalSkillSource.java b/core/src/main/java/com/google/adk/skills/LocalSkillSource.java index 4eafa4d5f..7033303c3 100644 --- a/core/src/main/java/com/google/adk/skills/LocalSkillSource.java +++ b/core/src/main/java/com/google/adk/skills/LocalSkillSource.java @@ -45,12 +45,18 @@ public LocalSkillSource(Path skillsBasePath) { @Override public Single> listResources(String skillName, String resourceDirectory) { - Path skillDir = skillsBasePath.resolve(skillName); + Path skillDir; + Path resourceDir; + try { + skillDir = validatePathWithinBase(skillsBasePath, skillName, SKILL_NOT_FOUND); + resourceDir = validatePathWithinBase(skillDir, resourceDirectory, RESOURCE_NOT_FOUND); + } catch (SkillSourceException e) { + return Single.error(e); + } if (!isDirectory(skillDir)) { return Single.error( new SkillSourceException("Skill not found: " + skillName, SKILL_NOT_FOUND)); } - Path resourceDir = skillDir.resolve(resourceDirectory); if (!isDirectory(resourceDir)) { return Single.error( new SkillSourceException( @@ -96,7 +102,13 @@ protected Flowable> listSkills() { @Override protected Single findResourcePath(String skillName, String resourcePath) { - Path file = skillsBasePath.resolve(skillName).resolve(resourcePath); + Path file; + try { + Path skillDir = validatePathWithinBase(skillsBasePath, skillName, SKILL_NOT_FOUND); + file = validatePathWithinBase(skillDir, resourcePath, RESOURCE_NOT_FOUND); + } catch (SkillSourceException e) { + return Single.error(e); + } if (!Files.exists(file)) { return Single.error( new SkillSourceException("Resource not found: " + file, RESOURCE_NOT_FOUND)); @@ -106,7 +118,12 @@ protected Single findResourcePath(String skillName, String resourcePath) { @Override protected Single findSkillMdPath(String skillName) { - Path skillDir = skillsBasePath.resolve(skillName); + Path skillDir; + try { + skillDir = validatePathWithinBase(skillsBasePath, skillName, SKILL_NOT_FOUND); + } catch (SkillSourceException e) { + return Single.error(e); + } if (!isDirectory(skillDir)) { return Single.error( new SkillSourceException("Skill directory not found: " + skillName, SKILL_NOT_FOUND)); @@ -122,6 +139,24 @@ protected ReadableByteChannel openChannel(Path path) throws IOException { return Files.newByteChannel(path); } + private static Path validatePathWithinBase(Path base, String component, String errorCode) + throws SkillSourceException { + // Parse the component against the base's own filesystem: LocalSkillSource accepts an arbitrary + // skillsBasePath, which may come from a non-default provider, so Path.of (which always uses + // FileSystems.getDefault()) could disagree with base.resolve below. + if (base.getFileSystem().getPath(component).isAbsolute()) { + throw new SkillSourceException("Absolute paths are not allowed: " + component, errorCode); + } + Path normalizedBase = base.normalize().toAbsolutePath(); + Path resolved = base.resolve(component).normalize().toAbsolutePath(); + if (!resolved.startsWith(normalizedBase)) { + throw new SkillSourceException( + "Path traversal detected; component must not escape its base directory: " + component, + errorCode); + } + return resolved; + } + private Optional findSkillMd(Path dir) { return Optional.of(dir.resolve("SKILL.md")) .filter(Files::exists) diff --git a/core/src/test/java/com/google/adk/skills/LocalSkillSourceTest.java b/core/src/test/java/com/google/adk/skills/LocalSkillSourceTest.java index 5efdb4ab9..2d38de4c6 100644 --- a/core/src/test/java/com/google/adk/skills/LocalSkillSourceTest.java +++ b/core/src/test/java/com/google/adk/skills/LocalSkillSourceTest.java @@ -374,4 +374,83 @@ public void testLoadInstructions_emptyFile() throws IOException { .hasMessageThat() .contains("Skill file must start with ---"); } + + @Test + public void testLoadResource_pathTraversalRejected() throws IOException { + Path skillsBase = tempFolder.getRoot().toPath().resolve("skills"); + Files.createDirectory(skillsBase); + // A secret file outside the skills base directory. + Files.writeString(tempFolder.getRoot().toPath().resolve("secret.txt"), "top-secret"); + + SkillSource source = new LocalSkillSource(skillsBase); + + // A skill name that escapes the skills base via "..". + var single = source.loadResource("..", "secret.txt"); + RuntimeException exception = assertThrows(RuntimeException.class, single::blockingGet); + assertThat(exception).hasCauseThat().isInstanceOf(SkillSourceException.class); + assertThat(exception).hasCauseThat().hasMessageThat().contains("Path traversal detected"); + } + + @Test + public void testListResources_pathTraversalRejected() throws IOException { + Path skillsBase = tempFolder.getRoot().toPath().resolve("skills"); + Files.createDirectory(skillsBase); + + SkillSource source = new LocalSkillSource(skillsBase); + + var single = source.listResources("../../etc", ""); + RuntimeException exception = assertThrows(RuntimeException.class, single::blockingGet); + assertThat(exception).hasCauseThat().isInstanceOf(SkillSourceException.class); + assertThat(exception).hasCauseThat().hasMessageThat().contains("Path traversal detected"); + } + + @Test + public void testLoadFrontmatter_pathTraversalRejected() throws IOException { + Path skillsBase = tempFolder.getRoot().toPath().resolve("skills"); + Files.createDirectory(skillsBase); + + SkillSource source = new LocalSkillSource(skillsBase); + + // loadFrontmatter routes through findSkillMdPath, which must reject a traversing skill name. + var single = source.loadFrontmatter("../.."); + RuntimeException exception = assertThrows(RuntimeException.class, single::blockingGet); + assertThat(exception).hasCauseThat().isInstanceOf(SkillSourceException.class); + assertThat(exception).hasCauseThat().hasMessageThat().contains("Path traversal detected"); + } + + @Test + public void testLoadResource_resourcePathTraversalRejected() throws IOException { + Path skillsBase = tempFolder.getRoot().toPath().resolve("skills"); + Files.createDirectory(skillsBase); + Files.createDirectory(skillsBase.resolve("skill-1")); + // A secret file outside the individual skill directory (but under the skills base). + Files.writeString(tempFolder.getRoot().toPath().resolve("secret.txt"), "top-secret"); + + SkillSource source = new LocalSkillSource(skillsBase); + + // Valid skill name, but a resource path that escapes the skill directory via "..". + var single = source.loadResource("skill-1", "../../secret.txt"); + RuntimeException exception = assertThrows(RuntimeException.class, single::blockingGet); + assertThat(exception).hasCauseThat().isInstanceOf(SkillSourceException.class); + assertThat(exception).hasCauseThat().hasMessageThat().contains("Path traversal detected"); + } + + @Test + public void testLoadResource_absolutePathRejected() throws IOException { + Path skillsBase = tempFolder.getRoot().toPath().resolve("skills"); + Files.createDirectory(skillsBase); + Path secret = tempFolder.getRoot().toPath().resolve("secret.txt"); + Files.writeString(secret, "top-secret"); + + SkillSource source = new LocalSkillSource(skillsBase); + + // An absolute skill name must be rejected outright. + var single = source.loadResource(secret.toAbsolutePath().toString(), ""); + RuntimeException exception = assertThrows(RuntimeException.class, single::blockingGet); + assertThat(exception).hasCauseThat().isInstanceOf(SkillSourceException.class); + assertThat(exception) + .hasCauseThat() + .hasMessageThat() + .contains("Absolute paths are not allowed"); + } }