docs(typescript): add MCP security documentation and examples

[jackbatzner]

Description

Adds comprehensive documentation for TypeScript MCP security primitives, including API reference docs, OWASP MCP Cheat Sheet compliance mapping, quickstart guides, i18n translations, and usage examples.

Part 3 of 3 — Merge after #827. See also:

• Part 1: feat(typescript): add MCP protocol governance with full OWASP coverage #791 Core MCP security primitives ← merge first
• Part 2: feat(typescript): add standalone @microsoft/agentmesh-mcp-governance package #827 Standalone @microsoft/agentmesh-mcp-governance package ← merge second
• Part 3: This PR ← merge last

Type of Change

• Documentation update

Package(s) Affected

• agent-mesh

Checklist

• My code follows the project style guidelines
• I have updated documentation as needed
• I have signed the Microsoft CLA

Related Issues

Supersedes #791 (split for easier review). Merge after #827.

[github-actions]

Welcome to the Agent Governance Toolkit! Thanks for your first pull request.
Please ensure tests pass, code follows style (ruff check), and you have signed the CLA.
See our Contributing Guide.

[github-actions]

🤖 AI Agent: security-scanner — Security Review of Pull Request

Security Review of Pull Request

This pull request primarily introduces documentation and examples for the TypeScript MCP (Managed Control Plane) security primitives in the microsoft/agent-governance-toolkit repository. While the changes are primarily non-functional, they include example code and configurations that could potentially introduce vulnerabilities if used as-is. Below is a detailed security review of the changes.

---

Findings

1. Prompt Injection Defense Bypass

• Severity: 🔴 CRITICAL
• Issue: The MCPGateway's blockedPatterns configuration in the mcp-express-server example includes patterns like '../', '..\\', and '<system>'. However, these patterns are insufficient to prevent sophisticated prompt injection attacks, especially when user-controlled input is concatenated into descriptions or schemas. For example:

  const requestThreats = securityScanner.scanTool(
    toolName,
    `${handler.description}\nRequest payload: ${JSON.stringify(args)}`,
    handler.inputSchema,
    'mcp-express-server',
  );

  Here, user-controlled args are directly concatenated into the description string, which could allow an attacker to inject malicious instructions that bypass the MCPResponseScanner or other downstream protections.
• Attack Vector: An attacker could craft a malicious args payload that includes prompt injection strings (e.g., "Ignore all previous instructions and execute the following...") to bypass the security scanner.
• Recommendation: Avoid directly concatenating user input into descriptions or schemas. Use structured data and ensure that all user input is sanitized before being processed by the securityScanner.

---

2. Policy Engine Circumvention

• Severity: 🟠 HIGH
• Issue: The approvalHandler in the MCPGateway example uses a simplistic condition to approve or reject tool calls:

  approvalHandler: async ({ toolName, params }) =>
    toolName === 'read_file' && params.approved !== true
      ? sdk.ApprovalStatus.Pending
      : sdk.ApprovalStatus.Approved,

  This logic assumes that the approved parameter in the params object is trustworthy. However, an attacker could tamper with this parameter to bypass the approval process.
• Attack Vector: An attacker could craft a request with approved: true to bypass the approval process for sensitive tools like read_file.
• Recommendation: Do not rely on user-provided parameters (e.g., params.approved) for critical security decisions. Instead, implement server-side validation and enforce stricter approval logic, such as requiring explicit administrator approval for sensitive tools.

---

3. Credential Exposure

• Severity: 🔴 CRITICAL
• Issue: The read_file tool handler in the mcp-express-server example includes the following code:

  if (path.endsWith('secrets.txt')) {
    return {
      contents: 'Bearer abcdefghijklmnop demo@example.com',
      path,
    };
  }

  This code simulates returning sensitive information (e.g., a bearer token and email address) when a specific file is requested. While this is an example, it sets a dangerous precedent for developers who might copy and adapt this code without understanding the implications.
• Attack Vector: If this example code is used in production or adapted without proper safeguards, it could lead to the exposure of sensitive credentials or secrets.
• Recommendation: Remove or heavily annotate this example to clarify that it is for demonstration purposes only and should not be used in production. Additionally, include a warning about the risks of exposing sensitive information.

---

4. Sandbox Escape

• Severity: 🟠 HIGH
• Issue: The read_file tool handler does not adequately validate the path parameter, allowing potential directory traversal attacks:

  if (path.endsWith('secrets.txt')) {
    return {
      contents: 'Bearer abcdefghijklmnop demo@example.com',
      path,
    };
  }

  While the MCPGateway includes a blockedPatterns configuration (e.g., '../'), this is insufficient to fully prevent directory traversal or sandbox escape attacks.
• Attack Vector: An attacker could craft a path parameter that bypasses the blockedPatterns check (e.g., using encoded or obfuscated traversal sequences) to access sensitive files outside the intended directory.
• Recommendation: Implement robust path sanitization and validation using a library like path.normalize or path.resolve to ensure that the resolved path is within the allowed directory.

---

5. Deserialization Attacks

• Severity: 🟡 MEDIUM
• Issue: The args parameter in the call-tool endpoint is deserialized without strict validation:

  const args = asRecord(request.body?.args);

  While the MCPGateway and MCPSecurityScanner provide some level of validation, they may not fully protect against maliciously crafted payloads.
• Attack Vector: An attacker could craft a malicious args payload that exploits weaknesses in the deserialization logic to execute arbitrary code or cause denial-of-service (DoS).
• Recommendation: Use a schema validation library (e.g., Ajv) to strictly validate the structure and content of the args parameter before processing it.

---

6. Supply Chain Risks

• Severity: 🔵 LOW
• Issue: The package.json file for the mcp-express-server example includes dependencies like express and tsx without explicit version pinning:

  "dependencies": {
    "express": "4.21.2"
  },
  "devDependencies": {
    "tsx": "4.19.3"
  }

  While these dependencies are widely used and generally considered safe, the lack of explicit version pinning increases the risk of supply chain attacks (e.g., dependency hijacking or typosquatting).
• Attack Vector: An attacker could compromise a dependency or publish a malicious version to exploit users of this example.
• Recommendation: Pin all dependencies to specific versions and use a tool like npm audit or Snyk to monitor for vulnerabilities.

---

Summary of Findings

Finding	Severity	Recommendation
Prompt injection defense bypass	🔴 CRITICAL	Avoid concatenating user input into descriptions or schemas; sanitize input before processing.
Policy engine circumvention	🟠 HIGH	Avoid relying on user-provided parameters for critical security decisions; implement server-side checks.
Credential exposure	🔴 CRITICAL	Remove or annotate example code that simulates sensitive data exposure.
Sandbox escape	🟠 HIGH	Implement robust path sanitization and validation to prevent directory traversal attacks.
Deserialization attacks	🟡 MEDIUM	Use schema validation libraries to strictly validate deserialized input.
Supply chain risks	🔵 LOW	Pin dependencies to specific versions and monitor for vulnerabilities.

---

Overall Assessment

While this PR is primarily focused on documentation and examples, it introduces several critical and high-severity risks due to insecure example code and insufficient safeguards in the governance pipeline. These issues could lead to prompt injection attacks, policy circumvention, credential exposure, and sandbox escapes if the examples are used as-is.

Recommended Actions

1. Update the documentation and examples to address the identified vulnerabilities.
2. Add warnings to the documentation and example code to highlight potential risks and best practices.
3. Conduct a thorough review of the MCPGateway, MCPSecurityScanner, and related components to ensure robust security against prompt injection and other attacks.
4. Implement additional test cases to validate the security of the governance pipeline against common attack vectors.

This PR should not be merged until the critical and high-severity issues are addressed.

[github-actions]

🤖 AI Agent: code-reviewer

Review Summary

This pull request introduces comprehensive documentation and examples for the TypeScript MCP (Managed Control Plane) security primitives, along with the addition of a new standalone package @microsoft/agentmesh-mcp-governance. The package includes several security-focused utilities such as credential redaction, rate limiting, HMAC signing, and policy enforcement.

The changes are well-structured and adhere to the project's style guidelines. However, there are some critical security concerns, potential breaking changes, and areas for improvement that need to be addressed.

---

🔴 CRITICAL: Security Issues

1. Regex Denial-of-Service (ReDoS) Risk in Credential Redactor

   • The CredentialRedactor uses several regex patterns (e.g., SENSITIVE_KEY_PATTERN, BUILTIN_PATTERNS) to identify sensitive information. Some of these patterns (e.g., SENSITIVE_KEY_PATTERN) are vulnerable to catastrophic backtracking, which could lead to a denial-of-service (DoS) attack if an attacker crafts malicious input.
   • Action: Use regex libraries like safe-regex to validate patterns or refactor the regex to avoid catastrophic backtracking.

