feat(typescript): add standalone @microsoft/agentmesh-mcp-governance package by jackbatzner · Pull Request #827 · microsoft/agent-governance-toolkit

jackbatzner · 2026-04-06T11:54:44Z

Description

Introduces the standalone @microsoft/agentmesh-mcp-governance npm package with zero AGT dependency. This allows MCP server operators to adopt MCP governance primitives without pulling in the full Agent Mesh SDK.

Package: @microsoft/agentmesh-mcp-governance

dependencies: {} — no AGT dependency
Re-exports MCP primitives from the core TypeScript SDK
README with installation, quick-start examples, and OWASP MCP Cheat Sheet mapping

Part 2 of 3 — Merge after #791. See also:

Part 1: feat(typescript): add MCP protocol governance with full OWASP coverage #791 Core MCP security primitives ← merge first

Part 3: Documentation and examples ← merge last

Type of Change

New feature (non-breaking change that adds functionality)

Package(s) Affected

agent-mesh

Checklist

My code follows the project style guidelines
I have added tests that prove my fix/feature works
All new and existing tests pass (npm test)
I have updated documentation as needed
I have signed the Microsoft CLA

Related Issues

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

github-actions · 2026-04-06T11:54:59Z

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: code-reviewer

Pull Request Review: feat(typescript): add standalone @microsoft/agentmesh-mcp-governance package

Summary

This PR introduces a new standalone TypeScript package, @microsoft/agentmesh-mcp-governance, which provides MCP governance primitives without requiring the full Agent Mesh SDK. The package includes features such as credential redaction, rate limiting, session authentication, message signing, and gateway enforcement. The implementation appears well-structured, with a focus on modularity and security.

🔴 CRITICAL Issues

Regex Denial of Service (ReDoS) Vulnerability
- Files Affected: credential-redactor.ts, gateway.ts
- Issue: The use of unbounded regular expressions (e.g., \bsk-[A-Za-z0-9]{16,}\b, \bBearer\s+[A-Za-z0-9._\-+/=]{10,}\b) in CredentialRedactor and McpGateway can lead to catastrophic backtracking, making the system vulnerable to ReDoS attacks.
- Recommendation: Use more efficient regex patterns or limit the input size before applying regex. Consider using libraries like safe-regex to validate regex patterns for safety.
Potential Credential Leakage
- Files Affected: credential-redactor.ts, gateway.ts
- Issue: The CredentialRedactor and McpGateway rely on regex patterns to identify sensitive data. If a pattern does not match a specific credential format, sensitive data may be leaked.
- Recommendation: Implement a fallback mechanism to flag any unrecognized patterns or enforce stricter input validation to minimize the risk of leakage.
Fail-Closed Behavior on Errors
- Files Affected: gateway.ts
- Issue: While the McpGateway implementation attempts to fail closed on errors, there are multiple try-catch blocks (e.g., in finalize and safeRedactParams) that silently handle errors without logging or propagating them.
- Recommendation: Ensure all errors are logged securely and consider propagating critical errors to higher layers for visibility.

🟡 WARNING: Potential Breaking Changes

Public API Stability
- Files Affected: index.ts, README.md
- Issue: The package is marked as "Public Preview," and the APIs are subject to change before GA. This could lead to breaking changes for early adopters.
- Recommendation: Clearly communicate API stability expectations in the documentation and consider versioning strategies to minimize disruption for users.
Custom Patterns in Credential Redactor
- Files Affected: credential-redactor.ts
- Issue: The CredentialRedactor allows custom patterns, but if users provide unsafe or inefficient regex patterns, it could lead to performance issues or ReDoS vulnerabilities.
- Recommendation: Validate custom regex patterns using a library like safe-regex before accepting them.

💡 Suggestions for Improvement

Thread Safety
- Files Affected: credential-redactor.ts, gateway.ts
- Observation: The CredentialRedactor and McpGateway classes use shared state (e.g., WeakMap for seen objects). While JavaScript is single-threaded, Node.js environments may involve worker threads or asynchronous operations that could lead to race conditions.
- Recommendation: Document the thread safety assumptions and consider using thread-safe data structures or synchronization mechanisms if multithreading is a possibility.
Enhanced Logging
- Files Affected: gateway.ts
- Observation: The McpGateway class could benefit from more detailed logging, especially for decisions made at each stage (e.g., policy evaluation, rate limiting, approval).
- Recommendation: Add structured logging for key decision points to improve observability and debugging.
Rate Limiter Configuration
- Files Affected: gateway.ts
- Observation: The McpSlidingRateLimiter is used as the default rate limiter, but its configuration is not well-documented in the README.
- Recommendation: Include detailed documentation on how to configure the rate limiter, including examples for different use cases.
Test Coverage
- Files Affected: jest.config.js, tests/
- Observation: While the PR mentions that tests have been added, the test coverage for critical components like McpGateway and CredentialRedactor is not visible in the diff.
- Recommendation: Ensure comprehensive test coverage for all critical paths, including edge cases, error handling, and performance under load.
Type Safety
- Files Affected: credential-redactor.ts, gateway.ts
- Observation: The use of unknown types in redactNode and safeRedactParams could lead to runtime errors if the input structure is not as expected.
- Recommendation: Use stricter TypeScript types and runtime validation (e.g., zod or io-ts) to ensure input data conforms to expected schemas.
OWASP Agentic Top 10 Compliance
- Files Affected: General
- Observation: The package appears to address several OWASP Agentic Top 10 risks (e.g., input validation, rate limiting, and credential redaction). However, there is no explicit mention of compliance in the documentation.
- Recommendation: Map the package's features to OWASP Agentic Top 10 risks in the README to highlight its security posture.
Backward Compatibility
- Files Affected: index.ts
- Observation: The package re-exports several classes with both their original and aliased names (e.g., McpResponseScanner and MCPResponseScanner). This could lead to confusion and potential issues with backward compatibility in future versions.
- Recommendation: Standardize on a single naming convention and deprecate one of the aliases to avoid confusion.

Conclusion

This PR introduces a valuable addition to the agent-governance-toolkit repository by providing a lightweight, standalone package for MCP governance. However, there are critical security issues related to regex patterns and error handling that must be addressed before merging. Additionally, attention should be given to API stability, thread safety, and documentation to ensure a robust and user-friendly release.

Let me know if you need further clarification or assistance!

github-actions · 2026-04-06T11:55:20Z

🤖 AI Agent: security-scanner — Security Review of `@microsoft/agentmesh-mcp-governance`

Security Review of `@microsoft/agentmesh-mcp-governance`

Summary

This pull request introduces a new standalone package, @microsoft/agentmesh-mcp-governance, which provides MCP governance primitives without requiring the full Agent Mesh SDK. The package includes several security-critical components, such as MCPGateway, CredentialRedactor, and various security-related utilities. Given the critical nature of this package, a thorough security review is essential.

Findings

1. Prompt Injection Defense Bypass

Rating: 🔴 CRITICAL

Issue:
The McpGateway class includes a checkSanitization method that scans parameters for blocked patterns and dangerous patterns using regular expressions. However, the BUILTIN_DANGEROUS_PATTERNS list and blockedPatterns configuration are insufficient to handle all potential prompt injection attacks. For example:

The BUILTIN_DANGEROUS_PATTERNS list does not include patterns for common prompt injection techniques, such as nested or obfuscated commands (e.g., {{"{{7*7}}"}} or {{"{{7+7}}"}}).
The hasShellExpansion function only checks for $(...) syntax but does not account for other shell expansion techniques like backticks (`command`) or ${var}.

Attack Vector:
An attacker could craft malicious input that bypasses the current blocked patterns and injects harmful commands or manipulates the behavior of downstream systems.

Recommendation:

Expand the BUILTIN_DANGEROUS_PATTERNS list to include additional patterns for common prompt injection techniques.
Enhance the hasShellExpansion function to detect other forms of shell expansion, such as backticks and ${}.
Consider integrating a more robust library or framework for detecting prompt injection and other malicious patterns.

2. Policy Engine Circumvention

Rating: 🟠 HIGH

Issue:
The evaluateToolCall method in McpGateway relies on a policyEvaluator callback to make policy decisions. However:

The policyEvaluator is optional, and if not provided, the system defaults to allowing all tools that are not explicitly denied or blocked by patterns.
There is no mechanism to enforce that the policyEvaluator is implemented securely or that it adheres to best practices.

Attack Vector:
If a developer fails to implement a robust policyEvaluator, or if the callback is omitted, the system could allow unauthorized or unsafe tool calls, leading to potential security breaches.

Recommendation:

Make the policyEvaluator a required configuration parameter.
Provide a default policyEvaluator implementation that enforces a strict deny-by-default policy.
Add documentation and examples emphasizing the importance of implementing a secure policyEvaluator.

3. Trust Chain Weaknesses

Rating: 🔵 LOW

Issue:
The MCPMessageSigner and MCPSessionAuthenticator components rely on secrets (MCP_SESSION_SECRET and MCP_SIGNING_SECRET) for signing and authenticating messages. However:

