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 (