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

Some Lotus CLI commands don't enforce number of arguments #12790

Open
rvagg opened this issue Dec 16, 2024 · 3 comments
Open

Some Lotus CLI commands don't enforce number of arguments #12790

rvagg opened this issue Dec 16, 2024 · 3 comments
Assignees
Labels
good first issue Good for newcomers

Comments

@rvagg
Copy link
Member

rvagg commented Dec 16, 2024

Ref: #12788

lotus/cli/wallet.go

Lines 735 to 739 in 7f2068e

// Get amount param
if cctx.NArg() < 1 {
return IncorrectNumArgs(cctx)
}
f, err := types.ParseFIL(cctx.Args().First())

lotus wallet add does a < 1 check on NArg(), but it really should be != 1 to match the usage <amount>. This would pick up any mis-use, like supplying the options too late (thinking that it's got GNU CLI flexibility when it's unfortunately Go's frustrating typical BSD CLI inflexibility). I haven't looked but it's likely that there are more in here with this same problem.

Marking this job as good first issue because it's really just a fairly straightforward audit; walking through all of the commands attached to the lotus CLI (we'll scope it to that for now) and checking that NArgs is being properly enforced to match the usage (or vice versa).

Ideally the CLI package should be able to understand its usage string and enforce that, or maybe we could come up with our own usage string parser (e.g. "<foo> [bar] [baz]" should have >=1 && <=3); but that could be a later-job and for now just getting them matched would be good.

@rvagg rvagg added the good first issue Good for newcomers label Dec 16, 2024
@github-project-automation github-project-automation bot moved this to 📌 Triage in FilOz Dec 16, 2024
@ameeetgaikwad
Copy link

would like to take this up!

@rvagg
Copy link
Member Author

rvagg commented Dec 17, 2024

Great, consider it yours @ameeetgaikwad! I don't know how many commands are in this situation so this is going to require a manual walk through the lotus CLI commands. There are few things to be aware of:

  1. There is more than just lotus in the codebase, there's a bunch of CLI tools that aren't in scope here (lotus-miner, lotus-shed, etc.); the scope of all of them is pretty huge so let's limit the task to just lotus.
  2. feat(docs): generate cli docs directly from the struct #12717 is due to land soon which will change things a bit, so be aware there will be some code churn (you're welcome to start from that branch if you like under the expectation that it represents the way the code will be soon)
  3. Command setup is done here: https://github.com/filecoin-project/lotus/blob/c76d6b97f44b5a86888c41e521d47670d86d1f20/cmd/lotus/main.go#L116-L117 - note that there's two separate batches of commands appended, local and the ones in clicommands.Commands. The latter are the larger main group but the fromer have a few special commands too that might be worth a look. You want to walk through each of the commands that go into this and see if they are doing the checks properly.

@rjan90 rjan90 moved this from 📌 Triage to ⌨️ In Progress in FilOz Dec 18, 2024
@rjan90
Copy link
Contributor

rjan90 commented Dec 18, 2024

@ameeetgaikwad I assigned the issue to you, and set its status to "In Progress". Let us know if you have any questions with regards to the issue, or need help to get unblocked on any techincal matters

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
Status: ⌨️ In Progress
Development

No branches or pull requests

3 participants