There is no mention of how these secrets are managed, rotated, or stored securely.
The MCPMessageSigner does not appear to validate the timestamp or nonce in the HMAC-SHA256 signature, which could allow replay attacks.

Attack Vector:
If the secrets are not securely managed or rotated, they could be leaked or compromised, allowing attackers to forge signatures or bypass authentication. Additionally, the lack of timestamp and nonce validation could allow attackers to replay previously captured messages.

Recommendation:

Provide guidance on secure secret management, including storage and rotation.
Implement timestamp and nonce validation in the MCPMessageSigner to prevent replay attacks.

4. Credential Exposure

Rating: 🟠 HIGH

Issue:
The CredentialRedactor class is designed to redact sensitive information from strings and objects. However:

The BUILTIN_PATTERNS list does not cover all possible credential formats (e.g., JWT tokens, OAuth tokens, or custom API keys).
The redactStringWithBudget method does not handle edge cases where sensitive information might be split across multiple substrings or encoded in non-standard formats.

Attack Vector:
Sensitive credentials could be inadvertently logged or exposed if they are not properly redacted, leading to potential data breaches.

Recommendation:

Expand the BUILTIN_PATTERNS list to include additional credential formats, such as JWT tokens and OAuth tokens.
Consider using a more comprehensive library for detecting and redacting sensitive information.
Add test cases to verify that the CredentialRedactor handles edge cases, such as split or encoded credentials.

5. Sandbox Escape

Rating: 🔵 LOW

Issue:
The McpGateway class includes checks for shell expansion patterns (e.g., $(...)) in the checkSanitization method. However:

The hasShellExpansion function only checks for a specific pattern and does not account for other potential sandbox escape techniques, such as file inclusion or command injection via environment variables.

Attack Vector:
An attacker could exploit unhandled sandbox escape techniques to execute arbitrary commands or access sensitive data.

Recommendation:

Expand the checkSanitization method to include additional checks for sandbox escape techniques, such as file inclusion and environment variable injection.
Consider integrating a dedicated library for detecting and mitigating sandbox escape vulnerabilities.

6. Deserialization Attacks

Rating: 🔵 LOW

Issue:
The code does not appear to perform any deserialization of untrusted data. However, if deserialization is introduced in the future, it could become a potential attack vector.

Recommendation:

Ensure that any future deserialization of untrusted data is performed using safe libraries or techniques.
Add tests to verify the safety of deserialization logic if it is introduced.

7. Race Conditions

Rating: 🟡 MEDIUM

Issue:
The McpSlidingRateLimiter and McpGateway classes rely on rate-limiting and approval mechanisms to enforce security policies. However:

There is no mention of how these mechanisms handle concurrent requests or prevent race conditions.
The rateLimiter and approvalHandler callbacks are external dependencies, and their implementations may not be thread-safe.

Attack Vector:
An attacker could exploit race conditions in the rate-limiting or approval mechanisms to bypass security policies or exceed rate limits.

Recommendation:

Ensure that the McpSlidingRateLimiter and McpGateway classes are thread-safe and can handle concurrent requests.
Provide guidance on implementing thread-safe rateLimiter and approvalHandler callbacks.

8. Supply Chain Risks

Rating: 🟡 MEDIUM

Issue:
The package.json file includes several dependencies, such as typescript, jest, and eslint. While these are well-known libraries, there is no mention of dependency auditing or verification.

Attack Vector:
An attacker could exploit dependency confusion or typosquatting to introduce malicious code into the project.

Recommendation:

Use a dependency auditing tool (e.g., npm audit, Snyk, or Dependabot) to regularly scan for vulnerabilities in dependencies.
Pin dependency versions to specific versions or ranges to prevent unintentional updates to malicious versions.
Verify the integrity of dependencies using checksums or signatures.

Summary Table of Findings

Finding	Rating	Attack Vector	Recommendation
Prompt injection defense bypass	🔴 CRITICAL	Malicious input bypasses blocked patterns or shell expansion checks.	Expand patterns, enhance `hasShellExpansion`, and consider a dedicated library.
Policy engine circumvention	🟠 HIGH	Optional `policyEvaluator` allows insecure defaults or weak implementations.	Make `policyEvaluator` mandatory and provide a secure default implementation.
Trust chain weaknesses	🔵 LOW	Lack of secret management guidance and replay protection in message signing.	Add secret management guidance and implement timestamp/nonce validation.
Credential exposure	🟠 HIGH	Incomplete redaction patterns may expose sensitive information.	Expand patterns, handle edge cases, and add comprehensive tests.
Sandbox escape	🔵 LOW	Limited checks for sandbox escape techniques.	Add checks for file inclusion, environment variable injection, and other methods.
Deserialization attacks	🔵 LOW	No deserialization currently, but future risks exist.	Use safe deserialization libraries if needed in the future.
Race conditions	🟡 MEDIUM	Potential race conditions in rate-limiting and approval mechanisms.	Ensure thread safety and provide guidance for thread-safe callback implementations.
Supply chain risks	🟡 MEDIUM	Dependency confusion or typosquatting risks in `package.json`.	Use dependency auditing tools and pin dependency versions.

Final Recommendation

This pull request introduces a critical security package, and the identified issues must be addressed before merging. Specifically:

Address the Prompt Injection Defense Bypass issue as a priority.
Ensure the Policy Engine Circumvention issue is resolved by requiring a secure policyEvaluator.
Expand the CredentialRedactor to cover more credential formats and edge cases.
Implement replay protection in MCPMessageSigner.

Once these critical and high-priority issues are resolved, the pull request can be re-evaluated for merging.

packages/agent-mesh/packages/mcp-governance/src/utils.ts

packages/agent-mesh/sdks/typescript/src/mcp-gateway.ts

packages/agent-mesh/sdks/typescript/src/mcp-security.ts

github-actions

🤖 AI Agent: code-reviewer

Pull Request Review: `feat(typescript): add standalone @microsoft/agentmesh-mcp-governance package`

General Feedback

This PR introduces a new standalone TypeScript package, @microsoft/agentmesh-mcp-governance, which provides MCP governance primitives without requiring the full Agent Mesh SDK. The package appears well-structured, with a clear focus on modularity and reusability. The provided documentation is comprehensive, and the code adheres to modern TypeScript and JavaScript best practices.

However, there are several areas where improvements can be made, particularly in terms of security, type safety, and backward compatibility. Below is a detailed review of the PR.

🔴 CRITICAL: Security Issues

