fix(mcp-server): validate snapshot cache id and combined shape - #21
fix(mcp-server): validate snapshot cache id and combined shape#21j-srodka wants to merge 1 commit into
Conversation
- Reject non-hex snapshot_id at Zod boundary and before fs reads - Parse cached JSON through isCombinedWorkspaceSnapshot; require id match - Refuse cache writes with malformed snapshot_id; document trusted root_path - Log decision in v0-decisions; smoke tests for invalid id and corrupt cache Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
This PR hardens the mcp-server snapshot cache interface by strictly validating snapshot_id values and verifying cached snapshot JSON structure before it is consumed by tools/adapters, improving safety against traversal and cache corruption.
Changes:
- Add a runtime type guard for cached combined snapshots (
isCombinedWorkspaceSnapshot) and use it when reading cached JSON. - Enforce
snapshot_idas a 64-char lowercase hex digest at both the Zod tool boundary and before any snapshot cache filesystem access/writes. - Add smoke tests for invalid
snapshot_idinputs and corrupt cache JSON; document the trustedroot_pathmodel and record the decision.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/mcp-server/src/snapshot-guards.ts | Adds isCombinedWorkspaceSnapshot structural validation for cached combined snapshots. |
| packages/mcp-server/src/snapshot-cache.ts | Validates snapshot ids before fs access; validates parsed cache JSON shape and id match. |
| packages/mcp-server/src/smoke.test.ts | Adds smoke coverage for invalid snapshot_id and corrupt cache handling. |
| packages/mcp-server/src/schemas.ts | Tightens tool input validation for snapshot_id via Zod regex. |
| packages/mcp-server/README.md | Documents trusted root_path model and snapshot cache validation behavior. |
| docs/impl/v0-decisions.md | Records the snapshot cache validation decision and rationale. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -30,7 +35,7 @@ const adapterFingerprintSchema = z.object({ | |||
| /** Exactly one of snapshot_id or snapshot is validated in the apply_batch handler (MCP boundary must stay permissive so tool errors are consistent). */ | |||
There was a problem hiding this comment.
The doc comment above applyBatchInputSchema says snapshot_id validation is handled in the apply_batch handler and that the MCP boundary must stay permissive, but snapshot_id is now constrained at the Zod boundary via snapshotCacheIdSchema. Please update the comment to reflect the new behavior (or adjust the schema if the intent is still to accept any string and validate later).
| /** Exactly one of snapshot_id or snapshot is validated in the apply_batch handler (MCP boundary must stay permissive so tool errors are consistent). */ | |
| /** | |
| * `snapshot_id`, when provided, is validated at the MCP/Zod boundary as a materialized snapshot id. | |
| * The apply_batch handler still validates the cross-field rule that exactly one of `snapshot_id` | |
| * or `snapshot` is supplied so tool errors remain consistent. | |
| */ |
| /** All materialized snapshot ids in this stack are SHA-256 hex (64 lowercase hex chars). */ | ||
| const SNAPSHOT_CACHE_ID_RE = /^[0-9a-f]{64}$/; | ||
|
|
||
| export function isValidSnapshotCacheId(snapshotId: string): boolean { | ||
| return SNAPSHOT_CACHE_ID_RE.test(snapshotId); | ||
| } |
There was a problem hiding this comment.
snapshot_id validation regex is duplicated here (SNAPSHOT_CACHE_ID_RE) and in schemas.ts (snapshotCacheIdSchema). To avoid drift (e.g., changing allowed casing/length in one place but not the other), consider centralizing the canonical regex/validator (export a shared SNAPSHOT_CACHE_ID_RE/isValidSnapshotCacheId and reuse it in the Zod schema via .refine(...), or export the Zod schema and reuse it here).
Made-with: Cursor