Fix: consolidate path containment into one shared helper and stop over-rejecting '..'-prefixed artifact filenames

[AmaadMartin]

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 — no public issue is open for this.
2. Or, if no issue exists, describe the change:

Problem: core/src had three separate implementations of "is this resolved path inside that directory?", and one of them was wrong.

1. core/src/utils/file_utils.ts — isInsideDir(), module-private, correct and separator-aware.
2. core/src/artifacts/file_artifact_service.ts — assertInsideRoot(), the same correct predicate spelled differently, but with [FileArtifactService] baked into its error string.
3. core/src/artifacts/file_artifact_service.ts — the inline check in getArtifactDir(), which was a defect:

const relative = path.relative(scopeRoot, artifactDir);
if (relative.startsWith('..') || path.isAbsolute(relative)) { throw ... }

relative.startsWith('..') is a bare string-prefix test with no path-separator boundary. path.relative() returns '..foo.txt' for an artifact literally named ..foo.txt, so the file was rejected even though it resolves strictly inside the scope root. This is an over-rejection (a false positive), not a security hole. It was also internally inconsistent: sub/..evil.txt was accepted while ..evil.txt at the top level was rejected, purely because of where the prefix landed.

Reproduction on main:

await service.saveArtifact({
  appName: 'app',
  userId: 'test-user',
  sessionId: 'test-session',
  filename: '..foo.txt',
  artifact: {text: 'hello'},
});
// Throws: Artifact filename ..foo.txt escapes storage directory.

Solution: export the one correct predicate from the shared utils module and delegate both artifact-layer call sites to it, which fixes the defect as a side effect of the consolidation.

• core/src/utils/file_utils.ts — isInsideDir becomes exported, resolves both arguments internally, and its doc comment is now caller-agnostic. It records honestly that the comparison is lexical: case-sensitive on every platform, and silent about symlinks or a TOCTOU race with the following filesystem call. Because the helper now resolves for its callers, materializeFiles' resolvedBaseDir pre-resolve became redundant and was dropped; path.resolve is idempotent, so passing dir straight through is behaviour-identical.
• core/src/artifacts/file_artifact_service.ts — assertInsideRoot is now the thin artifacts-layer wrapper that owns the feature-specific [FileArtifactService] error text; its signature, export, and message are byte-identical (visible in the diff: the message line is unchanged context). The inline path.relative arithmetic in getArtifactDir is replaced by an isInsideDir call.
• core/src/artifacts/file_artifact_service.ts now contains zero inline containment arithmetic, and exactly one containment predicate exists in core/src.

No traversal rejection is lost. Every path that resolves outside base has a relative form starting with .. (or absolute, e.g. a different Windows drive), so the new rejection set is a strict subset of the old one, and every path newly accepted provably resolves inside base. The change can only turn false positives into accepts; it cannot turn a reject into an escape. .., ../x, ../../secret.txt and absolute filenames are still rejected with the unchanged message Artifact filename <name> escapes storage directory.

Behaviour change (one, intended) and its adk-python parity justification. A saveArtifact call with a filename whose first segment merely starts with .. (e.g. ..foo.txt, ..., ..foo/bar.txt) previously threw and now succeeds. This aligns adk-js with adk-python, which is the parity gold standard for artifact-key semantics: src/google/adk/artifacts/file_artifact_service.py _resolve_scoped_artifact_path() uses candidate.relative_to(scope_root_resolved) and raises only on a genuine ValueError, so Python accepts ..foo.txt and ... and rejects .. and ../x — exactly the corrected behaviour here. No stored data is reinterpreted (these names could not previously be written), so there is no migration and no read-path compatibility concern.

The shared helper is deliberately NOT added to the public exports. isInsideDir is exported at module scope only. It is not added to core/src/index.ts or core/src/common.ts, because relocating/exposing an internal helper does not make it public API. assertInsideRoot, assertSafeSegment, getUserRoot and getSessionArtifactsDir remain module-level exports consumed only by the test file, exactly as before.

Collision check (required before implementing): gh pr list --repo AmaadMartin/adk-js --state open --limit 1000 returned 556 open PRs; I filtered for path/containment/artifact-adjacent titles and diffed the candidates with gh pr diff <n> --name-only. Findings:

• Fix: enforce path containment by segment, not string prefix, in materializeFiles #523 and Fix: test path containment, not string prefix, in materializeFiles #371 both re-fix the materializeFiles sibling-prefix hole and touch core/src/utils/file_utils.ts. Neither collides: that fix already landed upstream as 868ca1f3 (Fix: gate the Gemini Live e2e suite on a real Vertex project instead of on CI #603), which is the prerequisite this PR builds on, so both are superseded by main rather than by this PR. Neither touches the artifacts layer or the ..-prefix defect.
• Fix: contain skill script output files in a declared output directory instead of process.cwd() #556 / Fix: require an explicit output directory and resolve the default in SkillToolset #437 / Fix: persist skill script output files to the artifact service instead of the agent process cwd #410 / Fix: require an explicit destination directory so skill script output never lands in process.cwd() #353 / Fix: materialize skill script output into a declared directory, never process.cwd() #298 touch file_utils.ts but only the skill-script output-directory plumbing, not the containment predicate.
• Feat: Persist skill script outputs whenever an artifact service is configured #564 / Feat: Save skill script output files as artifacts (opt-in saveOutputsAsArtifacts) #517 / feat: refactor BaseArtifactService and ScopedArtifactService to use SessionScope (Part 2) #93 / feat(artifacts): refactor ArtifactService to use ArtifactKey composite key #74 / Fix: normalize path separators when detecting input files in UnsafeLocalCodeExecutor (Windows) #522 touch file_artifact_service.ts but none touch getArtifactDir's containment check or assertInsideRoot.

