diff --git a/internal/orchestrator/app/app.go b/internal/orchestrator/app/app.go index 5c29fad7a..dfa175edc 100644 --- a/internal/orchestrator/app/app.go +++ b/internal/orchestrator/app/app.go @@ -65,51 +65,88 @@ func Load(appPath *paths.Path) (ArduinoApp, error) { return ArduinoApp{}, fmt.Errorf("cannot get absolute path for app: %w", err) } + if err := ValidateApp(appPath); err != nil { + return ArduinoApp{}, err + } + app := ArduinoApp{ FullPath: appPath, Descriptor: AppDescriptor{}, } - if descriptorFile := app.GetDescriptorPath(); descriptorFile.Exist() { - desc, err := ParseDescriptorFile(descriptorFile) + desc, err := ParseDescriptorFile(app.GetDescriptorPath()) + if err != nil { + return ArduinoApp{}, err + } + app.Descriptor = desc + app.Name = desc.Name + + if app.Descriptor.Description == "" { + description, err := app.getAppDescriptionFromReadme() if err != nil { - return ArduinoApp{}, fmt.Errorf("error loading app descriptor file: %w", err) - } - app.Descriptor = desc - app.Name = desc.Name - - if app.Descriptor.Description == "" { - description, err := app.getAppDescriptionFromReadme() - if err != nil { - // Log the error but don't fail the loading process, as the description is optional - slog.Warn("cannot extract app description from README.md", "error", err) - } else { - app.Descriptor.Description = description - } + slog.Warn("cannot extract app description from README.md", "error", err) + } else { + app.Descriptor.Description = description } + } - } else { - return ArduinoApp{}, errors.New("descriptor app.yaml file missing from app") + app.MainPythonFile = appPath.Join("python", "main.py") + + sketchPath := appPath.Join("sketch") + if sketchPath.IsDir() { + app.mainSketchPath = sketchPath } - if appPath.Join("python", "main.py").Exist() { - app.MainPythonFile = appPath.Join("python", "main.py") + if appPath.Join("bricks").Exist() { + app.LocalBricks = loadBricksFromFolder(appPath.Join("bricks")) } - if appPath.Join("sketch", "sketch.ino").Exist() { - // TODO: check sketch casing? - app.mainSketchPath = appPath.Join("sketch") + return app, nil +} + +func ValidateApp(appPath *paths.Path) error { + descriptorFile := appPath.Join("app.yaml") + if _, err := validateAndParseDescriptor(descriptorFile); err != nil { + return err } - if app.MainPythonFile == nil && app.mainSketchPath == nil { - return ArduinoApp{}, errors.New("main python file and sketch file missing from app") + sketchPath := appPath.Join("sketch") + if err := isValidSketchFolder(sketchPath); err != nil { + return err } - if appPath.Join("bricks").Exist() { - app.LocalBricks = loadBricksFromFolder(appPath.Join("bricks")) + if !appPath.Join("python", "main.py").Exist() { + return errors.New("main python file missing from app") } - return app, nil + return nil +} + +func validateAndParseDescriptor(descriptorFile *paths.Path) (AppDescriptor, error) { + if !descriptorFile.Exist() { + return AppDescriptor{}, errors.New("descriptor app.yaml file missing from app") + } + appDescriptor, err := ParseDescriptorFile(descriptorFile) + if err != nil { + return AppDescriptor{}, fmt.Errorf("error loading app descriptor file: %w", err) + } + return appDescriptor, nil +} + +func isValidSketchFolder(sketchDir *paths.Path) error { + if sketchDir == nil { + return nil + } + + sketchIno := sketchDir.Join("sketch.ino") + sketchYaml := sketchDir.Join("sketch.yaml") + + if sketchIno.Exist() || sketchYaml.Exist() { + if !sketchIno.Exist() || !sketchYaml.Exist() { + return errors.New("sketch folder is incomplete: both sketch.ino and sketch.yaml are required") + } + } + return nil } func (a *ArduinoApp) GetSketchPath() (*paths.Path, bool) { @@ -119,15 +156,9 @@ func (a *ArduinoApp) GetSketchPath() (*paths.Path, bool) { return a.mainSketchPath, true } -// GetDescriptorPath returns the path to the app descriptor file (app.yaml or app.yml) +// GetDescriptorPath returns the path to the app descriptor file (app.yaml) func (a *ArduinoApp) GetDescriptorPath() *paths.Path { descriptorFile := a.FullPath.Join("app.yaml") - if !descriptorFile.Exist() { - alternateDescriptorFile := a.FullPath.Join("app.yml") - if alternateDescriptorFile.Exist() { - return alternateDescriptorFile - } - } return descriptorFile } diff --git a/internal/orchestrator/app/app_test.go b/internal/orchestrator/app/app_test.go index 41d67f9a7..53581538c 100644 --- a/internal/orchestrator/app/app_test.go +++ b/internal/orchestrator/app/app_test.go @@ -122,12 +122,12 @@ func TestMissingDescriptor(t *testing.T) { } func TestMissingMains(t *testing.T) { - appFolderPath := paths.New("testdata", "MissingMains") + appFolderPath := paths.New("testdata", "MissingMain") // Load app app, err := Load(appFolderPath) assert.Error(t, err) - assert.ErrorContains(t, err, "main python file and sketch file missing from app") + assert.ErrorContains(t, err, "main python file missing from app") assert.Empty(t, app) } diff --git a/internal/orchestrator/app/find.go b/internal/orchestrator/app/find.go index 252152177..6928543e7 100644 --- a/internal/orchestrator/app/find.go +++ b/internal/orchestrator/app/find.go @@ -42,9 +42,9 @@ func FindAppsInFolder(pathToExplore *paths.Path) (paths.PathList, error) { const tmpAppPrefix = ".tmp_" // DirHasAppDescriptor returns true if the given directory contains -// an app descriptor file (app.yaml or app.yml). +// an app descriptor file (app.yaml). func DirHasAppDescriptor(p *paths.Path) bool { - return p.Join("app.yaml").Exist() || p.Join("app.yml").Exist() + return p.Join("app.yaml").Exist() } // IsTmpAppDir returns true if the app path is a temporary app diff --git a/internal/orchestrator/app/parser.go b/internal/orchestrator/app/parser.go index 722ccf105..90a5fcb76 100644 --- a/internal/orchestrator/app/parser.go +++ b/internal/orchestrator/app/parser.go @@ -21,6 +21,7 @@ import ( "errors" "fmt" "io" + "regexp" emoji "github.com/Andrew-M-C/go.emoji" "github.com/arduino/go-paths-helper" @@ -126,10 +127,6 @@ func ParseDescriptorFile(file *paths.Path) (AppDescriptor, error) { return AppDescriptor{}, fmt.Errorf("cannot decode descriptor: %w", err) } - if descriptor.Name == "" { - return AppDescriptor{}, fmt.Errorf("application name is empty") - } - return descriptor, descriptor.IsValid() } @@ -140,6 +137,12 @@ func (a *AppDescriptor) IsValid() error { allErrors = errors.Join(allErrors, fmt.Errorf("icon %q is not a valid single emoji", a.Icon)) } } + if a.Name != "" { + if !isValidName(a.Name) { + allErrors = errors.Join(allErrors, fmt.Errorf("name %q is not valid", a.Name)) + } + } + return allErrors } @@ -157,3 +160,8 @@ func isSingleEmoji(s string) bool { } return emojis == 1 } + +func isValidName(s string) bool { + matched, _ := regexp.MatchString(`^[a-zA-Z0-9][a-zA-Z0-9_ -]*$`, s) + return matched +} diff --git a/internal/orchestrator/app/parser_test.go b/internal/orchestrator/app/parser_test.go index 9e13eb7dc..d50415386 100644 --- a/internal/orchestrator/app/parser_test.go +++ b/internal/orchestrator/app/parser_test.go @@ -144,3 +144,32 @@ bricks: require.Equal(t, 1, len(desc.Bricks)) require.Equal(t, []string{"my-dev-1", "my-dev-2"}, desc.Bricks[0].Devices) } + +func TestIsValidName(t *testing.T) { + tests := []struct { + input string + expected bool + }{ + {"ValidName", true}, + {"valid_name", true}, + {"valid-name", true}, + {"valid name", true}, + {"Valid Name With Spaces", true}, + {"a", true}, + {"A1", true}, + {"Test App", true}, + {"Image detection with UI", true}, + {"-invalid", false}, + {"", false}, + {"_invalid", false}, + {"name!", false}, + {"name@invalid", false}, + } + + for _, test := range tests { + t.Run(test.input, func(t *testing.T) { + result := isValidName(test.input) + require.Equal(t, test.expected, result, "Input: %s", test.input) + }) + } +} diff --git a/internal/orchestrator/app/testdata/MissingMains/python/other.py b/internal/orchestrator/app/testdata/AppSimple/sketch/sketch.yaml similarity index 100% rename from internal/orchestrator/app/testdata/MissingMains/python/other.py rename to internal/orchestrator/app/testdata/AppSimple/sketch/sketch.yaml diff --git a/internal/orchestrator/app/testdata/MissingMains/sketch/foo.cpp b/internal/orchestrator/app/testdata/AppSimpleNoDescription/sketch/sketch.yaml similarity index 100% rename from internal/orchestrator/app/testdata/MissingMains/sketch/foo.cpp rename to internal/orchestrator/app/testdata/AppSimpleNoDescription/sketch/sketch.yaml diff --git a/internal/orchestrator/app/testdata/AppWithLocalBricks/sketch/sketch.yaml b/internal/orchestrator/app/testdata/AppWithLocalBricks/sketch/sketch.yaml new file mode 100644 index 000000000..e69de29bb diff --git a/internal/orchestrator/app/testdata/MissingMains/app.yaml b/internal/orchestrator/app/testdata/MissingMain/app.yaml similarity index 100% rename from internal/orchestrator/app/testdata/MissingMains/app.yaml rename to internal/orchestrator/app/testdata/MissingMain/app.yaml diff --git a/internal/orchestrator/app/testdata/MissingMain/python/other.py b/internal/orchestrator/app/testdata/MissingMain/python/other.py new file mode 100644 index 000000000..e69de29bb diff --git a/internal/orchestrator/app/testdata/MissingMain/sketch/foo.cpp b/internal/orchestrator/app/testdata/MissingMain/sketch/foo.cpp new file mode 100644 index 000000000..e69de29bb diff --git a/internal/orchestrator/archive.go b/internal/orchestrator/archive.go index bf3cfa76b..711dd77f7 100644 --- a/internal/orchestrator/archive.go +++ b/internal/orchestrator/archive.go @@ -112,7 +112,7 @@ func zipAppToBuffer(bricksIndex *bricksindex.BricksIndex, sourcePath string, roo return nil } - if d.Name() == "app.yaml" || d.Name() == "app.yml" { // nolint:goconst + if d.Name() == "app.yaml" { // nolint:goconst desc, err := app.ParseDescriptorFile(paths.New(path)) if err != nil { return err @@ -169,10 +169,6 @@ func ImportAppFromZip( rawAppName = strings.TrimSuffix(originalZipName, filepath.Ext(originalZipName)) } - if err := validateAppZipContent(&r.Reader, rootPrefix); err != nil { - return app.ID{}, fmt.Errorf("%w:%v", ErrBadRequest, err) - } - appDescriptor, err := readAppDescriptorFromZip(&r.Reader, rootPrefix) if err != nil { return app.ID{}, fmt.Errorf("failed to read app.yaml: %w", err) @@ -202,6 +198,9 @@ func ImportAppFromZip( if finalDestPath.Exist() { return app.ID{}, ErrAppAlreadyExists } + if err := app.ValidateApp(tempDestDir); err != nil { + return app.ID{}, fmt.Errorf("%w: %v", ErrBadRequest, err) + } if err := tempDestDir.Rename(finalDestPath); err != nil { return app.ID{}, fmt.Errorf("failed to finalize app import (swap): %w", err) @@ -287,12 +286,11 @@ func readAppDescriptorFromZip(r *zip.Reader, rootPrefix string) (app.AppDescript var descriptor app.AppDescriptor targetAppYaml := paths.New(rootPrefix, "app.yaml") - targetAppYml := paths.New(rootPrefix, "app.yml") for _, f := range r.File { name := filepath.ToSlash(f.Name) - if name == targetAppYaml.String() || name == targetAppYml.String() { + if name == targetAppYaml.String() { rc, err := f.Open() if err != nil { return descriptor, err @@ -311,62 +309,6 @@ func readAppDescriptorFromZip(r *zip.Reader, rootPrefix string) (app.AppDescript return descriptor, fmt.Errorf("app.yaml not found in archive") } -// TODO implement centralized app validator to use everywhere is needed -// validateAppZipContent checks for mandatory files respecting the rootPrefix -func validateAppZipContent(r *zip.Reader, rootPrefix string) error { - hasAppYaml := false - hasMainPy := false - - hasSketchFolder := false - hasSketchIno := false - hasSketchYaml := false - - targetAppYaml := paths.New(rootPrefix, "app.yaml") - targetAppYml := paths.New(rootPrefix, "app.yml") - targetMainPy := paths.New(rootPrefix, "python/main.py") - - targetSketchPrefix := paths.New(rootPrefix, "sketch").String() + "/" - for _, f := range r.File { - name := filepath.ToSlash(f.Name) - - if name == targetAppYaml.String() || name == targetAppYml.String() { - hasAppYaml = true - } - if name == targetMainPy.String() { - hasMainPy = true - } - - if strings.HasPrefix(name, targetSketchPrefix) { - hasSketchFolder = true - if name == paths.New(rootPrefix, "sketch/sketch.ino").String() { - hasSketchIno = true - } - - if name == paths.New(rootPrefix, "sketch/sketch.yaml").String() { - hasSketchYaml = true - } - } - } - - if !hasAppYaml { - return errors.New("missing app.yaml") - } - if !hasMainPy { - return errors.New("missing python/main.py") - } - - if hasSketchFolder { - if !hasSketchIno { - return errors.New("sketch folder present but missing .ino file") - } - if !hasSketchYaml { - return errors.New("sketch folder present but missing .yaml file") - } - } - - return nil -} - func redactSecrets(bricksindex *bricksindex.BricksIndex, desc *app.AppDescriptor) { for i := range desc.Bricks { brick := &desc.Bricks[i] @@ -392,7 +334,7 @@ func redactSecrets(bricksindex *bricksindex.BricksIndex, desc *app.AppDescriptor func findZipRoot(r *zip.Reader) (string, error) { for _, f := range r.File { name := filepath.ToSlash(f.Name) - if filepath.Base(name) != "app.yaml" && filepath.Base(name) != "app.yml" { + if filepath.Base(name) != "app.yaml" { continue } slashCount := strings.Count(name, "/") diff --git a/internal/orchestrator/archive_test.go b/internal/orchestrator/archive_test.go index 76505f789..43302af56 100644 --- a/internal/orchestrator/archive_test.go +++ b/internal/orchestrator/archive_test.go @@ -153,166 +153,6 @@ func TestExportAppZip(t *testing.T) { }) } } -func TestValidateAppZipContent(t *testing.T) { - tests := []struct { - name string - files map[string]string - rootPrefix string - wantErr bool - errContains string - }{ - { - name: "Valid standard app (Flat Root)", - files: map[string]string{ - "app.yaml": "", - "python/main.py": "", - }, - rootPrefix: "", - wantErr: false, - }, - { - - name: "Valid nested app (With Root Folder)", - files: map[string]string{ - "my-app/app.yaml": "", - "my-app/python/main.py": "", - }, - rootPrefix: "my-app", - wantErr: false, - }, - { - name: "Valid app with yaml variant (.yml)", - files: map[string]string{ - "app.yml": "", - "python/main.py": "", - }, - rootPrefix: "", - wantErr: false, - }, - { - name: "Valid app with full sketch folder", - files: map[string]string{ - "app.yaml": "", - "python/main.py": "", - "sketch/sketch.ino": "", - "sketch/sketch.yaml": "", - }, - rootPrefix: "", - wantErr: false, - }, - { - name: "Valid Windows paths (Backslash handling)", - files: map[string]string{ - "app.yaml": "", - "python/main.py": "", - "sketch\\sketch.ino": "", - "sketch\\sketch.yaml": "", - }, - rootPrefix: "", - wantErr: false, - }, - { - name: "Ignore unrelated folders with similar prefix", - files: map[string]string{ - "app.yaml": "", - "python/main.py": "", - "sketch_backup/main.cpp": "", - }, - rootPrefix: "", - wantErr: false, - }, - { - name: "Missing app.yaml", - files: map[string]string{ - "python/main.py": "", - }, - rootPrefix: "", - wantErr: true, - errContains: "missing app.yaml", - }, - { - name: "Missing python/main.py", - files: map[string]string{ - "app.yaml": "", - }, - rootPrefix: "", - wantErr: true, - errContains: "missing python/main.py", - }, - { - name: "Sketch folder present but missing .ino", - files: map[string]string{ - "app.yaml": "", - "python/main.py": "", - "sketch/readme.txt": "", - "sketch/sketch.yaml": "", - }, - rootPrefix: "", - wantErr: true, - errContains: "missing .ino file", - }, - { - name: "Sketch folder present but missing .yaml", - files: map[string]string{ - "app.yaml": "", - "python/main.py": "", - "sketch/sketch.ino": "", - }, - rootPrefix: "", - wantErr: true, - errContains: "missing .yaml file", - }, - { - name: "Nested App missing main.py (Check relative path logic)", - files: map[string]string{ - "cool-app/app.yaml": "", - }, - rootPrefix: "cool-app", - wantErr: true, - errContains: "missing python/main.py", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - r := createMockZip(t, tt.files) - - gotErr := validateAppZipContent(r, tt.rootPrefix) - - if tt.wantErr { - require.Error(t, gotErr) - require.Contains(t, gotErr.Error(), tt.errContains, "Error message mismatch") - } else { - require.NoError(t, gotErr, "Expected success but got an error") - } - }) - } -} - -func createMockZip(t *testing.T, files map[string]string) *zip.Reader { - buf := new(bytes.Buffer) - w := zip.NewWriter(buf) - - for name, content := range files { - f, err := w.Create(name) - if err != nil { - t.Fatal(err) - } - _, err = f.Write([]byte(content)) - if err != nil { - t.Fatal(err) - } - } - if err := w.Close(); err != nil { - t.Fatal(err) - } - - r, err := zip.NewReader(bytes.NewReader(buf.Bytes()), int64(buf.Len())) - if err != nil { - t.Fatal(err) - } - return r -} func TestImportAppFromZip(t *testing.T) { type testCase struct { @@ -372,7 +212,7 @@ func TestImportAppFromZip(t *testing.T) { "app.yaml": "name: test", }, wantErr: true, - errorContains: "missing python/main.py", + errorContains: "main python file missing from app", }, { name: "Error - Zip Slip Attack", @@ -385,6 +225,47 @@ func TestImportAppFromZip(t *testing.T) { wantErr: true, errorContains: "illegal file path", }, + { + name: "Error - Invalid app name", + originalZipName: "test.zip", + zipFiles: map[string]string{ + "app.yaml": "name: -invalid-name", + "python/main.py": "pass", + }, + wantErr: true, + errorContains: "is not valid", + }, + { + name: "Error - Sketch folder missing .ino", + originalZipName: "test.zip", + zipFiles: map[string]string{ + "app.yaml": "name: test", + "python/main.py": "pass", + "sketch/sketch.yaml": "", + }, + wantErr: true, + errorContains: "both sketch.ino and sketch.yaml are required", + }, + { + name: "Error - Sketch folder missing .yaml", + originalZipName: "test.zip", + zipFiles: map[string]string{ + "app.yaml": "name: test", + "python/main.py": "pass", + "sketch/sketch.ino": "", + }, + wantErr: true, + errorContains: "both sketch.ino and sketch.yaml are required", + }, + { + name: "Error - Nested app missing main.py", + originalZipName: "test.zip", + zipFiles: map[string]string{ + "cool-app/app.yaml": "name: test", + }, + wantErr: true, + errorContains: "main python file missing from app", + }, } for _, tc := range tests { @@ -481,12 +362,6 @@ func TestFindZipRoot(t *testing.T) { wantRoot: "", wantErr: false, }, - { - name: "No root folder (with .yml)", - files: []string{"app.yml", "python/main.py"}, - wantRoot: "", - wantErr: false, - }, { name: "Nested root folder", files: []string{"my-app/app.yaml", "my-app/python/main.py"}, @@ -495,13 +370,13 @@ func TestFindZipRoot(t *testing.T) { }, { name: "Deep Nested folder", - files: []string{"deep/nested/app.yml"}, + files: []string{"deep/nested/app.yaml"}, wantRoot: "deep/nested", wantErr: true, }, { name: "Invalid: Very deep nested folder", - files: []string{"deep/nested/folder/app.yml"}, + files: []string{"deep/nested/folder/app.yaml"}, wantRoot: "", wantErr: true, }, diff --git a/internal/orchestrator/orchestrator.go b/internal/orchestrator/orchestrator.go index 63d5cc31c..4d9609d53 100644 --- a/internal/orchestrator/orchestrator.go +++ b/internal/orchestrator/orchestrator.go @@ -786,7 +786,7 @@ func CloneApp( if !originPath.Exist() { return CloneAppResponse{}, ErrAppDoesntExists } - if !originPath.Join("app.yaml").Exist() && !originPath.Join("app.yml").Exist() { + if !originPath.Join("app.yaml").Exist() { return CloneAppResponse{}, app.ErrInvalidApp } @@ -833,11 +833,10 @@ func CloneApp( } if (req.Name != nil && *req.Name != "") || (req.Icon != nil && *req.Icon != "") { - var appYamlPath *paths.Path - if dstPath.Join("app.yaml").Exist() { - appYamlPath = dstPath.Join("app.yaml") - } else { - appYamlPath = dstPath.Join("app.yml") + + appYamlPath := dstPath.Join("app.yaml") + if !appYamlPath.Exist() { + return CloneAppResponse{}, fmt.Errorf("app.yaml not found in cloned app") } descriptor, err := app.ParseDescriptorFile(appYamlPath) if err != nil { @@ -945,18 +944,7 @@ func EditApp( if req.Name != nil { editApp.Descriptor.Name = *req.Name - newPath := editApp.FullPath.Parent().Join(slug.Make(*req.Name)) - if newPath.Exist() { - return ErrAppAlreadyExists - } - if err := editApp.FullPath.Rename(newPath); err != nil { - editErr = fmt.Errorf("failed to rename app path: %w", err) - return editErr - } - editApp.FullPath = newPath - editApp.Name = editApp.Descriptor.Name } - if req.Icon != nil { editApp.Descriptor.Icon = *req.Icon } @@ -967,11 +955,20 @@ func EditApp( if err := editApp.Descriptor.IsValid(); err != nil { return fmt.Errorf("%w: %w", app.ErrInvalidApp, err) } - err := editApp.Save() - if err != nil { - return fmt.Errorf("failed to save app: %w", err) + + if req.Name != nil { + newPath := editApp.FullPath.Parent().Join(slug.Make(*req.Name)) + if newPath.Exist() { + return ErrAppAlreadyExists + } + if err := editApp.FullPath.Rename(newPath); err != nil { + return fmt.Errorf("failed to rename app path: %w", err) + } + editApp.FullPath = newPath + editApp.Name = editApp.Descriptor.Name } - return nil + + return editApp.Save() } func editAppDefaults(userApp *app.ArduinoApp, isDefault bool, cfg config.Configuration) error {