initwd: Error message for local paths escaping module packages

Our module installer has a somewhat-informal idea of a "module package",
which is some external thing we can go fetch in order to add one or more
modules to the current configuration. Our documentation doesn't talk much
about it because most users seem to have found the distinction between
external and local modules pretty intuitive without us throwing a lot of
funny terminology at them, but there are some situations where the
distinction between a module and a module package are material to the
end-user.

One such situation is when using an absolute rather than relative
filesystem path: we treat that as an external package in order to make the
resulting working directory theoretically "portable" (although users can
do various other things to defeat that), and so Terraform will copy the
directory into .terraform/modules in the same way as it would download and
extract a remote archive package or clone a git repository.

A consequence of this, though, is that any relative paths called from
inside a module loaded from an absolute path will fail if they try to
traverse upward into the parent directory, because at runtime we're
actually running from a copy of the directory that's been taking out of
its original context.

A similar sort of situation can occur in a truly remote module package if
the author accidentally writes a "../" source path that traverses up out
of the package root, and so this commit introduces a special error message
for both situations that tries to be a bit clearer about there being a
package boundary and use that to explain why installation failed.

We would ideally have made escaping local references like that illegal in
the first place, but sadly we did not and so when we rebuilt the module
installer for Terraform v0.12 we ended up keeping the previous behavior of
just trying it and letting it succeed if there happened to somehow be a
matching directory at the given path, in order to remain compatible with
situations that had worked by coincidence rather than intention. For that
same reason, I've implemented this as a replacement error message we will
return only if local module installation was going to fail anyway, and
thus it only modifies the error message for some existing error situations
rather than introducing new error situations.

This also includes some light updates to the documentation to say a little
more about how Terraform treats absolute paths, though aiming not to get
too much into the weeds about module packages since it's something that
most users can get away with never knowing.

internal/initwd/module_install.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"log"
 	"os"
+	"path"
 	"path/filepath"
 	"strings"
 
