maybeCoerceNumber bombs if coerce produced an object without toString

[groner]

I ran into this trying to use pg-connection-string.parse as a coerce function today, it returns an object with a null prototype which can't be coerced to a string.

import yargs from 'yargs';
import {parse as pgcs_parse} from 'pg-connection-string';


const parser = yargs('--db=postgresql://db/something'.split(/\s+/))
  .strict()
  .option('db', {
    coerce: pgcs_parse,
  })
  .fail((_, err) => {
    console.error(err.stack);
    process.exit(1);
  })
  ;

const argv = parser.parse();

console.log(argv);

:; ts-node test.ts
YError: Cannot convert object to primitive value
    at Object.parseArgs [as _parseArgs] (.../node_modules/yargs/build/index.cjs:2762:27)
    at Object.parse (.../node_modules/yargs/build/index.cjs:2260:31)
    at Object.<anonymous> (.../test.ts:16:21)
    at Module._compile (internal/modules/cjs/loader.js:1063:30)
    at Module.m._compile (/usr/local/lib/node_modules/ts-node/src/index.ts:858:23)
    at Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
    at Object.require.extensions.<computed> [as .ts] (/usr/local/lib/node_modules/ts-node/src/index.ts:861:12)
    at Module.load (internal/modules/cjs/loader.js:928:32)
    at Function.Module._load (internal/modules/cjs/loader.js:769:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12)

The original stack trace is lost when it gets converted to YError, but the problem occurs in looksLikeNumber when the regexp is applied. It seems to me that maybeCoerceNumber could check if value is a string before doing anything more.

[sTEVE7611]

Ever think about changing the words from yargs to yards?

…

On Thu, Feb 11, 2021 at 5:47 PM Kai Groner ***@***.***> wrote: I ran into this trying to use pg-connection-string.parse as a coerce function today, it returns an object with a null prototype which can't be coerced to a string. import yargs from 'yargs';import {parse as pgcs_parse} from 'pg-connection-string'; const parser = yargs('--db=postgresql://db/something'.split(/\s+/)) .strict() .option('db', { coerce: pgcs_parse, }) .fail((_, err) => { console.error(err.stack); process.exit(1); }) ; const argv = parser.parse(); console.log(argv); :; ts-node test.ts YError: Cannot convert object to primitive value at Object.parseArgs [as _parseArgs] (.../node_modules/yargs/build/index.cjs:2762:27) at Object.parse (.../node_modules/yargs/build/index.cjs:2260:31) at Object.<anonymous> (.../test.ts:16:21) at Module._compile (internal/modules/cjs/loader.js:1063:30) at Module.m._compile (/usr/local/lib/node_modules/ts-node/src/index.ts:858:23) at Module._extensions..js (internal/modules/cjs/loader.js:1092:10) at Object.require.extensions.<computed> [as .ts] (/usr/local/lib/node_modules/ts-node/src/index.ts:861:12) at Module.load (internal/modules/cjs/loader.js:928:32) at Function.Module._load (internal/modules/cjs/loader.js:769:14) at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12) The original stack trace is lost when it gets converted to YError, but the problem occurs in looksLikeNumber when the regexp is applied. It seems to me that maybeCoerceNumber could check if value is a string before doing anything more. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#352>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ASM4AJTPDSQGRMRQPWM7QHTS6SCD3ANCNFSM4XP4GMAQ> .

[bcoe]

@groner is the correct behavior for us to detect there's no .toString and just return false?

[groner]

The bug comes from something looksLikeNumber doing something that tries and fails to coerce an object to a string. My thinking is that if the value is anything but a string, it probably doesn't need further interpretation. I would add a typeof value === 'string' check in maybeCoerceNumber or looksLikeNumber would solve this.

A more conservative fix might be to check if String(value) works without throwing an exception. Testing for .toString is probably fine too. There's a corner case with null/undefined, but we know they're not numbers.