Refuse only ids whose leading segment could name a team by potiuk · Pull Request #70884 · apache/airflow

potiuk · 2026-08-01T01:17:27Z

Follow-up to #70736, which merged
with a refusal rule that is broader than it needs to be.

That change made the team agnostic lookup refuse any id of the form
_<x>___<y>, on the grounds that such an id could name a team's namespace. That
is true of some of them, not all: team names are validated as
^[a-zA-Z0-9_-]{3,50}$, so a one- or two-character leading segment cannot be a
team. A team agnostic secret whose id merely looks namespaced — _a___b,
_ab___x — is currently refused even though no team namespace can be spelled
that way, which blocks a legitimate lookup for no benefit.

Approach

Test the leading segment against the team name rule rather than testing
whether the id merely contains the separator:

for i in ...:                      # every position of the separator
    team, rest = secret_id[1:i], secret_id[i + 3:]
    if rest and _TEAM_NAME.fullmatch(team):
        return True

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. Ids that
cannot name a team resolve normally again.

Every split is considered, because a team name may itself contain the separator —
_a___b___c is both team a with id b___c and team a___b with id c — and
one plausible team name is enough to refuse. Overlapping separator positions are
included, so _abc____x is tested at both offsets.

teams sync now enforces the team name rule. This is what makes the above
sound: the guard assumes a stored team name is a valid one. airflow teams sync
creates teams from the dag bundle config and did not validate the names at all,
unlike teams create — so an unvalidated short name such as a would make the
test miss and reopen the cross-team read that #70736 closed. The pattern is
factored into TEAM_NAME_PATTERN and applied on both creation paths, with a
comment on the sync path recording why the secrets backend depends on it.

Behaviour change

Ids whose leading segment cannot be a team name resolve through the team
agnostic lookup again, for every caller scope. Ids that could name a team stay
refused exactly as Only resolve a team namespaced environment secret for its own team #70736 made them.
airflow teams sync now fails with a clear message when the dag bundle config
contains a team name that does not match ^[a-zA-Z0-9_-]{3,50}$, where it
previously created the team silently.

Test plan

test_secrets_environment_variables.py — 46 cases pass
New test_id_that_cannot_name_a_team_is_still_resolved — _a___b and
_ab___x resolve for every caller scope including none; it fails against the
currently merged rule
The existing cross-team tests are unchanged and still pass, including
test_team_whose_name_extends_the_callers_is_not_readable
test_team_command.py — 21 cases pass with the shared pattern
Exhaustive property check over six team names chosen to collide (team_a /
team_a___prod, abc / abc___b), both lookups, every caller scope, bare and
namespaced ids: 0 cross-team resolutions
ruff check / ruff format clean

Was generative AI tooling used to co-author this PR?

Yes — Claude Opus 5 (1M context)

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

Refusing every id of the form `_<x>___<y>` 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

shahar1

The relaxation itself is sound and the reasoning in the docstring is genuinely good — but it rests on an invariant ("a stored team name is a valid one") that isn't true for deployments upgrading from 3.3.0, the new teams sync guard that establishes it has no test, and while reading the same function I found the original cross-team read is still reachable by a different spelling.

The team-scoped path still leaks across teams (`environment_variables.py:44`, `:62`)

Pre-existing — not introduced here — but this PR rewrites the docstring that reasons about exactly this case, and correctly deletes the sentence claiming the scoped path "can only ever build the caller's own namespace". Nothing replaces it, and the claim was false:

# caller in team "team_a", asking for the BARE id "prod___dbconn"
os.environ.get(f"{CONN_ENV_PREFIX}_{team_name.upper()}___" + conn_id.upper())
# -> "AIRFLOW_CONN__TEAM_A___PROD___DBCONN"

# team "team_a___prod" stores its "dbconn" secret as
# "AIRFLOW_CONN_" + "_TEAM_A___PROD" + "___" + "DBCONN"
# -> "AIRFLOW_CONN__TEAM_A___PROD___DBCONN"     # <- identical

I confirmed the two strings are byte-identical. The id carries no leading _, so _names_a_team_namespace would not fire even if it were consulted — and it isn't, because the scoped branch returns first. test_team_whose_name_extends_the_callers_is_not_readable uses this exact pair of team names but only exercises the namespaced spelling _team_a___prod___dbconn; the bare spelling walks straight through.

Both this and the every-split loop below come from the same root: ___ is not a reserved character, so _<TEAM>___<ID> does not parse uniquely. Forbidding ___ inside a team name makes the namespace unambiguous, collapses the new loop into a single exact split, and closes the scoped-path collision — at the cost of a rename path for any existing team already containing ___. Worth considering as the follow-up rather than layering another heuristic.

