feat(go): add MCP protocol support with full OWASP coverage

[jackbatzner]

Description

Add MCP (Model Context Protocol) security primitives to the Go SDK (packages/agent-mesh/sdks/go), achieving parity with the Python, .NET, TypeScript, and Rust implementations. Covers 11/12 sections of the OWASP MCP Security Cheat Sheet (§11 Consent UI is out of scope for server-side SDKs).

Key Components

• MCPMessageSigner — HMAC-SHA256 message signing with 32-byte minimum key enforcement and nonce replay protection
• MCPSessionAuthenticator — TOCTOU-safe session binding with configurable TTL and concurrent-safe token management
• MCPRateLimiter — Per-tool sliding-window rate limiting with fail-closed behavior
• MCPInputSanitizer — Prompt injection detection, path traversal blocking, and SQL injection prevention
• MCPResponseScanner — Credential detection, PII scanning, and full PEM block redaction
• MCPGateway — Unified governance pipeline composing all security primitives
• CredentialRedactor — Pattern-based credential redaction with extensible custom patterns
• MCPMetrics — OpenTelemetry-based metrics with cardinality-guarded tool-name attributes

Enterprise Patterns

• Persistence seams (interface abstractions with in-memory defaults)
• Clock/nonce injection (no hardcoded time calls)
• Fail-closed enforcement on all security gates
• Thread safety via sync.RWMutex on all shared state
• Race-condition tested (go test -race)

Type of Change

• New feature (non-breaking change that adds functionality)

Package(s) Affected

• packages/agent-mesh/sdks/go

Checklist

• I have read the CONTRIBUTING guide
• My code follows the style guidelines of this project
• I have added tests that prove my fix/feature works
• New and existing tests pass locally
• I have added MIT license headers to all new source files
• My changes generate no new warnings

Stacked PRs

This is PR 1 of 3 (Core). Merge order:

1. This PR — Core MCP primitives
2. Standalone package wrapper (targets jb/go-mcp-package)
3. Documentation and examples (targets jb/go-mcp-docs)

Related Issues

Part of the multi-language MCP OWASP parity effort alongside #774, #775, #791, #796.

[github-actions]

🤖 AI Agent: code-reviewer

Review Summary

This PR introduces the MCP (Model Context Protocol) security primitives to the Go SDK, achieving parity with other language implementations. It aims to address 11 out of 12 sections of the OWASP MCP Security Cheat Sheet and includes robust security features such as HMAC-SHA256 message signing, nonce replay protection, rate limiting, input sanitization, and response scanning. The implementation appears well-structured, with a focus on security, thread safety, and fail-closed mechanisms. The provided tests cover a variety of scenarios, including edge cases like timeouts and invalid inputs.

However, there are a few areas that require attention to ensure the robustness, security, and maintainability of the code. Below are the detailed findings:

---

🔴 CRITICAL

1. Insufficient Key Management for HMAC-SHA256

   • The McpMessageSigner is initialized with a hardcoded key ("0123456789abcdef0123456789abcdef") in the NewMcpGateway function:

     signer, err := NewMcpMessageSigner(McpMessageSignerConfig{Key: []byte("0123456789abcdef0123456789abcdef")})

     This is a critical security issue as hardcoding cryptographic keys in source code is a direct violation of secure coding practices. Keys should be securely stored and retrieved from a secure key management system (KMS) or environment variables.
     • Action: Replace the hardcoded key with a mechanism to securely fetch keys from a KMS or environment variables. Ensure the key is rotated periodically.

2. Potential Regular Expression Denial of Service (ReDoS)

   • The buildMcpRedactionPatterns function compiles multiple regular expressions for credential redaction. Some of these patterns (e.g., (?s)-----BEGIN RSA PRIVATE KEY-----.*?-----END RSA PRIVATE KEY-----) are vulnerable to ReDoS attacks due to their use of unbounded quantifiers (.*?) in combination with the (?s) flag.
     • Action: Audit and optimize all regular expressions to ensure they are not vulnerable to ReDoS attacks. Consider using bounded quantifiers or alternative patterns where possible. Additionally, ensure that the regex timeout (RegexTimeout) is enforced consistently across all regex operations.

3. Potential Sandbox Escape via Tool Description

   • The InterceptToolCall function evaluates the ToolDescription field of the McpToolCallRequest for threats. However, there is no explicit validation or sanitization of this field before it is passed to the scanner.ScanTool method.
     • Action: Add explicit validation and sanitization for the ToolDescription field to prevent potential injection attacks, such as HTML/JavaScript injection or command injection.

4. Concurrency Issue in SignerNonceStore

   • The SignerNonceStore implementation (partially shown in the truncated diff) uses a sync.Mutex for thread safety. However, the Reserve and Cleanup methods are not shown in the diff, and it is unclear if they are implemented in a thread-safe manner.
     • Action: Review the SignerNonceStore implementation to ensure that all access to shared state is properly synchronized. Use sync.RWMutex where appropriate to optimize read-heavy workloads.

---

🟡 WARNING

1. Backward Compatibility
   • The addition of the MCP primitives introduces new public APIs (e.g., McpGateway, CredentialRedactor, McpMessageSigner). While these are new features, any changes to these APIs in the future could break backward compatibility.
     • Action: Clearly document the new APIs and mark them as stable or experimental, depending on their maturity. Consider versioning the SDK to signal compatibility guarantees.

---

💡 SUGGESTIONS