Regex Denial of Service (ReDoS) Vulnerabilities
- The use of regular expressions in CredentialRedactor and McpGateway introduces potential ReDoS vulnerabilities. For example:
```
const SENSITIVE_KEY_PATTERN = /(password|passwd|pwd|secret|token|api[_-]?key|connection.?string|accountkey|sharedaccesssignature|sas)/i;
const BUILTIN_DANGEROUS_PATTERNS = [
  /\b\d{3}-\d{2}-\d{4}\b/gi, // SSN
  /\b\d{4}[- ]?\d{4}[- ]?\d{4}[- ]?\d{4}\b/gi, // Credit card
  /;\s*(?:rm|del|format|mkfs)\b/gi, // Shell commands
  /\$$[^)\r\n]*$/g, // Command substitution
  /`[^`]+`/g, // Backticks
];
```
  These patterns are prone to catastrophic backtracking, especially when applied to large inputs. For example, the SENSITIVE_KEY_PATTERN and BUILTIN_DANGEROUS_PATTERNS could be exploited to cause significant performance degradation.
  
  Actionable Recommendation:
  - Use a library like safe-regex to validate the safety of regular expressions.
  - Consider using more efficient, non-backtracking regex patterns or alternative parsing methods.
Fail-Closed Behavior in McpGateway
- The evaluateToolCall method in McpGateway has a fail-closed mechanism for errors:
```
return {
  allowed: false,
  reason: getSafeErrorMessage(error, 'Internal error - access denied (fail-closed)'),
  redactedParams: this.safeRedactParams(params),
  findings: [],
};
```
  While this is a good practice, it would be better to log the error details (e.g., stack trace) for debugging purposes. Currently, the error is only converted to a safe error message, which might obscure critical debugging information.
  
  Actionable Recommendation:
  - Log the full error details (e.g., stack trace) to a secure logging system for debugging purposes, while still returning a sanitized error message to the caller.
Credential Redaction
- The CredentialRedactor class uses a set of predefined patterns to identify and redact sensitive information. However, the current implementation does not allow for dynamic updates to the list of patterns after the class is instantiated.
- Additionally, the redactNode method does not handle circular references in objects, which could lead to infinite recursion.
  
  Actionable Recommendation:
  - Allow dynamic updates to the list of patterns (e.g., via a addPattern method).
  - Add a mechanism to detect and handle circular references in redactNode.
HMAC-SHA256 Key Management
- The MCPMessageSigner class uses a secret key for HMAC-SHA256 signing. However, there is no mention of how this key is securely stored or rotated.
  
  Actionable Recommendation:
  - Provide guidance in the documentation on securely storing and rotating the HMAC secret key.
  - Consider integrating with a secure key management service (e.g., Azure Key Vault).

🟡 WARNING: Potential Breaking Changes

Public API Stability
- The package is marked as "Public Preview," and the README explicitly states that APIs may change before GA. While this is acceptable for a preview release, it is important to ensure that any breaking changes are clearly communicated to users.
  
  Actionable Recommendation:
  - Maintain a changelog that explicitly documents breaking changes.
  - Consider versioning the API to allow for smoother transitions between versions.

💡 Suggestions for Improvement

Type Safety
- The redact method in CredentialRedactor uses a generic type T, but the return type is cast to T without ensuring that the redacted object conforms to the original type. This could lead to runtime type errors.
  
  Actionable Recommendation:
  - Use a stricter type-checking mechanism to ensure that the redacted object conforms to the original type.
Concurrency in McpSlidingRateLimiter
- The McpSlidingRateLimiter implementation is not included in the diff, but rate limiters often involve shared state. Ensure that the implementation is thread-safe, especially in distributed environments.
  
  Actionable Recommendation:
  - Use atomic operations or distributed locks to ensure thread safety in the rate limiter.
Documentation
- The README provides a good overview of the package, but it could benefit from additional details on:
  - Security best practices (e.g., key management, rate limiter configuration).
  - Examples of integrating the package with other Agent Mesh components.
  Actionable Recommendation:
  - Expand the README to include these details.
Testing
- While the PR mentions that tests have been added, the diff does not include the test files. Ensure that the following are covered:
  - Edge cases for regex patterns (e.g., large inputs, malicious patterns).
  - Concurrency tests for the rate limiter.
  - End-to-end tests for the McpGateway class.
  Actionable Recommendation:
  - Add tests for edge cases and concurrency scenarios.
  - Include test coverage reports in the CI pipeline.

Summary of Feedback

🔴 CRITICAL: Address potential ReDoS vulnerabilities in regex patterns.
🔴 CRITICAL: Ensure proper error logging in fail-closed scenarios.
🔴 CRITICAL: Improve credential redaction to handle dynamic updates and circular references.
🔴 CRITICAL: Provide guidance on secure storage and rotation of HMAC secret keys.
🟡 WARNING: Clearly document potential breaking changes and maintain a changelog.
💡 SUGGESTION: Improve type safety in CredentialRedactor.
💡 SUGGESTION: Ensure thread safety in the rate limiter.
💡 SUGGESTION: Expand documentation to include security best practices and integration examples.
💡 SUGGESTION: Add comprehensive tests for edge cases, concurrency, and end-to-end scenarios.

This PR introduces a valuable addition to the project, but addressing the highlighted issues is critical to ensure the security, reliability, and usability of the package.

github-actions

🤖 AI Agent: code-reviewer

Pull Request Review: feat(typescript): add standalone @microsoft/agentmesh-mcp-governance package

🔴 CRITICAL

Credential Redaction Incompleteness:
- The CredentialRedactor class uses a set of predefined patterns (BUILTIN_PATTERNS) to detect and redact sensitive information. However, the list of patterns may not cover all possible credential formats, especially custom or less common ones.
- Impact: False negatives in credential redaction could lead to sensitive information being logged or exposed.
- Action: Add a mechanism to allow users to dynamically add or update patterns at runtime. Additionally, provide a comprehensive list of patterns in the documentation and encourage users to review and extend them as needed.
Fail-Closed Behavior in McpGateway:
- The evaluateToolCall method in McpGateway has a fail-closed mechanism for errors. While this is a good practice, the error message returned (Internal error - access denied (fail-closed)) is generic.
- Impact: Lack of detailed error messages may hinder debugging and troubleshooting.
- Action: Log detailed error messages internally (e.g., to an audit sink) while keeping the external error message generic for security reasons.
Regex Denial of Service (ReDoS) Risk:
- The use of regular expressions in both CredentialRedactor and McpGateway (e.g., BUILTIN_PATTERNS and BUILTIN_DANGEROUS_PATTERNS) introduces a potential risk of ReDoS attacks if the input is maliciously crafted.
- Action: Use libraries like safe-regex to validate the safety of regular expressions. Additionally, consider using a library like re2 for regex operations to mitigate ReDoS risks.
Session Secret Management:
- The McpSessionAuthenticator and McpMessageSigner rely on secrets (MCP_SESSION_SECRET and MCP_SIGNING_SECRET) provided via environment variables. There is no validation or rotation mechanism for these secrets.
- Impact: Weak or improperly managed secrets can lead to security vulnerabilities.
- Action: Add validation for the strength of the secrets and provide guidance on secure secret management. Consider integrating with a secret management solution (e.g., Azure Key Vault).

🟡 WARNING

Potential Breaking Changes in API:
- The package introduces a new standalone module with re-exported MCP primitives. While the PR mentions that APIs may change before GA, any changes to the exported interfaces (e.g., McpGateway, CredentialRedactor) could break existing implementations.
- Action: Clearly document the stability of each API and provide a migration guide for any future changes.
Default Configuration Assumptions:
- The McpGateway class uses default configurations for deniedTools, allowedTools, and sensitiveTools. These defaults may not align with the security policies of all users.
- Action: Explicitly document the default behavior and encourage users to review and customize these configurations during setup.

💡 SUGGESTIONS

TypeScript Type Safety:
- The use of Record<string, unknown> for parameters in McpGateway and CredentialRedactor could be improved by using stricter types or generics to enforce type safety.
- Action: Define specific interfaces or use generics to provide better type safety and developer experience.
Concurrency and Thread Safety:
- The McpSlidingRateLimiter and InMemoryMCPAuditSink implementations may face concurrency issues in multi-threaded or distributed environments.
- Action: Document that these implementations are suitable for single-threaded or test environments only. Provide guidance or examples for integrating with distributed stores (e.g., Redis).
OWASP MCP Compliance:
- The README mentions an "OWASP MCP Cheat Sheet mapping," but no details are provided.
- Action: Include a detailed mapping of how the package aligns with OWASP MCP guidelines in the documentation.
Testing Coverage:
- While the PR mentions that tests have been added, there is no mention of coverage metrics or specific test cases for critical components like McpGateway and CredentialRedactor.
- Action: Ensure 100% test coverage for security-critical components and include tests for edge cases (e.g., malformed inputs, large payloads).
Performance Optimization:
- The CredentialRedactor and McpGateway classes perform multiple regex scans on input data. For large payloads, this could lead to performance bottlenecks.
- Action: Benchmark the performance of these operations and consider optimizing the regex patterns or using alternative approaches for large datasets.
Documentation Improvements:
- The README provides a good overview but could benefit from additional details:
  - Examples of how to implement custom auditSink and rateLimiter.
  - Guidance on securely managing secrets.
  - Explanation of the scanTimeoutMs parameter and its impact on performance and security.
Versioning Strategy:
- The package is introduced as version 3.0.2, which might be confusing for users expecting a new package to start at 1.0.0.
- Action: Consider starting the version at 1.0.0 or provide a clear explanation for the versioning choice.

Summary

The PR introduces a useful and well-structured standalone package for MCP governance. However, there are critical security concerns related to credential redaction, regex safety, and secret management that must be addressed before merging. Additionally, there are potential breaking changes and areas for improvement in documentation, type safety, and performance optimization.

Action Items:

🔴 Address the critical issues related to credential redaction, ReDoS risks, and secret management.
🟡 Clearly document the API stability and provide a migration strategy for future changes.
💡 Improve type safety, concurrency handling, and documentation.

Once the critical issues are resolved and additional tests are added, this PR will be ready for merging.

jackbatzner · 2026-04-06T12:38:05Z

Addressing AI Reviewer Findings

"Credential Redaction Incompleteness" — Custom patterns are supported via the CredentialRedactor constructor. Built-in patterns cover PEM blocks (all key types), API keys, Bearer tokens, and connection strings. Extensibility is documented in the package README.

CodeQL ReDoS — Second round of regex hardening incoming to replace remaining polynomial patterns.

github-actions

🤖 AI Agent: code-reviewer

Pull Request Review: `feat(typescript): add standalone @microsoft/agentmesh-mcp-governance package`

🔴 CRITICAL

Regex Denial-of-Service (ReDoS) Vulnerability in CredentialRedactor
- The CredentialRedactor class uses regular expressions to identify sensitive data. Some of the patterns, such as pem_block and connection_string, are prone to catastrophic backtracking, which can lead to a denial-of-service (DoS) attack if a maliciously crafted input is provided.
- Actionable Fix: Use safe regex libraries like safe-regex to validate the regular expressions or refactor the regex patterns to avoid catastrophic backtracking.
Fail-Closed Behavior in McpGateway
- While the evaluateToolCall method attempts to fail-closed on errors, the finalize method may still allow partial or incomplete audit entries to be recorded. This could lead to inconsistent audit logs in case of runtime errors.
- Actionable Fix: Ensure that finalize explicitly logs any errors encountered during the audit process and provides a fallback mechanism to ensure audit integrity.
Insufficient Replay Protection in MCPMessageSigner
- The MCPMessageSigner class is mentioned in the README but not included in the diff. If it uses HMAC-SHA256 for signing payloads, ensure that it includes robust nonce handling and timestamp validation to prevent replay attacks.
- Actionable Fix: Review the implementation of MCPMessageSigner to confirm that it enforces strict nonce uniqueness and timestamp expiration checks.
Blocked Patterns in McpGateway
- The checkSanitization method in McpGateway uses both user-defined and built-in patterns to detect dangerous input. However, the built-in patterns include generic regexes like /\$$[^)]*$/g and /[^]+/g`, which may not cover all potential injection vectors.
- Actionable Fix: Expand the built-in dangerous patterns to include more comprehensive injection patterns (e.g., SQL injection, command injection). Consider integrating with a well-maintained library for detecting injection patterns.

