-
Notifications
You must be signed in to change notification settings - Fork 108
fix: enhance git remote validation by trying SSH URLs for generic hosts #584
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
zzoubian
wants to merge
8
commits into
microsoft:main
Choose a base branch
from
zzoubian:fix/self-hosted-git-failed-validation
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+155
−9
Open
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
920bb59
fix: enhance git remote validation by trying SSH URLs for generic hosts
zzoubian 0164035
add unit tests and add encoding to subprocess call
zzoubian 654af2a
Merge branch 'main' into fix/self-hosted-git-failed-validation
zzoubian b4d2c7c
Merge branch 'main' into fix/self-hosted-git-failed-validation
sergio-sisternes-epam 58e918e
Merge branch 'main' into fix/self-hosted-git-failed-validation
sergio-sisternes-epam 0c47bf0
Merge branch 'main' into fix/self-hosted-git-failed-validation
danielmeppiel 96fd9f5
Merge branch 'main' into fix/self-hosted-git-failed-validation
danielmeppiel f3c90aa
Merge branch 'main' into fix/self-hosted-git-failed-validation
danielmeppiel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -862,3 +862,134 @@ def test_global_rejects_tilde_local_path(self): | |||||
| assert "not supported at user scope" in result.output | ||||||
| finally: | ||||||
| os.chdir(self.original_dir) | ||||||
|
|
||||||
| # --------------------------------------------------------------------------- | ||||||
| # Generic-host SSH-first validation tests | ||||||
| # --------------------------------------------------------------------------- | ||||||
|
|
||||||
| class TestGenericHostSshFirstValidation: | ||||||
| """Tests for the SSH-first ls-remote logic added for generic (non-GitHub/ADO) hosts.""" | ||||||
|
|
||||||
| def _make_completed_process(self, returncode, stderr=""): | ||||||
| """Return a minimal subprocess.CompletedProcess-like mock.""" | ||||||
| mock = MagicMock() | ||||||
| mock.returncode = returncode | ||||||
| mock.stderr = stderr | ||||||
| mock.stdout = "" | ||||||
| return mock | ||||||
|
|
||||||
| @patch("subprocess.run") | ||||||
| def test_generic_host_tries_ssh_first_and_succeeds(self, mock_run): | ||||||
| """SSH URL is tried first for generic hosts and used when it succeeds.""" | ||||||
| from apm_cli.commands.install import _validate_package_exists | ||||||
|
|
||||||
| # SSH probe succeeds on the first call | ||||||
| mock_run.return_value = self._make_completed_process(returncode=0) | ||||||
|
|
||||||
| result = _validate_package_exists( | ||||||
| "git@git.example.org:org/group/repo.git", verbose=False | ||||||
| ) | ||||||
|
|
||||||
| assert result is True | ||||||
| # subprocess.run must have been called at least once | ||||||
| assert mock_run.call_count >= 1 | ||||||
| # First call must use the SSH URL | ||||||
| first_call_cmd = mock_run.call_args_list[0][0][0] | ||||||
| assert any("git@git.example.org" in arg for arg in first_call_cmd), ( | ||||||
| f"Expected SSH URL in first ls-remote call, got: {first_call_cmd}" | ||||||
| ) | ||||||
|
|
||||||
| @patch("subprocess.run") | ||||||
| def test_generic_host_falls_back_to_https_when_ssh_fails(self, mock_run): | ||||||
| """HTTPS fallback is used for generic hosts when SSH ls-remote fails.""" | ||||||
| from apm_cli.commands.install import _validate_package_exists | ||||||
|
|
||||||
| # SSH probe fails, HTTPS succeeds | ||||||
| mock_run.side_effect = [ | ||||||
| self._make_completed_process(returncode=128, stderr="ssh: connect to host"), | ||||||
| self._make_completed_process(returncode=0), | ||||||
| ] | ||||||
|
|
||||||
| result = _validate_package_exists( | ||||||
| "git@git.example.org:org/group/repo.git", verbose=False | ||||||
| ) | ||||||
|
|
||||||
| assert result is True | ||||||
| assert mock_run.call_count == 2 | ||||||
| # First call: SSH | ||||||
| first_cmd = mock_run.call_args_list[0][0][0] | ||||||
| assert any("git@git.example.org" in arg for arg in first_cmd), ( | ||||||
| f"Expected SSH URL in first call, got: {first_cmd}" | ||||||
| ) | ||||||
| # Second call: HTTPS | ||||||
| second_cmd = mock_run.call_args_list[1][0][0] | ||||||
| assert any("https://git.example.org" in arg for arg in second_cmd), ( | ||||||
|
sergio-sisternes-epam marked this conversation as resolved.
Dismissed
|
||||||
| f"Expected HTTPS URL in second call, got: {second_cmd}" | ||||||
| ) | ||||||
|
|
||||||
| @patch("subprocess.run") | ||||||
| def test_generic_host_returns_false_when_both_transports_fail(self, mock_run): | ||||||
| """Validation returns False when both SSH and HTTPS fail for a generic host.""" | ||||||
| from apm_cli.commands.install import _validate_package_exists | ||||||
|
|
||||||
| mock_run.return_value = self._make_completed_process( | ||||||
| returncode=128, stderr="fatal: could not read Username" | ||||||
| ) | ||||||
|
|
||||||
| result = _validate_package_exists( | ||||||
| "git@git.example.org:org/group/repo.git", verbose=False | ||||||
| ) | ||||||
|
|
||||||
| assert result is False | ||||||
| assert mock_run.call_count == 2 # tried SSH then HTTPS | ||||||
|
|
||||||
| @patch("subprocess.run") | ||||||
| def test_github_host_skips_ssh_attempt(self, mock_run): | ||||||
| """GitHub.com repositories do NOT go through the SSH-first ls-remote path.""" | ||||||
|
|
||||||
| import urllib.request | ||||||
| import urllib.error | ||||||
|
|
||||||
| from apm_cli.commands.install import _validate_package_exists | ||||||
|
|
||||||
| with patch("urllib.request.urlopen") as mock_urlopen: | ||||||
| mock_urlopen.side_effect = urllib.error.HTTPError( | ||||||
| url="https://api.github.com/repos/owner/repo", | ||||||
| code=404, msg="Not Found", hdrs={}, fp=None, | ||||||
| ) | ||||||
| result = _validate_package_exists("owner/repo", verbose=False) | ||||||
|
|
||||||
| assert result is False | ||||||
| # No ls-remote call should have been made for a github.com host | ||||||
| ls_remote_calls = [ | ||||||
| call for call in mock_run.call_args_list | ||||||
| if "ls-remote" in (call[0][0] if call[0] else []) | ||||||
| ] | ||||||
| assert len(ls_remote_calls) == 0, ( | ||||||
| f"Expected no ls-remote calls for github.com, got: {ls_remote_calls}" | ||||||
| ) | ||||||
|
|
||||||
| @patch("subprocess.run") | ||||||
| def test_ghes_host_skips_ssh_attempt(self, mock_run): | ||||||
| """A GHES host is treated as GitHub, not generic SSH probe is skipped.""" | ||||||
|
||||||
| """A GHES host is treated as GitHub, not generic SSH probe is skipped.""" | |
| """GHES hosts are treated as GitHub hosts and skip the generic SSH-first probe.""" |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the generic-host path,
validate_envintentionally omitsGIT_ASKPASS/GIT_CONFIG_GLOBAL(see above). Later in this function, the verbose stderr sanitization doesstderr_snippet.replace(validate_env.get(env_var, ""), "***"); when the key is missing this replaces the empty string, producing garbled output. Guard the replacement so it only runs when the env var value is a non-empty string (or use token/url sanitizers instead of env var substitution).