From 5014ad20a6ee8ff42075138ec4a75371c6fa14e1 Mon Sep 17 00:00:00 2001 From: Anso Date: Sat, 8 Aug 2026 17:33:56 -0400 Subject: [PATCH] fix(nodes): return conflict on duplicate rename (#1801) * fix(nodes): return conflict on duplicate rename PUT /api/nodes/:id left name uniqueness to the SQLite UNIQUE constraint, so a colliding rename surfaced the raw database error as a 500. Add a pre-flight shape guard and collision check (409, matching POST /) plus a catch remap for the concurrent-rename race. Tests cover collision, same-name, case-sensitivity, blank/null, partial updates, and the remap path. * test(nodes): drop unused variable in case-sensitivity test --- backend/src/__tests__/nodes.test.ts | 119 ++++++++++++++++++++++++++++ backend/src/routes/nodes.ts | 18 +++++ 2 files changed, 137 insertions(+) diff --git a/backend/src/__tests__/nodes.test.ts b/backend/src/__tests__/nodes.test.ts index d85603b3..f22d46f2 100644 --- a/backend/src/__tests__/nodes.test.ts +++ b/backend/src/__tests__/nodes.test.ts @@ -223,6 +223,125 @@ describe('PUT /api/nodes/:id preserves the token unless a new one is supplied (H }); }); +describe('PUT /api/nodes/:id name collision', () => { + async function createRemoteWithName(name: string): Promise { + const res = await request(app) + .post('/api/nodes') + .set('Authorization', authHeader) + .send({ + name, + type: 'remote', + mode: 'proxy', + api_url: 'http://192.168.1.77:1852', + api_token: 'tok-name-collision', + compose_dir: '/app/compose', + }); + expect(res.status).toBe(200); + return res.body.id as number; + } + + it('rejects a colliding rename with 409 and leaves the row untouched', async () => { + const aId = await createRemoteWithName(`collide-a-${Date.now()}`); + const bId = await createRemoteWithName(`collide-b-${Date.now()}`); + const bName = DatabaseService.getInstance().getNode(bId)!.name; + const aBefore = DatabaseService.getInstance().getNode(aId)!; + + const res = await request(app) + .put(`/api/nodes/${aId}`) + .set('Authorization', authHeader) + .send({ name: bName }); + + expect(res.status).toBe(409); + expect(res.body.error).toBe('A node with that name already exists'); + + const aAfter = DatabaseService.getInstance().getNode(aId)!; + expect(aAfter.name).toBe(aBefore.name); + expect(aAfter.compose_dir).toBe(aBefore.compose_dir); + expect(aAfter.api_url).toBe(aBefore.api_url); + }); + + it('allows renaming a node to its own current name', async () => { + const id = await createRemoteWithName(`same-name-${Date.now()}`); + const name = DatabaseService.getInstance().getNode(id)!.name; + const res = await request(app) + .put(`/api/nodes/${id}`) + .set('Authorization', authHeader) + .send({ name }); + expect(res.status).toBe(200); + }); + + it('treats names that differ only by case as distinct', async () => { + const bName = `case-prod-${Date.now()}`; + await createRemoteWithName(bName); + const aId = await createRemoteWithName(`other-${Date.now()}`); + const res = await request(app) + .put(`/api/nodes/${aId}`) + .set('Authorization', authHeader) + .send({ name: bName.toUpperCase() }); + expect(res.status).toBe(200); + }); + + it('rejects a blank name with 400', async () => { + const id = await createRemoteWithName(`blank-name-${Date.now()}`); + const res = await request(app) + .put(`/api/nodes/${id}`) + .set('Authorization', authHeader) + .send({ name: '' }); + expect(res.status).toBe(400); + expect(res.body.error).toBe('Node name is required'); + }); + + it('rejects a null name with 400 instead of a raw SQLite 500', async () => { + const id = await createRemoteWithName(`null-name-${Date.now()}`); + const res = await request(app) + .put(`/api/nodes/${id}`) + .set('Authorization', authHeader) + .send({ name: null }); + expect(res.status).toBe(400); + expect(res.body.error).toBe('Node name is required'); + }); + + it('allows saving a whitespace-only name unchanged', async () => { + // POST accepts whitespace-only names (truthiness check), so a legacy node + // may carry one; the full-form echo on Save must not 400 it. + const id = await createRemoteWithName(' '); + const save = await request(app) + .put(`/api/nodes/${id}`) + .set('Authorization', authHeader) + .send({ name: ' ' }); + expect(save.status).toBe(200); + }); + + it('still updates other fields when name is absent', async () => { + const id = await createRemoteWithName(`partial-update-${Date.now()}`); + const res = await request(app) + .put(`/api/nodes/${id}`) + .set('Authorization', authHeader) + .send({ compose_dir: '/tmp/renamed-compose' }); + expect(res.status).toBe(200); + const node = DatabaseService.getInstance().getNode(id)!; + expect(node.compose_dir).toBe('/tmp/renamed-compose'); + expect(node.name).toMatch(/^partial-update-/); + }); + + it('remaps a UNIQUE constraint from the DB to a clean 409', async () => { + const id = await createRemoteWithName(`unique-race-${Date.now()}`); + const uniqueErr = Object.assign(new Error('UNIQUE constraint failed: nodes.name'), { code: 'SQLITE_CONSTRAINT_UNIQUE' }); + const spy = vi.spyOn(DatabaseService.getInstance(), 'updateNode').mockImplementation(() => { throw uniqueErr; }); + try { + const res = await request(app) + .put(`/api/nodes/${id}`) + .set('Authorization', authHeader) + .send({ name: 'race-rename' }); + expect(res.status).toBe(409); + expect(res.body.error).toBe('A node with that name already exists'); + expect(res.body.error).not.toMatch(/UNIQUE/); + } finally { + spy.mockRestore(); + } + }); +}); + describe('POST /api/nodes/:id/test authorization (H-3)', () => { it('403s a non-admin (viewer) with PERMISSION_DENIED', async () => { const id = await createRemoteNode(authHeader); diff --git a/backend/src/routes/nodes.ts b/backend/src/routes/nodes.ts index 61b9a2cb..74798101 100644 --- a/backend/src/routes/nodes.ts +++ b/backend/src/routes/nodes.ts @@ -322,6 +322,21 @@ nodesRouter.put('/:id', async (req: Request, res: Response) => { return res.status(404).json({ error: 'Node not found' }); } + // updateNode writes any non-undefined name verbatim against a NOT NULL + // UNIQUE column, so validate shape here with the same message as POST / + // (stricter on blank names; the stored value is not trimmed, matching + // create). An unchanged name is exempt so a node whose stored name + // predates this validation can still be saved as-is. + const nameChanged = updates.name !== undefined && updates.name !== existingNode.name; + if (nameChanged && (typeof updates.name !== 'string' || updates.name.trim() === '')) { + return res.status(400).json({ error: 'Node name is required' }); + } + // Best-effort pre-check; the catch below remaps a racing UNIQUE violation + // to the same 409. + if (nameChanged && DatabaseService.getInstance().getNodes().some((n) => n.name === updates.name)) { + return res.status(409).json({ error: 'A node with that name already exists' }); + } + if (existingNode.mode === 'pilot_agent' && updates.compose_dir !== undefined) { const composeDir = normalizePilotComposeDir(updates.compose_dir); if (!composeDir) { @@ -378,6 +393,9 @@ nodesRouter.put('/:id', async (req: Request, res: Response) => { if (message.includes('Node type cannot be changed')) { return res.status(400).json({ error: message }); } + if (message.includes('UNIQUE constraint')) { + return res.status(409).json({ error: 'A node with that name already exists' }); + } console.error('Failed to update node:', error); res.status(500).json({ error: message || 'Failed to update node' }); }