Skip to content

Python: Harden FIDES security state and policy enforcement - #9652

Draft
Eduard van Valkenburg (eavanvalkenburg) wants to merge 28 commits into
mainfrom
fix-fides-security-hardening
Draft

Eduard van Valkenburg (eavanvalkenburg) wants to merge 28 commits into
mainfrom
fix-fides-security-hardening

Conversation

@eavanvalkenburg

Copy link
Copy Markdown
Member

Motivation & Context

A private security review identified several cases where FIDES could lose or weaken information-flow state across transcript replay, MCP result retention, hidden argument validation, durable serialization, and invalid tool metadata. These gaps could cause policy decisions to use less restrictive integrity or confidentiality information than the data warranted.

This change hardens SecureAgentConfig and the FIDES middleware pipeline while preserving restored session state and approval-resume behavior. Context-provider message labeling remains separate follow-up work because context providers are trusted plugins with an existing documented trust boundary.

Description & Review Guide

  • What are the major changes?
    • Reconstruct missing FIDES state from caller-replayed and history-provider messages, using conservative labels for unstamped assistant/tool data while keeping restored state authoritative.
    • Retain MCP _meta.ifc in an independently bounded security carrier so generic Host-payload budget exhaustion cannot discard the label; malformed or unretainable supplied IFC metadata fails closed.
    • Split hidden-argument handling into policy preflight and post-authorization materialization so expanded values do not reach Pydantic validators or tool bodies before policy permits the call.
    • Serialize opaque FIDES state with ASCII escapes so lone Unicode surrogates round-trip through JSON and MessagePack session stores.
    • Parse FIDES tool metadata centrally and block unknown or mistyped values with an auditable invalid_policy violation.
    • Add regression coverage across core and AG-UI approval/replay paths and update the FIDES guidance.
  • What is the impact of these changes?
    • Transcript replay without persisted FIDES state now restores a conservative cumulative label instead of starting trusted/public.
    • Invalid security metadata that previously warned and continued now blocks execution.
    • Validators no longer run on expanded hidden values for calls rejected by FIDES.
    • Existing restored sessions and AG-UI approval resumes retain their authoritative state and behavior.
    • Non-history context-provider messages remain under the documented trusted-plugin model pending the follow-up design.
  • What do you want reviewers to focus on?
    • The boundary between reconstructing missing replay state and preserving restored session state.
    • Approval binding and argument-materialization ordering, including validator transformations and runtime kwargs.
    • The independent MCP IFC carrier, its size bound, and fail-closed handling.

Related Issue

N/A - security hardening based on a privately reported issue. Per SECURITY.md, vulnerability details are not tracked in a public GitHub issue; maintainer direction was explicitly confirmed before implementation. The context-provider labeling follow-up is tracked separately in #9645 and is intentionally not closed by this PR. No competing open PR was found.

AI Assistance

  • No material AI assistance was used.
  • This is an AI-assisted contribution. I reviewed, understood, and verified all submitted content and accept responsibility for it.

AI assistance details: GitHub Copilot assisted with the security assessment, implementation, regression tests, documentation updates, and PR description.

Contribution Checklist

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • This PR links to an agreed issue with no competing open PR, or the Related Issue section documents a trivial-change or repository-automation exception.
  • This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.

Copilot AI balanced review requested due to automatic review settings October 23, 2026 14:15
@agent-framework-automation agent-framework-automation Bot added documentation Usage: [Issues, PRs], Target: documentation in the code base and learn docs python Usage: [Issues, PRs], Target: Python labels Oct 23, 2026
@github-code-quality

github-code-quality Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Code coverage overview

Languages: Python

Python / code-coverage/python

main 9e971db fix-fides-security-h... 750c3bc +/-
92% 92% 0%
View coverage of the 7 most impacted files.
File main 9e971db fix-fides-security-h... 750c3bc +/-
packages/core/a...amework/_mcp.py 91% 90% -1%
packages/ag-ui/...i/_agent_run.py 90% 90% 0%
packages/core/a.../_middleware.py 96% 96% 0%
packages/core/a...ework/_tools.py 95% 95% 0%
packages/core/a...work/_agents.py 95% 95% 0%
packages/core/a...ork/security.py 90% 91% +1%
packages/ag-ui/...age_adapters.py 93% 94% +1%

ℹ️ Calculations represent line coverage. Results may change as calculations are updated.


Updated October 30, 2026 07:04 UTC

