ephraimbuddy commented on code in PR #70889:
URL: https://github.com/apache/airflow/pull/70889#discussion_r3710894424
##########
airflow-core/src/airflow/api_fastapi/core_api/security.py:
##########
@@ -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.
Review Comment:
```suggestion
# 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.
```
Not blocking but i feel like it should be shorter
##########
airflow-core/tests/unit/api_fastapi/core_api/test_security.py:
##########
@@ -430,7 +433,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.
+ """
Review Comment:
```suggestion
"""When backfill_id is int but Backfill not found, dag_id from body
is used."""
```
Should we add a Todo for the 404 instead of the comment even though it's not
exploitable, i feel like we should fix it next since it leaks info
--
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]