1. Improved Test Coverage

   • While the tests cover a variety of scenarios, additional tests could be added to ensure comprehensive coverage:
     • Test cases for edge cases in CredentialRedactor, such as empty input strings or inputs with no matches.
     • Stress tests for McpGateway to simulate high-concurrency scenarios and ensure thread safety.
     • Tests for the SignerNonceStore implementation to verify nonce expiration and cleanup behavior.

2. Error Handling Consistency

   • The error handling in the InterceptToolCall function is robust, but there are some inconsistencies in how errors are logged and returned. For example, some errors are wrapped with fmt.Errorf, while others are not.
     • Action: Standardize error handling across the codebase. Consider using a structured error type or a logging library to ensure consistent error reporting.

3. Configuration Defaults

   • The NewMcpGateway function sets default values for several components (e.g., Authenticator, RateLimiter, Signer). While this is a good practice, it may lead to unexpected behavior if the defaults are not well-documented.
     • Action: Document the default values for all configuration fields in McpGatewayConfig. Consider logging a warning when defaults are used to make it clear to users.

4. Performance Optimization

   • The CredentialRedactor.Redact method uses a loop to apply each redaction pattern sequentially. This could become a performance bottleneck for large inputs or a large number of patterns.
     • Action: Consider optimizing the redaction process by using a single pass through the input string, if possible. Alternatively, evaluate the performance impact of the current implementation under realistic workloads.

5. Logging and Metrics

   • The McpGateway implementation includes audit logging and metrics recording, which is excellent. However, it is unclear if sensitive information (e.g., session tokens, payloads) is being logged.
     • Action: Ensure that all logs and metrics are scrubbed of sensitive information before being recorded. Add tests to verify this behavior.

6. Documentation

   • The PR mentions that documentation will be added in a future PR. However, it is important to ensure that the documentation is comprehensive and includes:
     • Examples of how to use the new MCP primitives.
     • Guidance on configuring and deploying the MCP Gateway in production.
     • Security considerations, such as key management and regular expression optimization.

---

Summary of Actions

Critical

• 🔴 Replace hardcoded HMAC key with a secure key management mechanism.
• 🔴 Audit and optimize regular expressions to prevent ReDoS attacks.
• 🔴 Add validation and sanitization for the ToolDescription field in InterceptToolCall.
• 🔴 Ensure thread safety in the SignerNonceStore implementation.

Warning

• 🟡 Document and version new public APIs to ensure backward compatibility.

Suggestions

• 💡 Add additional test cases for edge cases, stress testing, and nonce store behavior.
• 💡 Standardize error handling and consider using a structured error type.
• 💡 Document default configuration values and log warnings when defaults are used.
• 💡 Optimize the redaction process for performance.
• 💡 Ensure sensitive information is not logged or exposed in metrics.
• 💡 Provide comprehensive documentation in the upcoming documentation PR.

---

This PR demonstrates a strong focus on security and correctness, but addressing the critical issues outlined above is essential to ensure the robustness and safety of the implementation. Let me know if you need further clarification or assistance!

[github-actions]

🤖 AI Agent: security-scanner — Security Review of `feat(go): add MCP protocol support with full OWASP coverage`

Security Review of feat(go): add MCP protocol support with full OWASP coverage

This pull request introduces the MCP (Model Context Protocol) security primitives to the Go SDK, implementing several critical security features. Below is a detailed security review of the changes, focusing on the specified security concerns.

---

Findings

1. Prompt Injection Defense Bypass

• Risk: The McpGateway and McpInputSanitizer are responsible for detecting and mitigating prompt injection attacks. However, the implementation does not explicitly validate the ToolDescription or Input fields for malicious patterns such as instruction injection.
• Attack Vector: A malicious actor could craft a ToolDescription or Input field containing instructions to bypass governance policies or execute unintended actions.
• Severity: 🔴 CRITICAL
• Recommendation:
  • Introduce explicit validation and sanitization for ToolDescription and Input fields in McpGateway.
  • Use a strict allowlist for acceptable input patterns and reject any input that does not conform.
  • Add unit tests to verify that crafted inputs are detected and blocked.

---

2. Policy Engine Circumvention

• Risk: The McpGateway enforces governance policies, but the AllowPatterns and BlockOnSeverities configurations are not validated for completeness or correctness.
• Attack Vector: If AllowPatterns is misconfigured or incomplete, unauthorized tools could bypass governance. Similarly, if BlockOnSeverities does not include all critical severities, threats could be ignored.
• Severity: 🟠 HIGH
• Recommendation:
  • Validate AllowPatterns and BlockOnSeverities during initialization to ensure they cover all expected cases.
  • Add logging for any policy mismatches or gaps during runtime.
  • Include tests to verify that policies cannot be bypassed due to misconfigurations.

---

3. Trust Chain Weaknesses

• Risk: The McpMessageSigner uses HMAC-SHA256 for message signing, but the key management and rotation strategy are not detailed.
• Attack Vector: If the HMAC key (demoHMACKey) is leaked or reused across multiple environments, it could compromise the integrity of signed messages.
• Severity: 🟠 HIGH
• Recommendation:
  • Implement a secure key rotation mechanism and document it.
  • Avoid hardcoding keys like demoHMACKey in the codebase. Use environment variables or a secure secrets manager.
  • Add logging and monitoring for signature verification failures.

---

4. Credential Exposure

