Skip to content

fix: reject CR/LF in push notification Authorization header - #1043

Closed
ez-lbz wants to merge 2 commits into
a2aproject:mainfrom
ez-lbz:fix/push-auth-header-crlf
Closed

fix: reject CR/LF in push notification Authorization header#1043
ez-lbz wants to merge 2 commits into
a2aproject:mainfrom
ez-lbz:fix/push-auth-header-crlf

Conversation

@ez-lbz

@ez-lbz ez-lbz commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

What changed

1. Reject CR/LF characters in push notification Authorization header

Problem: BasePushNotificationSender.dispatchNotification built the Authorization header as scheme + " " + credentials with direct string concatenation. Both values are client-controlled (stored in the push notification config), so a credential or scheme containing \r/\n could inject arbitrary extra headers (CWE-113). The JDK HttpClient happens to validate headers, but A2AHttpClient is a pluggable SPI whose implementations (including custom ones) cannot be assumed to do the same.

Fix (server-common/src/main/java/org/a2aproject/sdk/server/tasks/BasePushNotificationSender.java):

  • Added buildAuthorizationHeader(AuthenticationInfo), which assembles "scheme credentials" and throws IllegalArgumentException if the resulting value contains CR or LF. The notification dispatch then fails safely (logged) and the injected header is never sent.

Fix (server-common/src/test/java/org/a2aproject/sdk/server/tasks/PushNotificationSenderTest.java):

  • testSendNotificationWithAuthHeader — a valid AuthenticationInfo("Bearer", "token123") sends Authorization: Bearer token123.
  • testSendNotificationRejectsCrlfInCredentials — credentials containing \r\nX-Injected: 1 cause the notification to be dropped; no HTTP call is made.
  • testSendNotificationRejectsCrlfInScheme — a scheme containing \n causes the notification to be dropped.

Behavior change: push notifications whose configured auth scheme/credentials contain CR or LF are no longer dispatched (previously the raw value was passed through to the HTTP client). Valid credentials are unaffected.

Testing

  • mvn -pl server-common test450 tests run, 0 failures, 0 errors, 0 skipped (BUILD SUCCESS), including the 3 new regression tests.

@ehsavoie ehsavoie self-assigned this Aug 11, 2026
@ehsavoie

Copy link
Copy Markdown
Collaborator

@ez-lbz I've updated your PR to cover the token too in #1053
Thanks for it

@ehsavoie ehsavoie closed this Aug 11, 2026
@ez-lbz

ez-lbz commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for taking this over and extending it to the notification token as well (#1053, CWE-113) — much appreciated. No action needed from my side on this one; happy to defer to your PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants