Forbid consecutive underscores in team names - #70886
Draft
potiuk wants to merge 4 commits into
Draft
Conversation
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
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___<ID>`. 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
A team specific secret is stored as `_<TEAM_NAME>___<SECRET_ID>` in the environment. While a team name could itself contain the `___` separator, that name had no single reading: `_a___b___c` is team `a` with id `b___c` and equally team `a___b` with id `c`, and nothing in the string chooses between them. Every guard over that namespace had to work around the ambiguity rather than resolve it, and the first two attempts at doing so were both wrong in ways that let one team read another's secrets. Remove the ambiguity at the source: `TEAM_NAME_PATTERN` no longer accepts an underscore. A team name can then never span the separator, so the stored id has exactly one reading and the namespace check collapses to a single match against the team name charset. This replaces the split-scanning guard added while the ambiguity still existed. The narrower behaviour it was carrying is preserved: an id whose leading segment could not be a team name still resolves through the team agnostic lookup. 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
A team secret lives at _<TEAM>___<ID>, so the namespace is only unambiguous while no team name can contain the ___ separator. Forbidding every underscore achieves that but is broader than needed and invalidates ordinary names like team_a. Forbidding two in a row is the smallest rule that makes the separator unspellable, so the split becomes a single partition on the first ___. That closes a cross-team read no guard on the team agnostic fall-through could 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 -- and the scoped lookup hits, so the fall-through is never consulted. team_a___prod is no longer a name a team can have. teams sync now validates stored names as well as the ones in the bundle config. It shipped without validation, so an upgrading deployment can already hold a name the guard cannot recognise, and that guard is what keeps one team's namespace out of another's reach. The pattern is defined once in the secrets backend and imported by the CLI, with a test pinning that they agree. It is unanchored and always used with re.fullmatch: re.match against a $-anchored pattern also accepts a trailing newline, and the sync path has no .strip().
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A team scoped secret lives in the
_<TEAM>___<SECRET_ID>namespace of theenvironment, so that namespace is only unambiguous while no team name can itself
contain the
___separator.An earlier revision of this PR got there by forbidding every underscore. That
works, but it is broader than the problem and invalidates ordinary names like
team_a. Forbidding two in a row is the smallest rule that makes theseparator unspellable:
With that, the first
___in the string is always the real separator, so theguard collapses from "try every split and test each candidate" to one
str.partition.What that closes
A cross-team read that no amount of guarding the team agnostic fall-through could
reach, because the fall-through is never consulted:
Byte-identical, and the team scoped lookup hits — so the caller reads the
other team's secret with no fall-through involved. It is closed here not by
another check but by
team_a___prodno longer being a name a team can have.teams syncnow validates stored names tooteams syncshipped with no name validation at all, so a deployment upgradinginto this version can already hold a name this rule rejects. Since the secrets
guard is what keeps one team's namespace out of another's reach, validating only
the incoming bundle config would leave that team's secrets reachable — and would
miss a team created by an earlier sync and since dropped from the config. Both
sets are checked; the command exits non-zero naming the offending names.
One pattern, one matcher
The pattern is defined once in the secrets backend and imported by the CLI, with
a test asserting the two agree so drift fails CI rather than silently reopening
the hole. It is deliberately unanchored and always used with
re.fullmatch:re.matchagainst a$-anchored pattern also accepts a trailing newline, andunlike
teams createthe sync path has no.strip().Behaviour change
__is rejected byteams createandteams sync.Single underscores are unaffected.
teams syncexits non-zero when any stored or configured team name violatesthe rule, where it previously accepted anything.
Test plan
test_team_create_rejects_consecutive_underscores—team__xandteam_a___prodtest_team_create_allows_a_single_underscore—team_astill createstest_team_sync_rejects_an_invalid_bundle_team_name— too-short,__,spans-the-separator, and trailing-newline cases; asserts no team row is created
test_team_sync_rejects_an_invalid_name_already_stored— a storedabfails the sync
test_team_name_pattern_is_shared_with_the_secrets_backendtest_a_valid_team_name_can_never_contain_the_separator— the invariant thepartition rests on
test_the_bare_id_collision_is_no_longer_expressibletest_secrets_environment_variables.pyandtest_team_command.pyruff check/ruff formatcleanWas generative AI tooling used to co-author this PR?
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