diff --git a/pkg/controller/core.oam.dev/v1beta1/application/application_controller.go b/pkg/controller/core.oam.dev/v1beta1/application/application_controller.go index bf6a1af28..78523d365 100644 --- a/pkg/controller/core.oam.dev/v1beta1/application/application_controller.go +++ b/pkg/controller/core.oam.dev/v1beta1/application/application_controller.go @@ -269,8 +269,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu } // Rebuild appliedResources from the ResourceTracker now that the workflow has finished - // dispatching. The RT is the authoritative source - app.Status.AppliedResources = handler.resourceKeeper.GetAppliedResources() + // dispatching. The RT is the authoritative source of which resources exist; the creator + // attribution, which the RT does not persist, is re-attached from the dispatch records. + app.Status.AppliedResources = handler.appliedResourcesWithCreator() var phase = common.ApplicationRunning isHealthy := evalStatus(logCtx, handler, appFile, appParser) diff --git a/pkg/controller/core.oam.dev/v1beta1/application/apply.go b/pkg/controller/core.oam.dev/v1beta1/application/apply.go index a6f11c898..1959f849b 100644 --- a/pkg/controller/core.oam.dev/v1beta1/application/apply.go +++ b/pkg/controller/core.oam.dev/v1beta1/application/apply.go @@ -129,6 +129,35 @@ func (h *AppHandler) Delete(ctx context.Context, _ client.Client, cluster string return nil } +// appliedResourcesWithCreator rebuilds the applied resources from the ResourceTracker, +// which is the authoritative source of which resources currently exist, and re-attaches +// the creator recorded while dispatching them. The ResourceTracker does not persist the +// creator, so rebuilding from it alone drops the creator attribution from the status. +// reconcile runs single threaded, matching addAppliedResource, so no locking is needed. +func (h *AppHandler) appliedResourcesWithCreator() []common.ClusterObjectReference { + rebuilt := h.resourceKeeper.GetAppliedResources() + for i := range rebuilt { + if rebuilt[i].Creator != "" { + continue + } + for _, dispatched := range h.appliedResources { + if dispatched.Creator != "" && sameResource(rebuilt[i], dispatched) { + rebuilt[i].Creator = dispatched.Creator + break + } + } + } + return rebuilt +} + +// sameResource reports whether two references point at the same object. It compares +// object identity only: creator is excluded on purpose (it is the field being restored), +// and UID is not populated on the ResourceTracker side. +func sameResource(a, b common.ClusterObjectReference) bool { + return a.APIVersion == b.APIVersion && a.Kind == b.Kind && + a.Name == b.Name && a.Namespace == b.Namespace && a.Cluster == b.Cluster +} + // addAppliedResource recorde applied resource. // reconcile run at single threaded. So there is no need to consider to use locker. func (h *AppHandler) addAppliedResource(previous bool, refs ...common.ClusterObjectReference) { diff --git a/pkg/controller/utils/capability.go b/pkg/controller/utils/capability.go index 7ae23e35c..a0a460888 100644 --- a/pkg/controller/utils/capability.go +++ b/pkg/controller/utils/capability.go @@ -20,9 +20,12 @@ package utils import ( "context" "fmt" + "io" + "net/url" "os" "path/filepath" "strings" + "time" "github.com/getkin/kin-openapi/openapi3" "github.com/go-git/go-git/v5" @@ -205,41 +208,268 @@ func GetOpenAPISchemaFromTerraformComponentDefinition(configuration string) ([]b // GetTerraformConfigurationFromRemote gets Terraform Configuration(HCL) func GetTerraformConfigurationFromRemote(name, remoteURL, remotePath string, sshPublicKey *gitssh.PublicKeys) (string, error) { + if err := validateModuleName(name); err != nil { + return "", err + } userHome, err := os.UserHomeDir() if err != nil { return "", err } cachePath := filepath.Join(userHome, ".vela", "terraform", name) - // Check if the directory exists. If yes, remove it. - entities, err := os.ReadDir(cachePath) - if err != nil || len(entities) == 0 { - fmt.Printf("loading terraform module %s into %s from %s\n", name, cachePath, remoteURL) - cloneOptions := &git.CloneOptions{ - URL: remoteURL, - Progress: os.Stdout, - } - if sshPublicKey != nil { - cloneOptions.Auth = sshPublicKey - } - if _, err = git.PlainClone(cachePath, false, cloneOptions); err != nil { + + // Reuse the cache only when it is populated AND was cloned from the same URL. + // Otherwise (empty, a changed URL, or a missing marker) re-clone, so a + // definition repointed at a different repository does not keep serving the old + // tree. See GHSA-fmgp-q6jx-gg3x. + if !cacheMatchesRemote(cachePath, remoteURL) { + _ = os.RemoveAll(cachePath) + klog.InfoS("cloning remote Terraform module", "module", name, "url", redactURLCredentials(remoteURL), "cache", cachePath) + if err = cloneTerraformModule(cachePath, remoteURL, sshPublicKey); err != nil { + // Do not leave a partial or oversized clone behind for the next reconcile. + klog.ErrorS(err, "failed to clone remote Terraform module", "module", name, "url", redactURLCredentials(remoteURL)) + _ = os.RemoveAll(cachePath) return "", err } + recordCacheRemote(cachePath, remoteURL) } sshKnownHostsPath := os.Getenv("SSH_KNOWN_HOSTS") _ = os.Remove(sshKnownHostsPath) - tfPath := filepath.Join(cachePath, remotePath, "variables.tf") - if _, err := os.Stat(tfPath); err != nil { - tfPath = filepath.Join(cachePath, remotePath, "main.tf") - if _, err := os.Stat(tfPath); err != nil { - return "", errors.Wrap(err, "failed to find main.tf or variables.tf in Terraform configurations of the remote repository") + conf, err := readTerraformConfigFromDir(cachePath, remotePath) + if err != nil { + // Drop the cached clone on any rejection or read failure so a corrected or + // replaced repository is re-cloned next time, rather than the controller + // re-reading a poisoned or stale tree forever. Wiping on a transient read + // error only costs one bounded re-clone, which beats risking a stuck poison. + klog.InfoS("evicting Terraform module cache after a failed read", "module", name, "cache", cachePath, "err", err.Error()) + _ = os.RemoveAll(cachePath) + return "", err + } + return conf, nil +} + +// cacheRemoteMarkerSuffix names the sibling file that records the remote a cached +// Terraform module was cloned from, used to detect a changed remote. A +// credential-stripped form of the URL is stored (see redactURLCredentials) so the +// marker never persists secrets embedded in an authenticated Git URL. +const cacheRemoteMarkerSuffix = ".remote-url" + +// validateModuleName rejects names that could steer the cache path (and the +// os.RemoveAll that cleans it) outside the module cache directory. Callers pass a +// Kubernetes object name, but the check keeps that guarantee local and explicit. +func validateModuleName(name string) error { + if name == "" || name == "." || name == ".." || strings.ContainsAny(name, `/\`) { + return errors.Errorf("invalid Terraform module name %q", name) + } + return nil +} + +// cacheMatchesRemote reports whether cachePath holds a populated clone recorded +// as coming from remoteURL. The marker stores a credential-stripped URL, so the +// comparison strips credentials from remoteURL the same way before matching. +func cacheMatchesRemote(cachePath, remoteURL string) bool { + entities, err := os.ReadDir(cachePath) + if err != nil || len(entities) == 0 { + return false + } + recorded, err := os.ReadFile(filepath.Clean(cachePath + cacheRemoteMarkerSuffix)) + if err != nil { + return false + } + return string(recorded) == redactURLCredentials(remoteURL) +} + +// recordCacheRemote records the remote a freshly cloned module came from. It stores a +// credential-stripped URL so the marker never persists secrets that an authenticated +// Git URL may embed. +func recordCacheRemote(cachePath, remoteURL string) { + if err := os.WriteFile(cachePath+cacheRemoteMarkerSuffix, []byte(redactURLCredentials(remoteURL)), 0600); err != nil { + klog.ErrorS(err, "failed to record Terraform module remote URL", "cache", cachePath) + } +} + +// redactURLCredentials strips any embedded userinfo (a username, password, or token) +// from a URL so it is safe to log and persist. Authenticated HTTPS Git URLs can carry +// credentials in the userinfo; scp-style SSH URLs ("git@host:path") fail url.Parse but +// authenticate with keys, so they carry no secret. The same stripped form is used for +// logging and for the cache marker, so record and match stay consistent. +func redactURLCredentials(raw string) string { + if u, err := url.Parse(raw); err == nil { + u.User = nil + return u.String() + } + // url.Parse failed (typically scp-style SSH). Strip a leading "user@" defensively + // in case a malformed URL still embeds credentials before the host. + if _, after, found := strings.Cut(raw, "@"); found { + return after + } + return raw +} + +const ( + // cloneTimeout bounds the network fetch of a remote Terraform module clone, so a + // slow or oversized attacker-supplied repository cannot hang the controller. It + // covers the git transport only; the worktree checkout that follows is not bound + // by this deadline. + cloneTimeout = 2 * time.Minute + // maxCloneBytes and maxCloneEntries cap the on-disk footprint of a cloned remote + // Terraform module (modules are small). They run AFTER the clone completes, so + // they bound the RETAINED clone (an oversized clone is rejected and removed by + // the caller) rather than peak transient disk during transfer/checkout; Depth:1 + // keeps that transient footprint to a single shallow commit. Fully bounding peak + // transfer disk (NoCheckout plus object-store reads, or an instrumented + // filesystem) is a follow-up. See GHSA-fmgp-q6jx-gg3x. + maxCloneBytes int64 = 64 << 20 // 64 MiB + // maxCloneEntries counts every filesystem entry (files, directories, and + // symlinks), so a tree of many empty directories or symlinks cannot bypass the + // bound and exhaust inodes. + maxCloneEntries = 50000 +) + +// cloneTerraformModule shallow-clones remoteURL into cachePath under a fetch +// timeout, then rejects an oversized clone. Depth:1 drops history, the timeout +// bounds the network fetch, and the post-clone caps reject a clone whose retained +// size or entry count is too large (the caller removes the rejected clone). +func cloneTerraformModule(cachePath, remoteURL string, sshPublicKey *gitssh.PublicKeys) error { + ctx, cancel := context.WithTimeout(context.Background(), cloneTimeout) + defer cancel() + + cloneOptions := &git.CloneOptions{ + URL: remoteURL, + Progress: os.Stdout, + Depth: 1, + } + if sshPublicKey != nil { + cloneOptions.Auth = sshPublicKey + } + if _, err := git.PlainCloneContext(ctx, cachePath, false, cloneOptions); err != nil { + return errors.Wrap(err, "failed to clone remote Terraform configuration") + } + return ensureDirWithinLimits(cachePath, maxCloneBytes, maxCloneEntries) +} + +// ensureDirWithinLimits returns an error as soon as the entries under root exceed +// maxEntries in count or the regular files exceed maxBytes in total size. Every +// entry (regular files, directories, and symlinks) counts toward maxEntries, so a +// tree of many directories or symlinks cannot slip past the bound and exhaust +// inodes; only regular files contribute to the byte total. It counts everything +// under root (including .git, by design) and does not follow symlinks. The caps +// bound the retained clone, not peak disk during the clone (see maxCloneBytes). +func ensureDirWithinLimits(root string, maxBytes int64, maxEntries int) error { + var ( + total int64 + entries int + ) + return filepath.WalkDir(root, func(_ string, d os.DirEntry, err error) error { + if err != nil { + return err + } + entries++ + if entries > maxEntries { + return errors.Errorf("remote Terraform repository exceeds the maximum allowed entry count of %d", maxEntries) + } + if !d.Type().IsRegular() { + return nil + } + info, err := d.Info() + if err != nil { + return err + } + total += info.Size() + if total > maxBytes { + return errors.Errorf("remote Terraform repository exceeds the maximum allowed size of %d bytes", maxBytes) + } + return nil + }) +} + +// maxTerraformConfigBytes caps how much of a remote Terraform configuration +// file the loader will read. HCL configurations are small, so the cap stops a +// malicious or compromised repository from steering the read at an unbounded +// source (for example a variables.tf symlinked to /dev/zero) and OOM-killing +// the controller. See GHSA-fmgp-q6jx-gg3x. +const maxTerraformConfigBytes int64 = 1 << 20 // 1 MiB + +// errTerraformConfigTooLarge builds the error returned when a candidate +// configuration file exceeds maxTerraformConfigBytes. +func errTerraformConfigTooLarge(path string, maxSize int64) error { + return errors.Errorf("refusing to read Terraform configuration %q: exceeds the maximum allowed size of %d bytes", path, maxSize) +} + +// readTerraformConfigFromDir reads variables.tf, or main.tf as a fallback, from +// the cloned remote repository at cacheRoot/remotePath. It refuses any candidate +// that is not a regular file contained within cacheRoot (defeating symlink and +// path-traversal escapes) and any file larger than maxTerraformConfigBytes. +func readTerraformConfigFromDir(cacheRoot, remotePath string) (string, error) { + baseDir := filepath.Join(cacheRoot, remotePath) + for _, name := range []string{"variables.tf", "main.tf"} { + content, found, err := readContainedRegularFile(cacheRoot, filepath.Join(baseDir, name), maxTerraformConfigBytes) + if err != nil { + return "", err + } + if found { + return content, nil } } - conf, err := os.ReadFile(filepath.Clean(tfPath)) + return "", errors.New("failed to find main.tf or variables.tf in Terraform configurations of the remote repository") +} + +// readContainedRegularFile reads path when it is a regular file whose real +// location stays inside root and whose size is within maxSize. found is false +// only when path does not exist, so callers can try a fallback name; any other +// problem (a symlink or traversal escape, an irregular file, or an oversize +// file) is a hard error rather than a silent fallback. +func readContainedRegularFile(root, path string, maxSize int64) (content string, found bool, err error) { + info, err := os.Lstat(path) if err != nil { - return "", errors.Wrap(err, "failed to read Terraform configuration") + if os.IsNotExist(err) { + return "", false, nil + } + return "", false, errors.Wrap(err, "failed to stat Terraform configuration") } - return string(conf), nil + // Reject any symlink at the target outright, even one that resolves inside + // the cache. This is intentionally stricter than the containment check + // below: variables.tf/main.tf are expected to be regular files, so a blanket + // rejection is the simplest defense against the GHSA-fmgp-q6jx-gg3x + // symlink-to-/dev/zero vector. The containment check still guards a symlinked + // parent directory whose leaf is itself a regular file. + if info.Mode()&os.ModeSymlink != 0 { + return "", true, errors.Errorf("refusing to read symlinked Terraform configuration %q", path) + } + + realRoot, err := filepath.EvalSymlinks(root) + if err != nil { + return "", true, errors.Wrap(err, "failed to resolve Terraform cache directory") + } + realPath, err := filepath.EvalSymlinks(path) + if err != nil { + return "", true, errors.Wrap(err, "failed to resolve Terraform configuration path") + } + if realPath != realRoot && !strings.HasPrefix(realPath, realRoot+string(os.PathSeparator)) { + return "", true, errors.Errorf("refusing to read Terraform configuration outside the cache directory: %q", path) + } + + if !info.Mode().IsRegular() { + return "", true, errors.Errorf("refusing to read non-regular Terraform configuration %q", path) + } + if info.Size() > maxSize { + return "", true, errTerraformConfigTooLarge(path, maxSize) + } + + f, err := os.Open(filepath.Clean(path)) + if err != nil { + return "", true, errors.Wrap(err, "failed to read Terraform configuration") + } + defer func() { _ = f.Close() }() + + buf, err := io.ReadAll(io.LimitReader(f, maxSize+1)) + if err != nil { + return "", true, errors.Wrap(err, "failed to read Terraform configuration") + } + if int64(len(buf)) > maxSize { + return "", true, errTerraformConfigTooLarge(path, maxSize) + } + return string(buf), true, nil } func parseOtherProperties4TerraformDefinition() map[string]*openapi3.Schema { diff --git a/pkg/controller/utils/capability_config_test.go b/pkg/controller/utils/capability_config_test.go index 0e1fb1c48..9b38beede 100644 --- a/pkg/controller/utils/capability_config_test.go +++ b/pkg/controller/utils/capability_config_test.go @@ -104,14 +104,21 @@ variable "aaa" { t.Run(name, func(t *testing.T) { home, _ := os.UserHomeDir() path := filepath.Join(home, ".vela", "terraform") + cacheRoot := filepath.Join(path, tc.args.name) tmpPath := filepath.Join(path, tc.args.name, tc.args.path) if len(tc.args.data) > 0 { err := os.MkdirAll(tmpPath, os.ModePerm) assert.NoError(t, err) err = os.WriteFile(filepath.Clean(filepath.Join(tmpPath, tc.args.variableFile)), tc.args.data, 0644) assert.NoError(t, err) + // Record the remote URL so the populated cache is reused. URL-keyed + // reuse was added for GHSA-fmgp-q6jx-gg3x; without the marker the + // loader would treat the cache as stale and attempt a real clone. + err = os.WriteFile(cacheRoot+cacheRemoteMarkerSuffix, []byte(tc.args.url), 0600) + assert.NoError(t, err) } - defer os.RemoveAll(tmpPath) + defer os.RemoveAll(cacheRoot) + defer os.Remove(cacheRoot + cacheRemoteMarkerSuffix) conf, err := GetTerraformConfigurationFromRemote(tc.args.name, tc.args.url, tc.args.path, nil) if tc.want.errMsg != "" { diff --git a/pkg/controller/utils/capability_fifo_test.go b/pkg/controller/utils/capability_fifo_test.go new file mode 100644 index 000000000..6c7145348 --- /dev/null +++ b/pkg/controller/utils/capability_fifo_test.go @@ -0,0 +1,47 @@ +//go:build unix + +/* + Copyright 2026 The KubeVela 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 utils + +import ( + "os" + "path/filepath" + "syscall" + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestReadTerraformConfigFromDirNonRegular guards the non-regular-file branch +// of the GHSA-fmgp-q6jx-gg3x fix: a non-symlink irregular file (a FIFO here) +// must be refused before any read, since reading it could block or stream +// without bound. This branch is what catches a non-symlink path to an +// unbounded source, complementing the symlink rejection. +func TestReadTerraformConfigFromDirNonRegular(t *testing.T) { + dir := t.TempDir() + fifo := filepath.Join(dir, "variables.tf") + if err := syscall.Mkfifo(fifo, 0600); err != nil { + t.Skipf("mkfifo unsupported on this platform: %v", err) + } + defer func() { _ = os.Remove(fifo) }() + + _, err := readTerraformConfigFromDir(dir, "") + assert.Error(t, err) + assert.Contains(t, err.Error(), "non-regular") +} diff --git a/pkg/controller/utils/capability_test.go b/pkg/controller/utils/capability_test.go index 79b7a1279..5d5d010ef 100644 --- a/pkg/controller/utils/capability_test.go +++ b/pkg/controller/utils/capability_test.go @@ -21,6 +21,7 @@ import ( "context" "fmt" "os" + "path/filepath" "strings" "testing" @@ -535,3 +536,321 @@ func TestGetGitSSHPublicKey(t *testing.T) { }) } } + +// TestReadTerraformConfigFromDir guards the remote Terraform loader against +// GHSA-fmgp-q6jx-gg3x: a variables.tf (or a parent directory) that symlinks +// outside the clone cache, and an oversized configuration file. The loader +// must read normal regular files but refuse anything that escapes the cache +// or exceeds the size cap, before any content is returned. +func TestReadTerraformConfigFromDir(t *testing.T) { + // A sentinel file outside the clone cache. A successful escape would leak + // its content; the guards must prevent that. + outside := t.TempDir() + secretPath := filepath.Join(outside, "secret.txt") + assert.NoError(t, os.WriteFile(secretPath, []byte("top-secret-host-data"), 0600)) + + t.Run("reads variables.tf", func(t *testing.T) { + dir := t.TempDir() + assert.NoError(t, os.WriteFile(filepath.Join(dir, "variables.tf"), []byte(`variable "x" {}`), 0600)) + got, err := readTerraformConfigFromDir(dir, "") + assert.NoError(t, err) + assert.Equal(t, `variable "x" {}`, got) + }) + + t.Run("falls back to main.tf", func(t *testing.T) { + dir := t.TempDir() + assert.NoError(t, os.WriteFile(filepath.Join(dir, "main.tf"), []byte(`resource "x" "y" {}`), 0600)) + got, err := readTerraformConfigFromDir(dir, "") + assert.NoError(t, err) + assert.Equal(t, `resource "x" "y" {}`, got) + }) + + t.Run("errors when neither file is present", func(t *testing.T) { + dir := t.TempDir() + _, err := readTerraformConfigFromDir(dir, "") + assert.Error(t, err) + assert.Contains(t, err.Error(), "failed to find") + }) + + t.Run("refuses a variables.tf that symlinks outside the cache", func(t *testing.T) { + dir := t.TempDir() + // This is the advisory's attack: variables.tf -> a path outside the clone. + assert.NoError(t, os.Symlink(secretPath, filepath.Join(dir, "variables.tf"))) + got, err := readTerraformConfigFromDir(dir, "") + assert.Error(t, err) + assert.NotContains(t, got, "top-secret-host-data") + }) + + t.Run("refuses a symlinked subdirectory that escapes the cache", func(t *testing.T) { + dir := t.TempDir() + outDir := t.TempDir() + assert.NoError(t, os.WriteFile(filepath.Join(outDir, "variables.tf"), []byte("secret-config"), 0600)) + assert.NoError(t, os.Symlink(outDir, filepath.Join(dir, "evil"))) + got, err := readTerraformConfigFromDir(dir, "evil") + assert.Error(t, err) + assert.NotContains(t, got, "secret-config") + }) + + t.Run("refuses a file larger than the size cap", func(t *testing.T) { + dir := t.TempDir() + big := make([]byte, maxTerraformConfigBytes+1024) + assert.NoError(t, os.WriteFile(filepath.Join(dir, "variables.tf"), big, 0600)) + _, err := readTerraformConfigFromDir(dir, "") + assert.Error(t, err) + assert.Contains(t, err.Error(), "exceeds the maximum") + }) + + t.Run("accepts a file exactly at the size cap", func(t *testing.T) { + dir := t.TempDir() + atCap := make([]byte, maxTerraformConfigBytes) + assert.NoError(t, os.WriteFile(filepath.Join(dir, "variables.tf"), atCap, 0600)) + got, err := readTerraformConfigFromDir(dir, "") + assert.NoError(t, err) + assert.Len(t, got, int(maxTerraformConfigBytes)) + }) + + t.Run("refuses a file one byte over the size cap", func(t *testing.T) { + dir := t.TempDir() + overCap := make([]byte, maxTerraformConfigBytes+1) + assert.NoError(t, os.WriteFile(filepath.Join(dir, "variables.tf"), overCap, 0600)) + _, err := readTerraformConfigFromDir(dir, "") + assert.Error(t, err) + assert.Contains(t, err.Error(), "exceeds the maximum") + }) + + t.Run("refuses lexical ../ traversal via remotePath with no symlink", func(t *testing.T) { + dir := t.TempDir() + // A real variables.tf in a sibling of the cache dir, reached purely by a + // cleaned ".." in remotePath, with no symlink anywhere. The containment + // check (not the symlink check) must reject this. + sibling := filepath.Join(filepath.Dir(dir), "sibling-"+filepath.Base(dir)) + assert.NoError(t, os.MkdirAll(sibling, 0700)) + defer func() { _ = os.RemoveAll(sibling) }() + assert.NoError(t, os.WriteFile(filepath.Join(sibling, "variables.tf"), []byte("escaped-config"), 0600)) + got, err := readTerraformConfigFromDir(dir, "../"+filepath.Base(sibling)) + assert.Error(t, err) + assert.NotContains(t, got, "escaped-config") + }) + + t.Run("refuses even a contained in-cache symlink", func(t *testing.T) { + dir := t.TempDir() + // Pins the deliberate reject-all-symlinks policy: a symlink whose target + // is a regular file inside the cache is still refused. + assert.NoError(t, os.WriteFile(filepath.Join(dir, "real.tf"), []byte(`variable "x" {}`), 0600)) + assert.NoError(t, os.Symlink(filepath.Join(dir, "real.tf"), filepath.Join(dir, "variables.tf"))) + got, err := readTerraformConfigFromDir(dir, "") + assert.Error(t, err) + assert.Contains(t, err.Error(), "symlinked") + assert.NotContains(t, got, "variable") + }) +} + +// TestEnsureDirWithinLimits covers the clone caps that bound how large (bytes) +// and how many files a cloned remote Terraform module may retain on disk +// (GHSA-fmgp-q6jx-gg3x). +func TestEnsureDirWithinLimits(t *testing.T) { + t.Run("passes when under the limits", func(t *testing.T) { + dir := t.TempDir() + assert.NoError(t, os.WriteFile(filepath.Join(dir, "a"), make([]byte, 1024), 0600)) + assert.NoError(t, os.WriteFile(filepath.Join(dir, "b"), make([]byte, 1024), 0600)) + assert.NoError(t, ensureDirWithinLimits(dir, 4096, 100)) + }) + + t.Run("passes when cumulative size is exactly at the limit", func(t *testing.T) { + dir := t.TempDir() + assert.NoError(t, os.WriteFile(filepath.Join(dir, "a"), make([]byte, 4096), 0600)) + assert.NoError(t, ensureDirWithinLimits(dir, 4096, 100)) + }) + + t.Run("fails when cumulative size is one byte over the limit", func(t *testing.T) { + dir := t.TempDir() + assert.NoError(t, os.WriteFile(filepath.Join(dir, "a"), make([]byte, 4097), 0600)) + err := ensureDirWithinLimits(dir, 4096, 100) + assert.Error(t, err) + assert.Contains(t, err.Error(), "exceeds the maximum allowed size") + }) + + t.Run("sums regular files across nested subdirectories", func(t *testing.T) { + dir := t.TempDir() + sub := filepath.Join(dir, "modules", "vpc") + assert.NoError(t, os.MkdirAll(sub, 0700)) + assert.NoError(t, os.WriteFile(filepath.Join(dir, "main.tf"), make([]byte, 3000), 0600)) + assert.NoError(t, os.WriteFile(filepath.Join(sub, "vars.tf"), make([]byte, 2000), 0600)) + assert.Error(t, ensureDirWithinLimits(dir, 4096, 100)) + }) + + t.Run("fails when the entry count exceeds the limit", func(t *testing.T) { + dir := t.TempDir() + for i := 0; i < 5; i++ { + assert.NoError(t, os.WriteFile(filepath.Join(dir, fmt.Sprintf("f%d", i)), []byte("x"), 0600)) + } + err := ensureDirWithinLimits(dir, 4096, 3) + assert.Error(t, err) + assert.Contains(t, err.Error(), "entry count") + }) + + t.Run("counts directories toward the entry limit (inode-exhaustion guard)", func(t *testing.T) { + dir := t.TempDir() + // Many empty directories carry no regular-file bytes and no regular files, + // but each is an inode, so they must still trip the entry cap. + for i := 0; i < 10; i++ { + assert.NoError(t, os.MkdirAll(filepath.Join(dir, fmt.Sprintf("d%d", i)), 0700)) + } + err := ensureDirWithinLimits(dir, 4096, 3) + assert.Error(t, err) + assert.Contains(t, err.Error(), "entry count") + }) + + t.Run("does not follow or count symlinks", func(t *testing.T) { + dir := t.TempDir() + outside := t.TempDir() + big := filepath.Join(outside, "big") + assert.NoError(t, os.WriteFile(big, make([]byte, 8192), 0600)) + assert.NoError(t, os.Symlink(big, filepath.Join(dir, "link"))) + // The symlink is skipped (not a regular file), so the 8192-byte target is + // neither followed nor counted against the 4096 cap. + assert.NoError(t, ensureDirWithinLimits(dir, 4096, 100)) + }) + + t.Run("returns an error when the root does not exist", func(t *testing.T) { + assert.Error(t, ensureDirWithinLimits(filepath.Join(t.TempDir(), "missing"), 4096, 100)) + }) +} + +// TestCacheMatchesRemote verifies the URL-keyed cache reuse decision. +func TestCacheMatchesRemote(t *testing.T) { + populated := func(t *testing.T, markerURL string, withMarker bool) string { + t.Helper() + cache := filepath.Join(t.TempDir(), "mod") + assert.NoError(t, os.MkdirAll(cache, 0700)) + assert.NoError(t, os.WriteFile(filepath.Join(cache, "variables.tf"), []byte("x"), 0600)) + if withMarker { + assert.NoError(t, os.WriteFile(cache+cacheRemoteMarkerSuffix, []byte(markerURL), 0600)) + } + return cache + } + + t.Run("matches when populated and the URL marker matches", func(t *testing.T) { + cache := populated(t, "git://example/repo.git", true) + assert.True(t, cacheMatchesRemote(cache, "git://example/repo.git")) + }) + + t.Run("does not match when the URL changed", func(t *testing.T) { + cache := populated(t, "git://example/old.git", true) + assert.False(t, cacheMatchesRemote(cache, "git://example/new.git")) + }) + + t.Run("does not match when the marker is missing", func(t *testing.T) { + cache := populated(t, "", false) + assert.False(t, cacheMatchesRemote(cache, "git://example/repo.git")) + }) + + t.Run("does not match when the cache is empty", func(t *testing.T) { + assert.False(t, cacheMatchesRemote(t.TempDir(), "git://example/repo.git")) + }) +} + +// TestRedactURLCredentials verifies credentials embedded in a remote URL are stripped +// before the URL is logged or written to the cache marker (GHSA-fmgp-q6jx-gg3x). +func TestRedactURLCredentials(t *testing.T) { + cases := map[string]struct { + in string + want string + }{ + "https with user and password": {"https://user:s3cr3t@github.com/org/repo.git", "https://github.com/org/repo.git"}, + "https with token as user": {"https://ghp_TOKEN123@github.com/org/repo.git", "https://github.com/org/repo.git"}, + "https without credentials": {"https://github.com/org/repo.git", "https://github.com/org/repo.git"}, + "ssh url with user": {"ssh://git@github.com/org/repo.git", "ssh://github.com/org/repo.git"}, + "scp-style ssh": {"git@github.com:org/repo.git", "github.com:org/repo.git"}, + "git scheme without credentials": {"git://example/repo.git", "git://example/repo.git"}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + got := redactURLCredentials(tc.in) + assert.Equal(t, tc.want, got) + assert.NotContains(t, got, "s3cr3t") + assert.NotContains(t, got, "ghp_TOKEN123") + }) + } +} + +// TestRecordCacheRemoteRedactsCredentials verifies the persisted cache marker never +// contains credentials from an authenticated remote URL, while the populated cache is +// still reused for the same repository (GHSA-fmgp-q6jx-gg3x). +func TestRecordCacheRemoteRedactsCredentials(t *testing.T) { + cache := filepath.Join(t.TempDir(), "mod") + assert.NoError(t, os.MkdirAll(cache, 0700)) + assert.NoError(t, os.WriteFile(filepath.Join(cache, "variables.tf"), []byte("x"), 0600)) + + recordCacheRemote(cache, "https://user:s3cr3t@github.com/org/repo.git") + + marker, err := os.ReadFile(filepath.Clean(cache + cacheRemoteMarkerSuffix)) + assert.NoError(t, err) + assert.NotContains(t, string(marker), "s3cr3t") + assert.NotContains(t, string(marker), "user") + assert.Equal(t, "https://github.com/org/repo.git", string(marker)) + + // The same repository, with or without credentials, still matches the recorded + // marker, so a populated cache is reused rather than re-cloned. + assert.True(t, cacheMatchesRemote(cache, "https://user:s3cr3t@github.com/org/repo.git")) + assert.True(t, cacheMatchesRemote(cache, "https://github.com/org/repo.git")) + // A different repository does not match. + assert.False(t, cacheMatchesRemote(cache, "https://github.com/org/other.git")) +} + +// TestGetTerraformConfigurationFromRemoteInvalidatesCacheOnRejection exercises +// the cache-reuse path with no network: a populated cache whose URL marker +// matches skips the clone, so a poisoned cached variables.tf (a symlink escaping +// the cache) is rejected AND the cache removed (GHSA-fmgp-q6jx-gg3x). +func TestGetTerraformConfigurationFromRemoteInvalidatesCacheOnRejection(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + outside := t.TempDir() + secret := filepath.Join(outside, "secret.txt") + assert.NoError(t, os.WriteFile(secret, []byte("top-secret-host-data"), 0600)) + + name := "poisoned-module" + url := "git://unused.invalid/repo.git" + cachePath := filepath.Join(home, ".vela", "terraform", name) + assert.NoError(t, os.MkdirAll(cachePath, 0700)) + assert.NoError(t, os.Symlink(secret, filepath.Join(cachePath, "variables.tf"))) + // Matching URL marker so the populated cache is reused and the clone is skipped. + assert.NoError(t, os.WriteFile(cachePath+cacheRemoteMarkerSuffix, []byte(url), 0600)) + + got, err := GetTerraformConfigurationFromRemote(name, url, "", nil) + assert.Error(t, err) + assert.Contains(t, err.Error(), "symlinked") + assert.NotContains(t, got, "top-secret-host-data") + + _, statErr := os.Stat(cachePath) + assert.True(t, os.IsNotExist(statErr), "cache should be removed after a rejected read") +} + +// TestGetTerraformConfigurationFromRemoteCleansUpOnCloneFailure verifies that a +// failed clone (here a non-existent local repo) leaves no cache directory behind +// for the next reconcile to reuse (GHSA-fmgp-q6jx-gg3x). +func TestGetTerraformConfigurationFromRemoteCleansUpOnCloneFailure(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + name := "clone-fail-module" + bogus := filepath.Join(t.TempDir(), "does-not-exist.git") + + _, err := GetTerraformConfigurationFromRemote(name, bogus, "", nil) + assert.Error(t, err) + + cachePath := filepath.Join(home, ".vela", "terraform", name) + _, statErr := os.Stat(cachePath) + assert.True(t, os.IsNotExist(statErr), "cache should be removed after a failed clone") +} + +// TestGetTerraformConfigurationFromRemoteRejectsInvalidName ensures a name that +// could steer the cache path outside the cache directory is rejected up front. +func TestGetTerraformConfigurationFromRemoteRejectsInvalidName(t *testing.T) { + for _, name := range []string{"", ".", "..", "a/b", "../evil"} { + _, err := GetTerraformConfigurationFromRemote(name, "git://example/repo.git", "", nil) + assert.Error(t, err) + assert.Contains(t, err.Error(), "invalid Terraform module name") + } +}