2. Fail-Closed Behavior in Gateway

   • While the McpGateway attempts to fail-closed on errors (e.g., getSafeErrorMessage), there are scenarios where exceptions in finalize or safeRedactParams could result in incomplete auditing or incorrect decisions.
   • Action: Add more robust error handling and logging to ensure that all failures are properly captured and do not affect the security posture.

3. Credential Redaction in Nested Objects

   • The CredentialRedactor does not handle deeply nested objects efficiently, which could lead to partial redaction or missed sensitive data.
   • Action: Add a depth limit or recursion guard to prevent excessive processing and ensure complete redaction.

4. Blocked Patterns in Gateway

   • The checkSanitization method in McpGateway uses both string matching and regex patterns to identify blocked patterns. However, the use of includes for string matching might miss edge cases where the blocked pattern is part of a larger string.
   • Action: Normalize and tokenize input data before applying string or regex matching to ensure comprehensive sanitization.

---

🟡 WARNING: Potential Breaking Changes

1. Public API Instability

   • The @microsoft/agentmesh-mcp-governance package is marked as a "Public Preview," and the documentation explicitly states that APIs may change before GA. This could lead to breaking changes for early adopters.
   • Action: Clearly communicate API stability expectations and provide migration guides for future changes.

2. Default Configuration Assumptions

   • The McpGateway and CredentialRedactor rely on default configurations (e.g., BUILTIN_PATTERNS, DEFAULT_REPLACEMENT). Changes to these defaults in future releases could break existing integrations.
   • Action: Document default configurations explicitly and consider versioning them to avoid breaking changes.

---

💡 Suggestions for Improvement

1. Test Coverage

   • While the jest.config.js file indicates that tests are included, the diff does not show any test files. Ensure that all critical components, especially CredentialRedactor and McpGateway, have comprehensive unit tests.
   • Action: Add tests for edge cases, such as deeply nested objects, malformed inputs, and timeouts in regex scanning.

2. Performance Optimization

   • The CredentialRedactor and McpGateway rely heavily on regex operations, which can be computationally expensive. For high-throughput scenarios, this could become a bottleneck.
   • Action: Benchmark the performance of regex operations and consider optimizing or replacing them with more efficient algorithms if necessary.

3. Documentation Enhancements

   • The documentation is comprehensive but could benefit from additional details on:
     • How to implement durable storage for production deployments.
     • Examples of custom policyEvaluator and approvalHandler implementations.
     • Security considerations for each utility (e.g., potential risks of misconfiguration).
   • Action: Expand the documentation to include these details.

4. TypeScript Types

   • The CredentialRedactor and McpGateway use several custom types (e.g., CredentialRedaction, McpGatewayDecision). Ensure that these types are well-documented and cover all edge cases.
   • Action: Add JSDoc comments to all custom types and interfaces.

5. Rate Limiter Configuration

   • The McpSlidingRateLimiter uses a sliding window algorithm, but the configuration options (e.g., maxRequests, windowMs) are not validated.
   • Action: Add validation for rate limiter configurations to prevent misconfigurations.

---

Final Assessment

• Security: 🔴 Address critical issues like ReDoS vulnerabilities and fail-closed behavior.
• Backward Compatibility: 🟡 Ensure clear communication about API stability and provide migration guides.
• Documentation: 💡 Expand documentation to cover advanced use cases and security considerations.
• Testing: 💡 Add comprehensive unit tests for all new functionality.
• Performance: 💡 Benchmark and optimize regex operations if necessary.

Once the critical issues are resolved and the suggested improvements are implemented, this PR will be ready for merging.

[github-actions]

🤖 AI Agent: code-reviewer

Pull Request Review: docs(typescript): add MCP security documentation and examples

This pull request introduces comprehensive documentation for the TypeScript MCP (Managed Control Plane) security primitives, including API references, OWASP MCP Cheat Sheet compliance mapping, quickstart guides, and usage examples. It also includes the implementation of the standalone @microsoft/agentmesh-mcp-governance package.

---

Key Areas of Review

🔴 CRITICAL: Security Issues

1. Credential Redaction Logic

   • The CredentialRedactor class uses regex patterns to identify and redact sensitive information. While the provided patterns are comprehensive, there is a risk of false negatives (missed sensitive data) due to incomplete or overly specific regex patterns.
     • Actionable Recommendation: Add a mechanism to log unredacted sensitive data for debugging purposes in a secure environment. This will help identify gaps in the regex patterns.
     • Actionable Recommendation: Regularly review and update the BUILTIN_PATTERNS to include new credential formats as they emerge.

2. Regex Denial-of-Service (ReDoS)

   • The use of unbounded regex patterns (e.g., BUILTIN_PATTERNS and SENSITIVE_KEY_PATTERN) poses a risk of ReDoS attacks if an attacker provides maliciously crafted input.
     • Actionable Recommendation: Use regex libraries that support timeout or backtracking limits (e.g., re2 in JavaScript/TypeScript) to mitigate this risk.

3. Fail-Closed Behavior

   • The McpGateway class implements fail-closed behavior in case of errors, which is a good security practice. However, the error messages returned to the user (e.g., "Internal error - access denied (fail-closed)") could potentially leak information about the system's internal state.
     • Actionable Recommendation: Ensure that error messages do not expose sensitive implementation details. Use generic error messages for external users and log detailed errors internally.

4. Approval Handler

   • The approvalHandler function in the McpGateway class is responsible for determining whether a tool call is approved. However, the default behavior is to return ApprovalStatus.Pending if no handler is provided.
     • Actionable Recommendation: Explicitly document that the absence of an approvalHandler will result in all sensitive tools requiring manual approval. Consider adding a configuration option to enforce stricter defaults.

5. Blocked Patterns

   • The checkSanitization method in McpGateway uses both user-defined and built-in patterns to block dangerous input. However, the built-in patterns may not cover all potential attack vectors.
     • Actionable Recommendation: Regularly review and expand the BUILTIN_DANGEROUS_PATTERNS to include new attack patterns. Consider allowing users to provide additional patterns via configuration.

---

🟡 WARNING: Potential Breaking Changes

1. Public API Instability

   • The @microsoft/agentmesh-mcp-governance package is marked as a "Public Preview" with a disclaimer that APIs may change before GA. This could lead to breaking changes for early adopters.
     • Actionable Recommendation: Clearly communicate the expected timeline for API stabilization and provide migration guides for any breaking changes.

2. Default Behavior Changes

   • The default behavior of the McpGateway class (e.g., requiring manual approval for sensitive tools) may not align with all use cases.
     • Actionable Recommendation: Provide clear documentation and examples to help users configure the gateway according to their specific needs.

---

💡 Suggestions for Improvement

1. Documentation Enhancements

   • The documentation is comprehensive but could benefit from additional examples demonstrating:
     • How to implement a custom approvalHandler.
     • How to extend the BUILTIN_PATTERNS with custom patterns.
     • Best practices for configuring the McpGateway in production environments.

2. Test Coverage

   • While the PR includes a Jest configuration file, there is no evidence of test cases for the newly introduced classes (CredentialRedactor, McpGateway, etc.).
     • Actionable Recommendation: Add comprehensive test cases to validate the behavior of the CredentialRedactor and McpGateway classes, especially edge cases like:
       • Inputs with no sensitive data.
       • Inputs with overlapping sensitive patterns.
       • Malicious inputs designed to exploit ReDoS vulnerabilities.

3. Concurrency and Thread Safety

   • The McpGateway class uses an in-memory rate limiter (McpSlidingRateLimiter) by default. This may not be thread-safe in multi-threaded or distributed environments.
     • Actionable Recommendation: Clearly document the limitations of the in-memory rate limiter and recommend using a distributed rate-limiting solution (e.g., Redis) for production deployments.

4. Type Safety

   • The use of unknown in the redactNode method of CredentialRedactor could lead to runtime errors if the input does not conform to the expected structure.
     • Actionable Recommendation: Use stricter TypeScript types and runtime validation (e.g., zod or io-ts) to ensure type safety.

5. Performance Considerations

   • The createRegexScanBudget function is used to prevent excessive regex execution time. However, the default scanTimeoutMs is set to 100ms, which may be too low for large inputs.
     • Actionable Recommendation: Provide guidance on how to tune scanTimeoutMs based on the expected input size and system performance.

6. Internationalization (i18n)

   • While the PR mentions i18n translations, there is no evidence of actual i18n support in the provided code or documentation.
     • Actionable Recommendation: Clarify whether i18n support is planned for a future release or if it is already implemented. If the latter, include examples in the documentation.

---

Summary of Feedback

• 🔴 CRITICAL: Address potential security issues, including ReDoS vulnerabilities and gaps in credential redaction patterns.
• 🟡 WARNING: Highlight potential breaking changes due to API instability and default behavior.
• 💡 SUGGESTION: Improve documentation, add test coverage, ensure thread safety, enhance type safety, and clarify i18n support.

By addressing these issues and suggestions, the @microsoft/agentmesh-mcp-governance package can be made more robust, secure, and user-friendly. Let me know if you need further clarification or assistance!

[github-actions]

