Copilot commented on code in PR #64618:
URL: https://github.com/apache/airflow/pull/64618#discussion_r3066476030
##########
airflow-ctl/src/airflowctl/api/client.py:
##########
@@ -143,6 +144,14 @@ def _bounded_get_new_password() -> str:
)
+def _safe_path_under_airflow_home(airflow_home: str, filename: str) -> str:
+ base = Path(airflow_home).resolve()
+ target = (base / filename).resolve()
+ if not target.is_relative_to(base):
+ raise ValueError(f"Resolved path escapes AIRFLOW_HOME: {target}")
+ return str(target)
Review Comment:
Raising a raw `ValueError` here is likely to surface as an unhandled
exception/stack trace in CLI flows (save/load). Consider raising an
airflowctl-specific exception with a more actionable message (e.g., include the
offending environment/filename and guidance like 'AIRFLOW_CLI_ENVIRONMENT
contains invalid characters' or 'path traversal detected') so users get a clear
CLI error instead of a generic ValueError.
##########
airflow-ctl/src/airflowctl/api/client.py:
##########
@@ -160,8 +169,11 @@ def __init__(
):
self.api_url = api_url
self.api_token = api_token
- self.api_environment = os.getenv("AIRFLOW_CLI_ENVIRONMENT") or
api_environment
+ raw_env = os.getenv("AIRFLOW_CLI_ENVIRONMENT")
+ if raw_env is None:
+ raw_env = api_environment
self.client_kind = client_kind
+ self.api_environment = os.getenv("AIRFLOW_CLI_ENVIRONMENT") or
api_environment
Review Comment:
The PR description mentions adding a regex allowlist for environment names,
but this diff doesn’t implement any validation. Also, `raw_env` is computed but
then ignored because `self.api_environment` re-reads `os.getenv(...)`. To match
the PR description and fully mitigate attacks, validate `raw_env` with the
stated allowlist (e.g., alphanumeric plus `._-`) and then assign
`self.api_environment = raw_env` (single source of truth) so the validated
value is actually used.
##########
airflow-ctl/tests/airflow_ctl/api/test_client.py:
##########
@@ -392,3 +392,8 @@ def
test_debug_mode_missing_debug_creds_reports_correct_error(self, monkeypatch,
creds = Credentials(client_kind=ClientKind.CLI,
api_environment="TEST_DEBUG")
with pytest.raises(AirflowCtlCredentialNotFoundException, match="Debug
credentials file not found"):
creds.load()
+
+
+def test_credentials_accepts_safe_env():
+ creds = Credentials(client_kind=ClientKind.CLI,
api_environment="prod-us_1")
+ assert creds.api_environment == "prod-us_1"
Review Comment:
This test only asserts a safe name round-trips, but the PR description
claims regression coverage for rejecting malicious paths (e.g., `../evil`). Add
tests that set `AIRFLOW_CLI_ENVIRONMENT` (and/or pass `api_environment`) to
traversal strings and assert the expected exception is raised, plus a case
covering path separators like `a/b` (which currently stays within
`AIRFLOW_HOME` but should be rejected once the allowlist validation is
implemented).
```suggestion
assert creds.api_environment == "prod-us_1"
@pytest.mark.parametrize("api_environment", ["../evil", "..\\evil", "a/b",
"a\\b"])
def test_credentials_rejects_unsafe_env_argument(api_environment):
with pytest.raises(ValueError, match="environment"):
Credentials(client_kind=ClientKind.CLI,
api_environment=api_environment)
@pytest.mark.parametrize("api_environment", ["../evil", "..\\evil", "a/b",
"a\\b"])
def
test_credentials_rejects_unsafe_env_from_environment_variable(monkeypatch,
api_environment):
monkeypatch.setenv("AIRFLOW_CLI_ENVIRONMENT", api_environment)
with pytest.raises(ValueError, match="environment"):
Credentials(client_kind=ClientKind.CLI)
```
--
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]
