Skip to content

Commit d44aa9d

Browse files
docs: update audit report (0 critical/high/medium) + add lessons learned
Audit report: all 3 mediums resolved (shared types, session template, integration tests). 79 tests across 8 files. 0/0/0/8 remaining. Audit prompt: added Lessons Learned section with patterns discovered across 3 audit rounds — MCP pitfalls, code quality patterns, testing gaps, security items easy to miss, and audit process improvements. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 1b77fbc commit d44aa9d

2 files changed

Lines changed: 60 additions & 26 deletions

File tree

_docs/AUDIT_PROMPT.md

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -205,12 +205,53 @@ End with:
205205

206206
---
207207

208+
## Lessons Learned (from v1.0.0 → v2.0.1 audit cycle)
209+
210+
These patterns were discovered during three rounds of auditing this project. Include them in every audit pass.
211+
212+
### Common MCP Server Pitfalls
213+
- **`server.tool()` is deprecated** — always verify tools use `server.registerTool()` with the config object pattern
214+
- **Missing `isError: true`** — tool failure responses MUST include `isError: true` per MCP spec, otherwise clients can't distinguish failures from success
215+
- **Annotations default to worst-case** — without `readOnlyHint`, every tool is treated as potentially destructive. Read-only tools (list, get, browse) need explicit `readOnlyHint: true`
216+
- **`openWorldHint`** — tools that reach Docker registries, Git servers, or external URLs should be `openWorldHint: true`
217+
- **Prompts referencing nonexistent tools** — always cross-check every tool name in prompts/skill against actual `registerTool()` calls
218+
- **SSL bypass with native fetch** — Node.js native `fetch()` ignores `https.Agent`. Use `NODE_TLS_REJECT_UNAUTHORIZED` or undici's `dispatcher` option
219+
220+
### Common Code Quality Patterns
221+
- **Inline interfaces drift** — tool files that define their own response interfaces will drift from the actual API. Use shared types
222+
- **Magic numbers** — size conversions (1e6, 1e9, 1073741824) should use utility functions, not inline math. Watch for SI vs binary unit inconsistency
223+
- **`toolHandler()` wrapper** — eliminates try-catch boilerplate and centralizes error formatting. Any raw try-catch in a tool handler is a red flag
224+
- **Version string duplication** — read version from package.json at runtime instead of hardcoding in multiple files
225+
- **Per-session overhead** — MCP servers with 100+ tools should share tool registrations across HTTP sessions, not re-register per connection
226+
227+
### Testing Gaps to Check
228+
- **Tests exist but never run** — vitest config with thresholds means nothing if no test files exist and CI doesn't run tests
229+
- **Tool handler tests need isError assertion** — if toolHandler was updated, the error test must verify `isError: true`
230+
- **Integration tests need mocked client** — mock `getArcaneClient()` to return a fake with `get`/`post`/`delete` stubs
231+
- **Prompt tests should verify tool names** — grep for `arcane_` in prompt content and cross-check against registered tools
232+
233+
### Security Items Easy to Miss
234+
- **`.env` committed to git** — always check `git ls-files` for secrets, not just `.gitignore`
235+
- **Config file permissions**`~/.arcane/config.json` with API keys should be 600, not world-readable
236+
- **Health endpoint metadata** — session counts, server internals should not be exposed without auth
237+
- **Path traversal on browse endpoints** — any tool accepting a `path` parameter for file operations needs `..` validation
238+
- **Rate limiting** — HTTP transport without rate limiting enables denial-of-service
239+
240+
### Audit Process Improvements
241+
- **Run the audit in phases** — fix critical/high first, re-audit, then medium, then low. Don't try to fix everything in one pass
242+
- **Use parallel agents in worktrees** — independent fixes (different files) can run simultaneously without merge conflicts
243+
- **Cross-check tool counts**`grep -c "registerTool(" src/tools/*.ts` should match documented counts
244+
- **Verify CI actually runs** — a green CI that only builds and doesn't test gives false confidence
245+
- **Check npm publish** — the published package may be stale if `npm publish` wasn't run after fixes
246+
247+
---
248+
208249
## Metadata
209250

