fix: hide input values from tool validation error messages - #3582
MohammadaminAlbooyeh wants to merge 1 commit into
Conversation
Tool validation errors previously echoed the rejected input value via pydantic's default str(ValidationError) format, leaking PII/PHI in servers that handle sensitive data. Set hide_input_in_errors=True on ArgModelBase.model_config so the generated arg model never includes input_value in its error messages. The error still describes the rule (type, constraint) but not the data. Fixes modelcontextprotocol#3572
github-actions
Bot
added
the
missing-issue-link
|
This PR has been closed automatically. This repo only keeps pull requests open when they come from a maintainer, or from a contributor a maintainer has assigned to the linked issue, and you aren't currently assigned to #3572. If a maintainer assigns you to #3572, this PR reopens on its own and there's nothing more you need to do here. Assignment is a maintainer call based on capacity; comments that only ask to be assigned don't factor in. What does help is engaging on the issue itself by confirming the repro, explaining why it matters for your use case, or describing the approach you'd take. You're welcome to keep pushing commits here (just avoid force-pushing, since GitHub can't reopen a rewritten branch), but that on its own won't get the PR reviewed or the issue assigned, and realistically most auto-closed PRs stay closed. There's no need to open a new PR either way. CONTRIBUTING.md has the full reasoning, but in short:
Maintainers: reopen, remove |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Nested user-defined models may still expose rejected values, and the regression coverage needs strengthening.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (3)
What changed in this PR
Fixes leakage of rejected input values in tool validation errors.
Changes:
- Enables hidden input values for generated argument models.
- Adds regression coverage for sanitized validation errors.
| File | Description |
|---|---|
tests/server/mcpserver/test_func_metadata.py |
Tests that rejected values are omitted from validation errors. |
src/mcp/server/mcpserver/utilities/func_metadata.py |
Configures generated argument models to hide validation inputs. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return kwargs | ||
|
|
||
| model_config = ConfigDict(arbitrary_types_allowed=True) | ||
| model_config = ConfigDict(arbitrary_types_allowed=True, hide_input_in_errors=True) |
| meta = func_metadata(fn) | ||
| with pytest.raises(Exception) as exc_info: | ||
| meta.arg_model.model_validate({"name": "Alice", "age": "not-a-number"}) |
| def fn(name: str, age: int) -> str: ... # pragma: no branch | ||
|
|
||
| meta = func_metadata(fn) | ||
| with pytest.raises(Exception) as exc_info: |
|
I've implemented a fix and opened PR #3582. The approach: set hide_input_in_errors=True on ArgModelBase.model_config — a one-line change that suppresses input_value from all pydantic validation errors for tool arguments. All 59 existing tests pass. Happy to address the Copilot review comments (nested model coverage, ValidationError instead of Exception, end-to-end test) if a maintainer assigns me. |
Fixes #3572
Motivation and Context
Tool validation errors echoed the rejected
input_valueback to the client via pydantic's defaultstr(ValidationError)format. Servers handling sensitive input (PII/PHI, credentials) must not leak data in error messages.How Has This Been Tested?
Added
test_validation_error_does_not_echo_input_valueintests/server/mcpserver/test_func_metadata.py. All 59 tests in that file pass locally.Breaking Changes
None. Error messages are less verbose (no data echoed), but the rule description (
type=string_type,int_parsing, etc.) is still present.Types of changes
Checklist
help wanted, or I'm a maintainer)