Skip to content

Commit 335477e

Browse files
authored
fix(review): review screen starts fail-closed with exact-count approve (Spec fix-review-defaults) (#1481)
## What changes for users The review screen used to start every "Allow this tool" checkbox checked, including write and destructive tools whose definitions were never verified. It now fails closed. - **Core / REST**: every tool in `GET /api/v1/servers/{id}/review` carries `default_allowed` (always present). It is true for an approved, enabled tool, and for a pending or changed read tool whose scan verdict is clean and that is not held. Write, destructive, unannotated, unknown, not-scanned, warnings, dangerous and held tools are false. An older core has no field, which every surface reads as false. - **Web**: only the core-selected tools start checked. A hint explains why. The approve button names the exact count ("Approve server (5 of 14 tools)", or "Approve without seeing tools" when nothing is captured). A separate "Approve all (14 tools)" button approves everything. A reload keeps the user's explicit choices (an explicit check is dropped if the tool's definition, verdict or tier changed underneath it). The forced retry after a dangerous finding re-sends the block list of the attempt that triggered it. - **macOS**: the review sheet has the same defaults, hint, labels, Approve All button, reload behaviour and force retry. - **CLI**: `mcpproxy review approve <server>` on a quarantined server blocks every tool outside the default selection. `--all` allows every tool, `--tools a,b` (now valid on a quarantined server) allows exactly those, `--except` subtracts, and `--all` with `--tools` or an unknown tool name is an error before any write. The prompt now follows the review read and names the count and the blocked tools. Table output prints one summary line first; JSON and YAML stay the bare REST object. Behaviour change: `review approve <server> --yes` used to allow every tool; add `--all` for that. The command has not shipped in a release. - **MCP**: no semantic change. `inspect_quarantined` carries `default_allowed` (captured tools from the composer, live tools `false`); `inspect_tools` does not. No `quarantine_security` operation approves a server. ## Findings and spec Codex user test F-02 (medium, security default). Spec: `specs/109-ux-navigation-consistency` (FR-021, FR-023, US2-2; research D41; tasks T199 to T210; contracts, data model, quickstart recipe, acceptance index and parity matrix row 6 updated). Docs updated: `docs/features/security-quarantine.md`, `docs/cli/review-commands.md`, `docs/cli/command-reference.md`, `docs/api/rest-api.md`. Accepted residual: a server that declares `readOnlyHint` dishonestly still gets its read tools pre-checked once the scan is clean, because annotations are self-declared and the scan checks descriptions. The tier stays visible and the tool can be blocked later. ## Tests added - Go: `TestReviewDefaultAllowed` (tier by status by verdict by disabled by held table), coverage-driven composer cases, JSON key present when false, MCP captured and live parity, CLI default selection, `--all`, `--tools`, prompt count, JSON output, `reviewApproveSelection` table, three goldens. - Web: `review-screen-default-selection.spec.ts` (helpers and component); three assertions in `review-screen.spec.ts` re-baselined on purpose because they pinned the old all-checked default. - macOS: `ReviewPresentationTests` (selection, merge, labels, hint parity with the Web sentence) and `ReviewPayloadTests` decode cases.
1 parent 8e5ccd3 commit 335477e

38 files changed

Lines changed: 1355 additions & 84 deletions

‎ROADMAP.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1036,6 +1036,6 @@ Legend: `shipped` ≥95% checked · `in-flight` 1–94% · `drafted` 0% · `—`
10361036
| [106-security-residual-fixes](./specs/106-security-residual-fixes/) | `shipped` | 18/19 (95%) |
10371037
| [107-server-edition-sso-hardening](./specs/107-server-edition-sso-hardening/) | `shipped` | 126/126 (100%) |
10381038
| [108-profiles-v3](./specs/108-profiles-v3/) | `shipped` | 194/195 (99%) |
1039-
| [109-ux-navigation-consistency](./specs/109-ux-navigation-consistency/) | `shipped` | 245/246 (100%) |
1039+
| [109-ux-navigation-consistency](./specs/109-ux-navigation-consistency/) | `shipped` | 257/258 (100%) |
10401040
| [110-catalog-popularity](./specs/110-catalog-popularity/) | `in-flight` | 19/23 (83%) |
10411041
| [112-client-header-forwarding](./specs/112-client-header-forwarding/) | `shipped` | 38/40 (95%) |

‎cmd/mcpproxy/review_cmd.go‎

Lines changed: 144 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ func newReviewCommand(confirm func(string) (bool, error)) *cobra.Command {
2525
var tools []string
2626
var force bool
2727
var yes bool
28+
var all bool
2829
cmd := &cobra.Command{Use: "review", Short: "Inspect and decide quarantined server reviews"}
2930
cmd.AddCommand(&cobra.Command{Use: "list", Short: "List servers needing review", RunE: func(_ *cobra.Command, _ []string) error {
3031
return runReviewRead("/api/v1/review")
@@ -36,40 +37,59 @@ func newReviewCommand(confirm func(string) (bool, error)) *cobra.Command {
3637
show.Flags().Bool("full", false, "Show full captured schemas and descriptions")
3738
cmd.AddCommand(show)
3839
approve := &cobra.Command{Use: "approve <server>", Short: "Approve a server through the scan gate", Args: cobra.ExactArgs(1), RunE: func(_ *cobra.Command, args []string) error {
39-
if !yes {
40-
confirmed, err := confirm(fmt.Sprintf("Approve review for server '%s'?", args[0]))
41-
if err != nil {
42-
return err
43-
}
44-
if !confirmed {
45-
fmt.Println("Approval cancelled.")
46-
return nil
47-
}
40+
server := args[0]
41+
if all && len(tools) > 0 {
42+
return errReviewAllWithTools
4843
}
49-
quarantined, err := reviewServerQuarantined(args[0])
44+
state, err := reviewServerState(server)
5045
if err != nil {
5146
return err
5247
}
53-
if quarantined {
48+
if !state.quarantined {
49+
if len(except) > 0 {
50+
return fmt.Errorf("--except applies only while approving a quarantined server")
51+
}
52+
if !yes {
53+
confirmed, err := confirm(fmt.Sprintf("Approve review for server '%s'?", server))
54+
if err != nil || !confirmed {
55+
return reviewDeclined("Approval cancelled.", err)
56+
}
57+
}
58+
body := map[string]interface{}{"approve_all": true}
5459
if len(tools) > 0 {
55-
return fmt.Errorf("--tools cannot select a quarantined server approval; use --except to block tools")
60+
body = map[string]interface{}{"tools": tools}
5661
}
57-
body := map[string]interface{}{"force": force}
58-
if len(except) > 0 {
59-
body["block"] = except
62+
return runReviewWrite(server, "tools/approve", body)
63+
}
64+
body := map[string]interface{}{"force": force}
65+
prompt := fmt.Sprintf("Approve server '%s' without seeing tools?", server)
66+
var summary string
67+
if len(state.tools) > 0 {
68+
block, allowed, err := reviewApproveSelection(state.tools, all, tools, except)
69+
if err != nil {
70+
return fmt.Errorf("%w for server '%s'", err, server)
71+
}
72+
if len(block) > 0 {
73+
body["block"] = block
6074
}
61-
return runReviewWrite(args[0], "security/approve", body)
75+
prompt, summary = reviewApproveWording(server, len(state.tools), allowed, block)
76+
} else if len(tools) > 0 || len(except) > 0 {
77+
// Nothing to select from: dropping --tools/--except would approve blind.
78+
return fmt.Errorf("no tool definitions captured for server '%s'; fetch them first (mcpproxy review show %s) before using --tools or --except", server, server)
6279
}
63-
if len(except) > 0 {
64-
return fmt.Errorf("--except applies only while approving a quarantined server")
80+
if !yes {
81+
confirmed, err := confirm(prompt)
82+
if err != nil || !confirmed {
83+
return reviewDeclined("Approval cancelled.", err)
84+
}
6585
}
66-
body := map[string]interface{}{"approve_all": true}
67-
if len(tools) > 0 {
68-
body = map[string]interface{}{"tools": tools}
86+
if summary != "" && ResolveOutputFormat() == "table" {
87+
fmt.Println(summary)
6988
}
70-
return runReviewWrite(args[0], "tools/approve", body)
89+
return runReviewWrite(server, "security/approve", body)
7190
}}
72-
approve.Flags().StringSliceVar(&tools, "tools", nil, "Approve only these pending or changed tools on a trusted server")
91+
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)")
92+
approve.Flags().StringSliceVar(&tools, "tools", nil, "Approve exactly these tools and block the rest (trusted server: only these pending or changed tools)")
7393
approve.Flags().StringSliceVar(&except, "except", nil, "Block these tools while approving the server")
7494
approve.Flags().BoolVar(&force, "force", false, "Force approval when the scan verdict is dangerous")
7595
approve.Flags().BoolVar(&yes, "yes", false, "Confirm the approval")
@@ -147,36 +167,130 @@ func runReviewWrite(server, operation string, value interface{}) error {
147167
return formatReviewResponse(ResolveOutputFormat(), response, false)
148168
}
149169

150-
func reviewServerQuarantined(server string) (bool, error) {
170+
var errReviewAllWithTools = fmt.Errorf("--all cannot be combined with --tools")
171+
172+
// reviewDeclined reports a declined confirmation as a clean exit.
173+
func reviewDeclined(message string, err error) error {
174+
if err != nil {
175+
return err
176+
}
177+
fmt.Println(message)
178+
return nil
179+
}
180+
181+
// reviewToolState is the part of a review tool the approval selection needs.
182+
// DefaultAllowed is nil when the core predates the field, which counts as
183+
// false: a mismatched core fails closed (D43.1).
184+
type reviewToolState struct {
185+
Name string `json:"name"`
186+
Tier string `json:"tier"`
187+
DefaultAllowed *bool `json:"default_allowed"`
188+
}
189+
190+
type reviewServerStateResult struct {
191+
quarantined bool
192+
tools []reviewToolState
193+
}
194+
195+
func reviewServerState(server string) (reviewServerStateResult, error) {
151196
client, _, err := newSecurityCLIClient()
152197
if err != nil {
153-
return false, err
198+
return reviewServerStateResult{}, err
154199
}
155200
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
156201
defer cancel()
157202
resp, err := client.DoRaw(ctx, http.MethodGet, "/api/v1/servers/"+url.PathEscape(server)+"/review", nil)
158203
if err != nil {
159-
return false, err
204+
return reviewServerStateResult{}, err
160205
}
161206
defer resp.Body.Close()
162207
body, err := io.ReadAll(resp.Body)
163208
if err != nil {
164-
return false, err
209+
return reviewServerStateResult{}, err
165210
}
166211
if resp.StatusCode != http.StatusOK {
167-
return false, parseAPIError(body, resp.StatusCode, "read review")
212+
return reviewServerStateResult{}, parseAPIError(body, resp.StatusCode, "read review")
168213
}
169214
var envelope struct {
170215
Data struct {
171216
Server struct {
172217
Quarantined bool `json:"quarantined"`
173218
} `json:"server"`
219+
Tools []reviewToolState `json:"tools"`
174220
} `json:"data"`
175221
}
176222
if err := json.Unmarshal(body, &envelope); err != nil {
177-
return false, err
223+
return reviewServerStateResult{}, err
224+
}
225+
return reviewServerStateResult{quarantined: envelope.Data.Server.Quarantined, tools: envelope.Data.Tools}, nil
226+
}
227+
228+
// reviewApproveSelection resolves which tools a quarantined-server approval
229+
// allows, the same rule the Web and macOS review screens use. The base is the
230+
// core's default selection, every tool (all) or exactly the named tools (only);
231+
// except then subtracts. block is every other tool, in review order. A name
232+
// that is not in the review is an error before anything is written.
233+
func reviewApproveSelection(tools []reviewToolState, all bool, only, except []string) (block []string, allowed int, err error) {
234+
if all && len(only) > 0 {
235+
return nil, 0, errReviewAllWithTools
236+
}
237+
known := make(map[string]bool, len(tools))
238+
for _, tool := range tools {
239+
known[tool.Name] = true
240+
}
241+
toSet := func(names []string) (map[string]bool, error) {
242+
set := make(map[string]bool, len(names))
243+
for _, name := range names {
244+
if !known[name] {
245+
return nil, fmt.Errorf("unknown tool '%s'", name)
246+
}
247+
set[name] = true
248+
}
249+
return set, nil
250+
}
251+
onlySet, err := toSet(only)
252+
if err != nil {
253+
return nil, 0, err
254+
}
255+
exceptSet, err := toSet(except)
256+
if err != nil {
257+
return nil, 0, err
258+
}
259+
for _, tool := range tools {
260+
var allow bool
261+
switch {
262+
case all:
263+
allow = true
264+
case len(only) > 0:
265+
allow = onlySet[tool.Name]
266+
default:
267+
allow = tool.DefaultAllowed != nil && *tool.DefaultAllowed
268+
}
269+
if allow && !exceptSet[tool.Name] {
270+
allowed++
271+
} else {
272+
block = append(block, tool.Name)
273+
}
274+
}
275+
return block, allowed, nil
276+
}
277+
278+
// reviewApproveWording builds the confirmation prompt and the table-mode
279+
// summary line, both naming the exact count.
280+
func reviewApproveWording(server string, total, allowed int, block []string) (prompt, summary string) {
281+
noun := func(n int) string {
282+
if n == 1 {
283+
return "tool"
284+
}
285+
return "tools"
286+
}
287+
if len(block) == 0 {
288+
return fmt.Sprintf("Approve server '%s' with all %d %s?", server, total, noun(total)),
289+
fmt.Sprintf("Allowing %d of %d %s; blocking none", allowed, total, noun(total))
178290
}
179-
return envelope.Data.Server.Quarantined, nil
291+
list := strings.Join(block, ", ")
292+
return fmt.Sprintf("Approve server '%s' with %d of %d %s? Blocked: %s.", server, allowed, total, noun(total), list),
293+
fmt.Sprintf("Allowing %d of %d %s; blocking %d: %s", allowed, total, noun(total), len(block), list)
180294
}
181295

182296
// formatReviewResponse drops the REST envelope so CLI JSON/YAML is exactly the

0 commit comments

Comments
 (0)