JelyFishhhhhh opened a new pull request, #71402:
URL: https://github.com/apache/airflow/pull/71402
## What
`SqlToSlackWebhookOperator.render_template_fields` renders in two passes,
because `slack_message` usually references `results_df`, which does not exist
until the query has run. The first pass renders every templated field *except*
`slack_message`; the second, from inside `_render_and_send_slack_message`, is
meant to fill in that one deferred field.
The second pass rendered `self.template_fields` — all of them, not just
the deferred one:
```python
if self.times_rendered == 0:
fields_to_render: Iterable[str] = (x for x in self.template_fields if x
!= "slack_message")
else:
fields_to_render = self.template_fields
```
So `sql`, already rendered by the first pass, was compiled a second time
with its own rendered output as the template source. This restricts the second
pass to `("slack_message",)`.
## Why it matters
Re-rendering turns a field's output back into template source, so Jinja
syntax that arrived inside a *context value* is evaluated on the second pass
rather than staying literal.
A DAG doing the ordinary thing:
```python
SqlToSlackWebhookOperator(
sql="SELECT * FROM events WHERE tenant = '{{ dag_run.conf.tenant }}'",
slack_message="{{ results_df }}",
...
)
```
A user triggering that Dag supplies `conf = {"tenant": "{{ ... }}"}`. After
pass 1 the operator's `sql` holds that Jinja verbatim; pass 2 then evaluates it
against the task context, which includes the `var` and `conn` accessors.
The practical impact today is limited — the re-rendered `sql` is not sent
to Slack (only `slack_message` is), and the query has already run by then, so
the evaluated result lands in the rendered-template record rather than anywhere
it can act. I am **not** reporting this as a vulnerability, and I checked the
exfiltration paths before opening this rather than assuming them. But "a
field's rendered output is re-compiled as a template" is not a property this
operator should have, and the fix is smaller than reasoning about where the
output ends up.
The first pass is unaffected, and `slack_message` still renders exactly as
before — it is simply the only field the second pass touches now.
## Test
`test_second_render_leaves_already_rendered_fields_alone` renders with a
context where `ds` resolves to a value containing Jinja, and asserts `sql`
still holds it literally after `execute()`.
Against the current code the second pass evaluates it and the test fails:
```
assert equals failed
-"SELECT 'SECRET'" +"SELECT '{{ leaked }}'"
```
With this change the full file passes — 11 tests, including the three
existing render tests, which are unaffected.
## Same shape elsewhere
Other operators defer part of their rendering the same way and may re-render
more than the deferred field. I have not audited them and this PR deliberately
does not touch them; flagging in case a maintainer wants them looked at:
- `@task.bash`
- the `common.ai` and `common.sql` decorators
- `kubernetes_cmd`
- `generic_transfer`
## Notes
- Provider changelogs are generated during release prep, so no newsfragment
is included. Happy to add one if that is wrong for this repo.
- No config, API, or behavioural change for any Dag that does not rely on a
second render mutating an already-rendered field.
<!-- SPDX-License-Identifier: Apache-2.0
https://www.apache.org/licenses/LICENSE-2.0 -->
<!--
Thank you for contributing!
Please provide above a brief description of the changes made in this pull
request.
Write a good git commit message following this guide:
https://chris.beams.io/posts/git-commit/
Please make sure that your code changes are covered with tests.
And in case of new features or big changes remember to adjust the
documentation.
For user-facing UI changes, please attach before/after screenshots (or a
short
screen recording) so reviewers can assess the visual impact.
Feel free to ping (in general) for the review if you do not see reaction for
a few days
(72 Hours is the minimum reaction time you can expect from volunteers) - we
sometimes miss notifications.
In case of an existing issue, reference it using one of the following:
* closes: #ISSUE
* related: #ISSUE
-->
---
##### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this PR,
please
change below checkbox to `[X]` followed by the name of the tool, uncomment
the "Generated-by".
-->
- [x] Yes (please specify the tool below)
Generated-by: Claude Code (Opus 5) following [the
guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
---
* Read the **[Pull Request
Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)**
for more information. Note: commit author/co-author name and email in commits
become permanently public when merged.
* For fundamental code changes, an Airflow Improvement Proposal
([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals))
is needed.
* When adding dependency, check compliance with the [ASF 3rd Party License
Policy](https://www.apache.org/legal/resolved.html#category-x).
* For significant user-facing changes create newsfragment:
`{pr_number}.significant.rst`, in
[airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments).
You can add this file in a follow-up commit after the PR is created so you
know the PR number.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
