From e805c29c703f38a8cb0f0203809d2fd5772c40a9 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 27 Sep 2026 13:03:14 -0700 Subject: [PATCH] Marketplace: refuse an update to a version that is not installable (PR review #1) Install and update now share ops::installable_at_head: the reviewed head must still be the head and the item's catalog entry there must be valid. The old update check (item_files) never parsed hook.json, so an upstream hook with an unknown event could be pinned and then held by every sync. Co-Authored-By: Claude Opus 5.5 --- .../src/commands/marketplace_commands.rs | 138 ++++++++++++++---- 1 file changed, 106 insertions(+), 32 deletions(-) diff --git a/app/src-tauri/src/commands/marketplace_commands.rs b/app/src-tauri/src/commands/marketplace_commands.rs index 2d716ad..bad3858 100644 --- a/app/src-tauri/src/commands/marketplace_commands.rs +++ b/app/src-tauri/src/commands/marketplace_commands.rs @@ -8,9 +8,7 @@ use tauri::{AppHandle, Emitter, State}; use tokio::sync::oneshot; use crate::docker::container::is_container_running; -use crate::marketplace::{ - self as mk, auth, catalog, diff, gh_login, git, tree::GitTree, MarketplaceManager, -}; +use crate::marketplace::{self as mk, auth, diff, gh_login, git, MarketplaceManager}; use crate::models::marketplace::{ is_valid_commit, is_valid_item_key, AccountMethod, FileDiff, InstallScope, ItemUpdate, Marketplace, MarketplaceAccount, MarketplaceInstall, MarketplaceItemRef, MarketplaceSnapshot, @@ -24,7 +22,7 @@ use crate::AppState; /// testable without a Tauri runtime. pub(crate) mod ops { use crate::marketplace::{auth, git}; - use crate::models::marketplace::{MarketplaceInstall, MarketplaceItemRef}; + use crate::models::marketplace::{MarketplaceInstall, MarketplaceItemRef, MarketplaceSnapshot}; /// Insert, or replace the install of the same item (a re-install re-pins). pub fn upsert_install(list: &mut Vec, inst: MarketplaceInstall) { @@ -82,6 +80,39 @@ pub(crate) mod ops { Ok(head.to_string()) } + /// The one rule install and update share (PR review #1): pin + /// `expected` only if it is still the head (see [`reviewed_head`]) and + /// the item, as the catalog reads it at that head, is valid. `snap`'s + /// items are always parsed at `snap.head_commit`, so for a hook this + /// means its `hook.json` parses and names only known events — exactly + /// what the payload later requires. `action` is "installed"/"updated". + pub fn installable_at_head( + snap: &MarketplaceSnapshot, + item: &MarketplaceItemRef, + expected: &str, + marketplace_name: &str, + action: &str, + ) -> Result { + let head = reviewed_head(snap.head_commit.as_deref(), expected, marketplace_name)?; + let entry = snap + .items + .iter() + .find(|i| i.kind == item.kind && i.key == item.key) + .ok_or_else(|| { + format!( + "\"{}\" is no longer in \"{}\" — refresh the marketplace.", + item.key, marketplace_name + ) + })?; + if let Some(reason) = &entry.invalid { + return Err(format!( + "\"{}\" cannot be {} at this version: {}", + entry.name, action, reason + )); + } + Ok(head) + } + /// An unvalidated value as it may appear in an error: quoted and escaped /// (`{:?}`) and capped at 60 characters, since it can come from an /// import file rather than from what the person just typed. @@ -263,6 +294,75 @@ pub(crate) mod ops { ); } + /// PR review #1: install and update share one rule — the item as the + /// catalog reads it at the reviewed head must be valid. `item_files` + /// alone (the old update check) accepts a hook whose `hook.json` names + /// an unknown event, which every sync would then hold back. + #[tokio::test] + async fn an_item_invalid_at_the_reviewed_head_is_neither_installed_nor_updated() { + use crate::marketplace::test_support::GitFixture; + use crate::marketplace::{catalog, tree::GitTree, MarketplaceManager}; + use crate::models::marketplace::Marketplace; + use crate::models::AppSettings; + + let Some(fx) = GitFixture::new() else { return }; + fx.with_all_kinds(); + fx.write( + "hooks/notify-on-stop/hook.json", + r#"{"hooks":{"PreFoo":[{"hooks":[{"type":"command","command":"x"}]}]}}"#, + ); + let head = fx.commit("bad hook event"); + let data = tempfile::tempdir().unwrap(); + let mgr = MarketplaceManager::new(data.path().to_path_buf()); + let mut settings = AppSettings::default(); + settings.marketplaces.push(Marketplace { + id: "m1".into(), + name: "Team".into(), + url: fx.url(), + branch: None, + account_id: None, + }); + let snap = crate::marketplace::refresh_marketplace(&mgr, &settings, "m1").await; + + let repo = crate::marketplace::git::cache_path(data.path(), "m1"); + let tree = GitTree::open(&repo, &head).unwrap(); + assert!( + catalog::item_files(&tree, ItemKind::Hook, "notify-on-stop").is_ok(), + "the old update check let this through" + ); + + let hook = MarketplaceItemRef { + marketplace_id: "m1".into(), + kind: ItemKind::Hook, + key: "notify-on-stop".into(), + }; + for action in ["installed", "updated"] { + let e = installable_at_head(&snap, &hook, &head, "Team", action).unwrap_err(); + assert!(e.contains(&format!("cannot be {action}")), "{e}"); + assert!(e.contains("PreFoo"), "{e}"); + } + let agent = MarketplaceItemRef { + kind: ItemKind::Agent, + key: "code-reviewer".into(), + ..hook.clone() + }; + assert_eq!( + installable_at_head(&snap, &agent, &head, "Team", "updated").unwrap(), + head + ); + let gone = MarketplaceItemRef { + key: "no-such-agent".into(), + ..agent + }; + let e = installable_at_head(&snap, &gone, &head, "Team", "updated").unwrap_err(); + assert!(e.contains("no longer in"), "{e}"); + assert!( + installable_at_head(&snap, &hook, &"b".repeat(40), "Team", "installed") + .unwrap_err() + .contains("changed since you reviewed") + ); + } + /// Pre-flight F13: the add form and the fetch agree on what a branch /// is, so a name the fetch would refuse is refused up front. #[test] @@ -693,23 +793,7 @@ pub async fn install_marketplace_item( let settings = state.settings_store.get(); let m = find_marketplace(&settings, &item.marketplace_id)?; let snap = snapshot_blocking(&state, &m).await?; - let head = ops::reviewed_head(snap.head_commit.as_deref(), &expected_commit, &m.name)?; - let entry = snap - .items - .iter() - .find(|i| i.kind == item.kind && i.key == item.key) - .ok_or_else(|| { - format!( - "\"{}\" is no longer in \"{}\" — refresh the marketplace.", - item.key, m.name - ) - })?; - if let Some(reason) = &entry.invalid { - return Err(format!( - "\"{}\" cannot be installed: {}", - entry.name, reason - )); - } + let head = ops::installable_at_head(&snap, &item, &expected_commit, &m.name, "installed")?; let inst = MarketplaceInstall { marketplace_id: item.marketplace_id.clone(), kind: item.kind, @@ -829,17 +913,7 @@ pub async fn update_marketplace_item( let settings = state.settings_store.get(); let m = find_marketplace(&settings, &item.marketplace_id)?; let snap = snapshot_blocking(&state, &m).await?; - let head = ops::reviewed_head(snap.head_commit.as_deref(), &expected_commit, &m.name)?; - - let repo = git::cache_path(state.marketplace.data_root(), &m.id); - let (kind, key, at) = (item.kind, item.key.clone(), head.clone()); - tokio::task::spawn_blocking(move || -> Result<(), String> { - let tree = GitTree::open(&repo, &at)?; - catalog::item_files(&tree, kind, &key).map(|_| ()) - }) - .await - .map_err(|e| format!("Checking the new version failed: {e}"))? - .map_err(|e| format!("\"{}\" cannot be updated: {e}", item.key))?; + let head = ops::installable_at_head(&snap, &item, &expected_commit, &m.name, "updated")?; match scope { InstallScope::Global => {