[PR #94] fix: address 5 critical security audit findings (#88) #115

New issue

Open

opened 2026-02-27 07:20:29 +03:00 by kerem · 0 comments

kerem commented

2026-02-27 07:20:29 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/KeygraphHQ/shannon/pull/94
Author: @JosephDoUrden
Created: 2/8/2026
Status: 🔄 Open

Base: main ← Head: fix/security-audit-88-critical

📝 Commits (1)

05ba8a5 fix: address 5 critical findings from Argus security audit (#88)

📊 Changes

18 files changed (+3623 additions, -268 deletions)

View changed files

📝 mcp-server/package-lock.json (+1588 -133)
📝 mcp-server/package.json (+5 -2)
📝 mcp-server/src/tools/generate-totp.ts (+3 -2)
➕ mcp-server/src/utils/file-operations.test.ts (+59 -0)
📝 mcp-server/src/utils/file-operations.ts (+16 -3)
➕ mcp-server/src/validation/totp-validator.test.ts (+35 -0)
📝 mcp-server/src/validation/totp-validator.ts (+18 -2)
➕ mcp-server/vitest.config.ts (+7 -0)
📝 package-lock.json (+1538 -119)
📝 package.json (+4 -1)
📝 src/ai/message-handlers.ts (+12 -2)
➕ src/config-parser.test.ts (+86 -0)
📝 src/config-parser.ts (+29 -1)
➕ src/error-handling.test.ts (+82 -0)
📝 src/error-handling.ts (+31 -1)
➕ src/utils/output-formatter.test.ts (+66 -0)
📝 src/utils/output-formatter.ts (+37 -2)
➕ vitest.config.ts (+7 -0)

📄 Description

Summary

This PR addresses the 5 Critical findings from the Argus Security Audit report:

Critical 1 (Command Injection): Added strict tool call JSON shape validation (isValidToolCall) and defensive validation in handleToolUseMessage, plus sanitizeForDisplay to prevent control-character/log injection.
Critical 2 (Path Traversal): Hardened saveDeliverableFile with filename validation and a resolved-path prefix check to ensure writes stay within the deliverables directory.
Critical 3 (TOTP Secret Validation): Enforced a minimum TOTP secret length of 32 base32 chars per RFC 4226 guidance, aligning validator + schema.
Critical 4 (Secret Exposure): Added redactSensitive() and applied it to error.context logging to prevent leaking tokens/passwords/keys.
Critical 5 (Prototype Pollution): Added a recursive forbidden-key scan (__proto__, constructor, prototype) and enabled safer YAML parsing options before any downstream use.

Files changed

src/utils/output-formatter.ts
src/ai/message-handlers.ts
mcp-server/src/utils/file-operations.ts
mcp-server/src/validation/totp-validator.ts
mcp-server/src/tools/generate-totp.ts
src/error-handling.ts
src/config-parser.ts
package.json
mcp-server/package.json
vitest.config.ts
mcp-server/vitest.config.ts
src/utils/output-formatter.test.ts
src/error-handling.test.ts
src/config-parser.test.ts
mcp-server/src/validation/totp-validator.test.ts
mcp-server/src/utils/file-operations.test.ts

Test plan

Added 38 vitest tests covering all 5 critical fixes
npm test passes in both root and mcp-server/
TypeScript build/compile succeeds for both packages
Optional: run end-to-end scan / real workflow to confirm behavior under production inputs

Closes #88

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/KeygraphHQ/shannon/pull/94 **Author:** [@JosephDoUrden](https://github.com/JosephDoUrden) **Created:** 2/8/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `fix/security-audit-88-critical` --- ### 📝 Commits (1) - [`05ba8a5`](https://github.com/KeygraphHQ/shannon/commit/05ba8a57a97fdacaed9b8b49ecc3cc7baa82c9bf) fix: address 5 critical findings from Argus security audit (#88) ### 📊 Changes **18 files changed** (+3623 additions, -268 deletions) <details> <summary>View changed files</summary> 📝 `mcp-server/package-lock.json` (+1588 -133) 📝 `mcp-server/package.json` (+5 -2) 📝 `mcp-server/src/tools/generate-totp.ts` (+3 -2) ➕ `mcp-server/src/utils/file-operations.test.ts` (+59 -0) 📝 `mcp-server/src/utils/file-operations.ts` (+16 -3) ➕ `mcp-server/src/validation/totp-validator.test.ts` (+35 -0) 📝 `mcp-server/src/validation/totp-validator.ts` (+18 -2) ➕ `mcp-server/vitest.config.ts` (+7 -0) 📝 `package-lock.json` (+1538 -119) 📝 `package.json` (+4 -1) 📝 `src/ai/message-handlers.ts` (+12 -2) ➕ `src/config-parser.test.ts` (+86 -0) 📝 `src/config-parser.ts` (+29 -1) ➕ `src/error-handling.test.ts` (+82 -0) 📝 `src/error-handling.ts` (+31 -1) ➕ `src/utils/output-formatter.test.ts` (+66 -0) 📝 `src/utils/output-formatter.ts` (+37 -2) ➕ `vitest.config.ts` (+7 -0) </details> ### 📄 Description ## Summary This PR addresses the **5 Critical** findings from the Argus Security Audit report: - **Critical 1 (Command Injection):** Added strict tool call JSON shape validation (`isValidToolCall`) and defensive validation in `handleToolUseMessage`, plus `sanitizeForDisplay` to prevent control-character/log injection. - **Critical 2 (Path Traversal):** Hardened `saveDeliverableFile` with filename validation and a resolved-path prefix check to ensure writes stay within the deliverables directory. - **Critical 3 (TOTP Secret Validation):** Enforced a minimum TOTP secret length of **32 base32 chars** per RFC 4226 guidance, aligning validator + schema. - **Critical 4 (Secret Exposure):** Added `redactSensitive()` and applied it to `error.context` logging to prevent leaking tokens/passwords/keys. - **Critical 5 (Prototype Pollution):** Added a recursive forbidden-key scan (`__proto__`, `constructor`, `prototype`) and enabled safer YAML parsing options before any downstream use. ## Files changed - src/utils/output-formatter.ts - src/ai/message-handlers.ts - mcp-server/src/utils/file-operations.ts - mcp-server/src/validation/totp-validator.ts - mcp-server/src/tools/generate-totp.ts - src/error-handling.ts - src/config-parser.ts - package.json - mcp-server/package.json - vitest.config.ts - mcp-server/vitest.config.ts - src/utils/output-formatter.test.ts - src/error-handling.test.ts - src/config-parser.test.ts - mcp-server/src/validation/totp-validator.test.ts - mcp-server/src/utils/file-operations.test.ts ## Test plan - [x] Added **38** vitest tests covering all 5 critical fixes - [x] `npm test` passes in both root and `mcp-server/` - [x] TypeScript build/compile succeeds for both packages - [ ] Optional: run end-to-end scan / real workflow to confirm behavior under production inputs Closes #88 --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>