Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions core/src/main/java/com/google/adk/agents/ToolResolver.java
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down
6 changes: 6 additions & 0 deletions core/src/main/java/com/google/adk/tools/ExampleTool.java
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
118 changes: 118 additions & 0 deletions core/src/test/java/com/google/adk/agents/ToolResolverTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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<FunctionDeclaration> 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);
}
Expand Down
28 changes: 28 additions & 0 deletions core/src/test/java/com/google/adk/tools/ExampleToolTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down
43 changes: 43 additions & 0 deletions dev/src/main/java/com/google/adk/web/CompiledAgentLoader.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -74,6 +75,9 @@
* <ul>
* <li>To enable this loader: {@code adk.agents.loader=compiled}
* <li>To specify agent source directory: {@code adk.agents.source-dir=/path/to/agents}
* <li>Directory confinement (recommended, off by default): {@code
* adk.agents.confine-to-source-dir=true} restricts compiled classes to {@code source-dir}. A
* warning is logged while it is disabled.
* </ul>
*/
@Service("agentLoader")
Expand All @@ -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(
Expand Down Expand Up @@ -220,6 +231,10 @@ private void loadAgentsFromDirectory(Path directory, Map<String, Supplier<BaseAg
? directory
: findBuildOutputDir(directory);

if (!isDirWithinSourceRoot(classesDir)) {
return;
}

try (URLClassLoader classLoader =
new URLClassLoader(
new URL[] {classesDir.toUri().toURL()},
Expand Down Expand Up @@ -251,6 +266,34 @@ private void loadAgentsFromDirectory(Path directory, Map<String, Supplier<BaseAg
}
}

/**
* When confinement is enabled ({@code adk.agents.confine-to-source-dir=true}), returns true only
* if {@code classesDir}'s real path is inside {@code source-dir}, blocking a build-output dir or
* symlink from escaping the source tree. Off by default, so it is a no-op otherwise.
*/
@VisibleForTesting
boolean isDirWithinSourceRoot(Path classesDir) {
if (!properties.isConfineToSourceDir()) {
return true;
}
try {
Path root = Paths.get(properties.getSourceDir()).toRealPath();
Path real = classesDir.toRealPath();
if (real.startsWith(root)) {
return true;
}
logger.warn(
"Skipping directory outside the configured source-dir"
+ " (adk.agents.confine-to-source-dir=true): {} is not within {}",
real,
root);
return false;
} catch (IOException e) {
logger.warn("Could not verify that {} is within the source-dir; skipping it", classesDir, e);
return false;
}
}

/** Checks if directory contains .class files directly. */
private boolean hasClassFiles(Path directory) {
try (Stream<Path> files = Files.list(directory)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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;
}
}
Loading
Loading