diff --git a/esphome/espidf/framework.py b/esphome/espidf/framework.py index b54a0c294b5..cfbae9ea46b 100644 --- a/esphome/espidf/framework.py +++ b/esphome/espidf/framework.py @@ -397,9 +397,10 @@ def _clone_idf_with_submodules( handles branches, tags, and SHAs uniformly (mirrors the approach in ``esphome.git.clone_or_update``). """ - from esphome.git import run_git_command + from esphome.git import run_git_command, update_submodules - _LOGGER.info("Cloning ESP-IDF from %s%s", git_url, f"@{ref}" if ref else "") + key = f"{git_url}@{ref}" if ref else git_url + _LOGGER.info("Cloning ESP-IDF from %s", key) run_git_command(["git", "clone", "--depth=1", "--", git_url, str(framework_path)]) if ref: run_git_command( @@ -410,25 +411,14 @@ def _clone_idf_with_submodules( ["git", "reset", "--hard", "FETCH_HEAD"], git_dir=framework_path, ) - run_git_command( - [ - "git", - "submodule", - "update", - "--init", - "--recursive", - "--depth=1", - ], - git_dir=framework_path, - ) + update_submodules(framework_path, key) - # Sanity-check the resulting tree. run_git_command only raises when - # stderr is non-empty, so a clone that silently produces no working - # tree would otherwise be marked extracted and stuck until - # ``esphome clean``. + # Sanity-check the resulting tree: a clone can exit 0 yet produce no + # usable ESP-IDF checkout, which would otherwise be marked extracted and + # stuck until ``esphome clean``. if not (framework_path / "tools" / "idf_tools.py").is_file(): raise RuntimeError( - f"Clone of {git_url} produced no usable ESP-IDF tree at {framework_path}" + f"Clone of {key} produced no usable ESP-IDF tree at {framework_path}" ) diff --git a/esphome/git.py b/esphome/git.py index 0c1ad563677..46cce50d9db 100644 --- a/esphome/git.py +++ b/esphome/git.py @@ -2,6 +2,7 @@ from collections.abc import Callable from dataclasses import dataclass import hashlib import logging +import os from pathlib import Path import re import subprocess @@ -11,7 +12,7 @@ import urllib.parse import esphome.config_validation as cv from esphome.core import CORE, EsphomeError, TimePeriodSeconds -from esphome.helpers import rmtree, write_file +from esphome.helpers import add_git_ceiling_directory, rmtree, write_file _LOGGER = logging.getLogger(__name__) @@ -26,6 +27,24 @@ NEVER_REFRESH = TimePeriodSeconds(seconds=-1) # it does not pollute the worktree. _CLONE_COMPLETE_MARKER = "esphome_clone_complete" +# Environment variables that scope git to a specific repository. Git hooks and +# some CI wrappers export these; if they leak into the git commands run here, +# git binds to the caller's repository instead of the one being managed. The +# effects range from loud (`git clone` producing a bare-style directory with +# no working tree) to silent (an ambient GIT_INDEX_FILE makes +# `git submodule update --init` exit 0 without initializing anything). +_GIT_REPO_SCOPING_ENV = frozenset( + { + "GIT_DIR", + "GIT_WORK_TREE", + "GIT_INDEX_FILE", + "GIT_OBJECT_DIRECTORY", + "GIT_ALTERNATE_OBJECT_DIRECTORIES", + "GIT_COMMON_DIR", + "GIT_NAMESPACE", + } +) + class GitException(cv.Invalid): """Base exception for git-related errors.""" @@ -43,32 +62,61 @@ class GitRepositoryError(GitException): """Exception raised when a git repository is in an invalid state.""" -def run_git_command(cmd: list[str], git_dir: Path | None = None) -> str: - if git_dir is not None: - _LOGGER.debug( - "Running git command with repository isolation: %s (git_dir=%s)", - " ".join(cmd), - git_dir, - ) - else: - _LOGGER.debug("Running git command: %s", " ".join(cmd)) +def _redact_url_credentials(text: str) -> str: + """Mask userinfo in any URLs embedded in ``text``. - # Set up environment for repository isolation if git_dir is provided - # Force git to only operate on this specific repository by setting - # GIT_DIR and GIT_WORK_TREE. This prevents git from walking up the - # directory tree to find parent repositories when the target repo's - # .git directory is corrupt. Without this, commands like 'git stash' - # could accidentally operate on parent repositories (e.g., the main - # ESPHome repo) instead of failing, causing data loss. - env: dict[str, str] | None = None - cwd: str | None = None + Users can put credentials directly in a git URL, and log output is + routinely pasted into public issues. + """ + return re.sub(r"://[^/@\s]+@", "://***@", text) + + +def run_git_command( + cmd: list[str], git_dir: Path | None = None, *, cwd: Path | None = None +) -> str: + """Run a git command and return its stdout. + + The repository-scoping environment variables in ``_GIT_REPO_SCOPING_ENV`` + are always stripped. ``git_dir`` additionally pins GIT_DIR/GIT_WORK_TREE + to that repository and runs the command there; ``cwd`` alone runs the + command in that directory with GIT_CEILING_DIRECTORIES capping repository + discovery at its parent. + """ + # Every invocation starts from an environment with the repository-scoping + # variables stripped (see _GIT_REPO_SCOPING_ENV) so a git hook or CI + # wrapper invoking ESPHome can never redirect these commands to its own + # repository or index. + # + # ``git_dir`` then re-adds GIT_DIR and GIT_WORK_TREE pointing at the + # managed repository. This prevents git from walking up the directory + # tree to find parent repositories when the target repo's .git directory + # is corrupt. Without this, commands like 'git stash' could accidentally + # operate on parent repositories (e.g., the main ESPHome repo) instead of + # failing, causing data loss. + # + # ``cwd`` (without ``git_dir``) runs the command in that directory + # without GIT_DIR/GIT_WORK_TREE. The ``git submodule`` porcelain needs + # this: on some installations (e.g. Windows setups where a shim hands + # git untranslated paths) it refuses to run when GIT_DIR/GIT_WORK_TREE + # are set, failing with "cannot be used without a working tree". + # GIT_CEILING_DIRECTORIES (which git only honors as an absolute path) + # keeps the parent-repo-walk protection instead: if the repo's .git is + # missing or corrupt, git fails rather than discovering an enclosing + # repository. + env = {k: v for k, v in os.environ.items() if k not in _GIT_REPO_SCOPING_ENV} if git_dir is not None: - env = { - **subprocess.os.environ, - "GIT_DIR": str(Path(git_dir) / ".git"), - "GIT_WORK_TREE": str(git_dir), - } - cwd = str(git_dir) + env["GIT_DIR"] = str(Path(git_dir) / ".git") + env["GIT_WORK_TREE"] = str(git_dir) + cwd = git_dir + elif cwd is not None: + add_git_ceiling_directory(env, Path(cwd).absolute().parent) + + _LOGGER.debug( + "Running git command: %s (cwd=%s, isolated=%s)", + _redact_url_credentials(" ".join(cmd)), + cwd, + git_dir is not None, + ) try: ret = subprocess.run( @@ -86,12 +134,17 @@ def run_git_command(cmd: list[str], git_dir: Path | None = None) -> str: "for installation instructions." ) from err - if ret.returncode != 0 and ret.stderr: - err_str = ret.stderr.decode("utf-8") - lines = [x.strip() for x in err_str.splitlines()] - if lines[-1].startswith("fatal:"): - raise GitCommandError(lines[-1][len("fatal: ") :]) - raise GitCommandError(err_str) + if ret.returncode != 0: + if ret.stderr: + err_str = ret.stderr.decode("utf-8") + lines = [x.strip() for x in err_str.splitlines()] + if lines[-1].startswith("fatal:"): + raise GitCommandError(lines[-1][len("fatal: ") :]) + raise GitCommandError(err_str) + raise GitCommandError( + f"git exited with code {ret.returncode}: " + f"{_redact_url_credentials(' '.join(cmd))}" + ) return ret.stdout.decode("utf-8").strip() @@ -123,6 +176,27 @@ def _remove_repo_dir(repo_dir: Path) -> None: rmtree(repo_dir) +def update_submodules(repo_dir: Path, key: str) -> None: + """Initialize/update every submodule the repository declares, recursively, + matching how PlatformIO clones libraries. + + Most repositories declare no submodules, so this does nothing when there + is no ``.gitmodules`` file. Which submodules get populated is git's own + policy (``update = none``, ``submodule.active``, sparse checkouts); + git's exit code is the error signal. + + Runs with plain ``cwd`` rather than ``git_dir`` isolation, which the + ``git submodule`` porcelain does not tolerate (see ``run_git_command``). + """ + if not (repo_dir / ".gitmodules").is_file(): + return + _LOGGER.info("Updating submodules for %s", _redact_url_credentials(key)) + run_git_command( + ["git", "submodule", "update", "--init", "--recursive", "--depth=1"], + cwd=repo_dir, + ) + + def resolve_symlink_stub(repo_dir: Path, file_path: Path) -> Path | None: """Return the symlink target if ``file_path`` is a Windows-checked-out symlink stub. @@ -217,12 +291,19 @@ def clone_or_update( domain: str, username: str = None, password: str = None, - submodules: list[str] | None = None, + init_submodules: bool = False, subpath: Path | None = None, _recover_broken: bool = True, ) -> tuple[Path, Callable[[], None] | None]: key = f"{url}@{ref}" + # The user may have embedded credentials in the URL itself; log this + # instead of key. + safe_key = _redact_url_credentials(key) + # Keep the caller's URL for the recovery re-clone below: rewriting the + # rewritten URL would double the userinfo, and the recursive call must + # compute the same cache key as this one. + original_url = url if username is not None and password is not None: url = url.replace( "://", f"://{urllib.parse.quote(username)}:{urllib.parse.quote(password)}@" @@ -238,12 +319,12 @@ def clone_or_update( # predates the marker; either way it cannot be trusted, especially # with NEVER_REFRESH where it would otherwise be reused forever. _LOGGER.warning( - "Removing incomplete clone of %s at %s, will re-clone", key, repo_dir + "Removing incomplete clone of %s at %s, will re-clone", safe_key, repo_dir ) _remove_repo_dir(repo_dir) if not repo_dir.is_dir(): - _LOGGER.info("Cloning %s", key) + _LOGGER.info("Cloning %s", safe_key) _LOGGER.debug("Location: %s", repo_dir) try: cmd = ["git", "clone", "--depth=1"] @@ -262,15 +343,8 @@ def clone_or_update( ["git", "reset", "--hard", "FETCH_HEAD"], git_dir=repo_dir ) - if submodules is not None: - _LOGGER.info( - "Initializing submodules (%s) for %s", ", ".join(submodules), key - ) - run_git_command( - ["git", "submodule", "update", "--init", "--depth=1", "--"] - + submodules, - git_dir=repo_dir, - ) + if init_submodules: + update_submodules(repo_dir, key) except GitException: # Remove incomplete clone to prevent stale state. Without this, @@ -290,12 +364,12 @@ def clone_or_update( ) except EsphomeError as err: _LOGGER.warning( - "Could not write clone completion marker for %s: %s", key, err + "Could not write clone completion marker for %s: %s", safe_key, err ) else: if refresh == NEVER_REFRESH or CORE.skip_external_update: - _LOGGER.debug("Skipping update for %s (refresh disabled)", key) + _LOGGER.debug("Skipping update for %s (refresh disabled)", safe_key) return repo_dir, None file_timestamp = Path(repo_dir / ".git" / "FETCH_HEAD") @@ -319,7 +393,7 @@ def clone_or_update( ["git", "rev-parse", "HEAD"], git_dir=repo_dir ) - _LOGGER.info("Updating %s", key) + _LOGGER.info("Updating %s", safe_key) _LOGGER.debug("Location: %s", repo_dir) # Stash local changes (if any) @@ -345,19 +419,25 @@ def clone_or_update( ["git", "reset", "--hard", "FETCH_HEAD"], git_dir=repo_dir, ) + + # Inside the try so a submodule failure routes through the + # recovery re-clone below instead of leaving a repo that the + # refresh window would silently accept on the next run. + if init_submodules: + update_submodules(repo_dir, key) except GitException as err: # Repository is in a broken state or update failed # Only attempt recovery once to prevent infinite recursion if not _recover_broken: _LOGGER.error( "Repository %s recovery failed, cannot retry (already attempted once)", - key, + safe_key, ) raise _LOGGER.warning( "Repository %s has issues (%s), attempting recovery", - key, + safe_key, err, ) _LOGGER.info("Removing broken repository at %s", repo_dir) @@ -367,31 +447,21 @@ def clone_or_update( # Recursively call clone_or_update to re-clone # Set _recover_broken=False to prevent infinite recursion result = clone_or_update( - url=url, + url=original_url, ref=ref, refresh=refresh, domain=domain, username=username, password=password, - submodules=submodules, + init_submodules=init_submodules, subpath=subpath, _recover_broken=False, ) - _LOGGER.info("Repository %s successfully recovered", key) + _LOGGER.info("Repository %s successfully recovered", safe_key) return result - if submodules is not None: - _LOGGER.info( - "Updating submodules (%s) for %s", ", ".join(submodules), key - ) - run_git_command( - ["git", "submodule", "update", "--init", "--depth=1", "--"] - + submodules, - git_dir=repo_dir, - ) - def revert(): - _LOGGER.info("Reverting changes to %s -> %s", key, old_sha) + _LOGGER.info("Reverting changes to %s -> %s", safe_key, old_sha) run_git_command(["git", "reset", "--hard", old_sha], git_dir=repo_dir) return repo_dir, revert diff --git a/esphome/platformio/library.py b/esphome/platformio/library.py index 7c8566b77a0..1a523ce0abf 100644 --- a/esphome/platformio/library.py +++ b/esphome/platformio/library.py @@ -134,7 +134,7 @@ class GitSource(Source): ref=self.ref, refresh=git.NEVER_REFRESH if not force else None, domain=domain, - submodules=[], + init_submodules=True, subpath=Path(dir_suffix), ) return path diff --git a/tests/unit_tests/test_espidf_framework.py b/tests/unit_tests/test_espidf_framework.py index eb68b175728..cbc9fe2cda1 100644 --- a/tests/unit_tests/test_espidf_framework.py +++ b/tests/unit_tests/test_espidf_framework.py @@ -137,10 +137,17 @@ def test_parse_git_source_rejected(source: str) -> None: assert _parse_git_source(source) is None -def _make_idf_tree(framework_path: Path) -> None: - """Create the minimum tree _clone_idf_with_submodules sanity-checks for.""" +def _make_idf_tree(framework_path: Path, *, gitmodules: bool = True) -> None: + """Create the minimum tree _clone_idf_with_submodules sanity-checks for. + + ``gitmodules=False`` simulates a fork that vendors components in-tree + instead of declaring submodules; update_submodules skips the git call + when that file is missing. + """ (framework_path / "tools").mkdir(parents=True) (framework_path / "tools" / "idf_tools.py").write_text("# stub\n") + if gitmodules: + (framework_path / ".gitmodules").write_text("# stub\n") def test_clone_idf_with_submodules_without_ref(tmp_path: Path) -> None: @@ -214,6 +221,28 @@ def test_clone_idf_with_submodules_raises_when_tree_missing( ) +def test_clone_idf_accepts_flattened_fork_without_gitmodules( + tmp_path: Path, +) -> None: + """A fork that vendors components in-tree instead of as submodules is valid. + + No .gitmodules means the submodule step is skipped entirely. + """ + framework_path = tmp_path / "idf" + framework_path.mkdir() + _make_idf_tree(framework_path, gitmodules=False) + + with patch("esphome.git.run_git_command", return_value="") as run_git_command_mock: + _clone_idf_with_submodules( + framework_path, + "https://github.com/example/flattened-esp-idf.git", + None, + ) + + calls = [c.args[0] for c in run_git_command_mock.call_args_list] + assert not any(c[1] == "submodule" for c in calls) + + # --------------------------------------------------------------------------- # Helpers for _tar_extract_all hard-link prefix-stripping tests # --------------------------------------------------------------------------- diff --git a/tests/unit_tests/test_git.py b/tests/unit_tests/test_git.py index c9e0339ad75..858eee5e9f8 100644 --- a/tests/unit_tests/test_git.py +++ b/tests/unit_tests/test_git.py @@ -1,8 +1,10 @@ """Tests for git.py module.""" from collections.abc import Callable +import logging import os from pathlib import Path +import subprocess import time from typing import Any from unittest.mock import Mock, patch @@ -71,19 +73,40 @@ def _simulate_cloned_repo(repo_dir: Path) -> None: (repo_dir / ".git").mkdir(exist_ok=True) -def _make_clone_side_effect(repo_dir: Path) -> Callable[..., str]: - """Return a run_git_command side effect whose clone creates the repo dir.""" +def _make_clone_side_effect( + repo_dir: Path, gitmodules: bool = False +) -> Callable[..., str]: + """Return a run_git_command side effect whose clone creates the repo dir. + + With ``gitmodules`` the cloned repo also declares submodules. + """ def git_command_side_effect( cmd: list[str], cwd: str | None = None, **kwargs: Any ) -> str: if _get_git_command_type(cmd) == "clone": _simulate_cloned_repo(repo_dir) + if gitmodules: + (repo_dir / ".gitmodules").write_text("test") return "" return git_command_side_effect +def _submodule_calls(mock: Mock) -> list[Any]: + """Return the mock's `git submodule` calls.""" + return [ + c for c in mock.call_args_list if _get_git_command_type(c[0][0]) == "submodule" + ] + + +def _assert_submodule_runs_without_isolation(call: Any, repo_dir: Path) -> None: + """Assert a git submodule call ran with plain cwd, not GIT_DIR/GIT_WORK_TREE + isolation, which breaks the submodule porcelain on some installations.""" + assert call.kwargs.get("git_dir") is None + assert call.kwargs.get("cwd") == repo_dir + + def test_run_git_command_success(tmp_path: Path) -> None: """Test that run_git_command returns output on success.""" # Create a simple git repo to test with @@ -100,6 +123,22 @@ def test_run_git_command_success(tmp_path: Path) -> None: assert isinstance(result, str) +def test_run_git_command_debug_log_redacts_credentials( + tmp_path: Path, mock_subprocess_run: Mock, caplog: pytest.LogCaptureFixture +) -> None: + """Embedded URL credentials never reach the debug log; -v output is + routinely pasted into public issues. subprocess is mocked so no real + git ever sees the URL (the path is not creatable on Windows).""" + mock_subprocess_run.return_value = Mock(returncode=0, stdout=b"", stderr=b"") + with caplog.at_level(logging.DEBUG, logger="esphome.git"): + git.run_git_command( + ["git", "clone", "https://user:hunter2@github.com/test/repo"], + cwd=tmp_path, + ) + assert "hunter2" not in caplog.text + assert "://***@github.com/test/repo" in caplog.text + + def test_run_git_command_with_git_dir_isolation( tmp_path: Path, mock_subprocess_run: Mock ) -> None: @@ -116,10 +155,17 @@ def test_run_git_command_with_git_dir_isolation( stderr=b"", ) - result = git.run_git_command( - ["git", "rev-parse", "HEAD"], - git_dir=repo_dir, - ) + # Ambient repo-scoping vars simulate a git hook invoking ESPHome; an + # ambient GIT_INDEX_FILE surviving into a git_dir invocation fails + # silently (git operates on the caller's index and exits 0). + with patch.dict( + os.environ, + {"GIT_INDEX_FILE": "/caller/index", "GIT_OBJECT_DIRECTORY": "/caller/objects"}, + ): + result = git.run_git_command( + ["git", "rev-parse", "HEAD"], + git_dir=repo_dir, + ) # Verify subprocess.run was called assert mock_subprocess_run.called @@ -131,6 +177,9 @@ def test_run_git_command_with_git_dir_isolation( assert "GIT_WORK_TREE" in env assert env["GIT_DIR"] == str(repo_dir / ".git") assert env["GIT_WORK_TREE"] == str(repo_dir) + # The ambient scoping vars must be stripped, not passed through. + assert "GIT_INDEX_FILE" not in env + assert "GIT_OBJECT_DIRECTORY" not in env assert result == "test output" @@ -216,6 +265,89 @@ def test_run_git_command_without_git_dir(mock_subprocess_run: Mock) -> None: assert result == "Cloning into 'test_repo'..." +@pytest.mark.parametrize("relative", [False, True], ids=["absolute", "relative"]) +def test_run_git_command_with_cwd_runs_in_dir_without_isolation( + tmp_path: Path, + mock_subprocess_run: Mock, + monkeypatch: pytest.MonkeyPatch, + relative: bool, +) -> None: + """The cwd parameter sets the working directory without GIT_DIR/GIT_WORK_TREE. + + Ambient GIT_DIR/GIT_WORK_TREE (e.g. from a git hook or CI wrapper) must be + stripped too, and GIT_CEILING_DIRECTORIES must stop git from walking up to + an enclosing repository if the target repo's .git is missing or corrupt. + Git silently ignores a relative ceiling entry, so the variable must come + out absolute even when the given cwd is relative. + """ + repo_dir = tmp_path / "test_repo" + repo_dir.mkdir() + if relative: + monkeypatch.chdir(tmp_path) + cwd_arg = Path("test_repo") + else: + cwd_arg = repo_dir + + mock_subprocess_run.return_value = Mock( + returncode=0, + stdout=b"test output", + stderr=b"", + ) + + with patch.dict( + os.environ, + { + "GIT_DIR": "/ambient/.git", + "GIT_WORK_TREE": "/ambient", + "GIT_INDEX_FILE": "/ambient/.git/index", + }, + ): + result = git.run_git_command(["git", "submodule", "update"], cwd=cwd_arg) + + call_args = mock_subprocess_run.call_args + env = call_args[1]["env"] + assert "GIT_DIR" not in env + assert "GIT_WORK_TREE" not in env + assert "GIT_INDEX_FILE" not in env + ceiling = Path(env["GIT_CEILING_DIRECTORIES"]) + assert ceiling.is_absolute() + assert ceiling.samefile(tmp_path) + assert call_args[1]["cwd"] == cwd_arg + assert result == "test output" + + +def test_run_git_command_raises_on_nonfatal_stderr( + tmp_path: Path, mock_subprocess_run: Mock +) -> None: + """Nonzero exit with stderr lacking a fatal: prefix raises with full stderr.""" + mock_subprocess_run.return_value = Mock( + returncode=1, + stdout=b"", + stderr=b"error: pathspec 'nope' did not match any file(s)\n", + ) + + with pytest.raises(GitCommandError, match="did not match"): + git.run_git_command(["git", "checkout", "nope"], git_dir=tmp_path) + + +def test_run_git_command_raises_on_nonzero_exit_without_stderr( + tmp_path: Path, mock_subprocess_run: Mock +) -> None: + """A nonzero exit must raise even when git printed nothing to stderr. + + Silent nonzero exits were previously treated as success, which is how + broken checkouts could be cached as complete. + """ + mock_subprocess_run.return_value = Mock( + returncode=1, + stdout=b"", + stderr=b"", + ) + + with pytest.raises(GitCommandError, match="exited with code 1"): + git.run_git_command(["git", "submodule", "update"], cwd=tmp_path) + + def test_run_git_command_without_git_dir_raises_error( mock_subprocess_run: Mock, ) -> None: @@ -1156,46 +1288,6 @@ def test_clone_with_ref_uses_shallow_fetch( assert ref in fetch_calls[0][0][0] -def test_clone_with_submodules_uses_shallow_submodule_update( - tmp_path: Path, mock_run_git_command: Mock -) -> None: - """Submodule init on a fresh clone should use --depth=1.""" - CORE.config_path = tmp_path / "test.yaml" - - url = "https://github.com/test/repo" - domain = "test" - repo_dir = _compute_repo_dir(url, None, domain) - - def git_command_side_effect( - cmd: list[str], cwd: str | None = None, **kwargs: Any - ) -> str: - if _get_git_command_type(cmd) == "clone": - repo_dir.mkdir(parents=True, exist_ok=True) - (repo_dir / ".git").mkdir(exist_ok=True) - return "" - - mock_run_git_command.side_effect = git_command_side_effect - - git.clone_or_update( - url=url, - ref=None, - refresh=None, - domain=domain, - submodules=["components/foo"], - ) - - submodule_calls = [ - c for c in mock_run_git_command.call_args_list if "submodule" in c[0][0] - ] - assert len(submodule_calls) == 1 - cmd = submodule_calls[0][0][0] - assert "--depth=1" in cmd - assert "components/foo" in cmd - # The `--` terminator must precede the submodule paths so a path - # beginning with `-` cannot be parsed as an option. - assert cmd.index("--") < cmd.index("components/foo") - - def test_refresh_fetch_is_shallow(tmp_path: Path, mock_run_git_command: Mock) -> None: """The refresh-path fetch should use --depth=1.""" CORE.config_path = tmp_path / "test.yaml" @@ -1220,10 +1312,91 @@ def test_refresh_fetch_is_shallow(tmp_path: Path, mock_run_git_command: Mock) -> assert cmd[-1] == ref -def test_refresh_submodule_update_is_shallow( +@pytest.mark.parametrize( + "refresh", [None, TimePeriodSeconds(days=1)], ids=["clone", "refresh"] +) +def test_all_submodules_skipped_without_gitmodules( + tmp_path: Path, mock_run_git_command: Mock, refresh: TimePeriodSeconds | None +) -> None: + """init_submodules is a no-op for repos with no .gitmodules. + + This is the esp-idf toolchain library scenario from issue #17860: the + PlatformIO library converter requests "all submodules" for every git + library, and most libraries declare none. The git submodule porcelain + must not run at all in that case — it fails outright on some git + installations. + """ + CORE.config_path = tmp_path / "test.yaml" + + url = "https://github.com/test/repo" + domain = "test" + repo_dir = _compute_repo_dir(url, None, domain) + + if refresh is None: + mock_run_git_command.side_effect = _make_clone_side_effect(repo_dir) + else: + _setup_old_repo(repo_dir) + mock_run_git_command.return_value = "abc123" + + git.clone_or_update( + url=url, + ref=None, + refresh=refresh, + domain=domain, + init_submodules=True, + ) + + assert not _submodule_calls(mock_run_git_command) + + +@pytest.mark.parametrize( + "refresh", [None, TimePeriodSeconds(days=1)], ids=["clone", "refresh"] +) +def test_all_submodules_updated_with_gitmodules( + tmp_path: Path, mock_run_git_command: Mock, refresh: TimePeriodSeconds | None +) -> None: + """init_submodules initializes all submodules when .gitmodules exists.""" + CORE.config_path = tmp_path / "test.yaml" + + url = "https://github.com/test/repo" + domain = "test" + repo_dir = _compute_repo_dir(url, None, domain) + + if refresh is None: + mock_run_git_command.side_effect = _make_clone_side_effect( + repo_dir, gitmodules=True + ) + else: + _setup_old_repo(repo_dir) + (repo_dir / ".gitmodules").write_text("test") + mock_run_git_command.return_value = "abc123" + + git.clone_or_update( + url=url, + ref=None, + refresh=refresh, + domain=domain, + init_submodules=True, + ) + + submodule_calls = _submodule_calls(mock_run_git_command) + # Which submodules get populated is git's own policy, so no status + # verification follows the update. + assert len(submodule_calls) == 1 + cmd = submodule_calls[0][0][0] + assert cmd[2] == "update" + assert "--depth=1" in cmd + # Recursive, mirroring PlatformIO's recursive library clones. + assert "--recursive" in cmd + _assert_submodule_runs_without_isolation(submodule_calls[0], repo_dir) + + +def test_recovery_reclone_keeps_credentials_and_cache_key( tmp_path: Path, mock_run_git_command: Mock ) -> None: - """The refresh-path submodule update should use --depth=1.""" + """The recovery re-clone must not re-apply credentials to the already + rewritten URL (no doubled userinfo) and must land in the same cache + directory, or a credentialed private repo re-clones on every run.""" CORE.config_path = tmp_path / "test.yaml" url = "https://github.com/test/repo" @@ -1231,24 +1404,235 @@ def test_refresh_submodule_update_is_shallow( repo_dir = _compute_repo_dir(url, None, domain) _setup_old_repo(repo_dir) - mock_run_git_command.return_value = "abc123" + (repo_dir / ".gitmodules").write_text("test") - git.clone_or_update( + calls = {"submodule": 0} + + def git_command_side_effect( + cmd: list[str], cwd: str | None = None, **kwargs: Any + ) -> str: + if _get_git_command_type(cmd) == "clone": + _simulate_cloned_repo(repo_dir) + if _get_git_command_type(cmd) == "submodule": + calls["submodule"] += 1 + if calls["submodule"] == 1: + raise git.GitCommandError("git submodule update exited with code 1") + return "" + + mock_run_git_command.side_effect = git_command_side_effect + + recovered_dir, _ = git.clone_or_update( url=url, ref=None, refresh=TimePeriodSeconds(days=1), domain=domain, - submodules=["components/foo"], + username="user", + password="hunter2", + init_submodules=True, ) - submodule_calls = [ - c for c in mock_run_git_command.call_args_list if "submodule" in c[0][0] + assert recovered_dir == repo_dir + clone_cmds = [ + c[0][0] + for c in mock_run_git_command.call_args_list + if _get_git_command_type(c[0][0]) == "clone" ] - assert len(submodule_calls) == 1 - cmd = submodule_calls[0][0][0] - assert "--depth=1" in cmd - assert "components/foo" in cmd - assert cmd.index("--") < cmd.index("components/foo") + assert clone_cmds + clone_url = clone_cmds[0][-2] + assert clone_url == "https://user:hunter2@github.com/test/repo" + assert clone_url.count("@") == 1 + + +def test_refresh_submodule_failure_recovers_then_raises( + tmp_path: Path, mock_run_git_command: Mock +) -> None: + """A refresh-path submodule failure routes through the recovery re-clone. + + The broken repo is removed and re-cloned; when the submodule update fails + again on the fresh clone the cache entry is removed and the error + propagates, instead of leaving behind a repo the refresh window would + silently accept on the next run. + """ + CORE.config_path = tmp_path / "test.yaml" + + url = "https://github.com/test/repo" + domain = "test" + repo_dir = _compute_repo_dir(url, None, domain) + + _setup_old_repo(repo_dir) + (repo_dir / ".gitmodules").write_text("test") + + def git_command_side_effect( + cmd: list[str], cwd: str | None = None, **kwargs: Any + ) -> str: + if _get_git_command_type(cmd) == "clone": + _simulate_cloned_repo(repo_dir) + (repo_dir / ".gitmodules").write_text("test") + if _get_git_command_type(cmd) == "submodule": + raise git.GitCommandError("git submodule update exited with code 1") + return "" + + mock_run_git_command.side_effect = git_command_side_effect + + with pytest.raises(git.GitCommandError, match="exited with code 1"): + git.clone_or_update( + url=url, + ref=None, + refresh=TimePeriodSeconds(days=1), + domain=domain, + init_submodules=True, + ) + + assert not repo_dir.is_dir() + # Recovery removed the repo and re-cloned before failing again. + assert any( + _get_git_command_type(c[0][0]) == "clone" + for c in mock_run_git_command.call_args_list + ) + + +def _real_git(*args: str, cwd: Path) -> None: + """Run real git to build a test fixture repository.""" + subprocess.run( + [ + "git", + "-c", + "user.email=test@test.invalid", + "-c", + "user.name=test", + "-c", + "commit.gpgsign=false", + "-c", + "protocol.file.allow=always", + *args, + ], + cwd=cwd, + check=True, + capture_output=True, + ) + + +# Git blocks file-protocol submodules by default (CVE-2022-39253); the e2e +# tests allow them via GIT_CONFIG_* environment variables, which reach the +# child git processes through run_git_command's filtered environment. +_ALLOW_FILE_PROTOCOL_ENV = { + "GIT_CONFIG_COUNT": "1", + "GIT_CONFIG_KEY_0": "protocol.file.allow", + "GIT_CONFIG_VALUE_0": "always", +} + + +def _make_real_repo(path: Path, filename: str) -> None: + """Create a real git repository containing one committed file.""" + path.mkdir() + _real_git("init", "-q", cwd=path) + (path / filename).write_text("content") + _real_git("add", filename, cwd=path) + _real_git("commit", "-q", "-m", "init", cwd=path) + + +def _add_submodule( + repo: Path, url: Path, path: str, *, update_none: bool = False +) -> None: + """Add ``url`` as a submodule of ``repo`` at ``path`` and commit it.""" + _real_git("submodule", "add", str(url), path, cwd=repo) + if update_none: + _real_git( + "config", "-f", ".gitmodules", f"submodule.{path}.update", "none", cwd=repo + ) + _real_git("add", ".gitmodules", cwd=repo) + _real_git("commit", "-q", "-m", f"add submodule {path}", cwd=repo) + + +def test_clone_or_update_real_git_without_submodules(tmp_path: Path) -> None: + """End-to-end with real git: a repo with no .gitmodules clones cleanly. + + This is the issue #17860 scenario: requesting "all submodules" on a + submodule-less repository must not invoke the git submodule porcelain + and must produce a usable checkout. + """ + CORE.config_path = tmp_path / "test.yaml" + + upstream = tmp_path / "upstream" + _make_real_repo(upstream, "README.md") + + repo_dir, _ = git.clone_or_update( + url=str(upstream), + ref=None, + refresh=None, + domain="test_e2e", + init_submodules=True, + ) + + assert (repo_dir / "README.md").is_file() + + +def test_clone_or_update_real_git_initializes_submodules(tmp_path: Path) -> None: + """End-to-end with real git: submodules are actually checked out. + + Exercises the real `git submodule update` invocation, including the + env handling in run_git_command that the mocked tests cannot cover. + """ + CORE.config_path = tmp_path / "test.yaml" + + sub_repo = tmp_path / "sub" + _make_real_repo(sub_repo, "sub_file.txt") + + upstream = tmp_path / "upstream" + _make_real_repo(upstream, "README.md") + _add_submodule(upstream, sub_repo, "vendor/sub") + + with patch.dict(os.environ, _ALLOW_FILE_PROTOCOL_ENV): + repo_dir, _ = git.clone_or_update( + url=str(upstream), + ref=None, + refresh=None, + domain="test_e2e", + init_submodules=True, + ) + + assert (repo_dir / "vendor" / "sub" / "sub_file.txt").is_file() + + +def test_clone_or_update_real_git_honors_update_none_submodule( + tmp_path: Path, +) -> None: + """End-to-end with real git: submodules declared `update = none` stay skipped. + + Shows git itself skipping the declared paths at both nesting levels + (and exiting 0) while the regular submodules check out. + """ + CORE.config_path = tmp_path / "test.yaml" + + sub_repo = tmp_path / "sub" + _make_real_repo(sub_repo, "sub_file.txt") + + # Intermediate submodule that itself declares a skipped nested submodule. + mid_repo = tmp_path / "mid" + _make_real_repo(mid_repo, "mid_file.txt") + _add_submodule(mid_repo, sub_repo, "vendor/leaf", update_none=True) + + upstream = tmp_path / "upstream" + _make_real_repo(upstream, "README.md") + _add_submodule(upstream, sub_repo, "vendor/sub") + _add_submodule(upstream, sub_repo, "vendor/skipped", update_none=True) + _add_submodule(upstream, mid_repo, "vendor/mid") + + with patch.dict(os.environ, _ALLOW_FILE_PROTOCOL_ENV): + repo_dir, _ = git.clone_or_update( + url=str(upstream), + ref=None, + refresh=None, + domain="test_e2e", + init_submodules=True, + ) + + assert (repo_dir / "vendor" / "sub" / "sub_file.txt").is_file() + assert not (repo_dir / "vendor" / "skipped" / "sub_file.txt").exists() + assert (repo_dir / "vendor" / "mid" / "mid_file.txt").is_file() + assert not ( + repo_dir / "vendor" / "mid" / "vendor" / "leaf" / "sub_file.txt" + ).exists() def test_refresh_picks_up_new_remote_commits(