Merge pull request #472 from opticon454/feature/git-host-auth-clis

feat(docker): opt-in gh + az CLIs with git credential helpers so Clone Repo and Docker cases can reach private repos

test/agent-image-build-args-parity.test.ts

@@ -20,11 +20,15 @@ import { fileURLToPath } from 'node:url';
 import {
   agentImageBuildArgPairs as mjsPairs,
   agentImageNpmPackages as mjsPackages,
+  GIT_HOST_CLI_BUILD_ARGS as mjsGitHostArgs,
+  gitHostCliBuildArgPairs as mjsGitHostPairs,
 } from '../scripts/lib/cli-catalog.mjs';
 import {
   agentImageBuildArgPairs as tsPairs,
   agentImageBuildArgs,
   agentImageNpmPackages as tsPackages,
+  GIT_HOST_CLI_BUILD_ARGS as tsGitHostArgs,
+  gitHostCliBuildArgPairs as tsGitHostPairs,
 } from '../src/docker-hosts.js';
 
 const CATALOG = JSON.parse(readFileSync(fileURLToPath(new URL('../config/clis.stock.json', import.meta.url)), 'utf-8'));
@@ -96,3 +100,48 @@ describe('agent-image build args: the .mjs and the TS mirror agree', () => {
     expect(extract(tsSource, 'docker-hosts.ts')).toBe(extract(mjsSource, 'cli-catalog.mjs'));
   });
 });
+
+describe('optional gh / az in the agent image: both producers pass the same switches', () => {
+  const ENV_GH = 'CODEMAN_AGENT_IMAGE_INSTALL_GH';
+  const ENV_AZ = 'CODEMAN_AGENT_IMAGE_INSTALL_AZ';
+
+  it('map the same environment variables to the same Dockerfile ARGs', () => {
+    expect(tsGitHostArgs).toEqual(mjsGitHostArgs);
+    expect(tsGitHostArgs.map(([, arg]) => arg)).toEqual(['CODEMAN_INSTALL_GH', 'CODEMAN_INSTALL_AZ']);
+  });
+
+  it('agree for every combination, and an unset or empty variable adds nothing', () => {
+    for (const gh of [undefined, '', '0', '1']) {
+      for (const az of [undefined, '', '0', '1']) {
+        const env: NodeJS.ProcessEnv = {};
+        if (gh !== undefined) env[ENV_GH] = gh;
+        if (az !== undefined) env[ENV_AZ] = az;
+        const expected: Array<[string, string]> = [];
+        if (gh) expected.push(['CODEMAN_INSTALL_GH', gh]);
+        if (az) expected.push(['CODEMAN_INSTALL_AZ', az]);
+        expect(tsGitHostPairs(env)).toEqual(expected);
+        expect(mjsGitHostPairs(env)).toEqual(expected);
+        expect(tsPairs(env)).toEqual(mjsPairs(CATALOG, env));
+      }
+    }
+  });
+
+  it('keeps the default argv unchanged when neither variable is set', () => {
+    expect(tsPairs({})).toEqual([['CLI_NPM_PACKAGES', tsPackages().join(' ')]]);
+  });
+
+  it('refuses anything but 0 or 1 on both sides, naming the variable', () => {
+    for (const bad of ['yes', 'true', '2', ' 1', '0 && echo']) {
+      expect(() => tsGitHostPairs({ [ENV_AZ]: bad })).toThrow(new RegExp(ENV_AZ));
+      expect(() => mjsGitHostPairs({ [ENV_AZ]: bad })).toThrow(new RegExp(ENV_AZ));
+    }
+  });
+
+  it('both Dockerfiles declare the switches, defaulting to OFF (opt-in)', () => {
+    for (const file of ['../docker/agent.Dockerfile', '../docker/server.Dockerfile']) {
+      const dockerfile = readFileSync(fileURLToPath(new URL(file, import.meta.url)), 'utf-8');
+      expect(dockerfile, file).toMatch(/^ARG CODEMAN_INSTALL_GH=0$/m);
+      expect(dockerfile, file).toMatch(/^ARG CODEMAN_INSTALL_AZ=0$/m);
+    }
+  });
+});

test/docker-hosts.test.ts

@@ -351,6 +351,70 @@ 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'), '');
+    writeFileSync(join(home, '.config', 'gh', 'config.yml'), '');
+    mkdirSync(join(home, '.azure', 'logs'), { recursive: true });
+    mkdirSync(join(home, '.azure', 'cliextensions'), { recursive: true });
+    writeFileSync(join(home, '.azure', 'azureProfile.json'), '{}');
+    writeFileSync(join(home, '.azure', 'msal_token_cache.json'), '{}');
+    writeFileSync(join(home, '.azure', 'config'), '');
+
+    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');
+    expect(dests).toContain('/home/agent/.azure/azureProfile.json');
+    expect(dests).toContain('/home/agent/.azure/msal_token_cache.json');
+    expect(dests).toContain('/home/agent/.azure/config');
+    // Absent files are skipped, and nothing outside the sign-in set is seeded.
+    expect(dests).not.toContain('/home/agent/.azure/service_principal_entries.json');
+    expect(dests.some((d) => d.includes('logs') || d.includes('cliextensions'))).toBe(false);
+    expect(seedCopies.filter((s) => /\.azure|\.config\/gh/.test(s.to)).every((s) => !s.recursive)).toBe(true);
+    // Every host credential file rides a READ-ONLY mount, so the container never writes back.
+    const credMounts = mounts.filter((m) => /\.azure|\.config[\\/]gh/.test(m.src));
+    expect(credMounts.length).toBe(5);
+    expect(credMounts.every((m) => m.readonly)).toBe(true);
+  });
+
   it('gates every artifact on existsSync (absent stores contribute nothing)', () => {
     const { mounts, seedCopies } = resolveDockerCredentialArtifacts(home);
     expect(mounts).toEqual([]);

test/git-clone.test.ts

@@ -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', () => {

test/routes/case-clone-credential-helpers.test.ts

@@ -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 });
+  });
+});