From 279da3014fd3c6ba73aa0272bb2f4885d38c32bc Mon Sep 17 00:00:00 2001 From: Kam Lall Date: Wed, 29 Jul 2026 17:44:35 -0400 Subject: [PATCH] fix: enforce agent ownership on GET /api/v1/external/invocations/{id} MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Service.Get had no ownership check, so any caller with a valid bearer token for one agent identity could read another agent's invocation record (tool input/output/error) by guessing or observing its id — the exact boundary RecordExecution already enforces on the write path. Extract the ownership check into a shared checkOwnership() helper used by both Get and RecordExecution. GET now maps ErrNotOwner to the same generic 404 as a missing invocation, so the response can't be used to confirm an id exists but belongs to someone else. --- internal/api/handlers.go | 12 ++-- internal/api/handlers_test.go | 33 +++++++++++ internal/invocation/record_execution_test.go | 54 +++++++++++++++++ internal/invocation/service.go | 61 +++++++++++++------- 4 files changed, 134 insertions(+), 26 deletions(-) diff --git a/internal/api/handlers.go b/internal/api/handlers.go index e11eb9ff..b0e50576 100644 --- a/internal/api/handlers.go +++ b/internal/api/handlers.go @@ -4394,11 +4394,15 @@ func (h *Handler) externalInvocationDetail(w http.ResponseWriter, r *http.Reques case http.MethodGet: resp, err := h.svc.Get(r.Context(), id) if err != nil { - status := http.StatusInternalServerError - if err == sql.ErrNoRows { - status = http.StatusNotFound + // Not-found and not-owner map to the same generic 404 so a + // non-owning caller can't distinguish "doesn't exist" from + // "exists but belongs to another agent" (existence would + // otherwise leak through the error path). + if err == sql.ErrNoRows || errors.Is(err, invocation.ErrNotOwner) { + writeError(w, http.StatusNotFound, "not found") + return } - writeError(w, status, err.Error()) + writeError(w, http.StatusInternalServerError, err.Error()) return } writeJSON(w, http.StatusOK, resp) diff --git a/internal/api/handlers_test.go b/internal/api/handlers_test.go index d38aa66a..fe7d04ba 100644 --- a/internal/api/handlers_test.go +++ b/internal/api/handlers_test.go @@ -2218,6 +2218,39 @@ func TestExternalInvocationPatchErrorStatusMapping(t *testing.T) { } } +func TestExternalInvocationGetErrorStatusMapping(t *testing.T) { + cases := []struct { + name string + err error + wantStatus int + }{ + {"not owner maps to 404", fmt.Errorf("invocation inv_1: %w", invocation.ErrNotOwner), http.StatusNotFound}, + {"missing invocation maps to 404", sql.ErrNoRows, http.StatusNotFound}, + {"generic error maps to 500", fmt.Errorf("boom"), http.StatusInternalServerError}, + {"success maps to 200", nil, http.StatusOK}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + svc := &stubService{getErr: tc.err} + h := NewHandler(svc, stubServerService{}, nil, nil, nil, nil, nil, nil, nil, nil) + req := httptest.NewRequest(http.MethodGet, "/api/v1/external/invocations/inv_1", nil) + w := httptest.NewRecorder() + h.Routes().ServeHTTP(w, req) + if w.Code != tc.wantStatus { + t.Fatalf("status = %d, want %d (body=%s)", w.Code, tc.wantStatus, w.Body.String()) + } + // The not-owner 404 must read identically to a real "missing + // invocation" 404 — a distinguishable body would leak that the + // invocation exists to a non-owning caller. + if tc.wantStatus == http.StatusNotFound { + if got, want := w.Body.String(), `{"error":{"message":"not found"}}`+"\n"; got != want { + t.Fatalf("body = %q, want %q (must not leak ownership details)", got, want) + } + } + }) + } +} + func TestAddExtraRoutesMountsRoutesOutsideAuthChains(t *testing.T) { h := NewHandler(&stubService{}, stubServerService{}, nil, nil, nil, nil, nil, nil, nil, nil) // A configured validator protects the normal runtime and privileged routes; diff --git a/internal/invocation/record_execution_test.go b/internal/invocation/record_execution_test.go index b3bf7086..8860225f 100644 --- a/internal/invocation/record_execution_test.go +++ b/internal/invocation/record_execution_test.go @@ -324,3 +324,57 @@ func TestRecordExecutionEnforcesAgentOwnership(t *testing.T) { } }) } + +// TestGetEnforcesAgentOwnership guards against a caller who knows another +// agent's invocation_id reading that agent's tool input/output/error via +// GET /api/v1/external/invocations/{id} — the same ownership boundary +// RecordExecution enforces on the write path (TestRecordExecutionEnforcesAgentOwnership). +func TestGetEnforcesAgentOwnership(t *testing.T) { + t.Run("different agent is rejected", func(t *testing.T) { + svc := newRecordExecutionService(t) + id := submitExternal(t, svc, "agent-a") + ctx := auth.WithIdentity(context.Background(), auth.Identity{AgentID: "agent-b"}) + _, err := svc.Get(ctx, id) + if !errors.Is(err, invocation.ErrNotOwner) { + t.Fatalf("err = %v, want ErrNotOwner", err) + } + }) + + t.Run("owning agent is allowed", func(t *testing.T) { + svc := newRecordExecutionService(t) + id := submitExternal(t, svc, "agent-a") + ctx := auth.WithIdentity(context.Background(), auth.Identity{AgentID: "agent-a"}) + resp, err := svc.Get(ctx, id) + if err != nil { + t.Fatal(err) + } + if resp.InvocationID != id { + t.Fatalf("invocation_id = %q, want %q", resp.InvocationID, id) + } + }) + + t.Run("caller without identity is exempt", func(t *testing.T) { + svc := newRecordExecutionService(t) + id := submitExternal(t, svc, "agent-a") + resp, err := svc.Get(context.Background(), id) + if err != nil { + t.Fatal(err) + } + if resp.InvocationID != id { + t.Fatalf("invocation_id = %q, want %q", resp.InvocationID, id) + } + }) + + t.Run("anonymous invocation accepts any identity", func(t *testing.T) { + svc := newRecordExecutionService(t) + id := submitExternal(t, svc, "") + ctx := auth.WithIdentity(context.Background(), auth.Identity{AgentID: "agent-b"}) + resp, err := svc.Get(ctx, id) + if err != nil { + t.Fatal(err) + } + if resp.InvocationID != id { + t.Fatalf("invocation_id = %q, want %q", resp.InvocationID, id) + } + }) +} diff --git a/internal/invocation/service.go b/internal/invocation/service.go index dfd56d16..ebaa627e 100644 --- a/internal/invocation/service.go +++ b/internal/invocation/service.go @@ -1728,8 +1728,8 @@ func (s *Service) summarizePendingApproval(invocationID string) { // pending_approval, or after a human denied it. var ErrInvalidTransition = errors.New("invalid execution status transition") -// ErrNotOwner is returned by RecordExecution when the verified agent identity -// on the request does not match the agent the invocation belongs to. +// ErrNotOwner is returned by Get and RecordExecution when the verified agent +// identity on the request does not match the agent the invocation belongs to. var ErrNotOwner = errors.New("invocation belongs to a different agent") // requireStatus rejects an execution-status transition unless the invocation @@ -1743,6 +1743,31 @@ func requireStatus(inv Invocation, target string, allowed ...Status) error { return fmt.Errorf("invocation %s cannot move to %s from %s: %w", inv.InvocationID, target, inv.Status, ErrInvalidTransition) } +// checkOwnership rejects access to inv when the request carries an +// authenticated agent identity that does not match the invocation's owner. +// Both the read (Get) and write (RecordExecution) paths on +// /api/v1/external/invocations/{id} run under the agent-runtime OAuth +// middleware, so in auth mode ctx carries the authenticated caller +// (inv.AgentID is set from that same identity at Submit time, so a match +// proves ownership). Invocations submitted anonymously (no agent_id) skip +// the check, as does any caller with no identity in context — no-auth mode, +// or the in-process managed-agents watcher and operator/review routes, which +// authenticate by a different path and never carry an agent identity. +func checkOwnership(ctx context.Context, inv Invocation) error { + callerID := strings.TrimSpace(auth.AgentIDFromContext(ctx)) + if callerID == "" { + return nil + } + owner := "" + if inv.AgentID != nil { + owner = strings.TrimSpace(*inv.AgentID) + } + if owner != "" && owner != callerID { + return fmt.Errorf("invocation %s: %w", inv.InvocationID, ErrNotOwner) + } + return nil +} + // RecordExecution updates an externally-executed invocation with the outcome // reported by the executor. Valid execStatus values: // @@ -1758,26 +1783,12 @@ func (s *Service) RecordExecution(ctx context.Context, invocationID string, upda if err != nil { return InvocationResponse{}, err } - // Ownership: update.Result/Error are surfaced to the judge as trusted - // evidence, so a caller who knows another agent's invocation_id could - // poison that agent's session context. The PATCH - // /api/v1/external/invocations/{id} route runs under the agent-runtime - // OAuth middleware, so in auth mode ctx carries the authenticated caller. - // When an identity is present, reject any attempt to write an invocation - // this agent does not own (inv.AgentID is set from the authenticated - // identity at Submit time, so a match proves ownership). Invocations - // submitted anonymously (no agent_id) skip the ownership check. In no-auth - // mode there is no identity to check against — behavior is unchanged, and - // the in-process managed-agents watcher (which calls RecordExecution - // directly with no auth identity) is likewise unaffected. - if callerID := strings.TrimSpace(auth.AgentIDFromContext(ctx)); callerID != "" { - owner := "" - if inv.AgentID != nil { - owner = strings.TrimSpace(*inv.AgentID) - } - if owner != "" && owner != callerID { - return InvocationResponse{}, fmt.Errorf("invocation %s: %w", invocationID, ErrNotOwner) - } + // update.Result/Error are surfaced to the judge as trusted evidence, so a + // caller who knows another agent's invocation_id could poison that + // agent's session context — reject writes to invocations this caller + // doesn't own. + if err := checkOwnership(ctx, inv); err != nil { + return InvocationResponse{}, err } now := time.Now().UTC() switch update.ExecutionStatus { @@ -1886,6 +1897,12 @@ func (s *Service) Get(ctx context.Context, id string) (InvocationResponse, error if err != nil { return InvocationResponse{}, err } + // A caller who knows another agent's invocation_id could otherwise read + // that agent's tool input/output/error — reject reads of invocations + // this caller doesn't own. + if err := checkOwnership(ctx, inv); err != nil { + return InvocationResponse{}, err + } resp := s.toResponse(inv) return resp, nil }