diff --git a/README.md b/README.md index 3a75175..ddb744e 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ A self-hosted, full-stack network management platform for MikroTik devices. Monitor, configure, and manage your entire MikroTik infrastructure — routers, switches, and wireless access points — from a single web interface. -![Version](https://img.shields.io/badge/version-0.17.7_Beta-blue) +![Version](https://img.shields.io/badge/version-0.18.0_Beta-blue) ![License](https://img.shields.io/badge/license-AGPLv3-blue) ![Docker](https://img.shields.io/badge/docker-compose-2496ED?logo=docker&logoColor=white) ![TypeScript](https://img.shields.io/badge/TypeScript-5.3-3178C6?logo=typescript&logoColor=white) diff --git a/backend/package.json b/backend/package.json index 837ab66..82ecbb8 100644 --- a/backend/package.json +++ b/backend/package.json @@ -1,6 +1,6 @@ { "name": "mikrotik-manager-backend", - "version": "0.17.7-beta", + "version": "0.18.0-beta", "description": "Mikrotik Network Management Platform - Backend API", "main": "dist/index.js", "scripts": { diff --git a/backend/src/db/migrate.ts b/backend/src/db/migrate.ts index d333cbd..faf112c 100644 --- a/backend/src/db/migrate.ts +++ b/backend/src/db/migrate.ts @@ -476,6 +476,28 @@ CREATE TABLE IF NOT EXISTS client_traffic_daily ( ); CREATE INDEX IF NOT EXISTS idx_client_traffic_daily_day ON client_traffic_daily(day DESC); +-- Change Guard: in-flight and historical "safe apply" operations. A pending row +-- means a restore point + auto-revert scheduler are armed on the device; the +-- manager either commits (disarms) or leaves the device to restore itself. +CREATE TABLE IF NOT EXISTS device_change_guards ( + id SERIAL PRIMARY KEY, + device_id INTEGER NOT NULL REFERENCES devices(id) ON DELETE CASCADE, + token VARCHAR(64) NOT NULL, + mode VARCHAR(10) NOT NULL DEFAULT 'binary', + restore_point VARCHAR(64), + scheduler_name VARCHAR(64), + status VARCHAR(16) NOT NULL DEFAULT 'pending', + change_kind VARCHAR(64), + change_summary TEXT, + note TEXT, + user_id INTEGER, + created_at TIMESTAMPTZ DEFAULT NOW(), + expires_at TIMESTAMPTZ, + committed_at TIMESTAMPTZ +); +CREATE INDEX IF NOT EXISTS idx_change_guards_device ON device_change_guards(device_id, created_at DESC); +CREATE INDEX IF NOT EXISTS idx_change_guards_pending ON device_change_guards(status, expires_at); + -- Wireless security profiles (WPA/WPA2/WPA3 config) CREATE TABLE IF NOT EXISTS wireless_security_profiles ( id SERIAL PRIMARY KEY, @@ -522,6 +544,9 @@ const DEFAULT_SETTINGS = [ { key: 'netflow_accept_unknown', value: true }, { key: 'netflow_retention_days', value: 30 }, { key: 'netflow_daily_retention_days', value: 365 }, + { key: 'change_guard_enabled', value: true }, + { key: 'change_guard_mode', value: 'binary' }, + { key: 'change_guard_timeout_sec', value: 120 }, ]; export async function runMigrations(): Promise { diff --git a/backend/src/index.ts b/backend/src/index.ts index 1d37471..5eaa18a 100644 --- a/backend/src/index.ts +++ b/backend/src/index.ts @@ -28,6 +28,7 @@ import { verifyToken, type AuthPayload } from './middleware/auth'; import { rateLimitRedis } from './middleware/rateLimitRedis'; import { initSecrets } from './utils/secrets'; import { reencryptStaleCredentials } from './db/reencryptCredentials'; +import { reconcileStaleGuards } from './services/changeGuard/ChangeGuard'; import { decrypt } from './utils/crypto'; import { corsMiddlewareOptions, socketIoCorsOptions } from './utils/corsOrigins'; @@ -354,6 +355,10 @@ async function start(): Promise { console.warn('[secrets] credential re-encryption sweep skipped:', (e as Error).message) ); + // Guards left 'pending' belong to a manager that stopped mid-change; the device + // has since restored itself, so settle those rows. + reconcileStaleGuards().catch(() => {}); + // Reset vendor entries that were previously set to '' due to API rate-limiting, // so they get re-resolved by the new local OUI database. await query(`UPDATE clients SET vendor = NULL WHERE vendor = ''`).catch(() => {}); diff --git a/backend/src/routes/devices.ts b/backend/src/routes/devices.ts index d4794ec..61db581 100644 --- a/backend/src/routes/devices.ts +++ b/backend/src/routes/devices.ts @@ -12,6 +12,7 @@ import { createDeviceFromBody, type CreateDeviceInput, type CreateDeviceContext import { parsePort } from '../utils/parsePort'; import { safeConnectionError } from '../utils/safeClientError'; import { detectLockoutRisk } from '../utils/firewallSafety'; +import { withSafeApply, type GuardDevice } from '../services/changeGuard/ChangeGuard'; import { redis } from '../config/redis'; import { enqueueBulkAddJob, getBulkAddJobState } from '../services/DeviceBulkAddWorker'; @@ -910,6 +911,60 @@ router.post('/:id/nat/move', requireWrite, async (req: Request, res: Response) = // ─── Firewall Address Lists ───────────────────────────────────────────────────── // Shared helper for the security-feature routes below (DRYs the device fetch + // connect/disconnect lifecycle the rest of this file uses inline). +/** + * Run a device mutation under the Change Guard safety net (see + * services/changeGuard/ChangeGuard.ts): the device saves a restore point and arms + * a self-restore before the change, and disarms it only once we prove the device + * is still reachable. Used for changes that can sever the manager's own path. + * + * The response always carries a `guard` block so the UI can say whether the change + * was confirmed, is being auto-reverted, or ran unprotected. + */ +async function withGuardedChange( + id: string, + req: Request, + res: Response, + meta: { kind: string; summary: string }, + fn: (c: DeviceCollector) => Promise, + afterConfirmed?: (c: DeviceCollector) => Promise +): Promise { + const deviceRow = await queryOne(`SELECT * FROM devices WHERE id = $1`, [id]); + if (!deviceRow) { res.status(404).json({ error: 'Device not found' }); return; } + + const collector = new DeviceCollector(deviceRow); + try { + const outcome = await withSafeApply( + deviceRow as unknown as GuardDevice, + { ...meta, userId: req.user?.userId ?? null }, + async () => { + await collector.connect(); + return fn(collector); + } + ); + + // Only re-read the device once we know it's still reachable. + if (outcome.confirmed && afterConfirmed) { + await afterConfirmed(collector).catch(() => { /* best effort */ }); + } + + res.json({ + ...(typeof outcome.result === 'object' && outcome.result !== null + ? outcome.result as Record + : { result: outcome.result }), + guard: { + protected: !outcome.unprotectedReason, + confirmed: outcome.confirmed, + auto_reverting: outcome.autoReverting, + unprotected_reason: outcome.unprotectedReason ?? null, + }, + }); + } catch (err) { + res.status(500).json({ error: (err as Error).message }); + } finally { + try { collector.disconnect(); } catch { /* device may be gone */ } + } +} + async function withCollector( id: string, res: Response, @@ -1004,7 +1059,42 @@ router.get('/:id/services', async (req, res) => { router.put('/:id/services/:serviceId', requireWrite, async (req, res) => { if (typeof req.body.disabled !== 'boolean') return res.status(400).json({ error: 'disabled (boolean) is required' }); - await withCollector(req.params.id, res, async (c) => { await c.setServiceDisabled(req.params.serviceId, req.body.disabled); return c.getServices(); }); + + const deviceRow = await queryOne(`SELECT * FROM devices WHERE id = $1`, [req.params.id]); + if (!deviceRow) return res.status(404).json({ error: 'Device not found' }); + + // Disabling the very service MikroTik Manager talks to is an immediate, certain + // self-lockout — and unlike other changes there is no legitimate way to do it + // from here, so it is refused outright rather than merely guarded. + if (req.body.disabled === true) { + const mgmtService = deviceRow.api_port === 8729 ? 'api-ssl' : 'api'; + const collector = new DeviceCollector(deviceRow); + try { + await collector.connect(); + const services = await collector.getServices(); + const target = (services as Record[]).find((s) => s['.id'] === req.params.serviceId); + if (target && target['name'] === mgmtService) { + return res.status(409).json({ + lockout: true, + reason: `'${mgmtService}' is the service MikroTik Manager uses to reach this device. ` + + `Disabling it would make the device unmanageable from here.`, + }); + } + } catch { /* fall through to the guarded path */ } finally { + collector.disconnect(); + } + } + + await withGuardedChange( + req.params.id, + req, + res, + { kind: 'service.toggle', summary: `${req.body.disabled ? 'Disable' : 'Enable'} IP service` }, + async (c) => { + await c.setServiceDisabled(req.params.serviceId, req.body.disabled); + return { services: await c.getServices() }; + } + ); }); // GET /api/devices/:id/security-posture — computes a hardening checklist. @@ -1769,22 +1859,21 @@ router.post('/:id/bonds', requireWrite, async (req: Request, res: Response) => { if (!name || !mode || !Array.isArray(slaves) || slaves.length < 2) { return res.status(400).json({ error: 'name, mode, and at least 2 slaves are required' }); } - const deviceRow = await queryOne(`SELECT * FROM devices WHERE id = $1`, [req.params.id]); - if (!deviceRow) return res.status(404).json({ error: 'Device not found' }); - const collector = new DeviceCollector(deviceRow); - try { - await collector.connect(); - await collector.createBond(name, slaves, mode, { - lacpRate: lacp_rate, hashPolicy: transmit_hash_policy, mtu, minLinks: min_links, - }); - await collector.collectInterfaces(); - const ports = await query(`SELECT * FROM interfaces WHERE device_id = $1 ORDER BY name`, [req.params.id]); - return res.status(201).json(ports.find((p: Record) => p['name'] === name) ?? { message: 'Bond created' }); - } catch (err) { - return res.status(500).json({ error: (err as Error).message }); - } finally { - collector.disconnect(); - } + // Guarded: creating a bond pulls its member ports out of their bridge first, so a + // failure part-way through can strand the uplink. + await withGuardedChange( + req.params.id, + req, + res, + { kind: 'bond.create', summary: `Create bond '${name}' over ${slaves.join(', ')}` }, + async (c) => { + await c.createBond(name, slaves, mode, { + lacpRate: lacp_rate, hashPolicy: transmit_hash_policy, mtu, minLinks: min_links, + }); + return { message: 'Bond created' }; + }, + async (c) => { await c.collectInterfaces(); } + ); }); // PUT /api/devices/:id/bonds/:bondName @@ -1811,39 +1900,44 @@ router.put('/:id/bonds/:bondName', requireWrite, async (req: Request, res: Respo }); // DELETE /api/devices/:id/bonds/:bondName +// Guarded: removing a bond releases its member ports, which can take the device +// off the network even when the bond looks unused. router.delete('/:id/bonds/:bondName', requireWrite, async (req: Request, res: Response) => { - const deviceRow = await queryOne(`SELECT * FROM devices WHERE id = $1`, [req.params.id]); - if (!deviceRow) return res.status(404).json({ error: 'Device not found' }); - const collector = new DeviceCollector(deviceRow); - try { - await collector.connect(); - await collector.deleteBond(req.params.bondName); - await query(`DELETE FROM interfaces WHERE device_id = $1 AND name = $2`, [req.params.id, req.params.bondName]); - await collector.collectInterfaces(); - return res.json({ message: 'Bond deleted' }); - } catch (err) { - return res.status(500).json({ error: (err as Error).message }); - } finally { - collector.disconnect(); - } + await withGuardedChange( + req.params.id, + req, + res, + { kind: 'bond.delete', summary: `Delete bond '${req.params.bondName}'` }, + async (c) => { + await c.deleteBond(req.params.bondName); + return { message: 'Bond deleted' }; + }, + async (c) => { + await query(`DELETE FROM interfaces WHERE device_id = $1 AND name = $2`, [req.params.id, req.params.bondName]); + await c.collectInterfaces(); + } + ); }); // PUT /api/devices/:id/bridge/:bridgeName/vlan-filtering router.put('/:id/bridge/:bridgeName/vlan-filtering', requireWrite, async (req: Request, res: Response) => { const { enabled } = req.body as { enabled: boolean }; - const deviceRow = await queryOne(`SELECT * FROM devices WHERE id = $1`, [req.params.id]); - if (!deviceRow) return res.status(404).json({ error: 'Device not found' }); - const collector = new DeviceCollector(deviceRow); - try { - await collector.connect(); - await collector.setBridgeVlanFiltering(req.params.bridgeName, Boolean(enabled)); - await collector.collectInterfaces(); - return res.json({ success: true, vlan_filtering: enabled }); - } catch (err) { - return res.status(500).json({ error: (err as Error).message }); - } finally { - collector.disconnect(); - } + // Guarded: turning on vlan-filtering with untagged management on the bridge is + // the single most common way to lock yourself out of a MikroTik. + await withGuardedChange( + req.params.id, + req, + res, + { + kind: 'bridge.vlan-filtering', + summary: `${enabled ? 'Enable' : 'Disable'} VLAN filtering on bridge '${req.params.bridgeName}'`, + }, + async (c) => { + await c.setBridgeVlanFiltering(req.params.bridgeName, Boolean(enabled)); + return { success: true, vlan_filtering: enabled }; + }, + async (c) => { await c.collectInterfaces(); } + ); }); // GET /api/devices/:id/hardware (live health: temps, voltages, fans, PSU) diff --git a/backend/src/services/PollerService.ts b/backend/src/services/PollerService.ts index ed74da6..be4c87c 100644 --- a/backend/src/services/PollerService.ts +++ b/backend/src/services/PollerService.ts @@ -254,7 +254,8 @@ export class PollerService { 'retention_clients_days', 'spectral_scan_enabled', 'spectral_scan_interval_hours', 'ap_scan_enabled', 'ap_scan_interval_hours', 'backup_schedule_enabled', - 'backup_schedule_cron')` + 'backup_schedule_cron', 'config_snapshot_enabled', + 'config_snapshot_interval_min')` ); const map: Record = {}; for (const row of rows) map[row.key] = row.value; diff --git a/backend/src/services/changeGuard/ChangeGuard.ts b/backend/src/services/changeGuard/ChangeGuard.ts new file mode 100644 index 0000000..bcb4dfd --- /dev/null +++ b/backend/src/services/changeGuard/ChangeGuard.ts @@ -0,0 +1,318 @@ +/** + * Change Guard — a device-side dead-man switch for risky configuration changes. + * + * A user change can sever the manager's own path to a device (deleting a bond that + * carried the uplink, enabling vlan-filtering, dropping the management IP...). No + * static analysis catches every case, so before applying such a change we make the + * device able to rescue itself: + * + * 1. save a restore point ON the device + * 2. arm a scheduler that restores it after N seconds + * 3. apply the change + * 4. prove we can still reach the device on a NEW connection + * 5. commit -> disarm the scheduler and delete the restore point + * abandon -> do nothing; the device restores itself and comes back + * + * The restore point is created *before* the scheduler, so a binary restore rolls + * back to a config containing neither the scheduler nor the change: self-cleaning, + * with no possibility of a revert loop. (Script mode has no reboot to clear the + * scheduler, so its on-event removes itself explicitly.) + * + * Everything here runs over the RouterOS API — no SSH — because many deployments + * never configure SSH credentials and would otherwise have no protection at all. + */ +import { randomBytes } from 'crypto'; +import { RouterOSClient } from '../mikrotik/RouterOSClient'; +import { decrypt } from '../../utils/crypto'; +import { query, queryOne } from '../../config/database'; +import { redis } from '../../config/redis'; + +export type GuardMode = 'binary' | 'script'; + +export interface GuardDevice { + id: number; + name: string; + ip_address: string; + api_port: number; + api_username: string; + api_password_encrypted: string; +} + +export interface ChangeMeta { + /** Stable identifier for the kind of change, e.g. 'bond.delete'. */ + kind: string; + /** Human-readable one-liner shown in the UI and stored for correlation. */ + summary: string; + userId?: number | null; + mode?: GuardMode; + timeoutSec?: number; +} + +export interface GuardOutcome { + result: T; + /** True when the change was applied and the device confirmed reachable. */ + confirmed: boolean; + /** True when we could not confirm reachability and left the device to self-revert. */ + autoReverting: boolean; + /** Set when protection could not be armed; the change ran unprotected. */ + unprotectedReason?: string; + guardId?: number; +} + +const DEFAULT_TIMEOUT_SEC = 120; +const VERIFY_ATTEMPTS = 4; +const VERIFY_CONNECT_TIMEOUT_MS = 8_000; +const LOCK_TTL_SEC = 600; + +function token(): string { + return randomBytes(5).toString('hex'); +} + +function newClient(device: GuardDevice, connectTimeoutMs = 15_000): RouterOSClient { + return new RouterOSClient( + device.ip_address, + device.api_port, + device.api_username, + decrypt(device.api_password_encrypted), + connectTimeoutMs, + 30_000 + ); +} + +/** Settings are read per call so an operator can change them without a restart. */ +async function readSettings(): Promise<{ enabled: boolean; mode: GuardMode; timeoutSec: number }> { + const rows = await query<{ key: string; value: unknown }>( + `SELECT key, value FROM app_settings + WHERE key IN ('change_guard_enabled', 'change_guard_mode', 'change_guard_timeout_sec')` + ).catch(() => []); + const map: Record = {}; + for (const r of rows) map[r.key] = r.value; + const mode = map['change_guard_mode'] === 'script' ? 'script' : 'binary'; + const timeoutSec = Number(map['change_guard_timeout_sec']) || DEFAULT_TIMEOUT_SEC; + return { enabled: map['change_guard_enabled'] !== false, mode, timeoutSec }; +} + +/** + * Can this device protect itself? Requires the API user to be able to write a + * restore point and add/remove a scheduler. Probes with a harmless scheduler + * (logs a line, fires in a day) that is removed immediately. + */ +export async function probeCapability(device: GuardDevice): Promise<{ ok: boolean; reason?: string }> { + const client = newClient(device); + const probeName = `mtm-probe-${token()}`; + try { + await client.connect(); + await client.execute('/system/scheduler/add', { + name: probeName, + interval: '1d', + 'on-event': ':log info "mtm-guard-probe"', + }); + const found = (await client.execute('/system/scheduler/print')).find((s) => s['name'] === probeName); + if (!found?.['.id']) return { ok: false, reason: 'could not create a scheduler on the device' }; + await client.execute('/system/scheduler/remove', { '.id': found['.id'] }); + return { ok: true }; + } catch (err) { + return { ok: false, reason: (err as Error).message }; + } finally { + client.disconnect(); + } +} + +/** Prove reachability on a brand-new connection, retried across the window. */ +async function verifyReachable(device: GuardDevice, attempts = VERIFY_ATTEMPTS): Promise { + for (let i = 0; i < attempts; i++) { + const client = newClient(device, VERIFY_CONNECT_TIMEOUT_MS); + try { + await client.connect(); + const rows = await client.execute('/system/identity/print'); + if (rows.length > 0) return true; + } catch { + /* fall through to retry */ + } finally { + client.disconnect(); + } + if (i < attempts - 1) await new Promise((r) => setTimeout(r, 3_000)); + } + return false; +} + +async function acquireLock(deviceId: number): Promise { + try { + const res = await redis.set(`changeguard:lock:${deviceId}`, '1', 'EX', LOCK_TTL_SEC, 'NX'); + return res === 'OK'; + } catch { + return true; // Redis unavailable — don't block legitimate changes + } +} + +async function releaseLock(deviceId: number): Promise { + await redis.del(`changeguard:lock:${deviceId}`).catch(() => {}); +} + +async function recordGuard( + device: GuardDevice, meta: ChangeMeta, mode: GuardMode, restorePoint: string, + schedulerName: string, timeoutSec: number +): Promise { + const row = await queryOne<{ id: number }>( + `INSERT INTO device_change_guards + (device_id, token, mode, restore_point, scheduler_name, status, change_kind, + change_summary, user_id, expires_at) + VALUES ($1,$2,$3,$4,$5,'pending',$6,$7,$8, NOW() + ($9 || ' seconds')::interval) + RETURNING id`, + [device.id, schedulerName, mode, restorePoint, schedulerName, meta.kind, + meta.summary, meta.userId ?? null, String(timeoutSec)] + ).catch(() => null); + return row?.id; +} + +async function setGuardStatus(id: number | undefined, status: string, note?: string): Promise { + if (!id) return; + // Casts are required: an untyped NULL in COALESCE makes Postgres fail to infer + // the parameter type. Failures are logged, never swallowed — a guard row stuck + // at 'pending' would later be misread as an unfinished change. + await query( + `UPDATE device_change_guards + SET status = $2::varchar, + note = COALESCE($3::text, note), + committed_at = CASE WHEN $2::varchar <> 'pending' THEN NOW() ELSE committed_at END + WHERE id = $1`, + [id, status, note ?? null] + ).catch((e) => { + console.error(`[ChangeGuard] Failed to record guard ${id} as '${status}':`, (e as Error).message); + }); +} + +/** + * Run `applyFn` with a self-restore safety net armed on the device. + * + * If protection can't be armed the change still runs (refusing would make the + * platform less useful than before), but the caller is told via + * `unprotectedReason` so the UI can say so plainly. + */ +export async function withSafeApply( + device: GuardDevice, + meta: ChangeMeta, + applyFn: () => Promise +): Promise> { + const settings = await readSettings(); + const mode: GuardMode = meta.mode ?? settings.mode; + const timeoutSec = meta.timeoutSec ?? settings.timeoutSec; + + if (!settings.enabled) { + return { result: await applyFn(), confirmed: false, autoReverting: false, unprotectedReason: 'Change Guard is disabled in settings' }; + } + if (!(await acquireLock(device.id))) { + throw new Error('Another protected change is already in progress on this device. Please wait for it to finish.'); + } + + const id = token(); + const restorePoint = `mtm-rp-${id}`; + const schedulerName = `mtm-revert-${id}`; + const client = newClient(device); + let armed = false; + let guardId: number | undefined; + + try { + // ── Arm ──────────────────────────────────────────────────────────────── + try { + await client.connect(); + if (mode === 'binary') { + await client.execute('/system/backup/save', { name: restorePoint }); + } else { + await client.execute('/export', { file: restorePoint }); + } + + const onEvent = mode === 'binary' + // Restores exactly (recovers deleted interfaces) and reboots. The restored + // config predates this scheduler, so it disappears with the restore. + ? `/system backup load name=${restorePoint} password=""` + // No reboot, but /import is additive, so the scheduler must remove itself. + : `/import file-name=${restorePoint}.rsc\n/system scheduler remove [find name="${schedulerName}"]`; + + await client.execute('/system/scheduler/add', { + name: schedulerName, + interval: `${timeoutSec}s`, + 'on-event': onEvent, + }); + armed = true; + guardId = await recordGuard(device, meta, mode, restorePoint, schedulerName, timeoutSec); + } catch (err) { + // Couldn't arm — clean up any half-created artifacts and run unprotected. + await cleanup(client, restorePoint, schedulerName, mode).catch(() => {}); + const result = await applyFn(); + return { + result, confirmed: false, autoReverting: false, + unprotectedReason: `Could not arm auto-revert on this device (${(err as Error).message})`, + }; + } + + // ── Apply ────────────────────────────────────────────────────────────── + let result: T; + try { + result = await applyFn(); + } catch (err) { + // The change itself failed. Disarm so the device isn't rebooted for nothing. + await cleanup(client, restorePoint, schedulerName, mode).catch(() => {}); + await setGuardStatus(guardId, 'failed', (err as Error).message); + armed = false; + throw err; + } + + // ── Verify ───────────────────────────────────────────────────────────── + const reachable = await verifyReachable(device); + if (!reachable) { + // Leave the scheduler armed: the device will restore itself and come back. + await setGuardStatus(guardId, 'reverted', 'Device unreachable after change; left to self-restore'); + armed = false; // deliberately not cleaned up + return { result, confirmed: false, autoReverting: true, guardId }; + } + + // ── Commit ───────────────────────────────────────────────────────────── + await cleanup(client, restorePoint, schedulerName, mode); + armed = false; + await setGuardStatus(guardId, 'committed'); + return { result, confirmed: true, autoReverting: false, guardId }; + } finally { + if (armed) await setGuardStatus(guardId, 'pending'); + client.disconnect(); + await releaseLock(device.id); + } +} + +/** Disarm: remove the scheduler, then the restore point. Uses a fresh connection if needed. */ +async function cleanup( + client: RouterOSClient, restorePoint: string, schedulerName: string, mode: GuardMode +): Promise { + if (!client.isConnected()) await client.connect(); + const scheds = await client.execute('/system/scheduler/print').catch(() => []); + for (const s of scheds) { + if (s['name'] === schedulerName && s['.id']) { + await client.execute('/system/scheduler/remove', { '.id': s['.id'] }); + } + } + const wanted = mode === 'binary' ? `${restorePoint}.backup` : `${restorePoint}.rsc`; + const files = await client.execute('/file/print').catch(() => []); + for (const f of files) { + if ((f['name'] === wanted || f['name'] === restorePoint) && f['.id']) { + await client.execute('/file/remove', { '.id': f['.id'] }).catch(() => {}); + } + } +} + +/** + * Startup reconciliation: guards still 'pending' after their window elapsed belong + * to a manager that crashed mid-change. The device has since restored itself. + */ +export async function reconcileStaleGuards(): Promise { + const rows = await query<{ id: number }>( + `UPDATE device_change_guards + SET status = 'reverted', committed_at = NOW(), + note = COALESCE(note, 'Manager restarted before commit; device self-restored') + WHERE status = 'pending' AND expires_at < NOW() + RETURNING id` + ).catch(() => []); + if (rows.length > 0) { + console.log(`[ChangeGuard] Reconciled ${rows.length} stale guard(s) left pending by a restart`); + } + return rows.length; +} diff --git a/backend/src/services/changeGuard/__tests__/ChangeGuard.test.ts b/backend/src/services/changeGuard/__tests__/ChangeGuard.test.ts new file mode 100644 index 0000000..c5f0aa0 --- /dev/null +++ b/backend/src/services/changeGuard/__tests__/ChangeGuard.test.ts @@ -0,0 +1,171 @@ +jest.mock('../../../config/database'); +jest.mock('../../../config/redis', () => ({ + redis: { set: jest.fn().mockResolvedValue('OK'), del: jest.fn().mockResolvedValue(1) }, +})); +jest.mock('../../../utils/crypto', () => ({ decrypt: () => 'pw' })); +jest.mock('../../mikrotik/RouterOSClient'); + +import { withSafeApply, type GuardDevice } from '../ChangeGuard'; +import { RouterOSClient } from '../../mikrotik/RouterOSClient'; +import { query, queryOne } from '../../../config/database'; +import { redis } from '../../../config/redis'; + +const mockedQuery = jest.mocked(query); +const mockedQueryOne = jest.mocked(queryOne); +const MockedClient = jest.mocked(RouterOSClient); + +const device: GuardDevice = { + id: 1, name: 'sw1', ip_address: '10.0.0.1', api_port: 8728, + api_username: 'admin', api_password_encrypted: 'enc', +}; + +/** Commands seen across every client instance, in order. */ +let commands: string[]; +/** When true, the *verification* clients fail to connect (device unreachable). */ +let unreachableAfterApply: boolean; +let applyDone: boolean; +/** Simulated device state, so cleanup has something real to find and remove. */ +let deviceSchedulers: Record[]; +let deviceFiles: Record[]; + +function installClientMock() { + commands = []; + applyDone = false; + deviceSchedulers = []; + deviceFiles = []; + MockedClient.mockImplementation(() => { + const client = { + connect: jest.fn(async () => { + // Verification uses fresh clients created after the change is applied. + if (applyDone && unreachableAfterApply) throw new Error('connect ETIMEDOUT'); + }), + disconnect: jest.fn(), + isConnected: jest.fn(() => true), + execute: jest.fn(async (cmd: string, params: Record = {}) => { + commands.push(cmd); + switch (cmd) { + case '/system/backup/save': + deviceFiles.push({ '.id': `*f${deviceFiles.length}`, name: `${params.name}.backup` }); + return []; + case '/export': + deviceFiles.push({ '.id': `*f${deviceFiles.length}`, name: `${params.file}.rsc` }); + return []; + case '/system/scheduler/add': + deviceSchedulers.push({ '.id': `*s${deviceSchedulers.length}`, name: params.name }); + return []; + case '/system/scheduler/remove': + deviceSchedulers = deviceSchedulers.filter((s) => s['.id'] !== params['.id']); + return []; + case '/file/remove': + deviceFiles = deviceFiles.filter((f) => f['.id'] !== params['.id']); + return []; + case '/system/scheduler/print': return deviceSchedulers; + case '/file/print': return deviceFiles; + case '/system/identity/print': return [{ name: 'sw1' }]; + default: return []; + } + }), + }; + return client as unknown as RouterOSClient; + }); +} + +beforeEach(() => { + jest.clearAllMocks(); + unreachableAfterApply = false; + installClientMock(); + // Settings lookup + any INSERT/UPDATE bookkeeping + mockedQuery.mockResolvedValue([]); + mockedQueryOne.mockResolvedValue({ id: 42 } as never); + jest.mocked(redis.set).mockResolvedValue('OK' as never); +}); + +describe('withSafeApply', () => { + it('arms a restore point and scheduler BEFORE running the change', async () => { + const order: string[] = []; + await withSafeApply(device, { kind: 'test', summary: 's' }, async () => { + order.push(...commands, 'APPLY'); + return 'ok'; + }); + + const armIdx = order.indexOf('/system/backup/save'); + const schedIdx = order.indexOf('/system/scheduler/add'); + const applyIdx = order.indexOf('APPLY'); + expect(armIdx).toBeGreaterThanOrEqual(0); + // Restore point must exist before the scheduler, so a restore also clears it + expect(armIdx).toBeLessThan(schedIdx); + expect(schedIdx).toBeLessThan(applyIdx); + }); + + it('commits by disarming the scheduler when the device stays reachable', async () => { + const out = await withSafeApply(device, { kind: 'test', summary: 's' }, async () => { + applyDone = true; + return 'ok'; + }); + + expect(out.confirmed).toBe(true); + expect(out.autoReverting).toBe(false); + // Nothing left behind on the device: no armed revert, no restore-point file + expect(deviceSchedulers).toHaveLength(0); + expect(deviceFiles).toHaveLength(0); + }); + + it('leaves the scheduler ARMED when the device is unreachable after the change', async () => { + unreachableAfterApply = true; + const out = await withSafeApply(device, { kind: 'test', summary: 's' }, async () => { + applyDone = true; + return 'ok'; + }); + + expect(out.confirmed).toBe(false); + expect(out.autoReverting).toBe(true); + // Critically: the revert must remain armed so the device can rescue itself + expect(deviceSchedulers).toHaveLength(1); + expect(deviceFiles).toHaveLength(1); + // Generous budget: this exercises the real retry-with-backoff before giving up. + }, 30_000); + + it('disarms and rethrows when the change itself fails, so the device is not rebooted for nothing', async () => { + await expect( + withSafeApply(device, { kind: 'test', summary: 's' }, async () => { + throw new Error('bad command'); + }) + ).rejects.toThrow('bad command'); + expect(deviceSchedulers).toHaveLength(0); + }); + + it('still applies the change when protection cannot be armed, and says so', async () => { + MockedClient.mockImplementation(() => ({ + connect: jest.fn(async () => {}), + disconnect: jest.fn(), + isConnected: jest.fn(() => true), + execute: jest.fn(async (cmd: string) => { + if (cmd === '/system/backup/save') throw new Error('no permission'); + return []; + }), + }) as unknown as RouterOSClient); + + let ran = false; + const out = await withSafeApply(device, { kind: 'test', summary: 's' }, async () => { ran = true; return 'ok'; }); + + expect(ran).toBe(true); + expect(out.unprotectedReason).toMatch(/no permission/); + expect(out.confirmed).toBe(false); + }); + + it('refuses to start a second protected change on the same device', async () => { + jest.mocked(redis.set).mockResolvedValueOnce(null as never); // lock already held + await expect( + withSafeApply(device, { kind: 'test', summary: 's' }, async () => 'ok') + ).rejects.toThrow(/already in progress/); + }); + + it('uses /export instead of a binary backup in script mode', async () => { + await withSafeApply(device, { kind: 'test', summary: 's', mode: 'script' }, async () => { + applyDone = true; + return 'ok'; + }); + expect(commands).toContain('/export'); + expect(commands).not.toContain('/system/backup/save'); + }); +}); diff --git a/backend/src/services/mikrotik/DeviceCollector.ts b/backend/src/services/mikrotik/DeviceCollector.ts index 454dec4..f067810 100644 --- a/backend/src/services/mikrotik/DeviceCollector.ts +++ b/backend/src/services/mikrotik/DeviceCollector.ts @@ -2715,16 +2715,40 @@ export class DeviceCollector { return policy; } + /** + * Bridge-port attributes that must survive a port being re-created. PVID above + * all: re-adding a port without it silently drops the port back to VLAN 1, which + * breaks tagged management even though the port looks bridged again. + */ + private static readonly BRIDGE_PORT_CARRY = [ + 'pvid', 'frame-types', 'ingress-filtering', 'edge', 'horizon', 'point-to-point', + ] as const; + + private static carryBridgePortAttrs(port: Record | undefined): Record { + const out: Record = {}; + if (!port) return out; + for (const key of DeviceCollector.BRIDGE_PORT_CARRY) { + const value = port[key]; + if (value !== undefined && value !== '') out[key] = value; + } + return out; + } + async createBond(name: string, slaves: string[], mode: string, opts: { lacpRate?: string; hashPolicy?: string; mtu?: number; minLinks?: number; }): Promise { - // Remove each slave from any bridge, recording the first bridge found + // Remove each slave from any bridge, recording the first bridge found along + // with its port settings so the bond can inherit them. let originalBridge: string | null = null; + let carry: Record = {}; for (const slave of slaves) { const bridgePorts = await this.client.execute('/interface/bridge/port/print', {}, [`?interface=${slave}`]).catch(() => []); for (const bp of bridgePorts) { if (bp['.id']) { - if (!originalBridge) originalBridge = bp['bridge'] ?? null; + if (!originalBridge) { + originalBridge = bp['bridge'] ?? null; + carry = DeviceCollector.carryBridgePortAttrs(bp); + } await this.client.execute('/interface/bridge/port/remove', { '.id': bp['.id'] }); } } @@ -2735,12 +2759,23 @@ export class DeviceCollector { if (opts.mtu) params['mtu'] = String(opts.mtu); if (opts.minLinks != null) params['min-links'] = String(opts.minLinks); await this.client.execute('/interface/bonding/add', params); - // Add the new bond interface to the same bridge the slaves were removed from + // Put the new bond into the bridge the slaves came out of. If this fails the + // member ports are left unbridged — never swallow it, the device may be + // unreachable and the caller must know. if (originalBridge) { - await this.client.execute('/interface/bridge/port/add', { - bridge: originalBridge, - interface: name, - }).catch(() => {}); + try { + await this.client.execute('/interface/bridge/port/add', { + bridge: originalBridge, + interface: name, + ...carry, + }); + } catch (err) { + throw new Error( + `Bond '${name}' was created, but adding it to bridge '${originalBridge}' failed: ` + + `${(err as Error).message}. The member ports (${slaves.join(', ')}) are no longer bridged ` + + `and may not forward traffic — check the device.` + ); + } } } @@ -2763,22 +2798,43 @@ export class DeviceCollector { const id = list[0]?.['.id']; if (!id) throw new Error(`Bond '${name}' not found on device`); const slaves = (list[0]?.['slaves'] ?? '').split(',').map((s: string) => s.trim()).filter(Boolean); - // Check if the bond is itself a bridge member + // Capture the bond's bridge membership *and its port settings* before removing + // it, so the freed slaves can inherit them (notably PVID). const bondBridgePorts = await this.client.execute('/interface/bridge/port/print', {}, [`?interface=${name}`]).catch(() => []); - const bridgeName = bondBridgePorts[0]?.['bridge'] ?? null; - if (bondBridgePorts[0]?.['.id']) { - await this.client.execute('/interface/bridge/port/remove', { '.id': bondBridgePorts[0]['.id'] }); + const bondPort = bondBridgePorts[0]; + const bridgeName = bondPort?.['bridge'] ?? null; + const carry = DeviceCollector.carryBridgePortAttrs(bondPort); + + if (bondPort?.['.id']) { + await this.client.execute('/interface/bridge/port/remove', { '.id': bondPort['.id'] }); } await this.client.execute('/interface/bonding/remove', { '.id': id }); - // Re-add each slave to the bridge the bond was in + + // Removing a bond releases its slaves as standalone interfaces. If the bond was + // bridged, put each slave back in that bridge with the same port settings — + // otherwise the physical ports stop forwarding and the device can drop off the + // network. A failure here is exactly how a switch goes silently offline, so it + // is reported rather than swallowed. + const failures: string[] = []; if (bridgeName) { for (const slave of slaves) { - await this.client.execute('/interface/bridge/port/add', { - bridge: bridgeName, - interface: slave, - }).catch(() => {}); + try { + await this.client.execute('/interface/bridge/port/add', { + bridge: bridgeName, + interface: slave, + ...carry, + }); + } catch (err) { + failures.push(`${slave} (${(err as Error).message})`); + } } } + if (failures.length > 0) { + throw new Error( + `Bond '${name}' was removed, but restoring bridge membership on '${bridgeName}' failed for: ` + + `${failures.join('; ')}. Those ports may not forward traffic — check the device.` + ); + } } // ─── Hardware Health ────────────────────────────────────────────────────── diff --git a/frontend/package.json b/frontend/package.json index f49f92d..a4a7aa0 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,6 +1,6 @@ { "name": "mikrotik-manager-frontend", - "version": "0.17.7-beta", + "version": "0.18.0-beta", "private": true, "type": "module", "scripts": { diff --git a/frontend/src/components/ChangeGuardDialog.tsx b/frontend/src/components/ChangeGuardDialog.tsx new file mode 100644 index 0000000..ef39e49 --- /dev/null +++ b/frontend/src/components/ChangeGuardDialog.tsx @@ -0,0 +1,104 @@ +import { AlertTriangle, RefreshCw, ShieldCheck } from 'lucide-react'; + +/** + * Confirmation for a change that could sever the manager's path to a device. + * + * Follows the existing LockoutDialog/ScanWarningModal pattern (amber = dangerous + * but intentional; red is reserved for delete), and explains the protection that + * will be applied so the user knows what happens if it goes wrong. + */ +export interface ChangeGuardDialogProps { + title: string; + /** What the user is about to do, in plain language. */ + description: string; + /** Optional extra risk detail (e.g. why this object matters to reachability). */ + reason?: string; + confirmLabel: string; + /** When false, the safety net is unavailable and the change is unprotected. */ + protectedByGuard?: boolean; + pending?: boolean; + onConfirm: () => void; + onCancel: () => void; +} + +export default function ChangeGuardDialog({ + title, description, reason, confirmLabel, + protectedByGuard = true, pending = false, onConfirm, onCancel, +}: ChangeGuardDialogProps) { + return ( +
+
+
+ +
+

{title}

+

{description}

+
+
+ + {reason && ( +
+

{reason}

+
+ )} + + {protectedByGuard ? ( +
+ + + Protected: the device saves a restore point first. If it stops responding after + this change, it restores itself automatically and comes back. + +
+ ) : ( +
+ + + Auto-revert is not available for this device, so this change cannot be undone + automatically if it goes wrong. + +
+ )} + +
+ + +
+
+
+ ); +} + +/** Shape returned by guarded endpoints so callers can report the outcome. */ +export interface GuardResult { + protected: boolean; + confirmed: boolean; + auto_reverting: boolean; + unprotected_reason: string | null; +} + +/** Human-readable outcome for a guarded change, or null when nothing to say. */ +export function guardOutcomeMessage(guard?: GuardResult): { tone: 'ok' | 'warn'; text: string } | null { + if (!guard) return null; + if (guard.auto_reverting) { + return { + tone: 'warn', + text: 'The device stopped responding after this change, so it is restoring itself to the ' + + 'previous configuration. It should come back shortly — the change was not kept.', + }; + } + if (guard.confirmed) { + return { tone: 'ok', text: 'Applied — device confirmed still reachable.' }; + } + if (guard.unprotected_reason) { + return { tone: 'warn', text: `Applied without auto-revert protection: ${guard.unprotected_reason}` }; + } + return null; +} diff --git a/frontend/src/components/ports/SwitchPortDiagram.tsx b/frontend/src/components/ports/SwitchPortDiagram.tsx index 76369d7..5b72d63 100644 --- a/frontend/src/components/ports/SwitchPortDiagram.tsx +++ b/frontend/src/components/ports/SwitchPortDiagram.tsx @@ -1,9 +1,10 @@ import { useState, useEffect } from 'react'; import { Link } from 'react-router-dom'; import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query'; -import { RefreshCw, X, Check, AlertCircle, Activity, Cpu, Link2, Trash2, Plus, Network, Users, Wifi } from 'lucide-react'; +import { RefreshCw, X, Check, AlertCircle, Activity, Cpu, Link2, Trash2, Plus, Network, Users, Wifi, ShieldCheck, AlertTriangle } from 'lucide-react'; import { devicesApi, metricsApi } from '../../services/api'; import { useCanWrite } from '../../hooks/useCanWrite'; +import ChangeGuardDialog, { guardOutcomeMessage, type GuardResult } from '../ChangeGuardDialog'; import type { SwitchPort, Vlan, TrafficPoint, PortMonitorData, PortClient } from '../../types'; import { LineChart, Line, XAxis, YAxis, CartesianGrid, Tooltip, @@ -596,6 +597,8 @@ export default function SwitchPortDiagram({ deviceId, autoOpenBridge, onBridgeOp const [trafficRange, setTrafficRange] = useState('1h'); const [showCreateTrunkModal, setShowCreateTrunkModal] = useState(false); const [editingBond, setEditingBond] = useState(null); + const [confirmDestroyBond, setConfirmDestroyBond] = useState(null); + const [guardNotice, setGuardNotice] = useState<{ tone: 'ok' | 'warn'; text: string } | null>(null); const [editForm, setEditForm] = useState({ disabled: false, comment: '', @@ -713,10 +716,17 @@ export default function SwitchPortDiagram({ deviceId, autoOpenBridge, onBridgeOp const deleteBondMutation = useMutation({ mutationFn: (name: string) => devicesApi.deleteBond(deviceId, name), - onSuccess: () => { + onSuccess: (res) => { queryClient.invalidateQueries({ queryKey: ['ports', deviceId] }); setEditingBond(null); + setConfirmDestroyBond(null); setBondError(''); + // Report what the safety net did — especially "the device is restoring itself". + const notice = guardOutcomeMessage((res?.data as { guard?: GuardResult })?.guard); + if (notice) { + setGuardNotice(notice); + if (notice.tone === 'ok') setTimeout(() => setGuardNotice(null), 6000); + } }, onError: (err: unknown) => { const msg = (err as { response?: { data?: { error?: string } } })?.response?.data?.error; @@ -1792,11 +1802,7 @@ export default function SwitchPortDiagram({ deviceId, autoOpenBridge, onBridgeOp + + + )} ); } diff --git a/frontend/src/version.ts b/frontend/src/version.ts index 87e032e..d2eeb37 100644 --- a/frontend/src/version.ts +++ b/frontend/src/version.ts @@ -1 +1 @@ -export const APP_VERSION = 'v0.17.7 Beta'; +export const APP_VERSION = 'v0.18.0 Beta';