No live PR lands this change, and nothing here needs to be stacked.

Note on the base branch. This branch was cut from a fork main that was 5 commits behind google/adk-js main and therefore did not contain the prerequisite 868ca1f3 (#603). Rather than re-implement isInsideDir (which would have duplicated a merged upstream commit), I fast-forwarded the fork's main to upstream main and rebased onto it. The diff below is only this change.

Review round 1 (complexity audit). Two findings, both accepted and fixed in fe1b438:

1. materializeFiles' const resolvedBaseDir = path.resolve(dir) became dead weight once the helper resolved internally — removed, and its two uses now pass dir directly.
2. The docstring's "not as a security boundary around the filesystem" clause was contradicted by this diff's own caller (assertInsideRoot guards the artifact storage root and is exactly that boundary). The six lines of caveat prose are now two that state the limitation rather than prescribing a conclusion.

The reviewer's non-blocking note — that assertInsideRoot's parameter is still named resolvedPath — is deliberately not actioned: keeping that signature byte-identical is a requirement of this change, and a parameter rename is not worth spending it.

After both fixes I re-ran mutation M1 to confirm the simplified call sites still route through the shared helper: dropping the separator arm still fails both the new predicate test and the pre-existing #603 materializeFiles sibling test.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.

Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

Commands run on the exact pushed commit:

# the two touched test files, plus the three downstream consumers of
# materializeFiles (which this change also simplifies):
npx vitest run --project unit:core \
  core/test/utils/file_utils_test.ts \
  core/test/artifacts/file_artifact_service_test.ts \
  core/test/tools/skills/run_skill_script_tool_test.ts \
  core/test/tools/skills/run_skill_inline_script_tool_test.ts \
  core/test/code_executors/unsafe_local_code_executor_test.ts
#  Test Files  5 passed (5)   Tests  97 passed (97)

npm run build        # ok
npm run lint         # exit 0, clean
npm run format:check # "All matched files use Prettier code style!"

New tests added (no existing test was edited, deleted, skipped, or weakened):

core/test/utils/file_utils_test.ts, new describe('isInsideDir') — accepts the base itself (equality arm); accepts a nested path (separator arm); rejects a sibling sharing the base's prefix (/tmp/agent vs /tmp/agent-evil/x); rejects the parent; accepts <base>/..foo.txt; resolves unnormalized arguments. All use path.join/path.sep so they hold on Windows CI.

core/test/artifacts/file_artifact_service_test.ts, new its in describe('path security') — ..foo.txt saves, lands at <sessionArtifacts>/..foo.txt/versions/0, and round-trips through loadArtifact; a bare .. filename still rejects with escapes storage directory; a . filename still lands under the artifact sentinel directory; and assertInsideRoot rejects a sibling sharing a name prefix with root.

Coverage. Every line and branch added or changed by this PR is covered — measured with --coverage.provider=v8 and read out of coverage-final.json, the uncovered-line sets for both touched source files contain none of the changed lines. Both arms of the || in isInsideDir and both sides of all three guards in the touched artifact code execute (non-zero branch counters). The file-level percentages are below the repo's global threshold, but every remaining gap is pre-existing and outside this diff (materializeFiles' second defence-in-depth guard, guessMimeType, and unrelated file_artifact_service.ts error paths); I did not touch them, since widening this diff to chase a file-level number is exactly the unrelated churn the contribution guide asks us not to bundle.

Proof the tests can fail (each mutation applied to the fixed code, tests run, then reverted):

• M1 — dropped the separator arm in isInsideDir (return resolvedTarget.startsWith(resolvedBase);). Two failures: the new rejects a sibling whose name shares the base directory prefix (AssertionError: expected true to be false) and the pre-existing Fix: gate the Gemini Live e2e suite on a real Vertex project instead of on CI #603 test should throw an error if file attempts to escape into a sibling directory sharing a name prefix (AssertionError: promise resolved "[ { …(4) } ]" instead of rejecting) — confirming the exported helper really is the one materializeFiles uses.
• M2 — restored the old inline check at getArtifactDir (relative.startsWith('..') || path.isAbsolute(relative)). accepts a filename that starts with two dots fails with exactly the reported bug: Error: Artifact filename ..foo.txt escapes storage directory.
• M3 — deleted the artifactDir === resolvedScopeRoot early return. stores a '.' filename under the artifact sentinel directory fails.
• M4 — inverted the wrapper condition (if (isInsideDir(...)) throw). The three pre-existing assertInsideRoot tests fail along with 20+ others, confirming the wrapper is still wired into every path.

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

Run npm run build, then against the built package construct new FileArtifactService(await fs.mkdtemp(...)) and:

1. Save and load report.pdf, ..foo.txt, nested/dir/report.txt and user:notes.txt. All four round-trip and all four appear in listArtifactKeys; ..foo.txt comes back under its original name.
2. Confirm ..foo.txt is stored inside the scope root at <root>/users/<uid>/sessions/<sid>/artifacts/..foo.txt/versions/0.
3. Confirm saveArtifact with filename: '../../secret.txt' still rejects with escapes storage directory, and that fs.access on the parent-directory targets rejects — nothing was written outside the temp root.
4. Confirm materializeFiles still writes out/result.txt inside its directory and still rejects both ../escape.txt and a sibling-prefix escape, with no sibling directory created.

I ran exactly this as a scratch script against the built package; all checks passed.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

Note: npm run ts:check is red on this branch, but it is equally red on main — 281 pre-existing TS2345 errors about BASE_AGENT_SIGNATURE_SYMBOL across the test tree. I diffed the error sets before and after my change and they are identical, so nothing here contributes to it and I did not attempt to fix unrelated breakage.