fix: do not allow invalid gist urls

isaacs · isaacs · commit d5cf8309be7a · 2019-08-04T15:48:33.000-07:00
Gists are either name/hex{32}, or just /hex{32}.

If someone omits the project, and just specifies a name, then that is an
error and should be treated as such.

Also, eliminate some dead code paths and bring the tests up to 100%.
diff --git a/git-host-info.js b/git-host-info.js
@@ -28,7 +28,7 @@ var gitHosts = module.exports = {
   gist: {
     'protocols': [ 'git', 'git+ssh', 'git+https', 'ssh', 'https' ],
     'domain': 'gist.github.com',
-    'pathmatch': /^[/](?:([^/]+)[/])?([a-z0-9]+)(?:[.]git)?$/,
+    'pathmatch': /^[/](?:([^/]+)[/])?([a-z0-9]{32,})(?:[.]git)?$/,
     'filetemplate': 'https://gist.githubusercontent.com/{user}/{project}/raw{/committish}/{path}',
     'bugstemplate': 'https://{domain}/{project}',
     'gittemplate': 'git://{domain}/{project}.git{#committish}',
diff --git a/git-host.js b/git-host.js
@@ -1,6 +1,7 @@
 'use strict'
 var gitHosts = require('./git-host-info.js')
 /* eslint-disable node/no-deprecated-api */
+/* istanbul ignore next */
 var extend = Object.assign || require('util')._extend
 
 var GitHost = module.exports = function (type, user, auth, project, committish, defaultRepresentation, opts) {
@@ -127,5 +128,6 @@ GitHost.prototype.getDefaultRepresentation = function () {
 }
 
 GitHost.prototype.toString = function (opts) {
-  return (this[this.default] || this.sshurl).call(this, opts)
+  const method = this.default || /* istanbul ignore next */ 'sshurl'
+  return this[method](opts)
 }
diff --git a/index.js b/index.js
@@ -6,15 +6,14 @@ var LRU = require('lru-cache')
 var cache = new LRU({max: 1000})
 
 var protocolToRepresentationMap = {
-  'git+ssh': 'sshurl',
-  'git+https': 'https',
-  'ssh': 'sshurl',
-  'git': 'git'
+  'git+ssh:': 'sshurl',
+  'git+https:': 'https',
+  'ssh:': 'sshurl',
+  'git:': 'git'
 }
 
 function protocolToRepresentation (protocol) {
-  if (protocol.substr(-1) === ':') protocol = protocol.slice(0, -1)
-  return protocolToRepresentationMap[protocol] || protocol
+  return protocolToRepresentationMap[protocol] || protocol.slice(0, -1)
 }
 
 var authProtocols = {
@@ -65,13 +64,17 @@ function fromUrl (giturl, opts) {
         var pathmatch = gitHostInfo.pathmatch
         var matched = parsed.path.match(pathmatch)
         if (!matched) return
-        if (matched[1] != null) user = decodeURIComponent(matched[1].replace(/^:/, ''))
-        if (matched[2] != null) project = decodeURIComponent(matched[2])
+        if (matched[1] !== null && matched[1] !== undefined) {
+          user = decodeURIComponent(matched[1].replace(/^:/, ''))
+        }
+        project = decodeURIComponent(matched[2])
         defaultRepresentation = protocolToRepresentation(parsed.protocol)
       }
       return new GitHost(gitHostName, user, auth, project, committish, defaultRepresentation, opts)
     } catch (ex) {
-      if (!(ex instanceof URIError)) throw ex
+      /* istanbul ignore else */
+      if (ex instanceof URIError) {
+      } else throw ex
     }
   }).filter(function (gitHostInfo) { return gitHostInfo })
   if (matches.length !== 1) return
@@ -101,7 +104,6 @@ function fixupUnqualifiedGist (giturl) {
 }
 
 function parseGitUrl (giturl) {
-  if (typeof giturl !== 'string') giturl = '' + giturl
   var matched = giturl.match(/^([^@]+)@([^:/]+):[/]?((?:[^/]+[/])?[^/]+?)(?:[.]git)?(#.*)?$/)
   if (!matched) return url.parse(giturl)
   return {
diff --git a/package.json b/package.json
@@ -24,7 +24,7 @@
     "postrelease": "npm publish && git push --follow-tags",
     "pretest": "standard",
     "release": "standard-version -s",
-    "test": "tap -J --nyc-arg=--all --coverage test"
+    "test": "tap -J --100 test/*.js"
   },
   "dependencies": {
     "lru-cache": "^5.1.1"
diff --git a/test/gist.js b/test/gist.js
@@ -3,42 +3,43 @@ var HostedGit = require('../index')
 var test = require('tap').test
 
 test('fromUrl(gist url)', function (t) {
+  var proj = new Array(33).join('2')
   function verify (host, label, branch) {
     var hostinfo = HostedGit.fromUrl(host)
     var hash = branch ? '#' + branch : ''
     t.ok(hostinfo, label)
     if (!hostinfo) return
-    t.is(hostinfo.https(), 'git+https://gist.github.com/222.git' + hash, label + ' -> https')
-    t.is(hostinfo.git(), 'git://gist.github.com/222.git' + hash, label + ' -> git')
-    t.is(hostinfo.browse(), 'https://gist.github.com/222' + (branch ? '/' + branch : ''), label + ' -> browse')
-    t.is(hostinfo.browse('C'), 'https://gist.github.com/222' + (branch ? '/' + branch : '') + '#file-c', label + ' -> browse(path)')
-    t.is(hostinfo.browse('C', 'A'), 'https://gist.github.com/222' + (branch ? '/' + branch : '') + '#file-c', label + ' -> browse(path, fragment)')
-    t.is(hostinfo.bugs(), 'https://gist.github.com/222', label + ' -> bugs')
-    t.is(hostinfo.docs(), 'https://gist.github.com/222' + (branch ? '/' + branch : ''), label + ' -> docs')
-    t.is(hostinfo.ssh(), 'git@gist.github.com:/222.git' + hash, label + ' -> ssh')
-    t.is(hostinfo.sshurl(), 'git+ssh://git@gist.github.com/222.git' + hash, label + ' -> sshurl')
-    t.is(hostinfo.shortcut(), 'gist:222' + hash, label + ' -> shortcut')
+    t.is(hostinfo.https(), 'git+https://gist.github.com/' + proj + '.git' + hash, label + ' -> https')
+    t.is(hostinfo.git(), 'git://gist.github.com/' + proj + '.git' + hash, label + ' -> git')
+    t.is(hostinfo.browse(), 'https://gist.github.com/' + proj + (branch ? '/' + branch : ''), label + ' -> browse')
+    t.is(hostinfo.browse('C'), 'https://gist.github.com/' + proj + (branch ? '/' + branch : '') + '#file-c', label + ' -> browse(path)')
+    t.is(hostinfo.browse('C', 'A'), 'https://gist.github.com/' + proj + (branch ? '/' + branch : '') + '#file-c', label + ' -> browse(path, fragment)')
+    t.is(hostinfo.bugs(), 'https://gist.github.com/' + proj, label + ' -> bugs')
+    t.is(hostinfo.docs(), 'https://gist.github.com/' + proj + (branch ? '/' + branch : ''), label + ' -> docs')
+    t.is(hostinfo.ssh(), 'git@gist.github.com:/' + proj + '.git' + hash, label + ' -> ssh')
+    t.is(hostinfo.sshurl(), 'git+ssh://git@gist.github.com/' + proj + '.git' + hash, label + ' -> sshurl')
+    t.is(hostinfo.shortcut(), 'gist:' + proj + hash, label + ' -> shortcut')
     if (hostinfo.user) {
-      t.is(hostinfo.file('C'), 'https://gist.githubusercontent.com/111/222/raw/' + (branch ? branch + '/' : '') + 'C', label + ' -> file')
-      t.is(hostinfo.tarball(), 'https://gist.github.com/111/222/archive/' + (branch || 'master') + '.tar.gz', label + ' -> tarball')
+      t.is(hostinfo.file('C'), 'https://gist.githubusercontent.com/111/' + proj + '/raw/' + (branch ? branch + '/' : '') + 'C', label + ' -> file')
+      t.is(hostinfo.tarball(), 'https://gist.github.com/111/' + proj + '/archive/' + (branch || 'master') + '.tar.gz', label + ' -> tarball')
     }
   }
 
-  verify('git@gist.github.com:222.git', 'git@')
-  var hostinfo = HostedGit.fromUrl('git@gist.github.com:/ef860c7z5e0de3179341.git')
+  verify('git@gist.github.com:' + proj + '.git', 'git@')
+  var hostinfo = HostedGit.fromUrl('git@gist.github.com:/c2b12db30a49324325a3781302668408.git')
   if (t.ok(hostinfo, 'git@hex')) {
-    t.is(hostinfo.https(), 'git+https://gist.github.com/ef860c7z5e0de3179341.git', 'git@hex -> https')
+    t.is(hostinfo.https(), 'git+https://gist.github.com/c2b12db30a49324325a3781302668408.git', 'git@hex -> https')
   }
-  verify('git@gist.github.com:/222.git', 'git@/')
-  verify('git://gist.github.com/222', 'git')
-  verify('git://gist.github.com/222.git', 'git.git')
-  verify('git://gist.github.com/222#branch', 'git#branch', 'branch')
-  verify('git://gist.github.com/222.git#branch', 'git.git#branch', 'branch')
+  verify('git@gist.github.com:/' + proj + '.git', 'git@/')
+  verify('git://gist.github.com/' + proj, 'git')
+  verify('git://gist.github.com/' + proj + '.git', 'git.git')
+  verify('git://gist.github.com/' + proj + '#branch', 'git#branch', 'branch')
+  verify('git://gist.github.com/' + proj + '.git#branch', 'git.git#branch', 'branch')
 
-  require('./lib/standard-tests')(verify, 'gist.github.com', 'gist')
+  require('./lib/standard-tests')(verify, 'gist.github.com', 'gist', proj)
 
-  verify(HostedGit.fromUrl('gist:111/222').toString(), 'round-tripped shortcut')
-  verify('gist:222', 'shortened shortcut')
+  verify(HostedGit.fromUrl('gist:111/' + proj).toString(), 'round-tripped shortcut')
+  verify('gist:' + proj, 'shortened shortcut')
 
   t.end()
 })
diff --git a/test/github.js b/test/github.js
@@ -6,17 +6,23 @@ test('fromUrl(github url)', function (t) {
   function verify (host, label, branch) {
     var hostinfo = HostedGit.fromUrl(host)
     var hash = branch ? '#' + branch : ''
+    var treebranch = branch ? '/tree/' + branch : ''
+    t.equal(hostinfo._fill(), undefined)
     t.ok(hostinfo, label)
     if (!hostinfo) return
     t.is(hostinfo.https(), 'git+https://github.com/111/222.git' + hash, label + ' -> https')
     t.is(hostinfo.git(), 'git://github.com/111/222.git' + hash, label + ' -> git')
-    t.is(hostinfo.browse(), 'https://github.com/111/222' + (branch ? '/tree/' + branch : ''), label + ' -> browse')
+    t.is(hostinfo.browse(), 'https://github.com/111/222' + treebranch, label + ' -> browse')
     t.is(hostinfo.browse('C'), 'https://github.com/111/222/tree/' + (branch || 'master') + '/C', label + ' -> browse(path)')
     t.is(hostinfo.browse('C', 'A'), 'https://github.com/111/222/tree/' + (branch || 'master') + '/C#a', label + ' -> browse(path, fragment)')
     t.is(hostinfo.bugs(), 'https://github.com/111/222/issues', label + ' -> bugs')
-    t.is(hostinfo.docs(), 'https://github.com/111/222' + (branch ? '/tree/' + branch : '') + '#readme', label + ' -> docs')
+    t.is(hostinfo.docs(), 'https://github.com/111/222' + treebranch + '#readme', label + ' -> docs')
     t.is(hostinfo.ssh(), 'git@github.com:111/222.git' + hash, label + ' -> ssh')
     t.is(hostinfo.sshurl(), 'git+ssh://git@github.com/111/222.git' + hash, label + ' -> sshurl')
+    t.is(hostinfo.sshurl({ noGitPlus: true }), 'ssh://git@github.com/111/222.git' + hash, label + ' -> sshurl (no git plus)')
+    t.is(hostinfo.path(), '111/222' + hash, ' -> path')
+    t.is(hostinfo.hash(), hash, ' -> hash')
+    t.is(hostinfo.path({ noCommittish: true }), '111/222', ' -> path (no committish)')
     t.is(hostinfo.shortcut(), 'github:111/222' + hash, label + ' -> shortcut')
     t.is(hostinfo.file('C'), 'https://raw.githubusercontent.com/111/222/' + (branch || 'master') + '/C', label + ' -> file')
     t.is(hostinfo.tarball(), 'https://codeload.github.com/111/222/tar.gz/' + (branch || 'master'), label + ' -> tarball')
@@ -41,5 +47,7 @@ test('fromUrl(github url)', function (t) {
 
   require('./lib/standard-tests')(verify, 'www.github.com', 'github')
 
+  t.equal(HostedGit.fromUrl('git+ssh://github.com/foo.git'), undefined)
+
   t.end()
 })
diff --git a/test/lib/standard-tests.js b/test/lib/standard-tests.js
@@ -1,27 +1,28 @@
 'use strict'
-module.exports = function (verify, domain, shortname) {
-  verify('https://' + domain + '/111/222', 'https')
-  verify('https://' + domain + '/111/222.git', 'https.git')
-  verify('https://' + domain + '/111/222#branch', 'https#branch', 'branch')
-  verify('https://' + domain + '/111/222.git#branch', 'https.git#branch', 'branch')
+module.exports = function (verify, domain, shortname, proj) {
+  proj = proj || '222'
+  verify('https://' + domain + '/111/' + proj, 'https')
+  verify('https://' + domain + '/111/' + proj + '.git', 'https.git')
+  verify('https://' + domain + '/111/' + proj + '#branch', 'https#branch', 'branch')
+  verify('https://' + domain + '/111/' + proj + '.git#branch', 'https.git#branch', 'branch')
 
-  verify('git+https://' + domain + '/111/222', 'git+https')
-  verify('git+https://' + domain + '/111/222.git', 'git+https.git')
-  verify('git+https://' + domain + '/111/222#branch', 'git+https#branch', 'branch')
-  verify('git+https://' + domain + '/111/222.git#branch', 'git+https.git#branch', 'branch')
+  verify('git+https://' + domain + '/111/' + proj, 'git+https')
+  verify('git+https://' + domain + '/111/' + proj + '.git', 'git+https.git')
+  verify('git+https://' + domain + '/111/' + proj + '#branch', 'git+https#branch', 'branch')
+  verify('git+https://' + domain + '/111/' + proj + '.git#branch', 'git+https.git#branch', 'branch')
 
-  verify('git@' + domain + ':111/222', 'ssh')
-  verify('git@' + domain + ':111/222.git', 'ssh.git')
-  verify('git@' + domain + ':111/222#branch', 'ssh', 'branch')
-  verify('git@' + domain + ':111/222.git#branch', 'ssh.git', 'branch')
+  verify('git@' + domain + ':111/' + proj, 'ssh')
+  verify('git@' + domain + ':111/' + proj + '.git', 'ssh.git')
+  verify('git@' + domain + ':111/' + proj + '#branch', 'ssh', 'branch')
+  verify('git@' + domain + ':111/' + proj + '.git#branch', 'ssh.git', 'branch')
 
-  verify('git+ssh://git@' + domain + '/111/222', 'ssh url')
-  verify('git+ssh://git@' + domain + '/111/222.git', 'ssh url.git')
-  verify('git+ssh://git@' + domain + '/111/222#branch', 'ssh url#branch', 'branch')
-  verify('git+ssh://git@' + domain + '/111/222.git#branch', 'ssh url.git#branch', 'branch')
+  verify('git+ssh://git@' + domain + '/111/' + proj, 'ssh url')
+  verify('git+ssh://git@' + domain + '/111/' + proj + '.git', 'ssh url.git')
+  verify('git+ssh://git@' + domain + '/111/' + proj + '#branch', 'ssh url#branch', 'branch')
+  verify('git+ssh://git@' + domain + '/111/' + proj + '.git#branch', 'ssh url.git#branch', 'branch')
 
-  verify(shortname + ':111/222', 'shortcut')
-  verify(shortname + ':111/222.git', 'shortcut.git')
-  verify(shortname + ':111/222#branch', 'shortcut#branch', 'branch')
-  verify(shortname + ':111/222.git#branch', 'shortcut.git#branch', 'branch')
+  verify(shortname + ':111/' + proj, 'shortcut')
+  verify(shortname + ':111/' + proj + '.git', 'shortcut.git')
+  verify(shortname + ':111/' + proj + '#branch', 'shortcut#branch', 'branch')
+  verify(shortname + ':111/' + proj + '.git#branch', 'shortcut.git#branch', 'branch')
 }

Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,7 @@`
`1`	`1`	`'use strict'`
`2`	`2`	`var gitHosts = require('./git-host-info.js')`
`3`	`3`	`/* eslint-disable node/no-deprecated-api */`
	`4`	`+/* istanbul ignore next */`
`4`	`5`	`var extend = Object.assign \|\| require('util')._extend`
`5`	`6`
`6`	`7`	`var GitHost = module.exports = function (type, user, auth, project, committish, defaultRepresentation, opts) {`
`@@ -127,5 +128,6 @@ GitHost.prototype.getDefaultRepresentation = function () {`
`127`	`128`	`}`
`128`	`129`
`129`	`130`	`GitHost.prototype.toString = function (opts) {`
`130`		`- return (this[this.default] \|\| this.sshurl).call(this, opts)`
	`131`	`+ const method = this.default \|\| /* istanbul ignore next */ 'sshurl'`
	`132`	`+ return this[method](opts)`
`131`	`133`	`}`