The invariant isn't retroactive (`team_command.py:180`)

airflow teams sync shipped in 3.3.0 without name validation, so a live 3.3.0 deployment can already hold a team named ab. After this change _ab___x resolves through the team agnostic lookup into AIRFLOW_CONN__AB___X — that team's secret x. That is precisely the read #70736 closed, and _ab___x is one of the two ids the new test asserts must resolve.

The new check validates the names in the Dag bundle config, which catches this only while the short name is still in the config; a team row created by an earlier sync and since dropped from the config is missed. The sync already has the stored names in hand one line later (Team.get_all_team_names(session=session)), so validating those too is nearly free — as a hard error, or a warning if failing on pre-existing data is too aggressive for a patch release.

Related, given the backport-to-v3-3-test label: on a 3.3.x patch upgrade a deployment whose Dag bundle config carries a short team name will see airflow teams sync start exiting non-zero where it previously succeeded. That is defensible, but it should be a deliberate call rather than a side effect.

No test for the new `teams sync` validation (`team_command.py:181-185`)

Flag any changed or added behaviour without a corresponding test, and flag tests that a reviewer cannot fail by reverting the PR's change. The target is exactly 100% coverage of what the PR changes — no more, no less.

— .github/instructions/code-review.instructions.md § Testing Requirements

The diff touches only test_secrets_environment_variables.py; test_team_command.py is unchanged, and its test_team_sync covers only the happy path. The PR body's "21 cases pass with the shared pattern" confirms the existing tests still pass, which is not the same thing — no test fails if the new invalid check is deleted. Given this guard is what makes the secrets relaxation sound, it is the one behaviour here that most needs pinning: one case with "team_name": "a" in the bundle config asserting SystemExit and that no team row is created.

Smaller observations

environment_variables.py:37 — the two spellings of "the same" pattern are not equivalent. $ also matches before a trailing newline, so re.match(r"^[a-zA-Z0-9_-]{3,50}$", "team1\n") is a match while _TEAM_NAME.fullmatch("team1\n") is not (verified). teams create is saved by its .strip(); the new sync path has none, so a Dag bundle config with a trailing newline in team_name creates a team the secrets guard cannot recognise. Using one unanchored pattern with re.fullmatch at both sites removes the divergence. A # Kept in sync with ... comment is also the kind of promise better held by a one-line test asserting the two constants are equal — otherwise drift silently reopens the hole instead of failing CI.
airflow-core/docs/core-concepts/multi-team.rst:517-550 — "Dag Bundle to Team Association" is where users actually set team_name, and the newly enforced naming rule isn't mentioned there. The existing note ("The team specified in team_name must exist in the database before syncing... Create teams first using airflow teams create") already reads oddly next to a sync that creates them.
environment_variables.py:34 — the comment now understates the coupling: after this PR the pattern is what teams create and teams sync accept.
Newsfragment — airflow teams sync now exits with an error on configs it previously accepted. That is user-facing CLI behaviour, more so with the 3.3 backport label, so airflow-core/newsfragments/70884.bugfix.rst looks warranted here rather than optional.

This review was drafted by an AI-assisted tool and confirmed by an Airflow maintainer. The findings below are observations, not blockers; an Airflow maintainer — a real person — will take the next look at the PR. If you think a finding is mis-applied, please reply on the PR and a maintainer will weigh in.

More on how Airflow handles maintainer review: contributing-docs/05_pull_requests.rst.

shahar1 · 2026-08-01T06:36:21Z

+        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.


Deleting the old "that path can only ever build the caller's own namespace" sentence is the right call — it was false. But the gap it papered over is still open, and it's the same cross-team read #70736 closed, just spelled differently.

A caller in team_a supplying the bare id prod___dbconn never reaches this guard: the scoped branch above returns first, and the id has no leading _ anyway.

# caller: get_conn_value("prod___dbconn", team_name="team_a") f"{CONN_ENV_PREFIX}_{team_name.upper()}___" + conn_id.upper() # -> AIRFLOW_CONN__TEAM_A___PROD___DBCONN # team "team_a___prod" stores "dbconn" as # "AIRFLOW_CONN_" + "_TEAM_A___PROD" + "___" + "DBCONN" # -> AIRFLOW_CONN__TEAM_A___PROD___DBCONN <- identical

I checked; the strings are byte-identical. test_team_whose_name_extends_the_callers_is_not_readable uses this exact pair of team names but only the namespaced spelling.

