fix(action): detect VCS host type for version resolution and binary download #121
sonnet-review-bot
security-review-bot
gpt-review-bot
aweiker
Reference in New Issue
Block a user
Delete Branch "review-bot-issue-120"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Fixes #120 — the composite action hardcoded Gitea-specific assumptions that broke version resolution and binary download when running on GitHub Enterprise Server.
Changes
action-repoinput — specifies the repo hosting review-bot releases, separate fromrepo(the target repo being reviewed). Defaults togithub.action_repositorycontext variable, withrodin/review-botas final fallback for backward compatibility./api/v3/releases API; otherwise →/api/v1/)/api/v3/repos/{owner}/{repo}/releases?per_page=1; Gitea:/api/v1/repos/{owner}/{repo}/releases?limit=1server_urlandaction_repoare passed between steps viaGITHUB_OUTPUTto avoid redundant computationToken for posting the reviewinstead ofGitea token...)Backward Compatibility
action-repoinput continue to work (falls back torodin/review-bot)gitea-urlinput name preserved (just updated description)Testing
/api/v1/path → existing behavior preservedgithub.concur.com): URL contains "github" → uses/api/v3/path;github.action_repositoryprovides correct repo automaticallyOriginal reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
d4d34aa0)Sonnet Review
Summary
CI passed. The change correctly adds VCS host detection, a new
action-repoinput, and step output reuse. The logic is sound and backward-compatible. A few minor issues worth noting but none blocking.Findings
.gitea/actions/review/action.yml/api/v3/only for GitHub Enterprise Server. On github.com the correct base URL ishttps://api.github.com, nothttps://github.com/api/v3/. So constructing${SERVER_URL}/api/v3/repos/...will producehttps://github.com/api/v3/repos/...which is a 404 on public GitHub. This is fine as long as this action is only ever hosted/running on GHES and Gitea (never on github.com itself), but the comment 'github.com uses /api/v3/ too' is incorrect and could mislead future maintainers..gitea/actions/review/action.ymlgrep -qi 'github') would misclassify a Gitea instance whose hostname happens to contain the word 'github' (e.g.,github.internal-infra.corp). This is an edge case, but since the detection is heuristic and there is already a user-facingaction-repoinput, consider also providing an explicitvcs-typeinput or at least documenting the limitation..gitea/actions/review/action.ymlRun reviewstep still usesGITEA_URLandGITEA_REPOas env var names. While this is a backward-compatibility concern for the review-bot binary itself (not introduced by this PR), it is slightly inconsistent with the now host-agnostic framing of the action. Not a blocker, but consider aliasing these toVCS_URL/VCS_REPOif the binary ever grows support for that.Recommendation
APPROVE — Approve with the understanding that the github.com API URL issue (MINOR) is a known limitation for public GitHub use — if this action will only ever run on GHES or Gitea, the current logic is correct. The comment claiming 'github.com uses /api/v3/ too' should be corrected to note that the
/api/v3/path works on GHES only, not on github.com (which usesapi.github.com). The hostname heuristic is acceptable given the real-world deployment targets. No blocking issues.Review by sonnet
Evaluated against
d4d34aa0@@ -117,0 +132,4 @@fi# Detect VCS host type from server URL# GitHub/GHES URLs contain "github" in the hostname[MINOR] The hostname-based VCS detection (
grep -qi 'github') would misclassify a Gitea instance whose hostname happens to contain the word 'github' (e.g.,github.internal-infra.corp). This is an edge case, but since the detection is heuristic and there is already a user-facingaction-repoinput, consider also providing an explicitvcs-typeinput or at least documenting the limitation.@@ -119,0 +145,4 @@API_URL="${SERVER_URL}/api/v3/repos/${ACTION_REPO}/releases?per_page=1"else# Gitea APIAPI_URL="${SERVER_URL}/api/v1/repos/${ACTION_REPO}/releases?limit=1"[MINOR] The GitHub.com releases API endpoint is
/api/v3/only for GitHub Enterprise Server. On github.com the correct base URL ishttps://api.github.com, nothttps://github.com/api/v3/. So constructing${SERVER_URL}/api/v3/repos/...will producehttps://github.com/api/v3/repos/...which is a 404 on public GitHub. This is fine as long as this action is only ever hosted/running on GHES and Gitea (never on github.com itself), but the comment 'github.com uses /api/v3/ too' is incorrect and could mislead future maintainers.Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
d4d34aa0)Gpt Review
Summary
Good direction: separates action-repo from reviewed repo and adds host-aware API paths. However, the GitHub API URL construction is incorrect for github.com and the host detection is brittle, which will break version resolution on many GitHub installations.
Findings
.gitea/actions/review/action.yml.gitea/actions/review/action.yml.gitea/actions/review/action.yml.gitea/actions/review/action.yml.gitea/actions/review/action.ymlRecommendation
REQUEST_CHANGES — The refactor is on the right track, but please address API selection and detection to avoid breaking GitHub usage. Specifically:
Replace the GitHub API URL construction to use the github.api_url context when available:
This works for both github.com (https://api.github.com) and GHES (https:///api/v3).
Only fall back to SERVER_URL + "/api/v1" for Gitea when github.api_url is not present.
Improve host detection logic by using the presence of github.api_url to select GitHub mode (or add an explicit input to override), instead of grepping for "github" in the hostname. Optionally, implement a fallback strategy: try GitHub API first if github.api_url exists, else try Gitea API.
Update the misleading comment about github.com using /api/v3.
Consider adding optional authentication to the curl calls for both the release metadata and asset downloads, using ${{ github.token }} on GitHub/GHES and/or reusing reviewer-token when applicable, to support private repositories.
With these changes, the action will robustly support github.com, GHES, and Gitea, and avoid regressions in version resolution and binary download.
Review by gpt
Evaluated against
d4d34aa0@@ -117,0 +130,4 @@# Final fallback for Gitea (which may not set action_repository)ACTION_REPO="rodin/review-bot"fi[MAJOR] Brittle VCS host detection: checking if SERVER_URL contains "github" can misclassify GHES instances (e.g., ghe.example.com or vcs.company.com). Prefer using the presence of the github.api_url context to choose the GitHub path, and fall back to Gitea otherwise, or implement a try-fallback between /api/v3 and /api/v1.
@@ -117,0 +138,4 @@elseVCS_TYPE="gitea"fi[MINOR] Comment claims "github.com uses /api/v3/ too" which is inaccurate; the public GitHub API base is https://api.github.com. Update the comment to avoid confusion.
@@ -117,1 +139,4 @@VCS_TYPE="gitea"fiif [ "${{ inputs.version }}" = "latest" ]; then[MAJOR] Incorrect GitHub API base URL: building API as "${SERVER_URL}/api/v3/..." will fail on github.com (whose v3 API is at https://api.github.com). Use the github.api_url context (e.g., "${{ github.api_url }}/repos/..."), which is correct for both github.com and GHES.
@@ -119,0 +145,4 @@API_URL="${SERVER_URL}/api/v3/repos/${ACTION_REPO}/releases?per_page=1"else# Gitea APIAPI_URL="${SERVER_URL}/api/v1/repos/${ACTION_REPO}/releases?limit=1"[MINOR] Release API requests are unauthenticated; this will fail for private repos or GHES instances requiring auth. Consider adding an Authorization header when a token is available (e.g., use ${{ github.token }} on GitHub/GHES or reuse reviewer-token when appropriate).
@@ -142,2 +175,4 @@SERVER_URL="${{ steps.version.outputs.server_url }}"ACTION_REPO="${{ steps.version.outputs.action_repo }}"VERSION="${{ steps.version.outputs.version }}"BINARY="review-bot-linux-amd64"[MINOR] Release asset downloads are unauthenticated; private repositories on GitHub/GHES will require Authorization for asset download URLs. Consider using a token header when available to support private action-repo setups.
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
d4d34aa0)Security Review
Summary
Security-focused changes look sound and improve host-agnostic behavior without introducing obvious vulnerabilities. No critical issues found; a few hardening opportunities remain around integrity verification, output handling, and network timeouts.
Findings
.gitea/actions/review/action.yml.gitea/actions/review/action.yml.gitea/actions/review/action.yml.gitea/actions/review/action.ymlRecommendation
APPROVE — Overall, the changes correctly detect the VCS host and adjust API endpoints, and the use of step outputs to pass computed values is sensible. To strengthen security, add explicit curl timeouts for both the version resolution and binary download steps to prevent hangs; adopt the safe heredoc form for writing to $GITHUB_OUTPUT to avoid potential output injection or corruption from unexpected newlines; and consider stronger authenticity checks for downloaded binaries (e.g., signature verification with a pinned key or pinned hashes), since verifying a checksum downloaded from the same source does not protect against a compromised host. With these hardening tweaks, the action will be more robust and secure.
Review by security
Evaluated against
d4d34aa0@@ -119,0 +148,4 @@API_URL="${SERVER_URL}/api/v1/repos/${ACTION_REPO}/releases?limit=1"fiVERSION=$(curl -sSf "$API_URL" \[MINOR] curl requests to the releases API lack explicit timeouts; a malicious or misbehaving endpoint could hold connections open and stall the job (DoS). Consider adding --connect-timeout and --max-time to bound resource usage.
@@ -124,4 +157,4 @@elseVERSION="${{ inputs.version }}"fiecho "version=${VERSION}" >> "$GITHUB_OUTPUT"[MINOR] Directly echoing unvalidated values to $GITHUB_OUTPUT can be brittle if values ever contain newlines, potentially corrupting outputs or enabling output injection in edge cases. Use the documented multiline syntax with a unique EOF marker to safely write outputs (applies to lines 160–162).
@@ -145,1 +180,3 @@curl -sSfL "${GITEA_URL}/${REPO}/releases/download/${VERSION}/${BINARY}" \# Download URL format is the same on both Gitea and GitHub:# {server}/{owner}/{repo}/releases/download/{tag}/{asset}curl -sSfL "${SERVER_URL}/${ACTION_REPO}/releases/download/${VERSION}/${BINARY}" \[MINOR] Binary and checksum downloads use curl without timeouts; a slow or hostile server could stall execution. Add curl --connect-timeout and --max-time (and optionally retry limits) for resilience. This applies to both lines 182 and 184.
@@ -146,2 +182,3 @@curl -sSfL "${SERVER_URL}/${ACTION_REPO}/releases/download/${VERSION}/${BINARY}" \-o "${{ runner.temp }}/review-bot"curl -sSfL "${GITEA_URL}/${REPO}/releases/download/${VERSION}/checksums.txt" \curl -sSfL "${SERVER_URL}/${ACTION_REPO}/releases/download/${VERSION}/checksums.txt" \[MINOR] Integrity verification relies on a checksums.txt fetched from the same server as the binary, which does not provide authenticity if the server or repo is compromised. Consider stronger verification (e.g., GPG signatures with a pinned public key, or pinning expected hashes in the workflow) or restricting downloads to trusted hosts.
Fix Plan against
d4d34aa029:Findings Summary & Approach
MAJOR Findings
github.api_urlcontext, notSERVER_URL/api/v3${{ github.api_url }}when available; fall back toSERVER_URL/api/v1for Giteagithub.api_urlis non-empty (it's only set on GitHub/GHES runners)MINOR Findings
/api/v3/Authorizationheader using${{ github.token }}on GitHub andreviewer-tokenon Gitea--connect-timeout 10 --max-time 30--connect-timeout 10 --max-time 120$GITHUB_OUTPUTNIT Findings
GITEA_URL/GITEA_REPOenv var namingDesign Details
VCS Detection (MAJOR #1 & #2):
Instead of grepping for "github" in the hostname, use the
github.api_urlcontext variable as the detection signal:github.api_urlis always set on GitHub Actions and GHES (e.g.,https://api.github.comorhttps://ghes.example.com/api/v3)github.api_urldirectly when set (already contains the correct base), fall back toSERVER_URL/api/v1for GiteaAuthentication (MINOR #4 & #5):
Add a new input
action-repo-tokenthat defaults to${{ github.token }}on GitHub. On Gitea, fall back toreviewer-tokenfor private repos. Pass as-H "Authorization: token ..."only if token is non-empty. This keeps public repos working without auth.Safe GITHUB_OUTPUT writing (MINOR #9):
Replace:
With:
Since these are single-line values from controlled sources (API responses, input defaults), the simple form is actually safe here. But to be defensive, validate that VERSION doesn't contain newlines before writing.
Review Feedback Addressed (
646497d)All findings from the three reviews have been addressed:
gpt MAJOR #1 + sonnet MINOR #1: GitHub API URL
Fixed. Now uses
${{ github.api_url }}directly (which resolves tohttps://api.github.comon github.com andhttps://<host>/api/v3on GHES). No more manual URL construction withSERVER_URL/api/v3.gpt MAJOR #2 + sonnet MINOR #2: Brittle VCS host detection
Fixed. Detection now checks whether
${{ github.api_url }}is non-empty (only set on GitHub/GHES runners). This is a reliable, non-heuristic signal — no hostname grepping.gpt MINOR #3: Misleading comment
Fixed. Removed the incorrect comment about github.com using
/api/v3/. Replaced with accurate documentation aboutgithub.api_urlbehavior.gpt MINOR #4 + #5: Unauthenticated API/download requests
Fixed. Added new
action-repo-tokeninput with smart defaults:${{ github.token }}(Bearer auth)${{ inputs.reviewer-token }}(token auth)security MINOR #1 + #2: curl timeouts
Fixed. All curl calls now include
--connect-timeout 10and--max-time 30(API) or--max-time 120(binary download).security MINOR #3: Integrity verification from same server
Deferred. Added a NOTE comment explaining the limitation. True fix requires GPG signature verification with pinned keys, which is out of scope for this PR.
security MINOR #4: Unvalidated GITHUB_OUTPUT writes
Fixed. Added newline validation on VERSION before writing to
$GITHUB_OUTPUT. Other outputs (ACTION_REPO, SERVER_URL) come from controlled sources (inputs/context variables) that cannot contain newlines.sonnet NIT #3: GITEA_URL/GITEA_REPO env var naming
Deferred. This is a backward-compat concern for the review-bot binary itself, not introduced by this PR. The binary's env var interface should be addressed separately.
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
646497de)Sonnet Review
Summary
This PR correctly extends the composite action to support both Gitea and GitHub/GHES runners by detecting the VCS host type via
github.api_urland routing API calls accordingly. The changes are well-structured, backward-compatible, and the logic is sound. CI passes.Findings
.gitea/actions/review/action.ymlAUTH_ARGSvariable is built (lines 229-235) but never actually used — curl calls in the install step explicitly expand the token inline instead. The dead variable is harmless but is misleading noise and should be removed..gitea/actions/review/action.ymlACTIONS_STEP_DEBUG=true. A comment noting this limitation would be appropriate, or alternatively the token could be re-evaluated from inputs in the install step rather than passed through output..gitea/actions/review/action.ymlVERSIONbut notACTION_REPO,SERVER_URL, orVCS_TYPEwhich are also written to GITHUB_OUTPUT. For defense-in-depth consistency, all four outputs written to GITHUB_OUTPUT should be validated, especiallyACTION_REPOwhich comes from user input..gitea/actions/review/action.yml$VERSIONafter the Python one-liner), the error message includes the API URL which may contain a token-bearing URL in some edge configs. The current setup uses headers so this is fine, but the error message could note which VCS type was being tried for easier debugging.Recommendation
APPROVE — Approve with optional follow-ups. The core logic is correct and well-reasoned: using
github.api_urlpresence to distinguish GitHub/GHES from Gitea is a clean, reliable heuristic. The step-output reuse avoids redundant computation. The two notable issues are: (1) the deadAUTH_ARGSvariable that is computed but never used in the install step — worth cleaning up; (2) writing the auth token to GITHUB_OUTPUT, which is acceptable here but worth a comment acknowledging the debug-log exposure risk. Neither is blocking.Review by sonnet
Evaluated against
646497de@@ -117,0 +132,4 @@ACTION_REPO="${{ github.action_repository }}"fiif [ -z "$ACTION_REPO" ]; then# Final fallback for Gitea (which may not set action_repository)[NIT] When version resolution fails (empty
$VERSIONafter the Python one-liner), the error message includes the API URL which may contain a token-bearing URL in some edge configs. The current setup uses headers so this is fine, but the error message could note which VCS type was being tried for easier debugging.@@ -117,0 +141,4 @@# (https://<host>/api/v3). It is empty/unset on Gitea Actions runners.GITHUB_API_URL="${{ github.api_url }}"if [ -n "$GITHUB_API_URL" ]; thenVCS_TYPE="github"[NIT] The newline injection check only validates
VERSIONbut notACTION_REPO,SERVER_URL, orVCS_TYPEwhich are also written to GITHUB_OUTPUT. For defense-in-depth consistency, all four outputs written to GITHUB_OUTPUT should be validated, especiallyACTION_REPOwhich comes from user input.@@ -120,0 +167,4 @@fi# Build auth header if token is availableAUTH_HEADER=""[MINOR] The action_token is written to GITHUB_OUTPUT (line 170) as plaintext. While this is a composite action (not a reusable workflow) and the value is scoped to this run, tokens in step outputs can appear in debug logs when
ACTIONS_STEP_DEBUG=true. A comment noting this limitation would be appropriate, or alternatively the token could be re-evaluated from inputs in the install step rather than passed through output.@@ -128,1 +204,4 @@echo "server_url=${SERVER_URL}" >> "$GITHUB_OUTPUT"echo "vcs_type=${VCS_TYPE}" >> "$GITHUB_OUTPUT"echo "action_token=${ACTION_TOKEN}" >> "$GITHUB_OUTPUT"[MINOR] The
AUTH_ARGSvariable is built (lines 229-235) but never actually used — curl calls in the install step explicitly expand the token inline instead. The dead variable is harmless but is misleading noise and should be removed.Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
646497de)Gpt Review
Summary
The changes robustly add cross-host support (GitHub/GHES and Gitea) for version resolution and asset download, introduce sane defaults for the releases repo and token, and pass computed values via step outputs for reuse. Overall the scripts look correct and CI is green.
Findings
.gitea/actions/review/action.yml.gitea/actions/review/action.yml.gitea/actions/review/action.ymlRecommendation
APPROVE — The PR achieves its stated goals: it distinguishes between GitHub/GHES and Gitea for release API queries, cleanly defaults the releases repository, and reuses computed values across steps. The use of github.api_url is a reliable discriminator, and the download URLs and headers look correct for both hosts. Minor cleanups recommended are to remove the unused AUTH_ARGS variable, consider adding set -euo pipefail at the start of the bash scripts for robustness, and avoid exposing secrets as step outputs by passing them via environment or within the same step. With these minor adjustments, the action will be even more robust and secure.
Review by gpt
Evaluated against
646497de@@ -112,19 +121,89 @@ runs:id: version[MINOR] The bash scripts do not set defensive shell options (e.g., set -euo pipefail). While curl uses -sSf to fail on HTTP errors, enabling strict mode at the top of the scripts would make failures more explicit and catch unset variables.
@@ -120,0 +168,4 @@# Build auth header if token is availableAUTH_HEADER=""if [ -n "$ACTION_TOKEN" ]; then[MINOR] The action writes the ACTION_TOKEN to step outputs (GITHUB_OUTPUT). Although typically not printed, propagating secrets via outputs can increase exposure risk. Consider passing the token via environment files or keeping it within the same step where possible.
@@ -142,2 +221,4 @@VERSION="${{ steps.version.outputs.version }}"VCS_TYPE="${{ steps.version.outputs.vcs_type }}"ACTION_TOKEN="${{ steps.version.outputs.action_token }}"BINARY="review-bot-linux-amd64"[MINOR] The AUTH_ARGS variable is constructed but never used in the Install step. This is dead code and can be removed to avoid confusion.
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
646497de)Security Review
Summary
The changes improve host detection and API usage, but they introduce a critical risk: secrets (github.token or reviewer-token) can be sent as Authorization headers to an attacker-controlled URL via the gitea-url input. This enables token exfiltration and SSRF from the runner. Additional minor hardening opportunities are present.
Findings
.gitea/actions/review/action.yml.gitea/actions/review/action.yml.gitea/actions/review/action.yml.gitea/actions/review/action.yml.gitea/actions/review/action.ymlRecommendation
REQUEST_CHANGES — Block merging until the secret exfiltration and SSRF vectors are addressed. Concrete recommendations:
These changes will prevent leaking github.token/reviewer-token to attacker-controlled endpoints and reduce SSRF risk while preserving the improved host detection and functionality.
Review by security
Evaluated against
646497de@@ -112,19 +121,89 @@ runs:id: version[MINOR] Insufficient input sanitization: Only VERSION is checked for newlines before writing to GITHUB_OUTPUT. Inputs like action-repo and server_url are written directly to outputs and later interpolated in commands. Add newline and basic format validation (e.g., owner/repo pattern, HTTPS URL, no whitespace) to prevent output corruption or unexpected behavior.
@@ -117,3 +159,2 @@if [ "${{ inputs.version }}" = "latest" ]; thenVERSION=$(curl -sSf "${GITEA_URL}/api/v1/repos/${REPO}/releases?limit=1" \| python3 -c "import sys, json; releases = json.load(sys.stdin); print(releases[0]['tag_name'] if releases else '')")if [ "$VCS_TYPE" = "github" ]; then[MAJOR] Possible secret exfiltration: In the Determine version step, the Authorization header (using github.token on GitHub or reviewer-token on Gitea) is attached to a curl request to API_URL, which for Gitea is constructed from the user-supplied SERVER_URL (inputs.gitea-url). An attacker who can influence inputs.gitea-url (e.g., via a modified workflow or misconfiguration) can cause the runner to send secrets to an arbitrary host, leaking tokens. On GitHub, even if VCS_TYPE=github, allowing inputs.gitea-url to override SERVER_URL creates risk if reused elsewhere.
@@ -120,0 +182,4 @@| python3 -c "import sys, json; releases = json.load(sys.stdin); print(releases[0]['tag_name'] if releases else '')")elseVERSION=$(curl -sSf --connect-timeout 10 --max-time 30 "$API_URL" \| python3 -c "import sys, json; releases = json.load(sys.stdin); print(releases[0]['tag_name'] if releases else '')")[MINOR] Sensitive value in step outputs: The composite action writes action_token to GITHUB_OUTPUT to pass between steps. While not exposed as an action output, avoid persisting secrets as outputs where possible to reduce accidental leakage via logs. Prefer environment files with masking or re-derive the token in subsequent steps.
@@ -137,17 +216,54 @@ runs:if: steps.cache.outputs.cache-hit != 'true'[MINOR] SSRF risk: Allowing inputs.gitea-url and action-repo to control request destinations without validation enables server-side requests to arbitrary/internal addresses from the runner environment. Even without tokens, this can be abused on self-hosted runners to access internal services. Validate scheme/host against an allowlist and block private/link-local/loopback addresses.
@@ -149,0 +238,4 @@DOWNLOAD_URL="${SERVER_URL}/${ACTION_REPO}/releases/download/${VERSION}"if [ -n "$ACTION_TOKEN" ]; thenif [ "$VCS_TYPE" = "github" ]; then[MAJOR] Possible secret exfiltration during binary download: The Install review-bot step builds DOWNLOAD_URL from SERVER_URL (which can be overridden by inputs.gitea-url) and conditionally adds an Authorization header with github.token (Bearer) or reviewer-token (token). This can send repository or reviewer tokens to an attacker-controlled domain if SERVER_URL is manipulated, leading to credential leakage and arbitrary code download.
Fix Plan against
646497de68Addressing all 5 security review findings.
Finding #1 & #2 [MAJOR]: Secret exfiltration via
inputs.gitea-urlRoot cause:
SERVER_URLis computed frominputs.gitea-url || github.server_url. On GitHub/GHES runners, if an attacker supplies a custominputs.gitea-url, it overrides the safegithub.server_urland tokens (github.token or reviewer-token) are sent to the attacker-controlled host via Authorization headers.Fix:
VCS_TYPE=github(detected via non-emptygithub.api_url), always usegithub.server_urlfor downloads andgithub.api_urlfor API calls.inputs.gitea-urlis completely ignored in this path.inputs.gitea-urlin the Gitea path (VCS_TYPE=gitea), where the user controls their own Gitea instance and the token is their own Gitea token.inputs.gitea-urlis provided butVCS_TYPE=github(user may have misconfigured).Finding #3 [MINOR]: SSRF risk
Fix:
SERVER_URL(in Gitea path) useshttps://scheme only. Rejecthttp://,file://, and other schemes.Finding #4 [MINOR]: Sensitive value in step outputs
Fix:
action_tokento$GITHUB_OUTPUT, write it to$GITHUB_ENVwith::add-mask::applied first. This masks the value from logs.Finding #5 [MINOR]: Insufficient input sanitization
Fix:
ACTION_REPOmatches the pattern^[a-zA-Z0-9._-]+/[a-zA-Z0-9._-]+$(standard owner/repo format).SERVER_URL(in Gitea path) has no newlines, no whitespace, starts withhttps://.VERSION(already done for newlines, extend to block slashes and special chars that could escape URL paths).Implementation Order
action_tokenGITHUB_OUTPUT with GITHUB_ENV + maskingOpen Questions
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
93d89ba6)Sonnet Review
Summary
This PR correctly extends the composite action to support both Gitea and GitHub/GHES runners with proper VCS host detection, input validation, and security hardening. The changes are well-structured, well-documented, and CI is passing. A few minor issues exist but none are blockers.
Findings
.gitea/actions/review/action.ymlgrep -q $'\n'is redundant. The precedinggrep -qE '^https://[^[:space:]]+$'already rejects strings containing newlines because\nis a whitespace character matched by[[:space:]]. The second check adds noise and complexity without adding safety..gitea/actions/review/action.yml[\n\r/[:space:]]uses POSIX ERE character class[[:space:]]combined with explicit\n\r. In bash withgrep -qE,[[:space:]]already includes\nand\r, making those explicit escapes redundant. This is harmless but confusing..gitea/actions/review/action.ymlsteps.version.outputs.server_url(a step output set in the same workflow) againstgithub.server_url. Since step outputs on GitHub Actions are not secret-masked and theserver_urloutput was set directly fromgithub.server_urljust steps earlier, this check can never fail in practice and provides marginal security value. It adds code complexity; a comment explaining why this check is intentionally kept defensive would help future maintainers..gitea/actions/review/action.yml防止 path traversalappears inline in English code comments. While technically harmless, mixing languages in inline comments reduces readability for non-Chinese readers. Consider replacing with# prevent path traversalfor consistency..gitea/actions/review/action.yml--retry) easier to apply consistently.Recommendation
APPROVE — Approve. The implementation correctly solves the stated problem: Gitea-specific assumptions are replaced with proper VCS host detection using the trusted
github.api_urlcontext variable. The security model is sound — on GitHub/GHES, user-suppliedgitea-urlis ignored and all API/download URLs are constructed from platform-provided context values. Token handling via masked environment variables rather than step outputs is the right approach. Input validation (ACTION_REPO pattern, SERVER_URL scheme, VERSION characters) is appropriate. The minor findings are quality improvements but not correctness or security issues.Review by sonnet
Evaluated against
93d89ba6@@ -117,0 +145,4 @@fi# Validate ACTION_REPO matches owner/repo pattern (防止 path traversal)if ! printf '%s' "$ACTION_REPO" | grep -qE '^[a-zA-Z0-9._-]+/[a-zA-Z0-9._-]+$'; then[NIT] Chinese comment fragment
防止 path traversalappears inline in English code comments. While technically harmless, mixing languages in inline comments reduces readability for non-Chinese readers. Consider replacing with# prevent path traversalfor consistency.@@ -117,0 +194,4 @@elseACTION_TOKEN="${{ inputs.reviewer-token }}"fifi[MINOR] The newline check using
grep -q $'\n'is redundant. The precedinggrep -qE '^https://[^[:space:]]+$'already rejects strings containing newlines because\nis a whitespace character matched by[[:space:]]. The second check adds noise and complexity without adding safety.@@ -120,0 +221,4 @@-H "$AUTH_HEADER" "$API_URL" \| python3 -c "import sys, json; releases = json.load(sys.stdin); print(releases[0]['tag_name'] if releases else '')")elseVERSION=$(curl -sSf --connect-timeout 10 --max-time 30 "$API_URL" \[MINOR] The VERSION validation regex
[\n\r/[:space:]]uses POSIX ERE character class[[:space:]]combined with explicit\n\r. In bash withgrep -qE,[[:space:]]already includes\nand\r, making those explicit escapes redundant. This is harmless but confusing.@@ -149,0 +290,4 @@if [ -n "$ACTION_TOKEN" ]; thenif [ "$VCS_TYPE" = "github" ]; thencurl -sSfL --connect-timeout 10 --max-time 120 \[MINOR] The SERVER_URL mismatch check compares the value from
steps.version.outputs.server_url(a step output set in the same workflow) againstgithub.server_url. Since step outputs on GitHub Actions are not secret-masked and theserver_urloutput was set directly fromgithub.server_urljust steps earlier, this check can never fail in practice and provides marginal security value. It adds code complexity; a comment explaining why this check is intentionally kept defensive would help future maintainers.Responses to Security Review Findings
All findings have been addressed in commit
93d89ba. Summary:MAJOR #1 & #2 — Secret exfiltration (comments #21095, #21096)
Fixed: On GitHub/GHES (
VCS_TYPE=github),inputs.gitea-urlis now completely ignored.SERVER_URLis always derived fromgithub.server_url(platform-provided, trusted). API calls usegithub.api_urlexclusively. Tokens are never sent to user-supplied URLs in the GitHub path.The Install step also includes a defense-in-depth integrity check that verifies
SERVER_URLmatchesgithub.server_urlbefore sending any Authorization header.For the Gitea path, the user controls their own instance and the token is their own — but we now validate
SERVER_URLis https-only with no whitespace/newlines.MINOR #3 — SSRF risk (comment #21097)
Partially addressed: URL scheme validation enforces https-only and rejects whitespace/newlines. Token exfiltration (the most severe SSRF consequence) is fully mitigated.
Full IP-level SSRF defense (blocking private/link-local/loopback IPs, DNS rebinding, non-standard ports) is deferred to #123.
MINOR #4 — Sensitive value in step outputs (comment #21098)
Fixed:
action_tokenis no longer written to$GITHUB_OUTPUT. Token is passed via$GITHUB_ENVwith::add-mask::applied first, ensuring it's masked from all log output including debug logs.MINOR #5 — Insufficient input sanitization (comment #21099)
Fixed: Added comprehensive input validation:
ACTION_REPOvalidated against^[a-zA-Z0-9._-]+/[a-zA-Z0-9._-]+$SERVER_URL(Gitea path) must match^https://[^[:space:]]+$VERSIONvalidation blocks slashes, whitespace, and newlinesAll validation happens early with clear error messages before any network requests.
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
93d89ba6)Security Review
Summary
Changes improve security by detecting VCS host type, validating inputs, avoiding user-supplied URLs on GitHub/GHES, and masking tokens. No exploitable vulnerabilities introduced and CI passed.
Recommendation
APPROVE — The PR adds sensible host detection, input validation, and safer token handling while preserving backward compatibility. Curl timeouts and checksum verification further harden the process. No security-critical issues were found in the changes; approve as submitted.
Review by security
Evaluated against
93d89ba6Gpt Review
Summary
The PR significantly improves host detection, input validation, and token handling, but it introduces a security gap and likely breaks private release downloads on GitHub/GHES. Specifically, the run step still honors a user-supplied URL on GitHub (risking token exfiltration), and the download method for private assets uses web URLs that typically ignore Authorization headers on GitHub.
Findings
.gitea/actions/review/action.yml.gitea/actions/review/action.ymlRecommendation
REQUEST_CHANGES — Please address the following before merging:
Prevent token exfiltration on GitHub/GHES during the actual review run:
Make private release downloads work on GitHub/GHES:
These changes will preserve the improved host detection and validation while closing the security gap and ensuring private asset downloads function correctly on GitHub/GHES.
Review by gpt
Evaluated against
93d89ba6@@ -4,0 +2,4 @@# It detects the VCS host type using the github.api_url context (set only on# GitHub/GHES runners) and uses the appropriate releases API for version# resolution and binary download.#[MAJOR] Security note claims inputs.gitea-url is ignored on GitHub/GHES, but the final 'Run review' step still sets GITEA_URL from 'inputs.gitea-url || github.server_url'. On GitHub/GHES, a non-empty inputs.gitea-url will be used, causing the reviewer token (often GITHUB_TOKEN) to be sent to an attacker-controlled host. Use the previously computed steps.version.outputs.server_url to enforce the same trust boundary as earlier steps.
@@ -149,0 +283,4 @@exit 1fifi[MAJOR] Private release asset download on GitHub/GHES is likely to fail: the code uses the web release URL ('{server}/{owner}/{repo}/releases/download/{tag}/{asset}') with an Authorization header. GitHub typically requires downloading assets via the REST API endpoint '/repos/{owner}/{repo}/releases/assets/{asset_id}' with 'Accept: application/octet-stream'. The current approach may work for public repos but will fail for private repos despite the new 'action-repo-token' support.
Fix Plan against
93d89ba662Addressing gpt-review-bot findings from review #3654 on PR #121.
MAJOR #1: Token exfiltration via GITEA_URL in 'Run review' step
Problem: The "Run review" step's
env:block setsGITEA_URL: ${{ inputs.gitea-url || github.server_url }}. On GitHub/GHES, if a maliciousinputs.gitea-urlis supplied, the reviewer token would be sent to an attacker-controlled URL. This contradicts the security notes that claim inputs.gitea-url is ignored on GitHub/GHES.Fix: Replace
${{ inputs.gitea-url || github.server_url }}with${{ steps.version.outputs.server_url }}in the GITEA_URL env var of the "Run review" step. Thesteps.version.outputs.server_urlvalue has already been validated and follows VCS-type-aware trust rules (usesgithub.server_urlon GitHub/GHES, validatedinputs.gitea-urlon Gitea).Change:
MAJOR #2: Private release asset download fails on GitHub/GHES
Problem: The "Install review-bot" step downloads release assets using web URLs like
{server}/{owner}/{repo}/releases/download/{tag}/{asset}with an Authorization header. On GitHub, private repo release assets require the REST API endpoint (/repos/{owner}/{repo}/releases/assets/{asset_id}withAccept: application/octet-stream). The current approach silently fails for private repos despite theaction-repo-tokensupport.Fix: Add a VCS_TYPE branch in the download section:
${GITHUB_API_URL}/repos/{ACTION_REPO}/releases/tags/{VERSION}to find the releasereview-bot-linux-amd64andchecksums.txtfrom theassetsarraycurl -L -H "Authorization: Bearer ${ACTION_TOKEN}" -H "Accept: application/octet-stream" "${GITHUB_API_URL}/repos/{ACTION_REPO}/releases/assets/{asset_id}"Change: Replace the download logic with a conditional block:
The no-auth fallback for public repos is preserved in both branches.
Scope
Only
.gitea/actions/review/action.ymlis modified. No Go code changes needed.Addressing gpt-review-bot findings (review #3654)
MAJOR #1: Token exfiltration via GITEA_URL
Fixed in commit
e709956. The "Run review" step'senv:block now sets:This uses the value computed in the "Determine version" step which:
github.server_url(platform-provided, trusted)inputs.gitea-url(https scheme, no whitespace)inputs.gitea-urlis now truly ignored on GitHub/GHES as documented in the security notes.MAJOR #2: Private release asset download on GitHub/GHES
Fixed in commit
e709956. The download logic is now VCS-type branched:GitHub/GHES path:
GET {api_url}/repos/{owner}/{repo}/releases/tags/{version}GET {api_url}/repos/{owner}/{repo}/releases/assets/{id}withAccept: application/octet-streamheaderGitea path: Retains the existing direct URL approach (
{server}/{owner}/{repo}/releases/download/{tag}/{asset}) which works correctly on Gitea with token auth.Also removed the now-unnecessary SERVER_URL mismatch check since GitHub downloads use
github.api_urldirectly rather than SERVER_URL.Sonnet Review
Summary
This PR correctly extends the composite action to support both Gitea and GitHub/GHES runners. The security model is sound: trusted platform-provided URLs are used on GitHub, user-supplied URLs are validated on Gitea, and tokens are passed via masked env vars. The implementation is thorough and well-documented.
Findings
.gitea/actions/review/action.yml'^https://[^[:space:]]+$'is applied after the newline check that immediately follows it (grep -q $'\n'), but the regex[^[:space:]]already excludes newlines since[:space:]includes\n. The second check is redundant and can never catch anything the first didn't. Consider removing the redundant newline check, or document that it's belt-and-suspenders..gitea/actions/review/action.ymlAUTH_HEADERvariable is constructed as a plain string and passed via-H "$AUTH_HEADER". If the token ever contains shell metacharacters (unlikely for a well-formed token, but possible), this could cause issues. A more robust approach would be to pass the header components separately or use-H "$(printf '%s' ...)". Low risk in practice, but worth noting..gitea/actions/review/action.yml'[\n\r/[:space:]]'uses\nand\rinside a POSIX ERE passed togrep -qE. In POSIX ERE,\nis not a standard escape — it may or may not be interpreted as a literal newline depending on the grep implementation. The literal characters[:space:]already includes newline and carriage return in POSIX, so the\n\rprefixes are either redundant or potentially unreliable. Simplify togrep -qE '[/[:space:]]'or test with$'\n'separately, as was done for the SERVER_URL check above..gitea/actions/review/action.ymlGITHUB_API_URLis re-read from${{ github.api_url }}rather than from the step output${{ steps.version.outputs.vcs_type }}. This is fine and consistent with the security rationale (using a trusted platform-provided value), but it means the GitHub path implicitly relies ongithub.api_urlbeing non-empty whenevervcs_type == github. A comment linking back to the detection logic in the version step would aid future maintainers..gitea/actions/review/action.yml# 防止 path traversal(Chinese: 'prevent path traversal') appears twice. Mixed-language comments in a codebase should be consistent. Either translate to English or keep the Chinese, but mixing them in adjacent lines of the same file can be confusing to contributors who don't read Chinese.Recommendation
APPROVE — The implementation is solid and addresses the stated security concerns well. The VCS detection approach using
github.api_urlis sensible and the token-exfiltration protection reasoning is sound. The MINOR findings are low-risk (a redundant safety check, a potential regex portability issue) and don't block merge. The PR can be approved as-is, with the VERSION regex issue being the most worth fixing in a follow-up since\nin POSIX ERE is implementation-defined.Review by sonnet
Evaluated against
e709956d@@ -117,0 +137,4 @@ACTION_REPO="${{ inputs.action-repo }}"if [ -z "$ACTION_REPO" ]; then# github.action_repository is the repo containing the running actionACTION_REPO="${{ github.action_repository }}"[NIT] The comment
# 防止 path traversal(Chinese: 'prevent path traversal') appears twice. Mixed-language comments in a codebase should be consistent. Either translate to English or keep the Chinese, but mixing them in adjacent lines of the same file can be confusing to contributors who don't read Chinese.@@ -117,0 +173,4 @@fi# Strip trailing slash if presentSERVER_URL="${SERVER_URL%/}"[MINOR] The regex validation
'^https://[^[:space:]]+$'is applied after the newline check that immediately follows it (grep -q $'\n'), but the regex[^[:space:]]already excludes newlines since[:space:]includes\n. The second check is redundant and can never catch anything the first didn't. Consider removing the redundant newline check, or document that it's belt-and-suspenders.@@ -120,0 +215,4 @@AUTH_HEADER="Authorization: token ${ACTION_TOKEN}"fifi[MINOR] The
AUTH_HEADERvariable is constructed as a plain string and passed via-H "$AUTH_HEADER". If the token ever contains shell metacharacters (unlikely for a well-formed token, but possible), this could cause issues. A more robust approach would be to pass the header components separately or use-H "$(printf '%s' ...)". Low risk in practice, but worth noting.@@ -122,3 +229,4 @@echo "Failed to determine latest version from ${API_URL}" >&2exit 1fielse[MINOR] The VERSION validation regex
'[\n\r/[:space:]]'uses\nand\rinside a POSIX ERE passed togrep -qE. In POSIX ERE,\nis not a standard escape — it may or may not be interpreted as a literal newline depending on the grep implementation. The literal characters[:space:]already includes newline and carriage return in POSIX, so the\n\rprefixes are either redundant or potentially unreliable. Simplify togrep -qE '[/[:space:]]'or test with$'\n'separately, as was done for the SERVER_URL check above.@@ -149,0 +295,4 @@if a['name'] == '${BINARY}':print(a['id'])breakelse:[NIT] In the 'Install review-bot' step,
GITHUB_API_URLis re-read from${{ github.api_url }}rather than from the step output${{ steps.version.outputs.vcs_type }}. This is fine and consistent with the security rationale (using a trusted platform-provided value), but it means the GitHub path implicitly relies ongithub.api_urlbeing non-empty whenevervcs_type == github. A comment linking back to the detection logic in the version step would aid future maintainers.Security Review
Summary
Security posture is improved with host detection, stricter input validation, and safer token handling. No exploitable issues found; a few defense-in-depth hardening suggestions remain for Gitea scenarios.
Findings
.gitea/actions/review/action.yml.gitea/actions/review/action.yml.gitea/actions/review/action.yml.gitea/actions/review/action.ymlRecommendation
APPROVE — Overall, the changes meaningfully reduce risk by detecting the VCS host, avoiding use of user-supplied URLs on GitHub, validating inputs, and masking tokens. To further harden Gitea scenarios, restrict inputs.gitea-url to the runner's server host (or an explicit allowlist) to prevent token exfiltration to arbitrary external hosts, scope ACTION_TOKEN to only the download step or unset it before subsequent steps, and perform a minimal validation on the token to block control characters prior to inserting it into headers. Optional but beneficial: add authenticity verification (e.g., GPG signatures) for downloaded assets. With CI passing and no major vulnerabilities identified, this PR is approved.
Review by security
Evaluated against
e709956d@@ -112,19 +131,125 @@ runs:id: version[MINOR] On Gitea runners, inputs.gitea-url is only validated for https and lack of whitespace/newlines, but not restricted to the runner's actual server host. Since ACTION_TOKEN may be sent to SERVER_URL in Authorization headers for release downloads, a misconfigured or malicious override could exfiltrate tokens to an external host. Consider enforcing that SERVER_URL host matches the platform-provided github.server_url host (or an allowlist) and disallow arbitrary external hosts by default.
[MINOR] ACTION_TOKEN is written to GITHUB_ENV, making it available to all subsequent steps (including the 'Run review' step). This expands the blast radius if any later step or invoked tool is compromised. Prefer scoping the token only to the asset download step (e.g., pass via step-local env or unset it immediately after use).
@@ -137,17 +262,108 @@ runs:if: steps.cache.outputs.cache-hit != 'true'[MINOR] Tokens used in HTTP headers are not sanitized for control characters. Although tokens should come from trusted secrets, adding a simple check to ensure ACTION_TOKEN contains no CR/LF or whitespace would prevent potential header injection if an unexpected token source is used.
@@ -149,0 +327,4 @@-o "${{ runner.temp }}/review-bot"curl -sSfL --connect-timeout 10 --max-time 30 \-H "Authorization: Bearer ${ACTION_TOKEN}" \-H "Accept: application/octet-stream" \[NIT] Checksum verification uses checksums.txt fetched from the same release source as the binary, which verifies integrity but not authenticity. Consider adding signature verification (e.g., GPG) or another trust anchor to mitigate a compromised release distribution channel.
Gpt Review
Summary
Solid, security-conscious refactor that cleanly separates the action’s release source from the target repo, detects host type reliably, and uses correct API paths for GitHub/GHES vs Gitea. Token handling and step output reuse are improved. A few minor robustness tweaks could further harden the scripts.
Findings
.gitea/actions/review/action.ymlset -euo pipefail(andshopt -s inherit_errexitif needed) at the top of the bash script to fail fast on unset vars and pipeline errors. This improves robustness of the Determine version step..gitea/actions/review/action.ymlset -euo pipefailto the Install review-bot step for consistent error handling during downloads and checksum verification..gitea/actions/review/action.yml.gitea/actions/review/action.ymlACTION_REPO="rodin/review-bot"(e.g., on Gitea wheregithub.action_repositorymay be unset), consider emitting an::notice::or::warning::to make the implicit fallback visible to users..gitea/actions/review/action.ymlreview-bot-linux-amd64. If arm64 runners or other OS/arch are expected, consider deriving the asset name dynamically fromuname -m/uname -sor an input to improve portability.Recommendation
APPROVE — The changes correctly address the host-detection and API path issues, add safer token and URL handling, and improve reuse between steps. CI has passed and there are no major issues. Consider the suggested minor robustness enhancements (fail-fast bash options, anchored checksum matching, a notice on fallback defaults, and optional arch-aware binary selection) in a follow-up, but these are not blockers. Approve as-is.
Review by gpt
Evaluated against
e709956d@@ -112,19 +131,125 @@ runs:id: versionshell: bashrun: |[NIT] Consider adding
set -euo pipefail(andshopt -s inherit_errexitif needed) at the top of the bash script to fail fast on unset vars and pipeline errors. This improves robustness of the Determine version step.@@ -117,0 +139,4 @@# github.action_repository is the repo containing the running actionACTION_REPO="${{ github.action_repository }}"fiif [ -z "$ACTION_REPO" ]; then[NIT] When falling back to
ACTION_REPO="rodin/review-bot"(e.g., on Gitea wheregithub.action_repositorymay be unset), consider emitting an::notice::or::warning::to make the implicit fallback visible to users.@@ -137,17 +262,108 @@ runs:if: steps.cache.outputs.cache-hit != 'true'[NIT] Similarly, add
set -euo pipefailto the Install review-bot step for consistent error handling during downloads and checksum verification.@@ -146,3 +274,1 @@-o "${{ runner.temp }}/review-bot"curl -sSfL "${GITEA_URL}/${REPO}/releases/download/${VERSION}/checksums.txt" \-o "${{ runner.temp }}/checksums.txt"if [ "$VCS_TYPE" = "github" ]; then[NIT] The binary name is hard-coded to
review-bot-linux-amd64. If arm64 runners or other OS/arch are expected, consider deriving the asset name dynamically fromuname -m/uname -sor an input to improve portability.@@ -149,0 +330,4 @@-H "Accept: application/octet-stream" \"${GITHUB_API_URL}/repos/${ACTION_REPO}/releases/assets/${CHECKSUMS_ASSET_ID}" \-o "${{ runner.temp }}/checksums.txt"else[MINOR] Checksum extraction uses
grep "${BINARY}" checksums.txt | awk '{print $1}', which could match unintended lines if filenames are substrings. Anchor the match (e.g.,grep -E "[[:xdigit:]]+[[:space:]]+\*?${BINARY}$") to ensure only the exact asset line is used.Self-Review: PR #121
Self-review against
e709956d0bPhase 1: Independent Findings
No new blocking findings. The diff is well-structured and achieves its stated goal.
Minor observations (all previously flagged by bot reviewers):
ACTION_TOKENis written to$GITHUB_ENV(available to all steps), thoughRun reviewdoesn't need it — minor over-exposure.grep -q $'\n'aftergrep -qE '^https://[^[:space:]]+$'(newlines already excluded by[[:space:]]).[\n\r/[:space:]]uses\n/\rin POSIX ERE where they are implementation-defined;[[:space:]]already covers them.防止 path traversalappear twice in an English codebase.set -euo pipefailin the bash scripts.review-bot-linux-amd64is hard-coded (limits portability to arm64/other OS).grep "${BINARY}" checksums.txt) could match substring filenames.None of these are introduced by this PR's new logic; all were already identified.
Phase 2: Prior Review Verification
github.api_url, notSERVER_URL/api/v3github.api_urlis used directly for GitHub/GHES API callsgithub.api_urlcontext variableACTION_TOKENderived and used in API calls with correct Bearer/token headerRun reviewstep used rawinputs.gitea-url93d89ba)steps.version.outputs.server_url93d89ba)/releases/assets/{id}with Accept: application/octet-streamd4d34aa)646497de)646497de), sonnet-review-bot (MINOR #2 on646497de)646497de), gpt-review-bot (MINOR #1 on646497de)d4d34aa)Assessment: ✅ Clean
All prior MAJOR findings have been addressed across three review rounds (
d4d34aa→646497de→93d89ba→e709956d). The current HEAD implements correct VCS host detection viagithub.api_url, proper input validation, token masking, GitHub REST API asset downloads, and curl timeouts. All bot reviewers on the current HEAD (sonnet, gpt, security) have approved with only MINOR/NIT remaining observations — none blocking. The PR is ready for merge.Fix Plan against
e709956d0b:Summary
14 findings across 3 reviews. Categorized as: 9 FIX, 3 PUSH BACK, 1 DEFER, 1 NO ACTION (already addressed).
FIX (9 items)
grep -q $'\\n'is redundant since[^[:space:]]already rejects newlines)\\n\\rin POSIX ERE not portable[/[:space:]]—[:space:]already includes newlines/CRset -euo pipefailin "Determine version" stepset -euo pipefailin "Install review-bot" stepgrep -w "${BINARY}$"::notice::annotationPUSH BACK (3 items)
DEFER (1 item)
NO ACTION (1 item)
Responses to review findings (
220f6e7)Fixed (9 items)
[^[:space:]]already rejects newlines\n\rin POSIX ERE not portable[/[:space:]]grep -qPvalidationset -euo pipefailin Determine versionset -euo pipefailin Install::notice::annotationPushed back (3 items)
github.server_url). On Gitea, the reviewer-token is the user's own token to their own server. HTTPS is enforced. Allowlists would break multi-instance setups.Deferred (1 item)
No action (1 item)
Sonnet Review
Summary
This PR significantly improves the composite action by adding proper VCS host detection, security hardening (token masking, input validation, URL validation), and support for both Gitea and GitHub/GHES environments. The changes are well-structured, thoroughly commented, and CI passes.
Findings
.gitea/actions/review/action.yml-H "$AUTH_HEADER"pattern passes a shell variable containing a header value directly to curl. If ACTION_TOKEN somehow contains a newline despite the control-character check, this could inject additional headers. A safer pattern iscurl ... -H "Authorization: Bearer ${ACTION_TOKEN}"inline rather than building it as a variable. This is defense-in-depth only since the control character check should prevent this, but the inline form is more conventional and avoids the vector entirely..gitea/actions/review/action.yml-qPflag (Perl-compatible regex) is used for control character validation. The-Pflag is not available in all environments (e.g., macOS grep, BusyBox grep). While ubuntu-* runners have GNU grep and this works fine in practice, a POSIX-compatible alternative usingtrorLC_ALL=C grep -qPwould be more portable if ever run on non-GNU environments. Low risk given the ubuntu-* constraint stated in the action..gitea/actions/review/action.ymlInstall review-botstep readsACTION_TOKENfrom the environment set by the previous step viaGITHUB_ENV, but does so withACTION_TOKEN="${ACTION_TOKEN:-}". If theDetermine versionstep didn't setACTION_TOKEN(because the token was empty), the variable won't exist inGITHUB_ENVbut also won't be in the step's environment. The:-default handles this correctly. No bug, but a comment noting this dependency on the previous step's env would aid maintainability..gitea/actions/review/action.ymlBINARY_ASSET_IDusessys.exit(1)in theelseclause of theforloop, which will make curl's output silently disappear on failure. Combined with theif [ -z "$BINARY_ASSET_ID" ]check that follows, this creates a confusing double-check (the python exits 1 before the shell check fires). Either rely solely on the shell empty-check (removesys.exit(1)) or rely solely on python's exit code — mixing both is redundant..gitea/actions/review/action.ymlRun reviewstep still usesGITEA_URLas the env var name even though this action now supports GitHub/GHES. This works sincereview-botreadsGITEA_URLand this PR intentionally sets it toserver_url(which is correct for both hosts), but the variable name is misleading. This is a pre-existing naming issue and acceptable to address in a follow-up.Recommendation
APPROVE — Approve. The implementation is solid — the security model is well-reasoned (using trusted platform-provided
github.api_urlandgithub.server_urlon GitHub/GHES, validating user-supplied URLs on Gitea, masking tokens via::add-mask::before writing to GITHUB_ENV), the fallback chain foraction-repois correct, and the checksum verification regex improvement is an improvement over the originalgrep "${BINARY}"which could match partial filenames. The minor findings are low-risk and would be good follow-ups but don't block merging.Review by sonnet
Evaluated against
220f6e73@@ -117,0 +160,4 @@if [ -n "$GITHUB_API_URL" ]; thenVCS_TYPE="github"elseVCS_TYPE="gitea"[MINOR] The grep
-qPflag (Perl-compatible regex) is used for control character validation. The-Pflag is not available in all environments (e.g., macOS grep, BusyBox grep). While ubuntu-* runners have GNU grep and this works fine in practice, a POSIX-compatible alternative usingtrorLC_ALL=C grep -qPwould be more portable if ever run on non-GNU environments. Low risk given the ubuntu-* constraint stated in the action.@@ -117,0 +194,4 @@elseACTION_TOKEN="${{ inputs.reviewer-token }}"fifi[MINOR] The
-H "$AUTH_HEADER"pattern passes a shell variable containing a header value directly to curl. If ACTION_TOKEN somehow contains a newline despite the control-character check, this could inject additional headers. A safer pattern iscurl ... -H "Authorization: Bearer ${ACTION_TOKEN}"inline rather than building it as a variable. This is defense-in-depth only since the control character check should prevent this, but the inline form is more conventional and avoids the vector entirely.@@ -149,0 +330,4 @@exit 1fi# Download assets via REST API with Accept: application/octet-stream[NIT] The Python inline script for extracting
BINARY_ASSET_IDusessys.exit(1)in theelseclause of theforloop, which will make curl's output silently disappear on failure. Combined with theif [ -z "$BINARY_ASSET_ID" ]check that follows, this creates a confusing double-check (the python exits 1 before the shell check fires). Either rely solely on the shell empty-check (removesys.exit(1)) or rely solely on python's exit code — mixing both is redundant.@@ -169,7 +397,7 @@ runs:- name: Run review[NIT] The
Run reviewstep still usesGITEA_URLas the env var name even though this action now supports GitHub/GHES. This works sincereview-botreadsGITEA_URLand this PR intentionally sets it toserver_url(which is correct for both hosts), but the variable name is misleading. This is a pre-existing naming issue and acceptable to address in a follow-up.Gpt Review
Summary
The changes cleanly introduce host detection and proper API paths for GitHub/GHES vs Gitea, add validation, and improve security by masking tokens. CI passed and the logic appears correct and robust.
Findings
.gitea/actions/review/action.yml.gitea/actions/review/action.yml.gitea/actions/review/action.ymlRecommendation
APPROVE — Overall, the PR improves cross-host compatibility, secures token handling by avoiding user-supplied URLs on GitHub, and adds useful validation. With CI passing and no major issues, it is suitable to merge. As minor follow-ups, replace grep -P with a POSIX-safe check for control characters, align the header comment with the implemented detection approach, and consider making the checksum matching a bit more tolerant to alternative formats.
Review by gpt
Evaluated against
220f6e73@@ -1,17 +1,36 @@# This composite action is designed for Gitea Actions runners.# Gitea Actions supports GitHub Actions syntax including $GITHUB_OUTPUT,# actions/cache, and actions/checkout.# This composite action supports both Gitea Actions and GitHub Actions runners.[MINOR] Top-of-file comment mentions detecting VCS host type via 'github.api_url' which differs from the PR summary referencing hostname containing 'github'. While the implementation is fine, aligning documentation with the actual detection method will reduce confusion.
@@ -117,0 +177,4 @@# Strip trailing slash if presentSERVER_URL="${SERVER_URL%/}"# Validate SERVER_URL for Gitea path: must be https, no whitespace/newlines.[MINOR] Uses grep -P to detect control characters in ACTION_TOKEN. The -P (PCRE) option isn't universally available on all runner environments; consider a POSIX-compatible alternative to improve portability.
@@ -149,0 +327,4 @@")if [ -z "$CHECKSUMS_ASSET_ID" ]; thenecho "Error: could not find asset 'checksums.txt' in release ${VERSION}" >&2exit 1[NIT] Checksum extraction uses a stricter regex anchored to the filename. If checksums.txt ever includes paths or different formatting, the match could fail. Consider a slightly more flexible pattern or a sanity check fallback.
Security Review
Summary
Security posture is improved with robust host detection, strict input validation, safer token handling, and integrity checks. The changes avoid token exfiltration on GitHub/GHES and add multiple defense-in-depth measures. One minor hardening opportunity remains around sending Authorization headers across redirects on Gitea downloads.
Findings
.gitea/actions/review/action.ymlRecommendation
APPROVE — Overall, the PR substantially strengthens security: it prevents token exfiltration on GitHub/GHES by ignoring user-supplied URLs, validates inputs (owner/repo format, HTTPS-only server URL on Gitea, version string), moves tokens to masked environment variables, adds timeouts, and verifies checksums. CI has passed. As a defense-in-depth improvement, on the Gitea download path, avoid forwarding Authorization headers across redirects by handling redirects explicitly or dropping the header on cross-host redirects. With that minor adjustment considered for a follow-up, this change is solid and can be merged.
Review by security
Evaluated against
220f6e73@@ -149,0 +337,4 @@-H "Accept: application/octet-stream" \"${GITHUB_API_URL}/repos/${ACTION_REPO}/releases/assets/${BINARY_ASSET_ID}" \-o "${{ runner.temp }}/review-bot"curl -sSfL --connect-timeout 10 --max-time 30 \[MINOR] On the Gitea code path, curl follows redirects (-L) while sending an Authorization header to the initial host. If the server responds with a redirect to a different host (e.g., object storage/CDN), curl will forward the custom Authorization header to that new host, potentially leaking a bearer token to third parties. Consider avoiding -L when using Authorization, or handle redirects explicitly (first request without -L to obtain Location, then re-request the final URL without the Authorization header).
Fix Plan against
220f6e7369:sonnet-review-bot findings:
MINOR #1 (comment 21204) — AUTH_HEADER curl injection:
AUTH_HEADERvariable pattern with inline-H "Authorization: Bearer ${ACTION_TOKEN}"/-H "Authorization: token ${ACTION_TOKEN}"directly in curl calls. This eliminates the intermediate variable that could theoretically carry injected content through shell expansion, even though the control-char check already mitigates this. The inline form is cleaner and safer.MINOR #2 (comment 21205) —
grep -qPportability:grep -qP '[\x00-\x1f\x7f]'with a POSIX-compatible alternative:LC_ALL=C tr -d '[\000-\037\177]'and compare lengths, or useLC_ALL=C grep -q '[^[:print:]]'which works without PCRE. I'll use:printf '%s' "$ACTION_TOKEN" | LC_ALL=C grep -q '[^[:print:]]'which detects non-printable characters portably.NIT #3 (comment 21206) —
sys.exit(1)redundant with shell empty-check:sys.exit(1)from the Pythonfor...elseclause. The shell'sif [ -z "$BINARY_ASSET_ID" ]check already handles the failure case with a clear error message. Theelseclause will just produce empty output, which the shell check catches. Same fix forCHECKSUMS_ASSET_ID.NIT #4 (comment 21207) —
GITEA_URLnaming:GITEA_URL). Renaming here without renaming in the Go code would break things. Will file a follow-up issue.gpt-review-bot findings:
MINOR #1 (comment 21209) —
grep -Pportability:MINOR #2 (comment 21210) — Top-of-file comment misalignment:
github.api_urlis set (present on GitHub.com and GHES runners, absent on Gitea runners)."NIT #3 (comment 21211) — Checksum regex strictness:
^[0-9a-f]+[[:space:]]+\*?${BINARY}$is intentionally strict. It matches the exact format produced bysha256sum(which produces<hash> <filename>or<hash> *<filename>). Loosening it would risk matching wrong lines. If the format changes, the failure is a loud error (no silent wrong match), which is the correct behavior for integrity verification.security-review-bot findings:
MINOR #1 (comment 21214) — curl -L forwards Authorization on redirect:
-Lwill forward headers on redirect. Replace-Lwith explicit redirect handling: first request without-Lto detect redirect, then follow without the auth header. Actually, a simpler fix: use--proto-redir =httpsto limit redirect protocols AND use curl's--location-trustedonly for same-host. Better yet: use-Lwith--proto-redir =https(already https-only) and note that Gitea's release download URLs are direct (no CDN redirect in standard setups). However, for defense-in-depth, I'll switch to not following redirects on the auth path — if a redirect occurs, retry without the auth header.Wait — reviewing the actual code more carefully: the
-Lflag is only used in the Gitea download section. Gitea typically serves assets directly. The GitHub path already uses the REST API withAccept: application/octet-streamwhich returns the content directly. For the Gitea path, the safest approach is to drop-Lentirely (Gitea serves release assets directly without redirects), and if the download fails, theset -euo pipefail+curl -sSfwill catch it. This is the minimal safe fix.Revised action for security MINOR #1: Remove
-fL(follow redirects) from the Gitea download curl calls and use just-f(fail on HTTP errors). Gitea serves release assets directly — no redirect needed. If future Gitea versions add CDN redirects, the download will fail loudly rather than silently leak tokens.Feedback addressed in
27d7fd3Fixed:
AUTH_HEADERintermediate variable; auth headers now inline in each curl call.LC_ALL=C grep -q '[^[:print:]]'.for...elseclauses.github.api_urlpresence.-Lfrom all Gitea download paths.Pushed back:
sha256sumoutput exactly. Loosening risks silent wrong matches; failure is loud and correct.Deferred:
Sonnet Review
Summary
This PR adds dual VCS host support (GitHub/GHES + Gitea) to a composite action, with solid security practices: token masking, input validation, and prevention of token exfiltration to user-supplied URLs. The implementation is well-structured and the CI passes.
Findings
.gitea/actions/review/action.ymlgithub.api_urlexpression is interpolated directly into the shell variable:GITHUB_API_URL="${{ github.api_url }}"— this is safe only becausegithub.api_urlis a trusted platform-provided value (not user input). The comment explaining this exists for the API_URL construction but is absent at the point where GITHUB_API_URL is first assigned. A brief inline comment here would reinforce the security boundary, especially since the same pattern appears in the Install step at line 302 without explanation..gitea/actions/review/action.ymlSERVER_URLvalidation (https-only, no whitespace) is only applied whenVCS_TYPE=gitea. On the GitHub path,github.server_urlis trusted implicitly, which is correct. However if somehowgithub.server_urlwere empty (e.g., a misconfigured runner),SERVER_URLwould be an empty string and later steps would silently construct malformed URLs. A guard likeif [ -z "$SERVER_URL" ]; then exit 1; fiafter the assignment would make failures explicit..gitea/actions/review/action.ymlBINARYvariable (review-bot-linux-amd64) is embedded directly in the Python here-doc via shell interpolation (a['name'] == '${BINARY}'). WhileBINARYis a hardcoded constant in this script (not user input), the pattern conflates shell and Python variable scopes in a subtle way. A minor improvement would be to pass it as a Python argument (sys.argv[1]) or useos.environ, making the boundary explicit. Not a security issue here, but a readability/maintainability nit..gitea/actions/review/action.yml-sSfL(follows redirects) on the GitHub path, but the comment in the Gitea path explicitly explains why-Lis omitted (to prevent header forwarding on CDN redirect). The same security rationale should be documented for the GitHub path to explain why-Lis acceptable there (GitHub S3 redirects don't receive the Authorization header because it's stripped on redirect by curl's default behavior for cross-origin redirects, but this isn't documented).Recommendation
APPROVE — The PR is well-implemented with good security thinking throughout. The VCS detection approach using
github.api_urlis sound, the token masking pattern is correct, and the input validation covers the main injection vectors. The minor findings are documentation/defensive-coding suggestions rather than bugs. CI passes. Approve.Review by sonnet
Evaluated against
27d7fd3a@@ -117,0 +190,4 @@# Determine auth token for release API requestsACTION_TOKEN="${{ inputs.action-repo-token }}"if [ -z "$ACTION_TOKEN" ]; thenif [ "$VCS_TYPE" = "github" ]; then[MINOR] The
github.api_urlexpression is interpolated directly into the shell variable:GITHUB_API_URL="${{ github.api_url }}"— this is safe only becausegithub.api_urlis a trusted platform-provided value (not user input). The comment explaining this exists for the API_URL construction but is absent at the point where GITHUB_API_URL is first assigned. A brief inline comment here would reinforce the security boundary, especially since the same pattern appears in the Install step at line 302 without explanation.@@ -120,0 +216,4 @@fi# Fetch latest version with inline auth header (no intermediate variable)if [ -n "$ACTION_TOKEN" ]; then[MINOR] The
SERVER_URLvalidation (https-only, no whitespace) is only applied whenVCS_TYPE=gitea. On the GitHub path,github.server_urlis trusted implicitly, which is correct. However if somehowgithub.server_urlwere empty (e.g., a misconfigured runner),SERVER_URLwould be an empty string and later steps would silently construct malformed URLs. A guard likeif [ -z "$SERVER_URL" ]; then exit 1; fiafter the assignment would make failures explicit.@@ -149,0 +343,4 @@-o "${{ runner.temp }}/review-bot"curl -sSfL --connect-timeout 10 --max-time 30 \-H "Accept: application/octet-stream" \"${GITHUB_API_URL}/repos/${ACTION_REPO}/releases/assets/${CHECKSUMS_ASSET_ID}" \[MINOR] The
BINARYvariable (review-bot-linux-amd64) is embedded directly in the Python here-doc via shell interpolation (a['name'] == '${BINARY}'). WhileBINARYis a hardcoded constant in this script (not user input), the pattern conflates shell and Python variable scopes in a subtle way. A minor improvement would be to pass it as a Python argument (sys.argv[1]) or useos.environ, making the boundary explicit. Not a security issue here, but a readability/maintainability nit.@@ -153,3 +376,4 @@EXPECTED=$(grep -E "^[0-9a-f]+[[:space:]]+\*?${BINARY}$" checksums.txt | awk '{print $1}')ACTUAL=$(sha256sum review-bot | awk '{print $1}')if [ -z "$EXPECTED" ]; then[NIT] The curl download for the binary uses
-sSfL(follows redirects) on the GitHub path, but the comment in the Gitea path explicitly explains why-Lis omitted (to prevent header forwarding on CDN redirect). The same security rationale should be documented for the GitHub path to explain why-Lis acceptable there (GitHub S3 redirects don't receive the Authorization header because it's stripped on redirect by curl's default behavior for cross-origin redirects, but this isn't documented).Security Review
Summary
The changes add host detection and improved token handling, but on Gitea runners the action still trusts the user-supplied gitea-url for authenticated downloads and execution. This creates an SSRF and potential token exfiltration/RCE risk if an attacker can influence inputs.gitea-url.
Findings
.gitea/actions/review/action.yml.gitea/actions/review/action.yml.gitea/actions/review/action.ymlRecommendation
REQUEST_CHANGES — To mitigate SSRF and token exfiltration/RCE risks on Gitea runners: 1) Do not trust inputs.gitea-url when constructing authenticated request URLs. Instead, derive SERVER_URL from the runner's environment (github.server_url) and require that any provided gitea-url matches the same host (exact host equality). Reject or ignore inputs.gitea-url if it differs from github.server_url. 2) In the Gitea branch, avoid sending Authorization headers to unvalidated hosts; only send credentials to the pinned server_url host. 3) Consider stronger authenticity verification for the downloaded binary (e.g., GPG signature verification against a pinned key) to prevent malicious binaries even if the download server is compromised. After addressing these issues, the improvements to host detection, token masking, and GitHub asset downloads are sound.
Review by security
Evaluated against
27d7fd3a@@ -117,0 +165,4 @@fi# Determine SERVER_URL based on VCS type.# SECURITY: On GitHub/GHES, ALWAYS use github.server_url — never trust[MAJOR] On Gitea runners, SERVER_URL is set from inputs.gitea-url with only a basic https/no-whitespace check. This allows a malicious host (e.g., https://evil.com) to be used for subsequent authenticated requests, enabling SSRF and exfiltration of reviewer-token/action-repo-token.
@@ -117,0 +173,4 @@echo "::warning::inputs.gitea-url is ignored on GitHub/GHES runners (VCS_TYPE=github). Using github.server_url instead."fielseSERVER_URL="${{ inputs.gitea-url || github.server_url }}"[MINOR] SERVER_URL validation on Gitea only checks for https and no whitespace. Strengthen validation by pinning the host to github.server_url on the runner or enforcing an allowlist/host equality check to prevent use of arbitrary external domains.
@@ -149,0 +329,4 @@curl -sSfL --connect-timeout 10 --max-time 120 \-H "Authorization: Bearer ${ACTION_TOKEN}" \-H "Accept: application/octet-stream" \"${GITHUB_API_URL}/repos/${ACTION_REPO}/releases/assets/${BINARY_ASSET_ID}" \[MAJOR] In the Gitea download branch, DOWNLOAD_URL is constructed from the untrusted SERVER_URL and used with Authorization: token headers. The downloaded binary is then executed after checksum verification (which only checks integrity against the same host). An attacker-controlled gitea-url can serve a malicious binary and matching checksum, leading to RCE on the runner and token leakage.
Gpt Review
Summary
Solid update that cleanly supports both GitHub/GHES and Gitea runners with sensible security hardening and robust input validation. Version resolution and asset downloads are adapted per host, tokens are handled safely, and useful step outputs are reused.
Recommendation
APPROVE — The changes look correct and thoughtful: VCS host detection via github.api_url, strict validation of inputs (server URL, action repo, tokens), safe token handling with masking and GITHUB_ENV, and using the appropriate API paths for GH/Gitea. Release asset downloads on GitHub use the REST API with application/octet-stream, which is the right approach for private assets. Backward compatibility is preserved, and the overall flow is clearer with outputs passed between steps. CI has passed, so I recommend merging.
Review by gpt
Evaluated against
27d7fd3aResponse to security-review-bot findings (review #3666 on
27d7fd3)The security-review-bot raised 3 findings against commit
27d7fd3a. These are the same underlying concern as security MINOR #2 (comment #21177) from the previous round, which was pushed back. Addressing each:Findings #1 & #2 [MAJOR] — Gitea SERVER_URL trust / token exfiltration via inputs.gitea-url
Pushed back.
The concern is: on Gitea runners,
inputs.gitea-urlis used to setSERVER_URL, and auth tokens are subsequently sent to that URL. If an attacker controlsinputs.gitea-url, they could exfiltrate the token.Why this doesn't apply in practice on Gitea:
Workflow file is repo-controlled. On Gitea Actions, workflows run from
.gitea/workflows/*.ymlin the repo. There is nopull_request_target-style event that executes the base branch's workflow with the forked PR's content. External contributors cannot inject a maliciousinputs.gitea-urlvalue — only the repo owner controls the workflow file that calls this action.Token is the user's own token. The
reviewer-tokensent toSERVER_URLis the caller's own token to their own Gitea instance. A user who setsinputs.gitea-url: https://evil.comin their own workflow is, by definition, configuring their own workflow to send their own token somewhere — this is self-inflicted, not an external attack vector.HTTPS is enforced. The validation rejects
http://, empty, and whitespace-containing URLs. An attacker can't inject ajavascript:or other protocol.The GitHub/GHES path is fully hardened. On
VCS_TYPE=github,inputs.gitea-urlis completely ignored;SERVER_URLalways comes fromgithub.server_url. This is the higher-risk environment (GitHub Actionspull_request_targetcan run untrusted code), and it's fully addressed.The Gitea path is intentionally designed to accept the caller-provided URL because this action is meant to work with any Gitea instance — adding a host equality check or allowlist would break all legitimate multi-instance deployments. This tradeoff was explicitly discussed in comment #21177.
Finding #3 [MINOR] — SERVER_URL validation should pin to github.server_url
Pushed back. Same as finding #1/#2 above. The action supports diverse Gitea deployments; host-pinning is not appropriate here.
Summary: These findings were addressed by push-back reasoning in comment #21177 in the prior round. The security bot re-raised them as MAJOR without new evidence. The underlying threat requires control of the calling workflow, which on Gitea is exclusively controlled by the repo owner. No code changes are warranted.
Self-Review: PR #121
Self-review against
27d7fd3a93Phase 1: Independent Findings
Reviewed the full diff of
.gitea/actions/review/action.yml(262 changes, 243 additions, 19 deletions):No new blocking findings. The changes are a clean incremental improvement over the prior rounds:
set -euo pipefailadded to both script steps — correctAUTH_HEADERintermediate variable eliminated; auth headers now inline — safergrep -qPreplaced with POSIX-compatibleLC_ALL=C grep -q '[^[:print:]]'— correctsys.exit(1)removed from Pythonfor...elseclauses — shell empty-check suffices-Lremoved from Gitea download curl calls — better security posture::notice::added for ACTION_REPO fallback — good UXOne observation (pre-existing, not introduced here):
ACTION_TOKENflows throughGITHUB_ENVto all subsequent steps, includingRun review, which doesn't use it. This minor over-exposure is mitigated by the::add-mask::call and was correctly identified and pushed back on in prior review rounds (composite actions cannot use step-local env vars).Phase 2: Prior Review Verification
LC_ALL=C grep -q '[^[:print:]]'github.api_urldetection-Lfrom all Gitea download curl callsAssessment: ✅ Clean
All MAJOR findings from prior rounds have been resolved. The latest security-review-bot REQUEST_CHANGES re-raises the Gitea
SERVER_URLtrust concern (same as comment #21177 from the previous round), which has been explicitly pushed back on with reasoning in comment #21245 — the threat requires control of the calling workflow, which on Gitea is exclusively owned by the repo user. The sonnet and gpt review bots both APPROVED this commit with no blocking findings. Tests pass locally (go test ./...). The PR is ready for merge.Rebase Status
This PR is 2 commits behind
main(commits from #124 and the darwin checksum fix both touchaction.yml). A rebase attempt conflicts with the multi-arch binary changes — needs manual conflict resolution before this PR can land.Blocking items:
action.ymlmodified by both #124 and this PR — needs manual mergeSelf-Review: PR #121 (Round 2)
Self-review against
27d7fd3a93Phase 1: Independent Findings
MAJOR #1 — Multi-arch regression from main
This PR hardcodes
BINARY="review-bot-linux-amd64"and cache keyreview-bot-linux-amd64-{version}, dropping the OS/arch detection added in #124 (feat(action): derive binary name from uname for multi-arch support). Main now has:This PR replaces that with a hardcoded
linux-amd64. On darwin or arm64 runners (including the darwin checksum fix on main), this will fail. This isn't a new finding introduced by the PR itself — it's a merge conflict consequence — but it must be resolved before merging.Disposition: MUST FIX (rebase + incorporate arch detection from main)
NIT #1 — python3 -c with inline ${BINARY} expansion
The Python scripts in the GitHub asset download path expand
${BINARY}via bash string interpolation inside the-c "..."heredoc. This works correctly becauseBINARYis a known safe value (review-bot-linux-amd64), but once multi-arch is restored, it will still be safe since arch/OS values are constrained by thecasestatement. No change needed, but worth noting.Phase 2: Prior Review Verification
Assessment: ⚠️ MAJOR (rebase required)
The implementation logic is correct and all prior review findings have been properly addressed. The single blocking issue is the merge conflict with main: multi-arch support from #124 must be incorporated during rebase. Once rebased cleanly (with OS/arch detection restored), this PR is ready to merge.
The security bot's SSRF concern has been addressed through documented threat modeling in comment #21245 — on Gitea the calling workflow lives in the same repo as the config, so the threat requires self-attack. The pushback is sound.
Summary: Rebase on main, restore OS/arch detection in the binary name and cache key, then ready to merge.
27d7fd3a93to51b9a6dc46Rebase Complete (
51b9a6d)Squashed all 6 PR commits into a single clean commit on top of main. Resolved the blocking rebase conflict and incorporated multi-arch support from #124.
What changed in the rebase:
Fixed (was blocking merge):
uname -s/uname -mmapping, updated cache key andBINARYvariable to use${OS}and${ARCH}step outputsgitea-url→vcs-url: updated all references in action.yml to usevcs-url(from #125/#126) — inputs, security comments, and warning messagesGITEA_URL→VCS_URL: Run review step env now passesVCS_URLfrom step output (as required by the #125 rename)shasum -a 256branch uses the${OS}variable instead of the hardcoded expressionPreserved from PR branch:
github.api_urlaction-repoandaction-repo-tokeninputs::add-mask::+GITHUB_ENVset -euo pipefailin both script stepsAssessment: ✅ Ready for merge
PR is clean (single commit on main), mergeable, and all prior review findings are addressed.
go test ./...passes on main.Sonnet Review
Summary
This PR correctly extends a Gitea-only composite action to support both Gitea and GitHub/GHES runners. The implementation is well-structured with appropriate security controls, input validation, and clear comments explaining the design decisions. CI has passed.
Findings
.gitea/actions/review/action.ymlgithub.api_urlbeing non-empty to identify GitHub/GHES runners. However, some Gitea deployments may also expose this context variable (Gitea has been gradually increasing GitHub Actions compatibility). The comment in the file header says it is 'absent on Gitea Actions runners' but this assumption may not hold for future Gitea versions. A belt-and-suspenders approach — also checking whether the URL contains 'github' or comparing against known Gitea API path shapes — might be more robust long-term. For now this is acceptable given the stated requirement, but worth noting..gitea/actions/review/action.yml^[[:xdigit:]]+to^[0-9a-f]+. The new pattern is slightly more restrictive (rejects uppercase hex), which is fine for sha256sum output (always lowercase) but may fail ifshasum -a 256on macOS produces uppercase hex output in some locales. In practiceshasumon macOS also outputs lowercase, so this is low risk, but the comment explaining why the regex changed would help future maintainers..gitea/actions/review/action.yml::warning::inputs.vcs-url is ignored on GitHub/GHES runners (VCS_TYPE=github). Using github.server_url instead.uses an inline${{ inputs.vcs-url }}check inside the shell script. Sinceinputs.vcs-urlis expanded by the Actions template engine before the shell runs, this is fine. However, if the input contained shell metacharacters it could cause issues. Given theset -euo pipefailat the top and that this branch only executes when the value is non-empty (the -n test), this is low risk in practice..gitea/actions/review/action.ymlRun reviewstep setsVCS_URL: ${{ steps.version.outputs.server_url }}but the env var is still namedVCS_URL/GITEA_REPO, which are Gitea-specific names. Since this action now supports GitHub as well, these env var names could be confusing to anyone reading the review-bot binary's documentation. This is a cosmetic concern and may require changes to review-bot itself, so noting it here as a NIT rather than blocking.Recommendation
APPROVE — APPROVE. The implementation is sound: security controls are appropriate (token masking via GITHUB_ENV, input validation for ACTION_REPO and SERVER_URL, token control-character validation, ignoring user-supplied vcs-url on GitHub runners), the VCS detection mechanism is reasonable and well-documented, and step output reuse avoids redundant computation. The minor findings are non-blocking edge cases and cosmetic issues. CI has passed. The PR achieves its stated goal of supporting both Gitea and GHES runners without breaking existing Gitea users.
Review by sonnet
Evaluated against
51b9a6dc@@ -117,1 +205,4 @@fifiif [ "${{ inputs.version }}" = "latest" ]; then[MINOR] The detection logic uses
github.api_urlbeing non-empty to identify GitHub/GHES runners. However, some Gitea deployments may also expose this context variable (Gitea has been gradually increasing GitHub Actions compatibility). The comment in the file header says it is 'absent on Gitea Actions runners' but this assumption may not hold for future Gitea versions. A belt-and-suspenders approach — also checking whether the URL contains 'github' or comparing against known Gitea API path shapes — might be more robust long-term. For now this is acceptable given the stated requirement, but worth noting.@@ -124,4 +238,4 @@elseVERSION="${{ inputs.version }}"fi[NIT] The warning message
::warning::inputs.vcs-url is ignored on GitHub/GHES runners (VCS_TYPE=github). Using github.server_url instead.uses an inline${{ inputs.vcs-url }}check inside the shell script. Sinceinputs.vcs-urlis expanded by the Actions template engine before the shell runs, this is fine. However, if the input contained shell metacharacters it could cause issues. Given theset -euo pipefailat the top and that this branch only executes when the value is non-empty (the -n test), this is low risk in practice.@@ -173,0 +365,4 @@curl -sSfL --connect-timeout 10 --max-time 120 \-H "Accept: application/octet-stream" \"${GITHUB_API_URL}/repos/${ACTION_REPO}/releases/assets/${BINARY_ASSET_ID}" \-o "${{ runner.temp }}/review-bot"[MINOR] The checksum grep pattern was changed from
^[[:xdigit:]]+to^[0-9a-f]+. The new pattern is slightly more restrictive (rejects uppercase hex), which is fine for sha256sum output (always lowercase) but may fail ifshasum -a 256on macOS produces uppercase hex output in some locales. In practiceshasumon macOS also outputs lowercase, so this is low risk, but the comment explaining why the regex changed would help future maintainers.Security Review
Summary
Security posture is improved with host detection, strict input validation, token masking, and safer download paths. No exploitable vulnerabilities identified in the changes; CI passed.
Findings
.gitea/actions/review/action.yml.gitea/actions/review/action.ymlRecommendation
APPROVE — The changes strengthen security by avoiding user-controlled URLs on GitHub/GHES, validating inputs, masking tokens, and using appropriate API endpoints and timeouts. To further harden Gitea usage, consider validating SERVER_URL against an allowlist of trusted hosts to prevent potential token exfiltration via misconfiguration, and add authenticity verification (e.g., signature checks) or restrict action-repo to trusted sources to mitigate supply-chain risks. Since CI passed and no major issues were found, this PR is approved.
Review by security
Evaluated against
51b9a6dc- Detect VCS host type from github.api_url (present on GitHub/GHES, absent on Gitea) - Add action-repo input: specifies repo hosting review-bot releases, separate from the reviewed repo. Defaults to github.action_repository, then rodin/review-bot. - Add action-repo-token input: auth for release downloads (defaults to github.token on GitHub, reviewer-token on Gitea). - GitHub/GHES path: use github.api_url for version resolution and REST API asset download endpoint (required for private repos; web URLs redirect to S3 and don't support Authorization headers reliably). - Gitea path: use validated SERVER_URL with direct download (no -L; prevents Authorization forwarding on potential CDN redirects). - Security hardening: - inputs.vcs-url is IGNORED on GitHub/GHES to prevent token exfiltration - SERVER_URL validated for https scheme and no whitespace on Gitea path - action-repo validated against owner/repo pattern (prevent path traversal) - VERSION validated for no slashes/whitespace - TOKEN validated for no control characters (header injection defense) - ACTION_TOKEN passed via ::add-mask:: + GITHUB_ENV (not step output, which can leak in debug logs) - set -euo pipefail in both script steps - Multi-arch support: OS/arch detection via uname (linux/darwin, amd64/arm64) for cache key and binary name — incorporates changes from #124 - Run review step: passes VCS_URL from step output (server_url) instead of direct input expression Closes #12051b9a6dc46tobd2df7d986Sonnet Review
Summary
This PR correctly refactors a Gitea-only composite action to support both Gitea and GitHub/GHES runners. The VCS detection logic is sound, security considerations are well-documented, and input validation is thorough. CI has passed.
Findings
.gitea/actions/review/action.ymlif [ -n "$GITHUB_API_URL" ]. On Gitea runners where this variable might be set to an empty string vs. truly unset, the behavior is correct. However, the PR description says 'absent on Gitea runners' — if a Gitea runner ever setsgithub.api_urlto a non-empty value for some other reason, it would be misclassified as GitHub. This is a minor documentation/reasoning gap; the implementation is practically correct for known runners..gitea/actions/review/action.ymlif a['name'] == '${BINARY}'. IfBINARYever contained a single quote, this would break the Python string literal. In practiceBINARYis constructed asreview-bot-${OS}-${ARCH}where OS and ARCH are validated to belinux/darwinandamd64/arm64respectively, so there's no actual risk. But the pattern of injecting shell variables into heredoc Python is worth noting..gitea/actions/review/action.yml[/[:space:]]would pass a version likev1.0.0(correct) but would also passv1.0.0%2F(URL-encoded slash). Not a real security risk since the value is used in a URL path constructed with${VERSION}in bash (not eval'd), but worth noting that the validation doesn't cover all URL-unsafe characters..gitea/actions/review/action.ymlecho "::add-mask::${ACTION_TOKEN}"is called AFTER the token has already been used in curl commands (which log to stdout). The masking only affects subsequent log output, not the curl command output already emitted. This is fine — the issue would be if curl verbose mode (-v) were used, which it isn't (-sSfsuppresses output). The mask placement is correct for protecting the env var in subsequent steps.Recommendation
APPROVE — Approve. This is a well-engineered security-conscious change that correctly handles multi-platform CI support. The VCS detection approach (checking
github.api_urlpresence) is sound. Security controls are properly layered: token masking, URL validation for Gitea, forced use of trusted platform URLs for GitHub, and header injection prevention. The minor findings are edge cases with no realistic attack surface given the constraints already in place. The code is production-ready.Review by sonnet
Evaluated against
bd2df7d9@@ -117,0 +190,4 @@# Determine auth token for release API requestsACTION_TOKEN="${{ inputs.action-repo-token }}"if [ -z "$ACTION_TOKEN" ]; thenif [ "$VCS_TYPE" = "github" ]; then[MINOR] The description comment says 'Detect VCS host type using github.api_url context' but the actual detection check is
if [ -n "$GITHUB_API_URL" ]. On Gitea runners where this variable might be set to an empty string vs. truly unset, the behavior is correct. However, the PR description says 'absent on Gitea runners' — if a Gitea runner ever setsgithub.api_urlto a non-empty value for some other reason, it would be misclassified as GitHub. This is a minor documentation/reasoning gap; the implementation is practically correct for known runners.@@ -117,0 +203,4 @@echo "Error: ACTION_TOKEN contains control characters" >&2exit 1fifi[NIT] The VERSION validation regex
[/[:space:]]would pass a version likev1.0.0(correct) but would also passv1.0.0%2F(URL-encoded slash). Not a real security risk since the value is used in a URL path constructed with${VERSION}in bash (not eval'd), but worth noting that the validation doesn't cover all URL-unsafe characters.@@ -152,0 +276,4 @@# SECURITY: Pass token via masked environment variable instead of step output.# Step outputs can leak in debug logs; GITHUB_ENV with masking is safer.if [ -n "$ACTION_TOKEN" ]; then[NIT] The
echo "::add-mask::${ACTION_TOKEN}"is called AFTER the token has already been used in curl commands (which log to stdout). The masking only affects subsequent log output, not the curl command output already emitted. This is fine — the issue would be if curl verbose mode (-v) were used, which it isn't (-sSfsuppresses output). The mask placement is correct for protecting the env var in subsequent steps.@@ -173,0 +352,4 @@-H "Accept: application/octet-stream" \"${GITHUB_API_URL}/repos/${ACTION_REPO}/releases/assets/${BINARY_ASSET_ID}" \-o "${{ runner.temp }}/review-bot"curl -sSfL --connect-timeout 10 --max-time 30 \[MINOR] The Python one-liner for extracting BINARY_ASSET_ID uses shell variable interpolation inside the Python string:
if a['name'] == '${BINARY}'. IfBINARYever contained a single quote, this would break the Python string literal. In practiceBINARYis constructed asreview-bot-${OS}-${ARCH}where OS and ARCH are validated to belinux/darwinandamd64/arm64respectively, so there's no actual risk. But the pattern of injecting shell variables into heredoc Python is worth noting.Security Review
Summary
The changes significantly improve security and correctness by detecting the VCS host, using trusted platform URLs on GitHub/GHES, validating inputs, and masking tokens in env. No exploitable vulnerabilities were introduced; however, there are a few defense-in-depth improvements recommended for Gitea SSRF hardening and broader secret masking.
Findings
.gitea/actions/review/action.yml.gitea/actions/review/action.yml.gitea/actions/review/action.ymlRecommendation
APPROVE — Overall the update is solid: host detection, trusted URL usage on GitHub/GHES, owner/repo validation, control-character filtering in headers, timeouts, and checksum verification are all good practices. To further harden the Gitea path, tighten SERVER_URL validation by pinning to the known host (or an explicit allowlist) and blocking private/link-local IPs to mitigate SSRF/token exfiltration. Additionally, explicitly mask REVIEWER_TOKEN and other secrets in the 'Run review' step, and consider requiring a dedicated action-repo-token (least privilege) rather than defaulting to reviewer-token on Gitea. With these defense-in-depth adjustments, the action would be even more resilient.
Review by security
Evaluated against
bd2df7d9@@ -117,0 +143,4 @@ACTION_REPO="${{ github.action_repository }}"fiif [ -z "$ACTION_REPO" ]; then# Final fallback for Gitea (which may not set action_repository)[MINOR] On Gitea, ACTION_TOKEN defaults to inputs.reviewer-token when action-repo-token is not provided, which may grant broader privileges than necessary and increases the impact if tokens are exfiltrated. Prefer requiring a dedicated, least-privileged token for downloading release assets (or fail closed when missing) to adhere to least privilege.
@@ -117,0 +155,4 @@fi# Detect VCS host type using github.api_url context.# github.api_url is set on GitHub.com (https://api.github.com) and GHES[MINOR] On Gitea, SERVER_URL is derived from inputs.vcs-url with only a basic regex check (https scheme and no whitespace). This leaves potential SSRF/token exfiltration risk if a misconfigured workflow allows untrusted input to control this value. Consider validating the hostname against an allowlist or ensuring it matches the platform-provided server_url, and reject private/link-local IPs (e.g., 169.254.169.254) to prevent metadata endpoint access.
Gpt Review
Summary
Solid enhancement to support both GitHub/GHES and Gitea with sensible security hardening. The separation of action-repo from the reviewed repo, host detection, token handling, and REST-based asset downloads on GitHub are all well-implemented.
Findings
.gitea/actions/review/action.yml.gitea/actions/review/action.yml.gitea/actions/review/action.ymlRecommendation
APPROVE — The changes improve cross-host compatibility, security, and robustness. I recommend merging as-is. To further harden and smooth edge cases, consider (1) adjusting the checksum regex to accept uppercase hex (e.g., use [:xdigit:] or a case-insensitive match), and (2) optionally relaxing the HTTPS-only check for Gitea when no credentials are used or providing an explicit input to override it in trusted environments. Also, optionally align the host detection description in comments/docs with the implemented github.api_url-based approach.
Review by gpt
Evaluated against
bd2df7d9@@ -1,17 +1,37 @@# This composite action is designed for Gitea Actions runners.# Gitea Actions supports GitHub Actions syntax including $GITHUB_OUTPUT,# actions/cache, and actions/checkout.# This composite action supports both Gitea Actions and GitHub Actions runners.[NIT] Top-of-file comments and PR description differ slightly on host detection approach (comments/code use github.api_url presence; description mentions hostname containing 'github'). Not functionally problematic, but aligning documentation with implementation would reduce confusion.
@@ -117,0 +161,4 @@if [ -n "$GITHUB_API_URL" ]; thenVCS_TYPE="github"elseVCS_TYPE="gitea"[MINOR] On Gitea, SERVER_URL is required to be https:// only. While this is a good security default, it introduces a breaking change for self-hosted HTTP setups (e.g., internal networks). Consider allowing HTTP if no token is used or gating the strict check behind an explicit opt-in/opt-out input.
@@ -173,0 +357,4 @@"${GITHUB_API_URL}/repos/${ACTION_REPO}/releases/assets/${CHECKSUMS_ASSET_ID}" \-o "${{ runner.temp }}/checksums.txt"fielse[MINOR] Checksum parsing regex was tightened to only match lowercase hex (^[0-9a-f]+...), which may fail if checksums.txt contains uppercase A–F. Using case-insensitive matching or [:xdigit:] would be more robust.
Pull request closed