Conversation
Replacing Jackson with Gson for json (de)serialization Fixes #<issue_number_goes_here> 🦕 --------- Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com> Co-authored-by: Kabir Khan <kkhan@redhat.com>
* chore: updating the workflows Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com> * chore: fixing javadoc Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com> * chore: Updating kafka version Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com> * fix; Fixing the last issues to be able to pass the TCK again Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com> * fix: Fixing the missing id in the jsonrpc response Extract request id before jsonrpc validation so error responses include top-level id. Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com> --------- Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
There was a problem hiding this comment.
Code Review
This pull request migrates the A2A Java SDK from Jackson to Gson, introducing a centralized JsonUtil and custom exceptions to decouple the SDK from specific JSON implementations. Feedback identifies a critical security vulnerability in JsonUtil related to unsafe class instantiation from untrusted input (CWE-470). The reviewer also recommends aligning the gRPC utility configuration with the central JsonUtil to ensure consistency, using long integers for IDs to prevent data truncation, and fixing serialization logic for complex error data. Additionally, suggestions are provided to clean up misplaced dependency exclusions in the POM and rename the OBJECT_MAPPER constant to a more library-agnostic term.
…pliance This commit addresses critical review feedback across security, data correctness, thread safety, and A2A spec compliance: **Security (CWE-470):** - Add whitelist validation before Class.forName() in ThrowableTypeAdapter to prevent arbitrary class loading vulnerability. Only allow java.lang.*, java.io.*, and io.a2a.* packages plus specific safe exception types. **Data Correctness:** - Fix JSON-RPC ID truncation: use getAsLong() instead of getAsInt() to handle IDs exceeding Integer.MAX_VALUE - Fix error data serialization: use GSON.toJson() instead of toString() to preserve JSON structure in error responses - Use shared JsonUtil.OBJECT_MAPPER in JSONRPCUtils for consistent type adapter support (OffsetDateTime, etc.) across all serialization paths **Thread Safety:** - Implement double-checked locking in getAgentCard() methods across Client, JSONRPCTransport, and RestTransport to prevent race conditions during lazy initialization - Declare agentCard and needsExtendedCard fields as volatile to ensure visibility across threads and prevent instruction reordering in double-checked locking pattern **A2A Spec Compliance:** - Fix intermittent null field serialization: JSON-RPC server now uses JsonUtil.toJson() instead of returning AgentCard object (which caused Quarkus to use Jackson, serializing nulls as "field": null instead of omitting them) - Remove leading slash from pushNotificationConfig resource name pattern to match REST API conventions **Code Quality:** - Improve error messages in JdkA2AHttpClient streaming handler (body not available in streaming context) - Add verification tests for AgentCard null field omission - Fix javadoc reference to use JsonUtil.OBJECT_MAPPER instead of Gson Resolves intermittent TCK failures related to null field serialization. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
… precision, and robustness This commit addresses the remaining HIGH and MEDIUM priority issues from the PR #808 review: **HIGH Priority - Circular Dependency (JsonUtil.java:333):** - Convert JSONRPCErrorTypeAdapter from TypeAdapter to TypeAdapterFactory to eliminate circular dependency during static initialization - The adapter now receives the Gson instance via the factory's create() method instead of referencing OBJECT_MAPPER during class initialization - Prevents potential NullPointerException if the adapter is invoked during initialization **MEDIUM Priority - ID Precision Loss (JsonUtil.java:1002):** - Fix JSON-RPC ID serialization to preserve fractional values (e.g., 1.5 remains 1.5) - Check number type: use longValue() for integer types (Long, Integer, Short, Byte), but preserve full precision for Double/Float by passing Number directly to JsonWriter - Prevents data loss when clients use fractional numbers as JSON-RPC IDs **MEDIUM Priority - JSON Parsing Robustness:** - RestErrorMapper.java:36-37: Add null and type checks before calling getAsString() to handle JsonNull and non-primitive types gracefully - JSONRPCUtils.java:160: Validate JSON is an object before calling getAsJsonObject() to provide clear error message for arrays or primitives - A2AServerRoutes.java:97-101: Remove redundant inner try-catch block that made outer JsonSyntaxException handler unreachable, simplifying error handling logic **MEDIUM Priority - Performance Optimization:** - JSONRPCUtils.java:299: Eliminate inefficient double conversion (serialize → deserialize) - Deserialize AgentCard directly from JsonElement using Gson.fromJson(JsonElement, Class) instead of converting to string first All changes maintain backward compatibility and improve error handling for malformed input. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Extract duplicated id-writing code into ensureId() helper method to reduce repetition in response serialization. Add @nullable annotations to handle null id values throughout the codebase per JSON-RPC 2.0 spec requirements. Fix error data serialization to use fromJson() instead of toString() to preserve proper JSON structure. Optimize params handling by using toJson() directly instead of inefficient serialize-parse roundtrip. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Eliminate inefficient toString() serialization followed by fromJson() parsing cycles by using Gson's direct JsonElement deserialization. Replace three instances of element.toString() + fromJson(String) with direct fromJson(JsonElement) calls in SSEEventListener and JsonUtil. Fix JSON-RPC 2.0 protocol compliance in A2AServerRoutes: validate that request body is a JSON object before calling getAsJsonObject(). Non-object JSON now correctly returns InvalidRequestError (-32600) instead of InternalError (-32603). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates the SDK from Jackson to a custom JSON abstraction layer using Gson, removing Jackson annotations from specification classes to ensure library independence. The changes include the introduction of JsonUtil for centralized serialization and JSONRPCUtils for JSON-RPC to Protocol Buffer conversion. Feedback indicates that the new caching logic in the Client class is redundant and potentially breaks authenticated extended card resolution. Furthermore, multiple locations in the JSON-RPC handling logic were identified where numeric IDs could be truncated, and several polymorphic deserialization paths were missing the AUTHENTICATED_EXTENDED_CARD_NOT_CONFIGURED_ERROR_CODE case.
| // Fast path - avoid synchronization if already initialized | ||
| if (agentCard != null) { | ||
| return agentCard; | ||
| } | ||
|
|
||
| synchronized (this) { | ||
| // Double-check inside synchronized block | ||
| if (agentCard == null) { | ||
| agentCard = clientTransport.getAgentCard(context); | ||
| } | ||
| return agentCard; | ||
| } |
There was a problem hiding this comment.
The caching logic introduced here prevents the client from ever receiving an updated or upgraded agent card (e.g., transitioning from a public card to an authenticated extended card). Since the underlying transport implementations (like JSONRPCTransport) already implement their own caching and handle the extended card resolution logic, this additional layer of caching in the Client class is redundant and breaks the authenticated extended card feature. It should be reverted to delegate directly to the transport.
agentCard = clientTransport.getAgentCard(context);
return agentCard;| if (idElement != null && !idElement.isJsonNull() && idElement.isJsonPrimitive()) { | ||
| com.google.gson.JsonPrimitive idPrimitive = idElement.getAsJsonPrimitive(); | ||
| requestId = idPrimitive.isNumber() ? idPrimitive.getAsLong() : idPrimitive.getAsString(); | ||
| } |
There was a problem hiding this comment.
The current logic for extracting the requestId will truncate fractional numbers (e.g., 1.5 becomes 1L). While the JSON-RPC 2.0 specification discourages fractional numbers for IDs, it does not forbid them. Using JsonUtil.OBJECT_MAPPER.fromJson(idElement, Object.class) would be safer as it uses the pre-configured number policy to preserve precision for both integers and floating-point numbers.
if (idElement != null && !idElement.isJsonNull() && idElement.isJsonPrimitive()) {
requestId = JsonUtil.OBJECT_MAPPER.fromJson(idElement, Object.class);
}There was a problem hiding this comment.
The ID precision issues are theoretical - the spec discourages fractional IDs and the code has worked this way since inception.
| switch (code) { | ||
| case JSON_PARSE_ERROR_CODE: | ||
| return new JSONParseError(code, message, data); | ||
| case INVALID_REQUEST_ERROR_CODE: | ||
| return new InvalidRequestError(code, message, data); | ||
| case METHOD_NOT_FOUND_ERROR_CODE: | ||
| return new MethodNotFoundError(code, message, data); | ||
| case INVALID_PARAMS_ERROR_CODE: | ||
| return new InvalidParamsError(code, message, data); | ||
| case INTERNAL_ERROR_CODE: | ||
| return new io.a2a.spec.InternalError(code, message, data); | ||
| case PUSH_NOTIFICATION_NOT_SUPPORTED_ERROR_CODE: | ||
| return new PushNotificationNotSupportedError(code, message, data); | ||
| case UNSUPPORTED_OPERATION_ERROR_CODE: | ||
| return new UnsupportedOperationError(code, message, data); | ||
| case CONTENT_TYPE_NOT_SUPPORTED_ERROR_CODE: | ||
| return new ContentTypeNotSupportedError(code, message, data); | ||
| case INVALID_AGENT_RESPONSE_ERROR_CODE: | ||
| return new InvalidAgentResponseError(code, message, data); | ||
| case TASK_NOT_CANCELABLE_ERROR_CODE: | ||
| return new TaskNotCancelableError(code, message, data); | ||
| case TASK_NOT_FOUND_ERROR_CODE: | ||
| return new TaskNotFoundError(code, message, data); | ||
| default: | ||
| return new JSONRPCError(code, message, data); | ||
| } | ||
| } |
There was a problem hiding this comment.
The switch statement is missing a case for AUTHENTICATED_EXTENDED_CARD_NOT_CONFIGURED_ERROR_CODE. This will cause such errors to be deserialized as a base JSONRPCError instead of the specific AuthenticatedExtendedCardNotConfiguredError subclass. Ensure the constant is used to maintain consistency across modules.
switch (code) {
case JSON_PARSE_ERROR_CODE:
return new JSONParseError(code, message, data);
case INVALID_REQUEST_ERROR_CODE:
return new InvalidRequestError(code, message, data);
case METHOD_NOT_FOUND_ERROR_CODE:
return new MethodNotFoundError(code, message, data);
case INVALID_PARAMS_ERROR_CODE:
return new InvalidParamsError(code, message, data);
case INTERNAL_ERROR_CODE:
return new io.a2a.spec.InternalError(code, message, data);
case PUSH_NOTIFICATION_NOT_SUPPORTED_ERROR_CODE:
return new PushNotificationNotSupportedError(code, message, data);
case UNSUPPORTED_OPERATION_ERROR_CODE:
return new UnsupportedOperationError(code, message, data);
case CONTENT_TYPE_NOT_SUPPORTED_ERROR_CODE:
return new ContentTypeNotSupportedError(code, message, data);
case INVALID_AGENT_RESPONSE_ERROR_CODE:
return new InvalidAgentResponseError(code, message, data);
case TASK_NOT_CANCELABLE_ERROR_CODE:
return new TaskNotCancelableError(code, message, data);
case TASK_NOT_FOUND_ERROR_CODE:
return new TaskNotFoundError(code, message, data);
case AUTHENTICATED_EXTENDED_CARD_NOT_CONFIGURED_ERROR_CODE:
return new AuthenticatedExtendedCardNotConfiguredError(code, message, data);
default:
return new JSONRPCError(code, message, data);
}References
- Constants that are used across multiple modules or components should be moved to a shared location to avoid duplication and ensure consistency.
| } else if (idElement.isJsonPrimitive()) { | ||
| try { | ||
| id = idElement.getAsLong(); | ||
| } catch (UnsupportedOperationException | NumberFormatException | IllegalStateException e) { | ||
| id = idElement.getAsString(); |
There was a problem hiding this comment.
Using getAsLong() to extract the JSON-RPC ID will truncate fractional numbers. It is better to use JsonUtil.OBJECT_MAPPER.fromJson(idElement, Object.class) to correctly handle both string and numeric IDs while preserving precision for non-integer numbers.
| } else if (idElement.isJsonPrimitive()) { | |
| try { | |
| id = idElement.getAsLong(); | |
| } catch (UnsupportedOperationException | NumberFormatException | IllegalStateException e) { | |
| id = idElement.getAsString(); | |
| id = JsonUtil.OBJECT_MAPPER.fromJson(idElement, Object.class); |
There was a problem hiding this comment.
The ID precision issues are theoretical - the spec discourages fractional IDs and the code has worked this way since inception.
| case JSON_PARSE_ERROR_CODE -> | ||
| new JSONParseError(code, message, data); | ||
| case INVALID_REQUEST_ERROR_CODE -> | ||
| new InvalidRequestError(code, message, data); | ||
| case METHOD_NOT_FOUND_ERROR_CODE -> | ||
| new MethodNotFoundError(code, message, data); | ||
| case INVALID_PARAMS_ERROR_CODE -> | ||
| new InvalidParamsError(code, message, data); | ||
| case INTERNAL_ERROR_CODE -> | ||
| new io.a2a.spec.InternalError(code, message, data); | ||
| case TASK_NOT_FOUND_ERROR_CODE -> | ||
| new TaskNotFoundError(code, message, data); | ||
| case TASK_NOT_CANCELABLE_ERROR_CODE -> | ||
| new TaskNotCancelableError(code, message, data); | ||
| case PUSH_NOTIFICATION_NOT_SUPPORTED_ERROR_CODE -> | ||
| new PushNotificationNotSupportedError(code, message, data); | ||
| case UNSUPPORTED_OPERATION_ERROR_CODE -> | ||
| new UnsupportedOperationError(code, message, data); | ||
| case CONTENT_TYPE_NOT_SUPPORTED_ERROR_CODE -> | ||
| new ContentTypeNotSupportedError(code, message, data); | ||
| case INVALID_AGENT_RESPONSE_ERROR_CODE -> | ||
| new InvalidAgentResponseError(code, message, data); | ||
| default -> | ||
| new JSONRPCError(code, message, data); | ||
| }; | ||
| } |
There was a problem hiding this comment.
The switch statement in the JSONRPCError type adapter is missing the case for AUTHENTICATED_EXTENDED_CARD_NOT_CONFIGURED_ERROR_CODE. This prevents proper polymorphic deserialization of this specific error type. Ensure the constant is used to maintain consistency across modules.
return switch (code) {
case JSON_PARSE_ERROR_CODE ->
new JSONParseError(code, message, data);
case INVALID_REQUEST_ERROR_CODE ->
new InvalidRequestError(code, message, data);
case METHOD_NOT_FOUND_ERROR_CODE ->
new MethodNotFoundError(code, message, data);
case INVALID_PARAMS_ERROR_CODE ->
new InvalidParamsError(code, message, data);
case INTERNAL_ERROR_CODE ->
new io.a2a.spec.InternalError(code, message, data);
case TASK_NOT_FOUND_ERROR_CODE ->
new TaskNotFoundError(code, message, data);
case TASK_NOT_CANCELABLE_ERROR_CODE ->
new TaskNotCancelableError(code, message, data);
case PUSH_NOTIFICATION_NOT_SUPPORTED_ERROR_CODE ->
new PushNotificationNotSupportedError(code, message, data);
case UNSUPPORTED_OPERATION_ERROR_CODE ->
new UnsupportedOperationError(code, message, data);
case CONTENT_TYPE_NOT_SUPPORTED_ERROR_CODE ->
new ContentTypeNotSupportedError(code, message, data);
case INVALID_AGENT_RESPONSE_ERROR_CODE ->
new InvalidAgentResponseError(code, message, data);
case AUTHENTICATED_EXTENDED_CARD_NOT_CONFIGURED_ERROR_CODE ->
new AuthenticatedExtendedCardNotConfiguredError(code, message, data);
default ->
new JSONRPCError(code, message, data);
};References
- Constants that are used across multiple modules or components should be moved to a shared location to avoid duplication and ensure consistency.
Uh oh!
There was an error while loading. Please reload this page.