From cfa6a6511634a76cdba716ef6b3f7d0fa593fa55 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Sat, 1 Aug 2026 03:16:26 +0200 Subject: [PATCH 1/2] Refuse only ids whose leading segment could name a team Refusing every id of the form `____` was too broad. A team agnostic secret whose id merely looks namespaced -- `_a___b`, `_ab___x` -- was refused even though `a` and `ab` are too short to be team names, so no team namespace can be spelled that way and the lookup was safe. That blocked a legitimate lookup for no benefit. Test the leading segment against the team name rule instead. Every id that does spell a real team's namespace still has a valid team name in that position by construction, so nothing reachable is let through, while ids that cannot name a team resolve normally again. Every split is considered, since a team name may itself contain the separator, and one plausible team name is enough to refuse. This makes the guard depend on stored team names actually being valid, so `teams sync` now enforces the same rule as `teams create`. It creates teams from the dag bundle config and did not validate the names at all, which would have left the guard's assumption unbacked -- an unvalidated short name such as "a" would make the test miss and reopen the cross-team read. Generated-by: Claude Opus 5 (1M context) following the guidelines at https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions --- .../src/airflow/cli/commands/team_command.py | 19 ++++++- .../airflow/secrets/environment_variables.py | 54 +++++++++++++------ .../test_secrets_environment_variables.py | 16 ++++++ 3 files changed, 70 insertions(+), 19 deletions(-) diff --git a/airflow-core/src/airflow/cli/commands/team_command.py b/airflow-core/src/airflow/cli/commands/team_command.py index 931b5507be325..5486fa09297c5 100644 --- a/airflow-core/src/airflow/cli/commands/team_command.py +++ b/airflow-core/src/airflow/cli/commands/team_command.py @@ -48,13 +48,16 @@ def _show_teams(teams, output): ) +TEAM_NAME_PATTERN = r"^[a-zA-Z0-9_-]{3,50}$" + + def _extract_team_name(args): """Extract and validate team name from args.""" team_name = args.name.strip() if not team_name: raise SystemExit("Team name cannot be empty") - if not re.match(r"^[a-zA-Z0-9_-]{3,50}$", team_name): - raise SystemExit("Invalid team name: must match regex ^[a-zA-Z0-9_-]{3,50}$") + if not re.match(TEAM_NAME_PATTERN, team_name): + raise SystemExit(f"Invalid team name: must match regex {TEAM_NAME_PATTERN}") return team_name @@ -169,6 +172,18 @@ def team_sync(args, *, session=NEW_SESSION): if bundle.team_name is not None } + # The bundle config is a second creation path for teams, so it has to enforce the same + # name rule as `teams create`. Other code relies on a stored team name being a valid one + # -- the environment secrets backend decides whether a supplied secret id could name a + # team namespace by testing the leading segment against this pattern, and an unvalidated + # short name such as "a" would make that test miss. + invalid = sorted(name for name in dag_bundle_teams if not re.match(TEAM_NAME_PATTERN, name)) + if invalid: + raise SystemExit( + f"Invalid team name(s) in the dag bundle config: {', '.join(invalid)}. " + f"Team names must match regex {TEAM_NAME_PATTERN}." + ) + teams_added = 0 try: diff --git a/airflow-core/src/airflow/secrets/environment_variables.py b/airflow-core/src/airflow/secrets/environment_variables.py index 1a50d58138a73..3efbb048c5fbe 100644 --- a/airflow-core/src/airflow/secrets/environment_variables.py +++ b/airflow-core/src/airflow/secrets/environment_variables.py @@ -27,6 +27,15 @@ CONN_ENV_PREFIX = "AIRFLOW_CONN_" VAR_ENV_PREFIX = "AIRFLOW_VAR_" +# Separator between the team name and the secret id in a team namespaced +# environment variable name: AIRFLOW_CONN_____. +TEAM_SEP = "___" + +# What ``airflow teams create`` accepts as a team name, so what a team +# namespace can actually be spelled with. +# Kept in sync with ``airflow.cli.commands.team_command``. +_TEAM_NAME = re.compile(r"[a-zA-Z0-9_-]{3,50}") + class EnvironmentVariablesBackend(BaseSecretsBackend): """Retrieves Connection object and Variable from environment variable.""" @@ -65,25 +74,36 @@ def get_variable(self, key: str, team_name: str | None = None) -> str | None: @staticmethod def _names_a_team_namespace(secret_id: str) -> bool: """ - Whether ``secret_id`` spells out a team namespaced environment variable name. + Whether ``secret_id`` could spell out a team namespaced environment variable name. A team specific secret lives in the ``____`` namespace of the - environment. An id of that shape therefore makes the team agnostic lookup -- which - prepends only ``AIRFLOW_CONN_`` / ``AIRFLOW_VAR_`` -- land inside some team's namespace, - so that lookup is refused for such an id. + environment. An id of that shape makes the team agnostic lookup -- which prepends only + ``AIRFLOW_CONN_`` / ``AIRFLOW_VAR_`` -- land inside some team's namespace, so that + lookup is refused for such an id. + + The test is whether the leading segment **could be a team name**, not merely whether + the id contains the separator. Team names are validated on creation, so an id like + ``_a___b`` cannot name a team namespace -- ``a`` is too short to be a team -- and + refusing it would block a legitimate team agnostic secret for no benefit. Every id + that does spell a real team's namespace has a valid team name in that position by + construction, so nothing reachable is let through. **The id is never attributed to a particular team**, because it cannot be: a team name - may itself contain the ``___`` separator, so ``_a___b___c`` is both team ``a`` with id - ``b___c`` and team ``a___b`` with id ``c``, and nothing in the string distinguishes them. - Only the caller's own namespace is ever constructed, never parsed. - - This is why the check is not "does the id belong to some team other than the caller's". - Comparing the id against the prefix the caller's team builds looks equivalent and is not: - for a caller in team ``a`` the id ``_a___b___c`` starts with ``_A___``, yet the variable - it resolves, ``AIRFLOW_CONN__A___B___C``, is team ``a___b``'s. Treating a prefix match as - ownership hands one team the secrets of every team whose name extends it. Callers reach - their own team's secrets with the bare id plus their team scope -- handled above, and safe - because that path can only ever build the caller's own namespace -- so nothing legitimate - needs a namespaced id here. + may itself contain the separator, so ``_a___b___c`` is both team ``a`` with id + ``b___c`` and team ``a___b`` with id ``c``. Every split is therefore considered, and + one plausible team name is enough to refuse. Comparing the id against the prefix the + caller's own team builds looks equivalent and is not: for a caller in team ``a`` the + id ``_a___b___c`` starts with ``_A___``, yet the variable it resolves, + ``AIRFLOW_CONN__A___B___C``, is team ``a___b``'s. Treating a prefix match as ownership + hands one team the secrets of every team whose name extends it. """ - return re.fullmatch(r"_.+___.+", secret_id) is not None + if not secret_id.startswith("_"): + return False + # Overlapping positions matter: ``_abc____x`` splits at both index 4 and 5. + for i in range(1, len(secret_id) - len(TEAM_SEP) + 1): + if secret_id[i : i + len(TEAM_SEP)] != TEAM_SEP: + continue + team, rest = secret_id[1:i], secret_id[i + len(TEAM_SEP) :] + if rest and _TEAM_NAME.fullmatch(team): + return True + return False diff --git a/airflow-core/tests/unit/always/test_secrets_environment_variables.py b/airflow-core/tests/unit/always/test_secrets_environment_variables.py index 18fb5d2444efd..6e83571eaba7c 100644 --- a/airflow-core/tests/unit/always/test_secrets_environment_variables.py +++ b/airflow-core/tests/unit/always/test_secrets_environment_variables.py @@ -98,6 +98,22 @@ def test_id_spelling_out_a_team_namespace_is_never_resolved( assert lookup(env_prefix, method, team_scoped_id(team_name), team_name) is None + @pytest.mark.parametrize(("env_prefix", "method"), LOOKUPS) + @pytest.mark.parametrize("secret_id", ["_a___b", "_ab___x"]) + @pytest.mark.parametrize("team_name", [None, *TEAM_NAMES]) + def test_id_that_cannot_name_a_team_is_still_resolved( + self, monkeypatch, env_prefix, method, secret_id, team_name + ): + """Refusing every ``_x___y`` id would block legitimate team agnostic secrets. + + Team names are validated as ``^[a-zA-Z0-9_-]{3,50}$``, so a one- or two-character + leading segment cannot be a team. Such an id names no team's namespace and must stay + resolvable through the team agnostic lookup, for any caller. + """ + monkeypatch.setenv(env_prefix + secret_id.upper(), GLOBAL_VALUE) + + assert lookup(env_prefix, method, secret_id, team_name) == GLOBAL_VALUE + @pytest.mark.parametrize(("env_prefix", "method"), LOOKUPS) def test_team_whose_name_extends_the_callers_is_not_readable(self, monkeypatch, env_prefix, method): """A team name may contain the ``___`` separator, so one team's namespace can start with another's. From e8d5fb347f53447fe55f844364b4b43049fe8ee0 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Sat, 1 Aug 2026 03:29:47 +0200 Subject: [PATCH 2/2] Reject team names that differ only in case Team names are case sensitive -- `data_eng` and `Data_Eng` are two distinct teams -- but the environment secrets backend upper-cases the team name when it builds the variable name, so both resolve `AIRFLOW_CONN__DATA_ENG___`. Two such teams share a single secrets namespace, and each reads the other's Connections and Variables. Whether the two rows can coexist at all is decided by the database's collation on a `String` primary key rather than by Airflow, so the same configuration behaves differently across backends. Rejecting the collision at creation makes the behaviour uniform and fails at the moment the name is chosen, rather than silently merging two teams' secrets later. Both creation paths are covered. `teams sync` checks the incoming names against each other as well as against the stored ones, because a bundle config can introduce both halves of a collision in the same run. `upper()` rather than `casefold()`: it is what the backend applies, and team names are restricted to ASCII by the name pattern. Generated-by: Claude Opus 5 (1M context) following the guidelines at https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions --- .../src/airflow/cli/commands/team_command.py | 49 ++++++++++++++++++- .../unit/cli/commands/test_team_command.py | 20 ++++++++ 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/airflow-core/src/airflow/cli/commands/team_command.py b/airflow-core/src/airflow/cli/commands/team_command.py index 5486fa09297c5..837adc23c7939 100644 --- a/airflow-core/src/airflow/cli/commands/team_command.py +++ b/airflow-core/src/airflow/cli/commands/team_command.py @@ -51,6 +51,30 @@ def _show_teams(teams, output): TEAM_NAME_PATTERN = r"^[a-zA-Z0-9_-]{3,50}$" +def _case_collision(team_name: str, existing_names) -> str | None: + """ + Return an existing team name that shares ``team_name``'s secrets namespace, if any. + + Team names are case sensitive -- ``data_eng`` and ``Data_Eng`` are two distinct teams -- + but the environment secrets backend upper-cases the team name when it builds the variable + name, so both resolve ``AIRFLOW_CONN__DATA_ENG___``. Two such teams therefore share a + single namespace and each reads the other's Connections and Variables. + + Whether the two rows can coexist at all is decided by the database's collation on a + ``String`` primary key rather than by Airflow, so the same configuration behaves + differently across backends. Rejecting the collision here makes the behaviour uniform and + fails at the moment the name is chosen, rather than silently merging two teams' secrets. + + ``upper()`` rather than ``casefold()``: it is what the backend applies, and team names are + restricted to ASCII by ``TEAM_NAME_PATTERN``. + """ + folded = team_name.upper() + for existing in existing_names: + if existing != team_name and existing.upper() == folded: + return existing + return None + + def _extract_team_name(args): """Extract and validate team name from args.""" team_name = args.name.strip() @@ -72,6 +96,15 @@ def team_create(args, *, session=NEW_SESSION): if session.scalar(select(Team).where(Team.name == team_name)): raise SystemExit(f"Team with name '{team_name}' already exists") + collision = _case_collision(team_name, Team.get_all_team_names(session=session)) + if collision: + raise SystemExit( + f"Team name '{team_name}' differs only in case from the existing team " + f"'{collision}'. They would share one secrets namespace, so each team could read " + f"the other's Connections and Variables. Choose a name that differs by more than " + f"case." + ) + # Create new team (UUID will be auto-generated by the database) new_team = Team(name=team_name) @@ -184,10 +217,24 @@ def team_sync(args, *, session=NEW_SESSION): f"Team names must match regex {TEAM_NAME_PATTERN}." ) + existing_names = Team.get_all_team_names(session=session) + # Check the incoming names against each other as well as against the stored ones: a bundle + # config can introduce both halves of a collision in the same sync. + seen: set[str] = set(existing_names) + for team_name in sorted(dag_bundle_teams): + collision = _case_collision(team_name, seen) + if collision: + raise SystemExit( + f"Team name '{team_name}' in the dag bundle config differs only in case from " + f"'{collision}'. They would share one secrets namespace, so each team could " + f"read the other's Connections and Variables." + ) + seen.add(team_name) + teams_added = 0 try: - for team_name in dag_bundle_teams - Team.get_all_team_names(session=session): + for team_name in dag_bundle_teams - existing_names: team = Team(name=team_name) session.add(team) teams_added += 1 diff --git a/airflow-core/tests/unit/cli/commands/test_team_command.py b/airflow-core/tests/unit/cli/commands/test_team_command.py index 1df56d9c3cc3f..1cbed6ab19d6c 100644 --- a/airflow-core/tests/unit/cli/commands/test_team_command.py +++ b/airflow-core/tests/unit/cli/commands/test_team_command.py @@ -88,6 +88,26 @@ def test_team_create_invalid_name(self): with pytest.raises(SystemExit, match="Invalid team name"): team_command.team_create(self.parser.parse_args(["teams", "create", "test with space"])) + def test_team_create_rejects_a_name_differing_only_in_case(self): + """Two teams differing only in case would share one secrets namespace. + + The environment secrets backend upper-cases the team name when building the variable + name, so ``data_eng`` and ``Data_Eng`` both resolve ``AIRFLOW_CONN__DATA_ENG___`` + and each team would read the other's Connections and Variables. + """ + team_command.team_create(self.parser.parse_args(["teams", "create", "data_eng"])) + + with pytest.raises(SystemExit, match="differs only in case from the existing team"): + team_command.team_create(self.parser.parse_args(["teams", "create", "Data_Eng"])) + + assert self.session.scalar(select(Team).where(Team.name == "Data_Eng")) is None + + def test_team_create_allows_a_name_differing_by_more_than_case(self): + team_command.team_create(self.parser.parse_args(["teams", "create", "data_eng"])) + team_command.team_create(self.parser.parse_args(["teams", "create", "data_eng2"])) + + assert self.session.scalar(select(Team).where(Team.name == "data_eng2")) is not None + def test_team_create_whitespace_name(self): """Test team creation with whitespace-only name.""" with pytest.raises(SystemExit, match="Team name cannot be empty"):