Skip to content

Commit 6a3688a

Browse files
committed
fix: use hosted-git-info to parse registry urls
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
1 parent ca93f3e commit 6a3688a

File tree

5 files changed

+103
-7
lines changed

5 files changed

+103
-7
lines changed

DEPENDENCIES.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,7 @@ graph LR;
157157
npm-registry-fetch-->proc-log;
158158
npmcli-arborist-->bin-links;
159159
npmcli-arborist-->cacache;
160+
npmcli-arborist-->hosted-git-info;
160161
npmcli-arborist-->json-parse-even-better-errors;
161162
npmcli-arborist-->minify-registry-metadata;
162163
npmcli-arborist-->nopt;
@@ -790,6 +791,7 @@ graph LR;
790791
npmcli-arborist-->cacache;
791792
npmcli-arborist-->chalk;
792793
npmcli-arborist-->common-ancestor-path;
794+
npmcli-arborist-->hosted-git-info;
793795
npmcli-arborist-->isaacs-string-locale-compare["@isaacs/string-locale-compare"];
794796
npmcli-arborist-->json-parse-even-better-errors;
795797
npmcli-arborist-->json-stringify-nice;

package-lock.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14050,6 +14050,7 @@
1405014050
"bin-links": "^4.0.1",
1405114051
"cacache": "^17.0.1",
1405214052
"common-ancestor-path": "^1.0.1",
14053+
"hosted-git-info": "^6.1.0",
1405314054
"json-parse-even-better-errors": "^3.0.0",
1405414055
"json-stringify-nice": "^1.1.4",
1405514056
"minimatch": "^5.1.0",

workspaces/arborist/lib/arborist/reify.js

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ const semver = require('semver')
99
const debug = require('../debug.js')
1010
const walkUp = require('walk-up-path')
1111
const log = require('proc-log')
12+
const hgi = require('hosted-git-info')
1213

