Harden workflow GitHub context handling#3372
Conversation
Pass attacker-controllable GitHub context and workflow values through environment variables before shell use. Co-authored-by: openhands <openhands@all-hands.dev>
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Good security hardening work. The approach — moving ${{ ... }} GitHub context expressions into env: blocks and referencing them as $VAR_NAME in shell — is the correct fix for script injection vulnerabilities (CWE-78). Coverage is comprehensive across all 12 workflow files.
Two minor points worth addressing before merge (both low-risk, one stylistic and one functional).
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation.
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
enyst
added
the
review-this
OpenHands AI
Co-authored-by: openhands <openhands@all-hands.dev>
enyst
added
the
review-this
OpenHands AI
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Taste Rating: Acceptable — the hardening direction is good, but two workflow-input paths still need tightening before approval.
Issues found:
.github/workflows/integration-runner.ymlstill expandsworkflow_dispatchmodel_idsinto Python source in theResolve model configurationsheredoc (model_ids = "$MODEL_IDS".split(",")). A quoted heredoc plusos.environwould avoid pre-validation code injection.- See inline note on
version-bump-prs.ymlfor$GITHUB_OUTPUTinjection before validation.
Risk: 🟡 Medium, because these workflows handle attacker-influenced inputs and some downstream jobs use elevated tokens/secrets.
This review was generated by an AI agent (OpenHands) on behalf of the user.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/software-agent-sdk/actions/runs/26371139976
Co-authored-by: openhands <openhands@all-hands.dev>
enyst
added
the
review-this
OpenHands AI
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Taste Rating: Acceptable — the hardening direction is right, but a few injection/regression issues still need follow-up.
I left inline comments for the model-resolution regression, unsafe output handling, and JSON payload construction. One additional sweep item: .github/workflows/server.yml still builds build-info/*.json with an unquoted heredoc containing ${{ env.IMAGE }} / step outputs, so workflow-dispatch image or malformed outputs can still be expanded by the shell; please generate that JSON via env + jq -n as well.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM. These are privileged CI/workflow paths, and the remaining shell/output/JSON handling gaps can break runs or preserve injection surfaces.
VERDICT: ❌ Needs follow-up before approval.
This review was generated by an AI agent (OpenHands) on behalf of the user.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/software-agent-sdk/actions/runs/26371627904
Co-authored-by: openhands <openhands@all-hands.dev>
enyst
added
the
review-this
OpenHands AI
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Taste Rating: Acceptable — the direction is right, but a few workflow input/output paths still need tightening.
Risk: 🟡 Medium (privileged CI/release workflows; issues are scoped to workflow hardening). This review was generated by an AI agent (OpenHands) on behalf of the user.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/software-agent-sdk/actions/runs/26372133139
Co-authored-by: openhands <openhands@all-hands.dev>
enyst
added
the
review-this
OpenHands AI
|
[Automatic Post]: It has been a while since there was any activity on this PR. @enyst, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. This comment was generated by an AI agent (OpenHands) on behalf of the repository maintainers. |
|
[Automatic Post]: This PR seems to be currently waiting for review. @enyst, could you please take a look when you have a chance? This comment was created by an AI agent (OpenHands) on behalf of the user. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: Harden workflow GitHub context handling
🟡 Acceptable — Addresses real problem with solid pattern, but one security gap remains.
[CRITICAL ISSUES]
- .github/workflows/integration-runner.yml Shell Injection: The
gh pr commentcommand still uses--body "$COMMENT_BODY"without proper escaping. IfCOMMENT_BODYcontains shell metacharacters (e.g.,$(rm -rf /)), they will be executed by the shell. The pattern you correctly applied intodo-management.ymlusingjq -nto escape JSON values should be applied here as well.
Or use gh pr comment --body-file - with stdin as shown in condenser-runner.yml.
[IMPROVEMENT OPPORTUNITIES]
-
.github/workflows/prepare-release.yml, Line ~80 Double substitution: The PR body template uses
__VERSION__as a placeholder, thensedreplaces it. This works but introduces a hidden dependency. Consider building the PR body entirely withjq -nor using an environment variable substitution pattern consistent with the rest of the file. -
.github/workflows/integration-runner.yml, Line ~132 PR_NUMBER removed from env: The
PR_NUMBER: ${{ github.event.pull_request.number }}was removed. Verify no other steps in this workflow still referencePR_NUMBER.
[STYLE NOTES]
- Generally good. The
write_output()function inapi-compliance-runner.ymlis a clean pattern for handling multi-line values in$GITHUB_OUTPUT. - The array-based command argument construction is the right approach for handling values with spaces.
[RISK ASSESSMENT]
⚠️ Risk Assessment: 🟡 MEDIUM- The PR correctly fixes shell injection in
todo-management.yml(critical security fix), but the same vulnerability pattern persists inintegration-runner.ymlwheregh pr comment --bodyis still used without escaping. - Context handling improvements are sound and follow the recommended GitHub Actions pattern.
- The
jq -napproach for JSON escaping is well-executed where applied.
VERDICT:
❌ Needs rework — Address the shell injection vulnerability in integration-runner.yml before merging.
KEY INSIGHT:
The security fix for shell injection was applied inconsistently — todo-management.yml was hardened with jq -n escaping, but integration-runner.yml still uses the vulnerable gh pr comment --body "$VAR" pattern.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
⚠️ QA Report: PASS WITH ISSUES
Functional workflow-hardening checks passed for representative GitHub Actions paths, but the PR currently has failing PR-description CI checks.
Does this PR achieve its stated goal?
Yes. The goal was to avoid shell interpretation of attacker-influenced GitHub context/workflow inputs inside run: blocks. I exercised representative before/after GitHub Actions shell behavior with command-substitution payloads: the baseline snippets executed the injected touch side effects, while the PR-style env-variable handling preserved those payloads as literal argument/output data and did not execute them. I also verified the TODO PR payload generation now emits valid JSON for quoted/multiline user-controlled content.
| Phase | Result |
|---|---|
| Environment Setup | ✅ PR commit 511f918a checked out; bash, jq, openssl, gh, and python3 available; no tests/linters run. |
| CI Status | Validate PR description checks were failing; qa-changes and unresolved-review-threads were in progress; other relevant checks were green or skipped. |
| Functional Verification | ✅ Representative workflow run-block behavior verified with malicious inputs and JSON payload generation. |
Functional Verification
I ran a local harness with python3 - <<'PY' ... PY that executed the affected bash snippets the same way GitHub Actions run: blocks do, using benign marker-file payloads such as $(touch /tmp/qa_pr_api_pwn).
Test 1: API compliance workflow input handling
Step 1 — Reproduce baseline without the fix:
Ran the old direct-interpolation-style shell assignment with PATTERNS="$(touch /tmp/qa_base_api_pwn) --include sdk":
baseline args: --patterns --include sdk --models litellm_proxy/claude-sonnet-4-20250514
baseline command substitution: EXECUTED
This confirms the bug class: command substitution in directly interpolated workflow input was evaluated by the shell.
Step 2 — Apply the PR's changes:
Used the checked-out PR behavior: PATTERNS_INPUT passed through env:, then assigned with PATTERNS="$PATTERNS_INPUT" and written via the multiline GITHUB_OUTPUT helper.
Step 3 — Re-run with the fix in place:
fixed command substitution: not executed
fixed output preview:
patterns<<patterns_d44c588b223318f432ed5fdacb079d33
$(touch /tmp/qa_pr_api_pwn) --include sdk
patterns_d44c588b223318f432ed5fdacb079d33
models<<models_d683858b3fcfc67899b5d05b893ba2a5
litellm_proxy/claude-sonnet-4-20250514
This shows the payload is preserved as literal data and not executed.
Test 2: Datadog/PostHog optional issue-parent argument
Step 1 — Reproduce baseline without the fix:
Ran the old direct-interpolation-style --issue-parent "$(touch /tmp/qa_base_parent_pwn)parent-123" command invocation:
baseline argv: [datadog_debugging.py] [--query-type] [logs] [--query] [service:api] [--repos] [OpenHands/software-agent-sdk] [--working-dir] [/tmp/datadog-debug] [--issue-repo] [OpenHands/software-agent-sdk] [--issue-parent] [parent-123] [--issue-prefix] [QA]
baseline issue_parent substitution: EXECUTED
This confirms the injected shell expression ran before the Python command received argv.
Step 2 — Apply the PR's changes:
Used the env-backed array form: ISSUE_PARENT_ARG=(--issue-parent "$INPUTS_ISSUE_PARENT") only when the env value is non-empty.
Step 3 — Re-run with the fix in place:
fixed argv: [datadog_debugging.py] [--query-type] [logs] [--query] [service:api] [--repos] [OpenHands/software-agent-sdk] [--working-dir] [/tmp/datadog-debug] [--issue-repo] [OpenHands/software-agent-sdk] [--issue-parent] [$(touch /tmp/qa_pr_parent_pwn)parent-123] [--issue-prefix] [QA]
fixed issue_parent substitution: not executed
empty-parent fixed argv: [datadog_debugging.py] [--query-type] [logs] [--query] [service:api] [--repos] [OpenHands/software-agent-sdk] [--working-dir] [/tmp/datadog-debug] [--issue-repo] [OpenHands/software-agent-sdk] [--issue-prefix] [QA]
This verifies both safe literal handling and the empty optional-argument path.
Test 3: TODO Management PR payload generation
Step 1 — Establish baseline without the fix:
Generated the old heredoc-style JSON with a title/body containing quotes and a newline:
baseline payload parse: INVALID
jq: parse error: Invalid numeric literal at line 2, column 38
This shows user-controlled text could break the API request body.
Step 2 — Apply the PR's changes:
Generated the payload using jq -n --arg ... with BASE_REF from env.
Step 3 — Re-run with the fix in place:
fixed payload parse: valid
release/base-branch
Resolve TODO with "quotes" and newline
2
This confirms the PR path produces valid JSON while preserving the intended base branch and multiline body.
Issues Found
- 🟡 Minor: CI is not fully green: two
PR Description Check / Validate PR descriptionruns were failing when checked. I did not rerun or debug CI per QA instructions.
This review was created by an AI agent (OpenHands) on behalf of the user.
|
@OpenHands Address these issues #3372 (review) |
|
I'm on it! enyst can track my progress at all-hands.dev |
Co-authored-by: openhands <openhands@all-hands.dev>
enyst
added
the
review-this
OpenHands AI
|
Addressed the requested workflow hardening follow-ups in d863eff:
Validation: YAML parsing passed for the edited workflow files, CI is green on d863eff (36 passing checks, skipped jobs only), and I re-applied the This comment was created by an AI agent (OpenHands) on behalf of the user. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
|
OpenHands encountered an error: **Failed to send message to agent server: HTTP 503 error: no available server See the conversation for more information. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: Harden workflow GitHub context handling
🟢 Good taste — Elegant, simple security hardening
This PR addresses a real security issue (CWE-78 shell injection) by moving attacker-influenced GitHub context values from direct ${{ }} interpolation in run: blocks into step env: blocks. The changes are consistent, comprehensive across 13 workflow files, and follow established patterns.
Analysis
Pattern Consistency: The PR establishes a uniform pattern across all modified files:
- Define env variables at step level:
env: VAR_NAME_INPUT: ${{ github.event.inputs.X }} - Reference via shell:
VAR_NAME="$VAR_NAME_INPUT" - Use the local shell variable in subsequent logic
This is the correct approach for preventing command injection in GitHub Actions workflows.
Key Observations:
| Pattern | Assessment |
|---|---|
| Env indirection | ✅ Correct - prevents direct shell interpolation |
Heredoc output with random delimiters (openssl rand -hex 16) |
✅ Elegant - prevents delimiter collision |
jq -n --arg for JSON construction |
✅ Good - eliminates heredoc JSON escaping issues |
printf '%s' instead of echo |
✅ Correct - avoids echo's interpretation of flags and special chars |
| Numeric input validation | ✅ Proportional - only where needed (version format) |
Empty-conditional array building (ISSUE_PARENT_ARG=(--issue-parent "$INPUT")) |
✅ Clean - handles optional arguments elegantly |
No Critical Issues Found
The security fix is complete and well-executed. All 12 workflow files have been updated consistently.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
This is a pure workflow hardening PR with no code logic changes. All CI checks pass. The changes are defensive (security hardening) rather than introducing new functionality, so the risk of regression is minimal.
VERDICT:
✅ Worth merging: Solid security fix with consistent, well-executed patterns across all workflow files.
KEY INSIGHT:
The write_output() function with random delimiters is the right abstraction — it handles the multi-line output case that $GITHUB_OUTPUT requires while preventing injection through the delimiter itself.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
3 participants
HUMAN:
This PR proposes a few tweaks to avoid interpolation of user-controlled data in GitHub context.
AGENT:
Why
Fixes #3371. GitHub Actions run blocks should not interpolate attacker-influenced GitHub context, workflow inputs, or derived outputs directly into shell scripts.
Summary
run:interpolation into stepenv:blocks.${{ ... }}expansion.Issue Number
Fixes #3371
How to Test
python+PyYAMLvalidation over all changed workflow/action YAML files across the audited repositories:validated changed yaml files: 33remaining suspicious run blocks: 0git diff --checkacross all audited repositoriesuv run pre-commit run --files $(git diff --name-only)inOpenHands/software-agent-sdk(yamlfmt passed; Python hooks skipped because only workflow files changed)Video/Screenshots
Not applicable; workflow hardening only.
Type
Notes
This PR was created by an AI agent (OpenHands) on behalf of the user.
@enyst can click here to continue refining the PR
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimgolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:d863eff-pythonRun
All tags pushed for this build
About Multi-Architecture Support
d863eff-python) is a multi-arch manifest supporting both amd64 and arm64d863eff-python-amd64) are also available if needed