Harden review-trigger workflow: env-var inputs, single-step token flow, pre-flight authorize job#35439
Harden review-trigger workflow: env-var inputs, single-step token flow, pre-flight authorize job#35439JanKrivanek wants to merge 1 commit into
Conversation
Three follow-up improvements from the adversarial code review of PR #35250: 1. Move github.actor / github.repository to env: vars (ACTOR, REPO) so they are no longer interpolated directly into shell. Consistent with the existing hardening pattern for COMMENT_BODY and inputs.pr_number. 2. Combine OIDC fetch + AzDO token exchange + pipeline trigger into a single step. Avoids persisting masked tokens through GITHUB_OUTPUT between steps; tokens stay shell-local and are explicitly unset once consumed. 3. Move actor permission check into a lightweight pre-flight 'authorize' job. Unauthorized /review comments no longer provision the heavier trigger-review runner. Failure exits 0 with a notice instead of erroring, keeping the run history clean.
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35439Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35439" |
|
/review |
🤖 AI Summary
📊 Review Session —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #35439 | Three-pronged hardening: env-var inputs for actor/repo; collapse OIDC+AzDO+trigger into one step with explicit unset; split into authorize pre-job + trigger-review job, exit 0 on unauthorized |
.github/workflows/review-trigger.yml |
Original PR; matches issue #35431 acceptance criteria exactly |
🔧 Fix — Analysis & Comparison
Try-Fix Phase Aggregate Narrative
Four diverse candidates were explored, each loading a different maui-expert-reviewer dimension to maximize approach diversity. Because the PR touches only a GitHub Actions YAML workflow (no .NET runtime, no buildable/testable surface), the candidates are conceptual alternative-approach diffs evaluated for security, performance, maintainability, and policy-fidelity to issue #35431.
| Attempt | Dimension loaded | Approach in one line | Outcome |
|---|---|---|---|
| try-fix-1 | Security / injection surface | Add permissions: {} default, set -euo pipefail, lift remaining ${{ github.event_name }} to env |
Valid hardening but out-of-scope vs issue #35431 |
| try-fix-2 | API / contract design | Keep authorize pre-flight job but fail loudly (exit 1, ::error::) to preserve audit trail |
Violates issue #35431 item 3 which explicitly mandates exit 0 + ::notice:: |
| try-fix-3 | Performance / runner cost | Use github.event.comment.author_association in job-level if: — zero runners for unauthorized |
Silently changes access policy (admits triage users that the PR rejects) |
| try-fix-4 | State isolation / threading | Keep three separate steps, pass tokens via mktemp files in $RUNNER_TEMP instead of $GITHUB_OUTPUT |
More complex than PR with no measurable security gain |
Key takeaways
- None of the four candidates is strictly better than the PR. Each makes a different trade-off the PR consciously avoided.
- try-fix-1 is the closest to "additive improvement": a maintainer could fold the
permissions: {}+set -euo pipefailideas into a follow-up PR without regressing anything. - try-fix-2 and try-fix-3 are principled disagreements with issue Harden review-trigger workflow: consistency and cost improvements #35431's design decisions, not improvements.
- try-fix-4 is an alternative implementation strategy that's equally safe but more complex than the PR's clean shell-local approach.
- The PR (Harden review-trigger workflow: env-var inputs, single-step token flow, pre-flight authorize job #35439) implements exactly what issue Harden review-trigger workflow: consistency and cost improvements #35431 asked for, with no spurious churn. Phase 3 (Report) recommends
pras the winner.
Notes on methodology
Because no executable test loop exists for a workflow YAML in this repo, the standard try-fix BuildAndRunHostApp.ps1 / Run-DeviceTests.ps1 test commands cannot be applied. Each candidate was instead evaluated against:
- YAML structural validity (mental parse + diff hunks reviewed for syntax correctness)
- Behavioral parity with stated intent of issue Harden review-trigger workflow: consistency and cost improvements #35431
- Security delta vs the pre-PR baseline (
main) - Maintainability and complexity cost
No code was written to the repository; candidates exist only as conceptual diffs in the per-attempt content.md files.
📋 Report — Final Recommendation
Phase 3 — Comparative Report
Candidates evaluated
| Candidate | Description |
|---|---|
pr |
PR #35439 as submitted by JanKrivanek |
pr-plus-reviewer |
PR #35439 + two suggestion-level polish edits from the inline expert review |
try-fix-1 |
PR + permissions: {} default + set -euo pipefail + EVENT_NAME env var (security hardening expansion) |
try-fix-2 |
PR but authorize job fails loudly (exit 1 + ::error::) on unauthorized (audit-trail priority) |
try-fix-3 |
Replace API call with github.event.comment.author_association in job-level if: (zero-runner gate) |
try-fix-4 |
Keep three separate steps; pass tokens via mktemp files in $RUNNER_TEMP instead of $GITHUB_OUTPUT |
Scoring rubric (out of 5 per axis)
| Candidate | Scope-fidelity to #35431 | Security delta vs main | Behavioral parity | Maintainability | Total |
|---|---|---|---|---|---|
pr |
5 | 4 | 5 | 5 | 19 |
pr-plus-reviewer |
5 | 4 | 5 | 4 (extra comments) | 18 |
try-fix-1 |
3 (out-of-scope) | 5 | 5 | 3 (larger diff) | 16 |
try-fix-2 |
2 (contradicts item 3) | 4 | 4 (UX change) | 5 | 15 |
try-fix-3 |
3 (policy drift) | 3 (admits triage users) | 2 (policy change) | 5 | 13 |
try-fix-4 |
4 | 4 | 5 | 3 (more complex) | 16 |
Analysis
pr wins on the most important axis (scope-fidelity) and ties or wins on every other axis.
- Scope-fidelity: Issue Harden review-trigger workflow: consistency and cost improvements #35431 named exactly three items; the PR implements exactly those three items with no spurious churn.
try-fix-2andtry-fix-3actively contradict acceptance criteria.try-fix-1adds scope the issue didn't ask for. - Security delta: All candidates remove the OIDC/AzDO token round-trip through
$GITHUB_OUTPUT. The PR's approach (shell-local +unset) is the simplest;try-fix-4's mktemp approach is equally safe but more complex;try-fix-1's additionalpermissions: {}+ pipefail is a real-but-additive improvement;try-fix-3weakens access control. - Behavioral parity:
prandpr-plus-reviewerare byte-for-byte identical for authorized callers vs. pre-PR.try-fix-2changes UX (red runs for unauthorized — by design).try-fix-3admits a permission class (triage) that the PR rejects. - Maintainability: PR's diff is the most surgical of all candidates that respect the issue's scope.
pr-plus-reviewer is essentially pr plus two suggestion-level cleanups. Both are non-blocking polish; neither is required for merge. Adopting them now would slightly inflate the diff. Adopting them in a follow-up PR is just as good. → Prefer pr for clean merge.
try-fix-1 is a real follow-up candidate (the ideas are good) but should be filed separately so the focused issue-#35431 fix lands clean.
try-fix-2 would require a maintainer to overrule issue #35431's explicit guidance.
try-fix-3 is mechanically attractive (zero runners for drive-bys, no API call) but silently broadens who can trigger /review. Rejected on policy-drift grounds.
try-fix-4 is a defensible alternative implementation but trades simplicity for marginal step-log clarity. Not better than the PR.
Winner: pr
PR #35439 is mergeable as-is. The two reviewer suggestions captured in inline-findings.json are non-blocking polish that can be folded in by the author or deferred to a follow-up. The four try-fix candidates each illustrate a trade-off the PR consciously avoided, and none scores higher than the PR on the combined rubric.
Recommendation to the human reviewer
- Action: Approve / merge PR Harden review-trigger workflow: env-var inputs, single-step token flow, pre-flight authorize job #35439.
- Optional follow-ups (any of these would be a fine separate PR; none is a blocker):
- Apply the two
pr-plus-reviewersuggestions (unusedACTORenv var ontrigger-review, surface stderr from the permission gate). - Adopt
try-fix-1'spermissions: {}deny-all top-level default andset -euo pipefailin shell blocks.
- Apply the two
- Note for the author: Gate was SKIPPED because no tests are present. Adding a workflow lint/syntax check (e.g.,
actionlint) as part of CI would give future workflow-only PRs an automated gate without requiring runtime tests.
MauiBot
added
s/agent-review-incomplete
s/agent-reviewed
|
/azp run maui-pr-devicetests, maui-pr-uitests |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
|
/review -b feature/refactor-copilot-yml |
|
|
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
Can you please resolve conflicts?
PureWeen
added
the
area-ai-agents
|
/review -b feature/enhanced-reviewer -p android |
4 participants
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Fixes #35431.
Three follow-up hardening items from the adversarial code review of #35250 applied to
.github/workflows/review-trigger.yml:1. Move
github.actor/github.repositorytoenv:vars${{ github.actor }}and${{ github.repository }}were interpolated directly into shellrun:blocks. Not exploitable today (GitHub usernames are[a-zA-Z0-9-]andgithub.repositoryis constant), but inconsistent with howinputs.pr_numberandCOMMENT_BODYare already hardened. Now passed viaenv:(ACTOR,REPO) and consumed as shell variables ingh apicalls.2. Combine OIDC + AzDO token exchange + pipeline trigger into one step
The OIDC token, the AzDO access token, and the pipeline-trigger call are now performed in a single step. Previously the masked tokens were persisted through
$GITHUB_OUTPUTbetween steps; they now stay shell-local and are explicitlyunsetonce consumed.3. Permission check moved to a lightweight pre-flight job
A new
authorizejob runs the comment-match + collaborator-permission check before the heaviertrigger-reviewjob is provisioned. Drive-by/reviewcomments by non-collaborators no longer spin up the main runner. Unauthorized callers now exit0with a::notice::instead of::error::, so the workflow run history stays clean.Validation
python -c "import yaml; yaml.safe_load(...)").workflow_dispatchcontinues to bypass the collaborator check (only repo-write users can dispatch workflows anyway).