• Risk: The CredentialRedactor effectively removes sensitive information from logs, but there is a potential for sensitive data to be logged before redaction.
• Attack Vector: If a developer accidentally logs raw input or output before redaction, sensitive credentials could be exposed.
• Severity: 🟡 MEDIUM
• Recommendation:
  • Enforce a strict policy to redact all sensitive data before logging.
  • Add a linting rule or static analysis check to detect unredacted logging calls.
  • Perform a security audit of logging practices across the codebase.

---

5. Sandbox Escape

• Risk: No evidence of sandboxing or process isolation is present in this implementation.
• Attack Vector: If the tool execution environment is not isolated, a malicious tool could escape and compromise the host system.
• Severity: 🟠 HIGH
• Recommendation:
  • Use containerization or process isolation to sandbox tool execution.
  • Enforce resource limits (e.g., CPU, memory) for each tool execution.
  • Add tests to verify that tools cannot access unauthorized resources.

---

6. Deserialization Attacks

• Risk: The json.Unmarshal function is used to parse incoming requests, but there is no validation of the deserialized data.
• Attack Vector: Maliciously crafted JSON payloads could exploit vulnerabilities in downstream processing.
• Severity: 🟡 MEDIUM
• Recommendation:
  • Validate all deserialized data against a strict schema before processing.
  • Use libraries that support JSON schema validation to enforce constraints on input data.

---

7. Race Conditions

• Risk: The sync.RWMutex is used for thread safety, but there is no explicit testing for race conditions in concurrent scenarios.
• Attack Vector: A race condition in session management or rate limiting could allow unauthorized access or bypass rate limits.
• Severity: 🟡 MEDIUM
• Recommendation:
  • Add stress tests with high concurrency to identify potential race conditions.
  • Use tools like go test -race to detect race conditions during testing.

---

8. Supply Chain Risks

• Risk: The go.mod file includes a dependency on gopkg.in/yaml.v3, which is a common target for dependency confusion attacks.
• Attack Vector: An attacker could publish a malicious version of gopkg.in/yaml.v3 to compromise the project.
• Severity: 🟡 MEDIUM
• Recommendation:
  • Pin dependencies to specific versions and use checksums to verify integrity.
  • Use a dependency scanning tool to detect vulnerabilities in third-party libraries.
  • Regularly update dependencies to include the latest security patches.

---

Summary of Findings

Finding	Severity	Action Required
Prompt injection defense bypass	🔴 CRITICAL	Add strict input validation and sanitization for ToolDescription and Input.
Policy engine circumvention	🟠 HIGH	Validate AllowPatterns and BlockOnSeverities for completeness and correctness.
Trust chain weaknesses	🟠 HIGH	Implement secure key management and rotation for HMAC keys.
Credential exposure	🟡 MEDIUM	Enforce redaction of sensitive data before logging.
Sandbox escape	🟠 HIGH	Use containerization or process isolation for tool execution.
Deserialization attacks	🟡 MEDIUM	Validate deserialized data against a strict schema.
Race conditions	🟡 MEDIUM	Add stress tests and use go test -race to detect race conditions.
Supply chain risks	🟡 MEDIUM	Pin dependencies and use a dependency scanning tool.

---

Overall Assessment

This pull request introduces critical security primitives for the MCP protocol. While the implementation is robust in many areas, there are several high-priority issues that need to be addressed to ensure the security of downstream users. Addressing these findings will significantly enhance the security posture of the library.

[github-actions]

🤖 AI Agent: code-reviewer

Feedback on PR: feat(go): add MCP protocol support with full OWASP coverage

This PR introduces a comprehensive implementation of the MCP (Model Context Protocol) security primitives in the Go SDK, achieving parity with other language implementations. The changes include several new components, such as MCPMessageSigner, MCPSessionAuthenticator, MCPRateLimiter, MCPInputSanitizer, MCPResponseScanner, MCPGateway, CredentialRedactor, and MCPMetrics. Below is a detailed review of the PR:

---

🔴 CRITICAL: Security Issues

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

   • The regexp.Compile function is used to compile potentially complex regular expressions for credential redaction. If a user provides a poorly constructed custom pattern, this could lead to catastrophic backtracking and a denial-of-service attack.
   • Actionable Fix: Use regexp.CompilePOSIX instead of regexp.Compile to enforce stricter matching rules. Additionally, consider implementing a timeout mechanism for regex evaluation to prevent long-running regex evaluations.

2. Hardcoded Secret Key in McpMessageSigner

   • The NewMcpGateway function uses a hardcoded key ("0123456789abcdef0123456789abcdef") for the McpMessageSigner. This is a critical security vulnerability as it exposes the key to potential attackers.
   • Actionable Fix: Replace the hardcoded key with a secure key management mechanism, such as environment variables, a secrets manager, or a secure configuration file.

3. Insufficient Validation of Custom Redaction Patterns

   • The buildMcpRedactionPatterns function does not validate the complexity or safety of custom patterns provided by users. This could allow users to introduce unsafe or overly broad patterns that could lead to unintended behavior or security issues.
   • Actionable Fix: Add validation logic to ensure that custom patterns are safe, efficient, and do not introduce ReDoS risks.

4. Potential Sandbox Escape via Custom Patterns

   • The CredentialRedactor allows users to define custom patterns, which are then compiled and executed. This could be exploited to execute malicious code if the input is not properly sanitized.
   • Actionable Fix: Sanitize and validate custom patterns before compiling them. Consider restricting the types of patterns that can be used.

---

🟡 WARNING: Potential Breaking Changes

