Added regex based input validation when creating a new sketch #2049
Conversation
|
I see some test's failed but I'm unsure of why or how to fix, let me know what steps to take and I'll give it my best shot |
per1234
added
topic: code
internal/cli/sketch/new.go
Outdated
| @@ -38,7 +39,14 @@ func initNewCommand() *cobra.Command { | |||
| Long: tr("Create a new Sketch"), | |||
| Example: " " + os.Args[0] + " sketch new MultiBlinker", | |||
| Args: cobra.ExactArgs(1), | |||
| Run: func(cmd *cobra.Command, args []string) { runNewCommand(args, overwrite) }, | |||
| Run: func(cmd *cobra.Command, args []string) { | |||
| re := regexp.MustCompile("^[a-zA-Z].") | |||
There was a problem hiding this comment.
This regular expression is incorrect. See the requirements in the Arduino Sketch Specification:
https://arduino.github.io/arduino-cli/dev/sketch-specification/#sketch-folders-and-files
You can see a correct regular expression (JavaScript-based) here:
internal/cli/sketch/new.go
Outdated
| Run: func(cmd *cobra.Command, args []string) { | ||
| re := regexp.MustCompile("^[a-zA-Z].") | ||
| if !re.MatchString(args[0]) { | ||
| fmt.Println("Error: Value can only contain alphabetic characters") |
There was a problem hiding this comment.
| fmt.Println("Error: Value can only contain alphabetic characters") | |
| fmt.Println("Error: Value can only contain alphabetic characters") |
The error message is incorrect. You can see an appropriate error message here:
| Run: func(cmd *cobra.Command, args []string) { runNewCommand(args, overwrite) }, | ||
| Run: func(cmd *cobra.Command, args []string) { | ||
| re := regexp.MustCompile("^[a-zA-Z].") | ||
| if !re.MatchString(args[0]) { |
There was a problem hiding this comment.
You are testing the entire path here. The specification must be applied only to the sketch folder name.
This is the reason the unit tests are failing. Even though they have a valid sketch folder name, the path starts with /, which is not allowed by your regex.
There was a problem hiding this comment.
Thanks for the quick reply, I'm working on those changes now.
There was a problem hiding this comment.
Please adopt the same validation in the direct gRPC flow, so that cli and gRPC users see the same behavior.
Also mark the pr as a breaking change since some currently working calls will start to fail.
PS: I realized too late that I was working on the same fix, my bad. However, I feel like you could take some inspiration, see #2051
Please check if the PR fulfills these requirements
See how to contribute
before creating one)
our contributing guidelines
UPGRADING.mdhas been updated with a migration guide (for breaking changes)What kind of change does this PR introduce?
Bug fix.
What is the current behavior?
Users are allowed inputs like 'sketch new $%^[].cpp'
What is the new behavior?
When 'sketch new' is called the input is checked against a regular expression to verify there are only alphabetic characters.
Does this PR introduce a breaking change, and is titled accordingly?
This code does not introduce any breaking changes.
Other information
Fixes issue #2043.