Repository navigation
Stop backfill endpoints disclosing ids across Dags you cannot see - #71113
Conversation
The routes that name a backfill in their path resolved the authorization subject from the request when the id matched no row, so a caller could tell "no such backfill" apart from "a backfill you may not see" and enumerate ids belonging to Dags they have no access to. The path names the backfill, so nothing the caller supplies alongside it can be the subject, and an id they may not see has to answer exactly as a missing one does. A caller who may read the Dag keeps the Forbidden answer: they can already list that Dag's backfills, so hiding it would only cost them the reason their request was refused. closes: apache#71080
potiuk
added
the
ready for maintainer review
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
This is a clean fix and it meets all three acceptance criteria from #71080 — the path id is the only authorization subject on those five routes, unknown and unreadable are indistinguishable, and list/create/dry-run keep authorizing off the request. Nice touches: all five routes already declared 404 in create_openapi_http_exception_doc, so nothing needs regenerating, and the lookup narrowed from a full Backfill row to select(Backfill.dag_id).
Approving. One thing worth a follow-up commit here, and two nits you can take or leave.
The 404 detail string changed on four routes too — worth a newsfragment line
See the inline comment on security.py. The newsfragment covers 403 → 404 for unreadable Dags, but for an authorized caller the message for an unknown id also changed, because the dependency now answers before the handler runs:
| Route | Before | After |
|---|---|---|
GET /backfills/{id} |
Backfill not found |
unchanged |
GET /backfills/{id}/dag_runs |
Backfill with id 999 not found |
Backfill not found |
PUT .../pause, .../unpause, .../cancel |
Could not find backfill with id 999 |
Backfill not found |
Nothing caught it because test_list_backfill_dag_runs_not_found asserts only the status code and the three PUT routes have no unknown-id detail assertion.
Smaller observations
- See the inline comment on
security.pyabout the secondis_authorized_dagcall. - See the inline comment on the new
dag_reader_test_clientfixture.
This review was drafted by an AI-assisted tool and
confirmed by an Airflow maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.More on how Airflow handles maintainer review:
contributing-docs/05_pull_requests.rst.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
For an id named in the path the access dependency now answers before the handler does, so the three different messages the handlers used to describe that single condition are no longer what a caller sees. A client matching on the detail string would otherwise have to know which route it hit, and which of the two places answered.
Thanks — the detail string was the part I missed, and it is a real break for anyone matching on it. Follow-up pushed: all five routes now raise On the second On the fixture: agreed, and I have kept it local here. A |
Backport failed to create: v3-3-test. View the failure log Run detailsNote: As of Merging PRs targeted for Airflow 3.X In matter of doubt please ask in #release-management Slack channel.
You can attempt to backport this manually by running: cherry_picker 02dbd2f v3-3-testThis should apply the commit to the v3-3-test branch and leave the commit in conflict state marking After you have resolved the conflicts, you can continue the backport process by running: cherry_picker --continueIf you don't have cherry-picker installed, see the installation guide. |
…1113) (#72192) * Stop backfill endpoints disclosing which backfill ids exist across Dags The routes that name a backfill in their path resolved the authorization subject from the request when the id matched no row, so a caller could tell "no such backfill" apart from "a backfill you may not see" and enumerate ids belonging to Dags they have no access to. The path names the backfill, so nothing the caller supplies alongside it can be the subject, and an id they may not see has to answer exactly as a missing one does. A caller who may read the Dag keeps the Forbidden answer: they can already list that Dag's backfills, so hiding it would only cost them the reason their request was refused. closes: #71080 * Add newsfragment for the backfill authorization change * Give every backfill route one answer for an id that is not there For an id named in the path the access dependency now answers before the handler does, so the three different messages the handlers used to describe that single condition are no longer what a caller sees. A client matching on the detail string would otherwise have to know which route it hit, and which of the two places answered. (cherry picked from commit 02dbd2f) Co-authored-by: rjgoyln <151457491+rjgoyln@users.noreply.github.com>
…1113) (#72192) * Stop backfill endpoints disclosing which backfill ids exist across Dags The routes that name a backfill in their path resolved the authorization subject from the request when the id matched no row, so a caller could tell "no such backfill" apart from "a backfill you may not see" and enumerate ids belonging to Dags they have no access to. The path names the backfill, so nothing the caller supplies alongside it can be the subject, and an id they may not see has to answer exactly as a missing one does. A caller who may read the Dag keeps the Forbidden answer: they can already list that Dag's backfills, so hiding it would only cost them the reason their request was refused. closes: #71080 * Add newsfragment for the backfill authorization change * Give every backfill route one answer for an id that is not there For an id named in the path the access dependency now answers before the handler does, so the three different messages the handlers used to describe that single condition are no longer what a caller sees. A client matching on the detail string would otherwise have to know which route it hit, and which of the two places answered. (cherry picked from commit 02dbd2f) Co-authored-by: rjgoyln <151457491+rjgoyln@users.noreply.github.com>
…ache#71113) * Stop backfill endpoints disclosing which backfill ids exist across Dags The routes that name a backfill in their path resolved the authorization subject from the request when the id matched no row, so a caller could tell "no such backfill" apart from "a backfill you may not see" and enumerate ids belonging to Dags they have no access to. The path names the backfill, so nothing the caller supplies alongside it can be the subject, and an id they may not see has to answer exactly as a missing one does. A caller who may read the Dag keeps the Forbidden answer: they can already list that Dag's backfills, so hiding it would only cost them the reason their request was refused. closes: apache#71080 * Add newsfragment for the backfill authorization change * Give every backfill route one answer for an id that is not there For an id named in the path the access dependency now answers before the handler does, so the three different messages the handlers used to describe that single condition are no longer what a caller sees. A client matching on the detail string would otherwise have to know which route it hit, and which of the two places answered.
Summary
The five backfill routes that name a backfill in their path fell back to the
dag_idsupplied on the request — body or query string — whenever the id matched no row, so an unknown id and a backfill on a Dag the caller cannot see answered differently, 404 against 403. That difference enumerates backfill ids across Dags.The path names the backfill, so its row is now the only thing those routes authorize against, and an id the caller may not see answers exactly as a missing one does. A caller who may read the Dag keeps the Forbidden answer on a write they are not allowed: they can already list that Dag's backfills, so hiding it would only cost them the reason their request was refused. The list, create and dry-run routes name no backfill in their path and keep authorizing off the body.
Behavior change
On those five routes a Dag the caller cannot read now answers 404 rather than 403.
closes: #71080
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 5) following the guidelines