From c7acbb9b41b1452ab7afc588757880bca40c1021 Mon Sep 17 00:00:00 2001 From: miles-kt-inkeep <135626743+miles-kt-inkeep@users.noreply.github.com> Date: Fri, 26 Jun 2026 13:32:03 -0400 Subject: [PATCH] fix(open-knowledge): preserve git SSH/credential-helper env for sync (#2197) * fix(open-knowledge): preserve git SSH and credential-helper env for server sync Reproduces inkeep/open-knowledge#310 by Ron Ulitsky against current main. Server-spawned git rebuilt a minimal environment that dropped HOME, SSH_AUTH_SOCK, and the Windows home variables, so SSH remotes and credential helpers could not authenticate (most visibly Windows with an SSH remote). buildGitEnv now preserves the user, home, and SSH auth vars. applyGitEnv merges commit author and committer overrides into that preserved env instead of replacing it, so auth state survives into the subsequent fetch and push. * fix(open-knowledge): pin commit.gpgsign and core.autocrlf off for sync git Preserving HOME (so SSH keys and credential helpers resolve) also lets server-spawned git read the user's global ~/.gitconfig. Two of its directives are unsafe on the unattended sync path: - commit.gpgsign: the merge-resolution git commit would try to GPG-sign with no TTY. Git aborts the commit on sign failure rather than writing it unsigned, so a cache-cold sync tick fails; a success would also sign a bot-authored commit with the user's key. - core.autocrlf: would rewrite content line endings on checkout and merge, fighting OK's byte-exact LF round-trip and churning the file watcher to CRDT path. Pin both off via -c at the createGitInstance level so every server-spawned git command inherits them. -c outranks global config, and the user's own terminal and IDE git stay untouched. * docs(open-knowledge): drop stale no-gitconfig note from buildGitEnv The closing JSDoc sentence claimed server-spawned git must NOT inherit ~/.gitconfig and that the env strip stays minimal. Both are false since buildGitEnv now preserves HOME and the user/SSH auth vars, and the three hazards it listed are handled elsewhere (commit.gpgsign and core.autocrlf pinned off in createGitInstance; url.insteadOf intentionally honored). * docs(open-knowledge): address review feedback on git-handle JSDoc and tests Cloud review (no blocking issues) flagged JSDoc this PR's own changes made stale, plus two polish items: - buildGitEnv: "Four vars must survive" was wrong once the auth-env allowlist (HOME, SSH_AUTH_SOCK, Windows home vars) started surviving too; added the bullet. - RelayGhToken: dropped the "no HOME" framing (HOME is preserved now). The relay still exists because OK's helper has no gh shell-out and server-side resolution guarantees the gh tier regardless of the curated env. - Dropped change-narration "now" from the gpgsign/autocrlf comment. - Added JSDoc to applyGitEnv (merges into handle.env; .env() mutates in place). - Consolidated the test env-isolation helpers into one module-scoped withEnvEntries; removed withEnv and the manual save/restore. GitOrigin-RevId: d74508c98ab30136d4240c144e2cabab99c80485 --- .changeset/windows-git-ssh-sync-env.md | 7 ++ packages/server/src/git-handle.test.ts | 90 ++++++++++++++++++++++---- packages/server/src/git-handle.ts | 52 +++++++++++++-- packages/server/src/sync-engine.ts | 6 +- 4 files changed, 134 insertions(+), 21 deletions(-) create mode 100644 .changeset/windows-git-ssh-sync-env.md diff --git a/.changeset/windows-git-ssh-sync-env.md b/.changeset/windows-git-ssh-sync-env.md new file mode 100644 index 000000000..2cc3b081c --- /dev/null +++ b/.changeset/windows-git-ssh-sync-env.md @@ -0,0 +1,7 @@ +--- +"@inkeep/open-knowledge": patch +--- + +Fix Git auto-sync when server-spawned Git needs the user's home directory, SSH agent, or credential-helper environment to reach a remote. This most visibly affected Windows repositories using SSH remotes, where `ok sync` and editor sync could fail with "Could not read from remote repository" while the same `git fetch` or `git push` worked in a terminal. + +Because preserving the home directory also lets server-spawned Git read the user's global config, OK now pins `commit.gpgsign=false` and `core.autocrlf=false` for its own Git commands only (via `-c`, leaving the user's own Git untouched): the first prevents the unattended sync commit from aborting when a global signing config can't prompt for a passphrase, and the second keeps line-ending conversion from churning content against OK's byte-exact round-trip. diff --git a/packages/server/src/git-handle.test.ts b/packages/server/src/git-handle.test.ts index 4ef1c5bb0..5ae47569e 100644 --- a/packages/server/src/git-handle.test.ts +++ b/packages/server/src/git-handle.test.ts @@ -4,22 +4,28 @@ import { mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { setTimeout as wait } from 'node:timers/promises'; -import { buildGitEnv, createGitInstance } from './git-handle.ts'; +import { applyGitEnv, buildGitEnv, createGitInstance, type GitHandle } from './git-handle.ts'; import { withParentLock } from './git-mutex.ts'; -describe('buildGitEnv', () => { - function withEnv(key: string, value: string | undefined, fn: () => void): void { - const saved = process.env[key]; - try { +function withEnvEntries(entries: Record, fn: () => void): void { + const saved = new Map(); + for (const key of Object.keys(entries)) { + saved.set(key, process.env[key]); + const value = entries[key]; + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + try { + fn(); + } finally { + for (const [key, value] of saved) { if (value === undefined) delete process.env[key]; else process.env[key] = value; - fn(); - } finally { - if (saved === undefined) delete process.env[key]; - else process.env[key] = saved; } } +} +describe('buildGitEnv', () => { test('forces LANG/LC_ALL=C for locale-stable stderr', () => { const env = buildGitEnv(); expect(env.LANG).toBe('C'); @@ -31,19 +37,49 @@ describe('buildGitEnv', () => { }); test('preserves PATH so a bare-command credential helper resolves', () => { - withEnv('PATH', '/custom/bin:/usr/bin', () => { + withEnvEntries({ PATH: '/custom/bin:/usr/bin' }, () => { expect(buildGitEnv().PATH).toBe('/custom/bin:/usr/bin'); }); }); + test('preserves user and SSH auth environment for Git transports', () => { + withEnvEntries( + { + HOME: '/Users/alice', + USERPROFILE: 'C:\\Users\\alice', + HOMEDRIVE: 'C:', + HOMEPATH: '\\Users\\alice', + ProgramData: 'C:\\ProgramData', + ALLUSERSPROFILE: 'C:\\ProgramData', + SSH_AUTH_SOCK: '/tmp/ssh-agent.sock', + }, + () => { + const env = buildGitEnv(); + expect(env.HOME).toBe('/Users/alice'); + expect(env.USERPROFILE).toBe('C:\\Users\\alice'); + expect(env.HOMEDRIVE).toBe('C:'); + expect(env.HOMEPATH).toBe('\\Users\\alice'); + expect(env.ProgramData).toBe('C:\\ProgramData'); + expect(env.ALLUSERSPROFILE).toBe('C:\\ProgramData'); + expect(env.SSH_AUTH_SOCK).toBe('/tmp/ssh-agent.sock'); + }, + ); + }); + + test('does not pass through GIT_SSH_COMMAND without explicit simple-git opt-in', () => { + withEnvEntries({ GIT_SSH_COMMAND: 'ssh -vv' }, () => { + expect('GIT_SSH_COMMAND' in buildGitEnv()).toBe(false); + }); + }); + test('preserves ELECTRON_RUN_AS_NODE so the packaged credential helper runs as Node', () => { - withEnv('ELECTRON_RUN_AS_NODE', '1', () => { + withEnvEntries({ ELECTRON_RUN_AS_NODE: '1' }, () => { expect(buildGitEnv().ELECTRON_RUN_AS_NODE).toBe('1'); }); }); test('omits ELECTRON_RUN_AS_NODE on a non-Electron host (var unset)', () => { - withEnv('ELECTRON_RUN_AS_NODE', undefined, () => { + withEnvEntries({ ELECTRON_RUN_AS_NODE: undefined }, () => { expect('ELECTRON_RUN_AS_NODE' in buildGitEnv()).toBe(false); }); }); @@ -62,6 +98,11 @@ describe('buildGitEnv', () => { describe('createGitInstance (credential.helper config)', () => { let tmpDir: string; + function readEnv(handle: GitHandle): Record { + // biome-ignore lint/suspicious/noExplicitAny: probing internal simple-git executor for spawn-env assertion + return ((handle.git as any)._executor?.env ?? {}) as Record; + } + beforeEach(() => { tmpDir = mkdtempSync(join(tmpdir(), 'ok-git-handle-test-')); execSync('git init -q', { cwd: tmpDir }); @@ -78,6 +119,31 @@ describe('createGitInstance (credential.helper config)', () => { const version = await handle.git.raw(['--version']); expect(version).toContain('git version'); }); + + test('merges author overrides without dropping git auth env', () => { + withEnvEntries({ USERPROFILE: 'C:\\Users\\alice' }, () => { + const handle = createGitInstance(tmpDir, { gitIndexFile: '.git/custom-index' }); + applyGitEnv(handle, { + GIT_AUTHOR_NAME: 'Alice', + GIT_AUTHOR_EMAIL: 'alice@example.com', + }); + + const env = readEnv(handle); + expect(env.USERPROFILE).toBe('C:\\Users\\alice'); + expect(env.GIT_INDEX_FILE).toBe(join(tmpDir, '.git/custom-index')); + expect(env.GIT_AUTHOR_NAME).toBe('Alice'); + expect(env.GIT_AUTHOR_EMAIL).toBe('alice@example.com'); + }); + }); + + test('pins commit.gpgsign and core.autocrlf off, overriding repo config', async () => { + execSync('git config commit.gpgsign true', { cwd: tmpDir }); + execSync('git config core.autocrlf true', { cwd: tmpDir }); + + const handle = createGitInstance(tmpDir); + expect((await handle.git.raw(['config', '--get', 'commit.gpgsign'])).trim()).toBe('false'); + expect((await handle.git.raw(['config', '--get', 'core.autocrlf'])).trim()).toBe('false'); + }); }); describe('withParentLock', () => { diff --git a/packages/server/src/git-handle.ts b/packages/server/src/git-handle.ts index be8409285..98c5d2473 100644 --- a/packages/server/src/git-handle.ts +++ b/packages/server/src/git-handle.ts @@ -18,6 +18,7 @@ export interface GitHandle { git: SimpleGit; projectDir: string; credentialArgs: string[]; + env: Record; } type CredentialHelperUnsafeGitOptions = SimpleGitOptions & { @@ -26,13 +27,37 @@ type CredentialHelperUnsafeGitOptions = SimpleGitOptions & { }; }; +const GIT_AUTH_ENV_KEYS = [ + 'HOME', + 'USERPROFILE', + 'HOMEDRIVE', + 'HOMEPATH', + 'APPDATA', + 'LOCALAPPDATA', + 'ProgramData', + 'ALLUSERSPROFILE', + 'SystemRoot', + 'WINDIR', + 'windir', + 'ComSpec', + 'TEMP', + 'TMP', + 'USERNAME', + 'USERDOMAIN', + 'PATHEXT', + 'SSH_AUTH_SOCK', + 'ELECTRON_RUN_AS_NODE', +] as const; + export function buildGitEnv(ghToken?: RelayGhToken): Record { const env: Record = { LANG: 'C', LC_ALL: 'C', GIT_TERMINAL_PROMPT: '0' }; - if (process.env.PATH !== undefined) { - env.PATH = process.env.PATH; + const path = process.env.PATH ?? process.env.Path; + if (path !== undefined) { + env.PATH = path; } - if (process.env.ELECTRON_RUN_AS_NODE !== undefined) { - env.ELECTRON_RUN_AS_NODE = process.env.ELECTRON_RUN_AS_NODE; + for (const key of GIT_AUTH_ENV_KEYS) { + const value = process.env[key]; + if (value !== undefined) env[key] = value; } if (ghToken) { env.OK_GH_TOKEN = ghToken.token; @@ -41,6 +66,17 @@ export function buildGitEnv(ghToken?: RelayGhToken): Record { return env; } +export function applyGitEnv( + handle: GitHandle, + overrides: Record, +): SimpleGit { + const env = { ...handle.env }; + for (const [key, value] of Object.entries(overrides)) { + if (value !== undefined) env[key] = value; + } + return handle.git.env(env); +} + export function createGitInstance(projectDir: string, options: GitHandleOptions = {}): GitHandle { const { credentialArgs = [], gitIndexFile, ghToken } = options; @@ -49,7 +85,11 @@ export function createGitInstance(projectDir: string, options: GitHandleOptions env.GIT_INDEX_FILE = resolve(projectDir, gitIndexFile); } - const gitConfig = credentialArgs.length >= 2 ? [credentialArgs[1]] : []; + const gitConfig = [ + 'commit.gpgsign=false', + 'core.autocrlf=false', + ...(credentialArgs.length >= 2 ? [credentialArgs[1]] : []), + ]; const gitOptions: Partial = { baseDir: projectDir, @@ -59,5 +99,5 @@ export function createGitInstance(projectDir: string, options: GitHandleOptions const git = simpleGit(gitOptions as Partial).env(env as Record); - return { git, projectDir, credentialArgs }; + return { git, projectDir, credentialArgs, env: env as Record }; } diff --git a/packages/server/src/sync-engine.ts b/packages/server/src/sync-engine.ts index 447b31042..485567acd 100644 --- a/packages/server/src/sync-engine.ts +++ b/packages/server/src/sync-engine.ts @@ -21,7 +21,7 @@ import { type UserFacingErrorCode, } from './error-classification.ts'; import { createGhTokenSource, type GhTokenSource } from './gh-token-source.ts'; -import { createGitInstance, type GitHandle, withParentLock } from './git-handle.ts'; +import { applyGitEnv, createGitInstance, type GitHandle, withParentLock } from './git-handle.ts'; import { resolveGitIdentity } from './git-identity.ts'; import { type CheckPushPermissionOptions, @@ -1045,7 +1045,7 @@ export class SyncEngine { const authorName = identity?.name ?? 'OpenKnowledge'; const authorEmail = identity?.email ?? 'sync@open-knowledge.local'; - handle.git.env({ + applyGitEnv(handle, { GIT_AUTHOR_NAME: authorName, GIT_AUTHOR_EMAIL: authorEmail, GIT_COMMITTER_NAME: authorName, @@ -1198,7 +1198,7 @@ export class SyncEngine { const identity = await resolveGitIdentity(this.projectDir); const authorName = identity?.name ?? 'OpenKnowledge'; const authorEmail = identity?.email ?? 'sync@open-knowledge.local'; - isoHandle.git.env({ + applyGitEnv(isoHandle, { GIT_AUTHOR_NAME: authorName, GIT_AUTHOR_EMAIL: authorEmail, GIT_COMMITTER_NAME: authorName,