fix: allow resolving repo root as jsonnet lib path (argoproj#11119)

Signed-off-by: shuai-zh <shuaiz8023@gmail.com>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Signed-off-by: emirot <emirot.nolan@gmail.com>

reposerver/repository/repository.go

@@ -856,7 +856,7 @@ func helmTemplate(appPath string, repoRoot string, env *v1alpha1.Env, q *apiclie
 		for _, val := range appHelm.ValueFiles {
 
 			// This will resolve val to an absolute path (or an URL)
-			path, isRemote, err := pathutil.ResolveFilePath(appPath, repoRoot, env.Envsubst(val), q.GetValuesFileSchemes())
+			path, isRemote, err := pathutil.ResolveValueFilePathOrUrl(appPath, repoRoot, env.Envsubst(val), q.GetValuesFileSchemes())
 			if err != nil {
 				return nil, err
 			}
@@ -896,7 +896,7 @@ func helmTemplate(appPath string, repoRoot string, env *v1alpha1.Env, q *apiclie
 			}
 		}
 		for _, p := range appHelm.FileParameters {
-			resolvedPath, _, err := pathutil.ResolveFilePath(appPath, repoRoot, env.Envsubst(p.Path), q.GetValuesFileSchemes())
+			resolvedPath, _, err := pathutil.ResolveValueFilePathOrUrl(appPath, repoRoot, env.Envsubst(p.Path), q.GetValuesFileSchemes())
 			if err != nil {
 				return nil, err
 			}
@@ -1504,7 +1504,7 @@ func makeJsonnetVm(appPath string, repoRoot string, sourceJsonnet v1alpha1.Appli
 	jpaths := []string{appPath}
 	for _, p := range sourceJsonnet.Libs {
 		// the jsonnet library path is relative to the repository root, not application path
-		jpath, _, err := pathutil.ResolveFilePath(repoRoot, repoRoot, p, nil)
+		jpath, err := pathutil.ResolveFileOrDirectoryPath(repoRoot, repoRoot, p)
 		if err != nil {
 			return nil, err
 		}
@@ -1752,7 +1752,7 @@ func populateHelmAppDetails(res *apiclient.RepoAppDetailsResponse, appPath strin
 		return err
 	}
 
-	if resolvedValuesPath, _, err := pathutil.ResolveFilePath(appPath, repoRoot, "values.yaml", []string{}); err == nil {
+	if resolvedValuesPath, _, err := pathutil.ResolveValueFilePathOrUrl(appPath, repoRoot, "values.yaml", []string{}); err == nil {
 		if err := loadFileIntoIfExists(resolvedValuesPath, &res.Helm.Values); err != nil {
 			return err
 		}
@@ -1762,7 +1762,7 @@ func populateHelmAppDetails(res *apiclient.RepoAppDetailsResponse, appPath strin
 	var resolvedSelectedValueFiles []pathutil.ResolvedFilePath
 	// drop not allowed values files
 	for _, file := range selectedValueFiles {
-		if resolvedFile, _, err := pathutil.ResolveFilePath(appPath, repoRoot, file, q.GetValuesFileSchemes()); err == nil {
+		if resolvedFile, _, err := pathutil.ResolveValueFilePathOrUrl(appPath, repoRoot, file, q.GetValuesFileSchemes()); err == nil {
 			resolvedSelectedValueFiles = append(resolvedSelectedValueFiles, resolvedFile)
 		} else {
 			log.Warnf("Values file %s is not allowed: %v", file, err)

reposerver/repository/repository_test.go

@@ -342,6 +342,27 @@ func TestGenerateJsonnetManifestInDir(t *testing.T) {
 	assert.Equal(t, 2, len(res1.Manifests))
 }
 
+func TestGenerateJsonnetManifestInRootDir(t *testing.T) {
+	service := newService("testdata/jsonnet-1")
+
+	q := apiclient.ManifestRequest{
+		Repo: &argoappv1.Repository{},
+		ApplicationSource: &argoappv1.ApplicationSource{
+			Path: ".",
+			Directory: &argoappv1.ApplicationSourceDirectory{
+				Jsonnet: argoappv1.ApplicationSourceJsonnet{
+					ExtVars: []argoappv1.JsonnetVar{{Name: "extVarString", Value: "extVarString"}, {Name: "extVarCode", Value: "\"extVarCode\"", Code: true}},
+					TLAs:    []argoappv1.JsonnetVar{{Name: "tlaString", Value: "tlaString"}, {Name: "tlaCode", Value: "\"tlaCode\"", Code: true}},
+					Libs:    []string{"."},
+				},
+			},
+		},
+	}
+	res1, err := service.GenerateManifest(context.Background(), &q)
+	assert.Nil(t, err)
+	assert.Equal(t, 2, len(res1.Manifests))
+}
+
 func TestGenerateJsonnetLibOutside(t *testing.T) {
 	service := newService(".")
 
@@ -358,7 +379,7 @@ func TestGenerateJsonnetLibOutside(t *testing.T) {
 	}
 	_, err := service.GenerateManifest(context.Background(), &q)
 	require.Error(t, err)
-	require.Contains(t, err.Error(), "value file '../../../testdata/jsonnet/vendor' resolved to outside repository root")
+	require.Contains(t, err.Error(), "file '../../../testdata/jsonnet/vendor' resolved to outside repository root")
 }
 
 func TestManifestGenErrorCacheByNumRequests(t *testing.T) {

reposerver/repository/testdata/jsonnet-1/guestbook-ui.jsonnet

@@ -0,0 +1,47 @@
+local service = import 'vendor/nested/service.libsonnet';
+local params = import 'params.libsonnet';
+
+function(tlaString, tlaCode)
+  [
+    service.new(params),
+    {
+      apiVersion: 'apps/v1beta2',
+      kind: 'Deployment',
+      metadata: {
+        name: params.name,
+      },
+      spec: {
+        replicas: params.replicas,
+        selector: {
+          matchLabels: {
+            app: params.name,
+          },
+        },
+        template: {
+          metadata: {
+            labels: {
+              app: params.name,
+              tlaString: tlaString,
+              tlaCode: tlaCode,
+              extVarString: std.extVar('extVarString'),
+              extVarCode: std.extVar('extVarCode'),
+            },
+          },
+          spec: {
+            containers: [
+              {
+                image: params.image,
+                name: params.name,
+                ports: [
+                  {
+                    containerPort: params.containerPort,
+                  },
+                ],
+              },
+            ],
+          },
+        },
+      },
+    },
+    null,
+  ]

reposerver/repository/testdata/jsonnet-1/params.libsonnet

@@ -0,0 +1,8 @@
+{
+  containerPort: 80,
+  image: "gcr.io/heptio-images/ks-guestbook-demo:0.2",
+  name: "guestbook-ui",
+  replicas: 1,
+  servicePort: 80,
+  type: "ClusterIP",
+}

reposerver/repository/testdata/jsonnet-1/vendor/nested/service.libsonnet

@@ -0,0 +1,23 @@
+local new(params) = {
+  apiVersion: 'v1',
+  kind: 'Service',
+  metadata: {
+    name: params.name,
+  },
+  spec: {
+    ports: [
+      {
+        port: params.servicePort,
+        targetPort: params.containerPort,
+      },
+    ],
+    selector: {
+      app: params.name,
+    },
+    type: params.type,
+  },
+};
+
+{
+  new:: new,
+}

util/helm/helm.go

@@ -133,7 +133,7 @@ func Version(shortForm bool) (string, error) {
 func (h *helm) GetParameters(valuesFiles []pathutil.ResolvedFilePath, appPath, repoRoot string) (map[string]string, error) {
 	var values []string
 	// Don't load values.yaml if it's an out-of-bounds link.
-	if _, _, err := pathutil.ResolveFilePath(appPath, repoRoot, "values.yaml", []string{}); err == nil {
+	if _, _, err := pathutil.ResolveValueFilePathOrUrl(appPath, repoRoot, "values.yaml", []string{}); err == nil {
 		out, err := h.cmd.inspectValues(".")
 		if err != nil {
 			return nil, err

util/helm/helm_test.go

@@ -59,7 +59,7 @@ func TestHelmTemplateValues(t *testing.T) {
 	require.NoError(t, err)
 	h, err := NewHelmApp(repoRootAbs, []HelmRepository{}, false, "", "", false)
 	assert.NoError(t, err)
-	valuesPath, _, err := path.ResolveFilePath(repoRootAbs, repoRootAbs, "values-production.yaml", nil)
+	valuesPath, _, err := path.ResolveValueFilePathOrUrl(repoRootAbs, repoRootAbs, "values-production.yaml", nil)
 	require.NoError(t, err)
 	opts := TemplateOpts{
 		Name:   "test",
@@ -98,7 +98,7 @@ func TestHelmGetParamsValueFiles(t *testing.T) {
 	require.NoError(t, err)
 	h, err := NewHelmApp(repoRootAbs, nil, false, "", "", false)
 	assert.NoError(t, err)
-	valuesPath, _, err := path.ResolveFilePath(repoRootAbs, repoRootAbs, "values-production.yaml", nil)
+	valuesPath, _, err := path.ResolveValueFilePathOrUrl(repoRootAbs, repoRootAbs, "values-production.yaml", nil)
 	require.NoError(t, err)
 	params, err := h.GetParameters([]path.ResolvedFilePath{valuesPath}, repoRootAbs, repoRootAbs)
 	assert.Nil(t, err)
@@ -113,9 +113,9 @@ func TestHelmGetParamsValueFilesThatExist(t *testing.T) {
 	require.NoError(t, err)
 	h, err := NewHelmApp(repoRootAbs, nil, false, "", "", false)
 	assert.NoError(t, err)
-	valuesMissingPath, _, err := path.ResolveFilePath(repoRootAbs, repoRootAbs, "values-missing.yaml", nil)
+	valuesMissingPath, _, err := path.ResolveValueFilePathOrUrl(repoRootAbs, repoRootAbs, "values-missing.yaml", nil)
 	require.NoError(t, err)
-	valuesProductionPath, _, err := path.ResolveFilePath(repoRootAbs, repoRootAbs, "values-production.yaml", nil)
+	valuesProductionPath, _, err := path.ResolveValueFilePathOrUrl(repoRootAbs, repoRootAbs, "values-production.yaml", nil)
 	require.NoError(t, err)
 	params, err := h.GetParameters([]path.ResolvedFilePath{valuesMissingPath, valuesProductionPath}, repoRootAbs, repoRootAbs)
 	assert.Nil(t, err)

util/io/path/resolved.go

@@ -10,10 +10,14 @@ import (
 	log "github.com/sirupsen/logrus"
 )
 
-// ResolvedFilePath represents a resolved file path and intended to prevent unintentional use of not verified file path.
-// It is always either a URL or an absolute path.
+// ResolvedFilePath represents a resolved file path and is intended to prevent unintentional use of an unverified file
+// path. It is always either a URL or an absolute path.
 type ResolvedFilePath string
 
+// ResolvedFileOrDirectoryPath represents a resolved file or directory path and is intended to prevent unintentional use
+// of an unverified file or directory path. It is an absolute path.
+type ResolvedFileOrDirectoryPath string
+
 // resolveSymbolicLinkRecursive resolves the symlink path recursively to its
 // canonical path on the file system, with a maximum nesting level of maxDepth.
 // If path is not a symlink, returns the verbatim copy of path and err of nil.
@@ -60,7 +64,24 @@ func isURLSchemeAllowed(scheme string, allowed []string) bool {
 	return isAllowed && scheme != ""
 }
 
-// ResolveFilePath will inspect and resolve given file, and make sure that its final path is within the boundaries of
+// We do not provide the path in the error message, because it will be
+// returned to the user and could be used for information gathering.
+// Instead, we log the concrete error details.
+func resolveFailure(path string, err error) error {
+	log.Errorf("failed to resolve path '%s': %v", path, err)
+	return fmt.Errorf("internal error: failed to resolve path. Check logs for more details")
+}
+
+func ResolveFileOrDirectoryPath(appPath, repoRoot, dir string) (ResolvedFileOrDirectoryPath, error) {
+	path, err := resolveFileOrDirectory(appPath, repoRoot, dir, true)
+	if err != nil {
+		return "", err
+	}
+
+	return ResolvedFileOrDirectoryPath(path), nil
+}
+
+// ResolveValueFilePathOrUrl will inspect and resolve given file, and make sure that its final path is within the boundaries of
 // the path specified in repoRoot.
 //
 // appPath is the path we're operating in, e.g. where a Helm chart was unpacked
@@ -88,15 +109,7 @@ func isURLSchemeAllowed(scheme string, allowed []string) bool {
 //
 // isRemote will be set to true if valueFile is an URL using an allowed
 // protocol scheme, or to false if it resolved to a local file.
-func ResolveFilePath(appPath, repoRoot, valueFile string, allowedURLSchemes []string) (resolvedPath ResolvedFilePath, isRemote bool, err error) {
-	// We do not provide the path in the error message, because it will be
-	// returned to the user and could be used for information gathering.
-	// Instead, we log the concrete error details.
-	resolveFailure := func(path string, err error) error {
-		log.Errorf("failed to resolve path '%s': %v", path, err)
-		return fmt.Errorf("internal error: failed to resolve path. Check logs for more details")
-	}
-
+func ResolveValueFilePathOrUrl(appPath, repoRoot, valueFile string, allowedURLSchemes []string) (resolvedPath ResolvedFilePath, isRemote bool, err error) {
 	// A value file can be specified as an URL to a remote resource.
 	// We only allow certain URL schemes for remote value files.
 	url, err := url.Parse(valueFile)
@@ -111,36 +124,45 @@ func ResolveFilePath(appPath, repoRoot, valueFile string, allowedURLSchemes []st
 		}
 	}
 
+	path, err := resolveFileOrDirectory(appPath, repoRoot, valueFile, false)
+	if err != nil {
+		return "", false, err
+	}
+
+	return ResolvedFilePath(path), false, nil
+}
+
+func resolveFileOrDirectory(appPath string, repoRoot string, fileOrDirectory string, allowResolveToRoot bool) (string, error) {
 	// Ensure that our repository root is absolute
 	absRepoPath, err := filepath.Abs(repoRoot)
 	if err != nil {
-		return "", false, resolveFailure(repoRoot, err)
+		return "", resolveFailure(repoRoot, err)
 	}
 
-	// If the path to the file is relative, join it with the current working directory (appPath)
+	// If the path to the file or directory is relative, join it with the current working directory (appPath)
 	// Otherwise, join it with the repository's root
-	path := valueFile
+	path := fileOrDirectory
 	if !filepath.IsAbs(path) {
 		absWorkDir, err := filepath.Abs(appPath)
 		if err != nil {
-			return "", false, resolveFailure(repoRoot, err)
+			return "", resolveFailure(repoRoot, err)
 		}
 		path = filepath.Join(absWorkDir, path)
 	} else {
 		path = filepath.Join(absRepoPath, path)
 	}
 
-	// Ensure any symbolic link is resolved before we
+	// Ensure any symbolic link is resolved before we evaluate the path
 	delinkedPath, err := resolveSymbolicLinkRecursive(path, 10)
 	if err != nil {
-		return "", false, resolveFailure(path, err)
+		return "", resolveFailure(repoRoot, err)
 	}
 	path = delinkedPath
 
 	// Resolve the joined path to an absolute path
 	path, err = filepath.Abs(path)
 	if err != nil {
-		return "", false, resolveFailure(path, err)
+		return "", resolveFailure(repoRoot, err)
 	}
 
 	// Ensure our root path has a trailing slash, otherwise the following check
@@ -150,10 +172,17 @@ func ResolveFilePath(appPath, repoRoot, valueFile string, allowedURLSchemes []st
 		requiredRootPath += string(os.PathSeparator)
 	}
 
-	// Make sure that the resolved path to values file is within the repository's root path
-	if !strings.HasPrefix(path, requiredRootPath) {
-		return "", false, fmt.Errorf("value file '%s' resolved to outside repository root", valueFile)
+	resolvedToRoot := path+string(os.PathSeparator) == requiredRootPath
+	if resolvedToRoot {
+		if !allowResolveToRoot {
+			return "", resolveFailure(path, fmt.Errorf("path resolved to repository root, which is not allowed"))
+		}
+	} else {
+		// Make sure that the resolved path to file is within the repository's root path
+		if !strings.HasPrefix(path, requiredRootPath) {
+			return "", fmt.Errorf("file '%s' resolved to outside repository root", fileOrDirectory)
+		}
 	}
 
-	return ResolvedFilePath(path), false, nil
+	return path, nil
 }