@@ -200,6 +201,7 @@ func (i *ModuleInstaller) installDescendentModules(rootMod *tfconfig.Module, roo
 			case isLocalSourceAddr(req.SourceAddr):
 				log.Printf("[TRACE] ModuleInstaller: %s has local path %q", key, req.SourceAddr)
 				mod, mDiags := i.installLocalModule(req, key, manifest, hooks)
+				mDiags = maybeImproveLocalInstallError(req, mDiags)
 				diags = append(diags, mDiags...)
 				return mod, nil, diags
 
@@ -588,3 +590,132 @@ func (i *ModuleInstaller) installGoGetterModule(req *earlyconfig.ModuleRequest,
 func (i *ModuleInstaller) packageInstallPath(modulePath addrs.Module) string {
 	return filepath.Join(i.modsDir, strings.Join(modulePath, "."))
 }
+
+// maybeImproveLocalInstallError is a helper function which can recognize
+// some specific situations where it can return a more helpful error message
+// and thus replace the given errors with those if so.
+//
+// If this function can't do anything about a particular situation then it
+// will just return the given diags verbatim.
+//
+// This function's behavior is only reasonable for errors returned from the
+// ModuleInstaller.installLocalModule function.
+func maybeImproveLocalInstallError(req *earlyconfig.ModuleRequest, diags tfdiags.Diagnostics) tfdiags.Diagnostics {
+	if !diags.HasErrors() {
+		return diags
+	}
+
+	// The main situation we're interested in detecting here is whether the
+	// current module or any of its ancestors use relative paths that reach
+	// outside of the "package" established by the nearest non-local ancestor.
+	// That's never really valid, but unfortunately we historically didn't
+	// have any explicit checking for it and so now for compatibility in
+	// situations where things just happened to "work" we treat this as an
+	// error only in situations where installation would've failed anyway,
+	// so we can give a better error about it than just a generic
+	// "directory not found" or whatever.
+	//
+	// Since it's never actually valid to relative out of the containing
+	// package, we just assume that any failed local package install which
+	// does so was caused by that, because to stop doing it should always
+	// improve the situation, even if it leads to another error describing
+	// a different problem.
+
+	// To decide this we need to find the subset of our ancestors that
+	// belong to the same "package" as our request, along with the closest
+	// ancestor that defined that package, and then we can work forwards
+	// to see if any of the local paths "escaped" the package.
+	type Step struct {
+		Path       addrs.Module
+		SourceAddr string
+	}
+	var packageDefiner Step
+	var localRefs []Step
+	localRefs = append(localRefs, Step{
+		Path:       req.Path,
+		SourceAddr: req.SourceAddr,
+	})
+	current := req.Parent // an earlyconfig.Config where Children isn't populated yet
+	for {
+		if current == nil {
+			// We've reached the root module, in which case we aren't
+			// in an external "package" at all and so our special case
+			// can't apply.
+			return diags
+		}
+		if !isLocalSourceAddr(current.SourceAddr) {
+			// We've found the package definer, then!
+			packageDefiner = Step{
+				Path:       current.Path,
+				SourceAddr: current.SourceAddr,
+			}
+			break
+		}
+
+		localRefs = append(localRefs, Step{
+			Path:       current.Path,
+			SourceAddr: current.SourceAddr,
+		})
+		current = current.Parent
+	}
+	// Our localRefs list is reversed because we were traversing up the tree,
+	// so we'll flip it the other way and thus walk "downwards" through it.
+	for i, j := 0, len(localRefs)-1; i < j; i, j = i+1, j-1 {
+		localRefs[i], localRefs[j] = localRefs[j], localRefs[i]
+	}
+
+	// Our method here is to start with a known base path prefix and
+	// then apply each of the local refs to it in sequence until one of
+	// them causes us to "lose" the prefix. If that happens, we've found
+	// an escape to report. This is not an exact science but good enough
+	// heuristic for choosing a better error message.
+	const prefix = "*/" // NOTE: this can find a false negative if the user chooses "*" as a directory name, but we consider that unlikely
+	packageAddr, startPath := splitAddrSubdir(packageDefiner.SourceAddr)
+	currentPath := path.Join(prefix, startPath)
+	for _, step := range localRefs {
+		// We're working in the simpler space of "path" rather than "filepath"
+		// for our heuristic here, so we need to slashify Windows-ish paths.
+		rel := filepath.ToSlash(step.SourceAddr)
+
+		nextPath := path.Join(currentPath, rel)
+		if !strings.HasPrefix(nextPath, prefix) { // ESCAPED!
+			escapeeAddr := step.Path.String()
+
+			var newDiags tfdiags.Diagnostics
+
+			// First we'll copy over any non-error diagnostics from the source diags
+			for _, diag := range diags {
+				if diag.Severity() != tfdiags.Error {
+					newDiags = newDiags.Append(diag)
+				}
+			}
+
+			// ...but we'll replace any errors with this more precise error.
+			var suggestion string
+			if strings.HasPrefix(packageAddr, "/") || filepath.VolumeName(packageAddr) != "" {
+				// It might be somewhat surprising that Terraform treats
+				// absolute filesystem paths as "external" even though it
+				// treats relative paths as local, so if it seems like that's
+				// what the user was doing then we'll add an additional note
+				// about it.
+				suggestion = "\n\nTerraform treats absolute filesystem paths as external modules which establish a new module package. To treat this directory as part of the same package as its caller, use a local path starting with either \"./\" or \"../\"."
+			}
+			newDiags = newDiags.Append(tfdiags.Sourceless(
+				tfdiags.Error,
+				"Local module path escapes module package",
+				fmt.Sprintf(
+					"The given source directory for %s would be outside of its containing package %q. Local source addresses starting with \"../\" must stay within the same package that the calling module belongs to.%s",
+					escapeeAddr, packageAddr, suggestion,
+				),
+			))
+
+			return newDiags
+		}
+
+		currentPath = nextPath
+	}
+
+	// If we get down here then we have nothing useful to do, so we'll just
+	// echo back what we were given.
+	return diags
+}

internal/initwd/module_install_test.go

@@ -1,6 +1,7 @@
 package initwd
 
 import (
+	"bytes"
 	"flag"
 	"fmt"
 	"io/ioutil"
@@ -104,6 +105,74 @@ func TestModuleInstaller_error(t *testing.T) {
 	}
 }
 
+func TestModuleInstaller_packageEscapeError(t *testing.T) {
+	fixtureDir := filepath.Clean("testdata/load-module-package-escape")
+	dir, done := tempChdir(t, fixtureDir)
+	defer done()
+
+	// For this particular test we need an absolute path in the root module
+	// that must actually resolve to our temporary directory in "dir", so
+	// we need to do a little rewriting. We replace the arbitrary placeholder
+	// %%BASE%% with the temporary directory path.
+	{
+		rootFilename := filepath.Join(dir, "package-escape.tf")
+		template, err := ioutil.ReadFile(rootFilename)
+		if err != nil {
+			t.Fatal(err)
+		}
+		final := bytes.ReplaceAll(template, []byte("%%BASE%%"), []byte(filepath.ToSlash(dir)))
+		err = ioutil.WriteFile(rootFilename, final, 0644)
+		if err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	hooks := &testInstallHooks{}
+
+	modulesDir := filepath.Join(dir, ".terraform/modules")
+	inst := NewModuleInstaller(modulesDir, nil)
+	_, diags := inst.InstallModules(".", false, hooks)
+
+	if !diags.HasErrors() {
+		t.Fatal("expected error")
+	} else {
+		assertDiagnosticSummary(t, diags, "Local module path escapes module package")
+	}
+}
+
+func TestModuleInstaller_explicitPackageBoundary(t *testing.T) {
+	fixtureDir := filepath.Clean("testdata/load-module-package-prefix")
+	dir, done := tempChdir(t, fixtureDir)
+	defer done()
+
+	// For this particular test we need an absolute path in the root module
+	// that must actually resolve to our temporary directory in "dir", so
+	// we need to do a little rewriting. We replace the arbitrary placeholder
+	// %%BASE%% with the temporary directory path.
+	{
+		rootFilename := filepath.Join(dir, "package-prefix.tf")
+		template, err := ioutil.ReadFile(rootFilename)
+		if err != nil {
+			t.Fatal(err)
+		}
+		final := bytes.ReplaceAll(template, []byte("%%BASE%%"), []byte(filepath.ToSlash(dir)))
+		err = ioutil.WriteFile(rootFilename, final, 0644)
+		if err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	hooks := &testInstallHooks{}
+
+	modulesDir := filepath.Join(dir, ".terraform/modules")
+	inst := NewModuleInstaller(modulesDir, nil)
+	_, diags := inst.InstallModules(".", false, hooks)
+
+	if diags.HasErrors() {
+		t.Fatalf("unexpected errors\n%s", diags.Err().Error())
+	}
+}
+
 func TestModuleInstaller_invalid_version_constraint_error(t *testing.T) {
 	fixtureDir := filepath.Clean("testdata/invalid-version-constraint")
 	dir, done := tempChdir(t, fixtureDir)

internal/initwd/testdata/load-module-package-escape/child/package-escape-child.tf

@@ -0,0 +1,8 @@
+module "grandchild" {
+  # NOTE: This seems like it ought to work because there is indeed a
+  # ../grandchild directory, but our caller loaded us as an external
+  # module using an absolute path and so we're actually isolated from
+  # the parent directory in a separate "module package", and so we
+  # can't traverse out to find the grandchild module.
+  source = "../grandchild"
+}

internal/initwd/testdata/load-module-package-escape/grandchild/package-escape-grandchild.tf

@@ -0,0 +1,2 @@
+# This is intentionally empty, just here to be referred to by the "child"
+# module in ../child .

internal/initwd/testdata/load-module-package-escape/package-escape.tf

@@ -0,0 +1,9 @@
+module "child" {
+  # NOTE: For this test we need a working absolute path so that Terraform
+  # will see this a an "external" module and thus establish a separate
+  # package for it, but we won't know which temporary directory this
+  # will be in at runtime, so we'll rewrite this file inside the test
+  # code to replace %%BASE%% with the actual path. %%BASE%% is not normal
+  # Terraform syntax and won't work outside of this test.
+  source = "%%BASE%%/child"
+}

internal/initwd/testdata/load-module-package-prefix/package-prefix.tf

@@ -0,0 +1,15 @@
+module "child" {
+  # NOTE: For this test we need a working absolute path so that Terraform
+  # will see this a an "external" module and thus establish a separate
+  # package for it, but we won't know which temporary directory this
+  # will be in at runtime, so we'll rewrite this file inside the test
+  # code to replace %%BASE%% with the actual path. %%BASE%% is not normal
+  # Terraform syntax and won't work outside of this test.
+  #
+  # Note that we're intentionally using the special // delimiter to
+  # tell Terraform that it should treat the "package" directory as a
+  # whole as a module package, with all of its descendents "downloaded"
+  # (copied) together into ./.terraform/modules/child so that child
+  # can refer to ../grandchild successfully.
+  source = "%%BASE%%/package//child"
+}

internal/initwd/testdata/load-module-package-prefix/package/child/package-prefix-child.tf

@@ -0,0 +1,9 @@
+module "grandchild" {
+  # NOTE: This only works because our caller told Terraform to treat
+  # the parent directory as a whole as a module package, and so
+  # the "./terraform/modules/child" directory should contain both
+  # "child" and "grandchild" sub directories that we can traverse between.
+  # This is the same as local paths between different directories inside
+  # a single git repository or distribution archive.
+  source = "../grandchild"
+}

internal/initwd/testdata/load-module-package-prefix/package/grandchild/package-prefix-grandchild.tf

@@ -0,0 +1,2 @@
+# This is intentionally empty, just here to be referred to by the "child"
+# module in ../child .

website/docs/language/modules/sources.html.md

@@ -74,6 +74,15 @@ that other sources are: the files are already present on local disk (possibly
 as a result of installing a parent module) and so can just be used directly.
 Their source code is automatically updated if the parent module is upgraded.
 
+Note that Terraform does not consider an _absolute_ filesystem path (starting
+with a slash, a drive letter, or similar) to be a local path. Instead,
+Terraform will treat that in a similar way as a remote module and copy it into
+the local module cache. An absolute path is a "package" in the sense described
+in [Modules in Package Sub-directories](#modules-in-package-sub-directories).
+We don't recommend using absolute filesystem paths to refer to Terraform
+modules, because it will tend to couple your configuration to the filesystem
+layout of a particular computer.
+
 ## Terraform Registry
 
 A module registry is the native way of distributing Terraform modules for use