From 0a404eb33d32536be00d4dbd67c6f05305cd8206 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 2 Oct 2026 22:31:32 +0300 Subject: [PATCH 1/6] feat(review): core computes the fail-closed default selection; CLI approve uses it (Spec fix-review-defaults) ReviewTool.default_allowed is true only for an approved enabled tool or a pending/changed read tool with a clean current scan that is not held. mcpproxy review approve blocks every other tool unless --all or --tools is given, and its prompt names the exact count. --- cmd/mcpproxy/review_cmd.go | 171 +++++++++-- .../review_cmd_default_selection_test.go | 279 ++++++++++++++++++ cmd/mcpproxy/review_cmd_test.go | 10 +- .../testdata/cli109/review-approve-all.golden | 2 + .../cli109/review-approve-default.golden | 2 + .../testdata/cli109/review-approve.golden | 1 + internal/runtime/review.go | 24 +- .../runtime/review_default_allowed_test.go | 160 ++++++++++ internal/server/e2e_test.go | 7 + internal/server/mcp.go | 1 + .../quarantine_inspect_review_parity_test.go | 58 ++++ 11 files changed, 681 insertions(+), 34 deletions(-) create mode 100644 cmd/mcpproxy/review_cmd_default_selection_test.go create mode 100644 cmd/mcpproxy/testdata/cli109/review-approve-all.golden create mode 100644 cmd/mcpproxy/testdata/cli109/review-approve-default.golden create mode 100644 internal/runtime/review_default_allowed_test.go diff --git a/cmd/mcpproxy/review_cmd.go b/cmd/mcpproxy/review_cmd.go index 180e03821..9c2e22abd 100644 --- a/cmd/mcpproxy/review_cmd.go +++ b/cmd/mcpproxy/review_cmd.go @@ -25,6 +25,7 @@ func newReviewCommand(confirm func(string) (bool, error)) *cobra.Command { var tools []string var force bool var yes bool + var all bool cmd := &cobra.Command{Use: "review", Short: "Inspect and decide quarantined server reviews"} cmd.AddCommand(&cobra.Command{Use: "list", Short: "List servers needing review", RunE: func(_ *cobra.Command, _ []string) error { return runReviewRead("/api/v1/review") @@ -36,40 +37,56 @@ func newReviewCommand(confirm func(string) (bool, error)) *cobra.Command { show.Flags().Bool("full", false, "Show full captured schemas and descriptions") cmd.AddCommand(show) approve := &cobra.Command{Use: "approve ", Short: "Approve a server through the scan gate", Args: cobra.ExactArgs(1), RunE: func(_ *cobra.Command, args []string) error { - if !yes { - confirmed, err := confirm(fmt.Sprintf("Approve review for server '%s'?", args[0])) - if err != nil { - return err - } - if !confirmed { - fmt.Println("Approval cancelled.") - return nil - } + server := args[0] + if all && len(tools) > 0 { + return errReviewAllWithTools } - quarantined, err := reviewServerQuarantined(args[0]) + state, err := reviewServerState(server) if err != nil { return err } - if quarantined { + if !state.quarantined { + if len(except) > 0 { + return fmt.Errorf("--except applies only while approving a quarantined server") + } + if !yes { + confirmed, err := confirm(fmt.Sprintf("Approve review for server '%s'?", server)) + if err != nil || !confirmed { + return reviewDeclined("Approval cancelled.", err) + } + } + body := map[string]interface{}{"approve_all": true} if len(tools) > 0 { - return fmt.Errorf("--tools cannot select a quarantined server approval; use --except to block tools") + body = map[string]interface{}{"tools": tools} } - body := map[string]interface{}{"force": force} - if len(except) > 0 { - body["block"] = except + return runReviewWrite(server, "tools/approve", body) + } + body := map[string]interface{}{"force": force} + prompt := fmt.Sprintf("Approve server '%s' without seeing tools?", server) + var summary string + if len(state.tools) > 0 { + block, allowed, err := reviewApproveSelection(state.tools, all, tools, except) + if err != nil { + return fmt.Errorf("%w for server '%s'", err, server) + } + if len(block) > 0 { + body["block"] = block } - return runReviewWrite(args[0], "security/approve", body) + prompt, summary = reviewApproveWording(server, len(state.tools), allowed, block) } - if len(except) > 0 { - return fmt.Errorf("--except applies only while approving a quarantined server") + if !yes { + confirmed, err := confirm(prompt) + if err != nil || !confirmed { + return reviewDeclined("Approval cancelled.", err) + } } - body := map[string]interface{}{"approve_all": true} - if len(tools) > 0 { - body = map[string]interface{}{"tools": tools} + if summary != "" && ResolveOutputFormat() == "table" { + fmt.Println(summary) } - return runReviewWrite(args[0], "tools/approve", body) + return runReviewWrite(server, "security/approve", body) }} - approve.Flags().StringSliceVar(&tools, "tools", nil, "Approve only these pending or changed tools on a trusted server") + approve.Flags().BoolVar(&all, "all", false, "Approve every tool (default: only read-only tools with a clean scan, as on the Web and macOS review screens)") + approve.Flags().StringSliceVar(&tools, "tools", nil, "Approve exactly these tools and block the rest (trusted server: only these pending or changed tools)") approve.Flags().StringSliceVar(&except, "except", nil, "Block these tools while approving the server") approve.Flags().BoolVar(&force, "force", false, "Force approval when the scan verdict is dangerous") approve.Flags().BoolVar(&yes, "yes", false, "Confirm the approval") @@ -147,36 +164,130 @@ func runReviewWrite(server, operation string, value interface{}) error { return formatReviewResponse(ResolveOutputFormat(), response, false) } -func reviewServerQuarantined(server string) (bool, error) { +var errReviewAllWithTools = fmt.Errorf("--all cannot be combined with --tools") + +// reviewDeclined reports a declined confirmation as a clean exit. +func reviewDeclined(message string, err error) error { + if err != nil { + return err + } + fmt.Println(message) + return nil +} + +// reviewToolState is the part of a review tool the approval selection needs. +// DefaultAllowed is nil when the core predates the field, which counts as +// false: a mismatched core fails closed (D41.1). +type reviewToolState struct { + Name string `json:"name"` + Tier string `json:"tier"` + DefaultAllowed *bool `json:"default_allowed"` +} + +type reviewServerStateResult struct { + quarantined bool + tools []reviewToolState +} + +func reviewServerState(server string) (reviewServerStateResult, error) { client, _, err := newSecurityCLIClient() if err != nil { - return false, err + return reviewServerStateResult{}, err } ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() resp, err := client.DoRaw(ctx, http.MethodGet, "/api/v1/servers/"+url.PathEscape(server)+"/review", nil) if err != nil { - return false, err + return reviewServerStateResult{}, err } defer resp.Body.Close() body, err := io.ReadAll(resp.Body) if err != nil { - return false, err + return reviewServerStateResult{}, err } if resp.StatusCode != http.StatusOK { - return false, parseAPIError(body, resp.StatusCode, "read review") + return reviewServerStateResult{}, parseAPIError(body, resp.StatusCode, "read review") } var envelope struct { Data struct { Server struct { Quarantined bool `json:"quarantined"` } `json:"server"` + Tools []reviewToolState `json:"tools"` } `json:"data"` } if err := json.Unmarshal(body, &envelope); err != nil { - return false, err + return reviewServerStateResult{}, err + } + return reviewServerStateResult{quarantined: envelope.Data.Server.Quarantined, tools: envelope.Data.Tools}, nil +} + +// reviewApproveSelection resolves which tools a quarantined-server approval +// allows, the same rule the Web and macOS review screens use. The base is the +// core's default selection, every tool (all) or exactly the named tools (only); +// except then subtracts. block is every other tool, in review order. A name +// that is not in the review is an error before anything is written. +func reviewApproveSelection(tools []reviewToolState, all bool, only, except []string) (block []string, allowed int, err error) { + if all && len(only) > 0 { + return nil, 0, errReviewAllWithTools + } + known := make(map[string]bool, len(tools)) + for _, tool := range tools { + known[tool.Name] = true + } + toSet := func(names []string) (map[string]bool, error) { + set := make(map[string]bool, len(names)) + for _, name := range names { + if !known[name] { + return nil, fmt.Errorf("unknown tool '%s'", name) + } + set[name] = true + } + return set, nil + } + onlySet, err := toSet(only) + if err != nil { + return nil, 0, err + } + exceptSet, err := toSet(except) + if err != nil { + return nil, 0, err + } + for _, tool := range tools { + var allow bool + switch { + case all: + allow = true + case len(only) > 0: + allow = onlySet[tool.Name] + default: + allow = tool.DefaultAllowed != nil && *tool.DefaultAllowed + } + if allow && !exceptSet[tool.Name] { + allowed++ + } else { + block = append(block, tool.Name) + } + } + return block, allowed, nil +} + +// 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) { + noun := func(n int) string { + if n == 1 { + return "tool" + } + return "tools" + } + 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)) } - return envelope.Data.Server.Quarantined, nil + list := strings.Join(block, ", ") + return fmt.Sprintf("Approve server '%s' with %d of %d %s? Blocked: %s.", server, allowed, total, noun(total), list), + fmt.Sprintf("Allowing %d of %d %s; blocking %d: %s", allowed, total, noun(total), len(block), list) } // formatReviewResponse drops the REST envelope so CLI JSON/YAML is exactly the diff --git a/cmd/mcpproxy/review_cmd_default_selection_test.go b/cmd/mcpproxy/review_cmd_default_selection_test.go new file mode 100644 index 000000000..6c3a85367 --- /dev/null +++ b/cmd/mcpproxy/review_cmd_default_selection_test.go @@ -0,0 +1,279 @@ +package main + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + + "github.com/stretchr/testify/require" +) + +// memoryReviewPayload is a quarantined server with nine tools: three read tools +// the core pre-selects (default_allowed true), one read tool from an older core +// (no default_allowed key, so it must count as false), two write tools, one +// destructive and one unannotated tool, and one more write tool. +const memoryReviewPayload = `{"success":true,"data":{"server":{"name":"memory","quarantined":true,"definitions_captured":true},"tools":[ +{"name":"read_a","tier":"read","approval_status":"pending","scan_verdict":"clean","default_allowed":true}, +{"name":"read_b","tier":"read","approval_status":"pending","scan_verdict":"clean","default_allowed":true}, +{"name":"read_c","tier":"read","approval_status":"pending","scan_verdict":"clean","default_allowed":true}, +{"name":"legacy_read","tier":"read","approval_status":"pending","scan_verdict":"clean"}, +{"name":"write_a","tier":"write","approval_status":"pending","scan_verdict":"clean","default_allowed":false}, +{"name":"write_b","tier":"write","approval_status":"pending","scan_verdict":"clean","default_allowed":false}, +{"name":"write_c","tier":"write","approval_status":"pending","scan_verdict":"clean","default_allowed":false}, +{"name":"delete_a","tier":"destructive","approval_status":"pending","scan_verdict":"clean","default_allowed":false}, +{"name":"plain_a","tier":"unannotated","approval_status":"pending","scan_verdict":"not_scanned","default_allowed":false} +]}}` + +const memoryDefaultBlock = "legacy_read, write_a, write_b, write_c, delete_a, plain_a" + +type reviewRecorder struct { + mu sync.Mutex + requests []reviewRequest +} + +func (r *reviewRecorder) add(req reviewRequest) { + r.mu.Lock() + defer r.mu.Unlock() + r.requests = append(r.requests, req) +} + +func (r *reviewRecorder) writes() []reviewRequest { + r.mu.Lock() + defer r.mu.Unlock() + var out []reviewRequest + for _, req := range r.requests { + if req.method == http.MethodPost { + out = append(out, req) + } + } + return out +} + +func newMemoryReviewDaemon(t *testing.T, recorder *reviewRecorder) { + t.Helper() + daemon := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/v1/status" { + _, _ = w.Write([]byte(`{"success":true,"data":{"running":true}}`)) + return + } + body, _ := io.ReadAll(r.Body) + recorder.add(reviewRequest{method: r.Method, path: r.URL.Path, body: string(body)}) + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/api/v1/servers/memory/review": + _, _ = w.Write([]byte(memoryReviewPayload)) + case "/api/v1/servers/bare/review": + _, _ = w.Write([]byte(`{"success":true,"data":{"server":{"name":"bare","quarantined":true,"definitions_captured":false},"tools":[]}}`)) + case "/api/v1/servers/trusted/review": + _, _ = w.Write([]byte(`{"success":true,"data":{"server":{"name":"trusted","quarantined":false},"tools":[]}}`)) + case "/api/v1/servers/memory/security/approve", "/api/v1/servers/bare/security/approve": + _, _ = w.Write([]byte(`{"success":true,"data":{"status":"approved","server_name":"memory"}}`)) + case "/api/v1/servers/trusted/tools/approve": + _, _ = w.Write([]byte(`{"success":true,"data":{"message":"Approved 1 tool for server trusted"}}`)) + default: + t.Errorf("unexpected review request %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + t.Cleanup(daemon.Close) + withReviewDaemon(t, daemon.URL) +} + +func runReviewApprove(t *testing.T, format string, confirm func(string) (bool, error), args ...string) (string, error) { + t.Helper() + setOutputGlobals(t, format, false) + var runErr error + out := captureReviewOutput(t, func() error { + cmd := newReviewCommand(confirm) + cmd.SetArgs(append([]string{"approve"}, args...)) + cmd.SilenceUsage = true + cmd.SilenceErrors = true + runErr = cmd.Execute() + return nil + }) + return out, runErr +} + +func approveBlock(t *testing.T, recorder *reviewRecorder, path string) []any { + t.Helper() + for _, req := range recorder.writes() { + if req.path == path { + var body map[string]any + require.NoError(t, json.Unmarshal([]byte(req.body), &body)) + block, _ := body["block"].([]any) + return block + } + } + t.Fatalf("no write to %s in %#v", path, recorder.requests) + return nil +} + +func TestReviewApproveQuarantinedUsesDefaultSelection(t *testing.T) { + recorder := &reviewRecorder{} + newMemoryReviewDaemon(t, recorder) + + out, err := runReviewApprove(t, "table", nil, "memory", "--yes") + require.NoError(t, err) + require.Equal(t, []any{"legacy_read", "write_a", "write_b", "write_c", "delete_a", "plain_a"}, + approveBlock(t, recorder, "/api/v1/servers/memory/security/approve"), + "no flag blocks every tool the core did not pre-select; a missing default_allowed counts as false") + require.Contains(t, out, "Allowing 3 of 9 tools; blocking 6: "+memoryDefaultBlock) + assertReviewGolden(t, "review-approve-default.golden", out+"\n") +} + +func TestReviewApproveAllFlag(t *testing.T) { + t.Run("--all allows every tool", func(t *testing.T) { + recorder := &reviewRecorder{} + newMemoryReviewDaemon(t, recorder) + out, err := runReviewApprove(t, "table", nil, "memory", "--all", "--yes") + require.NoError(t, err) + require.Empty(t, approveBlock(t, recorder, "/api/v1/servers/memory/security/approve")) + assertReviewGolden(t, "review-approve-all.golden", out+"\n") + }) + t.Run("--all --except subtracts", func(t *testing.T) { + recorder := &reviewRecorder{} + newMemoryReviewDaemon(t, recorder) + _, err := runReviewApprove(t, "table", nil, "memory", "--all", "--except", "delete_a", "--yes") + require.NoError(t, err) + require.Equal(t, []any{"delete_a"}, approveBlock(t, recorder, "/api/v1/servers/memory/security/approve")) + }) + t.Run("--except subtracts from the default selection", func(t *testing.T) { + recorder := &reviewRecorder{} + newMemoryReviewDaemon(t, recorder) + _, err := runReviewApprove(t, "table", nil, "memory", "--except", "read_a", "--yes") + require.NoError(t, err) + require.Equal(t, []any{"read_a", "legacy_read", "write_a", "write_b", "write_c", "delete_a", "plain_a"}, + approveBlock(t, recorder, "/api/v1/servers/memory/security/approve")) + }) + t.Run("--all on a trusted server is the approve-all default", func(t *testing.T) { + recorder := &reviewRecorder{} + newMemoryReviewDaemon(t, recorder) + _, err := runReviewApprove(t, "table", nil, "trusted", "--all", "--yes") + require.NoError(t, err) + assertReviewRequest(t, recorder.requests, "POST", "/api/v1/servers/trusted/tools/approve", map[string]any{"approve_all": true}) + }) + t.Run("--except stays rejected on a trusted server", func(t *testing.T) { + recorder := &reviewRecorder{} + newMemoryReviewDaemon(t, recorder) + _, err := runReviewApprove(t, "table", nil, "trusted", "--except", "x", "--yes") + require.Error(t, err) + require.Empty(t, recorder.writes()) + }) +} + +func TestReviewApproveToolsSelectsExactly(t *testing.T) { + t.Run("blocks the rest", func(t *testing.T) { + recorder := &reviewRecorder{} + newMemoryReviewDaemon(t, recorder) + out, err := runReviewApprove(t, "table", nil, "memory", "--tools", "read_a,write_a", "--yes") + require.NoError(t, err) + require.Equal(t, []any{"read_b", "read_c", "legacy_read", "write_b", "write_c", "delete_a", "plain_a"}, + approveBlock(t, recorder, "/api/v1/servers/memory/security/approve")) + require.Contains(t, out, "Allowing 2 of 9 tools; blocking 7: ") + }) + t.Run("unknown tool is an error and writes nothing", func(t *testing.T) { + recorder := &reviewRecorder{} + newMemoryReviewDaemon(t, recorder) + _, err := runReviewApprove(t, "table", nil, "memory", "--tools", "read_a,nope", "--yes") + require.EqualError(t, err, "unknown tool 'nope' for server 'memory'") + require.Empty(t, recorder.writes()) + }) + t.Run("--all with --tools is an error and writes nothing", func(t *testing.T) { + recorder := &reviewRecorder{} + newMemoryReviewDaemon(t, recorder) + _, err := runReviewApprove(t, "table", nil, "memory", "--all", "--tools", "read_a", "--yes") + require.Error(t, err) + require.Contains(t, err.Error(), "--all") + require.Empty(t, recorder.writes()) + }) +} + +func TestReviewApprovePromptNamesCount(t *testing.T) { + t.Run("default selection", func(t *testing.T) { + recorder := &reviewRecorder{} + newMemoryReviewDaemon(t, recorder) + var prompt string + _, err := runReviewApprove(t, "table", func(message string) (bool, error) { prompt = message; return false, nil }, "memory") + require.NoError(t, err) + require.Equal(t, "Approve server 'memory' with 3 of 9 tools? Blocked: "+memoryDefaultBlock+".", prompt) + require.Empty(t, recorder.writes(), "declining sends no write") + }) + t.Run("all", func(t *testing.T) { + recorder := &reviewRecorder{} + newMemoryReviewDaemon(t, recorder) + var prompt string + _, err := runReviewApprove(t, "table", func(message string) (bool, error) { prompt = message; return false, nil }, "memory", "--all") + require.NoError(t, err) + require.Equal(t, "Approve server 'memory' with all 9 tools?", prompt) + require.Empty(t, recorder.writes()) + }) + t.Run("nothing captured", func(t *testing.T) { + recorder := &reviewRecorder{} + newMemoryReviewDaemon(t, recorder) + var prompt string + _, err := runReviewApprove(t, "table", func(message string) (bool, error) { prompt = message; return false, nil }, "bare") + require.NoError(t, err) + require.Equal(t, "Approve server 'bare' without seeing tools?", prompt) + require.Empty(t, recorder.writes()) + }) + t.Run("confirming sends the write", func(t *testing.T) { + recorder := &reviewRecorder{} + newMemoryReviewDaemon(t, recorder) + _, err := runReviewApprove(t, "table", func(string) (bool, error) { return true, nil }, "memory") + require.NoError(t, err) + require.Len(t, recorder.writes(), 1) + }) +} + +func TestReviewApproveJSONIsTheBareRESTObject(t *testing.T) { + recorder := &reviewRecorder{} + newMemoryReviewDaemon(t, recorder) + out, err := runReviewApprove(t, "json", nil, "memory", "--yes") + require.NoError(t, err) + require.NotContains(t, out, "Allowing") + require.JSONEq(t, `{"status":"approved","server_name":"memory"}`, strings.TrimSpace(out)) +} + +func TestReviewApproveSelection(t *testing.T) { + yes, no := true, false + tools := []reviewToolState{ + {Name: "a", Tier: "read", DefaultAllowed: &yes}, + {Name: "b", Tier: "read", DefaultAllowed: &yes}, + {Name: "c", Tier: "write", DefaultAllowed: &no}, + {Name: "d", Tier: "read"}, + } + for _, tc := range []struct { + name string + all bool + only []string + except []string + block []string + allowed int + err string + }{ + {name: "default", block: []string{"c", "d"}, allowed: 2}, + {name: "all", all: true, block: nil, allowed: 4}, + {name: "all except", all: true, except: []string{"c"}, block: []string{"c"}, allowed: 3}, + {name: "only", only: []string{"c", "a"}, block: []string{"b", "d"}, allowed: 2}, + {name: "only except", only: []string{"a", "b"}, except: []string{"b"}, block: []string{"b", "c", "d"}, allowed: 1}, + {name: "default except", except: []string{"a"}, block: []string{"a", "c", "d"}, allowed: 1}, + {name: "unknown only", only: []string{"zz"}, err: "unknown tool 'zz'"}, + {name: "unknown except", except: []string{"zz"}, err: "unknown tool 'zz'"}, + {name: "all and only", all: true, only: []string{"a"}, err: "--all cannot be combined with --tools"}, + } { + t.Run(tc.name, func(t *testing.T) { + block, allowed, err := reviewApproveSelection(tools, tc.all, tc.only, tc.except) + if tc.err != "" { + require.EqualError(t, err, tc.err) + return + } + require.NoError(t, err) + require.Equal(t, tc.block, block) + require.Equal(t, tc.allowed, allowed) + }) + } +} diff --git a/cmd/mcpproxy/review_cmd_test.go b/cmd/mcpproxy/review_cmd_test.go index 0a8abbc2a..fe7c7eba8 100644 --- a/cmd/mcpproxy/review_cmd_test.go +++ b/cmd/mcpproxy/review_cmd_test.go @@ -85,15 +85,21 @@ func TestReviewCommandGoldens(t *testing.T) { func TestReviewCommandPromptsAndHonorsDecline(t *testing.T) { for _, action := range []string{"approve", "reject"} { t.Run(action, func(t *testing.T) { + // approve reads the review before it prompts (the prompt names the + // exact tool count), so it needs a daemon; reject does not. + recorder := &reviewRecorder{} + newMemoryReviewDaemon(t, recorder) + setOutputGlobals(t, "table", false) var prompt string cmd := newReviewCommand(func(message string) (bool, error) { prompt = message return false, nil }) - cmd.SetArgs([]string{action, "filesystem"}) + cmd.SetArgs([]string{action, "memory"}) require.NoError(t, cmd.Execute()) - require.Contains(t, prompt, "filesystem") + require.Contains(t, prompt, "memory") require.Contains(t, strings.ToLower(prompt), action) + require.Empty(t, recorder.writes(), "declining sends no write") }) } } diff --git a/cmd/mcpproxy/testdata/cli109/review-approve-all.golden b/cmd/mcpproxy/testdata/cli109/review-approve-all.golden new file mode 100644 index 000000000..e9be135f4 --- /dev/null +++ b/cmd/mcpproxy/testdata/cli109/review-approve-all.golden @@ -0,0 +1,2 @@ +Allowing 9 of 9 tools; blocking none +Approved server memory diff --git a/cmd/mcpproxy/testdata/cli109/review-approve-default.golden b/cmd/mcpproxy/testdata/cli109/review-approve-default.golden new file mode 100644 index 000000000..5a9145721 --- /dev/null +++ b/cmd/mcpproxy/testdata/cli109/review-approve-default.golden @@ -0,0 +1,2 @@ +Allowing 3 of 9 tools; blocking 6: legacy_read, write_a, write_b, write_c, delete_a, plain_a +Approved server memory diff --git a/cmd/mcpproxy/testdata/cli109/review-approve.golden b/cmd/mcpproxy/testdata/cli109/review-approve.golden index c81eaee35..2111acb5c 100644 --- a/cmd/mcpproxy/testdata/cli109/review-approve.golden +++ b/cmd/mcpproxy/testdata/cli109/review-approve.golden @@ -1 +1,2 @@ +Allowing 0 of 1 tool; blocking 1: delete_0 Approved server filesystem diff --git a/internal/runtime/review.go b/internal/runtime/review.go index f97cc2046..b79662a6f 100644 --- a/internal/runtime/review.go +++ b/internal/runtime/review.go @@ -108,8 +108,11 @@ type ReviewTool struct { ScanVerdict string `json:"scan_verdict"` HeldReason string `json:"held_reason"` HeldSignals []string `json:"held_signals"` - Previous *ReviewToolPrevious `json:"previous"` - Diff *ReviewToolDiff `json:"diff,omitempty"` + // DefaultAllowed is the review screens' fail-closed default selection + // (D41). Always serialised, so an older core (field absent) reads as false. + DefaultAllowed bool `json:"default_allowed"` + Previous *ReviewToolPrevious `json:"previous"` + Diff *ReviewToolDiff `json:"diff,omitempty"` } type ReviewToolPrevious struct { @@ -233,6 +236,7 @@ func (r *Runtime) GetServerReview(ctx context.Context, serverName string) (*Serv ScanVerdict: reviewToolScanVerdict(scanFindings, serverName, record, covered[record.ToolName]), HeldReason: record.HeldReason, HeldSignals: append([]string(nil), record.HeldSignals...), } + tool.DefaultAllowed = reviewDefaultAllowed(tool) if record.PreviousDescription != "" || record.PreviousSchema != "" || record.PreviousOutputSchema != "" || record.PreviousAnnotations != nil { tool.Previous = &ReviewToolPrevious{ Description: record.PreviousDescription, InputSchema: rawSchema(record.PreviousSchema), @@ -247,6 +251,22 @@ func (r *Runtime) GetServerReview(ctx context.Context, serverName string) (*Serv return result, nil } +// reviewDefaultAllowed is the default selection of the review screens (D41.2). +// An already blocked tool stays blocked; an approved tool stays allowed; a +// pending or changed tool starts allowed only when it is read-only, the scan +// verified its current definition as clean and nothing holds it. Everything +// else (write, destructive, unannotated, unknown, not scanned, warnings, +// dangerous, held) starts unchecked. +func reviewDefaultAllowed(tool ReviewTool) bool { + if tool.Disabled { + return false + } + if tool.ApprovalStatus == storage.ToolApprovalStatusApproved { + return true + } + return tool.Tier == contracts.TierRead && tool.ScanVerdict == "clean" && tool.HeldReason == "" +} + func reviewTier(annotations *config.ToolAnnotations) contracts.Tier { if annotations == nil { return contracts.TierUnknown diff --git a/internal/runtime/review_default_allowed_test.go b/internal/runtime/review_default_allowed_test.go new file mode 100644 index 000000000..d700e7907 --- /dev/null +++ b/internal/runtime/review_default_allowed_test.go @@ -0,0 +1,160 @@ +package runtime + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/contracts" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/security/scanner" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/storage" +) + +// TestReviewDefaultAllowed pins the fail-closed default selection (D41.2): +// the core decides which tools the review screens pre-check. +func TestReviewDefaultAllowed(t *testing.T) { + tiers := []contracts.Tier{ + contracts.TierRead, contracts.TierWrite, contracts.TierDestructive, + contracts.TierUnannotated, contracts.TierUnknown, + } + statuses := []string{ + storage.ToolApprovalStatusPending, storage.ToolApprovalStatusChanged, storage.ToolApprovalStatusApproved, + } + scanVerdicts := []string{"clean", "not_scanned", "warnings", "dangerous"} + + for _, tier := range tiers { + for _, status := range statuses { + for _, verdict := range scanVerdicts { + for _, disabled := range []bool{false, true} { + for _, held := range []string{"", "scan gate"} { + tool := ReviewTool{Tier: tier, ApprovalStatus: status, ScanVerdict: verdict, Disabled: disabled, HeldReason: held} + var want bool + switch { + case disabled: + want = false + case status == storage.ToolApprovalStatusApproved: + want = true + default: + want = tier == contracts.TierRead && verdict == "clean" && held == "" + } + require.Equal(t, want, reviewDefaultAllowed(tool), + "tier=%s status=%s verdict=%s disabled=%v held=%q", tier, status, verdict, disabled, held) + } + } + } + } + } + + // A tool held by the scan gate can carry HeldVerdict "clean" (only coverage + // failed), which surfaces as scan_verdict "clean". It must still start + // unchecked. + require.False(t, reviewDefaultAllowed(ReviewTool{ + Tier: contracts.TierRead, ApprovalStatus: storage.ToolApprovalStatusPending, ScanVerdict: "clean", HeldReason: "coverage failed", + })) + require.True(t, reviewDefaultAllowed(ReviewTool{ + Tier: contracts.TierRead, ApprovalStatus: storage.ToolApprovalStatusPending, ScanVerdict: "clean", + })) +} + +func TestReviewTool_DefaultAllowedSerialisedWhenFalse(t *testing.T) { + raw, err := json.Marshal(ReviewTool{Name: "x"}) + require.NoError(t, err) + var decoded map[string]any + require.NoError(t, json.Unmarshal(raw, &decoded)) + value, present := decoded["default_allowed"] + require.True(t, present, "default_allowed must always be present so an old core is distinguishable from false") + require.Equal(t, false, value) +} + +func saveTieredRecord(t *testing.T, rt *Runtime, server, tool, status string, readOnly, destructive *bool) { + t.Helper() + var annotations *config.ToolAnnotations + if readOnly != nil || destructive != nil { + annotations = &config.ToolAnnotations{ReadOnlyHint: readOnly, DestructiveHint: destructive} + } + require.NoError(t, rt.storageManager.SaveToolApproval(&storage.ToolApprovalRecord{ + ServerName: server, ToolName: tool, Status: status, CurrentDescription: "d", + CurrentHash: "h-" + tool, ApprovedHash: "h-" + tool, CurrentAnnotations: annotations, + })) +} + +func defaultsOf(review *ServerReview) map[string]bool { + out := map[string]bool{} + for _, tool := range review.Tools { + out[tool.Name] = tool.DefaultAllowed + } + return out +} + +func TestServerReview_DefaultAllowedFollowsCoverage(t *testing.T) { + yes, no := true, false + newRuntime := func(t *testing.T, quarantined bool) *Runtime { + return setupQuarantineRuntime(t, nil, []*config.ServerConfig{{Name: "srv", Enabled: true, Quarantined: quarantined}}) + } + seedMixed := func(t *testing.T, rt *Runtime) { + saveTieredRecord(t, rt, "srv", "read_a", storage.ToolApprovalStatusPending, &yes, nil) + saveTieredRecord(t, rt, "srv", "write_a", storage.ToolApprovalStatusPending, &no, nil) + saveTieredRecord(t, rt, "srv", "delete_a", storage.ToolApprovalStatusPending, &no, &yes) + saveTieredRecord(t, rt, "srv", "plain_a", storage.ToolApprovalStatusPending, nil, nil) + } + toolsCtx := func(names ...string) *scanner.ScanContext { + return &scanner.ScanContext{SourceMethod: "tool_definitions_only", ToolsExported: len(names), ToolNames: names} + } + + t.Run("current: only the clean read tool starts allowed", func(t *testing.T) { + rt := newRuntime(t, true) + seedMixed(t, rt) + seedReviewScanJob(t, rt, "srv", "j1", scanner.ScanJobStatusCompleted, toolsCtx("read_a", "write_a", "delete_a", "plain_a"), nil) + review := reviewOf(t, rt, "srv") + require.Equal(t, ReviewScanCoverageCurrent, review.Server.Scan.Coverage) + require.Equal(t, map[string]bool{"read_a": true, "write_a": false, "delete_a": false, "plain_a": false}, defaultsOf(review)) + }) + + t.Run("none: nothing starts allowed", func(t *testing.T) { + rt := newRuntime(t, true) + seedMixed(t, rt) + review := reviewOf(t, rt, "srv") + require.Equal(t, ReviewScanCoverageNone, review.Server.Scan.Coverage) + require.Equal(t, map[string]bool{"read_a": false, "write_a": false, "delete_a": false, "plain_a": false}, defaultsOf(review)) + }) + + t.Run("tools_not_scanned: nothing starts allowed", func(t *testing.T) { + rt := newRuntime(t, true) + seedMixed(t, rt) + seedReviewScanJob(t, rt, "srv", "j1", scanner.ScanJobStatusCompleted, &scanner.ScanContext{SourceMethod: "working_dir", TotalFiles: 5}, nil) + review := reviewOf(t, rt, "srv") + require.Equal(t, ReviewScanCoverageToolsNotScanned, review.Server.Scan.Coverage) + for name, allowed := range defaultsOf(review) { + require.False(t, allowed, name) + } + }) + + t.Run("stale: only the tool the scan did not cover starts unchecked", func(t *testing.T) { + rt := newRuntime(t, true) + saveTieredRecord(t, rt, "srv", "read_a", storage.ToolApprovalStatusPending, &yes, nil) + saveTieredRecord(t, rt, "srv", "read_b", storage.ToolApprovalStatusPending, &yes, nil) + seedReviewScanJob(t, rt, "srv", "j1", scanner.ScanJobStatusCompleted, toolsCtx("read_a"), nil) + review := reviewOf(t, rt, "srv") + require.Equal(t, ReviewScanCoverageStale, review.Server.Scan.Coverage) + require.Equal(t, map[string]bool{"read_a": true, "read_b": false}, defaultsOf(review)) + }) + + t.Run("dangerous finding unchecks the tool", func(t *testing.T) { + rt := newRuntime(t, true) + saveTieredRecord(t, rt, "srv", "read_a", storage.ToolApprovalStatusPending, &yes, nil) + saveTieredRecord(t, rt, "srv", "read_b", storage.ToolApprovalStatusPending, &yes, nil) + seedReviewScanJob(t, rt, "srv", "j1", scanner.ScanJobStatusCompleted, toolsCtx("read_a", "read_b"), []scanner.ScanFinding{ + {RuleID: "TPA-1", Severity: "critical", ThreatLevel: scanner.ThreatLevelDangerous, Title: "hidden instruction", Location: "tool:read_b", Scanner: "tpa"}, + }) + require.Equal(t, map[string]bool{"read_a": true, "read_b": false}, defaultsOf(reviewOf(t, rt, "srv"))) + }) + + t.Run("approved and enabled stays allowed on a requarantined server", func(t *testing.T) { + rt := newRuntime(t, true) + saveTieredRecord(t, rt, "srv", "write_a", storage.ToolApprovalStatusApproved, &no, nil) + review := reviewOf(t, rt, "srv") + require.Equal(t, map[string]bool{"write_a": true}, defaultsOf(review)) + }) +} diff --git a/internal/server/e2e_test.go b/internal/server/e2e_test.go index f09a46280..520b13723 100644 --- a/internal/server/e2e_test.go +++ b/internal/server/e2e_test.go @@ -1180,6 +1180,9 @@ func TestE2E_InspectQuarantined(t *testing.T) { Tier string `json:"tier"` ScanVerdict string `json:"scan_verdict"` Annotations json.RawMessage `json:"annotations"` + // default_allowed is always present and false on the live path: + // nothing was scanned, so nothing starts pre-selected (D41.7). + DefaultAllowed *bool `json:"default_allowed"` } `json:"tools"` } require.NoError(t, json.Unmarshal([]byte(resultText), &liveReview)) @@ -1188,6 +1191,10 @@ func TestE2E_InspectQuarantined(t *testing.T) { require.Equal(t, "write", liveReview.Tools[0].Tier) require.Equal(t, "not_scanned", liveReview.Tools[0].ScanVerdict) require.NotEmpty(t, liveReview.Tools[0].Annotations) + for _, tool := range liveReview.Tools { + require.NotNil(t, tool.DefaultAllowed, "live inspection must carry default_allowed for %s", tool.Name) + require.False(t, *tool.DefaultAllowed, "live inspection must start %s unchecked", tool.Name) + } // After inspection, server should be disconnected again (exemption revoked) time.Sleep(1 * time.Second) diff --git a/internal/server/mcp.go b/internal/server/mcp.go index 6697efdfe..daee550ed 100644 --- a/internal/server/mcp.go +++ b/internal/server/mcp.go @@ -5477,6 +5477,7 @@ func (p *MCPProxyServer) handleInspectQuarantinedTools(ctx context.Context, requ "approval_status": "pending", "disabled": false, "scan_verdict": "not_scanned", + "default_allowed": false, "server_name": serverName, "quarantine_status": "QUARANTINED", diff --git a/internal/server/quarantine_inspect_review_parity_test.go b/internal/server/quarantine_inspect_review_parity_test.go index 467b33ee0..d66e2fa3e 100644 --- a/internal/server/quarantine_inspect_review_parity_test.go +++ b/internal/server/quarantine_inspect_review_parity_test.go @@ -54,6 +54,64 @@ func TestInspectQuarantinedTools_UsesComposerWhenDefinitionsCaptured(t *testing. require.JSONEq(t, string(wantTools), string(gotTools)) } +// default_allowed is the review screens' fail-closed selection hint (D41). The +// captured inspection serialises the composer, so it must equal the REST value +// for a mix of tiers; inspect_tools is an approval-state listing and does not +// carry it. +func TestInspectQuarantinedCarriesDefaultAllowed(t *testing.T) { + proxy, rt := createTestProxyWithRuntime(t, []*config.ServerConfig{{ + Name: "github", Enabled: true, Quarantined: true, + }}) + require.NoError(t, rt.StorageManager().SaveUpstreamServer(&config.ServerConfig{ + Name: "github", Enabled: true, Quarantined: true, + })) + readOnly, writes := true, false + for name, hint := range map[string]*bool{"list_issues": &readOnly, "create_issue": &writes} { + require.NoError(t, rt.StorageManager().SaveToolApproval(&storage.ToolApprovalRecord{ + ServerName: "github", ToolName: name, Status: storage.ToolApprovalStatusPending, + CurrentHash: "h-" + name, CurrentDescription: name, + CurrentAnnotations: &config.ToolAnnotations{ReadOnlyHint: hint}, + })) + } + review, err := rt.GetServerReview(context.Background(), "github") + require.NoError(t, err) + want := map[string]bool{} + for _, tool := range review.Tools { + want[tool.Name] = tool.DefaultAllowed + } + require.Len(t, want, 2) + + result, err := proxy.handleInspectQuarantinedTools(context.Background(), quarantineRequest(map[string]interface{}{"name": "github"})) + require.NoError(t, err) + require.False(t, result.IsError, "%v", result.Content) + var payload struct { + Tools []map[string]json.RawMessage `json:"tools"` + } + require.NoError(t, json.Unmarshal([]byte(result.Content[0].(mcp.TextContent).Text), &payload)) + require.Len(t, payload.Tools, 2) + for _, tool := range payload.Tools { + var name string + require.NoError(t, json.Unmarshal(tool["name"], &name)) + raw, ok := tool["default_allowed"] + require.Truef(t, ok, "captured inspection must carry default_allowed for %s", name) + var got bool + require.NoError(t, json.Unmarshal(raw, &got)) + require.Equal(t, want[name], got, name) + } + + listed, err := proxy.handleInspectToolApprovals(context.Background(), quarantineRequest(map[string]interface{}{"name": "github"})) + require.NoError(t, err) + require.False(t, listed.IsError, "%v", listed.Content) + var approvals struct { + Tools []map[string]json.RawMessage `json:"tools"` + } + require.NoError(t, json.Unmarshal([]byte(listed.Content[0].(mcp.TextContent).Text), &approvals)) + require.NotEmpty(t, approvals.Tools) + for _, tool := range approvals.Tools { + require.NotContains(t, tool, "default_allowed", "inspect_tools is an approval listing, not a selection hint") + } +} + func TestInspectToolsIncludesCanonicalReviewFields(t *testing.T) { proxy, rt := createTestProxyWithRuntime(t, []*config.ServerConfig{{ Name: "github", Enabled: true, Quarantined: true, From 28282a25cbc86db9d6eb8737f25ab912e40cb11c Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 2 Oct 2026 22:34:26 +0300 Subject: [PATCH 2/6] feat(review): Web and macOS review screens start fail-closed with an exact-count approve label (Spec fix-review-defaults) Both screens read default_allowed from the core, show the selection hint, name the exact count on the approve button, add an explicit Approve all, keep the user's choices across reloads and re-send the attempted block list on the force retry. --- frontend/src/components/ReviewScreen.vue | 32 ++- frontend/src/types/api.ts | 2 + frontend/src/utils/reviewPresentation.ts | 59 ++++++ .../review-screen-default-selection.spec.ts | 196 ++++++++++++++++++ frontend/tests/unit/review-screen.spec.ts | 19 +- .../macos/MCPProxy/MCPProxy/API/Models.swift | 2 +- .../MCPProxy/Views/ReviewQueueView.swift | 57 ++++- .../MCPProxyTests/ReviewPayloadTests.swift | 9 + .../ReviewPresentationTests.swift | 55 +++++ 9 files changed, 409 insertions(+), 22 deletions(-) create mode 100644 frontend/tests/unit/review-screen-default-selection.spec.ts diff --git a/frontend/src/components/ReviewScreen.vue b/frontend/src/components/ReviewScreen.vue index 5dc8d341c..2ab51a35b 100644 --- a/frontend/src/components/ReviewScreen.vue +++ b/frontend/src/components/ReviewScreen.vue @@ -35,7 +35,7 @@

{{ tool.name }}

{{ tool.tier }} {{ tool.scan_verdict }}
- +
{{ toolState(tool, review.server.quarantined) === 'blocked' ? 'Blocked' : 'Approved' }}
@@ -45,8 +45,10 @@
+

{{ REVIEW_SELECTION_HINT }}

- + +
@@ -69,14 +71,14 @@ import type { ReviewTool, ServerReviewResponse } from '@/types' import ToolDefinitionText from '@/components/ToolDefinitionText.vue' import ScanHistory from '@/components/ScanHistory.vue' import { scanReportPath } from '@/utils/serverRoute' -import { reviewHeadline, scanBanner, toolState } from '@/utils/reviewPresentation' +import { REVIEW_SELECTION_HINT, approveAllLabel, approveLabel, mergeSelection, reviewHeadline, scanBanner, toolState, type SelectionChoice } from '@/utils/reviewPresentation' const props = defineProps<{ serverName: string; change?: string }>() const emit = defineEmits<{ approved: []; refreshed: [] }>() const review = ref(null) const loading = ref(false); const scanning = ref(false); const approving = ref(false); const error = ref('') const rescanning = ref(false); const requarantining = ref(false); const requarantineDialog = ref(null) -const allowedTools = ref([]); const confirmDialog = ref(null); const forceDialog = ref(null); const confirmOpen = ref(false) +const allowedTools = ref([]); const choices = new Map(); const lastBlock = ref([]); const confirmDialog = ref(null); const forceDialog = ref(null); const confirmOpen = ref(false) const tiers = ['read', 'write', 'destructive', 'unannotated', 'unknown'] const headline = computed(() => review.value ? reviewHeadline(review.value) : { state: 'review', title: '', subtitle: '' }) const banner = computed(() => { @@ -88,10 +90,14 @@ const banner = computed(() => { }) const bannerClass = computed(() => `alert-${banner.value?.severity ?? 'info'}`) const filteredTools = computed(() => review.value?.tools.filter(t => !props.change || t.approval_status === props.change) ?? []) +const primaryLabel = computed(() => approveLabel(allowedTools.value.length, review.value?.tools.length ?? 0, review.value?.server.definitions_captured ?? false)) +const showApproveAll = computed(() => !!review.value && review.value.server.quarantined && review.value.server.definitions_captured && review.value.tools.length > 0 && allowedTools.value.length < review.value.tools.length) const tierCounts = computed(() => Object.fromEntries(tiers.map(t => [t, (review.value?.tools ?? []).filter(x => x.tier === t).length]))) +// An explicit click is remembered with the payload the user saw, so a reload keeps it (D41.4). +function recordChoice(tool: ReviewTool, allowed: boolean) { choices.set(tool.name, { allowed, tool }) } function definitionText(tool: ReviewTool) { return JSON.stringify({ input_schema: tool.input_schema, output_schema: tool.output_schema, annotations: tool.annotations }, null, 2) } function diffText(tool: ReviewTool) { return Object.values(tool.diff ?? {}).filter(Boolean).join('\n\n') || JSON.stringify(tool.previous, null, 2) } -async function load() { if (typeof api.getServerReview !== 'function') return; loading.value = true; error.value = ''; const res = await api.getServerReview(props.serverName); loading.value = false; if (!res.success || !res.data) { error.value = res.error || 'Failed to load review'; return }; review.value = res.data; allowedTools.value = res.data.tools.filter(t => !t.disabled).map(t => t.name); emit('refreshed') } +async function load() { if (typeof api.getServerReview !== 'function') return; loading.value = true; error.value = ''; const res = await api.getServerReview(props.serverName); loading.value = false; if (!res.success || !res.data) { error.value = res.error || 'Failed to load review'; return }; review.value = res.data; allowedTools.value = mergeSelection(res.data.tools, choices); emit('refreshed') } async function rescan() { rescanning.value = true const res = await api.startScan(props.serverName) @@ -113,9 +119,19 @@ async function fetchDefinitions() { if (!res.success) { error.value = res.error || 'Failed to capture tool definitions'; return } await load() } -function requestApprove() { if (!review.value?.server.definitions_captured) { confirmOpen.value = true; confirmDialog.value?.showModal(); return }; void approve(false) } +function requestApprove(everything: boolean) { if (!review.value?.server.definitions_captured) { confirmOpen.value = true; confirmDialog.value?.showModal(); return }; void approve(false, everything ? [] : undefined) } function closeConfirm() { confirmOpen.value = false; confirmDialog.value?.close?.() } -async function approve(force: boolean) { closeConfirm(); forceDialog.value?.close?.(); approving.value = true; const all = review.value?.tools.map(t => t.name) ?? []; const res = await api.securityApprove(props.serverName, force, all.filter(name => !allowedTools.value.includes(name))); approving.value = false; if (!res.success) { error.value = res.error || 'Approval failed'; if (!force && /dangerous/i.test(error.value)) forceDialog.value?.showModal?.(); return }; emit('approved'); await load() } +// The force retry re-sends the block list of the attempt that triggered it (D41.5). +async function approve(force: boolean, block?: string[]) { + closeConfirm(); forceDialog.value?.close?.(); approving.value = true + const all = review.value?.tools.map(t => t.name) ?? [] + const blocked = block ?? (force ? lastBlock.value : all.filter(name => !allowedTools.value.includes(name))) + lastBlock.value = blocked + const res = await api.securityApprove(props.serverName, force, blocked) + approving.value = false + if (!res.success) { error.value = res.error || 'Approval failed'; if (!force && /dangerous/i.test(error.value)) forceDialog.value?.showModal?.(); return } + choices.clear(); emit('approved'); await load() +} async function rejectServer() { approving.value = true; const res = await api.securityReject(props.serverName); approving.value = false; if (!res.success) error.value = res.error || 'Reject failed'; else await load() } async function approveTool(name: string) { await api.approveTools(props.serverName, [name]); await load() } async function blockTool(name: string) { await api.blockTools(props.serverName, [name]); await load() } @@ -128,7 +144,7 @@ async function refreshAfterScanSettled(event: Event) { void load() } // Component reuse across /review/A -> /review/B: scan state belongs to the old server. -watch(() => props.serverName, () => { scanning.value = false; rescanning.value = false; error.value = ''; void load() }) +watch(() => props.serverName, () => { choices.clear(); scanning.value = false; rescanning.value = false; error.value = ''; void load() }) onMounted(() => { void load() window.addEventListener('mcpproxy:review-changed', refreshAfterReviewChange) diff --git a/frontend/src/types/api.ts b/frontend/src/types/api.ts index 97bea4b7d..0e97d6194 100644 --- a/frontend/src/types/api.ts +++ b/frontend/src/types/api.ts @@ -1430,6 +1430,8 @@ export interface ReviewTool { scan_verdict: string held_reason?: string held_signals?: string[] + /** Fail-closed default selection computed by the core (D41); absent on an older core, which reads as false. */ + default_allowed?: boolean previous?: ReviewToolPrevious | null diff?: ReviewToolDiff | null } diff --git a/frontend/src/utils/reviewPresentation.ts b/frontend/src/utils/reviewPresentation.ts index 9c303ee8d..8dfa7403b 100644 --- a/frontend/src/utils/reviewPresentation.ts +++ b/frontend/src/utils/reviewPresentation.ts @@ -94,3 +94,62 @@ export function toolState(tool: ReviewTool, quarantined: boolean): ToolControl { if (tool.approval_status === 'approved') return tool.disabled ? 'blocked' : 'approved' return 'approve-reject' } + +// --------------------------------------------------------------------------- +// Default selection (Spec 109 fix-review-defaults, D41). The core decides which +// tools start checked (`default_allowed`); these helpers only read it. A payload +// from an older core has no field, which reads as false: every tool starts +// unchecked, so a mismatched core fails closed. The macOS app +// (ReviewPresentation in ReviewQueueView.swift) uses the same sentences. +// --------------------------------------------------------------------------- + +export const REVIEW_SELECTION_HINT = 'Only read-only tools with a clean scan start checked. Unchecked tools stay blocked after approval until you enable them on the Tools tab.' + +/** What the user explicitly chose for one tool, with the payload they saw. */ +export interface SelectionChoice { + allowed: boolean + tool: ReviewTool +} + +/** The names that start checked: the core's `default_allowed`, a missing field counting as false. */ +export function initialSelection(tools: ReviewTool[]): string[] { + return tools.filter(t => t.default_allowed === true).map(t => t.name) +} + +function sameValue(a: unknown, b: unknown): boolean { + if (a === b) return true + if (a === null || b === null || typeof a !== 'object' || typeof b !== 'object') return false + if (Array.isArray(a) !== Array.isArray(b)) return false + const left = a as Record + const right = b as Record + const keys = new Set([...Object.keys(left), ...Object.keys(right)]) + for (const key of keys) { + if (!sameValue(left[key], right[key])) return false + } + return true +} + +/** + * The selection after a reload. An explicit uncheck always survives; an + * explicit check survives only while the tool's payload is the one the user + * saw (a changed definition, verdict or tier falls back to the default). + */ +export function mergeSelection(tools: ReviewTool[], choices: Map): string[] { + return tools.filter(tool => { + const choice = choices.get(tool.name) + if (!choice) return tool.default_allowed === true + if (!choice.allowed) return false + return sameValue(choice.tool, tool) ? true : tool.default_allowed === true + }).map(t => t.name) +} + +/** Primary approve button: the exact count, or the blind-approval wording when nothing is captured. */ +export function approveLabel(selected: number, total: number, definitionsCaptured: boolean): string { + if (!definitionsCaptured || total === 0) return 'Approve without seeing tools' + return `Approve server (${selected} of ${total} ${plural(total, 'tool', 'tools')})` +} + +/** The explicit approve-everything action. */ +export function approveAllLabel(total: number): string { + return `Approve all (${total} ${plural(total, 'tool', 'tools')})` +} diff --git a/frontend/tests/unit/review-screen-default-selection.spec.ts b/frontend/tests/unit/review-screen-default-selection.spec.ts new file mode 100644 index 000000000..f3406c121 --- /dev/null +++ b/frontend/tests/unit/review-screen-default-selection.spec.ts @@ -0,0 +1,196 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { flushPromises, mount } from '@vue/test-utils' +import ReviewScreen from '@/components/ReviewScreen.vue' +import api from '@/services/api' +import { + REVIEW_SELECTION_HINT, + approveAllLabel, + approveLabel, + initialSelection, + mergeSelection, + type SelectionChoice, +} from '@/utils/reviewPresentation' +import type { ReviewTool } from '@/types' + +vi.mock('@/services/api', () => ({ default: { + getServerReview: vi.fn(), securityApprove: vi.fn(), securityReject: vi.fn(), + approveTools: vi.fn(), blockTools: vi.fn(), discoverServerTools: vi.fn(), listScanHistory: vi.fn(), scanAll: vi.fn(), getQueueProgress: vi.fn(), cancelAllScans: vi.fn(), startScan: vi.fn(), quarantineServer: vi.fn(), +} })) + +const tool = (name: string, extra: Partial = {}): ReviewTool => ({ + name, description: name, tier: 'read', approval_status: 'pending', disabled: false, scan_verdict: 'clean', default_allowed: true, ...extra, +}) + +describe('review selection helpers (D41)', () => { + it('initialSelection reads default_allowed and counts a missing field as false', () => { + const tools = [ + tool('read_a'), + tool('write_a', { tier: 'write', default_allowed: false }), + tool('old_core', { default_allowed: undefined }), + ] + expect(initialSelection(tools)).toEqual(['read_a']) + }) + + it('mergeSelection keeps an explicit uncheck', () => { + const tools = [tool('read_a'), tool('read_b')] + const choices = new Map([['read_a', { allowed: false, tool: tools[0] }]]) + expect(mergeSelection(tools, choices)).toEqual(['read_b']) + }) + + it('mergeSelection keeps an explicit check only while the payload is unchanged', () => { + const before = tool('write_a', { tier: 'write', default_allowed: false }) + const choices = new Map([['write_a', { allowed: true, tool: before }]]) + expect(mergeSelection([structuredClone(before)], choices)).toEqual(['write_a']) + // The definition, the verdict or the tier changed after the click: back to the default. + expect(mergeSelection([{ ...before, description: 'now does more' }], choices)).toEqual([]) + expect(mergeSelection([{ ...before, scan_verdict: 'warnings' }], choices)).toEqual([]) + expect(mergeSelection([{ ...before, tier: 'destructive' }], choices)).toEqual([]) + }) + + it('mergeSelection ignores choices for tools that are gone', () => { + const choices = new Map([['gone', { allowed: true, tool: tool('gone') }]]) + expect(mergeSelection([tool('read_a')], choices)).toEqual(['read_a']) + }) + + it('labels name the exact count', () => { + expect(approveLabel(3, 9, true)).toBe('Approve server (3 of 9 tools)') + expect(approveLabel(0, 9, true)).toBe('Approve server (0 of 9 tools)') + expect(approveLabel(1, 1, true)).toBe('Approve server (1 of 1 tool)') + expect(approveLabel(0, 0, true)).toBe('Approve without seeing tools') + expect(approveLabel(0, 5, false)).toBe('Approve without seeing tools') + expect(approveAllLabel(9)).toBe('Approve all (9 tools)') + expect(approveAllLabel(1)).toBe('Approve all (1 tool)') + }) + + it('the hint is the shared sentence', () => { + expect(REVIEW_SELECTION_HINT).toBe('Only read-only tools with a clean scan start checked. Unchecked tools stay blocked after approval until you enable them on the Tools tab.') + }) +}) + +const payload = (tools: ReviewTool[], definitionsCaptured = true) => ({ + success: true, + data: { + server: { name: 'fixture', transport: 'stdio', quarantined: true, definitions_captured: definitionsCaptured }, + tools, + }, +}) + +const fixtureTools = () => [ + tool('read_file'), + tool('read_more'), + tool('write_file', { tier: 'write', default_allowed: false }), + tool('remove_file', { tier: 'destructive', approval_status: 'changed', scan_verdict: 'warnings', default_allowed: false }), + tool('implicit', { tier: 'unannotated', scan_verdict: 'not_scanned', default_allowed: false }), +] + +async function mountScreen(tools: ReviewTool[] = fixtureTools(), definitionsCaptured = true) { + ;(api.getServerReview as any).mockResolvedValue(payload(tools, definitionsCaptured)) + const wrapper = mount(ReviewScreen, { props: { serverName: 'fixture' }, global: { stubs: { RouterLink: { template: '' } } } }) + await flushPromises() + return wrapper +} + +const checked = (wrapper: Awaited>, name: string) => + (wrapper.get(`[data-test="review-allow-${name}"]`).element as HTMLInputElement).checked + +describe('ReviewScreen default selection (D41)', () => { + beforeEach(() => { + vi.clearAllMocks() + ;(api.securityApprove as any).mockResolvedValue({ success: true }) + ;(api.discoverServerTools as any).mockResolvedValue({ success: true }) + ;(api.listScanHistory as any).mockResolvedValue({ success: true, data: { scans: [], total: 0 } }) + ;(api.getQueueProgress as any).mockResolvedValue({ success: true, data: { status: 'idle' } }) + }) + + it('starts only the core-selected tools checked and names the exact count', async () => { + const wrapper = await mountScreen() + expect(checked(wrapper, 'read_file')).toBe(true) + expect(checked(wrapper, 'read_more')).toBe(true) + expect(checked(wrapper, 'write_file')).toBe(false) + expect(checked(wrapper, 'remove_file')).toBe(false) + expect(checked(wrapper, 'implicit')).toBe(false) + expect(wrapper.get('[data-test="review-approve-server"]').text()).toBe('Approve server (2 of 5 tools)') + expect(wrapper.get('[data-test="review-selection-hint"]').text()).toBe(REVIEW_SELECTION_HINT) + }) + + it('an older core without default_allowed fails closed', async () => { + const tools = fixtureTools().map(t => ({ ...t, default_allowed: undefined })) + const wrapper = await mountScreen(tools) + for (const t of tools) expect(checked(wrapper, t.name)).toBe(false) + expect(wrapper.get('[data-test="review-approve-server"]').text()).toBe('Approve server (0 of 5 tools)') + expect(wrapper.find('[data-test="review-approve-all"]').exists()).toBe(true) + }) + + it('the primary button sends the unchecked tools as the block list', async () => { + const wrapper = await mountScreen() + await wrapper.get('[data-test="review-approve-server"]').trigger('click') + await flushPromises() + expect(api.securityApprove).toHaveBeenCalledWith('fixture', false, ['write_file', 'remove_file', 'implicit']) + }) + + it('Approve all sends an empty block list', async () => { + const wrapper = await mountScreen() + expect(wrapper.get('[data-test="review-approve-all"]').text()).toBe('Approve all (5 tools)') + await wrapper.get('[data-test="review-approve-all"]').trigger('click') + await flushPromises() + expect(api.securityApprove).toHaveBeenCalledWith('fixture', false, []) + }) + + it('hides Approve all when everything is already selected or nothing is captured', async () => { + const everything = await mountScreen([tool('read_a'), tool('read_b')]) + expect(everything.find('[data-test="review-approve-all"]').exists()).toBe(false) + const blind = await mountScreen([], false) + expect(blind.find('[data-test="review-approve-all"]').exists()).toBe(false) + expect(blind.get('[data-test="review-approve-server"]').text()).toBe('Approve without seeing tools') + expect(blind.find('[data-test="review-selection-hint"]').exists()).toBe(false) + }) + + it('the forced retry re-sends the block list of the attempt that triggered it', async () => { + for (const trigger of ['review-approve-server', 'review-approve-all']) { + vi.clearAllMocks() + ;(api.securityApprove as any).mockResolvedValueOnce({ success: false, error: 'dangerous baseline finding' }).mockResolvedValueOnce({ success: true }) + const wrapper = await mountScreen() + const forceDialog = wrapper.findAll('dialog')[1].element as HTMLDialogElement & { showModal: () => void } + forceDialog.showModal = vi.fn() + await wrapper.get(`[data-test="${trigger}"]`).trigger('click') + await flushPromises() + expect(forceDialog.showModal).toHaveBeenCalled() + await wrapper.findAll('dialog')[1].get('button.btn-error').trigger('click') + await flushPromises() + const expected = trigger === 'review-approve-all' ? [] : ['write_file', 'remove_file', 'implicit'] + expect(api.securityApprove).toHaveBeenNthCalledWith(1, 'fixture', false, expected) + expect(api.securityApprove).toHaveBeenNthCalledWith(2, 'fixture', true, expected) + } + }) + + it('a review-changed reload keeps an explicit uncheck and an explicit check of an unchanged tool', async () => { + const wrapper = await mountScreen() + await wrapper.get('[data-test="review-allow-read_file"]').setValue(false) + await wrapper.get('[data-test="review-allow-write_file"]').setValue(true) + window.dispatchEvent(new CustomEvent('mcpproxy:review-changed')) + await flushPromises() + expect(checked(wrapper, 'read_file')).toBe(false) + expect(checked(wrapper, 'write_file')).toBe(true) + expect(wrapper.get('[data-test="review-approve-server"]').text()).toBe('Approve server (2 of 5 tools)') + }) + + it('a reload drops an explicit check when the tool changed underneath it', async () => { + const wrapper = await mountScreen() + await wrapper.get('[data-test="review-allow-write_file"]').setValue(true) + const changed = fixtureTools().map(t => t.name === 'write_file' ? { ...t, description: 'now also deletes things' } : t) + ;(api.getServerReview as any).mockResolvedValue(payload(changed)) + window.dispatchEvent(new CustomEvent('mcpproxy:review-changed')) + await flushPromises() + expect(checked(wrapper, 'write_file')).toBe(false) + }) + + it('choices are cleared after a successful approval', async () => { + const wrapper = await mountScreen() + await wrapper.get('[data-test="review-allow-read_file"]').setValue(false) + await wrapper.get('[data-test="review-approve-server"]').trigger('click') + await flushPromises() + window.dispatchEvent(new CustomEvent('mcpproxy:review-changed')) + await flushPromises() + expect(checked(wrapper, 'read_file')).toBe(true) + }) +}) diff --git a/frontend/tests/unit/review-screen.spec.ts b/frontend/tests/unit/review-screen.spec.ts index 05b173cc6..7b16df3b1 100644 --- a/frontend/tests/unit/review-screen.spec.ts +++ b/frontend/tests/unit/review-screen.spec.ts @@ -13,11 +13,11 @@ const review = (definitionsCaptured = true) => ({ data: { server: { name: 'fixture', transport: 'stdio', command: 'node ./fixture.js', trust_mode: 'manual', source_registry_id: 'official', source_registry_provenance: 'catalog', quarantined: true, definitions_captured: definitionsCaptured }, tools: [ - { name: 'read_file', description: 'read', tier: 'read', approval_status: 'pending', disabled: false, scan_verdict: 'clean' }, - { name: 'write_file', description: 'write', tier: 'write', approval_status: 'pending', disabled: false, scan_verdict: 'clean' }, - { name: 'remove_file', description: 'remove', tier: 'destructive', approval_status: 'changed', disabled: false, scan_verdict: 'warnings', diff: { description: '- old\n+ new' } }, - { name: 'implicit', description: 'implicit', tier: 'unannotated', approval_status: 'pending', disabled: false, scan_verdict: 'clean' }, - { name: 'legacy', description: 'legacy', tier: 'unknown', approval_status: 'pending', disabled: false, scan_verdict: 'clean' }, + { name: 'read_file', description: 'read', tier: 'read', approval_status: 'pending', disabled: false, scan_verdict: 'clean', default_allowed: true }, + { name: 'write_file', description: 'write', tier: 'write', approval_status: 'pending', disabled: false, scan_verdict: 'clean', default_allowed: false }, + { name: 'remove_file', description: 'remove', tier: 'destructive', approval_status: 'changed', disabled: false, scan_verdict: 'warnings', default_allowed: false, diff: { description: '- old\n+ new' } }, + { name: 'implicit', description: 'implicit', tier: 'unannotated', approval_status: 'pending', disabled: false, scan_verdict: 'clean', default_allowed: false }, + { name: 'legacy', description: 'legacy', tier: 'unknown', approval_status: 'pending', disabled: false, scan_verdict: 'clean', default_allowed: false }, ], }, }) @@ -51,10 +51,11 @@ describe('ReviewScreen (T086)', () => { it('sends unchecked tools as the block selection', async () => { const wrapper = await mountScreen() - await wrapper.get('[data-test="review-allow-remove_file"]').setValue(false) + // Only read_file starts checked (default_allowed); checking write_file takes it off the block list. + await wrapper.get('[data-test="review-allow-write_file"]').setValue(true) await wrapper.get('[data-test="review-approve-server"]').trigger('click') await flushPromises() - expect(api.securityApprove).toHaveBeenCalledWith('fixture', false, ['remove_file']) + expect(api.securityApprove).toHaveBeenCalledWith('fixture', false, ['remove_file', 'implicit', 'legacy']) }) it('offers definition capture and asks for a blind-approval confirmation', async () => { @@ -83,7 +84,7 @@ describe('ReviewScreen (T086)', () => { expect(wrapper.findAll('article[data-test^="review-tool-"]')).toHaveLength(1) await wrapper.get('[data-test="review-approve-server"]').trigger('click') await flushPromises() - expect(api.securityApprove).toHaveBeenCalledWith('fixture', false, []) + expect(api.securityApprove).toHaveBeenCalledWith('fixture', false, ['write_file', 'remove_file', 'implicit', 'legacy']) expect(api.listScanHistory).toHaveBeenCalled() }) @@ -97,7 +98,7 @@ describe('ReviewScreen (T086)', () => { expect(forceDialog.showModal).toHaveBeenCalled() await wrapper.findAll('dialog')[1].get('button.btn-error').trigger('click') await flushPromises() - expect(api.securityApprove).toHaveBeenNthCalledWith(2, 'fixture', true, []) + expect(api.securityApprove).toHaveBeenNthCalledWith(2, 'fixture', true, ['write_file', 'remove_file', 'implicit', 'legacy']) }) it('keeps fleet scan start and progress controls reachable from the review flow', async () => { diff --git a/native/macos/MCPProxy/MCPProxy/API/Models.swift b/native/macos/MCPProxy/MCPProxy/API/Models.swift index aeefdc99b..287d679c4 100644 --- a/native/macos/MCPProxy/MCPProxy/API/Models.swift +++ b/native/macos/MCPProxy/MCPProxy/API/Models.swift @@ -391,7 +391,7 @@ struct ReviewScan: Codable, Equatable { struct ReviewServerSummary: Codable, Equatable { let name: String; let transport: String?; let command: String?; let url: String?; let quarantined: Bool; let trustMode: String?; let sourceRegistryID: String?; let sourceRegistryProvenance: String?; let definitionsCaptured: Bool; let scan: ReviewScan?; enum CodingKeys: String, CodingKey { case name, transport, command, url, quarantined, scan; case trustMode = "trust_mode"; case sourceRegistryID = "source_registry_id"; case sourceRegistryProvenance = "source_registry_provenance"; case definitionsCaptured = "definitions_captured" } } struct ReviewToolPrevious: Codable, Equatable { let description: String; let inputSchema: JSONValue?; let outputSchema: JSONValue?; let annotations: JSONValue?; enum CodingKeys: String, CodingKey { case description, annotations; case inputSchema = "input_schema"; case outputSchema = "output_schema" } } struct ReviewToolDiff: Codable, Equatable { let description: String?; let inputSchema: String?; let outputSchema: String?; let annotations: String?; enum CodingKeys: String, CodingKey { case description, annotations; case inputSchema = "input_schema"; case outputSchema = "output_schema" } } -struct ReviewTool: Codable, Equatable, Identifiable { let name: String; let description: String; let inputSchema: JSONValue?; let outputSchema: JSONValue?; let annotations: JSONValue?; let tier: String; let approvalStatus: String; let disabled: Bool; let scanVerdict: String; let previous: ReviewToolPrevious?; let diff: ReviewToolDiff?; enum CodingKeys: String, CodingKey { case name, description, annotations, tier, disabled, previous, diff; case inputSchema = "input_schema"; case outputSchema = "output_schema"; case approvalStatus = "approval_status"; case scanVerdict = "scan_verdict" }; var id: String { name } } +struct ReviewTool: Codable, Equatable, Identifiable { let name: String; let description: String; let inputSchema: JSONValue?; let outputSchema: JSONValue?; let annotations: JSONValue?; let tier: String; let approvalStatus: String; let disabled: Bool; let scanVerdict: String; let defaultAllowed: Bool?; let previous: ReviewToolPrevious?; let diff: ReviewToolDiff?; enum CodingKeys: String, CodingKey { case name, description, annotations, tier, disabled, previous, diff; case inputSchema = "input_schema"; case outputSchema = "output_schema"; case approvalStatus = "approval_status"; case scanVerdict = "scan_verdict"; case defaultAllowed = "default_allowed" }; var id: String { name } } struct ServerReviewResponse: Codable, Equatable { let server: ReviewServerSummary; let tools: [ReviewTool] } // MARK: - OAuth Status diff --git a/native/macos/MCPProxy/MCPProxy/Views/ReviewQueueView.swift b/native/macos/MCPProxy/MCPProxy/Views/ReviewQueueView.swift index 1351889a9..d16d5a801 100644 --- a/native/macos/MCPProxy/MCPProxy/Views/ReviewQueueView.swift +++ b/native/macos/MCPProxy/MCPProxy/Views/ReviewQueueView.swift @@ -104,6 +104,38 @@ enum ReviewPresentation { return Headline(state: .approved, title: "\(name) is approved", subtitle: "\(summary) New or changed tools come back here for review.") } + // MARK: Default selection (Spec 109 fix-review-defaults, D41) + // The core decides which tools start checked (`default_allowed`); these + // helpers only read it. A missing field (an older core) reads as false, so a + // mismatched core fails closed. Sentences match the Web screen. + + static let selectionHint = "Only read-only tools with a clean scan start checked. Unchecked tools stay blocked after approval until you enable them on the Tools tab." + + /// What the user explicitly chose for one tool, with the payload they saw. + struct Choice: Equatable { let allowed: Bool; let tool: ReviewTool } + + static func initialSelection(_ tools: [ReviewTool]) -> Set { + Set(tools.filter { $0.defaultAllowed == true }.map(\.name)) + } + + /// The selection after a reload: an explicit uncheck always survives; an + /// explicit check survives only while the tool's payload is the one the user + /// saw (a changed definition, verdict or tier falls back to the default). + static func mergeSelection(_ tools: [ReviewTool], choices: [String: Choice]) -> Set { + Set(tools.filter { tool in + guard let choice = choices[tool.name] else { return tool.defaultAllowed == true } + if !choice.allowed { return false } + return choice.tool == tool ? true : tool.defaultAllowed == true + }.map(\.name)) + } + + static func approveLabel(selected: Int, total: Int, definitionsCaptured: Bool) -> String { + if !definitionsCaptured || total == 0 { return "Approve Without Seeing Tools" } + return "Approve Server (\(selected) of \(total) \(total == 1 ? "tool" : "tools"))" + } + + static func approveAllLabel(total: Int) -> String { "Approve All (\(total) \(total == 1 ? "tool" : "tools"))" } + enum ToolControl: Equatable { case allowToggle, approveReject, approved, blocked } /// The control a tool row gets: the quarantine toggle, Approve/Reject, or a plain state. @@ -120,6 +152,8 @@ struct ReviewSheet: View { let onDismiss: () -> Void @State private var review: ServerReviewResponse? @State private var allowed = Set() + @State private var choices: [String: ReviewPresentation.Choice] = [:] + @State private var pendingBlock: [String]? @State private var error: String? @State private var scanning = false @State private var showBlindApprovalConfirmation = false @@ -171,6 +205,8 @@ struct ReviewSheet: View { set: { isAllowed in if isAllowed { allowed.insert(tool.name) } else { allowed.remove(tool.name) } + // An explicit click survives a reload (D41.4). + choices[tool.name] = ReviewPresentation.Choice(allowed: isAllowed, tool: tool) } )).toggleStyle(.checkbox) } else { @@ -188,8 +224,14 @@ struct ReviewSheet: View { Spacer() } if review?.server.quarantined == true { + if let review, review.server.definitionsCaptured, !review.tools.isEmpty { + Text(ReviewPresentation.selectionHint).font(.caption).foregroundStyle(.secondary).padding(.horizontal) + } HStack { - Button("Approve Server (\(allowed.count) tools)") { requestApprove() }.buttonStyle(.borderedProminent) + Button(ReviewPresentation.approveLabel(selected: allowed.count, total: review?.tools.count ?? 0, definitionsCaptured: review?.server.definitionsCaptured ?? false)) { requestApprove(everything: false) }.buttonStyle(.borderedProminent) + if let review, review.server.definitionsCaptured, !review.tools.isEmpty, allowed.count < review.tools.count { + Button(ReviewPresentation.approveAllLabel(total: review.tools.count)) { requestApprove(everything: true) } + } Button("Reject Server", role: .destructive) { Task { await rejectServer() } } }.padding() } else if headline?.state == .approved { @@ -238,12 +280,19 @@ struct ReviewSheet: View { } private func load() async { guard let client = appState.apiClient else { return } - do { let value = try await client.serverReview(serverName); review = value; allowed = Set(value.tools.filter { !$0.disabled }.map(\.name)) } catch { self.error = error.localizedDescription } + do { let value = try await client.serverReview(serverName); review = value; allowed = ReviewPresentation.mergeSelection(value.tools, choices: choices) } catch { self.error = error.localizedDescription } + } + private func requestApprove(everything: Bool) { + if review?.server.definitionsCaptured == false { pendingBlock = nil; showBlindApprovalConfirmation = true; return } + pendingBlock = everything ? [] : nil + Task { await approve(force: false) } } - private func requestApprove() { if review?.server.definitionsCaptured == false { showBlindApprovalConfirmation = true } else { Task { await approve(force: false) } } } + /// The force retry re-sends the block list of the attempt that triggered it (D41.5). private func approve(force: Bool) async { guard let client = appState.apiClient, let review else { return } - do { try await client.securityApproveServer(serverName, force: force, block: review.tools.map(\.name).filter { !allowed.contains($0) }); await load() } + let block = pendingBlock ?? review.tools.map(\.name).filter { !allowed.contains($0) } + pendingBlock = block + do { try await client.securityApproveServer(serverName, force: force, block: block); choices = [:]; pendingBlock = nil; await load() } catch { self.error = error.localizedDescription; if !force, case let APIClientError.httpError(status, message) = error, status == 409, message.localizedCaseInsensitiveContains("dangerous") { showForceApprovalConfirmation = true } } } private func fetchDefinitions() async { diff --git a/native/macos/MCPProxy/MCPProxyTests/ReviewPayloadTests.swift b/native/macos/MCPProxy/MCPProxyTests/ReviewPayloadTests.swift index 4fb2ae937..bc25bbbc5 100644 --- a/native/macos/MCPProxy/MCPProxyTests/ReviewPayloadTests.swift +++ b/native/macos/MCPProxy/MCPProxyTests/ReviewPayloadTests.swift @@ -25,6 +25,15 @@ final class ReviewPayloadTests: XCTestCase { XCTAssertEqual(review.server.command, "node fixture.js") XCTAssertEqual(review.server.trustMode, "manual") XCTAssertEqual(review.server.sourceRegistryID, "official") + // A payload from an older core has no default_allowed and still decodes. + XCTAssertNil(review.tools.first?.defaultAllowed) + } + + func testReviewToolDecodesDefaultAllowed() throws { + let review = try JSONDecoder().decode(ServerReviewResponse.self, from: Data(""" + {"server":{"name":"fixture","quarantined":true,"definitions_captured":true},"tools":[{"name":"read_file","description":"d","tier":"read","approval_status":"pending","disabled":false,"scan_verdict":"clean","default_allowed":true},{"name":"write_file","description":"d","tier":"write","approval_status":"pending","disabled":false,"scan_verdict":"clean","default_allowed":false}]} + """.utf8)) + XCTAssertEqual(review.tools.map(\.defaultAllowed), [true, false]) } func testReviewQueueUsesDedicatedSidebarAndSheet() throws { diff --git a/native/macos/MCPProxy/MCPProxyTests/ReviewPresentationTests.swift b/native/macos/MCPProxy/MCPProxyTests/ReviewPresentationTests.swift index 7d07d5228..f3c1b184c 100644 --- a/native/macos/MCPProxy/MCPProxyTests/ReviewPresentationTests.swift +++ b/native/macos/MCPProxy/MCPProxyTests/ReviewPresentationTests.swift @@ -125,6 +125,61 @@ final class ReviewPresentationTests: XCTestCase { XCTAssertFalse(source.contains(#"/api/v1/servers/\(id)/quarantine"#)) } + // MARK: Default selection (Spec 109 fix-review-defaults, D41) + + private func selectionTool(_ name: String, defaultAllowed: Bool?, description: String = "d", verdict: String = "clean") throws -> ReviewTool { + let field = defaultAllowed.map { ",\"default_allowed\":\($0)" } ?? "" + let json = "{\"name\":\"\(name)\",\"description\":\"\(description)\",\"tier\":\"read\",\"approval_status\":\"pending\",\"disabled\":false,\"scan_verdict\":\"\(verdict)\"\(field)}" + return try JSONDecoder().decode(ReviewTool.self, from: Data(json.utf8)) + } + + func testDefaultSelectionFollowsDefaultAllowed() throws { + let tools = [ + try selectionTool("read_a", defaultAllowed: true), + try selectionTool("write_a", defaultAllowed: false), + // An older core sends no field: it reads as false, so a mismatched core fails closed. + try selectionTool("old_core", defaultAllowed: nil), + ] + XCTAssertEqual(tools[0].defaultAllowed, true) + XCTAssertNil(tools[2].defaultAllowed) + XCTAssertEqual(ReviewPresentation.initialSelection(tools), ["read_a"]) + } + + func testMergeSelectionKeepsUnchecksAndDropsStaleChecks() throws { + let readA = try selectionTool("read_a", defaultAllowed: true) + let writeA = try selectionTool("write_a", defaultAllowed: false) + + // An explicit uncheck always survives a reload. + XCTAssertEqual(ReviewPresentation.mergeSelection([readA], choices: ["read_a": .init(allowed: false, tool: readA)]), []) + // An explicit check survives while the payload is the one the user saw. + XCTAssertEqual(ReviewPresentation.mergeSelection([writeA], choices: ["write_a": .init(allowed: true, tool: writeA)]), ["write_a"]) + // A changed definition or verdict falls back to the default. + let redefined = try selectionTool("write_a", defaultAllowed: false, description: "now also deletes") + XCTAssertEqual(ReviewPresentation.mergeSelection([redefined], choices: ["write_a": .init(allowed: true, tool: writeA)]), []) + let rescanned = try selectionTool("write_a", defaultAllowed: false, verdict: "warnings") + XCTAssertEqual(ReviewPresentation.mergeSelection([rescanned], choices: ["write_a": .init(allowed: true, tool: writeA)]), []) + // A choice for a tool that is gone is ignored. + XCTAssertEqual(ReviewPresentation.mergeSelection([readA], choices: ["gone": .init(allowed: true, tool: writeA)]), ["read_a"]) + } + + func testApproveLabels() { + XCTAssertEqual(ReviewPresentation.approveLabel(selected: 3, total: 9, definitionsCaptured: true), "Approve Server (3 of 9 tools)") + XCTAssertEqual(ReviewPresentation.approveLabel(selected: 0, total: 9, definitionsCaptured: true), "Approve Server (0 of 9 tools)") + XCTAssertEqual(ReviewPresentation.approveLabel(selected: 1, total: 1, definitionsCaptured: true), "Approve Server (1 of 1 tool)") + XCTAssertEqual(ReviewPresentation.approveLabel(selected: 0, total: 0, definitionsCaptured: true), "Approve Without Seeing Tools") + XCTAssertEqual(ReviewPresentation.approveLabel(selected: 0, total: 5, definitionsCaptured: false), "Approve Without Seeing Tools") + XCTAssertEqual(ReviewPresentation.approveAllLabel(total: 9), "Approve All (9 tools)") + XCTAssertEqual(ReviewPresentation.approveAllLabel(total: 1), "Approve All (1 tool)") + } + + func testSelectionHintMatchesWeb() throws { + var url = URL(fileURLWithPath: #filePath) + for _ in 0..<5 { url.deleteLastPathComponent() } + let web = try String(contentsOf: url.appendingPathComponent("frontend/src/utils/reviewPresentation.ts")) + XCTAssertTrue(web.contains("'\(ReviewPresentation.selectionHint)'"), "the macOS hint must be the Web sentence") + XCTAssertEqual(ReviewPresentation.selectionHint, "Only read-only tools with a clean scan start checked. Unchecked tools stay blocked after approval until you enable them on the Tools tab.") + } + func testToolStateSelectsTheControl() throws { let tools = try review(quarantined: false, tools: [("p", "pending", false), ("c", "changed", false), ("a", "approved", false), ("b", "approved", true)]).tools XCTAssertEqual(ReviewPresentation.toolState(tools[0], quarantined: false), .approveReject) From 02a09864731fd3135a9eceb13a28ba1b08376696 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 2 Oct 2026 22:36:43 +0300 Subject: [PATCH 3/6] docs(spec-109): D41 default selection, contracts, CLI and quarantine docs (Spec fix-review-defaults) --- ROADMAP.md | 2 +- docs/api/rest-api.md | 1 + docs/cli/command-reference.md | 2 +- docs/cli/review-commands.md | 19 ++++++++++++++----- docs/features/security-quarantine.md | 19 +++++++++++++------ .../acceptance-index.json | 5 +++++ .../contracts/cli.md | 2 +- .../contracts/mcp-tools.md | 2 +- .../contracts/rest-api.md | 3 +++ .../data-model.md | 2 ++ .../parity-matrix.json | 11 ++++++++--- specs/109-ux-navigation-consistency/plan.md | 1 + .../quickstart.md | 1 + .../109-ux-navigation-consistency/research.md | 15 +++++++++++++++ specs/109-ux-navigation-consistency/spec.md | 6 +++--- specs/109-ux-navigation-consistency/tasks.md | 19 ++++++++++++++++++- 16 files changed, 88 insertions(+), 22 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 88499a9b0..8ba3b4b27 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -1036,6 +1036,6 @@ Legend: `shipped` ≥95% checked · `in-flight` 1–94% · `drafted` 0% · `—` | [106-security-residual-fixes](./specs/106-security-residual-fixes/) | `shipped` | 18/19 (95%) | | [107-server-edition-sso-hardening](./specs/107-server-edition-sso-hardening/) | `shipped` | 126/126 (100%) | | [108-profiles-v3](./specs/108-profiles-v3/) | `shipped` | 185/186 (99%) | -| [109-ux-navigation-consistency](./specs/109-ux-navigation-consistency/) | `shipped` | 233/234 (100%) | +| [109-ux-navigation-consistency](./specs/109-ux-navigation-consistency/) | `shipped` | 245/246 (100%) | | [110-catalog-popularity](./specs/110-catalog-popularity/) | `in-flight` | 19/23 (83%) | | [112-client-header-forwarding](./specs/112-client-header-forwarding/) | `shipped` | 38/40 (95%) | diff --git a/docs/api/rest-api.md b/docs/api/rest-api.md index e3a83ecb9..9c59cead4 100644 --- a/docs/api/rest-api.md +++ b/docs/api/rest-api.md @@ -957,6 +957,7 @@ The review payload of one server: a summary of the server (secrets in the URL, h - `tier` is `read`, `write`, `destructive`, `unannotated` (annotations captured, no hints) or `unknown` (nothing captured, a record from before the review screen). It comes from one function, so the Web UI, macOS, `mcpproxy tools list --tier` and the MCP `quarantine_security` inspect operations show the same value. - `approval_status` is `approved`, `pending` (shown as "New, needs review") or `changed` ("Changed, needs review"). - `scan.coverage` says whether the scan verdict describes the definitions in the payload: `current` (the latest completed scan analysed every captured definition as it is now), `stale` (some definitions were added or changed after that scan; `scan.unscanned_tools` lists them), `not_captured` (no definitions captured), `tools_not_scanned` (the scan completed but exported no tool definitions), `scanning` (a scan is running) or `none` (no completed scan). Show `risk_score` only for `current`. `scan.tools_scanned` is the number of definitions that scan exported. +- `default_allowed` (always present) is the review screens' fail-closed default selection: `true` for an approved tool, or for a pending or changed `read` tool with `scan_verdict` `clean` that is not held; `false` for everything else (write, destructive, unannotated, unknown, not scanned, warnings, dangerous, held, or already disabled). A client that finds no such field (an older core) treats it as `false`. - `scan_verdict` is `dangerous`, `warnings`, `clean` or `not_scanned`. `clean` means the latest scan covered this tool's current definition and found nothing; a tool whose definition changed after the scan is `not_scanned` (or carries its held verdict). - `definitions_captured: false` returns `tools: []`; `POST /api/v1/servers/{id}/discover-tools` captures the definitions without indexing them. After a baseline scan has listed a still-quarantined server's tools, MCPProxy runs the same capture itself. - Descriptions are returned verbatim and must be rendered as inert text. diff --git a/docs/cli/command-reference.md b/docs/cli/command-reference.md index ac7938922..0310834f3 100644 --- a/docs/cli/command-reference.md +++ b/docs/cli/command-reference.md @@ -172,7 +172,7 @@ Review quarantined servers and new or changed tools through the scan gate. See [ ```bash mcpproxy review list mcpproxy review show [--full] -mcpproxy review approve [--tools a,b] [--except a,b] [--force] [--yes] +mcpproxy review approve [--all | --tools a,b] [--except a,b] [--force] [--yes] mcpproxy review reject [--tools a,b] [--yes] ``` diff --git a/docs/cli/review-commands.md b/docs/cli/review-commands.md index e257644c6..7d51b01f7 100644 --- a/docs/cli/review-commands.md +++ b/docs/cli/review-commands.md @@ -66,13 +66,16 @@ Descriptions and schemas come from the upstream server and are shown as plain te ## review approve ```bash -mcpproxy review approve [--tools a,b] [--except a,b] [--force] [--yes] +mcpproxy review approve [--all | --tools a,b] [--except a,b] [--force] [--yes] ``` +On a quarantined server the command allows the same default selection as the Web and macOS review screens: only read-only tools with a clean scan (`default_allowed` in the review payload). Every other tool is blocked, meaning approved and disabled, until you enable it on the Tools tab. A core that does not send `default_allowed` blocks every tool. + | Flag | Meaning | |------|---------| -| `--except a,b` | Quarantined server only: block these tools while approving the server | -| `--tools a,b` | Trusted server only: approve only these pending or changed tools | +| `--all` | Approve every tool (the Web and macOS "Approve all"). On a trusted server this is the default and changes nothing | +| `--tools a,b` | Quarantined server: allow exactly these tools and block the rest. Trusted server: approve only these pending or changed tools | +| `--except a,b` | Quarantined server only: block these tools as well, whichever base was chosen | | `--force` | Approve although the scan verdict is dangerous (use only after reading the findings) | | `--yes` | Skip the confirmation prompt | @@ -80,19 +83,25 @@ The command picks the right endpoint for you: | Server state | Endpoint | Notes | |--------------|----------|-------| -| Quarantined | `POST /api/v1/servers/{id}/security/approve` | `--except` becomes `block`; `--force` is sent as `force` | +| Quarantined | `POST /api/v1/servers/{id}/security/approve` | `block` is every tool outside the selection (default, `--all` or `--tools`, minus `--except`); `--force` is sent as `force` | | Trusted (not quarantined) | `POST /api/v1/servers/{id}/tools/approve` | `--tools` selects tools; without it every pending or changed tool is approved | Using the wrong flag for the state fails with exit code 1 instead of doing something else: -- `--tools` on a quarantined server: `--tools cannot select a quarantined server approval; use --except to block tools` +- `--all` together with `--tools`: `--all cannot be combined with --tools` +- A tool name that is not in the review (`--tools` or `--except`): `unknown tool 'x' for server 's'`; 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. + +`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. + ```bash mcpproxy review approve filesystem --except delete_0 --force --yes ``` ``` +Allowing 0 of 1 tool; blocking 1: delete_0 Approved server filesystem ``` diff --git a/docs/features/security-quarantine.md b/docs/features/security-quarantine.md index 00c287094..b222ece4e 100644 --- a/docs/features/security-quarantine.md +++ b/docs/features/security-quarantine.md @@ -269,10 +269,15 @@ mcpproxy review show github [--full] **Web UI:** 1. Select the quarantined server from **Review queue**. -2. Choose the tools to allow; unselected tools are submitted as explicit - blocks with the approval decision. -3. Choose **Approve server**. If no tool definitions have been captured, the - UI asks for a separate confirmation before a blind approval can proceed. +2. Choose the tools to allow. Only read-only tools whose scan is clean start + checked; write, destructive, unannotated, not-scanned and held tools start + unchecked. Unselected tools are submitted as explicit blocks with the + approval decision and stay blocked until you enable them on the Tools tab. +3. Choose **Approve server**; the button names the exact count (for example + "Approve server (3 of 9 tools)"). **Approve all** is a separate action that + allows every tool. If no tool definitions have been captured, the button + reads "Approve without seeing tools" and the UI asks for a separate + confirmation before a blind approval can proceed. The review controls are deliberate: **Fetch tool definitions** uses the inspection-only `discover-tools` capture to store current upstream metadata @@ -301,8 +306,10 @@ curl -X POST -H "X-API-Key: your-key" -H "Content-Type: application/json" \ **CLI:** ```bash -# Quarantined server: scan-gated approval; --except keeps tools disabled -mcpproxy review approve github [--except a,b] [--force] [--yes] +# Quarantined server: scan-gated approval. By default only read-only tools with +# a clean scan are allowed; --all allows every tool, --tools a,b exactly those, +# --except keeps more tools disabled +mcpproxy review approve github [--all | --tools a,b] [--except a,b] [--force] [--yes] # Trusted server: approve only the listed new or changed tools mcpproxy review approve github --tools create_issue diff --git a/specs/109-ux-navigation-consistency/acceptance-index.json b/specs/109-ux-navigation-consistency/acceptance-index.json index a3ac30ea7..10e98cdc3 100644 --- a/specs/109-ux-navigation-consistency/acceptance-index.json +++ b/specs/109-ux-navigation-consistency/acceptance-index.json @@ -56,6 +56,10 @@ "summary": "Approve server with unchecked tools runs the scan-gated approval and blocks the rest", "tests": [ "vitest:frontend/tests/unit/review-screen.spec.ts", + "go:internal/runtime/review_default_allowed_test.go#TestReviewDefaultAllowed", + "vitest:frontend/tests/unit/review-screen-default-selection.spec.ts", + "xctest:native/macos/MCPProxy/MCPProxyTests/ReviewPresentationTests.swift#testApproveLabels", + "go:cmd/mcpproxy/review_cmd_default_selection_test.go#TestReviewApproveQuarantinedUsesDefaultSelection", "go:internal/httpapi/security_scanner_test.go#TestSecurityHandlerApproveServerWithBlocks", "go:internal/storage/scanner_test.go#TestSaveIntegrityBaselineWithBlocksIsAtomic", "go:internal/security/scanner/service_test.go#TestServiceApproveServerWithBlocksPersistsBeforeUnquarantine" @@ -82,6 +86,7 @@ "summary": "Uncaptured definitions say so and offer Fetch tool definitions", "tests": [ "vitest:frontend/tests/unit/review-screen.spec.ts", + "vitest:frontend/tests/unit/review-screen-default-selection.spec.ts", "go:internal/runtime/review_test.go#TestReviewPayload_QueueAndUncapturedDefinitions", "go:internal/server/mcp_quarantine_discovery_test.go", "go:internal/runtime/review_capture_test.go#TestShouldCaptureReviewDefinitionsAfterScan", diff --git a/specs/109-ux-navigation-consistency/contracts/cli.md b/specs/109-ux-navigation-consistency/contracts/cli.md index 976daeef3..3c3605a49 100644 --- a/specs/109-ux-navigation-consistency/contracts/cli.md +++ b/specs/109-ux-navigation-consistency/contracts/cli.md @@ -9,7 +9,7 @@ Every new command honours the global `-o table|json|yaml`, `--json`, `MCPPROXY_O | `mcpproxy attention` | `GET /attention` | `# KIND SUBJECT SUMMARY FIX`; `All clear` when empty. Exit 0 in both cases (it is a report, not a check) | | `mcpproxy review list` | `GET /review` | `SERVER KIND QUARANTINED PENDING CHANGED TIERS SCAN` | | `mcpproxy review show ` | `GET /servers/{id}/review` | server header block, then `TOOL TIER APPROVAL SCAN DESCRIPTION (first line)`; `--full` prints full descriptions and diffs, indented under a `from the server, not verified:` label | -| `mcpproxy review approve [--tools a,b \| --except a,b] [--force] [--yes]` | quarantined → `security/approve` (`block` = `--except`); trusted → `tools/approve` (`--tools` or all pending/changed) | `Approved (12 tools, 2 blocked)`. A dangerous verdict without `--force` → exit 1 with the Spec 077 message | +| `mcpproxy review approve [--all \| --tools a,b] [--except a,b] [--force] [--yes]` | quarantined → `security/approve` (`block` = the tools outside the base, which is the core's default selection, `--all` or `--tools`, plus `--except`; unknown names and `--all` with `--tools` are errors before any write); trusted → `tools/approve` (`--tools`, or all pending/changed; `--all` is an alias of that default; `--except` is rejected) | `Approved (12 tools, 2 blocked)`. A dangerous verdict without `--force` → exit 1 with the Spec 077 message | | `mcpproxy review reject [--tools a,b] [--yes]` | no `--tools` → `security/reject`; `--tools` → `tools/block` | `Rejected …` | | `mcpproxy client list` | `GET /clients` | `CLIENT STATE LAST SEEN SESSIONS CALLS 24H CONFIG PATH` (`display_path`). Spec 108 **appends** `CREDENTIAL PROFILE MODE SOURCE BLOCKED 24H` and `--profile`; it never removes or reorders these columns (`CONFIG PATH` stays) | | `mcpproxy client show ` | `GET /clients/{id}` | detail block incl. `reload_hint`, full `config_path`, sessions | diff --git a/specs/109-ux-navigation-consistency/contracts/mcp-tools.md b/specs/109-ux-navigation-consistency/contracts/mcp-tools.md index 63eee6307..8f32aa53a 100644 --- a/specs/109-ux-navigation-consistency/contracts/mcp-tools.md +++ b/specs/109-ux-navigation-consistency/contracts/mcp-tools.md @@ -5,7 +5,7 @@ Small by design. MCP is a client surface with per-tool authorization. Search and | Tool | Change | PR | Golden impact | |---|---|---|---| | `upstream_servers` `list` | each server's `health` gains `status`, `usable`, `actions` (shared struct) | 109-c | list-output goldens only (the tool schema is unchanged) | -| `quarantine_security` `inspect_quarantined`, `inspect_tools` | each tool gains `tier`, `annotations`, `scan_verdict` with the names and values of `GET /servers/{id}/review`; changed tools gain `previous` + `diff`; any server `command`/`url` in the output comes from the same redacted review composer (FR-021), never raw config. **The live inspection is kept**: when the composer reports `definitions_captured: false`, `inspect_quarantined` still performs today's temporary-exemption connect + `ListTools()` (`internal/server/mcp.go` ~4879–5018) and decorates those live tools with `tier` from `contracts.AnnotationTier` over their live annotations and `scan_verdict: "not_scanned"`, plus `definitions_source: "live"` (`"captured"` otherwise) — it never degrades to the composer's `tools: []`. `server_summary.scan` carries the same `coverage`, `tools_scanned` and `unscanned_tools` as the REST review (fix-review-screen) | 109-f | output goldens only | +| `quarantine_security` `inspect_quarantined`, `inspect_tools` | each tool gains `tier`, `annotations`, `scan_verdict` with the names and values of `GET /servers/{id}/review`; changed tools gain `previous` + `diff`; any server `command`/`url` in the output comes from the same redacted review composer (FR-021), never raw config. **The live inspection is kept**: when the composer reports `definitions_captured: false`, `inspect_quarantined` still performs today's temporary-exemption connect + `ListTools()` (`internal/server/mcp.go` ~4879–5018) and decorates those live tools with `tier` from `contracts.AnnotationTier` over their live annotations and `scan_verdict: "not_scanned"`, plus `definitions_source: "live"` (`"captured"` otherwise) — it never degrades to the composer's `tools: []`. `server_summary.scan` carries the same `coverage`, `tools_scanned` and `unscanned_tools` as the REST review (fix-review-screen). Captured tools carry `default_allowed` (the composer value); live tools carry `default_allowed: false`; `inspect_tools` does not carry it (fix-review-defaults, D41.7). No `quarantine_security` operation approves a server, and `approve_tool` and `approve_all_tools` approve only what they name | 109-f | output goldens only | | `search_servers` | `registry` becomes optional (omitted = all sources through `registries.SearchAll`); results gain `title`, `publisher`, `verified`, `official`, `popularity`, `source`, in the FR-060 order (research D37; the tool description text still says "official-first" and is frozen by the schema goldens, so changing it is a follow-up), and `from_cache: true` when a source's cached listing answered (its `unavailable[]` entry then carries `fallback` and `cached_at`, D35); descriptions say "catalog" and "catalog source". The retained `tag` parameter accepts only an empty value; a non-empty value returns a visible error because catalog entries carry no tags. | 109-j | **schema golden changes** (`registry` no longer `required`; description text). Declared in the 109-j PR body | | `list_registries` | description wording "catalog sources" | 109-j | schema golden (description text) | diff --git a/specs/109-ux-navigation-consistency/contracts/rest-api.md b/specs/109-ux-navigation-consistency/contracts/rest-api.md index 5cfdd1497..d3d520ca0 100644 --- a/specs/109-ux-navigation-consistency/contracts/rest-api.md +++ b/specs/109-ux-navigation-consistency/contracts/rest-api.md @@ -127,6 +127,7 @@ One row per server awaiting review: a quarantined server (`kind: server_review`) "disabled": false, "scan_verdict": "clean", "held_reason": "", "held_signals": [], + "default_allowed": false, "previous": null }, { @@ -136,6 +137,7 @@ One row per server awaiting review: a quarantined server (`kind: server_review`) "tier": "unknown", "approval_status": "changed", "scan_verdict": "warnings", + "default_allowed": false, "previous": {"description": "…", "input_schema": {}, "annotations": null}, "diff": {"description": "@@ -1 +1 @@\n-…\n+…", "input_schema": "", "annotations": ""} } @@ -147,6 +149,7 @@ One row per server awaiting review: a quarantined server (`kind: server_review`) - `annotations`/`tier` pairs: `annotations: null` → `tier: "unknown"` (nothing captured — a record from before this spec, as in the `search_code` example above); `annotations: {}` → `tier: "unannotated"` (captured, no hints); otherwise `contracts.AnnotationTier`. For instance `{"name": "list_dir", "annotations": {}, "tier": "unannotated", …}`. - `source_registry_id`, `source_registry_provenance`: the existing MCP-866 origin fields of the server config, `omitempty` (absent for a manually added server). - `tier`: `unknown` when the record carries no stored annotations (captured before this spec). +- `default_allowed` (always present, fix-review-defaults, D41.2): the review screens' fail-closed default selection. `false` for a disabled tool; `true` for an `approved` tool; for `pending` or `changed` it is `true` only when `tier` is `read`, `scan_verdict` is `clean` and `held_reason` is empty. Write, destructive, unannotated, unknown, not-scanned, warnings, dangerous and held tools are `false`. A payload from an older core has no field, which surfaces read as `false` (fail closed). - `scan.coverage` (always present): `current` = the latest completed scan analysed every captured definition as it is now; `stale` = at least one captured definition was added or changed after that scan (`scan.unscanned_tools` lists them, sorted); `not_captured` = no definitions captured (`definitions_captured: false`); `tools_not_scanned` = the scan completed but exported 0 tool definitions (source-only or URL scan); `scanning` = the newest baseline job is pending or running; `none` = no completed scan (never scanned, or the newest job failed or was cancelled). Precedence: `not_captured` > `scanning` > `none` > `tools_not_scanned` > `stale` > `current`. `scan.tools_scanned` is the number of definitions that scan exported (omitted when 0). Surfaces show `risk_score` only for `current`. A tool is covered when the scan's recorded tool names (`ScanContext.tool_names`) include it and its definition did not change after the scan started (`definition_changed_at`); for a scan recorded without names, approved records are covered, pending records are covered on a quarantined server only, and a changed record with no change time is not covered. A payload from an older core has no `coverage`; surfaces read that as `none`. - `scan_verdict` per tool: `dangerous|warnings|clean|not_scanned`. A **covered** tool gets the verdict of the latest baseline report's findings for that tool, else the record's `held_verdict`, else `clean`. A tool the scan did **not** cover gets its `held_verdict` (the Spec 086 in-process check of the current definition) or `not_scanned`; findings of an older scan describe an older definition and are not applied. - `definitions_captured: false` → `tools: []`. Surfaces offer "Fetch tool definitions" = `POST /servers/{id}/discover-tools`; the explicit inspection-only capture obtains a bounded supervisor exemption, stores approval records, emits `review.changed`, and never indexes the quarantined definitions. Baseline scanning remains a separate security operation, except that after a baseline scan completes having exported tool definitions for a still-quarantined server with no records, MCPProxy runs the same capture itself (the upstream was already started and listed for that scan, so no new process is started). With `security.auto_baseline_scan: false` and no manual scan, nothing is captured automatically. diff --git a/specs/109-ux-navigation-consistency/data-model.md b/specs/109-ux-navigation-consistency/data-model.md index 245a741c9..b212a89f1 100644 --- a/specs/109-ux-navigation-consistency/data-model.md +++ b/specs/109-ux-navigation-consistency/data-model.md @@ -105,6 +105,8 @@ Spec 108 input wiring (109-l): `(*Runtime).AttentionClientWarnings()` calls `Cli Scan coverage (fix-review-screen): `ReviewScan` adds `coverage`, `tools_scanned` and `unscanned_tools`. The composer reads the newest baseline job and its `ScanContext` (`tools_exported` and the new `tool_names`: the sorted, de-duplicated names of the exported definitions, recorded by Pass 1) and compares each approval record with it: a record is covered when the scan saw its name and `definition_changed_at` is not after the job's `started_at`. A scan recorded without `tool_names` (older core) covers approved records, and pending records of a quarantined server; it does not cover a pending record of a trusted server or a changed record with no change time. The per-tool `scan_verdict` follows from coverage (`clean` only for a covered tool). +Default selection (fix-review-defaults): `ReviewTool.DefaultAllowed` (`default_allowed`, always serialised) is set by `reviewDefaultAllowed` after `ScanVerdict` and `HeldReason`: false when the tool is disabled; true when `approval_status` is `approved`; otherwise true only for tier `read` with `scan_verdict` `clean` and no `held_reason`. The Web screen, the macOS sheet and `mcpproxy review approve` read it and never recompute it; a missing field reads as false (D41). + ## 6. Client presence (derived) — `internal/runtime/clients_presence.go` ```go diff --git a/specs/109-ux-navigation-consistency/parity-matrix.json b/specs/109-ux-navigation-consistency/parity-matrix.json index 4cf7f9250..649809bb2 100644 --- a/specs/109-ux-navigation-consistency/parity-matrix.json +++ b/specs/109-ux-navigation-consistency/parity-matrix.json @@ -288,6 +288,7 @@ ], "tests": [ "vitest:frontend/tests/unit/review-screen.spec.ts", + "vitest:frontend/tests/unit/review-screen-default-selection.spec.ts", "vitest:frontend/tests/unit/server-detail-approve-dialog.spec.ts" ] }, @@ -297,7 +298,8 @@ "symbol:native/macos/MCPProxy/MCPProxy/Views/ReviewQueueView.swift#ReviewSheet" ], "tests": [ - "xctest:native/macos/MCPProxy/MCPProxyTests/ApproveServerPathTests.swift#testSecurityApprovalUsesScanGateAndForcePayload" + "xctest:native/macos/MCPProxy/MCPProxyTests/ApproveServerPathTests.swift#testSecurityApprovalUsesScanGateAndForcePayload", + "xctest:native/macos/MCPProxy/MCPProxyTests/ReviewPresentationTests.swift#testDefaultSelectionFollowsDefaultAllowed" ] }, "cli": { @@ -307,11 +309,14 @@ "cmd:review reject" ], "tests": [ - "go:cmd/mcpproxy/review_cmd_test.go" + "go:cmd/mcpproxy/review_cmd_test.go", + "go:cmd/mcpproxy/review_cmd_default_selection_test.go#TestReviewApproveAllFlag" ], "fields": [ + "all", "except", - "force" + "force", + "tools" ] }, "mcp": { diff --git a/specs/109-ux-navigation-consistency/plan.md b/specs/109-ux-navigation-consistency/plan.md index 8bac29aad..c4eb0d68e 100644 --- a/specs/109-ux-navigation-consistency/plan.md +++ b/specs/109-ux-navigation-consistency/plan.md @@ -132,6 +132,7 @@ Merge order: `(a ∥ b ∥ c) → (f ∥ j ∥ k) → (d ∥ e) → (g ∥ h) | 109-j | `109-j-catalog-add-server` | `registries.SearchAll` + `Rank` over an internal `CatalogHit`; `GET /catalog/search` returning the distinct `CatalogResult` DTO (`toCatalogResult`; `ServerEntry` JSON untouched); `/add-server` (tabs Catalog · Paste · Import · Manual via `?tab=`; `?source=` stays the catalog-source filter); paste URL/command detection; secret toggle (per-kind keyring names `-env-`/`-header-`, never overwriting an existing entry; `secret_like` = registry flag OR name rule) + keyring availability; `/repositories` redirect; Settings → Catalog sources; macOS Add Server sheet with Catalog/Paste + toggle; CLI `catalog` group, `upstream add --secret-env/--secret-header`, `registry search|add` deprecation; MCP `search_servers` registry optional | FR-007 (catalog), 060–067; N3, C1, S7, X4 | a | all | | 109-k | `109-k-activity-scope-filters` | `useScopeQuery` with **all** parameters (`profile`/`client`/`token` registered hidden until `features.scope_filters` — merged here from Spec 108's former 108-j), the full link map incl. its hidden 108-target rows; Activity views + `/sessions` redirect + folding + column hiding + block reason + Duration; Tools/Usage/Servers scope-aware (Review follows in 109-g); chart-bar and Tools-row deep links (server-card links are 109-e's, Home-strip links 109-d's); token-savings estimate; macOS `ScopeFilter` + Activity segments/filters; CLI `--view`, `--from/--to` (sole owner; Spec 108-e does not register them); backend `unsupported_scope_filter` gate for `profile`/`client`/`token` (and `agent` where no handler honours it) until Spec 108-e fills the supported list (`internal/httpapi/scope_filters.go`; list and gate function created by whichever of 109-k/108-e merges first); `tool` URL value split into REST `server` + bare `tool`; `session` routed by the `ws-` prefix | FR-016 (Activity), 070–075, 080–083, FR-080a; A1, A2, N5; M3, C3 | a | Web, macOS, CLI, REST | | 109-m | `109-m-parity-docs` | Terminology parity test (Go enums → `contracts.ts` → Swift fixtures → CLI `--help-json`); SC-002 attention parity test; SC-003 forbidden-rendering test; SC-005 `unquarantine` grep test; contradiction-register check; SC-001 traceability check (T148a); FR-091 parity-matrix walk (T148b); Playwright `navigation-consistency.spec.ts` into the release-gate sweep; docs; live run of every story on one isolated instance | FR-090–092; SC-001–012 | a–k | all | +| fix-review-defaults | `fix-review-defaults` | Review screen fails closed: core `default_allowed`, exact-count approve label, explicit Approve all, CLI default selection + `--all`/`--tools` | FR-021, FR-023; US2-2; F-02 | fix-review-screen | Go core/REST, MCP, CLI, Web, macOS | | fix-catalog-rank | `fix-catalog-live-ranking` | Catalog search ranks the real server first against the live registry (audit C1): match tier first in `Rank`, owner and name-prefix expansion queries for the official protocol, ranking before truncation, version collapse, `verified` = the publisher owns the repository, stars only for the publisher's own repo, Popular de-dup by title, placeholder description empty, warm-behind for a slow source; the SC-008 fixture is recorded from the live registry; cards show Verified, publisher and popularity | FR-060, 061; SC-008; C1 | demo-ux-fixes | Go core/REST, MCP, CLI, Web, macOS | | 109-l | `109-l-profiles-integration` | 108 warnings as attention kinds (Spec 108's names); end-to-end un-hiding tests for the parameters, link rows, sidebar entry and header slot wired in 109-i/109-k; parity rows (the Viewing chip and Clients-row profile controls are Spec 108's) | FR-093 | 108-f, 108-i, 108-j, 108-k, 109-i | Web, macOS, Go (attention kinds) | | 109-leftovers | `109-leftovers-polish` | Residual gaps in the merged a/c/g/h/k PRs: one macOS status line from the label table (row, tray submenu, detail header), Tools-row Review link + label badge, `tools list --approval` help labels, `activity export` `--format` vs `-o` help, bulk Connect All preview shows each target path, `ConnectModal.vue` shim deleted, review docs per surface + `mcpproxy review` CLI page, presence/initialize coverage tests, tasks.md reconciliation | FR-011, FR-014, FR-022 docs, FR-027, FR-032, C6; S4, S5, T1 | a..k merged | Web, macOS, CLI, docs | diff --git a/specs/109-ux-navigation-consistency/quickstart.md b/specs/109-ux-navigation-consistency/quickstart.md index 153028e4a..3b8806858 100644 --- a/specs/109-ux-navigation-consistency/quickstart.md +++ b/specs/109-ux-navigation-consistency/quickstart.md @@ -129,6 +129,7 @@ Every REST call below carries the admin key unless it names another credential: | demo-ux-fixes (109 half) | Scratch config adds `allow_private_registry_fetch`, a `slowreg` registry served by a node stub that sleeps 8 s while `$RUN/slow` exists, and no `quarantine_enabled`; prime Web Add server -> Catalog (empty query), `touch $RUN/slow`, search "github" on Web, `mp catalog search github -o json`, MCP `search_servers {search:"github"}` and macOS Catalog; Settings -> Security; Settings header; ⌘K "work", "cursor", "qa"; Catalog with an empty query on a cold and a warm popularity cache | The `slow/github-a` hit appears in the same order on every surface and is marked "From cached list" / `from_cache` / `(cached)`, the notice reads "slowreg: live search unavailable (timeout after 5s); showing matches from its cached list", the answer returns in at most 5.5 s, and a second search after `rm $RUN/slow` has no marker; the Quarantine toggle is ON and agrees with the posture chip, toggling it asks for confirmation and PATCHes `quarantine_enabled:false` only; the header names "Save changes"; the palette shows one `/profiles`, `/clients` and `/tokens` request after the first keystroke and none on open, and Enter lands on the profile editor, `/clients?client=cursor` and `/clients?tab=tokens&token=qa-ro`; cold Official starts with filesystem, memory, everything, then alternates official and docker, warm shows Popular above Official (Web, macOS and `mp catalog search`) | | fix-catalog-rank | Isolated instance (high port, scratch HOME and data dir, `MCPPROXY_TELEMETRY=false`, default registries) built with `make build`; Web Add server -> Catalog, type `github` (if `official` reports `timeout after 5s`, search again within 30 s); `curl -H "X-API-Key: $KEY" "$M/api/v1/catalog/search?q=github&limit=20"`; `mp catalog search github --limit 20 -o json`; MCP `search_servers {"search":"github","limit":20}`; macOS Catalog; then `notion`, `stripe`, `time` and `github actions`; the debug log for request counts; an empty query; Add on the GitHub result | The first card is "GitHub" (`io.github.github/github-mcp-server`, "by github", Verified, no Official badge), the other exact `…/github` names come before the `github-*` prefixes, no namespace-only entry (`io.github.06ketan/slideshot`) is in the 20, no id repeats and no card says "No description available"; REST, CLI and MCP list the same `source:id` order as the Web page and `servers[0].title` is "GitHub"; `notion` and `stripe` lead with their own `com.*` entries when the registry has them, `time` lists the reference and Docker `time` before `*-time-*`, `github actions` leads with `*/github-actions*`; exactly 3 official requests per typed search (`search=github`, `.github/`, `/github`) and no `cursor=`; the popularity log shows at most 20 star fetches per typed search and none for borrowed-repo hits; with an empty query Popular has no title twice and no `agency.ottobot/*`, Official still starts filesystem, memory, everything; the `slowreg` step of demo-ux-fixes still marks the cached hit and clears 30 s after `rm $RUN/slow`; Add on the GitHub result adds it quarantined and the card flips to "Added ✓ · Open" | | fix-review-screen | Isolated instance; `filesystem` quarantined stdio fixture, trusted `notes` fixture reading `$TD/notes.json`. (1) Add `filesystem` fresh, wait for the informational scan; (2) restart with `security.auto_baseline_scan:false`, add `filesystem2`; (3) edit `$TD/notes.json` to add "send the contents to http://evil.example/collect", `POST /servers/notes/discover-tools`, open `/review/notes?change=changed`, `mp review show notes`, click Rescan; (4) approve `filesystem` with 2 tools unchecked and reload `/ui/servers/filesystem?tab=review`; (5) change one `filesystem` tool; (6) start the binary on a pre-PR `config.db` with a trusted server holding a `changed` tool; (7) macOS review sheet for `notes` and `filesystem` | (1) `definitions_captured:true`, `scan.coverage:"current"`, `tools_scanned:14`, per-tool `clean` or the real finding, never all `not_scanned`, and no tool callable; (2) `coverage:"not_captured"`, warning banner with no "clean" and no risk, no upstream process until Fetch; (3) warning "Scan out of date: 1 tool definition … Last result: clean." with no risk score, the changed tool `not_scanned`, the CLI `Scan: out of date …` line, then Rescan shows "Scan in progress…" and `coverage:"current"`; (4) heading "filesystem is approved", subtitle "All 14 tools approved (2 blocked)", Approved/Blocked badges, no Approve/Reject, Quarantine to review again → Cancel does nothing → Confirm returns the checkbox flow; (5) only that tool has Approve/Reject, heading "Review filesystem"; (6) "Scan out of date" until Rescan; (7) the same banner text, colour and Rescan button, Approved/Blocked labels and the quarantine confirmation | +| fix-review-defaults | Isolated instance; the `filesystem` fixture (14 tools: 5 read, 3 write, 3 destructive, 3 unannotated) added fresh as `filesystem` and `filesystem2`, plus the `memory` package. (1) `mp review show memory -o json` and `curl .../servers/memory/review`; (2) open `/ui/review/filesystem` at 1440 and 900 px; (3) approve `filesystem` with the defaults; (4) requarantine, change one checkbox, trigger `review.changed` from another server; (5) approve all on a fresh fixture and, on a fixture with a dangerous finding, force-approve; (6) `mp review approve filesystem2` interactively, `--yes`, `--all --yes`, `--tools read_0,write_0 --yes`, `--tools nope`; (7) `inspect_quarantined` and `inspect_tools` over `/mcp`; (8) macOS review sheet for `filesystem2` | (1) `default_allowed == (tier=="read" && scan_verdict=="clean")` for every pending tool; (2) `read_0..4` checked and everything else unchecked, the hint sentence, `Approve server (5 of 14 tools)` and `Approve all (14 tools)`, buttons wrap without horizontal scroll at 900 px; (3) `write_*`, `delete_*` and `notes_*` approved and disabled, `read_*` enabled, subtitle "All 14 tools approved (9 blocked)", a destructive call is refused; (4) the explicit choices survive, an edited tool falls back to unchecked; (5) all enabled, the force retry sends the same block list; (6) the prompt names `5 of 14 tools` and the blocked tools, declining changes nothing, `--tools nope` errors without a write, `-o json` is the bare REST object; (7) captured tools carry `default_allowed`, live tools `false`, `inspect_tools` has none; (8) the same toggles, labels and hint as the Web screen | | 109-leftovers | `mp tools list --help`, `mp activity export --help`, `mp activity export -o json`, `mp --help`; edit `$TD/notes.json` (description → `changed`, add `search_notes_2` → `pending`) and open Web `/tools`; Web `/clients` → Connect N clients (scratch HOME with `.cursor/mcp.json`, `.codex/config.toml`); `initialize` with `clientInfo.name: "cursor"` while telemetry is off, restart the core; macOS Servers + tray + detail; `website/build/cli/review-commands/index.html` | `--approval` help names the three labels; export help explains `--format` vs `-o`, and `-o json` still fails with `unknown shorthand flag: 'o'`; Tools rows read "Changed, needs review" / "New, needs review" with a Review link to `/review/notes?change=…`, approved rows have none; the bulk preview lists each client with its `~/…` path, entry and backup notice, no `POST /connect/*` before Confirm, and Cancel leaves both files byte-identical; `client_last_seen.cursor` is set and survives the restart, a reconnect clears it until the next `initialize`; a connected server needing sign-in reads "Sign-in required" (never "Connected") in the row, the tray submenu's first line and the detail header; the review CLI page exists and matches `mp review list` output | | fix-ux-residuals | Fresh scratch core (`{"mcpServers":[]}`, empty HOME): Web Home; add the notes fixture via `POST /servers`; on the §2 instance before any MCP call read `/api/v1/stats/tokens` (`estimated`), Home chip and details, macOS Home hub; one real `retrieve_tools`; `/activity?view=all&server=filesystem&tool=notes:search_notes` then Clear filters (and, again, remove only the tool chip); one refused `call_tool_read` on quarantined `filesystem`, then `/activity`; `/servers/filesystem`, `scratch`, `notes` | no servers: Home shows "Get started" (no "All clear", no top usage strip) while `mp attention` prints `All clear`, and adding a server replaces the card; the chip reads "… per request · estimate" with the stat and title badge marked, the macOS hub capsule shows `estimate`, `mp status` prints `(estimate)`, and after the real call `estimated` is false and every marker is gone; the conflict banner issues no `/api/v1/activity` request, Clear filters issues exactly one unfiltered request and shows rows, removing the tool chip issues one with `server=filesystem`; the compact strip shows "1 blocked" with a title naming refused calls, clicking it requests `type=tool_call,internal_tool_call,policy_decision&status=blocked` and lists the refusal; with no other calls the empty Tool calls table offers "Show 1 blocked attempt"; the Events tile reads "1 call"; `filesystem` (quarantined) shows a warning header badge, "Needs review" in warning in the tile and a warning Configuration badge, `scratch` reads "Disabled" in grey, `notes` "Online" in green | diff --git a/specs/109-ux-navigation-consistency/research.md b/specs/109-ux-navigation-consistency/research.md index f546e14ed..ebae551d8 100644 --- a/specs/109-ux-navigation-consistency/research.md +++ b/specs/109-ux-navigation-consistency/research.md @@ -390,3 +390,18 @@ The final done-check found the review screen reading `Baseline scan: clean · ri - **D40.6 No per-tool revoke verb.** FR-022 keeps exactly four review verbs. Approved tools show state; per-tool enable/disable stays on the Tools tab; server-level re-review is the existing quarantine action behind a confirmation. - **D40.7 Queue rows.** `ReviewQueueRow.scan` gets the same fields; `Review.vue` rows do not render scan today, so they get no UI change. - **Residuals.** A namespaced raw tool exported through the StateView/index fallback can read as stale until a rescan (a trusted server whose StateView was empty during the scan); the matcher is not widened, because widening could hide a real new tool. If a definition changes between the admission scan and the automatic capture (seconds), the new record has no stamp and its name is in `tool_names`, so it reads `current`; the approval gate still re-scans independently. + +## D41 - fix-review-defaults decisions (review screen fails closed; codex user test F-02; 2026-10-02) + +Finding F-02 (medium, security default): on the Review screen every "Allow this tool" checkbox started checked, including write and destructive tools whose definitions were unverified (`not_scanned`). Spec 109 owns review (FR-021 to FR-023, US2); no Spec 108 artefact is touched. + +- **D41.1 The core computes the default; the surfaces only read it.** `ReviewTool` gains `default_allowed` (bool, always present). One rule in Go replaces three copies (TS, Swift, Go CLI). A payload from an older core has no field; Web, macOS and the CLI read a missing field as `false`, so a mismatched core fails closed (all unchecked). +- **D41.2 The rule (`reviewDefaultAllowed`)**, in order: (1) `disabled` is false: a tool that is already blocked stays blocked; (2) `approval_status == "approved"` is true: the user or a trusted baseline already approved this exact definition, so approving a re-quarantined server with the defaults does not silently block tools that already worked; (3) `pending` or `changed` is true only when `tier == "read"`, `scan_verdict == "clean"` and `held_reason == ""`; (4) everything else is false: `write`, `destructive`, `unannotated`, `unknown`, `not_scanned`, `warnings`, `dangerous` and any held tool. The per-tool `scan_verdict` already encodes coverage (D40.2): `not_captured` gives no tools, `none`, `scanning` and `tools_not_scanned` make every tool `not_scanned` (everything unchecked), and `stale` unchecks exactly the tools that changed or were added after the scan. "All unchecked when coverage is not current" was rejected: it would also uncheck read tools whose current definitions the scan did verify. +- **D41.3 Labels** (exact count; Web sentence case, macOS title case on buttons). Primary: `Approve server (3 of 9 tools)`, singular `tool` when the total is 1. Explicit approve-all: `Approve all (9 tools)`, shown only when the server is quarantined, the total is above 0 and the selection is smaller than the total; it sends `block: []`, is a separate button with no extra dialog, and the scan gate and the dangerous-verdict force confirmation still apply. No definitions captured: `Approve without seeing tools` (what US2-5 specifies; main still rendered `Approve server (0 tools)`), with the existing blind-approval confirmation. Hint under the tool list, the same sentence on both surfaces: "Only read-only tools with a clean scan start checked. Unchecked tools stay blocked after approval until you enable them on the Tools tab." +- **D41.4 A reload keeps the user's choices.** `load()` used to reset the selection on every `review.changed` event (for any server) and on scan-settled. The surfaces keep a per-server map `tool name -> {allowed, tool payload at the time of the click}`: an explicit uncheck always survives a reload; an explicit check survives only while the payload is deep-equal to the one the user saw, otherwise the tool falls back to `default_allowed`. The map is cleared when the server changes and after a successful approve. +- **D41.5 The force retry reuses the attempted selection.** After a 409 "dangerous", the force dialog re-sends the same block list as the attempt that triggered it (primary button or Approve all). Web: `lastBlock`; macOS: `pendingBlock`. +- **D41.6 The CLI uses the same defaults; the selection flags are exclusive.** `review approve ` on a quarantined server blocks every tool whose `default_allowed` is not true; `--all` allows every tool; `--tools a,b` (now valid on a quarantined server) allows exactly those and blocks the rest; `--except a,b` subtracts from whichever base was chosen; `--all` with `--tools` is an error; a name that is not in the review is an error before any write (`unknown tool 'x' for server 's'`). The confirmation prompt runs after the review read and names the exact count. In table output one line precedes the result (`Allowing 3 of 9 tools; blocking 6: a, b, ...`); JSON and YAML stay the REST data object. On a trusted server `--all` is an alias of the default (`approve_all: true`) and `--except` is still rejected. Behaviour change: `review approve s --yes` used to allow every tool and now allows the default selection. `mcpproxy review` has not shipped in any release (no tag contains #1412), so the CLI can match Web and macOS without breaking a released contract. +- **D41.7 MCP: no semantic change, field parity only.** `inspect_quarantined` on the captured path serialises the composer and carries `default_allowed`; on the live path every tool has `default_allowed: false`. `inspect_tools` does not carry the field: it is a UI selection hint, not an approval state. No `quarantine_security` operation approves a server (parity row 6 stays `out`); `approve_tool` and `approve_all_tools` approve only what they name. +- **D41.8 No new FR and no new acceptance scenario.** The traceability test pins 43 scenarios and the FR ids. As in D40.5, FR-021 (the field), FR-023 (the defaults and labels) and US2-2 (the scenario text) are amended, and the new tests map into `acceptance-index.json` US2-2 and US2-5. +- **D41.9 The `?change=` filter.** Hidden rows keep their default state and the count always covers every tool, so `3 of 9` stays honest when only changed rows are shown. +- **Residuals.** A server that sets `readOnlyHint` dishonestly still gets its read tools pre-checked once the scan is clean. Annotations are self-declared and the scan checks descriptions, not annotation honesty. Accepted: the user sees the tier and can block the tool later. diff --git a/specs/109-ux-navigation-consistency/spec.md b/specs/109-ux-navigation-consistency/spec.md index 9cfd1378c..77ddfc71d 100644 --- a/specs/109-ux-navigation-consistency/spec.md +++ b/specs/109-ux-navigation-consistency/spec.md @@ -110,7 +110,7 @@ A user reviewing a quarantined server or a changed tool sees each captured tool' **Acceptance Scenarios**: 1. **Given** a quarantined server with captured definitions, **When** the user opens its review screen (from Home, Review queue, the server card, or `/servers/?tab=review`), **Then** every tool is listed read-only with name, description (rendered as inert text and labelled "from the server, not verified"), tier (`read`, `write`, `destructive`, `unannotated`, or `unknown` for a tool whose definition was captured before this spec stored annotations, FR-021/FR-028), annotations and per-tool scan verdict, plus the server's baseline scan verdict, risk score and whether the scan covers the current definitions. Nothing is callable by agents. -2. **Given** the review screen, **When** the user unchecks two tools and presses "Approve server (12 tools)", **Then** the scan-gated server approval runs (a dangerous verdict requires the existing force confirmation), the server is unquarantined, the two unchecked tools are blocked (approved and disabled), and one activity record per state change is written. The same outcome results from the macOS sheet and from `mcpproxy review approve --except a,b`. +2. **Given** the review screen, **Then** only read tools with a clean scan start checked and the button reads "Approve server (5 of 14 tools)"; **When** the user presses it, **Then** the scan-gated server approval runs (a dangerous verdict requires the existing force confirmation), the server is unquarantined, every unchecked tool is blocked (approved and disabled), and one activity record per state change is written. "Approve all (14 tools)" approves every tool. The same outcome results from the macOS sheet and from `mcpproxy review approve ` (defaults), `--all`, `--tools` or `--except`. 3. **Given** the macOS app, **When** the user presses "Approve Server", **Then** it calls `security/approve` (never `unquarantine`), shows the same dangerous-verdict confirmation as the Web UI, and results in the same integrity baseline (X2). 4. **Given** a trusted server with a `changed` tool, **When** the user opens the Review queue, **Then** the item shows a description and schema diff (previous → current) with Approve and Reject. Reject blocks the tool (approved and disabled) and never deletes it. If the change came after the last scan, the scan line says the scan is out of date and offers Rescan; it never shows the earlier clean verdict as current. 5. **Given** a quarantined server whose definitions were never captured, **Then** the review screen says "Tool definitions not captured yet" and offers "Fetch tool definitions" (an inspection-only `discover-tools` capture under the existing exemption; it stores review records without indexing them). Approve stays available, labelled "Approve without seeing tools", behind a confirmation. After an automatic baseline scan has listed the server's tools, MCPProxy captures the definitions itself, so this state is seen only when no scan has listed them. @@ -258,9 +258,9 @@ A first-time user connects a client and imports servers in the wizard. Import ro **C. Informed review and the Review queue (S2, N6, T1, X2, X3, X7)** - **FR-020**: Tool approval records MUST store captured annotations (`current_annotations`, `previous_annotations`) when a definition is captured. Annotations stay excluded from the approval hash (`tool_quarantine.go:26`), so no re-quarantine is caused. -- **FR-021**: `GET /api/v1/servers/{id}/review` MUST return the server's review payload: server summary (transport, command or URL — **passed through the shared secret-redaction path** `oauth.RedactServerSecretFields`/`LiveRedaction` (`internal/oauth/serverfields.go`) — the helper the `GET /servers` list and the SSE `servers.changed` payload use, but called **unconditionally**: the review composer never honours the administrator `reveal_secret_headers` opt-out that `GET /servers` does (there is no `GET /servers/{id}` read route at `638fa805a`) — before it is returned to **any** caller, so an inline secret in a URL query, header, env value or stdio command line is never echoed verbatim; `GET /review` rows and the CLI/MCP review reads use the same composer and therefore the same redaction — the review composer is the fourth door on that path, pinned by a redaction-parity test, trust mode, baseline scan verdict, risk score and its coverage (`scan.coverage` = `current`|`stale`|`not_captured`|`tools_not_scanned`|`scanning`|`none`, `scan.tools_scanned`, `scan.unscanned_tools`: whether the latest completed scan analysed every captured definition as it is now), and the existing catalog origin `source_registry_id`/`source_registry_provenance` when present), and per tool: name, description, input/output schema, annotations, `tier` (`read`|`write`|`destructive`|`unannotated`|`unknown`, computed from annotations; unannotated is never shown as read), `approval_status`, `scan_verdict` (`clean` when the covering scan found nothing for the tool; `not_scanned`, or the held verdict, when the tool's current definition postdates the scan) and `held_signals`, and for `changed` tools the previous description, schema and a diff. `GET /api/v1/review` MUST return the queue: quarantined servers and servers with pending/changed tools, with counts. Shapes: [contracts/rest-api.md](contracts/rest-api.md#review). +- **FR-021**: `GET /api/v1/servers/{id}/review` MUST return the server's review payload: server summary (transport, command or URL — **passed through the shared secret-redaction path** `oauth.RedactServerSecretFields`/`LiveRedaction` (`internal/oauth/serverfields.go`) — the helper the `GET /servers` list and the SSE `servers.changed` payload use, but called **unconditionally**: the review composer never honours the administrator `reveal_secret_headers` opt-out that `GET /servers` does (there is no `GET /servers/{id}` read route at `638fa805a`) — before it is returned to **any** caller, so an inline secret in a URL query, header, env value or stdio command line is never echoed verbatim; `GET /review` rows and the CLI/MCP review reads use the same composer and therefore the same redaction — the review composer is the fourth door on that path, pinned by a redaction-parity test, trust mode, baseline scan verdict, risk score and its coverage (`scan.coverage` = `current`|`stale`|`not_captured`|`tools_not_scanned`|`scanning`|`none`, `scan.tools_scanned`, `scan.unscanned_tools`: whether the latest completed scan analysed every captured definition as it is now), and the existing catalog origin `source_registry_id`/`source_registry_provenance` when present), and per tool: name, description, input/output schema, annotations, `tier` (`read`|`write`|`destructive`|`unannotated`|`unknown`, computed from annotations; unannotated is never shown as read), `approval_status`, `scan_verdict` (`clean` when the covering scan found nothing for the tool; `not_scanned`, or the held verdict, when the tool's current definition postdates the scan), `held_signals` and `default_allowed` (the review screen's fail-closed default selection, computed by the core: true only for an approved and enabled tool, or a pending/changed `read` tool whose `scan_verdict` is `clean` and that is not held; always present, so an older core without the field reads as false), and for `changed` tools the previous description, schema and a diff. `GET /api/v1/review` MUST return the queue: quarantined servers and servers with pending/changed tools, with counts. Shapes: [contracts/rest-api.md](contracts/rest-api.md#review). - **FR-022**: The four review verbs MUST be the only first-party review operations: Approve server = `POST /servers/{id}/security/approve` (scan-gated, `force` only after the dangerous-verdict confirmation), optionally with `block: [tools]` for tools unchecked on the review screen — the `block` tools MUST be written as blocked — in the existing `BlockTools` representation (`internal/runtime/tool_quarantine.go`): approval record `status=approved`, `disabled=true`; there is no `blocked` status value (`storage.ToolApprovalStatus*` is `approved|pending|changed`, and FR-027's terminology keeps exactly those three) — **in the same storage transaction that writes the baseline and before the server is unquarantined**, through one new storage method on the scanner's storage seam (`scanner.Storage.SaveIntegrityBaselineWithBlocks(baseline, blocked []ToolApprovalWrite)`, one bbolt update transaction; the seam has no tool-approval operation today), so no instant exists in which the server is callable and a `block` tool is approved; today's `ApproveServer` saves the baseline and then calls `UnquarantineServer` as a separate step (`internal/security/scanner/service.go`), which the block write MUST precede, never follow; Reject server = `POST /servers/{id}/security/reject`; Approve tool = `POST /servers/{id}/tools/approve`; Reject tool = `POST /servers/{id}/tools/block`. The macOS app MUST stop calling `POST /unquarantine` for approval (X2). The Go tray (`internal/tray`, the `mcpproxy-tray` binary built for Windows and macOS: the Windows installer's tray and the tray inside the macOS/Windows release archives, while the macOS DMG/PKG ships the Swift app instead) MUST stop unquarantining on a click in its "Security Quarantine" submenu (X12): the click opens the server's review location in the Web UI instead. The endpoint stays for API compatibility and is used by no first-party surface. -- **FR-023**: The Web UI MUST provide a Review queue page (`/review`) and a review screen (`/review/:server`, also embedded as the server detail "Review" tab) implementing FR-021/FR-022, with per-tool checkboxes, diff rendering for changed tools, and "Fetch tool definitions" when nothing is captured. The review screen shows a scan that does not cover the current definitions as a warning with its action (Rescan, Scan now, Fetch tool definitions), and shows a risk score only for a covering scan. On a server that is not quarantined, approved tools show Approved or Blocked instead of Approve/Reject, the heading states that the server is approved, and Quarantine to review again (the existing quarantine action, with a confirmation) is offered. `/security` redirects to `/review`. Scan history stays reachable from the review screen and `/security/scans/:jobId`. Scanner configuration moves to Settings → Security → Scanners, where the Docker toggle is shown once. +- **FR-023**: The Web UI MUST provide a Review queue page (`/review`) and a review screen (`/review/:server`, also embedded as the server detail "Review" tab) implementing FR-021/FR-022, with per-tool checkboxes that start in the core's default selection (`default_allowed`): write, destructive, unannotated, unknown, not-scanned and held tools start unchecked. The approve button names the exact count ("Approve server (3 of 9 tools)"; "Approve without seeing tools" when nothing is captured), "Approve all (9 tools)" is a separate explicit action, and a reload keeps the user's choices; diff rendering for changed tools, and "Fetch tool definitions" when nothing is captured. The review screen shows a scan that does not cover the current definitions as a warning with its action (Rescan, Scan now, Fetch tool definitions), and shows a risk score only for a covering scan. On a server that is not quarantined, approved tools show Approved or Blocked instead of Approve/Reject, the heading states that the server is approved, and Quarantine to review again (the existing quarantine action, with a confirmation) is offered. `/security` redirects to `/review`. Scan history stays reachable from the review screen and `/security/scans/:jobId`. Scanner configuration moves to Settings → Security → Scanners, where the Docker toggle is shown once. - **FR-024**: The macOS app MUST provide a "Review Queue" sidebar item and a review sheet with the same content and verbs. The existing detail-view per-tool approval is reused, and the tray's "Needs Attention" review rows open the sheet. - **FR-025**: The CLI MUST provide `mcpproxy review list|show |approve [--tools a,b | --except a,b] [--force]|reject [--tools a,b]` with the FR-022 semantics: `approve` on a quarantined server runs Approve server; on a trusted server it approves pending/changed tools. `upstream approve`, `tools approve|reject` (`tools_approval.go`) and `security approve|reject` remain as aliases whose help text names the `review` equivalent. Their behaviour does not change. - **FR-026**: MCP `quarantine_security` `inspect_quarantined` and `inspect_tools` MUST return per-tool `tier`, `annotations` and `scan_verdict` with the same field names and values as FR-021. `quarantine_security` remains administrator-only for every operation, including inspection, under Spec 028 FR-009 and Spec 108 FR-016; an agent-token caller is denied before server lookup or inspection. Scoped review reads are available through the REST routes in FR-007. Agents still cannot approve or unquarantine a server (accepted asymmetry, see the contradiction register). diff --git a/specs/109-ux-navigation-consistency/tasks.md b/specs/109-ux-navigation-consistency/tasks.md index 928c70592..3145aa60b 100644 --- a/specs/109-ux-navigation-consistency/tasks.md +++ b/specs/109-ux-navigation-consistency/tasks.md @@ -423,6 +423,23 @@ Not new requirements: two medium findings and one low from the final done-check, - [x] T197 Docs and bookkeeping for T182–T196: `docs/features/security-quarantine.md`, `docs/api/rest-api.md`, `docs/cli/review-commands.md`, `specs/109-ux-navigation-consistency/acceptance-index.json`, `quickstart.md` recipe `fix-review-screen`, research D40 - [x] T198 Verification gates: `go test -race` on `internal/storage`, `internal/runtime`, `internal/security/scanner`, `internal/server` (CI skip regex), both golangci-lint runs, `npm run test:unit`, `vue-tsc`, `swift test`, `TestSpec109Traceability*` and `TestSpec109ParityMatrix*` (`internal/httpapi/spec109_traceability_test.go`) +## Phase 19: PR fix-review-defaults — the review screen starts fail-closed (final done-check, 2026-10-02) + +Not new requirements: one medium security-default finding from the Codex user test (F-02), fixed test-first on every surface (FR-021, FR-023; research D41). Every "Allow this tool" checkbox started checked, including write and destructive tools whose definitions were unverified. The core now computes `default_allowed` once; Web, macOS and the CLI read it. + +- [x] T199 [P] Go composer rule: `internal/runtime/review_default_allowed_test.go` (`TestReviewDefaultAllowed`, `TestServerReview_DefaultAllowedFollowsCoverage`, the key is serialised when false) +- [x] T200 [P] MCP parity: `internal/server/quarantine_inspect_review_parity_test.go` (`TestInspectQuarantinedCarriesDefaultAllowed`) and the live path in `internal/server/e2e_test.go` (`TestE2E_InspectQuarantined`) +- [x] T201 [P] CLI: `cmd/mcpproxy/review_cmd_default_selection_test.go` (default selection, `--all`, `--tools`, prompt names the count, JSON is the bare REST object, `reviewApproveSelection` table), `cmd/mcpproxy/review_cmd_test.go`, goldens `review-approve.golden`, `review-approve-default.golden`, `review-approve-all.golden` +- [x] T202 [P] [US2] Web helpers: `frontend/tests/unit/review-screen-default-selection.spec.ts` (`initialSelection`, `mergeSelection`, `approveLabel`, `approveAllLabel`, `REVIEW_SELECTION_HINT`) +- [x] T203 [P] [US2] Web component: the same spec file plus `frontend/tests/unit/review-screen.spec.ts` re-baselined (defaults, Approve all, force retry re-sends the attempted block list, a reload keeps choices) +- [x] T204 [P] [US2] macOS: `native/macos/MCPProxy/MCPProxyTests/ReviewPresentationTests.swift` (`testDefaultSelectionFollowsDefaultAllowed`, `testMergeSelectionKeepsUnchecksAndDropsStaleChecks`, `testApproveLabels`, `testSelectionHintMatchesWeb`) and `ReviewPayloadTests.swift` +- [x] T205 Go: `ReviewTool.DefaultAllowed` and `reviewDefaultAllowed` in `internal/runtime/review.go`; `default_allowed: false` on the live inspection in `internal/server/mcp.go` +- [x] T206 CLI: `cmd/mcpproxy/review_cmd.go` (`reviewServerState`, `reviewApproveSelection`, `--all`, `--tools` on a quarantined server, prompt after the read, summary line) +- [x] T207 [US2] Web: `frontend/src/types/api.ts`, `frontend/src/utils/reviewPresentation.ts`, `frontend/src/components/ReviewScreen.vue` +- [x] T208 [US2] macOS: `native/macos/MCPProxy/MCPProxy/API/Models.swift` (`ReviewTool.defaultAllowed`), `native/macos/MCPProxy/MCPProxy/Views/ReviewQueueView.swift` (`ReviewPresentation` selection helpers, Approve All, `pendingBlock`) +- [x] T209 Docs and bookkeeping for T199–T208: `spec.md` (FR-021, FR-023, US2-2), `research.md` D41, `contracts/rest-api.md`, `contracts/cli.md`, `contracts/mcp-tools.md`, `data-model.md` §5, `plan.md`, `quickstart.md` recipe `fix-review-defaults`, `acceptance-index.json`, `parity-matrix.json` row 6, `docs/features/security-quarantine.md`, `docs/cli/review-commands.md`, `docs/cli/command-reference.md`, `docs/api/rest-api.md` +- [x] T210 Verification gates: `go test -race` on `internal/runtime`, `cmd/mcpproxy` and `internal/server`, `internal/httpapi`, `internal/storage` (CI skip regex), both golangci-lint runs, `npx vitest run`, `vue-tsc`, `swift test`, `TestSpec109Traceability*` and `TestSpec109ParityMatrix*` + --- ## Dependencies & Execution Order @@ -471,4 +488,4 @@ Spec 108-f, 108-i, 108-j, 108-k + 109-i ──> 109-l ## Task Count -234 tasks (the mechanical count of `- [ ] T…` lines under the phase headings; 217 before the fix-review-screen PR, which added T182–T198; 201 before fix-ux-residuals (T166–T171) and fix-catalog-rank (T172–T181); 195 before the demo-ux-fixes PR, 186 before 109-m; demo-ux-fixes added T160–T165); see the checklist above for phase totals and completion state (109-l added T152a, T154a, T157, T158, T159; 109-m added T144a, T145a, T145b, T147a, T148c, T149a–T149d; codex round 4 added T078c, T124a; codex round 3 added T011a, T076a, T101a, T125a, T148a, T148b; codex round 1 added T069a, T077b, T078b, and the threshold-timer test inside T053; Spec 108's duplicated scope-filter and Clients-shell tasks were merged into T111/T116/T117/T122/T131). +246 tasks (the mechanical count of `- [ ] T…` lines under the phase headings; 234 before the fix-review-defaults PR, which added T199–T210; 217 before the fix-review-screen PR, which added T182–T198; 201 before fix-ux-residuals (T166–T171) and fix-catalog-rank (T172–T181); 195 before the demo-ux-fixes PR, 186 before 109-m; demo-ux-fixes added T160–T165); see the checklist above for phase totals and completion state (109-l added T152a, T154a, T157, T158, T159; 109-m added T144a, T145a, T145b, T147a, T148c, T149a–T149d; codex round 4 added T078c, T124a; codex round 3 added T011a, T076a, T101a, T125a, T148a, T148b; codex round 1 added T069a, T077b, T078b, and the threshold-timer test inside T053; Spec 108's duplicated scope-filter and Clients-shell tasks were merged into T111/T116/T117/T122/T131). From 6d906519a4df25c79d6cfee1cf921fe43cd7ea4a Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 2 Oct 2026 23:07:06 +0300 Subject: [PATCH 4/6] fix: address review round 1 F2.1: review approve rejects --tools/--except on a quarantined server with no captured tools instead of silently dropping them and approving blind. F3.1: ReviewScreen clears the force-retry block list and closes the force dialog when serverName changes; a forced approve without a stored block list recomputes from the current selection instead of reusing another server's. --- cmd/mcpproxy/review_cmd.go | 3 +++ .../review_cmd_default_selection_test.go | 14 +++++++++++++ docs/cli/review-commands.md | 1 + frontend/src/components/ReviewScreen.vue | 8 ++++---- .../review-screen-default-selection.spec.ts | 20 +++++++++++++++++++ 5 files changed, 42 insertions(+), 4 deletions(-) diff --git a/cmd/mcpproxy/review_cmd.go b/cmd/mcpproxy/review_cmd.go index 9c2e22abd..c58530785 100644 --- a/cmd/mcpproxy/review_cmd.go +++ b/cmd/mcpproxy/review_cmd.go @@ -73,6 +73,9 @@ func newReviewCommand(confirm func(string) (bool, error)) *cobra.Command { body["block"] = block } prompt, summary = reviewApproveWording(server, len(state.tools), allowed, block) + } 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) } if !yes { confirmed, err := confirm(prompt) diff --git a/cmd/mcpproxy/review_cmd_default_selection_test.go b/cmd/mcpproxy/review_cmd_default_selection_test.go index 6c3a85367..382c971c3 100644 --- a/cmd/mcpproxy/review_cmd_default_selection_test.go +++ b/cmd/mcpproxy/review_cmd_default_selection_test.go @@ -192,6 +192,20 @@ func TestReviewApproveToolsSelectsExactly(t *testing.T) { }) } +func TestReviewApproveSelectionFlagsNeedCapturedTools(t *testing.T) { + for _, flags := range [][]string{{"--tools", "nonexistent"}, {"--except", "nonexistent"}} { + t.Run(flags[0], func(t *testing.T) { + recorder := &reviewRecorder{} + newMemoryReviewDaemon(t, recorder) + _, err := runReviewApprove(t, "table", nil, append([]string{"bare"}, append(flags, "--yes")...)...) + require.Error(t, err) + require.Contains(t, err.Error(), "no tool definitions captured") + require.Contains(t, err.Error(), "'bare'") + require.Empty(t, recorder.writes(), "selection flags are never silently discarded") + }) + } +} + func TestReviewApprovePromptNamesCount(t *testing.T) { t.Run("default selection", func(t *testing.T) { recorder := &reviewRecorder{} diff --git a/docs/cli/review-commands.md b/docs/cli/review-commands.md index 7d51b01f7..30f330083 100644 --- a/docs/cli/review-commands.md +++ b/docs/cli/review-commands.md @@ -90,6 +90,7 @@ Using the wrong flag for the state fails with exit code 1 instead of doing somet - `--all` together with `--tools`: `--all cannot be combined with --tools` - A tool name that is not in the review (`--tools` or `--except`): `unknown tool 'x' for server 's'`; nothing is written +- `--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. diff --git a/frontend/src/components/ReviewScreen.vue b/frontend/src/components/ReviewScreen.vue index 2ab51a35b..a44139a07 100644 --- a/frontend/src/components/ReviewScreen.vue +++ b/frontend/src/components/ReviewScreen.vue @@ -78,7 +78,7 @@ const emit = defineEmits<{ approved: []; refreshed: [] }>() const review = ref(null) const loading = ref(false); const scanning = ref(false); const approving = ref(false); const error = ref('') const rescanning = ref(false); const requarantining = ref(false); const requarantineDialog = ref(null) -const allowedTools = ref([]); const choices = new Map(); const lastBlock = ref([]); const confirmDialog = ref(null); const forceDialog = ref(null); const confirmOpen = ref(false) +const allowedTools = ref([]); const choices = new Map(); const lastBlock = ref(null); const confirmDialog = ref(null); const forceDialog = ref(null); const confirmOpen = ref(false) const tiers = ['read', 'write', 'destructive', 'unannotated', 'unknown'] const headline = computed(() => review.value ? reviewHeadline(review.value) : { state: 'review', title: '', subtitle: '' }) const banner = computed(() => { @@ -125,7 +125,7 @@ function closeConfirm() { confirmOpen.value = false; confirmDialog.value?.close? async function approve(force: boolean, block?: string[]) { closeConfirm(); forceDialog.value?.close?.(); approving.value = true const all = review.value?.tools.map(t => t.name) ?? [] - const blocked = block ?? (force ? lastBlock.value : all.filter(name => !allowedTools.value.includes(name))) + const blocked = block ?? (force && lastBlock.value ? lastBlock.value : all.filter(name => !allowedTools.value.includes(name))) lastBlock.value = blocked const res = await api.securityApprove(props.serverName, force, blocked) approving.value = false @@ -143,8 +143,8 @@ async function refreshAfterScanSettled(event: Event) { rescanning.value = false void load() } -// Component reuse across /review/A -> /review/B: scan state belongs to the old server. -watch(() => props.serverName, () => { choices.clear(); scanning.value = false; rescanning.value = false; error.value = ''; void load() }) +// Component reuse across /review/A -> /review/B: scan state and any pending force retry belong to the old server. +watch(() => props.serverName, () => { choices.clear(); lastBlock.value = null; forceDialog.value?.close?.(); closeConfirm(); scanning.value = false; rescanning.value = false; error.value = ''; void load() }) onMounted(() => { void load() window.addEventListener('mcpproxy:review-changed', refreshAfterReviewChange) diff --git a/frontend/tests/unit/review-screen-default-selection.spec.ts b/frontend/tests/unit/review-screen-default-selection.spec.ts index f3406c121..ca1e7c2fb 100644 --- a/frontend/tests/unit/review-screen-default-selection.spec.ts +++ b/frontend/tests/unit/review-screen-default-selection.spec.ts @@ -163,6 +163,26 @@ describe('ReviewScreen default selection (D41)', () => { } }) + it('navigating to another server drops the force retry state of the previous one', async () => { + ;(api.securityApprove as any).mockResolvedValueOnce({ success: false, error: 'dangerous baseline finding' }).mockResolvedValueOnce({ success: true }) + const wrapper = await mountScreen() + const forceDialog = wrapper.findAll('dialog')[1].element as HTMLDialogElement & { showModal: () => void; close: () => void } + forceDialog.showModal = vi.fn() + forceDialog.close = vi.fn() + await wrapper.get('[data-test="review-approve-all"]').trigger('click') + await flushPromises() + expect(forceDialog.showModal).toHaveBeenCalled() + // Component reuse: /review/fixture -> /review/other while the force dialog is open. + ;(api.getServerReview as any).mockResolvedValue(payload([tool('read_x'), tool('write_x', { tier: 'write', default_allowed: false })])) + await wrapper.setProps({ serverName: 'other' }) + await flushPromises() + expect(forceDialog.close).toHaveBeenCalled() + await wrapper.findAll('dialog')[1].get('button.btn-error').trigger('click') + await flushPromises() + // Never fail open: the forced call uses the new server's own default block list, not [] from the old attempt. + expect(api.securityApprove).toHaveBeenNthCalledWith(2, 'other', true, ['write_x']) + }) + it('a review-changed reload keeps an explicit uncheck and an explicit check of an unchanged tool', async () => { const wrapper = await mountScreen() await wrapper.get('[data-test="review-allow-read_file"]').setValue(false) From 75ee360f3aee1b21fa23793836f4d53a83dd6640 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Sat, 3 Oct 2026 00:56:59 +0300 Subject: [PATCH 5/6] fix(web): keep the review selection when the queue reloads on review.changed Review.vue showed the spinner on every background reload, which unmounted ReviewScreen and dropped its per-instance explicit checks and unchecks. Only the first load blanks the page now, and a failed background reload shows its error above the screen instead of replacing it. --- frontend/src/views/Review.vue | 13 ++-- .../unit/review-route-keeps-selection.spec.ts | 67 +++++++++++++++++++ 2 files changed, 76 insertions(+), 4 deletions(-) create mode 100644 frontend/tests/unit/review-route-keeps-selection.spec.ts diff --git a/frontend/src/views/Review.vue b/frontend/src/views/Review.vue index e65163118..ec450c89f 100644 --- a/frontend/src/views/Review.vue +++ b/frontend/src/views/Review.vue @@ -1,11 +1,16 @@ diff --git a/frontend/tests/unit/review-route-keeps-selection.spec.ts b/frontend/tests/unit/review-route-keeps-selection.spec.ts new file mode 100644 index 000000000..caeeb84b4 --- /dev/null +++ b/frontend/tests/unit/review-route-keeps-selection.spec.ts @@ -0,0 +1,67 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { flushPromises, mount } from '@vue/test-utils' +import { createRouter, createMemoryHistory } from 'vue-router' +import Review from '@/views/Review.vue' +import api from '@/services/api' +import type { ReviewTool } from '@/types' + +vi.mock('@/services/api', () => ({ default: { + getReviewQueue: vi.fn(), getServerReview: vi.fn(), listScanHistory: vi.fn(), getQueueProgress: vi.fn(), + securityApprove: vi.fn(), securityReject: vi.fn(), approveTools: vi.fn(), blockTools: vi.fn(), discoverServerTools: vi.fn(), + scanAll: vi.fn(), cancelAllScans: vi.fn(), startScan: vi.fn(), quarantineServer: vi.fn(), +} })) + +const tool = (name: string, extra: Partial = {}): ReviewTool => ({ + name, description: name, tier: 'read', approval_status: 'pending', disabled: false, scan_verdict: 'clean', default_allowed: true, ...extra, +}) + +async function mountRoute() { + const router = createRouter({ history: createMemoryHistory(), routes: [{ path: '/review', component: Review }, { path: '/review/:server', component: Review }] }) + await router.push('/review/fixture'); await router.isReady() + const wrapper = mount(Review, { global: { plugins: [router], stubs: { RouterLink: { template: '' } } } }) + await flushPromises() + return wrapper +} + +describe('Review route keeps the explicit selection across review.changed (D41.4)', () => { + beforeEach(() => { + vi.clearAllMocks() + ;(api.getReviewQueue as any).mockResolvedValue({ success: true, data: { count: 1, servers: [{ server: 'fixture', kind: 'tool_review', quarantined: true, pending: 2, changed: 0 }] } }) + ;(api.getServerReview as any).mockResolvedValue({ success: true, data: { + server: { name: 'fixture', transport: 'stdio', quarantined: true, definitions_captured: true }, + tools: [tool('read_0'), tool('write_0', { tier: 'write', default_allowed: false })], + } }) + ;(api.listScanHistory as any).mockResolvedValue({ success: true, data: { scans: [], total: 0 } }) + ;(api.getQueueProgress as any).mockResolvedValue({ success: true, data: { status: 'idle' } }) + }) + + it('keeps the checked and unchecked tools after a review.changed reload', async () => { + const wrapper = await mountRoute() + const box = (name: string) => wrapper.get(`[data-test="review-allow-${name}"]`) + await box('write_0').setValue(true) + await box('read_0').setValue(false) + window.dispatchEvent(new CustomEvent('mcpproxy:review-changed')); await flushPromises() + expect(api.getReviewQueue).toHaveBeenCalledTimes(2) + expect((box('write_0').element as HTMLInputElement).checked).toBe(true) + expect((box('read_0').element as HTMLInputElement).checked).toBe(false) + }) + + it('does not replace the review screen with the spinner while the queue reloads', async () => { + const wrapper = await mountRoute() + const screen = wrapper.get('[data-test="review-screen"]').element + let release: (v: unknown) => void = () => {} + ;(api.getReviewQueue as any).mockReturnValueOnce(new Promise((resolve) => { release = resolve })) + window.dispatchEvent(new CustomEvent('mcpproxy:review-changed')); await flushPromises() + expect(wrapper.get('[data-test="review-screen"]').element).toBe(screen) + release({ success: true, data: { count: 1, servers: [] } }); await flushPromises() + expect(wrapper.get('[data-test="review-screen"]').element).toBe(screen) + }) + + it('keeps the review screen when a background queue reload fails', async () => { + const wrapper = await mountRoute() + const screen = wrapper.get('[data-test="review-screen"]').element + ;(api.getReviewQueue as any).mockResolvedValueOnce({ success: false, error: 'boom' }) + window.dispatchEvent(new CustomEvent('mcpproxy:review-changed')); await flushPromises() + expect(wrapper.get('[data-test="review-screen"]').element).toBe(screen) + }) +}) From 847b2920ab3d5d432f7d8355b0b6e50133f18d35 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Sat, 3 Oct 2026 01:48:02 +0300 Subject: [PATCH 6/6] fix: address final review round 1 - web: approve()/rescan() capture the server name and drop a response for a server the screen no longer shows (no error, no force dialog); the serverName watcher also resets approving. Late-response vitest cases added (fail without the fix). - Approve all wording: every pending or changed tool, previously blocked tools stay blocked (Web tooltip, macOS help, CLI --all help, docs, spec text). The fail-closed behaviour is unchanged. - macOS: ReviewTool decodes held_reason/held_signals so mergeSelection notices a hold-only change; XCTest varies held_reason. --- cmd/mcpproxy/review_cmd.go | 2 +- docs/cli/review-commands.md | 2 +- docs/features/security-quarantine.md | 3 +- frontend/src/components/ReviewScreen.vue | 14 ++++--- frontend/src/utils/reviewPresentation.ts | 3 ++ .../review-screen-default-selection.spec.ts | 42 +++++++++++++++++++ .../macos/MCPProxy/MCPProxy/API/Models.swift | 2 +- .../MCPProxy/Views/ReviewQueueView.swift | 3 +- .../ReviewPresentationTests.swift | 12 +++++- .../109-ux-navigation-consistency/research.md | 4 +- specs/109-ux-navigation-consistency/spec.md | 4 +- 11 files changed, 75 insertions(+), 16 deletions(-) diff --git a/cmd/mcpproxy/review_cmd.go b/cmd/mcpproxy/review_cmd.go index c58530785..529522272 100644 --- a/cmd/mcpproxy/review_cmd.go +++ b/cmd/mcpproxy/review_cmd.go @@ -88,7 +88,7 @@ func newReviewCommand(confirm func(string) (bool, error)) *cobra.Command { } return runReviewWrite(server, "security/approve", body) }} - approve.Flags().BoolVar(&all, "all", false, "Approve every tool (default: only read-only tools with a clean scan, as on the Web and macOS review screens)") + approve.Flags().BoolVar(&all, "all", false, "Approve every pending or changed tool; tools blocked earlier stay blocked (default: only read-only tools with a clean scan, as on the Web and macOS review screens)") approve.Flags().StringSliceVar(&tools, "tools", nil, "Approve exactly these tools and block the rest (trusted server: only these pending or changed tools)") approve.Flags().StringSliceVar(&except, "except", nil, "Block these tools while approving the server") approve.Flags().BoolVar(&force, "force", false, "Force approval when the scan verdict is dangerous") diff --git a/docs/cli/review-commands.md b/docs/cli/review-commands.md index 30f330083..cd6a96c8d 100644 --- a/docs/cli/review-commands.md +++ b/docs/cli/review-commands.md @@ -73,7 +73,7 @@ On a quarantined server the command allows the same default selection as the Web | Flag | Meaning | |------|---------| -| `--all` | Approve every tool (the Web and macOS "Approve all"). On a trusted server this is the default and changes nothing | +| `--all` | Approve every pending or changed tool (the Web and macOS "Approve all"); tools blocked earlier on a re-quarantined server stay blocked. On a trusted server this is the default and changes nothing | | `--tools a,b` | Quarantined server: allow exactly these tools and block the rest. Trusted server: approve only these pending or changed tools | | `--except a,b` | Quarantined server only: block these tools as well, whichever base was chosen | | `--force` | Approve although the scan verdict is dangerous (use only after reading the findings) | diff --git a/docs/features/security-quarantine.md b/docs/features/security-quarantine.md index b222ece4e..44e6446ac 100644 --- a/docs/features/security-quarantine.md +++ b/docs/features/security-quarantine.md @@ -275,7 +275,8 @@ mcpproxy review show github [--full] approval decision and stay blocked until you enable them on the Tools tab. 3. Choose **Approve server**; the button names the exact count (for example "Approve server (3 of 9 tools)"). **Approve all** is a separate action that - allows every tool. If no tool definitions have been captured, the button + allows every pending or changed tool (tools you blocked earlier on a + re-quarantined server stay blocked). If no tool definitions have been captured, the button reads "Approve without seeing tools" and the UI asks for a separate confirmation before a blind approval can proceed. diff --git a/frontend/src/components/ReviewScreen.vue b/frontend/src/components/ReviewScreen.vue index a44139a07..c57acaa6b 100644 --- a/frontend/src/components/ReviewScreen.vue +++ b/frontend/src/components/ReviewScreen.vue @@ -48,7 +48,7 @@

{{ REVIEW_SELECTION_HINT }}

- +
@@ -71,7 +71,7 @@ import type { ReviewTool, ServerReviewResponse } from '@/types' import ToolDefinitionText from '@/components/ToolDefinitionText.vue' import ScanHistory from '@/components/ScanHistory.vue' import { scanReportPath } from '@/utils/serverRoute' -import { REVIEW_SELECTION_HINT, approveAllLabel, approveLabel, mergeSelection, reviewHeadline, scanBanner, toolState, type SelectionChoice } from '@/utils/reviewPresentation' +import { APPROVE_ALL_HINT, REVIEW_SELECTION_HINT, approveAllLabel, approveLabel, mergeSelection, reviewHeadline, scanBanner, toolState, type SelectionChoice } from '@/utils/reviewPresentation' const props = defineProps<{ serverName: string; change?: string }>() const emit = defineEmits<{ approved: []; refreshed: [] }>() @@ -99,8 +99,10 @@ function definitionText(tool: ReviewTool) { return JSON.stringify({ input_schema function diffText(tool: ReviewTool) { return Object.values(tool.diff ?? {}).filter(Boolean).join('\n\n') || JSON.stringify(tool.previous, null, 2) } async function load() { if (typeof api.getServerReview !== 'function') return; loading.value = true; error.value = ''; const res = await api.getServerReview(props.serverName); loading.value = false; if (!res.success || !res.data) { error.value = res.error || 'Failed to load review'; return }; review.value = res.data; allowedTools.value = mergeSelection(res.data.tools, choices); emit('refreshed') } async function rescan() { + const server = props.serverName rescanning.value = true - const res = await api.startScan(props.serverName) + const res = await api.startScan(server) + if (server !== props.serverName) return // late response for a server the screen no longer shows if (!res.success) { rescanning.value = false; error.value = res.error || 'Failed to start scan' } } function requestRequarantine() { requarantineDialog.value?.showModal?.() } @@ -123,11 +125,13 @@ function requestApprove(everything: boolean) { if (!review.value?.server.definit function closeConfirm() { confirmOpen.value = false; confirmDialog.value?.close?.() } // The force retry re-sends the block list of the attempt that triggered it (D41.5). async function approve(force: boolean, block?: string[]) { + const server = props.serverName // a response for a server the screen no longer shows is dropped below closeConfirm(); forceDialog.value?.close?.(); approving.value = true const all = review.value?.tools.map(t => t.name) ?? [] const blocked = block ?? (force && lastBlock.value ? lastBlock.value : all.filter(name => !allowedTools.value.includes(name))) lastBlock.value = blocked - const res = await api.securityApprove(props.serverName, force, blocked) + const res = await api.securityApprove(server, force, blocked) + if (server !== props.serverName) return // the watcher on serverName already reset approving and the force state approving.value = false if (!res.success) { error.value = res.error || 'Approval failed'; if (!force && /dangerous/i.test(error.value)) forceDialog.value?.showModal?.(); return } choices.clear(); emit('approved'); await load() @@ -144,7 +148,7 @@ async function refreshAfterScanSettled(event: Event) { void load() } // Component reuse across /review/A -> /review/B: scan state and any pending force retry belong to the old server. -watch(() => props.serverName, () => { choices.clear(); lastBlock.value = null; forceDialog.value?.close?.(); closeConfirm(); scanning.value = false; rescanning.value = false; error.value = ''; void load() }) +watch(() => props.serverName, () => { choices.clear(); lastBlock.value = null; approving.value = false; forceDialog.value?.close?.(); closeConfirm(); scanning.value = false; rescanning.value = false; error.value = ''; void load() }) onMounted(() => { void load() window.addEventListener('mcpproxy:review-changed', refreshAfterReviewChange) diff --git a/frontend/src/utils/reviewPresentation.ts b/frontend/src/utils/reviewPresentation.ts index 8dfa7403b..291772e11 100644 --- a/frontend/src/utils/reviewPresentation.ts +++ b/frontend/src/utils/reviewPresentation.ts @@ -149,6 +149,9 @@ export function approveLabel(selected: number, total: number, definitionsCapture return `Approve server (${selected} of ${total} ${plural(total, 'tool', 'tools')})` } +/** Tooltip of the approve-everything action: it does not re-enable tools blocked earlier. */ +export const APPROVE_ALL_HINT = 'Allows every pending or changed tool. Tools you blocked earlier on a re-quarantined server stay blocked.' + /** The explicit approve-everything action. */ export function approveAllLabel(total: number): string { return `Approve all (${total} ${plural(total, 'tool', 'tools')})` diff --git a/frontend/tests/unit/review-screen-default-selection.spec.ts b/frontend/tests/unit/review-screen-default-selection.spec.ts index ca1e7c2fb..4e0871243 100644 --- a/frontend/tests/unit/review-screen-default-selection.spec.ts +++ b/frontend/tests/unit/review-screen-default-selection.spec.ts @@ -3,6 +3,7 @@ import { flushPromises, mount } from '@vue/test-utils' import ReviewScreen from '@/components/ReviewScreen.vue' import api from '@/services/api' import { + APPROVE_ALL_HINT, REVIEW_SELECTION_HINT, approveAllLabel, approveLabel, @@ -131,6 +132,8 @@ describe('ReviewScreen default selection (D41)', () => { it('Approve all sends an empty block list', async () => { const wrapper = await mountScreen() expect(wrapper.get('[data-test="review-approve-all"]').text()).toBe('Approve all (5 tools)') + expect(wrapper.get('[data-test="review-approve-all"]').attributes('title')).toBe(APPROVE_ALL_HINT) + expect(APPROVE_ALL_HINT).toContain('stay blocked') await wrapper.get('[data-test="review-approve-all"]').trigger('click') await flushPromises() expect(api.securityApprove).toHaveBeenCalledWith('fixture', false, []) @@ -183,6 +186,45 @@ describe('ReviewScreen default selection (D41)', () => { expect(api.securityApprove).toHaveBeenNthCalledWith(2, 'other', true, ['write_x']) }) + it('a late dangerous 409 for the previous server does not open the force dialog on the next one', async () => { + let resolveA: (v: { success: boolean; error?: string }) => void = () => {} + ;(api.securityApprove as any).mockReturnValueOnce(new Promise(r => { resolveA = r })) + const wrapper = await mountScreen() + const forceDialog = wrapper.findAll('dialog')[1].element as HTMLDialogElement & { showModal: () => void; close: () => void } + forceDialog.showModal = vi.fn() + forceDialog.close = vi.fn() + await wrapper.get('[data-test="review-approve-server"]').trigger('click') + await flushPromises() + // Component reuse while the approve call for "fixture" is still in flight. + ;(api.getServerReview as any).mockResolvedValue(payload([tool('read_x'), tool('write_x', { tier: 'write', default_allowed: false })])) + await wrapper.setProps({ serverName: 'other' }) + await flushPromises() + resolveA({ success: false, error: 'dangerous baseline finding' }) + await flushPromises() + expect(forceDialog.showModal).not.toHaveBeenCalled() + expect(wrapper.text()).not.toContain('dangerous baseline finding') + // The new server's own buttons are usable again and no force retry is armed. + expect((wrapper.get('[data-test="review-approve-server"]').element as HTMLButtonElement).disabled).toBe(false) + ;(api.securityApprove as any).mockResolvedValueOnce({ success: true }) + await wrapper.get('[data-test="review-approve-server"]').trigger('click') + await flushPromises() + expect(api.securityApprove).toHaveBeenLastCalledWith('other', false, ['write_x']) + }) + + it('a late rescan failure for the previous server does not set an error on the next one', async () => { + let resolveScan: (v: { success: boolean; error?: string }) => void = () => {} + ;(api.startScan as any).mockReturnValueOnce(new Promise(r => { resolveScan = r })) + const wrapper = await mountScreen() + ;(wrapper.vm as any).rescan() + await flushPromises() + ;(api.getServerReview as any).mockResolvedValue(payload([tool('read_x')])) + await wrapper.setProps({ serverName: 'other' }) + await flushPromises() + resolveScan({ success: false, error: 'scan exploded for fixture' }) + await flushPromises() + expect(wrapper.text()).not.toContain('scan exploded for fixture') + }) + it('a review-changed reload keeps an explicit uncheck and an explicit check of an unchanged tool', async () => { const wrapper = await mountScreen() await wrapper.get('[data-test="review-allow-read_file"]').setValue(false) diff --git a/native/macos/MCPProxy/MCPProxy/API/Models.swift b/native/macos/MCPProxy/MCPProxy/API/Models.swift index 287d679c4..39cb1e0af 100644 --- a/native/macos/MCPProxy/MCPProxy/API/Models.swift +++ b/native/macos/MCPProxy/MCPProxy/API/Models.swift @@ -391,7 +391,7 @@ struct ReviewScan: Codable, Equatable { struct ReviewServerSummary: Codable, Equatable { let name: String; let transport: String?; let command: String?; let url: String?; let quarantined: Bool; let trustMode: String?; let sourceRegistryID: String?; let sourceRegistryProvenance: String?; let definitionsCaptured: Bool; let scan: ReviewScan?; enum CodingKeys: String, CodingKey { case name, transport, command, url, quarantined, scan; case trustMode = "trust_mode"; case sourceRegistryID = "source_registry_id"; case sourceRegistryProvenance = "source_registry_provenance"; case definitionsCaptured = "definitions_captured" } } struct ReviewToolPrevious: Codable, Equatable { let description: String; let inputSchema: JSONValue?; let outputSchema: JSONValue?; let annotations: JSONValue?; enum CodingKeys: String, CodingKey { case description, annotations; case inputSchema = "input_schema"; case outputSchema = "output_schema" } } struct ReviewToolDiff: Codable, Equatable { let description: String?; let inputSchema: String?; let outputSchema: String?; let annotations: String?; enum CodingKeys: String, CodingKey { case description, annotations; case inputSchema = "input_schema"; case outputSchema = "output_schema" } } -struct ReviewTool: Codable, Equatable, Identifiable { let name: String; let description: String; let inputSchema: JSONValue?; let outputSchema: JSONValue?; let annotations: JSONValue?; let tier: String; let approvalStatus: String; let disabled: Bool; let scanVerdict: String; let defaultAllowed: Bool?; let previous: ReviewToolPrevious?; let diff: ReviewToolDiff?; enum CodingKeys: String, CodingKey { case name, description, annotations, tier, disabled, previous, diff; case inputSchema = "input_schema"; case outputSchema = "output_schema"; case approvalStatus = "approval_status"; case scanVerdict = "scan_verdict"; case defaultAllowed = "default_allowed" }; var id: String { name } } +struct ReviewTool: Codable, Equatable, Identifiable { let name: String; let description: String; let inputSchema: JSONValue?; let outputSchema: JSONValue?; let annotations: JSONValue?; let tier: String; let approvalStatus: String; let disabled: Bool; let scanVerdict: String; let heldReason: String?; let heldSignals: [String]?; let defaultAllowed: Bool?; let previous: ReviewToolPrevious?; let diff: ReviewToolDiff?; enum CodingKeys: String, CodingKey { case name, description, annotations, tier, disabled, previous, diff; case inputSchema = "input_schema"; case outputSchema = "output_schema"; case approvalStatus = "approval_status"; case scanVerdict = "scan_verdict"; case heldReason = "held_reason"; case heldSignals = "held_signals"; case defaultAllowed = "default_allowed" }; var id: String { name } } struct ServerReviewResponse: Codable, Equatable { let server: ReviewServerSummary; let tools: [ReviewTool] } // MARK: - OAuth Status diff --git a/native/macos/MCPProxy/MCPProxy/Views/ReviewQueueView.swift b/native/macos/MCPProxy/MCPProxy/Views/ReviewQueueView.swift index d16d5a801..b66427ded 100644 --- a/native/macos/MCPProxy/MCPProxy/Views/ReviewQueueView.swift +++ b/native/macos/MCPProxy/MCPProxy/Views/ReviewQueueView.swift @@ -109,6 +109,7 @@ enum ReviewPresentation { // helpers only read it. A missing field (an older core) reads as false, so a // mismatched core fails closed. Sentences match the Web screen. + static let approveAllHint = "Allows every pending or changed tool. Tools you blocked earlier on a re-quarantined server stay blocked." static let selectionHint = "Only read-only tools with a clean scan start checked. Unchecked tools stay blocked after approval until you enable them on the Tools tab." /// What the user explicitly chose for one tool, with the payload they saw. @@ -230,7 +231,7 @@ struct ReviewSheet: View { HStack { Button(ReviewPresentation.approveLabel(selected: allowed.count, total: review?.tools.count ?? 0, definitionsCaptured: review?.server.definitionsCaptured ?? false)) { requestApprove(everything: false) }.buttonStyle(.borderedProminent) if let review, review.server.definitionsCaptured, !review.tools.isEmpty, allowed.count < review.tools.count { - Button(ReviewPresentation.approveAllLabel(total: review.tools.count)) { requestApprove(everything: true) } + Button(ReviewPresentation.approveAllLabel(total: review.tools.count)) { requestApprove(everything: true) }.help(ReviewPresentation.approveAllHint) } Button("Reject Server", role: .destructive) { Task { await rejectServer() } } }.padding() diff --git a/native/macos/MCPProxy/MCPProxyTests/ReviewPresentationTests.swift b/native/macos/MCPProxy/MCPProxyTests/ReviewPresentationTests.swift index f3c1b184c..4c29645e3 100644 --- a/native/macos/MCPProxy/MCPProxyTests/ReviewPresentationTests.swift +++ b/native/macos/MCPProxy/MCPProxyTests/ReviewPresentationTests.swift @@ -127,9 +127,10 @@ final class ReviewPresentationTests: XCTestCase { // MARK: Default selection (Spec 109 fix-review-defaults, D41) - private func selectionTool(_ name: String, defaultAllowed: Bool?, description: String = "d", verdict: String = "clean") throws -> ReviewTool { + private func selectionTool(_ name: String, defaultAllowed: Bool?, description: String = "d", verdict: String = "clean", heldReason: String? = nil, heldSignals: [String]? = nil) throws -> ReviewTool { let field = defaultAllowed.map { ",\"default_allowed\":\($0)" } ?? "" - let json = "{\"name\":\"\(name)\",\"description\":\"\(description)\",\"tier\":\"read\",\"approval_status\":\"pending\",\"disabled\":false,\"scan_verdict\":\"\(verdict)\"\(field)}" + let held = (heldReason.map { ",\"held_reason\":\"\($0)\"" } ?? "") + (heldSignals.map { ",\"held_signals\":[" + $0.map { "\"\($0)\"" }.joined(separator: ",") + "]" } ?? "") + let json = "{\"name\":\"\(name)\",\"description\":\"\(description)\",\"tier\":\"read\",\"approval_status\":\"pending\",\"disabled\":false,\"scan_verdict\":\"\(verdict)\"\(held)\(field)}" return try JSONDecoder().decode(ReviewTool.self, from: Data(json.utf8)) } @@ -158,6 +159,13 @@ final class ReviewPresentationTests: XCTestCase { XCTAssertEqual(ReviewPresentation.mergeSelection([redefined], choices: ["write_a": .init(allowed: true, tool: writeA)]), []) let rescanned = try selectionTool("write_a", defaultAllowed: false, verdict: "warnings") XCTAssertEqual(ReviewPresentation.mergeSelection([rescanned], choices: ["write_a": .init(allowed: true, tool: writeA)]), []) + // A hold that appears after the click (held_reason / held_signals only) is a changed payload too. + let held = try selectionTool("write_a", defaultAllowed: false, heldReason: "scan_findings", heldSignals: ["tpa.x"]) + XCTAssertEqual(held.heldReason, "scan_findings") + XCTAssertEqual(held.heldSignals, ["tpa.x"]) + XCTAssertEqual(ReviewPresentation.mergeSelection([held], choices: ["write_a": .init(allowed: true, tool: writeA)]), []) + let reheld = try selectionTool("write_a", defaultAllowed: false, heldReason: "scan_coverage", heldSignals: ["tpa.x"]) + XCTAssertEqual(ReviewPresentation.mergeSelection([reheld], choices: ["write_a": .init(allowed: true, tool: held)]), []) // A choice for a tool that is gone is ignored. XCTAssertEqual(ReviewPresentation.mergeSelection([readA], choices: ["gone": .init(allowed: true, tool: writeA)]), ["read_a"]) } diff --git a/specs/109-ux-navigation-consistency/research.md b/specs/109-ux-navigation-consistency/research.md index ebae551d8..f161ab6ee 100644 --- a/specs/109-ux-navigation-consistency/research.md +++ b/specs/109-ux-navigation-consistency/research.md @@ -397,10 +397,10 @@ Finding F-02 (medium, security default): on the Review screen every "Allow this - **D41.1 The core computes the default; the surfaces only read it.** `ReviewTool` gains `default_allowed` (bool, always present). One rule in Go replaces three copies (TS, Swift, Go CLI). A payload from an older core has no field; Web, macOS and the CLI read a missing field as `false`, so a mismatched core fails closed (all unchecked). - **D41.2 The rule (`reviewDefaultAllowed`)**, in order: (1) `disabled` is false: a tool that is already blocked stays blocked; (2) `approval_status == "approved"` is true: the user or a trusted baseline already approved this exact definition, so approving a re-quarantined server with the defaults does not silently block tools that already worked; (3) `pending` or `changed` is true only when `tier == "read"`, `scan_verdict == "clean"` and `held_reason == ""`; (4) everything else is false: `write`, `destructive`, `unannotated`, `unknown`, `not_scanned`, `warnings`, `dangerous` and any held tool. The per-tool `scan_verdict` already encodes coverage (D40.2): `not_captured` gives no tools, `none`, `scanning` and `tools_not_scanned` make every tool `not_scanned` (everything unchecked), and `stale` unchecks exactly the tools that changed or were added after the scan. "All unchecked when coverage is not current" was rejected: it would also uncheck read tools whose current definitions the scan did verify. -- **D41.3 Labels** (exact count; Web sentence case, macOS title case on buttons). Primary: `Approve server (3 of 9 tools)`, singular `tool` when the total is 1. Explicit approve-all: `Approve all (9 tools)`, shown only when the server is quarantined, the total is above 0 and the selection is smaller than the total; it sends `block: []`, is a separate button with no extra dialog, and the scan gate and the dangerous-verdict force confirmation still apply. No definitions captured: `Approve without seeing tools` (what US2-5 specifies; main still rendered `Approve server (0 tools)`), with the existing blind-approval confirmation. Hint under the tool list, the same sentence on both surfaces: "Only read-only tools with a clean scan start checked. Unchecked tools stay blocked after approval until you enable them on the Tools tab." +- **D41.3 Labels** (exact count; Web sentence case, macOS title case on buttons). Primary: `Approve server (3 of 9 tools)`, singular `tool` when the total is 1. Explicit approve-all: `Approve all (9 tools)`, shown only when the server is quarantined, the total is above 0 and the selection is smaller than the total; it sends `block: []` (tools blocked earlier on a re-quarantined server stay blocked; the tooltip says so), is a separate button with no extra dialog, and the scan gate and the dangerous-verdict force confirmation still apply. No definitions captured: `Approve without seeing tools` (what US2-5 specifies; main still rendered `Approve server (0 tools)`), with the existing blind-approval confirmation. Hint under the tool list, the same sentence on both surfaces: "Only read-only tools with a clean scan start checked. Unchecked tools stay blocked after approval until you enable them on the Tools tab." - **D41.4 A reload keeps the user's choices.** `load()` used to reset the selection on every `review.changed` event (for any server) and on scan-settled. The surfaces keep a per-server map `tool name -> {allowed, tool payload at the time of the click}`: an explicit uncheck always survives a reload; an explicit check survives only while the payload is deep-equal to the one the user saw, otherwise the tool falls back to `default_allowed`. The map is cleared when the server changes and after a successful approve. - **D41.5 The force retry reuses the attempted selection.** After a 409 "dangerous", the force dialog re-sends the same block list as the attempt that triggered it (primary button or Approve all). Web: `lastBlock`; macOS: `pendingBlock`. -- **D41.6 The CLI uses the same defaults; the selection flags are exclusive.** `review approve ` on a quarantined server blocks every tool whose `default_allowed` is not true; `--all` allows every tool; `--tools a,b` (now valid on a quarantined server) allows exactly those and blocks the rest; `--except a,b` subtracts from whichever base was chosen; `--all` with `--tools` is an error; a name that is not in the review is an error before any write (`unknown tool 'x' for server 's'`). The confirmation prompt runs after the review read and names the exact count. In table output one line precedes the result (`Allowing 3 of 9 tools; blocking 6: a, b, ...`); JSON and YAML stay the REST data object. On a trusted server `--all` is an alias of the default (`approve_all: true`) and `--except` is still rejected. Behaviour change: `review approve s --yes` used to allow every tool and now allows the default selection. `mcpproxy review` has not shipped in any release (no tag contains #1412), so the CLI can match Web and macOS without breaking a released contract. +- **D41.6 The CLI uses the same defaults; the selection flags are exclusive.** `review approve ` on a quarantined server blocks every tool whose `default_allowed` is not true; `--all` allows every pending or changed tool (tools blocked earlier stay blocked); `--tools a,b` (now valid on a quarantined server) allows exactly those and blocks the rest; `--except a,b` subtracts from whichever base was chosen; `--all` with `--tools` is an error; a name that is not in the review is an error before any write (`unknown tool 'x' for server 's'`). The confirmation prompt runs after the review read and names the exact count. In table output one line precedes the result (`Allowing 3 of 9 tools; blocking 6: a, b, ...`); JSON and YAML stay the REST data object. On a trusted server `--all` is an alias of the default (`approve_all: true`) and `--except` is still rejected. Behaviour change: `review approve s --yes` used to allow every tool and now allows the default selection. `mcpproxy review` has not shipped in any release (no tag contains #1412), so the CLI can match Web and macOS without breaking a released contract. - **D41.7 MCP: no semantic change, field parity only.** `inspect_quarantined` on the captured path serialises the composer and carries `default_allowed`; on the live path every tool has `default_allowed: false`. `inspect_tools` does not carry the field: it is a UI selection hint, not an approval state. No `quarantine_security` operation approves a server (parity row 6 stays `out`); `approve_tool` and `approve_all_tools` approve only what they name. - **D41.8 No new FR and no new acceptance scenario.** The traceability test pins 43 scenarios and the FR ids. As in D40.5, FR-021 (the field), FR-023 (the defaults and labels) and US2-2 (the scenario text) are amended, and the new tests map into `acceptance-index.json` US2-2 and US2-5. - **D41.9 The `?change=` filter.** Hidden rows keep their default state and the count always covers every tool, so `3 of 9` stays honest when only changed rows are shown. diff --git a/specs/109-ux-navigation-consistency/spec.md b/specs/109-ux-navigation-consistency/spec.md index 77ddfc71d..c158873f6 100644 --- a/specs/109-ux-navigation-consistency/spec.md +++ b/specs/109-ux-navigation-consistency/spec.md @@ -110,7 +110,7 @@ A user reviewing a quarantined server or a changed tool sees each captured tool' **Acceptance Scenarios**: 1. **Given** a quarantined server with captured definitions, **When** the user opens its review screen (from Home, Review queue, the server card, or `/servers/?tab=review`), **Then** every tool is listed read-only with name, description (rendered as inert text and labelled "from the server, not verified"), tier (`read`, `write`, `destructive`, `unannotated`, or `unknown` for a tool whose definition was captured before this spec stored annotations, FR-021/FR-028), annotations and per-tool scan verdict, plus the server's baseline scan verdict, risk score and whether the scan covers the current definitions. Nothing is callable by agents. -2. **Given** the review screen, **Then** only read tools with a clean scan start checked and the button reads "Approve server (5 of 14 tools)"; **When** the user presses it, **Then** the scan-gated server approval runs (a dangerous verdict requires the existing force confirmation), the server is unquarantined, every unchecked tool is blocked (approved and disabled), and one activity record per state change is written. "Approve all (14 tools)" approves every tool. The same outcome results from the macOS sheet and from `mcpproxy review approve ` (defaults), `--all`, `--tools` or `--except`. +2. **Given** the review screen, **Then** only read tools with a clean scan start checked and the button reads "Approve server (5 of 14 tools)"; **When** the user presses it, **Then** the scan-gated server approval runs (a dangerous verdict requires the existing force confirmation), the server is unquarantined, every unchecked tool is blocked (approved and disabled), and one activity record per state change is written. "Approve all (14 tools)" approves every pending or changed tool (tools blocked earlier on a re-quarantined server stay blocked). The same outcome results from the macOS sheet and from `mcpproxy review approve ` (defaults), `--all`, `--tools` or `--except`. 3. **Given** the macOS app, **When** the user presses "Approve Server", **Then** it calls `security/approve` (never `unquarantine`), shows the same dangerous-verdict confirmation as the Web UI, and results in the same integrity baseline (X2). 4. **Given** a trusted server with a `changed` tool, **When** the user opens the Review queue, **Then** the item shows a description and schema diff (previous → current) with Approve and Reject. Reject blocks the tool (approved and disabled) and never deletes it. If the change came after the last scan, the scan line says the scan is out of date and offers Rescan; it never shows the earlier clean verdict as current. 5. **Given** a quarantined server whose definitions were never captured, **Then** the review screen says "Tool definitions not captured yet" and offers "Fetch tool definitions" (an inspection-only `discover-tools` capture under the existing exemption; it stores review records without indexing them). Approve stays available, labelled "Approve without seeing tools", behind a confirmation. After an automatic baseline scan has listed the server's tools, MCPProxy captures the definitions itself, so this state is seen only when no scan has listed them. @@ -260,7 +260,7 @@ A first-time user connects a client and imports servers in the wizard. Import ro - **FR-020**: Tool approval records MUST store captured annotations (`current_annotations`, `previous_annotations`) when a definition is captured. Annotations stay excluded from the approval hash (`tool_quarantine.go:26`), so no re-quarantine is caused. - **FR-021**: `GET /api/v1/servers/{id}/review` MUST return the server's review payload: server summary (transport, command or URL — **passed through the shared secret-redaction path** `oauth.RedactServerSecretFields`/`LiveRedaction` (`internal/oauth/serverfields.go`) — the helper the `GET /servers` list and the SSE `servers.changed` payload use, but called **unconditionally**: the review composer never honours the administrator `reveal_secret_headers` opt-out that `GET /servers` does (there is no `GET /servers/{id}` read route at `638fa805a`) — before it is returned to **any** caller, so an inline secret in a URL query, header, env value or stdio command line is never echoed verbatim; `GET /review` rows and the CLI/MCP review reads use the same composer and therefore the same redaction — the review composer is the fourth door on that path, pinned by a redaction-parity test, trust mode, baseline scan verdict, risk score and its coverage (`scan.coverage` = `current`|`stale`|`not_captured`|`tools_not_scanned`|`scanning`|`none`, `scan.tools_scanned`, `scan.unscanned_tools`: whether the latest completed scan analysed every captured definition as it is now), and the existing catalog origin `source_registry_id`/`source_registry_provenance` when present), and per tool: name, description, input/output schema, annotations, `tier` (`read`|`write`|`destructive`|`unannotated`|`unknown`, computed from annotations; unannotated is never shown as read), `approval_status`, `scan_verdict` (`clean` when the covering scan found nothing for the tool; `not_scanned`, or the held verdict, when the tool's current definition postdates the scan), `held_signals` and `default_allowed` (the review screen's fail-closed default selection, computed by the core: true only for an approved and enabled tool, or a pending/changed `read` tool whose `scan_verdict` is `clean` and that is not held; always present, so an older core without the field reads as false), and for `changed` tools the previous description, schema and a diff. `GET /api/v1/review` MUST return the queue: quarantined servers and servers with pending/changed tools, with counts. Shapes: [contracts/rest-api.md](contracts/rest-api.md#review). - **FR-022**: The four review verbs MUST be the only first-party review operations: Approve server = `POST /servers/{id}/security/approve` (scan-gated, `force` only after the dangerous-verdict confirmation), optionally with `block: [tools]` for tools unchecked on the review screen — the `block` tools MUST be written as blocked — in the existing `BlockTools` representation (`internal/runtime/tool_quarantine.go`): approval record `status=approved`, `disabled=true`; there is no `blocked` status value (`storage.ToolApprovalStatus*` is `approved|pending|changed`, and FR-027's terminology keeps exactly those three) — **in the same storage transaction that writes the baseline and before the server is unquarantined**, through one new storage method on the scanner's storage seam (`scanner.Storage.SaveIntegrityBaselineWithBlocks(baseline, blocked []ToolApprovalWrite)`, one bbolt update transaction; the seam has no tool-approval operation today), so no instant exists in which the server is callable and a `block` tool is approved; today's `ApproveServer` saves the baseline and then calls `UnquarantineServer` as a separate step (`internal/security/scanner/service.go`), which the block write MUST precede, never follow; Reject server = `POST /servers/{id}/security/reject`; Approve tool = `POST /servers/{id}/tools/approve`; Reject tool = `POST /servers/{id}/tools/block`. The macOS app MUST stop calling `POST /unquarantine` for approval (X2). The Go tray (`internal/tray`, the `mcpproxy-tray` binary built for Windows and macOS: the Windows installer's tray and the tray inside the macOS/Windows release archives, while the macOS DMG/PKG ships the Swift app instead) MUST stop unquarantining on a click in its "Security Quarantine" submenu (X12): the click opens the server's review location in the Web UI instead. The endpoint stays for API compatibility and is used by no first-party surface. -- **FR-023**: The Web UI MUST provide a Review queue page (`/review`) and a review screen (`/review/:server`, also embedded as the server detail "Review" tab) implementing FR-021/FR-022, with per-tool checkboxes that start in the core's default selection (`default_allowed`): write, destructive, unannotated, unknown, not-scanned and held tools start unchecked. The approve button names the exact count ("Approve server (3 of 9 tools)"; "Approve without seeing tools" when nothing is captured), "Approve all (9 tools)" is a separate explicit action, and a reload keeps the user's choices; diff rendering for changed tools, and "Fetch tool definitions" when nothing is captured. The review screen shows a scan that does not cover the current definitions as a warning with its action (Rescan, Scan now, Fetch tool definitions), and shows a risk score only for a covering scan. On a server that is not quarantined, approved tools show Approved or Blocked instead of Approve/Reject, the heading states that the server is approved, and Quarantine to review again (the existing quarantine action, with a confirmation) is offered. `/security` redirects to `/review`. Scan history stays reachable from the review screen and `/security/scans/:jobId`. Scanner configuration moves to Settings → Security → Scanners, where the Docker toggle is shown once. +- **FR-023**: The Web UI MUST provide a Review queue page (`/review`) and a review screen (`/review/:server`, also embedded as the server detail "Review" tab) implementing FR-021/FR-022, with per-tool checkboxes that start in the core's default selection (`default_allowed`): write, destructive, unannotated, unknown, not-scanned and held tools start unchecked. The approve button names the exact count ("Approve server (3 of 9 tools)"; "Approve without seeing tools" when nothing is captured), "Approve all (9 tools)" is a separate explicit action that allows every pending or changed tool (previously blocked tools stay blocked; the tooltip says so), and a reload keeps the user's choices; diff rendering for changed tools, and "Fetch tool definitions" when nothing is captured. The review screen shows a scan that does not cover the current definitions as a warning with its action (Rescan, Scan now, Fetch tool definitions), and shows a risk score only for a covering scan. On a server that is not quarantined, approved tools show Approved or Blocked instead of Approve/Reject, the heading states that the server is approved, and Quarantine to review again (the existing quarantine action, with a confirmation) is offered. `/security` redirects to `/review`. Scan history stays reachable from the review screen and `/security/scans/:jobId`. Scanner configuration moves to Settings → Security → Scanners, where the Docker toggle is shown once. - **FR-024**: The macOS app MUST provide a "Review Queue" sidebar item and a review sheet with the same content and verbs. The existing detail-view per-tool approval is reused, and the tray's "Needs Attention" review rows open the sheet. - **FR-025**: The CLI MUST provide `mcpproxy review list|show |approve [--tools a,b | --except a,b] [--force]|reject [--tools a,b]` with the FR-022 semantics: `approve` on a quarantined server runs Approve server; on a trusted server it approves pending/changed tools. `upstream approve`, `tools approve|reject` (`tools_approval.go`) and `security approve|reject` remain as aliases whose help text names the `review` equivalent. Their behaviour does not change. - **FR-026**: MCP `quarantine_security` `inspect_quarantined` and `inspect_tools` MUST return per-tool `tier`, `annotations` and `scan_verdict` with the same field names and values as FR-021. `quarantine_security` remains administrator-only for every operation, including inspection, under Spec 028 FR-009 and Spec 108 FR-016; an agent-token caller is denied before server lookup or inspection. Scoped review reads are available through the REST routes in FR-007. Agents still cannot approve or unquarantine a server (accepted asymmetry, see the contradiction register).