Finding #1 [MAJOR]: replace os.Stat with os.Lstat in checkStaleDocs to
prevent symlink traversal. Symlinks under repoRoot could probe arbitrary
host file existence; Lstat never follows them. Symlinked docs are now
treated as stale.
Finding #2 [MINOR]: resolve --repo-root with filepath.Abs +
filepath.EvalSymlinks before passing to checkStaleDocs, so a symlinked
repo-root cannot bypass the filepath.Rel escape guard.
Finding #3 [NIT]: reject backslashes in ValidateDocPath to prevent
Windows platform edge cases where a path separator may be normalised
differently by the host OS or VCS backend.
Tests added:
- TestCheckStaleDocs_SymlinkOutside: symlink inside repo → outside
- TestCheckStaleDocs_SymlinkInsideRepo: intra-repo symlink also rejected
- TestRunValidateDocmap_SymlinkRepoRoot: symlinked --repo-root resolves OK
- TestValidateDocPath_Backslash: backslash paths rejected
- Backslash cases added to TestValidateDocPath invalid slice
All go test ./... pass, go vet ./... clean.
Export review.ValidateDocPath and use it in checkStaleDocs before
calling os.Stat. Add filepath.Clean + filepath.Rel confinement check
as defense-in-depth to ensure doc paths from PR-controlled YAML
cannot probe filesystem locations outside repoRoot.
Also add tests covering: ../../etc/passwd, /etc/passwd, ../outside,
a valid present path, and a valid missing path.
Addresses security finding from security-review-bot on PR #142.
TestRunValidateDocmap_Clean was reading real os.Stdin (fragile in CI).
Switch to stdinValidateDocmap with a covered file and empty-stdin test
already covered by TestRunValidateDocmap_EmptyStdin.
Adds 'review-bot validate-docmap' for CI hard-fail on docmap coverage gaps.
Usage:
git diff --name-only origin/main HEAD | \
review-bot validate-docmap --docmap .review-bot/doc-map.yml --repo-root .
Flags:
--docmap (required) path to doc-map YAML file
--repo-root (optional, default '.') root for resolving docs: paths
Two checks, both always run:
1. Coverage: every stdin file must match at least one paths: glob.
2. Stale docs: every docs: entry must exist on disk under --repo-root.
Exit codes: 0=clean, 1=failures found, 2=usage/parse error.
Tests cover: clean pass, uncovered file, stale doc, both failures,
empty stdin, blank-line stdin, and duplicate docs: deduplication.
Adds FileCoveredByDocMap(cfg *DocMapConfig, file string) bool — a thin wrapper
over the existing unexported mappingMatches that lets cmd/ check per-file docmap
coverage without duplicating glob logic.
Also adds unit tests covering matched globs, non-matching paths, empty file,
and empty config.
- budget/budget_test.go: add TestFit_DesignDocsInSystemPrompt,
TestFit_DesignDocsTrimmedBeforeFileContext, TestFit_DesignDocsEmptyNoHeading
to cover the new DesignDocs section through Fit() and buildResult()
- Remove PLAN-137.md (contained raw thinking stream, not suitable as repo doc)
- Add docs/DESIGN-137-doc-map.md with clean architectural decision record
- New --doc-map flag (DOC_MAP_FILE env var): path to YAML config mapping
source path globs to governing design docs
- New --doc-map-max-bytes flag (DOC_MAP_MAX_BYTES env var): cap on total
injected doc content, default 100KB
- review/docmap.go: DocMapConfig parsing, glob matching with ** support,
doc loading via VCS with directory expansion and size guard
- budget.Sections: new DesignDocs field, trimmed after conventions
- budget.buildResult: injects DesignDocs under ## Design Documents heading
- action.yml: doc-map and doc-map-max-bytes inputs wired to env vars
- CHANGELOG.md: created with unreleased entry
- Tests: ParseDocMapConfig, MatchDocs, globMatch, LoadMatchingDocs
Removed github/review.go and github/identity.go which were untracked orphan files
from an incomplete refactor (issue #130). They referenced a non-existent vcs package
and duplicated methods already in github/client.go.
All 6 packages pass: go test -count=1 ./... ✅
go build ./... and go vet ./... clean ✅
Updated TODO.md with current cycle status.
gitea: Add 4 tests for GetTimelineReviewCommentIDForReview (was 0% coverage):
- Success: find review in timeline by user login + body prefix match
- ReviewFetchError: 404 on review API
- EmptyBody: review with empty body returns error
- NotFoundInTimeline: body matches but user login doesn't
github: Add 3 tests for GetAllFilesInPath (was 0% coverage):
- DirectoryWithFiles: lists directory, fetches base64-encoded file content
- 404FallsBackToFile: 404 on dir path returns error when file also 404s
- DirectoryWithSubdir: recursive directory traversal
Coverage changes:
- gitea: 80.0% → 85.2%
- github: 79.9% → 86.3%
The test constructs github.Client directly (matching the Gitea integration
test pattern), so setting VCS_TYPE does not affect the code under test.
Remove the setenv call to avoid implying routing is being exercised.
- Strip VCS_TYPE and VCS_URL in cleanEnv() to prevent env leakage in
subprocess tests when VCS_TYPE=github is set in the runner environment
(fixes#135)
- Add TestGithubAPIURL table-driven tests covering:
- Empty string defaults to https://api.github.com
- https://github.com maps to https://api.github.com
- Trailing slash variant maps correctly
- GHES host (ghe.example.com) gets /api/v3 suffix
- GHES concur domain does not map to api.github.com
(fixes#134)
- Add TestIntegration_GitHub_PostAndVerifyReview: exercises the GitHub
adapter end-to-end via VCS_TYPE=github. Skips gracefully when
INTEGRATION_GITHUB_TOKEN, INTEGRATION_GITHUB_REPO, and
INTEGRATION_GITHUB_PR are not set. Verifies GetAuthenticatedUser,
GetPullRequest, PostReview, and ListReviews succeed; notes that
DeleteReview on submitted GitHub reviews is expected to fail (422).
(fixes#133)
Python's ipaddress module does NOT classify 100.64.0.0/10 (RFC6598
carrier-grade NAT) as private/loopback/link_local/multicast/reserved.
This means a SERVER_URL resolving to a CGN address would bypass the
Python SSRF check and reach curl with ACTION_TOKEN.
Add an explicit network membership check for 100.64.0.0/10 to both
Python validation blocks in action.yml:
- _ssrf_check.py (VCS URL pre-flight check)
- _ssrf_check_install.py (binary download URL check)
The Go-level IsBlockedIP already covers this range correctly (ipcheck.go);
this fix closes the gap in the action.yml Python layer.
Also update comments to mention RFC6598 explicitly.
- Clone http.DefaultTransport instead of bare &http.Transport{} to preserve
ProxyFromEnvironment, TLSHandshakeTimeout, IdleConnTimeout, connection
pooling, and HTTP/2 support (fixes transport regression).
- Add IPv6-mapped IPv4 normalization in action.yml Python SSRF checks to
prevent bypass via ::ffff:10.0.0.1 style AAAA records.
- Reject URLs with user-info (user:pass@host) in action.yml Python checks
to match validate-url subcommand behavior.
- Add test verifying DefaultTransport settings are preserved.
Previously safeDialContext only dialed the first resolved IP. If the
connection failed, it returned an error without trying other IPs.
Now it iterates all validated IPs and returns the first successful
connection, or the last error if all fail. This matches the resilience
behavior of a plain net.Dialer on multi-IP hostnames.
Addresses review finding: safeDialContext only dials first resolved IP.
All IPs are still validated before any dial attempt is made.
MAJOR fixes:
- gitea/ipcheck.go: replace startup panic with init()+error list pattern
Hard-coded CIDRs that fail to parse now recorded in blockedCIDRParseErrors
instead of panicking. TestBlockedCIDRsValid catches programming errors
in CI without violating CONVENTIONS.md 'never panic' rule.
- .gitea/actions/review/action.yml: re-validate SERVER_URL at start of
'Install review-bot' step to close DNS rebinding window between
'Determine version' and install-step curl calls.
MINOR fixes:
- gitea/client.go: add Timeout: 10*time.Second to net.Dialer per PLAN.md spec
- cmd/review-bot/validateurl.go: switch isValidateError to errors.As so
wrapped *validateError values are also detected
- gitea/ipcheck_test.go: clarify 198.51.100.1 (RFC5737 TEST-NET-2) comment;
add TestBlockedCIDRsValid to surface CIDR parse errors as test failures
NIT fixes:
- .gitea/actions/review/action.yml: refactor Python list comprehension in
SSRF check to for-loop (avoids side-effect-only comprehension, runner compat)
- gitea/export_test.go: expand comment explaining white-box test pattern
(why package gitea not gitea_test, Go stdlib precedent)
Remove PLAN.md (implementation complete)
## Changes
### Go: IP-level SSRF protection in gitea.Client (primary defense)
- Add gitea/ipcheck.go with IsBlockedIP() covering all blocked CIDR ranges:
loopback (127.0.0.0/8, ::1), RFC1918 (10/8, 172.16/12, 192.168/16),
link-local (169.254/16, fe80::/10), ULA (fc00::/7), CGN (100.64/10),
multicast, reserved, and unspecified ranges.
- IPv6-mapped IPv4 addresses (::ffff:x.x.x.x) are normalized before checking.
- Add safeDialContext to gitea.Client: resolves DNS, rejects any IP in a
blocked CIDR, then dials the resolved IP directly to narrow the DNS rebinding
window. NewClient now uses this safe transport by default.
- Add WithUnsafeDialer() for test code using httptest.Server (127.0.0.1).
- Update NewTestClient helper in export_test.go for all gitea unit tests.
- Update SetHTTPClient(nil) to restore the safe transport (not the plain one).
### Go: validate-url subcommand (defense-in-depth for future bash callers)
- Add 'review-bot validate-url <url>' subcommand: validates https scheme,
no user-info, resolves hostname, rejects any blocked IP.
- Exit 0=safe, 1=blocked, 2=validation error/dns failure.
- Add outWriter/errWriter vars to main.go for testable output capture.
### action.yml: Python3 IP check in 'Determine version' step
- After the https scheme validation, resolve SERVER_URL hostname with
socket.getaddrinfo and reject any result where
ipaddress.ip_address(ip).is_private/is_loopback/is_link_local/etc. is true.
- python3 is required on ubuntu-* runners (noted in existing comments).
- Covers the version-check curl that sends ACTION_TOKEN to SERVER_URL.
- SERVER_URL for install-step curls is covered by the same pre-check.
### Tests
- gitea/ipcheck_test.go: 30+ cases covering all blocked families + public IPs
- gitea/client_test.go: safe transport presence, WithUnsafeDialer, SSRF blocking
- cmd/review-bot/validateurl_test.go: scheme validation, user-info, exit codes
Closes#123
This PR addresses issue #120 by adding GitHub/GHES support to the composite
action. Key improvements:
- Detects VCS host type via github.api_url presence (GitHub/GHES vs Gitea)
- Uses correct API paths: /api/v3 for GitHub, /api/v1 for Gitea
- Prevents token exfiltration by ignoring inputs.vcs-url on GitHub runners
- Validates inputs (action-repo format, URL scheme, tokens)
- Adds multi-arch support (uname-derived binary/OS detection)
- Reuses computed values across steps via GITHUB_OUTPUT
- Properly handles private release assets on GitHub via REST API
- Backward compatible with existing Gitea workflows
Reviewed and approved by sonnet-review, gpt-review, and security-review.
- 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#120
Rodin
rodin
claw
aweiker