git: resolved git URLs should be in normal form

npm · Apr 16, 2015 · 387f889 · 387f889
1 parent 4a97339
commit 387f889
Show file tree

Hide file tree

Showing 2 changed files with 122 additions and 1 deletion.
diff --git a/lib/cache/add-remote-git.js b/lib/cache/add-remote-git.js
@@ -31,7 +31,8 @@ var VALID_VARIABLES = [
   'GIT_SSL_NO_VERIFY'
 ]
 
-module.exports = function addRemoteGit (uri, _cb) {
+module.exports = addRemoteGit
+function addRemoteGit (uri, _cb) {
   assert(typeof uri === 'string', 'must have git URL')
   assert(typeof _cb === 'function', 'must have callback')
   var cb = dezalgo(_cb)
@@ -286,6 +287,12 @@ function resolveHead (from, cloneURL, treeish, cachedRemote, cb) {
       log.silly('resolveHead', from, 'resolved treeish:', resolvedTreeish)
 
       var resolvedURL = getResolved(cloneURL, resolvedTreeish)
+      if (!resolvedURL) {
+        return cb(new Error(
+          'unable to clone ' + from + ' because git clone string ' +
+            cloneURL + ' is in a form npm can\'t handle'
+        ))
+      }
       log.verbose('resolveHead', from, 'resolved Git URL:', resolvedURL)
 
       // generate a unique filename
@@ -411,8 +418,21 @@ function gitEnv () {
   return gitEnv_
 }
 
+addRemoteGit.getResolved = getResolved
 function getResolved (uri, treeish) {
+  // normalize hosted-git-info clone URLs back into regular URLs
+  // this will only work on URLs that hosted-git-info recognizes
+  // https://github.com/npm/npm/issues/7961
+  var rehydrated = hostedFromURL(uri)
+  if (rehydrated) uri = rehydrated.toString()
+
   var parsed = url.parse(uri)
+
+  // non-hosted SSH strings that are not URLs ([email protected]:foo.git) are
+  // no bueno
+  // https://github.com/npm/npm/issues/7961
+  if (!parsed.protocol) return
+
   parsed.hash = treeish
   if (!/^git[+:]/.test(parsed.protocol)) {
     parsed.protocol = 'git+' + parsed.protocol

diff --git a/test/tap/add-remote-git-get-resolved.js b/test/tap/add-remote-git-get-resolved.js
@@ -0,0 +1,101 @@
+'use strict'
+var test = require('tap').test
+
+var npm = require('../../lib/npm.js')
+var common = require('../common-tap.js')
+
+test('setup', function (t) {
+  var opts = {
+    registry: common.registry,
+    loglevel: 'silent'
+  }
+  npm.load(opts, function (er) {
+    t.ifError(er, 'npm loaded without error')
+
+    t.end()
+  })
+})
+
+test('add-remote-git#get-resolved git: passthru', function (t) {
+  var getResolved = require('../../lib/cache/add-remote-git.js').getResolved
+
+  verify('git:github.com/foo/repo')
+  verify('git:github.com/foo/repo.git')
+  verify('git://github.com/foo/repo#decadacefadabade')
+  verify('git://github.com/foo/repo.git#decadacefadabade')
+
+  function verify (uri) {
+    t.equal(
+      getResolved(uri, 'decadacefadabade'),
+      'git://github.com/foo/repo.git#decadacefadabade',
+      uri + ' normalized to canonical form git://github.com/foo/repo.git#decadacefadabade'
+    )
+  }
+  t.end()
+})
+
+test('add-remote-git#get-resolved SSH', function (t) {
+  var getResolved = require('../../lib/cache/add-remote-git.js').getResolved
+
+  t.comment('tests for https://github.com/npm/npm/issues/7961')
+  verify('[email protected]:foo/repo')
+  verify('[email protected]:foo/repo#master')
+  verify('git+ssh://[email protected]/foo/repo#master')
+  verify('git+ssh://[email protected]/foo/repo#decadacefadabade')
+
+  function verify (uri) {
+    t.equal(
+      getResolved(uri, 'decadacefadabade'),
+      'git+ssh://[email protected]/foo/repo.git#decadacefadabade',
+      uri + ' normalized to canonical form git+ssh://[email protected]/foo/repo.git#decadacefadabade'
+    )
+  }
+  t.end()
+})
+
+test('add-remote-git#get-resolved HTTPS', function (t) {
+  var getResolved = require('../../lib/cache/add-remote-git.js').getResolved
+
+  verify('https://github.com/foo/repo')
+  verify('https://github.com/foo/repo#master')
+  verify('git+https://github.com/foo/repo.git#master')
+  verify('git+https://github.com/foo/repo#decadacefadabade')
+
+  function verify (uri) {
+    t.equal(
+      getResolved(uri, 'decadacefadabade'),
+      'git+https://github.com/foo/repo.git#decadacefadabade',
+      uri + ' normalized to canonical form git+https://github.com/foo/repo.git#decadacefadabade'
+    )
+  }
+  t.end()
+})
+
+test('add-remote-git#get-resolved edge cases', function (t) {
+  var getResolved = require('../../lib/cache/add-remote-git.js').getResolved
+
+  t.notOk(
+    getResolved('[email protected]:galbi.git', 'decadacefadabade'),
+    'non-hosted Git SSH non-URI strings are invalid'
+  )
+
+  t.equal(
+    getResolved('git+ssh://git.bananaboat.net/foo', 'decadacefadabade'),
+    'git+ssh://git.bananaboat.net/foo#decadacefadabade',
+    'don\'t break non-hosted SSH URLs'
+  )
+
+  t.equal(
+    getResolved('git://gitbub.com/foo/bar.git', 'decadacefadabade'),
+    'git://gitbub.com/foo/bar.git#decadacefadabade',
+    'don\'t break non-hosted git: URLs'
+  )
+
+  t.comment('test for https://github.com/npm/npm/issues/3224')
+  t.equal(
+    getResolved('git+ssh://[email protected]:my-repo.git#9abe82cb339a70065e75300f62b742622774693c', 'decadacefadabade'),
+    'git+ssh://[email protected]:my-repo.git#decadacefadabade',
+    'preserve weird colon in semi-standard ssh:// URLs'
+  )
+  t.end()
+})