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.
This commit is contained in:
Anso
2026-06-04 01:49:23 -04:00
committed by GitHub
parent 4596a90474
commit 0683aa9395
10 changed files with 230 additions and 77 deletions
@@ -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');
}
});
});
+36 -1
View File
@@ -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 () => {
+2 -1
View File
@@ -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);
+21 -1
View File
@@ -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<void> => {
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) {
@@ -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 <SectionSkeleton />;
return (
<div className="flex flex-col gap-10">
<fieldset disabled={readOnly} className="m-0 flex min-w-0 flex-col gap-10 border-0 p-0">
<SettingsSection title="Default registry">
<SettingsField
label="LinuxServer.io"
@@ -100,29 +103,31 @@ export function AppStoreSection() {
/>
</SettingsField>
<SettingsActions align="between" hint={templateRegistryUrl ? 'using custom registry' : 'using default'}>
<div className="flex items-center gap-2">
<Button
variant="outline"
size="sm"
onClick={() => setTemplateRegistryUrl('')}
disabled={isSavingRegistry || !templateRegistryUrl}
>
Reset to default
</Button>
<SettingsPrimaryButton onClick={saveRegistrySettings} disabled={isSavingRegistry}>
{isSavingRegistry ? (
<>
<RefreshCw className="w-4 h-4 animate-spin" />
Saving
</>
) : (
'Save & refresh'
)}
</SettingsPrimaryButton>
</div>
<SettingsActions align="between" hint={readOnly ? 'Read-only · admin access required to edit' : (templateRegistryUrl ? 'using custom registry' : 'using default')}>
{!readOnly && (
<div className="flex items-center gap-2">
<Button
variant="outline"
size="sm"
onClick={() => setTemplateRegistryUrl('')}
disabled={isSavingRegistry || !templateRegistryUrl}
>
Reset to default
</Button>
<SettingsPrimaryButton onClick={saveRegistrySettings} disabled={isSavingRegistry}>
{isSavingRegistry ? (
<>
<RefreshCw className="w-4 h-4 animate-spin" />
Saving
</>
) : (
'Save & refresh'
)}
</SettingsPrimaryButton>
</div>
)}
</SettingsActions>
</SettingsSection>
</div>
</fieldset>
);
}
@@ -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<DeveloperFields>({ ...DEFAULT_DEVELOPER });
const serverSettingsRef = useRef<DeveloperFields>({ ...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<string, string> = nodeRes.ok ? await nodeRes.json() : {};
const localData: Record<string, string> = (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 <SectionSkeleton />;
return (
<div className="flex flex-col gap-10">
<fieldset disabled={readOnly} className="m-0 flex min-w-0 flex-col gap-10 border-0 p-0">
<SettingsSection title="Diagnostics">
<SettingsField
label="Developer mode"
@@ -206,7 +202,7 @@ export function DeveloperSection({ onDirtyChange }: DeveloperSectionProps) {
</div>
</SettingsField>
{isPaid && license?.variant === 'admiral' && (
{isAdmiral && (
<SettingsField
label="Audit log"
helper="How long to keep audit trail entries."
@@ -226,18 +222,20 @@ export function DeveloperSection({ onDirtyChange }: DeveloperSectionProps) {
)}
</SettingsSection>
<SettingsActions hint={hasChanges ? 'unsaved changes' : undefined}>
<SettingsPrimaryButton onClick={saveSettings} disabled={isSaving || !hasChanges}>
{isSaving ? (
<>
<RefreshCw className="w-4 h-4 animate-spin" />
Saving
</>
) : (
'Save settings'
)}
</SettingsPrimaryButton>
<SettingsActions hint={readOnly ? 'Read-only · admin access required to edit' : (hasChanges ? 'unsaved changes' : undefined)}>
{!readOnly && (
<SettingsPrimaryButton onClick={saveSettings} disabled={isSaving || !hasChanges}>
{isSaving ? (
<>
<RefreshCw className="w-4 h-4 animate-spin" />
Saving
</>
) : (
'Save settings'
)}
</SettingsPrimaryButton>
)}
</SettingsActions>
</div>
</fieldset>
);
}
@@ -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 = {
@@ -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],
@@ -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 = {
@@ -89,7 +89,7 @@ function NumberChip({ value, onChange, suffix, min, max, step = 1, warnOver }: N
return (
<button
type="button"
className={cn(chipClass, 'focus-visible:ring-2 focus-visible:ring-brand/50 focus-visible:outline-none')}
className={cn(chipClass, 'focus-visible:ring-2 focus-visible:ring-brand/50 focus-visible:outline-none disabled:opacity-50 disabled:cursor-not-allowed')}
onClick={startEdit}
>
<span>{value || '0'}</span>
@@ -111,7 +111,7 @@ function TogglePill({ checked, onChange }: TogglePillProps) {
aria-checked={checked}
onClick={() => onChange(!checked)}
className={cn(
'inline-flex items-center justify-center rounded-md border px-2.5 py-1 font-mono text-xs uppercase tracking-[0.18em] transition-colors min-w-[60px] focus-visible:ring-2 focus-visible:ring-brand/50 focus-visible:outline-none',
'inline-flex items-center justify-center rounded-md border px-2.5 py-1 font-mono text-xs uppercase tracking-[0.18em] transition-colors min-w-[60px] focus-visible:ring-2 focus-visible:ring-brand/50 focus-visible:outline-none disabled:opacity-50 disabled:cursor-not-allowed',
checked
? 'border-success/30 bg-success/10 text-success hover:bg-success/15'
: 'border-card-border bg-card text-stat-subtitle hover:text-stat-value',
@@ -148,6 +148,7 @@ const DEFAULT_SYSTEM: SystemFields = {
export function SystemSection({ onDirtyChange }: SystemSectionProps) {
const { activeNode } = useNodes();
const { isAdmin } = useAuth();
const readOnly = !isAdmin;
const [settings, setSettings] = useState<SystemFields>({ ...DEFAULT_SYSTEM });
const serverSettingsRef = useRef<SystemFields>({ ...DEFAULT_SYSTEM });
const [isLoading, setIsLoading] = useState(false);
@@ -239,7 +240,7 @@ export function SystemSection({ onDirtyChange }: SystemSectionProps) {
if (isLoading) return <SettingsSkeleton />;
return (
<div className="flex flex-col gap-10">
<fieldset disabled={readOnly} className="m-0 flex min-w-0 flex-col gap-10 border-0 p-0">
<SettingsSection title="Host thresholds">
<SettingsField
label="CPU limit"
@@ -342,18 +343,20 @@ export function SystemSection({ onDirtyChange }: SystemSectionProps) {
</SettingsSection>
)}
<SettingsActions hint={hasChanges ? `${dirtyCount} unsaved` : undefined}>
<SettingsPrimaryButton onClick={saveSettings} disabled={isSaving || !hasChanges}>
{isSaving ? (
<>
<RefreshCw className="w-4 h-4 animate-spin" />
Saving
</>
) : (
'Save limits'
)}
</SettingsPrimaryButton>
<SettingsActions hint={readOnly ? 'Read-only · admin access required to edit' : (hasChanges ? `${dirtyCount} unsaved` : undefined)}>
{!readOnly && (
<SettingsPrimaryButton onClick={saveSettings} disabled={isSaving || !hasChanges}>
{isSaving ? (
<>
<RefreshCw className="w-4 h-4 animate-spin" />
Saving
</>
) : (
'Save limits'
)}
</SettingsPrimaryButton>
)}
</SettingsActions>
</div>
</fieldset>
);
}