Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: never use cached npm ping request #7789

Merged
merged 1 commit into from
Sep 30, 2024
Merged

fix: never use cached npm ping request #7789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/utils/ping.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@
// used by the ping and doctor commands
const fetch = require('npm-registry-fetch')
module.exports = async (flatOptions) => {
const res = await fetch('/-/ping?write=true', flatOptions)
const res = await fetch('/-/ping', { ...flatOptions, cache: false })
return res.json().catch(() => ({}))
}
2 changes: 1 addition & 1 deletion mock-registry/lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ class MockRegistry {
}

ping ({ body = {}, responseCode = 200 } = {}) {
this.nock = this.nock.get(this.fullPath('/-/ping?write=true')).reply(responseCode, body)
this.nock = this.nock.get(this.fullPath('/-/ping')).reply(responseCode, body)
}

// full unpublish of an entire package
Expand Down
8 changes: 4 additions & 4 deletions tap-snapshots/test/lib/commands/doctor.js.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1166,7 +1166,7 @@ Object {
exports[`test/lib/commands/doctor.js TAP ping 404 > ping 404 1`] = `
Connecting to the registry
Not ok
404 404 Not Found - GET https://registry.npmjs.org/-/ping?write=true
404 404 Not Found - GET https://registry.npmjs.org/-/ping
Checking npm version
Ok
current: v1.0.0, latest: v1.0.0
Expand Down Expand Up @@ -1226,7 +1226,7 @@ Object {
exports[`test/lib/commands/doctor.js TAP ping 404 in color > ping 404 in color 1`] = `
Connecting to the registry
Not ok
[36m404 404 Not Found - GET https://registry.npmjs.org/-/ping?write=true[39m
[36m404 404 Not Found - GET https://registry.npmjs.org/-/ping[39m
Checking npm version
Ok
current: v1.0.0, latest: v1.0.0
Expand Down Expand Up @@ -1286,7 +1286,7 @@ Object {
exports[`test/lib/commands/doctor.js TAP ping exception with code > ping failure 1`] = `
Connecting to the registry
Not ok
request to https://registry.npmjs.org/-/ping?write=true failed, reason: Test Error
request to https://registry.npmjs.org/-/ping failed, reason: Test Error
Checking npm version
Ok
current: v1.0.0, latest: v1.0.0
Expand Down Expand Up @@ -1346,7 +1346,7 @@ Object {
exports[`test/lib/commands/doctor.js TAP ping exception without code > ping failure 1`] = `
Connecting to the registry
Not ok
request to https://registry.npmjs.org/-/ping?write=true failed, reason: Test Error
request to https://registry.npmjs.org/-/ping failed, reason: Test Error
Checking npm version
Ok
current: v1.0.0, latest: v1.0.0
Expand Down
48 changes: 24 additions & 24 deletions test/lib/commands/doctor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ t.test('all clear', async t => {
...dirs,
})
tnock(t, npm.config.get('registry'))
.get('/-/ping?write=true').reply(200, '{}')
.get('/-/ping').reply(200, '{}')
.get('/npm').reply(200, npmManifest(npm.version))
tnock(t, 'https://nodejs.org')
.get('/dist/index.json').reply(200, nodeVersions)
Expand All @@ -108,7 +108,7 @@ t.test('all clear in color', async t => {
},
})
tnock(t, npm.config.get('registry'))
.get('/-/ping?write=true').reply(200, '{}')
.get('/-/ping').reply(200, '{}')
.get('/npm').reply(200, npmManifest(npm.version))
tnock(t, 'https://nodejs.org')
.get('/dist/index.json').reply(200, nodeVersions)
Expand All @@ -127,7 +127,7 @@ t.test('silent success', async t => {
...dirs,
})
tnock(t, npm.config.get('registry'))
.get('/-/ping?write=true').reply(200, '{}')
.get('/-/ping').reply(200, '{}')
.get('/npm').reply(200, npmManifest(npm.version))
tnock(t, 'https://nodejs.org')
.get('/dist/index.json').reply(200, nodeVersions)
Expand All @@ -146,7 +146,7 @@ t.test('silent errors', async t => {
...dirs,
})
tnock(t, npm.config.get('registry'))
.get('/-/ping?write=true').reply(404, '{}')
.get('/-/ping').reply(404, '{}')
await t.rejects(npm.exec('doctor', ['ping']), {
message: /Check logs/,
})
Expand All @@ -161,7 +161,7 @@ t.test('ping 404', async t => {
...dirs,
})
tnock(t, npm.config.get('registry'))
.get('/-/ping?write=true').reply(404, '{}')
.get('/-/ping').reply(404, '{}')
.get('/npm').reply(200, npmManifest(npm.version))
tnock(t, 'https://nodejs.org')
.get('/dist/index.json').reply(200, nodeVersions)
Expand All @@ -182,7 +182,7 @@ t.test('ping 404 in color', async t => {
},
})
tnock(t, npm.config.get('registry'))
.get('/-/ping?write=true').reply(404, '{}')
.get('/-/ping').reply(404, '{}')
.get('/npm').reply(200, npmManifest(npm.version))
tnock(t, 'https://nodejs.org')
.get('/dist/index.json').reply(200, nodeVersions)
Expand All @@ -198,7 +198,7 @@ t.test('ping exception with code', async t => {
...dirs,
})
tnock(t, npm.config.get('registry'))
.get('/-/ping?write=true').replyWithError({ message: 'Test Error', code: 'TEST' })
.get('/-/ping').replyWithError({ message: 'Test Error', code: 'TEST' })
.get('/npm').reply(200, npmManifest(npm.version))
tnock(t, 'https://nodejs.org')
.get('/dist/index.json').reply(200, nodeVersions)
Expand All @@ -214,7 +214,7 @@ t.test('ping exception without code', async t => {
...dirs,
})
tnock(t, npm.config.get('registry'))
.get('/-/ping?write=true').replyWithError({ message: 'Test Error', code: false })
.get('/-/ping').replyWithError({ message: 'Test Error', code: false })
.get('/npm').reply(200, npmManifest(npm.version))
tnock(t, 'https://nodejs.org')
.get('/dist/index.json').reply(200, nodeVersions)
Expand All @@ -230,7 +230,7 @@ t.test('npm out of date', async t => {
...dirs,
})
tnock(t, npm.config.get('registry'))
.get('/-/ping?write=true').reply(200, '{}')
.get('/-/ping').reply(200, '{}')
.get('/npm').reply(200, npmManifest('2.0.0'))
tnock(t, 'https://nodejs.org')
.get('/dist/index.json').reply(200, nodeVersions)
Expand All @@ -255,7 +255,7 @@ t.test('node out of date - lts', async t => {
...dirs,
})
tnock(t, npm.config.get('registry'))
.get('/-/ping?write=true').reply(200, '{}')
.get('/-/ping').reply(200, '{}')
.get('/npm').reply(200, npmManifest(npm.version))
tnock(t, 'https://nodejs.org')
.get('/dist/index.json').reply(200, nodeVersions)
Expand All @@ -280,7 +280,7 @@ t.test('node out of date - current', async t => {
...dirs,
})
tnock(t, npm.config.get('registry'))
.get('/-/ping?write=true').reply(200, '{}')
.get('/-/ping').reply(200, '{}')
.get('/npm').reply(200, npmManifest(npm.version))
tnock(t, 'https://nodejs.org')
.get('/dist/index.json').reply(200, nodeVersions)
Expand All @@ -297,7 +297,7 @@ t.test('non-default registry', async t => {
...dirs,
})
tnock(t, npm.config.get('registry'))
.get('/-/ping?write=true').reply(200, '{}')
.get('/-/ping').reply(200, '{}')
.get('/npm').reply(200, npmManifest(npm.version))
tnock(t, 'https://nodejs.org')
.get('/dist/index.json').reply(200, nodeVersions)
Expand All @@ -318,7 +318,7 @@ t.test('missing git', async t => {
...dirs,
})
tnock(t, npm.config.get('registry'))
.get('/-/ping?write=true').reply(200, '{}')
.get('/-/ping').reply(200, '{}')
.get('/npm').reply(200, npmManifest(npm.version))
tnock(t, 'https://nodejs.org')
.get('/dist/index.json').reply(200, nodeVersions)
Expand All @@ -344,7 +344,7 @@ t.test('windows skips permissions checks', async t => {
globalPrefixDir: {},
})
tnock(t, npm.config.get('registry'))
.get('/-/ping?write=true').reply(200, '{}')
.get('/-/ping').reply(200, '{}')
.get('/npm').reply(200, npmManifest(npm.version))
tnock(t, 'https://nodejs.org')
.get('/dist/index.json').reply(200, nodeVersions)
Expand All @@ -361,7 +361,7 @@ t.test('missing global directories', async t => {
globalPrefixDir: {},
})
tnock(t, npm.config.get('registry'))
.get('/-/ping?write=true').reply(200, '{}')
.get('/-/ping').reply(200, '{}')
.get('/npm').reply(200, npmManifest(npm.version))
tnock(t, 'https://nodejs.org')
.get('/dist/index.json').reply(200, nodeVersions)
Expand All @@ -377,7 +377,7 @@ t.test('missing local node_modules', async t => {
globalPrefixDir: dirs.globalPrefixDir,
})
tnock(t, npm.config.get('registry'))
.get('/-/ping?write=true').reply(200, '{}')
.get('/-/ping').reply(200, '{}')
.get('/npm').reply(200, npmManifest(npm.version))
tnock(t, 'https://nodejs.org')
.get('/dist/index.json').reply(200, nodeVersions)
Expand Down Expand Up @@ -406,7 +406,7 @@ t.test('incorrect owner', async t => {
...dirs,
})
tnock(t, npm.config.get('registry'))
.get('/-/ping?write=true').reply(200, '{}')
.get('/-/ping').reply(200, '{}')
.get('/npm').reply(200, npmManifest(npm.version))
tnock(t, 'https://nodejs.org')
.get('/dist/index.json').reply(200, nodeVersions)
Expand All @@ -430,7 +430,7 @@ t.test('incorrect permissions', async t => {
...dirs,
})
tnock(t, npm.config.get('registry'))
.get('/-/ping?write=true').reply(200, '{}')
.get('/-/ping').reply(200, '{}')
.get('/npm').reply(200, npmManifest(npm.version))
tnock(t, 'https://nodejs.org')
.get('/dist/index.json').reply(200, nodeVersions)
Expand Down Expand Up @@ -458,7 +458,7 @@ t.test('error reading directory', async t => {
...dirs,
})
tnock(t, npm.config.get('registry'))
.get('/-/ping?write=true').reply(200, '{}')
.get('/-/ping').reply(200, '{}')
.get('/npm').reply(200, npmManifest(npm.version))
tnock(t, 'https://nodejs.org')
.get('/dist/index.json').reply(200, nodeVersions)
Expand All @@ -481,7 +481,7 @@ t.test('cacache badContent', async t => {
...dirs,
})
tnock(t, npm.config.get('registry'))
.get('/-/ping?write=true').reply(200, '{}')
.get('/-/ping').reply(200, '{}')
.get('/npm').reply(200, npmManifest(npm.version))
tnock(t, 'https://nodejs.org')
.get('/dist/index.json').reply(200, nodeVersions)
Expand All @@ -504,7 +504,7 @@ t.test('cacache reclaimedCount', async t => {
...dirs,
})
tnock(t, npm.config.get('registry'))
.get('/-/ping?write=true').reply(200, '{}')
.get('/-/ping').reply(200, '{}')
.get('/npm').reply(200, npmManifest(npm.version))
tnock(t, 'https://nodejs.org')
.get('/dist/index.json').reply(200, nodeVersions)
Expand All @@ -527,7 +527,7 @@ t.test('cacache missingContent', async t => {
...dirs,
})
tnock(t, npm.config.get('registry'))
.get('/-/ping?write=true').reply(200, '{}')
.get('/-/ping').reply(200, '{}')
.get('/npm').reply(200, npmManifest(npm.version))
tnock(t, 'https://nodejs.org')
.get('/dist/index.json').reply(200, nodeVersions)
Expand Down Expand Up @@ -558,7 +558,7 @@ t.test('discrete checks', t => {
...dirs,
})
tnock(t, npm.config.get('registry'))
.get('/-/ping?write=true').reply(200, '{}')
.get('/-/ping').reply(200, '{}')
await npm.exec('doctor', ['ping'])
t.matchSnapshot(joinedOutput(), 'output')
t.matchSnapshot({ info: logs.info, warn: logs.warn, error: logs.error }, 'logs')
Expand Down Expand Up @@ -586,7 +586,7 @@ t.test('discrete checks', t => {
...dirs,
})
tnock(t, npm.config.get('registry'))
.get('/-/ping?write=true').reply(200, '{}')
.get('/-/ping').reply(200, '{}')
await npm.exec('doctor', ['registry'])
t.matchSnapshot(joinedOutput(), 'output')
t.matchSnapshot({ info: logs.info, warn: logs.warn, error: logs.error }, 'logs')
Expand Down
18 changes: 18 additions & 0 deletions test/lib/commands/ping.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
const t = require('tap')
const { load: loadMockNpm } = require('../../fixtures/mock-npm.js')
const MockRegistry = require('@npmcli/mock-registry')
const cacache = require('cacache')
const path = require('node:path')

t.test('no details', async t => {
const { npm, logs, joinedOutput } = await loadMockNpm(t)
Expand Down Expand Up @@ -74,3 +76,19 @@ t.test('invalid json', async t => {
details: {},
})
})
t.test('fail when registry is unreachable even if request is cached', async t => {
const { npm } = await loadMockNpm(t, {
config: { registry: 'https://ur.npmlocal.npmtest/' },
cacheDir: { _cacache: {} },
})
const url = `${npm.config.get('registry')}-/ping`
const cache = path.join(npm.cache, '_cacache')
await cacache.put(cache,
`make-fetch-happen:request-cache:${url}`,
'{}', { metadata: { url } }
)
t.rejects(npm.exec('ping', []), {
code: 'ENOTFOUND',
},
'throws ENOTFOUND error')
})
Loading