- 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
- Add --vcs-url flag as primary (reads VCS_URL env var)
- Keep --gitea-url and GITEA_URL as deprecated fallbacks with warnings
- Update action.yml: rename gitea-url input to vcs-url, pass VCS_URL to binary
- Update ci.yml: use VCS_URL env var in Run review step
- Update integration tests: INTEGRATION_GITEA_URL -> INTEGRATION_VCS_URL
- Update README: --vcs-url / VCS_URL with fallback note in env var table
Backward compat: existing GITEA_URL users get a deprecation warning and
continue to work unchanged until they migrate to VCS_URL.
- sha256sum is not available on macOS; use shasum -a 256 on darwin.
Select based on steps.version.outputs.os which is already computed.
Fixes MAJOR finding from gpt-review-bot (PR #127 review).
- Anchor checksum grep with ^ to avoid matching on partial lines.
Fixes MINOR finding from gpt-review-bot (PR #127 review).
- 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.
claw
Rodin
rodin
aweiker