🟡 WARNING

Backward Compatibility
- The introduction of a new standalone package @microsoft/agentmesh-mcp-governance is a non-breaking change, but it introduces a new public API. Any future changes to this package will need to maintain backward compatibility.
- Actionable Fix: Clearly document the API stability and versioning strategy for this package, especially since it is marked as a "Public Preview."
Potential for Misconfiguration
- The McpGateway class relies on user-provided configurations for critical security features like approvalHandler, rateLimiter, and policyEvaluator. Misconfiguration of these components could lead to security bypasses.
- Actionable Fix: Add validation for the McpGatewayConfig object to ensure that all required components are properly configured. For example, throw an error if approvalHandler is missing when sensitiveTools is defined.

💡 SUGGESTIONS

Type Safety Improvements
- The redact method in CredentialRedactor uses unknown as the type for input values. This reduces type safety when working with structured objects.
- Suggestion: Use a generic type parameter for the redact method to preserve type information. For example:
```
redact<T>(value: T): CredentialRedactionResult<T> {
  // Implementation
}
```
Performance Optimization for Regex Scans
- The createRegexScanBudget method is used to enforce a timeout for regex scans. However, this approach may introduce performance overhead in high-throughput scenarios.
- Suggestion: Consider using a streaming approach or pre-compiling regex patterns to improve performance.
Enhanced Documentation
- The README provides a good overview of the package but lacks detailed examples for advanced use cases, such as integrating with custom audit sinks or implementing durable rate-limit storage.
- Suggestion: Add a section to the README with advanced usage examples and best practices for production deployments.
Thread Safety
- The McpSlidingRateLimiter and InMemoryMCPAuditSink classes are mentioned but not included in the diff. If these classes use shared in-memory storage, they may not be thread-safe in concurrent environments.
- Suggestion: Ensure that all shared in-memory components are thread-safe or explicitly document that they are intended for single-threaded use only.
OWASP MCP Cheat Sheet Mapping
- The README mentions an OWASP MCP Cheat Sheet mapping but does not provide a direct link or detailed explanation.
- Suggestion: Include a link to the OWASP MCP Cheat Sheet and provide a table mapping the package's features to specific OWASP recommendations.

Summary

This pull request introduces a standalone @microsoft/agentmesh-mcp-governance package with a strong focus on modularity and security. While the implementation is generally robust, there are critical issues related to regex vulnerabilities, replay protection, and fail-closed behavior that must be addressed before merging. Additionally, there are opportunities to improve type safety, documentation, and performance.

Action Items:

Address the critical issues flagged above.
Add validation for user-provided configurations in McpGateway.
Expand the README with advanced usage examples and OWASP mappings.
Ensure thread safety for in-memory components.

Let me know if you need further clarification or assistance!

github-actions

🤖 AI Agent: code-reviewer

Pull Request Review: feat(typescript): add standalone @microsoft/agentmesh-mcp-governance package

Summary

This PR introduces a new standalone TypeScript package, @microsoft/agentmesh-mcp-governance, which provides MCP governance primitives without requiring the full Agent Mesh SDK. The package includes various security-related utilities such as credential redaction, rate limiting, payload signing, and gateway enforcement.

🔴 CRITICAL Issues

Regex Denial of Service (ReDoS) Vulnerability
- File: credential-redactor.ts and gateway.ts
- Issue: The use of regular expressions in BUILTIN_PATTERNS and BUILTIN_DANGEROUS_PATTERNS could lead to ReDoS attacks if the input is crafted maliciously. For example, patterns like /\bBearer\s+[A-Za-z0-9._\-+/=]{10,}\b/gi and /\b\d{4}[- ]?\d{4}[- ]?\d{4}[- ]?\d{4}\b/gi are susceptible to catastrophic backtracking.
- Recommendation: Use a library like safe-regex to validate the safety of regular expressions. Alternatively, consider using more efficient algorithms for pattern matching, such as DFA-based approaches.
Fail-Closed Behavior on Regex Timeout
- File: credential-redactor.ts and gateway.ts
- Issue: The createRegexScanBudget function enforces a timeout for regex scans, but the fallback behavior (content blocked or access denied) could lead to denial of service if legitimate inputs are incorrectly flagged.
- Recommendation: Log and monitor regex timeouts to identify patterns causing excessive scan times. Consider optimizing or replacing problematic regex patterns.
Insecure Default Configuration
- File: gateway.ts
- Issue: The McpGateway class defaults to an empty allowedTools list, which could lead to unintended tool calls being allowed if the configuration is incomplete.
- Recommendation: Default to a deny-all policy (allowedTools = []) and require explicit configuration for allowed tools.
Credential Redaction in Logs
- File: credential-redactor.ts
- Issue: The CredentialRedactor class redacts sensitive information but does not guarantee that all sensitive data is removed, especially if custom patterns are incomplete or misconfigured.
- Recommendation: Add a warning or validation mechanism for custom patterns to ensure they are comprehensive. Provide a way to test patterns against sample inputs.

🟡 WARNING: Potential Breaking Changes

Public API Instability
- File: README.md, package.json
- Issue: The package is marked as "Public Preview," and APIs may change before GA. This could lead to breaking changes for early adopters.
- Recommendation: Clearly document the expected stability of each API and provide a migration guide for future changes.
Behavior of McpGateway Policy Evaluator
- File: gateway.ts
- Issue: The policyEvaluator function can return undefined, which is treated as an implicit "allow." This behavior might lead to unexpected results if the evaluator fails to return a decision.
- Recommendation: Treat undefined as a failure and default to a fail-closed behavior unless explicitly configured otherwise.

💡 Suggestions for Improvement

Type Safety Enhancements
- File: credential-redactor.ts, gateway.ts
- Issue: Some methods, such as redactNode and safeRedactParams, use unknown types for input parameters, which can lead to runtime errors if the input is not validated.
- Recommendation: Use stricter TypeScript types or runtime type-checking libraries (e.g., io-ts or zod) for input validation.
Thread Safety
- File: gateway.ts
- Issue: The McpSlidingRateLimiter and InMemoryMCPAuditSink implementations may not be thread-safe, especially in multi-threaded environments.
- Recommendation: Document the thread safety assumptions and provide thread-safe alternatives for production use cases.
OWASP Agentic Top 10 Compliance
- File: gateway.ts
- Issue: The McpGateway class does not explicitly address all OWASP Agentic Top 10 risks, such as "Agent Impersonation" or "Exfiltration of Sensitive Data."
- Recommendation: Include specific mitigations for these risks, such as stronger identity verification mechanisms and data exfiltration detection.
Test Coverage
- File: jest.config.js
- Issue: The test configuration does not enforce a minimum coverage threshold.
- Recommendation: Add a coverageThreshold configuration to ensure critical components, such as McpGateway and CredentialRedactor, are thoroughly tested.
Documentation Improvements
- File: README.md
- Issue: The README does not provide detailed examples for all exported classes, such as McpResponseScanner and McpSecurityScanner.
- Recommendation: Include usage examples and best practices for each class to help users adopt the package securely.

Summary of Recommendations

🔴 Address ReDoS vulnerabilities in regex patterns.
🔴 Ensure fail-closed behavior for regex timeouts and policy evaluation.
🟡 Document API stability and provide migration guidance for future changes.
💡 Enhance type safety and runtime validation.
💡 Improve thread safety for rate limiter and audit sink implementations.
💡 Add minimum test coverage thresholds and expand documentation.

Let me know if you need further clarification or assistance!

packages/agent-mesh/packages/mcp-governance/src/utils.ts

packages/agent-mesh/packages/mcp-governance/tests/credential-redactor.test.ts

packages/agent-mesh/packages/mcp-governance/tests/gateway.test.ts

packages/agent-mesh/sdks/typescript/src/mcp-utils.ts

packages/agent-mesh/sdks/typescript/tests/credential-redactor.test.ts

packages/agent-mesh/sdks/typescript/tests/mcp-gateway.test.ts

github-actions

🤖 AI Agent: code-reviewer

Review Summary

This pull request introduces a new standalone TypeScript package, @microsoft/agentmesh-mcp-governance, which provides MCP governance primitives without requiring the full Agent Mesh SDK. The package includes several security-focused utilities such as credential redaction, rate limiting, HMAC signing, and policy enforcement. The implementation appears well-structured and adheres to best practices in many areas, but there are some critical security concerns, potential breaking changes, and areas for improvement.

🔴 CRITICAL

