Fix unchecked type assertion panic in ChangeImageNameAction (#10574)

* Fix unchecked type assertion panic in ChangeImageNameAction

replaceImageName reads a restored container's image field out of the
unstructured object and asserts it to string without checking ok. The
comma-ok map lookup on the line above only confirms the "image" key is
present -- it says nothing about the value's type. A restored resource
whose image field is present but not a JSON string (e.g. a number,
bool, null, array, or object) causes an unrecovered
"interface conversion: interface {} is not string" panic in this
RestoreItemAction plugin whenever the optional image-remapping
ConfigMap feature is configured.

Switch to the comma-ok form of the assertion and skip (with a log
message) any container whose image field isn't a string, instead of
panicking.

Signed-off-by: Kaizhe Huang <derek0405@gmail.com>

* Add regression test for non-string image field panic

Covers the comma-ok assertion fix: replaceImageName operates on
generic unstructured content decoded from a backup tarball, which
isn't validated against the Pod schema before this code runs, so
"image" isn't guaranteed to be a string.

Signed-off-by: Kaizhe Huang <derek0405@gmail.com>

* Guard container-entry type assertion, use unstructured.NestedString

Addresses reviewer feedback: container.(map[string]any) was also an
unchecked assertion, and unstructured.NestedString gives safer,
more idiomatic type-checking than a manual comma-ok assertion. Also
switches the skip-path logging from Info to Warn per review.

Signed-off-by: Kaizhe Huang <derek0405@gmail.com>

---------

Signed-off-by: Kaizhe Huang <derek0405@gmail.com>

changelogs/unreleased/10574-Kaizhe

@@ -0,0 +1 @@
+Fix panic in ChangeImageNameAction when a restored container image field is not a string

pkg/restore/actions/change_image_name_action.go

@@ -161,14 +161,24 @@ func (a *ChangeImageNameAction) replaceImageName(obj *unstructured.Unstructured,
 	}
 	for i, container := range containers {
 		log.Infoln("container:", container)
-		if image, ok := container.(map[string]any)["image"]; ok {
-			imageName := image.(string)
-			if exists, newImageName, err := a.isImageReplaceRuleExist(log, imageName, config); exists && err == nil {
-				needUpdateObj = true
-				log.Infof("Updating item's image from %s to %s", imageName, newImageName)
-				container.(map[string]any)["image"] = newImageName
-				containers[i] = container
-			}
+		containerMap, ok := container.(map[string]any)
+		if !ok {
+			log.Warnf("skipping container: container entry is not a map (got %T)", container)
+			continue
+		}
+		imageName, found, err := unstructured.NestedString(containerMap, "image")
+		if err != nil {
+			log.Warnf("skipping container: image field is not a string: %v", err)
+			continue
+		}
+		if !found || imageName == "" {
+			continue
+		}
+		if exists, newImageName, err := a.isImageReplaceRuleExist(log, imageName, config); exists && err == nil {
+			needUpdateObj = true
+			log.Infof("Updating item's image from %s to %s", imageName, newImageName)
+			containerMap["image"] = newImageName
+			containers[i] = containerMap
 		}
 	}
 	if needUpdateObj {

pkg/restore/actions/change_image_name_action_test.go

@@ -177,3 +177,90 @@ func TestChangeImageRepositoryActionExecute(t *testing.T) {
 		})
 	}
 }
+
+// TestReplaceImageNameSkipsNonStringImageField is a regression test for a
+// panic on a crafted/malformed backup: replaceImageName operates on generic
+// unstructured content decoded from a backup tarball, which is untrusted
+// input not validated against the Pod schema before this code runs, so the
+// "image" field is not guaranteed to be a string. Uses a hand-built
+// unstructured object (rather than the table-driven cases above, which all
+// go through a typed corev1api.Pod and so can never produce a non-string
+// image field) to exercise that path directly.
+func TestReplaceImageNameSkipsNonStringImageField(t *testing.T) {
+	a := NewChangeImageNameAction(logrus.StandardLogger(), nil)
+
+	obj := &unstructured.Unstructured{
+		Object: map[string]any{
+			"apiVersion": "v1",
+			"kind":       "Pod",
+			"metadata": map[string]any{
+				"name":      "pod1",
+				"namespace": "default",
+			},
+			"spec": map[string]any{
+				"containers": []any{
+					map[string]any{
+						"name":  "container1",
+						"image": float64(5000),
+					},
+				},
+			},
+		},
+	}
+
+	configMap := builder.ForConfigMap("velero", "change-image-name").
+		ObjectMeta(builder.WithLabels("velero.io/plugin-config", "", "velero.io/change-image-name", "RestoreItemAction")).
+		Data("specific", "1.1.1.1:5000,2.2.2.2:3000").
+		Result()
+
+	require.NotPanics(t, func() {
+		err := a.replaceImageName(obj, configMap, "spec", "containers")
+		require.NoError(t, err)
+	})
+
+	containers, _, err := unstructured.NestedSlice(obj.UnstructuredContent(), "spec", "containers")
+	require.NoError(t, err)
+	require.Len(t, containers, 1)
+	// the non-string image field must be left untouched, not modified or
+	// coerced into a string.
+	assert.Equal(t, float64(5000), containers[0].(map[string]any)["image"])
+}
+
+// TestReplaceImageNameSkipsNonMapContainerEntry is a regression test for the
+// same untrusted-content trust boundary as
+// TestReplaceImageNameSkipsNonStringImageField, but for the container list
+// entry itself rather than its "image" field: a crafted/malformed backup's
+// spec.containers entry is not guaranteed to be a map at all.
+func TestReplaceImageNameSkipsNonMapContainerEntry(t *testing.T) {
+	a := NewChangeImageNameAction(logrus.StandardLogger(), nil)
+
+	obj := &unstructured.Unstructured{
+		Object: map[string]any{
+			"apiVersion": "v1",
+			"kind":       "Pod",
+			"metadata": map[string]any{
+				"name":      "pod1",
+				"namespace": "default",
+			},
+			"spec": map[string]any{
+				"containers": []any{"not-a-map"},
+			},
+		},
+	}
+
+	configMap := builder.ForConfigMap("velero", "change-image-name").
+		ObjectMeta(builder.WithLabels("velero.io/plugin-config", "", "velero.io/change-image-name", "RestoreItemAction")).
+		Data("specific", "1.1.1.1:5000,2.2.2.2:3000").
+		Result()
+
+	require.NotPanics(t, func() {
+		err := a.replaceImageName(obj, configMap, "spec", "containers")
+		require.NoError(t, err)
+	})
+
+	containers, _, err := unstructured.NestedSlice(obj.UnstructuredContent(), "spec", "containers")
+	require.NoError(t, err)
+	require.Len(t, containers, 1)
+	// the non-map container entry must be left untouched.
+	assert.Equal(t, "not-a-map", containers[0])
+}