From ef941b5d7c476f7b744ef776eddedf5f0ae020b8 Mon Sep 17 00:00:00 2001 From: Elad Kalif <45845474+eladkal@users.noreply.github.com> Date: Thu, 26 Mar 2026 09:37:13 +0200 Subject: [PATCH 1/3] Fix LLMApprovalMixin to enforce allow_modifications in execute_complete --- .../airflow/providers/common/ai/mixins/approval.py | 6 ++++-- .../ai/tests/unit/common/ai/mixins/test_approval.py | 12 ++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py b/providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py index 22fa641333b52..d659b631bb34f 100644 --- a/providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py +++ b/providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py @@ -168,8 +168,10 @@ def execute_complete(self, context: Context, generated_output: str, event: dict[ output = generated_output params_input: dict[str, Any] = event.get("params_input") or {} - # If the reviewer provided modified output, return their version - if params_input: + # Only accept modified output when the operator explicitly allows modifications. + # Without this guard a reviewer could craft a request with params_input even + # when allow_modifications=False, bypassing the read-only approval flow. + if self.allow_modifications and params_input: modified = params_input.get("output") if modified is not None and modified != generated_output: log.info("output=%s modified by the reviewer=%s ", modified, responded_by_user) diff --git a/providers/common/ai/tests/unit/common/ai/mixins/test_approval.py b/providers/common/ai/tests/unit/common/ai/mixins/test_approval.py index 7b2d2b90f89b5..08355c3b92237 100644 --- a/providers/common/ai/tests/unit/common/ai/mixins/test_approval.py +++ b/providers/common/ai/tests/unit/common/ai/mixins/test_approval.py @@ -295,6 +295,18 @@ def test_approved_no_modifications_ignores_params_input(self, approval_op): assert result == "original" + def test_approved_no_modifications_rejects_tampered_params_input(self, approval_op): + """When allow_modifications=False, tampered params_input with output must be ignored.""" + event = { + "chosen_options": ["Approve"], + "responded_by_user": "reviewer", + "params_input": {"output": "tampered output"}, + } + + result = approval_op.execute_complete({}, generated_output="original", event=event) + + assert result == "original" + def test_event_missing_responded_by_user(self, approval_op): event = {"chosen_options": ["Approve"]} From 4baff5cf08abe98923ffae9cb537e3e7ea0d894f Mon Sep 17 00:00:00 2001 From: Elad Kalif <45845474+eladkal@users.noreply.github.com> Date: Thu, 26 Mar 2026 11:59:55 +0200 Subject: [PATCH 2/3] fix mypy --- .../ai/src/airflow/providers/common/ai/mixins/approval.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py b/providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py index d659b631bb34f..7a182f9c785ed 100644 --- a/providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py +++ b/providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py @@ -133,7 +133,9 @@ def defer_for_approval( timeout=self.approval_timeout, ) - def execute_complete(self, context: Context, generated_output: str, event: dict[str, Any]) -> str: + def execute_complete( + self: DeferForApprovalProtocol, context: Context, generated_output: str, event: dict[str, Any] + ) -> str: """ Resume after human review. From 886f8a292dbfc6dc7e50cc4266534b20f155619b Mon Sep 17 00:00:00 2001 From: Elad Kalif <45845474+eladkal@users.noreply.github.com> Date: Thu, 26 Mar 2026 20:15:25 +0200 Subject: [PATCH 3/3] fix mypy --- .../ai/src/airflow/providers/common/ai/mixins/approval.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py b/providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py index 7a182f9c785ed..df5929eedc0a6 100644 --- a/providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py +++ b/providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py @@ -133,9 +133,7 @@ def defer_for_approval( timeout=self.approval_timeout, ) - def execute_complete( - self: DeferForApprovalProtocol, context: Context, generated_output: str, event: dict[str, Any] - ) -> str: + def execute_complete(self, context: Context, generated_output: str, event: dict[str, Any]) -> str: """ Resume after human review. @@ -173,7 +171,7 @@ def execute_complete( # Only accept modified output when the operator explicitly allows modifications. # Without this guard a reviewer could craft a request with params_input even # when allow_modifications=False, bypassing the read-only approval flow. - if self.allow_modifications and params_input: + if getattr(self, "allow_modifications", False) and params_input: modified = params_input.get("output") if modified is not None and modified != generated_output: log.info("output=%s modified by the reviewer=%s ", modified, responded_by_user)