1. Default Policy Behavior

   • The McpGateway uses a default policy (DefaultMcpPolicy) if no policy is provided. This could lead to unexpected behavior if the default policy does not align with the user's expectations.
   • Actionable Fix: Clearly document the behavior of the default policy and provide examples of how to customize it. Consider requiring an explicit policy to be provided to avoid unintentional misconfigurations.

2. Backward Compatibility with Existing Systems

   • The introduction of MCP primitives in the Go SDK may require existing users to update their code to integrate with the new McpGateway and related components.
   • Actionable Fix: Provide clear migration guides and examples to help users transition to the new system.

---

💡 Suggestions for Improvement

1. Enhanced Logging for Debugging

   • The McpGateway and CredentialRedactor components could benefit from more detailed logging, especially in error and timeout scenarios.
   • Suggestion: Add structured logging (e.g., using a logging library) to capture detailed information about failures, timeouts, and threats detected.

2. Rate Limiter Configuration

   • The McpSlidingRateLimiter uses a fixed window size and maximum request count. These values may not be suitable for all use cases.
   • Suggestion: Allow dynamic configuration of the rate limiter parameters via the McpGatewayConfig.

3. Test Coverage

   • While the test coverage for the CredentialRedactor and McpGateway is good, additional edge cases could be tested, such as:
     • Invalid or malformed session tokens.
     • Extremely large payloads to test performance and timeout behavior.
     • Custom redaction patterns with edge-case regexes.
   • Suggestion: Add more test cases to cover these scenarios.

4. Documentation

   • The PR introduces several new components and concepts, but there is no accompanying documentation in this PR.
   • Suggestion: Include detailed documentation for each new component, including usage examples, configuration options, and security considerations.

5. Concurrency Safety

   • The CredentialRedactor and McpGateway rely on shared state (e.g., patterns, metrics). While sync.RWMutex is mentioned in the PR description, it is not explicitly visible in the provided code.
   • Suggestion: Ensure that all shared state is properly protected with mutexes or other concurrency-safe mechanisms.

---

Summary of Actionable Feedback

🔴 CRITICAL

1. Mitigate ReDoS risks in regex compilation and execution.
2. Remove hardcoded secret keys and use a secure key management solution.
3. Validate custom redaction patterns to prevent unsafe or overly broad patterns.
4. Sanitize and restrict custom patterns to prevent sandbox escape.

🟡 WARNING

1. Clearly document the behavior of the default policy and consider requiring explicit policy configuration.
2. Provide migration guides for users transitioning to the new MCP primitives.

💡 SUGGESTIONS

1. Add detailed logging for debugging and error scenarios.
2. Allow dynamic configuration of rate limiter parameters.
3. Expand test coverage to include edge cases and performance scenarios.
4. Include detailed documentation for new components and their usage.
5. Ensure concurrency safety for all shared state.

---

Overall Assessment

The PR introduces a robust set of security primitives for the Go SDK, aligning with OWASP MCP guidelines. However, there are critical security issues and potential breaking changes that need to be addressed before merging. Once these issues are resolved, the PR will significantly enhance the security and functionality of the Go SDK.

[github-actions]

🤖 AI Agent: code-reviewer

Review of PR: feat(go): add MCP protocol support with full OWASP coverage

---

🔴 CRITICAL Issues

1. Regex Denial of Service (ReDoS) Vulnerability:

   • The CredentialRedactor uses regular expressions for sensitive data redaction. While a timeout mechanism is implemented, the regex patterns themselves are complex and could be susceptible to ReDoS attacks.
   • Patterns like (?s)-----BEGIN RSA PRIVATE KEY-----.*?-----END RSA PRIVATE KEY----- and (?i)\bBearer\s+[A-Za-z0-9._~+/=-]{10,}\b are particularly risky because they involve unbounded quantifiers (.*?) and case-insensitive matching.
   • Recommendation: Use more specific patterns or limit the length of matches to mitigate potential ReDoS attacks. Consider pre-compiling regex patterns with stricter constraints or using a library designed to prevent ReDoS vulnerabilities.

2. Hardcoded Cryptographic Key:

   • In mcp_gateway.go, the NewMcpGateway function initializes the McpMessageSigner with a hardcoded key: []byte("0123456789abcdef0123456789abcdef").
   • This is a severe security issue as it exposes the cryptographic key in the source code.
   • Recommendation: Use a secure key management system (e.g., Azure Key Vault, AWS KMS) to securely store and retrieve cryptographic keys. Never hardcode sensitive keys in the source code.

3. Potential Sandbox Escape via Custom Patterns:

   • The CredentialRedactor allows custom patterns to be defined via CustomPatterns. If an attacker can inject arbitrary regex patterns into this configuration, it could lead to a denial of service or even sandbox escape.
   • Recommendation: Validate and sanitize custom patterns before compiling them. Ensure that only trusted users can define custom patterns.

4. Insufficient Validation for Custom Patterns:

   • The buildMcpRedactionPatterns function does not validate the Name field of McpRedactionPattern. This could allow an attacker to inject malicious or misleading names that could confuse logging or monitoring systems.
   • Recommendation: Validate the Name field to ensure it does not contain malicious or unexpected characters.

---

🟡 WARNING: Potential Breaking Changes

