fix: smarter git ssh override (#194)

npm will no longer manually set `GIT_ASKPASS` or `GIT_SSH_COMMAND` if it finds those values already defined in the user's git config.

## References

npm/cli#2891
#193
#129

---------

Co-authored-by: pacotedev <i+pacotedev@izs.me>

lib/opts.js

@@ -1,12 +1,57 @@
+const fs = require('node:fs')
+const os = require('node:os')
+const path = require('node:path')
+const ini = require('ini')
+
+const gitConfigPath = path.join(os.homedir(), '.gitconfig')
+
+let cachedConfig = null
+
+// Function to load and cache the git config
+const loadGitConfig = () => {
+  if (cachedConfig === null) {
+    try {
+      cachedConfig = {}
+      if (fs.existsSync(gitConfigPath)) {
+        const configContent = fs.readFileSync(gitConfigPath, 'utf-8')
+        cachedConfig = ini.parse(configContent)
+      }
+    } catch (error) {
+      cachedConfig = {}
+    }
+  }
+  return cachedConfig
+}
+
+const checkGitConfigs = () => {
+  const config = loadGitConfig()
+  return {
+    sshCommandSetInConfig: config?.core?.sshCommand !== undefined,
+    askPassSetInConfig: config?.core?.askpass !== undefined,
+  }
+}
+
+const sshCommandSetInEnv = process.env.GIT_SSH_COMMAND !== undefined
+const askPassSetInEnv = process.env.GIT_ASKPASS !== undefined
+const { sshCommandSetInConfig, askPassSetInConfig } = checkGitConfigs()
+
 // Values we want to set if they're not already defined by the end user
 // This defaults to accepting new ssh host key fingerprints
-const gitEnv = {
-  GIT_ASKPASS: 'echo',
-  GIT_SSH_COMMAND: 'ssh -oStrictHostKeyChecking=accept-new',
+const finalGitEnv = {
+  ...(askPassSetInEnv || askPassSetInConfig ? {} : {
+    GIT_ASKPASS: 'echo',
+  }),
+  ...(sshCommandSetInEnv || sshCommandSetInConfig ? {} : {
+    GIT_SSH_COMMAND: 'ssh -oStrictHostKeyChecking=accept-new',
+  }),
 }
+
 module.exports = (opts = {}) => ({
   stdioString: true,
   ...opts,
   shell: false,
-  env: opts.env || { ...gitEnv, ...process.env },
+  env: opts.env || { ...finalGitEnv, ...process.env },
 })
+
+// Export the loadGitConfig function for testing
+module.exports.loadGitConfig = loadGitConfig

package.json

@@ -38,6 +38,7 @@
   },
   "dependencies": {
     "@npmcli/promise-spawn": "^7.0.0",
+    "ini": "^4.1.3",
     "lru-cache": "^10.0.1",
     "npm-pick-manifest": "^9.0.0",
     "proc-log": "^4.0.0",

test/opts.js

@@ -1,40 +1,94 @@
 const t = require('tap')
-const gitOpts = require('../lib/opts.js')
+const ini = require('ini')
+let [GIT_ASKPASS, GIT_SSH_COMMAND] = ['', '']
+
+const mockFs = {
+  existsSync: () => false,
+  readFileSync: () => '',
+}
+
+const gitOpts = t.mock('../lib/opts.js', {
+  'node:fs': mockFs,
+})
+
+t.beforeEach(() => {
+  backupEnv()
+})
+
+t.afterEach(() => {
+  restoreEnv()
+})
 
 t.test('defaults', t => {
-  const { GIT_ASKPASS, GIT_SSH_COMMAND } = process.env
-  t.teardown(() => {
-    process.env.GIT_ASKPASS = GIT_ASKPASS
-    process.env.GIT_SSH_COMMAND = GIT_SSH_COMMAND
-  })
-  delete process.env.GIT_ASKPASS
-  delete process.env.GIT_SSH_COMMAND
-  t.match(gitOpts().env, {
-    GIT_ASKPASS: 'echo',
-    GIT_SSH_COMMAND: 'ssh -oStrictHostKeyChecking=accept-new',
+  t.match(gitOpts(), {
+    env: {
+      GIT_ASKPASS: 'echo',
+      GIT_SSH_COMMAND: 'ssh -oStrictHostKeyChecking=accept-new',
+    },
+    shell: false,
   }, 'got the git defaults we want')
-  t.equal(gitOpts().shell, false, 'shell defaults to false')
-  t.equal(gitOpts({ shell: '/bin/bash' }).shell, false, 'shell cannot be overridden')
+
   t.end()
 })
 
-t.test('does not override', t => {
-  const { GIT_ASKPASS, GIT_SSH_COMMAND } = process.env
-  t.teardown(() => {
-    process.env.GIT_ASKPASS = GIT_ASKPASS
-    process.env.GIT_SSH_COMMAND = GIT_SSH_COMMAND
+t.test('handle case when fs.existsSync throws an error', t => {
+  const gitOptsWithMockFs = t.mock('../lib/opts.js', {
+    'node:fs': {
+      ...mockFs,
+      existsSync: () => {
+        throw new Error('Mocked error')
+      },
+    },
   })
+
+  t.match(gitOptsWithMockFs(), {
+    env: {
+      GIT_ASKPASS: 'echo',
+      GIT_SSH_COMMAND: 'ssh -oStrictHostKeyChecking=accept-new',
+    },
+    shell: false,
+  }, 'should apply defaults when fs.existsSync throws an error')
+
+  t.end()
+})
+
+t.test('handle case when git config does not exist', t => {
+  const gitOptsWithMockFs = t.mock('../lib/opts.js', {
+    'node:fs': {
+      ...mockFs,
+      existsSync: () => false,
+    },
+  })
+
+  t.match(gitOptsWithMockFs(), {
+    env: {
+      GIT_ASKPASS: 'echo',
+      GIT_SSH_COMMAND: 'ssh -oStrictHostKeyChecking=accept-new',
+    },
+    shell: false,
+  }, 'should apply defaults when git config does not exist')
+
+  t.end()
+})
+
+t.test('does not override when sshCommand is set in env', t => {
   process.env.GIT_ASKPASS = 'test_askpass'
   process.env.GIT_SSH_COMMAND = 'test_ssh_command'
-  t.match(gitOpts().env, {
-    GIT_ASKPASS: 'test_askpass',
-    GIT_SSH_COMMAND: 'test_ssh_command',
+
+  t.match(gitOpts(), {
+    env: {
+      GIT_ASKPASS: 'test_askpass',
+      GIT_SSH_COMMAND: 'test_ssh_command',
+    },
+    shell: false,
   }, 'values already in process.env remain')
+
   t.end()
 })
 
 t.test('as non-root', t => {
   process.getuid = () => 999
+
   t.match(gitOpts({
     foo: 'bar',
     env: { override: 'for some reason' },
@@ -49,5 +103,80 @@ t.test('as non-root', t => {
     gid: undefined,
     abc: undefined,
   }, 'do not set uid/gid as non-root')
+
+  t.end()
+})
+
+t.test('does not override when sshCommand is set in git config', t => {
+  const gitConfigContent = `[core]
+  askpass = echo
+  sshCommand = custom_ssh_command
+`
+  const gitOptsWithMockFs = t.mock('../lib/opts.js', {
+    'node:fs': {
+      ...mockFs,
+      existsSync: () => true,
+      readFileSync: () => gitConfigContent,
+    },
+  })
+
+  t.match(gitOptsWithMockFs(), {
+    env: {
+      GIT_ASKPASS: null,
+      GIT_SSH_COMMAND: null,
+    },
+    shell: false,
+  }, 'sshCommand in git config remains')
+
   t.end()
 })
+
+t.test('does not override when sshCommand is set in git config', t => {
+  const gitConfigContent = `[core]
+  askpass = echo
+  sshCommand = custom_ssh_command
+`
+
+  const { loadGitConfig } = t.mock('../lib/opts.js', {
+    'node:fs': {
+      ...mockFs,
+      existsSync: () => true,
+      readFileSync: () => gitConfigContent,
+    },
+  })
+
+  t.match(loadGitConfig(),
+    ini.parse(gitConfigContent),
+    'cachedConfig should be populated with git config'
+  )
+
+  const gitOptsWithMockFs = t.mock('../lib/opts.js', {
+    'node:fs': {
+      ...mockFs,
+      existsSync: () => true,
+      readFileSync: () => gitConfigContent,
+    },
+  })
+
+  t.match(gitOptsWithMockFs(), {
+    env: {
+      GIT_ASKPASS: null,
+      GIT_SSH_COMMAND: null,
+    },
+    shell: false,
+  }, 'sshCommand in git config remains')
+
+  t.end()
+})
+
+function backupEnv () {
+  GIT_ASKPASS = process.env.GIT_ASKPASS
+  GIT_SSH_COMMAND = process.env.GIT_SSH_COMMAND
+  delete process.env.GIT_ASKPASS
+  delete process.env.GIT_SSH_COMMAND
+}
+
+function restoreEnv () {
+  process.env.GIT_ASKPASS = GIT_ASKPASS
+  process.env.GIT_SSH_COMMAND = GIT_SSH_COMMAND
+}