From a3df4632c19d857552af3d2c38373aabea9069de Mon Sep 17 00:00:00 2001 From: Google Team Member Date: Wed, 5 Aug 2026 15:15:04 -0700 Subject: [PATCH] fix: stop returning exception text to remote A2A peers `AgentExecutor#failedMessage` built the failure message sent back to the peer out of `Throwable#getMessage`. Exception text routinely contains absolute paths, class and module locations, configuration values and echoed request payloads, so a peer could map the host by provoking errors. It now logs the throwable under a short opaque id and sends only a fixed summary plus that id. It also took over the logging that used to happen at the call site, so the id in the response is the id in the log. Set `ADK_DEBUG_ERRORS=1` to get the detail back when debugging locally. PiperOrigin-RevId: 959895666 --- .../adk/a2a/executor/AgentExecutor.java | 86 ++++++++++++++++--- .../adk/a2a/executor/AgentExecutorTest.java | 56 +++++++++++- 2 files changed, 128 insertions(+), 14 deletions(-) diff --git a/a2a/src/main/java/com/google/adk/a2a/executor/AgentExecutor.java b/a2a/src/main/java/com/google/adk/a2a/executor/AgentExecutor.java index 57a0d9db2..618888c4c 100644 --- a/a2a/src/main/java/com/google/adk/a2a/executor/AgentExecutor.java +++ b/a2a/src/main/java/com/google/adk/a2a/executor/AgentExecutor.java @@ -29,6 +29,7 @@ import com.google.adk.runner.Runner; import com.google.adk.sessions.BaseSessionService; import com.google.adk.sessions.Session; +import com.google.common.base.Ascii; import com.google.common.collect.ImmutableList; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.genai.types.Content; @@ -66,6 +67,18 @@ public class AgentExecutor implements io.a2a.server.agentexecution.AgentExecutor private static final Logger logger = LoggerFactory.getLogger(AgentExecutor.class); private static final String USER_ID_PREFIX = "A2A_USER_"; private static final String A2A_METADATA_KEY = "a2a_metadata"; + + /** + * Env var that puts exception text back into the failure message sent to the peer. + * + *

Off by default, and meant for local debugging only: enabling it on a network-reachable + * deployment restores the disclosure {@link #failedMessage} exists to prevent. + */ + private static final String DEBUG_ERRORS_ENV_VAR = "ADK_DEBUG_ERRORS"; + + /** Length of the correlation id, matching {@code new_error_id()} in adk-python. */ + private static final int ERROR_ID_LENGTH = 12; + private final Map activeTasks = new ConcurrentHashMap<>(); private final Runner.Builder runnerBuilder; private final AgentExecutorConfig agentExecutorConfig; @@ -232,13 +245,7 @@ public void execute(RequestContext ctx, EventQueue eventQueue) { .ignoreElements() .materialize() .flatMapCompletable( - notification -> { - Throwable error = notification.getError(); - if (error != null) { - logger.error("Runner failed to execute", error); - } - return handleExecutionEnd(ctx, error, eventQueue); - }) + notification -> handleExecutionEnd(ctx, notification.getError(), eventQueue)) .doFinally(() -> cleanupTask(ctx.getTaskId())) .subscribe( () -> {}, @@ -250,7 +257,16 @@ public void execute(RequestContext ctx, EventQueue eventQueue) { private Completable handleExecutionEnd( RequestContext ctx, Throwable error, EventQueue eventQueue) { TaskState state = error != null ? TaskState.FAILED : TaskState.COMPLETED; - Message message = error != null ? failedMessage(ctx, error) : null; + Message message = null; + if (error != null) { + // The peer is not trusted with the throwable: exception text routinely names absolute + // filesystem paths, class and module locations, configuration values and echoed request + // payloads, none of which the caller needs and all of which are useful reconnaissance. It is + // logged here in full under a short opaque id; the peer gets only that id. + String errorId = newErrorId(); + logger.error("Runner failed to execute [error_id={}]", errorId, error); + message = failedMessage(ctx, error, errorId); + } TaskStatusUpdateEvent initialEvent = new TaskStatusUpdateEvent.Builder() .taskId(ctx.getTaskId()) @@ -304,16 +320,66 @@ private Maybe prepareSession( })); } - private static Message failedMessage(RequestContext context, Throwable e) { + /** + * Builds the failure message handed back to the remote peer. + * + *

It carries {@code errorId} rather than {@code e.getMessage()}, so an operator handed the id + * can find the real stack trace in the log while the peer learns nothing about the host. Set + * {@code ADK_DEBUG_ERRORS=1} to put the exception text back into the response while debugging + * locally. + */ + private static Message failedMessage(RequestContext context, Throwable e, String errorId) { return new Message.Builder() .messageId(UUID.randomUUID().toString()) .contextId(context.getContextId()) .taskId(context.getTaskId()) .role(Message.Role.AGENT) - .parts(ImmutableList.of(new TextPart(e.getMessage()))) + .parts(ImmutableList.of(new TextPart(failureText(e, errorId, debugErrorsEnabled())))) .build(); } + /** + * Returns a short opaque id tying the peer's failure message to the logged throwable. + * + *

{@value #ERROR_ID_LENGTH} hex characters, the same shape as {@code new_error_id()} in + * adk-python, so an operator sees the same kind of id whichever runtime produced it. + */ + private static String newErrorId() { + return UUID.randomUUID().toString().replace("-", "").substring(0, ERROR_ID_LENGTH); + } + + /** + * Returns the failure text that is safe to hand to the remote peer. + * + * @param includeDetail whether to append the exception type and message; see {@link + * #DEBUG_ERRORS_ENV_VAR}. + */ + static String failureText(Throwable e, String errorId, boolean includeDetail) { + String text = "Agent execution failed. (error_id: " + errorId + ")"; + if (includeDetail) { + text = text + ": " + e.getClass().getName() + ": " + e.getMessage(); + } + return text; + } + + private static boolean debugErrorsEnabled() { + return debugErrorsEnabled(System.getenv(DEBUG_ERRORS_ENV_VAR)); + } + + /** + * Returns whether {@code value}, as read from {@link #DEBUG_ERRORS_ENV_VAR}, turns the detail + * back on. Unset or unrecognized means off, matching {@code is_env_enabled} in adk-python. + * + *

Split from the env lookup so both outcomes are testable: {@code System.getenv} cannot be set + * from a test in-process. + */ + static boolean debugErrorsEnabled(String value) { + if (value == null) { + return false; + } + return value.equals("1") || Ascii.equalsIgnoreCase(value, "true"); + } + // Processor that will process all events related to the one runner invocation. private static class EventProcessor { private final String runArtifactId; diff --git a/a2a/src/test/java/com/google/adk/a2a/executor/AgentExecutorTest.java b/a2a/src/test/java/com/google/adk/a2a/executor/AgentExecutorTest.java index 99b12286e..68cb196f1 100644 --- a/a2a/src/test/java/com/google/adk/a2a/executor/AgentExecutorTest.java +++ b/a2a/src/test/java/com/google/adk/a2a/executor/AgentExecutorTest.java @@ -63,6 +63,10 @@ @RunWith(JUnit4.class) public final class AgentExecutorTest { + /** A throwable message shaped like the ones that leak host detail. */ + private static final String SECRET_ERROR = + "Runner error: /home/victim/.config/adk/credentials.json (No such file)"; + private EventQueue eventQueue; private List enqueuedEvents; private TestAgent testAgent; @@ -137,7 +141,7 @@ public void createAgentExecutor_noAgentExecutorConfig_throwsException() { public void execute_withBeforeExecuteCallback_cancelsExecutionOnError() { // If callback returns error, execution should stop/fail. Callbacks.BeforeExecuteCallback callback = - ctx -> Single.error(new RuntimeException("Cancelled")); + ctx -> Single.error(new RuntimeException(SECRET_ERROR)); AgentExecutorConfig config = AgentExecutorConfig.builder().beforeExecuteCallback(callback).build(); @@ -162,7 +166,10 @@ public void execute_withBeforeExecuteCallback_cancelsExecutionOnError() { assertThat(statusEvent.getStatus().state().toString()).isEqualTo("FAILED"); assertThat(statusEvent.getStatus().message().getParts().get(0)).isInstanceOf(TextPart.class); TextPart textPart = (TextPart) statusEvent.getStatus().message().getParts().get(0); - assertThat(textPart.getText()).contains("Cancelled"); + // The remote peer gets a correlation id for the logged throwable, not its + // message -- see AgentExecutor#failedMessage. + assertThat(textPart.getText()).startsWith("Agent execution failed. (error_id: "); + assertThat(textPart.getText()).doesNotContain(SECRET_ERROR); } @Test @@ -289,7 +296,7 @@ public void execute_withAfterExecuteCallback_modifiesStatus() { @Test public void execute_runnerFails_registersFailedEvent() { - testAgent.setEventsToEmit(Flowable.error(new RuntimeException("Runner error"))); + testAgent.setEventsToEmit(Flowable.error(new RuntimeException(SECRET_ERROR))); AgentExecutor executor = new AgentExecutor.Builder() .agentExecutorConfig(AgentExecutorConfig.builder().build()) @@ -316,7 +323,48 @@ public void execute_runnerFails_registersFailedEvent() { assertThat(statusEvent.getStatus().state()).isEqualTo(TaskState.FAILED); assertThat(statusEvent.getStatus().message().getParts().get(0)).isInstanceOf(TextPart.class); TextPart textPart = (TextPart) statusEvent.getStatus().message().getParts().get(0); - assertThat(textPart.getText()).isEqualTo("Runner error"); + // A runner failure is reported to the peer as a correlation id only: the + // throwable's message names host paths and is for the server log. The id is + // 12 hex characters, the shape adk-python emits. + assertThat(textPart.getText()) + .matches("Agent execution failed\\. \\(error_id: [0-9a-f]{12}\\)"); + assertThat(textPart.getText()).doesNotContain(SECRET_ERROR); + assertThat(textPart.getText()).doesNotContain("/home/victim"); + } + + @Test + public void failureText_withoutDebug_carriesOnlyTheCorrelationId() { + String text = AgentExecutor.failureText(new RuntimeException(SECRET_ERROR), "abc-123", false); + + assertThat(text).isEqualTo("Agent execution failed. (error_id: abc-123)"); + } + + @Test + public void failureText_withDebug_carriesTheThrowableDetail() { + // ADK_DEBUG_ERRORS=1 is the documented opt-in for local debugging. + String text = AgentExecutor.failureText(new RuntimeException(SECRET_ERROR), "abc-123", true); + + assertThat(text).startsWith("Agent execution failed. (error_id: abc-123): "); + assertThat(text).contains("java.lang.RuntimeException"); + assertThat(text).contains(SECRET_ERROR); + } + + @Test + public void debugErrorsEnabled_recognizesTheDocumentedValues() { + assertThat(AgentExecutor.debugErrorsEnabled("1")).isTrue(); + assertThat(AgentExecutor.debugErrorsEnabled("true")).isTrue(); + assertThat(AgentExecutor.debugErrorsEnabled("TRUE")).isTrue(); + assertThat(AgentExecutor.debugErrorsEnabled("True")).isTrue(); + } + + @Test + public void debugErrorsEnabled_defaultsToOff() { + // Anything else leaves the redaction in place, including an unset variable. + assertThat(AgentExecutor.debugErrorsEnabled(null)).isFalse(); + assertThat(AgentExecutor.debugErrorsEnabled("")).isFalse(); + assertThat(AgentExecutor.debugErrorsEnabled("0")).isFalse(); + assertThat(AgentExecutor.debugErrorsEnabled("false")).isFalse(); + assertThat(AgentExecutor.debugErrorsEnabled("yes")).isFalse(); } @Test