Harden manual integration test trust boundary#7081
Conversation
Require two write-capable approvals for the exact PR head SHA, pin all targets to immutable commits, and narrow secret and OIDC access. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b8c7d85f-7576-4fc7-a7b5-c77833344088
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 89%
✓ Correctness
This PR is well-implemented. The resolve_integration_test_target.js logic correctly reduces review state to the latest decisive review per reviewer, validates commit freshness, enforces write-capability checks, and excludes self-approvals. The workflow changes properly scope id-token:write to only the jobs neding OIDC, replace secrets:inherit with explicit allowlists, and pin checkouts to immutable SHAs resolved from trusted scripts checked out at github.sha. No correctness bugs found.
✓ Security Reliability
This is a well-implemented security hardening PR. The trust boundary enforcement is sound: scripts are checked out from the trusted workflow commit (github.sha), SHA validation prevents injection, approval checks verify write-capable maintainers for the exact head commit, secrets are explicitly allowlisted instead of inherited, and id-token permissions are scoped only to jobs requiring OIDC. The input validation, immutable SHA pining, and review-state reduction logic are all correctly implemented with appropriate test coverage. No blocking security or reliability issues found.
✓ Test Coverage
The test suite covers the key security-critical paths well: input validation, stale/self/read-only approval filtering, decisive review state reduction, dismissed reviews, and branch resolution. One notable gap is that the
hasWritePermissionfallback path (matching 'admin'/'maintain' permission strings whenuser.permissions.pushis absent) is never independently tested—the mock always sets both conditions simultaneously, so a regression in the string-based fallback would go undetected.
✓ Failure Modes
The PR implements a sound trust-boundary hardening for the manual integration test workflow. The resolve_integration_test_target.js script correctly validates inputs, pins commits to immutable SHAs, enforces fresh approval requirements from write-capable maintainers, and propagates all errors. The workflow changes properly scope id-token permissions to only jobs neding OIDC, replace secrets:inherit with explicit allowlists, and use validated SHA values for checkout and change detection. No silent failure paths, swallowed exceptions, or security-relevant operational gaps were identified.
✓ Design Approach
The trust-boundary hardening looks directionally right, but pining PR targets to a bare head SHA introduces a design regression fork-based pull requests. The new flow no longer carries enough information for downstream jobs to check out contributor code when the PR head commit is not in the base repository.
Automated review by moonbox3's agents
Declare credentials consumed by the reusable integration workflows as required while retaining the explicitly optional Foundry models key. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b8c7d85f-7576-4fc7-a7b5-c77833344088
There was a problem hiding this comment.
Pull request overview
This PR hardens the trust boundary for manually dispatched integration tests by ensuring secret-backed jobs only run against an immutable, explicitly authorized commit SHA (and not a mutable PR head ref), with OIDC and secrets scoped more tightly to only what’s needed.
Changes:
- Add a GitHub Script helper (
resolve_integration_test_target.js) that resolves PRs/branches to immutable SHAs and enforces “2 non-self approvals from write-capable maintainers” for the exact PR head commit. - Update the manual orchestration workflow to use the helper, pass the pinned SHA to reusable integration workflows, and detect changes via SHA-to-SHA comparison.
- Replace
secrets: inheritwith explicit secret allowlists and moveid-token: writeto only the jobs that require Azure OIDC auth.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/python-integration-tests.yml | Adds explicit reusable-workflow secret declarations and scopes OIDC permissions to Azure-auth jobs while continuing to checkout by provided SHA. |
| .github/workflows/integration-tests-manual.yml | Resolves targets to immutable SHAs with approval gating, uses SHA-based change detection, and explicitly passes only required secrets/OIDC permissions to called workflows. |
| .github/workflows/dotnet-integration-tests.yml | Adds reusable-workflow secret declarations and scopes OIDC permissions at the job level while checking out the pinned SHA. |
| .github/tests/test_resolve_integration_test_target.js | Adds Node test coverage for PR/branch resolution logic and approval/permission filtering behavior. |
| .github/scripts/resolve_integration_test_target.js | Implements immutable target resolution and approval/permission enforcement for manual integration test runs. |
Motivation & Context
Tightens authorization and target handling for manual integration-test runs.
Description & Review Guide
Related Issue
Addresses an internal MSRC case.
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.