Repository navigation
feat: add human_approval helper with pending decision contract - #8169
Shivani . (Shivani767) wants to merge 1 commit into
Conversation
|
This PR has been automatically closed because you are not assigned to the linked issue. External contributors must be assigned to an issue before opening a PR for it. Please:
Maintainers: reopen this PR or remove the |
|
Hi maintainers! I've created a PR for this issue (#8169). Could you please assign me to this issue? Thanks! |
![corridor-security[bot]](https://avatars.githubusercontent.com/in/1177339?s=60&v=4){kind=small}
There was a problem hiding this comment.
Security Issues
- Approval bypass via client-controlled decision IDs
human_approval()acceptsresume_command_idfrom the untrusted resume payload and creates a new pending decision when no existing record is found. An attacker who can resume an interrupted run can supply a fresh ID, avoid an existing expired/cancelled/mismatched decision, then resume again withaction: "approve"to execute an approval-gated tool. The decision ID should be generated and stored server-side before the first interrupt, and unknown resume IDs should fail closed.
| resume_command_id=resume_command_id, | ||
| expires_at=datetime.now() + expires_in, | ||
| ) | ||
| store.save(pending_decision) |
There was a problem hiding this comment.
The approval nonce is taken from the untrusted resume payload, and an unknown ID causes a new pending decision to be created instead of failing closed:
resume_command_id = resume_payload["resume_command_id"]
pending_decision = store.get(resume_command_id)
if pending_decision is None:
# First pass: create pending record and interrupt again
pending_decision = PendingDecision(
checkpoint_id=checkpoint_id or str(uuid.uuid4()),
node_name=node_name,
tool_name=tool_name,
tool_call_id=tool_call_id,
args_digest=args_digest,
decision_shape=decision_shape,
resume_command_id=resume_command_id,
expires_at=datetime.now() + expires_in,
)
store.save(pending_decision)Because resume_command_id is client-controlled here, a caller who can resume an interrupted graph can provide a fresh ID to bypass an existing expired, cancelled, or mismatched approval record. The helper then creates a new pending decision for that attacker-chosen ID and allows it to be approved on the next resume.
Remediation: Generate resume_command_id server-side before the first interrupt(), save the pending decision before exposing the approval request, include that server-generated ID in the interrupt payload, and treat store.get(resume_command_id) is None as a validation failure. Also bind the stored decision to the expected checkpoint/tool/tool-call/args before approval.
For more details, see the finding in Corridor.
Provide feedback: Reply with whether this is a valid vulnerability or false positive to help improve Corridor's accuracy.
Today's require-approval gate is a synchronous, in-process callback with a 30s fail-closed timeout and no way to resume a decision out of band. Add PendingApprovalRegistry (TS + Python) as a keyed, server-generated-id store for require-approval decisions, wired into governTool()/govern_tool() alongside the existing synchronous onApprovalRequired path (unchanged for callers who don't opt in). - registerPending()/get()/resolvePending() plus alias tolerance (registerAlias) so a rewritten thread/conversation id still resolves to the same pending entry -- modeled on the bug fixed in microsoft/agent-framework#6908. - resolvePending() never creates a new entry for an unrecognized id -- closes the resume-token bypass Corridor's security bot found in langchain-ai/langgraph#8169's human_approval() helper. - Edited arguments supplied to resolvePending() are re-run through the classifier before an "allow" is accepted; a still-risky edit is denied even after approval, with a real test proving it on both sides. - governTool()/govern_tool() persist a pending approval BEFORE invoking the synchronous handler. A genuine handler answer (allow/deny) closes the entry out as terminal; a fail-closed default (no handler, timeout, or a throwing handler) leaves it open for later resolution -- otherwise the async path would be permanently unreachable. - resumePendingApproval()/resume_pending_approval() closes the loop: given the resolved outcome, it actually invokes the tool and appends one trace entry with approvedBy populated end-to-end, not only on the sync path. 60 new TypeScript tests, 53 new Python tests, all existing suites green.
Fixes #8026
This PR adds a lightweight
human_approval()helper built on LangGraph's existinginterrupt()andCommand(resume=...)primitives, providing a reusable HITL approval workflow.Fixes #8026
Safety Guarantees
Tests
Includes comprehensive coverage in two test files:
test_human_approval.py: All main functionalitytest_human_approval_fail_closed.py: Specific focus on safety/fail-closed behaviorHow did you verify your code works?
Notes
This implementation intentionally avoids introducing a new graph-level primitive (like an ApprovalNode) and instead builds on existing LangGraph execution semantics, as requested by maintainers in the issue discussion.