fix(session): enforce user ownership in VertexAI DeleteSession

[kvmilos]

Link to Issue or Description of Change

Closes #1194

Problem: VertexAiSessionService's delete path did not verify session ownership. The Agent Engine backend keys DeleteSession by the session resource name and ignores the caller's user id, so any user who knows or guesses another user's session id could delete that user's session. getSession and listSessions already enforce/report ownership — only deleteSession was missing the check.

Solution: Before issuing the delete, deleteSession now fetches the session via the existing getSession helper and verifies ownership (mirrors this package's own getSession); a cross-user request is rejected and no DeleteSession RPC is sent. A session that no longer exists is treated as a no-op (returns nil), matching adk-python.

Cross-language parity (ownership on delete):

• adk-python — delete_session raises on a user_id mismatch and no-ops on 404.
• adk-java — deleteSession fetches and throws on a mismatch.
• adk-kotlin — deleteSession fetches and throws on a mismatch.

(Scope note: python/java also validate the session id first; that part isn't ported here.)

Testing Plan

• Hermetic bufconn test (session/vertexai/delete_ownership_test.go, no credentials or fixtures): a cross-user delete is rejected with zero DeleteSession RPCs, the owner can still delete, and a missing session is a no-op. Verified red on main, green on this branch.
• The shared session suite gains delete_session_respects_user_id as a cross-backend invariant; go test ./session/ ./session/database/ passes.

Note on CI: the session/vertexai replay fixtures need regenerating (UPDATE_REPLAYS=true), because adding a GetSession to the delete path changes the recorded RPC sequence — it trips every test's cleanup (which calls Delete). I don't have access to the recording project, so a maintainer would need to regenerate them.

#1217 decouples that teardown from Delete. If it lands first, only the two delete fixtures here need re-recording rather than most of testdata/.

Operational note

Delete now also requires the aiplatform.sessions.get permission (in addition to sessions.delete) and costs one extra round trip — both inherent to the read-before-delete design the other SDKs use.

Checklist

• I have read the CONTRIBUTING.md document.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas.
• I have added tests that prove my fix is effective.
• New and existing unit tests pass locally (in-memory + database suites, and the hermetic vertexai test).
• I have manually tested my changes end-to-end.
• Any dependent changes have been merged and published in downstream modules.

[kvmilos]

Could a maintainer regenerate the replay files? That should get the failing check green. Thanks!

[karolpiotrowicz]

Thanks for this — the fix is correct, and it closes a real vulnerability.

I verified it rather than eyeballing it. On main a cross-user delete goes straight through; with your change it's rejected and no DeleteSession RPC is issued at all:

before: Delete(user2) -> err=<nil>, RPCs=[DeleteSession], victim session destroyed
after:  Delete(user2) -> err="...does not belong to user user2", RPCs=[GetSession], session intact

The read-then-compare-then-delete shape also matches adk-python and adk-java.

On the red CI — you broke 2 tests, not 23

Worth untangling before anyone re-records anything, because the failure count is misleading.

Every test in session/vertexai cleans up via t.Cleanup -> deleteAll, which calls the production Delete. Your change adds a GetSession to that path, so every test's teardown now issues an RPC that no recording contains. Create, Get, List, all AppendEvent and all StateManagement cases fail in cleanup, not in the behavior they test.

I confirmed this is separable. Cleanup deletes as the correct owner anyway, so it gains nothing from the ownership check — pointing deleteAllFromApp at the raw DeleteSession RPC instead of the public Delete (13 lines in service_test.go, no production change, no fixtures touched):

	--- FAIL	--- PASS
this PR as-is	23	45
same PR, teardown decoupled	3	65

The 3 remaining are the parent node plus the only two subtests that genuinely exercise delete: Delete and your new delete_session_respects_user_id.

So ~20 of the 23 failures come from a pre-existing coupling in the test harness that your change happened to expose, not from the change itself.

Two ways forward — your call

1. Split the harness fix out first. A small PR decoupling cleanup from Delete lands green on main on its own merits — it stops unrelated tests breaking every time Delete changes. This PR then needs only two fixtures re-recorded instead of most of testdata/.
2. Just re-record everything. Simpler if someone with e2e access can do it in one go, but it leaves the coupling for the next person who touches Delete.

Whoever ends up recording: it'd be useful to capture what GetSession returns for a malformed id like "nonExistent". No current fixture has it, and it decides whether the assertion at service_suite.go:392 still holds.

Worth adding here: a hermetic test

Independent of the fixtures, the ownership guarantee itself can be tested with no credentials at all. NewSessionService already accepts option.ClientOption, so a bufconn server implementing aiplatformpb.SessionServiceServer injected via option.WithGRPCConn does it — and grpc/test/bufconn ships in the grpc version already required, so go.mod/go.sum are untouched.

This matters because the new subtest can't currently fail for the backend you're fixing (see the inline note). I wrote one while reviewing; it's red on main and green on your branch:

Sketch — drop in as session/vertexai/delete_ownership_test.go

type fakeSessions struct {
	aiplatformpb.UnimplementedSessionServiceServer
	deletes atomic.Int32
}

func (f *fakeSessions) GetSession(_ context.Context, req *aiplatformpb.GetSessionRequest) (*aiplatformpb.Session, error) {
	if strings.HasSuffix(req.GetName(), "/sessions/owned") {
		return &aiplatformpb.Session{Name: req.GetName(), UserId: "user1", UpdateTime: timestamppb.Now()}, nil
	}
	return nil, status.Errorf(codes.NotFound, "Session %s not found.", req.GetName())
}

func (f *fakeSessions) DeleteSession(_ context.Context, req *aiplatformpb.DeleteSessionRequest) (*longrunningpb.Operation, error) {
	f.deletes.Add(1)
	done, _ := anypb.New(&emptypb.Empty{})
	// NOTE: Done:true with a nil Result makes lro.Wait fail with "unsupported result type".
	return &longrunningpb.Operation{Name: req.GetName() + "/operations/1", Done: true,
		Result: &longrunningpb.Operation_Response{Response: done}}, nil
}

func newFakeService(t *testing.T) (session.Service, *fakeSessions) {
	t.Helper()
	lis := bufconn.Listen(1 << 20)
	srv := grpc.NewServer()
	fake := &fakeSessions{}
	aiplatformpb.RegisterSessionServiceServer(srv, fake)
	go func() { _ = srv.Serve(lis) }()
	t.Cleanup(srv.Stop)

	conn, err := grpc.NewClient("passthrough:///bufnet",
		grpc.WithContextDialer(func(ctx context.Context, _ string) (net.Conn, error) { return lis.DialContext(ctx) }),
		grpc.WithTransportCredentials(insecure.NewCredentials()))
	if err != nil {
		t.Fatalf("grpc.NewClient: %v", err)
	}
	t.Cleanup(func() { _ = conn.Close() })

	s, err := NewSessionService(t.Context(), VertexAIServiceConfig{
		Location: "us-central1", ProjectID: "p", ReasoningEngine: "123",
	}, option.WithGRPCConn(conn))
	if err != nil {
		t.Fatalf("NewSessionService: %v", err)
	}
	return s, fake
}

func TestDeleteRespectsUserID(t *testing.T) {
	s, fake := newFakeService(t)
	err := s.Delete(t.Context(), &session.DeleteRequest{AppName: "123", UserID: "user2", SessionID: "owned"})
	if err == nil || !strings.Contains(err.Error(), "does not belong to user") {
		t.Errorf("cross-user Delete: got %v, want an ownership error", err)
	}
	if got := fake.deletes.Load(); got != 0 {
		t.Errorf("DeleteSession RPCs = %d, want 0", got)
	}
}

Worth a companion case asserting the owner can still delete (deletes == 1), so a guard that rejects everything can't pass.

One thing missing from the fix

Deleting a session that's already gone should succeed silently. #1194 asks for it explicitly, and adk-python does exactly that (vertex_ai_session_service.py:346-348). Today any getSession error is propagated, so a repeat delete returns an error and the REST handler turns it into a 500. A few lines using the isNotFoundError helper already in the file — inline suggestion below. To be clear this isn't a regression; the previous code errored too, just from a different call.

Non-blocking

Worth a line in the description, since this is a public module: Delete now needs aiplatform.sessions.get in addition to sessions.delete, so a delete-only principal will start failing. It also costs an extra round trip. Both are inherent to the read-before-write design the other SDKs use, so I'd document rather than change it.

Small correction for the description: the parity claim holds for the ownership half, but adk-python and adk-java also validate the session id first (_validate_session_id / validateSessionId) and that isn't ported here. Fine to leave out of scope — just worth not claiming.

I also noticed a few pre-existing issues in this package while reading around the change. None are yours and none belong in this PR — I'll file them separately.

Nice catch on the original bug.

[karolpiotrowicz]

Suggest treating a missing session as success here, per #1194 ("a genuinely missing session should be a no-op") and to match adk-python (vertex_ai_session_service.py:346-348, if e.code == 404: return). isNotFoundError already exists in this file at line 110, and status.Code sees through the %w chain, so this works as-is:

Suggested change

	// Verify the session belongs to req.UserID before deleting (mirrors getSession).
	if _, err := c.getSession(ctx, &session.GetRequest{
	AppName: req.AppName,
	UserID: req.UserID,
	SessionID: req.SessionID,
	}); err != nil {
	return err
	}
	// Verify the session belongs to req.UserID before deleting (mirrors getSession).
	if _, err := c.getSession(ctx, &session.GetRequest{
	AppName: req.AppName,
	UserID: req.UserID,
	SessionID: req.SessionID,
	}); err != nil {
	if isNotFoundError(err) {
	return nil // A missing session is a no-op.
	}
	return err
	}

Only the wrapped gRPC NotFound from line 153 is swallowed. The ownership denial (line 160) and the engine-id failure (line 146) aren't gRPC statuses, so status.Code returns Unknown for them and they still propagate — the guard keeps working.

Side benefit: repeat deletes and stale list-then-delete cleanup loops become idempotent instead of returning a 500 through the REST handler.

It also settles something this change otherwise leaves hanging. service_suite.go:392 asserts codes.InvalidArgument for deleting a non-existent session. That code used to come from DeleteSession; now it comes from GetSession, and no fixture records what GetSession returns for a malformed id. With the no-op, both possibilities satisfy the assertion — nil if it answers NOT_FOUND, InvalidArgument if it answers that — so the branch stops depending on unverified backend behavior.

[kvmilos]

Done like suggested.

[karolpiotrowicz]

This subtest can't fail for the backend the PR fixes.

In-memory keys on the full {app, user, session} triple (session/inmemory.go:187-193) and database filters on storageSession{AppName, UserID, ID} (session/database/service.go:305-309) — both were already immune, so this passes on the merge base for both. For vertexai it can't run yet (no fixture), and once the fixture exists the assertion still won't distinguish fixed from unfixed code: rpcreplay matches a request by method + equality and replays the recorded response, modelling no server state, so the final Get succeeds either way.

Worth keeping as a cheap cross-backend invariant, but the assertion that actually guards this belongs in a hermetic session/vertexai test — sketch in the main review comment.

[kvmilos]

Keeping it as the cheap cross-backend invariant you suggested, and the assertion that actually guards the fix now lives in the hermetic session/vertexai test from your sketch.

[kvmilos]

Thank for the review.

Both suggestions are applied, and I went with option 1 - #1217 decouples the teardown from Delete.
Updated the description.
Carrying your recording note forward so it isn't lost: whoever regenerates the fixtures, it'd be useful to capture what GetSession returns for a malformed id like "nonExistent".