Conversation
- Add workflow-level default: permissions: contents: read - Add explicit permissions to test job (was missing) - Downgrade build job from contents: write to contents: read (upload-artifact uses Actions API, not contents API) - Add contents: read to publish-pypi (needed for checkout) - Add explicit permissions to update-homebrew (was missing) - integration-tests and release-validation already correct
There was a problem hiding this comment.
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_TOKENpermissions tocontents: readat the workflow level, with per-job permission grants only where required (e.g.,contents: writefor release creation,id-token: writefor PyPI publishing) - Enhanced GitHub downloader to automatically retry file downloads without authentication when encountering 401/403 errors, except for
*.ghe.comdomains 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
- Remove redundant mock_get.return_value in ghe.com test (side_effect takes precedence) - Add test for when both auth and unauth attempts fail (fallback-still-fails path) - Improve error message to distinguish SAML/SSO retry from simple token issues
…clarity - Use return_value instead of side_effect for ghe.com test mock to match real requests behavior (get() returns response, raise_for_status() throws) - Add explicit self.github_token check in error message elif for clarity and consistency with the retry guard condition
- Use case-insensitive .ghe.com host checks (host.lower().endswith)
- Include file_path and ref in 401/403 error messages for easier debugging
- Replace __import__('requests') with proper import in tests
Split CI/CD into three workflows: - ci.yml: unit tests + build (pull_request, no secrets needed) - ci-integration.yml: smoke/integration/release-validation (pull_request_target, gated by integration-tests environment) - build-release.yml: full pipeline for push/tag/schedule (secrets always available) This follows the industry standard pull_request_target + environment approval pattern used by major OSS projects. Fork PRs get immediate unit test feedback, and maintainers approve integration runs after reviewing the code.
Addresses CodeQL high alert (actions/untrusted-checkout/high) by switching ci-integration.yml from pull_request_target to workflow_run trigger. Security improvement: - PR code is NEVER checked out in a privileged context - Binary artifacts are downloaded from ci.yml (built unprivileged) - Test scripts always come from the default branch (main) - Environment approval gate still required before tests run - Commit status reported back to PR SHA for visibility
On push-to-main, these jobs were already validated during the PR via ci-integration.yml. Re-running them post-merge is redundant and wastes ~16 matrix jobs (~32 min of CI). They still run for: - tag pushes (release gate) - scheduled runs (regression detection) - manual dispatch (on-demand validation)
|
Fixes #12 |
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
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:
GITHUB_TOKENpermissions tocontents: readfor all jobs in.github/workflows/build-release.yml, following the principle of least privilege. [1] [2] [3] [4]GitHub downloader authentication fallback:
_download_github_fileingithub_downloader.pyto retry unauthenticated downloads if a 401/403 error is encountered (likely due to SSO/SAML restrictions), except for*.ghe.comdomains which cannot have public repos.Testing for fallback logic:
test_github_downloader.pyto verify:*.ghe.comdomains.