mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-05 15:09:42 +02:00
fix(custom-model): address fourth pre-merge review + merge upstream master (Ark0N)
Merged upstream/master (22 commits: reboot-restore recovery feature,
terminal keycode229 recovery work, install.sh/CLI-catalog generator
changes, CHANGELOG/version bump to 1.30.0) into this branch. No
conflicts; git auto-merged every overlapping file (CLAUDE.md,
docs/api-reference.md, app.js, index.html, styles.css, routes/index.ts,
session-routes.ts, schemas.ts, server.ts).
Two required fixes from the latest review:
1. privilegedEnvKeys widening (stock.ts) changes behaviour outside this
feature. The reviewer decided to keep both CLAUDE_CODE_MAX_CONTEXT_TOKENS
and CLAUDE_CONFIG_DIR listed (types.ts's rule that every traffic-
redirecting var this feature introduces must appear there stays
literally true), and asked for the real consequences documented
instead of hidden:
- Corrected session-env-clamp.ts's fileoverview, which stated the
opposite of what the code now does (reboot-restore's clamp call
used to be able to strip nothing for claude; it now strips a
persisted CLAUDE_CONFIG_DIR for a non-granted owner).
- Corrected the rationale comments in stock.ts: privilegedEnvKeys
has exactly one consumer (ownerClampedEnvKeys, feeding the
generic envOverrides clamp on create/quick-start/reboot-restore),
not the custom-model routes.
- Added a CLAUDE.md line to the CLAUDE_CONFIG_DIR gotcha covering
the admin-only-in-multi-user-mode and reboot-restore-strips-it
consequences.
- Added a "Claude multi-user clamp" test next to the existing
DeepSeek/OMP ones, pinning the new stripping behaviour.
2. GET .../running-status (custom-model-routes.ts) no longer passes
the raw llama-swap `cmd` field (the literal launch line, which can
carry model paths and --api-key) to the browser -- the frontend
only ever reads model/state, cmd exists solely for server-side
parseCtxFromCmd() during discovery. Added a test asserting the
response never contains cmd or a planted secret.
Also regenerated config/clis.stock.json and install.sh's catalogue
block (npm run generate:cli-catalog) to clear drift introduced by the
upstream merge, since it was failing the sync check.
Left to the reviewer, as they said they'd take at merge: the two
"comments pointing at removed code" cleanups, the two stale CLAUDE.md
counts, and the small items list (mode==='claude' frontend branch,
isCliAvailable() unknown-id gap, shared confirmed flag ordering,
one-shot cancel toast severity, pumpLlamaSwapLogTail buffer cap).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ea59JhUmHBm1gRCsiYF33R
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
56209e7829
commit
5fc391a47c
@@ -181,6 +181,34 @@ describe('custom model endpoint CRUD', () => {
|
||||
expect(res.json().error).toMatch(/refused.*169\.254\.169\.254/);
|
||||
});
|
||||
|
||||
it('running-status never hands the browser the raw llama-swap launch command (cmd)', async () => {
|
||||
const { app } = await setup();
|
||||
await app.inject({
|
||||
method: 'POST',
|
||||
url: '/api/model-endpoints',
|
||||
payload: { id: 'ep-running', label: 'A', baseUrl: 'http://localhost:8080', apiKey: 'k' },
|
||||
});
|
||||
fetchMock.mockImplementation(async (url: URL) => {
|
||||
if (url.pathname === '/running') {
|
||||
return new Response(
|
||||
JSON.stringify({
|
||||
running: [
|
||||
{ model: 'qwen3', state: 'ready', cmd: 'llama-server -m /models/qwen3.gguf --api-key sk-secret' },
|
||||
],
|
||||
}),
|
||||
{ status: 200 }
|
||||
);
|
||||
}
|
||||
throw new Error(`unexpected request in this test: ${url.href}`);
|
||||
});
|
||||
|
||||
const res = await app.inject({ method: 'GET', url: '/api/model-endpoints/ep-running/running-status' });
|
||||
const body = res.json();
|
||||
expect(body.data.running).toEqual([{ model: 'qwen3', state: 'ready' }]);
|
||||
expect(JSON.stringify(body)).not.toContain('sk-secret');
|
||||
expect(JSON.stringify(body)).not.toContain('cmd');
|
||||
});
|
||||
|
||||
it('refuses a baseUrl with embedded credentials or a non-http scheme at save time', async () => {
|
||||
const { app } = await setup();
|
||||
for (const baseUrl of ['http://user:pw@host:8080', 'ftp://host/models', 'http://169.254.169.254']) {
|
||||
|
||||
@@ -4,7 +4,7 @@
|
||||
* Port: N/A (app.inject, no real port needed)
|
||||
*/
|
||||
import { describe, it, expect, beforeEach, vi } from 'vitest';
|
||||
import { registerSessionRoutes } from '../../src/web/routes/session-routes.js';
|
||||
import { registerSessionRoutes, _clampEnvOverridesForOwner } from '../../src/web/routes/session-routes.js';
|
||||
import { createRouteTestHarness } from './_route-test-utils.js';
|
||||
import { createMockSession } from '../mocks/index.js';
|
||||
import { getDataDir } from '../../src/config/instance.js';
|
||||
@@ -620,3 +620,39 @@ describe('POST /api/sessions/:id/custom-model', () => {
|
||||
expect(session.setCustomModel).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('Claude multi-user clamp: the env-var half', () => {
|
||||
// CLAUDE_CODE_MAX_CONTEXT_TOKENS and CLAUDE_CONFIG_DIR were already reachable via
|
||||
// plain envOverrides before claude's privilegedEnvKeys existed (the first already
|
||||
// matches the CLAUDE_CODE_* allowedPrefix, the second is an allowed exact key), so
|
||||
// listing them here is not what makes this route safe — no custom-model route reads
|
||||
// privilegedEnvKeys at all. What it DOES do: ownerClampedEnvKeys() feeds the generic
|
||||
// envOverrides clamp on create/quick-start/reboot-restore, so a non-granted owner can
|
||||
// no longer set CLAUDE_CONFIG_DIR that way (the per-client-account feature, #255), and
|
||||
// a PERSISTED one is now stripped on reboot-restore for such an owner too — see
|
||||
// session-env-clamp.ts's own fileoverview for why that pass used to be a no-op for
|
||||
// claude specifically.
|
||||
const ORIGINAL = process.env.CODEMAN_MULTIUSER;
|
||||
beforeEach(() => {
|
||||
process.env.CODEMAN_MULTIUSER = '1';
|
||||
});
|
||||
afterEach(() => {
|
||||
if (ORIGINAL === undefined) delete process.env.CODEMAN_MULTIUSER;
|
||||
else process.env.CODEMAN_MULTIUSER = ORIGINAL;
|
||||
});
|
||||
|
||||
it('strips CLAUDE_CONFIG_DIR and CLAUDE_CODE_MAX_CONTEXT_TOKENS for a non-granted owner, leaving unrelated CLAUDE_CODE_* keys alone', async () => {
|
||||
const out = await _clampEnvOverridesForOwner('nobody', {
|
||||
CLAUDE_CONFIG_DIR: '/home/attacker/fake-claude-config',
|
||||
CLAUDE_CODE_MAX_CONTEXT_TOKENS: '999999',
|
||||
CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS: '1',
|
||||
});
|
||||
expect(out).toEqual({ CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS: '1' });
|
||||
});
|
||||
|
||||
it('is a no-op in single-user mode', async () => {
|
||||
delete process.env.CODEMAN_MULTIUSER;
|
||||
const input = { CLAUDE_CONFIG_DIR: '/home/attacker/fake-claude-config' };
|
||||
expect(await _clampEnvOverridesForOwner(undefined, input)).toBe(input);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user