Regex Denial of Service (ReDoS) Vulnerability
- The CredentialRedactor and McpGateway classes use regular expressions for sensitive data redaction and pattern matching. Some of these regex patterns (e.g., SENSITIVE_KEY_PATTERN, BUILTIN_PATTERNS, and BUILTIN_DANGEROUS_PATTERNS) are complex and may be vulnerable to ReDoS attacks, especially when processing large inputs.
- Actionable Fix: Use a regex library that supports timeout or backtracking limits (e.g., safe-regex or re2), or pre-validate inputs to ensure they are within acceptable size limits before applying regex patterns.
Fail-Closed Behavior on Errors
- While the McpGateway class attempts to fail-closed on errors (e.g., in evaluateToolCall and finalize), there are no guarantees that all exceptions are properly caught. For example, if an unexpected error occurs in the approvalHandler or auditSink, it could lead to undefined behavior.
- Actionable Fix: Add a global error boundary or fallback mechanism to ensure that any uncaught exceptions result in a fail-closed decision.
Credential Redaction Incompleteness
- The CredentialRedactor class relies on predefined patterns (BUILTIN_PATTERNS) and sensitive key matching (SENSITIVE_KEY_PATTERN). However, this approach may miss custom or less common credential formats, leading to incomplete redaction.
- Actionable Fix: Allow users to configure additional patterns dynamically at runtime and provide a mechanism to log unredacted sensitive data for debugging in a secure environment.
Shell Expansion Detection
- The hasShellExpansion function in McpGateway only checks for $(...) syntax but does not account for other shell expansion patterns like ${...} or backticks (`...`).
- Actionable Fix: Expand the shell expansion detection logic to cover additional patterns and ensure comprehensive sanitization.

🟡 WARNING

Potential Breaking Changes in Public API
- The @microsoft/agentmesh-mcp-governance package introduces new APIs that may evolve before GA. While this is marked as a "Public Preview," any changes to these APIs in future releases could break consumers relying on the current implementation.
- Actionable Fix: Clearly document the API stability guarantees (e.g., semantic versioning policy) and provide deprecation warnings for any future changes.
Rate Limiter Behavior
- The McpSlidingRateLimiter enforces rate limits but does not provide a mechanism for distributed rate limiting across multiple processes or nodes. This could lead to inconsistent enforcement in distributed deployments.
- Actionable Fix: Add support for a distributed rate limiter (e.g., Redis-based) or document the limitations of the current implementation.

💡 SUGGESTIONS

Type Safety Enhancements
- The McpGateway and CredentialRedactor classes rely on Record<string, unknown> for parameters and redacted outputs. This could lead to runtime type errors if the input structure is not as expected.
- Suggestion: Use stricter TypeScript types or generics to enforce type safety for inputs and outputs.
OWASP MCP Compliance
- While the package provides several security features, it would be beneficial to explicitly map these features to OWASP MCP (Machine Control Plane) compliance requirements in the documentation.
- Suggestion: Add a section in the README or documentation that maps each feature to specific OWASP MCP guidelines.
Performance Optimization
- The CredentialRedactor and McpGateway classes perform multiple regex scans on potentially large inputs. This could impact performance in high-throughput scenarios.
- Suggestion: Benchmark the performance of these classes under realistic workloads and optimize regex patterns or introduce caching where applicable.
Test Coverage
- The package includes unit tests, but there is no mention of integration or end-to-end tests to validate the behavior of the package in real-world scenarios.
- Suggestion: Add integration tests that simulate common use cases, such as evaluating tool calls with complex parameters or handling high request volumes.
Backward Compatibility
- The @microsoft/agentmesh-mcp-governance package is designed to be standalone, but it re-exports primitives from the core TypeScript SDK. Any changes to the core SDK could inadvertently affect this package.
- Suggestion: Use semantic versioning constraints in the package.json file to ensure compatibility with specific versions of the core SDK.

Final Recommendation

Address the critical security issues (ReDoS vulnerability, fail-closed behavior, credential redaction completeness, and shell expansion detection) before merging.
Consider the potential breaking changes and document API stability guarantees.
Implement the suggested improvements to enhance type safety, performance, and test coverage.

Once the critical issues are resolved, this package will be a valuable addition to the microsoft/agent-governance-toolkit repository.

github-actions

🤖 AI Agent: code-reviewer

Pull Request Review: feat(typescript): add standalone @microsoft/agentmesh-mcp-governance package

🔴 CRITICAL Issues

Regex Denial-of-Service (ReDoS) Risk in Credential Redactor:
- The CredentialRedactor class uses regular expressions for sensitive data redaction. While validateRegex is called, there is no explicit check for catastrophic backtracking in user-provided customPatterns. This could lead to a denial-of-service (DoS) attack if a maliciously crafted regex is passed.
- Action: Add a stricter validation mechanism for user-provided regex patterns to ensure they are safe and do not exhibit catastrophic backtracking behavior.
Fail-Closed Behavior in Gateway:
- The McpGateway class has a fail-closed mechanism in case of errors (e.g., in evaluateToolCall and finalize). However, the error handling does not log the full stack trace or provide sufficient context for debugging.
- Action: Enhance error logging to include stack traces and additional context (e.g., agent ID, tool name, and stage) to aid in debugging while ensuring sensitive data is not exposed.
HMAC Key Handling:
- The MCPMessageSigner uses a secret key (MCP_SIGNING_SECRET) for HMAC-SHA256 signing. However, there is no validation to ensure the secret key meets minimum entropy requirements.
- Action: Add validation to ensure the secret key is sufficiently long (e.g., at least 32 bytes) and random to prevent brute-force attacks.
Sensitive Key Redaction:
- The CredentialRedactor class uses a hardcoded regex (SENSITIVE_KEY_PATTERN) to identify sensitive keys. This approach may miss certain key patterns or result in false positives.
- Action: Allow users to extend or override the SENSITIVE_KEY_PATTERN to better match their use cases.

🟡 WARNING: Potential Breaking Changes

Public API Stability:
- The package is marked as "Public Preview," and the README explicitly states that APIs may change before GA. This could lead to breaking changes for early adopters.
- Action: Clearly document any planned breaking changes in future releases and provide migration guides.
Rate Limiter Behavior:
- The McpSlidingRateLimiter enforces rate limits per agent. However, if the rate limit configuration changes (e.g., maxRequests or windowMs), it could lead to unexpected behavior for existing deployments.
- Action: Document the implications of changing rate limit configurations and provide guidelines for safely updating these settings.

💡 Suggestions for Improvement

TypeScript Type Safety:
- The evaluateToolCall method in McpGateway accepts params as Record<string, unknown>. This type is too generic and could lead to runtime errors if the structure of params is not as expected.
- Suggestion: Define a stricter TypeScript interface for params and validate it at runtime using a library like zod or io-ts.
OWASP MCP Cheat Sheet Mapping:
- The README mentions an OWASP MCP Cheat Sheet mapping but does not provide a detailed explanation of how the package aligns with OWASP guidelines.
- Suggestion: Include a dedicated section in the documentation that maps each feature (e.g., MCPResponseScanner, MCPSessionAuthenticator) to specific OWASP recommendations.
Thread Safety:
- The CredentialRedactor and McpGateway classes do not appear to use any thread-safe mechanisms. While JavaScript is single-threaded, Node.js environments with worker threads or clustered processes could encounter race conditions.
- Suggestion: Document the thread safety assumptions and provide guidance for using these classes in multi-threaded environments.
Enhanced Test Coverage:
- While the PR includes tests, there is no explicit mention of coverage for edge cases (e.g., malformed input, regex timeouts, or invalid configurations).
- Suggestion: Add tests for edge cases, including:
  - Invalid or malicious regex patterns.
  - Extremely large or deeply nested objects in CredentialRedactor.
  - Rate limiter behavior under high concurrency.
Performance Optimization:
- The CredentialRedactor processes strings and objects recursively. For deeply nested objects, this could lead to performance bottlenecks.
- Suggestion: Implement a depth limit or optimize the recursion to handle large objects more efficiently.
Durable Storage for Production:
- The README mentions that "production deployments should provide durable implementations for session, nonce, rate-limit, and audit storage." However, no examples or guidelines are provided.
- Suggestion: Include examples or references to durable storage implementations (e.g., Redis, DynamoDB) in the documentation.
Security Scanner Enhancements:
- The MCPSecurityScanner is mentioned in the README but not included in the diff. If it uses regex patterns for scanning, it may also be susceptible to ReDoS attacks.
- Suggestion: Apply the same regex validation improvements to the MCPSecurityScanner.

Summary

This PR introduces a standalone TypeScript package for MCP governance primitives, which is a valuable addition to the project. However, there are critical security issues related to regex validation, error handling, and key management that need to be addressed before merging. Additionally, there are potential breaking changes and areas for improvement in type safety, documentation, and performance.

Recommended Actions Before Merging:

Address the critical issues related to regex validation, error handling, and key management.
Document the implications of API changes and rate limiter configurations.
Add tests for edge cases and performance bottlenecks.

Once these issues are resolved, the package will be a robust and secure addition to the project.

