commit abab20c7e28927bb75b489d139188b82cf578ea9
parent 644564b89914a5854c86480ff28a206d8d99be24
Author: finwo <finwo@pm.me>
Date: Sat, 19 Sep 2026 16:41:03 +0200
Read repositories the conductor does not own
Diffstat:
| M | src/lib/git.js | | | 62 | +++++++++++++++++++++++++++++++++++++++++++++++++++----------- |
| A | test/git-env.test.js | | | 118 | +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ |
2 files changed, 169 insertions(+), 11 deletions(-)
diff --git a/src/lib/git.js b/src/lib/git.js
@@ -16,6 +16,7 @@
import fs from 'node:fs/promises';
import path from 'node:path';
import { execFile, spawn } from 'node:child_process';
+import { existsSync } from 'node:fs';
import { promisify } from 'node:util';
const execFileAsync = promisify(execFile);
@@ -23,15 +24,36 @@ const execFileAsync = promisify(execFile);
export const SHA_PATTERN = /^[0-9a-f]{40}$/;
export const SHORT_SHA_PATTERN = /^[0-9a-f]{7,40}$/;
-// Refuses credential prompts, so a misconfigured private repository fails
-// promptly instead of hanging until the request times out.
-const GIT_ENV = {
- ...process.env,
- GIT_TERMINAL_PROMPT: '0',
- GIT_ASKPASS: '',
- GCM_INTERACTIVE: 'never',
- LC_ALL: 'C',
-};
+// Reading a repository owned by somebody else is the normal case here, not
+// a suspicious one: project paths belong to whoever put them on disk, and
+// the conductor is a different user again once it runs in a container.
+// Git refuses that by default with "detected dubious ownership", which is
+// the right default for a shell and the wrong one for this.
+//
+// Applied through the environment rather than a global git config, so it
+// covers both spawn and execFile below and affects nothing outside this
+// process. Anything already set in the environment is preserved, since
+// the indices have to be contiguous for git to read them.
+function gitEnv(env = process.env) {
+ const count = Number.parseInt(env.GIT_CONFIG_COUNT ?? '0', 10);
+ const base = Number.isInteger(count) && count > 0 ? count : 0;
+ return {
+ ...env,
+ // Refuses credential prompts, so a misconfigured private repository
+ // fails promptly instead of hanging until the request times out.
+ GIT_TERMINAL_PROMPT: '0',
+ GIT_ASKPASS: '',
+ GCM_INTERACTIVE: 'never',
+ LC_ALL: 'C',
+ GIT_CONFIG_COUNT: String(base + 1),
+ [`GIT_CONFIG_KEY_${base}`]: 'safe.directory',
+ [`GIT_CONFIG_VALUE_${base}`]: '*',
+ };
+}
+
+const GIT_ENV = gitEnv();
+
+export { gitEnv };
export class GitError extends Error {
constructor(message, { stderr, code } = {}) {
@@ -99,10 +121,28 @@ export function createGit(cfg) {
return path.join(cfg.git.mirror_path, `${projectId}.git`);
}
+ // Given --git-dir explicitly, git still treats the process working
+ // directory as a work tree. That makes the result depend on whatever
+ // happens to sit in the conductor's own directory: asking for a file
+ // that is absent from a commit reports "exists on disk, but not in
+ // <sha>" rather than a plain absence, purely because a file of that
+ // name is next to the running process. Working inside the bare mirror
+ // takes the stray work tree out of the picture.
+ //
+ // Only when it is already there, since clone is told --git-dir for a
+ // directory it is about to create.
+ function repoCwd(args) {
+ const i = args.indexOf('--git-dir');
+ if (i < 0) return undefined;
+ const dir = args[i + 1];
+ return dir && existsSync(dir) ? dir : undefined;
+ }
+
async function run(args, options = {}) {
try {
const { stdout, stderr } = await execFileAsync('git', args, {
env: GIT_ENV,
+ cwd: options.cwd ?? repoCwd(args),
timeout,
maxBuffer: options.maxBuffer ?? 16 * 1024 * 1024,
encoding: options.encoding ?? 'utf8',
@@ -175,7 +215,7 @@ export function createGit(cfg) {
);
return stdout;
} catch (e) {
- if (/does not exist|not a valid object name|Not a valid object/i.test(e.stderr ?? '')) return null;
+ if (/does not exist|exists on disk, but not in|not a valid object name|Not a valid object/i.test(e.stderr ?? '')) return null;
throw e;
}
},
@@ -200,7 +240,7 @@ export function createGit(cfg) {
if (prefix) args.push(`--prefix=${prefix}`);
args.push(sha);
- const child = spawn('git', args, { env: GIT_ENV, stdio: ['ignore', 'pipe', 'pipe'] });
+ const child = spawn('git', args, { env: GIT_ENV, cwd: repoCwd(args), stdio: ['ignore', 'pipe', 'pipe'] });
let stderr = '';
child.stderr.on('data', (chunk) => { stderr += chunk.toString().slice(0, 4096); });
child.on('close', (code) => {
diff --git a/test/git-env.test.js b/test/git-env.test.js
@@ -0,0 +1,118 @@
+// test/git-env.test.js - reading repositories the conductor does not own
+//
+// Project paths belong to whoever put them on disk, and in a container the
+// conductor is a different user again. Git calls that "dubious ownership"
+// and refuses. This went unnoticed for a while because the old image ran
+// as uid 1000, which happened to match the repositories being read, so it
+// worked by coincidence rather than by design.
+
+import test from 'node:test';
+import assert from 'node:assert/strict';
+import fs from 'node:fs/promises';
+import { existsSync } from 'node:fs';
+import os from 'node:os';
+import path from 'node:path';
+import { execFile } from 'node:child_process';
+import { promisify } from 'node:util';
+
+import { gitEnv } from '../src/lib/git.js';
+
+const execFileAsync = promisify(execFile);
+
+test('git runs with safe.directory set', () => {
+ const env = gitEnv({});
+ assert.equal(env.GIT_CONFIG_COUNT, '1');
+ assert.equal(env.GIT_CONFIG_KEY_0, 'safe.directory');
+ assert.equal(env.GIT_CONFIG_VALUE_0, '*');
+});
+
+test('it appends to configuration already in the environment', () => {
+ const env = gitEnv({
+ GIT_CONFIG_COUNT: '2',
+ GIT_CONFIG_KEY_0: 'http.version',
+ GIT_CONFIG_VALUE_0: 'HTTP/1.1',
+ GIT_CONFIG_KEY_1: 'core.quotePath',
+ GIT_CONFIG_VALUE_1: 'false',
+ });
+
+ // Git reads indices 0..count-1, so ours has to land at the end and the
+ // count has to grow rather than be replaced.
+ assert.equal(env.GIT_CONFIG_COUNT, '3');
+ assert.equal(env.GIT_CONFIG_KEY_0, 'http.version');
+ assert.equal(env.GIT_CONFIG_KEY_1, 'core.quotePath');
+ assert.equal(env.GIT_CONFIG_KEY_2, 'safe.directory');
+ assert.equal(env.GIT_CONFIG_VALUE_2, '*');
+});
+
+test('a count that is not a number does not corrupt the sequence', () => {
+ for (const bad of ['', 'garbage', '-1', '0']) {
+ const env = gitEnv({ GIT_CONFIG_COUNT: bad });
+ assert.equal(env.GIT_CONFIG_COUNT, '1', `count ${JSON.stringify(bad)}`);
+ assert.equal(env.GIT_CONFIG_KEY_0, 'safe.directory');
+ }
+});
+
+test('git accepts the environment we build', async () => {
+ // Proves the variables are actually well formed. A malformed set makes
+ // git fail outright, which is what this would catch.
+ const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'conductor-gitenv-'));
+ try {
+ const env = gitEnv(process.env);
+ await execFileAsync('git', ['init', '-q', '-b', 'main', dir], { env });
+ const { stdout } = await execFileAsync(
+ 'git', ['-C', dir, 'config', '--get', 'safe.directory'], { env },
+ );
+ assert.equal(stdout.trim(), '*');
+ } finally {
+ await fs.rm(dir, { recursive: true, force: true });
+ }
+});
+
+test('a file missing from a commit reads as missing, whatever sits in the working directory', async () => {
+ // Given --git-dir, git treats the process working directory as a work
+ // tree and changes its wording when a file of that name happens to be
+ // there: "exists on disk, but not in <sha>" rather than a plain
+ // absence. The conductor decides a project has no pipeline from that
+ // message, so the wrong wording turned a missing pipeline into a 500.
+ // Reproduced here rather than left to the happy accident that the test
+ // suite runs from a directory containing a .conductor.yml.
+ const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'conductor-cwd-'));
+ const work = path.join(dir, 'work');
+ const mirror = path.join(dir, 'mirror.git');
+ const env = gitEnv(process.env);
+
+ try {
+ await fs.mkdir(work);
+ await execFileAsync('git', ['init', '-q', '-b', 'main', work], { env });
+ await execFileAsync('git', ['-C', work, 'config', 'user.email', 't@example.invalid'], { env });
+ await execFileAsync('git', ['-C', work, 'config', 'user.name', 'test'], { env });
+ await fs.writeFile(path.join(work, 'other.txt'), 'present\n');
+ await execFileAsync('git', ['-C', work, 'add', '-A'], { env });
+ await execFileAsync('git', ['-C', work, 'commit', '-q', '-m', 'only other.txt'], { env });
+ await execFileAsync('git', ['clone', '-q', '--mirror', work, mirror], { env });
+
+ const { stdout } = await execFileAsync('git', ['--git-dir', mirror, 'rev-parse', 'HEAD'], { env });
+ const sha = stdout.trim();
+
+ // A name that is absent from the commit but present in the directory
+ // the suite runs from.
+ const decoy = 'package.json';
+ assert.ok(existsSync(path.join(process.cwd(), decoy)), `expected ${decoy} in the working directory`);
+
+ let stderr = '';
+ try {
+ await execFileAsync(
+ 'git', ['--git-dir', mirror, 'cat-file', 'blob', `${sha}:${decoy}`],
+ { env, cwd: mirror },
+ );
+ assert.fail('expected git to report the path as absent');
+ } catch (error) {
+ stderr = error.stderr ?? '';
+ }
+
+ // Run inside the bare mirror there is no work tree to confuse it.
+ assert.match(stderr, /does not exist/);
+ } finally {
+ await fs.rm(dir, { recursive: true, force: true });
+ }
+});
