You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Lee-W (via GitHub)" <gi...@apache.org> on 2026/10/08 05:31:42 UTC
[PR] Document that SQLToolset does not check functions without allowed_tables [airflow]
Lee-W opened a new pull request, #74444:
URL: https://github.com/apache/airflow/pull/74444
## Why
`SQLToolset` rejects write statements by scanning the parsed statement, but without `allowed_tables` it does not examine function calls inside a `SELECT`. The docs missed it. A `SELECT` can therefore still call side-effecting functions such as `nextval`, `pg_terminate_backend` or `dblink_exec`. These pass validation and run with the connection's privileges.
## What
- `providers/common/ai/docs/toolsets/sql.rst`: add a paragraph that says the check inspects statement types, not what a function does. It also says that without `allowed_tables` a function call inside a `SELECT` is not examined. Setting `allowed_tables` turns on the function check: every function sqlglot cannot type is rejected unless it is listed in `allowed_functions`. Either way, it recommends a least-privilege role, and a read-only transaction or session where the database supports one.
- `providers/common/ai/docs/agent_security.rst`: in the SQLToolset read-only row of the layers table, add the same limitation and the same recommendation.
<!-- 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] 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Document that SQLToolset does not check functions without allowed_tables [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil merged PR #74444:
URL: https://github.com/apache/airflow/pull/74444
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Document that SQLToolset does not check functions without allowed_tables [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on code in PR #74444:
URL: https://github.com/apache/airflow/pull/74444#discussion_r4218337197
##########
providers/common/ai/docs/agent_security.rst:
##########
@@ -112,7 +112,11 @@ No single layer is sufficient on its own. They work together.
(``DESCRIBE``/``SHOW``) statements pass; INSERT, UPDATE, DELETE, DROP,
and writes hidden behind ``EXPLAIN`` are rejected.
- Does not prevent the agent from reading sensitive data that the
- database user has SELECT access to.
+ database user has SELECT access to. Without ``allowed_tables`` the check is
Review Comment:
This reads as if setting `allowed_tables` closes the gap, but the function check only rejects calls sqlglot parses as `Anonymous`. Typed sequence syntax still passes with `allowed_tables` set: T-SQL `SELECT NEXT VALUE FOR seq` parses as `NextValueFor`, and Snowflake `SELECT seq.nextval` parses as a column. Maybe reuse the `sql.rst` wording here ("rejects only functions sqlglot cannot type")?
##########
providers/common/ai/docs/agent_security.rst:
##########
@@ -112,7 +112,11 @@ No single layer is sufficient on its own. They work together.
(``DESCRIBE``/``SHOW``) statements pass; INSERT, UPDATE, DELETE, DROP,
and writes hidden behind ``EXPLAIN`` are rejected.
- Does not prevent the agent from reading sensitive data that the
- database user has SELECT access to.
+ database user has SELECT access to. Without ``allowed_tables`` the check is
+ statement-level only: a side-effecting function inside a SELECT
+ (``nextval``, ``pg_terminate_backend``, ``dblink_exec``) is not blocked. Use a
+ least-privilege role, or a read-only transaction where the database supports
Review Comment:
I think this should be "and" rather than "or", which is also what `sql.rst` says. A read-only session does stop `nextval`, but on Postgres 17 `pg_terminate_backend` still succeeds with `default_transaction_read_only = on`, and `dblink_exec` inside `BEGIN READ ONLY` still ran a `DELETE`, because dblink opens its own remote session. So a reader who picks the read-only option is still open to two of the three functions named in the sentence above.
Also, "transaction" isn't something the user can set up here. `SQLToolset._query` goes through `hook.run()`, which opens a fresh connection and commits on every call, so only a role or session default reaches the agent's queries, e.g. `ALTER ROLE <role> SET default_transaction_read_only = on`.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Document that SQLToolset does not check functions without allowed_tables [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #74444:
URL: https://github.com/apache/airflow/pull/74444#discussion_r4227625048
##########
providers/common/ai/docs/agent_security.rst:
##########
@@ -112,7 +112,11 @@ No single layer is sufficient on its own. They work together.
(``DESCRIBE``/``SHOW``) statements pass; INSERT, UPDATE, DELETE, DROP,
and writes hidden behind ``EXPLAIN`` are rejected.
- Does not prevent the agent from reading sensitive data that the
- database user has SELECT access to.
+ database user has SELECT access to. Without ``allowed_tables`` the check is
+ statement-level only: a side-effecting function inside a SELECT
+ (``nextval``, ``pg_terminate_backend``, ``dblink_exec``) is not blocked. Use a
+ least-privilege role, or a read-only transaction where the database supports
Review Comment:
* The recommendation now pairs a least-privilege role with a read-only default set on the role or session (`ALTER ROLE <role> SET default_transaction_read_only = on` on PostgreSQL), and says the toolset opens its own connection per call so a transaction can't be set from the Dag.
* Dropped the "read-only transaction or session" wording from `toolsets/sql.rst`
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Document that SQLToolset does not check functions without allowed_tables [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #74444:
URL: https://github.com/apache/airflow/pull/74444#discussion_r4227620170
##########
providers/common/ai/docs/agent_security.rst:
##########
@@ -112,7 +112,11 @@ No single layer is sufficient on its own. They work together.
(``DESCRIBE``/``SHOW``) statements pass; INSERT, UPDATE, DELETE, DROP,
and writes hidden behind ``EXPLAIN`` are rejected.
- Does not prevent the agent from reading sensitive data that the
- database user has SELECT access to.
+ database user has SELECT access to. Without ``allowed_tables`` the check is
Review Comment:
Both pages now say the `allowed_tables` function check rejects only functions sqlglot cannot type, name those two examples, and state that `allowed_tables` is not a guarantee against side-effecting functions.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org