Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions samcli/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,12 @@
"""

from samcli.cli.main import cli # pragma: no cover
from samcli.lib.utils.hook_script import run_hook_script_if_requested # pragma: no cover

if __name__ == "__main__": # pragma: no cover
# A bundle runs cookiecutter's Python hooks by re-launching itself, so claim those invocations
# before the CLI treats the script path as a command name.
run_hook_script_if_requested()
# NOTE(TheSriram): prog_name is always set to "sam". This way when the CLI is invoked as a module,
# the help text that is generated still says "sam" instead of "__main__".
cli(prog_name="sam")
17 changes: 10 additions & 7 deletions samcli/lib/cookiecutter/template.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import logging
from typing import Dict, List, Optional

from cookiecutter import hooks as cookiecutter_hooks
from cookiecutter.exceptions import RepositoryNotFound, UnknownRepoType
from cookiecutter.main import cookiecutter

Expand All @@ -20,6 +21,7 @@
from samcli.lib.cookiecutter.plugin import Plugin
from samcli.lib.cookiecutter.processor import Processor
from samcli.lib.init.arbitrary_project import generate_non_cookiecutter_project
from samcli.lib.utils.hook_script import patched_hook_runner

LOG = logging.getLogger(__name__)

Expand Down Expand Up @@ -167,13 +169,14 @@ def generate_project(self, context: Dict, output_dir: str) -> None:

try:
LOG.debug("Baking a new template with cookiecutter with all parameters")
cookiecutter(
template=self._location,
output_dir=output_dir,
no_input=True,
extra_context=context,
overwrite_if_exists=True,
)
with patched_hook_runner(cookiecutter_hooks):
cookiecutter(
template=self._location,
output_dir=output_dir,
no_input=True,
extra_context=context,
overwrite_if_exists=True,
)
except RepositoryNotFound:
# cookiecutter.json is not found in the template. Let's just clone it directly without
# using cookiecutter and call it done.
Expand Down
5 changes: 4 additions & 1 deletion samcli/lib/init/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from pathlib import Path
from typing import Dict, Optional

from cookiecutter import hooks as cookiecutter_hooks
from cookiecutter.exceptions import CookiecutterException, RepositoryNotFound, UnknownRepoType
from cookiecutter.main import cookiecutter

Expand All @@ -22,6 +23,7 @@
from samcli.lib.init.template_modifiers.xray_tracing_template_modifier import XRayTracingTemplateModifier
from samcli.lib.telemetry.event import EventName, EventTracker, UsedFeature
from samcli.lib.utils import osutils
from samcli.lib.utils.hook_script import patched_hook_runner
from samcli.lib.utils.packagetype import ZIP
from samcli.local.common.runtime_template import RUNTIME_DEP_TEMPLATE_MAPPING, is_custom_runtime

Expand Down Expand Up @@ -119,7 +121,8 @@ def generate_project(
LOG.debug("Baking a new template with cookiecutter with all parameters")
# cookiecutter returns the directory it created, which is the only reliable way to know
# where the project landed when the template chooses its own project directory name.
project_directory = cookiecutter(**params)
with patched_hook_runner(cookiecutter_hooks):
Comment thread
roger-zhangg marked this conversation as resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[GENERAL] The two call sites this fix depends on have no test guarding them, and the failure mode is silent.

patched_hook_runner() is a no-op unless is_pyinstaller_bundle() is true, so every existing unit test in tests/unit/lib/init/test_init.py and tests/unit/lib/cookiecutter/test_template.py passes identically whether or not the with wrapper is present. The new tests/unit/lib/utils/test_hook_script.py covers the helper thoroughly but never asserts that anything applies it. That leaves the wiring — the part that actually fixes the bug — unverified at both samcli/lib/init/__init__.py:124 and samcli/lib/cookiecutter/template.py:172.

This is not hypothetical: the wrapper was initially applied to only one of the two cookiecutter() calls, and it took a review pass to catch it. A future refactor that drops or reorders the with block reintroduces the original silent bug (hook skipped, project generated wrong, exit 0) with no failing test, and the bundle path is not exercised by CI — validate_pyinstaller.yml only builds the binary.

A cheap regression guard in each existing test module:

@patch("samcli.lib.init.cookiecutter")
@patch("samcli.lib.init.patched_hook_runner")
def test_hook_runner_is_patched_around_cookiecutter(self, patched_hook_runner_mock, cookiecutter_patch):
   generate_project(location="/path", output_dir=".", name="sam-app")
   patched_hook_runner_mock.assert_called_once_with(cookiecutter_hooks)

An equivalent test against samcli.lib.cookiecutter.template.patched_hook_runner covers the sam pipeline init path. If you want the assertion to also prove ordering (that the patch is active while cookiecutter() runs, not merely entered), have the mock's __enter__ record into a list that the cookiecutter mock's side_effect also appends to.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid, fixed in 8a2fb4b. Your framing of the risk was right and the evidence you cite is fair — the missed second call site was caught by review, not by a test.

Added a guard to each existing module: test_hook_runner_is_active_while_cookiecutter_runs in tests/unit/lib/init/test_init.py and in tests/unit/lib/cookiecutter/test_template.py. I took the stronger ordering variant you suggested rather than a bare assert_called_once_with, so it proves the patch is active while cookiecutter() runs: __enter__, __exit__ and the cookiecutter mock all append to one list, asserted as ["enter", "cookiecutter", "exit"].

Confirmed the guards actually guard, by simulating the refactor you describe and deleting the with block at both call sites:

FAILED tests/unit/lib/init/test_init.py::TestInit::test_hook_runner_is_active_while_cookiecutter_runs
FAILED tests/unit/lib/cookiecutter/test_template.py::TestTemplate::test_hook_runner_is_active_while_cookiecutter_runs
2 failed, 20 passed

The 20 passed is your point restated: every other test in those modules is indifferent to the wrapper being present.

You are also right that CI never exercises the bundle path — validate_pyinstaller.yml only builds the binary. I verified the two interpreter paths locally against a simulated bundle instead, but that is not a substitute for CI coverage, and closing that gap is worth its own issue.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction to the reply above: the commit is a7b00a0fc, not 8a2fb4b.

project_directory = cookiecutter(**params)
# Fixes gradlew line ending issue caused by Windows git
# gradlew is a shell script which should not have CR LF line endings
# Putting the conversion after cookiecutter as cookiecutter processing will also change the line endings
Expand Down
136 changes: 136 additions & 0 deletions samcli/lib/utils/hook_script.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
"""
Support for running cookiecutter template hooks from a PyInstaller bundle.

