feat(#123): add IP-level SSRF defense to Gitea client and action
## Changes ### Go: IP-level SSRF protection in gitea.Client (primary defense) - Add gitea/ipcheck.go with IsBlockedIP() covering all blocked CIDR ranges: loopback (127.0.0.0/8, ::1), RFC1918 (10/8, 172.16/12, 192.168/16), link-local (169.254/16, fe80::/10), ULA (fc00::/7), CGN (100.64/10), multicast, reserved, and unspecified ranges. - IPv6-mapped IPv4 addresses (::ffff:x.x.x.x) are normalized before checking. - Add safeDialContext to gitea.Client: resolves DNS, rejects any IP in a blocked CIDR, then dials the resolved IP directly to narrow the DNS rebinding window. NewClient now uses this safe transport by default. - Add WithUnsafeDialer() for test code using httptest.Server (127.0.0.1). - Update NewTestClient helper in export_test.go for all gitea unit tests. - Update SetHTTPClient(nil) to restore the safe transport (not the plain one). ### Go: validate-url subcommand (defense-in-depth for future bash callers) - Add 'review-bot validate-url <url>' subcommand: validates https scheme, no user-info, resolves hostname, rejects any blocked IP. - Exit 0=safe, 1=blocked, 2=validation error/dns failure. - Add outWriter/errWriter vars to main.go for testable output capture. ### action.yml: Python3 IP check in 'Determine version' step - After the https scheme validation, resolve SERVER_URL hostname with socket.getaddrinfo and reject any result where ipaddress.ip_address(ip).is_private/is_loopback/is_link_local/etc. is true. - python3 is required on ubuntu-* runners (noted in existing comments). - Covers the version-check curl that sends ACTION_TOKEN to SERVER_URL. - SERVER_URL for install-step curls is covered by the same pre-check. ### Tests - gitea/ipcheck_test.go: 30+ cases covering all blocked families + public IPs - gitea/client_test.go: safe transport presence, WithUnsafeDialer, SSRF blocking - cmd/review-bot/validateurl_test.go: scheme validation, user-info, exit codes Closes #123
This commit is contained in:
+121
-37
@@ -36,7 +36,7 @@ func TestGetPullRequest(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
got, err := client.GetPullRequest(context.Background(), "owner", "repo", 1)
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
@@ -63,7 +63,7 @@ func TestGetPullRequestDiff(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
got, err := client.GetPullRequestDiff(context.Background(), "owner", "repo", 5)
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
@@ -88,7 +88,7 @@ func TestGetCommitStatuses(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
got, err := client.GetCommitStatuses(context.Background(), "owner", "repo", "abc123")
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
@@ -138,7 +138,7 @@ func TestPostReview(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
review, err := client.PostReview(context.Background(), "owner", "repo", 3, "APPROVED", "LGTM", "abc123def", nil)
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
@@ -158,7 +158,7 @@ func TestGetPullRequest_Non200(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
_, err := client.GetPullRequest(context.Background(), "owner", "repo", 999)
|
||||
if err == nil {
|
||||
t.Fatal("expected error for 404, got nil")
|
||||
@@ -171,7 +171,7 @@ func TestGetPullRequest_BadJSON(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
_, err := client.GetPullRequest(context.Background(), "owner", "repo", 1)
|
||||
if err == nil {
|
||||
t.Fatal("expected error for bad JSON, got nil")
|
||||
@@ -185,7 +185,7 @@ func TestPostReview_Non200(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "test", "", nil)
|
||||
if err == nil {
|
||||
t.Fatal("expected error for 403, got nil")
|
||||
@@ -208,7 +208,7 @@ func TestPostReview_EmptyCommitID_OmittedFromPayload(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "ok", "", nil)
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
@@ -226,7 +226,7 @@ func TestGetFileContent(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
got, err := client.GetFileContent(context.Background(), "owner", "repo", "CONVENTIONS.md")
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
@@ -246,7 +246,7 @@ func TestGetPullRequestFiles(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
files, err := client.GetPullRequestFiles(context.Background(), "owner", "repo", 1)
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
@@ -271,7 +271,7 @@ func TestGetFileContentRef(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
content, err := client.GetFileContentRef(context.Background(), "owner", "repo", "main.go", "feature-branch")
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
@@ -291,7 +291,7 @@ func TestListContents(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
entries, err := client.ListContents(context.Background(), "owner", "repo", "docs")
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
@@ -318,7 +318,7 @@ func TestListContents_DotPath(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
entries, err := client.ListContents(context.Background(), "owner", "repo", ".")
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
@@ -343,7 +343,7 @@ func TestListContents_FilePath(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
entries, err := client.ListContents(context.Background(), "owner", "repo", "README.md")
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
@@ -375,7 +375,7 @@ func TestGetAllFilesInPath_File(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
files, err := client.GetAllFilesInPath(context.Background(), "owner", "repo", "README.md")
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
@@ -428,7 +428,7 @@ func TestListReviews(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
reviews, err := client.ListReviews(context.Background(), "owner", "repo", 5)
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
@@ -468,7 +468,7 @@ func TestListReviews_Pagination(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
reviews, err := client.ListReviews(context.Background(), "owner", "repo", 5)
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
@@ -493,7 +493,7 @@ func TestDeleteReview(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
err := client.DeleteReview(context.Background(), "owner", "repo", 5, 10)
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
@@ -507,7 +507,7 @@ func TestDeleteReview_Forbidden(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
err := client.DeleteReview(context.Background(), "owner", "repo", 5, 10)
|
||||
if err == nil {
|
||||
t.Fatal("expected error for 403, got nil")
|
||||
@@ -536,7 +536,7 @@ func TestEditComment(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
err := client.EditComment(context.Background(), "owner", "repo", 42, "updated body")
|
||||
if err != nil {
|
||||
t.Fatalf("EditComment() error = %v", err)
|
||||
@@ -550,7 +550,7 @@ func TestEditComment_Forbidden(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
err := client.EditComment(context.Background(), "owner", "repo", 42, "new body")
|
||||
if err == nil {
|
||||
t.Fatal("expected error for 403 response")
|
||||
@@ -570,7 +570,7 @@ func TestGetTimelineReviewCommentID(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
id, err := client.GetTimelineReviewCommentID(context.Background(), "owner", "repo", 5, "<!-- review-bot:sonnet -->")
|
||||
if err != nil {
|
||||
t.Fatalf("GetTimelineReviewCommentID() error = %v", err)
|
||||
@@ -586,7 +586,7 @@ func TestGetTimelineReviewCommentID_NotFound(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
_, err := client.GetTimelineReviewCommentID(context.Background(), "owner", "repo", 5, "<!-- review-bot:sonnet -->")
|
||||
if err == nil {
|
||||
t.Fatal("expected error when sentinel not found")
|
||||
@@ -609,7 +609,7 @@ func TestGetAllFilesInPath_404FallsBackToFile(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
files, err := client.GetAllFilesInPath(context.Background(), "owner", "repo", "README.md")
|
||||
if err != nil {
|
||||
t.Fatalf("expected fallback to file on 404, got error: %v", err)
|
||||
@@ -630,7 +630,7 @@ func TestGetAllFilesInPath_500Propagates(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
_, err := client.GetAllFilesInPath(context.Background(), "owner", "repo", "somepath")
|
||||
if err == nil {
|
||||
t.Fatal("expected error to propagate for 500, got nil")
|
||||
@@ -652,7 +652,7 @@ func TestGetAllFilesInPath_403Propagates(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
_, err := client.GetAllFilesInPath(context.Background(), "owner", "repo", "private/stuff")
|
||||
if err == nil {
|
||||
t.Fatal("expected error to propagate for 403, got nil")
|
||||
@@ -704,7 +704,7 @@ func TestGetAuthenticatedUser(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
login, err := client.GetAuthenticatedUser(context.Background())
|
||||
if err != nil {
|
||||
t.Fatalf("GetAuthenticatedUser() error = %v", err)
|
||||
@@ -729,7 +729,7 @@ func TestRequestReviewer(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
err := client.RequestReviewer(context.Background(), "owner", "repo", 7, "bot-user")
|
||||
if err != nil {
|
||||
t.Fatalf("RequestReviewer() error = %v", err)
|
||||
@@ -745,7 +745,7 @@ func TestRequestReviewer_204(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
err := client.RequestReviewer(context.Background(), "owner", "repo", 1, "user")
|
||||
if err != nil {
|
||||
t.Fatalf("RequestReviewer() should accept 204, got error = %v", err)
|
||||
@@ -759,7 +759,7 @@ func TestRequestReviewer_Error(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
err := client.RequestReviewer(context.Background(), "owner", "repo", 1, "user")
|
||||
if err == nil {
|
||||
t.Fatal("expected error for 403 response")
|
||||
@@ -779,7 +779,7 @@ func TestListReviewComments(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
comments, err := client.ListReviewComments(context.Background(), "owner", "repo", 1, 42)
|
||||
if err != nil {
|
||||
t.Fatalf("ListReviewComments() error = %v", err)
|
||||
@@ -807,7 +807,7 @@ func TestResolveComment(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
err := client.ResolveComment(context.Background(), "owner", "repo", 99)
|
||||
if err != nil {
|
||||
t.Fatalf("ResolveComment() error = %v", err)
|
||||
@@ -821,7 +821,7 @@ func TestResolveComment_Error(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
err := client.ResolveComment(context.Background(), "owner", "repo", 99)
|
||||
if err == nil {
|
||||
t.Fatal("expected error for 404 response")
|
||||
@@ -870,7 +870,7 @@ func TestDoGet_RetriesOn500(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
// Use short backoff for fast tests
|
||||
client.RetryBackoff = []time.Duration{1 * time.Millisecond, 1 * time.Millisecond}
|
||||
|
||||
@@ -895,7 +895,7 @@ func TestDoGet_FailsAfterMaxRetries(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
// Use short backoff for fast tests
|
||||
client.RetryBackoff = []time.Duration{1 * time.Millisecond, 1 * time.Millisecond}
|
||||
|
||||
@@ -924,7 +924,7 @@ func TestDoGet_NoRetryOn4xx(t *testing.T) {
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
_, err := client.doGet(context.Background(), server.URL+"/test")
|
||||
if err == nil {
|
||||
t.Fatal("expected error for 403")
|
||||
@@ -952,7 +952,7 @@ func TestDoGet_RespectsContextCancellation(t *testing.T) {
|
||||
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
client := NewTestClient(server.URL, "test-token")
|
||||
// Use longer backoff to give us time to cancel during the wait
|
||||
client.RetryBackoff = []time.Duration{100 * time.Millisecond, 100 * time.Millisecond}
|
||||
|
||||
@@ -1285,3 +1285,87 @@ func TestSetHTTPClient_NilRestoresDefault(t *testing.T) {
|
||||
t.Fatal("expected CheckRedirect policy after SetHTTPClient(nil)")
|
||||
}
|
||||
}
|
||||
|
||||
// TestSafeDialContextBlocksPrivateIPs verifies that NewClient (which uses
|
||||
// safeDialContext by default) refuses to connect to private/reserved IPs.
|
||||
func TestSafeDialContextBlocksPrivateIPs(t *testing.T) {
|
||||
// These servers listen on 127.0.0.1, so the safe dialer will block them.
|
||||
// We use NewClient (NOT NewTestClient) to exercise the real safe dialer.
|
||||
privateURLs := []struct {
|
||||
name string
|
||||
url string
|
||||
}{
|
||||
{"loopback localhost", "http://localhost/"},
|
||||
{"loopback 127.0.0.1", "http://127.0.0.1/"},
|
||||
}
|
||||
|
||||
for _, tc := range privateURLs {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
c := NewClient(tc.url, "token")
|
||||
_, err := c.GetPullRequest(context.Background(), "owner", "repo", 1)
|
||||
if err == nil {
|
||||
t.Errorf("expected error connecting to %s, got nil", tc.url)
|
||||
}
|
||||
// Error must mention SSRF/blocked, not a random network error.
|
||||
if !strings.Contains(err.Error(), "blocked") &&
|
||||
!strings.Contains(err.Error(), "private") &&
|
||||
!strings.Contains(err.Error(), "loopback") &&
|
||||
!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.
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestWithUnsafeDialerAllowsLocalhost verifies that WithUnsafeDialer bypasses
|
||||
// the IP check, allowing tests to connect to httptest.Server (127.0.0.1).
|
||||
func TestWithUnsafeDialerAllowsLocalhost(t *testing.T) {
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
w.Write([]byte(`{"title":"test","body":"","head":{"sha":"abc","ref":"main"}}`))
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
// WithUnsafeDialer should allow connecting to 127.0.0.1.
|
||||
c := NewClient(server.URL, "token").WithUnsafeDialer()
|
||||
pr, err := c.GetPullRequest(context.Background(), "owner", "repo", 1)
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error with unsafe dialer: %v", err)
|
||||
}
|
||||
if pr.Title != "test" {
|
||||
t.Errorf("expected title 'test', got %q", pr.Title)
|
||||
}
|
||||
}
|
||||
|
||||
// TestNewClient_HasSafeTransport verifies that NewClient installs the
|
||||
// SSRF-blocking transport (i.e. Transport is not nil and DialContext is set).
|
||||
func TestNewClient_HasSafeTransport(t *testing.T) {
|
||||
c := NewClient("https://gitea.example.com", "token")
|
||||
if c.http.Transport == nil {
|
||||
t.Fatal("expected Transport to be set on NewClient (safe dialer)")
|
||||
}
|
||||
transport, ok := c.http.Transport.(*http.Transport)
|
||||
if !ok {
|
||||
t.Fatalf("expected *http.Transport, got %T", c.http.Transport)
|
||||
}
|
||||
if transport.DialContext == nil {
|
||||
t.Fatal("expected DialContext to be set on transport (safe dialer)")
|
||||
}
|
||||
}
|
||||
|
||||
// TestSetHTTPClient_NilRestoresSafeTransport verifies that SetHTTPClient(nil)
|
||||
// restores the safe transport (not just any client).
|
||||
func TestSetHTTPClient_NilRestoresSafeTransport(t *testing.T) {
|
||||
c := NewClient("https://gitea.example.com", "token")
|
||||
c.SetHTTPClient(&http.Client{}) // replace with plain client
|
||||
c.SetHTTPClient(nil) // restore
|
||||
transport, ok := c.http.Transport.(*http.Transport)
|
||||
if !ok {
|
||||
t.Fatalf("expected *http.Transport after SetHTTPClient(nil), got %T", c.http.Transport)
|
||||
}
|
||||
if transport.DialContext == nil {
|
||||
t.Fatal("expected DialContext to be restored after SetHTTPClient(nil)")
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user