From 7902dfa4e43bd489b9f749d8d1fa154658e193d4 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Wed, 15 Jul 2026 22:52:37 +0800 Subject: [PATCH] fix(packages): reject duplicate package id on POST /packages with 409 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST /api/v1/packages was an upsert — creating a package whose id already exists silently overwrote its manifest (name/version/…) with no warning, a data-loss footgun surfaced in Studio package-create dogfooding. It now returns 409 Conflict when the id already exists, and 400 when the id is missing. Intentional upgrade / re-install flows opt back in with `overwrite: true` (body) or `?overwrite=true`; the registry-level same-id reinstall path is untouched. Adds 4 handlePackages tests (create / duplicate-409 / overwrite / missing-id). Co-Authored-By: Claude Opus 4.8 --- packages/runtime/src/http-dispatcher.test.ts | 104 ++++++++++++++++++- packages/runtime/src/http-dispatcher.ts | 19 ++++ 2 files changed, 122 insertions(+), 1 deletion(-) diff --git a/packages/runtime/src/http-dispatcher.test.ts b/packages/runtime/src/http-dispatcher.test.ts index cca5e5ec2c..9e8cb5a7fc 100644 --- a/packages/runtime/src/http-dispatcher.test.ts +++ b/packages/runtime/src/http-dispatcher.test.ts @@ -1119,6 +1119,103 @@ describe('HttpDispatcher', () => { expect(updatePackage).toHaveBeenCalledWith({ packageId: 'a.b', patch: { description: 'hi' } }); }); + it('POST /packages creates a new package (201) after checking the id is free', async () => { + const installPackage = vi + .fn() + .mockReturnValue({ manifest: { id: 'com.acme.new', name: 'New', version: '0.1.0' } }); + const mockRegistry = { + getPackage: vi.fn().mockReturnValue(undefined), + installPackage, + getAllPackages: vi.fn().mockReturnValue([]), + }; + (kernel as any).getService = vi.fn().mockImplementation((name: string) => { + if (name === 'objectql') return Promise.resolve({ registry: mockRegistry }); + return null; // no protocol service → fall back to registry.installPackage + }); + + const result = await dispatcher.handlePackages( + '', + 'POST', + { manifest: { id: 'com.acme.new', name: 'New', version: '0.1.0', type: 'app' } }, + {}, + { request: {} }, + ); + expect(result.response?.status).toBe(201); + expect(mockRegistry.getPackage).toHaveBeenCalledWith('com.acme.new'); + expect(installPackage).toHaveBeenCalled(); + }); + + it('POST /packages rejects a duplicate id with 409 instead of silently overwriting', async () => { + const installPackage = vi.fn(); + const mockRegistry = { + getPackage: vi.fn().mockReturnValue({ manifest: { id: 'com.acme.crm', name: 'Existing' } }), + installPackage, + getAllPackages: vi.fn().mockReturnValue([]), + }; + (kernel as any).getService = vi.fn().mockImplementation((name: string) => { + if (name === 'objectql') return Promise.resolve({ registry: mockRegistry }); + return null; + }); + + const result = await dispatcher.handlePackages( + '', + 'POST', + { manifest: { id: 'com.acme.crm', name: 'Clobber', version: '9.9.9' } }, + {}, + { request: {} }, + ); + expect(result.response?.status).toBe(409); + // The existing manifest must NOT be overwritten. + expect(installPackage).not.toHaveBeenCalled(); + }); + + it('POST /packages?overwrite=true allows intentional overwrite of an existing id', async () => { + const installPackage = vi + .fn() + .mockReturnValue({ manifest: { id: 'com.acme.crm', name: 'Upgraded', version: '2.0.0' } }); + const mockRegistry = { + getPackage: vi.fn().mockReturnValue({ manifest: { id: 'com.acme.crm' } }), + installPackage, + getAllPackages: vi.fn().mockReturnValue([]), + }; + (kernel as any).getService = vi.fn().mockImplementation((name: string) => { + if (name === 'objectql') return Promise.resolve({ registry: mockRegistry }); + return null; + }); + + const result = await dispatcher.handlePackages( + '', + 'POST', + { manifest: { id: 'com.acme.crm', name: 'Upgraded', version: '2.0.0' } }, + { overwrite: 'true' }, + { request: {} }, + ); + expect(result.response?.status).toBe(201); + expect(installPackage).toHaveBeenCalled(); + }); + + it('POST /packages rejects a missing id with 400', async () => { + const mockRegistry = { + getPackage: vi.fn(), + installPackage: vi.fn(), + getAllPackages: vi.fn().mockReturnValue([]), + }; + (kernel as any).getService = vi.fn().mockImplementation((name: string) => { + if (name === 'objectql') return Promise.resolve({ registry: mockRegistry }); + return null; + }); + + const result = await dispatcher.handlePackages( + '', + 'POST', + { manifest: { name: 'No Id' } }, + {}, + { request: {} }, + ); + expect(result.response?.status).toBe(400); + expect(mockRegistry.installPackage).not.toHaveBeenCalled(); + }); + it('PATCH /packages/:id rejects an empty patch with 400', async () => { (kernel as any).getService = vi.fn().mockImplementation((name: string) => { if (name === 'objectql') return Promise.resolve({ registry: { getAllPackages: vi.fn().mockReturnValue([]) } }); @@ -1409,7 +1506,11 @@ describe('HttpDispatcher', () => { package: { manifest: { id: 'app.demo' }, status: 'installed' }, message: 'Installed package: app.demo', }); - const mockRegistry = { installPackage: vi.fn(), getAllPackages: vi.fn().mockReturnValue([]) }; + const mockRegistry = { + installPackage: vi.fn(), + getPackage: vi.fn().mockReturnValue(undefined), + getAllPackages: vi.fn().mockReturnValue([]), + }; (kernel as any).getService = vi.fn().mockImplementation((name: string) => { if (name === 'protocol') return Promise.resolve({ installPackage }); if (name === 'objectql') return Promise.resolve({ registry: mockRegistry }); @@ -1429,6 +1530,7 @@ describe('HttpDispatcher', () => { it('falls back to registry.installPackage when the protocol lacks the method', async () => { const mockRegistry = { installPackage: vi.fn().mockReturnValue({ manifest: { id: 'app.fb' }, status: 'installed' }), + getPackage: vi.fn().mockReturnValue(undefined), getAllPackages: vi.fn().mockReturnValue([]), }; (kernel as any).getService = vi.fn().mockImplementation((name: string) => { diff --git a/packages/runtime/src/http-dispatcher.ts b/packages/runtime/src/http-dispatcher.ts index 61ed2137b6..60549d1516 100644 --- a/packages/runtime/src/http-dispatcher.ts +++ b/packages/runtime/src/http-dispatcher.ts @@ -2316,6 +2316,25 @@ export class HttpDispatcher { // registry write only when the protocol service/method is unavailable. if (parts.length === 0 && m === 'POST') { const manifest = body.manifest || body; + const pkgId = typeof manifest?.id === 'string' ? manifest.id.trim() : ''; + // A package id is mandatory — without one the install cannot be keyed. + if (!pkgId) { + return { handled: true, response: this.error('Package id is required', 400) }; + } + // Duplicate-detection: POST /packages CREATES a package. If one with + // this id already exists, silently overwriting it destroys the existing + // manifest (name/version/…) with no warning — a data-loss footgun + // surfaced in Studio package-create dogfooding. Reject with 409 Conflict + // instead. Intentional upgrade / re-install flows opt back in with + // `overwrite: true` (body) or `?overwrite=true`. + const overwrite = + body?.overwrite === true || query?.overwrite === 'true' || query?.overwrite === true; + if (!overwrite && registry.getPackage(pkgId)) { + return { + handled: true, + response: this.error(`Package '${pkgId}' already exists`, 409), + }; + } let pkg: any; const protocolSvc: any = await this.resolveService('protocol').catch(() => null); if (protocolSvc && typeof protocolSvc.installPackage === 'function') {