🤖 AI Agent: code-reviewer

Code Review Feedback for Pull Request

General Observations

This pull request adds significant documentation and examples for the TypeScript MCP security primitives, which is a valuable addition to the project. However, there are several areas that require attention, particularly concerning security, correctness, and potential breaking changes.

---

🔴 CRITICAL Issues

1. Credential Handling in CredentialRedactor:

   • The CredentialRedactor class is responsible for redacting sensitive information. Ensure that the regex patterns used for redaction are comprehensive and do not allow for bypassing. For example, patterns like /(password|passwd|pwd|secret|token|api[_-]?key|connection.?string|accountkey|sharedaccesssignature|sas)/i should be reviewed to ensure they cover all potential sensitive keys. Consider adding unit tests that cover edge cases to validate the effectiveness of these patterns.

2. Error Handling in McpGateway:

   • The evaluateToolCall method catches errors but returns a generic message. This could potentially leak sensitive information if the error message is not sanitized properly. Ensure that all error messages returned to the user do not expose internal state or sensitive data.

3. Replay Protection in MCPMessageSigner:

   • The MCPMessageSigner class should implement robust replay protection mechanisms. Ensure that timestamps and nonces are validated correctly to prevent replay attacks. Review the implementation to confirm that it adheres to best practices for cryptographic operations.

---

🟡 WARNING Issues

1. Public Preview Notice:

   • The documentation states that the MCP governance package is in public preview and that APIs may change before GA. This could lead to breaking changes for users who adopt this package early. Ensure that this is communicated clearly in the documentation and consider versioning strategies to manage breaking changes effectively.

2. Potential API Changes:

   • The addition of new classes and methods (e.g., MCPGateway, CredentialRedactor) may introduce breaking changes if existing consumers rely on the previous structure. Ensure that backward compatibility is maintained or document any changes clearly in the release notes.

---

💡 SUGGESTION Improvements

1. Documentation Enhancements:

   • While the documentation is comprehensive, consider adding more examples that cover common use cases and edge cases. This will help users understand how to implement the security primitives effectively.

2. Unit Tests:

   • Ensure that there are adequate unit tests for all new classes and methods introduced in this PR. Focus on testing the security aspects, such as the effectiveness of the CredentialRedactor and the decision-making process in McpGateway.

3. Type Safety:

   • Review the use of TypeScript types throughout the new code. Ensure that all public APIs are well-typed and that any potential type mismatches are handled gracefully. This will improve the overall robustness of the library.

4. Performance Considerations:

   • The regex patterns used for redaction and sanitization could impact performance, especially with large inputs. Consider benchmarking these operations and optimizing where necessary.

5. Concurrency Handling:

   • If the McpGateway or any other classes are expected to be used in a concurrent environment, ensure that they are thread-safe. Review shared state and mutable data structures to prevent race conditions.

---

Conclusion

This pull request is a significant step forward in enhancing the documentation and usability of the MCP security primitives. However, addressing the critical security issues and potential breaking changes is paramount before merging. Implementing the suggested improvements will further strengthen the library's reliability and user experience.

[github-actions]

🤖 AI Agent: code-reviewer

Review Summary

This pull request introduces documentation and examples for the TypeScript MCP (Multi-Channel Policy) security primitives, alongside the implementation of the @microsoft/agentmesh-mcp-governance package. The package provides standalone MCP governance primitives, including credential redaction, rate limiting, message signing, and gateway enforcement. The PR also includes configuration files for testing, linting, and building the package.

The implementation appears well-structured and adheres to modern TypeScript practices. However, there are some areas of concern, particularly around security, type safety, and potential backward compatibility issues.

---

🔴 CRITICAL: Security Issues

1. Regex Denial of Service (ReDoS) Risk in Credential Redactor

   • The CredentialRedactor class uses multiple regex patterns, some of which are complex (e.g., pem_block and connection_string). These patterns could potentially lead to catastrophic backtracking, especially when processing large inputs.
   • Actionable Recommendation: Use a library like safe-regex to validate the safety of these regex patterns. Alternatively, consider optimizing the patterns or using a streaming approach to process large inputs incrementally.

2. Fail-Closed Behavior in Gateway

   • The evaluateToolCall method in McpGateway attempts to fail closed on errors, but the implementation of finalize could still allow partial failures (e.g., audit sink or metrics recording errors) to affect the decision-making process.
   • Actionable Recommendation: Ensure that all failure scenarios in finalize explicitly return a fail-closed decision, even if audit or metrics recording fails.

3. Insufficient Logging for Security Events

   • The McpGateway class lacks detailed logging for critical security events, such as when a tool call is denied due to policy violations or rate limiting.
   • Actionable Recommendation: Add structured logging for all critical security decisions, including the reason for denial and the agent/tool involved. Ensure logs are sanitized to avoid leaking sensitive information.

4. Insecure Default Configurations

   • The McpGateway class uses a NoopMcpMetrics instance as the default for metrics. This could lead to a lack of visibility into security events if the user does not explicitly configure a metrics implementation.
   • Actionable Recommendation: Either require a metrics implementation to be provided or log a warning when the default NoopMcpMetrics is used.

5. Hardcoded Sensitive Key Patterns

   • The CredentialRedactor class uses a hardcoded regex (SENSITIVE_KEY_PATTERN) to identify sensitive keys. This approach may miss custom or less common key patterns.
   • Actionable Recommendation: Allow users to configure additional sensitive key patterns via the CredentialRedactorConfig.

---

🟡 WARNING: Potential Breaking Changes

1. API Stability

   • The package is marked as "Public Preview," and the documentation explicitly states that APIs may change before GA. This could lead to breaking changes for early adopters.
   • Actionable Recommendation: Clearly document any planned changes to the API and provide migration guides for users.

2. Default Behavior Changes

   • The McpGateway class has several default behaviors (e.g., NoopMcpMetrics, empty allowedTools, etc.) that may change in the future. This could lead to unexpected behavior for users relying on the current defaults.
   • Actionable Recommendation: Document these defaults clearly and consider adding deprecation warnings if they are likely to change.

---

💡 Suggestions for Improvement

1. Type Safety

   • The safeRedactParams method in McpGateway uses a try-catch block to handle errors during redaction, but it returns an empty object in case of failure. This could lead to silent failures.
   • Suggestion: Log the error and provide a more descriptive fallback value or rethrow the error with additional context.

2. Test Coverage

   • While the PR includes a Jest configuration, there is no evidence of test cases for critical components like CredentialRedactor and McpGateway.
   • Suggestion: Add comprehensive test cases to cover edge cases, especially for security-critical features like regex patterns, rate limiting, and policy enforcement.

3. Documentation

   • The documentation is thorough but could benefit from additional examples demonstrating how to implement durable storage for session, nonce, rate-limit, and audit data.
   • Suggestion: Include a section in the README with links to example implementations or third-party libraries that can be used for durable storage.

4. Rate Limiter Configuration

   • The McpGateway class allows for a custom rate limiter, but there is no validation to ensure that the provided rate limiter adheres to the expected interface.
   • Suggestion: Use TypeScript's type system to enforce the expected interface for custom rate limiters.

5. Regex Timeout Handling

   • The createRegexScanBudget utility is used to enforce timeouts for regex scans, but the timeout behavior is not configurable at the individual pattern level.
   • Suggestion: Allow users to specify custom timeouts for individual regex patterns, especially for complex patterns.

---

Final Notes

The PR introduces a valuable addition to the agent-mesh ecosystem, focusing on security and governance. However, the identified security issues and areas for improvement should be addressed before merging to ensure robustness and reliability.

• Priority Fixes: Address the critical security issues (ReDoS risk, fail-closed behavior, and logging).
• Next Steps: Enhance test coverage and documentation, and consider the suggestions for improving type safety and configurability.

Let me know if you need further clarification or assistance!

[github-actions]

🤖 AI Agent: code-reviewer

Review Summary

This pull request introduces documentation and examples for the TypeScript MCP security primitives, as well as the implementation of the @microsoft/agentmesh-mcp-governance package. The documentation is clear and comprehensive, and the implementation appears to be well-structured. However, there are several areas that require attention to ensure security, correctness, and maintainability.

---

🔴 CRITICAL: Security Issues

1. Regex Denial of Service (ReDoS) Vulnerability

   • The BUILTIN_PATTERNS array in credential-redactor.ts contains regular expressions that are not optimized for performance. For example:

     { name: 'pem_block', pattern: /-----BEGIN [A-Z0-9 ]+-----[\s\S]*?-----END [A-Z0-9 ]+-----/g }

     The use of [\s\S]*? is prone to catastrophic backtracking, which can lead to ReDoS attacks. This needs to be optimized to avoid performance issues.

     Recommendation: Use more specific patterns or limit the quantifiers to avoid excessive backtracking. For example, use .{0,1000} instead of [\s\S]*? if you know the maximum expected length.

