From 193c89f67b220780ae33744189b8cc4eec855cac Mon Sep 17 00:00:00 2001 From: Luca Bianconi Date: Tue, 24 Jan 2023 14:11:51 +0100 Subject: [PATCH 1/6] fix: validate sketch name --- commands/sketch/new.go | 17 +++++++++ commands/sketch/new_test.go | 71 +++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+) create mode 100644 commands/sketch/new_test.go diff --git a/commands/sketch/new.go b/commands/sketch/new.go index b2871c48d95..b61419970e3 100644 --- a/commands/sketch/new.go +++ b/commands/sketch/new.go @@ -18,6 +18,7 @@ package sketch import ( "context" "errors" + "regexp" "github.com/arduino/arduino-cli/arduino" "github.com/arduino/arduino-cli/arduino/globals" @@ -34,6 +35,9 @@ void loop() { } `) +var sketchNameValidationRegex = regexp.MustCompile(`^([A-Za-z\d]+[_.-]*)+$`) +var sketchNameMaxLength = 63 + // NewSketch creates a new sketch via gRPC func NewSketch(ctx context.Context, req *rpc.NewSketchRequest) (*rpc.NewSketchResponse, error) { var sketchesDir string @@ -42,6 +46,19 @@ func NewSketch(ctx context.Context, req *rpc.NewSketchRequest) (*rpc.NewSketchRe } else { sketchesDir = configuration.Settings.GetString("directories.User") } + + if len(req.SketchName) > sketchNameMaxLength { + return nil, &arduino.CantCreateSketchError{Cause: errors.New(tr("sketch name too long (%d characters). Maximum allowed length is %d", + len(req.SketchName), + sketchNameMaxLength))} + } + + if !sketchNameValidationRegex.MatchString(req.SketchName) { + return nil, &arduino.CantCreateSketchError{Cause: errors.New(tr("invalid sketch name \"%s\". Required pattern %s", + req.SketchName, + sketchNameValidationRegex.String()))} + } + sketchDirPath := paths.New(sketchesDir).Join(req.SketchName) if err := sketchDirPath.MkdirAll(); err != nil { return nil, &arduino.CantCreateSketchError{Cause: err} diff --git a/commands/sketch/new_test.go b/commands/sketch/new_test.go new file mode 100644 index 00000000000..3b1c780ea70 --- /dev/null +++ b/commands/sketch/new_test.go @@ -0,0 +1,71 @@ +package sketch + +import ( + "context" + "testing" + + "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1" + "github.com/stretchr/testify/require" +) + +func Test_SketchNameWrongPattern(t *testing.T) { + invalidNames := []string{ + "&", + "", + ".hello", + "_hello", + "-hello", + "hello*", + "||||||||||||||", + ",`hack[}attempt{];", + } + for _, name := range invalidNames { + _, err := NewSketch(context.Background(), &commands.NewSketchRequest{ + SketchName: name, + SketchDir: t.TempDir(), + }) + require.NotNil(t, err) + + require.Error(t, err, `Can't create sketch: invalid sketch name "%s". Required pattern %s`, + name, + sketchNameValidationRegex) + } +} + +func Test_SketchNameTooLong(t *testing.T) { + tooLongName := make([]byte, sketchNameMaxLength+1) + for i := range tooLongName { + tooLongName[i] = 'a' + } + _, err := NewSketch(context.Background(), &commands.NewSketchRequest{ + SketchName: string(tooLongName), + SketchDir: t.TempDir(), + }) + require.NotNil(t, err) + + require.Error(t, err, `Can't create sketch: sketch name too long (%d characters). Maximum allowed length is %d`, + len(tooLongName), + sketchNameMaxLength) +} + +func Test_SketchNameOk(t *testing.T) { + lengthLimitName := make([]byte, sketchNameMaxLength) + for i := range lengthLimitName { + lengthLimitName[i] = 'a' + } + validNames := []string{ + "h", + "h.ello", + "h..ello-world", + "h..ello-world.", + "hello_world__", + string(lengthLimitName), + } + for _, name := range validNames { + _, err := NewSketch(context.Background(), &commands.NewSketchRequest{ + SketchName: name, + SketchDir: t.TempDir(), + }) + require.Nil(t, err) + } +} From d7ec61e5b6d01fb8d3f25572b8efa61e307ad1a6 Mon Sep 17 00:00:00 2001 From: Luca Bianconi Date: Tue, 24 Jan 2023 14:34:29 +0100 Subject: [PATCH 2/6] chore: updated UPGRADING.md --- docs/UPGRADING.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index b7b267ab1e1..69e5c0bbe3b 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -4,6 +4,14 @@ Here you can find a list of migration guides to handle breaking changes between ## 0.30.0 +### Sketch name validation + +The sketch name submitted via the `sketch new` command of the CLI or the gRPC command +`cc.arduino.cli.commands.v1.NewSketch` are now validated. The applied rules follow the +[sketch specifications](https://arduino.github.io/arduino-cli/dev/sketch-specification). + +Existing sketch names that don't respect the new constraints need to be updated. + ### `daemon` CLI command's `--ip` flag removal The `daemon` CLI command no longer allows to set a custom ip for the gRPC communication. Currently there is not enough From a69c4a4274c3d6092f1488ae59fac363a5b03c23 Mon Sep 17 00:00:00 2001 From: Luca Bianconi Date: Thu, 26 Jan 2023 09:23:30 +0100 Subject: [PATCH 3/6] refactor: cleanup --- commands/sketch/new.go | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/commands/sketch/new.go b/commands/sketch/new.go index b61419970e3..97afda4b661 100644 --- a/commands/sketch/new.go +++ b/commands/sketch/new.go @@ -35,8 +35,9 @@ void loop() { } `) -var sketchNameValidationRegex = regexp.MustCompile(`^([A-Za-z\d]+[_.-]*)+$`) +// sketchNameMaxLength could be part of the regex, but it's intentionally left out for clearer error reporting var sketchNameMaxLength = 63 +var sketchNameValidationRegex = regexp.MustCompile(`^[0-9a-zA-Z]{1}[0-9a-zA-Z_\.-]*$`) // NewSketch creates a new sketch via gRPC func NewSketch(ctx context.Context, req *rpc.NewSketchRequest) (*rpc.NewSketchResponse, error) { @@ -47,16 +48,8 @@ func NewSketch(ctx context.Context, req *rpc.NewSketchRequest) (*rpc.NewSketchRe sketchesDir = configuration.Settings.GetString("directories.User") } - if len(req.SketchName) > sketchNameMaxLength { - return nil, &arduino.CantCreateSketchError{Cause: errors.New(tr("sketch name too long (%d characters). Maximum allowed length is %d", - len(req.SketchName), - sketchNameMaxLength))} - } - - if !sketchNameValidationRegex.MatchString(req.SketchName) { - return nil, &arduino.CantCreateSketchError{Cause: errors.New(tr("invalid sketch name \"%s\". Required pattern %s", - req.SketchName, - sketchNameValidationRegex.String()))} + if err := validateSketchName(req.SketchName); err != nil { + return nil, err } sketchDirPath := paths.New(sketchesDir).Join(req.SketchName) @@ -76,3 +69,18 @@ func NewSketch(ctx context.Context, req *rpc.NewSketchRequest) (*rpc.NewSketchRe return &rpc.NewSketchResponse{MainFile: sketchMainFilePath.String()}, nil } + +func validateSketchName(name string) error { + if len(name) > sketchNameMaxLength { + return &arduino.CantCreateSketchError{Cause: errors.New(tr("sketch name too long (%d characters). Maximum allowed length is %d", + len(name), + sketchNameMaxLength))} + } + + if !sketchNameValidationRegex.MatchString(name) { + return &arduino.CantCreateSketchError{Cause: errors.New(tr("invalid sketch name \"%s\". Required pattern %s", + name, + sketchNameValidationRegex.String()))} + } + return nil +} From 90c43c9ca201438b2973302932c5f434dde20e1c Mon Sep 17 00:00:00 2001 From: Luca Bianconi Date: Thu, 26 Jan 2023 09:26:56 +0100 Subject: [PATCH 4/6] chore: upgrading.md wording --- docs/UPGRADING.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 69e5c0bbe3b..0f2ff7ef829 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -10,8 +10,7 @@ The sketch name submitted via the `sketch new` command of the CLI or the gRPC co `cc.arduino.cli.commands.v1.NewSketch` are now validated. The applied rules follow the [sketch specifications](https://arduino.github.io/arduino-cli/dev/sketch-specification). -Existing sketch names that don't respect the new constraints need to be updated. - +Existing sketch names violating the new constraint need to be updated. ### `daemon` CLI command's `--ip` flag removal The `daemon` CLI command no longer allows to set a custom ip for the gRPC communication. Currently there is not enough From ef98bbee55c0aff4ed6a2c97fe3de6d1cafb86ca Mon Sep 17 00:00:00 2001 From: Luca Bianconi Date: Thu, 26 Jan 2023 13:54:38 +0100 Subject: [PATCH 5/6] chore: prettier --- docs/UPGRADING.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 0f2ff7ef829..9c9cb4b068c 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -11,6 +11,7 @@ The sketch name submitted via the `sketch new` command of the CLI or the gRPC co [sketch specifications](https://arduino.github.io/arduino-cli/dev/sketch-specification). Existing sketch names violating the new constraint need to be updated. + ### `daemon` CLI command's `--ip` flag removal The `daemon` CLI command no longer allows to set a custom ip for the gRPC communication. Currently there is not enough From 35eec5e5e06206717489d030142d57566bd2a532 Mon Sep 17 00:00:00 2001 From: Luca Bianconi Date: Fri, 27 Jan 2023 14:47:23 +0100 Subject: [PATCH 6/6] fix: handle empty sketch name --- commands/sketch/new.go | 6 ++-- commands/sketch/new_test.go | 11 +++++++ internal/cli/sketch/new.go | 31 ++++++++++++++----- .../integrationtest/sketch/sketch_test.go | 10 ++++++ 4 files changed, 49 insertions(+), 9 deletions(-) diff --git a/commands/sketch/new.go b/commands/sketch/new.go index 97afda4b661..4e8134a5b6c 100644 --- a/commands/sketch/new.go +++ b/commands/sketch/new.go @@ -37,7 +37,7 @@ void loop() { // sketchNameMaxLength could be part of the regex, but it's intentionally left out for clearer error reporting var sketchNameMaxLength = 63 -var sketchNameValidationRegex = regexp.MustCompile(`^[0-9a-zA-Z]{1}[0-9a-zA-Z_\.-]*$`) +var sketchNameValidationRegex = regexp.MustCompile(`^[0-9a-zA-Z][0-9a-zA-Z_\.-]*$`) // NewSketch creates a new sketch via gRPC func NewSketch(ctx context.Context, req *rpc.NewSketchRequest) (*rpc.NewSketchResponse, error) { @@ -71,12 +71,14 @@ func NewSketch(ctx context.Context, req *rpc.NewSketchRequest) (*rpc.NewSketchRe } func validateSketchName(name string) error { + if name == "" { + return &arduino.CantCreateSketchError{Cause: errors.New(tr("sketch name cannot be empty"))} + } if len(name) > sketchNameMaxLength { return &arduino.CantCreateSketchError{Cause: errors.New(tr("sketch name too long (%d characters). Maximum allowed length is %d", len(name), sketchNameMaxLength))} } - if !sketchNameValidationRegex.MatchString(name) { return &arduino.CantCreateSketchError{Cause: errors.New(tr("invalid sketch name \"%s\". Required pattern %s", name, diff --git a/commands/sketch/new_test.go b/commands/sketch/new_test.go index 3b1c780ea70..88d2e426012 100644 --- a/commands/sketch/new_test.go +++ b/commands/sketch/new_test.go @@ -32,6 +32,17 @@ func Test_SketchNameWrongPattern(t *testing.T) { } } +func Test_SketchNameEmpty(t *testing.T) { + emptyName := "" + _, err := NewSketch(context.Background(), &commands.NewSketchRequest{ + SketchName: emptyName, + SketchDir: t.TempDir(), + }) + require.NotNil(t, err) + + require.Error(t, err, `Can't create sketch: sketch name cannot be empty`) +} + func Test_SketchNameTooLong(t *testing.T) { tooLongName := make([]byte, sketchNameMaxLength+1) for i := range tooLongName { diff --git a/internal/cli/sketch/new.go b/internal/cli/sketch/new.go index 3b361f3d8ea..2f76d691b4c 100644 --- a/internal/cli/sketch/new.go +++ b/internal/cli/sketch/new.go @@ -49,16 +49,33 @@ func initNewCommand() *cobra.Command { func runNewCommand(args []string, overwrite bool) { logrus.Info("Executing `arduino-cli sketch new`") // Trim to avoid issues if user creates a sketch adding the .ino extesion to the name - sketchName := args[0] - trimmedSketchName := strings.TrimSuffix(sketchName, globals.MainFileValidExtension) - sketchDirPath, err := paths.New(trimmedSketchName).Abs() - if err != nil { - feedback.Fatal(tr("Error creating sketch: %v", err), feedback.ErrGeneric) + inputSketchName := args[0] + trimmedSketchName := strings.TrimSuffix(inputSketchName, globals.MainFileValidExtension) + + var sketchDir string + var sketchName string + var sketchDirPath *paths.Path + var err error + + if trimmedSketchName == "" { + // `paths.New` returns nil with an empty string so `paths.Abs` panics. + // if the name is empty we rely on the "new" command to fail nicely later + // with the same logic in grpc and cli flows + sketchDir = "" + sketchName = "" + } else { + sketchDirPath, err = paths.New(trimmedSketchName).Abs() + if err != nil { + feedback.Fatal(tr("Error creating sketch: %v", err), feedback.ErrGeneric) + } + sketchDir = sketchDirPath.Parent().String() + sketchName = sketchDirPath.Base() } + _, err = sk.NewSketch(context.Background(), &rpc.NewSketchRequest{ Instance: nil, - SketchName: sketchDirPath.Base(), - SketchDir: sketchDirPath.Parent().String(), + SketchName: sketchName, + SketchDir: sketchDir, Overwrite: overwrite, }) if err != nil { diff --git a/internal/integrationtest/sketch/sketch_test.go b/internal/integrationtest/sketch/sketch_test.go index 496236b31d1..69854540536 100644 --- a/internal/integrationtest/sketch/sketch_test.go +++ b/internal/integrationtest/sketch/sketch_test.go @@ -73,6 +73,16 @@ func TestSketchNew(t *testing.T) { require.FileExists(t, currentSketchPath.Join(sketchName).String()+".ino") } +func TestSketchNewEmptyName(t *testing.T) { + // testing that we fail nicely. It panicked in the past + env, cli := integrationtest.CreateArduinoCLIWithEnvironment(t) + defer env.CleanUp() + + sketchName := "" + _, _, err := cli.Run("sketch", "new", sketchName) + require.Error(t, err, "Can't create sketch: sketch name cannot be empty") +} + func verifyZipContainsSketchExcludingBuildDir(t *testing.T, files []*zip.File) { require.Len(t, files, 8) require.Equal(t, paths.New("sketch_simple", "doc.txt").String(), files[0].Name)