1314
const { dirname, resolve, relative } = require('path')
1415
const { depth: dfwalk } = require('treeverse')
@@ -640,10 +641,15 @@ module.exports = cls => class Reifier extends cls {
640641
// and no 'bundled: true' setting.
641642
// Do the best with what we have, or else remove it from the tree
642643
// entirely, since we can't possibly reify it.
643-
const res = node.resolved ? `${node.name}@${this[_registryResolved](node.resolved)}`
644-
: node.packageName && node.version
645-
? `${node.packageName}@${node.version}`
646-
: null
644+
let res = null
645+
if (node.resolved) {
646+
const registryResolved = this[_registryResolved](node.resolved)
647+
if (registryResolved) {
648+
res = `${node.name}@${registryResolved}`
649+
}
650+
} else if (node.packageName && node.version) {
651+
res = `${node.packageName}@${node.version}`
652+
}
647653

648654
// no idea what this thing is. remove it from the tree.
649655
if (!res) {
@@ -721,12 +727,21 @@ module.exports = cls => class Reifier extends cls {
721727
// ${REGISTRY} or something. This has to be threaded through the
722728
// Shrinkwrap and Node classes carefully, so for now, just treat
723729
// the default reg as the magical animal that it has been.
724-
const resolvedURL = new URL(resolved)
730+
// const resolvedURL = new URL(resolved)
731+
const resolvedURL = hgi.parseUrl(resolved)
732+
733+
if (!resolvedURL) {
734+
// if we could not parse the url at all then returning nothing
735+
// here means it will get removed from the tree in the next step
736+
return
737+
}
738+
725739
if ((this.options.replaceRegistryHost === resolvedURL.hostname)
726740
|| this.options.replaceRegistryHost === 'always') {
727741
// this.registry always has a trailing slash
728-
resolved = `${this.registry.slice(0, -1)}${resolvedURL.pathname}${resolvedURL.searchParams}`
742+
return `${this.registry.slice(0, -1)}${resolvedURL.pathname}${resolvedURL.searchParams}`
729743
}
744+
730745
return resolved
731746
}
732747

workspaces/arborist/package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
"bin-links": "^4.0.1",
1717
"cacache": "^17.0.1",
1818
"common-ancestor-path": "^1.0.1",
19+
"hosted-git-info": "^6.1.0",
1920
"json-parse-even-better-errors": "^3.0.0",
2021
"json-stringify-nice": "^1.1.4",
2122
"minimatch": "^5.1.0",

workspaces/arborist/test/arborist/reify.js

Lines changed: 78 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2936,7 +2936,20 @@ t.test('installLinks', (t) => {
29362936
})
29372937

29382938
t.only('should preserve exact ranges, missing actual tree', async (t) => {
2939-
const Arborist = require('../../lib/index.js')
2939+
const Pacote = require('pacote')
2940+
const Arborist = t.mock('../../lib/arborist', {
2941+
pacote: {
2942+
...Pacote,
2943+
extract: async (...args) => {
2944+
if (args[0].startsWith('gitssh')) {
2945+
// we just want to test that this url is handled properly
2946+
// but its not a real git url we can clone so return early
2947+
return true
2948+
}
2949+
return Pacote.extract(...args)
2950+
},
2951+
},
2952+
})
29402953
const abbrev = resolve(__dirname,
29412954
'../fixtures/registry-mocks/content/abbrev/-/abbrev-1.1.1.tgz')
29422955
const abbrevTGZ = fs.readFileSync(abbrev)
@@ -2973,6 +2986,40 @@ t.only('should preserve exact ranges, missing actual tree', async (t) => {
29732986
},
29742987
})
29752988

2989+
const gitSshPackument = JSON.stringify({
2990+
_id: 'gitssh',
2991+
_rev: 'lkjadflkjasdf',
2992+
name: 'gitssh',
2993+
'dist-tags': { latest: '1.1.1' },
2994+
versions: {
2995+
'1.1.1': {
2996+
name: 'gitssh',
2997+
version: '1.1.1',
2998+
dist: {
2999+
// this is a url that `new URL()` cant parse
3000+
// https://github.com/npm/cli/issues/5278
3001+
tarball: 'git+ssh://[email protected]:a/b/c.git#lkjadflkjasdf',
3002+
},
3003+
},
3004+
},
3005+
})
3006+
3007+
const notAUrlPackument = JSON.stringify({
3008+
_id: 'notaurl',
3009+
_rev: 'lkjadflkjasdf',
3010+
name: 'notaurl',
3011+
'dist-tags': { latest: '1.1.1' },
3012+
versions: {
3013+
'1.1.1': {
3014+
name: 'notaurl',
3015+
version: '1.1.1',
3016+
dist: {
3017+
tarball: 'hey been trying to break this test',
3018+
},
3019+
},
3020+
},
3021+
})
3022+
29763023
t.only('host should not be replaced replaceRegistryHost=never', async (t) => {
29773024
const testdir = t.testdir({
29783025
project: {
@@ -2981,6 +3028,8 @@ t.only('should preserve exact ranges, missing actual tree', async (t) => {
29813028
version: '1.0.0',
29823029
dependencies: {
29833030
abbrev: '1.1.1',
3031+
gitssh: '1.1.1',
3032+
notaurl: '1.1.1',
29843033
},
29853034
}),
29863035
},
@@ -2994,6 +3043,14 @@ t.only('should preserve exact ranges, missing actual tree', async (t) => {
29943043
.get('/abbrev/-/abbrev-1.1.1.tgz')
29953044
.reply(200, abbrevTGZ)
29963045

3046+
tnock(t, 'https://registry.github.com')
3047+
.get('/gitssh')
3048+
.reply(200, gitSshPackument)
3049+
3050+
tnock(t, 'https://registry.github.com')
3051+
.get('/notaurl')
3052+
.reply(200, notAUrlPackument)
3053+
29973054
const arb = new Arborist({
29983055
path: resolve(testdir, 'project'),
29993056
registry: 'https://registry.github.com',
@@ -3011,6 +3068,8 @@ t.only('should preserve exact ranges, missing actual tree', async (t) => {
30113068
version: '1.0.0',
30123069
dependencies: {
30133070
abbrev: '1.1.1',
3071+
gitssh: '1.1.1',
3072+
notaurl: '1.1.1',
30143073
},
30153074
}),
30163075
},
@@ -3020,10 +3079,18 @@ t.only('should preserve exact ranges, missing actual tree', async (t) => {
30203079
.get('/abbrev')
30213080
.reply(200, abbrevPackument)
30223081

3082+
tnock(t, 'https://registry.github.com')
3083+
.get('/gitssh')
3084+
.reply(200, gitSshPackument)
3085+
30233086
tnock(t, 'https://registry.github.com')
30243087
.get('/abbrev/-/abbrev-1.1.1.tgz')
30253088
.reply(200, abbrevTGZ)
30263089

3090+
tnock(t, 'https://registry.github.com')
3091+
.get('/notaurl')
3092+
.reply(200, notAUrlPackument)
3093+
30273094
const arb = new Arborist({
30283095
path: resolve(testdir, 'project'),
30293096
registry: 'https://registry.github.com',
@@ -3041,6 +3108,8 @@ t.only('should preserve exact ranges, missing actual tree', async (t) => {
30413108
version: '1.0.0',
30423109
dependencies: {
30433110
abbrev: '1.1.1',
3111+
gitssh: '1.1.1',
3112+
notaurl: '1.1.1',
30443113
},
30453114
}),
30463115
},
@@ -3050,10 +3119,18 @@ t.only('should preserve exact ranges, missing actual tree', async (t) => {
30503119
.get('/abbrev')
30513120
.reply(200, abbrevPackument2)
30523121

3122+
tnock(t, 'https://registry.github.com')
3123+
.get('/gitssh')
3124+
.reply(200, gitSshPackument)
3125+
30533126
tnock(t, 'https://registry.github.com')
30543127
.get('/abbrev/-/abbrev-1.1.1.tgz')
30553128
.reply(200, abbrevTGZ)
30563129

3130+
tnock(t, 'https://registry.github.com')
3131+
.get('/notaurl')
3132+
.reply(200, notAUrlPackument)
3133+
30573134
const arb = new Arborist({
30583135
path: resolve(testdir, 'project'),
30593136
registry: 'https://registry.github.com',

0 commit comments

Comments
 (0)