diff --git a/cmd/mcpproxy/activity_cmd.go b/cmd/mcpproxy/activity_cmd.go index a3aca61e0..18d398579 100644 --- a/cmd/mcpproxy/activity_cmd.go +++ b/cmd/mcpproxy/activity_cmd.go @@ -454,7 +454,7 @@ func activityPeriodFromRelative(from string) (string, error) { case "-30d": return "30d", nil default: - return "", fmt.Errorf("--from on 'activity summary' accepts only -1h, -24h, -7d or -30d (aliases of --period); got '%s'", from) + return "", errors.New("summary supports --from -1h|-24h|-7d|-30d only") } } @@ -1585,6 +1585,11 @@ func runActivityWatch(cmd *cobra.Command, _ []string) error { if err := validateActivityWatchView(); err != nil { return outputActivityError(err, "INVALID_FILTER") } + // Conflicting scope flags (--token A --agent B) would otherwise make + // activityWatchScopeMatches reject every event and stream nothing forever. + if _, err := resolveActivityScope(); err != nil { + return outputActivityError(err, "INVALID_FILTER") + } // Setup logger cmdLogLevel, _ := cmd.Flags().GetString("log-level") diff --git a/cmd/mcpproxy/b5_cli_contract_test.go b/cmd/mcpproxy/b5_cli_contract_test.go new file mode 100644 index 000000000..fbaf7ac41 --- /dev/null +++ b/cmd/mcpproxy/b5_cli_contract_test.go @@ -0,0 +1,135 @@ +package main + +import ( + "bytes" + "errors" + "os" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + clioutput "github.com/smart-mcp-proxy/mcpproxy-go/internal/cli/output" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/connect" +) + +// 1451-3: conflicting --token/--agent must fail up front for watch. +func TestRunActivityWatch_ConflictingScopeFlagsFailFast(t *testing.T) { + resetActivityScopeFlags(t) + prevView, prevType := activityView, activityType + t.Cleanup(func() { activityView, activityType = prevView, prevType }) + activityView, activityType = "all", "" + activityToken, activityAgent = "A", "B" + setOutputGlobals(t, "table", false) + + err := runActivityWatch(activityWatchCmd, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "must name the same token") +} + +// 1451-8: the confirmation prompt never touches stdout. +func TestReadConfirmation_PromptGoesToWriter(t *testing.T) { + var out bytes.Buffer + ok, err := readConfirmation(strings.NewReader("y\n"), &out, "Revoke?") + require.NoError(t, err) + assert.True(t, ok) + assert.Equal(t, "Revoke? [y/N]: ", out.String()) +} + +// 1451-9: a REST refusal exits 1 even when its text mentions config. +func TestParseAPIError_RefusalsExitOne(t *testing.T) { + for _, body := range []string{ + `{"error":"invalid config value for scope"}`, + `{"error":"config error: server missing","field":"servers"}`, + `not json: config error invalid`, + } { + err := parseAPIError([]byte(body), 400, "create token") + require.Error(t, err) + assert.Equal(t, ExitCodeGeneralError, classifyError(err), body) + } + assert.Equal(t, ExitCodeConfigError, classifyError(errors.New("invalid configuration: x"))) +} + +// 1435-3b: a refused connect exits non-zero in every format, stdout unchanged. +func TestPrintConnectResult_FailureReturnsError(t *testing.T) { + result := &connect.ConnectResult{Success: false, Client: "cursor", Action: "failed", Message: "cursor config write failed"} + for _, format := range []string{"table", "json", "yaml"} { + formatter, err := clioutput.NewFormatter(format) + require.NoError(t, err) + var perr error + out := captureStdout(t, func() { perr = printConnectResult(result, formatter, format) }) + require.Error(t, perr, format) + assert.Equal(t, ExitCodeGeneralError, classifyError(perr)) + assert.Contains(t, out, "cursor config write failed", format) + } + dup := &connect.ConnectResult{Success: false, Client: "cursor", Action: "already_exists", Message: "dup"} + jf, _ := clioutput.NewFormatter("json") + captureStdout(t, func() { require.NoError(t, printConnectResult(dup, jf, "json")) }) + ok := &connect.ConnectResult{Success: true, Client: "cursor", Message: "ok", ConfigPath: "/x"} + formatter, _ := clioutput.NewFormatter("json") + captureStdout(t, func() { require.NoError(t, printConnectResult(ok, formatter, "json")) }) +} + +// 1466-12 / 1466-27 live in review tests; wording for --all: +func TestReviewApproveWording_All(t *testing.T) { + _, summary := reviewApproveWording("m", 9, 9, nil, true) + assert.Equal(t, "Allowing all pending or changed tools; previously blocked tools stay blocked", summary) + _, summary = reviewApproveWording("m", 9, 9, nil, false) + assert.Equal(t, "Allowing 9 of 9 tools; blocking none", summary) +} + +func TestReviewScanLine_EmptyCoverageIsNone(t *testing.T) { + assert.Equal(t, "Scan: none", reviewScanLine(map[string]interface{}{"name": "n", "scan": map[string]interface{}{"verdict": "clean"}})) +} + +// 1466-15: an explicit, unloadable --config is an error, not a silent default. +func TestLoadRegistryConfig_MissingExplicitPathErrors(t *testing.T) { + prev := registryConfigPath + t.Cleanup(func() { registryConfigPath = prev }) + registryConfigPath = t.TempDir() + "/nope.json" + _, err := loadRegistryConfig() + require.Error(t, err) +} + +// 1466-16: --data-dir with no config still gets the MCPPROXY_LISTEN overlay. +func TestLoadCLIConfig_DataDirOnlyAppliesEnvOverlay(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + t.Setenv("MCPPROXY_LISTEN", "127.0.0.1:18999") + prev := dataDir + t.Cleanup(func() { dataDir = prev }) + dataDir = t.TempDir() + cfg, err := loadCLIConfig("") + require.NoError(t, err) + assert.Equal(t, "127.0.0.1:18999", cfg.Listen) +} + +// 1466-17: an apostrophe inside a URL must not end the redaction early. +func TestRedactDoctorString_ApostropheInURL(t *testing.T) { + got := redactDoctorString("see https://h.example/x?label=Bob's&%74oken=sekret-1 ok") + assert.NotContains(t, got, "sekret-1") + quoted := redactDoctorString("dial 'https://h.example/x?token=sekret-2' failed") + assert.NotContains(t, quoted, "sekret-2") + assert.Contains(t, quoted, "REDACTED'") +} + +// 1394-5: FR-075 verbatim message. +func TestActivityPeriodFromRelative_FR075Message(t *testing.T) { + _, err := activityPeriodFromRelative("-3d") + require.Error(t, err) + assert.Equal(t, "summary supports --from -1h|-24h|-7d|-30d only", err.Error()) +} + +// Disconnecting a never-registered client is a no-op and must exit 0. +func TestConnectResultError_NotFoundDisconnectExitsZero(t *testing.T) { + nf := &connect.ConnectResult{Success: false, Client: "cursor", Action: "not_found", Message: "no entry"} + require.NoError(t, connectResultError(nf)) + require.Error(t, connectResultError(&connect.ConnectResult{Action: "failed", Message: "x"})) +} + +// The confirmation prompt must default to stderr, never stdout. +func TestConfirmPromptOut_DefaultsToStderr(t *testing.T) { + assert.Same(t, os.Stderr, confirmPromptOut) +} diff --git a/cmd/mcpproxy/cli_config.go b/cmd/mcpproxy/cli_config.go index 6008a661e..096d03d2c 100644 --- a/cmd/mcpproxy/cli_config.go +++ b/cmd/mcpproxy/cli_config.go @@ -33,6 +33,7 @@ func loadCLIConfig(explicitPath string) (*config.Config, error) { // $HOME/.mcpproxy/mcp_config.json and report the HOME defaults, which // contradicts the data dir the operator named. cfg = config.DefaultConfig() + config.ApplyEnvOverrides(cfg) } else { cfg, err = config.Load() } diff --git a/cmd/mcpproxy/connect_cmd.go b/cmd/mcpproxy/connect_cmd.go index a199b1315..4b025f54a 100644 --- a/cmd/mcpproxy/connect_cmd.go +++ b/cmd/mcpproxy/connect_cmd.go @@ -343,7 +343,7 @@ func printConnectResult(result *connect.ConnectResult, formatter clioutput.Outpu } else { fmt.Printf("Failed: %s\n", result.Message) } - return nil + return connectResultError(result) } // JSON/YAML @@ -352,7 +352,20 @@ func printConnectResult(result *connect.ConnectResult, formatter clioutput.Outpu return err } fmt.Println(out) - return nil + return connectResultError(result) +} + +// connectResultError turns a refused connect/disconnect result into a non-nil +// error so the exit code is 1; stdout is already printed unchanged. nil for a +// successful result or an already_exists no-op. +func connectResultError(result *connect.ConnectResult) error { + // already_exists (idempotent connect) and not_found (disconnecting a client + // that was never registered) are deliberate "result, not an error" no-ops, + // so they keep exiting 0. + if result == nil || result.Success || result.Action == "already_exists" || result.Action == "not_found" { + return nil + } + return cliRefusalError{fmt.Errorf("%s", result.Message)} } func loadConnectConfig() (*config.Config, error) { diff --git a/cmd/mcpproxy/connect_hint_test.go b/cmd/mcpproxy/connect_hint_test.go index 9e5a7e699..9d0e8e111 100644 --- a/cmd/mcpproxy/connect_hint_test.go +++ b/cmd/mcpproxy/connect_hint_test.go @@ -105,6 +105,7 @@ func TestPrintConnectResult_FailureHasNoNextLine(t *testing.T) { } out := captureStdout(t, func() { + // already_exists stays a result (exit 0); other failures exit 1 (#1435). if err := printConnectResult(result, formatter, "table"); err != nil { t.Errorf("printConnectResult: %v", err) } diff --git a/cmd/mcpproxy/daemon_rest.go b/cmd/mcpproxy/daemon_rest.go index ca42ef3a7..b2129dd82 100644 --- a/cmd/mcpproxy/daemon_rest.go +++ b/cmd/mcpproxy/daemon_rest.go @@ -405,6 +405,10 @@ func warningFix(w runtime.Warning) string { return "" } +// confirmPromptOut is where the confirmation prompt is written (stderr, so +// `-o json` stdout stays pure JSON). A var so a test can pin the default. +var confirmPromptOut io.Writer = os.Stderr + // confirmYes asks for confirmation on a terminal, or accepts --yes. A non-TTY // stdin without --yes is refused (exit 1) so a script never proceeds blind. func confirmYes(prompt string, yes bool) (bool, error) { @@ -414,8 +418,14 @@ func confirmYes(prompt string, yes bool) (bool, error) { if !term.IsTerminal(int(os.Stdin.Fd())) { return false, flagValidationError{errors.New("confirmation required; pass --yes")} } - fmt.Printf("%s [y/N]: ", prompt) - answer, err := bufio.NewReader(os.Stdin).ReadString('\n') + // The prompt goes to stderr so `-o json` stdout stays pure JSON. + return readConfirmation(os.Stdin, confirmPromptOut, prompt) +} + +// readConfirmation writes the [y/N] prompt to out and reads one answer from in. +func readConfirmation(in io.Reader, out io.Writer, prompt string) (bool, error) { + fmt.Fprintf(out, "%s [y/N]: ", prompt) + answer, err := bufio.NewReader(in).ReadString('\n') if err != nil { return false, fmt.Errorf("failed to read confirmation: %w", err) } diff --git a/cmd/mcpproxy/doctor_redact.go b/cmd/mcpproxy/doctor_redact.go index ae89d0729..af25d25cd 100644 --- a/cmd/mcpproxy/doctor_redact.go +++ b/cmd/mcpproxy/doctor_redact.go @@ -20,7 +20,10 @@ const doctorRedactionMask = "REDACTED" // in doctor output even outside a URL. runDoctor sets it for the run. var doctorSecretLiterals []string -var doctorURLPattern = regexp.MustCompile(`(?i)\b[a-z][a-z0-9+.-]*://[^\s"'<>]+`) +// An apostrophe is legal inside a URL (`?label=Bob's`), so the body only stops +// at whitespace, double quote or angle brackets; a trailing apostrophe that +// merely closes a single-quoted URL is trimmed in redactDoctorString. +var doctorURLPattern = regexp.MustCompile(`(?i)\b[a-z][a-z0-9+.-]*://[^\s"<>]+`) func doctorMask(string) string { return doctorRedactionMask } @@ -32,7 +35,11 @@ func redactDoctorString(s string) string { } if strings.Contains(s, "://") { s = doctorURLPattern.ReplaceAllStringFunc(s, func(u string) string { - return oauth.RedactURLQueryParamsWith(u, doctorMask) + trail := "" + if strings.HasSuffix(u, "'") { + u, trail = strings.TrimSuffix(u, "'"), "'" + } + return oauth.RedactURLQueryParamsWith(u, doctorMask) + trail }) } for _, lit := range doctorSecretLiterals { diff --git a/cmd/mcpproxy/main.go b/cmd/mcpproxy/main.go index 3b98b6b9a..7a00bb0fc 100644 --- a/cmd/mcpproxy/main.go +++ b/cmd/mcpproxy/main.go @@ -962,6 +962,14 @@ func applyServeRuntimeFlags(cmd *cobra.Command, cfg *config.Config) { // valid-values list happens not to contain "config"). type flagValidationError struct{ error } +// cliRefusalError marks a refusal reported by the daemon's REST API. Its text +// is the daemon's, so classifyError must not run the config/permission string +// heuristics over it (a message mentioning "config" is not a config-file +// error); it exits 1. +type cliRefusalError struct{ error } + +func (e cliRefusalError) Unwrap() error { return e.error } + func newFlagValidationError(format string, args ...any) error { return flagValidationError{fmt.Errorf(format, args...)} } @@ -976,6 +984,10 @@ func classifyError(err error) int { if errors.As(err, &flagErr) { return ExitCodeGeneralError } + var refusalErr cliRefusalError + if errors.As(err, &refusalErr) { + return ExitCodeGeneralError + } // Spec 098: a preflight verdict is a RESULT, not a failure of mcpproxy, and // it carries its own exit code (10/11/12). It is checked first so the diff --git a/cmd/mcpproxy/registry_cmd.go b/cmd/mcpproxy/registry_cmd.go index b8589d537..8b957d69c 100644 --- a/cmd/mcpproxy/registry_cmd.go +++ b/cmd/mcpproxy/registry_cmd.go @@ -627,6 +627,11 @@ func loadRegistryConfig() (*config.Config, error) { // $HOME/.mcpproxy/mcp_config.json. cfg, err := loadCLIConfig(registryConfigPath) if err != nil { + // An explicitly named config that cannot be loaded is an error, not a + // reason to silently use defaults. + if resolveCLIConfigPath(registryConfigPath) != "" { + return nil, err + } // Discovery should still work with defaults if no config is present. cfg = config.DefaultConfig() if dataDir != "" { diff --git a/cmd/mcpproxy/review_cmd.go b/cmd/mcpproxy/review_cmd.go index 568390647..ab6a79a95 100644 --- a/cmd/mcpproxy/review_cmd.go +++ b/cmd/mcpproxy/review_cmd.go @@ -72,7 +72,7 @@ func newReviewCommand(confirm func(string) (bool, error)) *cobra.Command { if len(block) > 0 { body["block"] = block } - prompt, summary = reviewApproveWording(server, len(state.tools), allowed, block) + prompt, summary = reviewApproveWording(server, len(state.tools), allowed, block, all) } else if len(tools) > 0 || len(except) > 0 { // Nothing to select from: dropping --tools/--except would approve blind. return fmt.Errorf("no tool definitions captured for server '%s'; fetch them first (mcpproxy review show %s) before using --tools or --except", server, server) @@ -277,13 +277,19 @@ func reviewApproveSelection(tools []reviewToolState, all bool, only, except []st // reviewApproveWording builds the confirmation prompt and the table-mode // summary line, both naming the exact count. -func reviewApproveWording(server string, total, allowed int, block []string) (prompt, summary string) { +func reviewApproveWording(server string, total, allowed int, block []string, all bool) (prompt, summary string) { noun := func(n int) string { if n == 1 { return "tool" } return "tools" } + if len(block) == 0 && all { + // --all approves pending or changed tools only; tools blocked earlier + // stay blocked, so "N of N; blocking none" would overstate it. + return fmt.Sprintf("Approve server '%s' with all %d %s?", server, total, noun(total)), + "Allowing all pending or changed tools; previously blocked tools stay blocked" + } if len(block) == 0 { return fmt.Sprintf("Approve server '%s' with all %d %s?", server, total, noun(total)), fmt.Sprintf("Allowing %d of %d %s; blocking none", allowed, total, noun(total)) @@ -371,10 +377,14 @@ func formatReviewResponse(format string, raw []byte, full bool) error { // as the Web and macOS review screens. It returns "" when the payload carries // no scan or predates coverage. func reviewScanLine(server map[string]interface{}) string { - scan, _ := server["scan"].(map[string]interface{}) + scan, hasScan := server["scan"].(map[string]interface{}) + if !hasScan { + return "" + } coverage, _ := scan["coverage"].(string) if coverage == "" { - return "" + // An older core sends no coverage: say "none" like the Web and macOS screens. + return "Scan: none" } rescan := "run: mcpproxy security rescan " + fmt.Sprint(server["name"]) switch coverage { diff --git a/cmd/mcpproxy/review_cmd_test.go b/cmd/mcpproxy/review_cmd_test.go index fe7c7eba8..b4957df2d 100644 --- a/cmd/mcpproxy/review_cmd_test.go +++ b/cmd/mcpproxy/review_cmd_test.go @@ -263,9 +263,9 @@ func TestFormatReviewShowPrintsScanCoverage(t *testing.T) { "Scan: not scanned yet; run: mcpproxy security rescan notes") require.Contains(t, show(`,"scan":{"verdict":"not_scanned","coverage":"scanning"}`), "Scan: in progress") - // No scan key (or a daemon that predates coverage): no Scan line at all. + // No scan object: no line. A scan without coverage (older core): "none", as on Web and macOS. require.NotContains(t, show(``), "Scan:") - require.NotContains(t, show(`,"scan":{"verdict":"clean"}`), "Scan:") + require.Contains(t, show(`,"scan":{"verdict":"clean"}`), "Scan: none") } func captureReviewOutput(t *testing.T, fn func() error) string { diff --git a/cmd/mcpproxy/testdata/cli109/review-approve-all.golden b/cmd/mcpproxy/testdata/cli109/review-approve-all.golden index e9be135f4..bf396ee64 100644 --- a/cmd/mcpproxy/testdata/cli109/review-approve-all.golden +++ b/cmd/mcpproxy/testdata/cli109/review-approve-all.golden @@ -1,2 +1,2 @@ -Allowing 9 of 9 tools; blocking none +Allowing all pending or changed tools; previously blocked tools stay blocked Approved server memory diff --git a/cmd/mcpproxy/token_cmd.go b/cmd/mcpproxy/token_cmd.go index b7eec49bf..50b8f5334 100644 --- a/cmd/mcpproxy/token_cmd.go +++ b/cmd/mcpproxy/token_cmd.go @@ -627,10 +627,10 @@ func parseAPIError(body []byte, statusCode int, operation string) error { // other flag-validation failure. return flagValidationError{fmt.Errorf("failed to %s: %s (field: %s)", operation, errMsg, field)} } - return fmt.Errorf("failed to %s: %s", operation, errMsg) + return cliRefusalError{fmt.Errorf("failed to %s: %s", operation, errMsg)} } } - return fmt.Errorf("failed to %s: HTTP %d: %s", operation, statusCode, string(body)) + return cliRefusalError{fmt.Errorf("failed to %s: HTTP %d: %s", operation, statusCode, string(body))} } func getMapString(m map[string]interface{}, key string) string { diff --git a/docs/cli/review-commands.md b/docs/cli/review-commands.md index cd6a96c8d..38426f76d 100644 --- a/docs/cli/review-commands.md +++ b/docs/cli/review-commands.md @@ -93,7 +93,7 @@ Using the wrong flag for the state fails with exit code 1 instead of doing somet - `--tools` or `--except` while no tool definitions are captured: `no tool definitions captured for server 's'; fetch them first ...`; nothing is written - `--except` on a trusted server: `--except applies only while approving a quarantined server` -The confirmation prompt reads the review first and names the exact count, for example `Approve server 'memory' with 3 of 9 tools? Blocked: a, b, c.`, `Approve server 'memory' with all 9 tools?` or `Approve server 'memory' without seeing tools?` when nothing is captured. In table output one line precedes the result: `Allowing 3 of 9 tools; blocking 6: a, b, ...`. JSON and YAML output stay the REST data object. +The confirmation prompt reads the review first and names the exact count, for example `Approve server 'memory' with 3 of 9 tools? Blocked: a, b, c.`, `Approve server 'memory' with all 9 tools?` or `Approve server 'memory' without seeing tools?` when nothing is captured. In table output one line precedes the result: `Allowing 3 of 9 tools; blocking 6: a, b, ...`; with `--all` it reads `Allowing all pending or changed tools; previously blocked tools stay blocked`. JSON and YAML output stay the REST data object. `mcpproxy review approve --yes` used to allow every tool. It now allows the default selection, so it matches the review screens; add `--all` to approve every tool. diff --git a/internal/config/loader.go b/internal/config/loader.go index e3d23014f..6ec4dd50d 100644 --- a/internal/config/loader.go +++ b/internal/config/loader.go @@ -743,6 +743,11 @@ func expandDataDir(cfg *Config) { cfg.DataDir = resolved } +// ApplyEnvOverrides applies the MCPPROXY_* environment overlay (listen, TLS, +// data dir, trusted hosts...) that Load applies, for callers that build a +// config from DefaultConfig without reading a file. +func ApplyEnvOverrides(cfg *Config) { applyTLSEnvOverrides(cfg) } + // applyTLSEnvOverrides applies the MCPPROXY_* environment overrides. Each one // goes through OverrideForProcess so no save path persists it (see // process_overrides.go); the env-sourced set is rebuilt from scratch on every