fix(security): enforce authorization for listTasks - #1038
Open
ez-lbz wants to merge 1 commit into
Open
Conversation
onListTasks in AuthorizationRequestHandlerDecorator delegated directly without any authorization check, unlike every other handler method. Add a list-scoped read check before delegation. The provider is invoked with an empty-string sentinel for the whole list scope (a null task ID would break ConcurrentHashMap-backed providers with an NPE on lookup). Per-task checkRead filtering in the TaskStore list() remains unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
1. Enforce authorization for
onListTasks(CWE-862)Problem:
onListTasksinAuthorizationRequestHandlerDecoratordelegated directly to the wrapped handler without any authorization check, unlike every other handler method which calls anenforce*check first. With aTaskAuthorizationProviderconfigured, a caller could invokelistTasksand bypass the authorization model entirely — only per-task filtering in theTaskStore(which custom store implementations may skip) stood between the caller and other users' tasks.Fix (server-common/src/main/java/org/a2aproject/sdk/server/requesthandlers/AuthorizationRequestHandlerDecorator.java):
enforceListRead) invoked fromonListTasksbefore delegation. SinceLIST_TASKShas no single task ID, the provider is invoked with an empty-string sentinel (LIST_TASKS_SCOPE_ID) representing the whole list scope; a denial throwsTaskNotFoundError(the same fail-closed error used by all other checks, avoiding information leakage). Anulltask ID was deliberately avoided because it breaks providers that key lookups on the task ID (e.g.ConcurrentHashMap-backed stores throw onget(null)).checkReadfiltering in theTaskStore.list()implementation remains in place and is unchanged.Fix (server-common/src/test/java/org/a2aproject/sdk/server/requesthandlers/AuthorizationRequestHandlerDecoratorTest.java):
onListTasks_denied— whencheckRead(context, "", LIST_TASKS)returnsfalse, the call throwsTaskNotFoundErrorand the delegate is never invoked.onListTasks_allowed— when the check returnstrue, the call is delegated and the result returned.Behavior change: with a
TaskAuthorizationProviderconfigured,listTasksnow goes through a list-scoped authorization gate; a provider that deniesLIST_TASKSwill reject list requests outright. Providers that allow unknown tasks (e.g.TestTaskAuthorizationProvider, whose lookup for the sentinel ID yields no owner and therefore allows) see no behavior change.Testing
mvn -pl server-common test -Dtest=AuthorizationRequestHandlerDecoratorTest— 29 tests run, 0 failures, 0 errors (includes the 2 new regression tests).mvn -pl reference/jsonrpc test -Dtest=QuarkusA2AJSONRPCWithTaskAuthorizationVertxTest— 7 tests run, 0 failures, 0 errors (this suite previously failed in CI withCannot invoke "Object.hashCode()" because "key" is null; the list-scoped authorization test now passes with the sentinel-based check).