Repository navigation
Python: Enforce FIDES MCP IFC labels - #9879
Eduard van Valkenburg (eavanvalkenburg) wants to merge 1 commit into
Conversation
GitHub Actions
Inactive
GitHub Actions
Inactive
agent-framework-automation
Bot
added
the
python
Code coverage overviewLanguages: Python Python / code-coverage/python
View coverage of the 7 most impacted files.
ℹ️ Calculations represent line coverage. Results may change as calculations are updated. Updated |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (1 commit(s)): 39ebe60bfd7a
Model: gpt-5.6-sol
Overview
This PR adds an independently bounded private carrier for MCP IFC metadata and defers detailed-error label commits until terminal result finalization, with strong regression coverage for payload exhaustion, retries, session scope, invalid identity labels, and hidden replay. The carrier is invocation-local and its private metadata is removed from normal finalized results. One Medium regression remains: malformed IFC on a successful result can leave a non-serializable private authority marker in public result properties when label-tracking middleware is absent.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
1 verified finding remained after source verification (1 medium) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/core/agent_framework/security.py
| metadata={"source": "invalid_mcp_ifc"}, | ||
| ), | ||
| ) | ||
| props[_AUTHORITATIVE_SECURITY_LABEL] = _INTERNAL_RESULT_MARKER |
There was a problem hiding this comment.
When a server supplies malformed or partial IFC, this object-valued marker remains in additional_properties unless LabelTrackingFunctionMiddleware consumes it. apply_mcp_security_labels() wraps tools independently of that middleware, so a default (trust_server_ifc=False) call can return this marker and json.dumps(Content.to_dict()) raises TypeError, allowing malformed remote metadata to break result or session serialization. Please keep the fail-closed label but ensure the private authority marker is consumed before the public result is returned, or represent authority without placing a non-serializable object in result properties.
39ebe60 to
7f43e7d
Compare
GitHub Actions
Inactive
There was a problem hiding this comment.
🟡 Changes recommended
Critical IFC enforcement gaps allow malformed identity labels and post-wrapper result parsing to bypass security restrictions, including exposing restricted error details.
3 open issues
What changed in this PR
This PR adds the final MCP enforcement layer of the FIDES hardening stack, preserving information-flow labels across payload limits, retries, and session persistence.
Changes:
- Retains server IFC metadata independently of generic Host payload budgets and fails closed on invalid or unretainable metadata.
- Labels terminal MCP errors in the captured session scope and prevents carrier metadata from returning to hidden placeholders.
- Adds regressions for budget exhaustion, identity restrictions, retries, serialization, and hidden-result replay.
| File | Description |
|---|---|
python/packages/core/tests/test_security.py |
Tests fail-closed handling of invalid and unretained IFC. |
python/packages/core/tests/core/test_mcp.py |
Adds MCP label enforcement, error, persistence, and replay regressions. |
python/packages/core/agent_framework/security.py |
Enforces retained IFC and captures terminal-error labels and scope. |
python/packages/core/agent_framework/_tools.py |
Adds private carrier state, error finalization, and hidden-placeholder metadata handling. |
python/packages/core/agent_framework/_mcp.py |
Introduces independently bounded IFC retention and propagation. |
Review effort: Balanced
| if self.security_meta is not None: | ||
| updated_item.additional_properties[_MCP_TOOL_RESULT_SECURITY_META_KEY] = self.security_meta |
| dynamic = _label_from_mcp_meta(security_meta_mapping.get("meta")) | ||
| invalid_ifc = dynamic is None |
| for key in carrier.exclusive_outer_keys | carrier.exclusive_item_keys: | ||
| updated_item.additional_properties.pop(key, None) |
Motivation & Context
Generated MCP results can lose server-provided
_meta.ifcwhen the generic Host payload budget is exhausted. This canmake successful results fall back to a less restrictive local label. Detailed MCP errors also pass through generic
function-error conversion, where labels can be lost, committed for a swallowed retry, applied to the wrong session
scope, or serialized with private marker objects. Invalid or oversized identity IFC must not inherit local principals.
This is the fifth and final layer of the upstream replacement stack for #9652. It builds on #9875, #9876, #9877, and
#9878 and isolates MCP IFC enforcement from the lower service-history, replay-authority, and AG-UI provenance layers.
Description & Review Guide
_meta.ifcthrough an independently bounded privatecarrier outside generic Host payload budgets. Successful results combine local policy with server IFC, while
invalid, oversized, or unretainable IFC fails closed, including identity confidentiality and principal handling.
Detailed MCP errors receive the same labels after retry finalization, preserve argument-derived confidentiality and
local identity principals, update the invocation's captured session scope, and remove private markers before
serialization. Hidden built-in MCP placeholders no longer regain carrier metadata during finalization, so replay
stays integrity-clean.
payload exhaustion, retries, persistence, session restore, and hidden replay. There are no public API or breaking
changes. Validation passed with:
cd python && uv run pytest -q packages/core/tests/core/test_mcp.py -k 'exhausted_aggregate_budget_preserves_ifc_security_label or exhausted_aggregate_budget_enforces_ifc_on_detailed_mcp_error or retried_mcp_error_does_not_commit_swallowed_label or mcp_error_finalizer_updates_invocation_session_scope or mcp_error_without_ifc_preserves_local_identity_principals or restriction_only_mcp_error_preserves_private_argument_fallback or oversized_identity_ifc_fails_closed_without_confidentiality_downgrade or invalid_identity_ifc_cannot_inherit_local_principals or hidden_builtin_mcp_result_replays_without_tainting_context'(10 passed)cd python && uv run pytest -q packages/core/tests/test_security.py -k 'stamp_contents_invalid_meta_fails_closed or stamp_contents_unretained_ifc_fails_closed'(5 passed)cd python && uv run pytest -q -n auto --dist worksteal packages/core/tests/core/test_mcp.py packages/core/tests/test_security.py(passed)cd python && uv run poe syntax -P core -C(passed)cd python && uv run poe pyright -P core(0 errors and 0 warnings)cd python && uv run poe test-typing -P core(mypy, pyrefly, pyright, ty, and zuban passed)cd python && uv run poe test -P core(9,174 passed, 20 skipped, 2 xfailed; 92% coverage)behavior, terminal-error finalizer timing, session-scope capture, and hidden-placeholder metadata cleanup. This layer
intentionally excludes service-history reuse, AG-UI or general replay logic, and the deferred follow-up to
automatically hide untrusted MCP detailed-error text.
Related Issue
Fixes #9652
#9652 is the original source draft. This PR is its MCP IFC enforcement replacement layer, stacked as
#9875 -> #9876 -> #9877 -> #9878 -> this PR rather than a competing implementation.
AI Assistance
AI assistance details: GitHub Copilot assisted with implementation, regression tests, validation, and this PR
description.
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.