Merge pull request #60 from arduino/per1234/disallowed-characters-in-base-name-check

Add checks for disallowed characters in library and sketch base names

Author: per1234

check/checkconfigurations/checkconfigurations.go

@@ -851,6 +851,36 @@ var configurations = []Type{
 		ErrorModes:      nil,
 		CheckFunction:   checkfunctions.LibraryHasExe,
 	},
+	{
+		ProjectType:     projecttype.Library,
+		Category:        "structure",
+		Subcategory:     "",
+		ID:              "",
+		Brief:           "disallowed characters in folder name",
+		Description:     "This will be problematic for people doing manual installation of the library.",
+		MessageTemplate: "Prohibited characters in folder name: {{.}}. See: https://arduino.github.io/arduino-cli/latest/library-specification/#library-root-folder",
+		DisableModes:    nil,
+		EnableModes:     []checkmode.Type{checkmode.All},
+		InfoModes:       nil,
+		WarningModes:    []checkmode.Type{checkmode.Permissive},
+		ErrorModes:      []checkmode.Type{checkmode.Default},
+		CheckFunction:   checkfunctions.ProhibitedCharactersInLibraryFolderName,
+	},
+	{
+		ProjectType:     projecttype.Sketch,
+		Category:        "structure",
+		Subcategory:     "",
+		ID:              "",
+		Brief:           "disallowed characters in file name",
+		Description:     "",
+		MessageTemplate: "Prohibited characters in file name(s): {{.}}. See: https://arduino.github.io/arduino-cli/latest/sketch-specification/#sketch-root-folder",
+		DisableModes:    nil,
+		EnableModes:     []checkmode.Type{checkmode.All},
+		InfoModes:       nil,
+		WarningModes:    nil,
+		ErrorModes:      []checkmode.Type{checkmode.Default},
+		CheckFunction:   checkfunctions.ProhibitedCharactersInSketchFileName,
+	},
 	{
 		ProjectType:     projecttype.Sketch,
 		Category:        "structure",

check/checkfunctions/checkfunctions.go

@@ -17,9 +17,17 @@
 package checkfunctions
 
 import (
+	"regexp"
+
 	"github.com/arduino/arduino-check/check/checkresult"
 )
 
 // Type is the function signature for the check functions.
 // The `output` result is the contextual information that will be inserted into the check's message template.
 type Type func() (result checkresult.Type, output string)
+
+// validProjectPathBaseName checks whether the provided library folder or sketch filename contains prohibited characters.
+func validProjectPathBaseName(name string) bool {
+	baseNameRegexp := regexp.MustCompile("^[a-zA-Z0-9][a-zA-Z0-9_.-]*$")
+	return baseNameRegexp.MatchString(name)
+}

check/checkfunctions/library.go

@@ -929,6 +929,15 @@ func LibraryHasExe() (result checkresult.Type, output string) {
 	return checkresult.Pass, ""
 }
 
+// ProhibitedCharactersInLibraryFolderName checks for prohibited characters in the library folder name.
+func ProhibitedCharactersInLibraryFolderName() (result checkresult.Type, output string) {
+	if !validProjectPathBaseName(checkdata.ProjectPath().Base()) {
+		return checkresult.Fail, checkdata.ProjectPath().Base()
+	}
+
+	return checkresult.Pass, ""
+}
+
 // spellCheckLibraryPropertiesFieldValue returns the value of the provided library.properties field with commonly misspelled words corrected.
 func spellCheckLibraryPropertiesFieldValue(fieldName string) (result checkresult.Type, output string) {
 	if checkdata.LibraryPropertiesLoadError() != nil {

check/checkfunctions/library_test.go

@@ -38,14 +38,14 @@ func init() {
 	schemasPath = paths.New(workingDirectory, "..", "..", "etc", "schemas")
 }
 
-type checkFunctionTestTable struct {
+type libraryCheckFunctionTestTable struct {
 	testName            string
 	libraryFolderName   string
 	expectedCheckResult checkresult.Type
 	expectedOutputQuery string
 }
 
-func checkCheckFunction(checkFunction Type, testTables []checkFunctionTestTable, t *testing.T) {
+func checkLibraryCheckFunction(checkFunction Type, testTables []libraryCheckFunctionTestTable, t *testing.T) {
 	for _, testTable := range testTables {
 		expectedOutputRegexp := regexp.MustCompile(testTable.expectedOutputQuery)
 
@@ -64,124 +64,124 @@ func checkCheckFunction(checkFunction Type, testTables []checkFunctionTestTable,
 }
 
 func TestLibraryPropertiesNameFieldMissingOfficialPrefix(t *testing.T) {
-	testTables := []checkFunctionTestTable{
+	testTables := []libraryCheckFunctionTestTable{
 		{"Unable to load", "InvalidLibraryProperties", checkresult.NotRun, ""},
 		{"Not defined", "MissingFields", checkresult.NotRun, ""},
 		{"Correct prefix", "Arduino_Official", checkresult.Pass, ""},
 		{"Incorrect prefix", "Recursive", checkresult.Fail, "^Recursive$"},
 	}
 
-	checkCheckFunction(LibraryPropertiesNameFieldMissingOfficialPrefix, testTables, t)
+	checkLibraryCheckFunction(LibraryPropertiesNameFieldMissingOfficialPrefix, testTables, t)
 }
 
 func TestLibraryPropertiesNameFieldDuplicate(t *testing.T) {
-	testTables := []checkFunctionTestTable{
+	testTables := []libraryCheckFunctionTestTable{
 		{"Unable to load", "InvalidLibraryProperties", checkresult.NotRun, ""},
 		{"Duplicate", "Indexed", checkresult.Fail, "^Servo$"},
 		{"Not duplicate", "NotIndexed", checkresult.Pass, ""},
 	}
 
-	checkCheckFunction(LibraryPropertiesNameFieldDuplicate, testTables, t)
+	checkLibraryCheckFunction(LibraryPropertiesNameFieldDuplicate, testTables, t)
 }
 
 func TestLibraryPropertiesNameFieldNotInIndex(t *testing.T) {
-	testTables := []checkFunctionTestTable{
+	testTables := []libraryCheckFunctionTestTable{
 		{"Unable to load", "InvalidLibraryProperties", checkresult.NotRun, ""},
 		{"In index", "Indexed", checkresult.Pass, ""},
 		{"Not in index", "NotIndexed", checkresult.Fail, "^NotIndexed$"},
 	}
 
-	checkCheckFunction(LibraryPropertiesNameFieldNotInIndex, testTables, t)
+	checkLibraryCheckFunction(LibraryPropertiesNameFieldNotInIndex, testTables, t)
 }
 
 func TestLibraryPropertiesNameFieldHeaderMismatch(t *testing.T) {
-	testTables := []checkFunctionTestTable{
+	testTables := []libraryCheckFunctionTestTable{
 		{"Unable to load", "InvalidLibraryProperties", checkresult.NotRun, ""},
 		{"Mismatch", "NameHeaderMismatch", checkresult.Fail, "^NameHeaderMismatch.h$"},
 		{"Match", "Recursive", checkresult.Pass, ""},
 	}
 
-	checkCheckFunction(LibraryPropertiesNameFieldHeaderMismatch, testTables, t)
+	checkLibraryCheckFunction(LibraryPropertiesNameFieldHeaderMismatch, testTables, t)
 }
 
 func TestLibraryPropertiesSentenceFieldSpellCheck(t *testing.T) {
-	testTables := []checkFunctionTestTable{
+	testTables := []libraryCheckFunctionTestTable{
 		{"Unable to load", "InvalidLibraryProperties", checkresult.NotRun, ""},
 		{"Not defined", "MissingFields", checkresult.NotRun, ""},
 		{"Misspelled word", "MisspelledSentenceParagraphValue", checkresult.Fail, "^grill broccoli now$"},
 		{"Correct spelling", "Recursive", checkresult.Pass, ""},
 	}
 
-	checkCheckFunction(LibraryPropertiesSentenceFieldSpellCheck, testTables, t)
+	checkLibraryCheckFunction(LibraryPropertiesSentenceFieldSpellCheck, testTables, t)
 }
 
 func TestLibraryPropertiesParagraphFieldSpellCheck(t *testing.T) {
-	testTables := []checkFunctionTestTable{
+	testTables := []libraryCheckFunctionTestTable{
 		{"Unable to load", "InvalidLibraryProperties", checkresult.NotRun, ""},
 		{"Not defined", "MissingFields", checkresult.NotRun, ""},
 		{"Misspelled word", "MisspelledSentenceParagraphValue", checkresult.Fail, "^There is a zebra$"},
 		{"Correct spelling", "Recursive", checkresult.Pass, ""},
 	}
 
-	checkCheckFunction(LibraryPropertiesParagraphFieldSpellCheck, testTables, t)
+	checkLibraryCheckFunction(LibraryPropertiesParagraphFieldSpellCheck, testTables, t)
 }
 
 func TestLibraryPropertiesParagraphFieldRepeatsSentence(t *testing.T) {
-	testTables := []checkFunctionTestTable{
+	testTables := []libraryCheckFunctionTestTable{
 		{"Unable to load", "InvalidLibraryProperties", checkresult.NotRun, ""},
 		{"Repeat", "ParagraphRepeatsSentence", checkresult.Fail, ""},
 		{"No repeat", "Recursive", checkresult.Pass, ""},
 	}
 
-	checkCheckFunction(LibraryPropertiesParagraphFieldRepeatsSentence, testTables, t)
+	checkLibraryCheckFunction(LibraryPropertiesParagraphFieldRepeatsSentence, testTables, t)
 }
 func TestLibraryPropertiesUrlFieldDeadLink(t *testing.T) {
-	testTables := []checkFunctionTestTable{
+	testTables := []libraryCheckFunctionTestTable{
 		{"Unable to load", "InvalidLibraryProperties", checkresult.NotRun, ""},
 		{"Not defined", "MissingFields", checkresult.NotRun, ""},
 		{"Bad URL", "BadURL", checkresult.Fail, "^Get \"http://invalid/\": dial tcp: lookup invalid:"},
 		{"HTTP error 404", "URL404", checkresult.Fail, "^404 Not Found$"},
 		{"Good URL", "Recursive", checkresult.Pass, ""},
 	}
 
-	checkCheckFunction(LibraryPropertiesUrlFieldDeadLink, testTables, t)
+	checkLibraryCheckFunction(LibraryPropertiesUrlFieldDeadLink, testTables, t)
 }
 
 func TestLibraryPropertiesDependsFieldNotInIndex(t *testing.T) {
-	testTables := []checkFunctionTestTable{
+	testTables := []libraryCheckFunctionTestTable{
 		{"Unable to load", "InvalidLibraryProperties", checkresult.NotRun, ""},
 		{"Dependency not in index", "DependsNotIndexed", checkresult.Fail, "^NotIndexed$"},
 		{"Dependency in index", "DependsIndexed", checkresult.Pass, ""},
 		{"No depends", "NoDepends", checkresult.NotRun, ""},
 	}
 
-	checkCheckFunction(LibraryPropertiesDependsFieldNotInIndex, testTables, t)
+	checkLibraryCheckFunction(LibraryPropertiesDependsFieldNotInIndex, testTables, t)
 }
 
 func TestLibraryPropertiesDotALinkageFieldTrueWithFlatLayout(t *testing.T) {
-	testTables := []checkFunctionTestTable{
+	testTables := []libraryCheckFunctionTestTable{
 		{"Unable to load", "InvalidLibraryProperties", checkresult.NotRun, ""},
 		{"Not defined", "MissingFields", checkresult.NotRun, ""},
 		{"Flat layout", "DotALinkageFlat", checkresult.Fail, ""},
 		{"Recursive layout", "DotALinkage", checkresult.Pass, ""},
 	}
 
-	checkCheckFunction(LibraryPropertiesDotALinkageFieldTrueWithFlatLayout, testTables, t)
+	checkLibraryCheckFunction(LibraryPropertiesDotALinkageFieldTrueWithFlatLayout, testTables, t)
 }
 
 func TestLibraryPropertiesIncludesFieldItemNotFound(t *testing.T) {
-	testTables := []checkFunctionTestTable{
+	testTables := []libraryCheckFunctionTestTable{
 		{"Unable to load", "InvalidLibraryProperties", checkresult.NotRun, ""},
 		{"Not defined", "MissingFields", checkresult.NotRun, ""},
 		{"Missing includes", "MissingIncludes", checkresult.Fail, "^Nonexistent.h$"},
 		{"Present includes", "Recursive", checkresult.Pass, ""},
 	}
 
-	checkCheckFunction(LibraryPropertiesIncludesFieldItemNotFound, testTables, t)
+	checkLibraryCheckFunction(LibraryPropertiesIncludesFieldItemNotFound, testTables, t)
 }
 
 func TestLibraryPropertiesPrecompiledFieldEnabledWithFlatLayout(t *testing.T) {
-	testTables := []checkFunctionTestTable{
+	testTables := []libraryCheckFunctionTestTable{
 		{"Unable to load", "InvalidLibraryProperties", checkresult.NotRun, ""},
 		{"Not defined", "MissingFields", checkresult.NotRun, ""},
 		{"Flat layout", "PrecompiledFlat", checkresult.Fail, "^true$"},
@@ -190,16 +190,16 @@ func TestLibraryPropertiesPrecompiledFieldEnabledWithFlatLayout(t *testing.T) {
 		{"Flat, not precompiled", "NotPrecompiledFlat", checkresult.NotRun, ""},
 	}
 
-	checkCheckFunction(LibraryPropertiesPrecompiledFieldEnabledWithFlatLayout, testTables, t)
+	checkLibraryCheckFunction(LibraryPropertiesPrecompiledFieldEnabledWithFlatLayout, testTables, t)
 }
 
 func TestLibraryHasSubmodule(t *testing.T) {
-	testTables := []checkFunctionTestTable{
+	testTables := []libraryCheckFunctionTestTable{
 		{"Has submodule", "Submodule", checkresult.Fail, ""},
 		{"No submodule", "Recursive", checkresult.Pass, ""},
 	}
 
-	checkCheckFunction(LibraryHasSubmodule, testTables, t)
+	checkLibraryCheckFunction(LibraryHasSubmodule, testTables, t)
 }
 
 func TestLibraryContainsSymlinks(t *testing.T) {
@@ -210,36 +210,44 @@ func TestLibraryContainsSymlinks(t *testing.T) {
 	require.Nil(t, err, "This test must be run as administrator on Windows to have symlink creation privilege.")
 	defer symlinkPath.RemoveAll() // clean up
 
-	testTables := []checkFunctionTestTable{
+	testTables := []libraryCheckFunctionTestTable{
 		{"Has symlink", testLibrary, checkresult.Fail, ""},
 	}
 
-	checkCheckFunction(LibraryContainsSymlinks, testTables, t)
+	checkLibraryCheckFunction(LibraryContainsSymlinks, testTables, t)
 
 	err = symlinkPath.RemoveAll()
 	require.Nil(t, err)
 
-	testTables = []checkFunctionTestTable{
+	testTables = []libraryCheckFunctionTestTable{
 		{"No symlink", testLibrary, checkresult.Pass, ""},
 	}
 
-	checkCheckFunction(LibraryContainsSymlinks, testTables, t)
+	checkLibraryCheckFunction(LibraryContainsSymlinks, testTables, t)
 }
 
 func TestLibraryHasDotDevelopmentFile(t *testing.T) {
-	testTables := []checkFunctionTestTable{
+	testTables := []libraryCheckFunctionTestTable{
 		{"Has .development file", "DotDevelopment", checkresult.Fail, ""},
 		{"No .development file", "Recursive", checkresult.Pass, ""},
 	}
 
-	checkCheckFunction(LibraryHasDotDevelopmentFile, testTables, t)
+	checkLibraryCheckFunction(LibraryHasDotDevelopmentFile, testTables, t)
 }
 
 func TestLibraryHasExe(t *testing.T) {
-	testTables := []checkFunctionTestTable{
+	testTables := []libraryCheckFunctionTestTable{
 		{"Has .exe file", "Exe", checkresult.Fail, ""},
 		{"No .exe files", "Recursive", checkresult.Pass, ""},
 	}
 
-	checkCheckFunction(LibraryHasExe, testTables, t)
+	checkLibraryCheckFunction(LibraryHasExe, testTables, t)
+}
+func TestProhibitedCharactersInLibraryFolderName(t *testing.T) {
+	testTables := []libraryCheckFunctionTestTable{
+		{"Has prohibited characters", "Prohibited CharactersInFolderName", checkresult.Fail, ""},
+		{"No prohibited characters", "Recursive", checkresult.Pass, ""},
+	}
+
+	checkLibraryCheckFunction(ProhibitedCharactersInLibraryFolderName, testTables, t)
 }

check/checkfunctions/sketch.go

@@ -22,8 +22,30 @@ import (
 
 	"github.com/arduino/arduino-check/check/checkdata"
 	"github.com/arduino/arduino-check/check/checkresult"
+	"github.com/arduino/arduino-check/project/sketch"
 )
 
+// ProhibitedCharactersInSketchFileName checks for prohibited characters in the sketch file names.
+func ProhibitedCharactersInSketchFileName() (result checkresult.Type, output string) {
+	directoryListing, _ := checkdata.ProjectPath().ReadDir()
+	directoryListing.FilterOutDirs()
+
+	foundInvalidSketchFileNames := []string{}
+	for _, potentialSketchFile := range directoryListing {
+		if sketch.HasSupportedExtension(potentialSketchFile) {
+			if !validProjectPathBaseName(potentialSketchFile.Base()) {
+				foundInvalidSketchFileNames = append(foundInvalidSketchFileNames, potentialSketchFile.Base())
+			}
+		}
+	}
+
+	if len(foundInvalidSketchFileNames) > 0 {
+		return checkresult.Fail, strings.Join(foundInvalidSketchFileNames, ", ")
+	}
+
+	return checkresult.Pass, ""
+}
+
 // PdeSketchExtension checks for use of deprecated .pde sketch file extensions.
 func PdeSketchExtension() (result checkresult.Type, output string) {
 	directoryListing, _ := checkdata.ProjectPath().ReadDir()

check/checkfunctions/sketch_test.go

@@ -0,0 +1,70 @@
+// This file is part of arduino-check.
+//
+// Copyright 2020 ARDUINO SA (http://www.arduino.cc/)
+//
+// This software is released under the GNU General Public License version 3,
+// which covers the main part of arduino-check.
+// The terms of this license can be found at:
+// https://www.gnu.org/licenses/gpl-3.0.en.html
+//
+// You can be released from the requirements of the above licenses by purchasing
+// a commercial license. Buying such a license is mandatory if you want to
+// modify or otherwise use the software for commercial activities involving the
+// Arduino software without disclosing the source code of your own applications.
+// To purchase a commercial license, send an email to license@arduino.cc.
+
+package checkfunctions
+
+import (
+	"os"
+	"regexp"
+	"testing"
+
+	"github.com/arduino/arduino-check/check/checkdata"
+	"github.com/arduino/arduino-check/check/checkresult"
+	"github.com/arduino/arduino-check/project"
+	"github.com/arduino/arduino-check/project/projecttype"
+	"github.com/arduino/go-paths-helper"
+	"github.com/stretchr/testify/assert"
+)
+
+var sketchesTestDataPath *paths.Path
+
+func init() {
+	workingDirectory, _ := os.Getwd()
+	sketchesTestDataPath = paths.New(workingDirectory, "testdata", "sketches")
+}
+
+type sketchCheckFunctionTestTable struct {
+	testName            string
+	sketchFolderName    string
+	expectedCheckResult checkresult.Type
+	expectedOutputQuery string
+}
+
+func checkSketchCheckFunction(checkFunction Type, testTables []sketchCheckFunctionTestTable, t *testing.T) {
+	for _, testTable := range testTables {
+		expectedOutputRegexp := regexp.MustCompile(testTable.expectedOutputQuery)
+
+		testProject := project.Type{
+			Path:             sketchesTestDataPath.Join(testTable.sketchFolderName),
+			ProjectType:      projecttype.Sketch,
+			SuperprojectType: projecttype.Sketch,
+		}
+
+		checkdata.Initialize(testProject, schemasPath)
+
+		result, output := checkFunction()
+		assert.Equal(t, testTable.expectedCheckResult, result, testTable.testName)
+		assert.True(t, expectedOutputRegexp.MatchString(output), testTable.testName)
+	}
+}
+
+func TestProhibitedCharactersInSketchFileName(t *testing.T) {
+	testTables := []sketchCheckFunctionTestTable{
+		{"Has prohibited characters", "ProhibitedCharactersInFileName", checkresult.Fail, "^Prohibited CharactersInFileName.h$"},
+		{"No prohibited characters", "Valid", checkresult.Pass, ""},
+	}
+
+	checkSketchCheckFunction(ProhibitedCharactersInSketchFileName, testTables, t)
+}

check/checkfunctions/testdata/libraries/Prohibited CharactersInFolderName/library.properties

@@ -0,0 +1,9 @@
+name=Prohibited CharactersInFolderName
+version=1.0.0
+author=Cristian Maglie <c.maglie@example.com>, Pippo Pluto <pippo@example.com>
+maintainer=Cristian Maglie <c.maglie@example.com>
+sentence=A library that makes coding a web server a breeze.
+paragraph=Supports HTTP1.1 and you can do GET and POST.
+category=Communication
+url=http://example.com/
+architectures=avr

check/checkfunctions/testdata/libraries/Prohibited CharactersInFolderName/src/ProhibitedCharactersInFolderName.h

@@ -0,0 +1,2 @@
+void setup() {}
+void loop() {}

check/checkfunctions/testdata/sketches/ProhibitedCharactersInFileName/Prohibited CharactersInFileName.h

@@ -0,0 +1,2 @@
+void setup() {}
+void loop() {}

check/checkfunctions/testdata/sketches/ProhibitedCharactersInFileName/ProhibitedCharactersInFileName.ino

@@ -30,3 +30,9 @@ func HasMainFileValidExtension(filePath *paths.Path) bool {
 	_, hasMainFileValidExtension := globals.MainFileValidExtensions[filePath.Ext()]
 	return hasMainFileValidExtension
 }
+
+// HasSupportedExtension returns whether the file at the given path has any of the file extensions supported for source/header files of a sketch.
+func HasSupportedExtension(filePath *paths.Path) bool {
+	_, hasAdditionalFileValidExtensions := globals.AdditionalFileValidExtensions[filePath.Ext()]
+	return hasAdditionalFileValidExtensions || HasMainFileValidExtension(filePath)
+}

check/checkfunctions/testdata/sketches/Valid/Valid.ino

@@ -0,0 +1,34 @@
+// This file is part of arduino-check.
+//
+// Copyright 2020 ARDUINO SA (http://www.arduino.cc/)
+//
+// This software is released under the GNU General Public License version 3,
+// which covers the main part of arduino-check.
+// The terms of this license can be found at:
+// https://www.gnu.org/licenses/gpl-3.0.en.html
+//
+// You can be released from the requirements of the above licenses by purchasing
+// a commercial license. Buying such a license is mandatory if you want to
+// modify or otherwise use the software for commercial activities involving the
+// Arduino software without disclosing the source code of your own applications.
+// To purchase a commercial license, send an email to license@arduino.cc.
+
+package sketch
+
+import (
+	"testing"
+
+	"github.com/arduino/go-paths-helper"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestHasMainFileValidExtension(t *testing.T) {
+	assert.True(t, HasMainFileValidExtension(paths.New("/foo/bar.ino")))
+	assert.False(t, HasMainFileValidExtension(paths.New("/foo/bar.h")))
+}
+
+func TestHasSupportedExtension(t *testing.T) {
+	assert.True(t, HasSupportedExtension(paths.New("/foo/bar.ino")))
+	assert.True(t, HasSupportedExtension(paths.New("/foo/bar.h")))
+	assert.False(t, HasSupportedExtension(paths.New("/foo/bar.baz")))
+}