e80b18370197c358e6792fab66c816ad27f408ce
- Author
- Vinayak Mishra <viks@vnykmshr.com>
- Committer
- GitHub <noreply@github.com>
- Date
Message
Merge commit from fork * fix: apply SSRF protection to LFS HTTP client The LFS HTTP client uses http.DefaultClient which has no SSRF protection. This allows server-side requests from LFS operations to reach private/internal networks. The webhook subsystem already has SSRF protection via secureHTTPClient with IP validation and redirect blocking, but the LFS code path was missed. Add a shared pkg/ssrf package with a secure HTTP client constructor that validates resolved IPs before dialing (blocking private, link- local, loopback, CGNAT, and reserved ranges) and blocks redirects. Replace http.DefaultClient in newHTTPClient() with ssrf.NewSecureClient() at both locations (batch API client and BasicTransferAdapter). * refactor: consolidate webhook SSRF protection into pkg/ssrf Pull shared IP validation into pkg/ssrf so both the LFS client and webhook client use the same SSRF protection. The webhook validator becomes a thin wrapper and the inline secureHTTPClient is replaced with ssrf.NewSecureClient(). Two latent issues in the webhook path fixed in the process: - nil ParseIP result was silently allowed through (now fail-closed) - IPv6-mapped IPv4 bypassed manual range checks (now normalized) Error aliases kept in pkg/webhook for backward compatibility.
Diff
This diff is truncated to protect this page.
1diff --git a/pkg/lfs/http_client.go b/pkg/lfs/http_client.go
2index 068c311355f63ba713b8e98335ecb81a8dffc303..22bfa7863176620f0de6885ae64647a7b3542e05 100644
3--- a/pkg/lfs/http_client.go
4+++ b/pkg/lfs/http_client.go
5@@ -9,6 +9,7 @@ import (
6 "net/http"
7
8 "charm.land/log/v2"
9+ "github.com/charmbracelet/soft-serve/pkg/ssrf"
10 )
11
12 // httpClient is a Git LFS client to communicate with a LFS source API.
13@@ -22,11 +23,12 @@ var _ Client = (*httpClient)(nil)
14
15 // newHTTPClient returns a new Git LFS client.
16 func newHTTPClient(endpoint Endpoint) *httpClient {
17+ client := ssrf.NewSecureClient()
18 return &httpClient{
19- client: http.DefaultClient,
20+ client: client,
21 endpoint: endpoint,
22 transfers: map[string]TransferAdapter{
23- TransferBasic: &BasicTransferAdapter{http.DefaultClient},
24+ TransferBasic: &BasicTransferAdapter{client},
25 },
26 }
27 }
28diff --git a/pkg/ssrf/ssrf.go b/pkg/ssrf/ssrf.go
29new file mode 100644
30index 0000000000000000000000000000000000000000..1ed96bd8f88c7005276e2d772161f3dbddf7c4a1
31--- /dev/null
32+++ b/pkg/ssrf/ssrf.go
33@@ -0,0 +1,178 @@
34+package ssrf
35+
36+import (
37+ "context"
38+ "errors"
39+ "fmt"
40+ "net"
41+ "net/http"
42+ "net/url"
43+ "slices"
44+ "strings"
45+ "time"
46+)
47+
48+var (
49+ // ErrPrivateIP is returned when a connection to a private or internal IP is blocked.
50+ ErrPrivateIP = errors.New("connection to private or internal IP address is not allowed")
51+ // ErrInvalidScheme is returned when a URL scheme is not http or https.
52+ ErrInvalidScheme = errors.New("URL must use http or https scheme")
53+ // ErrInvalidURL is returned when a URL is invalid.
54+ ErrInvalidURL = errors.New("invalid URL")
55+)
56+
57+// NewSecureClient returns an HTTP client with SSRF protection.
58+// It validates resolved IPs at dial time to block connections to private
59+// and internal networks. Since validation uses the already-resolved IP
60+// from the Transport's DNS lookup, there is no TOCTOU gap between
61+// resolution and connection. Redirects are disabled to match the
62+// webhook client convention and prevent redirect-based SSRF.
63+func NewSecureClient() *http.Client {
64+ return &http.Client{
65+ Timeout: 30 * time.Second,
66+ Transport: &http.Transport{
67+ DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) {
68+ host, _, err := net.SplitHostPort(addr)
69+ if err != nil {
70+ return nil, err //nolint:wrapcheck
71+ }
72+
73+ ip := net.ParseIP(host)
74+ if ip == nil {
75+ return nil, fmt.Errorf("unexpected non-IP address in dial: %s", host)
76+ }
77+ if isPrivateOrInternal(ip) {
78+ return nil, fmt.Errorf("%w", ErrPrivateIP)
79+ }
80+
81+ dialer := &net.Dialer{
82+ Timeout: 10 * time.Second,
83+ KeepAlive: 30 * time.Second,
84+ }
85+ return dialer.DialContext(ctx, network, addr)
86+ },
87+ MaxIdleConns: 100,
88+ IdleConnTimeout: 90 * time.Second,
89+ TLSHandshakeTimeout: 10 * time.Second,
90+ ExpectContinueTimeout: 1 * time.Second,
91+ },
92+ CheckRedirect: func(*http.Request, []*http.Request) error {
93+ return http.ErrUseLastResponse
94+ },
95+ }
96+}
97+
98+// isPrivateOrInternal checks if an IP address is private, internal, or reserved.
99+func isPrivateOrInternal(ip net.IP) bool {
100+ // Normalize IPv6-mapped IPv4 (e.g. ::ffff:127.0.0.1) to IPv4 form
101+ // so all checks apply consistently.
102+ if ip4 := ip.To4(); ip4 != nil {
103+ ip = ip4
104+ }
105+
106+ if ip.IsLoopback() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() ||
107+ ip.IsPrivate() || ip.IsUnspecified() || ip.IsMulticast() {
108+ return true
109+ }
110+
111+ if ip4 := ip.To4(); ip4 != nil {
112+ // 0.0.0.0/8
113+ if ip4[0] == 0 {
114+ return true
115+ }
116+ // 100.64.0.0/10 (Shared Address Space / CGNAT)
117+ if ip4[0] == 100 && ip4[1] >= 64 && ip4[1] <= 127 {
118+ return true
119+ }
120+ // 192.0.0.0/24 (IETF Protocol Assignments)
121+ if ip4[0] == 192 && ip4[1] == 0 && ip4[2] == 0 {
122+ return true
123+ }
124+ // 192.0.2.0/24 (TEST-NET-1)
125+ if ip4[0] == 192 && ip4[1] == 0 && ip4[2] == 2 {
126+ return true
127+ }
128+ // 198.18.0.0/15 (benchmarking)
129+ if ip4[0] == 198 && (ip4[1] == 18 || ip4[1] == 19) {
130+ return true
131+ }
132+ // 198.51.100.0/24 (TEST-NET-2)
133diff --git a/pkg/ssrf/ssrf_test.go b/pkg/ssrf/ssrf_test.go
134new file mode 100644
135index 0000000000000000000000000000000000000000..a3c684dcf1babf37855c20a734aceeda6ceb1107
136--- /dev/null
137+++ b/pkg/ssrf/ssrf_test.go
138@@ -0,0 +1,208 @@
139+package ssrf
140+
141+import (
142+ "context"
143+ "errors"
144+ "net"
145+ "net/http"
146+ "net/http/httptest"
147+ "testing"
148+ "time"
149+)
150+
151+func TestNewSecureClientBlocksPrivateIPs(t *testing.T) {
152+ client := NewSecureClient()
153+ transport := client.Transport.(*http.Transport)
154+
155+ tests := []struct {
156+ name string
157+ addr string
158+ wantErr bool
159+ }{
160+ {"block loopback", "127.0.0.1:80", true},
161+ {"block private 10.x", "10.0.0.1:80", true},
162+ {"block link-local", "169.254.169.254:80", true},
163+ {"block CGNAT", "100.64.0.1:80", true},
164+ {"allow public IP", "8.8.8.8:80", false},
165+ }
166+
167+ for _, tt := range tests {
168+ t.Run(tt.name, func(t *testing.T) {
169+ ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
170+ defer cancel()
171+
172+ conn, err := transport.DialContext(ctx, "tcp", tt.addr)
173+ if conn != nil {
174+ conn.Close()
175+ }
176+
177+ if tt.wantErr {
178+ if err == nil {
179+ t.Errorf("expected error for %s, got none", tt.addr)
180+ }
181+ } else {
182+ if err != nil && errors.Is(err, ErrPrivateIP) {
183+ t.Errorf("should not block %s with SSRF error, got: %v", tt.addr, err)
184+ }
185+ }
186+ })
187+ }
188+}
189+
190+func TestNewSecureClientNilIPNotErrPrivateIP(t *testing.T) {
191+ client := NewSecureClient()
192+ transport := client.Transport.(*http.Transport)
193+
194+ ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
195+ defer cancel()
196+
197+ conn, err := transport.DialContext(ctx, "tcp", "not-an-ip:80")
198+ if conn != nil {
199+ conn.Close()
200+ }
201+ if err == nil {
202+ t.Fatal("expected error for non-IP address, got none")
203+ }
204+ if errors.Is(err, ErrPrivateIP) {
205+ t.Errorf("nil-IP path should not wrap ErrPrivateIP, got: %v", err)
206+ }
207+}
208+
209+func TestNewSecureClientBlocksRedirects(t *testing.T) {
210+ redirectServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
211+ http.Redirect(w, r, "http://8.8.8.8:8080/safe", http.StatusFound)
212+ }))
213+ defer redirectServer.Close()
214+
215+ client := NewSecureClient()
216+ req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, redirectServer.URL, nil)
217+ if err != nil {
218+ t.Fatalf("Failed to create request: %v", err)
219+ }
220+
221+ resp, err := client.Do(req)
222+ if err != nil {
223+ // httptest uses 127.0.0.1, blocked by SSRF protection
224+ if !errors.Is(err, ErrPrivateIP) {
225+ t.Fatalf("Request failed with non-SSRF error: %v", err)
226+ }
227+ return
228+ }
229+ defer resp.Body.Close()
230+
231+ if resp.StatusCode != http.StatusFound {
232+ t.Errorf("Expected redirect response (302), got %d", resp.StatusCode)
233+ }
234+}
235+
236+func TestIsPrivateOrInternal(t *testing.T) {
237+ tests := []struct {
238diff --git a/pkg/webhook/ssrf_test.go b/pkg/webhook/ssrf_test.go
239index 3f7fc4948aad217d65a6e705d02fffeeda465f4f..24251613638115b1d2c513a00b6ea6f0a650b465 100644
240--- a/pkg/webhook/ssrf_test.go
241+++ b/pkg/webhook/ssrf_test.go
242@@ -2,217 +2,57 @@ package webhook
243
244 import (
245 "context"
246+ "errors"
247 "net/http"
248- "net/http/httptest"
249 "testing"
250 "time"
251
252 "github.com/charmbracelet/soft-serve/pkg/db/models"
253+ "github.com/charmbracelet/soft-serve/pkg/ssrf"
254 )
255
256-// TestSSRFProtection tests that the webhook system blocks SSRF attempts.
257+// TestSSRFProtection is an integration test verifying the webhook send path
258+// blocks private IPs end-to-end (models.Webhook -> secureHTTPClient -> ssrf).
259 func TestSSRFProtection(t *testing.T) {
260 tests := []struct {
261 name string
262 webhookURL string
263 shouldBlock bool
264- description string
265 }{
266- {
267- name: "block localhost",
268- webhookURL: "http://localhost:8080/webhook",
269- shouldBlock: true,
270- description: "should block localhost addresses",
271- },
272- {
273- name: "block 127.0.0.1",
274- webhookURL: "http://127.0.0.1:8080/webhook",
275- shouldBlock: true,
276- description: "should block loopback addresses",
277- },
278- {
279- name: "block 169.254.169.254",
280- webhookURL: "http://169.254.169.254/latest/meta-data/",
281- shouldBlock: true,
282- description: "should block cloud metadata service",
283- },
284- {
285- name: "block private network",
286- webhookURL: "http://192.168.1.1/webhook",
287- shouldBlock: true,
288- description: "should block private networks",
289- },
290- {
291- name: "allow public IP",
292- webhookURL: "http://8.8.8.8/webhook",
293- shouldBlock: false,
294- description: "should allow public IP addresses",
295- },
296+ {"block loopback", "http://127.0.0.1:8080/webhook", true},
297+ {"block metadata", "http://169.254.169.254/latest/meta-data/", true},
298+ {"allow public IP", "http://8.8.8.8/webhook", false},
299 }
300
301 for _, tt := range tests {
302 t.Run(tt.name, func(t *testing.T) {
303- // Create a test webhook
304- webhook := models.Webhook{
305+ w := models.Webhook{
306 URL: tt.webhookURL,
307 ContentType: int(ContentTypeJSON),
308- Secret: "",
309 }
310
311- // Try to send a webhook
312 ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
313 defer cancel()
314
315- // Create a simple payload
316- payload := map[string]string{"test": "data"}
317-
318- err := sendWebhookWithContext(ctx, webhook, EventPush, payload)
319-
320- if tt.shouldBlock {
321- if err == nil {
322- t.Errorf("%s: expected error but got none", tt.description)
323- }
324- } else {
325- // For public IPs, we expect a connection error (since 8.8.8.8 won't be listening)
326- // but NOT an SSRF blocking error
327- if err != nil && isSSRFError(err) {
328- t.Errorf("%s: should not block public IPs, got: %v", tt.description, err)
329- }
330+ req, err := http.NewRequestWithContext(ctx, "POST", w.URL, nil)
331+ if err != nil {
332+ t.Fatalf("failed to create request: %v", err)
333 }
334- })
335- }
336-}
337-
338-// TestSecureHTTPClientBlocksRedirects tests that redirects are not followed.
339-func TestSecureHTTPClientBlocksRedirects(t *testing.T) {
340- // Create a test server on a public-looking address that redirects
341- redirectServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
342diff --git a/pkg/webhook/validator.go b/pkg/webhook/validator.go
343index 0eecd0951173b7cb518b57a0c0826383f18e3e82..2cc13a7f15445fc56c4e576f897ceb381e3f8eba 100644
344--- a/pkg/webhook/validator.go
345+++ b/pkg/webhook/validator.go
346@@ -1,169 +1,20 @@
347 package webhook
348
349 import (
350- "context"
351- "errors"
352- "fmt"
353- "net"
354- "net/url"
355- "slices"
356- "strings"
357+ "github.com/charmbracelet/soft-serve/pkg/ssrf"
358 )
359
360+// Error aliases for backward compatibility.
361 var (
362- // ErrInvalidScheme is returned when the webhook URL scheme is not http or https.
363- ErrInvalidScheme = errors.New("webhook URL must use http or https scheme")
364- // ErrPrivateIP is returned when the webhook URL resolves to a private IP address.
365- ErrPrivateIP = errors.New("webhook URL cannot resolve to private or internal IP addresses")
366- // ErrInvalidURL is returned when the webhook URL is invalid.
367- ErrInvalidURL = errors.New("invalid webhook URL")
368+ ErrInvalidScheme = ssrf.ErrInvalidScheme
369+ ErrPrivateIP = ssrf.ErrPrivateIP
370+ ErrInvalidURL = ssrf.ErrInvalidURL
371 )
372
373 // ValidateWebhookURL validates that a webhook URL is safe to use.
374-// It checks:
375-// - URL is properly formatted
376-// - Scheme is http or https
377-// - Hostname does not resolve to private/internal IP addresses
378-// - Hostname is not localhost or similar.
379 func ValidateWebhookURL(rawURL string) error {
380- if rawURL == "" {
381- return ErrInvalidURL
382- }
383-
384- // Parse the URL
385- u, err := url.Parse(rawURL)
386- if err != nil {
387- return fmt.Errorf("%w: %v", ErrInvalidURL, err)
388- }
389-
390- // Check scheme
391- if u.Scheme != "http" && u.Scheme != "https" {
392- return ErrInvalidScheme
393- }
394-
395- // Extract hostname (without port)
396- hostname := u.Hostname()
397- if hostname == "" {
398- return fmt.Errorf("%w: missing hostname", ErrInvalidURL)
399- }
400-
401- // Check for localhost variations
402- if isLocalhost(hostname) {
403- return ErrPrivateIP
404- }
405-
406- // If it's an IP address, validate it directly
407- if ip := net.ParseIP(hostname); ip != nil {
408- if isPrivateOrInternalIP(ip) {
409- return ErrPrivateIP
410- }
411- return nil
412- }
413-
414- // Resolve hostname to IP addresses
415- ips, err := net.DefaultResolver.LookupIPAddr(context.Background(), hostname)
416- if err != nil {
417- return fmt.Errorf("%w: cannot resolve hostname: %v", ErrInvalidURL, err)
418- }
419-
420- // Check all resolved IPs
421- if slices.ContainsFunc(ips, isPrivateOrInternalIPAddr) {
422- return ErrPrivateIP
423- }
424-
425- return nil
426-}
427-
428-// isLocalhost checks if the hostname is localhost or similar.
429-func isLocalhost(hostname string) bool {
430- hostname = strings.ToLower(hostname)
431- return hostname == "localhost" ||
432- hostname == "localhost.localdomain" ||
433- strings.HasSuffix(hostname, ".localhost")
434-}
435-
436-// isPrivateOrInternalIPAddr is a helper function that users net.IPAddr instead of net.IP.
437-func isPrivateOrInternalIPAddr(ipAddr net.IPAddr) bool {
438- return isPrivateOrInternalIP(ipAddr.IP)
439-}
440-
441-// isPrivateOrInternalIP checks if an IP address is private, internal, or reserved.
442-func isPrivateOrInternalIP(ip net.IP) bool {
443- // Loopback addresses (127.0.0.0/8, ::1)
444- if ip.IsLoopback() {
445- return true
446diff --git a/pkg/webhook/validator_test.go b/pkg/webhook/validator_test.go
447index 9d4d4f2ac542dbe3d433e16e18c90d78e6236708..901d44a389bd08c2a36378976c139aa4855faca4 100644
448--- a/pkg/webhook/validator_test.go
449+++ b/pkg/webhook/validator_test.go
450@@ -1,315 +1,52 @@
451 package webhook
452
453 import (
454- "net"
455+ "errors"
456 "testing"
457+
458+ "github.com/charmbracelet/soft-serve/pkg/ssrf"
459 )
460
461+// TestValidateWebhookURL verifies the wrapper delegates correctly and
462+// error aliases work across the package boundary. IP range coverage
463+// is in pkg/ssrf/ssrf_test.go -- here we just confirm the plumbing.
464 func TestValidateWebhookURL(t *testing.T) {
465 tests := []struct {
466 name string
467 url string
468 wantErr bool
469 errType error
470- skip string
471 }{
472- // Valid URLs (these will perform DNS lookups, so may fail in some environments)
473- {
474- name: "valid https URL",
475- url: "https://1.1.1.1/webhook",
476- wantErr: false,
477- },
478- {
479- name: "valid http URL",
480- url: "http://8.8.8.8/webhook",
481- wantErr: false,
482- },
483- {
484- name: "valid URL with port",
485- url: "https://1.1.1.1:8080/webhook",
486- wantErr: false,
487- },
488- {
489- name: "valid URL with path and query",
490- url: "https://8.8.8.8/webhook?token=abc123",
491- wantErr: false,
492- },
493-
494- // Invalid schemes
495- {
496- name: "ftp scheme",
497- url: "ftp://example.com/webhook",
498- wantErr: true,
499- errType: ErrInvalidScheme,
500- },
501- {
502- name: "file scheme",
503- url: "file:///etc/passwd",
504- wantErr: true,
505- errType: ErrInvalidScheme,
506- },
507- {
508- name: "gopher scheme",
509- url: "gopher://example.com",
510- wantErr: true,
511- errType: ErrInvalidScheme,
512- },
513- {
514- name: "no scheme",
515- url: "example.com/webhook",
516- wantErr: true,
517- errType: ErrInvalidScheme,
518- },
519-
520- // Localhost variations
521- {
522- name: "localhost",
523- url: "http://localhost/webhook",
524- wantErr: true,
525- errType: ErrPrivateIP,
526- },
527- {
528- name: "localhost with port",
529- url: "http://localhost:8080/webhook",
530- wantErr: true,
531- errType: ErrPrivateIP,
532- },
533- {
534- name: "localhost.localdomain",
535- url: "http://localhost.localdomain/webhook",
536- wantErr: true,
537- errType: ErrPrivateIP,
538- },
539-
540- // Loopback IPs
541- {
542- name: "127.0.0.1",
543- url: "http://127.0.0.1/webhook",
544- wantErr: true,
545- errType: ErrPrivateIP,
546- },
547- {
548- name: "127.0.0.1 with port",
549- url: "http://127.0.0.1:8080/webhook",
550diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go
551index 176d8e25b56f7146ba39a9912493f0d94ca83d9c..dc3fe7dcb65d493c69932bf01e0fbce648b32a07 100644
552--- a/pkg/webhook/webhook.go
553+++ b/pkg/webhook/webhook.go
554@@ -10,14 +10,13 @@ import (
555 "errors"
556 "fmt"
557 "io"
558- "net"
559 "net/http"
560- "time"
561
562 "github.com/charmbracelet/soft-serve/git"
563 "github.com/charmbracelet/soft-serve/pkg/db"
564 "github.com/charmbracelet/soft-serve/pkg/db/models"
565 "github.com/charmbracelet/soft-serve/pkg/proto"
566+ "github.com/charmbracelet/soft-serve/pkg/ssrf"
567 "github.com/charmbracelet/soft-serve/pkg/store"
568 "github.com/charmbracelet/soft-serve/pkg/utils"
569 "github.com/charmbracelet/soft-serve/pkg/version"
570@@ -38,42 +37,8 @@ type Delivery struct {
571 Event Event
572 }
573
574-// secureHTTPClient creates an HTTP client with SSRF protection.
575-var secureHTTPClient = &http.Client{
576- Timeout: 30 * time.Second,
577- Transport: &http.Transport{
578- DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) {
579- // Parse the address to get the IP
580- host, _, err := net.SplitHostPort(addr)
581- if err != nil {
582- return nil, err //nolint:wrapcheck
583- }
584-
585- // Validate the resolved IP before connecting
586- ip := net.ParseIP(host)
587- if ip != nil {
588- if err := ValidateIPBeforeDial(ip); err != nil {
589- return nil, fmt.Errorf("blocked connection to private IP: %w", err)
590- }
591- }
592-
593- // Use standard dialer with timeout
594- dialer := &net.Dialer{
595- Timeout: 10 * time.Second,
596- KeepAlive: 30 * time.Second,
597- }
598- return dialer.DialContext(ctx, network, addr)
599- },
600- MaxIdleConns: 100,
601- IdleConnTimeout: 90 * time.Second,
602- TLSHandshakeTimeout: 10 * time.Second,
603- ExpectContinueTimeout: 1 * time.Second,
604- },
605- // Don't follow redirects to prevent bypassing IP validation
606- CheckRedirect: func(*http.Request, []*http.Request) error {
607- return http.ErrUseLastResponse
608- },
609-}
610+// secureHTTPClient is an HTTP client with SSRF protection.
611+var secureHTTPClient = ssrf.NewSecureClient()
612
613 // do sends a webhook.
614 // Caller must close the returned body.