From 02e5ede0846f164afb150559441fe5cbd4af0ac4 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 23 Jul 2026 11:17:11 +0000 Subject: [PATCH] fix(blueprints): fail closed on marker ownership for apply and withdraw Require a matching .blueprint.json before withdraw or in-lock apply overwrite, treat already-absent stacks as withdrawn, and block compose/marker file writes while a stack lifecycle lock is held so confirmed applies cannot deploy editor races or hijack unmanaged directories. Co-authored-by: Anso --- .../blueprints-compose-apply.test.ts | 30 ++++ .../__tests__/blueprints-edge-cases.test.ts | 41 +++++- .../blueprints-preview-apply.test.ts | 1 + .../blueprints-remote-deploy.test.ts | 70 ++++++++- backend/src/__tests__/blueprints.test.ts | 12 +- backend/src/routes/blueprints.ts | 11 +- backend/src/routes/stacks.ts | 38 +++++ backend/src/services/BlueprintService.ts | 133 +++++++++++++++--- backend/src/services/FileSystemService.ts | 2 + .../services/blueprintPreviewProjection.ts | 10 +- 10 files changed, 319 insertions(+), 29 deletions(-) diff --git a/backend/src/__tests__/blueprints-compose-apply.test.ts b/backend/src/__tests__/blueprints-compose-apply.test.ts index 7e8d6b21..5312b5b8 100644 --- a/backend/src/__tests__/blueprints-compose-apply.test.ts +++ b/backend/src/__tests__/blueprints-compose-apply.test.ts @@ -104,6 +104,10 @@ describe('Blueprint compose apply (real filesystem)', () => { await fsPromises.writeFile(path.join(stackDir, 'compose.yml'), 'services:\n a:\n image: a\n'); await fsPromises.writeFile(path.join(stackDir, 'docker-compose.yaml'), 'services:\n b:\n image: b\n'); await fsPromises.writeFile(path.join(stackDir, 'docker-compose.yml'), 'services:\n c:\n image: c\n'); + await fsPromises.writeFile( + path.join(stackDir, '.blueprint.json'), + JSON.stringify({ blueprintId: 2, revision: 2, lastApplied: 1 }, null, 2), + ); const composeContent = 'services:\n app:\n image: redis:7\n'; const markerContent = JSON.stringify({ blueprintId: 2, revision: 3, lastApplied: Date.now() }, null, 2); @@ -133,6 +137,32 @@ describe('Blueprint compose apply (real filesystem)', () => { await expectMissing(path.join(stackDir, 'docker-compose.yml')); expect(deploySpy).toHaveBeenCalledTimes(1); }); + + it('refuses to overwrite an existing unmanaged stack directory inside the lock', async () => { + const { BlueprintNameConflictError } = await import('../services/BlueprintService'); + const nodeId = seedLocalNode(); + const stackName = `bp-hijack-${counter}`; + const stackDir = path.join(process.env.COMPOSE_DIR!, stackName); + await fsPromises.mkdir(stackDir, { recursive: true }); + const original = 'services:\n mine:\n image: nginx:alpine\n'; + await fsPromises.writeFile(path.join(stackDir, 'compose.yaml'), original); + + const deploySpy = vi.spyOn(ComposeService.prototype, 'deployStack').mockResolvedValue(undefined); + + await expect( + BlueprintService.getInstance().applyLocalUnderLock( + nodeId, + stackName, + 'services:\n bp:\n image: redis:7\n', + JSON.stringify({ blueprintId: 99, revision: 1, lastApplied: Date.now() }, null, 2), + '/api/blueprints/test/apply', + ), + ).rejects.toBeInstanceOf(BlueprintNameConflictError); + + expect(await fsPromises.readFile(path.join(stackDir, 'compose.yaml'), 'utf-8')).toBe(original); + await expectMissing(path.join(stackDir, '.blueprint.json')); + expect(deploySpy).not.toHaveBeenCalled(); + }); }); describe('FileSystemService.removeAlternateRootComposeFiles', () => { diff --git a/backend/src/__tests__/blueprints-edge-cases.test.ts b/backend/src/__tests__/blueprints-edge-cases.test.ts index f7beba38..45278831 100644 --- a/backend/src/__tests__/blueprints-edge-cases.test.ts +++ b/backend/src/__tests__/blueprints-edge-cases.test.ts @@ -111,9 +111,9 @@ describe('Blueprint route edge cases', () => { describe('Blueprint delete guard', () => { // Rows with no stack of ours on the node must not block delete, AND the route must not run the - // withdraw primitive for them: withdrawFromNode proceeds on a missing marker and would - // down/delete a same-name stack Sencho never owned. A name_conflict is exactly that unmanaged - // stack, so it is excluded even though it carries a last_deployed_at timestamp. + // withdraw primitive for them. withdrawFromNode now refuses a missing marker on a present + // stack, but skipping these rows still avoids a useless conflict path. A name_conflict is + // exactly that unmanaged stack, so it is excluded even though it may carry last_deployed_at. it.each([ { label: 'never-deployed pending review', status: 'pending_state_review' as const, last_deployed_at: null }, { label: 'first-deploy failure', status: 'failed' as const, last_deployed_at: null }, @@ -271,6 +271,41 @@ describe('BlueprintService marker edge cases', () => { expect(dep).toBeDefined(); expect(dep?.status).toBe('name_conflict'); }); + + it('refuses to withdraw when the marker is missing but the local stack directory still exists', async () => { + const fs = await import('fs'); + const path = await import('path'); + const composeDir = process.env.COMPOSE_DIR!; + const localNode = DatabaseService.getInstance().getNodes()[0]; + DatabaseService.getInstance().getDb() + .prepare('UPDATE nodes SET compose_dir = ? WHERE id = ?') + .run(composeDir, localNode.id); + const refreshed = DatabaseService.getInstance().getNode(localNode.id)!; + const bp = seedBlueprint([refreshed.id]); + const bpObj = DatabaseService.getInstance().getBlueprint(bp.id)!; + DatabaseService.getInstance().upsertDeployment({ + blueprint_id: bp.id, + node_id: refreshed.id, + status: 'active', + applied_revision: bpObj.revision, + last_deployed_at: Date.now(), + }); + + const stackDir = path.join(composeDir, bpObj.name); + fs.mkdirSync(stackDir, { recursive: true }); + fs.writeFileSync(path.join(stackDir, 'compose.yaml'), 'services:\n app:\n image: nginx\n'); + + vi.spyOn(BlueprintService.getInstance(), 'readMarker').mockResolvedValue(null); + const { DeployedStackDeletionService } = await import('../services/DeployedStackDeletionService'); + const deleteSpy = vi.spyOn(DeployedStackDeletionService.getInstance(), 'deleteDeployedStack'); + + const result = await BlueprintService.getInstance().withdrawFromNode(bpObj, refreshed); + + expect(result.status).toBe('name_conflict'); + expect(deleteSpy).not.toHaveBeenCalled(); + expect(DatabaseService.getInstance().getDeployment(bp.id, refreshed.id)?.status).toBe('name_conflict'); + expect(fs.existsSync(path.join(stackDir, 'compose.yaml'))).toBe(true); + }); }); describe('BlueprintService developer-mode diagnostics', () => { diff --git a/backend/src/__tests__/blueprints-preview-apply.test.ts b/backend/src/__tests__/blueprints-preview-apply.test.ts index d466b367..38f638c3 100644 --- a/backend/src/__tests__/blueprints-preview-apply.test.ts +++ b/backend/src/__tests__/blueprints-preview-apply.test.ts @@ -326,6 +326,7 @@ describe('POST /api/blueprints/:id/apply confirm binding', () => { const conflict = await BlueprintService.getInstance().hasNameConflict( created.body.name as string, DatabaseService.getInstance().getNode(node.id)!, + created.body.id as number, ); expect(conflict).toBe(true); diff --git a/backend/src/__tests__/blueprints-remote-deploy.test.ts b/backend/src/__tests__/blueprints-remote-deploy.test.ts index d9a145b7..25bcc6cb 100644 --- a/backend/src/__tests__/blueprints-remote-deploy.test.ts +++ b/backend/src/__tests__/blueprints-remote-deploy.test.ts @@ -204,7 +204,15 @@ describe('BlueprintService remote deploy', () => { applied_revision: bpObj.revision, }); - vi.spyOn(axios, 'get').mockResolvedValue({ status: 404, data: {} }); // readMarker → null → proceed + const marker = { + blueprintId: bp.id, + revision: bpObj.revision, + lastApplied: Date.now(), + }; + vi.spyOn(axios, 'get').mockResolvedValue({ + status: 200, + data: { content: JSON.stringify(marker) }, + }); vi.spyOn(axios, 'post').mockResolvedValue({ status: 200, data: {} }); // remote down (best-effort) const delSpy = vi.spyOn(axios, 'delete').mockResolvedValue({ status: 200, data: {} }); @@ -215,6 +223,56 @@ describe('BlueprintService remote deploy', () => { expect(DatabaseService.getInstance().getDeployment(bp.id, node.id)).toBeUndefined(); }); + it('refuses remote withdraw when the marker is missing but the stack still exists', async () => { + const node = seedRemoteNode(); + const bp = seedBlueprint([node.id]); + const nodeObj = DatabaseService.getInstance().getNode(node.id)!; + const bpObj = DatabaseService.getInstance().getBlueprint(bp.id)!; + DatabaseService.getInstance().upsertDeployment({ + blueprint_id: bp.id, + node_id: node.id, + status: 'active', + applied_revision: bpObj.revision, + }); + + // Marker GET 404, then stack list shows the same-name stack still present. + vi.spyOn(axios, 'get') + .mockResolvedValueOnce({ status: 404, data: {} }) + .mockResolvedValueOnce({ status: 200, data: [{ name: bpObj.name }] }); + const delSpy = vi.spyOn(axios, 'delete'); + + const result = await BlueprintService.getInstance().withdrawFromNode(bpObj, nodeObj); + + expect(result.status).toBe('name_conflict'); + expect(delSpy).not.toHaveBeenCalled(); + expect(DatabaseService.getInstance().getDeployment(bp.id, node.id)?.status).toBe('name_conflict'); + }); + + it('clears a remote deployment row when the stack is already absent (no marker)', async () => { + const node = seedRemoteNode(); + const bp = seedBlueprint([node.id]); + const nodeObj = DatabaseService.getInstance().getNode(node.id)!; + const bpObj = DatabaseService.getInstance().getBlueprint(bp.id)!; + DatabaseService.getInstance().upsertDeployment({ + blueprint_id: bp.id, + node_id: node.id, + status: 'active', + applied_revision: bpObj.revision, + }); + + // Marker missing and stack list empty → already gone. + vi.spyOn(axios, 'get') + .mockResolvedValueOnce({ status: 404, data: {} }) + .mockResolvedValueOnce({ status: 200, data: [] }); + const delSpy = vi.spyOn(axios, 'delete'); + + const result = await BlueprintService.getInstance().withdrawFromNode(bpObj, nodeObj); + + expect(result.status).toBe('withdrawn'); + expect(delSpy).not.toHaveBeenCalled(); + expect(DatabaseService.getInstance().getDeployment(bp.id, node.id)).toBeUndefined(); + }); + it('does not clear hub role assignments when withdrawing a remote deployment', async () => { const bcrypt = await import('bcrypt'); const db = DatabaseService.getInstance(); @@ -237,7 +295,15 @@ describe('BlueprintService remote deploy', () => { user_id: userId, role: 'deployer', resource_type: 'stack', resource_id: bpObj.name, }); - vi.spyOn(axios, 'get').mockResolvedValue({ status: 404, data: {} }); + const marker = { + blueprintId: bp.id, + revision: bpObj.revision, + lastApplied: Date.now(), + }; + vi.spyOn(axios, 'get').mockResolvedValue({ + status: 200, + data: { content: JSON.stringify(marker) }, + }); vi.spyOn(axios, 'post').mockResolvedValue({ status: 200, data: {} }); const delSpy = vi.spyOn(axios, 'delete').mockResolvedValue({ status: 200, data: {} }); const rbacSpy = vi.spyOn(db, 'deleteRoleAssignmentsByResource'); diff --git a/backend/src/__tests__/blueprints.test.ts b/backend/src/__tests__/blueprints.test.ts index 935230af..926bf869 100644 --- a/backend/src/__tests__/blueprints.test.ts +++ b/backend/src/__tests__/blueprints.test.ts @@ -456,6 +456,12 @@ describe('BlueprintService per-stack lock', () => { const nodeId = seedNode(); const bp = seedBlueprint({ classification: 'stateless', nodeIds: [nodeId] }); const node = DatabaseService.getInstance().getNode(nodeId)!; + // Matching marker required before the delete lock is attempted. + vi.spyOn(BlueprintService.getInstance(), 'readMarker').mockResolvedValue({ + blueprintId: bp.id, + revision: bp.revision, + lastApplied: Date.now(), + }); // A manual operation holds the lock; the withdraw must not race it. StackOpLockService.getInstance().tryAcquire(nodeId, bp.name, 'update', 'admin'); @@ -499,7 +505,11 @@ describe('BlueprintService local withdraw clears stack-scoped role assignments', user_id: userId, role: 'deployer', resource_type: 'node', resource_id: String(otherNodeId), }); - vi.spyOn(BlueprintService.getInstance(), 'readMarker').mockResolvedValue(null); + vi.spyOn(BlueprintService.getInstance(), 'readMarker').mockResolvedValue({ + blueprintId: bp.id, + revision: bp.revision, + lastApplied: Date.now(), + }); const { ComposeService } = await import('../services/ComposeService'); const { FileSystemService } = await import('../services/FileSystemService'); vi.spyOn(ComposeService.prototype, 'downStack').mockResolvedValue(undefined); diff --git a/backend/src/routes/blueprints.ts b/backend/src/routes/blueprints.ts index ecdef6db..3d62a376 100644 --- a/backend/src/routes/blueprints.ts +++ b/backend/src/routes/blueprints.ts @@ -7,7 +7,7 @@ import { type BlueprintSelector, type DriftMode, } from '../services/DatabaseService'; -import { BlueprintService } from '../services/BlueprintService'; +import { BlueprintService, BlueprintNameConflictError } from '../services/BlueprintService'; import { BlueprintReconciler, messageForConfirmedOutcomes, @@ -320,8 +320,9 @@ blueprintsRouter.delete('/:id', async (req: Request, res: Response): Promise { + async hasNameConflict(blueprintName: string, node: Node, blueprintId: number): Promise { try { if (node.type === 'local') { const baseDir = NodeRegistry.getInstance().getComposeDir(node.id); @@ -170,13 +179,8 @@ export class BlueprintService { } catch { return false; // directory doesn't exist → no conflict } - const markerPath = path.join(stackDir, MARKER_FILENAME); - try { - await fsPromises.stat(markerPath); - return false; // marker present → ours - } catch { - return true; // directory exists but no marker → conflict - } + const marker = await this.readLocalMarkerFromDisk(node.id, blueprintName); + return marker == null || marker.blueprintId !== blueprintId; } const target = NodeRegistry.getInstance().getProxyTarget(node.id); if (!target) return false; @@ -192,12 +196,54 @@ export class BlueprintService { const exists = stacks.some(s => s?.name === blueprintName); if (!exists) return false; const marker = await this.readMarker(blueprintName, node); - return marker == null; + return marker == null || marker.blueprintId !== blueprintId; } catch { return false; } } + /** Read and parse a local on-disk marker without going through the remote HTTP path. */ + private async readLocalMarkerFromDisk(nodeId: number, stackName: string): Promise { + try { + const baseDir = NodeRegistry.getInstance().getComposeDir(nodeId); + const markerPath = path.resolve(baseDir, stackName, MARKER_FILENAME); + if (!markerPath.startsWith(path.resolve(baseDir))) return null; + const content = await fsPromises.readFile(markerPath, 'utf-8'); + return BlueprintService.parseMarker(content); + } catch { + return null; + } + } + + /** + * Probe whether a stack directory/name still exists. Used by withdraw to + * distinguish "already gone" (safe to clear the deployment row) from + * "present without our marker" (must refuse). + */ + private async probeStackExistence( + blueprintName: string, + node: Node, + ): Promise<'present' | 'absent' | 'unknown'> { + try { + if (node.type === 'local') { + return (await this.stackDirExists(node.id, blueprintName)) ? 'present' : 'absent'; + } + const target = NodeRegistry.getInstance().getProxyTarget(node.id); + if (!target) return 'unknown'; + const baseUrl = target.apiUrl.replace(/\/$/, ''); + const listRes = await axios.get(`${baseUrl}/api/stacks`, { + headers: this.remoteHeaders(target.apiToken), + timeout: REMOTE_HTTP_TIMEOUT_MS, + validateStatus: () => true, + }); + if (listRes.status !== 200) return 'unknown'; + const stacks = Array.isArray(listRes.data) ? listRes.data as Array<{ name?: string }> : []; + return stacks.some(s => s?.name === blueprintName) ? 'present' : 'absent'; + } catch { + return 'unknown'; + } + } + /** * Deploy this blueprint to the given target node. Caller must have already * resolved that the target should receive this blueprint (selector match @@ -222,7 +268,7 @@ export class BlueprintService { }); try { this.setStatus(blueprint.id, node.id, 'deploying'); - if (await this.hasNameConflict(blueprint.name, node)) { + if (await this.hasNameConflict(blueprint.name, node, blueprint.id)) { this.setStatus(blueprint.id, node.id, 'name_conflict', { last_error: `A stack named "${blueprint.name}" already exists on this node and is not managed by Sencho.`, }); @@ -249,6 +295,14 @@ export class BlueprintService { sanitizeForLog(blueprint.name), node.id, Date.now() - started); return { status: 'active' }; } catch (err) { + if (err instanceof BlueprintNameConflictError) { + this.setStatus(blueprint.id, node.id, 'name_conflict', { + last_error: err.message, + }); + console.warn('[BlueprintService] deploy name conflict blueprint=%s node=%s durationMs=%s', + sanitizeForLog(blueprint.name), node.id, Date.now() - started); + return { status: 'name_conflict', error: 'name_conflict' }; + } const message = BlueprintService.formatError(err); this.setStatus(blueprint.id, node.id, 'failed', { last_error: message }); console.error('[BlueprintService] deploy failed blueprint=%s node=%s durationMs=%s error=%s', @@ -280,11 +334,31 @@ export class BlueprintService { }); try { this.setStatus(blueprint.id, node.id, 'withdrawing'); - // Refuse to withdraw a directory we do not own + // Marker is the trust root: only withdraw when it matches this blueprint. + // Missing/unreadable marker on a still-present stack is refuse (not delete). + // Missing stack entirely is treated as already withdrawn. const marker = await this.readMarker(blueprint.name, node); - if (marker && marker.blueprintId !== blueprint.id) { + if (!marker || marker.blueprintId !== blueprint.id) { + if (marker && marker.blueprintId !== blueprint.id) { + this.setStatus(blueprint.id, node.id, 'name_conflict', { + last_error: `Marker on this node points to a different blueprint (id=${marker.blueprintId}); refusing to withdraw.`, + }); + return { status: 'name_conflict' }; + } + const existence = await this.probeStackExistence(blueprint.name, node); + if (existence === 'absent') { + DatabaseService.getInstance().deleteDeployment(blueprint.id, node.id); + console.info('[BlueprintService] withdraw noop (stack already absent) blueprint=%s node=%s durationMs=%s', + sanitizeForLog(blueprint.name), node.id, Date.now() - started); + return { status: 'withdrawn' }; + } + if (existence === 'unknown') { + const message = 'Could not verify blueprint marker before withdraw; refusing to delete.'; + this.setStatus(blueprint.id, node.id, 'failed', { last_error: message }); + return { status: 'failed', error: message }; + } this.setStatus(blueprint.id, node.id, 'name_conflict', { - last_error: `Marker on this node points to a different blueprint (id=${marker.blueprintId}); refusing to withdraw.`, + last_error: `Stack "${blueprint.name}" exists without a matching blueprint marker; refusing to withdraw.`, }); return { status: 'name_conflict' }; } @@ -431,6 +505,10 @@ export class BlueprintService { * on a remote node receiving a blueprint apply from its hub (so the file * writes hold the remote's lock, not just the deploy). On lock conflict * nothing is written and { ran: false } is returned. + * + * Ownership is re-validated inside the lock: an existing directory is only + * overwritten when its marker matches the marker being applied. A missing or + * foreign marker throws BlueprintNameConflictError without writing. */ async applyLocalUnderLock( nodeId: number, @@ -439,11 +517,22 @@ export class BlueprintService { markerContent: string, auditPath: string, ): Promise<{ ran: true } | { ran: false; existingAction: StackOpAction }> { + const expected = BlueprintService.parseMarker(markerContent); + if (!expected) { + throw new Error('Invalid blueprint marker'); + } const fs = FileSystemService.getInstance(nodeId); const lock = await StackOpLockService.getInstance().runExclusive( nodeId, stackName, 'deploy', 'system', async () => { - if (!(await this.stackDirExists(nodeId, stackName))) { + if (await this.stackDirExists(nodeId, stackName)) { + const existing = await this.readLocalMarkerFromDisk(nodeId, stackName); + if (!existing || existing.blueprintId !== expected.blueprintId) { + throw new BlueprintNameConflictError( + `A stack named "${stackName}" already exists on this node and is not managed by this blueprint.`, + ); + } + } else { await fs.createStack(stackName); } await fs.writeStackFile(stackName, COMPOSE_FILENAME, composeContent); @@ -519,6 +608,16 @@ export class BlueprintService { return; } if (res.status === 409) { + const body = res.data; + const code = body && typeof body === 'object' && typeof (body as { code?: unknown }).code === 'string' + ? (body as { code: string }).code + : ''; + if (code === 'name_conflict') { + throw new BlueprintNameConflictError( + BlueprintService.extractApiError(res.data) || + `A stack named "${blueprint.name}" already exists on this node and is not managed by this blueprint.`, + ); + } throw new Error(`blueprint apply skipped: ${BlueprintService.extractApiError(res.data) || 'another operation is already in progress'}`); } if (res.status >= 400) { diff --git a/backend/src/services/FileSystemService.ts b/backend/src/services/FileSystemService.ts index f0e038b1..6ac1266f 100644 --- a/backend/src/services/FileSystemService.ts +++ b/backend/src/services/FileSystemService.ts @@ -56,6 +56,8 @@ const PROTECTED_STACK_FILES = new Set([ 'docker-compose.yaml', 'docker-compose.yml', '.env', + // Blueprint ownership trust root — must not be deleted via the file explorer. + '.blueprint.json', ]); // Bookkeeping markers Sencho writes into the backup slot. They are never copied diff --git a/backend/src/services/blueprintPreviewProjection.ts b/backend/src/services/blueprintPreviewProjection.ts index f32e2c71..69838b98 100644 --- a/backend/src/services/blueprintPreviewProjection.ts +++ b/backend/src/services/blueprintPreviewProjection.ts @@ -457,12 +457,16 @@ function asApprovedBlueprint(blueprint: Blueprint): Blueprint & BlueprintApprova } /** Upgrade create rows to blockers when an unmanaged same-name stack already exists. */ -async function applyCreateNameConflictBlockers(blueprintName: string, raw: RawAction[]): Promise { +async function applyCreateNameConflictBlockers( + blueprintName: string, + blueprintId: number, + raw: RawAction[], +): Promise { const { BlueprintService } = await import('./BlueprintService'); const svc = BlueprintService.getInstance(); for (const row of raw) { if (row.action !== 'create') continue; - if (!(await svc.hasNameConflict(blueprintName, row.node))) continue; + if (!(await svc.hasNameConflict(blueprintName, row.node, blueprintId))) continue; row.action = 'blocked_name_conflict'; row.severity = 'blocker'; row.detail = 'Unmanaged stack with this name already exists on this node'; @@ -477,7 +481,7 @@ export async function buildBlueprintPreview(blueprintId: number): Promise