fix(build): refuse a --build-dir that would delete the project

samcli/commands/_utils/constants.py

@@ -5,6 +5,8 @@
 import os
 
 DEFAULT_STACK_NAME = "sam-app"
+# SAM CLI's own working directory; it holds state other than build output (cache, deps, pipeline config)
+DEFAULT_SAM_DIR = ".aws-sam"
 DEFAULT_BUILD_DIR = os.path.join(".aws-sam", "build")
 DEFAULT_BUILD_DIR_WITH_AUTO_DEPENDENCY_LAYER = os.path.join(".aws-sam", "auto-dependency-layer")
 DEFAULT_CACHE_DIR = os.path.join(".aws-sam", "cache")

samcli/commands/build/build_context.py

@@ -14,7 +14,7 @@
 
 import click
 
-from samcli.commands._utils.constants import DEFAULT_BUILD_DIR
+from samcli.commands._utils.constants import DEFAULT_BUILD_DIR, DEFAULT_SAM_DIR
 from samcli.commands._utils.experimental import ExperimentalFlag, is_experimental_enabled, prompt_experimental
 from samcli.commands._utils.template import (
     FOREACH_REQUIRED_ELEMENTS,
@@ -66,6 +66,7 @@
 from samcli.lib.providers.sam_stack_provider import SamLocalStackProvider
 from samcli.lib.telemetry.event import EventName, EventTracker, UsedFeature
 from samcli.lib.utils.osutils import BUILD_DIR_PERMISSIONS
+from samcli.lib.utils.path_utils import is_same_or_ancestor
 from samcli.local.docker.manager import ContainerManager
 from samcli.local.lambdafn.exceptions import (
     FunctionNotFound,
@@ -256,11 +257,14 @@ def set_up(self) -> None:
         )
         self._layer_provider = SamLayerProvider(self.stacks, self._use_raw_codeuri)
 
+        template_dir = str(pathlib.Path(self._template_file).resolve().parent)
         if not self._base_dir:
             # Base directory, if not provided, is the directory containing the template
-            self._base_dir = str(pathlib.Path(self._template_file).resolve().parent)
+            self._base_dir = template_dir
 
-        self._build_dir = self._setup_build_dir(self._build_dir, self._clean)
+        self._build_dir = self._setup_build_dir(
+            self._build_dir, self._clean, self._cache_dir, [template_dir, self._base_dir]
+        )
 
         if self._cached:
             cache_path = pathlib.Path(self._cache_dir)
@@ -1261,18 +1265,41 @@ def _gen_success_msg(self, artifacts_dir: str, output_template_path: str, is_def
         return msg
 
     @staticmethod
-    def _setup_build_dir(build_dir: str, clean: bool) -> str:
+    def _setup_build_dir(
+        build_dir: str, clean: bool, cache_dir: Optional[str] = None, project_dirs: Optional[List[str]] = None
+    ) -> str:
         build_path = pathlib.Path(build_dir)
-
-        if os.path.abspath(str(build_path)) == os.path.abspath(str(pathlib.Path.cwd())):
+        build_abs = os.path.abspath(str(build_path))
+        cwd = os.path.abspath(str(pathlib.Path.cwd()))
+
+        # The build dir is removed before building, so it must not be or contain the current working
+        # directory, the template's directory or the base dir: '--build-dir ..', or
+        # '-t proj/template.yaml --build-dir proj', would otherwise delete the project.
+        protected_project_dirs = [cwd] + [os.path.abspath(d) for d in project_dirs or []]
+        if any(is_same_or_ancestor(build_abs, protected) for protected in protected_project_dirs):
             exception_message = (
-                "Failing build: Running a build with build-dir as current working directory "
-                "is extremely dangerous since the build-dir contents is first removed. "
-                "This is no longer supported, please remove the '--build-dir' option from the command "
-                "to allow the build artifacts to be placed in the directory your template is in."
+                f"Failing build: build-dir '{build_dir}' is or contains the current working directory, "
+                "the template's directory or the base directory, and the build-dir contents is first removed, "
+                "which would delete them. This is not supported, please remove the '--build-dir' option from the "
+                "command to allow the build artifacts to be placed in the directory your template is in, "
+                "or choose a directory outside the project."
             )
             raise InvalidBuildDirException(exception_message)
 
+        # Nor should it contain the state SAM CLI keeps across builds. This still builds, since some
+        # users pass '--build-dir .aws-sam' today, but the state is lost on every build.
+        protected_dirs = [DEFAULT_SAM_DIR, DEFAULT_DEPENDENCIES_DIR] + ([cache_dir] if cache_dir else [])
+        if clean and any(is_same_or_ancestor(build_abs, os.path.join(cwd, protected)) for protected in protected_dirs):
+            LOG.warning(
+                "Warning: build-dir '%s' is or contains SAM CLI's own '%s' directory, its build cache or dependencies, "
+                "and the build-dir contents is first removed, so these are deleted on every build. "
+                "This will not be supported in a future release. Remove the '--build-dir' option to place "
+                "the build artifacts in '%s', or choose a directory that does not contain these.",
+                build_dir,
+                DEFAULT_SAM_DIR,
+                DEFAULT_BUILD_DIR,
+            )
+
         try:
             if build_path.exists() and os.listdir(build_dir) and clean:
                 # build folder contains something inside. Clear everything.

samcli/lib/utils/path_utils.py

@@ -42,3 +42,30 @@ def check_path_valid_type(path) -> bool:
         return True
     LOG.debug("Type error when trying to use input {} as Path, not string, int, bytes or os.PathLike ".format(path))
     return False
+
+
+def is_same_or_ancestor(ancestor: str, path: str) -> bool:
+    """
+    Checks whether ``ancestor`` is ``path`` itself or one of its parent directories.
+    Both arguments are expected to be absolute. The comparison is lexical (symlinks are not
+    resolved) and case-insensitive on Windows, where two spellings differing only in case
+    name the same directory.
+    Parameters
+    ----------
+    ancestor: str
+        the candidate parent directory
+    path: str
+        the path that may be inside ``ancestor``
+
+    Returns
+    -------
+    bool
+        True if ``path`` is ``ancestor`` or is located beneath it
+    """
+    ancestor = os.path.normcase(os.path.normpath(ancestor))
+    path = os.path.normcase(os.path.normpath(path))
+    try:
+        return os.path.commonpath([ancestor, path]) == ancestor
+    except ValueError:
+        # Paths on different drives on Windows share no common path
+        return False

tests/integration/buildcmd/test_build_cmd.py

@@ -518,6 +518,64 @@ def _get_python_version(self):
         return "python{}.{}".format(sys.version_info.major, sys.version_info.minor)
 
 
+@skipIf(
+    ((IS_WINDOWS and RUNNING_ON_CI) and not CI_OVERRIDE),
+    "Skip build tests on windows when running in CI unless overridden",
+)
+class TestBuildCommand_BuildDirGuard(BuildIntegBase):
+    # --build-dir is removed before building, so it must not hold the project or SAM CLI's own state
+    template = "template.yaml"
+    overrides = {"Runtime": "python3.11", "CodeUri": "PreBuiltPython", "Handler": "main.handler"}
+    warning = "will not be supported in a future release"
+
+    @parameterized.expand(["..", os.path.join("..", "..")])
+    def test_build_dir_parent_of_cwd_is_refused(self, build_dir):
+        project = Path(self.working_dir, "project")
+        project.mkdir()
+        sibling = Path(self.working_dir, "sibling.txt")
+        sibling.write_text("unrelated")
+
+        cmdlist = self.get_command_list(build_dir=build_dir, parameter_overrides=self.overrides)
+        command_result = run_command(cmdlist, cwd=str(project))
+
+        self.assertEqual(command_result.process.returncode, 1)
+        self.assertIn("is or contains the current working directory", command_result.stderr.decode("utf-8"))
+        self.assertTrue(project.is_dir())
+        self.assertTrue(sibling.exists())
+
+    def test_build_dir_containing_template_dir_is_refused(self):
+        # sam build -t proj/template.yaml --build-dir proj, run from proj's parent
+        project = Path(self.working_dir, "proj")
+        shutil.copytree(Path(self.test_data_path, "PreBuiltPython"), project / "PreBuiltPython")
+        shutil.copy(self.template_path, project / "template.yaml")
+
+        cmdlist = [self.cmd, "build", "-t", os.path.join("proj", "template.yaml"), "--build-dir", "proj"]
+        cmdlist += ["--parameter-overrides", self._make_parameter_override_arg(self.overrides)]
+        command_result = run_command(cmdlist, cwd=self.working_dir)
+
+        self.assertEqual(command_result.process.returncode, 1)
+        self.assertIn("the template's directory", command_result.stderr.decode("utf-8"))
+        self.assertTrue(Path(project, "template.yaml").exists())
+        self.assertTrue(Path(project, "PreBuiltPython", "main.py").exists())
+
+    @parameterized.expand([(".aws-sam",), (os.path.join(".aws-sam", "cache"),)])
+    def test_build_dir_containing_sam_state_warns_and_builds(self, build_dir):
+        cmdlist = self.get_command_list(build_dir=build_dir, parameter_overrides=self.overrides)
+        command_result = run_command(cmdlist, cwd=self.working_dir)
+
+        self.assertEqual(command_result.process.returncode, 0)
+        self.assertIn(self.warning, command_result.stderr.decode("utf-8"))
+        self.assertTrue(Path(self.working_dir, build_dir, "template.yaml").exists())
+
+    def test_default_build_dir_does_not_warn(self):
+        cmdlist = self.get_command_list(parameter_overrides=self.overrides)
+        command_result = run_command(cmdlist, cwd=self.working_dir)
+
+        self.assertEqual(command_result.process.returncode, 0)
+        self.assertNotIn(self.warning, command_result.stderr.decode("utf-8"))
+        self.assertTrue(self.built_template.exists())
+
+
 @skipIf(
     ((IS_WINDOWS and RUNNING_ON_CI) and not CI_OVERRIDE),
     "Skip build tests on windows when running in CI unless overridden",

tests/unit/commands/buildcmd/test_build_context.py

@@ -1,6 +1,8 @@
 import io
 import json
 import os
+import pathlib
+import tempfile
 from contextlib import redirect_stdout
 from unittest import TestCase
 from unittest.mock import ANY, MagicMock, Mock, call, patch
@@ -172,7 +174,7 @@ def test_must_setup_context(
         )
         SamFunctionProviderMock.assert_called_once_with([stack], False, locate_layer_nested=False)
         pathlib_mock.Path.assert_called_once_with("template_file")
-        setup_build_dir_mock.assert_called_with("build_dir", True)
+        setup_build_dir_mock.assert_called_with("build_dir", True, "cache_dir", ["basedir", "basedir"])
         ContainerManagerMock.assert_called_once_with(docker_network_id="network", skip_pull_image=True)
         func_provider_mock.get.assert_called_once_with("function_identifier")
 
@@ -511,7 +513,7 @@ def test_must_return_many_functions_to_build(
         )
         SamFunctionProviderMock.assert_called_once_with([stack], False, locate_layer_nested=False)
         pathlib_mock.Path.assert_called_once_with("template_file")
-        setup_build_dir_mock.assert_called_with("build_dir", True)
+        setup_build_dir_mock.assert_called_with("build_dir", True, "cache_dir", ["basedir", "basedir"])
         ContainerManagerMock.assert_called_once_with(docker_network_id="network", skip_pull_image=True)
         func_provider_mock.get_all.assert_called_once()
 
@@ -753,6 +755,7 @@ class TestBuildContext_setup_build_dir(TestCase):
     def test_build_dir_exists_with_non_empty_dir(self, pathlib_patch, os_patch, shutil_patch):
         path_mock = Mock()
         pathlib_patch.Path.return_value = path_mock
+        os_patch.path.join.side_effect = os.path.join
         os_patch.path.abspath.side_effect = ["/somepath", "/cwd/path"]
         path_mock.cwd.return_value = "/cwd/path"
         os_patch.listdir.return_value = True
@@ -780,6 +783,7 @@ def test_rmtree_oserror_is_converted_to_invalid_build_dir(self, pathlib_patch, o
         # must surface as InvalidBuildDirException so --output json still emits a JSON error.
         path_mock = Mock()
         pathlib_patch.Path.return_value = path_mock
+        os_patch.path.join.side_effect = os.path.join
         os_patch.path.abspath.side_effect = ["/somepath", "/cwd/path"]
         path_mock.exists.return_value = True
         os_patch.listdir.return_value = True
@@ -794,6 +798,7 @@ def test_rmtree_oserror_is_converted_to_invalid_build_dir(self, pathlib_patch, o
     def test_build_dir_exists_with_empty_dir(self, pathlib_patch, os_patch, shutil_patch):
         path_mock = Mock()
         pathlib_patch.Path.return_value = path_mock
+        os_patch.path.join.side_effect = os.path.join
         os_patch.listdir.return_value = False
         os_patch.path.abspath.side_effect = ["/somepath", "/cwd/path"]
         path_mock.cwd.return_value = "/cwd/path"
@@ -819,6 +824,7 @@ def test_build_dir_exists_with_empty_dir(self, pathlib_patch, os_patch, shutil_p
     def test_build_dir_does_not_exist(self, pathlib_patch, os_patch, shutil_patch):
         path_mock = Mock()
         pathlib_patch.Path.return_value = path_mock
+        os_patch.path.join.side_effect = os.path.join
         os_patch.path.abspath.side_effect = ["/somepath", "/cwd/path"]
         path_mock.cwd.return_value = "/cwd/path"
         path_mock.resolve.return_value = "long/full/path"
@@ -843,6 +849,7 @@ def test_build_dir_does_not_exist(self, pathlib_patch, os_patch, shutil_patch):
     def test_non_clean_build_when_dir_exists_with_non_empty_dir(self, pathlib_patch, os_patch, shutil_patch):
         path_mock = Mock()
         pathlib_patch.Path.return_value = path_mock
+        os_patch.path.join.side_effect = os.path.join
         os_patch.path.abspath.side_effect = ["/somepath", "/cwd/path"]
         path_mock.cwd.return_value = "/cwd/path"
         os_patch.listdir.return_value = True
@@ -868,6 +875,7 @@ def test_non_clean_build_when_dir_exists_with_non_empty_dir(self, pathlib_patch,
     def test_when_build_dir_is_cwd_raises_exception(self, pathlib_patch, os_patch, shutil_patch):
         path_mock = Mock()
         pathlib_patch.Path.return_value = path_mock
+        os_patch.path.join.side_effect = os.path.join
         os_patch.path.abspath.side_effect = ["/somepath", "/somepath"]
         path_mock.cwd.return_value = "/somepath"
         build_dir = "/somepath"
@@ -884,6 +892,105 @@ def test_when_build_dir_is_cwd_raises_exception(self, pathlib_patch, os_patch, s
         shutil_patch.rmtree.assert_not_called()
         pathlib_patch.Path.cwd.assert_called_once()
 
+    @parameterized.expand([("..",), (os.path.join("..", ".."),)])
+    def test_when_build_dir_is_parent_of_cwd_raises_and_removes_nothing(self, relative_build_dir):
+        with tempfile.TemporaryDirectory() as root:
+            project = pathlib.Path(root, "workspace", "project")
+            project.mkdir(parents=True)
+            template = project / "template.yaml"
+            template.write_text("Resources: {}")
+            sibling = pathlib.Path(root, "workspace", "sibling.txt")
+            sibling.write_text("unrelated")
+
+            with patch("samcli.commands.build.build_context.pathlib.Path.cwd", return_value=project):
+                with self.assertRaises(InvalidBuildDirException):
+                    BuildContext._setup_build_dir(str(project / relative_build_dir), True)
+
+            self.assertTrue(template.exists())
+            self.assertTrue(sibling.exists())
+
+    def test_when_build_dir_contains_template_dir_raises_and_removes_nothing(self):
+        # sam build -t proj/template.yaml --build-dir proj, run from proj's parent
+        with tempfile.TemporaryDirectory() as root:
+            cwd = pathlib.Path(root)
+            template = cwd / "proj" / "template.yaml"
+            template.parent.mkdir()
+            template.write_text("Resources: {}")
+
+            with patch("samcli.commands.build.build_context.pathlib.Path.cwd", return_value=cwd):
+                with self.assertRaises(InvalidBuildDirException):
+                    BuildContext._setup_build_dir(str(cwd / "proj"), True, None, [str(template.parent)])
+
+            self.assertTrue(template.exists())
+
+    @parameterized.expand(
+        [
+            (".aws-sam", None),
+            (os.path.join(".aws-sam", "cache"), os.path.join(".aws-sam", "cache")),
+            (os.path.join(".aws-sam", "deps"), os.path.join(".aws-sam", "cache")),
+            ("out", os.path.join("out", "cache")),
+            ("out", os.path.join("out", "nested", "cache")),
+        ]
+    )
+    def test_when_build_dir_contains_sam_state_warns(self, relative_build_dir, cache_dir):
+        with tempfile.TemporaryDirectory() as root:
+            project = pathlib.Path(root, "project")
+            (project / relative_build_dir).mkdir(parents=True)
+
+            with patch("samcli.commands.build.build_context.pathlib.Path.cwd", return_value=project):
+                with self.assertLogs("samcli.commands.build.build_context", level="WARNING") as logs:
+                    BuildContext._setup_build_dir(str(project / relative_build_dir), True, cache_dir)
+
+            self.assertIn("will not be supported in a future release", logs.output[0])
+            self.assertTrue((project / relative_build_dir).is_dir())
+
+    def test_when_build_dir_contains_sam_state_without_clean_does_not_warn(self):
+        with tempfile.TemporaryDirectory() as root:
+            project = pathlib.Path(root, "project")
+            state_file = project / ".aws-sam" / "cache" / "marker"
+            state_file.parent.mkdir(parents=True)
+            state_file.write_text("state")
+
+            with patch("samcli.commands.build.build_context.pathlib.Path.cwd", return_value=project):
+                with patch("samcli.commands.build.build_context.LOG") as log_mock:
+                    BuildContext._setup_build_dir(str(project / ".aws-sam"), False, os.path.join(".aws-sam", "cache"))
+
+            log_mock.warning.assert_not_called()
+            self.assertTrue(state_file.exists())
+
+    @parameterized.expand(
+        [
+            (os.path.join(".aws-sam", "build"), os.path.join(".aws-sam", "cache")),
+            (os.path.join(".aws-sam", "auto-dependency-layer"), os.path.join(".aws-sam", "cache")),
+            ("out", os.path.join(".aws-sam", "cache")),
+            ("out", None),
+        ]
+    )
+    def test_build_dir_beside_sam_state_is_allowed(self, relative_build_dir, cache_dir):
+        with tempfile.TemporaryDirectory() as root:
+            project = pathlib.Path(root, "project")
+            project.mkdir()
+
+            with patch("samcli.commands.build.build_context.pathlib.Path.cwd", return_value=project):
+                with patch("samcli.commands.build.build_context.LOG") as log_mock:
+                    BuildContext._setup_build_dir(str(project / relative_build_dir), True, cache_dir)
+
+            log_mock.warning.assert_not_called()
+            self.assertTrue((project / relative_build_dir).is_dir())
+
+    def test_build_dir_inside_cwd_is_cleaned(self):
+        with tempfile.TemporaryDirectory() as root:
+            project = pathlib.Path(root, "project")
+            build_dir = project / ".aws-sam" / "build"
+            build_dir.mkdir(parents=True)
+            (build_dir / "stale").write_text("old artifact")
+
+            with patch("samcli.commands.build.build_context.pathlib.Path.cwd", return_value=project):
+                BuildContext._setup_build_dir(str(build_dir), True)
+
+            self.assertTrue(build_dir.is_dir())
+            self.assertEqual(os.listdir(build_dir), [])
+
 
 class TestBuildContext_setup_cached_and_deps_dir(TestCase):
     @parameterized.expand([(True,), (False,)])