From bbb97f55e647dd09f0725c111b7a7081b9df7850 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Sat, 1 Aug 2026 04:17:30 +0200 Subject: [PATCH 1/3] Resolve backfill_id in the access dependency with the type the routes declare The backfill routes declare `backfill_id: NonNegativeInt`, but `requires_access_backfill` parsed the raw path value with `int()` and swallowed the failure. The two parsers do not agree: pydantic's lax mode validates "1.0" and "1.00" to 1, while `int()` rejects both. Dependencies resolve before the endpoint's own parameter validation, so for those spellings the dependency left the Dag unresolved on a request the handler then served against backfill 1 -- the two disagreed about which Dag the request concerned. Parse with the same TypeAdapter the routes declare so they cannot diverge. --- .../airflow/api_fastapi/core_api/security.py | 23 ++++++++- .../api_fastapi/core_api/test_security.py | 49 ++++++++++++++++++- 2 files changed, 68 insertions(+), 4 deletions(-) diff --git a/airflow-core/src/airflow/api_fastapi/core_api/security.py b/airflow-core/src/airflow/api_fastapi/core_api/security.py index 96478eb5a1483..fbbe1e7de8700 100644 --- a/airflow-core/src/airflow/api_fastapi/core_api/security.py +++ b/airflow-core/src/airflow/api_fastapi/core_api/security.py @@ -27,6 +27,7 @@ from fastapi.security import HTTPAuthorizationCredentials, HTTPBearer, OAuth2PasswordBearer from itsdangerous import BadSignature, URLSafeSerializer from jwt import ExpiredSignatureError, InvalidTokenError +from pydantic import NonNegativeInt, TypeAdapter, ValidationError from sqlalchemy import or_, select from sqlalchemy.orm import Session @@ -390,6 +391,12 @@ def depends_readable_event_logs_filter( ] +# The type the backfill routes declare for the `backfill_id` path parameter. Shared with +# `requires_access_backfill` so the authorization decision parses the id exactly as the handler +# does; see the comment there for why any divergence is a cross-Dag authorization bypass. +_BACKFILL_ID_ADAPTER: TypeAdapter[NonNegativeInt] = TypeAdapter(NonNegativeInt) + + def requires_access_backfill( method: ResourceMethod, ) -> Callable[[Request, BaseUser, Session], Coroutine[Any, Any, None]]: @@ -405,8 +412,20 @@ async def inner( # Try to retrieve the dag_id from the backfill_id path param backfill_id_raw = request.path_params.get("backfill_id") try: - backfill_id = int(backfill_id_raw) if backfill_id_raw is not None else None - except ValueError: + # Parse with the *same* type the endpoints declare for this path parameter, so this + # dependency and the handler always resolve the same backfill -- and therefore the + # same Dag. + # + # `int()` is not that parser: pydantic's lax mode accepts strings `int()` rejects, + # so "1.0" and "1.00" validate to 1 for the handler while `int()` raised here. Since + # dependencies resolve before the endpoint's own parameter validation, that left + # `dag_id` unresolved for a request the handler went on to serve against backfill 1. + backfill_id = ( + _BACKFILL_ID_ADAPTER.validate_python(backfill_id_raw) if backfill_id_raw is not None else None + ) + except ValidationError: + # Rejected by the endpoint's parser too, so the handler cannot run: FastAPI answers + # 422 before it is reached. Left as None, preserving that response. backfill_id = None if backfill_id is not None: diff --git a/airflow-core/tests/unit/api_fastapi/core_api/test_security.py b/airflow-core/tests/unit/api_fastapi/core_api/test_security.py index 37750e30da959..2a43460f4f459 100644 --- a/airflow-core/tests/unit/api_fastapi/core_api/test_security.py +++ b/airflow-core/tests/unit/api_fastapi/core_api/test_security.py @@ -367,7 +367,7 @@ async def test_requires_access_backfill_authorized_from_path( async def test_requires_access_backfill_authorized_from_body( self, mock_get_auth_manager, mock_get_team_name ): - """When backfill_id is missing or not int, dag_id can come from request body (POST backfill).""" + """With no backfill_id in the path, dag_id comes from the request body (POST backfill).""" auth_manager = Mock() auth_manager.is_authorized_dag.return_value = True mock_get_auth_manager.return_value = auth_manager @@ -430,7 +430,10 @@ async def test_requires_access_backfill_unauthorized(self, mock_get_auth_manager async def test_requires_access_backfill_backfill_not_found_falls_back_to_body( self, mock_get_auth_manager, mock_get_team_name ): - """When backfill_id is int but Backfill not found, dag_id from body is used.""" + """When backfill_id is int but Backfill not found, dag_id from body is used. + + Not exploitable: the handler answers 404 before acting, so no cross-Dag action follows. + """ auth_manager = Mock() auth_manager.is_authorized_dag.return_value = True mock_get_auth_manager.return_value = auth_manager @@ -455,6 +458,48 @@ async def test_requires_access_backfill_backfill_not_found_falls_back_to_body( user=user, ) + @pytest.mark.db_test + @pytest.mark.asyncio + @pytest.mark.parametrize("backfill_id", ["42", "42.0", "42.00"]) + @patch.object(DagModel, "get_team_name") + @patch("airflow.api_fastapi.core_api.security.get_auth_manager") + async def test_requires_access_backfill_authorizes_the_backfill_the_handler_will_act_on( + self, mock_get_auth_manager, mock_get_team_name, backfill_id + ): + """The dependency must resolve the same backfill the handler does, for every spelling. + + The endpoints declare ``backfill_id: NonNegativeInt``, and pydantic's lax mode coerces + ``"42.0"`` and ``"42.00"`` to ``42`` -- both are spellings the handler accepts and serves + against backfill 42. Parsing with ``int()`` here rejected them and left ``dag_id`` + unresolved, so the two disagreed about which Dag the request concerned. + """ + auth_manager = Mock() + auth_manager.is_authorized_dag.return_value = True + mock_get_auth_manager.return_value = auth_manager + mock_get_team_name.return_value = "team1" + + backfill = Mock() + backfill.dag_id = "backfill_dag" + session = Mock() + session.scalars.return_value.one_or_none.return_value = backfill + + request = Mock() + request.path_params = {"backfill_id": backfill_id} + request.query_params = {"dag_id": "some_other_dag"} + request.json = AsyncMock(return_value={"dag_id": "some_other_dag"}) + + user = Mock() + + await requires_access_backfill("PUT")(request, user, session) + + # the backfill's own Dag, not the one supplied on the request + auth_manager.is_authorized_dag.assert_called_once_with( + method="PUT", + access_entity=DagAccessEntity.RUN, + details=DagDetails(id="backfill_dag", team_name="team1"), + user=user, + ) + @pytest.mark.db_test @pytest.mark.asyncio @patch.object(DagModel, "get_team_name") From 8f271a5ef7d9bcbb0b8fdd6074d2133ae3b80e84 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Mon, 3 Aug 2026 21:53:13 +0200 Subject: [PATCH 2/3] Use spec'd mocks in the backfill authorization dependency test An unspecced Mock accepts any attribute, so the test would keep passing if the dependency started reading something the real Request, Session or Backfill does not have. --- .../unit/api_fastapi/core_api/test_security.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/airflow-core/tests/unit/api_fastapi/core_api/test_security.py b/airflow-core/tests/unit/api_fastapi/core_api/test_security.py index 2a43460f4f459..304344b686282 100644 --- a/airflow-core/tests/unit/api_fastapi/core_api/test_security.py +++ b/airflow-core/tests/unit/api_fastapi/core_api/test_security.py @@ -20,12 +20,14 @@ from unittest.mock import AsyncMock, Mock, patch import pytest -from fastapi import HTTPException +from fastapi import HTTPException, Request from jwt import ExpiredSignatureError, InvalidTokenError +from sqlalchemy.orm import Session from airflow import settings from airflow.api_fastapi.app import create_app -from airflow.api_fastapi.auth.managers.base_auth_manager import COOKIE_NAME_JWT_TOKEN +from airflow.api_fastapi.auth.managers.base_auth_manager import COOKIE_NAME_JWT_TOKEN, BaseAuthManager +from airflow.api_fastapi.auth.managers.models.base_user import BaseUser from airflow.api_fastapi.auth.managers.models.resource_details import ( AccessView, ConnectionDetails, @@ -55,6 +57,7 @@ resolve_user_from_token, ) from airflow.models import Connection, Pool, Variable +from airflow.models.backfill import Backfill from airflow.models.dag import DagModel from airflow.models.dagbundle import DagBundleModel from airflow.models.team import Team @@ -473,22 +476,22 @@ async def test_requires_access_backfill_authorizes_the_backfill_the_handler_will against backfill 42. Parsing with ``int()`` here rejected them and left ``dag_id`` unresolved, so the two disagreed about which Dag the request concerned. """ - auth_manager = Mock() + auth_manager = Mock(spec=BaseAuthManager) auth_manager.is_authorized_dag.return_value = True mock_get_auth_manager.return_value = auth_manager mock_get_team_name.return_value = "team1" - backfill = Mock() + backfill = Mock(spec=Backfill) backfill.dag_id = "backfill_dag" - session = Mock() + session = Mock(spec=Session) session.scalars.return_value.one_or_none.return_value = backfill - request = Mock() + request = Mock(spec=Request) request.path_params = {"backfill_id": backfill_id} request.query_params = {"dag_id": "some_other_dag"} request.json = AsyncMock(return_value={"dag_id": "some_other_dag"}) - user = Mock() + user = Mock(spec=BaseUser) await requires_access_backfill("PUT")(request, user, session) From f4f7fc317bc404f7c8791b1a6841b7a8fbcf5bad Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Tue, 4 Aug 2026 12:25:07 +0200 Subject: [PATCH 3/3] Point at the tracking issue for the unknown-backfill fallback A backfill_id that parses but matches no row falls through to the body's dag_id, so an unknown backfill answers 404 where an unauthorized one answers 403 and a caller can tell which ids exist. That is a separate fix from the parser divergence this change closes, and it has to keep the three body-authorized routes working, so it is tracked rather than folded in here. The comment above the adapter also loses the history that led to it; what matters going forward is the rule it states. --- .../src/airflow/api_fastapi/core_api/security.py | 14 ++++++-------- .../unit/api_fastapi/core_api/test_security.py | 5 +---- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/airflow-core/src/airflow/api_fastapi/core_api/security.py b/airflow-core/src/airflow/api_fastapi/core_api/security.py index fbbe1e7de8700..e223dd207360c 100644 --- a/airflow-core/src/airflow/api_fastapi/core_api/security.py +++ b/airflow-core/src/airflow/api_fastapi/core_api/security.py @@ -412,14 +412,8 @@ async def inner( # Try to retrieve the dag_id from the backfill_id path param backfill_id_raw = request.path_params.get("backfill_id") try: - # Parse with the *same* type the endpoints declare for this path parameter, so this - # dependency and the handler always resolve the same backfill -- and therefore the - # same Dag. - # - # `int()` is not that parser: pydantic's lax mode accepts strings `int()` rejects, - # so "1.0" and "1.00" validate to 1 for the handler while `int()` raised here. Since - # dependencies resolve before the endpoint's own parameter validation, that left - # `dag_id` unresolved for a request the handler went on to serve against backfill 1. + # Must parse exactly as the handler does (e.g. pydantic's lax mode coerces "1.0" to 1 + # where int() raises), or the two can authorize and act on different backfills. backfill_id = ( _BACKFILL_ID_ADAPTER.validate_python(backfill_id_raw) if backfill_id_raw is not None else None ) @@ -433,6 +427,10 @@ async def inner( dag_id = backfill.dag_id if backfill else None # Try to retrieve the dag_id from the request body (POST backfill) + # TODO: a backfill_id that parses but matches no row also lands here, so an unknown + # backfill is authorized against the body's dag_id and answers 404 where an unauthorized + # one answers 403 - disclosing which ids exist. Not exploitable for a cross-Dag action; + # tracked at https://github.com/apache/airflow/issues/71080 if dag_id is None: # Not a json body, ignore with suppress(JSONDecodeError): diff --git a/airflow-core/tests/unit/api_fastapi/core_api/test_security.py b/airflow-core/tests/unit/api_fastapi/core_api/test_security.py index 304344b686282..3709d50d3580b 100644 --- a/airflow-core/tests/unit/api_fastapi/core_api/test_security.py +++ b/airflow-core/tests/unit/api_fastapi/core_api/test_security.py @@ -433,10 +433,7 @@ async def test_requires_access_backfill_unauthorized(self, mock_get_auth_manager async def test_requires_access_backfill_backfill_not_found_falls_back_to_body( self, mock_get_auth_manager, mock_get_team_name ): - """When backfill_id is int but Backfill not found, dag_id from body is used. - - Not exploitable: the handler answers 404 before acting, so no cross-Dag action follows. - """ + """When backfill_id is int but Backfill not found, dag_id from body is used.""" auth_manager = Mock() auth_manager.is_authorized_dag.return_value = True mock_get_auth_manager.return_value = auth_manager