amoghrajesh commented on code in PR #70902:
URL: https://github.com/apache/airflow/pull/70902#discussion_r3701431649
##########
airflow-core/src/airflow/secrets/environment_variables.py:
##########
@@ -51,39 +59,18 @@ def get_variable(self, key: str, team_name: str | None =
None) -> str | None:
:param team_name: Team name associated to the task trying to access
the variable (if any)
:return: Variable Value
"""
+ if TEAM_SEP in key:
+ # Refused ahead of the team scoped lookup, not only ahead of the
team agnostic
+ # one. The scoped lookup builds PREFIX + _<TEAM>___ + <ID>, so an
id that itself
+ # contains the separator makes that string ambiguous: a caller in
team `team_a`
+ # asking for `prod___dbconn` builds exactly what team
`team_a___prod` builds for
+ # id `dbconn`. That lookup *hits*, so a guard placed after it
never runs.
Review Comment:
```suggestion
# Same collision risk as get_conn_value, see its code comment.
```
nit
##########
airflow-core/src/airflow/secrets/environment_variables.py:
##########
@@ -20,27 +20,35 @@
from __future__ import annotations
import os
-import re
from airflow.secrets import BaseSecretsBackend
CONN_ENV_PREFIX = "AIRFLOW_CONN_"
VAR_ENV_PREFIX = "AIRFLOW_VAR_"
+# Separates the team name from the secret id in a team namespaced environment
variable
+# name: AIRFLOW_CONN__<TEAM>___<ID>.
+TEAM_SEP = "___"
+
class EnvironmentVariablesBackend(BaseSecretsBackend):
"""Retrieves Connection object and Variable from environment variable."""
def get_conn_value(self, conn_id: str, team_name: str | None = None) ->
str | None:
+ if TEAM_SEP in conn_id:
+ # Refused ahead of the team scoped lookup, not only ahead of the
team agnostic
+ # one. The scoped lookup builds PREFIX + _<TEAM>___ + <ID>, so an
id that itself
+ # contains the separator makes that string ambiguous: a caller in
team `team_a`
+ # asking for `prod___dbconn` builds exactly what team
`team_a___prod` builds for
+ # id `dbconn`. That lookup *hits*, so a guard placed after it
never runs.
Review Comment:
```suggestion
# An id containing the separator could collide with another
team's namespace
# even on the scoped lookup below, so it must be refused before
either runs.
```
nit.
--
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]
