fix(security): explicit branch check on nightly workflow_run trigger#771
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe nightly workflow's Init job ChangesNightly Workflow Security Gating
Sequence Diagram(s)(No sequence diagram generated.) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
No description provided. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/nightly.yaml:
- Around line 54-57: The workflow currently allows any manual dispatch via the
condition that checks github.event_name == 'workflow_dispatch' unconditionally;
tighten this by requiring the dispatch to target the develop branch as well —
i.e., change the first arm to require both github.event_name ==
'workflow_dispatch' AND github.ref == 'refs/heads/develop' (so use
(github.event_name == 'workflow_dispatch' && github.ref == 'refs/heads/develop')
|| ... ), updating the existing conditional around the workflow dispatch check
to restrict manual runs to refs/heads/develop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 217f6636-8878-4083-b4b9-7fbc9ed4d7d3
📒 Files selected for processing (1)
.github/workflows/nightly.yaml
Add `head_branch == 'develop'` guard to the nightly workflow's init job. The `branches: [develop]` filter on the trigger already enforces this, but static analyzers (OSSF Scorecard's DangerousWorkflow rule) can't trace that — they see workflow_run + head_sha checkout + secrets: inherit and flag the pattern critical regardless of the trigger filter. The explicit branch check ties the trust boundary to develop's branch protection (PR + code-owner review + required checks), which is the actual safety guarantee. All downstream jobs depend on init via needs:, so they inherit the gate without per-job changes. Behavior is unchanged: workflow_run runs that wouldn't have fired under the trigger filter still don't fire. Manual workflow_dispatch remains unaffected. Note: `workflow_run` triggers always execute the version of this file from main (per GitHub's rules), so this fix takes effect for runtime safety only after develop → main lands. The static-analysis finding (#760, alert #565) closes once the Scorecard scan re-runs against develop with this change. Fixes #760
anandray
force-pushed
the
fix/dangerous-workflow-760
branch
from
47ebc5e to
a5f3976
Compare
Per CodeRabbit review on PR #771: the workflow_dispatch arm in nightly.yaml accepted any ref, which combined with secrets: inherit and write permissions creates a residual privilege-escalation path even for trusted users. Tying manual dispatch to refs/heads/develop makes the trust boundary uniform across both trigger paths (workflow_run already gated to develop's branch protection chain). Operational impact is negligible: hot-fix flows go through develop already, and "republishing a known-good ref" practically always means a develop commit. Anyone wanting to publish from a feature branch must merge to develop first — which is the right policy for SOC2 change-management evidence. Refs #760
kwit75
left a comment
kwit75
left a comment
There was a problem hiding this comment.
LGTM — straightforward defensive hardening. The explicit head_branch == 'develop' check makes the trust boundary visible to static analyzers (OSSF Scorecard's DangerousWorkflow rule) and ties privileged jobs (secrets: inherit, contents: write) explicitly to develop's branch protection (PR review + required checks). The trigger-level branches: [develop] filter already enforces this in practice, but having the check live in the job's if: means the rule survives any future trigger refactor.
No functional change — same workflow_run / workflow_dispatch matrix as before, just with the branch invariant restated at the job level.
Approving and merging — CI OK is green and #774 already landed, so the prerelease pipeline is back to working.
2 participants
Add
head_branch == 'develop'guard to the nightly workflow's init job. Thebranches: [develop]filter on the trigger already enforces this, but static analyzers (OSSF Scorecard's DangerousWorkflow rule) can't trace that — they see workflow_run + head_sha checkout + secrets: inherit and flag the pattern critical regardless of the trigger filter.The explicit branch check ties the trust boundary to develop's branch protection (PR + code-owner review + required checks), which is the actual safety guarantee. All downstream jobs depend on init via needs:, so they inherit the gate without per-job changes.
Behavior is unchanged: workflow_run runs that wouldn't have fired under the trigger filter still don't fire. Manual workflow_dispatch remains unaffected.
Note:
workflow_runtriggers always execute the version of this file from main (per GitHub's rules), so this fix takes effect for runtime safety only after develop → main lands. The static-analysis finding (#760, alert #565) closes once the Scorecard scan re-runs against develop with this change.Fixes #760
Summary
Type
Testing
./builder testpassesChecklist
Linked Issue
Fixes #
Summary by CodeRabbit