1. Policy Evaluation Logic:

   • The evaluatePolicy function in McpGateway introduces a new policy evaluation mechanism. If the DefaultDecision is not explicitly set in the McpPolicy, it defaults to Allow. This could lead to unintended behavior if the policy is not carefully configured.
   • Recommendation: Consider requiring an explicit DefaultDecision in the McpPolicy to avoid unintentional defaulting to Allow.

2. Custom Patterns Behavior:

   • The introduction of CustomPatterns in CredentialRedactorConfig may lead to unexpected behavior if users define overlapping or conflicting patterns.
   • Recommendation: Document the behavior when multiple patterns match the same input. Consider adding a mechanism to prioritize patterns or warn users about conflicts.

---

💡 Suggestions for Improvement

1. Improved Logging and Error Messages:

   • The error messages in the InterceptToolCall method are generic and may not provide sufficient context for debugging.
   • Recommendation: Include more detailed information in error messages, such as the specific step where the error occurred (e.g., "Rate limiting failed for agent ID: X").

2. Test Coverage:

   • While the test coverage is generally good, there are some gaps:
     • No tests for edge cases in CredentialRedactor with malformed or extremely large inputs.
     • No tests for McpGateway with misconfigured or missing components (e.g., missing RateLimiter or Authenticator).
   • Recommendation: Add tests for edge cases and misconfigurations to ensure robust behavior.

3. Concurrency Considerations:

   • While the PR mentions the use of sync.RWMutex for thread safety, the implementation does not explicitly show where and how it is used. For example, CredentialRedactor and McpGateway do not seem to use any synchronization primitives.
   • Recommendation: Review the code for potential race conditions, especially in shared state or concurrent operations. Add explicit synchronization where necessary.

4. Documentation:

   • The PR mentions that documentation will be added in a future PR. However, the current implementation introduces complex components like McpGateway, CredentialRedactor, and McpSessionAuthenticator, which require detailed documentation for proper usage.
   • Recommendation: Include at least minimal documentation in this PR to help reviewers and future developers understand the intended usage and configuration of these components.

5. OpenTelemetry Metrics:

   • The McpMetrics component is mentioned but not fully reviewed in this PR. Ensure that metrics are properly scoped and do not leak sensitive information (e.g., tool names or payloads).
   • Recommendation: Review the McpMetrics implementation in detail to ensure compliance with privacy and security best practices.

6. Backward Compatibility:

   • While this PR introduces new functionality, it does not appear to break existing APIs. However, the new McpGateway and its associated components may require changes to existing codebases to integrate.
   • Recommendation: Clearly document the integration steps and any potential impact on existing systems.

---

Summary

This PR introduces critical security features for the Go SDK, aligning it with OWASP MCP Security Cheat Sheet guidelines. However, there are several critical security issues that must be addressed before merging, including potential ReDoS vulnerabilities, hardcoded cryptographic keys, and insufficient validation for custom patterns. Additionally, there are some potential breaking changes and areas for improvement in testing, documentation, and concurrency handling.

Action Items:

1. 🔴 Address the ReDoS vulnerability in regex patterns.
2. 🔴 Remove the hardcoded cryptographic key and integrate a secure key management solution.
3. 🔴 Add validation for custom patterns to prevent sandbox escape and malicious inputs.
4. 🟡 Require explicit DefaultDecision in McpPolicy to avoid unintended defaults.
5. 💡 Add tests for edge cases, misconfigurations, and concurrency.
6. 💡 Include minimal documentation for new components in this PR.

Let me know if you need further clarification or assistance!

[github-actions]

🤖 AI Agent: code-reviewer

Review Summary

This PR introduces a Go implementation of the Model Context Protocol (MCP) security primitives, achieving parity with other language implementations in the repository. The implementation includes several security-focused components, such as MCPMessageSigner, MCPSessionAuthenticator, MCPRateLimiter, MCPInputSanitizer, and CredentialRedactor. The PR also includes an example HTTP server demonstrating the usage of these components.

The implementation is well-structured and adheres to the project's style guidelines. However, there are some areas that require attention, particularly around security, concurrency, and backward compatibility.

---

🔴 CRITICAL: Security Issues

1. Regex Denial-of-Service (ReDoS) Vulnerability

   • The regexp.Compile and regexp.MustCompile functions are used to compile regular expressions for sensitive data patterns. However, some of these regex patterns (e.g., (?s)-----BEGIN RSA PRIVATE KEY-----.*?-----END RSA PRIVATE KEY-----) are vulnerable to ReDoS attacks due to their use of .*? in a multi-line context.
   • Recommendation: Use more specific patterns or limit the input size before applying regex. Additionally, consider using libraries or tools designed to mitigate ReDoS risks.

2. Fail-Closed Behavior on Panic

   • While the Redact method uses a recover block to handle panics, the error message included in the McpThreat object reveals implementation details (e.g., "credential redaction failed closed: %v"). This could potentially leak sensitive information about the internal implementation.
   • Recommendation: Replace the detailed error message with a generic one to avoid exposing internal details.

3. Credential Redaction Coverage

   • The CredentialRedactor currently redacts a predefined set of patterns. However, there is no mechanism to ensure that all sensitive data types are covered, especially for custom use cases.
   • Recommendation: Provide a mechanism for users to validate their custom patterns against a comprehensive test suite. Additionally, document the limitations of the default patterns and encourage users to extend them as needed.

4. Session Token Validation

   • The handleCallTool method checks if req.SessionToken is empty but does not validate its format or length before passing it to ValidateSession. This could allow malformed or malicious tokens to reach the ValidateSession logic.
   • Recommendation: Add stricter validation for the session token format and length before passing it to ValidateSession.

