fix(security): prevent prototype pollution via untrusted map keys

[kalenkevich]

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

1. Link to an existing issue (if applicable):

• N/A — reported through an internal security review, no public issue.

2. Or, if no issue exists, describe the change:

Problem:

Several in-memory maps are keyed directly by values that arrive off the request
path or body — appName, userId, sessionId, eventId and session state
keys. They were plain {} object literals, so a key of __proto__ resolves to
Object.prototype instead of creating an own property:

// InMemorySessionService.createSession, before
if (!this.sessions[appName]) this.sessions[appName] = {};        // no-op when appName === '__proto__'
if (!this.sessions[appName][userId]) this.sessions[appName][userId] = {};  // writes Object.prototype[userId]

A single unauthenticated request to POST /apps/:appName/users/:userId/sessions/:sessionId
therefore writes onto Object.prototype for the remaining lifetime of the
process. The appendEvent stateDelta path is the more serious variant: the
planted value is fully attacker-controlled, so an arbitrary key can be given
an arbitrary object. Process-wide, that makes every if (opts.KEY),
obj[KEY] ??, KEY in obj and for…in attacker-influenced. The default dev
server (adk web / adk api_server) uses these services.

Two further consequences of the same root cause:

• InMemoryCredentialService shares the pattern, and additionally a lookup of
  an inherited credentialKey such as toString returned a Function rather
  than undefined.
• In AdkApiServer, in walks the prototype chain, so appName in this.runnerCache
  reported a hit for inherited names and handed back a Function where a
  Runner was expected. GET /debug/trace/toString likewise returned 200
  with Function.prototype.toString instead of 404.

Solution:

Key the affected maps with Object.create(null). A null-prototype map has no
inherited __proto__ accessor, so __proto__, constructor and prototype
become ordinary own properties, and in / property reads no longer see
inherited names.

This is a structural fix rather than an input filter. Rejecting __proto__ as
an appName at the route layer was considered and not taken: it is a breaking
API change that would also refuse legitimately-named apps, and it would have to
be repeated at every call site, whereas the null-prototype map removes the
primitive at the point of storage.

Changed:

• core/src/sessions/in_memory_session_service.ts — sessions, userState,
  appState, and the nested maps created in createSession / appendEvent.
• core/src/auth/credential_service/in_memory_credential_service.ts — credentials.
• dev/src/server/adk_api_server.ts — runnerCache, traceDict, sessionTraceDict.

Each package gets a small local createNullProtoMap() helper with a comment
explaining why, following the existing convention in
core/src/utils/streaming_utils.ts of keeping this kind of guard module-local
rather than widening the @google/adk public surface for an internal concern.

Behaviour is preserved: an app literally named __proto__ still stores and
retrieves normally, and no longer leaks phantom sessions into unrelated apps.

Not affected, checked while scoping: InMemoryMemoryService keys on
`${appName}/${userId}` and InMemoryArtifactService on
encodeURIComponent-joined path segments. Both keys always contain /, so
neither can ever equal __proto__. Left unchanged.

Testing Plan

Unit Tests:

• I have added or updated unit tests for my change.
• All unit tests pass locally.

10 new tests across 3 suites:

Suite	New tests	Covers
core/test/sessions/in_memory_session_service_test.ts	6	pollution via appName, via userId + user: state, via appName + app: state, via a __proto__ state key; no phantom cross-app sessions; app named __proto__ still works
core/test/auth/credential_service/in_memory_credential_service_test.ts	2	pollution via appName; inherited credentialKey returns undefined
dev/test/server/adk_api_server_test.ts	2	runnerCache builds a real Runner for an app named after an inherited key; /debug/trace/<inherited> returns 404

Each new test was confirmed to fail without the source fix (stash the source
change, re-run) rather than only passing with it. 8 of the 10 fail on the
unfixed code; the other 2 are behaviour-preservation tests that must pass both
before and after. Representative failures on the unfixed tree:

× does not pollute Object.prototype via userId in user state
× builds a real Runner for an app named after an inherited key
  → expected [Function toString] to be an instance of Runner
× returns 404 for a trace id matching an inherited key
  → expected 200 to be 404

Affected suites after the fix:

