fix: add Zod schema validation to readModels disk cache - #1099
fix: add Zod schema validation to readModels disk cache#1099daewoongoh wants to merge 2 commits into
Conversation
…dation logic - Extract `validateModelRecord` helper to deduplicate Zod validation logic - Add `modelRecordSchema.safeParse()` validation to `readModels` disk cache reads - Update tests to ensure Zod validation strictly enforces `ModelRecord` schema This resolves a vulnerability where a corrupted or tampered disk cache JSON file could lead to the injection of unexpected properties.
📝 WalkthroughWalkthroughThe model cache now validates JSON data against the model record schema during asynchronous and synchronous reads. Invalid data returns ChangesModel Cache Validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/api/providers/fetchers/__tests__/modelCache.spec.ts (1)
360-385: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTest the production cache-loading path.
This block validates a duplicate local schema. It does not exercise
validateModelRecordor the asynchronous and synchronous cache readers. The tests can pass if production code returns raw disk data or rejects malformed JSON incorrectly.Add tests through the existing public cache-loading entry points. Assert that valid data is returned and cached, invalid data returns
undefined, malformed JSON does not reject the read, and invalid data does not entermemoryCache. Also define the expected handling for unexpected properties and__proto__keys.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/fetchers/__tests__/modelCache.spec.ts` around lines 360 - 385, Replace the duplicate local schema tests in “validateModelRecord schema validation” with coverage through the public synchronous and asynchronous cache-loading entry points, verifying valid records are returned and cached, invalid records return undefined without entering memoryCache, and malformed JSON reads resolve without rejection. Add explicit assertions for unexpected properties and “__proto__” keys using the production schema behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/api/providers/fetchers/modelCache.ts`:
- Around line 220-228: Update readModels to catch JSON.parse or file-read syntax
failures for malformed or truncated cache data and return undefined instead of
rejecting before validateModelRecord runs. Match the existing synchronous
cache-read behavior, while preserving validation for successfully parsed data.
---
Nitpick comments:
In `@src/api/providers/fetchers/__tests__/modelCache.spec.ts`:
- Around line 360-385: Replace the duplicate local schema tests in
“validateModelRecord schema validation” with coverage through the public
synchronous and asynchronous cache-loading entry points, verifying valid records
are returned and cached, invalid records return undefined without entering
memoryCache, and malformed JSON reads resolve without rejection. Add explicit
assertions for unexpected properties and “__proto__” keys using the production
schema behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 64a5cfe1-be7e-429e-8fa8-f3bec2a55c24
📒 Files selected for processing (2)
src/api/providers/fetchers/__tests__/modelCache.spec.tssrc/api/providers/fetchers/modelCache.ts
| async function readModels(cacheKey: string): Promise<ModelRecord | undefined> { | ||
| const filename = `${cacheKeyToFilename(cacheKey)}_models.json` | ||
| const cacheDir = await getCacheDirectoryPath(ContextProxy.instance.globalStorageUri.fsPath) | ||
| const filePath = path.join(cacheDir, filename) | ||
| const exists = await fileExistsAtPath(filePath) | ||
| return exists ? JSON.parse(await fs.readFile(filePath, "utf8")) : undefined | ||
| if (!exists) return undefined | ||
|
|
||
| const data = JSON.parse(await fs.readFile(filePath, "utf8")) | ||
| return validateModelRecord(data, cacheKey) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle syntax errors in asynchronous cache reads.
JSON.parse at Line 227 throws for malformed or truncated _models.json files. The exception rejects readModels before validation runs. The synchronous path already catches this failure. Return undefined from both paths for corrupted disk data, as required by the PR objective.
Suggested error boundary
async function readModels(cacheKey: string): Promise<ModelRecord | undefined> {
+ try {
const filename = `${cacheKeyToFilename(cacheKey)}_models.json`
const cacheDir = await getCacheDirectoryPath(ContextProxy.instance.globalStorageUri.fsPath)
const filePath = path.join(cacheDir, filename)
const exists = await fileExistsAtPath(filePath)
if (!exists) return undefined
const data = JSON.parse(await fs.readFile(filePath, "utf8"))
return validateModelRecord(data, cacheKey)
+ } catch (error) {
+ console.error(`[MODEL_CACHE] Error loading ${cacheKey} models from disk:`, error)
+ return undefined
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function readModels(cacheKey: string): Promise<ModelRecord | undefined> { | |
| const filename = `${cacheKeyToFilename(cacheKey)}_models.json` | |
| const cacheDir = await getCacheDirectoryPath(ContextProxy.instance.globalStorageUri.fsPath) | |
| const filePath = path.join(cacheDir, filename) | |
| const exists = await fileExistsAtPath(filePath) | |
| return exists ? JSON.parse(await fs.readFile(filePath, "utf8")) : undefined | |
| if (!exists) return undefined | |
| const data = JSON.parse(await fs.readFile(filePath, "utf8")) | |
| return validateModelRecord(data, cacheKey) | |
| async function readModels(cacheKey: string): Promise<ModelRecord | undefined> { | |
| try { | |
| const filename = `${cacheKeyToFilename(cacheKey)}_models.json` | |
| const cacheDir = await getCacheDirectoryPath(ContextProxy.instance.globalStorageUri.fsPath) | |
| const filePath = path.join(cacheDir, filename) | |
| const exists = await fileExistsAtPath(filePath) | |
| if (!exists) return undefined | |
| const data = JSON.parse(await fs.readFile(filePath, "utf8")) | |
| return validateModelRecord(data, cacheKey) | |
| } catch (error) { | |
| console.error(`[MODEL_CACHE] Error loading ${cacheKey} models from disk:`, error) | |
| return undefined | |
| } | |
| } |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 226-226: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(filePath, "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/api/providers/fetchers/modelCache.ts` around lines 220 - 228, Update
readModels to catch JSON.parse or file-read syntax failures for malformed or
truncated cache data and return undefined instead of rejecting before
validateModelRecord runs. Match the existing synchronous cache-read behavior,
while preserving validation for successfully parsed data.
…s are fully exercised
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/api/providers/fetchers/__tests__/modelCache.spec.ts (1)
387-401: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that invalid disk data is not cached.
The test verifies the return value but does not verify the memory-cache boundary. Assert that
mockCache.setwas not called after validation fails.Proposed test update
expect(result).toBeUndefined() + expect(mockCache.set).not.toHaveBeenCalled() expect(consoleErrorSpy).toHaveBeenCalledWith(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/fetchers/__tests__/modelCache.spec.ts` around lines 387 - 401, Update the invalid-schema test for getModelsFromCache to assert that mockCache.set was not called after validation fails, while preserving the existing undefined result and error-log assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/api/providers/fetchers/__tests__/modelCache.spec.ts`:
- Around line 387-401: Update the invalid-schema test for getModelsFromCache to
assert that mockCache.set was not called after validation fails, while
preserving the existing undefined result and error-log assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c935076f-4839-4fc5-a6af-efe61b46cbbf
📒 Files selected for processing (1)
src/api/providers/fetchers/__tests__/modelCache.spec.ts
1 participant
Related GitHub Issue
Closes: #1098
Description
This PR addresses the missing schema validation vulnerability in the model disk cache identified in the recent analysis report.
validateModelRecordhelper to deduplicate code.modelRecordSchema.safeParse()to thereadModelsfunction to ensure disk cache reads are strictly validated before use.undefined, preventing injection of unexpected properties.modelCache.spec.tsto ensure the Zod schema behaves exactly as expected for valid/invalid records.Test Procedure
modelRecordSchemaaccepts valid records, empty records, and strictly rejects arrays, primitives, and improperly typed fields.npx vitest run api/providers/fetchers/__tests__/modelCache.spec.ts. All 48 tests pass successfully.turbo check-types) pass completely.Pre-Submission Checklist
Visual Snapshots
N/A (Backend cache logic only)
Videos (interaction / animation only)
N/A
Documentation Updates
Additional Notes
N/A
Get in Touch
hehegwk_23849
Summary by CodeRabbit