Pre-existing rather than introduced here, so not a blocker on this diff — but worth a follow-up, and the root cause is shared with the loop below: ___ isn't reserved, so _<TEAM>___<ID> doesn't parse uniquely. Forbidding ___ inside a team name would make it unambiguous, collapse this loop into one exact split, and close the scoped-path collision too.

shahar1 · 2026-08-01T06:36:21Z

+    # -- 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))


This validates the names in the Dag bundle config, but the secrets guard depends on every stored team name being valid — and teams sync shipped in 3.3.0 with no validation at all. A live 3.3.0 deployment can already hold a team named ab, and after the companion change _ab___x resolves through the team agnostic lookup into AIRFLOW_CONN__AB___X, that team's secret. That's the read #70736 closed, and _ab___x is one of the two ids the new test asserts must resolve.

The config-side check catches this only while the short name is still in the config; a team row created by an earlier sync and since dropped from the config is missed. The stored names are in hand one line later (Team.get_all_team_names(session=session)), so covering them too is nearly free — hard error, or a warning if failing on pre-existing data is too aggressive for a patch release.

Related, given the backport-to-v3-3-test label: on a 3.3.x patch upgrade, a deployment whose bundle config carries a short team name will see airflow teams sync start exiting non-zero where it previously succeeded. Defensible, but better as a deliberate call than a side effect.

Separately — re.match with a $-anchored pattern accepts a trailing newline, and unlike teams create this path has no .strip(). See the note on _TEAM_NAME in environment_variables.py.

Answering the question directly, since you asked for it to be a deliberate call rather than a side effect: hard error, and no backport.

This PR is now a draft on 3.4.0 with backport-to-v3-3-test removed, and the work moved to #70886. So the "3.3.x patch upgrade starts exiting non-zero" case you flagged does not arise — nothing here ships in a patch release. Given that, the warning option loses its only real justification, and a warning would leave the deployment believing its teams are isolated when they are not.

Both halves are implemented in #70886: teams sync validates the stored names alongside the bundle-config ones (Team.get_all_team_names as you pointed out, one line down and nearly free), and it exits naming the offending names.

Your other two points are in there as well — one unanchored pattern used with re.fullmatch at both sites, defined once in the secrets backend and imported by the CLI, with test_team_name_pattern_is_shared_with_the_secrets_backend so drift fails CI rather than silently reopening the hole. You were right about re.match and the trailing newline; my first attempt at that test asserted teams create rejects "team1\n" and it does not, because _extract_team_name strips first. The exposure is on the sync path exactly as you said, and the test moved there.

And the finding at the top of this review — the bare-id spelling — I reproduced it. A caller in team_a asking for prod___dbconn returns team_a___prod's secret. It is the reason this PR is held rather than fixed up: narrowing the team agnostic refusal cannot reach a read that the scoped lookup satisfies. #70886 closes it at the source by making team_a___prod unspellable, relaxed from forbidding every underscore to forbidding consecutive ones so team_a stays valid.

Worth being explicit about one consequence: this PR's own new test asserted _ab___x must resolve, and with an unvalidated stored team named ab that assertion pins the vulnerable behaviour. That is what your comment caught.

shahar1 · 2026-08-01T06:36:21Z

+        raise SystemExit(
+            f"Invalid team name(s) in the dag bundle config: {', '.join(invalid)}. "
+            f"Team names must match regex {TEAM_NAME_PATTERN}."
+        )


No test covers this new behaviour — the diff touches only test_secrets_environment_variables.py, and test_team_sync is unchanged and happy-path only.

Flag any changed or added behaviour without a corresponding test, and flag tests that a reviewer cannot fail by reverting the PR's change. The target is exactly 100% coverage of what the PR changes — no more, no less.

— .github/instructions/code-review.instructions.md § Testing Requirements

The PR body's "21 cases pass with the shared pattern" confirms nothing regressed, which isn't the same thing: delete this if invalid: block and the suite stays green. Since this guard is what makes the secrets relaxation sound, it's the behaviour here that most needs pinning — one case with "team_name": "a" in the bundle config asserting SystemExit and that no team row is created.

shahar1 · 2026-08-01T06:36:21Z

+# 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}")


These two spellings aren't equivalent, which undercuts the "kept in sync" promise: $ also matches before a trailing newline, so

re.match(r"^[a-zA-Z0-9_-]{3,50}$", "team1\n") # match _TEAM_NAME.fullmatch("team1\n") # None

(verified). teams create is saved by its .strip(); the new teams sync path has none, so a Dag bundle config with a trailing newline in team_name creates a team this guard cannot recognise. One unanchored pattern plus re.fullmatch at both sites removes the divergence entirely.

