From 724e1022bd8e2b2cf2af96214b152aaba3b092ea Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Mon, 13 May 2024 15:41:06 -0700 Subject: [PATCH 1/6] fix(config): protect proxy if it contains basic auth Fixes #3867 --- lib/commands/config.js | 28 ++++++++++++++++++---------- test/lib/commands/config.js | 14 ++++++++++++++ 2 files changed, 32 insertions(+), 10 deletions(-) diff --git a/lib/commands/config.js b/lib/commands/config.js index 2f1f11e762a73..bfafe3b0d1722 100644 --- a/lib/commands/config.js +++ b/lib/commands/config.js @@ -7,6 +7,7 @@ const pkgJson = require('@npmcli/package-json') const { defaults, definitions } = require('@npmcli/config/lib/definitions') const { log, output } = require('proc-log') const BaseCommand = require('../base-cmd.js') +const { redact } = require('@npmcli/redact') // These are the configs that we can nerf-dart. Not all of them currently even // *have* config definitions so we have to explicitly validate them here. @@ -53,7 +54,7 @@ const keyValues = args => { return kv } -const publicVar = k => { +const publicVar = (k, v) => { // _password if (k.startsWith('_')) { return false @@ -73,6 +74,10 @@ const publicVar = k => { } } } + // Hide proxy url if it contains basic auth + if (k === 'proxy') { + return redact(v) === v + } return true } @@ -206,7 +211,8 @@ class Config extends BaseCommand { const out = [] for (const key of keys) { - if (!publicVar(key)) { + const val = this.npm.config.get(key) + if (!publicVar(key, val)) { throw new Error(`The ${key} option is protected, and can not be retrieved in this way`) } @@ -338,14 +344,14 @@ ${defData} continue } - const keys = Object.keys(data).sort(localeCompare) + const keys = Object.entries(data).sort(([a], [b]) => localeCompare(a, b)) if (!keys.length) { continue } msg.push(`; "${where}" config from ${source}`, '') - for (const k of keys) { - const v = publicVar(k) ? JSON.stringify(data[k]) : '(protected)' + for (const [k, value] of keys) { + const v = publicVar(k, value) ? JSON.stringify(value) : '(protected)' const src = this.npm.config.find(k) const overridden = src !== where msg.push((overridden ? '; ' : '') + @@ -374,9 +380,10 @@ ${defData} const pkgPath = resolve(this.npm.prefix, 'package.json') msg.push(`; "publishConfig" from ${pkgPath}`) msg.push('; This set of config values will be used at publish-time.', '') - const pkgKeys = Object.keys(content.publishConfig).sort(localeCompare) - for (const k of pkgKeys) { - const v = publicVar(k) ? JSON.stringify(content.publishConfig[k]) : '(protected)' + const entries = Object.entries(content.publishConfig) + .sort(([a], [b]) => localeCompare(a, b)) + for (const [k, value] of entries) { + const v = publicVar(k, value) ? JSON.stringify(value) : '(protected)' msg.push(`${k} = ${v}`) } msg.push('') @@ -389,11 +396,12 @@ ${defData} async listJson () { const publicConf = {} for (const key in this.npm.config.list[0]) { - if (!publicVar(key)) { + const value = this.npm.config.get(key) + if (!publicVar(key, value)) { continue } - publicConf[key] = this.npm.config.get(key) + publicConf[key] = value } output.standard(JSON.stringify(publicConf, null, 2)) } diff --git a/test/lib/commands/config.js b/test/lib/commands/config.js index 563bb97dc1612..a49c684dfca76 100644 --- a/test/lib/commands/config.js +++ b/test/lib/commands/config.js @@ -503,6 +503,20 @@ t.test('config get private key', async t => { /_password option is protected/, 'rejects with protected string' ) + + await npm.exec('config', ['set', 'proxy', 'https://proxy.npmjs.org']) + + await t.resolves( + npm.exec('config', ['get', 'proxy']), + 'https://proxy.npmjs.org' + ) + + await npm.exec('config', ['set', 'proxy', 'https://username:password@proxy.npmjs.org']) + + await t.rejects( + npm.exec('config', ['get', 'proxy']), + /proxy option is protected/ + ) }) t.test('config edit', async t => { From 5656987561590d7fac4e0a4e885dbfa71200728e Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Tue, 14 May 2024 10:58:57 -0700 Subject: [PATCH 2/6] fixup! fix(config): protect proxy if it contains basic auth --- lib/commands/config.js | 37 +++++++++++++++++++++++++++---------- test/lib/commands/config.js | 25 +++++++++++++++---------- 2 files changed, 42 insertions(+), 20 deletions(-) diff --git a/lib/commands/config.js b/lib/commands/config.js index bfafe3b0d1722..8bbeef36d4324 100644 --- a/lib/commands/config.js +++ b/lib/commands/config.js @@ -39,6 +39,13 @@ const protected = [ 'username', ] +// These values might contain url basic auth so we only redact them +// if necessary +const redacted = [ + 'proxy', + 'registry', +] + // take an array of `[key, value, k2=v2, k3, v3, ...]` and turn into // { key: value, k2: v2, k3: v3 } const keyValues = args => { @@ -74,13 +81,25 @@ const publicVar = (k, v) => { } } } - // Hide proxy url if it contains basic auth - if (k === 'proxy') { + // Redacted fields are public unless they contain redacted info + if (redacted.includes(k)) { return redact(v) === v } return true } +const displayVar = (k, v) => { + let value + if (publicVar(k, v)) { + value = JSON.stringify(v) + } else if (redacted.includes(k)) { + value = JSON.stringify(redact(v)) + } else { + value = '(protected)' + } + return `${k} = ${value}` +} + class Config extends BaseCommand { static description = 'Manage the npm configuration files' static name = 'config' @@ -217,7 +236,7 @@ class Config extends BaseCommand { } const pref = keys.length > 1 ? `${key}=` : '' - out.push(pref + this.npm.config.get(key)) + out.push(pref + val) } output.standard(out.join('\n')) } @@ -350,12 +369,11 @@ ${defData} } msg.push(`; "${where}" config from ${source}`, '') - for (const [k, value] of keys) { - const v = publicVar(k, value) ? JSON.stringify(value) : '(protected)' + for (const [k, v] of keys) { + const display = displayVar(k, v) const src = this.npm.config.find(k) - const overridden = src !== where - msg.push((overridden ? '; ' : '') + - `${k} = ${v}${overridden ? ` ; overridden by ${src}` : ''}`) + msg.push(src === where ? display : `; ${display} ; overridden by ${src}`) + msg.push() } msg.push('') } @@ -383,8 +401,7 @@ ${defData} const entries = Object.entries(content.publishConfig) .sort(([a], [b]) => localeCompare(a, b)) for (const [k, value] of entries) { - const v = publicVar(k, value) ? JSON.stringify(value) : '(protected)' - msg.push(`${k} = ${v}`) + msg.push(displayVar(k, value)) } msg.push('') } diff --git a/test/lib/commands/config.js b/test/lib/commands/config.js index a49c684dfca76..1a3644a07623b 100644 --- a/test/lib/commands/config.js +++ b/test/lib/commands/config.js @@ -503,20 +503,25 @@ t.test('config get private key', async t => { /_password option is protected/, 'rejects with protected string' ) +}) - await npm.exec('config', ['set', 'proxy', 'https://proxy.npmjs.org']) +t.test('config redacted values', async t => { + const { npm, joinedOutput, clearOutput } = await loadMockNpm(t) - await t.resolves( - npm.exec('config', ['get', 'proxy']), - 'https://proxy.npmjs.org' - ) + await npm.exec('config', ['set', 'proxy', 'https://proxy.npmjs.org/']) + await npm.exec('config', ['get', 'proxy']) - await npm.exec('config', ['set', 'proxy', 'https://username:password@proxy.npmjs.org']) + t.equal(joinedOutput(), 'https://proxy.npmjs.org/') + clearOutput() - await t.rejects( - npm.exec('config', ['get', 'proxy']), - /proxy option is protected/ - ) + await npm.exec('config', ['set', 'proxy', 'https://u:password@proxy.npmjs.org/']) + + await t.rejects(npm.exec('config', ['get', 'proxy']), /proxy option is protected/) + + await npm.exec('config', ['ls']) + + t.match(joinedOutput(), 'proxy = "https://u:***@proxy.npmjs.org/"') + clearOutput() }) t.test('config edit', async t => { From 305241e7838999054dacc11653863b9b8eddabea Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Tue, 14 May 2024 12:46:57 -0700 Subject: [PATCH 3/6] fixup! fix(config): protect proxy if it contains basic auth --- lib/commands/config.js | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/lib/commands/config.js b/lib/commands/config.js index 8bbeef36d4324..bde818450d381 100644 --- a/lib/commands/config.js +++ b/lib/commands/config.js @@ -89,14 +89,9 @@ const publicVar = (k, v) => { } const displayVar = (k, v) => { - let value - if (publicVar(k, v)) { - value = JSON.stringify(v) - } else if (redacted.includes(k)) { - value = JSON.stringify(redact(v)) - } else { - value = '(protected)' - } + const value = publicVar(k, v) || redacted.includes(k) + ? JSON.stringify(redact(v)) + : '(protected)' return `${k} = ${value}` } From 709cbeabe1da79d5d45b1d94def3077e859c9bbc Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Tue, 14 May 2024 13:07:01 -0700 Subject: [PATCH 4/6] fixup! fix(config): protect proxy if it contains basic auth --- lib/commands/config.js | 40 +++++++++++++++------------------------- 1 file changed, 15 insertions(+), 25 deletions(-) diff --git a/lib/commands/config.js b/lib/commands/config.js index bde818450d381..04815199de64f 100644 --- a/lib/commands/config.js +++ b/lib/commands/config.js @@ -39,13 +39,6 @@ const protected = [ 'username', ] -// These values might contain url basic auth so we only redact them -// if necessary -const redacted = [ - 'proxy', - 'registry', -] - // take an array of `[key, value, k2=v2, k3, v3, ...]` and turn into // { key: value, k2: v2, k3: v3 } const keyValues = args => { @@ -61,39 +54,36 @@ const keyValues = args => { return kv } -const publicVar = (k, v) => { +const isProtected = (k) => { // _password if (k.startsWith('_')) { - return false + return true } if (protected.includes(k)) { - return false + return true } // //localhost:8080/:_password if (k.startsWith('//')) { if (k.includes(':_')) { - return false + return true } // //registry:_authToken or //registry:authToken for (const p of protected) { if (k.endsWith(`:${p}`) || k.endsWith(`:_${p}`)) { - return false + return true } } } - // Redacted fields are public unless they contain redacted info - if (redacted.includes(k)) { - return redact(v) === v - } - return true + return false } -const displayVar = (k, v) => { - const value = publicVar(k, v) || redacted.includes(k) - ? JSON.stringify(redact(v)) - : '(protected)' - return `${k} = ${value}` -} +// Redacted fields are public unless they contain redacted info +const isRedacted = (v) => redact(v) !== v + +const isPrivate = (k, v) => isProtected(k) || isRedacted(v) + +const displayVar = (k, v) => + `${k} = ${isProtected(k, v) ? '(protected)' : JSON.stringify(redact(v))}` class Config extends BaseCommand { static description = 'Manage the npm configuration files' @@ -226,7 +216,7 @@ class Config extends BaseCommand { const out = [] for (const key of keys) { const val = this.npm.config.get(key) - if (!publicVar(key, val)) { + if (isPrivate(key, val)) { throw new Error(`The ${key} option is protected, and can not be retrieved in this way`) } @@ -409,7 +399,7 @@ ${defData} const publicConf = {} for (const key in this.npm.config.list[0]) { const value = this.npm.config.get(key) - if (!publicVar(key, value)) { + if (isPrivate(key, value)) { continue } From 74e34e53ef5e1a90a6227f0128f43e063c32ebaf Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Tue, 14 May 2024 13:08:33 -0700 Subject: [PATCH 5/6] fixup! fix(config): protect proxy if it contains basic auth --- lib/commands/config.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/commands/config.js b/lib/commands/config.js index 04815199de64f..1475a461d2492 100644 --- a/lib/commands/config.js +++ b/lib/commands/config.js @@ -77,10 +77,8 @@ const isProtected = (k) => { return false } -// Redacted fields are public unless they contain redacted info -const isRedacted = (v) => redact(v) !== v - -const isPrivate = (k, v) => isProtected(k) || isRedacted(v) +// Private fields are either protected or they can redacted info +const isPrivate = (k, v) => isProtected(k) || redact(v) !== v const displayVar = (k, v) => `${k} = ${isProtected(k, v) ? '(protected)' : JSON.stringify(redact(v))}` From 6c362ccabbcd3a698873e4569d3430acdbd27f7b Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Tue, 14 May 2024 13:32:04 -0700 Subject: [PATCH 6/6] Update lib/commands/config.js --- lib/commands/config.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/commands/config.js b/lib/commands/config.js index 1475a461d2492..e8da195204c00 100644 --- a/lib/commands/config.js +++ b/lib/commands/config.js @@ -346,13 +346,13 @@ ${defData} continue } - const keys = Object.entries(data).sort(([a], [b]) => localeCompare(a, b)) - if (!keys.length) { + const entries = Object.entries(data).sort(([a], [b]) => localeCompare(a, b)) + if (!entries.length) { continue } msg.push(`; "${where}" config from ${source}`, '') - for (const [k, v] of keys) { + for (const [k, v] of entries) { const display = displayVar(k, v) const src = this.npm.config.find(k) msg.push(src === where ? display : `; ${display} ; overridden by ${src}`)