Repository navigation
Fail trusted validation for fork pull requests - #878
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: dd4ef8ae-bf2b-40d0-a491-5a6276f0eeed
|
👋 Thanks for your contribution, Brandon Miller (@brandom-msft)! This repository is read-only. If you are contributing on behalf of Microsoft, please submit your PR to the private staging repository instead: See CONTRIBUTING.md for full instructions. |
Run the required trusted job after non-cancelled dependency failures and validate the detector output contract before checkout. Extend the workflow structure harness with malformed-state regression cases. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: dd4ef8ae-bf2b-40d0-a491-5a6276f0eeed
There was a problem hiding this comment.
Pull request overview
This PR adjusts the validate workflow so the required trusted check always produces a definitive result for fork PRs (avoiding the “skipped required check counts as satisfied” behavior), and adds a self-test harness to prevent regressions in that workflow structure.
Changes:
- Makes
trustedalways run and explicitly fail fork PRs (after credential-free work), while guarding all Azure/OIDC/L4 steps to same-repo PRs. - Adds a contract-check step to fail closed if
detectfails or emits malformed outputs. - Extends the scripts self-test workflow with a hermetic workflow-structure regression harness and introduces the harness script.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| .github/workflows/validate.yml | Reworks the trusted job to always run, validate detect contract, explicitly fail forks, and gate all credentialed steps to same-repo. |
| .github/workflows/scripts-selftest.yml | Expands the self-test trigger scope and adds a workflow-structure harness job. |
| .github/scripts/test/run-workflow-structure-tests.sh | New hermetic script that asserts validate.yml retains the required trusted-gate structure and runs contract logic tests. |
Suppressed comments (2)
.github/workflows/validate.yml:173
trusteddepends ondetectvianeeds:; a dependent job is skipped when its dependency fails unless the job-level condition usesalways(). Withif: ${{ !cancelled() }}, a failingdetectcan still causetrustedto be skipped, recreating the “skipped required check” hole this PR is trying to close. Usealways() && !cancelled()to ensuretrustedstarts after dependency failure (but still doesn’t run on cancellation).
trusted:
needs: detect
if: ${{ !cancelled() }}
runs-on: ubuntu-latest
.github/workflows/validate.yml:162
- The comment block above
trustedsays the job-level condition is!cancelled(), but to run after a faileddetectit needs to bealways() && !cancelled(). If you adopt thealways()fix, update this doc text so it stays accurate.
# GitHub treats a required job that concludes `skipped` as satisfying the merge gate.
# - FAILS CLOSED when `detect` fails or emits malformed outputs. The exact job-level
# `!cancelled()` status condition starts trusted after dependency failure; the FIRST step
# validates the dependency result/output contract before checkout or docs-only handling.
# - Docs-only PRs SHORT-CIRCUIT before any Azure/secret step (has_changes gate). A docs-only
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (3)
.github/workflows/validate.yml:377
- Guard the fork-rejection step on the detection-contract step succeeding. Without this, the step can run after a contract failure and emit a promotion instruction even though the immediate remediation is to fix the detect contract/output corruption.
- name: Reject fork until maintainer promotion
if: ${{ !cancelled() && github.event.pull_request.head.repo.fork == true }}
run: |
.github/workflows/validate.yml:158
- The header comment claims forks run L3 then fail, but docs-only fork PRs will skip the L3 step (
has_changes == 'true') and go straight to the fork-rejection failure. Please adjust the comment so it matches the actual control flow.
# - ALWAYS RUNS for every triggered PR. Forks run credential-free L3, then fail in-job with
# a promotion-required error. Never restore a job-level fork/draft `if:`: P3.3 proved that
# GitHub treats a required job that concludes `skipped` as satisfying the merge gate.
.github/workflows/validate.yml:200
- The fork-rejection step will currently run even if the initial detection-contract step fails (because it has an explicit
if:that does not depend on prior step success). That can emit a promotion-required instruction on runs where the real failure is a broken/malformeddetectresult/output contract, which is contrary to the intent of making the contract failure terminal and self-explanatory.
- name: Validate sample detection contract
env:
DETECT_RESULT: ${{ needs.detect.result }}
HAS_CHANGES: ${{ needs.detect.outputs.has_changes }}
COUNT: ${{ needs.detect.outputs.count }}
SAMPLES: ${{ needs.detect.outputs.samples }}
run: |
Summary
trustedjob running for everypull_requestRoot cause
Live enforcement proof on microsoft-foundry/foundry-samples#874 showed that a job-level fork
if:concludestrustedasSKIPPED, and GitHub accepts that skipped required check as satisfied. The PR became mergeable when the review requirement was temporarily removed.The corrected job always runs. Forks perform credential-free L3 work, verify that no OIDC request surface is present, emit an explicit promotion-required error, and exit 1. Same-repo PRs retain the existing docs-only and conditional L4 paths.
Credential isolation
pull_request, neverpull_request_targetL4-validationwas verified to have no protection rules before implementationValidation
bash .github/scripts/test/run-workflow-structure-tests.sh— 17 passed, 0 failedbash -n .github/scripts/test/run-workflow-structure-tests.shgit diff --checkactionlintandshellcheckare not installed or configured in the current repository tooling, so no new linter dependency was introduced.Tracking
No ruleset or environment mutation is included in this PR.