P8.1: fix browser inbound re-homing — nodeId probe must parse form-encoded bodies

updateInbound detected an explicit nodeId by probing the raw request body as JSON only (requestHasJSONKey). The panel frontend, however, posts application/x-www-form-urlencoded by default (http-init.ts form-encodes every POST without a declared JSON Content-Type), so the probe missed on every real browser save and UpdateInbound silently kept the stored assignment — the edit dialog's 部署到 picker never moved anything, while JSON API callers (and the P8 E2E, which posted JSON) worked. requestHasKey is now content-type aware: JSON bodies are probed as before; urlencoded bodies are parsed with url.ParseQuery, where the browser's null encoding (a bare "nodeId=") also counts as provided — it binds as *int(0) and the existing 0->nil normalisation turns it into the move-to-local target. Regression tests post the exact browser wire format through the real handler stack: form move between nodes, form post without nodeId (assignment kept), form post with nodeId= (move to local, no binding error), plus the JSON move at HTTP level. The two form-move tests fail on the unfixed code and pass with the fix.
2026-10-21 15:37:45 +00:00
parent 7284fa6cf0
commit 6772c97f8e
2 changed files with 376 additions and 15 deletions
@@ -4,7 +4,9 @@ import (
 	"bytes"
 	"encoding/json"
 	"io"
+	"mime"
 	"net"
+	"net/url"
 	"strconv"
 	"strings"

@@ -224,11 +226,17 @@ func (a *InboundController) bulkDelInbounds(c *gin.Context) {
 	notifyClientsChanged()
 }

-// requestHasJSONKey reports whether the request body is a JSON object that
-// explicitly carries key — present with a null value counts. The body is
-// restored so the normal bind can read it again. Non-JSON bodies (form
-// posts) report false for every key.
-func requestHasJSONKey(c *gin.Context, key string) bool {
+// requestHasKey reports whether the request body explicitly carries key —
+// present with a JSON null or an empty form value counts. The probe must
+// understand both wire formats this endpoint receives: API clients post
+// JSON, but the panel frontend posts application/x-www-form-urlencoded by
+// default (frontend/src/api/http-init.ts form-encodes every POST that does
+// not declare a JSON Content-Type, encoding a null value as a bare "key=").
+// A JSON-only probe silently reports false for every real browser post —
+// that is exactly how browser-driven nodeId reassignments became a no-op
+// while JSON callers kept working. The body is restored so the normal
+// bind can read it again.
+func requestHasKey(c *gin.Context, key string) bool {
 	if c.Request == nil || c.Request.Body == nil {
 		return false
 	}
@@ -237,13 +245,29 @@ func requestHasJSONKey(c *gin.Context, key string) bool {
 		return false
 	}
 	c.Request.Body = io.NopCloser(bytes.NewReader(body))
-	var probe map[string]json.RawMessage
-	if json.Unmarshal(body, &probe) != nil {
-		return false
+
+	mediaType, _, _ := mime.ParseMediaType(c.GetHeader("Content-Type"))
+	if mediaType == "application/x-www-form-urlencoded" {
+		return formHasKey(body, key)
 	}
+	var probe map[string]json.RawMessage
+	if json.Unmarshal(body, &probe) == nil {
 		_, ok := probe[key]
 		return ok
 	}
+	// Undeclared or unexpected Content-Type with a non-JSON body: fall
+	// back to form parsing so the key is still understood.
+	return formHasKey(body, key)
+}
+
+// formHasKey reports whether a urlencoded body carries key, even with an
+// empty value. ParseQuery keeps every valid pair it finds, so its map is
+// usable even when it also returns an error for a later malformed pair.
+func formHasKey(body []byte, key string) bool {
+	values, _ := url.ParseQuery(string(body))
+	_, ok := values[key]
+	return ok
+}

 // updateInbound updates an existing inbound configuration.
 func (a *InboundController) updateInbound(c *gin.Context) {
@@ -255,16 +279,19 @@ func (a *InboundController) updateInbound(c *gin.Context) {
 	inbound := &model.Inbound{
 		Id: id,
 	}
-	// An explicitly sent nodeId (even null) is a (re)assignment request —
-	// UpdateInbound migrates the inbound when it differs from the stored
-	// assignment. An omitted nodeId keeps the stored assignment, so detect
-	// the key's presence on the raw body before binding consumes it.
-	nodeIDProvided := requestHasJSONKey(c, "nodeId")
+	// An explicitly sent nodeId (JSON null, or the form encoding of one —
+	// a bare "nodeId=") is a (re)assignment request: UpdateInbound migrates
+	// the inbound when it differs from the stored assignment. An omitted
+	// nodeId keeps the stored assignment, so detect the key's presence on
+	// the raw body before binding consumes it.
+	nodeIDProvided := requestHasKey(c, "nodeId")
 	if !middleware.BindAndValidateInto(c, inbound) {
 		return
 	}
 	inbound.NodeIDProvided = nodeIDProvided
-	// Same NodeID=0 → nil normalisation as addInbound.
+	// Same NodeID=0 → nil normalisation as addInbound. This also covers
+	// the browser's encoding of "back to local": the form post carries a
+	// bare "nodeId=", which gin binds as *int(0) rather than nil.
 	if inbound.NodeID != nil && *inbound.NodeID == 0 {
 		inbound.NodeID = nil
 	}
@@ -0,0 +1,334 @@
+package controller
+
+import (
+	"encoding/json"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"path/filepath"
+	"strconv"
+	"strings"
+	"testing"
+
+	"github.com/gin-contrib/sessions"
+	"github.com/gin-contrib/sessions/cookie"
+	"github.com/gin-gonic/gin"
+	"github.com/op/go-logging"
+
+	"github.com/mhsanaei/3x-ui/v3/internal/database"
+	"github.com/mhsanaei/3x-ui/v3/internal/database/model"
+	xuilogger "github.com/mhsanaei/3x-ui/v3/internal/logger"
+	"github.com/mhsanaei/3x-ui/v3/internal/web/service/panel"
+	"github.com/mhsanaei/3x-ui/v3/internal/web/session"
+	"github.com/mhsanaei/3x-ui/v3/internal/xray"
+)
+
+// Regression tests for the P8.1 hotfix: browser-driven inbound
+// re-homing (the "部署到" picker in the edit dialog) silently did nothing.
+//
+// Root cause: the panel frontend posts application/x-www-form-urlencoded
+// by default (frontend/src/api/http-init.ts form-encodes every POST that
+// does not declare a JSON Content-Type), but updateInbound detected an
+// explicit nodeId with a JSON-only probe of the raw body. The probe
+// reported false for every real browser post, so UpdateInbound kept the
+// stored assignment. JSON API callers worked — including the P8 sandbox
+// E2E — which is why these tests post the *form* wire format exactly as
+// the browser builds it (the field set of formValuesToWirePayload in
+// frontend/src/lib/xray/inbound-form-adapter.ts, encodeForm semantics:
+// every key present, a null value emitted as a bare "key=").
+
+func newInboundWireEngine(t *testing.T) *gin.Engine {
+	t.Helper()
+	xuilogger.InitLogger(logging.ERROR)
+	gin.SetMode(gin.TestMode)
+	dbDir := t.TempDir()
+	t.Setenv("XUI_DB_FOLDER", dbDir)
+	if err := database.InitDB(filepath.Join(dbDir, "x-ui.db")); err != nil {
+		t.Fatalf("InitDB: %v", err)
+	}
+	t.Cleanup(func() { _ = database.CloseDB() })
+
+	engine := gin.New()
+	store := cookie.NewStore([]byte("inbound-wire-test-secret"))
+	engine.Use(sessions.Sessions("3x-ui", store))
+	// Logs in as the first user so updateInbound's session lookup works,
+	// mirroring the production session path over a cookie round-trip.
+	engine.GET("/test-login", func(c *gin.Context) {
+		u, err := (&panel.UserService{}).GetFirstUser()
+		if err != nil {
+			c.Status(http.StatusInternalServerError)
+			return
+		}
+		if err := session.SetLoginUser(c, u); err != nil {
+			c.Status(http.StatusInternalServerError)
+			return
+		}
+		c.Status(http.StatusOK)
+	})
+	NewInboundController(engine.Group("/panel/api/inbounds"))
+	return engine
+}
+
+func inboundWireLogin(t *testing.T, engine *gin.Engine) []*http.Cookie {
+	t.Helper()
+	w := httptest.NewRecorder()
+	engine.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/test-login", nil))
+	if w.Code != http.StatusOK {
+		t.Fatalf("test login: status %d", w.Code)
+	}
+	cookies := w.Result().Cookies()
+	if len(cookies) == 0 {
+		t.Fatal("test login: no session cookie issued")
+	}
+	return cookies
+}
+
+func seedWireNode(t *testing.T, name string) *model.Node {
+	t.Helper()
+	n := &model.Node{
+		Name:                name,
+		Scheme:              "http",
+		Address:             "127.0.0.1",
+		Port:                1, // unreachable on purpose: UpdateInbound never dials nodes
+		BasePath:            "/",
+		ApiToken:            "tok",
+		Enable:              true,
+		AllowPrivateAddress: true,
+		Status:              "online",
+		InboundSyncMode:     "all",
+		InboundsAdoptedAt:   1,
+	}
+	if err := database.GetDB().Create(n).Error; err != nil {
+		t.Fatalf("seed node %s: %v", name, err)
+	}
+	return n
+}
+
+const wireTestSettings = `{"clients":[{"id":"11111111-2222-3333-4444-555555555555","email":"wire@example.com","enable":true}]}`
+
+func seedWireInbound(t *testing.T, tag string, port int, nodeID *int) model.Inbound {
+	t.Helper()
+	ib := &model.Inbound{
+		Tag:               tag,
+		Enable:            true,
+		Port:              port,
+		Protocol:          model.VLESS,
+		Settings:          wireTestSettings,
+		StreamSettings:    `{"network":"tcp"}`,
+		Sniffing:          `{"enabled":false}`,
+		NodeID:            nodeID,
+		ShareAddrStrategy: "node",
+		TrafficReset:      "never",
+	}
+	if err := database.GetDB().Create(ib).Error; err != nil {
+		t.Fatalf("seed inbound: %v", err)
+	}
+	ct := &xray.ClientTraffic{InboundId: ib.Id, Enable: true, Email: "wire@example.com", Up: 111, Down: 222}
+	if err := database.GetDB().Create(ct).Error; err != nil {
+		t.Fatalf("seed client traffic: %v", err)
+	}
+	return reloadWireInbound(t, ib.Id)
+}
+
+func reloadWireInbound(t *testing.T, id int) model.Inbound {
+	t.Helper()
+	var ib model.Inbound
+	if err := database.GetDB().First(&ib, id).Error; err != nil {
+		t.Fatalf("reload inbound %d: %v", id, err)
+	}
+	return ib
+}
+
+func wireNodeDirty(t *testing.T, id int) bool {
+	t.Helper()
+	var n model.Node
+	if err := database.GetDB().First(&n, id).Error; err != nil {
+		t.Fatalf("reload node %d: %v", id, err)
+	}
+	return n.ConfigDirty
+}
+
+func wireTrafficIntact(t *testing.T) {
+	t.Helper()
+	var ct xray.ClientTraffic
+	if err := database.GetDB().Where("email = ?", "wire@example.com").First(&ct).Error; err != nil {
+		t.Fatalf("read client traffic: %v", err)
+	}
+	if ct.Up != 111 || ct.Down != 222 {
+		t.Fatalf("client traffic changed: up=%d down=%d, want 111/222", ct.Up, ct.Down)
+	}
+}
+
+// browserFormBody renders the update body exactly as the browser sends
+// it: the formValuesToWirePayload field set, url-encoded, with nodeId
+// present as a number, present-but-empty (the null/local encoding), or
+// omitted entirely (legacy callers), per the arguments.
+func browserFormBody(ib model.Inbound, nodeID *int, omitNodeID bool) string {
+	v := url.Values{}
+	v.Set("up", strconv.FormatInt(ib.Up, 10))
+	v.Set("down", strconv.FormatInt(ib.Down, 10))
+	v.Set("total", strconv.FormatInt(ib.Total, 10))
+	v.Set("remark", ib.Remark)
+	v.Set("enable", strconv.FormatBool(ib.Enable))
+	v.Set("expiryTime", strconv.FormatInt(ib.ExpiryTime, 10))
+	v.Set("trafficReset", ib.TrafficReset)
+	v.Set("trafficResetDay", strconv.Itoa(ib.TrafficResetDay))
+	v.Set("lastTrafficResetTime", strconv.FormatInt(ib.LastTrafficResetTime, 10))
+	v.Set("listen", ib.Listen)
+	v.Set("port", strconv.Itoa(ib.Port))
+	v.Set("protocol", string(ib.Protocol))
+	v.Set("settings", ib.Settings)
+	v.Set("streamSettings", ib.StreamSettings)
+	v.Set("sniffing", ib.Sniffing)
+	v.Set("tag", ib.Tag)
+	v.Set("shareAddrStrategy", ib.ShareAddrStrategy)
+	v.Set("shareAddr", ib.ShareAddr)
+	v.Set("subSortIndex", strconv.Itoa(ib.SubSortIndex))
+	v.Set("disableFlow", strconv.FormatBool(ib.DisableFlow))
+	if !omitNodeID {
+		if nodeID == nil {
+			v.Set("nodeId", "") // encodeForm renders a null as a bare "nodeId="
+		} else {
+			v.Set("nodeId", strconv.Itoa(*nodeID))
+		}
+	}
+	return v.Encode()
+}
+
+type wireEnvelope struct {
+	Success bool   `json:"success"`
+	Msg     string `json:"msg"`
+}
+
+func postInboundUpdate(t *testing.T, engine *gin.Engine, cookies []*http.Cookie, id int, body, contentType string) wireEnvelope {
+	t.Helper()
+	req := httptest.NewRequest(http.MethodPost, fmt.Sprintf("/panel/api/inbounds/update/%d", id), strings.NewReader(body))
+	req.Header.Set("Content-Type", contentType)
+	for _, ck := range cookies {
+		req.AddCookie(ck)
+	}
+	w := httptest.NewRecorder()
+	engine.ServeHTTP(w, req)
+	if w.Code != http.StatusOK {
+		t.Fatalf("update %d: HTTP %d, body=%s", id, w.Code, w.Body.String())
+	}
+	var env wireEnvelope
+	if err := json.Unmarshal(w.Body.Bytes(), &env); err != nil {
+		t.Fatalf("update %d: decode envelope: %v body=%s", id, err, w.Body.String())
+	}
+	return env
+}
+
+// (a) The browser's headline flow: a form post carrying nodeId=<target>
+// must perform the move — row reassigned, both nodes marked dirty,
+// clients and traffic untouched.
+func TestUpdateInboundFormWire_MoveBetweenNodes(t *testing.T) {
+	engine := newInboundWireEngine(t)
+	cookies := inboundWireLogin(t, engine)
+	nodeA := seedWireNode(t, "wire-a")
+	nodeB := seedWireNode(t, "wire-b")
+	seeded := seedWireInbound(t, "wire-move", 2461, &nodeA.Id)
+
+	body := browserFormBody(seeded, &nodeB.Id, false)
+	env := postInboundUpdate(t, engine, cookies, seeded.Id, body, "application/x-www-form-urlencoded")
+	if !env.Success {
+		t.Fatalf("form move update failed: msg=%q", env.Msg)
+	}
+
+	after := reloadWireInbound(t, seeded.Id)
+	if after.NodeID == nil || *after.NodeID != nodeB.Id {
+		t.Fatalf("node_id after form move = %v, want %d", after.NodeID, nodeB.Id)
+	}
+	if !wireNodeDirty(t, nodeA.Id) || !wireNodeDirty(t, nodeB.Id) {
+		t.Fatalf("both nodes must be dirty after a move (A=%v B=%v)",
+			wireNodeDirty(t, nodeA.Id), wireNodeDirty(t, nodeB.Id))
+	}
+	wireTrafficIntact(t)
+}
+
+// (b) A form post without nodeId keeps the stored assignment (the legacy
+// caller contract, now exercised over the form wire format too).
+func TestUpdateInboundFormWire_OmittedNodeIDKeepsAssignment(t *testing.T) {
+	engine := newInboundWireEngine(t)
+	cookies := inboundWireLogin(t, engine)
+	nodeA := seedWireNode(t, "wire-a")
+	seedWireNode(t, "wire-b")
+	seeded := seedWireInbound(t, "wire-keep", 2462, &nodeA.Id)
+
+	body := browserFormBody(seeded, nil, true)
+	env := postInboundUpdate(t, engine, cookies, seeded.Id, body, "application/x-www-form-urlencoded")
+	if !env.Success {
+		t.Fatalf("form no-nodeId update failed: msg=%q", env.Msg)
+	}
+
+	after := reloadWireInbound(t, seeded.Id)
+	if after.NodeID == nil || *after.NodeID != nodeA.Id {
+		t.Fatalf("node_id after omitted-nodeId form update = %v, want %d (unchanged)", after.NodeID, nodeA.Id)
+	}
+	wireTrafficIntact(t)
+}
+
+// (c) The browser encodes "back to local panel" (allowClear on the
+// picker) as a bare "nodeId=". That must count as provided with a nil
+// target — and must not break binding of the *int field.
+func TestUpdateInboundFormWire_EmptyNodeIDMovesToLocal(t *testing.T) {
+	engine := newInboundWireEngine(t)
+	cookies := inboundWireLogin(t, engine)
+	nodeA := seedWireNode(t, "wire-a")
+	seeded := seedWireInbound(t, "wire-local", 2463, &nodeA.Id)
+
+	body := browserFormBody(seeded, nil, false)
+	if !strings.Contains(body, "nodeId=") {
+		t.Fatalf("test body must carry the bare nodeId= encoding, got %q", body)
+	}
+	env := postInboundUpdate(t, engine, cookies, seeded.Id, body, "application/x-www-form-urlencoded")
+	if !env.Success {
+		t.Fatalf("form empty-nodeId update failed: msg=%q", env.Msg)
+	}
+
+	after := reloadWireInbound(t, seeded.Id)
+	if after.NodeID != nil {
+		t.Fatalf("node_id after empty-nodeId form update = %v, want nil (local)", after.NodeID)
+	}
+	if !wireNodeDirty(t, nodeA.Id) {
+		t.Fatal("source node must be dirty after a move to local")
+	}
+	wireTrafficIntact(t)
+}
+
+// (d) The JSON caller path keeps working unchanged at the HTTP level.
+func TestUpdateInboundJSONWire_MoveBetweenNodes(t *testing.T) {
+	engine := newInboundWireEngine(t)
+	cookies := inboundWireLogin(t, engine)
+	nodeA := seedWireNode(t, "wire-a")
+	nodeB := seedWireNode(t, "wire-b")
+	seeded := seedWireInbound(t, "wire-json", 2464, &nodeA.Id)
+
+	payload := map[string]any{
+		"up": seeded.Up, "down": seeded.Down, "total": seeded.Total,
+		"remark": seeded.Remark, "enable": seeded.Enable,
+		"expiryTime": seeded.ExpiryTime, "trafficReset": seeded.TrafficReset,
+		"trafficResetDay": seeded.TrafficResetDay, "lastTrafficResetTime": seeded.LastTrafficResetTime,
+		"listen": seeded.Listen, "port": seeded.Port, "protocol": string(seeded.Protocol),
+		"settings": seeded.Settings, "streamSettings": seeded.StreamSettings,
+		"sniffing": seeded.Sniffing, "tag": seeded.Tag,
+		"shareAddrStrategy": seeded.ShareAddrStrategy, "shareAddr": seeded.ShareAddr,
+		"subSortIndex": seeded.SubSortIndex, "disableFlow": seeded.DisableFlow,
+		"nodeId": nodeB.Id,
+	}
+	raw, err := json.Marshal(payload)
+	if err != nil {
+		t.Fatalf("marshal payload: %v", err)
+	}
+	env := postInboundUpdate(t, engine, cookies, seeded.Id, string(raw), "application/json")
+	if !env.Success {
+		t.Fatalf("JSON move update failed: msg=%q", env.Msg)
+	}
+
+	after := reloadWireInbound(t, seeded.Id)
+	if after.NodeID == nil || *after.NodeID != nodeB.Id {
+		t.Fatalf("node_id after JSON move = %v, want %d", after.NodeID, nodeB.Id)
+	}
+	wireTrafficIntact(t)
+}