Add testing documentation - #4
Merged
Merged
Conversation
Merged
vk-playground
pushed a commit
to vk-playground/mcp-context-forge
that referenced
this pull request
Sep 14, 2025
Add testing documentation Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
vk-playground
pushed a commit
to vk-playground/mcp-context-forge
that referenced
this pull request
Sep 14, 2025
Add testing documentation
vk-playground
pushed a commit
to vk-playground/mcp-context-forge
that referenced
this pull request
Sep 16, 2025
Add testing documentation Signed-off-by: Vicky Kuo <vicky.kuo@ibm.com>
hughhennelly
added a commit
to hughhennelly/mcp-context-forge
that referenced
this pull request
Feb 12, 2026
1. Fix broken imports (Issue #1): - Change from ..database to ..db - Fix unified_pdp imports to use plugins.unified_pdp - Update in routes, services, schemas, and tests 2. Register sandbox router in main.py (Issue IBM#2): - Add import and app.include_router call 3. Fix XSS vulnerability (Issue IBM#3): - Replace f-string HTML with Jinja2 template - Create sandbox_simulate_results.html template - Add Request parameter for template access 4. Add authentication (Issue IBM#4): - Add Depends(get_current_user) to simulate endpoint 5. Remove scratch files (Issue IBM#5): - Delete sandbox_header.txt and sandbox_new_header.txt 6. Resolve schemas conflict (Issue IBM#6): - Merge schemas/sandbox.py into schemas.py - Remove conflicting schemas/ directory - Update imports in routes and services All changes tested and ready for review. Related to IBM#2226 Signed-off-by: hughhennelly <hughhennelly06@gmail.com>
crivetimihai
pushed a commit
that referenced
this pull request
Feb 24, 2026
1. Fix broken imports (Issue #1): - Change from ..database to ..db - Fix unified_pdp imports to use plugins.unified_pdp - Update in routes, services, schemas, and tests 2. Register sandbox router in main.py (Issue #2): - Add import and app.include_router call 3. Fix XSS vulnerability (Issue #3): - Replace f-string HTML with Jinja2 template - Create sandbox_simulate_results.html template - Add Request parameter for template access 4. Add authentication (Issue #4): - Add Depends(get_current_user) to simulate endpoint 5. Remove scratch files (Issue #5): - Delete sandbox_header.txt and sandbox_new_header.txt 6. Resolve schemas conflict (Issue #6): - Merge schemas/sandbox.py into schemas.py - Remove conflicting schemas/ directory - Update imports in routes and services All changes tested and ready for review. Related to #2226 Signed-off-by: hughhennelly <hughhennelly06@gmail.com>
Merged
6 tasks
12 tasks
aidbutlr
referenced
this pull request
in aidbutlr/mcp-context-forge
Mar 3, 2026
CYFR-280 merge from public repo 2026 01 07
2 tasks
MohanLaksh
added a commit
that referenced
this pull request
Apr 11, 2026
This commit fixes 5 issues identified in code review: **Issue #1 - Default config inconsistency (CRITICAL):** - Changed SSRF_ALLOW_LOCALHOST default from false to true - Fixes immediate failure on fresh installs where backend_rpc_url defaults to 127.0.0.1 - Updated README.md to reflect new defaults and clarify production vs development modes - Location: tools_rust/mcp_runtime/src/config.rs:208 **Issue #2 - SSRF bypass in two functions (CRITICAL):** - Added URL validation to backend_authenticate_url() before HTTP call - Added URL validation to backend_tools_call_resolve_url() before HTTP call - Both functions now use url_validator.validate_url() with SSRF protection - Locations: tools_rust/mcp_runtime/src/lib.rs:2564, 8158 **Issue #3 - Malformed CIDR fails open (HIGH):** - Changed CIDR parsing from fail-open (warn + continue) to fail-closed (return error) - Invalid CIDR in SSRF_BLOCKED_NETWORKS now fails runtime startup - Invalid CIDR in SSRF_ALLOWED_NETWORKS now fails runtime startup - Location: tools_rust/mcp_runtime/src/url_validator.rs:186-210 **Issue #4 - DNS re-resolution overhead (MEDIUM):** - Implemented DNS result caching with 5-minute TTL - Added Arc<RwLock<HashMap<String, (Vec<IpAddr>, Instant)>>> cache - Reduces DNS lookups on hot paths while maintaining security - Cache prevents DNS rebinding attacks with short TTL - Location: tools_rust/mcp_runtime/src/url_validator.rs:58-68, 486-530 **Issue #5 - Missing body-size limit test (LOW):** - Added integration test: request_body_size_limit_rejects_large_payloads() - Verifies 413 Payload Too Large response for >10MB request bodies - Tests the DefaultBodyLimit middleware enforcement - Location: tools_rust/mcp_runtime/src/lib.rs:13309-13338 All changes maintain backward compatibility except for stricter CIDR validation (fail-closed is more secure). Addresses reviewer feedback from lucarlig on PR #4111 Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com>
6 tasks
MohanLaksh
added a commit
that referenced
this pull request
Apr 11, 2026
This commit fixes 5 issues identified in code review: **Issue #1 - Default config inconsistency (CRITICAL):** - Changed SSRF_ALLOW_LOCALHOST default from false to true - Fixes immediate failure on fresh installs where backend_rpc_url defaults to 127.0.0.1 - Updated README.md to reflect new defaults and clarify production vs development modes - Location: tools_rust/mcp_runtime/src/config.rs:208 **Issue #2 - SSRF bypass in two functions (CRITICAL):** - Added URL validation to backend_authenticate_url() before HTTP call - Added URL validation to backend_tools_call_resolve_url() before HTTP call - Both functions now use url_validator.validate_url() with SSRF protection - Locations: tools_rust/mcp_runtime/src/lib.rs:2564, 8158 **Issue #3 - Malformed CIDR fails open (HIGH):** - Changed CIDR parsing from fail-open (warn + continue) to fail-closed (return error) - Invalid CIDR in SSRF_BLOCKED_NETWORKS now fails runtime startup - Invalid CIDR in SSRF_ALLOWED_NETWORKS now fails runtime startup - Location: tools_rust/mcp_runtime/src/url_validator.rs:186-210 **Issue #4 - DNS re-resolution overhead (MEDIUM):** - Implemented DNS result caching with 5-minute TTL - Added Arc<RwLock<HashMap<String, (Vec<IpAddr>, Instant)>>> cache - Reduces DNS lookups on hot paths while maintaining security - Cache prevents DNS rebinding attacks with short TTL - Location: tools_rust/mcp_runtime/src/url_validator.rs:58-68, 486-530 **Issue #5 - Missing body-size limit test (LOW):** - Added integration test: request_body_size_limit_rejects_large_payloads() - Verifies 413 Payload Too Large response for >10MB request bodies - Tests the DefaultBodyLimit middleware enforcement - Location: tools_rust/mcp_runtime/src/lib.rs:13309-13338 All changes maintain backward compatibility except for stricter CIDR validation (fail-closed is more secure). Addresses reviewer feedback from lucarlig on PR #4111 Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com>
MohanLaksh
added a commit
that referenced
this pull request
Apr 14, 2026
This commit fixes 5 issues identified in code review: **Issue #1 - Default config inconsistency (CRITICAL):** - Changed SSRF_ALLOW_LOCALHOST default from false to true - Fixes immediate failure on fresh installs where backend_rpc_url defaults to 127.0.0.1 - Updated README.md to reflect new defaults and clarify production vs development modes - Location: tools_rust/mcp_runtime/src/config.rs:208 **Issue #2 - SSRF bypass in two functions (CRITICAL):** - Added URL validation to backend_authenticate_url() before HTTP call - Added URL validation to backend_tools_call_resolve_url() before HTTP call - Both functions now use url_validator.validate_url() with SSRF protection - Locations: tools_rust/mcp_runtime/src/lib.rs:2564, 8158 **Issue #3 - Malformed CIDR fails open (HIGH):** - Changed CIDR parsing from fail-open (warn + continue) to fail-closed (return error) - Invalid CIDR in SSRF_BLOCKED_NETWORKS now fails runtime startup - Invalid CIDR in SSRF_ALLOWED_NETWORKS now fails runtime startup - Location: tools_rust/mcp_runtime/src/url_validator.rs:186-210 **Issue #4 - DNS re-resolution overhead (MEDIUM):** - Implemented DNS result caching with 5-minute TTL - Added Arc<RwLock<HashMap<String, (Vec<IpAddr>, Instant)>>> cache - Reduces DNS lookups on hot paths while maintaining security - Cache prevents DNS rebinding attacks with short TTL - Location: tools_rust/mcp_runtime/src/url_validator.rs:58-68, 486-530 **Issue #5 - Missing body-size limit test (LOW):** - Added integration test: request_body_size_limit_rejects_large_payloads() - Verifies 413 Payload Too Large response for >10MB request bodies - Tests the DefaultBodyLimit middleware enforcement - Location: tools_rust/mcp_runtime/src/lib.rs:13309-13338 All changes maintain backward compatibility except for stricter CIDR validation (fail-closed is more secure). Addresses reviewer feedback from lucarlig on PR #4111 Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com>
MohanLaksh
added a commit
that referenced
this pull request
Apr 21, 2026
This commit fixes 5 issues identified in code review: **Issue #1 - Default config inconsistency (CRITICAL):** - Changed SSRF_ALLOW_LOCALHOST default from false to true - Fixes immediate failure on fresh installs where backend_rpc_url defaults to 127.0.0.1 - Updated README.md to reflect new defaults and clarify production vs development modes - Location: tools_rust/mcp_runtime/src/config.rs:208 **Issue #2 - SSRF bypass in two functions (CRITICAL):** - Added URL validation to backend_authenticate_url() before HTTP call - Added URL validation to backend_tools_call_resolve_url() before HTTP call - Both functions now use url_validator.validate_url() with SSRF protection - Locations: tools_rust/mcp_runtime/src/lib.rs:2564, 8158 **Issue #3 - Malformed CIDR fails open (HIGH):** - Changed CIDR parsing from fail-open (warn + continue) to fail-closed (return error) - Invalid CIDR in SSRF_BLOCKED_NETWORKS now fails runtime startup - Invalid CIDR in SSRF_ALLOWED_NETWORKS now fails runtime startup - Location: tools_rust/mcp_runtime/src/url_validator.rs:186-210 **Issue #4 - DNS re-resolution overhead (MEDIUM):** - Implemented DNS result caching with 5-minute TTL - Added Arc<RwLock<HashMap<String, (Vec<IpAddr>, Instant)>>> cache - Reduces DNS lookups on hot paths while maintaining security - Cache prevents DNS rebinding attacks with short TTL - Location: tools_rust/mcp_runtime/src/url_validator.rs:58-68, 486-530 **Issue #5 - Missing body-size limit test (LOW):** - Added integration test: request_body_size_limit_rejects_large_payloads() - Verifies 413 Payload Too Large response for >10MB request bodies - Tests the DefaultBodyLimit middleware enforcement - Location: tools_rust/mcp_runtime/src/lib.rs:13309-13338 All changes maintain backward compatibility except for stricter CIDR validation (fail-closed is more secure). Addresses reviewer feedback from lucarlig on PR #4111 Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com>
MohanLaksh
added a commit
that referenced
this pull request
Apr 21, 2026
This commit fixes 5 issues identified in code review: **Issue #1 - Default config inconsistency (CRITICAL):** - Changed SSRF_ALLOW_LOCALHOST default from false to true - Fixes immediate failure on fresh installs where backend_rpc_url defaults to 127.0.0.1 - Updated README.md to reflect new defaults and clarify production vs development modes - Location: tools_rust/mcp_runtime/src/config.rs:208 **Issue #2 - SSRF bypass in two functions (CRITICAL):** - Added URL validation to backend_authenticate_url() before HTTP call - Added URL validation to backend_tools_call_resolve_url() before HTTP call - Both functions now use url_validator.validate_url() with SSRF protection - Locations: tools_rust/mcp_runtime/src/lib.rs:2564, 8158 **Issue #3 - Malformed CIDR fails open (HIGH):** - Changed CIDR parsing from fail-open (warn + continue) to fail-closed (return error) - Invalid CIDR in SSRF_BLOCKED_NETWORKS now fails runtime startup - Invalid CIDR in SSRF_ALLOWED_NETWORKS now fails runtime startup - Location: tools_rust/mcp_runtime/src/url_validator.rs:186-210 **Issue #4 - DNS re-resolution overhead (MEDIUM):** - Implemented DNS result caching with 5-minute TTL - Added Arc<RwLock<HashMap<String, (Vec<IpAddr>, Instant)>>> cache - Reduces DNS lookups on hot paths while maintaining security - Cache prevents DNS rebinding attacks with short TTL - Location: tools_rust/mcp_runtime/src/url_validator.rs:58-68, 486-530 **Issue #5 - Missing body-size limit test (LOW):** - Added integration test: request_body_size_limit_rejects_large_payloads() - Verifies 413 Payload Too Large response for >10MB request bodies - Tests the DefaultBodyLimit middleware enforcement - Location: tools_rust/mcp_runtime/src/lib.rs:13309-13338 All changes maintain backward compatibility except for stricter CIDR validation (fail-closed is more secure). Addresses reviewer feedback from lucarlig on PR #4111 Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com>
10 tasks
3 tasks
jonpspri
added a commit
that referenced
this pull request
May 6, 2026
…view passes Bundle of low-risk, non-blocking improvements that surfaced during the deep review iterations. Each item below was explicitly classified as fix-now in the review summary; broader / design-dependent items were filed as follow-ups under #4612 and #4613. * Schema parity (#1): add grpc_service_id to ToolRead in schemas.py alongside the existing gateway_id field so API consumers can identify gRPC-discovered tools (review S2.4 / TD1). * db.py cleanup (#2): drop redundant nullable=True from Tool.grpc_service_id; Mapped[Optional[str]] already implies it and the sibling gateway_id column omits it (review TD2). * translate_grpc.load_file_descriptors (#3): widen the parameter type from List[bytes] to Sequence[bytes] and reject a single bytes object passed by mistake. Without the guard Python would silently iterate byte-by-byte (review TD3). * translate_grpc descriptor pool conflict (#4): replace the inaccurate 'no-op if already added' comment with an explicit TypeError handler. protobuf raises TypeError when a file with the same name has conflicting content; the existing descriptor stays authoritative (review S3.3). * test_sync_tools_removes_stale_tools (#5): replace the brittle string-match assertion ('DELETE' in str(call)) with an explicit call_count == 4 (1 select + 3 deletes), which no longer depends on the SQLAlchemy Delete object repr (review S2.6). * translate_grpc close() (#6): convert the f-string log to lazy %-style for consistency with the rest of this PR's logging (review N2.1). * grpc_service _sync_tools_from_reflection (#7): document why input_schema['properties'] is empty by design — gRPC arg shape is validated at the protobuf invocation layer, not the MCP tool-call layer; the actual proto types live in the x-grpc-* extensions (review N2.2). * TestInvokeMethodGuards (#8): add 5 edge-case tests covering invoke_method paths the previous suite did not exercise: - service-not-found -> GrpcServiceNotFoundError - disabled service -> GrpcServiceError('is disabled') - invalid method format (no dot) -> GrpcServiceError - _validate_grpc_target spy called with service.target - _validate_tls_path spy called for both cert and key paths 525 targeted tests pass (was 520). Lint clean. Branch: GrpcMethodsAsTools-2854 Signed-off-by: Jonathan Springer <jps@s390x.com>
jonpspri
added a commit
that referenced
this pull request
May 6, 2026
…view passes Bundle of low-risk, non-blocking improvements that surfaced during the deep review iterations. Each item below was explicitly classified as fix-now in the review summary; broader / design-dependent items were filed as follow-ups under #4612 and #4613. * Schema parity (#1): add grpc_service_id to ToolRead in schemas.py alongside the existing gateway_id field so API consumers can identify gRPC-discovered tools (review S2.4 / TD1). * db.py cleanup (#2): drop redundant nullable=True from Tool.grpc_service_id; Mapped[Optional[str]] already implies it and the sibling gateway_id column omits it (review TD2). * translate_grpc.load_file_descriptors (#3): widen the parameter type from List[bytes] to Sequence[bytes] and reject a single bytes object passed by mistake. Without the guard Python would silently iterate byte-by-byte (review TD3). * translate_grpc descriptor pool conflict (#4): replace the inaccurate 'no-op if already added' comment with an explicit TypeError handler. protobuf raises TypeError when a file with the same name has conflicting content; the existing descriptor stays authoritative (review S3.3). * test_sync_tools_removes_stale_tools (#5): replace the brittle string-match assertion ('DELETE' in str(call)) with an explicit call_count == 4 (1 select + 3 deletes), which no longer depends on the SQLAlchemy Delete object repr (review S2.6). * translate_grpc close() (#6): convert the f-string log to lazy %-style for consistency with the rest of this PR's logging (review N2.1). * grpc_service _sync_tools_from_reflection (#7): document why input_schema['properties'] is empty by design — gRPC arg shape is validated at the protobuf invocation layer, not the MCP tool-call layer; the actual proto types live in the x-grpc-* extensions (review N2.2). * TestInvokeMethodGuards (#8): add 5 edge-case tests covering invoke_method paths the previous suite did not exercise: - service-not-found -> GrpcServiceNotFoundError - disabled service -> GrpcServiceError('is disabled') - invalid method format (no dot) -> GrpcServiceError - _validate_grpc_target spy called with service.target - _validate_tls_path spy called for both cert and key paths 525 targeted tests pass (was 520). Lint clean. Branch: GrpcMethodsAsTools-2854 Signed-off-by: Jonathan Springer <jps@s390x.com>
prakhar-singh1928
added a commit
that referenced
this pull request
Jul 31, 2026
Duration unit (finding #3): - Rename cpex.control.duration -> cpex.control.duration_ns in aggregate(), _per_control_attributes(), and both sinks to follow OTel unit-suffix convention. Rename rules targeting 'cpex.control.duration_ns' are now semantically correct. Missing ControlExecutionRecord fields (finding #2): - Add cpex.control.plugin_id, cpex.control.plugin_kind, cpex.control.matched, cpex.control.applied, cpex.control.payload_modified to per-control result spans. - Add cpex.control.result.requested_allowed (emitted only when requested_allow != None). results_count semantics (finding #6): - Fix aggregate() results_count to reflect min(accumulated, max_results) — the number of records exported after the per-invocation cap, per #5785 spec. invocation_count semantics (finding #6): - Count only controls that actually ran (completed/error/timeout); exclude skipped/disabled/cancelled per #5785 aggregation spec. Timeout post-hook gap (finding #5): - Add ctl_acc parameter to _run_timeout_post_invoke(); all four call sites now pass ctl_acc=_ctl_acc so timeout post-hook execution records feed the per-invocation accumulator and appear in the emitted telemetry. Fail-closed ValueError fallback (finding #8b): - apply_attribute_mapping() now returns attributes unchanged when compile_attribute_policy raises ValueError (e.g. otel.* destination, empty key, key > 256 chars). Previously it constructed partially-validated exact mappings that could bypass the otel.* guard — now fail-closed. CPEX denial path documentation (finding #4): - Add explanatory comment in except (PluginError, PluginViolationError) block documenting the upstream CPEX 0.1.2 framework gap: when violations_as_exceptions=True, CPEX raises before appending the denying plugin's ControlExecutionRecord, so _ctl_acc captures pre-denial records only. The effective_allow=False outcome is preserved via pre_denied flag. Tests updated for all changes above; 741 passed, 33 skipped. Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
prakhar-singh1928
added a commit
that referenced
this pull request
Jul 31, 2026
Duration unit (finding #3): - Rename cpex.control.duration -> cpex.control.duration_ns in aggregate(), _per_control_attributes(), and both sinks to follow OTel unit-suffix convention. Rename rules targeting 'cpex.control.duration_ns' are now semantically correct. Missing ControlExecutionRecord fields (finding #2): - Add cpex.control.plugin_id, cpex.control.plugin_kind, cpex.control.matched, cpex.control.applied, cpex.control.payload_modified to per-control result spans. - Add cpex.control.result.requested_allowed (emitted only when requested_allow != None). results_count semantics (finding #6): - Fix aggregate() results_count to reflect min(accumulated, max_results) — the number of records exported after the per-invocation cap, per #5785 spec. invocation_count semantics (finding #6): - Count only controls that actually ran (completed/error/timeout); exclude skipped/disabled/cancelled per #5785 aggregation spec. Timeout post-hook gap (finding #5): - Add ctl_acc parameter to _run_timeout_post_invoke(); all four call sites now pass ctl_acc=_ctl_acc so timeout post-hook execution records feed the per-invocation accumulator and appear in the emitted telemetry. Fail-closed ValueError fallback (finding #8b): - apply_attribute_mapping() now returns attributes unchanged when compile_attribute_policy raises ValueError (e.g. otel.* destination, empty key, key > 256 chars). Previously it constructed partially-validated exact mappings that could bypass the otel.* guard — now fail-closed. CPEX denial path documentation (finding #4): - Add explanatory comment in except (PluginError, PluginViolationError) block documenting the upstream CPEX 0.1.2 framework gap: when violations_as_exceptions=True, CPEX raises before appending the denying plugin's ControlExecutionRecord, so _ctl_acc captures pre-denial records only. The effective_allow=False outcome is preserved via pre_denied flag. Tests updated for all changes above; 741 passed, 33 skipped. Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
15 tasks
prakhar-singh1928
added a commit
that referenced
this pull request
Jul 31, 2026
Duration unit (finding #3): - Rename cpex.control.duration -> cpex.control.duration_ns in aggregate(), _per_control_attributes(), and both sinks to follow OTel unit-suffix convention. Rename rules targeting 'cpex.control.duration_ns' are now semantically correct. Missing ControlExecutionRecord fields (finding #2): - Add cpex.control.plugin_id, cpex.control.plugin_kind, cpex.control.matched, cpex.control.applied, cpex.control.payload_modified to per-control result spans. - Add cpex.control.result.requested_allowed (emitted only when requested_allow != None). results_count semantics (finding #6): - Fix aggregate() results_count to reflect min(accumulated, max_results) — the number of records exported after the per-invocation cap, per #5785 spec. invocation_count semantics (finding #6): - Count only controls that actually ran (completed/error/timeout); exclude skipped/disabled/cancelled per #5785 aggregation spec. Timeout post-hook gap (finding #5): - Add ctl_acc parameter to _run_timeout_post_invoke(); all four call sites now pass ctl_acc=_ctl_acc so timeout post-hook execution records feed the per-invocation accumulator and appear in the emitted telemetry. Fail-closed ValueError fallback (finding #8b): - apply_attribute_mapping() now returns attributes unchanged when compile_attribute_policy raises ValueError (e.g. otel.* destination, empty key, key > 256 chars). Previously it constructed partially-validated exact mappings that could bypass the otel.* guard — now fail-closed. CPEX denial path documentation (finding #4): - Add explanatory comment in except (PluginError, PluginViolationError) block documenting the upstream CPEX 0.1.2 framework gap: when violations_as_exceptions=True, CPEX raises before appending the denying plugin's ControlExecutionRecord, so _ctl_acc captures pre-denial records only. The effective_allow=False outcome is preserved via pre_denied flag. Tests updated for all changes above; 741 passed, 33 skipped. Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
prakhar-singh1928
added a commit
that referenced
this pull request
Aug 4, 2026
Duration unit (finding #3): - Rename cpex.control.duration -> cpex.control.duration_ns in aggregate(), _per_control_attributes(), and both sinks to follow OTel unit-suffix convention. Rename rules targeting 'cpex.control.duration_ns' are now semantically correct. Missing ControlExecutionRecord fields (finding #2): - Add cpex.control.plugin_id, cpex.control.plugin_kind, cpex.control.matched, cpex.control.applied, cpex.control.payload_modified to per-control result spans. - Add cpex.control.result.requested_allowed (emitted only when requested_allow != None). results_count semantics (finding #6): - Fix aggregate() results_count to reflect min(accumulated, max_results) — the number of records exported after the per-invocation cap, per #5785 spec. invocation_count semantics (finding #6): - Count only controls that actually ran (completed/error/timeout); exclude skipped/disabled/cancelled per #5785 aggregation spec. Timeout post-hook gap (finding #5): - Add ctl_acc parameter to _run_timeout_post_invoke(); all four call sites now pass ctl_acc=_ctl_acc so timeout post-hook execution records feed the per-invocation accumulator and appear in the emitted telemetry. Fail-closed ValueError fallback (finding #8b): - apply_attribute_mapping() now returns attributes unchanged when compile_attribute_policy raises ValueError (e.g. otel.* destination, empty key, key > 256 chars). Previously it constructed partially-validated exact mappings that could bypass the otel.* guard — now fail-closed. CPEX denial path documentation (finding #4): - Add explanatory comment in except (PluginError, PluginViolationError) block documenting the upstream CPEX 0.1.2 framework gap: when violations_as_exceptions=True, CPEX raises before appending the denying plugin's ControlExecutionRecord, so _ctl_acc captures pre-denial records only. The effective_allow=False outcome is preserved via pre_denied flag. Tests updated for all changes above; 741 passed, 33 skipped. Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
prakhar-singh1928
added a commit
that referenced
this pull request
Aug 4, 2026
Duration unit (finding #3): - Rename cpex.control.duration -> cpex.control.duration_ns in aggregate(), _per_control_attributes(), and both sinks to follow OTel unit-suffix convention. Rename rules targeting 'cpex.control.duration_ns' are now semantically correct. Missing ControlExecutionRecord fields (finding #2): - Add cpex.control.plugin_id, cpex.control.plugin_kind, cpex.control.matched, cpex.control.applied, cpex.control.payload_modified to per-control result spans. - Add cpex.control.result.requested_allowed (emitted only when requested_allow != None). results_count semantics (finding #6): - Fix aggregate() results_count to reflect min(accumulated, max_results) — the number of records exported after the per-invocation cap, per #5785 spec. invocation_count semantics (finding #6): - Count only controls that actually ran (completed/error/timeout); exclude skipped/disabled/cancelled per #5785 aggregation spec. Timeout post-hook gap (finding #5): - Add ctl_acc parameter to _run_timeout_post_invoke(); all four call sites now pass ctl_acc=_ctl_acc so timeout post-hook execution records feed the per-invocation accumulator and appear in the emitted telemetry. Fail-closed ValueError fallback (finding #8b): - apply_attribute_mapping() now returns attributes unchanged when compile_attribute_policy raises ValueError (e.g. otel.* destination, empty key, key > 256 chars). Previously it constructed partially-validated exact mappings that could bypass the otel.* guard — now fail-closed. CPEX denial path documentation (finding #4): - Add explanatory comment in except (PluginError, PluginViolationError) block documenting the upstream CPEX 0.1.2 framework gap: when violations_as_exceptions=True, CPEX raises before appending the denying plugin's ControlExecutionRecord, so _ctl_acc captures pre-denial records only. The effective_allow=False outcome is preserved via pre_denied flag. Tests updated for all changes above; 741 passed, 33 skipped. Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
10 tasks
prakhar-singh1928
added a commit
that referenced
this pull request
Aug 4, 2026
Duration unit (finding #3): - Rename cpex.control.duration -> cpex.control.duration_ns in aggregate(), _per_control_attributes(), and both sinks to follow OTel unit-suffix convention. Rename rules targeting 'cpex.control.duration_ns' are now semantically correct. Missing ControlExecutionRecord fields (finding #2): - Add cpex.control.plugin_id, cpex.control.plugin_kind, cpex.control.matched, cpex.control.applied, cpex.control.payload_modified to per-control result spans. - Add cpex.control.result.requested_allowed (emitted only when requested_allow != None). results_count semantics (finding #6): - Fix aggregate() results_count to reflect min(accumulated, max_results) — the number of records exported after the per-invocation cap, per #5785 spec. invocation_count semantics (finding #6): - Count only controls that actually ran (completed/error/timeout); exclude skipped/disabled/cancelled per #5785 aggregation spec. Timeout post-hook gap (finding #5): - Add ctl_acc parameter to _run_timeout_post_invoke(); all four call sites now pass ctl_acc=_ctl_acc so timeout post-hook execution records feed the per-invocation accumulator and appear in the emitted telemetry. Fail-closed ValueError fallback (finding #8b): - apply_attribute_mapping() now returns attributes unchanged when compile_attribute_policy raises ValueError (e.g. otel.* destination, empty key, key > 256 chars). Previously it constructed partially-validated exact mappings that could bypass the otel.* guard — now fail-closed. CPEX denial path documentation (finding #4): - Add explanatory comment in except (PluginError, PluginViolationError) block documenting the upstream CPEX 0.1.2 framework gap: when violations_as_exceptions=True, CPEX raises before appending the denying plugin's ControlExecutionRecord, so _ctl_acc captures pre-denial records only. The effective_allow=False outcome is preserved via pre_denied flag. Tests updated for all changes above; 741 passed, 33 skipped. Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
prakhar-singh1928
added a commit
that referenced
this pull request
Aug 4, 2026
Duration unit (finding #3): - Rename cpex.control.duration -> cpex.control.duration_ns in aggregate(), _per_control_attributes(), and both sinks to follow OTel unit-suffix convention. Rename rules targeting 'cpex.control.duration_ns' are now semantically correct. Missing ControlExecutionRecord fields (finding #2): - Add cpex.control.plugin_id, cpex.control.plugin_kind, cpex.control.matched, cpex.control.applied, cpex.control.payload_modified to per-control result spans. - Add cpex.control.result.requested_allowed (emitted only when requested_allow != None). results_count semantics (finding #6): - Fix aggregate() results_count to reflect min(accumulated, max_results) — the number of records exported after the per-invocation cap, per #5785 spec. invocation_count semantics (finding #6): - Count only controls that actually ran (completed/error/timeout); exclude skipped/disabled/cancelled per #5785 aggregation spec. Timeout post-hook gap (finding #5): - Add ctl_acc parameter to _run_timeout_post_invoke(); all four call sites now pass ctl_acc=_ctl_acc so timeout post-hook execution records feed the per-invocation accumulator and appear in the emitted telemetry. Fail-closed ValueError fallback (finding #8b): - apply_attribute_mapping() now returns attributes unchanged when compile_attribute_policy raises ValueError (e.g. otel.* destination, empty key, key > 256 chars). Previously it constructed partially-validated exact mappings that could bypass the otel.* guard — now fail-closed. CPEX denial path documentation (finding #4): - Add explanatory comment in except (PluginError, PluginViolationError) block documenting the upstream CPEX 0.1.2 framework gap: when violations_as_exceptions=True, CPEX raises before appending the denying plugin's ControlExecutionRecord, so _ctl_acc captures pre-denial records only. The effective_allow=False outcome is preserved via pre_denied flag. Tests updated for all changes above; 741 passed, 33 skipped. Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
christophercolumbusdog
pushed a commit
to christophercolumbusdog/mcp-context-forge
that referenced
this pull request
Aug 4, 2026
* feat: consume Cpex control execution records Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * fix(plugins): suppress pylint unused-import on cpex feature-detection probe Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * style: align inline comments in control_telemetry.py Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * test(plugins): add targeted coverage tests for control-telemetry and cpex-compat - Hoist all imports to top-level (no inline imports inside test methods) - Add TestAggregateExceptionPath: covers aggregate() inner except path (lines 192-193) - Add TestGetMaxResultsExceptionPath: covers _get_max_results() except branch (lines 484-485) - Add TestBuildFlattenedAttributesEdgeCases: error_code branch, per-record except, outer except (lines 553-555, 561-563) - Add TestEmitDbSpansExceptionPaths: empty-attrs skip (line 327), rollback on start_span failure (lines 342-348), close() failure swallowed (lines 353-354) - Add TestEmitOtelSpansActivePath: verifies OTel summary+result spans emitted when otel_tracing_enabled+otel_context_active (lines 379-387) - Add TestRecordControlTelemetryTruncated: truncated attribute present (line 260) - Add TestRecordControlTelemetryFlatten: flatten_results branch (lines 267-268) - Add TestExecutionRecordsSupportedImportError: ImportError branch (lines 51-52) - Add wildcard rule tests in TestWildcardAttributeMapping: key>256 rejection (line 535), wildcard/exact removal compilation (lines 543-551), segment extraction mismatch (line 593), ValueError fallback (lines 657-660) - Add TestConcurrencyIsolation and TestObservabilityServiceAPICompatibility Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * fix(plugins): address review findings from issue IBM#5785 PR IBM#6003 Duration unit (finding IBM#3): - Rename cpex.control.duration -> cpex.control.duration_ns in aggregate(), _per_control_attributes(), and both sinks to follow OTel unit-suffix convention. Rename rules targeting 'cpex.control.duration_ns' are now semantically correct. Missing ControlExecutionRecord fields (finding IBM#2): - Add cpex.control.plugin_id, cpex.control.plugin_kind, cpex.control.matched, cpex.control.applied, cpex.control.payload_modified to per-control result spans. - Add cpex.control.result.requested_allowed (emitted only when requested_allow != None). results_count semantics (finding IBM#6): - Fix aggregate() results_count to reflect min(accumulated, max_results) — the number of records exported after the per-invocation cap, per IBM#5785 spec. invocation_count semantics (finding IBM#6): - Count only controls that actually ran (completed/error/timeout); exclude skipped/disabled/cancelled per IBM#5785 aggregation spec. Timeout post-hook gap (finding IBM#5): - Add ctl_acc parameter to _run_timeout_post_invoke(); all four call sites now pass ctl_acc=_ctl_acc so timeout post-hook execution records feed the per-invocation accumulator and appear in the emitted telemetry. Fail-closed ValueError fallback (finding #8b): - apply_attribute_mapping() now returns attributes unchanged when compile_attribute_policy raises ValueError (e.g. otel.* destination, empty key, key > 256 chars). Previously it constructed partially-validated exact mappings that could bypass the otel.* guard — now fail-closed. CPEX denial path documentation (finding IBM#4): - Add explanatory comment in except (PluginError, PluginViolationError) block documenting the upstream CPEX 0.1.2 framework gap: when violations_as_exceptions=True, CPEX raises before appending the denying plugin's ControlExecutionRecord, so _ctl_acc captures pre-denial records only. The effective_allow=False outcome is preserved via pre_denied flag. Tests updated for all changes above; 741 passed, 33 skipped. Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * fix gaps Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * fix(plugins): account for tier-3 export-cap drops in cpex.control.truncated Records beyond CPEX_CONTROL_TELEMETRY_MAX_RESULTS were silently discarded by islice() in _emit_db_spans() and _emit_otel_spans() without being counted in the cpex.control.truncated summary attribute, leaving downstream operators with an incomplete truncation indicator. Fix: - Add _export_cap_dropped field to ControlTelemetryAccumulator - Add mark_export_cap_dropped(count) method called once per emit in record_control_telemetry() before building the summary span - truncated property now returns _truncated + _export_cap_dropped covering all three tiers: Tier 1: per-hook cap (MAX_RECORDS_PER_HOOK = 64) Tier 2: per-call accumulation cap (MAX_RECORDS_PER_CALL = 128) Tier 3: export cap (CPEX_CONTROL_TELEMETRY_MAX_RESULTS, default 32) Tests: - Add TestExportCapTruncation (5 tests) in test_control_telemetry.py - Update TestRecordControlTelemetryTruncated to assert correct total across all three tiers (2 tier-1/2 drops + 96 tier-3 drops = 98) Relates to IBM#5785 Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * fix linting Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * fix(plugins): address blocking review findings on cpex control telemetry PII / agent.id (blocking): - Gate cpex.control.agent.id behind CPEX_CONTROL_TELEMETRY_EMIT_AGENT_ID (default false). Adds _emit_agent_id_enabled() helper and new config field with explicit PII/GDPR warning in description and .env.example. config_keys amplification (functionally impacting): - Add _MAX_CONFIG_KEY_LEN=128 per-key cap and _MAX_CONFIG_KEYS_JOINED_LEN=4096 total joined-string cap. Applied in both _per_control_attributes() and _build_flattened_attributes() to prevent large/adversarial key names causing telemetry amplification or data leakage. Default-off (functionally impacting): - Change CPEX_CONTROL_TELEMETRY_ENABLED default from true to false. Each traced tool call creates up to 1 summary + MAX_RESULTS result DB spans; operators must explicitly opt in after reviewing storage/cardinality impact. E2E tests set CPEX_CONTROL_TELEMETRY_ENABLED=true explicitly via os.environ. E2E denial-path tests (test coverage): - Add denying_plugin_app fixture: PIIFilter with block_on_detection=true and enforcement_mode=pre_invoke. - Add TestDenialPathTelemetry with two tests: test_pre_invoke_denial_summary_span_allowed_false: verifies result.allowed=false and enforcement_point=pre when a real plugin denies on pre_invoke. test_pre_invoke_denial_tool_never_executed: verifies tool arguments do not leak into control telemetry attributes on the denial path. Both tests skip gracefully when PIIFilter does not detect PII (environment variation). Unit test fixes: - Add _enabled_settings() helper in test_record_control_telemetry.py so all TestRecordControlTelemetryDB tests work correctly with the new default=false. - Patch cfg_mod.settings in all DB-sink tests and TestRecordControlTelemetryTruncated. Relates to IBM#5785, PR IBM#6003 Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * addressed comments Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * fixed secrets Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * fix(plugins): address four follow-up review findings on cpex control telemetry Fix 1 — wildcard insertion order corrupted by cache key sorting: - apply_attribute_mapping() was building its cache key with sorted(mapping.items()), which reordered wildcard rules and changed which match fired first. The documented contract is 'first matching wildcard wins' (compile_attribute_policy evaluates rules in insertion order). Changed to tuple(mapping.items()) to preserve the caller's declared rule order. - Regression test: TestWildcardInsertionOrderPreserved (3 cases) in test_record_control_telemetry.py. Fix 2 — config_keys joined cap was char-based, not byte-aware: - Both projections (_per_control_attributes and _build_flattened_attributes) used joined[:_MAX_CONFIG_KEYS_JOINED_LEN] which counts Unicode code points, not UTF-8 bytes. Multibyte key names (e.g. CJK characters) could therefore exceed the intended byte budget. - Fixed both sites to use _safe_str(joined, _MAX_CONFIG_KEYS_JOINED_LEN) which is already byte-aware (encodes to UTF-8 before slicing). - Regression tests: TestConfigKeysByteCapAware (2 cases) covering multibyte input and ASCII round-trip. Fix 3 — _enforcement_point returned 'none' on denial with empty records: - When CPEX raises PluginViolationError with violations_as_exceptions=True it does so before appending a ControlExecutionRecord, so the accumulator can have no records even though a pre-hook fired. _enforcement_point was only inspecting acc.records and returned 'none' in this case. - Fixed to also check acc.pre_denied / acc.post_denied flags so a first-plugin pre-invoke denial correctly reports enforcement_point='pre'. - E2E denial test tightened: removed 'none' from accepted enforcement_point values; removed pytest.skip on missing summary span (absence is a regression); removed 500 from accepted status codes (indicates unhandled exception). - Unit tests: TestEnforcementPointDenialFlags (3 cases). Fix 4 — PluginError on first plugin emits no telemetry: - When the first plugin in the chain raises PluginError the accumulator has no records and no denial flags, so record_control_telemetry() returned early at the empty-accumulator guard, emitting nothing. Plugin outages were invisible in observability dashboards. - Added _plugin_errored field and mark_plugin_error() method to ControlTelemetryAccumulator. The outer except PluginError handler in tool_service.py now calls mark_plugin_error() before record_control_telemetry(). - Empty-accumulator guard now includes plugin_errored so first-plugin failures always emit a summary span. - aggregate() emits cpex.control.plugin_error=True when the flag is set, making plugin outages distinguishable from both successful allows and policy denials. - effective_allowed remains True on PluginError (intentional — outage != denial). - Unit tests: TestMarkPluginError (6 cases) in test_control_telemetry.py. 155 unit tests pass; ruff and pylint 10.00/10. Relates to IBM#5785, PR IBM#6003 Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * fix(plugins): address blocking and warning findings from ja8zyjits review Blocking 1: CHANGELOG.md CPEX_CONTROL_TELEMETRY_ENABLED default 'true' → 'false' to match mcpgateway/config.py:1987 (default=False). Operators would have expected the feature on by default when it is off. Blocking 2: mark_export_cap_dropped() idempotency — changed from += to a first-write-wins assignment guarded by _export_cap_dropped == 0. Subsequent calls on the same accumulator are now no-ops, preventing cpex.control.truncated inflation on retry paths or test reuse. Two regression tests added (TestExportCapTruncation::test_mark_export_cap_dropped_is_idempotent and ::test_mark_export_cap_dropped_zero_then_positive). Warning 1: Replaced undefined 'S4 principles' reference in control_telemetry.py module docstring with an explicit description: 'bounded cardinality and field-level sanitization ... _safe_str/_safe_num helpers and field-name allowlists' pointing to plugins/utils.py. Warning 2: Extracted the 5 repeated record_control_telemetry() call blocks in tool_service.py::invoke_tool() into a single nested helper _emit_ctl_telemetry() that closes over _ctl_acc, name, app_user_email, user_email, gateway_name, and server_id. Identity mapping rationale previously inline at the normal-return site is now in the helper docstring; exception-path sites are reduced to a single _emit_ctl_telemetry() call with a back-reference comment. 145 plugin unit tests pass; ruff all-clear; pylint 10.00/10. Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * fix(plugins): sanitize config_keys and make plugin_error result.allowed indeterminate config_keys sanitization (vishu-bh finding): - Add _sanitize_config_key() helper that validates each key against _CONFIG_KEY_RE (^[A-Za-z0-9_.-]{1,64}$). Rejects commas (the CSV delimiter), CR/LF/NUL/TAB (log-injection vectors), non-ASCII, and secret-shaped tokens containing '='. Keys that fail are dropped entirely rather than truncated, preventing ambiguous joined output. - Both _per_control_attributes() and _build_flattened_attributes() now use the validator. When every key in a record fails, the attribute is omitted rather than emitting an empty string. - 14 new tests in TestSanitizeConfigKey: comma, CR, LF, NUL, TAB, non-ASCII, secret-marker, empty, oversized, max-length, and three end-to-end tests through _per_control_attributes(). PluginError indeterminate decision (vishu-bh finding): - mark_plugin_error() now accepts hook='pre'|'post'|'' and stores it in _plugin_error_hook for enforcement_point derivation. - aggregate() omits cpex.control.result.allowed entirely when plugin_errored=True and no denial flag is set (decision is indeterminate — the chain did not complete). When both plugin_errored and a denial flag are set, denial takes precedence and result.allowed is emitted as False. - _enforcement_point() gains a third fallback tier: plugin_error_hook, so a first-plugin crash still reports the correct enforcement point rather than 'none'. - tool_service.py: _ctl_last_hook tracks 'pre'/'post' across all invoke sites; outer except PluginError passes hook=_ctl_last_hook. - 9 new tests in TestPluginErrorIndeterminate. - Updated TestMarkPluginError::test_plugin_error_flag_bypasses... to assert result.allowed is ABSENT (was: True) for the first-plugin error path. 849 plugin unit tests pass; ruff all-clear; pylint 10.00/10. Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * fixed ruff format Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * fix: tolerate stale merge-queue ref on SARIF upload When a merge-queue attempt is ejected (e.g. transient Docker cache error) the ephemeral ref refs/heads/gh-readonly-queue/main/pr-* is deleted. Any retried attempt carries the same stale ref baked into the job context, so codeql-action/upload-sarif fails with 'ref not found', failing fedramp-compliance and scan jobs and ejecting the queue again -- workflow_run_attempt 1, 2, and 3 all hit this loop. Add continue-on-error: true to both Upload SARIF to CodeQL steps so a stale-ref rejection from the CodeQL API does not fail the job. The scan result is unaffected; SARIF lands on the push-to-main run that follows a successful merge. Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * update secrets Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * undo .yml change Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> --------- Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
8 tasks
13 tasks
rakdutta
added a commit
that referenced
this pull request
Aug 12, 2026
- fix(vault): add _enforce_gateway_access() to vault_authorize to enforce gateway visibility/team-membership checks before initiating OAuth flow; without this any authenticated user could start an OAuth flow against a private gateway they are not a member of (Finding #1) - test(vault): add deny-path security regression tests for vault_router covering wrong-team, public-only token, server-not-found, no-oauth-gateways, valid-member, and admin scenarios (AGENTS.md requirement) - fix(oauth): revert fetch_tools_after_oauth detail=f'...{e}' back to generic 'Failed to fetch tools' to avoid leaking internal hostnames/token audience/ scope names to callers; add exc_info=True to GatewayConnectionError branch so full detail reaches operator logs (Finding #4) - fix(oauth): correct misleading comment on callback-minted session JWT; document admin-bypass behaviour and jwt_teams_claim path selection (Finding #2) - fix(db): repoint migration 12d4a0c7789c down_revision from c9f8e7d6a4b3 to e4f5a6b7c8d9 (current main head) to resolve multiple-heads error (Finding #3) Closes #5599 review findings #1 #2 #3 #4 Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com>
9 tasks
rakdutta
added a commit
that referenced
this pull request
Aug 13, 2026
- fix(vault): add _enforce_gateway_access() to vault_authorize to enforce gateway visibility/team-membership checks before initiating OAuth flow; without this any authenticated user could start an OAuth flow against a private gateway they are not a member of (Finding #1) - test(vault): add deny-path security regression tests for vault_router covering wrong-team, public-only token, server-not-found, no-oauth-gateways, valid-member, and admin scenarios (AGENTS.md requirement) - fix(oauth): revert fetch_tools_after_oauth detail=f'...{e}' back to generic 'Failed to fetch tools' to avoid leaking internal hostnames/token audience/ scope names to callers; add exc_info=True to GatewayConnectionError branch so full detail reaches operator logs (Finding #4) - fix(oauth): correct misleading comment on callback-minted session JWT; document admin-bypass behaviour and jwt_teams_claim path selection (Finding #2) - fix(db): repoint migration 12d4a0c7789c down_revision from c9f8e7d6a4b3 to e4f5a6b7c8d9 (current main head) to resolve multiple-heads error (Finding #3) Closes #5599 review findings #1 #2 #3 #4 Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com>
rakdutta
added a commit
that referenced
this pull request
Aug 14, 2026
- fix(vault): add _enforce_gateway_access() to vault_authorize to enforce gateway visibility/team-membership checks before initiating OAuth flow; without this any authenticated user could start an OAuth flow against a private gateway they are not a member of (Finding #1) - test(vault): add deny-path security regression tests for vault_router covering wrong-team, public-only token, server-not-found, no-oauth-gateways, valid-member, and admin scenarios (AGENTS.md requirement) - fix(oauth): revert fetch_tools_after_oauth detail=f'...{e}' back to generic 'Failed to fetch tools' to avoid leaking internal hostnames/token audience/ scope names to callers; add exc_info=True to GatewayConnectionError branch so full detail reaches operator logs (Finding #4) - fix(oauth): correct misleading comment on callback-minted session JWT; document admin-bypass behaviour and jwt_teams_claim path selection (Finding #2) - fix(db): repoint migration 12d4a0c7789c down_revision from c9f8e7d6a4b3 to e4f5a6b7c8d9 (current main head) to resolve multiple-heads error (Finding #3) Closes #5599 review findings #1 #2 #3 #4 Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com>
rakdutta
added a commit
that referenced
this pull request
Aug 14, 2026
- fix(vault): add _enforce_gateway_access() to vault_authorize to enforce gateway visibility/team-membership checks before initiating OAuth flow; without this any authenticated user could start an OAuth flow against a private gateway they are not a member of (Finding #1) - test(vault): add deny-path security regression tests for vault_router covering wrong-team, public-only token, server-not-found, no-oauth-gateways, valid-member, and admin scenarios (AGENTS.md requirement) - fix(oauth): revert fetch_tools_after_oauth detail=f'...{e}' back to generic 'Failed to fetch tools' to avoid leaking internal hostnames/token audience/ scope names to callers; add exc_info=True to GatewayConnectionError branch so full detail reaches operator logs (Finding #4) - fix(oauth): correct misleading comment on callback-minted session JWT; document admin-bypass behaviour and jwt_teams_claim path selection (Finding #2) - fix(db): repoint migration 12d4a0c7789c down_revision from c9f8e7d6a4b3 to e4f5a6b7c8d9 (current main head) to resolve multiple-heads error (Finding #3) Closes #5599 review findings #1 #2 #3 #4 Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com>
rakdutta
added a commit
that referenced
this pull request
Aug 17, 2026
- fix(vault): add _enforce_gateway_access() to vault_authorize to enforce gateway visibility/team-membership checks before initiating OAuth flow; without this any authenticated user could start an OAuth flow against a private gateway they are not a member of (Finding #1) - test(vault): add deny-path security regression tests for vault_router covering wrong-team, public-only token, server-not-found, no-oauth-gateways, valid-member, and admin scenarios (AGENTS.md requirement) - fix(oauth): revert fetch_tools_after_oauth detail=f'...{e}' back to generic 'Failed to fetch tools' to avoid leaking internal hostnames/token audience/ scope names to callers; add exc_info=True to GatewayConnectionError branch so full detail reaches operator logs (Finding #4) - fix(oauth): correct misleading comment on callback-minted session JWT; document admin-bypass behaviour and jwt_teams_claim path selection (Finding #2) - fix(db): repoint migration 12d4a0c7789c down_revision from c9f8e7d6a4b3 to e4f5a6b7c8d9 (current main head) to resolve multiple-heads error (Finding #3) Closes #5599 review findings #1 #2 #3 #4 Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com>
rakdutta
added a commit
that referenced
this pull request
Aug 17, 2026
- fix(vault): add _enforce_gateway_access() to vault_authorize to enforce gateway visibility/team-membership checks before initiating OAuth flow; without this any authenticated user could start an OAuth flow against a private gateway they are not a member of (Finding #1) - test(vault): add deny-path security regression tests for vault_router covering wrong-team, public-only token, server-not-found, no-oauth-gateways, valid-member, and admin scenarios (AGENTS.md requirement) - fix(oauth): revert fetch_tools_after_oauth detail=f'...{e}' back to generic 'Failed to fetch tools' to avoid leaking internal hostnames/token audience/ scope names to callers; add exc_info=True to GatewayConnectionError branch so full detail reaches operator logs (Finding #4) - fix(oauth): correct misleading comment on callback-minted session JWT; document admin-bypass behaviour and jwt_teams_claim path selection (Finding #2) - fix(db): repoint migration 12d4a0c7789c down_revision from c9f8e7d6a4b3 to e4f5a6b7c8d9 (current main head) to resolve multiple-heads error (Finding #3) Closes #5599 review findings #1 #2 #3 #4 Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com>
10 tasks
rakdutta
added a commit
that referenced
this pull request
Aug 21, 2026
- fix(vault): add _enforce_gateway_access() to vault_authorize to enforce gateway visibility/team-membership checks before initiating OAuth flow; without this any authenticated user could start an OAuth flow against a private gateway they are not a member of (Finding #1) - test(vault): add deny-path security regression tests for vault_router covering wrong-team, public-only token, server-not-found, no-oauth-gateways, valid-member, and admin scenarios (AGENTS.md requirement) - fix(oauth): revert fetch_tools_after_oauth detail=f'...{e}' back to generic 'Failed to fetch tools' to avoid leaking internal hostnames/token audience/ scope names to callers; add exc_info=True to GatewayConnectionError branch so full detail reaches operator logs (Finding #4) - fix(oauth): correct misleading comment on callback-minted session JWT; document admin-bypass behaviour and jwt_teams_claim path selection (Finding #2) - fix(db): repoint migration 12d4a0c7789c down_revision from c9f8e7d6a4b3 to e4f5a6b7c8d9 (current main head) to resolve multiple-heads error (Finding #3) Closes #5599 review findings #1 #2 #3 #4 Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com>
rakdutta
added a commit
that referenced
this pull request
Aug 21, 2026
- fix(vault): add _enforce_gateway_access() to vault_authorize to enforce gateway visibility/team-membership checks before initiating OAuth flow; without this any authenticated user could start an OAuth flow against a private gateway they are not a member of (Finding #1) - test(vault): add deny-path security regression tests for vault_router covering wrong-team, public-only token, server-not-found, no-oauth-gateways, valid-member, and admin scenarios (AGENTS.md requirement) - fix(oauth): revert fetch_tools_after_oauth detail=f'...{e}' back to generic 'Failed to fetch tools' to avoid leaking internal hostnames/token audience/ scope names to callers; add exc_info=True to GatewayConnectionError branch so full detail reaches operator logs (Finding #4) - fix(oauth): correct misleading comment on callback-minted session JWT; document admin-bypass behaviour and jwt_teams_claim path selection (Finding #2) - fix(db): repoint migration 12d4a0c7789c down_revision from c9f8e7d6a4b3 to e4f5a6b7c8d9 (current main head) to resolve multiple-heads error (Finding #3) Closes #5599 review findings #1 #2 #3 #4 Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com>
msureshkumar88
pushed a commit
that referenced
this pull request
Aug 24, 2026
- fix(vault): add _enforce_gateway_access() to vault_authorize to enforce gateway visibility/team-membership checks before initiating OAuth flow; without this any authenticated user could start an OAuth flow against a private gateway they are not a member of (Finding #1) - test(vault): add deny-path security regression tests for vault_router covering wrong-team, public-only token, server-not-found, no-oauth-gateways, valid-member, and admin scenarios (AGENTS.md requirement) - fix(oauth): revert fetch_tools_after_oauth detail=f'...{e}' back to generic 'Failed to fetch tools' to avoid leaking internal hostnames/token audience/ scope names to callers; add exc_info=True to GatewayConnectionError branch so full detail reaches operator logs (Finding #4) - fix(oauth): correct misleading comment on callback-minted session JWT; document admin-bypass behaviour and jwt_teams_claim path selection (Finding #2) - fix(db): repoint migration 12d4a0c7789c down_revision from c9f8e7d6a4b3 to e4f5a6b7c8d9 (current main head) to resolve multiple-heads error (Finding #3) Closes #5599 review findings #1 #2 #3 #4 Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com>
msureshkumar88
pushed a commit
that referenced
this pull request
Aug 24, 2026
- fix(vault): add _enforce_gateway_access() to vault_authorize to enforce gateway visibility/team-membership checks before initiating OAuth flow; without this any authenticated user could start an OAuth flow against a private gateway they are not a member of (Finding #1) - test(vault): add deny-path security regression tests for vault_router covering wrong-team, public-only token, server-not-found, no-oauth-gateways, valid-member, and admin scenarios (AGENTS.md requirement) - fix(oauth): revert fetch_tools_after_oauth detail=f'...{e}' back to generic 'Failed to fetch tools' to avoid leaking internal hostnames/token audience/ scope names to callers; add exc_info=True to GatewayConnectionError branch so full detail reaches operator logs (Finding #4) - fix(oauth): correct misleading comment on callback-minted session JWT; document admin-bypass behaviour and jwt_teams_claim path selection (Finding #2) - fix(db): repoint migration 12d4a0c7789c down_revision from c9f8e7d6a4b3 to e4f5a6b7c8d9 (current main head) to resolve multiple-heads error (Finding #3) Closes #5599 review findings #1 #2 #3 #4 Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com>
msureshkumar88
pushed a commit
that referenced
this pull request
Aug 24, 2026
- fix(vault): add _enforce_gateway_access() to vault_authorize to enforce gateway visibility/team-membership checks before initiating OAuth flow; without this any authenticated user could start an OAuth flow against a private gateway they are not a member of (Finding #1) - test(vault): add deny-path security regression tests for vault_router covering wrong-team, public-only token, server-not-found, no-oauth-gateways, valid-member, and admin scenarios (AGENTS.md requirement) - fix(oauth): revert fetch_tools_after_oauth detail=f'...{e}' back to generic 'Failed to fetch tools' to avoid leaking internal hostnames/token audience/ scope names to callers; add exc_info=True to GatewayConnectionError branch so full detail reaches operator logs (Finding #4) - fix(oauth): correct misleading comment on callback-minted session JWT; document admin-bypass behaviour and jwt_teams_claim path selection (Finding #2) - fix(db): repoint migration 12d4a0c7789c down_revision from c9f8e7d6a4b3 to e4f5a6b7c8d9 (current main head) to resolve multiple-heads error (Finding #3) Closes #5599 review findings #1 #2 #3 #4 Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com>
brian-hussey
pushed a commit
to brian-hussey/mcp-context-forge
that referenced
this pull request
Aug 24, 2026
…BM#5599) * feat: pluggable OAuth token storage with Database and Vault backends Implement team-scoped OAuth token storage supporting Database and Vault backends with comprehensive test coverage Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * chore: remove vault test files and reset config to main - Remove docker-compose.vault-test.yml and Makefile.vault-tests - Reset .gitignore and pyproject.toml to main branch versions - These files were branch-specific testing artifacts Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * chore: remove oauth_vault_lookup.sql script Remove branch-specific SQL script used for vault testing. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * refactor: use unified /oauth/callback endpoint for both OAuth authorization flows Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test: fix OAuth tests for teams parameter and state_data return fields Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * pre-commit fix Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: correct type annotations for optional Request parameters Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test cases add Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: correct Request parameter ordering in FastAPI routes FastAPI requires Request parameters to come before optional parameters. Moving request parameter to first position in oauth_callback and vault_authorize functions to fix parameter ordering syntax error. Resolves FastAPIError about invalid Request | None field type. Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * feat(vault): resolve per-user creds from Vault for all auth types Extend the Vault OAuth token backend so per-user credentials are read from Vault for non-OAuth auth types too, not just OAuth authorization_code. - VaultTokenBackend.get_user_auth_headers + TokenStorageService facade: read a per-user {header: value} dict stored under a 'headers' field at the same per-user Vault path used for OAuth tokens. - tool_service invoke paths: for non-OAuth gateways, resolve per-user Vault headers FIRST, then fall back to the gateway-wide (admin-set) static auth. Lets ICA write per-user bearer/basic/authheaders creds to Vault which CF injects at invocation, consistent with the OAuth flow. Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> * chore: fix file headers for pre-commit compliance - Add missing encoding declarations - Add missing Location comments - Update Copyright to contributors format - Add missing SPDX-License-Identifier lines - Remove stale Authors attributions per project standards Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * chore: remove temporary documentation and test files Remove working documentation, analysis files, and test scripts that should not be committed to the repository: - Design/analysis documents - Testing guides and summaries - Vault-specific helper scripts - Review artifacts Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: restore manual-test-uaid-cross-gateway-auth.md accidentally deleted Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: remove leftover conflict markers from test_token_storage_service.py Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * rebase issue fix Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test failure Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * chore: suppress false positive secret detection in oauth_router.py Add pragma comment to 'credentials: include' fetch option. Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(oauth): port PR IBM#5244 OAuth health check features to DatabaseTokenBackend after rebase Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(oauth): port PR IBM#5244 features (omit_resource, error handling) to DatabaseTokenBackend and fix tests after rebase Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(oauth): port PR IBM#5244 features to VaultTokenBackend Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(oauth): add unit tests for PR IBM#5244 VaultTokenBackend and refresh_helpers features Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(oauth): add coverage tests for vault_backend edge cases (87% -> 92%) Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(oauth): add store_tokens edge case tests - reach 93% coverage target - Test created_at preservation on token updates (line 298) - Test cache invalidation with cache enabled (lines 324-326) - vault_backend.py: 87% → 89% - Overall token_backends: 92% → 93% ✅ Achieves CI/CD 93% coverage requirement. Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test coverage Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: clean up token_storage_service.py after rebase conflicts Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com> * fix(db): update migration 12d4a0c7789c to point to correct parent c9f8e7d6a4b3 Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(oauth): fix failing tests after rebase - mock EmailUser query and learned_audience Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: update token backend tests and remove duplicate import Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test coverage Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(oauth): address PR IBM#5599 review findings - fix(vault): add _enforce_gateway_access() to vault_authorize to enforce gateway visibility/team-membership checks before initiating OAuth flow; without this any authenticated user could start an OAuth flow against a private gateway they are not a member of (Finding IBM#1) - test(vault): add deny-path security regression tests for vault_router covering wrong-team, public-only token, server-not-found, no-oauth-gateways, valid-member, and admin scenarios (AGENTS.md requirement) - fix(oauth): revert fetch_tools_after_oauth detail=f'...{e}' back to generic 'Failed to fetch tools' to avoid leaking internal hostnames/token audience/ scope names to callers; add exc_info=True to GatewayConnectionError branch so full detail reaches operator logs (Finding IBM#4) - fix(oauth): correct misleading comment on callback-minted session JWT; document admin-bypass behaviour and jwt_teams_claim path selection (Finding IBM#2) - fix(db): repoint migration 12d4a0c7789c down_revision from c9f8e7d6a4b3 to e4f5a6b7c8d9 (current main head) to resolve multiple-heads error (Finding IBM#3) Closes IBM#5599 review findings IBM#1 IBM#2 IBM#3 IBM#4 Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * chore: update uv.lock after pre-commit formatting Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: correct Revises comment from c9f8e7d6a4b3 to e4f5a6b7c8d9 in add_team_id_to_oauth_states migration Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(oauth): fix learned_aud persistence, vault refresh erasure, vault_router team context, manual refresh user_context, and OAuthError info leak (CWE-209) Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * refactor: address non-blocking review comments for pluggable token storage - Add URL encoding for team_id in Vault KV paths to prevent potential path traversal if future callers pass slugs/display names instead of UUIDs - Add backend capability check before DB session creation to avoid unnecessary round-trips on default database backend which doesn't implement per-user auth headers Both changes are performance/security hardening with no functional impact on current callers. Related: PR IBM#5599 review comments (suggestions 1-2) Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * refactor: consolidate OAuth token backend code duplication and fix tests Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(oauth): address round-3 blocking review findings (B2-B5, S6, S8) - B4: hard-fail OAuth callback when identity missing after state consumed (CWE-287) - B2: add ORDER BY to team queries for stable Vault path key - B3: remove cross-path 401 fallback violating IBM#5598 no-dual-backend rule - B5: formalise get_user_auth_headers on AbstractTokenBackend; drop getattr dispatch - S6: decrypt client_secret in VaultTokenBackend before token endpoint call - S8: omit teams claim from JWT when team_id is None to prevent scope widening Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(oauth): update TestGatewayService401Retry to reflect B3 no-fallback behaviour Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * style: fix pre-commit formatting in oauth_router.py (remove spaces in **() expression) Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(vault): address round-4 review findings (B2a–B2g, B3, S10–S13) - B2a: raise OAuthError on client_secret decryption failure (fail-closed) - B2b: add per-key asyncio refresh lock to prevent invalid_grant races - B2c: expire cache entry on revoke instead of deleting (cross-worker safety) - B2d: replace raw str(e) with generic user message in OAuth callback (CWE-209) - B2e: pass popup=True in vault_authorize to avoid overwriting session cookie - B2f: add GET /vault/authorize/{server_id} to _PERMISSION_PATTERNS (GATEWAYS_READ) - B2g: enforce Server row visibility before resolving gateway in vault_authorize - B3: restore two deleted regression tests in test_db_backend.py - S10: add TODO comment documenting Phase 2 wiring for get/store_oauth_credentials - S11: extract duplicated Vault auth-header fallback into _resolve_vault_auth_headers() - S12: standardise store_tokens() return type to TokenRecord across all backends - S13: update security-features.md to reflect VaultTokenBackend implementation status Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(vault): update tests for refresh-lock split and revoke expire-in-place, fix ruff/pylint warnings Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(vault): update tests for refresh-lock split and revoke expire-in-place, fix ruff/pylint warnings Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: address round-6 security review — CWE-863 vault path revocation, fail-closed vault errors, refresh-lock race, expire-in-place cache, read method error handling, revoke distinguishability, server-visibility deny-path test Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: address self-review findings — demote debug logs, fix Optional[str] types, add nosec suppressions, clarify vault router comment Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: pre-commit formatting, update secrets baseline, skip expired sunset test IBM#2754 Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test: skip test_get_redis_client_with_circuit_open — pre-existing failure on main, unrelated to this PR Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(coverage): add unit tests to boost diff coverage for pluggable token storage Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(coverage): add unit tests to boost diff coverage for pluggable token storage Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * pre-commit fix Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: fail-closed vault auth, deterministic gateway order, client_id 400, log sanitization, stale tests Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: sanitize all log args, opaque vault error message, demote debug checks, fix stale test mock Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * style: apply pre-commit black formatting to vault_router, vault_backend, tool_service Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: fail-closed Vault 5xx/4xx errors, guard store_tokens write, and thread jwt_teams_claim for admin token path alignment Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: update _capture_store mock signature to accept redirect_uri after rebase Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * alembic down_revision_change Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: add jwt_teams_claim=None to test_rpc_tool_invocation assertion Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(vault): handle header-only Vault records in get_user_token and cap _refresh_locks size B1: use .get() guards on 'token'/'access_token' keys — returns None instead of KeyError when ICA writes a header-only record at the shared Vault path. B2: evict oldest idle lock in _get_refresh_lock when dict reaches cache_max_size. Held locks are never evicted to preserve refresh serialisation correctness. Add four regression tests covering both fixes Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * chore: regenerate .secrets.baseline after rebase onto main Line-number drift only, no new or removed findings. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com> * fix: re-chain oauth_states team_id migration onto new main head Rebase onto origin/main pulled in d8d7939e73e9 (tool-preview permission), which shares db41939315aa as down_revision with 12d4a0c7789c, producing two alembic heads. Re-point 12d4a0c7789c onto d8d7939e73e9 to restore a single linear head. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com> * chore: regenerate .secrets.baseline to drop stale finding Rerunning the detect-secrets pre-commit hook (not just the make target) dropped a stale mcpgateway/routers/oauth_router.py:933 entry that no longer matches current file content. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com> --------- Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Co-authored-by: Madhav Kandukuri <madhav165@gmail.com> Co-authored-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
gcgoncalves
pushed a commit
that referenced
this pull request
Aug 24, 2026
…5599) * feat: pluggable OAuth token storage with Database and Vault backends Implement team-scoped OAuth token storage supporting Database and Vault backends with comprehensive test coverage Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * chore: remove vault test files and reset config to main - Remove docker-compose.vault-test.yml and Makefile.vault-tests - Reset .gitignore and pyproject.toml to main branch versions - These files were branch-specific testing artifacts Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * chore: remove oauth_vault_lookup.sql script Remove branch-specific SQL script used for vault testing. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * refactor: use unified /oauth/callback endpoint for both OAuth authorization flows Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test: fix OAuth tests for teams parameter and state_data return fields Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * pre-commit fix Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: correct type annotations for optional Request parameters Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test cases add Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: correct Request parameter ordering in FastAPI routes FastAPI requires Request parameters to come before optional parameters. Moving request parameter to first position in oauth_callback and vault_authorize functions to fix parameter ordering syntax error. Resolves FastAPIError about invalid Request | None field type. Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * feat(vault): resolve per-user creds from Vault for all auth types Extend the Vault OAuth token backend so per-user credentials are read from Vault for non-OAuth auth types too, not just OAuth authorization_code. - VaultTokenBackend.get_user_auth_headers + TokenStorageService facade: read a per-user {header: value} dict stored under a 'headers' field at the same per-user Vault path used for OAuth tokens. - tool_service invoke paths: for non-OAuth gateways, resolve per-user Vault headers FIRST, then fall back to the gateway-wide (admin-set) static auth. Lets ICA write per-user bearer/basic/authheaders creds to Vault which CF injects at invocation, consistent with the OAuth flow. Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> * chore: fix file headers for pre-commit compliance - Add missing encoding declarations - Add missing Location comments - Update Copyright to contributors format - Add missing SPDX-License-Identifier lines - Remove stale Authors attributions per project standards Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * chore: remove temporary documentation and test files Remove working documentation, analysis files, and test scripts that should not be committed to the repository: - Design/analysis documents - Testing guides and summaries - Vault-specific helper scripts - Review artifacts Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: restore manual-test-uaid-cross-gateway-auth.md accidentally deleted Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: remove leftover conflict markers from test_token_storage_service.py Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * rebase issue fix Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test failure Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * chore: suppress false positive secret detection in oauth_router.py Add pragma comment to 'credentials: include' fetch option. Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(oauth): port PR #5244 OAuth health check features to DatabaseTokenBackend after rebase Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(oauth): port PR #5244 features (omit_resource, error handling) to DatabaseTokenBackend and fix tests after rebase Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(oauth): port PR #5244 features to VaultTokenBackend Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(oauth): add unit tests for PR #5244 VaultTokenBackend and refresh_helpers features Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(oauth): add coverage tests for vault_backend edge cases (87% -> 92%) Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(oauth): add store_tokens edge case tests - reach 93% coverage target - Test created_at preservation on token updates (line 298) - Test cache invalidation with cache enabled (lines 324-326) - vault_backend.py: 87% → 89% - Overall token_backends: 92% → 93% ✅ Achieves CI/CD 93% coverage requirement. Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test coverage Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: clean up token_storage_service.py after rebase conflicts Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com> * fix(db): update migration 12d4a0c7789c to point to correct parent c9f8e7d6a4b3 Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(oauth): fix failing tests after rebase - mock EmailUser query and learned_audience Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: update token backend tests and remove duplicate import Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test coverage Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(oauth): address PR #5599 review findings - fix(vault): add _enforce_gateway_access() to vault_authorize to enforce gateway visibility/team-membership checks before initiating OAuth flow; without this any authenticated user could start an OAuth flow against a private gateway they are not a member of (Finding #1) - test(vault): add deny-path security regression tests for vault_router covering wrong-team, public-only token, server-not-found, no-oauth-gateways, valid-member, and admin scenarios (AGENTS.md requirement) - fix(oauth): revert fetch_tools_after_oauth detail=f'...{e}' back to generic 'Failed to fetch tools' to avoid leaking internal hostnames/token audience/ scope names to callers; add exc_info=True to GatewayConnectionError branch so full detail reaches operator logs (Finding #4) - fix(oauth): correct misleading comment on callback-minted session JWT; document admin-bypass behaviour and jwt_teams_claim path selection (Finding #2) - fix(db): repoint migration 12d4a0c7789c down_revision from c9f8e7d6a4b3 to e4f5a6b7c8d9 (current main head) to resolve multiple-heads error (Finding #3) Closes #5599 review findings #1 #2 #3 #4 Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * chore: update uv.lock after pre-commit formatting Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: correct Revises comment from c9f8e7d6a4b3 to e4f5a6b7c8d9 in add_team_id_to_oauth_states migration Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(oauth): fix learned_aud persistence, vault refresh erasure, vault_router team context, manual refresh user_context, and OAuthError info leak (CWE-209) Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * refactor: address non-blocking review comments for pluggable token storage - Add URL encoding for team_id in Vault KV paths to prevent potential path traversal if future callers pass slugs/display names instead of UUIDs - Add backend capability check before DB session creation to avoid unnecessary round-trips on default database backend which doesn't implement per-user auth headers Both changes are performance/security hardening with no functional impact on current callers. Related: PR #5599 review comments (suggestions 1-2) Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * refactor: consolidate OAuth token backend code duplication and fix tests Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(oauth): address round-3 blocking review findings (B2-B5, S6, S8) - B4: hard-fail OAuth callback when identity missing after state consumed (CWE-287) - B2: add ORDER BY to team queries for stable Vault path key - B3: remove cross-path 401 fallback violating #5598 no-dual-backend rule - B5: formalise get_user_auth_headers on AbstractTokenBackend; drop getattr dispatch - S6: decrypt client_secret in VaultTokenBackend before token endpoint call - S8: omit teams claim from JWT when team_id is None to prevent scope widening Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(oauth): update TestGatewayService401Retry to reflect B3 no-fallback behaviour Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * style: fix pre-commit formatting in oauth_router.py (remove spaces in **() expression) Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(vault): address round-4 review findings (B2a–B2g, B3, S10–S13) - B2a: raise OAuthError on client_secret decryption failure (fail-closed) - B2b: add per-key asyncio refresh lock to prevent invalid_grant races - B2c: expire cache entry on revoke instead of deleting (cross-worker safety) - B2d: replace raw str(e) with generic user message in OAuth callback (CWE-209) - B2e: pass popup=True in vault_authorize to avoid overwriting session cookie - B2f: add GET /vault/authorize/{server_id} to _PERMISSION_PATTERNS (GATEWAYS_READ) - B2g: enforce Server row visibility before resolving gateway in vault_authorize - B3: restore two deleted regression tests in test_db_backend.py - S10: add TODO comment documenting Phase 2 wiring for get/store_oauth_credentials - S11: extract duplicated Vault auth-header fallback into _resolve_vault_auth_headers() - S12: standardise store_tokens() return type to TokenRecord across all backends - S13: update security-features.md to reflect VaultTokenBackend implementation status Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(vault): update tests for refresh-lock split and revoke expire-in-place, fix ruff/pylint warnings Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(vault): update tests for refresh-lock split and revoke expire-in-place, fix ruff/pylint warnings Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: address round-6 security review — CWE-863 vault path revocation, fail-closed vault errors, refresh-lock race, expire-in-place cache, read method error handling, revoke distinguishability, server-visibility deny-path test Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: address self-review findings — demote debug logs, fix Optional[str] types, add nosec suppressions, clarify vault router comment Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: pre-commit formatting, update secrets baseline, skip expired sunset test #2754 Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test: skip test_get_redis_client_with_circuit_open — pre-existing failure on main, unrelated to this PR Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(coverage): add unit tests to boost diff coverage for pluggable token storage Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(coverage): add unit tests to boost diff coverage for pluggable token storage Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * pre-commit fix Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: fail-closed vault auth, deterministic gateway order, client_id 400, log sanitization, stale tests Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: sanitize all log args, opaque vault error message, demote debug checks, fix stale test mock Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * style: apply pre-commit black formatting to vault_router, vault_backend, tool_service Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: fail-closed Vault 5xx/4xx errors, guard store_tokens write, and thread jwt_teams_claim for admin token path alignment Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: update _capture_store mock signature to accept redirect_uri after rebase Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * alembic down_revision_change Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: add jwt_teams_claim=None to test_rpc_tool_invocation assertion Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(vault): handle header-only Vault records in get_user_token and cap _refresh_locks size B1: use .get() guards on 'token'/'access_token' keys — returns None instead of KeyError when ICA writes a header-only record at the shared Vault path. B2: evict oldest idle lock in _get_refresh_lock when dict reaches cache_max_size. Held locks are never evicted to preserve refresh serialisation correctness. Add four regression tests covering both fixes Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * chore: regenerate .secrets.baseline after rebase onto main Line-number drift only, no new or removed findings. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com> * fix: re-chain oauth_states team_id migration onto new main head Rebase onto origin/main pulled in d8d7939e73e9 (tool-preview permission), which shares db41939315aa as down_revision with 12d4a0c7789c, producing two alembic heads. Re-point 12d4a0c7789c onto d8d7939e73e9 to restore a single linear head. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com> * chore: regenerate .secrets.baseline to drop stale finding Rerunning the detect-secrets pre-commit hook (not just the make target) dropped a stale mcpgateway/routers/oauth_router.py:933 entry that no longer matches current file content. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com> --------- Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Co-authored-by: Madhav Kandukuri <madhav165@gmail.com> Co-authored-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
gcgoncalves
pushed a commit
that referenced
this pull request
Aug 24, 2026
…5599) * feat: pluggable OAuth token storage with Database and Vault backends Implement team-scoped OAuth token storage supporting Database and Vault backends with comprehensive test coverage Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * chore: remove vault test files and reset config to main - Remove docker-compose.vault-test.yml and Makefile.vault-tests - Reset .gitignore and pyproject.toml to main branch versions - These files were branch-specific testing artifacts Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * chore: remove oauth_vault_lookup.sql script Remove branch-specific SQL script used for vault testing. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * refactor: use unified /oauth/callback endpoint for both OAuth authorization flows Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test: fix OAuth tests for teams parameter and state_data return fields Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * pre-commit fix Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: correct type annotations for optional Request parameters Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test cases add Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: correct Request parameter ordering in FastAPI routes FastAPI requires Request parameters to come before optional parameters. Moving request parameter to first position in oauth_callback and vault_authorize functions to fix parameter ordering syntax error. Resolves FastAPIError about invalid Request | None field type. Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * feat(vault): resolve per-user creds from Vault for all auth types Extend the Vault OAuth token backend so per-user credentials are read from Vault for non-OAuth auth types too, not just OAuth authorization_code. - VaultTokenBackend.get_user_auth_headers + TokenStorageService facade: read a per-user {header: value} dict stored under a 'headers' field at the same per-user Vault path used for OAuth tokens. - tool_service invoke paths: for non-OAuth gateways, resolve per-user Vault headers FIRST, then fall back to the gateway-wide (admin-set) static auth. Lets ICA write per-user bearer/basic/authheaders creds to Vault which CF injects at invocation, consistent with the OAuth flow. Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> * chore: fix file headers for pre-commit compliance - Add missing encoding declarations - Add missing Location comments - Update Copyright to contributors format - Add missing SPDX-License-Identifier lines - Remove stale Authors attributions per project standards Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * chore: remove temporary documentation and test files Remove working documentation, analysis files, and test scripts that should not be committed to the repository: - Design/analysis documents - Testing guides and summaries - Vault-specific helper scripts - Review artifacts Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: restore manual-test-uaid-cross-gateway-auth.md accidentally deleted Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: remove leftover conflict markers from test_token_storage_service.py Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * rebase issue fix Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test failure Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * chore: suppress false positive secret detection in oauth_router.py Add pragma comment to 'credentials: include' fetch option. Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(oauth): port PR #5244 OAuth health check features to DatabaseTokenBackend after rebase Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(oauth): port PR #5244 features (omit_resource, error handling) to DatabaseTokenBackend and fix tests after rebase Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(oauth): port PR #5244 features to VaultTokenBackend Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(oauth): add unit tests for PR #5244 VaultTokenBackend and refresh_helpers features Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(oauth): add coverage tests for vault_backend edge cases (87% -> 92%) Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(oauth): add store_tokens edge case tests - reach 93% coverage target - Test created_at preservation on token updates (line 298) - Test cache invalidation with cache enabled (lines 324-326) - vault_backend.py: 87% → 89% - Overall token_backends: 92% → 93% ✅ Achieves CI/CD 93% coverage requirement. Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test coverage Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: clean up token_storage_service.py after rebase conflicts Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com> * fix(db): update migration 12d4a0c7789c to point to correct parent c9f8e7d6a4b3 Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(oauth): fix failing tests after rebase - mock EmailUser query and learned_audience Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: update token backend tests and remove duplicate import Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test coverage Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(oauth): address PR #5599 review findings - fix(vault): add _enforce_gateway_access() to vault_authorize to enforce gateway visibility/team-membership checks before initiating OAuth flow; without this any authenticated user could start an OAuth flow against a private gateway they are not a member of (Finding #1) - test(vault): add deny-path security regression tests for vault_router covering wrong-team, public-only token, server-not-found, no-oauth-gateways, valid-member, and admin scenarios (AGENTS.md requirement) - fix(oauth): revert fetch_tools_after_oauth detail=f'...{e}' back to generic 'Failed to fetch tools' to avoid leaking internal hostnames/token audience/ scope names to callers; add exc_info=True to GatewayConnectionError branch so full detail reaches operator logs (Finding #4) - fix(oauth): correct misleading comment on callback-minted session JWT; document admin-bypass behaviour and jwt_teams_claim path selection (Finding #2) - fix(db): repoint migration 12d4a0c7789c down_revision from c9f8e7d6a4b3 to e4f5a6b7c8d9 (current main head) to resolve multiple-heads error (Finding #3) Closes #5599 review findings #1 #2 #3 #4 Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * chore: update uv.lock after pre-commit formatting Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: correct Revises comment from c9f8e7d6a4b3 to e4f5a6b7c8d9 in add_team_id_to_oauth_states migration Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(oauth): fix learned_aud persistence, vault refresh erasure, vault_router team context, manual refresh user_context, and OAuthError info leak (CWE-209) Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * refactor: address non-blocking review comments for pluggable token storage - Add URL encoding for team_id in Vault KV paths to prevent potential path traversal if future callers pass slugs/display names instead of UUIDs - Add backend capability check before DB session creation to avoid unnecessary round-trips on default database backend which doesn't implement per-user auth headers Both changes are performance/security hardening with no functional impact on current callers. Related: PR #5599 review comments (suggestions 1-2) Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * refactor: consolidate OAuth token backend code duplication and fix tests Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(oauth): address round-3 blocking review findings (B2-B5, S6, S8) - B4: hard-fail OAuth callback when identity missing after state consumed (CWE-287) - B2: add ORDER BY to team queries for stable Vault path key - B3: remove cross-path 401 fallback violating #5598 no-dual-backend rule - B5: formalise get_user_auth_headers on AbstractTokenBackend; drop getattr dispatch - S6: decrypt client_secret in VaultTokenBackend before token endpoint call - S8: omit teams claim from JWT when team_id is None to prevent scope widening Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(oauth): update TestGatewayService401Retry to reflect B3 no-fallback behaviour Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * style: fix pre-commit formatting in oauth_router.py (remove spaces in **() expression) Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(vault): address round-4 review findings (B2a–B2g, B3, S10–S13) - B2a: raise OAuthError on client_secret decryption failure (fail-closed) - B2b: add per-key asyncio refresh lock to prevent invalid_grant races - B2c: expire cache entry on revoke instead of deleting (cross-worker safety) - B2d: replace raw str(e) with generic user message in OAuth callback (CWE-209) - B2e: pass popup=True in vault_authorize to avoid overwriting session cookie - B2f: add GET /vault/authorize/{server_id} to _PERMISSION_PATTERNS (GATEWAYS_READ) - B2g: enforce Server row visibility before resolving gateway in vault_authorize - B3: restore two deleted regression tests in test_db_backend.py - S10: add TODO comment documenting Phase 2 wiring for get/store_oauth_credentials - S11: extract duplicated Vault auth-header fallback into _resolve_vault_auth_headers() - S12: standardise store_tokens() return type to TokenRecord across all backends - S13: update security-features.md to reflect VaultTokenBackend implementation status Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(vault): update tests for refresh-lock split and revoke expire-in-place, fix ruff/pylint warnings Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(vault): update tests for refresh-lock split and revoke expire-in-place, fix ruff/pylint warnings Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: address round-6 security review — CWE-863 vault path revocation, fail-closed vault errors, refresh-lock race, expire-in-place cache, read method error handling, revoke distinguishability, server-visibility deny-path test Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: address self-review findings — demote debug logs, fix Optional[str] types, add nosec suppressions, clarify vault router comment Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: pre-commit formatting, update secrets baseline, skip expired sunset test #2754 Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test: skip test_get_redis_client_with_circuit_open — pre-existing failure on main, unrelated to this PR Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(coverage): add unit tests to boost diff coverage for pluggable token storage Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(coverage): add unit tests to boost diff coverage for pluggable token storage Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * pre-commit fix Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: fail-closed vault auth, deterministic gateway order, client_id 400, log sanitization, stale tests Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: sanitize all log args, opaque vault error message, demote debug checks, fix stale test mock Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * style: apply pre-commit black formatting to vault_router, vault_backend, tool_service Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: fail-closed Vault 5xx/4xx errors, guard store_tokens write, and thread jwt_teams_claim for admin token path alignment Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: update _capture_store mock signature to accept redirect_uri after rebase Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * alembic down_revision_change Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: add jwt_teams_claim=None to test_rpc_tool_invocation assertion Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(vault): handle header-only Vault records in get_user_token and cap _refresh_locks size B1: use .get() guards on 'token'/'access_token' keys — returns None instead of KeyError when ICA writes a header-only record at the shared Vault path. B2: evict oldest idle lock in _get_refresh_lock when dict reaches cache_max_size. Held locks are never evicted to preserve refresh serialisation correctness. Add four regression tests covering both fixes Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * chore: regenerate .secrets.baseline after rebase onto main Line-number drift only, no new or removed findings. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com> * fix: re-chain oauth_states team_id migration onto new main head Rebase onto origin/main pulled in d8d7939e73e9 (tool-preview permission), which shares db41939315aa as down_revision with 12d4a0c7789c, producing two alembic heads. Re-point 12d4a0c7789c onto d8d7939e73e9 to restore a single linear head. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com> * chore: regenerate .secrets.baseline to drop stale finding Rerunning the detect-secrets pre-commit hook (not just the make target) dropped a stale mcpgateway/routers/oauth_router.py:933 entry that no longer matches current file content. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com> --------- Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Co-authored-by: Madhav Kandukuri <madhav165@gmail.com> Co-authored-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
madhu-mohan-jaishankar
pushed a commit
that referenced
this pull request
Aug 31, 2026
…5599) * feat: pluggable OAuth token storage with Database and Vault backends Implement team-scoped OAuth token storage supporting Database and Vault backends with comprehensive test coverage Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * chore: remove vault test files and reset config to main - Remove docker-compose.vault-test.yml and Makefile.vault-tests - Reset .gitignore and pyproject.toml to main branch versions - These files were branch-specific testing artifacts Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * chore: remove oauth_vault_lookup.sql script Remove branch-specific SQL script used for vault testing. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * refactor: use unified /oauth/callback endpoint for both OAuth authorization flows Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test: fix OAuth tests for teams parameter and state_data return fields Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * pre-commit fix Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: correct type annotations for optional Request parameters Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test cases add Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: correct Request parameter ordering in FastAPI routes FastAPI requires Request parameters to come before optional parameters. Moving request parameter to first position in oauth_callback and vault_authorize functions to fix parameter ordering syntax error. Resolves FastAPIError about invalid Request | None field type. Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * feat(vault): resolve per-user creds from Vault for all auth types Extend the Vault OAuth token backend so per-user credentials are read from Vault for non-OAuth auth types too, not just OAuth authorization_code. - VaultTokenBackend.get_user_auth_headers + TokenStorageService facade: read a per-user {header: value} dict stored under a 'headers' field at the same per-user Vault path used for OAuth tokens. - tool_service invoke paths: for non-OAuth gateways, resolve per-user Vault headers FIRST, then fall back to the gateway-wide (admin-set) static auth. Lets ICA write per-user bearer/basic/authheaders creds to Vault which CF injects at invocation, consistent with the OAuth flow. Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> * chore: fix file headers for pre-commit compliance - Add missing encoding declarations - Add missing Location comments - Update Copyright to contributors format - Add missing SPDX-License-Identifier lines - Remove stale Authors attributions per project standards Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * chore: remove temporary documentation and test files Remove working documentation, analysis files, and test scripts that should not be committed to the repository: - Design/analysis documents - Testing guides and summaries - Vault-specific helper scripts - Review artifacts Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: restore manual-test-uaid-cross-gateway-auth.md accidentally deleted Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: remove leftover conflict markers from test_token_storage_service.py Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * rebase issue fix Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test failure Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * chore: suppress false positive secret detection in oauth_router.py Add pragma comment to 'credentials: include' fetch option. Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(oauth): port PR #5244 OAuth health check features to DatabaseTokenBackend after rebase Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(oauth): port PR #5244 features (omit_resource, error handling) to DatabaseTokenBackend and fix tests after rebase Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(oauth): port PR #5244 features to VaultTokenBackend Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(oauth): add unit tests for PR #5244 VaultTokenBackend and refresh_helpers features Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(oauth): add coverage tests for vault_backend edge cases (87% -> 92%) Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(oauth): add store_tokens edge case tests - reach 93% coverage target - Test created_at preservation on token updates (line 298) - Test cache invalidation with cache enabled (lines 324-326) - vault_backend.py: 87% → 89% - Overall token_backends: 92% → 93% ✅ Achieves CI/CD 93% coverage requirement. Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test coverage Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: clean up token_storage_service.py after rebase conflicts Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com> * fix(db): update migration 12d4a0c7789c to point to correct parent c9f8e7d6a4b3 Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(oauth): fix failing tests after rebase - mock EmailUser query and learned_audience Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: update token backend tests and remove duplicate import Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test coverage Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(oauth): address PR #5599 review findings - fix(vault): add _enforce_gateway_access() to vault_authorize to enforce gateway visibility/team-membership checks before initiating OAuth flow; without this any authenticated user could start an OAuth flow against a private gateway they are not a member of (Finding #1) - test(vault): add deny-path security regression tests for vault_router covering wrong-team, public-only token, server-not-found, no-oauth-gateways, valid-member, and admin scenarios (AGENTS.md requirement) - fix(oauth): revert fetch_tools_after_oauth detail=f'...{e}' back to generic 'Failed to fetch tools' to avoid leaking internal hostnames/token audience/ scope names to callers; add exc_info=True to GatewayConnectionError branch so full detail reaches operator logs (Finding #4) - fix(oauth): correct misleading comment on callback-minted session JWT; document admin-bypass behaviour and jwt_teams_claim path selection (Finding #2) - fix(db): repoint migration 12d4a0c7789c down_revision from c9f8e7d6a4b3 to e4f5a6b7c8d9 (current main head) to resolve multiple-heads error (Finding #3) Closes #5599 review findings #1 #2 #3 #4 Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * chore: update uv.lock after pre-commit formatting Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: correct Revises comment from c9f8e7d6a4b3 to e4f5a6b7c8d9 in add_team_id_to_oauth_states migration Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(oauth): fix learned_aud persistence, vault refresh erasure, vault_router team context, manual refresh user_context, and OAuthError info leak (CWE-209) Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * refactor: address non-blocking review comments for pluggable token storage - Add URL encoding for team_id in Vault KV paths to prevent potential path traversal if future callers pass slugs/display names instead of UUIDs - Add backend capability check before DB session creation to avoid unnecessary round-trips on default database backend which doesn't implement per-user auth headers Both changes are performance/security hardening with no functional impact on current callers. Related: PR #5599 review comments (suggestions 1-2) Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * refactor: consolidate OAuth token backend code duplication and fix tests Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(oauth): address round-3 blocking review findings (B2-B5, S6, S8) - B4: hard-fail OAuth callback when identity missing after state consumed (CWE-287) - B2: add ORDER BY to team queries for stable Vault path key - B3: remove cross-path 401 fallback violating #5598 no-dual-backend rule - B5: formalise get_user_auth_headers on AbstractTokenBackend; drop getattr dispatch - S6: decrypt client_secret in VaultTokenBackend before token endpoint call - S8: omit teams claim from JWT when team_id is None to prevent scope widening Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(oauth): update TestGatewayService401Retry to reflect B3 no-fallback behaviour Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * style: fix pre-commit formatting in oauth_router.py (remove spaces in **() expression) Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(vault): address round-4 review findings (B2a–B2g, B3, S10–S13) - B2a: raise OAuthError on client_secret decryption failure (fail-closed) - B2b: add per-key asyncio refresh lock to prevent invalid_grant races - B2c: expire cache entry on revoke instead of deleting (cross-worker safety) - B2d: replace raw str(e) with generic user message in OAuth callback (CWE-209) - B2e: pass popup=True in vault_authorize to avoid overwriting session cookie - B2f: add GET /vault/authorize/{server_id} to _PERMISSION_PATTERNS (GATEWAYS_READ) - B2g: enforce Server row visibility before resolving gateway in vault_authorize - B3: restore two deleted regression tests in test_db_backend.py - S10: add TODO comment documenting Phase 2 wiring for get/store_oauth_credentials - S11: extract duplicated Vault auth-header fallback into _resolve_vault_auth_headers() - S12: standardise store_tokens() return type to TokenRecord across all backends - S13: update security-features.md to reflect VaultTokenBackend implementation status Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(vault): update tests for refresh-lock split and revoke expire-in-place, fix ruff/pylint warnings Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(vault): update tests for refresh-lock split and revoke expire-in-place, fix ruff/pylint warnings Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: address round-6 security review — CWE-863 vault path revocation, fail-closed vault errors, refresh-lock race, expire-in-place cache, read method error handling, revoke distinguishability, server-visibility deny-path test Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: address self-review findings — demote debug logs, fix Optional[str] types, add nosec suppressions, clarify vault router comment Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: pre-commit formatting, update secrets baseline, skip expired sunset test #2754 Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test: skip test_get_redis_client_with_circuit_open — pre-existing failure on main, unrelated to this PR Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(coverage): add unit tests to boost diff coverage for pluggable token storage Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * test(coverage): add unit tests to boost diff coverage for pluggable token storage Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * pre-commit fix Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: fail-closed vault auth, deterministic gateway order, client_id 400, log sanitization, stale tests Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: sanitize all log args, opaque vault error message, demote debug checks, fix stale test mock Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * style: apply pre-commit black formatting to vault_router, vault_backend, tool_service Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: fail-closed Vault 5xx/4xx errors, guard store_tokens write, and thread jwt_teams_claim for admin token path alignment Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: update _capture_store mock signature to accept redirect_uri after rebase Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * alembic down_revision_change Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix: add jwt_teams_claim=None to test_rpc_tool_invocation assertion Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * fix(vault): handle header-only Vault records in get_user_token and cap _refresh_locks size B1: use .get() guards on 'token'/'access_token' keys — returns None instead of KeyError when ICA writes a header-only record at the shared Vault path. B2: evict oldest idle lock in _get_refresh_lock when dict reaches cache_max_size. Held locks are never evicted to preserve refresh serialisation correctness. Add four regression tests covering both fixes Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> * chore: regenerate .secrets.baseline after rebase onto main Line-number drift only, no new or removed findings. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com> * fix: re-chain oauth_states team_id migration onto new main head Rebase onto origin/main pulled in d8d7939e73e9 (tool-preview permission), which shares db41939315aa as down_revision with 12d4a0c7789c, producing two alembic heads. Re-point 12d4a0c7789c onto d8d7939e73e9 to restore a single linear head. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com> * chore: regenerate .secrets.baseline to drop stale finding Rerunning the detect-secrets pre-commit hook (not just the make target) dropped a stale mcpgateway/routers/oauth_router.py:933 entry that no longer matches current file content. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com> --------- Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com> Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Co-authored-by: Madhav Kandukuri <madhav165@gmail.com> Co-authored-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.