github-actions

🤖 AI Agent: code-reviewer

Pull Request Review: `feat(typescript): add standalone @microsoft/agentmesh-mcp-governance package`

Summary

This PR introduces a new standalone TypeScript package, @microsoft/agentmesh-mcp-governance, which provides MCP governance primitives without requiring the full Agent Mesh SDK. The package includes various security-related utilities such as credential redaction, rate limiting, payload signing, and policy enforcement. The PR also includes configuration files for ESLint, Jest, and TypeScript, as well as a README with usage instructions.

🔴 CRITICAL Issues

Regex Denial of Service (ReDoS) Vulnerability in CredentialRedactor
- Issue: The CredentialRedactor class uses regular expressions to identify sensitive information in strings. Some of the patterns, such as pem_block and connection_string, are vulnerable to ReDoS attacks due to their use of nested quantifiers and unbounded repetition.
- Impact: An attacker could craft malicious input that causes these regex patterns to exhibit exponential backtracking, leading to high CPU usage and potential denial of service.
- Recommendation: Use safer regex patterns or implement a timeout mechanism for regex execution. Consider using libraries like safe-regex to validate patterns for potential ReDoS vulnerabilities.
Potential Credential Exposure in Audit Logs
- Issue: The McpGateway class logs audit entries with redacted parameters. However, if the CredentialRedactor fails to redact sensitive information due to a missed pattern or a failure in the regex scan, credentials could be logged in plaintext.
- Impact: This could lead to sensitive information being exposed in audit logs, violating security best practices.
- Recommendation: Add a fail-safe mechanism to ensure that no sensitive information is logged, even if redaction fails. For example, replace all unredacted parameters with a generic placeholder.
Fail-Closed Behavior in McpGateway
- Issue: While the McpGateway class attempts to fail closed in case of errors, there are scenarios where exceptions in the finalize method could lead to incomplete audit logging or metrics recording.
- Impact: This could result in incomplete security monitoring and auditing, reducing the effectiveness of the governance system.
- Recommendation: Ensure that all critical operations in the finalize method are wrapped in try-catch blocks and that fallback mechanisms are in place to handle errors gracefully.

🟡 WARNING Issues

Backward Compatibility
- Issue: The introduction of a new standalone package does not directly affect existing APIs, but any future changes to the MCP primitives in the core SDK could break this package.
- Impact: Changes to the core SDK could inadvertently introduce breaking changes to this package, affecting users who rely on it.
- Recommendation: Clearly document the version compatibility between this package and the core SDK. Consider implementing integration tests to ensure compatibility.
Public Preview API Stability
- Issue: The README explicitly states that the package is in "Public Preview" and that APIs may change before GA.
- Impact: Users adopting this package in production environments may face breaking changes in future releases.
- Recommendation: Clearly communicate the stability level of each API and provide a migration guide for any breaking changes.

💡 Suggestions

Enhanced Documentation
- The README is well-written but could benefit from additional details:
  - Provide a more detailed explanation of each exported primitive, including examples of real-world use cases.
  - Include a section on how to implement custom durable storage for production deployments.
  - Add a security considerations section to highlight best practices for using the package securely.
Test Coverage
- While the PR mentions that tests have been added, it is unclear whether all critical paths (e.g., regex redaction, rate limiting, and approval handling) are thoroughly tested.
- Recommendation: Ensure that the test suite includes edge cases, such as:
  - Malicious inputs designed to exploit ReDoS vulnerabilities.
  - Scenarios where the CredentialRedactor fails to redact sensitive information.
  - Failures in the finalize method of McpGateway.
TypeScript Typing Improvements
- The CredentialRedactor class uses a generic T type for the redact method but does not enforce any constraints on T.
- Recommendation: Consider adding constraints to the generic type to ensure type safety. For example:
```
redact<T extends object | string>(value: T): CredentialRedactionResult<T> { ... }
```
Performance Optimization
- The CredentialRedactor class uses a naive approach to iterate over patterns and match them against the input. This could lead to performance bottlenecks for large inputs or a large number of patterns.
- Recommendation: Explore more efficient algorithms or libraries for pattern matching and redaction, such as using a single combined regex or a trie-based approach.
Thread Safety
- The McpGateway class uses shared instances of CredentialRedactor and McpSlidingRateLimiter. If these classes are not thread-safe, concurrent access could lead to race conditions.
- Recommendation: Ensure that all shared resources are thread-safe. If necessary, use synchronization mechanisms or create separate instances for each thread.
OWASP Agentic Top 10 Compliance
- The package addresses several OWASP Agentic Top 10 risks, such as improper access control and insufficient logging. However, it is unclear if all risks are fully mitigated.
- Recommendation: Perform a comprehensive review of the package against the OWASP Agentic Top 10 to identify and address any gaps.

Summary of Actions

🔴 CRITICAL

Address potential ReDoS vulnerabilities in regex patterns.
Implement a fail-safe mechanism to prevent credential exposure in audit logs.
Ensure robust fail-closed behavior in the McpGateway class.

🟡 WARNING

Document version compatibility and provide a migration guide for future changes.
Clearly communicate the stability level of the API during the public preview phase.

💡 SUGGESTIONS

Enhance the README with more detailed documentation and security considerations.
Expand test coverage to include edge cases and failure scenarios.
Improve TypeScript type safety for the CredentialRedactor class.
Optimize the performance of the CredentialRedactor class.
Ensure thread safety for shared resources in the McpGateway class.
Conduct a full review for OWASP Agentic Top 10 compliance.

Let me know if you need further clarification or assistance!

github-actions

🤖 AI Agent: code-reviewer

Pull Request Review: feat(typescript): add standalone @microsoft/agentmesh-mcp-governance package

Summary

This PR introduces a standalone @microsoft/agentmesh-mcp-governance npm package, which provides MCP governance primitives without requiring the full Agent Mesh SDK. The package includes features like credential redaction, rate limiting, HMAC signing, and gateway enforcement. Below is a detailed review of the changes.

🔴 CRITICAL

1. Potential Regex Denial of Service (ReDoS) in CredentialRedactor

Issue: The CredentialRedactor class uses regex patterns for credential redaction, including user-defined patterns. While validateRegex is called to validate patterns, the implementation of validateRegex is not provided in the diff. If this function does not adequately check for catastrophic backtracking or excessive backtracking, the regex patterns could be exploited for a ReDoS attack.
Impact: An attacker could craft malicious input that causes the regex engine to consume excessive CPU resources, leading to a denial-of-service condition.
Recommendation: Ensure that validateRegex includes checks for catastrophic backtracking and enforces a reasonable complexity limit on user-provided regex patterns. Additionally, consider using a library like safe-regex to validate regex patterns.

2. Fail-Closed Behavior in `McpGateway`

Issue: The McpGateway class has a fail-closed mechanism in case of errors during policy evaluation or finalization. While this is a good practice, the error messages returned in such cases (e.g., "Internal error - access denied (fail-closed)") may leak implementation details or provide attackers with hints about the system's behavior.
Impact: Leaking internal error details can aid attackers in crafting targeted attacks.
Recommendation: Replace error messages with generic ones that do not reveal internal details. For example, "An error occurred. Access denied." Additionally, ensure that all sensitive information is redacted from error messages.

🟡 WARNING

1. Backward Compatibility

Issue: The PR introduces a new package, @microsoft/agentmesh-mcp-governance, which re-exports MCP primitives from the core TypeScript SDK. While this is a new package and does not directly modify existing ones, any future changes to the core SDK could inadvertently break this package if not carefully managed.
Impact: Changes to the core SDK could break the standalone package, leading to potential runtime errors for users.
Recommendation: Implement a robust versioning strategy and add automated tests to ensure compatibility between the core SDK and the standalone package. Consider using semantic versioning to communicate breaking changes clearly.

💡 SUGGESTIONS

1. Improved Documentation

Observation: The README for the new package is comprehensive, but it could benefit from additional details on the following:
- Examples of how to implement custom durable storage for production deployments.
- A more detailed explanation of the approvalHandler function and its expected behavior.
- A section on security best practices for using the package, such as securely managing secrets and configuring rate limits.
Recommendation: Expand the documentation to include the above details, as they are critical for secure and effective usage of the package.

2. Thread Safety in `McpGateway`

Observation: The McpGateway class uses shared instances of CredentialRedactor, McpSlidingRateLimiter, and other components. While these components appear to be stateless or thread-safe, this assumption should be explicitly verified.
Recommendation: Review the implementation of shared components like CredentialRedactor and McpSlidingRateLimiter to ensure they are thread-safe. If they are not, consider adding synchronization mechanisms or creating separate instances for each thread.

3. Testing Coverage

Observation: The PR includes a Jest configuration for testing, but the diff does not include any test cases for the new package.
Recommendation: Add comprehensive test cases for the new package, covering:
- All public APIs.
- Edge cases for credential redaction (e.g., nested objects, large inputs, and user-defined regex patterns).
- Fail-closed behavior in McpGateway.
- Rate-limiting logic.
- Approval handler scenarios.

4. HMAC Key Management

