From 9b7adb9101af9fdd9a667537b10df9b184751f37 Mon Sep 17 00:00:00 2001 From: Om Date: Mon, 7 Sep 2026 01:32:53 +0530 Subject: [PATCH 1/3] Report a malformed config instead of panicking in pipectl migrate migrateApplicationConfig unmarshals a user's YAML into map[string]any and then asserts on three lookups without the comma-ok: spec, spec.pipeline.stages and kind. Any of them holding another type, or being absent, panics the CLI with an interface conversion instead of telling the user which field is wrong. Six configs a user could plausibly write all panic today, including a file with no kind at all. The same function already uses the checked form two lines further down for stages[] and stages[].with, so this is an omission rather than a convention. Return an error naming the field and the type found. Signed-off-by: Om --- .../pipectl/cmd/migrate/application_config.go | 29 +++++++++++-- .../cmd/migrate/malformed_config_test.go | 42 +++++++++++++++++++ 2 files changed, 68 insertions(+), 3 deletions(-) create mode 100644 pkg/app/pipectl/cmd/migrate/malformed_config_test.go diff --git a/pkg/app/pipectl/cmd/migrate/application_config.go b/pkg/app/pipectl/cmd/migrate/application_config.go index b96d237e1c..5aa67c6201 100644 --- a/pkg/app/pipectl/cmd/migrate/application_config.go +++ b/pkg/app/pipectl/cmd/migrate/application_config.go @@ -139,7 +139,12 @@ func (c *applicationConfig) migrateApplicationConfig(_ context.Context, configFi "driftDetection", } - oldSpec := cfg["spec"].(map[string]any) + oldSpec, ok := cfg["spec"].(map[string]any) + if !ok { + err := fmt.Errorf("spec must be a mapping, got %T", cfg["spec"]) + logger.Error("invalid application config", zap.String("config-file", configFile), zap.Error(err)) + return err + } for _, key := range keys { if _, ok := oldSpec[key]; ok { spec[key] = oldSpec[key] @@ -151,7 +156,19 @@ func (c *applicationConfig) migrateApplicationConfig(_ context.Context, configFi if oldPipelineCfg, ok := oldSpec["pipeline"]; ok { pipelineCfg := make(map[string][]any) - for _, oldStage := range oldPipelineCfg.(map[string]any)["stages"].([]any) { + oldPipeline, ok := oldPipelineCfg.(map[string]any) + if !ok { + err := fmt.Errorf("spec.pipeline must be a mapping, got %T", oldPipelineCfg) + logger.Error("invalid application config", zap.String("config-file", configFile), zap.Error(err)) + return err + } + oldStages, ok := oldPipeline["stages"].([]any) + if !ok && oldPipeline["stages"] != nil { + err := fmt.Errorf("spec.pipeline.stages must be a list, got %T", oldPipeline["stages"]) + logger.Error("invalid application config", zap.String("config-file", configFile), zap.Error(err)) + return err + } + for _, oldStage := range oldStages { if oldStageCfg, ok := oldStage.(map[string]any); ok { // Check if the stage is the analysis stage to determine if we need to fill plugins.analysis config if oldStageCfg["name"] == string(model.StageAnalysis) { @@ -177,7 +194,13 @@ func (c *applicationConfig) migrateApplicationConfig(_ context.Context, configFi spec["pipeline"] = pipelineCfg } - switch config.Kind(cfg["kind"].(string)) { + kind, ok := cfg["kind"].(string) + if !ok { + err := fmt.Errorf("kind must be a string, got %T", cfg["kind"]) + logger.Error("invalid application config", zap.String("config-file", configFile), zap.Error(err)) + return err + } + switch config.Kind(kind) { case config.KindKubernetesApp: logger.Info("migrating kubernetes application config", zap.String("config-file", configFile)) keys := []string{ diff --git a/pkg/app/pipectl/cmd/migrate/malformed_config_test.go b/pkg/app/pipectl/cmd/migrate/malformed_config_test.go new file mode 100644 index 0000000000..7ff721f9e1 --- /dev/null +++ b/pkg/app/pipectl/cmd/migrate/malformed_config_test.go @@ -0,0 +1,42 @@ +package migrate + +import ( + "context" + "os" + "path/filepath" + "testing" + + "go.uber.org/zap" +) + +// migrateApplicationConfig reads a user-supplied YAML file into map[string]any, +// so every lookup out of it can hold something other than the expected shape. +// A malformed file has to be reported, not panic the CLI. +func TestMigrateApplicationConfigMalformed(t *testing.T) { + cases := map[string]string{ + "spec is a string": "apiVersion: pipecd.dev/v1beta1\nkind: KubernetesApp\nspec: oops\n", + "spec missing": "apiVersion: pipecd.dev/v1beta1\nkind: KubernetesApp\n", + "kind missing": "apiVersion: pipecd.dev/v1beta1\nspec:\n name: x\n", + "kind is a number": "apiVersion: pipecd.dev/v1beta1\nkind: 42\nspec:\n name: x\n", + "pipeline is a list": "apiVersion: pipecd.dev/v1beta1\nkind: KubernetesApp\nspec:\n pipeline:\n - a\n", + "stages is a string": "apiVersion: pipecd.dev/v1beta1\nkind: KubernetesApp\nspec:\n pipeline:\n stages: nope\n", + } + for name, body := range cases { + t.Run(name, func(t *testing.T) { + dir := t.TempDir() + f := filepath.Join(dir, "app.pipecd.yaml") + if err := os.WriteFile(f, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + c := &applicationConfig{} + defer func() { + if r := recover(); r != nil { + t.Fatalf("panicked on a user-supplied config: %v", r) + } + }() + if err := c.migrateApplicationConfig(context.Background(), f, zap.NewNop()); err == nil { + t.Error("expected an error for a malformed config, got nil") + } + }) + } +} From b55a7748d9c984b7cb984ae51deb1bab14173779 Mon Sep 17 00:00:00 2001 From: Om Date: Mon, 7 Sep 2026 01:42:32 +0530 Subject: [PATCH 2/3] Reuse the checked kind in the unsupported-kind branch The default branch re-asserted cfg["kind"].(string) after the switch had already been entered on a checked value, so the same lookup was asserted twice. Use the string that was already validated. Signed-off-by: Om --- pkg/app/pipectl/cmd/migrate/application_config.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/app/pipectl/cmd/migrate/application_config.go b/pkg/app/pipectl/cmd/migrate/application_config.go index 5aa67c6201..a14c585ac2 100644 --- a/pkg/app/pipectl/cmd/migrate/application_config.go +++ b/pkg/app/pipectl/cmd/migrate/application_config.go @@ -295,8 +295,8 @@ func (c *applicationConfig) migrateApplicationConfig(_ context.Context, configFi } spec["plugins"] = pluginCfg default: - logger.Error("unsupported application kind", zap.String("config-file", configFile), zap.String("kind", cfg["kind"].(string))) - return fmt.Errorf("unsupported application kind: %s", cfg["kind"]) + logger.Error("unsupported application kind", zap.String("config-file", configFile), zap.String("kind", kind)) + return fmt.Errorf("unsupported application kind: %s", kind) } migrated["spec"] = spec From b946aef0a0f2e6d0e0a100b66c517f46f0d22574 Mon Sep 17 00:00:00 2001 From: Om Date: Mon, 7 Sep 2026 19:50:42 +0530 Subject: [PATCH 3/3] Add the license header to the new test file Signed-off-by: Om --- .../pipectl/cmd/migrate/malformed_config_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/pkg/app/pipectl/cmd/migrate/malformed_config_test.go b/pkg/app/pipectl/cmd/migrate/malformed_config_test.go index 7ff721f9e1..842b8c96cb 100644 --- a/pkg/app/pipectl/cmd/migrate/malformed_config_test.go +++ b/pkg/app/pipectl/cmd/migrate/malformed_config_test.go @@ -1,3 +1,17 @@ +// Copyright 2024 The PipeCD Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package migrate import (