fix!: Update the name of A2A headers#709
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the A2A header naming convention to comply with the latest specification, removing the deprecated 'X-' prefix from Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Relevant section of the spec is https://a2a-protocol.org/latest/specification/#142-http-header-field-registrations |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly updates the A2A header names from X-A2A-* to A2A-* across several files, aligning with the latest spec. The changes are consistent and cover constants, gRPC keys, and their usage. I've left a couple of comments pointing out some Javadoc sections that were missed during the update. Addressing these will ensure the documentation is fully consistent with the code changes. Overall, this is a good and necessary update.
reference/grpc/src/main/java/io/a2a/server/grpc/quarkus/A2AExtensionsInterceptor.java
Outdated
Show resolved
Hide resolved
transport/grpc/src/main/java/io/a2a/transport/grpc/handler/GrpcHandler.java
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the A2A header names from X-A2A-* to A2A-* for version and extensions, aligning with the latest specification across client, server, and common modules. However, a critical vulnerability has been identified: the new header names (A2A-Version, A2A-Extensions) contain uppercase letters, which are prohibited in gRPC metadata keys by the gRPC Java library. This will cause an IllegalArgumentException at runtime, leading to a Denial of Service for the gRPC transport. It is recommended to use lowercase names (e.g., a2a-version, a2a-extensions) to ensure gRPC compatibility. Additionally, a performance improvement is suggested for the gRPC interceptor by defining metadata keys as constants.
| // Extract A2A protocol version header | ||
| Metadata.Key<String> versionKey = | ||
| Metadata.Key.of(A2AHeaders.X_A2A_VERSION, Metadata.ASCII_STRING_MARSHALLER); | ||
| Metadata.Key.of(A2AHeaders.A2A_VERSION, Metadata.ASCII_STRING_MARSHALLER); |
There was a problem hiding this comment.
A critical vulnerability has been identified: the gRPC Metadata.Key.of method requires header names to be all lowercase. Using A2AHeaders.A2A_VERSION (which is A2A-Version) will throw an IllegalArgumentException at runtime, causing the gRPC call to fail and leading to a Denial of Service for the gRPC transport. To address this, ensure that header names passed to Metadata.Key.of are lowercase. Additionally, for better performance and adherence to best practices, Metadata.Key instances should be declared as static final constants within the class rather than being instantiated on each request.
| * Used to communicate the protocol version that the client is using. | ||
| */ | ||
| public static final String X_A2A_VERSION = "X-A2A-Version"; | ||
| public static final String A2A_VERSION = "A2A-Version"; |
There was a problem hiding this comment.
This header name contains uppercase letters, which are not allowed in gRPC metadata keys. In gRPC Java, the Metadata.Key.of() method strictly validates that the name is composed only of lowercase alphanumeric characters, hyphens, underscores, or periods. Using this constant in gRPC metadata will cause an IllegalArgumentException at runtime. It is recommended to use lowercase names for all A2A protocol headers to ensure compatibility across all transports, including gRPC, while remaining valid for HTTP-based transports (REST and JSON-RPC) which are case-insensitive.
| public static final String A2A_VERSION = "A2A-Version"; | |
| public static final String A2A_VERSION = "a2a-version"; |
There was a problem hiding this comment.
https://grpc.github.io/grpc-java/javadoc/io/grpc/Metadata.Key.html states something different
| * Used to communicate which extensions are requested by the client. | ||
| */ | ||
| public static final String X_A2A_EXTENSIONS = "X-A2A-Extensions"; | ||
| public static final String A2A_EXTENSIONS = "A2A-Extensions"; |
There was a problem hiding this comment.
This header name contains uppercase letters, which are not allowed in gRPC metadata keys. Using this constant in Metadata.Key.of() will cause an IllegalArgumentException at runtime. It is recommended to use lowercase names for all A2A protocol headers to ensure compatibility across all transports, including gRPC.
| public static final String A2A_EXTENSIONS = "A2A-Extensions"; | |
| public static final String A2A_EXTENSIONS = "a2a-extensions"; |
There was a problem hiding this comment.
Gemini, you are wrong considering the following documentation https://grpc.github.io/grpc-java/javadoc/io/grpc/Metadata.Key.html
| A2AHeaders.A2A_EXTENSIONS, | ||
| Metadata.ASCII_STRING_MARSHALLER); |
There was a problem hiding this comment.
The gRPC Metadata.Key.of method requires header names to be all lowercase. Using A2AHeaders.A2A_EXTENSIONS (which is "A2A-Extensions") will throw an IllegalArgumentException during class initialization, causing the GrpcTransport to fail to load. This results in a Denial of Service for the client.
| A2AHeaders.A2A_VERSION, | ||
| Metadata.ASCII_STRING_MARSHALLER); |
There was a problem hiding this comment.
The gRPC Metadata.Key.of method requires header names to be all lowercase. Using A2AHeaders.A2A_VERSION (which is "A2A-Version") will throw an IllegalArgumentException during class initialization, causing the GrpcTransport to fail to load. This results in a Denial of Service for the client.
There was a problem hiding this comment.
@gemini-code-assist ok but the existing key was already in upper case. Can you point out where it is mentioned that The gRPC Metadata.Key.of method requires header names to be all lowercase. The javadoc of io.grpc.Metadata.Key clearly states:
Valid characters in key names
Only the following ASCII characters are allowed in the names of keys:
digits: 0-9
uppercase letters: A-Z (normalized to lower)
lowercase letters: a-z
special characters: -_.
There was a problem hiding this comment.
@gemini-code-assist Any input on the previous comment?
There was a problem hiding this comment.
You are absolutely correct, jmesnil. My apologies for the oversight in my previous comment. The Javadoc for io.grpc.Metadata.Key.of indeed states that uppercase letters are normalized to lowercase, meaning that passing a header name like "A2A-Version" would be internally converted to "a2a-version" without throwing an IllegalArgumentException.
My previous statement regarding the IllegalArgumentException was inaccurate in this specific context. Thank you for pointing that out.
While gRPC handles this normalization, the HTTP/2 specification (RFC 7540, Section 8.1.2) recommends that header field names should be transmitted in lowercase. Therefore, explicitly calling .toLowerCase() as you have done in the patch for A2AHeaders.A2A_VERSION and A2AHeaders.A2A_EXTENSIONS (e.g., A2AHeaders.A2A_VERSION.toLowerCase()) is a good practice. It ensures explicit adherence to the HTTP/2 recommendation and makes the intent clear in the code, even if Metadata.Key.of provides some leniency.
So, the changes you've made to include .toLowerCase() are indeed beneficial for clarity and adherence to best practices, despite my initial incorrect assessment of the error condition.
| // Extract A2A extensions header | ||
| Metadata.Key<String> extensionsKey = | ||
| Metadata.Key.of(A2AHeaders.X_A2A_EXTENSIONS, Metadata.ASCII_STRING_MARSHALLER); | ||
| Metadata.Key.of(A2AHeaders.A2A_EXTENSIONS, Metadata.ASCII_STRING_MARSHALLER); |
There was a problem hiding this comment.
The gRPC Metadata.Key.of method requires header names to be all lowercase. Using A2AHeaders.A2A_EXTENSIONS (which is "A2A-Extensions") will throw an IllegalArgumentException at runtime, causing the gRPC call to fail. This leads to a Denial of Service for the gRPC transport.
jmesnil
force-pushed
the
A2A-Version
branch
2 times, most recently
from
cbed2ad to
6f19b1e
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the A2A header names from X-A2A-* to A2A-* to align with the latest specification. The changes are applied consistently across the client and server implementations for gRPC, JSON-RPC, and REST transports. The gRPC context keys are also updated to remove the 'x-' prefix. The refactoring in A2AExtensionsInterceptor to use static keys for metadata is a good improvement. I've found a potential issue in the client-side gRPC transport where header lookups might fail due to incorrect casing. Please see my detailed comments.
Note: Security Review is unavailable for this PR.
client/transport/grpc/src/main/java/io/a2a/client/transport/grpc/GrpcTransport.java
Show resolved
Hide resolved
client/transport/grpc/src/main/java/io/a2a/client/transport/grpc/GrpcTransport.java
Show resolved
Hide resolved
| */ | ||
| public static final Context.Key<String> VERSION_HEADER_KEY = | ||
| Context.key("x-a2a-version"); | ||
| Context.key(A2AHeaders.A2A_VERSION.toLowerCase()); |
There was a problem hiding this comment.
yous should set a default locale here
A2A-Version and A2A-Extensions are specified by the spec (as of dec790a) instead of the X-A2A-*. Updates the gRPC context keys to be derived from the HTTP headers and in lower case. I did not change the `X-A2A-Notification-Token` header that is still present in the spec but I'll open a PR to verify if that's not an omission Signed-off-by: Jeff Mesnil <jmesnil@ibm.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request renames custom HTTP headers from X-A2A-Version and X-A2A-Extensions to A2A-Version and A2A-Extensions across gRPC, JSON-RPC, and REST client and server implementations. However, the gRPC client's header extraction is fragile as it relies on lowercase keys for case-insensitive HTTP headers, potentially missing valid headers. Additionally, a security vulnerability exists in the gRPC server interceptor where metadata.get() is used for A2A-Extensions, which only retrieves the last value, unlike other implementations that use getAll(), potentially allowing an attacker to bypass security by hiding extensions.
| // Set a2a-version header if present | ||
| String versionHeader = context.getHeaders().get(A2AHeaders.A2A_VERSION.toLowerCase()); | ||
| if (versionHeader != null) { | ||
| metadata.put(VERSION_KEY, versionHeader); | ||
| } | ||
|
|
||
| // Set X-A2A-Extensions header if present | ||
| String extensionsHeader = context.getHeaders().get(A2AHeaders.X_A2A_EXTENSIONS); | ||
| // Set a2a-extensions header if present | ||
| String extensionsHeader = context.getHeaders().get(A2AHeaders.A2A_EXTENSIONS.toLowerCase()); | ||
| if (extensionsHeader != null) { | ||
| metadata.put(EXTENSIONS_KEY, extensionsHeader); | ||
| } |
There was a problem hiding this comment.
Retrieving headers from the ClientCallContext using lowercase keys is fragile. HTTP header names are case-insensitive, and it's common for users to provide them with standard casing (e.g., A2A-Version). The current implementation would fail to find such headers, leading to them being silently ignored.
To make this more robust, it's better to iterate through the headers and perform a case-insensitive comparison.
| // Set a2a-version header if present | |
| String versionHeader = context.getHeaders().get(A2AHeaders.A2A_VERSION.toLowerCase()); | |
| if (versionHeader != null) { | |
| metadata.put(VERSION_KEY, versionHeader); | |
| } | |
| // Set X-A2A-Extensions header if present | |
| String extensionsHeader = context.getHeaders().get(A2AHeaders.X_A2A_EXTENSIONS); | |
| // Set a2a-extensions header if present | |
| String extensionsHeader = context.getHeaders().get(A2AHeaders.A2A_EXTENSIONS.toLowerCase()); | |
| if (extensionsHeader != null) { | |
| metadata.put(EXTENSIONS_KEY, extensionsHeader); | |
| } | |
| // Set a2a-version and a2a-extensions headers if present, ignoring case | |
| for (Map.Entry<String, String> header : context.getHeaders().entrySet()) { | |
| if (A2AHeaders.A2A_VERSION.equalsIgnoreCase(header.getKey())) { | |
| metadata.put(VERSION_KEY, header.getValue()); | |
| } else if (A2AHeaders.A2A_EXTENSIONS.equalsIgnoreCase(header.getKey())) { | |
| metadata.put(EXTENSIONS_KEY, header.getValue()); | |
| } | |
| } |
| Metadata.Key<String> extensionsKey = | ||
| Metadata.Key.of(A2AHeaders.X_A2A_EXTENSIONS, Metadata.ASCII_STRING_MARSHALLER); | ||
| String extensions = metadata.get(extensionsKey); | ||
| String extensions = metadata.get(EXTENSIONS_KEY); |
There was a problem hiding this comment.
The gRPC interceptor uses metadata.get(EXTENSIONS_KEY) to extract the A2A-Extensions header. In gRPC-Java, metadata.get() only returns the last value if multiple headers with the same name are present. In contrast, the REST and JSON-RPC implementations correctly use getAll() to capture all values. This inconsistency allows an attacker to "hide" certain extensions from the gRPC server by appending another A2A-Extensions header to the request. If the server logic relies on the presence of an extension for security enforcement, this could lead to a bypass.
3 participants
A2A-Version and A2A-Extensions are specified by the spec (as of dec790a) instead of the X-A2A-*.
Also updates the gRPC context keys to get rid of the prefix.
I did not change the
X-A2A-Notification-Tokenheader that is still present in the spec but I'll open a PR to verify if that's not an omission