🍀 Proposal: Basic validation for command field

[iyear]

Description

I saw some TODOs left in plugin githubactions/golang/jobs.go. TODO(daniel-hutao): what should we validate here?

As an example, for both Build and Test struct, there is a user-defined command field. These commands are only likely to find errors when they are actually run, which is costly. I considered adding some basic validation to this field to prevent low-level errors.

This will give the user some error feedback when pluginengine calls Create Read Update Delete

Rough Plan

I will use a lightweight command line parser shellwords to convert command string to args.

For the standard library flag package, which is designed to be a parser rather than a validator, I'm considering modifying the underlying code so that it only needs to do the basic flag validation.

The flag validation here is not a parameter validation, but a check of the flag syntax.

So validating a command field requires two steps: convert to args and flag syntax validation

Eventually I will provide the project with an underlying utility function for validating command, which can be reused in many places.

Note: This proposal will import a new dependency github.com/mattn/go-shellwords

If the proposal makes sense, I will work for it.

[iyear]

I'm not sure if flag validation is needed, but from my understanding and the docs, we can add flag validation to command field to increase the constraint

[daniel-hutao]

Hi @iyear, thank you for your creative proposal!

Please take a look at #478 for the logic related to validation, and then maybe you will have some new ideas. Then we can further discuss how to improve the "robustness" of dtm

[iyear]

Perhaps the two validation types are not the same, but of course they can both be grouped in a validator package. aFlyBird is proposing a validation of os.Args, while I want to validate the command field. There should be a difference between them.

I have written the underlying code.If there are more validators in the future, we can consider the design of https://github.com/go-playground/validator

package main

import (
	"fmt"
	"github.com/mattn/go-shellwords"
)

func main() {
	fmt.Println(validate(`go env -w "GOOS=linux"`))
	fmt.Println(validate(`go env "-w" "GOOS=linux"`))
	fmt.Println(validate(`go env "-w" GOOS=linux""`))
	fmt.Println(validate(`go env ---w GOOS=linux`))
	fmt.Println(validate(`go env --=w GOOS=linux`))
	fmt.Println(validate(`go env -`))
	fmt.Println(validate(`go env --"`))
	fmt.Println(validate(`go"`))
	fmt.Println(validate(`go `))
	fmt.Println(validate(`go`))

}

func validate(cmd string) error {
	args, err := shellwords.Parse(cmd)
	if err != nil {
		return err
	}

	args = args[1:]
	for _, arg := range args {
		if arg == "" {
			continue
		}
		if arg[0] != '-' {
			continue
		}
		if len(arg) == 1 {
			return fmt.Errorf("argument %s is too short", arg)
		}

		minuses := 1
		if arg[1] == '-' {
			minuses++
			if len(arg) == 2 {
				return fmt.Errorf("argument %s is too short", arg)
			}
		}

		name := arg[minuses:]
		if len(name) == 0 || name[0] == '-' || name[0] == '=' {
			return fmt.Errorf("bad flag syntax: %s", arg)
		}
	}
	return nil
}

/*
Output:

<nil>
<nil>
<nil>
bad flag syntax: ---w
bad flag syntax: --=w
argument - is too short
invalid command line string
invalid command line string
<nil>
<nil>
*/

[daniel-hutao]

Hello @iyear, thank you for the detailed design. I'm so sorry for the delay replying.

Here are a few questions I want to discuss with you.

One is that if we verify the entire command, how should the rules be formulated? We already have a lot of commands, and there will definitely be many new commands in the future, the formulation of verification rules will be a point that needs to be carefully considered.

The other is that the place you mentioned where I left the todo(githubactions/golang/jobs.go) is not actually part of the command, it is some configuration that is rendered into the GitHub actions yaml file.

[iyear]

Thank you for your reply!

Maybe I'm misunderstanding the meaning of this field?

My understanding is that Build.Command and Test.Command will be rendered in the template as job.run. Its role is to run the user-defined command line.
I'm currently only thinking of doing basic syntax validation. I'm not sure if there should be restrictions on the specific content.

Of course I would then need to adapt it for the case of multiple commands, like the following:

run: |
      export PATH=$PATH:$(go env GOPATH)/bin
      go get -u golang.org/x/lint/golint 
      make lint

These two functions might end up looking like this, but of course this is just an example

func (b *Build) Validate() []error {
	retErrors := make([]error, 0)
	if err := validate(b.Command); err != nil {
		retErrors = append(retErrors, err)
	}
	// TODO(daniel-hutao): what should we validate here?

	return retErrors
}

func (t *Test) Validate() []error {
	retErrors := make([]error, 0)
	if err := validate(t.Command); err != nil {
		retErrors = append(retErrors, err)
	}

	// TODO(daniel-hutao): what should we validate here?

	return retErrors
}

func validate(cmd string) error {
	if cmd == "" {
		return nil
	}
	args, err := shellwords.Parse(cmd)
	if err != nil {
		return err
	}

	args = args[1:]
	for _, arg := range args {
		if arg == "" {
			continue
		}
		if arg[0] != '-' {
			continue
		}
		if len(arg) == 1 {
			return fmt.Errorf("argument %s is too short", arg)
		}

		minuses := 1
		if arg[1] == '-' {
			minuses++
			if len(arg) == 2 {
				return fmt.Errorf("argument %s is too short", arg)
			}
		}

		name := arg[minuses:]
		if len(name) == 0 || name[0] == '-' || name[0] == '=' {
			return fmt.Errorf("bad flag syntax: %s", arg)
		}
	}
	return nil
}

[daniel-hutao]

@iyear Build.Command is used in GitHub Actions workflows. So we won't know users how to define it.

[iyear]

@iyear Build.Command is used in GitHub Actions workflows. So we won't know users how to define it.

I have seen the doc and examples.
So the Command can not be a command line? Sorry about that, that is my mistake:(. I will think if there is another validation.

link: https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsrun

[daniel-hutao]

/assign @iyear Happy coding！

[iyear]

Summary: This proposal is a basic syntax check for the command field of the plugin configuration file, reducing low-level errors in user input, and the underlying functions are easy to reuse.

[daniel-hutao]

/close #533