From 0b8cc1930e733baa1c39c9c14c53cbb7964fee3e Mon Sep 17 00:00:00 2001 From: "ENABLON\\thomas.caudal" Date: Fri, 20 Mar 2026 13:55:09 +0100 Subject: [PATCH 1/5] fix(auth): avoid credential helper false negatives on interactive login --- .../docs/getting-started/authentication.md | 2 ++ src/apm_cli/core/token_manager.py | 31 +++++++++++++++- tests/test_token_manager.py | 36 +++++++++++++++++++ 3 files changed, 68 insertions(+), 1 deletion(-) diff --git a/docs/src/content/docs/getting-started/authentication.md b/docs/src/content/docs/getting-started/authentication.md index 559c757a..76418597 100644 --- a/docs/src/content/docs/getting-started/authentication.md +++ b/docs/src/content/docs/getting-started/authentication.md @@ -21,6 +21,8 @@ When APM has a token for a recognized host (GitHub.com, GitHub Enterprise under For single-file downloads from GitHub (which use the GitHub API rather than `git clone`), APM also queries `git credential fill` as a last-resort fallback when no token environment variable is set. This means credentials stored by `gh auth login` or your OS keychain work for both folder-level and file-level dependencies. +`git credential fill` can trigger interactive account selection on some systems. APM waits up to 60 seconds by default for this step. If your credential helper is slower, set `APM_GIT_CREDENTIAL_TIMEOUT` (seconds, max 180). + ### Object-style `git:` references The `git:` object form in `apm.yml` lets you reference any git URL explicitly — HTTPS, SSH, or any host: diff --git a/src/apm_cli/core/token_manager.py b/src/apm_cli/core/token_manager.py index 4da53b79..b3d0594c 100644 --- a/src/apm_cli/core/token_manager.py +++ b/src/apm_cli/core/token_manager.py @@ -25,6 +25,11 @@ class GitHubTokenManager: """Manages GitHub token environment setup for different AI runtimes.""" + + # `git credential fill` may invoke OS credential helpers (and account pickers + # on Windows). Give it enough time by default to avoid false negatives. + DEFAULT_GIT_CREDENTIAL_TIMEOUT_SECONDS = 60 + MAX_GIT_CREDENTIAL_TIMEOUT_SECONDS = 180 # Define token precedence for different use cases TOKEN_PRECEDENCE = { @@ -69,6 +74,28 @@ def _is_valid_credential_token(token: str) -> bool: return False return True + @classmethod + def _get_git_credential_timeout_seconds(cls) -> int: + """Get timeout for `git credential fill` from env or default. + + Environment override: + - APM_GIT_CREDENTIAL_TIMEOUT: integer seconds (1..120) + """ + raw = os.environ.get("APM_GIT_CREDENTIAL_TIMEOUT", "").strip() + if not raw: + return cls.DEFAULT_GIT_CREDENTIAL_TIMEOUT_SECONDS + + try: + parsed = int(raw) + except ValueError: + return cls.DEFAULT_GIT_CREDENTIAL_TIMEOUT_SECONDS + + if parsed < 1: + return cls.DEFAULT_GIT_CREDENTIAL_TIMEOUT_SECONDS + if parsed > cls.MAX_GIT_CREDENTIAL_TIMEOUT_SECONDS: + return cls.MAX_GIT_CREDENTIAL_TIMEOUT_SECONDS + return parsed + @staticmethod def resolve_credential_from_git(host: str) -> Optional[str]: """Resolve a credential from the git credential store. @@ -83,13 +110,15 @@ def resolve_credential_from_git(host: str) -> Optional[str]: Returns: The password/token from the credential store, or None if unavailable """ + timeout_seconds = GitHubTokenManager._get_git_credential_timeout_seconds() + try: result = subprocess.run( ['git', 'credential', 'fill'], input=f"protocol=https\nhost={host}\n\n", capture_output=True, text=True, - timeout=5, + timeout=timeout_seconds, env={**os.environ, 'GIT_TERMINAL_PROMPT': '0', 'GIT_ASKPASS': ''}, ) if result.returncode != 0: diff --git a/tests/test_token_manager.py b/tests/test_token_manager.py index 3b21a2e2..5dd957ba 100644 --- a/tests/test_token_manager.py +++ b/tests/test_token_manager.py @@ -204,6 +204,42 @@ def test_accepts_valid_gho_token(self): token = GitHubTokenManager.resolve_credential_from_git('github.com') assert token == 'gho_abc123def456' + def test_uses_default_credential_timeout(self): + """Uses default timeout when APM_GIT_CREDENTIAL_TIMEOUT is not set.""" + mock_result = MagicMock(returncode=0, stdout="password=tok\n") + with patch.dict(os.environ, {}, clear=True), patch( + 'subprocess.run', return_value=mock_result + ) as mock_run: + GitHubTokenManager.resolve_credential_from_git('github.com') + assert mock_run.call_args.kwargs['timeout'] == 60 + + def test_uses_custom_credential_timeout_from_env(self): + """Uses configured timeout from APM_GIT_CREDENTIAL_TIMEOUT.""" + mock_result = MagicMock(returncode=0, stdout="password=tok\n") + with patch.dict(os.environ, {'APM_GIT_CREDENTIAL_TIMEOUT': '60'}, clear=True), patch( + 'subprocess.run', return_value=mock_result + ) as mock_run: + GitHubTokenManager.resolve_credential_from_git('github.com') + assert mock_run.call_args.kwargs['timeout'] == 60 + + def test_invalid_timeout_env_falls_back_to_default(self): + """Invalid timeout env values should not break credential resolution.""" + mock_result = MagicMock(returncode=0, stdout="password=tok\n") + with patch.dict(os.environ, {'APM_GIT_CREDENTIAL_TIMEOUT': 'not-a-number'}, clear=True), patch( + 'subprocess.run', return_value=mock_result + ) as mock_run: + GitHubTokenManager.resolve_credential_from_git('github.com') + assert mock_run.call_args.kwargs['timeout'] == 60 + + def test_timeout_env_is_clamped_to_max(self): + """Very large timeout values are clamped for safety.""" + mock_result = MagicMock(returncode=0, stdout="password=tok\n") + with patch.dict(os.environ, {'APM_GIT_CREDENTIAL_TIMEOUT': '999'}, clear=True), patch( + 'subprocess.run', return_value=mock_result + ) as mock_run: + GitHubTokenManager.resolve_credential_from_git('github.com') + assert mock_run.call_args.kwargs['timeout'] == 180 + class TestIsValidCredentialToken: """Test _is_valid_credential_token validation.""" From 5bc8fa02dc6f4e1d8f89d2dfcc423e3a2ab40890 Mon Sep 17 00:00:00 2001 From: "ENABLON\\thomas.caudal" Date: Fri, 20 Mar 2026 14:10:02 +0100 Subject: [PATCH 2/5] fix(auth): deduplicate credential helper invocations per process Use a class-level shared cache keyed by host so that multiple GitHubTokenManager instances created within a single command (e.g. validation phase + install phase) only invoke the git credential helper once per host instead of once per instance. --- .../docs/getting-started/authentication.md | 2 ++ src/apm_cli/core/token_manager.py | 25 +++++++++++++------ tests/test_token_manager.py | 25 +++++++++++++++++++ 3 files changed, 45 insertions(+), 7 deletions(-) diff --git a/docs/src/content/docs/getting-started/authentication.md b/docs/src/content/docs/getting-started/authentication.md index 76418597..8215a982 100644 --- a/docs/src/content/docs/getting-started/authentication.md +++ b/docs/src/content/docs/getting-started/authentication.md @@ -23,6 +23,8 @@ For single-file downloads from GitHub (which use the GitHub API rather than `git `git credential fill` can trigger interactive account selection on some systems. APM waits up to 60 seconds by default for this step. If your credential helper is slower, set `APM_GIT_CREDENTIAL_TIMEOUT` (seconds, max 180). +Within a single APM command, credential-helper lookups are cached per host. This avoids repeated account-selection prompts when multiple install phases need authentication in the same run. + ### Object-style `git:` references The `git:` object form in `apm.yml` lets you reference any git URL explicitly — HTTPS, SSH, or any host: diff --git a/src/apm_cli/core/token_manager.py b/src/apm_cli/core/token_manager.py index b3d0594c..c44a2cb0 100644 --- a/src/apm_cli/core/token_manager.py +++ b/src/apm_cli/core/token_manager.py @@ -20,6 +20,7 @@ import os import subprocess +import threading from typing import Dict, Optional, Tuple @@ -30,6 +31,11 @@ class GitHubTokenManager: # on Windows). Give it enough time by default to avoid false negatives. DEFAULT_GIT_CREDENTIAL_TIMEOUT_SECONDS = 60 MAX_GIT_CREDENTIAL_TIMEOUT_SECONDS = 180 + + # Shared process-wide cache so multiple downloader/token-manager instances + # in a single command do not repeatedly invoke credential helpers. + _SHARED_CREDENTIAL_CACHE: Dict[str, Optional[str]] = {} + _SHARED_CREDENTIAL_CACHE_LOCK = threading.Lock() # Define token precedence for different use cases TOKEN_PRECEDENCE = { @@ -54,7 +60,7 @@ def __init__(self, preserve_existing: bool = True): preserve_existing: If True, never overwrite existing environment variables """ self.preserve_existing = preserve_existing - self._credential_cache: Dict[str, Optional[str]] = {} + self._credential_cache = self._SHARED_CREDENTIAL_CACHE @staticmethod def _is_valid_credential_token(token: str) -> bool: @@ -196,13 +202,18 @@ def get_token_with_credential_fallback(self, purpose: str, host: str, env: Optio token = self.get_token_for_purpose(purpose, env) if token: return token - - if host in self._credential_cache: - return self._credential_cache[host] - + + with self._SHARED_CREDENTIAL_CACHE_LOCK: + if host in self._credential_cache: + return self._credential_cache[host] + credential = self.resolve_credential_from_git(host) - self._credential_cache[host] = credential - return credential + + with self._SHARED_CREDENTIAL_CACHE_LOCK: + # Keep first computed value if another thread filled cache first. + if host not in self._credential_cache: + self._credential_cache[host] = credential + return self._credential_cache[host] def validate_tokens(self, env: Optional[Dict[str, str]] = None) -> Tuple[bool, str]: """Validate that required tokens are available. diff --git a/tests/test_token_manager.py b/tests/test_token_manager.py index 5dd957ba..3cde926e 100644 --- a/tests/test_token_manager.py +++ b/tests/test_token_manager.py @@ -9,6 +9,14 @@ from src.apm_cli.core.token_manager import GitHubTokenManager +@pytest.fixture(autouse=True) +def clear_shared_credential_cache(): + """Ensure shared credential cache is isolated between tests.""" + GitHubTokenManager._SHARED_CREDENTIAL_CACHE.clear() + yield + GitHubTokenManager._SHARED_CREDENTIAL_CACHE.clear() + + class TestModulesTokenPrecedence: """Test GH_TOKEN addition to the modules token precedence chain.""" @@ -342,3 +350,20 @@ def test_different_hosts_separate_cache(self): assert tok1 == 'tok-github.com' assert tok2 == 'tok-gitlab.com' assert mock_cred.call_count == 2 + + def test_cache_is_shared_across_instances(self): + """Credential helper should be queried once per host per process.""" + with patch.dict(os.environ, {}, clear=True): + first = GitHubTokenManager() + second = GitHubTokenManager() + + with patch.object( + GitHubTokenManager, + 'resolve_credential_from_git', + return_value='shared-token', + ) as mock_cred: + tok1 = first.get_token_with_credential_fallback('modules', 'github.com') + tok2 = second.get_token_with_credential_fallback('modules', 'github.com') + assert tok1 == 'shared-token' + assert tok2 == 'shared-token' + mock_cred.assert_called_once() From 3eeb697c5fa55128c822697c210bdf67d0f8d0b8 Mon Sep 17 00:00:00 2001 From: Thomas Caudal Date: Fri, 20 Mar 2026 16:36:46 +0100 Subject: [PATCH 3/5] Update src/apm_cli/core/token_manager.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- src/apm_cli/core/token_manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/apm_cli/core/token_manager.py b/src/apm_cli/core/token_manager.py index c44a2cb0..10837948 100644 --- a/src/apm_cli/core/token_manager.py +++ b/src/apm_cli/core/token_manager.py @@ -85,7 +85,7 @@ def _get_git_credential_timeout_seconds(cls) -> int: """Get timeout for `git credential fill` from env or default. Environment override: - - APM_GIT_CREDENTIAL_TIMEOUT: integer seconds (1..120) + - APM_GIT_CREDENTIAL_TIMEOUT: integer seconds (1..180) """ raw = os.environ.get("APM_GIT_CREDENTIAL_TIMEOUT", "").strip() if not raw: From cacac79a8ceacb01e068ccdd460a589b6a68d1ea Mon Sep 17 00:00:00 2001 From: Thomas Caudal Date: Fri, 20 Mar 2026 16:40:12 +0100 Subject: [PATCH 4/5] Update tests/test_token_manager.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- tests/test_token_manager.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_token_manager.py b/tests/test_token_manager.py index 3cde926e..1d1c9a92 100644 --- a/tests/test_token_manager.py +++ b/tests/test_token_manager.py @@ -224,11 +224,11 @@ def test_uses_default_credential_timeout(self): def test_uses_custom_credential_timeout_from_env(self): """Uses configured timeout from APM_GIT_CREDENTIAL_TIMEOUT.""" mock_result = MagicMock(returncode=0, stdout="password=tok\n") - with patch.dict(os.environ, {'APM_GIT_CREDENTIAL_TIMEOUT': '60'}, clear=True), patch( + with patch.dict(os.environ, {'APM_GIT_CREDENTIAL_TIMEOUT': '42'}, clear=True), patch( 'subprocess.run', return_value=mock_result ) as mock_run: GitHubTokenManager.resolve_credential_from_git('github.com') - assert mock_run.call_args.kwargs['timeout'] == 60 + assert mock_run.call_args.kwargs['timeout'] == 42 def test_invalid_timeout_env_falls_back_to_default(self): """Invalid timeout env values should not break credential resolution.""" From 696798fec5e6cc941c7e569cea2f509b124b0c02 Mon Sep 17 00:00:00 2001 From: Thomas Caudal Date: Fri, 20 Mar 2026 16:40:29 +0100 Subject: [PATCH 5/5] Update tests/test_token_manager.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- tests/test_token_manager.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/test_token_manager.py b/tests/test_token_manager.py index 1d1c9a92..af5ad299 100644 --- a/tests/test_token_manager.py +++ b/tests/test_token_manager.py @@ -12,9 +12,11 @@ @pytest.fixture(autouse=True) def clear_shared_credential_cache(): """Ensure shared credential cache is isolated between tests.""" - GitHubTokenManager._SHARED_CREDENTIAL_CACHE.clear() + with GitHubTokenManager._SHARED_CREDENTIAL_CACHE_LOCK: + GitHubTokenManager._SHARED_CREDENTIAL_CACHE.clear() yield - GitHubTokenManager._SHARED_CREDENTIAL_CACHE.clear() + with GitHubTokenManager._SHARED_CREDENTIAL_CACHE_LOCK: + GitHubTokenManager._SHARED_CREDENTIAL_CACHE.clear() class TestModulesTokenPrecedence: