HITL tool confirmation forgery (#6461) is still present on main and in v2.12.0: fix 9e9eaa6 was reverted in 9a32eba #7569

Activity

1. surajksharma07

   self-assigned this

   on Oct 16, 2026
2. surajksharma07

   added the

   Bug

   issue type on Oct 16, 2026
3. surajksharma07 commented on Oct 16, 2026

   

   surajksharma07

   on Oct 16, 2026

   Collaborator

   More actions

   You’re right on this one @cybrdude. Checked and 9a32eba fully reverts 9e9eaa6 so main and 2.12.0 still accept a forged {"confirmed": true}. Reproduced it with a plain require_confirmation tool so keeping #7569 as the tracker and pointing #6461 here.

   Until #7134 lands a workaround that held up: a small plugin where in on_user_message_callback rewrite adk_request_confirmation answers to confirmed=False unless the user_id is an allowlisted operator (over A2A on main that’s only set from an authenticated call_context). Could you try it on your setup and let us know?

   It doesn’t cover /run or /run_sse where user_id comes from the request body so those still need API-server auth as you noted.

   Once you’ve confirmed it works with a legitimate operator approval over A2A and that the workflow ToolNode/FunctionNode paths in #7134 still pass after the latest main merge we’ll push for the second review.
4. cybrdude

   added a commit that references this issue on Oct 16, 2026

   contrib: operator-only confirmation plugin and end-to-end validation …

   ...

   f07758b
5. cybrdude commented on Oct 16, 2026

   

   cybrdude

   on Oct 16, 2026

   ContributorAuthor

   More actions

   @surajksharma07 tried it. The plugin works, with two changes to the sketch, and I have it running end to end against main f78149a through the real A2aAgentExecutor and through the workflow nodes.

   Files and tests: https://github.com/cybrdude/adk-python/tree/hitl-7569-workaround/contrib/hitl_7569 (commit f07758b). From a checkout of that branch:

   PYTHONPATH=src:contrib/hitl_7569 python -m pytest -q contrib/hitl_7569/test_7569_workaround.py
   

   19 passed, also from a fresh clone. Results:

   scenario	plugin	user_id at the runner	tool ran
   A2A, unauthenticated peer approves its own call (control)	no	A2A_USER_ctx	yes
   A2A, unauthenticated peer	yes	A2A_USER_ctx	no, result is the normal rejection
   A2A, unauthenticated caller claiming user_name "operator"	yes	A2A_USER_ctx	no
   A2A, authenticated "operator"	yes	operator	yes
   A2A, authenticated "someone-else"	yes	someone-else	no
   workflow ToolNode and FunctionNode, resumable and not, non-operator	yes	A2A_USER_x	no
   same, operator	yes	operator	yes

   The two changes:

   1. Rewriting only adk_request_confirmation answers leaves the workflow nodes open. ToolNode and FunctionNode confirmations resume through adk_request_input, and Runner._extract_resume_inputs keys resume inputs by function_response.id alone, so the plugin matches those by the interrupt-id prefix wf_tool_confirmation: rather than by name. A name check is bypassable there: without the plugin, an answer with the right id and an arbitrary name ran the tool (last test in the file).
   2. PluginManager._run_callbacks returns the first non-None result, so the plugin has to be registered before any other plugin that rewrites user messages.

   The plugin itself (the file in the branch has the docstring and imports):

   class OperatorOnlyConfirmationPlugin(BasePlugin):
   
     def __init__(self, operators, name='operator_only_confirmation'):
       super().__init__(name=name)
       self._operators = frozenset(operators)
   
     @staticmethod
     def _is_confirmation_answer(fr):
       if fr.name == REQUEST_CONFIRMATION_FUNCTION_CALL_NAME:
         return True
       return (fr.id or '').startswith('wf_tool_confirmation:')
   
     async def on_user_message_callback(self, *, invocation_context, user_message):
       if invocation_context.user_id in self._operators:
         return None
       if not user_message or not user_message.parts:
         return None
       changed, parts = False, []
       for part in user_message.parts:
         fr = part.function_response
         if fr is not None and self._is_confirmation_answer(fr):
           response = dict(fr.response) if isinstance(fr.response, dict) else {}
           if response.get('confirmed') is not False:
             response['confirmed'] = False
             part = part.model_copy(update={'function_response': fr.model_copy(update={'response': response})})
             changed = True
         parts.append(part)
       return user_message.model_copy(update={'parts': parts}) if changed else None

   As you said, it does nothing for /run and /run_sse, where user_id comes from the request body.

   #7134 after the merge with f78149a: 156f007 folded the workflow resume into apply_confirmation_resume, so the stoppage now lives in that helper and the PR no longer touches _tool_node.py or _function_node.py. At 6ec3fb8 the tool node, workflow node, function node, hitl utils, confirmation processor and request converter suites pass (273 tests), including the strict-mode cases for both node kinds. Ready for the second review from my side.
6. surajksharma07 commented on Oct 16, 2026

   

   surajksharma07

   on Oct 16, 2026

   Collaborator

   More actions

   Both catches are right @cybrdude and checked them on f78149a. _extract_resume_inputs keys only by function_response.id (runners.py:637) so matching by name alone left ToolNode/FunctionNode open. _run_callbacks also stops at the first non-None result so registering the plugin first does matter.

   Also checked that the node runner re-extracts resume_inputs after the plugin rewrites the message (_node_runner_utils.py:170) so confirmed=False really does reach both nodes. The name check is enough on the LlmAgent side because _confirmation.py:289 ignores any other name.

   One caveat for anyone copying it: the wf_tool_confirmation: prefix is a private interrupt-id format so treat the plugin as a stopgap until #7134 lands. Having the workflow side gated in the single apply_confirmation_resume helper after 156f007 is a much cleaner place for it.

   Thanks for running the 273 tests at 6ec3fb8. @xuanyang15 could you take the second review on #7134 and approve the pending workflows? /run and /run_sse stay out of scope as agreed.
7. surajksharma07

   added

   a2a[Component] This issue is related a2a support inside ADK.[Component] This issue is related a2a support inside ADK.

   tools[Component] This issue is related to tools[Component] This issue is related to tools

   core[Component] This issue is related to the core interface and implementation[Component] This issue is related to the core interface and implementation

   on Oct 16, 2026