From 9a23c53c918d0202cc3dc18343ee8d57965e24fc Mon Sep 17 00:00:00 2001 From: meraklbz Date: Tue, 11 Aug 2026 14:44:49 +0200 Subject: [PATCH] fix(server-common): reject CR/LF in push notification headers (CWE-113) - Validate X-A2A-Notification-Token and Authorization values before dispatch - Mark AuthenticationInfo.credentials as @Nullable per the protocol spec Signed-off-by: Emmanuel Hugonnet --- server-common/pom.xml | 5 ++ .../tasks/BasePushNotificationSender.java | 58 ++++++++++++++- .../tasks/PushNotificationSenderTest.java | 72 +++++++++++++++++++ .../sdk/spec/AuthenticationInfo.java | 3 +- 4 files changed, 134 insertions(+), 4 deletions(-) diff --git a/server-common/pom.xml b/server-common/pom.xml index 9fa1d2da7..ea239f9b9 100644 --- a/server-common/pom.xml +++ b/server-common/pom.xml @@ -87,6 +87,11 @@ junit-jupiter-api test + + org.junit.jupiter + junit-jupiter-params + test + org.mockito mockito-core diff --git a/server-common/src/main/java/org/a2aproject/sdk/server/tasks/BasePushNotificationSender.java b/server-common/src/main/java/org/a2aproject/sdk/server/tasks/BasePushNotificationSender.java index b0f6e38d8..143f68842 100644 --- a/server-common/src/main/java/org/a2aproject/sdk/server/tasks/BasePushNotificationSender.java +++ b/server-common/src/main/java/org/a2aproject/sdk/server/tasks/BasePushNotificationSender.java @@ -19,6 +19,7 @@ import org.a2aproject.sdk.client.http.A2AHttpClient; import org.a2aproject.sdk.client.http.A2AHttpClientFactory; import org.a2aproject.sdk.jsonrpc.common.json.JsonUtil; +import org.a2aproject.sdk.spec.AuthenticationInfo; import org.a2aproject.sdk.spec.ListTaskPushNotificationConfigsParams; import org.a2aproject.sdk.spec.ListTaskPushNotificationConfigsResult; import org.a2aproject.sdk.spec.Message; @@ -194,11 +195,27 @@ private boolean dispatchNotification(StreamingEventKind event, A2AHttpClient.PostBuilder postBuilder = httpClient.createPost(); if (token != null && !token.isBlank()) { + try { + rejectCrlf(token, X_A2A_NOTIFICATION_TOKEN); + } catch (IllegalArgumentException e) { + LOGGER.warn("Rejecting push notification to {}: {}", url, e.getMessage()); + return false; + } postBuilder.addHeader(X_A2A_NOTIFICATION_TOKEN, token); } - if (pushInfo.authentication() != null && pushInfo.authentication().credentials() != null) { - postBuilder.addHeader("Authorization", - pushInfo.authentication().scheme() + " " + pushInfo.authentication().credentials()); + AuthenticationInfo authentication = pushInfo.authentication(); + if (authentication != null) { + String credentials = authentication.credentials(); + if (credentials != null) { + String authorizationHeader; + try { + authorizationHeader = buildAuthorizationHeader(authentication.scheme(), credentials); + } catch (IllegalArgumentException e) { + LOGGER.warn("Rejecting push notification to {}: {}", url, e.getMessage()); + return false; + } + postBuilder.addHeader("Authorization", authorizationHeader); + } } try { @@ -213,4 +230,39 @@ private boolean dispatchNotification(StreamingEventKind event, } return true; } + + /** + * Builds the Authorization header value for a push notification config. + * + *

The {@code scheme} and {@code credentials} are client-controlled values that are + * concatenated directly into the header. Rejecting CR/LF characters here prevents + * HTTP header injection (CWE-113). The {@link A2AHttpClient} SPI is pluggable, so we + * cannot rely on every implementation (or the underlying HTTP client) to validate + * header values.

+ * + * @param scheme the authentication scheme + * @param credentials the authentication credentials + * @return the assembled {@code "scheme credentials"} header value + * @throws IllegalArgumentException if either field contains CR or LF + */ + private static String buildAuthorizationHeader(String scheme, String credentials) { + rejectCrlf(scheme, "Authorization scheme"); + rejectCrlf(credentials, "Authorization credentials"); + return scheme + " " + credentials; + } + + /** + * Throws {@link IllegalArgumentException} if {@code value} contains CR or LF. + * + *

Prevents HTTP header injection (CWE-113) for client-controlled header values.

+ * + * @param value non-null string to validate + * @param label human-readable description of the field, used in the exception message + */ + private static void rejectCrlf(String value, String label) { + if (value.indexOf('\r') >= 0 || value.indexOf('\n') >= 0) { + throw new IllegalArgumentException( + label + " must not contain CR/LF characters"); + } + } } diff --git a/server-common/src/test/java/org/a2aproject/sdk/server/tasks/PushNotificationSenderTest.java b/server-common/src/test/java/org/a2aproject/sdk/server/tasks/PushNotificationSenderTest.java index 87226524d..2bf4a531f 100644 --- a/server-common/src/test/java/org/a2aproject/sdk/server/tasks/PushNotificationSenderTest.java +++ b/server-common/src/test/java/org/a2aproject/sdk/server/tasks/PushNotificationSenderTest.java @@ -15,6 +15,7 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.function.Consumer; +import java.util.stream.Stream; import org.a2aproject.sdk.client.http.A2AHttpClient; import org.a2aproject.sdk.client.http.A2AHttpResponse; @@ -23,6 +24,7 @@ import org.a2aproject.sdk.jsonrpc.common.json.JsonProcessingException; import org.a2aproject.sdk.jsonrpc.common.json.JsonUtil; import org.a2aproject.sdk.spec.Artifact; +import org.a2aproject.sdk.spec.AuthenticationInfo; import org.a2aproject.sdk.spec.Message; import org.a2aproject.sdk.spec.StreamingEventKind; import org.a2aproject.sdk.spec.Task; @@ -35,6 +37,9 @@ import org.jspecify.annotations.Nullable; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; public class PushNotificationSenderTest { @@ -516,4 +521,71 @@ public void testSendNotificationSkipsWhenFormatterReturnsNull() throws Interrupt assertTrue(testHttpClient.rawBodies.isEmpty()); } + + @Test + public void testSendNotificationRejectsCrlfInToken() { + String taskId = "task_send_crlf_token"; + Task taskData = createSampleTask(taskId, TaskState.TASK_STATE_COMPLETED); + TaskPushNotificationConfig config = createSamplePushConfig(taskId, "http://notify.me/here", "cfg-crlf-token", + "token\r\nX-Injected: 1"); + configStore.setInfo(config); + + // No latch needed: sendNotification() calls dispatchResult.get(), which blocks until + // all CompletableFuture dispatches complete, including the CRLF rejection path. + sender.sendNotification(taskData, null); + + assertTrue(testHttpClient.events.isEmpty(), "Notification with CRLF token must not be dispatched"); + assertTrue(testHttpClient.headers.isEmpty(), "No headers should have been sent"); + assertTrue(testHttpClient.rawBodies.isEmpty(), "No body should have been sent"); + } + + @Test + public void testSendNotificationWithAuthHeader() throws InterruptedException { + String taskId = "task_send_auth"; + Task taskData = createSampleTask(taskId, TaskState.TASK_STATE_COMPLETED); + TaskPushNotificationConfig config = TaskPushNotificationConfig.builder() + .url("http://notify.me/here") + .id("cfg-auth") + .taskId(taskId) + .authentication(new AuthenticationInfo("Bearer", "token123")) + .build(); + configStore.setInfo(config); + + testHttpClient.latch = new CountDownLatch(1); + sender.sendNotification(taskData, null); + + assertTrue(testHttpClient.latch.await(5, TimeUnit.SECONDS), "HTTP call should complete within 5 seconds"); + assertEquals(1, testHttpClient.events.size()); + assertEquals(1, testHttpClient.headers.size()); + Map sentHeaders = testHttpClient.headers.get(0); + assertEquals("Bearer token123", sentHeaders.get("Authorization")); + } + + static Stream crlfAuthVectors() { + return Stream.of( + Arguments.of("CRLF in credentials", "Bearer", "token\r\nX-Injected: 1"), + Arguments.of("LF in scheme", "Bearer\nX-Injected: 1", "token"), + Arguments.of("bare CR in credentials", "Bearer", "token\rX-Injected: 1"), + Arguments.of("bare CR in scheme", "Bearer\rX-Injected: 1", "token")); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("crlfAuthVectors") + public void testSendNotificationRejectsCrlfInAuthFields(String description, String scheme, String credentials) { + String taskId = "task_send_crlf_auth"; + Task taskData = createSampleTask(taskId, TaskState.TASK_STATE_COMPLETED); + TaskPushNotificationConfig config = TaskPushNotificationConfig.builder() + .url("http://notify.me/here") + .id("cfg-crlf") + .taskId(taskId) + .authentication(new AuthenticationInfo(scheme, credentials)) + .build(); + configStore.setInfo(config); + + sender.sendNotification(taskData, null); + + assertTrue(testHttpClient.events.isEmpty(), "Notification with " + description + " must not be dispatched"); + assertTrue(testHttpClient.headers.isEmpty(), "No headers should have been sent"); + assertTrue(testHttpClient.rawBodies.isEmpty(), "No body should have been sent"); + } } diff --git a/spec/src/main/java/org/a2aproject/sdk/spec/AuthenticationInfo.java b/spec/src/main/java/org/a2aproject/sdk/spec/AuthenticationInfo.java index d54ae2a77..bfcf7a67d 100644 --- a/spec/src/main/java/org/a2aproject/sdk/spec/AuthenticationInfo.java +++ b/spec/src/main/java/org/a2aproject/sdk/spec/AuthenticationInfo.java @@ -2,6 +2,7 @@ import org.a2aproject.sdk.util.Assert; +import org.jspecify.annotations.Nullable; /** * Authentication information for agent authentication and push notification endpoints. @@ -21,7 +22,7 @@ * @see SecurityScheme for security scheme definitions * @see A2A Protocol Specification */ -public record AuthenticationInfo(String scheme, String credentials) { +public record AuthenticationInfo(String scheme, @Nullable String credentials) { /** * Compact constructor that validates required fields.