Skip to content

Fix DAG named "DAGs" colliding with the global DAGs permission resource - #69106

Merged
vincbeck merged 2 commits into
apache:mainfrom
potiuk:fix-dag-named-dags-resource-collision
Jul 6, 2026
Merged

Fix DAG named "DAGs" colliding with the global DAGs permission resource#69106
vincbeck merged 2 commits into
apache:mainfrom
potiuk:fix-dag-named-dags-resource-collision

Conversation

@potiuk

@potiuk potiuk commented Jun 28, 2026

Copy link
Copy Markdown
Member

What

resource_name() returned the dag_id unchanged when it equalled a reserved resource name. Because a real dag_id can be DAGs (it passes the dag_id validator), a DAG named DAGs resolved to the global DAGs permission resource instead of its own per-DAG DAG:DAGs resource, so that DAG's permissions were applied to the wrong resource.

This drops the reserved-name short-circuit so a dag_id is always prefixed (DAG:<dag_id>). The already-prefixed branch (DAG:foo, DAG Run:foo) still short-circuits — safe because those prefixes contain a colon, which the dag_id validator (KEY_REGEX = ^[\w.-]+$) forbids, so no real dag_id can reach it. The same fix is mirrored in the duplicated airflow-core copy.

Tests

  • resource_name("DAGs", RESOURCE_DAG) now returns DAG:DAGs (FAB + core copies); ordinary and already-prefixed names unchanged
  • full providers/fab test_security.py suite passes; ruff clean
Was generative AI tooling used to co-author this PR?
  • Yes — Claude Opus 4.8

Generated-by: Claude Opus 4.8 following the guidelines at
https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions

@potiuk

potiuk commented Jun 28, 2026

Copy link
Copy Markdown
Member Author

@vincbeck — one judgement call to flag here: I removed the if dag_id in RESOURCE_DETAILS_MAP.keys(): return dag_id short-circuit in resource_name(). I couldn't find a call site that legitimately relies on passing a bare reserved resource name (DAGs / DAG Runs) as the dag_id arg — every caller passes a real dag_id, and the global-resource auth check is done separately — so removing it looks safe. But you've been in this code recently; if there was an intended use for that mapping that I'm missing, say so and I'll rework it. (I kept the already-prefixed DAG: branch, which is clearly load-bearing for idempotency.)

@vincbeck

vincbeck commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

@vincbeck — one judgement call to flag here: I removed the if dag_id in RESOURCE_DETAILS_MAP.keys(): return dag_id short-circuit in resource_name(). I couldn't find a call site that legitimately relies on passing a bare reserved resource name (DAGs / DAG Runs) as the dag_id arg — every caller passes a real dag_id, and the global-resource auth check is done separately — so removing it looks safe. But you've been in this code recently; if there was an intended use for that mapping that I'm missing, say so and I'll rework it. (I kept the already-prefixed DAG: branch, which is clearly load-bearing for idempotency.)

I agree, this is a good call, I have no idea why we have this logic in the code at the first place. This code has been sitting there for a while with no real explanation :)

Comment thread airflow-core/src/airflow/security/permissions.py Outdated
Comment thread airflow-core/tests/unit/security/test_permissions.py
@potiuk
potiuk marked this pull request as ready for review July 1, 2026 16:31
@potiuk
potiuk requested a review from vincbeck July 1, 2026 16:36
@potiuk

potiuk commented Jul 1, 2026

Copy link
Copy Markdown
Member Author

@vincbeck - addressed concerns :)

potiuk added 2 commits July 3, 2026 13:50
resource_name() returned the dag_id unchanged when it equalled a reserved
resource name. Because a real dag_id can be "DAGs" (it passes the dag_id
validator), a DAG named "DAGs" resolved to the global DAGs resource instead
of its own per-DAG "DAG:DAGs" resource, so permissions for that DAG were
applied to the wrong resource.

Drop the reserved-name short-circuit so a dag_id is always prefixed. The
already-prefixed branch (e.g. "DAG:foo") still short-circuits, which is safe
because the "DAG:" / "DAG Run:" prefixes contain a colon that the dag_id
validator (KEY_REGEX = ^[\w.-]+$) forbids. Mirrored in the airflow-core copy.

Generated-by: Claude Opus 4.8 following the guidelines at
https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions
Address @vincbeck's review: remove the explanatory NB comment in resource_name()
(the removed short-circuit is self-evident), and trim the regression test down to
the single collision assertion in both the core and FAB copies.

Generated-by: Claude Code (Opus 4.8)
@potiuk
potiuk force-pushed the fix-dag-named-dags-resource-collision branch from 5bd2d50 to 646207b Compare July 3, 2026 11:50
@vincbeck
vincbeck merged commit 447c20f into apache:main Jul 6, 2026
83 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants