Browse Source

fix(cli): let -getApiToken name the token it regenerates (#6405)

* fix(cli): let -getApiToken name the token it regenerates

The flag's help text said "Display current API token". It cannot display
anything -- tokens are stored as SHA-256 hashes, and the command's own first
two output lines say so. What it does is destroy and reissue a credential:
GetApiToken calls RecreateByName on the hardcoded name "cli-fallback". The
help therefore invited an operator to run a command they believed was
read-only, and it revoked a token someone else was holding.

Because that name is a single global slot, two callers silently invalidate
each other, and the loser is left with a token that answers HTTP 404 with an
empty body -- indistinguishable from a wrong base path, so the failure does
not even say what happened. install.sh is one of those callers, at lines 1231
and 1325, so the collision already exists inside this repository.

Add -tokenName, defaulting to cli-fallback so install.sh and every existing
invocation behave exactly as before. -getApiToken stays a boolean on purpose:
install.sh calls it as `x-ui setting -getApiToken true`, and a string flag
would swallow that trailing argument and mint a token named "true".

The name now reaches both branches of GetApiToken. On a database with no
tokens the command used to create one called "install", which the CLI could
then never rotate -- defeating the stated purpose of the cli-fallback constant,
that -getApiToken cannot accumulate admin-equivalent credentials it never
revokes. Both branches use the resolved name, so repeated calls rotate a
single slot instead of leaving a permanent token behind.

Also cap the name at 64 characters in RecreateByName. Create already enforces
that limit on the same column; RecreateByName did not, and it now receives
operator input.

Assisted-by: Claude Code:claude-opus-5 (mostly)

* fix(cli): keep the installer's token out of the rotated slot

Folding both branches of GetApiToken onto one name made the bug worse in the
exact case this change is about. install.sh records the token it gets on a
fresh panel; with both branches on cli-fallback, the next bare -getApiToken
rotated that very row and silently invalidated the credential written into the
install-result file.

Restore the split default -- "install" when the database has no tokens,
cli-fallback when it does -- so nothing about an unnamed call changes. An
explicit -tokenName still applies to both branches, which is what keeps the
flag coherent: -tokenName ci-bot now yields ci-bot on a fresh panel too,
rather than "install".

Pin it with a test that reads the install row's id and hash before and after a
rotation, since a name-only assertion would pass against a deleted-and-
recreated row.

* fix(cli): stop the `-getApiToken true` form from swallowing -tokenName

Three corrections from review.

Go's flag package stops parsing at the first non-flag argument, so the trailing
`true` in install.sh's invocation does not merely get ignored -- it terminates
parsing. An operator copying that documented shape and writing
`x-ui setting -getApiToken true -tokenName ci-bot` left tokenName empty, so the
command rotated cli-fallback: the shared-slot collision this change exists to
remove, reachable through the one form the repository itself demonstrates.
Verified against the built binary, which printed
`The API token "cli-fallback" has been regenerated`.

Drop the stray `true` from both install.sh call sites so the documented form no
longer teaches the trap, and warn whenever `setting` is given positional
arguments, naming what was ignored. A warning rather than an error, because an
older install.sh in the wild still passes `true` and must keep working.

Cover both branches in the help strings. They described only the rotation path,
so on a fresh panel -h announced cli-fallback while the command actually mints
`install`, and nothing is regenerated or invalidated there at all -- misleading
help being the defect this change set out to remove.

Assert the concrete error in the name-length test. It checked only that some
error came back, which RecreateByName's empty-name guard and its transaction
errors would satisfy just as well.
ilyusha 6 hours ago
parent
commit
d2ac3b4d7a
5 changed files with 208 additions and 9 deletions
  1. 148 0
      api_token_cli_test.go
  2. 2 2
      install.sh
  3. 4 0
      internal/web/service/panel/api_token.go
  4. 25 0
      internal/web/service/panel/api_token_test.go
  5. 29 7
      main.go

+ 148 - 0
api_token_cli_test.go

@@ -0,0 +1,148 @@
+package main
+
+// GetApiToken rotates a credential rather than displaying one, so these pin
+// which token name it destroys — the whole point of the -tokenName flag.
+
+import (
+	"flag"
+	"testing"
+
+	"github.com/mhsanaei/3x-ui/v3/internal/config"
+	"github.com/mhsanaei/3x-ui/v3/internal/database"
+	"github.com/mhsanaei/3x-ui/v3/internal/database/model"
+	"github.com/mhsanaei/3x-ui/v3/internal/web/service/panel"
+)
+
+func newTokenCLIEnv(t *testing.T) {
+	t.Helper()
+	t.Setenv("XUI_DB_FOLDER", t.TempDir())
+	if err := database.InitDB(config.GetDBPath()); err != nil {
+		t.Fatalf("init db: %v", err)
+	}
+	t.Cleanup(func() { _ = database.CloseDB() })
+}
+
+func tokenNames(t *testing.T) []string {
+	t.Helper()
+	tokens, err := (&panel.ApiTokenService{}).List()
+	if err != nil {
+		t.Fatalf("list tokens: %v", err)
+	}
+	names := make([]string, 0, len(tokens))
+	for _, token := range tokens {
+		names = append(names, token.Name)
+	}
+	return names
+}
+
+func tokenRow(t *testing.T, name string) model.ApiToken {
+	t.Helper()
+	var row model.ApiToken
+	if err := database.GetDB().Where("name = ?", name).First(&row).Error; err != nil {
+		t.Fatalf("load token %q: %v", name, err)
+	}
+	return row
+}
+
+func hasName(names []string, want string) bool {
+	for _, name := range names {
+		if name == want {
+			return true
+		}
+	}
+	return false
+}
+
+// The bug: two callers sharing one hardcoded slot silently revoke each other.
+// A named token must leave an differently-named one authenticating.
+func TestGetApiTokenRotatesOnlyTheNamedToken(t *testing.T) {
+	newTokenCLIEnv(t)
+
+	svc := panel.ApiTokenService{}
+	weekly, err := svc.RecreateByName("weekly-report")
+	if err != nil {
+		t.Fatalf("seed weekly-report: %v", err)
+	}
+
+	GetApiToken(true, "ci-bot")
+
+	names := tokenNames(t)
+	if !hasName(names, "ci-bot") {
+		t.Fatalf("token names = %v, want ci-bot among them", names)
+	}
+	if !svc.Match(weekly.Token) {
+		t.Fatal("weekly-report was revoked by a call naming ci-bot")
+	}
+}
+
+// An explicit name has to win on both branches, or the same command would
+// produce ci-bot on a populated panel and "install" on a fresh one.
+func TestGetApiTokenUsesGivenNameOnEmptyDatabase(t *testing.T) {
+	newTokenCLIEnv(t)
+
+	GetApiToken(true, "ci-bot")
+
+	names := tokenNames(t)
+	if !hasName(names, "ci-bot") {
+		t.Fatalf("token names = %v, want ci-bot among them", names)
+	}
+	if hasName(names, installTokenName) {
+		t.Fatalf("token names = %v, want no %s when a name was given", names, installTokenName)
+	}
+}
+
+// install.sh records the token it gets on a fresh panel. A later bare
+// -getApiToken must rotate the fallback slot and leave that record valid.
+func TestGetApiTokenPreservesInstallTokenWhenRotating(t *testing.T) {
+	newTokenCLIEnv(t)
+
+	GetApiToken(true, "")
+	installed := tokenRow(t, installTokenName)
+
+	GetApiToken(true, "")
+
+	names := tokenNames(t)
+	if !hasName(names, cliFallbackTokenName) {
+		t.Fatalf("token names = %v, want %s among them", names, cliFallbackTokenName)
+	}
+	if got := tokenRow(t, installTokenName); got.Id != installed.Id {
+		t.Fatalf("%s row id = %d, want %d — the installer's token was replaced", installTokenName, got.Id, installed.Id)
+	}
+	if got := tokenRow(t, installTokenName); got.Token != installed.Token {
+		t.Fatalf("the %s token hash changed, so the recorded credential stopped working", installTokenName)
+	}
+}
+
+// `-getApiToken true -tokenName ci-bot` parses tokenName as "", because flag
+// stops at the positional. The command must not then rotate the shared slot.
+func TestGetApiTokenWarnsOnIgnoredPositionalArgs(t *testing.T) {
+	set := flag.NewFlagSet("setting", flag.ContinueOnError)
+	var getApiToken bool
+	var tokenName string
+	set.BoolVar(&getApiToken, "getApiToken", false, "")
+	set.StringVar(&tokenName, "tokenName", "", "")
+
+	if err := set.Parse([]string{"-getApiToken", "true", "-tokenName", "ci-bot"}); err != nil {
+		t.Fatalf("parse: %v", err)
+	}
+	if tokenName != "" {
+		t.Fatalf("tokenName = %q; this test guards the case where flag drops it", tokenName)
+	}
+	if got := set.Args(); len(got) == 0 {
+		t.Fatal("leftover arguments must be visible so the CLI can warn instead of silently rotating cli-fallback")
+	}
+}
+
+func TestGetApiTokenTrimsName(t *testing.T) {
+	newTokenCLIEnv(t)
+
+	if _, err := (&panel.ApiTokenService{}).RecreateByName("seed"); err != nil {
+		t.Fatalf("seed: %v", err)
+	}
+	GetApiToken(true, "   ")
+
+	names := tokenNames(t)
+	if !hasName(names, cliFallbackTokenName) {
+		t.Fatalf("token names = %v, want a whitespace-only name to fall back to %s", names, cliFallbackTokenName)
+	}
+}

+ 2 - 2
install.sh

@@ -1228,7 +1228,7 @@ EOF
             prompt_and_setup_ssl "${config_port}" "${config_webBasePath}" "${server_ip}"
 
             # Retrieve the API token for display
-            local config_apiToken=$(${xui_folder}/x-ui setting -getApiToken true | grep -Eo 'apiToken: .+' | awk '{print $2}')
+            local config_apiToken=$(${xui_folder}/x-ui setting -getApiToken | grep -Eo 'apiToken: .+' | awk '{print $2}')
 
             # Display final credentials and access information
             echo ""
@@ -1322,7 +1322,7 @@ EOF
 
             # Persist a machine-parseable credentials file for cloud-init / MOTD.
             local config_apiToken
-            config_apiToken=$(${xui_folder}/x-ui setting -getApiToken true | grep -Eo 'apiToken: .+' | awk '{print $2}')
+            config_apiToken=$(${xui_folder}/x-ui setting -getApiToken | grep -Eo 'apiToken: .+' | awk '{print $2}')
             : "${SSL_SCHEME:=https}"
             : "${SSL_HOST:=${server_ip}}"
             write_install_result "${config_username}" "${config_password}" "${existing_port}" \

+ 4 - 0
internal/web/service/panel/api_token.go

@@ -124,6 +124,10 @@ func (s *ApiTokenService) RecreateByName(name string) (*ApiTokenView, error) {
 	if name == "" {
 		return nil, common.NewError("token name is required")
 	}
+	// Same column, same limit as Create: the CLI now feeds this operator input.
+	if len(name) > 64 {
+		return nil, common.NewError("token name must be 64 characters or fewer")
+	}
 	plaintext := random.Seq(apiTokenLength)
 	row := &model.ApiToken{Name: name, Token: crypto.HashTokenSHA256(plaintext), Enabled: true}
 	if err := database.GetDB().Transaction(func(tx *gorm.DB) error {

+ 25 - 0
internal/web/service/panel/api_token_test.go

@@ -2,6 +2,7 @@ package panel
 
 import (
 	"errors"
+	"strings"
 	"testing"
 
 	"gorm.io/gorm"
@@ -68,6 +69,30 @@ func TestRecreateByNamePreservesTokenWhenReplacementFails(t *testing.T) {
 	}
 }
 
+// Create caps the name at 64 characters; RecreateByName writes the same column
+// and now takes operator input from -tokenName, so it must cap it too.
+func TestRecreateByNameRejectsOverlongName(t *testing.T) {
+	t.Setenv("XUI_DB_FOLDER", t.TempDir())
+	if err := database.InitDB(config.GetDBPath()); err != nil {
+		t.Fatalf("init db: %v", err)
+	}
+	t.Cleanup(func() { _ = database.CloseDB() })
+
+	const wantErr = "token name must be 64 characters or fewer"
+
+	svc := ApiTokenService{}
+	_, err := svc.RecreateByName(strings.Repeat("n", 65))
+	if err == nil {
+		t.Fatal("expected a 65-character token name to be rejected")
+	}
+	if got := strings.TrimSpace(err.Error()); got != wantErr {
+		t.Fatalf("error = %q, want %q — any other error would pass a bare nil check", got, wantErr)
+	}
+	if _, err := svc.RecreateByName(strings.Repeat("n", 64)); err != nil {
+		t.Fatalf("64 characters is the documented limit, got: %v", err)
+	}
+}
+
 func TestRecreateByNameKeepsOneToken(t *testing.T) {
 	t.Setenv("XUI_DB_FOLDER", t.TempDir())
 	if err := database.InitDB(config.GetDBPath()); err != nil {

+ 29 - 7
main.go

@@ -11,6 +11,7 @@ import (
 	_ "net/http/pprof"
 	"os"
 	"os/signal"
+	"strings"
 	"syscall"
 	_ "unsafe"
 
@@ -36,6 +37,10 @@ import (
 // cannot accumulate admin-equivalent credentials that are never revoked.
 const cliFallbackTokenName = "cli-fallback"
 
+// installTokenName is minted once on a panel with no tokens and is deliberately
+// not the rotated slot, so the credential the installer recorded keeps working.
+const installTokenName = "install"
+
 // initNodeTokenCrypto loads the process codec, preferring the key file over
 // the environment and failing closed when an enabled policy lacks a key.
 func initNodeTokenCrypto() error {
@@ -493,10 +498,13 @@ func GetListenIP(getListen bool) {
 	}
 }
 
-func GetApiToken(getApiToken bool) {
+func GetApiToken(getApiToken bool, tokenName string) {
 	if !getApiToken {
 		return
 	}
+	// An explicit name applies to both branches below; without one each keeps
+	// the name it already used, so every existing invocation is unaffected.
+	name := strings.TrimSpace(tokenName)
 	err := database.InitDB(config.GetDBPath())
 	if err != nil {
 		fmt.Println("open database failed, error info:", err)
@@ -512,18 +520,25 @@ func GetApiToken(getApiToken bool) {
 		fmt.Printf("There are %d API token(s) configured. Existing tokens cannot be retrieved in plaintext because only hashes are stored.\n", len(tokens))
 		fmt.Println("If you have lost your token, you can manage and generate new tokens through the Panel UI (Settings -> API Tokens).")
 
-		// Rotate one reusable fallback so repeated calls cannot pile up
+		// Rotate one token per name so repeated calls cannot pile up
 		// indefinitely many admin-equivalent tokens that never expire.
-		created, err := apiTokenService.RecreateByName(cliFallbackTokenName)
+		rotated := name
+		if rotated == "" {
+			rotated = cliFallbackTokenName
+		}
+		created, err := apiTokenService.RecreateByName(rotated)
 		if err != nil {
 			fmt.Println("Failed to create a fallback API token:", err)
 			return
 		}
-		fmt.Println("\nThe CLI fallback token has been regenerated (any previous one is now invalid):")
+		fmt.Printf("\nThe API token %q has been regenerated (any previous one is now invalid):\n", rotated)
 		fmt.Println("apiToken:", created.Token)
 		return
 	}
-	created, err := apiTokenService.Create("install", "", 0)
+	if name == "" {
+		name = installTokenName
+	}
+	created, err := apiTokenService.Create(name, "", 0)
 	if err != nil {
 		fmt.Println("create apiToken failed, error info:", err)
 		return
@@ -605,6 +620,7 @@ func main() {
 	var show bool
 	var getCert bool
 	var getApiToken bool
+	var tokenName string
 	var resetTwoFactor bool
 	settingCmd.BoolVar(&reset, "reset", false, "Reset all settings")
 	settingCmd.BoolVar(&show, "show", false, "Display current settings")
@@ -616,7 +632,8 @@ func main() {
 	settingCmd.BoolVar(&resetTwoFactor, "resetTwoFactor", false, "Reset two-factor authentication settings")
 	settingCmd.BoolVar(&getListen, "getListen", false, "Display current panel listenIP IP")
 	settingCmd.BoolVar(&getCert, "getCert", false, "Display current certificate settings")
-	settingCmd.BoolVar(&getApiToken, "getApiToken", false, "Display current API token")
+	settingCmd.BoolVar(&getApiToken, "getApiToken", false, "Print an API token for CLI use, regenerating it and invalidating the previous one; on a panel with no tokens yet it mints one instead")
+	settingCmd.StringVar(&tokenName, "tokenName", "", "Name of the token -getApiToken acts on (default: "+cliFallbackTokenName+", or "+installTokenName+" on a panel with no tokens)")
 	settingCmd.StringVar(&webCertFile, "webCert", "", "Set path to public key file for panel")
 	settingCmd.StringVar(&webKeyFile, "webCertKey", "", "Set path to private key file for panel")
 	settingCmd.StringVar(&tgbottoken, "tgbottoken", "", "Set token for Telegram bot")
@@ -688,6 +705,11 @@ func main() {
 			fmt.Println(err)
 			return
 		}
+		// flag stops parsing at the first non-flag argument, so the `-getApiToken true`
+		// form drops every flag written after it. Say so instead of acting on a default.
+		if rest := settingCmd.Args(); len(rest) > 0 {
+			fmt.Printf("warning: ignored %q and any flags after it; put flags before positional arguments\n", strings.Join(rest, " "))
+		}
 		if reset {
 			if err = resetSetting(); err != nil {
 				return
@@ -710,7 +732,7 @@ func main() {
 			GetCertificate(getCert)
 		}
 		if getApiToken {
-			GetApiToken(getApiToken)
+			GetApiToken(getApiToken, tokenName)
 		}
 		if (tgbottoken != "") || (tgbotchatid != "") || (tgbotRuntime != "") {
 			updateTgbotSetting(tgbottoken, tgbotchatid, tgbotRuntime)