diff --git a/server/handlers/cms/plugins/install.ts b/server/handlers/cms/plugins/install.ts index 5b16ea358..e234bd387 100644 --- a/server/handlers/cms/plugins/install.ts +++ b/server/handlers/cms/plugins/install.ts @@ -19,7 +19,7 @@ * `runPluginLifecycleHook` and `runPluginMigrate` in `./lifecycle.ts` and * `../../../plugins/runtime`; the on-disk side lives in `./shared.ts`. */ -import { gt as semverGt, lt as semverLt } from 'semver' +import { lt as semverLt } from 'semver' import type { DbClient } from '../../../db/client' import type { AuthUser } from '../../../repositories/users' import { @@ -179,7 +179,14 @@ export async function handlePackageInstall( grantedPermissions, } - if (existing && semverGt(pluginPackage.manifest.version, existing.version)) { + // Anything still here is installed at the same version or lower, and the + // lower case already returned. So `existing` means upgrade-or-reinstall, + // and both belong on the upgrade path: it deactivates the running version + // before replacing its files, runs `migrate`, and rolls back on failure. + // The fresh path does none of that — it was written for a plugin that is + // not there, and re-uploading the same version used to fall into it, + // running `install` again on a live plugin with no rollback if it threw. + if (existing) { return await installUpgradeFromPackage({ ...ctx, existing }) } return await installFreshFromPackage(ctx) @@ -357,7 +364,14 @@ async function installUpgradeFromPackage(ctx: UpgradeContext): Promise // (they're imported inside the worker), so deleting them here doesn't // race the response write — straightforward `await rm` is safe in // both dev and production. - await removePluginVersionAssets(options.uploadsDir, pluginId, fromVersion) + // + // Except on a reinstall, where `fromVersion` and `newVersion` name the + // SAME directory: step 2 has already overwritten it with the new files, + // so removing "the old version" here would delete the plugin that was + // just installed. + if (newVersion !== fromVersion) { + await removePluginVersionAssets(options.uploadsDir, pluginId, fromVersion) + } // Re-fetch so the response carries the post-activation row (settings, // lifecycle = 'active', etc.). @@ -455,7 +469,12 @@ async function rollbackUpgrade(args: { // Drop new version assets — the upgrade didn't take. With worker // isolation, plugin server files no longer live in the host's `bun // --watch` graph; plain `await rm` is safe. - if (options.uploadsDir) { + // + // NOT when the versions match. A same-version reinstall writes into the + // directory the restored manifest still points at, so deleting "the new + // version" here would delete the only copy and leave every published page + // linking a 404. Restoring the row is the whole rollback in that case. + if (options.uploadsDir && newManifest.version !== existing.version) { await removePluginVersionAssets(options.uploadsDir, pluginId, newManifest.version) } diff --git a/src/__tests__/server/cmsPlugins.test.ts b/src/__tests__/server/cmsPlugins.test.ts index 33aa2b099..f7293f3cd 100644 --- a/src/__tests__/server/cmsPlugins.test.ts +++ b/src/__tests__/server/cmsPlugins.test.ts @@ -1514,6 +1514,69 @@ describe('CMS plugin handlers', () => { } }) + it('re-uploading the SAME version takes the upgrade path, not the fresh one', async () => { + // Uploading a rebuilt package under an unchanged version number used to + // fall through both semver guards into `installFreshFromPackage` — the + // path written for a plugin that is not installed. That skipped the + // running version's `deactivate`, ran `install` a second time on a live + // plugin, and had no rollback if it threw. + const uploadsDir = await mkdtemp(join(tmpdir(), 'instatic-reinstall-')) + const db = makeFakeDb() + const cookie = await createCookie(db) + const manifest = { + id: 'acme.reinstall', + name: 'Reinstall Demo', + version: '1.0.0', + apiVersion: 1, + permissions: ['cms.routes'], + entrypoints: { server: 'server/index.js' }, + resources: [], + adminPages: [], + } + const upload = async (body: string) => { + const form = new FormData() + form.set('file', pluginZip({ + 'plugin.json': JSON.stringify(manifest), + 'server/index.js': body, + })) + form.set('grantedPermissions', JSON.stringify(['cms.routes'])) + return await handleCmsRequest( + cmsFormRequest('http://localhost/admin/api/cms/plugins/package', form, { cookie }), + db, + { uploadsDir }, + ) + } + + try { + expect((await upload('export function activate() {}')).status).toBe(201) + + const again = await upload('export function activate() { globalThis.__v2 = true }') + expect(again.ok).toBe(true) + + // The upgrade path reports the transition; the fresh path reports none. + // Both versions are the same string here, which is the point: the route + // recognised an existing install rather than treating it as new. + const body = await again.json() as { upgrade?: { fromVersion: string; toVersion: string } } + expect(body.upgrade).toEqual({ fromVersion: '1.0.0', toVersion: '1.0.0' }) + + // Exactly one row, still at the same version — a reinstall replaces, it + // does not duplicate. + expect(db.plugins.filter((p) => p.id === 'acme.reinstall')).toHaveLength(1) + expect(db.plugins[0].version).toBe('1.0.0') + + // And the new bytes are on disk: with matching versions the upgrade's + // "drop the old version" sweep names the same directory it just wrote, + // so an unguarded sweep would delete the plugin it had installed. + const entry = await readFile( + join(uploadsDir, 'plugins/acme.reinstall/1.0.0/server/index.js'), + 'utf8', + ) + expect(entry).toContain('__v2') + } finally { + await rm(uploadsDir, { recursive: true, force: true }) + } + }) + it('rejects manifests targeting an unsupported apiVersion at the boundary', async () => { const db = makeFakeDb() const cookie = await createCookie(db) diff --git a/src/admin/pages/plugins/components/PermissionReviewSection/PermissionReviewSection.tsx b/src/admin/pages/plugins/components/PermissionReviewSection/PermissionReviewSection.tsx index 754bb33e6..79f310eac 100644 --- a/src/admin/pages/plugins/components/PermissionReviewSection/PermissionReviewSection.tsx +++ b/src/admin/pages/plugins/components/PermissionReviewSection/PermissionReviewSection.tsx @@ -29,6 +29,7 @@ * before activation is the only way the operator can make an informed * decision. */ +import { lt as semverLt } from 'semver' import { Button } from '@ui/components/Button' import { permissionDescription, @@ -111,8 +112,18 @@ export function PermissionReviewSection({ onCancel, onConfirm, }: PermissionReviewSectionProps) { - const isUpgrade = Boolean(pending.upgradeFromVersion) - const rows: PermissionDiffRow[] = isUpgrade + // Three cases, not two. `upgradeFromVersion` is now set whenever the plugin + // is already installed, so compare it to tell an upgrade from a reinstall of + // the same build — and from a downgrade, which the server refuses outright + // (install.ts) and which used to be described here as an update that would + // migrate and re-activate. + const installedVersion = pending.upgradeFromVersion + const isInstalled = Boolean(installedVersion) + const isReinstall = isInstalled && installedVersion === pending.manifest.version + const isDowngrade = + isInstalled && !isReinstall && semverLt(pending.manifest.version, installedVersion!) + const isUpgrade = isInstalled && !isReinstall && !isDowngrade + const rows: PermissionDiffRow[] = isInstalled ? computePermissionDiff( pending.manifest.permissions, pending.previouslyGrantedPermissions, @@ -141,16 +152,24 @@ export function PermissionReviewSection({ >

- {isUpgrade - ? `Update ${pending.manifest.name}` - : `Review ${pending.manifest.name}`} + {isReinstall + ? `Reinstall ${pending.manifest.name}` + : isDowngrade + ? `Cannot downgrade ${pending.manifest.name}` + : isUpgrade + ? `Update ${pending.manifest.name}` + : `Review ${pending.manifest.name}`}

- {isUpgrade - ? `Updating from ${pending.upgradeFromVersion} to ${pending.manifest.version}. Existing settings and stored data are preserved; the plugin runs its migrate hook before re-activating.` - : rows.length > 0 - ? `${pending.manifest.name} requests access before activation.` - : `${pending.manifest.name} is ready to install.`} + {isReinstall + ? `Version ${pending.manifest.version} is already installed. Reinstalling replaces its files in place and re-runs the plugin's lifecycle; settings and stored data are preserved.` + : isDowngrade + ? `${pending.manifest.name} is installed at ${installedVersion}. Downgrades are refused — uninstall it first if you need to go back to ${pending.manifest.version}.` + : isUpgrade + ? `Updating from ${installedVersion} to ${pending.manifest.version}. Existing settings and stored data are preserved; the plugin runs its migrate hook before re-activating.` + : rows.length > 0 + ? `${pending.manifest.name} requests access before activation.` + : `${pending.manifest.name} is ready to install.`}

@@ -273,19 +292,28 @@ export function PermissionReviewSection({ diff --git a/src/admin/pages/plugins/hooks/usePluginsWorkspace.ts b/src/admin/pages/plugins/hooks/usePluginsWorkspace.ts index e22d75cba..16246f67d 100644 --- a/src/admin/pages/plugins/hooks/usePluginsWorkspace.ts +++ b/src/admin/pages/plugins/hooks/usePluginsWorkspace.ts @@ -302,12 +302,17 @@ export function usePluginsWorkspace(): PluginsWorkspaceVM { ? await inspectCmsPluginPackage(file) : parsePluginManifest(JSON.parse(await file.text())) - // Detect upgrade vs. fresh install client-side so we can render the - // right copy in the confirmation dialog. The server detects upgrades - // independently — this is purely a UX hint. + // Detect upgrade vs. reinstall vs. fresh install client-side so the + // confirmation dialog can say which one is about to happen. The server + // decides independently — this is purely the copy. + // + // The installed version is carried whenever one exists, INCLUDING when + // it equals the incoming one. It used to be dropped in that case, so + // re-uploading the same build rendered the first-install screen: no + // mention of the plugin already being there, and every permission + // badged "new" even though the operator had approved them all. const existing = payload.plugins.find((p) => p.id === manifest.id) - const upgradeFromVersion = - existing && existing.version !== manifest.version ? existing.version : undefined + const upgradeFromVersion = existing ? existing.version : undefined const previouslyGrantedPermissions = existing ? existing.grantedPermissions : undefined