From 4ef9db487ec3301b349758d4cd9730d692c51050 Mon Sep 17 00:00:00 2001 From: peon <9ac6794b000690b7e814eb1805ad32405d0bec7d52838de3a86cf967565dacc0@buzz.block.builderlab.xyz> Date: Fri, 14 Aug 2026 17:28:48 -0700 Subject: [PATCH 1/5] fix(doctor): remove renderer-provided shell commands from doctor fixes The `run_doctor_fix` Tauri command accepted a renderer-controlled `command_override: Option` and forwarded it verbatim to the doctor crate's shell executor, giving a compromised renderer a direct native shell primitive. The agent-setup update path had the same hole: `SetupPlan.update_commands` carried renderer-supplied command strings run verbatim as `command_override`. Remove arbitrary command text from the renderer/backend contract: - `run_doctor_fix` now takes only `(check_id, fix_type)`. Commands are resolved from trusted backend state (managed installer, local check registry, or the crate's static `lookup_fix_command`). Unknown checks and check/fix-type combinations with no registered fix are rejected by the crate rather than executing arbitrary text. - The agent-setup plan carries typed `update_fix_types` (`updateMain` / `updateBridge`) instead of command strings. The backend re-resolves each command from the crate's trusted freshness readout via `resolve_update_command`, which rejects forged/mismatched fix types, absent readouts, and readouts with no actionable update. - Frontend `runDoctorFix`, `AgentSetupPlan`, and `AgentProviderCard` updated to the typed contract; no command string crosses the wire. Adds Rust regressions (forged/unknown/mismatched fixes fail, registered fixes still resolve) and a frontend contract regression (no command ever forwarded to `run_doctor_fix`). Co-authored-by: Kalvin Chau Signed-off-by: Kalvin Chau --- src-tauri/src/commands/agent_setup.rs | 187 +++++++++++++++--- src-tauri/src/commands/doctor.rs | 61 ++++-- src/features/providers/api/agentSetup.ts | 15 +- .../providers/stores/agentSetupStore.test.ts | 2 +- .../settings/ui/AgentProviderCard.tsx | 25 +-- src/features/settings/ui/DoctorCheckRow.tsx | 8 +- .../ui/__tests__/AgentProviderCard.test.tsx | 43 ++-- src/shared/api/__tests__/doctor.test.ts | 22 +++ src/shared/api/doctor.ts | 3 +- 9 files changed, 269 insertions(+), 97 deletions(-) diff --git a/src-tauri/src/commands/agent_setup.rs b/src-tauri/src/commands/agent_setup.rs index 51f363ab7..6b97a4883 100644 --- a/src-tauri/src/commands/agent_setup.rs +++ b/src-tauri/src/commands/agent_setup.rs @@ -126,9 +126,12 @@ pub struct SetupPlan { /// CLI, `bridge` for a missing ACP bridge). `null` for a pure update/auth. #[serde(default)] install_fix_type: Option, - /// Per-readout source-aware update commands to run after the install loop. + /// Per-readout update fix identities to run after the install loop. Only + /// `updateMain` / `updateBridge` are valid; the exact source-aware command + /// is resolved by the backend from the crate's trusted freshness readout, + /// never supplied verbatim by the renderer. #[serde(default)] - update_commands: Vec, + update_fix_types: Vec, /// Whether the post-fix step probes PATH to confirm the agent resolved on /// disk. The frontend sends `hasBinary && !isBuiltIn`: a built-in or /// binary-less provider has nothing to resolve, so a clean fix run is taken @@ -150,15 +153,6 @@ pub struct SetupPlan { bundled_bridge: bool, } -#[derive(Debug, serde::Deserialize)] -#[serde(rename_all = "camelCase")] -struct UpdateCommand { - /// `updateMain` or `updateBridge`. - fix_type: FixType, - /// The readout's `updateCommand`, run verbatim as `command_override`. - command: String, -} - /// Managed Tauri state: `providerId -> SetupOperation`. Keying by provider lets /// installs across different providers run concurrently. The spawned task owns /// an `Arc` clone so it keeps writing after `start_agent_setup` returns. @@ -343,13 +337,32 @@ async fn setup_env_vars(app: &AppHandle) -> Vec<(String, String)> { } async fn find_check(app: &AppHandle, provider_id: &str) -> Result { + find_check_with_options(app, provider_id, false).await +} + +/// Like [`find_check`], but runs the crate's freshness pass so the returned +/// check carries populated per-readout `update_command` / `update_fix_type` +/// fields. Used to resolve the source-aware update command for a requested +/// update fix from trusted crate state, rather than trusting a renderer string. +async fn find_check_fresh( + app: &AppHandle, + provider_id: &str, +) -> Result { + find_check_with_options(app, provider_id, true).await +} + +async fn find_check_with_options( + app: &AppHandle, + provider_id: &str, + check_freshness: bool, +) -> Result { let target = crate_check_id(provider_id); let env_vars = setup_env_vars(app).await; let bundled_tools_dir = managed_acp_tools::bundled_tools_dir_for_checks(app); let mut report = doctor::run_checks_with_options( doctor::RunChecksOptions { npm_registry: npm_registry(app), - check_freshness: false, + check_freshness, offline: false, env: None, // The crate labels binaries resolving from this dir as bundled and @@ -432,6 +445,53 @@ async fn verify_installed( } } +/// Resolve the exact, source-aware update command for a requested update fix +/// from the crate's freshness readout — the trusted source of truth. The +/// renderer names only the readout slot (`updateMain` / `updateBridge`); the +/// command string never crosses the wire. +/// +/// Rejects (returns `Err`) when: +/// - `fix_type` is not one of the two update slots (a forged/mismatched fix); +/// - the addressed readout is absent (`main` / `bridge` is `None`); +/// - the readout reports no actionable update (`update_command` is `None`), or +/// its own `update_fix_type` doesn't match the requested slot. +fn resolve_update_command( + check: &doctor::DoctorCheck, + fix_type: &FixType, +) -> Result { + let readout = match fix_type { + FixType::UpdateMain => check.main.as_ref(), + FixType::UpdateBridge => check.bridge.as_ref(), + other => { + return Err(format!( + "unsupported update fix type '{other:?}' for '{}'", + check.id + )); + } + }; + let readout = readout.ok_or_else(|| { + format!( + "no '{fix_type:?}' readout available for '{}' to resolve an update command", + check.id + ) + })?; + // Both fields are set together by the crate's freshness pass, and only when + // the update is actionable; a mismatched slot means the requested update + // isn't the one the readout offers. + if readout.update_fix_type.as_ref() != Some(fix_type) { + return Err(format!( + "'{fix_type:?}' does not match the actionable update for '{}'", + check.id + )); + } + readout.update_command.clone().ok_or_else(|| { + format!( + "no actionable '{fix_type:?}' update command for '{}'", + check.id + ) + }) +} + /// The install recipe a check still needs, if any. Only the two *install* fix /// types qualify — `Auth` (installed-but-signed-out) and the per-readout update /// types are handled by later chain steps, not the install loop. @@ -552,16 +612,16 @@ async fn run_install( // Update-after-install: a partial install with stale binaries (the "Fix" // state) is brought fully current in the same pass; for a plain install this - // list is empty and the loop is a no-op. - for update in &plan.update_commands { - run_fix( - app, - registry, - provider_id, - update.fix_type.clone(), - Some(update.command.clone()), - ) - .await?; + // list is empty and the loop is a no-op. The renderer only names *which* + // readouts to update (`updateMain` / `updateBridge`); the exact source-aware + // command is resolved here from the crate's trusted freshness readout, so a + // compromised renderer can't smuggle an arbitrary shell command through. + if !plan.update_fix_types.is_empty() { + let fresh = find_check_fresh(app, provider_id).await?; + for fix_type in &plan.update_fix_types { + let command = resolve_update_command(&fresh, fix_type)?; + run_fix(app, registry, provider_id, fix_type.clone(), Some(command)).await?; + } } // Only enter the visible Checking phase when there's a binary to probe; @@ -831,6 +891,87 @@ mod tests { } } + /// Build a readout with a paired `(update_command, update_fix_type)`, as + /// the crate's freshness pass emits for an actionable update. + fn readout_with_update(command: &str, fix_type: FixType) -> doctor::types::AgentVersionInfo { + doctor::types::AgentVersionInfo { + update_command: Some(command.to_string()), + update_fix_type: Some(fix_type), + ..Default::default() + } + } + + #[test] + fn resolve_update_command_returns_the_trusted_readout_command() { + let mut check = check_with_fix(None); + check.main = Some(readout_with_update( + "npm install -g @anthropic-ai/claude-code@latest", + FixType::UpdateMain, + )); + check.bridge = Some(readout_with_update( + "npm install -g claude-agent-acp@latest", + FixType::UpdateBridge, + )); + + assert_eq!( + resolve_update_command(&check, &FixType::UpdateMain).unwrap(), + "npm install -g @anthropic-ai/claude-code@latest" + ); + assert_eq!( + resolve_update_command(&check, &FixType::UpdateBridge).unwrap(), + "npm install -g claude-agent-acp@latest" + ); + } + + #[test] + fn resolve_update_command_rejects_non_update_fix_types() { + // A forged plan naming an install/auth fix as an "update" must never + // resolve to a command — those are not update slots. + let mut check = check_with_fix(None); + check.main = Some(readout_with_update("brew upgrade codex", FixType::UpdateMain)); + + for forged in [FixType::Command, FixType::Bridge, FixType::Auth] { + assert!( + resolve_update_command(&check, &forged).is_err(), + "{forged:?} must be rejected" + ); + } + } + + #[test] + fn resolve_update_command_rejects_absent_readout() { + // No `main` / `bridge` readout means there is no trusted command to run. + let check = check_with_fix(None); + assert!(resolve_update_command(&check, &FixType::UpdateMain).is_err()); + assert!(resolve_update_command(&check, &FixType::UpdateBridge).is_err()); + } + + #[test] + fn resolve_update_command_rejects_mismatched_slot() { + // The bridge readout carries a bridge update; requesting `updateMain` + // against it (a mismatched slot) must fail rather than run the bridge + // command under the wrong identity. + let mut check = check_with_fix(None); + check.main = Some(readout_with_update( + "npm install -g claude-agent-acp@latest", + FixType::UpdateBridge, + )); + assert!(resolve_update_command(&check, &FixType::UpdateMain).is_err()); + } + + #[test] + fn resolve_update_command_rejects_readout_without_actionable_command() { + // A readout with no derived update command (e.g. a self-updating or + // opaque install source) offers nothing to run, even for a valid slot. + let mut check = check_with_fix(None); + check.main = Some(doctor::types::AgentVersionInfo { + update_command: None, + update_fix_type: None, + ..Default::default() + }); + assert!(resolve_update_command(&check, &FixType::UpdateMain).is_err()); + } + #[test] fn install_fix_for_check_returns_the_two_install_recipes() { assert_eq!( @@ -1040,7 +1181,7 @@ mod tests { fn plan_with_requirements(verify_install: bool, bundled_bridge: bool) -> SetupPlan { SetupPlan { install_fix_type: None, - update_commands: Vec::new(), + update_fix_types: Vec::new(), verify_install, bundled_bridge, } diff --git a/src-tauri/src/commands/doctor.rs b/src-tauri/src/commands/doctor.rs index 2d854a9c6..8a0468a1f 100644 --- a/src-tauri/src/commands/doctor.rs +++ b/src-tauri/src/commands/doctor.rs @@ -1554,16 +1554,18 @@ pub async fn run_doctor_fresh( /// Run a fix command for a doctor check, identified by check ID and fix type. /// -/// `command_override` lets the frontend pass a verbatim shell command (used by -/// the per-readout Update affordances, whose source-aware commands aren't in -/// the crate's static lookup table); `None` falls back to the crate's -/// `lookup_fix_command`. The npm registry override is still applied either way. +/// The renderer sends only the typed `(check_id, fix_type)` identity; the exact +/// command (or native operation) is resolved here from trusted backend state — +/// the managed installer, the local check registry, or the crate's static +/// `lookup_fix_command`. There is no renderer-supplied command string: an +/// unknown check id or a check/fix-type combination with no registered fix is +/// rejected by the crate with an `Unknown check …` error rather than executing +/// arbitrary text. #[tauri::command] pub async fn run_doctor_fix( app_handle: AppHandle, check_id: String, fix_type: FixType, - command_override: Option, ) -> Result<(), String> { // The node-runtime fix is native — (re)install the pinned managed // runtime — not a shell command. @@ -1573,7 +1575,7 @@ pub async fn run_doctor_fix( // Managed bridge installs (claude, codex) go through the managed installer // so the floating `@latest` install lands in `packages/tools` with an // absolute-path shim, rather than the crate's `npm install -g`. - if command_override.is_none() && matches!(fix_type, FixType::Command | FixType::Bridge) { + if matches!(fix_type, FixType::Command | FixType::Bridge) { if let Some(provider_id) = managed_provider_for_check(&check_id) { let log_prefix = format!("[doctor fix {check_id}]"); return managed_acp_tools::install_managed_tool(&app_handle, provider_id, &|line| { @@ -1585,16 +1587,12 @@ pub async fn run_doctor_fix( } let captured_shell_env = dir_env::capture_home_interactive_env().await; let prepend_dirs = doctor_prepend_dirs(&app_handle); - if command_override.is_none() { - if let Some(fix) = find_local_fix(&LOCAL_DOCTOR_REGISTRY, &check_id, &fix_type) { - return execute_local_fix(fix.command, &captured_shell_env, &prepend_dirs).await; - } + if let Some(fix) = find_local_fix(&LOCAL_DOCTOR_REGISTRY, &check_id, &fix_type) { + return execute_local_fix(fix.command, &captured_shell_env, &prepend_dirs).await; } // npm-backed fixes run the managed npm into the private prefix, so the // managed runtime must exist before the command does. - let resolved_command = command_override - .clone() - .or_else(|| doctor::agents::lookup_fix_command(&check_id, &fix_type)); + let resolved_command = doctor::agents::lookup_fix_command(&check_id, &fix_type); if resolved_command .as_deref() .is_some_and(managed_acp_tools::is_npm_backed_command) @@ -1613,7 +1611,7 @@ pub async fn run_doctor_fix( check_id, fix_type, doctor::ExecuteFixOptions { - command_override, + command_override: None, npm_registry: crate::commands::agent_setup::npm_registry(&app_handle), env: None, } @@ -1909,6 +1907,41 @@ mod tests { assert!(text.find("== Tools ==").unwrap() < text.find("(git)").unwrap()); } + #[test] + fn run_doctor_fix_cannot_resolve_update_fix_types_without_an_override() { + // Regression for the renderer-command-override removal: `run_doctor_fix` + // no longer accepts a command string, so the only command source for an + // agent check is the crate's static table. Update fixes are derived + // per-readout and are intentionally absent from that table, so a forged + // `updateMain` / `updateBridge` request resolves to nothing and the + // executor rejects it rather than running arbitrary text. + for check_id in ["ai-agent-claude", "ai-agent-codex", "ai-agent-amp"] { + assert_eq!( + doctor::agents::lookup_fix_command(check_id, &FixType::UpdateMain), + None + ); + assert_eq!( + doctor::agents::lookup_fix_command(check_id, &FixType::UpdateBridge), + None + ); + } + } + + #[test] + fn run_doctor_fix_rejects_unknown_check_ids() { + // An unknown check id has no static fix and no local-registry fix, so + // there is nothing for the renderer to trigger — the executor path + // resolves `None` and fails closed. + assert_eq!( + doctor::agents::lookup_fix_command("totally-made-up-check", &FixType::Command), + None + ); + assert!( + find_local_fix(&LOCAL_DOCTOR_REGISTRY, "totally-made-up-check", &FixType::Command) + .is_none() + ); + } + #[test] fn converts_upstream_tools_category() { let check = DoctorCheck::from(upstream_check("git")); diff --git a/src/features/providers/api/agentSetup.ts b/src/features/providers/api/agentSetup.ts index dfedf74b2..fe895d9d5 100644 --- a/src/features/providers/api/agentSetup.ts +++ b/src/features/providers/api/agentSetup.ts @@ -34,11 +34,10 @@ export interface AgentSetupOperation { error: string | null; } -export interface AgentSetupUpdateCommand { - // `'updateMain'` or `'updateBridge'`, paired with the readout's command. - fixType: Extract; - command: string; -} +export type AgentSetupUpdateFixType = Extract< + FixType, + "updateMain" | "updateBridge" +>; // The execution recipe captured at click time. The card derives this from the // doctor report's actionable readouts, so the backend never has to re-derive @@ -47,7 +46,11 @@ export interface AgentSetupPlan { // The install recipe to seed the install loop with, or null for a pure // update / auth. installFixType: Extract | null; - updateCommands: AgentSetupUpdateCommand[]; + // Which per-readout updates to run after the install loop (`updateMain` / + // `updateBridge`). The card names only the readout slot; the backend resolves + // the exact source-aware command from the crate's trusted freshness readout, + // so no renderer-supplied shell command crosses the wire. + updateFixTypes: AgentSetupUpdateFixType[]; // Whether the backend probes PATH after the fix to confirm the agent landed. // `hasBinary && !isBuiltIn`: a built-in or binary-less provider has nothing to // resolve on disk, so the backend skips verification and takes a clean run as diff --git a/src/features/providers/stores/agentSetupStore.test.ts b/src/features/providers/stores/agentSetupStore.test.ts index 0b78a5bdc..c786fdab2 100644 --- a/src/features/providers/stores/agentSetupStore.test.ts +++ b/src/features/providers/stores/agentSetupStore.test.ts @@ -99,7 +99,7 @@ describe("useAgentSetupStore", () => { await useAgentSetupStore.getState().init(); await useAgentSetupStore.getState().startSetup("claude-acp", "install", { installFixType: "command", - updateCommands: [], + updateFixTypes: [], verifyInstall: true, }); diff --git a/src/features/settings/ui/AgentProviderCard.tsx b/src/features/settings/ui/AgentProviderCard.tsx index 4f768363d..8a2e92896 100644 --- a/src/features/settings/ui/AgentProviderCard.tsx +++ b/src/features/settings/ui/AgentProviderCard.tsx @@ -32,7 +32,7 @@ import { import { ArrowUpCircle } from "lucide-react"; import type { AgentSetupAction, - AgentSetupUpdateCommand, + AgentSetupUpdateFixType, } from "@/features/providers/api/agentSetup"; import { useAgentSetupStore } from "@/features/providers/stores/agentSetupStore"; import { @@ -213,15 +213,16 @@ export function AgentProviderCard({ const installFixType: Extract = versionCheck?.fixType === "bridge" ? "bridge" : "command"; - // Build the per-readout update commands the backend runs after the install - // loop. Readout *derivation* stays here (it already has the doctor report); - // only the resulting recipe crosses to Rust. - function buildUpdateCommands(): AgentSetupUpdateCommand[] { + // Build the per-readout update fix identities the backend runs after the + // install loop. Readout *derivation* stays here (it already has the doctor + // report) to decide *whether* an update is actionable; only the typed fix + // slot crosses to Rust, which re-resolves the exact command from the crate's + // trusted freshness readout. + function buildUpdateFixTypes(): AgentSetupUpdateFixType[] { return actionableReadouts.flatMap((readout) => - (readout.updateFixType === "updateMain" || - readout.updateFixType === "updateBridge") && - readout.updateCommand - ? [{ fixType: readout.updateFixType, command: readout.updateCommand }] + readout.updateFixType === "updateMain" || + readout.updateFixType === "updateBridge" + ? [readout.updateFixType] : [], ); } @@ -256,7 +257,7 @@ export function AgentProviderCard({ try { await startSetup(provider.id, "install", { installFixType, - updateCommands: buildUpdateCommands(), + updateFixTypes: buildUpdateFixTypes(), verifyInstall, ...(bundledBridge ? { bundledBridge } : {}), }); @@ -348,7 +349,7 @@ export function AgentProviderCard({ } void startSetup(provider.id, "update", { installFixType: null, - updateCommands: buildUpdateCommands(), + updateFixTypes: buildUpdateFixTypes(), verifyInstall, ...(bundledBridge ? { bundledBridge } : {}), }); @@ -362,7 +363,7 @@ export function AgentProviderCard({ } void startSetup(provider.id, "auth", { installFixType: null, - updateCommands: [], + updateFixTypes: [], verifyInstall, ...(bundledBridge ? { bundledBridge } : {}), }); diff --git a/src/features/settings/ui/DoctorCheckRow.tsx b/src/features/settings/ui/DoctorCheckRow.tsx index 0ba0ee893..cb80a0f31 100644 --- a/src/features/settings/ui/DoctorCheckRow.tsx +++ b/src/features/settings/ui/DoctorCheckRow.tsx @@ -53,7 +53,6 @@ export function DoctorCheckRow({ check, onFixed }: DoctorCheckRowProps) { const [activeFix, setActiveFix] = useState<{ fixType: FixType; command: string; - commandOverride: string | null; } | null>(null); const Icon = STATUS_ICON[check.status]; @@ -65,7 +64,6 @@ export function DoctorCheckRow({ check, onFixed }: DoctorCheckRowProps) { setActiveFix({ fixType: check.fixType, command: check.fixCommand, - commandOverride: null, }); setShowFixDialog(true); } @@ -75,11 +73,7 @@ export function DoctorCheckRow({ check, onFixed }: DoctorCheckRowProps) { setFixing(true); setFixError(null); try { - await runDoctorFix( - check.id, - activeFix.fixType, - activeFix.commandOverride ?? undefined, - ); + await runDoctorFix(check.id, activeFix.fixType); setShowFixDialog(false); onFixed?.(); } catch (e) { diff --git a/src/features/settings/ui/__tests__/AgentProviderCard.test.tsx b/src/features/settings/ui/__tests__/AgentProviderCard.test.tsx index 6e514ed59..616f9be82 100644 --- a/src/features/settings/ui/__tests__/AgentProviderCard.test.tsx +++ b/src/features/settings/ui/__tests__/AgentProviderCard.test.tsx @@ -409,7 +409,7 @@ describe("AgentProviderCard", () => { await waitFor(() => { expect(startAgentSetup).toHaveBeenCalledWith("claude-acp", "auth", { installFixType: null, - updateCommands: [], + updateFixTypes: [], verifyInstall: true, }); }); @@ -463,7 +463,7 @@ describe("AgentProviderCard", () => { await waitFor(() => { expect(startAgentSetup).toHaveBeenCalledWith("claude-acp", "install", { installFixType: "command", - updateCommands: [], + updateFixTypes: [], verifyInstall: true, }); }); @@ -494,7 +494,7 @@ describe("AgentProviderCard", () => { await waitFor(() => { expect(startAgentSetup).toHaveBeenCalledWith("claude-acp", "install", { installFixType: "command", - updateCommands: [], + updateFixTypes: [], verifyInstall: true, }); }); @@ -618,7 +618,7 @@ describe("AgentProviderCard", () => { await waitFor(() => { expect(startAgentSetup).toHaveBeenCalledWith("claude-acp", "install", { installFixType: "command", - updateCommands: [], + updateFixTypes: [], verifyInstall: true, }); }); @@ -680,12 +680,7 @@ describe("AgentProviderCard", () => { await waitFor(() => { expect(startAgentSetup).toHaveBeenCalledWith("claude-acp", "update", { installFixType: null, - updateCommands: [ - { - fixType: "updateMain", - command: "npm install -g @anthropic-ai/claude-code@latest", - }, - ], + updateFixTypes: ["updateMain"], verifyInstall: true, }); }); @@ -785,12 +780,7 @@ describe("AgentProviderCard", () => { await waitFor(() => { expect(startAgentSetup).toHaveBeenCalledWith("claude-acp", "update", { installFixType: null, - updateCommands: [ - { - fixType: "updateMain", - command: "npm install -g @anthropic-ai/claude-code@latest", - }, - ], + updateFixTypes: ["updateMain"], verifyInstall: true, }); }); @@ -809,7 +799,7 @@ describe("AgentProviderCard", () => { await waitFor(() => { expect(startAgentSetup).toHaveBeenCalledWith("claude-acp", "auth", { installFixType: null, - updateCommands: [], + updateFixTypes: [], verifyInstall: true, }); }); @@ -921,9 +911,7 @@ describe("AgentProviderCard", () => { await waitFor(() => { expect(startAgentSetup).toHaveBeenCalledWith("codex-acp", "install", { installFixType: "bridge", - updateCommands: [ - { fixType: "updateMain", command: "brew upgrade codex" }, - ], + updateFixTypes: ["updateMain"], verifyInstall: true, }); }); @@ -966,7 +954,7 @@ describe("AgentProviderCard", () => { await waitFor(() => { expect(startAgentSetup).toHaveBeenCalledWith("codex-acp", "install", { installFixType: "command", - updateCommands: [], + updateFixTypes: [], verifyInstall: true, // The backend's post-install verification mirrors the readiness gate: // a bundled-bridge provider must resolve its only binary under `path`, @@ -1062,7 +1050,7 @@ describe("AgentProviderCard", () => { await waitFor(() => { expect(startAgentSetup).toHaveBeenCalledWith("codex-acp", "install", { installFixType: "command", - updateCommands: [], + updateFixTypes: [], verifyInstall: true, }); }); @@ -1175,16 +1163,7 @@ describe("AgentProviderCard", () => { await waitFor(() => { expect(startAgentSetup).toHaveBeenCalledWith("claude-acp", "update", { installFixType: null, - updateCommands: [ - { - fixType: "updateMain", - command: "curl -fsSL https://example.com/install.sh | bash", - }, - { - fixType: "updateBridge", - command: "npm install -g claude-agent-acp@latest", - }, - ], + updateFixTypes: ["updateMain", "updateBridge"], verifyInstall: true, }); }); diff --git a/src/shared/api/__tests__/doctor.test.ts b/src/shared/api/__tests__/doctor.test.ts index 8663edd46..565031bec 100644 --- a/src/shared/api/__tests__/doctor.test.ts +++ b/src/shared/api/__tests__/doctor.test.ts @@ -35,6 +35,28 @@ describe("doctor API", () => { }); }); + it("never forwards a renderer-supplied command to run_doctor_fix", async () => { + // Regression for finding 7: the wire contract carries only the typed + // (checkId, fixType) identity. Even for an update fix — whose command used + // to ride along as `commandOverride` — no command string may cross to the + // backend, so a compromised renderer has no shell escape hatch. + mockedInvoke.mockResolvedValue(undefined); + + const { runDoctorFix } = await import("../doctor"); + await runDoctorFix("ai-agent-claude", "updateMain"); + + const payload = mockedInvoke.mock.calls.at(-1)?.[1] as Record< + string, + unknown + >; + expect(payload).toEqual({ + checkId: "ai-agent-claude", + fixType: "updateMain", + }); + expect(payload).not.toHaveProperty("commandOverride"); + expect(payload).not.toHaveProperty("command"); + }); + it("detects synthetic doctor timeout reports", async () => { const { isDoctorTimeoutReport } = await import("../useDoctorReport"); diff --git a/src/shared/api/doctor.ts b/src/shared/api/doctor.ts index d4406263f..3c69ea10e 100644 --- a/src/shared/api/doctor.ts +++ b/src/shared/api/doctor.ts @@ -95,7 +95,6 @@ export async function runDoctorFresh(): Promise { export async function runDoctorFix( checkId: string, fixType: FixType, - commandOverride?: string, ): Promise { - return invoke("run_doctor_fix", { checkId, fixType, commandOverride }); + return invoke("run_doctor_fix", { checkId, fixType }); } From 2c064627a971475d04bd062cb3e536ba395b5e28 Mon Sep 17 00:00:00 2001 From: peon <9ac6794b000690b7e814eb1805ad32405d0bec7d52838de3a86cf967565dacc0@buzz.block.builderlab.xyz> Date: Fri, 14 Aug 2026 17:52:22 -0700 Subject: [PATCH 2/5] fix(doctor): validate fix identity against trusted state before native and managed dispatch The typed-identity contract still let run_doctor_fix's native branches and the agent-setup install seed dispatch privileged operations on the shape of the request without checking the requested (check_id, fix_type) against the fix the check currently offers. (node-runtime, ) reinstalled Node even when passing; (ai-agent-claude, bridge) reached the managed installer though the registry offers only command; SetupPlan.install_fix_type was trusted verbatim. - doctor.rs: add ensure_offered_fix gate; resolve the check's current offered fix from trusted state (node_runtime_offered_fix / agent_setup's offered_install_fix) and reject mismatched or no-fix requests before the native reinstall or managed install. - agent_setup.rs: narrow the install-seed wire type to InstallFixType (command | bridge only) so auth/update variants cannot deserialize into the install slot, and authorize the seed against the provider's current doctor state in run_install before any install command runs. - Tests: command/dispatch-boundary regressions for forged, mismatched, unknown, and already-installed cases plus valid registered fixes; wire-type deserialization rejection for non-install variants. Co-authored-by: Kalvin Chau Signed-off-by: Kalvin Chau --- src-tauri/src/commands/agent_setup.rs | 160 +++++++++++++++++++++++++- src-tauri/src/commands/doctor.rs | 101 +++++++++++++++- 2 files changed, 256 insertions(+), 5 deletions(-) diff --git a/src-tauri/src/commands/agent_setup.rs b/src-tauri/src/commands/agent_setup.rs index 6b97a4883..18fedf6a2 100644 --- a/src-tauri/src/commands/agent_setup.rs +++ b/src-tauri/src/commands/agent_setup.rs @@ -115,6 +115,28 @@ impl SetupOperation { } } +/// The renderer-selectable install fix identity. Narrowed to the two *install* +/// slots so a forged `auth` / `updateMain` / `updateBridge` cannot deserialize +/// into the install seed at all; the backend still re-authorizes the value +/// against the provider's current doctor state before running it (see +/// [`authorize_install_seed`]). The TypeScript contract mirrors this, but the +/// narrow wire type — not the TS `Extract<>` — is the security boundary. +#[derive(Debug, Clone, PartialEq, Eq, serde::Deserialize)] +#[serde(rename_all = "camelCase")] +pub enum InstallFixType { + Command, + Bridge, +} + +impl From for FixType { + fn from(value: InstallFixType) -> Self { + match value { + InstallFixType::Command => FixType::Command, + InstallFixType::Bridge => FixType::Bridge, + } + } +} + /// The execution recipe captured at click time. Keeping readout *derivation* in /// TS (it already has the doctor report) avoids porting `actionableReadouts` /// into Rust; the backend just runs the recipe autonomously so the chain @@ -124,8 +146,11 @@ impl SetupOperation { pub struct SetupPlan { /// The install recipe to seed the install loop with (`command` for the main /// CLI, `bridge` for a missing ACP bridge). `null` for a pure update/auth. + /// Only the two install variants can deserialize here; the backend still + /// re-authorizes it against the provider's current doctor state before + /// running it (see [`authorize_install_seed`]). #[serde(default)] - install_fix_type: Option, + install_fix_type: Option, /// Per-readout update fix identities to run after the install loop. Only /// `updateMain` / `updateBridge` are valid; the exact source-aware command /// is resolved by the backend from the crate's trusted freshness readout, @@ -492,10 +517,47 @@ fn resolve_update_command( }) } +/// The install fix (`Command` / `Bridge`) the provider's check currently offers +/// from trusted crate state, or `None` when it offers no install fix (already +/// installed, or exposes only auth/update). Used by `run_doctor_fix` to reject a +/// forged or mismatched managed-install request before dispatching the +/// privileged managed installer. +pub(crate) async fn offered_install_fix( + app: &AppHandle, + provider_id: &str, +) -> Result, String> { + let check = find_check(app, provider_id).await?; + Ok(install_fix_for_check(&check)) +} + +/// Authorize a renderer-requested install seed against the provider's current +/// doctor state (`offered` = the install fix the check actually offers now). +/// The renderer names *which* install fix it intends; this returns the backend +/// value only when it matches, rejecting a mismatched seed or one against a +/// check that offers no install fix (already installed / auth-or-update only) +/// before any install shell command runs. Pure so the authorization boundary is +/// unit-testable without a Tauri `AppHandle`. +fn authorize_install_seed( + provider_id: &str, + requested: &InstallFixType, + offered: Option, +) -> Result { + let requested = FixType::from(requested.clone()); + match offered { + Some(offered) if offered == requested => Ok(offered), + Some(offered) => Err(format!( + "'{requested:?}' install is not the '{offered:?}' fix currently offered for '{provider_id}'" + )), + None => Err(format!( + "'{provider_id}' offers no '{requested:?}' install fix in its current state" + )), + } +} + /// The install recipe a check still needs, if any. Only the two *install* fix /// types qualify — `Auth` (installed-but-signed-out) and the per-readout update /// types are handled by later chain steps, not the install loop. -fn install_fix_for_check(check: &doctor::DoctorCheck) -> Option { +pub(crate) fn install_fix_for_check(check: &doctor::DoctorCheck) -> Option { match check.fix_type { Some(FixType::Command) => Some(FixType::Command), Some(FixType::Bridge) => Some(FixType::Bridge), @@ -601,7 +663,20 @@ async fn run_install( // after each install and run the next install fix the crate reports, so a // from-scratch Codex installs `codex` + `codex-acp` under one click. See // `next_install_fix` for the ≤2-pass bound that terminates a stuck install. - let mut pending = plan.install_fix_type.clone(); + // + // The renderer only names *which* install fix it intends; before running it + // we re-read the provider's doctor check and authorize the seed against that + // trusted state, so a forged/mismatched seed (or one against an + // already-installed check) rejects before the install shell command runs. + // Subsequent passes re-derive `pending` from the backend check directly, so + // only the renderer-supplied seed needs this gate. + let mut pending = match &plan.install_fix_type { + Some(requested) => { + let offered = offered_install_fix(app, provider_id).await?; + Some(authorize_install_seed(provider_id, requested, offered)?) + } + None => None, + }; let mut ran: Vec = Vec::new(); while let Some(fix) = next_install_fix(&pending, &ran) { ran.push(fix.clone()); @@ -1004,6 +1079,85 @@ mod tests { assert_eq!(install_fix_for_check(&check_with_fix(None)), None); } + #[test] + fn authorize_install_seed_returns_the_matching_backend_fix() { + // The renderer names the install fix it intends; when it matches the + // fix the check currently offers, the backend value is what runs. + assert_eq!( + authorize_install_seed( + "codex-acp", + &InstallFixType::Command, + Some(FixType::Command) + ), + Ok(FixType::Command) + ); + assert_eq!( + authorize_install_seed( + "codex-acp", + &InstallFixType::Bridge, + Some(FixType::Bridge) + ), + Ok(FixType::Bridge) + ); + } + + #[test] + fn authorize_install_seed_rejects_a_mismatched_seed() { + // A renderer that requests `bridge` when the check offers only `command` + // (or vice versa) must reject before any install shell command runs, + // rather than execute the provider's other install recipe on demand. + assert!( + authorize_install_seed("codex-acp", &InstallFixType::Bridge, Some(FixType::Command)) + .is_err() + ); + assert!( + authorize_install_seed("codex-acp", &InstallFixType::Command, Some(FixType::Bridge)) + .is_err() + ); + } + + #[test] + fn authorize_install_seed_rejects_when_no_install_fix_is_offered() { + // An already-installed provider (or one exposing only auth/update) + // offers no install fix, so a forged install seed must fail closed + // instead of re-running the install shell command. + assert!( + authorize_install_seed("codex-acp", &InstallFixType::Command, None).is_err() + ); + assert!( + authorize_install_seed("codex-acp", &InstallFixType::Bridge, None).is_err() + ); + } + + #[test] + fn install_fix_type_wire_rejects_non_install_variants() { + // The security boundary is the narrow Rust wire type: `auth` and the + // update slots must not deserialize into the install seed at all, so a + // forged plan can never smuggle a non-install identity through + // `installFixType` regardless of the TS `Extract<>` contract. + assert_eq!( + serde_json::from_str::("\"command\"").unwrap(), + InstallFixType::Command + ); + assert_eq!( + serde_json::from_str::("\"bridge\"").unwrap(), + InstallFixType::Bridge + ); + for forged in ["\"auth\"", "\"updateMain\"", "\"updateBridge\""] { + assert!( + serde_json::from_str::(forged).is_err(), + "{forged} must not deserialize into an install seed" + ); + } + // And it must not deserialize as the SetupPlan field either. + assert!( + serde_json::from_str::( + "{\"installFixType\":\"auth\",\"updateFixTypes\":[],\"verifyInstall\":false}" + ) + .is_err() + ); + } + /// Test model of the install loop in [`run_install`]: both share /// [`next_install_fix`] as their decision core, so this covers the loop's /// state transitions without the real (async, system-touching) doctor crate. diff --git a/src-tauri/src/commands/doctor.rs b/src-tauri/src/commands/doctor.rs index 8a0468a1f..f71e83e14 100644 --- a/src-tauri/src/commands/doctor.rs +++ b/src-tauri/src/commands/doctor.rs @@ -1568,15 +1568,30 @@ pub async fn run_doctor_fix( fix_type: FixType, ) -> Result<(), String> { // The node-runtime fix is native — (re)install the pinned managed - // runtime — not a shell command. + // runtime — not a shell command. Validate the requested identity against + // the fix the check currently offers before the privileged reinstall: a + // passing runtime offers no fix, and the only fix it ever offers is + // `Command`, so a forged or mismatched `(node-runtime, )` pair must + // fail closed rather than trigger a networked native download. if check_id == NODE_RUNTIME_CHECK.id { + ensure_offered_fix(&check_id, &fix_type, node_runtime_offered_fix(&app_handle).await)?; return ensure_managed_node_runtime_logged(&app_handle).await; } // Managed bridge installs (claude, codex) go through the managed installer // so the floating `@latest` install lands in `packages/tools` with an - // absolute-path shim, rather than the crate's `npm install -g`. + // absolute-path shim, rather than the crate's `npm install -g`. Only the + // install fix types route here, and only after confirming the check + // currently offers that exact install fix — the pinned registry offers + // `Command` for these managed checks and no `Bridge`, so a mismatched + // `(ai-agent-claude, bridge)` pair (or a request against an already-healthy + // check) rejects before the networked native install. if matches!(fix_type, FixType::Command | FixType::Bridge) { if let Some(provider_id) = managed_provider_for_check(&check_id) { + ensure_offered_fix( + &check_id, + &fix_type, + crate::commands::agent_setup::offered_install_fix(&app_handle, provider_id).await?, + )?; let log_prefix = format!("[doctor fix {check_id}]"); return managed_acp_tools::install_managed_tool(&app_handle, provider_id, &|line| { log::info!("{log_prefix} {line}"); @@ -1620,6 +1635,45 @@ pub async fn run_doctor_fix( .await } +/// Reject a fix request whose typed identity doesn't match the fix the check +/// currently offers from trusted backend state. `offered` is the check's +/// current fix type (`None` when it offers no fix — e.g. already healthy); the +/// request is authorized only when the requested `fix_type` equals it. This is +/// the backend-owned check/fix-combination gate that keeps a compromised +/// renderer from driving a native or managed operation outside the check's +/// registered, currently-actionable identity. +fn ensure_offered_fix( + check_id: &str, + requested: &FixType, + offered: Option, +) -> Result<(), String> { + match offered { + Some(ref offered) if offered == requested => Ok(()), + Some(offered) => Err(format!( + "'{requested:?}' does not match the '{offered:?}' fix currently offered for '{check_id}'" + )), + None => Err(format!( + "'{check_id}' offers no '{requested:?}' fix in its current state" + )), + } +} + +/// The fix the managed Node runtime check currently offers, resolved from the +/// same trusted state the report is built from: `Some(Command)` when the +/// runtime is missing/broken (the native reinstall), `None` when it is healthy +/// or unreported. Mirrors `build_managed_node_runtime_check`'s `offers_fix` +/// decision so the fix gate can't disagree with the report the renderer saw. +async fn node_runtime_offered_fix(app_handle: &AppHandle) -> Option { + let paths = ManagedRuntimePaths::resolve(app_handle); + let check = run_node_runtime_check( + paths.node_root, + paths.npm_prefix_bin_dir, + paths.shim_bin_dir, + ) + .await?; + check.fix_type +} + /// The provider id of a managed bridge, when this crate check id maps to one /// on this build/target — `ai-agent-claude` → `claude-acp`, unless the dev /// override or the disable feature has emptied the managed set. @@ -1907,6 +1961,49 @@ mod tests { assert!(text.find("== Tools ==").unwrap() < text.find("(git)").unwrap()); } + #[test] + fn ensure_offered_fix_authorizes_only_the_currently_offered_fix() { + // The dispatch gate the node-runtime and managed-install branches call + // before any native/managed side effect. A request is authorized only + // when it equals the fix the check currently offers from trusted state. + assert!(ensure_offered_fix("node-runtime", &FixType::Command, Some(FixType::Command)).is_ok()); + assert!( + ensure_offered_fix("ai-agent-claude", &FixType::Command, Some(FixType::Command)).is_ok() + ); + } + + #[test] + fn ensure_offered_fix_rejects_a_mismatched_fix_identity() { + // Concrete mismatch from the review: the managed Claude check offers + // `Command`, so `(ai-agent-claude, Bridge)` must reject before the + // networked native install, not silently install. + assert!( + ensure_offered_fix("ai-agent-claude", &FixType::Bridge, Some(FixType::Command)).is_err() + ); + // Any non-Command identity against node-runtime is a forged pair. + for forged in [ + FixType::Bridge, + FixType::Auth, + FixType::UpdateMain, + FixType::UpdateBridge, + ] { + assert!( + ensure_offered_fix("node-runtime", &forged, Some(FixType::Command)).is_err(), + "expected {forged:?} against a Command-only check to reject" + ); + } + } + + #[test] + fn ensure_offered_fix_rejects_when_the_check_offers_no_fix() { + // A passing runtime (or an already-installed managed agent) offers no + // fix, so every request must fail closed rather than trigger a + // privileged reinstall. + assert!(ensure_offered_fix("node-runtime", &FixType::Command, None).is_err()); + assert!(ensure_offered_fix("ai-agent-claude", &FixType::Command, None).is_err()); + assert!(ensure_offered_fix("ai-agent-claude", &FixType::Bridge, None).is_err()); + } + #[test] fn run_doctor_fix_cannot_resolve_update_fix_types_without_an_override() { // Regression for the renderer-command-override removal: `run_doctor_fix` From 16644c5832f7eabf3ba12837b86d175218585141 Mon Sep 17 00:00:00 2001 From: peon <9ac6794b000690b7e814eb1805ad32405d0bec7d52838de3a86cf967565dacc0@buzz.block.builderlab.xyz> Date: Fri, 14 Aug 2026 18:56:52 -0700 Subject: [PATCH 3/5] fix(doctor): authorize every doctor fix dispatch and run_auth against current offered state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prior gate covered the native reinstall and managed-install seeds, but two dispatch paths still ran on the shape of the request alone: the crate static- command branch (an (ai-agent-*, Auth) request reached ` login` even when the check offered Command or nothing) and run_auth (SetupAction::Auth ran the sign-in command without re-reading the provider's current fix). - doctor.rs: front run_doctor_fix with one pure planner, plan_doctor_fix, that calls ensure_offered_fix first (universal gate) then selects a DoctorFixDispatch target. offered_fix_for_check resolves the currently-offered fix per family (node-runtime native state; ai-agent-* crate report; else None). Every dispatch path — including the previously-unguarded CrateCommand branch and the forged-Auth case — is authorized before any side effect. - agent_setup.rs: extract run_crate_check_report; add offered_crate_check_fix (crate check's currently-offered fix) and pure authorize_auth; gate run_auth to re-read the provider check and authorize Auth before the auth command. Narrow offered_install_fix to private (now internal to run_install). - Tests: 5 planner dispatch/authorization-boundary regressions (forged Auth rejects before the static command; static command rejects while passing / on Auth mismatch; valid offered fixes route to the exact target; managed-install routing + Bridge mismatch reject; forged node-runtime pairs reject) and 2 authorize_auth regressions (forged/none reject; valid Auth allowed). Co-authored-by: Kalvin Chau Signed-off-by: Kalvin Chau --- src-tauri/src/commands/agent_setup.rs | 141 ++++++++++--- src-tauri/src/commands/doctor.rs | 284 ++++++++++++++++++++------ 2 files changed, 326 insertions(+), 99 deletions(-) diff --git a/src-tauri/src/commands/agent_setup.rs b/src-tauri/src/commands/agent_setup.rs index 18fedf6a2..9cf446ae1 100644 --- a/src-tauri/src/commands/agent_setup.rs +++ b/src-tauri/src/commands/agent_setup.rs @@ -382,6 +382,22 @@ async fn find_check_with_options( check_freshness: bool, ) -> Result { let target = crate_check_id(provider_id); + run_crate_check_report(app, check_freshness) + .await + .into_iter() + .find(|check| check.id == target) + .ok_or_else(|| format!("Unknown agent provider '{provider_id}'")) +} + +/// The crate's AI-agent doctor report (with the Windows managed-bridge repair +/// applied), built with the same env/registry/bundled-tools view the settings +/// screen reads. `check_freshness` mirrors [`find_check`] vs [`find_check_fresh`]: +/// the cheap path skips version/registry probing. Shared by the provider check +/// lookups and the offered-fix resolver so both authorize against one report. +async fn run_crate_check_report( + app: &AppHandle, + check_freshness: bool, +) -> Vec { let env_vars = setup_env_vars(app).await; let bundled_tools_dir = managed_acp_tools::bundled_tools_dir_for_checks(app); let mut report = doctor::run_checks_with_options( @@ -408,11 +424,25 @@ async fn find_check_with_options( ) .await; } - report - .checks + report.checks +} + +/// The top-level fix a crate AI-agent check (`ai-agent-*`) currently offers from +/// trusted state, resolved from the same non-fresh crate report the renderer +/// reads: `Some(Command | Bridge | Auth)` when the check currently offers that +/// fix, `None` when it offers none (already installed and authenticated) or the +/// check id isn't present. `run_doctor_fix` authorizes every agent fix request +/// against this before dispatch, so a forged or stale `(check_id, fix_type)` +/// pair fails closed rather than reaching a shell/native side effect. +pub(crate) async fn offered_crate_check_fix( + app: &AppHandle, + check_id: &str, +) -> Result, String> { + Ok(run_crate_check_report(app, false) + .await .into_iter() - .find(|check| check.id == target) - .ok_or_else(|| format!("Unknown agent provider '{provider_id}'")) + .find(|check| check.id == check_id) + .and_then(|check| check.fix_type)) } /// Whether the agent's main CLI or ACP bridge resolved on disk. Used as the @@ -517,12 +547,31 @@ fn resolve_update_command( }) } +/// Authorize a renderer-requested `Auth` action against the provider's current +/// doctor state (`offered` = the fix the check actually offers now). Sign-in is +/// only authorized when the check currently offers `Auth` (installed but not +/// authenticated); a request against a check that offers a different fix +/// (missing install → `Command`/`Bridge`) or no fix at all (already +/// authenticated, or auth status unknown) fails closed before the auth shell +/// command runs. Pure so the boundary is unit-testable without a Tauri handle. +fn authorize_auth(provider_id: &str, offered: Option) -> Result<(), String> { + match offered { + Some(FixType::Auth) => Ok(()), + Some(offered) => Err(format!( + "'{provider_id}' currently offers the '{offered:?}' fix, not sign-in" + )), + None => Err(format!( + "'{provider_id}' offers no sign-in in its current state" + )), + } +} + /// The install fix (`Command` / `Bridge`) the provider's check currently offers /// from trusted crate state, or `None` when it offers no install fix (already -/// installed, or exposes only auth/update). Used by `run_doctor_fix` to reject a -/// forged or mismatched managed-install request before dispatching the -/// privileged managed installer. -pub(crate) async fn offered_install_fix( +/// installed, or exposes only auth/update). Used by [`run_install`] to reject a +/// forged or mismatched managed-install seed before the install loop runs any +/// command. +async fn offered_install_fix( app: &AppHandle, provider_id: &str, ) -> Result, String> { @@ -718,6 +767,15 @@ async fn run_auth( provider_id: &str, plan: &SetupPlan, ) -> Result<(), String> { + // The renderer only names the *action*; before running the auth shell + // command we re-read the provider's doctor check and authorize `Auth` + // against the fix it currently offers. A forged/stale sign-in for a + // provider that is missing (offers `Command`/`Bridge`), already + // authenticated, or has an unknown auth state (offers no fix) rejects here + // rather than executing the static ` login` command on demand. + // `find_check` also fails closed on an unknown provider. + let check = find_check(app, provider_id).await?; + authorize_auth(provider_id, check.fix_type)?; set_phase(app, registry, provider_id, SetupPhase::Authenticating); run_fix(app, registry, provider_id, FixType::Auth, None).await?; @@ -1003,7 +1061,10 @@ mod tests { // A forged plan naming an install/auth fix as an "update" must never // resolve to a command — those are not update slots. let mut check = check_with_fix(None); - check.main = Some(readout_with_update("brew upgrade codex", FixType::UpdateMain)); + check.main = Some(readout_with_update( + "brew upgrade codex", + FixType::UpdateMain, + )); for forged in [FixType::Command, FixType::Bridge, FixType::Auth] { assert!( @@ -1092,11 +1153,7 @@ mod tests { Ok(FixType::Command) ); assert_eq!( - authorize_install_seed( - "codex-acp", - &InstallFixType::Bridge, - Some(FixType::Bridge) - ), + authorize_install_seed("codex-acp", &InstallFixType::Bridge, Some(FixType::Bridge)), Ok(FixType::Bridge) ); } @@ -1106,14 +1163,18 @@ mod tests { // A renderer that requests `bridge` when the check offers only `command` // (or vice versa) must reject before any install shell command runs, // rather than execute the provider's other install recipe on demand. - assert!( - authorize_install_seed("codex-acp", &InstallFixType::Bridge, Some(FixType::Command)) - .is_err() - ); - assert!( - authorize_install_seed("codex-acp", &InstallFixType::Command, Some(FixType::Bridge)) - .is_err() - ); + assert!(authorize_install_seed( + "codex-acp", + &InstallFixType::Bridge, + Some(FixType::Command) + ) + .is_err()); + assert!(authorize_install_seed( + "codex-acp", + &InstallFixType::Command, + Some(FixType::Bridge) + ) + .is_err()); } #[test] @@ -1121,12 +1182,26 @@ mod tests { // An already-installed provider (or one exposing only auth/update) // offers no install fix, so a forged install seed must fail closed // instead of re-running the install shell command. - assert!( - authorize_install_seed("codex-acp", &InstallFixType::Command, None).is_err() - ); - assert!( - authorize_install_seed("codex-acp", &InstallFixType::Bridge, None).is_err() - ); + assert!(authorize_install_seed("codex-acp", &InstallFixType::Command, None).is_err()); + assert!(authorize_install_seed("codex-acp", &InstallFixType::Bridge, None).is_err()); + } + + #[test] + fn authorize_auth_rejects_a_forged_sign_in_against_a_non_auth_state() { + // Regression for the auth bypass: `run_auth` re-reads the provider check + // and authorizes `Auth` against its current offered fix. A provider that + // is missing (offers `Command`/`Bridge`) or already authenticated + // (offers no fix) must reject before the ` login` command runs. + assert!(authorize_auth("copilot-acp", Some(FixType::Command)).is_err()); + assert!(authorize_auth("amp-acp", Some(FixType::Bridge)).is_err()); + assert!(authorize_auth("copilot-acp", None).is_err()); + } + + #[test] + fn authorize_auth_allows_a_currently_offered_sign_in() { + // Installed-but-signed-out is exactly the state that offers `Auth`, so a + // legitimate sign-in is authorized and reaches the auth command. + assert!(authorize_auth("copilot-acp", Some(FixType::Auth)).is_ok()); } #[test] @@ -1150,12 +1225,10 @@ mod tests { ); } // And it must not deserialize as the SetupPlan field either. - assert!( - serde_json::from_str::( - "{\"installFixType\":\"auth\",\"updateFixTypes\":[],\"verifyInstall\":false}" - ) - .is_err() - ); + assert!(serde_json::from_str::( + "{\"installFixType\":\"auth\",\"updateFixTypes\":[],\"verifyInstall\":false}" + ) + .is_err()); } /// Test model of the install loop in [`run_install`]: both share diff --git a/src-tauri/src/commands/doctor.rs b/src-tauri/src/commands/doctor.rs index f71e83e14..71f7be0b0 100644 --- a/src-tauri/src/commands/doctor.rs +++ b/src-tauri/src/commands/doctor.rs @@ -1567,72 +1567,138 @@ pub async fn run_doctor_fix( check_id: String, fix_type: FixType, ) -> Result<(), String> { - // The node-runtime fix is native — (re)install the pinned managed - // runtime — not a shell command. Validate the requested identity against - // the fix the check currently offers before the privileged reinstall: a - // passing runtime offers no fix, and the only fix it ever offers is - // `Command`, so a forged or mismatched `(node-runtime, )` pair must - // fail closed rather than trigger a networked native download. - if check_id == NODE_RUNTIME_CHECK.id { - ensure_offered_fix(&check_id, &fix_type, node_runtime_offered_fix(&app_handle).await)?; - return ensure_managed_node_runtime_logged(&app_handle).await; - } - // Managed bridge installs (claude, codex) go through the managed installer - // so the floating `@latest` install lands in `packages/tools` with an - // absolute-path shim, rather than the crate's `npm install -g`. Only the - // install fix types route here, and only after confirming the check - // currently offers that exact install fix — the pinned registry offers - // `Command` for these managed checks and no `Bridge`, so a mismatched - // `(ai-agent-claude, bridge)` pair (or a request against an already-healthy - // check) rejects before the networked native install. - if matches!(fix_type, FixType::Command | FixType::Bridge) { - if let Some(provider_id) = managed_provider_for_check(&check_id) { - ensure_offered_fix( - &check_id, - &fix_type, - crate::commands::agent_setup::offered_install_fix(&app_handle, provider_id).await?, - )?; + // Resolve the fix the check *currently offers* from trusted state, then let + // the pure planner authorize the request and pick the dispatch target in + // one step. A forged, stale, or mismatched `(check_id, fix_type)` pair (an + // install fix against a healthy check, an `Auth` fix against a check that + // offers `Command`, an unknown check id) yields an `Err` and no dispatch + // target, so it can never reach a shell/native side effect. + let offered = offered_fix_for_check(&app_handle, &check_id).await?; + match plan_doctor_fix(&check_id, &fix_type, offered)? { + // The node-runtime fix is native — (re)install the pinned managed + // runtime — not a shell command. + DoctorFixDispatch::NodeRuntime => ensure_managed_node_runtime_logged(&app_handle).await, + // Managed bridge installs (claude, codex) go through the managed + // installer so the floating `@latest` install lands in + // `packages/tools` with an absolute-path shim, rather than the crate's + // `npm install -g`. + DoctorFixDispatch::ManagedInstall(provider_id) => { let log_prefix = format!("[doctor fix {check_id}]"); - return managed_acp_tools::install_managed_tool(&app_handle, provider_id, &|line| { + managed_acp_tools::install_managed_tool(&app_handle, provider_id, &|line| { log::info!("{log_prefix} {line}"); }) .await - .map_err(|error| error.to_string()); + .map_err(|error| error.to_string()) + } + DoctorFixDispatch::LocalCommand(command) => { + let captured_shell_env = dir_env::capture_home_interactive_env().await; + let prepend_dirs = doctor_prepend_dirs(&app_handle); + execute_local_fix(command, &captured_shell_env, &prepend_dirs).await + } + DoctorFixDispatch::CrateCommand => { + let captured_shell_env = dir_env::capture_home_interactive_env().await; + let prepend_dirs = doctor_prepend_dirs(&app_handle); + // npm-backed fixes run the managed npm into the private prefix, so + // the managed runtime must exist before the command does. + let resolved_command = doctor::agents::lookup_fix_command(&check_id, &fix_type); + if resolved_command + .as_deref() + .is_some_and(managed_acp_tools::is_npm_backed_command) + { + ensure_managed_node_runtime_logged(&app_handle).await?; + } + let mut env_vars = path_env::env_vars_with_extended_path_and_prepended_dirs( + &captured_shell_env, + &prepend_dirs, + ); + managed_acp_tools::apply_managed_npm_env( + &mut env_vars, + &managed_acp_tools::managed_npm_env(&app_handle), + ); + doctor::execute_fix_with_env_options( + check_id, + fix_type, + doctor::ExecuteFixOptions { + command_override: None, + npm_registry: crate::commands::agent_setup::npm_registry(&app_handle), + env: None, + } + .with_env_snapshot(env_vars), + ) + .await } } - let captured_shell_env = dir_env::capture_home_interactive_env().await; - let prepend_dirs = doctor_prepend_dirs(&app_handle); - if let Some(fix) = find_local_fix(&LOCAL_DOCTOR_REGISTRY, &check_id, &fix_type) { - return execute_local_fix(fix.command, &captured_shell_env, &prepend_dirs).await; - } - // npm-backed fixes run the managed npm into the private prefix, so the - // managed runtime must exist before the command does. - let resolved_command = doctor::agents::lookup_fix_command(&check_id, &fix_type); - if resolved_command - .as_deref() - .is_some_and(managed_acp_tools::is_npm_backed_command) - { - ensure_managed_node_runtime_logged(&app_handle).await?; +} + +/// Where an authorized `run_doctor_fix` request dispatches. Selecting the target +/// is pure (it reads only static registries), so [`plan_doctor_fix`] can decide +/// it — and reject an unauthorized request before any target is chosen — under +/// unit test without a Tauri runtime. +#[derive(Debug, PartialEq, Eq)] +enum DoctorFixDispatch { + /// Native (re)install of the managed Node.js runtime. + NodeRuntime, + /// Managed bridge install through the managed installer (provider id). + ManagedInstall(&'static str), + /// A local-registry fix command run through `execute_local_fix`. + LocalCommand(&'static str), + /// A crate AI-agent static command run through `execute_fix_with_env_options`. + CrateCommand, +} + +/// Authorize a `run_doctor_fix` request against the fix the check currently +/// offers, then select its dispatch target — the single backend-owned check/fix +/// gate for every dispatch path. `offered` is the check's currently-offered fix +/// from trusted state (`None` when it offers none, e.g. a healthy check). The +/// request is rejected (and no target is returned) unless it matches, so a +/// forged/stale/mismatched pair can never reach a shell/native side effect. The +/// selection reads only static registries, so it is pure and testable. +fn plan_doctor_fix( + check_id: &str, + fix_type: &FixType, + offered: Option, +) -> Result { + ensure_offered_fix(check_id, fix_type, offered)?; + + if check_id == NODE_RUNTIME_CHECK.id { + return Ok(DoctorFixDispatch::NodeRuntime); } - let mut env_vars = path_env::env_vars_with_extended_path_and_prepended_dirs( - &captured_shell_env, - &prepend_dirs, - ); - managed_acp_tools::apply_managed_npm_env( - &mut env_vars, - &managed_acp_tools::managed_npm_env(&app_handle), - ); - doctor::execute_fix_with_env_options( - check_id, - fix_type, - doctor::ExecuteFixOptions { - command_override: None, - npm_registry: crate::commands::agent_setup::npm_registry(&app_handle), - env: None, + if matches!(fix_type, FixType::Command | FixType::Bridge) { + if let Some(provider_id) = managed_provider_for_check(check_id) { + return Ok(DoctorFixDispatch::ManagedInstall(provider_id)); } - .with_env_snapshot(env_vars), - ) - .await + } + if let Some(fix) = find_local_fix(&LOCAL_DOCTOR_REGISTRY, check_id, fix_type) { + return Ok(DoctorFixDispatch::LocalCommand(fix.command)); + } + Ok(DoctorFixDispatch::CrateCommand) +} + +/// The fix a doctor check currently offers from trusted backend state, resolved +/// per check family so the [`ensure_offered_fix`] gate authorizes every +/// `run_doctor_fix` dispatch against the same current state the renderer's +/// report reflects: +/// +/// - `node-runtime`: the native runtime check (`Some(Command)` when +/// missing/broken, `None` when healthy). +/// - `ai-agent-*`: the crate check's currently-offered top-level fix +/// (`Command`/`Bridge` when missing, `Auth` when installed-but-signed-out, +/// `None` when healthy), resolved from the crate report — this covers both +/// managed bridges and the static-command agents. +/// - anything else: the local registry defines no runtime fixes, so this +/// resolves to `None` and the gate rejects. (Add a resolver here if a local +/// check ever registers a runnable fix.) +async fn offered_fix_for_check( + app_handle: &AppHandle, + check_id: &str, +) -> Result, String> { + if check_id == NODE_RUNTIME_CHECK.id { + return Ok(node_runtime_offered_fix(app_handle).await); + } + if check_id.starts_with("ai-agent-") { + return crate::commands::agent_setup::offered_crate_check_fix(app_handle, check_id).await; + } + Ok(None) } /// Reject a fix request whose typed identity doesn't match the fix the check @@ -1966,9 +2032,12 @@ mod tests { // The dispatch gate the node-runtime and managed-install branches call // before any native/managed side effect. A request is authorized only // when it equals the fix the check currently offers from trusted state. - assert!(ensure_offered_fix("node-runtime", &FixType::Command, Some(FixType::Command)).is_ok()); assert!( - ensure_offered_fix("ai-agent-claude", &FixType::Command, Some(FixType::Command)).is_ok() + ensure_offered_fix("node-runtime", &FixType::Command, Some(FixType::Command)).is_ok() + ); + assert!( + ensure_offered_fix("ai-agent-claude", &FixType::Command, Some(FixType::Command)) + .is_ok() ); } @@ -1978,7 +2047,8 @@ mod tests { // `Command`, so `(ai-agent-claude, Bridge)` must reject before the // networked native install, not silently install. assert!( - ensure_offered_fix("ai-agent-claude", &FixType::Bridge, Some(FixType::Command)).is_err() + ensure_offered_fix("ai-agent-claude", &FixType::Bridge, Some(FixType::Command)) + .is_err() ); // Any non-Command identity against node-runtime is a forged pair. for forged in [ @@ -2004,6 +2074,88 @@ mod tests { assert!(ensure_offered_fix("ai-agent-claude", &FixType::Bridge, None).is_err()); } + #[test] + fn plan_doctor_fix_forged_auth_rejects_before_selecting_the_static_command() { + // Regression for the auth/command bypass: a forged `(ai-agent-claude, + // Auth)` request when the check currently offers `Command` (missing + // install) must return an `Err` from the planner — no dispatch target — + // so `run_doctor_fix` never reaches the `CrateCommand` branch that would + // resolve and run `claude-agent-acp --cli auth login`. + assert!( + plan_doctor_fix("ai-agent-claude", &FixType::Auth, Some(FixType::Command)).is_err() + ); + // An `Auth` request against a healthy (already-authenticated) check that + // offers nothing also rejects before target selection. + assert!(plan_doctor_fix("ai-agent-claude", &FixType::Auth, None).is_err()); + } + + #[test] + fn plan_doctor_fix_static_command_rejects_while_the_check_is_passing() { + // A registered static/local `Command` fix must not be callable while the + // check is passing: a healthy check offers no fix, so the planner rejects + // before returning any dispatch target. + assert!(plan_doctor_fix("ai-agent-copilot", &FixType::Command, None).is_err()); + // A `Command` request against a check that currently offers `Auth` + // (installed but signed out) is a mismatch and also rejects. + assert!( + plan_doctor_fix("ai-agent-copilot", &FixType::Command, Some(FixType::Auth)).is_err() + ); + } + + #[test] + fn plan_doctor_fix_authorizes_and_routes_the_currently_offered_fix() { + // The valid currently-offered paths still resolve to their exact + // dispatch target, so legitimate fixes keep working after the gate. + assert_eq!( + plan_doctor_fix("node-runtime", &FixType::Command, Some(FixType::Command)).unwrap(), + DoctorFixDispatch::NodeRuntime + ); + // A currently-offered `Auth` on a static-command agent routes to the + // crate command executor (which then resolves ` login`). + assert_eq!( + plan_doctor_fix("ai-agent-copilot", &FixType::Auth, Some(FixType::Auth)).unwrap(), + DoctorFixDispatch::CrateCommand + ); + } + + #[test] + fn plan_doctor_fix_routes_a_currently_offered_managed_install() { + // A managed bridge whose check currently offers `Command` routes to the + // managed installer; a mismatched `Bridge` request (the registry offers + // `Command`, not `Bridge`) rejects before any target is chosen. Guarded + // on the managed set being present on this build/target. + if let Some(provider_id) = managed_provider_for_check("ai-agent-claude") { + assert_eq!( + plan_doctor_fix("ai-agent-claude", &FixType::Command, Some(FixType::Command)) + .unwrap(), + DoctorFixDispatch::ManagedInstall(provider_id) + ); + assert!( + plan_doctor_fix("ai-agent-claude", &FixType::Bridge, Some(FixType::Command)) + .is_err() + ); + } + } + + #[test] + fn plan_doctor_fix_rejects_forged_node_runtime_pairs() { + // Any non-`Command` identity against node-runtime is a forged pair, and + // a request against a healthy runtime (offers nothing) fails closed — + // neither ever reaches the native reinstall target. + for forged in [ + FixType::Bridge, + FixType::Auth, + FixType::UpdateMain, + FixType::UpdateBridge, + ] { + assert!( + plan_doctor_fix("node-runtime", &forged, Some(FixType::Command)).is_err(), + "expected {forged:?} against a Command-only runtime to reject" + ); + } + assert!(plan_doctor_fix("node-runtime", &FixType::Command, None).is_err()); + } + #[test] fn run_doctor_fix_cannot_resolve_update_fix_types_without_an_override() { // Regression for the renderer-command-override removal: `run_doctor_fix` @@ -2033,10 +2185,12 @@ mod tests { doctor::agents::lookup_fix_command("totally-made-up-check", &FixType::Command), None ); - assert!( - find_local_fix(&LOCAL_DOCTOR_REGISTRY, "totally-made-up-check", &FixType::Command) - .is_none() - ); + assert!(find_local_fix( + &LOCAL_DOCTOR_REGISTRY, + "totally-made-up-check", + &FixType::Command + ) + .is_none()); } #[test] From da6333ae312dd61b62ba3b0e284f95565f4a6f5f Mon Sep 17 00:00:00 2001 From: peon <9ac6794b000690b7e814eb1805ad32405d0bec7d52838de3a86cf967565dacc0@buzz.block.builderlab.xyz> Date: Fri, 14 Aug 2026 19:14:21 -0700 Subject: [PATCH 4/5] fix(doctor): bound update dispatch and gate doctor fix on runtime policy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two backend-authorization gaps remained after the offered-state gate: - agent_setup.rs: SetupPlan.update_fix_types deserialized an unbounded Vec and run_install executed every entry against one freshness snapshot, so a compromised renderer could submit updateMain N times to rerun the trusted update command N times through one IPC request. Narrow the wire type to UpdateFixType (updateMain | updateBridge only) and add pure authorize_update_fixes, which rejects a duplicate slot before any executor target is produced. The card only ever names each slot once. - doctor.rs: run_doctor_fix never loaded runtime config, so it bypassed the doctor.enabled policy that run_doctor/run_doctor_fresh enforce — a renderer could invoke the command directly while Doctor is disabled and drive a native/managed/local/crate fix. Load RuntimeConfigState via ready_config and reject when doctor_enabled is false, before offered-state resolution or any side effect. - Tests: agent_setup gains duplicate-slot rejection and at-most-one-per-slot regressions; doctor gains a policy-gate regression pinning that disabled config selects no dispatch while default/explicit-enabled keep fixes runnable. Co-authored-by: Kalvin Chau Signed-off-by: Kalvin Chau --- src-tauri/src/commands/agent_setup.rs | 108 ++++++++++++++++++++++++-- src-tauri/src/commands/doctor.rs | 40 ++++++++++ 2 files changed, 142 insertions(+), 6 deletions(-) diff --git a/src-tauri/src/commands/agent_setup.rs b/src-tauri/src/commands/agent_setup.rs index 9cf446ae1..f7feb00c7 100644 --- a/src-tauri/src/commands/agent_setup.rs +++ b/src-tauri/src/commands/agent_setup.rs @@ -137,6 +137,29 @@ impl From for FixType { } } +/// The narrow wire type for a per-readout update slot. Only the two update +/// families deserialize here; a forged `command`/`bridge`/`auth` fix can't cross +/// the wire in the update list, and — combined with [`authorize_update_fixes`]'s +/// duplicate rejection — a compromised renderer can't submit the same slot N +/// times to rerun the trusted update command N times off one freshness +/// snapshot. Mirrors [`InstallFixType`]: the narrow wire type, not the TS +/// `Extract<>`, is the security boundary. +#[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Deserialize)] +#[serde(rename_all = "camelCase")] +pub enum UpdateFixType { + UpdateMain, + UpdateBridge, +} + +impl From for FixType { + fn from(value: UpdateFixType) -> Self { + match value { + UpdateFixType::UpdateMain => FixType::UpdateMain, + UpdateFixType::UpdateBridge => FixType::UpdateBridge, + } + } +} + /// The execution recipe captured at click time. Keeping readout *derivation* in /// TS (it already has the doctor report) avoids porting `actionableReadouts` /// into Rust; the backend just runs the recipe autonomously so the chain @@ -152,11 +175,14 @@ pub struct SetupPlan { #[serde(default)] install_fix_type: Option, /// Per-readout update fix identities to run after the install loop. Only - /// `updateMain` / `updateBridge` are valid; the exact source-aware command - /// is resolved by the backend from the crate's trusted freshness readout, - /// never supplied verbatim by the renderer. + /// `updateMain` / `updateBridge` are valid (enforced by the narrow + /// [`UpdateFixType`] wire type); the exact source-aware command is resolved + /// by the backend from the crate's trusted freshness readout, never supplied + /// verbatim by the renderer. Duplicate slots are rejected before dispatch + /// (see [`authorize_update_fixes`]) so a compromised renderer can't rerun the + /// trusted update command N times off one freshness snapshot. #[serde(default)] - update_fix_types: Vec, + update_fix_types: Vec, /// Whether the post-fix step probes PATH to confirm the agent resolved on /// disk. The frontend sends `hasBinary && !isBuiltIn`: a built-in or /// binary-less provider has nothing to resolve, so a clean fix run is taken @@ -500,6 +526,30 @@ async fn verify_installed( } } +/// Authorize the renderer-requested update slots before any executor runs. +/// Rejects a list with a duplicate slot: `run_install` runs each entry against +/// one freshness snapshot, so N copies of `updateMain` would rerun the trusted +/// update command N times through a single IPC request. The card only ever +/// names each slot at most once (`buildUpdateFixTypes` derives from distinct +/// readouts), so a duplicate is a forged/replayed request. Returns the +/// authorized `FixType` list on success. Pure so the boundary is unit-testable +/// without a Tauri handle. +fn authorize_update_fixes( + provider_id: &str, + requested: &[UpdateFixType], +) -> Result, String> { + let mut seen: Vec = Vec::with_capacity(requested.len()); + for slot in requested { + if seen.contains(slot) { + return Err(format!( + "duplicate '{slot:?}' update slot requested for '{provider_id}'" + )); + } + seen.push(*slot); + } + Ok(seen.into_iter().map(FixType::from).collect()) +} + /// Resolve the exact, source-aware update command for a requested update fix /// from the crate's freshness readout — the trusted source of truth. The /// renderer names only the readout slot (`updateMain` / `updateBridge`); the @@ -740,9 +790,13 @@ async fn run_install( // readouts to update (`updateMain` / `updateBridge`); the exact source-aware // command is resolved here from the crate's trusted freshness readout, so a // compromised renderer can't smuggle an arbitrary shell command through. - if !plan.update_fix_types.is_empty() { + // Authorize the slot list first — reject a duplicate slot before any + // executor runs, so a replayed slot can't rerun the update command N times + // off one freshness snapshot. + let update_fixes = authorize_update_fixes(provider_id, &plan.update_fix_types)?; + if !update_fixes.is_empty() { let fresh = find_check_fresh(app, provider_id).await?; - for fix_type in &plan.update_fix_types { + for fix_type in &update_fixes { let command = resolve_update_command(&fresh, fix_type)?; run_fix(app, registry, provider_id, fix_type.clone(), Some(command)).await?; } @@ -1108,6 +1162,48 @@ mod tests { assert!(resolve_update_command(&check, &FixType::UpdateMain).is_err()); } + #[test] + fn authorize_update_fixes_rejects_duplicate_slots_before_dispatch() { + // Regression: `run_install` runs each slot against one freshness + // snapshot, so a duplicate `updateMain` would rerun the trusted update + // command twice through one IPC request. A duplicate must reject before + // any executor target is produced. + assert!(authorize_update_fixes( + "claude", + &[UpdateFixType::UpdateMain, UpdateFixType::UpdateMain] + ) + .is_err()); + assert!(authorize_update_fixes( + "claude", + &[ + UpdateFixType::UpdateBridge, + UpdateFixType::UpdateMain, + UpdateFixType::UpdateBridge, + ] + ) + .is_err()); + } + + #[test] + fn authorize_update_fixes_allows_at_most_one_of_each_slot() { + // The card only ever names each slot once. An empty list, a single + // slot, and one of each (in either order) all authorize and map to the + // corresponding `FixType`. + assert_eq!(authorize_update_fixes("claude", &[]).unwrap(), Vec::new()); + assert_eq!( + authorize_update_fixes("claude", &[UpdateFixType::UpdateMain]).unwrap(), + vec![FixType::UpdateMain] + ); + assert_eq!( + authorize_update_fixes( + "claude", + &[UpdateFixType::UpdateBridge, UpdateFixType::UpdateMain] + ) + .unwrap(), + vec![FixType::UpdateBridge, FixType::UpdateMain] + ); + } + #[test] fn install_fix_for_check_returns_the_two_install_recipes() { assert_eq!( diff --git a/src-tauri/src/commands/doctor.rs b/src-tauri/src/commands/doctor.rs index 71f7be0b0..1a3b9afd4 100644 --- a/src-tauri/src/commands/doctor.rs +++ b/src-tauri/src/commands/doctor.rs @@ -1564,9 +1564,24 @@ pub async fn run_doctor_fresh( #[tauri::command] pub async fn run_doctor_fix( app_handle: AppHandle, + distro_state: State<'_, DistroBundleState>, + runtime_config_state: State<'_, RuntimeConfigState>, check_id: String, fix_type: FixType, ) -> Result<(), String> { + // Feature-policy gate: when Doctor is disabled by runtime config, + // `run_doctor`/`run_doctor_fresh` return an empty report, so no check is + // offered — but this command never loaded that config, so a renderer could + // invoke it directly and drive a native/managed/local/crate fix. Enforce the + // same policy here, before resolving offered state or any side effect. + // Hiding Doctor in the frontend is not a backend authorization boundary. + let runtime_config = runtime_config_state + .ready_config(distro_state.inner()) + .await?; + if !doctor_enabled(&runtime_config) { + return Err("Doctor is disabled by runtime configuration".to_string()); + } + // Resolve the fix the check *currently offers* from trusted state, then let // the pure planner authorize the request and pick the dispatch target in // one step. A forged, stale, or mismatched `(check_id, fix_type)` pair (an @@ -2027,6 +2042,31 @@ mod tests { assert!(text.find("== Tools ==").unwrap() < text.find("(git)").unwrap()); } + #[test] + fn run_doctor_fix_policy_gate_rejects_when_doctor_disabled() { + // `run_doctor_fix` guards on `doctor_enabled(&runtime_config)` before it + // resolves offered state or dispatches any fix. This pins the decision + // core of that guard: a config that disables Doctor selects no dispatch + // (the command returns `Err` before `offered_fix_for_check`), while the + // default (absent config) keeps fixes runnable. + let disabled = runtime_config_with_doctor(Some(RuntimeDoctorConfig { + enabled: Some(false), + kgoose_connectivity: None, + internal_tooling_checks: None, + })); + assert!(!doctor_enabled(&disabled)); + + let explicitly_enabled = runtime_config_with_doctor(Some(RuntimeDoctorConfig { + enabled: Some(true), + kgoose_connectivity: None, + internal_tooling_checks: None, + })); + assert!(doctor_enabled(&explicitly_enabled)); + + // Absent config defaults to enabled so the fix path keeps working. + assert!(doctor_enabled(&runtime_config_with_doctor(None))); + } + #[test] fn ensure_offered_fix_authorizes_only_the_currently_offered_fix() { // The dispatch gate the node-runtime and managed-install branches call From 236d1ecebbd886ac0ec415248d93a24246c95bc3 Mon Sep 17 00:00:00 2001 From: peon <9ac6794b000690b7e814eb1805ad32405d0bec7d52838de3a86cf967565dacc0@buzz.block.builderlab.xyz> Date: Sat, 15 Aug 2026 11:20:41 -0700 Subject: [PATCH 5/5] fix(doctor): authorize agent sign-in by backend-owned capability MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The offered-state auth gate required the current doctor check to offer FixType::Auth, which is impossible for an installed Copilot: the pinned doctor crate defines auth_command but no auth_status_command, so check_single_ai_agent reports AuthStatus::NotApplicable, status Pass, and fix_type None. The gate therefore blocked Copilot's only supported sign-in path (the product always presents Sign in for it via supportsAuth && !supportsAuthStatus). Authorize Auth against a backend-owned AuthCapability read from the pinned AI_AGENT_CHECKS table instead of trusting offered-state alone: - None (no auth_command, e.g. goose/pi): sign-in never authorized. - Probeable (has an auth-status probe, e.g. claude/codex/amp/cursor): authorized only when the check currently offers Auth, unchanged. - Unprobeable (login command but no status probe, i.e. Copilot): Doctor can't report Auth, so authorize the registered login when the agent is installed (path/bridge_path resolved) and offers no install fix. A not-installed provider still offers Command, so a forged sign-in rejects. The capability oracle is the static crate table, not renderer input; an unknown check id has no capability and rejects. Tests: replace the offered-state auth regressions with capability-based ones — probeable forged/offered cases, installed-Copilot allowed despite no offered fix, not-installed Copilot rejected, no-login-flow agent rejected, and an auth_capability table-mapping regression. Co-authored-by: Kalvin Chau Signed-off-by: Kalvin Chau --- src-tauri/src/commands/agent_setup.rs | 184 +++++++++++++++++++++----- 1 file changed, 151 insertions(+), 33 deletions(-) diff --git a/src-tauri/src/commands/agent_setup.rs b/src-tauri/src/commands/agent_setup.rs index f7feb00c7..89fb72fc9 100644 --- a/src-tauri/src/commands/agent_setup.rs +++ b/src-tauri/src/commands/agent_setup.rs @@ -597,22 +597,86 @@ fn resolve_update_command( }) } +/// The sign-in capability the pinned doctor crate declares for a check, read +/// from the backend-owned `AI_AGENT_CHECKS` table — never renderer input. It is +/// the authorization oracle for [`authorize_auth`]: "currently offers `Auth`" +/// is only valid for agents Doctor can actually probe. +#[derive(Debug, PartialEq, Eq)] +enum AuthCapability { + /// No `auth_command` (goose, pi): the agent has no sign-in flow, so a + /// renderer-requested `Auth` is never authorized. + None, + /// Both a login command and a status probe (claude, codex, amp, cursor): + /// Doctor reports `Auth` when installed-but-signed-out, so authorize a + /// sign-in only when the check currently offers it. + Probeable, + /// A login command but no status probe (copilot): Doctor can't observe the + /// auth state, so it reports `Pass`/no fix even when a sign-in is needed. + /// Authorize the registered login for the installed agent directly. + Unprobeable, +} + +/// The sign-in capability the pinned crate declares for a crate check id +/// (`ai-agent-*`). Resolved from the static `AI_AGENT_CHECKS` table so the auth +/// gate authorizes against backend-owned recipe metadata, not a renderer claim. +/// An unknown id has no capability, so auth is never authorized for it. +fn auth_capability(check_id: &str) -> AuthCapability { + doctor::agents::AI_AGENT_CHECKS + .iter() + .find(|info| info.id == check_id) + .map(|info| { + match ( + info.auth_command.is_some(), + info.auth_status_command.is_some(), + ) { + (false, _) => AuthCapability::None, + (true, true) => AuthCapability::Probeable, + (true, false) => AuthCapability::Unprobeable, + } + }) + .unwrap_or(AuthCapability::None) +} + /// Authorize a renderer-requested `Auth` action against the provider's current -/// doctor state (`offered` = the fix the check actually offers now). Sign-in is -/// only authorized when the check currently offers `Auth` (installed but not -/// authenticated); a request against a check that offers a different fix -/// (missing install → `Command`/`Bridge`) or no fix at all (already -/// authenticated, or auth status unknown) fails closed before the auth shell -/// command runs. Pure so the boundary is unit-testable without a Tauri handle. -fn authorize_auth(provider_id: &str, offered: Option) -> Result<(), String> { - match offered { - Some(FixType::Auth) => Ok(()), - Some(offered) => Err(format!( - "'{provider_id}' currently offers the '{offered:?}' fix, not sign-in" - )), - None => Err(format!( - "'{provider_id}' offers no sign-in in its current state" - )), +/// doctor `check` and its backend-owned [`AuthCapability`]. Sign-in fails closed +/// unless the pinned crate declares a login flow for the check: +/// +/// - `None` (no `auth_command`): never authorized. +/// - `Probeable` (has an auth-status probe): authorized only when the check +/// currently offers `Auth` (installed but not authenticated); a check that +/// offers an install fix or is already authenticated (no fix) rejects. +/// - `Unprobeable` (a login command but no status probe, e.g. Copilot): Doctor +/// can't report `Auth`, so authorize the registered login when the agent is +/// installed (`path`/`bridge_path` resolved) and offers no install fix. A +/// not-installed provider still offers `Command`, so a forged sign-in against +/// it rejects. +/// +/// Pure so the boundary is unit-testable without a Tauri handle. +fn authorize_auth(provider_id: &str, check: &doctor::DoctorCheck) -> Result<(), String> { + match auth_capability(&check.id) { + AuthCapability::None => Err(format!("'{provider_id}' has no sign-in flow")), + AuthCapability::Probeable => match &check.fix_type { + Some(FixType::Auth) => Ok(()), + Some(offered) => Err(format!( + "'{provider_id}' currently offers the '{offered:?}' fix, not sign-in" + )), + None => Err(format!( + "'{provider_id}' offers no sign-in in its current state" + )), + }, + AuthCapability::Unprobeable => { + if check.path.is_none() && check.bridge_path.is_none() { + return Err(format!( + "'{provider_id}' is not installed, so sign-in is unavailable" + )); + } + match &check.fix_type { + None => Ok(()), + Some(offered) => Err(format!( + "'{provider_id}' currently offers the '{offered:?}' fix, not sign-in" + )), + } + } } } @@ -823,13 +887,14 @@ async fn run_auth( ) -> Result<(), String> { // The renderer only names the *action*; before running the auth shell // command we re-read the provider's doctor check and authorize `Auth` - // against the fix it currently offers. A forged/stale sign-in for a - // provider that is missing (offers `Command`/`Bridge`), already - // authenticated, or has an unknown auth state (offers no fix) rejects here - // rather than executing the static ` login` command on demand. - // `find_check` also fails closed on an unknown provider. + // against its backend-owned sign-in capability (see [`authorize_auth`]). A + // forged/stale sign-in rejects here — for a probe-capable agent unless it + // currently offers `Auth`, and for an unprobeable one (Copilot) unless the + // agent is installed and offers no install fix — rather than executing the + // static ` login` command on demand. `find_check` also fails closed + // on an unknown provider. let check = find_check(app, provider_id).await?; - authorize_auth(provider_id, check.fix_type)?; + authorize_auth(provider_id, &check)?; set_phase(app, registry, provider_id, SetupPhase::Authenticating); run_fix(app, registry, provider_id, FixType::Auth, None).await?; @@ -1283,21 +1348,74 @@ mod tests { } #[test] - fn authorize_auth_rejects_a_forged_sign_in_against_a_non_auth_state() { - // Regression for the auth bypass: `run_auth` re-reads the provider check - // and authorizes `Auth` against its current offered fix. A provider that - // is missing (offers `Command`/`Bridge`) or already authenticated - // (offers no fix) must reject before the ` login` command runs. - assert!(authorize_auth("copilot-acp", Some(FixType::Command)).is_err()); - assert!(authorize_auth("amp-acp", Some(FixType::Bridge)).is_err()); - assert!(authorize_auth("copilot-acp", None).is_err()); + fn authorize_auth_rejects_forged_sign_in_for_a_probeable_agent() { + // Probe-capable agents (claude/codex/amp/cursor) report `Auth` only when + // installed-but-signed-out, so a forged sign-in against a missing + // (offers `Command`/`Bridge`) or already-authenticated (no fix) codex + // check must reject before the ` login` command runs. + let mut check = check_with_fix(Some(FixType::Command)); // id = ai-agent-codex + assert!(authorize_auth("codex-acp", &check).is_err()); + check.fix_type = Some(FixType::Bridge); + assert!(authorize_auth("codex-acp", &check).is_err()); + check.fix_type = None; + assert!(authorize_auth("codex-acp", &check).is_err()); } #[test] - fn authorize_auth_allows_a_currently_offered_sign_in() { - // Installed-but-signed-out is exactly the state that offers `Auth`, so a - // legitimate sign-in is authorized and reaches the auth command. - assert!(authorize_auth("copilot-acp", Some(FixType::Auth)).is_ok()); + fn authorize_auth_allows_a_currently_offered_sign_in_for_a_probeable_agent() { + // Installed-but-signed-out is the state a probe-capable agent reports as + // offering `Auth`, so a legitimate sign-in is authorized. + let check = check_with_fix(Some(FixType::Auth)); // id = ai-agent-codex + assert!(authorize_auth("codex-acp", &check).is_ok()); + } + + #[test] + fn authorize_auth_allows_installed_copilot_despite_no_offered_fix() { + // Regression for the Copilot auth-gate defect: Copilot declares an + // `auth_command` but no `auth_status_command`, so Doctor reports + // `Pass`/`fix_type = None` even when a sign-in is needed. The gate must + // authorize the registered login for the *installed* agent (resolved + // `path`) rather than blocking its only sign-in path. + let mut check = check_with_fix(None); + check.id = "ai-agent-copilot".into(); + check.path = Some("/opt/homebrew/bin/copilot".into()); + assert!(authorize_auth("copilot-acp", &check).is_ok()); + } + + #[test] + fn authorize_auth_rejects_copilot_sign_in_when_not_installed() { + // A not-installed Copilot still offers a `Command` install fix and has no + // resolved binary, so a forged sign-in against it must reject — the + // unprobeable capability authorizes login only for an installed agent. + let mut check = check_with_fix(Some(FixType::Command)); + check.id = "ai-agent-copilot".into(); + check.path = None; + check.bridge_path = None; + assert!(authorize_auth("copilot-acp", &check).is_err()); + } + + #[test] + fn authorize_auth_rejects_sign_in_for_an_agent_with_no_login_flow() { + // Goose declares no `auth_command`, so a renderer-requested sign-in is + // never authorized regardless of the reported check state. + let mut check = check_with_fix(None); + check.id = "ai-agent-goose".into(); + check.path = Some("/usr/local/bin/goose".into()); + assert!(authorize_auth("goose", &check).is_err()); + } + + #[test] + fn auth_capability_reflects_the_pinned_crate_table() { + // The gate's oracle is the backend-owned `AI_AGENT_CHECKS` table, not a + // renderer claim: probe-capable agents, the unprobeable Copilot, the + // no-login goose, and an unknown id each resolve to their capability. + assert_eq!(auth_capability("ai-agent-codex"), AuthCapability::Probeable); + assert_eq!( + auth_capability("ai-agent-copilot"), + AuthCapability::Unprobeable + ); + assert_eq!(auth_capability("ai-agent-goose"), AuthCapability::None); + assert_eq!(auth_capability("ai-agent-nope"), AuthCapability::None); } #[test]