---

🟡 WARNING: Potential Breaking Changes

1. Public API Changes
   • The introduction of new components like CredentialRedactor and McpGateway adds new public APIs to the Go SDK. While this is expected for a new feature, any future changes to these APIs will need to be carefully managed to maintain backward compatibility.
   • Recommendation: Clearly document these APIs and their expected behavior. Consider marking them as experimental if further changes are anticipated.

---

💡 Suggestions for Improvement

1. Concurrency Safety

   • While sync.RWMutex is used for shared state, it is not clear if all shared resources are adequately protected. For example, the seen map in the Redact method is not explicitly synchronized, which could lead to race conditions if Redact is called concurrently.
   • Recommendation: Use a sync.Map or explicitly synchronize access to shared resources.

2. Test Coverage

   • The test coverage for CredentialRedactor is good, but other components like McpGateway and McpSessionAuthenticator lack unit tests in this PR.
   • Recommendation: Add comprehensive unit tests for all new components, especially for edge cases like rate-limiting thresholds and session expiration.

3. Error Handling

   • The handleCallTool method uses writeJSON to return error messages but does not log these errors. This could make debugging more difficult.
   • Recommendation: Log all errors before returning them to the client.

4. Documentation

   • While the example HTTP server is well-documented, the new components themselves lack detailed comments explaining their purpose and usage.
   • Recommendation: Add detailed comments to all new public methods and structs to improve maintainability and ease of use.

5. Default Configuration

   • The CredentialRedactor uses a default regex timeout of defaultMcpRegexBudget if none is provided. However, the value of defaultMcpRegexBudget is not included in the PR.
   • Recommendation: Ensure that the default timeout is set to a reasonable value (e.g., 100ms) and document it clearly.

6. Error Wrapping

   • The buildMcpRedactionPatterns function wraps errors using fmt.Errorf but does not use the %w verb consistently.
   • Recommendation: Use the %w verb for error wrapping to allow for error unwrapping and better error handling.

7. Input Validation

   • The handleCallTool method does not validate the ToolName or ToolDescription fields for length or content. This could lead to unexpected behavior or vulnerabilities if these fields contain malicious input.
   • Recommendation: Add validation for these fields to ensure they meet expected constraints.

8. Logging

   • The handleCallTool method logs the sanitized request body but does not log the response or any governance decisions made by the McpGateway.
   • Recommendation: Add logging for responses and governance decisions to improve observability.

9. Error Codes

   • The statusCodeForError function is used to map errors to HTTP status codes, but its implementation is not included in the PR.
   • Recommendation: Ensure that this function is implemented to handle all possible error cases and that it is thoroughly tested.

10. OpenTelemetry Metrics

    • The McpMetrics component is initialized but not used in the example HTTP server.
    • Recommendation: Demonstrate how to use McpMetrics in the example server or remove it if it is not needed.

---

Final Assessment

The PR introduces a robust set of security primitives for the Go SDK, achieving significant progress toward MCP OWASP compliance. However, there are critical security issues that need to be addressed before merging. Additionally, improvements in test coverage, documentation, and error handling would enhance the quality and maintainability of the code.

• Merge Recommendation: 🚫 Do not merge until critical issues are resolved.
• Priority Fixes: Address the ReDoS vulnerability, improve session token validation, and ensure concurrency safety.
• Next Steps: Add missing tests, improve documentation, and address the suggestions for improvement.

[github-actions]

🤖 AI Agent: code-reviewer

Review Summary

The pull request introduces the Model Context Protocol (MCP) security primitives to the Go SDK, achieving significant OWASP coverage. The implementation appears robust, with a focus on security, thread safety, and extensibility. However, there are several areas that require attention to ensure compliance with security best practices and maintainability.

Actionable Feedback

🔴 CRITICAL Issues

1. Credential Handling:

   • The CredentialRedactor class uses regex patterns to identify sensitive information. Ensure that these patterns are comprehensive and tested against a wide range of potential inputs to avoid false negatives. Consider implementing a mechanism to log or alert when sensitive data is detected but not redacted.

2. Session Management:

   • The session management logic in McpSessionAuthenticator should be reviewed for potential race conditions, especially with concurrent access. Ensure that session creation and validation are atomic operations to prevent session fixation attacks.

3. Error Handling:

   • In handleCallTool, errors are logged but not sufficiently sanitized before being returned to the client. Ensure that sensitive information is not exposed in error messages. Use generic error messages for client-facing responses.

🟡 WARNING Issues

1. Backward Compatibility:

   • The introduction of new types and methods in the public API (e.g., CredentialRedactor, McpGateway) may break existing implementations if they are not properly versioned. Ensure that the changes are backward compatible or clearly document breaking changes.

2. Concurrency:

   • While sync.RWMutex is used for thread safety, ensure that all shared resources are adequately protected. Review all instances of shared state access to confirm that they are properly synchronized.

3. Rate Limiting:

   • The rate limiting implementation should be thoroughly tested under load to ensure it behaves as expected. Consider edge cases where the limits may be exceeded rapidly, leading to denial of service.

💡 SUGGESTION Improvements

1. Testing Coverage:

   • While tests have been added, consider expanding the test suite to include edge cases and potential attack vectors, such as SQL injection and path traversal attempts. Utilize fuzz testing to uncover unexpected vulnerabilities.

