Fix: proxy answer error masked by filterSDPAddresses error

psiphon/common/inproxy/api.go

@@ -905,7 +905,8 @@ func (params *TrafficShapingParameters) Validate() error {
 }
 
 // ValidateAndGetLogFields validates the ProxyAnswerRequest and returns
-// common.LogFields for logging.
+// common.LogFields for logging. A nil filteredSDP is returned when
+// ProxyAnswerRequest.AnswerError is set.
 func (request *ProxyAnswerRequest) ValidateAndGetLogFields(
 	lookupGeoIP LookupGeoIP,
 	baseAPIParameterValidator common.APIParameterValidator,
@@ -913,27 +914,34 @@ func (request *ProxyAnswerRequest) ValidateAndGetLogFields(
 	geoIPData common.GeoIPData,
 	proxyAnnouncementHasPersonalCompartmentIDs bool) ([]byte, common.LogFields, error) {
 
-	// The proxy answer SDP must contain at least one ICE candidate.
-	errorOnNoCandidates := true
-
-	// The proxy answer SDP may include RFC 1918/4193 private IP addresses in
-	// personal pairing mode. filterSDPAddresses should not filter out
-	// private IP addresses based on the broker's local interfaces; this
-	// filtering occurs on the client that receives the SDP.
-	allowPrivateIPAddressCandidates := proxyAnnouncementHasPersonalCompartmentIDs
-	filterPrivateIPAddressCandidates := false
-
-	// Proxy answer SDP candidate addresses must match the country and ASN of
-	// the proxy. Don't facilitate connections to arbitrary destinations.
-	filteredSDP, sdpMetrics, err := filterSDPAddresses(
-		[]byte(request.ProxyAnswerSDP.SDP),
-		errorOnNoCandidates,
-		lookupGeoIP,
-		geoIPData,
-		allowPrivateIPAddressCandidates,
-		filterPrivateIPAddressCandidates)
-	if err != nil {
-		return nil, nil, errors.Trace(err)
+	var filteredSDP []byte
+	var sdpMetrics *webRTCSDPMetrics
+	var err error
+
+	if request.AnswerError == "" {
+
+		// The proxy answer SDP must contain at least one ICE candidate.
+		errorOnNoCandidates := true
+
+		// The proxy answer SDP may include RFC 1918/4193 private IP addresses in
+		// personal pairing mode. filterSDPAddresses should not filter out
+		// private IP addresses based on the broker's local interfaces; this
+		// filtering occurs on the client that receives the SDP.
+		allowPrivateIPAddressCandidates := proxyAnnouncementHasPersonalCompartmentIDs
+		filterPrivateIPAddressCandidates := false
+
+		// Proxy answer SDP candidate addresses must match the country and ASN of
+		// the proxy. Don't facilitate connections to arbitrary destinations.
+		filteredSDP, sdpMetrics, err = filterSDPAddresses(
+			[]byte(request.ProxyAnswerSDP.SDP),
+			errorOnNoCandidates,
+			lookupGeoIP,
+			geoIPData,
+			allowPrivateIPAddressCandidates,
+			filterPrivateIPAddressCandidates)
+		if err != nil {
+			return nil, nil, errors.Trace(err)
+		}
 	}
 
 	// The proxy's self-reported ICECandidateTypes are used instead of the
@@ -950,10 +958,12 @@ func (request *ProxyAnswerRequest) ValidateAndGetLogFields(
 
 	logFields["connection_id"] = request.ConnectionID
 	logFields["ice_candidate_types"] = request.ICECandidateTypes
-	logFields["has_IPv6"] = sdpMetrics.hasIPv6
-	logFields["has_private_IP"] = sdpMetrics.hasPrivateIP
-	logFields["filtered_ice_candidates"] = sdpMetrics.filteredICECandidates
 	logFields["answer_error"] = request.AnswerError
+	if sdpMetrics != nil {
+		logFields["has_IPv6"] = sdpMetrics.hasIPv6
+		logFields["has_private_IP"] = sdpMetrics.hasPrivateIP
+		logFields["filtered_ice_candidates"] = sdpMetrics.filteredICECandidates
+	}
 
 	return filteredSDP, logFields, nil
 }

psiphon/common/inproxy/broker.go

@@ -22,6 +22,7 @@ package inproxy
 import (
 	"context"
 	std_errors "errors"
+	"fmt"
 	"net"
 	"strconv"
 	"sync"
@@ -1399,8 +1400,10 @@ func (b *Broker) handleProxyAnswer(
 		if answerError != "" {
 			// This is a proxy-reported error that occurred while creating the answer.
 			logFields["answer_error"] = answerError
+			logFields["error"] = fmt.Sprintf("proxy answer error: %s", answerError)
 		}
 		if retErr != nil {
+			// For the error field, retErr takes precedence over answerError
 			logFields["error"] = retErr.Error()
 		}
 		logFields.Add(transportLogFields)
@@ -1454,9 +1457,6 @@ func (b *Broker) handleProxyAnswer(
 		return nil, errors.Trace(err)
 	}
 
-	answerSDP := answerRequest.ProxyAnswerSDP
-	answerSDP.SDP = string(filteredSDP)
-
 	if answerRequest.AnswerError != "" {
 
 		// The proxy failed to create an answer.
@@ -1472,6 +1472,9 @@ func (b *Broker) handleProxyAnswer(
 		// Note that neither ProxyID nor ProxyIP is returned to the client.
 		// These fields are used internally in the matcher.
 
+		answerSDP := answerRequest.ProxyAnswerSDP
+		answerSDP.SDP = string(filteredSDP)
+
 		proxyAnswer = &MatchAnswer{
 			ProxyIP:        proxyIP,
 			ProxyID:        initiatorID,