From 0683aa93955f25071accfe6c78e76dcf8d078003 Mon Sep 17 00:00:00 2001 From: Anso Date: Thu, 4 Jun 2026 01:49:23 -0400 Subject: [PATCH] fix(settings): reflect role, tier, and node scope in settings panels (#1300) * fix(settings): reflect role, tier, and node scope in settings panels Three gate fixes so the Settings panels match what the backend enforces. Non-admin roles saw editable fields and a Save button on System Limits, Developer, and App Store, but writes require admin, so Save always failed. These panels now render read-only for non-admins (controls disabled, Save hidden) while still showing the values. The Developer panel is node-scoped but read and wrote the controlling instance regardless of the selected node, so a remote node's debug mode and retention windows could not be changed from the UI. It now targets the active node like System Limits. The settings shell derived the Admiral entitlement locally; it now consumes the backend-provided value the API authorizes against, and that value is corrected to require an active paid tier so an expired Admiral license no longer reports as Admiral. * fix(settings): gate audit_retention_days writes behind Admiral audit_retention_days configures the Admiral-only audit log (the audit-log routes require Admiral, and the Developer settings UI only shows the field to Admiral operators), but the settings POST/PATCH handlers only required an admin role. A non-Admiral admin (for example Skipper, or an expired-Admiral admin whose tier dropped to community) could still set it through the API. Gate writes to that key with requireAdmiral on both the single-key POST and the bulk PATCH paths, matching the audit-log routes and the UI. Other keys remain writable by any admin. * fix(settings): reject unknown keys on PATCH instead of silently stripping The bulk settings PATCH validated the body with a Zod object schema that strips unknown keys by default, so a request carrying a disallowed key (for example an auth_* secret) returned 200 as a no-op instead of being rejected. No secret was written, but it diverged from the single-key POST path, which rejects disallowed keys, and could hide client drift. PATCH now rejects any key outside the allowlist with a 400 before validation or write, keeping the bulk path fail-closed and consistent with POST. --- backend/src/__tests__/settings-routes.test.ts | 91 +++++++++++++++++++ backend/src/__tests__/users-rbac.test.ts | 37 +++++++- backend/src/routes/permissions.ts | 3 +- backend/src/routes/settings.ts | 22 ++++- .../components/settings/AppStoreSection.tsx | 51 ++++++----- .../components/settings/DeveloperSection.tsx | 52 +++++------ .../src/components/settings/SectionGate.tsx | 6 +- .../src/components/settings/SettingsPage.tsx | 6 +- .../components/settings/SettingsSidebar.tsx | 6 +- .../src/components/settings/SystemSection.tsx | 33 ++++--- 10 files changed, 230 insertions(+), 77 deletions(-) diff --git a/backend/src/__tests__/settings-routes.test.ts b/backend/src/__tests__/settings-routes.test.ts index 5618e811..20f98009 100644 --- a/backend/src/__tests__/settings-routes.test.ts +++ b/backend/src/__tests__/settings-routes.test.ts @@ -249,4 +249,95 @@ describe('PATCH /api/settings (bulk update)', () => { .send({}); expect(res.status).toBe(200); }); + + it('rejects a PATCH with unknown or disallowed keys (400) and writes nothing', async () => { + const before = DatabaseService.getInstance().getGlobalSettings().host_cpu_limit; + const res = await request(app) + .patch('/api/settings') + .set('Cookie', adminCookie) + .send({ host_cpu_limit: 65, auth_jwt_secret: 'pwned' }); + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/Invalid or disallowed setting key/); + // Fail-closed: the allowlisted key in the same body must not be written. + expect(DatabaseService.getInstance().getGlobalSettings().host_cpu_limit).toBe(before); + }); + + it('rejects a PATCH whose only key is a private auth secret (was a silent 200 no-op before)', async () => { + const res = await request(app) + .patch('/api/settings') + .set('Cookie', adminCookie) + .send({ auth_jwt_secret: 'pwned' }); + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/Invalid or disallowed setting key/); + // The auth secret must never be written through the settings API. + expect(DatabaseService.getInstance().getGlobalSettings().auth_jwt_secret).not.toBe('pwned'); + }); +}); + +describe('Admiral-only setting keys (audit_retention_days)', () => { + // audit_retention_days configures the Admiral-only audit log, so its write is + // gated by requireAdmiral in addition to the admin role. beforeAll mocks a + // paid Admiral license; individual tests override the variant to simulate an + // admin whose license is not Admiral. + it('allows an Admiral admin to write audit_retention_days', async () => { + const res = await request(app) + .patch('/api/settings') + .set('Cookie', adminCookie) + .send({ audit_retention_days: 120 }); + expect(res.status).toBe(200); + expect(DatabaseService.getInstance().getGlobalSettings().audit_retention_days).toBe('120'); + }); + + it('rejects an audit_retention_days PATCH from a non-Admiral admin (403) and does not apply it', async () => { + const before = DatabaseService.getInstance().getGlobalSettings().audit_retention_days; + const { LicenseService } = await import('../services/LicenseService'); + const spy = vi.spyOn(LicenseService.getInstance(), 'getVariant').mockReturnValue('skipper'); + try { + const res = await request(app) + .patch('/api/settings') + .set('Cookie', adminCookie) + .send({ audit_retention_days: 200 }); + expect(res.status).toBe(403); + expect(res.body.code).toBe('ADMIRAL_REQUIRED'); + expect(DatabaseService.getInstance().getGlobalSettings().audit_retention_days).toBe(before); + } finally { + spy.mockReturnValue('admiral'); + } + }); + + it('rejects an audit_retention_days single-key POST from a non-Admiral admin (403) and does not apply it', async () => { + const before = DatabaseService.getInstance().getGlobalSettings().audit_retention_days; + const { LicenseService } = await import('../services/LicenseService'); + const spy = vi.spyOn(LicenseService.getInstance(), 'getVariant').mockReturnValue('skipper'); + try { + const res = await request(app) + .post('/api/settings') + .set('Cookie', adminCookie) + .send({ key: 'audit_retention_days', value: '300' }); + expect(res.status).toBe(403); + expect(res.body.code).toBe('ADMIRAL_REQUIRED'); + expect(DatabaseService.getInstance().getGlobalSettings().audit_retention_days).toBe(before); + } finally { + spy.mockReturnValue('admiral'); + } + }); + + it('still lets a non-Admiral admin write non-Admiral keys via PATCH and POST (gate is per-key)', async () => { + const { LicenseService } = await import('../services/LicenseService'); + const spy = vi.spyOn(LicenseService.getInstance(), 'getVariant').mockReturnValue('skipper'); + try { + const patchRes = await request(app) + .patch('/api/settings') + .set('Cookie', adminCookie) + .send({ host_cpu_limit: 55 }); + expect(patchRes.status).toBe(200); + const postRes = await request(app) + .post('/api/settings') + .set('Cookie', adminCookie) + .send({ key: 'host_ram_limit', value: '55' }); + expect(postRes.status).toBe(200); + } finally { + spy.mockReturnValue('admiral'); + } + }); }); diff --git a/backend/src/__tests__/users-rbac.test.ts b/backend/src/__tests__/users-rbac.test.ts index b539c14c..2604f9d9 100644 --- a/backend/src/__tests__/users-rbac.test.ts +++ b/backend/src/__tests__/users-rbac.test.ts @@ -401,7 +401,42 @@ describe('GET /api/permissions/me', () => { expect(Array.isArray(res.body.globalPermissions)).toBe(true); expect(res.body.globalPermissions).toContain('stack:read'); expect(res.body.globalPermissions).toContain('system:users'); - expect(typeof res.body.isAdmiral).toBe('boolean'); + // beforeAll mocks a paid admiral license. + expect(res.body.isAdmiral).toBe(true); + }); + + it('reports isAdmiral=false when the admiral variant is no longer on a paid tier', async () => { + // An expired or downgraded admiral license keeps variant='admiral' but the + // effective tier drops to community. isAdmiral must track the effective tier + // (mirroring the requireAdmiral guard), not the lingering variant, or the + // frontend would unlock admiral-only surfaces that the API then 403s. + const { LicenseService } = await import('../services/LicenseService'); + const svc = LicenseService.getInstance(); + vi.spyOn(svc, 'getTier').mockReturnValue('community'); + try { + const res = await request(app) + .get('/api/permissions/me') + .set('Authorization', `Bearer ${adminToken()}`); + expect(res.status).toBe(200); + expect(res.body.isAdmiral).toBe(false); + } finally { + vi.spyOn(svc, 'getTier').mockReturnValue('paid'); + } + }); + + it('reports isAdmiral=false for a paid non-admiral (skipper) license', async () => { + const { LicenseService } = await import('../services/LicenseService'); + const svc = LicenseService.getInstance(); + vi.spyOn(svc, 'getVariant').mockReturnValue('skipper'); + try { + const res = await request(app) + .get('/api/permissions/me') + .set('Authorization', `Bearer ${adminToken()}`); + expect(res.status).toBe(200); + expect(res.body.isAdmiral).toBe(false); + } finally { + vi.spyOn(svc, 'getVariant').mockReturnValue('admiral'); + } }); it('returns 401 when not authenticated', async () => { diff --git a/backend/src/routes/permissions.ts b/backend/src/routes/permissions.ts index 8dfb9872..be56f0c1 100644 --- a/backend/src/routes/permissions.ts +++ b/backend/src/routes/permissions.ts @@ -30,7 +30,8 @@ permissionsRouter.get('/me', authMiddleware, (req: Request, res: Response): void globalRole, globalPermissions, scopedPermissions, - isAdmiral: LicenseService.getInstance().getVariant() === 'admiral', + isAdmiral: LicenseService.getInstance().getTier() === 'paid' + && LicenseService.getInstance().getVariant() === 'admiral', }); } catch (error) { console.error('[Permissions] Error:', error); diff --git a/backend/src/routes/settings.ts b/backend/src/routes/settings.ts index 5cbe45fa..b82c0938 100644 --- a/backend/src/routes/settings.ts +++ b/backend/src/routes/settings.ts @@ -2,7 +2,7 @@ import { Router, type Request, type Response } from 'express'; import { z } from 'zod'; import { DatabaseService } from '../services/DatabaseService'; import { authMiddleware } from '../middleware/auth'; -import { requireAdmin } from '../middleware/tierGates'; +import { requireAdmin, requireAdmiral } from '../middleware/tierGates'; // Strict allowlist of keys readable and writable via the generic settings // API. This is the single source of truth for what the endpoint exposes: @@ -26,6 +26,12 @@ const ALLOWED_SETTING_KEYS = new Set([ 'scan_history_per_image_limit', ]); +// Keys whose write requires the Admiral variant, not just an admin role. +// audit_retention_days configures the Admiral-only audit log (the audit-log +// routes are requireAdmiral and the UI only shows this field to Admiral +// operators), so a lower-tier admin must not be able to set it. +const ADMIRAL_ONLY_SETTING_KEYS = new Set(['audit_retention_days']); + // Bulk PATCH schema. All keys optional; present keys are fully validated. const SettingsPatchSchema = z.object({ host_cpu_limit: z.coerce.number().int().min(1).max(100).transform(String), @@ -71,6 +77,7 @@ settingsRouter.post('/', authMiddleware, async (req: Request, res: Response): Pr res.status(400).json({ error: `Invalid or disallowed setting key: ${key}` }); return; } + if (ADMIRAL_ONLY_SETTING_KEYS.has(key) && !requireAdmiral(req, res)) return; if (value === undefined || value === null) { res.status(400).json({ error: 'Setting value is required' }); return; @@ -109,11 +116,24 @@ settingsRouter.post('/', authMiddleware, async (req: Request, res: Response): Pr settingsRouter.patch('/', authMiddleware, async (req: Request, res: Response): Promise => { if (!requireAdmin(req, res)) return; try { + // Reject unknown/disallowed keys outright rather than letting Zod silently + // strip them. This keeps the bulk path fail-closed and consistent with the + // single-key POST, so a client sending a stale or disallowed key (e.g. an + // auth_* secret) gets a 400, not a misleading 200. + const body = req.body; + if (body && typeof body === 'object' && !Array.isArray(body)) { + const unknownKeys = Object.keys(body).filter(k => !ALLOWED_SETTING_KEYS.has(k)); + if (unknownKeys.length > 0) { + res.status(400).json({ error: `Invalid or disallowed setting key(s): ${unknownKeys.join(', ')}` }); + return; + } + } const parsed = SettingsPatchSchema.safeParse(req.body); if (!parsed.success) { res.status(400).json({ error: 'Validation failed', details: parsed.error.flatten().fieldErrors }); return; } + if (Object.keys(parsed.data).some(k => ADMIRAL_ONLY_SETTING_KEYS.has(k)) && !requireAdmiral(req, res)) return; const db = DatabaseService.getInstance(); const updateMany = db.getDb().transaction((entries: [string, string][]) => { for (const [k, v] of entries) { diff --git a/frontend/src/components/settings/AppStoreSection.tsx b/frontend/src/components/settings/AppStoreSection.tsx index 5e4c612f..8780e6af 100644 --- a/frontend/src/components/settings/AppStoreSection.tsx +++ b/frontend/src/components/settings/AppStoreSection.tsx @@ -4,6 +4,7 @@ import { Input } from '@/components/ui/input'; import { Skeleton } from '@/components/ui/skeleton'; import { toast } from '@/components/ui/toast-store'; import { apiFetch } from '@/lib/api'; +import { useAuth } from '@/context/AuthContext'; import { RefreshCw } from 'lucide-react'; import { SettingsSection } from './SettingsSection'; import { SettingsField } from './SettingsField'; @@ -19,6 +20,8 @@ function SectionSkeleton() { } export function AppStoreSection() { + const { isAdmin } = useAuth(); + const readOnly = !isAdmin; const [templateRegistryUrl, setTemplateRegistryUrl] = useState(''); const serverUrl = useRef(''); const [isLoading, setIsLoading] = useState(false); @@ -74,7 +77,7 @@ export function AppStoreSection() { if (isLoading) return ; return ( -
+
- -
- - - {isSavingRegistry ? ( - <> - - Saving - - ) : ( - 'Save & refresh' - )} - -
+ + {!readOnly && ( +
+ + + {isSavingRegistry ? ( + <> + + Saving + + ) : ( + 'Save & refresh' + )} + +
+ )} -
+ ); } diff --git a/frontend/src/components/settings/DeveloperSection.tsx b/frontend/src/components/settings/DeveloperSection.tsx index 8d55f881..8d199746 100644 --- a/frontend/src/components/settings/DeveloperSection.tsx +++ b/frontend/src/components/settings/DeveloperSection.tsx @@ -2,7 +2,7 @@ import { useState, useRef, useEffect } from 'react'; import { Input } from '@/components/ui/input'; import { TogglePill } from '@/components/ui/toggle-pill'; import { Skeleton } from '@/components/ui/skeleton'; -import { useLicense } from '@/context/LicenseContext'; +import { useAuth } from '@/context/AuthContext'; import { RefreshCw } from 'lucide-react'; import { apiFetch } from '@/lib/api'; import { toast } from '@/components/ui/toast-store'; @@ -41,8 +41,10 @@ const DEFAULT_DEVELOPER: DeveloperFields = { }; export function DeveloperSection({ onDirtyChange }: DeveloperSectionProps) { - const { isPaid, license } = useLicense(); + const { isAdmin, permissions } = useAuth(); const { activeNode } = useNodes(); + const readOnly = !isAdmin; + const isAdmiral = permissions?.isAdmiral ?? false; const [settings, setSettings] = useState({ ...DEFAULT_DEVELOPER }); const serverSettingsRef = useRef({ ...DEFAULT_DEVELOPER }); const [isLoading, setIsLoading] = useState(false); @@ -75,19 +77,14 @@ export function DeveloperSection({ onDirtyChange }: DeveloperSectionProps) { const fetchSettings = async () => { setIsLoading(true); try { - const isRemote = activeNode?.type === 'remote'; const nodeRes = await apiFetch('/settings'); - const localRes = isRemote ? await apiFetch('/settings', { localOnly: true }) : nodeRes; const nodeData: Record = nodeRes.ok ? await nodeRes.json() : {}; - const localData: Record = (isRemote && localRes.ok) - ? await localRes.json() - : nodeData; const safe: DeveloperFields = { - developer_mode: (localData.developer_mode as '0' | '1') ?? DEFAULT_SETTINGS.developer_mode, - metrics_retention_hours: localData.metrics_retention_hours ?? DEFAULT_SETTINGS.metrics_retention_hours, - log_retention_days: localData.log_retention_days ?? DEFAULT_SETTINGS.log_retention_days, - audit_retention_days: localData.audit_retention_days ?? DEFAULT_SETTINGS.audit_retention_days, - scan_history_per_image_limit: localData.scan_history_per_image_limit ?? DEFAULT_SETTINGS.scan_history_per_image_limit, + developer_mode: (nodeData.developer_mode as '0' | '1') ?? DEFAULT_SETTINGS.developer_mode, + metrics_retention_hours: nodeData.metrics_retention_hours ?? DEFAULT_SETTINGS.metrics_retention_hours, + log_retention_days: nodeData.log_retention_days ?? DEFAULT_SETTINGS.log_retention_days, + audit_retention_days: nodeData.audit_retention_days ?? DEFAULT_SETTINGS.audit_retention_days, + scan_history_per_image_limit: nodeData.scan_history_per_image_limit ?? DEFAULT_SETTINGS.scan_history_per_image_limit, }; setSettings(safe); serverSettingsRef.current = { ...safe }; @@ -118,7 +115,6 @@ export function DeveloperSection({ onDirtyChange }: DeveloperSectionProps) { const res = await apiFetch('/settings', { method: 'PATCH', body: JSON.stringify(payload), - localOnly: true, }); if (!res.ok) { const err = await res.json().catch(() => ({})); @@ -140,7 +136,7 @@ export function DeveloperSection({ onDirtyChange }: DeveloperSectionProps) { if (isLoading) return ; return ( -
+
- {isPaid && license?.variant === 'admiral' && ( + {isAdmiral && ( - - - {isSaving ? ( - <> - - Saving - - ) : ( - 'Save settings' - )} - + + {!readOnly && ( + + {isSaving ? ( + <> + + Saving + + ) : ( + 'Save settings' + )} + + )} -
+ ); } diff --git a/frontend/src/components/settings/SectionGate.tsx b/frontend/src/components/settings/SectionGate.tsx index b17a96e6..b667fd8e 100644 --- a/frontend/src/components/settings/SectionGate.tsx +++ b/frontend/src/components/settings/SectionGate.tsx @@ -18,11 +18,11 @@ interface SectionGateProps { * guards remain the authoritative enforcement. */ export function SectionGate({ sectionId, children }: SectionGateProps) { - const { isAdmin } = useAuth(); - const { isPaid, license } = useLicense(); + const { isAdmin, permissions } = useAuth(); + const { isPaid } = useLicense(); const { activeNode } = useNodes(); - const isAdmiral = isPaid && license?.variant === 'admiral'; + const isAdmiral = permissions?.isAdmiral ?? false; const isRemote = activeNode?.type === 'remote'; const visibility: VisibilityContext = { diff --git a/frontend/src/components/settings/SettingsPage.tsx b/frontend/src/components/settings/SettingsPage.tsx index f0f0ab0a..5580375b 100644 --- a/frontend/src/components/settings/SettingsPage.tsx +++ b/frontend/src/components/settings/SettingsPage.tsx @@ -100,11 +100,11 @@ export function SettingsPage(props: SettingsPageProps) { } function SettingsPageInner({ currentSection, onSectionChange }: SettingsPageProps) { - const { isAdmin } = useAuth(); - const { isPaid, license } = useLicense(); + const { isAdmin, permissions } = useAuth(); + const { isPaid } = useLicense(); const { activeNode } = useNodes(); const isRemote = activeNode?.type === 'remote'; - const isAdmiral = isPaid && license?.variant === 'admiral'; + const isAdmiral = permissions?.isAdmiral ?? false; const visibility: VisibilityContext = useMemo( () => ({ isRemote, isAdmin, isPaid, isAdmiral }), [isRemote, isAdmin, isPaid, isAdmiral], diff --git a/frontend/src/components/settings/SettingsSidebar.tsx b/frontend/src/components/settings/SettingsSidebar.tsx index bb83f32c..4ffbd1ce 100644 --- a/frontend/src/components/settings/SettingsSidebar.tsx +++ b/frontend/src/components/settings/SettingsSidebar.tsx @@ -16,11 +16,11 @@ interface SettingsSidebarProps { } export function SettingsSidebar({ currentSection, onSectionChange, dirtyFlags, onOpenPalette }: SettingsSidebarProps) { - const { isAdmin } = useAuth(); - const { isPaid, license } = useLicense(); + const { isAdmin, permissions } = useAuth(); + const { isPaid } = useLicense(); const { activeNode } = useNodes(); - const isAdmiral = isPaid && license?.variant === 'admiral'; + const isAdmiral = permissions?.isAdmiral ?? false; const isRemote = activeNode?.type === 'remote'; const visibility: VisibilityContext = { diff --git a/frontend/src/components/settings/SystemSection.tsx b/frontend/src/components/settings/SystemSection.tsx index 43f9f30c..3c7edcdf 100644 --- a/frontend/src/components/settings/SystemSection.tsx +++ b/frontend/src/components/settings/SystemSection.tsx @@ -89,7 +89,7 @@ function NumberChip({ value, onChange, suffix, min, max, step = 1, warnOver }: N return (