Comment thread python/packages/core/agent_framework/_tools.py Fixed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Replay reconstruction can miss loaded history and lose identity-scoped restrictions, leaving downstream policy checks weaker than required.

3 open issues
What changed in this PR

This PR hardens Python FIDES information-flow enforcement across transcript replay, MCP results, hidden arguments, and persisted session state. Non-history context-provider labeling remains a separate follow-up.

Changes:

  • Reconstruct missing replay security state while preserving restored labels.
  • Retain MCP security metadata independently and reject invalid tool policies.
  • Defer hidden-value validation until authorization, support surrogate-safe serialization, and add regression tests and guidance.
File Description
python/​samples/​02-agents/​security/​FIDES_DEVELOPER_GUIDE.md Documents replay handling, metadata validation, and authorization ordering.
python/​packages/​core/​tests/​test_security.py Adds policy, replay, serialization, and validator regressions.
python/​packages/​core/​tests/​core/​test_mcp.py Tests security-label retention when payload budgets are exhausted.
python/​packages/​core/​agent_framework/​security.py Implements replay reconstruction, policy parsing, and deferred argument materialization.
python/​packages/​core/​agent_framework/​_tools.py Integrates deferred security argument materialization into invocation.
python/​packages/​core/​agent_framework/​_middleware.py Tracks the active function middleware pipeline.
python/​packages/​core/​agent_framework/​_mcp.py Adds an independently bounded security-metadata carrier.
docs/​features/​FIDES_IMPLEMENTATION_SUMMARY.md Clarifies enforced flows and context-provider trust boundaries.

Review effort: Balanced

Comment thread python/packages/core/agent_framework/security.py Outdated
Comment thread python/packages/core/agent_framework/security.py Outdated
Comment thread python/packages/core/agent_framework/security.py

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAF Automated Review — Iteration 1

Result: Findings reported
Scope: full PR (1 commit(s)): 634139edf460
Model: gpt-5.6-sol

Overview

The PR adds conservative replay reconstruction, independently bounded MCP IFC retention, delayed hidden-argument materialization, ASCII-safe durable state, and centralized tool-policy parsing. Its strongest guardrails are fail-closed MCP labeling, identity-bound argument materialization, unconditional ContextVar cleanup, and targeted regression coverage. Two gaps remain: restored sessions stop conservatively labeling newly supplied assistant/tool input, and explicit null enum policies bypass the new invalid-policy path.

Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
2 verified findings remained after source verification (1 high, 1 medium) across 1 file. Details are attached to the affected lines below.

Affected areas: python/packages/core/agent_framework/security.py

Comment thread python/packages/core/agent_framework/security.py Outdated
Comment thread python/packages/core/agent_framework/security.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread python/packages/core/agent_framework/security.py
Comment thread python/packages/core/agent_framework/security.py Outdated
Comment thread python/packages/core/agent_framework/security.py Outdated
Comment thread python/packages/core/agent_framework/security.py
Comment thread python/packages/core/agent_framework/_agents.py Fixed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved confidentiality downgrades and synthetic-history tainting undermine the intended security and restored-session behavior.

2 open issues
1 previously missed issue

In code that hasn't changed since last review

Medium severity Synthetic skipped tool results incorrectly taint restored sessions

python/​packages/​ag-ui/​agent_framework_ag_ui/​_agent_run.py:3223

When a user abandons an unanswered client-tool call from the backend snapshot, normalization inserts a framework-generated “Tool execution skipped” result without snapshot provenance. FIDES treats that synthetic result as new UNTRUSTED/PRIVATE data, tainting the restored session and blocking unrelated protected tools. Preserve trusted provenance for synthetic results derived from backend-owned calls without trusting actual client-supplied results, and add an abandoned-call continuation regression with FIDES enabled.

4 resolved since last review

Review effort: Balanced

Comment thread python/packages/core/agent_framework/security.py Outdated
Comment thread python/packages/core/agent_framework/security.py Outdated
@eavanvalkenburg

Copy link
Copy Markdown
Member Author

Addressed the current-head Copilot review’s previously missed synthetic skipped-result issue in 4b3fd9660. AG-UI now propagates backend-snapshot provenance only to framework-generated skipped results derived from backend-owned calls; real client tool results remain untrusted. Added an abandoned-call continuation regression with FIDES enabled.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Cross-cutting security-boundary changes and unresolved label-handling defects require fixes and final security-focused human review.

