fix: align recipe git clone error handling with git_utils (#6544)

* fix: align recipe git clone error handling with git_utils

* fix: handle '@' outside userinfo when formatting git clone URLs

Author: aviruthen

sagemaker-core/src/sagemaker/core/git_utils.py

@@ -337,15 +337,17 @@ def _redact_credentials_from_url(url):
 
     Replaces any username, password, or token embedded before the '@' in an
     HTTPS URL with a placeholder so that credentials are never exposed in
-    logs or exception messages.
+    logs or exception messages. Only the authority (the part before the first
+    '/', '?' or '#') is considered, up to its last '@'. URLs without userinfo,
+    including ones with an '@' later in the path, are returned unchanged.
 
     Args:
         url (str): The Git repository URL that may contain embedded credentials.
 
     Returns:
         str: The URL with credentials replaced by '<credentials-redacted>'.
     """
-    return re.sub(r"(https://)([^@]+)@", r"\1<credentials-redacted>@", url)
+    return re.sub(r"^(https://)[^/?#]*@", r"\1<credentials-redacted>@", url)
 
 
 def _run_clone_command(repo_url, dest_dir):

sagemaker-core/src/sagemaker/core/modules/utils.py

@@ -23,6 +23,7 @@
 from datetime import datetime
 from typing import Literal, Any
 
+from sagemaker.core.git_utils import _redact_credentials_from_url
 from sagemaker.core.shapes import Unassigned
 from sagemaker.core.modules import logger
 
@@ -170,9 +171,16 @@ def _run_clone_command_silent(repo_url, dest_dir):
                 stderr=subprocess.DEVNULL,
             )
         except subprocess.CalledProcessError as e:
-            logger.error(f"Failed to clone repository: {repo_url}")
-            logger.error(f"Error output:\n{e}")
-            raise
+            safe_url = _redact_credentials_from_url(repo_url)
+            error = subprocess.CalledProcessError(
+                e.returncode,
+                ["git", "clone", safe_url, dest_dir],
+                output=e.output,
+                stderr=e.stderr,
+            )
+            logger.error(f"Failed to clone repository: {safe_url}")
+            logger.error(f"Error output:\n{error}")
+            raise error from None
     elif repo_url.startswith("git@") or repo_url.startswith("ssh://"):
         try:
             with tempfile.TemporaryDirectory() as tmp_dir:

sagemaker-core/tests/unit/modules/test_utils_clone.py

@@ -0,0 +1,75 @@
+# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License"). You
+# may not use this file except in compliance with the License. A copy of
+# the License is located at
+#
+#     http://aws.amazon.com/apache2.0/
+#
+# or in the "license" file accompanying this file. This file is
+# distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF
+# ANY KIND, either express or implied. See the License for the specific
+# language governing permissions and limitations under the License.
+"""Unit tests for _run_clone_command_silent in sagemaker.core.modules.utils."""
+
+import logging
+import subprocess
+from unittest.mock import patch
+
+import pytest
+
+from sagemaker.core.modules.utils import _run_clone_command_silent
+
+MODULE = "sagemaker.core.modules.utils"
+DEST = "/tmp/dest"
+
+
+@pytest.mark.parametrize(
+    "repo_url,userinfo",
+    [
+        ("https://oauth2:abc123@example.com/org/repo.git", "oauth2:abc123"),
+        ("https://user:p%40ss@example.com/org/repo.git", "user:p%40ss"),
+        ("https://user:p@ss@example.com/org/repo.git", "user:p@ss"),
+    ],
+)
+def test_clone_failure_uses_sanitized_url(repo_url, userinfo, caplog):
+    with patch(
+        f"{MODULE}.subprocess.check_call",
+        side_effect=subprocess.CalledProcessError(128, ["git", "clone", repo_url, DEST]),
+    ):
+        with caplog.at_level(logging.ERROR):
+            with pytest.raises(subprocess.CalledProcessError) as exc_info:
+                _run_clone_command_silent(repo_url, DEST)
+
+    error = exc_info.value
+    expected_url = "https://<credentials-redacted>@example.com/org/repo.git"
+    assert error.returncode == 128
+    assert error.cmd == ["git", "clone", expected_url, DEST]
+    assert error.__suppress_context__
+    assert userinfo not in str(error)
+    assert userinfo not in caplog.text
+    assert expected_url in caplog.text
+
+
+def test_clone_failure_without_userinfo_keeps_url(caplog):
+    repo_url = "https://example.com/org/repo.git"
+    with patch(
+        f"{MODULE}.subprocess.check_call",
+        side_effect=subprocess.CalledProcessError(128, ["git", "clone", repo_url, DEST]),
+    ):
+        with caplog.at_level(logging.ERROR):
+            with pytest.raises(subprocess.CalledProcessError) as exc_info:
+                _run_clone_command_silent(repo_url, DEST)
+
+    assert exc_info.value.cmd == ["git", "clone", repo_url, DEST]
+    assert repo_url in caplog.text
+
+
+def test_clone_success_uses_original_url():
+    repo_url = "https://oauth2:abc123@example.com/org/repo.git"
+    with patch(f"{MODULE}.subprocess.check_call") as mock_check_call:
+        _run_clone_command_silent(repo_url, DEST)
+
+    args, kwargs = mock_check_call.call_args
+    assert args[0] == ["git", "clone", repo_url, DEST]
+    assert kwargs["env"]["GIT_TERMINAL_PROMPT"] == "0"

sagemaker-core/tests/unit/test_git_utils.py

@@ -342,6 +342,30 @@ def test_no_redaction_without_credentials(self):
         result = git_utils._redact_credentials_from_url(url)
         assert result == url
 
+    def test_redact_password_containing_at_sign(self):
+        """Test that userinfo is redacted up to the last '@' in the authority."""
+        url = "https://user:p@ss@github.com/user/repo.git"
+        result = git_utils._redact_credentials_from_url(url)
+        assert result == "https://<credentials-redacted>@github.com/user/repo.git"
+
+    def test_redact_url_without_path(self):
+        """Test redaction when the URL has no path component."""
+        url = "https://user:token@github.com"
+        result = git_utils._redact_credentials_from_url(url)
+        assert result == "https://<credentials-redacted>@github.com"
+
+    @pytest.mark.parametrize(
+        "url",
+        [
+            "https://github.com/user/repo@v1.git",
+            "https://github.com/user/repo.git?ref=a@b",
+            "https://github.com/user/repo.git#a@b",
+        ],
+    )
+    def test_no_redaction_for_at_sign_outside_authority(self, url):
+        """Test that an '@' after the host is not treated as userinfo."""
+        assert git_utils._redact_credentials_from_url(url) == url
+
     def test_no_redaction_for_ssh_url(self):
         """Test that SSH URLs are not affected by redaction."""
         url = "git@github.com:user/repo.git"

sagemaker-train/src/sagemaker/train/utils.py

@@ -24,6 +24,7 @@
 from datetime import datetime
 from typing import Literal, Any
 
+from sagemaker.core.git_utils import _redact_credentials_from_url
 from sagemaker.core.helper.session_helper import Session
 from sagemaker.core.shapes import Unassigned
 from sagemaker.core.utils.utils import logger
@@ -217,9 +218,16 @@ def _run_clone_command_silent(repo_url, dest_dir):
                 stderr=subprocess.DEVNULL,
             )
         except subprocess.CalledProcessError as e:
-            logger.error(f"Failed to clone repository: {repo_url}")
-            logger.error(f"Error output:\n{e}")
-            raise
+            safe_url = _redact_credentials_from_url(repo_url)
+            error = subprocess.CalledProcessError(
+                e.returncode,
+                ["git", "clone", safe_url, dest_dir],
+                output=e.output,
+                stderr=e.stderr,
+            )
+            logger.error(f"Failed to clone repository: {safe_url}")
+            logger.error(f"Error output:\n{error}")
+            raise error from None
     elif repo_url.startswith("git@") or repo_url.startswith("ssh://"):
         try:
             with tempfile.TemporaryDirectory() as tmp_dir:

sagemaker-train/tests/unit/train/test_utils_clone.py

@@ -0,0 +1,75 @@
+# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License"). You
+# may not use this file except in compliance with the License. A copy of
+# the License is located at
+#
+#     http://aws.amazon.com/apache2.0/
+#
+# or in the "license" file accompanying this file. This file is
+# distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF
+# ANY KIND, either express or implied. See the License for the specific
+# language governing permissions and limitations under the License.
+"""Unit tests for _run_clone_command_silent in sagemaker.train.utils."""
+
+import logging
+import subprocess
+from unittest.mock import patch
+
+import pytest
+
+from sagemaker.train.utils import _run_clone_command_silent
+
+MODULE = "sagemaker.train.utils"
+DEST = "/tmp/dest"
+
+
+@pytest.mark.parametrize(
+    "repo_url,userinfo",
+    [
+        ("https://oauth2:abc123@example.com/org/repo.git", "oauth2:abc123"),
+        ("https://user:p%40ss@example.com/org/repo.git", "user:p%40ss"),
+        ("https://user:p@ss@example.com/org/repo.git", "user:p@ss"),
+    ],
+)
+def test_clone_failure_uses_sanitized_url(repo_url, userinfo, caplog):
+    with patch(
+        f"{MODULE}.subprocess.check_call",
+        side_effect=subprocess.CalledProcessError(128, ["git", "clone", repo_url, DEST]),
+    ):
+        with caplog.at_level(logging.ERROR):
+            with pytest.raises(subprocess.CalledProcessError) as exc_info:
+                _run_clone_command_silent(repo_url, DEST)
+
+    error = exc_info.value
+    expected_url = "https://<credentials-redacted>@example.com/org/repo.git"
+    assert error.returncode == 128
+    assert error.cmd == ["git", "clone", expected_url, DEST]
+    assert error.__suppress_context__
+    assert userinfo not in str(error)
+    assert userinfo not in caplog.text
+    assert expected_url in caplog.text
+
+
+def test_clone_failure_without_userinfo_keeps_url(caplog):
+    repo_url = "https://example.com/org/repo.git"
+    with patch(
+        f"{MODULE}.subprocess.check_call",
+        side_effect=subprocess.CalledProcessError(128, ["git", "clone", repo_url, DEST]),
+    ):
+        with caplog.at_level(logging.ERROR):
+            with pytest.raises(subprocess.CalledProcessError) as exc_info:
+                _run_clone_command_silent(repo_url, DEST)
+
+    assert exc_info.value.cmd == ["git", "clone", repo_url, DEST]
+    assert repo_url in caplog.text
+
+
+def test_clone_success_uses_original_url():
+    repo_url = "https://oauth2:abc123@example.com/org/repo.git"
+    with patch(f"{MODULE}.subprocess.check_call") as mock_check_call:
+        _run_clone_command_silent(repo_url, DEST)
+
+    args, kwargs = mock_check_call.call_args
+    assert args[0] == ["git", "clone", repo_url, DEST]
+    assert kwargs["env"]["GIT_TERMINAL_PROMPT"] == "0"