diff --git a/.changeset/ssh-auth-sock-harvest.md b/.changeset/ssh-auth-sock-harvest.md new file mode 100644 index 00000000..c6800d86 --- /dev/null +++ b/.changeset/ssh-auth-sock-harvest.md @@ -0,0 +1,5 @@ +--- +"@inkeep/open-knowledge-desktop": patch +--- + +Git sync now works when SSH keys live in an external SSH agent (1Password, Proton Pass, a custom `ssh-agent`). Finder-launched apps inherit macOS's default agent socket instead of the one your shell exports, so pushes over SSH failed with "Permission denied (publickey)" while terminal git worked. At startup the desktop app now reads `SSH_AUTH_SOCK` from your login shell and passes it to every git operation. diff --git a/.changeset/ssh-origin-probe-leniency.md b/.changeset/ssh-origin-probe-leniency.md new file mode 100644 index 00000000..2ff12181 --- /dev/null +++ b/.changeset/ssh-origin-probe-leniency.md @@ -0,0 +1,15 @@ +--- +'@inkeep/open-knowledge': patch +'@inkeep/open-knowledge-server': patch +'@inkeep/open-knowledge-core': patch +--- + +**Fix:** auto-sync no longer silently pauses for SSH-origin remotes with no +GitHub credential. The push-permission probe used to treat "no gh/OK token" +as signed-out and pause sync with a Sign-in prompt — wrong for self-hosted +forges (Gitea/Forgejo) and github.com-over-SSH setups, where pushes +authenticate with SSH keys and no OK sign-in path can ever help. The probe +now keys off the origin transport: HTTPS origins keep the signed-out denial +(and its Sign-in affordance, including GHES); `ssh://`, scp-style, and +`git://` origins abstain with a new `unknown/ssh-unverified` result, so sync +proceeds and the real push decides. diff --git a/packages/app/src/components/SyncStatusBadge.dom.test.tsx b/packages/app/src/components/SyncStatusBadge.dom.test.tsx index 8bd58e5c..33fbc98f 100644 --- a/packages/app/src/components/SyncStatusBadge.dom.test.tsx +++ b/packages/app/src/components/SyncStatusBadge.dom.test.tsx @@ -174,6 +174,13 @@ describe('SyncStatusBadge helper behavior', () => { ); expect(shouldOfferSignInAgain({ checkStatus: 'denied' })).toBe(false); expect(shouldOfferSignInAgain({ checkStatus: 'unknown', unknownError: 'network' })).toBe(false); + // ssh-unverified is the abstaining probe result for SSH-origin repos with + // no GitHub credential. Signing in can never help there (push auths with + // SSH keys), so broadening this predicate to match it would resurrect the + // misleading sign-in affordance the transport-keyed probe fix removed. + expect(shouldOfferSignInAgain({ checkStatus: 'unknown', unknownError: 'ssh-unverified' })).toBe( + false, + ); expect(shouldOfferSignInAgain(undefined)).toBe(false); }); }); diff --git a/packages/core/src/schemas/api/sync-seed.test.ts b/packages/core/src/schemas/api/sync-seed.test.ts index 234b9791..bf1544f5 100644 --- a/packages/core/src/schemas/api/sync-seed.test.ts +++ b/packages/core/src/schemas/api/sync-seed.test.ts @@ -3,6 +3,7 @@ import { ConflictEntrySchema, InstallSkillSuccessSchema, ProblemTypeSchema, + PushPermissionSchema, SeedApplyRequestSchema, SeedApplySuccessSchema, SeedPlanSuccessSchema, @@ -123,6 +124,32 @@ describe('SyncStatusSchema', () => { }); }); +describe('PushPermissionSchema', () => { + test('parses every unknown variant including ssh-unverified', () => { + for (const unknownError of [ + 'network', + 'timeout', + 'rate-limit', + 'token-invalid', + 'malformed-response', + 'ssh-unverified', + ]) { + const parsed = PushPermissionSchema.safeParse({ checkStatus: 'unknown', unknownError }); + expect(parsed.success).toBe(true); + } + }); + test('round-trips the ssh-unverified member', () => { + const wire = { checkStatus: 'unknown' as const, unknownError: 'ssh-unverified' as const }; + const parsed = PushPermissionSchema.parse(wire); + expect(parsed).toEqual(wire); + }); + test('rejects an unlisted unknownError code', () => { + expect( + PushPermissionSchema.safeParse({ checkStatus: 'unknown', unknownError: 'bogus' }).success, + ).toBe(false); + }); +}); + describe('SyncRemoteSchema', () => { test('accepts a github remote with label + https webUrl', () => { expect( diff --git a/packages/core/src/schemas/api/sync-seed.ts b/packages/core/src/schemas/api/sync-seed.ts index 7cbfd3cc..4483d2df 100644 --- a/packages/core/src/schemas/api/sync-seed.ts +++ b/packages/core/src/schemas/api/sync-seed.ts @@ -78,7 +78,14 @@ export const PushPermissionSchema = z.discriminatedUnion('checkStatus', [ .object({ checkStatus: z.literal('unknown'), unknownError: z - .enum(['network', 'timeout', 'rate-limit', 'token-invalid', 'malformed-response']) + .enum([ + 'network', + 'timeout', + 'rate-limit', + 'token-invalid', + 'malformed-response', + 'ssh-unverified', + ]) .optional(), }) .loose(), diff --git a/packages/desktop/src/main/git-spawn-env.test.ts b/packages/desktop/src/main/git-spawn-env.test.ts new file mode 100644 index 00000000..1ffa9c3b --- /dev/null +++ b/packages/desktop/src/main/git-spawn-env.test.ts @@ -0,0 +1,33 @@ +import { afterEach, describe, expect, it } from 'vitest'; +import { gitSpawnEnv } from './git-spawn-env.ts'; + +const ORIGINAL_SOCK = process.env.SSH_AUTH_SOCK; + +afterEach(() => { + if (ORIGINAL_SOCK === undefined) { + delete process.env.SSH_AUTH_SOCK; + } else { + process.env.SSH_AUTH_SOCK = ORIGINAL_SOCK; + } +}); + +describe('gitSpawnEnv', () => { + it('pins an English locale', () => { + const env = gitSpawnEnv(); + expect(env.LANG).toBe('C'); + expect(env.LC_ALL).toBe('C'); + }); + + it('reflects SSH_AUTH_SOCK changes made after a prior call', () => { + process.env.SSH_AUTH_SOCK = '/tmp/before.sock'; + expect(gitSpawnEnv().SSH_AUTH_SOCK).toBe('/tmp/before.sock'); + // The startup harvest patches process.env once; a frozen snapshot here + // would pin every later git spawn to the pre-harvest socket. + process.env.SSH_AUTH_SOCK = '/tmp/after.sock'; + expect(gitSpawnEnv().SSH_AUTH_SOCK).toBe('/tmp/after.sock'); + }); + + it('keeps the augmented PATH stable across calls', () => { + expect(gitSpawnEnv().PATH).toBe(gitSpawnEnv().PATH); + }); +}); diff --git a/packages/desktop/src/main/git-spawn-env.ts b/packages/desktop/src/main/git-spawn-env.ts index d5a9d598..e212b89b 100644 --- a/packages/desktop/src/main/git-spawn-env.ts +++ b/packages/desktop/src/main/git-spawn-env.ts @@ -14,8 +14,14 @@ * (`detectMissingGitHelper`, the add-error / branch-gone matchers) survives a * non-English host locale — same discipline as the server's `buildGitEnv`. * - * Computed lazily and cached: the augmentation stats well-known directories, - * and PATH/homedir don't change within a process lifetime. + * Only the PATH augmentation is cached (it stats well-known directories, and + * PATH/homedir don't change within a process lifetime). The env object itself + * is rebuilt from live `process.env` on every call: startup corrects + * `SSH_AUTH_SOCK` once via the login-shell harvest (`shell-env.ts` / + * `applyHarvestedAuthSock`), and a cached snapshot taken before that point + * would pin every later git spawn to launchd's default-agent socket. For the + * same reason, callers must not freeze this function's result into + * module-level constants — call it per spawn. */ import { existsSync, statSync } from 'node:fs'; @@ -23,7 +29,7 @@ import { homedir } from 'node:os'; import { delimiter } from 'node:path'; import { augmentGitSpawnPath } from '@inkeep/open-knowledge-core'; -let cached: Record | null = null; +let cachedPath: string | null = null; /** True iff `dir` exists and is a directory (symlinks followed). */ function isDir(dir: string): boolean { @@ -40,18 +46,18 @@ function isDir(dir: string): boolean { * share-fetch arm's `GIT_TERMINAL_PROMPT=0`). */ export function gitSpawnEnv(): Record { - if (cached === null) { - cached = { - ...process.env, - LANG: 'C', - LC_ALL: 'C', - PATH: augmentGitSpawnPath(process.env.PATH, { - platform: process.platform, - homeDir: homedir(), - isDir, - delimiter, - }), - }; + if (cachedPath === null) { + cachedPath = augmentGitSpawnPath(process.env.PATH, { + platform: process.platform, + homeDir: homedir(), + isDir, + delimiter, + }); } - return cached; + return { + ...process.env, + LANG: 'C', + LC_ALL: 'C', + PATH: cachedPath, + }; } diff --git a/packages/desktop/src/main/index.ts b/packages/desktop/src/main/index.ts index 3c2f2819..dfebdb5a 100644 --- a/packages/desktop/src/main/index.ts +++ b/packages/desktop/src/main/index.ts @@ -314,6 +314,7 @@ import { handleRevealExternal } from './reveal-external.ts'; import { createServerExitRecorder, type ServerExitRecorder } from './server-exit-record.ts'; import { startFirstRunHandshake } from './share-handoff.ts'; import { checkOutboundUrl, handleShellOpenExternal } from './shell-allowlist.ts'; +import { applyHarvestedAuthSock, harvestShellAuthSock } from './shell-env.ts'; import { createShowGateRegistry, type ShowGateRegistry } from './show-gate.ts'; import { reclaimProjectSkillsOnProjectOpen, reclaimUserSkillsOnLaunch } from './skill-reclaim.ts'; import { attachSpellcheckContextMenu } from './spellcheck-context-menu.ts'; @@ -5672,6 +5673,18 @@ function bootPrimaryInstance(): void { // its return tells the waterfall whether main spans are live. startupWaterfall.mark('appReady'); startupWaterfall.otelEnabled = beginRoot(); + // Login-shell SSH_AUTH_SOCK harvest — started here so its (2s-bounded) + // shell spawn overlaps the bootstrap I/O below; awaited + applied just + // before the window-open branch, ahead of the git preflight and both + // server-spawn paths (utility fork + detached spawn). Desktop-main git + // spawns pick the corrected value up automatically: gitSpawnEnv() + // rebuilds from live process.env per call and must never be frozen + // into a module-level constant (see git-spawn-env.ts). + const shellEnvLogger = { + event: (payload: Record & { event: string }) => + getLogger('shell-env').info(payload, payload.event), + }; + const authSockHarvest = harvestShellAuthSock({ logger: shellEnvLogger }); // One-time userData migration for the "Open Knowledge" → "OpenKnowledge" // rename. Dormant until the packaged productName flips the userData // basename to "OpenKnowledge"; then it relocates a verified-ours legacy @@ -5844,6 +5857,14 @@ function bootPrimaryInstance(): void { }); }); + // Apply the harvested login-shell SSH_AUTH_SOCK before the window-open + // branch. A Finder launch inherits launchd's default-agent socket, which + // holds no keys for external-agent users (1Password, Proton Pass) — + // patching process.env here lets every downstream git spawn inherit the + // agent the user's terminal actually uses. Failure or an empty value + // leaves the inherited socket untouched. + applyHarvestedAuthSock(process.env, await authSockHarvest, shellEnvLogger); + // Every project open spawns a NEW editor window. Boot restore order: // 1. An update relaunch left a `pendingWindowRestore` snapshot — open // EVERY project that was open before the relaunch, not just the diff --git a/packages/desktop/src/main/path-install.ts b/packages/desktop/src/main/path-install.ts index 1ec5bc4d..0d507bf2 100644 --- a/packages/desktop/src/main/path-install.ts +++ b/packages/desktop/src/main/path-install.ts @@ -326,7 +326,7 @@ function installCanonical(home: string, wrapper: string, fs: PathInstallFsOps): } } -async function defaultSpawn( +export async function defaultSpawn( command: string, args: string[], opts: { timeoutMs: number; env: Record }, diff --git a/packages/desktop/src/main/shell-env.test.ts b/packages/desktop/src/main/shell-env.test.ts new file mode 100644 index 00000000..892bb7b8 --- /dev/null +++ b/packages/desktop/src/main/shell-env.test.ts @@ -0,0 +1,210 @@ +import { describe, expect, it } from 'vitest'; +import { applyHarvestedAuthSock, harvestShellAuthSock, type ShellEnvLogger } from './shell-env.ts'; + +const MARK = '<>'; + +function collectingLogger(): { + logger: ShellEnvLogger; + events: string[]; + payloads: Array & { event: string }>; +} { + const events: string[] = []; + const payloads: Array & { event: string }> = []; + return { + logger: { + event: (payload) => { + events.push(payload.event); + payloads.push(payload); + }, + }, + events, + payloads, + }; +} + +function fakeSpawn(result: { + code: number | null; + stdout: string; + stderr?: string; + timedOut?: boolean; +}) { + const calls: Array<{ command: string; args: string[] }> = []; + const spawn = async (command: string, args: string[]) => { + calls.push({ command, args }); + return { stderr: '', ...result }; + }; + return { spawn, calls }; +} + +describe('harvestShellAuthSock', () => { + it('returns the sock wrapped in markers', async () => { + const { spawn } = fakeSpawn({ code: 0, stdout: `${MARK}/tmp/agent.sock${MARK}` }); + const sock = await harvestShellAuthSock({ + env: { SHELL: '/bin/zsh' }, + platform: 'darwin', + spawn, + }); + expect(sock).toBe('/tmp/agent.sock'); + }); + + it('extracts the sock despite rc-file noise around the markers', async () => { + const { spawn } = fakeSpawn({ + code: 0, + stdout: `Welcome!\nnvm loaded\n${MARK}/tmp/agent.sock${MARK}\ntrailing noise`, + }); + const sock = await harvestShellAuthSock({ + env: { SHELL: '/bin/zsh' }, + platform: 'darwin', + spawn, + }); + expect(sock).toBe('/tmp/agent.sock'); + }); + + it('returns null when the login shell has no SSH_AUTH_SOCK', async () => { + const { spawn } = fakeSpawn({ code: 0, stdout: `${MARK}${MARK}` }); + expect( + await harvestShellAuthSock({ env: { SHELL: '/bin/zsh' }, platform: 'darwin', spawn }), + ).toBeNull(); + }); + + it('returns null and logs on non-zero exit, carrying bounded stderr', async () => { + const calls: Array<{ command: string; args: string[] }> = []; + const spawn = async (command: string, args: string[]) => { + calls.push({ command, args }); + return { code: 1, stdout: '', stderr: `zsh: bad substitution${'x'.repeat(400)}` }; + }; + const { logger, events, payloads } = collectingLogger(); + expect( + await harvestShellAuthSock({ + env: { SHELL: '/bin/zsh' }, + platform: 'darwin', + spawn, + logger, + }), + ).toBeNull(); + expect(events).toContain('shell-authsock-harvest-failed'); + const failure = payloads.find((p) => p.event === 'shell-authsock-harvest-failed'); + expect(failure?.stderr).toMatch(/^zsh: bad substitution/); + expect((failure?.stderr as string).length).toBeLessThanOrEqual(300); + }); + + it('returns null and logs on timeout', async () => { + const { spawn } = fakeSpawn({ code: null, stdout: '', timedOut: true }); + const { logger, events } = collectingLogger(); + expect( + await harvestShellAuthSock({ + env: { SHELL: '/bin/zsh' }, + platform: 'darwin', + spawn, + logger, + }), + ).toBeNull(); + expect(events).toContain('shell-authsock-harvest-failed'); + }); + + it('returns null and logs bounded stdout when the markers are missing', async () => { + const { spawn } = fakeSpawn({ code: 0, stdout: `rc noise only, no marker${'y'.repeat(400)}` }); + const { logger, events, payloads } = collectingLogger(); + expect( + await harvestShellAuthSock({ + env: { SHELL: '/bin/zsh' }, + platform: 'darwin', + spawn, + logger, + }), + ).toBeNull(); + expect(events).toContain('shell-authsock-harvest-failed'); + const failure = payloads.find((p) => p.event === 'shell-authsock-harvest-failed'); + expect(failure?.reason).toBe('marker-missing'); + expect(failure?.stdout).toMatch(/^rc noise only/); + expect((failure?.stdout as string).length).toBeLessThanOrEqual(300); + }); + + it('returns null and logs when spawn throws', async () => { + const { logger, events } = collectingLogger(); + expect( + await harvestShellAuthSock({ + env: { SHELL: '/bin/zsh' }, + platform: 'darwin', + spawn: async () => { + throw new Error('ENOENT'); + }, + logger, + }), + ).toBeNull(); + expect(events).toContain('shell-authsock-harvest-failed'); + }); + + it('skips win32 without spawning', async () => { + const { spawn, calls } = fakeSpawn({ code: 0, stdout: `${MARK}/tmp/x${MARK}` }); + expect(await harvestShellAuthSock({ env: {}, platform: 'win32', spawn })).toBeNull(); + expect(calls).toHaveLength(0); + }); + + it('spawns $SHELL as an interactive login shell', async () => { + const { spawn, calls } = fakeSpawn({ code: 0, stdout: `${MARK}/tmp/x${MARK}` }); + await harvestShellAuthSock({ + env: { SHELL: '/opt/homebrew/bin/fish' }, + platform: 'darwin', + spawn, + }); + expect(calls[0]?.command).toBe('/opt/homebrew/bin/fish'); + expect(calls[0]?.args[0]).toBe('-ilc'); + }); + + it('falls back to zsh on darwin and bash on linux when SHELL is unset', async () => { + const darwin = fakeSpawn({ code: 0, stdout: `${MARK}/tmp/x${MARK}` }); + await harvestShellAuthSock({ env: {}, platform: 'darwin', spawn: darwin.spawn }); + expect(darwin.calls[0]?.command).toBe('/bin/zsh'); + + const linux = fakeSpawn({ code: 0, stdout: `${MARK}/tmp/x${MARK}` }); + await harvestShellAuthSock({ env: {}, platform: 'linux', spawn: linux.spawn }); + expect(linux.calls[0]?.command).toBe('/bin/bash'); + }); +}); + +describe('applyHarvestedAuthSock', () => { + it('patches env when the harvested sock differs, logging previous and new values', () => { + const env: Record = { SSH_AUTH_SOCK: '/launchd/Listeners' }; + const { logger, events, payloads } = collectingLogger(); + expect(applyHarvestedAuthSock(env, '/tmp/agent.sock', logger)).toBe(true); + expect(env.SSH_AUTH_SOCK).toBe('/tmp/agent.sock'); + expect(events).toContain('shell-authsock-harvested'); + const harvested = payloads.find((p) => p.event === 'shell-authsock-harvested'); + expect(harvested?.from).toBe('/launchd/Listeners'); + expect(harvested?.to).toBe('/tmp/agent.sock'); + }); + + it('logs from: null when no prior sock existed', () => { + const env: Record = {}; + const { logger, payloads } = collectingLogger(); + expect(applyHarvestedAuthSock(env, '/tmp/agent.sock', logger)).toBe(true); + const harvested = payloads.find((p) => p.event === 'shell-authsock-harvested'); + expect(harvested?.from).toBeNull(); + expect(harvested?.to).toBe('/tmp/agent.sock'); + }); + + it('sets env when no sock was present at all', () => { + const env: Record = {}; + expect(applyHarvestedAuthSock(env, '/tmp/agent.sock', collectingLogger().logger)).toBe(true); + expect(env.SSH_AUTH_SOCK).toBe('/tmp/agent.sock'); + }); + + it('no-ops when the harvested sock matches the current value', () => { + const env: Record = { SSH_AUTH_SOCK: '/tmp/agent.sock' }; + expect(applyHarvestedAuthSock(env, '/tmp/agent.sock', collectingLogger().logger)).toBe(false); + expect(env.SSH_AUTH_SOCK).toBe('/tmp/agent.sock'); + }); + + it('never downgrades: null harvest leaves the existing value untouched', () => { + const env: Record = { SSH_AUTH_SOCK: '/launchd/Listeners' }; + expect(applyHarvestedAuthSock(env, null, collectingLogger().logger)).toBe(false); + expect(env.SSH_AUTH_SOCK).toBe('/launchd/Listeners'); + }); + + it('never downgrades: empty-string harvest leaves the existing value untouched', () => { + const env: Record = { SSH_AUTH_SOCK: '/launchd/Listeners' }; + expect(applyHarvestedAuthSock(env, '', collectingLogger().logger)).toBe(false); + expect(env.SSH_AUTH_SOCK).toBe('/launchd/Listeners'); + }); +}); diff --git a/packages/desktop/src/main/shell-env.ts b/packages/desktop/src/main/shell-env.ts new file mode 100644 index 00000000..88bd066a --- /dev/null +++ b/packages/desktop/src/main/shell-env.ts @@ -0,0 +1,122 @@ +/** + * Login-shell `SSH_AUTH_SOCK` harvest for GUI launches. + * + * Finder/Dock-launched Electron inherits launchd's environment, whose + * `SSH_AUTH_SOCK` points at Apple's default ssh-agent — an agent that holds + * no keys when the user's keys live in an external agent (1Password, Proton + * Pass, custom `ssh-agent`) exported via `export SSH_AUTH_SOCK=...` in a + * shell rc file. GUI apps never read rc files, so every git-over-SSH spawn + * downstream (utility-process server, detached server, desktop-main git) + * authenticates against the wrong agent and fails with + * `Permission denied (publickey)` while the same command works in a + * terminal. Same launchd-impoverishment disease as the PATH handling in + * `git-spawn-env.ts` / `path-install.ts`, for a different variable. + * + * The remedy mirrors `discoverRealInteractivePath`: spawn one interactive + * login shell, capture the variable, and patch `process.env` before the + * first consumer reads it. Deliberately scoped to `SSH_AUTH_SOCK` only — + * harvesting more (PATH especially) has a much larger blast radius and its + * own established mechanisms. + */ + +import { defaultSpawn } from './path-install.ts'; + +export interface ShellEnvLogger { + event: (payload: Record & { event: string }) => void; +} + +const DEFAULT_LOGGER: ShellEnvLogger = { + event: (payload) => console.info('[shell-env]', payload), +}; + +/** + * Sentinel wrapping the captured value so rc-file noise (echoes, motd, + * profiling output) on stdout cannot corrupt it — unlike PATH discovery, + * which tolerates junk entries, a socket path must come back byte-exact. + */ +const MARK = '<>'; + +export interface HarvestShellAuthSockOpts { + env?: Record; + platform?: string; + spawn?: typeof defaultSpawn; + logger?: ShellEnvLogger; + timeoutMs?: number; +} + +/** + * Returns the login shell's `SSH_AUTH_SOCK`, or `null` on any failure + * (timeout, non-zero exit, empty value, spawn error). Callers must treat + * `null` as "leave the current value alone" — see `applyHarvestedAuthSock`. + * + * Skipped entirely on win32: Windows agents talk over a fixed named pipe, + * not an env-var-addressed socket. + */ +export async function harvestShellAuthSock( + opts: HarvestShellAuthSockOpts = {}, +): Promise { + const platform = opts.platform ?? process.platform; + if (platform === 'win32') return null; + const env = opts.env ?? process.env; + const shell = env.SHELL ?? (platform === 'linux' ? '/bin/bash' : '/bin/zsh'); + const spawn = opts.spawn ?? defaultSpawn; + const logger = opts.logger ?? DEFAULT_LOGGER; + try { + const result = await spawn(shell, ['-ilc', `printf %s "${MARK}$SSH_AUTH_SOCK${MARK}"`], { + timeoutMs: opts.timeoutMs ?? 2000, + env, + }); + if (result.code !== 0 || result.timedOut) { + logger.event({ + event: 'shell-authsock-harvest-failed', + shell, + code: result.code, + timedOut: result.timedOut ?? false, + // Bounded: a broken rc file can produce unbounded stderr. + stderr: result.stderr.slice(0, 300), + }); + return null; + } + const first = result.stdout.indexOf(MARK); + const last = result.stdout.lastIndexOf(MARK); + if (first === -1 || last <= first) { + logger.event({ + event: 'shell-authsock-harvest-failed', + shell, + reason: 'marker-missing', + // Bounded: the raw capture is the only clue to why markers are absent. + stdout: result.stdout.slice(0, 300), + }); + return null; + } + const value = result.stdout.slice(first + MARK.length, last).trim(); + return value === '' ? null : value; + } catch (err) { + logger.event({ + event: 'shell-authsock-harvest-failed', + shell, + error: err instanceof Error ? err.message : String(err), + }); + return null; + } +} + +/** + * Patches `env.SSH_AUTH_SOCK` with the harvested value. Never downgrades: + * a `null`/empty harvest or an unchanged value leaves `env` untouched, so a + * hung rc file or a genuinely agent-less login shell can't strip a working + * socket from a terminal-launched process. Returns true iff `env` changed. + */ +export function applyHarvestedAuthSock( + env: Record, + harvested: string | null, + logger: ShellEnvLogger = DEFAULT_LOGGER, +): boolean { + if (harvested === null || harvested === '' || harvested === env.SSH_AUTH_SOCK) { + return false; + } + const previous = env.SSH_AUTH_SOCK ?? null; + env.SSH_AUTH_SOCK = harvested; + logger.event({ event: 'shell-authsock-harvested', from: previous, to: harvested }); + return true; +} diff --git a/packages/desktop/src/main/worktree-recents.ts b/packages/desktop/src/main/worktree-recents.ts index 35f41cc2..9e60441d 100644 --- a/packages/desktop/src/main/worktree-recents.ts +++ b/packages/desktop/src/main/worktree-recents.ts @@ -24,8 +24,6 @@ import { basename, dirname, isAbsolute, resolve } from 'node:path'; import { promisify } from 'node:util'; import { gitSpawnEnv } from './git-spawn-env.ts'; -const GIT_ENV = gitSpawnEnv(); - const execFileAsync = promisify(execFile); export interface RecentGitInfo { @@ -61,7 +59,7 @@ export function readWorktreeBranch(projectPath: string): string | null { const out = String( execFileSync('git', ['symbolic-ref', '--quiet', '--short', 'HEAD'], { cwd: projectPath, - env: GIT_ENV, + env: gitSpawnEnv(), }), ).trim(); return out.length > 0 ? out : null; @@ -84,7 +82,7 @@ export async function readWorktreeBranchAsync(projectPath: string): Promise 0 ? out : null; @@ -145,7 +143,7 @@ const REV_PARSE_ARGS = [ function computeRecentGit(realPath: string): RecentGitInfo { let out: string; try { - out = String(execFileSync('git', [...REV_PARSE_ARGS], { cwd: realPath, env: GIT_ENV })); + out = String(execFileSync('git', [...REV_PARSE_ARGS], { cwd: realPath, env: gitSpawnEnv() })); } catch { return EMPTY; } @@ -157,7 +155,7 @@ async function computeRecentGitAsync(realPath: string): Promise { try { const { stdout } = await execFileAsync('git', [...REV_PARSE_ARGS], { cwd: realPath, - env: GIT_ENV, + env: gitSpawnEnv(), }); out = stdout; } catch { diff --git a/packages/desktop/src/main/worktree-service.ts b/packages/desktop/src/main/worktree-service.ts index 170df677..f4af4385 100644 --- a/packages/desktop/src/main/worktree-service.ts +++ b/packages/desktop/src/main/worktree-service.ts @@ -46,14 +46,15 @@ import { seedWorktreeProjectSetup } from './worktree-setup-inherit.ts'; const execFileAsync = promisify(execFile); -/** English-stable, PATH-augmented git env — see `git-spawn-env.ts`. */ -const GIT_ENV = gitSpawnEnv(); - -/** Fetch spawn env: `GIT_ENV` plus `GIT_TERMINAL_PROMPT=0`, mirroring the - * server's git discipline — desktop main has no terminal to answer a +/** Fetch spawn env: `gitSpawnEnv()` plus `GIT_TERMINAL_PROMPT=0`, mirroring + * the server's git discipline — desktop main has no terminal to answer a * credential prompt, so a credentialed remote must fail fast (into the - * `fetch-failed` arm) instead of stalling until the timeout kill. */ -const FETCH_GIT_ENV = { ...GIT_ENV, GIT_TERMINAL_PROMPT: '0' } as const; + * `fetch-failed` arm) instead of stalling until the timeout kill. Built per + * call, never frozen at module level: `gitSpawnEnv()` must reflect the + * startup `SSH_AUTH_SOCK` harvest (see `git-spawn-env.ts`). */ +function fetchGitEnv(): Record { + return { ...gitSpawnEnv(), GIT_TERMINAL_PROMPT: '0' }; +} /** Default bound for the share-checkout fetch — matches the server's * fast-forward fetch bound so a stalled network degrades to a typed @@ -171,7 +172,7 @@ export async function createWorktree(args: CreateWorktreeArgs): Promise { try { await execFileAsync('git', ['show-ref', '--verify', '--quiet', ref], { cwd: anchorPath, - env: GIT_ENV, + env: gitSpawnEnv(), timeout: 5_000, }); return true; @@ -320,7 +321,7 @@ async function fetchShareBranch( try { await execFileAsync('git', ['fetch', 'origin', branch], { cwd: anchorPath, - env: FETCH_GIT_ENV, + env: fetchGitEnv(), timeout: timeoutMs, }); return null; @@ -392,7 +393,7 @@ async function listLocalBranches(anchorPath: string): Promise { const { stdout } = await execFileAsync( 'git', ['for-each-ref', '--format=%(refname:short)', 'refs/heads/'], - { cwd: anchorPath, env: GIT_ENV }, + { cwd: anchorPath, env: gitSpawnEnv() }, ); return parseBranchList(String(stdout)); } catch { @@ -415,7 +416,7 @@ async function listRemoteBranches(anchorPath: string): Promise { const { stdout } = await execFileAsync( 'git', ['for-each-ref', '--format=%(refname:short)', 'refs/remotes/'], - { cwd: anchorPath, env: GIT_ENV }, + { cwd: anchorPath, env: gitSpawnEnv() }, ); // `refname:short` renders a `/HEAD` pointer as `` (no // trailing `/HEAD`) — drop any ref with no slash (a bare remote name), plus @@ -455,7 +456,7 @@ async function computeBehindCounts( const { stdout } = await execFileAsync( 'git', ['rev-list', '--count', `${branch}..${upstream}`], - { cwd: anchorPath, env: GIT_ENV }, + { cwd: anchorPath, env: gitSpawnEnv() }, ); const n = Number.parseInt(String(stdout).trim(), 10); if (Number.isFinite(n) && n >= 0) out[branch] = n; @@ -506,7 +507,7 @@ function ensureWorktreesExcluded(anchorPath: string): void { /** Synchronous git read that returns the trimmed stdout, or null on failure. */ function execFileSyncTrim(cmd: string, cmdArgs: string[], cwd: string): string | null { try { - return String(execFileSync(cmd, cmdArgs, { cwd, env: GIT_ENV })).trim(); + return String(execFileSync(cmd, cmdArgs, { cwd, env: gitSpawnEnv() })).trim(); } catch { return null; } diff --git a/packages/server/src/github-permissions.test.ts b/packages/server/src/github-permissions.test.ts index 4dba0c0d..b3dd05d9 100644 --- a/packages/server/src/github-permissions.test.ts +++ b/packages/server/src/github-permissions.test.ts @@ -302,6 +302,84 @@ describe('checkPushPermission — token resolution', () => { expect(calls).toHaveLength(0); }); + test('anonymous + transport ssh → unknown/ssh-unverified with NO HTTP call', async () => { + // Self-hosted forge (Gitea/Forgejo) pushed over SSH: no gh/OK token can + // ever exist for that host, but the push auths with SSH keys. The probe + // must abstain, not deny — denying pauses sync for a fully working setup. + const { fetch, calls } = mockFetch(() => jsonResponse(200, {})); + const { store } = fakeStore(null); + const result = await checkPushPermission({ + owner: 'acme', + repo: 'kb', + host: 'git.example.com', + transport: 'ssh', + detectGh: ghUnavailable(), + tokenStore: store, + _fetchFn: fetch, + }); + expect(result).toEqual({ kind: 'unknown', error: 'ssh-unverified' }); + expect(calls).toHaveLength(0); + }); + + test('anonymous + transport git → unknown/ssh-unverified (unauthenticated protocol, tokens irrelevant)', async () => { + const { fetch, calls } = mockFetch(() => jsonResponse(200, {})); + const result = await checkPushPermission({ + owner: 'acme', + repo: 'kb', + host: 'git.example.com', + transport: 'git', + _fetchFn: fetch, + }); + expect(result).toEqual({ kind: 'unknown', error: 'ssh-unverified' }); + expect(calls).toHaveLength(0); + }); + + test('anonymous + explicit transport https → denied/not-authenticated unchanged', async () => { + // HTTPS pushes auth with tokens; no token ⇒ the push cannot succeed, so + // the signed-out denial (and its Sign-in affordance) stays correct — + // including for GHES over HTTPS. + const { fetch, calls } = mockFetch(() => jsonResponse(200, {})); + const result = await checkPushPermission({ + owner: 'acme', + repo: 'kb', + host: 'ghes.acme.test', + transport: 'https', + _fetchFn: fetch, + }); + expect(result).toEqual({ kind: 'denied', reason: 'not-authenticated' }); + expect(calls).toHaveLength(0); + }); + + test('anonymous + github.com over SSH is lenient too (deliberate)', async () => { + // A github.com user with SSH keys and no gh CLI / stored token pushes + // fine; keying leniency on transport (not host) un-breaks them as well. + const { fetch, calls } = mockFetch(() => jsonResponse(200, {})); + const result = await checkPushPermission({ + owner: 'inkeep', + repo: 'open-knowledge', + host: 'github.com', + transport: 'ssh', + _fetchFn: fetch, + }); + expect(result).toEqual({ kind: 'unknown', error: 'ssh-unverified' }); + expect(calls).toHaveLength(0); + }); + + test('transport ssh with a resolved credential still probes normally', async () => { + // Transport only gates the ANONYMOUS branch. With a token the API answer + // is authoritative regardless of how git would transport the push. + const { fetch, calls } = mockFetch(() => jsonResponse(200, { permissions: { push: true } })); + const result = await checkPushPermission({ + owner: 'inkeep', + repo: 'open-knowledge', + transport: 'ssh', + detectGh: ghAvailable(), + _fetchFn: fetch, + }); + expect(result).toEqual({ kind: 'allowed' }); + expect(calls).toHaveLength(1); + }); + test('gh detection is scoped to the requested host', async () => { const seenHosts: Array = []; const detectGh: DetectGhFn = (host) => { diff --git a/packages/server/src/github-permissions.ts b/packages/server/src/github-permissions.ts index 6d1c80fb..fd9fc190 100644 --- a/packages/server/src/github-permissions.ts +++ b/packages/server/src/github-permissions.ts @@ -18,6 +18,7 @@ import type { Counter, Histogram } from '@opentelemetry/api'; import { getLogger } from './logger.ts'; +import type { OriginTransport } from './share/git-context.ts'; import { getMeter } from './telemetry.ts'; const log = getLogger('github-permissions'); @@ -36,12 +37,18 @@ type PushPermissionDeniedReason = | 'repo-not-found' | 'not-authenticated'; +// 'ssh-unverified' = anonymous credential resolution over a non-token +// transport (ssh/git origins). git auths those pushes with SSH keys, so token +// absence proves nothing about push ability — the probe abstains and lets the +// real push decide. Distinct from the transient errors so telemetry can tell +// "we couldn't reach GitHub" apart from "we deliberately didn't ask". type PushPermissionUnknownError = | 'network' | 'timeout' | 'rate-limit' | 'token-invalid' - | 'malformed-response'; + | 'malformed-response' + | 'ssh-unverified'; /** * Outcome of a single push-permission probe. @@ -72,6 +79,14 @@ export interface CheckPushPermissionOptions { repo: string; /** GitHub host. Defaults to `'github.com'`. */ host?: string; + /** + * How the origin URL authenticates a push. Defaults to `'https'` (token + * auth — the pre-transport behavior). For `'ssh'`/`'git'` origins an + * anonymous probe abstains (`unknown/ssh-unverified`) instead of denying: + * no token ⇒ no HTTPS push, but SSH pushes auth with keys the probe + * cannot see. + */ + transport?: OriginTransport; /** Tier A resolver (`gh` CLI). Defaults to "no gh available". */ detectGh?: DetectGhFn; /** Tier B/C credential store. Omit to skip stored-token resolution. */ @@ -207,6 +222,7 @@ async function runProbe(opts: CheckPushPermissionOptions): Promise ({ available: false }), tokenStore, _fetchFn = fetch, @@ -219,13 +235,30 @@ async function runProbe(opts: CheckPushPermissionOptions): Promise { host: 'github.com', owner: 'inkeep', repo: 'open-knowledge', + transport: 'https', }); }); @@ -131,6 +132,7 @@ describe('readOriginGitHubRepo', () => { host: 'github.com', owner: 'inkeep', repo: 'open-knowledge', + transport: 'ssh', }); }); @@ -143,6 +145,7 @@ describe('readOriginGitHubRepo', () => { host: 'github.com', owner: 'inkeep', repo: 'open-knowledge', + transport: 'ssh', }); }); @@ -155,6 +158,7 @@ describe('readOriginGitHubRepo', () => { host: 'github.com', owner: 'inkeep', repo: 'open-knowledge', + transport: 'https', }); }); @@ -178,6 +182,7 @@ describe('readOriginGitHubRepo', () => { host: 'ghes.acme.test', owner: 'inkeep', repo: 'open-knowledge', + transport: 'https', }); }); @@ -190,6 +195,7 @@ describe('readOriginGitHubRepo', () => { host: 'github.corp.example.com', owner: 'team', repo: 'kb', + transport: 'ssh', }); }); @@ -204,6 +210,7 @@ describe('readOriginGitHubRepo', () => { host: 'ghes.acme.test', owner: 'acme', repo: 'kb', + transport: 'https', }); }); @@ -216,6 +223,7 @@ describe('readOriginGitHubRepo', () => { host: 'github.com', owner: 'inkeep', repo: 'open-knowledge', + transport: 'git', }); }); @@ -228,6 +236,23 @@ describe('readOriginGitHubRepo', () => { host: 'github.com', owner: 'inkeep', repo: 'open-knowledge', + transport: 'https', + }); + }); + + test('ssh:// origin with a port carries transport ssh and a port-stripped host', () => { + // The exact shape of the local-forge repro: `ssh://git@localhost:2222/...` + // against a self-hosted Gitea. Must classify as ssh so the anonymous + // probe abstains instead of pausing sync. + seedRepo(dir, { + config: '[remote "origin"]\n\turl = ssh://git@git.acme.test:2222/acme/kb.git\n', + }); + expect(readOriginGitHubRepo(dir)).toEqual({ + kind: 'ok', + host: 'git.acme.test', + owner: 'acme', + repo: 'kb', + transport: 'ssh', }); }); @@ -276,6 +301,7 @@ describe('readOriginGitHubRepo', () => { host: 'github.com', owner: 'inkeep', repo: 'open-knowledge', + transport: 'https', }); }); @@ -289,6 +315,7 @@ describe('readOriginGitHubRepo', () => { host: 'github.com', owner: 'inkeep', repo: 'open-knowledge', + transport: 'https', }); }); }); @@ -502,6 +529,7 @@ describe('linked-worktree common-dir resolution', () => { host: 'github.com', owner: 'inkeep', repo: 'open-knowledge', + transport: 'https', }); }); diff --git a/packages/server/src/share/git-context.ts b/packages/server/src/share/git-context.ts index 7a14910f..612a267e 100644 --- a/packages/server/src/share/git-context.ts +++ b/packages/server/src/share/git-context.ts @@ -33,9 +33,18 @@ import { getLogger } from '../logger.ts'; const log = getLogger('git-context'); +/** + * Which URL form the origin was written in. Determines how a push would + * authenticate: `https` pushes auth with tokens; `ssh` (both `ssh://` and + * scp-style) auths with SSH keys; `git://` is unauthenticated. The + * push-permission probe keys leniency off this — token absence proves nothing + * about push ability for a non-token transport. + */ +export type OriginTransport = 'https' | 'ssh' | 'git'; + /** Outcome of `readOriginGitHubRepo`. */ export type OriginResult = - | { kind: 'ok'; host: string; owner: string; repo: string } + | { kind: 'ok'; host: string; owner: string; repo: string; transport: OriginTransport } | { kind: 'no-remote' } | { kind: 'non-github' }; @@ -58,6 +67,7 @@ interface ParsedOriginRepo { host: string; owner: string; repo: string; + transport: OriginTransport; } /** @@ -82,30 +92,35 @@ function parseGitHubOriginUrl(originUrl: string): ParsedOriginRepo | null { const raw = originUrl.trim(); if (!raw) return null; - const classify = (host: string, owner: string, repo: string): ParsedOriginRepo | null => { + const classify = ( + host: string, + owner: string, + repo: string, + transport: OriginTransport, + ): ParsedOriginRepo | null => { const normalized = normalizeGitHost(host); if (KNOWN_NON_GITHUB_GIT_HOSTS.has(normalized)) return null; - return { host: normalized, owner, repo }; + return { host: normalized, owner, repo, transport }; }; // https://[:port]//(.git)? let m = /^https?:\/\/([\w.-]+(?::\d+)?)\/([\w.\-~%]+)\/([\w.\-~%]+?)(?:\.git)?\/?$/.exec(raw); - if (m) return classify(m[1], m[2], m[3]); + if (m) return classify(m[1], m[2], m[3], 'https'); // ssh://[user@][:port]//(.git)? m = /^ssh:\/\/(?:[\w.-]+@)?([\w.-]+)(?::\d+)?\/([\w.\-~%]+)\/([\w.\-~%]+?)(?:\.git)?\/?$/.exec( raw, ); - if (m) return classify(m[1], m[2], m[3]); + if (m) return classify(m[1], m[2], m[3], 'ssh'); // @:/(.git)? (scp-style; `@` is required, so // Windows drive paths like `C:\x` can never match) m = /^[\w.-]+@([\w.-]+):([\w.\-~%]+)\/([\w.\-~%]+?)(?:\.git)?$/.exec(raw); - if (m) return classify(m[1], m[2], m[3]); + if (m) return classify(m[1], m[2], m[3], 'ssh'); // git://[:port]//(.git)? m = /^git:\/\/([\w.-]+(?::\d+)?)\/([\w.\-~%]+)\/([\w.\-~%]+?)(?:\.git)?\/?$/.exec(raw); - if (m) return classify(m[1], m[2], m[3]); + if (m) return classify(m[1], m[2], m[3], 'git'); return null; } @@ -131,8 +146,8 @@ export function readOriginGitHubRepo(projectDir: string): OriginResult { const parsed = readParsedOrigin(projectDir); if (!parsed) return { kind: 'no-remote' }; if (parsed.github) { - const { host, owner, repo } = parsed.github; - return { kind: 'ok', host, owner, repo }; + const { host, owner, repo, transport } = parsed.github; + return { kind: 'ok', host, owner, repo, transport }; } // Origin URL present but a known non-GitHub forge or unparseable — surface // as `non-github` so the caller renders the matching toast. diff --git a/packages/server/src/sync-engine.test.ts b/packages/server/src/sync-engine.test.ts index f4ad0d3b..722a268c 100644 --- a/packages/server/src/sync-engine.test.ts +++ b/packages/server/src/sync-engine.test.ts @@ -1597,6 +1597,39 @@ describe('SyncEngine push-permission probe', () => { expect(persisted).toBe(false); }); + test('passes the origin transport through to the probe (ssh origin)', async () => { + await initGitWithOrigin('git@git.example.com:acme/kb.git'); + const probe = fakeProbe({ kind: 'unknown', error: 'ssh-unverified' }); + const engine = makeProbeEngine({ syncEnabled: false, fakeProbe: probe.fn }); + await engine.start(); + await waitForPushPermissionResolved(engine); + expect(probe.opts[0]).toMatchObject({ + owner: 'acme', + repo: 'kb', + host: 'git.example.com', + transport: 'ssh', + }); + }); + + test('ssh-unverified probe result does NOT pause the engine (self-hosted forge over SSH)', async () => { + // The regression behind the forge sync pause: an SSH-origin user with no + // gh/OK token used to land in denied/not-authenticated → pausedReason= + // 'no-push-permission' → disabled, with a GitHub Sign-in button that can + // never help. The abstaining probe result must leave sync running. + await initGitWithOrigin('git@git.example.com:acme/kb.git'); + const probe = fakeProbe({ kind: 'unknown', error: 'ssh-unverified' }); + const engine = makeProbeEngine({ syncEnabled: true, fakeProbe: probe.fn }); + await engine.start(); + await waitForPushPermissionResolved(engine); + const status = engine.getStatus(); + expect(status.pushPermission).toEqual({ + checkStatus: 'unknown', + unknownError: 'ssh-unverified', + }); + expect(status.state).toBe('idle'); + expect(status.pausedReason).not.toBe('no-push-permission'); + }); + test('records `unknown` without changing state', async () => { await initGitWithOrigin(); const probe = fakeProbe({ kind: 'unknown', error: 'network' }); diff --git a/packages/server/src/sync-engine.ts b/packages/server/src/sync-engine.ts index 64a1fa81..560f1d91 100644 --- a/packages/server/src/sync-engine.ts +++ b/packages/server/src/sync-engine.ts @@ -92,15 +92,16 @@ export type PushPermissionStatus = | { checkStatus: 'allowed' } | { checkStatus: 'denied'; - deniedReason: - | 'no-collaborator' - | 'private-no-access' - | 'repo-not-found' - | 'not-authenticated'; + // Both payload unions derive structurally from the source-of-truth + // `PushPermission` in github-permissions.ts so a code added there can't + // silently drift from this wire shape. The Zod enum in core's + // sync-seed.ts still needs a manual matching update; its round-trip + // test is the runtime net for that half. + deniedReason: Extract['reason']; } | { checkStatus: 'unknown'; - unknownError?: 'network' | 'timeout' | 'rate-limit' | 'token-invalid' | 'malformed-response'; + unknownError?: Extract['error']; }; /** Flatten the tagged `PushPermission` from `github-permissions.ts` to wire shape. */ @@ -875,6 +876,7 @@ export class SyncEngine { owner: origin.owner, repo: origin.repo, host: origin.host, + transport: origin.transport, detectGh: this.detectGh, tokenStore: this.tokenStore, });