1 open issue
1 previously missed issue

In code that hasn't changed since last review

Medium severity Exclude unchanged authenticated placeholders from integrity fallback

python/​packages/​core/​agent_framework/​security.py:1147

Replaying an unchanged, authenticated hidden result as ordinary caller input still applies the UNTRUSTED fallback, even with restored session state. This taints an otherwise clean session and blocks unrelated protected tools although the payload remains hidden. Exempt authenticated-placeholder-only messages from the integrity fallback, retain conservative handling for modified or visible data, and add a caller-input regression.

2 resolved since last review

Review effort: Balanced

Comment thread python/packages/core/agent_framework/security.py
@eavanvalkenburg

Copy link
Copy Markdown
Member Author

Addressed the latest Copilot review’s previously missed authenticated-placeholder caller-input issue in a08f596e4. Replay reconstruction now skips the UNTRUSTED fallback only when the message contains authenticated session-owned hidden placeholders and no visible data; modified placeholders and visible/function-call content remain conservative. Added a caller-input regression that preserves TRUSTED/PRIVATE and allows an unrelated protected action.

@eavanvalkenburg

Copy link
Copy Markdown
Member Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAF Automated Review — Iteration 9

Result: No findings
Scope: 1 net-new commit(s): 7403b9de9f6e
Model: gpt-5.6-sol

Overview

This incremental update centralizes approval-control authority token generation, fails closed on recursive approval content, propagates the effective conversation identity into provider preparation, and reconstructs history whenever the authoritative FIDES label is absent. The added tests cover cyclic approval replay, missing-label state, service continuations without inspectable history, and agent-level default conversation IDs. No Critical, High, or Medium defect introduced by this review range remained after reconciliation.

Reviewed the supplied incremental change set across correctness, security/reliability, architecture, and failure behavior.
No publishable findings remained after source verification for this scope.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Human assessment is needed because replay labeling, incomplete service-history reconstruction, and client-edited call provenance can misclassify sessions or bypass trust protections.

No open issues 🎉

3 resolved since last review

Review effort: Balanced

@eavanvalkenburg

Copy link
Copy Markdown
Member Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAF Automated Review — Iteration 10

Result: No findings
Scope: 45 net-new commit(s): 3f61bf5bdfc0, 3f2705269ab8, 1c19834d996b, 0c1e6eed959c, f86c0ebd86cd, 172ed9139441, 18adf808a57e, 1168c001dd22, b1a4c9494ee4, 6e9167e101f7, 79f848f3a322, 7de7026a32a5, c4de86d41e34, 45ea037d474b, a9f29d0e324b, 14ce83b5635b, 63d0998c9e3a, 10a94cf736d7, 3a70a81a260b, abaf33a3c8eb, 61357e4950a2, 981f70e3c7bb, 5a950ad5d3ea, 6afcecd428dd, 20458faef1e5, 4a272d2b8e96, 9b72ed4f96bc, 49b715debe37, 2e931679dcf6, c2a8f91b3070, 5f469d26d519, 8624e76997b9, b2d552e3c8e9, 581751842e86, fc3c3c078937, 569a39e7ad7c, 13c22b50e158, 30ce7e39ce04, b6d068d730a7, 908ed6fa908e, 268461487a02, 9ef867212c7b, ce7d17463537, 9e971dba685e, 258de1ffa91e
Model: gpt-5.6-sol

Overview

This incremental range merges the current main branch into the PR and preserves the FIDES hardening already present at the previous head. The changed MCP task-discovery behavior is constrained to admitting task-optional tools while continuing to reject task-required tools, with focused tests covering registration and invocation. The panel found no new Critical, High, or Medium defect that can be anchored to a changed right-side line in the supplied diff.

Reviewed the supplied incremental change set across correctness, security/reliability, architecture, and failure behavior.
No publishable findings remained after source verification for this scope.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved critical gaps bypass IFC enforcement for MCP error text and allow injected duplicate-key arguments to gain trusted replay authority.

3 open issues

Review effort: Balanced

Comment thread python/packages/core/agent_framework/_mcp.py
Comment thread python/packages/core/agent_framework/security.py
Comment thread python/packages/core/agent_framework/security.py
@eavanvalkenburg

Copy link
Copy Markdown
Member Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAF Automated Review — Iteration 11

Result: Findings reported
Scope: 1 net-new commit(s): 2eecc48ea956
Model: gpt-5.6-sol