Cookiecutter runs a Python hook as ``[sys.executable, script]``. In a bundle ``sys.executable`` is
the sam executable itself, so the hook never runs. This module supplies a real interpreter: a system
python3 when one is available, otherwise this executable re-launched in hook mode.
"""

import logging
import os
import runpy
import shutil
import subprocess
import sys
from contextlib import contextmanager
from types import ModuleType
from typing import Iterator, Optional

from samcli.lib.utils.subprocess_utils import is_pyinstaller_bundle, isolate_library_paths_for_subprocess

LOG = logging.getLogger(__name__)

# Set on the hook subprocess so a re-launched bundle runs the script instead of parsing a command name.
HOOK_SCRIPT_ENV_VAR = "SAM_CLI_RUN_HOOK_SCRIPT"

# A bundle ships no interpreter of its own. A system one is preferred for isolation, so that SAM's
# bundled dependencies do not become an implicit contract for template authors -- not for fidelity
# with a pip install, where hooks can in fact import SAM's dependencies. The order matches
# _get_python_command_name in the terraform prepare hook, and includes the Windows launcher because
# a default python.org install puts only py.exe on PATH.
_INTERPRETER_CANDIDATES = ("python3", "py3", "python", "py")
# Matches requires-python, so a hook never sees an older Python than a pip install would give it.
_MINIMUM_PYTHON_VERSION = (3, 10)
_PROBE_TIMEOUT = 10


def run_hook_script_if_requested() -> None:
"""Run the script named in argv and exit, when re-launched by the patched hook runner."""
# Popped rather than read so a hook that shells out to sam again gets the normal CLI.
requested = os.environ.pop(HOOK_SCRIPT_ENV_VAR, None) == "1"
arguments = sys.argv[1:]
if not requested or not arguments:
return

LOG.debug("Running template hook script %s through this executable", arguments[0])
# The bootloader re-points library paths into the bundle for this process, and the CLI callback
# that normally undoes that is never reached here. Hooks routinely shell out to git, npm and pip.
isolate_library_paths_for_subprocess()
# A hook launched by a real interpreter sees only its own path in argv; run_path fixes argv[0]
# but would leave our second argument behind, so give the hook the argv it expects.
with _replaced_attribute(sys, "argv", [arguments[0]]):
runpy.run_path(arguments[0], run_name="__main__")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[GENERAL] The two interpreter paths give hooks different import environments, which makes the bundled dependencies an implicit contract on exactly the hosts the fallback serves.

The constant block states the intent for preferring a system interpreter:

# A bundle ships no interpreter of its own. A system one is preferred for isolation, so that SAM's
# bundled dependencies do not become an implicit contract for template authors ...

The fallback grants precisely that access. runpy.run_path executes the hook in-process inside the frozen interpreter, whose sys.path is the bundle, so the frozen importer resolves anything PyInstaller collected — yaml, jinja2, click, boto3, and samcli itself. Under the system-interpreter branch the hook runs out-of-process against a bare /usr/bin/python3, where those imports fail.

The observable consequence is that the same template behaves differently on two hosts running the same sam build, keyed on something the template author cannot see: whether a system Python ≥ 3.10 happens to be on PATH. A hook containing import yaml authored and tested on a host with only the native installer succeeds; the same hook on a host with python3 installed fails with an ImportError surfacing as cookiecutter's generic FailedHookException (exit status 1), with nothing pointing at the interpreter choice as the cause.

Two ways to close the gap, both consistent with the stated intent:

  • Have the fallback reduce the hook's import surface before executing it — for example running runpy.run_path with sys.path restricted to the script's own directory plus the stdlib, so a hook cannot reach bundled third-party packages regardless of which branch ran. Note this narrows but does not fully close the difference, since modules already in sys.modules from importing samcli.cli.main stay importable.
  • Alternatively, if the divergence is acceptable, say so in the module docstring and drop the isolation claim from the constant comment, so the next reader does not treat isolation as a guarantee the fallback silently breaks.

Either way the current pairing is self-contradictory: the comment promises a property that the code below it does not hold on the fallback path.

sys.exit(0)


def find_system_interpreter() -> Optional[str]:
"""Return the path to a usable system Python 3, or None if there isn't one."""
for candidate in _INTERPRETER_CANDIDATES:
path = shutil.which(candidate)
if not path or os.path.realpath(path) == os.path.realpath(sys.executable):
continue
# Executed rather than trusted because Windows ships a "python" App Execution Alias that
# resolves on PATH without being an interpreter, and because /usr/bin/python3 is 3.6 on
# older distributions, where a hook using newer syntax would fail with a SyntaxError.
try:
completed = subprocess.run(
[path, "-c", f"import sys; sys.exit(0 if sys.version_info >= {_MINIMUM_PYTHON_VERSION} else 1)"],
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
timeout=_PROBE_TIMEOUT,
check=False,
)
except (OSError, subprocess.SubprocessError):
continue
if completed.returncode == 0:
return path
return None


