mcp-server-reviewing

MCP Server Reviewing

Purpose

Audit MCP (Model Context Protocol) servers for security issues, architectural violations, missing validation, and production readiness. Ensure servers follow SDK best practices.

When to Use This Skill

• Reviewing MCP server code in PRs
• Auditing existing MCP servers for quality
• Pre-release security and quality checks
• Verifying MCP servers follow best practices

When NOT to Use This Skill

• Writing new MCP servers (use mcp-server-writing)
• General TypeScript/Python code review (use language-specific reviewers)
• Reviewing non-MCP server code

---

Severity Classification

Severity	Description	Action
CRITICAL	Security vulnerabilities, secrets exposure, data leaks	Block PR, fix immediately
HIGH	Missing validation, error swallowing, transport violations	Block PR, require fix
MEDIUM	Missing descriptions, poor error messages, logging issues	Request fix before merge
LOW	Style issues, optimization opportunities, minor improvements	Suggest improvements

---

Automated Detection Commands

Run these commands to detect common violations. Each command includes the violation code for reference.

Security Violations (S1-S5)

S1: Hardcoded Secrets

grep -rn "api[_-]\?key\s*[:=]" --include="*.ts" --include="*.py" src/ | grep -v "process\.env\|os\.environ"

S2: Console.log to stdout (breaks stdio transport)

grep -rn "console\.log\|print(" --include="*.ts" --include="*.py" src/ | grep -v "console\.error\|sys\.stderr\|\.test\.\|\.spec\."

S3: Missing ReDoS Protection

grep -rn "new RegExp\|\.match\|\.replace\|\.split" --include="*.ts" --include="*.py" src/ | grep -v "safeInput\|MAX_INPUT"

S4: Unsafe eval/exec

grep -rn "eval(\|exec(\|Function(" --include="*.ts" --include="*.py" src/

S5: SQL Injection Risk

grep -rn "query.*\`\|execute.*f\"\|cursor\.execute.*%" --include="*.ts" --include="*.py" src/

Architecture Violations (A1-A5)

A1: Missing Tool Descriptions

grep -rn "name:\s*['\"]" --include="*.ts" src/ -A 3 | grep -B 3 "inputSchema" | grep -v "description"

A2: Missing inputSchema Property Descriptions

grep -rn "properties:" --include="*.ts" src/ -A 10 | grep -E "type.*string|type.*number" | grep -v "description"

A3: Tools Without Validation

grep -rn "request\.params\|args\." --include="*.ts" src/ | grep -v "validate\|ajv\|zod\|schema"

A4: Missing isError Flag

grep -rn "content.*error\|success.*false" --include="*.ts" src/ | grep -v "isError"

A5: Direct Response Without JSON

grep -rn "text:.*\`" --include="*.ts" src/ | grep -v "JSON\.stringify"

Error Handling Violations (E1-E5)

E1: Empty Catch Blocks

grep -rn "catch.*{" --include="*.ts" --include="*.py" src/ -A 2 | grep -E "^\s*}\s*$"

E2: Swallowed Errors (catch without rethrow or return)

grep -rn "catch" --include="*.ts" src/ -A 5 | grep -v "throw\|return\|logger\|console\.error"

E3: Missing Error Context

grep -rn "throw new Error\|raise.*Error" --include="*.ts" --include="*.py" src/ | grep -v ":\|context\|failed"

E4: Generic Error Messages

grep -rn "\"error\"\|\"Error\"\|\"failed\"" --include="*.ts" src/ | grep -v "suggestion\|field\|message"

E5: Unhandled Promise Rejections

grep -rn "\.then(" --include="*.ts" src/ | grep -v "\.catch\|await"

Maintainability Violations (M1-M5)

M1: Magic Numbers/Strings

grep -rn "[0-9]\{4,\}\|timeout.*[0-9]" --include="*.ts" --include="*.py" src/ | grep -v "const\|MAX_\|MIN_\|DEFAULT_"

M2: Missing Type Annotations (Python)

grep -rn "def.*(" --include="*.py" src/ | grep -v "->.*:"

M3: TODO/FIXME in Production Code

grep -rn "TODO\|FIXME\|XXX\|HACK" --include="*.ts" --include="*.py" src/

M4: Duplicate Tool Definitions

grep -rn "name:\s*['\"]" --include="*.ts" src/ | cut -d: -f3 | sort | uniq -d

M5: Inconsistent Error Response Format

grep -rn "isError.*true" --include="*.ts" src/ -B 5 | grep "text:" | grep -v "success\|errors"

---

Manual Review Checklist

Security Review

• S1: No hardcoded API keys, passwords, or secrets
• S2: All logging goes to stderr (not stdout)
• S3: Input length limited before regex processing
• S4: No eval(), exec(), or dynamic code execution
• S5: Parameterized queries for any database access
• S6: Environment variables used for configuration
• S7: Sensitive data not logged (passwords, tokens, PII)

Architecture Review

• A1: All tools have clear description fields
• A2: All inputSchema properties have description
• A3: Input validation before any processing
• A4: Error responses include isError: true
• A5: All responses are JSON-serialized
• A6: Resources use appropriate URI schemes
• A7: Prompts have clear argument descriptions

Error Handling Review

• E1: No empty catch blocks
• E2: Errors logged or re-thrown, never swallowed
• E3: Error messages include context (what failed, why)
• E4: Error responses include suggestion field
• E5: Async operations properly awaited/handled
• E6: Graceful degradation for external dependencies

Production Readiness Review

• P1: Structured logging with JSON format
• P2: Health check endpoint or mechanism
• P3: Configurable timeouts for external calls
• P4: Graceful shutdown handling
• P5: No debug/development code in production
• P6: Dependencies pinned to specific versions
• P7: Dockerfile or deployment configuration present

