diff --git a/.changeset/plugin-admin-slug-uniqueness.md b/.changeset/plugin-admin-slug-uniqueness.md new file mode 100644 index 0000000000..a278de1014 --- /dev/null +++ b/.changeset/plugin-admin-slug-uniqueness.md @@ -0,0 +1,34 @@ +--- +"nextly": patch +"create-nextly-app": patch +"@nextlyhq/admin": patch +"@nextlyhq/admin-css": patch +"@nextlyhq/blocks-engine": patch +"@nextlyhq/blocks-react": patch +"@nextlyhq/ui": patch +"@nextlyhq/adapter-drizzle": patch +"@nextlyhq/adapter-postgres": patch +"@nextlyhq/adapter-mysql": patch +"@nextlyhq/adapter-sqlite": patch +"@nextlyhq/storage-s3": patch +"@nextlyhq/storage-uploadthing": patch +"@nextlyhq/storage-vercel-blob": patch +"@nextlyhq/plugin-form-builder": patch +"@nextlyhq/plugin-page-builder": patch +"@nextlyhq/plugin-seo": patch +"@nextlyhq/plugin-sdk": patch +"@nextlyhq/eslint-config": patch +"@nextlyhq/prettier-config": patch +"@nextlyhq/telemetry": patch +"@nextlyhq/tsconfig": patch +"@nextlyhq/builder": patch +"@nextlyhq/module-specifiers": patch +--- + +Reject duplicate plugin admin slugs at boot. `pluginAdminSlug` collapses every +non-alphanumeric run to a single dash, so distinct package names can map to one +slug and the plugins then share a single admin address — one plugin's detail +page opens the other's, and host `pluginOverrides` apply to the wrong package. +No lookup downstream can detect this, because every lookup along that address +returns a plugin. `resolvePlugins` now refuses to start, naming both packages +and the slug they collide on. diff --git a/packages/nextly/src/cli/utils/config-loader.ts b/packages/nextly/src/cli/utils/config-loader.ts index f4855cb5dc..64ac2f4483 100644 --- a/packages/nextly/src/cli/utils/config-loader.ts +++ b/packages/nextly/src/cli/utils/config-loader.ts @@ -56,6 +56,7 @@ import { finalizeRelationTargets, validateCrossPluginRelations, } from "../../plugins/schema/validate-relations"; +import { validatePluginSlugs } from "../../plugins/validate-slugs"; import { bundleAndRequire } from "./config-bundler"; @@ -559,6 +560,14 @@ async function loadConfigInternal( } } + // The transformed list, for the same reason the runtime validates its + // own: a `setup` transformer can add or rename plugins into a slug + // collision, and everything below consumes the transformed config. The + // CLI has to agree with boot here — otherwise `nextly build`, a + // migration or a db sync accepts and acts on a configuration the + // deployed app then refuses to start on. + validatePluginSlugs(transformedConfig.plugins ?? []); + // Fold plugin contributions. Extend targets that aren't code/plugin // entities are DEFERRED (candidate Builder/UI-schema targets) rather than // thrown, so a plugin may extend/relate to a Builder-made collection diff --git a/packages/nextly/src/di/register.ts b/packages/nextly/src/di/register.ts index e92a734544..5d04a4d5cc 100644 --- a/packages/nextly/src/di/register.ts +++ b/packages/nextly/src/di/register.ts @@ -125,6 +125,7 @@ import { registerPluginService, } from "../plugins/services/plugin-services-registry"; import { clearPluginSubscriptions } from "../plugins/subscription-tracker"; +import { validatePluginSlugs } from "../plugins/validate-slugs"; import type { CollectionSource, FieldDefinition, @@ -429,6 +430,12 @@ export async function registerServices( // Layer 0b: Process Plugin Config Transformers (resolved order) // ---------------------------------------- const setupConfig = await applyPluginConfigTransformers(resolvedConfig); + // Again on the transformed list, because a `setup` transformer may add, + // rename or replace entries in `plugins` — and everything from here down + // consumes the transformed config, not the list `resolvePlugins` checked. + // Boot is where this should fail; without it a transformer-introduced + // collision would surface on the first admin-meta request instead. + validatePluginSlugs(setupConfig.plugins ?? []); // ---------------------------------------- // Layer 0c: Fold declarative plugin schema contributions (D3/D12/D50) diff --git a/packages/nextly/src/plugins/admin-meta.ts b/packages/nextly/src/plugins/admin-meta.ts index e5365d6395..686f668138 100644 --- a/packages/nextly/src/plugins/admin-meta.ts +++ b/packages/nextly/src/plugins/admin-meta.ts @@ -27,6 +27,7 @@ import type { } from "./plugin-context"; import { pluginAdminSlug } from "./plugin-slug"; import { validatedClientConfig } from "./validate-client-config"; +import { validatePluginSlugs } from "./validate-slugs"; /** * The serialized admin-meta entry for a single plugin, consumed by the admin @@ -161,6 +162,15 @@ export function buildPluginAdminMeta( plugins: PluginDefinition[], pluginOverrides: Record | undefined ): PluginAdminMeta[] { + // Here, and not only at boot, because this is the one place a plugin list + // becomes ADDRESSES: the slug below is the admin's URL for the plugin and + // the key its host override is read by. Two boot paths do not reach it — + // `createDynamicHandlers` initializes services lazily and serves the public + // admin-meta endpoint before that happens, and a `setup` transformer can + // rewrite `config.plugins` after `resolvePlugins` has already run. Both + // would publish ambiguous addresses from a list nothing had checked. + validatePluginSlugs(plugins); + return plugins.map(plugin => { const slug = pluginAdminSlug(plugin.name); const hostOverride = pluginOverrides?.[slug]; diff --git a/packages/nextly/src/plugins/resolve.test.ts b/packages/nextly/src/plugins/resolve.test.ts index f476fd7374..5e580451c5 100644 --- a/packages/nextly/src/plugins/resolve.test.ts +++ b/packages/nextly/src/plugins/resolve.test.ts @@ -21,6 +21,27 @@ describe("resolvePlugins", () => { expect(out.map(x => x.name)).toEqual(["b", "a"]); }); + /** + * The wiring, not the check. `validatePluginSlugs` has its own suite; this + * asserts `resolvePlugins` actually calls it — without this, removing the + * call leaves every slug test green while nothing at boot runs it. + * + * The two names differ only by separators, which is the whole point: they + * are distinct legal packages that produce one admin address. + */ + it("rejects two plugins that resolve to the same admin slug", () => { + let reason: string | undefined; + try { + resolvePlugins([p("@acme/plugin-seo"), p("acme_plugin_seo")], { + coreVersion: "1.0.0", + }); + } catch (error) { + reason = (error as { logContext?: { reason?: string } }).logContext + ?.reason; + } + expect(reason).toBe("duplicate-admin-slug"); + }); + it("surfaces a version error even when the graph is otherwise orderable", () => { expect(() => resolvePlugins([p("a", { nextly: ">=2.0.0" }), p("b")], { diff --git a/packages/nextly/src/plugins/resolve.ts b/packages/nextly/src/plugins/resolve.ts index 3d9ff0e888..5f7817a919 100644 --- a/packages/nextly/src/plugins/resolve.ts +++ b/packages/nextly/src/plugins/resolve.ts @@ -1,6 +1,7 @@ import type { PluginDefinition } from "./plugin-context"; import { topoSortPlugins } from "./topo-sort"; import { assertClientConfigs } from "./validate-client-config"; +import { validatePluginSlugs } from "./validate-slugs"; import { validatePluginVersions } from "./validate-versions"; export interface ResolvePluginsOptions { @@ -28,5 +29,10 @@ export function resolvePlugins( // other fail-fast checks rather than surfacing when the admin first asks for // its metadata and losing the whole branding response with it. assertClientConfigs(plugins); + // Two plugins sharing an admin slug share an address, and nothing downstream + // can detect it: every lookup along that address returns a plugin, which is + // what a correct lookup returns. Registration is where the ambiguity is still + // observable. + validatePluginSlugs(plugins); return topoSortPlugins(plugins); } diff --git a/packages/nextly/src/plugins/validate-slugs.test.ts b/packages/nextly/src/plugins/validate-slugs.test.ts new file mode 100644 index 0000000000..613d0924d9 --- /dev/null +++ b/packages/nextly/src/plugins/validate-slugs.test.ts @@ -0,0 +1,140 @@ +/** + * A plugin's admin slug is its address: the admin builds + * `/admin/plugins/` from it, core namespaces plugin admin routes with + * it, and host `pluginOverrides` are keyed by it. Two plugins reaching the + * same slug therefore share one address, and nothing downstream can notice — + * which is why this is checked at registration. + */ +import { describe, expect, it } from "vitest"; + +import { buildPluginAdminMeta } from "./admin-meta"; +import type { PluginDefinition } from "./plugin-context"; +import { validatePluginSlugs } from "./validate-slugs"; + +function plugin(name: string): PluginDefinition { + return { name, version: "1.0.0", nextly: "*" } as PluginDefinition; +} + +/** + * The thrown `NextlyError` carries a deliberately generic `message` + * ("Plugin configuration is invalid.") for the client; the specific failure + * lives in `logContext.reason`. Matching on `message` would pass for every + * plugin-resolution failure alike — an incompatible version included — so + * these assert the reason instead. + */ +function collisionReason(names: string[]): string | undefined { + try { + validatePluginSlugs(names.map(plugin)); + } catch (error) { + return (error as { logContext?: { reason?: string } }).logContext?.reason; + } + return undefined; +} + +describe("validatePluginSlugs", () => { + it("accepts plugins whose slugs differ", () => { + expect(() => + validatePluginSlugs([plugin("@acme/one"), plugin("@acme/two")]) + ).not.toThrow(); + // The control on `collisionReason` itself: it must report undefined when + // nothing collides, or every assertion above would pass on a helper that + // always returned the reason it was looking for. + expect(collisionReason(["@acme/one", "@acme/two"])).toBeUndefined(); + }); + + it("accepts an empty plugin list", () => { + expect(() => validatePluginSlugs([])).not.toThrow(); + }); + + /** + * The separating cases, and the reason a check on the NAMES would not do. + * `pluginAdminSlug` lowercases and collapses each non-alphanumeric run to a + * single dash, so every pair below is two distinct, legal package names that + * produce one address. + */ + it.each([ + ["scope separator", "@acme/plugin-seo", "acme-plugin-seo"], + ["a dot for a dash", "@acme/plugin-seo", "@acme/plugin.seo"], + ["case", "@acme/plugin-seo", "@ACME/Plugin-SEO"], + ["underscores", "@acme/plugin-seo", "acme_plugin_seo"], + ["a doubled separator", "@acme/plugin-seo", "@acme//plugin--seo"], + ])("rejects two plugins colliding by %s", (_why, first, second) => { + expect(collisionReason([first, second])).toBe("duplicate-admin-slug"); + }); + + /** + * The message is the entire remedy — the reader has to rename one package, + * and this is the only place the pair is ever stated together. Asserting it + * throws would pass on an error naming neither. + */ + it("names both plugins and the slug they collide on", () => { + let caught: unknown; + try { + validatePluginSlugs([ + plugin("@acme/plugin.seo"), + plugin("acme_plugin_seo"), + ]); + } catch (error) { + caught = error; + } + + const message = (caught as { logMessage?: string })?.logMessage ?? ""; + expect(message).toContain("@acme/plugin.seo"); + expect(message).toContain("acme_plugin_seo"); + expect(message).toContain("acme-plugin-seo"); + expect( + (caught as { logContext?: { reason?: string } })?.logContext?.reason + ).toBe("duplicate-admin-slug"); + }); + + /** + * The same package listed twice is the same collision as far as addressing + * goes, and it is the likelier mistake — a plugin registered by both the app + * and a preset. It must not be waved through as "identical, so harmless". + */ + it("rejects the exact same name registered twice", () => { + expect(collisionReason(["@acme/one", "@acme/one"])).toBe( + "duplicate-admin-slug" + ); + }); +}); + +/** + * The seam, as distinct from the boot check. + * + * `buildPluginAdminMeta` is the one place in core that turns a plugin list + * into addresses — the slug it derives is the admin's URL for that plugin and + * the key its host override is read by. Two paths reach it without the boot + * check having run on the list it receives: `createDynamicHandlers` + * initializes services lazily and serves the public admin-meta endpoint first, + * and a `setup` transformer can rewrite `config.plugins` after + * `resolvePlugins` has already validated the original. + */ +describe("buildPluginAdminMeta", () => { + it("addresses plugins whose slugs differ", () => { + const meta = buildPluginAdminMeta( + [plugin("@acme/one"), plugin("@acme/two")], + undefined + ); + + // The positive control: it really does produce metadata for both, so the + // rejection below is about the collision rather than about a function that + // refuses everything. + expect(meta.map(m => m.name)).toEqual(["@acme/one", "@acme/two"]); + }); + + it("refuses to address two plugins that share a slug", () => { + let reason: string | undefined; + try { + buildPluginAdminMeta( + [plugin("@acme/plugin-seo"), plugin("acme_plugin_seo")], + undefined + ); + } catch (error) { + reason = (error as { logContext?: { reason?: string } }).logContext + ?.reason; + } + + expect(reason).toBe("duplicate-admin-slug"); + }); +}); diff --git a/packages/nextly/src/plugins/validate-slugs.ts b/packages/nextly/src/plugins/validate-slugs.ts new file mode 100644 index 0000000000..60ba7d1342 --- /dev/null +++ b/packages/nextly/src/plugins/validate-slugs.ts @@ -0,0 +1,52 @@ +import type { PluginDefinition } from "./plugin-context"; +import { pluginAdminSlug } from "./plugin-slug"; +import { resolutionError } from "./resolution-error"; + +/** + * Boot-check that no two plugins address the same admin slug. Throws + * fail-fast. + * + * `pluginAdminSlug` is deliberately lossy — it lowercases and collapses every + * non-alphanumeric run to one dash — so distinct package names routinely map + * to one slug: `@acme/plugin-seo`, `@acme/plugin.seo` and `ACME_Plugin_SEO` + * are all `acme-plugin-seo`. That slug is the plugin's address: the admin + * builds `/admin/plugins/` from it, core namespaces the plugin's admin + * routes with it, and host `pluginOverrides` are looked up by it. + * + * So a collision is not a cosmetic clash. Two plugins share one address, and + * every lookup along it answers with whichever the search reaches first: one + * plugin's page opens the other's, and one plugin's overrides silently apply + * to its neighbour. + * + * The check belongs HERE rather than at any of those lookups, and that is the + * whole point. At a lookup there is nothing to observe — `find()` returns a + * plugin, and a plugin is exactly what a correct lookup returns, so no code + * downstream can tell "the right one" from "the first of two". Registration is + * the last moment at which the ambiguity is still visible as ambiguity. + * + * @module plugins/validate-slugs + */ +export function validatePluginSlugs(plugins: PluginDefinition[]): void { + const byslug = new Map(); + + for (const plugin of plugins) { + const slug = pluginAdminSlug(plugin.name); + const owner = byslug.get(slug); + + if (owner !== undefined) { + // Both names, because neither alone is actionable: the reader has to + // rename one of them, and the message is the only place the pair is + // ever stated. The slug is included because it is not obvious from + // either name which characters collapsed. + throw resolutionError( + "duplicate-admin-slug", + `Plugins "${owner}" and "${plugin.name}" both resolve to the admin ` + + `slug "${slug}", so they would share one admin address. Rename one ` + + `of them.`, + { slug, plugins: [owner, plugin.name] } + ); + } + + byslug.set(slug, plugin.name); + } +}