Browse Source

fix(database): blank boolean-era verifyPeerCertByName values on hosts

A host's verify_peer_cert_by_name must only hold a server name an
operator typed. The column started life as a boolean, and the migration
that was meant to discard those values runs after AutoMigrate, which has
already cast the column to text: SQLite's table rebuild stores 0/1 as
"0"/"1", and Postgres' USING col::text stores "false"/"true". The
migration then sees a text column and leaves the values alone.

Since 3.10.0 the JSON subscription lifts host TLS verification fields to
the level xray reads, so that leftover "0" is now sent as the cert name to
verify and clients such as Happ refuse the server certificate.

A one-shot HostVerifyPeerCertByNameBoolFix seeder blanks exactly the four
cast values on existing databases; real names are untouched. The
Postgres variant joins the CI schema job, since that cast only happens
there.
MHSanaei 13 hours ago
parent
commit
74c3cc3882
3 changed files with 127 additions and 4 deletions
  1. 3 3
      .github/workflows/ci.yml
  2. 20 1
      internal/database/db.go
  3. 104 0
      internal/database/host_migration_test.go

+ 3 - 3
.github/workflows/ci.yml

@@ -83,12 +83,12 @@ jobs:
       - name: PostgreSQL schema and migration tests
         run: |
           set -o pipefail
-          go test ./internal/database -run '^(TestHostAutoMigrateCreatesColumns_Postgres|TestMigrate_Postgres|TestClientWeeklyRenewMigration_Postgres)$' -count=1 -v | tee /tmp/postgres-schema.log
+          go test ./internal/database -run '^(TestHostAutoMigrateCreatesColumns_Postgres|TestMigrate_Postgres|TestClientWeeklyRenewMigration_Postgres|TestMigrate_LegacyBooleanVerifyPeerCertByNameIsBlanked_Postgres)$' -count=1 -v | tee /tmp/postgres-schema.log
           # All must pass. Counting, not SKIP-matching: renaming a test would
           # otherwise leave this step green while testing nothing.
           passed=$(grep -c -- '^--- PASS' /tmp/postgres-schema.log || true)
-          if [ "$passed" -lt 3 ]; then
-            echo "expected at least 3 passing PostgreSQL schema tests, got $passed" >&2
+          if [ "$passed" -lt 4 ]; then
+            echo "expected at least 4 passing PostgreSQL schema tests, got $passed" >&2
             exit 1
           fi
 

+ 20 - 1
internal/database/db.go

@@ -431,6 +431,19 @@ func migrateHostVerifyPeerCertByNameColumn() error {
 	return db.Exec(`UPDATE hosts SET verify_peer_cert_by_name = '' WHERE verify_peer_cert_by_name IS NULL OR typeof(verify_peer_cert_by_name) <> 'text'`).Error
 }
 
