From 9965161112e265ce4c0ee2df4235730136e3be43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Moreau?= Date: Wed, 8 Apr 2026 15:39:12 -0400 Subject: [PATCH 1/2] Disambiguate GitHub auth fallback resolution --- .../docs/getting-started/authentication.md | 10 +- .../.apm/skills/apm-usage/authentication.md | 5 +- src/apm_cli/commands/install.py | 2 + src/apm_cli/core/auth.py | 48 +++++-- src/apm_cli/core/token_manager.py | 80 +++++++++-- src/apm_cli/marketplace/client.py | 1 + tests/test_token_manager.py | 136 +++++++++++++++--- tests/unit/test_auth.py | 21 ++- 8 files changed, 256 insertions(+), 47 deletions(-) diff --git a/docs/src/content/docs/getting-started/authentication.md b/docs/src/content/docs/getting-started/authentication.md index 6ba4b733..6140f62f 100644 --- a/docs/src/content/docs/getting-started/authentication.md +++ b/docs/src/content/docs/getting-started/authentication.md @@ -8,13 +8,14 @@ APM works without tokens for public packages on github.com. Authentication is ne ## How APM resolves authentication -APM resolves tokens per `(host, org)` pair. For each dependency, it walks a resolution chain until it finds a token: +APM resolves tokens per `(host, org)` pair, and includes repo-path context when available for credential-helper lookups. For each dependency, it walks a resolution chain until it finds a token: 1. **Per-org env var** — `GITHUB_APM_PAT_{ORG}` (GitHub-like hosts — not ADO) 2. **Global env vars** — `GITHUB_APM_PAT` → `GITHUB_TOKEN` → `GH_TOKEN` (any host) -3. **Git credential helper** — `git credential fill` (any host except ADO) +3. **GitHub CLI active account** — `gh auth token --hostname ` (GitHub-like hosts) +4. **Git credential helper** — `git credential fill` with repo-path context when available (any host except ADO) -If the global token doesn't work for the target host, APM automatically retries with git credential helpers. If nothing matches, APM attempts unauthenticated access (works for public repos on github.com). +If the global token doesn't work for the target host, APM next tries the active `gh` CLI account before falling back to git credential helpers. When APM knows the repository URL, it includes the repo path in the helper query to reduce ambiguous multi-account prompts on hosts like github.com. If nothing matches, APM attempts unauthenticated access (works for public repos on github.com). Results are cached per-process — the same `(host, org)` pair is resolved once. @@ -28,7 +29,8 @@ All token-bearing requests use HTTPS. Tokens are never sent over unencrypted con | 2 | `GITHUB_APM_PAT` | Any host | Falls back to git credential helpers if rejected | | 3 | `GITHUB_TOKEN` | Any host | Shared with GitHub Actions | | 4 | `GH_TOKEN` | Any host | Set by `gh auth login` | -| 5 | `git credential fill` | Per-host | System credential manager, `gh auth`, OS keychain | +| 5 | `gh auth token --hostname ` | GitHub-like hosts | Active `gh auth login` account | +| 6 | `git credential fill` | Per-host | System credential manager, `gh auth`, OS keychain | For Azure DevOps, the only token source is `ADO_APM_PAT`. diff --git a/packages/apm-guide/.apm/skills/apm-usage/authentication.md b/packages/apm-guide/.apm/skills/apm-usage/authentication.md index b5205e98..4ce58acc 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/authentication.md +++ b/packages/apm-guide/.apm/skills/apm-usage/authentication.md @@ -10,7 +10,10 @@ APM checks these sources in order, using the first valid token found: | 2 | `GITHUB_APM_PAT` | Global | Falls back to git credential if rejected | | 3 | `GITHUB_TOKEN` | Global | Shared with GitHub Actions | | 4 | `GH_TOKEN` | Global | Set by `gh auth login` | -| 5 | `git credential fill` | Per-host | System credential manager | +| 5 | `gh auth token --hostname ` | GitHub-like hosts | Active `gh auth login` account | +| 6 | `git credential fill` | Per-host and repo-path | System credential manager | + +When APM knows the repository URL, it includes the repo path in the credential-helper query. APM also checks the active `gh` CLI account before invoking OS credential helpers. This reduces ambiguous multi-account prompts on hosts like github.com. | -- | None | -- | Unauthenticated (public GitHub repos only) | ## Per-org setup diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index 09f57c6e..45834bcd 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -500,6 +500,7 @@ def _check_repo(token, git_env): return auth_resolver.try_with_fallback( host, _check_repo, org=org, + repo_path=f"{dep_ref.repo_url}.git", unauth_first=True, verbose_callback=verbose_log, ) @@ -549,6 +550,7 @@ def _check_repo_fallback(token, git_env): return auth_resolver.try_with_fallback( host, _check_repo_fallback, org=org, + repo_path=f"{repo_path}.git", unauth_first=True, verbose_callback=verbose_log, ) diff --git a/src/apm_cli/core/auth.py b/src/apm_cli/core/auth.py index fc18d4ca..9115203e 100644 --- a/src/apm_cli/core/auth.py +++ b/src/apm_cli/core/auth.py @@ -1,7 +1,8 @@ """Centralized authentication resolution for APM CLI. Every APM operation that touches a remote host MUST use AuthResolver. -Resolution is per-(host, org) pair, thread-safe, and cached per-process. +Resolution is per-(host, org, repo-path) tuple when repo context is known, +thread-safe, and cached per-process. All token-bearing requests use HTTPS — that is the transport security boundary. Global env vars are tried for every host; if the token is @@ -178,9 +179,18 @@ def detect_token_type(token: str) -> str: # -- core resolution ---------------------------------------------------- - def resolve(self, host: str, org: Optional[str] = None) -> AuthContext: - """Resolve auth for *(host, org)*. Cached & thread-safe.""" - key = (host.lower() if host else host, org.lower() if org else org) + def resolve( + self, + host: str, + org: Optional[str] = None, + repo_path: Optional[str] = None, + ) -> AuthContext: + """Resolve auth for *(host, org, repo_path)*. Cached & thread-safe.""" + key = ( + host.lower() if host else host, + org.lower() if org else org, + repo_path, + ) with self._lock: cached = self._cache.get(key) if cached is not None: @@ -193,7 +203,7 @@ def resolve(self, host: str, org: Optional[str] = None) -> AuthContext: # Bounded by APM_GIT_CREDENTIAL_TIMEOUT (default 60s). No deadlock # risk: single lock, never nested. host_info = self.classify_host(host) - token, source = self._resolve_token(host_info, org) + token, source = self._resolve_token(host_info, org, repo_path=repo_path) token_type = self.detect_token_type(token) if token else "unknown" git_env = self._build_git_env(token) @@ -211,11 +221,13 @@ def resolve_for_dep(self, dep_ref: "DependencyReference") -> AuthContext: """Resolve auth from a ``DependencyReference``.""" host = dep_ref.host or default_host() org: Optional[str] = None + repo_path: Optional[str] = None if dep_ref.repo_url: parts = dep_ref.repo_url.split("/") if parts: org = parts[0] - return self.resolve(host, org) + repo_path = f"{dep_ref.repo_url}.git" + return self.resolve(host, org, repo_path=repo_path) # -- fallback strategy -------------------------------------------------- @@ -225,6 +237,7 @@ def try_with_fallback( operation: Callable[..., T], *, org: Optional[str] = None, + repo_path: Optional[str] = None, unauth_first: bool = False, verbose_callback: Optional[Callable[[str], None]] = None, ) -> T: @@ -247,7 +260,7 @@ def try_with_fallback( (e.g. a github.com PAT tried on ``*.ghe.com``), the method retries with ``git credential fill`` before giving up. """ - auth_ctx = self.resolve(host, org) + auth_ctx = self.resolve(host, org, repo_path=repo_path) host_info = auth_ctx.host_info git_env = auth_ctx.git_env @@ -261,8 +274,11 @@ def _try_credential_fallback(exc: Exception) -> T: raise exc if host_info.kind == "ado": raise exc - _log(f"Token from {auth_ctx.source} failed, trying git credential fill for {host}") - cred = self._token_manager.resolve_credential_from_git(host) + _log(f"Token from {auth_ctx.source} failed, trying fallback credentials for {host}") + gh_token = self._token_manager.resolve_credential_from_gh_cli(host) + if gh_token: + return operation(gh_token, self._build_git_env(gh_token)) + cred = self._token_manager.resolve_credential_from_git(host, path=repo_path) if cred: return operation(cred, self._build_git_env(cred)) raise exc @@ -353,7 +369,7 @@ def build_error_context( # -- internals ---------------------------------------------------------- def _resolve_token( - self, host_info: HostInfo, org: Optional[str] + self, host_info: HostInfo, org: Optional[str], repo_path: Optional[str] = None ) -> tuple[Optional[str], str]: """Walk the token resolution chain. Returns (token, source). @@ -382,9 +398,17 @@ def _resolve_token( source = self._identify_env_source(purpose) return token, source - # 3. Git credential helper (not for ADO — uses its own PAT) + # 3. gh CLI active account (GitHub-like hosts only) + gh_token = self._token_manager.resolve_credential_from_gh_cli(host_info.host) + if gh_token: + return gh_token, "gh-auth-token" + + # 4. Git credential helper (not for ADO — uses its own PAT) if host_info.kind not in ("ado",): - credential = self._token_manager.resolve_credential_from_git(host_info.host) + credential = self._token_manager.resolve_credential_from_git( + host_info.host, + path=repo_path, + ) if credential: return credential, "git-credential-fill" diff --git a/src/apm_cli/core/token_manager.py b/src/apm_cli/core/token_manager.py index 93afdd60..6c39011a 100644 --- a/src/apm_cli/core/token_manager.py +++ b/src/apm_cli/core/token_manager.py @@ -11,7 +11,7 @@ - GITHUB_TOKEN: User-scoped PAT for GitHub Models API access Platform Token Selection: -- GitHub: GITHUB_APM_PAT -> GITHUB_TOKEN -> GH_TOKEN -> git credential helpers +- GitHub: GITHUB_APM_PAT -> GITHUB_TOKEN -> GH_TOKEN -> gh auth token -> git credential helpers - Azure DevOps: ADO_APM_PAT Runtime Requirements: @@ -92,7 +92,11 @@ def _get_credential_timeout(cls) -> int: return max(1, min(val, cls.MAX_CREDENTIAL_TIMEOUT)) @staticmethod - def resolve_credential_from_git(host: str) -> Optional[str]: + def resolve_credential_from_git( + host: str, + path: Optional[str] = None, + username: Optional[str] = None, + ) -> Optional[str]: """Resolve a credential from the git credential store. Uses `git credential fill` to query the user's configured credential @@ -101,14 +105,26 @@ def resolve_credential_from_git(host: str) -> Optional[str]: Args: host: The git host to resolve credentials for (e.g., "github.com") + path: Optional repository path (e.g., "owner/repo.git") used to + disambiguate multi-account credential helpers. + username: Optional username hint for credential helpers. Returns: The password/token from the credential store, or None if unavailable """ try: + request_lines = ['protocol=https', f'host={host}'] + if path: + normalized_path = path.lstrip('/') + if normalized_path: + request_lines.append(f'path={normalized_path}') + if username: + request_lines.append(f'username={username}') + request = '\n'.join(request_lines) + '\n\n' + result = subprocess.run( - ['git', 'credential', 'fill'], - input=f"protocol=https\nhost={host}\n\n", + ['git', '-c', 'credential.useHttpPath=true', 'credential', 'fill'], + input=request, capture_output=True, text=True, encoding="utf-8", @@ -128,6 +144,32 @@ def resolve_credential_from_git(host: str) -> Optional[str]: return None except (subprocess.TimeoutExpired, FileNotFoundError, OSError): return None + + @staticmethod + def resolve_credential_from_gh_cli(host: str) -> Optional[str]: + """Resolve a token from the active gh CLI account for the host. + + Uses `gh auth token --hostname ` as a non-interactive fallback + before invoking OS credential helpers that may display UI. + """ + try: + result = subprocess.run( + ['gh', 'auth', 'token', '--hostname', host], + capture_output=True, + text=True, + encoding='utf-8', + timeout=GitHubTokenManager._get_credential_timeout(), + env={**os.environ, 'GH_PROMPT_DISABLED': '1'}, + ) + if result.returncode != 0: + return None + + token = result.stdout.strip() + if token and GitHubTokenManager._is_valid_credential_token(token): + return token + return None + except (subprocess.TimeoutExpired, FileNotFoundError, OSError): + return None def setup_environment(self, env: Optional[Dict[str, str]] = None) -> Dict[str, str]: """Set up complete token environment for all runtimes. @@ -173,16 +215,26 @@ def get_token_for_purpose(self, purpose: str, env: Optional[Dict[str, str]] = No return token return None - def get_token_with_credential_fallback(self, purpose: str, host: str, env: Optional[Dict[str, str]] = None) -> Optional[str]: + def get_token_with_credential_fallback( + self, + purpose: str, + host: str, + path: Optional[str] = None, + username: Optional[str] = None, + env: Optional[Dict[str, str]] = None, + ) -> Optional[str]: """Get token for a purpose, falling back to git credential helpers. Tries environment variables first (via get_token_for_purpose), then - queries the git credential store as a last resort. Results are cached - per host to avoid repeated subprocess calls. + checks the active gh CLI account, then queries the git credential + store as a last resort. Results are cached per host to avoid repeated + subprocess calls. Args: purpose: Token purpose ('modules', etc.) host: Git host to resolve credentials for (e.g., "github.com") + path: Optional repository path for credential-helper disambiguation + username: Optional username hint for credential helpers env: Environment to check (defaults to os.environ) Returns: @@ -192,11 +244,17 @@ def get_token_with_credential_fallback(self, purpose: str, host: str, env: Optio if token: return token - if host in self._credential_cache: - return self._credential_cache[host] + cache_key = (host, path or '', username or '') + if cache_key in self._credential_cache: + return self._credential_cache[cache_key] + + gh_token = self.resolve_credential_from_gh_cli(host) + if gh_token: + self._credential_cache[cache_key] = gh_token + return gh_token - credential = self.resolve_credential_from_git(host) - self._credential_cache[host] = credential + credential = self.resolve_credential_from_git(host, path=path, username=username) + self._credential_cache[cache_key] = credential return credential def validate_tokens(self, env: Optional[Dict[str, str]] = None) -> Tuple[bool, str]: diff --git a/src/apm_cli/marketplace/client.py b/src/apm_cli/marketplace/client.py index cee2d6eb..de60c877 100644 --- a/src/apm_cli/marketplace/client.py +++ b/src/apm_cli/marketplace/client.py @@ -170,6 +170,7 @@ def _do_fetch(token, _git_env): source.host, _do_fetch, org=source.owner, + repo_path=f"{source.owner}/{source.repo}.git", unauth_first=True, ) except Exception as exc: diff --git a/tests/test_token_manager.py b/tests/test_token_manager.py index 1ddb02cb..9c87a16d 100644 --- a/tests/test_token_manager.py +++ b/tests/test_token_manager.py @@ -2,6 +2,7 @@ import os import subprocess +import sys from unittest.mock import patch, MagicMock import pytest @@ -122,6 +123,24 @@ def test_correct_input_sent(self): GitHubTokenManager.resolve_credential_from_git('github.com') call_kwargs = mock_run.call_args assert call_kwargs.kwargs['input'] == "protocol=https\nhost=github.com\n\n" + assert call_kwargs.args[0] == ['git', '-c', 'credential.useHttpPath=true', 'credential', 'fill'] + + def test_path_and_username_are_sent_when_provided(self): + """Verifies repo path and username are sent to disambiguate helpers.""" + mock_result = MagicMock(returncode=0, stdout="password=tok\n") + with patch('subprocess.run', return_value=mock_result) as mock_run: + GitHubTokenManager.resolve_credential_from_git( + 'github.com', + path='Devolutions/RDM.git', + username='mamoreau-devolutions', + ) + call_kwargs = mock_run.call_args + assert call_kwargs.kwargs['input'] == ( + "protocol=https\n" + "host=github.com\n" + "path=Devolutions/RDM.git\n" + "username=mamoreau-devolutions\n\n" + ) def test_git_terminal_prompt_disabled(self): """GIT_TERMINAL_PROMPT=0 is set in the subprocess env.""" @@ -137,7 +156,8 @@ def test_git_askpass_set_to_empty(self): with patch('subprocess.run', return_value=mock_result) as mock_run: GitHubTokenManager.resolve_credential_from_git('github.com') call_env = mock_run.call_args.kwargs['env'] - assert call_env['GIT_ASKPASS'] == '' + expected = 'echo' if sys.platform == 'win32' else '' + assert call_env['GIT_ASKPASS'] == expected def test_rejects_password_prompt_as_token(self): """Rejects 'Password for ...' prompt text echoed back by GIT_ASKPASS.""" @@ -205,6 +225,32 @@ def test_accepts_valid_gho_token(self): assert token == 'gho_abc123def456' +class TestResolveCredentialFromGhCli: + """Test resolve_credential_from_gh_cli static method.""" + + def test_success_returns_token(self): + mock_result = MagicMock(returncode=0, stdout="gho_cli_token\n") + with patch('subprocess.run', return_value=mock_result) as mock_run: + token = GitHubTokenManager.resolve_credential_from_gh_cli('github.com') + assert token == 'gho_cli_token' + assert mock_run.call_args.args[0] == ['gh', 'auth', 'token', '--hostname', 'github.com'] + assert mock_run.call_args.kwargs['env']['GH_PROMPT_DISABLED'] == '1' + + def test_nonzero_exit_returns_none(self): + mock_result = MagicMock(returncode=1, stdout="", stderr="not logged in") + with patch('subprocess.run', return_value=mock_result): + assert GitHubTokenManager.resolve_credential_from_gh_cli('github.com') is None + + def test_invalid_output_returns_none(self): + mock_result = MagicMock(returncode=0, stdout="Username for 'https://github.com':\n") + with patch('subprocess.run', return_value=mock_result): + assert GitHubTokenManager.resolve_credential_from_gh_cli('github.com') is None + + def test_timeout_returns_none(self): + with patch('subprocess.run', side_effect=subprocess.TimeoutExpired(cmd='gh', timeout=5)): + assert GitHubTokenManager.resolve_credential_from_gh_cli('github.com') is None + + class TestCredentialTimeout: """Tests for configurable git credential fill timeout.""" @@ -282,58 +328,112 @@ def test_returns_env_token_without_credential_fill(self): """Returns env var token and never calls credential fill.""" with patch.dict(os.environ, {'GITHUB_APM_PAT': 'env-token'}, clear=True): manager = GitHubTokenManager() - with patch.object(GitHubTokenManager, 'resolve_credential_from_git') as mock_cred: + with patch.object(GitHubTokenManager, 'resolve_credential_from_gh_cli') as mock_gh, \ + patch.object(GitHubTokenManager, 'resolve_credential_from_git') as mock_cred: token = manager.get_token_with_credential_fallback('modules', 'github.com') assert token == 'env-token' + mock_gh.assert_not_called() + mock_cred.assert_not_called() + + def test_falls_back_to_gh_cli_before_credential_fill(self): + """Uses gh CLI before git credential helpers when no env token exists.""" + with patch.dict(os.environ, {}, clear=True): + manager = GitHubTokenManager() + with patch.object(GitHubTokenManager, 'resolve_credential_from_gh_cli', return_value='gh-token') as mock_gh, \ + patch.object(GitHubTokenManager, 'resolve_credential_from_git') as mock_cred: + token = manager.get_token_with_credential_fallback('modules', 'github.com') + assert token == 'gh-token' + mock_gh.assert_called_once_with('github.com') mock_cred.assert_not_called() def test_falls_back_to_credential_fill(self): - """Falls back to resolve_credential_from_git when no env token.""" + """Falls back to resolve_credential_from_git when gh CLI has no token.""" with patch.dict(os.environ, {}, clear=True): manager = GitHubTokenManager() - with patch.object( - GitHubTokenManager, 'resolve_credential_from_git', return_value='cred-token' - ) as mock_cred: + with patch.object(GitHubTokenManager, 'resolve_credential_from_gh_cli', return_value=None) as mock_gh, \ + patch.object(GitHubTokenManager, 'resolve_credential_from_git', return_value='cred-token') as mock_cred: token = manager.get_token_with_credential_fallback('modules', 'github.com') assert token == 'cred-token' - mock_cred.assert_called_once_with('github.com') + mock_gh.assert_called_once_with('github.com') + mock_cred.assert_called_once_with('github.com', path=None, username=None) + + def test_falls_back_to_credential_fill_with_path_context(self): + """Repo path is threaded into credential-helper fallback and cache key.""" + with patch.dict(os.environ, {}, clear=True): + manager = GitHubTokenManager() + with patch.object(GitHubTokenManager, 'resolve_credential_from_gh_cli', return_value=None), \ + patch.object(GitHubTokenManager, 'resolve_credential_from_git', return_value='cred-token') as mock_cred: + token = manager.get_token_with_credential_fallback( + 'modules', + 'github.com', + path='Devolutions/RDM.git', + ) + assert token == 'cred-token' + mock_cred.assert_called_once_with( + 'github.com', + path='Devolutions/RDM.git', + username=None, + ) def test_caches_credential_result(self): """Second call uses cache, subprocess not invoked again.""" with patch.dict(os.environ, {}, clear=True): manager = GitHubTokenManager() - with patch.object( - GitHubTokenManager, 'resolve_credential_from_git', return_value='cached-tok' - ) as mock_cred: + with patch.object(GitHubTokenManager, 'resolve_credential_from_gh_cli', return_value=None) as mock_gh, \ + patch.object(GitHubTokenManager, 'resolve_credential_from_git', return_value='cached-tok') as mock_cred: first = manager.get_token_with_credential_fallback('modules', 'github.com') second = manager.get_token_with_credential_fallback('modules', 'github.com') assert first == second == 'cached-tok' + mock_gh.assert_called_once_with('github.com') mock_cred.assert_called_once() def test_caches_none_results(self): """None results are cached to avoid retrying failed lookups.""" with patch.dict(os.environ, {}, clear=True): manager = GitHubTokenManager() - with patch.object( - GitHubTokenManager, 'resolve_credential_from_git', return_value=None - ) as mock_cred: + with patch.object(GitHubTokenManager, 'resolve_credential_from_gh_cli', return_value=None) as mock_gh, \ + patch.object(GitHubTokenManager, 'resolve_credential_from_git', return_value=None) as mock_cred: first = manager.get_token_with_credential_fallback('modules', 'github.com') second = manager.get_token_with_credential_fallback('modules', 'github.com') assert first is None assert second is None + mock_gh.assert_called_once_with('github.com') mock_cred.assert_called_once() def test_different_hosts_separate_cache(self): """Different hosts get independent cache entries.""" with patch.dict(os.environ, {}, clear=True): manager = GitHubTokenManager() - with patch.object( - GitHubTokenManager, - 'resolve_credential_from_git', - side_effect=lambda h: f'tok-{h}', - ) as mock_cred: + with patch.object(GitHubTokenManager, 'resolve_credential_from_gh_cli', return_value=None) as mock_gh, \ + patch.object( + GitHubTokenManager, + 'resolve_credential_from_git', + side_effect=lambda h, path=None, username=None: f'tok-{h}', + ) as mock_cred: tok1 = manager.get_token_with_credential_fallback('modules', 'github.com') tok2 = manager.get_token_with_credential_fallback('modules', 'gitlab.com') assert tok1 == 'tok-github.com' assert tok2 == 'tok-gitlab.com' + assert mock_gh.call_count == 2 + assert mock_cred.call_count == 2 + + def test_different_paths_separate_cache(self): + """Different repo paths should not share a credential-helper cache entry.""" + with patch.dict(os.environ, {}, clear=True): + manager = GitHubTokenManager() + with patch.object(GitHubTokenManager, 'resolve_credential_from_gh_cli', return_value=None) as mock_gh, \ + patch.object( + GitHubTokenManager, + 'resolve_credential_from_git', + side_effect=lambda host, path=None, username=None: f'{host}:{path}', + ) as mock_cred: + tok1 = manager.get_token_with_credential_fallback( + 'modules', 'github.com', path='Devolutions/RDM.git' + ) + tok2 = manager.get_token_with_credential_fallback( + 'modules', 'github.com', path='Devolutions/RDM-Mobile.git' + ) + assert tok1 == 'github.com:Devolutions/RDM.git' + assert tok2 == 'github.com:Devolutions/RDM-Mobile.git' + assert mock_gh.call_count == 2 assert mock_cred.call_count == 2 diff --git a/tests/unit/test_auth.py b/tests/unit/test_auth.py index ab63b3b2..32e5551b 100644 --- a/tests/unit/test_auth.py +++ b/tests/unit/test_auth.py @@ -9,6 +9,7 @@ from apm_cli.core.auth import AuthResolver, HostInfo, AuthContext from apm_cli.core.token_manager import GitHubTokenManager +from apm_cli.models.dependency.reference import DependencyReference # --------------------------------------------------------------------------- @@ -140,7 +141,7 @@ def test_caching_is_singleflight_under_concurrency(self): """Concurrent resolve() calls for the same key should populate cache once.""" resolver = AuthResolver() - def _slow_resolve_token(host_info, org): + def _slow_resolve_token(host_info, org, repo_path=None): time.sleep(0.05) return ("cred-token", "git-credential-fill") @@ -191,6 +192,24 @@ def test_credential_fallback(self): assert ctx.token == "cred-token" assert ctx.source == "git-credential-fill" + def test_resolve_for_dep_passes_repo_path_to_credential_fill(self): + """Dependency-aware resolution includes repo path for helper disambiguation.""" + dep_ref = DependencyReference.parse("Devolutions/RDM/.claude/skills/add-culture-rdm") + with patch.dict(os.environ, {}, clear=True): + with patch.object( + GitHubTokenManager, + "resolve_credential_from_git", + return_value="cred-token", + ) as mock_cred: + resolver = AuthResolver() + ctx = resolver.resolve_for_dep(dep_ref) + assert ctx.token == "cred-token" + assert ctx.source == "git-credential-fill" + mock_cred.assert_called_once_with( + "github.com", + path="Devolutions/RDM.git", + ) + def test_global_var_resolves_for_non_default_host(self): """GITHUB_APM_PAT resolves for *.ghe.com (any host, not just default).""" with patch.dict(os.environ, {"GITHUB_APM_PAT": "global-token"}, clear=True): From 9803b4ea461d612e884e06ba1076de411367f293 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Moreau?= Date: Wed, 8 Apr 2026 16:00:06 -0400 Subject: [PATCH 2/2] Address auth PR review comments --- .../docs/getting-started/authentication.md | 12 ++--- .../.apm/skills/apm-usage/authentication.md | 2 +- src/apm_cli/core/auth.py | 25 ++++++---- src/apm_cli/core/token_manager.py | 47 +++++++++++++++++-- tests/test_token_manager.py | 23 ++++++++- tests/unit/test_auth.py | 7 +++ 6 files changed, 95 insertions(+), 21 deletions(-) diff --git a/docs/src/content/docs/getting-started/authentication.md b/docs/src/content/docs/getting-started/authentication.md index 6140f62f..db75d674 100644 --- a/docs/src/content/docs/getting-started/authentication.md +++ b/docs/src/content/docs/getting-started/authentication.md @@ -8,16 +8,16 @@ APM works without tokens for public packages on github.com. Authentication is ne ## How APM resolves authentication -APM resolves tokens per `(host, org)` pair, and includes repo-path context when available for credential-helper lookups. For each dependency, it walks a resolution chain until it finds a token: +APM resolves tokens per `(host, org, repo_path)` tuple when repo context is known. It also includes repo-path context when available for credential-helper lookups. For each dependency, it walks a resolution chain until it finds a token: -1. **Per-org env var** — `GITHUB_APM_PAT_{ORG}` (GitHub-like hosts — not ADO) -2. **Global env vars** — `GITHUB_APM_PAT` → `GITHUB_TOKEN` → `GH_TOKEN` (any host) -3. **GitHub CLI active account** — `gh auth token --hostname ` (GitHub-like hosts) -4. **Git credential helper** — `git credential fill` with repo-path context when available (any host except ADO) +1. **Per-org env var** -- `GITHUB_APM_PAT_{ORG}` (GitHub-like hosts -- not ADO) +2. **Global env vars** -- `GITHUB_APM_PAT` -> `GITHUB_TOKEN` -> `GH_TOKEN` (any host) +3. **GitHub CLI active account** -- `gh auth token --hostname ` (GitHub-like hosts) +4. **Git credential helper** -- `git credential fill` with repo-path context when available (any host except ADO) If the global token doesn't work for the target host, APM next tries the active `gh` CLI account before falling back to git credential helpers. When APM knows the repository URL, it includes the repo path in the helper query to reduce ambiguous multi-account prompts on hosts like github.com. If nothing matches, APM attempts unauthenticated access (works for public repos on github.com). -Results are cached per-process — the same `(host, org)` pair is resolved once. +Results are cached per-process -- the same `(host, org, repo_path)` tuple is resolved once. All token-bearing requests use HTTPS. Tokens are never sent over unencrypted connections. diff --git a/packages/apm-guide/.apm/skills/apm-usage/authentication.md b/packages/apm-guide/.apm/skills/apm-usage/authentication.md index 4ce58acc..2b367c06 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/authentication.md +++ b/packages/apm-guide/.apm/skills/apm-usage/authentication.md @@ -12,9 +12,9 @@ APM checks these sources in order, using the first valid token found: | 4 | `GH_TOKEN` | Global | Set by `gh auth login` | | 5 | `gh auth token --hostname ` | GitHub-like hosts | Active `gh auth login` account | | 6 | `git credential fill` | Per-host and repo-path | System credential manager | +| -- | None | -- | Unauthenticated (public GitHub repos only) | When APM knows the repository URL, it includes the repo path in the credential-helper query. APM also checks the active `gh` CLI account before invoking OS credential helpers. This reduces ambiguous multi-account prompts on hosts like github.com. -| -- | None | -- | Unauthenticated (public GitHub repos only) | ## Per-org setup diff --git a/src/apm_cli/core/auth.py b/src/apm_cli/core/auth.py index 9115203e..20d2ba7c 100644 --- a/src/apm_cli/core/auth.py +++ b/src/apm_cli/core/auth.py @@ -86,7 +86,8 @@ class AuthResolver: """Single source of truth for auth resolution. Every APM operation that touches a remote host MUST use this class. - Resolution is per-(host, org) pair, thread-safe, cached per-process. + Resolution is per-(host, org, repo_path) tuple when repo context is known, + thread-safe, cached per-process. """ def __init__(self, token_manager: Optional[GitHubTokenManager] = None): @@ -258,7 +259,8 @@ def try_with_fallback( When the resolved token comes from a global env var and fails (e.g. a github.com PAT tried on ``*.ghe.com``), the method - retries with ``git credential fill`` before giving up. + retries with ``gh auth token`` and then ``git credential fill`` + before giving up. """ auth_ctx = self.resolve(host, org, repo_path=repo_path) host_info = auth_ctx.host_info @@ -275,9 +277,10 @@ def _try_credential_fallback(exc: Exception) -> T: if host_info.kind == "ado": raise exc _log(f"Token from {auth_ctx.source} failed, trying fallback credentials for {host}") - gh_token = self._token_manager.resolve_credential_from_gh_cli(host) - if gh_token: - return operation(gh_token, self._build_git_env(gh_token)) + if host_info.kind in ("github", "ghe_cloud", "ghes"): + gh_token = self._token_manager.resolve_credential_from_gh_cli(host) + if gh_token: + return operation(gh_token, self._build_git_env(gh_token)) cred = self._token_manager.resolve_credential_from_git(host, path=repo_path) if cred: return operation(cred, self._build_git_env(cred)) @@ -378,7 +381,8 @@ def _resolve_token( 2. Global env vars ``GITHUB_APM_PAT`` → ``GITHUB_TOKEN`` → ``GH_TOKEN`` (any host — if the token is wrong for the target host, ``try_with_fallback`` retries with git credentials) - 3. Git credential helper (any host except ADO) + 3. gh CLI active account (GitHub-like hosts only) + 4. Git credential helper (any host except ADO) All token-bearing requests use HTTPS, which is the transport security boundary. Host-gating global env vars is unnecessary @@ -399,11 +403,12 @@ def _resolve_token( return token, source # 3. gh CLI active account (GitHub-like hosts only) - gh_token = self._token_manager.resolve_credential_from_gh_cli(host_info.host) - if gh_token: - return gh_token, "gh-auth-token" + if host_info.kind in ("github", "ghe_cloud", "ghes"): + gh_token = self._token_manager.resolve_credential_from_gh_cli(host_info.host) + if gh_token: + return gh_token, "gh-auth-token" - # 4. Git credential helper (not for ADO — uses its own PAT) + # 4. Git credential helper (not for ADO -- uses its own PAT) if host_info.kind not in ("ado",): credential = self._token_manager.resolve_credential_from_git( host_info.host, diff --git a/src/apm_cli/core/token_manager.py b/src/apm_cli/core/token_manager.py index 6c39011a..fb333a92 100644 --- a/src/apm_cli/core/token_manager.py +++ b/src/apm_cli/core/token_manager.py @@ -23,6 +23,13 @@ import sys from typing import Dict, Optional, Tuple +from apm_cli.utils.github_host import ( + default_host, + is_azure_devops_hostname, + is_github_hostname, + is_valid_fqdn, +) + class GitHubTokenManager: """Manages GitHub token environment setup for different AI runtimes.""" @@ -50,7 +57,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: Dict[tuple[str, str, str], Optional[str]] = {} @staticmethod def _is_valid_credential_token(token: str) -> bool: @@ -70,6 +77,29 @@ def _is_valid_credential_token(token: str) -> bool: return False return True + @staticmethod + def _has_control_chars(value: str) -> bool: + """Return True when *value* contains credential protocol control chars.""" + return any(ord(char) < 32 for char in value) + + @staticmethod + def _supports_gh_cli_host(host: Optional[str]) -> bool: + """Return True when *host* should use gh CLI fallback.""" + if not host: + return False + if is_github_hostname(host): + return True + + configured_host = default_host().lower() + host_lower = host.lower() + if host_lower != configured_host: + return False + if configured_host == "github.com" or configured_host.endswith(".ghe.com"): + return False + if is_azure_devops_hostname(configured_host): + return False + return is_valid_fqdn(configured_host) + # `git credential fill` may invoke OS credential helpers that show # interactive dialogs (e.g. Windows Credential Manager account picker). # The 60s default prevents false negatives on slow helpers. @@ -113,6 +143,14 @@ def resolve_credential_from_git( The password/token from the credential store, or None if unavailable """ try: + fields = [host] + if path: + fields.append(path) + if username: + fields.append(username) + if any(GitHubTokenManager._has_control_chars(field) for field in fields): + return None + request_lines = ['protocol=https', f'host={host}'] if path: normalized_path = path.lstrip('/') @@ -244,11 +282,14 @@ def get_token_with_credential_fallback( if token: return token - cache_key = (host, path or '', username or '') + normalized_host = host.lower() if host else host + cache_key = (normalized_host, path or '', username or '') if cache_key in self._credential_cache: return self._credential_cache[cache_key] - gh_token = self.resolve_credential_from_gh_cli(host) + gh_token = None + if self._supports_gh_cli_host(host): + gh_token = self.resolve_credential_from_gh_cli(host) if gh_token: self._credential_cache[cache_key] = gh_token return gh_token diff --git a/tests/test_token_manager.py b/tests/test_token_manager.py index 9c87a16d..2e97766f 100644 --- a/tests/test_token_manager.py +++ b/tests/test_token_manager.py @@ -142,6 +142,16 @@ def test_path_and_username_are_sent_when_provided(self): "username=mamoreau-devolutions\n\n" ) + def test_control_characters_in_request_fields_return_none(self): + """Reject malformed git credential protocol fields before spawning git.""" + with patch('subprocess.run') as mock_run: + token = GitHubTokenManager.resolve_credential_from_git( + 'github.com', + path='Devolutions/RDM.git\npassword=oops', + ) + assert token is None + mock_run.assert_not_called() + def test_git_terminal_prompt_disabled(self): """GIT_TERMINAL_PROMPT=0 is set in the subprocess env.""" mock_result = MagicMock(returncode=0, stdout="password=tok\n") @@ -414,9 +424,20 @@ def test_different_hosts_separate_cache(self): tok2 = manager.get_token_with_credential_fallback('modules', 'gitlab.com') assert tok1 == 'tok-github.com' assert tok2 == 'tok-gitlab.com' - assert mock_gh.call_count == 2 + mock_gh.assert_called_once_with('github.com') assert mock_cred.call_count == 2 + def test_non_github_host_skips_gh_cli(self): + """Generic hosts should not invoke gh CLI fallback.""" + with patch.dict(os.environ, {}, clear=True): + manager = GitHubTokenManager() + with patch.object(GitHubTokenManager, 'resolve_credential_from_gh_cli') as mock_gh, \ + patch.object(GitHubTokenManager, 'resolve_credential_from_git', return_value='cred-token') as mock_cred: + token = manager.get_token_with_credential_fallback('modules', 'gitlab.com') + assert token == 'cred-token' + mock_gh.assert_not_called() + mock_cred.assert_called_once_with('gitlab.com', path=None, username=None) + def test_different_paths_separate_cache(self): """Different repo paths should not share a credential-helper cache entry.""" with patch.dict(os.environ, {}, clear=True): diff --git a/tests/unit/test_auth.py b/tests/unit/test_auth.py index 32e5551b..7f2c9238 100644 --- a/tests/unit/test_auth.py +++ b/tests/unit/test_auth.py @@ -12,6 +12,13 @@ from apm_cli.models.dependency.reference import DependencyReference +@pytest.fixture(autouse=True) +def disable_gh_cli_fallback(): + """Keep auth tests deterministic regardless of local gh login state.""" + with patch.object(GitHubTokenManager, "resolve_credential_from_gh_cli", return_value=None): + yield + + # --------------------------------------------------------------------------- # TestClassifyHost # ---------------------------------------------------------------------------