Exclude user input URLs from net/url parse errors

psiphon/common/utils.go

@@ -23,10 +23,12 @@ import (
 	"bytes"
 	"compress/zlib"
 	"crypto/rand"
+	std_errors "errors"
 	"fmt"
 	"io"
 	"io/ioutil"
 	"math"
+	"net/url"
 	"os"
 	"time"
 
@@ -237,3 +239,37 @@ func DoFileMigration(migration FileMigration) error {
 
 	return nil
 }
+
+// SafeParseURL wraps url.Parse, stripping the input URL from any error
+// message. This allows logging url.Parse errors without unintentially logging
+// PII that may appear in the input URL.
+func SafeParseURL(rawurl string) (*url.URL, error) {
+	parsedURL, err := url.Parse(rawurl)
+	if err != nil {
+		// Unwrap yields just the url.Error error field without the url.Error URL
+		// and operation fields.
+		err = std_errors.Unwrap(err)
+		if err == nil {
+			err = std_errors.New("SafeParseURL: Unwrap failed")
+		} else {
+			err = fmt.Errorf("url.Parse: %v", err)
+		}
+	}
+	return parsedURL, err
+}
+
+// SafeParseRequestURI wraps url.ParseRequestURI, stripping the input URL from
+// any error message. This allows logging url.ParseRequestURI errors without
+// unintentially logging PII that may appear in the input URL.
+func SafeParseRequestURI(rawurl string) (*url.URL, error) {
+	parsedURL, err := url.ParseRequestURI(rawurl)
+	if err != nil {
+		err = std_errors.Unwrap(err)
+		if err == nil {
+			err = std_errors.New("SafeParseRequestURI: Unwrap failed")
+		} else {
+			err = fmt.Errorf("url.ParseRequestURI: %v", err)
+		}
+	}
+	return parsedURL, err
+}

psiphon/common/utils_test.go

@@ -22,7 +22,9 @@ package common
 import (
 	"bytes"
 	"encoding/json"
+	"net/url"
 	"reflect"
+	"strings"
 	"testing"
 )
 
@@ -90,3 +92,53 @@ func TestFormatByteCount(t *testing.T) {
 		})
 	}
 }
+
+func TestSafeParseURL(t *testing.T) {
+
+	invalidURL := "https://invalid url"
+
+	_, err := url.Parse(invalidURL)
+
+	if err == nil {
+		t.Error("unexpected parse success")
+	}
+
+	if strings.Index(err.Error(), invalidURL) == -1 {
+		t.Error("URL not in error string")
+	}
+
+	_, err = SafeParseURL(invalidURL)
+
+	if err == nil {
+		t.Error("unexpected parse success")
+	}
+
+	if strings.Index(err.Error(), invalidURL) != -1 {
+		t.Error("URL in error string")
+	}
+}
+
+func TestSafeParseRequestURI(t *testing.T) {
+
+	invalidURL := "https://invalid url"
+
+	_, err := url.ParseRequestURI(invalidURL)
+
+	if err == nil {
+		t.Error("unexpected parse success")
+	}
+
+	if strings.Index(err.Error(), invalidURL) == -1 {
+		t.Error("URL not in error string")
+	}
+
+	_, err = SafeParseRequestURI(invalidURL)
+
+	if err == nil {
+		t.Error("unexpected parse success")
+	}
+
+	if strings.Index(err.Error(), invalidURL) != -1 {
+		t.Error("URL in error string")
+	}
+}

psiphon/dialParameters.go

@@ -26,7 +26,6 @@ import (
 	"fmt"
 	"net"
 	"net/http"
-	"net/url"
 	"strings"
 	"sync/atomic"
 	"time"
@@ -618,7 +617,7 @@ func MakeDialParameters(
 
 	if config.UseUpstreamProxy() {
 		// Note: UpstreamProxyURL will be validated in the dial
-		proxyURL, err := url.Parse(config.UpstreamProxyURL)
+		proxyURL, err := common.SafeParseURL(config.UpstreamProxyURL)
 		if err == nil {
 			dialParams.UpstreamProxyType = proxyURL.Scheme
 		}

psiphon/httpProxy.go

@@ -316,7 +316,7 @@ func (proxy *HttpProxy) urlProxyHandler(responseWriter http.ResponseWriter, requ
 	}
 
 	// Origin URL must be well-formed, absolute, and have a scheme of "http" or "https"
-	originURL, err := url.ParseRequestURI(originURLString)
+	originURL, err := common.SafeParseRequestURI(originURLString)
 	if err != nil {
 		NoticeWarning("%s", errors.Trace(FilterUrlError(err)))
 		forceClose(responseWriter)
@@ -678,7 +678,7 @@ func (proxy *HttpProxy) serve() {
 
 // toAbsoluteURL takes a base URL and a relative URL and constructs an appropriate absolute URL.
 func toAbsoluteURL(baseURL *url.URL, relativeURLString string) string {
-	relativeURL, err := url.Parse(relativeURLString)
+	relativeURL, err := common.SafeParseURL(relativeURLString)
 
 	if err != nil {
 		return ""

psiphon/meekConn.go

@@ -445,7 +445,7 @@ func DialMeek(
 			(meekConfig.DialAddress == meekConfig.HostHeader ||
 				meekConfig.DialAddress == meekConfig.HostHeader+":80") {
 
-			url, err := url.Parse(dialConfig.UpstreamProxyURL)
+			url, err := common.SafeParseURL(dialConfig.UpstreamProxyURL)
 			if err != nil {
 				return nil, errors.Trace(err)
 			}

psiphon/upstreamproxy/proxy_http.go

@@ -54,6 +54,7 @@ import (
 	"net/url"
 	"time"
 
+	"github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/common"
 	"golang.org/x/net/proxy"
 )
 
@@ -131,7 +132,7 @@ type proxyConn struct {
 func (pc *proxyConn) handshake(addr, username, password string) error {
 	// HACK: prefix addr of the form 'hostname:port' with a 'http' scheme
 	// so it could be parsed by url.Parse
-	reqURL, err := url.Parse("http://" + addr)
+	reqURL, err := common.SafeParseURL("http://" + addr)
 	if err != nil {
 		pc.httpClientConn.Close()
 		pc.authState = HTTP_AUTH_STATE_FAILURE

psiphon/upstreamproxy/upstreamproxy.go

@@ -23,8 +23,8 @@ import (
 	"fmt"
 	"net"
 	"net/http"
-	"net/url"
 
+	"github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/common"
 	"golang.org/x/net/proxy"
 )
 
@@ -58,17 +58,17 @@ func NewProxyDialFunc(config *UpstreamProxyConfig) DialFunc {
 	if config.ProxyURIString == "" {
 		return config.ForwardDialFunc
 	}
-	proxyURI, err := url.Parse(config.ProxyURIString)
+	proxyURI, err := common.SafeParseURL(config.ProxyURIString)
 	if err != nil {
 		return func(network, addr string) (net.Conn, error) {
-			return nil, proxyError(fmt.Errorf("proxyURI url.Parse: %v", err))
+			return nil, proxyError(fmt.Errorf("NewProxyDialFunc: SafeParseURL failed: %v", err))
 		}
 	}
 
 	dialer, err := proxy.FromURL(proxyURI, config)
 	if err != nil {
 		return func(network, addr string) (net.Conn, error) {
-			return nil, proxyError(fmt.Errorf("proxy.FromURL: %v", err))
+			return nil, proxyError(fmt.Errorf("NewProxyDialFunc: proxy.FromURL: %v", err))
 		}
 	}
 	return dialer.Dial