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 1 commit
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
26 changes: 16 additions & 10 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 @@ -212,12 +214,16 @@ 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', string, string, string, string]
| ['user', string, string, string];

@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.

Nit, optional.

The declaration is positional, so the reads in listArtifactKeys (key[3] for user scope, key[4] for session scope) are hard to check without scrolling back here.

type ArtifactStorageKey =
  | ['session', string, string, string, string]
  | ['user', string, string, string];

Labelled tuple members are erased at runtime and make the indices self-explanatory:

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

Genuine trade-off: prettier will wrap the session arm as shown, so 3 lines become 9. Take it or leave it — I checked the labels don't affect the key[0] === 'session' narrowing.

Separately, while you're in this area: the JSDoc above artifactPath (lines 201-209) still reads "Constructs the path to the artifact" / "@return The path to the artifact", and every caller still names the result path. The point of this change is that the value is no longer a path, and leaving the wording invites someone to "simplify" it back to slashes later. Renaming the function would touch six call sites, so a one-line doc tweak is probably the right size of fix.


/**
* Checks if the filename has a user namespace prefix.
*
Expand Down
30 changes: 29 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,40 @@
*/

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 artifact = await service.loadArtifact({
appName: 'app',
userId: 'user',
sessionId: 'session',
filename: 'nested/report.txt',
});

expect(artifact?.text).toBe('artifact-a');

@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.

This pins the separator collision from #574, but not the other half of the change. The scope marker also fixes a cross-session listing leak, and nothing exercises it.

Under the old key scheme:

const usernamespacePrefix = `${appName}/${userId}/user/`;
...
} else if (path.startsWith(usernamespacePrefix)) {

an artifact saved into a session literally named user was stored at app/<user>/user/foo.txt, which matches usernamespacePrefix — so listArtifactKeys for any other session of the same user returned foo.txt. The 'session' / 'user' discriminant is what stops that, and it is the part the PR description calls out ("the explicit scope marker also keeps user-scoped artifacts separate"), but a regression would go unnoticed:

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([]);
});

I traced this by hand rather than running it: on the pre-change code the save lands at app/user/user/foo.txt and the usernamespacePrefix branch pushes foo.txt, so it fails; on your code key[0] === 'session' with key[3] === 'user' !== 'other' skips it, so it passes.

Also worth one more line in the test above — loading {sessionId: 'session/nested', filename: 'report.txt'} and asserting artifact-b — so the assertion pins both sides of the split instead of only proving the first key won.

});
});