Docker Compose scripts and docs#8
Merged
crivetimihai merged 1 commit intomainfrom May 29, 2025
Merged
Conversation
vk-playground
pushed a commit
to vk-playground/mcp-context-forge
that referenced
this pull request
Sep 14, 2025
Docker Compose scripts and docs 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
Docker Compose scripts and docs
vk-playground
pushed a commit
to vk-playground/mcp-context-forge
that referenced
this pull request
Sep 16, 2025
Docker Compose scripts and docs Signed-off-by: Vicky Kuo <vicky.kuo@ibm.com>
65 tasks
4 tasks
yiannis2804
added a commit
to yiannis2804/mcp-context-forge
that referenced
this pull request
Feb 19, 2026
…BM#8) Address code review suggestion from @jonpspri: Problem: The _check_resource_access logic (owner, team, visibility) is well-thought-out but never executed because no callsite passes resource_type to the decorator. Could be forgotten. Solution: - Added comprehensive NOTE explaining this is Phase 2+ scaffolding - Documents why it's currently not called (no resource_type parameter) - Provides Phase 2 activation plan with 4 clear steps - Includes example future usage - Prevents implementation from being forgotten Current State: - Resource always None in check_access() - _check_resource_access never executes - Permission checks are permission-level only Future Phase 2: - Decorators will pass resource_type - Extract resource_id from function params - Fine-grained per-resource access control - Check ownership, team membership, visibility Related: PR IBM#2682 Phase 1 Code Review Item IBM#8 Signed-off-by: yiannis2804 <yiannis2804@gmail.com>
crivetimihai
pushed a commit
that referenced
this pull request
Feb 24, 2026
) Address code review suggestion from @jonpspri: Problem: The _check_resource_access logic (owner, team, visibility) is well-thought-out but never executed because no callsite passes resource_type to the decorator. Could be forgotten. Solution: - Added comprehensive NOTE explaining this is Phase 2+ scaffolding - Documents why it's currently not called (no resource_type parameter) - Provides Phase 2 activation plan with 4 clear steps - Includes example future usage - Prevents implementation from being forgotten Current State: - Resource always None in check_access() - _check_resource_access never executes - Permission checks are permission-level only Future Phase 2: - Decorators will pass resource_type - Extract resource_id from function params - Fine-grained per-resource access control - Check ownership, team membership, visibility Related: PR #2682 Phase 1 Code Review Item #8 Signed-off-by: yiannis2804 <yiannis2804@gmail.com>
aidbutlr
added a commit
to aidbutlr/mcp-context-forge
that referenced
this pull request
Mar 3, 2026
CYFR-380 Resync project 20260114
2 tasks
gandhipratik203
added a commit
that referenced
this pull request
Mar 19, 2026
Closes #3740 ## What changed ### Plugin fixes (plugins/rate_limiter/rate_limiter.py) - Config validation at __init__: _validate_config() parses all rate strings at startup — bad config raises immediately, not mid-request - Graceful degradation: both hooks wrapped in try/except; unexpected errors are logged and the request is allowed through (permissive) - prompt_pre_fetch now enforces by_tool limits using prompt_id as key - MemoryBackend: asyncio.Lock makes counter increments atomic - MemoryBackend: background TTL sweep evicts expired windows (0.5s interval) - RedisBackend: atomic INCR+EXPIRE via Lua script; shared state across all gateway instances; native TTL expiry; falls back to memory on error ### Test additions (tests/unit/.../test_rate_limiter.py) - Gap tests: 4 xfail -> pass (shared state, eviction, prompt by_tool, graceful degradation); 1 xfail remains (fixed window burst, deferred) - Edge case tests: malformed/unsupported config raises at init (not request time); runtime errors degrade gracefully via mock injection - Redis backend test uses injected FakeRedis — no live server required ### Config changes - plugins/config.yaml: RateLimiterPlugin enabled with enforce mode - tests/performance/plugins/config.yaml: RateLimiterPlugin set to permissive for inclusion in cProfile benchmark runs Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
crivetimihai
pushed a commit
that referenced
this pull request
Mar 21, 2026
Closes #3740 ## What changed ### Plugin fixes (plugins/rate_limiter/rate_limiter.py) - Config validation at __init__: _validate_config() parses all rate strings at startup — bad config raises immediately, not mid-request - Graceful degradation: both hooks wrapped in try/except; unexpected errors are logged and the request is allowed through (permissive) - prompt_pre_fetch now enforces by_tool limits using prompt_id as key - MemoryBackend: asyncio.Lock makes counter increments atomic - MemoryBackend: background TTL sweep evicts expired windows (0.5s interval) - RedisBackend: atomic INCR+EXPIRE via Lua script; shared state across all gateway instances; native TTL expiry; falls back to memory on error ### Test additions (tests/unit/.../test_rate_limiter.py) - Gap tests: 4 xfail -> pass (shared state, eviction, prompt by_tool, graceful degradation); 1 xfail remains (fixed window burst, deferred) - Edge case tests: malformed/unsupported config raises at init (not request time); runtime errors degrade gracefully via mock injection - Redis backend test uses injected FakeRedis — no live server required ### Config changes - plugins/config.yaml: RateLimiterPlugin enabled with enforce mode - tests/performance/plugins/config.yaml: RateLimiterPlugin set to permissive for inclusion in cProfile benchmark runs Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
crivetimihai
added a commit
that referenced
this pull request
Mar 21, 2026
…dation (#3750) * test(rate-limiter): harden rate limiter plugin — gaps #1-#8 Closes #3740 ## What changed ### Plugin fixes (plugins/rate_limiter/rate_limiter.py) - Config validation at __init__: _validate_config() parses all rate strings at startup — bad config raises immediately, not mid-request - Graceful degradation: both hooks wrapped in try/except; unexpected errors are logged and the request is allowed through (permissive) - prompt_pre_fetch now enforces by_tool limits using prompt_id as key - MemoryBackend: asyncio.Lock makes counter increments atomic - MemoryBackend: background TTL sweep evicts expired windows (0.5s interval) - RedisBackend: atomic INCR+EXPIRE via Lua script; shared state across all gateway instances; native TTL expiry; falls back to memory on error ### Test additions (tests/unit/.../test_rate_limiter.py) - Gap tests: 4 xfail -> pass (shared state, eviction, prompt by_tool, graceful degradation); 1 xfail remains (fixed window burst, deferred) - Edge case tests: malformed/unsupported config raises at init (not request time); runtime errors degrade gracefully via mock injection - Redis backend test uses injected FakeRedis — no live server required ### Config changes - plugins/config.yaml: RateLimiterPlugin enabled with enforce mode - tests/performance/plugins/config.yaml: RateLimiterPlugin set to permissive for inclusion in cProfile benchmark runs Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com> * chore(config): enable Redis backend for RateLimiterPlugin in plugins/config.yaml Switch the default stack config from in-memory to Redis-backed rate limiting. This ensures the 30/m per-user limit is enforced as a true shared limit across all gateway instances rather than 30/m per process. Validated via Redis MONITOR: all 3 gateway instances atomically increment the same rl:user:<id>:60 counter via the Lua INCR+EXPIRE script. Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com> * test(rate-limiter): add benchmark-rate-limiter load test for multi-instance correctness Adds locustfile_rate_limiter.py and a make benchmark-rate-limiter target to demonstrate the multi-instance rate limit enforcement gap and its fix. The test sends 1 req/s (60 req/min = 2x the 30/m limit) through 3 gateway instances. With a memory backend each instance only sees ~20 req/min and never fires the limiter (~0% failures). With the Redis backend the shared counter reaches 30/min and blocks ~50% of requests — clearly showing the fix works across instances. Expected results: Memory backend: ~0% blocked (each instance sees 20 req/min < 30/m limit) Redis backend: ~50% blocked (shared counter: 60 req/min > 30/m limit) Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com> * test(rate-limiter): add hardening tests, bypass resistance, PII fix, and updated docs - Add 22 new unit tests (70 passed total, 4 xfailed): - Permissive vs enforce mode through PluginExecutor - Redis fallback: memory takeover when Redis is down, limit still enforced, no-fallback graceful degradation - Cross-tenant isolation: independent counters, no counter bleed between tenants - Header accuracy: Retry-After bounds, X-RateLimit-Reset future/consistency, Remaining decrement - Bypass resistance: None/whitespace user identity, tool name case sensitivity and whitespace (documented as xfail gaps) - PII: violation description must not contain user or tenant identifiers - Fix PII leak in violation description: remove user/tenant from description string in both prompt_pre_fetch and tool_pre_invoke — identifiers appeared in log output via permissive-mode manager warning and enforce-mode PluginViolationError message - Rewrite plugins/rate_limiter/README.md: was describing the old pre-fix implementation (in-memory only, no Redis, Redis as TODO). Now documents both backends, full config reference, response headers, examples, and accurate limitations table - Update plugin-manifest.yaml description to reflect Redis backend support Closes #3740 Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com> * fix(rate-limiter): review fixes — dead code, test correctness, config validation - Remove unused _allow() module-level function (dead code — plugin uses self._rate_backend.allow() directly) - Fix test_graceful_degradation test: was patching _allow() which is never called by the plugin; now patches backend.allow() via patch.object so the try/except error path is actually exercised - Add prompt_pre_fetch graceful degradation test (was only tested for tool_pre_invoke) - Fix inconsistent by_tool lookup in tool_pre_invoke: remove unnecessary hasattr(__contains__) guard, align with prompt_pre_fetch pattern - Add backend validation to _validate_config(): typo like 'reddis' now raises ValueError at startup instead of silently falling back to memory - Add test for malformed by_tool rate string validation - Add test for invalid backend name validation - Change default config mode from enforce to permissive for safety (consistent with all other security plugins in the default config) Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>
ecthelion77
pushed a commit
to forterro/mcp-context-forge
that referenced
this pull request
Mar 30, 2026
Merged IBM/mcp-context-forge upstream/main into feature/upstream-sync-march30. Key upstream additions: - Security: Server ID validation in Streamable HTTP, secrets detection, content size limits, service account support - SSO: Stale team membership revocation, groups claim extraction for generic OIDC providers, sync_roles flag - RBAC: Session-token team narrowing Layer 2, permission-based menu hiding - Observability: Fix duplicate DB session middleware, metrics returning 0 after cleanup, metrics_cache leak fix - Tools: Configurable forbidden description patterns (replaces our IBM#18) - Plugins: retry-with-backoff, PII filter Rust hardening, URL reputation - Infra: Remove MySQL/MongoDB support (PostgreSQL only), rate limiter fix - A2A: Cascade agent state changes to MCP tools - UI: Persist admin table filters, team member modal fixes Conflicts resolved (10 files): - admin.py: kept upstream team preservation on edit + our OIDC sync params - schemas.py: kept upstream configurable patterns + our meta-server fields - gateway_service.py: kept upstream visibility propagation fix - oauth_manager.py: kept our expires_in=None fix (patch IBM#20) - sso_service.py: adopted upstream _build_normalized_user_info refactor - team_management_service.py: kept our PermissionError + upstream UNSET/skip_limits - streamablehttp_transport.py: adapted meta-server loading to use validated server_id - sso_bootstrap.py: combined upstream scope preservation + our smart team_mapping merge - test_sso_*.py: adopted upstream test refactoring Patches now obsolete (superseded by upstream): - IBM#1 (SSO email_verified) — upstream b668d2b - IBM#8 (teams=None) — upstream b2b6c12 - IBM#18 (tool description sanitize) — upstream bd803e5 (configurable patterns)
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>
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>
jonpspri
added a commit
that referenced
this pull request
May 7, 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 7, 2026
* feat: Add grpc_service_id to Tool schema #2854 Branch: GrpcMethodsAsTools-2854 Signed-off-by: Gabe Goodhart <ghart@us.ibm.com> AI-usage: full * feat: Add tool registration and deletion for registering/deleting gRPC services #2854 Branch: GrpcMethodsAsTools-2854 Signed-off-by: Gabe Goodhart <ghart@us.ibm.com> AI-usage: full * feat: Add load_file_descriptors for grpc serialized proto descriptor bytes #2854 Branch: GrpcMethodsAsTools-2854 Signed-off-by: Gabe Goodhart <ghart@us.ibm.com> AI-usage: full * feat: Add gRPC tool invocation support #2854 Branch: GrpcMethodsAsTools-2854 Signed-off-by: Gabe Goodhart <ghart@us.ibm.com> AI-usage: full * test: Unit tests for gRPC tool registration #2854 Branch: GrpcMethodsAsTools-2854 Signed-off-by: Gabe Goodhart <ghart@us.ibm.com> AI-usage: full * style: make lint #2854 Branch: GrpcMethodsAsTools-2854 Signed-off-by: Gabe Goodhart <ghart@us.ibm.com> * test: Fix missing grpc_service_id in tool unit test #2854 Branch: GrpcMethodsAsTools-2854 Signed-off-by: Gabe Goodhart <ghart@us.ibm.com> AI-usage: full * test: Flesh out test coverage for new gRPC code paths #2854 Branch: GrpcMethodsAsTools-2854 Signed-off-by: Gabe Goodhart <ghart@us.ibm.com> AI-usage: full * fix: Better error handling for protobuf descriptor pool adding #2854 Branch: GrpcMethodsAsTools-2854 Signed-off-by: Gabe Goodhart <ghart@us.ibm.com> * fix: Inherit visibility from parent gRPC service when mapping to tools #2854 Branch: GrpcMethodsAsTools-2854 AI-usage: none Signed-off-by: Gabe Goodhart <ghart@us.ibm.com> * fix: Fix alembic migration after rebase #2854 Branch: GrpcMethodsAsTools-2854 AI-usage: full Signed-off-by: Gabe Goodhart <ghart@us.ibm.com> * fix: Repoint alembic migration parent to current head after rebase Updates down_revision from x7h8i9j0k1l2 to aa1b2c3d4e5f, the current head on origin/main. Several new migrations have landed since this PR was last rebased, so the previous parent is no longer the head. Verified single head: alembic heads -> w7x8y9z0a1b2 (head) #2854 Branch: GrpcMethodsAsTools-2854 Signed-off-by: Jonathan Springer <jps@s390x.com> * test: Add grpc_service_id=None to TestRustMcpExecutionPlan SimpleNamespace mocks Two tests added to TestRustMcpExecutionPlan after this PR was last rebased construct tool mocks via SimpleNamespace. The PR's change to _build_tool_cache_payload reads tool.grpc_service_id, so those mocks now AttributeError. Mirrors b576ec4's earlier fix for the same pattern in MagicMock(spec=DbTool) helpers. Affected: - TestRustMcpExecutionPlan.test_prepare_rust_mcp_tool_execution_uses_live_gateway_auth_fields_for_loaded_tools - TestRustMcpExecutionPlan.test_prepare_rust_mcp_tool_execution_uses_live_gateway_string_auth_values #2854 Branch: GrpcMethodsAsTools-2854 Signed-off-by: Jonathan Springer <jps@s390x.com> * style: make black isort Tightens whitespace and import grouping per project lint config: - mcpgateway/services/grpc_service.py: black removes a stray blank line - tests/unit/mcpgateway/{services/test_grpc_service,services/test_tool_service,test_translate_grpc}.py: isort regroups stdlib/third-party/first-party/local imports #2854 Branch: GrpcMethodsAsTools-2854 Signed-off-by: Jonathan Springer <jps@s390x.com> * fix(grpc): use protobuf 6.x kwarg in MessageToDict and isolate descriptor pool Two issues addressed in mcpgateway/translate_grpc.py: 1. Pre-existing runtime bug: invoke()/invoke_streaming() called json_format.MessageToDict(..., including_default_value_fields=True), the parameter name from protobuf 4.x. The repo pins protobuf>=6.33.6 (see pyproject.toml), where the parameter was renamed to always_print_fields_with_no_presence. Calls raised TypeError at runtime, breaking gRPC tool invocation. Renamed the kwarg and replaced the # pylint: disable=unexpected-keyword-arg suppression with an anti-regression comment. 2. Descriptor pool poisoning: GrpcEndpoint shared descriptor_pool.Default(), the process-wide singleton. Reflected FileDescriptorProto from one (untrusted) upstream service could leak across requests and cause symbol collisions or type confusion in subsequent calls. Switched to a per-endpoint descriptor_pool.DescriptorPool(); MessageFactory now binds to that private pool. Updated tests/unit/mcpgateway/test_translate_grpc.py monkeypatches to expose DescriptorPool() instead of Default() and MessageFactory(pool=...) instead of MessageFactory(). Closes review: B2, B11 #2854 Branch: GrpcMethodsAsTools-2854 Signed-off-by: Jonathan Springer <jps@s390x.com> * fix(grpc): security and robustness hardening from review Addresses BLOCKING findings from a multi-agent code review of PR #3202: Security: * Reject local-only gRPC schemes (unix:, unix-abstract:, vsock:, fd:) in _validate_grpc_target since they bypass the IP-based SSRF model. Also normalize dns:/// and ipv4:/ipv6: name-resolver prefixes and parse bracketed IPv6 hosts correctly so the host-extraction step cannot be tricked. (review B1) * Add _enforce_descriptor_limits to bound reflected FileDescriptorProto count, per-blob size, and aggregate size before storing or loading. Hardcoded constants (not settings) so a config change cannot silently weaken the DoS defense. (review B3) * Add _validate_reflected_tool_name that runs reflected tool names through SecurityValidator.validate_tool_name() before persisting. Reflected names from upstream gRPC servers no longer bypass the injection / length / character checks applied to user-registered tools. (review B4) * Strip metadata pseudo-keys (anything starting with '_') when copying discovered_services into endpoint._services so the _file_descriptors entry can never be confused with a service. * Decode stored descriptors with base64.b64decode(..., validate=True) to fail fast on tampered DB content. Robustness / Layer 1 invariants: * _sync_tools_from_reflection now runs each method through a per-tool try/except so a single bad method cannot abort the entire reflection sync. Counts created / updated / failed. (review B9) * _sync_tools_from_reflection now propagates parent service visibility, team_id, and owner_email to existing reflected tools, not just to newly created ones. Closes a Layer 1 token-scoping gap where a public->team visibility change on the parent service was silently ignored on already-discovered tools. (review B5) Operational: * invoke_method gains a timeout parameter (default settings.tool_timeout) and wraps endpoint.start() / endpoint.invoke() in asyncio.wait_for so a slow upstream cannot tie up a worker indefinitely. (review B6) * tool_service.invoke_tool gRPC branch wraps the call in asyncio.wait_for, surfaces ToolTimeoutError + post-invoke timeout hook (matches A2A branch), preserves CancelledError instead of swallowing it, and logs with %-style + exc_info=True. (review B6/B7/B8) * invoke_method preserves CancelledError, logs with %-style + exc_info=True, re-raises GrpcServiceError/GrpcServiceNotFoundError unwrapped instead of double-wrapping them. (review B8) #2854 Branch: GrpcMethodsAsTools-2854 Signed-off-by: Jonathan Springer <jps@s390x.com> * refactor(grpc): drop redundant inline settings import The inline 'from mcpgateway.config import settings' in _validate_grpc_target was made redundant by hoisting the same import to module top in the security hardening commit. Branch: GrpcMethodsAsTools-2854 Signed-off-by: Jonathan Springer <jps@s390x.com> * test(grpc): cover security-hardening helpers and Layer 1 invariants Adds two test classes exercising the helpers introduced by the security/robustness hardening commit: TestSecurityHardening: * _validate_grpc_target rejects unix:/unix-abstract:/vsock:/fd: schemes * _validate_grpc_target normalizes dns:///, dns://, dns:, ipv4:, ipv6: name-resolver prefixes * _validate_grpc_target accepts bracketed IPv6 literals and rejects malformed bracket syntax * _enforce_descriptor_limits enforces count, per-blob, and aggregate size caps; happy path passes within bounds * _validate_reflected_tool_name rejects empty / over-length / injection patterns and accepts proto-style identifiers TestVisibilityPropagation: * _sync_tools_from_reflection propagates parent service visibility, team_id, and owner_email to existing reflected tools (Layer 1 invariant). * _sync_tools_from_reflection isolates per-method failures: a method with an invalid name is skipped while the rest of the sync proceeds (B9 regression test). All 13 new tests pass; the broader gRPC + tool_service + translate_grpc suite is green at 520 tests. #2854 Branch: GrpcMethodsAsTools-2854 Signed-off-by: Jonathan Springer <jps@s390x.com> * fix(grpc): close remaining review gaps from iteration-2 oracle pass Addresses BLOCKING items the second-pass review identified as still present after the first round of fixes. * B1 (DNS resolution): _validate_grpc_target now delegates the hostname/IP-network/DNS policy to SecurityValidator._validate_ssrf so hostnames like metadata.google.internal are resolved and the resolved IPs go through the same blocklist as HTTP targets. Removes the duplicate hand-rolled IP-network logic that previously lived inline. * B5 (visibility on update): GrpcService.update_service now snapshots visibility / team_id / owner_email before applying the update and bulk-updates every child Tool with grpc_service_id == service.id in the same transaction whenever any of those fields change. Closes the Layer 1 token-scoping gap where a public->team change on the parent service was silently ignored on already-discovered tools. * B6 (real gRPC deadlines): GrpcEndpoint.start, _discover_services, and _discover_service_details now accept a timeout kwarg that is threaded into ServerReflectionInfo, and GrpcEndpoint.invoke binds the deadline onto the underlying unary_unary call so a slow upstream cannot keep an executor thread alive after asyncio.wait_for cancels the wrapping coroutine. invoke_method passes the effective tool timeout into both endpoint.start and endpoint.invoke. * B7 (cancellation propagation in outer except BaseException): adds except asyncio.CancelledError: raise immediately before the three outer except BaseException as e blocks in tool_service.invoke_tool so a cancellation is never wrapped as a ToolInvocationError. * B12 (protobuf 6.x compatibility): MessageFactory.GetPrototype was removed in protobuf >= 5.x. Both the unary and streaming invocation paths in translate_grpc now call message_factory.GetMessageClass(...) instead. Updated the corresponding unit-test mocks. * Cleanup: drops unused import ipaddress that became dead after the SecurityValidator delegation. Test mocks updated to match the new signatures (timeout kwarg on the populate_service / _populate callbacks; GetMessageClass at the module level instead of MessageFactory.GetPrototype). Lint clean (black, isort, ruff E3/E4/E7/E9/F/D1). 520 targeted tests pass. #2854 Branch: GrpcMethodsAsTools-2854 Signed-off-by: Jonathan Springer <jps@s390x.com> * fix(grpc): preserve timeout=0 fail-fast semantics in deadline ternaries The 'if timeout' ternary in three reflection/unary call sites silently drops timeout=0, but gRPC accepts timeout=0 as the explicit 'fail immediately' deadline. Switched to 'if timeout is not None' so a caller-supplied 0.0 reaches the underlying gRPC call with the right semantics. Found in iteration-3 oracle review (final pass, no new BLOCKING items). Branch: GrpcMethodsAsTools-2854 Signed-off-by: Jonathan Springer <jps@s390x.com> * fix(grpc): apply non-blocking review nits flagged across the three review 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> * style: clean lint cycle (make autoflake/isort/black/ruff/bandit/interrogate/pylint) Remaining fixes the project lint cycle surfaced after the fix-now bundle landed: * isort: re-sort sibling imports in test_grpc_service.py (_enforce_descriptor_limits before _GRPC_MAX_*; case-insensitive order). * ruff PLW0108 (unnecessary lambda) in test_translate_grpc.py: align the 4 mocks I added with the 7 pre-existing patterns in the same file (lambda **_kw: MagicMock() / lambda *_a, **_kw: MagicMock()). * ruff RET501 in test_grpc_service.py::tls_spy: drop the redundant trailing 'return None'. * pylint no-member on the alembic migration: add the project-standard '# pylint: disable=no-member' header that 9 other migrations use to silence alembic.op's dynamic-attribute false positives. * pylint try-except-raise on tool_service.invoke_tool gRPC branch: the 'except asyncio.CancelledError: raise' clause LOOKS like a no-op but exists specifically so the LATER 'except Exception' cannot swallow cancellation (PR #3202 review B7). Added an inline disable plus a comment citing B7 so the next lint sweep doesn't delete the anti-regression code. Verified after fix: make autoflake / isort / black -> clean make ruff (E3,E4,E7,E9,F,D1) -> All checks passed make bandit -> 0 issues, 18022 LoC make interrogate -> 100% docstring coverage make pylint -> 10.00/10 on all 6 production files pytest (targeted gRPC suite) -> 525/525 passing Branch: GrpcMethodsAsTools-2854 Signed-off-by: Jonathan Springer <jps@s390x.com> * fix: Re-parent alembic migration to current head after second rebase Updates down_revision from aa1b2c3d4e5f to 926d3e07d098. Origin/main gained the CPEX plugin framework replacement (#3754) which introduced two new migrations and a mergepoint, leaving our grpc_service_id migration as a sibling head. Re-pointing to the new head returns the chain to single-headed. Verified single head: alembic heads -> w7x8y9z0a1b2 (head) alembic upgrade head -> clean on a fresh sqlite DB #2854 Branch: GrpcMethodsAsTools-2854 Signed-off-by: Jonathan Springer <jps@s390x.com> * fix(grpc): restore reserved/multicast guard and update test_grpc_service_no_grpc Two related fixes for the no-grpc test file that surfaced after rebase: 1. Restore is_reserved / is_multicast pre-check in _validate_grpc_target. The earlier delegation to SecurityValidator._validate_ssrf inadvertently dropped this guard because the shared validator only checks blocked-networks / localhost / private. IP literals like 224.0.0.1 (multicast) or 0.0.0.0 (reserved) would have slipped past unless they were also in ssrf_blocked_networks. The restored check excludes loopback so ::1 (which Python flags as both is_loopback AND is_reserved) still passes through the localhost gate as intended. 2. Update test_grpc_service_no_grpc.py: - Add timeout=None kwarg to the three FakeEndpoint.start() and invoke() methods so they accept the per-RPC deadline added in the security/robustness hardening commit. - Update test_validate_grpc_target_enforces_ssrf_rules regex patterns to match the SecurityValidator messages reached via delegation (blocked hostname X, localhost address which is blocked, private network address which is blocked). All 4 gRPC test files pass: 528 / 528. #2854 Branch: GrpcMethodsAsTools-2854 Signed-off-by: Jonathan Springer <jps@s390x.com> * test: increase diff coverage for gRPC PR from 92% to 100% on grpc_service+translate_grpc Adds 11 targeted tests + a decision-record comment to address the diff-coverage report that flagged 18 previously-uncovered lines: grpc_service.py: 95.7% -> 100% (7/7 covered) translate_grpc.py: 91.7% -> 100% (3/3 covered) tool_service.py: 69.2% -> 62% (5/8 covered + 3 documented) New tests in tests/unit/mcpgateway/services/test_grpc_service.py: * test_validate_grpc_target_empty_string -> line 134 * test_invoke_method_propagates_cancelled_error -> line 1021 * test_invoke_method_re_raises_timeout -> lines 1023-1024 * test_invoke_method_re_raises_grpc_service_error_unwrapped -> line 1026 * test_update_service_propagates_visibility_to_child_tools -> lines 503-504 New tests in tests/unit/mcpgateway/test_translate_grpc.py: * test_load_file_descriptors_rejects_single_bytes -> line 409 * test_load_file_descriptors_skips_pool_conflict -> lines 420, 425 New tests in tests/unit/mcpgateway/services/test_tool_service.py: * test_invoke_grpc_tool_propagates_cancellation -> line 5847 * test_invoke_grpc_tool_timeout_raises_tool_timeout_error -> lines 5849-5850, 5852 Restored a regression: the earlier B1 refactor (delegating to SecurityValidator._validate_ssrf) inadvertently dropped the is_reserved / is_multicast guard, since SecurityValidator only checks blocked-networks / localhost / private. Restored the local guard with an is_loopback exclusion so IPv6 ::1 (which Python flags as both is_loopback AND is_reserved) still passes through the localhost gate. Coverage decision-record left in test_tool_service.py for the 3 remaining B7 anti-regression lines (5446 REST, 5632 MCP, 5851 gRPC timeout-with-pm): they are byte-identical to the gRPC variant at line 5847 (which is fully exercised). Equivalent REST/MCP tests require coaxing CancelledError through asyncio.wait_for, which Python's event loop converts to TimeoutError in some states. Protecting one branch is sufficient to detect a regression that would affect all three. 541 tests passing, ruff/black/isort/pylint clean. #2854 Branch: GrpcMethodsAsTools-2854 Signed-off-by: Jonathan Springer <jps@s390x.com> * fix: re-parent alembic migration onto current head + restore interrogate 100% Two CI failures triggered by main moving forward during this PR's review: * alembic multi-head: PR #4501 ("add ondelete=CASCADE to FK constraints") introduced 9fb98535724d as a sibling of our w7x8y9z0a1b2, both parented to 926d3e07d098. Re-parents w7x8y9z0a1b2 -> 9fb98535724d so the chain has a single head again and bootstrap_db / SQLite+Postgres upgrade validation pass. Verified single head with alembic heads. * interrogate --fail-under=100: three nested helper functions had no docstrings, dropping the package to 99.9%. One is mine (GrpcEndpoint.invoke._call introduced by the gRPC timeout fix in PR #3202 review B6); the other two (_user_obj_to_dict._iso, _user_dict_to_obj._dt in services/email_auth_service.py) landed on main via PR #4595 and were already failing main's CI. All three added one-line docstrings. Cascade fixed by these two changes: - SQLite + PostgreSQL Fresh/Upgrade (alembic head check) - pytest (py3.12) (bootstrap_db on alembic head check) - playwright-ci-smoke (depends on bootstrap) - sql-sanitizer E2E (depends on bootstrap) - interrogate (mcpgateway) (docstring coverage) - Run pre-commit hooks (interrogate is a pre-commit hook) The 2 email_auth_service docstrings are technically out of scope for this PR but the local lint cycle scoping I had been using (make interrogate TARGET=<my-files>) didn't catch them. Folding them in here unblocks PR-level CI rather than waiting for a separate fix to main. Verified locally: alembic heads -> single head w7x8y9z0a1b2 interrogate -> 100% on full mcpgateway/ black --check -> clean ruff check -> clean pylint -> 10.00/10 targeted gRPC tests -> 541 passing #2854 Branch: GrpcMethodsAsTools-2854 Signed-off-by: Jonathan Springer <jps@s390x.com> --------- Signed-off-by: Gabe Goodhart <ghart@us.ibm.com> Signed-off-by: Jonathan Springer <jps@s390x.com> Co-authored-by: Gabe Goodhart <ghart@us.ibm.com> Co-authored-by: Jonathan Springer <jps@s390x.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.