feat(admin): install sandboxed plugins from the experimental registry#1011
Conversation
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | 56fb2b4 | May 14 2026, 06:05 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-perf-coordinator | 56fb2b4 | May 14 2026, 06:04 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
docs | 56fb2b4 | May 14 2026, 06:05 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-i18n | 56fb2b4 | May 14 2026, 06:04 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | 56fb2b4 | May 14 2026, 06:05 AM |
🦋 Changeset detectedLatest commit: 56fb2b4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Scope checkThis PR changes 3,195 lines across 30 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/blocks
@emdash-cms/cloudflare
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
There was a problem hiding this comment.
Pull request overview
Adds an opt-in experimental decentralized plugin registry path (per RFC #694), wiring server-side install + verification, runtime loading, and admin UI browse/install behavior behind experimental.registry.aggregatorUrl.
Changes:
- Core: add registry config normalization + derived registry plugin IDs, plus runtime sync/loading support for registry-installed sandboxed plugins.
- Server: add registry install endpoint + handler implementing aggregator resolve → policy checks → artifact fetch/limits → checksum verification → bundle extraction → R2 storage →
_plugin_stateupdates. - DB/Admin: migrate
_plugin_statefor registry metadata and update the admin UI to browse/details/install via the aggregator whenmanifest.registryis present.
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-workspace.yaml | Reorders/adjusts catalog dependencies used by new registry packages. |
| pnpm-lock.yaml | Locks new workspace deps for registry client/lexicons and related dependency graph changes. |
| packages/core/tests/integration/database/migrations.test.ts | Adds migration 038_registry_plugin_state to trailing-migrations integration test coverage. |
| packages/core/src/registry/plugin-id.ts | Introduces deterministic derived r_<hash> plugin IDs for registry installs. |
| packages/core/src/registry/config.ts | Adds registry config normalization, duration parsing, and aggregator URL validation. |
| packages/core/src/plugins/state.ts | Extends plugin state to include registry source + DID/slug fields and adds getRegistryPlugins(). |
| packages/core/src/plugins/marketplace.ts | Exports extractBundle and tightens bundle extraction limits for reuse by registry installs. |
| packages/core/src/emdash-runtime.ts | Splits sandboxed plugin sync/load into marketplace vs registry tiers and adds registry to manifest. |
| packages/core/src/database/types.ts | Extends _plugin_state table typing with registry columns and source='registry'. |
| packages/core/src/database/migrations/runner.ts | Registers migration 038. |
| packages/core/src/database/migrations/038_registry_plugin_state.ts | Adds registry columns + partial index for registry plugin rows (SQLite/D1 + Postgres). |
| packages/core/src/astro/types.ts | Extends manifest to include registry config; adds handler syncRegistryPlugins. |
| packages/core/src/astro/routes/api/admin/plugins/registry/install.ts | New admin API route for registry installs. |
| packages/core/src/astro/routes/api/admin/plugins/[id]/enable.ts | Syncs runtime bundles for marketplace/registry plugins when enabling. |
| packages/core/src/astro/middleware.ts | Exposes syncRegistryPlugins on the handlers surface. |
| packages/core/src/astro/integration/runtime.ts | Adds experimental.registry config typing and documentation. |
| packages/core/src/astro/integration/routes.ts | Injects the new registry install route. |
| packages/core/src/api/handlers/registry.ts | Implements the registry install handler with verification chain, SSRF defenses, and download limits. |
| packages/core/src/api/handlers/plugins.ts | Extends plugin listing/enable/disable responses to support registry-source plugins. |
| packages/core/src/api/handlers/marketplace.ts | Generalizes R2 bundle storage/load/delete by `source: marketplace |
| packages/core/src/api/handlers/index.ts | Re-exports the new registry install handler/types. |
| packages/core/package.json | Adds registry client + lexicons dependencies used by core runtime/handlers. |
| packages/admin/src/router.tsx | Switches marketplace browse/detail routes to registry UI when manifest.registry is present. |
| packages/admin/src/lib/api/registry.ts | Adds admin-side registry API: aggregator discovery calls + server install POST + policy filtering. |
| packages/admin/src/lib/api/plugins.ts | Extends admin plugin info typing for source='registry' + registry metadata fields. |
| packages/admin/src/lib/api/client.ts | Extends admin manifest typing to include registry block. |
| packages/admin/src/components/RegistryPluginDetail.tsx | Adds registry plugin detail page with consent dialog + install mutation. |
| packages/admin/src/components/RegistryBrowse.tsx | Adds registry browse/search UI backed by aggregator discovery. |
| packages/admin/package.json | Adds registry client/lexicons deps for the admin bundle. |
| .changeset/real-plants-sell.md | Declares minor bumps for emdash and @emdash-cms/admin and documents the feature/policy. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
packages/core/src/registry/plugin-id.ts:31
- The comment claims registry ids “cannot collide with marketplace plugin ids” because of the
r_prefix, but marketplace ids can also start withr_(they just must start with[a-z]). Either reserve/disallow ther_prefix for marketplace/config plugin IDs, or adjust the comment to describe this as a convention rather than an enforced guarantee.
* Existing sandbox cache keys (`${pluginId}:${version}`) keep
* working because the id contains no `:`.
* - Cannot collide with marketplace plugin ids. Marketplace ids must
* start with an ASCII letter (`/^[a-z]/`); the `r_` prefix means
* registry ids never look like marketplace ids.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // generous (32 MiB) relative to any plausible plugin bundle and | ||
| // strict enough to bound the worst case. |
| * everything to carry a composite identifier, we normalize the registry | ||
| * tuple to an opaque content-addressed id that satisfies the existing | ||
| * `validatePluginIdentifier` shape (`/^[a-z][a-z0-9_]*$/`). | ||
| * |
| if ( | ||
| input.acknowledgedDeclaredAccess !== undefined && | ||
| JSON.stringify(input.acknowledgedDeclaredAccess) !== | ||
| JSON.stringify(bundle.manifest.capabilities) | ||
| ) { | ||
| // We compare against the bundle's *capabilities* (the legacy | ||
| // shape) for v1 because EmDash's existing sandbox enforces | ||
| // capabilities, not the RFC's structured `declaredAccess`. | ||
| // Once the runtime starts enforcing `declaredAccess` natively, | ||
| // this comparison switches to that shape. Until then the | ||
| // admin UI lifts capabilities from the release record's | ||
| // extension data and the comparison is meaningful. | ||
| return { | ||
| success: false, | ||
| error: { | ||
| code: "DECLARED_ACCESS_DRIFT", | ||
| message: | ||
| "Plugin manifest has changed since you consented. Re-open the install dialog to review the new permissions.", | ||
| }, | ||
| }; |
| const capabilities: string[] = Array.isArray(ext?.capabilities) | ||
| ? (ext?.capabilities as string[]) | ||
| : declaredAccessToCapabilityList(ext?.declaredAccess); | ||
|
|
||
| const profile = pkg?.profile as { name?: string; description?: string } | undefined; | ||
| const verified = (pkg?.labels ?? []).some((l: { val?: string }) => l.val === "verified"); | ||
|
|
||
| const policyOk = | ||
| release && pkg ? releasePassesPolicy(release, { did: pkg.did, slug }, config.policy) : true; | ||
|
|
||
| const installMutation = useMutation({ | ||
| mutationFn: () => | ||
| installRegistryPlugin({ | ||
| handle, | ||
| slug, | ||
| version: release?.version, | ||
| acknowledgedDeclaredAccess: capabilities, | ||
| }), |
|
/review |
|
UnknownError: The operation timed out. |
|
@ascorbic Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
ascorbic
changed the title
|
/review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adversarial review focused on the install flow and the new admin UX paths. Three findings I think are worth resolving before merge; one drift-detection footgun, one DID-only fallback that can never succeed, and one cross-source-collision UX regression for sites that previously used the marketplace. Detail in the comments.
The verification chain itself looks solid -- SSRF posture, fetch limits, aggregator identity cross-check, IPv6/loopback handling, and the manifest.id rewrite to the derived pluginId are all in the right places. The 5 prior review rounds are visible in the code.
| slug, | ||
| version: release?.version, | ||
| acknowledgedDeclaredAccess: capabilities, | ||
| }), |
There was a problem hiding this comment.
HIGH — drift check trips on every install when the release record's extension is empty.
The browser always sends acknowledgedDeclaredAccess: capabilities (line 102), where capabilities is derived from releaseDoc.extensions[...releaseExtension].capabilities or, falling back, the flattened declaredAccess. When the publisher hasn't filled in the releaseExtension block (or it's there but capabilities is undefined and declaredAccess is empty/missing), capabilities is [].
The server then runs:
if (input.acknowledgedDeclaredAccess !== undefined) {
const acknowledged = normalizeCapabilities(input.acknowledgedDeclaredAccess); // []
const actual = normalizeCapabilities(bundle.manifest.capabilities); // e.g. ['content:read']
if (acknowledged.length !== actual.length || ...) return DECLARED_ACCESS_DRIFT;
}So every install of a plugin whose release record doesn't carry an extension.capabilities (or a declaredAccess that flattens to exactly the same strings the bundle uses) fails with DECLARED_ACCESS_DRIFT. The user sees "Plugin manifest has changed since you consented. Re-open the install dialog to review the new permissions." -- but the manifest never matched what was acknowledged in the first place.
This is a hard install gate keyed on whether publishers fully populated the release-record extension and used exactly the same capability strings the bundle uses (no normalization beyond sort/dedupe). For an experimental registry where almost no publisher has wired up the extension yet, that effectively bricks the install path.
Two fixes worth considering:
- Skip the drift POST when the dialog has nothing to acknowledge. If
capabilities.length === 0(no extension data resolved), omitacknowledgedDeclaredAccessfrom the body so the server falls into the documented "no consent UI -> no drift check" branch. The dialog still ran -- the user consented to "no listed permissions" -- and the bundle's real capabilities are still bound to the checksum-verified bytes. - Or: treat "no extension data" as an error state in the UI before showing the dialog, so the user never gets to click Install. That's stricter but at least the error is explicable.
Either way, the current behavior is silent install failure for every plugin without a complete release extension.
| return ( | ||
| <Link | ||
| to="/plugins/marketplace/$pluginId" | ||
| params={{ pluginId: `${pkg.handle ?? pkg.did}/${pkg.slug}` }} |
There was a problem hiding this comment.
HIGH — DID-only publishers can never be installed.
When pkg.handle is undefined the link falls back to pkg.did, building a URL like did:plc:abc/myslug. RegistryPluginDetail then splits at the first /, giving handle = "did:plc:abc", and POSTs that as handle to /_emdash/api/admin/plugins/registry/install.
The install handler validates:
if (!handle.includes(".")) {
return { code: "INVALID_HANDLE", ... };
}did:plc:abc has no ., so the install fails. (Even if it did contain a dot, the aggregator's resolvePackage.handle param is typed as a handle, not a DID, so it would likely fail downstream too.)
Net effect: cards render fine for any package whose handle the aggregator couldn't resolve, but clicking through and pressing Install always returns INVALID_HANDLE. Either:
- Hide / disable the card when no handle is present, or
- Add a server-side install-by-DID path (
resolvePackagehas both forms in the lexicon; treating a DID-shapedhandleparam as a DID and dispatching accordingly would be straightforward), or - At minimum, surface a clearer error than
INVALID_HANDLEso the admin understands the package isn't installable until the publisher's handle resolves.
Minor related concern same file: handles can in principle contain a . but no /; if the aggregator ever returns a handle with a / in it the split logic in RegistryPluginDetail.tsx:49-51 breaks. Worth a slug-side anchor (pluginId.lastIndexOf('/') would be safer since slugs disallow /, but handles don't disallow it as strongly as one might hope).
| // Registry detail when configured. The `pluginId` route param carries | ||
| // `${handle}/${slug}` in the registry case; the slash is encoded once | ||
| // by the router and decoded back here. | ||
| if (manifest?.registry) { |
There was a problem hiding this comment.
MEDIUM — registry-enabled sites lose access to existing marketplace-installed plugin detail pages.
The branch is purely on manifest.registry. A site that's been running with marketplace: "..." and has plugins already installed under source = "marketplace" will, the moment experimental.registry.aggregatorUrl is configured, route every visit to /plugins/marketplace/<some-marketplace-id> to RegistryPluginDetail. That component tries to parse <some-marketplace-id> as ${handle}/${slug}, finds no /, returns "Plugin not found."
Given the PR's stated invariant -- "Behaviour for sites that don't opt in: unchanged. The marketplace install path and all its consumers are untouched on the happy path." -- this is technically a sites-that-do-opt-in case, but it still silently breaks deep links and existing-plugin admin from the PluginManager (which presumably still links to /plugins/marketplace/$pluginId for marketplace rows).
Two plausible fixes:
- Discriminate on the param shape rather than the manifest flag. A registry pluginId always has a
/; a marketplace one never does.pluginId.includes("/")is a stable predicate. - Or keep two distinct routes (
/plugins/marketplace/$pluginIdand/plugins/registry/$handle/$slug) and have PluginManager link accordingly persource.
The second is cleaner but invasive; the first is one line.
| } | ||
|
|
||
| // Step 7: store in R2 under the registry prefix. | ||
| await storeBundleInR2(storage, pluginId, version, bundle, "registry"); |
There was a problem hiding this comment.
MEDIUM — orphaned R2 bundle on state-write failure / concurrent install race.
storeBundleInR2 writes the manifest, backend.js, and (optionally) admin.js to R2, then the next line writes the _plugin_state row. If the state-row write fails -- DB connection drop, PK conflict from a concurrent install of the same (handle, slug), etc. -- the R2 objects remain. There's no cleanup in the catch.
The concurrency case is concrete: two admin tabs / two clicks / a CI re-trigger can all pass the existing pre-check (line 753) because none of them has written the row yet, all three call storeBundleInR2, all three call stateRepo.upsert. The first wins; the others hit the PK on plugin_id (or update an unrelated existing row depending on race ordering with existing cache) and bubble up as INSTALL_FAILED. R2 has 1-3 orphan trees.
Two low-effort mitigations:
- Move the R2 write to after a successful
INSERT(use raw insert, not upsert, so the PK conflict is the gate; on conflict, abort without touching R2). The download/extract/verify still happens first to fail fast on bad bytes, but the side-effects ordering becomes safe. - Or, on any failure after
storeBundleInR2, calldeleteBundleFromR2(storage, pluginId, version, "registry")in the catch. Same posture as the update path's best-effort cleanup of the old version.
The second is simpler if you don't want to restructure the install order.
| // signed createdAt from the publisher's PDS (deferred to the | ||
| // follow-up that adds full MST verification). If the timestamp | ||
| // is missing or malformed, we fail closed and reject the install. | ||
| const minimumReleaseAge = registryConfig.policy?.minimumReleaseAge; |
There was a problem hiding this comment.
LOW — parseDurationSeconds called per install on un-normalized config.
registryConfig here is the user-supplied RegistryConfig (the integration option), not the normalized ManifestRegistryConfig. So parseDurationSeconds(minimumReleaseAge) runs on every install, on a string that's already been parsed once at manifest-build time. If a user mutates the integration option to an invalid value at runtime (unlikely but possible in some dev-reload setups), every install starts throwing inside the outer try/catch and surfacing as a generic INSTALL_FAILED 500 rather than the explicit "invalid duration" message the normalize-time path produces.
Not a security issue, but the policy check would be more robust if it stored the already-parsed seconds value on the runtime alongside the manifest config and read it from there instead of re-parsing. Same for the per-install minimumReleaseAgeExclude lowercase pass on line 697 -- duplicated work that's already done at normalize time.
| * `syncMarketplacePlugins`, sandbox cache keys). Rather than refactor | ||
| * everything to carry a composite identifier, we normalize the registry | ||
| * tuple to an opaque content-addressed id that satisfies the existing | ||
| * `validatePluginIdentifier` shape (`/^[a-z][a-z0-9_-]*$/`). |
There was a problem hiding this comment.
LOW — comment claims the validator pattern is [a-z0-9_]* but the actual validatePluginIdentifier allows hyphens ([a-z0-9_-]*).
The derived id format never produces hyphens, so this is purely a doc-vs-code drift, but it's load-bearing for the "cannot collide with marketplace plugin ids" reasoning two paragraphs down. The collision-resistance comment on line 29 already concedes the marketplace ids may include r_ and hyphens, so this is consistent in spirit -- just the pattern citation is wrong. Easy fix: update the comment to match /^[a-z][a-z0-9_-]*$/.
|
Review posted. Summary of findings: Review posted as #1011 (review) 6 line-anchored findings:
|
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
Astro bundles its own Zod and re-exports it as 'astro/zod'. Trusted plugins like @emdash-cms/plugin-forms import their route schemas via 'astro/zod', then pass those schemas to definePlugin() in core. With emdash's 'zod: ^4.3.5' resolving independently of Astro's caret, pnpm kept two Zod 4 patches in the tree (e.g. 4.3.6 alongside 4.4.1). Zod 4 embeds its semver in the type system, so two patches of Zod 4 are not assignable to each other. The forms plugin's route schemas (ZodObject<..., $strip>) were rejected by PluginRoute<TInput>['input'] (ZodType<unknown, unknown, $ZodTypeInternals<unknown, unknown>>) with 'Type "3" is not assignable to type "4"' on the internal version field. The native definePlugin overload silently failed, TS fell through to the StandardPluginDefinition overload, and reported a misleading 'id does not exist' error -- masking 8 cascading errors. Catalog-pinning Zod forces a single workspace-wide instance and restores normal overload resolution. No code changes needed in core or plugins/forms. Also adds a pnpm-workspace.yaml comment explaining the gotcha so the next person doesn't bump emdash's pin past Astro's range.
Adds opt-in support for installing sandboxed plugins from the decentralized plugin registry described in RFC #694. Enabled via `experimental.registry.aggregatorUrl` in the EmDash integration options; when set, the admin UI replaces marketplace browse/install with the registry path. Server: new install handler (RFC verification chain), endpoint at POST /_emdash/api/admin/plugins/registry/install, migration 038 adds `source = 'registry'` plus `registry_publisher_did` / `registry_slug` columns on `_plugin_state`, runtime sync split into shared marketplace + registry tiers via a normalized opaque `r_<hash>` plugin id. Browser: aggregator XRPC calls go direct from the admin UI via @emdash-cms/registry-client. Install POST runs through the server. Includes a minimum-release-age policy with a per-publisher exclude allowlist, enforced both client-side (UX) and server-side (gate). Hardening (5 rounds of adversarial review): bundle id rewritten to the derived pluginId before storage, aggregator identity cross-checked, artifact and aggregator URLs validated for SSRF (https-only in prod, IPv6 brackets handled), per-request and total budgets on every outbound call, decompressed bundle capped at 256 KiB to match the RFC publish-time limit, migration 038 idempotent on both SQLite and Postgres. Known gaps tracked for follow-up: full MST signature verification against the publisher's PDS, multibase multihash decoding (hex SHA-256 is accepted today), registry plugin update + uninstall handlers.
The generated lexicon types are committed to git so consumers don't need the codegen toolchain. Running lex-cli generate as part of the default build pipeline broke Cloudflare Pages builds for sites that pull registry-lexicons in transitively, because lex-cli imports lex.config.ts directly and Node in the CF Pages build environment can't load .ts natively. Codegen moves to a separate `regen` script (`pnpm regen` runs codegen + full build). Maintainers run it when they edit the lexicons; consumers just consume the committed output.
- Drift check normalizes capabilities (filter strings, dedupe, sort) on both browser and server so reorderings or junk entries can't trigger spurious rejection. Adds a shared normalizeCapabilities helper in registry/config.ts and a mirror in admin/lib/api/registry.ts. - RegistryPluginDetail no longer trusts the aggregator-supplied ext?.capabilities as already-validated string[]; runs it through normalizeCapabilities before display and before send. - Fix stale '32 MiB' docstring on extractBundle (cap is actually MAX_DECOMPRESSED_BUNDLE_BYTES = 256 KiB). - Fix plugin-id.ts JSDoc: validatePluginIdentifier regex is /^[a-z][a-z0-9_-]*$/ (allows hyphens); the prior 'cannot collide with marketplace ids' claim was too strong and is now framed as 'syntactically distinct, plus an explicit pre-existing-row check in the install handler.'
CI fixes:
- Rename normalizeCapabilities -> canonicalCapabilitiesForDriftCheck
to avoid namespace clash with the existing capability normalizer
exported from @emdash-cms/plugin-types via core's index. The old
name shadowed plugin-types' helper at the top level of core's dist,
which made the definePlugin() overload set look ambiguous to TS in
plugins/forms and caused a typecheck cascade there.
- [...seen].toSorted() instead of [...seen].sort() to clear the
e18e/prefer-spread-syntax + unicorn/no-array-sort lint errors.
Review findings (ask-bonk[bot]):
- HIGH: drift check tripped on every install when the release record's
extension was empty. The browser now omits acknowledgedDeclaredAccess
when capabilities is empty, opting out of the server-side drift gate
for the (currently common) case where publishers haven't filled in
the extension block. The bundle's real capabilities are still bound
to the checksum-verified bytes.
- HIGH: DID-only publishers (no resolvable handle) could be linked from
the browse grid but never installed because the server rejects
handles without a '.'. Cards now render as non-interactive with a
'Publisher handle unresolved' badge; the detail page surfaces a
matching warning and disables Install.
- MEDIUM: registry-enabled sites were unconditionally routing existing
marketplace plugin detail URLs to RegistryPluginDetail, breaking deep
links. Detail-route selection now discriminates by param shape
(pluginId.includes('/')) rather than the manifest flag.
- MEDIUM: state-row write failure after storeBundleInR2 left orphan
bundles. Best-effort cleanup in the catch via deleteBundleFromR2.
- LOW: parseDurationSeconds runs on the user-supplied integration
option per install (not the already-normalized manifest shape). Wrap
in try/catch and surface as REGISTRY_POLICY_INVALID rather than
letting it bubble to a generic INSTALL_FAILED.
- LOW: validator-pattern doc drift in plugin-id.ts (already fixed in
the prior commit).
The new RegistryConfig + ExperimentalConfig interfaces lived alongside definePlugin's overloads in astro/integration/runtime.ts. tsdown + rolldown's chunking decided to inline a bigger subset of plugin-related types into the entry chunk as a result, which broke definePlugin() overload resolution for trusted plugins building against core's dist on CI (plugins/forms failed with 'id does not exist in type StandardPluginDefinition'). Move both types to packages/core/src/registry/types.ts (still re-exported from runtime.ts for backwards compatibility) so the chunking matches main's layout and definePlugin's overloads resolve as before.
Aggregator (apps/aggregator):
- Add CORS to /xrpc/* so the admin UI can call it from any origin
(preflight 204, response headers on every method). Aggregator is a
public read-only service; * is correct here.
Core (packages/core):
- Implement multibase-multihash checksum verification by re-encoding
our SHA-256 digest in the same 'b<base32>' shape the registry CLI
produces, rather than decoding the publisher's checksum. Same trust
contract, no base32 decoder needed. Bare hex SHA-256 still accepted
as a convenience fallback.
- Switch install handler to take 'did' (not handle) so packages whose
handle the aggregator couldn't resolve are still installable. The
browser resolves handle→DID via the aggregator before posting and
sends DID directly; the server skips resolvePackage and goes
straight to getPackage.
- Coerce 'experimental.registry' bare-string shorthand into the full
RegistryConfig object via 'coerceRegistryConfig'. 'registry:
"..."' is now equivalent to 'registry: { aggregatorUrl: "..." }'.
- Plumb 'experimental' through the integration's serializableConfig
so the manifest endpoint actually sees the user's registry block.
Previously it was being stripped, so the admin UI never branched to
the registry path.
- Split RegistryConfig + ExperimentalConfig types into their own
module (registry/types.ts) so they don't get bundled into the
astro/integration/runtime.ts dist chunk -- the wider inlining was
breaking definePlugin overload resolution for trusted plugins
building against core's dist.
Admin (packages/admin):
- New <PublisherHandle> component + usePublisherHandle hook with
tri-state result ('ok' / 'invalid' / 'missing'). Renders @handle,
'Unverified publisher' (red), or DID respectively. Uses
@atcute/identity-resolver's LocalActorResolver for bidirectional
handle verification, localStorage-cached for 24h.
- Detail page disables install on 'invalid' status (publisher claims
a handle that doesn't round-trip back to its DID -- impersonation
risk). Surfaces 'We couldn't verify this publisher's identity'
alert in plain language.
- Detail page reads installed state from fetchPlugins() and swaps
the Install button to 'Installed' (disabled) when the package
already has a 'source = "registry"' row matching its DID + slug.
React Query's existing ['plugins'] invalidation handles the
post-install UI update.
- Browse cards reuse <PublisherHandle> (variant='card') and link by
handle when available, DID otherwise. Detail page parses either
form from the URL.
- Browser sends 'did' (not handle) in the install POST.
Workspace:
- '@cloudflare/kumo' moved to the pnpm catalog and bumped to ^1.16.0
workspace-wide. Older 1.10.0 was missing Sidebar export and being
hoisted into the admin via packages/blocks's transitive dep.
- Add '@atcute/multibase' to core (for checksum encoding) and
'@atcute/identity-resolver' to admin (for DID->handle resolution).
- Update DEFAULT_AGGREGATOR_URL + DiscoveryClient doc example from
'experimental-registry.emdashcms.com' to 'registry.emdashcms.com'
(the actual production host).
ascorbic
force-pushed
the
feat/registry-admin-ui
branch
from
b0eb848 to
7fb0d7e
Compare
Addresses 7 findings from the round-6 adversarial review and documents the eighth. #1 (high) Capability consent bypass [registry.ts, RegistryPluginDetail.tsx] The drift check was gated on the client sending acknowledgedDeclaredAccess. If the publisher's release record had no extension, the admin saw an empty permission dialog, omitted the acknowledgement, and the server skipped the check entirely -- letting a bundle whose manifest declares real capabilities slip through behind an empty consent UI. Server now extracts capabilities from the bundle manifest after download and refuses with DECLARED_ACCESS_REQUIRED if the bundle declares any capabilities and no acknowledgement was sent. Client always sends the list (empty when no extension) so the new server check is always armed. #2 (high) Concurrent install bundle deletion [registry.ts] Two parallel installs of the same (did, slug, version) both passed the pre-existing-row check, both uploaded to the same deterministic R2 prefix, and one then won the state-row PK race. The loser's catch block deleted the R2 bundle the winner had just written. On state-write failure we now re-query the state row: if a winner exists, we lost the race and must not touch the R2 bundle. Cleanup runs only when the failure is a real DB error, not a lost concurrent install. #3 (high) SSRF via DNS-resolving public hostnames [registry.ts, ssrf.ts moved] Literal-IP blocklist alone left a DNS-rebinding gap: any public DNS service resolving an attacker-chosen hostname to loopback / RFC1918 / 169.254.169.254 passed the URL check. The import pipeline already shipped resolveAndValidateExternalUrl which does Cloudflare DoH resolution and rejects on any forbidden resolved address; reuse it for artifact downloads. Move src/import/ssrf.ts to src/security/ssrf.ts to reflect that it's not import-specific. Leave a re-export shim at the old path so 13 existing callers keep working unchanged. Add #security/* path alias. #5 (high) Aggregator-supplied handles treated as verified [PublisherHandle.tsx] usePublisherHandle returned status: 'ok' with the aggregator-supplied handle whenever one was present, skipping local DID->handle round-trip. A compromised aggregator could label an attacker DID as e.g. 'stripe.com' and the UI would render it as verified. Always run LocalActorResolver via resolveDidToHandle; use the aggregator handle only for a cross-check. If the aggregator's claim differs from the verified handle, mark the publisher invalid. #6 (medium) Postgres migration 038 schema-qualification [038_registry_plugin_state.ts] The columns probe queried information_schema.columns without filtering by table_schema. A _plugin_state table in another schema (multi-tenant Postgres, per-test schemas) could make the migration skip the column adds. Filter by table_schema = current_schema(). #7 (medium) Install errors leak full artifact URLs [registry.ts] fetchArtifact recorded each full URL in the joined error message that bubbled up to the admin client. Artifacts hosted on storage backends often carry presigned tokens in the query string; failed installs were leaking those into HTTP responses and logs. Strip query and fragment when building client-visible errors (origin + path only); log the full URL server-side for debugging. #8 (medium) Credentialed aggregator URLs accepted [config.ts] validateAggregatorUrl accepted https://user:pass@example.com. The normalized URL ends up in the admin manifest and is shipped to every admin browser; browser fetch() also rejects credentialed URLs outright. Reject them at config-validation time. #4 (high, documented not fixed) Aggregator-trust-root scope [types.ts] Full MST proof / publisher signature verification is not in this PR; the server still trusts the aggregator-supplied (did, slug, checksum, artifact URL). Expand the JSDoc on EmDashConfig.experimental.registry to spell out exactly what the v1 trust contract is, what EmDash does verify independently (checksum, manifest id/version/capabilities), and what it doesn't (release-record signatures, replay). Recommendation: point aggregatorUrl only at an aggregator you operate or trust at centralized-source level until signature verification lands.
|
/review |
There was a problem hiding this comment.
Round-7 adversarial pass. The round-6 fixes look good — drift check now post-download against the bundle (matches acknowledgedDeclaredAccess: capabilities always-sent client behavior), bundle-deletion concurrent-install race is closed with a re-probe of the state row, publisher handles are verified locally before display, the migration is current_schema()-scoped, install error messages strip query/fragment, and credentialed aggregator URLs are rejected. Router branches on pluginId.includes("/") and registry IDs (r_…) route to MarketplacePluginDetail (no slash), which is the documented behavior given looksLikeRegistryId semantics.
One new HIGH-severity regression hidden in the "shared install primitives" framing: the extractBundle size cap was tightened from "unlimited" to 256 KiB / 32 tar entries, and extractBundle is on the marketplace install path. The changeset claim "Marketplace behavior is unchanged when the option is not set" is not accurate for marketplace bundles larger than 256 KiB decompressed, which the marketplace server itself accepts up to 50 MiB / 200 files. See the inline comment.
Two smaller findings on top.
LOW — no UX uninstall path for registry plugins, but they appear in PluginManager.
The "Uninstall" button (line 485 of PluginManager.tsx) is gated on isMarketplace. Registry-installed plugins (source === "registry") get listed in the manager — including their capabilities, install timestamp, version — but offer no way to remove them from the UI. The PR description acknowledges "Registry plugin update + uninstall handlers (install only this round)" as a known gap, but the admin sees no indication of that: the plugin is just installed-and-permanent from their perspective until they edit the DB and R2 by hand.
Two ways to soften this:
- Disable the toggle and hide the "View in Marketplace" action with an inline note like "Uninstall not available yet for registry plugins" so admins aren't surprised.
- Or block registry installs entirely until the uninstall handler lands. Strictly safer but probably overkill since the install is opt-in behind
experimental.registry.
Not a correctness bug — just a sharp edge for the experimental flag's early adopters.
| // so the entry count cap is set comfortably above RFC's 20-file limit. | ||
| // Going over either is a strong signal the bundle isn't a legitimate | ||
| // sandboxed plugin. | ||
| const MAX_DECOMPRESSED_BUNDLE_BYTES = 256 * 1024; |
There was a problem hiding this comment.
HIGH — silently shrinks the marketplace bundle size limit from "unlimited" to 256 KiB and the file count from 200 to 32.
extractBundle is the marketplace install path's tar-decoder (MarketplaceClientImpl.downloadBundle calls it at line 293 above this hunk). On main, extractBundle had no decompressed-size limit — it just streamed the gzip into a single new Response(decompressedStream).arrayBuffer(). The marketplace server (packages/marketplace/src/routes/author.ts) accepts publishes up to 50 MiB decompressed / 200 files / 5 MiB per file. After this PR, any marketplace plugin that decompresses to more than 256 KiB or contains more than 32 tar entries fails to install with INVALID_BUNDLE: Bundle decompressed size exceeds limit (262144 bytes) or Bundle has too many tar entries.
The cap is justified in the comment as "Aligns with RFC 0001 §Bundle size limits". RFC 0001 is the registry publish spec. It doesn't bind already-published marketplace plugins, and there is no migration path: the marketplace can already host bundles between 256 KiB and 50 MiB, and MAX_BUNDLE_SIZE in packages/registry-cli/src/bundle/utils.ts is a publish-time check, not a download-time one.
The changeset (.changeset/real-plants-sell.md) explicitly says "Marketplace behavior is unchanged when the option is not set." That's not true — these caps apply unconditionally because extractBundle is shared, and the path is reached by every marketplace install regardless of experimental.registry config.
Trigger: marketplace.install("some-plugin-id") for any existing marketplace plugin whose .tgz decompresses to > 256 KiB. Demonstrable with a single React component + an icon font, or a plugin that bundles a small dependency.
Fix options, in rough preference order:
- Branch the cap by source: pass
MAX_DECOMPRESSED_BUNDLE_BYTES/MAX_BUNDLE_TAR_ENTRIESintoextractBundle(default to the current 50 MiB / 200 marketplace-side values), and have only the registry install handler call it with the 256 KiB / 32 caps. Keeps marketplace identical tomain, applies RFC 0001 caps only to registry installs. - Raise the shared caps to match the marketplace server (50 MiB / 200), accept that the registry's downstream cap is looser than its publish-time cap, and document it as a gzip-bomb defense rather than an RFC-compliance gate.
- Keep the tighter caps shared but call them out explicitly in the changeset as a breaking change for marketplace installs and bump the package accordingly.
Option 3 is the worst because the breakage is silent until an admin tries to install. Option 1 preserves the property the changeset already promises.
There was a problem hiding this comment.
No current plugins are larger than that, so it's moot
| } | ||
| | undefined; | ||
| const extensionEntries = releaseDoc?.extensions ? Object.entries(releaseDoc.extensions) : []; | ||
| const ext = extensionEntries.find(([k]) => |
There was a problem hiding this comment.
LOW — startsWith on extension keys widens the namespace beyond intent.
extensionEntries.find(([k]) => k.startsWith("com.emdashcms.experimental.package.releaseExtension")) matches any future or unexpected NSID that begins with that string — e.g. com.emdashcms.experimental.package.releaseExtensionV2, ….releaseExtensionAttacker, ….releaseExtension.deprecated. None of those is a real attack vector since the bundle manifest is the only source of truth the sandbox enforces (the install handler explicitly notes this at lines 942-948 of registry.ts), but the resulting capability list still drives the consent dialog and the acknowledgedDeclaredAccess payload — a publisher can pick a slightly-different extension name and end up rendering a different permissions list than another publisher would for the same RFC-0001 fields.
Recommend k === "com.emdashcms.experimental.package.releaseExtension" (exact match). The lexicon NSID is fixed by RFC 0001; there's no reason to accept prefix variants.
|
Review posted successfully at #1011 (review). Summary of findings: Review posted as #1011 (review) 3 findings (1 line-anchored HIGH + 1 line-anchored LOW + 1 LOW in the review body):
Note that prior bonk review's findings from round 6 appear to have all been addressed in commit |
Two LOW findings from the round-7 review (PR #1011 comment). NSID exact-match in RegistryPluginDetail.tsx Round-6 left a startsWith() match on the release-extension key. RFC 0001 fixes the NSID for the release extension; accepting prefix variants (...releaseExtensionV2, ...releaseExtension.deprecated) would let a publisher render a different capability list than the canonical key would. Use exact-equality keyed lookup. Registry plugin uninstall affordance in PluginManager.tsx Registry-installed plugins appear in PluginManager but the Uninstall button is gated on isMarketplace. Admins see a permanent-looking install with no way to remove it short of editing the DB and R2 by hand. Add an inline note for source === 'registry' rows that says uninstall isn't available yet and points the admin at the disable toggle. Full uninstall handler lands in a follow-up PR.
What does this PR do?
Adds opt-in support for installing sandboxed plugins from the decentralized plugin registry described in RFC #694. Enabled by setting
experimental.registry.aggregatorUrlin the EmDash integration options. When set, the admin UI replaces the centralized marketplace browse/install with the registry path; otherwise nothing changes.Server
api/handlers/registry.ts) implementing the RFC verification chain: aggregator resolve → release selection → yanked-label + minimum-release-age checks → artifact fetch (mirrors → declared URL) → checksum verification → bundle extract → R2 store →_plugin_staterow.POST /_emdash/api/admin/plugins/registry/install.source = 'registry'plusregistry_publisher_did/registry_slugcolumns to_plugin_state. Dialect-branched (SQLite/D1 + Postgres), idempotent under partial re-apply.r_<hash>plugin id (derived from(publisher_did, slug)). The bundle'smanifest.idis required to equal the slug at install time and is rewritten to the derived pluginId before R2 storage and sandbox load.Browser
@emdash-cms/registry-client. Install POSTs to the server which re-resolves and re-verifies.minimumReleaseAgepolicy with a per-publisherminimumReleaseAgeExcludeallowlist (DID-only matching), enforced both client-side (UX) and server-side (gate).Hardening (5 rounds of adversarial review)
listReleasespagination, total artifact download budgetCloses #
Type of change
The feature is the implementation of RFC #694, which itself was approved via Discussion #296 and Discussion #307.
Checklist
pnpm typecheckpassespnpm lintpasses (0 new warnings on top of main's 6)pnpm testpasses (3257 core tests + 875 admin tests)pnpm formathas been runAI-generated code disclosure
Notes
Known gaps tracked for follow-up:
// Caveat:block.Behaviour for sites that don't opt in: unchanged. The marketplace install path and all its consumers are untouched on the happy path.
Shared install primitives:
extractBundle(now exported),storeBundleInR2,loadBundleFromR2, anddeleteBundleFromR2are generalized oversource: "marketplace" | "registry"with a default of"marketplace"so all existing call sites stay byte-identical. Bundle extraction caps tightened to match RFC 0001's publish-time limits (256 KiB decompressed, 32 tar entries).