---

Common Violations with Fixes

S2: Logging to stdout (CRITICAL)

Problem: stdout is reserved for JSON-RPC messages. Logging there corrupts the transport.

// ❌ BAD: Breaks MCP stdio transport
console.log("Processing request:", args);

// ✅ GOOD: Log to stderr
console.error(
  JSON.stringify({
    timestamp: new Date().toISOString(),
    level: "INFO",
    message: "Processing request",
    context: { args_keys: Object.keys(args) },
  }),
);

S3: Missing ReDoS Protection (HIGH)

Problem: Unbounded input to regex can cause catastrophic backtracking.

// ❌ BAD: No input length limit
const matches = userInput.match(/complex.*pattern/);

// ✅ GOOD: Limit input before regex
const MAX_INPUT_LENGTH = 50_000;
const safeInput = (text: string) =>
  text.length > MAX_INPUT_LENGTH ? text.slice(0, MAX_INPUT_LENGTH) : text;

const matches = safeInput(userInput).match(/complex.*pattern/);

A1: Missing Tool Description (MEDIUM)

Problem: LLM can't understand when to use the tool.

// ❌ BAD: No description
{
  name: "process_data",
  inputSchema: { type: "object", properties: { data: { type: "string" } } }
}

// ✅ GOOD: Clear description explaining purpose and return value
{
  name: "process_data",
  description: "Validates and transforms input data according to specified format. Returns structured result with validation errors if any.",
  inputSchema: {
    type: "object",
    properties: {
      data: {
        type: "string",
        description: "Raw data string to process"
      },
      format: {
        type: "string",
        enum: ["json", "csv", "xml"],
        description: "Expected input format for validation"
      }
    },
    required: ["data", "format"]
  }
}

A4: Missing isError Flag (HIGH)

Problem: Client can't distinguish errors from successful responses.

// ❌ BAD: Error without isError flag
return {
  content: [{ type: "text", text: JSON.stringify({ error: "Not found" }) }],
};

// ✅ GOOD: Proper error response
return {
  content: [
    {
      type: "text",
      text: JSON.stringify({
        success: false,
        errors: [
          {
            field: "id",
            message: "Resource not found",
            suggestion: "Verify the ID exists and try again",
          },
        ],
      }),
    },
  ],
  isError: true,
};

E1: Empty Catch Block (HIGH)

Problem: Errors are silently swallowed, making debugging impossible.

// ❌ BAD: Silent failure
try {
  await riskyOperation();
} catch (error) {
  // do nothing
}

// ✅ GOOD: Log and handle appropriately
try {
  await riskyOperation();
} catch (error) {
  logger.error("Risky operation failed", { error: error.message });
  return createErrorResponse([
    {
      field: "operation",
      message: "Operation failed",
      suggestion: "Check input parameters and retry",
    },
  ]);
}

E4: Generic Error Messages (MEDIUM)

Problem: Users can't understand what went wrong or how to fix it.

// ❌ BAD: Unhelpful error
return { error: "Something went wrong" };

// ✅ GOOD: Specific, actionable error
return {
  success: false,
  errors: [
    {
      field: "partner_id",
      message: "Invalid UUID format: 'abc123'",
      suggestion: "Use format: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx",
    },
  ],
};

---

Review Report Template

Use this template when reporting review findings:

# MCP Server Review: [Server Name]

**Reviewer:** [Name]
**Date:** [Date]
**Files Reviewed:** [List of files]

## Summary

| Severity | Count |
| -------- | ----- |
| CRITICAL | X     |
| HIGH     | X     |
| MEDIUM   | X     |
| LOW      | X     |

**Recommendation:** [APPROVE / APPROVE WITH CHANGES / REQUEST CHANGES / BLOCK]

## Critical Issues

### [S1] Hardcoded API Key

- **File:** src/config.ts:42
- **Code:** `const API_KEY = "sk_live_..."`
- **Fix:** Move to environment variable

## High Issues

### [A4] Missing isError Flag

- **File:** src/tools/processor.ts:128
- **Code:** Error response without isError
- **Fix:** Add `isError: true` to error responses

## Medium Issues

### [A1] Missing Tool Description

- **File:** src/index.ts:56
- **Tool:** `validate_input`
- **Fix:** Add description explaining tool purpose

## Low Issues

### [M3] TODO Comment

- **File:** src/utils/parser.ts:89
- **Comment:** `// TODO: optimize this`
- **Suggestion:** Create ticket or remove

## Checklist Results

- [x] Security: 6/7 passed
- [x] Architecture: 5/7 passed
- [ ] Error Handling: 4/6 passed
- [x] Production: 7/7 passed

---

Workflow

Step 1: Run Automated Detection

# Run all security checks
grep -rn "console\.log\|api[_-]\?key\s*[:=]" --include="*.ts" src/

# Run all architecture checks
grep -rn "name:\s*['\"]" --include="*.ts" src/ -A 5 | grep -v description

Step 2: Manual Checklist Review

Walk through each section of the manual review checklist, marking items as pass/fail.

Step 3: Code Walkthrough

1. Read each tool definition
2. Trace request handling flow
3. Verify error paths
4. Check resource and prompt definitions

Step 4: Generate Report

Use the review report template to document findings with specific file locations and code snippets.

Step 5: Classify and Prioritize

1. Group findings by severity
2. Identify blocking issues (CRITICAL/HIGH)
3. Suggest fixes for each issue

---

Related Skills

• mcp-server-writing - Create new MCP servers
• security-scan - General security scanning
• quality-check - Code quality checks

Resources

• MCP Best Practices
• MCP FAQs
• See REFERENCE.md for complete violation catalog
• See CHECKLISTS/ for printable audit checklists