✓ core/test/auth/credential_service/in_memory_credential_service_test.ts (6 tests)
✓ core/test/sessions/in_memory_session_service_test.ts (37 tests)
✓ dev/test/server/adk_api_server_test.ts (53 tests)
Test Files  3 passed (3)
     Tests  96 passed (96)

Full unit suite (unit:core + unit:dev):

Test Files  1 failed | 183 passed (184)
     Tests  1 failed | 2679 passed (2680)

The single failure is createAgent > Interactive Mode > should handle Vertex AI selection with gcloud defaults, which is pre-existing and unrelated — it fails
identically on a clean tree at origin/main because it reads ambient gcloud
configuration.

npm run ts:check reports 281 errors in 43 files both with and without this
change, i.e. the pre-existing baseline is unchanged and no new type errors are
introduced. eslint and prettier --check are clean on all changed files.

Manual End-to-End (E2E) Tests:

With an agents directory available:

npx adk api_server --host 127.0.0.1 --port 8000 <agents_dir> &

# Create a session under an app named __proto__
curl -sS -X POST http://127.0.0.1:8000/apps/__proto__/users/polluted_key/sessions/poc_sid \
  -H 'Content-Type: application/json' -d '{}'

# Before: returns the planted session for an app/user pair never created.
# After:  empty list.
curl -sS http://127.0.0.1:8000/apps/polluted_key/users/polluted_key/sessions

# Before: 200 with Function.prototype.toString serialised.
# After:  404.
curl -sS -o /dev/null -w '%{http_code}\n' http://127.0.0.1:8000/debug/trace/toString

# Unchanged: an app genuinely named __proto__ still round-trips.
curl -sS http://127.0.0.1:8000/apps/__proto__/users/polluted_key/sessions

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 or that my feature works.
• New and existing unit tests pass locally with my changes.
• I have manually tested my changes end-to-end.
• Any dependent changes have been merged and published in downstream modules. (None — self-contained.)

Additional context

Object.create(null) maps behave identically for every read and write the
affected code performs. Object.keys, Object.values and Object.entries are
own-property operations and are unaffected; mergeStates uses Object.entries,
and JSON.stringify / res.json() serialise null-prototype objects normally.
The only behavioural difference is the intended one:

key=toString        'in'=false typeof=undefined
key=constructor     'in'=false typeof=undefined
key=__proto__       'in'=false typeof=undefined
after write, ({}).x = undefined | own key stored = [ '__proto__' ]

Note that these maps are never handed to callers directly, so the null
prototype does not leak into the public API surface.

[AmaadMartin]

The core change is correct, and I verified the two dev-server claims against the source at the head commit. getRunner uses appName in this.runnerCache at dev/src/server/adk_api_server.ts:1029, and /debug/trace/:eventId reads this.traceDict[eventId] at line 276, so both new dev tests fail on the unfixed code. I also confirmed that no null-prototype map reaches a caller, because mergeStates puts an app: or user: prefix on every key.

Four points below: one test that cannot fail, one sink on the same request path that this change does not close, one sibling map with the same shape, and one helper that you can delete.

[AmaadMartin]

Not a nit. This test passes on the unfixed code, so it does not guard the fix.

stateDelta: {
  [`${State.USER_PREFIX}__proto__`]: {
    baseUrl: 'https://evil.test',
  },
},
...
expect(({} as Record<string, unknown>)['baseUrl']).toBeUndefined();

Before the fix, this.userState['app1']['u1'] is a plain {}. The statement
map['__proto__'] = {baseUrl} calls the inherited __proto__ setter. That
setter changes the prototype of map. It does not write to Object.prototype.
Therefore ({}).baseUrl is undefined before the fix and after the fix.

The real damage before the fix is different. The key leaves the map, so the
user state value is lost. Assert that instead:

const stored = await service.getSession({
  appName: 'app1',
  userId: 'u1',
  sessionId: 's1',
});
expect(stored?.state['user:__proto__']).toEqual({
  baseUrl: 'https://evil.test',
});

mergeStates puts the user: prefix back on each own key
(core/src/sessions/base_session_service.ts:247). Before the fix the map has
no own key, so the value is undefined and the test fails. After the fix
__proto__ is an own key, so the value comes back.

The other five tests in this block are correct. Each one fails on the unfixed
code. I checked each one by hand.

[AmaadMartin]

Nit, optional. The cleanup list is not complete.

const POLLUTED_KEYS = [
  'polluted',
  'httpOptions',
  'pwned',
  'baseUrl',
  'poc_sid',
];

The test does not pollute Object.prototype via appName in app state uses
appName: '__proto__' and userId: 'u1'. On unfixed code, createSession
writes Object.prototype.u1 before it reaches the app: state. The cleanup
leaves that key in place for the tests that follow.

Add 'u1' to the list.

This matters only when a person reverts the fix. That is the moment when the
suite must stay correct.

[AmaadMartin]

Not a nit. The same primitive stays open one level up, on the same request path.

POST /apps/:appName/users/:userId/sessions/:sessionId reads the state from
the request body:

const state = req.body['state'] || {};   // dev/src/server/adk_api_server.ts:447

That value goes to createSession, which calls trimTempState. trimTempState
copies each caller key into a plain object:

// core/src/sessions/base_session_service.ts:230
const filteredState: Record<string, unknown> = {};
for (const [key, value] of Object.entries(state)) {
  if (!key.startsWith(State.TEMP_PREFIX)) {
    filteredState[key] = value;
  }
}

A body of {"state": {"__proto__": {"isAdmin": true}}} does two things.
JSON.parse makes __proto__ an own key, so Object.entries returns it. The
copy then calls the __proto__ setter, the key leaves the map, and the
prototype of the new session state becomes the attacker object.

The session keeps that prototype. State wraps the session state and uses the
in operator:

// core/src/sessions/state.ts:31
get<T>(key: string, defaultValue?: T): T | undefined {
  if (key in this.delta) { ... }
  if (key in this.value) { ... }

So state.get('isAdmin') returns true for that session.
trimTempDeltaState at line 213 has the same shape.

Use Object.create(null) for both filtered maps:

const filteredState: Record<string, unknown> = Object.create(null);

Three notes on this suggestion, because it changes behaviour:

1. The blast radius is one session, not the process. This is one tier below
   the bugs that you fixed here.
2. I traced the prototype through the copies. lodash-es/_initCloneObject
   calls baseCreate(getPrototype(object)), so cloneDeep in getSession and
   in mergeStates keeps the attacker prototype on the returned state.
3. I checked the dependants of the change. The suite holds one
   toStrictEqual, in core/test/agents/functions_test.ts:402, and it does
   not touch session state. A null-prototype state object therefore breaks no
   current assertion. cloneDeep also turns a null-prototype object back into
   a plain {}, so no null-prototype map reaches a caller.

[AmaadMartin]

Nit. The helper is not necessary, and it is here three times.

function createNullProtoMap<T>(): Record<string, T> {
  return Object.create(null) as Record<string, T>;
}

The TypeScript signature of Object.create is (o: object | null): any. An
any value assigns to any type. The cast does nothing. Assign the call
directly:

private sessions: Record<string, Record<string, Record<string, Session>>> =
  Object.create(null);

The three copies are here, at
core/src/auth/credential_service/in_memory_credential_service.ts:22, and at
dev/src/server/adk_api_server.ts:62. Each copy has a different comment. If
you delete all three, you remove approximately 40 of the 269 added lines. Keep
one comment on each field, or one comment on each class.

I checked the lint rule that can object to the direct assignment.
eslint.config.js extends tseslint.configs.recommended, not the type-checked
configuration. @typescript-eslint/no-unsafe-assignment is therefore off.

[AmaadMartin]

Nit. The scoping note in the description covers the outer key only.

The description says that InMemoryMemoryService is not affected, because the
key always contains /. That is true for the outer key. The inner key is the
session ID:

// core/src/memory/in_memory_memory_service.ts:30
if (!this.sessionEvents[userKey]) {
  this.sessionEvents[userKey] = {};
}
this.sessionEvents[userKey][session.id] = session.events.filter(...);

session.id comes from the request path, and it holds no /. A session with
the ID __proto__ replaces the prototype of the inner map. The entry then has
no own key, so Object.values at line 47 steps over it. searchMemory can
never find that session.

This is a silent loss of data, not process pollution. The fix is the same one
line that you use here:

this.sessionEvents[userKey] = Object.create(null);

The claim for InMemoryArtifactService holds. artifactPath always joins the
segments with /, and the map has one level only.