fix(outbound): strip URL userinfo before allow and deny list matching

pkg/gotenberg/outbound.go

@@ -249,7 +249,9 @@ func httpLikeScheme(scheme string) bool {
 //
 // The semantics:
 //
-//  1. The URL is parsed and its scheme and host lowercased.
+//  1. The URL is parsed, its scheme and host lowercased, and any userinfo
+//     dropped from the form the regexes see. The request still carries the
+//     credentials.
 //  2. allowList and denyList apply against the normalized form with OR
 //     semantics. The deny-list always applies.
 //  3. For http, https, ws, and wss, the host is resolved and every
@@ -274,7 +276,19 @@ func DecideOutbound(ctx context.Context, rawURL string, allowList, denyList []*r
 	}
 	parsed.Scheme = strings.ToLower(parsed.Scheme)
 	parsed.Host = strings.ToLower(parsed.Host)
-	normalized := parsed.String()
+
+	// Match on a credential-free form. [url.URL.String] re-emits userinfo
+	// between "scheme://" and the host, so keeping it would let any
+	// "^https?://<host>" pattern be shifted past its own anchor:
+	// http://a@127.0.0.1/ escapes a deny-list anchored on 127\. and
+	// http://trusted.example.com@10.0.0.1/ satisfies an allow-list anchored on
+	// trusted\.example\.com. The host checks below already read
+	// [url.URL.Hostname], which ignores userinfo, so only the regex layer was
+	// affected. Dropping the credentials here also keeps them out of the error
+	// strings below, which reach operator logs and any OTEL log exporter.
+	matchable := *parsed
+	matchable.User = nil
+	normalized := matchable.String()
 
 	allowMatched := false
 	if len(allowList) > 0 {

pkg/gotenberg/outbound_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"net/netip"
+	"strings"
 	"testing"
 	"time"
 
@@ -520,3 +521,122 @@ func TestDecideOutbound_Permissive_AllowsPrivate(t *testing.T) {
 		t.Fatalf("decision.Pinned = %v, want [10.0.0.5]", decision.Pinned)
 	}
 }
+
+// privateIPsDenyList is the textual private-IP deny-list that shipped as the
+// default for api-download-from-deny-list and webhook-deny-list in v8.31.0 and
+// is still published as a migration recipe. Every alternative is anchored on
+// "://", so userinfo used to slide the private address past the anchor.
+const privateIPsDenyList = `^https?://(10\.|172\.(1[6-9]|2[0-9]|3[01])\.|192\.168\.|169\.254\.|0\.0\.0\.0|127\.|localhost|\[::1\]|\[fd)`
+
+func TestDecideOutbound_UserinfoDoesNotEvadeDenyList(t *testing.T) {
+	for _, rawURL := range []string{
+		"http://127.0.0.1:9999/",
+		"http://a@127.0.0.1:9999/",
+		"http://@127.0.0.1:9999/",
+		"http://:@127.0.0.1:9999/",
+		"http://%61@127.0.0.1:9999/",
+		"http://user:pass@127.0.0.1:9999/",
+		"HTTP://A@127.0.0.1:9999/",
+		"http://a@169.254.169.254/latest/meta-data/",
+		// url.Parse takes the last "@" as the userinfo separator, so the host
+		// here is the second literal.
+		"http://a@127.0.0.1:9999@127.0.0.1:9999/",
+	} {
+		t.Run(rawURL, func(t *testing.T) {
+			withStubResolver(t, func(host string) ([]netip.Addr, error) {
+				t.Fatalf("unexpected DNS lookup for %q: the deny-list must reject before resolution", host)
+				return nil, nil
+			})
+
+			// Deny-list only, with the permissive IP defaults the modules ship.
+			_, err := DecideOutbound(
+				context.Background(),
+				rawURL,
+				nil,
+				[]*regexp2.Regexp{regexp2.MustCompile(privateIPsDenyList, 0)},
+				time.Now().Add(5*time.Second),
+			)
+			if !errors.Is(err, ErrFiltered) {
+				t.Fatalf("userinfo must not evade the deny-list, got: %v", err)
+			}
+		})
+	}
+}
+
+func TestDecideOutbound_UserinfoDoesNotSatisfyAllowList(t *testing.T) {
+	// A host-terminated allow-list, the shape the documentation recommends.
+	allowList := []*regexp2.Regexp{regexp2.MustCompile(`^https://trusted\.example\.com(:[0-9]+)?(/|$)`, 0)}
+
+	for _, rawURL := range []string{
+		"https://trusted.example.com@169.254.169.254/latest/meta-data/",
+		"https://trusted.example.com@10.0.0.5/",
+		"https://trusted.example.com:443@10.0.0.5/",
+	} {
+		t.Run(rawURL, func(t *testing.T) {
+			withStubResolver(t, func(host string) ([]netip.Addr, error) {
+				return mustAddrs(t, "10.0.0.5"), nil
+			})
+
+			decision, err := DecideOutbound(
+				context.Background(),
+				rawURL,
+				allowList, nil,
+				time.Now().Add(5*time.Second),
+				WithDenyPrivateIPs(true),
+			)
+			if err == nil {
+				t.Fatalf("userinfo must not satisfy the allow-list, got decision %+v", decision)
+			}
+			if decision.Bypass {
+				t.Fatal("userinfo must never produce a bypass")
+			}
+		})
+	}
+}
+
+func TestDecideOutbound_UserinfoKeptOutOfErrorMessages(t *testing.T) {
+	withStubResolver(t, func(host string) ([]netip.Addr, error) {
+		t.Fatalf("unexpected DNS lookup for %q", host)
+		return nil, nil
+	})
+
+	_, err := DecideOutbound(
+		context.Background(),
+		"http://alice:hunter2@127.0.0.1:9999/",
+		nil,
+		[]*regexp2.Regexp{regexp2.MustCompile(privateIPsDenyList, 0)},
+		time.Now().Add(5*time.Second),
+	)
+	if err == nil {
+		t.Fatal("expected the URL to be filtered")
+	}
+	if strings.Contains(err.Error(), "hunter2") || strings.Contains(err.Error(), "alice") {
+		t.Fatalf("error message must not leak URL credentials: %v", err)
+	}
+}
+
+func TestDecideOutbound_LegitimateCredentialsStillReachTheHost(t *testing.T) {
+	withStubResolver(t, func(host string) ([]netip.Addr, error) {
+		if host != "example.com" {
+			t.Fatalf("host = %q, want example.com: userinfo must not reach resolution", host)
+		}
+		return mustAddrs(t, "93.184.216.34"), nil
+	})
+
+	// Stripping userinfo is a matching concern only. A credentialed URL that
+	// breaks no rule must still be allowed through.
+	decision, err := DecideOutbound(
+		context.Background(),
+		"https://alice:hunter2@example.com/report.pdf",
+		[]*regexp2.Regexp{regexp2.MustCompile(`^https://example\.com(:[0-9]+)?(/|$)`, 0)},
+		nil,
+		time.Now().Add(5*time.Second),
+		WithDenyPrivateIPs(true),
+	)
+	if err != nil {
+		t.Fatalf("credentialed URL matching the allow-list must pass, got: %v", err)
+	}
+	if !decision.Bypass {
+		t.Fatalf("decision.Bypass = false, want true")
+	}
+}