Third-party plugin marketplaces - #1582
Conversation
b116566 to
a88a380
Compare
|
🚨 SLOP COP 🚨 · I am SlopCop. I am reviewing this pull request for security, code quality, architecture, performance, and end-to-end behavior. I will post one final review after the parallel checks finish. |
![bb-slop-cop[bot]](https://avatars.githubusercontent.com/in/4577281?s=60&v=4){kind=small}
| <ResourceBrowseGrid className="grid-cols-[repeat(auto-fill,minmax(min(100%,18rem),1fr))] gap-2"> | ||
| {section.entries.map((entry) => ( | ||
| <BrowseCard | ||
| key={`${entry.marketplace}/${entry.entryId}`} |
There was a problem hiding this comment.
🚨 slopcop/review — Medium: The new card key keeps marketplace identity only inside React.
The open callback and detail route still use only pluginId. The server also marks each same-ID listing as installed.
Two marketplaces can list the same ID. One card can then open the wrong listing or offer removal for another source.
Carry the marketplace and entry ID through the route and detail lookup. Show a separate occupied-ID state for another source.
| fetch: fetchMarketplace, | ||
| }); | ||
| try { | ||
| const name = materialized.catalog.name; |
There was a problem hiding this comment.
🚨 slopcop/review — Medium: Addition accepts a name that later routes reject.
The manifest name has no 64-character limit. The public SDK and route contract has that limit.
BB can add a marketplace that supported commands cannot refresh or remove by name.
Use the shared pluginMarketplaceNameSchema when the manifest enters the system. Remove the duplicate name rule.
| args.icon.path, | ||
| `entry "${args.entryId}" icon`, | ||
| ); | ||
| const bytes = new Uint8Array(await readFile(path)); |
There was a problem hiding this comment.
🚨 slopcop/review — High: The icon size check occurs after the full file read.
readFile loads all bytes before the 256 KiB validator runs. A Git or path marketplace can point to a very large local file.
Open the file once. Read at most the limit plus one byte. Then validate the same opened file.
| db: deps.db, | ||
| marketplaceName: name, | ||
| base: materialized.iconBase, | ||
| entries: materialized.catalog.plugins, |
There was a problem hiding this comment.
🚨 slopcop/review — High: A marketplace can start unbounded serial icon work.
The manifest has a 1 MiB JSON limit, but it has no plugin count or total icon byte limit.
The icon loader processes each entry in series. Thousands of entries can hold a refresh for hours and store large BLOB data.
Add an entry limit and an aggregate icon byte limit. Add one overall refresh deadline.
| icon: entryIconName(entry), | ||
| iconUrl: entryIconUrl(entry.id), | ||
| category: entryCategory(entry), | ||
| iconUrl: entryIconUrl(row.name, entry.id), |
There was a problem hiding this comment.
🚨 slopcop/review — Medium: Catalog search performs BLOB and installed-state queries for each entry.
entryIconUrl reads the full icon row, including its bytes. The result builder also queries the installed plugin table for each entry.
A large catalog causes thousands of synchronous SQLite queries and reads unused BLOB data.
Load icon hashes and installed identities with bulk queries. Do not select icon bytes for search results.
| }, | ||
|
|
||
| async install(input) { | ||
| const resolved = resolveEntry(input); |
There was a problem hiding this comment.
🚨 slopcop/review — High: Installation does not use the marketplace lock.
Refresh and removal use a lock for the marketplace name. Installation resolves the row and entry without that lock.
Removal can delete the row before installation writes catalog provenance. Refresh can also change the source during this operation.
Use the same name lock for install, refresh, removal, and same-name addition. Resolve the entry again inside the lock.
| "install refused: the marketplace source changed after confirmation; review it again", | ||
| ); | ||
| } | ||
| if (current.kind === "npm") return undefined; |
There was a problem hiding this comment.
🚨 slopcop/review — High: The npm confirmation does not identify exact code.
The confirmation contains only the package, range or tag, and registry. This branch accepts that data without an exact version or integrity value.
A mutable tag or range can select different full-trust code after the user confirms it.
Resolve the exact version and integrity before confirmation. Require the installer to use those exact values.
| }); | ||
| replacePluginMarketplaceIcons(tx, name, icons); | ||
| }); | ||
| deps.notifyCatalogChanged?.(); |
There was a problem hiding this comment.
🚨 slopcop/review — Medium: Marketplace notifications leave the marketplace list stale.
This new mutation sends the existing plugins-changed event. Its frontend handler invalidates catalog search, but not the marketplace list query.
A CLI command or another window can leave Settings with old counts, times, and errors.
Add the marketplace list key to the realtime invalidation set. Add a cache invalidation test.
| ...(entry.engines === undefined ? {} : { engines: entry.engines }), | ||
| ...(resolved.npmRegistry === undefined | ||
| ? {} | ||
| : { npmRegistry: resolved.npmRegistry }), |
There was a problem hiding this comment.
🚨 slopcop/review — High: A marketplace can select an unprotected npm registry.
This value comes from an untrusted marketplace manifest. The install path gives it to the npm resolver without the marketplace network controls.
The registry response can also select a tarball URL. Either request can reach a private host through DNS or a redirect.
Use the guarded downloader for registry and tarball requests. Add redirect, time, and byte limits.
| "clone", | ||
| "--quiet", | ||
| "--no-checkout", | ||
| source.url, |
There was a problem hiding this comment.
🚨 slopcop/review — High: Git marketplace clones bypass the protected network path.
The parser accepts HTTP URLs, private hosts, and embedded credentials. This call gives the URL directly to git clone.
Git uses its own DNS and redirect path. Thus, publicMarketplaceFetch cannot protect this request.
The clone also downloads unrestricted history. A refresh can contact private services or fill the server disk.
Require public HTTPS without credentials. Apply DNS and redirect checks. Use a shallow fetch and a checkout size limit.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain-English summary
This change lets BB add external plugin catalogs from HTTPS, Git, or a local path.
It adds marketplace controls to Settings, plugin discovery, installation confirmation, the CLI, the SDK, and the database.
Adding a marketplace only stores its catalog. It does not install or run a plugin.
Findings
I found six high-risk defects and four medium-risk defects.
- High: Git catalogs bypass the new network guard and can clone unlimited repository data.
- High: Custom npm registries and tarball locations bypass the same guard.
- High: Npm confirmation does not bind the exact version and integrity.
- High: Local icon files load before the size check.
- High: A catalog can start unbounded serial icon work and store unbounded icon data.
- High: Install can race with marketplace refresh or removal.
- Medium: A long manifest name creates a marketplace that public routes cannot manage.
- Medium: Catalog search runs queries for each entry and reads unused icon BLOB data.
- Medium: Duplicate plugin IDs lose marketplace identity in the detail and removal flows.
- Medium: Other clients can leave the marketplace list stale after a change.
The Browse view also renders every catalog card at once. Large catalogs need pagination or a windowed list.
The CLI help still calls all catalog entries official. That text now gives incorrect trust information.
Architecture and duplicate logic
The code now has two marketplace name rules. The manifest rule lacks the shared contract's length limit.
The network trust policy also has separate paths. HTTPS catalogs use the guard, but Git and npm do not.
Catalog search repeats icon and installed-state queries. Bulk queries can replace this duplicate work.
Checks
- GitHub CI passed on the reviewed SHA.
- All 86 focused marketplace server tests passed.
- All 80 focused app tests passed.
- All 25 focused CLI tests passed.
- Type checks passed for six affected packages.
- Browser QA passed add, browse, source disclosure, cancel, and remove.
- The dev CLI listed the added marketplace correctly.
- A larger local server selection passed 155 of 158 tests.
Two update tests exceeded their five-second limit under parallel load. One test found the macOS /tmp path alias.
I posted this as a comment-only review. I did not approve the pull request or use the request-changes option.
b26c5f6 to
1b150a4
Compare
02d86d4 to
11fb888
Compare
baec3b5 to
9d8d99a
Compare
Add, list, refresh, and remove the marketplaces bb reads plugin catalogs from. A marketplace stays a discovery layer: adding one validates and caches its catalog and installs nothing, removing one keeps its installed plugins running as direct installs with their source intent and exact resolution intact. The Phase 1 refresh and icon machinery now runs per marketplace row instead of only for bb-official, and reads a manifest over HTTPS, from a git checkout, or from a directory. git checkouts are staged, read, and deleted: the manifest and the validated icon bytes are what bb keeps. Install routing gains `<entry-id>@<marketplace>`; a bare entry id installs the single marketplace match, falls back to the bundled official plugin of that name, or is refused with the `id@marketplace` choices. The new install-plan route reports the true resolved source — npm package and range, or git url, ref/range, subdir, and the tag and commit a range currently lands on — so the CLI prompt and the app dialog confirm against what will actually be fetched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cover `bb marketplace add|list|refresh|remove`, `<id>@<marketplace>` install routing, the resolved-source confirmation, and the removal disposition in the plugins guide chapter, the bb-cli skill, the bb-plugin-authoring skill, and docs/configuration.md. The guide-coverage test now asserts every `bb marketplace` subcommand is documented too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Add a server suite over in-memory SQLite with a fake fetch and real git fixtures: add/refresh/remove for git and path marketplaces, the name collision and reserved-name refusals, ambiguous bare-install resolution, the removal disposition (provenance becomes direct while every field the update pipeline reads survives), icon isolation between marketplaces, and bb-official refreshing normally while a third-party refresh fails. Cover the route boundary, the CLI confirmation text for each source variant, the `bb marketplace` command output, the Browse grouping, and the app's third-party install disclosure. Regenerate the bundled plugin SDK types for the widened catalog contract. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The store lists plugins bundled with the app under bb-official, so a Browse card sends "<id>@BB-official" for them. Resolve that qualified form to the bundled copy instead of refusing it, and name the listing marketplace in `bb plugin search` once a third-party entry appears. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A git or path marketplace read its manifest fully into memory and checked the size afterwards. Stat first, so an oversize document is refused before it is loaded — the same bound an https manifest gets from its content-length. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The marketplace list and the install plan added two more query keys to the catalog query module. The plugin cache owner reads the marketplace prefix, so the keys belong with the other plugin keys in query-keys.ts, where the boot-path registry can read them without the catalog client. Boot payload: 1667.7 KB -> 1658.7 KB raw, 437.9 KB -> 435.6 KB brotli.
Category returns to filter-only; entries render as one grid per marketplace group instead of tag-derived sections. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An entry this BB cannot install is noise on an install surface. The search API keeps returning incompatible entries with reasons for the CLI status output. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The installed corner control reused the Download arrow, which reads as an available install. A check reads as state; the tooltip names the uninstall action it opens. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The manifest accepted a name with no length limit while the public SDK and route contract cap it at 64 characters. BB could store a marketplace that no supported command could refresh or remove by name. The manifest now parses its own name with pluginMarketplaceNameSchema, so one rule decides what bb can address, and the published schema carries the same limit.
Refresh, removal, and same-name addition each took the per-marketplace lock; installation did not. A removal could delete the row, or a refresh retarget the entry, between resolving the plan and writing catalog provenance — and the plugin would trace back to a listing that no longer said what it said. Installation now takes the same lock and resolves the entry again inside it, so it runs against the row that is current under the lock. Its test lands with the npm binding in the next commit: the four new third-party cases share one block.
A git entry's confirmation named the exact commit, and the install refused any other one. The npm side showed only the package, the range or dist-tag, and the registry. Both of those are mutable, so a listing could deliver different full-trust code after the user confirmed it. The install plan now resolves the exact version, and the integrity when the registry publishes one, through the same registry and the same selection rules the install uses. A third-party install carries that pair into the installer, which refuses any drift — symmetric with the git commit binding. The four new third-party cases share one block, so this also brings the tests for the marketplace install lock and the manifest name limit.
The search help called every listing one of "BB's official plugins", and the install messages called any catalog entry an "official plugin entry". A host can now add marketplaces BB does not review, so the text names the source and says which one BB reviews.
main reordered a pending-interaction enum, so this layer's committed bundled declarations and the template copy no longer matched what the sources generate.
The published schema and the parser both cap a marketplace name at 64 characters now, so the parity fixture set covers it.
Cards read "By: <author>"; bundled entries attribute to BB Team at the server boundary so the CLI shows the same. The marketplaces settings section moves its intro into the section description, groups the add form with its hint, pads list rows, and renders Official as a badge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
9d8d99a to
17a5bb2
Compare
A catalog install can come from any marketplace, and the plugin list does not say which, so labeling every catalog install BB Official was a wrong trust signal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Top layer (4/4) of the plugin marketplace stack. Prerequisite: #1581 (git tag semver).
Summary
bb marketplace add | list | refresh | removeover new validated routes, accepting an https manifest URL,git:<url>[@ref], orpath:<dir>. The manifest's own name is its identity; collisions and the reservedbb-officialare refused; identity drift on refresh is rejected. Adding installs nothing.bb plugin install <entry>@<marketplace>; a bare id resolves across marketplaces (one match installs, several fail listing qualified choices), with every existing source form untouched.directprovenance retaining full source intent and exact resolution — update checks keep working afterward — then deletes catalog rows and icons. Re-adding the same marketplace works.plugins.marketplaces.*,catalog.installPlan), guide, and skills updated in the same change.Validation
Standalone green: workspace typecheck and lint; server/app/cli/db/integration suites (known umask failure aside). End-to-end tests cover the add → install-with-plan → range update → moved-tag refusal → removal → still-updates-as-direct lifecycle, icon isolation, SSRF policy, and re-add.
No server↔daemon wire shape changes anywhere in the stack; protocol version 119 is unchanged deliberately.