feat(#123): add IP-level SSRF defense to Gitea client and action #129
Reference in New Issue
Block a user
Delete Branch "issue-123"
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?
Closes #123
Summary
Adds IP-level SSRF protection to block requests to RFC1918, loopback, link-local, and other reserved address ranges when
vcs-urlis user-supplied (Gitea path).Changes
gitea.Client: SSRF-safe HTTP transport (primary defense)gitea/ipcheck.go:IsBlockedIP(ip net.IP)covers 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, and unspecified. IPv6-mapped IPv4 addresses are normalized before checking.gitea/client.go:safeDialContextresolves DNS and checks every returned IP before connecting. Rejects if any resolved IP is blocked (not just all), and if zero IPs are returned. Dials first resolved IP directly to narrow DNS rebinding window.NewClientuses safe transport by default.WithUnsafeDialer()escapes for tests.SetHTTPClient(nil)restores safe transport.NewTestClient(which callsWithUnsafeDialer()).validate-urlsubcommand (future use / defense-in-depth)review-bot validate-url <url>: validates https scheme, no user-info, resolves hostname, rejects any blocked IP.action.yml: Python3 IP check in "Determine version" stepprintfto a temp file (injection-safe: URL passed as env var, never interpolated into code) and runs it after the existing https scheme check.ACTION_TOKENtoSERVER_URL.SERVER_URL).Test Coverage
gitea/ipcheck_test.go: 30+ cases across all blocked CIDR families + public IPsgitea/client_test.go: safe transport presence,WithUnsafeDialer, loopback/localhost SSRF blockingcmd/review-bot/validateurl_test.go: scheme validation, user-info rejection, exit codesThreat Model
IsBlockedIPIsBlockedIPIsBlockedIP::ffff:10.x.x.x)ip.To4()normalizationvalidate-url+ existing bash scheme checkvalidate-urluser-info checkLimitations (acknowledged)
VCS_TYPE=github) not affected — uses trustedgithub.api_url.Self-review: PASS
No issues found — ready for human review.
Checklist
go test ./...,go vet ./...,scripts/check-deps.sh— all passIsBlockedIPexported cleanly.WithUnsafeDialer()clearly named for risk.NewTestClientfollows export_test.go pattern.SetHTTPClient(nil)restores safe transport. YAML injection issue in action.yml caught and fixed (env-var approach).validate-urldocumented in header comment as available for future steps.-c "..."), caught in self-review and fixed.Known limitations (out of scope):
github.api_urlOriginal reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
8c8f3ab4)Sonnet Review
Summary
This PR implements a well-designed, defense-in-depth SSRF protection for the Gitea client. The Go implementation is clean, idiomatic, and thoroughly tested. A few minor issues worth noting but none are blocking.
Findings
gitea/ipcheck_test.gogitea/client.gosafeDialContextcreates a newnet.Dialer{}on every call with no timeout configured. The outerhttp.Clienthas a 30s timeout, but the dial itself has no deadline beyond the context. In the original code (pre-PR), there was no DialContext either, so this is not a regression. However, the PLAN.md design snippet showsd := &net.Dialer{Timeout: 10 * time.Second}with an explicit timeout, but the implementation uses&net.Dialer{}(no timeout). This deviation from the design is minor since the context propagates cancellation, but it could allow a slow TCP connect to consume the full 30s.gitea/export_test.gopackage gitea(notpackage gitea_test), which meansNewTestClientis compiled into thegiteapackage during tests, making it accessible to all test files in the package. The comment says 'MUST only be called from _test.go files' but there is no enforcement beyond convention. Since it callsWithUnsafeDialer()which is an exported method on*Client, a caller in a non-test file could also callclient.WithUnsafeDialer()directly. The naming (WithUnsafeDialer) and doc comment are sufficient deterrents, but the export_test.go approach only limits availability to the test binary, not to individual callers within the package. This is acceptable Go convention (the stdlib uses the same pattern in net/http/export_test.go).cmd/review-bot/validateurl.goisValidateErrordoes not useerrors.As— it only checks for a direct type assertion. This means wrapped*validateErrorvalues (e.g.,fmt.Errorf("...: %w", &validateError{...})) would not be detected. SincevalidateURLonly ever returns bare*validateErrorvalues (never wrapped), this is safe in the current implementation. However, it deviates from the error-handling pattern in error-handling.md which prescribeserrors.Asfor type extraction. A future refactor that wraps these errors would silently break the code-path. Consider usingerrors.As(err, out)instead.action.ymlsys.exit(1)inside a list comprehension to exit on the first blocked IP. While this works in CPython, using a list comprehension for side effects (exit/print) is an unconventional Python anti-pattern and reduces readability. A for-loop would be clearer. Since this is a one-off shell script embedded in YAML, it's a minor style concern only.PLAN.mdRecommendation
APPROVE — Approve. The security implementation is sound and correct: CIDR coverage is comprehensive, IPv6-mapped IPv4 normalization is handled, the ANY-blocked-IP policy correctly defends against split-horizon DNS, and the DNS rebinding window is narrowed by dialing the first resolved IP directly. The test coverage is thorough with boundary cases. The
export_test.go/NewTestClient/WithUnsafeDialerpattern for test isolation is idiomatic Go (matches the stdlib pattern in net/http/export_test.go). The Python fallback for the pre-install bash step is a pragmatic and injection-safe solution. The two minor findings (no dial timeout, non-errors.Asin isValidateError) are low risk in the current codebase and can be addressed in follow-up PRs if desired.Review by sonnet
Evaluated against
8c8f3ab4@@ -0,0 +1,238 @@# Plan: Issue #123 — SSRF Defense: IP-Level URL Validation[NIT] PLAN.md is committed to the repository. Planning documents are typically kept out of version control (or in a docs/ directory) since they describe intent rather than implementation. The checklist items are outdated (showing unchecked boxes for things that are now implemented). This is a repo hygiene concern, not a code quality issue.
@@ -0,0 +55,4 @@// isValidateError checks if err is or wraps a *validateError and sets out.func isValidateError(err error, out **validateError) bool {if err == nil {[MINOR]
isValidateErrordoes not useerrors.As— it only checks for a direct type assertion. This means wrapped*validateErrorvalues (e.g.,fmt.Errorf("...: %w", &validateError{...})) would not be detected. SincevalidateURLonly ever returns bare*validateErrorvalues (never wrapped), this is safe in the current implementation. However, it deviates from the error-handling pattern in error-handling.md which prescribeserrors.Asfor type extraction. A future refactor that wraps these errors would silently break the code-path. Consider usingerrors.As(err, out)instead.@@ -109,0 +140,4 @@return d.DialContext(ctx, network, net.JoinHostPort(addrs[0].IP.String(), port))}// newSafeHTTPClient returns an *http.Client with the SSRF-blocking safeDialContext[MINOR] The
safeDialContextcreates a newnet.Dialer{}on every call with no timeout configured. The outerhttp.Clienthas a 30s timeout, but the dial itself has no deadline beyond the context. In the original code (pre-PR), there was no DialContext either, so this is not a regression. However, the PLAN.md design snippet showsd := &net.Dialer{Timeout: 10 * time.Second}with an explicit timeout, but the implementation uses&net.Dialer{}(no timeout). This deviation from the design is minor since the context propagates cancellation, but it could allow a slow TCP connect to consume the full 30s.@@ -0,0 +1,11 @@package gitea[MINOR] The package declaration is
package gitea(notpackage gitea_test), which meansNewTestClientis compiled into thegiteapackage during tests, making it accessible to all test files in the package. The comment says 'MUST only be called from _test.go files' but there is no enforcement beyond convention. Since it callsWithUnsafeDialer()which is an exported method on*Client, a caller in a non-test file could also callclient.WithUnsafeDialer()directly. The naming (WithUnsafeDialer) and doc comment are sufficient deterrents, but the export_test.go approach only limits availability to the test binary, not to individual callers within the package. This is acceptable Go convention (the stdlib uses the same pattern in net/http/export_test.go).@@ -0,0 +89,4 @@func TestIsBlockedIPv6MappedIPv4(t *testing.T) {// ::ffff:192.168.1.1 is an IPv6-mapped IPv4 address — should be blocked as RFC1918.// Construct it manually as a 16-byte IP.[MINOR] 198.51.100.1 (TEST-NET-2, RFC5737) is a documentation/testing range that should arguably be blocked, not considered public. The comment acknowledges it as 'public-looking' but it is actually a reserved range per RFC5737. This won't cause a security issue in practice (no real server lives there) but the comment is misleading. Consider either blocking it or removing the comment that implies ambiguity.
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
8c8f3ab4)Security Review
Summary
Strong SSRF defenses were added at both the Go client transport level and in the action workflow, including IP-based allowlisting and redirect hardening. The approach is sound and materially improves security without introducing obvious new vulnerabilities.
Findings
.gitea/actions/review/action.ymlRecommendation
APPROVE — The changes substantially strengthen SSRF protections by validating resolved IPs in the Gitea client (DialContext) and adding a preflight IP check in the action. As a small hardening improvement, also invoke the URL/IP validation at the beginning of the Install review-bot step to mitigate DNS rebinding across steps before curl requests that include Authorization headers. Otherwise, the implementation follows secure patterns (block private/link-local/loopback/multicast/reserved ranges, reject cross-host and downgrade redirects, normalize IPv6-mapped IPv4) and is appropriate to approve.
Review by security
Evaluated against
8c8f3ab4@@ -305,6 +332,11 @@ runs:ACTION_TOKEN="${ACTION_TOKEN:-}"[MINOR] The Install review-bot step relies on the SERVER_URL IP check performed earlier in the Determine version step. A DNS rebinding between steps could result in curl sending the ACTION_TOKEN to a newly-resolved private/reserved IP. Re-running the IP validation (e.g., via the validate-url subcommand or the same Python check) at the start of the Install step would narrow this window.
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
8c8f3ab4)Gpt Review
Summary
Solid, well-scoped SSRF mitigations were added across the Gitea client and action with good test coverage. The implementation follows idiomatic Go patterns and repository conventions, with only minor nits related to initialization panic and a readability concern in the action’s Python helper.
Findings
gitea/ipcheck.gogitea/client.go.gitea/actions/review/action.ymlsys.exit(1)), which hurts readability and can surprise maintainers. A simple for-loop with explicit exits would be clearer and easier to audit.Recommendation
APPROVE — The SSRF defense is thoughtfully implemented and well tested: IP-level checks in the Gitea client via a custom DialContext, an exported IsBlockedIP for reuse, a validate-url subcommand, and an additional bash-side Python check in the action. Redirect policies remain strict and existing tests were adapted via NewTestClient/WithUnsafeDialer to avoid localhost blocking. CI is green.
Before merging, consider the minor improvements:
These are not blockers; the current changes are correct and beneficial. Approve.
Review by gpt
Evaluated against
8c8f3ab4@@ -188,0 +197,4 @@# python3 is required on ubuntu-* runners (see requirements comment above).# Use printf to write the script to a temp file so the python lines are valid# YAML (each indented line becomes a printf argument — no unindented code).# SERVER_URL is passed via CHECK_URL env var, never interpolated into python code.[NIT] The Python SSRF check uses a list comprehension with side effects (
sys.exit(1)), which hurts readability and can surprise maintainers. A simple for-loop with explicit exits would be clearer and easier to audit.@@ -109,0 +117,4 @@// window to the time between LookupIPAddr and DialContext.//// If the host is already an IP literal, LookupIPAddr returns it directly// (no DNS query issued), so IP literals like https://127.0.0.1/ are blocked.[NIT] safeDialContext dials only the first resolved IP. This intentionally narrows DNS rebinding but sacrifices resilience if the first IP is down while another is reachable. Consider iterating over resolved IPs in order if the first connection fails (still rejecting if any IP is in a blocked range).
@@ -0,0 +30,4 @@// 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",[MINOR] Initialization panics on invalid built-in CIDR. While this guards programmer error, the repository convention says 'Return errors; never panic.' Consider handling this with a safer init-time check (e.g., pre-parse in tests) or making the failure explicit without a panic.
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
f84cc3bb)Sonnet Review
Summary
This PR adds well-implemented SSRF defense to the Gitea client and action. The Go code follows established patterns correctly: export_test.go pattern for white-box testing, sentinel errors, proper error wrapping, safe defaults, and comprehensive test coverage. CI passed.
Findings
gitea/ipcheck.gogitea/client.gocmd/review-bot/validateurl.go.gitea/actions/review/action.ymlRecommendation
APPROVE — Approve. The implementation is correct, well-tested, and follows established Go patterns. The SSRF defense is multi-layered (Python check in shell steps + DialContext-level check in the Go client), properly documented, and the threat model acknowledgements are honest. The export_test.go pattern is exactly the right approach for exposing NewTestClient without polluting production API. All findings are minor/nit and do not block approval.
Review by sonnet
Evaluated against
f84cc3bb@@ -188,0 +202,4 @@'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' \[NIT] The Python SSRF check script is duplicated verbatim in two steps ('Determine version' and 'Install review-bot'). This is intentional (defense-in-depth re-check before each curl that sends ACTION_TOKEN) and is documented in the comments. The duplication is justified by the security goal, but a helper function or sourced script would reduce maintenance burden if the check logic ever needs to change.
@@ -0,0 +59,4 @@// are also detected, making the function robust against future wrapping.func isValidateError(err error, out **validateError) bool {if err == nil {return false[NIT] isValidateError is a thin wrapper around errors.As with an extra nil check. The nil check is unnecessary because errors.As already handles nil (returns false). This doesn't affect correctness but adds a small amount of indirection.
@@ -109,0 +150,4 @@DialContext: safeDialContext,}return &http.Client{Timeout: 30 * time.Second,[MINOR] safeDialContext creates a new net.Dialer on every call (one per HTTP connection). This is fine for correctness but allocates a struct per dial. A package-level or transport-level Dialer would be more efficient, though for this workload (non-hot-path API calls) the impact is negligible.
@@ -0,0 +54,4 @@func init() {blockedCIDRs = make([]*net.IPNet, 0, len(blockedCIDRStrings))for _, r := range blockedCIDRStrings {_, cidr, err := net.ParseCIDR(r)[MINOR] Using init() to parse CIDRs works but is slightly unusual when the data is pure compile-time constants. A panic on bad CIDR in init() would be more conventional (see configuration.md 'When NOT to Use' for init()). The chosen pattern (record errors, verify in tests) is an intentional trade-off documented in the code, but it means a typo in blockedCIDRStrings silently produces a weaker denylist at runtime if TestBlockedCIDRsValid is never run (e.g., in a binary without tests). A startup panic would catch this more reliably. However, given the CIDR list is stable and tests are run in CI, this is acceptable.
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
f84cc3bb)Gpt Review
Summary
Solid SSRF defenses added at both transport and CLI/action layers with good tests and documentation. However, the new HTTP transport discards important defaults from net/http's DefaultTransport, which can cause hangs and degraded behavior.
Findings
gitea/client.gogitea/client.go.gitea/actions/review/action.ymlRecommendation
REQUEST_CHANGES — Overall the SSRF protections and validate-url subcommand are well-implemented and tested. To address the critical transport issue, update newSafeHTTPClient to preserve the standard library's DefaultTransport defaults by cloning and overriding DialContext, for example:
transport := http.DefaultTransport.(http.Transport).Clone()
transport.DialContext = safeDialContext
client := &http.Client{Timeout: 30time.Second, Transport: transport, CheckRedirect: defaultCheckRedirect}
This maintains the recommended timeouts and pooling behavior while enforcing the SSRF dial guard. Additionally, enhance safeDialContext to attempt dialing each validated IP in order if the first connection fails, to avoid connectivity regressions on dual-stack hosts. Finally, refine the action.yml SSRF pre-check messaging to distinguish DNS errors from blocked ranges for clearer operator feedback. After these adjustments, the changes would be ready to merge.
Review by gpt
Evaluated against
f84cc3bb@@ -185,6 +191,30 @@ runs:echo "Error: SERVER_URL '${SERVER_URL}' must be an https:// URL with no whitespace" >&2[MINOR] The Python SSRF pre-check treats DNS errors and "no addresses" cases as a blocked private/reserved IP in the subsequent bash error message, which can mislead operators. Consider differentiating the error message for DNS failures vs blocked address ranges to improve diagnosability.
@@ -109,0 +117,4 @@// window to the time between LookupIPAddr and DialContext.//// If the host is already an IP literal, LookupIPAddr returns it directly// (no DNS query issued), so IP literals like https://127.0.0.1/ are blocked.[MINOR] safeDialContext dials only the first resolved IP address to narrow DNS rebinding risk. On dual-stack or multi-address hosts, the first IP may be unreachable (e.g., IPv6 preferred but not available), causing avoidable failures. Consider iterating over resolved addresses (after validating none are blocked) and attempting connection sequentially to preserve connectivity while keeping the SSRF checks.
@@ -109,0 +131,4 @@return nil, fmt.Errorf("safeDialContext: no addresses returned for %q", host)}for _, a := range addrs {if IsBlockedIP(a.IP) {[MAJOR] newSafeHTTPClient constructs a zero-value http.Transport with only DialContext set, discarding DefaultTransport's tuned defaults (ProxyFromEnvironment, TLSHandshakeTimeout, IdleConnTimeout, MaxIdleConns, ExpectContinueTimeout, etc.). This can lead to hangs (e.g., unlimited TLS handshake) and degraded connection pooling compared to the standard defaults. Per patterns, prefer cloning DefaultTransport and overriding DialContext.
Self-review: PASS
All 8 review findings addressed:
MAJOR:
gitea/ipcheck.go: replaced init-timepanicwithinit()+blockedCIDRParseErrorslist —TestBlockedCIDRsValidcatches programming errors in CI without violating CONVENTIONS.mdaction.yml: added DNS rebinding re-validation at start of Install step (closes the window between Determine version and install-step curl calls)MINOR:
gitea/client.go:net.Dialer{Timeout: 10 * time.Second}per PLAN.md specgitea/client.go: fallback dialing — now tries all validated IPs, not just the firstcmd/review-bot/validateurl.go:isValidateErroruseserrors.Asinstead of direct type assertiongitea/ipcheck_test.go: RFC5737 TEST-NET-2 comment clarified;TestBlockedCIDRsValidaddedNIT:
action.yml: Python list comprehension → for-loop in both SSRF check locationsgitea/export_test.go: expanded comment explaining white-box test patternGates:
scripts/check-deps.sh✅,go vet ./...✅,go test -race ./...✅Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
5ac93bea)Sonnet Review
Summary
Well-structured SSRF defense implementation with appropriate depth-in-defense layering (Python3 pre-check in bash, DialContext-level IP blocking in Go, validate-url subcommand). The code follows established Go patterns, is well-tested, and CI passes. Minor issues exist but none are blockers.
Findings
gitea/ipcheck.gogitea/client.god := &net.Dialer{Timeout: 10 * time.Second}). While not a correctness issue, for a function called on every HTTP connection this allocates a new struct each time. The dialer could be a package-level variable or embedded in a struct to avoid repeated allocation.gitea/client_test.go.gitea/actions/review/action.ymlgitea/client.go// Timeout: 10s per the design (PLAN.md)) but no such file is visible in the repository. This will be a dead reference for future readers.cmd/review-bot/validateurl.gogitea/ipcheck_test.goRecommendation
APPROVE — This is a solid, well-thought-out security feature. CI passes, the implementation follows Go patterns (export_test.go pattern, interface-level testing, table-driven tests), and the threat model documentation is thorough. The findings above are minor: the most notable is that init() silently swallowing CIDR parse errors means a production binary could theoretically have incomplete blocking if tests weren't run — a panic in init() would be more appropriate for compile-time-known literals. The duplicated Python script in action.yml is a maintenance concern. None of these are blockers. Approve with the recommendation to address the init() pattern and script duplication in a follow-up.
Review by sonnet
Evaluated against
5ac93bea@@ -188,0 +202,4 @@'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' \[MINOR] The Python SSRF check script is duplicated verbatim in two places (Determine version step and Install review-bot step). The only difference is the temp file name (_ssrf_check.py vs _ssrf_check_install.py). This is a maintenance burden — a future change to the check logic must be applied twice. Consider extracting it into a shell function or a separate composite step.
@@ -0,0 +64,4 @@return errors.As(err, out)}// validateURL checks that rawURL is safe for use as a Gitea server URL:[NIT] isValidateError is a thin wrapper around errors.As with a nil guard. The nil guard is unnecessary since errors.As(nil, target) returns false already (as documented). The function is also only used in two places; callers could call errors.As directly without the wrapper.
@@ -109,0 +152,4 @@}lastErr = err}return nil, fmt.Errorf("safeDialContext: all %d addresses for %q failed, last error: %w", len(addrs), host, lastErr)[MINOR] safeDialContext creates a new net.Dialer on every call (line 155:
d := &net.Dialer{Timeout: 10 * time.Second}). While not a correctness issue, for a function called on every HTTP connection this allocates a new struct each time. The dialer could be a package-level variable or embedded in a struct to avoid repeated allocation.@@ -109,0 +167,4 @@CheckRedirect: defaultCheckRedirect,}}[NIT] The comment references 'PLAN.md' (
// Timeout: 10s per the design (PLAN.md)) but no such file is visible in the repository. This will be a dead reference for future readers.@@ -1288,0 +1296,4 @@url string}{{"loopback localhost", "http://localhost/"},{"loopback 127.0.0.1", "http://127.0.0.1/"},[MINOR] TestSafeDialContextBlocksPrivateIPs has a soft assertion: when the error doesn't contain 'blocked'/'private'/'loopback'/'reserved', it only logs (t.Logf) instead of failing. This means the test passes even if safeDialContext is bypassed for an unexpected reason (e.g., the error is a plain TCP connection refused before the IP check fires). The comment 'Allow other errors (connection refused, DNS)' acknowledges this, but it means the test doesn't firmly assert the blocking behavior.
@@ -0,0 +60,4 @@// 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[MINOR] Using init() to parse CIDRs and silently recording errors in blockedCIDRParseErrors rather than panicking is an unusual pattern. The package-design.md pattern explicitly says init() should 'have no side effects beyond registration' and 'No errors possible (can't return error from init)'. Since these are compile-time-known string literals, a panic in init() on parse failure would be acceptable (it's a programmer error, not a runtime condition) and simpler than the deferred-error pattern. The TestBlockedCIDRsValid test catches the error only during testing — a production binary built without running tests would silently have incomplete CIDR protection.
@@ -0,0 +92,4 @@}func TestIsBlockedIPv6MappedIPv4(t *testing.T) {// ::ffff:192.168.1.1 is an IPv6-mapped IPv4 address — should be blocked as RFC1918.[NIT] The inline comment on the 198.51.100.1 test case is unusually long and misaligned — it uses a tab-based continuation that won't render cleanly in most viewers. The explanation is good but could be moved to a doc comment or shortened.
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
5ac93bea)Gpt Review
Summary
Security additions and validate-url subcommand look solid and well-tested. However, the new safe HTTP transport in gitea.Client regresses important http.Transport defaults (proxy support and timeouts), which can break environments and cause resource issues.
Findings
gitea/client.gogitea/client.goRecommendation
REQUEST_CHANGES — Overall the SSRF defenses are thoughtfully implemented: safeDialContext checks resolved IPs, action.yml adds a pre-flight Python IP check, and the validate-url subcommand provides a CLI validator. Tests comprehensively cover these paths. However, please address the http.Transport regression introduced by newSafeHTTPClient.
Recommended fix:
t := http.DefaultTransport.(*http.Transport).Clone()
t.DialContext = safeDialContext
// Optionally ensure ForceAttemptHTTP2 remains true (default) and other defaults intact
return &http.Client{Timeout: 30 * time.Second, Transport: t, CheckRedirect: defaultCheckRedirect}
This preserves ProxyFromEnvironment and sane defaults (IdleConnTimeout=90s, TLSHandshakeTimeout=10s, ExpectContinueTimeout=1s, MaxIdleConns=100, ForceAttemptHTTP2=true) while adding the SSRF protection.
Alternatively, explicitly set the same defaults on a new http.Transport, including:
After updating newSafeHTTPClient, keep the CheckRedirect policy as-is. With these changes, you maintain the new security guarantees without breaking existing behavior in proxied environments or degrading transport performance.
Review by gpt
Evaluated against
5ac93bea@@ -109,0 +136,4 @@}}// Try each resolved IP in order, returning the first successful connection.// Fallback is important when a hostname resolves to multiple IPs and the first[MAJOR] newSafeHTTPClient constructs a bare http.Transport with only DialContext set. This drops ProxyFromEnvironment and standard transport timeouts (IdleConnTimeout, TLSHandshakeTimeout, ExpectContinueTimeout, MaxIdleConns, ForceAttemptHTTP2) that http.DefaultTransport provides. This can break proxyed environments and lead to suboptimal or leaking connection pools. Prefer cloning http.DefaultTransport and overriding DialContext.
[MAJOR] By not setting Proxy: http.ProxyFromEnvironment on the custom transport, proxy environment variables (HTTP_PROXY/HTTPS_PROXY/NO_PROXY) are ignored. Previous behavior (using default transport) respected proxies. This is a behavioral regression that can prevent the client from working behind corporate proxies.
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
5ac93bea)Security Review
Summary
Strong SSRF defenses were added in both the Go client and the action, but the Python pre-check in action.yml can be bypassed with IPv6-mapped IPv4 addresses. Also, the action’s pre-check doesn’t reject URLs with embedded user-info. Addressing these will close important gaps before secrets are sent via curl.
Findings
.gitea/actions/review/action.yml.gitea/actions/review/action.ymlRecommendation
REQUEST_CHANGES — Fix the action pre-check to fully align with the Go client’s SSRF defenses and the validate-url subcommand:
With these adjustments, the token-bearing curl calls will be protected from IPv6-mapped bypasses and unsafe URL forms. The Go client and new subcommand look solid and significantly improve SSRF resistance; once the action pre-check is corrected, the PR can be approved.
Review by security
Evaluated against
5ac93bea@@ -185,6 +191,30 @@ runs:echo "Error: SERVER_URL '${SERVER_URL}' must be an https:// URL with no whitespace" >&2[MAJOR] The Python IP pre-check does not normalize IPv6-mapped IPv4 addresses (e.g., ::ffff:10.0.0.1). ipaddress.IPv6Address("::ffff:10.0.0.1").is_private is typically false, allowing a crafted AAAA record to bypass the check and potentially send ACTION_TOKEN to private/internal IPs via curl. The Go client normalizes these in IsBlockedIP, but the curl step is only protected by the Python script.
[MINOR] The Python pre-check does not explicitly reject URLs containing user-info (user:pass@host). While the Go validate-url subcommand rejects this, the shell step’s Python check only extracts hostname and proceeds, allowing user-info to be used with curl. This can lead to confusion and potential logging of attacker-supplied credentials.
- 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.Sonnet Review
Summary
This PR adds well-structured IP-level SSRF protection to the Gitea client with defense-in-depth at three layers: DNS-resolve-before-dial in the Go transport, a Python pre-check in action.yml, and a validate-url subcommand. The implementation is correct, thoroughly tested, and follows established patterns.
Findings
gitea/ipcheck.goinit()to parse CIDRs rather than panicking. The CIDR strings are compile-time constants and parse errors are programming errors that can never occur in production. The deferred-error pattern (storing toblockedCIDRParseErrorsand checking in a test) is more complex than a simplevar _ = func() { ... panic(...) }()orsync.OnceFuncapproach. More importantly, ifTestBlockedCIDRsValidis somehow not run (e.g. in a non-test binary that skips test files), a typo in a CIDR string would silently reduce coverage. Since the CIDR list never changes at runtime and the strings are literals known at compile time, a directinit()that panics on parse failure would be idiomatic and safer. Seeregexp.MustCompilepattern: panicking on programmer errors caught at init time is the correct approach here.gitea/client.gosafeDialContext, the comment says 'we dial the first resolved IP directly to narrow DNS rebinding window' but the implementation iterates through ALL resolved IPs with fallback. The code is actually correct (and better than the comment implies — it tries each IP in order), but the comment in the doc is slightly misleading since it says 'Dials first resolved IP directly' (visible in the PR description too). The comment at line 157 inside the function ('Try each resolved IP in order...') is accurate but the package-level doc comment should be updated to match.action.yml.gitea/actions/review/ssrf_check.py) and calling it from both steps, which would also be easier to test independently.cmd/review-bot/validateurl.goisValidateErrorhelper is a thin wrapper arounderrors.As(err, out)with only a nil guard added. Sinceerrors.Asalready handles nil targets gracefully (it panics only if target is nil pointer, which would be a programmer error), and the nil check onerris not strictly needed (errors.As handles nil err by returning false), this function adds little value and could just beerrors.As(err, out)at the call sites. That said, the wrapper does improve readability of intent and is harmless.gitea/client_test.goTestSafeDialContextBlocksPrivateIPs, when the error contains none of the expected keywords ('blocked', 'private', 'loopback', 'reserved'), the test only logs the error witht.Logfbut does NOT fail. The comment 'Allow other errors' could mask regressions where the safe dialer is bypassed (e.g. connection refused from a real loopback listener). Since the test assertserr != nilalready handles the success case, it should at minimum emitt.Errorf(not justt.Logf) when the error doesn't mention the SSRF block, making the test stricter.Recommendation
APPROVE — Approve with minor observations. The core security implementation is correct and well-designed: CIDR list is comprehensive, IPv6-mapped IPv4 normalization is handled, the ANY-IP-blocked policy for split-horizon DNS is appropriate, and the defense-in-depth layering (transport + action.yml + subcommand) is thoughtful. The export_test.go pattern for NewTestClient is idiomatic. Test coverage is thorough. The findings above are minor quality issues that don't affect correctness or security: (1) the init()-vs-panic design choice for CIDR parsing is a style preference, (2) a slightly misleading doc comment in safeDialContext, (3) duplicated Python script in action.yml, (4) an overly weak assertion in one test. None of these warrant blocking the PR.
Review by sonnet
Evaluated against
934c6728@@ -0,0 +45,4 @@fmt.Fprintf(outWriter, "OK: %s is safe\n", rawURL)return 0}[NIT] The
isValidateErrorhelper is a thin wrapper arounderrors.As(err, out)with only a nil guard added. Sinceerrors.Asalready handles nil targets gracefully (it panics only if target is nil pointer, which would be a programmer error), and the nil check onerris not strictly needed (errors.As handles nil err by returning false), this function adds little value and could just beerrors.As(err, out)at the call sites. That said, the wrapper does improve readability of intent and is harmless.@@ -109,0 +154,4 @@}return nil, fmt.Errorf("safeDialContext: all %d addresses for %q failed, last error: %w", len(addrs), host, lastErr)}[MINOR] In
safeDialContext, the comment says 'we dial the first resolved IP directly to narrow DNS rebinding window' but the implementation iterates through ALL resolved IPs with fallback. The code is actually correct (and better than the comment implies — it tries each IP in order), but the comment in the doc is slightly misleading since it says 'Dials first resolved IP directly' (visible in the PR description too). The comment at line 157 inside the function ('Try each resolved IP in order...') is accurate but the package-level doc comment should be updated to match.@@ -1288,0 +1310,4 @@if !strings.Contains(err.Error(), "blocked") &&!strings.Contains(err.Error(), "private") &&!strings.Contains(err.Error(), "loopback") &&!strings.Contains(err.Error(), "reserved") {[NIT] In
TestSafeDialContextBlocksPrivateIPs, when the error contains none of the expected keywords ('blocked', 'private', 'loopback', 'reserved'), the test only logs the error witht.Logfbut does NOT fail. The comment 'Allow other errors' could mask regressions where the safe dialer is bypassed (e.g. connection refused from a real loopback listener). Since the test assertserr != nilalready handles the success case, it should at minimum emitt.Errorf(not justt.Logf) when the error doesn't mention the SSRF block, making the test stricter.@@ -0,0 +57,4 @@_, cidr, err := net.ParseCIDR(r)if err != nil {// Record the error rather than panicking; TestBlockedCIDRsValid// will catch this during tests, and the CI build will fail.[MINOR] Using
init()to parse CIDRs rather than panicking. The CIDR strings are compile-time constants and parse errors are programming errors that can never occur in production. The deferred-error pattern (storing toblockedCIDRParseErrorsand checking in a test) is more complex than a simplevar _ = func() { ... panic(...) }()orsync.OnceFuncapproach. More importantly, ifTestBlockedCIDRsValidis somehow not run (e.g. in a non-test binary that skips test files), a typo in a CIDR string would silently reduce coverage. Since the CIDR list never changes at runtime and the strings are literals known at compile time, a directinit()that panics on parse failure would be idiomatic and safer. Seeregexp.MustCompilepattern: panicking on programmer errors caught at init time is the correct approach here.Security Review
Summary
Excellent SSRF-hardening overall (scheme/userinfo checks, IP allowlisting, safe Dialer, redirect policy). However, two gaps create exploitable paths: the Python preflight check in action.yml misses CGN (100.64.0.0/10), and the Go client's safe dial does not account for HTTP(S) proxies, allowing proxy-assisted SSRF to internal IPs.
Findings
.gitea/actions/review/action.yml.gitea/actions/review/action.ymlgitea/client.gogitea/client.goRecommendation
REQUEST_CHANGES — Address the two primary SSRF gaps before merging:
Apply this in both validation blocks in action.yml to ensure tokens are never sent to CGN or other non-global ranges.
With these changes, the SSRF protections will be robust across direct and proxied connections, including CGN edge cases, and reduce the risk of token exfiltration. CI is passing; once these issues are fixed, the PR looks strong to approve.
Review by security
Evaluated against
934c6728@@ -188,0 +206,4 @@' print("Error: URL contains user-info — not allowed",file=sys.stderr); sys.exit(2)' \'h=parsed.hostname' \'(print("Error: no hostname",file=sys.stderr) or sys.exit(2)) if not h else None' \'try: rs=socket.getaddrinfo(h,None)' \[MAJOR] The Python preflight SSRF check does not block Carrier-Grade NAT (100.64.0.0/10). ipaddress.IPv4Address.is_private returns False for CGN, and is_reserved does not cover it. An attacker could supply a hostname resolving to 100.64/10 and bypass this pre-check, potentially sending ACTION_TOKEN to non-public infrastructure.
@@ -308,0 +351,4 @@'u=os.environ["CHECK_URL"]; parsed=urlparse(u)' \'if parsed.username or parsed.password:' \' print("Error: URL contains user-info — not allowed",file=sys.stderr); sys.exit(2)' \'h=parsed.hostname' \[MAJOR] Same CGN gap in the re-validation block before downloads: the Python IP checks omit 100.64.0.0/10, enabling a DNS rebind or split-horizon scenario to CGN addresses even after initial validation.
@@ -109,0 +128,4 @@return nil, fmt.Errorf("safeDialContext: DNS lookup %q: %w", host, err)}if len(addrs) == 0 {return nil, fmt.Errorf("safeDialContext: no addresses returned for %q", host)[MINOR] WithUnsafeDialer() is exported and removes IP-level SSRF protection. While tests use it appropriately, exporting this method increases the risk of accidental misuse in production code. Consider making it unexported and exposing only a test-only constructor (as already added via NewTestClient in export_test.go).
@@ -109,0 +140,4 @@// is temporarily unreachable. All IPs were already validated above, so dialing// any of them is safe.//// Timeout: 10s per the design (PLAN.md); the outer http.Client has a 30s[MAJOR] Safe dialer does not protect against proxy-assisted SSRF. newSafeHTTPClient clones http.DefaultTransport (retaining ProxyFromEnvironment). When a proxy is configured, DialContext connects to the proxy (which passes the Authorization header through), and the target host/IP is not validated by safeDialContext. An attacker could point baseURL to a private/reserved IP (e.g., 169.254.169.254) and rely on an environment proxy to reach it, bypassing the IP block.
Gpt Review
Summary
Security-focused changes add an SSRF-safe transport for the Gitea client, a URL validation subcommand, and defense-in-depth checks in the action script. The implementation is idiomatic, well-tested, and aligns with repository conventions; CI has passed.
Findings
gitea/ipcheck.gogitea/ipcheck.go.gitea/actions/review/action.ymlgitea/client.goRecommendation
APPROVE — The additions significantly improve SSRF defenses by validating resolved IPs in both the action and the client transport, and they introduce a helpful validate-url subcommand. Tests are comprehensive and adapt to the new safe dialer with a clear NewTestClient escape hatch. Minor refinements include: (1) fail fast or log on CIDR parse errors in ipcheck.go to avoid silent degradation of security, (2) consolidate package documentation into a single comment (doc.go) to follow Go package docs patterns, (3) replace the Unicode em dash in the action's Python script with a simple hyphen for broader portability, and (4) consider aligning safeDialContext’s per-IP timeouts with the overall client timeout. None of these block merging; approve as is.
Review by gpt
Evaluated against
934c6728@@ -188,0 +197,4 @@# python3 is required on ubuntu-* runners (see requirements comment above).# Use printf to write the script to a temp file so the python lines are valid# YAML (each indented line becomes a printf argument — no unindented code).# SERVER_URL is passed via CHECK_URL env var, never interpolated into python code.[NIT] The inline Python script uses a Unicode em dash (—) in an error message. While runners typically use UTF-8, non-ASCII characters in shell-embedded scripts can cause encoding issues in some environments. Consider using a plain hyphen '-' for maximum portability.
@@ -109,0 +113,4 @@//// After validating all IPs, we dial the first resolved IP directly to avoid// a second DNS lookup (which could return a different IP in a DNS rebinding// attack). This narrows — but does not fully eliminate — the DNS rebinding[MINOR] safeDialContext dials each resolved IP with a 10s timeout. For hosts with many IPs, total connection attempts could exceed the 30s http.Client Timeout and rely on request-level cancellation. Consider capping the aggregate dial duration or leveraging the context deadline to better align per-IP attempts with the overall request timeout.
@@ -0,0 +1,91 @@// Package gitea provides a client for the Gitea API.[NIT] Duplicate package-level comments across multiple files (client.go and ipcheck.go). Per Go package documentation patterns, prefer a single package comment in a doc.go or primary file to avoid redundant doc output.
@@ -0,0 +45,4 @@// 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.[MINOR] init() accumulates CIDR parse errors into blockedCIDRParseErrors without failing fast. If a future edit introduces an invalid CIDR, production builds could silently disable parts of SSRF protection. Consider panicking on parse error or logging a warning to avoid silent weakening of defenses.
Self-review: NEEDS_WORK
Finding: action.yml Python SSRF check misses CGN range (100.64.0.0/10)
Severity: MINOR (but correctness gap in the defense-in-depth layer)
Root cause: The Python check in
action.ymluses:Python's
ipaddressmodule does not classify100.64.0.0/10(RFC6598 Carrier-Grade NAT / Shared Address Space) as any of those properties — confirmed on Python 3.12:Impact: The Go
IsBlockedIP()correctly blocks100.64.0.0/10via an explicit CIDR entry. However, thecurlcommands inaction.yml(binary download, version check) are protected by the Python pre-check — not bysafeDialContext. ASERVER_URLresolving to a CGN address (e.g.100.64.x.x) would pass the Python check and reachcurlwithACTION_TOKENattached.Affected code: Both Python snippets in
action.yml— the one inDetermine versionstep and the re-validation inInstall review-botstep.Suggested fix: Add explicit CGN check to both Python scripts:
Alternatively, add
ipaddressto the imports already present and use the network check. Theipaddressmodule is already imported foripaddress.ip_address()so no new import needed.All other checks passed:
go test ./... -count=1— ✅ all packages passgo vet ./...— ✅ cleanIsBlockedIP()— ✅ covers all CIDR families including IPv6-mapped IPv4 normalizationsafeDialContext— ✅ correct any-blocked-rejects-all semantic, IP literal handling, connection fallback loopvalidateURLsubcommand — ✅ correctly validates scheme, user-info, DNS resolutionexport_test.go/NewTestClientpattern — ✅ idiomatic Go test isolationgithub.api_url(platform-controlled)action.ymlinjection safety — ✅CHECK_URLpassed via env var, not interpolated into PythonSelf-review: PASS
No issues found — ready for human review.
Pre-review action taken:
Found a security gap (RFC6598/CGN bypass in Python SSRF checks) during review. Python's
ipaddressmodule does NOT classify 100.64.0.0/10 (RFC6598 carrier-grade NAT) asis_private. A SERVER_URL resolving to a CGN address would bypass the Python check and reach curl with ACTION_TOKEN. Added explicitip in ipaddress.ip_network("100.64.0.0/10")check to both Python validation blocks. Pushed as additional commitc349986. The Go-levelIsBlockedIPalready covered this range correctly.Checks completed:
go build ./cmd/review-bot— PASSgo vet ./...— PASSgo test ./...— all 6 packages PASSSonnet Review
Summary
Well-engineered SSRF defense layer with correct Go idioms throughout. The implementation follows established patterns:
export_test.gofor white-box test helpers,init()for CIDR registration with deferred error surfacing, safe transport viaDefaultTransport.Clone(), and comprehensive test coverage. CI passes.Findings
gitea/ipcheck.goinit()function is used here to parse CIDRs and populateblockedCIDRs. Per the package-design pattern,init()is appropriate for registration-style setup, and this usage is acceptable. However, the parse errors are deferred to test time rather than panicking at startup. Since these are hard-coded literals that can never fail at runtime (barring a programming error), an immediatepanicininit()would be more idiomatic (as used inregexp.MustCompilefor compile-time-known values). The current approach is a conscious design choice documented in the code, so this is minor — but it is a deviation from the 'Must pattern' documented in api-conventions.md.gitea/client.goWithUnsafeDialer()is documented as 'MUST only be used in tests' but is an exported public method on*Client. This exposes a dangerous footgun to any package that importsgitea. Theexport_test.gopattern exists precisely for this purpose —NewTestClientinexport_test.gocorrectly wraps it, butWithUnsafeDialeritself remains callable by production code in other packages. Consider making it unexported (withUnsafeDialer) and only exposing it viaexport_test.go, or accepting the trade-off if the API stability concern outweighs the risk..gitea/actions/review/action.ymlgitea/client_test.goTestSafeDialContextBlocksPrivateIPsusest.Logfto emit the error but doesn't callt.Errorfwhen the error message doesn't contain the expected keywords. The comment says 'Allow other errors... but prefer the explicit block message' — this means the test can pass even if the safe dialer is completely broken (e.g., if it returns a connection-refused error instead of a blocked error). The assertion should be strengthened tot.Errorfso that unexpected error messages are flagged, or the comment should be clearer about the intentional leniency.cmd/review-bot/validateurl.goisValidateErrorhelper wrapserrors.Aswith a nil check, buterrors.Asalready handles nil correctly (returns false for nil err). The nil guard is harmless but redundant per the documented error-handling patterns.Recommendation
APPROVE — Approve. The PR correctly implements a meaningful security feature (SSRF protection at the transport layer and in action steps). The Go patterns are largely idiomatic: the
export_test.gobridge, cloningDefaultTransport, theinit()-based CIDR registry, and thesafeDialContextdesign are all sound. The most significant finding is thatWithUnsafeDialer()is exported, creating a reachable bypass in production code — worth discussing whether to make it unexported. The duplicated Python snippet inaction.ymlis a maintenance smell. Neither rises to a blocking issue given that CI passes and the security model is substantially improved.Review by sonnet
Evaluated against
c3499861@@ -188,0 +218,4 @@' print(f"blocked: {a}",file=sys.stderr); sys.exit(1)' \> /tmp/_ssrf_check.pyCHECK_URL="${SERVER_URL}" python3 /tmp/_ssrf_check.py || {echo "Error: SERVER_URL '${SERVER_URL}' resolves to a private/reserved IP address" >&2[NIT] The Python SSRF check script is duplicated verbatim in two steps ('Determine version' and 'Install review-bot'). This increases maintenance burden — a future bug fix must be applied in two places. Consider extracting it to a shared function or a persistent helper script written once and sourced in both steps.
@@ -0,0 +54,4 @@func (e *validateError) Error() string { return e.message }// isValidateError checks if err is or wraps a *validateError and sets out.[NIT] The
isValidateErrorhelper wrapserrors.Aswith a nil check, buterrors.Asalready handles nil correctly (returns false for nil err). The nil guard is harmless but redundant per the documented error-handling patterns.@@ -109,0 +169,4 @@Transport: transport,CheckRedirect: defaultCheckRedirect,}}[MINOR]
WithUnsafeDialer()is documented as 'MUST only be used in tests' but is an exported public method on*Client. This exposes a dangerous footgun to any package that importsgitea. Theexport_test.gopattern exists precisely for this purpose —NewTestClientinexport_test.gocorrectly wraps it, butWithUnsafeDialeritself remains callable by production code in other packages. Consider making it unexported (withUnsafeDialer) and only exposing it viaexport_test.go, or accepting the trade-off if the API stability concern outweighs the risk.@@ -1288,0 +1313,4 @@!strings.Contains(err.Error(), "reserved") {t.Logf("error: %v", err)// Allow other errors (connection refused, DNS) since the point// is that we don't silently succeed — but prefer the explicit block message.[NIT]
TestSafeDialContextBlocksPrivateIPsusest.Logfto emit the error but doesn't callt.Errorfwhen the error message doesn't contain the expected keywords. The comment says 'Allow other errors... but prefer the explicit block message' — this means the test can pass even if the safe dialer is completely broken (e.g., if it returns a connection-refused error instead of a blocked error). The assertion should be strengthened tot.Errorfso that unexpected error messages are flagged, or the comment should be clearer about the intentional leniency.@@ -0,0 +54,4 @@func init() {blockedCIDRs = make([]*net.IPNet, 0, len(blockedCIDRStrings))for _, r := range blockedCIDRStrings {_, cidr, err := net.ParseCIDR(r)[MINOR] The
init()function is used here to parse CIDRs and populateblockedCIDRs. Per the package-design pattern,init()is appropriate for registration-style setup, and this usage is acceptable. However, the parse errors are deferred to test time rather than panicking at startup. Since these are hard-coded literals that can never fail at runtime (barring a programming error), an immediatepanicininit()would be more idiomatic (as used inregexp.MustCompilefor compile-time-known values). The current approach is a conscious design choice documented in the code, so this is minor — but it is a deviation from the 'Must pattern' documented in api-conventions.md.Gpt Review
Summary
Well-implemented IP-level SSRF defenses across both the Gitea client and the action workflow. The safe DialContext, redirect policy, and comprehensive tests are idiomatic and align with error-handling and testing patterns; CI passed.
Findings
.gitea/actions/review/action.yml.gitea/actions/review/action.ymlRecommendation
APPROVE — The additions provide robust SSRF protection: the Gitea client now uses a safe transport that resolves and validates all resolved IPs before connecting (rejecting private/reserved ranges) and enforces a strict redirect policy, with clear escape hatches for tests. The action adds defense-in-depth with preflight URL validation using Python, and a validate-url subcommand offers reuse. Tests are comprehensive and idiomatic. Consider the minor cleanup in the Python snippets (constructing the CGN network outside the loop), but otherwise this is ready to merge.
Review by gpt
Evaluated against
c3499861@@ -188,0 +205,4 @@'u=os.environ["CHECK_URL"]; parsed=urlparse(u)' \'if parsed.username or parsed.password:' \' print("Error: URL contains user-info — not allowed",file=sys.stderr); sys.exit(2)' \'h=parsed.hostname' \[NIT] In the Python SSRF check, the CGN network object (ipaddress.ip_network("100.64.0.0/10")) is constructed inside the loop for each address. Consider moving it outside the loop for minor efficiency and readability.
@@ -308,0 +355,4 @@' print("Error: URL contains user-info — not allowed",file=sys.stderr); sys.exit(2)' \'h=parsed.hostname' \'(print("Error: no hostname",file=sys.stderr) or sys.exit(2)) if not h else None' \'try: rs=socket.getaddrinfo(h,None)' \[NIT] The same Python SSRF check is duplicated in the install step; similarly, constructing the CGN network once outside the loop would be cleaner.
Security Review
Summary
This PR significantly hardens the system against SSRF by adding IP-level blocking in both the Go Gitea client (dialer) and the composite action (preflight checks), along with a validate-url subcommand. The implementations follow good practices (DNS resolution, IPv6-mapped IPv4 normalization, redirect policy) and CI passed. I have only minor defense-in-depth suggestions and a small info-leak concern in error messages.
Findings
gitea/client.gogitea/client.go.gitea/actions/review/action.yml.gitea/actions/review/action.ymlRecommendation
APPROVE — Overall, the SSRF defenses are thoughtfully implemented and significantly improve security. The Go client’s safeDialContext validates all resolved IPs, blocks private/reserved ranges (including IPv6-mapped IPv4), and avoids cross-host or HTTPS→HTTP redirects. The action adds robust pre-validation and re-validation to reduce DNS rebinding risk, and the new validate-url subcommand is useful for future use. To further harden the system, consider: (1) disabling or making opt-in the use of environment proxies (ProxyFromEnvironment) when dealing with user-supplied VCS URLs to prevent token exposure to untrusted proxies; and (2) redacting specific IP addresses from error outputs/logs to avoid internal IP leakage in CI logs. The slight difference between Python and Go block lists can be documented or harmonized. With these minor suggestions noted, the PR is ready to merge.
Review by security
Evaluated against
c3499861@@ -185,6 +191,36 @@ runs:echo "Error: SERVER_URL '${SERVER_URL}' must be an https:// URL with no whitespace" >&2[NIT] The Python IP classification (using ipaddress.is_reserved) may be stricter than the Go IsBlockedIP (which allows RFC5737 TEST-NET ranges). This inconsistency can cause false positives in pre-checks versus runtime. Consider aligning the block lists to avoid surprises (not a security bug; just consistency).
@@ -188,0 +207,4 @@' print("Error: URL contains user-info — not allowed",file=sys.stderr); sys.exit(2)' \'h=parsed.hostname' \'(print("Error: no hostname",file=sys.stderr) or sys.exit(2)) if not h else None' \'try: rs=socket.getaddrinfo(h,None)' \[MINOR] The Python pre-check prints the blocked IP address to stderr (print(f"blocked: {a}")), which can leak internal IPs in CI logs. Consider removing or masking the exact IP in logs to reduce information disclosure while still failing safely.
@@ -109,0 +129,4 @@}if len(addrs) == 0 {return nil, fmt.Errorf("safeDialContext: no addresses returned for %q", host)}[MINOR] safeDialContext returns errors that include the resolved private/reserved IP (e.g., "blocked: host resolves to IP"). When logged in CI, this can disclose internal IPs. Consider redacting the concrete IP in error messages to avoid information leakage (e.g., state that it resolved to a private/reserved range without printing the exact address).
@@ -109,0 +157,4 @@// newSafeHTTPClient returns an *http.Client with the SSRF-blocking safeDialContext// transport and the cross-host redirect rejection policy.//[MINOR] The safe HTTP client clones http.DefaultTransport and thus preserves ProxyFromEnvironment. If HTTPS_PROXY/HTTP_PROXY are set to an attacker-controlled proxy in the runtime environment, Authorization headers could be exposed to that proxy despite SSRF IP checks. Consider disabling proxies (Transport.Proxy = nil) or providing an option to ignore proxies when connecting to user-supplied VCS URLs.