Skip to content

Commit

Permalink
fix(logging): sanitize logged argv
Browse files Browse the repository at this point in the history
Wraps logged process.argv in `replaceInfo`

Removes logged process.argv from EJSONPARSE warning for top level
package.json merge conflicts.  It is currently not even working (er.file
is not being populated by the parsing library right now), and
process.argv contains fullly resolved paths which isn't very nice
looking.  The user knows what they typed, it's enough to tell them to
retry.

PR-URL: #3658
Credit: @wraithgar
Close: #3658
Reviewed-by: @nlf
  • Loading branch information
wraithgar committed Aug 17, 2021
1 parent 7a58264 commit 487731c
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 7 deletions.
3 changes: 2 additions & 1 deletion lib/cli.js
Expand Up @@ -27,7 +27,8 @@ module.exports = async (process) => {
if (process.argv[1][process.argv[1].length - 1] === 'g')
process.argv.splice(1, 1, 'npm', '-g')

log.verbose('cli', process.argv)
const replaceInfo = require('../lib/utils/replace-info.js')
log.verbose('cli', replaceInfo(process.argv))

log.info('using', 'npm@%s', npm.version)
log.info('using', 'node@%s', process.version)
Expand Down
4 changes: 1 addition & 3 deletions lib/utils/error-message.js
Expand Up @@ -109,9 +109,7 @@ module.exports = (er, npm) => {
[
'Merge conflict detected in your package.json.',
'',
'Please resolve the package.json conflict and retry the command:',
'',
`$ ${process.argv.join(' ')}`,
'Please resolve the package.json conflict and retry.',
].join('\n'),
])
break
Expand Down
4 changes: 1 addition & 3 deletions tap-snapshots/test/lib/utils/error-message.js.test.cjs
Expand Up @@ -1537,9 +1537,7 @@ Object {
String(
Merge conflict detected in your package.json.
Please resolve the package.json conflict and retry the command:
$ arg v
Please resolve the package.json conflict and retry.
),
],
],
Expand Down
26 changes: 26 additions & 0 deletions test/lib/cli.js
Expand Up @@ -104,6 +104,32 @@ t.test('calling with --versions calls npm version with no args', async t => {
t.strictSame(exitHandlerCalled, [])
})

t.test('logged argv is sanitized', async t => {
const proc = processMock({
argv: ['node', 'npm', 'testcommand', 'https://username:password@npmjs.org/test_url_with_a_password'],
})
const { npm } = mockNpm(t)
const cli = cliMock(npm)

npm.commands.testcommand = (args, cb) => {
cb()
}

await cli(proc)
t.equal(proc.title, 'npm')
t.strictSame(logs, [
'pause',
['verbose', 'cli', [
'node',
'npm',
'testcommand',
'https://username:***@npmjs.org/test_url_with_a_password',
]],
['info', 'using', 'npm@%s', npm.version],
['info', 'using', 'node@%s', process.version],
])
})

t.test('print usage if no params provided', async t => {
const proc = processMock({
argv: ['node', 'npm'],
Expand Down

0 comments on commit 487731c

Please sign in to comment.