Skip to content

Fix/workflow permissions#87

Merged
danielmeppiel merged 10 commits intomainfrom
fix/workflow-permissions
Feb 24, 2026
Merged

Fix/workflow permissions#87
danielmeppiel merged 10 commits intomainfrom
fix/workflow-permissions

Conversation

@danielmeppiel
Copy link
Collaborator

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.

- 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
Copilot AI review requested due to automatic review settings February 16, 2026 11:34
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

- 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
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

…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
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

@danielmeppiel danielmeppiel added this to the 0.7.4 milestone Feb 16, 2026
@danielmeppiel danielmeppiel added the bug Something isn't working label Feb 16, 2026
- 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)
@danielmeppiel
Copy link
Collaborator Author

danielmeppiel commented Feb 23, 2026

Fixes #12

@github-advanced-security
Copy link
Contributor

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.

@danielmeppiel danielmeppiel merged commit 0bb6665 into main Feb 24, 2026
9 of 11 checks passed
@danielmeppiel danielmeppiel deleted the fix/workflow-permissions branch February 27, 2026 09:42
@danielmeppiel danielmeppiel mentioned this pull request Mar 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Fork PR Testing does not work (no access to secrets)

3 participants