From ba017ee665bb2b7f7ef479b5d8a9f8cbb0681302 Mon Sep 17 00:00:00 2001 From: Anso Date: Sun, 2 Aug 2026 03:00:09 -0400 Subject: [PATCH] fix: forward scoped node-admin permission for Settings writes through remote proxy (#1748) * fix: forward scoped node-admin permission for Settings writes through remote proxy The proxy forwards only the user's global role via PROXY_ROLE_HEADER to remote nodes. Scoped role assignments live only on the hub's role_assignments table and are never transmitted, so a scoped Node Admin could not save settings on their granted remote node through the hub proxy. Add a settings-write pre-authorization gate in runGatedProxy that buffers the body, extracts required permission buckets from SETTING_WRITE_PERMISSIONS, checks them hub-side, and elevates PROXY_ROLE_HEADER to 'node-admin' when the scoped check passes. The gate is fail-closed: empty or unparseable bodies require checkNodeManage on the hub, matching the existing requireSettingsWritePermission empty-keys branch. Fixes the gate-parity gap where scoped node:manage worked locally but not through the proxy for Settings writes. * fix: remove unused UserRole import from remoteNodeProxy.ts * test(self-update): poll instead of a fixed delay in triggerUpdate assertion The 600ms sleep raced the route's 500ms post-response timer plus the persist/watch work executeClaimedCommunityUpdate does before calling triggerUpdate, leaving too little margin under CI's forked test pool. Poll with vi.waitFor instead, matching the pattern already used elsewhere in this suite. --- .../proxy-scoped-settings-authz.test.ts | 343 ++++++++++++++++++ .../self-update-pinned-routes.test.ts | 15 +- backend/src/proxy/remoteNodeProxy.ts | 110 +++++- backend/src/routes/settings.ts | 2 +- backend/src/types/express.ts | 7 +- 5 files changed, 454 insertions(+), 23 deletions(-) create mode 100644 backend/src/__tests__/proxy-scoped-settings-authz.test.ts diff --git a/backend/src/__tests__/proxy-scoped-settings-authz.test.ts b/backend/src/__tests__/proxy-scoped-settings-authz.test.ts new file mode 100644 index 00000000..2262dd13 --- /dev/null +++ b/backend/src/__tests__/proxy-scoped-settings-authz.test.ts @@ -0,0 +1,343 @@ +/** + * Hub → remote proxy coverage for scoped node-admin Settings writes. + * Exercises the settings pre-authorization gate in createRemoteProxyMiddleware + * through live loopback remotes. + */ +import { describe, it, expect, beforeAll, afterAll, beforeEach, vi } from 'vitest'; +import http from 'http'; +import bcrypt from 'bcrypt'; +import request from 'supertest'; +import jwt from 'jsonwebtoken'; +import { setupTestDb, cleanupTestDb, TEST_JWT_SECRET } from './helpers/setupTestDb'; +import { PROXY_ROLE_HEADER } from '../services/license-headers'; + +let tmpDir: string; +let app: import('express').Express; +let viewerBearer: string; +let viewerId: number; + +let grantedServer: http.Server; +let ungrantedServer: http.Server; +let grantedNodeId: number; +let ungrantedNodeId: number; + +interface CapturedHop { + method: string; + url: string; + roleHeader: string | undefined; +} +const grantedHops: CapturedHop[] = []; +const ungrantedHops: CapturedHop[] = []; + +function captureHop(req: http.IncomingMessage, into: CapturedHop[]): void { + into.push({ + method: req.method ?? '', + url: req.url ?? '', + roleHeader: req.headers[PROXY_ROLE_HEADER] as string | undefined, + }); +} + +function grantedRemote(): http.Server { + return http.createServer((req, res) => { + if (req.url?.startsWith('/api/meta')) { + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ + version: '0.93.0', + capabilities: ['cross-node-rbac'], + })); + return; + } + captureHop(req, grantedHops); + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ success: true })); + }); +} + +function ungrantedRemote(): http.Server { + return http.createServer((req, res) => { + if (req.url?.startsWith('/api/meta')) { + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ + version: '0.93.0', + capabilities: ['cross-node-rbac'], + })); + return; + } + captureHop(req, ungrantedHops); + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ success: true })); + }); +} + +async function listen(server: http.Server): Promise { + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + return (server.address() as import('net').AddressInfo).port; +} + +beforeAll(async () => { + tmpDir = await setupTestDb(); + ({ app } = await import('../index')); + const { DatabaseService } = await import('../services/DatabaseService'); + const { LicenseService } = await import('../services/LicenseService'); + vi.spyOn(LicenseService.getInstance(), 'getTier').mockReturnValue('paid'); + + const db = DatabaseService.getInstance(); + const hash = await bcrypt.hash('password123', 1); + viewerId = db.addUser({ + username: 'settings-scoped-viewer', + password_hash: hash, + role: 'viewer', + }); + const viewer = db.getUserByUsername('settings-scoped-viewer')!; + viewerBearer = jwt.sign( + { username: 'settings-scoped-viewer', role: 'viewer', tv: viewer.token_version }, + TEST_JWT_SECRET, + { expiresIn: '5m' }, + ); + + grantedServer = grantedRemote(); + ungrantedServer = ungrantedRemote(); + const grantedPort = await listen(grantedServer); + const ungrantedPort = await listen(ungrantedServer); + + grantedNodeId = db.addNode({ + name: 'settings-granted-remote', + type: 'remote', + mode: 'proxy', + compose_dir: '/tmp', + is_default: false, + api_url: `http://127.0.0.1:${grantedPort}`, + api_token: 'granted-token', + }); + ungrantedNodeId = db.addNode({ + name: 'settings-ungranted-remote', + type: 'remote', + mode: 'proxy', + compose_dir: '/tmp', + is_default: false, + api_url: `http://127.0.0.1:${ungrantedPort}`, + api_token: 'ungranted-token', + }); +}); + +afterAll(async () => { + await new Promise((resolve) => grantedServer.close(() => resolve())); + await new Promise((resolve) => ungrantedServer.close(() => resolve())); + cleanupTestDb(tmpDir); +}); + +beforeEach(async () => { + vi.restoreAllMocks(); + const { LicenseService } = await import('../services/LicenseService'); + vi.spyOn(LicenseService.getInstance(), 'getTier').mockReturnValue('paid'); + grantedHops.length = 0; + ungrantedHops.length = 0; +}); + +describe('remote proxy scoped node-admin settings writes', () => { + it('allows scoped node-admin on granted remote node and elevates role header', async () => { + const db = (await import('../services/DatabaseService')).DatabaseService.getInstance(); + db.addRoleAssignment({ + user_id: viewerId, + role: 'node-admin', + resource_type: 'node', + resource_id: String(grantedNodeId), + }); + + const res = await request(app) + .patch('/api/settings') + .set('Authorization', `Bearer ${viewerBearer}`) + .set('x-node-id', String(grantedNodeId)) + .send({ host_cpu_limit: 85 }); + + expect(res.status).toBe(200); + const hop = grantedHops.find((h) => h.url?.includes('/settings')); + expect(hop).toBeDefined(); + expect(hop!.roleHeader).toBe('node-admin'); + + db.deleteRoleAssignmentsByUser(viewerId); + }); + + it('denies scoped node-admin on ungranted remote node', async () => { + const db = (await import('../services/DatabaseService')).DatabaseService.getInstance(); + db.addRoleAssignment({ + user_id: viewerId, + role: 'node-admin', + resource_type: 'node', + resource_id: String(grantedNodeId), + }); + + const res = await request(app) + .patch('/api/settings') + .set('Authorization', `Bearer ${viewerBearer}`) + .set('x-node-id', String(ungrantedNodeId)) + .send({ host_cpu_limit: 85 }); + + expect(res.status).toBe(403); + expect(res.body.code).toBe('PERMISSION_DENIED'); + + db.deleteRoleAssignmentsByUser(viewerId); + }); + + it('denies viewer with no scoped grant on any remote node', async () => { + const res = await request(app) + .patch('/api/settings') + .set('Authorization', `Bearer ${viewerBearer}`) + .set('x-node-id', String(grantedNodeId)) + .send({ host_cpu_limit: 85 }); + + expect(res.status).toBe(403); + expect(res.body.code).toBe('PERMISSION_DENIED'); + }); + + it('denies empty-body PATCH from viewer with no grant (fail-closed)', async () => { + const res = await request(app) + .patch('/api/settings') + .set('Authorization', `Bearer ${viewerBearer}`) + .set('x-node-id', String(grantedNodeId)) + .send({}); + + expect(res.status).toBe(403); + expect(res.body.code).toBe('PERMISSION_DENIED'); + }); + + it('allows empty-body PATCH from scoped node-admin on granted node', async () => { + const db = (await import('../services/DatabaseService')).DatabaseService.getInstance(); + db.addRoleAssignment({ + user_id: viewerId, + role: 'node-admin', + resource_type: 'node', + resource_id: String(grantedNodeId), + }); + + const res = await request(app) + .patch('/api/settings') + .set('Authorization', `Bearer ${viewerBearer}`) + .set('x-node-id', String(grantedNodeId)) + .send({}); + + expect(res.status).toBe(200); + const hop = grantedHops.find((h) => h.url?.includes('/settings')); + expect(hop).toBeDefined(); + expect(hop!.roleHeader).toBe('node-admin'); + + db.deleteRoleAssignmentsByUser(viewerId); + }); + + it('denies mixed node:manage + system:settings PATCH from scoped node-admin', async () => { + const db = (await import('../services/DatabaseService')).DatabaseService.getInstance(); + db.addRoleAssignment({ + user_id: viewerId, + role: 'node-admin', + resource_type: 'node', + resource_id: String(grantedNodeId), + }); + + const res = await request(app) + .patch('/api/settings') + .set('Authorization', `Bearer ${viewerBearer}`) + .set('x-node-id', String(grantedNodeId)) + .send({ host_cpu_limit: 85, developer_mode: '1' }); + + expect(res.status).toBe(403); + expect(res.body.code).toBe('PERMISSION_DENIED'); + + db.deleteRoleAssignmentsByUser(viewerId); + }); + + it('rejects compressed settings body with 415', async () => { + const db = (await import('../services/DatabaseService')).DatabaseService.getInstance(); + db.addRoleAssignment({ + user_id: viewerId, + role: 'node-admin', + resource_type: 'node', + resource_id: String(grantedNodeId), + }); + + const res = await request(app) + .patch('/api/settings') + .set('Authorization', `Bearer ${viewerBearer}`) + .set('x-node-id', String(grantedNodeId)) + .set('Content-Encoding', 'gzip') + .send(Buffer.from('compressed')); + + expect(res.status).toBe(415); + expect(res.body.code).toBe('encoding_unsupported'); + + db.deleteRoleAssignmentsByUser(viewerId); + }); + + it('rejects oversized settings body with 413', async () => { + const db = (await import('../services/DatabaseService')).DatabaseService.getInstance(); + db.addRoleAssignment({ + user_id: viewerId, + role: 'node-admin', + resource_type: 'node', + resource_id: String(grantedNodeId), + }); + + // Build a body larger than 100 KB + const bigValue = 'x'.repeat(102 * 1024); + const res = await request(app) + .patch('/api/settings') + .set('Authorization', `Bearer ${viewerBearer}`) + .set('x-node-id', String(grantedNodeId)) + .set('Content-Length', String(bigValue.length + 30)) + .send(Buffer.from(bigValue)); + + expect(res.status).toBe(413); + expect(res.body.code).toBe('entity_too_large'); + + db.deleteRoleAssignmentsByUser(viewerId); + }); + + it('allows global node-admin to write on remote without elevation gate', async () => { + const db = (await import('../services/DatabaseService')).DatabaseService.getInstance(); + // The viewer already has role viewer; create a global node-admin + const nodeAdminHash = await bcrypt.hash('nodeadmin123', 1); + const nodeAdminId = db.addUser({ + username: 'settings-global-na', + password_hash: nodeAdminHash, + role: 'node-admin', + }); + const nodeAdmin = db.getUserByUsername('settings-global-na')!; + const nodeAdminBearer = jwt.sign( + { username: 'settings-global-na', role: 'node-admin', tv: nodeAdmin.token_version }, + TEST_JWT_SECRET, + { expiresIn: '5m' }, + ); + + const res = await request(app) + .patch('/api/settings') + .set('Authorization', `Bearer ${nodeAdminBearer}`) + .set('x-node-id', String(grantedNodeId)) + .send({ host_cpu_limit: 90 }); + + expect(res.status).toBe(200); + + db.deleteUser(nodeAdminId); + }); + + it('allows scoped node-admin POST single key on granted node', async () => { + const db = (await import('../services/DatabaseService')).DatabaseService.getInstance(); + db.addRoleAssignment({ + user_id: viewerId, + role: 'node-admin', + resource_type: 'node', + resource_id: String(grantedNodeId), + }); + + const res = await request(app) + .post('/api/settings') + .set('Authorization', `Bearer ${viewerBearer}`) + .set('x-node-id', String(grantedNodeId)) + .send({ key: 'host_cpu_limit', value: 95 }); + + expect(res.status).toBe(200); + const hop = grantedHops.find((h) => h.url?.includes('/settings')); + expect(hop).toBeDefined(); + expect(hop!.roleHeader).toBe('node-admin'); + + db.deleteRoleAssignmentsByUser(viewerId); + }); +}); diff --git a/backend/src/__tests__/self-update-pinned-routes.test.ts b/backend/src/__tests__/self-update-pinned-routes.test.ts index 50da0aca..051de0bb 100644 --- a/backend/src/__tests__/self-update-pinned-routes.test.ts +++ b/backend/src/__tests__/self-update-pinned-routes.test.ts @@ -115,13 +115,14 @@ describe('POST /api/system/update', () => { expect(res.status).toBe(202); expect(res.body?.message).toMatch(/restart/i); - // triggerUpdate runs on res finish + delay; flush the microtask queue. - await new Promise(r => setTimeout(r, 600)); - expect(triggerSpy).toHaveBeenCalledWith(expect.objectContaining({ - targetVersion: '0.99.0', - successMarkerFile: expect.stringMatching(/image-op-success-[\w-]+\.json$/), - successMarkerContent: expect.stringMatching(/"operationId":"[\w-]+"/), - })); + // The route schedules triggerUpdate 500ms after responding, so poll for it. + await vi.waitFor(() => { + expect(triggerSpy).toHaveBeenCalledWith(expect.objectContaining({ + targetVersion: '0.99.0', + successMarkerFile: expect.stringMatching(/image-op-success-[\w-]+\.json$/), + successMarkerContent: expect.stringMatching(/"operationId":"[\w-]+"/), + })); + }, { timeout: 5_000 }); }); }); diff --git a/backend/src/proxy/remoteNodeProxy.ts b/backend/src/proxy/remoteNodeProxy.ts index 37a8545e..bbb29531 100644 --- a/backend/src/proxy/remoteNodeProxy.ts +++ b/backend/src/proxy/remoteNodeProxy.ts @@ -34,6 +34,8 @@ import { ROLE_PERMISSIONS, scopedActionsForStack, } from '../middleware/permissions'; +import type { PermissionAction } from '../middleware/permissions'; +import { SETTING_WRITE_PERMISSIONS } from '../routes/settings'; /** * Per-request hop timing for the critical hydration GETs, kept off the Request @@ -420,6 +422,59 @@ export function createRemoteProxyMiddleware(): RequestHandler { } } + // Settings-write pre-auth gate: when a non-admin, non-global-node-admin + // user writes settings on a remote node, the remote only sees the global + // role header and cannot verify scoped assignments. Check hub-side first + // and elevate PROXY_ROLE_HEADER to node-admin when the scoped check passes. + if (isSettingsWrite(req) && req.user?.role !== 'admin' && req.user?.role !== 'node-admin') { + if (hasNonIdentityContentEncoding(req)) { + await drainRequestBody(req); + res.status(415).json({ + error: 'Compressed request bodies are not supported for remote settings writes', + code: 'encoding_unsupported', + }); + return; + } + try { + req.rawBody = await bufferRequestBody(req, SETTINGS_PROXY_BODY_LIMIT); + } catch (err) { + const status = Number((err as { status?: number }).status); + if (status === 413) { + res.status(413).json({ error: 'Settings payload too large', code: 'entity_too_large' }); + return; + } + if (status === 400) { + res.status(400).json({ error: 'Incomplete request body' }); + return; + } + throw err; + } + const needed = settingsBodyPermissions(req.rawBody); + // Fail-closed on empty/unparseable body: require hub-side node:manage on + // the target node (mirrors requireSettingsWritePermission's empty-keys + // branch in routes/settings.ts:62-68). A user with no scoped grant is + // denied; a user with a scoped grant passes through elevated. + let preAuthOk = true; + if (needed.length === 0) { + preAuthOk = checkNodeManageOnHub(req); + } else { + for (const action of needed) { + const ok = action === 'node:manage' + ? checkNodeManageOnHub(req) + : checkPermission(req, action); + if (!ok) { + preAuthOk = false; + break; + } + } + } + if (!preAuthOk) { + res.status(403).json({ error: 'Permission denied.', code: 'PERMISSION_DENIED' }); + return; + } + req.proxyElevatedRole = 'node-admin'; + } + // Alerts POST scoped-evidence gate: when a non-admin, non-node-admin user // creates a stack-scoped alert on a remote node, forward the scoped grant // as evidence so the remote can authorize the write. The body was already @@ -539,6 +594,50 @@ export function createRemoteProxyMiddleware(): RequestHandler { }; } +/** Max request body size for buffered settings writes (same as ALERT_PROXY_BODY_LIMIT). */ +const SETTINGS_PROXY_BODY_LIMIT = 100 * 1024; + +/** True when the request is a settings write destined for a remote node (path is post-/api strip). */ +function isSettingsWrite(req: Request): boolean { + if (req.method !== 'POST' && req.method !== 'PATCH') return false; + return /^\/settings\/?$/.test(req.path); +} + +/** + * Hub-side `node:manage` resolve against the active node so scoped Node Admin + * grants on the target remote node are detected before the hop. + */ +function checkNodeManageOnHub(req: Request): boolean { + if (typeof req.nodeId === 'number') { + return checkPermission(req, 'node:manage', 'node', String(req.nodeId)); + } + return checkPermission(req, 'node:manage'); +} + +/** + * Extract the set of required PermissionAction values from a buffered settings + * body. Returns the distinct actions for a valid body, or an empty array when + * the body is empty or JSON.parse fails (caller must then fall back to requiring + * checkNodeManageOnHub, fail-closed). + */ +function settingsBodyPermissions(rawBody: Buffer): PermissionAction[] { + if (rawBody.length === 0) return []; + try { + const parsed = JSON.parse(rawBody.toString('utf-8')) as Record; + // POST /api/settings sends { key, value }; PATCH sends a flat key/value map. + const keys = typeof parsed.key === 'string' ? [parsed.key] : Object.keys(parsed); + if (keys.length === 0) return []; + const needed = new Set(); + for (const key of keys) { + const action = SETTING_WRITE_PERMISSIONS[key]; + if (action) needed.add(action); + } + return [...needed]; + } catch { + return []; + } +} + /** POST /stacks/:stackName/down with ?removeVolumes=true (path is post-/api strip). */ function isStackDownWithRemoveVolumes(req: Request): boolean { if (req.method !== 'POST') return false; @@ -578,17 +677,6 @@ function isImageRefreshNodeWide(req: Request): boolean { return req.method === 'POST' && /^\/image-updates\/refresh\/?$/.test(req.path); } -/** - * Hub-side node:manage resolve against the active node so scoped Node Admin - * grants on the target remote node are detected before the hop. - */ -function checkNodeManageOnHub(req: Request): boolean { - if (typeof req.nodeId === 'number') { - return checkPermission(req, 'node:manage', 'node', String(req.nodeId)); - } - return checkPermission(req, 'node:manage'); -} - /** * Extract the stack_name from a buffered JSON POST body. * Returns a valid stack name, `null` when the field is absent or diff --git a/backend/src/routes/settings.ts b/backend/src/routes/settings.ts index fcbae7c9..51971e39 100644 --- a/backend/src/routes/settings.ts +++ b/backend/src/routes/settings.ts @@ -11,7 +11,7 @@ import { parseNotificationDispatchRetries } from '../helpers/notificationDispatc // keys so secrets written to global_settings by other subsystems (cloud // backup credentials, auth_* login secrets) are never returned; writes // outside the map are rejected. -const SETTING_WRITE_PERMISSIONS: Record = { +export const SETTING_WRITE_PERMISSIONS: Record = { host_cpu_limit: 'node:manage', host_ram_limit: 'node:manage', host_disk_limit: 'node:manage', diff --git a/backend/src/types/express.ts b/backend/src/types/express.ts index 1b04e5e4..2292e2a0 100644 --- a/backend/src/types/express.ts +++ b/backend/src/types/express.ts @@ -45,10 +45,9 @@ declare global { */ proxyNamedStackRoute?: { stackName: string; action: PermissionAction }; /** - * Hub-side role override for the outbound proxy hop. When set, the - * PROXY_ROLE_HEADER is elevated to node-admin instead of the caller's - * global role, so scoped node-level grants can be forwarded. - * Request-scoped; consumed only by remoteNodeProxy.ts. + * Elevated role for a single proxied request. Set by the settings + * pre-authorization gate when the hub-side scoped permission check + * passes for a non-admin user. Resets to undefined after the hop. */ proxyElevatedRole?: 'node-admin'; }