This is an automated email from the ASF dual-hosted git repository.
vatsrahul1001 pushed a commit to branch v3-3-test
in repository https://gitbox.apache.org/repos/asf/airflow.git
The following commit(s) were added to refs/heads/v3-3-test by this push:
new ac30d808c66 [v3-3-test] Resolve backfill_id in the access dependency
with the type the routes declare (#70889) (#71090)
ac30d808c66 is described below
commit ac30d808c66556a0149554c03e7714be95c13ce7
Author: github-actions[bot]
<41898282+github-actions[bot]@users.noreply.github.com>
AuthorDate: Tue Aug 4 19:40:47 2026 +0530
[v3-3-test] Resolve backfill_id in the access dependency with the type the
routes declare (#70889) (#71090)
* 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.
* 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.
* 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.
(cherry picked from commit a6265b77cf57be4d59fd736cdc2554f94db9e2a2)
Co-authored-by: Jarek Potiuk <[email protected]>
Co-authored-by: Rahul Vats <[email protected]>
---
.../src/airflow/api_fastapi/core_api/security.py | 21 ++++++++-
.../unit/api_fastapi/core_api/test_security.py | 51 ++++++++++++++++++++--
2 files changed, 67 insertions(+), 5 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 720423797b5..f5c86a5d6a9 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 import Depends, HTTPException, Request, status
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
@@ -357,6 +358,12 @@ ReadableBackfillsFilterDep = Annotated[
]
+# 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]]:
@@ -372,8 +379,14 @@ def requires_access_backfill(
# 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:
+ # 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
+ )
+ 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:
@@ -381,6 +394,10 @@ def requires_access_backfill(
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 8824969f976..c6afe642e99 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 json import JSONDecodeError
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 (
ConnectionDetails,
DagAccessEntity,
@@ -54,6 +56,7 @@ from airflow.api_fastapi.core_api.security import (
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
@@ -366,7 +369,7 @@ class TestFastApiSecurity:
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
@@ -454,6 +457,48 @@ class TestFastApiSecurity:
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(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(spec=Backfill)
+ backfill.dag_id = "backfill_dag"
+ session = Mock(spec=Session)
+ session.scalars.return_value.one_or_none.return_value = backfill
+
+ 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(spec=BaseUser)
+
+ 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")
