From 7929fd6ee94cd975ccc45046936130ff4983366e Mon Sep 17 00:00:00 2001
From: Rod Hynes <rod-hynes@users.noreply.github.com>
Date: Fri, 14 Jun 2024 08:37:56 -0400
Subject: [PATCH] Bug fixes

- Always use concurrency-safe `constraints` in serverEntriesReporter.

- Don't try to parse empty SteeringIP in handshake response; log when an
  unexpected steering IP is received; change info notices to warnings.

- Add TargetAPIEncoding workaround to new clientlib test cases.
---
 ClientLibrary/clientlib/clientlib_test.go | 30 ++++++++++++++
 psiphon/controller.go                     |  2 +-
 psiphon/serverApi.go                      | 48 +++++++++++++----------
 3 files changed, 58 insertions(+), 22 deletions(-)

diff --git a/ClientLibrary/clientlib/clientlib_test.go b/ClientLibrary/clientlib/clientlib_test.go
index 90b9d58a8..375e54888 100644
--- a/ClientLibrary/clientlib/clientlib_test.go
+++ b/ClientLibrary/clientlib/clientlib_test.go
@@ -272,6 +272,21 @@ func TestMultipleStartTunnel(t *testing.T) {
 		t.Skipf("error loading configuration file: %s", err)
 	}
 
+	var config map[string]interface{}
+	err = json.Unmarshal(configJSON, &config)
+	if err != nil {
+		t.Fatalf("json.Unmarshal failed: %v", err)
+	}
+
+	// Use the legacy encoding to both exercise that case, and facilitate a
+	// gradual network upgrade to new encoding support.
+	config["TargetAPIEncoding"] = protocol.PSIPHON_API_ENCODING_JSON
+
+	configJSON, err = json.Marshal(config)
+	if err != nil {
+		t.Fatalf("json.Marshal failed: %v", err)
+	}
+
 	testDataDirName, err := os.MkdirTemp("", "psiphon-clientlib-test")
 	if err != nil {
 		t.Fatalf("ioutil.TempDir failed: %v", err)
@@ -331,6 +346,21 @@ func TestPsiphonTunnel_Dial(t *testing.T) {
 		t.Skipf("error loading configuration file: %s", err)
 	}
 
+	var config map[string]interface{}
+	err = json.Unmarshal(configJSON, &config)
+	if err != nil {
+		t.Fatalf("json.Unmarshal failed: %v", err)
+	}
+
+	// Use the legacy encoding to both exercise that case, and facilitate a
+	// gradual network upgrade to new encoding support.
+	config["TargetAPIEncoding"] = protocol.PSIPHON_API_ENCODING_JSON
+
+	configJSON, err = json.Marshal(config)
+	if err != nil {
+		t.Fatalf("json.Marshal failed: %v", err)
+	}
+
 	testDataDirName, err := os.MkdirTemp("", "psiphon-clientlib-test")
 	if err != nil {
 		t.Fatalf("ioutil.TempDir failed: %v", err)
diff --git a/psiphon/controller.go b/psiphon/controller.go
index fce16a5bb..9a07ecd8e 100755
--- a/psiphon/controller.go
+++ b/psiphon/controller.go
@@ -786,7 +786,7 @@ loop:
 
 			NoticeCandidateServers(
 				controller.config.EgressRegion,
-				controller.protocolSelectionConstraints,
+				constraints,
 				response.initialCandidates,
 				response.candidates,
 				duration)
diff --git a/psiphon/serverApi.go b/psiphon/serverApi.go
index 00754e898..df4b88ae9 100644
--- a/psiphon/serverApi.go
+++ b/psiphon/serverApi.go
@@ -409,7 +409,7 @@ func (serverContext *ServerContext) doHandshakeRequest(ignoreStatsRegexps bool)
 				err := serverContext.tunnel.config.SetParameters(
 					tacticsRecord.Tag, true, tacticsRecord.Tactics.Parameters)
 				if err != nil {
-					NoticeInfo("apply handshake tactics failed: %s", err)
+					NoticeWarning("apply handshake tactics failed: %s", err)
 				}
 				// The error will be due to invalid tactics values
 				// from the server. When SetParameters fails, all
@@ -418,28 +418,34 @@ func (serverContext *ServerContext) doHandshakeRequest(ignoreStatsRegexps bool)
 		}
 	}
 
-	if serverContext.tunnel.dialParams.steeringIPCacheKey != "" {
+	if handshakeResponse.SteeringIP != "" {
+
+		if serverContext.tunnel.dialParams.steeringIPCacheKey == "" {
+			NoticeWarning("unexpected steering IP")
 
-		// Cache any received steering IP, which will also extend the TTL for
-		// an existing entry.
-		//
-		// As typical tunnel duration is short and dialing can be challenging,
-		// this established tunnel is retained and the steering IP will be
-		// used on any subsequent dial to the same fronting provider,
-		// assuming the TTL has not expired.
-		//
-		// Note: to avoid TTL expiry for long-lived tunnels, the TTL could be
-		// set or extended at the end of the tunnel lifetime; however that
-		// may result in unintended steering.
-
-		IP := net.ParseIP(handshakeResponse.SteeringIP)
-		if IP != nil && !common.IsBogon(IP) {
-			serverContext.tunnel.dialParams.steeringIPCache.Set(
-				serverContext.tunnel.dialParams.steeringIPCacheKey,
-				handshakeResponse.SteeringIP,
-				lrucache.DefaultExpiration)
 		} else {
-			NoticeInfo("ignoring invalid steering IP")
+
+			// Cache any received steering IP, which will also extend the TTL for
+			// an existing entry.
+			//
+			// As typical tunnel duration is short and dialing can be challenging,
+			// this established tunnel is retained and the steering IP will be
+			// used on any subsequent dial to the same fronting provider,
+			// assuming the TTL has not expired.
+			//
+			// Note: to avoid TTL expiry for long-lived tunnels, the TTL could be
+			// set or extended at the end of the tunnel lifetime; however that
+			// may result in unintended steering.
+
+			IP := net.ParseIP(handshakeResponse.SteeringIP)
+			if IP != nil && !common.IsBogon(IP) {
+				serverContext.tunnel.dialParams.steeringIPCache.Set(
+					serverContext.tunnel.dialParams.steeringIPCacheKey,
+					handshakeResponse.SteeringIP,
+					lrucache.DefaultExpiration)
+			} else {
+				NoticeWarning("ignoring invalid steering IP")
+			}
 		}
 	}