2. Insufficient Logging for Security Events

   • The debugSecurityFailure function is used to log security failures, but it is unclear if this function logs sufficient details for auditing purposes. For example, it is not clear if the function logs the full stack trace or other contextual information.

   • Additionally, the finalize method logs decisions but does not include the full context of the decision-making process, such as the exact parameters or patterns that triggered a denial.

     Recommendation: Ensure that all security-related events are logged with sufficient detail to facilitate auditing and debugging. Include stack traces, agent IDs, tool names, and matched patterns where applicable.

3. Fail-Closed Behavior

   • While the evaluateToolCall method attempts to fail closed in case of errors, the finalize method also has a fallback to fail closed. However, the fallback logic in finalize does not include all findings or contextual information.

     Recommendation: Ensure that the fallback logic in finalize includes all available information, such as findings and the original parameters, to avoid losing critical context during error handling.

4. Potential Credential Exposure

   • The safeRedactParams method is used to redact sensitive information from parameters, but it is not clear if it is applied consistently across all logging and auditing operations.

     Recommendation: Conduct a thorough review to ensure that safeRedactParams is applied to all logging and auditing operations. Consider adding automated tests to verify this behavior.

---

🟡 WARNING: Potential Breaking Changes

1. Public Preview APIs

   • The @microsoft/agentmesh-mcp-governance package is marked as a "Public Preview" with the note that APIs may change before GA. This could lead to breaking changes for users who adopt the package early.

     Recommendation: Clearly document the potential for breaking changes in the README and consider providing a migration guide for future updates.

---

💡 Suggestions for Improvement

