From 3185cc60c6f37fe3ac517f817955d7d37674c598 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 2 Jun 2021 15:08:19 +0200 Subject: [PATCH 1/2] feat(cli-repl): add redactHistory config option MONGOSH-603 Remove the undocumented, untested `--redactInfo` CLI flag and offer a config option with three possible values: - `keep`: Retain the history as-is - `remove`: Remove lines with sensitive commands - `remove-redact`: `remove` + redact sensitive data from non-sensitive commands (e.g. email addresses) --- packages/cli-repl/src/arg-parser.ts | 1 - packages/cli-repl/src/mongosh-repl.spec.ts | 51 +++++++++++++++++++ packages/cli-repl/src/mongosh-repl.ts | 21 ++++---- .../service-provider-core/src/cli-options.ts | 1 - packages/types/src/index.spec.ts | 6 +++ packages/types/src/index.ts | 6 +++ 6 files changed, 75 insertions(+), 11 deletions(-) diff --git a/packages/cli-repl/src/arg-parser.ts b/packages/cli-repl/src/arg-parser.ts index 011944f259..589bdd982c 100644 --- a/packages/cli-repl/src/arg-parser.ts +++ b/packages/cli-repl/src/arg-parser.ts @@ -55,7 +55,6 @@ const OPTIONS = { 'nodb', 'norc', 'quiet', - 'redactInfo', 'retryWrites', 'shell', 'smokeTests', diff --git a/packages/cli-repl/src/mongosh-repl.spec.ts b/packages/cli-repl/src/mongosh-repl.spec.ts index fcbdbdb77f..51cec07a6e 100644 --- a/packages/cli-repl/src/mongosh-repl.spec.ts +++ b/packages/cli-repl/src/mongosh-repl.spec.ts @@ -551,6 +551,57 @@ describe('MongoshNodeRepl', () => { input.write(`${arrowUp}`); await tick(); }); + + context('redaction', () => { + it('removes sensitive commands by default', async() => { + input.write('connect\n'); + await once(mongoshRepl.runtimeState().repl, 'flushHistory'); + input.write('connection\n'); + await once(mongoshRepl.runtimeState().repl, 'flushHistory'); + input.write('db.test.insert({ email: "foo@example.org" })\n'); + await once(mongoshRepl.runtimeState().repl, 'flushHistory'); + + expect(getHistory()).to.deep.equal([ + 'db.test.insert({ email: "foo@example.org" })', + 'connection' // connection is okay, connect is considered sensitive + ]); + }); + + it('keeps sensitive commands when asked to', async() => { + input.write('config.set("redactHistory", "keep");\n'); + await tick(); + input.write('connect\n'); + await once(mongoshRepl.runtimeState().repl, 'flushHistory'); + input.write('connection\n'); + await once(mongoshRepl.runtimeState().repl, 'flushHistory'); + input.write('db.test.insert({ email: "foo@example.org" })\n'); + await once(mongoshRepl.runtimeState().repl, 'flushHistory'); + + expect(getHistory()).to.deep.equal([ + 'db.test.insert({ email: "foo@example.org" })', + 'connection', + 'connect', + 'config.set("redactHistory", "keep");' + ]); + }); + + it('removes other sensitive data when asked to', async() => { + input.write('config.set("redactHistory", "remove-redact");\n'); + await tick(); + input.write('connect\n'); + await once(mongoshRepl.runtimeState().repl, 'flushHistory'); + input.write('connection\n'); + await once(mongoshRepl.runtimeState().repl, 'flushHistory'); + input.write('db.test.insert({ email: "foo@example.org" })\n'); + await once(mongoshRepl.runtimeState().repl, 'flushHistory'); + + expect(getHistory()).to.deep.equal([ + 'db.test.insert({ email: "" })', + 'connection', + 'config.set("redactHistory", "remove-redact");' + ]); + }); + }); }); } }); diff --git a/packages/cli-repl/src/mongosh-repl.ts b/packages/cli-repl/src/mongosh-repl.ts index ba3bec0b35..9d72607fb0 100644 --- a/packages/cli-repl/src/mongosh-repl.ts +++ b/packages/cli-repl/src/mongosh-repl.ts @@ -22,7 +22,6 @@ import { LineByLineInput } from './line-by-line-input'; import { LogEntry, parseAnyLogEntry } from './log-entry'; export type MongoshCliOptions = ShellCliOptions & { - redactInfo?: boolean; quiet?: boolean; }; @@ -110,7 +109,7 @@ class MongoshNodeRepl implements EvaluationListener { started = false; showStackTraces = false; loadNestingLevel = 0; - + redactHistory: 'keep' | 'remove' | 'remove-redact' = 'remove'; constructor(options: MongoshNodeReplOptions) { this.input = options.input; @@ -261,15 +260,16 @@ class MongoshNodeRepl implements EvaluationListener { (repl as Mutable).line = ''; const historyFile = this.ioProvider.getHistoryFilePath(); - const { redactInfo } = this.shellCliOptions; try { await promisify(repl.setupHistory).call(repl, historyFile); // repl.history is an array of previous commands. We need to hijack the // value we just typed, and shift it off the history array if the info is // sensitive. - repl.on('flushHistory', function() { - const history: string[] = (repl as any).history; - changeHistory(history, redactInfo); + repl.on('flushHistory', () => { + if (this.redactHistory !== 'keep') { + const history: string[] = (repl as any).history; + changeHistory(history, this.redactHistory === 'remove-redact'); + } }); // We also want to group multiline history entries and .editor input into // a single entry per evaluation, so that arrow-up functionality @@ -603,13 +603,16 @@ class MongoshNodeRepl implements EvaluationListener { (this.runtimeState().repl as any).historySize = value; } if (key === 'inspectCompact') { - this.inspectCompact = value as number | boolean; + this.inspectCompact = value as CliUserConfig['inspectCompact']; } if (key === 'inspectDepth') { - this.inspectDepth = +value; + this.inspectDepth = value as CliUserConfig['inspectDepth']; } if (key === 'showStackTraces') { - this.showStackTraces = !!value; + this.showStackTraces = value as CliUserConfig['showStackTraces']; + } + if (key === 'redactHistory') { + this.redactHistory = value as CliUserConfig['redactHistory']; } } return result; diff --git a/packages/service-provider-core/src/cli-options.ts b/packages/service-provider-core/src/cli-options.ts index fca0d051e4..acd94b8c2c 100644 --- a/packages/service-provider-core/src/cli-options.ts +++ b/packages/service-provider-core/src/cli-options.ts @@ -30,7 +30,6 @@ export default interface CliOptions { password?: string; port?: string; quiet?: boolean; - redactInfo?: boolean; retryWrites?: boolean; shell?: boolean; tls?: boolean; diff --git a/packages/types/src/index.spec.ts b/packages/types/src/index.spec.ts index 3df457ffc9..6157ce8e91 100644 --- a/packages/types/src/index.spec.ts +++ b/packages/types/src/index.spec.ts @@ -20,6 +20,12 @@ describe('config validation', () => { expect(await validate('showStackTraces', -1)).to.equal('showStackTraces must be a boolean'); expect(await validate('showStackTraces', false)).to.equal(null); expect(await validate('showStackTraces', true)).to.equal(null); + expect(await validate('redactHistory', 'foo')).to.equal("redactHistory must be one of 'keep', 'remove', or 'remove-redact'"); + expect(await validate('redactHistory', -1)).to.equal("redactHistory must be one of 'keep', 'remove', or 'remove-redact'"); + expect(await validate('redactHistory', false)).to.equal("redactHistory must be one of 'keep', 'remove', or 'remove-redact'"); + expect(await validate('redactHistory', 'keep')).to.equal(null); + expect(await validate('redactHistory', 'remove')).to.equal(null); + expect(await validate('redactHistory', 'remove-redact')).to.equal(null); expect(await validate('batchSize', 'foo')).to.equal('batchSize must be a positive integer'); expect(await validate('batchSize', -1)).to.equal('batchSize must be a positive integer'); expect(await validate('batchSize', 0)).to.equal('batchSize must be a positive integer'); diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 13f0603cb6..2b0a106e5e 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -242,6 +242,7 @@ export class CliUserConfig extends ShellUserConfig { inspectDepth = 6; historyLength = 1000; showStackTraces = false; + redactHistory: 'keep' | 'remove' | 'remove-redact' = 'remove'; } export class CliUserConfigValidator extends ShellUserConfigValidator { @@ -267,6 +268,11 @@ export class CliUserConfigValidator extends ShellUserConfigValidator { return `${key} must be a boolean`; } return null; + case 'redactHistory': + if (value !== 'keep' && value !== 'remove' && value !== 'remove-redact') { + return `${key} must be one of 'keep', 'remove', or 'remove-redact'`; + } + return null; default: return super.validate(key as keyof ShellUserConfig, value as any); } From d3d10fd7dce1a1b25b51ad929059d15ff9f744aa Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 3 Jun 2021 12:17:25 +0200 Subject: [PATCH 2/2] fixup: cli-repl config options list --- packages/cli-repl/src/cli-repl.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli-repl/src/cli-repl.spec.ts b/packages/cli-repl/src/cli-repl.spec.ts index 078eb5d41b..f5a8c19bef 100644 --- a/packages/cli-repl/src/cli-repl.spec.ts +++ b/packages/cli-repl/src/cli-repl.spec.ts @@ -181,7 +181,7 @@ describe('CliRepl', () => { it('returns the list of available config options when asked to', () => { expect(cliRepl.listConfigOptions()).to.deep.equal([ - 'batchSize', 'enableTelemetry', 'inspectCompact', 'inspectDepth', 'historyLength', 'showStackTraces' + 'batchSize', 'enableTelemetry', 'inspectCompact', 'inspectDepth', 'historyLength', 'showStackTraces', 'redactHistory' ]); });