From 3967cfa6297530e8274fad4ab0ec833525c2db69 Mon Sep 17 00:00:00 2001 From: Damian Momot Date: Thu, 16 Jul 2026 06:42:26 -0700 Subject: [PATCH] fix: confine config-driven dynamic class loading to intended types Config-driven loaders confine every reflectively loaded class to the intended type (BaseTool, BaseToolset, BaseExampleProvider, or the caller's expectedType) before constructing it or reading a static field, so a non-intended type is never instantiated nor its static initializer run (type-confinement, not a sandbox). CompiledAgentLoader also gains source-dir confinement (adk.agents.confine-to-source-dir), keeping compiled agents within source-dir. Type confinement is on by default; source-dir confinement is off by default (with a warning) to avoid breaking existing setups, and will default on in a future release. PiperOrigin-RevId: 948941874 --- .../com/google/adk/agents/ToolResolver.java | 12 ++ .../com/google/adk/tools/ExampleTool.java | 6 + .../google/adk/agents/ToolResolverTest.java | 118 ++++++++++++++++++ .../com/google/adk/tools/ExampleToolTest.java | 28 +++++ .../google/adk/web/CompiledAgentLoader.java | 43 +++++++ .../web/config/AgentLoadingProperties.java | 13 ++ .../adk/web/CompiledAgentLoaderTest.java | 77 ++++++++++++ .../java/com/google/adk/maven/WebMojo.java | 17 ++- 8 files changed, 311 insertions(+), 3 deletions(-) create mode 100644 dev/src/test/java/com/google/adk/web/CompiledAgentLoaderTest.java diff --git a/core/src/main/java/com/google/adk/agents/ToolResolver.java b/core/src/main/java/com/google/adk/agents/ToolResolver.java index 09a3d79c1..ad2b1b7b7 100644 --- a/core/src/main/java/com/google/adk/agents/ToolResolver.java +++ b/core/src/main/java/com/google/adk/agents/ToolResolver.java @@ -273,6 +273,7 @@ static BaseToolset resolveToolsetFromClass( // Try reflection to get class try { Class clazz = Thread.currentThread().getContextClassLoader().loadClass(className); + // Confine to BaseToolset: a non-intended type is never constructed (not a sandbox). if (BaseToolset.class.isAssignableFrom(clazz)) { toolsetClass = clazz.asSubclass(BaseToolset.class); // Optimization: register for reuse @@ -349,6 +350,11 @@ static BaseToolset resolveToolsetInstanceViaReflection(String toolsetName) try { Field field = clazz.getField(fieldName); + // Confine to BaseToolset before field.get() runs its static initializer (not a sandbox). + if (!BaseToolset.class.isAssignableFrom(field.getType())) { + logger.debug("Field {} in class {} is not a BaseToolset field", fieldName, className); + return null; + } if (!Modifier.isStatic(field.getModifiers())) { logger.debug("Field {} in class {} is not static", fieldName, className); return null; @@ -398,6 +404,7 @@ static BaseTool resolveToolFromClass(String className, ToolArgsConfig args, Stri // Try reflection to get class try { Class clazz = Thread.currentThread().getContextClassLoader().loadClass(className); + // Confine to BaseTool: a non-intended type is never constructed (not a sandbox). if (BaseTool.class.isAssignableFrom(clazz)) { toolClass = clazz.asSubclass(BaseTool.class); // Optimization: register for reuse @@ -495,6 +502,11 @@ static BaseTool resolveInstanceViaReflection(String toolName) try { Field field = clazz.getField(fieldName); + // Confine to BaseTool before field.get() runs its static initializer (not a sandbox). + if (!BaseTool.class.isAssignableFrom(field.getType())) { + logger.debug("Field {} in class {} is not a BaseTool field", fieldName, className); + return null; + } if (!Modifier.isStatic(field.getModifiers())) { logger.debug("Field {} in class {} is not static", fieldName, className); return null; diff --git a/core/src/main/java/com/google/adk/tools/ExampleTool.java b/core/src/main/java/com/google/adk/tools/ExampleTool.java index c11259c52..0184c3593 100644 --- a/core/src/main/java/com/google/adk/tools/ExampleTool.java +++ b/core/src/main/java/com/google/adk/tools/ExampleTool.java @@ -135,6 +135,12 @@ private static BaseExampleProvider resolveExampleProvider(String ref) try { Class clazz = Thread.currentThread().getContextClassLoader().loadClass(className); Field field = clazz.getField(fieldName); + // Confine to BaseExampleProvider before field.get() runs its static initializer (not a + // sandbox). + if (!BaseExampleProvider.class.isAssignableFrom(field.getType())) { + throw new ConfigurationException( + "Field '" + fieldName + "' in class '" + className + "' is not a BaseExampleProvider"); + } if (!Modifier.isStatic(field.getModifiers())) { throw new ConfigurationException( "Field '" + fieldName + "' in class '" + className + "' is not static"); diff --git a/core/src/test/java/com/google/adk/agents/ToolResolverTest.java b/core/src/test/java/com/google/adk/agents/ToolResolverTest.java index b4bd70491..c3834ff00 100644 --- a/core/src/test/java/com/google/adk/agents/ToolResolverTest.java +++ b/core/src/test/java/com/google/adk/agents/ToolResolverTest.java @@ -29,6 +29,7 @@ import com.google.genai.types.FunctionDeclaration; import io.reactivex.rxjava3.core.Flowable; import java.util.Optional; +import java.util.concurrent.atomic.AtomicBoolean; import org.junit.After; import org.junit.Before; import org.junit.BeforeClass; @@ -209,6 +210,67 @@ public void testResolveToolInstance_withInvalidReflectionPath_returnsNull() { assertThat(resolved).isNull(); } + @Test + public void resolveInstanceViaReflection_nonIntendedType_isRejectedWithoutInitializing() + throws Exception { + // A non-intended type (not a BaseTool) is rejected before the field is read, so its static + // initializer never runs. + String toolName = NonToolWithStaticInit.class.getName() + ".NOT_A_TOOL"; + + BaseTool resolved = ToolResolver.resolveInstanceViaReflection(toolName); + + assertThat(resolved).isNull(); + assertThat(nonToolInitFired.get()).isFalse(); + } + + @Test + public void resolveToolInstance_nonIntendedType_isRejectedWithoutInitializing() { + // Same guarantee through the public resolveToolInstance entry point. + String toolName = NonToolWithStaticInit.class.getName() + ".NOT_A_TOOL"; + + BaseTool resolved = ToolResolver.resolveToolInstance(toolName); + + assertThat(resolved).isNull(); + assertThat(nonToolInitFired.get()).isFalse(); + } + + @Test + public void resolveToolsetInstanceViaReflection_nonIntendedType_isRejectedWithoutInitializing() + throws Exception { + String toolsetName = NonToolsetWithStaticInit.class.getName() + ".NOT_A_TOOLSET"; + + BaseToolset resolved = ToolResolver.resolveToolsetInstanceViaReflection(toolsetName); + + assertThat(resolved).isNull(); + assertThat(nonToolsetInitFired.get()).isFalse(); + } + + @Test + public void resolveInstanceViaReflection_properToolType_stillLoadsEvenWithSideEffects() + throws Exception { + // This guard is type-confinement, not a sandbox: a proper BaseTool type is still loaded and its + // static initializer still runs. Only non-intended types are rejected. + String toolName = SideEffectingProperTool.class.getName() + ".INSTANCE"; + + BaseTool resolved = ToolResolver.resolveInstanceViaReflection(toolName); + + assertThat(resolved).isNotNull(); + assertThat(properToolInitFired.get()).isTrue(); + } + + @Test + public void resolveInstanceViaReflection_holderClassWithToolField_stillResolves() + throws Exception { + // A non-tool holder class exposing a BaseTool-typed static field is still supported (mirrors + // module-level instance references), since the field type is confined to BaseTool. + String toolName = ToolHolder.class.getName() + ".HELD_TOOL"; + + BaseTool resolved = ToolResolver.resolveInstanceViaReflection(toolName); + + assertThat(resolved).isNotNull(); + assertThat(resolved).isSameInstanceAs(ToolHolder.HELD_TOOL); + } + @Test public void testResolveToolFromClass_withFromConfigMethod() throws Exception { String className = TestToolWithFromConfig.class.getName(); @@ -451,6 +513,62 @@ public static final class TestClassWithNonToolField { private TestClassWithNonToolField() {} } + // Side-effect channels flipped by the helper classes' static initializers. They live on the test + // class so assertions can observe them without touching (and thereby initializing) those classes. + private static final AtomicBoolean nonToolInitFired = new AtomicBoolean(false); + private static final AtomicBoolean nonToolsetInitFired = new AtomicBoolean(false); + private static final AtomicBoolean properToolInitFired = new AtomicBoolean(false); + + /** Non-intended type (not a BaseTool) with a side-effecting static initializer. */ + public static final class NonToolWithStaticInit { + public static final String NOT_A_TOOL = "not a tool"; + + static { + nonToolInitFired.set(true); + } + + private NonToolWithStaticInit() {} + } + + /** Non-intended type (not a BaseToolset) with a side-effecting static initializer. */ + public static final class NonToolsetWithStaticInit { + public static final String NOT_A_TOOLSET = "not a toolset"; + + static { + nonToolsetInitFired.set(true); + } + + private NonToolsetWithStaticInit() {} + } + + /** + * A proper BaseTool type with a side-effecting static initializer. Documents that the guard is + * type-confinement, not a sandbox: a class of the intended type is still loaded and initialized. + */ + public static final class SideEffectingProperTool extends BaseTool { + public static final SideEffectingProperTool INSTANCE = new SideEffectingProperTool(); + + static { + properToolInitFired.set(true); + } + + private SideEffectingProperTool() { + super("side_effecting_tool", "Proper tool with a side-effecting static initializer"); + } + + @Override + public Optional declaration() { + return Optional.empty(); + } + } + + /** Non-tool holder exposing a BaseTool-typed static field (module-style reference). */ + public static final class ToolHolder { + public static final BaseTool HELD_TOOL = new TestToolWithDefaultConstructor(); + + private ToolHolder() {} + } + private BaseTool.ToolConfig createToolConfig(String name, BaseTool.ToolArgsConfig args) { return new BaseTool.ToolConfig(name, args); } diff --git a/core/src/test/java/com/google/adk/tools/ExampleToolTest.java b/core/src/test/java/com/google/adk/tools/ExampleToolTest.java index e56afe60b..7f06407fc 100644 --- a/core/src/test/java/com/google/adk/tools/ExampleToolTest.java +++ b/core/src/test/java/com/google/adk/tools/ExampleToolTest.java @@ -33,6 +33,7 @@ import com.google.genai.types.Content; import com.google.genai.types.Part; import io.reactivex.rxjava3.core.Flowable; +import java.util.concurrent.atomic.AtomicBoolean; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -306,6 +307,33 @@ static final class WrongTypeProviderHolder { private WrongTypeProviderHolder() {} } + // Side-effect channel flipped by the helper class's static initializer. It lives on the test + // class so the assertion can observe it without initializing that class. + private static final AtomicBoolean nonProviderInitFired = new AtomicBoolean(false); + + /** Non-intended type (not a BaseExampleProvider) with a side-effecting static initializer. */ + static final class NonProviderWithStaticInit { + public static final String NOT_A_PROVIDER = "not a provider"; + + static { + nonProviderInitFired.set(true); + } + + private NonProviderWithStaticInit() {} + } + + @Test + public void fromConfig_withNonIntendedType_isRejectedWithoutInitializing() { + BaseTool.ToolArgsConfig args = new BaseTool.ToolArgsConfig(); + args.setAdditionalProperty( + "examples", ExampleToolTest.NonProviderWithStaticInit.class.getName() + ".NOT_A_PROVIDER"); + + // A non-intended type is rejected before the field is read, so its static initializer never + // runs. A proper BaseExampleProvider would still load, even if it were modified. + assertThrows(ConfigurationException.class, () -> ExampleTool.fromConfig(args)); + assertThat(nonProviderInitFired.get()).isFalse(); + } + @Test public void declaration_isEmpty() { ExampleTool tool = ExampleTool.builder().build(); diff --git a/dev/src/main/java/com/google/adk/web/CompiledAgentLoader.java b/dev/src/main/java/com/google/adk/web/CompiledAgentLoader.java index 7e7146c20..87f250d8f 100644 --- a/dev/src/main/java/com/google/adk/web/CompiledAgentLoader.java +++ b/dev/src/main/java/com/google/adk/web/CompiledAgentLoader.java @@ -18,6 +18,7 @@ import com.google.adk.agents.BaseAgent; import com.google.adk.web.config.AgentLoadingProperties; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Suppliers; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -74,6 +75,9 @@ * */ @Service("agentLoader") @@ -94,6 +98,13 @@ public CompiledAgentLoader(AgentLoadingProperties properties) { // Load path-based agents if (properties.getSourceDir() != null && !properties.getSourceDir().isEmpty()) { + if (!properties.isConfineToSourceDir()) { + logger.warn( + "Directory confinement is disabled (adk.agents.confine-to-source-dir=false); compiled" + + " agents may be loaded from outside the configured source-dir via symlinks or" + + " build-output dirs. Set adk.agents.confine-to-source-dir=true to restrict" + + " loading to the source tree."); + } loadPathBasedAgents(allAgents); } else { logger.warn( @@ -220,6 +231,10 @@ private void loadAgentsFromDirectory(Path directory, Map files = Files.list(directory)) { diff --git a/dev/src/main/java/com/google/adk/web/config/AgentLoadingProperties.java b/dev/src/main/java/com/google/adk/web/config/AgentLoadingProperties.java index 85631a0ae..bfdbca50e 100644 --- a/dev/src/main/java/com/google/adk/web/config/AgentLoadingProperties.java +++ b/dev/src/main/java/com/google/adk/web/config/AgentLoadingProperties.java @@ -26,6 +26,11 @@ public class AgentLoadingProperties { private String sourceDir = "."; private String[] buildOutputDirs = {"target/classes", "build/classes/java/main", "build/classes"}; + // When true, compiled agents are only loaded from within sourceDir, so a build-output dir or + // symlink cannot escape it. Off by default to preserve existing behavior (a warning is logged + // while disabled); a no-op for normal layouts, which already live under sourceDir. + private boolean confineToSourceDir = false; + public String getSourceDir() { return sourceDir; } @@ -41,4 +46,12 @@ public String[] getBuildOutputDirs() { public void setBuildOutputDirs(String[] buildOutputDirs) { this.buildOutputDirs = buildOutputDirs; } + + public boolean isConfineToSourceDir() { + return confineToSourceDir; + } + + public void setConfineToSourceDir(boolean confineToSourceDir) { + this.confineToSourceDir = confineToSourceDir; + } } diff --git a/dev/src/test/java/com/google/adk/web/CompiledAgentLoaderTest.java b/dev/src/test/java/com/google/adk/web/CompiledAgentLoaderTest.java new file mode 100644 index 000000000..ddefbbeb4 --- /dev/null +++ b/dev/src/test/java/com/google/adk/web/CompiledAgentLoaderTest.java @@ -0,0 +1,77 @@ +/* + * Copyright 2025 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.adk.web; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import com.google.adk.web.config.AgentLoadingProperties; +import java.nio.file.Files; +import java.nio.file.Path; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +/** Tests for {@link CompiledAgentLoader}, focused on the opt-in directory confinement. */ +public class CompiledAgentLoaderTest { + + @Test + public void confineToSourceDir_defaultsOff() { + // Off by default to preserve existing behavior; a warning is logged recommending it be enabled. + assertFalse(new AgentLoadingProperties().isConfineToSourceDir()); + } + + @Test + public void isDirWithinSourceRoot_confinementOff_allowsOutsideDir(@TempDir Path tmp) + throws Exception { + Path root = Files.createDirectory(tmp.resolve("root")); + Path outside = Files.createDirectory(tmp.resolve("outside")); + CompiledAgentLoader loader = newLoader(root, /* confine= */ false); + + assertTrue(loader.isDirWithinSourceRoot(outside)); + } + + @Test + public void isDirWithinSourceRoot_confinementOn_blocksOutsideAllowsInside(@TempDir Path tmp) + throws Exception { + Path root = Files.createDirectory(tmp.resolve("root")); + Path inside = Files.createDirectories(root.resolve("target").resolve("classes")); + Path outside = Files.createDirectory(tmp.resolve("outside")); + CompiledAgentLoader loader = newLoader(root, /* confine= */ true); + + assertTrue(loader.isDirWithinSourceRoot(inside)); + assertFalse(loader.isDirWithinSourceRoot(outside)); + } + + @Test + public void isDirWithinSourceRoot_confinementOn_blocksSymlinkEscape(@TempDir Path tmp) + throws Exception { + Path root = Files.createDirectory(tmp.resolve("root")); + Path outside = Files.createDirectory(tmp.resolve("outside")); + // A symlink inside the source root that points outside it must not be accepted. + Path escape = Files.createSymbolicLink(root.resolve("escape"), outside); + CompiledAgentLoader loader = newLoader(root, /* confine= */ true); + + assertFalse(loader.isDirWithinSourceRoot(escape)); + } + + private static CompiledAgentLoader newLoader(Path sourceDir, boolean confine) { + AgentLoadingProperties props = new AgentLoadingProperties(); + props.setSourceDir(sourceDir.toString()); + props.setConfineToSourceDir(confine); + return new CompiledAgentLoader(props); + } +} diff --git a/maven_plugin/src/main/java/com/google/adk/maven/WebMojo.java b/maven_plugin/src/main/java/com/google/adk/maven/WebMojo.java index 97c3f1e4a..78935155a 100644 --- a/maven_plugin/src/main/java/com/google/adk/maven/WebMojo.java +++ b/maven_plugin/src/main/java/com/google/adk/maven/WebMojo.java @@ -376,6 +376,16 @@ private T tryLoadFromStaticField(String classAndField, Class expectedType Class clazz = projectClassLoader.loadClass(className); Field field = clazz.getField(fieldName); + // Confine to expectedType before field.get() runs its static initializer (not a sandbox). + if (!expectedType.isAssignableFrom(field.getType())) { + throw new MojoExecutionException( + "Field " + + fieldName + + " in class " + + className + + " is not an instance of " + + expectedType.getSimpleName()); + } Object instance = field.get(null); if (!expectedType.isInstance(instance)) { throw new MojoExecutionException( @@ -410,12 +420,13 @@ private T tryLoadFromConstructor(String className, Class expectedType) throws MojoExecutionException { try { Class clazz = projectClassLoader.loadClass(className); - Object instance = clazz.getDeclaredConstructor().newInstance(); - if (!expectedType.isInstance(instance)) { + // Confine to expectedType before constructing it (not a sandbox). + if (!expectedType.isAssignableFrom(clazz)) { throw new MojoExecutionException( "Class " + className + " does not implement/extend " + expectedType.getSimpleName()); } - return expectedType.cast(instance); + // The isAssignableFrom check above guarantees the constructed instance is an expectedType. + return expectedType.cast(clazz.getDeclaredConstructor().newInstance()); } catch (ClassNotFoundException e) { throw new MojoExecutionException(