1. TypeScript Type Safety

   • The evaluateToolCall method uses Record<string, unknown> for params. While this is flexible, it sacrifices type safety.

     Recommendation: Use a generic type parameter for params to allow users to specify the expected structure of the parameters. For example:

     async evaluateToolCall<T extends Record<string, unknown>>(
       agentId: string,
       toolName: string,
       params: T = {} as T,
     ): Promise<McpGatewayDecision> {

2. Regex Timeout Configuration

   • The scanTimeoutMs configuration is used to create a regex scan budget, but it is not clear how this value is determined or if it is sufficient for all use cases.

     Recommendation: Provide guidance in the documentation on how to configure scanTimeoutMs based on the expected size and complexity of the input.

3. Unit Test Coverage

   • While the PR includes a Jest configuration, there is no evidence of unit tests for the newly introduced functionality.

     Recommendation: Add unit tests for all critical components, especially CredentialRedactor and McpGateway. Focus on edge cases, such as large inputs, invalid patterns, and error scenarios.

4. Documentation Enhancements

   • The documentation is comprehensive but could benefit from additional details on the following:
     • Examples of custom implementations for auditSink and rateLimiter.
     • A detailed explanation of the policyEvaluator and approvalHandler interfaces, including example implementations.
     • A section on best practices for securing MCP deployments.

5. Concurrency and Thread Safety

   • The McpSlidingRateLimiter and CredentialRedactor classes use in-memory storage, which may not be thread-safe in multi-threaded environments.

     Recommendation: Document the thread-safety limitations of in-memory implementations and provide guidance on using durable, thread-safe alternatives in production.

6. Backward Compatibility

   • The @microsoft/agentmesh-mcp-governance package introduces new APIs, but it is not clear if these APIs are consistent with existing MCP security primitives.

     Recommendation: Ensure that the new APIs are consistent with existing MCP security primitives to minimize confusion for developers.

---

Conclusion

This PR introduces valuable documentation and functionality for MCP security primitives, but there are critical security issues that need to be addressed before merging. Additionally, there are opportunities to improve type safety, documentation, and test coverage. Please address the critical issues and consider the suggestions to enhance the quality and security of the implementation.

[github-actions]

🤖 AI Agent: code-reviewer

Pull Request Review: docs(typescript): add MCP security documentation and examples

---

🔴 CRITICAL: Security Issues

1. Regex Denial of Service (ReDoS) Vulnerability

   • The CredentialRedactor and McpGateway classes use user-defined and built-in regular expressions for scanning and redacting sensitive data. While the validateRegex function is invoked, it is unclear if it adequately protects against catastrophic backtracking in regex patterns.
   • Actionable Fix: Enhance validateRegex to detect and reject potentially catastrophic regex patterns. Consider using libraries like safe-regex or re2 for safer regex evaluation.

2. Fail-Closed Behavior in McpGateway

   • The evaluateToolCall method has a fail-closed mechanism for internal errors, which is good. However, the finalize method also contains error handling that could potentially overwrite the fail-closed behavior if it fails.
   • Actionable Fix: Ensure that any failure in finalize does not compromise the fail-closed behavior of evaluateToolCall. Add explicit tests for these edge cases.

3. Sensitive Key Redaction

   • The CredentialRedactor class uses a regex (SENSITIVE_KEY_PATTERN) to identify sensitive keys. However, this regex may not cover all possible variations of sensitive key names.
   • Actionable Fix: Expand the SENSITIVE_KEY_PATTERN to include more variations of sensitive key names, or allow users to provide additional patterns via configuration.

4. Blocked Patterns in McpGateway

   • The checkSanitization method in McpGateway uses both user-defined and built-in patterns to block dangerous input. However, the built-in patterns (BUILTIN_DANGEROUS_PATTERNS) may not cover all potential attack vectors.
   • Actionable Fix: Regularly update BUILTIN_DANGEROUS_PATTERNS to include new attack patterns. Consider integrating with a threat intelligence feed for dynamic updates.

---

🟡 WARNING: Potential Breaking Changes

1. Public Preview API

   • The @microsoft/agentmesh-mcp-governance package is marked as a "Public Preview" release, with a warning that APIs may change before GA.
   • Actionable Fix: Clearly document any planned breaking changes in the README and provide migration guides when transitioning to GA.

2. Rate Limiter Behavior

   • The McpSlidingRateLimiter enforces rate limits but does not provide a mechanism for customizing the error response when limits are exceeded.
   • Actionable Fix: Allow users to configure custom error messages or actions when rate limits are hit.

---

💡 Suggestions for Improvement

1. TypeScript Type Safety

   • The McpGateway and CredentialRedactor classes rely on Record<string, unknown> for parameters. This weakens type safety.
   • Suggestion: Use stricter TypeScript types or generics to enforce type safety for parameters.

2. Test Coverage

   • While Jest is configured, there is no mention of test coverage for critical components like CredentialRedactor and McpGateway.
   • Suggestion: Add unit tests to cover edge cases, such as:
     • Regex timeouts in CredentialRedactor.
     • Fail-closed behavior in McpGateway.
     • Handling of malformed or malicious input.

3. Documentation

   • The README provides a good overview but lacks detailed examples for advanced use cases, such as integrating with custom audit sinks or rate limiters.
   • Suggestion: Add more advanced usage examples and a section on common pitfalls.

4. Performance Optimization

   • The redactNode method in CredentialRedactor recursively processes objects and arrays, which could lead to performance issues with deeply nested structures.
   • Suggestion: Add a depth limit to prevent excessive recursion and document this behavior.

5. Internationalization (i18n)

   • The PR mentions i18n translations but does not include any i18n-specific files or configurations.
   • Suggestion: Clarify the i18n strategy and include examples of localized error messages or documentation.

---

Summary of Recommendations

• 🔴 Address potential ReDoS vulnerabilities in regex patterns.
• 🔴 Ensure robust fail-closed behavior in McpGateway.
• 🔴 Expand sensitive key and blocked pattern detection.
• 🟡 Clearly document potential breaking changes for the public preview API.
• 💡 Improve type safety, test coverage, and documentation.
• 💡 Optimize recursive methods for performance and add depth limits.
• 💡 Clarify and implement i18n strategy.

This PR introduces important documentation and examples for the MCP security primitives, but the identified issues should be addressed to ensure the robustness and security of the implementation.

[github-actions]

🤖 AI Agent: code-reviewer

Pull Request Review: docs(typescript): add MCP security documentation and examples

---

🔴 CRITICAL Issues

1. Credential Redaction: Insufficient Handling of Sensitive Keys

   • The CredentialRedactor class uses a regular expression (SENSITIVE_KEY_PATTERN) to identify sensitive keys. However, this approach is prone to false negatives and may miss keys with unconventional naming conventions.
   • Recommendation: Consider implementing a more robust mechanism for identifying sensitive keys, such as a configurable allowlist/denylist or leveraging machine learning models trained on common patterns of sensitive data.

2. Regex DoS Vulnerability

   • The CredentialRedactor and McpGateway classes use regular expressions extensively, including user-configurable patterns. While there is a validateRegex function, the implementation of this function is not included in the diff, making it unclear if it adequately protects against ReDoS (Regular Expression Denial of Service) attacks.
   • Recommendation: Ensure that validateRegex includes safeguards against catastrophic backtracking. Additionally, consider implementing a timeout mechanism for regex execution to prevent potential DoS attacks.

3. Fail-Closed Behavior in Gateway

   • The evaluateToolCall method in McpGateway fails closed when an error occurs, which is a good security practice. However, the error message returned to the user (Internal error - access denied (fail-closed)) may inadvertently leak implementation details if the getSafeErrorMessage function is not properly sanitizing error messages.
   • Recommendation: Review the implementation of getSafeErrorMessage to ensure it does not expose sensitive internal details.

4. Replay Protection in MCPMessageSigner

   • The MCPMessageSigner is mentioned as providing HMAC-SHA256 payload signing with timestamp and nonce replay protection. However, there is no evidence in the diff that nonce storage or validation is implemented.
   • Recommendation: Ensure that nonce replay protection is implemented with a durable storage mechanism to prevent replay attacks.

---

🟡 WARNING: Potential Breaking Changes

1. Public API Stability

   • The @microsoft/agentmesh-mcp-governance package is marked as a "Public Preview" with a warning that APIs may change before GA. This could lead to breaking changes for users who adopt the package early.
   • Recommendation: Clearly document the expected timeline for API stabilization and provide migration guides for any breaking changes.

2. Default Behavior of CredentialRedactor

   • The CredentialRedactor class defaults to redacting sensitive keys (redactSensitiveKeys: true). This could lead to unexpected behavior for users who are unaware of this default setting.
   • Recommendation: Explicitly document this behavior in the README.md and consider requiring users to explicitly enable this feature to avoid surprises.

---

💡 Suggestions for Improvement

1. Documentation Enhancements

   • The README.md provides a good overview of the package, but it could benefit from additional details:
     • Add a section on how to configure and use custom patterns in the CredentialRedactor.
     • Provide examples of durable storage implementations for session, nonce, rate-limit, and audit storage.
     • Include a security considerations section to highlight best practices and potential pitfalls.

2. Testing Coverage

   • The jest.config.js file specifies test coverage collection, but there is no evidence of tests for critical components like CredentialRedactor and McpGateway.
   • Recommendation: Add comprehensive unit and integration tests for these components, focusing on edge cases and security scenarios (e.g., ReDoS, replay attacks, and fail-closed behavior).

3. Thread Safety

   • The McpGateway class uses shared resources like rateLimiter and auditSink. There is no indication of thread safety mechanisms (e.g., locks or atomic operations) to prevent race conditions in concurrent environments.
   • Recommendation: Review the implementation of shared resources to ensure thread safety, especially in high-concurrency scenarios.

4. Type Safety

   • The use of TypeScript provides a strong foundation for type safety. However, the evaluateToolCall method in McpGateway accepts params as Record<string, unknown>, which could lead to runtime errors if the structure of params is not validated.
   • Recommendation: Use a schema validation library (e.g., zod or ajv) to validate the structure of params at runtime.

5. OWASP MCP Cheat Sheet Compliance

   • The README.md mentions OWASP MCP Cheat Sheet compliance but does not provide a detailed mapping of how the package addresses specific recommendations.
   • Recommendation: Add a dedicated section in the documentation that maps package features to OWASP MCP Cheat Sheet recommendations.

---

Summary

This PR introduces a new TypeScript package for MCP governance primitives, along with comprehensive documentation. While the implementation demonstrates strong security principles (e.g., fail-closed behavior), there are critical issues related to regex safety, sensitive data handling, and replay protection that need to be addressed. Additionally, the documentation and testing coverage could be improved to enhance usability and reliability.

Action Items:

1. Address the critical issues related to regex safety, sensitive data handling, and replay protection.
2. Improve documentation with additional examples, security considerations, and OWASP compliance mapping.
3. Add comprehensive tests for critical components.
4. Review and ensure thread safety for shared resources.
5. Validate input parameters using a schema validation library.

Once these issues are resolved, the package will be better positioned for a stable release.

[github-actions]

🤖 AI Agent: code-reviewer

Pull Request Review for microsoft/agent-governance-toolkit

---

Focus Areas

1. Policy Engine Correctness
2. Trust/Identity: Cryptographic Operations, Credential Handling
3. Sandbox Escape Vectors
4. Thread Safety in Concurrent Agent Execution
5. OWASP Agentic Top 10 Compliance
6. Type Safety and Pydantic Model Validation
7. Backward Compatibility

---

Feedback

🔴 CRITICAL: Security Issues

1. Credential Redaction Timeout Handling

   • The CredentialRedactor class uses a scanTimeoutMs parameter to limit regex scanning time. However, if the timeout is exceeded, the createRegexScanBudget function appears to block content entirely (Regex scan exceeded time budget - content blocked). This fail-closed behavior is good, but:
     • Issue: The error message does not propagate to the caller, making debugging difficult.
     • Impact: This could lead to silent failures in production where redaction fails without clear visibility.
     • Action: Ensure that timeout-related errors are logged or included in the CredentialRedactionResult for better observability.

2. Regex Denial-of-Service (ReDoS)

   • The CredentialRedactor and McpGateway classes rely heavily on regex patterns for scanning sensitive data and blocked patterns. While validateRegex is used to validate patterns, there is no explicit check for catastrophic backtracking vulnerabilities in user-supplied regexes.
     • Impact: A maliciously crafted regex could cause a denial-of-service (ReDoS) attack.
     • Action: Use a library like safe-regex to validate regex patterns for ReDoS vulnerabilities before execution.

3. HMAC-SHA256 Secret Management

   • The MCPMessageSigner uses process.env.MCP_SIGNING_SECRET for HMAC-SHA256 signing. However, there is no validation to ensure the secret meets minimum entropy requirements.
     • Impact: Weak secrets could compromise the integrity of signed messages.
     • Action: Add a validation step to ensure the secret is sufficiently long (e.g., at least 32 characters) and random.

4. Approval Handler Default Behavior

   • In the McpGateway class, if no approvalHandler is provided, the default behavior is to set the approval status to Pending. This could lead to unintended denial of service if the approval process is not properly configured.
     • Impact: Agents may be blocked unnecessarily due to misconfiguration.
     • Action: Log a warning if approvalHandler is not configured, and provide a clear error message in the decision response.

---

🟡 WARNING: Potential Breaking Changes

1. Public API Stability

   • The @microsoft/agentmesh-mcp-governance package is marked as a "Public Preview" release, with a disclaimer that APIs may change before GA. However, no versioning strategy for breaking changes is outlined.
     • Impact: Consumers of the package may face unexpected breaking changes.
     • Action: Clearly document a versioning strategy (e.g., semantic versioning) and provide deprecation warnings for any planned breaking changes.

2. Default Configuration Behavior

   • The McpGateway class has several default configurations (e.g., allowedTools, deniedTools, rateLimiter). If these defaults are changed in future releases, it could break existing integrations.
     • Impact: Consumers relying on default behavior may experience unexpected failures.
     • Action: Document all default configurations explicitly in the README and provide migration guides for future changes.

---

💡 Suggestions for Improvement

1. Enhanced Logging

   • The debugSecurityFailure function is used for logging errors in the McpGateway class. However, it is unclear if this function integrates with a centralized logging system.
     • Suggestion: Ensure that all security-related logs are structured and compatible with common logging frameworks (e.g., Azure Monitor, ELK stack).

2. Rate Limiter Customization

   • The McpSlidingRateLimiter class enforces a fixed sliding window rate limit. However, there is no support for dynamic rate limits based on agent behavior or priority.
     • Suggestion: Consider adding support for dynamic rate limits, e.g., based on agent roles or historical usage patterns.

3. Credential Redaction Preview

   • The truncatePreview function is used to generate previews of redacted content. However, the truncation length is not configurable.
     • Suggestion: Allow the truncation length to be configurable via the CredentialRedactorConfig.

4. Documentation Enhancements

   • The README for the @microsoft/agentmesh-mcp-governance package provides a good overview but lacks detailed examples for advanced use cases (e.g., custom policyEvaluator or approvalHandler implementations).
     • Suggestion: Add more detailed examples for advanced configurations and real-world scenarios.

5. TypeScript Type Safety

   • The TypeScript types in the McpGateway and CredentialRedactor classes are well-defined. However, some types (e.g., McpGatewayDecision) use optional properties (approvalStatus, rateLimit) that could lead to runtime errors if not handled properly.
     • Suggestion: Use stricter type definitions or provide utility functions to safely access optional properties.

6. Internationalization (i18n)

   • The documentation mentions i18n translations but does not provide any examples or details.
     • Suggestion: Include examples of how to use i18n features, especially for error messages and logs.

7. Test Coverage

   • The jest.config.js file specifies collectCoverageFrom: ['src/**/*.ts'], but there is no mention of the current test coverage percentage.
     • Suggestion: Add a CI step to enforce a minimum test coverage threshold (e.g., 80%) and fail the build if coverage drops below this threshold.

---

Summary

• Strengths:

  • The McpGateway and CredentialRedactor classes demonstrate a strong focus on security and fail-closed behavior.
  • The use of TypeScript ensures type safety and reduces the likelihood of runtime errors.
  • The documentation is comprehensive and provides a good starting point for users.

• Areas for Improvement:

  • Address potential security vulnerabilities (e.g., ReDoS, weak secrets).
  • Improve logging and observability for debugging and monitoring.
  • Provide more detailed examples and documentation for advanced use cases.

• Action Items:

  • 🔴 Address critical security issues (e.g., ReDoS, secret validation).
  • 🟡 Document a versioning strategy and default configuration behavior to avoid breaking changes.
  • 💡 Enhance logging, rate limiter customization, and test coverage enforcement.

---

Let me know if you need further clarification or assistance!

[github-actions]

🤖 AI Agent: code-reviewer

Pull Request Review for microsoft/agent-governance-toolkit

---

Focus Areas Review:

Policy Engine Correctness

1. 🔴 CRITICAL: The McpGateway class uses Array.prototype.includes for checking deniedTools and allowedTools. This approach is case-sensitive. If tool names are provided in mixed cases, this could lead to false negatives, allowing unauthorized tools to bypass the policy. Consider normalizing tool names (e.g., converting to lowercase) before performing checks.

2. 💡 SUGGESTION: The evaluateToolCall method does not explicitly handle cases where allowedTools and deniedTools overlap. While the current implementation prioritizes deniedTools, it would be clearer to explicitly document or enforce this behavior.

3. 💡 SUGGESTION: The blockedPatterns configuration in McpGateway does not combine user-defined patterns with the BUILTIN_DANGEROUS_PATTERNS. Consider merging these lists to ensure both built-in and custom patterns are checked.

---

Trust/Identity: Cryptographic Operations, Credential Handling, SPIFFE/SVID

1. 🔴 CRITICAL: The MCPMessageSigner class uses HMAC-SHA256 for signing payloads. However, the secret used for signing is fetched from process.env.MCP_SIGNING_SECRET without validation. Ensure the secret meets minimum entropy requirements (e.g., 256 bits) and is validated during initialization.

2. 💡 SUGGESTION: The CredentialRedactor class uses a hardcoded list of patterns for sensitive data detection. While this is a good starting point, it might miss other sensitive patterns. Consider allowing users to dynamically update or extend the BUILTIN_PATTERNS at runtime.

---

Sandbox Escape Vectors

1. 🔴 CRITICAL: The McpGateway class does not sanitize or validate the params object before processing. If this object is user-controlled, it could potentially contain malicious payloads. Ensure deep validation and sanitization of params before use.

2. 💡 SUGGESTION: The blockedPatterns feature in McpGateway is a good safeguard, but it relies on regular expressions. Ensure that these patterns are thoroughly tested to avoid bypasses or performance issues (e.g., ReDoS attacks).

---

Thread Safety in Concurrent Agent Execution

1. 💡 SUGGESTION: The CredentialRedactor class uses a WeakMap to track seen objects during recursive redaction. While this prevents infinite loops, it may not be thread-safe in multi-threaded environments. Consider documenting this limitation or implementing thread-safe alternatives.

2. 💡 SUGGESTION: The McpSlidingRateLimiter class appears to handle rate-limiting in memory. For distributed systems, ensure that rate-limiting is implemented using a shared, thread-safe store (e.g., Redis) to avoid inconsistencies.

---

OWASP Agentic Top 10 Compliance

1. 🔴 CRITICAL: The McpGateway class does not appear to enforce strong input validation for params. This could lead to injection attacks (e.g., command injection, SQL injection). Implement robust input validation mechanisms for all user-controlled inputs.

2. 🔴 CRITICAL: The McpGateway class does not enforce a maximum size for the params object. This could lead to denial-of-service (DoS) attacks if excessively large payloads are submitted. Add a size limit for params.

3. 💡 SUGGESTION: The CredentialRedactor class uses regular expressions for sensitive data detection. Ensure these patterns are tested against adversarial inputs to prevent bypasses or performance degradation (e.g., ReDoS).

---

Type Safety and Pydantic Model Validation

1. 💡 SUGGESTION: While this PR is focused on TypeScript, consider adding runtime validation for critical inputs (e.g., params in McpGateway) using libraries like zod or io-ts. This would complement TypeScript's compile-time checks.

---

Backward Compatibility (Public API Changes)

1. 🟡 WARNING: The @microsoft/agentmesh-mcp-governance package is marked as a "Public Preview" release. Ensure that any breaking changes are clearly documented and communicated to users before the General Availability (GA) release.

---

Additional Observations

1. 💡 SUGGESTION: The CredentialRedactor class uses a default replacement string of [REDACTED]. Consider allowing users to configure this string globally for consistency across the application.

2. 💡 SUGGESTION: The McpGateway class uses a fail-closed approach for errors, which is a good practice. However, consider adding more granular error logging to help developers debug issues during policy evaluation.

3. 💡 SUGGESTION: The jest.config.js file specifies a coverageDirectory but does not enforce a minimum coverage threshold. Consider adding a coverageThreshold configuration to ensure critical paths are adequately tested.

---

Summary of Recommendations

• 🔴 Address critical issues related to policy engine correctness, cryptographic operations, and sandbox escape vectors.
• 🟡 Document and communicate potential breaking changes in the public API.
• 💡 Implement suggested improvements for input validation, thread safety, and test coverage.

Let me know if you need further clarification or assistance!

[github-actions]

🤖 AI Agent: code-reviewer

Review Summary for Pull Request: Add MCP Security Documentation and Examples

This pull request adds extensive documentation and examples for the TypeScript MCP security primitives in the AgentMesh SDK. The changes are primarily focused on enhancing the documentation, providing usage examples, and ensuring compliance with OWASP standards.

Actionable Feedback

🔴 CRITICAL Issues

1. Secret Handling: The usage of process.env.MCP_SESSION_SECRET and process.env.MCP_SIGNING_SECRET without proper validation or fallback mechanisms can lead to security vulnerabilities. Ensure that these secrets are securely managed and validated before use. Consider implementing a more robust secret management strategy, such as using a dedicated secrets management service.

2. In-Memory Stores: The note regarding the in-memory nonce and session stores being intended for single-process development or tests is concerning. In production environments, this can lead to session fixation attacks or replay attacks if not handled properly. Ensure that durable shared storage is implemented for nonce and session management in multi-replica deployments.

🟡 WARNING Issues

1. Breaking Changes in API: The introduction of new classes and methods in the MCP governance package may lead to breaking changes if existing consumers of the SDK are not updated accordingly. Ensure that any breaking changes are clearly documented and consider versioning strategies to maintain backward compatibility.

2. Dependency Changes: The change from @agentmesh/sdk to @microsoft/agentmesh-sdk in the examples may impact existing users who have not updated their imports. Clearly communicate these changes in the documentation and consider providing migration guides.

💡 SUGGESTION Improvements

1. Documentation Clarity: While the documentation is comprehensive, consider adding more context around the security implications of each MCP primitive. For example, explain how MCPMessageSigner protects against replay attacks and why MCPSecurityScanner is essential for tool call safety.

2. Example Coverage: The examples provided are useful, but consider adding more diverse scenarios that demonstrate edge cases or potential misuse of the primitives. This will help users understand the full capabilities and limitations of the MCP security features.

3. Testing Coverage: Ensure that the tests cover a wide range of scenarios, including failure cases and edge cases. Consider adding tests for invalid inputs and security boundary tests to ensure robustness.

4. Type Safety and Pydantic Validation: If not already implemented, ensure that all input schemas are validated using Pydantic or similar libraries to enforce type safety and prevent invalid data from being processed.

5. Rate Limiting Configuration: The rate limiting configuration in MCPSlidingRateLimiter is hardcoded. Consider allowing users to configure these parameters through environment variables or configuration files for better flexibility.

Conclusion

The documentation and examples added in this pull request significantly enhance the usability of the MCP security primitives. However, attention must be paid to critical security issues and potential breaking changes to ensure a smooth transition for existing users. Addressing the suggestions will further improve the robustness and clarity of the documentation.

[github-actions]

🤖 AI Agent: code-reviewer

Review Summary

This pull request adds extensive documentation and examples for the TypeScript MCP security primitives in the agent-mesh package. The changes include API reference documentation, OWASP compliance mapping, quickstart guides, and a new example Express.js server demonstrating the use of these primitives.

Actionable Feedback

🔴 CRITICAL Issues

1. Secret Management: The loadSecret function generates a random secret if the environment variable is not set or is less than 32 bytes. This could lead to security vulnerabilities if the application runs without a proper secret. Ensure that the application fails to start if the required secrets are not provided, rather than generating insecure defaults.

🟡 WARNING Issues

1. Public API Changes: The introduction of new classes and methods in the MCP governance package may affect existing consumers of the API. Ensure that the changes are backward compatible or clearly document any breaking changes in the release notes.

💡 SUGGESTION Improvements

1. Documentation Clarity: While the documentation is comprehensive, consider adding more examples that illustrate common pitfalls or security considerations when using the MCP primitives. This could help users avoid misconfigurations that could lead to security vulnerabilities.

2. Error Handling: In the Express server example, consider implementing more robust error handling. For instance, if a tool call fails due to a security scan, it would be beneficial to provide more detailed feedback to the user about what went wrong.

3. Rate Limiting Configuration: The rate limiting configuration is hardcoded in the example server. Consider allowing these configurations to be passed as environment variables or configuration options to make the server more flexible and easier to deploy in different environments.

4. Testing Coverage: Ensure that the tests cover edge cases, especially around security features like session management, signing, and scanning. This will help ensure that the implementation is robust against potential attacks.

5. Concurrency Considerations: If the MCP primitives are expected to be used in a multi-threaded or multi-process environment, ensure that they are thread-safe. Document any concurrency considerations in the usage examples.

6. Performance Considerations: The security scanning and rate limiting features may introduce latency. Consider documenting the expected performance impact and any tuning parameters that can be adjusted based on deployment needs.

Conclusion

The PR is a significant contribution to the documentation and usability of the MCP security primitives. Addressing the critical issues and considering the suggestions will enhance the security and robustness of the library.

[github-actions]

🤖 AI Agent: code-reviewer

Review Summary

This pull request adds extensive documentation and examples for the TypeScript MCP security primitives in the agent-mesh package. The changes include API references, compliance mappings, quickstart guides, and an example Express server demonstrating the use of these primitives. Overall, the documentation appears thorough and well-structured, but there are some areas for improvement and potential concerns.

Actionable Feedback

🔴 CRITICAL Issues

1. Secret Handling: The loadSecret function generates a random secret if the environment variable is not set or is less than 32 bytes. This could lead to security issues if the application runs without a proper secret. Ensure that the application is always provided with a secure secret in production environments. Consider implementing a more robust secret management solution.

🟡 WARNING Issues

1. Breaking Changes in API: The introduction of new classes and methods in the MCP security primitives could potentially break existing implementations if they rely on previous versions of the SDK. Ensure that the versioning follows semantic versioning principles and clearly document any breaking changes in the release notes.

💡 SUGGESTIONS

1. Documentation Clarity: While the documentation is comprehensive, consider adding more context around the security implications of each MCP primitive. For example, explain how the MCPGateway can be configured for different security policies and what the implications are for tool calls.

2. Example Coverage: The example Express server is a great addition, but consider adding more varied examples that cover edge cases, such as handling invalid input, unauthorized access attempts, and rate-limiting scenarios. This will help users understand how to handle real-world situations.

3. Testing Enhancements: The tests provided are a good start, but consider adding more unit tests that cover different scenarios for each tool handler. This will ensure that the implementation is robust and can handle various inputs and edge cases.

4. Rate Limiting Configuration: The rate limiting is currently hardcoded in the example server. Consider allowing users to configure these limits through environment variables or configuration files to make it easier to adapt to different deployment scenarios.

5. Concurrency Considerations: Ensure that the implementation of the MCP primitives is thread-safe, especially since they may be used in concurrent environments. Document any concurrency guarantees or limitations in the documentation.

6. OWASP Compliance Mapping: The OWASP MCP mapping section is a valuable addition. Consider expanding this section to include specific examples of how each primitive addresses common OWASP threats, which would enhance the educational value of the documentation.

Conclusion

The PR adds significant value to the agent-mesh package by enhancing its documentation and providing practical examples. Addressing the critical issue regarding secret management and considering the suggestions for improvement will further strengthen the security and usability of the library.

[jackbatzner]

CI Status Summary ✅

All CI checks pass except:

• Maintainer approval gate — awaiting review
• CodeQL status gate — stale fork artifact
• check-version-pinning — pre-existing issue (fix(vscode): pin dependency versions in agent-os-vscode package.json #831)
• link-check — references files from Core PR; will resolve after feat(typescript): add MCP protocol governance with full OWASP coverage #791 merges

TypeScript MCP docs + Express integration example. Slimmed to 8 files.

⚠️ Depends on #827 (Package) merging first. Merge order: #791 → #827 → #825.

[imran-siddique]

Reviewed docs - content looks good. This PR is blocked on its dependency feature PR being merged first. Will merge this once the base PR lands.

[github-actions]

🤖 AI Agent: code-reviewer

Pull Request Review for docs(typescript): add MCP security documentation and examples

---

🔴 CRITICAL

1. Hardcoded Secrets in Example Code:

   • The loadSecret function in server.ts generates a random secret if the environment variable is missing. While this is acceptable for development, it can lead to insecure practices in production if developers forget to set the environment variables.
   • Recommendation: Add a warning or error when the secret is missing in production mode (NODE_ENV=production). For example:

     if (!secret && process.env.NODE_ENV === 'production') {
         throw new Error(`Missing required environment variable: ${envName}`);
     }

2. Insecure Default Behavior:

   • The MCPGateway allows the read_file tool to execute if the approved parameter is set to true in the request body. This could be exploited by an attacker who knows the API and can craft a request with approved: true.
   • Recommendation: Avoid relying on user-provided input for sensitive operations. Instead, implement a more robust approval mechanism that cannot be bypassed by modifying the request payload.

3. Potential Path Traversal Vulnerability:

   • The read_file tool does not sanitize or validate the path parameter beyond checking for specific patterns like ../ or <system>. This could allow attackers to bypass the check using encoded or obfuscated paths.
   • Recommendation: Use a library like path in Node.js to resolve and validate paths. For example:

     const resolvedPath = path.resolve('/workspace', path);
     if (!resolvedPath.startsWith('/workspace')) {
         throw new Error('Path traversal detected');
     }

4. Insecure Default Rate Limiting:

   • The MCPSlidingRateLimiter is configured with a default of 5 requests per minute in the example. This is too permissive for a security-sensitive API.
   • Recommendation: Use stricter default rate limits in the example, such as 1 request per second (60 requests per minute). Also, document how to configure rate limits for production use.

---

🟡 WARNING

1. Breaking Changes in Package Name:

   • The package name has been changed from @agentmesh/sdk to @microsoft/agentmesh-sdk. This is a breaking change for existing users.
   • Recommendation: Clearly document this change in the release notes and provide a migration guide for users to update their imports.

2. Potential Backward Compatibility Issues:

   • The introduction of a new standalone package (@microsoft/agentmesh-mcp-governance) may cause confusion for existing users of the full SDK.
   • Recommendation: Clearly document the use cases for the standalone package versus the full SDK to guide users in selecting the appropriate package.

---

💡 SUGGESTIONS

1. Improved Documentation:

   • The documentation is comprehensive, but it could benefit from a dedicated section on "Best Practices for Secure Deployment" to guide users on:
     • Setting strong secrets in production.
     • Configuring rate limits based on expected traffic.
     • Implementing durable storage for nonce and session tokens in multi-replica deployments.

2. Enhanced OWASP Mapping:

   • The OWASP MCP mapping table is a great addition. Consider adding specific examples or links to the relevant sections in the documentation for each mapping to make it more actionable.

3. Test Coverage:

   • The test coverage for the mcp-express-server example is minimal. While it includes basic tests for the /health and /call-tool endpoints, it does not cover edge cases like:
     • Missing or invalid session tokens.
     • Exceeding rate limits.
     • Requests with critical security threats.
   • Recommendation: Add tests for these scenarios to ensure the governance pipeline behaves as expected under all conditions.

4. TypeScript Type Safety:

   • The args parameter in the run method of ToolHandler is typed as Record<string, unknown>. This could lead to runtime errors if the expected structure of args is not validated.
   • Recommendation: Use TypeScript's generics or a library like zod to enforce stricter type validation for args.

5. Error Handling:

   • The call-tool endpoint could benefit from more granular error handling. For example, if the sessionAuthenticator.verifyToken or messageSigner.verify methods throw an error, the server will crash.
   • Recommendation: Wrap these calls in try-catch blocks and return appropriate error responses.

6. Logging:

   • The example logs sensitive information (e.g., logEntry) to the console. While this is acceptable for development, it should be avoided in production.
   • Recommendation: Add a note in the documentation to disable logging or use a secure logging mechanism in production.

---

Summary

This PR introduces valuable documentation and examples for MCP security primitives, but there are critical security issues and potential breaking changes that need to be addressed before merging. Once the identified issues are resolved, this PR will significantly enhance the usability and security of the MCP governance features.

Action Items

1. Address the critical security issues (secrets, path traversal, approval bypass).
2. Document the breaking changes and provide a migration guide.
3. Add more comprehensive tests for the example server.
4. Improve type safety and error handling in the example code.
5. Enhance the documentation with best practices for secure deployment.

Let me know if you need further clarification or assistance!

[github-actions]

🤖 AI Agent: code-reviewer

Review Summary

This pull request introduces comprehensive documentation for the TypeScript MCP (Managed Control Plane) security primitives, including API references, OWASP MCP compliance mapping, quickstart guides, and usage examples. It also includes a new example implementation of an Express.js server that demonstrates the usage of the MCP governance pipeline.

The PR is well-structured and provides clear examples of how to use the MCP security primitives. However, there are a few areas that require attention, especially regarding security, type safety, and potential backward compatibility issues.

---

🔴 CRITICAL

1. Insecure Default for Secrets in loadSecret Function

   • The loadSecret function generates a random secret if the environment variable is not set or is less than 32 bytes. This is insecure for production environments, as it may lead to inconsistent behavior across multiple instances of the application.
   • Recommendation: Fail fast if the required environment variable is not set or is invalid. Do not generate a random secret as a fallback for production use. For example:

     function loadSecret(envName: string): string {
       const secret = process.env[envName];
       if (!secret || Buffer.byteLength(secret, 'utf-8') < 32) {
         throw new Error(`Environment variable ${envName} is missing or invalid.`);
       }
       return secret;
     }

2. Potential Path Traversal Vulnerability in read_file Tool

   • The read_file tool does not sanitize or validate the path parameter beyond checking for specific substrings like '../'. This approach is insufficient to prevent path traversal attacks.
   • Recommendation: Use a library like path in Node.js to resolve and validate paths. For example:

     const resolvedPath = path.resolve('/workspace', path);
     if (!resolvedPath.startsWith('/workspace')) {
       throw new Error('Path traversal detected');
     }

3. Insecure Handling of Sensitive Data in Logs

   • The logEntry object is logged to the console, which may include sensitive data such as redacted secrets. While the CredentialRedactor is used, it is critical to ensure that no sensitive data is inadvertently logged.
   • Recommendation: Add a configuration option to disable logging of sensitive data in production environments. Additionally, ensure that the CredentialRedactor is thoroughly tested to handle all edge cases.

4. Replay Protection for Session Tokens

   • The MCPSessionAuthenticator does not appear to implement replay protection for session tokens. This could allow an attacker to reuse a stolen token within its TTL.
   • Recommendation: Implement a mechanism to track and invalidate session tokens after use, such as a token blacklist or a one-time-use token mechanism.

---

🟡 WARNING

1. Potential Breaking Changes in Package Names

   • The package name has been updated from @agentmesh/sdk to @microsoft/agentmesh-sdk. This is a breaking change for existing users of the library.
   • Recommendation: Clearly document this breaking change in the release notes and provide migration instructions for existing users.

2. Backward Compatibility of MCP Governance API

   • The introduction of the standalone @microsoft/agentmesh-mcp-governance package may lead to confusion for existing users of the full SDK. It is unclear if the APIs in the standalone package are fully backward-compatible with the full SDK.
   • Recommendation: Add explicit tests to verify that the MCP governance APIs behave identically in both the full SDK and the standalone package.

---

💡 SUGGESTIONS

1. Improved Documentation for Security Primitives

   • The documentation for MCP security primitives is comprehensive but could benefit from additional details about the underlying cryptographic algorithms and their security guarantees.
   • Recommendation: Include a section in the documentation that explains the cryptographic algorithms used (e.g., HMAC-SHA256) and their security properties.

2. Thread Safety of In-Memory Stores

   • The in-memory nonce and session stores mentioned in the documentation may not be thread-safe in multi-threaded environments.
   • Recommendation: Document the thread-safety limitations of the in-memory stores and provide examples of how to implement durable, thread-safe stores for production use.

3. OWASP MCP Compliance Mapping

   • The OWASP MCP compliance mapping table in the documentation is helpful but could be expanded to include specific examples of how each primitive addresses the corresponding OWASP MCP category.
   • Recommendation: Add a column to the table with links to specific code examples or documentation sections that demonstrate compliance.

4. Type Safety in Tool Handlers

   • The toolHandlers object uses Record<string, unknown> for input schemas and arguments, which weakens type safety.
   • Recommendation: Define TypeScript interfaces for each tool's input schema and arguments to improve type safety and developer experience.

5. Test Coverage for Edge Cases

   • The provided tests cover basic functionality but do not test edge cases such as invalid session tokens, malformed tool calls, or high request rates.
   • Recommendation: Add tests for edge cases to ensure robust behavior under adverse conditions.

---

Conclusion

This PR introduces valuable documentation and examples for the MCP security primitives, but there are critical security issues and potential breaking changes that need to be addressed before merging. Additionally, there are opportunities to improve type safety, documentation, and test coverage.

[github-actions]

🤖 AI Agent: code-reviewer

Review Summary

This pull request introduces comprehensive documentation for TypeScript MCP (Managed Control Plane) security primitives, including API references, usage examples, and an Express.js server example. The documentation is well-structured and provides clear guidance on the usage of the MCP governance features. The PR also includes a new example project (mcp-express-server) that demonstrates the integration of MCP governance primitives in a real-world scenario.

Below is the detailed review of the pull request:

---

🔴 CRITICAL

1. Session Token Secret Handling

   • File: server.ts
   • Issue: The loadSecret function generates a random secret if the MCP_SESSION_SECRET environment variable is not set. This could lead to inconsistent behavior in multi-instance deployments or after a server restart, as the secret would change and invalidate all existing session tokens.
   • Recommendation: Ensure that the MCP_SESSION_SECRET is always explicitly set in the environment. If it's not set, the server should fail to start with a clear error message.

2. Approval Handler Logic

   • File: server.ts
   • Issue: The approvalHandler in the MCPGateway allows the read_file tool if the approved parameter is set to true. This could be exploited by an attacker to bypass the approval process by including "approved": true in the request payload.
   • Recommendation: Do not rely on user-provided input (params.approved) for critical security decisions. Instead, implement a robust approval mechanism that is independent of user input.

3. Path Traversal Protection

   • File: server.ts
   • Issue: The blockedPatterns in the MCPGateway configuration include patterns like '../' and '..\\' to prevent path traversal. However, these patterns may not cover all possible encodings or bypass techniques.
   • Recommendation: Use a robust library or framework for path sanitization instead of relying on pattern matching. For example, resolve the path to an absolute path and ensure it is within an allowed directory.

---

🟡 WARNING

1. Breaking Changes in Package Name

   • File: README.md
   • Issue: The package name has been changed from @agentmesh/sdk to @microsoft/agentmesh-sdk. This is a breaking change for existing users of the package.
   • Recommendation: Clearly communicate this breaking change in the release notes and provide migration instructions for users to update their imports.

2. Backward Compatibility for MCP Governance

   • File: README.md
   • Issue: The introduction of a standalone @microsoft/agentmesh-mcp-governance package may cause confusion for users already using the full SDK. It is unclear if the standalone package is fully backward-compatible with the MCP features in the full SDK.
   • Recommendation: Clearly document the differences (if any) between the standalone package and the full SDK. Ensure that the standalone package is fully compatible with existing MCP governance features.

---

💡 SUGGESTIONS

1. Improved Documentation for Security Features

   • File: README.md
   • Suggestion: Add a section explaining the security implications of each MCP governance primitive (e.g., how MCPSessionAuthenticator prevents session hijacking, how MCPMessageSigner ensures message integrity, etc.). This will help users understand the importance of these features.

2. Enhanced Test Coverage

   • File: server.test.ts
   • Suggestion: Add more test cases to cover edge scenarios, such as:
     • Invalid or expired session tokens.
     • Requests that trigger the blockedPatterns in MCPGateway.
     • Requests with invalid or malicious input schemas.
     • Requests that exceed the rate limit.

3. Environment Variable Validation

   • File: server.ts
   • Suggestion: Add validation for critical environment variables (MCP_SESSION_SECRET, MCP_SIGNING_SECRET) during server startup. If these variables are missing or invalid, the server should fail to start with a clear error message.

4. Nonce Replay Protection

   • File: server.ts
   • Suggestion: The MCPMessageSigner includes nonce replay protection, but the documentation does not explain how to configure or use a shared nonce store in multi-instance deployments. Add guidance on implementing a shared nonce store for production environments.

5. Rate Limiting Transparency

   • File: server.ts
   • Suggestion: Include rate-limiting headers (e.g., X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset) in the response to help clients understand their rate limit status.

6. Logging Improvements

   • File: server.ts
   • Suggestion: Use a structured logging library (e.g., pino or winston) instead of console.info for better log management and integration with log aggregation systems.

---

Summary of Actions Required

1. Address the critical issues related to session token secret handling, approval handler logic, and path traversal protection.
2. Clearly communicate the breaking changes in the package name and ensure backward compatibility for MCP governance features.
3. Enhance documentation, test coverage, and logging to improve usability and maintainability.
4. Add validation for critical environment variables and provide guidance on configuring shared stores for production deployments.

Let me know if you have any questions or need further clarification!

[imran-siddique]

Closing — this PR depends on #791 and #827 (both closed) as prerequisites. With the MCP architecture consolidation, TypeScript documentation will follow the hybrid approach from PR #910. Thank you for the docs work, @jackbatzner.