Skip to content

Commit

Permalink
fix: use hosted-git-info to parse registry urls
Browse files Browse the repository at this point in the history 
Previously this was using `new URL` which would fail on some urls that
`hosted-git-info` is able to parse. But if we still get a url that can't
be parsed, we now set it to be removed from the tree instead of
erroring.

Fixes: #5278
  • Loading branch information
@lukekarrys
lukekarrys committed Oct 26, 2022
1 parent b61dedc commit 951d22e
Show file tree
Hide file tree
Showing 5 changed files with 103 additions and 7 deletions.
2 changes: 2 additions & 0 deletions DEPENDENCIES.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ graph LR;
npm-registry-fetch-->proc-log;
npmcli-arborist-->bin-links;
npmcli-arborist-->cacache;
npmcli-arborist-->hosted-git-info;
npmcli-arborist-->json-parse-even-better-errors;
npmcli-arborist-->minify-registry-metadata;
npmcli-arborist-->nopt;
Expand Down Expand Up @@ -791,6 +792,7 @@ graph LR;
npmcli-arborist-->cacache;
npmcli-arborist-->chalk;
npmcli-arborist-->common-ancestor-path;
npmcli-arborist-->hosted-git-info;
npmcli-arborist-->isaacs-string-locale-compare["@isaacs/string-locale-compare"];
npmcli-arborist-->json-parse-even-better-errors;
npmcli-arborist-->json-stringify-nice;
Expand Down
1 change: 1 addition & 0 deletions package-lock.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14053,6 +14053,7 @@
"bin-links": "^4.0.1",
"cacache": "^17.0.1",
"common-ancestor-path": "^1.0.1",
"hosted-git-info": "^6.1.0",
"json-parse-even-better-errors": "^3.0.0",
"json-stringify-nice": "^1.1.4",
"minimatch": "^5.1.0",
Expand Down
27 changes: 21 additions & 6 deletions workspaces/arborist/lib/arborist/reify.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const semver = require('semver')
const debug = require('../debug.js')
const walkUp = require('walk-up-path')
const log = require('proc-log')
const hgi = require('hosted-git-info')

