fix: ship a workspaces monorepo function's own dependencies when building in source

aws_lambda_builders/workflows/nodejs_npm/actions.py

@@ -4,14 +4,20 @@
 
 import logging
 import os
+import re
 from typing import Optional
 
+from aws_lambda_builders import utils
 from aws_lambda_builders.actions import ActionFailedError, BaseAction, Purpose
 from aws_lambda_builders.utils import extract_tarfile
 from aws_lambda_builders.workflows.nodejs_npm.npm import NpmExecutionError, SubprocessNpm
 
 LOG = logging.getLogger(__name__)
 
+# A package name as node_modules can spell it: `pkg` or `@scope/pkg`, and nothing else. No separator
+# beyond the single scope slash, so no traversal, no absolute path and no drive letter can get through.
+NODE_MODULES_PACKAGE_NAME = re.compile(r"^(?:@[^/\\:]+/)?[^.@/\\:][^/\\:]*$")
+
 
 class NodejsNpmPackAction(BaseAction):
     """
@@ -374,3 +380,252 @@ def execute(self):
 
         except NpmExecutionError as ex:
             raise ActionFailedError(str(ex))
+
+
+class NodejsNpmLinkDependencyClosureAction(BaseAction):
+    """
+    A Lambda Builder Action that links only this function's own dependencies into the artifacts directory.
+
+    Used when npm installed somewhere other than the function's directory, which is what npm does for a
+    workspaces monorepo: it hoists every workspace package's dependencies into one node_modules at the
+    monorepo root. Linking that whole directory would ship every sibling function's dependencies too, so
+    this asks npm which packages this function actually resolves and links those under their own names.
+
+    Every name comes from the directory npm installed the package in, not from the package's own manifest -
+    see `_link_name` for the alias that makes those two differ, and for why a manifest value does not belong
+    in a path. When npm cannot report a closure at all, `_link_every_installed_package` links the installed
+    packages one by one rather than the whole tree in one link, which is the only form of "ship everything"
+    that is genuinely a superset of what the function resolves.
+    """
+
+    NAME = "NpmLinkDependencyClosure"
+    DESCRIPTION = "Linking this function's dependencies into the artifacts directory"
+    PURPOSE = Purpose.LINK_SOURCE
+
+    def __init__(self, install_dir, project_root, artifacts_dir, subprocess_npm, osutils):
+        """
+        Parameters
+        ----------
+        install_dir : str
+            the directory npm ran in, whose project's closure is wanted
+        project_root : str
+            the directory npm installed into, linked whole if the closure cannot be resolved
+        artifacts_dir : str
+            an existing (writable) directory where node_modules is assembled
+        subprocess_npm : aws_lambda_builders.workflows.nodejs_npm.npm.SubprocessNpm
+            An instance of the NPM process wrapper
+        osutils : aws_lambda_builders.workflows.nodejs_npm.utils.OSUtils
+            An instance of OS Utilities for file manipulation
+        """
+        super(NodejsNpmLinkDependencyClosureAction, self).__init__()
+        self._install_dir = install_dir
+        self._project_root = project_root
+        self._artifacts_dir = artifacts_dir
+        self._subprocess_npm = subprocess_npm
+        self._osutils = osutils
+
+    def execute(self):
+        closure = self._subprocess_npm.resolve_dependency_closure(self._install_dir)
+        destination = os.path.join(self._artifacts_dir, "node_modules")
+
+        if closure is None:
+            self._link_every_installed_package(destination)
+            return
+
+        for name, package_dir in self._packages_by_name(self._outermost_packages(closure)).items():
+            self._link(package_dir, destination, name)
+
+    def _link_every_installed_package(self, destination):
+        """
+        Link every package npm installed, when npm could not say which ones this function resolves.
+
+        AN OVERLAY, NOT ONE LINK FOR THE WHOLE DIRECTORY. Linking `project_root/node_modules` as the
+        artifacts' `node_modules` looks simpler and is not a superset of what the function resolves: in a
+        workspaces install a member's dependency that conflicts with the hoisted version is installed
+        under the member's OWN `node_modules`, and a single link to the root's directory cannot carry it.
+        The function would then resolve the root's different version, or nothing at all for a package that
+        was never hoisted. Per-entry links can carry both, with the function's own copy winning, which is
+        what node resolution does for the function anyway.
+
+        It also removes the dangling link that one symlink produced: `create_symlink_or_copy` goes
+        straight to `os.symlink`, which succeeds against a missing target on POSIX, so a function with no
+        installed dependencies got a `node_modules` pointing nowhere - worse than the no-op it replaced,
+        because the copy that ships the artifacts then follows it.
+        """
+        # The function's own directory LAST, so its entries replace the hoisted ones of the same name.
+        # Resolved into one mapping BEFORE anything is linked, because `create_symlink_or_copy` returns
+        # early when the destination already exists as a symlink - linking in order would keep the first
+        # of each name, which is the opposite of the precedence this needs.
+        chosen = {}
+        for directory in (self._project_root, self._install_dir):
+            tree = os.path.join(directory, "node_modules")
+            if not os.path.isdir(tree):
+                LOG.debug("NODEJS no dependencies installed in %s, nothing to link from there", tree)
+                continue
+            LOG.debug("NODEJS linking every package installed in %s into the artifacts", tree)
+            chosen.update(dict(self._installed_packages(tree)))
+
+        if not chosen:
+            LOG.warning(
+                "No installed dependencies were found for %s; the artifacts will have no node_modules",
+                self._install_dir,
+            )
+        for name, package_dir in chosen.items():
+            self._link(package_dir, destination, name)
+
+    def _installed_packages(self, tree):
+        """
+        `(name, directory)` for every package directly inside one `node_modules`, scopes walked one level.
+
+        Entries beginning with a dot are skipped: `.package-lock.json` and `.bin` are npm's bookkeeping
+        rather than packages, and the name rule rejects them anyway.
+        """
+        for entry in sorted(os.listdir(tree)):
+            if entry.startswith("."):
+                continue
+            path = os.path.join(tree, entry)
+            if entry.startswith("@"):
+                if os.path.isdir(path):
+                    for scoped in sorted(os.listdir(path)):
+                        if not scoped.startswith("."):
+                            yield f"{entry}/{scoped}", os.path.join(path, scoped)
+                continue
+            yield entry, path
+
+    def _packages_by_name(self, package_dirs):
+        """
+        Map each package to the one name it will be linked under, resolving same-name collisions.
+
+        TWO PATHS CAN WANT ONE NAME, and until this existed whichever npm printed first won:
+        `create_symlink_or_copy` returns early when the destination already exists, so the other version
+        was silently dropped. The case is a function that pins its own version of a package the root also
+        hoists - `install_dir/node_modules/lodash` and `project_root/node_modules/lodash` - and neither is
+        nested inside the other, so both reach here.
+
+        The function's own copy wins, because that is what node resolves for the function's code. The
+        hoisted version is not lost for anyone else: every other package is linked as a symlink into the
+        real tree, so a dependency resolving `lodash` from inside its real directory still walks up to the
+        root's copy.
+
+        NOT FIXED BY TREATING `install_dir` AS A NESTING CONTAINER, which is the obvious reading of "the
+        nesting test never matches install_dir". `project_root` contains every hoisted package, so adding
+        it as a container would exclude all of them, and excluding the function's own pinned copy would
+        put back the bug this action exists to fix.
+        """
+        chosen = {}
+        for package_dir in package_dirs:
+            name = self._link_name(package_dir)
+            if name in chosen:
+                if self._is_inside(package_dir, self._install_dir):
+                    LOG.debug("NODEJS %s pins its own %s; it wins over %s", self._install_dir, name, chosen[name])
+                    chosen[name] = package_dir
+                else:
+                    # Logged rather than silently resolved: two copies of one name that are not the
+                    # function's own is npm reporting something this rule does not model.
+                    LOG.warning(
+                        "Two installed copies of %s claim the same name; keeping %s and ignoring %s",
+                        name,
+                        chosen[name],
+                        package_dir,
+                    )
+                continue
+            chosen[name] = package_dir
+        return chosen
+
+    def _link_name(self, package_dir):
+        """
+        The name node must find this package under.
+
+        FROM THE PATH, NOT THE MANIFEST, wherever npm installed it. npm supports aliases -
+        `"lodash4": "npm:lodash@^4.0.0"` installs at `node_modules/lodash4` while the manifest inside
+        still says `"name": "lodash"` - so reading the manifest renames the package and `require("lodash4")`
+        fails with `Cannot find module`, which is the very failure this action exists to prevent. The
+        directory npm chose is the name npm resolves, so the segments after the last `node_modules`
+        component are the answer, scope included.
+
+        It also keeps an untrusted value out of a filesystem path. The manifest belongs to a third-party
+        dependency, and `"name": "../../../evil"` or `"/tmp/x"` joined onto the destination writes outside
+        the artifacts directory.
+
+        The manifest is still the only source for a path that is NOT under node_modules - a workspace
+        dependency, which npm reports as its own source directory, whose basename need not be its name.
+        That value is validated like any other.
+        """
+        segments = os.path.normpath(package_dir).split(os.sep)
+        for index in range(len(segments) - 1, -1, -1):
+            if os.path.normcase(segments[index]) == "node_modules":
+                return self._validated_name("/".join(segments[index + 1 :]), package_dir)
+
+        try:
+            name = self._osutils.parse_json(os.path.join(package_dir, "package.json"))["name"]
+        except (OSError, ValueError, KeyError) as ex:
+            # a directory npm named but that carries no readable manifest cannot be placed under a
+            # name, and guessing one from the path is how a package lands where node will not find it
+            raise ActionFailedError(f"Cannot read the package name of {package_dir}: {ex}")
+        return self._validated_name(name, package_dir)
+
+    @staticmethod
+    def _validated_name(name, package_dir):
+        if not isinstance(name, str) or not NODE_MODULES_PACKAGE_NAME.match(name):
+            raise ActionFailedError(f"{package_dir} claims the unusable package name {name!r}")
+        return name
+
+    def _link(self, package_dir, destination, name):
+        link_path = os.path.join(destination, *name.split("/"))
+        # Belt and braces behind the name rule: that rule already makes an escape unconstructible, and
+        # this says so in a way that survives someone relaxing it.
+        #
+        # THE PARENT IS RESOLVED, NOT THE LINK ITSELF. realpath on link_path follows a link that is
+        # already there and reports its TARGET, which sits outside the artifacts by design - so checking
+        # that would refuse every legitimate re-link. Resolving the directory it will be created in, and
+        # appending the final component unresolved, asks the actual question: where will this land.
+        # NORMALISED, because commonpath compares components without interpreting them: a final `..`
+        # stays a literal component, so `<destination>/..` reads as contained and the link lands on the
+        # artifacts directory itself. Measured while mutation-testing this guard with the name rule
+        # relaxed - the only one of four hostile names it then still caught was `../../../evil`.
+        real_destination = os.path.realpath(destination)
+        landing = os.path.normpath(
+            os.path.join(os.path.realpath(os.path.dirname(link_path)), os.path.basename(link_path))
+        )
+        try:
+            contained = os.path.commonpath([real_destination, landing]) == real_destination
+        except ValueError:
+            # different drives on Windows, which is an escape by definition rather than an error to pass on
+            contained = False
+        if not contained:
+            raise ActionFailedError(f"{package_dir} would be linked outside the artifacts as {name!r}")
+        os.makedirs(os.path.dirname(link_path), exist_ok=True)
+        utils.create_symlink_or_copy(package_dir, link_path)
+
+    @staticmethod
+    def _is_inside(path, directory):
+        parent = os.path.normcase(os.path.realpath(directory))
+        return os.path.normcase(os.path.realpath(path)).startswith(parent + os.sep)
+
+    def _outermost_packages(self, closure):
+        """
+        Keep the packages that need their own entry in node_modules.
+
+        npm reports the project itself, which is not one of its own dependencies, and it reports a nested
+        copy of a package that a dependency pins to a different version. A nested copy must stay where it
+        is - hoisting it would shadow the top-level version for every other caller - and it is already
+        reachable through the dependency that contains it, so only the outermost paths are linked.
+        """
+        # Every comparison below goes through normcase, never the raw path: these paths are npm's
+        # spelling while project_root and install_dir are the build's, and on Windows two spellings
+        # differing only in case name the same directory. An unmatched project root would be linked as
+        # if it were one of its own dependencies, and an unrecognised nested copy would be hoisted to
+        # the top level, shadowing the version every other caller resolves. The original paths are what
+        # gets linked, so only the comparisons are normalised. No-op off Windows.
+        paths = [os.path.realpath(path) for path in closure]
+        excluded = {os.path.normcase(os.path.realpath(d)) for d in (self._project_root, self._install_dir)}
+        candidates = [path for path in paths if os.path.normcase(path) not in excluded]
+
+        def is_nested_in_another(path):
+            key = os.path.normcase(path)
+            return any(
+                key != os.path.normcase(other) and key.startswith(os.path.normcase(other) + os.sep)
+                for other in candidates
+            )
+
+        return [path for path in candidates if not is_nested_in_another(path)]

aws_lambda_builders/workflows/nodejs_npm/npm.py

@@ -3,6 +3,7 @@
 """
 
 import logging
