Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix --verbose argument sanitation #1819

Closed
wants to merge 2 commits into from

Conversation

kelson42
Copy link
Collaborator

Fixes #1818

@kelson42
Copy link
Collaborator Author

@pavel-karatsiuba Please review and fix (linting)… afaik #1785 was not working at all. You confirm or I miss something?

@pavel-karatsiuba
Copy link
Contributor

@kelson42 I have created the branch to print the "verbose" parameter value into the log. Can you please test your command with this branch: #1820

I created the branch because I can't reproduce this bug. If I execute the command with --verbose in the source code it is equal to true.

@kelson42
Copy link
Collaborator Author

@pavel-karatsiuba OK.... I will.

@kelson42 kelson42 force-pushed the fix-verbose-argument-sanitation branch from 915c1f2 to 5365d4d Compare March 30, 2023 10:50
@kelson42
Copy link
Collaborator Author

kelson42 commented Mar 30, 2023

Your PR does not really clearly help IMO.

I have just added a debug log to dump the argv (see last commit). Here is what I get:

$ mw --mwUrl="https://bm.wikipedia.org" --verbose

> [email protected] start
> ./node_modules/.bin/ts-node-esm ./src/cli.ts [email protected] --osTmpDir=/dev/shm --verbose --optimisationCacheUrl=http://s3.us-west-1.wasabisys.com/?bucketName=org-kiwix-dev-mwoffliner&keyId=6JE56A5IWY5Z3H3L3KH6&secretAccessKey=... --mwUrl=https://bm.wikipedia.org --verbose

arguments: {
  _: [ [length]: 0 ],
  adminEmail: '[email protected]',
  'admin-email': '[email protected]',
  osTmpDir: '/dev/shm',
  'os-tmp-dir': '/dev/shm',
  verbose: [ true, true, [length]: 2 ],
  optimisationCacheUrl: 'http://s3.us-west-1.wasabisys.com/?bucketName=org-kiwix-dev-mwoffliner&keyId=6JE56A5IWY5Z3H3L3KH6&secretAccessKey=m2vQMjnneDjnrYztjfHrAUKneIf0SNGt75k9iITm',
  'optimisation-cache-url': 'http://s3.us-west-1.wasabisys.com/?bucketName=org-kiwix-dev-mwoffliner&keyId=6JE56A5IWY5Z3H3L3KH6&secretAccessKey=m2vQMjnneDjnrYztjfHrAUKneIf0SNGt75k9iITm',
  mwUrl: 'https://bm.wikipedia.org',
  'mw-url': 'https://bm.wikipedia.org',
  '$0': 'node_modules/ts-node/dist/child/child-entrypoint.js'
}
[log] [2023-03-30T10:45:37.725Z] Starting mwoffliner v1.12.1...

and

$ mw --mwUrl="https://bm.wikipedia.org" --verbose=foobar

> [email protected] start
> ./node_modules/.bin/ts-node-esm ./src/cli.ts [email protected] --osTmpDir=/dev/shm --verbose --optimisationCacheUrl=http://s3.us-west-1.wasabisys.com/?bucketName=org-kiwix-dev-mwoffliner&keyId=6JE56A5IWY5Z3H3L3KH6&secretAccessKey=... --mwUrl=https://bm.wikipedia.org --verbose=foobar

arguments: {
  _: [ [length]: 0 ],
  adminEmail: '[email protected]',
  'admin-email': '[email protected]',
  osTmpDir: '/dev/shm',
  'os-tmp-dir': '/dev/shm',
  verbose: [ true, 'foobar', [length]: 2 ],
  optimisationCacheUrl: 'http://s3.us-west-1.wasabisys.com/?bucketName=org-kiwix-dev-mwoffliner&keyId=6JE56A5IWY5Z3H3L3KH6&secretAccessKey=m2vQMjnneDjnrYztjfHrAUKneIf0SNGt75k9iITm',
  'optimisation-cache-url': 'http://s3.us-west-1.wasabisys.com/?bucketName=org-kiwix-dev-mwoffliner&keyId=6JE56A5IWY5Z3H3L3KH6&secretAccessKey=m2vQMjnneDjnrYztjfHrAUKneIf0SNGt75k9iITm',
  mwUrl: 'https://bm.wikipedia.org',
  'mw-url': 'https://bm.wikipedia.org',
  '$0': 'node_modules/ts-node/dist/child/child-entrypoint.js'
}
[error] [2023-03-30T10:49:58.215Z] Failed to run mwoffliner after [0s]: {
	"stack": "Error: verbose should be empty or one of [info, log, warn, error, quiet].\n    at sanitize_verbose (file:///home/kelson/code/mwoffliner/src/sanitize-argument.ts:93:11)\n    at sanitize_all (file:///home/kelson/code/mwoffliner/src/sanitize-argument.ts:25:3)\n    at file:///home/kelson/code/mwoffliner/src/cli.ts:59:1\n    at ModuleJob.run (node:internal/modules/esm/module_job:194:25)",
	"message": "verbose should be empty or one of [info, log, warn, error, quiet]."
}
[error] [2023-03-30T10:49:58.215Z] 

**********

verbose should be empty or one of [info, log, warn, error, quiet].

**********

@pavel-karatsiuba Do you get other values for argv? If "no", you confirm it works fine now?

@pavel-karatsiuba
Copy link
Contributor

@kelson42
You should edit your mw command. When you execute $ mw --mwUrl="https://bm.wikipedia.org" --verbose you are executing ./node_modules/.bin/ts-node-esm ./src/cli.ts [email protected] --osTmpDir=/dev/shm --verbose ... --mwUrl=https://bm.wikipedia.org --verbose=foobar.

So mw already contains --verbose and you write the parameter twice.
Also, edit your previous comment because it contains sensual information "secretAccessKey".

@kelson42
Copy link
Collaborator Author

kelson42 commented Mar 30, 2023

  • How should mw look like?
  • What is the purpose of ./src/cli.ts then?
  • Why the README given command npm run mwoffliner -- --help does not work for me?
  • Please make a PR immediatly to secure that arguments which should be given only once generates an yargs error if given twice.

@kelson42
Copy link
Collaborator Author

kelson42 commented Apr 2, 2023

@pavel-karatsiuba I'm still expecting an answer here.

@pavel-karatsiuba
Copy link
Contributor

  • Actually, you don't need to edit mw command. Because after I fix this issue, your command should work or you will see an error with a clear message.
  • ./src/cli.ts is the script entry point.
  • Maybe npm run mwoffliner -- --help is not working because you did not execute npm i. Or it should give you the error message

@kelson42
Copy link
Collaborator Author

kelson42 commented Apr 5, 2023

Inappropriate PR as the root of the problem is not a problem in the parsing of the --verbose argument but that I have overseen the duplicated --verbose (fixed with #1823).

@kelson42 kelson42 closed this Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate in cmd line arguments are not checked (was: --verbose does not accept empty value)
2 participants