2. Documentation:

   • Enhance the documentation for the new MCP primitives, particularly around usage patterns and security implications. Include examples of how to integrate these components into existing systems.

3. Configuration Options:

   • Provide more configuration options for the CredentialRedactor, such as enabling/disabling specific patterns or adjusting severity levels dynamically. This would enhance flexibility for different deployment scenarios.

4. Logging and Monitoring:

   • Implement structured logging for better observability. Consider integrating with existing logging frameworks to allow for centralized logging and monitoring of security events.

5. Dependency Management:

   • Regularly review and update dependencies to mitigate vulnerabilities. Consider using tools like dependabot to automate this process.

Conclusion

The implementation of MCP security primitives is a significant step forward for the Go SDK. Addressing the critical issues and warnings outlined above will enhance the security posture and maintainability of the codebase. The suggestions provided will further improve the robustness of the implementation and ensure compliance with best practices in security and software development.

[jackbatzner]

CI Status Summary — All findings addressed ✅

All CI checks pass except:

• Maintainer approval gate — awaiting your review
• link-check — not reproducible locally (changed markdown has no hyperlinks); likely flaky CI

Fixes this round:

• Removed hardcoded fallback HMAC key from NewMcpGateway — now fails closed unless caller provides config.Signer
• ReDoS concern: Go uses RE2 engine (no catastrophic backtracking) + bounded scan budget
• Thread safety: InMemorySignerNonceStore uses sync.Mutex, passes go test -race

This is the Core PR in a 3-PR stack. Merge order: #832 (Core) → #833 (Package) → #834 (Docs).

[imran-siddique]

