From fcecc67fd527818ed4abb9f2ec0e6f1c1f0e812d Mon Sep 17 00:00:00 2001 From: Mostafa Sadeghi <205455727+mostafasadeghidev@users.noreply.github.com> Date: Sat, 5 Sep 2026 13:44:43 +0200 Subject: [PATCH] fix(plugins): treat a same-version upload as a reinstall, not a first install MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-uploading a plugin at an unchanged version number fell through both semver guards — `semverGt` false, `semverLt` false — and landed in `installFreshFromPackage`, the path written for a plugin that is not there. On a live install that meant: the running version's `deactivate` never ran; `install` ran a second time on a plugin that was already installed; `migrate` was skipped; there was no rollback if a hook threw, so a failure returned 201 with the plugin parked in `error` and its assets already overwritten; and the audit event said `plugin.install`, recording a replacement as a first-time install. Rebuilding a package without bumping the version is the normal inner loop of plugin development, so this is the path a developer hits most. Since a lower version already returned above, anything reaching the branch is installed at the same version or higher — so `existing` alone now selects the upgrade path. That surfaced two places where "old version" and "new version" are the same directory, and both would have deleted a working plugin: - Step 6 drops the old version's assets after a successful upgrade. With equal versions that names the directory step 2 has just written. - `rollbackUpgrade` drops the new version's assets on failure. With equal versions that is the only copy, and the restored row still points at it. Both are now guarded on the versions actually differing. The review dialog told the operator none of this: it only set `upgradeFromVersion` when the versions differed, so a reinstall rendered the first-install screen — no mention of the plugin already being there, every permission badged "new" though all were already approved. It now carries the installed version whenever there is one and distinguishes three cases: reinstall, update, and downgrade. Downgrade previously promised "settings and stored data are preserved; the plugin runs its migrate hook" and was then refused by the server; it now says so before the operator commits, and the confirm button is disabled rather than buying them a round trip. Co-Authored-By: Claude Opus 5 --- server/handlers/cms/plugins/install.ts | 27 ++++++-- src/__tests__/server/cmsPlugins.test.ts | 63 ++++++++++++++++++ .../PermissionReviewSection.tsx | 66 +++++++++++++------ .../plugins/hooks/usePluginsWorkspace.ts | 15 +++-- 4 files changed, 143 insertions(+), 28 deletions(-) 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