diff --git a/sagemaker-core/src/sagemaker/core/git_utils.py b/sagemaker-core/src/sagemaker/core/git_utils.py index d087d50b8d..b200595ef6 100644 --- a/sagemaker-core/src/sagemaker/core/git_utils.py +++ b/sagemaker-core/src/sagemaker/core/git_utils.py @@ -337,7 +337,9 @@ 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. @@ -345,7 +347,7 @@ def _redact_credentials_from_url(url): Returns: str: The URL with credentials replaced by ''. """ - return re.sub(r"(https://)([^@]+)@", r"\1@", url) + return re.sub(r"^(https://)[^/?#]*@", r"\1@", url) def _run_clone_command(repo_url, dest_dir): diff --git a/sagemaker-core/src/sagemaker/core/modules/utils.py b/sagemaker-core/src/sagemaker/core/modules/utils.py index c9bd896689..e3778bddc1 100644 --- a/sagemaker-core/src/sagemaker/core/modules/utils.py +++ b/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: diff --git a/sagemaker-core/tests/unit/modules/test_utils_clone.py b/sagemaker-core/tests/unit/modules/test_utils_clone.py new file mode 100644 index 0000000000..0c59fe8b2f --- /dev/null +++ b/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://@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" diff --git a/sagemaker-core/tests/unit/test_git_utils.py b/sagemaker-core/tests/unit/test_git_utils.py index 9d8367bedd..cbe78bb3cc 100644 --- a/sagemaker-core/tests/unit/test_git_utils.py +++ b/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://@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://@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" diff --git a/sagemaker-train/src/sagemaker/train/utils.py b/sagemaker-train/src/sagemaker/train/utils.py index 31d26daecd..e9483c3b91 100644 --- a/sagemaker-train/src/sagemaker/train/utils.py +++ b/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: diff --git a/sagemaker-train/tests/unit/train/test_utils_clone.py b/sagemaker-train/tests/unit/train/test_utils_clone.py new file mode 100644 index 0000000000..14170b55b2 --- /dev/null +++ b/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://@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"