fix: validate workflow steps after environment substitution - #97
Shubham-Padkonde wants to merge 1 commit into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughWorkflow parsing now substitutes environment variables in each raw step before validating it. Tests cover validation of substituted names and commands, parsing a substituted ChangesWorkflow step substitution and validation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The workflow validation change is ready to merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Workflow variables can now select operations and failure behavior. Allowed-command checks and credential requirements remain, but the trust level of variables supplied to workflows is not established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Hardening Proposals
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the problem, solution, affected behavior, regression coverage, and test results. It does not follow the repository template because it omits the required section headings, related-issue entry, selected change type, and checklist confirmations.
✨ 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 |
Workflow steps are currently validated before environment variables are expanded. This lets invalid final names through: a literal
fetchstep and a${STEP_NAME}step are accepted even whenSTEP_NAME=fetch. At runtime their outputs share the sameStepContextkey. An empty expanded name is also accepted, while valid variable-basedcommandandon_errorvalues are rejected prematurely.Substitute environment variables before applying structural validation and duplicate-name checks. Deferred
${steps.*}references retain their existing behavior.Five regression cases fail before the change and pass afterward. The complete suite passes on Windows/Python 3.13: 188 passed, 3 skipped. Ruff 0.5.5 (the project's declared version) and
git diff --checkpass for the changed files.Prepared with Codex assistance.
Summary by CodeRabbit