Also: a # Kept in sync with ... comment is the kind of promise better held by a one-line test asserting the two constants are equal — otherwise drift silently reopens the hole instead of failing CI.

Minor: the comment above now understates the coupling — after this PR the pattern is what teams create and teams sync accept.

potiuk · 2026-08-01T11:02:23Z

Superseded by #70886, which now targets the same 3.4.0 milestone.

@shahar1's review here found that the cross-team read is still open in a spelling
none of these guards reach: a caller in team_a asking for the bare id
prod___dbconn builds AIRFLOW_CONN__TEAM_A___PROD___DBCONN, byte-identical to
what team team_a___prod builds for id dbconn. The scoped lookup hits, so the
team agnostic fall-through — which is all this PR narrows — is never consulted. I
reproduced it; it returns the other team's secret.

The root cause he identified is that ___ is not reserved, so _<TEAM>___<ID>
does not parse uniquely, and every revision here has been an after-the-fact
ambiguity check. #70886 fixes that at the source by forbidding consecutive
underscores in a team name (relaxed from forbidding every underscore, so team_a
stays legal). With the separator unspellable the split becomes one partition,
and team_a___prod simply stops being a name a team can have.

His other two findings are folded in there as well:

teams sync now validates stored names, not only the bundle config — it
shipped in 3.3.0 with no validation, so an upgrading deployment can already hold
a name the guard cannot recognise;
the pattern is defined once and imported, unanchored and used with
re.fullmatch throughout, with a test pinning that the CLI and backend agree —
re.match against a $-anchored pattern accepts a trailing newline and the sync
path has no .strip().

This PR is now a draft on 3.4.0 with the backport label removed. Closing it in
favour of #70886 once that lands; leaving it open for now so the review thread
stays reachable.

Worth stating plainly: the relaxation here rested on team names being at least 3
characters, and teams sync never enforced that — so its new test asserting
_ab___x must resolve was pinning the vulnerable behaviour. Good catch.

potiuk requested review from ashb, bugraoz93, dheerajturaga, dstandish and henry3260 as code owners August 1, 2026 01:17

boring-cyborg Bot added area:CLI area:secrets labels Aug 1, 2026

potiuk added this to the Airflow 3.3.1 milestone Aug 1, 2026

potiuk added the backport-to-v3-3-test Backport to v3-3-test label Aug 1, 2026

This was referenced Aug 1, 2026

Reject team names that differ only in case #70885

Open

Forbid consecutive underscores in team names #70886

Draft

shahar1 reviewed Aug 1, 2026

View reviewed changes

shahar1 approved these changes Aug 1, 2026

View reviewed changes

potiuk marked this pull request as draft August 1, 2026 10:16

potiuk removed the backport-to-v3-3-test Backport to v3-3-test label Aug 1, 2026

potiuk modified the milestones: Airflow 3.3.1, Airflow 3.4.0 Aug 1, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refuse only ids whose leading segment could name a team - #70884

Refuse only ids whose leading segment could name a team#70884
potiuk wants to merge 1 commit into
apache:mainfrom
potiuk:narrow-team-namespace-refusal-to-plausible-team-names

potiuk commented Aug 1, 2026

Uh oh!

shahar1 left a comment

Uh oh!

shahar1 Aug 1, 2026

Uh oh!

shahar1 Aug 1, 2026

Uh oh!

potiuk Aug 1, 2026

Uh oh!

shahar1 Aug 1, 2026

Uh oh!

shahar1 Aug 1, 2026

Uh oh!

potiuk commented Aug 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

potiuk commented Aug 1, 2026

Approach

Behaviour change

Test plan

Was generative AI tooling used to co-author this PR?

Uh oh!

shahar1 left a comment

Choose a reason for hiding this comment

The team-scoped path still leaks across teams (environment_variables.py:44, :62)

The invariant isn't retroactive (team_command.py:180)

No test for the new teams sync validation (team_command.py:181-185)

Smaller observations

Uh oh!

shahar1 Aug 1, 2026

Choose a reason for hiding this comment

Uh oh!

shahar1 Aug 1, 2026

Choose a reason for hiding this comment

Uh oh!

potiuk Aug 1, 2026

Choose a reason for hiding this comment

Uh oh!

shahar1 Aug 1, 2026

Choose a reason for hiding this comment

Uh oh!

shahar1 Aug 1, 2026

Choose a reason for hiding this comment

Uh oh!

potiuk commented Aug 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

The team-scoped path still leaks across teams (`environment_variables.py:44`, `:62`)

The invariant isn't retroactive (`team_command.py:180`)

No test for the new `teams sync` validation (`team_command.py:181-185`)