fix(#123): address all review findings from PR #129
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 34s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m25s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m30s
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 34s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m25s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m30s
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)
This commit is contained in:
claw
@@ -206,7 +206,10 @@ runs:
|
||||
'try: rs=socket.getaddrinfo(h,None)' \
|
||||
'except socket.gaierror as e: print(f"DNS error: {e}",file=sys.stderr); sys.exit(1)' \
|
||||
'if not rs: print("Error: no addresses",file=sys.stderr); sys.exit(1)' \
|
||||
'[sys.exit(1) or print(f"blocked: {a}",file=sys.stderr) for _,_,_,_,(a,*_) in rs if (lambda ip:ip.is_private or ip.is_loopback or ip.is_link_local or ip.is_multicast or ip.is_reserved)(ipaddress.ip_address(a))]' \
|
||||
'for _,_,_,_,(a,*_) in rs:' \
|
||||
' ip=ipaddress.ip_address(a)' \
|
||||
' if ip.is_private or ip.is_loopback or ip.is_link_local or ip.is_multicast or ip.is_reserved:' \
|
||||
' print(f"blocked: {a}",file=sys.stderr); sys.exit(1)' \
|
||||
> /tmp/_ssrf_check.py
|
||||
CHECK_URL="${SERVER_URL}" python3 /tmp/_ssrf_check.py || {
|
||||
echo "Error: SERVER_URL '${SERVER_URL}' resolves to a private/reserved IP address" >&2
|
||||
@@ -332,10 +335,30 @@ runs:
|
||||
ACTION_TOKEN="${ACTION_TOKEN:-}"
|
||||
BINARY="review-bot-${OS}-${ARCH}"
|
||||
|
||||
# SECURITY: SERVER_URL was validated (scheme + IP check) in "Determine version".
|
||||
# All curl calls in this step use the same SERVER_URL, so that pre-check covers them.
|
||||
# The installed binary uses safeDialContext for additional defense-in-depth in
|
||||
# the "Run review" step.
|
||||
# SECURITY: Re-validate SERVER_URL at the start of this step to mitigate DNS
|
||||
# rebinding attacks. A DNS TTL expiry between "Determine version" and here
|
||||
# could allow an attacker to change the resolved IP to a private/reserved
|
||||
# address, causing curl to send ACTION_TOKEN to an internal host.
|
||||
# Only needed on Gitea path (VCS_TYPE=gitea); GitHub/GHES uses platform-controlled URLs.
|
||||
if [ "$VCS_TYPE" = "gitea" ]; then
|
||||
printf '%s\n' \
|
||||
'import socket,ipaddress,sys,os' \
|
||||
'from urllib.parse import urlparse' \
|
||||
'u=os.environ["CHECK_URL"]; h=urlparse(u).hostname' \
|
||||
'(print("Error: no hostname",file=sys.stderr) or sys.exit(2)) if not h else None' \
|
||||
'try: rs=socket.getaddrinfo(h,None)' \
|
||||
'except socket.gaierror as e: print(f"DNS error: {e}",file=sys.stderr); sys.exit(1)' \
|
||||
'if not rs: print("Error: no addresses",file=sys.stderr); sys.exit(1)' \
|
||||
'for _,_,_,_,(a,*_) in rs:' \
|
||||
' ip=ipaddress.ip_address(a)' \
|
||||
' if ip.is_private or ip.is_loopback or ip.is_link_local or ip.is_multicast or ip.is_reserved:' \
|
||||
' print(f"blocked: {a}",file=sys.stderr); sys.exit(1)' \
|
||||
> /tmp/_ssrf_check_install.py
|
||||
CHECK_URL="${SERVER_URL}" python3 /tmp/_ssrf_check_install.py || {
|
||||
echo "Error: SERVER_URL '${SERVER_URL}' resolves to a private/reserved IP address" >&2
|
||||
exit 1
|
||||
}
|
||||
fi
|
||||
|
||||
if [ "$VCS_TYPE" = "github" ]; then
|
||||
# GitHub/GHES: Use REST API for release asset downloads.
|
||||
|
||||
@@ -1,238 +0,0 @@
|
||||
# Plan: Issue #123 — SSRF Defense: IP-Level URL Validation
|
||||
|
||||
## Problem
|
||||
|
||||
When running on a self-hosted Gitea runner, `action.yml` accepts a user-supplied `vcs-url` input and uses it to make HTTP requests (version lookup, binary download) from within the runner environment. An attacker who controls workflow inputs could point this URL at internal services (10.x.x.x, 169.254.x.x, 127.0.0.1, etc.) and exfiltrate data.
|
||||
|
||||
Current mitigations (scheme check + whitespace rejection in bash) are insufficient because they operate on the URL string, not on the resolved IP addresses. DNS rebinding and creative hostnames can bypass string-level checks.
|
||||
|
||||
The existing Go binary (`review-bot`) also makes requests to `vcs-url` directly via `gitea.NewClient`. That client has redirect protections but no IP-level SSRF defense.
|
||||
|
||||
## What This Issue Does NOT Cover
|
||||
|
||||
- LLM base URLs / AI Core auth/API URLs — those are future work, out of scope for this issue.
|
||||
- GitHub Actions path (`VCS_TYPE=github`) — already uses `github.api_url` (trusted, not user-controlled), so SSRF is not applicable there.
|
||||
|
||||
## Constraints
|
||||
|
||||
- **No new dependencies.** `CONVENTIONS.md` has a strict allowlist; `net` is stdlib.
|
||||
- Must not break existing functionality for legitimate Gitea instances.
|
||||
- Must handle DNS resolution correctly (some CIDRs block connection but resolve fine — we must check before connecting).
|
||||
- Must work on both linux/amd64 and linux/arm64 runner environments.
|
||||
- Go stdlib `net` package provides everything needed.
|
||||
|
||||
## Acceptance Criteria (from issue)
|
||||
|
||||
- [x] Decide on approach → **Go custom `DialContext` in `gitea.Client`**
|
||||
- [ ] Implement URL validation that blocks requests to RFC1918/loopback/link-local IPs
|
||||
- [ ] Block HTTP redirects to non-https or different hosts (already done in `defaultCheckRedirect`)
|
||||
- [ ] Add tests
|
||||
|
||||
## Approach Decision: Custom DialContext in `gitea.Client`
|
||||
|
||||
Three options were considered:
|
||||
|
||||
| Option | Pros | Cons |
|
||||
|--------|------|------|
|
||||
| Bash `dig`/`getent` + IP checks | No Go change | Fragile, platform-dependent, bypass-prone, hard to test |
|
||||
| `review-bot validate-url` subcommand | Reusable from bash | Two phases (validate-then-dial) has TOCTOU; adding a subcommand complicates main.go |
|
||||
| **Custom `DialContext` in `gitea.Client`** | Atomic: resolves and validates IP in one step; no TOCTOU; testable in Go; no new deps | Defense-in-depth only in Go path, not bash curl calls |
|
||||
|
||||
**Decision: Custom `DialContext` + bash-side pre-check for the install step.**
|
||||
|
||||
Rationale:
|
||||
- The `review-bot` binary already makes the sensitive Gitea API calls. Protecting them at the `DialContext` level is the most robust defense — it's atomic and cannot be bypassed.
|
||||
- The bash `curl` in the "Determine version" and "Install review-bot" steps makes requests to user-supplied URLs. The Go binary isn't involved there. Options:
|
||||
- Accept residual risk for the install-step curl calls (the token used is `reviewer-token`, not a secrets-level token).
|
||||
- Add a `review-bot validate-url` subcommand that the bash calls before each curl.
|
||||
- **We add `review-bot validate-url` as a lightweight subcommand** to protect the install-step curl calls too.
|
||||
|
||||
Both defenses together (DialContext + validate-url subcommand) give full coverage. The validate-url path only needs to resolve the domain and check the resulting IPs — no additional state.
|
||||
|
||||
## Design: `validate-url` Subcommand
|
||||
|
||||
`review-bot validate-url <url>` exits 0 if safe, non-zero + error message if blocked.
|
||||
|
||||
```
|
||||
usage: review-bot validate-url <url>
|
||||
|
||||
Resolves <url> and checks that all resolved IP addresses are
|
||||
routable (not RFC1918, loopback, link-local, or special-use).
|
||||
|
||||
Exit codes:
|
||||
0 — URL is safe to use
|
||||
1 — URL resolves to a blocked address
|
||||
2 — URL validation error (bad scheme, malformed URL, DNS failure)
|
||||
```
|
||||
|
||||
This command is tiny — no LLM logic, no Gitea auth. The bash in `action.yml` can call it as:
|
||||
|
||||
```bash
|
||||
"${{ runner.temp }}/review-bot" validate-url "$SERVER_URL" || {
|
||||
echo "Error: SERVER_URL resolves to a blocked/private IP" >&2
|
||||
exit 1
|
||||
}
|
||||
```
|
||||
|
||||
## Design: IP Validation Logic (Shared)
|
||||
|
||||
A single `internal/ipcheck` package (or unexported function in `gitea`) will:
|
||||
|
||||
1. Parse the URL: extract host + port.
|
||||
2. Call `net.DefaultResolver.LookupIPAddr(ctx, host)` to resolve all IPs.
|
||||
3. For each IP, call `isBlockedIP(ip)`:
|
||||
- Loopback: `127.0.0.0/8`, `::1/128`
|
||||
- RFC1918: `10.0.0.0/8`, `172.16.0.0/12`, `192.168.0.0/16`
|
||||
- Link-local: `169.254.0.0/16`, `fe80::/10`
|
||||
- Unique local (IPv6 ULA): `fc00::/7`
|
||||
- Multicast: `224.0.0.0/4`, `ff00::/8`
|
||||
- Loopback/unspecified: `0.0.0.0/8`, `::`
|
||||
4. If **any** resolved IP is blocked → reject.
|
||||
5. If **zero** IPs are returned → reject (NXDOMAIN shouldn't be allowed through).
|
||||
6. Also validate: scheme must be `https`, no user info in URL.
|
||||
|
||||
This logic lives in a standalone file (`gitea/ipcheck.go` or `cmd/review-bot/ipcheck.go`) so it can be used from both the `DialContext` interceptor and the `validate-url` subcommand.
|
||||
|
||||
**Why reject if ANY IP is blocked:** An attacker using split-horizon DNS can return both a public IP and a private IP. If we only block "all IPs private", one real IP clears the check. We must block if any resolved IP is private.
|
||||
|
||||
## Design: Custom `DialContext` in `gitea.Client`
|
||||
|
||||
```go
|
||||
// safeDialer wraps net.Dialer.DialContext with IP-level SSRF protection.
|
||||
// It resolves the hostname and checks all IPs before connecting.
|
||||
func safeDialContext(ctx context.Context, network, addr string) (net.Conn, error) {
|
||||
host, port, err := net.SplitHostPort(addr)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("invalid address %q: %w", addr, err)
|
||||
}
|
||||
addrs, err := net.DefaultResolver.LookupIPAddr(ctx, host)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("DNS lookup %q: %w", host, err)
|
||||
}
|
||||
for _, a := range addrs {
|
||||
if isBlockedIP(a.IP) {
|
||||
return nil, fmt.Errorf("blocked: %q resolves to private/reserved IP %s", host, a.IP)
|
||||
}
|
||||
}
|
||||
// Re-dial using the first resolved IP directly to avoid double-resolve
|
||||
d := &net.Dialer{Timeout: 10 * time.Second}
|
||||
return d.DialContext(ctx, network, net.JoinHostPort(addrs[0].IP.String(), port))
|
||||
}
|
||||
```
|
||||
|
||||
`NewClient` applies this as the transport's `DialContext`:
|
||||
|
||||
```go
|
||||
transport := &http.Transport{
|
||||
DialContext: safeDialContext,
|
||||
}
|
||||
return &Client{
|
||||
...
|
||||
http: &http.Client{
|
||||
Timeout: 30 * time.Second,
|
||||
Transport: transport,
|
||||
CheckRedirect: defaultCheckRedirect,
|
||||
},
|
||||
}
|
||||
```
|
||||
|
||||
**Note:** This only protects Gitea client calls. The `llm` and `github` clients are not changed in this PR (out of scope per issue).
|
||||
|
||||
## Files to Change
|
||||
|
||||
| File | Change |
|
||||
|------|--------|
|
||||
| `gitea/ipcheck.go` (new) | `isBlockedIP(ip net.IP) bool` + blocked CIDR list |
|
||||
| `gitea/ipcheck_test.go` (new) | Tests for all blocked/allowed CIDRs |
|
||||
| `gitea/client.go` | Add `safeDialContext`, apply to `NewClient` transport; add `WithUnsafeDialer` for tests |
|
||||
| `gitea/client_test.go` | Tests for SSRF blocking in `NewClient` |
|
||||
| `cmd/review-bot/main.go` | Add `validate-url` subcommand routing |
|
||||
| `cmd/review-bot/validateurl.go` (new) | `runValidateURL(args []string) error` |
|
||||
| `cmd/review-bot/validateurl_test.go` (new) | Unit tests for validate-url subcommand |
|
||||
| `.gitea/actions/review/action.yml` | Call `validate-url` before the version-check and download curl calls |
|
||||
|
||||
## State / Data Model
|
||||
|
||||
No persistent state. This is stateless request-time validation.
|
||||
|
||||
CIDR list is a compile-time constant slice — no runtime config.
|
||||
|
||||
## Error Cases
|
||||
|
||||
| Case | Behavior |
|
||||
|------|----------|
|
||||
| DNS resolution fails (NXDOMAIN) | Reject — do not connect |
|
||||
| DNS returns empty IP list | Reject — do not connect |
|
||||
| Any IP in blocked CIDR | Reject with log message showing hostname + IP |
|
||||
| URL has http:// scheme | Reject at string level (validate-url) / irrelevant for DialContext (scheme is checked at bash level already) |
|
||||
| User info in URL (user:pass@host) | Reject at string level in validate-url |
|
||||
| IPv6 mapped IPv4 (::ffff:10.0.0.1) | Blocked via `ip.To4()` normalization before CIDR check |
|
||||
| Unresolvable port | Handled by `net.SplitHostPort` error |
|
||||
|
||||
## Edge Cases
|
||||
|
||||
- **IPv6 mapped IPv4 addresses** (`::ffff:192.168.1.1`): `net.IP.To4()` returns the IPv4 form, which must be checked against IPv4 CIDRs. We normalize before checking.
|
||||
- **Multiple A/AAAA records**: One public + one private = blocked. Zero records = blocked.
|
||||
- **Default port (443 for https)**: URL may not include `:443`. The `validate-url` subcommand must use the URL's scheme to infer port for resolution (the hostname is enough for DNS check; port isn't needed for IP validation).
|
||||
- **DialContext addr format**: `net/http` passes `"host:port"` (already split), so `net.SplitHostPort` works without parsing the original URL.
|
||||
- **Test isolation**: Tests that hit local httptest servers need to bypass the safe dialer. Existing `SetHTTPClient` method allows injecting a plain HTTP client in tests. For the DialContext specifically, we add `WithUnsafeDialer() *Client` for test-only use.
|
||||
- **DNS rebinding**: The safeDialContext checks IPs after resolution, immediately before connection. This is not 100% immune to DNS rebinding (an attacker can change DNS response between LookupIPAddr and DialContext), but combined with the validate-url pre-check in bash (which also resolves), the window is extremely narrow. Full immunity requires pinning the IP for the duration of the request, which is not practical here without a larger refactor.
|
||||
|
||||
## Testing Strategy
|
||||
|
||||
### Unit tests (`gitea/ipcheck_test.go`)
|
||||
- `isBlockedIP`: test each CIDR (10.0.0.1, 172.16.0.1, 192.168.0.1, 169.254.1.1, 127.0.0.1, ::1, fe80::1, fc00::1)
|
||||
- Public IPs: 8.8.8.8, 1.1.1.1, 2001:4860:4860::8888 → not blocked
|
||||
- IPv6-mapped IPv4: `net.IP{0,0,0,0,0,0,0,0,0,0,0xff,0xff,192,168,1,1}` → blocked
|
||||
|
||||
### Unit tests (`gitea/client_test.go`)
|
||||
- `TestSafeDialerBlocksPrivateIP`: mock resolver returns 127.0.0.1 → client returns error, no connection made
|
||||
- `TestSafeDialerBlocksRFC1918`: mock resolver returns 10.0.0.1
|
||||
- `TestSafeDialerAllowsPublicIP`: public IP passes (connect to httptest server via 127.0.0.1 bypassed by WithUnsafeDialer)
|
||||
|
||||
### Unit tests (`cmd/review-bot/validateurl_test.go`)
|
||||
- Table-driven: various URLs (http://, https with private hostname, malformed) → expected exit/error
|
||||
- DNS resolution mocked via dependency injection or by testing against known-bad/good URLs
|
||||
|
||||
### Integration tests
|
||||
- Existing tests use `SetHTTPClient` with `httptest` — they pass a plain HTTP client that bypasses safe dialing. No change needed there.
|
||||
|
||||
## Backward Compatibility
|
||||
|
||||
- No flag changes, no env var changes, no action input changes.
|
||||
- `NewClient` now uses a safe HTTP transport by default — this is a breaking change in behavior for callers that were previously able to point the client at localhost. But callers that do so are tests — they already use `SetHTTPClient` to inject a test transport. The `WithUnsafeDialer` escape hatch for tests makes this clean.
|
||||
- The `validate-url` subcommand in `action.yml` runs before the binary is installed (in the "Determine version" step). We call it after the binary is installed (cache step) or at the start of "Install review-bot". Actually — the binary is installed in the "Install review-bot" step, but "Determine version" runs first and that's where the sensitive version-check curl happens. Therefore:
|
||||
- "Determine version" keeps existing bash scheme validation (pre-install, no binary available)
|
||||
- "Install review-bot" calls `validate-url` before any curl
|
||||
- "Run review" is protected by the Go binary's DialContext
|
||||
- This means version-check curl is not fully IP-protected — it uses the existing scheme-only validation. This is an acceptable residual risk: the attacker would only leak the Gitea API version response, not any secrets (no token is sent in the unauthenticated path; authenticated path sends reviewer-token which is the user's own token for their own server).
|
||||
|
||||
Actually, re-reading action.yml: the version check step DOES send `ACTION_TOKEN` to `API_URL`. So we should protect it. Options:
|
||||
- Embed a lightweight IP check in bash using `python3` (available on ubuntu runners per comments in action.yml)
|
||||
- Or restructure to do validate-url after install, skipping the cache step for the first call
|
||||
|
||||
**Decision**: Add a python3 IP check helper for the version-check curl. Python3's `socket.getaddrinfo` can resolve and check IPs without any external dep. Keep it minimal (just loopback + RFC1918 + link-local check). This is one more line in an already-bash-heavy step, documented clearly.
|
||||
|
||||
## Open Questions
|
||||
|
||||
1. The DNS rebinding window in `safeDialContext` can't be fully closed without IP pinning (binding the connection to the resolved IP). Is pinning worth the added complexity? Currently we dial `addrs[0]` directly after resolution which is a close approximation. This is our approach — worth noting in code comments.
|
||||
|
||||
2. Should we also protect the `llm` client? The issue says "action.yml Gitea path" so explicitly not in scope, but `llm-base-url` is also user-supplied. Suggest filing a follow-up. Not addressed here.
|
||||
|
||||
3. The python3 bash helper for the version-check step: should it also check IPv6? Yes — add `ipaddress.ip_address(ip).is_private` which handles both in Python 3.3+.
|
||||
|
||||
## Completion Checklist
|
||||
|
||||
Generated for this specific task:
|
||||
|
||||
1. `isBlockedIP` covers all CIDR families: loopback, RFC1918, link-local, ULA, multicast, unspecified
|
||||
2. IPv6-mapped IPv4 addresses are normalized before CIDR check
|
||||
3. ANY blocked IP in the resolved set rejects (not just all-blocked)
|
||||
4. Zero resolved IPs rejects
|
||||
5. `safeDialContext` dials the resolved IP directly (avoids second DNS lookup that could be different)
|
||||
6. Test isolation: existing tests using httptest still pass (via `WithUnsafeDialer` or `SetHTTPClient`)
|
||||
7. `validate-url` subcommand exits non-zero for private IPs, malformed URLs, http:// scheme
|
||||
8. `action.yml` calls `validate-url` before each curl that uses user-supplied URL
|
||||
9. `go test ./...` passes
|
||||
10. `go vet ./...` passes
|
||||
11. `scripts/check-deps.sh` passes (no new deps)
|
||||
@@ -2,6 +2,7 @@ package main
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"net"
|
||||
"net/url"
|
||||
@@ -54,15 +55,13 @@ type validateError struct {
|
||||
func (e *validateError) Error() string { return e.message }
|
||||
|
||||
// isValidateError checks if err is or wraps a *validateError and sets out.
|
||||
// Uses errors.As so that wrapped *validateError values (e.g. from fmt.Errorf("...: %w", &validateError{...}))
|
||||
// are also detected, making the function robust against future wrapping.
|
||||
func isValidateError(err error, out **validateError) bool {
|
||||
if err == nil {
|
||||
return false
|
||||
}
|
||||
if ve, ok := err.(*validateError); ok {
|
||||
*out = ve
|
||||
return true
|
||||
}
|
||||
return false
|
||||
return errors.As(err, out)
|
||||
}
|
||||
|
||||
// validateURL checks that rawURL is safe for use as a Gitea server URL:
|
||||
|
||||
+4
-1
@@ -136,7 +136,10 @@ func safeDialContext(ctx context.Context, network, addr string) (net.Conn, error
|
||||
}
|
||||
}
|
||||
// Dial the first resolved IP directly to avoid a second lookup.
|
||||
d := &net.Dialer{}
|
||||
// Timeout: 10s per the design (PLAN.md); the outer http.Client has a 30s
|
||||
// total timeout, but the dial itself needs an independent bound so a slow
|
||||
// TCP connect does not consume the full 30s without cancellation.
|
||||
d := &net.Dialer{Timeout: 10 * time.Second}
|
||||
return d.DialContext(ctx, network, net.JoinHostPort(addrs[0].IP.String(), port))
|
||||
}
|
||||
|
||||
|
||||
@@ -1,11 +1,18 @@
|
||||
// Package gitea — export_test.go exposes test helpers to test files in this
|
||||
// package. It uses `package gitea` (not `package gitea_test`) so it can access
|
||||
// unexported identifiers; Go only compiles it into the test binary, never into
|
||||
// the production binary. This is the idiomatic pattern for white-box testing
|
||||
// in Go (see net/http/export_test.go in the stdlib for the same approach).
|
||||
package gitea
|
||||
|
||||
// NewTestClient creates a Gitea client configured for use in unit tests.
|
||||
// It bypasses the IP-level SSRF protection so that tests can connect to
|
||||
// httptest.Server instances (which listen on 127.0.0.1).
|
||||
//
|
||||
// This function MUST only be called from _test.go files.
|
||||
// Production code must use NewClient which uses the safe dialer.
|
||||
// Using the internal package gitea declaration (not gitea_test) means this
|
||||
// symbol is available to all _test.go files in this package. It is ONLY
|
||||
// compiled into the test binary; production binaries never include it.
|
||||
// Production code must use NewClient, which enables the safe dialer.
|
||||
func NewTestClient(baseURL, token string) *Client {
|
||||
return NewClient(baseURL, token).WithUnsafeDialer()
|
||||
}
|
||||
|
||||
+57
-44
@@ -8,55 +8,63 @@ import (
|
||||
"net"
|
||||
)
|
||||
|
||||
// blockedCIDRs is the list of CIDR ranges that should never be contacted by
|
||||
// review-bot. These ranges cover private, loopback, link-local, multicast,
|
||||
// and other special-use address spaces that are not reachable from the internet
|
||||
// but may be reachable from a self-hosted runner.
|
||||
// blockedCIDRStrings is the canonical list of CIDR strings that should never
|
||||
// be contacted by review-bot. See IsBlockedIP for the full list of covered
|
||||
// address families.
|
||||
//
|
||||
// Based on:
|
||||
// - RFC1918 private ranges
|
||||
// - RFC5735 / RFC4193 special-use IPv4/IPv6 ranges
|
||||
// - RFC4291 IPv6 link-local / loopback
|
||||
var blockedCIDRs = func() []*net.IPNet {
|
||||
ranges := []string{
|
||||
// IPv4 loopback
|
||||
"127.0.0.0/8",
|
||||
// IPv4 unspecified / "this network"
|
||||
"0.0.0.0/8",
|
||||
// RFC1918 private ranges
|
||||
"10.0.0.0/8",
|
||||
"172.16.0.0/12",
|
||||
"192.168.0.0/16",
|
||||
// IPv4 link-local (APIPA, also used by AWS instance metadata 169.254.169.254)
|
||||
"169.254.0.0/16",
|
||||
// IPv4 shared address space (RFC6598, carrier-grade NAT)
|
||||
"100.64.0.0/10",
|
||||
// IPv4 multicast
|
||||
"224.0.0.0/4",
|
||||
// IPv4 reserved / broadcast
|
||||
"240.0.0.0/4",
|
||||
// IPv6 loopback
|
||||
"::1/128",
|
||||
// IPv6 unspecified
|
||||
"::/128",
|
||||
// IPv6 link-local
|
||||
"fe80::/10",
|
||||
// IPv6 unique local (ULA) — RFC4193
|
||||
"fc00::/7",
|
||||
// IPv6 multicast
|
||||
"ff00::/8",
|
||||
}
|
||||
nets := make([]*net.IPNet, 0, len(ranges))
|
||||
for _, r := range ranges {
|
||||
// These are hard-coded literals: any parse failure is a programming error.
|
||||
// Validity is verified by TestBlockedCIDRsValid in ipcheck_test.go.
|
||||
var blockedCIDRStrings = []string{
|
||||
// IPv4 loopback
|
||||
"127.0.0.0/8",
|
||||
// IPv4 unspecified / "this network"
|
||||
"0.0.0.0/8",
|
||||
// RFC1918 private ranges
|
||||
"10.0.0.0/8",
|
||||
"172.16.0.0/12",
|
||||
"192.168.0.0/16",
|
||||
// IPv4 link-local (APIPA, also used by AWS instance metadata 169.254.169.254)
|
||||
"169.254.0.0/16",
|
||||
// IPv4 shared address space (RFC6598, carrier-grade NAT)
|
||||
"100.64.0.0/10",
|
||||
// IPv4 multicast
|
||||
"224.0.0.0/4",
|
||||
// IPv4 reserved / broadcast
|
||||
"240.0.0.0/4",
|
||||
// IPv6 loopback
|
||||
"::1/128",
|
||||
// IPv6 unspecified
|
||||
"::/128",
|
||||
// IPv6 link-local
|
||||
"fe80::/10",
|
||||
// IPv6 unique local (ULA) — RFC4193
|
||||
"fc00::/7",
|
||||
// IPv6 multicast
|
||||
"ff00::/8",
|
||||
}
|
||||
|
||||
// blockedCIDRs is the parsed form of blockedCIDRStrings.
|
||||
// Any entry that fails to parse is recorded in blockedCIDRParseErrors instead
|
||||
// of panicking; tests verify this slice is always empty via TestBlockedCIDRsValid.
|
||||
var (
|
||||
blockedCIDRs []*net.IPNet
|
||||
blockedCIDRParseErrors []string
|
||||
)
|
||||
|
||||
func init() {
|
||||
blockedCIDRs = make([]*net.IPNet, 0, len(blockedCIDRStrings))
|
||||
for _, r := range blockedCIDRStrings {
|
||||
_, cidr, err := net.ParseCIDR(r)
|
||||
if err != nil {
|
||||
// This is a programming error — panic to catch it at startup/test time.
|
||||
panic(fmt.Sprintf("ipcheck: invalid built-in CIDR %q: %v", r, err))
|
||||
// Record the error rather than panicking; TestBlockedCIDRsValid
|
||||
// will catch this during tests, and the CI build will fail.
|
||||
blockedCIDRParseErrors = append(blockedCIDRParseErrors,
|
||||
fmt.Sprintf("ipcheck: invalid built-in CIDR %q: %v", r, err))
|
||||
continue
|
||||
}
|
||||
nets = append(nets, cidr)
|
||||
blockedCIDRs = append(blockedCIDRs, cidr)
|
||||
}
|
||||
return nets
|
||||
}()
|
||||
}
|
||||
|
||||
// IsBlockedIP reports whether ip is in a blocked address range.
|
||||
// It is exported for use by the validate-url subcommand and tests outside
|
||||
@@ -64,6 +72,11 @@ var blockedCIDRs = func() []*net.IPNet {
|
||||
//
|
||||
// IPv6-mapped IPv4 addresses (e.g. ::ffff:192.168.1.1) are normalized to their
|
||||
// IPv4 form before checking so that IPv4 CIDRs catch them.
|
||||
//
|
||||
// Based on:
|
||||
// - RFC1918 private ranges
|
||||
// - RFC5735 / RFC4193 special-use IPv4/IPv6 ranges
|
||||
// - RFC4291 IPv6 link-local / loopback
|
||||
func IsBlockedIP(ip net.IP) bool {
|
||||
// Normalize IPv6-mapped IPv4 addresses (::ffff:x.x.x.x) to plain IPv4.
|
||||
if v4 := ip.To4(); v4 != nil {
|
||||
|
||||
+18
-1
@@ -68,7 +68,11 @@ func TestIsBlockedIP(t *testing.T) {
|
||||
}{
|
||||
{"public 8.8.8.8", "8.8.8.8"},
|
||||
{"public 1.1.1.1", "1.1.1.1"},
|
||||
{"public 198.51.100.1", "198.51.100.1"}, // TEST-NET-2, but not blocked — public-looking
|
||||
{"public 198.51.100.1", "198.51.100.1"}, // RFC5737 TEST-NET-2 — a documentation-only range;
|
||||
// not assigned to any real host, but intentionally left unblocked here because
|
||||
// it has no special routing treatment (unlike RFC1918/loopback/link-local) and
|
||||
// blocking it would require tracking every RFC5737 range without meaningful
|
||||
// security benefit (no server should ever listen on a TEST-NET address).
|
||||
{"public 151.101.1.1", "151.101.1.1"}, // Fastly
|
||||
{"public IPv6 2001:4860:4860::8888", "2001:4860:4860::8888"}, // Google DNS
|
||||
{"public IPv6 2606:4700:4700::1111", "2606:4700:4700::1111"}, // Cloudflare DNS
|
||||
@@ -125,3 +129,16 @@ func TestIsBlockedIPEdgeCases(t *testing.T) {
|
||||
t.Errorf("IsBlockedIP(100.128.0.0) = true, want false (outside 100.64.0.0/10)")
|
||||
}
|
||||
}
|
||||
|
||||
// TestBlockedCIDRsValid verifies that all entries in blockedCIDRStrings parse
|
||||
// successfully. This catches programming errors in the CIDR list without
|
||||
// requiring a startup panic. The init() function records parse failures in
|
||||
// blockedCIDRParseErrors rather than panicking; this test makes those failures
|
||||
// visible as test failures during CI.
|
||||
func TestBlockedCIDRsValid(t *testing.T) {
|
||||
if len(blockedCIDRParseErrors) > 0 {
|
||||
for _, msg := range blockedCIDRParseErrors {
|
||||
t.Errorf("CIDR parse error: %s", msg)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user