rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases 6 Wiki Activity

feat(#123): add IP-level SSRF defense to Gitea client and action #129

Merged
rodin merged 5 commits from issue-123 into main 2026-05-14 19:10:20 +00:00
Conversation 18 Commits 5 Files Changed 11
+66 -5
3 changed files with 66 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 934c6728ee - Show all commits
.gitea/actions/review/action.yml
+10 -2
View File
6
 @@ -201,13 +201,17 @@ runs:
printf '%s\n' \
'import socket,ipaddress,sys,os' \
'from urllib.parse import urlparse' \
'u=os.environ["CHECK_URL"]; h=urlparse(u).hostname' \
'u=os.environ["CHECK_URL"]; parsed=urlparse(u)' \
'if parsed.username or parsed.password:' \
sonnet-review-bot commented 2026-05-14 08:44:04 +00:00
Outdated
Review
Copy Link
Copy Source

[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.

**[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.
sonnet-review-bot commented 2026-05-14 08:46:37 +00:00
Outdated
Review
Copy Link
Copy Source

[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.

**[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.
' 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' \
gpt-review-bot commented 2026-05-14 13:44:02 +00:00
Review
Copy Link
Copy Source

[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.

**[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.
'try: rs=socket.getaddrinfo(h,None)' \
security-review-bot commented 2026-05-14 11:51:56 +00:00
Review
Copy Link
Copy Source

[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.

**[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.
'except socket.gaierror as e: print(f"DNS error: {e}",file=sys.stderr); sys.exit(1)' \
security-review-bot commented 2026-05-14 13:44:22 +00:00
Review
Copy Link
Copy Source

[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.

**[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.
'if not rs: print("Error: no addresses",file=sys.stderr); sys.exit(1)' \
'for _,_,_,_,(a,*_) in rs:' \
' ip=ipaddress.ip_address(a)' \
' if isinstance(ip,ipaddress.IPv6Address) and ip.ipv4_mapped: ip=ip.ipv4_mapped' \
' 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
2
 @@ -344,13 +348,17 @@ runs:
printf '%s\n' \
'import socket,ipaddress,sys,os' \
'from urllib.parse import urlparse' \
'u=os.environ["CHECK_URL"]; h=urlparse(u).hostname' \
'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' \
security-review-bot commented 2026-05-14 11:51:56 +00:00
Review
Copy Link
Copy Source

[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.

**[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.
'(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)' \
gpt-review-bot commented 2026-05-14 13:44:02 +00:00
Review
Copy Link
Copy Source

[NIT] The same Python SSRF check is duplicated in the install step; similarly, constructing the CGN network once outside the loop would be cleaner.

**[NIT]** The same Python SSRF check is duplicated in the install step; similarly, constructing the CGN network once outside the loop would be cleaner.
'for _,_,_,_,(a,*_) in rs:' \
' ip=ipaddress.ip_address(a)' \
' if isinstance(ip,ipaddress.IPv6Address) and ip.ipv4_mapped: ip=ip.ipv4_mapped' \
' 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
gitea/client.go
+6 -3
View File
12
 @@ -157,10 +157,13 @@ func safeDialContext(ctx context.Context, network, addr string) (net.Conn, error
sonnet-review-bot commented 2026-05-14 11:50:40 +00:00
Review
Copy Link
Copy Source

[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.

**[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.
// newSafeHTTPClient returns an *http.Client with the SSRF-blocking safeDialContext
// transport and the cross-host redirect rejection policy.
//
security-review-bot commented 2026-05-14 13:44:22 +00:00
Review
Copy Link
Copy Source

[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.

**[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.
// We clone http.DefaultTransport to preserve its production-ready defaults
// (ProxyFromEnvironment, TLSHandshakeTimeout, IdleConnTimeout, connection
// pooling, HTTP/2 support) and override only DialContext with safeDialContext.
func newSafeHTTPClient() *http.Client {
transport := &http.Transport{
DialContext: safeDialContext,
}
transport := http.DefaultTransport.(*http.Transport).Clone()
transport.DialContext = safeDialContext
return &http.Client{
Timeout: 30 * time.Second,
Transport: transport,
2
gitea/client_test.go
+50
View File
3
 @@ -1369,3 +1369,53 @@ func TestSetHTTPClient_NilRestoresSafeTransport(t *testing.T) {
t.Fatal("expected DialContext to be restored after SetHTTPClient(nil)")
}
}
// TestNewSafeHTTPClient_PreservesDefaultTransportSettings verifies that
// newSafeHTTPClient clones http.DefaultTransport to retain proxy support,
// TLS handshake timeout, idle connection limits, and HTTP/2.
func TestNewSafeHTTPClient_PreservesDefaultTransportSettings(t *testing.T) {
c := NewClient("https://gitea.example.com", "token")
transport, ok := c.http.Transport.(*http.Transport)
if !ok {
t.Fatalf("expected *http.Transport, got %T", c.http.Transport)
}
defaults := http.DefaultTransport.(*http.Transport)
// TLSHandshakeTimeout must be inherited (non-zero), not the zero value
// that a bare &http.Transport{} would have.
if transport.TLSHandshakeTimeout == 0 {
t.Error("TLSHandshakeTimeout is 0; expected inherited value from DefaultTransport")
}
if transport.TLSHandshakeTimeout != defaults.TLSHandshakeTimeout {
t.Errorf("TLSHandshakeTimeout = %v, want %v", transport.TLSHandshakeTimeout, defaults.TLSHandshakeTimeout)
}
// IdleConnTimeout must be inherited.
if transport.IdleConnTimeout == 0 {
t.Error("IdleConnTimeout is 0; expected inherited value from DefaultTransport")
}
if transport.IdleConnTimeout != defaults.IdleConnTimeout {
t.Errorf("IdleConnTimeout = %v, want %v", transport.IdleConnTimeout, defaults.IdleConnTimeout)
}
// MaxIdleConns must be inherited.
if transport.MaxIdleConns == 0 {
t.Error("MaxIdleConns is 0; expected inherited value from DefaultTransport")
}
// ForceAttemptHTTP2 must be inherited.
if !transport.ForceAttemptHTTP2 {
t.Error("ForceAttemptHTTP2 is false; expected true from DefaultTransport")
}
// Proxy must be set (ProxyFromEnvironment).
if transport.Proxy == nil {
t.Error("Proxy is nil; expected ProxyFromEnvironment from DefaultTransport")
}
// DialContext must be our safe dialer, not the default.
if transport.DialContext == nil {
t.Error("DialContext is nil; expected safeDialContext")
}
}