Observation: The MCPMessageSigner uses a secret key for HMAC-SHA256 signing. However, there is no mention of how this key should be securely managed or rotated.
Recommendation: Provide guidance in the documentation on securely managing and rotating the HMAC key. Consider integrating with a secure key management solution (e.g., Azure Key Vault).

5. Default Configuration for `McpGateway`

Observation: The McpGateway constructor allows for extensive customization, but the default configuration may not be secure for production use (e.g., no rate limiter, no blocked patterns).
Recommendation: Provide a secure default configuration that includes reasonable rate limits, a set of blocked patterns, and a fail-closed policy evaluator.

6. Logging and Metrics

Observation: The McpGateway class uses a logger and metrics object for logging and telemetry. However, the implementation of these components is not included in the diff.
Recommendation: Ensure that the logger and metrics implementations are secure and do not log sensitive information. Add tests to verify this behavior.

Summary of Review

This PR introduces a valuable addition to the project by providing a standalone package for MCP governance primitives. However, there are critical security concerns related to regex validation and error handling that must be addressed before merging. Additionally, there are opportunities to improve documentation, testing, and default configurations to enhance the package's usability and security.

Action Items

Address Critical Issues:
- Ensure regex patterns are safe from ReDoS attacks.
- Replace error messages with generic ones to avoid leaking internal details.
Resolve Warnings:
- Implement a robust versioning strategy to ensure backward compatibility.
Implement Suggestions:
- Expand documentation with security best practices and implementation details.
- Add comprehensive test cases for all new functionality.
- Review thread safety of shared components.
- Provide guidance on HMAC key management.
- Define a secure default configuration for McpGateway.

Once these issues are addressed, the PR will be ready for merging.

jackbatzner · 2026-04-06T15:39:20Z

CI Status Summary ✅

All CI checks pass except:

