From 220f6e73697e948622919af2e656e74cf6fd9c3b Mon Sep 17 00:00:00 2001
From: claw <claw@weiker.me>
Date: Wed, 13 May 2026 21:54:08 -0700
Subject: [PATCH] fix(action): address review findings - validation hardening
 and cleanup

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
---
 .gitea/actions/review/action.yml | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/.gitea/actions/review/action.yml b/.gitea/actions/review/action.yml
index 4e275da..1859d41 100644
--- a/.gitea/actions/review/action.yml
+++ b/.gitea/actions/review/action.yml
@@ -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