diff --git a/src/aap_eda/services/project/git.py b/src/aap_eda/services/project/git.py index e204522f7..f867deea7 100644 --- a/src/aap_eda/services/project/git.py +++ b/src/aap_eda/services/project/git.py @@ -32,6 +32,12 @@ class GitError(Exception): pass +class GitAuthenticationError(Exception): + """Git Authentication error.""" + + pass + + class ExecutableNotFoundError(Exception): """Executable Not Found error.""" @@ -47,7 +53,10 @@ class GitRepository: """Represents a git repository.""" def __init__( - self, root: StrPath, *, _executor: Optional[GitExecutor] = None + self, + root: StrPath, + *, + _executor: Optional[GitExecutor] = None, ): """ Create an instance for existing repository. @@ -116,17 +125,22 @@ def clone( if _executor is None: _executor = GitExecutor() + final_url = url + secret = "" if credential: - passwd = credential.secret.get_secret_value() - _executor.ENVIRON["GIT_PASSWORD"] = passwd + secret = credential.secret.get_secret_value() + # TODO(alex): encapsulate url composition index = 0 if url.startswith("https://"): index = 8 elif url.startswith("http://"): index = 7 if index > 0: - user = credential.username - url = f"{url[:index]}{user}:${{GIT_PASSWORD}}@{url[index:]}" + # user can be optional + user_part = ( + f"{credential.username}:" if credential.username else "" + ) + final_url = f"{url[:index]}{user_part}{secret}@{url[index:]}" if not verify_ssl: _executor.ENVIRON["GIT_SSL_NO_VERIFY"] = "true" @@ -134,9 +148,17 @@ def clone( cmd = ["clone", "--quiet"] if depth is not None: cmd.extend(["--depth", str(depth)]) - cmd.extend([url, os.fspath(path)]) - _executor(cmd) - return GitRepository(path, _executor=_executor) + cmd.extend([final_url, os.fspath(path)]) + logger.info("Cloning repository: %s", url) + try: + _executor(cmd) + except GitError as e: + msg = str(e) + if secret: + msg = msg.replace(secret, "****", 1) + logger.warning("Git clone failed: %s", msg) + raise GitError(msg) from None + return cls(path, _executor=_executor) def _execute_cmd(self, cmd: Iterable[str]): return self._executor(cmd, cwd=self.root) @@ -150,10 +172,10 @@ class GitExecutor: def __call__( self, - args: Union[str, Iterable[str]], + args: Iterable[str], timeout: Optional[float] = None, cwd: Optional[StrPath] = None, - stdout: Optional[IO] = None, + stdout: Union[IO, int, None] = None, ): if stdout is None: stdout = subprocess.PIPE @@ -162,12 +184,9 @@ def __call__( if timeout is None: timeout = self.DEFAULT_TIMEOUT - if isinstance(args, Iterable): - args = " ".join(args) try: return subprocess.run( - f"{GIT_COMMAND} {args}", - shell=True, + [GIT_COMMAND, *args], check=True, encoding="utf-8", env=self.ENVIRON, @@ -180,15 +199,14 @@ def __call__( logger.warning("%s", str(e)) raise GitError(str(e)) from e except subprocess.CalledProcessError as e: - message = ( - "Command returned non-zero exit status %s:" - "\n\tcommand: %s" - "\n\tstderr: %s" - ) - logger.warning(message, e.returncode, e.cmd, e.stderr) if "Authentication failed" in e.stderr: - raise GitError("Authentication failed") + raise GitAuthenticationError("Authentication failed") from None if "could not read Username" in e.stderr: - raise GitError("Credentials not provided") - usr_msg = f"{e} Error: {e.stderr}" - raise GitError(usr_msg) from e + raise GitAuthenticationError( + "Credentials not provided" + ) from None + # generic error + usr_msg = f"Command git failed with return code {e.returncode}." + if e.stderr: + usr_msg += f" Error: {e.stderr}" + raise GitError(usr_msg) from None diff --git a/tests/unit/test_git.py b/tests/unit/test_git.py index fe54b62ea..6a398cdfe 100644 --- a/tests/unit/test_git.py +++ b/tests/unit/test_git.py @@ -19,15 +19,25 @@ import pytest from aap_eda.core.models import Credential -from aap_eda.services.project.git import GitError, GitExecutor, GitRepository +from aap_eda.services.project.git import ( + GitAuthenticationError, + GitError, + GitExecutor, + GitRepository, +) -@pytest.mark.django_db -def test_git_clone(): +@pytest.fixture +def credential() -> Credential: credential = Credential.objects.create( - name="name", username="me", secret="pass" + name="name", username="me", secret="supersecret" ) credential.refresh_from_db() + return credential + + +@pytest.mark.django_db +def test_git_clone(credential: Credential): executor = mock.MagicMock(ENVIRON={}) repository = GitRepository.clone( "https://git.example.com/repo.git", @@ -39,15 +49,71 @@ def test_git_clone(): [ "clone", "--quiet", - "https://me:${GIT_PASSWORD}@git.example.com/repo.git", + "https://me:supersecret@git.example.com/repo.git", "/path/to/repository", - ] + ], ) assert "GIT_SSL_NO_VERIFY" not in executor.ENVIRON assert isinstance(repository, GitRepository) assert repository.root == "/path/to/repository" +@mock.patch("subprocess.run") +@pytest.mark.django_db +def test_git_clone_leak_password( + subprocess_run_mock: mock.Mock, + credential: Credential, +): + executor = GitExecutor() + + def raise_error(cmd, **kwargs): + raise subprocess.CalledProcessError( + 128, + cmd, + stderr="fatal: Unable to access " + "'https://me:supersecret@git.example.com/repo.git'", + ) + + subprocess_run_mock.side_effect = raise_error + + with pytest.raises(GitError) as exc_info: + GitRepository.clone( + "https://git.example.com/repo.git", + "/path/to/repository", + credential=credential, + _executor=executor, + ) + assert "supersecret" not in str(exc_info) + assert "****" in str(exc_info) + + +@mock.patch("subprocess.run") +@pytest.mark.django_db +def test_git_clone_auth_error( + subprocess_run_mock: mock.Mock, + credential: Credential, +): + executor = GitExecutor() + + def raise_error(cmd, **kwargs): + raise subprocess.CalledProcessError( + 128, + cmd, + stderr="Authentication failed", + ) + + subprocess_run_mock.side_effect = raise_error + + with pytest.raises(GitAuthenticationError) as exc_info: + GitRepository.clone( + "https://git.example.com/repo.git", + "/path/to/repository", + credential=credential, + _executor=executor, + ) + assert "supersecret" not in str(exc_info) + + def test_git_clone_without_ssl_verification(): executor = mock.MagicMock(ENVIRON={}) _ = GitRepository.clone( @@ -108,16 +174,13 @@ def test_git_executor_call(run_mock: mock.Mock): executor = GitExecutor() executor(["clone", "https://git.example.com/repo.git", "/test/repo"]) run_mock.assert_called_once_with( - " ".join( - [ - shutil.which("git"), - "clone", - "https://git.example.com/repo.git", - "/test/repo", - ] - ), + [ + shutil.which("git"), + "clone", + "https://git.example.com/repo.git", + "/test/repo", + ], check=True, - shell=True, encoding="utf-8", env={ "GIT_TERMINAL_PROMPT": "0", @@ -140,7 +203,7 @@ def raise_timeout(cmd, **_kwargs): executor = GitExecutor() message = re.escape( - f"""Command '{shutil.which("git")} status' """ + f"""Command '['{shutil.which("git")}', 'status']' """ """timed out after 10 seconds""" ) with pytest.raises(GitError, match=message): @@ -151,15 +214,17 @@ def raise_timeout(cmd, **_kwargs): def test_git_executor_error(run_mock: mock.Mock): def raise_error(cmd, **_kwargs): raise subprocess.CalledProcessError( - 128, cmd, stderr="fatal: not a git repository" + 128, + cmd, + stderr="fatal: not a git repository", ) run_mock.side_effect = raise_error executor = GitExecutor() message = re.escape( - f"""Command '{shutil.which("git")} status'""" - " returned non-zero exit status 128." + "Command git failed with return code 128. " + "Error: fatal: not a git repository", ) with pytest.raises(GitError, match=message): executor(["status"])