Skip to content

fix: address 5 critical security audit findings (#88)#94

Open
JosephDoUrden wants to merge 1 commit intoKeygraphHQ:mainfrom
JosephDoUrden:fix/security-audit-88-critical
Open

fix: address 5 critical security audit findings (#88)#94
JosephDoUrden wants to merge 1 commit intoKeygraphHQ:mainfrom
JosephDoUrden:fix/security-audit-88-critical

Conversation

@JosephDoUrden
Copy link

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

…Q#88)

- Validate parsed tool call JSON structure before property access (command injection)
- Add path traversal protection in saveDeliverableFile with resolve+prefix check
- Enforce minimum 32-char TOTP secret length per RFC 4226
- Add redactSensitive() helper to prevent secret exposure in error logs
- Add forbidden key detection to block prototype pollution via YAML configs

Includes vitest setup and 38 tests covering all 5 fixes.
}

// Keys whose values should be redacted in log output
const SENSITIVE_KEY_PATTERN = /^(password|pass|passwd|secret|token|apikey|api_key|authorization|cookie|session|sessionid|session_id|privatekey|private_key|credentials|totp_secret|access_token|refresh_token)$/i;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be expanded but good start.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security Audit: 18 findings (5 Critical, 7 High) - Argus Security Report

3 participants