Skip to content

Commit 5fff4b6

Browse files
committed
fix: address code review findings
- Add --paginate flag to consumer repo search to handle >100 repos - Add validation for SHA-pinned refs without version comments - Replace unsafe heredoc PR body with printf to prevent command injection - Add trap-based cleanup for temp directories to prevent resource leaks - Move ROOT_COMMENT_ID validation to shared step for both auth paths Fixes identified in strict code review: - HIGH: Consumer repo pagination truncation - MEDIUM: Command injection via FILE_PATH in PR body - MEDIUM: Weak validation pattern for SHA pinning - MEDIUM: Temp directory leaks on errors - MEDIUM: Missing ROOT_COMMENT_ID validation in authorized path
1 parent f02e258 commit 5fff4b6

File tree

2 files changed

+39
-16
lines changed

2 files changed

+39
-16
lines changed

.github/workflows/release.yml

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,15 @@ jobs:
144144
exit 1
145145
fi
146146
147+
# Check for any SHA-pinned refs that lack version comments (won't be auto-updated)
148+
UNVERSIONED=$(grep -n 'uses: *docker/cagent-action[^@]*@[a-f0-9]\{40\}' "${PINNED_FILES[@]}" 2>/dev/null | \
149+
grep -v '# v' || true)
150+
if [ -n "$UNVERSIONED" ]; then
151+
echo "::error::Found SHA-pinned refs without version comments (won't be auto-updated):"
152+
echo "$UNVERSIONED"
153+
exit 1
154+
fi
155+
147156
# Verify no refs with the old SHA remain in pinned files
148157
REMAINING=$(grep -n "$OLD_PIN_PATTERN" "${PINNED_FILES[@]}" 2>/dev/null | grep -v "${HEAD_SHA}" || true)
149158
if [ -n "$REMAINING" ]; then
@@ -219,7 +228,7 @@ jobs:
219228
# Uses GitHub code search API — results may have eventual consistency lag
220229
# for very recently created repos, but this is fine for release automation.
221230
echo "Searching for consumer repos..."
222-
REPOS=$(gh api '/search/code?per_page=100' \
231+
REPOS=$(gh api --paginate '/search/code?per_page=100' \
223232
-f q='org:docker "docker/cagent-action/.github/workflows/review-pr.yml@" language:YAML path:/^\.github\/workflows\//' \
224233
--jq '[.items[] | {repo: .repository.full_name, path: .path}] | unique_by(.repo) | .[] | "\(.repo) \(.path)"')
225234
@@ -245,6 +254,9 @@ jobs:
245254
echo "=========================================="
246255
247256
# Clone the repo into a temp directory
257+
# Set up cleanup trap for this iteration
258+
trap 'cd /; rm -rf "$WORK_DIR"' EXIT
259+
248260
WORK_DIR=$(mktemp -d)
249261
if ! gh repo clone "$REPO" "$WORK_DIR" -- --depth=1 2>/dev/null; then
250262
echo "::warning::Failed to clone $REPO — skipping (token may lack access)"
@@ -302,11 +314,14 @@ jobs:
302314
# Create or update PR
303315
EXISTING_PR=$(gh pr list --repo "$REPO" --head "$BRANCH" --state open --json number --jq '.[0].number')
304316
305-
PR_BODY="## Summary
306-
Updates \`cagent-action\` reference in \`${FILE_PATH}\` to [$VERSION]($RELEASE_URL).
307-
- **Commit**: \`${SHA}\`
308-
- **Version**: \`${VERSION}\`
309-
> Auto-generated by the [release](${RUN_URL}) workflow."
317+
# Build PR body safely using printf to avoid shell expansion of FILE_PATH
318+
# FILE_PATH comes from GitHub API and could theoretically contain shell metacharacters
319+
printf -v PR_BODY '%s\n%s\n%s\n%s\n%s' \
320+
"## Summary" \
321+
"Updates \`cagent-action\` reference in \`${FILE_PATH}\` to [${VERSION}](${RELEASE_URL})." \
322+
"- **Commit**: \`${SHA}\`" \
323+
"- **Version**: \`${VERSION}\`" \
324+
"> Auto-generated by the [release](${RUN_URL}) workflow."
310325
311326
if [ -n "$EXISTING_PR" ]; then
312327
echo "Updating existing PR #$EXISTING_PR in $REPO"
@@ -322,6 +337,9 @@ jobs:
322337
--reviewer "derekmisler" || echo "::warning::Failed to create PR in $REPO"
323338
fi
324339
340+
# Clear trap and cleanup
341+
trap - EXIT
342+
325343
cd /
326344
rm -rf "$WORK_DIR"
327345
echo ""

.github/workflows/review-pr.yml

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -457,6 +457,21 @@ jobs:
457457
echo "⏭️ Not a reply to agent comment, skipping"
458458
fi
459459
460+
- name: Validate root comment ID
461+
if: steps.check.outputs.is_agent == 'true'
462+
shell: bash
463+
env:
464+
ROOT_COMMENT_ID: ${{ steps.check.outputs.root_comment_id }}
465+
run: |
466+
if [ -z "$ROOT_COMMENT_ID" ]; then
467+
echo "::error::ROOT_COMMENT_ID is not set"
468+
exit 1
469+
fi
470+
if ! [[ "$ROOT_COMMENT_ID" =~ ^[0-9]+$ ]]; then
471+
echo "::error::ROOT_COMMENT_ID is not a valid integer: '$ROOT_COMMENT_ID'"
472+
exit 1
473+
fi
474+
460475
- name: Check authorization
461476
if: steps.check.outputs.is_agent == 'true'
462477
id: auth
@@ -509,16 +524,6 @@ jobs:
509524
ROOT_COMMENT_ID: ${{ steps.check.outputs.root_comment_id }}
510525
AUTHOR: ${{ github.event.comment.user.login }}
511526
run: |
512-
# Validate ROOT_COMMENT_ID is a valid integer before using it
513-
if [ -z "$ROOT_COMMENT_ID" ]; then
514-
echo "::error::ROOT_COMMENT_ID is not set"
515-
exit 1
516-
fi
517-
518-
if ! [[ "$ROOT_COMMENT_ID" =~ ^[0-9]+$ ]]; then
519-
echo "::error::ROOT_COMMENT_ID is not a valid integer: '$ROOT_COMMENT_ID'"
520-
exit 1
521-
fi
522527
523528
jq -n \
524529
--arg body "Sorry @$AUTHOR, conversational replies are currently available to repository collaborators only. Your feedback has still been captured and will be used to improve future reviews.

0 commit comments

Comments
 (0)