Repository: oclif/core
Pull request #665 (2023-03-20 19:35:46)

feat: add param noSensitiveData to Plugin.load
@W-12513575@

peternhale: @mshanemc With this change to core, oclif manifest will be changed to pass true to plugin.load(). Any custom default or defaultHelp functions will then be able to omit sensitive info from return values.
peternhale: > So we pass it around like a hot potato to make sure the various "consumers" get the correct value, but it's up to the user-defined `default` function to decide what to do.
> 
> I don't love the name...would I set it to true if I'm including sensitive information that I want omitted (and therefore `containsSensitiveInformation`:true would be a better name.
> 
> Or...does `noSensitiveData : true` mean "this contains no sensitive data" and so things should proceed as normal?

Backwards compatibility. Plugin.load is currently a no parameter function. I could change the name to `suppressSensitiveData` with default of false.

Patch:
diff --git a/src/config/config.ts b/src/config/config.ts
index 6f3aafb..d4a6d8b 100644
--- a/src/config/config.ts
+++ b/src/config/config.ts
@@ -813,11 +813,11 @@ export class Config implements IConfig {
 }
 
 // when no manifest exists, the default is calculated.  This may throw, so we need to catch it
-const defaultFlagToCached = async (flag: CompletableOptionFlag<any>) => {
+const defaultFlagToCached = async (flag: CompletableOptionFlag<any>, isWritingManifest = false) => {
   // Prefer the helpDefaultValue function (returns a friendly string for complex types)
   if (typeof flag.defaultHelp === 'function') {
     try {
-      return await flag.defaultHelp()
+      return await flag.defaultHelp({options: flag, flags: {}}, isWritingManifest)
     } catch {
       return
     }
@@ -826,18 +826,18 @@ const defaultFlagToCached = async (flag: CompletableOptionFlag<any>) => {
   // if not specified, try the default function
   if (typeof flag.default === 'function') {
     try {
-      return await flag.default({options: {}, flags: {}})
+      return await flag.default({options: {}, flags: {}}, isWritingManifest)
     } catch {}
   } else {
     return flag.default
   }
 }
 
-const defaultArgToCached = async (arg: Arg<any>): Promise<any> => {
+const defaultArgToCached = async (arg: Arg<any>, isWritingManifest = false): Promise<any> => {
   // Prefer the helpDefaultValue function (returns a friendly string for complex types)
   if (typeof arg.defaultHelp === 'function') {
     try {
-      return await arg.defaultHelp()
+      return await arg.defaultHelp({options: arg, flags: {}}, isWritingManifest)
     } catch {
       return
     }
@@ -846,14 +846,14 @@ const defaultArgToCached = async (arg: Arg<any>): Promise<any> => {
   // if not specified, try the default function
   if (typeof arg.default === 'function') {
     try {
-      return await arg.default({options: {}, flags: {}})
+      return await arg.default({options: arg, flags: {}}, isWritingManifest)
     } catch {}
   } else {
     return arg.default
   }
 }
 
-export async function toCached(c: Command.Class, plugin?: IPlugin): Promise<Command.Cached> {
+export async function toCached(c: Command.Class, plugin?: IPlugin | undefined, isWritingManifest?: boolean): Promise<Command.Cached> {
   const flags = {} as {[k: string]: Command.Flag.Cached}
 
   for (const [name, flag] of Object.entries(c.flags || {})) {
@@ -894,7 +894,7 @@ export async function toCached(c: Command.Class, plugin?: IPlugin): Promise<Comm
         dependsOn: flag.dependsOn,
         relationships: flag.relationships,
         exclusive: flag.exclusive,
-        default: await defaultFlagToCached(flag),
+        default: await defaultFlagToCached(flag, isWritingManifest),
         deprecated: flag.deprecated,
         deprecateAliases: c.deprecateAliases,
         aliases: flag.aliases,
@@ -914,7 +914,7 @@ export async function toCached(c: Command.Class, plugin?: IPlugin): Promise<Comm
       description: arg.description,
       required: arg.required,
       options: arg.options,
-      default: await defaultArgToCached(arg),
+      default: await defaultArgToCached(arg, isWritingManifest),
       hidden: arg.hidden,
     }
   }
diff --git a/src/config/plugin.ts b/src/config/plugin.ts
index 7ca0975..0320c50 100644
--- a/src/config/plugin.ts
+++ b/src/config/plugin.ts
@@ -136,7 +136,13 @@ export class Plugin implements IPlugin {
 
   constructor(public options: PluginOptions) {}
 
-  public async load(): Promise<void> {
+  /**
+   * Loads a plugin
+   * @param isWritingManifest - if true, exclude selected data from manifest
+   * default is false to maintain backwards compatibility
+   * @returns Promise<void>
+   */
+  public async load(isWritingManifest?: boolean): Promise<void> {
     this.type = this.options.type || 'core'
     this.tag = this.options.tag
     const root = await findRoot(this.options.name, this.options.root)
@@ -160,7 +166,7 @@ export class Plugin implements IPlugin {
 
     this.hooks = mapValues(this.pjson.oclif.hooks || {}, i => Array.isArray(i) ? i : [i])
 
-    this.manifest = await this._manifest(Boolean(this.options.ignoreManifest), Boolean(this.options.errorOnManifestCreate))
+    this.manifest = await this._manifest(Boolean(this.options.ignoreManifest), Boolean(this.options.errorOnManifestCreate), isWritingManifest)
     this.commands = Object
     .entries(this.manifest.commands)
     .map(([id, c]) => ({
@@ -244,7 +250,7 @@ export class Plugin implements IPlugin {
     return cmd
   }
 
-  protected async _manifest(ignoreManifest: boolean, errorOnManifestCreate = false): Promise<Manifest> {
+  protected async _manifest(ignoreManifest: boolean, errorOnManifestCreate = false, isWritingManifest = false): Promise<Manifest> {
     const readManifest = async (dotfile = false): Promise<Manifest | undefined> => {
       try {
         const p = path.join(this.root, `${dotfile ? '.' : ''}oclif.manifest.json`)
@@ -279,7 +285,7 @@ export class Plugin implements IPlugin {
       version: this.version,
       commands: (await Promise.all(this.commandIDs.map(async id => {
         try {
-          return [id, await toCached(await this.findCommand(id, {must: true}), this)]
+          return [id, await toCached(await this.findCommand(id, {must: true}), this, isWritingManifest)]
         } catch (error: any) {
           const scope = 'toCached'
           if (Boolean(errorOnManifestCreate) === false) this.warn(error, scope)
diff --git a/src/help/index.ts b/src/help/index.ts
index a768286..d5572dc 100644
--- a/src/help/index.ts
+++ b/src/help/index.ts
@@ -106,7 +106,7 @@ export class Help extends HelpBase {
     const command = this.config.findCommand(subject)
     if (command) {
       if (command.hasDynamicHelp && command.pluginType !== 'jit') {
-        const dynamicCommand = await toCached(await command.load())
+        const dynamicCommand = await toCached(await command.load(), undefined, false)
         await this.showCommandHelp(dynamicCommand)
       } else {
         await this.showCommandHelp(command)
diff --git a/src/interfaces/parser.ts b/src/interfaces/parser.ts
index c7ad4c0..5b2f15f 100644
--- a/src/interfaces/parser.ts
+++ b/src/interfaces/parser.ts
@@ -43,6 +43,7 @@ export type Metadata = {
 
 type MetadataFlag = {
   setFromDefault?: boolean;
+  defaultHelp?: unknown;
 }
 
 export type ListItem = [string, string | undefined]
@@ -55,11 +56,94 @@ export type DefaultContext<T> = {
   flags: Record<string, string>;
 }
 
-export type FlagDefault<T, P = CustomOptions> = T | ((context: DefaultContext<P & OptionFlag<T, P>>) => Promise<T>)
-export type FlagDefaultHelp<T, P = CustomOptions> = T | ((context: DefaultContext<P & OptionFlag<T, P>>) => Promise<string | undefined>)
-
-export type ArgDefault<T, P = CustomOptions> = T | ((context: DefaultContext<Arg<T, P>>) => Promise<T>)
-export type ArgDefaultHelp<T, P = CustomOptions> = T | ((context: DefaultContext<Arg<T, P>>) => Promise<string | undefined>)
+/**
+ * Type to define a default value for a flag.
+ * @param context The context of the flag.
+ * @param isWritingManifest Informs the function that a manifest file is being written.
+ * The manifest file is used to store the flag definitions, with a default value if present, for a command and is published to npm.
+ * When a manifest file is being written, the default value may contain data that should not be included in the manifest.
+ * The plugin developer can use isWritingManifest to determine if the default value should be omitted from the manifest.
+ * in the function's implementation.
+ * @example
+ * static flags = {
+ *   foo: flags.string({
+ *     defaultHelp: async (context, isWritingManifest) => {
+ *       if (isWritingManifest) {
+ *         return undefined
+ *       }
+ *       return 'value that is used outside a manifest'
+ *     },
+ *    }),
+ *  }
+ */
+export type FlagDefault<T, P = CustomOptions> = T | ((context: DefaultContext<P & OptionFlag<T, P>>, isWritingManifest?: boolean) => Promise<T>)
+
+/**
+ * Type to define a defaultHelp value for a flag.
+ * The defaultHelp value is used in the help output for the flag and when writing a manifest.
+ * It is also can be used to provide a value for the flag when issuing certain error messages.
+ *
+ * @param context The context of the flag.
+ * @param isWritingManifest Informs the function that a manifest file is being written.
+ * The manifest file is used to store the flag definitions, with a default value if present via defaultHelp, for a command and is published to npm.
+ * When a manifest file is being written, the default value may contain data that should not be included in the manifest.
+ * The plugin developer can use isWritingManifest to determine if the defaultHelp value should be omitted from the manifest.
+ * in the function's implementation.
+ * @example
+ * static flags = {
+ *   foo: flags.string({
+ *     defaultHelp: async (context, isWritingManifest) => {
+ *       if (isWritingManifest) {
+ *         return undefined
+ *       }
+ *       return 'value that is used outside a manifest'
+ *     },
+ *    }),
+ *  }
+ */
+export type FlagDefaultHelp<T, P = CustomOptions> = T | ((context: DefaultContext<P & OptionFlag<T, P>>, isWritingManifest?: boolean) => Promise<string | undefined>)
+
+/**
+ * Type to define a default value for an arg.
+ * @param context The context of the arg.
+ * @param isWritingManifest Informs the function that a manifest file is being written.
+ * The manifest file is used to store the arg definitions, with a default value if present, for a command and is published to npm.
+ * When a manifest file is being written, the default value may contain data that should not be included in the manifest.
+ * The plugin developer can use isWritingManifest to determine if the default value should be omitted from the manifest.
+ * in the function's implementation.
+ * @example
+ *   public static readonly args = {
+ *     one: Args.string({
+ *       default: async (context, isWritingManifest) => {
+ *          if (isWritingManifest) {
+ *             return undefined
+ *          }
+ *          return 'value that is used outside a manifest'
+ *       }),
+ *   };
+ */
+export type ArgDefault<T, P = CustomOptions> = T | ((context: DefaultContext<Arg<T, P>>, isWritingManifest?: boolean) => Promise<T>)
+
+/**
+ * Type to define a defaultHelp value for an arg.
+ * @param context The context of the arg.
+ * @param isWritingManifest Informs the function that a manifest file is being written.
+ * The manifest file is used to store the arg definitions, with a default value if present via defaultHelp, for a command and is published to npm.
+ * When a manifest file is being written, the default value may contain data that should not be included in the manifest.
+ * The plugin developer can use isWritingManifest to determine if the default value should be omitted from the manifest.
+ * in the function's implementation.
+ * @example
+ *   public static readonly args = {
+ *     one: Args.string({
+ *       defaultHelp: async (context, isWritingManifest) => {
+ *          if (isWritingManifest) {
+ *             return undefined
+ *          }
+ *          return 'value that is used outside a manifest'
+ *       }),
+ *   };
+ */
+export type ArgDefaultHelp<T, P = CustomOptions> = T | ((context: DefaultContext<Arg<T, P>>, isWritingManifest?: boolean) => Promise<string | undefined>)
 
 export type FlagRelationship = string | {name: string; when: (flags: Record<string, unknown>) => Promise<boolean>};
 export type Relationship = {
diff --git a/src/interfaces/plugin.ts b/src/interfaces/plugin.ts
index ea61b66..303f208 100644
--- a/src/interfaces/plugin.ts
+++ b/src/interfaces/plugin.ts
@@ -73,5 +73,5 @@ export interface Plugin {
 
   findCommand(id: string, opts: { must: true }): Promise<Command.Class>;
   findCommand(id: string, opts?: { must: boolean }): Promise<Command.Class> | undefined;
-  load(): Promise<void>;
+  load(isWritingManifest: boolean): Promise<void>;
 }
diff --git a/src/parser/parse.ts b/src/parser/parse.ts
index 09064fd..8bf7e01 100644
--- a/src/parser/parse.ts
+++ b/src/parser/parse.ts
@@ -217,6 +217,14 @@ export class Parser<T extends ParserInput, TFlags extends OutputFlags<T['flags']
       const flag = this.input.flags[token.flag]
 
       if (!flag) throw new CLIError(`Unexpected flag ${token.flag}`)
+
+      // if flag has defaultHelp, capture its value into metadata
+      if (Reflect.has(flag, 'defaultHelp')) {
+        const defaultHelpProperty = Reflect.get(flag, 'defaultHelp')
+        const defaultHelp = (typeof defaultHelpProperty === 'function' ? await defaultHelpProperty({options: flag, flags, ...this.context}) : defaultHelpProperty)
+        this.metaData.flags[token.flag] = {...this.metaData.flags[token.flag], defaultHelp}
+      }
+
       if (flag.type === 'boolean') {
         if (token.input === `--no-${flag.name}`) {
           flags[token.flag] = false
@@ -256,7 +264,7 @@ export class Parser<T extends ParserInput, TFlags extends OutputFlags<T['flags']
     for (const k of Object.keys(this.input.flags)) {
       const flag = this.input.flags[k]
       if (flags[k]) continue
-      if (flag.env && Object.prototype.hasOwnProperty.call(process.env, flag.env)) {
+      if (flag.env && Reflect.has(process.env, flag.env)) {
         const input = process.env[flag.env]
         if (flag.type === 'option') {
           if (input) {
@@ -271,7 +279,7 @@ export class Parser<T extends ParserInput, TFlags extends OutputFlags<T['flags']
       }
 
       if (!(k in flags) && flag.default !== undefined) {
-        this.metaData.flags[k] = {setFromDefault: true}
+        this.metaData.flags[k] = {...this.metaData.flags[k], setFromDefault: true}
         const defaultValue = (typeof flag.default === 'function' ? await flag.default({options: flag, flags, ...this.context}) : flag.default)
         flags[k] = defaultValue
       }
diff --git a/src/parser/validate.ts b/src/parser/validate.ts
index b2b1096..1748e3e 100644
--- a/src/parser/validate.ts
+++ b/src/parser/validate.ts
@@ -118,7 +118,8 @@ export async function validate(parse: {
       if (parse.output.metadata.flags && parse.output.metadata.flags[name]?.setFromDefault)
         continue
       if (parse.output.flags[flag] !== undefined) {
-        return {...base, status: 'failed', reason: `--${flag}=${parse.output.flags[flag]} cannot also be provided when using --${name}`}
+        const flagValue = parse.output.metadata.flags?.[flag]?.defaultHelp ?? parse.output.flags[flag]
+        return {...base, status: 'failed', reason: `--${flag}=${flagValue} cannot also be provided when using --${name}`}
       }
     }
 


Test patch:
diff --git a/test/command/command.test.ts b/test/command/command.test.ts
index e86db5f..15fccf5 100644
--- a/test/command/command.test.ts
+++ b/test/command/command.test.ts
@@ -103,7 +103,7 @@ describe('command', () => {
       }
       }
 
-      const c = await toCached(C)
+      const c = await toCached(C, undefined, false)
 
       expect(c).to.deep.equal({
         id: 'foo:bar',
diff --git a/test/help/help-test-utils.ts b/test/help/help-test-utils.ts
index 4f636b2..a58ca8a 100644
--- a/test/help/help-test-utils.ts
+++ b/test/help/help-test-utils.ts
@@ -47,7 +47,7 @@ export class TestHelp extends Help {
 
 export const commandHelp = (command?: any) => ({
   async run(ctx: {help: TestHelp; commandHelp: string; expectation: string}) {
-    const cached = await toCached(command!, {} as any)
+    const cached = await toCached(command!, {} as any, false)
     const help = ctx.help.formatCommand(cached)
     if (process.env.TEST_OUTPUT === '1') {
       console.log(help)
diff --git a/test/parser/parse.test.ts b/test/parser/parse.test.ts
index c5d34f9..aa9246d 100644
--- a/test/parser/parse.test.ts
+++ b/test/parser/parse.test.ts
@@ -684,6 +684,10 @@ See more help with --help`)
         constructor(input: string) {
           this.prop = input
         }
+
+        public toString(): string {
+          return this.prop
+        }
       }
       it('uses default via value', async () => {
         const out = await parse([], {
@@ -707,6 +711,43 @@ See more help with --help`)
         })
         expect(out.flags.foo?.prop).to.equal('baz')
       })
+      it('should error with exclusive flag violation', async () => {
+        try {
+          const out = await parse(['--foo', 'baz', '--bar'], {
+            flags: {
+              foo: Flags.custom<TestClass>({
+                parse: async input => new TestClass(input),
+                defaultHelp: new TestClass('bar'),
+              })(),
+              bar: Flags.boolean({
+                exclusive: ['foo'],
+              }),
+            },
+          })
+          expect.fail(`Should have thrown an error ${JSON.stringify(out)}`)
+        } catch (error) {
+          assert(error instanceof Error)
+          expect(error.message).to.include('--foo=bar cannot also be provided when using --bar')
+        }
+      })
+      it('should error with exclusive flag violation and defaultHelp value', async () => {
+        try {
+          const out = await parse(['--foo', 'baz', '--bar'], {
+            flags: {
+              foo: Flags.custom<TestClass>({
+                parse: async input => new TestClass(input),
+              })(),
+              bar: Flags.boolean({
+                exclusive: ['foo'],
+              }),
+            },
+          })
+          expect.fail(`Should have thrown an error ${JSON.stringify(out)}`)
+        } catch (error) {
+          assert(error instanceof Error)
+          expect(error.message).to.include('--foo=baz cannot also be provided when using --bar')
+        }
+      })
       it('uses parser when value provided', async () => {
         const out = await parse(['--foo=bar'], {
           flags: {