fix: bound JSON Schema pattern evaluation in a worker process - #6957
Open
msureshkumar88 wants to merge 27 commits into
Open
msureshkumar88 wants to merge 27 commits into
msureshkumar88 wants to merge 27 commits into
Conversation
msureshkumar88
requested review from
Lang-Akshay,
brian-hussey,
crivetimihai and
ja8zyjits
as code owners
September 22, 2026 10:05
Collaborator
|
1. The sandbox wait still blocks the event loop — Both call sites wait on the worker from async code, so a hostile request freezes the loop for a second instead of forever — a steady stream still stalls the worker. The jq sandbox in this same file uses 2. The new e2e case has never run —
|
Collaborator
Author
|
Thanks — both confirmed. Fixed the first, correcting myself on the second. 1. Event loop blocking ( Both call sites now wrap the sandbox validation in Proven both directions with a new regression test (
Note the actual stall is bounded by the sandbox's own Also confirmed: the existing 2. E2E test never runs — you're right, and I misstated this. I said "CI is its first run" in the PR body; that's wrong. Verified: |
msureshkumar88
force-pushed
the
fix/bound-schema-pattern-evaluation
branch
from
September 22, 2026 14:16
795cae3 to
15ee4da
Compare
added 22 commits
September 22, 2026 15:29
Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
Move the generic pool lifecycle into mcpgateway/utils/sandbox_pool.py. The pool builds workers, proves they start, gates admission, and kills a worker that overruns its budget. Rebuild jq_runner on the new pool. The public API and the jq exception types stay the same. Read the worker count and the timeout through callables. A captured value would ignore a setting that a test changes after import. Stamp the admission gate on the executor. A gate held on the pool object would admit against a different executor after a rebuild. Tighten the start-method test to expect a TimeoutError. A broad Exception also accepts a broken pool, which passes for the wrong reason. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
The budget read sat between the gate acquire and the try block that releases the permit. A raising timeout function leaked the permit, and the pool then returned SandboxBusy forever. Move the read above the acquire. It needs no permit. Add a regression test. It arms a raising timeout function, then proves a later call still reaches a worker. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
…dbox A JSON Schema regex is attacker-influenced. Python holds the GIL while it backtracks. An unbounded match freezes the gateway worker for every tenant. Route a schema that carries a regex keyword into a killable worker pool. Bound each validation by a wall-clock limit. Never inspect pattern content. Whole-validation routing also covers the paths that reach re inside jsonschema itself, which no keyword override intercepts. Fail closed on every abnormal outcome. A timeout, a busy pool, a broken pool, an oversized instance, an unserializable input, and an absent sandbox all raise ValidationError. A truncated validation is never a pass. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
An elapsed-time ceiling passes for any fast failure. A pattern that merely rejects quickly satisfies it, so the corpus could stay green over a guard that routes every schema inline. This project has shipped that failure once. Assert the raised error carries the safety-budget wording, so a bound is told apart from an ordinary mismatch. Add a test that asserts the sandbox submission itself: present for a regex keyword, absent without one. Routing is now proved directly rather than inferred from timing. Name the real cause when the pool fails to start. The previous text blamed the platform for every failure, including a configuration fault, and sent an operator to the wrong place during an incident. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
…for real The two patternProperties escape tests swallowed every error and asserted only elapsed time. They passed with no sandbox at all, so the one hole that killed the previous design read as covered while nothing checked it. Assert the timeout wording instead, and cover both keywords from one case. Match the timeout phrase rather than the generic sandbox-failure phrase. The generic text also appears when the pool is broken, so the corpus went green against a pool that failed every submission in microseconds. Refuse a validator class outside the draft table. Substituting a draft gave an extended validator different semantics in the sandbox than inline, which weakens the boundary and says nothing. Name the class instead. Drop the config reference to an untracked design document. Place the new main import in sorted order. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
Route _validate_with_cached_schema through validate_safely. The sandbox now bounds both tool input validation and tool output validation, the two sites that shared this helper. A catastrophic-backtracking schema no longer holds the GIL. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
Add a 30s pytest-timeout marker to every test that asserts a validation bound. Without it, a sandbox regression hangs the test instead of failing it, and CI stalls with no named cause. Delete the now-unused _NO_RETRIEVE_REGISTRY constant and correct the SSRF-guard comment above it: validators are built against an empty registry inside validate_safely, not against this file's own registry, which no longer exists. Update _validate_with_cached_schema's docstring to describe the sandbox delegation instead of stale inline-validator mechanics. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
DbPrompt.validate_arguments() called jsonschema.validate() directly, bypassing the sandboxed validation path. A crafted argument_schema with a catastrophic regex pattern could freeze prompt rendering, and this path needs only permission to render a prompt to trigger. Route the call through validate_safely() so the schema runs in the same killable, time-bounded worker as tool input and output validation. The ValueError contract for callers is unchanged. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
The vendored MCP SDK validates outputSchema with stock jsonschema at mcp/server/lowlevel/server.py:573. The gateway decorator validate_input=False gates only the input validator at server.py:536. The output call stays active. A remote MCP server controls tool.output_schema. It therefore controls a regex that runs unbounded inside site-packages, where no gateway change reaches. _to_mcp_tool now omits outputSchema when schema_uses_regex reports a regex keyword at any depth. The advertised tool drops the key, so the SDK skips the validation. The bounded check in tool_service still validates the same data. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
_proxy_list_tools_to_gateway returned the remote gateway's SDK tool models unchanged. Those models carry the remote's outputSchema into the SDK tool cache, where mcp/server/lowlevel/server.py:573 validates against it with stock jsonschema. The previous commit guarded only _to_mcp_tool, so a direct_proxy gateway still reached the unbounded validator. Extract _guard_output_schema and call it from both paths. _to_mcp_tool builds a dict payload; the proxy path holds constructed pydantic models, so _guard_proxied_tools adapts the shape with model_copy. The regex decision lives in one function, so the two paths cannot drift. The SDK skips output validation when outputSchema is None, so replacing the value is enough; the field need not be absent. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
Registration, gateway sync and OpenAPI import now call warn_unprovable_patterns at the point each schema reaches storage. Each call logs one warning naming the schema's source and never raises, so no registration, sync or import can fail because of this check. The sandbox already bounds every regex match; this only gives an operator an inventory of which schemas carry a regex keyword. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
…ailure path register_gateway ran warn_unprovable_patterns before appending the new DbTool to db_tools, inside the same try/except that drops a tool from federation on any exception. The warning cannot raise today, but a diagnostic call sitting ahead of the data operation it describes turns any future change to that call into silent federation data loss. Move both calls to run after the tool is already in db_tools, so a raising warning can only fail to log, never unregister the tool. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
…aise 74e5029 introduced warn_unprovable_patterns and stated in its commit body that no registration, sync or import could fail because of the check. That was true only because the function happened not to raise; nothing in the code enforced it. Four call sites relied on this unstated assumption without a guard: the before_insert/before_update listeners in db.py catch only jsonschema.exceptions.SchemaError, so any other exception would abort the SQLAlchemy flush and fail tool or prompt registration, and the OpenAPI import sites in openapi_schema_router.py and admin.py had no exception handling at all. Rather than wrap every call site in its own try/except, move the guarantee into the function itself. warn_unprovable_patterns now catches and logs any exception from its own body, including from schema_uses_regex, so it cannot fail its caller by construction. Every current and future call site inherits this for free. The gateway_service.py federation-loop ordering fix from the prior commit is unaffected and stays as defence in depth. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
…ound Four plugins compile operator-supplied regex strings at load time: regex_filter, resource_filter, code_safety_linter and harmful_content_detector. These patterns come from deploy-time configuration, not from a request, so a pathological pattern here is administrator self-denial-of-service, not an attack surface. Route them through a log line, not the validation sandbox. Add warn_unprovable_pattern_source() to safe_jsonschema.py, alongside warn_unprovable_patterns(). It never raises, so a plugin cannot fail to load because a diagnostic failed. Call it immediately before each operator-configured re.compile() site. The hardcoded default lexicons in code_safety_linter and harmful_content_detector stay silent: they are project-authored, and a warning on a pattern the project itself ships is noise. Convert the harmful_content_detector comprehension to a loop so the log call can run per pattern, keeping the existing non-string entry passthrough unchanged. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
…ngine A grep over first-party code cannot prove this. The escape found in review lived in site-packages, which a grep over mcpgateway/ never reads. Verify by execution instead. Three layers cover the tool input path, the tool output path and the prompt argument path. Layer 1 spies on the validation sandbox. It asserts each path submits a regex-bearing schema, and submits nothing for a schema without one. Only a positive assertion can prove that routing happened. Layer 2 replaces the pattern and patternProperties keyword implementations in every stock draft with a function that raises. Any pattern evaluated in this process fails the test by name. Patch the keyword tables, not jsonschema._keywords: each draft captured the function object at import time, so the module attribute is never read again. A module-attribute patch was measured and does not fire. Layer 3 makes jsonschema.validate raise. Nothing calls it today. It guards against a future change that reintroduces a direct call. The sandbox worker is a separate process, so no in-process patch reaches it. A correctly routed validation still succeeds while the tripwires are armed. Each layer carries a self-test that proves the layer can fail. One more test rejects an instance that breaks the pattern, which proves the worker evaluated the schema. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
… content The module proved three first-party sites and was blind to the fourth. The vendored SDK validates outputSchema in-process and unbounded at mcp/server/lowlevel/server.py:573. validate_input=False does not gate that branch. The only defense is _guard_output_schema, which withholds the schema. That guard submits nothing to the sandbox and evaluates no keyword, so the submit spy and the keyword tripwire both miss it. Removing the guard left all tests green. Add a fourth layer. Assert _to_mcp_tool and _guard_proxied_tools withhold a regex-bearing outputSchema, then drive the real SDK CallToolRequest handler with the tripwires armed. A self-test passes an unguarded schema to prove the SDK site is live. Hold the SDK input branch shut as well. inputSchema reaches the SDK unguarded, so validate_input=True would open a second unbounded site. Read the flag off the registered handler and assert it stays False. Assert what the rejection says, not that a rejection happened. Every fail-closed branch of validate_safely produces the same outcome in-process with no worker running, so the former assertion passed with the sandbox switched off. Each schema shape now names the message fragment only its own evaluation produces. Cover the shapes the corpus missed. Add a pattern under anyOf, which a router that skips JSON array nodes reports as regex-free, and a patternProperties schema, which the tripwire armed but nothing exercised. Parametrize every routing, tripwire, guard and rejection test over all three shapes. Record two limits in place. The jsonschema.validate patch cannot reach a caller that binds the function at import time, and no schema here may carry $anchor or $dynamicAnchor, because the metaschema checks both with its own pattern keyword. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
…eDoS A hostile subject against a nested-quantifier pattern stalls the event loop for about 8 seconds when the match runs in this process. Moving the match to a thread changes nothing, because CPython holds the GIL through a regex match. A test that only measures how long validation took passes against that non-fix. Measure the loop instead. The heartbeat wakes every 10 ms and records how late each wake-up was. The test asserts three things: the heartbeat produced samples, the worst wake-up stayed inside twice the sandbox budget, and the validation was stopped by that budget. The last assertion separates a working sandbox from a broken pool that refuses every regex-bearing schema and also keeps the loop free. The subject is 28 characters. Backtracking doubles per added character, so a longer subject runs for hours under a regression, and pytest.mark.timeout cannot break it: the signal is delivered between bytecodes and a regex match never yields one. The test would hang instead of fail. The ReDoS test in test_input_validation.py stored a catastrophic pattern and asserted it was stored. It now validates a hostile subject against each stored schema, and asserts the budget stopped the one pattern that backtracks. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
…n site Three sites asserted a bound with elapsed time plus a weak outcome check. That pair cannot tell "the sandbox bounded a runaway" apart from "rejected instantly for an unrelated reason". Each site now asserts the message contains "exceeded the execution time limit", which only the timeout path produces. A mismatch gives a jsonschema message, and any other sandbox fault gives the broader "could not be completed safely" text. The prompt site reads the phrase through the ValueError that validate_arguments raises. The pinning test for warn_unprovable_patterns broke only the schema_uses_regex call, so it pinned one path rather than the never-raises property. Moving the success-path warning out of the try would have kept it green while reintroducing an unguarded statement. The test is now parametrized over each raisable statement in the function body. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
…ings Both warn_unprovable_patterns and warn_unprovable_pattern_source promise to swallow every failure, including a failure from logging itself. The fallback warning in each except handler was unguarded, so broken logging still raised into a SQLAlchemy before_insert listener, a federation sync loop, and a plugin load path. Those callers drop a tool registration when a diagnostic raises. Both handlers now report through _report_diagnostic_failure, which guards the fallback and drops a second failure. Nothing remains to do once logging is unusable. warn_unprovable_pattern_source had no pinning test. It now carries the same parametrized test as its sibling, one raisable statement at a time: the len(pattern) the log call evaluates, the log call itself, and the fallback log. Both tests gain a case that breaks every logging call, which is what pins the new guard. The subject-length comment in the ReDoS test claimed pytest.mark.timeout cannot interrupt a regex match. That is false; the signal arrives during the match. Measured behavior: the timeout reports on schedule, then the test call waits for the match, because a Python thread cannot be killed and closing the loop joins its executor. A 3 second timeout on a 30 character match reports at 3 seconds and returns after 42. The comment now states that. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
…dation wording The event-loop regression test derived its stall budget from regex_timeout_seconds, the same unpinned setting the sandbox uses for its own timeout. Raising that setting past about 3.85 seconds let the budget grow past the actual GIL stall, so the test kept passing while a thread-only sandbox stalled the loop for close to 8 seconds. Add an explicit precondition that fails when regex_timeout_seconds exceeds the value this suite was measured against, and cap the derived budget so neither the setting nor the derivation can silently widen the check. Apply the measured MAX_VALID_SUBJECT_SECONDS bound to the valid-subject test, which previously allowed a 500x regression to pass unnoticed. Correct the BOUNDED comment at the three remaining sites that claimed the timeout path is the only one producing the outer "could not be completed safely" text. validate_safely wraps every sandbox fault in that text; the inner "exceeded the execution time limit" phrase is what actually comes from SandboxTimeout alone. Add the missing validation-pool fixture to the ReDoS coverage in test_input_validation.py, scoped to the one test that needs it rather than autoused across the file's many unrelated security tests, so the pool's lifecycle no longer depends on lazy _ensure() and on no other module having left the sandbox marked down. Proved the budget fix catches the regression it was written for: swapping ThreadPoolExecutor into SandboxPool._build and setting REGEX_TIMEOUT_SECONDS=5 now fails the event-loop test where it previously passed. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
Registers a tool whose input_schema carries a catastrophic backtracking pattern, invokes it through the real MCP path (HTTP routing, auth, RBAC, the sandboxed validator), and asserts the gateway still answers /health immediately afterwards. Unit-level coverage proves validate_safely is bounded in isolation; this proves the property survives everything between a unit test and a real request. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
added 5 commits
September 22, 2026 15:29
_mcp_tool_call rides a ClientSession capped at _CLIENT_TIMEOUT (default 5s), which fires before a 20s elapsed assertion ever could. A genuine regression now surfaces as a diagnosed pytest.fail naming both possible causes -- an unbounded sandbox or an overloaded CI box -- instead of an uncaught McpError traceback. Also delete the throwaway server before the tool it holds, reusing the file's existing _delete_owned helper so a cleanup failure is reported rather than swallowed by a bare suppress(Exception). Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
_mcp_tool_call runs initialize() then call_tool() on the same session, so the SDK's anyio TaskGroups wrap a real timeout in ExceptionGroup twice (per _unwrap_exception_group's own docstring). A bare 'except McpError' never catches that shape, so a regression would still surface as a raw nested traceback instead of the intended diagnostic. Widen the except to (McpError, ExceptionGroup) and unwrap with the file's existing _unwrap_exception_group helper, matching the pattern already used by TestTokenLifecycle.test_scoped_token_denied_tool_execute. Also reinstate the elapsed assertion. _CLIENT_TIMEOUT bounds a single send_request round trip, not the whole call, and this call makes two (initialize, call_tool) after session setup, so the honest ceiling is a small multiple of _CLIENT_TIMEOUT rather than the raw value or a dropped check. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
Document the sandboxed schema-validation worker process in the CHANGELOG under Unreleased: pattern/patternProperties schemas now validate in a killable worker, registration behavior is unchanged, and a tool's outputSchema is withheld when it carries a regex keyword. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
Both call sites in mcpgateway/services/tool_service.py invoked the schema validator synchronously from async methods. SandboxPool.submit() blocks on Future.result(timeout=...) while a worker process validates a regex-bearing schema, so a hostile request held the event loop for the sandbox's timeout budget on every call, not just once. Wrap both calls in asyncio.to_thread, matching the existing jq offload pattern in this file. _extract_and_validate_structured_content stays synchronous; only the call site changes, so its doctests and its architectural documentation (Validator B) are unaffected. Adds a regression test driving the real production entry point (preview_tool_invocation) with a catastrophic input_schema, measuring event-loop lateness. Proven both directions: fails at 1.01s stall with the offload removed, passes at 0.006s with it restored -- the existing sandbox test alone could not catch this, since it wraps its own call in asyncio.to_thread and so never exercises whether production offloads it. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
Rebasing onto origin/main conflicted only in .secrets.baseline, resolved with main's version per AGENTS.md merge-conflict guidance. Regenerating here recomputes the baseline against the rebased tree so it reflects the actual current file set and line numbers rather than a stale merge. 543 secrets reviewed: no unaudited, live, or newly-real findings. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
msureshkumar88
force-pushed
the
fix/bound-schema-pattern-evaluation
branch
from
September 22, 2026 14:34
15ee4da to
809e752
Compare
gandhipratik203
approved these changes
Sep 22, 2026
gandhipratik203
left a comment
gandhipratik203
left a comment
Collaborator
There was a problem hiding this comment.
LGTM! Both comments adressed.
This branch has not been deployed
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
2 participants
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.
🔗 Related Issue
No linked public issue.
📝 Summary
Runs JSON Schema validation in a killable worker process whenever a schema carries a
patternorpatternPropertieskeyword. Schemas without either keyword validate inline, unchanged from today.Adds three settings to
mcpgateway/config.py:regex_timeout_seconds(default1.0),regex_workers(default2), andregex_max_subject_bytes(default262144). No setting disables the sandbox.Extracts the generic worker-pool lifecycle from
mcpgateway/utils/jq_runner.pyinto a newmcpgateway/utils/sandbox_pool.py.jq_runnernow builds onsandbox_pool; its own behavior is unchanged.A tool's
outputSchemais withheld from the advertised MCP tool when it carries apatternorpatternPropertieskeyword, because the MCP SDK validates that schema outside the gateway's bounded validation path. This is a visible protocol change: such a tool presents without anoutputSchemato MCP clients. Server-side output validation of the same data is unaffected.Registration behavior is unchanged. A schema carrying a regex keyword is still accepted, and an operator warning names the gateway, tool, and pattern length.
📏 Reviewability
🏷️ Type of Change
🧪 Verification
make ruff bandit verifyruffandverifypass; 8 pre-existingbanditfindings remain, none in files this PR touchesmake testmake coverage diff-covertests/live_gateway/e2e/gains a black-box case exercising this path against a running gateway. It has not run locally; this PR is its first run in CI.✅ Checklist
make black isort pre-commit)📓 Notes (optional)
mcpgateway/utils/jq_runner.pyshrinks by roughly 200 lines as its pool lifecycle moves into the sharedsandbox_poolmodule; its public API and settings are unchanged.