@contextmanager
def _replaced_attribute(target: object, name: str, value: object) -> Iterator[None]:
"""Set an attribute for the duration of the block, restoring whatever was there before."""
original = getattr(target, name)
setattr(target, name, value)
try:
yield
finally:
setattr(target, name, original)


@contextmanager
def _hook_script_env() -> Iterator[None]:
"""Mark the environment so the re-launched executable runs the hook script."""
original = os.environ.get(HOOK_SCRIPT_ENV_VAR)
os.environ[HOOK_SCRIPT_ENV_VAR] = "1"
try:
yield
finally:
if original is None:
os.environ.pop(HOOK_SCRIPT_ENV_VAR, None)
else:
os.environ[HOOK_SCRIPT_ENV_VAR] = original


@contextmanager
def patched_hook_runner(hooks_module: ModuleType) -> Iterator[None]:
"""Make cookiecutter's Python hooks runnable while frozen; a no-op when not frozen.

The module is passed in so this stays importable without pulling in cookiecutter, which every
sam invocation would otherwise pay for at startup.
"""
if not is_pyinstaller_bundle():
yield
return

original_run_script = hooks_module.run_script

def run_script(script_path: str, cwd: str = ".") -> None:
# Only .py hooks go through an interpreter; anything else already runs on its own.
if not script_path.endswith(".py"):
original_run_script(script_path, cwd)
return

interpreter = find_system_interpreter()
if interpreter:
LOG.debug("Running template hook with system interpreter %s", interpreter)
with _replaced_attribute(sys, "executable", interpreter):
original_run_script(script_path, cwd)
return

LOG.debug("No system interpreter found, re-launching this executable to run the template hook")
with _hook_script_env():
original_run_script(script_path, cwd)

with _replaced_attribute(hooks_module, "run_script", run_script):
yield
2 changes: 1 addition & 1 deletion tests/integration/init/test_init_command.py
Original file line number Diff line number Diff line change
Expand Up @@ -684,7 +684,7 @@ def _assert_template_with_cfn_lint(self, cwd):
You can run 'sam init' without any options for an interactive initialization flow, or you can provide one of the following required parameter combinations:
\t--name, --location, or
\t--name, --package-type, --base-image, or
\t--name, --runtime, --app-template, --dependency-manager
\t--name, --runtime, --dependency-manager, --app-template
"""


Expand Down
Loading
Loading