fix: Handle goproxy scheme handling bug (#181)

* fix: Handle goproxy scheme handling bug * fix: Code review fixes
2026-08-03 07:24:09 +02:00 · 2026-03-10 10:35:28 +05:30
parent f8fcdf6929
commit 2d1926388c
2 changed files with 163 additions and 0 deletions
@@ -6,6 +6,8 @@ import (
 	"fmt"
 	"net"
 	"net/http"
+	"net/url"
+	"strings"
 	"sync"
 	"time"

@@ -139,6 +141,9 @@ func newUpstreamTransport(config *ProxyConfig) *http.Transport {

 	// Keep transport behavior close to goproxy defaults and only harden TLS:
 	// enforce server certificate verification and require TLS 1.2+.
+	// Proxy honours the environment (HTTP_PROXY, HTTPS_PROXY, NO_PROXY) so
+	// that PMG works in enterprise environments that require a corporate
+	// upstream proxy to reach the internet.
 	return &http.Transport{
 		Proxy:               http.ProxyFromEnvironment,
 		DialContext:         dialer.DialContext,
@@ -219,6 +224,78 @@ func (ps *proxyServer) RemoveInterceptor(name string) {
 	log.Debugf("Removed interceptor: %s", name)
 }

+// normalizeRequestURL fixes malformed URLs produced by goproxy's MITM URL reconstruction.
+//
+// When a client (e.g., npm) sends absolute-form Request-URIs inside a CONNECT tunnel
+// (e.g., "POST http://registry.npmjs.org:443/-/npm/v1/security/advisories/bulk"),
+// goproxy's MITM code checks if req.URL starts with "scheme://" and, if not, naively
+// prepends "scheme://connectHost" to the full URI. Since the client's URI starts with
+// "http://" but the MITM scheme is "https", the check fails and goproxy produces:
+//
+//	https://registry.npmjs.org:443http://registry.npmjs.org:443/-/npm/v1/security/advisories/bulk
+//
+// This function detects the embedded absolute URI and extracts it, preserving the
+// correct scheme from the MITM connection.
+func normalizeRequestURL(req *http.Request) {
+	if req == nil || req.URL == nil {
+		return
+	}
+
+	host := req.URL.Host
+	if host == "" {
+		return
+	}
+
+	// The goproxy bug produces a Host field like "registry.npmjs.org:443http:"
+	// where the embedded scheme leaks into the authority. A valid host:port
+	// never contains "http:" or "https:", so this check is precise and avoids
+	// false positives from query parameters or path segments.
+	var embeddedScheme string
+	var schemeIdx int
+	if idx := strings.Index(host, "http:"); idx > 0 {
+		embeddedScheme = "http"
+		schemeIdx = idx
+	} else if idx := strings.Index(host, "https:"); idx > 0 {
+		embeddedScheme = "https"
+		schemeIdx = idx
+	}
+
+	if embeddedScheme == "" {
+		return
+	}
+
+	// Extract the real host (everything before the embedded scheme)
+	realHost := host[:schemeIdx]
+
+	// Reconstruct the embedded URL from the scheme found in the host
+	// plus the path portion that Go's URL parser placed after the authority.
+	// The full original URL looks like: scheme://realHost + embeddedScheme://embeddedHost/path
+	// Go parsed the authority as "realHost + embeddedScheme:" and the path as
+	// "//embeddedHost/path", so we combine them back.
+	embeddedURL := embeddedScheme + ":" + req.URL.Path
+	if req.URL.RawQuery != "" {
+		embeddedURL += "?" + req.URL.RawQuery
+	}
+	if req.URL.Fragment != "" {
+		embeddedURL += "#" + req.URL.Fragment
+	}
+
+	parsed, err := url.Parse(embeddedURL)
+	if err != nil {
+		return
+	}
+
+	// If the embedded URL parsed to a valid host, use it. Otherwise fall back
+	// to the real host we extracted from the authority.
+	if parsed.Host == "" {
+		parsed.Host = realHost
+	}
+
+	// Preserve the MITM scheme (e.g. https) rather than the client's scheme
+	parsed.Scheme = req.URL.Scheme
+	req.URL = parsed
+}
+
 func (ps *proxyServer) configureMITM() {
 	// Configure selective MITM based on interceptors
 	ps.proxy.OnRequest().HandleConnect(goproxy.FuncHttpsHandler(func(host string, ctx *goproxy.ProxyCtx) (*goproxy.ConnectAction, string) {
@@ -277,6 +354,13 @@ func (ps *proxyServer) configureMITM() {

 func (ps *proxyServer) registerHandlers() {
 	ps.proxy.OnRequest().DoFunc(func(req *http.Request, ctx *goproxy.ProxyCtx) (*http.Request, *http.Response) {
+		// Fix malformed URLs produced by goproxy's MITM URL reconstruction.
+		// When a client sends absolute-form Request-URIs (e.g., http://host:port/path)
+		// inside a CONNECT tunnel, goproxy naively prepends scheme://connectHost to the
+		// full URI, producing malformed URLs like https://host:443http://host:443/path.
+		// We detect and fix this before processing.
+		normalizeRequestURL(req)
+
 		reqCtx, err := newRequestContext(req)
 		if err != nil {
 			log.Errorf("Failed to create request context: %v", err)
@@ -4,6 +4,7 @@ import (
 	"crypto/tls"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"testing"
 	"time"

@@ -27,6 +28,84 @@ func TestNewProxyServerSecuresUpstreamTLSConfig(t *testing.T) {
 	assert.GreaterOrEqual(t, internalProxy.proxy.Tr.TLSClientConfig.MinVersion, uint16(tls.VersionTLS12), "minimum TLS version should be 1.2+")
 }

+func TestNormalizeRequestURL(t *testing.T) {
+	tests := []struct {
+		name        string
+		inputURL    string
+		expectedURL string
+	}{
+		{
+			name:        "malformed URL with http embedded in https",
+			inputURL:    "https://registry.npmjs.org:443http://registry.npmjs.org:443/-/npm/v1/security/advisories/bulk",
+			expectedURL: "https://registry.npmjs.org:443/-/npm/v1/security/advisories/bulk",
+		},
+		{
+			name:        "malformed URL with http embedded for github packages",
+			inputURL:    "https://npm.pkg.github.com:443http://npm.pkg.github.com:443/download/some_package/0.2.7-rc2/abc123",
+			expectedURL: "https://npm.pkg.github.com:443/download/some_package/0.2.7-rc2/abc123",
+		},
+		{
+			name:        "normal https URL is unchanged",
+			inputURL:    "https://registry.npmjs.org/-/npm/v1/security/advisories/bulk",
+			expectedURL: "https://registry.npmjs.org/-/npm/v1/security/advisories/bulk",
+		},
+		{
+			name:        "normal http URL is unchanged",
+			inputURL:    "http://registry.npmjs.org/-/npm/v1/security/advisories/bulk",
+			expectedURL: "http://registry.npmjs.org/-/npm/v1/security/advisories/bulk",
+		},
+		{
+			name:        "URL with scoped package encoding",
+			inputURL:    "https://npm.pkg.github.com:443http://npm.pkg.github.com:443/@scope%2fpackage",
+			expectedURL: "https://npm.pkg.github.com:443/@scope/package",
+		},
+		{
+			name:        "URL with http in query parameter is unchanged",
+			inputURL:    "https://example.com/api?redirect=http://foo.com",
+			expectedURL: "https://example.com/api?redirect=http://foo.com",
+		},
+		{
+			name:        "URL with https in query parameter is unchanged",
+			inputURL:    "https://example.com/api?url=https://bar.com/path",
+			expectedURL: "https://example.com/api?url=https://bar.com/path",
+		},
+		{
+			name:        "URL with http in path segment is unchanged",
+			inputURL:    "https://example.com/proxy/http://target.com/resource",
+			expectedURL: "https://example.com/proxy/http://target.com/resource",
+		},
+		{
+			name:        "URL with http in fragment is unchanged",
+			inputURL:    "https://example.com/docs#http://ref.com",
+			expectedURL: "https://example.com/docs#http://ref.com",
+		},
+		{
+			name:        "malformed URL with query string preserved",
+			inputURL:    "https://registry.npmjs.org:443http://registry.npmjs.org:443/-/npm/v1/security/advisories/bulk?foo=bar",
+			expectedURL: "https://registry.npmjs.org:443/-/npm/v1/security/advisories/bulk?foo=bar",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			parsed, err := url.Parse(tt.inputURL)
+			assert.NoError(t, err)
+
+			req := &http.Request{URL: parsed}
+			normalizeRequestURL(req)
+
+			assert.Equal(t, tt.expectedURL, req.URL.String())
+		})
+	}
+}
+
+func TestNormalizeRequestURLNilSafety(t *testing.T) {
+	// Should not panic
+	normalizeRequestURL(nil)
+	normalizeRequestURL(&http.Request{})
+	normalizeRequestURL(&http.Request{URL: &url.URL{}})
+}
+
 func TestNewProxyServerRejectsUntrustedUpstreamCertByDefault(t *testing.T) {
 	target := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
 		w.WriteHeader(http.StatusOK)