mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-09-30 12:39:42 +02:00
fix(docker): gate gh/az seeding on its switch; no shared git sign-in for non-admin clones
Addresses the review on #472. - CRED_STORES: `.config/gh` and `.azure` now carry `enabledByEnv` (CODEMAN_AGENT_IMAGE_INSTALL_GH / _AZ), and resolveDockerCredentialArtifacts skips a store unless that variable is exactly `1`, read at container create. A host that merely has ~/.config/gh/hosts.yml or a plaintext MSAL cache no longer copies them into every case container. Tests: the default environment seeds neither even with the files present, and each store follows only its own switch. - Multi-user mode: a non-admin's Clone Repo clone and preflight run with `git -c credential.helper=` (GIT_NO_CREDENTIAL_HELPERS, placed before the subcommand), so the server account's helpers are never lent to them. Verified against a real private repo that it also clears the URL-scoped credential.<url>.helper entries, and that public clones still work. Tests: the argv in test/git-clone.test.ts, and the route decision (non-admin cleared; admin and single-user kept) in test/routes/case-clone-credential-helpers.test.ts. - Docs: recreate the case container to pick up seeds (docker/README.md, Docker-Cases wiki, docker-cases.md); the multi-user behaviour in docker/README.md and security-architecture.md; "functionally unchanged" instead of "unchanged" for an image built with both switches off (server.Dockerfile comment, README, changeset). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0167CiuzLrmjYWxwKp3rMWjw
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
5cf5a45438
commit
02e40f506b
@@ -351,6 +351,40 @@ describe('resolveDockerCredentialArtifacts (isolated codex/gemini/gcloud/opencod
|
||||
expect(mounts.filter((m) => m.readonly && m.dst.includes('cred-seeds')).length).toBeGreaterThanOrEqual(3);
|
||||
});
|
||||
|
||||
/** Host files for both opt-in stores, present whether or not the switches are on. */
|
||||
function writeGhAzHostFiles(): void {
|
||||
mkdirSync(join(home, '.config', 'gh'), { recursive: true });
|
||||
writeFileSync(join(home, '.config', 'gh', 'hosts.yml'), '');
|
||||
writeFileSync(join(home, '.config', 'gh', 'config.yml'), '');
|
||||
mkdirSync(join(home, '.azure'), { recursive: true });
|
||||
writeFileSync(join(home, '.azure', 'azureProfile.json'), '{}');
|
||||
writeFileSync(join(home, '.azure', 'msal_token_cache.json'), '{}');
|
||||
}
|
||||
const isGhOrAz = (p: string) => /\.azure|\.config[\\/]gh/.test(p);
|
||||
|
||||
it('gh + az: the DEFAULT environment seeds neither, even when the host files exist', () => {
|
||||
writeGhAzHostFiles();
|
||||
for (const env of [{}, { CODEMAN_AGENT_IMAGE_INSTALL_GH: '0', CODEMAN_AGENT_IMAGE_INSTALL_AZ: '' }]) {
|
||||
const { mounts, seedCopies } = resolveDockerCredentialArtifacts(home, env);
|
||||
expect(mounts.filter((m) => isGhOrAz(m.src))).toEqual([]);
|
||||
expect(seedCopies.filter((s) => isGhOrAz(s.to))).toEqual([]);
|
||||
}
|
||||
});
|
||||
|
||||
it('gh + az: each store follows ONLY its own switch, and only the exact value 1', () => {
|
||||
writeGhAzHostFiles();
|
||||
const dests = (env: NodeJS.ProcessEnv) => resolveDockerCredentialArtifacts(home, env).seedCopies.map((s) => s.to);
|
||||
const ghOnly = dests({ CODEMAN_AGENT_IMAGE_INSTALL_GH: '1' });
|
||||
expect(ghOnly).toContain('/home/agent/.config/gh/hosts.yml');
|
||||
expect(ghOnly.some((d) => d.includes('.azure'))).toBe(false);
|
||||
const azOnly = dests({ CODEMAN_AGENT_IMAGE_INSTALL_AZ: '1' });
|
||||
expect(azOnly).toContain('/home/agent/.azure/msal_token_cache.json');
|
||||
expect(azOnly.some((d) => d.includes('.config/gh'))).toBe(false);
|
||||
expect(
|
||||
dests({ CODEMAN_AGENT_IMAGE_INSTALL_GH: 'true', CODEMAN_AGENT_IMAGE_INSTALL_AZ: 'yes' }).some(isGhOrAz)
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it('gh + az: seed only the sign-in files, never logs/extensions/caches', () => {
|
||||
mkdirSync(join(home, '.config', 'gh'), { recursive: true });
|
||||
writeFileSync(join(home, '.config', 'gh', 'hosts.yml'), '');
|
||||
@@ -361,7 +395,10 @@ describe('resolveDockerCredentialArtifacts (isolated codex/gemini/gcloud/opencod
|
||||
writeFileSync(join(home, '.azure', 'msal_token_cache.json'), '{}');
|
||||
writeFileSync(join(home, '.azure', 'config'), '');
|
||||
|
||||
const { mounts, seedCopies } = resolveDockerCredentialArtifacts(home);
|
||||
const { mounts, seedCopies } = resolveDockerCredentialArtifacts(home, {
|
||||
CODEMAN_AGENT_IMAGE_INSTALL_GH: '1',
|
||||
CODEMAN_AGENT_IMAGE_INSTALL_AZ: '1',
|
||||
});
|
||||
const dests = seedCopies.map((s) => s.to);
|
||||
expect(dests).toContain('/home/agent/.config/gh/hosts.yml');
|
||||
expect(dests).toContain('/home/agent/.config/gh/config.yml');
|
||||
|
||||
@@ -201,6 +201,25 @@ describe('buildCloneArgs / buildLsRemoteArgs', () => {
|
||||
'd',
|
||||
]);
|
||||
});
|
||||
|
||||
it('clears every credential helper, BEFORE the subcommand, only when asked', () => {
|
||||
// Multi-user non-admin clones must not borrow the server account's git sign-in.
|
||||
// `-c` is a global option: after `clone` git would read it as an unknown flag.
|
||||
expect(
|
||||
buildCloneArgs({ repository: 'https://example.com/r.git', destination: 'd', withoutCredentialHelpers: true })
|
||||
).toEqual(['-c', 'credential.helper=', 'clone', '--', 'https://example.com/r.git', 'd']);
|
||||
expect(buildLsRemoteArgs('https://example.com/r.git', { withoutCredentialHelpers: true })).toEqual([
|
||||
'-c',
|
||||
'credential.helper=',
|
||||
'ls-remote',
|
||||
'--symref',
|
||||
'--',
|
||||
'https://example.com/r.git',
|
||||
]);
|
||||
// Absent or false leaves the argv exactly as it was before the option existed.
|
||||
expect(buildCloneArgs({ repository: 'r', destination: 'd', withoutCredentialHelpers: false })[0]).toBe('clone');
|
||||
expect(buildLsRemoteArgs('r', {})[0]).toBe('ls-remote');
|
||||
});
|
||||
});
|
||||
|
||||
describe('gitNonInteractiveEnv', () => {
|
||||
|
||||
@@ -0,0 +1,94 @@
|
||||
/**
|
||||
* @fileoverview Clone Repo must not lend the server account's git sign-in to
|
||||
* non-admins in multi-user mode (PR #472 review).
|
||||
*
|
||||
* Every Codeman user's git runs as the one server account, so a credential
|
||||
* helper that account has (the Docker image's opt-in `gh`/`az` helpers, or any
|
||||
* `gh auth setup-git`) would otherwise clone a PRIVATE repository with the
|
||||
* signed-in admin's credentials into a non-admin's case space, the same
|
||||
* boundary the local-transport rule guards. These tests pin the ROUTE decision:
|
||||
* who gets `withoutCredentialHelpers`. The argv it becomes is pinned in
|
||||
* `test/git-clone.test.ts`, and the real-git clone path in
|
||||
* `case-clone-routes.test.ts`.
|
||||
*
|
||||
* Only the two network calls are mocked, so no git runs and nothing leaves the
|
||||
* machine; everything else in `git-clone.ts` (URL parsing included) is real.
|
||||
*
|
||||
* Port: N/A (app.inject).
|
||||
*/
|
||||
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
|
||||
import { createRouteTestHarness } from './_route-test-utils.js';
|
||||
import { registerCaseRoutes } from '../../src/web/routes/case-routes.js';
|
||||
|
||||
const calls = vi.hoisted(() => ({
|
||||
probe: [] as Array<{ repository: string; opts: unknown }>,
|
||||
clone: [] as Array<Record<string, unknown>>,
|
||||
}));
|
||||
|
||||
vi.mock('../../src/git-clone.js', async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import('../../src/git-clone.js')>();
|
||||
return {
|
||||
...actual,
|
||||
isGitAvailable: () => true,
|
||||
probeGitRemote: async (repository: string, _timeoutMs?: number, opts?: unknown) => {
|
||||
calls.probe.push({ repository, opts });
|
||||
return { reachable: false, branches: [], tags: [] };
|
||||
},
|
||||
cloneRepository: async (opts: Record<string, unknown>) => {
|
||||
calls.clone.push(opts);
|
||||
return { ok: false, failure: { code: 'AUTH_REQUIRED', message: 'needs auth', stderr: '' } };
|
||||
},
|
||||
};
|
||||
});
|
||||
|
||||
const REPO = 'https://github.com/example/private-repo.git';
|
||||
|
||||
type Who = { username: string; role: 'admin' | 'user' } | undefined;
|
||||
|
||||
async function run(who: Who, multiUser: boolean): Promise<{ probe: unknown; clone: unknown }> {
|
||||
const prev = process.env.CODEMAN_MULTIUSER;
|
||||
if (multiUser) process.env.CODEMAN_MULTIUSER = '1';
|
||||
else delete process.env.CODEMAN_MULTIUSER;
|
||||
try {
|
||||
const { app } = await createRouteTestHarness(registerCaseRoutes, who ? { authUser: who } : undefined);
|
||||
await app.inject({ method: 'POST', url: '/api/cases/clone-preflight', payload: { repository: REPO } });
|
||||
await app.inject({
|
||||
method: 'POST',
|
||||
url: '/api/cases/clone',
|
||||
payload: { name: `cred-${Math.random().toString(36).slice(2, 10)}`, repository: REPO },
|
||||
});
|
||||
await app.close();
|
||||
expect(calls.probe, 'the preflight never reached probeGitRemote').toHaveLength(1);
|
||||
expect(calls.clone, 'the clone never reached cloneRepository').toHaveLength(1);
|
||||
return {
|
||||
probe: (calls.probe[0].opts as { withoutCredentialHelpers?: boolean } | undefined)?.withoutCredentialHelpers,
|
||||
clone: calls.clone[0].withoutCredentialHelpers,
|
||||
};
|
||||
} finally {
|
||||
if (prev === undefined) delete process.env.CODEMAN_MULTIUSER;
|
||||
else process.env.CODEMAN_MULTIUSER = prev;
|
||||
}
|
||||
}
|
||||
|
||||
describe('Clone Repo credential helpers by caller', () => {
|
||||
beforeEach(() => {
|
||||
calls.probe.length = 0;
|
||||
calls.clone.length = 0;
|
||||
});
|
||||
afterEach(() => {
|
||||
vi.clearAllMocks();
|
||||
});
|
||||
|
||||
it('clears them for a NON-ADMIN in multi-user mode (preflight AND clone)', async () => {
|
||||
expect(await run({ username: 'mallory', role: 'user' }, true)).toEqual({ probe: true, clone: true });
|
||||
});
|
||||
|
||||
it('keeps them for an admin in multi-user mode', async () => {
|
||||
expect(await run({ username: 'root', role: 'admin' }, true)).toEqual({ probe: false, clone: false });
|
||||
});
|
||||
|
||||
it('keeps them in single-user mode, where the sole user owns the account', async () => {
|
||||
expect(await run(undefined, false)).toEqual({ probe: false, clone: false });
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user