Skip to content

Restrict the second render pass in SqlToSlackWebhookOperator - #71402

Merged
potiuk merged 3 commits into
apache:mainfrom
JelyFishhhhhh:fix-slack-double-render
Oct 5, 2026
Merged

potiuk merged 3 commits into
apache:mainfrom
JelyFishhhhhh:fix-slack-double-render

Conversation

@JelyFishhhhhh

Copy link
Copy Markdown
Contributor

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:

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:

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.

Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

Generated-by: Claude Code (Opus 5) following the guidelines


  • Read the 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) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

@boring-cyborg

boring-cyborg Bot commented Aug 11, 2026

Copy link
Copy Markdown

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our prek-hooks will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example Dag that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@JelyFishhhhhh
JelyFishhhhhh force-pushed the fix-slack-double-render branch 2 times, most recently from 2e69115 to dd2061f Compare August 14, 2026 05:43
JelyF1shhhhhh and others added 2 commits October 5, 2026 17:14
The operator renders in two passes because slack_message references
results_df, which does not exist until the query has run. The first pass
renders every templated field except slack_message; the second is meant to
fill in that one deferred field.

The second pass rendered self.template_fields instead, so sql - already
rendered by the first pass - was compiled again with its own rendered output
as the template source. Jinja syntax arriving inside a context value was
therefore evaluated on the second pass rather than staying literal.

Restrict the second pass to slack_message. The first pass is unchanged and
slack_message still renders exactly as before.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A mapped task's first render bypasses render_template_fields and leaves
times_rendered at 0, so sending re-rendered the already-rendered sql.
Render the deferred slack_message field directly instead of relying on
the counter.

Generated-by: Claude Opus 5
@potiuk
potiuk force-pushed the fix-slack-double-render branch from 44a67ad to 3939cb3 Compare October 5, 2026 15:14
A mapped task's first render also renders slack_message, so rendering
the attribute again at send time evaluated Jinja that arrived in a
context value. Keep the template given to the constructor and render
that, once, when sending.

Generated-by: Claude Opus 5

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I approved after pushing two fixup commits for the mapped-task path (.partial().expand()):

  • A mapped task's first render goes through MappedOperator, which renders every template field and never increments times_rendered. So execute() still took the "first render" branch and rendered the already-rendered sql again, with the same double evaluation this PR removes.
  • Sending now renders only slack_message, once, from the template given to the constructor, so no field is rendered twice on either path. .j2 templates keep working, because the kept value is the file name, which render_template loads as usual.
  • Added test_send_renders_each_field_once_after_mapped_first_render, which reproduces the mapped first render with injected Jinja in both sql and slack_message.

Drafted-by: Claude Code (Opus 5.5); reviewed by @potiuk before posting

@potiuk
potiuk merged commit 11bf874 into apache:main Oct 5, 2026
80 checks passed
@boring-cyborg

boring-cyborg Bot commented Oct 5, 2026

Copy link
Copy Markdown

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants