Fix/workflow permissions

[danielmeppiel]

This pull request introduces two main sets of changes: it improves security and compliance in the GitHub Actions workflow by restricting token permissions, and it enhances the GitHub file downloader to better handle SAML/SSO authentication failures, including robust test coverage for these scenarios.

Workflow security improvements:

• Restricted default GITHUB_TOKEN permissions to contents: read for all jobs in .github/workflows/build-release.yml, following the principle of least privilege. [1] [2] [3] [4]

GitHub downloader authentication fallback:

• Updated _download_github_file in github_downloader.py to retry unauthenticated downloads if a 401/403 error is encountered (likely due to SSO/SAML restrictions), except for *.ghe.com domains which cannot have public repos.

Testing for fallback logic:

• Added tests in test_github_downloader.py to verify:
  • Retry without token on 401/403 for public repos.
  • No retry for *.ghe.com domains.
  • Retry applies to GHES custom domains.

[Copilot]

Pull request overview

This pull request implements security hardening in the GitHub Actions workflow and adds robust handling for SAML/SSO authentication failures in the GitHub file downloader, ensuring that public repositories remain accessible even when a personal access token lacks SSO authorization for an organization.

Changes:

• Restricted default GITHUB_TOKEN permissions to contents: read at the workflow level, with per-job permission grants only where required (e.g., contents: write for release creation, id-token: write for PyPI publishing)
• Enhanced GitHub downloader to automatically retry file downloads without authentication when encountering 401/403 errors, except for *.ghe.com domains which cannot have public repositories
• Added comprehensive test coverage verifying the SAML/SSO fallback logic works for github.com and GHES custom domains, but is correctly excluded for GitHub Enterprise Cloud Data Residency domains

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.

File	Description
.github/workflows/build-release.yml	Added workflow-level default permissions (contents: read) and explicit per-job permissions following the principle of least privilege
src/apm_cli/deps/github_downloader.py	Implemented SAML/SSO fallback logic that retries file downloads without authentication on 401/403 errors for github.com and GHES, excluding *.ghe.com domains
tests/test_github_downloader.py	Added three test cases verifying the SAML fallback behavior: successful retry for github.com, no retry for *.ghe.com, and successful retry for GHES custom domains

Comments suppressed due to low confidence (1)

tests/test_github_downloader.py:418

• While the code handles both 401 and 403 status codes (line 595 in github_downloader.py), the test suite only verifies the successful fallback for 401 errors. Consider adding a test case that verifies the fallback also works correctly for 403 errors on github.com or GHES custom domains.

    def test_download_raw_file_saml_fallback_retries_without_token(self):
        """Test that download_raw_file retries without token on 401/403 (SAML/SSO)."""
        with patch.dict(os.environ, {'GITHUB_APM_PAT': 'saml-blocked-token'}, clear=True):
            downloader = GitHubPackageDownloader()
            dep_ref = DependencyReference.parse('microsoft/some-public-repo/sub/dir')

            # First call (with token) returns 401, second call (without token) returns 200
            mock_response_401 = Mock()
            mock_response_401.status_code = 401
            mock_response_401.raise_for_status = Mock(
                side_effect=__import__('requests').exceptions.HTTPError(response=mock_response_401)
            )

            mock_response_200 = Mock()
            mock_response_200.status_code = 200
            mock_response_200.content = b'# SKILL.md content'
            mock_response_200.raise_for_status = Mock()

            with patch('apm_cli.deps.github_downloader.requests.get') as mock_get:
                mock_get.side_effect = [mock_response_401, mock_response_200]
                
                result = downloader.download_raw_file(dep_ref, 'sub/dir/SKILL.md', 'main')
                assert result == b'# SKILL.md content'
                
                # First call should include auth header
                first_call_headers = mock_get.call_args_list[0][1].get('headers', {})
                assert 'Authorization' in first_call_headers
                
                # Second (retry) call should NOT include auth header
                second_call_headers = mock_get.call_args_list[1][1].get('headers', {})
                assert 'Authorization' not in second_call_headers

[Copilot]

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.

[Copilot]

host.endswith(".ghe.com") is case-sensitive. Since DependencyReference.parse() can preserve the original casing for host prefixes (e.g., Company.GHE.com/...), this check may misclassify the host and incorrectly enable the unauthenticated retry (and elsewhere build the wrong API URL). Normalize host once (e.g., host = host.lower()) or use host.lower().endswith(...) consistently.

[Copilot]

The new SAML/SSO fallback changes user-visible behavior when a token is set but blocked by org SSO (public repos can still be fetched). Please add a short note in the docs (e.g., token configuration section) explaining the 401/403 unauthenticated retry behavior and the *.ghe.com exception, so users understand why a second request happens and what to do if it still fails.

[Copilot]

The raised RuntimeError on 401/403 doesn’t include file_path (or ref), which makes it hard to diagnose which fetch failed when installing/validating multiple files. Consider including the path and ref in the authentication failure message (similar to the 404 branch) while keeping it free of credentials.

[Copilot]

Using __import__('requests') inline makes these tests harder to read and can be replaced with a normal import requests at the top (then reference requests.exceptions.HTTPError). This will also make it easier to extend these tests to assert on the exception details in the future.