Skip to content

Commit d1b1d92

Browse files
Mateusz Krawieccopybara-github
authored andcommitted
fix: prevent cross-user session data disclosure in VertexAiSessionService
- listSessions now sends the user id as a quoted, escaped AIP-160 filter literal so its contents cannot alter the query. - getSession and deleteSession verify the session belongs to the requesting user. - Session ids are validated before being used in request paths. PiperOrigin-RevId: 942103179
1 parent c2b087c commit d1b1d92

4 files changed

Lines changed: 165 additions & 7 deletions

File tree

core/src/main/java/com/google/adk/sessions/VertexAiClient.java

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,13 +105,27 @@ private Completable pollOperation(String operationId, int attempt) {
105105
}
106106

107107
Maybe<JsonNode> listSessions(String reasoningEngineId, String userId) {
108+
// Send the user id as a quoted AIP-160 literal so its contents cannot alter
109+
// the filter, then URL-escape the whole filter for transport.
110+
String filter = "user_id=" + quoteFilterLiteral(userId);
108111
return performApiRequest(
109112
"GET",
110-
"reasoningEngines/" + reasoningEngineId + "/sessions?filter=user_id=" + userId,
113+
"reasoningEngines/"
114+
+ reasoningEngineId
115+
+ "/sessions?filter="
116+
+ UrlEscapers.urlFormParameterEscaper().escape(filter),
111117
"")
112118
.flatMapMaybe(VertexAiClient::getJsonResponse);
113119
}
114120

121+
/**
122+
* Wraps a value in an AIP-160 double-quoted string literal. Per go/aip/160, only backslashes and
123+
* double quotes need escaping inside the quotes.
124+
*/
125+
private static String quoteFilterLiteral(String value) {
126+
return "\"" + value.replace("\\", "\\\\").replace("\"", "\\\"") + "\"";
127+
}
128+
115129
Maybe<JsonNode> listEvents(String reasoningEngineId, String sessionId, @Nullable String filter) {
116130
String path = "reasoningEngines/" + reasoningEngineId + "/sessions/" + sessionId + "/events";
117131
if (filter != null) {

core/src/main/java/com/google/adk/sessions/VertexAiSessionService.java

Lines changed: 49 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,7 @@ private ListSessionsResponse parseListSessionsResponse(
164164

165165
@Override
166166
public Single<ListEventsResponse> listEvents(String appName, String userId, String sessionId) {
167+
validateSessionId(sessionId);
167168
return listEventsInternal(appName, sessionId, /* filter= */ null);
168169
}
169170

@@ -195,11 +196,21 @@ private ListEventsResponse parseListEventsResponse(JsonNode listEventsResponse)
195196
@Override
196197
public Maybe<Session> getSession(
197198
String appName, String userId, String sessionId, Optional<GetSessionConfig> config) {
199+
validateSessionId(sessionId);
198200
String reasoningEngineId = parseReasoningEngineId(appName);
199201
return client
200202
.getSession(reasoningEngineId, sessionId)
201203
.flatMap(
202204
getSessionResponseMap -> {
205+
// Enforce ownership using the owner reported by the backend, not the
206+
// requested user id. Deny as not-found so existence is not revealed.
207+
String ownerUserId =
208+
Optional.ofNullable(getSessionResponseMap.get("userId"))
209+
.map(JsonNode::asText)
210+
.orElse(null);
211+
if (!userId.equals(ownerUserId)) {
212+
return Maybe.<Session>empty();
213+
}
203214
String sessId =
204215
Optional.ofNullable(getSessionResponseMap.get("name"))
205216
.map(name -> Iterables.getLast(Splitter.on('/').splitToList(name.asText())))
@@ -274,12 +285,31 @@ private static List<Event> filterEvents(
274285

275286
@Override
276287
public Completable deleteSession(String appName, String userId, String sessionId) {
288+
validateSessionId(sessionId);
277289
String reasoningEngineId = parseReasoningEngineId(appName);
278-
return client.deleteSession(reasoningEngineId, sessionId);
290+
// Fetch first and enforce ownership: the backend delete ignores user id, so
291+
// without this check any user could delete another user's session. A missing
292+
// session completes as a no-op.
293+
return client
294+
.getSession(reasoningEngineId, sessionId)
295+
.flatMapCompletable(
296+
getSessionResponseMap -> {
297+
String ownerUserId =
298+
Optional.ofNullable(getSessionResponseMap.get("userId"))
299+
.map(JsonNode::asText)
300+
.orElse(null);
301+
if (!userId.equals(ownerUserId)) {
302+
return Completable.error(
303+
new SecurityException(
304+
"Session " + sessionId + " does not belong to user " + userId + "."));
305+
}
306+
return client.deleteSession(reasoningEngineId, sessionId);
307+
});
279308
}
280309

281310
@Override
282311
public Single<Event> appendEvent(Session session, Event event) {
312+
validateSessionId(session.id());
283313
String reasoningEngineId = parseReasoningEngineId(session.appName());
284314
return BaseSessionService.super
285315
.appendEvent(session, event)
@@ -319,4 +349,22 @@ static String parseReasoningEngineId(String appName) {
319349
private static final Pattern APP_NAME_PATTERN =
320350
Pattern.compile(
321351
"^projects/([a-zA-Z0-9-_]+)/locations/([a-zA-Z0-9-_]+)/reasoningEngines/(\\d+)$");
352+
353+
/** Rejects session ids that could escape the URL path segment. */
354+
static void validateSessionId(String sessionId) {
355+
if (sessionId == null || !SESSION_ID_PATTERN.matcher(sessionId).matches()) {
356+
throw new IllegalArgumentException(
357+
"Invalid session id: "
358+
+ sessionId
359+
+ ". It must match "
360+
+ SESSION_ID_PATTERN.pattern()
361+
+ ".");
362+
}
363+
}
364+
365+
/**
366+
* Allowed session id characters. Matches the adk-python {@code _validate_session_id} allowlist
367+
* and keeps the id within a single URL path segment (no '/', '?', '#', or '..').
368+
*/
369+
private static final Pattern SESSION_ID_PATTERN = Pattern.compile("^[a-zA-Z0-9_-]+$");
322370
}

core/src/test/java/com/google/adk/sessions/MockApiAnswer.java

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import com.fasterxml.jackson.databind.ObjectMapper;
55
import com.google.adk.JsonBaseModel;
66
import com.google.adk.events.Event;
7+
import java.io.IOException;
78
import java.net.URLDecoder;
89
import java.nio.charset.StandardCharsets;
910
import java.time.Instant;
@@ -29,7 +30,8 @@ class MockApiAnswer implements Answer<ApiResponse> {
2930
private static final Pattern SESSIONS_REGEX =
3031
Pattern.compile("^reasoningEngines/([^/]+)/sessions$");
3132
private static final Pattern SESSIONS_FILTER_REGEX =
32-
Pattern.compile("^reasoningEngines/([^/]+)/sessions\\?filter=user_id=([^/]+)$");
33+
Pattern.compile("^reasoningEngines/([^/]+)/sessions\\?filter=(.+)$");
34+
private static final String USER_ID_FILTER_PREFIX = "user_id=";
3335
private static final Pattern APPEND_EVENT_REGEX =
3436
Pattern.compile("^reasoningEngines/([^/]+)/sessions/([^/]+):appendEvent$");
3537
private static final Pattern EVENTS_REGEX =
@@ -135,7 +137,20 @@ private ApiResponse handleGetSessions(String path) throws Exception {
135137
if (!sessionsMatcher.matches()) {
136138
return null;
137139
}
138-
String userId = sessionsMatcher.group(2);
140+
// Decode the URL-escaped filter and read the quoted user_id literal back with
141+
// a JSON parser, as the real server would. An unquoted/injected filter is
142+
// rejected.
143+
String decodedFilter = URLDecoder.decode(sessionsMatcher.group(2), StandardCharsets.UTF_8);
144+
if (!decodedFilter.startsWith(USER_ID_FILTER_PREFIX)) {
145+
throw new IllegalArgumentException("Unsupported sessions filter: " + decodedFilter);
146+
}
147+
String userId;
148+
try {
149+
userId =
150+
mapper.readValue(decodedFilter.substring(USER_ID_FILTER_PREFIX.length()), String.class);
151+
} catch (IOException e) {
152+
throw new IllegalArgumentException("Unsupported sessions filter: " + decodedFilter, e);
153+
}
139154
List<String> userSessionsJson = new ArrayList<>();
140155
for (String sessionJson : sessionMap.values()) {
141156
Map<String, Object> session =

core/src/test/java/com/google/adk/sessions/VertexAiSessionServiceTest.java

Lines changed: 84 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import com.google.common.collect.ImmutableSet;
2020
import com.google.genai.types.Content;
2121
import com.google.genai.types.Part;
22+
import io.reactivex.rxjava3.core.Completable;
2223
import io.reactivex.rxjava3.core.Single;
2324
import java.time.Instant;
2425
import java.util.Arrays;
@@ -324,7 +325,8 @@ public void listSessions_empty() {
324325

325326
@Test
326327
public void listSessions_missingSessionsField_returnsEmpty() {
327-
when(mockApiClient.request("GET", "reasoningEngines/123/sessions?filter=user_id=userX", ""))
328+
when(mockApiClient.request(
329+
"GET", "reasoningEngines/123/sessions?filter=user_id%3D%22userX%22", ""))
328330
.thenAnswer(new MockApiAnswer("{}"));
329331

330332
assertThat(vertexAiSessionService.listSessions("123", "userX").blockingGet().sessions())
@@ -333,13 +335,90 @@ public void listSessions_missingSessionsField_returnsEmpty() {
333335

334336
@Test
335337
public void listSessions_nullSessionsField_returnsEmpty() {
336-
when(mockApiClient.request("GET", "reasoningEngines/123/sessions?filter=user_id=userY", ""))
338+
when(mockApiClient.request(
339+
"GET", "reasoningEngines/123/sessions?filter=user_id%3D%22userY%22", ""))
337340
.thenAnswer(new MockApiAnswer("{\"sessions\": null}"));
338341

339342
assertThat(vertexAiSessionService.listSessions("123", "userY").blockingGet().sessions())
340343
.isEmpty();
341344
}
342345

346+
@Test
347+
public void listSessions_maliciousUserId_isNeutralized() {
348+
// AIP-160 filter-injection payload.
349+
String payload = "\" OR user_id=~\"user";
350+
351+
ListSessionsResponse response =
352+
vertexAiSessionService.listSessions("123", payload).blockingGet();
353+
354+
// Treated as a single literal user id that matches nobody: no other user's
355+
// sessions leak.
356+
assertThat(response.sessions()).isEmpty();
357+
358+
ArgumentCaptor<String> pathCaptor = ArgumentCaptor.forClass(String.class);
359+
verify(mockApiClient, atLeastOnce()).request(eq("GET"), pathCaptor.capture(), eq(""));
360+
String listPath =
361+
pathCaptor.getAllValues().stream()
362+
.filter(p -> p.contains("/sessions?filter="))
363+
.findFirst()
364+
.orElseThrow(() -> new AssertionError("No list-sessions request was made"));
365+
// The value is sent as a quoted, URL-escaped literal (= -> %3D, " -> %22);
366+
// no raw quotes reach the query string.
367+
assertThat(listPath).contains("filter=user_id%3D%22");
368+
assertThat(listPath).doesNotContain("\"");
369+
}
370+
371+
@Test
372+
public void getSession_wrongUser_returnsEmpty() {
373+
// Session "1" belongs to "user"; a different user must not be able to read it.
374+
assertThat(
375+
vertexAiSessionService
376+
.getSession("123", "attacker", "1", Optional.empty())
377+
.blockingGet())
378+
.isNull();
379+
}
380+
381+
@Test
382+
public void deleteSession_wrongUser_deniedAndSessionKept() {
383+
// The ownership error surfaces on subscription, so hoist the Completable out.
384+
Completable deletion = vertexAiSessionService.deleteSession("123", "attacker", "1");
385+
assertThrows(SecurityException.class, deletion::blockingAwait);
386+
// The session is still readable by its real owner, i.e. it was not deleted.
387+
assertThat(
388+
vertexAiSessionService.getSession("123", "user", "1", Optional.empty()).blockingGet())
389+
.isNotNull();
390+
}
391+
392+
// Session id validation is synchronous, so each call below throws before returning a stream.
393+
@Test
394+
public void getSession_invalidSessionId_throws() {
395+
assertThrows(
396+
IllegalArgumentException.class,
397+
() -> vertexAiSessionService.getSession("123", "user", "1/../2", Optional.empty()));
398+
}
399+
400+
@Test
401+
public void deleteSession_invalidSessionId_throws() {
402+
assertThrows(
403+
IllegalArgumentException.class,
404+
() -> vertexAiSessionService.deleteSession("123", "user", "1\" OR 1"));
405+
}
406+
407+
@Test
408+
public void listEvents_invalidSessionId_throws() {
409+
assertThrows(
410+
IllegalArgumentException.class,
411+
() -> vertexAiSessionService.listEvents("123", "user", "a?b"));
412+
}
413+
414+
@Test
415+
public void appendEvent_invalidSessionId_throws() {
416+
Session session = Session.builder("bad/id").appName("123").userId("user").build();
417+
Event event = Event.builder().author("user").build();
418+
assertThrows(
419+
IllegalArgumentException.class, () -> vertexAiSessionService.appendEvent(session, event));
420+
}
421+
343422
@Test
344423
public void listEvents_empty() {
345424
assertThat(vertexAiSessionService.listEvents("789", "user1", "3").blockingGet().events())
@@ -348,9 +427,11 @@ public void listEvents_empty() {
348427

349428
@Test
350429
public void listEmptySession_success() {
430+
// Session "3" belongs to "user2"; request as the owner so the events list is
431+
// exercised (a non-owner is now denied).
351432
assertThat(
352433
vertexAiSessionService
353-
.getSession("789", "user1", "3", Optional.empty())
434+
.getSession("789", "user2", "3", Optional.empty())
354435
.blockingGet()
355436
.events())
356437
.isEmpty();

0 commit comments

Comments
 (0)