-
Notifications
You must be signed in to change notification settings - Fork 8.9k
fix: Zod validation, and type safety across servers #3007
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.
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
base: main
Are you sure you want to change the base?
fix: Zod validation, and type safety across servers #3007
Conversation
Schema defines thoughtNumber, totalThoughts, revisesThought, and branchFromThought as integers, but validation only checked type number. This allowed floats (1.5, 3.7) to pass validation. Added Number.isInteger() checks after type validation. Added 4 test cases for float rejection. Bug: Lines 30-34, 46-48 in original lib.ts
No validation on thought string length allows DoS via unbounded memory. Added MAX_THOUGHT_SIZE constant (100KB) and validation check. Added 2 test cases: max size accepted, max+1 rejected. Bug: Lines 27-29 in original lib.ts
Bug 3: Branch arrays could grow unbounded, bypassing global MAX_THOUGHT_HISTORY. Each branch tracked separately with no per-branch limit. Bug 4: parseInt() on env vars accepted absurd values (200000, -1, NaN). No bounds checking on MAX_THOUGHT_HISTORY, MAX_BRANCHES. Fixes: - Added parseEnvInt() helper with bounds validation (1-100000) - Added MAX_THOUGHTS_PER_BRANCH limit (default: 1000) - Enforce FIFO eviction per branch when limit exceeded - Enforce LRU eviction of branches when MAX_BRANCHES exceeded - Added 3 integration tests for memory limits Bugs: Lines 91-96 (unbounded branches), lines 17,21 (env validation)
Border calculation used header.length which includes invisible ANSI escape codes from chalk colors. This caused misaligned borders where the visual width didn't match the calculated width. Added stripAnsi() method using regex to remove ANSI escape sequences before calculating visual length for border sizing. Added 2 tests verifying border alignment with ANSI codes present. Bug: Lines 119-120 in original lib.ts (now 125-128)
Create comprehensive integration tests covering: - ListToolsRequest returns correct schema (all 9 fields) - Valid CallToolRequest processes correctly - Invalid arguments return proper errors (missing thought, non-integer) - Unknown tool name returns error - Server initialization works - Tool schema matches runtime validation Increases test coverage to 42 passing tests. Target: verify index.ts handlers work correctly with lib.ts. Test coverage: lib.ts 96%, total 42 tests passing.
…chId Add runtime validation for optional fields in lib.ts (lines 89-109): - isRevision: boolean type check - needsMoreThoughts: boolean type check - branchId: string type check, non-empty, max 256 chars Add 5 new tests verifying validation errors for: - Non-boolean isRevision - Non-boolean needsMoreThoughts - Non-string branchId - Empty branchId - branchId exceeding 256 characters Test coverage: 47 tests passing, lib.ts 96.4% coverage.
Change comment on line 186 from "LRU-style eviction" to "FIFO eviction (oldest branch first)". The implementation uses Object.keys()[0] which returns the first inserted key (FIFO), not least recently used (LRU). The comment was misleading about the actual eviction strategy.
Split thought validation into two distinct checks (lib.ts lines 48-53): 1. Type check: "must be a string" (for non-string values) 2. Length check: "cannot be empty" (for empty strings) Previously, the combined check (!data.thought || typeof !== 'string') returned "must be a string" for both missing/empty and non-string values, making it unclear what the actual problem was. Also fix TypeScript error in index.test.ts by adding type assertion for schema.required field. Test coverage: 47 tests passing, lib.ts 96.47% coverage.
- Replace 80-line manual validation with Zod schema
- Add comprehensive validation rules:
* Integer validation for thoughtNumber, totalThoughts, revisesThought, branchFromThought
* Lower bounds (.min(1)) to reject negative and zero values
* Size limits (100KB for thought, 256 char for branchId)
* Logical constraints via .refine():
- revisesThought < thoughtNumber
- branchFromThought < thoughtNumber
- isRevision=true requires revisesThought
- branchFromThought requires branchId
- Auto-generate JSON Schema for MCP tool definition via zodToJsonSchema
- Improve error messages with field paths and validation reason
- Add 8 new tests for logical constraints (47/47 tests passing)
- 100% function coverage, 94% statement coverage
Fixes all validation vulnerabilities discovered in adversarial testing.
Enables manual workflow triggering via GitHub Actions UI for testing and validation.
Fixes TypeScript error where code attempted to access .text property on a union type that could be text, image, or audio content. Added explicit type check to ensure content is text before accessing .text property.
Adds 'npm test' command at root to execute tests across all TypeScript servers. Servers without test scripts are skipped.
Ensures npm ci runs before tests to install all workspace dependencies, and uses --if-present to skip servers without test scripts.
domdomegg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah unfortunately I think we did something effectively similar recently with updating everything to the newer SDK endpoints
If there are other things here that are valuable to port across can you rebase or recreate this PR? Thanks!
|
(It also might be worth splitting this up into smaller chunks, where possible, to make reviewing easier and reduce # of merge conflicts in future) |
Description
This PR fixes critical validation vulnerabilities and improves robustness across multiple MCP servers:
Sequential Thinking Server (Primary Focus):
Everything Server:
Development Infrastructure:
workflow_dispatchtriggers for manual CI runsServer Details
Motivation and Context
Critical Validation Bugs (Sequential Thinking Server)
Bug 1: Float Injection
thoughtNumber,totalThoughts,revisesThought,branchFromThoughttypeof === 'number', allowing floats (1.5, 3.7)Bug 2: DoS via Unbounded Thought Strings
Bug 3: Unbounded Branch Memory
MAX_THOUGHT_HISTORYBug 4: Environment Variable Validation
parseInt()accepted absurd values (200000, -1, NaN)MAX_THOUGHT_HISTORY,MAX_BRANCHESBug 5: Display Bug (ANSI Codes)
header.lengthincluding invisible ANSI escape codesBug 6: Type Safety (Everything Server)
.textproperty on union type without type guardBug 7: Missing Logical Constraints
revisesThought < thoughtNumberisRevision=truerequiresrevisesThoughtbranchFromThoughtrequiresbranchIdSolution
Zod Migration (Sequential Thinking)
Replaced 80-line manual validation with declarative Zod schema:
Key Improvements:
.int().min(1)) to reject negatives/zero.refine():revisesThought < thoughtNumberbranchFromThought < thoughtNumberisRevision=truerequiresrevisesThoughtbranchFromThoughtrequiresbranchIdzodToJsonSchemaMemory Management
MAX_BRANCHESexceededDisplay Fix
Type Safety (Everything Server)
How Has This Been Tested?
Sequential Thinking Server:
Everything Server:
Infrastructure:
npm testat root runs tests across all workspace serversnpm ciinstalls dependencies before tests--if-presentskips servers without test scriptsTesting Methodology:
Adversarial testing approach - attempted to break validation with:
Breaking Changes
No breaking changes. All modifications maintain backward compatibility:
Types of changes
Checklist
Additional context
Test Coverage
Before:
After:
Files Changed
Environment Variables (Sequential Thinking)
Security Improvements
Performance Impact