+// AutoMigrate casts the boolean-era column to text before the migration above runs,
+// so a saved false/true reads back as the cert name "0"/"false"/"1"/"true".
+func blankLegacyBoolVerifyPeerCertByName() error {
+	return db.Transaction(func(tx *gorm.DB) error {
+		if err := tx.Model(&model.Host{}).
+			Where("verify_peer_cert_by_name IN ?", []string{"0", "1", "false", "true"}).
+			Update("verify_peer_cert_by_name", "").Error; err != nil {
+			return err
+		}
+		return tx.Create(&model.HistoryOfSeeders{SeederName: "HostVerifyPeerCertByNameBoolFix"}).Error
+	})
+}
+
 func seedHostsFromExternalProxy() error {
 	var history []string
 	if err := db.Model(&model.HistoryOfSeeders{}).Pluck("seeder_name", &history).Error; err != nil {
@@ -1349,7 +1362,7 @@ func runSeeders(isUsersEmpty bool) error {
 	}
 
 	if empty && isUsersEmpty {
-		seeders := []string{"UserPasswordHash", "ClientsTable", "InboundClientsArrayFix", "InboundClientTgIdFix2", "InboundClientSubIdFix", "FreedomFinalRulesReverseFix", "FreedomFinalRulesPrivateEgressBlock", "UppercaseFreedomFinalRulesFix", "InboundRealityFinalmaskTcpStrip", "ApiTokensHash", "LegacyProxySettingsCleanup", "OutboundRemovedKeysFix", "FreedomDomainStrategyFix", "DNSOutboundLegacyKeysFix", "DNSOutboundQTypeZeroFix", "WireguardDomainStrategyFix", "XdnsFinalmaskNamesAddrsFix", "UDPHopClientMaskFix", "WireguardPeersToClients", "MtprotoSecretsToClients", "NodeInboundsAdopted", "ResetIpLimitNoFail2ban"}
+		seeders := []string{"UserPasswordHash", "ClientsTable", "InboundClientsArrayFix", "InboundClientTgIdFix2", "InboundClientSubIdFix", "FreedomFinalRulesReverseFix", "FreedomFinalRulesPrivateEgressBlock", "UppercaseFreedomFinalRulesFix", "InboundRealityFinalmaskTcpStrip", "ApiTokensHash", "LegacyProxySettingsCleanup", "OutboundRemovedKeysFix", "FreedomDomainStrategyFix", "DNSOutboundLegacyKeysFix", "DNSOutboundQTypeZeroFix", "WireguardDomainStrategyFix", "XdnsFinalmaskNamesAddrsFix", "UDPHopClientMaskFix", "HostVerifyPeerCertByNameBoolFix", "WireguardPeersToClients", "MtprotoSecretsToClients", "NodeInboundsAdopted", "ResetIpLimitNoFail2ban"}
 		for _, name := range seeders {
 			if err := db.Create(&model.HistoryOfSeeders{SeederName: name}).Error; err != nil {
 				return err
@@ -1502,6 +1515,12 @@ func runSeeders(isUsersEmpty bool) error {
 		}
 	}
 
+	if !slices.Contains(seedersHistory, "HostVerifyPeerCertByNameBoolFix") {
+		if err := blankLegacyBoolVerifyPeerCertByName(); err != nil {
+			return err
+		}
+	}
+
 	if !slices.Contains(seedersHistory, "NodeInboundsAdopted") {
 		if err := seedNodeInboundsAdopted(); err != nil {
 			return err

+ 104 - 0
internal/database/host_migration_test.go

@@ -110,6 +110,110 @@ func TestBackfillEmptyHostGroupIds_RepairsLegacyRows(t *testing.T) {
 	}
 }
 
+// verify_peer_cert_by_name began as a boolean; AutoMigrate's cast to text turned it
+// into the cert name "0"/"1", which xray clients then fail to verify against.
+func TestMigrate_LegacyBooleanVerifyPeerCertByNameIsBlanked(t *testing.T) {
+	dbPath := filepath.Join(t.TempDir(), "x-ui.db")
+	if err := InitDB(dbPath); err != nil {
+		t.Fatalf("InitDB: %v", err)
+	}
+	t.Cleanup(func() { _ = CloseDB() })
+	ib := seedInboundWithStream(t, "vcn-legacy", 5557, `{"network":"tcp","security":"tls"}`)
+	for _, stmt := range []string{
+		"ALTER TABLE hosts DROP COLUMN verify_peer_cert_by_name",
+		"ALTER TABLE hosts ADD COLUMN verify_peer_cert_by_name numeric",
+	} {
+		if err := GetDB().Exec(stmt).Error; err != nil {
+			t.Fatalf("%s: %v", stmt, err)
+		}
+	}
+	legacy := map[string]any{"bool-false": false, "bool-true": true, "named": "cert.example.com"}
+	for remark, vcn := range legacy {
+		if err := GetDB().Exec(
+			"INSERT INTO hosts (group_id, inbound_id, remark, address, security, verify_peer_cert_by_name) VALUES (?, ?, ?, 'cdn.example.com', 'tls', ?)",
+			remark, ib.Id, remark, vcn,
+		).Error; err != nil {
+			t.Fatalf("insert legacy host %s: %v", remark, err)
+		}
+	}
+	if err := GetDB().Where("seeder_name = ?", "HostVerifyPeerCertByNameBoolFix").Delete(&model.HistoryOfSeeders{}).Error; err != nil {
+		t.Fatalf("clear seeder history: %v", err)
+	}
+	if err := CloseDB(); err != nil {
+		t.Fatalf("CloseDB: %v", err)
+	}
+
+	if err := InitDB(dbPath); err != nil {
+		t.Fatalf("reopen InitDB: %v", err)
+	}
+
+	var hosts []model.Host
+	if err := GetDB().Where("inbound_id = ?", ib.Id).Find(&hosts).Error; err != nil {
+		t.Fatalf("load hosts: %v", err)
+	}
+	if len(hosts) != len(legacy) {
+		t.Fatalf("hosts = %d, want %d", len(hosts), len(legacy))
+	}
+	for _, h := range hosts {
+		want := ""
+		if h.Remark == "named" {
+			want = "cert.example.com"
+		}
+		if h.VerifyPeerCertByName != want {
+			t.Errorf("host %s: verifyPeerCertByName = %q, want %q", h.Remark, h.VerifyPeerCertByName, want)
+		}
+	}
+}
+
+// Postgres casts the boolean column with USING col::text whenever the hosts table
+// also lacks a newer column, the shape every boolean-era table had.
+func TestMigrate_LegacyBooleanVerifyPeerCertByNameIsBlanked_Postgres(t *testing.T) {
+	if strings.TrimSpace(os.Getenv("XUI_DB_DSN")) == "" || os.Getenv("XUI_DB_TYPE") != "postgres" {
+		t.Skip("set XUI_DB_TYPE=postgres and XUI_DB_DSN to run the postgres migration test")
+	}
+	if err := InitDB(""); err != nil {
+		t.Fatalf("InitDB: %v", err)
+	}
+	t.Cleanup(func() { _ = CloseDB() })
+	GetDB().Exec("TRUNCATE TABLE hosts, inbounds RESTART IDENTITY CASCADE")
+	ib := seedInboundWithStream(t, "vcn-legacy-pg", 5558, `{"network":"tcp","security":"tls"}`)
+	for _, stmt := range []string{
+		"ALTER TABLE hosts DROP COLUMN cipher_suites",
+		"ALTER TABLE hosts DROP COLUMN verify_peer_cert_by_name",
+		"ALTER TABLE hosts ADD COLUMN verify_peer_cert_by_name boolean DEFAULT false",
+	} {
+		if err := GetDB().Exec(stmt).Error; err != nil {
+			t.Fatalf("%s: %v", stmt, err)
+		}
+	}
+	for _, vcn := range []bool{false, true} {
+		if err := GetDB().Exec(
+			"INSERT INTO hosts (group_id, inbound_id, remark, address, security, verify_peer_cert_by_name) VALUES ('g', ?, 'legacy', 'cdn.example.com', 'tls', ?)",
+			ib.Id, vcn,
+		).Error; err != nil {
+			t.Fatalf("insert legacy host %v: %v", vcn, err)
+		}
+	}
+	if err := GetDB().Where("seeder_name = ?", "HostVerifyPeerCertByNameBoolFix").Delete(&model.HistoryOfSeeders{}).Error; err != nil {
+		t.Fatalf("clear seeder history: %v", err)
+	}
+	if err := CloseDB(); err != nil {
+		t.Fatalf("CloseDB: %v", err)
+	}
+
+	if err := InitDB(""); err != nil {
+		t.Fatalf("reopen InitDB: %v", err)
+	}
+
+	var names []string
+	if err := GetDB().Model(&model.Host{}).Where("inbound_id = ?", ib.Id).Pluck("verify_peer_cert_by_name", &names).Error; err != nil {
+		t.Fatalf("load hosts: %v", err)
+	}
+	if len(names) != 2 || names[0] != "" || names[1] != "" {
+		t.Fatalf("verifyPeerCertByName = %q, want two blanks", names)
+	}
+}
+
 // #2 — a second run is a no-op (the HistoryOfSeeders gate).
 func TestMigrate_Idempotent(t *testing.T) {
 	initMigrateDB(t)