210-
- **Target:** Arcane MCP Server v2.0.0
251+
- **Target:** Arcane MCP Server v2.0.1+
211252
- **Repo:** github.com/RandomSynergy17/Arcane-MCP-Server
212253
- **npm:** @randomsynergy/arcane-mcp-server
213-
- **Prior audit:** v1.0.0 (100+ issues, 19 critical)
254+
- **Prior audits:** v1.0.0 (100+ issues, 19 critical), v2.0.0 (31 issues, 2 critical), v2.0.1 (8 low remaining)
214255
- **Stack:** TypeScript, Node.js 18+, @modelcontextprotocol/sdk, Express, Zod
215256
- **References:**
216257
- [MCP Specification 2025-11-25](https://modelcontextprotocol.io/specification/2025-11-25)

_docs/AUDIT_REPORT_v2.md

Lines changed: 17 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
# Arcane MCP Server — Audit Report
22

3-
**Version:** 2.0.1
4-
**Audit Date:** April 6, 2026 (updated April 7, 2026)
3+
**Version:** 2.0.1+
4+
**Audit Date:** April 6, 2026 (final update April 7, 2026)
55
**Prior Audit:** v1.0.0 (February 4, 2026 — 100+ issues, 19 critical)
66
**Auditor:** Claude Opus 4.6 (automated 13-category review per AUDIT_PROMPT.md)
7-
**Tool Count:** 180 (verified) | **Resources:** 2 | **Prompts:** 4 | **Tests:** 50
7+
**Tool Count:** 180 (verified) | **Resources:** 2 | **Prompts:** 4 | **Tests:** 79
88

99
---
1010

@@ -19,17 +19,17 @@ v2.0.1 resolves all critical and high issues from prior audits. The codebase has
1919
| Security | 0 | 0 | 0 | 0 |
2020
| Code Quality | 0 | 0 | 0 | 2 |
2121
| Error Handling | 0 | 0 | 0 | 1 |
22-
| TypeScript | 0 | 0 | 1 | 1 |
22+
| TypeScript | 0 | 0 | 0 | 1 |
2323
| MCP Protocol | 0 | 0 | 0 | 0 |
2424
| API Design | 0 | 0 | 0 | 1 |
25-
| Testing | 0 | 0 | 1 | 0 |
26-
| Performance | 0 | 0 | 1 | 0 |
25+
| Testing | 0 | 0 | 0 | 0 |
26+
| Performance | 0 | 0 | 0 | 0 |
2727
| Dependencies | 0 | 0 | 0 | 2 |
2828
| Skill Quality | 0 | 0 | 0 | 0 |
2929
| Plugin Format | 0 | 0 | 0 | 1 |
3030
| Publishing | 0 | 0 | 0 | 0 |
3131
| Cross-Platform | 0 | 0 | 0 | 0 |
32-
| **TOTALS** | **0** | **0** | **3** | **8** |
32+
| **TOTALS** | **0** | **0** | **0** | **8** |
3333

3434
---
3535

@@ -58,27 +58,16 @@ All critical and high issues from the v2.0.0 audit have been resolved:
5858
| API-01 | `tag: "latest"` default | FIXED — tag now required |
5959
| API-02 | Pagination defaults hardcoded | FIXED �� container-tools uses constants (proof of concept) |
6060
| CQ-04 | Version duplicated in 5 places | FIXED — server.ts reads from package.json at runtime |
61-
| TEST-01 | Zero test files | FIXED — 50 tests across 4 files |
61+
| TEST-01 | Zero test files | FIXED — 79 tests across 8 files |
6262
| TEST-02 | CI missing test/audit | FIXED — `npm test` + `npm audit` in pipeline |
63+
| TS-01 | Duplicated interfaces in tool files | FIXED — 33 interfaces in `src/types/arcane-types.ts` |
64+
| PERF-01 | McpServer created per HTTP session | FIXED — template pattern shares registrations |
65+
| TEST-03 | Coverage below 60% | FIXED — 29 integration tests added (79 total) |
6366

6467
---
6568

6669
## Remaining Issues (Medium + Low)
6770

68-
### [MEDIUM] TS-01: Interface definitions duplicated across tool files
69-
- **File:** All 25 tool files define local interfaces
70-
- **Description:** Each tool file has its own `Container`, `Volume`, etc. interfaces instead of importing from `src/types/generated/arcane-api.ts`. Drift risk.
71-
- **Recommendation:** Refactor to shared types in a future release. Low urgency since interfaces are simple and the generated types have complex nested generics.
72-
73-
### [MEDIUM] PERF-01: New McpServer created per HTTP session
74-
- **File:** `src/tcp-server.ts`
75-
- **Description:** Each session creates a fresh McpServer + registers 180 tools. With 100 max sessions, this is non-trivial memory use.
76-
- **Recommendation:** Profile actual memory usage under load before optimizing. The per-session isolation is a security benefit.
77-
78-
### [MEDIUM] TEST-03: Test coverage below 60% threshold
79-
- **Description:** 50 tests cover utilities and config, but no tool modules or resources/prompts are tested.
80-
- **Recommendation:** Add integration tests for 2-3 tool modules with mocked HTTP client.
81-
8271
### [LOW] CQ-03: Logger import boilerplate in 25 tool files
8372
- **Description:** All tool files import logger for a single `logger.debug("Registered X tools")` call.
8473

@@ -140,11 +129,15 @@ All critical and high issues from the v2.0.0 audit have been resolved:
140129
- No path separator issues
141130
- Portable plugin paths
142131

143-
### Testing: 50 tests, 4 files
132+
### Testing: 79 tests, 8 files
144133
- tool-helpers (success/error/isError/params)
145134
- format (formatSize, formatSizeCompact, formatSizeMB, formatSizeGB, validatePath)
146135
- error-handler (all error classes, formatError dispatch)
147136
- config (defaults, env overrides, caching, validation)
137+
- container-tools (list, get, delete, error handling)
138+
- dashboard-tools (snapshot, action items, errors)
139+
- resources (environments, version, errors)
140+
- prompts (all 4 prompts, tool references, message structure)
148141

149142
---
150143

@@ -163,7 +156,7 @@ All critical and high issues from the v2.0.0 audit have been resolved:
163156
- **Auditor:** Claude Opus 4.6 (1M context)
164157
- **Method:** Full 13-category review per AUDIT_PROMPT.md
165158
- **Build:** Clean (zero errors, zero warnings)
166-
- **Tests:** 50 passing across 4 files
159+
- **Tests:** 79 passing across 8 files
167160
- **npm Audit:** 5 moderate vulnerabilities (all dev-only vitest chain)
168161
- **npm Pack:** ~141 KB compressed
169162
- **Tool Cross-Check:** 27 prompt refs + 44 skill refs verified against 180 registrations

0 commit comments

Comments
 (0)