Python: fix: bound pending policy approvals - #7996
Python: fix: bound pending policy approvals#7996Patel Namraa (Namraa310806) wants to merge 7 commits into
Conversation
Note on Related WorkA draft PR (#7893) was already opened for this issue, and I reviewed it while working on #7890. Since the draft had not yet progressed to an active PR while the issue remained unresolved in the main codebase, I opened this PR with a more concise approach focused on directly bounding the pending approval state and preserving the existing approval-binding behavior. The intent is not to duplicate or compete with the existing work, but to make sure the underlying bug is fixed with a focused and maintainable approach. I’m happy to adjust or consolidate the implementation based on maintainer feedback. |
| call_id = self._get_call_id(context) | ||
| if call_id: | ||
| # If bounded, evict oldest entry when adding a new unique call_id would exceed limit. | ||
| # Do not evict when updating an existing call_id (re-request scenario). | ||
| if ( | ||
| self._max_pending_approvals is not None | ||
| and call_id not in self._pending_policy_approvals | ||
| and len(self._pending_policy_approvals) >= self._max_pending_approvals | ||
| ): | ||
| # Evict oldest (first) entry | ||
| oldest_call_id = next(iter(self._pending_policy_approvals)) | ||
| del self._pending_policy_approvals[oldest_call_id] | ||
| logger.debug( | ||
| f"Evicted oldest pending approval '{oldest_call_id}' to maintain limit of {self._max_pending_approvals}" | ||
| ) | ||
| self._pending_policy_approvals[call_id] = self._pending_record(context, violations) |
There was a problem hiding this comment.
Should the bounded map stay keyed by approval_id rather than provider call_id? Framework calls can carry a distinct function_call_occurrence_id; _matches_pending_approval looks up that occurrence-aware ID at security.py:1868 and _consume_pending_approval removes it at security.py:1908, but this path inserts by call_id. Once the crash above is fixed, valid occurrence-aware approvals will still miss the stored record and be re-requested instead of executing.
There was a problem hiding this comment.
Thanks for catching this. You're right — the pending-approval map needs to use the same canonical identity as the lookup and consume paths.
I've updated the insertion path to use _get_approval_id(context) instead of the provider call_id, while keeping call_id as part of the existing approval-binding validation.
I also added a regression test covering the function_call_occurrence_id case to verify the full request → store → approve → match → consume lifecycle.
There was a problem hiding this comment.
Could we restore approval_id = self._get_approval_id(context) before building the request? _request_policy_violation_approval now defines only call_id, but passes approval_id at security.py:1971, so every violating call with approval_on_violation=True raises NameError before MiddlewareTermination can return the approval request.
There was a problem hiding this comment.
Thanks, you're right. I had missed restoring the approval_id = self._get_approval_id(context) assignment in _request_policy_violation_approval().
I've restored it and verified that the approval request is now created with the same approval_id used for storing, matching, and consuming the pending approval.
Motivation & Context
PolicyEnforcementFunctionMiddlewarecan retain pending policy-approval entries indefinitely whenapproval_on_violation=Trueand an approval is never consumed.The pending approvals are stored in
_pending_policy_approvals. When applications reuse a middleware instance across many interactions, unconsumed approvals can accumulate over time, causing unbounded memory growth.This PR fixes the lifecycle of those pending approvals while preserving the existing approval-binding and fail-closed security behavior.
Fixes #7890
Description & Review Guide
What are the major changes?
_pending_policy_approvalswith a configurablemax_pending_approvalslimit, defaulting to1000.call_idis encountered from a different session.What is the impact of these changes?
_pending_policy_approvalsfrom growing without bound under repeated unconsumed policy violations.What do you want reviewers to focus on?
max_pending_approvals=1000is an appropriate bound for the experimental FIDES policy-approval flow.Related Issue
Fixes #7890
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.