Approved and ready to merge, but this PR has merge conflicts with main (likely from #774 merging). Please rebase on latest main to resolve conflicts, then we can merge.

[github-actions]

🤖 AI Agent: code-reviewer

Review Summary

This PR introduces the MCP (Model Context Protocol) security primitives to the Go SDK, achieving parity with other language implementations. The implementation includes a comprehensive set of security features, such as HMAC-based message signing, session authentication, rate limiting, input sanitization, and response scanning. The PR also includes a demo HTTP server showcasing the integration of these primitives.

The code demonstrates a strong focus on security, thread safety, and fail-closed behavior, aligning with OWASP MCP guidelines. However, there are areas where improvements can be made, and a few potential issues need to be addressed.

---

🔴 CRITICAL Issues

1. Regex Denial of Service (ReDoS) Vulnerability

   • File: credential_redactor.go
   • Issue: The use of complex regular expressions (e.g., (?s)-----BEGIN RSA PRIVATE KEY-----.*?-----END RSA PRIVATE KEY-----) without proper safeguards can lead to ReDoS attacks if an attacker provides maliciously crafted input. Although a timeout mechanism is implemented, it is not sufficient to prevent all potential ReDoS attacks.
   • Recommendation: Use a library like re2 or a similar regex engine that is resistant to ReDoS attacks. Alternatively, pre-validate inputs to ensure they are within acceptable bounds before applying regex.

2. Insufficient Validation of Custom Patterns

   • File: credential_redactor.go
   • Issue: Custom patterns provided in CredentialRedactorConfig are compiled without sufficient validation. If a user provides an invalid or malicious regex, it could lead to runtime panics or performance degradation.
   • Recommendation: Add stricter validation for custom regex patterns during the initialization of CredentialRedactor. For example, limit regex complexity or use a sandboxed regex engine.

3. Potential Key Management Issue

   • File: main.go
   • Issue: The HMAC key (demoHMACKey) is hardcoded in the example server. This could lead to accidental exposure or misuse in production environments.
   • Recommendation: Use a secure key management solution (e.g., Azure Key Vault, AWS KMS) to store and retrieve sensitive keys. Ensure that the example code includes comments or documentation emphasizing this best practice.

---

🟡 WARNING: Potential Breaking Changes

1. Custom Pattern Behavior

   • File: credential_redactor.go
   • Issue: The CredentialRedactor allows users to define custom patterns with a default severity of McpSeverityWarning if not explicitly specified. This behavior may lead to unexpected results if users assume a different default severity.
   • Recommendation: Consider requiring users to explicitly specify the severity for custom patterns. Alternatively, document this behavior clearly in the API documentation.

2. Session Token Expiry Handling

   • File: main.go
   • Issue: The handleCallTool function returns a 401 Unauthorized status code for expired sessions. This could break existing clients that expect a different status code (e.g., 403 Forbidden).
   • Recommendation: Confirm that this behavior aligns with the expected behavior of other language implementations. If not, consider aligning the status code for consistency.

---

💡 Suggestions for Improvement

1. Enhanced Logging

   • File: main.go
   • Observation: The server logs sanitized request and response payloads, but it does not log the associated session or tool information.
   • Recommendation: Include session and tool identifiers in the logs to improve traceability and debugging.

2. Test Coverage

   • File: credential_redactor_test.go
   • Observation: While the tests cover basic functionality, edge cases such as invalid custom patterns, extremely large inputs, and edge-case regex patterns are not tested.
   • Recommendation: Add tests for edge cases, including:
     • Invalid regex patterns in CredentialRedactorConfig.
     • Inputs that exceed the maximum allowed size.
     • Inputs designed to trigger ReDoS vulnerabilities.

3. Error Handling in handleCallTool

   • File: main.go
   • Observation: The handleCallTool function uses a generic error message for invalid JSON payloads ("invalid json payload").
   • Recommendation: Provide more specific error messages to help users debug issues, such as "missing required field: session_token".

4. Concurrency Testing

   • File: credential_redactor.go, main.go
   • Observation: While the PR mentions that thread safety is ensured using sync.RWMutex, there are no tests to validate concurrent behavior.
   • Recommendation: Add tests to simulate concurrent access to shared resources (e.g., CredentialRedactor, McpSessionAuthenticator) and ensure thread safety under high concurrency.

5. Documentation

   • File: README.md
   • Observation: The example server's README is clear but does not provide guidance on how to integrate the MCP primitives into a real-world application.
   • Recommendation: Include a section in the README or documentation that explains how to use the MCP primitives in production scenarios, including best practices for key management, logging, and error handling.

6. Default Configuration

   • File: credential_redactor.go
   • Observation: The CredentialRedactor uses a default regex timeout of defaultMcpRegexBudget, but the value of this constant is not shown in the diff.
   • Recommendation: Ensure that the default timeout is reasonable (e.g., 100ms) and document it in the code and user documentation.

7. Error Recovery

   • File: credential_redactor.go
   • Observation: The Redact method uses a defer statement to recover from panics. While this is a good fail-safe mechanism, it may mask underlying issues during development.
   • Recommendation: Log the stack trace of the panic to aid debugging. Use debug.Stack() from the runtime package to capture the stack trace.

---

General Observations

• Policy Engine Correctness: The McpGateway implementation appears robust, with clear fail-closed behavior and support for policy-based decision-making. However, additional tests for edge cases (e.g., malformed policies, conflicting rules) would be beneficial.
• Thread Safety: The use of sync.RWMutex and other concurrency-safe constructs is commendable. However, as mentioned, concurrency tests should be added to validate these implementations.
• OWASP MCP Compliance: The implementation covers 11 out of 12 sections of the OWASP MCP Security Cheat Sheet, which is excellent. The omission of §11 (Consent UI) is reasonable for a server-side SDK.
• Backward Compatibility: This PR introduces new functionality and does not modify existing APIs, so it is unlikely to cause breaking changes.

---

Conclusion

This PR is a well-thought-out and comprehensive implementation of MCP security primitives for the Go SDK. However, the identified critical issues (ReDoS vulnerability, insufficient validation of custom patterns, and hardcoded HMAC key) must be addressed before merging. Additionally, addressing the warnings and suggestions will further improve the robustness and usability of the implementation.

[github-actions]

🤖 AI Agent: code-reviewer

Review Summary

This pull request introduces the Model Context Protocol (MCP) security primitives to the Go SDK of the agent-governance-toolkit, achieving significant OWASP compliance. The implementation appears well-structured and includes several security features. However, there are some areas that require attention to ensure robustness, security, and maintainability.

Actionable Feedback

🔴 CRITICAL Issues

1. Credential Handling:

   • The CredentialRedactor class includes hardcoded patterns for sensitive data (e.g., API keys, bearer tokens). Ensure that these patterns are comprehensive and regularly updated to cover new types of sensitive information. Consider implementing a more dynamic approach to detect sensitive data rather than relying solely on regex patterns, which can be bypassed.

2. Error Handling:

   • In the handleCallTool function, errors from s.gateway.InterceptToolCall are logged but not sufficiently handled. Ensure that sensitive information is not leaked in error messages returned to the client. Always sanitize error messages before sending them to the client.

3. Replay Protection:

   • While the MCPMessageSigner includes nonce replay protection, ensure that the implementation is robust against replay attacks. Consider adding additional checks or logging mechanisms to detect and respond to potential replay attempts.

🟡 WARNING Issues

1. Breaking Changes:

   • The introduction of new types and methods (e.g., CredentialRedactor, MCPMessageSigner) may affect existing consumers of the SDK. Ensure that the documentation clearly states any breaking changes and provides migration paths for users.

2. Concurrency and Thread Safety:

   • While sync.RWMutex is used for shared state, ensure that all shared resources are adequately protected. Review the entire codebase for potential race conditions, especially in areas where multiple goroutines may access shared data.

3. Dependency Management:

   • The go.mod file includes a direct dependency on github.com/microsoft/agent-governance-toolkit/sdks/go. Ensure that this does not lead to versioning conflicts in the future. Consider using a versioned module path to avoid potential issues.

💡 SUGGESTION Improvements

1. Unit Tests:

   • The tests for CredentialRedactor are a good start, but consider expanding the test coverage to include edge cases and potential failure scenarios. For example, test how the redactor behaves with malformed input or unexpected patterns.

2. Documentation:

   • The README for the MCP HTTP server example is clear, but consider adding more detailed comments within the code to explain the purpose of each function and the rationale behind certain design decisions. This will help future maintainers understand the code better.

3. Logging:

   • Implement structured logging throughout the application to facilitate better monitoring and debugging. Consider using a logging library that supports log levels and structured data.

4. Configuration Management:

   • The configuration for the CredentialRedactor and other components is currently hardcoded in the demo server. Consider externalizing these configurations to a file or environment variables to enhance flexibility and security.

5. Performance Testing:

   • Given that the MCP includes rate limiting and session management, consider implementing performance tests to ensure that the system can handle the expected load without degradation in performance.

Conclusion

Overall, the implementation of MCP security primitives is a significant enhancement to the Go SDK. Addressing the critical issues and considering the warnings and suggestions will help ensure that the library remains secure, maintainable, and user-friendly.

[imran-siddique]

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 Go MCP core (3,065 lines) 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.