Maintainer approval gate — awaiting review
CodeQL status gate — stale fork artifact (Analyze jobs pass)
check-version-pinning — pre-existing issue (fix(vscode): pin dependency versions in agent-os-vscode package.json #831)

Standalone @microsoft/agentmesh-mcp-governance npm package. Slimmed to 25 files.

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

github-actions

🤖 AI Agent: code-reviewer

Code Review for `@microsoft/agentmesh-mcp-governance` Package

General Observations

This PR introduces a standalone TypeScript package, @microsoft/agentmesh-mcp-governance, which provides MCP governance primitives without requiring the full Agent Mesh SDK. The package includes utilities for authentication, signing, redaction, scanning, rate limiting, and gateway enforcement. The implementation appears well-structured, with a focus on modularity and extensibility.

🔴 CRITICAL

Regex Denial of Service (ReDoS) Vulnerability:
- The CredentialRedactor and McpGateway classes use regular expressions for pattern matching (e.g., BUILTIN_PATTERNS and BUILTIN_DANGEROUS_PATTERNS). Some of these patterns (e.g., pem_block, connection_string, dangerous_pattern) are complex and could be susceptible to ReDoS attacks if they are applied to large or maliciously crafted inputs.
  - Recommendation: Use libraries like safe-regex to validate regex patterns for safety. Additionally, consider limiting the size of inputs processed by these regexes to mitigate potential performance issues.
Fail-Closed Behavior in McpGateway:
- While the McpGateway class generally adheres to fail-closed principles, the safeRedactParams method catches errors during redaction and returns an empty object ({}) as a fallback. This could lead to a loss of critical information in the audit logs.
  - Recommendation: Instead of returning an empty object, log the error and include a sanitized version of the parameters (e.g., with sensitive fields redacted) to ensure audit completeness.
Credential Leakage in Audit Logs:
- The auditSink implementation logs redacted parameters, but there is no guarantee that custom auditSink implementations provided by users will handle sensitive data securely.
  - Recommendation: Document the importance of redacting sensitive data before passing it to the auditSink. Consider providing a default secure implementation or validating user-provided auditSink implementations to ensure compliance.
Insufficient Protection Against Replay Attacks:
- The MCPMessageSigner class (referenced in the README) mentions HMAC-SHA256 payload signing with timestamp and nonce replay protection. However, the implementation of nonce storage and validation is not included in this PR.
  - Recommendation: Ensure that nonce storage and validation are implemented securely, with a mechanism to prevent replay attacks. This is critical for maintaining the integrity of signed messages.
Shell Expansion Detection:
- The hasShellExpansion function only checks for the presence of $( and ). This simplistic approach may result in false negatives or positives.
  - Recommendation: Use a more robust method to detect shell expansion patterns, such as leveraging a shell parser or a more comprehensive regex.

🟡 WARNING

Potential Breaking Changes:
- The package is introduced as a standalone module with no dependencies on the full Agent Mesh SDK. While this is a non-breaking change for existing users of the SDK, it introduces a new package that may require separate maintenance and versioning. Ensure that this is clearly communicated to users, especially if there are plans to deprecate overlapping functionality in the main SDK.
API Stability:
- The README explicitly states that this package is in "Public Preview" and that APIs may change before GA. This could lead to breaking changes for early adopters.
  - Recommendation: Consider versioning the package as 0.x.x to signal that it is not yet stable, or use a clear versioning strategy to indicate breaking changes.

💡 SUGGESTIONS

Improved Documentation:
- The README provides a good overview of the package, but it could benefit from additional details:
  - Add a section on "Security Best Practices" to guide users on securely configuring and using the package.
  - Provide examples of how to implement custom auditSink and rateLimiter to ensure compliance with security best practices.
  - Include a section on how to handle errors and exceptions, especially in fail-closed scenarios.
Rate Limiter Extensibility:
- The McpSlidingRateLimiter is used as the default rate limiter. While this is a reasonable choice, the package could benefit from additional rate-limiting strategies (e.g., token bucket, fixed window).
  - Recommendation: Consider making the rate limiter interface more extensible to allow users to plug in custom implementations.
Test Coverage:
- The PR includes a jest.config.js file and mentions test coverage, but the actual test cases are not included in the diff.
  - Recommendation: Ensure comprehensive test coverage for all critical components, especially the CredentialRedactor and McpGateway classes. Include tests for edge cases, such as large inputs, malformed data, and failure scenarios.
TypeScript Typing:
- The TypeScript typings appear robust, but some areas could benefit from stricter type definitions:
  - The params argument in evaluateToolCall is typed as Record<string, unknown>. Consider using a stricter type or a generic type parameter to enforce structure.
  - The McpGatewayConfig type could include stricter definitions for policyEvaluator, approvalHandler, and auditSink to ensure type safety.
Performance Optimization:
- The redactNode method in CredentialRedactor uses recursion to traverse nested objects. This could lead to stack overflow errors for deeply nested structures.
  - Recommendation: Replace recursion with an iterative approach using a stack or queue to handle deeply nested structures more robustly.
Default Configuration:
- The McpGateway class uses default configurations for several components (e.g., rateLimiter, metrics). While this is convenient, it may lead to unexpected behavior if users are unaware of the defaults.
  - Recommendation: Document the default configurations in the README and provide examples of how to override them.
Error Handling:
- The McpGateway class uses getSafeErrorMessage to sanitize error messages. However, the implementation of getSafeErrorMessage is not included in the diff.
  - Recommendation: Review the implementation of getSafeErrorMessage to ensure it does not inadvertently expose sensitive information.
Backward Compatibility:
- While this package is standalone, it re-exports MCP primitives from the core TypeScript SDK. Ensure that any changes to the core SDK do not inadvertently break this package.
  - Recommendation: Add integration tests to verify compatibility between this package and the core SDK.

Summary

This PR introduces a valuable addition to the Agent Mesh ecosystem, providing standalone MCP governance primitives. While the implementation is generally robust, there are critical security concerns (e.g., ReDoS vulnerabilities, replay attack protection) and areas for improvement (e.g., documentation, test coverage, type safety). Addressing these issues will enhance the security, reliability, and usability of the package.

github-actions

🤖 AI Agent: code-reviewer

Pull Request Review for `feat(typescript): add standalone @microsoft/agentmesh-mcp-governance package`

🔴 CRITICAL

Regex Denial of Service (ReDoS) Vulnerability in CredentialRedactor
- The CredentialRedactor class uses regular expressions to match sensitive data patterns. Some of these patterns, such as SENSITIVE_KEY_PATTERN and BUILTIN_PATTERNS, may be vulnerable to ReDoS attacks if they are applied to large or maliciously crafted inputs. For example:
```
const SENSITIVE_KEY_PATTERN = /(password|passwd|pwd|secret|token|api[_-]?key|connection.?string|accountkey|sharedaccesssignature|sas)/i;
```
  - This pattern could potentially be exploited with inputs designed to cause catastrophic backtracking.
- Action: Use safe regex libraries like safe-regex to validate these patterns. Alternatively, consider using a library like re2 for regex matching, which avoids catastrophic backtracking.
Fail-Closed Behavior in McpGateway
- The evaluateToolCall method in McpGateway has a fail-closed mechanism for errors during evaluation. While this is a good practice, the error handling in finalize does not log the error details or provide sufficient information for debugging:
```
debugSecurityFailure(this.config.logger, 'gateway.finalize', error);
```
  - If debugSecurityFailure does not log the full stack trace or error details, it could make debugging difficult.
- Action: Ensure that debugSecurityFailure logs the full error stack trace and relevant context for easier debugging.
Potential Bypass of McpGateway Policies
- The evaluateToolCall method relies on a policyEvaluator function to make policy decisions. However, the implementation does not validate the policyEvaluator's output beyond checking if it equals 'deny' or 'review'. If the policyEvaluator returns an unexpected value, it could lead to undefined behavior.
```
const policyDecision = await this.config.policyEvaluator?.evaluate(toolName, {
  agentId,
  ...params,
});
if (policyDecision === 'deny') {
  ...
}
```
- Action: Validate the output of policyEvaluator to ensure it only returns expected values ('allow', 'deny', or 'review'). Log a warning or error if an unexpected value is returned.
Insufficient Validation of blockedPatterns
- The blockedPatterns array in McpGatewayConfig accepts both strings and regular expressions. However, the checkSanitization method does not validate string patterns, which could lead to unexpected behavior or security issues.
```
if ((typeof pattern === 'string' && serialized.includes(pattern)) || (pattern instanceof RegExp && hasMatch(pattern, serialized))) {
```
- Action: Validate string patterns in blockedPatterns to ensure they are safe and do not introduce vulnerabilities.

🟡 WARNING

Backward Compatibility
- This PR introduces a new package, @microsoft/agentmesh-mcp-governance, which re-exports MCP primitives from the core TypeScript SDK. While this is a new package and does not directly modify existing functionality, any changes to the core SDK's MCP primitives could inadvertently break this package.
- Action: Ensure that changes to the core SDK are thoroughly tested against this new package to maintain backward compatibility.

💡 SUGGESTIONS

Documentation Improvements
- The README provides a good overview of the package, but it could benefit from additional details:
  - Add a section on how to configure the McpGateway class, including examples of McpGatewayConfig options like policyEvaluator, rateLimiter, and approvalHandler.
  - Provide more detailed examples of how to use each exported primitive (e.g., CredentialRedactor, MCPMessageSigner).
Unit Test Coverage
- While the PR mentions that tests have been added, the provided diff does not include test files. It is crucial to ensure that all critical components, such as CredentialRedactor and McpGateway, are thoroughly tested for edge cases, especially for security-related functionality.
- Action: Add unit tests for:
  - Edge cases in regular expression matching (e.g., large inputs, malicious patterns).
  - Fail-closed behavior in evaluateToolCall and finalize.
  - Validation of policyEvaluator output.
  - Handling of blockedPatterns with both strings and regular expressions.
Thread Safety
- The McpGateway class uses shared resources like rateLimiter and auditSink. If these resources are not thread-safe, it could lead to race conditions in concurrent environments.
- Action: Ensure that shared resources like rateLimiter and auditSink are thread-safe. If they are not, consider using locks or other synchronization mechanisms.
Type Safety
- The safeRedactParams method casts the result of this.redactor.redact(params).redacted to Record<string, unknown> without verifying its type.
```
return this.redactor.redact(params).redacted as Record<string, unknown>;
```
- Action: Add runtime type checks to ensure that redacted is a valid Record<string, unknown> before casting.
Default Configuration Values
- The McpGateway constructor sets default values for some configuration options (e.g., deniedTools, allowedTools, sensitiveTools). However, these defaults may not align with the security requirements of all use cases.
- Action: Consider providing more secure defaults, such as an empty allowedTools list and a non-empty deniedTools list, to encourage fail-closed behavior by default.
Rate Limiter Configuration
- The McpSlidingRateLimiter is instantiated with a default configuration if rateLimit is provided. However, there is no validation of the rateLimit configuration.
- Action: Validate the rateLimit configuration to ensure it has reasonable values (e.g., maxRequests > 0, windowMs > 0).

Summary

This PR introduces a standalone @microsoft/agentmesh-mcp-governance package with useful MCP governance primitives. While the implementation is generally solid, there are critical security issues related to regular expression usage, policy evaluation, and parameter sanitization. Addressing these issues and incorporating the suggested improvements will enhance the security, reliability, and usability of the package.

…package Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…alse positive Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

github-actions

🤖 AI Agent: code-reviewer

Pull Request Review for `@microsoft/agentmesh-mcp-governance`

🔴 CRITICAL: Security Issues

Regex Denial of Service (ReDoS) Vulnerability
- File: credential-redactor.ts, gateway.ts
- Issue: The use of unbounded regular expressions (e.g., SENSITIVE_KEY_PATTERN, BUILTIN_PATTERNS, and BUILTIN_DANGEROUS_PATTERNS) introduces a potential ReDoS vulnerability. If an attacker crafts malicious input that causes excessive backtracking, it could lead to performance degradation or denial of service.
- Recommendation: Use safe regex patterns or libraries like safe-regex to validate patterns. Alternatively, consider limiting the input size or using non-backtracking regex engines.
Shell Injection Risk
- File: gateway.ts
- Issue: The hasShellExpansion function checks for shell expansion patterns like $(...). However, this is insufficient to prevent command injection attacks. For example, backticks (`) are also used for shell commands in some contexts.
- Recommendation: Extend the hasShellExpansion function to detect additional shell injection patterns, such as backticks and other command execution syntax. Consider using a library like shell-escape to sanitize inputs.
Fail-Closed Behavior
- File: gateway.ts
- Issue: While the evaluateToolCall method attempts to fail closed on errors, the finalize method also contains error handling that defaults to denial. This redundancy could lead to inconsistent behavior if the finalize method fails.
- Recommendation: Ensure that the fail-closed behavior is consistent across all error handling paths. Consider centralizing error handling logic to avoid discrepancies.
HMAC-SHA256 Key Management
- File: README.md, gateway.ts
- Issue: The MCPMessageSigner and MCPSessionAuthenticator rely on secrets (MCP_SESSION_SECRET and MCP_SIGNING_SECRET) provided via environment variables. However, there is no mention of secure key rotation or storage mechanisms.
- Recommendation: Document secure key management practices, including rotation, storage in a secure vault (e.g., Azure Key Vault), and periodic audits.

🟡 WARNING: Potential Breaking Changes

API Stability
- File: README.md, package.json
- Issue: The package is marked as "Public Preview," and the APIs may change before GA. This could lead to breaking changes for early adopters.
- Recommendation: Clearly communicate the API stability status in the documentation and consider versioning strategies (e.g., semantic versioning) to minimize disruption.

💡 Suggestions for Improvement

Type Safety
- File: credential-redactor.ts, gateway.ts
- Observation: The use of unknown and any types in methods like redactNode and safeRedactParams reduces type safety.
- Recommendation: Use stricter TypeScript types or generics to enforce type safety. For example, replace unknown with a well-defined interface or type.
Performance Optimization
- File: credential-redactor.ts
- Observation: The redactNode method uses recursive traversal for nested objects, which could lead to stack overflow for deeply nested structures.
- Recommendation: Use an iterative approach or limit the recursion depth to prevent stack overflow.
Logging and Auditing
- File: gateway.ts
- Observation: The finalize method logs audit entries but does not include a mechanism for log rotation or retention.
- Recommendation: Implement log rotation and retention policies to comply with security best practices and avoid excessive log growth.
Documentation
- File: README.md
- Observation: The README provides a good overview but lacks detailed examples for advanced use cases like custom patterns or durable storage implementations.
- Recommendation: Add examples for advanced configurations and integration with production-grade storage systems.
Testing Coverage
- File: jest.config.js
- Observation: The collectCoverageFrom configuration only includes src/**/*.ts. This may exclude utility files or edge cases.
- Recommendation: Ensure comprehensive test coverage, including edge cases and error handling paths.

Summary

This pull request introduces a standalone @microsoft/agentmesh-mcp-governance package with a focus on MCP governance primitives. While the implementation is robust and adheres to many best practices, there are critical security issues (e.g., ReDoS, shell injection) and areas for improvement (e.g., type safety, documentation). Addressing these concerns will enhance the package's security, reliability, and usability.

Critical Issues: 4
Warnings: 1
Suggestions: 5

Please address the 🔴 CRITICAL issues before merging.

imran-siddique · 2026-04-09T16:32:13Z

Thank you for this work, @jackbatzner — the MCP governance implementation across multiple languages is impressive and clearly well-thought-out.

After reviewing the full series, we've decided to take a hybrid approach (Decision C): keep MCP core logic in agent-os/agent-mesh and provide thin standalone re-export packages for enterprise repackaging — similar to what we shipped in PR #910 for Python.

We're closing this PR because we'll implement the TypeScript standalone package in-house following this architecture. Your work directly informed our approach — the OWASP mapping methodology, the gateway/rate-limiter/session-auth pattern, and the trust proxy design all carry forward.

We're keeping your documentation PRs open for review as they add independent value. Thank you for the sustained contribution effort across the entire MCP governance surface.

github-actions bot added documentation Improvements or additions to documentation tests agent-mesh agent-mesh package size/XL Extra large PR (500+ lines) labels Apr 6, 2026