Copilot commented on code in PR #64105:
URL: https://github.com/apache/airflow/pull/64105#discussion_r3066494348
##########
providers/git/src/airflow/providers/git/hooks/git.py:
##########
@@ -145,20 +144,24 @@ def _build_ssh_command(self, key_path: str | None = None)
-> str:
def _process_git_auth_url(self):
if not isinstance(self.repo_url, str):
return
- if self.auth_token and self.repo_url.startswith("https://";):
- encoded_user = urlquote(self.user_name, safe="")
- encoded_token = urlquote(self.auth_token, safe="")
- self.repo_url = self.repo_url.replace("https://";,
f"https://{encoded_user}:{encoded_token}@";, 1)
- elif self.auth_token and self.repo_url.startswith("http://";):
- encoded_user = urlquote(self.user_name, safe="")
- encoded_token = urlquote(self.auth_token, safe="")
- self.repo_url = self.repo_url.replace("http://";,
f"http://{encoded_user}:{encoded_token}@";, 1)
- elif self.repo_url.startswith("http://";):
- # if no auth token, use the repo url as is
- pass
- elif not self.repo_url.startswith("git@") and not
self.repo_url.startswith("https://";):
+
+ if not self.repo_url.startswith("git@") and not
self.repo_url.startswith("https://";):
self.repo_url = os.path.expanduser(self.repo_url)
+ @contextlib.contextmanager
+ def _token_askpass_env(self):
+ if not self.auth_token:
+ yield
+ return
+
+ with tempfile.NamedTemporaryFile(mode="w", suffix=".sh", delete=True)
as askpass_script:
+ askpass_script.write(f"#!/bin/sh\necho
{shlex.quote(self.auth_token)}\n")
+ askpass_script.flush()
+ os.chmod(askpass_script.name, stat.S_IRWXU)
+
+ self.env["GIT_ASKPASS"] = askpass_script.name
+ yield
Review Comment:
The `GIT_ASKPASS` script always echoes the token, but Git commonly prompts
separately for `Username` and `Password` over HTTPS. This can cause auth
failures when Git asks for a username (the token would be returned instead).
Update the script to branch on the prompt argument (`$1`) and return
`self.user_name` for username prompts and `self.auth_token` for password
prompts (and consider a safe default for unexpected prompts). Add/adjust unit
tests to validate both prompt paths.
##########
providers/git/src/airflow/providers/git/hooks/git.py:
##########
@@ -145,20 +144,24 @@ def _build_ssh_command(self, key_path: str | None = None)
-> str:
def _process_git_auth_url(self):
if not isinstance(self.repo_url, str):
return
- if self.auth_token and self.repo_url.startswith("https://";):
- encoded_user = urlquote(self.user_name, safe="")
- encoded_token = urlquote(self.auth_token, safe="")
- self.repo_url = self.repo_url.replace("https://";,
f"https://{encoded_user}:{encoded_token}@";, 1)
- elif self.auth_token and self.repo_url.startswith("http://";):
- encoded_user = urlquote(self.user_name, safe="")
- encoded_token = urlquote(self.auth_token, safe="")
- self.repo_url = self.repo_url.replace("http://";,
f"http://{encoded_user}:{encoded_token}@";, 1)
- elif self.repo_url.startswith("http://";):
- # if no auth token, use the repo url as is
- pass
- elif not self.repo_url.startswith("git@") and not
self.repo_url.startswith("https://";):
+
+ if not self.repo_url.startswith("git@") and not
self.repo_url.startswith("https://";):
self.repo_url = os.path.expanduser(self.repo_url)
+ @contextlib.contextmanager
+ def _token_askpass_env(self):
+ if not self.auth_token:
+ yield
+ return
+
+ with tempfile.NamedTemporaryFile(mode="w", suffix=".sh", delete=True)
as askpass_script:
+ askpass_script.write(f"#!/bin/sh\necho
{shlex.quote(self.auth_token)}\n")
+ askpass_script.flush()
+ os.chmod(askpass_script.name, stat.S_IRWXU)
+
+ self.env["GIT_ASKPASS"] = askpass_script.name
+ yield
Review Comment:
When relying on `GIT_ASKPASS`, Git may still attempt interactive prompting
depending on its prompt fallback behavior. To prevent tasks from hanging in
non-interactive environments, consider setting `GIT_TERMINAL_PROMPT=0` in the
same scope as `GIT_ASKPASS` (and cleaning it up/restoring it on exit similarly).
```suggestion
old_git_askpass = self.env.get("GIT_ASKPASS")
old_git_terminal_prompt = self.env.get("GIT_TERMINAL_PROMPT")
try:
self.env["GIT_ASKPASS"] = askpass_script.name
self.env["GIT_TERMINAL_PROMPT"] = "0"
yield
finally:
for var, old_val in [
("GIT_ASKPASS", old_git_askpass),
("GIT_TERMINAL_PROMPT", old_git_terminal_prompt),
]:
if old_val is None:
self.env.pop(var, None)
else:
self.env[var] = old_val
```
##########
providers/git/src/airflow/providers/git/hooks/git.py:
##########
@@ -145,20 +144,24 @@ def _build_ssh_command(self, key_path: str | None = None)
-> str:
def _process_git_auth_url(self):
if not isinstance(self.repo_url, str):
return
- if self.auth_token and self.repo_url.startswith("https://";):
- encoded_user = urlquote(self.user_name, safe="")
- encoded_token = urlquote(self.auth_token, safe="")
- self.repo_url = self.repo_url.replace("https://";,
f"https://{encoded_user}:{encoded_token}@";, 1)
- elif self.auth_token and self.repo_url.startswith("http://";):
- encoded_user = urlquote(self.user_name, safe="")
- encoded_token = urlquote(self.auth_token, safe="")
- self.repo_url = self.repo_url.replace("http://";,
f"http://{encoded_user}:{encoded_token}@";, 1)
- elif self.repo_url.startswith("http://";):
- # if no auth token, use the repo url as is
- pass
- elif not self.repo_url.startswith("git@") and not
self.repo_url.startswith("https://";):
+
+ if not self.repo_url.startswith("git@") and not
self.repo_url.startswith("https://";):
self.repo_url = os.path.expanduser(self.repo_url)
+ @contextlib.contextmanager
+ def _token_askpass_env(self):
+ if not self.auth_token:
+ yield
+ return
+
+ with tempfile.NamedTemporaryFile(mode="w", suffix=".sh", delete=True)
as askpass_script:
+ askpass_script.write(f"#!/bin/sh\necho
{shlex.quote(self.auth_token)}\n")
+ askpass_script.flush()
+ os.chmod(askpass_script.name, stat.S_IRWXU)
+
+ self.env["GIT_ASKPASS"] = askpass_script.name
+ yield
Review Comment:
`self.env[\"GIT_ASKPASS\"]` is set for the duration of the context, but it
is never removed/restored afterwards, leaving a stale path in `hook.env`
post-context. Wrap the `yield` in a `try/finally` and restore any previous
`GIT_ASKPASS` value (or delete the key) on exit to keep `hook.env` accurate and
reduce accidental reuse.
```suggestion
had_git_askpass = "GIT_ASKPASS" in self.env
old_git_askpass = self.env.get("GIT_ASKPASS")
try:
self.env["GIT_ASKPASS"] = askpass_script.name
yield
finally:
if had_git_askpass:
self.env["GIT_ASKPASS"] = old_git_askpass
else:
self.env.pop("GIT_ASKPASS", None)
```
--
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]