+from typing import Dict, List, Optional
 
 from aws_lambda_builders.workflows.nodejs_npm.exceptions import NpmExecutionError
 
@@ -33,6 +34,66 @@ def __init__(self, osutils, npm_exe=None):
                 npm_exe = "npm"
 
         self.npm_exe = npm_exe
+        self._project_root_cache: Dict[str, Optional[str]] = {}
+
+    def resolve_project_root(self, cwd: str) -> Optional[str]:
+        """
+        Ask npm which directory it treats as the project root when it runs in ``cwd``, caching the answer.
+
+        `npm prefix` walks up to the nearest directory holding a package.json, which for a workspace
+        package is the monorepo root - where npm keeps the single lockfile and hoists node_modules to -
+        and is the directory itself for any other package. Both the lockfile lookup that selects the
+        install command and the link step that points the artifacts at the installed dependencies need
+        that answer for the same directory, so it is resolved once per directory rather than per caller.
+
+        Parameters
+        ----------
+        cwd : str
+            the directory npm will run in
+
+        Returns
+        -------
+        Optional[str]
+            npm's project root, or None when npm could not be asked
+        """
+        if cwd not in self._project_root_cache:
+            try:
+                self._project_root_cache[cwd] = self.run(["prefix"], cwd=cwd).strip()
+            except NpmExecutionError as ex:
+                LOG.debug("NODEJS could not resolve the npm project root of %s: %s", cwd, ex)
+                self._project_root_cache[cwd] = None
+
+        return self._project_root_cache[cwd]
+
+    def resolve_dependency_closure(self, cwd: str) -> Optional[List[str]]:
+        """
+        Ask npm for every package the project in ``cwd`` actually resolves, production only.
+
+        `npm ls --all --parseable --omit=dev` walks the installed tree and prints one absolute path per
+        resolved package. In a workspaces monorepo that answers a question the directory layout cannot:
+        which of the packages hoisted to the monorepo root belong to THIS function, and which belong to
+        a sibling. The paths are real paths, so a workspace dependency is reported as its own source
+        directory rather than as the link under node_modules.
+
+        Parameters
+        ----------
+        cwd : str
+            the directory whose project npm should report on
+
+        Returns
+        -------
+        Optional[List[str]]
+            the resolved package directories, or None when npm could not answer. npm exits non-zero for
+            any tree it considers incomplete (missing peer, invalid version) and its partial output is
+            not worth trusting, so callers fall back to shipping the whole installed tree instead.
+        """
+        try:
+            output = self.run(["ls", "--all", "--parseable", "--omit=dev"], cwd=cwd)
+        except NpmExecutionError as ex:
+            LOG.debug("NODEJS could not resolve the dependency closure of %s: %s", cwd, ex)
+            return None
+
+        return [line.strip() for line in output.splitlines() if line.strip()]
 
     def run(self, args, cwd=None):
         """