fix(action): address review findings - validation hardening and cleanup
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 16s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 38s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 59s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m14s
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 16s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 38s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 59s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m14s
Addresses findings from reviews #3655 (sonnet), #3657 (security), #3658 (gpt): - Add set -euo pipefail to both script steps for fail-fast behavior - Remove redundant newline check ([:space:] already covers it) - Simplify VERSION regex: remove non-portable \n\r in POSIX ERE - Add ACTION_TOKEN control character validation (defense-in-depth) - Anchor checksum grep to exact filename match (prevent substring collision) - Add ::notice:: when falling back to default ACTION_REPO - Translate Chinese comments to English for consistency - Add comment linking GITHUB_API_URL usage back to VCS detection
This commit is contained in:
@@ -131,6 +131,8 @@ runs:
|
||||
id: version
|
||||
shell: bash
|
||||
run: |
|
||||
set -euo pipefail
|
||||
|
||||
# --- Input Validation ---
|
||||
|
||||
# Determine the repo hosting review-bot releases (not the repo being reviewed)
|
||||
@@ -142,9 +144,10 @@ runs:
|
||||
if [ -z "$ACTION_REPO" ]; then
|
||||
# Final fallback for Gitea (which may not set action_repository)
|
||||
ACTION_REPO="rodin/review-bot"
|
||||
echo "::notice::action-repo not specified and github.action_repository is empty; falling back to rodin/review-bot"
|
||||
fi
|
||||
|
||||
# Validate ACTION_REPO matches owner/repo pattern (防止 path traversal)
|
||||
# Validate ACTION_REPO matches owner/repo pattern (prevent path traversal)
|
||||
if ! printf '%s' "$ACTION_REPO" | grep -qE '^[a-zA-Z0-9._-]+/[a-zA-Z0-9._-]+$'; then
|
||||
echo "Error: action-repo '${ACTION_REPO}' does not match expected owner/repo format" >&2
|
||||
exit 1
|
||||
@@ -174,16 +177,13 @@ runs:
|
||||
# Strip trailing slash if present
|
||||
SERVER_URL="${SERVER_URL%/}"
|
||||
|
||||
# Validate SERVER_URL for Gitea path: must be https, no whitespace/newlines
|
||||
# Validate SERVER_URL for Gitea path: must be https, no whitespace/newlines.
|
||||
# The [^[:space:]] class already rejects newlines, so no separate newline check needed.
|
||||
if [ "$VCS_TYPE" = "gitea" ]; then
|
||||
if ! printf '%s' "$SERVER_URL" | grep -qE '^https://[^[:space:]]+$'; then
|
||||
echo "Error: SERVER_URL '${SERVER_URL}' must be an https:// URL with no whitespace" >&2
|
||||
exit 1
|
||||
fi
|
||||
if printf '%s' "$SERVER_URL" | grep -q $'\n'; then
|
||||
echo "Error: SERVER_URL contains unexpected newline" >&2
|
||||
exit 1
|
||||
fi
|
||||
fi
|
||||
|
||||
# Determine auth token for release API requests
|
||||
@@ -196,6 +196,14 @@ runs:
|
||||
fi
|
||||
fi
|
||||
|
||||
# Validate token contains no control characters (defense-in-depth against header injection)
|
||||
if [ -n "$ACTION_TOKEN" ]; then
|
||||
if printf '%s' "$ACTION_TOKEN" | grep -qP '[\x00-\x1f\x7f]'; then
|
||||
echo "Error: ACTION_TOKEN contains control characters" >&2
|
||||
exit 1
|
||||
fi
|
||||
fi
|
||||
|
||||
if [ "${{ inputs.version }}" = "latest" ]; then
|
||||
if [ "$VCS_TYPE" = "github" ]; then
|
||||
# SECURITY: Use github.api_url which is a trusted platform-provided value.
|
||||
@@ -233,8 +241,9 @@ runs:
|
||||
VERSION="${{ inputs.version }}"
|
||||
fi
|
||||
|
||||
# Validate VERSION: no newlines, no slashes, no whitespace (防止 path traversal)
|
||||
if printf '%s' "$VERSION" | grep -qE '[\n\r/[:space:]]'; then
|
||||
# Validate VERSION: no slashes or whitespace (prevent path traversal).
|
||||
# [:space:] includes newlines and carriage returns in POSIX.
|
||||
if printf '%s' "$VERSION" | grep -qE '[/[:space:]]'; then
|
||||
echo "Error: VERSION '${VERSION}' contains invalid characters (newline, slash, or whitespace)" >&2
|
||||
exit 1
|
||||
fi
|
||||
@@ -262,6 +271,8 @@ runs:
|
||||
if: steps.cache.outputs.cache-hit != 'true'
|
||||
shell: bash
|
||||
run: |
|
||||
set -euo pipefail
|
||||
|
||||
SERVER_URL="${{ steps.version.outputs.server_url }}"
|
||||
ACTION_REPO="${{ steps.version.outputs.action_repo }}"
|
||||
VERSION="${{ steps.version.outputs.version }}"
|
||||
@@ -276,6 +287,7 @@ runs:
|
||||
# Web release URLs ({server}/.../releases/download/{tag}/{asset}) redirect
|
||||
# to S3 and don't reliably support Authorization headers for private repos.
|
||||
# The REST API endpoint with Accept: application/octet-stream is required.
|
||||
# GITHUB_API_URL: trusted platform value, same as detected in "Determine version" step.
|
||||
GITHUB_API_URL="${{ github.api_url }}"
|
||||
|
||||
if [ -n "$ACTION_TOKEN" ]; then
|
||||
@@ -365,7 +377,7 @@ else:
|
||||
# authenticity — both binary and checksums come from the same server.
|
||||
# For stronger guarantees, consider GPG signature verification.
|
||||
cd "${{ runner.temp }}"
|
||||
EXPECTED=$(grep "${BINARY}" checksums.txt | awk '{print $1}')
|
||||
EXPECTED=$(grep -E "^[0-9a-f]+[[:space:]]+\*?${BINARY}$" checksums.txt | awk '{print $1}')
|
||||
ACTUAL=$(sha256sum review-bot | awk '{print $1}')
|
||||
|
||||
if [ -z "$EXPECTED" ]; then
|
||||
|
||||
Reference in New Issue
Block a user