mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-20 09:09:35 +02:00
fix(multi-user): keep the admin's Deepgram key out of a non-admin's settings read
GET /api/settings returned the whole settings.json to every signed-in user. An admin's App Settings save stores the Deepgram key there (voiceSettings.apiKey), so any regular user could read it, and their browser copied it into its own voice config and dictated on the admin's account. caseOrder and lastUsedCase also named the admin's cases. In multi-user mode a non-admin now reads the file without those three (settingsForNonAdmin). Admins and single-user mode get the file as it is. Pinned by route tests and the live multi-user test. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
1 parent
31c03ab62d
commit
33f0bc423f
3 files changed
+94
-7
No files matched your search
@@ -17,7 +17,7 @@ import { ApiErrorCode, createErrorResponse, getErrorMessage, type NiceConfig } f
|
|||||||
import { isUnauthenticatedNetworkAcknowledged } from '../network-auth-policy.js';
|
import { isUnauthenticatedNetworkAcknowledged } from '../network-auth-policy.js';
|
||||||
import { isMultiUserMode } from '../../config/multiuser.js';
|
import { isMultiUserMode } from '../../config/multiuser.js';
|
||||||
import { findUser, canUsernameRunPrivilegedCommands } from '../../user-store.js';
|
import { findUser, canUsernameRunPrivilegedCommands } from '../../user-store.js';
|
||||||
import { getAuthUser, requireAdmin, canAccessOwned } from '../route-helpers.js';
|
import { getAuthUser, isAdmin, requireAdmin, canAccessOwned } from '../route-helpers.js';
|
||||||
import {
|
import {
|
||||||
ConfigUpdateSchema,
|
ConfigUpdateSchema,
|
||||||
SettingsUpdateSchema,
|
SettingsUpdateSchema,
|
||||||
@@ -144,6 +144,28 @@ export function resolveSpanUrl(hostHeader: string | undefined, fallbackPort = '3
|
|||||||
return `http://localhost:${port}`;
|
return `http://localhost:${port}`;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* What GET /api/settings answers a NON-ADMIN in multi-user mode: settings.json minus what
|
||||||
|
* belongs to the server's admin rather than to the reader. `voiceSettings.apiKey` is the
|
||||||
|
* Deepgram key an admin saved, a credential (the reader's browser seeded it into its own
|
||||||
|
* voice config and dictated on the admin's account); `caseOrder` and `lastUsedCase` name the
|
||||||
|
* admin's cases. Everything else stays readable, because the page reads the synced feature
|
||||||
|
* switches from it. Writes are admin-only there (PUT /api/settings). Exported for unit testing.
|
||||||
|
*/
|
||||||
|
export function settingsForNonAdmin(settings: unknown): unknown {
|
||||||
|
if (!settings || typeof settings !== 'object' || Array.isArray(settings)) return settings;
|
||||||
|
const out: Record<string, unknown> = { ...(settings as Record<string, unknown>) };
|
||||||
|
delete out.caseOrder;
|
||||||
|
delete out.lastUsedCase;
|
||||||
|
const voice = out.voiceSettings;
|
||||||
|
if (voice && typeof voice === 'object' && !Array.isArray(voice) && 'apiKey' in voice) {
|
||||||
|
const voiceOut: Record<string, unknown> = { ...(voice as Record<string, unknown>) };
|
||||||
|
delete voiceOut.apiKey;
|
||||||
|
out.voiceSettings = voiceOut;
|
||||||
|
}
|
||||||
|
return out;
|
||||||
|
}
|
||||||
|
|
||||||
export function registerSystemRoutes(
|
export function registerSystemRoutes(
|
||||||
app: FastifyInstance,
|
app: FastifyInstance,
|
||||||
ctx: SessionPort & EventPort & ConfigPort & InfraPort & AuthPort & TabLayoutPort
|
ctx: SessionPort & EventPort & ConfigPort & InfraPort & AuthPort & TabLayoutPort
|
||||||
@@ -941,7 +963,7 @@ export function registerSystemRoutes(
|
|||||||
|
|
||||||
// ========== Settings ==========
|
// ========== Settings ==========
|
||||||
|
|
||||||
app.get('/api/settings', async () => {
|
app.get('/api/settings', async (req) => {
|
||||||
// A plain read. This route must NEVER write settings.json: readJsonConfig()
|
// A plain read. This route must NEVER write settings.json: readJsonConfig()
|
||||||
// answers `{}` for ANY read failure (a parse error, EACCES, EMFILE, a read
|
// answers `{}` for ANY read failure (a parse error, EACCES, EMFILE, a read
|
||||||
// that lands inside PUT's non-atomic write), not only for a missing file,
|
// that lands inside PUT's non-atomic write), not only for a missing file,
|
||||||
@@ -953,7 +975,11 @@ export function registerSystemRoutes(
|
|||||||
// `workspaceHooksEnabled` means ON, and the client resolves its own display
|
// `workspaceHooksEnabled` means ON, and the client resolves its own display
|
||||||
// default through planUsageChipEnabled(). Pinned by
|
// default through planUsageChipEnabled(). Pinned by
|
||||||
// test/routes/system-routes-settings-get-plan-usage-default.test.ts.
|
// test/routes/system-routes-settings-get-plan-usage-default.test.ts.
|
||||||
return readJsonConfig(SETTINGS_PATH, 'settings', {});
|
const settings = await readJsonConfig<unknown>(SETTINGS_PATH, 'settings', {});
|
||||||
|
// Readable by every signed-in user, but a non-admin in multi-user mode gets it without
|
||||||
|
// the admin's credential and case names (settingsForNonAdmin). isAdmin is always true in
|
||||||
|
// single-user mode, so that answer is the file as it is.
|
||||||
|
return isAdmin(req) ? settings : settingsForNonAdmin(settings);
|
||||||
});
|
});
|
||||||
|
|
||||||
app.put('/api/settings', async (req, reply) => {
|
app.put('/api/settings', async (req, reply) => {
|
||||||
|
|||||||
@@ -10,14 +10,16 @@
|
|||||||
* tunnel, the tmux history limit);
|
* tunnel, the tmux history limit);
|
||||||
* - the same gate on the other server-wide writes: PUT /api/execution/model-config,
|
* - the same gate on the other server-wide writes: PUT /api/execution/model-config,
|
||||||
* PUT /api/config and PUT /api/cases/order;
|
* PUT /api/config and PUT /api/cases/order;
|
||||||
* - an admin, and single-user mode, write as before; GET /api/settings stays readable.
|
* - an admin, and single-user mode, write as before; GET /api/settings stays readable,
|
||||||
|
* but a non-admin reads it without the admin's Deepgram key and case names
|
||||||
|
* (settingsForNonAdmin).
|
||||||
*
|
*
|
||||||
* Uses app.inject(), so no real HTTP port is needed. Port: N/A.
|
* Uses app.inject(), so no real HTTP port is needed. Port: N/A.
|
||||||
*/
|
*/
|
||||||
|
|
||||||
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
|
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
|
||||||
import { createRouteTestHarness, type RouteTestHarness } from './_route-test-utils.js';
|
import { createRouteTestHarness, type RouteTestHarness } from './_route-test-utils.js';
|
||||||
import { registerSystemRoutes } from '../../src/web/routes/system-routes.js';
|
import { registerSystemRoutes, settingsForNonAdmin } from '../../src/web/routes/system-routes.js';
|
||||||
import { registerCaseRoutes } from '../../src/web/routes/case-routes.js';
|
import { registerCaseRoutes } from '../../src/web/routes/case-routes.js';
|
||||||
|
|
||||||
const { STORED, subagentWatcher, imageWatcher, workflowRunWatcher } = vi.hoisted(() => {
|
const { STORED, subagentWatcher, imageWatcher, workflowRunWatcher } = vi.hoisted(() => {
|
||||||
@@ -66,6 +68,7 @@ vi.mock('../../src/workflow-run-watcher.js', () => ({ workflowRunWatcher }));
|
|||||||
|
|
||||||
import fs from 'node:fs/promises';
|
import fs from 'node:fs/promises';
|
||||||
const writeFile = vi.mocked(fs.writeFile);
|
const writeFile = vi.mocked(fs.writeFile);
|
||||||
|
const readFile = vi.mocked(fs.readFile);
|
||||||
|
|
||||||
const NON_ADMIN = { username: 'bob', role: 'user' } as const;
|
const NON_ADMIN = { username: 'bob', role: 'user' } as const;
|
||||||
const ADMIN = { username: 'root', role: 'admin' } as const;
|
const ADMIN = { username: 'root', role: 'admin' } as const;
|
||||||
@@ -174,6 +177,54 @@ describe('server-wide settings writes are admin-only in multi-user mode', () =>
|
|||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe("GET /api/settings: what is the admin's stays the admin's", () => {
|
||||||
|
// An admin's App Settings save stores the Deepgram key (voiceSettings.apiKey) here, and
|
||||||
|
// the admin's own cases are named in caseOrder / lastUsedCase.
|
||||||
|
const WITH_SECRETS = {
|
||||||
|
autoNameSessions: true,
|
||||||
|
runMode: 'shell',
|
||||||
|
caseOrder: ['rootcase', 'secret-project'],
|
||||||
|
lastUsedCase: 'secret-project',
|
||||||
|
voiceSettings: { provider: 'deepgram', apiKey: 'dg-SECRET-probe-123', language: 'en' },
|
||||||
|
};
|
||||||
|
|
||||||
|
it('gives a non-admin the settings without the Deepgram key and the case names', async () => {
|
||||||
|
const { app } = await as(NON_ADMIN);
|
||||||
|
readFile.mockResolvedValueOnce(JSON.stringify(WITH_SECRETS));
|
||||||
|
const res = await app.inject({ method: 'GET', url: '/api/settings' });
|
||||||
|
expect(res.statusCode).toBe(200);
|
||||||
|
expect(res.body).not.toContain('dg-SECRET-probe-123');
|
||||||
|
expect(res.body).not.toContain('secret-project');
|
||||||
|
expect(JSON.parse(res.body)).toEqual({
|
||||||
|
autoNameSessions: true,
|
||||||
|
runMode: 'shell',
|
||||||
|
voiceSettings: { provider: 'deepgram', language: 'en' },
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
for (const [label, user] of [
|
||||||
|
['an admin in multi-user mode', ADMIN],
|
||||||
|
['single-user mode', null],
|
||||||
|
] as const) {
|
||||||
|
it(`gives ${label} the file as it is`, async () => {
|
||||||
|
const { app } = await as(user);
|
||||||
|
readFile.mockResolvedValueOnce(JSON.stringify(WITH_SECRETS));
|
||||||
|
const res = await app.inject({ method: 'GET', url: '/api/settings' });
|
||||||
|
expect(res.statusCode).toBe(200);
|
||||||
|
expect(JSON.parse(res.body)).toEqual(WITH_SECRETS);
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
it('settingsForNonAdmin leaves its input alone and passes a non-object through', () => {
|
||||||
|
const input = structuredClone(WITH_SECRETS);
|
||||||
|
settingsForNonAdmin(input);
|
||||||
|
expect(input).toEqual(WITH_SECRETS);
|
||||||
|
expect(settingsForNonAdmin([1, 2])).toEqual([1, 2]);
|
||||||
|
expect(settingsForNonAdmin(null)).toBe(null);
|
||||||
|
expect(settingsForNonAdmin({ voiceSettings: { language: 'de' } })).toEqual({ voiceSettings: { language: 'de' } });
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
describe('PUT /api/execution/model-config', () => {
|
describe('PUT /api/execution/model-config', () => {
|
||||||
const config = { defaultModel: 'opus', agentTypeOverrides: { explore: 'sonnet' } };
|
const config = { defaultModel: 'opus', agentTypeOverrides: { explore: 'sonnet' } };
|
||||||
|
|
||||||
|
|||||||
@@ -7,7 +7,8 @@
|
|||||||
* real file back, so "settings.json is unchanged" is asserted on bytes:
|
* real file back, so "settings.json is unchanged" is asserted on bytes:
|
||||||
* - PUT /api/settings, PUT /api/execution/model-config and PUT /api/cases/order are
|
* - PUT /api/settings, PUT /api/execution/model-config and PUT /api/cases/order are
|
||||||
* 403 FORBIDDEN for a non-admin, and the file is byte-identical afterwards;
|
* 403 FORBIDDEN for a non-admin, and the file is byte-identical afterwards;
|
||||||
* - GET /api/settings stays readable by the non-admin;
|
* - GET /api/settings stays readable by the non-admin, without the admin's Deepgram
|
||||||
|
* key and case names (an admin still reads the file as it is);
|
||||||
* - a non-admin's quick-start does not record its case as `lastUsedCase` (a key every
|
* - a non-admin's quick-start does not record its case as `lastUsedCase` (a key every
|
||||||
* user can read through GET /api/settings), while an admin's still does;
|
* user can read through GET /api/settings), while an admin's still does;
|
||||||
* - an admin's PUT /api/settings still writes.
|
* - an admin's PUT /api/settings still writes.
|
||||||
@@ -37,6 +38,7 @@ const INITIAL = {
|
|||||||
lastUsedCase: 'rootcase',
|
lastUsedCase: 'rootcase',
|
||||||
caseOrder: ['rootcase'],
|
caseOrder: ['rootcase'],
|
||||||
modelConfig: { defaultModel: 'sonnet' },
|
modelConfig: { defaultModel: 'sonnet' },
|
||||||
|
voiceSettings: { provider: 'deepgram', apiKey: 'dg-SECRET-live-456', language: 'en' },
|
||||||
};
|
};
|
||||||
const readSettingsBytes = () => fs.readFile(SETTINGS_PATH, 'utf-8');
|
const readSettingsBytes = () => fs.readFile(SETTINGS_PATH, 'utf-8');
|
||||||
|
|
||||||
@@ -101,10 +103,18 @@ describe('a non-admin and the server-wide settings.json', () => {
|
|||||||
expect(await readSettingsBytes()).toBe(before);
|
expect(await readSettingsBytes()).toBe(before);
|
||||||
});
|
});
|
||||||
|
|
||||||
it('can still read it', async () => {
|
it("can still read it, without the admin's Deepgram key and case names", async () => {
|
||||||
const res = await send('GET', '/api/settings', bob);
|
const res = await send('GET', '/api/settings', bob);
|
||||||
expect(res.status).toBe(200);
|
expect(res.status).toBe(200);
|
||||||
expect(res.body.data.autoNameSessions).toBe(false);
|
expect(res.body.data.autoNameSessions).toBe(false);
|
||||||
|
expect(JSON.stringify(res.body)).not.toContain('dg-SECRET-live-456');
|
||||||
|
expect(res.body.data.voiceSettings).toEqual({ provider: 'deepgram', language: 'en' });
|
||||||
|
expect('caseOrder' in res.body.data).toBe(false);
|
||||||
|
expect('lastUsedCase' in res.body.data).toBe(false);
|
||||||
|
|
||||||
|
const admin = await send('GET', '/api/settings', root);
|
||||||
|
expect(admin.body.data.voiceSettings.apiKey).toBe('dg-SECRET-live-456');
|
||||||
|
expect(admin.body.data.caseOrder).toEqual(['rootcase']);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("does not get its quick-start case recorded as everyone's lastUsedCase; an admin's still is", async () => {
|
it("does not get its quick-start case recorded as everyone's lastUsedCase; an admin's still is", async () => {
|
||||||
|
|||||||
Reference in new issue
Block a user