Addresses findings from reviews #3655 (sonnet), #3657 (security), #3658 (gpt):
- Add set -euo pipefail to both script steps for fail-fast behavior
- Remove redundant newline check ([:space:] already covers it)
- Simplify VERSION regex: remove non-portable \n\r in POSIX ERE
- Add ACTION_TOKEN control character validation (defense-in-depth)
- Anchor checksum grep to exact filename match (prevent substring collision)
- Add ::notice:: when falling back to default ACTION_REPO
- Translate Chinese comments to English for consistency
- Add comment linking GITHUB_API_URL usage back to VCS detection
Addresses gpt-review-bot findings on PR #121:
MAJOR #1: The 'Run review' step set GITEA_URL from inputs.gitea-url which
could exfiltrate the reviewer token to an attacker-controlled host on
GitHub/GHES. Now uses steps.version.outputs.server_url which enforces
VCS-type-aware trust (github.server_url on GitHub, validated input on Gitea).
MAJOR #2: Private release asset downloads on GitHub/GHES used web URLs
({server}/.../releases/download/{tag}/{asset}) which redirect to S3 and
don't support Authorization headers for private repos. Now uses the GitHub
REST API: fetches release metadata by tag, extracts asset IDs, and downloads
via /repos/{owner}/{repo}/releases/assets/{id} with Accept: octet-stream.
Gitea path retains direct URL downloads (which work correctly there).
Security fixes:
- On GitHub/GHES (VCS_TYPE=github), inputs.gitea-url is now completely
ignored. API calls use github.api_url; downloads use github.server_url.
Tokens are never sent to user-supplied URLs.
- Replace action_token step output with masked GITHUB_ENV variable to
prevent token leakage in debug logs.
- Validate action-repo against owner/repo pattern to prevent path traversal.
- Validate SERVER_URL in Gitea path: require https:// scheme, reject
whitespace and newlines.
- Strengthen VERSION validation: block slashes and whitespace in addition
to newlines.
- Add integrity check in Install step: verify SERVER_URL matches
github.server_url on GitHub runners.
Addresses findings from security-review-bot on PR #121.
Deferred: full IP-level SSRF defense (see #123).
- Use github.api_url context for VCS type detection instead of hostname
grep (fixes brittle detection that could misclassify GHES instances)
- Use github.api_url directly for GitHub API calls (correct for both
github.com and GHES, fixes incorrect /api/v3/ assumption)
- Add action-repo-token input with smart defaults (github.token on
GitHub, reviewer-token on Gitea) for private repo support
- Add curl --connect-timeout and --max-time to all HTTP requests
- Add checksum verification caveat noting same-server limitation
- Add newline validation on VERSION before writing to GITHUB_OUTPUT
- Remove incorrect comment about github.com using /api/v3/
The composite action hardcoded Gitea-specific API paths and repo references
that broke when running on GitHub Enterprise Server.
Changes:
- Add 'action-repo' input to specify the repo hosting review-bot releases,
separate from the 'repo' input (which is the target repo being reviewed)
- Auto-detect action repo from github.action_repository context variable,
falling back to 'rodin/review-bot' for backward compatibility
- Detect VCS host type (Gitea vs GitHub) from server URL using hostname
heuristic (URLs containing 'github' use /api/v3/, others use /api/v1/)
- Pass computed server_url and action_repo between steps via GITHUB_OUTPUT
to avoid redundant computation
- Update descriptions to be host-agnostic
Fixes#120
- Fix AllowInsecureHTTP doc comment: say '_test.go file in the same
package' instead of 'export_test.go' (MINOR #1)
- Remove dead u.Fragment = "" from redactURL: HTTP requests never
carry fragments over the wire per RFC 7230 §5.1 (MINOR #2)
- Use 127.0.0.1:1 in scheme-rejection tests to make intent clearer
that no network call should occur (NIT #3)
- Restore timer.Stop() no-op in case <-timer.C for symmetry with ctx.Done
- Add missing blank line between TestNoInsecureOption_RejectsHTTP and
TestNoInsecureOption_RejectsUppercaseHTTP
- Remove double blank line before TestAllowInsecureHTTP_WithoutEnvVar_Rejected
Resolves review comments from sonnet-review-bot on PR #113.
Address review feedback on PR #113:
- MAJOR (both reviews): Replace strings.HasPrefix(reqURL, "http://")
with url.Parse + strings.EqualFold for case-insensitive scheme
comparison per RFC 3986. Prevents bypass via HTTP:// or Http://.
- MINOR (security): Enhance redactURL to strip userinfo component
(user:pass@host) in addition to query params, preventing credential
leakage in error messages and logs.
- NIT (gpt): Remove redundant timer.Stop() after timer.C fires —
it's a no-op and the comment was misleading.
- Add tests for uppercase/mixed-case HTTP scheme rejection and
userinfo redaction.
Three-layer defense for the AllowInsecureHTTP client option:
1. Environment gate: AllowInsecureHTTP() requires REVIEW_BOT_ALLOW_INSECURE=1
env var. Without it, the option is silently ignored with a slog.Warn.
2. Warning log on activation: When the env gate IS satisfied, a slog.Warn
fires at client construction time so operators notice in production logs.
3. Test bypass: AllowInsecureHTTPForTest() skips the env gate entirely,
keeping test code clean (no t.Setenv needed in every test).
Additionally, doRequest now rejects HTTP URLs unless allowInsecureHTTP is set
on the client, providing defense-in-depth against credential leakage.
Closes#96
Add commitID parameter to gitea.Client.PostReview so the review is
anchored to the specific commit that was evaluated. The caller
(cmd/review-bot) already computes evaluatedSHA from pr.Head.Sha;
this wires it through to the Gitea API payload.
When commitID is empty, omitempty drops it from the JSON and Gitea
defaults to the current PR head (backward-compatible).
Closes#107
- Replace Unicode arrows (→) with ASCII (->) in error messages and
comments for log compatibility (gpt-review NITs #19626, #19628)
- Improve guard comment to clarify it exists for testability, not
runtime safety (sonnet-review NIT #19619)
- Add cross-reference comments noting intentional duplication between
gitea/client.go and github/client.go (sonnet-review #19618,
gpt-review #19625, #19627)
Pushed back on:
- internal/ package for dedup: structural overhead not warranted for
a single ~25-line function
- strings.EqualFold for scheme: Go's url.Parse normalizes schemes to
lowercase, making case-insensitive comparison unnecessary
Add defaultCheckRedirect to both GitHub and Gitea clients that rejects:
- HTTPS→HTTP protocol downgrades (prevents plaintext leakage)
- Cross-host redirects entirely (prevents consuming untrusted responses)
Same-host, same-or-upgraded-scheme redirects remain allowed.
Both NewClient constructors wire the policy, and SetHTTPClient(nil)
restores it. Callers providing a non-nil client are responsible for
configuring their own safe redirect policy.
Closes#95
- Extract doGetWithReader to share retry/backoff logic between doGet and
doGetLimited, eliminating ~60 lines of duplicated code (addresses MINOR
finding from all reviewers).
- redactURL now strips userinfo credentials (user:pass@host) in addition
to query parameters (addresses security-review-bot finding).
- GetPullRequestDiff treats MaxDiffSize == math.MaxInt64 as disabled,
preventing the silent enforcement bypass where the overflow clamp makes
the size check unreachable (addresses security-review-bot finding).
- Improved error message wording: 'response exceeds N bytes' (NIT fix).
- Add concurrency safety note to MaxDiffSize field documentation,
mirroring the existing note on RetryBackoff
- Consolidate six individual test functions into a single table-driven
test (TestGetPullRequestDiff_SizeLimits) reducing repetition
- Add //nolint:errcheck annotation to test handler w.Write calls
- Clamp maxBytes+1 to prevent integer overflow to negative when
maxBytes == math.MaxInt64 (falls back to math.MaxInt64)
- Update MaxDiffSize doc: 'any negative value' disables the limit,
matching actual behavior of 'maxSize < 0' check
Add a configurable MaxDiffSize field to Client that limits how much
data GetPullRequestDiff will read into memory. The default is 10 MB
(DefaultMaxDiffSize). When the diff exceeds the limit, ErrDiffTooLarge
is returned, allowing callers to skip position translation gracefully.
Implementation uses io.LimitReader to read maxBytes+1, detecting
overflow without buffering the entire response. Setting MaxDiffSize
to -1 disables the limit entirely.
Closes#92
- Add timer.Stop() to the timer.C branch to prevent timer leak on the
normal path (previously only called in ctx.Done branch)
- Rename struct field 'http' to 'httpClient' to avoid shadowing the
net/http import
Addresses self-review findings on PR #110.
- Add TODO comment on unused baseURL field explaining its intended
future use by higher-level exported methods
- Rewrite TestDoRequest_ContextCanceled to actually exercise the
timer-cancel path in the retry select (goroutine cancels context
while client is blocked in timer.C select)
- Change Retry-After: 1 to Retry-After: 0 in IntegerSeconds test
to avoid unnecessary 1s sleep during test runs
- Add doc note on SetRetryBackoff explaining that an empty non-nil
slice silently drops Retry-After delays
- Fix data race: copy retryBackoff slice per-request to prevent
concurrent doRequest calls from racing on shared state
- Fix parseRetryAfter: trim whitespace before parsing for robustness
- Fix parseRetryAfter: treat delta-seconds of 0 as valid per RFC 7231
- Add bounded read on success path (10 MB limit) for defense-in-depth
- Clean up TestDoRequest_429_RetryAfter_IntegerSeconds: remove dead
code block and commented-out reasoning
- Fix import ordering: context before errors (goimports compliance)
Implement the github package client with Retry-After header parsing that
supports both integer seconds (e.g. "Retry-After: 120") and HTTP-date
format (e.g. "Retry-After: Thu, 01 Dec 2025 16:00:00 GMT") per RFC 7231
§7.1.3.
Key design decisions:
- Use http.ParseTime which handles RFC 1123, RFC 850, and ASCTIME formats
- Cap maximum retry delay at 60s (maxRetryAfter) to prevent stalling
- If HTTP-date is in the past, use delay of 0 (retry immediately)
- Inject time.Now via c.now field for deterministic testing
Closes#94
Update the approved dependency table to document go-yaml subpackage
usage (ast, parser) and remove the deviation comment now that the
proper allowlist process is being followed.
Closes#91
- Clarify depth-aware short-circuit comment to unambiguously describe
the relationship between current depth and previous validation depth
- Add comment to MappingValueNode case explaining intentional depth+2
behavior from parent MappingNode perspective
- Restructure unmarshalYAMLWithDepthLimit doc comment as bullet list
covering all three safety checks (depth, multi-doc, strict fields)
- Replace t.Error with t.Fatal in TestYAMLEmptyFileRejection to remove
redundant nil guard on subsequent err.Error() call
- Add explicit case for *ast.MergeKeyNode in checkYAMLDepth switch to
make it clear this is an intentional leaf (no children to recurse)
rather than relying on the default case. Prevents future library
changes from silently bypassing depth checks.
- Add MaxPersonaFileSize bound check at the top of ParsePersonaBytes.
While callers already check size, the public API should defend itself
(defense in depth) against arbitrarily large inputs that could cause
excessive memory/CPU before AST validation runs.
- Add tests for both behaviors.
Addresses review #2879 findings.
- Add safety note on Strict() decoder not expanding aliases recursively,
since alias resolution uses the pre-validated AST (finding #1)
- Document that ast.Node map keys rely on pointer identity, which holds
because all goccy/go-yaml AST types are pointer receivers (finding #2)
- Clarify AnchorNode comment: effective depth budget is reduced for
anchor+alias pairs, not literally halved (finding #3)
- Improve test depth trace comment for accuracy (finding #4)
- Add HTML comment in CONVENTIONS.md referencing #91 for the two-step
process deviation (finding #5)
- Fix validated map comment: says 'minimum depth' but stores the maximum
depth at which a node was validated (overwritten on deeper visits).
- Replace dec.More() with explicit dec.Decode check for trailing JSON
content. More() is documented for use inside arrays/objects; the
explicit EOF check is clearer at the top-level stream.
MINOR fixes:
- docs/DESIGN-57-yaml-persona.md: fix Error Cases table entry to reflect
custom AST walk (checkYAMLDepth) instead of stale library-level reference
- review/persona.go: add EOF check after JSON decode to reject trailing
garbage after a valid JSON object (prevents silent acceptance of malformed
input like '{"name":"x"}garbage')
- review/persona_test.go: add TestJSONTrailingContentRejected test
NIT fixes:
- review/persona.go: add default case to checkYAMLDepth switch with
explanatory comment about scalar leaf nodes
- review/persona.go: document AnchorNode depth+1 conservative asymmetry
- review/persona.go: simplify redundant if-guard in ListBuiltinPersonas
- Move nodeCount increment after cycle detection to avoid over-counting
cyclic references (sonnet #2)
- Use underscores in test case names used as filenames (sonnet #3)
- Fix function comment: 'prevent silent data loss' → 'prevent confusing
behavior where additional documents are silently ignored' (sonnet #4)
- Mark design doc pseudocode as historical since implementation uses
goccy/go-yaml ast.Node, not gopkg.in/yaml.v3 yaml.Node (sonnet #5)
The global 'seen' set allowed anchored subtrees validated at a shallow
depth to be skipped when later referenced via alias at a greater depth.
This could let effective nesting exceed MaxYAMLDepth, enabling DoS.
Fix: replace the single 'seen' set with two tracking maps:
- validated (node -> min depth): only short-circuits when current depth
<= previously validated depth; re-checks at deeper contexts.
- visiting (node -> bool): per-path recursion stack for true cycle
detection (breaks alias loops without suppressing depth checks).
Add TestYAMLAliasDepthBypass that constructs a document with an
anchored 15-level subtree referenced via alias under 6 levels of
nesting, verifying the combined effective depth (22) is rejected.
Addresses security-review-bot findings on review #2774.
Fixes#87.
PR #58 incorrectly added gopkg.in/yaml.v3 (abandoned library) instead of
github.com/goccy/go-yaml as required by issue #57.
Changes:
- Replace gopkg.in/yaml.v3 with github.com/goccy/go-yaml v1.19.2
- Update review/persona.go to use goccy/go-yaml API:
- parser.ParseBytes for AST-based depth/node count checking
- yaml.Strict() decoder option instead of KnownFields(true)
- ast.Node types instead of yaml.Node for tree walking
- Update review/persona_test.go to use ast types for cycle tests
- Remove gopkg.in/yaml.v3 from go.mod and go.sum
All existing YAML tests pass with the new library.
Add defensive check for empty Name and Path fields when unmarshaling
a single ContentEntry in the fallback path. While Gitea API won't
return empty objects for valid file paths, this guard:
- Explicitly documents the invariant we expect
- Catches potential API behavior changes early
- Costs nothing at runtime
Addresses [MINOR] from sonnet-review-bot on PR #74.
When ListContents is called with a path that points to a file (not a
directory), Gitea returns a single JSON object instead of an array.
Previously this caused json.Unmarshal to fail with:
json: cannot unmarshal object into Go value of type []gitea.ContentEntry
Now ListContents tries array unmarshal first, and falls back to single
object unmarshal, wrapping it in a slice. This allows patterns-files
config to specify individual files like 'README.md' without triggering
a parse error.
Also updates TestGetAllFilesInPath_File to reflect actual Gitea behavior
(single object response, not 404).
Fixes#73
aweiker
claw
rodin
Rodin