kaxil commented on code in PR #72640:
URL: https://github.com/apache/airflow/pull/72640#discussion_r3951113534
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/partitioned_dag_runs.py:
##########
@@ -283,7 +283,14 @@ def get_partitioned_dag_runs(
if not (rows := session.execute(query).all()):
if dag_id.value is not None and total_entries == 0:
- dag_exists =
session.scalar(select(DagModel.dag_id).where(DagModel.dag_id == dag_id.value))
+ # Scope the existence probe to Dags the caller may read: without
this, an
+ # unreadable-but-existing Dag returns 200-empty while a
nonexistent Dag
+ # returns 404, letting the caller distinguish the two and learn
which Dags
+ # exist outside their permitted set.
+ dag_exists_query = select(DagModel.dag_id).where(DagModel.dag_id
== dag_id.value)
Review Comment:
3.3.0 and 3.3.1 ship this same unscoped probe (the readable-dags row filter
landed in #65344, which is in 3.3.0), so the leak is in released versions, not
just main. Does this want a v3-3 backport?
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/partitioned_dag_runs.py:
##########
@@ -283,7 +283,14 @@ def get_partitioned_dag_runs(
if not (rows := session.execute(query).all()):
if dag_id.value is not None and total_entries == 0:
- dag_exists =
session.scalar(select(DagModel.dag_id).where(DagModel.dag_id == dag_id.value))
+ # Scope the existence probe to Dags the caller may read: without
this, an
+ # unreadable-but-existing Dag returns 200-empty while a
nonexistent Dag
+ # returns 404, letting the caller distinguish the two and learn
which Dags
+ # exist outside their permitted set.
+ dag_exists_query = select(DagModel.dag_id).where(DagModel.dag_id
== dag_id.value)
+ if readable_dag_ids is not None:
+ dag_exists_query =
dag_exists_query.where(DagModel.dag_id.in_(readable_dag_ids))
Review Comment:
`readable_dag_ids` already comes out of `DagModel` in both in-tree auth
managers (`BaseAuthManager.get_authorized_dag_ids` selects `DagModel.dag_id`
joined to `DagBundleModel`, and FAB's override selects from `DagModel` in both
branches), so this is equivalent to a plain `dag_id.value in readable_dag_ids`
and would skip the query plus a potentially large IN list. Is the `DagModel`
round-trip deliberate cover for a third-party auth manager that returns ids not
in `DagModel`?
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/ui/test_partitioned_dag_runs.py:
##########
@@ -301,6 +301,29 @@ def
test_partitioned_dag_runs_filters_unreadable_dags(self, _, test_client, dag_
dag_ids = {r["dag_id"] for r in body["partitioned_dag_runs"]}
assert "restricted_dag" not in dag_ids
+ @mock.patch(
+
"airflow.api_fastapi.auth.managers.base_auth_manager.BaseAuthManager.get_authorized_dag_ids",
+ return_value={"other_dag"},
+ )
+ def test_dag_id_filter_does_not_disclose_unreadable_dag_existence(
+ self, _, test_client, dag_maker, session
+ ):
+ """
+ An unreadable-but-existing Dag must not be distinguishable from a
nonexistent
+ Dag via the ``dag_id`` filter: both return 404. Without the scoped
existence
+ probe, the existing Dag returned 200-empty while the nonexistent one
returned
+ 404, giving the caller an oracle for Dag ids outside their permitted
set.
+ """
+ schedule = PartitionedAssetTimetable(assets=Asset(uri="s3://bucket/a",
name="a"))
+ with dag_maker(dag_id="restricted_dag", schedule=schedule,
serialized=True):
+ EmptyOperator(task_id="t")
+ dag_maker.sync_dagbag_to_db()
+ session.commit()
Review Comment:
Worth adding an `AssetPartitionDagRun` for `restricted_dag` here, the way
`test_partitioned_dag_runs_filters_unreadable_dags` above does. With no rows at
all the 404 comes from the existence probe alone, so this passes even if the
readable filter on the main query were removed, and that filter dropping the
rows is the case the docstring describes.
--
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]
