Skip to content

fix(artifacts): isolate in-memory composite keys #576

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kalenkevich merged 3 commits into from
Aug 4, 2026
+104 −12
Merged

fix(artifacts): isolate in-memory composite keys #576

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
18004a4
fix(artifacts): isolate in-memory composite keys
fallintoplace Jul 30, 2026
a025180
test(artifacts): cover storage key boundaries
fallintoplace Jul 30, 2026
d0ff108
refactor(artifacts): encode in-memory key segments
fallintoplace Jul 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 24 additions & 12 deletions core/src/artifacts/in_memory_artifact_service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,17 +101,19 @@ export class InMemoryArtifactService implements BaseArtifactService {
userId,
sessionId,
}: ListArtifactKeysRequest): Promise<string[]> {
const sessionPrefix = `${appName}/${userId}/${sessionId}/`;

@kalenkevich kalenkevich Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just encode/decode every part of the path (appName, userId, sessionId, filename) to not to have the delimiter (/)?

No need to create an array as a key. I think this is antipatern and better to keep keys as numbers or strings but not as objects.

const usernamespacePrefix = `${appName}/${userId}/user/`;
const filenames: string[] = [];

for (const path in this.artifacts) {
if (path.startsWith(sessionPrefix)) {
const filename = path.replace(sessionPrefix, '');
filenames.push(filename);
} else if (path.startsWith(usernamespacePrefix)) {
const filename = path.replace(usernamespacePrefix, '');
filenames.push(filename);
const key = JSON.parse(path) as ArtifactStorageKey;
if (
key[0] === 'session' &&
key[1] === appName &&
key[2] === userId &&
key[3] === sessionId
) {
filenames.push(key[4]);
} else if (key[0] === 'user' && key[1] === appName && key[2] === userId) {
filenames.push(key[3]);
}
}

Expand Down Expand Up @@ -197,13 +199,13 @@ export class InMemoryArtifactService implements BaseArtifactService {
}

/**
* Constructs the path to the artifact.
* Constructs the storage key for the artifact.
*
* @param appName The app name.
* @param userId The user ID.
* @param sessionId The session ID.
* @param filename The filename.
* @return The path to the artifact.
* @return The encoded storage key for the artifact.
*/
function artifactPath(
appName: string,
Expand All @@ -212,12 +214,22 @@ function artifactPath(
filename: string,
): string {
if (fileHasUserNamespace(filename)) {
return `${appName}/${userId}/user/${filename}`;
return JSON.stringify(['user', appName, userId, filename]);
}

return `${appName}/${userId}/${sessionId}/${filename}`;
return JSON.stringify(['session', appName, userId, sessionId, filename]);

@AmaadMartin AmaadMartin Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a nit, but out of scope for this PR — worth a follow-up issue rather than a change here.

The encoding on this line is right, but GcsArtifactService still builds its object names the way this file used to, so the exact repro from #574 conflates artifacts there too:

// core/src/artifacts/gcs_artifact_service.ts:283-285
const prefix = isUser
  ? `${appName}/${userId}/user/${cleanFilename}`
  : `${appName}/${userId}/${sessionId}/${cleanFilename}`;

{sessionId: 'session', filename: 'nested/report.txt'} and {sessionId: 'session/nested', filename: 'report.txt'} both resolve to app/user/session/nested/report.txt/<version>.

It is actually worse there than it was here. GCS strips the user: prefix before writing (cleanFilename, line 281), so a session literally named user collides with the user namespace outright: {sessionId: 'user', filename: 'foo.txt'} and {filename: 'user:foo.txt'} land on the same blob. The in-memory service never had that particular collision, because it kept user: inside the stored filename.

For contrast, FileArtifactService is the sibling that already gets this right — assertSafeSegment (file_artifact_service.ts:411) rejects separators in userId/sessionId, and user- vs session-scoped artifacts live under structurally distinct roots.

I am not asking you to fix GCS in this PR; flagging it so #574 doesn't get closed as fully resolved when only one of the two implementations is fixed.

}

type ArtifactStorageKey =
| [
'session',
appName: string,
userId: string,
sessionId: string,
filename: string,
]
| ['user', appName: string, userId: string, filename: string];

/**
* Checks if the filename has a user namespace prefix.
*
Expand Down
57 changes: 56 additions & 1 deletion core/test/artifacts/in_memory_artifact_service_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,67 @@
*/

import {InMemoryArtifactService} from '@google/adk';
import {describe} from 'vitest';
import {describe, expect, it} from 'vitest';
import {runArtifactServiceTests} from './artifact_service_test_utils.js';

describe('InMemoryArtifactService', () => {
runArtifactServiceTests(
async () => new InMemoryArtifactService(),
async () => {},
);

it('keeps artifacts with ambiguous path components isolated', async () => {
const service = new InMemoryArtifactService();

await service.saveArtifact({
appName: 'app',
userId: 'user',
sessionId: 'session',
filename: 'nested/report.txt',
artifact: {text: 'artifact-a'},
});
await service.saveArtifact({
appName: 'app',
userId: 'user',
sessionId: 'session/nested',
filename: 'report.txt',
artifact: {text: 'artifact-b'},
});

const artifactA = await service.loadArtifact({
appName: 'app',
userId: 'user',
sessionId: 'session',
filename: 'nested/report.txt',
});
const artifactB = await service.loadArtifact({
appName: 'app',
userId: 'user',
sessionId: 'session/nested',
filename: 'report.txt',
});

expect(artifactA?.text).toBe('artifact-a');
expect(artifactB?.text).toBe('artifact-b');
});

it('does not leak a session named user into other sessions', async () => {
const service = new InMemoryArtifactService();

await service.saveArtifact({
appName: 'app',
userId: 'user',
sessionId: 'user',
filename: 'foo.txt',
artifact: {text: 'session-scoped'},
});

const keys = await service.listArtifactKeys({
appName: 'app',
userId: 'user',
sessionId: 'other',
});

expect(keys).toEqual([]);
});
});