potiuk commented on code in PR #70884:
URL: https://github.com/apache/airflow/pull/70884#discussion_r3695586636
##########
airflow-core/src/airflow/cli/commands/team_command.py:
##########
@@ -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))
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