const { dirname, resolve, relative } = require('path')
const { depth: dfwalk } = require('treeverse')
Expand Down Expand Up @@ -640,10 +641,15 @@ module.exports = cls => class Reifier extends cls {
// and no 'bundled: true' setting.
// Do the best with what we have, or else remove it from the tree
// entirely, since we can't possibly reify it.
const res = node.resolved ? `${node.name}@${this[_registryResolved](node.resolved)}`
: node.packageName && node.version
? `${node.packageName}@${node.version}`
: null
let res = null
if (node.resolved) {
const registryResolved = this[_registryResolved](node.resolved)
if (registryResolved) {
res = `${node.name}@${registryResolved}`
}
} else if (node.packageName && node.version) {
res = `${node.packageName}@${node.version}`
}

// no idea what this thing is. remove it from the tree.
if (!res) {
Expand Down Expand Up @@ -721,12 +727,21 @@ module.exports = cls => class Reifier extends cls {
// ${REGISTRY} or something. This has to be threaded through the
// Shrinkwrap and Node classes carefully, so for now, just treat
// the default reg as the magical animal that it has been.
const resolvedURL = new URL(resolved)
// const resolvedURL = new URL(resolved)
const resolvedURL = hgi.parseUrl(resolved)

if (!resolvedURL) {
// if we could not parse the url at all then returning nothing
// here means it will get removed from the tree in the next step
return
}

if ((this.options.replaceRegistryHost === resolvedURL.hostname)
|| this.options.replaceRegistryHost === 'always') {
// this.registry always has a trailing slash
resolved = `${this.registry.slice(0, -1)}${resolvedURL.pathname}${resolvedURL.searchParams}`
return `${this.registry.slice(0, -1)}${resolvedURL.pathname}${resolvedURL.searchParams}`
}

return resolved
}

Expand Down
1 change: 1 addition & 0 deletions workspaces/arborist/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
"bin-links": "^4.0.1",
"cacache": "^17.0.1",
"common-ancestor-path": "^1.0.1",
"hosted-git-info": "^6.1.0",
"json-parse-even-better-errors": "^3.0.0",
"json-stringify-nice": "^1.1.4",
"minimatch": "^5.1.0",
Expand Down
79 changes: 78 additions & 1 deletion workspaces/arborist/test/arborist/reify.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2936,7 +2936,20 @@ t.test('installLinks', (t) => {
})

t.only('should preserve exact ranges, missing actual tree', async (t) => {
const Arborist = require('../../lib/index.js')
const Pacote = require('pacote')
const Arborist = t.mock('../../lib/arborist', {
pacote: {
...Pacote,
extract: async (...args) => {
if (args[0].startsWith('gitssh')) {
// we just want to test that this url is handled properly
// but its not a real git url we can clone so return early
return true
}
return Pacote.extract(...args)
},
},
})
const abbrev = resolve(__dirname,
'../fixtures/registry-mocks/content/abbrev/-/abbrev-1.1.1.tgz')
const abbrevTGZ = fs.readFileSync(abbrev)
Expand Down Expand Up @@ -2973,6 +2986,40 @@ t.only('should preserve exact ranges, missing actual tree', async (t) => {
},
})

const gitSshPackument = JSON.stringify({
_id: 'gitssh',
_rev: 'lkjadflkjasdf',
name: 'gitssh',
'dist-tags': { latest: '1.1.1' },
versions: {
'1.1.1': {
name: 'gitssh',
version: '1.1.1',
dist: {
// this is a url that `new URL()` cant parse
// https://github.com/npm/cli/issues/5278
tarball: 'git+ssh://git@github.com:a/b/c.git#lkjadflkjasdf',
},
},
},
})

const notAUrlPackument = JSON.stringify({
_id: 'notaurl',
_rev: 'lkjadflkjasdf',
name: 'notaurl',
'dist-tags': { latest: '1.1.1' },
versions: {
'1.1.1': {
name: 'notaurl',
version: '1.1.1',
dist: {
tarball: 'hey been trying to break this test',
},
},
},
})

t.only('host should not be replaced replaceRegistryHost=never', async (t) => {
const testdir = t.testdir({
project: {
Expand All @@ -2981,6 +3028,8 @@ t.only('should preserve exact ranges, missing actual tree', async (t) => {
version: '1.0.0',
dependencies: {
abbrev: '1.1.1',
gitssh: '1.1.1',
notaurl: '1.1.1',
},
}),
},
Expand All @@ -2994,6 +3043,14 @@ t.only('should preserve exact ranges, missing actual tree', async (t) => {
.get('/abbrev/-/abbrev-1.1.1.tgz')
.reply(200, abbrevTGZ)

tnock(t, 'https://registry.github.com')
.get('/gitssh')
.reply(200, gitSshPackument)

tnock(t, 'https://registry.github.com')
.get('/notaurl')
.reply(200, notAUrlPackument)

const arb = new Arborist({
path: resolve(testdir, 'project'),
registry: 'https://registry.github.com',
Expand All @@ -3011,6 +3068,8 @@ t.only('should preserve exact ranges, missing actual tree', async (t) => {
version: '1.0.0',
dependencies: {
abbrev: '1.1.1',
gitssh: '1.1.1',
notaurl: '1.1.1',
},
}),
},
Expand All @@ -3020,10 +3079,18 @@ t.only('should preserve exact ranges, missing actual tree', async (t) => {
.get('/abbrev')
.reply(200, abbrevPackument)

tnock(t, 'https://registry.github.com')
.get('/gitssh')
.reply(200, gitSshPackument)

tnock(t, 'https://registry.github.com')
.get('/abbrev/-/abbrev-1.1.1.tgz')
.reply(200, abbrevTGZ)

tnock(t, 'https://registry.github.com')
.get('/notaurl')
.reply(200, notAUrlPackument)

const arb = new Arborist({
path: resolve(testdir, 'project'),
registry: 'https://registry.github.com',
Expand All @@ -3041,6 +3108,8 @@ t.only('should preserve exact ranges, missing actual tree', async (t) => {
version: '1.0.0',
dependencies: {
abbrev: '1.1.1',
gitssh: '1.1.1',
notaurl: '1.1.1',
},
}),
},
Expand All @@ -3050,10 +3119,18 @@ t.only('should preserve exact ranges, missing actual tree', async (t) => {
.get('/abbrev')
.reply(200, abbrevPackument2)

tnock(t, 'https://registry.github.com')
.get('/gitssh')
.reply(200, gitSshPackument)

tnock(t, 'https://registry.github.com')
.get('/abbrev/-/abbrev-1.1.1.tgz')
.reply(200, abbrevTGZ)

tnock(t, 'https://registry.github.com')
.get('/notaurl')
.reply(200, notAUrlPackument)

const arb = new Arborist({
path: resolve(testdir, 'project'),
registry: 'https://registry.github.com',
Expand Down

0 comments on commit 951d22e

Please sign in to comment.