diff --git a/cmd/mcpproxy/datadir_flag_test.go b/cmd/mcpproxy/datadir_flag_test.go index 3c19092e..9295d233 100644 --- a/cmd/mcpproxy/datadir_flag_test.go +++ b/cmd/mcpproxy/datadir_flag_test.go @@ -29,6 +29,7 @@ func TestLoadersHonorGlobalDataDirFlag(t *testing.T) { {"call", func(p string) { callConfigPath = p }, loadCallConfig}, {"code", func(p string) { codeConfigPath = p }, loadCodeConfig}, {"tools", func(p string) { configPath = p }, loadToolsConfig}, + {"token", func(p string) { tokenConfigPath = p }, loadTokenConfig}, } for _, tc := range cases { diff --git a/cmd/mcpproxy/token_cmd.go b/cmd/mcpproxy/token_cmd.go index 11b971d1..97106caf 100644 --- a/cmd/mcpproxy/token_cmd.go +++ b/cmd/mcpproxy/token_cmd.go @@ -23,6 +23,9 @@ var ( tokenPermissions string tokenExpires string tokenProfilePin string + + // tokenConfigPath is the token command's --config override (GH #897). + tokenConfigPath string ) // GetTokenCommand returns the token parent command. @@ -43,6 +46,8 @@ Examples: mcpproxy token revoke deploy-bot`, } + tokenCmd.PersistentFlags().StringVarP(&tokenConfigPath, "config", "c", "", "Path to configuration file") + // Subcommands tokenCmd.AddCommand(newTokenCreateCmd()) tokenCmd.AddCommand(newTokenListCmd()) @@ -122,9 +127,37 @@ Examples: } } +// loadTokenConfig loads the token command's config, honoring the --config and +// global --data-dir flags (GH #897, same class as #854). Without the DataDir +// override, socket.DetectSocketPath probes the default data dir and the CLI +// either reports "daemon is not reachable" or silently talks to the wrong +// daemon instance. +func loadTokenConfig() (*config.Config, error) { + if tokenConfigPath != "" { + cfg, err := config.LoadFromFile(tokenConfigPath) + if err != nil { + return nil, err + } + // Respect global --data-dir flag + if dataDir != "" { + cfg.DataDir = dataDir + } + return cfg, nil + } + cfg, err := config.Load() + if err != nil { + return nil, err + } + // Respect global --data-dir flag + if dataDir != "" { + cfg.DataDir = dataDir + } + return cfg, nil +} + // newTokenCLIClient creates a cliclient.Client connected to the running MCPProxy. func newTokenCLIClient() (*cliclient.Client, *config.Config, error) { - cfg, err := config.Load() + cfg, err := loadTokenConfig() if err != nil { return nil, nil, fmt.Errorf("failed to load config: %w", err) } @@ -185,9 +218,9 @@ func runTokenCreate(_ *cobra.Command, _ []string) error { return parseAPIError(respBody, resp.StatusCode, "create token") } - var result map[string]interface{} - if err := json.Unmarshal(respBody, &result); err != nil { - return fmt.Errorf("failed to parse response: %w", err) + result, err := parseTokenAPIResponse(respBody) + if err != nil { + return err } // Format output @@ -242,9 +275,9 @@ func runTokenList(_ *cobra.Command, _ []string) error { return parseAPIError(respBody, resp.StatusCode, "list tokens") } - var result map[string]interface{} - if err := json.Unmarshal(respBody, &result); err != nil { - return fmt.Errorf("failed to parse response: %w", err) + result, err := parseTokenAPIResponse(respBody) + if err != nil { + return err } format := ResolveOutputFormat() @@ -327,9 +360,9 @@ func runTokenShow(_ *cobra.Command, args []string) error { return parseAPIError(respBody, resp.StatusCode, "get token") } - var result map[string]interface{} - if err := json.Unmarshal(respBody, &result); err != nil { - return fmt.Errorf("failed to parse response: %w", err) + result, err := parseTokenAPIResponse(respBody) + if err != nil { + return err } format := ResolveOutputFormat() @@ -484,9 +517,9 @@ func runTokenRegenerate(_ *cobra.Command, args []string) error { return parseAPIError(respBody, resp.StatusCode, "regenerate token") } - var result map[string]interface{} - if err := json.Unmarshal(respBody, &result); err != nil { - return fmt.Errorf("failed to parse response: %w", err) + result, err := parseTokenAPIResponse(respBody) + if err != nil { + return err } format := ResolveOutputFormat() @@ -523,6 +556,23 @@ func splitAndTrim(s string) []string { return result } +// parseTokenAPIResponse unmarshals a token REST response and unwraps the +// standard {"success":true,"data":{...}} envelope (contracts.APIResponse). +// The CLI table paths read fields like "token"/"tokens" at the top level, so +// without unwrapping, `token list` always printed "No agent tokens configured" +// and `token create` never displayed the minted token (found verifying #897). +// A body without the envelope is passed through unchanged. +func parseTokenAPIResponse(body []byte) (map[string]interface{}, error) { + var result map[string]interface{} + if err := json.Unmarshal(body, &result); err != nil { + return nil, fmt.Errorf("failed to parse response: %w", err) + } + if data, ok := result["data"].(map[string]interface{}); ok { + return data, nil + } + return result, nil +} + func parseAPIError(body []byte, statusCode int, operation string) error { var errResp map[string]interface{} if err := json.Unmarshal(body, &errResp); err == nil { diff --git a/cmd/mcpproxy/token_cmd_test.go b/cmd/mcpproxy/token_cmd_test.go index 660496d0..5be47e12 100644 --- a/cmd/mcpproxy/token_cmd_test.go +++ b/cmd/mcpproxy/token_cmd_test.go @@ -1,10 +1,19 @@ package main import ( + "encoding/json" + "fmt" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" "testing" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/contracts" ) func TestGetTokenCommand(t *testing.T) { @@ -134,3 +143,160 @@ func TestTokenCreateCmd_RequiredFlags(t *testing.T) { assert.NotNil(t, expiresFlag, "should have --expires flag") assert.Equal(t, "30d", expiresFlag.DefValue, "default expires should be 30d") } + +// GH #897 verification follow-up: the REST API wraps token responses in the +// standard envelope {"success":true,"data":{...}}, but the CLI table paths +// read top-level keys — so `token list` always printed "No agent tokens +// configured" and `token create` never displayed the minted token. +// parseTokenAPIResponse must unwrap the envelope (and tolerate the bare +// legacy shape). +func TestParseTokenAPIResponse(t *testing.T) { + t.Run("unwraps success envelope", func(t *testing.T) { + body := []byte(`{"success":true,"data":{"tokens":[{"name":"a"}],"token":"mcp_agt_x"}}`) + result, err := parseTokenAPIResponse(body) + assert.NoError(t, err) + assert.Equal(t, "mcp_agt_x", result["token"]) + tokens, ok := result["tokens"].([]interface{}) + assert.True(t, ok) + assert.Len(t, tokens, 1) + }) + + t.Run("passes through bare legacy shape", func(t *testing.T) { + body := []byte(`{"tokens":[],"token":"mcp_agt_y"}`) + result, err := parseTokenAPIResponse(body) + assert.NoError(t, err) + assert.Equal(t, "mcp_agt_y", result["token"]) + }) + + t.Run("invalid json errors", func(t *testing.T) { + _, err := parseTokenAPIResponse([]byte("not json")) + assert.Error(t, err) + }) +} + +// --- GH #897 / PR #907 handler-level regression tests --- +// +// These drive the real run* handlers against an httptest daemon (via the +// MCPPROXY_TRAY_ENDPOINT seam in daemonEndpoint) with responses marshaled +// through contracts.NewSuccessResponse — the exact envelope the httpapi +// handlers emit — so CLI/handler drift like the pre-#907 "No agent tokens +// configured" bug cannot regress silently. They mutate package globals, so +// no t.Parallel. + +func newTokenTestDaemon(t *testing.T, tokensHandler http.HandlerFunc) string { + t.Helper() + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/status", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]interface{}{"success": true, "data": map[string]interface{}{"running": true}}) + }) + mux.HandleFunc("/api/v1/tokens", tokensHandler) + server := httptest.NewServer(mux) + t.Cleanup(server.Close) + + tmp := t.TempDir() + cfgPath := filepath.Join(tmp, "cfg.json") + cfgJSON := fmt.Sprintf(`{"listen":"127.0.0.1:0","data_dir":%q,"api_key":"test-key","mcpServers":[]}`, filepath.Join(tmp, "data")) + if err := os.WriteFile(cfgPath, []byte(cfgJSON), 0o600); err != nil { + t.Fatal(err) + } + + t.Setenv("MCPPROXY_TRAY_ENDPOINT", server.URL) + t.Setenv("MCPPROXY_OUTPUT", "") + oldCfgPath := tokenConfigPath + tokenConfigPath = cfgPath + t.Cleanup(func() { tokenConfigPath = oldCfgPath }) + return server.URL +} + +func captureTokenStdout(t *testing.T, fn func() error) string { + t.Helper() + old := os.Stdout + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + os.Stdout = w + fnErr := fn() + _ = w.Close() + os.Stdout = old + out, _ := io.ReadAll(r) + if fnErr != nil { + t.Fatalf("handler returned error: %v\noutput so far:\n%s", fnErr, out) + } + return string(out) +} + +func TestRunTokenList_TableOutput(t *testing.T) { + newTokenTestDaemon(t, func(w http.ResponseWriter, _ *http.Request) { + resp := contracts.NewSuccessResponse(map[string]interface{}{ + "tokens": []map[string]interface{}{{ + "name": "qa-list-token", "token_prefix": "mcp_agt_ab12", + "allowed_servers": []string{"*"}, "permissions": []string{"read"}, + "revoked": false, "expires_at": "2026-08-23T00:00:00Z", "created_at": "2026-07-24T00:00:00Z", + }}, + }) + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(resp) + }) + + out := captureTokenStdout(t, func() error { return runTokenList(nil, nil) }) + assert.Contains(t, out, "qa-list-token", "table must list the token") + assert.Contains(t, out, "mcp_agt_ab12") + assert.NotContains(t, out, "No agent tokens configured", "pre-#907 envelope bug must not regress") +} + +func TestRunTokenCreate_DisplaysMintedToken(t *testing.T) { + const minted = "mcp_agt_deadbeefcafe0123456789" + newTokenTestDaemon(t, func(w http.ResponseWriter, _ *http.Request) { + resp := contracts.NewSuccessResponse(map[string]interface{}{ + "name": "qa-create", "token": minted, + "allowed_servers": []string{"*"}, "permissions": []string{"read"}, + "expires_at": "2026-08-23T00:00:00Z", + }) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(resp) + }) + + oldName, oldServers, oldPerms, oldExpires := tokenName, tokenServers, tokenPermissions, tokenExpires + tokenName, tokenServers, tokenPermissions, tokenExpires = "qa-create", "*", "read", "30d" + defer func() { + tokenName, tokenServers, tokenPermissions, tokenExpires = oldName, oldServers, oldPerms, oldExpires + }() + + out := captureTokenStdout(t, func() error { return runTokenCreate(nil, nil) }) + assert.Contains(t, out, minted, "create must display the minted token — it is shown only once") +} + +// GH #897: the config.Load() branch of loadTokenConfig (no --config flag) is +// the exact reported user flow. Sandbox HOME so config.Load touches only a +// temp dir, then assert the global --data-dir flag overrides the default. +func TestLoadTokenConfig_LoadBranchHonorsDataDir(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) // Windows os.UserHomeDir + t.Setenv("MCPPROXY_API_KEY", "") + + oldCfgPath, oldDataDir := tokenConfigPath, dataDir + tokenConfigPath = "" + flagDir := filepath.Join(home, "flag-data") + dataDir = flagDir + defer func() { tokenConfigPath, dataDir = oldCfgPath, oldDataDir }() + + cfg, err := loadTokenConfig() + if err != nil { + t.Fatalf("loadTokenConfig via config.Load(): %v", err) + } + if cfg.DataDir != flagDir { + t.Errorf("config.Load() branch DataDir = %q, want --data-dir %q (GH #897 regression)", cfg.DataDir, flagDir) + } +} + +func TestTokenCommand_ConfigFlagRegistered(t *testing.T) { + cmd := GetTokenCommand() + flag := cmd.PersistentFlags().Lookup("config") + if assert.NotNil(t, flag, "token command must have persistent --config flag") { + assert.Equal(t, "c", flag.Shorthand) + } +}