Skip to content

fix(security): path-traversal guards for chunkCache and fetch-chunk (CWE-22) - #2

Open
jamubc wants to merge 1 commit into
masterfrom
security/cwe22-path-traversal
Open

fix(security): path-traversal guards for chunkCache and fetch-chunk (CWE-22)#2
jamubc wants to merge 1 commit into
masterfrom
security/cwe22-path-traversal

Conversation

@jamubc

@jamubc jamubc commented Jun 3, 2026

Copy link
Copy Markdown
Owner

Summary

Closes the CWE-22 path-traversal vector in the chunk cache subsystem.

  • getChunks() — format validation (primary guard): adds a ^[a-f0-9]{8}$ regex check as the first statement. Any cacheKey that is not exactly 8 lowercase hex characters (i.e. a sha256 slice) is rejected immediately. This blocks ../, absolute paths, and other traversal payloads at the source, protecting every caller regardless of entry point.
  • getChunks() — path containment (defense-in-depth): adds path.resolve + startsWith(CACHE_DIR + sep) to verify the resolved file path stays inside CACHE_DIR even after format validation passes.
  • getChunks() — remove DELETE primitive: the catch block previously called fs.unlinkSync(filePath) on any parse error. With an unvalidated cacheKey this was a path-traversal-driven delete. The unlink is removed; the file is left in place on error.
  • fetch-chunk.tool.ts — entry validation: adds the same ^[a-f0-9]{8}$ check at the tool boundary before calling getChunks().
  • ask-qwen.tool.ts — entry validation: adds format validation on chunkCacheKey in the cache-continuation path, which previously bypassed the fetch-chunk check entirely.

Test Plan

  • npm run lint (tsc --noEmit) passes clean
  • Manual: valid 8-char hex key retrieves cached chunks as before
  • Manual: keys such as ../../etc, /etc/passwd, abc are rejected with a clear error message

…CWE-22)

- Add ^[a-f0-9]{8}$ format validation as the first check in getChunks()
  so every caller is protected regardless of entry point.
- Add path.resolve + startsWith(CACHE_DIR + sep) containment check as
  defense-in-depth.
- Remove silent fs.unlinkSync on parse errors in getChunks(); the previous
  behaviour created a DELETE path-traversal primitive when cacheKey was
  not validated upstream.
- Add cacheKey format validation at the entry of fetch-chunk.tool.ts.
- Add chunkCacheKey format validation in ask-qwen.tool.ts, which previously
  bypassed the fetch-chunk check entirely.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces input validation for cache keys across tools and utility functions, ensuring they conform to an 8-character hexadecimal format to prevent path traversal vulnerabilities. It also adds a defense-in-depth path containment check in the chunk cache utility and removes the automatic file deletion on read errors to mitigate potential path-traversal deletion risks. The review feedback suggests a more robust and platform-independent way to perform the path containment check using path.relative instead of string concatenation.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/utils/chunkCache.ts
Comment on lines +72 to +76
const resolved = path.resolve(filePath);
if (!resolved.startsWith(path.resolve(CACHE_DIR) + path.sep)) {
Logger.debug(`Rejected cacheKey escaping CACHE_DIR: ${cacheKey}`);
return null;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Using string concatenation (path.resolve(CACHE_DIR) + path.sep) for path containment checks can be fragile and may fail on certain platforms or edge cases (for example, if CACHE_DIR resolves to a root directory like / or C:\).

A more robust and standard way to verify that a resolved path remains within a target directory in Node.js is to use path.relative and check if the result starts with .. or is absolute.

Suggested change
const resolved = path.resolve(filePath);
if (!resolved.startsWith(path.resolve(CACHE_DIR) + path.sep)) {
Logger.debug(`Rejected cacheKey escaping CACHE_DIR: ${cacheKey}`);
return null;
}
const relative = path.relative(CACHE_DIR, filePath);
if (!relative || relative.startsWith('..') || path.isAbsolute(relative)) {
Logger.debug(`Rejected cacheKey escaping CACHE_DIR: ${cacheKey}`);
return null;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant