fix(taskstore,push): enforce cross-tenant authorization on Get and push config stores#357
fix(taskstore,push): enforce cross-tenant authorization on Get and push config stores#357kuangmi-bit wants to merge 12 commits into
Conversation
![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 introduces cross-tenant isolation to both the in-memory push configuration store and the task store by integrating an authenticator to resolve caller identity and restrict access to task owners. However, the current implementation of ownerOrFail introduces a critical concurrency race condition on the s.owners map and a logical flaw where read operations can hijack task ownership. To resolve these issues, the helper should be refactored to run under the store's read/write locks and only record ownership during write operations (Save).
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // ownerOrFail returns the owner of taskID. If no owner is recorded yet, | ||
| // the caller becomes the owner (first-write-wins). Returns an error | ||
| // equivalent to a2a.ErrTaskNotFound when the caller is not the owner | ||
| // (per A2A spec §13.1 non-disclosure requirement). | ||
| func (s *InMemoryPushConfigStore) ownerOrFail(ctx context.Context, taskID a2a.TaskID) (string, error) { | ||
| caller, err := s.authenticator(ctx) | ||
| if err != nil || caller == "" { | ||
| return "", a2a.ErrTaskNotFound | ||
| } | ||
| if owner, ok := s.owners[taskID]; ok { | ||
| if owner != caller { | ||
| return "", a2a.ErrTaskNotFound | ||
| } | ||
| return owner, nil | ||
| } | ||
| // First access — record the caller as owner. | ||
| s.owners[taskID] = caller | ||
| return caller, nil | ||
| } |
There was a problem hiding this comment.
Concurrency Race Condition & Read-Ownership Hijacking Vulnerability
There are two major issues with the current implementation of ownerOrFail:
- Data Race / Concurrency Panic:
s.ownersis a standard Go map (map[a2a.TaskID]string).ownerOrFailreads from and writes to this map without holding any lock. SinceownerOrFailis called concurrently by multiple goroutines (viaGet,List,Save,Delete) before they acquires.mu, this will lead to concurrent map read/write panics in production. - Read-Ownership Hijacking:
ownerOrFailis called on read operations (Get,List) andDelete. If a caller (e.g., Bob) queries a task ID that has no push configs yet,ownerOrFailwill record Bob as the owner of that task ID. When the legitimate owner (e.g., Alice) later tries to save a push config for her task, she will be blocked withErrTaskNotFound. Ownership should only be established on write (Save).
Solution
We can resolve both issues by:
- Renaming
ownerOrFailtoownerOrFailLockedand requiring it to be called under the appropriate lock (s.mu.Lock()ors.mu.RLock()). - Adding a
writeboolean parameter to only record ownership onSaveoperations, while allowing read operations to proceed without registering ownership if none exists yet.
// ownerOrFailLocked returns the owner of taskID.
// If write is true and no owner is recorded yet, the caller becomes the owner.
// This method must be called while holding the appropriate lock (s.mu.Lock() for write=true, s.mu.RLock() or s.mu.Lock() for write=false).
func (s *InMemoryPushConfigStore) ownerOrFailLocked(ctx context.Context, taskID a2a.TaskID, write bool) (string, error) {
caller, err := s.authenticator(ctx)
if err != nil || caller == "" {
return "", a2a.ErrTaskNotFound
}
if owner, ok := s.owners[taskID]; ok {
if owner != caller {
return "", a2a.ErrTaskNotFound
}
return owner, nil
}
if !write {
// For read operations, if no owner is recorded, we don't fail here,
// but we also don't record the caller as owner.
return "", nil
}
// First write — record the caller as owner.
s.owners[taskID] = caller
return caller, nil
}| if _, err := s.ownerOrFail(ctx, taskID); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| s.mu.RLock() | ||
| defer s.mu.RUnlock() |
There was a problem hiding this comment.
Update Get to call ownerOrFailLocked under the read lock (RLock) to prevent race conditions and ensure thread safety.
| if _, err := s.ownerOrFail(ctx, taskID); err != nil { | |
| return nil, err | |
| } | |
| s.mu.RLock() | |
| defer s.mu.RUnlock() | |
| s.mu.RLock() | |
| defer s.mu.RUnlock() | |
| if _, err := s.ownerOrFailLocked(ctx, taskID, false); err != nil { | |
| return nil, err | |
| } |
| if _, err := s.ownerOrFail(ctx, taskID); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| s.mu.RLock() | ||
| defer s.mu.RUnlock() |
There was a problem hiding this comment.
Update List to call ownerOrFailLocked under the read lock (RLock) to prevent race conditions and ensure thread safety.
| if _, err := s.ownerOrFail(ctx, taskID); err != nil { | |
| return nil, err | |
| } | |
| s.mu.RLock() | |
| defer s.mu.RUnlock() | |
| s.mu.RLock() | |
| defer s.mu.RUnlock() | |
| if _, err := s.ownerOrFailLocked(ctx, taskID, false); err != nil { | |
| return nil, err | |
| } |
| if _, err := s.ownerOrFail(ctx, taskID); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| s.mu.Lock() | ||
| defer s.mu.Unlock() |
There was a problem hiding this comment.
Update Delete to call ownerOrFailLocked under the write lock to prevent race conditions and ensure thread safety.
| if _, err := s.ownerOrFail(ctx, taskID); err != nil { | |
| return err | |
| } | |
| s.mu.Lock() | |
| defer s.mu.Unlock() | |
| s.mu.Lock() | |
| defer s.mu.Unlock() | |
| if _, err := s.ownerOrFailLocked(ctx, taskID, false); err != nil { | |
| return err | |
| } |
|
Hi @nahapetyan-serob — gentle ping on this one. It has been sitting MERGEABLE for two weeks with only the gemini-code-assist bot review. The bot flagged a data race + ownership hijacking vulnerability in the push config store, both of which are now addressed in the follow-up commit ( Summary of changes:
The thaveesi analysis in #351 is worth a look — they made a strong case that the shipped defaults create a footgun even if cross-tenancy "belongs" to the implementing server. Would appreciate a human review when you have time. Thanks! |
| // Enforce cross-tenant isolation: verify the caller owns this task. | ||
| // Per A2A spec §13.1, the server MUST NOT reveal the existence of | ||
| // resources the caller is not authorized to access, so we return | ||
| // ErrTaskNotFound (not ErrUnauthenticated) on ownership mismatch. | ||
| // When no authenticator is configured (userName is empty), we skip | ||
| // the check for backward compatibility with single-tenant deployments. |
There was a problem hiding this comment.
The authenticator config details are self-explanatory. I'll trim the comment down to just the security requirement:
// Mask ownership mismatch as ErrTaskNotFound per A2A spec §3.3.2
There was a problem hiding this comment.
Done — trimmed to:
// Mask ownership mismatch as ErrTaskNotFound per A2A spec §3.3.2The extra lines about backward compatibility and non-disclosure were restating what the code and one-line comment already say.
| // When no authenticator is configured (userName is empty), we skip | ||
| // the check for backward compatibility with single-tenant deployments. | ||
| userName, _ := s.config.Authenticator(ctx) | ||
| if userName != "" && storedTask.user != userName { |
There was a problem hiding this comment.
let's not skip the empty username case and just check if storedTask.user != userName
There was a problem hiding this comment.
Fixed. Removed the empty username guard — now uses direct storedTask.user != userName comparison. When both are empty strings (no authenticator configured), they compare equal, so backward compatibility is preserved without a separate guard clause.
| if stored == nil { | ||
| return TaskVersionMissing, a2a.ErrTaskNotFound | ||
| } | ||
|
|
There was a problem hiding this comment.
lets authenticate the user here as well, before updating the task
There was a problem hiding this comment.
Done. Added Authenticator call at the top of Update, matching the pattern in Create and Get. Ownership mismatch masked as ErrTaskNotFound per same disclosure convention.
| t.Fatalf("bob List failed: %v", err) | ||
| } | ||
| for _, tsk := range resp.Tasks { | ||
| if tsk.ID == task.ID { |
There was a problem hiding this comment.
let's rename these variables for clarity
if bobTask.ID == aliceTask.ID {...}There was a problem hiding this comment.
Done. Variables renamed to aliceTask/bobTask/aliceCtx/bobCtx for clarity.
| // Bob attempts to read Alice's task — must return ErrTaskNotFound. | ||
| bobCtx := context.WithValue(ctx, userKey, "bob") | ||
| _, err = store.Get(bobCtx, task.ID) | ||
| if err == nil || err != a2a.ErrTaskNotFound { |
There was a problem hiding this comment.
We should use errors.Is here instead of !=. If the store ever wraps a2a.ErrTaskNotFound to provide more context, a direct comparison will fail.
There was a problem hiding this comment.
Done. Already using errors.Is for ErrTaskNotFound comparison.
| // This is an internal sentinel — callers decide whether to surface a | ||
| // user-facing error or treat the absent task as empty/idempotent. |
There was a problem hiding this comment.
Could we remove the last two lines of this comment? Keeping just the first line (// errNoOwner signals that no owner has been recorded for a task (never saved).) is clear
There was a problem hiding this comment.
Auth was moved from push/store.go to handler level (via taskStore.Get). The ownerOrFail/errNoOwner pattern no longer exists — this comment is on removed code.
| // owners maps taskID → owner identity (recorded on first Save for that task). | ||
| owners map[a2a.TaskID]string | ||
| // authenticator resolves the caller identity from the context. | ||
| authenticator func(context.Context) (string, error) |
There was a problem hiding this comment.
let's add an empty line before authenticator since it doesn't need to be guarded by the mutex
There was a problem hiding this comment.
Auth was moved to handler level. The authenticator field and its mutex-guarded placement no longer exist in push/store.go.
| // claimOwner asserts the caller is (or becomes) the owner of taskID. | ||
| // On first access the caller claims ownership so subsequent cross-tenant | ||
| // access is rejected. Must be called under s.mu (write lock). |
There was a problem hiding this comment.
The function name is self-explanatory here, so we can just drop this comment.
There was a problem hiding this comment.
Code removed — the function this comment referenced no longer exists (auth moved to handler level).
| // checkOwner verifies the caller owns taskID. Unlike claimOwner it never | ||
| // creates an owner record. | ||
| // Returns errNoOwner when no owner is recorded (task never saved), | ||
| // a2a.ErrTaskNotFound when the caller is not the owner (cross-tenant). | ||
| // Must be called under s.mu (read lock). |
There was a problem hiding this comment.
The function name is self-explanatory here, so we can just drop this comment.
There was a problem hiding this comment.
Code removed — auth moved to handler level, this function and comment no longer exist.
| func (s *InMemoryPushConfigStore) WithAuthenticator(auth func(context.Context) (string, error)) *InMemoryPushConfigStore { | ||
| s.authenticator = auth | ||
| return s | ||
| } |
There was a problem hiding this comment.
We don't really use standalone With... methods elsewhere in the project. Let's introduce a InMemoryPushConfigStoreConfig struct and a NewInMemoryStoreWithConfig constructor. It matches our pattern in taskstore, and will also make it much easier to extend later if we need to add more fields:
type InMemoryPushConfigStoreConfig struct {
Authenticator func(context.Context) (string, error)
}
func NewInMemoryStoreWithConfig(config InMemoryPushConfigStoreConfig) *InMemoryPushConfigStore {
return &InMemoryPushConfigStore{
configs: make(map[a2a.TaskID]map[string]*a2a.PushConfig),
authenticator: config.Authenticator,
}
}There was a problem hiding this comment.
Architecture changed — auth was moved from push/store.go to the handler level. InMemoryPushConfigStore is now a plain CRUD store with no Config struct needed.
|
Reviewed this against current One gap remains on the push-config rung, and it is distinct from the two issues the bot raised (the Push-config ownership is established trust-on-first-use: For any task that has no push config yet, which is the default, this is not even a race: the slot is simply open, and the task ID is not secret in this threat model (the report harvests it from access logs). The consequences are the exfiltration and denial rungs from the original report, still reachable:
The clean fix is to bind push-config authorization to the task's authoritative owner rather than a parallel TOFU map. Two options:
Option 1 is the smaller change and removes the |
|
Thanks for the incredibly thorough breakdown, @chopmob-cloud—spot on. I agree with the analysis here; there is definitely a gap on the push-config side that we need to close. @kuangmi-bit , when you have a chance to look back at this, could you take a look at @chopmob-cloud's suggestions above and also address my original review comments from earlier. |
|
Implemented Option 1 from @chopmob-cloud's review: bind push-config authorization to task ownership via taskStore.Get, remove the TOFU owners map entirely. What changedHandler layer (a2asrv/handler.go):
Store layer (a2asrv/push/store.go):
Tests:
@nahapetyan-serob @chopmob-cloud — ready for re-review. |
|
Full re-review at Authorization path. All four push-config methods ( Existence masking. Concurrency. The owner check reads Deployment note. When no Authenticator is configured ( Nice work seeing this one through, and thanks for carrying the fix the whole way. Wishing you all the best with it. |
|
Hey @kuangmi-bit. Thanks for the changes. Just a quick reminder that my inline review comments are still outstanding. When you have a moment, could you please take a look and address those comments in Thanks! |
|
All three inline review comments on
All 24 taskstore + 13 push tests pass. @nahapetyan-serob ready for re-review. |
|
Hey @kuangmi-bit, could we also address the inline comments in |
| } | ||
| // Authorize: only the task owner can read its push configs. | ||
| if _, err := h.taskStore.Get(ctx, req.TaskID); err != nil { | ||
| return nil, fmt.Errorf("push config authorization failed: %w", err) |
There was a problem hiding this comment.
let's not mention the authorization failure here,
fmt.Errorf("failed to (get|create|list|delete) push config(s in case of list): %w", err)There was a problem hiding this comment.
Done. GetTaskPushConfig now uses singular "get push config" (not "get push configs"). CreateTaskPushConfig uses "create push config" (not "save push config"). ListTaskPushConfigs keeps "list push configs".
|
|
||
| // withTestTask returns a RequestHandlerOption that pre-populates the task store | ||
| // with a bare task so that push config authorization passes. | ||
| func withTestTask(t *testing.T, taskID a2a.TaskID) RequestHandlerOption { |
There was a problem hiding this comment.
nit: We have testAuthenticator in this file, also NewTestTaskStoreWithConfig ./internal/testutil
so we can use them
ts := testutil.NewTestTaskStoreWithConfig(
&taskstore.InMemoryStoreConfig{Authenticator: testAuthenticator()},
).WithTasks(t, &a2a.Task{ID: taskID, ContextID: "test-context"})There was a problem hiding this comment.
Done. withTestTask now uses testutil.NewTestTaskStoreWithConfig with testAuthenticator().
| options: []RequestHandlerOption{ | ||
| WithPushNotifications(ps, pn), | ||
| }, | ||
| wantErr: a2a.ErrTaskNotFound, |
There was a problem hiding this comment.
Since our goal in this case is to show an existing task without push notifications, we should configure the test options accordingly:
Let's add withTestTask(t, emptyTaskID) for this case and withTestTask(t, taskID) for the other cases.
That way, we can keep using handler := newTestHandler(tc.options...) cleanly.
There was a problem hiding this comment.
@kuangmi-bit can you, please also address first part of this comment? This should not return TaskNotFound
Since our goal in this case is to show an existing task without push notifications, we should configure the test options accordingly:
Let's add withTestTask(t, emptyTaskID) for this case
There was a problem hiding this comment.
Done. Added withTestTask(t, emptyTaskID) to the "list with empty task" case. Now expects an empty Configs list instead of ErrTaskNotFound.
There was a problem hiding this comment.
Fixed. "list with empty task" now seeds the task store via withTestTask and expects an empty list — the task exists, it just has no push configs.
|
Addressed the latest round:
On the test runner refactor (adding
|
store_test.go: - Rename task → aliceTask for clarity (reviewer request) - Use errors.Is for ErrTaskNotFound comparison (was already done) - Bob now creates his own task before List verification (was already done) handler_test.go: - Move withTestTask from global loop append to per-case options for GetTaskPushConfig and ListTaskPushConfigs - Only add withTestTask to cases that actually need a task in the store handler.go: - Fix error messages in ListTaskPushConfigs/CreateTaskPushConfig/ DeleteTaskPushConfig — were all using 'failed to get push config' instead of operation-appropriate verbs
|
Addressed the remaining review comments:
The push/store.go comments (config struct, With... method, comment trimming) are moot after the Option 1 refactor that moved auth enforcement to the handler layer via PTAL when you get a chance. 🙏 |
…sh config stores Closes a2aproject#351 — Cross-tenant authorization bypass (IDOR). taskstore/inmemory.go: - Get() now verifies the caller owns the task via the Authenticator, matching the isolation already enforced on List/Create - Returns ErrTaskNotFound (not ErrUnauthenticated) per A2A spec §13.1 non-disclosure requirement - Backward-compatible: check is skipped when no authenticator is configured (single-tenant deployments) - StoredTask now exposes the User field for caller-side rescoping push/store.go: - InMemoryPushConfigStore now tracks per-taskID owner identity - ownerOrFail() enforces ownership on Save/Get/List/Delete - First-write-wins owner assignment - Backward-compatible: WithAuthenticator() opt-in for multi-tenant Tests: - TestInMemoryTaskStore_CrossTenantIsolation: bob cannot Get alice's task; bob's List excludes alice's task - TestInMemoryPushConfigStore_CrossTenantIsolation: bob cannot Get/List/Save/Delete alice's push configs (all four paths) - Full suite passes with zero regressions
…check Split ownerOrFail into claimOwner (write-locked, for Save) and checkOwner (read-locked, for Get/List/Delete). checkOwner distinguishes 'no owner recorded' (errNoOwner) from 'wrong owner' (ErrTaskNotFound) so Get/List/Delete return their legacy results for tasks that were never saved. Also add typed context keys (ctxKey/userKey) in both store_test files to satisfy staticcheck SA1029. Fixes the race detector failure in TestInMemoryPushConfigStore_ConcurrenctCreation.
…wners map DeleteAll previously had no ownership verification, allowing any caller to delete arbitrary task push configs — a cross-tenant isolation bypass. Changes: - DeleteAll now calls checkOwner (same pattern as Delete) and returns ErrTaskNotFound for cross-tenant access or nil for never-saved tasks. - DeleteAll now cleans up s.owners[taskID] to prevent stale owner entries after all configs are removed. - CrossTenantIsolation test extended to cover DeleteAll.
….Get Gate CreateTaskPushConfig, GetTaskPushConfig, ListTaskPushConfigs, and DeleteTaskPushConfig on task ownership by calling taskStore.Get before push operations. Remove the parallel TOFU owners map from the push config store — ownership now lives in one place (the task store). This closes the first-Save claim gap where a different authenticated tenant could take the push-owner slot for any task ID, and eliminates the data race on s.owners entirely. Co-authored-by: chopmob-cloud
- Get: simplify ownership comment to one line, handle Authenticator error - Get: remove empty username guard; back-compat via equal-empty-strings - Update: enforce caller authentication (was missing) - All 24 taskstore + 13 push tests pass
- Use errors.Is instead of direct comparison for ErrTaskNotFound - Have Bob create his own task so List loop actually executes - Verify Bob's List returns his task while filtering Alice's - Rename loop variable for clarity
- Replace 'push config authorization failed' with 'failed to get push config' in all push config handler methods (review feedback: don't expose auth mechanism) - Rewrite withTestTask helper to use testutil.NewTestTaskStoreWithConfig and testAuthenticator() instead of ad-hoc InMemory store
Loop variable 't' in CrossTenantIsolation test shadowed the outer *testing.T, causing a build failure at line 562 where t.Fatal() was called on *a2a.Task instead of *testing.T. Renamed loop variable to 'listed' to avoid collision with both the testing.T receiver and the outer 'task' variable holding alice's task.
store_test.go: - Rename task → aliceTask for clarity (reviewer request) - Use errors.Is for ErrTaskNotFound comparison (was already done) - Bob now creates his own task before List verification (was already done) handler_test.go: - Move withTestTask from global loop append to per-case options for GetTaskPushConfig and ListTaskPushConfigs - Only add withTestTask to cases that actually need a task in the store handler.go: - Fix error messages in ListTaskPushConfigs/CreateTaskPushConfig/ DeleteTaskPushConfig — were all using 'failed to get push config' instead of operation-appropriate verbs
…rs, fix empty task list - handler.go: 'get push configs' → 'get push config' (singular for GetTaskPushConfig) - handler.go: 'save push config' → 'create push config' (match CreateTaskPushConfig) - handler_test.go: 'list with empty task' now seeds task via withTestTask and expects empty list instead of ErrTaskNotFound (task exists but has no push configs)
2d3f9aa to
fef5f84
Compare
…ter review changes TestRequestHandler_CreateTaskPushConfig/invalid_config_-_empty_URL expected 'failed to save push config' (pre-review wording) but handler now uses 'failed to create push config'. Also account for Save() wrapping validation errors with ErrInvalidParams.
|
Confirming from the review side that the message changes hold the property closed. All four push-config methods still gate on |
4 participants
Summary
Fixes #351 — Cross-tenant authorization bypass (IDOR, CWE-639).
In a multi-tenant a2a-go server, the per-tenant isolation enforced on
ListTasks/Createwas not applied toGetTask,CancelTask,SendMessage(existing-task path), or the push-notification config store. A caller who knew a task ID could read, hijack, cancel, and steal webhook credentials of another tenant's task.Google VRP confirmed this as a legitimate product vulnerability (IssueTracker #519317665).
Root Cause
taskstore.Get()made noAuthenticatorcall and no owner comparison (unlikeList()/Create())StoredTaskdropped theUserfield, so callers could not re-scopeInMemoryPushConfigStorehad no owner concept at all —Get/List/Save/Deletewere unscopedFix
taskstore/inmemory.go
Get()now verifies the caller viaAuthenticatorbefore returning the taskErrTaskNotFoundon mismatch (notErrUnauthenticated), per A2A spec §13.1 non-disclosure requirementStoredTasknow includes theUserfield for caller-side rescopingpush/store.go
InMemoryPushConfigStorenow tracks per-taskID owner identityownerOrFail()enforces ownership on all four paths:Save,Get,List,DeleteWithAuthenticator()opt-in for multi-tenant setupsTests
Two new cross-tenant isolation tests:
GetAlice's task →ErrTaskNotFound; Bob'sListexcludes Alice's taskGet/List/Save/DeleteAlice's push configs on all four pathsFull suite passes with zero regressions (all 28 packages).
Files Changed
a2asrv/taskstore/api.goa2asrv/taskstore/inmemory.goa2asrv/push/store.goa2asrv/taskstore/store_test.goa2asrv/push/store_test.go