Overview

This increment closes three concrete FIDES gaps: it rejects duplicate-key approval replays, avoids relabeling state-covered public history, and carries bounded MCP IFC metadata through detailed error conversion. The new tests exercise streaming and non-streaming history, nested duplicate keys, exhausted payload budgets, invalid IFC, and downstream policy enforcement. One residual lifecycle issue remains because the MCP exception path commits its label before retry middleware determines whether that failed attempt becomes conversation content.

Reviewed the supplied incremental 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

Comment thread python/packages/core/agent_framework/security.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

MCP error handling exposes a private authority marker that breaks serialization and omits configured identity principals on errors without IFC metadata, causing later authorization failures.

2 open issues
3 resolved since last review

Review effort: Balanced

Comment thread python/packages/core/agent_framework/security.py
Comment thread python/packages/core/agent_framework/security.py Outdated
@eavanvalkenburg

Copy link
Copy Markdown
Member Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAF Automated Review — Iteration 12

Result: Findings reported
Scope: 1 net-new commit(s): dbba990622d1
Model: gpt-5.6-sol

Overview

This incremental change correctly stages MCP error labels on the invocation carrier so a successful retry does not taint the session, and it adds useful guards around serialization and identity-principal fallback. Carrier replacement on each MCP attempt and finalization only for converted exceptions constrain the deferred update. However, the finalizer now runs after the label tracker's task-local session scope has been reset, so shared non-fixed middleware can update the wrong scope and leave the actual session under-labeled.

Reviewed the supplied incremental 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

Comment thread python/packages/core/agent_framework/security.py

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The error finalizer can update the wrong security scope, exposing private or untrusted MCP errors without updating session labels, while legacy-session reconstruction can break continuations or incorrectly taint restored state.

1 open issue
2 resolved since last review

Review effort: Balanced

Comment thread python/packages/core/agent_framework/security.py
@eavanvalkenburg

Copy link
Copy Markdown
Member Author

/review

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

MCP error handling can drop argument-derived privacy restrictions and expose private data to public sinks, while legacy session restoration can fail or add unnecessary taint.

1 open issue
1 resolved since last review

Review effort: Balanced

Comment thread python/packages/core/agent_framework/security.py Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAF Automated Review — Iteration 13

Result: No findings
Scope: 4 net-new commit(s): 7951f33f15f7, 85f3fc6bec6f, dd8cf056ac70, 446722b11303
Model: gpt-5.6-sol

Overview

No publishable findings were identified in the reviewed scope.

Reviewed the supplied incremental change set across correctness, security/reliability, architecture, and failure behavior.
No publishable findings remained after source verification for this scope.

@eavanvalkenburg

Copy link
Copy Markdown
Member Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAF Automated Review — Iteration 14

Result: No findings
Scope: 6 net-new commit(s): 21c5ab124bc6, 36cf53c7c918, 4d28d0a4ba77, 1a6300e28765, efb57506017d, 750c3bcfd3c8
Model: gpt-5.6-sol

Overview

The incremental FIDES correction now preserves the invocation fallback label for valid server IFC in restriction-only mode while retaining authoritative trusted-server behavior and fail-closed handling for malformed metadata. Its end-to-end regression proves that private argument data remains private after an MCP error and blocks a later public sink. The merged .NET changes are similarly constrained by focused tests around DNS-pinned browsing, citation offsets, local-history markers, and declarative workflow behavior; no publishable concern remains in the authoritative range.

Reviewed the supplied incremental change set across correctness, security/reliability, architecture, and failure behavior.
No publishable findings remained after source verification for this scope.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Untrusted MCP errors can bypass automatic hiding and expose raw text to the model, leaving a critical security issue unresolved.

2 open issues
1 resolved since last review

Review effort: Balanced

Comment on lines +5728 to +5731
def commit_error_label() -> None:
invocation_context.metadata["result_label"] = error_label
active_middleware._update_context_label( # pyright: ignore[reportPrivateUsage]
error_label,
Comment on lines +1876 to +1880
await provider.before_run(
agent=self,
session=provider_session,
context=approval_history_context,
state=provider_session.state.setdefault(provider.source_id, {}),

This branch was previously deployed

1 inactive deployment
github-app-auth — 750c3bcf Deployed Oct 30, 2026 by eavanvalkenburg via add_label #26614
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Usage: [Issues, PRs], Target: documentation in the code base and learn docs python Usage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants