lirantal / lockfile-lint

Lint an npm or yarn lockfile to analyze and detect security issues
Apache License 2.0
781 stars 35 forks source link

The tests fail because yargs detects the OS' locale #41

Closed MathieuAA closed 4 years ago

MathieuAA commented 4 years ago

I can do a PR for the issue if you want.

Expected Behavior

The tests should pass when cloning & installing the project.

Current Behavior

They do not. See:

  CLI tests
    ✕ Running without parameters should display help (94ms)
    ✕ Running without parameters should display a requirement for the p option (85ms)
    ✕ Linting a file that has wrong host should display an error message and use exit code 1 (76ms)
    ✓ Linting a file that has wrong host should return exit code 1 (74ms)

  ● CLI tests › Running without parameters should display help

    expect(received).not.toBe(expected) // Object.is equality

    Expected: not -1

      17 |       expect(output.indexOf('Usage:')).not.toBe(-1)
      18 |       expect(output.indexOf('Options:')).not.toBe(-1)
    > 19 |       expect(output.indexOf('Examples:')).not.toBe(-1)
         |                                               ^
      20 |       done()
      21 |     })
      22 |   })

      at Socket.toBe (__tests__/cli.test.js:19:47)

That's because my locale is set to the french one, and yargs translates the CLI output (at least some of it) (See https://github.com/yargs/yargs/blob/master/docs/api.md#detectlocaleboolean Your tests rely on the fact that the user has an english locale.

See the output of the npx lockfile-lint -h command:

Usage: lockfile-lint --path <path-to-lockfile> --allowed-hosts yarn npm

Options:
  --version              Affiche le numéro de version                  [booléen]
  --help, -h             Affiche de l'aide                             [booléen]
  -p, --path             path to the lockfile     [chaine de caractère] [requis]
  -t, --type             lockfile type, options are "npm" or "yarn"
                                                           [chaine de caractère]
  -s, --validate-https   validates the use of HTTPS as protocol schema for all
                         resources                                     [booléen]
  -e, --empty-hostname   allows empty hostnames, or set to false if you wish for
                         a stricter policy              [booléen] [défaut: true]
  -a, --allowed-hosts    validates a whitelist of allowed hosts to be used for
                         resources in the lockfile                     [tableau]
  -o, --allowed-schemes  validates a whitelist of allowed schemes to be used for
                         resources in the lockfile                     [tableau]

Exemples:
  lockfile-lint --path yarn.lock --validate-https
  lockfile-lint --path yarn.lock --validate-https --allowed-hosts npm yarn
  verdaccio
  lockfile-lint --path yarn.lock --allowed-schemes "https:" "git+ssh:"
  --allowed-hosts npm yarn verdaccio

curated by Liran Tal at https://github.com/lirantal/lockfile-lint

Notice the Exemples string

Possible Solution

Adding

.detectLocale(false)

to the lockfile-lint/src.main.js file works.

Steps to Reproduce (for bugs)

Well, I don't know how to set my locale to another, but I guess https://www.tecmint.com/set-system-locales-in-linux/ should help

  1. Clone the project
  2. Install the dependencies
  3. Launch the tests

Context

I was trying to improve the output of the expected protocol; I've tried to analyze one of my project's lockfile and I was surprised at the output (I expected HTTPS, not https:, do you want to a PR for that?)

Your Environment

Ubuntu LTS, french locale

The output of the locale command.

LANG=fr_FR.UTF-8
LANGUAGE=
LC_CTYPE="fr_FR.UTF-8"
LC_NUMERIC="fr_FR.UTF-8"
LC_TIME="fr_FR.UTF-8"
LC_COLLATE="fr_FR.UTF-8"
LC_MONETARY="fr_FR.UTF-8"
LC_MESSAGES="fr_FR.UTF-8"
LC_PAPER="fr_FR.UTF-8"
LC_NAME="fr_FR.UTF-8"
LC_ADDRESS="fr_FR.UTF-8"
LC_TELEPHONE="fr_FR.UTF-8"
LC_MEASUREMENT="fr_FR.UTF-8"
LC_IDENTIFICATION="fr_FR.UTF-8"
LC_ALL=
MathieuAA commented 4 years ago

By the way, thanks for the article!

lirantal commented 4 years ago

@MathieuAA apologies for the late reply on this. Would definitely merge a PR that fixes this, and seems the solution is just to disable detection? Would be happy to land it.

lirantal commented 4 years ago

I'm not sure on translating the https: to HTTPS because I'm not convinced it will be easier to understand. Did it confuse you? what will we do with other protocol schemes like git+ssh: ?

MathieuAA commented 4 years ago

As there's a finite set of possible protocols, formatting them shouldn't be too big a deal. Frankly, just for that, it's not worth adding the formatting. Fixing the test is more important. I won't be able to do it til tomorrow. The fix stays the same, if you wanna do it before tomorrow, by all means.

lirantal commented 4 years ago

@MathieuAA no rush on my side so when you get to it in the upcoming week that's fine